Skip to content

fix(plugins,services): guarantee a warn channel on twelve optional-error sinks - #10691

Merged
os-warren merged 4 commits into
mainfrom
claude/issue-10556-optional-error-sink-paydown
Aug 21, 2026
Merged

fix(plugins,services): guarantee a warn channel on twelve optional-error sinks#10691
os-warren merged 4 commits into
mainfrom
claude/issue-10556-optional-error-sink-paydown

Conversation

@os-warren

@os-warrenos-warren commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Part of #10556

⚠️Deliberately Part of, not a closing keyword. The dispatch asked for
Fixes, and that would be wrong here: #10556 covers fifteen ledger rows and
this PR pays down twelve. The three that remain are the two DESIGN calls the
card escalates and one serialisation leftover. A closing keyword would merge this
and silently close the card that is the only tracking artifact for those design
decisions — and inbox filters only read open. The card should be re-triaged, not
auto-closed.

Scope, and the one exclusion

Twelve of the card's thirteen mechanical repairs: drop the ? from warn so a
sink declaring an optional error always has somewhere to put a durability
report, then re-check the package's call sites. error stays optional on every
one of them — requiring it was measured and rejected as #9754 option C.

plugin-approvals/src/approval-service.ts is excluded — serialisation
against PR #10546, which owned that file. 12/13, not an omission.

⚠️The reason for that exclusion has since expired, and this PR does not act on
it.
PR #10546 merged at 2026-08-21T05:08:46Z, so the file is no longer held.
It is a one-line repair with no design content, and its ledger row now says so.
Left for a follow-up dispatch rather than silently widened.

The plugin-audit question, verified rather than inherited

The card holds the two plugin-audit entries back because that package was PR
#10450's file surface. Re-checked directly: #10450 merged at
2026-08-21T04:53:22Z.
The package is free, so AuthEventAuditLogger and
ReadAuditLogger are repaired normally here. AuthEventAuditLogger is the sink
#9754's body calls the sharpest instance, and its docblock now records the whole
arc: error?+debug? and no warn at all → #9750 added warn? → non-optional.

What tsc found that the card did not predict

The card sizes all thirteen as one-line repairs. Two of them are not, and three
call sites went red. Every one of these is a real signal, and none was papered
over by widening a consumer.

Three forwarding seams

A guarantee is lost at the seam where a looser sink is handed to a tighter one.
tsc named each; none was guessed at.

seamforwarded intorepair
MinimalLogger — plugin-sharing rule-hooks.tsstashAffectedRowsOnCtxwarn required
MinimalLogger — plugin-sharing record-share-cascade.tsstashAffectedRowswarn required
AuthManagerOptions['logger'] — plugin-authReconcileMembershipDeps['logger']warn required

The third is the only externally visible one, so it was measured before being
tightened rather than after: the sole non-test construction site in this repo is
auth-plugin.ts, which passes ctx.logger — the kernel Logger, whose warn
is already required — so the in-tree cost is zero. The field stays optional;
what changed is that a logger which is supplied must be able to warn.

Two of the twelve carry the = {} default the card fences off as a design call

report-service.ts and auto-enqueuer.ts both did opts.logger ?? {} — the
same shape the ledger records against plugin-security, where it is explicitly
not a ?-deletion. So the card's "thirteen are a one-line repair" is not
quite right, and this PR does not quietly decide the question it opens.

Neither default was invented here. The private field is now honestly optional
(this.logger?.warn?.(…)) instead of being handed a minted { warn: () => {} }
— a sink that declares it can report and discards everything, which is the
failure #9754 exists to stop, not a repair for it. Runtime behaviour is
unchanged in both directions
: an absent logger and {} both printed nothing
before and print nothing now. Whether an absent host sink should instead default
to a console-backed one is left to the maintainer, with the rest of the design
call.

Two DESIGN calls: measured, not decided

Per the dispatch these are reported, not answered. Both measurements are new.

1. plugin-security/src/security-plugin.ts — the = {} field is not merely
a boot-window risk. start() has two early returns above the
this.logger = ctx.logger assignment (missing objectql/metadata service, and
an engine without registerMiddleware), so on either path this.logger stays
{} for the lifetime of the instance — the field is assigned nowhere else. Cost
of requiring warn: a default must be chosen, which is the decision.

⚠️A first version of this measurement was overstated, and is corrected here
rather than quietly dropped.
I first wrote that the security service
"registered back in init() keeps serving, so its six fail-closed reports go
nowhere". Both halves were wrong: registerService('security', …) is at :1175
inside start() (which spans :812:3085), not in init(), so on an
early-return boot the service is never registered and those reports are
unreachable rather than silent; and the file has eleventhis.logger.*
sites, not six — six was a partial grep window. Mapping each line to its
enclosing method (rather than eyeballing) leaves exactly one site reachable with
logger === {}: :3576 in checkAuthoredRowWrite, a public instance method
that does not depend on registration. That path is structurally possible and
not demonstrated — no in-repo caller invokes it on the instance rather than
through the registered service. Filed with the full correction as #10706, so
the finding survives however the default question is ruled.

2. service-settings/src/settings-service.types.ts — cost of a required
warn, counted: 10 one-member { error } spies across 6 test files stop
being assignable. Two corrections to the card's framing, both measured:
config-change-audit.ts already re-adds the channel locally as
SettingsDiagnosticsLogger & { warn?: … } and degrades warnerror, so part
of the tree has already paid for the missing member; and the three report sites in
settings-service.ts are not silent at runtime — each is
if (this.logger?.error) … else console.error(message). The no-fallback
verdict is a statement about the type, and the module routes around it with a
hard-coded console. That makes this the same question as (1) — console escape
hatch, or a declared channel — which is an argument for ruling on both together.

Ablation

