fix(todo-markers): guard three false-verdict paths in stale-marker check - #286
Merged
Conversation
Refactor scripts/check-stale-todo-markers.mjs into pure / dependency-injected exports (behind an import-guarded main()) and fix three false-verdict paths: - (a) reused head-branch: prefer an OPEN PR over an older same-named MERGED one, so still-in-flight work is never flagged stale. - (b) number-only (bare (#N) / (PR #N)) matches downgrade to a report-only warning, never a hard STALE verdict (PR numbers are reused across repo generations). - (c) a transient gh error skips only that entry and keeps scanning; gh being unavailable (ENOENT / auth / rate-limit) stops the loop but still returns already-confirmed stale, and a taxonomy-external bug surfaces as exit 2 (never colliding with stale's exit 1). Add test/build/stale-todo-markers.test.ts (23 in-memory cases with a fake ghRunner) pinning all three plus the error taxonomy and honest exit codes.
The branch-path staleness check listed --state all --limit 5 and scanned the slice for an OPEN PR. A busy reused branch could push the in-flight OPEN PR past the result cap, letting an older same-named merged PR flag still-open work as stale. Query OPEN independently (--state open) before looking for a merged PR, so the in-flight guarantee no longer depends on gh's implicit sort order or a result limit.
An expired or invalid gh token surfaces as "HTTP 401: Bad credentials", which persists for the whole run, so it belongs with the other run-fatal auth failures rather than the per-entry transient bucket. Misclassifying it as transient would silently skip every remaining entry and still exit 0. 403 stays out of the pattern: it can be a genuine per-entry blip.
parseInflight had no direct test — its exclusion of checked (- [x]) and non-🚧 lines rode only on the higher-level scan tests. Pin the contract directly so a widened checkbox pattern (e.g. [ ] -> [.]) that lets done work read as in-flight fails here, and assert scanTodo.inflightCount.
… rot List parseInflight among the ghRunner-tested exports and split out classifyGhError (fake error objects) and ghExec (fake exec) as their own seams so the roster matches what the tests actually exercise. Replace the orphaned (Finding 2) review-cycle reference on the rethrow comment with its plain meaning (never swallowed).
The Finding 1/2/4 prefixes named a past review cycle, not the behaviour under test. Describe the invariant instead; assertions are unchanged.
Consolidate the trailing `export { DEFAULT_TODO }` into an inline
`export const` so every export in the file is declared inline. No
behaviour change.
…alternatives - Replace the vacuous "no truncation dependency" test with one that pins the actual contract: an OPEN PR short-circuits resolveEntry before the merged query ever runs (a throwing merged route proves it). The old test passed the same either way because a hit on open never touched merged. - Add two classifyGhError cases so the HTTP-401 detection is pinned on each regex alternative independently (a bare HTTP 401 with no 'bad credentials' text, and a 'Bad credentials' message with no HTTP status), instead of relying on a single fixture that satisfies both alternatives at once.
… wording - Correct the header's test-coverage roster: parseInflight, parseEntry, and summarize are pure transforms pinned with literal fixtures, not ghRunner-dependent like resolveEntry/scanTodo/runScan. - Narrow the 'never flagged stale' / 'is NEVER stale' claims (header fix (a), resolveEntry's docstring, and its inline comments) to what actually holds: the branch reads clean WHILE an OPEN PR exists. A reused branch whose new work has not yet opened a PR still falls through to the MERGED check and reads stale — a branch-name-identity limitation, not a bug, tracked in the TODO backlog for a durable PR identity fix. No behaviour change.
…o-markers-false-verdicts
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
1 participant
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.
Summary
Fixes three false-verdict paths in
scripts/check-stale-todo-markers.mjs(the local backstop that flags stale in-flight 🚧 TODO markers whose PR merged outside/merge-pr). The script is refactored into pure / dependency-injected exports behind an import-guardedmain(), so its logic is unit-testable with in-memory fixtures and a fakeghRunner— no realgh, network, or auth needed.The three fixes
resolveEntrynow prefers an OPEN PR for the head branch, so still-in-flight work is never flagged stale by an older same-named merged PR.(#N)/(PR #N)number is now report-only (a warning), never a hard STALE verdict. A conforming in-flight entry names a(branch: X)and resolves via the branch path, which still exits 1 when stale.Error taxonomy & honest exit codes
GhUnavailable(ENOENT / auth / rate-limit — all persist for the run) stops the scan but still returns everything already found, so a stale marker confirmed before a mid-scan auth expiry is not silently discarded.GhTransient(single 404, network blip, unparseable JSON) skips one entry and continues.0= clean or partial scan (fail-soft),1= a genuine stale marker,2= TODO unreadable or an internal error.Tests
test/build/stale-todo-markers.test.ts— 23 in-memory cases pinning all three fixes plus the error taxonomy, the abort-preserves-stale behaviour, and the honest exit codes (mirrors the siblingtest/build/todo-hygiene.test.tspattern).Verification
pnpm compile✓ · Biome clean ✓gh, none stale, exit 0