test(db): prove the statement observer's production path is unchanged - #62
Merged
Conversation
- new `observe.ts`: `StatementObserver`/`StatementEvent`/`StatementAttribution` plus `setStatementObserver()` / `statementObserver()`, the `setDbClient` shape - uninstalled is one property read and one branch: the accessor returns the installed identity so the call site guards before building an event, and a throw from `onStatement` still reaches the caller (strict test mode) - one observer, not a list — a second install replaces the first (axiom 1) Co-Authored-By: Claude <noreply@anthropic.com>
- `runOn` (client.ts) and `statement()` (pglite.ts) split into a raw `sendOn`/`send` plus an observed shell: guard on the accessor first, so uninstalled is one property read and one branch — no clock, no event - both settle paths emit; a failure carries `rows: 0` and the wrapped X_DB_UNAVAILABLE the caller is about to be thrown - notify outside the statement's own `try`, or a throwing observer would be re-reported as a database failure and strict test mode would vanish - `rows` comes from the helper `execute()` already uses, so the report and the return value cannot disagree; reserve, boot and close stay out - split `pglite-embedded.test.ts` out of `pglite.test.ts` (500-line ceiling): fakes pin the adapter, the real WASM boot pins the binding Co-Authored-By: Claude <noreply@anthropic.com>
- `statement-span.ts`: `withStatementSpan` wraps the send alone — `db.<verb>`, OTel kind `client`, the text on `db.statement` - opened inside the observed branch, so installing a `StatementObserver` is the one switch for statement instrumentation and production allocates no span - `dev-traces.ts`: `db.` joins the prefix table as `sql`, so the `/_x` timeline gains DB children and `repeatedSql` groups SQL text instead of span names Co-Authored-By: Claude <noreply@anthropic.com>
- expectedQueryLoop(reason, fn) rides an AsyncLocalStorage; both funnels stamp the innermost reason onto StatementEvent.expected at settle time, inside the branch that already found an observer — prod pays nothing - one mechanism only: no comment pragma, no exempt-call-site list (axiom 1); a blank reason is X_INVARIANT, since an exemption with no argument is a pragma - applied at source to the framework's own deliberate loops: migrate()/rollback() (one transaction per migration) and admin search (one indexed lookup per field) - a verdict is suppressed, never a statement: the SQL still sends, still observes, still opens its span Co-Authored-By: Claude <noreply@anthropic.com>
- observe.test.ts: with no observer installed, neither driver ever reads the clock (the signal an event was about to be assembled), and a silent observer changes nothing about what a statement returns — closes the loop on client.ts/pglite.ts/statement-span.ts's existing "sees nothing" coverage by proving the uninstalled branch, not just its output - document, don't fix: jobs has no @ultimat3/db dependency, but the framework's own boot wiring (dev-queue.ts's startQueue, reached by every role) backs the job driver's PgExecutor with a real db client, so queue statements do pass through the observer today, unattributed — corrects the plan's "invisible by design" premise in packages/db/CLAUDE.md and packages/jobs/CLAUDE.md Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughChangesDatabase diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR adds statement observation while preserving existing query behavior, but events currently omit attribution and an edge case can produce different affected-row counts between database paths. The change is mergeable with explicit owner awareness and follow-up on these bounded observability and consistency risks. Sequence Diagram(s)sequenceDiagram
participant Client
participant StatementSpan
participant DatabaseDriver
participant StatementObserver
Client->>StatementSpan: send SQL statement
StatementSpan->>DatabaseDriver: execute statement
DatabaseDriver-->>StatementSpan: return rows or error
StatementSpan->>StatementObserver: report settled event
StatementObserver-->>Client: propagate observer result or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/admin/src/registry.test.ts`:
- Around line 201-235: Move the test named “search declares its per-field loop,
and the scope ends with it” from registry.test.ts to search.test.ts beside the
search implementation. Preserve its assertions and required fixtures or imports,
removing any registry-only setup that is no longer needed.
In `@packages/cli/src/dev-traces.test.ts`:
- Around line 34-45: Centralize the db.statement attribute key by exporting the
existing STATEMENT_ATTRIBUTE from statement-span.ts through the db package
index. Update dev-traces.ts and the fixture around withSpan to import and use
that exported symbol instead of literal "db.statement" values, preserving the
existing span shape and behavior.
In `@packages/db/README.md`:
- Around line 40-41: Date the load-bearing documentation claims with “As of
2026-07”: update the public observer and query-loop API descriptions at
packages/db/README.md:40-41 and the scope, observation, and framework
integration claims at packages/db/README.md:175-197. Ensure each affected claim
clearly carries the date without changing the documented behavior.
In `@packages/db/src/client.test.ts`:
- Around line 268-290: Add coverage for StatementEvent.expected propagation: in
packages/db/src/client.test.ts:268-290, run client.query inside
expectedQueryLoop, assert the observed event contains the reason, and assert a
statement outside the scope has undefined expected; in
packages/db/src/pglite.test.ts:361-387, add the equivalent assertion through the
in-transaction embedded funnel, ensuring the reason survives queued execution
and awaits. Make both tests fail if expectedQueryLoop scope is not attached to
emitted events.
In `@packages/db/src/expected-loop.test.ts`:
- Around line 138-149: Update the blank-reason test around expectedQueryLoop to
capture the thrown value as unknown and assert toBeUltimateError('X_INVARIANT')
after the call, removing the manual bare Error control-flow guard. Keep the
existing bare errors in the caller-body failure tests unchanged.
In `@packages/db/src/expected-loop.ts`:
- Line 6: Add a concise explanatory comment immediately above the
AsyncLocalStorage import, stating that Bun provides no non-node: async context
primitive and this Node API is therefore unavoidable. Preserve the existing
node: import and keep the explanation specific to this dependency.
- Around line 33-40: Add core’s X_INVARIANT code to the DB_BORROWED_ERROR_CODES
declaration in errors.ts so assertion failures from expectedQueryLoop’s assert()
are recognized by the database error-code set.
In `@packages/db/src/migrate.test.ts`:
- Around line 120-121: Update the test helper reasonFor to fail loudly when no
statement text matches the requested needle, rather than returning undefined
from an optional find result. Preserve returning the matched statement’s reason,
so the scope assertions distinguish an executed statement without a reason from
a statement that never ran.
In `@packages/db/src/observe.test.ts`:
- Around line 126-155: Move the performance.now spy cleanup into the existing
afterEach alongside the Bun.SQL and observer cleanup, ensuring it restores the
per-test spy even when assertions fail. Assign each test’s spy to the shared
cleanup variable, and remove the trailing clock.mockRestore() calls from the
three tests: “the pooled client never reads the clock,” “the embedded client
never reads the clock,” and “installing an observer is what makes the clock read
happen at all.”
- Around line 99-115: The production paths in runOn and statement do not
populate StatementEvent.attribution, making the field unused outside manually
constructed tests. Either add attribution generation and include it in events
emitted by runOn and statement, or remove the attribution field and update the
test and related types to match.
In `@packages/db/src/pglite.ts`:
- Around line 112-121: The client-side affected-row helper must match the
positive-count-or-row-count behavior of rowsOf in pglite.ts. Update affectedBy
in the client implementation to return the command count only when it is
positive, otherwise return the result row count, preserving zero for writes that
modify no rows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1921a43-070b-4827-9187-2157e51e90f3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!**/bun.lock
📒 Files selected for processing (25)
CHANGELOG.mdpackages/admin/CLAUDE.mdpackages/admin/package.jsonpackages/admin/src/registry.test.tspackages/admin/src/search.tspackages/cli/CLAUDE.mdpackages/cli/src/dev-traces.test.tspackages/cli/src/dev-traces.tspackages/db/CLAUDE.mdpackages/db/README.mdpackages/db/src/client.test.tspackages/db/src/client.tspackages/db/src/expected-loop.test.tspackages/db/src/expected-loop.tspackages/db/src/index.tspackages/db/src/migrate.test.tspackages/db/src/migrate.tspackages/db/src/observe.test.tspackages/db/src/observe.tspackages/db/src/pglite-embedded.test.tspackages/db/src/pglite.test.tspackages/db/src/pglite.tspackages/db/src/statement-span.test.tspackages/db/src/statement-span.tspackages/jobs/CLAUDE.md
- STATEMENT_ATTRIBUTE is exported from @ultimat3/db and imported by dev-traces.ts and its fixture: the key was a literal in three places, so renaming it left the timeline grouping nothing with tests green - attribution has no producer, and the docs now say so — observe.ts, db/CLAUDE.md, jobs/CLAUDE.md and CHANGELOG all claimed entity's driver threads it; every event in every process reads undefined today - affectedBy trusts the command tag only when > 0, the rule rowsOf already used: one answer per statement whichever driver is behind it - adminSearch's tests moved to search.test.ts, beside their source - reasonFor fails when nothing matched: find(...)?.reason answered undefined both for "ran outside every scope" and "never ran", and the toBeUndefined() assertions were about the first - the clock spy is restored in afterEach, not below the expect that can throw past it; the blank-reason test asserts on what it caught - node:async_hooks carries the why comment the repo rule requires - one more funnel test: the reason survives PGlite's turn queue and the awaits inside withTransaction - date the new README claims As of 2026-08 Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Looks good — CI is green and this is a routine feature PR (new statement observer API, minor semver impact). Ready to merge when you're ready. 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
observe.test.ts: with noStatementObserverinstalled, neitherclient.tsnorpglite.tsever reads the clock (the signal an event was about to be assembled) and a silently-installed observer changes nothing about what a statement returns — the "byte-identicalrunOnbehavior" guarantee, proven directly rather than inferred from "the observer saw nothing."@ultimat3/jobshas no@ultimat3/dbdependency and never will, but the framework's own boot wiring (packages/cli/src/dev-queue.ts'sstartQueue, reached by every role throughserve.ts/dev-runtime.ts) backs the job driver'sPgExecutorwith a real@ultimat3/dbclient — so queue statements do pass through the observer today, just unattributed. Corrected this inpackages/db/CLAUDE.mdandpackages/jobs/CLAUDE.md, since the plan's "invisible by design" framing overstated it.Test plan
bun test packages/db/src/observe.test.ts— 10 passbun test packages/db— 188 pass / 2 skipbun run verify— 12/17 green (5 honest skips: job, eval, drift, contract-diff, budgets)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Documentation