fix(dor): stop the reconcile sweep mis-reading every routed issue as un-routed - #1001
Merged
Conversation
…un-routed
The per-issue record was `@tsv` read back with `IFS=$'\t'`. Tab is an IFS
*whitespace* character, so bash collapses a run of tabs into one delimiter: an
empty field in the middle of the record shifts every field after it left.
`sk_label` is empty on almost every issue (only a live build holds a sidekick),
so the common record `...<TAB><TAB>state:decompose` parsed as
sk_label='state:decompose', state_label=''. Two consequences, both live:
* every correctly-routed issue took the "no state:* label" branch and was
reported "the agent likely never ran" — 30 of them on the Feature board and
#997 on the Bug board, all false;
* that branch `continue`s, so the drift check, the approval-backlog count and
the stale-waiting warnings below it never ran for any routed issue at all.
Join on US (\x1f) instead, which is not IFS whitespace, so empty fields survive.
Verified against live repo data: #997 now yields sk_label='',
state_label='state:out-of-pipeline'; under the old scheme it yields the reverse.
A read-only replay over both boards shows all 31 current exceptions go silent
and none turns into a drift flag.
Also treat "Out of pipeline" as terminal in the closed-issue check. The column
means "handled by the normal dev flow", so closing from it is the expected end,
not drift — #995 and #874 were being nagged about forever for being resolved
exactly as intended.
The harness could not have caught this: its fixture hand-encoded the record, so
the jq -> `read` contract was duplicated rather than tested. The stub now serves
the JSON `gh` would return and applies the script's OWN --jq program to it.
7 new assertions; the 4 that target these defects fail against main, the 9
existing #995 assertions still pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the second half of the previous commit. Leaving the pipeline is not the same as being filed away: a closed issue sitting in "Out of pipeline" still has to be walked over to Done, and the 🔚 line is the only reminder that it is there. Done stays the single resting state on the board. The field-shift fix is untouched — that is what silences the 31 false 🕳️ lines. Test 7 now pins the behaviour rather than removing it: a closed "Out of pipeline" issue MUST be flagged, with a second closed row as the control so both arms of the assertion are two-sided. 17 assertions; the same 4 field-shift ones fail against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #995 / #996. The field-shift below is a regression from #963.
Every hourly sweep was mailing out exceptions that were not real. 30 of the 31 items across the two health issues (#886, #938) were false.
The bug
The per-issue record was built with
@tsvand read back withIFS=$'\t'. Tab is an IFS whitespace character, so bash collapses a run of tabs into a single delimiter — an empty field in the middle of a record shifts every field after it left.sk_labelsits in that middle slot and is empty on almost every issue (only a live build holds a sidekick). So the overwhelmingly common record,…<TAB><TAB>state:out-of-pipeline, parsed as:Both lines are real output from
gh issue listagainst this repo with the same jq program, old separator vs new.Two consequences, both live:
state:*label" branch and was reported "the agent likely never ran" — 29 on the Feature board plus [CI] Lockfile-only PRs skip both Vitest jobs — every npm Dependabot PR ships untested #997 on the Bug board. [CI] Lockfile-only PRs skip both Vitest jobs — every npm Dependabot PR ships untested #997 has carriedstate:out-of-pipelinesince the moment it was triaged.continues, so everything below it — the drift check (🔀), the approval-backlog count (🚦) and the stale-waiting warnings (⏳) — has never run for a routed issue in the sweep's entire life.sk_labelwas inserted into the middle of the record by #963, which is where this starts.The fix
Join the record on US (
\u001f) instead. It is not IFS whitespace, so empty fields survive, and neither an issue number nor a GitHub label name can contain a control character.issues_tsvis renamedissues_rows, the old name having become a lie.That is the whole behavioural change: two lines.
Scope note
An earlier revision of this PR also made
Out of pipelineterminal in the closed-issue check, to silence the🔚lines on #995 and #874. That was reverted — leaving the pipeline is not the same as being filed away, and the🔚line is the only reminder that a closed issue is still parked there. Done remains the single resting state on the board. Test 7 now pins that behaviour instead of removing it.Does this undo #996?
No. #996's
build_phase()change is correct and all 9 of its assertions still pass here. Its target population — build-phase issues, which deliberately carry no state label — parses fine either way, because therestate_labelis a trailing empty field thatreadstrips cleanly. Its reading of #370 was right.What it could not see is the complementary population: issues that do have a state label. #996 fixed the control flow while the record feeding it was scrambled, so the sweep kept lying — just about a different set of issues.
Tests
The harness #996 shipped could not have caught this: its fixture hand-encoded the record, so the jq →
readcontract was duplicated in the test rather than exercised by it. The stub now serves the JSONghwould actually return and applies the script's own--jqprogram to it — the contract is under test.8 new assertions, 17 total. Against
main, the 4 that target the field shift fail and the 9 existing #995 assertions pass:The four closed-issue assertions pass against
maintoo — they exist to pin behaviour that had no coverage at all, so the🔚signal cannot be quietly removed later.Verification against live data
ghagainst this repo: [CI] Lockfile-only PRs skip both Vitest jobs — every npm Dependabot PR ships untested #997 now yieldsstate_label='state:out-of-pipeline'; the old scheme yields the reverse.ci-scriptssuites still pass.What the sweep will report after this
#886 drops to a single line —
🚦 2 issue(s) waiting in Awaiting approval— a real signal that has never fired. Four⏳stale warnings (#872, #869, #680, #373, all at 13d) will appear as they cross the 14d threshold. #938 goes empty and auto-closes.🤖 Generated with Claude Code