fix: bind Refs parser to one token #252

Merged
andres merged 6 commits from build/234-bind-refs-token into main 2026-08-24 18:15:11 +00:00

Closes #234

Rebinds each Refs keyword to exactly the single reference token that immediately follows it, preventing same-line prose from creating false issue-flow edges.

Worklog

  • Add parser boundary regressions and prove the current implementation fails them
  • Rewrite refs_references to emit one bound token per keyword occurrence
  • Preserve the original left boundary between repeated occurrences
  • Run the focused issue-flow reconciler suite
  • Add the changelog fragment
  • Run chunked shellcheck and final scoped verification

CI fix round — run 1796 / job 2

  • Incorporate current origin/main development-version bump without rewriting pushed history
  • Re-run changelog-armed on the corrected tree
  • Run the complete issue-flow regression suite and verify a clean worktree
  • Push the corrected head and post the exact-head round signal

Acceptance criteria

  • crew!64 fixture returns 52 and nothing else
  • Existing parser fixture still returns 8, 12, and 175 unmodified
  • Refs #8, #9 returns 8 alone
  • Cross-repo tokens remain non-local, while the parenthesized local form returns 175
  • Refs #8. Refs #9. returns both local references
  • Unrelated prose and refs: none return nothing
  • Open-PR issue-union tests pass unchanged
  • test/issueflow-reconcile.test.sh passes whole
  • Chunked shellcheck is clean and awk remains mawk-compatible

Test evidence

  • Red baseline at b2048f6: the four new behavior checks fail for trailing prose, comma narrowing, cross-repo trailing prose, and repeated same-line declarations; the preserved forms pass.
  • Green at b105939: the original implementation made the full issue-flow reconciler suite pass.
  • Review probe at 5232027: Refs #8Refs #9 exposed a lost left boundary (8, 9); fixed at d712f06 to return 8 alone.
  • Final head d712f0636f4a27be8b052024ea90a74208d2b52c: issue-flow 574 passed, 0 failed; changelog suites 48 + 50 + 37 + 20 passed; chunked ShellCheck clean; mawk 1.3.4; diff and worktree clean.

Round log

Round at 4fb01e8b

Round passed with no written reply.

Closes #234 Rebinds each `Refs` keyword to exactly the single reference token that immediately follows it, preventing same-line prose from creating false issue-flow edges. ## Worklog - [x] Add parser boundary regressions and prove the current implementation fails them - [x] Rewrite `refs_references` to emit one bound token per keyword occurrence - [x] Preserve the original left boundary between repeated occurrences - [x] Run the focused issue-flow reconciler suite - [x] Add the changelog fragment - [x] Run chunked shellcheck and final scoped verification ### CI fix round — run 1796 / job 2 - [x] Incorporate current `origin/main` development-version bump without rewriting pushed history - [x] Re-run `changelog-armed` on the corrected tree - [x] Run the complete issue-flow regression suite and verify a clean worktree - [x] Push the corrected head and post the exact-head round signal ## Acceptance criteria - [x] crew!64 fixture returns `52` and nothing else - [x] Existing parser fixture still returns `8`, `12`, and `175` unmodified - [x] `Refs #8, #9` returns `8` alone - [x] Cross-repo tokens remain non-local, while the parenthesized local form returns `175` - [x] `Refs #8. Refs #9.` returns both local references - [x] Unrelated prose and `refs: none` return nothing - [x] Open-PR issue-union tests pass unchanged - [x] `test/issueflow-reconcile.test.sh` passes whole - [x] Chunked shellcheck is clean and awk remains mawk-compatible ## Test evidence - Red baseline at `b2048f6`: the four new behavior checks fail for trailing prose, comma narrowing, cross-repo trailing prose, and repeated same-line declarations; the preserved forms pass. - Green at `b105939`: the original implementation made the full issue-flow reconciler suite pass. - Review probe at `5232027`: `Refs #8Refs #9` exposed a lost left boundary (`8`, `9`); fixed at `d712f06` to return `8` alone. - Final head `d712f0636f4a27be8b052024ea90a74208d2b52c`: issue-flow `574 passed, 0 failed`; changelog suites `48 + 50 + 37 + 20` passed; chunked ShellCheck clean; mawk 1.3.4; diff and worktree clean. ## Round log <!-- round:4fb01e8b9fb8547831485112710686930d12c25c --> **Round at 4fb01e8b** _Round passed with no written reply._
codex-bot-andresmgsl added 1 commit 2026-08-24 16:02:37 +00:00
test: pin Refs token boundaries (#234)
Some checks failed
CI / test (pull_request) Failing after 48s
CI / release-exercise (pull_request) Successful in 24s
CI / self-guards (pull_request) Failing after 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
Refs guard / refs-not-closing (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
b2048f63bd
codex-bot-andresmgsl added 1 commit 2026-08-24 16:04:13 +00:00
fix: bind Refs declarations to one token (#234)
Some checks failed
CI / test (pull_request) Failing after 49s
CI / release-exercise (pull_request) Successful in 23s
CI / self-guards (pull_request) Failing after 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
Refs guard / refs-not-closing (pull_request) Successful in 19s
b105939d95
codex-bot-andresmgsl added 1 commit 2026-08-24 16:04:44 +00:00
docs: record bounded Refs parsing (#234)
Some checks failed
CI / test (pull_request) Failing after 49s
CI / release-exercise (pull_request) Successful in 23s
CI / self-guards (pull_request) Failing after 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
Refs guard / refs-not-closing (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
6b2b467b7c
codex-bot-andresmgsl added 1 commit 2026-08-24 16:06:23 +00:00
test: mark Refs fixture Markdown literal (#234)
Some checks failed
CI / test (pull_request) Successful in 3m42s
CI / release-exercise (pull_request) Successful in 24s
CI / self-guards (pull_request) Failing after 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
Refs guard / refs-not-closing (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
5232027361
codex-bot-andresmgsl added 1 commit 2026-08-24 16:13:30 +00:00
fix: preserve Refs keyword boundaries (#234)
Some checks failed
CI / test (pull_request) Successful in 3m44s
CI / release-exercise (pull_request) Successful in 24s
CI / self-guards (pull_request) Failing after 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
Refs guard / refs-not-closing (pull_request) Successful in 19s
d712f0636f
Author
Member

Initial build round answered whole at head d712f0636f.

  • Bound every Refs occurrence to its single immediately following token and retained the existing LOCAL/CROSS classification plus numeric sort.
  • Added regression coverage for the crew!64 prose leak, deliberate comma narrowing, cross-repo filtering, parenthesized prose, repeated same-line declarations, negative prose, and concatenated non-keywords.
  • Preserved the original left boundary while scanning repeated occurrences, so Refs #8Refs #9 yields only 8.
  • Added changelog.d/234.md.
  • Verified issue-flow 574/574, changelog suites 48/48 + 50/50 + 37/37 + 20/20, chunked ShellCheck, mawk 1.3.4, clean diff, and clean worktree.

Every acceptance criterion and worklog item is checked in the PR body; no review point is outstanding.

Initial build round answered whole at head d712f0636f4a27be8b052024ea90a74208d2b52c. - Bound every `Refs` occurrence to its single immediately following token and retained the existing LOCAL/CROSS classification plus numeric sort. - Added regression coverage for the crew!64 prose leak, deliberate comma narrowing, cross-repo filtering, parenthesized prose, repeated same-line declarations, negative prose, and concatenated non-keywords. - Preserved the original left boundary while scanning repeated occurrences, so `Refs #8Refs #9` yields only `8`. - Added `changelog.d/234.md`. - Verified issue-flow `574/574`, changelog suites `48/48 + 50/50 + 37/37 + 20/20`, chunked ShellCheck, mawk 1.3.4, clean diff, and clean worktree. Every acceptance criterion and worklog item is checked in the PR body; no review point is outstanding.
Author
Member

📣 round answered at head d712f0636f

📣 round answered at head d712f0636f4a27be8b052024ea90a74208d2b52c
codex-bot-andresmgsl changed title from WIP: fix: bind Refs parser to one token to fix: bind Refs parser to one token 2026-08-24 16:15:10 +00:00
forgejo-actions added the
state:addressing
label 2026-08-24 16:20:29 +00:00
forgejo-actions added the
scope:labels
label 2026-08-24 16:34:09 +00:00
Author
Member

🔧 addressing round on head d712f0636f

Classification: ATTRIBUTABLE.

The actual CI / self-guards (pull_request) log for run 1796, job 2 shows checkout and repository fetch both succeeded, then the repo-owned changelog-armed guard ran against this head and failed deterministically: these fragments were not consumed: changelog.d/238.md, changelog.d/234.md — re-run 'changelog-assemble 0.6.2'. The branch is based on the 0.6.2 release merge and does not yet carry current main's ca7ce6e development-version bump, so this is branch state rather than an infrastructure/setup failure. I will not rerun the unchanged head.

Round analysis:

  • Agree — blocking CI point: update the branch onto current origin/main so the change is evaluated as 0.6.3-dev, while retaining the required #234 fragment; run the failing self-guard locally and the scoped regression suite; push the corrective commit(s).
  • Reviewer points: none have been posted on this PR.
  • I will add these fix steps to the PR Worklog, keep them checked as they complete, then post the exact-head round signal once the complete fix is pushed. I will not request or re-request the panel; the engine owns that after the head settles.
🔧 addressing round on head d712f0636f4a27be8b052024ea90a74208d2b52c Classification: **ATTRIBUTABLE**. The actual `CI / self-guards (pull_request)` log for run 1796, job 2 shows checkout and repository fetch both succeeded, then the repo-owned `changelog-armed` guard ran against this head and failed deterministically: `these fragments were not consumed: changelog.d/238.md, changelog.d/234.md — re-run 'changelog-assemble 0.6.2'`. The branch is based on the 0.6.2 release merge and does not yet carry current `main`'s `ca7ce6e` development-version bump, so this is branch state rather than an infrastructure/setup failure. I will not rerun the unchanged head. Round analysis: - **Agree — blocking CI point:** update the branch onto current `origin/main` so the change is evaluated as 0.6.3-dev, while retaining the required #234 fragment; run the failing self-guard locally and the scoped regression suite; push the corrective commit(s). - **Reviewer points:** none have been posted on this PR. - I will add these fix steps to the PR Worklog, keep them checked as they complete, then post the exact-head round signal once the complete fix is pushed. I will not request or re-request the panel; the engine owns that after the head settles.
codex-bot-andresmgsl added 1 commit 2026-08-24 17:08:57 +00:00
chore: merge main development bump (#234)
All checks were successful
CI / test (pull_request) Successful in 4m6s
CI / release-exercise (pull_request) Successful in 23s
CI / self-guards (pull_request) Successful in 19s
CI / action-exercise (pull_request) Successful in 19s
CI / docs-sync-exercise (pull_request) Successful in 19s
labels / labels (pull_request) Successful in 21s
Refs guard / refs-not-closing (pull_request) Successful in 20s
4fb01e8b9f
Author
Member

Round answered whole at head 4fb01e8b9f.

  • The blocking CI / self-guards (pull_request) failure was ATTRIBUTABLE, not infrastructure: run 1796 job 2 checked out successfully and the repo-owned changelog-armed guard rejected the branch's released 0.6.2 version with surviving fragments changelog.d/238.md and changelog.d/234.md. The unchanged failing head was not rerun.
  • Merged current origin/main at ca7ce6e9194d0b427c093cfc74f03eb2c84fe25f without rewriting pushed history. That carries the required 0.6.3-dev development-version bump while preserving the #234 fragment and parser changes.
  • Reproduced the failure locally before the merge, then verified the corrected tree: issue-flow regression suite completed successfully; changelog-armed, changelog-monotonic, changelog-assembled, drill-recorded, and runner-isolated all pass; git diff --check is clean; the worktree is clean and matches the pushed remote head.
  • No reviewer points were posted in this round.

All CI-fix Worklog items are checked. I have not requested or re-requested any reviewer; the engine owns the panel request after this head settles.

Round answered whole at head 4fb01e8b9fb8547831485112710686930d12c25c. - The blocking `CI / self-guards (pull_request)` failure was **ATTRIBUTABLE**, not infrastructure: run 1796 job 2 checked out successfully and the repo-owned `changelog-armed` guard rejected the branch's released `0.6.2` version with surviving fragments `changelog.d/238.md` and `changelog.d/234.md`. The unchanged failing head was not rerun. - Merged current `origin/main` at `ca7ce6e9194d0b427c093cfc74f03eb2c84fe25f` without rewriting pushed history. That carries the required `0.6.3-dev` development-version bump while preserving the #234 fragment and parser changes. - Reproduced the failure locally before the merge, then verified the corrected tree: issue-flow regression suite completed successfully; `changelog-armed`, `changelog-monotonic`, `changelog-assembled`, `drill-recorded`, and `runner-isolated` all pass; `git diff --check` is clean; the worktree is clean and matches the pushed remote head. - No reviewer points were posted in this round. All CI-fix Worklog items are checked. I have not requested or re-requested any reviewer; the engine owns the panel request after this head settles.
Author
Member

📣 round answered at head 4fb01e8b9f

📣 round answered at head 4fb01e8b9fb8547831485112710686930d12c25c
forgejo-actions added the
blocker:unrequested
label 2026-08-24 17:38:25 +00:00
codex-bot-andresmgsl requested review from glm-bot-andresmgsl 2026-08-24 17:40:52 +00:00
codex-bot-andresmgsl requested review from claude-bot-andresmgsl 2026-08-24 17:40:52 +00:00
codex-bot-andresmgsl requested review from kimi-bot-andresmgsl 2026-08-24 17:40:52 +00:00
codex-bot-andresmgsl added the
state:bots-reviewing
label 2026-08-24 17:40:53 +00:00
forgejo-actions removed the
blocker:unrequested
state:addressing
labels 2026-08-24 17:41:46 +00:00

🔎 reviewing head 4fb01e8b9f

  • scope: whole PR vs issue #234 acceptance criteria, load-bearing constraints, code/tests
  • issue-flow suite: 574 passed, 0 failed (mawk 1.3.4); shellcheck-all clean (64 scripts); changelog suites 50/20/37/103 green; head CI status success (self-guards recovered)
  • red baseline reproduced against the pre-PR parser (crew 52+65, comma 8+9, crossprose 12, repeat 8-only) — new tests discriminate
  • verdict: APPROVE submitted at head 4fb01e8b9f
🔎 reviewing head 4fb01e8b9fb8547831485112710686930d12c25c - scope: whole PR vs issue #234 acceptance criteria, load-bearing constraints, code/tests - issue-flow suite: 574 passed, 0 failed (mawk 1.3.4); shellcheck-all clean (64 scripts); changelog suites 50/20/37/103 green; head CI status success (self-guards recovered) - red baseline reproduced against the pre-PR parser (crew 52+65, comma 8+9, crossprose 12, repeat 8-only) — new tests discriminate - verdict: APPROVE submitted at head 4fb01e8b9fb8547831485112710686930d12c25c

🔎 reviewing head 4fb01e8b9f

  • read issue #234 acceptance criteria and test plan
  • ran test/issueflow-reconcile.test.sh: 574 passed, 0 failed
  • ran shellcheck -x on changed script and test: clean
  • ran .github/scripts/{marker-check,vendored-check,self-ref-check}.sh: all green
  • ran full test/run.sh: only package-json cases fail because node/npm is not installed on this box; all other files pass
  • CI status shows CI/test SUCCESS for this head
  • approving: implementation matches the spec and the new regression tests fail on the old parser
🔎 reviewing head 4fb01e8b9fb8547831485112710686930d12c25c - read issue #234 acceptance criteria and test plan - ran test/issueflow-reconcile.test.sh: 574 passed, 0 failed - ran shellcheck -x on changed script and test: clean - ran .github/scripts/{marker-check,vendored-check,self-ref-check}.sh: all green - ran full test/run.sh: only package-json cases fail because node/npm is not installed on this box; all other files pass - CI status shows CI/test SUCCESS for this head - approving: implementation matches the spec and the new regression tests fail on the old parser
glm-bot-andresmgsl approved these changes 2026-08-24 17:50:45 +00:00
glm-bot-andresmgsl left a comment
Member

Approve — verified at head 4fb01e8b9f in a detached throwaway worktree.

Acceptance criteria (issue #234) — all run, all pass:

  • test/issueflow-reconcile.test.sh whole: 574 passed, 0 failed on this box, whose awk is mawk 1.3.4 (the runner shape the mawk criterion names); the crew!64 fixture is in-file and returns 52 alone; the pre-existing refs_body fixture still returns 8\n12\n175 and its check is untouched by the diff.
  • Deliberate narrowing pinned by name ("Refs comma-list narrowing is deliberate"): Refs #8, #98; cross-repo forms (Refs heavy-duty/rig#4, with , #12) → empty; paren case returns 175; Refs #8. Refs #9. → both; Refs #8Refs #98; fix refs parsing from #200 and Closes #40; refs: none → nothing; open_pr_issues union checks unchanged and passing.
  • Chunked shellcheck (.github/scripts/shellcheck-all.sh): clean over all 64 tracked scripts. No \x escapes introduced.

Failure cases actually fail: I re-ran the four must-change shapes through the pre-PR parser (origin/main's refs_references, reproduced verbatim) — crew line yields the false 65, comma yields 8 9, cross-repo prose yields 12, repeated declaration yields 8 only. The new tests genuinely discriminate; the builder's red-baseline claim at b2048f6 is consistent with what I measured.

Code: the rewrite binds each keyword occurrence to exactly the one token the spec's regex allows, loops over occurrences, deletes the sub(/[.(;].*/) truncation and the guard it served, keeps the issue_references → LOCAL → sort -nu tail unchanged. The retained final byte (RSTART + RLENGTH - 1) is always a digit, so it can only suppress a false fresh boundary and cannot loop forever (every match consumes ≥ keyword+token). Output now comes from tolower($0); I checked classification is case-blind where it matters (#N → LOCAL, any qualified token → CROSS and dropped), so lowercasing changes nothing observable. Changelog fragment present.

CI: head status is success for all seven checks, including the CI / self-guards (pull_request) that failed run 1796 job 2 on the prior head; changelog suites armed/monotonic/assembled/changelog pass locally (50/20/37/103).

Not verified, relied on instead: the historical run-1796 log itself (I verified the corrected outcome, not the old failure — the builder's account plus the current green guard stand in); actionlint and the full test/run.sh matrix with CEREMONY_REQUIRE_NPM/YQ=1 (no npm/yq drive here — relied on the green CI jobs for the same head).

On-panel (author codex-bot recuses; this verdict is a gating one, not advisory). Non-blocking nit, builder's discretion: none worth logging.

Approve — verified at head 4fb01e8b9fb8547831485112710686930d12c25c in a detached throwaway worktree. **Acceptance criteria (issue #234) — all run, all pass:** - `test/issueflow-reconcile.test.sh` whole: **574 passed, 0 failed** on this box, whose awk is mawk 1.3.4 (the runner shape the mawk criterion names); the crew!64 fixture is in-file and returns `52` alone; the pre-existing `refs_body` fixture still returns `8\n12\n175` and its check is untouched by the diff. - Deliberate narrowing pinned by name ("Refs comma-list narrowing is deliberate"): `Refs #8, #9` → `8`; cross-repo forms (`Refs heavy-duty/rig#4`, with `, #12`) → empty; paren case returns `175`; `Refs #8. Refs #9.` → both; `Refs #8Refs #9` → `8`; `fix refs parsing from #200` and `Closes #40; refs: none` → nothing; `open_pr_issues` union checks unchanged and passing. - Chunked shellcheck (`.github/scripts/shellcheck-all.sh`): clean over all 64 tracked scripts. No `\x` escapes introduced. **Failure cases actually fail:** I re-ran the four must-change shapes through the pre-PR parser (origin/main's `refs_references`, reproduced verbatim) — crew line yields the false `65`, comma yields `8 9`, cross-repo prose yields `12`, repeated declaration yields `8` only. The new tests genuinely discriminate; the builder's red-baseline claim at `b2048f6` is consistent with what I measured. **Code:** the rewrite binds each keyword occurrence to exactly the one token the spec's regex allows, loops over occurrences, deletes the `sub(/[.(;].*/)` truncation and the guard it served, keeps the `issue_references` → LOCAL → `sort -nu` tail unchanged. The retained final byte (`RSTART + RLENGTH - 1`) is always a digit, so it can only suppress a false fresh boundary and cannot loop forever (every match consumes ≥ keyword+token). Output now comes from `tolower($0)`; I checked classification is case-blind where it matters (`#N` → LOCAL, any qualified token → CROSS and dropped), so lowercasing changes nothing observable. Changelog fragment present. **CI:** head status is `success` for all seven checks, including the `CI / self-guards (pull_request)` that failed run 1796 job 2 on the prior head; changelog suites armed/monotonic/assembled/changelog pass locally (50/20/37/103). **Not verified, relied on instead:** the historical run-1796 log itself (I verified the corrected outcome, not the old failure — the builder's account plus the current green guard stand in); actionlint and the full `test/run.sh` matrix with `CEREMONY_REQUIRE_NPM/YQ=1` (no npm/yq drive here — relied on the green CI jobs for the same head). On-panel (author codex-bot recuses; this verdict is a gating one, not advisory). Non-blocking nit, builder's discretion: none worth logging.
kimi-bot-andresmgsl approved these changes 2026-08-24 17:54:42 +00:00
kimi-bot-andresmgsl left a comment
Member

Reviewed against issue #234's acceptance criteria and the repo's conventions.

  • refs_references now loops over every Refs occurrence and emits only the immediately following #N or owner/repo#N token, matching the sibling refs-not-closing parser.
  • The sub(/[.(;].*/, "", line) truncation is removed; the token boundary replaces it.
  • Extraction still routes through issue_references, so cross-repo refs stay CROSS and the output stays sort -nu local issues.
  • New regression tests in test/issueflow-reconcile.test.sh cover the crew!64 fixture, comma-list narrowing, cross-repo prose, parenthesized prose, multiple occurrences on one line, concatenated keyword misspellings, and non-Refs uses of the word. test/issueflow-reconcile.test.sh passes whole (574/574).
  • shellcheck -x is clean on the changed script and test.
  • .github/scripts/{marker-check,vendored-check,self-ref-check}.sh are green on this head.
  • Full test/run.sh only fails the package-json cases because node/npm is not installed on this box; every other test file passes, and the PR's CI/test status is SUCCESS at this head.
  • No workflow files are changed, so I did not run actionlint.

This head satisfies #234.

Reviewed against issue #234's acceptance criteria and the repo's conventions. - `refs_references` now loops over every `Refs` occurrence and emits only the immediately following `#N` or `owner/repo#N` token, matching the sibling `refs-not-closing` parser. - The `sub(/[.(;].*/, "", line)` truncation is removed; the token boundary replaces it. - Extraction still routes through `issue_references`, so cross-repo refs stay `CROSS` and the output stays `sort -nu` local issues. - New regression tests in `test/issueflow-reconcile.test.sh` cover the crew!64 fixture, comma-list narrowing, cross-repo prose, parenthesized prose, multiple occurrences on one line, concatenated keyword misspellings, and non-Refs uses of the word. `test/issueflow-reconcile.test.sh` passes whole (574/574). - `shellcheck -x` is clean on the changed script and test. - `.github/scripts/{marker-check,vendored-check,self-ref-check}.sh` are green on this head. - Full `test/run.sh` only fails the package-json cases because `node`/`npm` is not installed on this box; every other test file passes, and the PR's CI/test status is SUCCESS at this head. - No workflow files are changed, so I did not run `actionlint`. This head satisfies #234.

🔎 reviewing head 4fb01e8b9f

  • scope: whole PR (6 commits, base ca7ce6e) vs #234 acceptance criteria, repo constraints, code/tests
  • issue-flow suite: 574 passed, 0 failed (mawk 1.3.4); chunked shellcheck clean; marker/vendored/self-ref guards green
  • red baseline constructed: old parser swapped into this tree -> 569 passed, 5 failed, exactly the new behavior checks
  • differential fuzz over 3445 lines: every gain is a later Refs occurrence (intended); every drop is the decided narrowing; no bound reference ever lost
  • changelog-armed run as action.yml drives it: 0.6.3-dev agrees with fragment mode, rc=0; forge CI green at this head
  • criteria: all verified; verdict submitted
🔎 reviewing head 4fb01e8b9fb8547831485112710686930d12c25c - scope: whole PR (6 commits, base ca7ce6e) vs #234 acceptance criteria, repo constraints, code/tests - issue-flow suite: 574 passed, 0 failed (mawk 1.3.4); chunked shellcheck clean; marker/vendored/self-ref guards green - red baseline constructed: old parser swapped into this tree -> 569 passed, 5 failed, exactly the new behavior checks - differential fuzz over 3445 lines: every gain is a later Refs occurrence (intended); every drop is the decided narrowing; no bound reference ever lost - changelog-armed run as action.yml drives it: 0.6.3-dev agrees with fragment mode, rc=0; forge CI green at this head - criteria: all verified; verdict submitted
claude-bot-andresmgsl approved these changes 2026-08-24 18:07:05 +00:00
claude-bot-andresmgsl left a comment
Member

Approve at head 4fb01e8b9fb8547831485112710686930d12c25c. Reviewed the whole PR (all six commits, base ca7ce6e) against #234's acceptance criteria, this repo's constraints, then the code.

What I ran, in a detached worktree at this head

  • test/issueflow-reconcile.test.sh574 passed, 0 failed (mawk 1.3.4, bash 5.2.37).
  • Red baseline, constructed rather than trusted. I copied the tree, swapped refs_references back to ca7ce6e's body and left everything else at this head. The suite then reported 569 passed, 5 failed, and the five that fail are exactly the new behavior checks: ignores issue prose after the bound token, comma-list narrowing is deliberate, prose after a cross-repo Refs token stays non-local, every Refs occurrence on one line contributes its bound token, a concatenated Refs spelling is not a second keyword occurrence. The failure cases actually fail.
  • Direct old-vs-new probe of every row of the issue's test plan. Old → new: crew!64 line 52,6552; Refs #8, #9 8,98; Refs heavy-duty/rig#4, #12 12→∅; Refs #8. Refs #9. 88,9. Preserved forms all unchanged: the :101 fixture 8,12,175; Refs #175 (split from #150)175; Refs heavy-duty/rig#4→∅; Also refs: #8 and heavy-duty/rig#4.8; fix refs parsing from #200→∅; Closes #40; refs: none→∅.
  • Differential fuzz, 3445 generated lines (keyword casing/spelling, separator, token shape, leading Markdown, trailing prose) run through both parsers. Every reference the new parser gains is a later Refs occurrence on the same line — spec item 1, intended. Every reference it drops (585 lines) is either a cross-repo token in the keyword's bound position or prose after the bound token — the two narrowings #234 decides on purpose. A separate assertion over the same corpus: whenever the first keyword is immediately followed by a local #N, that N is in the output. Zero losses of a bound reference.
  • .github/scripts/shellcheck-all.sh — clean, rc=0 (chunked helper, as the criterion requires). marker-check.sh, vendored-check.sh, self-ref-check.sh — all green.
  • changelog-armed driven the way action.yml drives it (CHANGELOG=CHANGELOG.md VERSION_SOURCE=file FRAGMENTS_DIR=changelog.d): version '0.6.3-dev' agrees with fragment mode, rc=0. That is the CI-fix round proven on the tree, not just on the badge. Changelog suites: armed 50, assembled 37, assemble 48, monotonic 20, changelog 103 — all 0 failed.
  • Forge CI at this head: run 1819 jobs 0–4, 1821, 1822 — all success.

Acceptance criteria

Every box in #234 checks out: crew!64 fixture → 52 alone; :101 fixture byte-identical in the diff and still 8\n12\n175; comma narrowing pinned by a test whose name says it is deliberate; both cross-repo forms and the paren form pinned; Refs #8. Refs #9. returns both; the two negatives return nothing; open_pr_issues union block at :104-110 unchanged (context-only in the diff); suite passes whole; chunked shellcheck clean; awk is mawk-compatible with no \x escapes. Tasks likewise: the sub(/[.(;].*/, "", line) truncation and the ^(#|…)#[0-9]+ guard are both gone, and the issue_referencesLOCALsort -nu tail is untouched. changelog.d/234.md is present under ### Fixed and cites (#234). Closes #234 is the right keyword — #234 marks no criterion post-merge.

Code

The loop terminates: the match ends in [0-9]+, so each pass advances rest by at least RLENGTH-1 ≥ 6 bytes. The retained final byte is always a digit, which is why ^ cannot manufacture a keyword boundary out of a concatenated suffix — d712f06's fix, and the comment names why with its issue number, per CONTRIBUTING. The greedy sub(/^.*refs[[:space:]:]+/, "", token) is safe rather than lucky: the reference-token charset [[:alnum:]_.-/] contains neither space nor colon, so a second refs[[:space:]:]+ cannot occur inside a matched region — the fuzz found no misbinding.

Nits — non-blocking, builder's discretion

  1. rest = tolower($0) means the printed token is the lowercased one, where the old code matched on a lowered copy but sliced the original line. Today this is invisible: refs_references keeps only LOCAL rows and those are digits. It does mean a cross-repo Owner/Repo#4 reaches issue_references case-folded, so if that side ever becomes load-bearing the case is already gone.
  2. The two parsers for this relation still disagree on the bracketed form — actions/refs-not-closing accepts [[]?#[0-9]+, the reconciler does not. Unchanged by this PR, and #234 puts refs-not-closing out of scope explicitly, so this belongs in a discussion, not here.

What I could not verify, and what I relied on instead

  • test/run.sh is not whole-green in this box: 29 test files passed, 2 failedtest/labels-bootstrap.test.sh (7 passed, 7 failed) and test/labels-dispatch.test.sh (7 passed, 14 failed), every failure a yq: command not found. I checked out base ca7ce6e in a second detached worktree and ran both files there: identical failure counts. Pre-existing environment gap on this box, not this PR. test/labels-scope.test.sh likewise emits SKIP: yq not found. For the runner-side proof of those files I rely on CI run 1819 job 0.
  • I did not read crew!64 over the network. The issue directs the fixture-in-the-test-file form, and that is what I exercised; the fixture line matches the body quoted in #234.
  • I did not run the reconciler against a live board. Board-level effects — the claimedpost-merge transition and the OPEN_PR_ISSUES union — are covered here only by the suite's unit and gather tests, which pass.
**Approve** at head `4fb01e8b9fb8547831485112710686930d12c25c`. Reviewed the whole PR (all six commits, base `ca7ce6e`) against #234's acceptance criteria, this repo's constraints, then the code. ## What I ran, in a detached worktree at this head - `test/issueflow-reconcile.test.sh` — **574 passed, 0 failed** (mawk 1.3.4, bash 5.2.37). - **Red baseline, constructed rather than trusted.** I copied the tree, swapped `refs_references` back to `ca7ce6e`'s body and left everything else at this head. The suite then reported **569 passed, 5 failed**, and the five that fail are exactly the new behavior checks: `ignores issue prose after the bound token`, `comma-list narrowing is deliberate`, `prose after a cross-repo Refs token stays non-local`, `every Refs occurrence on one line contributes its bound token`, `a concatenated Refs spelling is not a second keyword occurrence`. The failure cases actually fail. - Direct old-vs-new probe of every row of the issue's test plan. Old → new: crew!64 line `52,65`→`52`; `Refs #8, #9` `8,9`→`8`; `Refs heavy-duty/rig#4, #12` `12`→∅; `Refs #8. Refs #9.` `8`→`8,9`. Preserved forms all unchanged: the `:101` fixture `8,12,175`; `Refs #175 (split from #150)`→`175`; `Refs heavy-duty/rig#4`→∅; `Also refs: #8 and heavy-duty/rig#4.`→`8`; `fix refs parsing from #200`→∅; `Closes #40; refs: none`→∅. - **Differential fuzz, 3445 generated lines** (keyword casing/spelling, separator, token shape, leading Markdown, trailing prose) run through both parsers. Every reference the new parser gains is a later `Refs` occurrence on the same line — spec item 1, intended. Every reference it drops (585 lines) is either a cross-repo token in the keyword's bound position or prose after the bound token — the two narrowings #234 decides on purpose. A separate assertion over the same corpus: whenever the first keyword is immediately followed by a local `#N`, that `N` is in the output. **Zero losses of a bound reference.** - `.github/scripts/shellcheck-all.sh` — clean, rc=0 (chunked helper, as the criterion requires). `marker-check.sh`, `vendored-check.sh`, `self-ref-check.sh` — all green. - `changelog-armed` driven the way `action.yml` drives it (`CHANGELOG=CHANGELOG.md VERSION_SOURCE=file FRAGMENTS_DIR=changelog.d`): `version '0.6.3-dev' agrees with fragment mode`, rc=0. That is the CI-fix round proven on the tree, not just on the badge. Changelog suites: armed 50, assembled 37, assemble 48, monotonic 20, changelog 103 — all 0 failed. - Forge CI at this head: run 1819 jobs 0–4, 1821, 1822 — **all success**. ## Acceptance criteria Every box in #234 checks out: crew!64 fixture → `52` alone; `:101` fixture byte-identical in the diff and still `8\n12\n175`; comma narrowing pinned by a test whose name says it is deliberate; both cross-repo forms and the paren form pinned; `Refs #8. Refs #9.` returns both; the two negatives return nothing; `open_pr_issues` union block at `:104-110` unchanged (context-only in the diff); suite passes whole; chunked shellcheck clean; awk is mawk-compatible with no `\x` escapes. Tasks likewise: the `sub(/[.(;].*/, "", line)` truncation and the `^(#|…)#[0-9]+` guard are both gone, and the `issue_references` → `LOCAL` → `sort -nu` tail is untouched. `changelog.d/234.md` is present under `### Fixed` and cites `(#234)`. `Closes #234` is the right keyword — #234 marks no criterion post-merge. ## Code The loop terminates: the match ends in `[0-9]+`, so each pass advances `rest` by at least `RLENGTH-1 ≥ 6` bytes. The retained final byte is always a digit, which is why `^` cannot manufacture a keyword boundary out of a concatenated suffix — `d712f06`'s fix, and the comment names why with its issue number, per CONTRIBUTING. The greedy `sub(/^.*refs[[:space:]:]+/, "", token)` is safe rather than lucky: the reference-token charset `[[:alnum:]_.-/]` contains neither space nor colon, so a second `refs[[:space:]:]+` cannot occur inside a matched region — the fuzz found no misbinding. ## Nits — non-blocking, builder's discretion 1. `rest = tolower($0)` means the printed token is the lowercased one, where the old code matched on a lowered copy but sliced the original line. Today this is invisible: `refs_references` keeps only `LOCAL` rows and those are digits. It does mean a cross-repo `Owner/Repo#4` reaches `issue_references` case-folded, so if that side ever becomes load-bearing the case is already gone. 2. The two parsers for this relation still disagree on the bracketed form — `actions/refs-not-closing` accepts `[[]?#[0-9]+`, the reconciler does not. Unchanged by this PR, and #234 puts `refs-not-closing` out of scope explicitly, so this belongs in a discussion, not here. ## What I could not verify, and what I relied on instead - **`test/run.sh` is not whole-green in this box: 29 test files passed, 2 failed** — `test/labels-bootstrap.test.sh` (7 passed, 7 failed) and `test/labels-dispatch.test.sh` (7 passed, 14 failed), every failure a `yq: command not found`. I checked out base `ca7ce6e` in a second detached worktree and ran both files there: **identical failure counts**. Pre-existing environment gap on this box, not this PR. `test/labels-scope.test.sh` likewise emits `SKIP: yq not found`. For the runner-side proof of those files I rely on CI run 1819 job 0. - I did not read crew!64 over the network. The issue directs the fixture-in-the-test-file form, and that is what I exercised; the fixture line matches the body quoted in #234. - I did not run the reconciler against a live board. Board-level effects — the `claimed` → `post-merge` transition and the `OPEN_PR_ISSUES` union — are covered here only by the suite's unit and gather tests, which pass.
codex-bot-andresmgsl requested review from andres 2026-08-24 18:14:54 +00:00
codex-bot-andresmgsl added the
state:needs-human
label 2026-08-24 18:14:54 +00:00
andres merged commit 46458ba8cd into main 2026-08-24 18:15:11 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: heavy-duty/ceremony#252
No description provided.