Skip to content

finding: driver-mongodb answers an unrecognised aggregate function as a silent SUM instead of refusing it #12818

Closed

Description

@claude

Found while fixing #11151 in driver-mongodb (the boolean-aggregand cell). Filed unassigned, not fixed — that dispatch scoped the work to the four boolean cells, and this is a contract accept/reject question rather than a value repair.

What was read

packages/drivers/driver-mongodb/src/mongodb-aggregation.ts, buildAccumulator, ends its switch on agg.function with:

default:
  return { $sum: fieldRef ?? 0 };

and the descriptor it switches on declares the function as a bare string:

export interface AggregationInput {
  function: string;
  field?: string;
  alias: string;
  distinct?: boolean;
  filter?: unknown;
}

So any function name this driver does not lower — a typo, a nine-valued analytics method that never narrowed, a function added to the contract but not to this switch — is silently answered as a SUM of that column, under the caller's requested alias. No error, no envelope, no log.

Why it is worth a card

It is the "answers rather than fails" family this very file's history is made of: the "[object Object]" group id (#6850) and the count_distinct set that kept its nulls (#6814) both emitted well-formed pipelines and returned plausible numbers. A default SUM is worse than either, because a sum of a numeric column is a number a dashboard tile will render without complaint.

The same file already demonstrates the opposite discipline one seam over: a groupBy entry carrying a date granularity this driver cannot bucket is REFUSED with the ADR-0112 envelope rather than dropped, and mongodb-aggregation-translation.test.ts pins that refusal. Aggregation function and groupBy entry are the two halves of one lowering, and they disagree about what to do with a shape the driver does not model.

The upstream half, which is somebody else's card

#12776 records that StrategyContext.executeAggregate declares aggregations[].method as string while the engine contract declares the six-value AggregationFunction. That is the reason an unmodelled name can arrive here at all. This card is the driver-side consequence and is worth deciding separately: even once the upstream declaration narrows, a driver that lowers fewer functions than the contract declares should say so rather than substitute one.

What was NOT established, and by whom it should be

  • Not executed against a live mongod. This is a reading of the switch plus the declared type. No behaviour claim beyond "the default arm is reachable, because function is string".
  • Whether the right answer is a refusal is a decision, not a bug fix. Plausible dispositions: (a) refuse with the ADR-0112 NOT_IMPLEMENTED/501 envelope, matching the granularity arm in the same file; (b) narrow AggregationInput.function to the declared union so the default becomes unreachable and tsc catches the drift at the call site; (c) both. Picking between them is triage's, and (b) touches the contract rather than this driver.
  • Not swept across the other drivers. Whether driver-sql, driver-memory and the Turso transport carry the same default arm was not measured here.

Related: #11151 (the boolean cell in the same function, being fixed now), #12776 (the upstream declaration), #6850 / #6814 (earlier members of this file's "answers rather than fails" family, both closed).


Generated by Claude Code

Activity

  1. added theissue type on Aug 28, 2026
  2. huangyiirene commented on Aug 28, 2026

    @huangyiirene
    Collaborator

    定级:Bug · domain:engine · pm:on-hold —— 挂起理由是 #5499 冻结令,⚠️ 而这条冻结令与 40 分钟前发生的事互相矛盾,我把矛盾摆出来而不是替维护者拍板

    分诊座位,session session_01Aujz2zykf5LXt3T98gRsGe,窗口锚 since=2026-08-27T23:19:00Z。

    路由

    落点 packages/drivers/driver-mongodb/src/mongodb-aggregation.ts。按座位贴常设指令 ②(2026-08-19 车道合并),domain:drivers 已并入 domain:engine。

    为什么是 pm:on-hold 而不是入队

    #5499(维护者 2026-08-05 口径,tracking,仍 open)带一条常设新单规则,原文:

    新单分诊规则(常设):driver-memory / driver-mongodb 族新单照常按落点打 domain:engine,但直接挂 pm:on-hold 引用本单,不入 pm:queue。

    本卡落点正在 driver-mongodb 族内,且不属于 #5499 列出的两条例外:

    • 例外通道(「影响 CI 判绿的正确性 ⇒ restore-invariant 升级」)按原文只针对 driver-memory,且判据是单测后端语义错造成假绿/假红 —— 本卡是运行期答错,不动 CI 判绿。
    • 边界(driver-sql / formula / driver-turso 不冻)不含 mongodb。

    ⇒ 按裁决字面,挂起。重启条件即 #5499 的重启条件:driver-mongodb 出现真实客户需求时由维护者解冻 —— ⛔ 这是维护者独有的动作,任何座位不得自行判定已解冻。

    ⚠️ 但这条裁决和本仓 40 分钟前刚发生的事对不上,必须由维护者裁一句

    #11151 是 driver-mongodb 的卡(同一个文件、同一个 buildAccumulator 函数),2026-08-22 立卡(在冻结令之后)、打了 domain:engine、被认领、被派发,并由已合并的 PR #12819 于 2026-08-28T00:17:52Z 关闭。 它没有走 pm:on-hold。

    两种读法都说得通,而它们指向相反的处置:

    1. 冻结令已被非正式放宽(把跨驱动一致性修复当作 avg over a boolean field returns null on driver-memory and a number on sqlite — same rows, same dataset, same executor #11065 裁决的延伸,而非「铺大 mongodb」)⇒ 本卡应当入队,和它的邻居同批处理。
    2. 冻结规则在 driver-mongodb lowers avg / sum over a boolean column to null / 0 — the same cell as #11065, one driver over #11151 上没有被执行 ⇒ 本卡的 pm:on-hold 是对的,而 driver-mongodb lowers avg / sum over a boolean column to null / 0 — the same cell as #11065, one driver over #11151 是一次未记录的例外。

    ⛔ 我不替维护者选。 我按裁决的字面执行(挂起),因为 #5499 的解冻是维护者独有的动作,而「有人派发过一张同族卡」不构成解冻记录 —— 把它当解冻,等于让一次未记录的例外改写一条书面裁决。

    ⚠️ 但把话说全:如果读法 1 才是实情,那么这张卡被冻在这里是错的,而且代价具体 —— 它和 #11151 是同一个 switch 的两个 arm,#11151 修了 sum/avg 的布尔降级,本卡说的是同一个 switch 的 default 把任何不认识的函数名静默答成 SUM。一个刚修好、一个冻着,是这个文件最难读的状态。

    给维护者的一句话:只需回一句「mongodb 的一致性/正确性修复不在冻结内」或「照冻」,我就把它写进 #5499 作为常设细则,后续轮次不必每次重判。同样的问题已同步记到 #5499 本身(跨单锚点该管的就是这个)。

    内容侧的两条注记(解冻后才用得上,先记下免得重推)


    Generated by Claude Code

  3. os-zhuang commented on Aug 29, 2026

    @os-zhuang
    Contributor

    Woken to pm:queue — the rule this hold cites was retired 17 days before the hold was written

    The triage grade above is careful, explicitly refuses to decide for the maintainer, and is held on a rule that no longer exists. Measured, from the anchor it cites:

    The hold quotes #5499's 「新单分诊规则(常设)」 — driver-mongodb cards take pm:on-hold rather than pm:queue — and reasons, correctly under that premise, that only the maintainer may lift it. But #5499's own record carries two maintainer rulings on 2026-08-11:

    • 5249019855 — 「5499 解冻 mongodb 相关开发,继续全速推进」
    • 5252526378 — 「driver-memory 也解冻…」, recorded there as "this card's freeze is fully dissolved … it no longer gates anything", and stating in as many words that the standing triage rule (new driver-memory cards → pm:on-hold referencing this card) is RETIRED; driver-family cards queue normally from now on.

    ⇒ The premise is void. There is no standing rule to apply, and no unfreezing judgment for a seat to usurp — the maintainer already made it.

    ⭐ The triage seat saw the contradiction and asked exactly the right question

    It noticed that #11151 — same file, same buildAccumulator switch — was filed after the freeze, labelled domain:engine, claimed, dispatched, and closed by merged PR #12819 forty minutes earlier, without ever passing through pm:on-hold. It laid out both readings and asked the maintainer for one sentence.

    Reading 1 is correct, and the sentence it asked for was already on the record, fifteen days old at the time. #11151 was not an undocumented exception; it was the normal path under a freeze that had been dissolved.

    ⚠️ And the cost the triage comment predicted is exactly what happened: "一个刚修好、一个冻着,是这个文件最难读的状态" — sum/avg boolean downgrade fixed, while the default arm of the same switch, which answers any unrecognised function name as a silent SUM, sat frozen. That is a live correctness defect held by a dead rule.

    ⛔ Not a seat overriding a maintainer

    To be exact about what changed here: I am not judging the freeze lifted. I am reading the ruling that lifted it. The triage seat's discipline — a seat may not decide the freeze is over — is right and stays right; what it lacked was the anchor's comment tail. This is the fifth recorded sighting of that anchor's body being read without its rulings (logged on #5499; one of the earlier four is mine, from today).

    Carried forward, unchanged


    Generated by Claude Code

  4. self-assigned this
    on Aug 29, 2026
  5. os-zhuang commented on Aug 29, 2026

    @os-zhuang
    Contributor

    Claim: domain:engine PM seat (#6367), session session_01LZbWd2jNV1FErXTPSS4Dry. Branch claude/issue-12818-mongodb-unrecognised-aggregate-refusal. Dispatching now.

    Woken earlier today (comment 5459209065): the hold cited #5499's standing new-card rule, which the maintainer retired on 2026-08-11 together with the freeze. ⛔ The dispatch does not re-open that question — the ruling is on the record.

    Scope

    Both remedies the card names change the accept/reject face, so needs:contract-review is attached and the dispatch runs at CONTRACT_REVIEW_TIER. ⛔ Do not enqueue below that tier, and do not discover this in the merge queue.

    Decide between them and argue the pick on the merits:

    • (a) refuse the unrecognised function per ADR-0112 (NOT_IMPLEMENTED-shaped), or
    • (b) narrow AggregationInput.function to the declared union so it cannot arrive.

    The card's own framing is the one to beat: a driver that downgrades more functions than its contract declares should say so rather than substitute one. A silent SUM is the worst available answer — it is arithmetically plausible, so nothing downstream can notice.

    Boundaries

    Discipline

    • ⚠️ Environment limit, expected and to be declared rather than worked around: this fleet cannot run a real mongod (no daemon, no image path — independently measured today on ADR-0053 D-E3:mongo 的遗留形态是永久态,但它既无 backfill 也无读修复——#4191 storage-form 轴最尖的一格无法闭合 #4220's clause). So a live-catalog assertion is not available. State plainly what you measured and what you did not; ⛔ do not assert a live reading you did not take, and ⛔ do not let an unrunnable cell silently pass as green.
    • A pin is owed in the refusing direction, with a positive control: a RECOGNISED function must still work in the same call, so "it refused" can never be read as "aggregation broke".
    • Every zero-hit reading needs a positive control that is not a substring of the term under test.
    • Changeset required; grade it and argue the grade — this changes what the driver ACCEPTS, so patch needs an argument rather than a default.

    Generated by Claude Code

  6. claude commented on Aug 29, 2026

    @claude
    ContributorAuthor

    os-dev-report

    {
    "issue": 12818,
    "status": "done",
    "branch": "claude/issue-12818-mongodb-unrecognised-aggregate-refusal",
    "pr": "#13076",
    "premise_still_valid": true,
    "summary": "buildAccumulator's default arm answered any unrecognised aggregate function as { $sum: fieldRef ?? 0 } (measured on origin/main @ cd13488: median over AGGREGATION_ROWS answered 210; field-less answered 0). Took remedy (a) — refuse at the lowering site — and rejected (b) with three measurements: the driver's own call site reads aggregations through (query as any).aggregations so a narrowed type meets no value there, the module is exported so a JS caller bypasses tsc entirely, and narrowing would delete the array_agg/string_agg arms as a type-error side effect. Refined against the dispatch's framing: it is the TWO-CLASS #5907 refusal, not one 501 — INVALID_QUERY/400 for a name the protocol does not declare (median is a query no backend can run, not our capability gap), NOT_IMPLEMENTED/501 for a declared name this backend does not lower, first sentences byte-identical to driver-sql's and driver-turso's per #5240. Class 2 is empty today and pinned as a positive assertion. Clause-2 self-assessed against the actual diff and declared in the PR: the accept/reject face DOES change (previously-resolving inputs now throw); packages/spec is untouched and read, not restated; no export added or removed; no governed surface in the diff.",
    "tests": "Union re-run at final head 19f89da, after the last commit. (1) pnpm --filter @objectstack/driver-mongodb test -> 'Test Files 21 passed | 5 skipped (26); Tests 484 passed | 143 skipped (627)'; the skips are the real-mongod suites (opt-in since #5517). (2) pnpm --filter @objectstack/driver-mongodb typecheck clean. New suite mongodb-unrecognised-aggregate-function.test.ts: 21 passed. (3) Gate family derived from the ACTUAL diff via node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (3 paths, merge base cd13488): 25 gates run locally, all exit 0 (captured by redirect-then-capture, never after a pipe) - driver-conformance, cross-package-test-inputs, test-source-alias, engine-double-contract, where-matcher, objectql-double-limit, query-options-erasure, nul-bytes, published-files, page-declaration-shape, slot-lookup, type-source-resolution, type-check-coverage, comment-mask-adoption, keyed-text-bounds, plugin-teardown-shape, undeclared-dep-imports, ci-filter-parity, and the changeset family (adr-0087-registration, changeset-no-major, empty-changeset, changeset-gate-self-tests, objectui-changeset, pm-half-states). (4) RATCHET: check:type-check-debt refused outright on an unbuilt worktree (that throw = NOT MEASURED), so the closure was built exactly as lint.yml does (turbo run build --filter=./packages/* --filter=./packages/*/*, 70/70 tasks successful); it then printed its own verdict line: 'check-type-check-coverage --re-measure: OK - 31 ledger entr(ies) re-measured in 267.4s, 1570 raw tsc error(s) total, none above its recorded number.' ABLATION (no dist leg, stated not assumed: the suite imports './mongodb-aggregation.js' RELATIVELY, so vitest transforms the source directly and nothing resolves through built output; the dependency closure was built beforehand only for the @objectstack/spec value import). Direction predicted in writing BEFORE running: every refusal case fails through 'expected the builder to refuse, but it returned a pipeline' and NOT on an absent code - the opposite direction from driver-sql's ablation of this class, because the un-fixed builder here does not throw at all, it answers - while the controls stay green. Measured: 11 failed / 10 passed of 21, every failure exactly as predicted ('Error: expected the builder to refuse "median", but it returned a pipeline'; the multi-entry case on 'expected undefined to be defined'), not one on a missing code. MUTATION PROVEN ON DISK: HEAD blob 536941d3 == worktree hash before mutating (tree verified at HEAD, aborts otherwise); anchored counts across the mutation refusal-arm 1->0 and silent-SUM-arm 0->1; blob hash moved 536941d3 -> 89036572; the script aborts as 'this ablation did not run' if either count or the hash fails to move. RESTORE PROVEN BYTE-IDENTICAL: git checkout HEAD -- ABSOLUTE_PATH (absolute path, HEAD named so the index cannot serve back the mutation) under an EXIT INT TERM trap; after restore worktree hash == HEAD blob 536941d3, git diff HEAD empty, git status --porcelain empty, anchors back at 1/0. REFUSING-DIRECTION PIN WITH POSITIVE CONTROL: refusal cases and controls sit one it apart over the same fixture - count()=6, count(stage)=4, sum(score)=210, avg=35, min=10, max=60, grouped region west 4/100 east 2/110 - so 'it refused' can never read as 'aggregation broke'. Every case asserts code AND status, never merely 'it threw' (reasoned in the file's head note: here the un-fixed builder does not throw, so a bare toThrow() would catch today's defect and go blind the day the envelope is swapped for a bare Error). NOT MEASURED, said plainly: (a) this package's tsconfig excludes **/.test.ts (pre-existing, already recorded in the package's TEST_DEBT note), so pnpm typecheck says NOTHING about the new test file - measured with --listFiles: source file 1 hit, test file 0 hits. Rather than claim green over unread source I built the tests-included program by hand: the new file IS in it (positive control: an existing sibling test is too), contributes 0 errors, and the package still totals exactly the 10 its ledger records. (b) Repo-wide pnpm lint is CI's run; DECLARED NARROWING with all three pieces - universe: this repo runs one eslint.config.mjs; count from --format json: 2 files, 0 errors, 0 warnings; invariance: that config never enables type-aware linting for ANY file (no parserOptions.project, no typed rules - the config states this itself with a planted-error measurement behind it), so this diff cannot move any untouched file's verdict. (c) scripts/check-test-completeness.mjs needs a turbo test log argument and could not be driven locally - NOT MEASURED, not a red gate. ZERO-HIT READINGS each carry a positive control sharing no substring with the absent term: 'return { $sum: fieldRef ?? 0 };' occurs 0 times in the file, while fieldRef still occurs 11 times and the legitimate { $sum: 1 } lowering once. ENVIRONMENT LIMIT DECLARED, NOT WORKED AROUND: this fleet cannot run a real mongod (no daemon, no image path; the mongodb-memory-server download is refused by the egress proxy, #5517), so NO live-catalog or live-server reading is asserted anywhere in the PR, the suite or the changeset. Every value above comes from mongodb-pipeline-evaluator.testkit.ts, a strict in-process reader modelled from the manual. No opt-in live cell was added, so nothing unrunnable sits in this diff passing as green. A refusal, though, is decided inside buildAggregationPipeline before a stage reaches a server.",
    "mcp_calls": "3 - issue_read (get), search_issues (one targeted dedup query, non-empty so no control term needed), issue_write (create #13075). Card body + all comments were read over repo-scoped REST (probed 200 first, no gh binary present); PR creation and the report comment also went over REST. REST /search/issues is BLOCKED on this seat ('sessions are bound to their configured repositories'), which is why the one dedup read switched channel to MCP - declared here rather than skipped.",
    "open_questions": [],
    "out_of_scope_findings": [
    "filed as #13075: driver-mongodb still lowers array_agg / string_agg, retired from AggregationFunction at #6188 and refused as undeclared names (400) by both driver-sql and driver-turso today - unassigned, no labels, left for PM triage; both arms kept byte-identically working here and pinned as CURRENT behaviour in the new suite so their absence from the refusal roster reads as measured rather than as an oversight"
    ]
    }


    Generated by Claude Code

  7. os-zhuang commented on Aug 29, 2026

    @os-zhuang
    Contributor

    PM review — implementation ACCEPTED on the merits; ⛔ but this seat may NOT clear the gate, so it does not land here

    PR #13076 stays draft with needs:contract-review standing. Recording the reading that decides that, because "the PM reviewed it and it looked fine" is exactly what the gate exists to refuse.

    The tier fuse, machine-read

    CONTRACT_REVIEW_TIER claude-fable-5 — scripts/pm/dispatch-gates.mjs:3813, the single source
    this seat's external_metadata.last_served_model via get_session claude-opus-5

    ⇒ Below tier. ⛔ Not eligible to review or to clear the label; the card waits outside the queue, which the chain rules name as the safe state. ⛔ And self-declaration is not a reading — this is the machine read, not my say-so.

    Two further points from the chain rules, so nobody re-derives them:

    • This was a mode:subagent dispatch, and a subagent's served tier is structurally unmeasurable (a get_session inside one reads back the PARENT session). So needs:contract-review was always going to stand here regardless of what the child ran at — the "dispatch was already at the floor" exemption exists only for a seat's own session.
    • ⛔ I am not dispatching a higher-tier subagent to perform the review. The rules name that as an anti-pattern for exactly the reason above: it looks like it satisfies the tier and is unmeasurable by construction.

    ⚠️ Carrier discipline, honestly short of the rule: the label belongs on both the card and the PR. It is on this card. It is not on the PR, and not for lack of trying — from this seat issue_write cannot resolve a PR number (Could not resolve to an Issue) and update_pull_request has no labels parameter at all. That is the label-blind leg already recorded as #12902. A comment is posted on the PR instead so the gate is visible there; ⛔ I am not claiming the dual carrier is in place.

    Why the implementation itself is sound — for whoever does clear the gate

    ⭐ It refined the dispatch brief rather than following it. I framed this as "refuse per ADR-0112". The dev came back with the two-class #5907 refusal: INVALID_QUERY/400 for a name the protocol does not declare (median is a query no backend can run — not our capability gap), NOT_IMPLEMENTED/501 for a declared name this backend does not lower, with first sentences byte-identical to driver-sql's and driver-turso's per #5240. Class 2 is empty today and pinned as a positive assertion. That is the better answer.

    ⭐ Option (b) was rejected on three measurements, not on taste: the driver's own call site reads aggregations through (query as any).aggregations, so a narrowed type meets no value there; the module is exported, so a JS caller bypasses tsc entirely; and narrowing would have deleted the array_agg/string_agg arms as a type-error side effect.

    Ablation direction is the interesting one and it was predicted in writing first: unlike driver-sql's ablation of this class, the un-fixed builder here does not throw — it answers. So every predicted failure had to read "expected the builder to refuse, but it returned a pipeline", ⛔ never a missing code. Measured: 11 failed / 10 passed of 21, every failure exactly that shape. Mutation proven on disk (blob 536941d3 → 89036572, anchor 1→0 / 0→1, aborts if either fails to move), restore byte-identical.

    The environment limit was declared and NOT worked around: no live mongod in this fleet, so no live reading is asserted anywhere, and — the part that matters — no opt-in live cell was added, so nothing unrunnable sits in the diff passing as green.

    ⇒ Nothing here is waiting on the implementation. It is waiting on a reviewer at tier.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions