Skip to content

fix(service-analytics): resolve {current_user_id} on the direct analytics query path, at parity on both compiled surfaces - #12376

Merged
os-trump merged 2 commits into
mainfrom
claude/issue-12230-analytics-current-user-token
Aug 25, 2026
Merged

fix(service-analytics): resolve {current_user_id} on the direct analytics query path, at parity on both compiled surfaces#12376
os-trump merged 2 commits into
mainfrom
claude/issue-12230-analytics-current-user-token

Conversation

@os-trump

Copy link
Copy Markdown
Collaborator

Fixes#12230

Premise re-derivation — the tree moved since 17.1.0, the defect narrowed but survived

The card's zero-hit grep was measured on the published @objectstack/service-analytics@17.1.0 tarball. On origin/main today, framework#3582's evaluator (resolveFilterTokens, @objectstack/core) already covers the ObjectQL engine and DatasetExecutor (the dashboard dataset door). Two holes survived, both the exact "one vocabulary, two verdicts" shape the card names — and both were measured red before fixing (dissolution runs below):

  1. The direct door (POST /analytics/queryAnalyticsService.query, and generateSql for /analytics/sql) resolved nothing itself. The ObjectQL strategy resolves downstream in the engine; NativeSQLStrategy compiles a raw SELECT … WHERE and bound the literalowner = '{current_user_id}' matches no row, so every user-scoped widget on a SQL driver rendered a silent 0 for every viewer.
  2. The Measure definitions are not fully compiled: aggregate: 'count' ignores field, and /api/v1/analytics/query drops every per-measure filter — the SQL is COUNT(*) for all of them #10298 dataset-scope channel handed strategies the registry's compiled dataset filter — shared across requests, never resolved. On the dashboard door the executor resolves its own copy into where, and the strategy then ANDed the unresolved twin: owner = $viewer AND owner = '{current_user_id}' selects nothing, even though the presence-only assertions in the existing token test stayed green ([ 'usr_1', '{current_user_id}' ] — both bound, verbatim from the dissolution run). The "redundant and idempotent" reasoning that justified the conjunct holds only for token-free filters. The same channel zeroed measure-scoped filters through the CASE WHEN arm.

The fix — option 1 (parity substitution), one seam

AnalyticsService.query() and generateSql() now expand every filter placeholder before strategy selection, with one instant per call, via the platform's one evaluator (resolveFilterTokens — consumed from @objectstack/core, not redefined; zero packages/spec changes):

Copy-on-write throughout: caller-owned query objects and registry entries are never mutated, so no request's user id is ever baked into a shared definition. Token-free trees pass through by reference (the resolver's cheap pre-pass), so internal queries cost nothing.

The refusal arm comes with the same seam (the card's option 2, for the cases substitution cannot serve): an unknown spelling refuses FILTER_TOKEN_UNKNOWN / 400 with the near-miss suggestion, and a vocabulary token with no value ({current_user_id} on an unauthenticated request) refuses FILTER_TOKEN_UNRESOLVED / 400 — never IS NULL, never a plausible zero. Both asserted on codeandstatus (ADR-0112).

Both compiled surfaces, named, with what each does with the token

No half was shipped: the same seam feeds both strategies (and any custom or fallback-delegate strategy), and queryDataset's executor path keeps its own resolution for the preview evaluator it alone feeds.

The load-bearing pin — per-viewer contrast, both directions

This writes a viewer identity into a SQL WHERE; the failure that matters is resolving to the wrong viewer. query-filter-tokens.test.ts runs the same saved filter as two users against one service instance and asserts both directions at three depths: bound params (A's call binds usr_a and not usr_b, B's the reverse, the literal nowhere), the aggregate bridge's filter, and real rows on a real database (sql.js: A sees A's 2 high-priority cases and none of B's; B sees B's 1 low-priority case and none of A's — dashboard door and direct door both). The existing registry-mutation pin (dataset-filter-tokens.test.ts) already guards the resolved-once-at-boot shape; it gained the absence assertions that the double-predicate hole was slipping past.

Verification (all runs at head e3011f6c4 after the rebase onto e3430a2cb)

  • pnpm --filter @objectstack/service-analytics test82 files, 1803 tests, all green (verify-lock VERDICT command-exit 0).
  • tsc --noEmit on the package: exactly the 10 pre-existing ledger-debt errors (check:type-check-coverage ledger count 10; same files and classes as the ledger note) — zero new.
  • Dissolution verification: git checkout origin/main -- src/analytics-service.ts (mutation proven on disk: anchored grep resolveQueryTokens → 0 hits, git diff --stat HEAD = 116 lines reversed in that one file), then the two token test files → 16 of 19 red, e.g. AssertionError: expected [ 'usr_1', '{current_user_id}' ] to not include '{current_user_id}' and expected [ '{current_user_id}', …(1) ] to include 'usr_a'; the 3 green survivors are the pre-existing executor-half pins, correctly untouched. Restored via git checkout HEAD -- <path>, disk == index == HEAD verified (git status --porcelain empty, git diff HEAD empty, anchor grep = 3), re-run → 19/19 green.
  • Consumer sweep, prefix form ...@objectstack/service-analytics = downstream consumers (rest, runtime, cli, verify, qa/dogfood): turbo run build --filter='...@objectstack/service-analytics' → 67 tasks, all successful; runtime's analytics withhold test 10/10 and rest's 7 analytics route/gate files 85/85 green.
  • Derived gates (node scripts/pm/dispatch-gates.mjs, no paths passed — change set derived from merge base e3430a2cb; re-derived after the rebase, list unchanged): all 17 path-matched families green (check:changeset-gate-self-tests, check:objectui-changeset, check:page-declaration-shape, check:published-files, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check:cross-package-test-inputs both spellings, check-adr-0087-registration, check-changeset-no-major, check-ci-filter-parity, check-empty-changeset, check-plugin-teardown-shape, check-affected-docs, release-rehearsal-clone --self-test) plus the convention-triggered families for new test files: check:engine-double-contract, check:where-matcher (which caught the fixture matcher being combinator-blind — fixed to refuse, second commit), check:query-options-erasure, check:type-check-coverage — all green. check-drift-comment needs PR context and runs in CI.
  • Declared narrowings (CI runs the full farm regardless): ① check:type-check-debt --re-measure re-runs tsc per ledger entry; the only entry whose program compiles files in this diff is @objectstack/service-analytics (population read from the gate's own ledger; check:cross-package-test-inputs green proves no other package's tests read these files; consumers see only the built .d.ts, whose exported shape is unchanged and whose 67-task dts builds passed) — re-measured at head: exactly 10, the ledger count. ② Repo-scan eslint: targeted run of the 3 edited TS files under the repo's single eslint.config.mjs--format json reports 3 files linted, 0 errors 0 warnings; the config never enables type-aware linting for any file (its own §"no parserOptions.project" comment), so verdicts are per-file and this diff cannot move any untouched file's result; the 4th changed file is a .changeset/*.md, outside eslint's population and covered by the changeset gates above.

Changeset

minor for @objectstack/service-analytics (.changeset/analytics-current-user-token.md): a widget that silently rendered 0 now either works or refuses. Not major.

Scope notes

Generated by Claude Code


Generated by Claude Code

…tics query path, at parity on both compiled surfaces
The direct door (AnalyticsService.query / generateSql) resolved no filter
placeholder itself: the ObjectQL strategy's engine bridge resolved
downstream, but NativeSQLStrategy compiled a raw SELECT ... WHERE and bound
the literal text -- one vocabulary, two verdicts, and the losing verdict was
a silent 0 for every viewer of every user-scoped widget. The #10298
dataset-scope channel had the same hole one layer down: strategies received
the registry's unresolved filter copy and ANDed it in beside the executor's
resolved one, selecting nothing.
Both close at one seam: query()/generateSql() expand `where`,
`timeDimensions[].dateRange`, and the per-request dataset-scope getter with
one instant per call, before strategy selection, via the platform's one
token evaluator (resolveFilterTokens, @objectstack/core). Unresolvable
placeholders refuse with FILTER_TOKEN_UNKNOWN / FILTER_TOKEN_UNRESOLVED
(400) instead of charting zero. Per-viewer contrast pinned on both surfaces
and end-to-end on a real database.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
…rity row fixture
check:where-matcher flagged the fixture matcher as combinator-blind: $or
would have been read as a field name and silently matched nothing. Refuse
every combinator and operator the fixture does not model, per the gate's
convention.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics, touching 6 documentable anchor(s).

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v14.mdx(via generateSql (symbol))
  • content/docs/releases/v17.mdx(via generateSql (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 177: 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; 107 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 — 8 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 d516483f655681414796f244a46f903df2bf82f2packageMentionDocs.

Which tree this was computed on

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

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

@os-trumpClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract-review verdict: PASS (#12230)

Reviewed by the contract-review chain at CONTRACT_REVIEW_TIER. Tier fuse, machine reading this sub-round: get_session.external_metadata.last_served_model = claude-fable-5.

Reviewed: the full diff (the service seam, both test files, the changeset), the card's triage constraints and claim, the dev report, and the seat's ACCEPT — including the #3594 sentence the seat put in front of this review.

The Clause-② surface, weighed with "presentation scope, never an access boundary" in hand: this PR implements the frozen vocabulary's own declared semantics at parity — {current_user_id} means "the viewer" on every surface, exactly as the list and dashboard paths already read it — and adds no access boundary: RLS/read-scope compilation is untouched (the #3602 engine-side context belt and the getReadScope pre-resolution are byte-level unchanged in the diff), and the changeset speaks parity, not protection. The two contract movements — parity substitution, and FILTER_TOKEN_UNKNOWN/FILTER_TOKEN_UNRESOLVED 400 replacing a silently-bound literal — are the vocabulary's contract enforced, with the "never IS NULL, never a plausible zero" direction pinned.

Verified in the seam patch: one evaluator imported from @objectstack/core, never re-implemented; copy-on-write with same-reference passthrough (a caller-owned saved definition can never have one request's id baked in — and the registry-mutation pin now carries the absence assertions that the double-predicate hole was slipping past); one instant per call, applied identically in query() and generateSql() before strategy selection, so the dry-run door answers what the real run would.

The load-bearing pin does what the brief demanded and more: per-viewer contrast in both directions at three depths (bound params, the aggregate bridge, real rows on a real database, both doors), the #10298 double-predicate hole pinned by name, refusals asserting code AND status with zero SQL executed, and the in-memory fixture's matcher refusing unimplemented combinators — the check:where-matcher lesson applied inside the fixture itself. Dissolution 16/19 red with the three survivors diagnosed, restore verified.

The premise correction is a good run: the card's tarball-measured zero was stale; the dev found the two narrower live holes (the direct door, and the #10298 channel invisible to presence-only assertions) and measured both red before fixing. The open trailer-spelling question is answered A by standing practice (session URL, no model names) — already what these commits carry.

Landing completed by the chain in the same stroke per the maintainer's standing instruction (2026-08-25, verbatim: 「审核通过你应该直接负责合并吧,还要等原始的项目经理吗」): clearing needs:contract-review on this PR and #12230, flipping ready, arming the queue. Post-merge follow-on (objectstack-ai/hotcrm#510) stays with the owning seat as recorded.


Generated by Claude Code

@os-trump
os-trump marked this pull request as ready for review August 25, 2026 21:26
@os-trump
os-trump added this pull request to the merge queueAug 25, 2026
Merged via the queue into main with commit 74cee59Aug 25, 2026
43 checks passed
@os-trump
os-trump deleted the claude/issue-12230-analytics-current-user-token branch August 25, 2026 21:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationteststooling

Projects

None yet

2 participants

@os-trump@claude