Repository navigation
[AI-1528] Trust-by-default borrowed-review advertisement - #376
Merged
Merged
Conversation
Borrowed-review capability was gated on the installed cursor-agent matching an exact-build certification record. Cursor auto-updates, so the record went stale within days: the daemon then advertised BorrowedReviewSupported=false, the server resolved workspace_mode=fallback, and reviewers read a stale committed base instead of the requester's working tree — silently, with no warning in the daemon log and no notice to the requester. Capability is now a property of the vendor's factory alone. The two matching pre-spawn throws go with it (advertisement and launch must agree); the sibling borrowed_snapshot_containment_mismatch check stays, since it validates a declared-containment invariant rather than a version fact. The certification record is demoted to CursorBorrowedReviewValidation / TryMatchValidatedBuild — a record of the last build the maintainer probe suite validated, gating nothing, with a non-match the expected steady state. One Information line per unattended vendor at daemon startup names the probed CliVersion (or the literal `unknown`), worded as a startup observation that goes stale on a vendor update. It deliberately does not evaluate agreement with the validation record, and nothing equivalent is logged per launch. Removing the gate also removes its macOS/arm64 preconditions, so borrowed review is now advertised on every platform where the vendor CLI is installed. That expansion is deliberate — the snapshot path is platform-neutral. Design: docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md
Adds the two-checkout orchestrator integration test the borrowed-snapshot path never had. It launches with RepoPath = a daemon-registered decoy checkout and BorrowCwd = a divergent requester checkout, so the load-bearing assertion is that WorktreeInfo.SourceRepo is the REQUESTER's git root — a single-directory test satisfies every content assertion while passing just as happily against the stale-base behavior it exists to prevent. Also asserts: branch-only commit, tracked-modified content and non-ignored untracked file present; .gitignore'd file and decoy content absent; snapshot root outside the source repo; reviewer mutation isolated; per-round refresh picks up requester changes AND drops reviewer-only files and git metadata. The "byte-unchanged" claim uses a real content-hash baseline (per-path SHA-256 plus unix mode) re-checked after snapshot creation, reviewer mutation, refresh and stop, plus HEAD sha + porcelain status for git state. SnapshotTree is not reused: it records entry names only, so it would miss in-place corruption. Docs: state the trust-by-default posture in CLAUDE.md and correct the README's "certified borrowed-context strategy" wording. CI matrix records why macOS is absent and how that platform is covered instead.
PR Summary by QodoTrust-by-default Cursor borrowed-review capability and snapshot launch
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Overly verbose test narrative comments
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+99
to
+114
| // ── Borrowed-snapshot regression net (two distinct checkouts) ───────────────────────── | ||
| // docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md | ||
| // | ||
| // Until the capability gate above was removed this path had, on the evidence in that design, | ||
| // never executed in production — the daemon advertised no borrowed support, the server resolved | ||
| // workspace_mode=fallback, and reviewers ran in an owned worktree at the last commit. Its | ||
| // correctness was therefore unevidenced, which is what this test supplies. | ||
| // | ||
| // TWO checkouts are mandatory. The orchestrator snapshots the borrow cwd's own canonical git | ||
| // root, deliberately INDEPENDENT of the launch command's registered RepoPath, so a test pointing | ||
| // both at one directory satisfies every content assertion below while proving nothing about | ||
| // which checkout was selected — it would pass just as happily against the stale-base behavior | ||
| // this test exists to prevent. The launch below registers the DAEMON checkout as RepoPath and | ||
| // borrows the REQUESTER checkout, and the load-bearing assertion is that the snapshot's | ||
| // SourceRepo is the requester's git root. | ||
|
|
There was a problem hiding this comment.
1. Overly verbose test narrative comments 📘 Rule violation ⚙ Maintainability
The PR introduces extensive multi-paragraph narrative comments across a unit test, core daemon logic, and the CI workflow configuration, which adds noise and makes the code and config harder to read and maintain. These historical/motivational explanations are better kept brief inline (with a pointer to external docs when needed) rather than embedded as long narratives in source files.
Agent Prompt
## Issue description
Long, narrative comment blocks were added in multiple places (a unit test, core daemon logic, and the CI workflow), which increases noise and reduces readability/maintainability; these should be reduced to brief, essential comments, with detailed historical rationale moved to external documentation (and referenced via a short pointer) when necessary.
## Issue Context
Compliance (PR Compliance ID 6) requires keeping comments concise and essential, preferring self-explanatory code/configuration. Where additional rationale or historical failure-mode context is important, keep a short inline summary and include a link/reference to a design spec or other external documentation rather than embedding multi-paragraph narratives directly in the codebase.
## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/AgentOrchestratorBorrowLaunchTests.cs[99-114]
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[612-616]
- .github/workflows/ci.yml[24-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
realtonyyoung
added a commit
that referenced
this pull request
Jul 28, 2026
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.
realtonyyoung
added a commit
that referenced
this pull request
Jul 28, 2026
* Address Qodo findings from #376/#377 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. * fix: contain ack CONSTRUCTION, not just the send 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. * fix: no wire send may gate settlement or kill the lane 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. * fix: keep the lane alive through diagnostics, and get every send out 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. * fix: commit-then-notify ordering, and diagnostics that cannot become 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. * fix: address Qodo findings 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.
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.
Problem
A live
spec-reviewand a follow-upcode-review, both with an explicitly selected Cursor reviewer and default workspace behavior, reviewed the stale committed base instead of the requester's working tree. Untracked and tracked-but-uncommitted changes were simply absent.The root cause is not a snapshot defect — the snapshot path never ran.
CursorBorrowedReviewCertification.TryCertifypinned the exact build2026.07.20-8cc9c0b(version-directory name + launcher SHA-256 + whole-bundle digest). Cursor auto-updated on 2026-07-23, soTryCertifyreturnednull,ComputeUnattendedVendorCapabilitiesadvertisedBorrowedReviewSupported = false, and the server correctly resolvedworkspace_mode = fallbackfrom the capability it was told about.Nothing reported any of this. The daemon's only Cursor diagnostic covers a missing binary, so an installed, working, uncertified one is silent; and on fallback the loud notice goes into the reviewer's prompt, not to the requester. Exact-pinning a whole-bundle digest against a binary that auto-updates guarantees recurrence — the certification shipped 07-20 and was dead by 07-23.
Change
Trust-by-default. A new version of a coding harness is assumed to work. Borrowed-review capability is now a property of the vendor's factory alone:
The two matching pre-spawn throws go with it, or advertisement and launch disagree and a borrowed launch hard-fails instead of degrading:
descriptor.ResolveBinaryPath(config)every other vendor uses;cursor_borrowed_artifact_not_certifiedguard is deleted.The sibling
borrowed_snapshot_containment_mismatchcheck stays — it validates the descriptor's declared containment against the launch context, which is a code-level invariant, not a version fact.CursorBorrowedReviewCertification→CursorBorrowedReviewValidation,TryCertify→TryMatchValidatedBuild. It keeps its digests and implementation but loses its authority: a record of the last build the maintainer probe suite validated, gating nothing, where a non-match is the expected steady state rather than an error. Its only production caller is now that manual workflow.CursorLauncherPolicyVersionand thereviewer_certification_changedlaunch rejection are a different mechanism (daemon launcher policy vs. server certification revision) and are untouched.One Information line per unattended vendor at daemon startup naming the probed
CliVersion(or the literalunknown). No new wire field —UnattendedVendorCapability.CliVersionalready exists and is already populated. The wording marks it as a startup observation that goes stale on a vendor update, because it genuinely is: the launch path resolves the binary at spawn time, so a reviewer launched after an auto-update runs a different build than the one logged. That limitation is stated rather than hidden.No automated drift detection of any kind — deliberately. No marker comparison at startup (that would be exactly the version-drift detection this rejects, and would hand the demoted record a production caller immediately). No per-launch version probe or binary-path logging (
ResolveBinaryPathis justcfg.CursorPath, normally the bare tokencursor-agent; making it meaningful needs final-symlink inspection added specifically to reveal drift, and would still be racy). A defective vendor release is handled by a human report and a corrected record. That trade is weaker for containment than for reads, and the design says so openly rather than implying the probe suite still guards it.TryCertifyalso returnednullon non-macOS and non-arm64, so removing the gate removes those preconditions. Cursor borrowed review is now advertised on every OS and architecture wherecursor-agentis installed — not only macOS arm64, but Linux, Windows, and macOS x64 too.This is accepted deliberately. No genuine platform dependency exists to encode:
WorktreeManager.CreateBorrowedSnapshotAsync/SyncFromSourceAsyncare platform-neutral git plus managed file operations whose branches already handle Windows explicitly;SafeUnixPathis a certification concern (vendor bundle permissions), not a snapshot one; and neitherAcpHostedAgentRuntimeFactorynor the Cursor descriptor carries any other OS/arch gate. Reintroducing a platform gate would be a certification remnant in a different hat.Coverage for that claim, recorded in
DaemonRunnerCursorAvailabilityTestsand in the CI matrix comment:ubuntu-latestwindows-latestA Linux-only assertion would have been insufficient, so the residual macos-x64 risk is instead bounded by a host-independent source guard: neither the advertisement path (
DaemonRunner.cs) nor the launch path (AcpHostedAgentRuntimeFactory.cs) may reference the validated-build record. A re-introduced gate fails that guard from any host, including for the leg no runner executes — which a behavioral assertion can never do. The guard ships with its own self-test.Tests
BorrowedReviewSupported = truewithindependent-snapshot(the direct regression test — it replaces one that pinned the opposite rule and passed onmain); a factory withSupportsBorrowedReviewFlow = falseis still not advertised (the gate that remains, asserted on Cursor so a blanket "always true" would be caught); the advertisement mirrors the factory flag across a vendor matrix.borrowed_snapshot_containment_mismatchstill throws before spawn on a genuine mismatch.unknownand the vendor is still advertised borrowed-capable; the wording marks it a startup observation. Two negative tests, and they are load-bearing: the startup line performs no marker-agreement evaluation, and a launch logs no vendor version and no binary path — a test demanding either would drag the rejected behavior straight back in.RepoPath= daemon-registered decoy,BorrowCwd= divergent requester — becauseAgentOrchestratorsnapshotsBorrowCwd/CanonicalGitRootindependently ofRepoPath, so a single-directory test satisfies every content assertion while passing just as happily against the stale-base behavior. The load-bearing assertion isWorktreeInfo.SourceRepo == requester git root; verified by mutation (repointingBorrowCwdat the decoy fails at exactly that assertion). It further asserts branch-only commit / tracked-modified content / untracked file present,.gitignored file and decoy content absent, snapshot root outside the source repo, reviewer-mutation isolation, per-round refresh picking up requester changes, and reviewer-only files and.gitmetadata disappearing from the refreshed snapshot (the weaker "didn't reach the requester" form passes against a refresh that accumulates droppings; the droppings are asserted to exist first, so the disappearance is not vacuous).SnapshotTreeis deliberately not reused for the byte-comparison — despite its comments it records entry names only and would miss in-place corruption. The baseline is per-path SHA-256 plus unix mode, re-checked at all four points (after creation, after reviewer mutation, after refresh, after stop), with.gitexcluded (read-only git plumbing legitimately refreshes index stat metadata) and git integrity covered separately by HEAD sha + porcelain status.Results
origin/main)Tests.Unit(macOS arm64)Tests.IntegrationThe 42 failures are byte-identical to the
origin/mainbaseline (diffof the sorted failure lists is empty) — the documented pre-existing macOS/tmp-symlink / parallel-load flakes in the config-toml, MCP-registration and uninstall tests. +22 new tests, all green.Marker validation finding
cursor-agent --versionon the installed macOS build prints exactly the version-directory form (2026.07.23-e383d2b), so the probedCliVersionand the marker'sVersionconstant agree today. They remain independent facts and nothing in production compares them; this is recorded in the marker's doc comment.Outstanding (requires a live authenticated Cursor — not available in this environment)
2026.07.23-e383d2b. The embedded digests are deliberately left at2026.07.20-8cc9c0b: recomputing the hashes without running the probes would record a validation that never happened. Under trust-by-default a stale marker gates nothing, so this does not block the change — but it is a real outstanding item, not a skipped one.workspace_mode=borrowed; snapshot under{WorktreeRoot}/borrowed-snapshots/rather than an ownedagent-{guid}; three sentinels in round 1; refreshed contents in round 2; write isolation proven positively — a successful mutation under the snapshot root plus a content-hash baseline showing the requester byte-identical). The executable sequence is one flow, two rounds on the same run — not two aliases, sinceSubmitReviewRoundRequestcarries nokindand a follow-up resolvesrun.DefinitionId. Items 1/2/4/5 exist for the sibling Copilot issue's rollout gate; confirming item 3 alone satisfies this issue but leaves that one blocked.docs/HOW_IT_WORKS.md's Copilot/Cursor items live in kcap-server and are corrected there, alongside the submodule bump after this merges.Knock-on
The sibling Copilot regression's design is built on the opposite rule and cannot ship unamended: its
CopilotBorrowedReviewCertification+ record table, the pre-spawnTryCertifyrevalidation and its identity cache, the "binary swapped after advertisement" test matrix, the certification-lag UX, and the additive-installed-version-field claim (CliVersionalready exists) all collapse. Snapshot routing, the fail-closed start, thecontext-onlycontract, and thecopilot + native-tool-clampclassification arm survive — that arm keys on a vendor-descriptor fact, not a certification fact.Compatibility
No new events, no projection bump, no new error codes, no flow-response changes, and no new field on the daemon's unattended-capability advertisement. New daemon + current server is fixed with no server change (the server already accepts
independent-snapshotand resolves borrow from the advertised capability); new daemon + old server is fixed identically since only the advertised value differs. The change is the removal of a gate plus one startup log line per unattended vendor.Design:
docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md(kcap-server).