diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e69dba..1135fca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,33 @@ 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 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 + (`5070cf1bbea438e74149dfe0cf9b171a6f7cdb92`) +- docs: record the code-simplicity delivery and close out corpus population + (`875091c32301eafd807d2d5a3e2b402e7ffaca53`) +- feat: populate the code-simplicity stratum with four adjudicated cases + (`f3c064a7bbaf3f89f7a6a5495846b254a54e9a0b`) +- fix: sweep sanitization across every reviewer-visible field, not only the diff + (`ab3921a904ec7835bc4d03ed40b7c8a28d12d2c1`) +- fix: make every s2 packet internally consistent after the sanitization rename + (`1ec231bb17cd0c1db82258756aa7a78e8e7f63ab`) +- fix: sanitize the solution-simplicity cases against source-vocabulary leakage + (`2b56c022c91a925b574a5748112e63bdcbbbf8f2`) +- docs: record the solution-simplicity delivery and settle the grading method + (`da8f53b06072ba0380d01ce06fc4f4a324a6219e`) +- feat: populate the solution-simplicity stratum with four adjudicated cases + (`3105b8e84da78c691f4f93883f39887ff9ae784f`) - 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..344e706 100644 --- a/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md +++ b/review-suite/evals/baseline/v1/ADJUDICATION-PLAN.md @@ -191,6 +191,40 @@ 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. + +## 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 | **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. + ## 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..1641424 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 @@ -288,41 +296,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 +427,346 @@ 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. + +## 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. + +## 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. + +## 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 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 + +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. + +## 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. + +## 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. + +## 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. + +## 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/baseline/v1/SOURCING.md b/review-suite/evals/baseline/v1/SOURCING.md index 4190a09..030324d 100644 --- a/review-suite/evals/baseline/v1/SOURCING.md +++ b/review-suite/evals/baseline/v1/SOURCING.md @@ -176,23 +176,86 @@ 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 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 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. 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 +**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 — **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 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: +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 @@ -206,7 +269,14 @@ 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. +- 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 3b83876..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_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": "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", @@ -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,12 +100,21 @@ "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", - "state": "declared_unpopulated", + "state": "populated_not_scored", "target_skill": "review-code-simplicity", "target_skill_dependencies": [], "closure_documents": 2, @@ -125,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", @@ -169,8 +187,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 (4) and s3-code-simplicity-lens (4). No oracle exists for either subject; recommended dispositions are recorded per case." ], "pilot_reports": [ { @@ -211,6 +228,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..584d773 100644 --- a/review-suite/evals/strata/README.md +++ b/review-suite/evals/strata/README.md @@ -125,7 +125,32 @@ 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` 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 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..8761ff1 --- /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 `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 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 `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/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..a25f58f --- /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 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", + "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", + "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 new file mode 100644 index 0000000..ca98f85 --- /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 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 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", + "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" + ] + } + ], + "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..e4c01c5 --- /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`, `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", + "equivalent_formulations": [ + "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" + ] + } + ], + "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..887b3de --- /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, 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.", + "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..140151f --- /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 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.", + "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..2fd5ee1 --- /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, 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.", + "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..3773bbb --- /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 _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": { + "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, `_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": [ + "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..2bf7ec9 --- /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/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": { + "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 record is auto-closed rather than left runnable.", + "The auto-close path does not publish the record." + ], + "non_goals": [ + "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 CLI update call." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Deferred-by-default rules", + "location": "AGENTS.md", + "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": [ + { + "label": "Task creation contract", + "location": "docs/task-creation.md" + } + ], + "nearby_patterns": [ + { + "label": "Sibling project-creation script with the same protection", + "location": "planner/create_project.py" + }, + { + "label": "Creation tests", + "location": "tests/test_create_task.py" + } + ] + }, + "validation": [ + { + "name": "creation tests", + "command": "pytest tests/test_create_task.py", + "scope": "focused", + "status": "passed", + "result": "9 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "154 passed" + } + ], + "context": { + "data": [ + "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 project-creation script already carries the identical retry-then-auto-close protection, added in the same release." + ], + "operational": [ + "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 new file mode 100644 index 0000000..4a58f65 --- /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,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": "Add queue message operations (enqueue, drain) to the registry client.", + "acceptance_criteria": [ + "Queue messages can be enqueued and drained through the registry client.", + "The new operations reach the registry through the existing transport." + ], + "non_goals": [ + "Change record mutation behaviour.", + "Change the CLI transport itself." + ], + "preserved_behaviors": [ + "`_RecordMutationOps` and `_TransportSession` are unchanged." + ] + }, + "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": "Existing bound transport session", + "location": "registry/client.py:_TransportSession" + }, + { + "label": "Existing record mutation client", + "location": "registry/client.py:_RecordMutationOps" + }, + { + "label": "Client tests", + "location": "tests/test_client.py" + } + ] + }, + "validation": [ + { + "name": "client tests", + "command": "pytest tests/test_client.py", + "scope": "focused", + "status": "passed", + "result": "6 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "176 passed" + } + ], + "context": { + "data": [ + "`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." + ] + } +} 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..9bf3206 --- /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,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": { + "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 workspace.", + "The resolution is testable without touching the filesystem." + ], + "non_goals": [ + "Change what `_prepare` does with the resolved target.", + "Change how a workspace id maps to a directory." + ], + "preserved_behaviors": [ + "`SetupService`'s public `run(workspace_id)` signature is unchanged." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Service tier rules", + "location": "AGENTS.md", + "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": [ + { + "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.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." + ], + "operational": [ + "This service runs once per `setup` invocation; it is not on a hot path." + ] + } +} 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..08988d8 --- /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 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": [ + "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 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 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/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..d23954a --- /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. 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 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/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..1d79cad --- /dev/null +++ b/review-suite/evals/strata/s3-code-simplicity-lens/reviewer/watcher-check-policy-duplication/packet.json @@ -0,0 +1,85 @@ +{ + "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/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": "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_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 `build_is_mergeable`'s own behaviour.", + "Change what counts as a red or aborted run." + ], + "preserved_behaviors": [ + "`build_is_mergeable`'s existing behaviour and its call to the shared predicate are unchanged." + ] + }, + "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. 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 archive contract", + "location": "docs/watcher-contract.md" + }, + { + "label": "Shared gating predicate", + "location": "watcher/policy.py" + } + ], + "nearby_patterns": [ + { + "label": "Existing caller of the shared predicate", + "location": "watcher/gate.py:build_is_mergeable" + }, + { + "label": "Gate tests", + "location": "tests/test_gate.py" + } + ] + }, + "validation": [ + { + "name": "gate tests", + "command": "pytest tests/test_gate.py", + "scope": "focused", + "status": "passed", + "result": "11 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "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 `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_archivable` gates an automation that deletes retained run data, so a wrong answer here is also user-visible." + ] + } +}