fix: unify tool-success mapping and honor telemetryFunctionId (plan 007)#75
Merged
Conversation
The provider races each LLM call against rejectAfterIdle(timeout). When the timer won it only rejected (and the retry condition retries on that message), but nothing aborted the original request — so a legitimately slow generateText/ generateObject kept running while a retry started, producing duplicate concurrent LLM calls (extra tokens/cost, spurious failures). Create a per-attempt AbortController inside the withRetry callback, combine it with the global executionController signal via AbortSignal.any, pass the combined signal to the SDK call (after the ...config spread so it wins), and call controller.abort() inside rejectAfterIdle before rejecting. Applied at both the generateWithTools and generateObject race sites. Deferred (see plan 005): making the 30s idle timeout configurable per agent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Provider.invokeConversation paired tool calls with results by array position and
defaulted a missing `success` field to failed; Conversation.getToolExecutions
paired by toolCallId, unwrapped the {type:json,value} envelope, and defaulted to
passed. Tester (former) and Pilot (latter) could disagree about the same call.
Extract one shared toToolExecution mapper (owns unwrap + `success !== false`
default) and route both sites through it; invokeConversation now pairs by
toolCallId via a Map. Data-shaped tools with no `success` field now count as
successful in both views, matching what Pilot already trusted.
Honor the telemetryFunctionId provider option in getTelemetry (ten call sites
passed it expecting a named Langfuse trace; only `telemetry.functionId` was read).
Explicit telemetry.functionId still wins. Delete the dead
Conversation.telemetryFunctionId field/param/clone-arg (never read by anything).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ad abortSignal Per re-review of the idle-timeout abort fix: - Remove the now-dead `abortSignal: executionController.getAbortSignal()` from the generateWithTools/generateObject config builders — both are overridden by the combined signal at the race site, so they were misleading. - Collapse the duplicated `AbortSignal.any([...].filter(Boolean) as AbortSignal[])` at both race sites into a `combinedAbortSignal(controller)` helper (also drops the cast). - Rename `rejectAfterIdle` -> `abortAfterIdle` (it now aborts, not just rejects) and its sentinel param `signal` -> `cancel` (it is not an AbortSignal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… into fix/tool-success-telemetry
…metry # Conflicts: # src/ai/provider.ts # tests/unit/provider.test.ts
Explorbot Self-RegressionCommit
Attempt details
Session analysis — basic (native): Session AnalysisThe issue tracking list page was tested for creation, search, and filtering capabilities. All core flows work correctly: creating issues, keyword search, status filtering, label filtering, and combined filters all produce the expected results. Coverage
What works
Execution Issues
|
DavertMik
added a commit
that referenced
this pull request
Jul 12, 2026
* chore: dead-code sweep steps 1-2 (action-result, action) action-result.ts: delete uncalled saveBrowserLogs()/saveHtmlOutput(), the now-dead browserLogsContent getter, and the unused Diff.ariaRemoved getter. action.ts: delete uncalled expect(), caputrePageWithScreenshot(), waitForInteraction(), setActor(), getCurrentState() (exact dup of getActionResult()), the unused prevActionResult local and expectation field; drop 6 unused imports. Remove the never-read `experience` param from Action.attempt and Explorer.attemptAction and the dead arg at all call sites (the callers' skip-experience intent was never functional — follow-up). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: dead-code sweep step 3 (explorer) Delete uncalled Explorer methods: ensureActiveTestPageAvailable(), getCurrentUrl(), getConfigPath(), reload(), hasPlaywrightLocator(), and setUserResolve() + the never-read userResolveFn field (drop the forwarding line in explorbot.ts; keep ExplorBot.setUserResolve and App.tsx). Collapse the impossible `!this.config` guards in getConfig()/start() (config is a required ctor param). Drop unused UserResolveFunction/outputPath imports. Also strip the now-dead 3rd attempt() arg from boat/doc-collector call sites (it imports Explorer from src). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: dead-code sweep step 4 (state-manager) Delete uncalled getCurrentContext() and getPreviousState(), and loadHtmlFromFile() (a byte-for-byte copy of ActionResult.loadHtmlFromFile). Drop the now-unused tag and outputPath imports and an orphaned JSDoc block. (The knowledge scanner is left untouched — plan 010 removes it and is not yet in main; see report.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: dead-code sweep step 5 (explorbot, navigator) explorbot.ts: delete isReady(), needsInput field, getConfigParser(), the unused agentEmoji local, and the dead captain.setQuartermaster/setHistorian + rerunner.setQuartermaster wiring (captain consumes neither; rerunner consumes only getHistorian). Keep tester's wiring and rerunner.setHistorian. navigator.ts: delete the never-read experienceCompactor field/param/assignment/ import and drop the argument at the explorbot construction site; drop the unused WebPageState import. planner.ts: drop the unused getStyles import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: dead-code sweep step 6 (conversation, provider) Conversation: delete the write-only id field + generateId(), and the production-unused addUserImage() (the two unit tests that used it now push a non-string message directly, preserving their cleanupTag/hasTag coverage). Provider: make getProviderOptionsForAgent/getReasoningForAgent private (only internal callers), and move CONTEXT_LENGTH_PATTERNS to a module-level const. Skipped (entangled with the still-open plan-007 PR #75, which rewrites clone()/ invokeConversation): Conversation.clone() and Provider.lastConversation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: DavertMik <davert@testomat.io> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Implements plan 007 (
plans/007-unify-tool-success-and-telemetry.md) — a P1 bug fix.Problem 1 — two classifiers disagree
Provider.invokeConversationpaired tool calls with results by array position and defaulted a missingsuccessfield to failed.Conversation.getToolExecutionspaired bytoolCallId, unwrapped the AI-SDK{type:json,value}envelope, and defaulted to passed. Tester consumes the former, Pilot's session log the latter — so Pilot could review a step as "succeeded" that Tester counted as failed. Positional pairing also mis-attributes outputs if the SDK returns results out of order.Fix: one exported
toToolExecution(toolName, input, rawOutput)owns the unwrap +success !== falsedefault; both sites route through it.invokeConversationnow pairs bytoolCallIdvia aMap. Data-shaped tools with nosuccessfield now count as successful in both views (matching what Pilot already trusted — action/assertion tools always return explicitsuccessviasuccessToolResult/failedToolResult, so stuck-detection is unaffected).Problem 2 — dead
telemetryFunctionIdoptionTen call sites passed
telemetryFunctionIdexpecting a named Langfuse trace, butgetTelemetryonly readoptions.telemetry. Normalized the shorthand into{ functionId }(explicittelemetry.functionIdstill wins). Deleted the never-readConversation.telemetryFunctionIdfield/param/clone-arg.Verification
toToolExecutionunit tests (unwrap, default-true, explicit-false, missing output) +getTelemetrytests (shorthand honored, explicit wins).bun test tests/unit→ 734 pass, 0 fail;bun run lintclean.bun test tests/integration/planner.test.ts(exercises the tester tool loop throughinvokeConversation) → 10 pass, 0 fail.🤖 Generated with Claude Code