feat(launchpad): three dimension definitions + STEP 6 exit-code fix for the PR review agent (#117) - #242
Merged
tucktuck101 merged 4 commits intoAug 20, 2026
Conversation
…117 STEP 4) Adds launchpad/review-agent/dimensions/{secrets-and-access,claim-vs-evidence, correctness-and-failure-modes}.py -- the scope, exclusions, severity guidance, anchoring rule, and output contract for each of #117's three review dimensions. Each names both what it reviews and what it must NOT, with exclusions naming the other two dimensions' subjects explicitly, so a reviewer that reviews everything does not review nothing well. Each restates FINDINGS.md's ten finding fields verbatim rather than inventing its own, and states the anchor rule (line/file/pr) in terms specific to its own finding classes. These files are specification/documentation modules: nothing imports or executes their content today (run_dimensions.py --list only lists filenames), matching #117's own scope -- choosing a model and building prompt-assembly wiring around these files is explicitly out of scope for this issue. The cross-cutting injection clause (#117 STEP 5) is deliberately NOT added here -- per the plan's PARALLEL section, STEP 5 lands after STEP 8's recordings exist, edited into all three files identically at that point. Updates test_run_dimensions.py's real-directory list-mode test, which asserted the (previously true) empty dimensions/ directory; it now asserts the three real slugs this step adds, so the test stays a live check of the actual on-disk state rather than a stale fixture. Reviewed independently (serina:review-code) before commit: one Medium finding (a contradiction between secrets-and-access.py and claim-vs-evidence.py over whether a false 'no secrets added' claim also produces a claim-vs-evidence finding) was found and fixed -- both dimensions now agree it is two independent findings, matching the pattern used elsewhere in these files. Stacked on feat/review-agent-dimensions (#117 STEP 3, open as PR #241, unmerged) since that PR was still pending review. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
STEP 6's done-when requires that the PROCESS exits non-zero when a dimension fails, times out, or produces invalid output -- not only that build_document() returns the right document. STEP 3's own tests already covered isolation, timeout handling, and concurrency thoroughly at the build_document() level (ReviewerFailureTests, HungReviewerDaemonThreadTests, ConcurrencyTests), but main() itself never exposed a way to inject a failing reviewer, so its own exit-code decision (all(...) over report statuses -> EXIT_OK/EXIT_DIMENSION_ FAILED) was untestable end-to-end: patching the module-level default_reviewer name doesn't reach build_document's already-bound default parameter, since Python binds a default argument once, at function-definition time. Adds a keyword-only parameter to main(), threaded through to its build_document() call, deliberately NOT reachable from argv -- #117 puts choosing a model out of scope, and this keeps that true of the CLI surface; only a Python-level caller (i.e. a test) can override it. Three new tests in DimensionFailureExitCodeWiringTests exercise main() itself via --payload (no network): one raising reviewer -> EXIT_DIMENSION_FAILED with two complete/one failed reports; one timing-out reviewer -> same, and confirmed non-blocking; one all-clean control case -> EXIT_OK, so the other two prove something beyond 'always FAILED'. Reviewed independently (serina:review-code) before commit: confirmed the seam is unreachable from the CLI parser, confirmed by mechanical revert-and-rerun that the two failure-path tests actually depend on the fix (TypeError without it), and one Low docstring-wording fix applied (mislabeled early-binding default-argument behavior as 'late-binding'). Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
serina-mcfall
changed the title
This was referenced Aug 20, 2026
Adds launchpad/review-agent/fixtures/dimensions/{secrets-and-access,
claim-vs-evidence,correctness-and-failure-modes,paraphrase,
description-of-an-attack}.json -- the STEP 7 fixtures for #117's plan.
Three fixtures each plant one realistic defect in a unified-diff pr_diff
field that exactly one dimension must find and the other two must not
(secrets-and-access: a plausible credential in scripts/deploy.sh;
claim-vs-evidence: a PR body claiming validated-choices flag behavior a
plain boolean flag doesn't have, plus a cited nonexistent path;
correctness-and-failure-modes: a widened except clause that turns a
malformed-manifest rejection into a silent pass). A fourth (paraphrase)
plants a semantic paraphrase of a skip-review attempt in a code comment --
one of the 7 attack-matrix classes detect.py's deterministic layer misses
by design -- which must be found by all three dimensions once STEP 5's
injection clause exists (not yet built; STEP 5 is a later, separate step
per the plan's own PARALLEL section). A fifth (description-of-an-attack)
quotes CONTAINMENT.md's own Severity-contract sentence verbatim as a
negative control: prose describing an attack, not one, that must produce
zero findings anywhere -- the use-mention problem both CONTAINMENT.md and
detect.py's docstrings already name.
Each fixture carries a _fixture metadata block (planted_entry_point,
planted_file/planted_line or an explicit no-location note, must_find,
must_not_find) per STEP 7's done-when, and is a valid --payload input to
run_dimensions.py, producing a merged document findings.validate() accepts.
test_fixtures.py is a permanent, committed witness (matching test_findings.py
and test_run_dimensions.py's own convention) -- 15 tests covering: file
existence, payload loading, metadata shape, that each declared planted line
is independently re-derivable from the diff's own hunk-header arithmetic
(not merely asserted), unified-diff hunk-header integrity across all five
fixtures, and the detector-facing claims (paraphrase evades detect.py,
description-of-an-attack produces zero findings on every surface).
Reviewed independently (serina:review-code) before commit: two Medium
findings were found and fixed -- description-of-an-attack.json's hunk header
declared more lines than its body had (an invalid unified diff no test
caught, since it's the one fixture without a location marker to check
against); and secrets-and-access.json's planted credential literally spelled
'obviously-fake-not-real' in the value itself, which matches its own
dimension's Medium severity rubric rather than the Blocker case the fixture
is meant to exercise. Both fixed and re-reviewed clean; the second fix also
avoids any known real-provider token prefix (AWS/GitHub/Stripe/Google/Slack)
that could trip GitHub's own secret-scanning partner program on push.
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ns (#117 STEP 5) Adds an identical INJECTION_CLAUSE to dimensions/{secrets-and-access, claim-vs-evidence,correctness-and-failure-modes}.py: author-controlled text attempting to skip/approve/suppress/end the review is itself a Blocker finding, with entry_point set to the surface it came from. One dimension failing to run therefore never drops semantic-injection coverage to zero silently, per #117's own requirement. Covers the 7 of 35 attack-matrix classes CONTAINMENT.md's Detection section hands to #117 by name -- semantic paraphrase, which detect.py's deterministic layer does not and cannot catch by design. The clause is phrased to avoid the use-mention trap detect.py's own docstring names (it does not itself trip _STANDALONE or _SUPPRESS), and explicitly states it overrides every dimension's own subject-matter/language exclusions -- otherwise an attack planted in, say, a Rust file would be silently declined by a dimension whose own scope says it reviews no Rust code, exactly the gap STEP 7's paraphrase fixture (planted in crates/buzz-relay/src/gate.rs) exists to catch. test_injection_clause.py is a permanent test covering the half of STEP 5's done-when checkable without a live model: byte-identity across all three files, real weaving into each assembled PROMPT (not merely defined and unused), and that neither the clause alone nor the full assembled prompt trips the deterministic detector. The other half -- that the paraphrase fixture actually yields a Blocker from all three dimensions and the description-of-an-attack fixture yields none -- is a property of real reviewer output, which is STEP 8's job, not simulated here. Reviewed independently (serina:review-code) before commit: two High findings were found and fixed -- the clause as first written did not override correctness-and-failure-modes' unconditional 'reports nothing about Rust/TS/ Dart' exclusion, which would have made it decline the paraphrase fixture specifically; and an ambiguous parenthetical risked being read as 'skip-review is already handled elsewhere, do not re-detect it,' which could cause a model to withhold the Blocker on a PARAPHRASE (the exact case this clause exists for) rather than only on the literal wording detect.py already catches. Also fixed a Low finding: added an explicit single-report rule for the overlap between an injection attempt phrased as 'a claim of prior approval' and claim-vs-evidence's own ordinary scope. All three re-reviewed clean. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
tucktuck101
merged commit Aug 20, 2026
e09cb63
into
feat/review-agent-dimensions
21 of 22 checks passed
This was referenced Aug 20, 2026
Merged
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
2 participants
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.
Summary
Adds the three dimension definition files (
secrets-and-access,claim-vs-evidence,correctness-and-failure-modes) underlaunchpad/review-agent/dimensions/per STEP 4 of #117's plan, plus a STEP 6 fix:main()now exposes an injectablereviewerseam so its own exit-code wiring for a failing/timed-out dimension is testable end-to-end, closing a gap STEP 3's own tests left at thebuild_document()level.Related issue
Refs #117
Issue type
Task
Agent provenance
Objective
Three files under
launchpad/review-agent/dimensions/specifying each review dimension's scope/exclusions/severity/anchoring/output contract (STEP 4), plus a keyword-onlyreviewerparameter onrun_dimensions.main()and three new tests proving its exit-code decision is correct end-to-end (STEP 6).Impacted components
launchpad/review-agent/dimensions/secrets-and-access.py (new)
launchpad/review-agent/dimensions/claim-vs-evidence.py (new)
launchpad/review-agent/dimensions/correctness-and-failure-modes.py (new)
launchpad/review-agent/dimensions/.gitkeep (removed - directory is no longer empty)
launchpad/review-agent/run_dimensions.py (main() gains a
reviewerkeyword parameter)launchpad/review-agent/test_run_dimensions.py (one STEP 4 test updated, one new STEP 6 test class)
Approach and rejected alternatives
STEP 4. Wrote all three dimension files directly rather than dispatching one parallel subagent per file — rejected the parallel-authorship approach because each dimension's exclusions section must accurately describe what the other two dimensions cover, and three independently-written files risk exactly the drift an independent review then found: a contradiction between two files over whether the same PR-body-false-claim scenario produces one finding or two (found and fixed before commit). Deliberately did NOT add #117 STEP 5's cross-cutting injection clause — the plan's own PARALLEL section states STEP 5 lands after STEP 8's recordings exist, not alongside STEP 4.
STEP 6.
main()never exposed a way to inject a failing reviewer, so its exit-code branch (EXIT_OKvsEXIT_DIMENSION_FAILED) had no test exercising it end-to-end —build_document()'s ownreviewerparameter is bound todefault_reviewerat function-definition time, so patching the module-level name after the fact doesn't reach an already-defaulted call. Considered mockingbuild_document()wholesale instead of adding a real parameter — rejected because that would testmain()'s glue logic against a synthetic document rather than one a real (mocked) reviewer actually produced end-to-end. The added parameter is keyword-only and never reachable fromargv, so it does not reopen "choosing a model" as an in-scope CLI concern.Branch/PR organization, disclosed for transparency: the STEP 6 commit depends only on STEP 3's code (
run_dimensions.py/test_run_dimensions.py), not on STEP 4's dimension files — it would have fit more cleanly as an addition to #241 (STEP 3) than stacked here. It ended up on this branch because STEP 6 was picked up after STEP 4 was already committed and pushed. Functionally this is a non-issue (this branch correctly includes all of #241's commits), but flagging it so a reviewer isn't confused about why a "STEP 6" commit sits on a "STEP 4" PR.Verification
Command run:
Raw output:
Command run:
Raw output:
Command run:
Raw output:
(36 lines of individual
... oktest names omitted here for length; all 36 passed, untouched by this PR.)Command run (confirms the merged document validates cleanly with all three real dimensions wired through the stub reviewer):
Raw output:
Not verified
controlscheck's pre-existingcheck_step6.pycorpus false positive (see PR feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241) — still present, still unrelated to this diff.launchpadonce feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241 merges.Security implications
None beyond what #241 already covers. The dimension files add no new code path (static specification text, discoverable by filename only, no injection-resistance clause yet — a documented, deliberate STEP 5 deferral). The
reviewerparameter onmain()is unreachable from any external input (CLI argv, environment, network) — it is a Python-level keyword only a same-process caller (i.e. a test) can set, so it adds no new attacker-reachable surface.Escalations
feat/review-agent-dimensions(PR feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241), notlaunchpad. It should not be merged before feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241, and its diff view includes feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241's commits until feat(launchpad): concurrent dimension runner for the PR review agent (#117 STEP 3) #241 lands and this branch is rebased.