From 8d84cee11ec00ca796d2b98dcd45a65f308f3cc9 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 01:32:43 -0700 Subject: [PATCH] fix(core): the workflow-settings identity resolver contradicted its own docs (2 long-red tests) (#2671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second of the four long-red live-PG suites, after #2669. This one is **stale documentation making a stale test look like a code bug** — behaviour is unchanged. ## What was wrong `getWorkflowSettingsProjectIdImpl` documented a three-step resolution order: ``` (a) store.asyncLayer?.projectId — central-registry id (PG) (b) store.db.getProjectIdentity()?.id — legacy SQLite identity (c) store.rootDir — last-resort key ``` The code does (a), then returns `rootDir`. **Step (b) was removed** by `FNXC:SqliteDualPathCleanup 2026-07-26-14:15` — but the doc block kept describing it, and a comment three lines above the return still said *"Only the true legacy (non-backend) path consults the SQLite identity"*, which has been false for every caller since. ## Which side was wrong — settled by construction, not judgement In my triage on #2669 I said I would not guess between "the test is stale" and "the code lost a needed branch", because the two have opposite consequences and the stale comments made the intent unreadable from outside. That was the right call then; it is now answerable: `dbImpl` **throws unconditionally and ignores its store argument** (`task-id-integrity.ts:58`): ```ts export function dbImpl(_store: TaskStore): Database { throw new Error("TaskStore.db: SQLite Database is not available in backend mode …"); } ``` There is no mode in which `store.db` yields a usable SQLite handle. Step (b) is unreachable **by construction**, not merely unused — so the code is right and the documentation was wrong. ## Why the tests passed review originally They build a store double whose `getProjectIdentity()` **returns** a value: ```ts db: { getProjectIdentity() { return { id: "legacy_identity_id" }; } } ``` Production cannot produce that shape. The double made an unreachable branch look testable, which is how the assertion survived the cleanup that deleted the branch. Rewritten to the shipped contract. A neighbouring case that already asserted `rootDir` *when the stub throws* was passing all along — the two forms of the same store disagreed inside one file. ## Verification Suite **7/9 → 9/9**. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0. `tsc -p packages/core/tsconfig.json` clean. `pnpm lint` clean. No changeset: no behaviour change, and no user-visible effect. ## Remaining from the four - ✅ `store-wedge-resolution` — real product bug, fixed in #2669 - ✅ `workflow-settings-project-identity` — this PR - ⬜ `agent-logs-and-monitor` — `expected +0 to be 2` on an aggregation - ⬜ `central-archive-secrets` — an assertion on `warn` arguments Two of four were real problems hiding behind "pre-existing". The other two are still unruled-out. Co-authored-by: Claude Opus 5 (1M context) --- ...kflow-settings-project-identity.pg.test.ts | 16 ++++++++++-- .../src/task-store/branch-and-pr-entities.ts | 26 +++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/packages/core/src/__tests__/postgres/workflow-settings-project-identity.pg.test.ts b/packages/core/src/__tests__/postgres/workflow-settings-project-identity.pg.test.ts index 12cc37755d..158c29db7c 100644 --- a/packages/core/src/__tests__/postgres/workflow-settings-project-identity.pg.test.ts +++ b/packages/core/src/__tests__/postgres/workflow-settings-project-identity.pg.test.ts @@ -208,7 +208,18 @@ describe("getWorkflowSettingsProjectIdImpl resolution order (unit)", () => { }, }, } as unknown as TaskStore; - expect(getWorkflowSettingsProjectIdImpl(store)).toBe("legacy_identity_id"); + /* + FNXC:CentralProjectIdentity 2026-07-31-13:00: + rootDir, NOT the legacy identity — and the store double above is the reason this case used to + expect otherwise. It supplies a `db.getProjectIdentity()` that RETURNS a value, which the real + accessor can never do: `dbImpl` throws unconditionally and ignores its store argument entirely + (`task-id-integrity.ts:58`), so there is no mode in which `store.db` yields a usable SQLite + handle. The legacy-identity branch is unreachable BY CONSTRUCTION, not merely unused. + + So this was asserting behaviour for a store shape that cannot exist, and the resolver's doc block + promising a three-step order was stale in the same way — corrected alongside this. + */ + expect(getWorkflowSettingsProjectIdImpl(store)).toBe("/tmp/root"); }); it("falls back to rootDir when the SQLite stub throws and no layer is bound (old backend behavior)", () => { @@ -234,6 +245,7 @@ describe("getWorkflowSettingsProjectIdImpl resolution order (unit)", () => { }, }, } as unknown as TaskStore; - expect(getWorkflowSettingsProjectIdImpl(store)).toBe("legacy_identity_id"); + // Same reason as above: an unbound layer falls through to rootDir, never to a SQLite identity. + expect(getWorkflowSettingsProjectIdImpl(store)).toBe("/tmp/root"); }); }); diff --git a/packages/core/src/task-store/branch-and-pr-entities.ts b/packages/core/src/task-store/branch-and-pr-entities.ts index e2e7847dfe..0a0758dd1c 100644 --- a/packages/core/src/task-store/branch-and-pr-entities.ts +++ b/packages/core/src/task-store/branch-and-pr-entities.ts @@ -625,8 +625,19 @@ export function getWorkflowSettingsProjectIdImpl(store: TaskStore): string { * central-registry project (e.g. "proj_2f4be0f31a404d2c"). This is the * id the rest of the system partitions by, so workflow settings MUST * key by it too. - * (b) `store.db.getProjectIdentity()?.id` — legacy SQLite identity id. - * (c) `store.rootDir` — absolute filesystem path, last-resort legacy key. + * (b) `store.rootDir` — absolute filesystem path, last-resort key. + * + * FNXC:CentralProjectIdentity 2026-07-31-13:00 (documentation corrected): + * There USED to be a middle step reading `store.db.getProjectIdentity()?.id`, and this block still + * described it long after `FNXC:SqliteDualPathCleanup 2026-07-26-14:15` removed it. It is not + * merely unused — it is unreachable BY CONSTRUCTION: `dbImpl` throws unconditionally and ignores + * its store argument (`task-id-integrity.ts:58`), so `store.db` can never yield a usable SQLite + * handle in any mode. + * + * Two cases in `workflow-settings-project-identity.pg.test.ts` were red on main because they + * asserted that removed step, using a store double whose `getProjectIdentity()` returns a value — + * a shape production cannot produce. Stale documentation is what made that look like a code bug + * rather than a stale test, so both are fixed together. * * BUG this fixes: the old code went straight to (b). In backend mode * `store.db` is a SQLite stub whose `getProjectIdentity()` THROWS @@ -645,10 +656,15 @@ export function getWorkflowSettingsProjectIdImpl(store: TaskStore): string { An unscoped backend store (asyncLayer present but projectId empty) must NOT reach `store.db` below — it throws the removed-SQLite stub, which the catch then swallows, so the throw was invisible. Return the same rootDir key the - swallow produced, without the spurious stub throw. Only the true legacy - (non-backend) path consults the SQLite identity. + swallow produced, without the spurious stub throw. + */ + /* + FNXC:SqliteDualPathCleanup 2026-07-26-14:15: project id for workflow settings is rootDir under PG. + Unconditional on purpose — see the corrected resolution order above. The comment block immediately + before this once said "Only the true legacy (non-backend) path consults the SQLite identity", + which has been false for every caller since that cleanup; it is removed rather than left to + mislead the next reader the way it misled me. */ - /* FNXC:SqliteDualPathCleanup 2026-07-26-14:15: project id for workflow settings is rootDir under PG. */ return store.rootDir; }