Skip to content

fix(plugin-approvals): require warn on ApprovalServiceOptions logger sink - #10937

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-10556-approval-service-sink
Aug 21, 2026
Merged

fix(plugin-approvals): require warn on ApprovalServiceOptions logger sink#10937
os-warren merged 1 commit into
mainfrom
claude/issue-10556-approval-service-sink

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Part of #10556

What

Drops the ? from warn on ApprovalServiceOptions['logger'] so a sink
declaring an optional error always has a guaranteed durability-report
channel (#9754). error stays optional. This is the last of the card's
thirteen mechanical repairs — held out of #10691 to serialize against
PR #10547, which owned approval-service.ts while it was open; that fence
cleared when #10739 merged (2026-08-21T14:40Z).

Deletes the now-stale logger@ApprovalServiceOptions row from the
shrink-only scripts/optional-error-sink-contract.baseline.json, identified
by running pnpm check:optional-error-sink after the type fix and using its
own "1 stale entry(ies)" verdict as the deletion criterion (not by line
number). The other two rows — plugin-security's default-sink design call
and service-settings's no-fallback design call — are untouched; both
remain escalated to the maintainer per #10556's re-triage
(issuecomment-5368213444). Baselined count after this PR: 2 (as
expected).

⛔ Not Fixes #10556 — the card still carries two unruled design
decisions; a closing keyword would silently close their only tracking
artifact.

Construction-site re-check

The sole non-test construction site (ApprovalsServicePlugin.start, same
package) passes ctx.logger (PluginContext['logger']), whose Logger
contract already requires warn — zero widening needed. pnpm --filter '@objectstack/plugin-approvals' typecheck is clean (echoed tsc --noEmit
then done — not a zero-match pass). Test-file loggers (noopLogger,
capturing, inline { warn: ... } spies) already declare warn; none
needed a change.

Ablation (both directions, predicted before mutating)

  • Leg A (un-repair, ledger row stays deleted): predicted the gate reds
    with "no guaranteed fallback channel". Observed exactly:
    ✗ 1 sink type(s) declare an optional error with no guaranteed fallback channel ... sink: inline type logger@ApprovalServiceOptions { info? warn? error? debug? }. Restored via git checkout HEAD -- packages/plugins/plugin-approvals/src/approval-service.ts (fix was
    already committed) — git hash-object confirms byte-identical restore
    (20083b9a4ab5ef6e3d3cff46bfcf12701d968df8).
  • Leg B (repair kept, ledger row restored): predicted stale entry.
    Observed exactly: ✗ 1 stale entry(ies) in scripts/optional-error-sink-contract.baseline.json ... packages/plugins/plugin-approvals/src/approval-service.ts::logger@ApprovalServiceOptions
    — proves the row deletion is a required part of this fix, not tidying.
    Restored via git checkout HEAD -- scripts/optional-error-sink-contract.baseline.json,
    byte-identical (1a933af08d6aa911b7377d63effc352c8116d80a).

Tests

All at final head 1a9baa986, exit codes captured before any pipe.

  • pnpm check:optional-error-sink: exit 0 — ✓ optional-error sink contract: every sink declaring an optional errorguarantees awarn channel (**2** baselined, shrink-only).
  • pnpm --filter '@objectstack/plugin-approvals^...' build: exit 0 (dependency
    closure — upstream direction, ^...).
  • pnpm --filter '@objectstack/plugin-approvals' typecheck: exit 0.
  • pnpm --filter '@objectstack/plugin-approvals' test -- --maxWorkers=2: exit
    0 — 536 tests / 28 files passed.
  • pnpm check:route-envelope -- --self-test and pnpm check:dispatcher-error-vocabulary -- --self-test: both exit 0. The
    gate-union derivation (node scripts/pm/dispatch-gates.mjs, no path args)
    did not name either — this diff touches no packages/plugins/plugin-auth/**
    path, so class [finding] Every PM dispatch list is short by the same ~5 changeset-triggered gate families — they are path-derivable, but the changeset does not exist yet when the list is derived #10309 does not apply to this PR's surface; run anyway per
    dispatch item 7 and both green.
  • Changeset gates: check:empty-changeset, check:adr-0087-registration,
    check:changeset-no-major (and the bundled check:changeset-gate-self-tests)
    all exit 0 on the committed tree. check-adr-0087-registration verdict:
    [BREAKING] not-required (runtime-interface-only) -- verified: packages/plugins/plugin-approvals/src/approval-service.ts#ApprovalServiceOptions (interface).
  • Full named-gate sweep from node scripts/pm/dispatch-gates.mjs (no path
    args, run after the final commit, clean tree): all 15 path-matched
    families green, plus the one convention-triggered gate (pnpm check:i18n,
    triggered because this package owns an i18n-extract.config.ts) green
    after building the CLI (check:i18n's own prerequisite — it exits 1 with
    "PREREQUISITE NOT MET" and checks nothing until the CLI is built).

Bump / ADR-0087 reasoning (read #10691's changeset first, per dispatch)

minor, matching #10691's twelve sibling repairs: major is banned by
check-changeset-no-major.mjs during the launch window (lockstep
versioning), and patch would be wrong because this can break a consumer's
build (compile-time only, no runtime change). No exemption for a types-only
break was found, matching #10691's own search.

not-required (runtime-interface-only): ApprovalServiceOptions is a plain
TypeScript interface — no Zod projection, no metadata surface, not
referenced by one — so objectstack migrate meta has nothing to rewrite.
Nothing is removed or renamed (AGENTS.md's FROM→TO prescription requirement
is scoped to removal/rename) and no stored value moves.

⭐ Instrument note (confirmed again on this PR):
check-adr-0087-registration reads changeset content from git, not the
working tree — it must be re-run after committing the changeset to carry
any information.


Generated by Claude Code

…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
@github-actionsgithub-actionsBot added size/s documentation Improvements or additions to documentation tooling labels Aug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 5 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 9d101d28424fcdb067277709bbbb9097a3bc2fd9packageMentionDocs.

@os-warren
os-warren marked this pull request as ready for review August 21, 2026 19:39
@os-warren
os-warren enabled auto-merge August 21, 2026 19:39
@os-warren
os-warren added this pull request to the merge queueAug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32520389049 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 4.70s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 45 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/stooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-warren@claude