Skip to content

docs(service-settings): state getMany's all-or-nothing key validation on the declaration that owns it - #12169

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-11680-getmany-all-or-nothing-doc
Aug 25, 2026
Merged

docs(service-settings): state getMany's all-or-nothing key validation on the declaration that owns it#12169
os-litant merged 1 commit into
mainfrom
claude/issue-11680-getmany-all-or-nothing-doc

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#11680

Documentation and a pin. No behaviour change — the implementation file's
diff is 34 added lines and 0 removed (git diff --numstat), all of them
inside one doc comment.

Routing deviation — read this first

Triage routed this card to domain:spec on the premise that "the
SettingsService.getMany declaration" lives in packages/spec. Measured
against origin/main at 577fabf, it does not:

  • git grep getMany -- packages/speczero hits.
  • packages/spec/src/contracts/ holds 49 non-test contract modules; none is
    a settings service
    , and the only SettingsService mentions anywhere under
    that directory are three prose references inside crypto-provider.ts.
  • The only SettingsService interface declarations in the repo are
    SettingsServicePluginOptions and SettingsServiceOptions, both in
    packages/services/service-settings/ — neither declares getMany.

So the contract that owns getMany is the concrete method's own doc comment in
packages/services/service-settings/src/settings-service.ts — which is exactly
where the issue body pointed. This PR touches nopackages/spec/** path,
so the spec-surface review limb does not apply to it; the card's domain label
is the PM's to correct.

The issue's own premise is intact and was verified line by line before any
edit.

The rule, as measured

getMany checks every requested key against the namespace manifest before
it reads a single env override and before it loads a single row:

constreg=this.registry.get(namespace);if(!reg)thrownewUnknownNamespaceError(namespace);for(constkeyofkeys){if(!reg.scopes.has(key))thrownewUnknownKeyError(namespace,key);}

One undeclared key therefore rejects the whole call — UnknownKeyError,
code: 'SETTINGS_UNKNOWN_KEY' — and the caller receives nothing, not the
subset it was entitled to. N per-key get() calls part ways on exactly that
input: each declared key still answers, and only the undeclared one throws.

The doc comment was otherwise detailed — it explained the grouped row load and
the env-override ordering, and claimed row-for-row equivalence with per-key
get "BY CONSTRUCTION". That claim holds for every key that resolves and not
for the refusal, and the comment did not draw the line. getMany's doc comment
now states the rule, its blast radius, why validating ahead of the grouped walk
makes the refusal independent of key order and scope grouping, and what a caller
against a partial manifest should expect. setMany already pre-flights its
whole patch the same way, and the comment says so.

The pin, and why the existing one was not enough

settings-getmany.test.ts already asserted
await expect(svc.getMany('localization', ['timezone', 'nope'])).rejects.toThrow(/nope/)
— green whatever the blast radius is. The new sibling pin asserts the property
instead:

  • the error envelope (code: 'SETTINGS_UNKNOWN_KEY' plus the message clause).
    This is a service-layer error class carrying code and nostatus
    there is no HTTP boundary here, so code is the whole machine-readable
    envelope there is to assert;
  • zero rows loaded (engine.find called 0 times) with the undeclared key
    placed last in the request — that is the "up-front" half, and it is the
    half the old pin cannot see;
  • the contrast: per-key get() still resolves each declared key on the same
    input, asserted on the cascade layer rather than the literal (this
    fixture stores JSON text in value while the service persists values
    verbatim — see the finding below).

Ablation

Deferred getMany's validation until after the grouped row load, then restored.

  • Resolution path: the test imports ./settings-service.jsrelatively,
    which vitest resolves to src/ — no package exportsdist hop is
    involved, so no rebuild is required for the mutation to take effect. Stated
    because a rebuild-free ablation is the exception, not the default.
  • Mutation confirmed on disk, not from an editor's exit code: the count of
    if (!reg.scopes.has(key)) throw new UnknownKeyError(namespace, key); in the
    file went 3 → 2 (the getMany occurrence gone; get and setMany
    untouched), and the mutated getMany head was dumped and read back.
  • Restore confirmed on disk: injected marker count back to 0, throw
    count back to 3. The script carried trap … EXIT INT TERM, and the trap
    fired.
  • Predicted direction, and what happened: the new pin turned red on
    expect(engine.find).toHaveBeenCalledTimes(0)"expected vi.fn() to be
    called +0 times, but got 2 times"
    — while the pre-existing toThrow(/nope/)
    pin stayed green through the same ablation (6 passed / 1 failed). That is
    the measurement the new pin exists for.
  • One honesty note: the mutation script annotated its marker count as
    "expect 4" where 3 injection sites can only produce 3. The stated expectation
    was wrong, not the observation; the load-bearing on-disk proof is the 3 → 2
    throw count and the head dump, both of which held.

Verification

Union run at 5067b79, after the final commit.

  • pnpm --filter @objectstack/service-settings typecheck (tsc --noEmit,
    script name echoed — not a zero-match pass) and … test29 test files,
    515 tests passed
    .
  • pnpm --filter @objectstack/service-settings exec vitest run src/settings-getmany.test.ts
    7 passed (7).
  • Gate families derived from the real change set with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
    (no hand-built path list), all exit 0: check:nul-bytes,
    check:error-code-casing, check:changeset-gate-self-tests,
    check:objectui-changeset, check:published-files, check:slot-lookup,
    check:test-source-alias, check:type-source-resolution,
    check-adr-0087-registration, check-changeset-no-major,
    check-empty-changeset, check-plugin-teardown-shape,
    release-rehearsal-clone --self-test, docs-audit/check-affected-docs,
    and the convention-triggered set for a test edit —
    check:query-options-erasure, check:engine-double-contract,
    check:where-matcher, check:cross-package-test-inputs,
    check:type-check-coverage.
  • npx eslint . --no-inline-config --format json — the repo-wide run, not a
    narrowing: 5220 files resolved by eslint's own config, 0 errors, 0
    warnings
    . (Type-aware linting is off — the config states "no
    parserOptions.project, no typed @typescript-eslint rules".)
  • Not run locally, declared: check:type-check-debt --re-measure, which
    refuses on an unbuilt worktree and needs the whole workspace closure built —
    a repo-scale run CI owns. docs-audit/check-drift-comment needs PR context.

Out-of-scope finding, not filed

settings-getmany.test.ts's ROWS fixture stores JSON text in value
('"America/New_York"'), but sys_setting.value is a Field.json column and
the service writes storedValue = rawValue and reads return row.value ?? null
— verbatim both ways, with the driver owning the JSON codec. The fake engine
skips that codec and returns rows as written, so every value in this fixture
resolves one JSON encoding deep. It is invisible today because every assertion
in the file compares getMany against get, and both are equally affected —
the same "a fake that lies consistently" shape this file's own header warns
about for $or matchers. It cost one test iteration here and would mislead the
next author who asserts a concrete value.

Not filed as an issue: the dedup search this repo requires before filing goes
through the REST list endpoint, and from this session that endpoint answers
"GitHub access is not enabled for this session" (gh is not installed
either). Reported to the PM instead rather than filed without a duplicate
check.


Generated by Claude Code

… on its own declaration
`SettingsService.getMany` validates every requested key against the namespace
manifest before reading any env override and before loading any row, so one
undeclared key rejects the whole call with `UnknownKeyError`
(`code: 'SETTINGS_UNKNOWN_KEY'`) and the caller gets nothing — not the subset
it was entitled to. N per-key `get()` calls part ways on exactly that input.
The doc comment was otherwise detailed and claimed row-for-row equivalence
with per-key `get` "BY CONSTRUCTION" — true for every key that resolves, and
not for the refusal. It now draws that line, states the blast radius, and
says what a caller against a partial manifest should expect.
Adds a sibling pin that asserts the property rather than the throw: the error
envelope, that zero rows were loaded (undeclared key last in the request), and
that per-key `get()` still answers each declared key. The pre-existing pin
asserted only `rejects.toThrow(/nope/)`, which stays green whatever the blast
radius is — measured: it survived an ablation that deferred validation until
after the row load, while the new pin turned red.
No behaviour change: the implementation file's diff is 34 added lines and 0
removed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDGG54XF5gbTLdQzCtnaVV
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-settings, touching 1 documentable anchor(s).

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

  • content/docs/data-modeling/drivers.mdx(via SettingsService (symbol))
  • content/docs/protocol/kernel/config-resolution.mdx(via SettingsService (symbol))
  • content/docs/protocol/kernel/index.mdx(via SettingsService (symbol))
What this run could not see
  • 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 — 7 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 71f9cd1d753a595d130406d72367c1069274de37packageMentionDocs.

Which tree this was computed on

This run read content/docs from bbd6041a28d7b50ea544a90d795cf532b448a9df — the merge of head 5067b790d95b8f969806d2dc669b9b0f78e368ba into base 71f9cd1d753a595d130406d72367c1069274de37, 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 bbd6041a28d7b50ea544a90d795cf532b448a9df && git checkout bbd6041a28d7b50ea544a90d795cf532b448a9df
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 71f9cd1d753a595d130406d72367c1069274de37 5067b790d95b8f969806d2dc669b9b0f78e368ba && git checkout -B drift-repro 71f9cd1d753a595d130406d72367c1069274de37 && git merge --no-ff 5067b790d95b8f969806d2dc669b9b0f78e368ba
node scripts/docs-audit/affected-docs.mjs --json 71f9cd1d753a595d130406d72367c1069274de37

⚠️ 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 71f9cd1d753a595d130406d72367c1069274de37 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 25, 2026
@os-litant
os-litant marked this pull request as ready for review August 25, 2026 12:28
@os-litant
os-litant added this pull request to the merge queueAug 25, 2026
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Aug 25, 2026
@os-litant
os-litant added this pull request to the merge queueAug 25, 2026
Merged via the queue into main with commit fc8627eAug 25, 2026
34 checks passed
@os-litant
os-litant deleted the claude/issue-11680-getmany-all-or-nothing-doc branch August 25, 2026 13:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SettingsService.getMany's all-or-nothing key validation is undocumented on the contract that owns it

2 participants

@os-litant@claude