mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 00:26:31 +00:00
8eeb510097
* feat(acp-bridge): F3 — multi-client permission coordination (#4175) [rebased onto F1]
Squashed F3 implementation rebased from origin/main onto
daemon_mode_b_main (post-F1 #4319). F1 lifted the bridge core to
@qwen-code/acp-bridge package; F3's edits to the pre-F1
httpAcpBridge.ts BridgeClient class + factory were ported to the
new file locations:
- BridgeClient.requestPermission rewrite → bridgeClient.ts
- Factory mediator construction / pendingPermissions deletion /
cancelPendingForSession refactor / respondTo*Permission
rewrites / pendingPermissionCount + permissionPolicy getters /
teardown sites (closeSession, killSession, shutdown drain)
→ bridge.ts
- Error class re-exports → cli/src/serve/httpAcpBridge.ts shim
(added CancelSentinelCollisionError, PermissionForbiddenError,
PermissionPolicyNotImplementedError to the F1 re-export block)
This commit folds 13 logical F3 commits + 4 review fold-ins (Copilot
inline comments + 3 final-pass agent reviews) into a single
post-rebase squash. The full review trail is in
.claude/plans/fluttering-coalescing-kettle*.md (worktree-local).
Strategies (4): first-responder (default, byte-for-byte preserved),
designated, consensus (default N=floor(M/2)+1), local-only.
New SSE events: permission_partial_vote, permission_forbidden.
Capability tag: permission_mediation (always-on with build-supported
modes list); active policy at /capabilities.policy.permission.
Settings: policy.permissionStrategy enum + policy.consensusQuorum
number, both requiresRestart: true (F3 v1 reads at boot).
3 new typed errors: PermissionForbiddenError → 403,
PermissionPolicyNotImplementedError → 501 (forward-compat for future
policy literals), CancelSentinelCollisionError → 500 (agent / daemon
contract violation).
Hardness invariants: N1 synchronous-register, N2 cleanup ordering,
N3 originatorClientId stamping, O5 cancel sentinel pre-publish
collision check, O8 pre-F3 permission_resolved wire shape preserved.
Tests: 35 mediator unit + 10 audit ring + 56 SDK reducer + 6
bridgeClient + 3 bridge integration. Pre-existing
httpAcpBridge.test.ts cross-session-vote suite passes byte-for-byte.
Issue: #4175 (F3)
* fix(f3): build/capability fixes from Copilot review (#4335)
- packages/sdk-typescript/src/daemon/index.ts: re-export the four F3
permission event types (`DaemonPermissionForbiddenData/Event`,
`DaemonPermissionPartialVoteData/Event`) so the public package barrel
at `src/index.ts` (which forwards them via `from './daemon/index.js'`)
resolves at build time. Without this fix `npm run build
--workspace=packages/sdk-typescript` failed with TS2305/TS2724;
vitest passed only because it resolves TS source via tsx and
bypasses tsc compilation. Reported in PR #4335 review comments
3270615836 / 3270622302 (wenshao via Qwen Code /review).
- packages/cli/src/serve/server.test.ts: append `'permission_mediation'`
to `EXPECTED_STAGE1_FEATURES` and adjust `EXPECTED_REGISTERED_FEATURES`
reordering so the test fixture matches the registry's actual order
(`...workspace_mcp_restart, require_auth, auth_device_flow,
permission_mediation`). Without this fix four `serve capability
registry` tests asserted via `.toEqual` against a stale list.
- docs/developers/qwen-serve-protocol.md: swap `permission_mediation`
and `auth_device_flow` in the documented capability list so the order
mirrors `SERVE_CAPABILITY_REGISTRY` declaration order.
- packages/vscode-ide-companion/schemas/settings.schema.json: regenerate
the IDE-companion JSON schema with the new `policy` section (was
pending from Commit 5 of the F3 series; checked in here so the
IDE companion sees the same `permissionStrategy` / `consensusQuorum`
shape that the CLI accepts).
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): wire production audit ring + restore timeout stderr (#4335)
Wenshao review #4335 surfaced two related Critical findings:
1. **Audit publisher silently no-op in production** (3270622298). The
`bridgeOptions.ts:305` JSDoc claimed "the bridge allocates an
internal `PermissionAuditRing`" but the actual fallback at
`bridge.ts:543` is `createNoOpPermissionAuditPublisher()`, and
`runQwenServe.ts` never wired one. All 5 audit record types
(`requested`, `voted`, `forbidden`, `resolved`, `timeout`) were
silently discarded — the forensic audit trail the F3 plan
committed to ("ring 留给后续 PR 加查询接口") never existed in
any deployed daemon.
2. **Timeout breadcrumb lost** (3270622304). Pre-F3 wrote
`"timed out after Xms"` to daemon stderr on every permission
timeout. F3 removed that direct write and delegated to
`audit.recordTimeout()`, but the audit publisher is the no-op
fallback in production (see #1). Operators tailing daemon
stderr could no longer observe permission timeouts.
Fixes:
- `runQwenServe.ts` allocates a `PermissionAuditRing` (default cap 512)
+ `createPermissionAuditPublisher` and passes the publisher via
`BridgeOptions.permissionAudit`. The ring is held in the daemon
host's closure for the lifetime of the daemon — a future
`GET /workspace/permission/audit` route (out of F3 v1 scope) can
lift it out for query without further bridge changes.
- `permissionMediator.ts` writes the stderr breadcrumb directly from
the timer callback, before forwarding to the (potentially no-op)
audit publisher. Wrapped in try/catch because `process.stderr.write`
can synchronously throw on EPIPE — losing observability is
preferable to crashing the timer queue.
- `bridgeOptions.ts` JSDoc rewritten to match reality: the bridge
falls back to a no-op publisher; production wiring lives in
`runQwenServe.ts`; the stderr breadcrumb is in the mediator
(independent of the publisher).
- New unit test `writes a stderr breadcrumb when the timer fires`
spies on `process.stderr.write` and asserts the breadcrumb format
contains the requestId, sessionId, and the timeout duration so
future refactors can't silently drop the line again.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): drop dead helper + propagate originator to F3 view state (#4335)
Two small follow-ups from wenshao review #4335:
- **`bridge.ts:672-682` — dead `_resolutionToAcpResponse` helper**
(3270622309). Defined and immediately suppressed with `void`. The
identical `resolutionToAcpResponse` lives at `bridgeClient.ts:41`
and is the one actually used by `BridgeClient.requestPermission`
— the bridge-factory copy was a stranded leftover from the lift
out of inline closures into the mediator pattern. Removed
declaration, `void` statement, and the now-unused
`RequestPermissionResponse` (`@agentclientprotocol/sdk`) and
`PermissionResolution` (`./permission.js`) imports.
- **SDK reducer `mergeOriginator` for F3 events** (3270622311). The
mediator stamps `originatorClientId` (= prompt originator per
N3) on the `permission_partial_vote` / `permission_forbidden`
envelope, but the reducer cases used `next.push({ ...event.data })`
which only copies `data` fields. SDK consumers reading
`permissionVoteProgress[reqId]` / `forbiddenVotes[i]` could not
determine which client's prompt was targeted by the partial-vote
progress / forbidden vote — same gap PR #4282 fixed for
approval-mode / tool-toggle / workspace-init / mcp-restart.
Applied the existing `mergeOriginator` helper to both reducer
cases. Added `originatorClientId?: string` to both Data
interfaces with JSDoc explaining the propagation contract
(preserve any pre-existing `data.originatorClientId`; otherwise
stamp from the envelope; for forbidden votes the field is
distinct from `data.clientId` which carries the rejected voter).
Three new reducer tests:
1. `permission_partial_vote` propagates envelope originator into
`permissionVoteProgress`.
2. `permission_forbidden` propagates envelope originator into
`forbiddenVotes`, distinct from `data.clientId`.
3. `mergeOriginator` preserves any pre-existing
`data.originatorClientId` over the envelope value.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): wenshao Round 4 — defensive stderr, audit accuracy, orphan cleanup (#4335)
Four findings from wenshao review #4324937255 — the Critical one
masked an actual hang scenario; the other three are observability /
correctness fixes that round out F3 v1.
**[Critical] safeEmit / safeAudit stderr breadcrumb wraps** (3271041461).
Both helpers wrote `process.stderr.write` inside their `catch` block
WITHOUT a nested `try/catch`. If stderr itself synchronously throws
(EPIPE during daemon shutdown), the exception escapes the "safe"
wrapper. In `resolveEntry`'s cleanup ladder
(`safeEmit → rememberResolved → safeAudit → pending.resolve`), an
escaping safeEmit exception aborts before `pending.resolve(resolution)`
runs — the request was already deleted from `this.pending` (no
double-resolve guard), so the agent's awaiting Promise never
settles. `requestPermission` hangs until the timeout fires. The
timer callback already wraps its breadcrumb in `try/catch` for the
same reason — applied the matching pattern to safeEmit + safeAudit.
**[Suggestion] Idempotent re-vote audit shows attempted optionId,
not the original** (3271041464). When `client_A` originally voted
for `proceed_once` and later attempts `proceed_always`, the tally
silently keeps `proceed_once` (idempotent) but the audit ring
recorded `optionId: proceed_always`. An operator reading the ring
would see a vote for proceed_always that never counted toward
quorum. Look up the originally-voted option from the tally and
substitute it into the audit record. Added regression test
asserting the audit reflects tally state.
**[Suggestion] SDK reducer leaks `permissionVoteProgress` on
mid-permission reconnect** (3271041465). When an SDK client
reconnects and misses `permission_request`, then receives
`permission_partial_vote` (stored in `permissionVoteProgress`),
then receives `permission_resolved` — the early-return path on
unmatched `requestId` did NOT clear `permissionVoteProgress`. The
orphan progress entry persisted until session end. Both
`permission_resolved` and `permission_already_resolved` reducer
cases now unconditionally clear any orphan entry on the unmatched
path. Two new reducer tests cover the recovery contract; the
misleading "the next `permission_resolved` will clear both"
comment on `permission_partial_vote` is corrected.
**[Suggestion] Document votersAtIssue snapshot timing window**
(3271041469). The snapshot fires synchronously after
`entry.events.publish`, with no event-loop yield between, so a NEW
HTTP client cannot register between publish and snapshot. But an
SSE-only subscriber (no `X-Qwen-Client-Id` registered yet) that
connected BEFORE publish is invisible to the snapshot — `consensus`
silently rejects its later vote as `forbidden`. Documented the
window in `votersForSession` JSDoc; future PRs surfacing
`eligibleVoters[]` on `permission_request.data` should source it
from the same snapshot for consistency. No code change — the
narrow window is acceptable for F3 v1, and the structural fix
(snapshot at publish time) requires bridge-level refactor.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): wenshao Round 5 — sentinel injection guard, observability, /8 loopback (#4335)
Four findings from wenshao review #4325130053. The Critical one is
a real security gap; the others are observability + correctness
hardening.
**[Critical] Cancel sentinel injection bypass** (3271185588). The
mediator's `vote()` recognizes `CANCEL_VOTE_SENTINEL` BEFORE
validating the option against `allowedOptionIds`, so a wire client
sending `{outcome:'selected', optionId:'__cancelled__'}` would
short-circuit ALL policy dispatch (designated originator check,
consensus quorum, local-only loopback gate). The mediator's JSDoc
documented the precondition ("callers MUST NOT forward an
incoming vote.optionId === CANCEL_VOTE_SENTINEL from a wire
client") but the precondition was never enforced — the bridge's
`respondToSessionPermission` mapped the wire optionId straight
through. Added an explicit `InvalidPermissionOptionError` throw
when the wire payload is `{selected, CANCEL_VOTE_SENTINEL}`. The
collision-defense at request issue time
(`CancelSentinelCollisionError`) already prevents agents from
advertising the sentinel as a legitimate option; this closes the
remaining vector.
**[Suggestion] Silent quorum cap + M=0 hang observability**
(3271185594). Two related diagnostic gaps in the consensus
policy:
- When `policy.consensusQuorum` exceeds `votersAtIssue.size`, the
cap fires silently. Operators investigating "why did consensus
resolve at N=2 when I configured 5?" had no breadcrumb.
- When `policy === 'consensus'` and `votersAtIssue.size === 0`,
every vote rejects as `forbidden: designated_mismatch` because
the empty snapshot can never match any voter clientId. The
request hangs until `permissionTimeoutMs` with no
diagnostic signal.
Added stderr breadcrumbs at both points: cap-applied (once per
request via a `consensusQuorumCapNoted` flag on `MediatorPending`)
and at issue time when consensus M=0. No semantic change — the
cap and the timeout-only resolution behavior are intentional per
the F3 plan; the breadcrumbs just make them debuggable.
**[Suggestion] detectFromLoopback misses 127.0.0.0/8** (3271185597).
Per RFC 1122 the entire `127.0.0.0/8` block is loopback. The
exact-match Set of three literals (`127.0.0.1`, `::1`,
`::ffff:127.0.0.1`) silently fail-CLOSED on legitimate
`127.0.0.2` / `127.0.1.1` / `::ffff:127.0.0.2` peers, causing
unexpected `remote_not_allowed` rejections under `local-only`
policy. Switched to a prefix test so the entire `/8` and its
dual-stack mirror are accepted. Direction stays fail-CLOSED for
unrecognized address shapes.
**[Suggestion] VSCode JSON schema integer/min validation**
(3271185604). `runQwenServe.ts` validates
`Number.isInteger(consensusQuorum) && >= 1`, but the generated
`settings.schema.json` declared `"type": "number"` so VSCode's
inline JSON Schema validation accepted `0` / `-1` / `1.5` and
the user only learned the value was invalid on the next daemon
restart. Added `jsonSchemaOverride: {type:'integer', minimum:1}`
to the `consensusQuorum` settings entry and regenerated the
schema. IDE editors now flag invalid values immediately.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): Round 6 — wenshao APPROVED + DeepSeek follow-ups (#4335)
Mixed batch: bridge-test backfill from wenshao's APPROVED review
plus 4 DeepSeek/v4-pro suggestions and the 3 typecheck/test
blockers DeepSeek named in CHANGES_REQUESTED #4325674833.
**Pre-merge blockers (DeepSeek #4325674833 body)**
- `server.test.ts:529` `FakeBridge` — added the F3-required
`permissionPolicy: 'first-responder' as const`. Tests don't
exercise mediation; the literal pins the pre-F3 default so
existing assertions stay shape-compatible.
- `server.test.ts:3994` `WorkspaceFileSystemFactory.forRequest()`
mock — added the missing `writeTextOverwrite` method that PR
#4334 introduced on `WorkspaceFileSystem` after this branch
forked.
- 4 vote-context test failures from `fromLoopback` plumbing —
updated the four `expect(...).toEqual(...)` assertions in
`POST /session/:id/permission/:requestId` and
`POST /permission/:requestId` to include `fromLoopback: true`
on the captured context. The supertest peer is `127.0.0.1`,
so `detectFromLoopback(req)` correctly stamps the field; the
pre-F3 expected shape was stale.
**Inline suggestions adopted**
- **3271420267** (wenshao APPROVED, security-critical) — added
bridge-level test `rejects cancel sentinel injection via
{selected,'__cancelled__'}` in `httpAcpBridge.test.ts`. Without
it, a future refactor could silently remove the wire-injection
guard that closes the policy-bypass attack surface introduced
in Round 5 (#3271185588). Required `npm run build
--workspace=packages/acp-bridge` to refresh `dist/` before
vitest picked up the F3 bridge.ts changes; documented for
future contributors editing F3 acp-bridge code.
- **3271627444** (DeepSeek) — `request()` JSDoc rewritten to
drop "Promise contract — never rejects" without qualification.
The `CancelSentinelCollisionError` synchronous throw is real
and intentional (a never-settling Promise alongside a thrown
error is worse than fail-fast), but callers must be aware of
it. Updated the contract doc to call out the sync-throw
exception explicitly and documented that async callers get
the throw via their own Promise machinery.
- **3271627446** (DeepSeek) — fixed "Bounded LRU" comment on
`MAX_RESOLVED_PERMISSION_RECORDS` to "Bounded FIFO" since
`rememberResolved` uses `resolvedOrder.shift()` (drop oldest).
Mirrors the parallel `PermissionAuditRing` correction in
commit b0242ddec.
- **3271627457** (DeepSeek) — added stderr breadcrumbs to all 3
forbidden-vote sites (voteDesignated / voteConsensus /
voteLocalOnly). Audit ring is in-memory only (no v1 query
route), SSE events are transient — operators tailing daemon
stderr previously had zero indication of permission rejections.
New `writeForbiddenStderr` helper centralizes the formatting
+ try/catch defensive posture (mirrors the timeout breadcrumb
pattern from Round 4).
- **3271627459** (DeepSeek) — added a `TODO(forward-compat)`
comment at `voteConsensus`'s rejection site documenting the
`designated_mismatch` reason-code overload. The same wire
string covers two distinct semantic cases: "voter is not the
prompt originator" (designated policy) and "voter not in
consensus votersAtIssue snapshot" (consensus). Splitting them
into distinct codes is deferred to a future PR once an SDK
consumer needs to disambiguate.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): Round 7 — error precedence + 7 hardening fixes from wenshao (#4335)
8 findings from wenshao Round 7. The Critical one closes a session-
existence information leak; 6 Suggestions improve observability,
type safety, and test coverage; 1 documents the cancel-sentinel
escape hatch in the local-only setting description.
**[Critical] Error precedence regression in respondToSessionPermission**
(3271978329). When `peekSessionFor(requestId)` returned `undefined`
(timed out / LRU-evicted / never registered), the cross-session
guard at line 2033 didn't fire (`!== undefined` skips it), so
execution fell through to `resolveTrustedClientId` which throws
`InvalidClientIdError` (HTTP 400) when the caller's clientId
isn't registered. Pre-F3 returned `false` (HTTP 404) for unknown
requestIds regardless of clientId validity. Without the explicit
guard, a probe with a fabricated clientId could distinguish
"session exists with these registered clients" (400) from "no
such request" (404). Added an explicit `actualSessionId ===
undefined → return false` short-circuit BEFORE the clientId
validation. The defensive `unknown_request` switch case below
becomes unreachable in practice; left in place for defense-in-depth.
**[Suggestion] Cancel sentinel cross-policy escape hatch under
`local-only`** (3271978336). Documented in `voteLocalOnly` JSDoc
and the settings description that a remote voter can ABORT a
pending permission via `{outcome:'cancelled'}` even though they
cannot RESOLVE one. The F3 plan calls this out as intentional
(cross-policy cancel for consistency with first-responder /
designated / consensus); operators wanting strict-cancel-too need
a dedicated loopback-bound daemon. Doc-only — semantic change
deferred.
**[Suggestion] CapabilitiesEnvelope.policy.permission widens
silently** (3271978342). Replaced the inlined string-literal
union with `import type { PermissionPolicy } from
'@qwen-code/acp-bridge'`. Adding a 5th policy upstream would now
trigger a compile error here instead of silently accepting the
narrower set.
**[Suggestion] M=2 unanimity surprise** (3271978356). Default
quorum `floor(M/2)+1` requires unanimity for even M (M=2 →
quorum=2; both voters must agree). An operator picking
`consensus` with two clients expecting "majority of 2 = 1" gets
unanimity instead — a split vote silently hangs until
`permissionTimeoutMs`. Added stderr breadcrumb at issue time
when the default formula yields unanimity (M ≥ 2 and floor(M/2)+1
== M). Mirrors the existing M=0 / cap-applied breadcrumbs added
in Round 5. Formula stays unchanged (true majority for all M is
mutually exclusive with M=1 → quorum=1). Description in the
settings schema also calls out the M=2 case explicitly.
**[Suggestion] Cancel sentinel adversarial test gap**
(3271978359). The existing "resolves cancelled regardless of
policy" test used the originator under designated and a
votersAtIssue voter under consensus — those would be ACCEPTED by
the policies even without the sentinel bypass. Added two
adversarial tests that pin the cross-policy escape hatch:
non-originator voter under designated and not-in-snapshot voter
under consensus.
**[Suggestion] BridgeClient pre-publish collision test gap**
(3271978365). `bridgeClient.requestPermission` throws
`CancelSentinelCollisionError` BEFORE publishing the SSE
`permission_request` to prevent orphan events (the mediator-level
collision check in `mediator.request` happens too late if publish
goes first). Added test asserting the throw + asserting publish
was NOT called + asserting `pendingPermissionIds` was NOT
incremented.
**[Suggestion] Settings descriptions missing security caveats**
(3271978370). Added explicit caveats to `permissionStrategy`
description: (a) `designated` notes that client identity is
self-declared with no proof-of-possession (impersonation by
observing originatorClientId on SSE frames is possible); (b)
`local-only` notes the cancel-sentinel cross-policy escape hatch.
Schema regenerated to `vscode-ide-companion/schemas/settings.schema.json`.
**[Suggestion] Boot validation error class** (3271978374). Replaced
`err.message.includes('invalid policy.')` substring matching with
a dedicated `InvalidPolicyConfigError` class checked via
`instanceof`. A future reworded validation message would have
silently downgraded operator misconfiguration to "fall back to
defaults" under the previous fragile match.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): Round 8 — close legacy clientId oracle + 5 hardening fixes (#4335)
6 follow-up findings from wenshao Round 8 review #4326742064 (state:
COMMENTED — not blocking but addresses leftover risk surfaces).
**[Suggestion] Legacy `respondToPermission` info leak** (3272493777).
Round 7 closed the cross-session client-registration oracle on the
session-scoped vote route, but the legacy workspace-level route
(`POST /permission/<requestId>`) still called
`resolveAnyTrustedClientId` on unknown-requestId paths, throwing
`InvalidClientIdError` (400) for unregistered clientIds and
returning false (404) for registered ones — the same oracle. The
PR #4231 reasoning ("preserve security boundary") was inverted:
the 400-vs-404 distinction WAS the leak. Removed the call,
deleted the now-unused `resolveAnyTrustedClientId` helper, and
updated the previously-leak-asserting test (`rejects unknown
permission votes with unregistered client ids`) to assert the
new uniform `false` behavior across all 3 input shapes
(unregistered / registered / no-clientId).
**[Suggestion] Error-precedence regression test gap +
observability inconsistency** (3272493792). Two parts:
- Added regression test `returns false (not InvalidClientIdError)
when session exists but requestId is unknown and clientId is
unregistered` to lock the Round-7 fix against future refactors.
- Promoted the error-precedence guard's stderr line from
debug-gated `writeServeDebugLine` to unconditional
`writeStderrLine`, matching the `writeForbiddenStderr` posture
in the mediator. Operators tailing stderr at 3 AM no longer
need `QWEN_SERVE_DEBUG=1` to see unexpected 404s on the
permission endpoint.
**[Suggestion] Settings description "UNANIMITY for even M" was
factually wrong** (3272493795). `floor(M/2)+1` equals M only when
M=2; for M=4 it gives 3 (supermajority), M=6 gives 4 (~67%).
The mediator's own unanimity warning correctly fires only when
M=2. Settings description now reads "UNANIMITY for M=2 (quorum=2,
both must agree) and supermajority for larger even M (M=4 →
quorum=3; M=6 → quorum=4)". VSCode JSON schema regenerated.
**[Suggestion] runQwenServe.ts inline policy unions** (3272493805).
Same drift-protection rationale as the types.ts fix in Round 7.
Imported `PermissionPolicy` from `@qwen-code/acp-bridge`,
replaced 3 inline unions: the `let` declaration, the `as` cast,
and the `VALID_PERMISSION_POLICIES` Set construction. Used a
typed-array + Set<string> pattern (drift caught at array
construction; runtime Set keeps `.has(string)` ergonomics).
**[Suggestion] InvalidPolicyConfigError discrimination needs
positive tests** (3272493818). Extracted the inline
`policyConfig`-validation logic into an exported
`validatePolicyConfig(policyConfig, onWarning?)` helper and
exported `InvalidPolicyConfigError` itself. Added 7 unit tests
covering: empty config, all 4 valid literals, invalid literal
throws (with class identity check + message regex), 4
non-positive-integer quorum cases throw, valid combination
returns, mismatch (consensusQuorum + non-consensus strategy)
emits warning without throwing, no-warning happy path, and
error messages name the failed field. The boot path in
`runQwenServe` now delegates to the helper (one call site,
DRY).
**[Suggestion] Unanimity breadcrumb spammed per-request**
(3272493829). The Round-7 unanimity stderr line fires inside the
synchronous Promise executor of every `request()` call, which
for a 2-client consensus session is EVERY permission request (M=2
unanimity is the normal operating mode, not a rare edge). Added
`unanimityBreadcrumbEmitted` boolean to the mediator class
(per-mediator dedup, parallel to `consensusQuorumCapNoted` on
`MediatorPending`). One emit per daemon lifetime — visible at
boot, silent thereafter. Comment also corrects the "for even M"
generalization to "for M=2" specifically, matching the actual
condition (`floor(M/2)+1 === M` only for M=1 and M=2).
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): Round 9 — terminal-event forbidden cleanup + 7 hardening fixes (#4335)
8 follow-up findings from wenshao Round 9 (4 separate review
records: 4326832742 / 4326833568 / 4326844430 / 4326851074, the
last one a non-blocking comment review). 1 Critical + 7 Suggestions.
**[Critical] Terminal events leaked forbiddenVotes history**
(3272576003). `session_died` / `session_closed` / `client_evicted`
/ `stream_error` reducer cases cleared `pendingPermissions` and
`permissionVoteProgress` but not `forbiddenVotes` /
`forbiddenVoteCount`. Adapters reading view state for a dead
session would render stale rejection data. All 4 cases now
zero out the rejection ring + counter. Parameterized regression
test asserts the cleanup contract.
**[Suggestion] safeAudit JSDoc was orphaned over
writeForbiddenStderr** (3272567323). Two consecutive JSDoc
blocks were stacked back-to-back but the method definitions
followed in the opposite order, so IDE hover and API doc
generation showed `safeAudit`'s docs as `writeForbiddenStderr`'s.
Reordered method definitions so each JSDoc precedes its actual
method.
**[Suggestion] writeForbiddenStderr had no test coverage**
(3272568031). Added a 3-path test (designated / consensus /
local-only) that spies on `process.stderr.write` and asserts each
breadcrumb contains the expected reason fragment plus the
requestId + sessionId for grep-ability. Pins the format so a
future refactor can't silently drop the line.
**[Suggestion] resolveEntry numbered list contradicted code**
(3272581553). The N2-invariant cleanup ladder docstring bundled
"delete from pending + write to resolved" into step 2 ahead of
the SSE emit, but the actual code defers `rememberResolved`
until AFTER `safeEmit` (the I5 inline comment on line 1103
correctly explains this). Split step 2 into two halves around
the emit so the spec faithfully describes the ordering invariant.
**[Suggestion] Dead exports in bridgeClient.ts** (3272581548).
`MAX_RESOLVED_PERMISSION_RECORDS`, `PendingPermission`, and
`PermissionResolutionRecord` were defined and exported but no
longer referenced — the mediator owns the same state under
different names (`permissionMediator.ts:77` / `:319`). The
JSDoc still pointed at deleted closures (`registerPending`,
`resolvedPermissions` map). Removed all three definitions and
the matching re-exports in `cli/src/serve/httpAcpBridge.ts`.
**[Suggestion] detectFromLoopback prefix-match had no direct test**
(3272581557). Supertest in the broader server.test.ts suite
always connects from `127.0.0.1`, so the Round-5 prefix-match
fix for `127.x`-beyond-`.0.0.1`, `::1`, `::ffff:127.*`, and the
fail-closed branches had no coverage. Exported the helper from
`server.ts` (loosened parameter type to a minimal shape so tests
don't need to spin up Express) and added an `it.each` table
covering the variants the fix targets, plus an explicit "does
NOT consult X-Forwarded-For" assertion as a security pin.
**[Suggestion] Validate-policies set is a 4th hardcoded copy**
(3272581563). The policy literals already exist in 3 places —
`PermissionPolicy` type, `SERVE_CAPABILITY_REGISTRY.permission_
mediation.modes`, and `settingsSchema.ts` enum options.
`validatePolicyConfig` now derives its valid-set from
`SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (single
runtime source of truth). Adding a 5th policy upstream lands in
one place; a future drift between the registry and the type
union would still surface at the `as PermissionPolicy` cast.
**[Suggestion] BridgeClient over-coupled to MultiClientPermissionMediator**
(3272581569). `BridgeClient` only ever calls `mediator.request()`
but its field was typed as the concrete class, forcing every
test stub to fake all 6 mediator members. Narrowed the field type
to `Pick<PermissionMediator, 'request'>` (the frozen interface
from `permission.ts`); the bridge factory still passes the full
`MultiClientPermissionMediator` instance via structural typing.
Test stubs simplified from 6 placeholder members to 1.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(f3): Round 10 — wenshao APPROVED + 3 final polish (#4335)
wenshao APPROVED the PR (review 4327485978: "No issues found in
the latest Round 9 changes... LGTM ✅") with 3 minor follow-up
suggestions in a separate COMMENTED review (4327443147). All
adopted; the 4th suggestion (3273077262) was already addressed
in Round 9.
**[Suggestion] Symmetric stderr breadcrumb on legacy
respondToPermission** (3273077256). The session-scoped sibling
already writes an unconditional `writeStderrLine` on its
`actualSessionId === undefined` rejection path (Round 8 /
3272493792); the legacy `POST /permission/<id>` route returned
`false` silently after the Round-8 oracle removal, leaving an
observability gap. Added matching `writeStderrLine`. Operators
tailing stderr at 3 AM now see legacy-route 404s without needing
QWEN_SERVE_DEBUG=1.
**[Suggestion] consensusQuorum contract mismatch** (3273077270).
The warning text told the operator "the override will be
ignored" but the function still propagated `permissionConsensusQuorum`
to BridgeOptions. The downstream mediator only reads it under
the consensus policy, so behavior was correct — but the public
contract contradicted itself. Adopt option (a): drop the value
to `undefined` when the strategy is not 'consensus' so the
returned struct matches what the warning promises. Updated the
existing `validatePolicyConfig` test to assert the new contract.
**[Suggestion] Stderr-breadcrumb assertion missing from
error-precedence regression test** (3273077272). The Round-8
test pinned the return-value behavior (`false`) but not the
unconditional-stderr promotion that was the primary behavioral
change of that hunk. Added `vi.spyOn(process.stderr, 'write')`
+ assertions for both "rejected permission vote" and the literal
requestId in the test. A future refactor that drops or downgrades
the log line is now caught.
**[Suggestion] _validPolicies underscore-prefix misleading**
(3273077262 — already addressed). Round 9's commit
|
||
|---|---|---|
| .. | ||
| daemon-client-adapters | ||
| development | ||
| examples | ||
| tools | ||
| _meta.ts | ||
| architecture.md | ||
| channel-plugins.md | ||
| contributing.md | ||
| qwen-serve-protocol.md | ||
| roadmap.md | ||
| sdk-java.md | ||
| sdk-python.md | ||
| sdk-typescript.md | ||