fix(runtime): bind retained environment ownership - #832
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — d271a95d
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-13T22:34:26Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 561db34f
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-13T23:01:31Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 298.7s (2 bridge agents) |
| Total | 298.7s |
💰 Value — sound
Adds a fail-closed pre-admission proof that an existing retained environment was created under the caller's idempotency key, closing a wrong-owner dispatch hole in the in-environment start path added hours earlier in #827; it reuses the established metadata-proof pattern and is verified green.
- What it does: startRetainedRunInEnvironment now requires the provider to prove environment ownership before any admission or dispatch: it calls provider.list({metadata: {retainedIdempotencyKey}}), re-filters returned summaries client-side for id + provider + exact key, and fails closed on zero matches, duplicates, redacted metadata, or a provider with no list operation (src/runtime/retained-run-start.ts:165-204
- Goals it achieves: Prevent wrong-owner/cross-tenant dispatch: before this, a caller could name any environment id and claim any idempotency key, and the runtime would dispatch into an environment whose origin key it never verified — while minting session/execution identities from the claimed key. After the merge, an environment only accepts an in-environment retained start when its provider-side metadata still carri
- Assessment: Good and in-grain. The runtime's retained-run design is uniformly prove-or-refuse (assertRetainedCapabilities at line 398, exactSession binding at dispatch, recoverRetainedRun's 'unverifiable' outcome), and this check follows the same shape, naming, and error-message style. The design constraint is real: AgentEnvironment returned by provider.get has no metadata field (agent-interface environment-r
- Better / existing approach: none — this is the right approach. Alternatives examined and rejected on repo evidence: (1) reading metadata off provider.get is impossible because AgentEnvironment carries no metadata field (environment-runtime.d.ts:620-661); changing that means a cross-repo agent-interface redesign disproportionate to the fix. (2) Verifying via session controlRef like recoverRetainedRun only works post-dispatch;
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
Correct security hardening that prevents wrong-owner dispatch into retained environments by proving ownership through provider metadata, using the only viable mechanism given that AgentEnvironment lacks a metadata field.
- Integration: startRetainedRunInEnvironment is a @stable public API (src/runtime/index.ts:369, documented in docs/canonical-api.md:144). The ownership assertion is wired directly into its flow at retained-run-start.ts:165-169 — after get and capability checks, before dispatchRetainedRun. No non-test callers exist yet; this is a capability ahead of its first production caller, which is normal for fast work. Not
- Fit with existing patterns: The AgentEnvironment interface (environment-runtime.d.ts:620-662) has NO metadata field, while AgentEnvironmentSummary (environment-requests.d.ts:32-38) does. This means provider.list is the only way to verify ownership via metadata — provider.get cannot do it. The AgentEnvironmentQuery.metadata field was designed for exactly this. The codebase already uses metadata-based list filtering in tangle-
- Real-world viability: Metadata round-trips correctly through the production adapter: create preserves it (sandboxOptionsFromCreateInput:497), readBoxMetadata reads it back (environment-provider.ts:1297-1302). Error paths — missing list, mismatched key, duplicate summaries — are all handled by the matches.length !== 1 guard and tested. 37/37 tests pass. One pre-existing limitation: sandboxClientAsProvider.list (environm
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Metadata-query proof shape now exists in two places [maintenance] ``
The 'list by metadata query, re-filter client-side for provider + exact metadata equality, require exactly one match' shape is near-identical in src/runtime/retained-run-start.ts:193-203 and src/candidate-execution/exact-process-executor.ts:370-390. They operate on different types (AgentEnvironmentSummary vs AgentExactProcessEnvironment) in different layers, so extracting a shared helper now would be premature; worth revisiting if a third caller appears.
🎯 Usefulness Audit
🟡 Production provider ignores metadata query filter, causing full-list scan [robustness] ``
sandboxClientAsProvider.list (environment-provider.ts:292) passes only query.providerOptions to client.list(), silently dropping query.metadata. The ownership check compensates with client-side .filter() in assertRetainedEnvironmentOwnership (retained-run-start.ts:196-198), so correctness is unaffected. But the call fetches every environment in the fleet to verify one. This is a pre-existing provider limitation (also present in tangle-sandbox-exact-process-provider.ts:90-94), not introduced by t
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Fail closed before admission or dispatch when an existing retained environment cannot prove its original key through provider metadata.
The provider must expose the exact environment summary and matching retained key. A missing list operation, redacted key, mismatched key, duplicate summary, wrong provider, or wrong environment fails before work starts.
Proof: