Skip to content

fix(plugin-auth): keep the logger receiver when selecting a log channel — audience refusals report their verdict instead of 500 null - #12803

Merged
os-litant merged 3 commits into
mainfrom
claude/issue-12773-logger-receiver-detach
Aug 27, 2026
Merged

fix(plugin-auth): keep the logger receiver when selecting a log channel — audience refusals report their verdict instead of 500 null#12803
os-litant merged 3 commits into
mainfrom
claude/issue-12773-logger-receiver-detach

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#12773

Selecting a log channel by extracting the method loses the receiver. a.b in call
position
passes a as the receiver; (a.b ?? c.d)(...) evaluates to the bare function
first, so the call runs with this === undefined. A plain-closure logger does not read
this and survives it — which is why no suite caught this. @objectstack/core's
ObjectLogger is a real class with prototype methods and no constructor binding, so it
threw and the audience refusal was delivered as HTTP 500 with a null body.

All verification below was run at commit 8b7ac8336.

Measured

Each claim names its file and line, and the command that produced it.

The defect site, re-located. The card says auth-manager.ts:3499; the PM's dispatch
says :3610. On this branch's merge base ead731756 it is at
packages/plugins/plugin-auth/src/auth-manager.ts:3610 — the PM's number, confirmed by
grep, not trusted from either source.

The host logger really is receiver-sensitive.packages/core/src/logger.ts:219
declares class ObjectLogger; :414 is error(...) calling this.writeErrorLike(...),
and :401/:405/:409 are debug/info/warn calling this.write(...). There is no
bind(this) anywhere in the constructor. So every channel of the real host logger
dereferences this, not just error.

Why the failure surfaced as 500 null rather than a logged warning. In
validateAudienceAdmission the call at auth-manager.ts:3724 sits inside a try, and
that block's catch at :3735 calls the same helper again. The first throw lands in
the catch, the second escapes the gate entirely — so the refusal reached neither the
caller nor the log. This is read from the control flow at those two lines.

Three live sites, all inside plugin-auth, all fixed here:

FileLine (at ead731756)ShapeChannel detached
src/auth-manager.ts3610(logger?.error ?? logger?.warn)?.(...)error, else warn
src/reconcile-membership.ts161const log = deps.logger?.error ?? deps.logger?.warnerror, else warn
src/adopt-membership.ts239const log = options.logger?.info ?? ...info

The second and third are siblings the PM's sweep did not find, because they are the
two-step const fn = obj.method; fn(...) form rather than the syntactic (a ?? b)(...)
one. Triage's scope ("fix any siblings inside plugin-auth in the same PR") covers them.
Each is now a property call that keeps its fallback.

Tests.pnpm --filter @objectstack/plugin-auth test — its own judgment line:

 Test Files 83 passed (83)
Tests 1695 passed (1695)

pnpm --filter @objectstack/plugin-auth typecheck — exit 0. Note it needs the package's
own dist built first: on an unbuilt worktree its second program
(tsc -p tsconfig.examples.json) fails with TS2307 on examples/basic-usage.ts, which
is a prerequisite, not a regression.

Gate union, re-derived live on the actual changed set with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack: 21 path-derived
families plus 6 convention-triggered ones (this PR adds a test file). All run, all exit 0,
including check:cross-package-test-inputs, check:engine-double-contract,
check:where-matcher, check:type-check-coverage, check:published-files,
check:test-source-alias and check:nul-bytes. pnpm lint
(eslint . --no-inline-config, whole repo) exits 0 — the full scan, not a narrowing.

check:type-check-debt (the ratchet half, run against a fully built closure) exits 0;
its own line: OK — 31 ledger entr(ies) re-measured in 233.1s, 1570 raw tsc error(s) total, none above its recorded number.

⚠️node scripts/pm/check-half-states.mjs returned exit 3 = PREREQUISITE NOT MET
no GitHub credential in this container. Its own text: "Nothing was swept ... it is no
reading at all."
Recorded as NOT MEASURED, neither pass nor fail. The lint.yml
wrapper check:pm-half-states exits 0.

The ablation — proof the pin can go red

Three legs, one per fixed site, each mutating the file back to the detached form and
running the whole package suite so "nothing else fails" is measured across all 1695
tests. Every leg proves the mutation reached the disk by git hash-object against the
HEAD blob (never an editor exit code, never a bare git diff --stat), and proves the
restore by hash equality plus empty git diff HEAD plus empty git status --porcelain.
The replacement asserts an exact hit count of 1 and aborts otherwise, so a zero-hit no-op
cannot be reported as a green ablation. A trap ... EXIT INT TERM on an absolute path
from git rev-parse --show-toplevel restores on any kill.

LegHEAD blobMutated blobResult
auth-manager.ts299bc028a3b5b5a8659c2 failed, 1693 passed
reconcile-membership.ts1a5476f9d86c7c07b7e72 failed, 1693 passed
adopt-membership.tsa96f3cfc6cff578d317b1 failed, 1694 passed

In every leg the failures were exactly the cases pinning that site and nothing else,
and each failed with TypeError: Cannot read properties of undefined (reading 'record')
structurally identical to the production reading 'writeErrorLike'. All three restores
verified clean.

