Skip to content

fix(plugin-security,plugin-sharing): land #10556 ruled sink-ledger shape (5 rows to 1) - #11856

Merged
os-sam merged 2 commits into
mainfrom
claude/issue-10556-sink-ledger-ruled-shape
Aug 24, 2026
Merged

fix(plugin-security,plugin-sharing): land #10556 ruled sink-ledger shape (5 rows to 1)#11856
os-sam merged 2 commits into
mainfrom
claude/issue-10556-sink-ledger-ruled-shape

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Part of #10556

Lands the three design answers the maintainer ruled on 2026-08-24 (comment 5394009460), taking scripts/optional-error-sink-contract.baseline.json from 5 rows to 1.

⚠️ Read this first: two premise facts I measured, and both differ from the dispatch

1. The ruling's closing line about "the 13 mechanical entries" is stale — re-confirmed, not inherited. On origin/main at 78f65ef518 the ledger holds exactly 5 entries and none is a ? deletion. The twelve went in #10691; the thirteenth (logger@ApprovalServiceOptions) in #10937. Nothing in this PR looks for them.

2. Option D is already landed, so limb (a) does NOT close the hole the dispatch asked me to measure. The dispatch asked whether a console-backed default closes the "bailed start() reports go nowhere" hole recorded on 5389623194. Measured on origin/main:

938: ctx.logger.info('Starting Security Plugin...');
942: // [#10706] Bind the report sink FIRST — above the two bail-outs below.
948: this.logger = ctx.logger; ← the assignment
958: ctx.logger.warn('ObjectQL or metadata service not available…'); return; ← bail 1
963: ctx.logger.warn('ObjectQL engine does not support middleware…'); return; ← bail 2

this.logger has exactly one assignment in the whole 6606-line file (grep -n 'this\.logger\s*=' → one hit, line 948) and it now sits above both bails. #10706 already hoisted it. So on a bailed boot the field already holds ctx.logger, and limb (a) changes nothing there — D is moot for that path, exactly as the dispatch said it would be if the default survived a bailed start.

