fix(ally): make the idempotency query actually match, and the test actually run

[kkroo]

Thinking Path

Ally reviewed #1607 and found a Critical in it: the replacement idempotency query I introduced there matches nothing, so the skip it implements can never fire. I verified that empirically against the exact binary rather than accepting the finding, confirmed it, and fixed it along with the two Important items and both Suggestions from the same review. #1607 was manually enqueued before I could push this, and a queued branch cannot be updated, so this lands as the immediate follow-up.

What Changed

Critical — the query matched nothing. gh api --jq is gojq (Go RE2), not jq's Oniguruma, and the two disagree about flags: jq's "m" maps to DOTALL in gojq rather than to multiline anchoring. So test("^Reviewed head: …$"; "m") never matched a review body with a heading above the attestation, ALREADY was permanently 0, and every wake would re-review the same head — reproducing the exact outage #1607 documents. Confirmed against the prescribed binary:

$ gh api … --jq '("## Ally — Consolidated PR Review\nReviewed head: <sha>\nmore\n")
    | {with_m_flag: test("^Reviewed head: <sha>\\s*$"; "m"),
       inline_pm:   test("(?m)^Reviewed head: <sha>\\s*$")}'
{"inline_pm":true,"with_m_flag":false}

Fixed with the flag-free (^|\n) form rather than (?m). (?m) would work in RE2 but means dotall in Oniguruma — engine-dependent in precisely the way that caused this bug. (^|\n) is identical in both engines.

Important 1 — a consumer with no producer. #1607 added a reader of Reviewed head: but nothing that writes it: Step 4's template emits a short SHA inside a heading. An agent following the document end-to-end would produce reviews Step 2 could never recognise, so fixing the regex alone would still leave the guard inert. Step 4 now emits the 40-hex line, and a test pins the two halves together.

Important 2 — the tests pinned the string, not the behaviour. This is why a query matching nothing passed 7/7. The tests now extract the jq program from the fence and execute it against synthetic review payloads.

Suggestions. --paginate with a trailing | length prints one integer per page, so >30 reviews yields "0\n1" and -gt 0 dies with integer expression expected; the query now emits one line per match and counts lines (--slurp is not in this gh build). The anchor was also stricter than the attestation forms in circulation, and now tolerates the quoted and emphasised variants the live bundle's own parser accepts.

Separately: this test file was never wired into CI. pr.yml enumerates every scripts/ test individually and this one was not among them, so since #1599 it has run nowhere. Added. This is the same shape as #1607's own finding — an artifact that looks live and executes nowhere.

Verification

node --test scripts/ally-agent-idempotency-contract.test.mjs → 11/11 pass.

The decisive check is that the suite now catches the defect it previously green-lit. Restoring the exact broken regex:

	before this PR	after
broken "m" regex	7/7 pass (defect invisible)	fails tests 7 and 9

The new executable tests run the document's own jq program:

• positive control first — one attesting review must count 1, so no 0 below can pass vacuously
• counts 0 for: a different head, no attestation, the allyblockcast User seat, a dismissed review, a foreign bot, a mid-line mention
• counts 1 for the quoted, bold, and bare attestation forms

Cross-engine parity checked explicitly, since that is the whole bug — the (^|\n) pattern gives identical results in gh's gojq (RE2) and local jq 1.7 (Oniguruma) across all seven cases.

End-to-end against the live PR, which is the check Ally asked for in place of trusting the suite:

ALREADY on 1607 head ab832622 = 1     # Ally's review attests this head
ALREADY on a never-reviewed SHA = 0   # negative control

Workflow edit validated with the repo's own gates: check-workflows-parse.test.mjs 11/11, policy-node-test-timeouts.test.mjs 3/3.

Risks

Low, and lower than it looks. The edited document is not wired to the runtime — that is #1607's finding — so neither the defect nor this fix changes Ally's behaviour. What changes is that the documented query now works if followed, and that the test enforcing it actually runs in CI.

The one real risk is the reverse of the bug: (^|\n) was chosen specifically to avoid engine-dependent flag semantics, and the tests execute against local jq rather than gh's embedded gojq. That substitution is only valid because the pattern is flag-free, which is verified above in both engines. If someone later reintroduces an inline flag, the local-jq test could diverge from production behaviour — the comment in the fence says so explicitly.

Unchanged from #1607 and still for a human: the underlying drift (two hand-maintained copies) is not fixed by either PR, and Ally agreed in review that marking the file superseded is probably cheaper than wiring a sync.

• Searched for similar or duplicate PRs before opening this one — this is the direct follow-up to docs(ally): reconcile the reviewer doc with the live managed bundle #1607; no other open PR touches .planning/ally-agent/AGENTS.md or this test.

Model Used

claude-opus-5[1m]

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 915bf5a

The root cause here is right and the (^|\n) choice over (?m) is the correct one. Two findings, both in the same family as the ones this PR fixes: one is a fail-open the removed $ anchor was holding closed, the other is a string assertion guarding the producer/consumer pair in a PR whose thesis is that string assertions hid a dead query.

Critical Issues (0)

Important Issues (2)

