test: close merged review coverage gaps - #2597
branarakic wants to merge 3 commits into
Conversation
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 20th, 2026 9:00 AM.
| }); | ||
| }); | ||
|
|
||
| it('gives canonical tracker attribution precedence over its legacy compatibility map', () => { |
There was a problem hiding this comment.
🔴 Bug: The "canonical attribution precedence" test can pass even if normalization ignores attributions and uses the legacy map.
What's wrong
normalizeRpcUsageWindow branches on the presence of attributions: when present it ignores the legacy ethCallByConsumer/ethGetLogsByConsumerAndEndpointSlot maps and rebuilds ethCallByConsumer from the attributions array. The new test feeds a window produced by RpcUsageTracker.drainWindow(), and that method builds attributions from ethCallByConsumer (rpc-usage.ts:438-465), so the two representations are always identical in a tracker-drained window. Since the assertions only check values that are the same either way, a regression that reverted normalization to reading the legacy map would still pass green — the test pins { 'token.balanceOf': 1 } regardless of which branch the implementation takes, which is exactly the "false confidence" failure mode for a test whose name claims precedence.
Example
The precedence shape is only observable when the two fields disagree. A window such as normalizeRpcUsageWindow({ byMethod: { eth_call: 3 }, ethCallByConsumer: { 'stale.legacy': 9 }, attributions: [{ method: 'eth_call', consumer: 'token.balanceOf', count: 3 }], lifetimeTotal: 3 }) must yield ethCallByConsumer: { 'token.balanceOf': 3 } (and drop stale.legacy). The added test never constructs that input, so an implementation returning { 'stale.legacy': 9 } would still pass every assertion in this test. A second unexercised shape is a window that carries an eth_getLogs attribution alongside ethCallByConsumer; with a legacy-map implementation that eth_getLogs attribution would silently disappear, and the test would not notice.
Suggested direction
Keep the tracker round-trip case for the "both fields present and agree" shape, and add a hand-built dual-format window whose legacy map and canonical attributions disagree (including at least one eth_getLogs attribution) and assert the canonical values win and the stale legacy entries are absent.
For Agents
In packages/chain/test/rpc-usage.unit.test.ts (around the new test at line 287), replace or extend the tracker-driven input with a hand-written window object that sets ethCallByConsumer to values that contradict attributions (e.g. legacy { 'stale.legacy': 9 } vs canonical [{ method: 'eth_call', consumer: 'token.balanceOf', count: 3 }]), optionally adding an eth_getLogs attribution. Preserve the existing tracker round-trip assertions. The test must fail if normalizeRpcUsageWindow prefers the legacy map; assert the normalized ethCallByConsumer equals the canonical consumer and that attributions still contains the eth_getLogs entry.
| }); | ||
|
|
||
| it('does not let named metadata graphs consume the root graph capture cap', async () => { | ||
| it('captures the root graph with one behavioral store read and no graph enumeration', async () => { |
There was a problem hiding this comment.
🟡 Issue: The rewritten capture test drops the only guard that named subgraph metadata graphs cannot enter the legacy SWM capture
What's wrong
The deleted tests does not let named metadata graphs consume the root graph capture cap and uses one bounded fully correlated capture query were the only place asserting that the one-time RFC-64 capture query (a) filters to the canonical root workspace meta graph rather than per-subgraph .../_shared_memory_meta/<name> graphs and (b) stays bounded by LIMIT 100_001. The replacement test seeds a single root graph and then asserts the boundary count stays 1 after a restart — but at that point the durable capture file already exists, so initializeRfc64LegacySwmBoundaryV1 reads the capture and never re-runs the head query. The assertion therefore cannot observe the named-graph exclusion it appears to protect. Among the remaining suites, the live Blazegraph fixture only ever writes the root META_GRAPH, so no test now exercises a named subgraph metadata graph against the live capture query. This matters because the caps are fail-closed: if the capture query regressed to include named metadata graphs, an install with many subgraph lanes could exceed RFC64_LEGACY_SWM_META_GRAPH_LIMIT_V1/RFC64_LEGACY_SWM_HEAD_LIMIT_V1 and refuse to start (or scan far more of the store) rather than silently corrupting data — a startup-availability regression that would ship unnoticed.
Example
Current replacement test (line 306):
// capture file is written here
await initializeRfc64LegacySwmBoundaryV1(firstOwner, root, store);
expect(readRfc64LegacySwmBoundaryCountV1(firstOwner, CONTEXT_GRAPH_ID)).toBe(1);
// a named subgraph head is inserted only AFTER the capture exists...
await store.insert(legacySwmBoundaryFixtureQuadsV1({ graph: SUBGRAPH_META_GRAPH, ... }));
const restartedOwner = {};
await initializeRfc64LegacySwmBoundaryV1(restartedOwner, root, store);
expect(readRfc64LegacySwmBoundaryCountV1(restartedOwner, CONTEXT_GRAPH_ID)).toBe(1);
Expected observation that is now missing: initialize against a fresh persistence root with (i) a root-graph legacy head and (ii) 16_384+ named subgraph _shared_memory_meta/<name> heads, and assert the capture count is exactly 1 and the agent.rfc64.legacySwmBoundary.readHeads query still carries LIMIT 100001.
Suggested direction
Keep the behavioural simplification but restore one capture-path assertion set: a fresh-root initialize with a root head plus named subgraph metadata heads, asserting the captured count excludes the named graphs and that the emitted query remains bounded (e.g. source-filtered query text asserting LIMIT 100001 and the two shareOperationId ?shareId correlations, or an equivalent count-based proof). Splitting it into a small dedicated test avoids re-coupling the whole suite to query text.
Confidence note
Medium confidence: I could not execute the suites (no node_modules in this worktree), so the claim rests on reading the production capture path (captureRfc64LegacySwmBoundaryV1) and confirming the live fixture writes only META_GRAPH. The remaining live assertions (count === LEGACY_HEAD_COUNT with 20_000 share-mismatched heads and >100_000 historical operation rows) still cover the share-id correlation and bound behaviourally, so the residual risk is specifically the named-subgraph exclusion and the explicit bound pinning.
For Agents
Look at captureRfc64LegacySwmBoundaryV1 in packages/agent/src/rfc64/legacy-swm-boundary-v1.ts and the deleted tests at the old lines ~294-344 of packages/agent/test/rfc64-legacy-swm-boundary-v1.test.ts. Preserve the new real-OxigraphStore, no-listGraphs behaviour, and add/reinstate a capture-path case proving named subgraph metadata graphs do not consume the legacy boundary. The fixing test should fail if the FILTER(... CONCAT ...) canonical-meta-graph restriction or the LIMIT 100_001 bound is removed from the capture query.
| const MAX_REPORTED_FILES = 200; | ||
|
|
||
| function isIdentityWalletEvmPath(filePath) { | ||
| return /^packages\/node-ui\/(?:src\/ui\/(?:web3\/(?:identityWalletActions|browserWalletTransaction)\.[cm]?[jt]sx?|pages\/identity-wallets\/)|integration\/identity-wallet-actions-v10\.test\.ts$)/ |
There was a problem hiding this comment.
🟡 Issue: The node-ui identity-wallet EVM rule is a hardcoded, unguarded second copy of the chain-scope test ownership declared in scripts/ci/evm-test-scopes.mjs.
What's wrong
The planner now decides the chain EVM scope from an inline regex that enumerates exactly two src/ui/web3 modules, then adds a whole page directory and the integration test path at two other granularities. The coupling it encodes — "the chain scope's real-EVM journey lives in packages/node-ui and exercises these modules" — is already declared canonically in EVM_TEST_SCOPES.chain.files (scripts/ci/evm-test-scopes.mjs:10). Nothing links the two copies, and the planner copy is an allowlist: add a wallet module to the journey, move browserWalletTransaction.ts into a subdirectory, or rename either module, and planCi silently keeps returning evmScopes: [] while every existing test stays green. That silently re-opens exactly the coverage gap this PR is closing, in a gate whose whole purpose is to fail closed. The single expression also hides three distinct rules behind nested alternations with no comment about where the knowledge came from, which is harder to reason about than the data-shaped maps this file already uses (WORKSPACE_OWNING_EVM_SCOPES, WORKSPACE_RULES[].evmScopes, SOLIDITY_RELEVANT_PATHS).
Example
Follow-up PR edits packages/node-ui/src/ui/web3/session.ts (or renames browserWalletTransaction.ts) while keeping integration/identity-wallet-actions-v10.test.ts green. planCi({ eventName: 'pull_request', changeEntries: [change('packages/node-ui/src/ui/web3/session.ts')] }) returns evmScopes: [], so the chain EVM lane is skipped even though the journey it protects changed — and the new assertion in scripts/lib/__tests__/ci-delta.test.mjs still passes because it only covers the two hardcoded names.
Suggested direction
Make the mapping data that sits next to the fact it derives from: either export the scope's source-trigger paths alongside EVM_TEST_SCOPES.chain.files in scripts/ci/evm-test-scopes.mjs and have ci-delta.mjs consume them (note the trusted-controller sparse-checkout lists in .github/workflows/ci.yml and evm-integration.yml currently fetch only plan-ci.mjs, assert-ci-results.mjs, ci-delta.mjs, ci-results.mjs, so a new import needs those lists updated), or at minimum declare a module-level table in ci-delta.mjs (path predicate -> scopes + reason, like WORKSPACE_OWNING_EVM_SCOPES) with a comment naming EVM_TEST_SCOPES as the source of truth. Either way, cover the coupling with a focused test next to the EVM routing tests rather than two more cases inside the general 'leaf and shared package snapshots' test, so a new wallet module fails a test instead of silently shrinking coverage.
Confidence note
The narrow module list may be deliberately limited to the modules the journey currently imports; the concern is the unguarded duplication of that fact, not the exact set. I verified EVM_TEST_SCOPES.chain.files names the node-ui integration test and that no existing test ties the planner rule to that declaration.
For Agents
In scripts/lib/ci-delta.mjs, replace the isIdentityWalletEvmPath regex + inline branch (lines 311-313, 556-559) with a declared path->EVM-scope mapping whose source of truth is co-located with or derived from EVM_TEST_SCOPES in scripts/ci/evm-test-scopes.mjs. Preserve current behavior: packages/node-ui/src/ui/web3/identityWalletActions.ts and browserWalletTransaction.ts, src/ui/pages/identity-wallets/**, and integration/identity-wallet-actions-v10.test.ts select evmScopes: ['chain'] with the existing reason text, while other node-ui paths select none. Prove it with a focused planner test (adding a new wallet module under those trigger paths must select chain), and update both workflow sparse-checkout lists if a new module import is introduced.
| assert.deepEqual(localLlm.evmScopes, []); | ||
|
|
||
| const identityWallet = pullRequestPlan([ | ||
| change('packages/node-ui/src/ui/web3/identityWalletActions.ts'), |
There was a problem hiding this comment.
🟡 Issue: The identity-wallet EVM routing test covers only one of the four path shapes the new regex matches.
What's wrong
isIdentityWalletEvmPath is a four-branch alternation (web3/identityWalletActions., web3/browserWalletTransaction., pages/identity-wallets/*, and integration/identity-wallet-actions-v10.test.ts), and it is the only thing that selects the real-EVM chain scope for node-ui identity-wallet changes. The added test asserts the plan for identityWalletActions.ts plus a negative for Dashboard.tsx, so a typo or regression in the other three branches (or an over-narrow extension class like .ts only) would leave the suite green while identity-wallet changes silently plan with evmScopes: [] and skip the chain lane. The verification notes flag exactly these paths as the ones that matter, and I confirmed with node that all four branches currently match and that neither Dashboard.tsx nor the sibling web3 files match.
Example
change('packages/node-ui/src/ui/web3/browserWalletTransaction.ts') or change('packages/node-ui/src/ui/pages/identity-wallets/useIdentityWalletManagement.ts') plans with evmScopes: [] if the corresponding alternation branch is broken, and no test in this file would fail. Same for packages/node-ui/integration/identity-wallet-actions-v10.test.ts.
Suggested direction
Turn the new block into a small table over the four matched shapes (identityWalletActions.ts/.tsx, browserWalletTransaction.ts, a file under pages/identity-wallets/, and the integration test) asserting evmScopes deep-equals ['chain'] and the reason string, plus the Dashboard.tsx negative.
Confidence note
I could not run the Node test suite in this worktree (no node_modules); the regex match results above were produced by evaluating the exact pattern with node against the real repository paths.
For Agents
In scripts/lib/tests/ci-delta.test.mjs near line 272, expand the added case into an it.each/loop over packages/node-ui/src/ui/web3/identityWalletActions.tsx, packages/node-ui/src/ui/web3/browserWalletTransaction.ts, a file under packages/node-ui/src/ui/pages/identity-wallets/, and packages/node-ui/integration/identity-wallet-actions-v10.test.ts, asserting each yields evmScopes of ['chain'] and the identity-wallet browser actions reason; keep the Dashboard.tsx negative. Preserve the existing behavior that only these path shapes select chain.
| import type { SyncPageResult } from '../../src/sync/requester/page-fetch.js'; | ||
| import { createSharedMemorySnapshotMaterializer } from | ||
| '../../src/sync/requester/swm-snapshot-materializer.js'; | ||
| export { MemoryWorkspaceSnapshotStore as MemorySnapshotStore } from |
There was a problem hiding this comment.
🔵 Nit: The re-export keeps a second name for the shared MemoryWorkspaceSnapshotStore.
Why it matters
An alias that only exists to avoid updating two import lists adds a naming indirection: readers must chase it to learn which store implementation a test uses, and future callers have no signal about which name is canonical.
Suggestion
Drop the alias (or re-export under the canonical name) and update the two importers (swm-recovery.test.ts, swm-recovery-revocation.test.ts) so the codebase has one name for the shared helper; folding the remaining local copies into the shared module can follow separately since they are pre-existing.
| rfc64CatalogOverride?: RequestContext['rfc64Catalog'], | ||
| rfc64PublicCatalogOverride?: RequestContext['rfc64PublicCatalog'], | ||
| routeRpcTransport?: DaemonRouteRpcTransport, | ||
| opWalletsOverride: RequestContext['opWallets'] = { wallets: [] }, |
There was a problem hiding this comment.
💡 Suggestion: The new wallet-balances suite appends an eighth positional override to the shared request harness and re-copies the fake-RPC/runtime scaffolding.
Why it matters
Positional placeholder calls and copy-pasted server/runtime setup make the test file harder to scan and to extend, and they hide which options actually matter to a given scenario.
Suggestion
Give the new suite its own path: either convert requestStatusWithAgent to an options object (or add an options-object overload) so the wallet case is self-documenting, or extract a small helper that starts the fake JSON-RPC server and returns the governed runtime, then move the /api/wallets/balances describe block into its own test file that uses it.
| publish([root, siblingRoot], [...payload, ...siblingPayload]), | ||
| publish([root], payload, 'research'), | ||
| ]); | ||
| const recoveryOrderedMeta = ( |
There was a problem hiding this comment.
💡 Suggestion: Manifest ordering was removed from the shared publisher fixture and re-derived inside one consumer.
Why it matters
Order-sensitive expectations that depend on incidental CONSTRUCT ordering are a classic source of flaky tests; keeping the ordering policy with the fixture data makes the dependency explicit and reusable.
Suggestion
Export the ordering from the fixture module (for example manifestOrderedMeta(fixture) or an opt-in ordered view) so the policy is defined once next to slices/sliceSubject and consumers opt in explicitly, and have the recovery test call that instead of re-deriving slice order locally.
…nifest
Review follow-ups on the coverage this PR adds.
rpc-usage precedence: the added test drained a live RpcUsageTracker, so its
ethCallByConsumer map and its attributions array are built from the same
counters and always agree — the test could not observe the precedence its
name claims, and passed unchanged when normalizeRpcUsageWindow was forced
down the legacy branch. Kept the round trip as the "both present and
agreeing" case and appended a hand-built window where the two disagree, so
the legacy branch now yields { 'stale.legacy': 9 } and drops the
eth_getLogs attribution. No cast needed: ethCallByConsumer, attributions
and ethGetLogsByConsumerAndEndpointSlot are all optional on RpcUsageWindow.
LIMIT 100001: rewriting the legacy-SWM capture tests behaviourally removed
the only assertion on the capture query's LIMIT. The fail-closed head cap
(bindings.length > RFC64_LEGACY_SWM_HEAD_LIMIT_V1) is reachable only while
the query fetches one row MORE than the limit, so a silent edit to
LIMIT 100000 would make it dead code with nothing failing. `git grep`
confirms no other assertion pins the literal. Re-added that one line only;
the BIND/STRENDS/share-id query-text assertions stay removed because
'captures only fully joined legacy heads through the real Oxigraph query'
covers them behaviourally.
ci-delta identity-wallet rule: lifted the alternation into a module-level
IDENTITY_WALLET_EVM_PATTERNS table, matching the shape the file already
uses for SOLIDITY_RELEVANT_PATHS and BLAZEGRAPH_ARM64_PATHS, with a comment
naming EVM_TEST_SCOPES.chain.files as the source of truth and recording why
it cannot be imported: ci-delta.mjs runs from the trusted-controller sparse
checkout, pinned to the four CONTROLLER_POLICY_FILES and enforced by
sparseCheckoutPaths(), so a fifth entry hard-fails every planner job. The
split is a deliberate no-op — the original $ binds only to the integration
alternative, so the web3 pattern keeps no trailing anchor; a 20-path probe
run before and after is byte-identical.
Planner test: moved the two identity-wallet cases out of the leaf/shared
snapshot test into their own routing test covering all four matched path
shapes plus two negatives, and linked the two copies with a test-side
EVM_TEST_SCOPES import so renaming or moving the node-ui journey the chain
scope runs fails here instead of silently shrinking the lane.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| ); | ||
|
|
||
| await vi.waitFor(() => expect(reconcile).toHaveBeenCalledOnce()); | ||
| expect(membershipUpsert).toHaveBeenCalledTimes( |
There was a problem hiding this comment.
🔵 Nit: The storeless branch asserts a never-wired mock was not called, so part of the new test is vacuous
Why it matters
A vacuous assertion adds noise and can imply coverage that does not exist; the meaningful guarantees (awaiting reconciliation, propagating failure) are already asserted for both branches.
Suggestion
Remove the storeless-branch call-count assertion or assert the real observable (no membership store configured).
2 participants
Summary
Follow-up fixes for actionable review findings on already-merged PRs in the #2490+ audit range:
/api/wallets/balancesthrough the daemon-owned governed transport in both healthy and locally saturated casesReview follow-ups
Validation
The live Blazegraph variant was not run locally; this patch only removes query-text coupling from it, while the equivalent behavioral contract passed against real Oxigraph and the CI planner retains the live lane.