[AI-1529] Render workspace diagnostics on every flow surface (CLI) - #380
Merged
Conversation
The CLI half of AI-1529. The server now reports whether a reviewer read the
caller's working tree or a checkout at the last commit; this makes it visible.
ONE helper (AppendWorkspaceDiagnostics) reached from all five formatting paths:
roundless start, the polled round result, the rounded round response, status and
close. A helper being correct is worth nothing if a surface forgets to call it,
so the tests assert through the REAL formatters rather than the helper -- the
polled path especially, since it is what an agent reads on nearly every flow and
a parity test written against the blocking path alone would pass while the
dominant path stayed silent.
Three outcomes, and the third is the one that matters:
borrowed -> "the reviewer saw your working tree, uncommitted changes included"
fallback -> "workspace: fallback (<reason>)" plus a caution that the reviewer
read the last commit and did NOT see uncommitted work
absent -> "workspace: unknown", NEVER borrowed
An older server, a multi-participant run and a generic single-participant flow
all omit the field. Guessing "borrowed" there would be the one wrong answer that
reads as reassurance, so absent and explicit-null both render unknown. A mode
this build does not recognise is reported verbatim rather than collapsed into a
known case -- inventing a category is how a caller ends up trusting a decision
the server never made.
Reasons are pass-through: no allowlist, no mapping table. Pinned by a test that
includes a token this CLI has never heard of, which a hardcoded example fails.
A fallback with no reason still discloses and still warns -- an empty
parenthetical would be worse than none, and swallowing the line would hide the
caution.
Also asserted: no rendered response leaks borrowed_cwd or any absolute path, and
equivalent payloads render identically whatever alias produced them (the server
never sees an MCP alias name, so this can only be checked CLI-side).
The start tool description now states that uncommitted-work visibility is
CONDITIONAL on borrowing, names the three values, and scopes the disclosure to
the reserved review aliases.
18 new tests, 118/118 across the McpFlows suites, check-linear-ids clean.
PR Summary by QodoRender workspace diagnostics across CLI flow responses
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
3 (correctness) is the real one. The fallback caution claimed the reviewer "read a checkout at the last commit" -- false for context_only_requested, where the reviewer read the SUBMITTED CONTEXT and no repository at all. A caller would have concluded committed code was reviewed when none was. Special-casing that reason would fix the one case and reintroduce the allowlist this feature exists without. The claim is now worded to hold for EVERY reason: "Your working tree was NOT borrowed -- the reviewer did not see uncommitted work." True for every fallback, and it makes no assertion about what WAS read. A regression test asserts the context-only render contains neither "last commit" nor "checkout". 1 (docs). README's flows section now documents the three outcomes, states that reasons are verbatim and open-ended, and spells out that unknown is NOT borrowed -- treat it as "assume uncommitted work was not seen". 2 (comment length). Trimmed the helper's XML doc from 16 lines to 5. Kept the two facts the code cannot state itself: absent renders unknown and never borrowed, and reasons are not enumerated anywhere. 19 workspace tests, 119/119 across the McpFlows suites, check-linear-ids clean.
…k claim Two things caught by reading the PR's own diff. The new helper was inserted BETWEEN AppendReviewerModelAudit's doc comment and its method, so that doc silently became the summary for AppendWorkspace Diagnostics and the audit helper was left undocumented. Moved the helper below the audit method; both now carry their own summary. The start tool description still said 'On fallback the reviewer read the last commit' -- the exact claim just corrected in the renderer for context_only_requested, where the reviewer reads the submitted context and no repository at all. Same fix, same reason: say only what is true for every fallback reason. It now reads 'your working tree was NOT borrowed'. 119/119 McpFlows, check-linear-ids clean.
Codex review of #380. 'the reviewer did not see uncommitted work' is not true for context_only_requested: the working tree was not borrowed, but the caller may have inlined an uncommitted diff into the submitted context, so the reviewer may well have seen uncommitted work -- just not from the tree. Two wordings have now been wrong in the same direction, both asserting more than the CLI can know: 'read a checkout at the last commit' -> false where no repository is read 'did not see uncommitted work' -> false where the caller inlined a diff The only claim that holds for every reason is about the AUTOMATIC channel: the tree was not borrowed, so nothing uncommitted arrived that way, and anything uncommitted the reviewer saw came from the submitted context. Naming reasons individually would fix each case and reintroduce the allowlist this feature exists without. The context-only test now also asserts the claim does NOT overreach the other way, so a future edit cannot quietly restore the stronger phrasing. 19/19 workspace 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.
The CLI half of AI-1529. The server (kcap-server#1205) now reports whether a reviewer read your working tree or a checkout at the last commit; this PR makes that visible to the caller.
Ships independently of the server side — every field is optional, and a server that doesn't send them renders
workspace: unknown.Why
If a reviewer ran in an owned worktree, it reviewed the last commit, not your working tree. Ask it to review work you haven't committed and it reviewed something else — and today nothing tells you. AI-1527 makes it sharper:
context_only_requestedis a fallback the caller asked for, and it was equally invisible.What it renders
One helper, five paths
AppendWorkspaceDiagnosticsis reached from all five formatting paths: roundless start, the polled round result, the rounded round response, status, and close.The tests assert through the real formatters, not the helper. A correct helper is worth nothing if a surface forgets to call it — and the polled path is what an agent reads on nearly every flow, so a parity test written against the blocking path alone would pass while the dominant path stayed silent.
The case that matters most
An older server, a multi-participant run, and a generic single-participant flow all omit the field. Rendering that as
borrowedwould be the one wrong answer that reads as reassurance, so absent and explicit-null both renderunknown, and a test asserts the word "borrowed" appears nowhere in that output.A mode this build doesn't recognise (a future server, or
owned) is reported verbatim rather than collapsed into a known case. Inventing a category is how a caller ends up trusting a decision the server never made.Pass-through, by construction
No allowlist, no mapping table, on either side. Pinned by a parameterised test whose last case is
a_reason_this_cli_has_never_heard_of— a hardcoded example set fails it.A fallback with no reason still discloses and still warns: an empty
()would be worse than none, and swallowing the line entirely would hide the caution.Also asserted
borrowed_cwdor any absolute path — that's a real path on the requester's machine; the mode/reason pair is the whole contract.Tool description
The start tool now states that uncommitted-work visibility is conditional on borrowing, names the three values, and scopes the disclosure to the reserved review aliases — generic flow kinds report
unknownby design (that gap is AI-1548).18 new tests, 118/118 across the McpFlows suites,
check-linear-idsclean.