feat(quad-cli): record declared vs effective reviewers and add a live smoke preflight - #16
Merged
Merged
Conversation
This was referenced Aug 27, 2026
drvoss
added a commit
that referenced
this pull request
Aug 27, 2026
…x docs (closes #15) Implements the remaining scope of issue #15 that PR #16 explicitly deferred, plus three non-blocking findings surfaced by cross-model review (Opus/Sonnet/Codex) of PR #16. Transport-integrity nonce: - Each run issues a random per-run token and instructs every reviewer to echo it back as a top-level transport_nonce field, so a schema-valid response can be distinguished from one that silently worked from a truncated/stale/altered prompt. - reviewers.nonce_status records confirmed/not-echoed/mismatch/unavailable per declared tool (only under --artifact, alongside the rest of reviewers/environment). - A mismatch (present but wrong echo) drops that reviewer from consensus even though its JSON was otherwise schema-valid. A missing echo is NOT penalized -- it is indistinguishable from an older CLI/model that never learned the field, per issue #15's own framing. - QUAD_CLI_TRANSPORT_NONCE env var overrides the generated token for deterministic test fixtures only. --min-reviewers revisit: already resolved by PR #16's documented decision to keep the default at 2 with recorded rationale; no code change needed, closing out the remaining issue #15 item. Review follow-ups: - observed_failure diagnostics now redact embedded absolute paths (home-dir and otherwise), not just resolved_path, closing the leak Codex's review of PR #16 found. - Documented that reviewers/environment are only populated under --artifact (doc previously implied they were unconditional like status/reviewers_effective). - Documented the ~40s, --gate-timeout-exempt cost of --artifact's CLI version re-probing. Refs #15
drvoss
added a commit
that referenced
this pull request
Aug 27, 2026
…fore Sonnet's cross-review of this PR found that placing the nonce instruction before <diff> only proves the reviewer received the instructions header, not the diff body -- the realistic failure mode (tail truncation of the usually-large diff) would still let a reviewer see and correctly echo the nonce while never receiving the diff, producing a false 'confirmed'. Moving the instruction after </diff> means confirmed now requires having received the entire prompt, diff included; any truncation before that point yields not-echoed instead. Also verified (and rejecting as a false positive) Sonnet's other finding: the local 'main' branch ref used during review was stale (pre-dating PR #16's squash-merge), which made PR #16's already-merged --min-reviewers rationale text look new in a diff against local main. origin/main already contains it; no change needed there.
drvoss
added a commit
that referenced
this pull request
Aug 27, 2026
) * fix(quad-cli): add transport-integrity nonce, redact leaked paths, fix docs (closes #15) Implements the remaining scope of issue #15 that PR #16 explicitly deferred, plus three non-blocking findings surfaced by cross-model review (Opus/Sonnet/Codex) of PR #16. Transport-integrity nonce: - Each run issues a random per-run token and instructs every reviewer to echo it back as a top-level transport_nonce field, so a schema-valid response can be distinguished from one that silently worked from a truncated/stale/altered prompt. - reviewers.nonce_status records confirmed/not-echoed/mismatch/unavailable per declared tool (only under --artifact, alongside the rest of reviewers/environment). - A mismatch (present but wrong echo) drops that reviewer from consensus even though its JSON was otherwise schema-valid. A missing echo is NOT penalized -- it is indistinguishable from an older CLI/model that never learned the field, per issue #15's own framing. - QUAD_CLI_TRANSPORT_NONCE env var overrides the generated token for deterministic test fixtures only. --min-reviewers revisit: already resolved by PR #16's documented decision to keep the default at 2 with recorded rationale; no code change needed, closing out the remaining issue #15 item. Review follow-ups: - observed_failure diagnostics now redact embedded absolute paths (home-dir and otherwise), not just resolved_path, closing the leak Codex's review of PR #16 found. - Documented that reviewers/environment are only populated under --artifact (doc previously implied they were unconditional like status/reviewers_effective). - Documented the ~40s, --gate-timeout-exempt cost of --artifact's CLI version re-probing. Refs #15 * fix(quad-cli): place transport-integrity nonce after the diff, not before Sonnet's cross-review of this PR found that placing the nonce instruction before <diff> only proves the reviewer received the instructions header, not the diff body -- the realistic failure mode (tail truncation of the usually-large diff) would still let a reviewer see and correctly echo the nonce while never receiving the diff, producing a false 'confirmed'. Moving the instruction after </diff> means confirmed now requires having received the entire prompt, diff included; any truncation before that point yields not-echoed instead. Also verified (and rejecting as a false positive) Sonnet's other finding: the local 'main' branch ref used during review was stale (pre-dating PR #16's squash-merge), which made PR #16's already-merged --min-reviewers rationale text look new in a diff against local main. origin/main already contains it; no change needed there.
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.
Symptom
The final report records
reviewers_effective— a single number. It does not record whichreviewer dropped out, at which stage, or why.
Reproduction
With stub CLIs (
QUAD_CLI_MOCK_DIR), declare four reviewers and make two of them fail.The report says
reviewers_effective: 2and nothing about the other two.Hypotheses rejected
17/20 schema-valid. All four CLIs return correct JSON when invoked correctly (issue quad-cli-consensus-gate: cursor/agy fail to return schema-valid JSON in live runs (2/4 effective reviewers) #9).
--min-reviewersto 3 or 4 would raise confidence." Rejected — revieweravailability fails for reasons unrelated to review quality. Measurement supports rejecting
4as a gate threshold; it does not establish that any specific higher number is better.
Cause layer
Not the model layer — the invocation and observation layer. Issue #9's real defects were an
agy -pmisinvocation (it had never actually run) and a WindowsspawnENOENT for thecursor-agentshim. Both discarded their failure reasons and surfaced as "schema violation".Remaining scope
not distinguish a truncated transport from a model that ignored the instruction, so it needs a
design decision first.
Refs #15