Skip to content

retrieve_entities total describes neither the returned set nor the unfiltered one #2467

Description

@ateles-agent

Summary

retrieve_entities returns a total that describes neither the returned set nor any set the caller can obtain. Anything paging on that field is trusting a number that does not describe its own result.

Reproduction

Against prod, entity type skill:

Call Rows returned total reported excluded_merged
default (limit: 3) 3 79 true
default, unpaged 72 79 true
with merged rows included 93 93 false

72 live + 21 merged = 93. 79 matches neither 72 nor 93.

An independent count via get_entity_type_counts is a third figure again, which is how this surfaced — a consumer cross-checking two Neotoma surfaces against each other.

Why this matters beyond a wrong number

total is the field a pager stops on. A caller that loops while fetched < total against 79, over a live set of 72, either spins on an exhausted cursor or concludes rows are missing and retries. A caller that trusts 79 as a denominator reports a completeness percentage that cannot reach 100%.

This is also a silent-undercount shape rather than an error: nothing 4xxs, no warning is emitted, and the mismatch is only visible if the caller counts what it received and compares. A consumer that trusts the field never learns.

The same class already has precedent in this repo's consumers: a reader that checked one field spelling reported 0% against a truth of 94.8%, and an inventory reported a 15% undercount from a glob that returned a clean run. In each case the instrument reported success while describing something other than reality.

What the fix needs to settle

Not proposing an implementation — the semantics are the question, and picking one silently is how the next consumer gets a different wrong number:

  • total means exactly one thing, stated in the tool's own description: rows matching the filter as applied, including whatever excluded_merged did to it
  • When excluded_merged: true, total counts the post-exclusion set — a total that includes rows the same call deliberately removed is not a total of anything the caller can fetch
  • total, the row count on an unpaged call, and get_entity_type_counts agree for the same type and filter, or each documents precisely what it counts and why they differ
  • A regression asserts agreement, so a future divergence fails rather than being rediscovered by a consumer

Provenance

