Skip to content

fix(interface): define generic environment create idempotency - #199

Merged
drewstone merged 2 commits into
mainfrom
fix/generic-provider-create-idempotency-20260816
Aug 17, 2026
Merged

fix(interface): define generic environment create idempotency#199
drewstone merged 2 commits into
mainfrom
fix/generic-provider-create-idempotency-20260816

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • define one keyed environment-create identity in Agent Interface
  • coalesce equal retries and reject changed input before provider effects
  • apply the contract to Tangle, CLI Bridge, E2B, ComputeSDK, and Daytona

Proof

  • Agent Interface: 440 tests
  • provider testkit: 24 tests
  • Tangle: 142 tests
  • CLI Bridge: 73 tests
  • E2B, ComputeSDK, Daytona: focused tests pass
  • all seven package typechecks pass
  • changeset validation passes

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 3859baad

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-16T13:47:58Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 4 (4 weak-concern)
Heuristic 0.0s
Duplication 0.1s
Interrogation 181.3s (2 bridge agents)
Total 181.4s

💰 Value — sound-with-nits

Defines and uniformly enforces a canonical retry-safety contract on the pre-existing but underspecified CreateAgentEnvironmentInput.idempotencyKey field, via one shared digest+coalescing helper applied across all five providers — coherent, in-grain, and worth shipping; only minor retention/scoping n

  • What it does: Before this change, idempotencyKey existed on CreateAgentEnvironmentInput (environment-runtime.ts:635, introduced in 8fd0d63) but had provider-specific semantics: Tangle forwarded it to the server, the CLI bridge used it as environmentId, and E2B/Daytona/ComputeSDK ignored it entirely (keyed retries created fresh sandboxes). This PR (1) documents the contract on the field and on AgentEnvironmentPr
  • Goals it achieves: Makes keyed generic environment create one retry-safe operation everywhere: a retried create with the same key and canonically equal input returns the same environment instead of leaking a duplicate sandbox (previously guaranteed nowhere except implicitly at CLI bridge), and a caller bug reusing a key with different input fails fast before spending provider resources. It also closes a contract gap
  • Assessment: Good change, strongly in the grain of the codebase. The repo's established pattern is digest-bound idempotency contracts defined in agent-interface and enforced by testkit conformance checks — workspace checkpoint/fork digest binding (workspace-checkpoint.ts:268, workspace-fork.ts:396) and 'exact-process-idempotency' (exact-process-conformance.ts:63,91-94) — and generic create was the one keyed op
  • Better / existing approach: none — this is the right approach. Searched for existing equivalents: grep for inFlight/coalesce/pending-Promise maps in agent-interface found only the new helper; withRetry (agent-core/src/retry/index.ts:58) is backoff-only retry, not keyed coalescing; the Tangle exact-process idempotency (exact-process.ts:74,105-121) verifies digests in server-owned sandbox metadata, which is Tangle-specific and
  • 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

A well-fit extension of the repo's established keyed-idempotency grain to generic environment create, applied uniformly across all five providers and enforced by the conformance testkit, with only minor memory-retention and cross-restart notes.

  • Integration: Fully wired and reachable. The helper (environment-runtime.ts:673) is applied on the create path of every provider: tangle-provider.ts:166, cli-bridge index.ts:72, e2b index.ts:64, computesdk index.ts:70, daytona index.ts:69. It has a real consumer now: the provider testkit conformance harness injects a key (provider-conformance.ts:34-36) and asserts replay-sameness and changed-input rejection (li
  • Fit with existing patterns: Matches the codebase's dominant pattern exactly rather than competing with it. The 'canonical digest of request material with key and attempt-signal excluded, conflict on changed input' shape is the same one used by workspace checkpoint/fork (workspace-fork.ts:231-398), native context continuation, portable context transfer, and interactive session requests. It reuses the shared canonicalCandidate
  • Real-world viability: Holds up beyond the happy path: concurrent retries coalesce on the pending promise, a failed create evicts its record so the next attempt re-runs (environment-runtime.ts:703-706), an already-aborted replay rejects with the abort reason without becoming part of create identity, and the digest is key-order-insensitive via the shared RFC 8785-style canonicalizer (tests at environment-provider.test.ts
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Idempotency records grow without bound for the adapter's lifetime [robustness] ``

createAgentEnvironmentWithIdempency stores every keyed record (digest + pending + resolved environment) in a plain Map (environment-runtime.ts:674-698) with no eviction, and each record pins the created AgentEnvironment. A long-lived provider singleton serving many keyed creates accumulates memory indefinitely. Consider bounding the cache or dropping the resolved environment once the backing service can reconstruct via get(). Does not gate shipping; the conformance and retry use cases touch few

🟡 Keyed contract is in-process only outside Tangle [integration] ``

README.md:59 states providers backed by a remote service must forward and retain the key; only Tangle forwards it (tangle-create-options.ts:79). E2B, Daytona, and ComputeSDK adapters apply the contract purely in-memory, so a retry after process restart silently creates a second environment with the same key. This is a documented, deliberate layering (environment-runtime.ts:668-669) and those backing SDKs lack native key support, so no materially better adapter-side approach exists — a note for c

💰 Value Audit

🟡 Idempotency records retain settled environments forever [maintenance] ``

createAgentEnvironmentWithIdempotency (environment-runtime.ts:659-706) stores record.environment on success and never evicts, even after environment.destroy(). A long-lived provider adapter accumulating many unique keys holds a strong reference to every environment object it created. If sustained, add eviction on destroy or an LRU bound; the per-adapter Map shape already leaves room for it.

🟡 E2B and Daytona default mappers drop the key, so their idempotency is process-local only [better-architecture] ``

rg confirms computesdk forwards the key (computesdk index.ts:201) and Tangle/CLI-bridge reach services that retain it, but e2bCreateOptions (e2b index.ts:148) and daytonaCreateParams (daytona index.ts:169) never forward idempotencyKey. After adapter reconstruction, a keyed retry against those two providers creates a second environment, which the new README contract ('must return or reconstruct the same environment') does not hold for. The helper's doc comment scopes this honestly to the backing


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.

value-audit · 20260816T135505Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — 3859baad

Review health 100/100 · Reviewer score 0/100 · Confidence 85/100 · 47 findings (10 medium, 37 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 20 55 0 0
Confidence 85 85 85 85
Correctness 20 55 0 0
Security 20 55 0 0
Testing 20 55 0 0
Architecture 20 55 0 0

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Explicitly-undefined optional fields throw in create-input digest instead of matching absent fields — packages/agent-interface/src/environment-runtime.ts

The destructure strips only idempotencyKey and signal, so an input like {profile, name: undefined} or metadata: {x: undefined} keeps an undefined-valued own property in material. isCanonicalJsonValue (agent-candidate-schema-common.ts:220-221) rejects undefined, so canonicalCandidateDigest throws 'candidate document must be finite, acyclic RFC 8785 JSON' — reproduced empirically via vitest. Per JSON semantics {name: undefined} and {} are the same create input and must produce the same digest; instead the entire keyed create hard-fails with an error that never mentions input identity. This is the common TS pattern of forwarding defaulted-undefined option fields, and every provider adapter (e.g. cli-bridge/src/index.ts:72, tangle-provider.ts:166) passes caller input through verbatim, so it

🟠 MEDIUM Idempotency records map retains every resolved environment forever — packages/agent-interface/src/environment-runtime.ts

On success, record.environment = environment caches the resolved AgentEnvironment and the entry is never evicted; AgentEnvironmentCreateIdempotencyRecord.environment (line 662) pins it for the life of the records Map. The in-tree adapter (agent-provider-cli-bridge/src/index.ts) creates one Map per provider instance with no pruning, so every distinct keyed create ever issued pins its environment (which can hold streaming/session resources) indefinitely. Impact: unbounded memory growth on long-lived adapters. Fix: cap/TTL the map (e.g., LRU or evict after a bounded retention window), or document that callers must bound key cardinality.

🟠 MEDIUM Unbounded in-memory retention of every successful create — packages/agent-interface/src/environment-runtime.ts

On success, record.environment is set (line 701) and the record is never evicted; only the failure path calls records.delete (line 704). The Map is held for the lifetime of the provider factory, so each distinct idempotency key permanently pins its resolved AgentEnvironment (a live sandbox handle in e2b/daytona/computesdk, a process-spawned environment in cli-bridge) plus its digest. A long-lived adapter processing many distinct keys grows without bound and also retains environments the caller h

🟠 MEDIUM idempotencyKey contract promises same-environment returns without qualifying persistence scope — packages/agent-interface/src/environment-runtime.ts

The interface docs (lines 627-635 and the AgentEnvironmentProvider.create doc at 718-724) state the provider 'must return or reconstruct the same environment' for the same key and canonical input, with no scope qualifier. The helper's own doc (line 668-669) concedes the backing service must retain the key across adapter reconstruction, and all in-repo adapters hold only an in-memory Map (e.g. agent-provider-e2b/src/index.ts:48). After process restart, the same key+input silently create

🟠 MEDIUM Keyed create hard-fails on partial/non-JSON input that the base pipeline tolerated — packages/agent-provider-cli-bridge/src/index.ts

create() passes the raw caller input to createAgentEnvironmentWithIdempotency, which computes agentEnvironmentCreateInputDigest over it (environment-runtime.ts:682). That digest requires the entire input minus idempotencyKey/signal to be finite RFC 8785 JSON: isCanonicalJsonValue returns false for any undefined value or non-JSON object anywhere in the tree (agent-candidate-schema-common.ts:218-246), so canonicalCandidateJson throws 'candidate document must be finite, acyclic RFC 8785 JSON'. The base flow only snapshotted the profile and forwarded the rest to JSON.stringify-based wire bodies (wire.ts:38 spreads metadata), which silently drop undefined. Concretely, create({ profile: { name: 'worker' }, metadata: undefined, idempotencyKey: 'k' }) or profile.model: undefined (both legal per Cr

🟠 MEDIUM Idempotency is process-local; E2B cannot reconstruct across restart (billing risk) — packages/agent-provider-e2b/src/index.ts

createRecords is a closure-scoped in-memory Map (index.ts:46-49, used at 63-69). The shared helper's contract states 'The provider's backing service remains responsible for retaining the key across adapter reconstruction' (environment-runtime.ts:666-671), and the interface JSDoc added in this PR requires the same key to 'return or reconstruct the same environment' (environment-runtime.ts:625-635). E2B's Sandbox.create has no idempotency-key parameter and the adapter never persists key->sandboxId, so a retry with the same idempotencyKey after a process restart creates a brand-new billable sandbox instead of reconstructing via get()/Sandbox.connect (index.ts:70-75). Impact: double billing and contract violation in crash-retry workflows. Fix: persist key->sandboxId (or rely on a backend key)

🟠 MEDIUM Idempotency key never reaches E2B; restart or new provider instance creates duplicate sandboxes — packages/agent-provider-e2b/src/index.ts

createRecords is a closure-local Map, and e2bCreateOptions (index.ts:148-157) does not include input.idempotencyKey in the E2B create options or metadata, nor does the provider implement list() to reconstruct by key. After a process restart, or when a caller constructs a second createE2BProvider() with the same options, the same idempotencyKey with canonically equal input creates a second live sandbox — duplicate billing and a silent violation of the documented create contract ('must return or reconstruct the same environment'). The helper's doc (environment-runtime.ts:668-670) explicitly delegates cross-reconstruction retention to the backing service, which for this adapter is E2B, yet the key is never sent there. Fix: forward the key into sandbox metadata (e2bCreateOptions already passes

🟠 MEDIUM Replay returns a possibly-dead environment; no reconstruction attempt — packages/agent-provider-e2b/src/index.ts

After a keyed environment is destroyed (index.ts:140-143) or its E2B sandbox is reaped server-side, a retry with the same key returns the cached record.environment object (environment-runtime.ts:690) whose sandbox handle is gone; subsequent exec/read calls fail. E2B sandboxes are cheap but not reconstructable from a cached wrapper — the adapter has get()/Sandbox.connect (index.ts:70-75) but never uses it on replay. Impact: a 'successful' idempotent create yields a dead environment. Fix: on cached replay, probe sandbox liveness and reconstruct via Sandbox.connect, or re-run create.

🟠 MEDIUM createRecords map grows unboundedly; no eviction on environment destroy — packages/agent-provider-e2b/src/index.ts

Every keyed create inserts a record that is only removed on create failure (environment-runtime.ts:703-705). Successful keyed creates — now every conformance-suite create, since provider-conformance.ts forces an idempotencyKey — are retained for the provider's lifetime, and each record pins the AgentEnvironment (and its E2BSandboxLike handle) via the environment field (environment-runtime.ts:701). destroy() (index.ts:140-143) has no path to remove its record. In a long-lived process issuing many unique keys this leaks one environment object per key. Fix: drop the record when its environment is destroyed, or bound the map (LRU/TTL).

🟠 MEDIUM Idempotency digest is computed on raw input, so effective-equal retries can spuriously conflict and explicit-undefined fields make keyed creates throw — packages/agent-provider-tangle/src/tangle-provider.ts

create() delegates to createAgentEnvironmentWithIdempotency, whose digest (environment-runtime.ts:648-656) drops only idempotencyKey and signal and RFC-8785-canonicalizes the rest of the RAW input. Two consequences. (1) False conflict: {profile, idempotencyKey} and {profile, idempotencyKey, backend:'opencode'} are semantically identical (sandboxOptionsFromCreateInput defaults backend to 'opencode' at tangle-create-options.ts:82) but produce different digests, so a retry-after-timeout that re-serializes with an explicit default is rejected with 'conflicts with a different create input' instead of coalescing — the exact failure the retry mechanism exists to avoid, and the error drives the caller to a new key and a second billed box. (2) Regression: tangle's own validation accepts explicit-un

🟡 LOW No test coverage for the failure-and-retry path — packages/agent-interface/src/environment-provider.test.ts

The 'coalesces same-key retries' test covers success replay, collision rejection, and entry-abort, but not: (1) a failing create deleting its record so a later retry with the same key invokes create() a second time, and (2) a second coalesced caller receiving the same rejection from existing.pending. These are the two most error-prone branches of the helper (lines 699-705) and are currently untested. Add a case where create rejects, assert the key is cleared and a retry re-invokes create, and a case where two callers share one rejected pending promise.

🟡 LOW Test gaps: key-exclusion from digest, concurrent in-flight coalescing, and failure eviction untested — packages/agent-interface/src/environment-provider.test.ts

Three behaviors this file is the natural home for are untested: (1) the digest test (lines 82-96) never varies idempotencyKey, so the claim 'the operation key is not part of input identity' is never exercised — only signal exclusion and metadata key-order canonicalization are; (2) no concurrent in-flight test, so the existing.environment ?? existing.pending branch with a still-unresolved promise (environment-runtime.ts:690) is never hit here — I verified it works via a scratch test with a deferred create; (3) no test that a rejected create evicts its record so the same key can retry fresh (environment-runtime.ts:703-705) — also ver

🟡 LOW @internal exports are de facto public API of the environment-provider entrypoint — packages/agent-interface/src/environment-runtime.ts

environment-provider.ts:6 does export * from "./environment-runtime.js", the package publishes dist/environment-provider.d.ts, and stripInternal is not enabled in tsconfig (only declaration: true), so the three @internal-tagged symbols (agentEnvironmentCreateInputDigest, createAgentEnvironmentWithIdempotency, AgentEnvironmentCreateIdempotencyRecord) ship in the public type surface — and five other workspace packages already import them across package boundaries. The @internal tags are misleading: either remove them and accept these as public contract (they now are), or enable stripInternal and route cross-package sharing through a dedicated subpath.

🟡 LOW Canonicalization collapses -0 with 0 in create input — packages/agent-interface/src/environment-runtime.ts

-0 passes Number.isFinite in isCanonicalJsonValue and serializes via JSON.stringify to "0", so metadata {x: -0} and {x: 0} hash identically (confirmed with a probe). RFC 8785 numbers exclude -0, so the validator should reject it rather than silently treating two distinct inputs as one idempotent operation. Impact is limited to a rare input-distinctness collision in metadata/providerOptions. Fix: reject Object.is(value, -0) in isCanonicalJsonValue.

🟡 LOW Digest throws for non-canonical-JSON inputs the type permits — packages/agent-interface/src/environment-runtime.ts

agentEnvironmentCreateInputDigest feeds the full material (metadata, providerOptions are Record<string, unknown>) into canonicalCandidateDigest, which calls isCanonicalJsonValue and throws 'candidate document must be finite, acyclic RFC 8785 JSON' for undefined, Date, BigInt, function, or circular values. A caller spreading a partial input (e.g. { ...base, name: undefined } or metadata:{ x: undefined }) hits a hard, cryptic failure even on the keyed path before any create effect. The interface type (Record<string, unknown>) is broader than the digest's actual requirement. Low impact today because the bundled providers build clean inputs, but the type/contract mismatch is a landmine. Fix: validate/normalize input before hashing, or tighten the JSDoc to state the canonical-JSON requirement e

🟡 LOW Digest throws on explicitly-undefined optional create fields — packages/agent-interface/src/environment-runtime.ts

{ idempotencyKey, signal, ...material } keeps keys whose value is undefined in material; canonicalCandidateJson then calls isCanonicalJsonValue(undefined) which returns false and throws 'candidate document must be finite, acyclic RFC 8785 JSON'. tsconfig has no exactOptionalPropertyTypes, so { ...input, backend: undefined } is legal TS. Empirically confirmed with a vitest probe: the digest throws instead of canonicalizing. Impact: a keyed retry whose input contains an explicit undefined field crashes before the record lookup instead of replaying the cached environment. Fix: strip undefined values from material before hashing (they canonically serialize to nothing anyway).

🟡 LOW Failure path deletes the record, so the changed-input guarantee lapses — packages/agent-interface/src/environment-runtime.ts

The interface contract (line 630-633) and provider docstring state that the same key with a changed input 'must be rejected'. In this helper the collision check only applies while a record is in the in-memory Map; records.delete(key) on failure (and any adapter reconstruction) drops that memory, so a later same-key call with a different input re-runs create and may create a second environment. This is mitigated because the helper docstring delegates durable key retention to the provider's backing service, but the client-side guarantee is weaker than the interface docstring states. Suggest the docstring explicitly note the guarantee h

🟡 LOW Idempotency record map grows without bound and retains settled promises — packages/agent-interface/src/environment-runtime.ts

Records are never evicted after success, and each record keeps readonly pending: Promise<T> forever even after settlement, although line 690 short-circuits on environment so the settled promise is dead weight. A long-lived adapter accumulates one record (promise + environment reference) per keyed create, indefinitely; no eviction, TTL, or pruning API is offered. Not a correctness bug, but for a hot service doing many keyed creates this is unbounded memory retention of garbage. Fix: make pending mutable and null it on settlement, and/or document that the owning adapter must evict.

🟡 LOW signal is checked once at entry, never wired into the create effect — packages/agent-interface/src/environment-runtime.ts

input.signal?.throwIfAborted() runs once (line 678), but create is typed () => Promise and none of the four provider call sites (e.g. () => createEnvironment(input)) observe input.signal during the sandbox create. An abort issued after entry therefore does not cancel or reject the pending create, and coalesced callers awaiting existing.pending get no abort wiring either. The JSDoc claims 'signal controls one attempt', which overstates the actual behavior. Not a regression (providers already ignored signal), but the contract is misleading. Fix: pass the signal into create, or narrow the doc to 'signal aborts the attempt only before the crea

🟡 LOW A rejected keyed create frees the key for a different input — packages/agent-provider-cli-bridge/src/index.ts

createAgentEnvironmentWithIdempotency deletes the record when the underlying create rejects (environment-runtime.ts:703-706), so after create({idempotencyKey:'K', profile:A}) fails, a create({idempotencyKey:'K', profile:B}) with a different input is accepted instead of rejected as a conflict. This is a necessary tradeoff (otherwise a transient failure permanently poisons the key) and for cli-bridge create performs no server-side effect, so no duplicate side effect is possible — but callers relying on the adapter alone for strict reject-on-different-input after a failure will see the looser behavior. Documented nowhere; acceptable given the backing service is the durable idempotency authority.

🟡 LOW Idempotency record map never evicts and holds strong environment references — packages/agent-provider-cli-bridge/src/index.ts

createRecords (index.ts:29-32) stores one AgentEnvironmentCreateIdempotencyRecord per idempotencyKey for the provider's lifetime; the helper (environment-runtime.ts:694-701) only sets record.environment on success and never deletes settled records. A long-lived provider issuing many keyed creates retains every environment handle in memory. Impact is small because cli-bridge environments are stateless facades, and this matches the identical pattern in e2b/daytona/computesdk/tangle adapters plus the helper's documented stance that the backing service owns durable retention. If desired, store only {digest} after settlement or cap the map; not a merge blocker.

🟡 LOW Idempotency records are never evicted; environment handles retained for provider lifetime — packages/agent-provider-cli-bridge/src/index.ts

createRecords Map (index.ts:29-32) is populated on every keyed create and the helper only deletes a record on creation error (environment-runtime.ts:703-705); successful records and their resolved environment objects are kept forever. Each environment handle closes over options, the caller's fetch, the snapshot profile, and internal runs/sessions maps (retained-environment.ts:65-71). A long-running host creating many keyed environments grows memory unboundedly. Also, create() with the same key after environment.destroy() returns the destroyed handle, whose stream/dispatch throw 'cli-bridge environment is destroyed' — a trap for callers expecting a fresh environment. Fix: evict the record on destroy (and/or document that a new idempotencyKey is required after destroy).

🟡 LOW createRecords map grows without eviction — packages/agent-provider-cli-bridge/src/index.ts

The per-provider createRecords Map is never pruned: successful keyed creates cache {digest, pending, environment} forever (the helper deletes only on create failure). Each record holds a strong reference to the fully retained AgentEnvironment (runs, sessions, readers, transport). A long-lived provider creating many distinct-idempotencyKey environments leaks them after destroy(). Impact: unbounded memory growth for long-running processes; acceptable for the typical short-lived CLI bridge. Fix: expose a release/evict on environment destroy(), or cap the map size (e.g. LRU).

🟡 LOW E2B test does not exercise the new idempotency path — packages/agent-provider-e2b/src/index.ts

packages/agent-provider-e2b/src/index.test.ts mock Sandbox.create always returns the same id 'e2b-1' (index.test.ts:8-23), so the conformance suite's replay assertion (replay.id === environment.id, provider-conformance.ts:97-106) passes even if the idempotency map were removed; only the collision-rejection assertion actually exercises the map. The changed code's coalescing behavior is therefore effectively untested for E2B. Fix: add a mock that counts/varies create calls and assert create is invoked once for same-key replays.

🟡 LOW Keyed create now throws on non-JSON providerOptions before any E2B call — packages/agent-provider-e2b/src/index.ts

agentEnvironmentCreateInputDigest digests the full input including providerOptions/metadata/env/secrets via canonicalCandidateDigest (environment-runtime.ts:648-656), which throws 'candidate document must be finite, acyclic RFC 8785 JSON' for any non-JSON value (agent-candidate-schema-common.ts:58-63). Previously providerOptions was passed straight to Sandbox.create; now, when idempotencyKey is set, a Date/class-instance/function in providerOptions fails the create with an unrelated digest error before E2B is contacted. Impact: new confusing failure mode for keyed creates. Fix: digest only a JSON-safe projection of the input, or validate and raise a clearer error.

🟡 LOW Same-key retry after destroy() returns a dead sandbox that reports status 'running' — packages/agent-provider-e2b/src/index.ts

destroy() kills/closes the sandbox but has no hook into createRecords, so a subsequent create() with the same idempotencyKey returns the cached environment (environment-runtime.ts:690 returns existing.environment ?? existing.pending) whose sandbox is dead, while environment.status() is hardcoded to return "running" (index.ts:95). The caller gets an apparently-healthy handle to a killed sandbox. Fix: evict the record on destroy (e.g., pass an onDispose callback that deletes the record) or make status() reflect real sandbox state.

🟡 LOW Unbounded growth of createRecords map on successful creates — packages/agent-provider-e2b/src/index.ts

createRecords (line 46) is populated for every successful keyed create and never evicted. In the helper (environment-runtime.ts:693-702) the record is set on create and record.environment retained on success; only the error path deletes it. A long-lived provider instance serving many distinct idempotency keys retains one AgentEnvironment (which holds closures over the sandbox) per key forever. For per-request (serverless) provider lifetime this is fine; for a persistent process it is a leak. Fix: add eviction/TTL or document a lifetime contract on the map.

🟡 LOW createRecords grows without bound for the provider's lifetime — packages/agent-provider-e2b/src/index.ts

Successful keyed creates are retained forever: the helper sets record.environment (environment-runtime.ts:701) and never deletes it, and each record strongly references the full AgentEnvironment and E2B sandbox handle. In a long-lived host process that creates many keyed environments, memory grows monotonically with no TTL, size cap, or eviction on destroy. Not a correctness bug for the single-process contract, but unbounded retention. Fix: cap the map size / add TTL, or drop the environment reference once the sandbox is destroyed.

🟡 LOW idempotencyKey not forwarded to E2B backing create — packages/agent-provider-e2b/src/index.ts

The provider now advertises idempotent create, but e2bCreateOptions (lines 148-157) forwards only template/apiKey/envs/metadata/providerOptions and never passes input.idempotencyKey to Sandbox.create. Dedup therefore holds only within one createE2BProvider instance (the in-memory Map). If the provider instance is reconstructed (server restart, cold start, horizontal scale-out), a replay of the same key creates a new — and separately billable — sandbox rather than returning the original. The helper doc explicitly delegates cross-reconstruction retention to the backing service, which E2B does not do. Not a regression, but the advertised guara

🟡 LOW Custom mapper can inject an idempotencyKey the caller never supplied — packages/agent-provider-tangle/src/tangle-provider.ts

The preservation guard is gated on input.idempotencyKey !== undefined, so it only fires when the caller supplied a key. When the caller creates without a key, a mapCreateInput implementation may add options.idempotencyKey = '' and it passes assertMappedCreateOptions untouched. The generic contract says keyless creates 'may create a fresh environment' each call, but the injected key makes the sandbox service dedupe them, silently collapsing keyless creates onto one box. Mapper code is trusted embedder configuration, so impact is low. Fix: require createOptions.idempotencyKey === input.idempotencyKey unconditionally (drop the input-side undefined gate).

🟡 LOW Idempotency record map grows without bound for the provider's lifetime — packages/agent-provider-tangle/src/tangle-provider.ts

createRecords (Map<string, AgentEnvironmentCreateIdempotencyRecord>) only ever gains entries: the shared helper sets on first keyed create and deletes only on failure (environment-runtime.ts:698-704), never on success or environment deletion. A long-lived provider process issuing many distinct keys retains every record's digest, resolved pending promise, and AgentEnvironment handle forever, preventing GC of deleted environments. No eviction or size bound exists. Impact is gradual memory growth in long-running hosts. Fix: drop the record when the environment's delete resolves, or bound the map (LRU) while documenting that the sandbox service holds the durable key mapping (the helper's doc already states the backing service is responsible for retention across adapter reconstruction).

🟡 LOW Input digest and record key computed before size/shape bounds are enforced — packages/agent-provider-tangle/src/tangle-provider.ts

The wrapper computes agentEnvironmentCreateInputDigest over the raw input and inserts the key into createRecords before createEnvironment runs assertCreateInputShape/assertBoundedJson/boundedIdentifier. An oversized metadata blob or idempotencyKey string is therefore canonicalized (deep JSON stringify + SHA-256) and briefly stored as a Map key before rejection. Caller trust is typically in-process for this SDK, so this is a marginal hardening gap, not an exploitable hole; the record is deleted on the validation throw. Fix (optional): run the cheap shape/bound assertions before the digest, or bound the key length inside the wrapper.

🟡 LOW Keyed replay after environment deletion returns a stale, dead handle — packages/agent-provider-tangle/src/tangle-provider.ts

If a caller deletes the environment (env.delete()) and then retries the identical keyed create, createAgentEnvironmentWithIdempotency returns existing.environment (environment-runtime.ts:690) — the cached handle to the already-deleted box — instead of reconstructing or erroring. The generic contract permits 'return or reconstruct', but the returned object fails on first use against a deleted sandbox, surfacing as a confusing downstream client error rather than a fresh environment. Fix: clear the record when the environment wrapper's delete succeeds, or check a status/liveness flag on replay before returning the cached instance.

🟡 LOW Same-key replay after destroy() returns the destroyed environment wrapper — packages/agent-provider-tangle/src/tangle-provider.ts

The idempotency record is never invalidated by environment.destroy(), so a caller that destroys a keyed environment and later replays provider.create with the same key and input receives the SAME already-destroyed environment object (helper returns record.environment, environment-runtime.ts:690). The returned wrapper's operations now hit a deleted sandbox box with no signal of the destroy. The interface contract says the same key must return 'the same environment', which the destroyed wrapper technically satisfies, but a caller relying on replay-to-reconstruct (per the 'return or reconstruct' wording) will silently get a dead handle. At minimum document that destroy() permanently invalidates a key in this provider, or clear the record on destroy.

🟡 LOW createRecords map grows without bound and retains destroyed environments — packages/agent-provider-tangle/src/tangle-provider.ts

createRecords is a per-provider Map with no eviction: every unique keyed create stays forever and the record holds the resolved AgentEnvironment (which closes over the sandbox box and client) after completion. A caller that uses a fresh idempotencyKey per environment — the documented pattern, since the sandbox service retains keys only across reconstruction — leaks the full environment graph for the process lifetime, including environments already destroyed via environment.destroy(). Bounded in practice for single-session agents but unbounded for orchestration loops that create many environments. Consider storing a weak reference or evicting on destroy()/on environment termination.

🟡 LOW createRecords map retains destroyed environments for provider lifetime — packages/agent-provider-tangle/src/tangle-provider.ts

createRecords (Map<string, AgentEnvironmentCreateIdempotencyRecord>) is never evicted. Every distinct idempotencyKey inserts a record holding the resolved environment object (record.environment) plus its pending promise, and successful records are kept even after the environment is destroyed. For a long-lived provider instance (server/bridge) creating many keyed environments, this grows without bound. Evidence: tangle-provider.ts:75-78 declares the map; create() at 165-171 delegates to createAgentEnvironmentWithIdempotency, whose success path (environment-runtime.ts:699-701) only ever sets record.environment and never deletes. Fix: bound or evict entries (e.g., delete on destroy, or a size/TTL cap) since the doc comment (environment-runtime.ts:668-671) states cross-adapte

🟡 LOW Cleanup destroy can mask the conformance assertion — packages/agent-provider-testkit/src/provider-conformance.ts

await changedEnvironment.destroy?.() runs before the assert(collisionRejected, ...) at line 125. If destroy rejects (provider already tearing down, network error), the cleanup error propagates and replaces the intended 'reusing a create key with changed input must reject' diagnostic, making a contract failure look like a cleanup failure. Swap the order or wrap the destroy in try/catch so the conformance verdict is always the reported error.

🟡 LOW Collision check accepts any thrown error, allowing spurious passes — packages/agent-provider-testkit/src/provider-conformance.ts

The bare catch { collisionRejected = true; } treats every failure — transient network error, quota exhaustion, abort — as proof the provider rejects key reuse with changed input. A provider whose third create fails for an unrelated reason passes 'create-idempotency-collision' without implementing the contract. The interface currently defines no typed conflict error, so the kit cannot check the error class; at minimum document the limitation, or introduce a typed IdempotencyConflictError in agent-interface and assert on it. Inherent eval-validity weakness, not a wrong result in the happy path.

🟡 LOW Collision rejection cannot be distinguished from any other create failure — packages/agent-provider-testkit/src/provider-conformance.ts

catch { collisionRejected = true } treats ANY throw from the changed-name create as a valid idempotency collision. A provider that rejects the -changed name for an unrelated reason (name validation, transient backend error, rate limit) would satisfy the create-idempotency-collision check spuriously, producing a false pass. The only changed field is name, so this is unlikely but not impossible. Fix: narrow the check — e.g., inspect the error message/type for the collision contract, or re-run the collision with the ORIGINAL input (which must NOT reject) to isolate that rejection is collision-specific.

🟡 LOW Derived idempotency key is not run-unique; repeat runs against server-side-keyed providers fail — packages/agent-provider-testkit/src/provider-conformance.ts

Key is ${options.name}-environment-create, and withEnvironmentCleanup destroys the environment at the end of the run. For a provider whose idempotency is retained by the backing service (explicitly encouraged by the interface doc: 'the backing service remains responsible for retaining the key across adapter reconstruction'), a second run with the same options.name replays the first run's destroyed environment; replay.id === environment.id still passes but stream() then fails, producing a misleading conformance failure that is a test-key collision, not a provider defect. Derive the key with a run-unique component (e.g. options.name + a per-run nonce), which keeps the in-run replay/collision checks valid.

🟡 LOW Idempotency checks are unconditional with no opt-out in the public conformance API — packages/agent-provider-testkit/src/provider-conformance.ts

The harness now always injects idempotencyKey (when absent) and always runs create-idempotency/create-idempotency-collision. But CreateAgentEnvironmentInput.idempotencyKey is documented as optional, and AgentEnvironmentProvider.create states 'Without a key, each call may create a fresh environment' — i.e. keyless providers are contractually legitimate. ProviderConformanceOptions (conformance-types.ts:37-44) has no toggle to skip idempotency, so a third-party provider that only supports keyless create now fails conformance. This is coordinated with every in-repo provider (cli-bridge, computesdk, daytona, e2b, tangle all gain the helper in this PR), so it is intentional — but it tightens the public testkit contract silently and should be called out in the README/release notes.

🟡 LOW Key-order probe covers top-level fields only — packages/agent-provider-testkit/src/provider-conformance.ts

Object.fromEntries(Object.entries(createInput).reverse()) reverses only top-level key insertion order; nested objects (profile, env, metadata, providerOptions) keep their original order. RFC 8785 sorts recursively, so the probe is valid but under-tests nested-order insensitivity — a provider that canonicalizes top-level keys but raw-stringifies a nested object would still pass. Strengthen by also shuffling one nested object (e.g. profile) in the replay input.

🟡 LOW No opt-out: conformance now hard-requires keyed idempotency from every provider — packages/agent-provider-testkit/src/provider-conformance.ts

idempotencyKey is force-injected when the caller omits it, and ProviderConformanceOptions.createInput cannot opt out ({ idempotencyKey: undefined } after spread still hits the === undefined branch). Every provider run through the published testkit (0.8.2) must now implement coalesce-on-replay and reject-on-changed-input or the suite fails with a message ('same create key and canonical input...') that does not name the new contract. In-repo providers were all updated in this PR so CI stays green, but third-party adapters get a silent breaking bar-raise. Consider a requireIdempotency?: boolean flag (default true) or a changelog/README note that the 1.0.0 interface makes create idempotency mandatory.

🟡 LOW Replay check asserts id/provider equality only, not functional sameness — packages/agent-provider-testkit/src/provider-conformance.ts

The create-idempotency check passes if the replay returns any object with matching id and provider; a stub with the same identity but no working stream() would pass. Subsequent checks (stream, workspace) run only against the original environment, so the replay's functional identity is never exercised. The digest-based collision check carries the real guarantee; consider also asserting that the replay is the same object instance (coalesced) or at least running the stream/terminal checks on the replay.

🟡 LOW Replay environment is never cleaned up; collision-path cleanup is inconsistent — packages/agent-provider-testkit/src/provider-conformance.ts

replay (line 100) is discarded and only the original environment is destroyed by withEnvironmentCleanup. For providers using the in-memory helper (all in-repo adapters, incl. tangle-provider.ts which wraps the same helper) replay returns the identical object, so this is a no-op; but for a provider that legitimately 'reconstructs' a fresh wrapper per create (a valid reading of the interface contract), the fresh handle's owned resources are never released and the original destroy tears the shared backing environment down underneath it. In the collision branch the code destroys changedEnvironment only when id/provider differ ([line

🟡 LOW Replay environment leaks when id assertion fails — packages/agent-provider-testkit/src/provider-conformance.ts

If the replay create returns an environment with a different id/provider, the assert throws inside withEnvironmentCleanup, which destroys only the ORIGINAL environment; the mismatched replay environment is never destroyed. Against real providers (e2b/daytona conformance runs) that leaks a live sandbox until an external reaper fires. The collision branch below (lines 118-124) explicitly destroys a stray changedEnvironment, so the replay branch is inconsistent with the authors' own cleanup discipline elsewhere in the same function. Fix: capture replay, destroy it when replay.id !== environment.id before asserting.

🟡 LOW Replayed environment is never destroyed — packages/agent-provider-testkit/src/provider-conformance.ts

const replay = await provider.create(replayInput) creates a second environment object that is asserted but never passed to any destroy path. withEnvironmentCleanup destroys only the original environment, and the collision branch destroys changedEnvironment only when distinct — so the harness already has a destroy-when-distinct pattern it does not apply here. For helper-wrapped providers (all in-repo providers) replay === environment, so no leak; but the interface contract explicitly allows a provider to 'return or reconstruct' the same environment, and a third-party provider that reconstructs a new object on idempotent replay leaks its resources. Fix: after the replay assertion, if (replay !== environment) await replay.destroy?.() guarded with try/catch (or fold it into cleanup).


tangletools · 2026-08-16T14:15:45Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 47 non-blocking findings — 3859baad

Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 15 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-16T14:15:45Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 9e94f701

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-16T14:16:56Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 3 (3 weak-concern)
Heuristic 0.0s
Duplication 0.1s
Interrogation 434.5s (2 bridge agents)
Total 434.6s

💰 Value — sound-with-nits

Defines and enforces a uniform retry-safe contract for keyed environment creation across the provider interface, built with the repo's existing canonical-digest machinery — a good, in-grain change with two minor durability/retention notes.

  • What it does: Turns the existing CreateAgentEnvironmentInput.idempotencyKey field (already present pre-PR at environment-runtime.ts:625) into a defined contract: same key + canonically equal input must return/reconstruct the same environment; changed input under the same key must reject before any second create effect (contract docs at packages/agent-interface/src/environment-runtime.ts:627-635 and 718-724). It
  • Goals it achieves: Retry safety for the one operation in the provider surface that lacked it. Before this PR the key's semantics were provider-specific and untested: the CLI bridge silently used it as the environment id (index.ts:57), Tangle forwarded it to the sandbox service (tangle-create-options.ts:79), and E2B/ComputeSDK/Daytona callers had no protection at all against a timeout-then-retry creating a duplicate
  • Assessment: Good and coherent. (1) It reuses the repo's established idempotency grain rather than inventing one: digest-verified request identity via canonicalCandidateDigest is the exact pattern already used by workspace fork/checkpoint (workspace-fork.ts:49, workspace-checkpoint.ts:33), run control (runtime-control.ts:69), and interactive sessions (environment-interactive.ts:762) — searched for a prior gene
  • Better / existing approach: none — this is the right approach. Searched for alternatives: (a) no pre-existing coalescing or generic create-idempotency helper exists anywhere in packages/ (grep for pending/coalesce/inFlight found only the new code); (b) the repo's heavier durable model — requestDigest echoed in results plus lookup/created-vs-replayed status used by workspace branching (workspace-fork.ts:232-266) — would requi
  • 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

A coherent, in-grain contract: it formalizes the codebase's existing keyed-idempotency pattern for generic environment create, applies one shared helper uniformly across all five providers, and enforces it through the conformance testkit — with one durability gap on E2B/Daytona.

  • Integration: Fully reachable and already exercised. Every provider's create() routes through createAgentEnvironmentWithIdempotency (tangle-provider.ts:165-167, cli-bridge/src/index.ts, e2b/computesdk/daytona src/index.ts), the testkit conformance suite now always sets a key and asserts replay plus collision rejection on every run (provider-conformance.ts:34-36, 97-130), and the CLI bridge already uses create i
  • Fit with existing patterns: Matches the established grain rather than competing with it. Keyed idempotency with canonical digests already exists for exact processes (exact-process.ts:113-118, server-backed), checkpoint/fork (control-conformance.test.ts:1349-1493), and turns; this PR extends the same contract to generic create and reuses the shared RFC 8785 canonicalizer (canonicalCandidateDigest, environment-runtime.ts:652)
  • Real-world viability: The happy path and the realistic retry paths are both handled: concurrent retries share one pending promise, a failed create deletes its record so the key can be retried, digest collisions reject before any provider effect, and an aborted retry rejects without poisoning the record (environment-runtime.ts:673-707, tested in environment-provider.test.ts). The one limit: the coalescing Map is per pro
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Keyed create is not durable on E2B and Daytona, below the README's contract wording [robustness] ``

The interface README states providers backed by a remote service must forward the key and reconstruct the same environment 'including after an ambiguous provider response', and the helper doc assigns durable retention to the backing service (environment-runtime.ts:668-670). Tangle, ComputeSDK, and CLI Bridge satisfy this; E2B and Daytona keep the key only in the in-process Map (no idempotencyKey appears outside tests in either package), so cross-process retries duplicate environments. In-process

💰 Value Audit

🟡 Durability promise is only as strong as the backing API; E2B and Daytona get in-memory coalescing only [maintenance] ``

The new README contract says remote-backed providers 'must forward the key and retain its canonical input through environment reconstruction' (packages/agent-interface/README.md, added in 3859baa), but E2B (index.ts:64) and Daytona (index.ts:69) apply only the in-memory helper and never forward a key — grep confirms no idempotency reference in their create-param mappers, while Tangle (tangle-create-options.ts:79) and ComputeSDK (index.ts:201) do forward it. For those two providers a retry after

🟡 Idempotency records are never evicted and hold strong environment references [maintenance] ``

createAgentEnvironmentWithIdempotency (environment-runtime.ts:693-707) keeps every successful record — including the full AgentEnvironment object — in the per-adapter Map forever, so destroyed environments stay reachable and the map grows with each distinct key over the provider's lifetime. Retention is what makes replay return the identical environment, so eviction would change semantics; still, a long-lived adapter with many keyed creates accumulates unboundedly. A size bound or weak-ref desig


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.

value-audit · 20260816T151515Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — 9e94f701

Review health 100/100 · Reviewer score 10/100 · Confidence 85/100 · 50 findings (10 medium, 40 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 13 42 10 10
Confidence 85 85 85 85
Correctness 13 42 10 10
Security 13 42 10 10
Testing 13 42 10 10
Architecture 13 42 10 10

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Digest throws for type-valid input with explicit undefined optional fields — packages/agent-interface/src/environment-runtime.ts

agentEnvironmentCreateInputDigest destructures { idempotencyKey, signal, ...material } and hashes material directly via canonicalCandidateDigest. Object rest keeps every own enumerable key, including optional fields explicitly set to undefined (e.g. input built as { profile, name: body.name, env: opts.env } where a field is missing). serializeCanonicalCandidate receives undefined through Object.keys, and isCanonicalJsonValue returns false for undefined, so canonicalCandidateJson throws 'candidate document must be finite, acyclic RFC 8785 JSON' (reproduced: material = { profile: { name: 'worker' }, backend: undefined } throws). CreateAgentEnvironmentInput is typed with optional fields, so this input is type-valid; the crash propagates out of createAgentEnvironmentWithIdempotency before crea

🟠 MEDIUM Keyed create throws on undefined / non-JSON create material — packages/agent-interface/src/environment-runtime.ts

agentEnvironmentCreateInputDigest passes the stripped input material to canonicalCandidateDigest (environment-runtime.ts:652-655), which rejects undefined and non-plain-object/non-finite values: isCanonicalJsonValue returns false for typeof 'undefined' (agent-candidate-schema-common.ts:218-220) and throws 'candidate document must be finite, acyclic RFC 8785 JSON'. CreateAgentEnvironmentInput.metadata/providerOptions are Record<string, unknown> (environment-runtime.ts:625,637) and AgentProfile.extensions is typed Record<string, Record<string, unknown> | undefined> (agent-profile.ts:457), so a provider passing e.g. metadata: { a: undefined } or extensions: { x: undefined } with an idempotencyKey gets an opaque throw before create() runs, even though the create itself would have been valid. T

🟠 MEDIUM Idempotency cache retains destroyed environments and grows unbounded — packages/agent-provider-cli-bridge/src/index.ts

createRecords Map is created per provider instance and never evicted. createAgentEnvironmentWithIdempotency sets record.environment on success and deletes the record only on create failure (environment-runtime.ts:699-706), so a successful keyed create is retained for the provider's lifetime, holding the resolved promise and the environment object. Empirically verified: create({idempotencyKey:'env-destroy'}) -> destroy() -> create(same key) returns the SAME object whose destroyed=true (retained-environment.ts:177) and closePromise is set; status() resolves 'stopped' and stream()/dispatch() throw 'cli-bridge environment is destroyed'. A caller that destroys and re-creates under the same key silently gets a dead environment instead of a fresh one, and every distinct keyed create leaks one env

🟠 MEDIUM E2B create idempotency is process-local only; same key after restart creates a new paid sandbox — packages/agent-provider-e2b/src/index.ts

input.idempotencyKey is never forwarded to the E2B service: createEnvironment (index.ts:50-59) and e2bCreateOptions (index.ts:148-157) only pass template/apiKey/envs/metadata/providerOptions. The create contract (environment-runtime.ts:721-725) requires same key + canonical input to 'return or reconstruct the same environment', and the helper's doc (environment-runtime.ts:667-671) says the backing service must retain the key across adapter reconstruction. Verified against e2b@2.31.0 dist/index.d.ts: the NewSandbox API schema has no idempotencyKey field, so the E2B service cannot dedupe either. Net effect: a create retried after a process restart (adapter reconstruction) bills for and returns a second sandbox with a different environment.id, so cross-restart idempotency is silently broken w

🟠 MEDIUM Keyed create retry after destroy returns a dead environment that reports running — packages/agent-provider-e2b/src/index.ts

createRecords is never evicted when an environment is destroyed: e2bSandboxAsEnvironment (line 85) does not receive the map, and destroy() (lines 140-143) cannot remove the record. Sequence: create({idempotencyKey: K}) -> environment.destroy() -> create({idempotencyKey: K, same input}) returns the memoized killed sandbox, and status() is hardcoded to "running" (line 95), so the

🟠 MEDIUM Abort-driven fire-and-forget delete races service-level idempotency key retention on retry — packages/agent-provider-tangle/src/tangle-provider.ts

When abort wins the race (lines 115-129), the provider background-deletes the late box while the helper deletes the local record; an immediate retry with the same key re-calls client.create with the same idempotencyKey. The helper's contract says the backing service retains the key ('retaining the key across adapter reconstruction', environment-runtime.ts:669), so the retry can be handed back the exact box the background cleanup is deleting, giving the caller a live handle to a terminating sandbox. Fix: when input.idempotencyKey is set and abort wins, skip the delete and defer to service-side key retention (or tombstone the key until t

🟠 MEDIUM Idempotency record cache is never evicted; replay after destroy returns a stale environment — packages/agent-provider-tangle/src/tangle-provider.ts

createRecords (tangle-provider.ts:75-78) is created per provider and only ever written on success (environment-runtime.ts:701) and deleted on failure (environment-runtime.ts:704). environment.destroy() (tangle-environment.ts:410-416) calls box.delete() but nothing removes the record. Two consequences: (a) replaying the same idempotencyKey after destroy returns the cached environment wrapping a deleted box instead of reconstructing via the sandbox service, which the doc comment (environment-runtime.ts:668) claims retains the key — the in-memory cache actively masks that reconstruction; (b) long-lived providers accumulate one record per keyed create forever, pinning the box handle in memory. Evidence: replay path is return existing.environment ?? existing.pending (environment-runtime.ts:69

🟠 MEDIUM createRecords grows without bound and pins created environments for the provider's lifetime — packages/agent-provider-tangle/src/tangle-provider.ts

The Map at lines 75-78 is only ever deleted on create failure (environment-runtime.ts:704); every successful keyed create stores {digest, pending, environment} forever. The record's environment field holds closures over the sandbox instance and client, so a long-lived host process issuing many keyed creates accumulates memory without bound and prevents GC of dead environments. Replays after the caller destroyed the environment also return a stale handle with no invalidation hook. Fix: bound the map (size cap or TTL) or clear a record's environment reference when the underlying box is deleted; at minimum document the retention contract

🟠 MEDIUM Collision check false-passes on any unrelated create error — packages/agent-provider-testkit/src/provider-conformance.ts

The collision probe wraps provider.create({...createInput, name: name+'-changed'}) in try { } catch { collisionRejected = true; } with an empty catch that does not inspect the error. A provider that throws for any reason other than detecting a key/digest collision — e.g. a name-format validator that rejects the '-changed' suffix, a transient backend failure, or an unrelated create bug — marks collisionRejected=true and the assertion 'reusing a create key with changed input must reject' passes. This certifies idempotency without any idempotency implementation. Contrast with the workspace-branching conformance which verifies a typed status === 'conflict'/digest mismatch. Fix: require a distinguishable collision signal (e.g. assert on a typed conflict result or match a collision marker in

🟠 MEDIUM Deterministic default idempotency key makes back-to-back conformance runs ill-defined against persistent providers — packages/agent-provider-testkit/src/provider-conformance.ts

The default key ${options.name}-environment-create is deterministic, and the runner destroys the environment at the end (withEnvironmentCleanup(..., true) at line 199). The contract the runner enforces (environment-runtime.ts:627-634) says the same key with canonically equal input 'must return or reconstruct the same environment', and the helper doc (environment-runtime.ts:669-670) says the backing service retains keys across adapter reconstruction — but nothing defines whether destroy clears the create record. So a second conformance run with the same options.name against the same deployment either gets rejected at first create or

🟡 LOW In-flight coalescing and failure-eviction paths are untested in-repo — packages/agent-interface/src/environment-provider.test.ts

The describe block tests replay only AFTER the first create settles (line 105 awaits first before replay at line 110) and a conflict only after settle (line 123). The doc comment on createAgentEnvironmentWithIdempotency claims 'coalesces concurrent retries', and the failure path (record eviction enabling retry, shared rejectio

🟡 LOW Missing coverage for error-cleanup retry and no-key passthrough — packages/agent-interface/src/environment-provider.test.ts

The new describe block covers canonical-digest equality, key-order independence, same-key coalescing, changed-input rejection, and abort rejection, but does not test: (1) create() throwing → record deleted → a later same-key call re-attempts create (the records.delete path at environment-runtime.ts:704), nor (2) the idempotencyKey === undefined passthrough (environment-runtime.ts:680). Both are the paths most likely to regress silently.

🟡 LOW Missing test coverage for in-flight coalescing, failure-retry, and unkeyed paths — packages/agent-interface/src/environment-provider.test.ts

Tests cover sequential replay, collision rejection, and pre-aborted signal, but not: (1) the return existing.pending coalescing branch under two concurrent in-flight calls (the core concurrency guarantee), (2) the delete-on-failure + successful retry path, (3) the idempotencyKey === undefined path that calls create() directly, or (4) the undefined-optional-field input that currently crashes the digest (agentEnvironmentCreateInputDigest throws, see finding above). The concurrency branch is the main advertised value of the helper and is only exercised in its post-completion form.

🟡 LOW Coalesced retries share the first caller's create closure and signal — packages/agent-interface/src/environment-runtime.ts

create is invoked as create() with no input/signal argument (environment-runtime.ts:693), so the adapter's closure captures the first caller's input (including its AbortSignal). A second concurrent caller with the same key+digest returns existing.pending, meaning if the first caller's signal aborts, the second caller's attempt is also aborted even though its own signal was never aborted. The doc comment ('signal controls one attempt') covers this intent, so it is a semantic nuance rather than a bug, but a provider adapter could silently cancel a client that never cancelled.

🟡 LOW Digest throws on explicit-undefined optional fields with a misleading error — packages/agent-interface/src/environment-runtime.ts

The destructure at line 651 removes only idempotencyKey and signal; any other explicit-undefined optional (e.g. { ...base, name: maybeName } where maybeName is undefined, a common conditional-spread pattern) reaches isCanonicalJsonValue, and Object.entries DOES include undefined-valued keys, so it returns false and canonicalCandidateJson throws 'candidate document must be finite, acyclic RFC 8785 JSON'. Verified by execution. Impact: callers get a hard, confusing failure naming 'candidate document' in an environment-create context; it fails before any create effect so no duplicate-create risk. The sibling nativeContextContinuationRequestDig

🟡 LOW Idempotency records retained for the life of the map on success — packages/agent-interface/src/environment-runtime.ts

On success the record is never removed (records.delete only happens in the catch at environment-runtime.ts:704). record.environment is set and the entry stays in the map so later replays return the same object (the test asserts replay toBe(first)). For a long-lived adapter this is unbounded growth: one retained environment object per unique idempotencyKey. Retention is required by the replay contract, but there is no eviction/TTL. Low impact given the helper is @internal and the map is per-adapter-instance, but worth documenting or bounding.

🟡 LOW Input digest deterministically covers secrets and env values — packages/agent-interface/src/environment-runtime.ts

material includes secrets (string[] | Record<string,string>) and env, so record.digest is a deterministic unsalted SHA-256 over secret material, retained indefinitely in the adapter-owned records map (AgentEnvironmentCreateIdempotencyRecord.digest is public and settled records are never evicted). Identical secret material always yields the same digest, making it a stable correlator and a dictionary-attack target if a digest is ever logged or heap-dumped. Only the digest is stored (no plaintext retention), and nothing in this shot logs it, so this is a hardening note: consider hashing with the idempotencyKey or documenting that digests are secret-derived and must not be logged.

🟡 LOW Key collision surfaced only as untyped Error message text — packages/agent-interface/src/environment-runtime.ts

The same-key/different-input conflict throws a plain Error whose only machine-readable signal is the message string 'agent environment create idempotency key conflicts with a different create input' (line 686-688); the shipped test matches on the regex /conflicts with a different create input/ (environment-provider.test.ts:129). Any caller that needs to distinguish conflict from transport failure must string-match. Fix: attach a stable code property (e.g. 'AGENT_ENVIRONMENT_CREATE_IDEMPOTENCY_CONFLICT') or export a typed error class, keeping the message for display.

🟡 LOW Secret values are folded into an unsalted deterministic digest — packages/agent-interface/src/environment-runtime.ts

material includes secrets (raw values when passed as Record<string,string>) and env values, hashed by canonicalCandidateDigest with no salt or per-record secret beyond the literal 'agent-environment-create.v1' kind string. sha256 preimage resistance protects high-entropy secrets, but low-entropy secret values (short tokens, PIN-like strings) could be brute-forced offline if the digest is persisted anywhere readable (e.g. a backing idempotency table). It also means a retry that supplies a freshly rotated/refreshed secret value hashes differently and is wrongly rejected as an input collision, even though the logical create is identical. Consider excluding secret values (only names) from create identity, or at minimum documenting that the digest must never be treated as a redaction-proof valu

🟡 LOW Settled idempotency records accumulate for the adapter's lifetime with no eviction guidance — packages/agent-interface/src/environment-runtime.ts

On success the record keeps both environment and the settled pending promise (lines 693-701) and is never removed, so a long-lived adapter (tangle-provider holds createRecords for the process lifetime) grows one record per distinct key forever. Retention itself is required for post-settle same-key replay (tested behavior), so this is inherent; but the settled pending reference is redundant once environment is set, and the helper offers no size/TTL hook or doc guidance for adapters that create many keyed environments. Fix: clear pending after settle (or store only the environment), and note an adapter-side eviction policy in the

🟡 LOW records map grows without bound and caches stale environments — packages/agent-interface/src/environment-runtime.ts

Every successful keyed create writes record.environment permanently; nothing ever evicts an entry. A long-running adapter accumulates one cached environment object per distinct key (unbounded memory, and each cache holds a live provider handle that keeps the environment alive). Separately, a later same-key call returns the cached environment even if it was destroyed by the caller, so the adapter can hand back a dead environment instead of recreating. The contract doc says the backing service retains the key, but the in-memory cache has no TTL, size cap, or destroy-invalidation. Consider bounding the cache and validating the cached environment (or clearing the entry on destroy).

🟡 LOW Public index exports helpers documented as @internalpackages/agent-interface/src/index.ts

agentEnvironmentCreateInputDigest and createAgentEnvironmentWithIdempotency are both marked @internal in environment-runtime.ts (lines 646 and 671) but are added to the public barrel export in index.ts. This widens the committed API surface with helpers that may change shape; note this matches existing practice (canonicalCandidateDigest and agentInteractiveSessionRequestDigest are also public), so it is consistent but worth confirming the surface is intentional. Consider exporting from the environment-provider subpath only if these are meant to stay internal.

🟡 LOW Digest runs on raw pre-snapshot input and throws on non-JSON values for keyed creates — packages/agent-provider-cli-bridge/src/index.ts

createAgentEnvironmentWithIdempotency computes agentEnvironmentCreateInputDigest(input) on the raw input before createEnvironment applies snapshotAgentProfile or the string-profile guard. canonicalCandidateDigest -> canonicalCandidateJson -> isCanonicalJsonValue throws ('candidate document must be finite, acyclic RFC 8785 JSON') for any non-canonical value (undefined, NaN, Date, class instances, functions) anywhere in metadata/providerOptions/profile. Previously a keyed create with such metadata was forwarded untouched; now it rejects at digest time. This is fail-closed and only affects keyed creates, but it is a silent behavior change for callers passing e.g. metadata: { x: undefined }. Non-blocking; consider documenting the RFC-8785-JSON requirement on CreateAgentEnvironmentInput for key

🟡 LOW Idempotency records Map grows without bound — packages/agent-provider-cli-bridge/src/index.ts

createRecords never evicts completed entries, and each record holds the full AgentEnvironment (transport, runs, sessions, usageLog). A long-lived provider serving many keyed creates retains every environment handle forever, a slow memory leak. The interface doc (environment-runtime.ts:668-670) defers cross-restart retention to the backing service, but for cli-bridge the Map is the only retention. Impact is bounded because creates are lazy handles, not live resources. Fix: cap the Map or evict destroyed environments; note this same pattern ships in e2b/daytona/computesdk/tangle, so fix in the shared helper if at all.

🟡 LOW Keyed create rejects inputs outside the RFC 8785 canonical domain — packages/agent-provider-cli-bridge/src/index.ts

create() now passes the raw input to agentEnvironmentCreateInputDigest (environment-runtime.ts:648-656), which canonicalizes every non-key/non-signal field via canonicalCandidateDigest -> canonicalCandidateJson. That function throws 'candidate document must be finite, acyclic RFC 8785 JSON' for values like an explicitly-undefined optional field (e.g. metadata:{x:undefined} or env:{PATH:undefined}), a non-finite number, a sparse array, or a class instance. Empirically verified: provider.create({profile:{name:'worker'}, metadata:{x:undefined}, idempotencyKey:'k'}) rejects with that error, while the identical input WITHOUT an idempotencyKey still creates successfully (the digest is only computed on the keyed path, environment-runtime.ts:679-682). Regression: adding an idempotencyKey to an inp

🟡 LOW Keyed re-create after destroy returns the destroyed environment — packages/agent-provider-cli-bridge/src/index.ts

If a caller creates with key K, calls environment.destroy(), then re-invokes create with the same key and identical input, the helper returns the cached destroyed instance (stream/dispatch throw 'cli-bridge environment is destroyed', status reports 'stopped'). The generic contract's 'return or reconstruct the same environment' is arguably satisfied, but the caller has no path to a usable environment under key K without changing input. Originates in the shared helper (environment-runtime.ts:690 returns existing.environment unconditionally), not this adapter; if deemed a defect, fix by clearing the record on destroy or reconstructing when the cached environment is stopped. No test covers destroy-then-replay.

🟡 LOW Unbounded createRecords map retains environments for provider lifetime — packages/agent-provider-cli-bridge/src/index.ts

createRecords (Map<string, AgentEnvironmentCreateIdempotencyRecord>) is never evicted: on a successful keyed create, the record keeps environment set and stays in the map for the provider's lifetime (environment-runtime.ts:700-702 only deletes on error). Each retained AgentEnvironment closes over options, fetch, and capabilities, so a long-lived adapter creating many distinct idempotency keys grows memory without bound. This is identical to the sibling providers (daytona/tangle/e2b/computesdk), so it is not a regression introduced here, and retention is arguably required by the 'return the same environment for the same canonical input' contract. Fix if desired: document the growth bound or add explicit eviction semantics at the helper level, not in this adapter alone.

🟡 LOW Idempotency is in-memory only; adapter restart creates duplicate billable sandboxes — packages/agent-provider-e2b/src/index.ts

e2bCreateOptions never forwards input.idempotencyKey into the E2B create payload (e.g. as sandbox metadata), and the provider implements no list() to reconstruct by key. After process restart or provider reconstruction, the helper's Map is empty, so a same-key retry silently creates a second billable E2B sandbox instead of returning or rejecting — the documented contract ("must return or reconstruct the same environment") is only enforceable within one adapter instance. The helper JSDoc delegates durability to the backing service, which E2B lacks here. Cheapest mitigation: stamp the key into the sandbox metadata now so a later list-based reconstruction is possible, and document the restart limitation.

🟡 LOW New runtime import depends on agent-interface export not yet published — packages/agent-provider-e2b/src/index.ts

index.ts:1 imports createAgentEnvironmentWithIdempotency from @tangle-network/agent-interface/environment-provider, declared as ^1.0.0 in this package's package.json. That symbol is added by this PR and only ships in agent-interface 1.0.1 (the patch changeset bumps all seven packages). If the provider is published before interface 1.0.1 or a consumer's lockfile pins interface 1.0.0, create() fails at runtime with 'createAgentEnvironmentWithIdempotency is not a function'. Changesets config updateInternalDependencies:patch rewrites the provider's range to ^1.0.1 at release time, which mitigates this, but the import is unguarded. Fix: no code change required if release ordering holds; optionally guard the import or bump the declared range to ^1.0.1 in the same PR.

🟡 LOW Unbounded retention of idempotency records and sandbox handles — packages/agent-provider-e2b/src/index.ts

Every successful keyed create stores a record (digest + AgentEnvironment holding the E2B sandbox handle) in the closure Map forever; there is no TTL, cap, or eviction. In a long-lived host process creating many keyed environments, memory grows without bound, including handles to already-destroyed sandboxes. This tension is inherent to the replay contract (eviction silently breaks same-key replay), so the fix belongs at the helper/design level (documented cap with contract note, or eviction tied to destroy as above). Same pattern in all five provider adapters, so not an e2b-only regression.

🟡 LOW createRecords Map is never evicted and returns stale/dead sandboxes on replay — packages/agent-provider-e2b/src/index.ts

createRecords (index.ts:46-49) keeps one record per distinct idempotencyKey forever: each record holds the resolved pending promise and the AgentEnvironment, which pins the live E2BSandboxLike handle. In a long-lived process with many keyed creates this grows without bound (memory leak), and E2B sandboxes are short-lived (default TTL 15s per NewSandbox.timeout), so a same-key replay after the sandbox expired returns the cached environment whose status() hardcodes 'running' (index.ts:95) and whose exec()/stream() will fail against a dead sandbox. Impact: unbounded retention and silent hand-back of dead environments for replayed keys. Fix: evict records on destroy()/kill, on replay verify liveness via get(id) before returning the cached environment, or cap the map.

🟡 LOW createRecords map grows unbounded and retains dead sandbox references — packages/agent-provider-e2b/src/index.ts

createRecords is instantiated once per provider (lines 46-49) and passed to createAgentEnvironmentWithIdempotency. The helper deletes a record only when the create effect rejects (environment-runtime.ts:704); on success it stores environment and never removes it. Each record retains the full AgentEnvironment returned by e2bSandboxAsEnvironment, which closes over the E2B sandbox object. The destroy() method (lines 140-143) kills/closes the sandbox but does not remove the record from the map, so a long-lived pr

🟡 LOW No provider-level test for keyed retry after failure or concurrent same-key coalescing — packages/agent-provider-tangle/src/index.test.ts

The two new tests cover replay identity, key-order canonicalization, conflict rejection without a second create effect, and mapper key-drop rejection. Not covered at the provider wiring level: (1) a failed keyed create (e.g. client.create rejects) allows a same-key retry to issue a fresh create (record-deletion path through the Tangle mapper), and (2) two concurrent same-key creates coalesce to one client.create call. Both are covered for the helper in agent-interface tests but not for this provider's integration, which is where the mapper-preservation guard runs.

🟡 LOW Test coverage gaps for the idempotency contract — packages/agent-provider-tangle/src/index.test.ts

The new tests (index.test.ts:115-164) cover sequential replay, collision rejection, and mapper key-drop, and the conformance suite (provider-conformance.ts:97-131) covers replay equality and collision. Not covered: (a) concurrent same-key creates coalescing while the first is in flight (the critical race the pending-promise design targets); (b) create failure followed by a same-key retry succeeding (record-eviction path, environment-runtime.ts:703-706); (c) destroy-then-replay behavior (which currently returns a stale environment — see the medium finding). Adding these three would pin the correctness claims the helper documents.

🟡 LOW Coalesced replay does not observe the replay caller's abort signal — packages/agent-provider-tangle/src/tangle-provider.ts

On a same-key replay, createAgentEnvironmentWithIdempotency returns existing.pending (environment-runtime.ts:690), which was created from the FIRST caller's closure and signal. The replay caller's signal is checked once at entry (environment-runtime.ts:678) but is not attached to the shared promise; a replay caller that aborts mid-flight still receives the resolved environment. The doc comment says 'signal controls one attempt' (environment-runtime.ts:633) and coalescing is the documented intent, so this is arguably by design — flagging as a documented-behavior gap rather than a defect. Consider aborting the shared await in the helper if the replay caller's signal fires.

🟡 LOW Digest treats defaulted and explicit fields as different create input — packages/agent-provider-tangle/src/tangle-provider.ts

agentEnvironmentCreateInputDigest hashes raw input, so { idempotencyKey, profile } and { idempotencyKey, profile, backend: 'opencode' } produce different digests and the second call is rejected as a collision (environment-runtime.ts:685-689) even though both map to identical create options (tangle-create-options.ts:82 defaults backend). Fail-closed (safe: no wrong-environment reuse), but a caller who retries a key while making the default explicit will get a spurious conflict. Acceptable per the strict-input-equality contract; noted for awareness. Could be fixed by hashing mapped createOptions, but that is a larger contract change.

🟡 LOW Keyed create records never evict; replay returns a destroyed environment without reconstruction — packages/agent-provider-tangle/src/tangle-provider.ts

createRecords (tangle-provider.ts:75-78) retains every successful keyed create's record (digest + resolved environment) for the provider's lifetime with no eviction path. The contract states the provider must 'return or reconstruct' the same environment (environment-runtime.ts:720-724), but create() (tangle-provider.ts:165-171) only returns createAgentEnvironmentWithIdempotency's cached record and never re-fetches via client.get. destroy() (tangle-environment.ts:410-414) calls box.delete() only and does not touch createRecords, so a replay of the same key after destroy returns the now-deleted environment object instead of reconstructing it, and destroy would call box.delete() again. Impact: unbounded memory growth in a long-lived provider and a stale-environment replay after deletion. This

🟡 LOW Keyed create rejects inputs the un-keyed path accepts when an optional field is explicitly undefined — packages/agent-provider-tangle/src/tangle-provider.ts

agentEnvironmentCreateInputDigest (environment-runtime.ts:648-656) spreads every own enumerable field of input, including fields explicitly set to undefined. canonicalCandidateDigest -> isCanonicalJsonValue returns false for undefined (agent-candidate-schema-common.ts:214-247), so canonicalCandidateDigest throws 'candidate document must be finite, acyclic RFC 8785 JSON'. Example: provider.create({ profile, idempotencyKey, name: undefined }) rejects with the RFC8785 error, while the identical input without idempotencyKey succeeds (sandboxOptionsFromCreateInput treats name: undefined as absent, tangle-create-options.ts:26-29). Reachable whenever a caller spreads a partially-populated options object. Fail-closed, so impact is a spurious rejection and a misleading error, not data loss. Fix: om

🟡 LOW Mapper idempotencyKey check is asymmetric when input carries no key — packages/agent-provider-tangle/src/tangle-provider.ts

The preservation check is guarded by 'input.idempotencyKey !== undefined' (tangle-provider.ts:97-104). When the caller supplies no key, a custom mapCreateInput that injects an idempotencyKey into createOptions is not rejected; createAgentEnvironmentWithIdempotency returns create() directly for key===undefined (environment-runtime.ts:680), so the generic layer neither records nor coalesces that key while client.create still receives it. Two unkeyed calls through such a mapper each reach client.create, deferring all dedup to the sandbox service. Not a safety issue (fail-closed is preserved), but a consistency gap worth a test or a matching guard for the injected-key direction.

🟡 LOW Mapper may inject an idempotencyKey the caller never supplied — packages/agent-provider-tangle/src/tangle-provider.ts

The preservation check only fires when input.idempotencyKey !== undefined, so options.mapCreateInput can add its own key when the caller passed none. The service then dedupes creates the caller believes are keyless and independent; on a fresh adapter (records empty) the same keyless input creates a new box, so dedupe is invisible and inconsistent across adapter reconstruction. Low risk since it requires a deliberately mapping host; consider rejecting mapped keys that the caller did not provide, or documenting that mapper-injected keys are host-private.

🟡 LOW No negative test proves the new idempotency checks can fail — packages/agent-provider-testkit/src/index.test.ts

Every fake provider was upgraded to COMPLY with keyed create (index.test.ts:105-128 via createAgentEnvironmentWithIdempotency; control-conformance.test.ts:1541-1550 conformanceCreate). The only assertions on the new behavior are positive: report.checked contains 'create-idempotency'/'create-idempotency-collision' (index.test.ts:23-24). No test feeds the runner a provider that ignores the key (returns a new environment) or accepts changed input, so the runner's two new rejection branches (provider-conformance.ts:101-105, 125-129) are never exercised. Add two rejects-style tests: one provider whose create always builds a fresh environment, one whose create silently returns the existing environment on changed input.

🟡 LOW Collided-environment destroy can mask the rejection assertion — packages/agent-provider-testkit/src/provider-conformance.ts

When the provider incorrectly succeeds on the changed-name create and returns a different environment, await changedEnvironment.destroy?.() runs outside the try/catch. If that destroy throws, the destroy error propagates instead of the intended ProviderConformanceError('reusing a create key with changed input must reject'), masking the actual conformance failure and confusing diagnosis. Wrap the best-effort destroy so cleanup failure cannot suppress the assertion.

🟡 LOW Collision check accepts any rejection as idempotency rejection — packages/agent-provider-testkit/src/provider-conformance.ts

catch { collisionRejected = true; } treats a throw for any reason — validation, rate limit, transient backend error, even a provider bug — as proof that the keyed create rejected changed input. A provider that returns deterministic ids and errors on any second distinct-name create passes create-idempotency-collision without implementing keyed idempotency. Fix: assert on the error (e.g. message matching an idempotency/conflict pattern) or reject the run when the failure is unrelated.

🟡 LOW Collision-path destroy can mask the conformance failure message — packages/agent-provider-testkit/src/provider-conformance.ts

await changedEnvironment.destroy?.() runs before the collisionRejected assert (125-129). If that destroy rejects, the destroy error propagates and the operator never sees 'reusing a create key with changed input must reject' — the actual contract violation is hidden behind a cleanup error. Wrap the destroy in try/catch (or assert first, cleanup after) so the contract failure is the reported one.

🟡 LOW Create idempotency enforced unconditionally with no capability gate — packages/agent-provider-testkit/src/provider-conformance.ts

The harness now always injects idempotencyKey = ${name}-environment-create when absent and asserts idempotent replay/collision, but idempotencyKey is optional in CreateAgentEnvironmentInput and AgentEnvironmentCapabilities declares no create-idempotency flag. Any provider that does not implement keyed create idempotency now fails create-idempotency/create-idempotency-collision with no opt-out. This matches the changeset intent ('Define and enforce canonical idempotency'), so it is a deliberate blanket requirement rather than a bug, but it is a silent behavioral contract change for existing conforming providers and deserves an explicit capability flag or migration note.

🟡 LOW Create idempotency is mandatory and validated only in-process — packages/agent-provider-testkit/src/provider-conformance.ts

The harness unconditionally injects idempotencyKey and hard-fails any provider that does not reject a changed-input collision; ProviderConformanceOptions has no flag to skip the requirement. Additionally the replay/collision checks run against one adapter instance with an in-memory records map, so a provider whose backing service does not retain keys across adapter reconstruction (the durability the PR doc makes the backing service responsible for) still passes. No capability flag and no cross-restart coverage — consider documenting the mandatory contract and, where feasible, testing key retention across a provider get/reconstruct cycle.

🟡 LOW Replay check only reverses top-level key order, under-testing canonicalization — packages/agent-provider-testkit/src/provider-conformance.ts

Object.fromEntries(Object.entries(createInput).reverse()) reorders only the top-level keys; nested material (profile, metadata, workspace, resources, env, secrets, providerOptions) is passed with identical insertion order and identical object references. The digest (agentEnvironmentCreateInputDigest → RFC 8785) canonicalizes recursively, but a provider that canonicalizes top-level keys while comparing nested fields non-canonically (e.g. raw JSON.stringify on nested objects) would not be caught, since the nested order never changes between the original and replay. Low-impact coverage gap; consider shuffling a nested field (e.g. metadata key order) in the replay input to prove full-depth order insensitivity.

🟡 LOW Replay environment leaks when provider violates same-key semantics — packages/agent-provider-testkit/src/provider-conformance.ts

If the replay create resolves to a DIFFERENT environment (contract violation), the assert at 101-105 throws and the replay environment handle is dropped without destroy — only the primary environment is cleaned up by withEnvironmentCleanup. The collision path (lines 118-124) already does defensive destroy of a wrongly-created different environment; the replay path should do the same before asserting.

🟡 LOW Replay-created environment is never destroyed — packages/agent-provider-testkit/src/provider-conformance.ts

const replay = await provider.create(replayInput) may return a distinct environment, but only the first environment is wrapped in withEnvironmentCleanup (line 38) and the collision branch destroys only changedEnvironment when id/provider differ (lines 118-124). A non-compliant provider that returns a new env on the replay orphans it in the backing service — a live, billable sandbox for real providers running this conformance in CI. Fix: `if (replay !== environment) aw

🟡 LOW Undefined optional createInput fields crash the digest — packages/agent-provider-testkit/src/provider-conformance.ts

...(options.createInput ?? {}) spreads a Partial without pruning undefined, and agentEnvironmentCreateInputDigest throws "candidate document must be finite, acyclic RFC 8785 JSON" on any undefined top-level or nested value (verified empirically: {...base, secrets: undefined} and {...base, metadata: {nested: undefined}} both throw). A caller passing createInput: { metadata: undefined } or { providerOptions: undefined } — legal TS — crashes the conformance run with a misleading error instead of a conformance failure. Fix: drop undefined entries when constructing createInput, or make the digest tolerate undefined.


tangletools · 2026-08-16T15:34:55Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 50 non-blocking findings — 9e94f701

Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 16 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-16T15:34:55Z · immutable trace

@drewstone
drewstone merged commit b594b96 into main Aug 17, 2026
1 check passed
@drewstone
drewstone deleted the fix/generic-provider-create-idempotency-20260816 branch August 17, 2026 00:57
@tangletools tangletools mentioned this pull request Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants