Skip to content

fix(toolsets): register earnings-transcript tools once - #114

Merged
imbenrabi merged 2 commits into
mainfrom
claude/project-thread-jwql4s
Oct 13, 2026
Merged

imbenrabi merged 2 commits into
mainfrom
claude/project-thread-jwql4s

Conversation

@imbenrabi

Copy link
Copy Markdown
Owner

Requested by Ben · project thread

Before: the calendar and earnings tool sets both listed the earnings-transcript module, so its four tools (getLatestEarningsTranscripts, getEarningsTranscript, getEarningsTranscriptDates, getAvailableTranscriptSymbols) were registered twice in ALL_TOOLS mode. Toolception 0.7.0 silently keeps one; 0.8.0 (code-rabi/toolception#33, strict) fails initialize, so the default mode would not start.

After: earnings owns the transcript module and calendar keeps only calendar (still includes getEarningsCalendar). A new test fails on any module listed in two tool sets or any tool name registered twice.

How

  • src/constants/toolSets.ts: calendar.modules is ["calendar"]. earnings is named "Earnings & Transcripts", while calendar is described as dividends/IPOs/splits/earnings dates, so transcripts belong to earnings. No other module is shared between tool sets.
  • __tests__/unit/constants/toolSets.test.ts (new): checks every module has an adapter, appears in exactly one tool set, and that loading every module yields unique tool names. It fails on main with the four tools above.
  • docs/src/tools/AGENTS.md and CHANGELOG.md (Unreleased) updated. docs/API_REFERENCE.md already lists earnings and calendar for these tools, which is still accurate.
  • Behaviour change: in STATIC_TOOL_SETS/dynamic mode, selecting only calendar no longer exposes the transcript tools; select earnings. ALL_TOOLS exposes the same 253 tools as before.

Verification

Merge order

Base is main; this is independent of #113. Both must merge before the server takes toolception 0.8.0 (which then needs verify:toolception -- --source 0.8.0 green once 0.8.0 is on npm).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lq4R64VHie4GLCcJbKtMTS


Generated by Claude Code

The calendar and earnings tool sets both listed the earnings-transcript
module, so its four tools were registered twice in ALL_TOOLS mode.
Toolception 0.8.0 rejects duplicate tool names at initialize.
Drop the module from calendar (earnings owns transcripts) and add a test
that fails on any module or tool name shared between tool sets.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lq4R64VHie4GLCcJbKtMTS
@imbenrabi imbenrabi self-assigned this Oct 10, 2026

Copy link
Copy Markdown
Owner Author

TS review: pipeline-ts-reviewer (feature-pipeline) — round 1

Verdict: APPROVE (no CRITICAL/HIGH findings). Static analysis only, against skills typescript-standards and karpathy-guidelines (marketplace @48f5800).

Reviewed: the diff, src/constants/toolSets.ts, src/types/index.ts, src/toolception-adapters/*, docs/API_REFERENCE.md. Confirmed calendar.ts registers only getEarningsCalendar and earnings-transcript.ts registers the four transcript tools; module and adapter sets match one to one (28 each).

Note on scope: this run used a stale local origin/main ref, so its diff also contained the already-merged 2.7.0 work. Findings 1, 5 and 6 below come from that stale base, not from this PR (the real diff is 4 files, +67/−3). The reviewer is being re-run on the correct diff after the fixes.

# Sev Location Finding
1 LOW CHANGELOG.md [Unreleased] Entry sits under [Unreleased], which is below [2.6.12]/[2.7.0]; if it ships in 2.7.0 it belongs there.
2 LOW CHANGELOG.md User-visible for STATIC_TOOL_SETS/DYNAMIC (calendar no longer exposes transcripts) but filed only as "Fixed"; add a "Changed"/migration note.
3 LOW toolSets.test.ts test 2 No reverse check that every MODULE_ADAPTERS key is referenced by a tool set.
4 LOW toolSets.test.ts test 3 MODULE_ADAPTERS[module]({}) unguarded (obscure TypeError if adapter missing); owners loop duplicated between tests 2 and 3.
5 LOW __tests__/smoke/* New (p: any)/(t: any) callbacks.
6 INFO diff scope Diff contains stacked 0.7 upgrade, verify script, version bump, etc.
{"recommendation":"APPROVE","summary":"The calendar/earnings duplicate-module fix is minimal and correct, and the new catalog test properly guards against shared modules and duplicate tool names; only low-severity polish items remain.","findings":[{"severity":"LOW","file":"CHANGELOG.md","issue":"Entry under [Unreleased] below the 2.7.0 section"},{"severity":"LOW","file":"CHANGELOG.md","issue":"User-visible change filed only as Fixed"},{"severity":"LOW","file":"__tests__/unit/constants/toolSets.test.ts","issue":"No reverse check that every MODULE_ADAPTERS key is referenced"},{"severity":"LOW","file":"__tests__/unit/constants/toolSets.test.ts","issue":"Unguarded adapter call and duplicated owners loop"},{"severity":"LOW","file":"__tests__/smoke/smoke-test-dynamic.test.ts","issue":"any in callbacks (stale-base artifact)"},{"severity":"LOW","file":"(diff scope)","issue":"Stacked changes in diff (stale-base artifact)"}]}

Generated by Claude Code

Add the reverse module check, guard missing adapters, share the owners
helper, and file the changelog entry above 2.7.0 with a Changed note.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lq4R64VHie4GLCcJbKtMTS

Copy link
Copy Markdown
Owner Author

Fix mapping for TS review round 1

Fixes are in c93bb18.

  1. Changelog placement — fixed. The entry now sits in an [Unreleased] section above [2.7.0] (2.7.0 is already released, so the fix ships next). The empty [Unreleased] heading that was buried below 2.5.x is removed.
  2. Changed note — fixed. The entry is split into "Fixed" (the ALL_TOOLS duplicate registration) and "Changed" (calendar no longer exposes transcripts in STATIC_TOOL_SETS/DYNAMIC_TOOL_DISCOVERY; select earnings).
  3. Reverse module check — fixed. The "exactly one toolset" test now also asserts every MODULE_ADAPTERS key is referenced by a tool set.
  4. Unguarded adapter call / duplicated loop — fixed. The module-to-toolsets map is a shared moduleOwners() helper, and the tool-name test asserts the adapter exists before calling it. I kept that test iterating tool set → module so it still fails on a shared module even without the other test (checked: against the old toolSets.ts, 2 of 3 tests fail).
  5. any in smoke tests — not applicable. Those lines are not in this PR; they came from the stale base ref the reviewer diffed against. Nothing to change.
  6. Diff scope — not applicable for the same reason. The PR diff is CHANGELOG.md, docs/src/tools/AGENTS.md, src/constants/toolSets.ts and the new test, based directly on main.

typecheck, lint and the new tests pass. Re-running the reviewer on the corrected diff next.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

TS review: pipeline-ts-reviewer (feature-pipeline) — round 2

Verdict: APPROVE (no CRITICAL/HIGH; 3 LOW nits, none blocking). Static analysis of the full 134-line diff against main (8f99e6d) at c93bb18.

Verified: calendar.modules = ["calendar"] removes the double listing; getEarningsCalendar stays in calendar; a static count of server.tool( in src/tools/*.ts is 253, matching the changelog's "same 253 tools in ALL_TOOLS". All four round-1 fixes are in place (changelog placement and Changed note, reverse adapter check, guarded adapter call, shared moduleOwners() helper). No any, spy restored in afterAll, non-vacuous guard on the tool count.

# Sev Location Finding Disposition
1 LOW docs/API_REFERENCE.md:24,28 Row 24 lists transcripts and getEarningsCalendar together under earnings, calendar; the Calendar row omits getEarningsCalendar. Left as is: the row is a sample of tools across two tool sets and stays accurate, and the file is untouched by this PR. Not worth another review round.
2 LOW toolSets.test.ts:39-54 moduleOwners() TSDoc has no @returns; the file-level doc block is attached to no symbol. Left as is: nit on a test helper; the block explains why the test exists.
3 LOW toolSets.test.ts:72-80 The "exactly one toolset" test makes two assertions, so a first failure hides the second. Left as is: both assertions state one invariant (the module-to-toolset partition), and the first failure already names the offending modules.
{"recommendation":"APPROVE","summary":"Minimal, correct fix: calendar no longer lists earnings-transcript, the ALL_TOOLS count is unchanged at 253, and the round-1 findings are all addressed. Only LOW nits remain.","findings":[{"severity":"LOW","file":"docs/API_REFERENCE.md","line":24,"issue":"Earnings row groups tools across earnings and calendar; Calendar row omits getEarningsCalendar"},{"severity":"LOW","file":"__tests__/unit/constants/toolSets.test.ts","line":45,"issue":"moduleOwners() TSDoc lacks @returns; file-level doc block attached to no symbol"},{"severity":"LOW","file":"__tests__/unit/constants/toolSets.test.ts","line":72,"issue":"One test makes two independent assertions"}]}

Generated by Claude Code

@imbenrabi
imbenrabi marked this pull request as ready for review October 10, 2026 18:23
@imbenrabi
imbenrabi merged commit 2a0a99b into main Oct 13, 2026
2 checks passed
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.

2 participants