No rebuild is involved: the pin imports the sites relatively (./auth-manager.js), so
vitest resolves package source, not dist. The mutation going red is itself the proof
that the source under test is the code that ran.

The pin, and why the double is class-based

src/logger-receiver-detach.test.ts. The double is a class whose channels dispatch
through this.record(...), mirroring ObjectLogger's this.write / this.writeErrorLike.
A bare-closure double passes against the broken code and pins nothing.

Case ⓪ asserts that directly, so the file cannot quietly become vacuous: it asserts the
class double throws when detached, and — beside it — that a closure double does
not, which records in executable form why the closure shape must never be substituted
here. Cases ①–② drive the real audience gate rather than the private helper, so what is
pinned is the user-visible contract: the refusal reaches the caller carrying
AUTH_CONFIG_ERROR and its message, and a host shipping only warn still gets the
fallback (#9754). Cases ③–④ cover the other two sites, each on both the error and the
reduced-sink path where one exists.

The sweep — what I covered, and what I did not

Re-derived rather than taken from the dispatch. Every regex was validated against a
synthetic positive control before its zero-hit result was believed, and two of them
were wrong on the first try and were fixed:

  • the (a ?? b)(...) regex initially matched nothing at all, including the known
    defect site — a broken instrument reading as a clean repo;
  • the assignment-detachment regex was anchored at ^ with an unindented control, so it
    missed every indented const, which is every real site. Fixing that anchor is what
    surfaced reconcile-membership.ts:161 and adopt-membership.ts:239.

Covered (packages + apps, source, excluding dist/node_modules):

  1. (a ?? b)(...) and (a ?? b)?.(...) — 1 site in plugin-auth (fixed); 9 call sites in
    packages/drivers/driver-sql/src/sql-driver.ts (:10575, :10744, :10791, :11408,
    :11415, :11426, :11482, :14162, :14515) plus 2 occurrences inside its docblock
    prose; packages/cli/src/utils/format.ts:1033;
    packages/cli/src/utils/dev-restart.ts:275; one test file.
  2. (a || b)(...) — zero, control validated.
  3. (cond ? a.m : b.m)(...) — zero real sites; every hit was a regex literal.
  4. Method-reference assignment (const f = o.m, including ??/|| chains) — the shape
    the PM flagged as unsearched. Found the two plugin-auth siblings fixed here.
  5. Destructuring (const { error } = logger) and method-as-callback
    (arr.forEach(logger.error)) — swept; no live defect in plugin-auth source.

Not covered — stated so nobody reads this as a clean bill:

  • Whether the 9 driver-sql sites actually crash is NOT established here, and this PR
    deliberately does not touch that package. They are carried on [finding] driver-sql has 10 inline (this.logger.error ?? this.logger.warn)(…) detach-then-call sites — the shape #12773 measured crashing, in a file that already contains the correct spelling #12792. I measured 9
    call sites where the dispatch brief said 10; the difference is not adjudicated here and
    belongs to that card. My regex is single-line, so a fallback split across lines would
    have been missed in either count.
  • Sweeps 4 and 5 are reliable only within plugin-auth, where I reviewed every hit by
    hand. Repo-wide they are dominated by false positives (value fields named error,
    expect(logger.error) assertions), so I make no repo-wide zero claim for those two
    shapes.
  • Detachment through an alias, a re-export, or a dynamic obj[name] lookup was not
    searched at all.
  • The four correct .bind() sites found incidentally
    (trigger-record-change/src/record-change-trigger.ts:289,
    trigger-schedule/src/schedule-trigger.ts:88,
    trigger-schedule/src/time-relative-trigger.ts:400,
    services/service-job/src/db-job-adapter.ts:253) are listed as evidence that the
    receiver-preserving idiom already exists in this codebase — they need no change.

Inferred, not measured

  • That a class-based logger is what real EE compositions inject into plugin-auth is taken
    from the issue's captured stack trace; I did not boot a composed deployment myself.
  • The dogfood report's 500 null is reproduced here at the level of mechanism (the
    escaping second throw, and a TypeError of the same shape), not by driving HTTP.

Scope notes

No security label and no target:v17, per triage: the gate still refuses and fails
closed; the cost is diagnostic. Clause ② stays no — re-derived live on the actual
changed set, the union names no contract family, so the falsifiable condition the
dispatch set did not trigger.

One thing checked and deliberately not filed: plugin-auth/tsconfig.json excludes
**/*.test.ts, so no tsc program compiles this package's 82 test files. That is not an
undeclared gap — it is a declared, ratcheted TEST_DEBT ledger entry
(scripts/check-type-check-coverage.mjs:895, errors: 97), already governed by #6376.
The new test file adds 0 errors to it: measured at 94 with this diff applied and 94
with the three source files reverted to ead731756 and the new test parked, so the
ledger's -3 drift is entirely pre-existing and none of it is mine.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth, touching 4 documentable anchor(s).

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

  • content/docs/kernel/contracts/auth-service.mdx(via AuthManager (symbol))
  • content/docs/kernel/services-checklist.mdx(via AuthManager (symbol))
  • content/docs/permissions/authentication.mdx(via AuthManager (symbol))
What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 11 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 96dc446c9c19063edfae26ae30ff75143ef0c5b7packageMentionDocs.

Which tree this was computed on

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

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

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

2 participants

@os-litant@claude