Skip to content

fix(settings): verify the repoint before reaping a rotated secret (#8262) - #8680

Merged
qq9340100 merged 3 commits into
mainfrom
claude/issue-8262-reaper-verify-not-infer
Aug 14, 2026
Merged

fix(settings): verify the repoint before reaping a rotated secret (#8262)#8680
qq9340100 merged 3 commits into
mainfrom
claude/issue-8262-reaper-verify-not-infer

Conversation

@qq9340100

Copy link
Copy Markdown
Collaborator

Fixes#8262

The defect, reproduced before it was fixed

SettingsService.reapRotatedSecret deleted the handle upsertRow reported as
previousEnc, and inferred that the repoint it was cleaning up after had taken
effect from previousEnc !== nextEnc. That inference holds for the shipped
adapter, which forwards context: { isSystem: true }. It does not hold for an
adapter that drops context — the reader SettingsEngine's own doc comment
contemplates, and a documented extension point rather than a mistake nobody
makes.

With context dropped, sys_setting.value_enc is readonly: true, so the
UPDATE has it stripped and the row keeps naming the OLD handle. The reaper then
deleted the ciphertext still in force: materialiseRow dereferenced a
dangling handle, got nothing, and the setting silently read as empty —
unrecoverably, since the audit trail records digests rather than handles or
ciphertext.

The card's table, reproduced on today's main against a real ObjectQL over
the real SysSetting / SysSecret schemas, before any source change:

AssertionError: expected [ 1, 1, 2 ] to deeply equal [ 1, 2, 3 ]
AssertionError: expected false to be true // the in-force ciphertext was gone
AssertionError: expected '' to match /did NOT take effect/ // and it said nothing

The tests were written first and run against unmodified source, so their
red-to-green transition is the anti-vacuity evidence rather than a post-hoc
ablation.

The fix

The reaper re-reads the row after the write and deletes previousEnc only once
storage confirms the row no longer names it.

The criterion is current !== previousEnc, not the narrower
current === nextEnc the card suggested. Both refuse the case that matters (the
row still naming previousEnc), but under a concurrent rotation the row may
already have moved on to a third handle — where previousEnc is genuinely
unreferenced and the narrower test would leak exactly the orphan the reaping
exists to prevent. The card labelled its direction a suggestion, not a ruling;
this keeps its substance (verify, do not infer) with the wider-of-the-two safe
tests.

Three properties held deliberately:

  • The write is never failed by it. The call site's constraint ("never
    allowed to fail the write") now covers the read as well as the delete. Every
    failure branch — read throws, row unreadable, row still naming the handle —
    logs and returns.
  • Every refusal leaves an orphan, which is the recoverable direction and the
    one an orphan sweep can clean up. There is no recoverable direction on the
    other side.
  • The read is paid only where a destructive delete would otherwise follow.
    It sits behind all four existing cheap guards, so a rotation that cannot reap
    anything issues no extra I/O.

The failure also stops being silent: a non-forwarding adapter now produces a
server-side error naming the row, the refusal and the cause. That is the only
signal available — the HTTP response still says the write landed, because the
strip happens a layer below and nothing at this layer can make such an adapter
correct. What this change buys is that the failure is survivable and visible.

The stale doc sentence, which rides the fix

SettingsEngine.update's warning still described only the pre-#8063
consequence ("the rotated-away credential stays in force"), so an adapter author
reading it would not learn the failure had become data loss. It now states the
real consequence, records the #8063-to-#8262 window explicitly (an adapter
written against that window's docs is exposed to the destructive form), and
keeps the MUST-forward requirement unweakened.

Scope

⛔ No #8259 rider: the orphan-sweep decision is orthogonal to this defect
(sweeping old orphans vs. the reaper deleting a live value) and nothing here
touches it. #8103 is likewise untouched — it remains open by design.
packages/services/service-datasource (sibling card #8154) is not touched.

premise_still_valid: true — the reaper, its call site and the stale sentence
were all where the card said, allowing for today's line drift.

Verification

Union re-run after the final commit, at e901161ba:

check:query-options-erasure was genuinely red at first — the verification read
had picked up an as any, growing that file's erasure count 2 to 3. Fixed at
the source by narrowing rowIdentity's bypass type, so the new read goes
through the declared SettingsEngine.find options type. The count is back to
baseline.

The two engine doubles this PR adds are pinned to the real dispatch contract via
assertEngineUpdateDispatch, matching the existing double in the same package.


Generated by Claude Code

)
`reapRotatedSecret` deleted the handle `upsertRow` reported as `previousEnc`
and inferred that the repoint had taken effect from `previousEnc !== nextEnc`.
That inference fails on a `SettingsEngine` adapter that drops `context`: the
readonly `value_enc` is stripped from the non-system UPDATE, the row keeps
naming the old handle, and the reaper destroyed the ciphertext STILL IN FORCE
— leaving a dangling `value_enc` that reads as empty, unrecoverably.
Re-read the row and delete only once storage confirms it no longer names the
handle. Refusals leave an orphan (recoverable) and are logged loudly, so a
non-forwarding adapter now announces itself instead of silently losing values.
The verification read sits behind every cheap guard and inside the reaper's
existing "never fail the write" guarantee.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MX1qcBzfwZb5wkRrJTNbhH
…s type (#8262)
`rowIdentity`'s `bypass` is typed `{ bypassTenantAudit?: true }` rather than
`Record<string, unknown>`, so the reaper's re-read spreads into a
`SettingsEngine.find` options object without an `as any`. The
query-options-erasure ratchet caught the erasure as a new site — correctly:
the verification read's whole value is that it goes through the declared
contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MX1qcBzfwZb5wkRrJTNbhH
@vercel

vercelBot commented Aug 14, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 14, 2026 1:10pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/kernel/runtime-services/audit-service.mdx(via packages/services/service-settings)
  • content/docs/kernel/runtime-services/index.mdx(via packages/services/service-settings)
  • content/docs/kernel/runtime-services/settings-service.mdx(via packages/services/service-settings)
  • content/docs/plugins/packages.mdx(via @objectstack/service-settings)
  • content/docs/protocol/kernel/config-resolution.mdx(via @objectstack/service-settings)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/service-settings)
  • content/docs/releases/v9.mdx(via @objectstack/service-settings)

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.

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.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 14, 2026
@qq9340100
qq9340100 marked this pull request as ready for review August 14, 2026 13:53
@qq9340100
qq9340100 added this pull request to the merge queueAug 14, 2026
Merged via the queue into main with commit 501ed0eAug 14, 2026
27 checks passed
@qq9340100
qq9340100 deleted the claude/issue-8262-reaper-verify-not-infer branch August 14, 2026 14:07
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.

reapRotatedSecret deletes the IN-FORCE ciphertext when a SettingsEngine adapter drops context, leaving a dangling value_enc and an unreadable setting

2 participants

@qq9340100@claude