Skip to content

fix(hooks): query by where, not filter, on ctx.api reads - #573

Merged
yinlianghui merged 2 commits into
mainfrom
claude/l2-hook-filter-predicate-ogstlu
Jul 31, 2026
Merged

fix(hooks): query by where, not filter, on ctx.api reads#573
yinlianghui merged 2 commits into
mainfrom
claude/l2-hook-filter-predicate-ogstlu

Conversation

@yinlianghui

@yinlianghui yinlianghui commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Description

Hook handlers were reading records by a query key the kernel silently drops.
Seventeen hook-side ctx.api calls passed their predicate as filter: instead
of where:. On the ObjectQL 16.1.0 read paths that key is not a synonym — and
the mismatch produces no error, no warning, and no empty result. It produces the
wrong record.

Reproduced against a real ObjectQL engine (in-memory driver, real
ScopedContext — the exact object the kernel injects as ctx.api), two rows
seeded, asking for the second by id:

findOne({ where:  { id: <row2> } })  ->  row2   ✅
findOne({ filter: { id: <row2> } })  ->  row1   ❌ the object's first row

findOne({ where:  { id: 'no_such_id' } })  ->  null   ✅
findOne({ filter: { id: 'no_such_id' } })  ->  row1   ❌ a record, not null

count({ where:  { annual_revenue: 100 } })  ->  1   ✅ (1 of 3 rows matches)
count({ filter: { annual_revenue: 100 } })  ->  3   ❌ the whole object

Root cause is per-method, which is why it looked like filter "mostly worked" —
in @objectstack/objectql@16.1.0, dist/index.js:

method code result
find if (ast.filter != null && ast.where == null) ast.where = ast.filter aliases — works
findOne const ast = { object, ...query, limit: 1 } no alias; unknown key dropped → predicate gone, limit: 1first row
count ast: { object, where: query?.where } reads where only → predicate gone → whole-object count

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Related Issues

n/a — found by inspection while auditing the L2 hook sandbox read path.

Changes Made

  • src/objects/_hook-api.ts — dropped filter? from HookQuery. This is the
    root fix: the compiler now rejects the spelling at authoring time instead of
    leaving it to be discovered in the data. Removing it surfaced every call site
    at once, including ten the original report had not identified.
  • Fixed 17 hook-side call sites across account, campaign, case,
    contract, opportunity, opportunity_line_item, product, quote,
    quote_line_item, and the shared _line-item-price-fill.ts.
  • test/helpers/hook-harness.ts — the harness resolved its predicate as
    q.filter ?? q.where ?? {}, accepting both spellings. That made the stand-in
    more permissive than the kernel it stands in for, so every affected hook
    tested green while querying wrong in production. It now throws on filter.
  • test/hook-query-predicate.test.ts (new) — pins the contract against a
    real kernel rather than the harness, characterises the kernel's
    silent-drop so the trap stays documented, and adds a source scan over
    src/objects/**.ts that fails any file reintroducing the key.
  • src/actions/lead.actions.ts — normalized to where:. Behaviour-identical
    (it is a find), but it was the seed of the next copy-paste.

Impact — what was actually computed wrong

The hypothesis named four hooks; the compiler found eight files plus a shared
module. The two most consequential were not in the original list:

Site Read Consequence
_line-item-price-fill.ts (both line-item objects) product by id, for list_price Wrong money on the record. list_price — and unit_price on create — defaulted from the first product in the catalog, not the one the rep chose. Silently mispriced line items flow into opportunity amount and quote totals via the rollups.
product.hook.ts count of referencing line items Delete guard counted every line item in the object. Blocks deletion of any product, with an invented reference count in the error text. Fails closed, so no data loss — but the stated number was never real.
account.hook.ts count of open opportunities Same shape: whole-object count. Customer accounts became undeletable, citing opportunities that do not reference them.
campaign.hook.ts count of opportunities / converted leads Campaign ROI snapshots recorded whole-object counts as campaign attribution — every completed campaign credited with every opportunity and lead in the system.
quote.hook.ts opportunity by id, for its stage gate Quote acceptance checked a stranger's deal stage, then wrote closed_won to the correct one. Either closes a deal the gate should have stopped, or skips one it should have closed.
opportunity.hook.ts, contract.hook.ts account by id, for type !== 'customer' Account promotion gate read off an unrelated account; the write targeted the right id. Causes a skipped or redundant promotion, not a cross-account write.
case.hook.ts account by id, for owner Escalated-case follow-up task assigned to the first account's owner — lands in the wrong rep's queue.
opportunity_line_item.hook.ts, quote_line_item.hook.ts (rollups) parent by id, for the closed/accepted freeze guard Rollup freeze gate evaluated against the wrong parent — a settled deal's amount could be rewritten, or a live one's skipped.

Writes were consistently targeted by explicit id, so no hook wrote to the
wrong record
. The damage is in the reads: gates decided on someone else's row,
and money and metrics computed from it.

Note on the main merge

main advanced while this was open, and #570 extracted the two near-verbatim
price-fill handlers into a shared src/objects/_line-item-price-fill.ts. That
extraction wins in the merge, and it had carried the filter: along with it
— so the fix moves into the shared module.

That is also why the source scan covers every .ts under src/objects/ rather
than the *.hook.ts glob it started with: a guard keyed to a filename
convention would have missed the one instance most likely to survive a
refactor. The scan now asserts it can see _line-item-price-fill.ts, so its
coverage cannot silently narrow again. The rollup fixes in both line-item hooks
merged cleanly and are unchanged.

Testing

  • Unit tests pass (pnpm test) — 25 files, 480 passed / 2 skipped
  • Linting passes (pnpm lint)
  • Build succeeds (pnpm build)
  • Manual testing completed — reproduced and re-verified against a real
    ObjectQL engine before and after the fix
  • New tests added

Full pnpm verify (validate → typecheck → lint → hygiene → build → test) is
green on the merged tree. Coverage thresholds hold (lines 99.6, statements 95.2,
functions 95.1, branches 80.3). scripts/check-stackblitz-lock.mjs and
changeset status — the two CI steps pnpm verify does not cover — also pass.

The new guards were confirmed to have teeth, not just to pass. Reverting a
single hook call site back to filter: fails four tests: three behavioural ones
in hooks-runtime-sales.test.ts (via the hardened harness) and the source scan,
which names the file and prints the offending line. Those same three tests
passed before this PR with the bug present, which is the clearest evidence that
the harness was masking it. Reverting the shared module's line was checked
separately and fails the widened scan by name.

Screenshots

n/a

Checklist

  • I have added a changeset.changeset/hook-query-where-not-filter.md
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Notes

Deliberately out of scope — a platform-side report was written up separately.
The framework behaviour underneath this — an unsupported predicate key is
dropped silently, and findOne then returns the first row rather than raising
or returning null
— is a @objectstack/objectql concern, not a HotCRM one,
and is not patched here. This PR only stops HotCRM from tripping over it. Two
things make that behaviour worth fixing upstream:

  1. find aliases filter while findOne and count do not, so the key
    appears to work until it is used on the path where being wrong is silent.
  2. ObjectQL.createContext's own doc comment demonstrates the broken spelling:
    await users.find({ filter: { status: 'active' } }).

The characterisation block in test/hook-query-predicate.test.ts will start
failing if and when the kernel is fixed — that is intentional, and the comment
there says what to do about it.

Closes #574

Eighteen `ctx.api` calls across eight `*.hook.ts` files passed their
predicate as `filter:`. The ObjectQL kernel does not accept that key on
every read path, and drops it without raising:

  - `find`    normalizes `filter` -> `where`, so those calls worked by luck.
  - `findOne` spreads the query into the AST (`{ ...query, limit: 1 }`) and
    never aliases, so the predicate vanished and the call returned the
    object's FIRST ROW — for any argument, including one matching nothing.
    It never returned null and never threw.
  - `count`   reads `query.where` explicitly, so the predicate vanished and
    the call counted the ENTIRE object.

Verified against a real ObjectQL 16.1.0 engine, not the test harness.

Impact of the mis-reads:
  - opportunity/quote line items defaulted `list_price` / `unit_price` from
    the first product in the catalog instead of the chosen one — wrong money
    on the record.
  - quote acceptance, won-opportunity and contract-activation evaluated
    their "already in the target state?" gate against an unrelated record
    while writing to the correct one.
  - case escalation assigned the follow-up task to the first account's owner.
  - account and product delete guards counted every row in the object, so
    they blocked deletes with no real references and reported a made-up
    count in the error message.
  - campaign ROI snapshots stored whole-table opportunity and lead counts as
    campaign attribution.

`HookQuery` no longer declares `filter?`, so this fails at compile time now
rather than in the data. `src/actions/lead.actions.ts` is normalized too —
behaviour-identical there (it is a `find`), but it was the seed of the next
copy-paste.

The suite could not have caught this: `test/helpers/hook-harness.ts`
resolved its predicate as `q.filter ?? q.where ?? {}`, making the stand-in
more permissive than the kernel it stands in for, so every affected hook
tested green. The harness now throws on `filter`, and the new
`test/hook-query-predicate.test.ts` pins the contract against a real
in-memory kernel — including the kernel's silent-drop behaviour, so the trap
stays documented — plus a source scan that fails any `*.hook.ts`
reintroducing the key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R7h5XeGgNPBqNuV6LCRWpg
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hotcrm Ignored Ignored Jul 31, 2026 11:02am

Request Review

Resolves conflicts in the two `*_line_item.hook.ts` files. main's #570
extracted the two near-verbatim price-fill handlers into a shared
`_line-item-price-fill.ts`; that extraction wins, and the `where:` fix moves
into the shared module — which had carried the `filter:` along with it.

Widens the source scan in `test/hook-query-predicate.test.ts` from
`src/objects/*.hook.ts` to every `.ts` in that directory. The narrow glob
would have missed `_line-item-price-fill.ts` entirely: hook bodies get
factored into shared modules, and a dropped predicate riding along in one of
those is exactly as broken while matching no `*.hook.ts` pattern. The scan now
asserts it can see that file, so the coverage cannot silently narrow again.

The rollup fixes in both line-item hooks merged cleanly and are unchanged.

pnpm verify: 25 files, 480 passed | 2 skipped. Coverage thresholds hold.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R7h5XeGgNPBqNuV6LCRWpg
@yinlianghui
yinlianghui marked this pull request as ready for review July 31, 2026 13:55
@yinlianghui
yinlianghui merged commit 3ed27b2 into main Jul 31, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hook reads pass filter:, which ObjectQL drops silently — 17 call sites read the wrong record

2 participants