feat(review): /ca:review can review an inbound pull request#503
Merged
Conversation
The reviewer fleet only ever ran on the diff you just wrote. `/ca:watch` watches a PR's CI but never runs the fleet against its diff, so for a tool that markets itself as a team gate, the half of the value that comes from reviewing code you did NOT write was missing (#80). `/ca:review #123` now fetches that PR's diff and runs the same fleet, the same path matrix, and the same finding-triage -> checkpoint-aggregator funnel. Deliberately an ARGUMENT, not a /ca:review-pr command. The scope resolver already took one, the fleet is scope-agnostic, and every phase downstream operates on a diff regardless of where it came from - so a second command would be a whole public surface (the catalog, three host projections, the README counts, the site sidebar) whose only distinguishing feature is where the diff was fetched from. The command count is unchanged at 40, which check_badge_ consistency confirms. Three guards ride with it, each closing a way this could go wrong quietly: - a missing or unauthenticated `gh` STOPs rather than falling back to the working diff, which would report a verdict on the wrong change under the PR's name; - the diff is resolved ONCE, because the fleet runs in parallel and a PR updated mid-review would otherwise have different reviewers reading different code and a triage reconciling two versions; - posting is a separate, confirmed step that never uses --approve or --request-changes. A review comment on someone else's PR notifies subscribers and cannot be un-sent, and those two flags carry merge authority this command does not have. The hard gate also refuses to CHECK OUT the PR branch: reviewing an inbound change means reading its diff, not adopting its code into a working tree the hooks trust. ca-pi advances 0.1.31 -> 0.1.32; ca (2.9.1) and ca-codex (0.3.0) carry no tag yet, so their entries join the open sections. Closes #80 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L
This was referenced Jul 26, 2026
SUaDtL
added a commit
that referenced
this pull request
Jul 26, 2026
The CI failure this addresses arrived as `expected undefined to be defined` with no cause, and the test's own diagnostic - "no job handle, the contained launch never returned" - actively misdescribed it. The launch DID return. It returned a refusal. Diagnosis first, because the obvious reading was wrong. `launch` is declared `Promise<Snapshot | undefined>` and `undefined` is a DESIGNED outcome: all five refusal paths transition the job to a terminal state before returning, so the runtime is fail-closed and never owns an untracked process. The registration race I feared when filing #504 does not exist, and that AC is withdrawn. What is wrong is that path 5 - the `#openTree` spawn - discards its error in a bare `catch {}`, so a real environment failure is indistinguishable from a policy refusal. Fixing that properly means extending the published activity event (`{kind, id, label, state}` has no detail field) or changing one path's refusal semantics, both of which deserve a decision rather than being slipped in behind a flake fix. So this takes the part that needs NO contract change: the live test wraps the tree opener, captures the throw, and reports it. Verified by mutation rather than by inspection - pointing shellPath at a nonexistent binary now fails with launch refused because the spawn threw: ENOENT: no such file or directory, lstat '...no-such-shell.exe' instead of the bare assertion. The diagnostic also distinguishes the two cases: a spawn that threw, versus a refusal that never attempted one. This does not fix the flake. It makes the next occurrence explain itself, which is what was missing when it cost a re-run cycle on #503. Refs #504 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L Co-authored-by: SUaDtL <SUaDtL@users.noreply.github.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.
What
/ca:review #123reviews an inbound GitHub pull request through the existing fleet. Closes #80.Why
The reviewer fleet only ever ran on the diff you just wrote.
/ca:watchwatches a PR's CI but never runs the fleet against its diff. For a tool that markets itself as a team gate, the half of the value that comes from reviewing code you did not write was missing.Same fleet, same path matrix, same
finding-triage→checkpoint-aggregatorfunnel. As the issue puts it: "plumbing a new input into an existing pipeline, not net-new logic."An argument, not a new command
This follows the principle you set when ruling on #382: the target becomes a parameter, not a new surface.
The scope resolver already took an argument, the fleet is scope-agnostic, and every phase downstream operates on a diff regardless of where it came from. A
/ca:review-prwould be a whole public surface — the catalog, three host projections, the README counts, the site sidebar — whose only distinguishing feature is where the diff was fetched from.Command count unchanged at 40, confirmed by
check_badge_consistency.py.Three guards
Each closes a way this could go wrong quietly:
ghSTOPs rather than falling back to the working diff — which would report a verdict on the wrong change under the PR's name.--approveor--request-changes. A review comment on someone else's PR notifies subscribers and cannot be un-sent; those two flags carry merge authority this command does not have. It produces a finding list, not a maintainer's decision.The hard gate also refuses to check out the PR branch — reviewing an inbound change means reading its diff, not adopting its code into a working tree the hooks trust.
Verification
test_ci_impact,test_badge_consistency,test_public_pi_docs,test_public_codex_docs,test_pi_parity,test_codex_parity_fixture,test_build_surface,test_routing_and_cleanup_surface— all OK. Site-voice gate clean. Surface rebuilt to all three hosts.Versions
ca-pi 0.1.31 → 0.1.32; ca (2.9.1) and ca-codex (0.3.0) carry no tag yet, so their entries join the open sections. Pi release guard confirms manifest, changelog, and generated root metadata advanced together.
Closes #80
https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L