fix(core): the workflow-settings identity resolver contradicted its own docs (2 long-red tests) (#2671)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user