From f3523cc8ac134bb197d6632641a5949600ffa400 Mon Sep 17 00:00:00 2001 From: Fixture Date: Mon, 24 Aug 2026 06:39:59 -0700 Subject: [PATCH] refactor(premerge): apply the Reuse/Quality/Efficiency lens pass to the stack Three code-simplifier lenses over #768's diff returned 41 findings. Only the behaviour-preserving ones are applied here; everything that would change what the code does is filed instead. Guards that were vacuous, each demonstrated before the fix: * C11 greps a whole card for `StructuredOutput` to prove the body documents the dual-channel return. #763 put that literal into the `tools:` frontmatter of all eight cards, so the frontmatter satisfied it. Stripping the stanza from all eight bodies still passed; now scoped to `## Output` and it reds. * model-routing keyed "declares tools:" on bare key presence, so a key with an empty value counted as a declaration. Emptying one on a live card left the suite at PASS (383 checks) - the exact state #749 exists to red on. Now matches testers-agent-contract's semantics. * ci-shard-diagnose transcribed the harness's membership rule instead of running it. On a plan naming a fixture no invocation offers it reported `MATCH match=12 miss=0` for a shard that reds 11/12. It now carves the job's own run_one out of the live workflow and executes it. * The premerge Constants cap had a name-grep where its two siblings had value locks; it passed at any value. Five mutations now red it. * The unpinned sort/comm detector's narrow twin, plus a bare-terminator mutant row so the widening is itself tested. Prose that contradicted the code it documents. `_growth_ratio` excludes carried rows from both sides and `_growth_round_counts` adds a third eligibility term, but five surfaces still described the pre-narrowing behaviour. Executed against one document with 1 reviewer blocker + 2 carried rows, the prose denominator gives 3 and the code gives 1 - 0.33 vs 1.00, straddling the 0.50 ceiling, i.e. the two copies disagreed about whether the loop stops. SKILL.md, commands/premerge.md and common-mistakes.md corrected; RFC 0021 gets a new A6 amendment rather than an edit, because A5 was accurate when it landed and the narrowings came after it. Also: the fix-commit push now captures git's stderr the way the five SKILL.md fences do (driven against a hostile pre-receive hook), an unreachable fallback deleted after 50 executions with a live anti-vacuity control, the S10 loop re-runs its extractor 3 times instead of 17, and several comments that named dead consumers now name the live ones. Verified: 20 suites green, all at or above baseline. Artifact bytes unchanged where it matters - the premerge-findings refactor was diffed end to end and all 17 run artifacts plus both streams are byte-identical to HEAD's copy. --- .github/workflows/test.yml | 27 ++- docs/rfc/0021-premerge-stack-integration.md | 131 +++++++++++++ plugins/uberdev/commands/premerge.md | 40 ++-- plugins/uberdev/lib/goal-state.sh | 26 ++- plugins/uberdev/lib/premerge-findings.py | 184 ++++++++++++++---- plugins/uberdev/lib/review-consolidate.sh | 31 ++- .../uberdev/skills/premerge-pipeline/SKILL.md | 164 ++++++++-------- .../references/common-mistakes.md | 70 +++++-- tests/agent-description-budget.test.sh | 22 ++- tests/ci-shard-diagnose.sh | 176 +++++++++++++++-- tests/ci-shard-env.sh | 21 +- tests/ci-shard.test.sh | 172 +++++++++------- tests/model-routing.test.sh | 24 ++- tests/premerge-findings.test.sh | 37 ++-- tests/premerge.test.sh | 80 +++++++- tests/review-pr-consolidate.test.sh | 51 ++++- tests/testers-agent-contract.test.sh | 74 ++++++- 17 files changed, 1055 insertions(+), 275 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7f25687e..8aac1a4d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -491,7 +491,14 @@ jobs: echo " The plan value and the membership test disagree about these names -- that is what the dump above shows." fi if [ -z "$shard_unrun_mentioned" ] && [ -z "$shard_unrun_skipped" ]; then - echo " Every name in the plan ran, yet the counts differ: the plan's word count and its distinct names disagree (a repeated entry, or separators splitting into more words than names)." + # ONE cause reaches here, not two. A separator the split + # above does not recognise cannot: it merges its neighbours + # into a token no name the harness ran can match, so it + # prints in the NEVER INVOKED arm above -- as the two names + # run together -- and never gets this far. What is left is a + # plan that names one fixture twice. + echo " Every name in the plan ran, yet the counts differ: the plan names one fixture more than once, so its word count exceeds the number of distinct names run." + echo " The plan producer cannot emit that -- it de-duplicates by first occurrence and refuses a repeated name outright -- so this value did not come from it." fi fail=$((fail + 1)) failed="${failed}${failed:+ | }SHARD-COVERAGE($shard_ran/$shard_plan_len)" @@ -779,7 +786,14 @@ jobs: echo " The plan value and the membership test disagree about these names -- that is what the dump above shows." fi if [ -z "$shard_unrun_mentioned" ] && [ -z "$shard_unrun_skipped" ]; then - echo " Every name in the plan ran, yet the counts differ: the plan's word count and its distinct names disagree (a repeated entry, or separators splitting into more words than names)." + # ONE cause reaches here, not two. A separator the split + # above does not recognise cannot: it merges its neighbours + # into a token no name the harness ran can match, so it + # prints in the NEVER INVOKED arm above -- as the two names + # run together -- and never gets this far. What is left is a + # plan that names one fixture twice. + echo " Every name in the plan ran, yet the counts differ: the plan names one fixture more than once, so its word count exceeds the number of distinct names run." + echo " The plan producer cannot emit that -- it de-duplicates by first occurrence and refuses a repeated name outright -- so this value did not come from it." fi fail=$((fail + 1)) failed="${failed}${failed:+ | }SHARD-COVERAGE($shard_ran/$shard_plan_len)" @@ -1347,7 +1361,14 @@ jobs: echo " The plan value and the membership test disagree about these names -- that is what the dump above shows." fi if [ -z "$shard_unrun_mentioned" ] && [ -z "$shard_unrun_skipped" ]; then - echo " Every name in the plan ran, yet the counts differ: the plan's word count and its distinct names disagree (a repeated entry, or separators splitting into more words than names)." + # ONE cause reaches here, not two. A separator the split + # above does not recognise cannot: it merges its neighbours + # into a token no name the harness ran can match, so it + # prints in the NEVER INVOKED arm above -- as the two names + # run together -- and never gets this far. What is left is a + # plan that names one fixture twice. + echo " Every name in the plan ran, yet the counts differ: the plan names one fixture more than once, so its word count exceeds the number of distinct names run." + echo " The plan producer cannot emit that -- it de-duplicates by first occurrence and refuses a repeated name outright -- so this value did not come from it." fi fail=$((fail + 1)) failed="${failed}${failed:+ | }SHARD-COVERAGE($shard_ran/$shard_plan_len)" diff --git a/docs/rfc/0021-premerge-stack-integration.md b/docs/rfc/0021-premerge-stack-integration.md index e6c1dd24..2eb910c3 100644 --- a/docs/rfc/0021-premerge-stack-integration.md +++ b/docs/rfc/0021-premerge-stack-integration.md @@ -1016,3 +1016,134 @@ The A1 ordering (gate before repair), the line-independent multiset fingerprint, controller-only commits, and the wave scope guard are all untouched. The reviewed surface is still the whole stack, every attempt. C1–C8 remain the open constraint set for the design that changes that, and A5 is not it. + +### A6 — the growth population narrowed after A5 (2026-08-24) + +A5 is the design authority for `STOP_SELF_REFERENTIAL`, and it was accurate +against the tree it landed on. Two later changes narrowed the detector without +touching the amendment: #766 put a second kind of row into the array A5's formula +divides by, and the first two `/premerge` repair rounds on the stack that +combined #765 with #766 excluded that row from the ratio and added a term to the +stop. A5 is **retained as written** — the record of what was decided on +2026-08-23 — and narrowed here, the way A5 narrows A3 and A4 narrows §8. + +Both narrowings run the same way. `GROWTH` speaks about a **smaller** population +than A5 gives it, and the stop asks **one more question** than A5 records. + +#### 1. A carried row counts on neither side + +A5's *What it computes* reads, verbatim: + +> ``` +> GROWTH(N) = |{ b in blockers(N) : b.file in modified(N-1) +> and b.fingerprint not in blockers(N-1) }| +> / |blockers(N)| +> ``` +> +> `blockers(N)` is attempt N's `classified-.json` + +When that was written, the array held one kind of row: a blocker a reviewer +raised. A5 says nothing about carried rows because there were none to say +anything about. + +**#766 put a second kind in the same array.** A fixer whose honest scope spans +two files now returns `blocked_on_file`, and `_read_blocked_on` turns each such +path into a blocker finding on the **next** attempt, stamped +`blocked_on_carry: True` and carrying a controller-authored summary that states a +scheduling fact rather than a defect. Up to `MAX_BLOCKED_ON` (`8`) of them can +enter one attempt. They are blockers because the wave plan and the clean gate +have to see them — otherwise a run whose only outstanding work was carried gated +green, dispatched nothing and filed nothing — but no reviewer raised them. + +**They are excluded from both sides, not one.** `_growth_ratio` iterates +`_reviewer_blockers(document["blockers"])`, and the previous attempt's evidence +reaches it through `_blocker_fingerprints`, which iterates the same filter. So +`blockers(N)`, `blockers(N-1)` and the divisor all mean *the reviewer-raised rows +of* that array — and `-` for "no blockers to divide by" now means no +reviewer-raised blocker: an attempt holding nothing but carried rows reports `-`, +not a ratio. + +The numerator half is forced. A path is carried precisely BECAUSE the previous +fixer could not edit it, so it is never in `fix-scope-.modified` and could +not have satisfied the membership test anyway. The **denominator** half is the +load-bearing one: left in the divisor, a row that can never be counted dilutes +the ratio every round, and the suppression is largest on the runs with the most +cross-file churn — exactly the runs `STOP_SELF_REFERENTIAL` exists for. Two +suppressed rounds in a row is all it takes for it never to fire. + +Executed against the shipped module — one reviewer-raised blocker in a file the +last repair modified, alongside one carried consequence: + +| | stored `blockers` array | divisor | `BLOCKERS=` | `GROWTH=` | +| --- | --- | --- | --- | --- | +| shipped | 2 | 1 | 1 | `1.00` | +| A5's formula as written | 2 | 2 | — | `0.50` | + +**A5's other denominator claim survives intact.** "the denominator is the same +population `BLOCKERS=` already reports" is still true: `BLOCKERS=` is +`_blocker_fingerprints`' multiset total, and the two narrowed together through +`_reviewer_blockers` — which is also the single derivation +`counts.blocker_reviewer` uses, so the three spellings of that population cannot +drift apart. What changed is what the shared population *is*, not that the two +agree. + +#### 2. The streak test has a third term + +A5's *The stop* reads, verbatim: + +> It fires when `GROWTH` is at or above `CONVERGE_GROWTH_CEILING` (`0.50`) on +> `CONVERGE_GROWTH_RUNS` (`2`) consecutive **measured** attempts + +`_growth_round_counts` — one predicate, applied to this round's numbers and then +to the previous round's own `converge.jsonl` columns — asks **three** things of +each round. It is measured; it is at or above the ceiling; **and +`blockers >= previous_blockers`**, the round did not reduce the reviewer-raised +blocker count. + +The third term is not defensive plumbing. `GROWTH=` answers *what is the residual +made of*, never *which way is this going*, and those two come apart in one +direction: the denominator is the residual, so it shrinks as the loop succeeds, +while the numerator counts findings the reviewer newly sampled in the file the +repair just touched — the file it was always likeliest to resample. **The ratio +therefore rises toward `1.00` precisely as a run converges.** Reproduced against +the shipped module with `--max-repairs 5`: five blockers, then four cleared +leaving one survivor and one new (`0.50`), then one more cleared leaving one +survivor and one new (`0.50`) — `STOP_SELF_REFERENTIAL` on a run that went +5 → 2 → 2, with three of five repair rounds unspent. `CONVERGE_GROWTH_RUNS` does +not mitigate it: at two rounds, a single new finding in a repaired file *is* +`0.50`, twice running. + +That is C3's failure with the ratio's own name on it — the failure A5 argues +against for one high round and then leaves undefended for two. A round that +removed more of the reviewer's blockers than it was left holding is a round in +which repairing demonstrably worked, whatever its residual is made of, so it is +not eligible to be half of a divergence streak. The ratio is still measured, +still printed and still recorded on such a round; a genuine pump, which by +definition is not reducing the count, reaches the ceiling on consecutive eligible +rounds exactly as A5 describes. + +Executed against the shipped predicate at `GROWTH=0.50`: + +| round | `previous_blockers` → `blockers` | eligible | +| --- | --- | --- | +| count held | 5 → 5 | yes | +| count grew | 2 → 5 | yes | +| count fell | 5 → 2 | **no** | +| not measured (`GROWTH=-`) | any | **no** | + +#### What this does not change + +The threshold (`0.50`), the streak length (`2`), the cascade position (after +`STOP_NO_PROGRESS`, before `STOP_EXHAUSTED`), the file-granularity proxy and its +recorded limits, `GROWTH=-` meaning *not measured* rather than `0.00`, and the +survivors reaching Phase 5 through `--include-blockers` are all as A5 wrote them. +Carried rows leave the **evidence** questions only: they still enter the wave +plan, still gate `blockers_remaining=`, and are still filed — at the §5 +`SUGGESTION` tier, via `_as_deferred_carry_row` — when the loop stops with the +edit outstanding. + +`skills/premerge-pipeline/SKILL.md` (`## Phase 3b — CONVERGE`) and the docstrings +on `_growth_ratio`, `_reviewer_blockers` and `_growth_round_counts` state both +narrowings at the code. This amendment is what stops the design authority +outranking them with the wider population. C1–C8 remain the open constraint set, +and A6, like A5, is not the design that closes them. diff --git a/plugins/uberdev/commands/premerge.md b/plugins/uberdev/commands/premerge.md index 44066412..d2993601 100644 --- a/plugins/uberdev/commands/premerge.md +++ b/plugins/uberdev/commands/premerge.md @@ -70,11 +70,15 @@ see infinite progress and never stop. A third measurement catches what those two cannot. Both compare fingerprint sets drawn from independent full-stack reviews, so neither can tell a defect the fixers just wrote from one that was always there. `STOP_SELF_REFERENTIAL` fires -when, for two attempts running, at least half of the blockers a round returned -did not exist the round before **and** sit in files the previous repair -modified — the loop reviewing its own output. It reports `GROWTH=` on every -attempt so you can see the difference between a run that was still converging -and one that had started generating its own work. The findings that trip it are +on two consecutive **eligible** rounds, and eligibility is three conditions you +can read straight off the `PREMERGE_CONVERGE` line: `GROWTH=` is measured, it is +at or above `0.50` — at least half of the blockers the round returned did not +exist the round before **and** sit in files the previous repair modified — and +`BLOCKERS=` did not fall below `PREV=`. That last condition is what keeps the +stop off a loop that is working: a round that cut the reviewer's blocker count +repaired something, whatever its residual is made of, so it is never half of a +streak however high its ratio. A run log showing `GROWTH=0.90` twice and no stop +is that rule firing, not the detector failing. The findings that trip it are filed as issues like every other survivor, not dropped. Why that measurement has to exist at all: a reviewer asked *what is wrong with @@ -90,19 +94,23 @@ the correct response to that is to **stop**, not to repair again. still winning, raise `--converge`"* and *"the loop was reviewing itself"* are **opposite** operator responses — and an operator shown the budget stop for a run that did the second will raise the budget on a loop that can never converge. For -the same reason the stop takes **two consecutive measured rounds** at or above -that half-mark, never one. Each review is a fresh sample, and the files most -likely to be resampled next round are exactly the ones the last repair touched, -so a single high round is equally consistent with a healthy convergence whose -repairs happen to be concentrated in a few files. Removing the two-round -requirement makes the detector stop loops that were working — the failure it -exists to avoid. +the same reason the stop takes **two consecutive eligible rounds**, never one. +Each review is a fresh sample, and the files most likely to be resampled next +round are exactly the ones the last repair touched, so a single high round is +equally consistent with a healthy convergence whose repairs happen to be +concentrated in a few files. Dropping the two-round requirement — or reducing +eligibility to the ratio alone — makes the detector stop loops that were +working, the failure it exists to avoid. `GROWTH=` is a signal, not a proof. Attribution is at file granularity, so a -genuinely new defect elsewhere in a repaired file over-counts as growth; and a -round whose repair committed nothing leaves no scope list to attribute against, -so it prints `GROWTH=-` — *not measured*, which is a different answer from -`0.00`. +genuinely new defect elsewhere in a repaired file over-counts as growth. And it +prints `GROWTH=-` — *not measured*, a different answer from `0.00` — on attempt +1, on a round with no reviewer-raised blocker to divide by, and whenever the +previous repair left no readable scope list: it committed nothing, its list was +unreadable (announced on stderr), or it went through the Phase 3 CI arm, which +pushes real code edits but publishes no `fix-scope` list. That last one is the +case to watch — while a loop keeps repairing through CI it cannot accumulate two +eligible rounds, so `STOP_SELF_REFERENTIAL` is unreachable for it. `--converge` is the runaway backstop behind those three, not the stop condition, and that distinction is the answer to the hazard RFC 0021 originally refused a diff --git a/plugins/uberdev/lib/goal-state.sh b/plugins/uberdev/lib/goal-state.sh index a96d9c1e..3311e286 100644 --- a/plugins/uberdev/lib/goal-state.sh +++ b/plugins/uberdev/lib/goal-state.sh @@ -3862,11 +3862,27 @@ _uberdev_goal_rebase_collision_chain() { # LC_ALL=C ON EVERY MEMBER OF THE sort/comm TRIO, INCLUDING `comm` ITSELF. # This compares PATHS AS BYTES. `sort` orders by the ambient collation and # `comm`'s merge walk assumes the order its own collation would produce; when - # the two disagree the walk desynchronises. Here that fails OPEN -- a `comm - # -12` intersection that misses a shared path reports NO collision between two - # PRs that do collide, and the rebase this function exists to trigger never - # fires. The sorts happen to share a shell today, so they agree by accident; - # pinning makes it a property of the code. + # the two disagree the walk desynchronises and a `comm -12` intersection can + # miss a shared path, reporting NO collision between two PRs that do collide. + # + # THAT MISS IS NOT A DEFECT IN ITSELF, and this pin does not claim otherwise. + # The contract above is best-effort by design -- rc 0 always, collision + # detection never halts the goal -- and the `gh pr diff` arm right above + # already routes EVERY API failure to an empty path-set and so to the same + # silent non-collision, by a far more likely route. That fail-open is + # accepted by the contract and is deliberately left as it stands. Nor is the + # miss wholly unguarded downstream: its renumbering half is re-derived by + # `_uberdev_goal_ensure_version_bump`, which re-fetches `origin/` and + # compares head-vs-base itself before every merge dispatch (goal-watch.sh + # step 2c) and fails CLOSED when it cannot guarantee the bump. + # + # What the pin buys is narrower, and sits INSIDE that best-effort guarantee: + # when both path-sets ARE in hand, the comparison answers correctly instead + # of silently desynchronising. A collision missed because the API was + # unreachable is degraded INPUT, which the contract blesses; one missed while + # computing over complete input is a wrong ANSWER, which it does not. The + # sorts happen to share a shell today, so they agree by accident; pinning + # makes the agreement a property of the code. merged_sorted="$(printf '%s\n' "$merged_diff" | LC_ALL=C sort -u)" local pr while IFS= read -r pr; do diff --git a/plugins/uberdev/lib/premerge-findings.py b/plugins/uberdev/lib/premerge-findings.py index 57526d9b..b51605ba 100644 --- a/plugins/uberdev/lib/premerge-findings.py +++ b/plugins/uberdev/lib/premerge-findings.py @@ -436,6 +436,24 @@ def _beneath_repo_root(root: str, relative: str) -> bool: No symlink is tracked in this repo today, which is exactly why the check belongs in the code rather than in a sentence about the tree. + THAT SAMENESS IS ASSERTED IN PROSE AND PINNED BY NOTHING. This body is a + transcription of `code_fixer_contract.beneath` (the only difference is that + this one takes a repo-relative argument and joins it), and no test imports + both and compares them. The two agree today -- executed across eleven + inputs, including a non-str argument, an absolute path, `..` and a + normalising path, with zero disagreements -- but agreeing today is what a + copy does right up until one side is hardened. This module has already been + burned by exactly that: `_repo_path`'s docstring records a prose-asserted + equivalence in this same file that was FALSE and would have shipped the + symlink escape this function exists to refuse. So: any change to either side + must be mirrored by hand, and the durable fix is an executed cross-module + equality row in tests/premerge-findings.test.sh, or importing the sibling + the way `code_fixer_contract.py` itself loads `run_manifest` and + `atomic_move`. Importing was weighed and not taken here: it would turn a + stdlib-only module into one whose import pulls in three sibling files, and + couple `/premerge`'s triage half to the fixer-authority half for six lines + of pure path arithmetic. + Returns False on any error rather than raising: the caller drops a row it cannot vouch for, and a validator that throws where the caller expected a verdict is a validator that fails open at the call site. @@ -1097,6 +1115,31 @@ def _is_carried(finding: object) -> bool: return isinstance(finding, dict) and finding.get("blocked_on_carry") is True +def _reviewer_blockers(blockers: "list") -> "list": + """A blocker array narrowed to the rows a REVIEWER raised. + + ONE derivation of that population, for the three places that need it: + `_blocker_fingerprints`, `_growth_ratio`'s denominator, and the + `counts.blocker_reviewer` field `cmd_plan` writes. The field used to be a + subtraction over `cmd_plan`'s own locals -- `len(blockers) - len(synthesised)` + -- which is a THIRD spelling of the same question. It agrees with the filter + for every document this file builds, because `_normalise_findings` composes + a fixed key set and can therefore never stamp `blocked_on_carry` on a + reviewer row. But it is arithmetic over transient locals rather than a + predicate over the rows, so it answers on provenance where the other two + answer on the marker, and on a hand-edited or resumed artifact where a + reviewer row DOES carry the flag the two spellings return different numbers + (measured: filter 0, subtraction 1). Routing all three through here removes + the possibility rather than documenting it. + + NO VALIDATION HERE, DELIBERATELY. It filters and nothing else, so each + caller keeps the refusal it already had for a malformed row -- and a + non-object is never `_is_carried`, so it survives this filter and still + reaches the caller's own check in the same relative order. + """ + return [row for row in blockers if not _is_carried(row)] + + def _as_deferred_carry_row(finding: "dict") -> "dict": """A carried row as `cmd_defer` files it: same finding, cleanup tier. @@ -1157,9 +1200,7 @@ def _blocker_fingerprints(document: "dict") -> "collections.Counter[str]": treating its own scheduling notes as evidence of progress. """ prints: "collections.Counter[str]" = collections.Counter() - for finding in document["blockers"]: - if _is_carried(finding): - continue + for finding in _reviewer_blockers(document["blockers"]): value = finding.get("fingerprint") if isinstance(finding, dict) else None if not isinstance(value, str) or not re.fullmatch(r"[0-9a-f]{16}", value): # Pre-loop artifacts carry no fingerprint. Refusing is the only @@ -1209,18 +1250,43 @@ def _carry_prior_suggestions(out_dir: str, attempt: int) -> "list[dict]": # The controller-authored identity text of a carried cross-file consequence. # -# NO ATTEMPT NUMBER IN IT, DELIBERATELY. This string becomes the row's -# `summary`, and `_fingerprint` hashes the summary into the cross-attempt -# identity `_blocker_fingerprints` counts on. It used to interpolate the -# reporting attempt, so one consequence that persisted across two attempts -# arrived under two different fingerprints: `_blocker_fingerprints` counted the -# old one as `fixed` and the new one as `new` on every single round, which is -# manufactured progress. STOP_NO_PROGRESS needs an UNCHANGED multiset and could -# therefore never fire while such a row was carried, and STOP_REGRESSED needs -# `fixed == 0`, which the phantom made false every time. The comment on the old -# site called this summary "controller-authored and deterministic"; the attempt -# number made it the opposite of deterministic across the only axis that -# mattered. +# NO ATTEMPT NUMBER IN IT, DELIBERATELY -- BUT NOT FOR THE CONSUMER THIS +# COMMENT USED TO NAME. This string becomes the row's `summary`, and +# `_fingerprint` hashes the summary into the row's stored identity. It used to +# interpolate the reporting attempt, so one consequence that persisted across +# two attempts arrived under two different fingerprints: `_blocker_fingerprints` +# counted the old one as `fixed` and the new one as `new` on every single round, +# which is manufactured progress. STOP_NO_PROGRESS needs an UNCHANGED multiset +# and could therefore never fire while such a row was carried, and +# STOP_REGRESSED needs `fixed == 0`, which the phantom made false every time. +# The comment on the old site called this summary "controller-authored and +# deterministic"; the attempt number made it the opposite of deterministic +# across the only axis that mattered. +# +# THAT CONSUMER NO LONGER READS THIS TEXT AT ALL. The same defect was closed a +# second and wider way, because removing the attempt number never finished the +# job -- the fingerprint is `hash(file, summary)` and the FILE varies across +# attempts too. `_blocker_fingerprints` and `_growth_ratio` now drop every +# carried row through `_reviewer_blockers` BEFORE they read anything off it. +# Executed against the shipped module: a carried row whose `fingerprint` is the +# literal `GARBAGE-NOT-HEX` yields an empty multiset and no refusal, while the +# same row with `blocked_on_carry` removed exits 74 on `fingerprint_missing`. +# So no progress arithmetic in this file keys on this string any more. +# +# THE STABILITY CONSTRAINT IS STILL LIVE, FOR A CONSUMER OUTSIDE THIS FILE. +# `_encode_aggregate` publishes neither `fingerprint` nor `blocked_on_carry`, so +# `agents/findings-to-issues.md` derives a carried row's per-finding identity +# itself: Step 8's `MEMBER_FP = sha256(path:line:normalised_summary)[:16]`. A +# carried row's `path` is fixed by the row and its `line` is `None` -- encoded +# as `1` by `_encode_aggregate` -- so THIS TEXT IS THE ONLY VARYING TERM. +# Reinterpolate the attempt number and one consequence that survives two runs +# is recorded as two members of that file's container issue instead of one +# recurring member. +# +# AND NOTHING EXECUTES THAT. No test asserts a carried row's summary, and the +# filer is an agent rather than an importable function, so this comment is the +# whole of the guard. Do NOT read the skip in `_blocker_fingerprints` as proof +# the constraint died with it. # # The attempt number is not lost. It goes into `failure_scenario`, which is # where the agent's own words already live precisely because nothing keys on @@ -1428,9 +1494,13 @@ def _read_blocked_on( "blocked_on_schema_invalid", "blocked_on summary", ) - # The reporting attempt lives here and nowhere else: this field is - # the one nothing keys on, which is what lets the summary above stay - # identical across attempts. + # The reporting attempt lives here and nowhere else: of the three + # strings this row carries, `failure_scenario` is the one no + # identity is derived from -- not `_fingerprint` below, and not the + # filer's `path:line:normalised_summary` member key -- which is what + # lets the summary above stay identical across attempts. See + # `_BLOCKED_ON_SUMMARY` for who still depends on that stability, and + # for why the consumer that comment originally named is not it. detail = _bounded_text( "attempt %d's fixer for %s reported: %s" % (prior, origin, reason), MAX_DETAIL_BYTES, @@ -1500,19 +1570,55 @@ def _read_blocked_on( "failure_scenario": detail, "category": None, "verdict": None, + # KEPT ON PURPOSE THOUGH NOTHING READS IT. `_reviewer_blockers` + # drops this row before `_blocker_fingerprints` validates + # identities and before `_growth_ratio` measures, and + # `_encode_aggregate` publishes no `fingerprint` key at all, so + # neither a consumer nor a validator ever sees this value. It + # stays for SCHEMA UNIFORMITY with reviewer rows -- whole rows + # are pasted verbatim into the fixer wave prompt -- and because + # dropping it would change the bytes of `classified-NN.json` and + # `fix-waves-NN.json` for a value no decision depends on. "fingerprint": _fingerprint(target, summary), "severity": "blocker", "severity_source": "controller", - # THE MARKER `_growth_ratio` READS. This row is not something a - # reviewer reported and not something the previous repair wrote - # -- it is the loop carrying its own bookkeeping forward, and by - # construction it names a file that repair did NOT modify. Left - # in the GROWTH denominator it can only ever dilute the ratio - # downward: three genuinely self-referential blockers plus three - # carried consequences measured 0.50 instead of 1.00, and the - # suppression is largest on the runs with the most cross-file - # churn -- the runs the detector exists for. An additive key, so - # a pre-loop artifact that lacks it reads exactly as it did. + # THE MARKER FOR "THIS ROW IS THE LOOP'S OWN BOOKKEEPING", AND + # IT HAS FOUR READERS, NOT ONE. No reviewer reported this row + # and the previous repair did not write it -- the loop is + # carrying its own scheduling note forward, and by construction + # it names a file that repair did NOT modify. Every reader goes + # through `_is_carried`, three of them via `_reviewer_blockers`; + # the evidence-versus-work line that decides which way each one + # falls is documented above that predicate. The four, and what + # each one does: + # + # 1. `_growth_ratio` -- excluded from BOTH sides. It can never + # enter the numerator (the file is not in the measured + # repair scope), and in the denominator it only dilutes: + # three genuinely self-referential blockers plus three + # carried consequences measured 0.50 instead of 1.00, and + # the suppression is largest on the runs with the most + # cross-file churn -- the runs the detector exists for. + # 2. `_blocker_fingerprints` -- excluded from the progress + # multisets, so a carried row can no longer read as + # `FIXED=1 NEW=1` on a round that repaired nothing, which + # is what made STOP_NO_PROGRESS and STOP_REGRESSED + # structurally unreachable. + # 3. `cmd_defer` -- `_as_deferred_carry_row` demotes the row to + # cleanup tier, so a stranded consequence is still filed but + # never outranks a proven defect at the filer's `max_new` + # cap. + # 4. `cmd_plan` -- excluded from `counts.blocker_reviewer`, the + # field that names the reviewer-raised population instead of + # leaving an operator to subtract `BLOCKED=` from + # `BLOCKER=`. The only one of the four that decides nothing; + # it is routed through the same filter so it cannot answer + # differently from the two that do. + # + # WHAT IT DOES NOT TOUCH: `_fix_waves` still plans this row and + # `assert-green` still counts it into `blockers_remaining=`. An + # additive key, so a pre-loop artifact that lacks it reads + # exactly as it did. "blocked_on_carry": True, } ) @@ -1787,11 +1893,9 @@ def _growth_ratio( return None grown = 0 measured = 0 - for finding in document["blockers"]: + for finding in _reviewer_blockers(document["blockers"]): if not isinstance(finding, dict): fail("finding_schema_invalid", "a blocker is not an object") - if _is_carried(finding): - continue measured += 1 # `_scope_key`, NOT `_repo_path`. Both sides of this test have to be # derived the same way or a padded path silently leaves the numerator, @@ -2249,9 +2353,21 @@ def cmd_plan(args: argparse.Namespace) -> int: # numbers, and an operator who reads `BLOCKER=1` on one line and # `BLOCKERS=0` on the next deserves better than arithmetic on # `BLOCKED=` to reconcile them. This key is that number, written - # down. It equals `blocker - blocked_on_admitted` by construction, - # which is what makes the artifact self-checking. - "blocker_reviewer": len(blockers) - len(synthesised), + # down. + # + # IT IS WRITTEN FOR A HUMAN, NOT FOR A CHECKER. A repo-wide search + # finds one writer -- this line -- and no reader: no fence, no test + # and no agent parses it, and the loop's own progress arithmetic + # runs off `_blocker_fingerprints` and `_growth_ratio` instead. It + # still equals `blocker - blocked_on_admitted` by construction, so + # the three counts in this object can be reconciled by eye; nothing + # reconciles them automatically, and calling that self-checking + # overstates it. + # + # DERIVED THROUGH THE SAME FILTER THE OTHER TWO USE, not by + # subtracting `len(synthesised)` -- see `_reviewer_blockers` for the + # document shape on which the subtraction and the filter disagree. + "blocker_reviewer": len(_reviewer_blockers(blockers)), "suggestion": len(suggestions), # How many severities were machine-checked rather than judged. The # run summary prints this so an operator can see at a glance whether diff --git a/plugins/uberdev/lib/review-consolidate.sh b/plugins/uberdev/lib/review-consolidate.sh index d1dc59d7..17768415 100644 --- a/plugins/uberdev/lib/review-consolidate.sh +++ b/plugins/uberdev/lib/review-consolidate.sh @@ -169,7 +169,9 @@ review_consolidate_survivors() { excluded_numbers="$(review_consolidate_excluded_numbers "$scan_dir")" || return 2 numeric="$(jq -R 'tonumber?' <<<"$excluded_numbers")" || return 2 dropped="$(jq -sc '.' <<<"$numeric")" || return 2 - [ -n "$dropped" ] || dropped='[]' + # No empty-string fallback for $dropped: the line above returns 2 on any + # non-zero jq, and `jq -sc '.'` writes an array -- `[]` at minimum -- on every + # zero exit, so the empty case a fallback would cover cannot occur. jq -c --argjson dropped "$dropped" \ '[ .[] | select((.number as $n | $dropped | index($n)) == null) ]' \ <"$scan_dir/candidates.json" 2>/dev/null || printf '[]\n' @@ -1031,8 +1033,31 @@ review_consolidate_push_branch() { return 79 ;; esac - git -C "$worktree" push -u origin "$branch" >/dev/null 2>&1 || { - _uberdev_consolidate_error "pushing $branch to origin failed" + # git's own words are CAPTURED, not discarded. `>/dev/null 2>&1 || return 2` + # sent the explanation of a non-fast-forward, an expired token or a + # protected-branch rule to the same place as the progress output, and this is + # the push the rest of Phase 0 depends on: it aborts the run mid-loop with the + # cause already thrown away. The five push sites in + # skills/premerge-pipeline/SKILL.md are the model; this one lives in the + # library, outside the fence corpus tests/premerge.test.sh P18 scans. + local push_err + push_err="$(git -C "$worktree" push -u origin "$branch" 2>&1 1>/dev/null)" || { + # Bounded and defused BEFORE anything reads it, because a pre-receive hook + # chooses the length and the content of every `remote:` line. The same three + # jobs the fences' one-liner does -- references/phase-contracts.md, `### What + # a failed push is allowed to say`: 200 chars, one line, no live token. + # + # BOTH scraped markers are broken, because this library's stderr is the + # stream BOTH callers scrape positionally: commands/review-pr.md prints + # `REVIEW_CONSOLIDATE PR_NUMBER=... SCAN_ID=...` from the very fence that + # calls this function, and the premerge fences that source this file print + # `PREMERGE ...`. Either spelling arriving on a `remote:` line is a + # well-formed result no matter where it sits. `tr` and `cut` read to EOF, so + # this pipeline is not the early-exiting shape tests/epipe-guard.test.sh bans. + push_err="${push_err//PREMERGE/PRE-MERGE}" + push_err="${push_err//REVIEW_CONSOLIDATE/REVIEW-CONSOLIDATE}" + push_err="$(printf '%s' "$push_err" | tr -s '\n\r\t' ' ' | cut -c1-200)" + _uberdev_consolidate_error "pushing $branch to origin failed: $push_err" return 2 } return 0 diff --git a/plugins/uberdev/skills/premerge-pipeline/SKILL.md b/plugins/uberdev/skills/premerge-pipeline/SKILL.md index c4fa17c5..1576deac 100644 --- a/plugins/uberdev/skills/premerge-pipeline/SKILL.md +++ b/plugins/uberdev/skills/premerge-pipeline/SKILL.md @@ -1365,99 +1365,99 @@ successfully converged stack as a broken one. | `CONTINUE` | something is still fixable and the last attempt changed something | repair (3c) → Phase 1 at attempt+1 | | `STOP_NO_PROGRESS` | an attempt changed neither the blockers nor the CI reason | **Phase 5, not green** | | `STOP_REGRESSED` | our own fixes grew the blocker set | **Phase 5, not green** | -| `STOP_SELF_REFERENTIAL` | two rounds running, most of the blockers were defects that did not exist before this loop's own repairs wrote them | **Phase 5, not green** | +| `STOP_SELF_REFERENTIAL` | two **eligible** rounds running (below), at least half the blockers were defects that did not exist before this loop's own repairs wrote them | **Phase 5, not green** | | `STOP_EXHAUSTED` | the runaway backstop | **Phase 5, not green** | | `STOP_UNREADABLE` | stale evidence, or CI that never settled | **Phase 5, not green** | -`STOP_NO_PROGRESS` and `STOP_REGRESSED` are the ones that matter. A counter alone -would stop a loop that was converging perfectly well *and* let three useless -attempts run; these two stop the moment repairing stops working, and they are -computed from blocker **fingerprints**, not from prose. +`STOP_NO_PROGRESS` and `STOP_REGRESSED` are the ones that matter: a counter alone would +stop a converging loop *and* let three useless attempts run, while these two stop the +moment repairing stops working — computed from blocker **fingerprints**, not prose. **Why a fingerprint and not `file:line`.** A fix moves lines — including in other -findings' hunks — so an identity keyed on the line reports every survivor as -brand new. The loop would see infinite progress and never stop. `lib/premerge-findings.py` -hashes `path` + a case-folded, punctuation-stripped `summary` instead, which is -stable across exactly the edits the loop makes. It is deliberately **not** either -fingerprint `findings-to-issues` computes. That agent keys a per-finding identity -on `file:line:summary` because a finding is about a location, and a per-ISSUE -identity on the owning file alone because an issue is now about a file. Same -width, three different questions. - -**The growth ratio, and why it is a different question.** `FIXED=` and `NEW=` -compare two fingerprint multisets, and RFC 0021 A3 records what that cannot see. -A critic asked *"what is wrong with this?"* samples an **unbounded** set, so the -two multisets are independent draws from a generator, not two readings of a fixed -population: a blocker the sampler simply did not roll counts `FIXED`, a defect -that was always there counts `NEW`, and `findings == 0` is a property of the -sample, never of the artifact. That is why find→fix→re-find has no fixed point -and runs until something outside it says stop. `GROWTH=` is that something. It -asks the one question the fingerprints cannot: **of this attempt's blockers, what -fraction did not exist last attempt AND sits in a file the last repair actually -modified?** At `1.00` the loop is reviewing what its own repairs wrote, and the -answer to that is to stop, not to repair again. - -- **Denominator** — every blocker in this attempt's `classified-.json`, the - same number printed as `BLOCKERS=`. +findings' hunks — so an identity keyed on the line reports every survivor as brand new, +and the loop would see infinite progress and never stop. `lib/premerge-findings.py` +hashes `path` + a case-folded, punctuation-stripped `summary` instead, stable across +exactly the edits the loop makes. It is deliberately **not** either fingerprint +`findings-to-issues` computes: a per-finding identity on `file:line:summary` because a +finding is about a location, and a per-ISSUE identity on the owning file because an +issue is now about a file. Same width, three different questions. + +**The growth ratio, and why it is a different question.** `FIXED=` and `NEW=` compare +two fingerprint multisets; RFC 0021 A3 records what that cannot see. A critic asked +*"what is wrong with this?"* samples an **unbounded** set, so the two multisets are +independent draws from a generator, not two readings of one population: a blocker the +sampler did not roll counts `FIXED`, an always-present defect counts `NEW`, and +`findings == 0` is a property of the sample, never of the artifact. Find→fix→re-find +therefore has no fixed point; it runs until something outside says stop, and `GROWTH=` +asks what the fingerprints cannot: **of this attempt's reviewer-raised blockers, what +fraction did not exist last attempt AND sits in a file the last repair modified?** At +`1.00` the loop is reviewing what its own repairs wrote; stop, do not repair again. + +- **Denominator** — this attempt's **reviewer-raised** blockers in + `classified-.json` (`blocked_on_carry` rows excluded): the number printed as + `BLOCKERS=`. - **Numerator** — those blockers whose `file` is in the previous attempt's - `fix-scope-.modified` and whose fingerprint the previous attempt's - evidence did not carry. Excluding carried fingerprints is what keeps a - *survivor* out of the count: the wave plan assigns exactly the files this - attempt's blockers named, so a finding the fixer did not clear is always in a - touched file, and counting it would make a converging loop stop on its own - success. The numerator is therefore a strict subset of `NEW=`. -- **Both inputs are already on disk** in the run directory. Nothing new is - written, no phase is added, and `converge` takes no new argument. - -**Two consecutive rounds, and never one.** `PREMERGE_GROWTH_CEILING` is compared -with `>=`, but one round at or above it changes no decision: the stop needs -`PREMERGE_GROWTH_RUNS` consecutive **measured** rounds, read back off the previous -attempt's own `converge.jsonl` row rather than carried in a variable a fence could -forget. This is not caution for its own sake, and it is the part most likely to be -"simplified" away by someone who has not read why it is there. A3 §2 establishes -that attempt N's blocker set is a *fresh sample*, so a defect present since -attempt 1 but first rolled at attempt 3 counts as new — and the files most likely -to be resampled are exactly the ones the repair just touched. One high round is -therefore what a healthy convergence with concentrated repairs looks like, and -firing on it would stop a loop that was working, which is the failure A3 C3 names -outright. A loop that really is pumping pays one extra round for this, every time; -that is the deliberate price. **Do not reduce the requirement to a single round.** - -**Where it sits in the cascade, and what it pre-empts.** The ratio is read after -`STOP_NO_PROGRESS` — the two fingerprint detectors are sharper diagnoses and keep -their place — and **before** `STOP_EXHAUSTED`, which it pre-empts on purpose. -"The loop was still winning, raise `--converge`" and "the loop was reviewing -itself" are opposite operator responses, and the budget stop says the first about -a run that did the second. - -`GROWTH=-` means *not measured*, not *zero*, and it has **four** causes: + `fix-scope-.modified` and whose fingerprint the previous evidence did not carry. + That exclusion keeps a *survivor* out: the wave plan assigns exactly the files this + attempt's blockers named, so an uncleared finding is always in a touched file, and + counting it would stop a converging loop on its own success. The numerator is + therefore a strict subset of `NEW=`. +- **Both inputs are already on disk** in the run directory: nothing new is written, no + phase is added, and `converge` takes no new argument. + +**Two consecutive ELIGIBLE rounds, and never one.** `PREMERGE_GROWTH_CEILING` is +compared with `>=`, but one round at or above it changes no decision: the stop needs +`PREMERGE_GROWTH_RUNS` consecutive **eligible** rounds — measured, at or above the +ceiling, **and** not having cut the reviewer-raised blocker count +(`blockers >= previous_blockers`) — read off the previous attempt's own `converge.jsonl` +row, not a variable a fence could forget. That third test is why a run can print +`GROWTH=0.90` twice and not stop: a round that cut the count demonstrably repaired +something, whatever its residual is made of. Without it a measured 5 → 2 → 2 run read +`0.50` twice and stopped with three of five repair rounds unspent. A3 §2 makes attempt +N's blocker set a *fresh sample*: a defect present all along but first rolled at attempt +3 counts as new, and the files likeliest to be resampled are the ones the repair just +touched. One high round is therefore healthy concentrated convergence, and firing on it +would stop a loop that was working — the failure A3 C3 names outright. A real pump pays +one extra round every time: the deliberate price. **Do not reduce the requirement to a +single round, or eligibility to the ratio alone.** + +**Where it sits in the cascade.** The ratio is read after `STOP_NO_PROGRESS` — the +fingerprint detectors are sharper diagnoses and keep their place — and **before** +`STOP_EXHAUSTED`: "still winning, raise `--converge`" and "the loop was reviewing +itself" are opposite operator responses, and the budget stop says the first about a run +that did the second. + +`GROWTH=-` means *not measured*, not *zero*, and it has **five** causes: 1. **attempt 1** — there is no predecessor to attribute anything to; 2. **a previous attempt that committed no edits**, so wrote no scope list — the - fix-commit fence's `REASON=no-edits` early exit. A repair that only re-ran CI - is a real `CONTINUE`, and there is no repair to blame this round's findings on; -3. **no blockers this attempt** — nothing to divide by; -4. **a scope list that *is* present but could not be read** — the only one of the - four that is a **fault** rather than an undefined ratio. It announces itself, - printing `premerge-findings: repair_scope_unreadable:` with the path on - **stderr**. So a `-` on the line is the cue to check stderr before concluding - the ratio was merely undefined. - -None of the four stops the loop, and none of them is `0.00`. Reading `-` as zero -turns *we did not look* into *we looked and it was fine*. - -**Deliberately the coarse proxy.** The precise question is whether a finding -landed in text the previous repair *wrote*, which needs diff-hunk mapping nothing -in this pipeline produces (A3 C8: the reviewer accepts a PR, a branch or a path, -not a two-SHA range). File-level membership counts a new finding anywhere in a -touched file, so it errs toward stopping — which the two-round requirement is -there to absorb. Do not build the hunk-mapping version to "fix" this; build it, if + fix-commit fence's `REASON=no-edits` early exit, leaving nothing to blame this + round's findings on; +3. **a previous attempt repaired through the CI arm** — `arm=commit` makes real code + edits and pushes but publishes only `ci-scope-.*`, never + `fix-scope-.modified`. This is the one cause that switches the detector **off**: + such a loop never gets two consecutive eligible rounds, so `STOP_SELF_REFERENTIAL` is + unreachable until that fence publishes a scope of its own; +4. **no reviewer-raised blocker this attempt** — nothing to divide by; +5. **a scope list that *is* present but could not be read** — the only one of the five + that is a **fault** rather than an undefined ratio. It announces itself on **stderr** + as `premerge-findings: repair_scope_unreadable:` with the path, so `-` is the cue to + check stderr before reading the ratio as undefined. + +None of the five stops the loop, and none of them is `0.00`. Reading `-` as zero turns +*we did not look* into *we looked and it was fine*. + +**Deliberately the coarse proxy.** The precise question — did this finding land in text +the previous repair *wrote* — needs diff-hunk mapping nothing here produces (A3 C8: the +reviewer takes a PR, a branch or a path, not a two-SHA range). File membership counts a +new finding anywhere in a touched file, so it errs toward stopping, which the two-round +requirement absorbs. Do not build the hunk-mapping version to "fix" this; build it, if ever, as the C1–C8 design A3 is waiting for. -**The findings that trip it are not discarded.** `STOP_SELF_REFERENTIAL` is a -not-green stop like the others, so Phase 5 runs with `PREMERGE_SURVIVORS=1` and -`defer` is handed `--include-blockers`. Every surviving blocker becomes an issue. -The ratio's claim is "not *this loop's* work", never "not work". +**The findings that trip it are not discarded.** `STOP_SELF_REFERENTIAL` is a not-green +stop, so Phase 5 runs with `PREMERGE_SURVIVORS=1` and `defer` gets `--include-blockers`: +every surviving blocker becomes an issue. The claim is "not *this loop's* work", never +"not work". ### 3c — Repair, by reason diff --git a/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md b/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md index 2b818c04..0f39f6ce 100644 --- a/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md +++ b/plugins/uberdev/skills/premerge-pipeline/references/common-mistakes.md @@ -39,12 +39,17 @@ repair modified — work this loop made for itself. At `1.00` the loop is review its own output, and the response to that is to stop, not to repair again. **Reading `GROWTH=-` as `GROWTH=0.00`.** The dash means the ratio was not -measured, and four things produce it: (1) attempt 1, which has no previous repair +measured, and five things produce it: (1) attempt 1, which has no previous repair to attribute anything to; (2) a previous attempt that committed no edits, so the commit fence exited `REASON=no-edits` and wrote no `fix-scope-.modified`; -(3) an attempt with no blockers to divide by; and (4) a scope file that is -present but could not be read. Only the fourth is a fault, and it is the one an -operator otherwise misses: it announces itself as +(3) a previous attempt that repaired through the Phase 3 CI arm, which pushes +real code edits but publishes `ci-scope-.*` and no scope list this +measurement reads — the one cause that makes the stop unreachable, and the +subject of *Reading "the round is not measured"…* below; (4) an attempt with no +**reviewer-raised** blocker to divide by, which includes one holding only +`blocked_on_carry` rows, since a carried row counts on neither side; and (5) a +scope file that is present but could not be read. Only the fifth is a fault, and +it is the one an operator otherwise misses: it announces itself as `premerge-findings: repair_scope_unreadable:` on stderr and nowhere else, so a `-` on the line is a reason to go and check stderr, not only to note that the ratio was undefined. `0.00` by contrast IS a measurement — the previous repair @@ -62,23 +67,50 @@ either way — it is a not-green stop, so Phase 5 runs with `PREMERGE_SURVIVORS= and files every surviving blocker as an issue. The claim is "not *this loop's* work", never "not work". -**Firing the growth stop on a single round.** It takes two consecutive measured -attempts at or above `PREMERGE_GROWTH_CEILING`, and the second one is not -timidity. Every attempt's blocker set is a fresh sample of an unbounded -generator, and the files most likely to be resampled are exactly the ones the -last repair touched — so one high round is equally consistent with a convergence -whose repairs happen to be concentrated in a few files. A detector that fired on -it would stop loops that were working, and RFC 0021 A3 records that this — not a -detector falling silent — is the real failure mode. +**Firing the growth stop on a single round — or on the ratio alone.** Two ways of +under-counting the same test, and both stop loops that were working. A round is +**eligible** on **three** conditions, not one: `GROWTH=` is measured, it is at or +above `PREMERGE_GROWTH_CEILING`, **and** the round did not reduce the +reviewer-raised blocker count (`blockers >= previous_blockers`, read off the +previous attempt's own `converge.jsonl` row rather than a variable a fence could +forget). Then `PREMERGE_GROWTH_RUNS` consecutive rounds must clear all three. +The third condition is not decoration: `GROWTH=` answers what the residual is +made **of**, never which way the run is going, and since the denominator shrinks +as the loop succeeds while the reviewer keeps resampling the file the repair just +touched, the ratio RISES toward `1.00` exactly as a run converges. Shipped +without it, a measured 5 → 2 → 2 run read `0.50` twice and stopped with three of +its five repair rounds unspent. The second round is not timidity either — every +attempt's blocker set is a fresh sample of an unbounded generator, so one high +round is equally consistent with a convergence whose repairs happen to be +concentrated in a few files. RFC 0021 A3 records that stopping such a loop, not a +detector falling silent, is the real failure mode; a run log showing `GROWTH=0.90` +twice and no stop is the count condition doing its job, not the detector failing. **Making the growth ratio precise before making it used.** File-level membership -over-counts: a new finding anywhere in a touched file counts as growth, and a -round whose repair committed nothing is not measured at all. It is a signal, not -a proof, and that is deliberate — it is also why the stop needs two consecutive -rounds rather than one. The precise version needs diff-hunk mapping against a -review pointed at one repair's delta, and RFC 0021 A3 C8 records that no such -dispatch exists yet — building it here would replace a working coarse signal with -a keystone nothing can invoke. +over-counts: a new finding anywhere in a touched file counts as growth. It is a +signal, not a proof, and that is deliberate — it is also why the stop needs two +consecutive eligible rounds rather than one. The precise version needs diff-hunk +mapping against a review pointed at one repair's delta, and RFC 0021 A3 C8 records +that no such dispatch exists yet — building it here would replace a working coarse +signal with a keystone nothing can invoke. + +**Reading "the round is not measured" as "the round repaired nothing".** The one +inference that is worth more than the ratio's precision, because it is the case +where the detector is switched **off** rather than merely coarse. +`_repair_scope_path` opens `fix-scope-.modified` and nothing else, so a +*present, pushed, genuinely code-changing* repair still reads as no scope if it +published under another name — and one arm does exactly that. Phase 3's CI fence +on `arm=commit` dispatches `ci-code-fixer`, compares its diff against +`ci-scope-.allowed`, pushes, and publishes `ci-scope-.*` only. The next +attempt therefore prints `GROWTH=-` and is not eligible, and a loop that keeps +repairing through that arm can never accumulate two consecutive eligible rounds: +`STOP_SELF_REFERENTIAL` is structurally **unreachable** for it. Not hypothetical +— the run that surfaced this took that path at attempt 1. Nor is it closable from +the findings side: `ci-scope-.changed` is written before the scope comparison +and before the push, and attributing a round's findings to edits that may never +have left the machine trades one wrong measurement for another. The half that +closes it is a durable publish in the CI fence itself — SKILL.md, not the library +— and until that lands, `-` is the honest report and it stops nothing. **Reading `converge`'s exit status as success or failure.** `STOP_GREEN` exits 1. 1 means "stop", 0 means "go round again". Branch on `DECISION=`. diff --git a/tests/agent-description-budget.test.sh b/tests/agent-description-budget.test.sh index 26cea3ae..99c282ff 100644 --- a/tests/agent-description-budget.test.sh +++ b/tests/agent-description-budget.test.sh @@ -163,10 +163,24 @@ note_fail() { echo " FAIL $1"; FAIL=$((FAIL + 1)); } # (finding-verifier), which carries a real dispatch contract rather than # scaffolding. Set it any looser and it stops being a cap. # -# MAX_TOTAL is the post-#746 measured total plus a deliberate ~7% of headroom, -# so adding a normal-sized agent does not red CI on the day it lands, while a -# second ``-scale regression (thousands of chars) does. Measured -# post-#746 total: 11,720. +# MAX_TOTAL is the post-#746 measured total plus a deliberate ~7% of headroom +# (780 chars). Measured post-#746 total: 11,720. +# +# That headroom is for GROWING AN EXISTING description, not for adding an agent. +# Editing a description in place leaves the corpus size alone, so it never goes +# near A0 and A4 absorbs the growth without anyone re-pinning anything — while a +# second ``-scale regression (thousands of chars) still blows straight +# through it. Measured: growing one description by 348 chars lands A4 at +# 12,068/12,500 and every row stays green. +# +# ADDING an agent is gated by A0's exact pin, never by this headroom. A 48th +# file reds A0 the day it lands however small its description is — measured, a +# 48th agent at the corpus mean of 249 chars passes A4 with 531 chars to spare +# and reds A0 anyway — and the fix is to re-pin EXPECT_AGENTS and MAX_TOTAL in +# the same diff. That is the design and not an oversight: MAX_TOTAL is only +# meaningful as a calibration against a known corpus size, so the count it was +# calibrated against has to move with it, under the same human approval (see the +# EXACT-pin note above, and A0's own failure message). # # MIN_SKILLS / MIN_COMMANDS are floors for the two corpora A1 gained in #744. # Deliberately well below the live counts (30 skills, 18 commands) so a routine diff --git a/tests/ci-shard-diagnose.sh b/tests/ci-shard-diagnose.sh index e0e42d99..144f8459 100644 --- a/tests/ci-shard-diagnose.sh +++ b/tests/ci-shard-diagnose.sh @@ -7,6 +7,13 @@ # fewer fixtures than it was planned. Keeping the reporter separate from them # is what lets it run on a runner whose plan is already broken. # +# It prints, but it does not merely READ. `dump`'s section [C] carves the job's +# own `run_one` out of the live workflow and runs it, with every interpreter its +# invocations name stubbed out, so the routing verdict it reports is the one the +# job reaches rather than a transcription of the rule that reaches it. See "The +# live harness" below for why a copy there was worse than a copy anywhere else +# in this file. +# # WHY BOTH SIDES OF THE GITHUB_ENV BOUNDARY. The plan is produced by one step # and consumed by the next, and between them sits a file this repo's shell # writes and the runner reads back. Under Git Bash the packer's stdout is a @@ -161,10 +168,162 @@ ci_diag_make_boundary_shim() { printf '%s\n' "$ci_diag_tmp" } +# --------------------------------------------------------------------------- +# The live harness — for `dump`'s section [C] +# --------------------------------------------------------------------------- +# +# THE ROUTING RULE IS EXECUTED, NEVER TRANSCRIBED. [C] used to hand-write the +# sentinel-space membership test and run it over names parsed out of the plan. +# That was a COPY, and the worst-placed one in this file: `dump` is the verb CI +# actually runs, and both the SHARD-COVERAGE tail and tests/ci-shard-env.sh's +# refusal name it as the thing to read when a shard comes back short. A copy +# would keep agreeing with itself while the job's `run_one` drifted, and would +# report MATCH from a rule the job no longer runs — exonerating the exact bug +# the operator was sent here to find. +# +# It also could not model the other half of the rule. `run_one` keys on the +# ARGV of an invocation, not on a plan entry: it scans the arguments for the one +# that is a fixture path and strips it to a basename. A planned name therefore +# runs only if some invocation in that job hands run_one an argument that rule +# reads as that fixture — which is a fact about the job block, not about the +# plan string, and no amount of re-reading the plan can see it. +# +# So the block is carved out of the LIVE workflow and run. This is the shape +# tests/ci-shard.test.sh's S8/S10 already use against this same file, for the +# reason stated there: a copy here would keep agreeing with itself while CI +# drifted away from it. Nothing below re-implements the membership test; the +# only thing this file still decides is WHICH of the harness's own RUN/SKIP +# lines is about which planned name. +ci_diag_workflow="$ci_diag_dir/../.github/workflows/test.yml" + +ci_diag_harness_block() { # $1 = job key -> that job's run: block, invocations included + awk -v JOB="$1" ' + $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } + in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } + in_job && /^[[:space:]]+run:[[:space:]]*[|][[:space:]]*$/ { in_run = 1; next } + in_run { + if ($0 ~ /^[[:space:]]*$/) { print ""; next } + if ($0 !~ /^ /) { in_run = 0; next } + sub(/^ /, "") + print + } + ' "$ci_diag_workflow" +} + +# Runs that harness against the plan in THIS process's environment and writes +# its stdout+stderr to $2. rc: 0 ran, 2 could not be built, 3 extracted without +# invocations. +# +# The stubs are DERIVED from the invocation lines rather than listed here. A job +# that grows a fifth interpreter must not quietly turn this reporter into +# something that executes the suite — and a list kept here would be one more +# copy of the workflow to drift. Stubbing the leading word changes nothing the +# routing reads: `run_one` still receives the whole argv, `env MSYSTEM=MSYS bash +# tests/x.test.sh` included, and still prints it on the RUN/SKIP line. +# +# The harness's own rc is dropped on purpose. Its SHARD-COVERAGE tail exits +# non-zero on a short run, and a short run is the FINDING this section exists to +# print, not a failure of the dump. `dump` stays a reporter, never a gate. +ci_diag_harness_decisions() { # $1 = job key, $2 = destination file + [ -r "$ci_diag_workflow" ] || return 2 + ci_diag_hdir="$(mktemp -d)" || return 2 + ci_diag_harness_block "$1" >"$ci_diag_hdir/block" || { rm -rf "$ci_diag_hdir"; return 2; } + ci_diag_inv="$(awk '$1 == "run_one" { n++ } END { print n + 0 }' "$ci_diag_hdir/block")" + [ "${ci_diag_inv:-0}" -ge 1 ] || { rm -rf "$ci_diag_hdir"; return 3; } + { + awk '$1 == "run_one" && $2 != "" && !ci_diag_seen[$2]++ { printf "%s() { :; }\n", $2 }' \ + "$ci_diag_hdir/block" + cat "$ci_diag_hdir/block" + } >"$ci_diag_hdir/harness.sh" || { rm -rf "$ci_diag_hdir"; return 2; } + # `-e` is what the runner uses for a `run: |` block on all three platforms. + bash -e "$ci_diag_hdir/harness.sh" >"$2" 2>&1 + rm -rf "$ci_diag_hdir" + return 0 +} + # --------------------------------------------------------------------------- # Verbs # --------------------------------------------------------------------------- +# [C] of `dump`: what the job's OWN harness did with the plan this step +# inherited, one line per planned name, in plan order. +ci_diag_dump_routing() { + echo "--- [C] routing verdict: the plan in [B] driven through the $ci_diag_job job's OWN run_one" + ci_diag_probe="$(mktemp -d)" || { + echo " !! no scratch directory; no routing verdict" + return 0 + } + # Split exactly as before: CRs deleted, then spaces. This decides only which + # names to ASK about — every verdict below comes from the harness. + printf '%s' "${UBERDEV_SHARD_PLAN-}" | tr -d "$ci_diag_cr" | tr ' ' '\n' >"$ci_diag_probe/names" + ci_diag_harness_decisions "$ci_diag_job" "$ci_diag_probe/decisions" + ci_diag_hrc=$? + if [ "$ci_diag_hrc" -ne 0 ]; then + # NO FALLBACK, deliberately. Re-implementing the rule here when the carve-out + # fails would restore the copy this section exists not to be, and it would do + # it at the one moment the workflow is known to have moved. + echo " !! the $ci_diag_job harness could not be carved out of $ci_diag_workflow (rc=$ci_diag_hrc)." + echo " No routing verdict. tests/ci-shard.test.sh S8/S10 gate that extraction and will name the cause." + rm -rf "$ci_diag_probe" + return 0 + fi + ci_diag_cov="$(awk '/^=== SHARD-COVERAGE shard=/ { ci_diag_line = $0 } END { print ci_diag_line }' \ + "$ci_diag_probe/decisions")" + # Matched before it is stripped. A bare `##*ran=` on a line that carries no + # `ran=` returns the WHOLE line, which reads as 0 and would raise the drift + # alarm below against a run that never disagreed with anything. + case "$ci_diag_cov" in + *ran=*) ci_diag_ran="${ci_diag_cov##*ran=}" ;; + *) ci_diag_ran="" ;; + esac + # Pass 1 reads the harness's verdicts; pass 2 walks the planned names. The + # files are told apart by FILENAME rather than by FNR == NR, which silently + # mis-files the second file when the first one comes back empty. + # + # A name is matched against the BASENAME of each argument the harness printed, + # which is line identification, not the routing rule: if run_one's key ever + # stopped being the basename of a tests/ path, the verdict read off that line + # would still be the one the job reached. + awk -v DEC="$ci_diag_probe/decisions" -v RAN="$ci_diag_ran" ' + FILENAME == DEC { + if ($1 == "---" && ($2 == "RUN" || $2 == "SKIP")) { + for (i = 3; i <= NF; i++) { + tok = $i + sub(/^.*\//, "", tok) + seen[tok] = 1 + if ($2 == "RUN") ran[tok] = 1 + } + } + next + } + { + name = $1 + if (name == "") next + if (name in ran) { verdict = "MATCH"; m++ } + else if (name in seen) { verdict = "MISS"; s++ } + else { verdict = "UNWIRED"; u++ } + printf " %-5s %s\n", verdict, name + } + END { + printf " match=%d miss=%d\n", m + 0, s + 0 + if (u + 0 > 0) { + printf " unwired=%d: planned, but no invocation in this job ever handed that name to run_one.\n", u + 0 + } + # The harness counts what it ran itself. Two readings of one run that + # disagree mean this reporter has drifted from the job, which is the one + # failure a probe like this cannot be allowed to make quietly. + if (RAN != "" && RAN + 0 != m + 0) { + printf " !! this section read %d planned name(s) as run; the harness tallied ran=%s on the SAME run.\n", m + 0, RAN + } + } + ' "$ci_diag_probe/decisions" "$ci_diag_probe/names" + [ -z "$ci_diag_cov" ] || printf ' [harness] %s\n' "$ci_diag_cov" + echo " A plan of length N with match=1 is the #753 signature: only the entry" + echo " whose trailing CR was consumed at the file boundary is space-bounded." + rm -rf "$ci_diag_probe" + return 0 +} + ci_diag_dump() { echo "=== ci-shard-diagnose dump: $ci_diag_job shard $ci_diag_shard/$ci_diag_shards" # $BASH_VERSION rather than a `bash --version` pipeline into an @@ -190,22 +349,7 @@ ci_diag_dump() { echo " (unset in this step's environment)" fi - echo "--- [C] membership probe, using the harness's own rule against [B]" - ci_diag_hit=0 - ci_diag_miss=0 - while IFS= read -r ci_diag_name; do - [ -n "$ci_diag_name" ] || continue - case " ${UBERDEV_SHARD_PLAN-} " in - *" ${ci_diag_name} "*) ci_diag_hit=$((ci_diag_hit + 1)); ci_diag_verdict=MATCH ;; - *) ci_diag_miss=$((ci_diag_miss + 1)); ci_diag_verdict=MISS ;; - esac - printf ' %-5s %s\n' "$ci_diag_verdict" "$ci_diag_name" - done <&2 echo "${0##*/}: a CR-delimited plan makes the shard filter match only the last entry (#753)." >&2 - echo "${0##*/}: dump the bytes with: bash $ci_shard_env_dir/ci-shard-diagnose.sh dump $ci_shard_env_job $ci_shard_env_shards $ci_shard_env_shard" >&2 + echo "${0##*/}: dump the packer's own bytes with: bash \"$ci_shard_env_dir/ci-shard-diagnose.sh\" packer-bytes $ci_shard_env_job $ci_shard_env_shards $ci_shard_env_shard" >&2 + echo "${0##*/}: CRLF there means ci-shard-plan.py's newline pin regressed; LF means the CR entered below that pin." >&2 + echo "${0##*/}: not the 'dump' verb here: it re-runs this same refusal and reads a plan this step never emitted, so all three of its sections come back empty." >&2 exit 2 ;; esac diff --git a/tests/ci-shard.test.sh b/tests/ci-shard.test.sh index 07eec57c..6900486a 100644 --- a/tests/ci-shard.test.sh +++ b/tests/ci-shard.test.sh @@ -52,6 +52,56 @@ plan_shard() { # plan_shard N K -> that shard's fixtures on stdout python3 -I -B "$PLAN" --shards "$1" --shard "$2" --weights "$WORK/weights.tsv" <"$WORK/corpus" } +# ONE reader per shape of the workflow, shared by every section below. +# +# The job anchor, the stop-at-the-next-job rule and the ten-space de-indent the +# `run: |` block scalar carries are ONE contract about the workflow's YAML, and +# a second transcription of it is the "one contract, N uncompared copies" class +# tests/ci-wiring.test.sh W12.4 exists to refuse: a fix to the shape lands on +# one copy, leaves the other narrow, and every row built on the narrow one then +# passes over a truncated block instead of failing. + +# run_block_of -> that job's `run: |` body, de-indented. +# marker NON-EMPTY every `run_one` line is replaced by ONE marker line, so +# synthetic invocations can be spliced in at that point +# marker EMPTY the block exactly as the workflow ships it, invocations +# included +run_block_of() { + awk -v JOB="$1" -v MARK="$2" ' + $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } + in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } + in_job && /^[[:space:]]+run:[[:space:]]*[|][[:space:]]*$/ { in_run = 1; next } + in_run { + if ($0 ~ /^[[:space:]]*$/) { print ""; next } + if ($0 !~ /^ /) { in_run = 0; next } + sub(/^ /, "") + if (MARK != "" && $0 ~ /^run_one /) { if (!ins) { print MARK; ins = 1 } next } + print + } + ' "$WORKFLOW" +} + +# matrix_shards_of -> how many shards that job's matrix declares, or +# the empty string. Read out of the job's own `shard: [1, 2, ...]` row. +matrix_shards_of() { + awk -v JOB="$1" ' + $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } + in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } + in_job && /^[[:space:]]+shard:[[:space:]]*\[/ { print gsub(/,/, ",") + 1; exit } + ' "$WORKFLOW" +} + +# DECLARE, THEN COMPARE — the order tests/ci-wiring.test.sh W13.0 uses, and the +# reason these are constants rather than literals scattered through the rows. +# Every row below plans for these counts; S10 checks each one against the job's +# OWN matrix before any row is allowed to trust it. Written as literals in each +# row instead, a job resharded in the workflow would leave those rows planning +# for a count that is no longer shipped — and their properties still hold for +# that count, so they would stay green over a configuration nobody runs. +LINUX_SHARDS=6 +MACOS_SHARDS=2 +WINDOWS_SHARDS=6 + echo "== S1: the pack is complete and disjoint ==" : >"$WORK/union" S1_RC=0 @@ -139,7 +189,7 @@ s5_refuses "ci-shard-env with a non-numeric shard" \ sh -c "bash '$ENVSH' shape-checks 6 two" echo "== S6: ci-shard-env emits exactly the three variables the harness reads ==" -if bash "$ENVSH" supervision-smoke-macos 2 1 >"$WORK/env.out" 2>"$WORK/env.err"; then +if bash "$ENVSH" supervision-smoke-macos "$MACOS_SHARDS" 1 >"$WORK/env.out" 2>"$WORK/env.err"; then S6_KEYS="$(sed -n 's/^\([A-Z_]*\)=.*/\1/p' "$WORK/env.out" | tr '\n' ',')" [ "$S6_KEYS" = "UBERDEV_SHARD_PLAN,UBERDEV_SHARD,UBERDEV_SHARDS," ] \ && ok "S6: the three GITHUB_ENV keys are present and in order" \ @@ -188,20 +238,6 @@ echo "== S8: the WORKFLOW'S OWN run_one routes by the plan ==" # shape, so the rows below keep measuring the harness rather than the position # they happened to be pasted at. S8_MARK='@@S8-INVOCATIONS@@' -s8_harness() { # s8_harness -> the run: block, invocations replaced by ONE marker - awk -v JOB="$1" -v MARK="$S8_MARK" ' - $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } - in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } - in_job && /^[[:space:]]+run:[[:space:]]*[|][[:space:]]*$/ { in_run = 1; next } - in_run { - if ($0 ~ /^[[:space:]]*$/) { print ""; next } - if ($0 !~ /^ /) { in_run = 0; next } - sub(/^ /, "") - if ($0 ~ /^run_one /) { if (!ins) { print MARK; ins = 1 } next } - print - } - ' "$WORKFLOW" -} s8_script() { # s8_script # FAILS LOUDLY, never splices into nothing. A marker that moved, or an empty # invocation file, would write a harness that invokes NOTHING -- and every row @@ -224,7 +260,7 @@ s8_script() { # s8_script ' "$1" >"$3" \ || { echo "FATAL: ${0##*/}: $S8_MARK did not splice exactly one non-empty invocation set into $1" >&2; exit 2; } } -S8_BODY="$(s8_harness shape-checks)" +S8_BODY="$(run_block_of shape-checks "$S8_MARK")" if [ -z "$S8_BODY" ]; then bad "S8: the ubuntu harness extracted empty — the job key or the block shape moved" else @@ -264,6 +300,31 @@ else *"SKIP bash tests/not-in-plan.test.sh"*) ok "S8: an unplanned fixture is SKIPPED" ;; *) bad "S8: an unplanned fixture was not skipped — the shards would each run everything" ;; esac + + # Polarity: with no plan the SAME harness must run every row. This is the + # standalone case ci-wiring W12.3 depends on, asserted here at the routing seam. + # + # THE UNSET IS EXPLICIT, and it has to be. This fixture runs INSIDE a shard, + # where UBERDEV_SHARD_PLAN is exported by the planning step -- so a subshell + # that merely declines to set it inherits the real plan, which holds none of + # the synthetic names below and skips all six. It passed locally for the only + # reason such a row ever does: the variable happened not to exist in the + # author's shell. + S8_ALL="$( + unset UBERDEV_SHARD_PLAN UBERDEV_SHARD UBERDEV_SHARDS + bash -e "$WORK/s8.sh" 2>&1 || true + )" + S8_N="$(printf '%s\n' "$S8_ALL" | grep -c '^--- RUN ')" + [ "$S8_N" = "6" ] && ok "S8: with no plan set, all six run (the standalone case)" \ + || bad "S8: an unset plan ran $S8_N of 6 — the default is not fail-open" + + # S9 — the routing KEY, driven through the same extracted harness. + # + # These rows carry their own identifier and therefore their own header: printed + # under S8's, twelve S9 rows would read as S8's and a row shipped as a + # duplicate of its predecessor would have nothing to distinguish it, which is + # the row-id ambiguity ci-wiring's own anti-vacuity row exists to refuse. + echo "== S9: the routing key survives every argv shape the workflow uses ==" case "$S8_OUT" in *"RUN bash tests/flagged.test.sh --artifact-publication-only"*) ok "S9: a fixture with a TRAILING FLAG runs (the key is not the last argument)" ;; @@ -328,23 +389,6 @@ else ok "S9: an argv naming no fixture runs — W12.3's synthetic fixtures survive the filter" ;; *) bad "S9: a non-fixture argv was skipped — ci-wiring W12.3 would red" ;; esac - - # Polarity: with no plan the SAME harness must run every row. This is the - # standalone case ci-wiring W12.3 depends on, asserted here at the routing seam. - # - # THE UNSET IS EXPLICIT, and it has to be. This fixture runs INSIDE a shard, - # where UBERDEV_SHARD_PLAN is exported by the planning step -- so a subshell - # that merely declines to set it inherits the real plan, which holds none of - # the synthetic names below and skips all six. It passed locally for the only - # reason such a row ever does: the variable happened not to exist in the - # author's shell. - S8_ALL="$( - unset UBERDEV_SHARD_PLAN UBERDEV_SHARD UBERDEV_SHARDS - bash -e "$WORK/s8.sh" 2>&1 || true - )" - S8_N="$(printf '%s\n' "$S8_ALL" | grep -c '^--- RUN ')" - [ "$S8_N" = "6" ] && ok "S8: with no plan set, all six run (the standalone case)" \ - || bad "S8: an unset plan ran $S8_N of 6 — the default is not fail-open" fi echo "== S10: every shard of every job runs its whole plan (aggregate) ==" @@ -357,19 +401,6 @@ echo "== S10: every shard of every job runs its whole plan (aggregate) ==" # tests/ci-job-fixtures.sh, which reads the run_one lines back out of the # workflow -- a guard that re-derived the count the way the harness does would # agree with the harness by construction and see nothing. -s10_block() { # $1 = job key -> the run: block WITH its invocations - awk -v JOB="$1" ' - $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } - in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } - in_job && /^[[:space:]]+run:[[:space:]]*[|][[:space:]]*$/ { in_run = 1; next } - in_run { - if ($0 ~ /^[[:space:]]*$/) { print ""; next } - if ($0 !~ /^ /) { in_run = 0; next } - sub(/^ /, "") - print - } - ' "$WORKFLOW" -} # The four interpreters the workflow's run_one lines use, stubbed so the # replay costs nothing. `--- RUN` is echoed BEFORE the invocation, so the # count is unaffected by the stub. @@ -379,7 +410,7 @@ s10_runner() { # $1 = job key -> path to a replayable script printf 'zsh() { :; }\n' printf 'python() { :; }\n' printf 'env() { shift; "$@"; }\n' - s10_block "$1" + run_block_of "$1" '' } >"$WORK/s10.$1.sh" printf '%s\n' "$WORK/s10.$1.sh" } @@ -391,13 +422,27 @@ while IFS= read -r s10_spec; do [ -n "$s10_spec" ] || continue s10_job="${s10_spec%%|*}" s10_shards="${s10_spec##*|}" - s10_wired="$(bash "$FIXTURES" "$s10_job" | grep -c .)" + # DECLARE, THEN COMPARE. The count above comes from this file's own constants; + # nothing below may plan for it until the job's own matrix agrees. A job + # resharded in the workflow reds here — and stops, rather than measuring a + # shard count that is no longer shipped and calling the result green. + s10_matrix="$(matrix_shards_of "$s10_job")" + if [ "$s10_matrix" != "$s10_shards" ]; then + bad "S10: $s10_job's matrix declares ${s10_matrix:-} shard(s), this file plans for $s10_shards — reshard the constants at the top" + continue + fi + # ONE extraction per job, not one per shard. The wired list is a pure function + # of (job key, workflow file) and neither moves during the run, so re-deriving + # it inside the shard loop bought a fresh process tree per shard and nothing + # else. Both consumers below read the same capture. + s10_fixtures="$(bash "$FIXTURES" "$s10_job")" + s10_wired="$(printf '%s\n' "$s10_fixtures" | grep -c .)" s10_script="$(s10_runner "$s10_job")" s10_total=0 s10_bad='' s10_k=1 while [ "$s10_k" -le "$s10_shards" ]; do - s10_plan="$(bash "$FIXTURES" "$s10_job" | python3 -I -B "$PLAN" --shards "$s10_shards" --shard "$s10_k")" + s10_plan="$(printf '%s\n' "$s10_fixtures" | python3 -I -B "$PLAN" --shards "$s10_shards" --shard "$s10_k")" s10_len="$(printf '%s\n' "$s10_plan" | grep -c .)" s10_flat="$(printf '%s' "$s10_plan" | tr '\n' ' ')" s10_run="$(s10_run_count "$s10_script" "$s10_flat" "$s10_k" "$s10_shards")" @@ -416,9 +461,9 @@ while IFS= read -r s10_spec; do bad "S10: $s10_job executed $s10_total of $s10_wired wired fixtures — $((s10_wired - s10_total)) never ran in ANY shard" fi done <&1)" +S11_VALUE="$(bash "$REPO_ROOT/tests/ci-shard-diagnose.sh" crlf-plan "$S11_JOB" "$WINDOWS_SHARDS" 2)" +S11_LEN="$(bash "$FIXTURES" "$S11_JOB" | python3 -I -B "$PLAN" --shards "$WINDOWS_SHARDS" --shard 2 | grep -c .)" +S11_OUT="$(UBERDEV_SHARD_PLAN="$S11_VALUE" UBERDEV_SHARD=2 UBERDEV_SHARDS="$WINDOWS_SHARDS" bash -e "$S11_SCRIPT" 2>&1)" S11_RC=$? S11_RUN="$(printf '%s\n' "$S11_OUT" | grep -c '^--- RUN ')" # The `-n` is not decoration. An EMPTY value is read by the harness as an unset @@ -458,10 +503,10 @@ echo "== S12: the aggregate comparison is not vacuous ==" # one entry from a shard's plan is the smallest defect it must catch. S12_JOB=shape-checks-windows S12_SCRIPT="$(s10_runner "$S12_JOB")" -S12_FULL="$(bash "$FIXTURES" "$S12_JOB" | python3 -I -B "$PLAN" --shards 6 --shard 3)" +S12_FULL="$(bash "$FIXTURES" "$S12_JOB" | python3 -I -B "$PLAN" --shards "$WINDOWS_SHARDS" --shard 3)" S12_LEN="$(printf '%s\n' "$S12_FULL" | grep -c .)" S12_SHORT="$(printf '%s\n' "$S12_FULL" | sed '$d' | tr '\n' ' ')" -S12_RUN="$(s10_run_count "$S12_SCRIPT" "$S12_SHORT" 3 6)" +S12_RUN="$(s10_run_count "$S12_SCRIPT" "$S12_SHORT" 3 "$WINDOWS_SHARDS")" if [ "$S12_RUN" = "$((S12_LEN - 1))" ] && [ "$S12_RUN" != "$S12_LEN" ]; then ok "S12: a plan one fixture short runs one fixture fewer — S10's comparison can see it" else @@ -479,7 +524,7 @@ echo "== S13: the producer refuses a pack its packer delivered with CRs ==" # own stdout -- see the TWO SHIMS note in tests/ci-shard-diagnose.sh. A row fed # only through the producer it is meant to test stops testing it the moment # that producer is fixed. -S13_OUT="$(bash "$REPO_ROOT/tests/ci-shard-diagnose.sh" simulate shape-checks-windows 6 2 2>"$WORK/s13.err")" +S13_OUT="$(bash "$REPO_ROOT/tests/ci-shard-diagnose.sh" simulate shape-checks-windows "$WINDOWS_SHARDS" 2 2>"$WORK/s13.err")" S13_RC=$? if [ "$S13_RC" -ne 0 ]; then ok "S13: ci-shard-env.sh refuses a CR-bearing pack (rc=$S13_RC)" @@ -507,7 +552,7 @@ echo "== S14: the packer pins its own stdout to LF ==" # would hand $? the status of `tr`, so a packer that died would arrive as an # empty haystack containing no CR -- and the row would report PASS for the one # reason that is not a pass. -bash "$REPO_ROOT/tests/ci-shard-diagnose.sh" packer-bytes shape-checks-windows 6 2 \ +bash "$REPO_ROOT/tests/ci-shard-diagnose.sh" packer-bytes shape-checks-windows "$WINDOWS_SHARDS" 2 \ >"$WORK/s14.bytes" 2>"$WORK/s14.err" S14_RC=$? if [ "$S14_RC" -ne 0 ] || [ ! -s "$WORK/s14.bytes" ]; then @@ -554,13 +599,6 @@ s15_dump_line_of() { # $1 = job key -> that job's dump step, leading indent stri in_job && $0 ~ PAT { sub(/^[[:space:]]*/, ""); sub(/\r$/, ""); print; exit } ' "$WORKFLOW" } -s15_matrix_shards_of() { # $1 = job key -> how many shards the job's matrix declares - awk -v JOB="$1" ' - $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } - in_job && /^ [[:alnum:]_-]+:[[:space:]]*$/ { exit } - in_job && /^[[:space:]]+shard:[[:space:]]*\[/ { print gsub(/,/, ",") + 1; exit } - ' "$WORKFLOW" -} s15_lineno_of() { # $1 = job key, $2 = ERE -> line number of its first match inside the job awk -v JOB="$1" -v PAT="$2" ' $0 ~ ("^ " JOB ":[[:space:]]*$") { in_job = 1; next } @@ -569,7 +607,7 @@ s15_lineno_of() { # $1 = job key, $2 = ERE -> line number of its first match ins ' "$WORKFLOW" } for s15_job in shape-checks supervision-smoke-macos shape-checks-windows; do - s15_shards="$(s15_matrix_shards_of "$s15_job")" + s15_shards="$(matrix_shards_of "$s15_job")" s15_line="$(s15_dump_line_of "$s15_job")" s15_want="run: bash tests/ci-shard-diagnose.sh dump $s15_job $s15_shards \"\${{ matrix.shard }}\"" if [ -z "$s15_shards" ]; then diff --git a/tests/model-routing.test.sh b/tests/model-routing.test.sh index 55047309..f3194fb8 100644 --- a/tests/model-routing.test.sh +++ b/tests/model-routing.test.sh @@ -242,6 +242,27 @@ check(policy["delegation_contracts"]["leaf"] == {"features_multi_agent": False, # FAILURE, so the list cannot rot into a permanent exemption, and a NEW agent # shipped with no declaration reds instead of silently joining them. FRONTMATTER_RE = re.compile(r"(?s)\A---\n(.*?)\n---\n") +# WHAT COUNTS AS A DECLARATION. This has to be the SAME answer declared_tools() +# gives in tests/testers-agent-contract.test.sh -- that function and this block +# ask one question of one set of cards, and the eight testers personas are inside +# both subjects. +# +# Bare key PRESENCE is not that answer. `tools:` with nothing after it and no +# block-sequence items is YAML null: the loader grants EVERY tool, exactly as if +# the key were absent. Keying on presence therefore made every row below vacuous +# on precisely the state #749 exists to red on -- measured on this tree by +# emptying the value on a live testers card, the role stayed in declares_tools, +# so the testers-subset row and the read-only projection row both still printed +# PASS while that agent held every tool. It is also the exact spelling a +# half-finished migration produces, which is what this stack IS. +# +# So a declaration is a `tools:` key with a NON-EMPTY value, in either YAML +# spelling of a list -- a same-line flow value, or the block-sequence item lines +# that follow an empty same-line value. ONLY `- item` lines are absorbed: an +# indented continuation of another key's folded scalar is not a tool declaration. +# Widening WHICH SPELLINGS count is not a relaxation; requiring the value be +# non-empty is the strictness the guard was always meant to have. +TOOLS_RE = re.compile(r"(?m)^tools:[ \t]*(.*(?:\n[ \t]*-[ \t]+\S.*)*)$") declares_tools = set() declares_allowed_tools = set() for agent_path in sorted((ROOT / "plugins/uberdev/agents").glob("*.md")): @@ -250,7 +271,8 @@ for agent_path in sorted((ROOT / "plugins/uberdev/agents").glob("*.md")): block = matched.group(1) named = re.search(r"(?m)^name:[ \t]*(\S+)", block) role = named.group(1).strip() if named else agent_path.stem - if re.search(r"(?m)^tools:", block): + declared = TOOLS_RE.search(block) + if declared is not None and declared.group(1).strip(): declares_tools.add(role) if re.search(r"(?m)^allowed-tools:", block): declares_allowed_tools.add(role) diff --git a/tests/premerge-findings.test.sh b/tests/premerge-findings.test.sh index a2893852..a0fc9a0c 100644 --- a/tests/premerge-findings.test.sh +++ b/tests/premerge-findings.test.sh @@ -2257,9 +2257,9 @@ scope() { # substitution runs in a subshell, so an rc stashed inside never reaches the # caller -- and rc is half of what is under test here, since a stop exits 1. # -# stderr is kept in DIR/g.err rather than discarded, because one of the four -# routes to an unmeasured ratio -- an OSError on a scope file that IS present -- -# is indistinguishable from the three benign ones by its `-` alone. Its +# stderr is kept in DIR/g.err rather than discarded, because one of the five +# causes of an unmeasured ratio -- an OSError on a scope file that IS present -- +# is indistinguishable from the four benign ones by its `-` alone. Its # announcement is the only thing that separates "we could not look" from "there # was nothing to look at". decide_g() { @@ -2380,15 +2380,28 @@ plan_at "$D" 2 "$SHA_B" || bad "B32: survivor setup 2 failed" expect_g "$(decide_g "$D" 2 6 not_green 'blockers_remaining=2')" 'CONTINUE|0.00|0' \ "a survivor in the repaired file plus a new blocker elsewhere is 0.00" -# ---- the FOUR routes to `-`, each driven, none of them a stop --------------- +# ---- the FIVE causes of `-`, over the FOUR branches, none of them a stop ---- # -# `-` is not `0.00`: it says the ratio was not measured. Four distinct -# conditions produce it, and a block that only exercised one of them would pass -# just as happily over the other three being dead code. +# `-` is not `0.00`: it says the ratio was not measured. SKILL.md `## Phase 3b` +# and `_read_repair_scope`'s docstring name FIVE causes, and they reach FOUR +# distinct branches: (2) and (3) below arrive at the SAME absent-file return, so +# the one row driven for (2) is the driven test for both. A block that exercised +# only one BRANCH would pass just as happily over the other three being dead +# code -- which is why the count that matters here is branches, not causes. # (1) attempt 1: there is no predecessor to attribute anything to. # (2) a predecessor that committed nothing, so the commit fence wrote no scope -# list (`REASON=no-edits`) or the repair only re-ran CI. +# list (`REASON=no-edits`) or the repair only re-ran CI (`arm=rerun`). +# (3) a predecessor that repaired through the CI fence's `arm=commit` path: real +# code edits, really pushed, but that fence publishes `ci-scope-.*` and +# never `fix-scope-.modified`. It is the one cause that switches the +# detector OFF -- such a loop never accumulates two consecutive eligible +# rounds, so STOP_SELF_REFERENTIAL is unreachable for it until that fence +# publishes a scope of its own. It gets no case of its own because it has no +# BRANCH of its own: `_read_repair_scope` stats exactly one path, no +# `ci-scope-*` file is it, and the row below returns None from the same +# `os.path.exists` arm. An earlier version of this header counted four and +# left this cause out; the enumeration was short, the coverage was not. D_DASH="$(new_case)" write_input "$D_DASH" '[{"file":"lib/a.sh","line":1,"summary":"alpha","failure_scenario":"crash","severity":"blocker"}]' plan_at "$D_DASH" 1 "$SHA_A" || bad "B32: unmeasured setup 1 failed" @@ -2397,7 +2410,7 @@ expect_g "$(decide_g "$D_DASH" 1 6 not_green 'blockers_remaining=1')" 'CONTINUE| write_input "$D_DASH" '[{"file":"lib/q.sh","line":1,"summary":"quebec","failure_scenario":"crash","severity":"blocker"}]' plan_at "$D_DASH" 2 "$SHA_B" || bad "B32: unmeasured setup 2 failed" expect_g "$(decide_g "$D_DASH" 2 6 not_green 'blockers_remaining=1')" 'CONTINUE|-|0' \ - "an attempt whose predecessor wrote no scope list reports no ratio" + "a predecessor that published no scope list -- no-edits or CI-arm -- reports no ratio" # And the field is on the line even when it has no value: a line whose field set # varies is a line no `case` pattern can match. @@ -2408,7 +2421,7 @@ case "$B32_LINE" in *) bad "B32: the GROWTH field is missing or misplaced: $B32_LINE" ;; esac -# (3) an attempt with NO blockers to divide by. The scope list is present and +# (4) an attempt with NO blockers to divide by. The scope list is present and # the predecessor's evidence is on disk, so the only thing standing between # this row and a ZeroDivisionError traceback is the empty-denominator guard # -- which is why this row asserts rc=0 as well as `-`. @@ -2422,10 +2435,10 @@ plan_at "$D" 2 "$SHA_B" || bad "B32: zero-blocker setup 2 failed" expect_g "$(decide_g "$D" 2 6 not_green 'ci=red')" 'CONTINUE|-|0' \ "an attempt with no blockers reports no ratio and does not divide by zero" -# (4) an OSError on a scope file that IS present. This one is a FAULT, not an +# (5) an OSError on a scope file that IS present. This one is a FAULT, not an # absence, and it is the only `-` an operator has to act on -- so it # announces itself on stderr with a token while degrading exactly like the -# other three. A directory at the scope path is the portable way to make +# other four. A directory at the scope path is the portable way to make # `os.path.exists` true and `open()` raise, with no dependence on the test # running as an unprivileged user. D_ERR="$(new_case)" diff --git a/tests/premerge.test.sh b/tests/premerge.test.sh index eabbfe08..9e6c4be3 100644 --- a/tests/premerge.test.sh +++ b/tests/premerge.test.sh @@ -78,7 +78,11 @@ # exactly one agent owns it. Exactly one producer, and no fence names # the blocked-on artifact # P22c — the widening is declared on both surfaces, capped, and reads exactly -# one predecessor; both bypass entries are in common-mistakes.md +# one predecessor; both bypass entries are in common-mistakes.md. The +# cap is compared VALUE-for-value with the library's MAX_BLOCKED_ON, +# and the wave size the SKILL says it equals is compared with what the +# fence actually passes as --max-per-wave — a name grep on either side +# is green at any value (#370/#371) set -u @@ -1334,6 +1338,80 @@ assert_fixed "$FENCES" '--carry-blocked' "P22c: a fence turns the widening on" assert_fixed "$FENCES" '--repo-root "$PREMERGE_ROOT"' "P22c: and hands it a root to contain against" assert_fixed "$SKILL" 'PREMERGE_MAX_BLOCKED_ON' "P22c: the SKILL declares the cap" assert_fixed "$LIB" 'MAX_BLOCKED_ON' "P22c: the library declares the cap it enforces" +# ...and the VALUE behind both those names. The two rows above are +# `assert_fixed` — a fixed-string grep for the identifier, on each side — so the +# pair stayed green at any value and the two copies were free to drift the +# moment either moved: the SKILL's number is what a reader plans against, the +# library's is what actually drops rows. That is the #370/#371 "one contract, N +# uncompared copies" class P3b names above, and this stack's other restated +# constants are not exposed to it — PREMERGE_REPAIR_CEILING / +# PREMERGE_WAIT_CI_CEILING and the growth pair are compared value-for-value in +# tests/premerge-findings.test.sh (B15b, B32). This one is compared here, +# beside the Constants block's other locks (P19, P20). +# +# EVALUATE the library's constant, never parse it: P3b states that rule for this +# repo, and the cross-platform import recipe is P3b's too — cd into the lib dir, +# hand python3 a RELATIVE filename (a Git Bash absolute path is not openable by +# native Windows python), strip the CR native-Windows python writes. +P22C_ERR="$(mktemp)" +P22C_LIB_CAP="$( (cd "$LIB_DIR" && python3 -c " +import importlib.util +spec = importlib.util.spec_from_file_location('premerge_findings', '$LIB_BASE') +mod = importlib.util.module_from_spec(spec) +spec.loader.exec_module(mod) +print(mod.MAX_BLOCKED_ON) +") 2>"$P22C_ERR" | tr -d '\r')" +# Spaced (`NAME = 8`) is the documentation, as in P19: the Constants block is the +# only copy of this number the prose points at. +P22C_DOC_CAP="$(sed -n 's/^PREMERGE_MAX_BLOCKED_ON[[:space:]][[:space:]]*=[[:space:]]*\([0-9][0-9]*\).*$/\1/p' "$SKILL" | sed -n '1p')" +case "$P22C_LIB_CAP" in + ''|*[!0-9]*) + echo " FAIL P22c: could not evaluate MAX_BLOCKED_ON from the library (got '$P22C_LIB_CAP')" + [ -s "$P22C_ERR" ] && echo " python stderr: $(tr '\n' ' ' <"$P22C_ERR")" + FAIL=$((FAIL + 1)) ;; + *) + if [ -z "$P22C_DOC_CAP" ]; then + echo " FAIL P22c: the Constants block declares no PREMERGE_MAX_BLOCKED_ON value"; FAIL=$((FAIL + 1)) + elif [ "$P22C_DOC_CAP" != "$P22C_LIB_CAP" ]; then + echo " FAIL P22c: Constants says PREMERGE_MAX_BLOCKED_ON=$P22C_DOC_CAP, the library enforces $P22C_LIB_CAP" + FAIL=$((FAIL + 1)) + else + echo " PASS P22c: the SKILL's restated cap == the library's enforced MAX_BLOCKED_ON ($P22C_LIB_CAP)" + PASS=$((PASS + 1)) + fi ;; +esac +rm -f "$P22C_ERR" + +# The equality the SKILL argues from. It says the cap is equal to +# PREMERGE_MAX_FIX_WAVE "on purpose" — the widening is one extra wave's worth of +# files rather than a number someone picked — and that reasoning is the entire +# justification for the value. Nothing enforced it: PREMERGE_MAX_FIX_WAVE exists +# on its own Constants row and inside the sentence claiming the equality, and +# nowhere else in tests/ or lib/, so the argument could go false with no row +# moving. What makes the wave size real is the fence, which passes +# `--max-per-wave` EXPLICITLY rather than leaning on the library's argparse +# default — so the fence's literal, not the default, is what the cap must equal. +P22C_DOC_WAVE="$(sed -n 's/^PREMERGE_MAX_FIX_WAVE[[:space:]][[:space:]]*=[[:space:]]*\([0-9][0-9]*\).*$/\1/p' "$SKILL" | sed -n '1p')" +P22C_FENCE_WAVES="$(sed -n 's/^[[:space:]]*--max-per-wave[[:space:]][[:space:]]*\([0-9][0-9]*\).*$/\1/p' "$FENCES")" +P22C_FENCE_WAVE="$(sort -u <<<"$P22C_FENCE_WAVES")" +P22C_FENCE_WAVE_N="$(grep -c . <<<"$P22C_FENCE_WAVE")" +if [ -z "$P22C_DOC_WAVE" ]; then + echo " FAIL P22c: the Constants block declares no PREMERGE_MAX_FIX_WAVE value"; FAIL=$((FAIL + 1)) +elif [ "$P22C_FENCE_WAVE_N" != "1" ]; then + echo " FAIL P22c: the fences pass $P22C_FENCE_WAVE_N distinct --max-per-wave values:" + sed 's/^/ /' <<<"$P22C_FENCE_WAVES" + FAIL=$((FAIL + 1)) +elif [ "$P22C_FENCE_WAVE" != "$P22C_DOC_WAVE" ]; then + echo " FAIL P22c: Constants says PREMERGE_MAX_FIX_WAVE=$P22C_DOC_WAVE, the fence runs --max-per-wave $P22C_FENCE_WAVE" + FAIL=$((FAIL + 1)) +elif [ "$P22C_DOC_WAVE" != "$P22C_DOC_CAP" ]; then + echo " FAIL P22c: the SKILL says the cap equals the wave size, but declares $P22C_DOC_CAP and $P22C_DOC_WAVE" + FAIL=$((FAIL + 1)) +else + echo " PASS P22c: the fence runs the declared wave size and the cap equals it ($P22C_DOC_WAVE)" + PASS=$((PASS + 1)) +fi + assert_fixed "$LIB" 'blocked-on-%02d.json' "P22c: the library reads ONE predecessor by name, not a range" assert_fixed "$LIB" '_beneath_repo_root' "P22c: containment is a filesystem check the library actually has" assert_fixed "$SKILL" 'blocked-on-.json' "P22c: the SKILL names the artifact the controller writes" diff --git a/tests/review-pr-consolidate.test.sh b/tests/review-pr-consolidate.test.sh index d5387830..9064ecab 100644 --- a/tests/review-pr-consolidate.test.sh +++ b/tests/review-pr-consolidate.test.sh @@ -914,10 +914,26 @@ rcx15_unpinned() { # rcx15_unpinned FILE OPENER -> offending lines, empty when # # Full-line comments are stripped first — the guard carries its own rationale # and that paragraph says the words "sort" and "comm" in prose. + # + # THE TRAILING CLASS IS EVERY COMMAND TERMINATOR, NOT JUST WHITESPACE. This + # tail is the TWIN of the one in tests/goal.test.sh (`g56_unpinned`), and it + # is widened here for the same measured reason: a `[[:space:]]`-only tail can + # only see an invocation that carries an argument, and `sort` needs none — it + # reads stdin. So `… | sort)` closing a command substitution, `… | sort` at + # end of line, `sort|uniq`, `sort;`, `sort&` and `sort>f` were every one of + # them a LIVE unpinned sort that the narrow grep scored as ZERO hits, and + # RCX15.live printed PASS over the lot. The two mutants below cannot catch + # that: both spell every occurrence with a following flag (`sort -u`, + # `comm -13`), which is precisely the whitespace case the old tail already + # covered. RCX15.mutation-bare is the row that pins the widened tail. + # + # `=`, `"`, `'` and `}` stay OUT of the class deliberately — those are how an + # ASSIGNMENT or a variable READ ends, so admitting them would red `sort=/bin/x`, + # `"$sort"` and `${sort}`, none of which invokes anything. rcx15_body "$1" "$2" \ | grep -vE '^[[:space:]]*#' \ | sed -E 's/LC_ALL=C[[:space:]]+(sort|comm)/PINNED_\1/g' \ - | grep -nE '(^|[^=[:alnum:]_./-])(sort|comm)[[:space:]]' || : + | grep -nE '(^|[^=[:alnum:]_./-])(sort|comm)([[:space:])|;&<>`]|$)' || : } RCX15_OPENER='review_consolidate_continue() {' RCX15_LINES="$(rcx15_body "$CONSOLIDATE_LIB" "$RCX15_OPENER" | grep -c . || :)" @@ -964,6 +980,39 @@ if [ -n "$RCX15_HALF_HITS" ]; then else fail "RCX15.mutation-half: a half-pinned line reads as clean — the predicate is line-level, so RCX15.live is blind to the next edit to that line" fi +# THE BARE-TERMINATOR MUTANT — the hole the two rows above CANNOT see. Both of +# them spell every occurrence with a following flag (`sort -u`, `comm -13`), so +# a `[[:space:]]`-only tail was enough to red them and the rest of the offender +# class went untested. `sort` takes no operand, so these three spellings are the +# ones a rewrite of the shipped `allowed=` / `staged=` / `extra=` lines actually +# produces: the word ends at `)`, at end-of-line, or at `|`. Every one of them +# is a live unpinned sort. Against the pre-widening `[[:space:]]` tail this body +# scores ZERO hits — which is what makes the widening a fix and not a cosmetic. +# +# No `comm` row here, and that is not an omission: `comm` requires TWO operands, +# so a terminator always follows an operand, never the command name itself. The +# tail is shared by the `(sort|comm)` alternation and the two mutants above +# already prove `comm` reaches this grep. +RCX15_BARE="$TMP/rcx15-mutant-bare.sh" +cat >"$RCX15_BARE" <<'EOF_RCX15B' +review_consolidate_continue() { + local allowed staged extra + allowed="$(printf '%s\n' "$allowed_raw" | sort)" + staged="$(printf '%s\n' "$staged_raw" | sort|uniq)" + extra="$(LC_ALL=C comm -13 <(printf '%s\n' "$allowed") <(printf '%s\n' "$staged"))" + printf '%s\n' "$extra" | sort +} +EOF_RCX15B +RCX15_BARE_HITS="$(rcx15_unpinned "$RCX15_BARE" "$RCX15_OPENER")" +RCX15_BARE_COUNT="$(printf '%s\n' "$RCX15_BARE_HITS" | grep -c . || :)" +# ALL THREE, never merely one: a tail widened for `)` alone would satisfy a +# non-empty check while leaving the end-of-line and `|` bypasses wide open. +if [ "${RCX15_BARE_COUNT:-0}" -eq 3 ]; then + pass "RCX15.mutation-bare: all 3 bare-terminator sorts red — ')', end-of-line and '|' are in the offender class" +else + fail "RCX15.mutation-bare: expected 3 bare-terminator hits, got ${RCX15_BARE_COUNT:-0} — the offender tail is still whitespace-only" \ + "$RCX15_BARE_HITS" +fi echo echo "== Summary ==" diff --git a/tests/testers-agent-contract.test.sh b/tests/testers-agent-contract.test.sh index d05fb654..f0acba1e 100755 --- a/tests/testers-agent-contract.test.sh +++ b/tests/testers-agent-contract.test.sh @@ -41,6 +41,18 @@ EXPECTED_AGENTS=( pass() { printf 'PASS %s\n' "$1"; } fail() { printf 'FAIL %s\n' "$1" >&2; exit 1; } +# A HARNESS error -- a predicate that CRASHED instead of judging -- must reach an +# operator, and `fail` cannot carry it. Every rejection-expecting call site below +# runs its predicate under `2>/dev/null` to swallow the EXPECTED rejection +# message, and that redirection used to eat the harness diagnostic with it: +# measured by making the predicate exit an unexpected status for one row, the run +# stopped with a failing status and printed NOTHING -- not the harness text, not +# even the row name. FD 3 is a duplicate of this script's stderr taken before any +# redirection exists; `2>/dev/null` rebinds FD 2 for one command only, so it +# cannot reach FD 3. Pass/fail outcomes are unchanged -- only the explanation of +# why the suite died is recovered. +exec 3>&2 +fail_harness() { printf 'FAIL %s\n' "$1" >&3; exit 1; } # C1: invariants.yaml exists and has 10 invariants [ -f "$INVARIANTS" ] || fail "C1: invariants.yaml missing at $INVARIANTS" @@ -215,7 +227,7 @@ $C5_VERDICT_PY" || c5_rc=$? case "$c5_rc" in 0) return 0 ;; 9) return 1 ;; - *) fail "C5: harness error -- the predicate exited $c5_rc for '$2'. A predicate that CRASHED must never read as 'rejected'; that would make every C5b reject row vacuous" ;; + *) fail_harness "C5: harness error -- the predicate exited $c5_rc for '$2'. A predicate that CRASHED must never read as 'rejected'; that would make every C5b reject row vacuous" ;; esac } @@ -351,7 +363,7 @@ $C5_SPELLING_PY" || c5_rc=$? case "$c5_rc" in 0) return 0 ;; 9) return 1 ;; - *) fail "C5c: harness error -- the convention predicate exited $c5_rc for '$2'" ;; + *) fail_harness "C5c: harness error -- the convention predicate exited $c5_rc for '$2'" ;; esac } @@ -382,6 +394,15 @@ if c5_spelling "$(c5_mutant conv-block-single-star 'tools: - Write(.uberdev/research/*)')" "mutant/conv-block-single-star" 2>/dev/null; then fail "C5c: the convention predicate ACCEPTED an off-convention scope written as a block sequence -- block-form scopes are recognised but not compared" fi +# The "nothing to inspect" arm is otherwise UNREACHABLE from every call site: the +# live-card loop above runs only after C5 has already exited on any card lacking +# a declaration, and every other mutant here carries one. This row is what keeps +# that arm executed, so it cannot rot into dead code that silently stops +# rejecting. It is a reject row because there is genuinely nothing to compare +# against the convention -- absence is not compliance. +if c5_spelling "$(c5_mutant conv-no-tools-key '# this card declares no tool key at all')" "mutant/conv-no-tools-key" 2>/dev/null; then + fail "C5c: the convention predicate ACCEPTED a card with no 'tools:' declaration -- there is nothing to inspect, so it must not report on-convention" +fi pass "C5c: all 8 scoped write grants carry the run-dir convention (documentation of intent, not a runtime ceiling)" # C6: each persona references at least one invariant ID from invariants.yaml @@ -506,31 +527,64 @@ pass "C10: rl-curl shim, persona allowlists and per-call injection are consisten # The disk YAML stays the evidence channel (C7 keeps verdict/findings/confidence); # this asserts the ADDED dual-channel stanza names the exact schema.* field set # workflow.js sends (S.persona / S.monitorPrimary / S.monitorDA). +# +# SCOPED TO THE `## Output` SECTION, never to the whole file. `StructuredOutput` +# is also a TOOL NAME, and all eight cards grant it in the `tools:` frontmatter +# array -- so a whole-file grep for it is satisfied by the GRANT and says nothing +# about the body. Measured on this tree: every card carries exactly one +# frontmatter occurrence and one body occurrence, and stripping the stanza from +# the body of all eight (frontmatter untouched) left this row printing PASS with +# the suite green. That is the "whole-file grep satisfied by text the file +# carries for another reason" class, and the extractor below is what makes the +# row able to red again. Every needle is checked against the extracted section +# for the same reason -- an assertion about what the `## Output` section says +# must read only the `## Output` section. +c11_output_section() { # $1=card path -> the `## Output...` heading line through + # the line before the next `## ` heading, or EOF. `###` subsections (the + # stanza lives under one) are NOT boundaries: `^## ` cannot match `###`. + awk ' + /^## / { if (inside) exit; if ($0 ~ /^## Output/) inside = 1 } + inside { print } + ' "$1" +} +# Herestrings, never `printf ... | grep -q`: an early-exiting reader on the right +# of a pipe poisons the pipeline rc under pipefail (tests/epipe-guard.test.sh). +c11_names() { # $1=section text $2=label $3=needle $4=which schema + grep -qF -- "$3" <<<"$1" \ + || fail "C11 $2: ## Output section does not name '$3' ($4)" +} for a in "${PERSONA_AGENTS[@]}"; do F="$AGENT_DIR/$a.md" - grep -q 'StructuredOutput' "$F" || fail "C11 $a: ## Output lacks the StructuredOutput dual-channel stanza (DR-4 prompt tension — the schema dispatch is undocumented)" + C11_OUT="$(c11_output_section "$F")" + [ -n "$C11_OUT" ] || fail "C11 $a: no '## Output' section -- the dual-channel stanza has nowhere to live" + grep -qF 'StructuredOutput' <<<"$C11_OUT" \ + || fail "C11 $a: ## Output lacks the StructuredOutput dual-channel stanza (DR-4 prompt tension — the schema dispatch is undocumented)" for field in scratchPath findingCount; do - grep -qF "$field" "$F" || fail "C11 $a: StructuredOutput stanza missing the '$field' schema field (S.persona)" + c11_names "$C11_OUT" "$a" "$field" "S.persona" done # The persona return field is `persona`; assert the stanza names it as a return field. - grep -qE '`persona`' "$F" || fail "C11 $a: StructuredOutput stanza missing the 'persona' return field (S.persona)" + c11_names "$C11_OUT" "$a" '`persona`' "the 'persona' return field, S.persona" done # monitor-primary: scratchPath + followUps (camelCase return) + verifiedAdded, # AND the explicit reconciliation with the snake_case disk key. MP="$AGENT_DIR/testers-monitor-primary.md" -grep -q 'StructuredOutput' "$MP" || fail "C11 monitor-primary: missing the StructuredOutput dual-channel stanza" +MP_OUT="$(c11_output_section "$MP")" +[ -n "$MP_OUT" ] || fail "C11 monitor-primary: no '## Output' section -- the dual-channel stanza has nowhere to live" +grep -qF 'StructuredOutput' <<<"$MP_OUT" || fail "C11 monitor-primary: missing the StructuredOutput dual-channel stanza" for field in scratchPath followUps verifiedAdded; do - grep -qF "$field" "$MP" || fail "C11 monitor-primary: StructuredOutput stanza missing the '$field' schema field (S.monitorPrimary)" + c11_names "$MP_OUT" "monitor-primary" "$field" "S.monitorPrimary" done # The camelCase return vs snake_case disk-key reconciliation must be explicit # (both names present so the two channels are documented in lockstep). -grep -q 'follow_ups_for_next_wave' "$MP" \ +grep -qF 'follow_ups_for_next_wave' <<<"$MP_OUT" \ || fail "C11 monitor-primary: stanza does not reconcile the snake_case follow_ups_for_next_wave disk key with the camelCase followUps return" # monitor-devils-advocate: scratchPath + rejected. MDA="$AGENT_DIR/testers-monitor-devils-advocate.md" -grep -q 'StructuredOutput' "$MDA" || fail "C11 monitor-devils-advocate: missing the StructuredOutput dual-channel stanza" +MDA_OUT="$(c11_output_section "$MDA")" +[ -n "$MDA_OUT" ] || fail "C11 monitor-devils-advocate: no '## Output' section -- the dual-channel stanza has nowhere to live" +grep -qF 'StructuredOutput' <<<"$MDA_OUT" || fail "C11 monitor-devils-advocate: missing the StructuredOutput dual-channel stanza" for field in scratchPath rejected; do - grep -qF "$field" "$MDA" || fail "C11 monitor-devils-advocate: StructuredOutput stanza missing the '$field' schema field (S.monitorDA)" + c11_names "$MDA_OUT" "monitor-devils-advocate" "$field" "S.monitorDA" done pass "C11: all 8 agents document the StructuredOutput dual-channel return matching the workflow schema (DR-4)"