Skip to content

test(mcp-builder): cover 4 session setter handlers (48 tests)#167

Merged
trevormil merged 2 commits intomainfrom
test/mcp-session-setters
Apr 22, 2026
Merged

test(mcp-builder): cover 4 session setter handlers (48 tests)#167
trevormil merged 2 commits intomainfrom
test/mcp-session-setters

Conversation

@trevormil
Copy link
Copy Markdown
Collaborator

Summary

  • Adds unit tests for 4 MCP builder session setters under src/builder/tools/session/ that had zero test coverage
  • 48 new tests (1933 to 1981 total), all passing
  • No bugs discovered; existing behavior pinned down

Handlers covered

  • handleSetValidTokenIds — uint64 range validation (negative, MAX_UINT64 overflow, start>end, non-numeric), session mutation, sessionId isolation
  • handleSetInvariants — each invariant shape (simple, subscription-style, smart-token with cosmosCoinBackedPath), null-clear, re-call replacement, session isolation
  • handleSetMintEscrowCoins — 1-entry chain cap, JSON-string coercion fallback (AI quirk), unparseable-JSON error path
  • handleSetStandards — KNOWN_STANDARDS whitelist, dynamic qualifier skip (ListView:*, NFTPricingDenom:*, DefaultDisplayCurrency:*), typo warnings, custom standards still allowed

Test plan

  • npx jest src/builder/tools/session/ passes (48/48)
  • npm test full SDK suite passes (1981/1981)
  • Human review of test cases before merge
  • Ensure no flaky behavior (session state reset between specs)

Tracked in autopilot backlog 0288.

Generated with Claude Code

The MCP builder's per-field session setters under src/builder/tools/session/
had no unit tests. These four handlers are the primary interface the LLM
uses to mutate collection state — a silent regression here corrupts every
collection the builder produces.

Added specs for:
- handleSetValidTokenIds — all validation branches (negative, >MAX_UINT64,
  start>end, non-numeric), plus session mutation and sessionId isolation
- handleSetInvariants — happy paths for each invariant shape, explicit null
  clear, re-call replacement (not merge), session isolation
- handleSetMintEscrowCoins — 1-entry cap, JSON-string coercion fallback,
  parse-failure error path, session mutation
- handleSetStandards — whitelist pass-through, dynamic qualifier skip
  (ListView:*, NFTPricingDenom:*, DefaultDisplayCurrency:*), typo warnings,
  custom standards still allowed

Coverage: 1933 -> 1981 tests (all passing). No bugs discovered.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

This PR adds 48 unit tests (1933 → 1981 total) covering four previously-untested MCP builder session setter handlers under src/builder/tools/session/. The tests pin down validation logic, session mutation behaviour, and per-sessionId isolation for handleSetValidTokenIds, handleSetInvariants, handleSetMintEscrowCoins, and handleSetStandards.

  • All four new spec files use resetAllSessions() in beforeEach, which correctly isolates state between tests.
  • Branch coverage is thorough: every validation error path in handleSetValidTokenIds (negative, overflow, reversed, non-numeric) has a dedicated test; the AI JSON-string coercion quirk in handleSetMintEscrowCoins is exercised; dynamic qualifier skip logic in handleSetStandards is verified for all three known prefix patterns.
  • Minor test-quality issue: the updateValidTokenIds, updateInvariants, and updateStandards flag assertions don't actually verify the handler wrote the flag — these flags default to true in every freshly-initialized session template, so the assertions are vacuous and would not catch a regression where the setter stopped writing the flag.
  • One comment in setValidTokenIds.spec.ts is misleading about session non-existence at the point of assertion.

Confidence Score: 5/5

Test-only PR with no production code changes — safe to merge.

All 48 tests pass and coverage is broad. The only findings are non-blocking P2 style suggestions about vacuous flag assertions and a misleading comment.

No files require special attention; the vacuous updateXxx=true assertions are worth a follow-up improvement but do not block merge.

Important Files Changed

