Skip to content

refactor(cli): drop the dead { records } normalizer from os secret orphans, and make the driver port state the array contract - #15093

Merged
os-trump merged 1 commit into
mainfrom
claude/issue-14843-orphans-records-normalizer
Sep 3, 2026
Merged

refactor(cli): drop the dead { records } normalizer from os secret orphans, and make the driver port state the array contract#15093
os-trump merged 1 commit into
mainfrom
claude/issue-14843-orphans-records-normalizer

Conversation

@os-trump

@os-trump os-trump commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #14843

os secret orphans wrapped both of its driver reads in a local rowsOf() that unwrapped a data envelope, lifted a bare row into a one-element list, and filtered out non-object entries. Every limb was unreachable. It is removed, and both call sites now read the arrays their driver returns.

Type spellings in this body are written in words — "a Promise of an array of row records" rather than the angle-bracket form. That is not style: GitHub's body sanitizer deletes generic-looking angle-bracket fragments outright, code fences included, and a silently emptied span is worse than a wordy one.

The card's caveat was the whole job, so the concrete driver was read

Triage made the filing's own caveat binding and refused to let the removal follow from IDataDriver.find being declared to resolve to an array of string-keyed unknown records — because the counter-case is real. The console's ObjectStackAdapter.find() resolves to a normalized QueryResult envelope and never to an array. Two methods spelled find, opposite answers. So secretDriver was resolved to its concrete implementation instead of inferred.

Static resolution. stack.kernel.getService('objectql') is the ObjectQL engine; getDriverForObject(name) calls getDriver(), which returns the instance held in the engine's drivers map with nothing interposed (registerDriver stores the instance as given). sys_secret declares no datasource binding, so it resolves to the default driver, which the standalone stack builds through createDefaultDatasourceDriverFactory. That factory can build exactly five things, and they are every IDataDriver implementation in this tree:

driver find terminal shape
SqlDriver sql-driver.ts:5611, delegating to findRows array on every path: an empty array at the recovery ladder's terminal, an empty array when the builder answered a non-array, else the row array
SqliteWasmDriver inherited — it extends SqlDriver with no find override same
TursoDriver turso-driver.ts:644 remote: formatRemoteRows(...) over RemoteTransport.find, whose every return is a mapRows(...) map or an empty array; local: super.find
MongoDBDriver mongodb-driver.ts:333 an empty array, or the cast result array
InMemoryDriver memory-driver.ts:556 results, an array from its first assignment through every branch

Runtime reading, which is what the card actually asked for. orphans.driver-contract.test.ts boots the stack this command boots — bootSchemaStack with the command's own extraPlugins list — against a real sqlite file, and reads the seam:

  • getDriverForObject('sys_secret') and getDriverForObject('sys_setting') both resolve com.objectstack.driver.sql, a SqlDriver;
  • both reads answer a bare array with the seeded row as a direct element — asserted together with the row, because an Array.isArray check alone is satisfied by a driver that answers an empty array to everything, which is the reading that would make this removal look safe while the command silently reported nothing;
  • a data property and a records property are asserted absent on the value itself;
  • an empty read answers an empty array, not a nullish value, which is the normalizer's if (!result) limb.

⇒ The premise holds. The limb was dead.

The second file, declared

Beyond orphans.ts and its test, this PR changes one line plus its doc comment in packages/cli/src/utils/secret-reference-union.ts: SecretReferenceDriverLike.find is narrowed from a Promise of unknown to a Promise of an array of string-keyed unknown records.

That port is where the normalizer came from, and it is the contract-first half of this change rather than a convenience. The sentence directly above that declaration already read "Matches IDataDriver.find" — which declares an array — so the declaration and its own comment disagreed, and the comment was the correct one. A caller that cannot see an array in the type writes its own answer for envelope shapes no producer emits, and that is exactly what orphans.ts did. Fixing it at the consumer instead (a cast, or a restated local port) would have kept the drift and added a second de-facto spelling of one contract.

Blast radius, measured: the union's own three reads each declare an explicit unknown local for the result and are untouched; the port's other implementors are the real ObjectQL (through getDriverForObject, already an IDataDriver) and the two hand-built engine slices in secret-reference-union.test.ts and sys-secret-orphan-sweep.test.ts, both of which delegate to the real engine. All compile and pass unchanged.

Nothing else is touched. ⛔ scripts/check-react-page-adapter-contract.mjs and its population are untouched, per triage's fence.

Both call sites, before and after

Reverse verification from the committed state, both source files restored from origin/main for the BEFORE leg, with the mutation confirmed on disk by anchored greps in both directions before anything was measured, and the restore confirmed by blob hash against HEAD plus an empty git diff HEAD:

orphans.ts: injected(rowsOf call site)=1  removed(new comment)=0
union.ts:   injected(loose port return)=1  removed(new comment)=0
mutation confirmed on disk
BEFORE  Test Files 2 passed (2) | Tests 16 passed (16)   BEFORE_VITEST_EXIT=0
orphans.ts after restore = aca253f56f72539512baff2d1db651b50395df0d   (equals the HEAD blob)
union.ts   after restore = dc6bf38080d65449ac1a7b0526f71118bc841017   (equals the HEAD blob)
restore confirmed: both blobs byte-identical to HEAD, git diff HEAD empty
AFTER   Test Files 2 passed (2) | Tests 16 passed (16)   AFTER_VITEST_EXIT=0

The end-to-end half is inside those 16: the command is run for real, with --json, against the same database, and the report is checked for a value that could only have travelled through each of the two former rowsOf call sites — counts.total and the seeded row id from the sys_secret read, and legacyInlineRows from the sys_setting read, which is derived from settingRows and from nothing else. Identical on both legs.

Behaviour

Unchanged for any driver that keeps the contract. What changes is what happens if one ever does not: the command fails loudly instead of dropping the row. That is the right direction here — a sys_secret row dropped from this read is a row dropped from the report, and this command's whole safety property is that no row goes unmentioned. A driver answering something other than an array is a contract violation to fix at that driver, not to absorb at a consumer.

Verification

At d4a66d4bd2 (final HEAD), clean tree:

  • pnpm --filter @objectstack/cli typecheck — exit 0. That is tsc --noEmit plus check:test-typecheck, which reported "OK — @objectstack/cli's test layer compiles under packages/cli/tsconfig.test.json; 3 file(s) / 28 error(s) / 6 pinned signature(s) held", unchanged. The new test lives under src/, so it is inside the build config's src include and really is typechecked — the sibling test/ tree that config never reaches is a separate layer, covered by the second command.
  • pnpm --filter @objectstack/cli exec vitest run --maxWorkers=2 src/commands/secret/ src/utils/secret-reference-union.test.ts src/utils/sys-secret-orphan-sweep.test.tsTest Files 4 passed (4), Tests 62 passed (62).
  • Gate union derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack at d4a66d4bd2 on a clean tree: 42 families. The ones this diff actually moves were run locally and are green, each exit code captured after a redirect and never across a pipe: check:nul-bytes (8199 files, no raw control bytes), check:engine-double-contract, check:where-matcher, check:objectql-double-limit, check:test-source-alias, check:cross-package-test-inputs, check-undeclared-dep-imports, check:published-files, check:changeset-gate-self-tests, check-empty-changeset, check-changeset-no-major, check-adr-0087-registration, check:objectui-changeset, check:dispatcher-error-vocabulary, check:query-options-erasure, check:type-check-coverage, check-keyed-text-bounds, check-closing-keyword-parity, check-comment-mask-adoption, and check:i18n after building its declared prerequisite closure ("OK — 9 package(s), all bundles in sync, no undeclared authoring keys"). The rest of the 42 is CI's farm.
  • Two families are NOT MEASURED here, reported as such rather than as passes or as reds. check:i18n-coverage exits 3 — COULD NOT MEASURE: 1 of its 13 configs (examples/app-showcase/objectstack.config.ts) could not be linted because @objectstack/connector-mcp has no build output in this worktree, and the gate refuses to judge its ratchet on a partial round. That is a worktree build-state gap in a package this diff does not touch, and clearing it needs a build across the example apps, which is CI's to run.
  • scripts/pm/check-half-states.mjs exits 3 — PREREQUISITE NOT MET in this container (it cannot reach the trigger-file index). Reported as NOT MEASURED, not as a pass; it is a PM-board gate and reads nothing this diff changes.
  • ESLint: this repo's config enables no type-aware linting anywhere (parserOptions.project and projectService occur zero times in eslint.config.mjs, and the config's own header records the positive-control measurement for that), so a per-file run is a complete reading for those files and this diff cannot move the verdict on a file it does not touch. pnpm exec eslint --no-inline-config --format json over the three changed TypeScript files: 3 of 3 files linted — an out-of-population file would have come back as an ignore warning instead — with 0 errors, 0 warnings, exit 0. The repo-wide pnpm lint is CI's run; this narrowing is declared rather than silently taken.
  • Every build and test ran through scripts/pm/os-verify-lock.sh, with verdicts read off its VERDICT line rather than a bare shell status.

Out of scope, filed rather than ridden in

#15092 records a THIRD instance of the same class, found while doing this: rowsOf in packages/cli/src/utils/secret-reference-union.ts:295, over three driver reads. It is deliberately not fixed here — it is a different judgement (that module's contract is that a read which did not happen is a gap, and its filter would silently drop a malformed row rather than gap the family, a defect pointing the other way), and triage fenced this card to one file.

It is also the evidence #14460's ruling said would be needed before the domain:devx gate-population card could be argued: three instances now, in three files and three kinds of caller — an app-showcase job handler, a CLI command, and a CLI utility module — all under a gate whose population is app-showcase page modules plus content/docs react-page samples. ⛔ No gate change is proposed here or there; that call belongs to the domain:devx card, and #15092 is unassigned and ungraded.

🤖 Generated with Claude Code

https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza


Generated by Claude Code

… orphans`

`os secret orphans` wrapped both of its driver reads in a local `rowsOf()` that
unwrapped `{ data: [...] }`, lifted a bare row into `[row]` and filtered out
non-object entries. Every limb was unreachable.

The card that asked for this refused to let unreachability be inferred from
`IDataDriver.find`'s declaration, because the counter-case is real: the
console's `ObjectStackAdapter.find()` resolves to a normalized `QueryResult`
envelope and never to an array. So the concrete driver was read instead.
`secretDriver` resolves through `ObjectQL.getDriverForObject('sys_secret')`,
which hands back a registered driver instance unwrapped; the five
`IDataDriver` implementations in this tree — `SqlDriver` (and
`SqliteWasmDriver`, which extends it without overriding `find`), `TursoDriver`
local and remote, `MongoDBDriver`, `InMemoryDriver` — return an array on every
path they can return on, `[]` included. Driven for real, the CLI's own boot
resolves `com.objectstack.driver.sql` for both objects and both reads answer a
bare array holding the seeded rows.

`SecretReferenceDriverLike.find` is narrowed from `Promise<unknown>` to
`Promise<Record<string, unknown>[]>` in the same change. That port is where the
normalizer came from: its own doc comment already said it matched
`IDataDriver.find`, and the return type said otherwise, so every caller that
could not see an array in the type wrote its own answer for shapes no producer
emits. The port now states the contract it claimed to state. The union's three
reads keep their explicit `let result: unknown` locals and are untouched.

Behaviour is unchanged for any driver that keeps the contract. One that does
not now fails loudly rather than having a row silently dropped — and a
`sys_secret` row dropped from this read is a row dropped from the report, which
this command exists to prevent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza
@github-actions github-actions Bot added the size/m label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 5 documentable anchor(s).

10 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/webhooks.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/data-modeling/drivers.mdx (via sys_secret (literal, a string literal in SecretOrphans), sys_setting (literal, a string literal in SecretOrphans))
  • content/docs/data-modeling/external-datasources.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/data-modeling/objects.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/data-modeling/validation-rules.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/deployment/backup-restore.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/deployment/cli.mdx (via sys_secret (literal, a string literal in SecretOrphans), sys_setting (literal, a string literal in SecretOrphans), os secret orphans (command, read off packages/cli/src/commands/secret/orphans.ts))
  • content/docs/deployment/environment-variables.mdx (via sys_secret (literal, a string literal in SecretOrphans), sys_setting (literal, a string literal in SecretOrphans))
  • content/docs/permissions/authorization.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/protocol/kernel/config-resolution.mdx (via sys_secret (literal, a string literal in SecretOrphans), sys_setting (literal, a string literal in SecretOrphans))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/releases/v16.mdx (via sys_secret (literal, a string literal in SecretOrphans))
  • content/docs/releases/v17.mdx (via sys_secret (literal, a string literal in SecretOrphans), sys_setting (literal, a string literal in SecretOrphans))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 22 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 6665c5c916caf6d95022734328ae82154e1a5f10packageMentionDocs.

Which tree this was computed on

This run read content/docs from 665045de06845c96bed98a98e158849a50a83289 — the merge of head d4a66d4bd22633401a2b6ec52bfe14fc33b117e3 into base 6665c5c916caf6d95022734328ae82154e1a5f10, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 665045de06845c96bed98a98e158849a50a83289 && git checkout 665045de06845c96bed98a98e158849a50a83289
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6665c5c916caf6d95022734328ae82154e1a5f10 d4a66d4bd22633401a2b6ec52bfe14fc33b117e3 && git checkout -B drift-repro 6665c5c916caf6d95022734328ae82154e1a5f10 && git merge --no-ff d4a66d4bd22633401a2b6ec52bfe14fc33b117e3

node scripts/docs-audit/affected-docs.mjs --json 6665c5c916caf6d95022734328ae82154e1a5f10

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 6665c5c916caf6d95022734328ae82154e1a5f10 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 3, 2026
@os-trump
os-trump marked this pull request as ready for review September 3, 2026 21:45
@os-trump
os-trump enabled auto-merge September 3, 2026 21:45
@os-trump
os-trump added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit 7ad3d4e Sep 3, 2026
35 checks passed
@os-trump
os-trump deleted the claude/issue-14843-orphans-records-normalizer branch September 3, 2026 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] A second dead { records } normalizer: packages/cli/src/commands/secret/orphans.ts wraps an IDataDriver.find() the contract types as an array

2 participants