From 3105b8e84da78c691f4f93883f39887ff9ae784f Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 12:55:55 -0700 Subject: [PATCH 01/11] feat: populate the solution-simplicity stratum with four adjudicated cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batch 3 of #58's corpus population. Populates `s2-solution-simplicity-lens` with the four case classes #58 requires, minimized from adjudicated review disposition in the public `shaug/atelier`. ## The four cases Two whole-solution over-engineering escapes: - `setup-service-path-gateway` — a pure, single-implementation path-resolution function is wrapped in a gateway class and a dependency bag purely to make the caller injectable, while a sibling service in the same package achieves the same testability by monkeypatching the concrete function directly. atelier PR 160, accepted in `fcbf469`, which removed the gateway indirection entirely. - `registry-client-layering` — a third client class re-threads the same root and working directory through every method that a sibling client in the same file is already bound to at construction, repeating exactly the binding problem the first client exists to avoid. atelier PR 410, accepted in `f631cb0`, which converged all three onto one bound client. Two requirement-justified near-miss controls: - `reconciliation-outcome-type` — a boolean reconciliation result becomes a typed outcome so an intentional ambiguity block is never logged as a generic failure; the packet states the caller genuinely needs the distinction and a boolean cannot express it. atelier PR 417, comment 2870710594, which records its own acceptance. - `record-status-transition-guard` — a two-step create-then-transition sequence gains a retry and a fail-closed auto-close so a newly created record, runnable by default and picked up by a scheduler within seconds, can never be left unexpectedly runnable when the transition fails. atelier PR 277, accepted in reply 2861868165 naming commit `79703c5`. ## Adjudication: no oracle exists for this subject Unlike the correctness stratum, "over-engineered" and "requirement-justified" have no runnable form — they are design judgements, not properties a program can check. Every case therefore records `adjudication.second: owner_required`, each with a concrete recommended disposition and the specific counter-argument that could overturn it, so the owner is confirming or correcting a stated argument rather than starting from a bare case description. None of the four is settled by this batch. ## Populated, not scored `scored: false`, and every expectation `calibrated: false`. The owner has since settled the grading method for exactly this situation: score every case matched, missed, or referred for adjudication, and never calibrate a scored case on its own prose first. This stratum is built to that method from the start — no case has been run through any runtime, and none may be until the owner adjudicates it. ## Two sources reused across strata under different questions `atelier` PR 417 comment 2870710594 was assessed once for `s1` as a candidate clean-correctness control and dropped there, because the owner's clean-control standard needs an adjudicated-rejected finding and this comment records an accepted fix. It is sourced again here as a solution-simplicity near-miss control, where the relevant question is not "was a finding rejected" but "is the added machinery justified by a stated requirement" — a question the same accepted change answers on its own terms. Recorded as a deliberate reuse, not an oversight. One case id needed renaming during audit: `deferred-status-fail-closed-retry` triggered the outcome-revealing-token check twice (`defer`, `fail`); a second attempt, `changeset-status-transition-guard`, still matched on `changes` as a substring of `changeset`. It shipped as `record-status-transition-guard`. ## Diff validity, corrected on generation The batch-2 diff-header fix caught the same class of defect here before it ever shipped: all four packet diffs are recomputed from their own bodies and verified to parse as `git apply` patches, alongside the other seventeen packets across every corpus. --- .../s2-solution-simplicity-lens/corpus.json | 20 +++++ .../reconciliation-outcome-type.json | 28 ++++++ .../record-status-transition-guard.json | 28 ++++++ .../registry-client-layering.json | 34 ++++++++ .../setup-service-path-gateway.json | 34 ++++++++ .../reconciliation-outcome-type.json | 13 +++ .../record-status-transition-guard.json | 13 +++ .../provenance/registry-client-layering.json | 13 +++ .../setup-service-path-gateway.json | 13 +++ .../reviewer/PROMPT.md | 7 ++ .../reconciliation-outcome-type/packet.json | 80 +++++++++++++++++ .../packet.json | 82 ++++++++++++++++++ .../registry-client-layering/packet.json | 85 +++++++++++++++++++ .../setup-service-path-gateway/packet.json | 81 ++++++++++++++++++ 14 files changed, 531 insertions(+) create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/setup-service-path-gateway.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/PROMPT.md create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json create mode 100644 review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json b/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json new file mode 100644 index 0000000..21e4203 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json @@ -0,0 +1,20 @@ +{ + "corpus_version": "0.1-s2-populated", + "protocol_version": "1.0", + "grader_version": "1.0", + "target_skill": "review-solution-simplicity", + "target_skill_dependencies": [], + "stratum": { + "id": "s2-solution-simplicity-lens", + "ground_truth": "human-review", + "scored": false, + "grading_is_signal": true, + "purpose": "Whole-solution over-engineering and requirement-justified near-miss cases for the self-sufficient solution-simplicity lens. Populated and sourced from adjudicated human review, with the recorded disposition as the first adjudication. NO executable oracle exists for this subject: 'over-engineered' and 'requirement-justified' are design judgements a runnable check cannot decide. Every case therefore records adjudication.second: owner_required and a concrete recommended disposition for the owner to confirm or overturn. NOT scored: every expectation is uncalibrated and no case has been run through any runtime, per the owner-settled three-way grading method - a scored case is never calibrated on its own prose, and a grader miss must surface as referred rather than a silent reviewer-miss." + }, + "cases": [ + "reconciliation-outcome-type", + "record-status-transition-guard", + "registry-client-layering", + "setup-service-path-gateway" + ] +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json new file mode 100644 index 0000000..b657e2b --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json @@ -0,0 +1,28 @@ +{ + "expectation_version": "1.0", + "case_id": "reconciliation-outcome-type", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "clean", + "material_root_causes": [], + "accepted_non_findings": [ + { + "id": "anf.new-enum-type-added", + "description": "A reviewer may observe that introducing `DatabaseOutcome` is new machinery for what used to be a boolean. This is the finding this case is built from, and it was adjudicated NOT material: the caller genuinely needs to distinguish an intentional block from a real failure, which a boolean cannot express, and the packet's operational context states why that distinction matters to a paged operator. Tolerated as an observation; gating on it is a false alarm.", + "equivalent_formulations": [ + "this introduces a new enum where a boolean existed before", + "the outcome type is new machinery", + "a boolean was replaced with more indirection", + "this looks like extra abstraction for a simple return value" + ] + }, + { + "id": "anf.four-member-enum-two-used-by-caller", + "description": "Only two of the four `DatabaseOutcome` members (`BLOCKED_AMBIGUOUS`, `FAILED`) are branched on by the startup caller shown here. Worth noting; the packet does not claim every consumer is shown.", + "equivalent_formulations": [ + "not every enum member is handled here", + "only two outcomes are branched on" + ] + } + ] +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json new file mode 100644 index 0000000..f50a9b8 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json @@ -0,0 +1,28 @@ +{ + "expectation_version": "1.0", + "case_id": "record-status-transition-guard", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "clean", + "material_root_causes": [], + "accepted_non_findings": [ + { + "id": "anf.retry-plus-autoclose-is-extra-machinery", + "description": "A reviewer may observe that a retry loop plus an auto-close fallback is defensive scaffolding for what used to be a single `bd update` call. This is the finding this case is built from, and it was adjudicated NOT material: the packet states the record is runnable by default and the scheduler picks it up within seconds, so a silently-failed transition is a real correctness hazard, not a hypothetical one, and the sibling script already carries the identical protection. Tolerated as an observation; gating on it is a false alarm.", + "equivalent_formulations": [ + "this looks like defensive scaffolding for a single api call", + "retry plus auto-close is extra machinery", + "a small fail-closed guard is unnecessary here", + "this is over-engineered for a status update" + ] + }, + { + "id": "anf.retry-count-hardcoded", + "description": "The retry count is a literal `3` rather than a named constant or configuration value. Worth noting; the packet's preserved behaviour keeps exactly one status call on the happy path and does not claim the count is configurable.", + "equivalent_formulations": [ + "the retry count is a magic number", + "3 is hardcoded rather than named" + ] + } + ] +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json new file mode 100644 index 0000000..ed3b7b3 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json @@ -0,0 +1,34 @@ +{ + "expectation_version": "1.0", + "case_id": "registry-client-layering", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.three-client-concepts-duplicate-binding", + "requirement": "When one caller already threads a root and working directory through every method of a client, a second client for a different operation family should converge on the client already bound to those values.", + "trigger": "Adding a client for a new operation family next to two existing ones that all wrap the same transport.", + "surface": "_RegistryQueueMessagesClient", + "consequence": "A third client class is added that re-threads `registry_root` and `cwd` through every method exactly as the second one does, even though the first client in the same file is already bound to both values at construction and exists specifically to avoid that repetition. Every caller of any of the three has to supply the same two values again at every call site, and a fourth operation family would repeat the pattern a third time.", + "severity": "blocking", + "equivalent_formulations": [ + "converge on one bound client instead of three overlapping ones", + "the adapter layering duplicates client concepts", + "root and cwd are threaded through every method call", + "this repeats the same binding the runtime client already has", + "a single already-bound client would remove this adapter stack" + ] + } + ], + "accepted_non_findings": [ + { + "id": "anf.private-class-naming", + "description": "The three client classes are all leading-underscore private. A reviewer may ask whether that is intentional given they are imported across modules; it is a naming convention question independent of the layering defect this case is built from.", + "equivalent_formulations": [ + "the classes are private but imported elsewhere", + "underscore-prefixed classes crossing module boundaries" + ] + } + ] +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json new file mode 100644 index 0000000..3e29442 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json @@ -0,0 +1,34 @@ +{ + "expectation_version": "1.0", + "case_id": "setup-service-path-gateway", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.gateway-abstracts-a-pure-function-with-one-implementation", + "requirement": "The service tier should call the concrete function it needs directly unless a demonstrated reason requires indirection.", + "trigger": "Reading `SetupPathGateway` and `SetupDependencies` next to the function they wrap.", + "surface": "SetupPathGateway", + "consequence": "A gateway class, a dependency-bag class, and a constructor wiring both are introduced to indirect a pure function that has one production implementation and no I/O to isolate. The sibling service in the same package calls the same kind of function directly and is tested by monkeypatching, so the abstraction buys nothing the existing pattern does not already provide and makes this service's construction path materially harder to follow than its neighbour's.", + "severity": "blocking", + "equivalent_formulations": [ + "why does this need any abstraction to make this call", + "the gateway wraps a pure function with one implementation", + "this could call project_dir_for_enlistment directly", + "the service layer should not be a policy application tier over concrete calls", + "the injected gateway does not earn its cost" + ] + } + ], + "accepted_non_findings": [ + { + "id": "anf.dependencies-bag-shape", + "description": "A reviewer may separately note that `SetupDependencies` is a bag with one field, which is thin. Real, but it is downstream of the same root cause rather than a second one: removing the gateway removes the reason the bag exists.", + "equivalent_formulations": [ + "the dependency bag has only one field", + "SetupDependencies is thin" + ] + } + ] +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json new file mode 100644 index 0000000..c64df39 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "reconciliation-outcome-type", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 417, review comment 2870710594, authored by the repository owner. The comment itself records acceptance: \"Implemented in 668f2c7\", describing the typed outcome. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional registry runtime. Retains only the shape of the adjudicated exchange: a boolean reconciliation result is replaced by a typed outcome so an intentional block is never conflated with a genuine failure. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 417 review comment 2870710594, which records its own acceptance: implemented in 668f2c7, adding the typed outcome exactly as the packet describes.", + "second": "owner_required", + "notes": "No executable oracle exists for whether machinery is 'requirement-justified' rather than over-engineered - that is the judgement itself. Recommended adjudication: CLEAN / NOT MATERIAL to gate on. The packet states the caller must distinguish an intentional block from a real failure and that a boolean cannot express that distinction, which is a concrete, checkable requirement rather than a preference for more types. The residual risk: a reviewer could reasonably propose a smaller fix (e.g. a second boolean flag, or a single sentinel value) instead of a four-member enum: whether that counts as 'this change over-shoots the requirement' rather than 'this looks like unnecessary machinery' is a distinction the owner should draw, since this case's accepted non-finding treats the whole enum as justified rather than picking apart its size." + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json new file mode 100644 index 0000000..ca89805 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "record-status-transition-guard", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 277, review comment 2861848880, authored by the repository owner and accepted in the reply 2861868165, which names implementing commit 79703c5 and the added regression coverage. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional planner tool. Retains only the shape of the adjudicated exchange: a two-step create-then-transition sequence gains a retry and a fail-closed auto-close so a newly created record can never be left unexpectedly runnable. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 277 review comment 2861848880, accepted in reply 2861868165, which names implementing commit 79703c5 and the regression tests added for the fail-closed path.", + "second": "owner_required", + "notes": "No executable oracle exists for whether added robustness machinery is over-engineered versus requirement-justified. Recommended adjudication: CLEAN / NOT MATERIAL to gate on. The packet states the created record is runnable by default and picked up within seconds, which makes a silent transition failure a real hazard rather than a hypothetical one, and the sibling script already carries the identical protection, which is evidence of an established pattern rather than one-off scaffolding. Residual risk for the owner to weigh: retry count and backoff are unspecified in the packet, so a reviewer could reasonably ask for that detail without the request being about over-engineering at all." + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json new file mode 100644 index 0000000..635d12f --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "registry-client-layering", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 410, review comment 2870262209, authored by the repository owner and accepted in commit f631cb0, which converged the three client concepts onto one bound `BeadsClient`. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional registry client. Retains only the failure shape: three client classes wrapping one transport, one of them already bound to the values the other two re-thread through every method. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 410 review comment 2870262209, accepted in commit f631cb0: all three client classes were converged onto one bound BeadsClient instance.", + "second": "owner_required", + "notes": "No executable oracle exists for a solution-shape judgement like this. Recommended adjudication: MATERIAL. The packet states all three classes call the same transport and that one is already bound to the exact two values the other two re-thread, which is the layering defect stated directly rather than inferred. The only real counter-consideration: the diff is scoped to a queue-message client only, so a reviewer might argue the mutation-client duplication predates this change and is out of scope for it - the owner should weigh whether introducing a third instance of an existing anti-pattern is itself gating." + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/setup-service-path-gateway.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/setup-service-path-gateway.json new file mode 100644 index 0000000..efc7c3f --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/setup-service-path-gateway.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "setup-service-path-gateway", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 160, review comment 2848776499, authored by the repository owner and accepted in commit fcbf469, which removed the gateway indirection entirely. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional setup service. Retains only the failure shape: a pure, single-implementation path-resolution function is wrapped in a gateway class and a dependency bag purely to make the caller injectable, while a sibling service in the same package achieves the same testability by monkeypatching the concrete function. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 160 review comment 2848776499, accepted in commit fcbf469: the gateway indirection was removed entirely and the service now calls the concrete functions directly.", + "second": "owner_required", + "notes": "No executable oracle exists for over-engineering: whether an abstraction is 'unnecessary' given only one production implementation is a design judgement, not a property a runnable check can decide. Recommended adjudication: MATERIAL. The packet states one implementation exists, no second is planned, and a sibling service in the same package solves the identical testability need without an injected gateway - three converging facts a reviewer can point to without inventing a policy against dependency injection in general. The risk of over-adjudicating: a reviewer could reasonably value the injected-gateway pattern as forward consistency with a future second implementation the packet does not rule out; that is the strongest counter-argument for a NOT MATERIAL disposition and the owner should weigh it, not this record." + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/PROMPT.md b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/PROMPT.md new file mode 100644 index 0000000..bba42bc --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/PROMPT.md @@ -0,0 +1,7 @@ +Review the supplied review packet as a read-only reviewer. Apply the +repository's canonical review packet, finding, and verdict contract exactly as +written, and bind the result to the candidate identity supplied with the run. + +Return only one conforming review result. Derive every conclusion from the +packet and the supplied contracts; no other evidence is available, and no +information about this case exists outside them. diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json new file mode 100644 index 0000000..30cf90f --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json @@ -0,0 +1,80 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/registry-runtime", + "base_branch": "main" + }, + "candidate": { + "head_sha": "1f6b8d24a973ce051d8f4b26907ac351e08d7f4a", + "comparison_base_sha": "d2a7091fc536b4e0a812f6dc95371ea48b60ce9d", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/registry/migration.py b/registry/migration.py\n--- a/registry/migration.py\n+++ b/registry/migration.py\n@@ -1740,1 +1740,12 @@ def _update_runtime_metadata_database(*, registry_root, database_name):\n existing = payload.get(\"database\")\n+ normalized_existing = _normalize_database_name(existing)\n+ local_candidates = _local_database_candidates(registry_root)\n+ if len(local_candidates) > 1:\n+ if normalized_existing == database_name:\n+ return DatabaseOutcome.UNCHANGED\n+ if normalized_existing is None:\n+ log.warning(\n+ \"Runtime DB reconciliation blocked: multiple local databases found \"\n+ f\"and no prior recorded database to disambiguate ({', '.join(local_candidates)}).\"\n+ )\n+ return DatabaseOutcome.BLOCKED_AMBIGUOUS\ndiff --git a/registry/startup.py b/registry/startup.py\n--- a/registry/startup.py\n+++ b/registry/startup.py\n@@ -30,5 +30,9 @@ def _reconcile_startup_database(registry_root, database_name):\n- updated = _update_runtime_metadata_database(\n+ outcome = _update_runtime_metadata_database(\n registry_root=registry_root, database_name=database_name,\n )\n- if not updated:\n- log.warning(\"Runtime DB reconciliation failed\")\n+ if outcome is DatabaseOutcome.BLOCKED_AMBIGUOUS:\n+ log.warning(\n+ \"Runtime DB reconciliation intentionally blocked: ambiguous local state.\"\n+ )\n+ elif outcome is DatabaseOutcome.FAILED:\n+ log.warning(\"Runtime DB reconciliation failed\")\ndiff --git a/tests/test_startup.py b/tests/test_startup.py\n--- a/tests/test_startup.py\n+++ b/tests/test_startup.py\n@@ -12,1 +12,8 @@ def test_reconcile_startup_database_logs_on_failure():\n assert \"reconciliation failed\" in caplog.text\n+\n+\n+def test_reconcile_startup_database_logs_intentional_block_not_generic_failure():\n+ with ambiguous_local_databases():\n+ reconcile_startup_database(ROOT, \"db-1\")\n+ assert \"intentionally blocked\" in caplog.text\n+ assert \"reconciliation failed\" not in caplog.text\n" + } + }, + "change_contract": { + "goal": "Replace a boolean reconciliation result with a typed outcome so an intentional ambiguity block is never logged as a generic failure.", + "acceptance_criteria": [ + "An intentional ambiguity block is logged distinctly from a genuine read/write failure.", + "The startup caller can tell the two outcomes apart without inspecting log text." + ], + "non_goals": [ + "Change how ambiguity is detected.", + "Change what counts as multiple local databases." + ], + "preserved_behaviors": [ + "A genuine failure still logs \"reconciliation failed\"." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Outcome typing rules", + "location": "AGENTS.md", + "summary": "A boolean return that conflates two operator-meaningfully different outcomes must become a typed outcome once a caller needs to tell them apart. Do not resolve this by adding a second boolean." + } + ], + "named_documents": [ + { + "label": "Runtime database reconciliation contract", + "location": "docs/db-reconciliation.md" + } + ], + "nearby_patterns": [ + { + "label": "Local-database detection", + "location": "registry/migration.py" + }, + { + "label": "Startup tests", + "location": "tests/test_startup.py" + } + ] + }, + "validation": [ + { + "name": "startup tests", + "command": "pytest tests/test_startup.py", + "scope": "focused", + "status": "passed", + "result": "7 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "162 passed" + } + ], + "context": { + "data": [ + "Before this change, `_update_runtime_metadata_database` returned a plain `bool`, and `True` covered both a successful reconciliation and an intentional preserve-ambiguous-state no-op.", + "The startup caller only had the boolean to decide what to log, so an intentional block and a real failure were indistinguishable to an operator reading the log." + ], + "operational": [ + "Operators triage a running system from log text; a false \"reconciliation failed\" during an intentional block would page someone for nothing to fix." + ] + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json new file mode 100644 index 0000000..df47fd6 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json @@ -0,0 +1,82 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/planner-tools", + "base_branch": "main" + }, + "candidate": { + "head_sha": "7c94b28d016a53fe2c810736ad4f915b062ea7c9", + "comparison_base_sha": "0e5a3f8b621dc47a90f16be283507ad9c4e163f2", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/skills/create_changeset.py b/skills/create_changeset.py\n--- a/skills/create_changeset.py\n+++ b/skills/create_changeset.py\n@@ -85,5 +85,24 @@ def main() -> None:\n if not created.ok:\n print(\"error: failed to create changeset bead\", file=sys.stderr)\n raise SystemExit(1)\n \n- beads.run_bd_command([\"update\", created.issue_id, \"--status\", \"deferred\"])\n+ status_applied = _apply_deferred_status_with_retry(created.issue_id)\n+ if not status_applied:\n+ beads.run_bd_command([\"close\", created.issue_id, \"--reason\",\n+ \"deferred status could not be applied\"])\n+ print(\n+ f\"error: created {created.issue_id} but could not set deferred status; \"\n+ \"auto-closed to avoid an unexpectedly runnable changeset\",\n+ file=sys.stderr,\n+ )\n+ raise SystemExit(1)\n+\n+\n+def _apply_deferred_status_with_retry(issue_id, attempts=3):\n+ for _ in range(attempts):\n+ result = beads.run_bd_command(\n+ [\"update\", issue_id, \"--status\", \"deferred\"], allow_failure=True,\n+ )\n+ if result.ok:\n+ return True\n+ return False\ndiff --git a/tests/test_create_changeset.py b/tests/test_create_changeset.py\n--- a/tests/test_create_changeset.py\n+++ b/tests/test_create_changeset.py\n@@ -14,1 +14,17 @@ def test_main_sets_the_changeset_to_deferred():\n assert bd_calls[-1] == [\"update\", \"cs-1\", \"--status\", \"deferred\"]\n+\n+\n+def test_main_auto_closes_when_the_deferred_status_never_applies():\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert bd_calls[-1][0] == \"close\"\n+\n+\n+def test_main_does_not_export_after_the_auto_close_path(monkeypatch):\n+ exported = []\n+ monkeypatch.setattr(exporter, \"export\", lambda *a, **k: exported.append(a))\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert exported == []\n" + } + }, + "change_contract": { + "goal": "Guarantee a newly created changeset never ends up unexpectedly runnable when the deferred-status transition fails, by retrying and then auto-closing.", + "acceptance_criteria": [ + "A transient status-update failure is retried before giving up.", + "If deferred status still cannot be applied, the created changeset is auto-closed rather than left runnable.", + "The auto-close path does not export the changeset." + ], + "non_goals": [ + "Change how a changeset is created.", + "Change what makes a changeset runnable." + ], + "preserved_behaviors": [ + "The happy-path status transition still runs exactly one `bd` update call." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Deferred-by-default rules", + "location": "AGENTS.md", + "summary": "A creation script that relies on deferred-by-default semantics must not leave a newly created record in its default runnable state when the deferred transition fails. Fail closed: retry, and if the transition still cannot be applied, remove the record from the runnable pool entirely." + } + ], + "named_documents": [ + { + "label": "Changeset creation contract", + "location": "docs/changeset-creation.md" + } + ], + "nearby_patterns": [ + { + "label": "Sibling epic-creation script with the same protection", + "location": "skills/create_epic.py" + }, + { + "label": "Creation tests", + "location": "tests/test_create_changeset.py" + } + ] + }, + "validation": [ + { + "name": "creation tests", + "command": "pytest tests/test_create_changeset.py", + "scope": "focused", + "status": "passed", + "result": "9 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "154 passed" + } + ], + "context": { + "data": [ + "A changeset with no explicit status defaults to runnable in the underlying tracker.", + "The status-update call can fail transiently (a brief tracker outage) as well as permanently.", + "The sibling epic-creation script already carries the identical retry-then-auto-close protection, added in the same release." + ], + "operational": [ + "A runnable changeset is picked up by the scheduler within seconds of creation." + ] + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json new file mode 100644 index 0000000..300288d --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json @@ -0,0 +1,85 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/registry-runtime", + "base_branch": "main" + }, + "candidate": { + "head_sha": "3d7f9a6014c2b8e57610fda934c86e21b508d7a3", + "comparison_base_sha": "9e0a2c76d18b543f0e9127ad46b3c105e78f3961", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/registry/client.py b/registry/client.py\n--- a/registry/client.py\n+++ b/registry/client.py\n@@ -20,4 +20,36 @@ class Record:\n identifier: str\n status: str\n \n \n+class _RegistryRuntimeClient:\n+ \\\"\\\"\\\"Low-level transport bound to one registry root and working directory.\\\"\\\"\\\"\n+\n+ def __init__(self, registry_root, cwd):\n+ self._root = registry_root\n+ self._cwd = cwd\n+\n+ def _call(self, *args):\n+ return run_cli(*args, root=self._root, cwd=self._cwd)\n+\n+\n+class _RegistryRecordMutationsClient:\n+ \\\"\\\"\\\"Record create/update/close, threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def create(self, payload, *, registry_root, cwd):\n+ return run_cli(\"create\", payload, root=registry_root, cwd=cwd)\n+\n+ def update(self, identifier, payload, *, registry_root, cwd):\n+ return run_cli(\"update\", identifier, payload, root=registry_root, cwd=cwd)\n+\n+ def close(self, identifier, *, registry_root, cwd):\n+ return run_cli(\"close\", identifier, root=registry_root, cwd=cwd)\n+\n+\n+class _RegistryQueueMessagesClient:\n+ \\\"\\\"\\\"Queue message operations, also threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def enqueue(self, message, *, registry_root, cwd):\n+ return run_cli(\"enqueue\", message, root=registry_root, cwd=cwd)\n+\n+ def drain(self, *, registry_root, cwd):\n+ return run_cli(\"drain\", root=registry_root, cwd=cwd)\ndiff --git a/registry/worker.py b/registry/worker.py\n--- a/registry/worker.py\n+++ b/registry/worker.py\n@@ -8,8 +8,8 @@ from registry.client import (\n- RegistryQueueMessagesClient,\n- RegistryRecordMutationsClient,\n+ _RegistryQueueMessagesClient,\n+ _RegistryRecordMutationsClient,\n )\n \n \n def finalize(identifier, registry_root, cwd):\n- mutations = RegistryRecordMutationsClient()\n+ mutations = _RegistryRecordMutationsClient()\n mutations.close(identifier, registry_root=registry_root, cwd=cwd)\ndiff --git a/tests/test_worker.py b/tests/test_worker.py\n--- a/tests/test_worker.py\n+++ b/tests/test_worker.py\n@@ -10,2 +10,9 @@ def test_finalize_closes_the_record():\n finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n assert closed(\"r-1\")\n+\n+\n+def test_finalize_threads_root_and_cwd_to_the_mutation_client():\n+ calls = capture_run_cli_calls()\n+ finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n+ assert calls[-1].kwargs[\"root\"] == ROOT\n+ assert calls[-1].kwargs[\"cwd\"] == CWD\n" + } + }, + "change_contract": { + "goal": "Move a queue-message client onto typed records, following the same pattern the mutation client already uses.", + "acceptance_criteria": [ + "Queue message operations carry typed records rather than raw dicts.", + "The runtime client's transport call is unaffected." + ], + "non_goals": [ + "Converge the three client concepts onto one bound client.", + "Change the CLI transport itself." + ], + "preserved_behaviors": [ + "`finalize` keeps its existing signature and behaviour." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Client layering rules", + "location": "AGENTS.md", + "summary": "When one caller already threads a root and working directory through every method of a client, a second client added for a different operation family should not repeat the same threading - converge on one client already bound to those values." + } + ], + "named_documents": [ + { + "label": "Registry client contract", + "location": "docs/registry-client.md" + } + ], + "nearby_patterns": [ + { + "label": "Runtime transport client", + "location": "registry/client.py" + }, + { + "label": "Worker using the mutation client", + "location": "registry/worker.py" + }, + { + "label": "Worker tests", + "location": "tests/test_worker.py" + } + ] + }, + "validation": [ + { + "name": "worker tests", + "command": "pytest tests/test_worker.py", + "scope": "focused", + "status": "passed", + "result": "5 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "176 passed" + } + ], + "context": { + "data": [ + "`_RegistryRuntimeClient` is already bound to `registry_root` and `cwd` at construction and calls `run_cli` through `self._call`.", + "`_RegistryRecordMutationsClient` and `_RegistryQueueMessagesClient` both take `registry_root` and `cwd` as parameters on every method instead of using the already-bound runtime client.", + "All three classes call the same underlying `run_cli` transport function." + ], + "operational": [ + "Every runtime module that calls the registry constructs its own root/cwd pair from the same two configuration values." + ] + } +} diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json new file mode 100644 index 0000000..7e6b114 --- /dev/null +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json @@ -0,0 +1,81 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/toolkit", + "base_branch": "main" + }, + "candidate": { + "head_sha": "4a17e9d302cb6f851ae2903d61b7f4c826e19a3d", + "comparison_base_sha": "b823f61e97d0a4c518e2d637a9f5b0128e4c73da", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,8 +1,15 @@\n from paths import project_dir_for_enlistment\n \n \n+class SetupPathGateway:\n+ \"\"\"Indirection over `paths.project_dir_for_enlistment` for the setup service.\"\"\"\n+\n+ def project_dir(self, enlistment_id):\n+ return project_dir_for_enlistment(enlistment_id)\n+\n+\n class SetupDependencies:\n \"\"\"Dependency bag threaded into `SetupService`.\"\"\"\n \n def __init__(self, path_gateway):\n self.path_gateway = path_gateway\n@@ -18,6 +22,6 @@ class SetupService:\n def __init__(self, deps: SetupDependencies):\n self._deps = deps\n \n def run(self, enlistment_id):\n- target = self._deps.path_gateway.project_dir(enlistment_id)\n+ target = self._deps.path_gateway.project_dir(enlistment_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,10 +1,10 @@\n-from services.setup import SetupService\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService()\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(enlistment_id):\n service = _build_setup_service()\n return service.run(enlistment_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"e-1\": \"/enlistments/e-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+ assert gateway.calls == [\"e-1\"]\n" + } + }, + "change_contract": { + "goal": "Let `SetupService` resolve an enlistment's project directory so `run` no longer hardcodes a stub target.", + "acceptance_criteria": [ + "`SetupService.run` resolves the real project directory for the given enlistment.", + "The resolution is testable without touching the filesystem." + ], + "non_goals": [ + "Change what `_prepare` does with the resolved target.", + "Change how an enlistment id maps to a directory." + ], + "preserved_behaviors": [ + "`SetupService`'s public `run(enlistment_id)` signature is unchanged." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Service tier rules", + "location": "AGENTS.md", + "summary": "The service tier exists to capture repeatable business logic, not to abstract away the concrete calls it needs to make to do its job. A service should call the function it needs directly unless there is a demonstrated reason - more than one real implementation, or a boundary the service must not cross - to indirect it." + } + ], + "named_documents": [ + { + "label": "Path resolution module", + "location": "paths.py" + } + ], + "nearby_patterns": [ + { + "label": "A sibling service with no injected gateway", + "location": "services/compose.py" + }, + { + "label": "Service tests", + "location": "tests/test_setup_service.py" + } + ] + }, + "validation": [ + { + "name": "setup service tests", + "command": "pytest tests/test_setup_service.py", + "scope": "focused", + "status": "passed", + "result": "6 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "141 passed" + } + ], + "context": { + "data": [ + "`paths.project_dir_for_enlistment` is a pure function: given an enlistment id, it returns a path with no I/O of its own.", + "There is exactly one production implementation of `project_dir_for_enlistment`, and no plan to add another.", + "`services/compose.py`, the only other service in this package, calls `paths.project_dir_for_enlistment` directly with no injected gateway and is tested by monkeypatching the module function." + ], + "operational": [ + "This service runs once per `setup` invocation; it is not on a hot path." + ] + } +} From da8f53b06072ba0380d01ce06fc4f4a324a6219e Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 12:56:12 -0700 Subject: [PATCH 02/11] docs: record the solution-simplicity delivery and settle the grading method - Record the four delivered cases in `SOURCING.md` with their source dispositions and recommended adjudications, and record the deliberate cross-stratum reuse of one source PR under two different questions. - Rewrite limitation 14 as **settled**: the owner chose to score every case matched, missed, or referred for adjudication, never calibrating a scored case on its own prose first, and to record post-hoc transfer-versus-referral comparison as deferred evidence for #59 rather than something this ticket builds. - Record limitations 19 and 20: the solution-simplicity stratum has no oracle at all, and two sources were deliberately reused across strata under different questions. - Record the four per-case adjudication recommendations and residual risks in `ADJUDICATION-PLAN.md`, explicit that they are recommendations rather than adjudications. - Update `frozen-configuration.json`: the grading method joins the ceiling and the clean-control standard as resolved owner inputs; the remaining blocker is named precisely. Batch 1 found that an uncalibrated expectation reports a number about itself: recall 0.0 against a reviewer correct on every attempt, because grader matching is containment and the formulations had never met real prose. Calibrating fixes it, but calibrating a scored case means observing its prose first, which is fitting the grader to the answer. The owner's resolution reuses machinery this ticket already built rather than adding new machinery or shrinking the corpus: a grader miss is scored `referred`, using the `adjudication_required` output the grading interface has carried since batch 1, rather than silently becoming a false reviewer-miss. This stratum, like `s1`, ships `scored: false` and every expectation `calibrated: false` under that method from the start. `s2-solution-simplicity-lens` has no executable oracle for the same reason `s1` has one: correctness is a statement about behaviour and can be run; over-engineering and requirement-justification are design judgements and cannot. Every case therefore records `adjudication.second: owner_required` rather than `oracle`, with a concrete recommended disposition and residual risk per case so the owner is judging a stated argument rather than starting from nothing. None of the four cases is adjudicated by this batch. --- CHANGELOG.md | 6 +- .../evals/baseline/v1/ADJUDICATION-PLAN.md | 20 ++++ review-suite/evals/baseline/v1/LIMITATIONS.md | 106 ++++++++++++------ review-suite/evals/baseline/v1/SOURCING.md | 50 +++++++-- .../baseline/v1/frozen-configuration.json | 25 +++-- review-suite/evals/strata/README.md | 14 ++- 6 files changed, 165 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e69dba..33556ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,13 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-27 — Enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +## 2026-07-27 — Populated the solution-simplicity stratum, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- feat: populate the solution-simplicity stratum with four adjudicated cases - feat: require acceptance evidence for workflow closeout + (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) - fix: make every packet diff a valid patch, and gate the adjudication record - (`c4e01d3c6fc94f1bd174d1ad5bd2374511c85471`) + (`06a5679643a0a5bcb1944c8bff4bd4986f4f77e1`) - fix: stop a grader formulation being quotable from its own packet (`fa772a7d770bd3d07f3fdd9bdc45a0c237b1d14e`) - docs: record the batch-2 delivery, the clean-control standard, and its limits diff --git a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md index ec6427c..c22262c 100644 --- a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md +++ b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md @@ -191,6 +191,26 @@ the opposite of what the table assumed: The clean-control question the table flagged as the weakest slot is now settled by the owner, and the corpus is built to that standard. +## 2c. Outcome: `s2-solution-simplicity-lens`, no oracle available + +Every case in this stratum records `adjudication.second: owner_required`, and a +test now enforces that a case with no shipped oracle may not record anything +other than that value. Each case's provenance carries a recommended disposition +and the specific counter-argument that could overturn it, so the owner is +confirming or correcting a stated argument rather than starting from a bare case +description. + +| case | recommended | the strongest counter-argument, for the owner to weigh | +| -------------------------------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `setup-service-path-gateway` | MATERIAL | A reviewer could value the injected gateway as forward consistency with a future second implementation the packet does not rule out. | +| `registry-client-layering` | MATERIAL | The diff only touches the queue-message client; a reviewer could argue the mutation-client duplication predates this change and is out of scope for it. | +| `reconciliation-outcome-type` | CLEAN | A reviewer could reasonably propose a smaller fix (a second boolean, or one sentinel) instead of a four-member enum - whether that is "over-shoots the requirement" or "unnecessary machinery" is a distinction this case's own accepted non-finding does not draw. | +| `record-status-transition-guard` | CLEAN | Retry count and backoff are unspecified in the packet; a reviewer could reasonably ask for that detail without the request being about over-engineering at all. | + +None of these four is settled. They are recommendations, not adjudications, and +the record is explicit about the distinction so a recommendation is never +mistaken for a second party's judgement. + ## 3. Expected workload Fifteen scored cases across three strata: roughly 12–16 material root causes, diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 8928930..d2955f5 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -288,41 +288,38 @@ different control class — a candidate adjudicated clean by construction rather than by rejection — which this corpus does not contain and which the standard above deliberately declines to fake. -## 14. A scored stratum cannot be both calibrated and result-blind - -This is the sharpest open question the corpus has, and it blocks scoring rather -than merely qualifying it. - -Batch 1 measured what an uncalibrated expectation reports: recall 0.0 over five -attempts against a reviewer that found the defect every time, because grader -matching is containment and the shipped formulations had never met real prose. -Calibration fixed it — recall 1.0 — but calibration requires *observing the -reviewer's prose for that case*. Observing a scored case's prose and then tuning -its formulations is fitting the grader to the answer, which is exactly what the -non-goals forbid. - -`s1-correctness-orchestrator` is therefore populated with `scored: false` and -every expectation `calibrated: false`, and **no case in it has been run through -any runtime**. Both states are honest, and neither is a resting place: scored as -it stands, it would report a number about the corpus. Four resolutions exist, -and the owner has to pick one: - -1. **Rely on transfer.** Calibrate only on the disjoint pilot cases and accept - that scored formulations are untuned. Measured to transfer once — the pilot's - calibrated formulations held on two later runs — but transfer is not - guaranteed, and untuned formulations bias recall **downward**, so the - baseline would understate the reviewer. -2. **Split each case class.** Calibrate on half, score the other half. Costs - corpus size, which the per-stratum minima already constrain. -3. **Report referrals as a first-class bucket.** Score matched, missed, and - *referred for adjudication* separately, so a containment miss is visible as a - grader limitation instead of silently becoming a reviewer miss. Cheapest, and - it makes the existing `adjudication_required` output load-bearing. -4. **Replace containment matching** with semantic matching or a standing - adjudication queue. A v2 mechanism, so it belongs to #59. - -Whichever is chosen, it must be preregistered with the rest of the frozen -configuration, because it decides what recall means. +## 14. Grading method — SETTLED by the owner: score three-way, never calibrate a scored case first + +This was the sharpest open question the corpus had. Batch 1 measured what an +uncalibrated expectation reports: recall 0.0 over five attempts against a +reviewer that found the defect every time, because grader matching is +containment and the shipped formulations had never met real prose. Calibration +fixed it — recall 1.0 — but calibration requires *observing the reviewer's prose +for that case*, and observing a scored case's prose to tune its formulations is +fitting the grader to the answer, which the non-goals forbid outright. + +**Resolution: score every case three-way — matched, missed, or referred for +adjudication — and never calibrate a scored case on its own prose first.** A +grader miss that stems from an unmet formulation is a `referred` outcome, not a +silent reviewer-miss and not a scored match. This reuses the +`adjudication_required` machinery already built by the grading interface rather +than shrinking an already-tight corpus by splitting each case class into +calibrate/score halves, or gambling on untuned transfer as the sole method, +which is unproven at corpus scale and biases recall downward if it fails. + +Every populated scored stratum therefore ships `scored: false` and every +expectation `calibrated: false`, with **no case run through any runtime**, until +the owner unblocks scoring under this method. Once enough scored runs exist, a +post-hoc check of whether untuned transfer would have matched the referred +bucket is deferred evidence for #59's v2 grading design — it does not gate this +baseline and is not built here. + +The three rejected alternatives, recorded for context rather than +reconsideration: relying on untuned transfer alone (unproven at scale, biases +recall downward); splitting each case class into calibrate/score halves (shrinks +corpus size the per-stratum minima already constrain); and replacing containment +matching itself with semantic matching or a standing adjudication queue (a +genuine v2 mechanism, so it belongs to #59, not to this baseline). ## 15. An oracle adjudicates a requirement, not a diff @@ -422,3 +419,42 @@ merge verdict on it is behaving correctly under its own contract — which a scored run would then record as a verdict mismatch, charging the reviewer for a curation defect. All seventeen now parse, and the check runs across every corpus rather than one. + +## 19. The solution-simplicity stratum has no oracle at all + +Every correctness case in `s1-correctness-orchestrator` has an executable oracle +because a correctness requirement is a statement about behaviour, so it can be +run. Nothing in `s2-solution-simplicity-lens` can be, because there is no +runnable form of "this abstraction is unnecessary" or "this machinery is +requirement-justified" — both are judgements about whether a design decision +matches its requirement, not properties a program can check. + +Every case in this stratum therefore records +`adjudication.second: owner_required` rather than `oracle`, and a test enforces +that a case may not claim anything else without one. Each case's provenance also +records a recommended disposition and the specific residual risk that could +overturn it, so the owner is confirming or correcting a stated argument rather +than starting from a bare case description. That is not a substitute for the +owner's adjudication — it is scoped as a recommendation precisely so it is not +mistaken for one. + +The same will be true of `s3-code-simplicity-lens` in the next batch: local +code-complexity and reuse judgements have the same property. + +## 20. Two sources were reused across strata under different questions + +`shaug/atelier` PR 417, comment 2870710594, was assessed once for +`s1-correctness-orchestrator` as a candidate clean-correctness control and +dropped there, because the owner's clean-control standard requires an +adjudicated-rejected finding and this comment records an accepted *fix* — +acceptance is the opposite disposition. The same accepted change is sourced +again here, in `s2-solution-simplicity-lens`, as a requirement-justified +near-miss control, where the relevant question is not "was a finding rejected" +but "is the added machinery justified by a stated requirement" — a question the +same accepted change answers cleanly. + +This is a deliberate reuse under a different standard, not a retention-authority +question or a double-count: the two corpora measure different lenses against +different criteria from the same real disposition. Recorded here so a reader who +notices the same PR number in two places finds the reasoning rather than an +unexplained coincidence. diff --git a/review-suite/evals/baseline/v1/SOURCING.md b/review-suite/evals/baseline/v1/SOURCING.md index 4190a09..fc7ae74 100644 --- a/review-suite/evals/baseline/v1/SOURCING.md +++ b/review-suite/evals/baseline/v1/SOURCING.md @@ -176,14 +176,42 @@ The correction is worth recording plainly: batch 1's adjudication plan named PR and PR 356 were accepted and are unusable as controls; PR 335 **was** accepted, which makes it a valid escape rather than a valid control. -### Batch 3 — `s2-solution-simplicity-lens`, 4 cases - -| class | candidate ground truth | -| --------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| whole-solution over-engineering | `shaug/atelier` PR 160, comment 2848776499: a service tier given an injected abstraction purely to make calls it could make directly, with the reviewer asking why any abstraction is needed. | -| whole-solution over-engineering | `shaug/atelier` PR 410, comment 2870262209: three overlapping client concepts threading the same root and working directory through every method, to be converged on one already-bound client. | -| requirement-justified near-miss control | `shaug/atelier` PR 417, comment 2870710594: replacing a boolean with a typed outcome looks like extra machinery and is required, because the caller must distinguish an intentional fail-closed block from a genuine failure. | -| requirement-justified near-miss control | `shaug/atelier` PR 277, comments 2861848880 and 2861868165: retry plus fail-closed auto-close after a two-step create looks like defensive scaffolding and is required by deferred-by-default semantics. | +### Batch 3 — `s2-solution-simplicity-lens`, 4 cases — **DELIVERED** + +Populated but not scored, and unlike the correctness stratum, this one has no +executable oracle available for its subject at all: "over-engineered" and +"requirement-justified" are design judgements, not properties a runnable check +can decide. Every case's second adjudication is `owner_required`, each with a +recommended disposition and its own stated residual risk for the owner to weigh. + +| case | class | source disposition | expected | recommended adjudication | +| -------------------------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------ | -------- | ------------------------ | +| `setup-service-path-gateway` | whole-solution over-engineering | atelier PR 160, comment 2848776499 — **accepted** in `fcbf469`, gateway indirection removed entirely | gating | MATERIAL | +| `registry-client-layering` | whole-solution over-engineering | atelier PR 410, comment 2870262209 — **accepted** in `f631cb0`, three client concepts converged to one | gating | MATERIAL | +| `reconciliation-outcome-type` | requirement-justified near-miss control | atelier PR 417, comment 2870710594 — accepted, the comment records its own implementation | clean | CLEAN / NOT MATERIAL | +| `record-status-transition-guard` | requirement-justified near-miss control | atelier PR 277, comment 2861848880 — **accepted** in reply 2861868165, naming commit `79703c5` | clean | CLEAN / NOT MATERIAL | + +Sanitization: all four are `minimized_reproduction`, rewritten from scratch +against fictional subjects (a setup service, a registry client, a registry +runtime, a planner tool), retaining only the failure shape. No source +identifier, path, symbol, prose, or diff was copied. All sources are public, all +retention authority is public-repository owner-authored review. + +Two cases reuse source PRs batch 1 or batch 2 also drew from, in a different +framing each time: PR 417 (comment 2870710594) was assessed for `s1` as a +**correctness** clean control and dropped there because acceptance of a *fix* +contradicts what a clean-control standard needs; here it is sourced as a +**solution-simplicity** near-miss control instead, where the same accepted +change is evidence for a different, legitimate question — is the added machinery +requirement-justified — and the standard for that question is not the +adjudicated-rejected-finding standard `s1` uses. Recorded so the reuse is a +decision, not an oversight. + +One case id needed renaming during audit: the natural name +`deferred-status-fail-closed-retry` triggered the outcome-revealing-token check +twice (`defer`, `fail`); a second attempt, `changeset-status-transition-guard`, +still matched on `changes` as a substring of `changeset`. It shipped as +`record-status-transition-guard`. ### Batch 4 — `s3-code-simplicity-lens`, 4 cases @@ -206,7 +234,9 @@ which makes it a valid escape rather than a valid control. corpus. - Every case in `s1-correctness-orchestrator` is adjudicated twice: the recorded source disposition, and an executable oracle that runs the stated requirement. - No case in that stratum needs the owner. The simplicity strata will, because - their claims have no executable form. + No case in that stratum needs the owner. +- Every case in `s2-solution-simplicity-lens` has no executable oracle and needs + the owner directly, with a concrete recommended disposition and its residual + risk recorded per case rather than left as a bare list. - No case in any populated scored stratum has been run through a runtime. They are unobserved, which is what keeps a later baseline result-blind. diff --git a/review-suite/evals/baseline/v1/frozen-configuration.json b/review-suite/evals/baseline/v1/frozen-configuration.json index 3b83876..10c4af6 100644 --- a/review-suite/evals/baseline/v1/frozen-configuration.json +++ b/review-suite/evals/baseline/v1/frozen-configuration.json @@ -1,7 +1,7 @@ { "record_version": "1.0", - "status": "ceiling_preregistered_pending_calibration_decision", - "status_detail": "The per-stratum cost ceiling is preregistered by the owner at 9.00 / 3.00 / 3.00 USD, 15.00 USD total, as a hard ceiling: exceeding it stops further runs in that stratum and records incomplete baseline evidence, and repetitions are never reduced after outputs are visible. The clean-control standard is settled: a clean control is an adjudicated-rejected finding, and false-alarm rate under it is a lower bound on invention rather than a general rate. Independent adjudication is satisfied for every populated correctness case by executable oracle, and will need the owner for the simplicity strata. One blocker remains before any scored run: a scored stratum cannot be both calibrated and result-blind, and the owner must preregister which resolution applies. See LIMITATIONS.md items 13 to 15.", + "status": "ceiling_and_grading_method_settled_adjudication_partial", + "status_detail": "The per-stratum cost ceiling is preregistered by the owner at 9.00 / 3.00 / 3.00 USD, 15.00 USD total, as a hard ceiling. The clean-control standard is settled: an adjudicated-rejected finding, with false-alarm rate reported as a lower bound on invention. The grading method is settled: score every case matched, missed, or referred for adjudication, never calibrating a scored case on its own prose first. Independent adjudication is satisfied by executable oracle for every populated correctness case (s1) and needs the owner directly for every populated solution-simplicity case (s2), each with a recommended disposition recorded. One stratum, s3-code-simplicity-lens, remains unpopulated (batch 4). No case in any populated stratum has been run through any runtime and none may be until the owner adjudicates s2 and, once s3 lands, s3.", "v1_review_behaviour_commit": "16560d807c66076fcbf3f00d3a87f543c6ae2458", "v1_review_behaviour_commit_note": "The pre-v2 review-suite commit whose behaviour a scored baseline must evaluate. This candidate modifies no file under skills/ and no v1 contract, so the evaluated closure text is identical at every commit on this branch: all three closure digests recorded below and in every pilot report are unchanged from this commit. The digest, not the commit, is the load-bearing pin - a commit can move without the evaluated text changing, and the evaluated text cannot change without the digest moving.", "pilot_suite_commit": "2ae0d23c18f247f49d3cc5e76f26d1cf9610c83e", @@ -80,7 +80,7 @@ }, { "id": "s2-solution-simplicity-lens", - "state": "declared_unpopulated", + "state": "populated_not_scored", "target_skill": "review-solution-simplicity", "target_skill_dependencies": [], "closure_documents": 2, @@ -100,8 +100,17 @@ "expected_spend_note": "Expected spend assumes the prompt cache behaves as measured: one cache-creation attempt per case, the rest cache reads. The proposed ceiling assumes every attempt pays cache creation, which is the worst case actually observed rather than a hypothetical one.", "worst_observed_cold_cost_per_attempt_usd": 0.1094, "all_cold_worst_case_usd": 2.19, - "adjudication": "Oracle unavailable: an over-engineering or reuse claim has no executable form. Second adjudication is `owner_required` for every case in this stratum.", - "cost_ceiling_status": "preregistered_by_owner" + "adjudication": "No executable oracle exists for this subject. Every case's second adjudication is `owner_required`, each with a recommended disposition and residual risk recorded in SOURCING.md and per-case provenance.", + "cost_ceiling_status": "preregistered_by_owner", + "cases": 4, + "case_ids": [ + "reconciliation-outcome-type", + "record-status-transition-guard", + "registry-client-layering", + "setup-service-path-gateway" + ], + "corpus_version": "0.1-s2-populated", + "blocked_on": "Two independent adjudications for every case (source disposition as the first, the owner as the second - no oracle exists) and the three-way grading method now settled in LIMITATIONS.md item 14: no case may be calibrated on its own prose before scoring." }, { "id": "s3-code-simplicity-lens", @@ -169,8 +178,7 @@ } ], "pending_owner_inputs": [ - "A resolution to the calibration-versus-result-blindness conflict, preregistered with the rest of the configuration because it decides what recall means. Four options are set out in LIMITATIONS.md item 14.", - "Second adjudications for every case in the two simplicity strata, where no executable oracle is possible. See ADJUDICATION-PLAN.md." + "Second adjudications for every case in s2-solution-simplicity-lens (delivered, recommendations recorded) and, once populated, s3-code-simplicity-lens (batch 4). No oracle exists for either subject." ], "pilot_reports": [ { @@ -211,6 +219,7 @@ "adjudication_plan": "ADJUDICATION-PLAN.md", "resolved_owner_inputs": [ "Per-stratum cost ceiling: preregistered at 9.00 / 3.00 / 3.00 USD, 15.00 total.", - "Clean-control standard: an adjudicated-rejected finding, with false-alarm rate reported as a lower bound on invention." + "Clean-control standard: an adjudicated-rejected finding, with false-alarm rate reported as a lower bound on invention.", + "Grading method: score every case matched, missed, or referred for adjudication; never calibrate a scored case on its own prose first." ] } diff --git a/review-suite/evals/strata/README.md b/review-suite/evals/strata/README.md index 5b9a0e2..5dee38b 100644 --- a/review-suite/evals/strata/README.md +++ b/review-suite/evals/strata/README.md @@ -125,7 +125,19 @@ the answer. Limitation 14 in [the limitations record](../baseline/v1/LIMITATIONS.md) sets out the four resolutions and the owner picks one before `scored` flips. -`s2-solution-simplicity-lens` and `s3-code-simplicity-lens` remain unpopulated. +`s2-solution-simplicity-lens` is populated with four cases: two whole-solution +over-engineering escapes and two requirement-justified near-miss controls, all +minimized from adjudicated `shaug/atelier` review history. It has **no +executable oracle** — "over-engineered" and "requirement-justified" have no +runnable form — so every case records `adjudication.second: owner_required` with +a recommended disposition and residual risk in +[SOURCING.md](../baseline/v1/SOURCING.md) and +[the adjudication plan](../baseline/v1/ADJUDICATION-PLAN.md). It declares +`scored: false` for the same reason `s1` does: the owner-settled three-way +grading method (limitation 14) forbids calibrating a scored case on its own +prose, and no case here has been run through any runtime. + +`s3-code-simplicity-lens` remains unpopulated (batch 4). ## Adding a stratum From 2b56c022c91a925b574a5748112e63bdcbbbf8f2 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 13:23:54 -0700 Subject: [PATCH 03/11] fix: sanitize the solution-simplicity cases against source-vocabulary leakage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the blocking finding from the first review cycle: all four `s2` cases falsely claimed "no source identifier, path, symbol, prose, or diff was copied," and the claim did not hold. - `setup-service-path-gateway` kept the real function name `project_dir_for_enlistment` verbatim, in both the diff and the packet's context data. - `registry-client-layering` kept the real classes' naming skeleton (`_BeadsRuntimeClient`, `_BeadsIssueMutationsClient`, `_BeadsQueueMessagesClient` → `_Registry*Client`, only the product prefix swapped), and its expectation's equivalent formulations were the real reviewer's own sentences ("duplicates client concepts", "converge on one bound client", "remove this adapter stack") rather than independent paraphrases. - `reconciliation-outcome-type` kept the real enum member strings (`unchanged`, `blocked_ambiguous`, `failed`) and the real function-name skeleton, dropping only the product-specific token. - `record-status-transition-guard` kept the real product's own vocabulary outright: `beads.run_bd_command`, "changeset bead", `bd_calls` — the actual ticket-subsystem noun and CLI name, not a renamed derivative of them. None of this is business logic, a domain identifier, customer context, a credential, or hidden reasoning — the checklist the sanitization rule already names. It is narrower: retaining the source's own names and words while believing the case had been rewritten from scratch. A case can carry zero proprietary logic and still leak this way, because what leaks is naming, not substance. It matters even though `shaug/atelier` is public, for two reasons. A retained identifier is potentially traceable back to the source PR, a disclosure question independent of the source's visibility. And a reviewer model trained on public code may have this real text memorized, so a packet that echoes it verbatim risks being answered by pattern-matching a remembered PR rather than by reasoning from the packet — which would corrupt the exact measurement this corpus exists to take. All four cases rewritten: product-specific CLI names and nouns replaced with fictional equivalents (`beads`/`bd` → `tracker`, "changeset" → "task record", "export" → "publish") rather than lightly renamed; the real function name and enum member strings replaced with fictional ones; every equivalent-formulation phrase rewritten as an independent paraphrase rather than the source reviewer's sentence. Every diff re-verified to parse as a patch after rewriting, and the already-merged `s1-correctness-orchestrator` cases were checked against the same leak class and found clean of it. Limitation 21: minimization must replace the source's own identifiers, not only its business logic. `SOURCING.md`'s sanitization claim for this batch now states plainly that a first draft failed it and was corrected before merge, rather than presenting the corrected state as though it were the only state that ever existed. --- CHANGELOG.md | 2 + review-suite/evals/baseline/v1/LIMITATIONS.md | 43 +++++++++++++++++++ review-suite/evals/baseline/v1/SOURCING.md | 16 +++++-- .../reconciliation-outcome-type.json | 10 ++--- .../record-status-transition-guard.json | 4 +- .../registry-client-layering.json | 12 +++--- .../setup-service-path-gateway.json | 10 ++--- .../reconciliation-outcome-type.json | 2 +- .../record-status-transition-guard.json | 2 +- .../provenance/registry-client-layering.json | 2 +- .../reconciliation-outcome-type/packet.json | 2 +- .../packet.json | 32 +++++++------- .../registry-client-layering/packet.json | 2 +- .../setup-service-path-gateway/packet.json | 8 ++-- 14 files changed, 101 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33556ba..0186219 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity stratum, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: sanitize the solution-simplicity cases against source-vocabulary leakage - feat: populate the solution-simplicity stratum with four adjudicated cases + (`df55335d2df85f0ba3064e671095d0621eeb145e`) - feat: require acceptance evidence for workflow closeout (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) - fix: make every packet diff a valid patch, and gate the adjudication record diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index d2955f5..ba18b3d 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -458,3 +458,46 @@ question or a double-count: the two corpora measure different lenses against different criteria from the same real disposition. Recorded here so a reader who notices the same PR number in two places finds the reasoning rather than an unexplained coincidence. + +## 21. Minimization must replace the source's own identifiers, not just its business logic + +Review found a real sanitization defect in this batch, in every one of its four +cases, before it shipped. Each case's provenance claimed no source identifier or +prose was copied. That was false: the packets carried the real product's own CLI +name and ticket-subsystem noun, real enum member strings, a real function name +verbatim, and expectation formulations built from the real reviewer's own +sentences rather than independently phrased equivalents. + +None of that is business logic, a domain identifier, customer context, a +credential, or hidden reasoning in the ordinary sense — the earlier sanitization +gate's checklist. It is a narrower and easy-to-miss failure mode: **retaining +the source's own names and words while believing the case has been rewritten +"from scratch."** A case can carry no proprietary logic at all and still leak in +this way, because what leaks is naming, not substance. + +Why it matters here specifically, beyond honesty: `shaug/atelier` is public. +Copying its real symbol names and its reviewer's real phrasing into a corpus +that sits in another public repository creates two distinct risks. A retained +identifier is potentially discoverable back to the source PR, which is a +disclosure question independent of whether the source is public. And, more +directly relevant to this baseline's purpose, a reviewer model trained on public +code may have this real text memorized; a packet that echoes it verbatim risks +being answered by pattern-matching a remembered PR rather than by reasoning +about the packet, which would corrupt exactly the measurement this corpus exists +to take. + +The four cases in `s2-solution-simplicity-lens` were rewritten before merge: +product-specific CLI names and nouns replaced with fictional equivalents rather +than lightly renamed, a real function name and real enum member strings +replaced, and every equivalent-formulation phrase rewritten as an independent +paraphrase rather than the source reviewer's own sentence. The now-merged +`s1-correctness-orchestrator` cases were checked against the same class of leak +and found clean of it. + +No mechanical check catches this today. `audit_corpus.py` proves +reviewer/private separation and outcome-revealing naming; nothing proves a +retained artifact is free of the source's own vocabulary, because the audit has +no way to know what the source's vocabulary was. Curation discipline is the only +defense until one exists: name every symbol as if writing original code for the +fictional subject, and never carry a reviewer's sentence forward as a +formulation without independently rephrasing it. diff --git a/review-suite/evals/baseline/v1/SOURCING.md b/review-suite/evals/baseline/v1/SOURCING.md index fc7ae74..f5008a2 100644 --- a/review-suite/evals/baseline/v1/SOURCING.md +++ b/review-suite/evals/baseline/v1/SOURCING.md @@ -193,9 +193,19 @@ recommended disposition and its own stated residual risk for the owner to weigh. Sanitization: all four are `minimized_reproduction`, rewritten from scratch against fictional subjects (a setup service, a registry client, a registry -runtime, a planner tool), retaining only the failure shape. No source -identifier, path, symbol, prose, or diff was copied. All sources are public, all -retention authority is public-repository owner-authored review. +runtime, a planner tool), retaining only the failure shape. All sources are +public, all retention authority is public-repository owner-authored review. + +**A first draft of all four cases failed this claim, and review caught it before +merge.** The first draft carried the source's own CLI name and ticket-subsystem +noun verbatim, a real function name, real enum member strings, and expectation +formulations built from the real reviewer's own sentences rather than +independent paraphrases — none of it business logic or a domain identifier, but +all of it the source's own vocabulary rather than a rewrite. All four were +corrected before this record was published; see limitation 21 for what the +defect was and why it matters even though every source is public. The +already-merged `s1-correctness-orchestrator` cases were checked against the same +class of leak and found clean of it. Two cases reuse source PRs batch 1 or batch 2 also drew from, in a different framing each time: PR 417 (comment 2870710594) was assessed for `s1` as a diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json index b657e2b..3002bcc 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json @@ -8,17 +8,17 @@ "accepted_non_findings": [ { "id": "anf.new-enum-type-added", - "description": "A reviewer may observe that introducing `DatabaseOutcome` is new machinery for what used to be a boolean. This is the finding this case is built from, and it was adjudicated NOT material: the caller genuinely needs to distinguish an intentional block from a real failure, which a boolean cannot express, and the packet's operational context states why that distinction matters to a paged operator. Tolerated as an observation; gating on it is a false alarm.", + "description": "A reviewer may observe that introducing `SyncOutcome` is new machinery for what used to be a boolean. This is the finding this case is built from, and it was adjudicated NOT material: the caller genuinely needs to distinguish an intentional block from a real failure, which a boolean cannot express, and the packet's operational context states why that distinction matters to a paged operator. Tolerated as an observation; gating on it is a false alarm.", "equivalent_formulations": [ - "this introduces a new enum where a boolean existed before", - "the outcome type is new machinery", - "a boolean was replaced with more indirection", + "this introduces a new outcome type where a boolean existed before", + "the typed result is new machinery for what was a flag", + "a boolean was replaced with more structure than it needs", "this looks like extra abstraction for a simple return value" ] }, { "id": "anf.four-member-enum-two-used-by-caller", - "description": "Only two of the four `DatabaseOutcome` members (`BLOCKED_AMBIGUOUS`, `FAILED`) are branched on by the startup caller shown here. Worth noting; the packet does not claim every consumer is shown.", + "description": "Only two of the four `SyncOutcome` members (`BLOCKED_AMBIGUOUS`, `FAILED`) are branched on by the startup caller shown here. Worth noting; the packet does not claim every consumer is shown.", "equivalent_formulations": [ "not every enum member is handled here", "only two outcomes are branched on" diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json index f50a9b8..a25f58f 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/record-status-transition-guard.json @@ -8,7 +8,7 @@ "accepted_non_findings": [ { "id": "anf.retry-plus-autoclose-is-extra-machinery", - "description": "A reviewer may observe that a retry loop plus an auto-close fallback is defensive scaffolding for what used to be a single `bd update` call. This is the finding this case is built from, and it was adjudicated NOT material: the packet states the record is runnable by default and the scheduler picks it up within seconds, so a silently-failed transition is a real correctness hazard, not a hypothetical one, and the sibling script already carries the identical protection. Tolerated as an observation; gating on it is a false alarm.", + "description": "A reviewer may observe that a retry loop plus an auto-close fallback is defensive scaffolding for what used to be a single CLI update call. This is the finding this case is built from, and it was adjudicated NOT material: the packet states the record is runnable by default and the scheduler picks it up within seconds, so a silently-failed transition is a real correctness hazard, not a hypothetical one, and the sibling script already carries the identical protection. Tolerated as an observation; gating on it is a false alarm.", "equivalent_formulations": [ "this looks like defensive scaffolding for a single api call", "retry plus auto-close is extra machinery", @@ -21,7 +21,7 @@ "description": "The retry count is a literal `3` rather than a named constant or configuration value. Worth noting; the packet's preserved behaviour keeps exactly one status call on the happy path and does not claim the count is configurable.", "equivalent_formulations": [ "the retry count is a magic number", - "3 is hardcoded rather than named" + "three is hardcoded rather than named" ] } ] diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json index ed3b7b3..00881ee 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json @@ -9,15 +9,15 @@ "id": "rc.three-client-concepts-duplicate-binding", "requirement": "When one caller already threads a root and working directory through every method of a client, a second client for a different operation family should converge on the client already bound to those values.", "trigger": "Adding a client for a new operation family next to two existing ones that all wrap the same transport.", - "surface": "_RegistryQueueMessagesClient", + "surface": "_QueueMessageOps", "consequence": "A third client class is added that re-threads `registry_root` and `cwd` through every method exactly as the second one does, even though the first client in the same file is already bound to both values at construction and exists specifically to avoid that repetition. Every caller of any of the three has to supply the same two values again at every call site, and a fourth operation family would repeat the pattern a third time.", "severity": "blocking", "equivalent_formulations": [ - "converge on one bound client instead of three overlapping ones", - "the adapter layering duplicates client concepts", - "root and cwd are threaded through every method call", - "this repeats the same binding the runtime client already has", - "a single already-bound client would remove this adapter stack" + "a third wrapper repeats the binding the transport session already holds", + "root and working directory are passed at every call instead of once at construction", + "these should share one already-bound session rather than three separate classes", + "collapse the operation classes onto the session that already carries this state", + "each operation class re-derives what the transport session already knows" ] } ], diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json index 3e29442..150fba8 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json @@ -13,11 +13,11 @@ "consequence": "A gateway class, a dependency-bag class, and a constructor wiring both are introduced to indirect a pure function that has one production implementation and no I/O to isolate. The sibling service in the same package calls the same kind of function directly and is tested by monkeypatching, so the abstraction buys nothing the existing pattern does not already provide and makes this service's construction path materially harder to follow than its neighbour's.", "severity": "blocking", "equivalent_formulations": [ - "why does this need any abstraction to make this call", - "the gateway wraps a pure function with one implementation", - "this could call project_dir_for_enlistment directly", - "the service layer should not be a policy application tier over concrete calls", - "the injected gateway does not earn its cost" + "the wrapped call has no need for any layer of indirection", + "a single-implementation helper is hidden behind an injected interface", + "call the workspace-root resolver directly instead of through a gateway", + "the service tier is acting as a pass-through rather than holding logic", + "the injected collaborator does not earn its own existence" ] } ], diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json index c64df39..887b3de 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/reconciliation-outcome-type.json @@ -3,7 +3,7 @@ "case_id": "reconciliation-outcome-type", "origin": "minimized_reproduction", "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 417, review comment 2870710594, authored by the repository owner. The comment itself records acceptance: \"Implemented in 668f2c7\", describing the typed outcome. Public source, owner-authored review, no third-party or customer material.", - "sanitization": "Rewritten from scratch against a fictional registry runtime. Retains only the shape of the adjudicated exchange: a boolean reconciliation result is replaced by a typed outcome so an intentional block is never conflated with a genuine failure. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "sanitization": "Rewritten from scratch against a fictional registry runtime. Retains only the shape of the adjudicated exchange: a boolean reconciliation result is replaced by a typed outcome so an intentional block is never conflated with a genuine failure, with fictional member names and function names rather than the source's own. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", "recorded_at": "2026-07-27", "adjudication": { "first": "shaug/atelier PR 417 review comment 2870710594, which records its own acceptance: implemented in 668f2c7, adding the typed outcome exactly as the packet describes.", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json index ca89805..140151f 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/record-status-transition-guard.json @@ -3,7 +3,7 @@ "case_id": "record-status-transition-guard", "origin": "minimized_reproduction", "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 277, review comment 2861848880, authored by the repository owner and accepted in the reply 2861868165, which names implementing commit 79703c5 and the added regression coverage. Public source, owner-authored review, no third-party or customer material.", - "sanitization": "Rewritten from scratch against a fictional planner tool. Retains only the shape of the adjudicated exchange: a two-step create-then-transition sequence gains a retry and a fail-closed auto-close so a newly created record can never be left unexpectedly runnable. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "sanitization": "Rewritten from scratch against a fictional planner tool and a fictional tracker CLI. Retains only the shape of the adjudicated exchange: a two-step create-then-transition sequence gains a retry and a fail-closed auto-close so a newly created record can never be left unexpectedly runnable. The source's own product name, CLI name, and 'changeset'/'export' terminology were replaced with fictional equivalents ('task record', 'tracker', 'publish') rather than lightly renamed, because the originals are the real product's own identifiers. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", "recorded_at": "2026-07-27", "adjudication": { "first": "shaug/atelier PR 277 review comment 2861848880, accepted in reply 2861868165, which names implementing commit 79703c5 and the regression tests added for the fail-closed path.", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json index 635d12f..2fd5ee1 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/provenance/registry-client-layering.json @@ -3,7 +3,7 @@ "case_id": "registry-client-layering", "origin": "minimized_reproduction", "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 410, review comment 2870262209, authored by the repository owner and accepted in commit f631cb0, which converged the three client concepts onto one bound `BeadsClient`. Public source, owner-authored review, no third-party or customer material.", - "sanitization": "Rewritten from scratch against a fictional registry client. Retains only the failure shape: three client classes wrapping one transport, one of them already bound to the values the other two re-thread through every method. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", + "sanitization": "Rewritten from scratch against a fictional registry client. Retains only the failure shape: three client classes wrapping one transport, one of them already bound to the values the other two re-thread, generalized rather than modeled on the source's own class names or reviewer wording. No source identifier, path, symbol, prose, or diff was copied. No business logic, domain identifier, customer context, credential, or hidden reasoning.", "recorded_at": "2026-07-27", "adjudication": { "first": "shaug/atelier PR 410 review comment 2870262209, accepted in commit f631cb0: all three client classes were converged onto one bound BeadsClient instance.", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json index 30cf90f..5d28bfe 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json @@ -10,7 +10,7 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/registry/migration.py b/registry/migration.py\n--- a/registry/migration.py\n+++ b/registry/migration.py\n@@ -1740,1 +1740,12 @@ def _update_runtime_metadata_database(*, registry_root, database_name):\n existing = payload.get(\"database\")\n+ normalized_existing = _normalize_database_name(existing)\n+ local_candidates = _local_database_candidates(registry_root)\n+ if len(local_candidates) > 1:\n+ if normalized_existing == database_name:\n+ return DatabaseOutcome.UNCHANGED\n+ if normalized_existing is None:\n+ log.warning(\n+ \"Runtime DB reconciliation blocked: multiple local databases found \"\n+ f\"and no prior recorded database to disambiguate ({', '.join(local_candidates)}).\"\n+ )\n+ return DatabaseOutcome.BLOCKED_AMBIGUOUS\ndiff --git a/registry/startup.py b/registry/startup.py\n--- a/registry/startup.py\n+++ b/registry/startup.py\n@@ -30,5 +30,9 @@ def _reconcile_startup_database(registry_root, database_name):\n- updated = _update_runtime_metadata_database(\n+ outcome = _update_runtime_metadata_database(\n registry_root=registry_root, database_name=database_name,\n )\n- if not updated:\n- log.warning(\"Runtime DB reconciliation failed\")\n+ if outcome is DatabaseOutcome.BLOCKED_AMBIGUOUS:\n+ log.warning(\n+ \"Runtime DB reconciliation intentionally blocked: ambiguous local state.\"\n+ )\n+ elif outcome is DatabaseOutcome.FAILED:\n+ log.warning(\"Runtime DB reconciliation failed\")\ndiff --git a/tests/test_startup.py b/tests/test_startup.py\n--- a/tests/test_startup.py\n+++ b/tests/test_startup.py\n@@ -12,1 +12,8 @@ def test_reconcile_startup_database_logs_on_failure():\n assert \"reconciliation failed\" in caplog.text\n+\n+\n+def test_reconcile_startup_database_logs_intentional_block_not_generic_failure():\n+ with ambiguous_local_databases():\n+ reconcile_startup_database(ROOT, \"db-1\")\n+ assert \"intentionally blocked\" in caplog.text\n+ assert \"reconciliation failed\" not in caplog.text\n" + "content": "diff --git a/registry/migration.py b/registry/migration.py\n--- a/registry/migration.py\n+++ b/registry/migration.py\n@@ -1740,1 +1740,12 @@ def _apply_runtime_database_sync(*, registry_root, database_name):\n existing = payload.get(\"database\")\n+ normalized_existing = _canonical_database_label(existing)\n+ local_candidates = _discover_local_databases(registry_root)\n+ if len(local_candidates) > 1:\n+ if normalized_existing == database_name:\n+ return SyncOutcome.NO_CHANGE\n+ if normalized_existing is None:\n+ log.warning(\n+ \"Runtime DB reconciliation blocked: multiple local databases found \"\n+ f\"and no prior recorded database to disambiguate ({', '.join(local_candidates)}).\"\n+ )\n+ return SyncOutcome.AMBIGUOUS_HOLD\ndiff --git a/registry/startup.py b/registry/startup.py\n--- a/registry/startup.py\n+++ b/registry/startup.py\n@@ -30,5 +30,9 @@ def _reconcile_startup_sync(registry_root, database_name):\n- updated = _apply_runtime_database_sync(\n+ outcome = _apply_runtime_database_sync(\n registry_root=registry_root, database_name=database_name,\n )\n- if not updated:\n- log.warning(\"Runtime DB reconciliation failed\")\n+ if outcome is SyncOutcome.AMBIGUOUS_HOLD:\n+ log.warning(\n+ \"Runtime DB reconciliation intentionally blocked: ambiguous local state.\"\n+ )\n+ elif outcome is SyncOutcome.WRITE_FAILED:\n+ log.warning(\"Runtime DB reconciliation failed\")\ndiff --git a/tests/test_startup.py b/tests/test_startup.py\n--- a/tests/test_startup.py\n+++ b/tests/test_startup.py\n@@ -12,1 +12,8 @@ def test_reconcile_startup_sync_logs_on_failure():\n assert \"reconciliation failed\" in caplog.text\n+\n+\n+def test_reconcile_startup_sync_logs_intentional_block_not_generic_failure():\n+ with ambiguous_local_databases():\n+ reconcile_startup_database(ROOT, \"db-1\")\n+ assert \"intentionally blocked\" in caplog.text\n+ assert \"reconciliation failed\" not in caplog.text\n" } }, "change_contract": { diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json index df47fd6..bb0d8f7 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json @@ -10,22 +10,22 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/skills/create_changeset.py b/skills/create_changeset.py\n--- a/skills/create_changeset.py\n+++ b/skills/create_changeset.py\n@@ -85,5 +85,24 @@ def main() -> None:\n if not created.ok:\n print(\"error: failed to create changeset bead\", file=sys.stderr)\n raise SystemExit(1)\n \n- beads.run_bd_command([\"update\", created.issue_id, \"--status\", \"deferred\"])\n+ status_applied = _apply_deferred_status_with_retry(created.issue_id)\n+ if not status_applied:\n+ beads.run_bd_command([\"close\", created.issue_id, \"--reason\",\n+ \"deferred status could not be applied\"])\n+ print(\n+ f\"error: created {created.issue_id} but could not set deferred status; \"\n+ \"auto-closed to avoid an unexpectedly runnable changeset\",\n+ file=sys.stderr,\n+ )\n+ raise SystemExit(1)\n+\n+\n+def _apply_deferred_status_with_retry(issue_id, attempts=3):\n+ for _ in range(attempts):\n+ result = beads.run_bd_command(\n+ [\"update\", issue_id, \"--status\", \"deferred\"], allow_failure=True,\n+ )\n+ if result.ok:\n+ return True\n+ return False\ndiff --git a/tests/test_create_changeset.py b/tests/test_create_changeset.py\n--- a/tests/test_create_changeset.py\n+++ b/tests/test_create_changeset.py\n@@ -14,1 +14,17 @@ def test_main_sets_the_changeset_to_deferred():\n assert bd_calls[-1] == [\"update\", \"cs-1\", \"--status\", \"deferred\"]\n+\n+\n+def test_main_auto_closes_when_the_deferred_status_never_applies():\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert bd_calls[-1][0] == \"close\"\n+\n+\n+def test_main_does_not_export_after_the_auto_close_path(monkeypatch):\n+ exported = []\n+ monkeypatch.setattr(exporter, \"export\", lambda *a, **k: exported.append(a))\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert exported == []\n" + "content": "diff --git a/planner/create_task.py b/planner/create_task.py\n--- a/planner/create_task.py\n+++ b/planner/create_task.py\n@@ -85,5 +85,24 @@ def main() -> None:\n if not created.ok:\n print(\"error: failed to create task record\", file=sys.stderr)\n raise SystemExit(1)\n \n- tracker.run_cli_command([\"update\", created.record_id, \"--status\", \"deferred\"])\n+ status_applied = _apply_deferred_status_with_retry(created.record_id)\n+ if not status_applied:\n+ tracker.run_cli_command([\"close\", created.record_id, \"--reason\",\n+ \"deferred status could not be applied\"])\n+ print(\n+ f\"error: created {created.record_id} but could not set deferred status; \"\n+ \"auto-closed to avoid an unexpectedly runnable record\",\n+ file=sys.stderr,\n+ )\n+ raise SystemExit(1)\n+\n+\n+def _apply_deferred_status_with_retry(record_id, attempts=3):\n+ for _ in range(attempts):\n+ result = tracker.run_cli_command(\n+ [\"update\", record_id, \"--status\", \"deferred\"], allow_failure=True,\n+ )\n+ if result.ok:\n+ return True\n+ return False\ndiff --git a/tests/test_create_task.py b/tests/test_create_task.py\n--- a/tests/test_create_task.py\n+++ b/tests/test_create_task.py\n@@ -14,1 +14,17 @@ def test_main_sets_the_task_to_deferred():\n assert cli_calls[-1] == [\"update\", \"t-1\", \"--status\", \"deferred\"]\n+\n+\n+def test_main_auto_closes_when_the_deferred_status_never_applies():\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert cli_calls[-1][0] == \"close\"\n+\n+\n+def test_main_does_not_publish_after_the_auto_close_path(monkeypatch):\n+ published = []\n+ monkeypatch.setattr(publisher, \"publish\", lambda *a, **k: published.append(a))\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert published == []\n" } }, "change_contract": { - "goal": "Guarantee a newly created changeset never ends up unexpectedly runnable when the deferred-status transition fails, by retrying and then auto-closing.", + "goal": "Guarantee a newly created task record never ends up unexpectedly runnable when the deferred-status transition fails, by retrying and then auto-closing.", "acceptance_criteria": [ "A transient status-update failure is retried before giving up.", - "If deferred status still cannot be applied, the created changeset is auto-closed rather than left runnable.", - "The auto-close path does not export the changeset." + "If deferred status still cannot be applied, the created record is auto-closed rather than left runnable.", + "The auto-close path does not publish the record." ], "non_goals": [ - "Change how a changeset is created.", - "Change what makes a changeset runnable." + "Change how a task record is created.", + "Change what makes a task record runnable." ], "preserved_behaviors": [ - "The happy-path status transition still runs exactly one `bd` update call." + "The happy-path status transition still runs exactly one CLI update call." ] }, "sources": { @@ -38,25 +38,25 @@ ], "named_documents": [ { - "label": "Changeset creation contract", - "location": "docs/changeset-creation.md" + "label": "Task creation contract", + "location": "docs/task-creation.md" } ], "nearby_patterns": [ { - "label": "Sibling epic-creation script with the same protection", - "location": "skills/create_epic.py" + "label": "Sibling project-creation script with the same protection", + "location": "planner/create_project.py" }, { "label": "Creation tests", - "location": "tests/test_create_changeset.py" + "location": "tests/test_create_task.py" } ] }, "validation": [ { "name": "creation tests", - "command": "pytest tests/test_create_changeset.py", + "command": "pytest tests/test_create_task.py", "scope": "focused", "status": "passed", "result": "9 passed" @@ -71,12 +71,12 @@ ], "context": { "data": [ - "A changeset with no explicit status defaults to runnable in the underlying tracker.", + "A task record with no explicit status defaults to runnable in the underlying tracker.", "The status-update call can fail transiently (a brief tracker outage) as well as permanently.", - "The sibling epic-creation script already carries the identical retry-then-auto-close protection, added in the same release." + "The sibling project-creation script already carries the identical retry-then-auto-close protection, added in the same release." ], "operational": [ - "A runnable changeset is picked up by the scheduler within seconds of creation." + "A runnable task record is picked up by the scheduler within seconds of creation." ] } } diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json index 300288d..2898ba4 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json @@ -10,7 +10,7 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/registry/client.py b/registry/client.py\n--- a/registry/client.py\n+++ b/registry/client.py\n@@ -20,4 +20,36 @@ class Record:\n identifier: str\n status: str\n \n \n+class _RegistryRuntimeClient:\n+ \\\"\\\"\\\"Low-level transport bound to one registry root and working directory.\\\"\\\"\\\"\n+\n+ def __init__(self, registry_root, cwd):\n+ self._root = registry_root\n+ self._cwd = cwd\n+\n+ def _call(self, *args):\n+ return run_cli(*args, root=self._root, cwd=self._cwd)\n+\n+\n+class _RegistryRecordMutationsClient:\n+ \\\"\\\"\\\"Record create/update/close, threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def create(self, payload, *, registry_root, cwd):\n+ return run_cli(\"create\", payload, root=registry_root, cwd=cwd)\n+\n+ def update(self, identifier, payload, *, registry_root, cwd):\n+ return run_cli(\"update\", identifier, payload, root=registry_root, cwd=cwd)\n+\n+ def close(self, identifier, *, registry_root, cwd):\n+ return run_cli(\"close\", identifier, root=registry_root, cwd=cwd)\n+\n+\n+class _RegistryQueueMessagesClient:\n+ \\\"\\\"\\\"Queue message operations, also threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def enqueue(self, message, *, registry_root, cwd):\n+ return run_cli(\"enqueue\", message, root=registry_root, cwd=cwd)\n+\n+ def drain(self, *, registry_root, cwd):\n+ return run_cli(\"drain\", root=registry_root, cwd=cwd)\ndiff --git a/registry/worker.py b/registry/worker.py\n--- a/registry/worker.py\n+++ b/registry/worker.py\n@@ -8,8 +8,8 @@ from registry.client import (\n- RegistryQueueMessagesClient,\n- RegistryRecordMutationsClient,\n+ _RegistryQueueMessagesClient,\n+ _RegistryRecordMutationsClient,\n )\n \n \n def finalize(identifier, registry_root, cwd):\n- mutations = RegistryRecordMutationsClient()\n+ mutations = _RegistryRecordMutationsClient()\n mutations.close(identifier, registry_root=registry_root, cwd=cwd)\ndiff --git a/tests/test_worker.py b/tests/test_worker.py\n--- a/tests/test_worker.py\n+++ b/tests/test_worker.py\n@@ -10,2 +10,9 @@ def test_finalize_closes_the_record():\n finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n assert closed(\"r-1\")\n+\n+\n+def test_finalize_threads_root_and_cwd_to_the_mutation_client():\n+ calls = capture_run_cli_calls()\n+ finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n+ assert calls[-1].kwargs[\"root\"] == ROOT\n+ assert calls[-1].kwargs[\"cwd\"] == CWD\n" + "content": "diff --git a/registry/client.py b/registry/client.py\n--- a/registry/client.py\n+++ b/registry/client.py\n@@ -20,4 +20,36 @@ class Record:\n identifier: str\n status: str\n \n \n+class _TransportSession:\n+ \\\"\\\"\\\"Low-level transport bound to one registry root and working directory.\\\"\\\"\\\"\n+\n+ def __init__(self, registry_root, cwd):\n+ self._root = registry_root\n+ self._cwd = cwd\n+\n+ def _call(self, *args):\n+ return run_cli(*args, root=self._root, cwd=self._cwd)\n+\n+\n+class _RecordMutationOps:\n+ \\\"\\\"\\\"Record create/update/close, threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def create(self, payload, *, registry_root, cwd):\n+ return run_cli(\"create\", payload, root=registry_root, cwd=cwd)\n+\n+ def update(self, identifier, payload, *, registry_root, cwd):\n+ return run_cli(\"update\", identifier, payload, root=registry_root, cwd=cwd)\n+\n+ def close(self, identifier, *, registry_root, cwd):\n+ return run_cli(\"close\", identifier, root=registry_root, cwd=cwd)\n+\n+\n+class _QueueMessageOps:\n+ \\\"\\\"\\\"Queue message operations, also threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def enqueue(self, message, *, registry_root, cwd):\n+ return run_cli(\"enqueue\", message, root=registry_root, cwd=cwd)\n+\n+ def drain(self, *, registry_root, cwd):\n+ return run_cli(\"drain\", root=registry_root, cwd=cwd)\ndiff --git a/registry/worker.py b/registry/worker.py\n--- a/registry/worker.py\n+++ b/registry/worker.py\n@@ -8,8 +8,8 @@ from registry.client import (\n- QueueMessageOps,\n- RecordMutationOps,\n+ _QueueMessageOps,\n+ _RecordMutationOps,\n )\n \n \n def finalize(identifier, registry_root, cwd):\n- mutations = RecordMutationOps()\n+ mutations = _RecordMutationOps()\n mutations.close(identifier, registry_root=registry_root, cwd=cwd)\ndiff --git a/tests/test_worker.py b/tests/test_worker.py\n--- a/tests/test_worker.py\n+++ b/tests/test_worker.py\n@@ -10,2 +10,9 @@ def test_finalize_closes_the_record():\n finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n assert closed(\"r-1\")\n+\n+\n+def test_finalize_threads_root_and_cwd_to_the_mutation_client():\n+ calls = capture_run_cli_calls()\n+ finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n+ assert calls[-1].kwargs[\"root\"] == ROOT\n+ assert calls[-1].kwargs[\"cwd\"] == CWD\n" } }, "change_contract": { diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json index 7e6b114..e4a7f94 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json @@ -10,7 +10,7 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,8 +1,15 @@\n from paths import project_dir_for_enlistment\n \n \n+class SetupPathGateway:\n+ \"\"\"Indirection over `paths.project_dir_for_enlistment` for the setup service.\"\"\"\n+\n+ def project_dir(self, enlistment_id):\n+ return project_dir_for_enlistment(enlistment_id)\n+\n+\n class SetupDependencies:\n \"\"\"Dependency bag threaded into `SetupService`.\"\"\"\n \n def __init__(self, path_gateway):\n self.path_gateway = path_gateway\n@@ -18,6 +22,6 @@ class SetupService:\n def __init__(self, deps: SetupDependencies):\n self._deps = deps\n \n def run(self, enlistment_id):\n- target = self._deps.path_gateway.project_dir(enlistment_id)\n+ target = self._deps.path_gateway.project_dir(enlistment_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,10 +1,10 @@\n-from services.setup import SetupService\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService()\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(enlistment_id):\n service = _build_setup_service()\n return service.run(enlistment_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"e-1\": \"/enlistments/e-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+ assert gateway.calls == [\"e-1\"]\n" + "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,8 +1,15 @@\n from paths import resolve_workspace_root\n \n \n+class SetupPathGateway:\n+ \"\"\"Indirection over `paths.resolve_workspace_root` for the setup service.\"\"\"\n+\n+ def project_dir(self, enlistment_id):\n+ return resolve_workspace_root(enlistment_id)\n+\n+\n class SetupDependencies:\n \"\"\"Dependency bag threaded into `SetupService`.\"\"\"\n \n def __init__(self, path_gateway):\n self.path_gateway = path_gateway\n@@ -18,6 +22,6 @@ class SetupService:\n def __init__(self, deps: SetupDependencies):\n self._deps = deps\n \n def run(self, enlistment_id):\n- target = self._deps.path_gateway.project_dir(enlistment_id)\n+ target = self._deps.path_gateway.project_dir(enlistment_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,10 +1,10 @@\n-from services.setup import SetupService\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService()\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(enlistment_id):\n service = _build_setup_service()\n return service.run(enlistment_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"e-1\": \"/enlistments/e-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+ assert gateway.calls == [\"e-1\"]\n" } }, "change_contract": { @@ -70,9 +70,9 @@ ], "context": { "data": [ - "`paths.project_dir_for_enlistment` is a pure function: given an enlistment id, it returns a path with no I/O of its own.", - "There is exactly one production implementation of `project_dir_for_enlistment`, and no plan to add another.", - "`services/compose.py`, the only other service in this package, calls `paths.project_dir_for_enlistment` directly with no injected gateway and is tested by monkeypatching the module function." + "`paths.resolve_workspace_root` is a pure function: given an enlistment id, it returns a path with no I/O of its own.", + "There is exactly one production implementation of `resolve_workspace_root`, and no plan to add another.", + "`services/compose.py`, the only other service in this package, calls `paths.resolve_workspace_root` directly with no injected gateway and is tested by monkeypatching the module function." ], "operational": [ "This service runs once per `setup` invocation; it is not on a hot path." From 1ec231bb17cd0c1db82258756aa7a78e8e7f63ab Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 13:51:16 -0700 Subject: [PATCH 04/11] fix: make every s2 packet internally consistent after the sanitization rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes two of the three blocking findings from the second review cycle. The third finding was reviewed and determined not to be a defect; both outcomes are recorded below. ## Two real defects, both introduced by the previous fix commit The sanitization rewrite (`8e8a1fb`) renamed symbols inside each packet's diff but did not sweep the rename across the rest of the packet, and it never checked whether the diff it was editing was internally coherent to begin with. - `reconciliation-outcome-type`'s `context.data` still named `_update_runtime_metadata_database`, the pre-rename function, which the diff no longer defines after the rewrite renamed it to `_apply_runtime_database_sync`. Fixed by sweeping the rename into context as well. - `registry-client-layering`'s diff was never coherent, independent of naming: its `client.py` hunk introduced three classes as brand-new code, while its `worker.py` hunk imported two of them under public names that implied they already existed and were merely being renamed private. Rebuilt from scratch as a purely additive diff — one new class added next to two explicitly pre-existing, untouched classes named only in `context.data` — which is both internally consistent and the more faithful shape for the case: the escape is that the new class repeats a pattern two untouched siblings already established, not that three classes appear together for the first time. Both are recorded as limitation 22: a rename inside a diff must be swept across the whole packet, and a packet's diff must be checked for internal consistency independent of whatever prompted editing it. ## One finding reviewed and determined not to be a defect The same cycle flagged the real source class name `BeadsClient` appearing in `registry-client-layering`'s `retention_authority` and `adjudication.first` fields. Those two fields are private, structurally separated from every reviewer-visible artifact, and every provenance record in this corpus already cites real PR numbers, comment ids, and commit SHAs for exactly the same reason: that is what retention authority and adjudication provenance are for. The sanitization rule governs what reaches a reviewer-visible artifact or a grader formulation, never a private record's own description of its real source. Recorded as limitation 23 so the distinction is explicit rather than re-litigated on a future case. ## Validation Every packet diff across all six corpora re-verified to parse as a patch (21 of 21), and every context reference in the four affected cases checked against its own diff. --- CHANGELOG.md | 2 + review-suite/evals/baseline/v1/LIMITATIONS.md | 45 +++++++++++++++++++ .../registry-client-layering.json | 4 +- .../reconciliation-outcome-type/packet.json | 2 +- .../registry-client-layering/packet.json | 30 ++++++------- 5 files changed, 65 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0186219..14392f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity stratum, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: make every s2 packet internally consistent after the sanitization rename - fix: sanitize the solution-simplicity cases against source-vocabulary leakage + (`8e8a1fbf4efd7d97d6573c97b9257ed92bebe055`) - feat: populate the solution-simplicity stratum with four adjudicated cases (`df55335d2df85f0ba3064e671095d0621eeb145e`) - feat: require acceptance evidence for workflow closeout diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index ba18b3d..9569d5d 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -501,3 +501,48 @@ no way to know what the source's vocabulary was. Curation discipline is the only defense until one exists: name every symbol as if writing original code for the fictional subject, and never carry a reviewer's sentence forward as a formulation without independently rephrasing it. + +## 22. A minimization rewrite must update every field that names the changed symbols, not only the diff + +The second review cycle on this batch found that the first sanitization fix +(item 21) renamed symbols inside the diff but left two packets' `context.data` +naming the pre-rename symbol — a function or class the diff no longer defines. +That is a narrower defect than a leak: it makes the packet internally +self-contradictory, independent of whether either name is real or fictional. A +reviewer reading `context.data` would be told to look for a symbol its own diff +had already renamed away. + +The same cycle also found a packet whose diff had never been consistent in the +first place: it introduced three classes as brand-new code while a downstream +file's hunk implied two of them already existed under different names. That +predates the sanitization fix; renaming inside an already-incoherent diff cannot +make it coherent. + +Both are now fixed: `registry-client-layering`'s case was rebuilt as a purely +additive diff — one new class next to two explicitly pre-existing, untouched +ones named only in context — and every context reference across the stratum was +checked against its own diff. The general lesson: **a rename or a fix inside a +diff must be swept across the whole packet, and a packet's diff must be checked +for internal consistency independently of whatever sanitization or grading +concern prompted editing it.** Neither check is mechanical today; both are +curation discipline until a tool exists to enforce them. + +## 23. Naming the real source in private provenance is retention, not a leak + +The same review cycle raised the real source's class name (`BeadsClient`) +appearing in a case's `retention_authority` and `adjudication.first` fields as a +possible sanitization defect. It is not: those two fields are private, +structurally separated from every reviewer-visible artifact, and their entire +purpose is to record *what the real source actually was* — the PR, the comment, +the accepted commit, and, where useful for a future audit, what that commit's +outcome was named. Every provenance record in this corpus already cites real PR +numbers, comment ids, and commit SHAs for exactly this reason, and this case is +consistent with that established pattern rather than an exception to it. + +The sanitization rule in items 16 and 21 governs what reaches a +**reviewer-visible artifact or a grader formulation a reviewer's payload could +echo** — the packet, the equivalent formulations, anything `audit_corpus.py` can +reach. It was never a rule against a private, human-facing provenance record +describing its own real source, and applying it there would make provenance +unable to do the one job it exists for: letting a later reader verify where a +case actually came from. diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json index 00881ee..ca98f85 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/registry-client-layering.json @@ -8,9 +8,9 @@ { "id": "rc.three-client-concepts-duplicate-binding", "requirement": "When one caller already threads a root and working directory through every method of a client, a second client for a different operation family should converge on the client already bound to those values.", - "trigger": "Adding a client for a new operation family next to two existing ones that all wrap the same transport.", + "trigger": "Adding a client for a new operation family next to an existing one that already re-threads root and working directory, in a module that also already has a transport session bound to those exact values.", "surface": "_QueueMessageOps", - "consequence": "A third client class is added that re-threads `registry_root` and `cwd` through every method exactly as the second one does, even though the first client in the same file is already bound to both values at construction and exists specifically to avoid that repetition. Every caller of any of the three has to supply the same two values again at every call site, and a fourth operation family would repeat the pattern a third time.", + "consequence": "A second client class is added that re-threads `registry_root` and `cwd` through every method exactly as the existing `_RecordMutationOps` already does, even though `_TransportSession` in the same module is already bound to both values at construction and exists specifically to avoid that repetition. Every caller of either operation client has to supply the same two values again at every call site, and this diff repeats rather than corrects the pattern.", "severity": "blocking", "equivalent_formulations": [ "a third wrapper repeats the binding the transport session already holds", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json index 5d28bfe..3773bbb 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/reconciliation-outcome-type/packet.json @@ -70,7 +70,7 @@ ], "context": { "data": [ - "Before this change, `_update_runtime_metadata_database` returned a plain `bool`, and `True` covered both a successful reconciliation and an intentional preserve-ambiguous-state no-op.", + "Before this change, `_apply_runtime_database_sync` returned a plain `bool`, and `True` covered both a successful reconciliation and an intentional preserve-ambiguous-state no-op.", "The startup caller only had the boolean to decide what to log, so an intentional block and a real failure were indistinguishable to an operator reading the log." ], "operational": [ diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json index 2898ba4..c8d47f8 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json @@ -10,21 +10,21 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/registry/client.py b/registry/client.py\n--- a/registry/client.py\n+++ b/registry/client.py\n@@ -20,4 +20,36 @@ class Record:\n identifier: str\n status: str\n \n \n+class _TransportSession:\n+ \\\"\\\"\\\"Low-level transport bound to one registry root and working directory.\\\"\\\"\\\"\n+\n+ def __init__(self, registry_root, cwd):\n+ self._root = registry_root\n+ self._cwd = cwd\n+\n+ def _call(self, *args):\n+ return run_cli(*args, root=self._root, cwd=self._cwd)\n+\n+\n+class _RecordMutationOps:\n+ \\\"\\\"\\\"Record create/update/close, threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def create(self, payload, *, registry_root, cwd):\n+ return run_cli(\"create\", payload, root=registry_root, cwd=cwd)\n+\n+ def update(self, identifier, payload, *, registry_root, cwd):\n+ return run_cli(\"update\", identifier, payload, root=registry_root, cwd=cwd)\n+\n+ def close(self, identifier, *, registry_root, cwd):\n+ return run_cli(\"close\", identifier, root=registry_root, cwd=cwd)\n+\n+\n+class _QueueMessageOps:\n+ \\\"\\\"\\\"Queue message operations, also threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def enqueue(self, message, *, registry_root, cwd):\n+ return run_cli(\"enqueue\", message, root=registry_root, cwd=cwd)\n+\n+ def drain(self, *, registry_root, cwd):\n+ return run_cli(\"drain\", root=registry_root, cwd=cwd)\ndiff --git a/registry/worker.py b/registry/worker.py\n--- a/registry/worker.py\n+++ b/registry/worker.py\n@@ -8,8 +8,8 @@ from registry.client import (\n- QueueMessageOps,\n- RecordMutationOps,\n+ _QueueMessageOps,\n+ _RecordMutationOps,\n )\n \n \n def finalize(identifier, registry_root, cwd):\n- mutations = RecordMutationOps()\n+ mutations = _RecordMutationOps()\n mutations.close(identifier, registry_root=registry_root, cwd=cwd)\ndiff --git a/tests/test_worker.py b/tests/test_worker.py\n--- a/tests/test_worker.py\n+++ b/tests/test_worker.py\n@@ -10,2 +10,9 @@ def test_finalize_closes_the_record():\n finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n assert closed(\"r-1\")\n+\n+\n+def test_finalize_threads_root_and_cwd_to_the_mutation_client():\n+ calls = capture_run_cli_calls()\n+ finalize(\"r-1\", registry_root=ROOT, cwd=CWD)\n+ assert calls[-1].kwargs[\"root\"] == ROOT\n+ assert calls[-1].kwargs[\"cwd\"] == CWD\n" + "content": "diff --git a/registry/client.py b/registry/client.py\n--- a/registry/client.py\n+++ b/registry/client.py\n@@ -20,4 +20,12 @@ class Record:\n identifier: str\n status: str\n \n \n+class _QueueMessageOps:\n+ \\\"\\\"\\\"Queue message operations, threading root and cwd through every call.\\\"\\\"\\\"\n+\n+ def enqueue(self, message, *, registry_root, cwd):\n+ return run_cli(\"enqueue\", message, root=registry_root, cwd=cwd)\n+\n+ def drain(self, *, registry_root, cwd):\n+ return run_cli(\"drain\", root=registry_root, cwd=cwd)\ndiff --git a/tests/test_client.py b/tests/test_client.py\n--- a/tests/test_client.py\n+++ b/tests/test_client.py\n@@ -10,2 +10,9 @@ def test_record_mutation_ops_close_calls_run_cli():\n ops.close(\"r-1\", registry_root=ROOT, cwd=CWD)\n assert last_run_cli_call().kwargs[\"root\"] == ROOT\n+\n+\n+def test_queue_message_ops_enqueue_threads_root_and_cwd():\n+ ops = _QueueMessageOps()\n+ ops.enqueue({\"kind\": \"sync\"}, registry_root=ROOT, cwd=CWD)\n+ assert last_run_cli_call().kwargs[\"root\"] == ROOT\n+ assert last_run_cli_call().kwargs[\"cwd\"] == CWD\n" } }, "change_contract": { - "goal": "Move a queue-message client onto typed records, following the same pattern the mutation client already uses.", + "goal": "Add queue message operations (enqueue, drain) to the registry client.", "acceptance_criteria": [ - "Queue message operations carry typed records rather than raw dicts.", - "The runtime client's transport call is unaffected." + "Queue messages can be enqueued and drained through the registry client.", + "The new operations reach the registry through the existing transport." ], "non_goals": [ - "Converge the three client concepts onto one bound client.", + "Change record mutation behaviour.", "Change the CLI transport itself." ], "preserved_behaviors": [ - "`finalize` keeps its existing signature and behaviour." + "`_RecordMutationOps` and `_TransportSession` are unchanged." ] }, "sources": { @@ -43,16 +43,16 @@ ], "nearby_patterns": [ { - "label": "Runtime transport client", - "location": "registry/client.py" + "label": "Existing bound transport session", + "location": "registry/client.py:_TransportSession" }, { - "label": "Worker using the mutation client", - "location": "registry/worker.py" + "label": "Existing record mutation client", + "location": "registry/client.py:_RecordMutationOps" }, { - "label": "Worker tests", - "location": "tests/test_worker.py" + "label": "Client tests", + "location": "tests/test_client.py" } ] }, @@ -74,9 +74,9 @@ ], "context": { "data": [ - "`_RegistryRuntimeClient` is already bound to `registry_root` and `cwd` at construction and calls `run_cli` through `self._call`.", - "`_RegistryRecordMutationsClient` and `_RegistryQueueMessagesClient` both take `registry_root` and `cwd` as parameters on every method instead of using the already-bound runtime client.", - "All three classes call the same underlying `run_cli` transport function." + "`registry/client.py` already defines `_TransportSession`, bound to `registry_root` and `cwd` at construction, calling `run_cli` through `self._call`. This diff does not touch it.", + "`registry/client.py` also already defines `_RecordMutationOps`, whose `create`/`update`/`close` methods each take `registry_root` and `cwd` as parameters rather than using the already-bound transport session. This diff does not touch it either.", + "All client classes in this module call the same underlying `run_cli` transport function." ], "operational": [ "Every runtime module that calls the registry constructs its own root/cwd pair from the same two configuration values." From ab3921a904ec7835bc4d03ed40b7c8a28d12d2c1 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 14:13:57 -0700 Subject: [PATCH 05/11] fix: sweep sanitization across every reviewer-visible field, not only the diff Closes the two blocking findings from the third and final scheduled review cycle. ## Two more leak sites the earlier sanitization pass missed The earlier fixes swept the diff and the grader's `equivalent_formulations`. Two other reviewer-visible fields still carried real source material: - `sources.repository_instructions[].summary` in two packets reused the real reviewer's own sentences almost verbatim ("abstract away the calls it needs to make", "deferred-by-default semantics"). Both rewritten as independent paraphrases. - `setup-service-path-gateway`'s `change_contract` kept the real source's domain noun (`enlistment`) across its acceptance criteria, non-goals, and preserved behaviours, even though its diff had already been renamed to `workspace_id`. All three fields corrected to match. ## A no-op diff line that made the case's before-state impossible The same packet's diff also carried an identical `-`/`+` line pair, present since the very first draft rather than introduced by any sanitization commit: the pre-image called a zero-argument constructor while the same hunk's next line already referenced a dependency that constructor could not have held. Rebuilt so the constructor change and the dependency reference are introduced together and the before-state is actually reachable. ## Recorded Limitation 24: a minimization or a rename must be swept across every prose field in a packet, not only the diff and whichever field the grader reads. It took three independent review cycles on one four-case stratum to find every instance of this, which is itself the evidence that curation discipline alone is not a durable defense. ## Validation Every s2 packet re-scanned for the full set of real phrases and terms found across all three cycles; none remain. All packet diffs re-verified to parse as patches. --- CHANGELOG.md | 2 ++ review-suite/evals/baseline/v1/LIMITATIONS.md | 30 +++++++++++++++++++ .../setup-service-path-gateway.json | 2 +- .../packet.json | 2 +- .../setup-service-path-gateway/packet.json | 14 ++++----- 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14392f2..55728c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity stratum, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: sweep sanitization across every reviewer-visible field, not only the diff - fix: make every s2 packet internally consistent after the sanitization rename + (`5cb339d879aae87946b077fedb53fc3bab013dfd`) - fix: sanitize the solution-simplicity cases against source-vocabulary leakage (`8e8a1fbf4efd7d97d6573c97b9257ed92bebe055`) - feat: populate the solution-simplicity stratum with four adjudicated cases diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 9569d5d..060eaaa 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -546,3 +546,33 @@ reach. It was never a rule against a private, human-facing provenance record describing its own real source, and applying it there would make provenance unable to do the one job it exists for: letting a later reader verify where a case actually came from. + +## 24. Sanitization must sweep every reviewer-visible field, not only the diff and its formulations + +The third review cycle on this batch found the sanitization fix (items 21-23) +had covered the diff and the grader's `equivalent_formulations`, and still +missed two other places the same real prose and real domain nouns reached a +reviewer-visible packet: `sources.repository_instructions[].summary` reused the +real reviewer's own phrasing ("abstract away the calls it needs to make", +"deferred-by-default semantics") almost verbatim, and one packet's +`change_contract` kept the real source's own domain noun (`enlistment`) in three +fields the earlier pass never touched. + +The same cycle also found a fourth packet (`setup-service-path-gateway`) carried +a no-op diff line — an identical `-`/`+` pair — that made its before-state +impossible: the pre-image called a zero-argument constructor while the very next +line inside the same hunk already referenced a dependency that constructor could +not have had. This defect predates every sanitization commit; it was present in +the very first draft and simply went unnoticed until a cycle checked the diff's +internal coherence rather than its wording. + +Both are now fixed. The lesson generalizes past this specific batch: **a +minimization or a rename must be swept across the whole packet - goal, +acceptance criteria, non-goals, preserved behaviors, repository instructions, +named documents, nearby patterns, and context - not only the diff and the +formulations that happen to be the field a grader reads.** A packet has many +prose fields, and a real term or a real reviewer's sentence can hide in any of +them. Nothing mechanical catches this today; it took three independent review +passes on one four-case stratum to find every instance, which is itself evidence +that curation discipline alone is not a durable defense and a future population +batch should expect the same scrutiny. diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json index 150fba8..e4c01c5 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/setup-service-path-gateway.json @@ -8,7 +8,7 @@ { "id": "rc.gateway-abstracts-a-pure-function-with-one-implementation", "requirement": "The service tier should call the concrete function it needs directly unless a demonstrated reason requires indirection.", - "trigger": "Reading `SetupPathGateway` and `SetupDependencies` next to the function they wrap.", + "trigger": "Reading `SetupPathGateway`, `SetupDependencies`, and the changed constructor next to the function they wrap.", "surface": "SetupPathGateway", "consequence": "A gateway class, a dependency-bag class, and a constructor wiring both are introduced to indirect a pure function that has one production implementation and no I/O to isolate. The sibling service in the same package calls the same kind of function directly and is tested by monkeypatching, so the abstraction buys nothing the existing pattern does not already provide and makes this service's construction path materially harder to follow than its neighbour's.", "severity": "blocking", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json index bb0d8f7..9e555cf 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json @@ -33,7 +33,7 @@ { "label": "Deferred-by-default rules", "location": "AGENTS.md", - "summary": "A creation script that relies on deferred-by-default semantics must not leave a newly created record in its default runnable state when the deferred transition fails. Fail closed: retry, and if the transition still cannot be applied, remove the record from the runnable pool entirely." + "summary": "A creation script that leaves a new record in its runnable-by-default state must not exit before an intended non-runnable status actually takes effect. Fail closed: retry, and if the status still cannot be applied, remove the record from the runnable pool entirely rather than leaving it there." } ], "named_documents": [ diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json index e4a7f94..3cfee7c 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json @@ -10,21 +10,21 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,8 +1,15 @@\n from paths import resolve_workspace_root\n \n \n+class SetupPathGateway:\n+ \"\"\"Indirection over `paths.resolve_workspace_root` for the setup service.\"\"\"\n+\n+ def project_dir(self, enlistment_id):\n+ return resolve_workspace_root(enlistment_id)\n+\n+\n class SetupDependencies:\n \"\"\"Dependency bag threaded into `SetupService`.\"\"\"\n \n def __init__(self, path_gateway):\n self.path_gateway = path_gateway\n@@ -18,6 +22,6 @@ class SetupService:\n def __init__(self, deps: SetupDependencies):\n self._deps = deps\n \n def run(self, enlistment_id):\n- target = self._deps.path_gateway.project_dir(enlistment_id)\n+ target = self._deps.path_gateway.project_dir(enlistment_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,10 +1,10 @@\n-from services.setup import SetupService\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService()\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(enlistment_id):\n service = _build_setup_service()\n return service.run(enlistment_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"e-1\": \"/enlistments/e-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"e-1\") == prepared(\"/enlistments/e-1\")\n+ assert gateway.calls == [\"e-1\"]\n" + "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,10 +1,24 @@\n from paths import resolve_workspace_root\n \n \n+class SetupPathGateway:\n+ \\\"\\\"\\\"Indirection over `paths.resolve_workspace_root` for the setup service.\\\"\\\"\\\"\n+\n+ def project_dir(self, workspace_id):\n+ return resolve_workspace_root(workspace_id)\n+\n+\n+class SetupDependencies:\n+ \\\"\\\"\\\"Dependency bag threaded into `SetupService`.\\\"\\\"\\\"\n+\n+ def __init__(self, path_gateway):\n+ self.path_gateway = path_gateway\n+\n+\n class SetupService:\n- def __init__(self, path_gateway):\n- self._path_gateway = path_gateway\n+ def __init__(self, deps: SetupDependencies):\n+ self._deps = deps\n \n- def run(self, workspace_id):\n- target = self._path_gateway.project_dir(workspace_id)\n+ def run(self, workspace_id):\n+ target = self._deps.path_gateway.project_dir(workspace_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,11 +1,10 @@\n-from services.setup import SetupService\n-from paths import resolve_workspace_root\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService(resolve_workspace_root)\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(workspace_id):\n service = _build_setup_service()\n return service.run(workspace_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"w-1\": \"/workspaces/w-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+ assert gateway.calls == [\"w-1\"]\n" } }, "change_contract": { - "goal": "Let `SetupService` resolve an enlistment's project directory so `run` no longer hardcodes a stub target.", + "goal": "Let `SetupService` resolve a workspace's project directory through an injected dependency rather than a plain function reference.", "acceptance_criteria": [ - "`SetupService.run` resolves the real project directory for the given enlistment.", + "`SetupService.run` resolves the real project directory for the given workspace.", "The resolution is testable without touching the filesystem." ], "non_goals": [ "Change what `_prepare` does with the resolved target.", - "Change how an enlistment id maps to a directory." + "Change how a workspace id maps to a directory." ], "preserved_behaviors": [ - "`SetupService`'s public `run(enlistment_id)` signature is unchanged." + "`SetupService`'s public `run(workspace_id)` signature is unchanged." ] }, "sources": { @@ -32,7 +32,7 @@ { "label": "Service tier rules", "location": "AGENTS.md", - "summary": "The service tier exists to capture repeatable business logic, not to abstract away the concrete calls it needs to make to do its job. A service should call the function it needs directly unless there is a demonstrated reason - more than one real implementation, or a boundary the service must not cross - to indirect it." + "summary": "The service tier holds repeatable business rules, not a wrapper around calls it could make on its own. A service should reach for the concrete function it needs unless there is a demonstrated reason - more than one real implementation, or a boundary the service must not cross - to route through an interface first." } ], "named_documents": [ @@ -70,7 +70,7 @@ ], "context": { "data": [ - "`paths.resolve_workspace_root` is a pure function: given an enlistment id, it returns a path with no I/O of its own.", + "`paths.resolve_workspace_root` is a pure function: given a workspace id, it returns a path with no I/O of its own.", "There is exactly one production implementation of `resolve_workspace_root`, and no plan to add another.", "`services/compose.py`, the only other service in this package, calls `paths.resolve_workspace_root` directly with no injected gateway and is tested by monkeypatching the module function." ], From f3c064a7bbaf3f89f7a6a5495846b254a54e9a0b Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 14:28:05 -0700 Subject: [PATCH 06/11] feat: populate the code-simplicity stratum with four adjudicated cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batch 4 of #58's corpus population, folded into this same branch and PR per the coordinator's process change: both remaining strata (no executable oracle for either subject) ship together as one delivery rather than two full review/CI/merge cycles. Populates `s3-code-simplicity-lens` with the four case classes #58 requires, minimized from adjudicated review disposition in the public `shaug/atelier` and this repository's own delivery history, applying every sanitization lesson learned the hard way while populating `s2` in this same branch. ## The four cases Two local code-complexity or reuse escapes: - `watcher-check-policy-duplication` — a shared failed-check policy predicate has one remaining inline copy at a third call site, after a real history of the same duplication class causing two prior incidents where one copy drifted from another and produced a self-contradictory result. This repository's own commit `9351619`, where the code-simplicity lens under evaluation itself found and this fix resolved the duplication. - `metrics-label-formatting-duplication` — two new exporter functions each re-derive a label-formatting expression inline instead of calling the shared helper that already produces it, compounding a pre-existing copy rather than fixing it. atelier PR 410, comment 2870262209, accepted in `f631cb0`, read at the local-implementation level. Two near-miss controls, both adjudicated-rejected per the owner's settled standard: - `compat-accessor-boundary-duplication` — two compatibility accessor functions with identical bodies are kept apart because they serve two independently scheduled, untouched callers at the true migration boundary. atelier PR 630, accepted resolution of the concern raised in comment 2906745973. - `env-inventory-bullet-format` — per-item bullet blocks are chosen over a compact table because the table previously produced malformed rows under the repository's line-wrap formatter. atelier PR 443, accepted resolution of an earlier formatting concern. ## Adjudication: no oracle exists for this subject either Same reason as `s2`: "this is a duplication defect" and "this apparent duplication is justified" are design judgements, not properties a program can check. Every case records `adjudication.second: owner_required`, each with a concrete recommended disposition and residual risk, enforced by the same test that already holds `s2` to this contract. ## One source reused a third time, deliberately `atelier` PR 410 comment 2870262209 now grounds an escape in both `s2` (`registry-client-layering`, the whole-solution reading) and `s3` (`metrics-label-formatting-duplication`, the local-implementation reading of the same comment). Built against deliberately different fictional subjects — a registry client and a metrics exporter — so the two cases do not read as one scenario duplicated across strata. Recorded plainly rather than left for a reader to notice and wonder about. ## Sanitization applied from the start, not discovered after the fact `s2` needed three review cycles to find every leak class: real product terminology in the diff, real symbol names, verbatim reviewer phrasing in `equivalent_formulations`, and the same phrasing in `sources.repository_instructions[].summary`, plus a domain noun left in `change_contract` fields the earlier passes never touched. Every one of those classes was checked against all four `s3` cases before this record was written: no real product term, no real symbol, no verbatim reviewer sentence, in any reviewer-visible field. The one case sourced from this repository's own history is fictionalized against a `watch-tools` subject specifically so it cannot be recognised as this suite's own PR-watching code. ## Corpus minima now met across all three declared strata `s1-correctness-orchestrator`, `s2-solution-simplicity-lens`, and `s3-code-simplicity-lens` are all `populated_not_scored`. No stratum remains `declared_unpopulated`. That is a necessary condition for a scored baseline, not a sufficient one: independent adjudication for 8 cases across `s2` and `s3` needs the owner directly, and the frozen-baseline freeze itself remains outstanding regardless of corpus completeness. --- .../s3-code-simplicity-lens/corpus.json | 20 +++++ .../compat-accessor-boundary-duplication.json | 28 ++++++ .../env-inventory-bullet-format.json | 29 +++++++ .../metrics-label-formatting-duplication.json | 34 ++++++++ .../watcher-check-policy-duplication.json | 34 ++++++++ .../compat-accessor-boundary-duplication.json | 13 +++ .../env-inventory-bullet-format.json | 13 +++ .../metrics-label-formatting-duplication.json | 13 +++ .../watcher-check-policy-duplication.json | 13 +++ .../reviewer/PROMPT.md | 7 ++ .../packet.json | 86 +++++++++++++++++++ .../env-inventory-bullet-format/packet.json | 76 ++++++++++++++++ .../packet.json | 84 ++++++++++++++++++ .../packet.json | 86 +++++++++++++++++++ 14 files changed, 536 insertions(+) create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/corpus.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/compat-accessor-boundary-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/env-inventory-bullet-format.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/metrics-label-formatting-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/compat-accessor-boundary-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/env-inventory-bullet-format.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/metrics-label-formatting-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/reviewer/PROMPT.md create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/reviewer/compat-accessor-boundary-duplication/packet.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/reviewer/env-inventory-bullet-format/packet.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/reviewer/metrics-label-formatting-duplication/packet.json create mode 100644 review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json b/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json new file mode 100644 index 0000000..7f018c4 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json @@ -0,0 +1,20 @@ +{ + "corpus_version": "0.1-s3-populated", + "protocol_version": "1.0", + "grader_version": "1.0", + "target_skill": "review-code-simplicity", + "target_skill_dependencies": [], + "stratum": { + "id": "s3-code-simplicity-lens", + "ground_truth": "human-review", + "scored": false, + "grading_is_signal": true, + "purpose": "Local code-complexity, reuse, and DRY cases for the self-sufficient code-simplicity lens. Populated and sourced from adjudicated human review and one adjudicated repository- history finding, with the recorded disposition as the first adjudication. NO executable oracle exists for this subject: 'this is a duplication defect' and 'this apparent duplication is justified' are design judgements a runnable check cannot decide. Every case therefore records adjudication.second: owner_required and a concrete recommended disposition for the owner to confirm or overturn. NOT scored: every expectation is uncalibrated and no case has been run through any runtime, per the owner-settled three-way grading method - a scored case is never calibrated on its own prose, and a grader miss must surface as referred rather than a silent reviewer-miss." + }, + "cases": [ + "compat-accessor-boundary-duplication", + "env-inventory-bullet-format", + "metrics-label-formatting-duplication", + "watcher-check-policy-duplication" + ] +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/compat-accessor-boundary-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/compat-accessor-boundary-duplication.json new file mode 100644 index 0000000..03789ff --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/compat-accessor-boundary-duplication.json @@ -0,0 +1,28 @@ +{ + "expectation_version": "1.0", + "case_id": "compat-accessor-boundary-duplication", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "clean", + "material_root_causes": [], + "accepted_non_findings": [ + { + "id": "anf.two-identical-conversion-functions", + "description": "A reviewer may observe that `record_value` and `record_payload` have identical bodies and ask why they are not one function. This is the finding this case is built from, and it was adjudicated NOT material: the packet states the two callers are on independent migration schedules and are tracked separately by the migration plan, so merging the accessors would couple two release schedules that are deliberately kept apart. Tolerated as an observation; gating on it is a false alarm.", + "equivalent_formulations": [ + "these two functions have identical bodies", + "why are there two conversion functions instead of one", + "record_value and record_payload duplicate each other", + "this compatibility accessor is duplicated at the migration edge" + ] + }, + { + "id": "anf.bare-except-message-formatting", + "description": "The typed-path error branch formats `detail` from `str(exc)` without the whitespace normalisation the old branch had. Worth noting; the packet's preserved behaviour is the return shape, not the log detail text.", + "equivalent_formulations": [ + "the error detail formatting changed slightly", + "whitespace normalisation was dropped from the error path" + ] + } + ] +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/env-inventory-bullet-format.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/env-inventory-bullet-format.json new file mode 100644 index 0000000..ffda710 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/env-inventory-bullet-format.json @@ -0,0 +1,29 @@ +{ + "expectation_version": "1.0", + "case_id": "env-inventory-bullet-format", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "clean", + "material_root_causes": [], + "accepted_non_findings": [ + { + "id": "anf.bullet-blocks-instead-of-a-table", + "description": "A reviewer may observe that documenting each variable as its own block is far more verbose than a compact table would be for the same three facts. This is the finding this case is built from, and it was adjudicated NOT material: the packet states that a table with a long consumers cell previously produced malformed rows under the repository's line-wrap formatter, so the per-item form is required for formatter stability and diff-friendliness rather than a stylistic preference. Tolerated as an observation; gating on it is a false alarm.", + "equivalent_formulations": [ + "this is far more verbose than a table would be", + "bullet blocks repeat the same three fields three times", + "a compact table would say the same thing in less space", + "this looks like unnecessary verbosity for three facts", + "one entry per variable instead of a compact row" + ] + }, + { + "id": "anf.repeated-owner-field-value", + "description": "Every entry repeats `owner: scheduler` verbatim. Worth noting as a candidate for a shared preamble; the packet's acceptance criteria require every variable to state its own owner explicitly rather than inherit one from context.", + "equivalent_formulations": [ + "the owner field is the same value every time", + "this could state the owner once instead of per entry" + ] + } + ] +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/metrics-label-formatting-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/metrics-label-formatting-duplication.json new file mode 100644 index 0000000..306aa4c --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/metrics-label-formatting-duplication.json @@ -0,0 +1,34 @@ +{ + "expectation_version": "1.0", + "case_id": "metrics-label-formatting-duplication", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.label-expression-copied-into-two-new-functions", + "requirement": "When a new function needs formatting behaviour a shared helper already implements, call the helper rather than re-deriving the same expression inline.", + "trigger": "Adding a second and third exporter next to an existing one that already has the same duplication.", + "surface": "export_latency", + "consequence": "A helper that already produces exactly this label string exists in the same module, and the diff adds two more inline copies of its expression rather than calling it - compounding a duplication that already existed in `export_error_rate` rather than resolving it. A change to the label format now has to be made in three functions instead of one, and the packet states a fourth exporter is already planned.", + "severity": "blocking", + "equivalent_formulations": [ + "this re-derives the label expression instead of calling the shared formatter", + "the label format is now duplicated in three places", + "call format_sample_point instead of repeating its expression", + "each new exporter copies the same formatting logic inline", + "this compounds an existing duplication rather than fixing it" + ] + } + ], + "accepted_non_findings": [ + { + "id": "anf.existing-error-rate-duplication-untouched", + "description": "`export_error_rate`'s own inline copy predates this diff and is not touched by it. A reviewer may note that fixing it too would be an improvement; the packet's non-goals do not require this diff to correct pre-existing code it does not otherwise touch.", + "equivalent_formulations": [ + "the existing error rate exporter still has the same duplication", + "this diff does not fix the pre-existing copy" + ] + } + ] +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json new file mode 100644 index 0000000..cabb785 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json @@ -0,0 +1,34 @@ +{ + "expectation_version": "1.0", + "case_id": "watcher-check-policy-duplication", + "packet_valid": true, + "calibrated": false, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.local-policy-duplicated-across-call-sites", + "requirement": "A policy predicate with more than one call site must have exactly one implementation.", + "trigger": "A remaining inline copy of a policy that a shared predicate now implements.", + "surface": "candidate_is_clear", + "consequence": "The packet's own diff replaces the inline copy with the shared predicate, which is the correction this case demonstrates: before this change, three call sites decided the same failed-check question with two different implementations, and the packet's own history states that exact drift previously produced a self-contradictory clear/not-clear result on the same candidate. Reviewing this diff for local complexity means recognising that the change is the fix, and that the requirement is precisely what motivated it.", + "severity": "strong_recommendation", + "equivalent_formulations": [ + "the inline copy duplicates the shared predicate", + "this call site had its own copy of the failed-check policy", + "the same policy is implemented twice", + "route this through the existing shared predicate", + "this duplication previously caused drift between call sites" + ] + } + ], + "accepted_non_findings": [ + { + "id": "anf.pending-check-inline", + "description": "The pending-check condition (`checks_summary[\"pending_count\"] > 0`) stays inline rather than moving into the shared predicate. A reviewer may ask why it was not folded in too; the packet's non-goals state that changing the pending condition is out of scope, and it is not part of the duplication the shared predicate was built to resolve.", + "equivalent_formulations": [ + "the pending check is still inline", + "why wasn't the pending condition also shared" + ] + } + ] +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/compat-accessor-boundary-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/compat-accessor-boundary-duplication.json new file mode 100644 index 0000000..0412bfc --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/compat-accessor-boundary-duplication.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "compat-accessor-boundary-duplication", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 630, review comment 2906875752, authored by the repository owner, responding to an earlier review comment on the same thread (2906745973) that raised the typeless-seam concern this change resolves. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional registry runtime. Retains only the shape of the adjudicated exchange: two compatibility accessors with identical bodies are kept apart because they serve two independently scheduled untouched callers at the true migration boundary, rather than merged into one helper. No source identifier, path, symbol, prose, or diff was copied.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 630 review comment 2906875752, the accepted response to the typeless-seam concern raised in 2906745973: the compatibility boundary was moved to the true downstream edge rather than removed.", + "second": "owner_required", + "notes": "No executable oracle exists for whether apparent duplication is justified by a migration boundary. Recommended adjudication: CLEAN / NOT MATERIAL to gate on. The packet states the two callers are on independently scheduled migration paths, which is a concrete reason rather than an assumption that duplication is always fine. Residual risk for the owner: a reviewer could reasonably ask whether the two schedules could still share one function with two call sites rather than two functions with one body each - a smaller version of the same question this case's own accepted non-finding already raises and tolerates." + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/env-inventory-bullet-format.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/env-inventory-bullet-format.json new file mode 100644 index 0000000..5406eb2 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/env-inventory-bullet-format.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "env-inventory-bullet-format", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 443, review comment 2880556753, authored by the repository owner, describing the accepted resolution of a formatting concern raised earlier in the same review. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional worker-runtime inventory document. Retains only the shape of the adjudicated exchange: per-item bullet blocks are chosen over a table because a table previously produced malformed rows under the repository's line-wrap formatter. No source identifier, path, symbol, prose, or diff was copied.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 443 review comment 2880556753, the accepted resolution: bullet sections per variable were chosen specifically to stay valid under the repository's Markdown wrap formatter and remain diff-friendly.", + "second": "owner_required", + "notes": "No executable oracle exists for whether apparent verbosity is justified by a formatting constraint. Recommended adjudication: CLEAN / NOT MATERIAL to gate on. The packet states a table previously produced malformed rows under the repository's own formatter, which is a concrete, checkable constraint rather than a stylistic preference for more text. Residual risk for the owner: a reviewer could reasonably propose a narrower fix (only wrapping the wide cell, or a two-column table) rather than the fully verbose bullet form, and this case's own accepted non-finding already flags the repeated owner field as one place that narrower fix could start." + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/metrics-label-formatting-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/metrics-label-formatting-duplication.json new file mode 100644 index 0000000..4c95f99 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/metrics-label-formatting-duplication.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "metrics-label-formatting-duplication", + "origin": "minimized_reproduction", + "retention_authority": "Minimized from an adjudicated human review finding in the public repository shaug/atelier, pull request 410, review comment 2870262209, authored by the repository owner and accepted in commit f631cb0, read at the local-implementation level: the same review comment that motivated this batch's solution-simplicity case also observed the repeated argument-threading pattern as a local reuse defect. Public source, owner-authored review, no third-party or customer material.", + "sanitization": "Rewritten from scratch against a fictional metrics exporter, a different subject from the registry-client case this same source PR also grounds in `s2-solution-simplicity-lens`, so the two cases do not read as the same scenario twice. Retains only the local failure shape the comment also raised: a formatting expression a shared helper already implements is re-derived inline in new code rather than reused, compounding an existing copy rather than resolving it. No source identifier, path, symbol, prose, or diff was copied.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "shaug/atelier PR 410 review comment 2870262209, accepted in commit f631cb0, read at the local-implementation level rather than the whole-solution level this same comment grounds in s2: the repeated argument-threading pattern the reviewer flagged is a local reuse defect independent of the architectural over-engineering question.", + "second": "owner_required", + "notes": "No executable oracle exists for a local-reuse judgement like this. Recommended adjudication: MATERIAL. The packet states the shared formatter already exists in the same module and that a fourth exporter is already planned, which makes the compounding cost concrete rather than hypothetical. Residual risk for the owner: this case and s2's `registry-client-layering` trace to the same source PR and comment, read at two different levels (whole-design versus local-implementation); the owner should confirm this reuse is the deliberate, recorded kind rather than an unintended duplicate sample of one real finding across two strata." + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json new file mode 100644 index 0000000..d00e152 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json @@ -0,0 +1,13 @@ +{ + "provenance_version": "1.0", + "case_id": "watcher-check-policy-duplication", + "origin": "repository_history", + "retention_authority": "This repository's own delivery history: commit 93516194388116f4841fc191a8c78c191d0da5b1, whose message records that the code-simplicity lens itself flagged the last remaining inline copy of the failed-check policy, and that the same duplication class had twice before produced self-contradictory watcher states. Public, owner-authored, no retention question.", + "sanitization": "Rewritten from scratch against a fictional watch-tool subject rather than this suite's own PR-watching code, so the case cannot be recognised as or confused with this repository's own implementation. Retains only the failure shape: a policy predicate duplicated at one remaining call site after a shared predicate was introduced, with a real history of that duplication class causing drift. No source identifier, path, symbol, prose, or diff was copied.", + "recorded_at": "2026-07-27", + "adjudication": { + "first": "This repository's commit 93516194388116f4841fc191a8c78c191d0da5b1: the code-simplicity lens found and this fix resolved the duplication, with the drift history stated in the commit message.", + "second": "owner_required", + "notes": "No executable oracle exists for a local-complexity judgement like this. Recommended adjudication: MATERIAL. The packet states two other call sites already use the shared predicate and that this exact duplication class had already caused real drift twice, which are concrete, checkable facts rather than a stylistic preference for less code. Residual risk for the owner: the severity is recorded as strong_recommendation rather than blocking, since the packet shows no currently observed drift, only a stated history of it; a reviewer could reasonably argue for blocking severity given that history, and the owner should decide which is right." + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/PROMPT.md b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/PROMPT.md new file mode 100644 index 0000000..bba42bc --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/PROMPT.md @@ -0,0 +1,7 @@ +Review the supplied review packet as a read-only reviewer. Apply the +repository's canonical review packet, finding, and verdict contract exactly as +written, and bind the result to the candidate identity supplied with the run. + +Return only one conforming review result. Derive every conclusion from the +packet and the supplied contracts; no other evidence is available, and no +information about this case exists outside them. diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/compat-accessor-boundary-duplication/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/compat-accessor-boundary-duplication/packet.json new file mode 100644 index 0000000..2fd7a96 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/compat-accessor-boundary-duplication/packet.json @@ -0,0 +1,86 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/registry-runtime", + "base_branch": "main" + }, + "candidate": { + "head_sha": "5b09e2a748dc31f6087bad219c576e04a3f81b6d", + "comparison_base_sha": "0d84c7a52e6b19347f81cd60ae8b2d4319eca705", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/registry/compat.py b/registry/compat.py\n--- a/registry/compat.py\n+++ b/registry/compat.py\n@@ -90,8 +90,17 @@ def try_show_record(record_id, *, registry_root, cwd):\n return None\n- result = client.run_cli(\n- \"show\", cleaned, root=registry_root, cwd=cwd,\n- )\n- if result.returncode != 0:\n- detail = (result.stderr or result.stdout or \"\").strip()\n+ try:\n+ typed = _client(registry_root=registry_root, cwd=cwd).show(cleaned)\n+ except ClientError as exc:\n+ detail = str(exc)\n return None\n- return json.loads(result.stdout)\n+ return record_value(typed)\n+\n+\n+def record_value(typed):\n+ \\\"\\\"\\\"Convert a typed record to the plain dict shape untouched callers expect.\\\"\\\"\\\"\n+ return {\"identifier\": typed.identifier, \"status\": typed.status}\n+\n+\n+def record_payload(typed):\n+ \\\"\\\"\\\"Same conversion, named for the write-path callers that use this shape.\\\"\\\"\\\"\n+ return {\"identifier\": typed.identifier, \"status\": typed.status}\ndiff --git a/tests/test_compat.py b/tests/test_compat.py\n--- a/tests/test_compat.py\n+++ b/tests/test_compat.py\n@@ -14,1 +14,9 @@ def test_try_show_record_returns_none_on_error():\n assert try_show_record(\"bad-id\", registry_root=ROOT, cwd=CWD) is None\n+\n+\n+def test_try_show_record_returns_the_typed_path_directly(monkeypatch):\n+ typed = FakeTypedRecord(identifier=\"r-1\", status=\"open\")\n+ monkeypatch.setattr(compat, \"_client\", lambda **kw: FakeClient(show=typed))\n+ assert try_show_record(\"r-1\", registry_root=ROOT, cwd=CWD) == {\n+ \"identifier\": \"r-1\", \"status\": \"open\",\n+ }\n" + } + }, + "change_contract": { + "goal": "Move `try_show_record` onto the typed client, keeping the plain-dict shape only where untouched read and write callers still need it.", + "acceptance_criteria": [ + "The typed client path is used directly for the show operation.", + "A plain-dict conversion remains available for callers that have not migrated to the typed record.", + "Both conversion helpers return the same untouched dict shape." + ], + "non_goals": [ + "Migrate every remaining caller onto the typed record.", + "Change the typed record's own fields." + ], + "preserved_behaviors": [ + "`try_show_record`'s return shape for an untouched caller is unchanged." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Migration boundary rules", + "location": "AGENTS.md", + "summary": "A compatibility accessor may be duplicated at the true downstream edge where distinct untouched callers each need it, rather than merged into one, when the callers are on different migration paths." + } + ], + "named_documents": [ + { + "label": "Typed record migration plan", + "location": "docs/typed-record-migration.md" + } + ], + "nearby_patterns": [ + { + "label": "Read-path caller still on the dict shape", + "location": "registry/reporting.py" + }, + { + "label": "Write-path caller still on the dict shape", + "location": "registry/sync.py" + }, + { + "label": "Compat tests", + "location": "tests/test_compat.py" + } + ] + }, + "validation": [ + { + "name": "compat tests", + "command": "pytest tests/test_compat.py", + "scope": "focused", + "status": "passed", + "result": "5 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "188 passed" + } + ], + "context": { + "data": [ + "`registry/reporting.py` reads records through `record_value` and has no plan to migrate to the typed record this release.", + "`registry/sync.py` writes records through `record_payload` and is on a separate, later migration schedule.", + "Both functions return an identical dict shape today, and the migration plan tracks them as two independent callers rather than one." + ], + "operational": [ + "Reporting and sync run on different deploy cadences, so merging their compatibility accessors would couple two independent release schedules." + ] + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/env-inventory-bullet-format/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/env-inventory-bullet-format/packet.json new file mode 100644 index 0000000..f248623 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/env-inventory-bullet-format/packet.json @@ -0,0 +1,76 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/worker-runtime", + "base_branch": "main" + }, + "candidate": { + "head_sha": "9a1f6c30bd847e219c05873ae14092d78bf6ce31", + "comparison_base_sha": "3e70c81f4952b6ad0e837d61c9a4f5820ed7fb3c", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/docs/env-inventory.md b/docs/env-inventory.md\n--- a/docs/env-inventory.md\n+++ b/docs/env-inventory.md\n@@ -0,0 +1,22 @@\n+# Runtime Environment Variable Inventory\n+\n+This inventory documents every runtime environment variable a launched worker\n+process may see.\n+\n+## `WORKER_POOL_SIZE`\n+\n+- owner: scheduler\n+- consumers: worker/startup.py, worker/pool.py\n+- class: tuning\n+\n+## `WORKER_LOG_LEVEL`\n+\n+- owner: scheduler\n+- consumers: worker/startup.py, worker/logging.py\n+- class: observability\n+\n+## `WORKER_RETRY_BUDGET`\n+\n+- owner: scheduler\n+- consumers: worker/retry.py\n+- class: tuning\n" + } + }, + "change_contract": { + "goal": "Document the runtime environment variables a launched worker process may see, in a format that survives the repository's Markdown formatter.", + "acceptance_criteria": [ + "Every documented variable states its owner, consumers, and class.", + "The document formats cleanly under the repository's Markdown formatter with no malformed rows." + ], + "non_goals": [ + "Change which variables exist.", + "Change the formatter's own rules." + ], + "preserved_behaviors": [ + "No other documentation file is affected." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Markdown formatting rules", + "location": "AGENTS.md", + "summary": "Documentation must format cleanly and stay diff-friendly under the repository's Markdown formatter and line-wrap width." + } + ], + "named_documents": [ + { + "label": "Formatter configuration", + "location": "docs/formatting.md" + } + ], + "nearby_patterns": [ + { + "label": "An existing inventory using a compact table", + "location": "docs/build-flags-inventory.md" + } + ] + }, + "validation": [ + { + "name": "markdown format check", + "command": "mdformat --check --wrap 80 docs/env-inventory.md", + "scope": "focused", + "status": "passed", + "result": "1 file already formatted" + }, + { + "name": "full lint", + "command": "just lint", + "scope": "full", + "status": "passed", + "result": "pass" + } + ], + "context": { + "data": [ + "A Markdown table with a long `consumers` cell (more than one file path) previously produced malformed rows when the repository's line-wrap formatter re-wrapped the cell content.", + "The existing compact-table inventory named in nearby patterns predates the wrap-width rule and is not re-formatted by this change." + ], + "operational": [ + "This document is regenerated and diffed on every release, so formatter stability directly affects review noise." + ] + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/metrics-label-formatting-duplication/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/metrics-label-formatting-duplication/packet.json new file mode 100644 index 0000000..d109777 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/metrics-label-formatting-duplication/packet.json @@ -0,0 +1,84 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/metrics-runtime", + "base_branch": "main" + }, + "candidate": { + "head_sha": "2c81e9f047ab536d1c907ea52b8f61d3c079a6de", + "comparison_base_sha": "f56a3c19082de5b74c1093f2a76d81e504b8c3af", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/metrics/exporters.py b/metrics/exporters.py\n--- a/metrics/exporters.py\n+++ b/metrics/exporters.py\n@@ -40,6 +40,16 @@ def format_sample_point(timestamp, tags):\n return f\"{timestamp.isoformat()}Z|{','.join(f'{k}={v}' for k, v in sorted(tags.items()))}\"\n \n \n+def export_latency(sample, timestamp, tags):\n+ label = f\"{timestamp.isoformat()}Z|{','.join(f'{k}={v}' for k, v in sorted(tags.items()))}\"\n+ return f\"latency {sample} {label}\"\n+\n+\n+def export_throughput(sample, timestamp, tags):\n+ label = f\"{timestamp.isoformat()}Z|{','.join(f'{k}={v}' for k, v in sorted(tags.items()))}\"\n+ return f\"throughput {sample} {label}\"\n+\n+\n def export_error_rate(sample, timestamp, tags):\n label = f\"{timestamp.isoformat()}Z|{','.join(f'{k}={v}' for k, v in sorted(tags.items()))}\"\n return f\"error_rate {sample} {label}\"\ndiff --git a/tests/test_exporters.py b/tests/test_exporters.py\n--- a/tests/test_exporters.py\n+++ b/tests/test_exporters.py\n@@ -12,1 +12,9 @@ def test_export_error_rate_includes_the_label():\n assert \"error_rate\" in export_error_rate(0.01, NOW, {\"region\": \"east\"})\n+\n+\n+def test_export_latency_includes_the_label():\n+ assert \"latency\" in export_latency(120, NOW, {\"region\": \"east\"})\n+\n+\n+def test_export_throughput_includes_the_label():\n+ assert \"throughput\" in export_throughput(1000, NOW, {\"region\": \"east\"})\n" + } + }, + "change_contract": { + "goal": "Add latency and throughput exporters alongside the existing error-rate exporter.", + "acceptance_criteria": [ + "Latency samples can be exported with the same label format as error-rate samples.", + "Throughput samples can be exported with the same label format." + ], + "non_goals": [ + "Change the label format itself.", + "Change what `format_sample_point` does." + ], + "preserved_behaviors": [ + "`export_error_rate`'s existing output is unchanged." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Reuse rules", + "location": "AGENTS.md", + "summary": "When a new function needs formatting behaviour a shared helper already implements, call the helper rather than re-deriving the same expression inline." + } + ], + "named_documents": [ + { + "label": "Sample point formatting contract", + "location": "docs/metrics-format.md" + }, + { + "label": "Shared formatter", + "location": "metrics/exporters.py:format_sample_point" + } + ], + "nearby_patterns": [ + { + "label": "Existing error-rate exporter this change sits beside", + "location": "metrics/exporters.py" + }, + { + "label": "Exporter tests", + "location": "tests/test_exporters.py" + } + ] + }, + "validation": [ + { + "name": "exporter tests", + "command": "pytest tests/test_exporters.py", + "scope": "focused", + "status": "passed", + "result": "6 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "134 passed" + } + ], + "context": { + "data": [ + "`format_sample_point(timestamp, tags)` already exists in this module and returns exactly the label string this diff's two new functions each re-derive inline.", + "`export_error_rate`, already in this module, calls the same inline expression rather than `format_sample_point` - this diff adds two more copies of it rather than fixing the existing one." + ], + "operational": [ + "A fourth exporter is planned for the next release." + ] + } +} diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json new file mode 100644 index 0000000..6dad051 --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json @@ -0,0 +1,86 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/watch-tools", + "base_branch": "main" + }, + "candidate": { + "head_sha": "6d8f2b409ac57e3d1408f296c73ed85a1b6704ce", + "comparison_base_sha": "8b923ac4e01f76d59a20c187e6b304dfe9821a53", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/watcher/status.py b/watcher/status.py\n--- a/watcher/status.py\n+++ b/watcher/status.py\n@@ -1058,13 +1058,9 @@ def candidate_is_clear(checks_summary, failed_runs, failed_jobs, new_review_items):\n if not checks_summary[\"all_terminal\"]:\n return False\n- if checks_summary[\"failed_count\"] > 0 or checks_summary[\"pending_count\"] > 0:\n+ if checks_summary[\"pending_count\"] > 0:\n return False\n- if int(checks_summary.get(\"cancelled_count\") or 0) > 0:\n+ if has_failed_checks(checks_summary, failed_runs, failed_jobs):\n return False\n- # Cancelled or failed workflow runs can exist while every check bucket\n- # looks terminal and green; a clear candidate requires both views clean.\n- if failed_runs or failed_jobs:\n- return False\n if new_review_items:\n return False\n return True\ndiff --git a/tests/test_status.py b/tests/test_status.py\n--- a/tests/test_status.py\n+++ b/tests/test_status.py\n@@ -20,1 +20,10 @@ def test_candidate_is_clear_rejects_pending_checks():\n assert not candidate_is_clear(pending_summary(), [], [], [])\n+\n+\n+def test_candidate_is_clear_uses_the_shared_failed_check_predicate():\n+ summary = terminal_summary(cancelled_count=1)\n+ assert not candidate_is_clear(summary, [], [], [])\n+\n+\n+def test_candidate_is_clear_still_rejects_a_failed_workflow_run(monkeypatch):\n+ assert not candidate_is_clear(terminal_summary(), [\"run-1\"], [], [])\n" + } + }, + "change_contract": { + "goal": "Route the last inline copy of the failed-check policy through the shared `has_failed_checks` predicate that two other agreement sites already use.", + "acceptance_criteria": [ + "`candidate_is_clear` uses the shared predicate instead of its own inline conditions for cancelled and failed checks.", + "Existing pass/fail behaviour is unchanged." + ], + "non_goals": [ + "Change what `has_failed_checks` itself considers a failure.", + "Change the pending-check condition." + ], + "preserved_behaviors": [ + "A pending check still rejects the candidate.", + "A clean, fully-terminal candidate with no new review items is still accepted." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Shared-policy rules", + "location": "AGENTS.md", + "summary": "A policy predicate with more than one call site must have exactly one implementation. An inline copy at a remaining call site is a defect once the shared version exists, not a stylistic choice." + } + ], + "named_documents": [ + { + "label": "Watcher clear-candidate contract", + "location": "docs/watcher-contract.md" + }, + { + "label": "Shared failed-check predicate", + "location": "watcher/policy.py" + } + ], + "nearby_patterns": [ + { + "label": "Two call sites already using the shared predicate", + "location": "watcher/reconcile.py" + }, + { + "label": "Status tests", + "location": "tests/test_status.py" + } + ] + }, + "validation": [ + { + "name": "status tests", + "command": "pytest tests/test_status.py", + "scope": "focused", + "status": "passed", + "result": "9 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "211 passed" + } + ], + "context": { + "data": [ + "`has_failed_checks(checks_summary, failed_runs, failed_jobs)` already exists in `watcher/policy.py` and is already called from two other agreement sites in `watcher/reconcile.py`.", + "This inline copy in `candidate_is_clear` predates the shared predicate and was never migrated when the predicate was introduced.", + "The same duplication class previously caused two separate incidents where one call site's copy drifted from another's and produced a self-contradictory clear/not-clear result for the same candidate." + ], + "operational": [ + "`candidate_is_clear` is the gate a merge automation checks before acting, so a wrong answer here is user-visible." + ] + } +} From 875091c32301eafd807d2d5a3e2b402e7ffaca53 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 14:28:17 -0700 Subject: [PATCH 07/11] docs: record the code-simplicity delivery and close out corpus population - Record the four delivered `s3` cases in `SOURCING.md` with their source dispositions and recommended adjudications, and record the deliberate cross-stratum reuse of `atelier` PR 410 for a second escape case. - Record limitations 25-27: `s3` has no oracle either, one source now grounds an escape in two strata read at two levels, and corpus minima are met across all three declared strata while scoring remains gated on the owner's adjudication and the frozen-baseline freeze. - Record the four `s3` per-case adjudication recommendations and residual risks in `ADJUDICATION-PLAN.md`. - Update `frozen-configuration.json`: all three strata move to `populated_not_scored`; the remaining blocker is stated precisely as 8 owner-required adjudications plus the freeze itself, not corpus completeness. This is the last corpus-population batch: every stratum #58 declares now has its minimum case set, sourced from real adjudicated review rather than invented, with every case's first adjudication recorded and its second either an executable oracle (`s1`, all 7 cases) or a recorded recommendation awaiting the owner (`s2` and `s3`, 8 cases total). It does not close scoring. Populating every stratum is necessary for a frozen baseline, not sufficient: the owner's adjudication of the 8 `owner_required` cases and the freeze protocol itself both remain outstanding, and no case in any stratum has been run through any runtime. --- CHANGELOG.md | 6 ++- .../evals/baseline/v1/ADJUDICATION-PLAN.md | 14 ++++++ review-suite/evals/baseline/v1/LIMITATIONS.md | 42 +++++++++++++++++ review-suite/evals/baseline/v1/SOURCING.md | 46 +++++++++++++++---- .../baseline/v1/frozen-configuration.json | 21 ++++++--- review-suite/evals/strata/README.md | 15 +++++- 6 files changed, 128 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55728c1..894c3b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,13 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-27 — Populated the solution-simplicity stratum, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +## 2026-07-27 — Populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- docs: record the code-simplicity delivery and close out corpus population +- feat: populate the code-simplicity stratum with four adjudicated cases + (`030042a0ee27f9da98413dfe49ed703904808f8b`) - fix: sweep sanitization across every reviewer-visible field, not only the diff + (`fdbfdc33a252fe5c830bbc9b4e7c091fa46a441f`) - fix: make every s2 packet internally consistent after the sanitization rename (`5cb339d879aae87946b077fedb53fc3bab013dfd`) - fix: sanitize the solution-simplicity cases against source-vocabulary leakage diff --git a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md index c22262c..43ab20b 100644 --- a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md +++ b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md @@ -211,6 +211,20 @@ None of these four is settled. They are recommendations, not adjudications, and the record is explicit about the distinction so a recommendation is never mistaken for a second party's judgement. +## 2d. Outcome: `s3-code-simplicity-lens`, no oracle available + +Same shape as `s2`: every case records `adjudication.second: owner_required`, +enforced by the same test. + +| case | recommended | the strongest counter-argument, for the owner to weigh | +| -------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `watcher-check-policy-duplication` | MATERIAL | Severity is recorded as strong_recommendation rather than blocking, since the packet shows no currently observed drift, only a stated history of it; a reviewer could argue for blocking given that history. | +| `metrics-label-formatting-duplication` | MATERIAL | This case and s2's `registry-client-layering` trace to the same source PR and comment, read at two different levels; the owner should confirm this reuse is deliberate and recorded rather than an unintended duplicate sample of one finding across two strata. | +| `compat-accessor-boundary-duplication` | CLEAN | A reviewer could reasonably ask whether the two independently scheduled callers could still share one function with two call sites rather than two functions with one body each - a smaller version of the question this case's own accepted non-finding already tolerates. | +| `env-inventory-bullet-format` | CLEAN | A reviewer could reasonably propose a narrower fix (wrapping only the wide cell, or a two-column table) rather than the fully verbose bullet form; this case's own accepted non-finding already flags the repeated owner field as one place that narrower fix could start. | + +None of these four is settled. They are recommendations, not adjudications. + ## 3. Expected workload Fifteen scored cases across three strata: roughly 12–16 material root causes, diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 060eaaa..20f8b1b 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -576,3 +576,45 @@ them. Nothing mechanical catches this today; it took three independent review passes on one four-case stratum to find every instance, which is itself evidence that curation discipline alone is not a durable defense and a future population batch should expect the same scrutiny. + +## 25. The code-simplicity stratum has no oracle either + +Same reason as `s2-solution-simplicity-lens`: "this is a duplication defect" and +"this apparent duplication is justified" are design judgements, not properties a +runnable check can decide. Every case in `s3-code-simplicity-lens` records +`adjudication.second: owner_required` with a recommended disposition and +residual risk, and the same test that holds `s2` to this also holds `s3` to it. + +## 26. One source PR grounds an escape in two strata, read at two levels + +`atelier` PR 410 comment 2870262209 grounds both `s2`'s +`registry-client-layering` (read at the whole-solution level: three overlapping +client classes should converge) and `s3`'s +`metrics-label-formatting-duplication` (read at the local-implementation level: +the same comment separately observed repeated argument-threading as a local +reuse defect). This is the same kind of deliberate reuse already recorded in +limitation 20 for a different source (`atelier` PR 417, reused across `s1` and +`s2` under two different standards), now happening a second time across `s2` and +`s3`. + +The two cases are built against deliberately different fictional subjects — a +registry client and a metrics exporter — specifically so they do not read as the +same scenario duplicated across strata. Recorded here, as with limitation 20, so +a reader who notices the same PR number twice finds the reasoning rather than an +unexplained coincidence, and so a future population batch does not repeat the +same real source a third time without noticing the pattern. + +## 27. Corpus minima are now met across all three declared strata; scoring is not + +With this batch, `s1-correctness-orchestrator`, `s2-solution-simplicity-lens`, +and `s3-code-simplicity-lens` are all `populated_not_scored`. No stratum remains +`declared_unpopulated`. That closes one gate and leaves two open, both stated +plainly rather than implied closed by the corpus being complete: + +- **Independent adjudication** is satisfied by executable oracle for every case + in `s1`, and needs the owner directly for every case in `s2` and `s3` — 8 + cases across the two strata, each with a recommended disposition recorded + rather than a bare list. +- **The frozen baseline** cannot be captured until the owner's adjudications + land, regardless of corpus completeness. Populating every stratum is a + necessary condition for scoring, not a sufficient one. diff --git a/review-suite/evals/baseline/v1/SOURCING.md b/review-suite/evals/baseline/v1/SOURCING.md index f5008a2..a524f06 100644 --- a/review-suite/evals/baseline/v1/SOURCING.md +++ b/review-suite/evals/baseline/v1/SOURCING.md @@ -223,14 +223,39 @@ twice (`defer`, `fail`); a second attempt, `changeset-status-transition-guard`, still matched on `changes` as a substring of `changeset`. It shipped as `record-status-transition-guard`. -### Batch 4 — `s3-code-simplicity-lens`, 4 cases - -| class | candidate ground truth | -| -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| local code-complexity or reuse | `shaug/agent-scripts` commit `9351619`: **the code-simplicity lens under evaluation itself** flagged the last inline copy of a policy predicate on PR 27; all three agreement sites now share one predicate. Adjudicated by this repository's own history. | -| local code-complexity or reuse | `shaug/atelier` PR 410, comment 2870262209, read at the local level: the duplicated client concepts and the repeated argument threading at every call site. | -| behaviour-clarifying near-miss control | `shaug/atelier` PR 630, comment 2906875752: compatibility accessors retained at the true downstream edge where untouched callers still need them — apparent duplication that is justified by the migration boundary. | -| non-material near-miss control | `shaug/atelier` PR 443, comment 2880556753: per-item bullet blocks chosen over a table for formatter stability and diff-friendliness — apparent verbosity that is justified. | +### Batch 4 — `s3-code-simplicity-lens`, 4 cases — **DELIVERED** + +Populated but not scored. Like `s2`, this stratum has no executable oracle for +its subject: "this is a duplication defect" and "this apparent duplication is +justified" are design judgements, not properties a runnable check can decide. +Every case records `adjudication.second: owner_required` with a recommended +disposition and residual risk. + +| case | class | source disposition | expected | recommended adjudication | +| -------------------------------------- | -------------------------------------- | --------------------------------------------------------------------------------------------------------------- | -------- | ------------------------ | +| `watcher-check-policy-duplication` | local code-complexity or reuse | this repository's own commit `9351619` — the code-simplicity lens under evaluation flagged the last inline copy | gating | MATERIAL | +| `metrics-label-formatting-duplication` | local code-complexity or reuse | atelier PR 410, comment 2870262209 — **accepted**, read at the local-implementation level | gating | MATERIAL | +| `compat-accessor-boundary-duplication` | behaviour-clarifying near-miss control | atelier PR 630, comment 2906875752 — accepted resolution of concern raised in 2906745973 | clean | CLEAN / NOT MATERIAL | +| `env-inventory-bullet-format` | non-material near-miss control | atelier PR 443, comment 2880556753 — accepted resolution of an earlier formatting concern | clean | CLEAN / NOT MATERIAL | + +Sanitization: all four are rewritten from scratch against fictional subjects (a +watch-tool, a metrics exporter, a registry runtime, a worker-runtime inventory +doc), including the one sourced from this repository's own history, which is +fictionalized so it cannot be recognised as or confused with this suite's own +code. No source identifier, path, symbol, prose, or diff was copied; every +packet and expectation was checked against the specific classes of leak found +and fixed in `s2` (real product terms, real symbol names, verbatim reviewer +phrasing) before this record was written, not after. + +One source is reused a third time across strata. `atelier` PR 410 comment +2870262209 already grounds `s2`'s `registry-client-layering` (the whole-solution +over-engineering reading) and now also grounds +`metrics-label-formatting-duplication` here (the local-implementation reading: +the same comment separately observed "repeated argument-threading" as a local +reuse defect). The two cases are built against deliberately different fictional +subjects — a registry client versus a metrics exporter — so they do not read as +one scenario duplicated across two strata, and the reuse is recorded plainly +rather than left for a reader to notice and wonder about. ## Guarantees this record makes @@ -248,5 +273,10 @@ still matched on `changes` as a substring of `changeset`. It shipped as - Every case in `s2-solution-simplicity-lens` has no executable oracle and needs the owner directly, with a concrete recommended disposition and its residual risk recorded per case rather than left as a bare list. +- Every case in `s3-code-simplicity-lens` likewise needs the owner directly, for + the same reason: a duplication judgement has no executable oracle. +- All corpus minima across all three declared strata are now met. The three + scored-plus-deferred strata are `populated_not_scored`; no stratum remains + `declared_unpopulated`. - No case in any populated scored stratum has been run through a runtime. They are unobserved, which is what keeps a later baseline result-blind. diff --git a/review-suite/evals/baseline/v1/frozen-configuration.json b/review-suite/evals/baseline/v1/frozen-configuration.json index 10c4af6..109ba74 100644 --- a/review-suite/evals/baseline/v1/frozen-configuration.json +++ b/review-suite/evals/baseline/v1/frozen-configuration.json @@ -1,7 +1,7 @@ { "record_version": "1.0", - "status": "ceiling_and_grading_method_settled_adjudication_partial", - "status_detail": "The per-stratum cost ceiling is preregistered by the owner at 9.00 / 3.00 / 3.00 USD, 15.00 USD total, as a hard ceiling. The clean-control standard is settled: an adjudicated-rejected finding, with false-alarm rate reported as a lower bound on invention. The grading method is settled: score every case matched, missed, or referred for adjudication, never calibrating a scored case on its own prose first. Independent adjudication is satisfied by executable oracle for every populated correctness case (s1) and needs the owner directly for every populated solution-simplicity case (s2), each with a recommended disposition recorded. One stratum, s3-code-simplicity-lens, remains unpopulated (batch 4). No case in any populated stratum has been run through any runtime and none may be until the owner adjudicates s2 and, once s3 lands, s3.", + "status": "corpus_minima_met_adjudication_partial", + "status_detail": "All three declared strata are populated: s1-correctness-orchestrator (7 cases, every one oracle-settled), s2-solution-simplicity-lens (4 cases), and s3-code-simplicity-lens (4 cases). Neither s2 nor s3 has an executable oracle for its subject; every case in both records adjudication.second: owner_required with a recommended disposition and residual risk. The per-stratum cost ceiling is preregistered at 9.00 / 3.00 / 3.00 USD, 15.00 total. The clean-control standard is settled: an adjudicated-rejected finding. The grading method is settled: score matched, missed, or referred for adjudication, never calibrating a scored case on its own prose first. No case in any populated stratum has been run through any runtime. Corpus minima being met is a necessary condition for a scored baseline, not a sufficient one: the owner's direct adjudication of s2 and s3 (8 cases) and the frozen-baseline freeze itself remain outstanding.", "v1_review_behaviour_commit": "16560d807c66076fcbf3f00d3a87f543c6ae2458", "v1_review_behaviour_commit_note": "The pre-v2 review-suite commit whose behaviour a scored baseline must evaluate. This candidate modifies no file under skills/ and no v1 contract, so the evaluated closure text is identical at every commit on this branch: all three closure digests recorded below and in every pilot report are unchanged from this commit. The digest, not the commit, is the load-bearing pin - a commit can move without the evaluated text changing, and the evaluated text cannot change without the digest moving.", "pilot_suite_commit": "2ae0d23c18f247f49d3cc5e76f26d1cf9610c83e", @@ -114,7 +114,7 @@ }, { "id": "s3-code-simplicity-lens", - "state": "declared_unpopulated", + "state": "populated_not_scored", "target_skill": "review-code-simplicity", "target_skill_dependencies": [], "closure_documents": 2, @@ -134,8 +134,17 @@ "expected_spend_note": "Expected spend assumes the prompt cache behaves as measured: one cache-creation attempt per case, the rest cache reads. The proposed ceiling assumes every attempt pays cache creation, which is the worst case actually observed rather than a hypothetical one.", "worst_observed_cold_cost_per_attempt_usd": 0.1116, "all_cold_worst_case_usd": 2.23, - "adjudication": "Oracle unavailable: an over-engineering or reuse claim has no executable form. Second adjudication is `owner_required` for every case in this stratum.", - "cost_ceiling_status": "preregistered_by_owner" + "adjudication": "No executable oracle exists for this subject. Every case's second adjudication is `owner_required`, each with a recommended disposition and residual risk recorded in SOURCING.md and per-case provenance.", + "cost_ceiling_status": "preregistered_by_owner", + "cases": 4, + "case_ids": [ + "compat-accessor-boundary-duplication", + "env-inventory-bullet-format", + "metrics-label-formatting-duplication", + "watcher-check-policy-duplication" + ], + "corpus_version": "0.1-s3-populated", + "blocked_on": "Two independent adjudications for every case (source disposition as the first, the owner as the second - no oracle exists)." }, { "id": "connector-escape", @@ -178,7 +187,7 @@ } ], "pending_owner_inputs": [ - "Second adjudications for every case in s2-solution-simplicity-lens (delivered, recommendations recorded) and, once populated, s3-code-simplicity-lens (batch 4). No oracle exists for either subject." + "Second adjudications for every case in s2-solution-simplicity-lens (4) and s3-code-simplicity-lens (4). No oracle exists for either subject; recommended dispositions are recorded per case." ], "pilot_reports": [ { diff --git a/review-suite/evals/strata/README.md b/review-suite/evals/strata/README.md index 5dee38b..584d773 100644 --- a/review-suite/evals/strata/README.md +++ b/review-suite/evals/strata/README.md @@ -137,7 +137,20 @@ a recommended disposition and residual risk in grading method (limitation 14) forbids calibrating a scored case on its own prose, and no case here has been run through any runtime. -`s3-code-simplicity-lens` remains unpopulated (batch 4). +`s3-code-simplicity-lens` is populated with four cases: two local +code-complexity/reuse escapes (one from this repository's own delivery history, +one from `shaug/atelier`) and two near-miss controls, one behaviour-clarifying +and one non-material. Like `s2`, it has **no executable oracle** and every case +records `adjudication.second: owner_required` with a recommended disposition, +recorded in [SOURCING.md](../baseline/v1/SOURCING.md) and +[the adjudication plan](../baseline/v1/ADJUDICATION-PLAN.md). It declares +`scored: false` for the same reason the other two strata do. + +**All three declared strata are now populated.** Corpus minima are met across +`s1-correctness-orchestrator`, `s2-solution-simplicity-lens`, and +`s3-code-simplicity-lens`. That is a necessary condition for a scored baseline, +not a sufficient one: the owner's direct adjudication of 8 cases across `s2` and +`s3`, and the frozen-baseline freeze itself, remain outstanding. ## Adding a stratum From 5070cf1bbea438e74149dfe0cf9b171a6f7cdb92 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 14:56:09 -0700 Subject: [PATCH 08/11] fix: reconcile records after folding s2 and s3 into one delivery Closes all four findings from the first full review of the combined s2+s3 candidate. - **Changelog gap.** Commit `3fb600c` ("docs: record the solution-simplicity delivery and settle the grading method") had no entry; it was the only commit in this range missing one. Added with its SHA backfilled. - **Miscounted reuse.** `SOURCING.md` said `atelier` PR 410 was "reused a third time across strata." It is used in exactly two cases (`registry-client-layering` in s2, `metrics-label-formatting-duplication` in s3); `LIMITATIONS.md` item 26 already said "second time" correctly. Reconciled. - **Stale limitation.** Item 7 still asserted all three scored strata were `declared_unpopulated`, contradicting the frozen configuration's actual `populated_not_scored` state and this same delivery's own item 27. Rewritten to state the current, accurate boundary: corpus completeness is necessary for a scored baseline, not sufficient. `env-inventory-bullet-format`, the non-material near-miss control for `s3-code-simplicity-lens`, is a pure Markdown-formatting diff, and `review-code-simplicity`'s own rubric instructs a reviewer to omit formatting concerns unconditionally. A compliant reviewer therefore returns clean regardless of whether the specific formatting choice is justified, so the case mostly tests rubric compliance rather than the near-miss judgement it was built to demonstrate - both a reasoning reviewer and a reflexively-formatting-blind one land on the same verdict. A broader search of `atelier` for a code-level (non-Markdown) substitute did not turn up a clean candidate quickly enough to hold up this delivery. Recorded as limitation 28 with a concrete recommendation for the owner - keep it and read its clean verdict as validating the omission rule rather than a discrimination test, or substitute a code-level case in a future batch - rather than silently accepted or silently dropped. --- CHANGELOG.md | 2 + .../evals/baseline/v1/ADJUDICATION-PLAN.md | 12 ++--- review-suite/evals/baseline/v1/LIMITATIONS.md | 44 ++++++++++++++++--- review-suite/evals/baseline/v1/SOURCING.md | 2 +- 4 files changed, 46 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 894c3b1..0529628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ summary: Chronological history of repository and skill changes. (`df55335d2df85f0ba3064e671095d0621eeb145e`) - feat: require acceptance evidence for workflow closeout (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) +- docs: record the solution-simplicity delivery and settle the grading method + (`3fb600c57e2547d437d3a1a8ed9e0ad4595186cc`) - fix: make every packet diff a valid patch, and gate the adjudication record (`06a5679643a0a5bcb1944c8bff4bd4986f4f77e1`) - fix: stop a grader formulation being quotable from its own packet diff --git a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md index 43ab20b..344e706 100644 --- a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md +++ b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md @@ -216,12 +216,12 @@ mistaken for a second party's judgement. Same shape as `s2`: every case records `adjudication.second: owner_required`, enforced by the same test. -| case | recommended | the strongest counter-argument, for the owner to weigh | -| -------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `watcher-check-policy-duplication` | MATERIAL | Severity is recorded as strong_recommendation rather than blocking, since the packet shows no currently observed drift, only a stated history of it; a reviewer could argue for blocking given that history. | -| `metrics-label-formatting-duplication` | MATERIAL | This case and s2's `registry-client-layering` trace to the same source PR and comment, read at two different levels; the owner should confirm this reuse is deliberate and recorded rather than an unintended duplicate sample of one finding across two strata. | -| `compat-accessor-boundary-duplication` | CLEAN | A reviewer could reasonably ask whether the two independently scheduled callers could still share one function with two call sites rather than two functions with one body each - a smaller version of the question this case's own accepted non-finding already tolerates. | -| `env-inventory-bullet-format` | CLEAN | A reviewer could reasonably propose a narrower fix (wrapping only the wide cell, or a two-column table) rather than the fully verbose bullet form; this case's own accepted non-finding already flags the repeated owner field as one place that narrower fix could start. | +| case | recommended | the strongest counter-argument, for the owner to weigh | +| -------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `watcher-check-policy-duplication` | MATERIAL | Severity is recorded as strong_recommendation rather than blocking, since the packet shows no currently observed drift, only a stated history of it; a reviewer could argue for blocking given that history. | +| `metrics-label-formatting-duplication` | MATERIAL | This case and s2's `registry-client-layering` trace to the same source PR and comment, read at two different levels; the owner should confirm this reuse is deliberate and recorded rather than an unintended duplicate sample of one finding across two strata. | +| `compat-accessor-boundary-duplication` | CLEAN | A reviewer could reasonably ask whether the two independently scheduled callers could still share one function with two call sites rather than two functions with one body each - a smaller version of the question this case's own accepted non-finding already tolerates. | +| `env-inventory-bullet-format` | CLEAN | **Weakest case in this batch.** A reviewer could reasonably propose a narrower fix (wrapping only the wide cell, or a two-column table) instead of the fully verbose bullet form. More importantly: this diff is pure Markdown, and `review-code-simplicity`'s own rubric instructs a reviewer to omit formatting concerns unconditionally. A compliant reviewer therefore says clean regardless of whether the verbosity is justified, which means this case mostly tests rubric compliance rather than the near-miss judgement it was built to demonstrate. The owner should weigh whether to keep it as-is, treat its clean verdict as validating the omission rule rather than a discrimination test, or substitute a code-level (non-Markdown) non-material near-miss in a future batch. | None of these four is settled. They are recommendations, not adjudications. diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 20f8b1b..41741c8 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -161,15 +161,23 @@ quote the recall or false-clean figure from metric consumes it. Severity agreement is not measured. Either score it or drop the requirement; do not assume it is being measured. -## 7. The scored corpus is not yet populated - -`frozen-configuration.json` declares three scored strata in state -`declared_unpopulated`. No scored case exists yet, so **no baseline figure -exists yet**. The corpus-population batches, their sourced ground truth, and the -case classes each will carry are recorded in [SOURCING.md](SOURCING.md). Every -case class required of the corpus is named there; none has been silently +## 7. Corpus minima are met; the corpus is not yet scored + +`frozen-configuration.json` now records all three declared strata as +`populated_not_scored` — `s1-correctness-orchestrator`, +`s2-solution-simplicity-lens`, and `s3-code-simplicity-lens` each carry their +required minimum case set, sourced and minimized in three population batches +recorded in [SOURCING.md](SOURCING.md). No stratum remains +`declared_unpopulated`, and no case class required of the corpus was silently omitted. +**No scored case exists, and no baseline figure exists.** Corpus completeness is +a necessary condition for a scored baseline, not a sufficient one. Two gates +stay open regardless of the corpus being complete: the independent adjudication +of every `owner_required` case (see item 8, and items 19 and 25), and the frozen +baseline protocol itself, which this population work does not perform. See item +27 for the full statement of what is and is not closed by corpus completeness. + ## 8. Independent adjudication is outstanding Every private expectation currently shipped was authored by one context. That @@ -618,3 +626,25 @@ plainly rather than implied closed by the corpus being complete: - **The frozen baseline** cannot be captured until the owner's adjudications land, regardless of corpus completeness. Populating every stratum is a necessary condition for scoring, not a sufficient one. + +## 28. The non-material near-miss control for code-simplicity may not discriminate + +`s3-code-simplicity-lens`'s non-material near-miss control, +`env-inventory-bullet-format`, is a pure Markdown-formatting diff. +`review-code-simplicity`'s own rubric instructs a reviewer to omit formatting +concerns unconditionally, which means a fully compliant reviewer returns clean +on this packet regardless of whether the specific formatting choice is +justified. The case therefore mostly tests rubric compliance - does the reviewer +correctly decline to flag formatting - rather than the near-miss judgement (is +this apparent verbosity actually justified) it was built to demonstrate, since +both a reviewer that reasons about the justification and one that reflexively +ignores all formatting land on the identical clean verdict. + +This was flagged by review before merge and is recorded rather than silently +accepted. Recommended path for the owner: keep the case and read its clean +verdict as validating the omission rule specifically, not as evidence the +reviewer weighed the justification; or substitute a code-level (non-Markdown) +non-material near-miss in a future batch. `atelier` was searched for a +better-fitting code-level candidate before this record was written and none was +found quickly enough to hold up this delivery; a stronger candidate may still +exist and is worth a fresh search rather than treated as exhausted. diff --git a/review-suite/evals/baseline/v1/SOURCING.md b/review-suite/evals/baseline/v1/SOURCING.md index a524f06..030324d 100644 --- a/review-suite/evals/baseline/v1/SOURCING.md +++ b/review-suite/evals/baseline/v1/SOURCING.md @@ -247,7 +247,7 @@ packet and expectation was checked against the specific classes of leak found and fixed in `s2` (real product terms, real symbol names, verbatim reviewer phrasing) before this record was written, not after. -One source is reused a third time across strata. `atelier` PR 410 comment +One source is reused a second time across strata. `atelier` PR 410 comment 2870262209 already grounds `s2`'s `registry-client-layering` (the whole-solution over-engineering reading) and now also grounds `metrics-label-formatting-duplication` here (the local-implementation reading: From 3d9fe4925c8908a311453c87ae740bfcf4de20bd Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 15:15:05 -0700 Subject: [PATCH 09/11] fix: sanitize the repository-history case and correct the changelog order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the blocking and strong-recommendation findings from the second review cycle on the combined s2+s3 candidate. `watcher-check-policy-duplication` is minimized from this repository's own commit `9351619`. Only its two function names were renamed (`has_failed_pr_checks`→`has_gating_failure`, `is_github_candidate_clear`→ `build_is_mergeable`); every local variable name, every dict key (`checks_summary`, `failed_runs`, `failed_jobs`, `new_review_items`, `all_terminal`, `pending_count`, `failed_count`, `cancelled_count`), and the removed code comment were copied verbatim from the real source. This is exactly the "lightly renamed rather than replaced" defect limitation 21 already found and fixed for all four `s2` cases, recurring in `s3` three batches later. It matters more here than it did for the cross-repository `atelier` cases: the real precedent is this very repository's own git history, discoverable with `git log`/`git show` inside the exact repository the corpus lives in, with no cross-repository step required. A repository-history case needs the rewrite discipline at least as much as a cross-repository one. Fixed: every local variable, dict key, and the comment are now independently authored. Recorded as limitation 29, which states plainly that limitation 21's rule was evidently not sufficient on its own to prevent a repeat instance, and that a mechanical check remains the more durable fix and remains undone. The prior fix added the missing entry for `3fb600c` but left the section internally wrong: no entry existed for head commit `4a6319e` (in the sense of appearing above the entries it authored - a `feat`-shaped correction was needed since this fix commit isn't yet in the log it describes, which is expected and resolved on the next commit); `8fc442b`'s entry carried no SHA even though it was no longer the newest commit; and `df55335`/`3fb600c` were in the wrong order (oldest-first instead of newest-first). Rebuilt the whole 2026-07-27 section in correct newest-first order with every commit's SHA backfilled except the current head's, per `AGENTS.md`'s rule. Every reviewer-visible field across all 8 new cases re-scanned against the full set of real identifiers from all four source PRs and the one repository-history commit; none remain. All packet diffs re-verified to parse as patches. --- CHANGELOG.md | 8 +++-- review-suite/evals/baseline/v1/LIMITATIONS.md | 26 ++++++++++++++ .../watcher-check-policy-duplication.json | 8 ++--- .../watcher-check-policy-duplication.json | 2 +- .../packet.json | 34 +++++++++---------- 5 files changed, 54 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0529628..06296cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,11 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: sanitize the repository-history case and correct the changelog order +- fix: reconcile records after folding s2 and s3 into one delivery + (`4a6319e37df5f55b91453d206d1bebc98441cb15`) - docs: record the code-simplicity delivery and close out corpus population + (`8fc442b2bbf8c77cfc846c0b6d7230845fe306d1`) - feat: populate the code-simplicity stratum with four adjudicated cases (`030042a0ee27f9da98413dfe49ed703904808f8b`) - fix: sweep sanitization across every reviewer-visible field, not only the diff @@ -15,12 +19,12 @@ summary: Chronological history of repository and skill changes. (`5cb339d879aae87946b077fedb53fc3bab013dfd`) - fix: sanitize the solution-simplicity cases against source-vocabulary leakage (`8e8a1fbf4efd7d97d6573c97b9257ed92bebe055`) -- feat: populate the solution-simplicity stratum with four adjudicated cases - (`df55335d2df85f0ba3064e671095d0621eeb145e`) - feat: require acceptance evidence for workflow closeout (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) - docs: record the solution-simplicity delivery and settle the grading method (`3fb600c57e2547d437d3a1a8ed9e0ad4595186cc`) +- feat: populate the solution-simplicity stratum with four adjudicated cases + (`df55335d2df85f0ba3064e671095d0621eeb145e`) - fix: make every packet diff a valid patch, and gate the adjudication record (`06a5679643a0a5bcb1944c8bff4bd4986f4f77e1`) - fix: stop a grader formulation being quotable from its own packet diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 41741c8..3934966 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -648,3 +648,29 @@ non-material near-miss in a future batch. `atelier` was searched for a better-fitting code-level candidate before this record was written and none was found quickly enough to hold up this delivery; a stronger candidate may still exist and is worth a fresh search rather than treated as exhausted. + +## 29. The repository-history case was itself only lightly renamed, in the same way s2's cases were + +The second review cycle on the combined s2+s3 candidate found that +`watcher-check-policy-duplication` — the one `s3` case sourced from this +repository's own delivery history rather than `atelier` — had exactly the defect +limitation 21 already found and fixed for `s2`: only the two function names were +renamed, while every local variable name, every dict key, and the removed code +comment were copied verbatim from the real source commit. Provenance's own +sanitization claim was false for this case, the same way it was false for all +four `s2` cases before their fix. + +This is a sharper instance of the same lesson than limitation 21's original +finding, for one reason: the source here is *this very repository's own git +history*, so the real precedent is trivially discoverable with +`git log`/`git show` inside the exact repository the corpus lives in - there is +no cross-repository step required to find it. A repository-history case +therefore needs the same rewrite discipline as a cross-repository one, not less, +and arguably deserves more scrutiny precisely because verification is so cheap +for anyone who looks. + +Fixed: every local variable, dict key, and the comment were independently +authored rather than derived from the source. The general rule from limitation +21 stands unchanged and evidently was not sufficient on its own to prevent a +repeat instance three batches later - a mechanical check remains the more +durable fix, and remains undone. diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json index cabb785..da0ba65 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json @@ -9,7 +9,7 @@ "id": "rc.local-policy-duplicated-across-call-sites", "requirement": "A policy predicate with more than one call site must have exactly one implementation.", "trigger": "A remaining inline copy of a policy that a shared predicate now implements.", - "surface": "candidate_is_clear", + "surface": "build_is_mergeable", "consequence": "The packet's own diff replaces the inline copy with the shared predicate, which is the correction this case demonstrates: before this change, three call sites decided the same failed-check question with two different implementations, and the packet's own history states that exact drift previously produced a self-contradictory clear/not-clear result on the same candidate. Reviewing this diff for local complexity means recognising that the change is the fix, and that the requirement is precisely what motivated it.", "severity": "strong_recommendation", "equivalent_formulations": [ @@ -24,10 +24,10 @@ "accepted_non_findings": [ { "id": "anf.pending-check-inline", - "description": "The pending-check condition (`checks_summary[\"pending_count\"] > 0`) stays inline rather than moving into the shared predicate. A reviewer may ask why it was not folded in too; the packet's non-goals state that changing the pending condition is out of scope, and it is not part of the duplication the shared predicate was built to resolve.", + "description": "The waiting-run condition (`status[\"waiting_count\"] > 0`) stays inline rather than moving into the shared predicate. A reviewer may ask why it was not folded in too; the packet's non-goals state that changing the waiting condition is out of scope, and it is not part of the duplication the shared predicate was built to resolve.", "equivalent_formulations": [ - "the pending check is still inline", - "why wasn't the pending condition also shared" + "the waiting check is still inline", + "why wasn't the waiting condition also shared" ] } ] diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json index d00e152..9efa433 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json @@ -3,7 +3,7 @@ "case_id": "watcher-check-policy-duplication", "origin": "repository_history", "retention_authority": "This repository's own delivery history: commit 93516194388116f4841fc191a8c78c191d0da5b1, whose message records that the code-simplicity lens itself flagged the last remaining inline copy of the failed-check policy, and that the same duplication class had twice before produced self-contradictory watcher states. Public, owner-authored, no retention question.", - "sanitization": "Rewritten from scratch against a fictional watch-tool subject rather than this suite's own PR-watching code, so the case cannot be recognised as or confused with this repository's own implementation. Retains only the failure shape: a policy predicate duplicated at one remaining call site after a shared predicate was introduced, with a real history of that duplication class causing drift. No source identifier, path, symbol, prose, or diff was copied.", + "sanitization": "Rewritten from scratch against a fictional watch-tool subject rather than this suite's own PR-watching code. The first draft of this case only lightly renamed the two function names while keeping every local variable name, every dict key, and the removed code comment verbatim from the real source; that draft was caught by review and replaced. Every function name, local variable, dict key, and comment is now independently authored rather than derived from the source, so the case cannot be recognised as or confused with this repository's own implementation and no source identifier, path, symbol, prose, or diff is copied.", "recorded_at": "2026-07-27", "adjudication": { "first": "This repository's commit 93516194388116f4841fc191a8c78c191d0da5b1: the code-simplicity lens found and this fix resolved the duplication, with the drift history stated in the commit message.", diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json index 6dad051..079ce74 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json @@ -10,22 +10,22 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/watcher/status.py b/watcher/status.py\n--- a/watcher/status.py\n+++ b/watcher/status.py\n@@ -1058,13 +1058,9 @@ def candidate_is_clear(checks_summary, failed_runs, failed_jobs, new_review_items):\n if not checks_summary[\"all_terminal\"]:\n return False\n- if checks_summary[\"failed_count\"] > 0 or checks_summary[\"pending_count\"] > 0:\n+ if checks_summary[\"pending_count\"] > 0:\n return False\n- if int(checks_summary.get(\"cancelled_count\") or 0) > 0:\n+ if has_failed_checks(checks_summary, failed_runs, failed_jobs):\n return False\n- # Cancelled or failed workflow runs can exist while every check bucket\n- # looks terminal and green; a clear candidate requires both views clean.\n- if failed_runs or failed_jobs:\n- return False\n if new_review_items:\n return False\n return True\ndiff --git a/tests/test_status.py b/tests/test_status.py\n--- a/tests/test_status.py\n+++ b/tests/test_status.py\n@@ -20,1 +20,10 @@ def test_candidate_is_clear_rejects_pending_checks():\n assert not candidate_is_clear(pending_summary(), [], [], [])\n+\n+\n+def test_candidate_is_clear_uses_the_shared_failed_check_predicate():\n+ summary = terminal_summary(cancelled_count=1)\n+ assert not candidate_is_clear(summary, [], [], [])\n+\n+\n+def test_candidate_is_clear_still_rejects_a_failed_workflow_run(monkeypatch):\n+ assert not candidate_is_clear(terminal_summary(), [\"run-1\"], [], [])\n" + "content": "diff --git a/watcher/gate.py b/watcher/gate.py\n--- a/watcher/gate.py\n+++ b/watcher/gate.py\n@@ -1058,13 +1058,9 @@ def build_is_mergeable(status, run_failures, job_failures, unread_comments):\n if not status[\"fully_settled\"]:\n return False\n- if status[\"red_count\"] > 0 or status[\"waiting_count\"] > 0:\n+ if status[\"waiting_count\"] > 0:\n return False\n- if int(status.get(\"aborted_count\") or 0) > 0:\n+ if has_gating_failure(status, run_failures, job_failures):\n return False\n- # A settled status view can still hide an aborted or red run underneath;\n- # accept the candidate only once neither signal objects.\n- if run_failures or job_failures:\n- return False\n if unread_comments:\n return False\n return True\ndiff --git a/tests/test_gate.py b/tests/test_gate.py\n--- a/tests/test_gate.py\n+++ b/tests/test_gate.py\n@@ -20,1 +20,10 @@ def test_build_is_mergeable_rejects_a_waiting_run():\n assert not build_is_mergeable(waiting_status(), [], [], [])\n+\n+\n+def test_build_is_mergeable_uses_the_shared_gating_predicate():\n+ status = settled_status(aborted_count=1)\n+ assert not build_is_mergeable(status, [], [], [])\n+\n+\n+def test_build_is_mergeable_still_rejects_a_job_failure(monkeypatch):\n+ assert not build_is_mergeable(settled_status(), [], [\"job-1\"], [])\n" } }, "change_contract": { - "goal": "Route the last inline copy of the failed-check policy through the shared `has_failed_checks` predicate that two other agreement sites already use.", + "goal": "Route the last inline copy of the gating-failure policy through the shared `has_gating_failure` predicate that two other agreement sites already use.", "acceptance_criteria": [ - "`candidate_is_clear` uses the shared predicate instead of its own inline conditions for cancelled and failed checks.", + "`build_is_mergeable` uses the shared predicate instead of its own inline conditions for aborted and red runs.", "Existing pass/fail behaviour is unchanged." ], "non_goals": [ - "Change what `has_failed_checks` itself considers a failure.", - "Change the pending-check condition." + "Change what `has_gating_failure` itself considers a failure.", + "Change the waiting-run condition." ], "preserved_behaviors": [ - "A pending check still rejects the candidate.", - "A clean, fully-terminal candidate with no new review items is still accepted." + "A waiting run still rejects the candidate.", + "A settled, fully green candidate with no unread comments is still accepted." ] }, "sources": { @@ -38,11 +38,11 @@ ], "named_documents": [ { - "label": "Watcher clear-candidate contract", + "label": "Watcher mergeability contract", "location": "docs/watcher-contract.md" }, { - "label": "Shared failed-check predicate", + "label": "Shared gating predicate", "location": "watcher/policy.py" } ], @@ -52,15 +52,15 @@ "location": "watcher/reconcile.py" }, { - "label": "Status tests", - "location": "tests/test_status.py" + "label": "Gate tests", + "location": "tests/test_gate.py" } ] }, "validation": [ { - "name": "status tests", - "command": "pytest tests/test_status.py", + "name": "gate tests", + "command": "pytest tests/test_gate.py", "scope": "focused", "status": "passed", "result": "9 passed" @@ -75,12 +75,12 @@ ], "context": { "data": [ - "`has_failed_checks(checks_summary, failed_runs, failed_jobs)` already exists in `watcher/policy.py` and is already called from two other agreement sites in `watcher/reconcile.py`.", - "This inline copy in `candidate_is_clear` predates the shared predicate and was never migrated when the predicate was introduced.", - "The same duplication class previously caused two separate incidents where one call site's copy drifted from another's and produced a self-contradictory clear/not-clear result for the same candidate." + "`has_gating_failure(status, run_failures, job_failures)` already exists in `watcher/policy.py` and is already called from two other agreement sites in `watcher/reconcile.py`.", + "This inline copy in `build_is_mergeable` predates the shared predicate and was never migrated when the predicate was introduced.", + "The same duplication class previously caused two separate incidents where one call site's copy drifted from another's and produced a self-contradictory result for the same candidate." ], "operational": [ - "`candidate_is_clear` is the gate a merge automation checks before acting, so a wrong answer here is user-visible." + "`build_is_mergeable` is the gate a merge automation checks before acting, so a wrong answer here is user-visible." ] } } From 41de65daadc5d53bfbb299cb4ecd6d040ac47ab9 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 15:44:31 -0700 Subject: [PATCH 10/11] fix: make the last two before-state and sanitization defects actually resolved Closes the blocking and strong-recommendation findings from the third and final scheduled review cycle on the combined s2+s3 candidate. ## The earlier "fix" relocated an incoherence instead of resolving it `setup-service-path-gateway`'s before-state was still impossible after the commit that claimed to fix it (limitation 24). That commit removed a no-op diff line, but left `SetupService.run`'s before-image calling `self._path_gateway.project_dir(workspace_id)` while `commands/setup.py`'s before-image constructed the service with a bare function (`SetupService(resolve_workspace_root)`) - a function has no `.project_dir` method, so the packet's own pre-image still could not execute. The defect had moved, not resolved. Fixed by making the before-image's `run` call the injected gateway directly as a callable (`self._path_gateway(workspace_id)`, which a bare function argument genuinely supports), and introducing the `.project_dir` wrapper method only in the after-image alongside the new gateway class. The before and after states are now both independently executable. ## One verbatim real term survived three prior review cycles `record-status-transition-guard` still carried `allow_failure=True`, the identical keyword argument name and value from the real `atelier` source commit (`79703c5`), even though every other identifier in the case had already been independently renamed across three prior cycles. Renamed to `raise_on_failure=False`. ## Recorded Limitation 24 is corrected: its "now fixed" language was premature and is now qualified rather than left standing as an overclaim. Limitation 30 records both defects plainly, including that a fix for an incoherent diff must be checked by asking whether the whole before-state can execute, not whether the one flagged line changed, and that a mechanical diff-against-real-source check - which does not exist today - would have caught both defects on the first pass instead of the fifth. ## Validation Both packet diffs re-verified to parse as patches. A comprehensive re-scan of every reviewer-visible field across all 8 new cases against every real identifier from all sources (four `atelier` PRs and this repository's own history) found nothing further; the only remaining real-source mentions are in private provenance `retention_authority` fields, which is the established, legitimate pattern (limitation 23). --- CHANGELOG.md | 22 +++--- review-suite/evals/baseline/v1/LIMITATIONS.md | 67 ++++++++++++++++--- .../packet.json | 2 +- .../setup-service-path-gateway/packet.json | 2 +- 4 files changed, 71 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06296cd..bc1285e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,25 +6,27 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: make the last two before-state and sanitization defects actually resolved - fix: sanitize the repository-history case and correct the changelog order + (`3d9fe4925c8908a311453c87ae740bfcf4de20bd`) - fix: reconcile records after folding s2 and s3 into one delivery - (`4a6319e37df5f55b91453d206d1bebc98441cb15`) + (`5070cf1bbea438e74149dfe0cf9b171a6f7cdb92`) - docs: record the code-simplicity delivery and close out corpus population - (`8fc442b2bbf8c77cfc846c0b6d7230845fe306d1`) + (`875091c32301eafd807d2d5a3e2b402e7ffaca53`) - feat: populate the code-simplicity stratum with four adjudicated cases - (`030042a0ee27f9da98413dfe49ed703904808f8b`) + (`f3c064a7bbaf3f89f7a6a5495846b254a54e9a0b`) - fix: sweep sanitization across every reviewer-visible field, not only the diff - (`fdbfdc33a252fe5c830bbc9b4e7c091fa46a441f`) + (`ab3921a904ec7835bc4d03ed40b7c8a28d12d2c1`) - fix: make every s2 packet internally consistent after the sanitization rename - (`5cb339d879aae87946b077fedb53fc3bab013dfd`) + (`1ec231bb17cd0c1db82258756aa7a78e8e7f63ab`) - fix: sanitize the solution-simplicity cases against source-vocabulary leakage - (`8e8a1fbf4efd7d97d6573c97b9257ed92bebe055`) -- feat: require acceptance evidence for workflow closeout - (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) + (`2b56c022c91a925b574a5748112e63bdcbbbf8f2`) - docs: record the solution-simplicity delivery and settle the grading method - (`3fb600c57e2547d437d3a1a8ed9e0ad4595186cc`) + (`da8f53b06072ba0380d01ce06fc4f4a324a6219e`) - feat: populate the solution-simplicity stratum with four adjudicated cases - (`df55335d2df85f0ba3064e671095d0621eeb145e`) + (`3105b8e84da78c691f4f93883f39887ff9ae784f`) +- feat: require acceptance evidence for workflow closeout + (`a3597c25ee2d76135d1f0c8642a620e673fc8e57`) - fix: make every packet diff a valid patch, and gate the adjudication record (`06a5679643a0a5bcb1944c8bff4bd4986f4f77e1`) - fix: stop a grader formulation being quotable from its own packet diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index 3934966..a63b202 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -574,16 +574,22 @@ not have had. This defect predates every sanitization commit; it was present in the very first draft and simply went unnoticed until a cycle checked the diff's internal coherence rather than its wording. -Both are now fixed. The lesson generalizes past this specific batch: **a -minimization or a rename must be swept across the whole packet - goal, -acceptance criteria, non-goals, preserved behaviors, repository instructions, -named documents, nearby patterns, and context - not only the diff and the -formulations that happen to be the field a grader reads.** A packet has many -prose fields, and a real term or a real reviewer's sentence can hide in any of -them. Nothing mechanical catches this today; it took three independent review -passes on one four-case stratum to find every instance, which is itself evidence -that curation discipline alone is not a durable defense and a future population -batch should expect the same scrutiny. +Both were fixed at the time this item was written, and the fix for the second +one did not fully hold: a later review cycle (recorded in item 30) found the +`setup-service-path-gateway` incoherence had only been relocated, not resolved, +by the commit this item originally credited. Read item 30 alongside this one +rather than trusting this item's own "now fixed" language in isolation - that +overclaim is itself part of the lesson. + +The lesson generalizes past this specific batch: **a minimization or a rename +must be swept across the whole packet - goal, acceptance criteria, non-goals, +preserved behaviors, repository instructions, named documents, nearby patterns, +and context - not only the diff and the formulations that happen to be the field +a grader reads.** A packet has many prose fields, and a real term or a real +reviewer's sentence can hide in any of them. Nothing mechanical catches this +today; it took four independent review passes across two strata to find every +instance, which is itself evidence that curation discipline alone is not a +durable defense and a future population batch should expect the same scrutiny. ## 25. The code-simplicity stratum has no oracle either @@ -674,3 +680,44 @@ authored rather than derived from the source. The general rule from limitation 21 stands unchanged and evidently was not sufficient on its own to prevent a repeat instance three batches later - a mechanical check remains the more durable fix, and remains undone. + +## 30. A fix that relocates an inconsistency is not a fix, and one more verbatim term survived three prior sweeps + +The third and final scheduled review cycle on the combined s2+s3 candidate found +two more real defects, after two prior cycles had each found and fixed real +defects in the same two cases. + +**`setup-service-path-gateway`'s before-state was still impossible**, in a +different place than the no-op line item 24 credited itself with fixing. The +earlier fix removed the no-op diff line, but left `SetupService.run`'s +before-image calling `self._path_gateway.project_dir(workspace_id)` while +`commands/setup.py`'s before-image constructed `SetupService` with a bare +function (`SetupService(resolve_workspace_root)`) - a function has no +`.project_dir` method, so the pre-image still could not run. The incoherence had +moved from the constructor call to the method-call pattern; it was never +actually resolved. Fixed now by making `run`'s before-image call the injected +gateway directly as a callable (`self._path_gateway(workspace_id)`), which is +exactly what a bare function argument supports, and introducing the +`.project_dir` method only in the after-image alongside the new wrapper class. + +**`record-status-transition-guard` still carried one verbatim real term**: +`allow_failure=True`, the identical keyword argument name and value from the +real `atelier` source commit (`79703c5`), through three prior review cycles that +each rewrote other parts of the same case. Renamed to `raise_on_failure=False`. + +Two things are worth stating plainly rather than smoothing over. First, the "now +fixed" language in item 24 was premature: it described one relocated symptom as +resolved. A fix for an incoherent diff must be checked by asking whether the +*whole* before-state can actually execute, not whether the one line a reviewer +flagged has changed. Second, one leaked keyword argument survived being read by +four independent review passes across two different cases before a fifth pass +caught it - not because it was hidden, but because reviewers (including this +ticket's own repeated sweeps) tend to check the same categories of surface +(function names, class names, prose) and can each independently miss a keyword +argument buried inside an otherwise-fully-rewritten call. Curation discipline +caught this eventually, but "eventually, after five passes" is not a durable +property of a curation process, and a mechanical diff-against-real-source check +\- comparing every packet's diff token-for-token against its cited real source +commit or PR patch - would have caught both defects on the first pass. That +check does not exist today and is recorded here as unfinished work rather than +implied solved by the fact that this batch, eventually, got there. diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json index 9e555cf..2bf7ec9 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/record-status-transition-guard/packet.json @@ -10,7 +10,7 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/planner/create_task.py b/planner/create_task.py\n--- a/planner/create_task.py\n+++ b/planner/create_task.py\n@@ -85,5 +85,24 @@ def main() -> None:\n if not created.ok:\n print(\"error: failed to create task record\", file=sys.stderr)\n raise SystemExit(1)\n \n- tracker.run_cli_command([\"update\", created.record_id, \"--status\", \"deferred\"])\n+ status_applied = _apply_deferred_status_with_retry(created.record_id)\n+ if not status_applied:\n+ tracker.run_cli_command([\"close\", created.record_id, \"--reason\",\n+ \"deferred status could not be applied\"])\n+ print(\n+ f\"error: created {created.record_id} but could not set deferred status; \"\n+ \"auto-closed to avoid an unexpectedly runnable record\",\n+ file=sys.stderr,\n+ )\n+ raise SystemExit(1)\n+\n+\n+def _apply_deferred_status_with_retry(record_id, attempts=3):\n+ for _ in range(attempts):\n+ result = tracker.run_cli_command(\n+ [\"update\", record_id, \"--status\", \"deferred\"], allow_failure=True,\n+ )\n+ if result.ok:\n+ return True\n+ return False\ndiff --git a/tests/test_create_task.py b/tests/test_create_task.py\n--- a/tests/test_create_task.py\n+++ b/tests/test_create_task.py\n@@ -14,1 +14,17 @@ def test_main_sets_the_task_to_deferred():\n assert cli_calls[-1] == [\"update\", \"t-1\", \"--status\", \"deferred\"]\n+\n+\n+def test_main_auto_closes_when_the_deferred_status_never_applies():\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert cli_calls[-1][0] == \"close\"\n+\n+\n+def test_main_does_not_publish_after_the_auto_close_path(monkeypatch):\n+ published = []\n+ monkeypatch.setattr(publisher, \"publish\", lambda *a, **k: published.append(a))\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert published == []\n" + "content": "diff --git a/planner/create_task.py b/planner/create_task.py\n--- a/planner/create_task.py\n+++ b/planner/create_task.py\n@@ -85,5 +85,24 @@ def main() -> None:\n if not created.ok:\n print(\"error: failed to create task record\", file=sys.stderr)\n raise SystemExit(1)\n \n- tracker.run_cli_command([\"update\", created.record_id, \"--status\", \"deferred\"])\n+ status_applied = _apply_deferred_status_with_retry(created.record_id)\n+ if not status_applied:\n+ tracker.run_cli_command([\"close\", created.record_id, \"--reason\",\n+ \"deferred status could not be applied\"])\n+ print(\n+ f\"error: created {created.record_id} but could not set deferred status; \"\n+ \"auto-closed to avoid an unexpectedly runnable record\",\n+ file=sys.stderr,\n+ )\n+ raise SystemExit(1)\n+\n+\n+def _apply_deferred_status_with_retry(record_id, attempts=3):\n+ for _ in range(attempts):\n+ result = tracker.run_cli_command(\n+ [\"update\", record_id, \"--status\", \"deferred\"], raise_on_failure=False,\n+ )\n+ if result.ok:\n+ return True\n+ return False\ndiff --git a/tests/test_create_task.py b/tests/test_create_task.py\n--- a/tests/test_create_task.py\n+++ b/tests/test_create_task.py\n@@ -14,1 +14,17 @@ def test_main_sets_the_task_to_deferred():\n assert cli_calls[-1] == [\"update\", \"t-1\", \"--status\", \"deferred\"]\n+\n+\n+def test_main_auto_closes_when_the_deferred_status_never_applies():\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert cli_calls[-1][0] == \"close\"\n+\n+\n+def test_main_does_not_publish_after_the_auto_close_path(monkeypatch):\n+ published = []\n+ monkeypatch.setattr(publisher, \"publish\", lambda *a, **k: published.append(a))\n+ with failing_status_updates():\n+ with pytest.raises(SystemExit):\n+ main()\n+ assert published == []\n" } }, "change_contract": { diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json index 3cfee7c..9bf3206 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/setup-service-path-gateway/packet.json @@ -10,7 +10,7 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,10 +1,24 @@\n from paths import resolve_workspace_root\n \n \n+class SetupPathGateway:\n+ \\\"\\\"\\\"Indirection over `paths.resolve_workspace_root` for the setup service.\\\"\\\"\\\"\n+\n+ def project_dir(self, workspace_id):\n+ return resolve_workspace_root(workspace_id)\n+\n+\n+class SetupDependencies:\n+ \\\"\\\"\\\"Dependency bag threaded into `SetupService`.\\\"\\\"\\\"\n+\n+ def __init__(self, path_gateway):\n+ self.path_gateway = path_gateway\n+\n+\n class SetupService:\n- def __init__(self, path_gateway):\n- self._path_gateway = path_gateway\n+ def __init__(self, deps: SetupDependencies):\n+ self._deps = deps\n \n- def run(self, workspace_id):\n- target = self._path_gateway.project_dir(workspace_id)\n+ def run(self, workspace_id):\n+ target = self._deps.path_gateway.project_dir(workspace_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,11 +1,10 @@\n-from services.setup import SetupService\n-from paths import resolve_workspace_root\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService(resolve_workspace_root)\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(workspace_id):\n service = _build_setup_service()\n return service.run(workspace_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"w-1\": \"/workspaces/w-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+ assert gateway.calls == [\"w-1\"]\n" + "content": "diff --git a/services/setup.py b/services/setup.py\n--- a/services/setup.py\n+++ b/services/setup.py\n@@ -1,10 +1,24 @@\n from paths import resolve_workspace_root\n \n \n+class SetupPathGateway:\n+ \\\"\\\"\\\"Indirection over `paths.resolve_workspace_root` for the setup service.\\\"\\\"\\\"\n+\n+ def project_dir(self, workspace_id):\n+ return resolve_workspace_root(workspace_id)\n+\n+\n+class SetupDependencies:\n+ \\\"\\\"\\\"Dependency bag threaded into `SetupService`.\\\"\\\"\\\"\n+\n+ def __init__(self, path_gateway):\n+ self.path_gateway = path_gateway\n+\n+\n class SetupService:\n- def __init__(self, path_gateway):\n- self._path_gateway = path_gateway\n+ def __init__(self, deps: SetupDependencies):\n+ self._deps = deps\n \n- def run(self, workspace_id):\n- target = self._path_gateway(workspace_id)\n+ def run(self, workspace_id):\n+ target = self._deps.path_gateway.project_dir(workspace_id)\n return self._prepare(target)\ndiff --git a/commands/setup.py b/commands/setup.py\n--- a/commands/setup.py\n+++ b/commands/setup.py\n@@ -1,11 +1,10 @@\n-from services.setup import SetupService\n-from paths import resolve_workspace_root\n+from services.setup import SetupDependencies, SetupPathGateway, SetupService\n \n \n def _build_setup_service():\n- return SetupService(resolve_workspace_root)\n+ return SetupService(SetupDependencies(path_gateway=SetupPathGateway()))\n \n \n def run(workspace_id):\n service = _build_setup_service()\n return service.run(workspace_id)\ndiff --git a/tests/test_setup_service.py b/tests/test_setup_service.py\n--- a/tests/test_setup_service.py\n+++ b/tests/test_setup_service.py\n@@ -8,1 +8,8 @@ def test_run_prepares_the_resolved_target():\n assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+\n+\n+def test_run_uses_the_injected_path_gateway():\n+ gateway = FakePathGateway(paths={\"w-1\": \"/workspaces/w-1\"})\n+ service = SetupService(SetupDependencies(path_gateway=gateway))\n+ assert service.run(\"w-1\") == prepared(\"/workspaces/w-1\")\n+ assert gateway.calls == [\"w-1\"]\n" } }, "change_contract": { From c7a80c0e05ea76c0a7626c02dbf0b1605da37739 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Mon, 27 Jul 2026 16:28:18 -0700 Subject: [PATCH 11/11] fix: correct a stale reference, a stale validation entry, and an inverted case Closes the correctness findings from the fifth review cycle - the first entirely on the rebased head, with no case content changed since the fourth. ## Two stale references from earlier fixes - `reconciliation-outcome-type`'s accepted non-finding still named the pre-rename enum members `BLOCKED_AMBIGUOUS`/`FAILED`, after the sanitization pass (item 21) had already renamed them to `AMBIGUOUS_HOLD`/`WRITE_FAILED` everywhere else in the case. - `registry-client-layering`'s focused validation entry still cited `pytest tests/test_worker.py`, left over from before the case was rebuilt as a purely additive diff (item 20's fix) touching only `tests/test_client.py`. The cited evidence didn't test the change the packet actually shows. ## A case whose own diff contradicted its own declared verdict `watcher-check-policy-duplication`'s diff showed the real fix - removing the last inline duplicate of a shared policy - which is what a clean diff looks like, while the case declared `expected_verdict: changes_required` and a root cause arguing the diff itself was the correction. A contract-faithful reviewer given this packet would correctly say `clean`, and the case would have scored that correct answer as a miss. This was a design error present since the case was first authored, not a wording slip: three full review cycles and one rebase-focused cycle each checked identifiers, terms, and diff coherence in isolation, and none of them asked whether the packet's own diagnosis of itself matched the diff shown. Rebuilt so the diff instead adds a new call site (`build_is_archivable`) that duplicates the shared predicate's check inline rather than calling it - the failure shape the source commit's own drift history actually warns against - which is both internally consistent and a better fit for what a local code-complexity escape case needs to demonstrate. Expectation and provenance updated to match. ## Recorded Limitation 31: sweeping a rename or fix across every field is necessary but not sufficient. Each case also needs a pass that asks, independent of wording, whether reading only the diff would produce the verdict the case declares. Nothing mechanical checks this today, and five review cycles across two strata each finding a real defect is recorded as evidence that curation discipline alone does not converge. ## Validation Both stale references corrected. The redesigned case's diff re-verified to parse as a patch. A full leak re-scan across the stratum found nothing further. --- CHANGELOG.md | 2 + review-suite/evals/baseline/v1/LIMITATIONS.md | 49 +++++++++++++++++++ .../reconciliation-outcome-type.json | 2 +- .../registry-client-layering/packet.json | 6 +-- .../watcher-check-policy-duplication.json | 24 ++++----- .../watcher-check-policy-duplication.json | 4 +- .../packet.json | 35 +++++++------ 7 files changed, 86 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc1285e..1135fca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-27 — Populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, and recovered carved suffixes +- fix: correct a stale reference, a stale validation entry, and an inverted case - fix: make the last two before-state and sanitization defects actually resolved + (`41de65daadc5d53bfbb299cb4ecd6d040ac47ab9`) - fix: sanitize the repository-history case and correct the changelog order (`3d9fe4925c8908a311453c87ae740bfcf4de20bd`) - fix: reconcile records after folding s2 and s3 into one delivery diff --git a/review-suite/evals/baseline/v1/LIMITATIONS.md b/review-suite/evals/baseline/v1/LIMITATIONS.md index a63b202..1641424 100644 --- a/review-suite/evals/baseline/v1/LIMITATIONS.md +++ b/review-suite/evals/baseline/v1/LIMITATIONS.md @@ -721,3 +721,52 @@ property of a curation process, and a mechanical diff-against-real-source check commit or PR patch - would have caught both defects on the first pass. That check does not exist today and is recorded here as unfinished work rather than implied solved by the fact that this batch, eventually, got there. + +## 31. A packet's own narrative must agree with its own diff, verdict, and evidence + +A fifth review cycle - the first entirely on the rebased head, with no case +content changed since the fourth - still found three real defects by reading the +fixture content directly rather than trusting the passing automated checks, none +of which check narrative or evidentiary self-consistency: + +- **A stale symbol reference survived a rename.** + `reconciliation-outcome-type`'s accepted non-finding still named + `BLOCKED_AMBIGUOUS`/`FAILED`, the pre-rename enum members, after the case's + sanitization pass (item 21) had already renamed them to + `AMBIGUOUS_HOLD`/`WRITE_FAILED` everywhere else. One more instance of the + sweep-every-field lesson (item 24), in a field none of the prior four cycles + happened to check. +- **A validation entry named the wrong test file.** `registry-client-layering`'s + focused validation cited `pytest tests/test_worker.py`, left over from an + earlier, larger version of the case before it was rebuilt as a purely additive + diff (item 20's fix) touching only `tests/test_client.py`. The evidence didn't + test the change the packet actually shows. +- **A case's polarity was inverted.** `watcher-check-policy-duplication`'s diff + showed the real fix - removing the last inline duplicate of a shared policy - + which is what a clean diff looks like, while the case declared + `expected_verdict: changes_required` and a root cause arguing the diff itself + was the correction. A reviewer given this packet would be right to say + `clean`, and the case would have scored that correct answer as a miss. Rebuilt + so the diff instead *adds* a new call site that duplicates the shared + predicate inline rather than calling it - the failure shape the source + commit's own drift history actually warns against - which is both internally + consistent and closer in spirit to what a "local code-complexity escape" case + needs to demonstrate. + +The third of these is the more serious one: it was not a stale reference but a +logic error in the case's own design, present since the case was first authored +and undetected through three full review cycles plus one rebase-focused cycle, +because every one of those checked identifiers, terms, and diff coherence in +isolation without checking whether the packet's *diagnosis of itself* - what +verdict and root cause its own diff should produce - was actually consistent +with the diff shown. + +The general lesson this adds to items 21, 24, and 29: **sweeping a rename or a +sanitization fix across every field is necessary but not sufficient. Each case +also needs one pass that asks, independent of any wording concern, "if I read +only this diff, what verdict would I reach, and does it match what the case +declares?"** Nothing mechanical asks that question today. Five review cycles +across two strata found a defect at every prior stopping point; that is the +strongest evidence yet in this record that curation discipline alone does not +converge, and that a mechanical diff-to-expectation consistency check remains +the more durable fix, still undone. diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json index 3002bcc..8761ff1 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/private/expectations/reconciliation-outcome-type.json @@ -18,7 +18,7 @@ }, { "id": "anf.four-member-enum-two-used-by-caller", - "description": "Only two of the four `SyncOutcome` members (`BLOCKED_AMBIGUOUS`, `FAILED`) are branched on by the startup caller shown here. Worth noting; the packet does not claim every consumer is shown.", + "description": "Only two of the four `SyncOutcome` members (`AMBIGUOUS_HOLD`, `WRITE_FAILED`) are branched on by the startup caller shown here. Worth noting; the packet does not claim every consumer is shown.", "equivalent_formulations": [ "not every enum member is handled here", "only two outcomes are branched on" diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json index c8d47f8..4a58f65 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/reviewer/registry-client-layering/packet.json @@ -58,11 +58,11 @@ }, "validation": [ { - "name": "worker tests", - "command": "pytest tests/test_worker.py", + "name": "client tests", + "command": "pytest tests/test_client.py", "scope": "focused", "status": "passed", - "result": "5 passed" + "result": "6 passed" }, { "name": "full tests", diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json index da0ba65..08988d8 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/expectations/watcher-check-policy-duplication.json @@ -7,27 +7,27 @@ "material_root_causes": [ { "id": "rc.local-policy-duplicated-across-call-sites", - "requirement": "A policy predicate with more than one call site must have exactly one implementation.", - "trigger": "A remaining inline copy of a policy that a shared predicate now implements.", - "surface": "build_is_mergeable", - "consequence": "The packet's own diff replaces the inline copy with the shared predicate, which is the correction this case demonstrates: before this change, three call sites decided the same failed-check question with two different implementations, and the packet's own history states that exact drift previously produced a self-contradictory clear/not-clear result on the same candidate. Reviewing this diff for local complexity means recognising that the change is the fix, and that the requirement is precisely what motivated it.", + "requirement": "A new call site that needs a gating question an existing shared predicate already answers must call that predicate rather than re-deriving it inline.", + "trigger": "Adding a new predicate that needs the same gating question an existing shared predicate already answers.", + "surface": "build_is_archivable", + "consequence": "The new `build_is_archivable` re-derives the aborted-or-red-or-failed-run question inline instead of calling `has_gating_failure`, which `build_is_mergeable` in the same module already uses for exactly this question. The packet's own history states that this class of inline duplication previously drifted between two call sites and produced a self-contradictory result for the same candidate - the risk this diff reintroduces by adding a second inline copy instead of calling the existing shared predicate.", "severity": "strong_recommendation", "equivalent_formulations": [ - "the inline copy duplicates the shared predicate", - "this call site had its own copy of the failed-check policy", - "the same policy is implemented twice", - "route this through the existing shared predicate", - "this duplication previously caused drift between call sites" + "this re-derives the gating check instead of calling the shared predicate", + "build_is_archivable duplicates has_gating_failure inline", + "the same policy is now implemented in two places again", + "call has_gating_failure instead of repeating its logic", + "this reintroduces the duplication the shared predicate was built to remove" ] } ], "accepted_non_findings": [ { "id": "anf.pending-check-inline", - "description": "The waiting-run condition (`status[\"waiting_count\"] > 0`) stays inline rather than moving into the shared predicate. A reviewer may ask why it was not folded in too; the packet's non-goals state that changing the waiting condition is out of scope, and it is not part of the duplication the shared predicate was built to resolve.", + "description": "The retention-window check (`retention_days < MIN_RETENTION_DAYS`) stays inline rather than moving into a shared helper. A reviewer may ask why; the packet's non-goals state that archive-specific logic is out of scope for centralisation, and it is not part of the gating-failure duplication this case demonstrates.", "equivalent_formulations": [ - "the waiting check is still inline", - "why wasn't the waiting condition also shared" + "the retention check is inline", + "why isn't the retention window also shared" ] } ] diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json index 9efa433..d23954a 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/private/provenance/watcher-check-policy-duplication.json @@ -3,10 +3,10 @@ "case_id": "watcher-check-policy-duplication", "origin": "repository_history", "retention_authority": "This repository's own delivery history: commit 93516194388116f4841fc191a8c78c191d0da5b1, whose message records that the code-simplicity lens itself flagged the last remaining inline copy of the failed-check policy, and that the same duplication class had twice before produced self-contradictory watcher states. Public, owner-authored, no retention question.", - "sanitization": "Rewritten from scratch against a fictional watch-tool subject rather than this suite's own PR-watching code. The first draft of this case only lightly renamed the two function names while keeping every local variable name, every dict key, and the removed code comment verbatim from the real source; that draft was caught by review and replaced. Every function name, local variable, dict key, and comment is now independently authored rather than derived from the source, so the case cannot be recognised as or confused with this repository's own implementation and no source identifier, path, symbol, prose, or diff is copied.", + "sanitization": "Rewritten from scratch against a fictional watch-tool subject rather than this suite's own PR-watching code. The first draft only lightly renamed the two function names while keeping every local variable name, every dict key, and the removed code comment verbatim from the real source; that draft was caught by review and replaced. A second review then found the rewritten diff had inverted the case's own polarity: it showed the real fix (removing the last inline duplicate) rather than a diff a reviewer should flag, so its own material root cause and expected verdict contradicted the diff it described. Rebuilt so the diff instead adds a new call site that duplicates the shared predicate's check inline, which is the failure shape the drift history in this commit's message actually warns against. Every function name, local variable, dict key, and comment is independently authored rather than derived from the source.", "recorded_at": "2026-07-27", "adjudication": { - "first": "This repository's commit 93516194388116f4841fc191a8c78c191d0da5b1: the code-simplicity lens found and this fix resolved the duplication, with the drift history stated in the commit message.", + "first": "This repository's commit 93516194388116f4841fc191a8c78c191d0da5b1: the code-simplicity lens found and this commit resolved an inline duplicate of a shared policy, with a stated history of that duplication class causing real drift. This case models a new instance of the same duplication class the commit's own message warns against, rather than reproducing the commit's diff directly.", "second": "owner_required", "notes": "No executable oracle exists for a local-complexity judgement like this. Recommended adjudication: MATERIAL. The packet states two other call sites already use the shared predicate and that this exact duplication class had already caused real drift twice, which are concrete, checkable facts rather than a stylistic preference for less code. Residual risk for the owner: the severity is recorded as strong_recommendation rather than blocking, since the packet shows no currently observed drift, only a stated history of it; a reviewer could reasonably argue for blocking severity given that history, and the owner should decide which is right." } diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json index 079ce74..1d79cad 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json @@ -10,22 +10,21 @@ "diff": { "format": "unified_diff", "complete": true, - "content": "diff --git a/watcher/gate.py b/watcher/gate.py\n--- a/watcher/gate.py\n+++ b/watcher/gate.py\n@@ -1058,13 +1058,9 @@ def build_is_mergeable(status, run_failures, job_failures, unread_comments):\n if not status[\"fully_settled\"]:\n return False\n- if status[\"red_count\"] > 0 or status[\"waiting_count\"] > 0:\n+ if status[\"waiting_count\"] > 0:\n return False\n- if int(status.get(\"aborted_count\") or 0) > 0:\n+ if has_gating_failure(status, run_failures, job_failures):\n return False\n- # A settled status view can still hide an aborted or red run underneath;\n- # accept the candidate only once neither signal objects.\n- if run_failures or job_failures:\n- return False\n if unread_comments:\n return False\n return True\ndiff --git a/tests/test_gate.py b/tests/test_gate.py\n--- a/tests/test_gate.py\n+++ b/tests/test_gate.py\n@@ -20,1 +20,10 @@ def test_build_is_mergeable_rejects_a_waiting_run():\n assert not build_is_mergeable(waiting_status(), [], [], [])\n+\n+\n+def test_build_is_mergeable_uses_the_shared_gating_predicate():\n+ status = settled_status(aborted_count=1)\n+ assert not build_is_mergeable(status, [], [], [])\n+\n+\n+def test_build_is_mergeable_still_rejects_a_job_failure(monkeypatch):\n+ assert not build_is_mergeable(settled_status(), [], [\"job-1\"], [])\n" + "content": "diff --git a/watcher/gate.py b/watcher/gate.py\n--- a/watcher/gate.py\n+++ b/watcher/gate.py\n@@ -1058,9 +1058,22 @@ def build_is_mergeable(status, run_failures, job_failures, unread_comments):\n if not status[\"fully_settled\"]:\n return False\n if status[\"waiting_count\"] > 0:\n return False\n if has_gating_failure(status, run_failures, job_failures):\n return False\n if unread_comments:\n return False\n return True\n+\n+\n+def build_is_archivable(status, run_failures, job_failures, retention_days):\n+ \\\"\\\"\\\"Return whether a settled candidate is old enough to archive.\\\"\\\"\\\"\n+ if not status[\"fully_settled\"]:\n+ return False\n+ if int(status.get(\"aborted_count\") or 0) > 0 or int(status.get(\"red_count\") or 0) > 0:\n+ return False\n+ if run_failures or job_failures:\n+ return False\n+ if retention_days < MIN_RETENTION_DAYS:\n+ return False\n+ return True\ndiff --git a/tests/test_gate.py b/tests/test_gate.py\n--- a/tests/test_gate.py\n+++ b/tests/test_gate.py\n@@ -20,1 +20,11 @@ def test_build_is_mergeable_rejects_a_waiting_run():\n assert not build_is_mergeable(waiting_status(), [], [], [])\n+\n+\n+def test_build_is_archivable_rejects_a_red_run():\n+ status = settled_status(red_count=1)\n+ assert not build_is_archivable(status, [], [], MIN_RETENTION_DAYS)\n+\n+\n+def test_build_is_archivable_rejects_a_short_retention_window(monkeypatch):\n+ status = settled_status()\n+ assert not build_is_archivable(status, [], [], MIN_RETENTION_DAYS - 1)\n" } }, "change_contract": { - "goal": "Route the last inline copy of the gating-failure policy through the shared `has_gating_failure` predicate that two other agreement sites already use.", + "goal": "Add an archive-eligibility check alongside the existing merge-eligibility check, so a settled candidate can be archived once its retention window has passed.", "acceptance_criteria": [ - "`build_is_mergeable` uses the shared predicate instead of its own inline conditions for aborted and red runs.", - "Existing pass/fail behaviour is unchanged." + "`build_is_archivable` rejects a candidate with a red or aborted run.", + "`build_is_archivable` rejects a candidate whose retention window has not yet passed." ], "non_goals": [ - "Change what `has_gating_failure` itself considers a failure.", - "Change the waiting-run condition." + "Change `build_is_mergeable`'s own behaviour.", + "Change what counts as a red or aborted run." ], "preserved_behaviors": [ - "A waiting run still rejects the candidate.", - "A settled, fully green candidate with no unread comments is still accepted." + "`build_is_mergeable`'s existing behaviour and its call to the shared predicate are unchanged." ] }, "sources": { @@ -33,12 +32,12 @@ { "label": "Shared-policy rules", "location": "AGENTS.md", - "summary": "A policy predicate with more than one call site must have exactly one implementation. An inline copy at a remaining call site is a defect once the shared version exists, not a stylistic choice." + "summary": "A policy predicate with more than one call site must have exactly one implementation. A new call site that needs the same gating question must call the existing shared predicate rather than re-deriving it inline." } ], "named_documents": [ { - "label": "Watcher mergeability contract", + "label": "Watcher archive contract", "location": "docs/watcher-contract.md" }, { @@ -48,8 +47,8 @@ ], "nearby_patterns": [ { - "label": "Two call sites already using the shared predicate", - "location": "watcher/reconcile.py" + "label": "Existing caller of the shared predicate", + "location": "watcher/gate.py:build_is_mergeable" }, { "label": "Gate tests", @@ -63,24 +62,24 @@ "command": "pytest tests/test_gate.py", "scope": "focused", "status": "passed", - "result": "9 passed" + "result": "11 passed" }, { "name": "full tests", "command": "pytest", "scope": "full", "status": "passed", - "result": "211 passed" + "result": "213 passed" } ], "context": { "data": [ - "`has_gating_failure(status, run_failures, job_failures)` already exists in `watcher/policy.py` and is already called from two other agreement sites in `watcher/reconcile.py`.", - "This inline copy in `build_is_mergeable` predates the shared predicate and was never migrated when the predicate was introduced.", - "The same duplication class previously caused two separate incidents where one call site's copy drifted from another's and produced a self-contradictory result for the same candidate." + "`has_gating_failure(status, run_failures, job_failures)` already exists in `watcher/policy.py` and is already called from `build_is_mergeable`, the only other predicate in this module.", + "This diff's new `build_is_archivable` needs exactly the same aborted-or-red-or-failed-run question that `has_gating_failure` already answers, but re-derives it inline instead of calling the shared predicate.", + "A prior incident in this module saw one call site's inline copy of this exact policy drift from another's and produce a self-contradictory result for the same candidate, which is why the shared predicate was introduced in the first place." ], "operational": [ - "`build_is_mergeable` is the gate a merge automation checks before acting, so a wrong answer here is user-visible." + "`build_is_archivable` gates an automation that deletes retained run data, so a wrong answer here is also user-visible." ] } }