Skip to content

fix(service-storage,service-sms): converge the authored-value criterion on ResolvedSettingValue.source - #14135

Merged
os-steve merged 1 commit into
mainfrom
claude/issue-5536-settings-source-convergence
Sep 1, 2026
Merged

fix(service-storage,service-sms): converge the authored-value criterion on ResolvedSettingValue.source#14135
os-steve merged 1 commit into
mainfrom
claude/issue-5536-settings-source-convergence

Conversation

@claude

@claude claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #5536

What this converges

Settings-bound services answered "has anyone actually configured this?" two different ways. plugin-email already answers it correctly, from ResolvedSettingValue.source, with its reason in place: "the manifest default (source: 'default') is not a decision anyone made, and treating it as one would let a settings page nobody opened silently switch off a mode the deployment declared." The two remaining seams answered it by value presence — and the manifest defaults are non-empty on every boot, so an unopened settings page read as configuration. This PR copies the email shape into both seams; no third shape is invented.

  • service-storage (storage-service-plugin.ts): the swap gate used to be "any value non-empty". Manifest defaults (adapter: 'local', local_root: './.objectstack/data/uploads') satisfied that on every boot, so the schema-default target could silently shadow the constructor-built adapter and move the backing store. Now the gate asks whether an adapter-relevant key — exactly the inputs resolveStorageTarget reads: adapter, local_root, and the six s3_* keys — carries source !== 'default'. Per-decision gating is the email shape applied faithfully: an admin save that touched only the upload limit does not author the adapter, so it cannot open the swap gate either.
  • service-sms (sms-plugin.ts): the downgrade to LogSmsTransport used to fire on values.provider === 'log' — which cannot tell an operator's selection from the manifest default 'log'. A deployment that declared a real provider via constructor options had its transport switched off on every boot of an unopened settings page. Now the downgrade additionally requires sources.provider !== 'default'. The non-log rebuild path is untouched (the manifest default is 'log', so a non-log value is always authored).

Admin-saved rows and env overrides behave exactly as before in both seams: 'env', 'global', 'tenant', 'user' all count as authored — an env override is a deployment decision, same as the email precedent treats it.

snapshotOf — measured, and left alone

The dispatch asked for this decided by measurement. Measured on this tree:

  • snapshotOf is a private method of SettingsService (settings-service.ts, private async snapshotOf), called from exactly three sites, all inside createClient (initial snapshot, subscription refresh, manual refresh()); nothing else references it.
  • Both repaired seams call settings.getNamespace(...) directly — the same bypass plugin-email uses — and getNamespace returns full ResolvedSettingValue entries, source included (settings-service.ts builds them with source: effective.scope).

So the two dropping seams are repairable without touching snapshotOf, and it is untouched. createClient consumers still get plain values; widening that snapshot remains a separate decision nobody currently needs.

Clause-②: no. No spec surface touched. Full path list of this diff:

  • packages/services/service-storage/src/storage-service-plugin.ts
  • packages/services/service-storage/src/storage-service-plugin.test.ts
  • packages/services/service-sms/src/sms-plugin.ts
  • packages/services/service-sms/src/sms-plugin.test.ts
  • .changeset/settings-authored-source-convergence.md

packages/spec is not in the diff; ResolvedSettingValue is unchanged; the createClient snapshot carries what it carried before.

Declared exception — the sms daily_quota reader stands

sms-plugin.ts carries a comment that cites #5536 by number and deliberately declines to read source for the daily cost ceiling (#2814): "Read by VALUE, never by ResolvedSettingValue.source — an env-locked OS_SMS_DAILY_QUOTA and an admin-saved row are the same instruction to this reader." The reason still holds: the ceiling is operator policy about the deployment, an env-locked and an admin-saved quota must bind identically, and the schema default (0 = no limit) applied by value is behaviourally identical to not applying it. The code is untouched and the declination is now pinned by a test ("daily_quota still binds by VALUE at source=default"), so the authored-value criterion cannot creep into the quota path unnoticed.

Reverse control — which side is conservative when source is absent, and why

A snapshot with no source (an un-upgraded or non-conforming settings implementation; the spec makes source required, so this is defence in depth) reads as 'default' — i.e. NOT authored — in both seams, via the same ?? 'default' spelling plugin-email uses:

  • storage: the conservative side is the one that does NOT move the backing store. Swapping on unattributable authorship strands every file behind the old adapter; declining to swap keeps files reachable through the deployment-declared adapter, and a real admin save (which always carries source) still triggers the swap via the subscription.
  • sms: the conservative side keeps the deployment-declared transport DELIVERING. The other side's failure mode is silently routing every send to the log transport on nobody's say-so; an operator can still downgrade explicitly, and that save carries source.

Both are pinned by tests named "reverse control".

Evidence — sitting on the disagreement, as dispatched

The old criterion (value non-empty) and the new one (source !== 'default') agree whenever an admin wrote a non-empty value, so the proof sits on the cases where they disagree:

  • Positive (this case IS the card): non-empty schema defaults with source: 'default' — storage: "keeps the constructor adapter when every value is a schema default"; "an authored save that touched no adapter key does not open the swap gate"; sms: "keeps the constructor-declared transport when provider is only the schema default".
  • Controls (declared as controls, green both directions, NOT fix evidence): authored rows behave byte-identically — storage: "swaps the inner adapter when storage settings change" (fake now models source: 'global', which is what that test always meant), "still warns when the backing store really moves"; sms: "still downgrades when an operator actually saved provider=log", plus the daily_quota declination pin.
  • Reverse controls: source absent lands on the conservative side (above) — one test per seam.

Ablation (fix committed first; restore leg points at HEAD): predicted direction declared in the script header before each run — reverting each seam to the old value-presence criterion turns exactly the disagreement tests red and leaves every control green. Observed: storage 3 failed / 13 passed; sms 2 failed / 14 passed — matching the prediction test-for-test. The mutation was proven ON DISK per leg by anchored marker counts (old-criterion text 1 to 0, injected marker 0 to 1) and by git hash-object (storage blob f79514a3 to bd79d9fa; sms blob 9f5b7d5f to 2fe58ea4), never by the editor's exit code; the restore was proven BY STATE (blob hash equal to the HEAD blob again, git diff HEAD empty), with an EXIT/INT/TERM trap on absolute paths as the crash-path backstop. No rebuild sits between mutation and measurement by construction: both suites import the mutated file by same-package relative path, so vitest transpiles the mutated SOURCE directly — dist is not in the loop for the subject under test (its dependencies were built beforehand).

Suites (after the closure build, under the shared verify lock, exit codes captured before any pipe, at commit ae4065b):

  • service-storage full suite: 35 files, 531 tests, all passed.
  • service-sms full suite: 5 files, 74 tests, all passed.

Typecheck, with the debt ledger compared rather than assumed (at ae4065b):

  • sms package tsconfig: 0 errors — and tsc --listFiles shows sms-plugin.ts IN that program while sms-plugin.test.ts is NOT (the package excludes test files), so that green says nothing about the test file and is not claimed to.
  • sms TEST_DEBT replication (package tsconfig with the test exclusion lifted, the gate's own method): exactly 1 error, the ledgered TS2493 in transports.test.ts — ledger says 1; my edited test file contributes zero. --listFiles on the lifted program shows both edited sms files present.
  • storage package tsconfig: 51 errors — exactly the ledgered DEBT count of 51 (all pre-existing families: TS2835/TS7006/TS2347/TS2339/TS2550/TS6196/TS6133); my edits contribute zero drift. --listFiles shows both edited storage files in the program.

Gate families, re-derived from the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (no path arguments; both output sections read whole), and the harvested union of all 36 commands run at git rev-parse --short HEAD = ae4065b (the final commit; the tree is byte-identical to what CI will check out): 33 green — including check:i18n after building its self-named prerequisite closure, check:i18n-stale-fill, check:engine-double-contract, check:where-matcher, check:query-options-erasure, check:type-check-coverage, check:cross-package-test-inputs, check:test-source-alias, check:nul-bytes — and 3 locally NOT MEASURED (prerequisite refusals, not reds, all exit 3 or self-declared "nothing was checked"):

  • check:dual-build-cjs-loads — wants the entire workspace built; this diff changes no build wiring (no package.json, tsup, or export-map edits), and CI's Build Core runs it authoritatively.
  • check-test-completeness — parses CI shard summaries that do not exist locally.
  • check:type-check-debt full form — refuses on a partially built workspace. Narrowed replication above: both touched packages re-measured against their ledger entries with exact matches (51 == 51; 1 == 1 with the identical error identity). Untouched packages cannot drift from this diff: it exists only inside these two packages and changes no exported type surface (a private method signature and local constants).

ESLint, narrowed with the three-part proof instead of the repo sweep: the four changed .ts files lint clean under --no-inline-config (0 errors, 0 warnings; the changeset .md is refused by the config as unconfigured, i.e. outside the population); file count 4 read from --format json; and untouched files cannot move because this repo's eslint.config.mjs never enables type-aware linting for any file (its own in-file measurement, around line 328), so per-file verdicts depend only on that file's content plus a config this diff does not touch.

Out of scope, honoured

Generated by Claude Code


Generated by Claude Code

…on on ResolvedSettingValue.source

A schema default (source: 'default') is not a decision anyone made. The
storage swap gate now requires an authored adapter-relevant key before
settings may override the constructor-built adapter; the sms downgrade to
LogSmsTransport now requires an operator-authored provider selection.
Same criterion, same reason as EmailServicePlugin. The sms daily_quota
reader keeps its declared by-value exception.

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

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

9 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

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 — 8 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 bf3bbf13b9be4f3996726a239cfe5642395c2396packageMentionDocs.

Which tree this was computed on

This run read content/docs from 7d8810460e00b8b7bb83e568f3c5801d06706a5e — the merge of head ae4065b12b884cbbec49eea9b960c14139e6f52f into base bf3bbf13b9be4f3996726a239cfe5642395c2396, 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 7d8810460e00b8b7bb83e568f3c5801d06706a5e && git checkout 7d8810460e00b8b7bb83e568f3c5801d06706a5e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bf3bbf13b9be4f3996726a239cfe5642395c2396 ae4065b12b884cbbec49eea9b960c14139e6f52f && git checkout -B drift-repro bf3bbf13b9be4f3996726a239cfe5642395c2396 && git merge --no-ff ae4065b12b884cbbec49eea9b960c14139e6f52f

node scripts/docs-audit/affected-docs.mjs --json bf3bbf13b9be4f3996726a239cfe5642395c2396

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

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@os-steve
os-steve marked this pull request as ready for review September 1, 2026 09:34
@os-steve
os-steve enabled auto-merge September 1, 2026 09:34
@os-steve
os-steve added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 6426b86 Sep 1, 2026
34 checks passed
@os-steve
os-steve deleted the claude/issue-5536-settings-source-convergence branch September 1, 2026 09:57
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.

settings 消费缝丢弃 ResolvedSettingValue.source —— 服务无法区分「管理员写过的值」和「schema 默认值」

2 participants