• [gstack/review] .planning/ally-agent/AGENTS.md:125 — dropping the trailing anchor turns an empty or unset HEAD_SHA from fail-closed into fail-open: the guard silently skips review of an unreviewed head.

  With HEAD_SHA="" the pattern degenerates to (^|\n)[ \t>]*[_*]*Reviewed head:[ \t_*]*, which matches *any* Ally review carrying *any* attestation. Run against the prescribed program (jq 1.7), payload = one review attesting aaa…aand one attestingbbb…b`:

  $ HEAD_SHA=aaa…a jq -r "$PROG"   # => 1          (correct)
  $ HEAD_SHA=""    jq -r "$PROG"   # => 1\n2       (both, incl. the OTHER head)
  $ env -u HEAD_SHA jq -r "$PROG"  # => 1\n2

  So ALREADY ≥ 1, Step 2 exit 0s having reviewed nothing, and posts Re-review requested but PR has not changed since last review at ` ` — an empty SHA in the comment is the only trace. HEAD_SHA is set at AGENTS.md:81 by HEAD_SHA=$(git rev-parse HEAD), and the block carries no set -euo pipefail and no non-empty guard, so any Step 1 clone/gh pr checkout failure lands exactly here. The "m"-flag pattern being replaced was $-anchored and did not have this property — it matched nothing on an empty SHA, i.e. it re-reviewed. This is a strict regression on that axis, and the outcome (an unreviewed head skipped) is the outage class the document exists to prevent, not merely the one it currently reproduces.

  • Validate before the query rather than restoring $: case "$HEAD_SHA" in [0-9a-f][0-9a-f]*) ;; *) echo "no HEAD_SHA" >&2; exit 1;; esac, or inside the program select(env.HEAD_SHA | test("^[0-9a-f]{40}$")) so a malformed value fails closed. Then pin it: runQuery([attesting], "") must be 0.

• [pr-review-toolkit: tests] scripts/ally-agent-idempotency-contract.test.mjs:251 — the producer/consumer test is a string assertion, which is the exact weakness this PR was opened to remove.

  assert.match(step4, /Reviewed head: /) checks that the literal appears in Step 4; it does not check that what Step 4 emits is something Step 2 can match. Those come apart on plausible edits — verified against the shipped Step 2 pattern:

  Step 4 template line	/Reviewed head: /	Step 2 regex
  Reviewed head: <sha>	pass	true
  - Reviewed head: <sha>	pass	false
  * Reviewed head: <sha>	pass	false

  The prefix class [ \t>]*[_*]* admits blockquote and emphasis but not a list marker, so re-formatting the template as a bullet — a routine markdown edit — reinstates "a consumer with no producer" (this PR's own Important 1) with the suite green at 11/11. Note the coverage is also asymmetric: > _Reviewed head:_ \`` is accepted by Step 2 and rejected by this assertion.

  • Make it behavioural, reusing the machinery already added: extract Step 4's attestation line, substitute a real 40-hex SHA for the placeholder, and assert runQuery([review({ body: thatLine })]) === 1. That is the only form that actually pins the two halves together.

Suggestions (2)

• [native-codex] scripts/ally-agent-idempotency-contract.test.mjs:191 — the local-jq-stands-in-for-gojq substitution is sound only while the pattern is flag-free, and the fence says so in a comment that nothing enforces. One assertion makes the caveat executable: the extracted program contains no second argument to test(...) and no inline (? group. Without it, a future (?m) passes locally (Oniguruma: dotall, harmless here) while changing meaning in production gojq — the precise divergence this PR is fixing.
• [pr-review-toolkit: code] scripts/ally-agent-idempotency-contract.test.mjs:185 — id: Math.floor(Math.random() * 1e6) makes failures non-reproducible for no benefit; the id is never asserted on. A module-level counter is equivalent and deterministic.

Strengths

• Positive control first, stated as such: "if this cannot find a review that plainly attests the head, every 0 below is meaningless." That ordering is what makes the six negative cases load-bearing rather than vacuous, and it is the discipline whose absence produced the original 7/7-green dead query.
• The root-cause analysis is correct and the (^|\n) over (?m) choice is right for the stated reason — (?m) would work in RE2 and mean dotall in Oniguruma, so the fix would have carried the same engine-dependence as the bug.
• The before/after table (broken regex: 7/7 pass → fails tests 7 and 9) demonstrates the suite now catches the defect it previously green-lit, which is the only evidence that actually settles a "tests were green anyway" claim.
• Verified independently at this head: the newly-wired step runs and passes 11/11 in CI (policy → "Test Ally agent instruction contract", 939ms), and jq is present on arc-light with a sibling suite already asserting it — so the new execFileSync("jq", …) dependency is safe on both runner labels.

Recommended Action

1. No Critical issues — nothing blocks on correctness of the shipped regex for a well-formed SHA.
2. Address both Important items this cycle: the fail-open is one guard clause plus one test case; the producer/consumer test should be behavioural before it is relied on.
3. Suggestions opportunistically.

Unrelated to this diff: policy is currently red at this head on prcheckloop: a sub-minute interval is clamped to 60s (scripts/__tests__/, bounded PR-check polling), a suite this PR does not touch. It will need to go green independently before merge.