fix(agent): Claude alias resolution + skill/error tracing + error sanitize (QA F-031/F-029/F-030) - #4855
Conversation
…itize (QA F-031/F-029/F-030) F-031: a bare Claude alias (haiku/sonnet/opus + [1m]) or a dated claude-* id now resolves to the anthropic provider in the SDK connection resolver instead of failing F-017's MissingProviderError, and the missing-provider hint names the harness-reachable provider (anthropic/<m> on Claude, openai/<m> elsewhere). The alias set is reused from capabilities.py; the harness is threaded from RuntimeAuthContext. F-029: the agent span now carries ag.agent.skills.loaded / ag.agent.skills.count with the materialized skill names (author + forced _agenta.* platform skills) on every harness — the sandbox-agent otel stamps it for Claude/Daytona, and local Pi gets it on Pi's own span via the new AGENTA_SKILLS_LOADED env. F-030: a new recordError(message, provider) on the sandbox-agent otel stamps ag.error.message, ag.error.provider, an exception event, and ERROR status on the agent span before flush; local Pi (span-less in the runner) emits a standalone agent_error span so the diagnostic still reaches the /invoke trace. Error sanitize: result_from_wire and the ts_runner transport errors route through sanitize_runner_error / _transport_error — one clean user-facing line, stack/path leak stripped, length-capped; the full detail is logged, never shown. Tests: SDK connection + wire-contract unit tests (alias, harness-correct hint, sanitizer); runner otel + pi-assets + orchestration unit tests. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates missing-provider handling for Claude models, sanitizes Python transport and wire error messages, and threads skills metadata plus run-error recording through the sandbox agent tracing stack. ChangesPython connection resolution
Python runner error sanitization
Agent tracing stack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
|
Review needed on three specific judgment calls (rest is mechanical):
Live trace re-verify (skills + error attributes actually landing on a real run) is the orchestrator's; this PR is implement + unit-test only. Do NOT merge. |
Railway Preview Environment
Updated at 2026-06-25T20:23:14.960Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
sdks/python/agenta/sdk/agents/platform/connections.py (1)
61-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the resolver contract docs for the Claude exception.
ModelRefstill documents “There is no model-id->provider table,” andRuntimeAuthContextstill says the vault resolver is harness-agnostic. These lines intentionally add a Claude alias/id provider table and pass harness into resolution, so please update those docs to describe this narrow exception.Also applies to: 403-411
services/agent/src/tracing/otel.ts (1)
1169-1203: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
recordErroris only idempotent on the owned-span path.The doc comment states it is idempotent, but in the span-less branch each call to
recordErrordoestracer.startSpan("agent_error", ...), so a second invocation emits a duplicateagent_errorspan. This can happen ifrun.finish()/run.flush()throws after the swallowed-error path already calledrecordError(thecatchinrunSandboxAgentthen calls it again). Consider a latch (e.g. anerrorRecordedflag) so the standalone branch is one-shot.services/agent/tests/unit/sandbox-agent-orchestration.test.ts (1)
125-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFake
output()mirrorsfinish(), so the swallowed-error guard divergence can't be covered here.Both
output()andfinish()returnoptions.output. The realoutput()(rawaccumulated) andfinish()(banner-stripped) can diverge — the exact gap noted onsandbox_agent.tsLine 425. Consider letting the fake return distinct values (e.g.options.rawOutputforoutput()) so a test can assert swallowed-error detection still fires when only a banner streamed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a39f81bf-08e9-4640-9727-5f9d90f1cb86
📒 Files selected for processing (13)
sdks/python/agenta/sdk/agents/connections/errors.pysdks/python/agenta/sdk/agents/platform/connections.pysdks/python/agenta/sdk/agents/utils/ts_runner.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/oss/tests/pytest/unit/agents/platform/test_connections_http.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/agent/src/engines/sandbox_agent.tsservices/agent/src/engines/sandbox_agent/pi-assets.tsservices/agent/src/extensions/agenta.tsservices/agent/src/tracing/otel.tsservices/agent/tests/unit/otel-skills-error.test.tsservices/agent/tests/unit/sandbox-agent-orchestration.test.tsservices/agent/tests/unit/sandbox-agent-pi-assets.test.ts
| if response.status_code >= 400: | ||
| body = await response.aread() | ||
| raise RuntimeError( | ||
| f"Agent runner HTTP {response.status_code}: {body[:1000]!r}" | ||
| raise _transport_error( | ||
| f"Agent runner HTTP {response.status_code}", | ||
| detail=repr(body[:1000]), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('sdks/python/agenta/sdk/agents/utils/ts_runner.py')
for start, end in [(1, 240)]:
print(f'লines {start}-{end}')
with path.open() as f:
for i, line in enumerate(f, 1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: Agenta-AI/agenta
Length of output: 9205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('sdks/python/agenta/sdk/agents/utils/ts_runner.py')
for start, end in [(1, 240)]:
print(f"lines {start}-{end}")
with path.open() as f:
for i, line in enumerate(f, 1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: Agenta-AI/agenta
Length of output: 9205
Avoid buffering the full streamed error body. await response.aread() still loads the whole error payload into memory even though only the first 1000 bytes are logged; read a bounded prefix instead and let the response close.
| message = raw.split("\n", 1)[0].strip() | ||
| # If even the first line is a raw stack frame, fall back to a generic line. | ||
| if not message or _STACK_FRAME_RE.match(message): | ||
| return "agent run failed" | ||
| if len(message) > _ERROR_MAX_LEN: | ||
| message = message[: _ERROR_MAX_LEN - 1].rstrip() + "…" | ||
| return message |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Single-line path leaks still pass through.
This only falls back when the first retained line starts with a stack-frame pattern. Errors like ENOENT: ... '/tmp/session/.claude/settings.json' or Cannot find module /app/... stay intact and will still be surfaced by result_from_wire(), which misses the PR’s “no stack/path leak” contract. Please redact path-like substrings in the kept line, or fall back whenever the retained line still contains one, and add a regression test for that case.
Also applies to: 132-135
| const swallowedPiError = | ||
| plan.isPi && | ||
| !plan.isDaytona && | ||
| !output.trim() && | ||
| !run.output().trim() && | ||
| !run.events().some((e) => e.type === "tool_call") | ||
| ) { | ||
| const piError = findSwallowedPiError(plan.sourcePiAgentDir, plan.cwd); | ||
| if (piError) { | ||
| return { | ||
| ok: false, | ||
| error: conciseError( | ||
| new Error(piError), | ||
| plan.harness, | ||
| request.provider, | ||
| ), | ||
| }; | ||
| } | ||
| ? findSwallowedPiError(plan.sourcePiAgentDir, plan.cwd) | ||
| : undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'output\s*:\s*\(\)\s*=>\s*accumulated' services/agent/src/tracing/otel.ts
rg -nP -C3 'stripStartupBanner|splitLeadingBanner' services/agent/src/tracing/otel.ts
# Find where the startup banner originates and whether it is appended regardless of sink
rg -nP -C3 'banner' services/agent/src/tracing/otel.tsRepository: Agenta-AI/agenta
Length of output: 6670
🏁 Script executed:
sed -n '1180,1285p' services/agent/src/tracing/otel.ts
sed -n '410,440p' services/agent/src/engines/sandbox_agent.tsRepository: Agenta-AI/agenta
Length of output: 4691
🏁 Script executed:
sed -n '1180,1285p' services/agent/src/tracing/otel.ts
sed -n '410,440p' services/agent/src/engines/sandbox_agent.tsRepository: Agenta-AI/agenta
Length of output: 4691
Compare against banner-stripped output here run.output().trim() still includes pi-acp’s startup banner, while finish() strips that banner before treating the turn as empty. A banner-only failed local-Pi run can skip findSwallowedPiError(...) and still return an empty ok:true turn. Use the same banner-stripped check as finish().
| } catch (err) { | ||
| const error = conciseError(err, plan.harness, request.provider); | ||
| // Stamp the error message + provider on the agent span before finishing it (F-030), so a | ||
| // trace carries the same diagnostic the response does (it previously held only a count). | ||
| otel?.recordError(error, request.provider); | ||
| otel?.finish(); | ||
| await otel?.flush().catch(() => {}); | ||
| return { | ||
| ok: false, | ||
| error: conciseError(err, plan.harness, request.provider), | ||
| error, | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
otel?.finish() is unguarded in the catch path.
otel?.flush() swallows rejections via .catch(() => {}), but otel?.finish() can throw and would escape the catch block, replacing the sanitized error with a raw thrown exception (potentially leaking stack/path detail the sanitization step removed). Wrap it defensively.
🛡️ Proposed guard
- otel?.recordError(error, request.provider);
- otel?.finish();
- await otel?.flush().catch(() => {});
+ otel?.recordError(error, request.provider);
+ try {
+ otel?.finish();
+ } catch {
+ // tracing teardown must not mask the sanitized error
+ }
+ await otel?.flush().catch(() => {});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| const error = conciseError(err, plan.harness, request.provider); | |
| // Stamp the error message + provider on the agent span before finishing it (F-030), so a | |
| // trace carries the same diagnostic the response does (it previously held only a count). | |
| otel?.recordError(error, request.provider); | |
| otel?.finish(); | |
| await otel?.flush().catch(() => {}); | |
| return { | |
| ok: false, | |
| error: conciseError(err, plan.harness, request.provider), | |
| error, | |
| }; | |
| } catch (err) { | |
| const error = conciseError(err, plan.harness, request.provider); | |
| // Stamp the error message + provider on the agent span before finishing it (F-030), so a | |
| // trace carries the same diagnostic the response does (it previously held only a count). | |
| otel?.recordError(error, request.provider); | |
| try { | |
| otel?.finish(); | |
| } catch { | |
| // tracing teardown must not mask the sanitized error | |
| } | |
| await otel?.flush().catch(() => {}); | |
| return { | |
| ok: false, | |
| error, | |
| }; |
1 participant
Why
Three QA findings from the final agent-workflows sweep, all in the SDK connection resolver and the runner tracing/error boundary. Disjoint from the HITL (#4854) and MCP (#4853) lanes.
F-031 — the F-017 fix (#4846) broke the documented Claude alias. A
claudeharness run with the documentedmodel: "haiku"returned500 model 'haiku' needs a provider prefix (e.g. 'openai/haiku')and never reached the harness. The alias is the documented Claude form, and the hint suggestedopenai/on an Anthropic-only harness.F-029 — skills were invisible in traces. The trace echoed the author skill config but had no "skill loaded" signal and never showed the forced
_agenta.*platform skills the harness injects.F-030 — error runs traced only a count. An errored run carried
errors.cumulative=1and nothing else: no message, no provider, no exception event. The actionable text lived only in the HTTP body.Error leak (Codex MEDIUM). The runner
errorstring was rethrown verbatim into the user-facing workflow error, so a stack frame / internal path / HTTP-body dump could reach the UI.What
platform/connections.py,connections/errors.py): a bare Claude alias (haiku/sonnet/opus+[1m], reused fromcapabilities.pyCLAUDE_MODEL_ALIASES) or a datedclaude-*id now resolves to theanthropicprovider before the F-017 fail-loud check, so it reaches auth exactly likeanthropic/haiku. The missing-provider hint is harness-correct (anthropic/<m>on Claude,openai/<m>elsewhere), derived from the capability table. Inference only fills a missing provider; an explicit one is always honored.tracing/otel.ts,extensions/agenta.ts,pi-assets.ts,sandbox_agent.ts): the agent span carriesag.agent.skills.loaded+ag.agent.skills.countwith the materialized names (author + forced_agenta.*). Claude/Daytona get it from the runner's tracer; local Pi gets it on Pi's own span via a newAGENTA_SKILLS_LOADEDenv.tracing/otel.ts,sandbox_agent.ts): a newrecordError(message, provider)stampsag.error.message,ag.error.provider, an OTel exception event, and ERROR status on the agent span before flush — on both the catch path and the swallowed-Pi-error path. Local Pi (span-less in the runner) emits a standaloneagent_errorspan so the diagnostic still reaches the/invoketrace.utils/wire.py,utils/ts_runner.py):result_from_wireand the transport errors route throughsanitize_runner_error/_transport_error— one clean line, stack-frame/path leak stripped, capped to 300 chars; the full detail is logged, never shown. The runner's existingconciseErroris untouched (a clean message passes through unchanged).No
/runwire fields changed (skills ride an internal env, not the wire); the golden contract is unchanged.Before / after
model: "haiku"500 needs a provider prefix (e.g. 'openai/haiku')anthropic, reaches authopenai/<m>anthropic/<m>ag.agent.skills.loaded(incl. forced_agenta.*)errors.cumulative=1onlyag.error.message+ag.error.provider+ exception eventTests
test_connections_http.py— bare alias resolves to anthropic, datedclaude-*resolves, harness-correct hint (Claude vs Pi).test_wire_contract.py—sanitize_runner_error(clean passthrough, multiline strip, stack-frame fallback, length cap, none/empty) + a leaky-error round-trip.otel-skills-error.test.ts(skills stamped on both tracers, omitted when none, error stamped on the owned span, standaloneagent_errorspan for self-instrumenting Pi, provider fallback).sandbox-agent-pi-assets.test.ts—AGENTA_SKILLS_LOADEDcarried under tracing, omitted otherwise.sandbox-agent-orchestration.test.ts— fake otel gainsrecordError/output.tscclean; ruff clean.Decisions logged in
qa/final-sweep-decisions.md(D-009..D-012). Live trace re-verify is the orchestrator's.https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc