Skip to content

docs(core,service-cluster): retire the two docblocks left stale by IPubSub's corrected delivery guarantee (#12836) - #12954

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-12836-ipubsub-docblock-staleness
Aug 28, 2026
Merged

docs(core,service-cluster): retire the two docblocks left stale by IPubSub's corrected delivery guarantee (#12836)#12954
os-litant merged 1 commit into
mainfrom
claude/issue-12836-ipubsub-docblock-staleness

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#12836

#12651 (PR #12837) corrected IPubSub's contract docblock: delivery is whatever the configured driver declares, no shipped driver exceeds at-most-once, a missed message is EXPECTED, handlers must be idempotent and tolerate loss, and deliverySemantics is the per-channel ask-not-get surface.

Two docblocks elsewhere still described the pre-#12651 world. This PR retires both. Prose only — no behaviour change, no test change.

Site 1 — packages/core/src/security/authz-invalidation-channel.ts

The docblock carried a paragraph asserting, in the present tense, that the interface docblock "still says" At-least-once delivery, and that repairing it "is a packages/spec change and is filed separately, deliberately not made here."

That paragraph declared its own expiry condition, and the condition is now met: the separate filing was #12651, and it has landed. Leaving it is worse than never having written it — the next author goes looking for a live disagreement between the interface and the drivers, and finds none. Replaced with a plain pointer to the now-honest interface docblock.

Everything else in that docblock is unchanged, verbatim, because it is all still correct: the at-most-once reasoning with its three measured citations, the TTL-is-the-bound rule, and the best-effort-at-the-publish-site note ("a publish failure is logged and swallowed, never propagated into the write that triggered it").

Site 2 — packages/services/service-cluster/src/memory/pubsub.ts

The line was:

 * - At-least-once semantics held vacuously (a single in-process delivery).

This line was wrong on its own terms even before #12651, which is what makes the change more than a consequential edit. The same docblock, two bullets up, states that handler errors are "swallowed and logged via onError (so one bad subscriber can't poison the bus)". A handler that throws therefore loses the message outright: no retry, no persistence, no replay. That is not at-least-once in any sense — "vacuously" does not save it, because the single delivery attempt can itself fail and nothing redelivers.

Confirmed against the implementation in the same file, not only its prose:

  • a synchronous throw goes to this.onError(err, channel) and is dropped;
  • a rejected async handler goes to .catch((err) => this.onError(err, channel)) and is dropped;
  • publish() returns early when a channel has no subscriber bucket, so a publish nobody is subscribed to at that moment is a silent no-op.

Replaced with the honest statement: at-most-once, one synchronous in-process attempt per subscriber, no persistence, no retry, no replay.

Blast radius — the nine other At-least-once sites are deliberately untouched

A repo-wide grep for At-least-once returned eleven hits before this change and nine after. The nine that remain are all different subjects, and two of them sit on governed surfaces:

SiteWhy excluded
packages/triggers/trigger-api/src/api-trigger.tsADR-0041 inbound API-trigger events — different subject, different transport
docs/adr/0041-flow-trigger-family.mdgoverned surface
docs/adr/0119-plugin-reachable-transactions-and-honest-atomic-batch.mdgoverned surface
.changeset/ipubsub-delivery-guarantee-docblock.md (2 hits)#12651's own changeset, quoting the old text deliberately — rewriting it would falsify the release record
content/docs/automation/webhooks.mdxwebhook receivers — different subject, and the claim there is true
packages/objectql/src/engine.tsat-least-once hook semantics — different subject
packages/services/service-messaging/src/dispatcher.tsmessaging outbox — different subject, and true
packages/services/service-messaging/src/http-dispatcher.tssame, HTTP leg

Verification

Every run below is on e50b1d2dd, this PR's head commit, with a clean working tree.

Prose-only, proven rather than asserted. Both changed source files were compiled at the merge base and at head with removeComments: true, and the emitted output compared byte for byte:

IDENTICAL comment-stripped emit packages/core/src/security/authz-invalidation-channel.ts
IDENTICAL comment-stripped emit packages/services/service-cluster/src/memory/pubsub.ts
PROSE-ONLY PROVEN: comment-stripped emit is byte-identical for every changed source file.

There is no ablation leg in this PR, deliberately. This is a comment-only change: no pin can be made to go red by mutating prose, so an ablation here would be fabricated rather than measured.

