chore(sandbox): address post-merge review threads on #14 + #17

[kjgbot]

Follow-up cleanup for the eight unresolved review threads that were still open on #14 and #17 at merge time — four on src/daytona/runtime.ts (from #14) and four on src/port.ts (from #17). All threads are P1/P2 from cubic-dev-ai and chatgpt-codex-connector; no human authors.

Resolution shape: 4 FIX, 4 ACK+RESOLVE.

Closes on #14 (4 threads → 2 FIX, 2 ACK+RESOLVE)

• FIX — src/daytona/runtime.ts line 772 (cubic P2, "bound refreshData with the same lookup deadline") — commit 6b286bc. Wraps the unbounded refreshData() call in recreateAfterFailedStart with awaitLookupOperation(..., lookupDeadline(undefined), ...), matching the same pattern used 5 other places in this file. A stalled refresh no longer wedges restart recovery.
• FIX — src/daytona/runtime.test.ts line 1890 (cubic P2, "delete-after-destroy race in smoke cleanup") — commit 7142b14. Extends the existing isTestDaytonaNotFound guard from the adjacent daytona.get(id) call to the daytona.delete(sandbox) call in the same cleanup loop, so eventually-consistent 404s on delete don't fail the after hook.
• ACK+RESOLVE — src/daytona/runtime.ts line 750 (chatgpt-codex-connector P1, "restrict recreation to verified Toolbox failures"). Prior lead reply on the thread explains: needs error-classification (401/403/429 vs 502/503/504) with SDK-shape tests we don't have; misclassifying dead-daemon as transient is the opposite failure mode and worse; recreateOnFailedStart: false escape hatch already documented. Tracked as a fast-follow for whoever adds the classifier tests.
• ACK+RESOLVE — src/daytona/runtime.ts line 893 (cubic P2, "networkAllowList as array"). Prior lead reply cites Daytona TypeScript SDK docs pinning networkAllowList: string (comma-separated). No array variant exists to preserve.

Closes on #17 (4 threads → 2 FIX (dedup'd), 2 ACK+RESOLVE)

• FIX — src/port.ts line 255 (chatgpt-codex-connector P2 + cubic-dev-ai P2, "keep resolved descriptor source-compatible" / "making modes required breaks TS consumers") — commit 6e166c9. Both threads flag the same source-compat regression. Split into two types: SandboxRuntimeCapabilities (five booleans + modes?: SandboxCapabilityModes, source-compatible again) and ResolvedSandboxRuntimeCapabilities (intersection with modes: SandboxCapabilityModes required) as the resolver's return type. Preserves both the source-compat contract and the always-present-after-resolution invariant. Two new tests pin the contract.
• ACK+RESOLVE — src/port.ts (chatgpt-codex-connector P2, outdated, "encode the adapter-hidden streaming state"). Directly addressed by the second commit of feat(port): structured capability modes alongside the boolean flags #17 which added CapabilityAbsence = "unknown" | "not-exposed" | "unsupported" — "not-exposed" is exactly the vocabulary for "provider has it, this package's ports do not reach it." Thread also isOutdated: true on the current file layout.
• ACK+RESOLVE — src/port.ts line 86 (cubic-dev-ai P2, "add distinct adapter-buffered output mode"). Design decision, not oversight: outputStreams describes observable API behavior (what the caller receives), not underlying provider capability. Adding "adapter-buffered" would push adapters to make claims about the provider's untested capability, which is precisely the "manufacture false confidence" trap the whole modes design is built to avoid. Provider-vs-adapter distinction, if wanted, belongs on a separate axis (a sibling providerCapabilityHints field), not as another member of this union — worth its own PR if it's a first-class concern.

Test discipline

• npm run build && npm test after each commit. Suite: 220 → 222 (+2 tests for the port source-compat contract). 217 pass / 5 skipped / 0 fail on the tip.
• Skip count callout: the +1 test-only change (runtime.test.ts 404 tolerance) lands inside the DAYTONA_API_KEY-gated DaytonaRuntime smoke suite that was already skipping in CI — so skipped count is unchanged (5 before, 5 after) rather than +1 as a naïve diff might imply. This is a coverage-preserving change, not a coverage regression.

Follow-ups tracked (not landed here)

• The P1 error-classification for assertSandboxExecHealthy (from the ACK'd fix(daytona): recover exec after sandbox restart #14 line-750 thread). Deferred because it needs SDK-error-shape tests to avoid misclassifying dead-daemon as transient. Consumers who don't want automatic replacement can already set recreateOnFailedStart: false.
• If a first-class "provider-can-stream, adapter-hides" signal is genuinely needed (from the ACK'd feat(port): structured capability modes alongside the boolean flags #17 line-86 thread), a separate providerCapabilityHints sibling field is the shape to consider rather than expanding OutputStreamMode.

Cross-links

• Original: fix(daytona): recover exec after sandbox restart #14 (fix(daytona): recover exec after sandbox restart), merged 68d8418
• Original: feat(port): structured capability modes alongside the boolean flags #17 (feat(port): structured capability modes alongside the boolean flags), merged 51d7ee9

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d5b6dd78-f272-485a-99de-4e90f8912e31

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kjgbot]

sandbox-lead review pass — PASS

Human read confirming safe to surface. This is the post-merge thread-cleanup PR for the already-merged #14 and #17.

• Every FIX claim in the body verified true:
  • 6b286bc — refreshData() now wrapped in awaitLookupOperation(..., lookupDeadline(undefined), ...), matches the pattern used 5 other places in src/daytona/runtime.ts. Optional-chained call, no regression risk.
  • 7142b14 — daytona.delete(sandbox) in smoke cleanup now guarded by the existing isTestDaytonaNotFound helper, matching the adjacent get() call. Bounded to DAYTONA_API_KEY-gated smoke.
  • 6e166c9 — SandboxRuntimeCapabilities.modes is now optional; ResolvedSandboxRuntimeCapabilities = SandboxRuntimeCapabilities & { readonly modes: SandboxCapabilityModes } is new; resolver return type updated; WeakMap cache updated; ResolvedSandboxRuntimeCapabilities exported from index.ts.
• ACK+RESOLVE decisions defensible:
  • fix(daytona): recover exec after sandbox restart #14:750 (error classification) — sound deferral; recreateOnFailedStart: false escape hatch is documented, misclassifying dead-daemon as transient is the opposite failure mode.
  • fix(daytona): recover exec after sandbox restart #14:893 (networkAllowList as array) — Daytona SDK docs pin the field to comma-separated string, no array variant exists to preserve.
  • feat(port): structured capability modes alongside the boolean flags #17:86 (outputStreams: adapter-buffered) — correct architectural decision. Outputstreams describes observable API behavior (what caller sees), not underlying provider capability; adding adapter-buffered would push adapters to manufacture claims about capabilities they haven't tested.
  • feat(port): structured capability modes alongside the boolean flags #17 encode adapter-hidden streaming state — satisfied by CapabilityAbsence = "unknown" | "not-exposed" | "unsupported" already in the merged feat(port): structured capability modes alongside the boolean flags #17.
• Source-compat contract pinned: exactly 2 new tests in src/port.test.ts (lines 124–152) — one constructs pre-modes fixture (5 booleans only), one reads resolver output into strict type without cast. Old fixture constructors continue to compile; new callers get compiler enforcement of modes presence.

Safe.