fix(guard): my own PR put two FALSE entries in the unwired-lane guard — name the type - #2936
Closed
gsxdsm wants to merge 1 commit into
Closed
fix(guard): my own PR put two FALSE entries in the unwired-lane guard — name the type#2936gsxdsm wants to merge 1 commit into
gsxdsm wants to merge 1 commit into
Conversation
… — name the type #2875 (merged) added `InReviewDurationLanes` and wired it: `getInReviewDurationEventsImpl` builds both lane sets and passes them. The guard reported both properties as UNWIRED, so `unwired-lane-parameter-guard.test.ts` has been failing on main since — a guard I wrote, reporting the exact opposite of the truth, on my own change. The cause is the owner-scoping I added earlier: a mention only counts if it comes from a file that also names the DECLARING symbol. That is right for a function — a caller must name it to call it — and structurally impossible for an interface passed as an inferred object literal. The caller never writes `InReviewDurationLanes` and never will, because the type comes from the parameter. FIXED AT THE CALLER, NOT IN THE TOOL, after trying the tool three ways. Relaxing type-owned properties to the original loose rule hid TWELVE genuine entries. Resolving the owner to the functions that consume the type hid six. Each refinement traded the false positive for false negatives, which is the usual sign a co-occurrence heuristic has reached its limit — and a guard that under-reports is worse than one that occasionally over-reports, because nobody notices the silence. Annotating the local costs one word, makes the wiring visible to the reader as well as the tool, and leaves the rule alone. The alternative — recording two wired parameters in KNOWN_UNWIRED — would have put non-debt in the debt list, which is how a ratchet starts lying. Guard back to 9/9 with the baseline unchanged at 17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe workflow duration-event implementation now explicitly types its local lane mapping with the exported ChangesWorkflow duration typing
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/task-store/workflow-definitions.ts (1)
1012-1026: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the FNXC rationale while retaining the key decision.
This comment is substantially more detailed than necessary and duplicates implementation history. Keep the reason for naming the type and the tool-contract impact, but omit the heuristic experiments and PR-history details.
Suggested wording
/* FNXC:WorkflowLifecycleColumns 2026-07-31-06:20: - Named type, not an inferred literal, and the reason is a tool contract rather than style. - - `scripts/lib/unwired-lane-parameter.mjs` decides a lane parameter is WIRED when some other file - mentions both the parameter name and its declaring symbol. For an interface passed as an inferred - object literal there is no mention of the type anywhere, so this call site — which does supply - both lanes — was reported as unwired, and `#2875` landed two false entries into a guard written to - catch the opposite mistake. - - Annotating the local is the smallest honest fix. I tried three variations of the heuristic first; - each traded the false positive for false NEGATIVES (the widest hid twelve genuine entries), which - is the usual sign that a co-occurrence check has reached its limit. Naming the type costs one - word, makes the wiring visible to both the reader and the tool, and leaves the guard's rule alone. + Name the lane type so wiring analysis sees both lane parameters explicitly + without changing runtime resolution or fallback behavior. */As per coding guidelines, FNXC comments should be concise; decision-relevant causal provenance may remain when it prevents regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/task-store/workflow-definitions.ts` around lines 1012 - 1026, Shorten the FNXC comment associated with the workflow lifecycle columns to state only that the local uses a named type because the wiring tool requires references to the declaring symbol, preserving the tool-contract rationale. Remove the heuristic experiment results, false-positive counts, PR history, and other implementation-history details.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/task-store/workflow-definitions.ts`:
- Around line 1012-1026: Shorten the FNXC comment associated with the workflow
lifecycle columns to state only that the local uses a named type because the
wiring tool requires references to the declaring symbol, preserving the
tool-contract rationale. Remove the heuristic experiment results, false-positive
counts, PR history, and other implementation-history details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 636a6ac0-29d2-4b76-a7d4-57145dfbb8f8
📒 Files selected for processing (1)
packages/core/src/task-store/workflow-definitions.ts
Collaborator
Author
|
Superseded by #2925 — folded per the queue freeze. Commit Worth flagging for merge order: this is the commit that un-reds main's |
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…#2925, #2930, #2936) (#2925) **Consolidated per the queue freeze.** Three single-fix PRs of mine folded into this one branch; #2930 and #2936 are closed as superseded. Net effect on the queue: **3 → 1**. All three are the same root cause — a lifecycle lane compared against a legacy id — and all three carry a measured revert proof. Verified scoped (not full suite) on the folded branch: `tsc --noEmit` clean, `pnpm lint` clean, SQL-literal gate green, census `--strict` green, and 61 tests across five suites plus the guard at 9/9. --- ### 1. `getLiveTaskColumn` produced the archived sentinel from a literal (was #2925) `getLiveTaskColumn` **manufactures** the string `"archived"` that a dozen comparisons across five files trust — and it tested `row.column === "archived"`. A live row in a renamed archived lane read as **live**, so the gates hiding an archived card's artifacts and document listings never closed. Fixing those twelve comparisons individually would have been wrong twice over: **they are sentinels, and the defect was in the producer.** One line, once, and all twelve become correct. `resolveArchivedLanes` moved to `project-lane-vocabulary.ts` — three private copies of one fact is how the "write guard says yes, publication guard says no" disagreement happens at scale. *Revert proof (real PostgreSQL):* restore the literal → `expected [ { …(14) } ] to deeply equal []`. **Caught myself shipping the unwired shape here:** I added the parameter to seven functions and wired none of their impl callers — the exact inert-conversion defect this program exists to remove. The failing test is the only reason I noticed. ### 2. Mission delivery repair refused a completed card (was #2930) `getTerminalTaskEvidence` tested only `column === "done"`, so a completed card on a renamed board classified as `nonterminal` and `reconcileFeatureDoneWithTerminalTask` threw `TASK_NOT_TERMINAL: … not shipped`. Valid operator work refused — with the message naming the real column while the check couldn't see it. The **type** blocked the fix from the far end: `TerminalTaskEvidence` pinned `column: "done"` / `"archived"`, so the resolver couldn't report the real column without a compile error. `kind` already carries the role, so `column` is free to carry the truth. *Revert proof (real PostgreSQL):* restore the literal → `TerminalTaskReconciliationError: … not shipped`. I had deferred this twice on the premise that `AsyncMissionStore` "holds a layer, not a store". It holds an **optional `taskStore`**, and the single production construction site supplies it. ### 3. The unwired-lane guard reported two FALSE entries (was #2936) `unwired-lane-parameter-guard.test.ts` has been **red on main** since #2875, flagging two `InReviewDurationLanes` properties as unwired when the impl demonstrably supplies both. Cause: my own owner-scoping rule requires a mention from a file naming the declaring symbol — correct for a function, structurally impossible for an interface passed as an inferred object literal. Fixed at the caller (name the type) after trying the tool three ways: relaxing type-owned properties hid **12** genuine entries; resolving owners to consuming functions hid **6**. Each refinement traded the false positive for false negatives — the sign a co-occurrence heuristic has hit its limit. Recording two *wired* parameters in `KNOWN_UNWIRED` was rejected: that puts non-debt in the debt list, which is how a ratchet starts lying. Guard back to **9/9**, baseline unchanged at 17. **This un-reds main.** 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- 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
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.
unwired-lane-parameter-guard.test.tshas been failing on main since #2875 merged, and the two entries it reports are wrong.Both are wired —
getInReviewDurationEventsImplresolves the lane sets and passes them. So a guard I wrote, to catch parameters nobody supplies, was reporting the opposite of the truth about my own change.Cause: owner-scoping cannot apply to a type the caller never writes
The rule I added earlier requires a mention to come from a file that also names the declaring symbol. Correct for a function — a caller has to name it to call it. Structurally impossible for an interface passed as an inferred object literal:
Nothing there writes
InReviewDurationLanes, and nothing ever will — the type comes from the parameter.Fixed at the caller, after trying the tool three ways
Each heuristic refinement traded the false positive for false negatives. That is the usual sign a co-occurrence check has reached its limit — and an under-reporting guard is worse than an occasionally over-reporting one, because nobody notices the silence.
The other option was recording two wired parameters in
KNOWN_UNWIRED. That would put non-debt in the debt list, which is how a ratchet starts lying — the same failure this PR is fixing, one layer up.Naming the type is also just better code: the wiring is now visible to a reader, not only inferred.
Verification
unwired-lane-parameter-guard.test.ts— 9/9, baseline unchanged at 17pnpm test:gate— 161 / 487 / 13 / 71 passedpnpm lint— cleantsc --noEmit(@fusion/core) — cleanactivity-log-parity.pg.test.ts— 5 passed🤖 Generated with Claude Code
Summary by CodeRabbit