Repository navigation
Conversation
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
backend
python
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds scoped trace-ingest tokens, separates general and export credentials across the SDK and runner, refreshes OTLP credentials per turn, delivers Pi tool specifications through files, and adds OTLP export gating and diagnostics. ChangesTelemetry authentication and export
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The change refreshes telemetry credentials per turn and adds scoped export tokens, but invalid TTL configuration can still make those tokens unusable, malformed credentials may return the wrong error class, and warm sessions may retain exporter resources longer than intended; a failed credential-file re-preparation can also proceed unexpectedly. These bounded correctness and runtime risks should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant PermissionAPI
participant SDKAuthMiddleware
participant WorkflowTracing
participant RunnerRuntime
participant AgentaExtension
participant OTLPExporter
PermissionAPI->>PermissionAPI: mint trace-ingest telemetry credential
PermissionAPI-->>SDKAuthMiddleware: return credentials and telemetry_credentials
SDKAuthMiddleware->>WorkflowTracing: store separate credentials
WorkflowTracing->>RunnerRuntime: serialize authorization and exportAuthorization
RunnerRuntime->>RunnerRuntime: refresh OTLP auth file per turn
AgentaExtension->>AgentaExtension: reread export credential before tracing
AgentaExtension->>OTLPExporter: export spans with export authorization
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e36ef66b-5939-44af-b60b-13c15bcb346a
📒 Files selected for processing (23)
api/oss/src/apis/fastapi/access/router.pyapi/oss/src/middlewares/auth.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/middlewares/test_auth_secret_token.pysdks/python/agenta/sdk/agents/tracing.pysdks/python/agenta/sdk/contexts/tracing.pysdks/python/agenta/sdk/decorators/routing.pysdks/python/agenta/sdk/decorators/running.pysdks/python/agenta/sdk/middlewares/routing/auth.pysdks/python/oss/tests/pytest/unit/agents/test_tracing.pysdks/python/oss/tests/pytest/unit/test_auth_middleware_credentials.pyservices/runner/src/engines/sandbox_agent/pi-assets.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/environment/runtime-lifecycle.tsservices/runner/src/extensions/agenta.tsservices/runner/src/tracing/export-diagnostics.tsservices/runner/src/tracing/otel.tsservices/runner/tests/setup/hermetic-env.tsservices/runner/tests/unit/otel-export-diagnostics.test.tsservices/runner/tests/unit/otel-flush-export.test.tsservices/runner/tests/unit/otel-trace-target-attribution.test.tsservices/runner/tests/unit/otlp-auth-per-turn.test.tsservices/runner/tests/utils/otel-export.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.
| except DecodeError as exc: | ||
| raise UnauthorizedException() from exc | ||
| log.debug( | ||
| "[auth] secret token unauthorized", | ||
| path=request.url.path, | ||
| method=request.method, | ||
| reason="invalid_token", | ||
| ) | ||
|
|
||
| except ExpiredSignatureError as exc: | ||
| raise UnauthorizedException() from exc | ||
| raise UnauthorizedException(reason="invalid_token") from exc | ||
|
|
||
| except HTTPException as exc: | ||
| # Scope rejections (and any other deliberate 401/5xx) must reach the client | ||
| # with their status — never collapsed into a 500 by the catch-all below. | ||
| raise exc | ||
|
|
||
| except Exception as exc: # pylint: disable=bare-except | ||
| raise InternalServerErrorException() from exc |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'from jwt.*exceptions|InvalidTokenError|DecodeError|ExpiredSignatureError' \
api/oss/src/middlewares/auth.py
rg -n -C 4 'invalid_token|unsupported|algorithm|verify_secret_token' \
api/oss/tests/pytest/unit/middlewares/test_auth_secret_token.pyRepository: Agenta-AI/agenta
Length of output: 4390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- auth.py imports and verifier ---'
sed -n '1,25p;900,990p' api/oss/src/middlewares/auth.py
printf '%s\n' '--- secret-token tests ---'
sed -n '1,320p' api/oss/tests/pytest/unit/middlewares/test_auth_secret_token.py
printf '%s\n' '--- PyJWT dependency declarations ---'
rg -n -i 'pyjwt|jwt' api/oss pyproject.toml poetry.lock requirements*.txt 2>/dev/null || true
printf '%s\n' '--- exception definitions or vendored sources ---'
rg -n 'class (InvalidTokenError|DecodeError|InvalidAlgorithmError|InvalidIssuedAtError)|InvalidTokenError' . \
-g '*.py' -g '!node_modules' -g '!dist' -g '!build' 2>/dev/null || trueRepository: Agenta-AI/agenta
Length of output: 15387
🌐 Web query:
PyJWT stable API exceptions InvalidTokenError DecodeError InvalidAlgorithmError InvalidIssuedAtError inheritance
💡 Result:
In the PyJWT library, InvalidTokenError is the base exception class for all errors related to a token being invalid during the decode process [1][2]. Both DecodeError and other specific validation errors, such as InvalidAlgorithmError and InvalidIssuedAtError, are subclasses of InvalidTokenError [1][3][4]. The exception hierarchy is structured as follows: - PyJWTError: The top-level base class for all exceptions in the library [1][5]. - InvalidTokenError: A subclass of PyJWTError and the specific base class for issues encountered when calling jwt.decode [1][2]. - DecodeError: A subclass of InvalidTokenError, typically raised when a token cannot be parsed or decoded (e.g., malformed token structure) [1][6][3]. - InvalidAlgorithmError: A subclass of InvalidTokenError, raised when the algorithm specified in the token header is not supported or does not match the expected algorithm [3][7]. - InvalidIssuedAtError: A subclass of InvalidTokenError, raised when the 'iat' (Issued At) claim validation fails (e.g., the claim is in the future) [3][4]. Because these are all subclasses of InvalidTokenError, catching this single exception is an effective way to handle various token validation failures [2]. For a broader catch-all that includes any library-related errors (including configuration or key-related issues), one may catch the parent PyJWTError [5].
Citations:
- 1: https://github.com/jpadilla/pyjwt/blob/master/jwt/exceptions.py
- 2: https://github.com/jpadilla/pyjwt/blob/0ab287bddacd78537a8105263a8b48fbee8c789a/README.md
- 3: https://pyjwt.readthedocs.io/en/2.1.0/api.html
- 4: https://pyjwt.readthedocs.io/en/2.0.0/api.html
- 5: https://stackoverflow.com/questions/74747024/what-is-the-base-exception-for-an-invalid-jwt-in-pyjwt
- 6: https://pyjwt.readthedocs.io/en/2.6.0/api.html
- 7: https://github.com/jpadilla/pyjwt/blob/master/jwt/api_jws.py
Map PyJWT validation failures to invalid_token.
InvalidAlgorithmError, InvalidIssuedAtError, and other InvalidTokenError subclasses bypass the DecodeError handler and become InternalServerErrorException. Import and catch InvalidTokenError after ExpiredSignatureError. Add a regression test for an unsupported JWT algorithm.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 963-968: Logging request-derived input unsanitized allows log forging (CRLF injection); strip newlines / encode the value before logging.
Context: log.debug(
"[auth] secret token unauthorized",
path=request.url.path,
method=request.method,
reason="invalid_token",
)
Note: [CWE-117] Improper Output Neutralization for Logs. OWASP A09:2021 Security Logging and Monitoring Failures.
(log-injection-python)
| token_ttl_seconds: int = int( | ||
| os.getenv("AGENTA_OTLP_TOKEN_TTL_SECONDS") or str(2 * 60 * 60) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-positive telemetry token lifetimes.
A value of 0 creates a token that expires at issuance. A negative value creates an already expired token. Both values cause every telemetry export to fail after configuration is loaded.
Validate AGENTA_OTLP_TOKEN_TTL_SECONDS as greater than zero before assigning token_ttl_seconds.
| const target = | ||
| (runId ? byRun?.get(runId) : undefined) ?? defaultTarget(); | ||
| const problem = { | ||
| traceId, | ||
| endpoint: target.endpoint, | ||
| authorization: target.authorization, | ||
| spans: group.length, | ||
| redactors, | ||
| }; | ||
| if (!target.authorization?.trim() && isAgentaIngest(target.endpoint)) { | ||
| // Agenta's ingest rejects an unauthenticated export, so sending one buys nothing and | ||
| // reports as a 401 that reads like a BAD credential. Say the credential is missing | ||
| // instead, and drop the batch here. | ||
| logExportProblem({ ...problem, outcome: "skipped" }); | ||
| return Promise.resolve(); | ||
| } | ||
| return new Promise<void>((resolve) => { | ||
| try { | ||
| getExporter(target).export(orderParentFirst(group), (result) => { | ||
| if (result.code === ExportResultCode.FAILED) | ||
| console.error( | ||
| "otel: trace export failed", | ||
| traceId, | ||
| result.error, | ||
| ); | ||
| logExportProblem({ | ||
| ...problem, | ||
| outcome: "failed", | ||
| error: result.error, | ||
| }); | ||
| resolve(); | ||
| }); | ||
| } catch (err) { | ||
| // A synchronous export throw (e.g. misconfigured exporter) must stay best-effort: | ||
| // flush() is awaited without a catch, so a reject here would break the run. | ||
| console.error("otel: trace export threw", traceId, err); | ||
| logExportProblem({ ...problem, outcome: "threw", error: err }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound the exporter cache by credential lifetime.
Line 370 calls getExporter(target). exporterCache keys entries with the complete authorization value. Each refreshed trace-ingest token creates a new exporter entry. The cache releases entries only during TraceBatchProcessor.shutdown().
A warm runner can retain expired bearer values and exporter instances for every credential refresh. Remove exporters after their final export, or add bounded eviction that calls shutdown() for evicted exporters.
Railway Preview Environment
Updated at 2026-08-22T11:15:40.767Z |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fc24dc2a-265f-446b-ba60-6523a50a19e4
📒 Files selected for processing (11)
sdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonsdks/python/oss/tests/pytest/unit/agents/test_dtos_capabilities_events.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_secret_repr.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/runner/src/engines/sandbox_agent/pi-assets.tsservices/runner/src/engines/sandbox_agent/runtime-policy.tsservices/runner/src/protocol.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/wire-contract.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review.
| export_authorization: Optional[str] = Field( | ||
| default=None, alias="exportAuthorization" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'class WireOtlpExporter|export_authorization|repr=False|WireOtlpExporter\(' \
sdks/python/agenta/sdk/agents/wire_models.py \
sdks/python/oss/tests/pytest/unit/agents/test_dtos_secret_repr.pyRepository: Agenta-AI/agenta
Length of output: 3928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- wire model imports and base class ---'
sed -n '1,80p' sdks/python/agenta/sdk/agents/wire_models.py
printf '%s\n' '--- related secret fields and tests ---'
rg -n -C 5 'class TraceContext|class _WireModel|export_authorization|test_.*repr|WireOtlpExporter' \
sdks/python/agenta/sdk/agents sdks/python/oss/tests/pytest/unit/agents
printf '%s\n' '--- Pydantic dependency declarations ---'
rg -n -C 2 'pydantic' sdks/python -g 'pyproject.toml' -g 'requirements*.txt' -g 'poetry.lock' -g 'uv.lock'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pydantic import BaseModel, ConfigDict, Field
class WireOtlpExporter(BaseModel):
model_config = ConfigDict(populate_by_name=True, extra="allow")
export_authorization: str | None = Field(
default=None, alias="exportAuthorization"
)
class SafeWireOtlpExporter(BaseModel):
model_config = ConfigDict(populate_by_name=True, extra="allow")
export_authorization: str | None = Field(
default=None, alias="exportAuthorization", repr=False
)
secret = "Secret trace-ingest-token"
unsafe = WireOtlpExporter(exportAuthorization=secret)
safe = SafeWireOtlpExporter(exportAuthorization=secret)
print(f"pydantic={__import__('pydantic').VERSION}")
print(f"unsafe_repr={repr(unsafe)}")
print(f"unsafe_secret_in_repr={secret in repr(unsafe)}")
print(f"safe_repr={repr(safe)}")
print(f"safe_secret_in_repr={secret in repr(safe)}")
print(f"wire_value_preserved={unsafe.export_authorization == secret == safe.export_authorization}")
PYRepository: Agenta-AI/agenta
Length of output: 275
🌐 Web query:
Pydantic 2 Field repr=False BaseModel __repr__ documentation
💡 Result:
In Pydantic V2, the repr parameter in the Field function controls whether a specific field is included in the model's generated repr output [1][2]. When you define a field using Field(repr=False), that field is excluded from the string representation of the model instance [1][3]. By default, repr is set to True for all fields [1][4]. Example usage: from pydantic import BaseModel, Field class User(BaseModel): name: str = Field(repr=True) age: int = Field(repr=False) user = User(name='John', age=42) print(repr(user)) # Output: User(name='John') For computed fields, you can similarly use the repr parameter within the @computed_field decorator [2][5]. For public computed fields, repr defaults to True, while for private computed fields, it defaults to False [2][5]. If you need more advanced customization of the model's representation beyond simply including or excluding fields, you can override the repr or str methods directly on your BaseModel class [6]. For further control over how specific complex types appear in the representation, wrapping them in a RootModel and defining a custom repr on that model is a recommended approach [7].
Citations:
- 1: https://pydantic.dev/docs/validation/latest/concepts/fields/
- 2: https://pydantic.dev/docs/validation/latest/api/pydantic/fields/
- 3: https://pydantic.dev/docs/validation/2.12/concepts/fields/
- 4: https://github.com/pydantic/pydantic/blob/main/docs/concepts/fields.md
- 5: https://pydantic.dev/docs/validation/2.11/api/pydantic/fields/
- 6: Add a method like __repr__ or __str__ pydantic/pydantic#5057
- 7: Modifiable repr for fields pydantic/pydantic#7234
Hide export_authorization from WireOtlpExporter representations. Set repr=False on the field and add a test that the export credential does not appear in repr(WireOtlpExporter(...)).
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
|
The services unit suite was red on every PR stacked above this one, from this lane's own change: Separately, and worth knowing before this merges: |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fb0894ec-9d4d-452e-b821-5751e9254b95
📒 Files selected for processing (1)
services/oss/tests/pytest/unit/agent/test_subscription_status.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| # `get_credentials` returns the caller's credential and a separate trace-ingest | ||
| # one; a single value here unpacks as two characters in the middleware. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the unpacking failure accurately.
Returning the old string would not unpack as two characters. Python would iterate the full string and raise ValueError: too many values to unpack (expected 2). Update the comment to document the actual failure mode.
Trace exports from warm agent sessions failed with 401 for every turn after the first few minutes, so no agent traces reached observability at all. The export bearer was captured ONCE per session: the runner wrote it to the read-once auth file only during environment build, and Pi's telemetry extension read it once at process start. The bearer lives minutes; a warm session lives hours. Two changes, either of which alone leaves a gap. Per-turn refresh. The runner now rewrites the auth file at TURN DISPATCH with the incoming turn's credential, mirroring how the tool relay already starts each turn from the incoming request's toolCallback rather than the parked copy. The extension re-reads the file at before_agent_start, keeping the read-once-then-delete handling per turn instead of per process. A turn whose file is absent keeps the previous bearer, so a failed write is never worse than today. This ships inside the runner image, so it deploys atomically. Turn-sized telemetry credential. Per-turn refresh still leaves a single turn longer than the credential's life exporting with an expired bearer. Rather than lengthen the general credential, /permissions/check now mints a SECOND token carrying a scope claim of trace-ingest with its own TTL (default 2h, override AGENTA_OTLP_TOKEN_TTL_SECONDS) and returns it as telemetry_credentials. The SDK threads it to the trace exporter only; the general 15-minute credential is untouched in lifetime, payload shape, and behavior. The long TTL is safe only because verify_secret_token confines a scoped token to its scope's allowed paths (trace-ingest to /otlp/v1/traces) and fails closed on an unknown scope. That check lives inside verify_secret_token, not at any call site, so no future caller can bypass it. Both directions degrade safely: an API that omits telemetry_credentials leaves the SDK on today's fallback to the general credential, and an SDK that ignores the field is unaffected. Trade-off: the telemetry credential is delivered through /permissions/check rather than as a second token on the /invoke wire, because that endpoint already hands the SDK its credential and both sides cache it together. A single turn that outlives even the 2h scoped TTL would still export with an expired bearer.
Every minted token now carries an `iat` claim alongside `exp`, both derived from one instant so a token's age and its remaining life sum to the lifetime it was minted with. Without it, export-failure diagnostics can report that a credential expired but not how OLD it is, which is the difference between a stale credential and a wrong one — the ambiguity that let a week of 401s go undiagnosed.
…edential The runner reads the Authorization slot to authenticate session-coordination calls, so that slot has to stay the caller's full-authority credential. Carrying the trace-ingest token there broke those calls. Give the export-only credential its own export_authorization field, and leave it unset when no telemetry credential was issued so the export falls back to the caller's credential.
Defines export_authorization across the wire: the runner protocol type, the runner's exportAuthorizationOf accessor and its use in the OTLP exporter and auth-file writer, and the SDK's TraceContext DTO and wire model, with the contract tests and golden request updated to match. Without this the export-only credential has nowhere to ride, and the callers added in the preceding commits reference a field that does not exist.
…xture This lane made get_credentials return the caller's credential and a separate trace-ingest one, but the fixture that stands in for it still returned a single string, so the middleware's two-value unpack turned every authenticated request in that suite into a 500. Five tests were failing on every PR stacked above this one.
5004379 to
f20bede
Compare
dosubot
Bot
added
size:L
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/runner/src/engines/sandbox_agent/pi-assets.ts (1)
775-782: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate
agentDirWritableduring re-preparation.reprepareLocalPiAssetsreturns only.dir, so a failed second write probe is discarded and the run can continue toward harness startup. EnforcePI_AGENT_DIR_UNWRITABLE_MESSAGEon this path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 635716ad-a151-415e-82da-1d54629431b5
📒 Files selected for processing (4)
api/oss/src/utils/env.pyservices/runner/src/engines/sandbox_agent/pi-assets.tsservices/runner/src/environment/runtime-lifecycle.tsservices/runner/src/extensions/agenta.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
|
Closed by owner decision: the per-turn credential refresh is a workaround. Trace export ownership moves to the runner; design in #6211. |
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
…call site CodeRabbit review round on #6164: - Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO. A source walk pins every call site, and the webhook rotation path gets real coverage. - The forged-token test asserts UnauthorizedException/401/invalid_token instead of any exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one). - The cache-key tenancy test no longer reads as if the truncated-id collision were an invariant: it records the tracked exception (issue #6166) that flipping the default needs invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys. - The vault route fake now models production invalidation faithfully: it reaches neither full-id namespace, so the stale-snapshot test passes on the generation, not on the fake. - The design note names #6065 (frontend refactor, merged) and #6135 (stacking base) separately, and records the cache-tenancy gap.
The symptom
No agent traces were reaching observability. Every span batch from a warm agent
session was rejected with
401 Unauthorized, and had been for at least a weekbefore anyone noticed. The export log said
Unauthorizedand nothing else, sothere was no way to tell a stale credential from a wrong one.
Why it happened
The export bearer was captured once per session. The runner wrote it to the
read-once auth file during environment build only, and Pi's telemetry extension
read that file once at process start.
That bearer lives for minutes. A warm agent session lives for hours. So every
turn after the first few minutes exported with a dead credential.
What changes
The runner refreshes the auth file at turn dispatch, using the incoming
turn's credential, mirroring how the tool relay already starts each turn from the
incoming request rather than from a parked copy. The extension re-reads the file
at
before_agent_start, so the read-once-then-delete handling is per turninstead of per process. A turn whose file is missing keeps the previous bearer,
so a failed write is never worse than today's behavior.
/permissions/checkmints a second, turn-sized telemetry token. Per-turnrefresh alone still leaves one turn longer than the credential's life exporting
with an expired bearer. Rather than lengthen the general credential, the endpoint
now issues a token with a
trace-ingestscope claim and its own TTL, two hoursby default, overridable with
AGENTA_OTLP_TOKEN_TTL_SECONDS. It comes back astelemetry_credentials, and the SDK threads it to the trace exporter only. Thegeneral 15 minute credential is untouched in lifetime, payload shape, and
behavior.
The longer TTL is safe only because
verify_secret_tokenconfines a scoped tokento its scope's allowed paths,
trace-ingestto/otlp/v1/traces, and failsclosed on an unknown scope. That check lives inside
verify_secret_tokenratherthan at any call site, so a future caller cannot bypass it.
The export credential gets its own wire field. The runner reads the
Authorizationslot to authenticate session-coordination calls, so that slot hasto stay the caller's full-authority credential. Carrying the trace-ingest token
there broke those calls.
export_authorizationnow carries the export-onlycredential, and stays unset when no telemetry credential was issued so the export
falls back to the caller's credential.
Every minted token now carries an
iatclaim alongsideexp, both derivedfrom one instant, so a token's age and its remaining life sum to the lifetime it
was minted with. Without it, export diagnostics could report that a credential
had expired but not how old it was, which is exactly the ambiguity that let a
week of 401s go undiagnosed.
Before and after
before_agent_starttrace-ingestscoped token, 2h defaultAuthorizationslotexport_authorizationUnauthorizedCompatibility
Both directions degrade safely. An API that omits
telemetry_credentialsleavesthe SDK on today's fallback to the general credential. An SDK that ignores the
field is unaffected. The runner change ships inside the runner image, so it
deploys atomically.
Trade-off worth reviewing
The telemetry credential is delivered through
/permissions/checkrather than asa second token on the
/invokewire, because that endpoint already hands the SDKits credential and both sides cache them together. A single turn that outlives
even the two hour scoped TTL would still export with an expired bearer.
About the commit list
The bottom two commits,
make trace export failures diagnosableandtreat blank export credentials as missing, already merged as #6109 intorelease/v0.112.3.That release branch has not reached
mainyet, so they still appear here. Theywill deduplicate when the release lands.
Testing
test_cli_stream_terminal_only_on_empty_request, also fails on plainmain;this branch touches no streaming or CLI code.
test_auth_secret_token.py: 20 passing.services/runner/tests/unit/otlp-auth-per-turn.test.tsand therunner and SDK wire-contract tests, plus SDK and API tests for the scoped token
and the
iatclaim.