Tests — both affected packages, green:

  • pnpm --filter @objectstack/core testTest Files 41 passed (41), Tests 1015 passed (1015)
  • pnpm --filter @objectstack/service-cluster testTest Files 4 passed (4), Tests 66 passed (66)

Lint — repo-wide, not narrowed: pnpm lint (eslint . --no-inline-config) exits 0 with no findings, so no narrowing is claimed or owed.

Gate families — derived from the actual changed set by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, which read the merge base itself (3 paths, commit e50b1d2dd, repo assertion held). All 21 named families are green, plus check:nul-bytes. Selected judgment lines, each as printed by the gate itself:

✓ affected-docs self-test: 457 cases pass.
✓ check-drift-comment: 56 cases pass across 5 fixture diff(s).
OK check:comment-mask-adoption — 14 private comment-stripper(s) under packages/** + examples/**, all 14 recorded and every recorded row still reached
check-test-source-alias OK — 72 packages with tests scanned
check-type-check-coverage: OK — 65/78 workspace packages type-checked (plus the root), 13 in the DEBT ledger

Typecheck. Neither @objectstack/core nor @objectstack/service-cluster declares a typecheck script, so a filtered run typecheck would match nothing and exit 0 — not a measurement. Ran npx tsc --noEmit -p tsconfig.json from inside each package instead. Both exit 2, entirely on pre-existing diagnostics in test files these packages hide from tsc; both sit in the type-check DEBT ledger, and check-type-check-coverage is green on that ledger. The comment-stripped-emit proof above is what establishes this PR cannot have moved those counts.

One gate NOT MEASURED — reported as such, not folded into the green list.node scripts/pm/check-half-states.mjs (the live board sweep, whose standing caller is half-state-patrol.yml) exits 3:

check-half-states: PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential

That is an environment classification, not a verdict on this PR, and lint.yml says so where it wires the gate: the live sweep's "non-zero exits classify the ENVIRONMENT — no token, exhausted quota, unreachable host — which is not a verdict about the PR running it." The form CI runs in the PR lane is the offline self-test, pnpm check:pm-half-states, which is green here.


Generated by Claude Code

…ubSub's corrected delivery guarantee (#12836)
#12651 corrected IPubSub's contract docblock to state delivery driver-relatively:
no shipped driver exceeds at-most-once, a missed message is EXPECTED, handlers
must be idempotent AND tolerate loss. Two docblocks elsewhere still described the
pre-#12651 world.
- packages/core/src/security/authz-invalidation-channel.ts: the 'known
contradiction' paragraph asserted in the present tense that the interface
docblock 'still says' At-least-once, and that repairing it was filed
separately. That filing landed, so the paragraph is now false, not merely
stale. Replaced with a plain pointer to the interface docblock; the rest of
the docblock is unchanged.
- packages/services/service-cluster/src/memory/pubsub.ts: 'At-least-once
semantics held vacuously' was wrong on its own terms even before #12651 --
handler errors are swallowed via onError, so a throwing handler loses the
message with no retry and no persistence. Replaced with the honest statement.
Prose only. No behaviour change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

⚠️2 changed file(s) yielded no anchor (packages/core/src/security/authz-invalidation-channel.ts, packages/services/service-cluster/src/memory/pubsub.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files. Nothing else in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 2 changed package(s)).

What this run could not see
  • 2 changed file(s) yielded no anchor (packages/core/src/security/authz-invalidation-channel.ts, packages/services/service-cluster/src/memory/pubsub.ts) — pages documenting those are invisible to this run
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 26 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 c94be6291d4ca988670866e325df840e7fd4ed41packageMentionDocs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tooling labels Aug 28, 2026
@os-litantClaude

Copy link
Copy Markdown
CollaboratorAuthor

Review — ACCEPT, pending CI. Verified against the diff and the tree, not the report's self-description.

PM seat domain:services, session session_0194kbQJxUvv2yvsGRtuXpP5.

Scope and blast radius — held exactly

CheckReading
Changed files (get_files, not self-report)3 — the changeset and the two pinned sources. Nothing else.
Site 1 fence−7/+6 confined to the "Known contradiction" paragraph. The best-effort-at-the-publish-site note immediately above and the ## Why a new channel… section immediately below are untouched in the diff context
The nine excluded At-least-once sitesUntouched by construction — the PR contains 3 files and none is an ADR, trigger-api, webhooks.mdx, engine.ts, either messaging dispatcher, or #12651's own changeset
⛔⛔ Governed surfacesdocs/adr/0041-* and docs/adr/0119-* not in the file list; Governed Surface Queue Guard green
Changesetpatch on both @objectstack/core and @objectstack/service-cluster, as scoped

The dispatch's biggest risk was a full-text search "helpfully" fixing the other nine. It didn't happen.

⭐ The new prose is accurate against the CODE, not just against the card

The card asserted the old memory-driver line was wrong on its own terms. The dev didn't stop at that claim — it read the implementation and wrote the docblock from it. I re-verified all three legs independently on the branch:

  • packages/services/service-cluster/src/memory/pubsub.ts:63if (!bucket || bucket.size === 0) return; ⇒ the "publish to a channel nobody is subscribed to is a silent no-op" clause is measured, and it is a fact the card never mentioned;
  • :76(result as Promise<void>).catch((err) => this.onError(err, channel)); ⇒ a rejected async handler is swallowed;
  • :78-79catch (err) { this.onError(err, channel); } ⇒ a synchronous throw is swallowed.

⇒ "no persistence, no retry, no replay" is derived from the code, not paraphrased from the issue. That is the correct standard for a docblock repair, and it made the replacement text strictly better than what the card asked for.

Site 1's replacement also does the right thing structurally: it does not merely delete the stale paragraph, it leaves a pointer saying the interface, cluster.mdx §4.2 and the redis driver now agree — which is what stops the next reader from going looking for the disagreement the old paragraph advertised.

⭐ The ablation call — declining to fabricate one, and substituting something better

This is the part worth recording. Told an ablation was expected in the general case, the dev correctly reported that a comment-only change admits none, and ⛔ did not invent one. In its place it ran a real positive measurement of the very property under review: both changed files compiled at the merge base and at head with removeComments: true, emitted output compared byte for byte — identical.

That instrument is better designed than an ablation would have been here, and it does two jobs at once:

  1. it proves the change is genuinely prose-only, rather than asserting it;
  2. it independently confirms the clause-② no grading instead of taking my dispatch's word for it — a comment-only change cannot move contract accept/reject behaviour.

It also disposes of the one loose end honestly: npx tsc --noEmit -p tsconfig.json exits 2 in both packages on pre-existing diagnostics in hidden test files. Left as a bare claim that would need investigating; backed by the byte-identical emit, it is provably not a regression this PR could have caused. check-type-check-coverage is green on the ledger entries.

Reporting discipline

node scripts/pm/check-half-states.mjs → exit 3, PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential, reported as NOT MEASURED and ⛔ not folded into the green list, quoting the gate's own line: "it is not a clean board and it is not a dirty one — it is no reading at all." Correct. The offline sibling pnpm check:pm-half-states is green, and that is the form CI runs in the PR lane.

My false-green warning was confirmed rather than assumed: neither package declares a typecheck script, so pnpm --filter … run typecheck would have matched nothing and exited 0.

⚠️ Environment note worth carrying to other seats

The dev reports the zero-quota web read-back channel served a stale cached snapshot of #12836 — showing 3 comments and omitting both my claim comment and the dev's own freshly-posted report. A naive reading would have scored that as "my comment was eaten by the sanitizer" and prompted a duplicate repost. The authoritative MCP read confirmed both were intact. ⇒ ⛔ Do not treat a web-channel read-back as evidence a comment failed to post.

Landing

⛔ Not armed yet: CI is still running (Test Core shards, Dogfood, Lint & Repo Gates, three Type Check jobs all in_progress). Entry qualification is every check green, not the required subset — arming follows when the run completes.


Generated by Claude Code

@os-litant
os-litant marked this pull request as ready for review August 28, 2026 11:26
@os-litant
os-litant enabled auto-merge August 28, 2026 11:26
@os-litant
os-litant added this pull request to the merge queueAug 28, 2026
Merged via the queue into main with commit 2d5cee3Aug 28, 2026
34 checks passed
@os-litant
os-litant deleted the claude/issue-12836-ipubsub-docblock-staleness branch August 28, 2026 11:47
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.

Two docblocks still reference IPubSub's retired "at-least-once" claim after #12651 corrects it

2 participants

@os-litant@claude