Found while building a skill inventory across all repositories and worktrees (ateles#1126, stage 0 of the skills migration). The inventory needed a denominator for Neotoma-resident skill entities and could not obtain one it could defend, so it recorded the discrepancy rather than picking a figure.

Filed from the ateles side because the dispatched agent's grant was Neotoma read-only and it correctly declined to leave durable work in an unclaimable form rather than filing outside its scope.

Refs markmhendrickson/ateles#1126, markmhendrickson/ateles#1123.

Swarm specification

This section is maintained by the Ateles swarm. Each lens agent owns exactly one subsection below; the human-written description above these markers is never modified.

Design basis

Design basis: docs/foundation/principles.md#3-validate-the-instrument-before-believing-the-measurement — total is the instrument a pager trusts, and it currently reports a figure that describes neither set the caller can obtain.

Product / Scope (PM)

Decision. retrieve_entities.total is the cardinality of the set the same request can page to exhaustion, after every filter that request applied, including merge exclusion. It is not the length of the current page, and it is not a pre-exclusion census.

Problem. A pager that stops on total is trusting a number that describes neither the returned page nor the set it can fetch. On type skill, the default listing returned 72 live rows with total 79; including merged rows returned 93 with total 93.

In scope

  • Plain listing (no search): total equals that pageable cardinality for both the default exclusion of merged rows and a call that includes them.
  • One sentence of that meaning in the MCP tool description and in the OpenAPI description of total, including that this field is not get_entity_type_counts.

Out of scope

Acceptance criteria

  • On a fixture with live rows and merged-away rows, an unpaged default call returns N entities and total == N, with excluded_merged: true. total is not N plus the merged-away count.
  • The same fixture with merged rows included returns M entities and total == M, where M is the live set plus the merged-away rows that call actually returns.
  • Paging with limit smaller than N, following next_cursor until it is absent, collects exactly total distinct ids, the same set as the unpaged call. The test fails if total is larger than the fetchable set (the reported effect: while fetched < total does not finish on the real set).
  • The assertion is the counts and the id set, not that the response validates or that total is present.
  • Those three assertions hold on every exposing surface, each driven with that surface's call shape: MCP retrieve_entities, POST /entities/query, GET /entities, and CLI neotoma entities list (JSON).
  • The MCP tool description and the OpenAPI description of total state this meaning, and say it is not get_entity_type_counts.

Priority. Contract bug. Unblocks any caller using total as a denominator for one listing. Does not unblock #2269 or #2380.

Confidence. High. Assumption: the defect to close is the plain-listing count disagreeing with the pageable set, not the dashboard-stats census.

Open questions. None that block sequencing.

Routing. bug plus contract_discrepancy, and the change touches the response contract and the tool description, so the arch gate stays required. Next owner is arch, not cicada.

Engineering

Do not add a second counter. No open PR changes this total. Merged #2267 (countVisibleEntities in src/shared/action_handlers/entity_handlers.ts) counts entity_snapshots and, when include_merged is true, adds every entities row with merged_to_entity_id set. tests/integration/entity_query_deleted_count.test.ts only merges through mergeEntities, which deletes the snapshot in the same transaction, so that test stays green when a snapshot outlives the merge. The reported skill figures are that gap: the default page returns the non-merged rows (72) while the snapshot count (79) still includes merged rows whose snapshot was not deleted; include_merged then happens to print 93 because the addend and the leftover snapshots overlap. Do not revert #2267's refusal to scan observation fields (#2266). Do not touch search totals (#2269), get_entity_type_counts (#2380), or REST's omitted excluded_merged (#2268). Do not repair the stale snapshot rows.

Predicate. On the plain-listing path (no search), total is how many rows queryEntities (src/services/entity_queries.ts) returns if the caller follows next_cursor until it is absent. Same user, same normalizeEntityTypeFilter (no type predicate when the filter is empty):

  • includeMerged false: entities.merged_to_entity_id IS NULL, and the row is not soft-deleted.
  • includeMerged true: that merge predicate is dropped; each merged-away row is in the set once.
  • Soft-deleted: no entity_snapshots row, merged_to_entity_id is null, and at least one observations row exists (getDeletedEntityIds). Out of the set.
  • Never-observed: no snapshot, merged_to_entity_id is null, no observation. On the page today; must be in total.
  • Merged-away after mergeEntities: no snapshot and no observations (they were moved). Not never-observed. Tell them apart by merged_to_entity_id, not by missing observations.
  • Merged-away that still has a snapshot: excluded when includeMerged is false, counted once when true.

The count must match this SQL. One statement, indexed EXISTS / NOT EXISTS. Do not load ids into JS and do not read observation fields.

SELECT COUNT(*)
FROM entities e
WHERE e.user_id = :user
  AND /* same type filter as the page */
  AND (:includeMerged OR e.merged_to_entity_id IS NULL)
  AND (
    EXISTS (
      SELECT 1 FROM entity_snapshots s
      WHERE s.entity_id = e.id AND s.user_id = e.user_id
    )
    OR e.merged_to_entity_id IS NOT NULL
    OR NOT EXISTS (
      SELECT 1 FROM observations o
      WHERE o.entity_id = e.id AND o.user_id = e.user_id
    )
  )

When includeMerged is false the AND already drops merged rows, so the middle OR cannot re-include them.

countVisibleEntities.

  • Fast branch (!needsEntityTableFilters && !needsSnapshotJsonFilters): delete countLiveSnapshots and countMergedAway. countMergedAway on top of a snapshot count double-counts a merged row that still has a snapshot. Run the predicate above instead.
  • Filtered branch (updated_since, created_since, published, published_after, published_before): keep starting from entities with the merge filter. Keep counting merged candidates as-is and non-merged candidates that have a snapshot. Also count never-observed non-merged rows (no snapshot, no observation) when no published filter is set. A published filter cannot match a snapshot-less row; leave those out. Do not add countMergedAway on top of this branch.
  • queryEntitiesWithCount: leave the search branch alone. Leave the identityBasis || snapshotFilters branch alone (it already sets total from the unpaged queryEntities result).

Do not change queryEntities, getDeletedEntityIds, mergeEntities, or get_entity_type_counts.

Contract text. No new field. total stays an integer. excluded_merged stays MCP-only.

Append this sentence to the existing retrieve_entities text in docs/developer/mcp/tool_descriptions.yaml. Do not replace the pagination text. src/tool_definitions.ts desc() serves that yaml; do not copy the sentence into the fallback string.

total is the number of entities this same request can page to exhaustion after every filter it applied, including merge exclusion. It is not the length of entities on this page, and it is not get_entity_type_counts.

Set that same sentence as description on total in openapi.yaml for both GET /entities (listEntities, the 200 schema) and POST /entities/query (queryEntities, the 200 schema). Run npm run openapi:generate and commit src/shared/openapi_types.ts in the same PR.

Tests. New file tests/integration/entity_query_total_page_parity.test.ts. Boot app the way tests/integration/correction_scalar_tiebreak_surfaces.test.ts does. Drive MCP with an in-memory client calling tool retrieve_entities (tests/integration/entity_queries_cursor.test.ts). Drive CLI with node dist/cli/index.js --json --api-only --base-url <that server> entities list.

Seed one user and one type directly, not only through mergeEntities:

  • two live rows (entity + snapshot + observation)
  • one merged-away row from mergeEntities (snapshot gone)
  • one merged-away row inserted with merged_to_entity_id set and a snapshot row still present
  • one soft-deleted row (softDeleteEntity)
  • one never-observed row (entity row only)

Assert the counts and the id set, not that the body validates:

  • Default (include_merged omitted): ids are the two live rows plus the never-observed row. total equals that length (3). The two merged rows and the deleted row are absent. total is not 3 plus the merged count.
  • include_merged: true: those 3 plus both merged rows (5). total === 5. The stale-snapshot merged row appears once.
  • limit: 1, follow next_cursor until absent: distinct ids equal the unpaged set, and that length equals total.

Run those three on MCP retrieve_entities, POST /entities/query, GET /entities?entity_type=, and entities list --type <type> --json (plus --include-merged for the second). MCP also asserts excluded_merged true then false. HTTP and CLI do not assert excluded_merged. CLI pages with --limit 1 --cursor <next_cursor>.

In tests/integration/entity_query_deleted_count.test.ts, the never-observed case currently pins page-only. Change it so w_bare is in total (page length 4, total 4). Leave the other cases in that file green.

Build order.

  1. Replace the fast-path count; add the never-observed term on the filtered branch.
  2. Update the never-observed assertion in entity_query_deleted_count.test.ts.
  3. Add entity_query_total_page_parity.test.ts. Run it and entity_query_deleted_count.test.ts.
  4. Append the sentence to the yaml and both OpenAPI total properties. npm run openapi:generate.
  5. npm run generate:test-catalog, then npm run validate:test-catalog.
  6. PR body includes closes #2467 and Design basis: docs/foundation/principles.md#3-validate-the-instrument-before-believing-the-measurement.

QA / Test Plan

Surface. MCP tool retrieve_entities is the primary agent-facing surface (default eval format below); POST /entities/query, GET /entities, and CLI entities list are secondary surfaces the Eng plan requires parity on and are covered by the integration test file, not duplicated as separate agentic_eval fixtures.

Eval (agentic_eval fixture — primary deliverable)

New fixture: tests/fixtures/agentic_eval/retrieve_entities_total_page_parity.json

  • meta.id: retrieve_entities_total_page_parity
  • events: a single turn invoking MCP tool retrieve_entities for a fixture entity type seeded with the five-row shape from the Eng plan (2 live, 1 merged-away via mergeEntities, 1 merged-away with stale snapshot, 1 soft-deleted, 1 never-observed).
  • assertions.default:
    • entity_stored-style structural check is not applicable here; use a tool_output_shape / custom assertion type (whichever the harness's typed-assertion vocabulary already supports for numeric field checks — confirm against existing assertion types in tests/fixtures/agentic_eval/*.json before inventing a new assertion kind) asserting total === 3 and excluded_merged === true on the default call.
    • Second event (or second fixture) with include_merged: true asserting total === 5.
    • turn_compliance: response contains no fabricated denominator language (e.g. does not surface 79-style pre-exclusion counts).
  • This fixture runs under npm run eval:tier1 via the existing harness × model matrix — no new CI wiring needed, it rides agentic_evals.

If the repo's typed-assertion vocabulary cannot express "assert numeric field X equals Y" today, that's a harness gap worth flagging separately (not blocking this PR) — fall back to the integration test below as the authoritative check and note the fixture as best-effort/illustrative only in the PR body.

Regression test (existing bug)

tests/integration/entity_query_deleted_count.test.ts:

  • Update the never-observed case: w_bare now counted in total (page length 4, total 4) — per Eng plan. This is the direct regression assertion for the bug as reported (79 vs 72 vs 93 mismatch class).
  • All other existing cases in this file remain green — run and diff before/after.

New contract test — tests/integration/entity_query_total_page_parity.test.ts

Seed (single user, single type, direct inserts — not exclusively through mergeEntities):

  1. two live rows (entity + snapshot + observation)
  2. one merged-away row via mergeEntities (snapshot deleted)
  3. one merged-away row inserted directly with merged_to_entity_id set, snapshot row NOT deleted (the exact gap fix(entities): stop re-scanning observations to count visible entities #2267 left open)
  4. one soft-deleted row (softDeleteEntity)
  5. one never-observed row (entity row only, no snapshot, no observation)

Definition-of-done checklist, each run against all four surfaces (MCP retrieve_entities, POST /entities/query, GET /entities?entity_type=, CLI entities list --type <type> --json):

  • Default call (include_merged omitted): returned ids == {live×2, never-observed} (3 rows). total === 3. Rows 2, 3, 4 absent from both the page and the count.
  • Regression guard for the reported bug shape: assert total is NOT 3 + merged_count — i.e. explicitly assert total !== 5 is insufficient by itself; assert equality to the fetchable set size, not just inequality to the old wrong value. (Equality assertions above already cover this; call out here so a future refactor can't silently reintroduce the additive bug pattern.)
  • include_merged: true: returned ids == {live×2, never-observed, merged-via-mergeEntities, merged-with-stale-snapshot} (5 rows). total === 5. The stale-snapshot merged row (case 3) appears exactly once — this is the specific double-count edge case countMergedAway + snapshot count produced; assert count of that row's id in the result array is exactly 1.
  • Pagination parity, default: limit: 1, follow next_cursor until absent. Distinct collected ids have length 3, set-equal to the unpaged default call's ids, and that length equals total from either call. Fails if total exceeds the fetchable set (reproduces the reported "while fetched < total never terminates" failure mode).
  • Pagination parity, include_merged: same with limit: 1 and include_merged: true — 5 distinct ids, set-equal to unpaged, equal to total.
  • MCP-only: excluded_merged is true on the default call and false on include_merged: true. HTTP and CLI surfaces are not asserted on this field (per Eng plan — REST's omission is REST /entities/query response omits excluded_merged (MCP/REST envelope disagreement) #2268, out of scope here).
  • CLI paging shape: --limit 1 --cursor <next_cursor> chain terminates and produces the same distinct-id set as above.
  • Assertions are on the id set and integer counts, not on schema validity or field presence alone — a passing-but-wrong-number result must fail the test.

Edge cases for the new/changed branches (Eng plan's countVisibleEntities)

  • Fast branch, no filters, no merged rows exist at all: total equals live+never-observed count; confirms the deleted countLiveSnapshots/countMergedAway removal didn't regress the zero-merge case.
  • Fast branch, all rows merged-away: total === 0 on default call, total === N on include_merged: true. Guards against an off-by-construction error where the base predicate over-excludes.
  • Filtered branch (updated_since/created_since/published/etc.): never-observed row included in total only when no published/published_after/published_before filter is set; excluded when one is (per Eng plan note that a published filter can't match a snapshot-less row). Add one case with published_after set and confirm the never-observed row drops out of both the page and total together (parity preserved even when the row is legitimately excluded).
  • Soft-deleted row never appears in total under any filter combination or include_merged value — negative assertion, run once per branch (fast and filtered).

Contract / description tests

  • docs/developer/mcp/tool_descriptions.yaml: assert (via existing description-lint or a simple string-contains test on the loaded yaml) that the retrieve_entities total sentence from the Eng plan is present verbatim, including the "not the length of entities on this page" and "not get_entity_type_counts" clauses.
  • openapi.yaml: total property description matches the same sentence on both GET /entities (listEntities) and POST /entities/query (queryEntities) 200 schemas — a snapshot/string test against the generated src/shared/openapi_types.ts comment (post npm run openapi:generate) or against the source yaml directly.
  • Confirm src/tool_definitions.ts desc() fallback string was NOT modified (Eng plan explicitly forbids duplicating the sentence there) — a grep-based guard or comment in the PR description suffices; not worth a runtime test.

Out of scope for this eval (do not assert, per PM/Eng scope)

CI wiring

  • entity_query_total_page_parity.test.ts and the amended entity_query_deleted_count.test.ts run under the existing integration test lane (no new lane needed).
  • New agentic_eval fixture rides npm run eval:tier1 / agentic_evals CI lane automatically once added to tests/fixtures/agentic_eval/.
  • npm run generate:test-catalog then npm run validate:test-catalog pass with the new test file registered (per Eng build order step 5).

Definition of done (roll-up)

  • Regression assertion in entity_query_deleted_count.test.ts updated and green.
  • entity_query_total_page_parity.test.ts created, all four surfaces covered, all bullets above pass.
  • retrieve_entities_total_page_parity agentic_eval fixture created and green under eval:tier1 (or flagged as best-effort with reason if the assertion vocabulary can't express it — see note above).
  • Tool/OpenAPI description tests pass; src/shared/openapi_types.ts regenerated and committed.
  • Test catalog regenerated and validated.
  • CI agentic_evals lane green on the PR — link included in the QA report posted to the issue.

Sign-off condition. Sign off qa gate only once all boxes above are checked and eval:tier1 / agentic_evals is green on the PR's actual commit — not on this plan being written.

Security / Arch

Prior art: no open PR changes this total. Merged #2267 (countVisibleEntities) is the count this issue corrects. #2269, #2380, and #2268 stay out of scope. GET /entities is already covered in tests/security/tenant_isolation_matrix.test.ts.

Decision. One total. On a listing with no search, it is how many rows this same authenticated request returns if next_cursor is followed until it is absent, after every filter that request applied, including merge exclusion. Not the length of entities. Not get_entity_type_counts. No second field.

Options.

  1. Chosen. Replace the fast-path snapshot count plus additive countMergedAway with one user-scoped predicate that matches queryEntities. Coupling stays inside countVisibleEntities. Revert is that function.
  2. Set total from an unpaged queryEntities (already done when identityBasis or snapshotFilters is set). This cannot drift from the page, because it is the page. It reloads ids on every listing, which is the cost fix(entities): stop re-scanning observations to count visible entities #2267 removed. Right only if the fast path is dropped.
  3. Add pageable_total and leave total as a census. Callers keep compiling. The pager still stops on the wrong number. A second field is the parallel instrument docs/foundation/principles.md#6-extend-the-mechanism-that-already-generalizes-do-not-build-a-parallel-one forbids.

Design basis. Conforms to docs/foundation/principles.md#3-validate-the-instrument-before-believing-the-measurement. The planted positive is a fixture whose fetchable id set equals total. A 200 with an integer present is not evidence.

Contract-first (docs/architecture/change_guardrails_rules.mdc, docs/architecture/openapi_contract_flow.md).

  • No new operation, tool, command, response field, or error code. No contract_mappings.ts row. total stays an integer. The OpenAPI diff is description-only.
  • In openapi.yaml, set description on total for both GET /entities (listEntities, 200) and POST /entities/query (queryEntities, 200) to the Eng sentence, then this following sentence: Search totals are not this contract (#2269). Run npm run openapi:generate and commit src/shared/openapi_types.ts in the same PR, with the handler, not after.
  • Append those same two sentences to retrieve_entities in docs/developer/mcp/tool_descriptions.yaml. Do not replace the pagination text. Do not copy them into the desc() fallback in src/tool_definitions.ts (docs/foundation/principles.md#9-one-source-defined-once-a-comment-claiming-parity-is-not-parity).
  • Do not add a behavioral paragraph to docs/developer/cli_agent_instructions.md. docs/developer/agent_instructions_sync_rules.mdc forbids a second copy of a rule that already lives in docs/developer/mcp/instructions.md. Deep pagination already stops when next_cursor is absent. That remains the terminator. Do not add a loop on fetched < total.
  • No previously accepted request shape starts failing, so the tightening hint in docs/subsystems/errors.md does not apply. Do not invent an ERR_* for the old number. If the count query errors, throw into the existing 500 envelope. Do not catch it and return total: 0 with 200.
  • excluded_merged stays MCP-only (REST /entities/query response omits excluded_merged (MCP/REST envelope disagreement) #2268). Do not add it on REST or CLI in this change.
  • MCP retrieve_entities, POST /entities/query, GET /entities, and CLI entities list --api-only all keep calling queryEntitiesWithCount. A count that exists on only one of those surfaces is a structural miss.

Tenant isolation (docs/architecture/change_guardrails_rules.mdc MUST 5; GHSA-wrr4-782v-jhwh).

  • userId is the value getAuthenticatedUserId already passes into queryEntitiesWithCount. A body or query user_id is not an access-control input.
  • countVisibleEntities keeps userId required. Do not make it optional to match getDeletedEntityIds. If it is absent, fail closed. Do not count across users.
  • The outer entities predicate and both probes (entity_snapshots EXISTS, observations NOT EXISTS) bind that same user_id. An unscoped NOT EXISTS on observations would treat another user's observation as this user's and drop or keep the wrong row.
  • Do not add a new route row to tests/security/tenant_isolation_matrix.test.ts. GET /entities is already there. Extend that case, or the new parity test with two users, so user B's entities and total exclude user A's never-observed row and user A's merged row that still has a snapshot. Assert the id set and the integer, not status 200 alone.

Idempotency and writes.

  • This change is a read. No snapshot repair, no backfill, no new mutating endpoint. No new idempotency_key.
  • Tests seed through existing mergeEntities and softDeleteEntity. Do not add a write that deletes leftover snapshots.

Auth and credentials.

  • Same authentication as today's listing. No guest expansion and no new trust tier.
  • Fixtures use the local test server. No production base URL, bearer token, or operator user id in the test file or the agentic_eval fixture.

Reversibility. High. Revert countVisibleEntities. Callers who stored the old census see a smaller total. That is the fix. Stale snapshot rows stay; repairing them is a different change.

Risks, not blocking. The observations probe stays an indexed existence check and does not read observation fields (#2266). If never-observed rows are common, watch listing latency. Do not drop those rows from total while the page still returns them. The Eng sentence alone would claim search totals already match the pageable set; the second sentence is the bound until #2269.

Sign-off. gate_status.arch read back signed_off. ux is not_required, so current_owner advances to cicada (read back). Verdict comment: #2467 (comment)

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingcontract_discrepancylanius-triageApplied by Lanius triage workflowquestionFurther information is requested

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions