bb6c08d9d6fae702a366a1bee3fc75697edadde5
3746 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
gsxdsm
|
fb53a96eaa |
test(engine): the lease-seam alarm fired downward — re-point it, and close two ways it could pass without a fix (#2987)
## What happened Both of my source-level audits went red on the advance that landed #2975. They are exact counters, not floors, so this is the alarm working **downward** — the direction it was written for. #2975 converted the two self-healing `shouldHoldActiveFileScopeLease` call sites and closed the 2-of-4 seam that `workflow-file-scope-lease-caller-gap-live-e2e.pg.test.ts` was measuring. I judged the conversion real before updating anything. Both sites now derive their answers from `resolveProjectColumnsForRoles(...)` sets the sweep had already resolved a few lines above (`self-healing.ts:4754`, `:5825`) — trait membership, not a literal. So the numbers moved on purpose. ## The part that is not bookkeeping Re-pointing an audit to whatever the code now says is how a guard goes dead. Both assertions could have been satisfied by something that is **not** a fix, so both were tightened: | Way it could pass without a fix | Old assertion | Now | |---|---|---| | `isWipColumn: true` hardcoded at a self-healing site — the original defect wearing the converted call shape, answering "yes" for a blocker resting anywhere | only checked the key was *absent* | requires resolved-set membership: `/isWipColumn:\s*\w+\.has\(\w+\.column\)/` | | a site answering one of the two independent role questions and not the other | `includes(a) \|\| includes(b)` counted it as converted | `&&` | The scheduler's own two sites *do* pass literal `true`, correctly — they have already filtered to a role-resolved bucket, so there the answer is a fact about the loop, not about the card. The form check is scoped to `self-healing.ts` for that reason. ## Mutation evidence Not reasoned — measured. Each mutant applied to `self-healing.ts`, suite re-run, file restored: | Mutant | Result | |---|---| | baseline | 8 passed | | M1 — hardcode `isWipColumn: true` at one site | **1 failed** | | M2 — drop `isReviewColumn` at one site (half-converted) | **2 failed** | | M3 — revert both sites to pre-#2975 | **2 failed** | M2 is the one that justifies the `&&`: re-running it with the counter reverted to `||` leaves the file **green (4/4 passing)**. The old counter provably could not distinguish a closed seam from a half-closed one — the exact blind spot this file exists to remove. ## Verification `test:gate` exit 0 · live-PG E2E surface **171/171** · lifecycle-column census exit 0 · FNXC date ratchet exit 0 · `pnpm lint` clean. Production files untouched (`git status` clean on `self-healing.ts` after every mutant). ## Scope The other measured seam, `evaluateParkedAgentTaskLink`, is **unchanged at 2-of-6** — four callers still omit the resolved columns, so the class is not closed, only one of its two instances is. That remains characterized, not fixed, in the same file; converting those four is the capacity worker's file, not mine. The call-site facts are still asserted against source text rather than driven through the self-healing sweep, which would need the full dependency-lease reconcile harness. That limit was stated in the original file and still is. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated end-to-end workflow checks to verify resolved workflow-role values at active lease call sites. * Strengthened assertions for WIP and review column detection. * Updated audit coverage to reflect conversion of all active-file-scope lease callers. * Preserved tracking for the remaining parked-link integration points. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
gsxdsm
|
16921fc518 |
fix(engine,core): role resolution was half-done in two shared lifecycle predicates (surfacing family + file-scope leases) (#2975)
The three surfacing sweeps stopped reporting anything for a card resting in a board's **second** review or hold column. A lifecycle role is a **trait**, and any number of columns may carry it. The shared runner resolved it with `resolveLifecycleColumns()[role]` — **first match** — then gated on it: ```ts const roleColumn = lifecycle?.[spec.role]; // FIRST column carrying the trait if (task.column !== resolved.roleColumn) continue; // everything else dropped ``` A workflow that splits human sign-off from the merge lane has two review columns; one that parks dependency-blocked cards separately has two hold columns. Cards in the second got **no stale-paused-todo, no stale-paused-review, no in-review-stalled** diagnostic — silently, with no error, on all three sweeps at once. ## The second bug hiding inside the fix for the first Resolving membership but still reading `roleColumns[0]`'s declared `recovery` applies the **merge lane's** threshold to a card sitting in the **sign-off** lane. Each card's policy now comes from its own column, and one of the new cases fails if it doesn't: the first role column declares a policy that suppresses the signal, the card's own column declares one that fires. ## Reverted | | | |---|---| | **6 of 12** new cases fail | `fires for a card in the SECOND column carrying its role` and `reads the recovery policy of the card's OWN role column` — × 3 sweeps | | the other 6 pass either way | non-regression halves: still fires for the FIRST role column, still does **not** fire for a card outside every role column. Membership must widen the gate, not move it. | The pre-existing 45 cases were all green throughout — the single-role-column fixture could not express the case, which is why the table-driven file that exists to stop these three sweeps drifting apart never caught it. ## Verification `pnpm test:gate` 161 + 13 + 487 + 71 · surfacing family 57 · core stale-paused 20 · lint · census `--strict` · sql-literals · fnxc-dates · lane-wiring · changesets — all green. ## Note `holdColumns` was missing from the lane-wiring vocabulary, so the gate could not see that argument dropped. Added in the same commit. While reviewing, I found and measured **two problems in #2974** (comment posted there): six of its newly-visible sites are `satisfies`-wrapped false positives, and baselining them means deleting a real `reviewColumns` argument keeps the count unchanged and the gate green; and its baseline predates #2970, re-opening the slot that PR closed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved stale-card detection across all applicable review and hold columns. * Cards are now surfaced using the policies configured for their specific lifecycle column. * Cards outside matching lifecycle columns are no longer incorrectly surfaced. * Preserved existing fallback behavior when no lifecycle columns are configured. * **Tests** * Added coverage for workflows with split review and hold columns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- ## Second commit: the same predicate, half-converted (`shouldHoldActiveFileScopeLease`) Folded in here rather than stacked — same file, same class, and a stacked PR on an unmerged base is not mergeable. Reversible; say the word and I'll split it. `shouldHoldActiveFileScopeLease` is the **scheduler's** lease predicate, shared with the self-healing repair paths deliberately so the two cannot disagree about who holds a file-scope lease. Its two role answers are optional parameters defaulting to the legacy ids. The scheduler's own call sites were converted to pass resolved answers; self-healing's two were not: ```ts const isWipColumn = options?.isWipColumn ?? task.column === "in-progress"; const isReviewColumn = options?.isReviewColumn ?? task.column === "in-review"; ``` On a renamed board neither branch matches, so the predicate returns `false` for every card. The scheduler kept the lease; self-healing saw none, cleared `overlapBlockedBy`, and **released a dependent to edit files another agent still holds** — the outcome `groupOverlappingFiles` exists to prevent. Membership comes from the wip/review sets each sweep already resolved a few lines above, so this adds no reads. **Reverted:** both new cases fail with `overlapBlockedBy` = `null` — the release itself, not a proxy. The pre-existing legacy-column case in the same file passes either way, because `in-progress` satisfies the literal default; that is exactly why it never caught this. Lane-wiring baseline re-recorded `9 -> 7` in the same commit (the ratchet refused a stale allowance, as intended). **Verification:** gate 161 + 13 + 487 + 71 · surfacing 57 · overlap-seam + scheduler-lease + query-blindness 79 · core stale-paused 20 · lint · census `--strict` · sql-literals · fnxc-dates · changesets — green. |
||
|
gsxdsm
|
41af5e5dbd |
fix(gate): the lane-wiring census could not see its own motivating case (#2956) (#2974)
#2966 shipped a gate that **cannot detect the defect named first in its own header.** `findLaneAcceptingFunctions` matched a lane parameter only when `param.type` was a `TypeLiteralNode` — an inline `{ reviewColumns?: … }`. But the real code declares these as interfaces: ```ts export function getInReviewStallReason( task: Pick<Task, …>, context: InReviewStallContext = {}, // TypeReference — invisible ): InReviewStallSignal | undefined ``` so the function never entered `accepting` and none of its call sites were examined. ### Measured, both directions | | before | after | |---|---|---| | lane-accepting functions detected | 20 | **30** | | `getInReviewStallReason` detected | no | **yes** | | re-introduce #2956 (drop `reviewColumns` from one call site) | `none added` — **passes** | **fails**: `reads.ts: 7 unwired now, baseline allows 6` | The gate now catches the thing it was built for. ### The baseline moves 10 → 24, and that number needs context `10 unwired call site(s) across 8 files` → `24 across 15`. **No entry was removed** — every previously-recorded file kept its count and 14 sites became visible for the first time: ``` core/task-store/reads.ts 0 -> 6 engine/self-healing.ts 2 -> 4 core/task-store/branch-and-pr-entities.ts 0 -> 1 core/task-store/task-update.ts 0 -> 1 engine/scheduler.ts 0 -> 1 dashboard/routes/register-task-workflow-routes 0 -> 1 cli/commands/dashboard-tui/bucket-mapping.ts 0 -> 1 cli/extension.ts 0 -> 1 ``` **These are newly VISIBLE, not newly broken** — they have been unwired all along. I have **not** audited them, and recording them in the baseline is not a claim that they are fine; it is the ratchet doing what its header describes, since the census's own note says roughly half of the original hits were legitimately unwired (identity proven by a stronger means, sentinel columns, dead exports). Someone should walk the 14. Two stand out as worth a look first: **`reads.ts` at 6** is the file #2956 was about, and **`scheduler.ts`** is a dispatch path. Flagging rather than fixing, because wiring a call site that should not be wired is its own defect and each needs the judgement call the census header describes. ### Regression test `packages/engine/src/__tests__/lane-wiring-census-named-types.test.ts` pins the detector's shape — named interface, type alias, inline literal, positional — against fixtures rather than live counts, so it does not churn when someone legitimately wires a call site. Plus one anti-vacuity case asserting the named-type arm is still load-bearing on real source (`getInReviewStallReason` resolves in the live tree), so the fixtures cannot pass while the tool has quietly stopped applying here. **Mutation:** removing the `TypeReference` arm fails **4 of 5**. ### Also worth knowing `findLaneAcceptingFunctions` still only visits `ts.isFunctionDeclaration` at top level, so `export const fn = (ctx) => …` remains invisible. I checked — no exported arrow function currently takes a lane argument, so nothing is missed today, and I left it rather than widen the surface in the same change. Resolved by **name across the corpus** instead of a type-checker `Program`: these are plain source scans and a checker would cost a full type-resolution pass for one lookup. Two same-named types merge, which only ever widens what counts as wired — safe for a ratchet. **Verified:** 5/5 new tests, `check-lane-wiring` clean at the new baseline, lint clean, FNXC gate exit 0. Core suite on main is green (4923 passed / 0 failed) — unrelated, but I had it running. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved lane-wiring analysis to recognize named interfaces and type aliases. * Added support for wrapped configuration expressions and positional parameters when detecting lane information. * **Tests** * Added comprehensive coverage for lane-wiring detection, including named contexts and live-tree validation. * **Chores** * Updated baseline counts to reflect newly recognized application areas and improved self-healing detection. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
126cee7e6d |
engine: finalization parked ALREADY-MERGED work as failed on a renamed board (#2964)
**The worst symptom in this family: the branch landed, and the board says the task failed.** `project-engine`'s merge-confirmed finalization spread the task's **real** column into `getTaskHardMergeBlocker` with no `reviewColumns`, so the identity check ran against the literal `in-review`. On a renamed board it returned `task is in 'signoff', must be in 'in-review'`, and the caller parked the card: ``` status: "failed" error: "Merge confirmed but finalization blocked: task is in 'signoff', must be in 'in-review'" ``` For work that had already merged. ## Its sibling had already solved this `auto-merge-finalization.ts` passes the **review-eligible sentinel** instead of the card's own column, with the reasoning recorded at that site: `getTaskHardMergeBlocker` asks *"is this card blocked by anything other than where it sits?"*, and its callers are recovery paths for landed work that a graph crash can leave resting in any column. `project-engine` simply never got the same treatment. ## One name instead of two spellings Rather than write the sentinel a second time, it is exported once as `REVIEW_ELIGIBLE_SENTINEL_COLUMN` next to the helper whose contract gives it meaning, and both recovery paths use it. **Two sites independently spelling a magic value is how one of them came to be missing it** — that is the actual root cause here, not the literal itself. This also answers the census, which flagged the new literal — correctly. Its guidance (which I wrote, in #2909) is to hoist a deliberate literal into a *declaration*, where a `DELIBERATE-LITERAL` marker actually attaches, instead of leaving it mid-expression where the marker is silently ignored. The shared constant is exactly that, and it lowers `auto-merge-finalization`'s literal count too. ## Revert result | | reverted → | | --- | --- | | sentinel replaced by the card's own renamed column | reproduces the shipped string | The middle test asserts that string deliberately — it is what landed in `task.error`, so a regression reports what the operator would actually have seen. A third case checks the sentinel does **not** suppress genuine blockers: incomplete steps still block finalization in any lane. These drive the helper directly; reaching `project-engine`'s finalization end to end needs a live engine, a merge run and a real repo, while the defect is entirely in *what the blocker is asked*. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `project-engine` + `auto-merge-finalization` + the new suite, 207; `tsc` clean on core and engine; lint, census `--strict`, FNXC gate, changesets all clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed merge-confirmed tasks being finalized correctly when boards use renamed workflow columns. * Prevented already-merged tasks from being incorrectly marked as failed due to custom review-column names. * Preserved enforcement of genuine incomplete-step blockers. * **Tests** * Added coverage for finalization on renamed lanes and legitimate merge blockers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
8e0219d573 |
fix(merger-ai): gate the no-commits dep-sync skip on the branch diff — the P1 #2501 shipped without (#2958)
## #2501 merged without its P1 fix; this is that fix, alone #2501 has landed. Its review threads were resolved — I judged and fixed them — but its head was a fork branch I could not push to, so **the fixes were never in it**. Confirmed on `main` at `c1c1b964af`: ``` merger-ai.ts:902: if (ctx.noCommitsExpected === true) { ← bare flag, no diff gate merge-dependency-sync.ts: export const LOCKFILE_CANDIDATES → 0 matches ``` Rebasing dropped this PR's five duplicated base commits, so it is now **one commit**: the review fix and its regression. ## The defect on main The dep-sync skip trusts `ctx.noCommitsExpected` alone, and **only ever runs on a branch that has commits** — the `rev-list --count` short-circuit ~50 lines above returns `outcome: "empty"` at zero ahead, so control reaches it only when the branch is AHEAD. Nothing revalidates the flag. Both downstream empty-lane guards carve no-commits tasks out explicitly — `merger-ai.ts:1372` (#2259 already-landed proof) and `:1994` (FN-8141 executor veto) — and both guard the *opposite* direction: commit-expected task, empty branch. The inverse has no check. So a task marked no-commits whose executor committed a manifest or lockfile change gets its dependency install **and** its frozen-lockfile validation skipped, and the change lands unvalidated. ## The fix The flag says *look*; the branch diff decides. A `main...branch` diff touching `package.json` or any `LOCKFILE_CANDIDATES` entry falls through to the normal sync and emits an audit row with `skipOverridden: true`. An unreadable diff **also** syncs — matching the hard-fail contract documented directly above that block, rather than treating absence of evidence as evidence of safety. `LOCKFILE_CANDIDATES` is exported instead of duplicated, so the skip and the installer cannot drift on what counts as a dependency change. **Mutation-verified:** reverting to trust-the-flag fails exactly the new case and nothing else. The existing *"lands successfully with noCommitsExpected: true and actual changes"* case is untouched and still passes — `feature.txt` is not a dependency file, so an ordinary source change on a no-commits task still skips. The new case differs only in *which* file the branch touches. ## Also carried over from the #2501 review **coderabbit's env nit** — `process.env.X = undefined` stores the string `"undefined"`, leaving a previously-absent var truthy and leaking into later tests. `restoreEnv` applied at both sites. **Both entry paths** — deferred with reasons: `runAiMerge`/`landWorkspaceTask` sit behind real worktrees, sessions and a merge agent, and the cheap version is a mirrored-implementation test that cannot fail on a revert (this repo has deleted two of those). The fix above also means propagation is no longer the only thing between a stale flag and an unvalidated lockfile. ## A correction to my own work My first version of the regression committed the lockfile while the fixture had left the tree on `main`, so the `main...branch` diff could not see it and the case **passed for the wrong reason**. Corrected, with the reason recorded in the test. ## Verification - `merger-ai-no-commits-deps-skip` — **5/5**, mutation-verified - `merge-dependency-sync-lockfile-heal` — **10/10** - engine typecheck — clean - `pnpm lint` — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Drew Donaldson
|
c1c1b964af |
fix(dashboard): expose full permission-mapped task toolset in chat sessions (#2376)
## Bug Chat-session tool surface missing task-mutation tools that exist outside of chat, even when the agent's permission record grants them.\n\nRepro: agent-09dcf8b2 (role: custom, CEO) in NextGenEHS has tasks:archive / tasks:delete / tasks:merge / tasks:retry / tasks:update true in its permission record with permissionPolicy.presetId = unrestricted and task_agent_mutation = allow. Calling fn_task_archive / fn_task_delete / fn_task_merge in chat returns: Tool fn_task_* not found.\n\nRoot cause: packages/dashboard/src/chat.ts createChatFusionToolset() built a hardcoded narrow chat-only allowlist while heartbeat registered the complete lifecycle surface unconditionally.\n\nFix:\n- Add exported factories in packages/engine/src/agent-tools.ts for missing lifecycle tools: fn_task_archive, fn_task_unarchive, fn_task_delete, fn_task_retry, fn_task_pause, fn_task_unpause, fn_task_duplicate, fn_task_merge, fn_task_update, fn_task_add_dep, fn_task_promote, fn_trait_list, fn_ask_question, fn_reflect_on_performance, fn_read_evaluations, fn_update_identity, fn_send_message, fn_read_messages.\n- Wire those factories into createChatFusionToolset(). Mission/ideation mutations stay behind missionMutationGated. Agent-scoped tools still require agentId.\n- Re-export from packages/engine/src/index.ts.\n- Regression test: packages/dashboard/src/__tests__/chat-toolset-permissions.test.ts (3/3 passing). Existing chat.test.ts (14/14 passing). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Chat now exposes task lifecycle actions—including archive, retry, pause, duplicate, merge, and dependency updates—when permitted by the agent’s action controls. - Added support for identity updates and evaluation viewing in agent-linked chats. - Existing read-only tools remain available, while restricted actions stay hidden when authorization is unavailable. - **Tests** - Added regression coverage for authorized and unauthorized chat tool surfaces, including preservation of read-only capabilities. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: gsxdsm <gsxdsm@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
189f237a07 |
fix(security): make the approval and permission gates actually enforce (#2457)
## Why
An AI agent autonomously deleted a live task (FN-8609) via
`fn_task_delete`. Investigating that led to two audits, which found the
gate machinery is well-built and its *enforcement* well-tested — but the
*decision boundary* was unguarded and the tools that matter never
reached the gate at all.
The headline defect: all ~76 tools in `packages/cli/src/extension.ts`
are delivered through `hostExtensionPaths` → `DefaultResourceLoader`,
which never passes through `wrapToolsWithActionGate`. `fn_task_delete`
*was* classified as gated in `gating-classifications.ts` — that
classification was dead code. Roughly 15 destructive tools were ungated
even for an operator who had locked their policy all the way down.
Second: `POST /api/approvals/:id/decision` took the approver identity
from the request body, validated for shape only, with no check that the
decider was human or differed from the requester. Combined with a bash
tool whose command string was never inspected (only `params.cwd`), an
agent could read the daemon token from `~/.fusion/settings.json`, find
its own pending request, and approve it.
## Scope
This does **not** tighten defaults. The `unrestricted` preset is
byte-for-byte unchanged — `git diff` on `agent-permission-policy.ts` is
empty — and regression tests assert that an out-of-the-box install
behaves exactly as before. The bug was never "the default is too
permissive"; it was "strict policy doesn't enforce." This makes turning
security up actually work.
The one deliberate exception: the containment that stops an agent
escalating its *own* privileges (reading the daemon token / credentials,
calling the approvals API to self-approve) applies at every preset
including `unrestricted`. That is a privilege-escalation boundary rather
than a permission preference — if it only engaged under strict policy it
would not have prevented the incident that prompted this.
## What changed
8 bisectable commits:
- **Approval lifecycle** — self-approval blocked via server-derived
deciders; same-verdict replay 409s; decide re-reads and re-validates
inside the transaction; expiry TTLs; `markCompleted` ownership check;
session identity registry in core.
- **Engine gates enforce for real** — unclassified tools resolve to a
policy-governed category instead of hardcoded `allow`; missing-policy
fail-open closed; bash containment floor + exact-command approval
binding.
- **Dashboard decision routes** — stop trusting client-supplied actors
(decision, bypass-review, worktrunk → 403 on forged actors).
- **`fn serve` authenticated by default** — auto-mints a token following
the existing `fn dashboard` precedent; `--no-auth` opts out.
- **Sibling entry points closed** — user-sourced hard-cancel moves, ACP
execute-once approvals, plugin task-store gating.
- **pi-extension principal resolution** — the extension resolves the
acting principal and can withhold or policy-gate the previously ungated
destructive tools.
- **Root-cause bonus fix** — `findLatestByDedupeKey` was broken in
PostgreSQL backend mode (already-parsed jsonb fed through a string-only
parser), so approved-grant redemption **never matched in production**,
minting duplicate requests. This explains the live DB state of 17
approved / 0 completed. *(Also cherry-picked to `main` as `a9b30013bb`,
since it is an active production defect on its own.)*
- **Review follow-ups** (`627f1b1fa8`) — operator-configured
provisioning privilege and a configurable grant TTL; see below.
## Review follow-ups
**Provisioning privilege is operator-configured, not role-derived.**
`isCallerPrivileged` had gone from `caller.reportsTo == null` (every
top-level agent privileged — permanent escalation by creating a
manager-less agent) to `caller.role === "ceo"`, which swapped an
implicit rule for a magic string: any agent config can claim that role,
while an operator who genuinely wants a privileged agent had no
supported way to say so. Privilege now derives solely from
`agentProvisioning.trustedAgentIds` / `trustedRoles` and fails closed
when settings are unresolvable.
It is also no longer forwarded to `resolveAgentProvisioningPolicy` as
`isPrivileged`, because that flag short-circuits ahead of
`alwaysApproveDelete` — a trusted caller was bypassing delete approval
entirely. The policy applies the same trusted rules itself, in the right
order. The function now governs only the org-chart escape hatch (acting
outside your own direct reports).
**Grant TTL defaults to 1 hour and is configurable.** Approval →
redemption is not instantaneous: an operator approving from their phone,
an engine restart, a queued lane, or a task waiting on a worktree all
routinely exceeded 15 minutes, after which the grant expired and the
agent silently re-requested. One hour remains far short of the
"redeemable forever" hazard the TTL exists to bound. Override via
`FUSION_APPROVAL_GRANT_TTL_MS` or `configureApprovalRequestTtls()`;
invalid overrides are ignored rather than widening the window to
infinity or collapsing it to zero.
## Behavior changes requiring operator review before rollout
1. `fn serve` requires a bearer token by default (`--no-auth` opts out);
unauthenticated clients get 401.
2. Agents can no longer run withheld destructive tools
(`fn_task_delete`, `fn_task_bypass_review`,
mission/milestone/slice/feature/workflow deletes, `experiment_finalize`,
`skills_install`). Operators keep them via CLI/dashboard. **This is the
incident fix.**
3. Agents get provisioning privilege only when the operator lists them
in `agentProvisioning.trustedAgentIds` / `trustedRoles`; the
provisioning gate is now live in production. Previously-implicit
privilege (top-level position, or a `ceo` role) no longer grants
anything on its own.
4. Decision replay 409s (was 200); pending approvals expire after 24h,
approved grants after 1h (configurable); bash approvals bind per exact
command.
5. Forged/body actors on decision, bypass-review, worktrunk routes →
403; `archive-all-done` requires `{confirm:true}` (external scripts
affected).
6. `fn_secret_get` approvals grant exactly one reveal (previously
granted nothing and looped forever); ACP approvals are execute-once
(previously infinite reuse).
7. Bash containment denies token/credential/approvals-API commands in
all agent sessions at every preset.
## Verification
Independently re-run against the branch, not just self-reported:
- 5 typechecks (core, engine, cli, dashboard `tsconfig.json` +
`tsconfig.app.json`) — clean
- `pnpm lint` — clean
- `pnpm test:gate` — 379 passed
- `pnpm build --force` — green (a plain `pnpm build` skips packages as
unchanged and does **not** compile the branch)
- `pnpm check:changesets` — clean
- ~650 file-scoped tests including new negative-path suites for the
decision boundary, which previously had **zero** test coverage
`packages/engine/src/__tests__/plugin-runner.test.ts` fails 56/80 —
**verified pre-existing**, reproducing identically at base commit
`93a403af67` on `main`. Not in the merge gate.
### A mutation check that failed to fail
Worth recording, because it nearly shipped an untested security fix. The
first mutation check on the provisioning change reintroduced the `ceo`
hardcode and **all 17 tests still passed** — the tests asserted through
the policy path, which can no longer observe `isCallerPrivileged` at
all, precisely because `isPrivileged` is no longer forwarded there.
Org-chart cases that do exercise the function were added; the hardcode
now fails exactly 1 of 19, and restoring is green. A green mutation run
is only meaningful if the test can actually see the code under test.
## Known limitations (stated, not papered over)
- The bash containment floor is string-matching: a cost-raiser, not a
sandbox. Quoting, encoding, `$HOME`, symlinks, or an interpreter
one-liner can evade it. The durable protection is the decision route
refusing agent-originated deciders — the filter is the belt, not the
braces.
- Approval expiry is lazy (evaluated at decide/complete/redeem), not
swept, so an expired pending row stays visible in lists until touched.
- The extension's require-approval path returns a pending message but
cannot suspend a pi session mid-turn; engine-side pause hooks cover
engine lanes only.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Security**
* Hardened approval and permission gating with server-side decider
attribution, self-approval blocking, ownership checks, replay/race
protection, and status/TTL enforcement.
* Added fail-closed behavior for sensitive/unclassified tools and
sandbox provisioning approvals.
* Blocked credential/approval access via bash containment; plugin
destructive task operations now require explicit permission.
* **New Features**
* `fn serve` now defaults to bearer-token auth, with `--no-auth` as the
explicit opt-out.
* **Bug Fixes**
* Improved task move-source attribution (`moveSource: "user"`) and
tightened dashboard archive/bypass confirmation and operator attribution
behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
gsxdsm
|
7712e0ada2 |
engine: merging was broken outright on a board with a renamed review lane (#2963)
**Not a degraded message — no task on such a board could be merged at all.** `getTaskMergeBlocker`'s column-identity check *returns a blocker* when the task's column is not a review lane. Both merge entry points called it without `reviewColumns`, so the check ran against the literal `in-review`: ``` Cannot merge FN-1: task is in 'signoff', must be in 'in-review' ``` `aiMergeTask` (`merger.ts`) and `runAiMerge` (`merger-ai.ts`) turn that into a thrown error. Every merge on a renamed board fails, with a message naming a column the board does not have. ## This exact defect was already found once The helper's own FNXC comment records it, in `moves.ts`: > *"so on a renamed board that move threw `Cannot move FN-1 to done: task is in 'signoff', must be in 'in-review'` even though the transition had just been validated as legal. A half-conversion, where the outer question is resolved and the inner one is not."* That fix added the `reviewColumns` option and wired `moves.ts`. **These two callers were missed** — same shape, one layer out. A fix that adds an optional parameter is only as good as the call-site sweep that follows it. ## How it was found By enumerating the call sites of every lane-taking helper, rather than trusting the `unwired-lane-parameter` guard. That guard is deliberately conservative — a mention of the parameter *anywhere* satisfies it — so **partial** wiring is invisible to it, and `reviewColumns` is mentioned plentifully elsewhere. This is the method #2956 used on a sibling defect, applied to every seam I have touched. ## Two sites deliberately unchanged - **`moves.ts`** passes `skipColumnIdentityCheck: true`. It has already proven lane identity from resolved IR traits, so supplying lanes *as well* would be contradictory rather than additive — the helper's comment is explicit that the two options answer different questions. - **`isTaskReadyForMerge`** has **zero** production callers. Adding a parameter there is precisely the unwired-parameter anti-pattern this program keeps removing. ## Revert result | | reverted → | | --- | --- | | `reviewColumns` at either call | reproduces the shipped string exactly | The middle test pins that string deliberately: it is the operator-visible failure, so if the wiring regresses the test says what the operator would have seen. A third case checks that supplying lanes does **not** switch the identity check off — a card in the wip lane is still blocked, and the message names the resolved lanes rather than a column the board lacks. The cases drive `getTaskMergeBlocker` directly: reaching it through the merge entry points needs a real repo, worktree and merge run, while the defect is entirely in *which columns the blocker is asked about*. The wiring itself is covered by tsc and the guard. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `merger` + `merger-ai` + `self-healing` suites 461; `tsc` engine clean; lint, census `--strict`, FNXC gate, changesets all clean. |
||
|
ischindl
|
8d6acf1314 |
fix(RUFU-018): add noCommitsExpected dep-sync skip and corepack/pnpm env passthrough (#2501)
Manually land RUFU-018 fix bypassing the AI merge pipeline. ## Summary - Add `noCommitsExpected` flag to `LandRepoContext`; skip dependency sync when set - Forward `COREPACK_HOME`/`PNPM_HOME`/`npm_config_registry` in `installWorktreeDependencies` - Add comprehensive tests for both changes This unblocks all downstream RUFU audit tasks. ## Surface Enumeration - Providers/bridges: `installWorktreeDependencies` called from `landOneRepo` (AI merge) and legacy `merger.ts`; `landOneRepo` called from `runAiMerge` and `landWorkspaceTask` - Data states: `noCommitsExpected` can be `true`, `false`, or `undefined` — both callers use `=== true` strict check <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved support for tasks that do not produce commits by skipping unnecessary dependency installation during merges. - Preserved normal merge and review behavior when dependency installation is skipped. - **Bug Fixes** - Dependency installation now correctly preserves relevant package-manager and system environment settings. - Reduced installation failures caused by missing or unavailable package-manager configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Fusion <noreply@runfusion.ai> Co-authored-by: gsxdsm <gsxdsm@users.noreply.github.com> |
||
|
gsxdsm
|
01f081e8aa |
engine: restore the stall-signal lane wiring #2951 dropped (and the test that proved it) (#2961)
**My defect, shipped in #2951 — and the same family as the one #2956 just fixed.** Found by auditing my own seams after that, not by a failing check. ## What is on `main` right now `surfaceInReviewStalls` reads the project's review columns (converted in #2951), then calls `getInReviewStallReason` **without** `reviewColumns`. The classifier falls back to the literal `in-review`, returns no signal for a renamed-lane card, and the sweep surfaces nothing. That is the textbook **missed pair** this program has a ratchet for: a widened read handing every renamed-board card to a literal classifier. The resolve work happens and is then discarded. On a renamed board an operator sees no stall warnings at all. #2951's conflict resolution dropped two things together: - the per-card `stallLanes` map and the `reviewColumns` argument - **the test that proved the wiring** ## Why nothing caught it **A deleted test cannot fail.** I verified that rebase by comparing the 68 conflict *hunks* — stripping FNXC stamps, confirming 0 of 68 had real content differences — and then ran the gate. The gate passed precisely because the proving test had gone with the code it proved. I verified the conflicts. I did not verify the outcome. Those are different things, and the difference is invisible when the evidence disappears alongside the feature. The `unwired-lane-parameter` guard cannot catch this either, by design: it is deliberately conservative — a mention of the parameter *anywhere* satisfies it — so **partial** wiring is outside its reach. `reviewColumns` is mentioned plenty in `reads.ts`, so the guard is green while this call site goes unwired. ## How I found it The check #2956 used on the sibling defect, applied to every lane seam I have touched: enumerate each function's **call sites** and confirm each one carries the parameter. That enumeration also flags several other call sites without `reviewColumns`/lane arguments (`merger.ts`, `moves.ts`, `auto-merge-finalization.ts`, `merger-ai.ts`, `project-engine.ts`) — I have **not** touched those here; they need per-site judgement about whether the lane answer is even available, and that is a separate change rather than a sweep. ## Revert result | | reverted → | | --- | --- | | `reviewColumns` at the call (i.e. exactly what #2951 shipped) | fails the restored test | ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; blindness suite 71; `self-healing.test.ts` 412; `tsc` engine clean; lint, census `--strict`, FNXC gate, changesets all clean. |
||
|
gsxdsm
|
fd795883c5 |
feat(missions): per-mission taskPrefix override for triaged task ids (#2347)
## Summary Maintainer re-land of [#2334](https://github.com/Runfusion/Fusion/pull/2334) (fork `flexi767:feat/per-mission-task-prefix`) after resolving merge conflicts with current `main`. Fork push was unavailable despite `maintainerCanModify`, so this branch carries the conflict resolution. ### Feature - Optional per-mission `taskPrefix` for triaged task ids (inherits project prefix when unset) - Dashboard MissionManager + routes + store/triage plumbing - Postgres migration for `project.missions.task_prefix` ### Conflict resolution - Main claimed migration **0026** (bigint counters) and **0027** (workflow IR pin) - Mission task-prefix migration renumbered **0026 → 0028** - Baseline `0000_initial.sql` includes `task_prefix` on missions - `legacy.ts` keeps code-org re-exports; `missions.ts` carries `taskPrefix` on create/update types ## Test plan - [ ] CI green (lint/typecheck/build/gate) - [ ] Create mission with custom prefix; triage feature → task ids use that prefix - [ ] Clear mission prefix via PATCH null; new tasks inherit project prefix Closes / supersedes #2334 once this lands (or re-point the fork PR). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Missions can now set an optional per-mission task ID prefix (overriding the project default). * Added task prefix support to mission create/edit UI and dashboard APIs, including normalized uppercase values and validation. * **Bug Fixes** * Improved commit hook generation for custom prefixes and special characters, with safer shell handling to prevent unsafe interpretation. * **Chores** * Added PostgreSQL migration and schema-applier support to persist and propagate mission task prefixes, including upgrade/backfill coverage. * **Tests** * Added backend and UI/API test coverage for task-prefix creation, clearing, and ID minting behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
gsxdsm
|
e6e70a2562 |
test(engine): delete two temp-cleanup mechanisms guarding a leak the harness already prevents (#2960)
`scheduler-paused-dispatch-refusal.test.ts` carried **two** tracking arrays and **two** `afterEach` hooks, both collecting the same `mkdtempSync` path and removing it twice. One was added per review round on #2779 — I wrote both, and neither round noticed the other. The obvious fix is to merge them into one. **I checked whether the leak was real first, and it isn't.** ### Measured `packages/core/src/__test-utils__/vitest-setup.ts` **redirects `os.tmpdir()`** to a per-worker sink and sweeps it by owning pid. So `tmpdir()` inside a test does not resolve to the real temp root at all. Probing the paths this file actually creates: ``` /var/folders/.../T/fusion-test-workers-8Tv8um/redir-5845/fusion-paused-dispatch-ZUhUVL ``` | run | fixtures created | left behind | |---|---|---| | cleanup as shipped | 4 | 0 | | **cleanup disabled** | 4 | **0** | The sink is reclaimed either way. Both mechanisms were appeasing a review comment about a problem that could not occur. ### Why deleted rather than merged A cleanup that cannot be observed to clean anything is not a cheap safety net — it is a claim the file cannot back, and it misreports which layer owns temp lifetime. Keeping one "just in case" would leave the next reader believing this file manages its own fixtures. If the redirect is ever removed, cleanup belongs in the shared setup for **every** test, not re-added file by file. An FNXC note records the measurement and says exactly that, so a third round doesn't re-add a third copy. ### A note on my own measurement My first check was `ls $TMPDIR/fusion-paused-dispatch-*` before and after — it reported zero leaked with cleanup **on**, which I nearly took as "cleanup works." It also reported zero with cleanup **off**. That contradiction is the only reason I looked further; the glob was measuring a directory the fixtures never reach. The before/after count would have "confirmed" a working cleanup just as readily as a redundant one. **Verified:** 4/4 pass, `tsc` 0 errors, lint clean, FNXC gate exit 0. Test-only, no product change, no changeset. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
8e5e1147d2 |
core,engine: the last literal lifecycle query — and the three stall signals that disagreed (#2951)
**This is the last one.** `surfaceInReviewStalls` was the final literal
`listTasks({ column })` in production — I verified it by direct scan,
not by census arithmetic: **1 remaining before this, 0 after.**
It tells an operator that a card is stalled in review. On a renamed
board the stall was real and the board simply never said so.
## It came last on purpose
Converting the read alone would have been **worse than leaving it**.
`getInReviewStallReason` gated on the literal `in-review` itself, so a
widened read hands every renamed-board card to a classifier that drops
it — the missed-pair class, wearing the shape of a clean one-line
conversion.
## What was actually there
Three sibling signals decorate the same row, and they **disagreed about
which lane it is in**:
| signal | before |
| --- | --- |
| `getInReviewStalledSignal` | singular `reviewColumn` — resolved, but
**first-per-role** |
| `getStalePausedReviewSignal` | singular `reviewColumn` — same |
| `getInReviewStallReason` | **no seam at all** — literal |
So one row could be judged in-review by one signal and not by another.
And the singular ones are the **arity trap**:
`resolveLifecycleColumns().review` is the *first* column carrying a
review role, so a board with a separate merge lane beside its
human-review lane had a second review column matching none of them.
All three now take `reviewColumns` (membership), resolved **once per
row** through `resolveReviewColumns` — the union of the three review
roles — so they cannot disagree by construction. The singular/literal
paths remain as the no-metadata fallback, so a caller passing nothing is
byte-identical to today. Ten call sites in `reads.ts` wired from that
one answer; the singular resolver is deleted.
## Revert results
Each applied alone and re-run:
| conversion | reverted → |
| --- | --- |
| the resolved read | fails — the card is never listed |
| `reviewColumns` at the call | fails — the classifier drops the renamed
card the widened read just found |
That second row is the whole point: it proves the pair had to move
together, which is the thing I got wrong twice earlier in this series.
## Second commit: a red on `main`, not from this branch
`check-fnxc-future-dates` landed and **`main` fails it** — verified by
running the script on a clean `origin/main` checkout rather than
inferring. Nine files carry stamps dated after today, so every worker's
gate fails on a check none of their changes caused. Several are mine: I
had been stamping tomorrow's date across this whole series, which is
precisely the out-of-order record the check exists to prevent.
Scope held deliberately: a repo-wide sweep touched **266 files** across
docs, scripts and every package. I ran it, backed it out, and limited
this to the nine files the check actually flags — a mechanical rewrite
that size during a queue freeze would conflict with every in-flight
branch, which is worse than the red it fixes.
## Verification
`pnpm test:gate` 161 + 487 + 13 + 71 (green **only** with the stamp
commit); `@fusion/core` full suite **4810 passed**; engine self-healing
+ blindness + both ratchets **758 passed**; `tsc` clean on core and
engine; `pnpm lint`, `check:changesets`, `lifecycle-column-census
--strict`, `check-sql-column-literals` and `check-fnxc-future-dates` all
clean.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Review-stall detection now recognizes renamed and multiple review
columns while retaining support for the legacy review column.
- Paused tasks continue to be excluded from stall detection.
- Self-healing review-stall sweeps now search all configured review
lanes and avoid duplicate task results.
- **Tests**
- Added regression coverage for renamed and legacy review-lane queries.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
gsxdsm
|
f49e487d91 |
feat(core): untraited-project lane opt-in — and main was red on the FNXC gate (#2949)
Two things, and the second is why the first does not ship alone. ## The opt-in `resolveProjectColumnsForRoles` gains `untraitedProject: "declared-columns"`. When **no** workflow in the project expresses **any** lifecycle trait, every declared column id joins the answer. This is the three-state rule at **project** scope — the last item on the deferred list, recorded at three self-healing call sites (#2869, #2876). A board that renames its lanes and declares no traits contributes nothing today, so its cards are **absent from every role-keyed query**, and the correct per-card fallback downstream never runs for them. A fallback cannot rescue a card the query never returned. **Not "no workflow declares this role."** A project that expresses traits and has no review lane has *answered*; widening there would invent lanes it deliberately lacks. Mutation-verified both directions — widening unconditionally fails 1 of 12, making the option a no-op fails 1 of 12. **Opt-in, not default**, because the safe direction differs per caller — the finding in `project-union-versus-per-task-lanes.md`: | caller | over-inclusion costs | |---|---| | sweep | nothing — the per-card check discards the extra rows | | aggregator | an inflated number an operator reads (#2864, #2866) | | action site | a card routed or notified under a vocabulary that is not its own (#2852, #2891) | Making it the default moves all three at once, in the one direction two of them must not. Verified byte-identical without the option, so this lands with **no caller changes** and each site adopts it on its own reasoning. ## Main was red, and my own gate caught me first I dated the new comments `2026-07-31` while today is `2026-07-30` — **the exact defect `check-fnxc-future-dates` exists to prevent, committed while writing the feature.** The gate I added yesterday failed my own commit. Correcting mine surfaced that the merged sentinel batch, #2947, and three engine test files carried future-dated stamps too, so **the gate was failing on `main` for everyone**, not just here. All corrected to real dates rather than raising the ceiling. The stamps were simply wrong, and a baseline bump would have recorded the error as permitted — which is the failure mode that ratchet exists to prevent. Core and engine `tsc` clean, `pnpm lint` clean, census `--strict` 0, FNXC gate 0 (469 known, none added), gate green (161/487/13/71). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
c3df0f641b |
executor: orphaned tasks were never resumed after a restart on a renamed board (#2947)
`resumeOrphaned` is the only path that recovers tasks after a crash or
restart. On a board with renamed columns it recovered **nothing**.
## A missed pair, not an unconverted read
```ts
const tasks = await this.listWipLaneTasks(); // resolved by role — already converted
const inProgress = tasks.filter(
(t) => t.column === "in-progress" && …, // literal — discards everything the read found
);
```
The read was already resolved. The filter directly beneath it
re-asserted the literal on the rows that read returned, so the sweep
found the orphans and threw them all away.
**This is the worse half of the class, and it hid well:**
- the read *looks* converted, so scanning for `listTasks({ column: "…"
})` finds nothing;
- the census scores only the comparison, so the backlog number moves the
**wrong way** as you convert;
- a **structural test already existed** pinning "the read asks for
resolved lanes" — `executor-resume-query-lanes.test.ts` — and it was
green the entire time the sweep was dead. A test asserting the read
exists says nothing about the filter beneath it.
The failure only surfaces after a crash, when an operator is already
investigating the crash and has every reason to blame that instead.
## The ratchet, generalised
#2944 ratcheted this class inside `self-healing.ts` after review found
one instance and a follow-up audit found five more. This generalises it
to every engine source: a function that resolves lanes **and** compares
a column id in the same body is a pair.
Excluded, deliberately:
- the **fallback arm** of a resolved ternary (`lanes ? lanes.has(c) : c
=== "done"`) — the correct shape;
- four files whose literals are deliberate, each with the reason
recorded: `ephemeral-worker-manager` (unresolvable-workflow default),
`triage` (the U11 orphan case), `scheduler` and `replan-target` (sync
listeners on the inert sync IR reader, already pinned by
`sync-workflow-ir-is-always-default.pg.test.ts`);
- `self-healing.ts`, because it has a **dedicated** ratchet that is
strictly more precise. Two ratchets allowlisting the same site is one
fact with two owners, free to drift — the exact failure mode this
program keeps hitting. One file, one ratchet.
It carries a positive control: a wrong source path would make every case
pass by scanning nothing.
**I swept the rest of the engine with it and executor.ts was the only
genuine hit** — everything else is documented-deliberate or blocked on
the inert sync reader.
## Revert results
Each measured by restoring the literal filter and re-running:
| | reverted → |
| --- | --- |
| behavioural case | fails — the renamed card is dropped and the sweep
returns before touching it |
| the ratchet | fails, naming the site: `resumeOrphaned:
executor.ts:5974` |
A non-vacuous companion (card in the review lane → not resumed) rules
out a filter that matches everything: a card in review has no session to
resume, and re-dispatching it would restart finished work.
**Measured:** `executor.ts` column guards 8 → 7; baseline re-recorded
downward.
## Verification
`pnpm test:gate` 161 + 487 + 13 + 71; executor prompt/soft-delete/resume
suites plus the new ratchet, 357 passed; `tsc` engine clean; `pnpm
lint`, `check:changesets`, census `--strict` and
`check-sql-column-literals` clean, each run explicitly.
|
||
|
gsxdsm
|
8b75a42d22 |
batch: self-healing sweeps were blind on renamed boards (26 sweeps, folds 23 PRs) (#2944)
**Consolidation of 23 open PRs into one.** Every one shared a single root cause and mostly touched a single file; 23 CI runs for that was indefensible. Folds and supersedes: #2867 #2869 #2876 #2879 #2883 #2891 #2899 #2901 #2902 #2905 #2906 #2914 #2916 #2918 #2919 #2920 #2922 #2927 #2929 #2932 #2934 #2937 #2939. (#2865, #2882, #2897, #2909, #2912 already merged and are not re-folded.) ## The root cause A self-healing sweep selects its work with `listTasks({ column: "in-review" })`. On a board whose lanes are renamed that returns **nothing**, so the sweep never runs — no error, no log line, no failed task. Several sweeps had already had their *predicates* converted to resolved lanes, which dropped a census count and changed nothing, because the query above the loop had already returned an empty list. **26 sweeps converted.** Each one: read the project's columns for the role, then decide each card against **its own** workflow, with the legacy ids unioned so a board mid-rename is never skipped. ## What each sweep stops silently failing to do | | | | --- | --- | | stale merger status | one finished card held the **merge queue** for everything behind it | | stale `blockedBy` / completed-task release | dependents stayed blocked on work that had already finished — the board stops moving | | workspace partial lands | a task left with **some repos merged and some not** | | mid-merge retry stamp | the card stalled *and* the operator's manual Retry was gated by the same stamp | | in-progress limbo / no-progress failures | dead cards held a work slot forever | | partial-progress retry | real work parked failed with its **retry budget unspent** | | orphaned-execution signal | visibility only — the one signal pointing at an orphan went silent | | zero-commit audit | went **half-blind**: the error arm kept working, the lane arm did not | Plus: ghost review cards, transient merge failures, misclassified failures, branch misbinding, missing-worktree failures, merged-but-unfinished finalization, done-metadata repair, self-owned branch conflicts, orphan-only scope violations, post-done wedges, idle assigned agents, PR-conflict worktree ownership, and orphaned workspace worktrees. ## Two defects the conversion itself introduced, both caught and fixed 1. **Missed pairs.** Widening a read without converting the guards beneath it is *worse than not converting*: the sweep starts admitting renamed-board cards and then mis-decides every one. Review caught a second guard on a re-read row; the audit that triggered found **five more**, one of which gates the `reviewProof` triple-proof — a renamed review card would have been moved backward with the safety check silently skipped. Column guards 86 → 81. 2. **Duplicate processing.** The literal reads were disjoint by construction; resolved reads are not, so a column carrying two role flags put one card in two buckets — duplicate moves, duplicate audit rows, inflated counts. Both now have ratchets. `self-healing-converted-sweeps-have-no-literal-lane-guards.test.ts` **derives** its sweep list (a sweep counts as converted when its body calls `resolveProjectColumnsForRoles`), so it cannot go stale, and it carries two positive controls because a broken regex finds no offenders and a broken derivation iterates nothing — an empty loop registers no tests and reads green. ## Deliberately unchanged - 22 `moveTask` destinations carrying `recoveryRehome: true` — `moves.ts` exempts these so a card stranded in an undeclared column stays rescuable. - One literal in `clearStaleBlockedBy`'s log-dedup closure (allowed by name in the ratchet, with the reason). - `surfaceInReviewStalls` — hot list-read path, needs a batched prefetch; that is a performance design decision, not a conversion. - `scheduler.ts` and `replan-target.ts` — built on `resolveTaskWorkflowIrSync`, which returns the default IR for every task in production. Converting there produces inert code. ## The fold itself is worth one note All 23 branches appended to the **same test file at the same anchor**, so every automatic strategy — git 3-way, `merge-file --union`, and three hand-written resolvers — interleaved them mid-block. Two attempts committed conflict markers before I caught it. The file is therefore **reconstructed**: head authored once, body assembled as the union of each branch's own intact top-level segments keyed by test title, with the nested `already-merged hard blocker` describe appended whole (flattening it orphaned its helper). Verified by *parsing after every step* rather than trusting the merge — which is how each interleaving was caught. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71. Scoped suites 592 passed (self-healing, the blindness suite at 68 cases, the ratchet, and the notification suite). `tsc` engine clean; `pnpm lint`, `check:changesets`, `lifecycle-column-census --strict` and `check-sql-column-literals` all clean, each run explicitly. Each folded conversion was individually revert-proven on its original branch — the read reverted alone, and the per-card verdict reverted alone — and those measurements are recorded in the commit messages carried into this branch. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
c6767cb258 |
self-healing: foreign-only contamination never cleared on a renamed board (fourteenth sweep) (#2891)
`recoverForeignOnlyContaminatedInReviewTasks` classifies a branch carrying **only foreign commits** and clears the contamination park that nothing else clears. Two literal reads meant that on a renamed board it classified nothing, and the task stayed parked indefinitely. ## The two redundant guards were the interesting part Both filters carried a `task.column === …` check that was **redundant** while the query pinned the column. Under a resolved read they stop being redundant and become the per-card verdict — so they convert here rather than being deleted. Deleting them would have silently widened the sweep, which is the failure this whole class is about. ## Dedupe matters more here than elsewhere The concatenated candidate list is deduped (the P1 reviewed on #2879). It bites harder in this sweep because the two filters have **different predicates**: a column carrying both a review role and the wip role could satisfy both and classify one branch twice. Explicit `has` guard rather than `new Map(entries)` — that constructor keeps first insertion *order* but the **last** value for a repeated key, so it reads as first-bucket precedence while doing the opposite. (Corrected in #2879 and #2883 for the same reason.) ## Revert results Each applied alone and the file re-run: | conversion | reverted → | | --- | --- | | the resolved reads | fails — the card is never listed, so the classifier is never called | | the review verdict | fails — the renamed review lane does not match and the card is filtered out | Observable is **candidacy**: `classifyForeignOnlyContamination` runs once per accepted card and not at all for a rejected one, which is exactly the read-plus-verdict under test. It is a static named import, so it is intercepted with a scoped `vi.mock` (spyOn cannot rebind an already-resolved ESM binding); only that one export is overridden, so the sweeps in this file that use `inspectBranchConflict` are unaffected. A non-vacuous companion (same card in the board's hold lane → never classified) rules out a read that returns everything. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71, plus `self-healing.test.ts` 412; `tsc` engine clean; `pnpm lint`, `check:changesets`, census `--strict` clean, each run explicitly. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
f6e368205e |
self-healing: a card stuck mid-merge could not be retried on a renamed board (twenty-first sweep) (#2912)
`recoverStaleMergingStatus` clears a `merging`/`merging-pr` stamp left on a review card with no live merger behind it. The literal read meant that on a renamed board the stamp was never cleared. **The operator's escape hatch was closed by the same bug that caused the stall.** That stamp is consulted by the merger *and* by the dashboard's manual Retry gate, so the card could neither progress on its own nor be retried by hand. ## The redundant guard converts `task.column !== "in-review"` was redundant while the query pinned the column; under a resolved read it becomes the per-card verdict. Carries the #2891 shape — **narrow when the card can answer, broad when it cannot**. ## Fixture note worth keeping `updatedAt` in the test is deliberately ancient. `isStaleMergeActiveStatus` requires the stamp to have sat untouched for `minAgeMs`, so a fresh fixture would be filtered out for a reason that has nothing to do with lanes — and would then have passed with the fix reverted. That is the shape of most of the vacuous assertions on this branch: a *later* filter rejecting the card, masking whether the lane logic worked at all. ## Revert results Each applied alone and the file re-run: | conversion | reverted → | | --- | --- | | the resolved read | fails — the card is never listed | | the per-card verdict | fails — the renamed review lane is filtered out | A non-vacuous companion (same stamp on a wip card → untouched) rules out a read that returns everything; a merge stamp in the wip lane belongs to `recoverInProgressLimbo` and the executor, not here. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71, plus `self-healing.test.ts` 412; `tsc` engine clean; `pnpm lint`, `check:changesets`, census `--strict` and `check-sql-column-literals` clean, each run explicitly. |
||
|
gsxdsm
|
aacd18e847 |
docs(lanes): audit the last three files the census points at with no reason attached (#2908)
Second pass of #2873's sweep, over the files that still carry lifecycle guards and **zero** audit notes. No source change — every literal stays counted, none gets an exemption marker. ## `project-store-ops.ts` (1) — dead sync path, do **not** convert The literal would leak a merge-queue entry on a renamed board: a card leaving review would never be dequeued. Except the function cannot run — it reaches for `store.db.prepare`, which throws in PostgreSQL backend mode. The live path is `dequeueMergeQueueOnColumnExitInTransaction` (`async-merge-coordination.ts`, called from `moves.ts`), and it is **already converted** — it takes `moveReviewColumns` and the caller supplies them. Recorded so the census entry is not mistaken for unconverted debt, and so it can be deleted alongside the rest of the sync SQLite residue. ## `task-id-integrity.ts` (2) — one real, one sentinel, and the real one must not go alone ```ts return cached?.column === "archived"; // ← board lane: real if (live === "archived") return true; // ← getLiveTaskColumn's manufactured value: sentinel ``` Converting the first while `getLiveTaskColumn` still keys on the literal would leave the two disagreeing about what "archived" means. It waits for that one, which is the single highest-leverage line in this cluster — fixing it makes five downstream sentinel checks correct without touching any of them. ## `auto-merge-finalization.ts` (3) — one real but diagnostic-only, two non-defects `task.column === "done"` selects which **reason string** is reported; both arms return `{ ok: false }`. So a renamed board is refused with the generic `missing-merge-confirmation` instead of the specific `done-without-merge-confirmation`. Real, and worth less than the signature change required to fix it — the resolver two functions up already computes `isCompleteColumn`, but this function does not receive it. The other two are **not** defects and it is worth saying so explicitly: the `columnId === "done"` near the top is the resolver's documented degraded fallback (the live arm calls `columnHasFlag`), and the `step.status` comparison is a **step status**, not a column. ## The pattern across both passes Of **6 files and 15 guards** audited: **2** were live defects worth converting, **4** were sentinels or dead paths that would have *broken* a renamed board if converted, and the rest were diagnostics or misfiled step statuses. That ratio is the argument for these notes existing. A file's census count is an upper bound on convertible sites, not a work estimate — and in this cluster the naive reading of the number would have made things worse more often than better. ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/core`, `@fusion/engine`) — clean - census `--strict` — exit 0, counts unchanged (that is the point) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
8e505dc4e1 |
docs(recovery): the reason this parameter is optional stopped being true (#2903)
Comment-only. No source change, no behaviour change.
The note on `isInReviewMissingWorktreeSessionStartFailure` said:
> Optional rather than required because the other caller
(`extension.ts`) still asks BOTH questions with the literal.
**It doesn't.** All three production callers pass the resolved answer:
```
packages/cli/src/extension.ts:1927 retryReviewColumns.has(task.column)
packages/cli/src/commands/task.ts:1390 retryReviewColumns.has(task.column)
packages/dashboard/src/routes/register-task-workflow-routes.ts:2885 retryReviewColumns.has(task.column)
```
Left standing, that sentence tells the next reader an unconverted caller
exists — and "we keep the fallback because someone still needs it" is
exactly the justification that keeps an inert-conversion shape alive. It
is the specific failure this program has spent the day removing, in the
form of a comment rather than code.
## The parameter stays optional, for a reason that does not rot
I checked whether to make it **required** — the unwired-lane-parameter
guard's own failure message suggests exactly that ("make the parameter
required so the compiler finds the call sites") — and decided against
it, for measured reasons:
- **25 test call sites** use the optional form, several of them
*precisely* to pin the degraded mode (`cli-active-count-lanes.test.ts`
exercises the no-argument path on both a legacy and a renamed lane).
Requiring the parameter deletes that coverage.
- The enforcement it would buy already exists: `isReviewColumn` is in
the guard's vocabulary, so if any of those three callers stops passing
it, the build fails.
So the note now gives the durable reason instead of the expired one.
## How this surfaced
Not from the census — the count here is unchanged, and an *omitted
argument* is invisible to it anyway. It came from reading an audit note
that named a specific caller and checking the claim. Notes that assert
facts about other files decay silently; this one had.
## Verification
- `pnpm lint` — clean
- `tsc --noEmit` (`@fusion/engine`) — clean
- `restart-recovery-coordinator.test.ts` — 12 passed
- `cli-active-count-lanes.test.ts` — 10 passed
- unwired-lane guard — 9/9, no new entries
- SQL-literal gate — green
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
10f9df1600 |
fix(overseer): the whole oversight loop was inert on a renamed board (#2898)
`resolveWatchedStage` keyed on the literals `in-progress`/`in-review`, so on a board that renames either it returned `null` for **every** card. That is three literals with an outsized blast radius. `observeTask` returns early on a null stage, so: - no `OverseerStageObservation` is recorded, - no `overseer:intervention` entry is emitted, - and `PlannerRecoveryController`, which consumes those observations, has nothing to steer, retry or targeted-fix. **The entire oversight loop was inert and silent about it** — the same shape as the self-healing sweeps whose queries returned empty arrays. ## I deferred this myself, on a cost argument that was wrong The audit note I wrote for this site said resolving inside `observeTask` "buys a workflow read per card per poll". Then I read the caller: the poll **already awaits `resolveEffectiveSettings` per task**. It is a per-task async loop regardless, so with an IR cache keyed by workflow the addition is *(distinct workflows)* resolutions, not *(cards)*. Pricing the fix before checking the caller cost a deferral. Worth recording, because "this needs a cost judgement" is the most comfortable place in this program to leave something. ## The review test is the three-trait union, deliberately `isReviewColumnRole` checks only `mergeBlocker || humanReview`. A board whose review lane carries `merge` (**mergeOrchestration**) — the built-in default's own shape — would classify as *not in review* and be skipped. Reaching for the obvious helper would have reintroduced the bug this change removes, through the helper meant to fix it. There is a case asserting exactly that. ## Wiring Both call sites, because either alone leaves a hole: | site | why it matters | |---|---| | the poll (`project-engine.ts`) | per-poll IR cache — a workflow edit is picked up next tick rather than served stale | | the manual nudge | otherwise a renamed board answers `no-active-stage` to an operator pressing the button | `columnFlags` is in the `unwired-lane-parameter` vocabulary, so the wiring cannot silently rot — the guard reports it if a future change drops the argument. Fail-soft throughout: an unresolvable workflow yields `undefined` and the callee falls back to the legacy ids, which is exactly today's behaviour. A v1 IR declares no columns, so it takes the same path. ## Revert proof (measured) Drop the `columnFlags` branch and **exactly the three renamed-lane cases fail**: ``` expected null to be "executor" expected null to be "merger" (mergeOrchestration lane) expected null to be "merger" (humanReview lane) ``` The legacy-id and neither-role cases stay green — the gate must still gate, and watching every column would be its own defect. ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/engine`) — clean - `planner-overseer.test.ts` + `planner-recovery-controller-human-control.test.ts` — 64 passed - unwired-lane guard — 9/9, no new entries Carries the one-line SQL-baseline re-record (`team-analytics.ts: 6 → 3`) that #2864 left behind, same as my other open branches — main is red on it, and identical changes to that line merge without conflict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
9366bc8382 |
fix(workflow): the review handoff killed the walk on a renamed review lane (#2900)
The sharpest lane defect left in the backlog, and the one I have been
deferring since the first sweep.
```ts
if (seam === "review-handoff") {
const result = await primitives.transitionTask(primitiveCtx, context.task, {
column: "in-review", // ← post-U12 this is a rejected destination on a renamed board
```
Post-U12 `moveTask` **rejects** a destination the workflow does not
declare. So on any board with a renamed review lane, the handoff threw
`TransitionRejectionError` and **killed the workflow walk mid-run**. Not
a silent wrong answer for once — a hard failure in the middle of a task,
which is why it outranked everything else once it became reachable.
**Why it was deferred:** every fix threads a resolver out of
`executor.ts`, and #2820 was editing that file. It merged at 22:08, so
this was finally free of the conflict.
## The role travels, not the column
Seam handlers in `workflow-node-handlers.ts` are pure functions over an
IR node and a task — no store, no task id to resolve from — so a handler
can only ever name a literal. The runtime primitive in `executor.ts`
**does** hold the store, so the seam now asks for `columnRole: "review"`
and the primitive resolves it against the task's **own** selection.
One authority, deliberately. Answering one question with two reads is
what took #2843 five review rounds, and I would rather not relearn it
here.
Compatibility is preserved in both directions:
- `column` still wins when both are supplied — an explicit destination
is an explicit destination;
- an unresolvable role falls back to the legacy `in-review` rather than
failing the transition, which is exactly the behaviour every caller had
before.
## The test asserts the literal is *gone*, not merely accompanied
`column` takes precedence over `columnRole` downstream, so a diff that
added the role while leaving the literal would look converted and be
completely inert. That is the exact shape this program keeps finding — a
documented fallback in front of a literal that still decides everything
— so the assertion is:
```ts
expect(input.columnRole).toBe("review");
expect(input.column).toBeUndefined(); // ← the half that matters
```
**Revert proof, measured:** restore `column: "in-review"` in the seam
and it fails with `expected undefined to be 'review'`.
## Verification
- `pnpm test:gate` — 161 / 487 / 13 / 71 passed
- `pnpm lint` — clean
- `tsc --noEmit` (`@fusion/engine`) — clean
- new `review-handoff-lane.test.ts` plus the two neighbouring seam
suites — 41 passed
Carries the one-line SQL-baseline re-record (`team-analytics.ts: 6 → 3`)
that #2864 left behind, same as my other open branches — main is red on
it, and identical changes to that line merge without conflict.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
a453912ddf |
self-healing: merged-but-unfinished tasks never finalized on a renamed board (fifteenth sweep) (#2897)
`recoverMergedReviewTasks` finalizes a task whose merge is **confirmed** but which never reached the complete lane. Two literal reads meant that on a renamed board it was never found, so a card whose commit is already on the base branch sat in review or hold indefinitely — merged work the board still shows as unfinished. ## The two redundant guards convert, they don't get deleted Both `t.column === …` checks were redundant while the query pinned the column. Under a resolved read they become the per-card verdict. Deleting them would have silently widened the sweep — the same trap called out in #2891. ## Carries the two shapes review established earlier in this series - **Narrow when the card can answer, broad when it cannot** (#2891). `resolveWorkflowIrForTask` *substitutes* the built-in IR rather than failing, so a card with an unreadable selection would otherwise be rejected by the very verdict that the project-scoped query had just admitted it under. It falls back to the project sets instead. - **Deduped across the buckets** (#2879), so a column carrying both a review role and the hold role cannot finalize one card twice. Both were review findings on earlier PRs in this series, applied here up front rather than waiting to be caught again. ## Revert results Each applied alone and the file re-run: | conversion | reverted → | | --- | --- | | the resolved reads | fails — the card is never listed | | the per-card review verdict | fails — the renamed review lane does not match | Observable is `resolveSelfHealingMergeTarget`, a private method called once per candidate, so the assertion sits downstream of both halves without a git fixture. A non-vacuous companion (merge-confirmed card in the wip lane → untouched) rules out a read that returns everything. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71, plus `self-healing.test.ts` 412; `tsc` engine clean; `pnpm lint`, `check:changesets`, census `--strict` clean, each run explicitly. |
||
|
gsxdsm
|
cfb713bda1 |
notification: record the measured reason four wedge-progress ids stay literal, and un-red main's gate (#2882)
Two small things, neither of which changes behaviour. ## 1. A conversion I attempted, measured, and reverted `hasProgressed` in the wedge-episode path names four column ids outright. I converted them to a resolved lane set. It **broke an existing gate test** — `task-wedge-notification.test.ts` → *"sends one actionable push and mailbox message per active terminal episode"*: 1 message delivered, 2 expected. The note already in that file was right, and stronger than it read. The hazard is **not** specific to the resolve/claim ordering — it is **any `await` added before the resolve**. Column resolution needs one. `task:updated` listeners fire synchronously, so a re-wedge arriving close behind a recovery reaches `claim` while the first episode is still open, and the operator's second alert is dropped. Product change reverted; only the comment lands, now carrying the measurement and naming the failing test as the acceptance check for whoever owns the wedge-episode contract. **Left counted, not exempted** — the census should keep pointing here. Worth stating: the pre-existing note was a warning written speculatively. Attempting the conversion is what turned it into evidence, and the evidence says the blocker is real but sits somewhere else (per-task serialisation) than the note implied. ## 2. `main`'s gate is red, and not from this branch `pnpm test:gate` fails on a clean `origin/main` tree at `check-sql-column-literals`: ``` packages/core/src/team-analytics.ts: 3 site(s) now, baseline still allows 6 — re-record it ``` A reduction landed without re-recording the baseline in the same commit, which that check explicitly asks for. Reproduced on `origin/main` with my changes stashed, so it is not mine — but it blocks **every** open PR until recorded. Ratchets **31 → 28** sites across 14 files, downward only. ## The vacuous assertion this round (sixth) The first version of the reverted test passed **with the fix reverted**. `hasProgressed` is a three-clause OR, and the middle clause — *status is a string and is not `failed`* — is true for a recovered task on any board, so `status: "in-progress"` in the fixture satisfied it regardless of column. Same shape as the other five: something the code does anyway. Found by running the revert, not by reading it. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71 (green only with the baseline commit); `tsc` engine clean; notification suite 77 passed; `pnpm lint` and census `--strict` clean. |
||
|
gsxdsm
|
ab15e5f9f7 |
docs(lanes): audit the three files the census points at with no reason attached (#2873)
**No source change.** Every literal stays counted and none gets an exemption marker. What changes is that the census now points at these three with the analysis attached, instead of making each worker who reaches them re-derive it. Peers have already done this well for `notification-service.ts` — converted it, *measured* a real delivery regression, reverted, and left it counted with the reason. These three had nothing at all, and one of them is the highest-impact unowned site I found. ## `planner-overseer.ts` (3 guards) — REAL, and larger than three literals suggest On a renamed board `resolveWatchedStage` returns `null` for every card. `observeTask` returns early on a null stage, so **no observation is recorded**, no `overseer:intervention` entry is emitted, and `PlannerRecoveryController` — which consumes those observations — has nothing to steer, retry, or targeted-fix. **The entire oversight loop is inert and silent about it**, exactly like the self-healing sweeps whose queries returned empty arrays. Not mechanical, which is why it is flagged rather than converted. `resolveWatchedStage` is a pure sync function over a `Partial<OverseerTaskRef>` with no store and no task id, so the lane answer has to arrive as a parameter. Its only production caller, `observeTask`, *is* async and the monitor *does* hold a store — but it runs **once per task per poll**, so resolving inside it buys a workflow read per card on a timer. The shape that works is the one the board-load enrichment landed on in #2845: resolve at the **poll**, once, with an IR cache keyed by workflow, and pass the flags down. That makes it a change to `project-engine.ts`'s poll as much as to this file — a cost judgement about a periodic engine loop, not a rename. `columnFlags` is in the unwired-lane-parameter vocabulary, so whoever adds the parameter cannot leave it unwired. ## `async-mission-store-queries.ts` (1 of 3) — REAL `getTerminalTaskEvidence` tests only `column === "done"` for its `done` verdict, so a completed card on a renamed board falls through every branch to `{ kind: "nonterminal" }`. The caller is mission **terminal evidence repair**, so a finished feature reads as unfinished — a wrong *verdict*, not an error. The `archived` test beside it has the same defect, masked for soft-deleted rows by its `deletedAt` companion, which is why only the `done` half bites in practice. Takes a bare `QueryHandle`: no store, no task object, no workflow. The fix is a resolved terminal-lane set threaded in by the caller — the same shape `getLiveTaskColumn` needs, and it should land *with* it so the two cannot disagree about what "finished" means. ## `audit-ops.ts` (2) — one sentinel, one real, and they look identical ```ts if (state === "archived") // ← getLiveTaskColumn's MANUFACTURED value: do NOT convert if (pgRow.column === "archived") // ← a real board lane: convertible ``` The first compares against a string `getLiveTaskColumn` *fabricates* for an archived-or-soft-deleted parent, so converting it to `isArchivedColumnRole` would keep passing on the built-in board and start **failing** on a renamed one — a soft-deleted task's log would become writable. The second reads the task row, so a renamed archived column keeps accepting log writes; its `deletedAt` companion masks that in practice. Two lines that look the same and need opposite treatment is precisely the reason these notes are worth more than the count. ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/core`, `@fusion/engine`) — clean - `planner-overseer.test.ts` — 50 passed - census `--strict` — exit 0, **counts unchanged** (that is the point) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
5cb731420c |
test(engine): re-point the stale-spec ratchet at the improved guard (#2863)
## Main is red; this fixes it
`executor-stale-spec-active-lanes.test.ts` — 2 failures on `main`:
```
× resolves the task's lifecycle columns before deciding the skip
→ expected source to contain 'const activeLifecycle = resolveLifecy…'
× adds the wip, review and complete lanes to the active set
```
**Nothing regressed — the guard got better and the ratchet didn't
follow.**
## What changed in the product
The stale-spec skip used to build its active set from
`resolveLifecycleColumns(...)`, taking `?.wip`, `?.review` and
`?.complete`. That returns the **first** column carrying each trait, so
a board with two wip lanes — or a review lane plus a second
merge-blocking one — had only one of each recognised as active. A card
in the other read as **inactive**, and its prompt file was treated as
reclaimable.
It now resolves the IR once and unions `columnsWithFlag` over five
flags, which returns **every** column carrying each:
```ts
const activeIr = await resolveWorkflowIrForTask(this.store, task.id);
const activeColumns = new Set<string>(["in-progress", "in-review", "done"]);
if (activeIr) {
for (const flag of ["countsTowardWip", "mergeOrchestration", "mergeBlocker", "humanReview", "complete"] as const) {
for (const lane of columnsWithFlag(activeIr, flag)) activeColumns.add(lane);
```
That is a real fix to an arity bug, and the legacy trio stays unioned in
for the documented reason: under-reporting active is the destructive
direction.
## Why re-point rather than loosen
This is a **source ratchet** in the `engine-no-blocking-shellout` style,
and its entire value is that it fails on a revert. A substring loose
enough to match both the old and new shapes would keep the file green
through exactly the regression it exists to catch.
So the assertions now name the new shape precisely, including the **flag
list**, so dropping one of the five is caught here too.
**Mutation-verified:** replacing the IR resolution with `undefined`
fails the ratchet. It still does its job.
## Scope
The file's own header notes this is *not* a behavioural proof — the
guard sits inside `execute()` behind worktree and session setup a unit
test has no business standing up, and it asks whoever next touches that
scaffolding to add the end-to-end case. Re-pointing is maintenance; I
have not taken on that harness here, and the note still stands.
## Verification
- `executor-stale-spec-active-lanes.test.ts` — **4/4**, fails on revert
- `pnpm lint` — clean
Test-only; no changeset. Found by running the full `engine-default`
project after #2855 merged — the remaining `main` red is my own audit
case, fixed by the pending #2857.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
b9785ec10f |
fix(tests): stop the census guard reddening main on every legitimate conversion (#2856)
## This is the cause of four main reds today, not a fifth instance of them I have now fixed the census baseline on `main` three times (#2814, plus a withdrawn branch, plus watching #2811 and #2844 do the same). Rather than do it a fourth time, here is why it keeps happening. `census-baseline-corruption-guard` asserted: ```ts expect(result).toContain("every file matches its baseline exactly"); ``` That demands the **committed baseline be byte-in-step with the tree at all times**. **It is not, by design.** A conversion PR that removes guards leaves the tree holding *fewer* than the baseline allows, and the CLI treats that as the good case — it tightens the pin and exits 0. Measured directly: ``` simulated drop → EXIT ON DROP: 0 "The baseline file has been rewritten downward. COMMIT IT … in CI this write is discarded with the runner, which is why the gate is green and not silent." ``` So the ratchet was already happy while this test went red. Every conversion that did not *also* re-record the baseline turned `main` red for a condition that was never a defect. That is the mechanism behind **#2783's markers, #2837's query split and two more** — plus three collisions between workers racing to re-record the same file (#2811/#2814, #2844, and a branch of mine I deleted rather than open as a duplicate). ## The fix matches the guard's own stated intent Its comment says: *"a guard that always fails is no guard"* — its job is to prove the **corruption** diagnosis does not false-positive on a healthy file. **A tightened baseline is healthy.** So it now asserts what that needs: - the run **succeeds** — `execFileSync` throws on a non-zero exit, so a **rise still fails before any assertion runs**; a rise is real debt and must stay loud - the corruption diagnosis is **absent** - the outcome is one of the two healthy shapes the CLI can report ## Measured discrimination — all four cases | scenario | before | after | |---|---|---| | **drop** (legitimate conversion) | ❌ 1 failed — *the false red* | ✅ 3 passed | | **rise** (real debt) | ❌ fails | ❌ **still fails** | | **corrupt JSON** | ❌ fails | ❌ still fails (case unchanged) | | **unreadable file** | ❌ fails | ❌ still fails (case unchanged) | Only *"somebody converted guards and has not re-recorded the pin yet"* stops being a red. ## Scope Engine **11022 passed / 0 failed** · gate **732 green** · lint clean. Test-only. **Does not change** the CLI, the ratchet, or what `--strict` reports. Re-recording the baseline on a drop is still the right thing to do — it just stops being an emergency that reddens main and blocks everyone else while three people race to fix it. The baseline file is restored byte-clean after every simulation above (`git diff` verified). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Expanded baseline validation coverage to detect unreadable or invalid baseline data. - Added support for both exact baseline matches and successfully tightened baselines. - Improved health checks for baseline verification outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
9d103fb171 |
test(engine): record the partial planning-continuation conversion the audit caught (#2857)
## The audit fired, as designed `workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts` went red on `main`: ``` × AUDIT — one of the two classifier call sites is converted, and the inner predicate is not → expected 2 to be 1 ``` That is the alarm working. #2799 built this case to fail **in both directions** — a new unconverted caller *or* a conversion of an existing one — precisely so a change here cannot land silently. Someone converted the inner half: ```ts if (!isPlanningContinuationTaskDispatchable(task, terminal)) { ``` ## What actually changed, and what did not | site | before | now | |---|---|---| | `drainDuePlanningContinuations` | passes `{ terminalColumns }` | unchanged ✅ | | `resolvePlanningContinuationCandidate` → inner predicate | **not threaded** | **threaded** ✅ | | `selectActionablePlanningContinuations` | passes nothing | **still passes nothing** ❌ | **The behavioural case is untouched and still passes.** A card in a renamed COMPLETE lane still comes back `actionable` and still re-enters plan-review, because `selectActionablePlanningContinuations` is the site that decides that. That combination is the useful shape of a **partial conversion**: the code reads more converted than before, and the operator-visible defect is exactly where it was. Without the behavioural case sitting next to the audit, the arity change would have looked like the fix. The fixer's own comment agrees, and names the narrow case the inner half reaches — a board declaring `done` as a *non-terminal* column id, where the outer check passes and the inner one skipped the continuation as "paused". Real, and not the case the characterization covers. ## The change Arity assertion updated `1` → `2`, with the reasoning recorded at the assertion and in the header. **Updated deliberately, not loosened** — arity is still the property asserted, so the next change to either site lands here again. Nothing else moved. ## Verification - suite — **4/4** - full live-PG E2E surface — **171/171** - `pnpm lint` — clean Test-only; no changeset. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
7784cb1fe8 |
self-healing: six recovery sweeps that never ran on a renamed board — and the guards widening their queries activates (#2838)
**Six self-healing sweeps did not run at all on a renamed board. Each is a recovery path — the thing that unsticks a card when something has already gone wrong.** #2800 measured this class and could not fix it: a read happens *before* any task is in hand, so there is nothing to resolve a per-task lane from. `resolveProjectColumnsForRoles` (landed separately) is the seam that was missing. ## What was silently dead | sweep | what stayed broken on a renamed board | | --- | --- | | `reconcileDoneTaskIntegrity` | a landed card kept **no commit sha**, forever | | `recoverAlreadyMergedReviewTasks` | a card whose merge **succeeded** stayed parked with `status: "failed"` | | `recoverStuckMergeDeadlocks` | **doubly blind** — no candidates *and* no dependents | | `recoverInterruptedMergingTasks` | a task interrupted mid-merge sat in `merging` indefinitely | | `recoverMergeableReviewTasks` | a card ready to merge was never re-enqueued | | `recoverReviewTasksWithFailedPreMergeSteps` | a card parked on a failed review step was never revived | The census scored the `task.column === "..."` re-assertion *inside* each loop, never the query above it. Converting those comparisons would have dropped six counts and changed nothing — the loop bodies were already unreachable. ## The conversion shape — five parts, three of which review taught me Documented in `self-healing-sweeps-are-blind-on-a-renamed-board.md`, because the second sweep **drifted from the first**: I wrote it from the pre-review version and reproduced a flaw already fixed one commit earlier. 1. **Read** — project union, query each column, dedupe by id. Legacy ids unioned so a board mid-rename is not skipped. 2. **Verdict** — per card against **its own** workflow. Widening the read and widening the verdict are different decisions: *a missed row is invisible, a wrong row is a write.* Using the project union as a per-card test claims a card because some **other** board calls its column that role. 3. **Provenance** — the resolver **substitutes** the built-in IR rather than failing, so `length > 0` reads as "this card answered" when nobody did. It does not change the verdict (measured: identical) — it makes the unrepaired card **reportable**. 4. **The log strings** — widening a query invalidates every message naming the old literal. One logged `"stale merging task(s) in in-review"` after its read covered several lanes. 5. **The guards the query ACTIVATES.** ## Part 5 is the one that bites A guard downstream of a literal query is **unreachable** on a renamed board — and unreachable is indistinguishable from correct. That is why these sit unwired indefinitely. `recoverReviewTasksWithFailedPreMergeSteps` filters on `blocker !== "task has failed pre-merge workflow steps"` — an **exact string match**. Unwired, the blocker returns `"task is in 'checking', must be in 'in-review'"`, so widening the query alone would have made the sweep **find every card and reject every card**. Measured: **6 sweeps hold both a literal query and an unwired lane guard**; 30 hold a literal query with no such guard. All six are named in the doc. **One of the six was my own already-converted sweep.** I widened `recoverAlreadyMergedReviewTasks` two commits before noticing its `getTaskHardMergeBlocker` was unwired — so for two commits it found renamed-board cards and declined them. The scan must run **before** widening; I did it after, and only caught it because the next sweep forced the question. `getTaskHardMergeBlocker` was the blind spot for four of the six: a wrapper, no lane parameter at all, every caller behind a literal query. ## Corrections to my own work, kept visible - The project union used as a **per-card verdict** — the flat-set mistake `project-lane-vocabulary.ts` warns about in its own header, which I quoted while writing it. - A **provenance fix that was a no-op**: measured identical verdicts in every state, revert passed its own new test, so it was thrown away rather than shipped with a comment claiming otherwise. - The second sweep **reproducing the first's pre-fix shape**. - Three assertions that were **vacuous until the revert exposed them** — including one where the write needed a real git repo, so `commitSha` could not distinguish accepted from rejected. ## Verification - `pnpm test:gate` — 161 + 487 + 13 + 71 - `self-healing.test.ts` 412, query-blindness suite 12 - `tsc` on core and engine; `pnpm lint`; `check:changesets`; census `--strict` — all clean, each run explicitly - Every conversion revert-measured, **each direction independently** where a sweep has two (read and guard) ## Scope **42 queries remain**, 5 of the 6 activation-risk sweeps among them. Each is per-sweep work — its own filter semantics, its own downstream guards, its own log strings — so they land one at a time with the pattern proven, never swept. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Self-healing workflows now work correctly on boards with renamed lifecycle columns. - Improved recovery for completed, in-review, interrupted, stalled, and failed-merge tasks. - Prevented tasks from being incorrectly classified using another workflow’s columns. - Added warnings when a task’s workflow lanes cannot be resolved. - **Documentation** - Expanded guidance on renamed-board recovery behavior and related diagnostic limitations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
68fd781eb8 |
test(engine): stop engine-default exiting 1 with zero failing tests (#2855)
## The problem
On `main`, the whole `engine-default` project exits **1 while reporting
736 files passed and 0 failed**:
```
Test Files 736 passed | 1 skipped (737)
Tests 9944 passed | 14 skipped | 1 todo (9959)
Errors 9 errors
```
Nine unhandled rejections — `TypeError: this.store.listTasks is not a
function`, all from `self-healing-db-corruption.test.ts`, raised *after*
its tests had passed.
That is worse than a plain failure: the lane is red, **nothing names a
test**, and a genuine regression landing later arrives in an already-red
lane where it reads as more of the same noise.
## Why the existing isolation didn't hold
The suite stubs every other maintenance sweep so only the corruption
path runs. But `runMaintenance` invokes the surfacing family like this:
```ts
{ name: "surface-in-review-stalled", fn: () => this.surfaceInReviewStalled(maintenanceSurfacing()) },
```
`maintenanceSurfacing()` lazily calls `openSurfacingCycle()`, which does
`await this.store.listTasks({ slim: false })`. **Spying on
`surfaceInReviewStalled` replaces the method, but the call site still
evaluates its argument** — so the shared cycle opened regardless,
against a mock store that deliberately implements only what corruption
surfacing needs.
## Two fixes that look right and are not
Both tried, both reverted — recorded because each is the obvious next
move:
| attempt | result |
|---|---|
| add `listTasks` to the mock | **3 tests fail** — the sweeps then run
far enough to record audit events the assertions don't expect |
| stub the 27 maintenance methods missing from the suite's lists | **5
tests fail, and all 9 errors remain** — `openSurfacingCycle` isn't among
`runMaintenance`'s own calls, so the drift was a real but unrelated gap
|
The second result is what located the fault: stubbing everything
`runMaintenance` calls does not silence the rejections, so they don't
originate there. The stack confirmed it —
`SelfHealingManager.openSurfacingCycle` at `self-healing.ts:8015`. I
should have read that stack before guessing twice.
## The fix
Stub the cycle itself. `null` is exactly what `openSurfacingCycle`
returns when the engine is paused, so every consumer already handles it,
and the corruption-surfacing path this suite exists to test is
untouched.
## Verification
- `self-healing-db-corruption.test.ts` — **6/6, 0 errors, exit 0**
- full `engine-default` project — **736 files passed, 9944 tests, 0
errors, exit 0** (was exit 1)
- `pnpm lint` — clean
Test-only change; no production file is touched, so no changeset.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
92d82b7a17 |
fix(guard): the unwired-lane check reported 0 because its question was too weak (#2852)
A guard nobody has proven can fail is a number, not a check. This one
was returning a clean `[]` while **18** real unwired lane declarations
sat on `main` — including one it was specifically built to catch.
## The escape that found it
`diffSnapshots` in the glasses plugin:
```ts
opts: { notifyOnColumns: ReadonlySet<ColumnId>; completeColumnsByTaskId?: ReadonlyMap<...> }
...
const completeColumns = opts.completeColumnsByTaskId?.get(task.id);
const isComplete = completeColumns ? completeColumns.has(task.column) : task.column === "done";
```
**No file anywhere builds that map.** The conversion was decorative —
the literal decided every real poll, so on a renamed board the wearer is
notified of every column transition *except the card finishing*, the one
they care about. The name was already in the guard's vocabulary list,
the declaration was exported and optional; it satisfied every condition
the guard checks, and the guard said nothing.
## Three independent blind spots
| # | blind spot | why it mattered |
|---|---|---|
| 1 | `SCANNED_PACKAGES` omitted `plugins/` | plugins hold lane logic
like anything else — this one resolves workflow IRs and decides what
"finished" means |
| 2 | inline options-object types were not walked | only bare parameters
and *named* interfaces were. Whether a lane answer arrives as a
parameter, an interface property, or an inline field is a style choice —
**a check evadable by a style choice is decorative** |
| 3 | the mention rule was `source.includes(parameter)` **anywhere** |
satisfied by coincidence for any ordinarily-named parameter |
Fixing 1 or 2 alone would still have missed it: **measured on `main`,
the guard found 0 unwired across 1753 files, and 0 again across 2114
once `plugins` was added**, because the shape was invisible too.
### On (3), I proved it on myself
Renaming the unwired parameter from `completeColumnsByTaskId` to
`completeColumns` — a better name, chosen for good reasons — **silenced
the guard instantly**, because 15 unrelated production files declare a
local called `completeColumns`. The check had not been satisfied; it had
been switched off by a rename. That is exactly the failure the code
comment two lines up condemns, committed one edit later.
The fix is the cause, not a name blocklist: a file that never references
`diffSnapshots` cannot be the thing that wires `diffSnapshots`'s
options. Still deliberately loose — a co-occurrence test, not call-graph
analysis — which keeps the low false-positive rate that makes the guard
bearable while removing a false **negative** that scaled with how
ordinary a parameter's name was.
## The 17 this uncovered
Tightening (3) surfaced 17 further unwired declarations across core,
engine and dashboard. **Spot-checked, not assumed**:
`buildUnblockWeightMap` in `task-priority.ts` declares `terminalColumns`
and `reviewColumns`, and the only files that pass either are its own
tests — the production caller silently uses the built-in `{done,
archived}` default. That is the inert-conversion shape this module
exists to name.
They span three other batches, so they are recorded as a **ratcheted
baseline** in the shape `scripts/lifecycle-column-census.mjs` already
uses here — keyed on `file + parameter` so an unrelated edit above them
cannot manufacture a failure. A new one fails immediately; these can
only leave the list. Listing them beats pretending for another week that
they do not exist.
## The glasses fix
`completeColumnsByTaskId` -> a flat `completeColumns` set, matching its
sibling `notifyOnColumns` in the same options object, resolved **once
per poll** by `notifier.ts` via `resolveProjectColumnsForRoles`.
Project-scoped and not per task because this runs on a polling timer
over the whole board — a per-card workflow read would scale with the
board on every tick. Best-effort: a failed resolve leaves the diff on
its documented legacy default rather than dropping a poll.
Still gated by `alsoNotifyOnDone`, which the production caller passes as
`false`, so it remains unobservable at runtime. Wired anyway: the day
someone enables the flag the resolution must already be right — and now
the guard will say so if the wiring is removed.
## Revert proofs (measured, one per fix)
| revert | failure |
|---|---|
| unwire `notifier.ts` | baseline gains `plugins/…/diff.ts
completeColumns` |
| drop the inline-options walk | "covers an INLINE options-object type"
fails `expected [] to deeply equal [ 'completeColumns' ]`, and the repo
scan loses the glasses entry |
| drop the owner scoping | the repo scan loses **all 17** pre-existing
entries |
| restore `task.column === "done"` | both new `diff.test.ts` cases fail
|
The new diff cases assert **both** directions — a card in the resolved
lane fires, and a card in the legacy `done` does *not* once the caller
resolved other lanes. The second is what proves the resolved set
replaces the default rather than being unioned with it.
## Verification
- `pnpm test:gate` — 161 / 487 / 13 / 71 passed
- `pnpm lint` — clean
- `tsc --noEmit` — clean
- guard suite — 9/9; full glasses plugin — 188 passed across 19 files
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
89aaf341d0 |
the unwired-seam audit: 9 defects the census cannot see, incl. a reviewed card that cannot merge (#2820)
**Nine operator-visible defects in a class the census cannot see, plus
the audit method that found them.**
The census scans for lifecycle-column **comparisons**. This PR is about
guards that have no literal to find: a helper takes an optional
*resolved* lane set, its own test passes it, the census entry is gone —
and the callers pass nothing. **A resolved seam nobody wired is
indistinguishable from no seam at all.**
## What was broken
| defect | operator sees |
| --- | --- |
| `getTaskMergeBlocker` unwired in `mergeTaskImpl` | `Cannot merge FN-1:
task is in 'checking', must be in 'in-review'` — **a reviewed card
cannot merge** |
| …and in the completion move | `Cannot move FN-1 to done: …` — **and
cannot complete** |
| `isParkedTaskColumn` unwired ×2 (`agent-heartbeat`) | a durable agent
keeps claiming a parked card; **Health Check renders it RUNNING** |
| `resolveLinkSyncColumnRoles` first-per-role | link hygiene skips a
**second hold lane** entirely |
| `executor` active-task predicate first-per-role | a card in a **second
wip lane reads as INACTIVE**; its prompt file becomes reclaimable |
| `isPlanningContinuationTaskDispatchable` partially threaded | a board
declaring `done` as *non-terminal* stalls its cards — **stalled by a
lane name** |
| `default-workflow-hooks:72`, `executor:2404` | resolved gate admits
the move, unresolved blocker refuses it |
## The recurring shape, which is sharper than "a caller forgot an
argument"
Four sites resolve the lane and then re-ask with the literal, **a few
lines apart in the same function**:
- `task-artifacts-ops` resolves `completeColumn`, then asks the blocker
with the literal.
- `default-workflow-hooks:72` gates on `lifecycleColumns?.review`, then
the literal.
- `executor:2404` compares `resolveResumeLanes(…).review`, then the
literal.
- `resolvePlanningContinuationCandidate` applies the caller's terminal
set, then delegates without it.
**Grep for the helper, not the literal.** The literal is one function
away, correctly annotated as a fallback — which is exactly why the
census is blind to all of it.
## The arity trap, named and measured (six occurrences, one caught by
review here)
`resolveLifecycleColumns` answers *"which column is **the** hold
lane?"*. A `.includes()`/`.has()` test asks *"is this **any** hold
lane?"*. Nothing distinguishes them — same types, no literal.
**A default-vs-renamed differential cannot catch it**, because the
default board declares one column per role and therefore cannot express
the failing shape. It needs a *structurally* different fixture. That is
a sharper rule than "test both vocabularies", and it would have caught
all six.
Scanned: 12 candidate sites. **4 fixed · 3 blocked (2 on the inert sync
IR reader; `triage:833` also query-shaped) · 1 needs a hook-contract
change · 3 not defects (a returned tuple; an ordering-sensitive
precedence list) · 1 false positive of my own scan.**
A sweep over all twelve would have broken the ordering-sensitive pair,
delivered nothing at the sync-blocked ones, and "fixed" a site that was
already correct.
## Two traps in fixing this class — I hit both here
1. **The legacy id is a FALLBACK, not a member.** Pre-seeding
`"in-review"` admits a board that *declares* `in-review` as its WIP
column — a card mid-implementation merges prematurely. A real resolved
answer must **replace** the default. (Caught by review; it is the same
unscoped-legacy-acceptance the glasses plugin's review caught earlier,
which I had read and reintroduced.)
2. **Two guards, one assertion.** `toContain("must be in")` passed with
`mergeTaskImpl` reverted, because the *completion* guard caught the card
instead. The assertion now names the site (`Cannot merge` vs `Cannot
move … to done`) so the two fail independently.
## Corrections I made to my own work, recorded rather than quietly fixed
- My first PG test was **vacuous three ways**:
`saveWorkflowDefinition?.()`/`setTaskWorkflowSelection?.()` do not exist
(the `?.` swallowed both, so the task kept the builtin workflow),
`updateTask({column})` does not move a card, and a two-node IR made
every setup move illegal. Premise is now **asserted**, not assumed.
- My doc claimed the audit was complete. It enumerated **helpers**, not
every **caller** — `getTaskMergeBlocker` alone has 13 call sites.
Corrected in place, with the still-unwired ones listed by file and line
and a note to distrust any "audit complete" claim including mine.
- A severity correction to another worker's E2E:
`selectActionablePlanningContinuations` has **no production caller**, so
its stated consequence is latent, not live.
## Verification
- `pnpm test:gate` — 161 + 487 + 13 + 71
- `tsc` on core and engine; `pnpm lint`; `check:changesets`; census
`--strict` — all clean, each run explicitly
- Every fix revert-measured; each has a non-vacuous companion. The
two-hold-lane and repurposed-`in-review` cases exist because the default
board cannot express those shapes.
## Deliberately not done, with reasons in
`resolved-seams-nobody-wired.md`
`isTaskReadyForMerge` (dead in production — wiring it would be the
anti-pattern itself); `getTaskHardMergeBlocker` (3 of 4 callers are
query-gated sweeps); `getInReviewStallReason` (needs a **batch
prefetch**, not a per-task resolve — its callers decorate every task on
every list read; the in-review stall badge is wrong on renamed boards
until then); `default-workflow-hooks` planning/live-work sets (needs
`DefaultWorkflowMoveContext` to carry the IR — a shared contract
change).
|
||
|
gsxdsm
|
cea9637dfc |
feat(census): split the query class into read-shaped and write — most of the remainder must not be converted (#2837)
Splits the census's query class into **read-shaped** (convertible) and
**write** (must not be converted), reported beside the existing total.
On current `main`:
```
QUERY filters (column: "<legacy>"): 63
of those: 48 read-shaped (convertible), 5 writes (do NOT convert), 10 other
```
## Why the single number misleads
`column:` sits in an options-shaped object for both a source query and a
write, so the existing definition-vs-query rule cannot separate them.
The result reads as "dead reads to convert" — and after #2818 landed,
**48 of the 63 are `self-healing.ts` and the rest are largely not
convertible at all.**
Converting a write in this class is **harmful, not merely pointless**.
`async-persistence.ts` soft-deletes with `.set({ column: "archived",
deletedAt, … })`, and `getLiveTaskColumn` returns `"archived"` as a
**sentinel** for any soft-deleted row — the write and the sentinel have
to agree. A sweep that "finished the query class" by converting all 63
would break live-column resolution for every deleted task.
That is the same shape #2808 flagged for `recoveryRehome` moves. **Two
of the census-invisible classes now have members that must not be
fixed**, and in both cases the count alone cannot tell you which.
## Reported, not ratcheted
`properties.query` and `queryByFile` are byte-identical, so the pinned
baseline does not move and no open PR's Lint changes. The split is one
extra line of output.
**Changing what a ratchet enforces is the owner's call; improving what
it says is not.** Same line I drew when making marker-only failures
legible without loosening them.
## Honest limit
Read-shaped is a better filter than the raw count and **still not a
verdict**. `auto-merge-finalization.ts:242` is classified read-shaped
and must NOT be converted — its own comment records that it is
`getTaskHardMergeBlocker`'s review-eligible sentinel, deliberately not
re-keyed. Nothing mechanical would catch that; only the comment beside
it does. The split narrows a haystack to a readable list; it does not
decide the list.
## Verification
4 new cases — a `listTasks` filter counts read, a `.set()` tombstone
counts write, IR node definitions stay excluded from the class entirely
(the pre-existing rule must keep working), and the pinned total is
unchanged by the split. Revert proof: dropping the write branch fails
the tombstone case.
Census's own suites **87 passed**, gate **161 / 13 / 487 / 71**, lint
clean, `--strict` exits 0.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
a6af3188df |
fix(core): startup recovery deadlocked on its own per-task lock (#2809)
## The bug `recoverStaleTransitionPendingImpl` runs its whole per-task body inside `store.withTaskLock(id, …)`. On the PostgreSQL arm it then read the task with `store.getTask(id)` — and `getTaskImpl` opens with `store.withTaskLock(id, …)` too. **The per-task lock is non-reentrant.** This codebase states that invariant in prose in two other files: > "nesting inside `withTaskLock` would deadlock since the lock is non-reentrant" — `branch-and-pr-entities.ts:561` > "because the per-task lock is non-reentrant" — `workflow-ops.ts:464` So the sweep waited forever on a lock its own frame was holding. **PostgreSQL-only — which is every production install.** The SQLite arm on the very next line reads through `readTaskFromDb`, a lock-free row read. The backend-mode port swapped only the PostgreSQL arm to `getTask`. The fix restores a lock-free read (`readTaskRow`) on that arm; nothing else changes. ## Why it survived until now The branch is entered **only** when a stale marker names a plugin hook the trait registry still knows (`hasSurvivingPluginHook`). Three nearby cases all miss it: | marker | path | |---|---| | none | the row is never scanned | | only `default-workflow:postCommit` | `hasSurvivingPluginHook` false — marker just cleared | | names an **uninstalled** plugin hook | reconciled away as degraded; nothing survives to re-run | | names a **registered** plugin hook | **reaches the in-lock read → deadlock** | Those first three are what the existing tests cover. The fourth is precisely the state a crash mid-hook leaves behind. All four are asserted in the new suite so the path cannot be re-narrowed and called covered. ## Impact This sweep runs at **startup**. A task left with such a marker deadlocks startup recovery — and because it deadlocks *while holding the task's lock*, that task is also left permanently unlockable. ## How it was found, including a correction By **bisection**, not by reading. An earlier attempt of mine to drive this recovery reported that "the sweep never returns". That was wrong in a way worth recording: the sweep returns fine in three of the four cases, and generalising the one hang to the whole function is what hid the actual trigger across several sessions. Narrowing case by case — empty store, plain task, default-only marker, unknown-hook marker, registered-hook marker — put the fault on one line. ## Verification - **Mutation-verified against the real defect.** With the fix reverted, the regression case fails by name — `recoverStaleTransitionPendingImpl did not settle within 8000ms — deadlock` — while the other three stay green. That is the actual pre-fix behaviour, not a simulation of it. - Every case is **timeboxed** on purpose: a deadlock otherwise surfaces as a suite-level timeout naming no case, which is useless for locating the fault. The deadline is not a flake knob — the fixed code settles in ~150 ms and the broken code never settles, so there is no value in between to tune. - A **vacuity guard** (no markers → scans nothing) so a change that stopped listing marked rows can't leave the other cases green. - `pnpm test:gate` — **exit 0** - full live-PG E2E surface — **152/152** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
109204c590 |
fix: the query class — three sweeps that never ran on a renamed board (#2818)
Three sweeps that **never ran at all** on a renamed board, plus the shared answer the rest of the class needs. Consolidated from three handoff branches so the helper appears once. #2811 merged, so this is my only open PR. `#2800` measured this class and shipped evidence deliberately without conversions: `listTasks({ column: "<literal>" })` filters in the store, so on a renamed board the read returns an **empty array** and the sweep it feeds does nothing. The census scores the comparison *inside* the loop, never the query above it. ## What was broken | file | census count | what actually happened on a renamed board | |---|---|---| | `backlog-pressure-reporter.ts` | **0** | both reads empty, ratio computed as 0/0 — **the alert never fired**, on a board that may be under exactly the pressure it reports | | `stale-task-reporter.ts` | **0** | both reads empty — **no stale-task signal ever raised**, where work is most likely sitting unnoticed | | `restart-recovery-coordinator.ts` | flagged | sweep never ran — **an engine restart left interrupted tasks stuck with no requeue** | Two of the three have a census count of **zero**. They contain no lifecycle comparison at all, so they have never appeared in the backlog, in a per-file list, or in any "N → 0" claim — and were completely inert. **A file at zero is not evidence of anything.** ## The shared answer, and what it is not Every existing resolver answers a **per-task** question. A query has no task in hand, so it needs the project-level one: every column any workflow declares for a role, unioned with the legacy ids so a board mid-rename still finds rows under the old ones. The set is never empty, so a caller cannot accidentally query nothing. The header states what it is **not**: answering a per-card question from the union would mark a card as review because some *other* workflow calls its column review — the flat-set mistake this program has made four times. ## The finding that generalises: the query is rarely the whole defect `stale-task-reporter` **still reported zero after the query was fixed** — `getTaskAgeStalenessSignal` defaults to the legacy pair, so a card the query now returned was refused inside the signal. Converting only the query would have looked like a fix and changed nothing. That is a caveat on #2800's approach, offered as refinement rather than correction: **asserting the query ARGUMENT is right when pinning a known defect** (the outcome is 0 either way) **and insufficient when proving a fix**, because the outcome is the only thing that distinguishes a real conversion from a deeper one. All three conversions here assert outcomes. `restart-recovery` had three layers — query, a redundant re-assertion (deleted; a test pins the `paused` guard it did contribute), and a move destination that was **already** resolved but whose warning comment was stale. A stale warning is its own hazard: it told the next reader a defect existed where none did. ## Verification - helper **8 passed** · three reporter/coordinator suites **29 passed** - `pnpm test:gate` **161 / 13 / 487 / 71** · lint clean · `--strict` exits 0 · four `tsc` targets clean - each conversion revert-proven independently; the failing case is named in each test header ## Two mistakes worth recording **The helper's own test caught a bug in it.** My first draft wrapped the definition loop in one `try`, and `parseWorkflowIr` **validates** rather than parses — one malformed row would have returned legacy-only lanes for *every* workflow, indistinguishable from the bug it exists to fix. Now isolated per definition. **I clobbered the core barrel** by taking `index.ts` wholesale from a handoff branch, dropping two exports `main` had added since; three packages stopped compiling. Taking a file from another branch takes its whole contents, including what is now stale — for a barrel that is nearly always wrong. Re-applied as a single edit on top of `main`. ## Not included `self-healing.ts`'s 49 — actively owned and mid-conversion; an outside refactor there produces conflicting halves of one sweep. `project-engine.ts` (7) and `executor.ts` (2) need their own read of what each sweep does with the rows, which these three are the argument for. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
1824c04584 |
fix(core): restore an archived card to the lane it came from (#2832)
## What Fixes the defect #2824 measured. That PR's characterization cases — merged and asserting the wrong-but-real behaviour — are flipped here to the correct lanes, which is what they were written to do. ## The bug ```ts // archive-lifecycle-2.ts const preArchiveColumn = task.preArchiveColumn ?? "todo"; ``` **`preArchiveColumn` has no database column.** It exists on the `Task` type and in the archive snapshot, and nowhere else — so the in-place restore cannot carry it, `store.getTask(id)` reads a live row that never had it, and the literal decided the destination for **every unarchive that has ever run**. | board | what happened | |---|---| | **default** | `todo` is declared, so the resolver returned it. Restores landed in the queue and **looked right**. | | **custom** | `todo` is declared nowhere, so the resolver took its "no usable history" branch and returned the **complete** lane. A card archived mid-implementation came back marked **finished**. | That coincidence is why this survived **three** separate fixes to `resolveUnarchiveTargetColumnImpl` — a `?? "done"` that invented a column, an `isColumn` legacy-enum gate, and the same gate one function over. Every one was correcting how the resolver interprets a value that never arrived. ## The fix is two halves, and either alone does nothing 1. **Capture** — `taskToArchiveEntryImpl` records `task.column` into the snapshot. That is the last place the original is still in hand, since the entry's own `column` is set to `"archived"` on the line above. 2. **Read** — `unarchiveTaskImpl` reads the **snapshot it already loaded**, not the restored row. I shipped half of this first and watched the destination stay wrong, which is how I found that the field has no row to live on. Mutation matrix: | state | result | |---|---| | both halves | **5/5 pass** | | capture only (read reverted) | **3 fail** | | read only (capture reverted) | **3 fail** | I also tried carrying it through `restoreTaskFromArchive`'s row update — that fails to typecheck, which is the proof that no such column exists and the snapshot is the only source. ## Behaviour changes, deliberately - **Custom boards** — a card returns to the lane it was archived from instead of appearing finished. - **Default board** — a card archived from `done` restored to `todo` under the literal and now restores to `done`. Returning finished work to the queue was the fallback showing through, not a rule anyone chose; the resolver's own branches say a card archived from a declared column goes back to it. ## One expectation of mine was wrong, and the resolver was right I expected a card archived from the review lane to return to **hold**. It returns to the review lane, and that is correct: `.review` is derived from the `mergeOrchestration` flag, **not** from `human-review`. The fixture's review column declares `human-review` + `merge-blocker` only, so it is not a `.review` lane to the resolver — just a declared column with usable history. The case now asserts that with the reasoning attached, so the next reader does not "fix" it back to hold. ## Verification - unarchive suite — **5/5**, mutation matrix above - `pnpm test:gate` — **exit 0** - full live-PG E2E surface — **164/164** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
654d28c104 |
test(engine): live-PG coverage for the timing role — and it is correct (#2828)
## What One new live-PostgreSQL E2E suite, 2 tests. **No production file is touched** — evidence, per the E2E worker's remit. `packages/engine/src/__tests__/workflow-timing-trait-live-e2e.pg.test.ts` ## This one is a clean bill of health Coverage for the last lifecycle role that had none — and the seam turns out to be **correct**. Recorded deliberately: a working conversion with no test is one refactor away from a silent regression, and this program should be able to say which seams are right, not only which are broken. `cumulativeActiveMs` accrues while a card sits in the WIP lane, and both halves of the segment boundary resolve that lane by role: ```ts const isWip = (column) => inRole(column, ctx.lifecycleColumnSets?.wip, ctx.lifecycleColumns?.wip, "in-progress"); ``` Note the literal at the end. That is the **same optional-parameter shape** this series found broken at four other seams — `shouldHoldActiveFileScopeLease` (#2795), `evaluateParkedAgentTaskLink` (#2798), `resolvePlanningContinuationCandidate` (#2799), `hasAutoHealableVerificationBufferFailure` (#2802) — where a caller failed to supply the resolved answer and the literal silently took over. Here the move path **does** supply it. Measured on a live store: ``` default: after exit cumulativeActiveMs=84 renamed: after exit cumulativeActiveMs=75 <- accrues on `building`, not just `in-progress` ``` ## What it guards If the move path ever stops populating the resolved columns, the literal takes over and a renamed board's cards accrue **exactly zero** active time — silently, because zero is a legitimate value for a card that has not run. The operator sees no error, just wrong numbers on every custom board. **Mutation-verified against precisely that regression:** blinding the resolved answer (`inRole(column, undefined, undefined, "in-progress")`) fails the renamed case and leaves the control green. This is a guard with demonstrated power, not a test that happens to pass. ## Both boundaries are asserted `cumulativeActiveMs` is `0` while the card is still in WIP and only accrues when it leaves; `firstExecutionAt` is stamped on entry. A test that checked only the final number would pass against an implementation that accrued at the wrong boundary, so both are pinned. ## Verification - new suite — **2/2 passed**, mutation-verified - full live-PG E2E surface — **161/161** - `pnpm lint` — clean Lane: `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is reachable, so the merge gate is unaffected. Throwaway per-file database; never port 4040. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
e5c9ea3870 |
fix(core): resolve the task's own terminal node in the node-override guard (#2812)
## What
Fixes the defect **#2793 measured** but deliberately did not fix. That
PR's two characterization tests are flipped here to assert the correct
behaviour — which is what they were written to do.
## The bug
`updateTask({ nodeId })` passes through `validateNodeOverrideChange`
**twice**, and both calls were wrong in different ways:
| call | how it answered "is this terminal?" |
|---|---|
| `branch-and-pr-entities.ts:568` | `resolveTaskWorkflowIrSync` — the
**default** workflow for every task under PostgreSQL |
| `task-update.ts:53` | no options at all → `defaultIsTerminalNodeId`,
the bare literal `nodeId === "end"` |
On a board whose terminal node is not named `end`, the FN-7641 guard
inverted in both directions:
- an override to a **non-terminal** node that happens to be named `end`
was **rejected** with a merge-proof error about finalizing a card the
operator was not finalizing;
- an override to the board's **real** terminal node was **written
verbatim**, no error, card unadvanced — the silent no-op FN-7641 exists
to prevent.
## The fix
A new `isTaskTerminalNodeIdAsync` resolves the task's own workflow, with
the **identical** literal fail-soft for an unresolvable one. Both call
sites use it — pre-resolved, because `validateNodeOverrideChange`'s
callback is synchronous and it asks the question at most once.
**Nothing forced the sync call at either site**: both frames are already
`async` and already awaiting. That is the same finding as #2809's
review, one file over.
The sync helper is **deleted, not kept as a fallback**. Keeping both
would re-create the half-conversion this program keeps finding — one
caller resolved, one not, and no way to tell from a call site which it
got. `branch-and-pr-entities.ts` also leaves the sync-resolver call-site
allow-list (ratchet green, 3/3), the **second** of the six allow-listed
sites to close.
## Why both halves were needed — and how that is proven
#2793's mutation matrix showed the rejected-`end` case is
**over-determined**: both guards independently called it terminal, so
correcting either one alone changed nothing an operator could see. That
is why fixing only the allow-listed sync site would have looked like
progress and delivered none.
Re-measured here, on the fixed tree:
| state | result |
|---|---|
| both guards fixed | **3/3 pass** |
| inner guard reverted to no-options | **1 fails** |
| outer guard reverted to the literal | **1 fails** |
## Tests
#2793's two cases now assert the fixed behaviour and keep their
reasoning:
- the non-terminal `end` override is **written**, and the card stays in
review — a routing change, not a finalize;
- the real terminal `finish` override is **refused** without merge
proof, **and** the field is not written on the way to refusing.
The fixture-integrity case (`finish` is the end node, `end` is not) is
unchanged — it is what stops both assertions passing for the wrong
reason.
## Verification
- terminal-node suite — **3/3**, both-halves matrix above
- sync-resolver call-site allow-list ratchet — **3/3**
- `pnpm test:gate` — **exit 0**
- full live-PG E2E surface — **151/151**
- `pnpm lint` — clean
Changeset included (`patch`, category `fix`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
7c30b54255 |
test(engine): live-PG evidence that restore lands every custom-board card in DONE (#2824)
## What One new live-PostgreSQL E2E suite, 5 tests. **No production file is touched** — evidence, per the E2E worker's remit. `packages/engine/src/__tests__/workflow-unarchive-target-live-e2e.pg.test.ts` ## The finding I went looking for coverage of the one lifecycle role that had none — `archived` — in the function with the worst track record in this program. `resolveUnarchiveTargetColumnImpl`'s own comments record **three** separate defects fixed on those few lines: a `?? "done"` that invented an undeclared column, an `isColumn` legacy-enum gate that rejected every renamed id, and the same gate one function over that dropped a renamed board's stored history on read. All three were reasoned from source. **None had a live-store test.** With one, the path is still broken — and the cause is upstream of everything those fixes touched: ```ts // archive-lifecycle-2.ts:441 const preArchiveColumn = task.preArchiveColumn ?? "todo"; ``` **`preArchiveColumn` is never written.** Across `packages/core`, every occurrence *reads* it or copies it through — into the archive entry, back out of it, through serialization. Nothing ever sets it from `task.column` when a card is archived. Measured on both boards: ``` default: after archive column=archived preArchiveColumn=undefined -> resolver target=todo renamed: after archive column=archived preArchiveColumn=undefined -> resolver target=shipped ``` So the fallback fires for **every restore that has ever happened**, and the two boards diverge on what `"todo"` means to each: | board | what happens | |---|---| | **default** | `todo` is declared, so the resolver returns it. Every restore lands in the queue — right for a card archived mid-implementation, wrong for one archived from `done`, and it **looks** right. | | **renamed** | `todo` is declared nowhere, so the resolver takes its "no usable history" branch and returns the **complete** lane. Archive a card mid-implementation, restore it, and it comes back marked **finished**. | That coincidence on the default board is why this survived three rounds of fixes to the resolver: they were correcting how it interprets a value that never arrives. ## Evidence discipline - **Observed state** — the persisted `column` after a real `archiveTask` + `unarchiveTask` against a real store and real stored workflows. - **Characterization**: the cases assert today's wrong-but-real behaviour so the defect is executable rather than argued, and they are written to flip when the write is added. - **The default-board control** is what shows the coincidence, not just the failure. - **Mutation-verified**: changing the hardcoded fallback from `"todo"` to the renamed hold id fails **all five** cases — every assertion binds to that literal, which is the claim. ## The fix, proposed rather than included One line: persist the card's column when archiving, so `preArchiveColumn` carries real history and the resolver — already correct — can use it. I did not include it because **it also changes default-board behaviour**: a card archived from `done` currently restores to `todo` (the fallback), and with real history it would restore to `done`. That is the better outcome, but it is a visible placement change for every existing install, and that decision belongs to whoever owns archive semantics rather than to an evidence PR. The five cases above are the acceptance criteria for it — the three renamed expectations become the lane the card was actually in. ## Verification - new suite — **5/5 passed**, mutation-verified - full live-PG E2E surface — **159/159** - `pnpm lint` — clean Lane: `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is reachable, so the merge gate is unaffected. Throwaway per-file database; never port 4040. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
9a5155c71a |
fix(tests): main red — a census source guard that a COMMENT could invert (#2825)
## Red on main
```
lifecycle-column-census > the baseline can always be re-recorded
> writes the baseline BEFORE the rise check can exit
AssertionError: expected 19345 to be greater than 26374
```
Read literally, that says the CLI now runs its rise check *before* the
`--update-baseline` write — which would break the one command whose
entire job is re-recording, and would be a genuine bug worth stopping
for.
**It does not.** The order in code is correct and unchanged:
| | line |
|---|---|
| `if (updateBaseline) { … writeBaseline() … process.exit(0)` |
`scripts/lifecycle-column-census.mjs:487` |
| `"column-guard count ROSE"` + `process.exit(1)` | `:510` |
## What actually moved was a comment
Line **359** explains this exact failure mode and quotes the marker
verbatim:
> …`"column-guard count ROSE"`, which is the opposite of what happened
and sends the reader looking for…
So `cli.indexOf("column-guard count ROSE")` found the **prose**, 7000
characters before the branch it was meant to locate.
A guard that a comment can invert is not measuring control flow. And the
honest-looking fix — reword the comment — silently re-arms the same trap
for whoever explains this next.
`cliSource()` now strips comments before indexing. The same defence is
already used by `archived-column-gate-parity.test.ts`, for the same
reason: notes documenting *why* a literal is dangerous have to mention
the literal.
## Kept, not deleted
The end-to-end block below these does cover the contract — it drives the
real CLI and asserts exit code, baseline content and printed output, and
its own comment names the ordering bug. It would have been defensible to
delete the two source-text cases as redundant.
I kept them because two guards at different levels is the point: **e2e
proves the behaviour, these locate the branch that provides it.** They
only needed to stop being defeated by prose.
## Evidence
The real ordering bug — make a rise exit before the update branch writes
— fires **all three**:
| guard | failure |
|---|---|
| source order | `expected 9454 to be greater than 9505` |
| slice / uniqueness | `expected 10433 to be -1` |
| end-to-end | `expected 1 to be +0` (exit code) |
**My first mutation attempt was invalid** and I nearly reported it as
evidence: it moved the block by line range, mangled the file, and both
markers disappeared — the resulting `-1`s look like a firing guard but
prove nothing. A mutation that corrupts its target is not evidence that
a guard works.
Engine **10991 passed / 0 failed** · gate **732 green** · lint clean.
Test-only; the CLI is restored clean.
## Note on duplicated effort
#2811 and my #2814 both re-recorded the census baseline for #2783's
rise, concurrently. No harm done — but this file is now a fleet-wide
contention point, and the per-file baseline shape exists precisely to
avoid that. Worth one owner for census/ratchet fixes rather than whoever
notices first.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
8c79da364c |
fix(engine): FN-8356's duplicate-marker cleanup was inert on a renamed board (5 call sites) (#2827)
Fourth unowned finding picked up after both batch PRs (#2783 core, #2785 engine) merged without addressing their reports. Completes the `isNearDuplicateCanonicalInactive` seam alongside #2823, which fixed the sixth (core) site. ## The defect All five engine call sites — `self-healing.ts` ×2, `triage.ts` ×3 — called the predicate without the canonical's resolved column flags, so it fell back to the legacy `done`/`archived` ids. A canonical resting in a renamed complete column (`shipped`) read as **still active**, so every *"the canonical is inactive, so clear the marker"* branch failed to fire. The user-visible result is precisely the stranding FN-8356 was written to remove: a card keeps its **"Needs your decision"** duplicate badge pointing at work that shipped days ago, and no decision can resolve it — the detail banner deliberately offers none for an inactive canonical. ## Measured With the fix reverted, exactly one case flips: ``` ✓ clears the FN-8353-shaped hidden decision for every inactive canonical state × renamed vocabulary: clears the decision for a canonical resting in a RENAMED complete column ✓ renamed vocabulary: leaves the decision alone while the canonical is still in the WIP lane ✓ leaves active canonical decisions, user pauses, unrelated reasons, non-marker sources untouched Tests 1 failed | 3 passed (4) ``` With it: `Tests 4 passed (4)`. ## Wiring is proven separately from behaviour A behaviour test on one call site says nothing about the other four — that is the failure this whole lane keeps re-finding, so I did not rely on it. With #2822's barrel-import fix applied locally, the seam gate's staleness check **fails both engine allow-list entries as "now supplied"** — it can no longer find an omitting call site in either file. That is the proof for all five. Consequence worth flagging: **when the second of #2822 / this PR lands, the two engine entries in #2822 must be deleted.** CI fails until they are, by design — the exemption cannot outlive its fix. ## Both negatives included A canonical still in the WIP lane must **not** have its decision cleared, under each vocabulary. Resolving real flags must not degrade into "every column is terminal", which would dismiss a duplicate decision the operator has not made yet. ## Design note The helper is module-private in each file rather than shared. `findColumn` is already duplicated exactly this way in `hold-release.ts`, `merge-trait.ts`, and `workflow-capacity.ts` — following the established shape beat adding a cross-module abstraction for a bug fix. ## Verification `pnpm test:gate` green · 27 triage suites / 387 tests green · engine `tsc` 0 · lint 0 · changeset included. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
bcc8bf17c1 |
fix(core): brand the unregistered built-in workflow fallback (#2815)
> **Corrected after opening.** The first version of this PR claimed the
id cross-check reported `"default"` for *every* authored workflow and
stranded cards in triage recovery forever. That was wrong, and the
correction is below in full. The reachable defect is the branding hole;
removing the id check is correctness, not a live bug fix.
## The bug (reachable)
`resolveWorkflowIrById` has four ways to substitute the default coding
IR. Three brand the result via `markFellBack`. The fourth did not:
```ts
if (isBuiltinWorkflowId(workflowId)) {
const builtin = getBuiltinWorkflow(workflowId);
const ir = builtin?.ir ?? defaultCodingWorkflowIr(); // <- unmarked substitution
```
An id that *looks* built-in but is not registered — a workflow removed
between releases, a typo'd selection — lands there and silently gets the
default coding IR. `resolveWorkflowIrForTaskWithProvenance` then reports
**`source: "selection"`** for it, handing a caller the default board's
graph under the selected workflow's name. That is precisely the lying
signal the API exists to prevent.
Found by review on this PR (thanks — see the thread), not by me.
## The correction to my own claim
I also deleted an id cross-check that ran after the marker check and
reported `"default"` when the resolved IR's `id` differed from the
requested workflow id. I justified that by saying it misfired for every
authored workflow, because `createWorkflowDefinition` stores an IR
verbatim while minting `WF-NNN` separately:
```
store workflow id = WF-001 stored ir.id = custom:prov
PROVENANCE source = default resolved ir.id = custom:prov <- the CORRECT IR, called a guess
```
**That measurement is real but it came from this suite's own fixture.**
Neither `WorkflowIrV1` nor `WorkflowIrV2` declares an `id` field. An
editor-authored workflow carries none, so `resolvedId` is `undefined`
and the check passed it as `"selection"` — the misfire never reached
`triage.ts`'s post-U11 intake recovery, the one production consumer. My
"declined forever, not deferred" claim does not hold.
Removing the check is still right: it is **unreliable** (it interrogates
a property the IR types do not declare, and when one is present it is
the author's id, unrelated to the store-minted row id) and **redundant**
(all four substitutions are now branded, and the marker is checked
first). But it is a cleanup, not a fix — and saying otherwise is how a
narrow change gets backported as a critical one.
## Tests
| case | asserts |
|---|---|
| unregistered **built-in** id | **`source: "default"`** — the reachable
defect |
| missing definition | still `"default"` |
| no selection | still `"default"` |
| IR carrying its own id | `"selection"`, with the id mismatch asserted
explicitly so it can't pass for the wrong reason |
| the resolved IR is the task's own board | contains the renamed hold
column, not a default-board id |
**Mutation-verified:** removing only the branding fails the
unregistered-builtin case and nothing else — which shows it covers that
specific hole rather than overlapping the others. Restoring the id check
fails only the carries-its-own-id case, leaving both fallback cases
green, which shows the deletion did not widen trust.
## Blast radius
`triage.ts:1211` is the only production consumer of the provenance
result across core, engine, dashboard and CLI. Every other `.source ===`
hit is an unrelated field on an unrelated type; the two index hits are
re-exports.
## Verification
- new suite — **5/5**, mutation-verified in both directions
- `pnpm test:gate` — **exit 0**
- `pnpm lint` — clean
Changeset included (`patch`, category `fix`), rewritten to describe the
branding fix rather than the overstated one.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
2ccd78abbc |
fix: main is red on the lifecycle ratchet — re-record the census baseline (#2811)
**`main` is RED on the lifecycle ratchet right now.** `node scripts/lifecycle-column-census.mjs --strict` exits **1** on pristine `origin/main`, which is the `Lint` job's *Lifecycle-column ratchet* step — so **every open PR fails Lint** until this lands, regardless of its own contents. Verified on a detached checkout of `origin/main`, not on a branch of mine. ## Cause Eight `DELIBERATE-LITERAL` markers were added across seven files without re-recording the baseline: ``` packages/core/src/task-move-disposer.ts (in-progress, todo) packages/core/src/task-store/archive-lifecycle-2.ts (archived) packages/dashboard/src/github-tracking-comments.ts (done) packages/dashboard/src/gitlab-tracking-comments.ts (in-progress) packages/dashboard/src/server.ts (archived) packages/dashboard/src/task-planner-chat-context.ts (done) packages/dashboard/src/test/mockCoreEngine.ts (in-review) ``` Adding a marker RECLASSIFIES a site (column-guard → deliberate), so the tracked deliberate totals move and `--strict` fails until the baseline records the new shape. It is the same mechanism that turned #2775 red earlier today — a marker landing without its baseline — which is worth noting because it has now happened twice from different PRs. ## The fix Baseline re-recorded, nothing else. Zero source changes; the diff is one derived file. - `--strict` exits **0** - `pnpm test:gate` — **161 / 13 / 487 / 71** - `pnpm lint` clean ## Worth a follow-up by whoever owns the ratchet The failure is structural rather than careless: a PR that adds a marker is *doing the right thing*, and the baseline requirement is only discovered when CI goes red — after merge, for everyone else. Two options, neither of which I am taking unilaterally on a red-main fix: 1. have `--strict` treat a marker-only reclassification as an accepted rise (it is not new debt — the count of unconverted guards goes **down**); 2. or fail the PR that adds the marker, by comparing against the base ref rather than the recorded baseline — the machinery for that already exists in this script. I would take (1): a marker is the documented way to close a site, and requiring a second mechanical step to record it is a trap that catches good behaviour. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved lifecycle census error messages to distinguish genuine increases in column-guard debt from reclassified deliberate literals. * Added clearer remediation guidance for reclassified results, including when to update the baseline. * Updated lifecycle census baseline mappings to reflect current classifications. * **Tests** * Added coverage for unchanged baselines, genuine guard-count increases, and marker-only reclassification scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
9fab32e1d9 |
fix(tests): 5 engine reds from #2783 — a stale census baseline and a turn-counting test (#2814)
## Red on main #2783 (batch-core) landed and put **5 failures** on `main`. Both causes are the bookkeeping half of correct changes, not defects in them. ## 1. The census baseline — 4 failures `census-baseline-corruption-guard` plus 3 `lifecycle-column-census` ratchet cases, all downstream of one thing: ``` lifecycle-column-census --strict: column-guard count ROSE packages/core/src/task-move-disposer.ts (DELIBERATE-LITERAL: in-progress): 0 -> 1 packages/core/src/task-move-disposer.ts (DELIBERATE-LITERAL: todo): 0 -> 1 packages/core/src/task-store/archive-lifecycle-2.ts (DELIBERATE-LITERAL: archived): 0 -> 1 packages/dashboard/src/github-tracking-comments.ts (DELIBERATE-LITERAL: done): 0 -> 1 packages/dashboard/src/gitlab-tracking-comments.ts (DELIBERATE-LITERAL: in-progress): 0 -> 1 packages/dashboard/src/server.ts (DELIBERATE-LITERAL: archived): 0 -> 1 ``` **The rise is legitimate.** #2783 *annotated* documented fast-path literals — e.g. `task-move-disposer.ts`'s *"a fast path, not the guard … the actual lane decision is the RESOLVED membership test inside this block"* — and the census tracks marked literals per file. Re-recorded with `--strict --update-baseline`; the same run also **tightened 20 entries whose counts dropped**, so this moves the ratchet down as well as up. ## 2. The disposal-order test — 1 failure ``` executor-user-cancel > re-dispatch (task:moved → in-progress) awaits prior disposal before execute() AssertionError: expected -1 to be greater than 2 ``` `-1` reads like the re-dispatch was **dropped**. It was not — that would be a real cancel-race bug, so I checked before touching the test: ``` PROBE_MICROTASK callOrder=["abort-started","abort-resolved","dispose","execute"] PROBE_AFTER_TIMER callOrder=["abort-started","abort-resolved","dispose","execute"] ``` Correct order, reached once drained, unchanged after a real 50ms timer. The test drained exactly **two** microtask turns and #2783's disposer refactor added await hops, so `execute` had not been recorded yet. A fixed turn count encodes today's await depth into the test: any added `await` on the product path fails it for a reason that has nothing to do with the invariant. It now waits on the **outcome** via `vi.waitFor`. The ordering assertion is untouched and is still the point. ## Evidence | mutation | result | |---|---| | `execute` never recorded (stands in for a dropped re-dispatch) | **fails** — `waitFor` times out | | `execute` observed *before* `dispose` | **fails** — `expected 'execute' to be 'dispose'` | | baseline: fresh `--strict` run | *"every file matches its baseline exactly"* | Engine **10985 passed / 0 failed** (was 5 failed) · gate **732 green** · lint clean. ## Method note, against myself I pre-flighted #2783 and **reported it clean** — but I ran only `@fusion/core` and the dashboard `api` group, because that is what the diff touches. The census and disposal tests live in `packages/engine`, which batch-core does not modify at all. **The suite that breaks is not always the suite the diff points at.** A cross-package ratchet like the census is exactly the case where scoping pre-flight to the changed packages produces a confident "clean" that is wrong. Pre-flight needs the engine suite regardless of which package a batch touches; I have adjusted accordingly for the remaining queue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
6fc98fd6c7 |
the third census-invisible class: 51 hardcoded moveTask destinations, measured — and duplicates never archived on a renamed board (#2808)
A third census-invisible class, measured — plus the two worst instances
fixed.
## The shape
```ts
if (task.column !== "in-review") { … return; } // the census counts THIS
await this.store.moveTask(taskId, "in-progress"); // and cannot see THIS
```
The census is an AST scan for **comparisons**. A `moveTask` destination
is a **call argument**, so no backlog entry ever points at one.
Converting the guard alone is *worse than converting neither*: the
handler starts admitting work on a renamed board and then tries to move
the card into a lane that board may not declare.
This bit twice in one week — #2797 (`branch-worktree` requeued into a
lane that may not exist) and #2807 (a GitHub "changes requested" review
dropped, then a move to a hardcoded `in-progress`). Both times it was
found only because the guard *next to it* happened to be under
conversion. So I went looking.
## Measured
Across `core`/`engine`/`dashboard`/`cli`/`plugins`, excluding
`__tests__`/`*.test.*` and comment lines:
| | count |
| --- | ---: |
| hardcoded `moveTask` destinations in production | **51** |
| …passing `recoveryRehome: true` — **deliberate**, not defects | 22 |
| …plain, rejected on a board that does not declare the target | **29**
|
**The 22 must not be "fixed".** `moves.ts` exempts them on purpose
(#1411): a card stranded in an undeclared column has to stay rescuable
to a legacy safe-landing column, or it can never be recovered at all. A
sweep that converts them deletes the rescue path. That distinction is
the reason this is 29 and not 51, and it is why I measured before
writing.
## Why this got sharper recently
The `workflowHasColumn(workflowIr, toColumn)` rejection used to sit
inside a block gated on `isWorkflowColumnsCompatibilityFlagEnabled` — a
settings key **nothing in production writes** — so it never executed and
the legacy `VALID_TRANSITIONS` table decided instead. U12 hoisted it out
of that dead branch and it is now live, proven on a real store by
`live-move-path-undeclared-target.test.ts`:
```
moveTask(card in "todo" -> "triage") now REJECTS: /Unknown column for this workflow/
```
That changed the failure mode of all 29 from *"silently lands the card
in an undeclared column"* to *"throws"*.
**29 is not a crash count.** Whether a throw surfaces or disappears
depends on whether the caller catches, which is per-site and I did
**not** measure it — the doc says so explicitly rather than letting the
number imply severity it hasn't earned.
## Fixed here: 9 of the 29
`duplicate-intake` and `duplicate-guard` both archive a duplicate. On a
renamed archive lane the move is rejected, so **the duplicate is never
archived and keeps sitting on the operator's board as live work** — and
in `duplicate-guard` the row has already been stamped
`deterministicDuplicateOf`, so it is *marked* a duplicate while
occupying an active lane. Half-applied, which is the same trap as
#2797's branch clear.
Both now resolve the `archived`-trait column from the task's own
workflow through one shared helper, unioned with the legacy id.
**`cli/commands/task-lifecycle`** — `finalizePullRequestMerge` and
`finalizeNoOpMergeTask` both move the card to a hardcoded `"done"`, and
both run `updateTask({ status: null, mergeRetries: 0 })` *first*. On a
rejection the merge has already landed and the bookkeeping is already
cleared while the card never reaches its complete lane: the operator
sees a merged branch, a card still sitting in review, and a reset retry
counter. Same half-applied shape as #2797's branch clear. Both now route
through one resolver so they cannot drift.
**`contamination` / `foreign-only-contamination` (×2) /
`restart-recovery-coordinator`** — four recovery requeues to a hardcoded
`"todo"`, none of them a `recoveryRehome` escape. On a board without
that column the move is rejected and **the recovery never completes** —
the card stays contaminated or stranded, which is precisely the state
these paths exist to clear.
**Consolidation.** `resolveReboundTargetForTask` and
`resolveArchiveTargetForTask` now live beside
`resolveTaskLifecycleColumns` in `workflow-lifecycle-traits`, already
the store-dependent resolution seam. My first pass put the archive
helper inside `duplicate-intake` and had `duplicate-guard` import it
from there — wrong home, and it would have grown a copy per caller as
more sites converted. Seven call sites now share two definitions.
**Plain (non-`recoveryRehome`) destinations: 29 → 21.**
**Coverage on the CLI pair is scoped, and I'd rather say so than imply
more:** the test covers the *resolver*, not the two call sites. Both
enclosing functions are private and reachable only through
`processPullRequest`, which needs a live GitHub surface — exporting them
purely to test wiring is a worse trade than stating what is covered.
Three cases: renamed lane resolves, no-workflow falls back to the legacy
id (which also pins that a default board is byte-identical), and a
throwing lookup falls back.
## Revert result (measured)
| conversion | reverted → |
| --- | --- |
| duplicate archive destination | new case fails — `moveTask` called
with `"archived"` on a board whose archive lane is `boxed` |
| CLI complete-lane resolver | replacing the body with a bare `return
"done"` fails the renamed case |
| both move-target resolvers | replacing either body with a bare return
of its legacy id fails 5 cases across the resolver suite and
`duplicate-guard` |
Each resolver has a **non-vacuous companion** asserting it does *not*
return the legacy id on a renamed board — without it, a resolver
returning any string would pass. The fallback cases are load-bearing
rather than padding: `resolveWorkflowIrForTask` degrades to the built-in
IR rather than throwing, and the built-in rebound/archive lanes *are*
`todo`/`archived`, so those cases also pin that a default board is
byte-identical.
The pre-existing case asserting the legacy `"archived"` passes both
ways, which is exactly why it could not detect this and why the new one
supplies a workflow.
## Ownership note
`packages/core` was `batch-core`'s territory and `packages/cli` was
`batch-cli-plugins`'. Both batches have landed, and this is
newly-discovered work in the class documented here rather than leftover
conversion backlog. Four sites, two shared helpers — happy for either
half to move if those owners would rather carry it.
## Verification
- `pnpm test:gate` — 161 + 487 + 13 + 71, green
- `duplicate-guard` + `duplicate-intake` — 40 passed
- `tsc` on core and engine — clean
- `pnpm lint`, `check:changesets`, census `--strict` — all clean (run
explicitly; a clean `pnpm lint` alone is not evidence the CI Lint check
passes)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Duplicate tasks are now archived to each workflow’s configured archive
lane.
- Completed tasks are moved to the workflow-specific completion lane,
with a safe fallback for older workflows.
- Recovery and requeue actions now use each workflow’s configured
rebound lane instead of assuming a fixed destination.
- **Documentation**
- Added guidance on avoiding failures caused by hardcoded workflow
destinations and incomplete lifecycle conversions.
- **Tests**
- Added coverage for renamed workflow lanes, fallback behavior,
duplicate archiving, and recovery destinations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
240a6be0aa |
fix(core): dependency update deadlocked on a self-blocked task (#2810)
## The bug `updateTaskDependenciesImpl` wraps its whole body in `store.withTaskLock(id, …)`, then reads the current blocker with `readDepTask(task.blockedBy)` → `store.getTask()`. `getTaskImpl` opens with `withTaskLock(id, …)` too, and the per-task lock is **non-reentrant**. So when `blockedBy` is the task's **own id**, the call waits forever on a lock its own frame holds — and holds that lock while doing so, leaving the row permanently unlockable. ## Found by generalising #2809, not by luck #2809 removed one `getTask`-inside-`withTaskLock`. An AST scan for the same shape across `packages/core` and `packages/engine` returned **exactly three sites**: | site | verdict | |---|---| | `lifecycle-ops.ts:1049` | the deadlock fixed in #2809 | | `update-task-deps.ts:233` (`assertTaskExists`) | **safe** — a self-dependency is rejected 15 lines earlier | | `update-task-deps.ts:344` (`readDepTask`) | **this bug** | Both surviving sites carry the same `FNXC:SqliteDualPathCleanup` note — *"In backend mode, readTaskFromDb uses store.db (SQLite) which is unavailable. Replace with async store.getTask() calls."* That port is the common cause across the whole class: it swapped a **lock-free** read for a **lock-acquiring** one. ## Why `blockedBy === id` is reachable The dependencies list rejects self-reference explicitly (*"Task X cannot depend on itself"*) — and that guard is precisely why the sibling `assertTaskExists` read on this same lock is safe, so it is left unchanged. **`blockedBy` has no such guard:** `updateTask({ blockedBy })` accepts the task's own id. The first test asserts that rather than assuming it. The whole regression rests on that state being reachable, so it is proven, not stipulated — and it also pins the asymmetry, so a future guard on `blockedBy` will show up here as a deliberate change. ## The fix Return the in-lock copy already in scope instead of re-reading. One line, no new read path, and **strictly more correct than a re-read**: it is the state this mutation is reasoning about, rather than whatever a concurrent writer left behind. ## Verification - **Mutation-verified against the real defect.** With the fix reverted the regression case fails by name — `updateTaskDependencies did not settle within 8000ms — deadlock` — while the precondition and the ordinary-path cases stay green. That is the actual pre-fix behaviour. - **A differential** covering the ordinary case (blocked by *another* task). Without it, a fix that short-circuited *every* blocker read would pass everything else. - Timeboxed for the same reason as #2809: a deadlock otherwise surfaces as a suite-level timeout naming no case. Not a flake knob — the fixed path settles in ~0.5 s and the broken one never settles. - `pnpm test:gate` — **exit 0** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). Independent of #2809 — different file, no overlap — but the same class, and the scan above is the argument that the class is now closed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
b7288572a1 |
engine: a GitHub "changes requested" review was silently dropped on a renamed board (1 → 0) (#2807)
A human reviewer's feedback was being thrown away.
`PrCommentHandler.handleChangesRequested` gated on `task.column !==
"in-review"` and returned early. On any board whose review lane is
renamed, a GitHub **"changes requested"** review produced **no steering
comment** and the card **never went back to work** — the feedback
vanished behind a log line nobody reads. No error, no audit row.
## Census
| file | main | here |
| --- | ---: | ---: |
| `packages/engine/src/pr-comment-handler.ts` | 1 | **0** |
## Two literals, only one countable — again
```ts
if (task.column !== "in-review") { … return; } // counted
…
await this.store.moveTask(taskId, "in-progress"); // INVISIBLE to the census
```
The census scores comparisons. The requeue **destination** is a call
argument, so nothing in the backlog pointed at it — the same pairing as
the branch-worktree auto-requeue in #2797, and the same trap: converting
the gate alone would make the handler *admit* the review and then
attempt a move into a lane the board may not declare, which `moveTask`
rejects. A half-conversion here turns a silent drop into a thrown
rejection. They convert together or not at all.
That is now the second confirmed instance of this shape. The pattern to
look for is a **counted guard whose body performs a hardcoded
`moveTask`** — the guard is the visible half and the move is the
dangerous one.
## Revert results (measured, each run independently)
| conversion | reverted → |
| --- | --- |
| review-lane gate | RENAMED case fails — `updateTask`/`moveTask` never
called; the review is dropped |
| requeue destination | RENAMED case fails — `moveTask` called with
`"in-progress"` instead of the board's wip lane |
The legacy case passes both ways, which is why both vocabularies run. A
non-vacuous companion (renamed board, card sitting in the hold lane)
keeps a gate that admits everything from passing.
## Verification
- `pnpm test:gate` — 161 + 487 + 13 + 71, green
- `pr-comment-handler.test.ts` — 34 passed
- `npx tsc -p packages/engine/tsconfig.json --noEmit` — clean
- `pnpm lint` — clean
- `node scripts/lifecycle-column-census.mjs --strict` — exit 0
(Running the census explicitly, not just `pnpm lint`: CI's Lint job runs
both, and a clean local `pnpm lint` is **not** evidence the Lint check
passes — that cost a round-trip on #2797.)
|
||
|
gsxdsm
|
e84e9d7f60 |
fix: the caller audit — five unwired parameters, five defects in their callers (#2803)
Seven fixes that were sitting on separate handoff branches with no owner while `main` moved. Consolidated, rebased onto current `main`, and verified **together** rather than only per-branch. The individual branches remain if a subset is preferred. This is the same consolidation that got `batch-core` and #2787 adopted. **Close it if it breaks queue policy** — the branch keeps the work safe either way. ## Where these came from #2787's review found an optional parameter whose production caller never passed it. That is a class, so I ran it against everything I had landed and found five more. **All five turned out to have their real defect in the CALLER, not the parameter** — in four of them the parameter was unreachable: | unwired parameter | what was actually wrong | |---|---| | `blocker-fanout.escalationColumns` | the hold default made the count zero — **no bottleneck warning was emitted at all** | | analytics `columnFlagsByName` | routes never built a map — **0 in-progress / 0 in-review beside correct cost totals** | | `isLegacyAutoMergeStampCandidate` | the read **queried a column a renamed board does not have**, so the backfill iterated nothing | | `rankAssignedTasksForWakeDelta` | `getTasksByAssignedAgent`'s `excludeArchived` used the literal — **archived cards returned as open work** | | `duplicate-intake.columnFlagsByColumnId` | intake could **archive or soft-delete a newly created task** as a duplicate of finished work | The heuristic worth keeping: **an optional parameter no production caller fills is a marker pointing at an unexamined caller.** The census cannot see any of these five — every gate is a `Set`/array literal or a query filter, i.e. a definition rather than a comparison. ## Also included - **`executor.ts`** — the stale-spec guard did the exact thing its own comment forbids: on a renamed board it ran on a LIVE task and pulled it out of execution into replan. `activeMergeStatuses` protected merging cards *by accident*, which is why the symptom looked arbitrary. - **`register-project-routes.ts`** — project health reported **0 active tasks**; its list also still contained `triage`, dead since U11. - **`dashboard/app/utils/taskTiming.ts`** — a **second copy** of `getTotalAgentActiveMs`. Core's was converted; the card chip imports this one, so the census counted the site as done while the rendered number stayed keyed on `"in-progress"`. ## Verification Verified as a set: `pnpm test:gate` **161 / 13 / 487 / 71** · core suites **15 passed** · engine **7** · dashboard **12** · four `tsc` targets clean · lint clean · census `--strict` exits 0. Each fix is revert-proven individually; the specific case that fails is named in each test header. ## Two honesty notes **Three guards here are structural, not behavioural, and say so in their headers.** `sanitizeAgentTaskLinks` is a closure inside `createApiRoutes`; the analytics aggregators need a live `AsyncDataLayer`; the stale-spec guard sits deep inside `execute()`. Each ratchet fails on revert — verified — but none is an end-to-end proof, and the headers state which half they cover. **One of my behavioural test sets would have lied.** The intake-dedup cases drive `findSameAgentDuplicates` directly; I removed the wiring to measure the revert and **they stayed green**, because they pin the predicate and not the caller. That is the exact illusion this audit was chasing, reproduced in my own file. The forward now has its own structural check. ## Deliberately not included `worktree-pool.ts:1205` — it **fails safe** (a missed match protects a branch from cleanup rather than deleting it) and sits in the merger's branch-reaping path where the opposite error destroys work. That deserves its owner's judgement, not a drive-by conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
a8cfce8fbd |
executor: stale merge evidence re-entering execution, and a live checkout that read as unowned (12 → 8) (#2805)
Two executor conversions with real operator consequences, one census false positive, and three sites deliberately left with their reasons recorded. ## Census | file | main | here | | --- | ---: | ---: | | `packages/engine/src/executor.ts` | 12 | **8** | Of the 4: three genuine conversions, one reclassification. ## What was broken **`resetMergeStateIfNeeded` — cards re-entered execution carrying stale merge evidence.** Merge state is cleared when a card *leaves* a lane where a merge could have been recorded. Keyed on `in-review`/`done`, a renamed board matched neither, so a card bouncing back into execution kept `mergeDetails` — including a **commit sha from its previous pass** — into its next run. `review` is not a trait, so this resolves through the same five flags (`complete`, `mergeOrchestration`, `mergeBlocker`, `humanReview`) the dependency gates in this file already use; two gates answering "is this a merge-bearing lane?" differently would be a split brain. **The worktree-owner scan — a live checkout read as unowned.** `findActiveWorktreeOwner` asks "is anyone else working in this checkout?". Its in-memory `activeWorktrees` leg is vocabulary-independent, but the **durable** leg — the one that answers after an engine restart, when the in-memory map is empty — filtered with `t.column !== "in-progress"`. On a renamed board that matched nobody, so the worktree read as free and a second task could be handed a checkout another task is live in. Post-restart is exactly when this function matters. Not the query-filter class: that `listTasks` call passes no `column`, so the predicate is the only lane gate on the path. ## A third census false positive in this package Line 16094's `to` is a **review-addressing record status** — the method signature is `to: "queued" | "in-progress" | "addressed" | "failed"`, and the next two lines test it against `"addressed"` and `"failed"`, which are not columns at all. Marked `DELIBERATE-LITERAL`. That is the third in `packages/engine` after the two `cli-agent` `CliMachineState` ones (#2797). The backlog total includes non-columns; a sweep that "converts" them turns a status machine into a workflow role. ## Revert results (measured, each run independently) | conversion | reverted → | | --- | --- | | worktree-owner wip predicate | RENAMED case fails — checkout reads as **free** while another task is live in it | | `resetMergeStateIfNeeded` lanes | RENAMED case fails — card keeps `commitSha: "abc123"` from its previous pass | Both DEFAULT cases pass before and after, which is why both vocabularies run. Each has a non-vacuous companion (holder sitting in the complete lane; a return from the hold lane) so a predicate matching every column would not pass. **Both reach their seam directly through a cast.** The public routes are `handleBranchConflict` (needs a real `BranchConflictError` plus a git repo) and the `task:moved` listener (drags in the whole `execute()` path); going through either would make these tests about a git fixture rather than about the lane predicate. The alternative was the status quo — all 91 `executor-worktree*.test.ts` cases seed `column: "in-progress"`, so they assert the legacy fallback and pass either way. I shipped the conversions in one commit *stating* they were unproven, then closed that gap in the next; the history shows both. ### Two fake defects found while writing those tests Worth naming, because both are the documented green-for-the-wrong-reason shape: 1. The first fake had no `updateTask`, so the cleanup **threw** rather than asserting anything. 2. The second returned a new object without persisting — and `cleanupMergeStateForReverification` **re-reads through `getTask`**. The re-read handed back the stale row, so *both* vocabularies reported "nothing changed" and it would have read as a passing negative test. ## Deliberately NOT converted, with reasons - **The `task:moved` listener cluster** (`3521`/`3545`/`3596`/`3606`), including the AGENTS Move-Task hard-cancel contract `userCanceled: source === "user" && to === "todo"`. Its prologue is synchronous (`userCanceledTaskIds.delete`, watchdog clear) and deferring it to a microtask changes hard-cancel ordering. The sync IR reader is not an option — it returns the DEFAULT workflow for every task in production. A safe conversion needs lanes resolved on an earlier async boundary: new machinery plus an ordering change, which is out of fleet scope and not a guess worth making on a hard-cancel path. - **`17081`** pairs `latestColumn === "in-progress"` with a **hardcoded** `moveTask(taskId, "in-progress")` two lines above — census-invisible, the same shape as the branch-worktree requeue bug in #2797. They have to convert together, and the move needs the same rejection guard. - **`5903`** is the query-filter class: `listTasks({ column: "in-progress" })` followed by a re-assertion of the same literal. Converting it drops a count and changes nothing — see `docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md` (#2800). ## Verification - `pnpm test:gate` — 161 + 487 + 13 + 71, green - `npx tsc -p packages/engine/tsconfig.json --noEmit` — clean - `pnpm lint` — clean - `--strict` exits 0 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
7dc7a41e0e |
fix(tests): the load-lane guard matched a VARIABLE NAME — make it AST-based (#2804)
## A red that no CI run can see Found by pre-flighting #2796 against current `main`. Merged, the engine suite fails: ``` scheduler-load-lane-union.test.ts > the scheduler builds this same union expected 'import {…' to contain '...columnsWithFlag(loadLaneIr, "intake")' ``` **Neither side is red on its own.** `main` is green (10913 passed, 0 failed) and #2796's own CI is green — this test landed on `main` via **#2787**, *after* #2796 was cut, and #2796 does not touch the file. It fails only in the merged state, which is exactly the shape no branch's CI checks. ## #2796 is not at fault The union is still built (`scheduler.ts`, the `columnsWithFlag(ir, ...)` spread). Resolving assignment load per task renamed the local from `loadLaneIr` to `ir`, and the guard hardcoded that name: ```ts expect(source).toContain(`...columnsWithFlag(loadLaneIr, "${flag}")`); ``` It would fail identically on a reformat, a line wrap, or any rename — reporting drift that did not happen. And the reflex fix is to edit the string to match, which protects nothing and teaches nobody anything. ## The fix Parse `scheduler.ts` and collect the string literal passed as the **second** argument to every `columnsWithFlag(...)` call, whatever the first argument is called. Same invariant — every legacy role is unioned somewhere in the scheduler — now actually checked. It also asserts the parse found **something** before checking the six flags. A visitor that matched nothing would make every assertion below it vacuous, which is the specific failure mode this guard family keeps producing. Still structural rather than behavioural, for the reason the file header already gives: the call site sits inside a dispatch path a unit test has no business standing up. The three sibling cases cover the resolver's behaviour; this one covers the wiring. ## Evidence — the discrimination is the right way round | mutation | expected | result | |---|---|---| | rename `ir` → `loadLaneIr` (behaviour identical) | pass | **4/4 passed** | | drop `...columnsWithFlag(ir, "hold")` from the union | fail | **fails**: `scheduler.ts no longer passes "hold" to columnsWithFlag` | Engine **10936 passed / 0 failed** · gate **732 green** · lint clean · engine `tsc --noEmit` **0 errors**. Test-only; `scheduler.ts` restored clean after the mutations. **Unblocks #2796 with no change needed on its side** — commented there. ## Pre-flight results for the rest of the queue Same method (merge with current `main`, run the suites), since batch-engine's earlier landing put 32 failures on main that were only caught post-merge: | PR | result | |---|---| | #2785 batch-engine-tail | clean — 10903 passed | | #2783 batch-core-2 | clean — core 4751 passed; its one api failure is pre-existing on main | | #2797 engine tail | clean — 10941 passed | | #2772 batch-dashboard-app | clean — backfill total 112 → **111**, no lane regresses | | #2796 assignment load | **this failure only** | 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |