Skip to content

fix(cli,service-storage): stop re-pointing the storage adapter every boot — and mean it when it warns (#4096) - #4166

Merged
os-zhuang merged 1 commit into
mainfrom
claude/plugin-boot-logs-visibility-12kkwh
Jul 30, 2026
Merged

fix(cli,service-storage): stop re-pointing the storage adapter every boot — and mean it when it warns (#4096)#4166
os-zhuang merged 1 commit into
mainfrom
claude/plugin-boot-logs-visibility-12kkwh

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Closes #4096.

The issue's premise was half right — the warning was telling the truth

#4096 reports this on every clean boot and calls it spurious:

WARN StorageServicePlugin: storage adapter swapped (LocalStorageAdapter →
LocalStorageAdapter). Existing files were NOT migrated and may be unreachable
through the new adapter.

I built the "nothing changed, so don't swap" fix first — and the warning survived it. A probe at the decision point said why:

values.local_root = "./.objectstack/data/uploads"
current  = local://./storage                        ← what the plugin was running
next     = local://./.objectstack/data/uploads      ← what settings said

The backing store really was moving on every boot. The adapter was starting in the wrong directory and being corrected seconds later, and the warning was an accurate report of a real misconfiguration nobody could act on.

Root cause: option keys nothing reads

serve constructed the plugin with { driver: 'local', root }. StorageServicePluginOptions declares neither key — it takes { adapter, local: { rootDir } }. Both were dropped on the floor, so:

  • the plugin fell back to its own ./storage default;
  • OS_STORAGE_ROOT did nothing — a documented env var with no effect;
  • uploads landed in ./storage, not the .objectstack/data/uploads/ the code comment promises;
  • the storage settings namespace then corrected the root on its first read (its manifest default at storage.manifest.ts:38 is ./.objectstack/data/uploads), swapping the adapter and warning — forever.

A plain object literal passed to a parameter without an index signature type-checks fine and does nothing at runtime. Nothing was going to catch this except reading it.

Three defects, three fixes

  1. serve passes what the plugin reads{ adapter: 'local', local: { rootDir } }. Extracted as resolveStorageCapabilityArg so the option shape is pinned by tests, including an explicit not.toHaveProperty('driver'|'root').
  2. A swap is skipped when nothing changed. The plugin records what the running adapter points at and compares resolved configurations, instead of rebuilding whenever the settings namespace held any value — true on every boot once that namespace persists its own defaults.
  3. The warning means what it says. It fires when the backing store moved (kind change, different root, different bucket/region/endpoint) — not merely when the adapter object was replaced. A credential rotation swaps so the new key takes effect and logs at info: same bucket, nothing stranded. A swap() from a caller that resolved no target still warns — ignorance must not silence the one message that exists to be impossible to miss.

Path spellings are normalised, so the platform writing one default two ways (./.objectstack/data/uploads in the settings manifest, .objectstack/data/uploads in the CLI) is not read as a migration between a directory and itself.

The fingerprint that decides "did anything change" includes S3 credentials and is never logged; the location that goes into the warning text deliberately excludes them, and a test asserts it.

Verification

examples/app-todo, real boot:

before after
Boot-diagnostics warnings 4 3 — storage line gone
adapter swapped occurrences 1 0
./storage created yes no — uploads go to the intended root

Tests

  • 19 unit cases on the target resolver and the swap/warn split, weighted toward the distinctions that matter: credential rotation (swap, don't warn), basePath change (URLs move, bytes don't), kind change, same bucket on a different endpoint, ./x vs x, and the unknown-caller case that must still warn.
  • 4 plugin-level cases pinning what a boot does and says — including that a quiet boot does not disarm the next genuine migration.
  • 7 on the CLI option shape.

Confirmed to fail against the pre-fix code (3 plugin cases fail, one with the exact adapter swapped (LocalStorageAdapter → LocalStorageAdapter) text). service-storage 19 files / 241 tests; cli 89 files / 917 tests; eslint clean; tsc error count unchanged from main (42 pre-existing, none added).

Left alone deliberately

config.storage authored with the driver/root dialect is still forwarded verbatim and still not read by the plugin — the same mismatch one layer up, so a host writing { driver: 's3', … } silently gets local disk. Fixing it means deciding whether the plugin accepts that dialect or the config schema is wrong; a lenient alias here would fossilize the wrong contract (AGENTS.md Prime Directive #12), so it is filed separately rather than papered over. The helper's doc comment says so at the call site.


Generated by Claude Code

…boot — and mean it when it warns (#4096)

Every `os dev` / `os serve` boot printed "storage adapter swapped
(LocalStorageAdapter → LocalStorageAdapter). Existing files were NOT migrated
and may be unreachable through the new adapter."

The warning was telling the truth. `serve` constructed the plugin with
`{ driver: 'local', root }` and `StorageServicePluginOptions` declares neither
key, so both were dropped silently: the plugin applied its own `./storage`
default, OS_STORAGE_ROOT changed nothing, and uploads landed in a directory
nobody named. The `storage` settings namespace then corrected the root on its
first read — its manifest default is `./.objectstack/data/uploads` — genuinely
moving the backing store, every boot, forever.

Three defects, three fixes:

- serve passes options the plugin reads: `{ adapter: 'local', local: { rootDir } }`.
  OS_STORAGE_ROOT takes effect and local uploads land under
  `.objectstack/data/uploads` from the first byte. Extracted as
  `resolveStorageCapabilityArg` so the option SHAPE is pinned — a mismatch like
  this type-checks fine as an argument and does nothing at runtime.
- A swap is skipped when nothing changed. The plugin records what the running
  adapter points at and compares resolved CONFIGURATIONS, rather than rebuilding
  whenever the settings namespace held any value at all (true on every boot once
  that namespace has persisted its own defaults).
- The warning means what it says: it fires when the BACKING STORE moved, not
  when the adapter object was replaced. A credential rotation swaps so the new
  key takes effect and logs at info — same bucket, nothing stranded. A swap from
  a caller that resolved no target still warns; ignorance must not silence it.

Path spellings are normalised, so the platform writing one default two ways
(`./.objectstack/data/uploads` vs `.objectstack/data/uploads`) is not read as a
migration between a directory and itself.

Verified on examples/app-todo: boot diagnostics went from four warnings to
three, the storage line is gone, and `./storage` is no longer created. 19 unit
cases on the target resolver and the swap/warn split, 4 plugin-level cases on
what a boot does and says, 7 on the CLI option shape. service-storage 241 tests,
cli 917 tests, eslint clean, tsc error count unchanged from main.

`config.storage` authored with the `driver`/`root` dialect is still forwarded
verbatim and still unread by the plugin — the same mismatch one layer up, filed
separately rather than papered over with a lenient alias here (Prime Directive
#12).

Closes #4096

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HrRNgrWaRtggzmrHpbomyh
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Jul 30, 2026 1:15pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/cli, packages/services.

22 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/skills-reference.mdx (via packages/cli)
  • content/docs/api/client-sdk.mdx (via @objectstack/cli)
  • content/docs/api/data-flow.mdx (via @objectstack/cli)
  • content/docs/api/environment-routing.mdx (via @objectstack/cli)
  • content/docs/api/error-catalog.mdx (via @objectstack/cli)
  • content/docs/automation/hook-bodies.mdx (via packages/cli)
  • content/docs/automation/webhooks.mdx (via packages/services)
  • content/docs/deployment/backup-restore.mdx (via @objectstack/cli)
  • content/docs/deployment/cli.mdx (via @objectstack/cli)
  • content/docs/deployment/self-hosting.mdx (via @objectstack/cli)
  • content/docs/getting-started/your-first-project.mdx (via @objectstack/cli)
  • content/docs/kernel/runtime-services/audit-service.mdx (via packages/services)
  • content/docs/kernel/runtime-services/data-service.mdx (via packages/cli)
  • content/docs/kernel/runtime-services/index.mdx (via packages/cli, packages/services)
  • content/docs/kernel/runtime-services/settings-service.mdx (via packages/services)
  • content/docs/permissions/authentication.mdx (via @objectstack/cli)
  • content/docs/plugins/packages.mdx (via @objectstack/cli, packages/services)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/services)
  • content/docs/protocol/kernel/plugin-spec.mdx (via @objectstack/cli)
  • content/docs/protocol/kernel/realtime-protocol.mdx (via @objectstack/cli)
  • content/docs/releases/implementation-status.mdx (via @objectstack/cli)
  • content/docs/releases/v16.mdx (via @objectstack/cli)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Copy link
Copy Markdown
Contributor Author

Checked the docs-drift advisory rather than waving it through, and it turned up something that belongs in the record: no doc needs changing, because the docs were already right and the code was wrong. Three of them describe exactly the behaviour this PR restores.

doc what it promises what actually happened before this PR
content/docs/deployment/environment-variables.mdx:54 OS_STORAGE_ROOT … default ./.objectstack/data/uploads, "used by os serve's default storage capability wiring" the value was discarded — the var had no effect at all
content/docs/deployment/backup-restore.mdx:79 "The local storage adapter keeps uploads under OS_STORAGE_ROOT (default ./.objectstack/data/uploads)" uploads landed in ./storage
content/docs/deployment/backup-restore.mdx:22 .objectstack/…/data/uploads/"Include in file backups on single-host deployments" that directory was never written to, so following the documented procedure backed up none of the uploaded files

The third row is the one worth pausing on: a documented backup procedure that silently captured nothing, on the exact deployment shape it is written for. The only visible symptom was the boot warning in #4096 — which, as the PR description covers, was accurate and was reporting this.

That also settles the direction question. { adapter, local: { rootDir } } is not a preference: it is what the documented contract requires, so fixing the producer (as here) rather than teaching the plugin the driver/root dialect is the correct half to move. The remaining config.storage mismatch is #4167, and this is a further argument for resolving that one as "the plugin's shape is canonical, reject the other dialect at authoring" rather than aliasing it.

No documentation edits in this PR — the fix makes the existing pages true.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review July 30, 2026 13:43
@os-zhuang
os-zhuang merged commit efcd68c into main Jul 30, 2026
17 checks passed
@os-zhuang
os-zhuang deleted the claude/plugin-boot-logs-visibility-12kkwh branch July 30, 2026 13:43
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/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

StorageServicePlugin warns "storage adapter swapped (LocalStorageAdapter → LocalStorageAdapter) … files may be unreachable" on every clean boot

2 participants