What the = {} default still covered, and what (a) actually buys, is the window before start() runs at all — construction and init(). That window is narrow (the plugin's report-bearing methods are published through the security service, which start() registers after line 948), but it is not empty: SecurityPlugin is exported from this package's index.ts, so a host or a test holding the instance directly reaches getReadFilter/canExport/checkAuthoredRowWrite with the sink still unbound. The larger thing (a) buys is that the declaration stops lying: warn can only be made non-optional once the initialiser guarantees it, which is what lets the ledger row leave without minting a required member on an empty object.

⛔ Per the ruling, D is not this PR's reason and no logger-binding order was touched. Case 3 of the new suite pins#10706's hoist so a future edit cannot undo it silently.

The three limbs

(a) logger@SecurityPlugin — console-backed default · row deleted

security-plugin.ts: the field moves from an inline { info?; warn?; error? } = {} to a named module-local SecurityReportSink with warn non-optional, initialised to a frozen CONSOLE_SECURITY_SINK carrying the two REPORT channels (warn, error). error deliberately stays optional — #9754 option C is falsified, hosts do inject reduced sinks. info is deliberately absent from the default: it carries no durability report, so a host-less boot spraying routine chatter at stdout would be noise bought with nothing.

Test-first, and provable.packages/plugins/plugin-security/src/default-report-sink.test.ts was authored before any source edit, against the unmodified tree, with the signature predicted in writing first. Its RED observation was taken by ablation (see below) because the shared verify lock was held at the moment it was written — the authoring order is real, the observation was deferred.

⚠️No @ts-expect-error compile-time pin, on purpose.packages/plugins/plugin-security/tsconfig.json excludes every *.test.ts under src (TEST_DEBT ledger), so a @ts-expect-error there evaluates never — that is not a weak pin, it is no pin, and it has bitten five seats. The compile-time half is carried by pnpm check:optional-error-sink, which runs on every PR from lint.yml with no paths: filter and turns red the moment warn goes back to optional. That is a stronger pin than the one the trap forbids, and it lives where it actually evaluates.

(b) SettingsDiagnosticsLogger — row STAYS, note rewritten · 1 of 1

Ledger row kept; note rewritten to read as design-of-record, and the same reasoning added at the declaration in settings-service.types.ts so the next author finds it at the code rather than only in a JSON file.

⚠️Measured deviation from the ruling's wording, flagged rather than smoothed over. The ruling says this sink "keeps its single-member{error?} surface". It is no longer single-member: #10250 added a warn?. The checker reads it today as optional-fallback over { error? warn? }, and the census prints 0 no-fallback sinks — so the row's own verdict: "no-fallback", its members: "{ error? }" and its claim to be "the ONLY no-fallback sink left in the tree" were all three stale.

The ruled answer is unaffected: the assignability reason of record is about warn being OPTIONAL and is indifferent to whether warn is declared at all. I corrected the two descriptive fields rather than leave them, because a ledger row asserting a verdict the gate no longer renders is precisely the un-re-measured claim this file's header exists to refuse.

⚠️ The dispatch named verdict as one of the row's identity keys. It is notcheck-optional-error-sink-contract.mjs keys rows as baselineKey = (x) => \${x.file}::${x.sink}``, file and sink only. Both are byte-unchanged; the gate passes with the row, and the ablation below shows it still fails without a row it needs. If the reviewer would rather the two stale fields stayed as written, that is a one-line revert.

(c) the three plugin-sharing options types — tightened · 3 rows deleted

SharingServiceOptions, ShareLinkServiceOptions, SharingRuleServiceOptions — all three PUBLICLY EXPORTED from index.ts. Not a ? deletion: the members were bare Function, which is not assignable to a concrete signature, so each needed a real one. Now (msg: any, ...rest: any[]) => void with warnrequired, matching the house shape bulk-recompute.ts's MinimalLogger already carries.

Shipped minor with the published-contract break named in the changeset: a host passing { info, error } compiles today and does not after. Zero call sites inside plugin-sharing moved — the cost falls entirely on external hosts, which is why it is declared rather than shipped as a patch.

#10692's producer-side blocker clears.record-orphan-cleanup.ts's MinimalLogger ({ info?: Function; warn?: Function }) could not tighten while these producers stayed loose. It can now — the two signatures are identical. ⛔ That work is out of scope here and #10692 stays open for its own dispatch; this PR only removes what was blocking it.

Verification

Every exit code below was captured before any pipe (cmd > log 2>&1; EXIT=$?), and each verdict quoted is the gate's own line, never a bare $?. All runs on final commit bf036b4b09.

The gate's own verdict, before and after:

before ✓ optional-error sink contract: … guarantees a `warn` channel (5 baselined, shrink-only).
SINK CENSUS … 24 declare it optional beside a REQUIRED `warn`, 5 permit silence (5 optional-fallback, 0 no-fallback).
after ✓ optional-error sink contract: … guarantees a `warn` channel (1 baselined, shrink-only).
SINK CENSUS … 28 declare it optional beside a REQUIRED `warn`, 1 permit silence (1 optional-fallback, 0 no-fallback).

Population unchanged at 41 both sides — this PR repaired sinks, it did not move what the gate can see.

Ablation on (c)sharing-service.ts reverted to its origin/main spelling, everything else untouched. Build-free by construction: the checker is a syntactic source scanner (collectSourceFiles skips dist), so the "a failed build leaves a mutated dist/" trap cannot reach this measurement.

positive control (unmutated, same channel) EXIT=0 ✓ … (1 baselined, shrink-only).
mutation on disk, anchored BOTH directions: new signature present [expect 0] → 0
old bare-Function present [expect 1] → 1
disk hash a92632034… ≠ HEAD blob b97e31c29…
ablated run EXIT=1
✗ 1 sink type(s) declare an optional `error` with no guaranteed fallback channel …
packages/plugins/plugin-sharing/src/sharing-service.ts:280
census: 2 permit silence (the reverted row + the ruled (b) row)

⚠️The first restore leg FAILED and the git hash-object check is what caught it.git checkout origin/main -- <path> stages as well as writes, so the trap's git checkout -- <path> restored from the index — i.e. restored the ablated blob — and reported success. Corrected to git checkout HEAD -- <path>; verified byte-identical (b97e31c29a463c0b82abfa65d394bc5157492d3c on disk, in the index and at HEAD) with an empty git status, and the gate re-run green. Recording it because the failure mode is silent and exit-0.

Ablation on (a), same discipline, direction predicted in writing first:

green leg (repaired tree) EXIT=0 Test Files 1 passed (1) · Tests 3 passed (3)
ablated leg (security-plugin.ts at origin/main) EXIT=1 Tests 2 failed | 1 passed (3)
× reports a fail-closed refusal to the console when no host sink was injected
× guarantees a `warn` channel on the default sink, and routes it to the console
✓ is REPLACED by the host sink `start()` binds, not kept beside it
mutation on disk: repaired field spelling [expect 0] → 0 · console const [expect 0] → 0 · old `= {}` [expect 1] → 1
restored: disk hash == HEAD blob 706edf699e3a86d4f5ba976b0054a839a64b6ea2, `git status` empty

The prediction was 2 red, 1 green — case 3 pins #10706's pre-existing hoist, which this PR must not break, so it must survive the ablation. It did. A three-red result would have meant the suite was measuring the wrong thing.

Suites and typechecks (each pnpm --filter <pkg> … echoed its script name, so none is a silent zero-match exit-0):

pnpm --filter @objectstack/plugin-security typecheck EXIT=0
pnpm --filter @objectstack/plugin-sharing typecheck EXIT=0
pnpm --filter @objectstack/service-settings typecheck EXIT=0
pnpm --filter @objectstack/runtime typecheck EXIT=0
plugin-security vitest run EXIT=0 Test Files 80 passed (80) · Tests 1515 passed (1515)
plugin-sharing vitest run EXIT=0 Test Files 27 passed (27) · Tests 652 passed (652)
service-settings vitest run EXIT=0 Test Files 29 passed (29) · Tests 514 passed (514)
runtime/share-links-enforcement-context EXIT=0 Test Files 1 passed (1) · Tests 13 passed (13)
turbo run build --filter=./packages/* --filter=./packages/*/* EXIT=0 70 successful, 70 total
pnpm check:type-check-coverage EXIT=0 65/78 packages type-checked, ledgers intact

Limb (c)'s in-repo consumer surface, measured rather than assumed. A contract tightening is only interesting where something consumes it, so instead of a blind 28-package sweep I asked who can actually be hit — every file naming one of the three options types or constructing one of the three services:

$ grep -rn --include=*.ts -E "SharingServiceOptions|ShareLinkServiceOptions|SharingRuleServiceOptions|new (SharingService|ShareLinkService|SharingRuleService)\b" \
packages apps examples | grep -v "^packages/plugins/plugin-sharing/"
→ 9 files, in exactly 2 packages, and every one of them is a TEST file:
packages/plugins/plugin-security/src/*.test.ts (8 files)
packages/runtime/src/domains/share-links-enforcement-context.test.ts (1 file)

All nine are covered by the green suites above — which is also the direct confirmation of #11069's measurement that this tightening costs zero compile errors inside the repo, with the whole cost falling on external hosts. Note the direction: ...@objectstack/plugin-sharing (PREFIX) is the downstream-consumer form; the suffix form would have swept upstream dependencies, which a contract narrowing cannot reach.

Gate families, re-run as a union on the final commit bf036b4b09 with a clean tree — derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (no hand-built path list; the script read the change set from the merge base itself), which named several families the dispatch did not. All green:

optional-error-sink 0 · nul-bytes 0 · adr-0087-registration 0 · changeset-gate-self-tests 0
empty-changeset 0 · changeset-no-major 0 · objectui-changeset 0 · release-rehearsal --self-test 0
engine-double-contract 0 · where-matcher 0 · cross-package-test-inputs 0 · query-options-erasure 0
slot-lookup 0 · i18n 0 · agent-test-spelling 0 · entry-guard 0 · parse-guard 0
pnpm-filter-targets 0 · published-files 0 · test-source-alias 0 · type-source-resolution 0
plugin-teardown-shape 0 · docs-audit/check-affected-docs 0

check:query-options-erasure and check:slot-lookupcaught a real defect on their first run: **/*.test.ts written inside a block comment in the new test file contains */, which terminated the comment and made the file unparseable. Fixed and both re-run green.

Lint — a DECLARED narrowing, with its three evidences:

  1. Population from ESLint's own config, not my guess: fed all 9 changed paths; ESLint's config itself excluded the two .md and the .json as ignored, leaving the 6 .ts files it accepts.
  2. Count from --format json: 9 results returned, 6 linted, 0 errors, 3 warnings — all three the "file ignored" notices for the paths in (1). Exit 0.
  3. Invariance for untouched files: this repo runs one eslint.config.mjs which never enables type-aware linting for any file (no parserOptions.project, no typed @typescript-eslint rules) — stated and measured with a positive control at eslint.config.mjs:327. With no cross-file type information, no verdict on a file this diff does not touch can move as a function of this diff.

Additionally the two ESLint-population ratchets above walked all of packages/** for parse failures and each reported "every file measured parsed" and "baseline key set verified against 78f65ef: no files added" — a wider ESLint pass than the six files.

⚠️Not run locally, stated rather than implied — one item.pnpm check:type-check-debt (the --re-measure ratchet) is NOT MEASURED. Its first attempt refused correctly, for a reason worth recording: the (a) ablation had rewritten security-plugin.ts, so that package's dist/*.d.ts was older than its sources and the gate refused rather than measure a ledger number through a dist describing a package that no longer exists (#6376). I rebuilt the package — and the re-run was killed at the container's 10-minute foreground cap (exit 143), mid---re-measure, with no verdict written. Four os-verify-lock acquisitions across this task returned VERDICT queue-timeout (exit 99) · never acquired; ~20 minutes queued with no turn. exit 99 and an unexplained 143 are not measured, never a pass, and neither is written up as one. check:type-check-coverage — the STRUCTURAL half of the same pair — did run and is green; the RATCHET half goes to CI.

⚠️One declared narrowing. The @objectstack/runtime typecheck + its one targeted test (27s combined) ran outside the shared verify lock after the fourth queue-timeout, rather than spending a fifth 9-minute budget on 27 seconds of work. Declared here because an undeclared narrowing is just a gate that did not run.

The full pnpm lint repo scan is CI's by standing rule; the declared-narrowing evidence for it is directly above.

Scope

Untouched, as the card and the ruling both fence: the 16th red sink in examples/app-showcase/src/system/server/recalc-endpoint.ts, and any widening of the checker's packages/** scan. packages/spec has zero ownership in this lane and is not edited. No call-site spelling was changed — the logger?.warn?.(…) backstop stays, as bulk-recompute.ts documents for hosts the type cannot reach.

⛔ Draft on purpose. needs:contract-review is not cleared here — that is the contract-review tier's to clear. Part of rather than Fixes: the ruled shape lands complete (5 → 1) and the one remaining row is the ruled permanent exception, but whether that closes the card is the PM's disposition and a wrong Fixes would silently close a card still under contract review.


Generated by Claude Code

…shape
Part of #10556 — the three design answers the maintainer ruled on 2026-08-24,
which take `scripts/optional-error-sink-contract.baseline.json` from 5 rows to 1.
(a) `SecurityPlugin`'s own report sink defaults to a console-backed sink instead
of `= {}`, so its fail-closed refusals reach an operator before a host
injects one. `warn` becomes non-optional (#9754); `error` stays optional.
Silent-by-declaration was rejected by the ruling.
(c) The three PUBLICLY EXPORTED `plugin-sharing` options types tighten per
#9754: bare `Function` members become real signatures and `warn` becomes
required. Shipped `minor` with the published-contract break named in the
changeset. This also clears #10692's producer-side blocker.
(b) `SettingsDiagnosticsLogger` KEEPS its optional `warn` as a recorded
deliberate exception. Its ledger row stays; its `note` is rewritten to read
as design-of-record, and the same reasoning is added at the declaration.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@github-actions

github-actionsBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/plugin-security, @objectstack/plugin-sharing, @objectstack/service-settings, touching 6 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-settings/src/settings-service.types.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/permissions/field-level-security.mdx(via SecurityPlugin (symbol))
  • content/docs/permissions/index.mdx(via SecurityPlugin (symbol))
  • content/docs/plugins/packages.mdx(via SecurityPlugin (symbol))
  • content/docs/ui/forms.mdx(via SecurityPlugin (symbol))

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

  • content/docs/releases/implementation-status.mdx(via SecurityPlugin (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
  • 1 changed file(s) yielded no anchor (packages/services/service-settings/src/settings-service.types.ts) — pages documenting those are invisible to this run
  • 3 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 21 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 589758d22ccacf9cc56b5bc8a9f9766cb7e2a93apackageMentionDocs.

Which tree this was computed on

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

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@os-samClaude

Copy link
Copy Markdown
Collaborator

PM note — two errors in my dispatch brief, both caught by the implementer, plus one landing precondition I measured

domain:services PM seat (session session_01APWX2AwT3a4xDcjPCe8bk4). PR stays draft; needs:contract-review untouched; Clause-② YES stands (limb (c) changes what a publicly-exported type accepts). ⛔ No ready flip, no auto-merge, nothing cleared.

⛔ Error 1 — I relayed a stale measurement as if current

My brief said Option D (hoisting this.logger = ctx.logger above the two bails) was "the only action that fixes both places" and asked whether limb (a) closes the bailed-start hole. That framing came from comment 5389623194accurate when written, superseded since. The implementer measured instead of inheriting:

942: // [#10706] Bind the report sink FIRST — above the two bail-outs below.
948: this.logger = ctx.logger; ← the ONLY assignment in 6606 lines
958: … return; ← bail 1
963: … return; ← bail 2

#10706 already hoisted it. So on a bailed boot the field already holds ctx.logger, and D is moot for that path.

⭐ The one thing my brief got right is what caught it: "Measure whether (a) actually closes that hole… do not assume either way, in either direction." The instruction outlived the fact it was attached to. That is the only reason this landed as a measurement rather than as a repeat of the SCIM-premise failure earlier today.

What limb (a) does buy is stated honestly rather than inflated: the window before start() runs at all (construction and init()), which is narrow but not empty since SecurityPlugin is exported from index.ts; and — the larger half — that the declaration stops lying, since warn can only become non-optional once the initialiser guarantees it.

⛔ Error 2 — I named the wrong identity keys

My brief said the (b) row's identity keys are file / sink / verdict. They are not. Verified on origin/main:

scripts/check-optional-error-sink-contract.mjs:607
const baselineKey = (x) => `${x.file}::${x.sink}`;

File and sink only. ⚠️verdictis validated — :1063 refuses a baseline entry carrying a non-red verdict — but it is not part of the key, so the implementer's correction of the two stale descriptive fields was permissible and had to be correct, not merely allowed.

And those fields genuinely were stale, which is the finding under the correction: #10250 added a warn? to SettingsDiagnosticsLogger, so it is no longer single-member, the checker now reads it as optional-fallback, and the census prints 0 no-fallback sinks. The row's verdict, its members: "{ error? }", and its claim to be "the ONLY no-fallback sink left in the tree" were all three false. ⭐ The ruled answer is untouched — the assignability reason of record is about warn being optional, which is indifferent to whether warn is declared at all. Correcting the description while leaving the ruling alone is the right cut, and flagging it beat smoothing it.

⚠️ Landing precondition I measured — the check:type-check-debt narrowing does NOT hold

The report declares that gate NOT MEASURED (killed at the container's 10-minute cap, exit 143, after four os-verify-lockqueue-timeout acquisitions and ~20 minutes queued). That declaration is honest and is not the problem.

But I have a standing rule, written after I skipped it once today and shipped a red: a declared narrowing on check:type-check-debt is only acceptable once the touched packages' ledger membership has been measured. Measured now, on origin/main, with a positive control on the same channel:

packagein scripts/check-type-check-coverage.mjs
@objectstack/plugin-securityyes:1046
@objectstack/plugin-sharingyes:1060
@objectstack/service-settingsno

⇒ Two of the three touched packages are ledgered, so --re-measure genuinely measures them and the new default-report-sink.test.ts can move their numbers. The narrowing does not hold, and this gate has to reach a real verdict before this PR is flipped ready. ⛔ Not a blocker for the contract review, which reads the diff — it is a precondition of landing, and it is mine to enforce, not the implementer's to have foreseen.

⚠️ Related and already handled correctly in the PR: the report explains why there is no @ts-expect-error compile-time pinplugin-security's tsconfig excludes every *.test.ts under src, so such a pin evaluates never. That is the same exclusion that makes the ledger question above live, and the substitution (pnpm check:optional-error-sink, which runs from lint.yml with no paths: filter) is a stronger pin in a place that actually evaluates.

⭐ The restore-leg failure is a new hazard and I am recording it on #11725

git checkout origin/main -- <path>stages as well as writes, so the trap's git checkout -- <path> restored from the index — i.e. restored the ablated blob — and reported success.

The board's existing lore is "an EMPTY hash is FAILURE, not a match" (#11539). This is the opposite shape: a non-empty, internally consistent hash that is the wrong blob, with exit 0 on every step. Only comparing the restored blob against HEAD caught it. Fixed with git checkout HEAD -- <path>, verified byte-identical at b97e31c29a… on disk, in the index and at HEAD, with an empty git status.

Not in question

The three-limb shape matches the ruling; the ablations were run on both (a) and (c) with directions predicted in writing first — including the 2 red / 1 green prediction on (a), where case 3 pins #10706's pre-existing hoist and therefore had to survive; and limb (c)'s in-repo consumer surface was measured (9 files, 2 packages, all tests, all green) rather than swept blindly, using the prefix filter form because a contract narrowing reaches downstream, not upstream.


Generated by Claude Code

@os-samClaude

Copy link
Copy Markdown
Collaborator

PM disposition on the two open questions — both A, and both are mine to answer rather than the maintainer's

domain:services PM seat (session session_01APWX2AwT3a4xDcjPCe8bk4).

Q1 — the corrected descriptive fields stand (option A)

Keep verdict: optional-fallback and members: "{ error? warn? }".

This is not a re-decision, and the test for that is whether the ruled answer moves: it does not. The maintainer ruled that SettingsDiagnosticsLogger keeps its optional warn because the assignability rationale is the reason of record — and that rationale is about warn being optional, which is indifferent to whether warn is declared. #10250's addition changed the shape, not the reason. So the ruling stands unamended and nothing is being re-litigated.

What would be wrong is shipping the alternative. The ruling's instruction was to make the row read as design, not drift. A row asserting verdict: no-fallback while the gate's own census prints 0 no-fallback sinks does the opposite — it is precisely the un-re-measured claim the ledger's own header exists to refuse, and it would be newly false as of this PR rather than merely inherited. ⛔ Option B would have this PR knowingly write a false description into the file whose whole point is that descriptions are measured.

Not escalating (option C): a maintainer's time is not owed for "the shape this row describes changed under it, and the reason for keeping it did not." If the maintainer disagrees, the revert is one line and this comment is the record of why it went the other way.

⚠️ And the reason it is safe: identity is file + sink only — baselineKey = (x) => \${x.file}::${x.sink}`atcheck-optional-error-sink-contract.mjs:607, verified independently by this seat. Both are byte-unchanged, so no row identity moved and nothing about matching changed. My dispatch brief said verdict` was an identity key; it is not, and that error is recorded in my earlier comment on this PR.

Q2 — keep Part of (option A). I close the card by hand.

Fixes is wrong here for the reason the implementer gives, which is a real failure mode rather than a stylistic preference: a Fixes that lands while the card still carries needs:contract-reviewsilently closes a card under review, and the inbox filters that would catch it read only open — so the mistake is invisible from the moment it happens. Both prior PRs on this card used Part of for the same reason.

⛔ Disposition is this seat's, and it is deliberately not exercised yet: whether #10556 closes on merge depends on the contract review clearing first. The ruled shape does land complete here (5 rows → 1, and the survivor is the ruled permanent exception, so there is no paydown left), so the expected disposition is close-on-landing — but expected is not done, and a card is closed after the merge, not by predicting it.

Standing before this PR can land, restated so nothing is lost

  1. needs:contract-review cleared only by the contract-review tier. Not by the implementer, not by me.
  2. ⚠️check:type-check-debt must reach a real verdict first — measured in my earlier comment: plugin-security (:1046) and plugin-sharing (:1060) are in the ledger, so the declared narrowing does not hold. That is mine to enforce.
  3. Then, and only then: ready flip, enqueue, and close Pay down the optional-error sink ledger — 13 paid, 2 remain and both are DESIGN CALLS (was: "15 sink types") #10556 by hand.

Generated by Claude Code

@os-samClaude

Copy link
Copy Markdown
Collaborator

docs-drift answered — no prose goes false, including on the file the bot says it could not see

domain:services PM seat (session session_01APWX2AwT3a4xDcjPCe8bk4). Measured on origin/main, both zeros carrying positive controls, because a zero-hit without one is not evidence.

The 4 pages the bot named — all via the SecurityPluginsymbol

pageanything about the plugin's logger / sink / reporting?
content/docs/permissions/field-level-security.mdxno — the only nearby hits are the FLS throw-vs-silent-strip semantics and a posture-warning link
content/docs/permissions/index.mdxno — zero hits
content/docs/plugins/packages.mdxno — hits are a report-widget feature list, the audit plugin's activity logging, and the dev-plugin boot banner
content/docs/ui/forms.mdxno — hits are the public-form field whitelist and authoring-time error semantics

Positive control: SecurityPlugin appears in all four (1 / 1 / 2 / 1 hits) — which is why they were listed — so the searches reached the right pages and the logging-related zero is a real zero.

⇒ The change is to a module-private field's declaration and its default initialiser. No page documents that. The bot listed these correctly by its own rule (the symbol is named), and correctly says nothing about whether prose went false — it cannot see that axis (#9192).

⚠️ The bot's declared blind spot, measured rather than shrugged at

It flagged packages/services/service-settings/src/settings-service.types.ts as yielding no anchor, so "pages documenting it are NOT COVERED by this run" — and that file is limb (b), the SettingsDiagnosticsLogger note. Checked directly:

  • SettingsDiagnosticsLogger / diagnosticsLogger / "settings diagnostics" across content/docszero hits.
  • Control 1: the symbol really exists in the tree — 4 files under packages/services/service-settings/.
  • Control 2: the docs channel does resolve settings symbols — SettingsService hits 4 pages including references/system/settings-client.mdx.

Both controls pass, so the zero is real: no hand-written page documents that type, and the blind spot costs nothing on this diff.

Release-owned, untouched

content/docs/releases/implementation-status.mdx was listed read-only. ⛔ Not edited, and nothing in it was found to be wrong — so there is nothing to route as a docs-only card either.

No docs work outstanding on this PR. Recorded here so the contract reviewer does not have to re-derive it, and so a later reader does not read the bot's four rows as an unaddressed list.


Generated by Claude Code

… `warn`
Part of #10556 — repairs the `Type Check · debt ledger` failure on this PR.
`check:type-check-debt` re-measured `@objectstack/plugin-sharing` at 4 raw tsc
errors against a TEST_DEBT entry recording 3. The +1 is this PR's own:
sharing-service.test.ts(1585,7): error TS2741: Property 'warn' is missing in
type '{ error: (...args: any[]) => void; }' but required in type
'{ info?…; warn: (msg: any, ...rest: any[]) => void; error?…; debug?… }'
— the "required in type" is verbatim the signature limb (c) introduced.
Fixed at the test, never at the ledger: the entry stays at 3, the tightened
contract stays as ruled. Both channels land in the same `logged` array on
purpose — #9754's rule is that a fail-closed report DEGRADES to `warn` when
`error` is absent, so a double capturing only `error` is one that would not
notice the degradation this test exists to assert on.
The package's own `typecheck` and its vitest suite were both green while this
was red: the tsconfig hides tests from tsc (which is what TEST_DEBT measures)
and vitest does not typecheck. Reproduced with a probe tsconfig written INSIDE
the repo, mirroring `remeasureProject`, removed under a trap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@os-samClaude

Copy link
Copy Markdown
Collaborator

Landing — contract review CLEARED (verified as a PASS, not a drop), CI green, flipping ready

domain:services PM seat (session session_01APWX2AwT3a4xDcjPCe8bk4). Head 040932a131.

The label question, answered by measurement rather than by its absence

The implementer flagged that needs:contract-review had been removed from #10556 by a third actor at 21:41:05Z and correctly did not restore it. ⚠️ That is the moment the #10778 rule exists for: PASS + label absent = CLEARED; no PASS + label absent = DROPPED, and a drop is asked about, never re-hung. An absent label alone decides nothing.

Checked: contract review PASS posted at 21:40:53Z, twelve seconds before the label came off, with its tier verified by fuse (get_session.external_metadata.last_served_model = claude-fable-5, matched against CONTRACT_REVIEW_TIER read fresh from origin/main at dispatch-gates.mjs:3070) and its standing authorization named. ⇒ CLEARED. Nothing re-hung, nothing assumed.

⚠️The PASS was taken at bf036b4b09; the head is now 040932a131. I checked the delta myself rather than waving it through: 11 lines in one test file (plugin-sharing/src/sharing-service.test.ts), adding the now-required warn to a { error }-only spy in the [#6428] fail-closed block. It moves none of the three limbs the review assessed — no source, no ledger, no signature. The PASS still describes what is being landed.

⭐ And the fix is better than the type error demanded: both channels land in the samelogged array deliberately, because #9754's rule is that a fail-closed report degrades to warn when error is absent — so an error-only double is one that would not notice the degradation that test exists to assert. The type repair made the test stronger rather than merely compilable.

CI

31 of 31 green on the current head, including the two that were red — Type Check · debt ledger and its TypeScript Type Check aggregator — plus Lint & Repo Gates. Nothing pending.

⚠️ For the record, since it was mine to prevent: the debt-ledger red was predicted 20 minutes before CI found it. I measured that plugin-security (:1046) and plugin-sharing (:1060) are both in the ledger, so the declared narrowing did not hold — and then wrote it here as a landing precondition without also sending it to the implementer as work. Measuring a risk and delivering it are two steps, and I did one.

Open questions — both answered

1 — the corrected (b) descriptive fields: A, keep them. Reached independently by this seat and by the contract review, on the same ground: the ruled answer does not move (the assignability reason of record is about warn being optional, indifferent to whether it is declared), identity keys file::sink are byte-unchanged, and a row asserting a verdict the gate no longer renders is the un-re-measured claim that file's own header refuses. The review put it sharply — reverting to the ruled spelling "would re-create exactly the class of ledger lie this file exists to refuse."

2 — card disposition: A, Part of stands. The ruled shape lands complete (5 rows → 1, the survivor being the ruled permanent exception), so #10556 is expected to close on merge — but expectation is not completion, and a wrong Fixes closes a card silently while inbox filters read only open. This seat closes it by hand after the merge, not by prediction.

Landing: flipping ready and arming auto-merge; it holds for green.


Generated by Claude Code

@os-sam
os-sam marked this pull request as ready for review August 24, 2026 22:01
@os-sam
os-sam added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 18b53acAug 24, 2026
32 checks passed
@os-sam
os-sam deleted the claude/issue-10556-sink-ledger-ruled-shape branch August 24, 2026 22:41
os-trump pushed a commit that referenced this pull request Aug 25, 2026
…n every required warn (#10692)
Part of #10692 — the ruled remainder (option A, maintainer 2026-08-25) after
PR #11856 landed the three publicly exported producers under #10556 limb (c).
- record-orphan-cleanup.ts's module-local MinimalLogger drops its bare
Function members for the producers' real signatures and makes warn
REQUIRED: every report the sweep emits lands on warn, so a logger without
a guaranteed warn is one the sweep loses its only output into (#9754's
permit-silence shape, one module downstream of the tightened producers).
Zero in-repo call-site cost: both callers forward the owning services'
logger options, whose warn is already required.
- logger-required-warn.pin.ts pins the contract at compile time for all four
sinks (three public options types + the sweep parameter): a
{ info, error }-only literal is refused, a bare { warn } stub stays legal,
and a value of type Function no longer satisfies the members. Pins live in
a src-level .pin.ts because this package's tsconfig excludes tests from
tsc; the file deliberately declares no type literals with channel members
so check:optional-error-sink's census never reads it.
- logger-shapes.ts's shape map is updated: the record-orphan-cleanup row no
longer describes bare Function as blocked, and names the pin file.
- Changeset: minor with the published-contract break declared (BREAKING) and
the one-line host fix named, per the launch-window convention.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
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.

1 participant

@os-sam