Background
Non-blocking notes from code review on PR #177 (issue #166) that were explicitly deferred as "no change required for this merge." Comment-text only, zero behavioral impact — worth folding in whenever something next touches this section of workflows/ticketmill.js / .claude/workflows/ticketmill.js (lockstep pair).
Items
-
"below the TICKETMILL-TEST-HARNESS-SPLIT marker" is inverted in seven new comments — at (pre-merge line numbers, re-locate at fix time) :539, :1036, :1059, :1318, :1399, :1443, :8122, describing fetchGateStateBlocks, verifyGateState, and attachGateStateBlocks as living below the marker. They are declared above it and are in harness scope (tests/gate-state-read.test.js calls context.fetchGateStateBlocks(...) directly). The file's pre-existing uses of the phrase mean "the top-level execution block after the marker," so the new usage reverses an established convention and could mislead a future maintainer into thinking these functions aren't directly unit-testable.
-
verifyGateState's no-intent doc bullet is narrower than the code — it lists "the unit died before its first boundary," but runPool's isolated-throw path discards ctx wholesale, so a unit that died after a successful boundary post also lands in no-intent. Fails quiet in the safe direction; just tighten the doc bullet.
-
Stale line-number cross-reference in a comment above verifyGateState (workflows/ticketmill.js near line 4949) — cites "see :5871" for runPool's per-unit throw isolation, but that line is the closing brace of the merge-success return in reviewAndMerge, not runPool. The actual per-unit try/catch is in drainUnit (around line 6099 at review time). Fix the cross-reference.
Remember to keep workflows/ticketmill.js and .claude/workflows/ticketmill.js byte-identical (node scripts/lint-engine.js) when fixing these.
Source
Code review iteration 2 non-blocking notes, and a code review nit, on PR #177.
Background
Non-blocking notes from code review on PR #177 (issue #166) that were explicitly deferred as "no change required for this merge." Comment-text only, zero behavioral impact — worth folding in whenever something next touches this section of
workflows/ticketmill.js/.claude/workflows/ticketmill.js(lockstep pair).Items
"below the
TICKETMILL-TEST-HARNESS-SPLITmarker" is inverted in seven new comments — at (pre-merge line numbers, re-locate at fix time):539,:1036,:1059,:1318,:1399,:1443,:8122, describingfetchGateStateBlocks,verifyGateState, andattachGateStateBlocksas living below the marker. They are declared above it and are in harness scope (tests/gate-state-read.test.jscallscontext.fetchGateStateBlocks(...)directly). The file's pre-existing uses of the phrase mean "the top-level execution block after the marker," so the new usage reverses an established convention and could mislead a future maintainer into thinking these functions aren't directly unit-testable.verifyGateState'sno-intentdoc bullet is narrower than the code — it lists "the unit died before its first boundary," butrunPool's isolated-throw path discardsctxwholesale, so a unit that died after a successful boundary post also lands inno-intent. Fails quiet in the safe direction; just tighten the doc bullet.Stale line-number cross-reference in a comment above
verifyGateState(workflows/ticketmill.jsnear line 4949) — cites "see :5871" forrunPool's per-unit throw isolation, but that line is the closing brace of the merge-success return inreviewAndMerge, notrunPool. The actual per-unit try/catch is indrainUnit(around line 6099 at review time). Fix the cross-reference.Remember to keep
workflows/ticketmill.jsand.claude/workflows/ticketmill.jsbyte-identical (node scripts/lint-engine.js) when fixing these.Source
Code review iteration 2 non-blocking notes, and a code review nit, on PR #177.