fix(server): repair tool_use integration tests broken in #3637 - #3655
Closed
bokelley wants to merge 1 commit into
Closed
fix(server): repair tool_use integration tests broken in #3637#3655bokelley wants to merge 1 commit into
bokelley wants to merge 1 commit into
Conversation
Three test files added in #3637 (route-level integration tests for the tool_use refactor) broke the Server integration tests CI job. Each had a distinct cause: 1. property-enhancement-function.test.ts — the AdAgentsManager mock used `vi.fn().mockImplementation(() => ({...}))`. The arrow function passed to mockImplementation cannot be called with `new`, but property-enhancement.ts does `new AdAgentsManager()` at module scope. Failure was a module-load TypeError, killing all tests in the file. Fix: use `class FakeAdAgentsManager { ... }` so the mock supports `new`. 2. brand-classifier-route.test.ts and brand-enrichment-route.test.ts — SUFFIX used `${process.pid}_${Date.now()}`. Underscores are invalid in domain names per RFC 1035, and both `enrichBrand` and the seed loop in `expandHouse` validate against `^[a-z0-9.-]+\.[a-z]{2,}$`. The route returned {status: 'failed', error: 'Invalid domain format'} — yielding HTTP 500 where the tests expected 200, and seeded=0 sub-brands where they expected 2. Fix: use a hyphen separator. 3. prospect-triage-function.test.ts (not in the original failure list, but flakily fails ~1-2/3 runs locally) — triageEmailDomain fired logTriageDecision without awaiting, so the test could query prospect_triage_log before the INSERT completed. logTriageDecision swallows its own errors, so awaiting it is contract-preserving and cannot cause triage to fail. Fix: await the call; removed the now-redundant outer .catch(). Verified locally: all 4 test files pass (9/9 tests), prospect-triage stable across 3 consecutive runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Superseded by #3656, which merged ~2 minutes earlier with the same three fixes (hyphen SUFFIX + class mock + prospect-triage flake). Different choice on prospect-triage: #3656 added an |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three integration-test failures introduced in #3637 (route-level integration tests for the tool_use refactor) that have been breaking the
Server integration testsCI job onmain. Each had a distinct cause:property-enhancement-function.test.ts— module-loadTypeError. TheAdAgentsManagermock usedvi.fn().mockImplementation(() => ({...})). The arrow function passed tomockImplementationcannot be called withnew, butproperty-enhancement.ts:18doesnew AdAgentsManager()at module scope. Fix: replace withclass FakeAdAgentsManager { validateDomain = mocks.validateDomain }.brand-classifier-route.test.ts+brand-enrichment-route.test.ts—SUFFIXused${process.pid}_${Date.now()}. Underscores are invalid in domain names per RFC 1035, and bothenrichBrandand the seed loop inexpandHousevalidate against^[a-z0-9.-]+\.[a-z]{2,}$. The route returned{status: 'failed', error: 'Invalid domain format'}— yielding HTTP 500 where the tests expected 200, and seeded=0 where they expected 2. Fix: hyphen separator.prospect-triage-function.test.tsflakiness (not in the original CI failure list — that run got lucky — but reproducibly fails ~1-2/3 runs locally).triageEmailDomainfiredlogTriageDecisionwithout awaiting, so the test queriedprospect_triage_logbefore theINSERTlanded.logTriageDecisionalready swallows its own errors internally, so awaiting it is contract-preserving and cannot cause triage to fail. Fix:awaitthe call; remove the now-redundant outer.catch().Why the production code change is safe
logTriageDecision(lines 247–267 ofprospect-triage.ts) wraps itspool.queryin try/catch and only logs warnings — awaiting cannot bubble new errors. All 3 callers oftriageEmailDomainalreadyawaitit; no caller depended on early return. The added latency is one synchronous INSERT intoprospect_triage_logon a function that already does an Anthropicmessages.create(~hundreds of ms), so it's noise.The deeper signal: the test was correctly asserting on a side effect that the implementation was firing-and-forgetting. The contract should be "function returned" = "audit row exists." Awaiting aligns the implementation with that invariant.
Verification
prospect-triage-function.test.tsstable across 3 consecutive runs.code-reviewerandnodejs-testing-expertagents — both say ship; no blockers.Out of scope (follow-up)
brand-properties-parse.test.ts:69andaddie-brand-property-tools.test.ts:51still use${process.pid}_${Date.now()}underscore-SUFFIX. They pass today because their domains skip the RFC 1035 regex, but they're latent traps. Worth a separate issue to standardize hyphen-SUFFIX across all integration tests (or extract a shared helper). Not in this PR.🤖 Generated with Claude Code