Predicted signature written before mutating. Resolution path: the checker
readFileSyncs .tssource under packages/** (SCAN_ROOTS = ['packages'])
— no dist/, no module resolution — so no rebuild is involved, and no ablation
leg here can be a build artifact.

Leg A — un-repair auth-event-audit.ts, ledger row already deleted.
Predicted: exit 1, census 22 → 21 guaranteed / 3 → 4 red, violation naming
AuthEventAuditLogger. Observed, exactly:

✗ 1 sink type(s) declare an optional `error` with no guaranteed fallback channel
packages/plugins/plugin-audit/src/auth-event-audit.ts:106
sink : interface AuthEventAuditLogger { error? warn? debug? }

Leg B — sink repaired, its ledger row put back. Predicted: exit 1, census
unchanged at 3 red, a stale-entry failure. Observed, exactly:

✗ 1 stale entry(ies) in scripts/optional-error-sink-contract.baseline.json — the sink is no longer red
packages/plugins/plugin-audit/src/auth-event-audit.ts::AuthEventAuditLogger

Leg B is the one that matters for review: it proves deleting the twelve rows was
mandatory in this PR, not cosmetic tidying.

Both restores proved byte-identical with git hash-object
(01f65820ed8da845e70129f7fd3b37776115b59a and
568bfc54f3153d6b254a16d8424b956e3ee48ee3), and the restore leg re-measured
green (exit 0, 3 baselined).

Verification

Gate union derived by node scripts/pm/dispatch-gates.mjs with no path
arguments
, re-derived after the source-final commit at 5243df47f (21
paths; the family set came back identical to the 19-path derivation). Exit codes
captured before any pipe — cmd > file 2>&1; EXIT=$? — never through tail.

⚠️Scope of the re-run after the changeset correction (44232c2c8), stated
rather than implied.
That commit changes one file,
.changeset/optional-error-sink-paydown.md:
git diff --name-only 5243df47f HEAD -- packages/ scripts/ returns 0 files,
so every source input the ratchets read is byte-identical and the full suite was
not re-run. What was re-run at the new head: the five changeset gates below,
plus check:optional-error-sink (exit 0, 3 baselined) and check:nul-bytes
(exit 0) because those two are cheap and read the tree directly. The test suites,
the 44-package consumer sweep and check:type-check-debt --re-measure are the
5243df47f measurements and are labelled as such.

13 path-matched families + 6 convention-triggered + check:optional-error-sink
and check:nul-bytes — all exit 0.
Verdict lines as each gate printed them:

✓ optional-error sink contract: every sink declaring an optional `error` guarantees a `warn` channel (3 baselined, shrink-only).
check-nul-bytes: OK (scanned 6212 text file(s) ... no raw ASCII control bytes).
check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured in 815.9s, 1916 raw tsc error(s) total, none above its recorded number.

Census movement, the whole point of the card: 15 red → 3, and
fallback-guaranteed10 → 22.

⚠️ Per the dispatch, check:route-envelope and
check:dispatcher-error-vocabulary were run explicitly (each also with
--self-test) because the diff touches packages/plugins/plugin-auth/** — class
#10309. The derivation did name route-envelope on its own (via
cloud-connection-plugin.ts) but notdispatcher-error-vocabulary; both
green.

Tests — all ten changed packages, 5410 passed across 323 files:
cloud-connection 210, metadata-protocol 1738 (+10 skipped), plugin-approvals 519,
plugin-audit 313, plugin-auth 1359, plugin-email 421, plugin-reports 70,
plugin-sharing 624, plugin-webhooks 119, service-knowledge 37.
typecheck clean on all seven packages that declare the script — each echoed
tsc --noEmit then Done, so none was a zero-match silent pass.

Consumer sweep, direction stated. The downstream (prefix) direction is
where a contract tightening lands, so the filters were spelled
--filter '...@objectstack/plugin-auth' — dots before the name, consumers —
and not the suffix form '@objectstack/plugin-auth^...', which is the opposite
direction (dependencies). 44 packages built green in the consumer direction,
after the dependency closure was built first with the suffix form.

(Spelled out with a real package name on purpose: GitHub's body sanitizer eats
short angle-bracket fragments, so an angle-bracketed pkg placeholder here
silently became --filter '...' — and the whole distinction this paragraph
exists to make disappeared with it. Observed on this very PR body, and corrected.)

Two things the package builds could not have caught

@objectstack/cloud-connection, @objectstack/metadata-protocol and
@objectstack/service-knowledge declare no typecheck script, so a
--filter … typecheck run over them exits 0 having run nothing. Both findings
below came from measuring instead:

  • check:type-check-debt --re-measure caught metadata-protocol DEBT drifting
    63 → 64. Cause: partial-index-probe.test.ts passed {} to pin that
    logProblemtolerates neither channel. {} is exactly what the tightened
    type forbids, so the case now casts deliberately and says why — the type-level
    guarantee and the runtime ?. backstop for a lying host are different
    promises, and that case asserts the second. Ledger number untouched, back at
    63
    .
  • @objectstack/plugin-auth's hidden test layer. Measured directly with the test
    exclusion dropped: 98 with this branch's plugin-auth changes reverted, 101
    with them
    — so the +3 was this branch's, and three { error: vi.fn() }
    doubles in reconcile-membership.test.ts are repaired. Back to 98.

TEST_DEBT ledger untouched at 109, and --lower was NOT run. The 11-error
surplus #10615 reports is left exactly as that card describes it — this branch
neither consumes nor closes it. (The gate reports the surplus as , not an
error.)

Scope discipline

⛔ No packages/spec file is modified — git diff --name-only covers
.changeset/, ten packages/** source trees, three test files and
scripts/optional-error-sink-contract.baseline.json only. ⛔ Nothing under
content/docs/releases/. The card's own fences hold: the examples/app-showcase
16th sink is untouched and the checker's packages/** scan is not widened.

Changeset: present, minor for all ten published packages, declared
**BREAKING**, and carrying its ADR-0087 disposition.

Three corrections after PM review, all measured rather than taken on advice:

  • The type name was wrong.AuthManagerConfig does not exist —
    git grep AuthManagerConfig -- packages/ returns zero source hits and only
    two lines in a stale plugin-auth/CHANGELOG.md. The real declaration is
    AuthManagerOptions (auth-manager.ts:375, public via
    export * from './auth-manager.js'), with the tightened logger at :647.
    This is worse than a typo because a changeset body ships verbatim as
    CHANGELOG.md: an embedder who hit the compile error and grepped the name
    would have found an already-retired spelling and been led away from the fix.
  • patch contradicted the changeset's own text.
    check-changeset-no-major.mjs states the convention in its own WHY THIS
    EXISTS
    block — "During the launch window we ship breaking changes as minor
    (pre-1.0 semantics …)"
    — and every publishable package is in the Changesets
    fixed group, so the bump level is the only channel that tells the whole stack
    a release can break a build. I looked for a convention exempting a types-only
    break and found none. major is banned by that same guard.
  • ADR-0087 disposition added, and the marker was chosen by reading the gate
    rather than pattern-matching the common case. no-migration-prescription is
    refused when the body carries a prescription, and runtime-interface-only
    inherits that same refusal (ADR-0087's changeset disposition has no category for published runtime TS interfaces with no metadata surface — the #8277 exemption argument is correct, unverifiable, and will be re-litigated #8299) — a narrowing, never an escape. The
    honest and mechanically re-checkable claim here is runtime-interface-only,
    and the gate verified all seven named symbols resolve as plain interfaces that
    are neither metadata surfaces nor referenced by one:
✓ check-adr-0087-registration: 1 declared-breaking changeset(s), each carrying an ADR-0087 disposition.
.changeset/optional-error-sink-paydown.md [BREAKING] not-required (runtime-interface-only) -- verified: … (interface) ×7

⚠️ Worth recording: the first run of that gate returned exit 0 saying
no declared-breaking changeset (1 non-breaking changeset(s) seen)
— because
the gate reads changeset content from git, and the rewrite was still
uncommitted. The green was about the old patch text. Committing first and
re-running is what produced the verdict above; the pair is the evidence that the
first green was uninformative.

Release condition, not an observation

This closes silence at authoring time, not at runtime. Call sites keep the
logger?.warn?.(…) spelling — deliberately, per SweepLogger's recorded
measurement — so a host that lies about its shape (a plain-JS embedder, or a cast
at the boundary) still degrades to silence rather than throwing inside a
durability catch. The guarantee this PR buys is that such a host can no longer be
written in TypeScript against these contracts; it does not make a lying host
loud. Three sinks still permit silence by declaration and are listed in the
ledger with the reason each is still there.


Generated by Claude Code

…`error` sinks
Pays down twelve of the fifteen rows in the shrink-only optional-error sink
ledger (#9754): each sink declaring an optional `error` now declares a
NON-optional `warn`, so a durability report always has somewhere to land.
`error` stays optional everywhere — requiring it was measured and rejected as
#9754 option C. The twelve ledger rows are deleted in this same commit, which
the checker requires (it fails on a stale entry).
Three forwarding seams were tightened with them, each reported by `tsc` rather
than guessed at: `MinimalLogger` in plugin-sharing's `rule-hooks.ts` and
`record-share-cascade.ts` (both forward into bulk-recompute's now-guaranteed
sink), and `AuthManagerConfig['logger']` in plugin-auth (forwards verbatim into
`ReconcileMembershipDeps`).
Two of the twelve turned out NOT to be one-line repairs: `report-service.ts`
and `auto-enqueuer.ts` both defaulted their logger to `?? {}` — the same shape
the ledger fences off as an open design call for plugin-security. Neither
default was decided here. The field is now honestly optional instead of being
given a minted sink that declares it can warn and discards everything; runtime
behaviour is unchanged in both directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
…ghtened sink
`partial-index-probe.test.ts` passed `{}` as an `IndexMigrationLogger` to pin
that `logProblem` degrades to silence rather than throwing. `{}` stopped being
a legal value of that type when `warn` became non-optional, so the case now
casts deliberately and says why: the type-level guarantee and the runtime
backstop are different promises, and this case asserts the second.
Caught by `check:type-check-debt --re-measure` (DEBT 63 -> 64), not by the
package build: `@objectstack/metadata-protocol` declares no `typecheck` script,
so its test files are hidden from tsc and a `--filter ... typecheck` run would
have reported a silent zero-match pass. Ledger number untouched, back at 63.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
…r doubles
`ReconcileMembershipDeps['logger']` guarantees a `warn` channel (#9754), so the
three `{ error: vi.fn() }` doubles in `reconcile-membership.test.ts` stopped
being legal values of it. A fourth double in the same file already declared
`warn` and was unaffected.
Measured rather than assumed, because these files are HIDDEN from
`pnpm typecheck` (plugin-auth's tsconfig excludes its own tests, TEST_DEBT):
`tsc` over a tsconfig with the test exclusion dropped reports 98 with this
branch's plugin-auth changes reverted and 101 with them, so the +3 was this
branch's. With these three doubles repaired it is 98 again.
⛔ The TEST_DEBT ledger number is untouched at 109 and `--lower` was NOT run.
The 11-error surplus #10615 reports is left exactly as that card describes it —
neither consumed by this branch nor closed by it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

github-actionsBot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 10 package(s): @objectstack/cloud-connection, @objectstack/metadata-protocol, @objectstack/plugin-approvals, @objectstack/plugin-audit, @objectstack/plugin-auth, @objectstack/plugin-email, @objectstack/plugin-reports, @objectstack/plugin-sharing, @objectstack/plugin-webhooks, @objectstack/service-knowledge, touching 27 documentable anchor(s).

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

  • content/docs/automation/webhooks.mdx(via AutoEnqueuer (symbol))

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

  • content/docs/releases/v15.mdx(via ReportService (symbol))

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
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 31 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 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4packageMentionDocs.

Which tree this was computed on

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

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

…inor`
Three defects in the changeset text, all found in PM review of #10691:
1. `AuthManagerConfig` does not exist. The type is `AuthManagerOptions`
(auth-manager.ts:375, public via `export * from './auth-manager.js'`), and
the tightened `logger` field is :647 inside it. Measured: `AuthManagerConfig`
has ZERO hits in `packages/**` source and only two in a stale CHANGELOG.md —
so an embedder who hit the compile error and grepped the name this changeset
shipped would have found an already-retired spelling and been led away from
the fix. That is what makes it worse than a typo: a changeset body IS the
published CHANGELOG.
2. `patch` contradicted the changeset's own text. `check-changeset-no-major.mjs`
records the convention in its own WHY THIS EXISTS block: "During the launch
window we ship breaking changes as `minor` (pre-1.0 semantics ...)" — every
publishable package is in the Changesets `fixed` group and versions in
lockstep, so the bump level is the only channel that tells the stack a release
can break a build. A changeset that says "an external embedder ... will now
see a compile error" cannot also be `patch`. Now `minor`; `major` is banned by
that same guard.
3. The break is now DECLARED (`**BREAKING**`) and carries its ADR-0087
disposition, which a declared-breaking changeset must state in writing
(AGENTS.md, enforced by `check:adr-0087-registration`).
The marker is `runtime-interface-only`, naming the seven exported symbols, and it
was chosen by reading the gate rather than by pattern-matching the common case:
`no-migration-prescription` is REFUSED when the body carries a prescription, and
`runtime-interface-only` inherits that same refusal (#8299) — it is a narrowing,
never an escape. It is claimable here because it is TRUE and mechanically
re-checkable: every tightened type is a plain TS logger interface, no Zod
projection, no metadata surface and unreferenced by one, so `objectstack migrate
meta` provably has nothing to rewrite. Nothing is removed or renamed, so
AGENTS.md's FROM → TO requirement (scoped to removals and renames) is not
triggered; the consumer action is stated plainly instead.
Changeset text only — no source file is touched by this commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@os-warren
os-warren marked this pull request as ready for review August 21, 2026 09:21
@os-warren
os-warren enabled auto-merge August 21, 2026 09:21
@os-warren
os-warren added this pull request to the merge queueAug 21, 2026
Merged via the queue into main with commit e222a53Aug 21, 2026
35 checks passed
@os-warren
os-warren deleted the claude/issue-10556-optional-error-sink-paydown branch August 21, 2026 09:39
os-warren pushed a commit that referenced this pull request Aug 21, 2026
…sink
Drops the `?` from `warn` on ApprovalServiceOptions['logger'] so a sink
declaring an optional `error` always has a guaranteed durability-report
channel (#9754, #10556). `error` stays optional. This is the thirteenth of
the card's thirteen mechanical repairs -- held out of #10691 to serialize
against PR #10547, which owned this file while open; that fence cleared
when #10739 merged.
Deletes the now-stale row from the shrink-only
scripts/optional-error-sink-contract.baseline.json (the checker itself
named the row as no-longer-red). The other two rows -- plugin-security's
default-sink design call and service-settings' no-fallback design call --
are untouched; both remain escalated to the maintainer.
No construction site needed a widening fix: the sole non-test call
(ApprovalsServicePlugin.start) passes ctx.logger, whose warn is already
required by the core Logger contract.
Part of #10556.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042
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.

2 participants

@os-warren@claude