Filename Overview
packages/bitbadgesjs-sdk/src/builder/tools/session/setValidTokenIds.spec.ts New spec with 17 tests for uint64 range validation, session mutation, and sessionId isolation; two minor style issues flagged in review comments.
packages/bitbadgesjs-sdk/src/builder/tools/session/setInvariants.spec.ts 8 tests covering each invariant shape, null-clear, re-call replacement, and session isolation; updateInvariants=true assertion is vacuous because the default is already true.
packages/bitbadgesjs-sdk/src/builder/tools/session/setMintEscrowCoins.spec.ts 11 tests covering single-entry happy path, chain 1-entry cap enforcement, JSON-string coercion, and session isolation; all assertions test non-default values, no vacuous checks.
packages/bitbadgesjs-sdk/src/builder/tools/session/setStandards.spec.ts 12 tests covering KNOWN_STANDARDS whitelist, dynamic qualifier skip, typo warnings, and session mutation; updateStandards=true assertion is vacuous because the flag defaults to true.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Test: beforeEach resetAllSessions] --> B[Call handler with input]
    B --> C{Validation passes?}
    C -- No --> D[Return success:false with error message]
    C -- Yes --> E[getOrCreateSession]
    E --> F[setXxxInSession]
    F --> G[Return success:true with result]

    subgraph setValidTokenIds
        V1[Iterate ranges] --> V2{start/end numeric?}
        V2 -- No --> D
        V2 -- Yes --> V3{negative or overflow?}
        V3 -- Yes --> D
        V3 -- No --> V4{start > end?}
        V4 -- Yes --> D
        V4 -- No --> E
    end

    subgraph setMintEscrowCoins
        M1{coins is string?} -- Yes --> M2[JSON.parse]
        M2 -- fail --> D
        M2 -- ok --> M3{length > 1?}
        M1 -- No --> M3
        M3 -- Yes --> D
        M3 -- No --> E
    end

    subgraph setStandards
        S1[For each standard] --> S2{contains ':'?}
        S2 -- Yes --> S3[Skip - dynamic qualifier]
        S2 -- No --> S4{In KNOWN_STANDARDS?}
        S4 -- No --> S5[Append warning]
        S4 -- Yes --> E
        S5 --> E
    end
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/bitbadgesjs-sdk/src/builder/tools/session/setValidTokenIds.spec.ts
Line: 125-130

Comment:
**Vacuous `updateValidTokenIds` assertion**

The assertion on line 129 (`expect(session.messages[0].value.updateValidTokenIds).toBe(true)`) doesn't actually verify the handler sets the flag — `updateValidTokenIds` defaults to `true` in every freshly-initialized session (see `sessionState.ts` line 85). The check would pass even if `setValidTokenIds` stopped calling `value.updateValidTokenIds = true`.

To make this assertion meaningful it needs to start from a known-`false` baseline, e.g.:

```ts
it('writes tokenIds into the session and flips updateValidTokenIds', () => {
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  const session = getOrCreateSession();
  // Reset the flag to verify the setter actually writes it
  session.messages[0].value.updateValidTokenIds = false;
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  expect(session.messages[0].value.validTokenIds).toEqual([{ start: '1', end: '5' }]);
  expect(session.messages[0].value.updateValidTokenIds).toBe(true);
});
```

The same default-is-`true` caveat applies to `updateInvariants` in `setInvariants.spec.ts` (line 25) and `updateStandards` in `setStandards.spec.ts` (line 101).

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/bitbadgesjs-sdk/src/builder/tools/session/setValidTokenIds.spec.ts
Line: 117-121

Comment:
**Misleading comment about session non-existence**

The inline comment on line 118 says "Session should not exist since validation failed before getOrCreateSession is called", but the very next statement calls `getOrCreateSession()` which *creates* the default session if it doesn't already exist. The test is logically correct — no invalid token IDs were written, so the freshly-created session shows the default `validTokenIds: []` — but the comment is confusing because it implies the test is checking for session absence when it is actually checking for the default post-creation state.

Consider updating the comment to clarify:

```ts
// Validation aborted before getOrCreateSession was called, so the session
// does not exist yet. Calling getOrCreateSession() here creates a fresh
// blank session whose validTokenIds default is [].
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "test(mcp-builder): cover 4 session sette..." | Re-trigger Greptile

Comment on lines +125 to +130
it('writes tokenIds into the session and flips updateValidTokenIds', () => {
handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
const session = getOrCreateSession();
expect(session.messages[0].value.validTokenIds).toEqual([{ start: '1', end: '5' }]);
expect(session.messages[0].value.updateValidTokenIds).toBe(true);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Vacuous updateValidTokenIds assertion

The assertion on line 129 (expect(session.messages[0].value.updateValidTokenIds).toBe(true)) doesn't actually verify the handler sets the flag — updateValidTokenIds defaults to true in every freshly-initialized session (see sessionState.ts line 85). The check would pass even if setValidTokenIds stopped calling value.updateValidTokenIds = true.

To make this assertion meaningful it needs to start from a known-false baseline, e.g.:

it('writes tokenIds into the session and flips updateValidTokenIds', () => {
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  const session = getOrCreateSession();
  // Reset the flag to verify the setter actually writes it
  session.messages[0].value.updateValidTokenIds = false;
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  expect(session.messages[0].value.validTokenIds).toEqual([{ start: '1', end: '5' }]);
  expect(session.messages[0].value.updateValidTokenIds).toBe(true);
});

The same default-is-true caveat applies to updateInvariants in setInvariants.spec.ts (line 25) and updateStandards in setStandards.spec.ts (line 101).

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/bitbadgesjs-sdk/src/builder/tools/session/setValidTokenIds.spec.ts
Line: 125-130

Comment:
**Vacuous `updateValidTokenIds` assertion**

The assertion on line 129 (`expect(session.messages[0].value.updateValidTokenIds).toBe(true)`) doesn't actually verify the handler sets the flag — `updateValidTokenIds` defaults to `true` in every freshly-initialized session (see `sessionState.ts` line 85). The check would pass even if `setValidTokenIds` stopped calling `value.updateValidTokenIds = true`.

To make this assertion meaningful it needs to start from a known-`false` baseline, e.g.:

```ts
it('writes tokenIds into the session and flips updateValidTokenIds', () => {
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  const session = getOrCreateSession();
  // Reset the flag to verify the setter actually writes it
  session.messages[0].value.updateValidTokenIds = false;
  handleSetValidTokenIds({ tokenIds: [{ start: '1', end: '5' }] });
  expect(session.messages[0].value.validTokenIds).toEqual([{ start: '1', end: '5' }]);
  expect(session.messages[0].value.updateValidTokenIds).toBe(true);
});
```

The same default-is-`true` caveat applies to `updateInvariants` in `setInvariants.spec.ts` (line 25) and `updateStandards` in `setStandards.spec.ts` (line 101).

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +117 to +121
expect(res.success).toBe(false);
// Session should not exist since validation failed before getOrCreateSession is called
const session = getOrCreateSession();
expect(session.messages[0].value.validTokenIds).toEqual([]);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Misleading comment about session non-existence

The inline comment on line 118 says "Session should not exist since validation failed before getOrCreateSession is called", but the very next statement calls getOrCreateSession() which creates the default session if it doesn't already exist. The test is logically correct — no invalid token IDs were written, so the freshly-created session shows the default validTokenIds: [] — but the comment is confusing because it implies the test is checking for session absence when it is actually checking for the default post-creation state.

Consider updating the comment to clarify:

// Validation aborted before getOrCreateSession was called, so the session
// does not exist yet. Calling getOrCreateSession() here creates a fresh
// blank session whose validTokenIds default is [].
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/bitbadgesjs-sdk/src/builder/tools/session/setValidTokenIds.spec.ts
Line: 117-121

Comment:
**Misleading comment about session non-existence**

The inline comment on line 118 says "Session should not exist since validation failed before getOrCreateSession is called", but the very next statement calls `getOrCreateSession()` which *creates* the default session if it doesn't already exist. The test is logically correct — no invalid token IDs were written, so the freshly-created session shows the default `validTokenIds: []` — but the comment is confusing because it implies the test is checking for session absence when it is actually checking for the default post-creation state.

Consider updating the comment to clarify:

```ts
// Validation aborted before getOrCreateSession was called, so the session
// does not exist yet. Calling getOrCreateSession() here creates a fresh
// blank session whose validTokenIds default is [].
```

How can I resolve this? If you propose a fix, please make it concise.

… them

Addresses greptile P2 on PR #167: three assertions
(setValidTokenIds:129, setInvariants:25, setStandards:101) checked
that update*=true after the handler ran, but update* defaults to true
in a freshly-initialized session, making the check vacuous.

Each test now resets the flag to false before calling the handler so
the assertion genuinely verifies the setter writes the flag.

Also clarifies the "session should not exist" comment on
setValidTokenIds:118 — what the test actually exercises is the default
post-creation state, not session absence.

1981 tests pass (60 suites). No runtime change.
@trevormil
Copy link
Copy Markdown
Collaborator Author

Thanks for the review! Applied both P2 fixes in 29c9b7f:

  • Reset update-flags to false before the handler call in all three specs (setValidTokenIds, setInvariants, setStandards). Assertions now genuinely verify the setter writes the flag.
  • Clarified the getOrCreateSession comment in setValidTokenIds.

1981 tests pass.

@trevormil trevormil merged commit 4b2fe42 into main Apr 22, 2026
1 of 3 checks passed
@trevormil trevormil deleted the test/mcp-session-setters branch April 22, 2026 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant