Repository navigation
[AI-1526][AI-1528] Address Qodo findings from #376/#377 - #378
Merged
Merged
Conversation
Keep the liveness read off the sequenced-command lock, correct the README's settlement-retry description, and trim design-doc-grade comments. - SequencedCommandProcessor no longer calls the readLiveness delegate while holding _lock. The proactive terminal ack introduced by #377 put that read on every settled command, not just a duplicate replay, so lock-hold time grew and could delay concurrent SubmitAsync/AckPrefix callers. Both ack paths now record the outcome in the cache under the lock and build the ack after releasing it, preserving the ordering invariant that an ack can never advertise an outcome the cache has not recorded. - AgentKillQuarantine gains an allocation-free IsQuarantined(agentId); AgentOrchestrator.ReadLiveness uses it instead of Snapshot().Any(...), which allocated a projected list on every call. - README: settlement-layer coded 409s are no longer described as retried "a few times" and normally invisible. Documents the real 3-minute elapsed deadline, the backoff shape, which codes retry, and the explicit isError tool result the flows MCP server returns when the deadline is exhausted. - Trimmed restated-mechanics comments in FlowRetryTiming, SequencedCommandProcessor and AgentOrchestratorBorrowLaunchTests. Comments recording a non-obvious why (the pinned backoff formula, the two-checkout requirement, the non-vacuity guards) are kept. Tests: new regression coverage that the liveness delegate is never invoked under the processor lock (mutation-verified), that the cache records the outcome before the ack is built, and for IsQuarantined.
realtonyyoung
marked this pull request as ready for review
July 28, 2026 11:43
Collaborator
Author
Local test results (macOS,
|
| total | failed | skipped | |
|---|---|---|---|
baseline origin/main (4e7ee2c) |
4059 | 44 | 8 |
| this branch | 4062 | 42 | 8 |
+3 total = the three new regression tests, all passing.
Failing-set diff: zero new failures. All 42 branch failures are a subset of the 44 baseline failures — the known local tmp-symlink / parallel-load flakes (RegisterKcapMcpServers_*, UnregisterKcapMcpServers_*, InstallCodex_*, User_level_uninstall_*, EnableNetworkAccess_*, …). Two baseline failures (Dispose_flushes_the_partial_buffer_…, Transcript_channel_at_capacity_…) did not reproduce on the branch — both are unrelated to these changes and are themselves flaky.
Mutation check on the new guard (so it is not vacuous): reverting both ack paths to build the CommandAck inside _lock makes Liveness_is_never_read_while_the_processor_lock_is_held fail with readsUnderLock == 2 (one per path). Reverted before commit.
scripts/check-linear-ids.sh → exit 0.
PR Summary by QodoMove settlement liveness reads outside the command lock
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codex review of #378. SendProactiveAck(BuildProcessedAck(..)) evaluates its argument before entering the try, so a throwing readLiveness -- a delegate that walks the orchestrator's live lifecycle collections -- escaped the containment entirely. From the lane that faults RunLaneAsync and leaves the item's Done unresolved: the submitter waits forever and every later command is stranded, which the server reads as a permanently held daemon capacity slot. From SubmitAsync it escapes into the hub handler, and that is the RECOVERY path -- a duplicate replay is what the server sends when it never received the terminal ack. SendProactiveAck becomes SendSettledAck(item, outcome) and builds inside the try. The replay path now goes through it too, replacing a bare `_ = _sendAck(...)` that contained neither a synchronous throw nor a faulted task. Three tests. The lane one is bounded on purpose: reverting the fix makes it HANG rather than fail (verified -- an unbounded await pinned the suite until the harness timeout), and a timed-out CI job is a much worse signal than a named assertion. Also corrected the comment on Proactive_ack_is_built_only_after_the_outcome_ is_recorded_in_the_cache: the review pointed out it is not a regression net for the lock placement, since Monitor is reentrant and LastProcessedSeq would read the same updated value from inside the lock. It pins the ordering, which is worth pinning; lock placement is covered by the test above it. 20/20 unit tests, check-linear-ids clean.
Codex review round 2 of #378. Both findings predate this PR -- they are in the unreviewed #376/#377 surface -- but I asked whether any send was left uncontained, so they belong here. P1: the lane's rejection sends. Both arms called _sendRejected BEFORE the cache entry was marked Processed and Done resolved. A synchronous throw escaped RunLaneAsync, permanently killing the single serial consumer: the command stayed nonterminal, its submitter waited forever, and every queued command was stranded -- the same held-capacity-slot class the ack fix addressed. Now the lane settles FIRST, then notifies, with both sends contained; relative order (rejection then ack) is unchanged. The whole body is wrapped in try/finally so Done is released even if something unforeseen throws. P2: the in-progress duplicate's Accepted ack was still a bare send under _lock. A synchronous throw escaped into the hub, a faulted task went unobserved, and any synchronous transport work ran inside the processor's critical section -- the section this PR exists to keep narrow. HandleDuplicateLocked now only CAPTURES which answer is owed; SubmitAsync sends after releasing the lock. Consolidated into one SendContained(Func<Task>, seq, what) helper that invokes the delegate inside the try, so argument construction is covered at every site. Every _sendRejected/_sendAck in the file now routes through it. Two more tests: a throwing rejection send must not fault the lane or lose the settlement, and a throwing Accepted-ack send must not escape an in-progress duplicate. Both bounded, for the reason recorded on the previous test. 22/22 unit tests, check-linear-ids clean.
…of the lock Codex review round 3 of #378. The try/finally comment I wrote last round was wrong, and the review said so. finally releases the submitter but the exception still propagates out of the await foreach and faults the lane -- and it reports SUCCESS to a submitter whose command may never have been marked terminal. The realistic trigger is diagnostics: an ILogger provider throwing from the execute-fault LogWarning, or from SendContained's own catch. There is now a real per-item catch that synthesizes the terminal state when the normal path did not reach it and lets the loop continue, with every diagnostic inside it individually guarded -- because the most likely way to arrive there is diagnostics throwing. Second: SendContained catches exceptions but still invokes the delegate immediately, so it does nothing about lock scope. RejectLocked, the duplicate-collision branch and SynthesizeErrorLocked were all still running transport delegates inside _lock -- the same objection that moved the Accepted ack out. They now only record WHICH rejection is owed; SubmitAsync sends it after releasing the lock. Containment and lock-scope were two problems and I had only fixed one. Two tests, both aimed at the gaps the review identified in the previous ones: a throwing ILogger (NullLogger cannot reach that path) asserting the command is still terminal and the lane survives, and a direct assertion that no rejection send runs with the lock held, covering the two locked reject paths the lane-based test could not. 24/24 unit tests, check-linear-ids clean.
…the failure Codex review round 4 of #378. P1: `settled` was set AFTER the rejection notification, so a throw escaping SendRejectedContained reached the outer catch with the real terminal outcome already committed -- and the catch overwrote it with InternalError. A LaunchRejected(daemon_capacity) could be cached and announced as one result and replayed later as a contradictory one. It is now set the instant the cache+watermark commit succeeds, before any notification: the cache is authoritative from that point and announcing it must never be able to rewrite it. P2: SendContained's own diagnostics were unguarded. On the hub-side paths (stale epoch, gap, duplicate collision, accepted/processed replay) there is no outer per-item catch, so a transport throw followed by a logging throw escaped straight into the hub; and the async branch's unguarded LogDebug faulted its own DISCARDED continuation, recreating the unobserved task failure the wrapper exists to prevent. Both now route through LogQuietly. One new test, mutation-verified: un-guarding the synchronous diagnostic makes it fail. Deliberately NO test for the `settled` ordering. I wrote one, then mutation-tested it and it passed against the un-fixed code -- with the diagnostics guarded, no post-commit path can escape into the outer catch, so the overwrite is currently unreachable. The ordering stays as defence-in-depth (free, and it removes a latent contradiction) but a test that passes either way would claim coverage that is not there, which is the exact criticism this review made of an earlier ordering test. The reasoning is recorded in a comment where the test would have gone. 25/25 unit tests, check-linear-ids clean.
1. Removed the design-doc paths from FlowRetryTiming's XML comments. They embedded an internal issue slug AND pointed at a file in a private repo, so a public reader could not follow them anyway. The API explanation stays. 3. The deadline message asserted a cause the client had not observed. LastCode is null when the deadline cancels an in-flight request before any coded response is parsed -- the very first attempt can time out -- and the message still said "the daemon is still settling a prior launch" while printing the client's own default code in the Error(...) position, which reads as a server verdict. The code token stays stable (agents match on it) but the narrative now says where it came from. README corrected to match. Two tests pin both arms of the discriminator. 2. Trimmed the duplicated concurrency narration: the caller-side comments now point at SendSettledAck instead of restating its rationale, and BuildProcessedAck's summary is shorter. The invariants themselves stay -- "never under _lock" and "the cache is committed before the ack is built" are exactly what five rounds of review found, and losing them would invite the same defects back. 38/38 settlement-retry tests, 25/25 processor tests, check-linear-ids clean.
Collaborator
Author
|
All three addressed at 1 — 3 — Error provenance. Correct, and it was worth more than a README edit, so I fixed the message too.
Two tests pin both arms: no coded response must not say "still settling", and a real coded response must still say it. README updated to match. 2 — Verbose concurrency comments. Partly taken. I trimmed the duplication you pointed at: the two caller-side comments now point at I did not remove the invariants themselves, and I want to be explicit about why rather than quietly declining. "Never invoke the liveness read under A codex review flow also ran on this PR to clean over five rounds; the fixes it produced are in the commits above |
alexeyzimarev
added a commit
that referenced
this pull request
Jul 30, 2026
* docs: design for flow-participant-aware agent commands Protects review and review-flow agents from accidental attach-injection and stop, enforced daemon-side. Read-only attach, stop refuses without --force, stop --all skips and says so. Refs AI-1557, #379. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: implementation plan for flow-participant-aware agent commands Also corrects the spec's Attached mechanism: its payload ends with an unbounded snapshot, so a trailing flag would be painted onto the terminal rather than parsed. Uses a separate AttachedReadOnly frame instead, which also fails closed against an older client. Refs AI-1557, #379. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(agent): carry and show each agent's launch kind * feat(ipc): add StopV2 and AttachedReadOnly frames * feat(daemon): attach to review and flow agents read-only * test(daemon): prove the read-only attach test discriminates on stdin delivery Attaching_to_a_flow_participant_is_read_only previously only asserted the frame type and empty ClientDims — neither observes the Stdin arm, so NoopPtyProcess silently swallowed writes and the test stayed green even with the readOnly guard deleted. Seed the agent with a recording PTY double instead and assert zero writes; add the mirror assertion to the plain-agent case so the pair brackets the behaviour. * feat(cli): print the read-only banner and stop sending input * feat(daemon): refuse to stop review and flow agents without force * fix(daemon): stop-all skip uses predicate negation, not record Except Except hashed AgentInstance's mutable teardown fields (Status, LastOutputAt, ...) rather than identity, and was enumerated after the concurrent stops had already started mutating them — a stopped agent could be misreported as skipped too, with a duplicate row in the ack. Also corrects the refusal message for a plain (non-flow) review agent, which previously claimed a flow round it doesn't have. * feat(cli): add `agent stop --force` and report skipped agents Switches the client onto the StopV2 frame the daemon has enforced protection against since #378: `stop --all` now partitions review/review-flow agents out of the confirmation prompt and reports them as skipped (exit 0) rather than stopping everything unconditionally, and `--force` opts back in. Also pins LocalControlServer's StopV2 decode-and-dispatch over a real socket, the one hop the codec round-trip and handler tests don't individually cover. * docs: document read-only attach and stop --force for review agents * docs: fix version-skew inaccuracies in the agent-protection caveat The claim that daemon-side enforcement "holds regardless of client version" was wrong: an old kcap sends the legacy Stop request (no --force concept), and the daemon treats that as --force, so a stale client can silently force-stop a review/review-flow agent against an up-to-date daemon. The old-daemon sentence also conflated stop with ls/attach: those degrade silently, but stop always sends the newer request format an old daemon can't decode, so it hard-fails and tells the user to restart the daemon instead of running unprotected. Also filled in two gaps in help-agent.txt's parallel text: attach's read-only view also ignores terminal resize, and --force lifts the single-id refusal, not just the --all skip. * fix: address whole-branch review findings on flow-participant agent commands - stop --all --force now groups protected agents under their own labelled heading, matching the spec (the plan's snippet had it backwards; fixed the plan too so re-execution won't replay the defect) - the PID-record stop path (a prior-incarnation survivor) now honours Kind-based protection instead of reaping unconditionally; the decision reads the record via a new FindPidRecord accessor, keeping TryStopByPidRecordAsync itself policy-free for its server-origin caller - KindText/IsProtectedKind/ProtectionReason fail safe on an unrecognised LaunchKind instead of defaulting to unprotected - FrameCodec.StopV2 guards against a zero-length payload - ParseAgentRow and the ls fetch path restore the 3-column floor a stray tab in a repo path could otherwise defeat - doc/help/test fixes: help-agent.txt synopsis gets [--force], stale ls/AgentList column docs updated, and the tautological --force dispatch test is supplemented with one that can actually fail Closes #379, AI-1557 * docs: correct the FindPidRecord summary and the ls row shape in top-level help The FindPidRecord extraction left the original XML summary attached to the new method, so it claimed to "reap it by identity and delete the record on confirmed death" — which it does not; it only reads. Moved back onto TryStopByPidRecordAsync, which does do that and was left undocumented. help-usage.txt still described `agent ls` as (id, status, repo); it grew a KIND column. This was the fourth instance of that stale text, the other three having been corrected already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Neutralise table delimiters in the agent list and refuse malformed rows `AgentList` is a tab/newline-delimited table, and RepoPath, FlowRunId and FlowRole are free-form — a repo path may legally contain a tab or newline. Emitted raw, one shifts the reader's columns or splits the row. That is not merely cosmetic. The CLI keys `stop --all`'s confirmation off the kind column, so a shifted row makes a plain agent read as protected: the prompt says "Skipping 1 review agent", the user confirms N, and the daemon — which computes eligibility itself and is authoritative — stops N+1. The confirmation understates the blast radius, which is the property the prompt exists to convey. Closed at both layers. The daemon replaces tab/CR/LF with a space in the three free-form cells. The CLI accepts only exactly 3 columns (older daemon) or exactly 6 (current) and refuses the whole table otherwise, rather than acting on a guess. Found by Qodo on #408. Two earlier reviews rated this class Minor on the grounds that the daemon stays authoritative for the stop — true, but it is the prompt that the user consents to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #376 and #377, both of which merged with unaddressed Qodo review findings. No behavior change to the settlement protocol itself.
Finding 1 — liveness read inside the sequenced-command lock (real bug/perf, from #377)
SequencedCommandProcessor.BuildProcessedAckLocked()invoked thereadLivenessdelegate while holding_lock. In production that delegate isAgentOrchestrator.ReadLiveness(), which calledAgentKillQuarantine.Snapshot()and LINQ'd over the projected list — allocating inside the critical section. #377's proactive terminal ack moved that from "only on a duplicate replay" to every settled command, so lock-hold time grew and could delay concurrentSubmitAsync/AckPrefixcallers.Both halves fixed:
AgentKillQuarantine.IsQuarantined(agentId)— an allocation-freeConcurrentDictionary.ContainsKeymembership test.AgentOrchestrator.ReadLivenessnow uses it instead ofSnapshot().Any(q => q.Id == agentId)._readLivenessis no longer invoked under_lockon either ack path:HandleDuplicateLockednow only captures the cachedCommandOutcome(a struct copy) into anoutparameter;SubmitAsyncbuilds and sends the ack after releasing the lock.SubmitAsync's body moved intoSubmitLockedso the lock scope stays a single, obvious block.BuildProcessedAckLockedis renamedBuildProcessedAckand its doc now states the "must not be called under_lock" contract.The deliberate ordering invariant is preserved: both callers record the outcome in the cache before the ack is built, so an ack can never advertise an outcome the cache has not recorded. Only the ack construction moved out of the lock, not the cache write.
Finding 2 — README described the old settlement-retry behavior (from #377)
The README said settlement-layer coded 409s were retried "a few times" and were "normally invisible to the caller." That stopped being true when the flows MCP server moved to a 3-minute elapsed deadline with an explicit coded timeout as a tool error. The section now documents:
flow_settlement_busy,reviewer_launch_incarnation_superseded) and that everything else surfaces on the first attempt;isError: truetool result in theError (code): messageshape, carrying the last coded error plus attempt count and elapsed time — with an example — and that it is retryable;Finding 3 — comment verbosity (both PRs, "review recommended")
Trimmed restated mechanics and repeated rationale at the three flagged sites, to roughly the density of the surrounding untouched code:
FlowRetryTiming.cs—FlowRetryClockandSettlementBackoffclass docs condensed. Kept the pinned backoff formula: it is the assertable contract the tests pin, not a restatement of the code.SequencedCommandProcessor.cs— dropped the "Task 13 / Task 15" plan-ticket call-site annotations and the duplicated §3.2 F rationale that appeared in three places; the ordering invariant is now stated once, onBuildProcessedAck.AgentOrchestratorBorrowLaunchTests.cs— dropped the historical "this path had never executed in production" paragraph. Kept the two-checkout requirement (a future simplification to one repo would silently make the test vacuous), the decoy-content note, and the non-vacuity guards ("the droppings must genuinely exist first", "re-basing the baseline cannot launder a corruption") — those record a non-obvious why and prevent regressions. Also kept theContentBaseline/GitStatehelper docs explaining why.gitis excluded and whySnapshotTreeis not used.Tests
New regression coverage in
SequencedCommandProcessorTests:Liveness_is_never_read_while_the_processor_lock_is_held— a test seam (LockHeldByCurrentThreadForTest,Monitor.IsEntered) lets the injectedreadLivenessdelegate assert directly that it is not holding the lock. No sleeps, no thread choreography. Mutation-verified: reverting either ack path to build under the lock fails this test withreadsUnderLock == 2.Proactive_ack_is_built_only_after_the_outcome_is_recorded_in_the_cache— guards the ordering invariant the lock move had to preserve.AgentKillQuarantineTests.IsQuarantined_answers_by_id— including the ordinal-comparison behavior of the backing dictionary.Suite:
Capacitor.Cli.Tests.Uniton macOS — baselineorigin/main4059 total / 44 failed (the known local tmp-symlink + parallel-load flakes: config-toml, MCP-registration, uninstall). Branch result recorded in a follow-up comment; failing sets diffed to confirm no new failures.scripts/check-linear-ids.shpasses (no Linear IDs in.cs).