Repository navigation
Behaviour-changing findings from the #768 simplify pass #782
Description
The three-lens simplify pass over #768 returned 41 findings. The behaviour-preserving ones landed in #781. These are the ones that change what the code does, so they were deliberately left out of a polish pass — each needs a reviewed decision.
1. skills/testers-pipeline/workflow.js:268 — unquoted rate-state directory
The polite-rate directive every persona prompt carries interpolates the rate-state directory bare, while the plugin-root path immediately before it on the same line — and every other path the file emits — is quoted. The value derives from the working directory, so it inherits any space in the repo path. This repo's own path has one.
Executed against the real shim with a stubbed curl: as written, the shim exits 2 with a missing-directory error and performs no request at all; quoted, the same call succeeds and creates the host bucket. The split fragment lands where the shim's option loop stops parsing, so the path tail is consumed as the URL.
The second copy of this directive, in the pipeline skill, quotes it correctly — two copies of one contract disagreeing, and the unquoted one is the copy that reaches the agents.
No-op on a space-free path; on a spaced path it moves persona HTTP calls from failing closed to working.
2. lib/goal-state.sh:3876 — the producer-status half of the set-comparison contract
review-consolidate.sh holds the full contract: capture each producer, check its status, then sort and comm under a pinned collation — on the stated ground that a producer which failed is indistinguishable from one that had nothing to say. #768 gave goal-state.sh only the collation half.
Both producers there still end in swallow-and-continue, so a failed lookup yields an empty path set, an empty intersection, and a verdict of no collision — and the rebase the function exists to trigger never fires.
Closing it makes the function report or audit a state it currently returns silently. Note the function's contract explicitly accepts best-effort degradation (it must never halt the goal), so this is a genuine design question, not an obvious bug — #781 reconciled the comment to say so honestly.
3. lib/review-consolidate.sh:702 — staged-path encoding mismatch
The guard's headline says it compares paths as bytes, and a comment argues an absent record can only shrink the allowed set. But the allowed set is built from two sources in two different encodings: the record is written by a command that escapes non-ASCII, while the conflict list arrives as raw bytes.
Verified on real merge conflicts in a scratch repo: the two commands spell the same non-ASCII path differently, and with the record deleted the guard refuses a correct resolution for a non-ASCII path while an ASCII path passes. So the record is load-bearing — it is the only source of the escaped spelling — and the guard currently passes by an accident of overlap rather than the byte equality it claims.
Changes behaviour for non-ASCII paths, from refusing a correct resolution to accepting it. Related: #771.
4. lib/review-consolidate.sh:175 — the masked producer one line below the rule that describes it
jq … 2>/dev/null || printf '[]' maps a crashed producer to a definite answer with rc 0. The comment block immediately above states the rule precisely and the three lines that follow apply it; this line does not. Two further sites in the file share the shape.
Changes behaviour only on crash paths — success paths identical, and a crashed producer moves from a silently wrong value to an explicit refusal. Already tracked in #771; recorded here for completeness.
5. .github/workflows/test.yml:195 — planner and router disagree on what a fixture path is
The extractor that builds the plan matches a fixture path with a character class that cannot span a directory separator; the harness arm that routes uses a glob whose wildcard can. Executed on an invocation naming a fixture one directory deeper: the extractor plans nothing and the harness routes it, so that fixture is skipped in every shard and no row notices.
A second face: a leading-dot invocation runs but the shard still reds, and the failure advises wiring it or removing the path — advice that is inverted, because it is already wired.
Latent today (no nested or dot-prefixed fixture exists). Tightening the producer or loosening the consumer starts routing an argv shape that currently falls open, so it needs a deliberate decision.
6. Eight testers-* cards: read-only prose in three wordings
Six persona cards had their read-only rule rewritten to say the declaration is a ceiling over tool names only; the two monitor cards kept the pre-correction sentence, which reads as though the write scope is enforced. The six themselves carry three different wordings.
These are agent prompts, so consolidating changes what the agents are told. The safe shape is one shared reference the cards link, with the monitors brought onto the corrected wording.
Related and larger: commands/testers.md:22 names Write as the only unconfined grant, but every persona also carries Bash(curl*) / Bash(node*). Under the same names-only rule that is unrestricted Bash — a bigger hole than Write. Tracked separately in #769.
Activity