[fix] Land the v7 audit findings that need no product decision by junaway · Pull Request #5286 · Agenta-AI/agenta

junaway · 2026-07-13T18:52:44Z

Context

The v7 audit of big-agents left a ledger of confirmed defects. This lands every one that needed no product decision from a reviewer: 16 findings across the runner, the SDK, the API, and the Helm chart. The open questions are listed at the bottom and are deliberately not touched here.

Three of these are the kind that only show up in production. The Helm runner was the one deployment template missing commonEnv, so a stock chart install brought up a runner that could not reach the API. A retried cron tick re-ran a real workflow, so a transient failure after a Slack post or a PR open did it again. And a migration's downgrade() deleted flag keys from rows it never wrote.

Changes

Security

The runner handed every harness every provider key. buildDaemonEnv copied all of KNOWN_PROVIDER_ENV_VARS into the harness process, so a run on Claude also received the OpenAI, Gemini and Mistral keys. Now it inherits only the keys its declared provider needs, narrowing by default (AGENTA_RUNNER_INHERIT_ALL_PROVIDER_KEYS opts out). Claude-on-Bedrock still gets AWS_*, because that belongs to provider=anthropic + deployment=bedrock. A run that declares no provider keeps today's full inheritance, so an un-migrated caller cannot break silently.

/kill tore down every tenant's work. It took no session argument and drained every keep-alive pool and every in-flight sandbox on the box. It now requires a sessionId and 400s an unscoped call. The unscoped teardown survives as an in-process function on the SIGTERM path, which is its only real caller: the endpoint had zero HTTP callers (the web Kill button goes to DELETE /sessions/streams/{id}, and the orphan sweeper only touches Redis and the DB).

AGENTA_RUNNER_TOKEN was undiscoverable. The runner's only authentication appeared in no compose file, no Helm template and no .env.example. An operator had to read the sidecar's TypeScript to learn it existed. It is now declared in all 7 compose files, the Helm chart and the env examples, and documented in the self-host guide. Two details mattered: the runner uses an explicit environment: allowlist rather than env_file:, so an env-example-only entry would never have reached the container; and the SDK reads the same var on the caller side, so wiring only the runner would have locked the Services API out with 401s.

Redaction was built and never wired. redaction.ts was a complete Redactor with zero importers, while the transcript sink and the span sink both persisted raw text. Both are now wired. The trap here was the seeding: seedFromEnv reads as "seed from process env" but its signature takes resolvedSecrets and runCredential, and nobody ever passed them. Per-run provider keys arrive on the wire and are never in process.env, so a bare seedFromEnv() would have caught the sidecar's own keys and missed every key that matters. Seeding is now per-run.

Secrets and user content get opposite treatment, deliberately. The known-value pass only matches strings the caller handed us as live credentials, so it has no false positives by construction and cannot mangle conversation text. A pasted sk--shaped string that is not a live secret passes through untouched; the real key in the same transcript is scrubbed. captureContent keeps its default.

Tool errors leaked upstream bodies into the model's context. callback.ts interpolated bodyText.slice(0, 500) into the error it threw back to the model, while its sibling direct.ts deliberately kept the body server-side and returned only a status code. callback.ts now matches. The usability objection turned out not to apply: the gateway returns business failures (a malformed argument, a missing field) as HTTP 200 with STATUS_CODE_ERROR, which travels the success path and always reached the model. The non-2xx throw only ever carried infrastructure faults the model cannot fix by rewriting an argument. That signal is now surfaced explicitly from the 200 envelope rather than arriving by routing accident, with a test pinning it.

Also: mount.ts interpolated cwd and geesefs args into sh -c unquoted while its sibling defined a shellQuote() one file away (four sites, including one splicing bucket:prefix that the finding never named); tool results came back uncapped; skill sizes were enforced only by the SDK's pydantic models and not at the runner's wire; and the Python SDK guarded custom_provider.url but not the MCP server url.

The approval card could misrepresent a tool

resolvedSpecOf probed four fields — spec, toolSpec, resolvedTool, tool — that no production code ever sets. For real traffic it always returned undefined, so the human approval card never saw a tool's true permission and fell back to the plan default. A write tool could be presented as read-only, which trains people to rubber-stamp.

Enforcement was never affected (two downstream gates re-derive the verdict from the real spec), so this went unnoticed. The fix resolves the spec from the same source relay-guard.ts uses. There were already three separate new Map(specs.map(s => [s.name, s])) sites; rather than adding a fourth, they now share one toolSpecsByName(). The card and the guard cannot drift apart again, which is how this happened.

The old tests passed by setting toolCall.spec inline — fabricating the very field production never writes. They were corrected to real harness shape, and the new tests were verified to fail against the pre-fix code.

Correctness

A migration's downgrade() destroyed data it never wrote. oss000000010 stripped is_agent/is_skill from any row carrying an explicit false, not just the rows it backfilled. The root cause was upstream: _dump_stored_flags used exclude_none=True but not exclude_unset=True (unlike its sibling), so any revision setting is_chat materialized is_agent: false into stored JSON and landed in the downgrade's WHERE.

Both are fixed. upgrade() now records which keys it added and downgrade() strips only those. Verified against a real Postgres 14: the old downgrade also wiped {"is_agent": false, "is_skill": true} to {} on untouched rows, which is worse than the finding described.

A retried cron tick re-ran the workflow. dispatch_schedule had no dedup gate (its docstring said so plainly) while the worker registered it with retry_on_error=True. Its sibling dispatch_subscription already had the gate, and dedup_seen_schedule already existed in the DAO — the dispatcher simply never called it. It does now.

The zero-content backstop could not fire. The guard against a blank chat bubble counted parts before _conform dropped them, so a run whose only content was a malformed file part counted as one part, stayed silent, and rendered the blank bubble it existed to prevent. The counter now counts what is actually yielded. The bug was in both stream projections, not just the one the finding cited, and tool_result incremented unconditionally even when it yielded nothing.

The client killed runs the runner considered healthy. The SDK's idle timeout was 180s; the runner's is 300s. In that 120s window the runner deliberately treats as alive, the client had already torn down the transport, so the clean terminal record never reached the caller. The SDK default is now 360s: the authority always trips first.

Also: UnsupportedDeploymentError is documented as 422 but had no status_code and fell through to 500; ResolvedConnection.env was masked from repr but not model_dump(); and WireToolCallback.authorization lacked repr=False.

Tests

Runner: 1107 passing, typecheck clean (was 1072 on base).
SDK unit: 1808 passing, 10 pre-existing xfails (OpenRouter catalog lag).
API unit: 1335 passing.
Acceptance suites need a live stack and were not run here.
helm lint clean; helm template verified to render the token into the runner, the services deployment and the secret when set, and nothing when unset.

Two findings were closed with no code change, because verification did not support them. F-PD-3 was already fixed on big-agents. F-CONFINE-2 (the dev image runs as root while Dockerfile.gh drops to USER node) turned out to be an ownership constraint, not the FUSE constraint everyone assumed — geesefs mounts fine as uid 1000 because fusermount is setuid-root. The real blockers are mkdir -p /pi-agent and a build step writing to root-owned /app. Forcing USER node would break dev to close a P3, so the reason is recorded instead.

What to QA

Start a chat and click a tool-approval card for a tool that writes. The card must show its real permission, not a permissive default.
Run an agent turn with a slow tool call (>3 min quiet). It should complete rather than dying at 3 minutes with a client timeout.
Regression: a tool call that fails validation (a missing required field) must still show the model a useful message so it can retry. A tool call that fails with a 5xx must show only the status code.
Regression: stock helm install — the runner pod should now reach the API.

mount.ts interpolated cwd, geesefs args, and the log-file path unquoted into `sh -c` strings (remoteMountAlive, unmountRemoteDeadMount, mountStorageRemote). Not exploitable today since every value traces to a UUID/hex-derived prefix, but a future caller threading a user-nameable value into cwd would reintroduce shell injection with nothing to catch it. Reuse the shellQuote() helper agent-mount.ts already defines for the identical problem, rather than hand-rolling a second implementation: moved it into mount.ts (the module agent-mount.ts already imports types from) and exported it, so both files share one escaping function. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

MCPResolver.resolve now runs the http-transport server url through assert_endpoint_url_allowed, mirroring the existing custom_provider guard. Raises MCPServerURLBlockedError on a blocked target instead of silently passing it through. Not a live SSRF today (the runner independently guards via validateUserMcpUrl at the actual network boundary), but this closes the defense-in-depth gap for any future direct consumer of ResolvedMCPServer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The dtos.py half of this finding was already fixed; wire_models.py's copy still lacked repr=False, so a model dump or exception repr could surface the gateway-tool credential. Mirrors the existing dtos.py fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

callAgentaTool read the full /tools/call response body with no size bound and handed the resolved content back to the model verbatim; only the error string (500 chars) and the debug log (300 chars) were truncated. transcript.ts caps rendering at 4000 chars, but that only protects the replay window — nothing protected ingress, so an oversized upstream body (a discover_tools dump can be 30-60 KB) was still fully materialized and passed to the harness. relay.ts and the in-sandbox relay-client.ts reader consume the same value, so capping at the source covers both transports. Reuse tool-mcp-http.ts's existing MAX_BODY_BYTES (1MB) mechanism rather than inventing a new cap: moved the constant into callback.ts (tool-mcp-http.ts already imports EMPTY_OBJECT_SCHEMA from there, so this avoids a cycle) and added capToolResultText(), which truncates and appends a "[... N bytes omitted]" marker in the same shape transcript.ts already uses, so the model can tell a result was cut. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… drops to USER node The USER asymmetry between Dockerfile.gh and Dockerfile.dev was unexplained. It is not a FUSE constraint: geesefs mounts fine as uid 1000 (fusermount is setuid-root and /etc/fuse.conf sets user_allow_other). The dev image needs root for two ownership reasons instead — it rebuilds dist/ under root-owned /app on every container start, and the dev compose command mkdir's the root-level /pi-agent. Record the reason rather than force USER node and break dev. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…boundary Skill size caps were enforced ONLY by the SDK's pydantic models. WireSkill in protocol.ts carries no runtime caps and resolveSkillDirs wrote whatever it received, so a non-SDK client could POST an arbitrarily large body or bundled file straight to disk. The runner already re-validates skill NAMES for exactly this reason ("the wire is an untrusted boundary (a non-SDK client can POST anything)"); the identical reasoning applies to size and was not applied. Mirror the SDK's caps verbatim from sdks/python/agenta/sdk/agents/skills/models.py so the two sides cannot drift: description <= 1024, body <= 50_000, file path <= 255, file content <= 200_000 (name <= 64 was already enforced). Rejection reuses the existing skip-with-log shape (an over-cap skill or file is skipped, the run continues) rather than introducing a new failure mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

env was masked from repr but not from model_dump()/model_dump_json(), and the docstring admitted the invariant rested on caller discipline ("Never log a raw dump"). A field_serializer now masks the values on every dump, so the leak is impossible by construction rather than by convention. Kept env as Dict[str, str] rather than switching to SecretStr: SecretStr is a breaking type change on a public SDK model (every producer and the 15 tests asserting plain-string equality would have to unwrap), and it buys nothing extra here — the serializer closes the same gap on both dump paths while attribute access stays plain for the one legitimate consumer (handler.py, which hands env to the harness via the `secrets` wire field). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

qa/matrix.md documents UnsupportedDeployment as HTTP 422 (and calls any other status a finding), but the class carried no status_code, so the invoke remap in decorators/routing.py fell through to 500. GatewayToolResolutionError is deliberately NOT changed: its `.status` is the UPSTREAM status of the failed /tools/resolve call, not a status this SDK should return to its own caller. Renaming it to `status_code` would make the remap echo a downstream code (and collide with the existing 424 downstream-error remap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…enant `POST /kill` took no arguments and drained every keep-alive pool plus every in-flight sandbox on the process — across all tenants — behind an auth gate that is off by default (`AGENTA_RUNNER_TOKEN` unset). Any caller that could reach the sidecar could kill every other project's live session. The route now requires a `sessionId` (with an optional `projectId`) in the body and tears down only that session: `pool.destroy(<projectId>:<sessionId>)` on each provider pool, keyed exactly as `poolKeyFor` keys it, plus the new `destroyInFlightSandboxesForSession`. An unscoped call is rejected with 400 rather than silently read as "kill everything". The process-wide drain stays where it belongs: the SIGTERM/SIGINT shutdown handler still calls `destroyAll` / `destroyInFlightSandboxes` in-process. Only the HTTP surface is scoped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…every key `buildDaemonEnv` copied the WHOLE `KNOWN_PROVIDER_ENV_VARS` inventory into the harness process on any run that was not credential-mode `env` — and that permissive branch is the default. A run that declared an OpenAI model was handed the sidecar's Anthropic, Gemini, Groq, Bedrock and Vertex credentials too. Narrow by default: the inherited set is now derived from the run's resolved `provider` + `deployment` (both already on the wire) via `inheritableProviderEnvVars`. Provider families and their `*_API_KEY` names mirror the canonical Python map (`agenta.sdk.agents.capabilities.PROVIDER_ENV_VARS`); the deployment groups mirror the SDK's `resolved_env` channels (bedrock -> `AWS_BEARER_TOKEN_BEDROCK`, azure -> `AZURE_OPENAI_API_KEY`). This is least privilege WITHIN a run, not a deployment-posture knob — passing the Anthropic key to an OpenAI run never made zero-config self-hosting work, it only widened the blast radius. Zero-config still works: a run that declares no provider (un-migrated caller) keeps today's full inheritance, and `AGENTA_RUNNER_INHERIT_ALL_PROVIDER_KEYS` is the explicit opt-out for an operator whose harness resolves a model outside its declared provider. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… context Two sibling transports disagreed on what a failed tool call discloses. direct.ts is deliberate and explicit — it logs the upstream body server-side and returns only the status code ("the model gets only the status code"). callback.ts — the path every gateway/Composio tool takes — did the opposite, interpolating bodyText.slice(0, 500) straight into the error thrown back into the model's context. No credential leaks (auth is attached server-side and never echoed), but internal error detail, upstream vendor messages, and potentially PII from a connected account landed in a context a prompt-injected attacker can exfiltrate via a later tool call. Match direct.ts: console.error the body server-side, throw only `tool call ${callRef} failed: HTTP ${status}`. This costs the model nothing it can act on. The gateway returns a CORRECTABLE tool failure as HTTP 200 with status.code = STATUS_CODE_ERROR and the upstream message in status.message (api/oss/src/apis/fastapi/tools/router.py call_tool: "unsuccessful tool execution responses remain business-level errors -> 200"), which travels the success path. A non-2xx is an infrastructure/config fault (slug 400s, connection 404s, auth 424s, 5xxs) the model cannot fix by rewriting an argument. That self-correction signal previously worked only by accident of routing, so surface status.message EXPLICITLY on the 200/STATUS_CODE_ERROR envelope. A future change to that route can no longer silently take the model's ability to self-correct away — the test is the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

`resolvedSpecOf` probed the ACP tool-call for its spec across four candidate fields (`spec ?? toolSpec ?? resolvedTool ?? tool`). No production code sets any of them — only test fixtures did — so for real harness traffic it always returned undefined and the approval gate never saw the tool's true `permission`/`readOnly`, silently falling back to the plan default. A write tool could be presented to a human as read-only, which trains the reviewer to rubber-stamp the card. This is a DISPLAY/classification fix, not an authorization fix. Enforcement was already correct and is untouched: Claude's SDK settings are pre-rendered server-side from the real spec, and `buildRelayExecutionGuard` re-derives the verdict from the real spec before execution regardless of what the card showed. The real spec lives in `plan.toolSpecs`, keyed by name — the same source the relay execute loop hands to the relay execution guard. Rather than hand-roll a second lookup (the drift that caused this bug), factor the name->spec index into a shared `toolSpecsByName()` in `tools/public-spec.ts` and route every consumer through it: the relay execute loop (-> relay-guard), the internal tool MCP server, and now the ACP gate. The card and the guard can no longer disagree. The ACP gate resolves the spec by the gate's bare tool name, reusing the existing `bareToolName()` helper (a Claude MCP title arrives as `mcp__agenta-tools__<tool>`). The dead four-field probe is deleted. Tests: the existing fixtures set `toolCall.spec` inline — a field production never writes — which is precisely what masked this. Corrected them to real harness shape (spec resolved by name from the run's specs) and added regression tests proving a spec-less, realistically-shaped tool call now carries its true `permission`/`readOnly` onto the card, including that a mutating tool is never shown as read-only under a permissive plan default. Both new tests fail against the pre-fix behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…inks `services/runner/src/redaction.ts` shipped complete but with zero importers, so the two sinks it exists to protect ran wide open: the persisted transcript and the exported OTel spans both wrote the run's resolved provider keys verbatim. Implements Slice 1 of docs/designs/online-redaction (WP1.1 seeding, WP1.4 records, WP1.5 spans) on the runner side; the SDK side was already wired. - `seedForRun` builds the per-run deny-set from the request's resolved `secrets` + the run credential (+ the mount STS pair at the engine). The run's provider keys are injected per run and are NEVER in the sidecar's process env, so seeding from `seedFromEnv()` alone would have missed exactly the secrets that matter. - persist: redact in `persistEvent`, the single choke point every durable record passes through. The live stream and the harness's in-memory conversation state are untouched — only the stored copy is scrubbed. - spans: redact span attributes in `TraceBatchProcessor.flush`, the last point before they leave the process, so no span-building call site can forget to. `captureContent` keeps its default (on). The fix is to scrub known secrets from captured content, not to stop capturing it: blanket-redacting user content would break trace viewing, and the known-value pass has zero false positives by construction. No default changes for a zero-config self-hoster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The dev-only twin (agent_run_to_vercel_parts) returned straight out of its `except`, ending the stream on the error part with no finish-step/finish. The live twin (agent_stream_to_vercel_stream) guarantees both from a `finally:`. The live twin is normative, so the dev one now drains the same way — a consumer waiting on the finish frame no longer hangs. test_ui_messages.py's test_terminal_failure_emits_error_part_and_no_finish asserted the OLD behavior. Verified against the live twin first: on the same ok:false terminal failure it emits [... error, finish-step, finish]. The "no finish frame" was the divergence SDK-3 reports, not a contract, so the test is updated to the normative shape and renamed. Edit deliberately confined to the terminator logic (concurrent SDK-1 work is in the same file, on the content_parts_emitted counter / _conform). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The SDK's AGENTA_RUNNER_TIMEOUT_SECONDS is an IDLE bound (gap between records), the same semantics as the runner's DEFAULT_IDLE_TIMEOUT_MS (300s) — but it defaulted to 180s, 120s narrower. In the 180-300s window the runner still considers healthy, the client had already torn down the transport, so the runner's structured terminal record (the point of the RUN-33 fix) never reached the caller. The runner is the authority, so move the client: default to 360s, strictly wider than the runner's 300s, so the authority always trips first and no tie can race. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The zero-content backstop ("The agent produced no output.") counted inside the impl generators, but _conform filters OUTSIDE them in the wrapper — dropping a file part missing url/mediaType, and tool parts missing toolCallId. A run whose only content was a malformed part therefore counted as content_parts_emitted == 1, the backstop stayed silent, and the user got exactly the blank bubble it exists to prevent. Count what is actually yielded: consult _conform before incrementing, at each droppable site, in both projections (the live routing-layer stream and the dev-only AgentStream twin). _conform is idempotent, so the wrapper's second call is a no-op on the same part. Also drops an unconditional increment on tool_result that counted even when the orphaned-result branch yielded nothing at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… reversible Two halves of one defect. Root cause: _dump_stored_flags (the persisting helper) used exclude_none=True but not exclude_unset=True, unlike its sibling _dump_flags. Any revision that set is_chat/is_llm/is_managed therefore materialized is_agent: false, is_skill: false into stored JSON — a flag the caller never mentioned, indistinguishable from one they declared. Consequence: oss000000010's downgrade stripped is_agent/is_skill from every row carrying an explicit false, which — thanks to the above — included ordinary rows the backfill never wrote. Verified against Postgres: the old downgrade also wiped {"is_agent": false, "is_skill": true} to {} on a row the backfill never touched. Fixes: - _dump_stored_flags now excludes unset flags. An explicit false a caller DID send is still "set", so it still persists; the is_static anti-forgery scrub is likewise unaffected (a supplied is_static is set, so it still reaches the scrubber and is hard-coded to False). - oss000000010 now records, per row, which keys the backfill itself added (under a private __oss000000010__ marker) and the downgrade strips only those, then drops the marker. Reversible by construction rather than by a predicate that cannot tell the two populations apart. Round-trip verified against Postgres 14. Two existing tests asserted the phantom-false output; they pinned the defect and are updated to the correct behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…tifact `AGENTA_RUNNER_TOKEN` is the runner's only authentication — unset means no check at all (server.ts:129-133) — yet it appeared in zero compose files, zero Helm templates/values, and zero .env examples. An operator could not discover it, let alone turn it on. Declared everywhere its sibling knobs are: - The 7 compose files that define a runner service. The runner uses an explicit `environment:` allowlist (not `env_file:`), so the var had to be added there or it would never reach the container; the `services` container reads it from the env file it already loads. - The 4 `env.*.example` files, as bare commented keys (house style: no inline prose in env examples). - Helm: `agentRunner.token` -> the release Secret, `secretKeyRef` into BOTH the runner deployment (which enforces the gate) and the services deployment (the caller that must present it — set it on only one side and the API locks itself out). Plus values.yaml and values.schema.json. `helm lint` passes and `helm template` shows all three renders; unset renders no env at all (gate off). - The self-host configuration docs: table row + an admonition saying it is off by default, what off means, and that both sides need the same value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…side effects triggers.dispatch_schedule is registered with retry_on_error=True and max_retries=TRIGGER_MAX_RETRIES, but dispatch_schedule's only gate was is_active — it then called _run unconditionally. _run writes a delivery and re-raises when the workflow invocation fails, so a transient failure AFTER the workflow's side effects had landed retried the whole dispatch and re-invoked the workflow: re-posting to Slack, re-opening a PR, re-sending mail, up to max_retries times. Give it the same event_id-keyed gate its sibling dispatch_subscription already has, using the existing (previously uncalled-from-here) dedup_seen_schedule DAO method — the schedule-keyed twin of dedup_seen. Same mechanism, same log line, no second scheme. The pre-existing dedup_seen_schedule call in the cron refresh loop is an ENQUEUE-time check; it cannot see a taskiq retry, which re-enters dispatch_schedule directly. The gate belongs here too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ployment runner-deployment.yaml was the only deployment template with zero occurrences of commonEnv; every sibling (services, api, cron, worker-streams, worker-queues, alembic-job) includes it. commonEnv supplies AGENTA_LICENSE, AGENTA_AUTH_KEY, AGENTA_CRYPT_KEY, POSTGRES_PASSWORD and the API wiring (AGENTA_API_URL), so a stock chart install brought up a runner that could not reach the API. Added at the same nindent as the siblings, first under env: so the runner-specific vars and the user env range still layer on top. helm lint and helm template pass; the rendered runner now carries the full commonEnv block. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

vercel · 2026-07-13T18:52:49Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agenta-documentation	Ready	Preview, Comment	Jul 14, 2026 2:46pm

Copilot