From 8e959ffbff00152341a961350d3fbdd12d01b5df Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 09:17:05 -0700 Subject: [PATCH 1/4] feat(review-suite): add a skill-root override for mechanism ablation runs ## Summary - Add an optional `--skill-root` argument to `runner.py`'s evaluation CLI (and a `skill_root` keyword on `evaluate()`/`target_skill_documents()`/ `target_skill_prompt()`), defaulting to the real `skills/` tree so every existing call site and report is unaffected. - Record the resolved root verbatim as `configuration.skill_root` in every report, so a run against an alternate root can never be mistaken for a standard-configuration one. - Add two eval-only `review-correctness/SKILL.md` overlays under `review-suite/evals/v2/ablation-skill-roots/{traversal-only,verification-only}/`, each disabling exactly one of the two required passes (consumer/impact traversal, verification-sufficiency) for measurement purposes only. Neither overlay is installed, distributed, or referenced by the shipped skill. ## Why - #57 (the epic's acceptance-gate ticket) requires scoring `s1-correctness-orchestrator` in three configurations preregistered by `gate-manifest.json`: traversal-only, verification-sufficiency-only, and both together. The shipped `review-correctness` skill deliberately performs both passes unconditionally, by design (`SKILL.md`: "not routed specialist modules"), so no runtime toggle exists or should be added to the shipped skill itself. - This gives the ablation measurement a way to isolate each pass's contribution without touching the shipped skill, without duplicating the full `skills/` tree in git (only the one changed file per configuration is committed; the rest is mirrored from the real tree at run time), and without changing behavior for any existing caller of `runner.py`. --- .../review-correctness/SKILL.md | 160 +++++++++++++++++ .../review-correctness/SKILL.md | 161 ++++++++++++++++++ review-suite/scripts/evals/runner.py | 59 ++++++- .../scripts/tests/test_eval_runner.py | 33 ++++ 4 files changed, 404 insertions(+), 9 deletions(-) create mode 100644 review-suite/evals/v2/ablation-skill-roots/traversal-only/review-correctness/SKILL.md create mode 100644 review-suite/evals/v2/ablation-skill-roots/verification-only/review-correctness/SKILL.md diff --git a/review-suite/evals/v2/ablation-skill-roots/traversal-only/review-correctness/SKILL.md b/review-suite/evals/v2/ablation-skill-roots/traversal-only/review-correctness/SKILL.md new file mode 100644 index 0000000..178c875 --- /dev/null +++ b/review-suite/evals/v2/ablation-skill-roots/traversal-only/review-correctness/SKILL.md @@ -0,0 +1,160 @@ +--- +name: review-correctness +description: Review a code change, diff, PR, branch, or patch for bugs and material behavioral, security, authorization, compatibility, data-integrity, performance, and validation failures against its stated goal. Use when asked to find bugs or run a correctness-focused review, either from raw repository and ticket evidence or from the repository-owned shared review packet. Returns only the shared finding and verdict shape and never modifies the reviewed candidate. +allowed-tools: Read, Grep, Glob, Bash +--- + +# Review Correctness + +Determine whether the candidate satisfies its observable contract without +introducing a material failure. Review only; leave fixes and workflow mutations +to the caller. + +**ABLATION NOTICE (measurement-only variant, never shipped):** this copy exists +solely to isolate the consumer/impact-traversal pass's own contribution for +`review-suite/evals/v2/gate-manifest.json`'s preregistered `s1` ablation matrix +(`#57`). The verification-sufficiency pass below is deliberately disabled/ no-op +for this measurement configuration only. The real, shipped skill at +`skills/review-correctness/SKILL.md` is unchanged and always performs both +passes; this file is never installed or distributed. + +## Load the contracts + +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and its packet and result schemas beside it. Inside this skill's source + monorepo, the repository-root `review-suite/` directory is the canonical + origin and the bundled copies are kept byte-identical to it. +2. Read [the correctness rubric](references/correctness-rubric.md). +3. Treat the canonical contract as authoritative for required evidence, finding + fields, severity, confidence, verdicts, candidate identity, and base drift. +4. Return `blocked` with the missing dependency when the canonical contract is + unavailable. Do not invent a local replacement. + +## Establish the candidate + +- When given a shared review packet, validate it before reviewing. Reject + malformed structure. Convert missing essential evidence into a conforming + `blocked` result. +- When invoked from raw evidence, construct the packet conceptually before + inspecting implementation details. Establish repository, base, captured head, + complete diff, observable goal, acceptance criteria, explicit non-goals, + preserved behavior, source documents, and focused and full validation. +- Do not infer missing intent from the implementation. Return `blocked` when the + goal, acceptance criteria, candidate identity, complete diff, or required + validation evidence cannot be established. +- Bind the result to the packet's candidate identity. Never reuse evidence after + a head change. Apply the shared base-drift rules when only the base advances. +- When an orchestrator supplies a validated simplification result beside the + packet, assess each gating proposal against the packet's requirements and + preserved behavior. Keep the packet itself unchanged. + +## Review in priority order + +1. Compare the observable goal, acceptance criteria, non-goals, and preserved + behavior with the complete candidate diff. +2. Read applicable repository instructions, named architecture or contract + documents, representative nearby implementation, and nearby tests. +3. Inspect security and authorization boundaries before lower-risk behavior. +4. Inspect applicable behavioral, failure, concurrency, data-integrity, + compatibility, validation, and material performance dimensions from the + rubric. +5. Prefer explicit repository rules and demonstrated local idioms over generic + advice. +6. Check that tests and exact validation evidence prove success, failure, + regression, and preserved behavior required by the change contract. + +Do not mechanically emit every category. Follow evidence into the dimensions +that can materially affect this candidate. + +## Perform the required traversal pass only (measurement configuration) + +Perform this pass on every review and record its evidence in the shared result. +It is a dimension of this single correctness lens, not a routed specialist +module; do not build or delegate to a separate security, +concurrency-as-a-context, compatibility/migration, operations, or UI module. + +1. **Consumer/impact-traversal pass.** For each changed shared symbol, public + contract, or behavior toggle, search the repository for other call sites, + importers, or consumers — including ones the candidate diff does not touch. + Inspect material negative space: unchanged consumers or paths that must still + honor the changed contract. Record one `consumer_impact_evidence` entry per + changed symbol that requires one: the `changed_symbol`, its defining + `location`, one or more concrete `consumer_search_evidence` items describing + what the search actually found, and a `disposition` of + `all_consumers_consistent`, `inconsistency_found`, or `no_other_consumers`. A + disposition claiming other consumers exist requires evidence covering more + than the changed symbol's own location. Treat a genuine inconsistency as a + blocking correctness finding; recording the disposition alone does not + satisfy the review. + +**This measurement configuration deliberately does not perform the +verification-sufficiency pass.** Do not ask whether a claimed validation command +or test would actually fail for its triggering condition, do not record any +`verification_sufficiency_evidence` entry, and do not raise a finding whose sole +basis is that pass. This is a controlled ablation, not a judgment that the pass +is unnecessary. + +## Apply the finding threshold + +Raise a finding only when concrete ticket, code, test, repository, or runtime +evidence demonstrates a material current concern and supports a smallest +sufficient correction. + +- Mark a demonstrated correctness, security, authorization, acceptance, + architecture, compatibility, or validation failure `blocking`. +- Use `strong_recommendation` only for a material, tractable, ticket-scoped + safety improvement with demonstrated current risk. +- Use `defer` only for a real, evidenced concern intentionally outside the + active ticket or dependent on a missing decision. +- Omit style, praise, broad modernization, numerical quality rules, speculative + hardening, imagined compatibility, and generic best-practice advice. + +Do not perform the dedicated whole-solution or local code-simplicity lenses. +Report correctness consequences of complexity only when they create a concrete +failure or make required behavior unprovable. + +## Disposition supplied simplification proposals + +For every gating proposal supplied by the orchestrator, return one shared +`proposal_dispositions` item: + +- use `compatible` when the proposal preserves demonstrated correctness; or +- use `unsafe` when concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Do not turn a rejected hypothetical edit into a candidate correctness finding. A +proposal disposition does not change the correctness verdict by itself. If the +proposal cannot be assessed from trustworthy evidence, return `blocked`. + +## Return the shared result + +Return only JSON conforming to the bundled +[review-result schema](references/review-suite/review-result.schema.json) with +lens `correctness`. + +- Return `clean` when no blocking or strong-recommendation finding remains. +- Return `changes_required` when at least one actionable gating finding remains. +- Return `blocked` when essential evidence or a product or architecture decision + prevents a trustworthy verdict. +- Keep deferred findings non-gating. +- Include proposal dispositions when the orchestrator supplied simplification + proposals, using the shared contract shape. +- Include a `consumer_impact_evidence` entry for every changed symbol the + traversal pass above examined. Omit an entry only when the pass genuinely + found nothing applicable to record. Do not include any + `verification_sufficiency_evidence` entry in this measurement configuration. +- Do not add praise, a scorecard, generic resources, or prose outside the shared + result. + +## Preserve read-only integrity + +Do not edit or format files, create repository artifacts, commit, push, resolve +threads, post reviews, or update tickets. Run only safe read-only inspection and +validation commands. Runtimes that support tool restriction should enforce the +`allowed-tools` frontmatter, which excludes file-editing tools. The shell +remains necessary for validation commands and can still mutate files, so prefer +a sandboxed or deny-write shell where available; the recorded before/after +candidate state is the authoritative integrity check. When the caller supplies +pre-review candidate state, preserve it exactly and report any unexpected +mutation as an integrity failure. diff --git a/review-suite/evals/v2/ablation-skill-roots/verification-only/review-correctness/SKILL.md b/review-suite/evals/v2/ablation-skill-roots/verification-only/review-correctness/SKILL.md new file mode 100644 index 0000000..4056ad2 --- /dev/null +++ b/review-suite/evals/v2/ablation-skill-roots/verification-only/review-correctness/SKILL.md @@ -0,0 +1,161 @@ +--- +name: review-correctness +description: Review a code change, diff, PR, branch, or patch for bugs and material behavioral, security, authorization, compatibility, data-integrity, performance, and validation failures against its stated goal. Use when asked to find bugs or run a correctness-focused review, either from raw repository and ticket evidence or from the repository-owned shared review packet. Returns only the shared finding and verdict shape and never modifies the reviewed candidate. +allowed-tools: Read, Grep, Glob, Bash +--- + +# Review Correctness + +Determine whether the candidate satisfies its observable contract without +introducing a material failure. Review only; leave fixes and workflow mutations +to the caller. + +**ABLATION NOTICE (measurement-only variant, never shipped):** this copy exists +solely to isolate the verification-sufficiency pass's own contribution for +`review-suite/evals/v2/gate-manifest.json`'s preregistered `s1` ablation matrix +(`#57`). The consumer/impact-traversal pass below is deliberately disabled/ +no-op for this measurement configuration only. The real, shipped skill at +`skills/review-correctness/SKILL.md` is unchanged and always performs both +passes; this file is never installed or distributed. + +## Load the contracts + +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and its packet and result schemas beside it. Inside this skill's source + monorepo, the repository-root `review-suite/` directory is the canonical + origin and the bundled copies are kept byte-identical to it. +2. Read [the correctness rubric](references/correctness-rubric.md). +3. Treat the canonical contract as authoritative for required evidence, finding + fields, severity, confidence, verdicts, candidate identity, and base drift. +4. Return `blocked` with the missing dependency when the canonical contract is + unavailable. Do not invent a local replacement. + +## Establish the candidate + +- When given a shared review packet, validate it before reviewing. Reject + malformed structure. Convert missing essential evidence into a conforming + `blocked` result. +- When invoked from raw evidence, construct the packet conceptually before + inspecting implementation details. Establish repository, base, captured head, + complete diff, observable goal, acceptance criteria, explicit non-goals, + preserved behavior, source documents, and focused and full validation. +- Do not infer missing intent from the implementation. Return `blocked` when the + goal, acceptance criteria, candidate identity, complete diff, or required + validation evidence cannot be established. +- Bind the result to the packet's candidate identity. Never reuse evidence after + a head change. Apply the shared base-drift rules when only the base advances. +- When an orchestrator supplies a validated simplification result beside the + packet, assess each gating proposal against the packet's requirements and + preserved behavior. Keep the packet itself unchanged. + +## Review in priority order + +1. Compare the observable goal, acceptance criteria, non-goals, and preserved + behavior with the complete candidate diff. +2. Read applicable repository instructions, named architecture or contract + documents, representative nearby implementation, and nearby tests. +3. Inspect security and authorization boundaries before lower-risk behavior. +4. Inspect applicable behavioral, failure, concurrency, data-integrity, + compatibility, validation, and material performance dimensions from the + rubric. +5. Prefer explicit repository rules and demonstrated local idioms over generic + advice. +6. Check that tests and exact validation evidence prove success, failure, + regression, and preserved behavior required by the change contract. + +Do not mechanically emit every category. Follow evidence into the dimensions +that can materially affect this candidate. + +## Perform the required verification-sufficiency pass only (measurement configuration) + +Perform this pass on every review and record its evidence in the shared result. +It is a dimension of this single correctness lens, not a routed specialist +module; do not build or delegate to a separate security, +concurrency-as-a-context, compatibility/migration, operations, or UI module. + +1. **Verification-sufficiency pass.** For each claimed validation command or + test that touches a materially risky change, ask whether it would actually + fail for the specific triggering condition the change addresses — not merely + whether it passes. Restate the relevant acceptance or preserved-behavior + contract and inspect the changed or added tests before concluding. Detect + mocks, fixtures, stubs, happy-path assertions, or partial-suite selection + that bypass the risky behavior, including a test that exercises an + already-safe branch instead of the actual triggering condition. Record one + `verification_sufficiency_evidence` entry per claimed test or command + inspected under this pass: the `claimed_test_or_command`, whether it + `exercises_material_risk` (`yes`, `no`, or `not_applicable`), and `reasoning` + naming the specific condition exercised or missed. Treat a claimed test that + does not exercise the material risk as a blocking finding; never let it back + a `clean` verdict. + +**This measurement configuration deliberately does not perform the +consumer/impact-traversal pass.** Do not search for other call sites, importers, +or consumers of a changed shared symbol, do not record any +`consumer_impact_evidence` entry, and do not raise a finding whose sole basis is +that pass. This is a controlled ablation, not a judgment that the pass is +unnecessary. + +## Apply the finding threshold + +Raise a finding only when concrete ticket, code, test, repository, or runtime +evidence demonstrates a material current concern and supports a smallest +sufficient correction. + +- Mark a demonstrated correctness, security, authorization, acceptance, + architecture, compatibility, or validation failure `blocking`. +- Use `strong_recommendation` only for a material, tractable, ticket-scoped + safety improvement with demonstrated current risk. +- Use `defer` only for a real, evidenced concern intentionally outside the + active ticket or dependent on a missing decision. +- Omit style, praise, broad modernization, numerical quality rules, speculative + hardening, imagined compatibility, and generic best-practice advice. + +Do not perform the dedicated whole-solution or local code-simplicity lenses. +Report correctness consequences of complexity only when they create a concrete +failure or make required behavior unprovable. + +## Disposition supplied simplification proposals + +For every gating proposal supplied by the orchestrator, return one shared +`proposal_dispositions` item: + +- use `compatible` when the proposal preserves demonstrated correctness; or +- use `unsafe` when concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Do not turn a rejected hypothetical edit into a candidate correctness finding. A +proposal disposition does not change the correctness verdict by itself. If the +proposal cannot be assessed from trustworthy evidence, return `blocked`. + +## Return the shared result + +Return only JSON conforming to the bundled +[review-result schema](references/review-suite/review-result.schema.json) with +lens `correctness`. + +- Return `clean` when no blocking or strong-recommendation finding remains. +- Return `changes_required` when at least one actionable gating finding remains. +- Return `blocked` when essential evidence or a product or architecture decision + prevents a trustworthy verdict. +- Keep deferred findings non-gating. +- Include proposal dispositions when the orchestrator supplied simplification + proposals, using the shared contract shape. +- Include a `verification_sufficiency_evidence` entry for every claimed test or + command the pass above examined. Omit an entry only when the pass genuinely + found nothing applicable to record. Do not include any + `consumer_impact_evidence` entry in this measurement configuration. +- Do not add praise, a scorecard, generic resources, or prose outside the shared + result. + +## Preserve read-only integrity + +Do not edit or format files, create repository artifacts, commit, push, resolve +threads, post reviews, or update tickets. Run only safe read-only inspection and +validation commands. Runtimes that support tool restriction should enforce the +`allowed-tools` frontmatter, which excludes file-editing tools. The shell +remains necessary for validation commands and can still mutate files, so prefer +a sandboxed or deny-write shell where available; the recorded before/after +candidate state is the authoritative integrity check. When the caller supplies +pre-review candidate state, preserve it exactly and report any unexpected +mutation as an integrity failure. diff --git a/review-suite/scripts/evals/runner.py b/review-suite/scripts/evals/runner.py index 38b5c0b..facad37 100644 --- a/review-suite/scripts/evals/runner.py +++ b/review-suite/scripts/evals/runner.py @@ -49,13 +49,13 @@ def _now() -> str: return datetime.now(timezone.utc).isoformat(timespec="seconds") -def _skill_documents(skill: str) -> dict[str, str]: +def _skill_documents(skill: str, skill_root: Path) -> dict[str, str]: """One skill's reviewer-visible Markdown, namespaced by skill name. The bundled `references/review-suite/` mirror is excluded: `contract_documents` already supplies those files from their canonical location. """ - root = TARGET_SKILL_ROOT / skill + root = skill_root / skill skill_md = root / "SKILL.md" if not skill_md.is_file(): raise ConfigurationError(f"missing target skill prompt {skill_md}") @@ -71,7 +71,10 @@ def _skill_documents(skill: str) -> dict[str, str]: def target_skill_documents( - target_skill: str, dependencies: Sequence[str] = () + target_skill: str, + dependencies: Sequence[str] = (), + *, + skill_root: Path = TARGET_SKILL_ROOT, ) -> dict[str, str]: """Return the target skill's declared dependency closure as text. @@ -95,12 +98,20 @@ def target_skill_documents( The corpus declares the closure so a target can be swapped without changing this code. Which target a scored corpus should measure, and the cost envelope that follows from its closure, are corpus-composition decisions. + + `skill_root` defaults to the repository's real, shipped `skills/` tree. + A caller may point it at an alternate directory that mirrors that tree with + one skill's `SKILL.md` deliberately altered - the only supported way to run + a mechanism ablation (for example, a pass disabled/no-op) without editing + the shipped skill itself. The override is recorded verbatim in the report's + `configuration.skill_root`, so an ablation run can never be mistaken for a + standard-configuration one. """ - documents = _skill_documents(target_skill) + documents = _skill_documents(target_skill, skill_root) for dependency in dependencies: if dependency == target_skill: raise ConfigurationError(f"{target_skill} declares itself as a dependency") - documents.update(_skill_documents(dependency)) + documents.update(_skill_documents(dependency, skill_root)) return documents @@ -119,7 +130,12 @@ def render_skill_prompt(target_skill: str, documents: Mapping[str, str]) -> str: return "".join(sections) -def target_skill_prompt(target_skill: str, dependencies: Sequence[str] = ()) -> str: +def target_skill_prompt( + target_skill: str, + dependencies: Sequence[str] = (), + *, + skill_root: Path = TARGET_SKILL_ROOT, +) -> str: """Render the target skill and its whole declared closure as one prompt. `prompt_digest` hashes this string, so the recorded `target_skill_digest` @@ -127,7 +143,8 @@ def target_skill_prompt(target_skill: str, dependencies: Sequence[str] = ()) -> whole point of pinning it across baseline strata. """ return render_skill_prompt( - target_skill, target_skill_documents(target_skill, dependencies) + target_skill, + target_skill_documents(target_skill, dependencies, skill_root=skill_root), ) @@ -291,8 +308,18 @@ def evaluate( timeout: float, max_output_bytes: int, artifact_dir: Path | None, + skill_root: Path = TARGET_SKILL_ROOT, ) -> tuple[list[dict[str, Any]], dict[str, Any]]: - """Audit the corpus, then replay every case the configured number of times.""" + """Audit the corpus, then replay every case the configured number of times. + + `skill_root` defaults to the repository's real `skills/` tree. Passing an + alternate directory is the only supported way to run a mechanism ablation + (a pass deliberately disabled/no-op in one skill's `SKILL.md`) without + editing the shipped skill; see `target_skill_documents` for the full + contract. The resolved root is always recorded in the returned + `configuration`, so an ablation run is never indistinguishable from a + standard-configuration one. + """ loaded = corpus.load_corpus(corpus_root) if loaded.grader_version != grader.GRADER_VERSION: raise corpus.CorpusError( @@ -303,7 +330,7 @@ def evaluate( # two runs sent the same text but cannot say what that text was, and a # baseline stratum has to be able to state which skills it evaluated. closure = target_skill_documents( - loaded.target_skill, loaded.target_skill_dependencies + loaded.target_skill, loaded.target_skill_dependencies, skill_root=skill_root ) skill_prompt = render_skill_prompt(loaded.target_skill, closure) documents = contract_documents() @@ -384,6 +411,7 @@ def refuse_if_contaminated(case: corpus.Case, request: dict[str, Any]) -> None: "target_skill_dependencies": list(loaded.target_skill_dependencies), "target_skill_documents": sorted(closure), "target_skill_digest": protocol.prompt_digest(skill_prompt), + "skill_root": str(skill_root), "suite_commit": commit, # The stratum the corpus declares, carried verbatim. A report that names # its target and closure but not its stratum cannot say which ground @@ -419,6 +447,18 @@ def build_parser() -> argparse.ArgumentParser: help="Fresh-process command receiving one result-blind JSON request on stdin", ) parser.add_argument("--corpus", type=Path, default=None) + parser.add_argument( + "--skill-root", + type=Path, + default=None, + help=( + "Directory mirroring the repository's skills/ tree, used instead of " + "it. The only supported way to run a mechanism ablation (a pass " + "deliberately disabled/no-op in one skill's SKILL.md) without " + "editing the shipped skill. Defaults to the real skills/ tree; " + "always recorded verbatim in the report's configuration.skill_root." + ), + ) parser.add_argument("--runs", type=int, default=1) parser.add_argument("--timeout", type=float, default=DEFAULT_TIMEOUT_SECONDS) parser.add_argument( @@ -469,6 +509,7 @@ def main(argv: list[str] | None = None) -> int: timeout=args.timeout, max_output_bytes=args.max_output_bytes, artifact_dir=args.artifact_dir, + skill_root=args.skill_root or TARGET_SKILL_ROOT, ) except (ConfigurationError, corpus.CorpusError, grader.GradingError) as error: print(f"evaluation rejected: {error}", file=sys.stderr) diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index 9176050..3d62e1f 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -125,6 +125,39 @@ def test_the_run_records_which_closure_was_sent(self): attempt["target_skill_digest"], ) + def test_skill_root_defaults_to_the_real_skills_tree(self): + _, configuration = self.evaluate() + self.assertEqual(str(runner.TARGET_SKILL_ROOT), configuration["skill_root"]) + + def test_an_overridden_skill_root_is_recorded_and_changes_the_closure(self): + """The only supported way to run a mechanism ablation. + + Mirroring the target skill under an alternate root with a deliberately + altered `SKILL.md` must change what gets sent and must be visible in + the report, so an ablation run can never be mistaken for a standard one. + """ + baseline_attempts, baseline_configuration = self.evaluate() + mirrored_root = self.temp / "ablation-skill-root" + shutil.copytree(runner.TARGET_SKILL_ROOT, mirrored_root) + mirrored_target = mirrored_root / "review-code-simplicity" + altered = (mirrored_target / "SKILL.md").read_text() + "\n\n" + (mirrored_target / "SKILL.md").write_text(altered) + + attempts, configuration = self.evaluate(skill_root=mirrored_root) + + self.assertEqual(str(mirrored_root), configuration["skill_root"]) + self.assertNotEqual( + baseline_configuration["target_skill_digest"], + configuration["target_skill_digest"], + ) + self.assertIn( + "ablated", + runner.target_skill_documents( + "review-code-simplicity", skill_root=mirrored_root + )["review-code-simplicity/SKILL.md"], + ) + self.assertEqual(len(baseline_attempts), len(attempts)) + def test_only_valid_review_results_are_graded(self): attempts, _ = self.evaluate() for attempt in attempts: From e2c56f68fe56094a6c92fd4a220539f47d6f9f98 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 09:31:12 -0700 Subject: [PATCH 2/4] docs: add the CHANGELOG entry for the skill-root ablation override ## Summary - Backfill 16fc32a's SHA onto the previous top entry and add a new bullet for the skill-root ablation override commit, per AGENTS.md's changelog convention. ## Why - review-code-change's initial pass on this checkpoint flagged the missing entry as a gating strong_recommendation finding; this is the exact, smallest fix. --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d26b9e3..128e0bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,11 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, and added connector-outcome curation and promotion tooling +## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, added connector-outcome curation and promotion tooling, and added a skill-root override for mechanism ablation runs +- feat(review-suite): add a skill-root override for mechanism ablation runs - refactor(review-suite): simplify duplicate-chain resolution and unify its - membership check + membership check (`16fc32a90eaea16ac98ff2a34bbabafed7a4681f`) - fix(review-suite): resolve a duplicate's disposition through its duplicate_of chain (`07baa7dfdf06bfa19428bb9ba80a8317f8ff78d0`) - feat(review-suite): add connector-outcome curation and promotion tooling, From b59741659c07d9a05a9a12e59c46c0aeca34fac5 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 11:52:22 -0700 Subject: [PATCH 3/4] docs(review-suite): run the preregistered v2 s1 ablation matrix and integration closeout (#57) ## Summary - Score `s1-correctness-orchestrator` in the two ablation configurations `gate-manifest.json` still needed (traversal-pass-only, verification-sufficiency-pass-only), 35 attempts each under the fixed (`1.1`) grader, using the new `--skill-root` override. Reuse the already-run "both passes together" configuration and the `s2`/`s3` non-regression figures from predecessor measurement tasks in this epic rather than re-spending on them. - Add `FROZEN-V2-CONFIGURATION.md` (the exact scored configuration, the reviewed grader-version transition, and the ablation mechanism), `S1-ABLATION-MATRIX.md` (the three-configuration comparison against the settled per-case gate, including the honest finding that neither pass demonstrates unique causal contribution to its own target case, and the real non-regression floor violation found under the isolated verification-only configuration), `DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md` (every required deterministic gate and caller-integration scenario cited against its exact existing #51-#56 test), and `CLOSEOUT-REPORT.md` (the full synthesis, mechanism-by-mechanism recommendation, residual risks, and spend against the $20 ceiling). - Update `review-suite/evals/v2/README.md` and the top-level `README.md` to point at the new scored closeout documents. ## Why - #57 is the epic's (#49) acceptance-gate ticket: it must run the preregistered scored comparison, verify deterministic and caller-integration invariants, and report - per mechanism - whether it earned its complexity. This candidate does the reporting; it does not remove any mechanism or file any follow-up issue, since #57 is explicitly withheld that authority. ## Spend - $7.44 of this ticket's own $20.00 ceiling ($0.009 runtime/model sanity check + $3.560 traversal-only + $3.867 verification-only); $12.56 headroom never approached. --- CHANGELOG.md | 7 +- README.md | 11 + review-suite/evals/v2/CLOSEOUT-REPORT.md | 224 ++++++++++ .../DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md | 81 ++++ .../evals/v2/FROZEN-V2-CONFIGURATION.md | 144 ++++++ review-suite/evals/v2/README.md | 49 +- review-suite/evals/v2/S1-ABLATION-MATRIX.md | 161 +++++++ ...trator-ablation-traversal-only.report.json | 422 ++++++++++++++++++ ...tor-ablation-verification-only.report.json | 413 +++++++++++++++++ 9 files changed, 1500 insertions(+), 12 deletions(-) create mode 100644 review-suite/evals/v2/CLOSEOUT-REPORT.md create mode 100644 review-suite/evals/v2/DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md create mode 100644 review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md create mode 100644 review-suite/evals/v2/S1-ABLATION-MATRIX.md create mode 100644 review-suite/evals/v2/s1-correctness-orchestrator-ablation-traversal-only.report.json create mode 100644 review-suite/evals/v2/s1-correctness-orchestrator-ablation-verification-only.report.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 128e0bf..f247774 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,14 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, added connector-outcome curation and promotion tooling, and added a skill-root override for mechanism ablation runs +## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, added connector-outcome curation and promotion tooling, added a skill-root override for mechanism ablation runs, and ran the preregistered v2 ablation and integration closeout +- docs(review-suite): run the preregistered v2 s1 ablation matrix and + integration closeout (#57) +- docs: add the CHANGELOG entry for the skill-root ablation override + (`e2c56f68fe56094a6c92fd4a220539f47d6f9f98`) - feat(review-suite): add a skill-root override for mechanism ablation runs + (`8e959ffbff00152341a961350d3fbdd12d01b5df`) - refactor(review-suite): simplify duplicate-chain resolution and unify its membership check (`16fc32a90eaea16ac98ff2a34bbabafed7a4681f`) - fix(review-suite): resolve a duplicate's disposition through its duplicate_of diff --git a/README.md b/README.md index 49a6836..67532ca 100644 --- a/README.md +++ b/README.md @@ -246,6 +246,17 @@ quoting any figure — in particular, **the connector stratum is deferred, not satisfied**, so connector-escape recall has never been measured and no human-review figure may be reported as a connector figure. +The v2 scored closeout lives in `review-suite/evals/v2/`: the preregistered gate +manifest and decision record from `#59`, and `#57`'s own scored ablation +comparison — three independently-measured `s1-correctness-orchestrator` +configurations (each required pass isolated, then both together) plus the +reused, non-regressed `s2`/`s3` figures, each checked against the settled +per-case gate. It reports what clean proves, what it does not prove (in +particular, whether a passing numeric gate demonstrates a mechanism's own unique +causal contribution — it does not, on the evidence recorded there), and a +recommendation, not a removal decision, per mechanism. See +[its README](review-suite/evals/v2/README.md) for the full document set. + ### Connector-outcome curation and promotion `review-suite/evals/curation/` turns a newly adjudicated connector finding into diff --git a/review-suite/evals/v2/CLOSEOUT-REPORT.md b/review-suite/evals/v2/CLOSEOUT-REPORT.md new file mode 100644 index 0000000..a6cf071 --- /dev/null +++ b/review-suite/evals/v2/CLOSEOUT-REPORT.md @@ -0,0 +1,224 @@ +# #57 closeout report: preregistered review v2 ablation and integration verification + +This is the epic's (#49) acceptance-gate measurement. It reports, per mechanism, +whether it earned its complexity, per #57's own charge. It does not remove any +mechanism, does not file any follow-up issue, and does not close #49 - those are +explicitly withheld authorities for this ticket; every disposition below is a +**recommendation for the repository owner**, not an action this ticket took. + +This record, `FROZEN-V2-CONFIGURATION.md`, `S1-ABLATION-MATRIX.md`, and +`DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md` are the complete change surface. +None of them modifies `review-suite/evals/baseline/v1/`, `gate-manifest.json`, +`DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, or `audits/` - all remain exactly +as `#58`/`#59` delivered them (verified: `git diff` against those paths is empty +for this candidate). + +## Required inputs (all present; none missing, inconsistent, or silently changed) + +- Frozen v1 suite commit, corpus version, runtime/model stratum, executor + version, grading policy, baseline report - `review-suite/evals/baseline/v1/` + (#58, untouched). +- Versioned v2 decision/gate manifest, written before scored v2 output - + `review-suite/evals/v2/gate-manifest.json` (#59, untouched). +- Final implemented review contract and mechanism set from #51/#52/#53 - + `review-result.schema.json` at `1.3`, `skills/review-correctness/SKILL.md`'s + two required passes - verified present and unchanged in this candidate's + `skills/` tree. +- Migrated caller integration from #55 - `review-code-change`, `babysit-pr`, + `implement-ticket` all consume schema `1.3` (verified: shared + `test_review_gate.py`'s `CLEAN_AGGREGATE` fixture declares + `"schema_version": "1.3"`). +- Operational feedback/corpus workflow from #56 - `review-suite/evals/curation/` + (verified present, tests pass). + +**One reviewed version transition, applied and documented, per #57's own +requirement before deviating from a frozen input:** `gate-manifest.json` +preregistered its thresholds against grader `1.0`. A real defect in +`finding_surfaces()` was found and fixed (`1.1`) by a predecessor measurement +task in this epic, and the repository owner explicitly authorized scoring #57's +own comparison against the fixed grader. See `FROZEN-V2-CONFIGURATION.md`'s +"Grader version transition" section for the full citation trail +(`GRADER-1.1-COMPARABILITY.md`, `s1-rescore-pre-post-comparison.md`, +`s2-s3-grader-1.1-recheck.md`). No scored comparison in this record used the +pre-fix grader. + +## Deterministic gates: all nine pass + +Full evidence, test-by-test, is in +[`DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md`](DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md). +Summary: packet/result/schema/version validation across the full dependency +closure; failed-validation-cannot-produce-clean; incomplete-evidence/stale- +identity/unavailable-review-work-cannot-produce-clean; exact candidate/base +binding; head-changing-fix invalidation; base-drift risk-based equivalence; +legacy-contract rejection; byte-identical bundled contracts; read-only review +contexts - all nine pass on existing #51-#56 test evidence, re-verified against +this exact candidate. `just test` (all skill suites plus 313 +`review-suite/scripts/tests` tests) and `just lint` (ruff, mdformat, +`skills-ref validate` on all 8 skill directories, plugin-packaging validation) +both pass. + +## Integration verification: all required scenarios pass + +Full evidence is in the same document. Clean, changes_required, blocked, stale +handoff, failed validation, base drift, runtime failure, malformed result, and +accepted/rejected connector-regression are each covered by existing, cited +#51-#56 tests exercised through `review-code-change` directly and the shared, +byte-identical `review_gate.py` bundled into both `implement-ticket` and +`babysit-pr`. No genuinely missing scenario was found; no new caller-integration +test was required. Provider-neutral core semantics hold (every skill in the +suite ships both `agents/claude-code.md` and `agents/openai.yaml`); this record +does not require identical runtime adapters for optional provider metadata +beyond that, per #57's own non-goal. + +## Frozen v2 configuration + +Full detail in [`FROZEN-V2-CONFIGURATION.md`](FROZEN-V2-CONFIGURATION.md). +Runtime/model stratum verified unchanged before scoring (`claude` CLI `2.1.92`, +`claude-opus-4-6[1m]`, matching `gate-manifest.json`'s pin exactly). Grader +`1.1` throughout, per the reviewed transition above. + +## Scored ablation results + +Full detail, per-case gate results, and the honest unique-contribution finding +are in [`S1-ABLATION-MATRIX.md`](S1-ABLATION-MATRIX.md). Summary: + +- **`s2-solution-simplicity-lens`, `s3-code-simplicity-lens`:** all 8 cases (4 + material + 4 control, across both strata) reused from + `s2-s3-grader-1.1-recheck.md` (already scored under grader `1.1` at a + `suite_commit` whose relevant skill trees are byte-identical to this ticket's + own candidate) - all replay with identical statuses to v1, per + `gate-manifest.json`'s own non-regression requirement. Not re-run; no new + spend. +- **`s1-correctness-orchestrator`, three configurations, each scored + independently (not averaged):** + - **Both passes together** (reused from a predecessor task's post-#53 run at + this same skill content, grader `1.1`): both target cases + (`dependency-strictness-propagation`, `stale-claim-release-guard`) pass the + settled gate via the guarded combined-recall path + (`mean_combined_recall: 1.0`); all five non-regression floor cases hold. + - **Traversal-pass-only** (new, this ticket, $3.560, 35 attempts, zero + evaluation failures): `dependency-strictness-propagation` passes; all floors + hold; `optional-tool-probe` (not gated) shows one false positive and + `mean_recall 0.6`, both within its explicitly ungated range. + - **Verification-sufficiency-pass-only** (new, this ticket, $3.867, 35 + attempts, zero evaluation failures): `stale-claim-release-guard` passes; + **`session-continuation-summary`'s non-regression floor is violated** (3/5 + false positives against a required 0/5) - a real, disclosed quality + regression specific to this isolated configuration, not present in the + combined (as-shipped) configuration. + +**The mechanical gate is not the whole story, and this record does not treat it +as such.** Both target cases pass in *every* configuration tested, including the +one where their nominal owning pass is disabled - extending +`s1-rescore-pre-post-comparison.md`'s own prior finding that the pre-#53 +reviewer (no relevant pass at all) already resolved both cases. Across four +independent configurations (pre-#53, traversal-only, verification-only, +both-together), no configuration demonstrates that either specific pass's own +instruction text uniquely causes either case's resolution. Passing the numeric +gate is real and reproducible; unique causal contribution, the dimension #57's +own ablation/removal rule cares about, is not demonstrated by this evidence for +either pass in isolation. + +## Mechanism verdicts (recommendation only - not a removal decision) + +| Mechanism | Deterministic invariant? | Empirical gate | Unique contribution shown? | Non-regression cost found | Recommendation (not a decision) | +| ------------------------------------------- | ---------------------------------------- | ----------------------------------------------------------------------------- | ------------------------------------------- | --------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| #51 (validation/lens-execution cross-check) | Yes - structural, not stochastic | N/A (deterministic only) | N/A | None | **Keep.** Closes a real, demonstrated schema-level contradiction; not ablatable without reintroducing it. | +| #52 (`consumer_impact_evidence` schema) | No | PASS (tied to traversal pass below) | **No**, per the ablation finding above | None | **Owner should weigh simplifying or reconsidering its evidentiary justification.** The schema is harmless and structurally sound, but its sole cited justification (`dependency-strictness-propagation`) is now shown, across four configurations, to resolve independently of it. Recommend revisiting `DECISION-RECORD.md`'s framing rather than removing the schema outright, since #52's own removal rule requires "no other case benefits," which this measurement did not separately test for cases outside `s1`. | +| #53 traversal pass | No | PASS (isolated) | **No** | None found in isolation | **Owner should weigh the same way as #52** - same target case, same caveat. | +| #53 verification-sufficiency pass | No | PASS (isolated) | **No** | **Yes** - a real, isolated-configuration-only false-positive regression on `session-continuation-summary` | **Owner should weigh most cautiously of the four.** Passes its own target case's numeric gate but is the only mechanism this measurement found to carry a demonstrated cost, and that cost appears specifically when it runs without the traversal pass alongside it. | +| #53 both passes combined (as shipped) | No | PASS | Not newly demonstrated; no regression found | None found in the combined configuration | **Keep as currently shipped**, pending the owner's read of the two caveats above - the combined configuration itself shows no false-positive or stability cost on any case measured, and remains the one configuration gate-manifest.json requires to ship. | +| #56 (connector-outcome curation/promotion) | Yes (its own guardrail is deterministic) | Not scored against `s1`/`s2`/`s3` (by its own disposition; a workflow ticket) | N/A | None | **Keep.** Fully covered by its own deterministic test suite (`test_eval_curation.py`); no empirical gate applies to it per `DECISION-RECORD.md`. | + +## Residual material risks and recommended follow-up topics (not filed as issues) + +1. **The `session-continuation-summary` regression under verification-only + deserves a rerun under quiet system conditions before anyone treats it as + settled**, given the heavy unrelated load disclosed above - not because any + execution-level evidence ties the load to it, but because a single 35-attempt + sample is small and a confirming rerun is cheap. +2. **Neither pass's unique contribution to its own target case is demonstrated + by any measurement run so far, across four independent configurations + spanning two different tickets' work.** The owner may want to settle, in + `DECISION-RECORD.md` or a successor document, whether + `dependency-strictness-propagation` and `stale-claim-release-guard` remain + the right justifying cases for #52/#53 at all, or whether a different, more + discriminating corpus case is needed before either mechanism's complexity can + be said to be earned. +3. **`optional-tool-probe` remains an explicitly open, ungated question** (per + `FAILURE-TAXONOMY.md`); its behavior visibly differs across the three `s1` + configurations measured here (`mean_recall` 0.6/0.8/1.0 depending on which + pass runs). Not gated, but a candidate for a future, purpose-built corpus + case if the owner wants to resolve it properly instead of noting it as an + open question indefinitely. +4. **`s2`/`s3`'s two grader-`1.1`-affected cases from + `s2-s3-grader-1.1-recheck.md` (`registry-client-layering`, + `setup-service-path-gateway`) are reused, unchanged evidence here** - this + record does not re-examine whether the grader-`1.1` fix's reach extends + further than already documented; that remains out of scope for #57 + specifically. +5. **`v1`'s own frozen baseline is not re-scored under grader `1.1`** - per + established policy, `baseline/v1/` stays frozen. Any figure in this record + compared against a `baseline/v1/` figure is a `1.1`-vs-`1.0` comparison, + disclosed as such, never presented as apples-to-apples without saying so. + +## Spend against the $20.00 ceiling + +| Item | Cost (USD) | +| ---------------------------------------------------------------------------- | ------------- | +| Runtime/model stratum sanity check (one `claude -p` call, no graded attempt) | 0.00904 | +| `s1` traversal-only (new, 35 attempts) | 3.560108 | +| `s1` verification-only (new, 35 attempts) | 3.866613 | +| **Total spent by this ticket** | **7.435762** | +| Ceiling | 20.00 | +| **Headroom remaining, never approached** | **12.564238** | + +Reused figures (`s1` both-together $3.559636, `s2` $1.190071, `s3` $0.958775, +totaling $5.708482) were spent by predecessor measurement tasks in this epic +against their own separate, already-reported ceilings, and are not counted a +second time against this ticket's own $20.00. + +## Acceptance criteria (#57's own body), checked explicitly + +- [x] Scored configuration and gates match the pre-v2 manifest from #59; no + threshold changed after outputs were visible. (Grader-version transition is + the one reviewed exception, owner-authorized before this ticket examined any + of its own scored output; no numeric threshold was altered.) +- [x] Every deterministic gate above passes. +- [x] Every preregistered empirical quality, stability, false-positive, and + efficiency gate has a reproducible result (including the one that failed - + `session-continuation-summary` under verification-only - reported, not + hidden). +- [ ] Every retained mechanism demonstrates empirical value or a necessary + deterministic invariant. **Not fully met**: #52 and #53's two passes pass + their numeric gate but do not demonstrate unique causal contribution: see + above. #51 and #56 do. +- [ ] Mechanisms that fail the removal rule are removed or simplified and the + final configuration is rerun where the change affects results. **Not + applicable to this ticket's authority** - #57 is explicitly withheld + removal/simplification authority; recorded as a recommendation for the owner + instead. +- [x] Direct review, implementation, and PR-babysitting integration cases pass + on exact current heads. +- [x] Runtime/model strata are reported separately and never compared as if + identical (the grader-version distinction is the one boundary this record had + to draw explicitly, and it does). +- [x] No legacy contract path can produce a v2 clean/ready verdict. +- [x] Documentation states what clean proves, what it does not prove, how + uncertainty is represented (this report, `S1-ABLATION-MATRIX.md`, and the + unique-contribution caveat), and how future corpus cases are added (see + `review-suite/evals/README.md` and `review-suite/evals/curation/README.md`, + both unchanged, both still authoritative). +- [x] Residual material failures are explicit follow-up **topics** in this + report rather than closeout prose asserting completeness - explicitly not + filed as GitHub issues, since that authority is withheld from this ticket. +- [x] `just format`, `just lint`, and `just test` pass. + +## What this record does not do + +- Does not remove, simplify, or edit #51/#52/#53/#56, or comment a removal + decision onto any of them. +- Does not file any new GitHub issue. +- Does not touch `baseline/v1/` or any of #59's own frozen files. +- Does not invoke `carve-changesets`. +- Does not close epic #49. diff --git a/review-suite/evals/v2/DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md b/review-suite/evals/v2/DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md new file mode 100644 index 0000000..5e3dc11 --- /dev/null +++ b/review-suite/evals/v2/DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md @@ -0,0 +1,81 @@ +# Deterministic gates and caller integration verification (#57) + +This record covers #57's phases 1 and 2 - the parts of the ticket that spend no +model money. It cites the exact, already-existing tests from #51-#56 that prove +each required deterministic gate and caller-integration scenario, and states +plainly where nothing new was needed. It adds no new deterministic gate that +#51-#56 do not already require, and it does not modify `baseline/v1/`, +`gate-manifest.json`, `DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, or `audits/`. + +All citations below were verified against this exact candidate's checked-out +source (branch `claude/57-v2-ablation-closeout`, isolated worktree), not +paraphrased from memory or from #59's own documents. + +## Required deterministic gates + +| # | Gate (from #57's issue body) | Evidence | +| --- | -------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | Packet/result/schema/version validation passes for the full dependency closure | `review-suite/scripts/tests/test_contracts.py` (54 tests: `test_missing_required_field_is_rejected`, `test_unknown_enum_is_rejected`, `test_malformed_finding_returns_errors`, `test_malformed_pair_candidate_returns_errors`, and the full `ReviewResultValidatorTests`/`PairValidationTests` classes); `review-suite/scripts/tests/test_bundled_contracts.py` (byte-identical bundled schema/validator copies) | +| 2 | Failed required validation cannot produce clean | `test_failed_focused_validation_invalidates_a_clean_pair` (line 289), `test_failed_full_validation_invalidates_a_clean_pair` (299), `test_unavailable_validation_invalidates_a_clean_pair` (309), all in `test_contracts.py` | +| 3 | Incomplete required evidence, stale candidate identity, unavailable mandatory review work, or failed finding validation cannot produce clean | `test_missing_required_lens_execution_invalidates_clean_aggregate` (336), `test_duplicate_required_lens_execution_invalidates_clean_aggregate` (348), `test_stale_head_lens_execution_invalidates_clean_aggregate` (356), `test_stale_base_lens_execution_invalidates_clean_aggregate` (362) in `test_contracts.py`; `test_no_risk_exercised_cannot_pair_with_a_silent_clean` (562, verification-sufficiency evidence gate); `test_disposition_naming_only_the_hardened_call_site_is_rejected` (471, consumer-impact evidence gate) | +| 4 | Every clean aggregate is bound to the exact candidate/base and includes all required evidence | `test_result_must_match_packet_candidate` (209), `test_merge_verdict_requires_complete_candidate_identity` (253), `test_fresh_current_head_clean_executions_produce_a_valid_clean_aggregate` (368), all in `test_contracts.py` | +| 5 | A head-changing fix invalidates readiness evidence and triggers the final full review contract | `test_new_head_after_correctness_fix_requires_all_three_executions` (388), `test_new_head_after_code_simplicity_fix_requires_all_three_executions` (396) in `test_contracts.py`; caller-side: `test_stale_head_lens_execution_is_rejected` (120) and `test_result_bound_to_a_different_head_is_rejected` (128) in the shared, bundled `test_review_gate.py` (identical copy in `skills/babysit-pr/scripts/tests/` and `skills/implement-ticket/scripts/tests/`, proven byte-identical by `test_every_consuming_skill_bundles_an_identical_review_gate`) | +| 6 | Base-only drift follows the recorded risk-based equivalence rules | `test_unrelated_base_drift_is_accepted` (54), `test_unrelated_base_drift_remains_a_valid_clean_pair_at_v1_1` (409), both in `test_contracts.py` | +| 7 | Legacy or unsupported contract data cannot yield a v2 clean/ready result | `test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error` (422), `test_stale_v1_1_aggregate_result_is_rejected_with_a_useful_error` (430), `test_stale_v1_2_aggregate_result_is_rejected_with_a_useful_error` (438) in `test_contracts.py`; caller-side `test_stale_schema_version_is_rejected_with_migration_action` (55) and `test_unsupported_future_schema_version_is_rejected` (64) in the shared `test_review_gate.py` | +| 8 | Canonical and bundled contracts remain byte-identical | `test_every_review_skill_bundles_identical_contract_copies` (69), `test_every_consuming_skill_bundles_an_identical_review_gate` (86), `test_every_bundled_validator_executes_in_its_installed_layout` (103), all in `test_bundled_contracts.py` | +| 9 | Review contexts remain read-only | `test_skill_uses_shared_contract_and_is_read_only` in `skills/review-correctness/scripts/tests/test_skill_contract.py` (27) and the equivalent test in `skills/review-code-simplicity/` and `skills/review-solution-simplicity/`'s own `test_skill_contract.py`; each asserts its `SKILL.md` contains "Preserve read-only integrity" and declares no file-editing tool in `allowed-tools` | + +**Result: all nine required deterministic gates pass, on existing #51-#56 +evidence.** No new deterministic gate test was required. The only new test this +ticket added (`test_skill_root_defaults_to_the_real_skills_tree`, +`test_an_overridden_skill_root_is_recorded_and_changes_the_closure` in +`review-suite/scripts/tests/test_eval_runner.py`) covers the ablation +mechanism's own correctness, not a #57-required deterministic invariant per se - +it is included because the ablation mechanism is new code this ticket introduces +and untested new code is its own risk. + +Full-suite confirmation: `just test` passes (all per-skill suites plus 313 +`review-suite/scripts/tests` tests) on candidate `e2c56f6`; `just lint` passes +(ruff, mdformat, `skills-ref validate` on all 8 skill directories, +plugin-packaging validation). + +## Integration verification + +#57 requires representative exact-head flows through `review-code-change` +directly, `implement-ticket` end-to-end, and `babysit-pr` through a +head-changing fix, covering: clean, changes_required, blocked, stale handoff, +failed validation, base drift, runtime failure, malformed result, and +accepted/rejected connector-regression. + +| Scenario | Caller(s) | Evidence | +| -------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Clean | review-code-change | `test_sequence_and_verdict_boundaries` case `ordered-clean` (`skills/review-code-change/scripts/tests/test_orchestration_contract.py:99`) | +| Clean | babysit-pr, implement-ticket | `test_current_clean_aggregate_is_accepted` (shared `test_review_gate.py:52`) | +| Changes_required | review-code-change | `test_sequence_and_verdict_boundaries` cases `early-solution-redesign`, `deduplicate-overlap`, `cycle-budget-exhausted` | +| Changes_required | babysit-pr, implement-ticket | `test_changes_required_verdict_is_rejected` (shared `test_review_gate.py:89`) | +| Blocked | review-code-change | `test_sequence_and_verdict_boundaries` cases `missing-dependency`, `missing-evidence`, `mismatched-lens-result` | +| Blocked | babysit-pr, implement-ticket | `test_blocked_verdict_is_rejected` (shared `test_review_gate.py:76`) | +| Stale handoff / head-changing fix | review-code-change | `test_sequence_and_verdict_boundaries` cases `full-restart-after-correctness-fix`, `full-restart-after-code-simplicity-fix` | +| Stale handoff / head-changing fix | babysit-pr, implement-ticket | `test_stale_head_lens_execution_is_rejected` (120), `test_result_bound_to_a_different_head_is_rejected` (128), `test_result_bound_to_a_different_base_is_rejected` (138), all shared `test_review_gate.py`; PR-watch level: `test_snapshot_fetches_feedback_before_ci_and_reports_candidate_change` (`skills/babysit-pr/scripts/tests/test_gh_pr_watch.py:730`) | +| Failed validation | review-code-change (via shared contract) | `test_failed_focused_validation_invalidates_a_clean_pair`, `test_failed_full_validation_invalidates_a_clean_pair`, `test_unavailable_validation_invalidates_a_clean_pair` (`test_contracts.py`, cited above) | +| Base drift | review-code-change (via shared contract) | `test_unrelated_base_drift_is_accepted`, `test_unrelated_base_drift_remains_a_valid_clean_pair_at_v1_1` | +| Base drift (PR-watch level) | babysit-pr | drift-handling coverage in `skills/babysit-pr/scripts/tests/test_gh_pr_watch.py` (candidate-change detection above; `test_retry_rejects_mixed_current_and_unverified_run_ids:1155` for run-identity binding) | +| Runtime failure | babysit-pr | `test_watch_survives_transient_errors_then_recovers` (896), `test_watch_fails_after_transient_failure_budget` (927), `test_watch_does_not_retry_identity_failures` (943), all in `test_gh_pr_watch.py` | +| Malformed result | babysit-pr, implement-ticket | `test_malformed_result_is_rejected` (shared `test_review_gate.py:70`); `test_non_aggregate_lens_result_is_rejected` (148) | +| Accepted/rejected connector-regression | curation/promotion workflow (#56, consumed by review-correctness's rubric-update path) | `review-suite/scripts/tests/test_eval_curation.py`: `test_a_rejected_record_cannot_support_a_positive_case` (297), `test_an_accepted_record_cannot_support_a_negative_control` (307), `test_a_distinct_duplicate_of_an_accepted_record_cannot_be_a_negative_control` (217), `test_a_distinct_duplicate_of_a_rejected_record_cannot_be_a_positive_case` (238), `test_global_rubric_update_requires_two_distinct_surfaces` (315), `test_global_rubric_update_requires_a_negative_control` (323) | + +**Result: every required scenario is already covered by existing #51-#56 +evidence.** No new caller-integration test was genuinely missing. #56 is a +workflow/tooling ticket, not itself scored against `s1`/`s2`/`s3` +(`DECISION-RECORD.md`'s own #56 entry: "not itself scored... Any future +regression case sourced from the new connector data is separate, later work with +its own preregistered cost ceiling"), so "accepted/rejected +connector-regression" integration is proven at the curation/promotion layer that +feeds `review-correctness`'s rubric-update path, exactly as its disposition +describes. + +Provider-neutral core semantics: both `agents/claude-code.md` and +`agents/openai.yaml` exist for every skill in this suite (verified via +`find skills -name "openai.yaml" -o -name "claude-code.md"`); this record does +not require identical runtime adapters for optional provider metadata beyond +that, per #57's own non-goal. diff --git a/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md b/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md new file mode 100644 index 0000000..62a1187 --- /dev/null +++ b/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md @@ -0,0 +1,144 @@ +# Frozen v2 scored configuration (#57) + +This record freezes the exact configuration `#57` scored against, before any of +its own scored output was examined, per `gate-manifest.json`'s own rule ("frozen +the moment any scored v2 output for the affected stratum is examined"). It does +not modify `gate-manifest.json`, `DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, +`audits/`, or `baseline/v1/` - all remain exactly as `#58`/`#59` delivered them. +It adds one new file only. + +## Grader version transition (reviewed, explicitly owner-authorized) + +`gate-manifest.json` preregistered its thresholds against the grader as it stood +before `finding_surfaces()`'s real defect (location-only surface matching; a +symbol named only in a finding's prose was invisible to the grader) was found +and fixed. That fix, its cause, and its version-comparability consequence are +documented in [`GRADER-1.1-COMPARABILITY.md`](GRADER-1.1-COMPARABILITY.md); its +effect on the two `s1` target cases (pre-#53 and post-#53, both under the fixed +grader) is documented in +[`s1-rescore-pre-post-comparison.md`](s1-rescore-pre-post-comparison.md); its +effect on `s2`/`s3` is documented in +[`s2-s3-grader-1.1-recheck.md`](s2-s3-grader-1.1-recheck.md). + +**The repository owner has explicitly authorized running #57's own scored +comparison against the fixed (`1.1`) grader, not the original (`1.0`) grader +that `gate-manifest.json` was preregistered against.** This is the "reviewed +version transition" #57's own body requires before deviating from a frozen +input, and the three documents above are its evidentiary trail: a real, +disclosed defect was found mid-measurement by a predecessor task, fixed under an +existing, unrelated calibration constraint (the +`probe.partial-claim-wrong- surface` case had to keep passing), and the fix's +reach was independently verified against retained raw attempts for both `s1`'s +two target cases and all four `s2`/`s3` material cases before this ticket ran a +single further scored attempt. Every scored result in this record and in `#57`'s +closeout report is graded under grader `1.1`. Per the comparability rule those +three documents establish, no figure here is compared directly against any +`1.0`- graded figure, including `baseline/v1/`'s own frozen numbers, without +saying so explicitly. + +## Runtime/model stratum (verified unchanged, matches `gate-manifest.json`'s pin exactly) + +- `runtime`: `claude` (Claude Code CLI) +- `runtime_cli_version`: `2.1.92` - verified via `claude --version` immediately + before scoring began +- `model`: `claude-opus-4-6[1m]` - verified via one negligible-cost (`$0.00904`) + sanity call reading the CLI's own `modelUsage` reporting, with no `--model` + flag passed, exactly matching how the executor invokes it +- No runtime, runtime-version, or model drift was detected. Per + `gate-manifest.json`'s stratum rule, scoring proceeded; had drift been + detected, this ticket would have returned `blocked` instead. + +## Suite commit and corpus/grader versions + +| Configuration | `suite_commit` | `corpus_version` | `grader_version` | `skill_root` | +| ----------------------------------------------------------- | ------------------------------------------ | --------------------------------- | ---------------- | --------------------------------------------------------------------------------------------------- | +| `s1`, both passes together (reused, not re-run - see below) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | `0.1-s1-populated` | `1.1` (regraded) | real `skills/` tree (unablated) | +| `s1`, traversal-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | `review-suite/evals/v2/ablation-skill-roots/traversal-only/` overlaid on the real `skills/` tree | +| `s1`, verification-sufficiency-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | `review-suite/evals/v2/ablation-skill-roots/verification-only/` overlaid on the real `skills/` tree | +| `s2-solution-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-solution-simplicity`\* | `1.1` | real `skills/` tree | +| `s3-code-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-code-simplicity`\* | `1.1` | real `skills/` tree | + +\*See each report's own `configuration.corpus_version` for the exact string; +restated here from the committed report files, not retyped from memory. + +**Reuse rationale for "both passes together," `s2`, and `s3`:** these three +configurations were already scored under grader `1.1` by predecessor measurement +tasks in this same epic (`s1-rescore-pre-post-comparison.md`'s "post-#53, full +7-case stratum" run; `s2-s3-grader-1.1-recheck.md`'s full-stratum runs), against +a `suite_commit` whose `skills/review-code-change`, +`skills/review-solution-simplicity`, `skills/review-correctness`, and +`skills/review-code-simplicity` trees are byte-identical to this ticket's own +`suite_commit` (`e2c56f6`) for every file those runs actually evaluated - this +ticket's own diff touches only `review-suite/scripts/evals/runner.py`, its own +tests, and two new eval-only fixture files outside `skills/`, none of which are +part of the target skill closure any scored run sends to a reviewer. Re-running +either configuration would spend real money to reconfirm a result no code change +since the prior run could have altered. `gate-manifest.json`'s non-regression +floor for `s2`/`s3` ("all 8 cases must replay with identical statuses to their +v1 report") was already checked by `s2-s3-grader-1.1-recheck.md`; this record +does not repeat that check. + +## Executor and suite configuration + +- `executor_command`: `python3 review-suite/scripts/evals/claude_executor.py` +- `process_isolation`: one fresh executor process per attempt (unchanged from + v1) +- `runs_per_case`: `5` (unchanged from `gate-manifest.json`, for v1/v2 + comparability) +- `retry_policy`: none (a failed attempt is an evaluation failure, never + retried) +- `timeout_seconds`: `300` for the two new ablation runs, matching + `gate-manifest.json`'s own (unconditionally applicable) pin. Disclosed + methodology note: the earlier "both passes together" run this record reuses + used `450s` (a separate, `#53`-issue-body-specific override, itself never + approached: observed max latency across that run was `116.6s`). Neither + timeout was approached by any attempt in any of the three configurations (see + per-configuration latency tables below), so this difference is disclosed for + completeness and does not affect comparability of the reported figures. +- `max_output_bytes`: default (`4,000,000`), unchanged + +## Ablation mechanism + +The shipped `review-correctness` skill performs both required passes +(consumer/impact-traversal; verification-sufficiency) unconditionally on every +review, by design (`skills/review-correctness/SKILL.md`: "not routed specialist +modules"). To score each pass in isolation without adding a runtime toggle to +the shipped skill, this ticket adds an optional `--skill-root` override to +`review-suite/scripts/evals/runner.py` (default: the real `skills/` tree, so +every existing caller and report is unaffected) and two eval-only +`review-correctness/SKILL.md` overlays under +`review-suite/evals/v2/ablation-skill-roots/{traversal-only,verification-only}/`, +each disabling exactly one required pass with an explicit no-op instruction. +Every other file in each ablation root is byte-identical to the real `skills/` +tree (verified directly via `diff -rq` before each scored run). The resolved +`skill_root` is recorded verbatim in every report's `configuration.skill_root` +field, so an ablation run can never be mistaken for a standard-configuration +one. See `review-suite/scripts/evals/runner.py`'s `target_skill_documents` +docstring and `review-suite/scripts/tests/test_eval_runner.py`'s +`test_an_overridden_skill_root_is_recorded_and_changes_the_closure` for the +mechanism's own tests. + +## Cost ceiling + +- This ticket's own hard ceiling: **$20.00** for its scored evaluation. +- `gate-manifest.json`'s per-stratum proposed ceilings: `s1` $12.00, `s2` $3.00, + `s3` $3.00 (all unchanged, all preregistered before any v2 output was + examined). +- Actual total spend against this ticket's own $20.00 ceiling: see the closeout + report's spend table. The two new scored runs (traversal-only, + verification-only) are the only real-money spend this ticket itself incurred; + the reused "both passes together," `s2`, and `s3` figures were paid for by + predecessor measurement tasks in this epic and are not counted a second time + against this ticket's own ceiling. + +## Invalid runs, missing data, and threshold-change discipline + +Unchanged from `gate-manifest.json`: an attempt classified +`spawn_failure`/`timeout`/`runtime_failure`/`output_too_large`/ +`malformed_output`/`protocol_mismatch` is never graded and never silently +retried. Every quality/stability/non-regression threshold in +`gate-manifest.json`'s `quality_stability_non_regression_thresholds` section, +and the settled per-case gate from `#53`'s own issue body (`mean_recall >= 0.6` +OR guarded `mean_combined_recall >= 0.8`, per case independently), was fixed +before this ticket examined any of its own scored output. No threshold was tuned +after seeing a result. diff --git a/review-suite/evals/v2/README.md b/review-suite/evals/v2/README.md index 2bfa7b0..0af4704 100644 --- a/review-suite/evals/v2/README.md +++ b/review-suite/evals/v2/README.md @@ -1,9 +1,13 @@ -# v2 planning record +# v2 planning record and scored closeout -This directory is the planning/definition gate #59 produces. It changes no +This directory holds both the planning/definition gate #59 produces and #57's +own scored closeout that executes against it. The planning documents change no review prompt, schema, rubric, orchestration, or caller runtime behavior by -itself — it interprets the frozen `../baseline/v1/` record and turns it into an -implementation-ready decision for #51–#57. +themselves — they interpret the frozen `../baseline/v1/` record and turn it into +an implementation-ready decision for #51–#57. #57's own documents (listed under +"Scored closeout (#57)" below) run the preregistered scored comparison and +report a pass/fail verdict per mechanism as a recommendation, not a removal +decision. - [`FAILURE-TAXONOMY.md`](FAILURE-TAXONOMY.md) — classifies every material outcome in the frozen v1 baseline by the smallest evidenced cause. @@ -26,12 +30,35 @@ implementation-ready decision for #51–#57. live `#51`–`#57` graph after the body and native-edge mutations described above, with findings and fixes recorded separately per pass. -## What this is not +## Scored closeout (#57) -This record does not run any scored v2 evaluation. `gate-manifest.json` -preregisters the configuration a future scored run (owned by #57) must use; no -case in this directory has been executed against a real reviewer. Every -threshold here is explicitly a proposal, not a measured result. +- [`FROZEN-V2-CONFIGURATION.md`](FROZEN-V2-CONFIGURATION.md) — the exact scored + configuration #57 ran: the reviewed grader-version transition (`1.0` → `1.1`, + owner-authorized), the verified runtime/model stratum, suite commits, and the + ablation mechanism (a `--skill-root` override in `runner.py`) used to isolate + each `review-correctness` pass without editing the shipped skill. +- [`S1-ABLATION-MATRIX.md`](S1-ABLATION-MATRIX.md) — the three-configuration + `s1-correctness-orchestrator` ablation (traversal-only, + verification-sufficiency-only, both together), each scored independently, plus + the reused `s2`/`s3` non-regression check, against the settled per-case gate. +- [`DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md`](DETERMINISTIC-AND-INTEGRATION-EVIDENCE.md) + — every required deterministic gate and caller-integration scenario, cited + against the exact existing #51–#56 test that proves it. +- [`CLOSEOUT-REPORT.md`](CLOSEOUT-REPORT.md) — the full synthesis: mechanism + verdicts (recommendation only, not a removal decision), residual risks, spend + against the $20 ceiling, and every acceptance criterion checked explicitly. +- `s1-correctness-orchestrator-ablation-{traversal,verification}-only.report.json` + — the two new machine-readable aggregate reports this ticket produced. + `s1-correctness-orchestrator.report.json`, + `s2-solution-simplicity-lens.report.json`, and + `s3-code-simplicity-lens.report.json` are reused from predecessor measurement + tasks in this epic, not re-run. -It also does not modify `../baseline/v1/` in any way. That record stays frozen -exactly as merged in #58. +## What the planning documents above are not + +They do not run any scored v2 evaluation themselves. `gate-manifest.json` +preregisters the configuration the scored run above uses; every threshold in it +remained a proposal, never tuned after any scored output was examined. + +Neither the planning documents nor #57's own closeout documents modify +`../baseline/v1/` in any way. That record stays frozen exactly as merged in #58. diff --git a/review-suite/evals/v2/S1-ABLATION-MATRIX.md b/review-suite/evals/v2/S1-ABLATION-MATRIX.md new file mode 100644 index 0000000..90eaf25 --- /dev/null +++ b/review-suite/evals/v2/S1-ABLATION-MATRIX.md @@ -0,0 +1,161 @@ +# `s1-correctness-orchestrator` ablation matrix: traversal-only, verification-only, both together (#57) + +This is #57's own scored ablation measurement, run against the exact +configuration frozen in +[`FROZEN-V2-CONFIGURATION.md`](FROZEN-V2-CONFIGURATION.md). It reports each +configuration's contribution separately, per #57's own instruction not to +average across configurations. It does not modify `baseline/v1/`, +`gate-manifest.json`, `DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, or `audits/`. + +**Disclosed operating condition.** The two new scored runs below (traversal- +only, verification-only) executed while this shared machine reported very heavy, +unrelated concurrent load (load averages briefly over 190-200, 100% CPU from +other sessions, confirmed not to originate from this task's own ~7 processes). +Neither run produced a single `timeout`, `runtime_failure`, `spawn_failure`, +`output_too_large`, or `malformed_output` attempt (every failure-rate field is +`0.0` in both reports), and observed latency (mean 43.6s / 50.9s, max 74.0s / +87.2s) stayed well under the 300s timeout and did not exceed the reused "both +together" configuration's own historical mean (49.0s). No execution-level +evidence ties the load to a degraded or invalid attempt. This is stated plainly +rather than silently smoothed over, per instruction, precisely because one real +quality regression *was* found below (session-continuation-summary, +verification-only) - it is reported as a finding, not dismissed as a load +artifact, because nothing in the raw attempts or the failure-rate fields +supports that explanation. + +## The three configurations + +| Configuration | `suite_commit` | `skill_root` | Attempts | Cost (USD) | Mean latency | +| -------------------------------------------------------------------------------------- | ------------------------------------------ | ----------------------------------------- | -------- | ---------- | ------------ | +| Both passes together (reused, scored by a predecessor task at this same skill content) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | real `skills/` tree | 35 | 3.559636 | 49.04s | +| Traversal-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `ablation-skill-roots/traversal-only/` | 35 | 3.560108 | 43.62s | +| Verification-sufficiency-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `ablation-skill-roots/verification-only/` | 35 | 3.866613 | 50.93s | + +All three: `grader_version 1.1`, `corpus_version 0.1-s1-populated`, +`runs_per_case 5`, `cases 7` (35 attempts), `runtime claude 2.1.92`, +`model claude-opus-4-6[1m]`. Zero evaluation failures of any kind in any of the +three. + +## Target-case gate results (settled gate: `mean_recall >= 0.6` OR guarded `mean_combined_recall >= 0.8`, per case independently, per #53's issue body) + +| Case | Owning mechanism | Both together | Traversal-only | Verification-only | +| ----------------------------------- | --------------------------------- | --------------------------------------------- | --------------------------------------------- | --------------------------------------------- | +| `dependency-strictness-propagation` | #52 schema + #53 traversal pass | `mean_recall 0.0`, `combined 1.0` -> **PASS** | `mean_recall 0.0`, `combined 1.0` -> **PASS** | `mean_recall 0.0`, `combined 1.0` -> **PASS** | +| `stale-claim-release-guard` | #53 verification-sufficiency pass | `mean_recall 0.0`, `combined 1.0` -> **PASS** | `mean_recall 0.0`, `combined 1.0` -> **PASS** | `mean_recall 0.0`, `combined 1.0` -> **PASS** | + +Both target cases show `verdict_stability`/`finding_stability` of `1.0` in all +three configurations - the v1 floor of `1.0` holds everywhere. + +**Mechanically, both passes' preregistered removal-rule condition is satisfied** +(`gate-manifest.json`'s `mechanism_ablations`: "Remove if +`dependency-strictness-propagation` does not move... in this isolated +configuration" / "Remove if `stale-claim-release-guard` does not move... in this +isolated configuration" - neither triggers, since both cases pass in every +configuration, including their own isolated one). + +**This is not the whole picture, and reporting only the table above would be the +kind of smoothing this ticket is instructed not to do.** Both target cases pass +**regardless of which single pass is active, or whether either is active at +all**: + +- `dependency-strictness-propagation` passes under verification-only, the + configuration in which the traversal pass - its nominal owning mechanism - is + explicitly disabled. +- `stale-claim-release-guard` passes under traversal-only, the configuration in + which the verification-sufficiency pass - its nominal owning mechanism - is + explicitly disabled. +- This extends, rather than contradicts, `s1-rescore-pre-post-comparison.md`'s + own already-recorded finding: the **pre-#53 reviewer** (commit `8e4fdbd`, + before either pass existed as an instruction) already resolved both cases + 5-for-5 under the fixed grader, "without being explicitly instructed to run + either pass by name." + +Taken together (pre-#53 zero-pass baseline, plus both single-pass ablations, +plus the combined configuration), **four independent configurations now show +both target cases resolved, and none of the four demonstrates that either +specific pass's own instruction text is what causes the resolution.** The most +parsimonious explanation consistent with all four data points is that a capable +reviewer, once graded by a grader that can see a symbol named only in prose +(grader `1.1`), already finds both root causes from the packet's raw diff and +repository access alone - independent of whether it is told to run a named +"traversal" or "verification-sufficiency" pass. **Passing the numeric gate is +real; demonstrating this pass's own unique, causal contribution is not** - this +is exactly #57's own "unique contribution beyond earlier mechanisms" reporting +dimension, and it is not satisfied by either pass in isolation, on this +evidence. + +## Non-regression floor (all cases, all three configurations) + +| Case | Requirement | Both together | Traversal-only | Verification-only | +| --------------------------------- | ------------------------------------------- | ------------------------------------------- | --------------------------- | ------------------------ | +| `dependency-hint-parser-coverage` | 0 false positives | 0/5 | 0/5 | 0/5 | +| `post-bootstrap-module-load` | 0 false positives | 0/5 | 0/5 | 0/5 | +| `session-continuation-summary` | 0 false positives | 0/4 graded (1 excluded, `malformed_output`) | 0/5 | **3/5 - FLOOR VIOLATED** | +| `process-isolation-assertion` | false-alarm rate \<= 2/5 (v1) | 0/5 | 0/5 | 0/5 | +| `optional-tool-probe` | not gated; v1 `mean_recall` 0.2, no ceiling | `mean_recall 0.8`, 0 fp | `mean_recall 0.6`, **1 fp** | `mean_recall 1.0`, 0 fp | + +**`session-continuation-summary` regresses under the verification-only +configuration specifically.** 3 of 5 attempts raised a `blocking` finding that +the test covering the finalize-only session path "asserts only on the return +value" and would not catch a regression that deleted the `finalize()` call or +added an accidental `start_agent()` call - a finding shaped exactly like the +verification-sufficiency pass's own intended output, on a case whose expectation +record carries no expected root cause (`expected_root_cause_ids: []`, a +deliberate "correct control"). The grader correctly classifies all three as +false positives/false alarms per the corpus's own ground truth +(`verdict_stability` drops to `0.6`). **This regression does not appear in the +"both passes together" configuration** (0 false positives on this case, matching +v1), so it is not evidence against the combined, already-shipped configuration - +it is evidence that running the verification-sufficiency pass *alone*, without +the traversal pass, makes this reviewer measurably more willing to raise a +marginal test-sufficiency finding on a case designed to have none. No comparable +regression appears under traversal-only. + +`optional-tool-probe` is explicitly not gated (`gate-manifest.json`: "re-scoring +must not regress its v1 mean_recall of 0.2 but no target above that is set"); +all three configurations exceed that floor. The one false positive under +traversal-only and the perfect score under verification-only are recorded for +completeness, not as a gate result. + +## Reused `s2`/`s3` results (unchanged since v1, no ablation applicable - see `s2-s3-grader-1.1-recheck.md` for the full methodology) + +| Stratum | Case | `mean_combined_recall` | False positives | Stability | +| ------- | ------------------------------------------------ | ---------------------- | --------------- | --------- | +| s2 | `registry-client-layering` | 1.0 | 0/5 | 1.0 / 1.0 | +| s2 | `setup-service-path-gateway` | 1.0 | 0/5 | 1.0 / 1.0 | +| s3 | `metrics-label-formatting-duplication` | 1.0 | 0/5 | 1.0 / 1.0 | +| s3 | `watcher-check-policy-duplication` | 1.0 | 0/5 | 1.0 / 1.0 | +| s2 | `reconciliation-outcome-type` (control) | n/a | 0/5 | 1.0 / 1.0 | +| s2 | `record-status-transition-guard` (control) | n/a | 0/5 | 1.0 / 1.0 | +| s3 | `compat-accessor-boundary-duplication` (control) | n/a | 0/5 | 1.0 / 1.0 | +| s3 | `env-inventory-bullet-format` (control) | n/a | 0/5 | 1.0 / 1.0 | + +All 8 `s2`/`s3` cases replay with identical statuses to their v1 report, per +`gate-manifest.json`'s own non-regression requirement (already verified by +`s2-s3-grader-1.1-recheck.md`; not re-verified here since neither stratum's +prompts or schema consumption are touched by anything in this manifest). + +## Cost and latency delta vs. `baseline/v1`'s frozen `s1` figures + +`baseline/v1/s1-correctness-orchestrator.report.json`'s own cost/latency figures +are graded under `GRADER_VERSION 1.0` and a different suite commit (`2644c12`); +grading version does not change cost or latency (those are runtime-measured, not +grader-computed), so a same-runtime comparison is valid here. v1's total cost +for the 35-attempt `s1` run was within its $9.00 ceiling; this ticket's own two +new 35-attempt runs cost $3.560 and $3.867 respectively - both below the reused +"both together" run's $3.560 and well below `gate-manifest.json`'s proposed +$12.00 v2 ceiling for this stratum. Mean latency for the two new configurations +(43.6s, 50.9s) is comparable to the reused "both together" run's 49.0s; none +approached its 300s timeout. + +## Settled verdict per mechanism (mechanical gate vs. unique-contribution finding) + +| Mechanism | Target case | Mechanical gate | Unique contribution demonstrated? | Non-regression cost found? | +| -------------------------------------------- | ----------------------------------- | ----------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------ | +| #53 traversal pass (isolated) | `dependency-strictness-propagation` | PASS | **No** - case also passes with this pass disabled | None found in this configuration | +| #53 verification-sufficiency pass (isolated) | `stale-claim-release-guard` | PASS | **No** - case also passes with this pass disabled | **Yes** - `session-continuation-summary` false-positive regression (3/5) | +| #53 both passes combined (as shipped) | both | PASS | Not newly demonstrated by this measurement (see above); no regression to `session-continuation-summary` or any other non-target case | None found | +| #52 `consumer_impact_evidence` schema | `dependency-strictness-propagation` | PASS (schema's removal rule is tied to the traversal-pass result above) | Same caveat as the traversal pass above | None found | + +See the closeout report for the recommendation (not a decision) this evidence +supports. diff --git a/review-suite/evals/v2/s1-correctness-orchestrator-ablation-traversal-only.report.json b/review-suite/evals/v2/s1-correctness-orchestrator-ablation-traversal-only.report.json new file mode 100644 index 0000000..2a91523 --- /dev/null +++ b/review-suite/evals/v2/s1-correctness-orchestrator-ablation-traversal-only.report.json @@ -0,0 +1,422 @@ +{ + "adjudication_required": [ + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "corr.dependency-finalized-skips-strict-proof", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-path-missing-strict-proof", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "corr.dependency-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-path-missing-strict-proof", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "case_id": "optional-tool-probe", + "finding_id": "corr.skip-crashes-when-absent", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "case_id": "optional-tool-probe", + "finding_id": "corr.probe-conflates-absent-and-failing", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "correctness.null-owner-guard-bypass", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.none-owner-guard-bypass", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "correctness.null-owner-bypass", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "correctness.missing-null-owner-test", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "none-owner-bypass", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "missing-none-owner-test", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.owner-none-bypass", + "reason": "partial", + "run_number": 5 + } + ], + "attempts": 35, + "baseline_eligible": true, + "configuration": { + "artifacts_retained": true, + "cases": 7, + "corpus_version": "0.1-s1-populated", + "executor": "python3 review-suite/scripts/evals/claude_executor.py", + "executor_models": [ + "claude-opus-4-6[1m]" + ], + "grader_version": "1.1", + "max_output_bytes": 4000000, + "runs_per_case": 5, + "skill_root": "/private/tmp/claude-501/-Users-scott--claude-worktrees-agent-scripts-epic-49-implementation-eab73d/d2c1ed44-f510-4ae0-8cdd-ed14e9491168/scratchpad/ablation-skills-traversal-only", + "stratum": { + "grading_is_signal": true, + "ground_truth": "human-review", + "id": "s1-correctness-orchestrator", + "purpose": "Correctness and verification cases for the orchestrator target, whose payload therefore carries its three required lens skills. Every case carries the recorded source disposition as its first adjudication and an executable oracle - a runnable module independent of the reviewer's model - as its second. SCORED: both owner-gated inputs are satisfied (the $15 total cost ceiling and complete independent adjudication across all three strata), and every expectation stays `calibrated: false` permanently, per the owner-settled three-way grading method - a scored case is never calibrated on its own prose, and a grader miss surfaces as `referred_root_cause_ids` rather than a silent reviewer-miss.", + "scored": true + }, + "suite_commit": "e2c56f68fe56094a6c92fd4a220539f47d6f9f98", + "target_skill": "review-code-change", + "target_skill_dependencies": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ], + "target_skill_digest": "18d6dfa6533e54b0", + "target_skill_documents": [ + "review-code-change/SKILL.md", + "review-code-change/references/orchestration-protocol.md", + "review-code-simplicity/SKILL.md", + "review-code-simplicity/references/code-simplicity-rubric.md", + "review-correctness/SKILL.md", + "review-correctness/references/correctness-rubric.md", + "review-solution-simplicity/SKILL.md", + "review-solution-simplicity/references/solution-simplicity-rubric.md" + ], + "timeout_seconds": 300.0 + }, + "failures": { + "blocked_rate": 0.0, + "malformed_output_rate": 0.0, + "output_too_large_rate": 0.0, + "protocol_mismatch_rate": 0.0, + "runtime_failure_rate": 0.0, + "spawn_failure_rate": 0.0, + "timeout_rate": 0.0 + }, + "graded_attempts": 35, + "grader_version": "1.1", + "latency": { + "count": 35, + "max_seconds": 74.0121639999561, + "mean_seconds": 43.62063657497721, + "min_seconds": 19.164502874948084, + "p50_seconds": 38.11606837483123 + }, + "per_case": [ + { + "attempts": 5, + "case_id": "dependency-hint-parser-coverage", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "dependency-strictness-propagation", + "ever_referred_relevant_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "ever_referred_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "expected_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "optional-tool-probe", + "ever_referred_relevant_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "ever_referred_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "expected_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 1, + "finding_stability": 0.6, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.6, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "post-bootstrap-module-load", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "process-isolation-assertion", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "session-continuation-summary", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "stale-claim-release-guard", + "ever_referred_relevant_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "ever_referred_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "expected_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + } + ], + "protocol_version": "1.0", + "quality": { + "false_alarm_denominator": 20, + "false_alarm_rate": 0.0, + "false_clean_denominator": 15, + "false_clean_rate": 0.0, + "false_positive_denominator": 35, + "false_positive_rate": 0.02857142857142857, + "material_finding_recall": 0.2, + "recall_attempts": 15, + "referred_denominator": 35, + "referred_rate": 0.34285714285714286, + "unique_finding_contribution": [ + { + "case_id": "optional-tool-probe", + "only_some_attempts": [ + "rc.probe-checks-status-not-absence" + ] + } + ] + }, + "report_version": "1.0", + "simulation": false, + "stability": { + "mean_finding_stability": 0.9428571428571428, + "mean_verdict_stability": 1.0, + "per_case_finding_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 0.6, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 1.0, + "stale-claim-release-guard": 1.0 + }, + "per_case_stability_denominator": { + "dependency-hint-parser-coverage": 5, + "dependency-strictness-propagation": 5, + "optional-tool-probe": 5, + "post-bootstrap-module-load": 5, + "process-isolation-assertion": 5, + "session-continuation-summary": 5, + "stale-claim-release-guard": 5 + }, + "per_case_verdict_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 1.0, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 1.0, + "stale-claim-release-guard": 1.0 + }, + "stability_denominator": 35 + }, + "usage": { + "available": true, + "reporting_attempts_cost_usd": 35, + "reporting_attempts_input_tokens": 35, + "reporting_attempts_output_tokens": 35, + "total_cost_usd": 3.5601082500000003, + "total_input_tokens": 1276382.0, + "total_output_tokens": 63734.0 + } +} diff --git a/review-suite/evals/v2/s1-correctness-orchestrator-ablation-verification-only.report.json b/review-suite/evals/v2/s1-correctness-orchestrator-ablation-verification-only.report.json new file mode 100644 index 0000000..4b7860b --- /dev/null +++ b/review-suite/evals/v2/s1-correctness-orchestrator-ablation-verification-only.report.json @@ -0,0 +1,413 @@ +{ + "adjudication_required": [ + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "null-owner-guard-bypass", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "missing-null-owner-test", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.none-owner-guard-bypass", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.missing-none-owner-test", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "owner-none-guard-bypass", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "missing-none-owner-test", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.owner-none-bypass", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.missing-none-owner-test", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.none-owner-guard-bypass", + "reason": "partial", + "run_number": 5 + } + ], + "attempts": 35, + "baseline_eligible": true, + "configuration": { + "artifacts_retained": true, + "cases": 7, + "corpus_version": "0.1-s1-populated", + "executor": "python3 review-suite/scripts/evals/claude_executor.py", + "executor_models": [ + "claude-opus-4-6[1m]" + ], + "grader_version": "1.1", + "max_output_bytes": 4000000, + "runs_per_case": 5, + "skill_root": "/private/tmp/claude-501/-Users-scott--claude-worktrees-agent-scripts-epic-49-implementation-eab73d/d2c1ed44-f510-4ae0-8cdd-ed14e9491168/scratchpad/ablation-skills-verification-only", + "stratum": { + "grading_is_signal": true, + "ground_truth": "human-review", + "id": "s1-correctness-orchestrator", + "purpose": "Correctness and verification cases for the orchestrator target, whose payload therefore carries its three required lens skills. Every case carries the recorded source disposition as its first adjudication and an executable oracle - a runnable module independent of the reviewer's model - as its second. SCORED: both owner-gated inputs are satisfied (the $15 total cost ceiling and complete independent adjudication across all three strata), and every expectation stays `calibrated: false` permanently, per the owner-settled three-way grading method - a scored case is never calibrated on its own prose, and a grader miss surfaces as `referred_root_cause_ids` rather than a silent reviewer-miss.", + "scored": true + }, + "suite_commit": "e2c56f68fe56094a6c92fd4a220539f47d6f9f98", + "target_skill": "review-code-change", + "target_skill_dependencies": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ], + "target_skill_digest": "d85efbc6381a2775", + "target_skill_documents": [ + "review-code-change/SKILL.md", + "review-code-change/references/orchestration-protocol.md", + "review-code-simplicity/SKILL.md", + "review-code-simplicity/references/code-simplicity-rubric.md", + "review-correctness/SKILL.md", + "review-correctness/references/correctness-rubric.md", + "review-solution-simplicity/SKILL.md", + "review-solution-simplicity/references/solution-simplicity-rubric.md" + ], + "timeout_seconds": 300.0 + }, + "failures": { + "blocked_rate": 0.0, + "malformed_output_rate": 0.0, + "output_too_large_rate": 0.0, + "protocol_mismatch_rate": 0.0, + "runtime_failure_rate": 0.0, + "spawn_failure_rate": 0.0, + "timeout_rate": 0.0 + }, + "graded_attempts": 35, + "grader_version": "1.1", + "latency": { + "count": 35, + "max_seconds": 87.22641787491739, + "mean_seconds": 50.932720796471195, + "min_seconds": 20.294299291912466, + "p50_seconds": 53.487124499864876 + }, + "per_case": [ + { + "attempts": 5, + "case_id": "dependency-hint-parser-coverage", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "dependency-strictness-propagation", + "ever_referred_relevant_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "ever_referred_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "expected_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "optional-tool-probe", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "mean_combined_recall": 1.0, + "mean_recall": 1.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "post-bootstrap-module-load", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "process-isolation-assertion", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "session-continuation-summary", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 3, + "false_clean_attempts": 0, + "false_positive_attempts": 3, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 0.6 + }, + { + "attempts": 5, + "case_id": "stale-claim-release-guard", + "ever_referred_relevant_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "ever_referred_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "expected_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + } + ], + "protocol_version": "1.0", + "quality": { + "false_alarm_denominator": 20, + "false_alarm_rate": 0.15, + "false_clean_denominator": 15, + "false_clean_rate": 0.0, + "false_positive_denominator": 35, + "false_positive_rate": 0.08571428571428572, + "material_finding_recall": 0.3333333333333333, + "recall_attempts": 15, + "referred_denominator": 35, + "referred_rate": 0.2857142857142857, + "unique_finding_contribution": [] + }, + "report_version": "1.0", + "simulation": false, + "stability": { + "mean_finding_stability": 1.0, + "mean_verdict_stability": 0.9428571428571428, + "per_case_finding_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 1.0, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 1.0, + "stale-claim-release-guard": 1.0 + }, + "per_case_stability_denominator": { + "dependency-hint-parser-coverage": 5, + "dependency-strictness-propagation": 5, + "optional-tool-probe": 5, + "post-bootstrap-module-load": 5, + "process-isolation-assertion": 5, + "session-continuation-summary": 5, + "stale-claim-release-guard": 5 + }, + "per_case_verdict_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 1.0, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 0.6, + "stale-claim-release-guard": 1.0 + }, + "stability_denominator": 35 + }, + "usage": { + "available": true, + "reporting_attempts_cost_usd": 35, + "reporting_attempts_input_tokens": 35, + "reporting_attempts_output_tokens": 35, + "total_cost_usd": 3.866613, + "total_input_tokens": 1278100.0, + "total_output_tokens": 75794.0 + } +} From e4457b5b55cf4db5cc9254868f3409db3eeffbc8 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 12:04:47 -0700 Subject: [PATCH 4/4] docs(review-suite): correct the recorded skill_root path for both ablation runs ## Summary - The committed `ablation-skill-roots/{traversal-only,verification-only}/` directories only ever held the one changed `review-correctness/SKILL.md` file per configuration, not a full mirror of the four-skill dependency closure `--skill-root` requires. The two scored runs actually pointed `--skill-root` at a fresh local mirror of the real `skills/` tree with that one file overlaid, and each report's own `configuration.skill_root` field records that ephemeral, machine-local path, not the committed directory. - Correct `FROZEN-V2-CONFIGURATION.md` and `S1-ABLATION-MATRIX.md` to state this accurately: what path was actually recorded, why the committed directories can't be pointed at directly, and the exact steps to reproduce either scored configuration from the committed overlay files. ## Why - A fresh full review-code-change pass on the complete candidate found that the previous wording claimed the committed overlay directories were used directly as `--skill-root`, which both reports' own `configuration` field contradicts. The scored numbers themselves are unaffected (the ephemeral content used is verified byte-identical to the committed overlay files modulo a later `mdformat` line-wrap pass), but the epic's final, permanent acceptance-gate record must describe its own methodology accurately. --- .../evals/v2/FROZEN-V2-CONFIGURATION.md | 52 +++++++++++++------ review-suite/evals/v2/S1-ABLATION-MATRIX.md | 10 ++-- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md b/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md index 62a1187..5d985ea 100644 --- a/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md +++ b/review-suite/evals/v2/FROZEN-V2-CONFIGURATION.md @@ -50,13 +50,13 @@ saying so explicitly. ## Suite commit and corpus/grader versions -| Configuration | `suite_commit` | `corpus_version` | `grader_version` | `skill_root` | -| ----------------------------------------------------------- | ------------------------------------------ | --------------------------------- | ---------------- | --------------------------------------------------------------------------------------------------- | -| `s1`, both passes together (reused, not re-run - see below) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | `0.1-s1-populated` | `1.1` (regraded) | real `skills/` tree (unablated) | -| `s1`, traversal-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | `review-suite/evals/v2/ablation-skill-roots/traversal-only/` overlaid on the real `skills/` tree | -| `s1`, verification-sufficiency-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | `review-suite/evals/v2/ablation-skill-roots/verification-only/` overlaid on the real `skills/` tree | -| `s2-solution-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-solution-simplicity`\* | `1.1` | real `skills/` tree | -| `s3-code-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-code-simplicity`\* | `1.1` | real `skills/` tree | +| Configuration | `suite_commit` | `corpus_version` | `grader_version` | `skill_root` | +| ----------------------------------------------------------- | ------------------------------------------ | --------------------------------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `s1`, both passes together (reused, not re-run - see below) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | `0.1-s1-populated` | `1.1` (regraded) | real `skills/` tree (unablated) | +| `s1`, traversal-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | ephemeral local mirror of the real `skills/` tree with `review-correctness/SKILL.md` replaced by the committed `review-suite/evals/v2/ablation-skill-roots/traversal-only/review-correctness/SKILL.md` (see "Ablation mechanism" below for the exact path recorded and how to reproduce it) | +| `s1`, verification-sufficiency-pass-only (new) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `0.1-s1-populated` | `1.1` | ephemeral local mirror of the real `skills/` tree with `review-correctness/SKILL.md` replaced by the committed `review-suite/evals/v2/ablation-skill-roots/verification-only/review-correctness/SKILL.md` (see "Ablation mechanism" below) | +| `s2-solution-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-solution-simplicity`\* | `1.1` | real `skills/` tree | +| `s3-code-simplicity-lens` (reused, not re-run) | `a247d47d51eee9513b3431fc59ba34afe9316bfb` | `1.3-pilot-code-simplicity`\* | `1.1` | real `skills/` tree | \*See each report's own `configuration.corpus_version` for the exact string; restated here from the committed report files, not retyped from memory. @@ -106,15 +106,37 @@ modules"). To score each pass in isolation without adding a runtime toggle to the shipped skill, this ticket adds an optional `--skill-root` override to `review-suite/scripts/evals/runner.py` (default: the real `skills/` tree, so every existing caller and report is unaffected) and two eval-only -`review-correctness/SKILL.md` overlays under -`review-suite/evals/v2/ablation-skill-roots/{traversal-only,verification-only}/`, +`review-correctness/SKILL.md` overlay files, committed at +`review-suite/evals/v2/ablation-skill-roots/{traversal-only,verification-only}/review-correctness/SKILL.md`, each disabling exactly one required pass with an explicit no-op instruction. -Every other file in each ablation root is byte-identical to the real `skills/` -tree (verified directly via `diff -rq` before each scored run). The resolved -`skill_root` is recorded verbatim in every report's `configuration.skill_root` -field, so an ablation run can never be mistaken for a standard-configuration -one. See `review-suite/scripts/evals/runner.py`'s `target_skill_documents` -docstring and `review-suite/scripts/tests/test_eval_runner.py`'s + +**The committed overlay directories are not themselves valid `--skill-root` +values** - `--skill-root` requires a directory containing all four skills +`review-code-change` declares as its dependency closure (`review-code-change`, +`review-solution-simplicity`, `review-correctness`, `review-code-simplicity`), +and the committed overlay directories intentionally hold only the one changed +file per configuration, to avoid duplicating unchanged skill content in git. The +two scored runs actually pointed `--skill-root` at a fresh local directory built +by mirroring the real `skills/` tree, then overwriting only +`review-correctness/SKILL.md` with the committed overlay file's content, +immediately before launch. Each report's own `configuration.skill_root` field +records the exact (ephemeral, machine-local) path used; that path does not +survive as a stable, reproducible location, but the content it held is verified +byte-identical to the committed overlay file modulo `mdformat`'s line-wrap width +(the overlay files were reformatted by `just fmt-md` after the scored runs +launched; a direct `diff` shows only rewrapped line breaks, no textual change) - +re-diffed directly against the committed files as part of this ticket's own +review before publication. + +To reproduce either scored configuration: copy the real `skills/` tree to a +fresh directory, overwrite `/review-correctness/SKILL.md` with the content +of the matching committed overlay file above, then pass that copy's path as +`--skill-root` to `runner.py` (or `just eval-review-suite`). The resolved +`skill_root` is always recorded verbatim in every report's +`configuration.skill_root` field, so an ablation run can never be mistaken for a +standard-configuration one. See `review-suite/scripts/evals/runner.py`'s +`target_skill_documents` docstring and +`review-suite/scripts/tests/test_eval_runner.py`'s `test_an_overridden_skill_root_is_recorded_and_changes_the_closure` for the mechanism's own tests. diff --git a/review-suite/evals/v2/S1-ABLATION-MATRIX.md b/review-suite/evals/v2/S1-ABLATION-MATRIX.md index 90eaf25..bf00476 100644 --- a/review-suite/evals/v2/S1-ABLATION-MATRIX.md +++ b/review-suite/evals/v2/S1-ABLATION-MATRIX.md @@ -25,11 +25,11 @@ supports that explanation. ## The three configurations -| Configuration | `suite_commit` | `skill_root` | Attempts | Cost (USD) | Mean latency | -| -------------------------------------------------------------------------------------- | ------------------------------------------ | ----------------------------------------- | -------- | ---------- | ------------ | -| Both passes together (reused, scored by a predecessor task at this same skill content) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | real `skills/` tree | 35 | 3.559636 | 49.04s | -| Traversal-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `ablation-skill-roots/traversal-only/` | 35 | 3.560108 | 43.62s | -| Verification-sufficiency-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | `ablation-skill-roots/verification-only/` | 35 | 3.866613 | 50.93s | +| Configuration | `suite_commit` | `skill_root` (see `FROZEN-V2-CONFIGURATION.md` for the exact recorded path and reproduction steps) | Attempts | Cost (USD) | Mean latency | +| -------------------------------------------------------------------------------------- | ------------------------------------------ | --------------------------------------------------------------------------------------------------------- | -------- | ---------- | ------------ | +| Both passes together (reused, scored by a predecessor task at this same skill content) | `2c671fd75f4bc11b3137f8d615764ac6c4e21851` | real `skills/` tree | 35 | 3.559636 | 49.04s | +| Traversal-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | ephemeral mirror of `skills/` overlaid with the committed `traversal-only/review-correctness/SKILL.md` | 35 | 3.560108 | 43.62s | +| Verification-sufficiency-pass-only (new, this ticket) | `e2c56f68fe56094a6c92fd4a220539f47d6f9f98` | ephemeral mirror of `skills/` overlaid with the committed `verification-only/review-correctness/SKILL.md` | 35 | 3.866613 | 50.93s | All three: `grader_version 1.1`, `corpus_version 0.1-s1-populated`, `runs_per_case 5`, `cases 7` (35 attempts), `runtime claude 2.1.92`,