Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): keep the logger receiver at the nine detach-then-call sites — a class-based host logger no longer turns a durability warning into a TypeError - #12821
Conversation
…l sites `(this.logger.error ?? this.logger.warn)(…)` evaluates to the bare function and then calls it, so the call runs with `this === undefined`. The eight `error ?? warn` sites now call `logDurabilityFailure()` — the property-access helper this class already had three lines from the docblock that explained it. The ninth is `info ?? warn` and reports a reconcile that SUCCEEDED, so it keeps its level with an in-place property-access spelling rather than being escalated to the durability channel. The `:4290` docblock is rewritten: its closing paragraph explained why the inline sites were left alone, next to code that no longer uses that shape. Part of #12792
…d a shape scan Three sections. ⓪ asserts the doubles — and `@objectstack/core`'s real `ObjectLogger` — are receiver-sensitive, and that a closure double is not, so the file cannot go quietly decorative. ①/② drive the real reconcile and the real declared-index sync (real sqlite, real duplicate rows) against a class-based logger and against a warn-only one. ③ is a structural pin over `sql-driver.ts`: an AST walk for all four detach shapes, with a control sample proving it fires — including on a fallback split across lines, which the two single-line counts on this card could not see. Part of #12792
…ns red Prediction, committed before the mutation so it cannot be written after the fact. Reverting the durability site in `syncDeclaredIndexes` to `(this.logger.error ?? this.logger.warn)(…)` must produce, against the class-based double: ① §2 red — the detached call throws `TypeError: Cannot read properties of undefined (reading 'record')`, escaping `syncDeclaredIndexes`, so the `resolves.toBeUndefined()` assertion rejects instead. The mirror of the production shape, where `ObjectLogger.error` reaches `this.writeErrorLike`. ② §2's warn-only case red for the same reason (the FALLBACK leg detaches too). ③ §2's real-ObjectLogger case red — the platform logger, same path. ④ §3 red — the structural scan reports one `parenthesized-callee` finding. ⑤ §0 and §1 stay GREEN: they do not touch this site, so a red there would mean the mutation was not the thing measured. Direction: RED. Not "fewer diagnostics" and not a reversal — the assertions are behavioural and the scan is a direct count of the shape. Part of #12792
…t shape The second ablation, because §1's assertions are about a different site on a different channel and the first one does not cover them. Reverting the `info` site in `reconcileAndWarnDrift` to `(this.logger.info ?? this.logger.warn)(…)` must produce: ① §1 red — but NOT as an escaping TypeError. That site sits inside the reconcile's own `try`, so the throw is CAUGHT and re-reported as `[schema-drift] dev auto-reconcile failed … — falling back to warning`. So the failure is `infos` empty AND a bogus failure line present: a reconcile that really happened, announced as a failure. ② §1's warn-only case red the same way (the fallback leg detaches too). ③ §3 red — one more `parenthesized-callee` finding. ④ §0 and §2 stay GREEN. Direction: RED, via SWALLOW-AND-MISLABEL rather than a propagating throw. That asymmetry is the point of running this one separately. Part of #12792
…st the line wanted The info site's defect has two halves and the second one is a line that must NOT be present: the site sits inside the reconcile's own `try`, so a detached call is caught and re-reported as `dev auto-reconcile failed`. Asserting the full transcript puts both halves in one diff instead of hiding the mislabel behind an earlier expectation that fails first. Part of #12792
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 48e4840648a583d0c6fc110eb0f20a3b6053c9f7 && git checkout 48e4840648a583d0c6fc110eb0f20a3b6053c9f7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 632e862d14497e882885e0a75f5c31cc61c5186d 7d64433d83d22ff7785caf70effbab76d1cec93c && git checkout -B drift-repro 632e862d14497e882885e0a75f5c31cc61c5186d && git merge --no-ff 7d64433d83d22ff7785caf70effbab76d1cec93c
node scripts/docs-audit/affected-docs.mjs --json 632e862d14497e882885e0a75f5c31cc61c5186d
|
os-zhuang
commented
Aug 28, 2026
Reviewer-of-record notes. Three things, one of which is a correction to my own dispatch brief. 1. The release-owned page, audited (read-only, but the check says audit it)
Not falsified — repaired. Before this PR, those sites handed a receiver-sensitive logger logged neither The page's other four 2. The six hand-written rows are coarse-anchor noise, and the check invites saying soEvery row was matched on the anchor This is the mirror of the failure mode the check documents at length. It reasons carefully about pages it cannot see; a whole-class anchor is the case where it sees far too many. Recording it as the check asks — a wrong row is reportable rather than merely annoying. 3. ⭐ The nine sites are not one shape, and my dispatch brief said they wereMy brief said "convert the 9 to the named helper", carrying the triage's framing without questioning it. That was wrong, and the seat caught it: eight are Routing that one through the durability channel would have escalated a functional report from The ablation makes the distinction executable rather than asserted: two predictions, committed empty before either mutation, predicting failure in different shapes. Reverting a durability site rejects; reverting the info site is swallowed by the reconcile's own 4. The gate hypothesis came back NEGATIVE, which is the valuable outcomeTriage's both-axes hypothesis — that converting the 9 also closes false silent-swallow sources for ⭐ Also worth naming: the instrument's zero-hits are proven twice — the same AST scanner run over historical Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#12792
Nine sites in
packages/drivers/driver-sql/src/sql-driver.tsselected a log channel by extracting the method before calling it.a.b(…)passesaas the receiver;(a.b ?? c.d)(…)evaluates to the bare function first, so the call runs withthis === undefined. Eight now calllogDurabilityFailure()— the property-access helper this class already had, three lines from the docblock that explains it. The ninth reports a reconcile that succeeded, so it keeps itsinfolevel with an in-place property-access spelling instead of being escalated onto the durability channel.⭐ The first deliverable was a measurement: what logger does
SqlDriveractually get?Nothing composes a logger into
SqlDriverat all today. Measured onorigin/mainat23843d3f4:driver-sql/src/index.tsonEnableloggerfrom the plugin context, uses it for its own two lines, and buildsnew SqlDriver(config)— never passes it inSqlDriverconstructorschemaMode/autoMigrate/sqliteJournalMode/sqliteAbsentFileand hands the rest to knex; reads nologgerkeySqlDriver.loggerfieldconsole.warn/console.errordriver.logger = …in the repo*.test.tsor inpackages/runtime/src/expected-read-refusal-noise.ts, a testkit whose only importers are integration testsSqliteWasmDriverconstructorif (config.logger) this.logger = config.logger— inherited straight into this class. The one production seam that can install one; no caller passes it yet.So these were latent, not live. ⛔ That was not treated as grounds to close: a call that runs with
this === undefinedis a defect whatever today's wiring tolerates, andSqliteWasmDriver's constructor is a one-line change away from making it live. The fix landed regardless, which is what the card asked for.The receiver half was already settled from the other end and is re-confirmed here as an executable assertion:
@objectstack/core'sObjectLoggerdereferencesthison every channel —error/fatalviathis.writeErrorLike,debug/info/warnviathis.write, declared as methods atlogger.ts:401/405/409/413.Both counts this card carried came from single-line regexes, so 9 was published as a floor. It was re-derived here with an AST instrument — comments are not nodes and line breaks are not syntax, so both of that instrument's blind spots are closed — and swept for the three shapes a parenthesized-callee regex cannot see at all.
driver-sql(a.x ?? a.y)(on one lineconst fn = a.x ?? a.y;…fn(…)const { error } = this.loggerrun(this.logger.warn)The three ② candidates, read rather than counted:
hostAfterCreate = pool.afterCreatetwice (a host-supplied knex pool hook — a plain config callback, not a method with a receiver contract) andrunner = trx ?? this.knex(a knex instance is a callable object designed to be invoked standalone). None is a log channel.Zero-hit discipline. A zero from ②③④ is only a reading if the instrument fires, so it was proven twice on real code: against the pre-fix blobs of the sibling card (
git show 26deb31a0^:…) it reports ① atauth-manager.ts:3610and ② atreconcile-membership.ts:161andadopt-membership.ts:239— exactly the three that card found, including the two its regex could not see. A synthetic control carrying all four shapes plus a multi-line fallback is checked into the pin as its own test case (§3), where the regex finds 1 and the AST finds 2.Swept repo-wide too, over 5,096 tracked
.tsfiles: outsidedriver-sqland the already-landedplugin-authfix there is no further live instance. The 2 remaining ① sites are onconsole(bound in Node and in browsers), and the 25 remaining ② sites are the idiomatic options-callback shape — a caller-supplied plain function with aconsoledefault (opts.warn,opts.info), not a method lifted off a receiver-sensitive class. Filed as an observation — see below.The triage's hypothesis was that converting the nine would close two axes at once: the receiver bug and nine false silent-swallow sources for
check:durability-degradation-log-level. Verified against the gate rather than asserted, before and after:The reason is measurable: none of the nine sits inside a
catchguarding aDURABILITY_CRITICAL_CALLEESoperation, so the checker never discovered them and they produced no findings to begin with. The two driver seams it does discover (runWideningAlters, at:9952/:10057before this diff) already reachedlogDurabilityFailureand are still classifiedrecovers on one branch, loud (error@… via logDurabilityFailure())after it —--listdiffers only in the line numbers the docblock rewrite shifted. Reason ② is why the helper exists; it is not what this conversion bought.The docblock at
:4290needed rewriting, and was rewrittenIts closing paragraph explained why the inline sites were left alone — prose that would now sit next to code that no longer uses that shape, which is exactly the fossil this repo keeps paying for. It now carries three reasons instead of two, with the receiver as the load-bearing one, and records three things a future reader would otherwise have to re-derive: that the eight were converted and why they had been left (the receiver question was not in scope then, not that they were judged safe); that the ninth must not be converted, because escalating a functional report onto the durability channel is the over-application AGENTS.md names; and that reason ② was measured not to improve, so nobody re-litigates it.
Tests
packages/drivers/driver-sql/src/logger-receiver-detach.test.ts— 11 cases, 3 sections.this, and §0 asserts that directly — plus that a closure double survives the same detachment, which is why this defect had no red test in the first place. It also asserts that@objectstack/core's realObjectLoggerthrows on every detached channel, which is the link that makes this card a defect rather than a style preference.autoMigrate: 'safe'. Asserts the whole log transcript, because the second half of the defect is a line that must not be there.CREATE UNIQUE INDEXthat refuses. Run against a class-based logger, against a warn-only one (the fallback leg is held to the same standard), and against the platform's realObjectLogger.sql-driver.tsitself, with the control sample as its own preceding test case so a green §3 can never mean "the scanner stopped working".Ablation, both channels, prediction committed first
Each prediction is an empty commit made before the mutation (
79a297735,64fd23506), so it cannot have been written to fit the result. Every leg proved the mutation on disk with anchored greps in both directions before running, and proved the restore withgit hash-objectagainst the HEAD blob plus an emptygit diff HEAD; the mutation scripts carrytrap … EXIT INT TERMwith absolute paths.A — the durability site (
syncDeclaredIndexes), reverted to(this.logger.error ?? this.logger.warn)(. Predicted red on §2's three cases and §3; §0 and §1 green. Observed exactly that:The second frame is the production failure this card describes, reproduced against the platform's own logger.
B — the info site, reverted the same way. Predicted red in a different shape: that site sits inside the reconcile's own
try, so the throw is caught, not propagated. Observed exactly that — no rejection, and a transcript that is worse than an empty one:⭐ Both lines are false. The reconcile succeeded — the index was created before the log call — and the throw skipped the post-reconcile re-detect, so the driver reports a successful reconcile as a failure and then tells the operator to run
os migrate applyfor an index that already exists. That third harm was not in the prediction; it is the loop shape #11722 documents, reached from a different direction.dist/is involved in either ablation, and the stack frames prove it: they namesrc/sql-driver.tsand../../core/src/logger.ts. The pin imports the driver relatively and this package's vitest config aliases@objectstack/coretocore/src/index.ts, so both sides of the measurement are the source that was mutated.Gates run locally, at this exact tree (
7d64433d8)Green:
check:nul-bytes·check:changeset-gate-self-tests·check:cross-package-test-inputs·check:driver-conformance·check:objectql-double-limit·check:objectui-changeset·check:page-declaration-shape·check:pm-half-states·check:published-files·check:slot-lookup·check:test-source-alias·check:type-source-resolution·check:engine-double-contract·check:where-matcher·check:query-options-erasure·check:type-check-coverage·check:durability-log-level·check:type-check-debt(--re-measure, 31 entries, after building the closure exactly aslint.ymldoes) ·check-adr-0087-registration·check-changeset-no-major·check-ci-filter-parity·check-comment-mask-adoption·check-empty-changeset·check-plugin-teardown-shape·release-rehearsal-clone --self-test.Package suites:
@objectstack/driver-sql144 files / 2206 tests passed, 8 files / 129 skipped;@objectstack/driver-sqlite-wasm(the one subclass) 26 files / 439 tests passed;@objectstack/driver-sql typecheckclean, andtsc --listFilesconfirms the new pin is one of the 152 test files inside that program rather than excluded from it.NOT MEASURED, not red:
scripts/pm/check-half-states.mjsrefused withPREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential(exit 3, its own distinct code). Nothing was swept, so it is no reading at all; it needs a real credential and CI has one.Declared narrowing: the repo-wide
pnpm lintsweep was not run here — CI runs the farm exactly once regardless. Everything above is the affected package plus the derived families, re-derived from the actual diff on this head withnode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(3 paths, 22 path-matched families plus the convention-triggered ones), not from a list carried in the brief.Out of scope, filed not fixed
The repo-wide sweep above turned up no live sibling defect, but it did establish that this class has now been fixed card-by-card three times with no guard behind it, each round finding the next round's population by hand with a weaker instrument than the last. Recorded in #12820 — filed unassigned and labelled
finding, ungraded, with the measured population (2 benignconsolesites, 25 legitimate options-callback sites, 0 live) and an explicit note that no gate is proposed because the false-positive surface has not been measured. #12820 is not addressed here. Routing is triage's.Generated by Claude Code