Skip to content

feat(acp): bridge runtime permission requests - #2201

Closed
simple-agent-manager[bot] wants to merge 13 commits into
mainfrom
sam/execute-task-using-skill-zafwb6
Closed

simple-agent-manager[bot] wants to merge 13 commits into
mainfrom
sam/execute-task-using-skill-zafwb6

Conversation

@simple-agent-manager

@simple-agent-manager simple-agent-manager Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ACP agents now route permission requests through the durable Cloudflare interaction authority instead of broadcasting raw requests or choosing the first option. The runtime creates bounded encrypted detail, waits for an exact generation-bound answer, and settles cancellation, expiry, process loss, connection replacement, or Stop without a browser-to-runtime response path.

This coordinator follow-up closes three runtime races:

  • Instant capability and answer forwarding now uses ctx.container.getTcpPort(...).fetch(...) after persisted and actual-running checks. It never calls the pinned Containers 0.3.7 containerFetch() helper, which can auto-start compute.
  • Permission create and answer waiting are owned by the exact outgoing prompt attempt and run under the same bounded deadline/lifecycle. Ambiguous create acknowledgements no longer discard an answer that already won.
  • Settle retries a temporary 404 within its existing bounded budget, covering the ordering where cancellation settles before a slow create commits.

The real pinned acp-go-sdk@v0.13.5 fixture proves its inbound permission context lacks the outgoing prompt deadline, then proves attempt-owned deadline/cancel settlement while the same connection remains usable. Global enablement remains false; forms and URL elicitation are not advertised.

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm build
  • Focused API no-wake, callback, delivery, guard, and cross-boundary tests: 68 passed
  • Previously load-sensitive API files rerun in isolation: 76 passed
  • go test ./...
  • go vet ./...
  • go test -race ./internal/acp ./internal/server
  • Focused permission lifecycle and pinned-SDK race tests: 5 repetitions passed after the final fix
  • Real SDK fixture: inbound permission context had no deadline; prompt deadline settled expired; explicit cancel settled wrapper_cancelled; Initialize and a later prompt succeeded; stale attempt cancellation could not claim the later waiter
  • Full local pnpm test exercised 11,079 API tests; three unrelated load-sensitive failures occurred in two files, and both files then passed all 76 tests in isolation
  • Remote PR checks passed, including Test, Build, Lint, Type Check, Durable Object Workers, VM Agent Test/Integration/E2E, smoke tests, SonarCloud, and secret scan
  • Candidate-selection volume/cost statement — N/A: no sweep, cron, alarm, or candidate selection changed

Staging Verification (REQUIRED for all code changes — merge-blocking)

  • Staging deployment green — explicitly delegated to the coordinator
  • Live app verified via Playwright — explicitly delegated to the coordinator
  • Existing workflows confirmed working — explicitly delegated to the coordinator
  • New feature/fix verified on staging — coordinator owns the integrated permission roundtrip
  • Infrastructure verification completed — coordinator will verify VM and Instant transports
  • Mobile and desktop verification notes added for UI changes — N/A: no UI changes

Staging Verification Evidence

Raphaël explicitly instructed this delegated follow-up not to deploy or mutate shared staging. Staging remains required before readiness or merge and is owned by the coordinator.

UI Compliance Checklist (Required for UI changes)

N/A: no files under apps/web or packages/acp-client changed; UI PR #2200 was untouched.

UI Screenshot Evidence

N/A: no UI surface changed.

End-to-End Verification (Required for multi-component changes)

  • Data flow traced from input to final outcome
  • Real pinned-SDK fixture exercises Prompt → RequestPermission → durable HTTP create/settle and connection reuse
  • Worker boundary tests cover capability/answer delivery and exact no-wake behavior
  • Deployed Worker↔runtime roundtrip — explicitly delegated to coordinator staging

Data Flow Trace

  1. Worker start delivery builds and serializes the centralized acpInteractions contract for VM and Instant runtimes.
  2. VM start delivery validates and durably claims the contract; each ACP connection attachment receives a fresh UUID generation.
  3. The active outgoing Prompt attempt owns permission lifetime because the SDK inbound callback context is connection-scoped.
  4. RequestPermission registers the attempt/generation waiter before starting bounded durable create. Only a definitive create rejection cancels immediately; ambiguous acknowledgement keeps waiting for a potentially committed answer.
  5. Cloudflare verifies callback JWT identity and stores bounded detail encrypted; the creator-only answer route remains the sole answer authority.
  6. VM answer handling verifies workspace, session, runtime, generation, interaction, and exact option identity. Receipts distinguish consumed, duplicate, conflict, stale generation, and no waiter.
  7. Instant delivery checks persisted plus actual runtime state and forwards through the direct container TCP port. A stopped runtime returns 410; a stop at forwarding returns 503 for capability or 409 for answer without start/recovery.
  8. Cancellation/expiry/loss/Stop settle independently. A temporary settle 404 is retried because the slow create may commit after the first settle attempt.

Untested Gaps

The deployed Worker↔VM/Instant roundtrip has not run because this delegated task forbids shared staging mutation. Coordinator staging should run the deterministic permission-reversed fixture, duplicate/conflicting answers, Stop/process loss, and stopped/sleeping no-wake checks.

Post-Mortem (Required for bug fix PRs)

The original no-wake guard trusted persisted lifecycle state but then called an SDK helper whose internal health check could start a stopped container. The permission path also executed create synchronously and assumed the inbound SDK callback inherited the outgoing Prompt context. The fix uses a direct non-starting port primitive, explicit prompt-attempt ownership, and ambiguity-aware create/settle ordering tests.

Specialist Review Evidence (Required for agent-authored PRs)

  • All scoped follow-up reviewers completed and findings were addressed
Reviewer Status Outcome
go-specialist / test-engineer ADDRESSED → PASS Found settle-before-create 404 ordering gap; 7e6030174 retries within the bounded settle context and adds the exact race regression
cloudflare-specialist PASS Direct TCP port path contains no start/recovery call; stale-state and stop-race behavior covered for capability and answer
security-auditor PASS Cloudflare sole authority, encrypted detail, structural logs, exact-option validation, JWT/runtime/generation fencing preserved
constitution-validator PASS No new fixed operational timeout, limit, URL, identifier, secret, or production flag
task-completion-validator / doc-sync-validator PASS Follow-up findings, checklist, acceptance criteria, diff, tests, and contract prose align; staging remains an explicit coordinator deferral

Residual risk: a deliberately noncompliant injected Go RoundTripper can retain its own goroutine if it ignores both request context and client timeout forever. The production HTTP client is bounded and context-aware. Direct-port behavior is unit-tested against the documented Cloudflare primitive; deployed confirmation remains with coordinator staging.

CodeRabbit Review Evidence (Required for agent-authored PRs)

  • CodeRabbit requested after readiness — deferred because the PR must remain draft

CodeRabbit Notes

The current check reports review skipped by label configuration. The coordinator owns readiness and any later review gate.

Exceptions (If any)

  • Scope: integrated staging only
  • Rationale: Raphaël explicitly directed this delegated follow-up not to deploy or mutate shared staging
  • Expiration: before this PR is marked ready or merged

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

  • ACP permission request/response protocol documentation
  • Pinned github.com/coder/acp-go-sdk@v0.13.5 source and executable fixture
  • Pinned @cloudflare/containers@0.3.7 source plus the direct Durable Object container port contract

Codebase Impact Analysis

  • packages/vm-agent: prompt-attempt ownership, bounded create/wait, ambiguous acknowledgement, settle ordering, receipts, and real SDK fixture
  • apps/api: direct-port Instant no-wake forwarding and capability/answer race coverage
  • specs/001-mvp/contracts/api.md: exact attempt ownership, ambiguity, and direct-port delivery contract
  • No UI changes

Documentation & Specs

  • specs/001-mvp/contracts/api.md
  • tasks/active/2026-09-30-acp-runtime-permission-bridge.md

Constitution & Risk Check

The follow-up reuses the centralized configurable deadlines, response bounds, and retry schedule. No rollout flag changed. Identity remains fenced by workspace, agent session, execution runtime, connection generation, interaction ID, and exact option ID.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • coderabbit-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: raphaeltm/simple-agent-manager/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: da88ace5-290e-41d1-b094-35dfbfdca7fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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

@codspeed

codspeed Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing sam/execute-task-using-skill-zafwb6 (96df904) with main (762a97c)

Open in CodSpeed

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Coordinator review of d7439c9: changes required before integrated staging/readiness.

  1. High — no-wake proxy still calls an auto-starting SDK method (apps/api/src/durable-objects/vm-agent-container.ts:173-181). proxyHttpNoWake checks persisted lifecycleStatus then calls containerFetch. In pinned @cloudflare/containers 0.3.7, containerFetch calls startAndWaitForPorts if the actual container is not running OR SDK state is not healthy. Persisted running can lag a crash, and the container can exit after the check. I executed the exact npm 0.3.7 method with persisted running / actual stopped: autoStartCalls=1. Current tests replace containerFetch with a successful mock and miss this behavior. Use a forwarding primitive that cannot start compute, including under a stop/crash race; do not merely add another precheck before containerFetch. Cover stale persisted running, stop between checking and forwarding, and both capability/answer requests. Official semantics: https://developers.cloudflare.com/containers/api/container-class/ .

  2. Medium — create transport is outside the permission deadline/lifecycle wait (packages/vm-agent/internal/acp/session_host_interactions.go:439-456). The computed permission deadline only creates a timer after synchronous createAcpInteraction(ctx, request) returns. A slow create response can hold RequestPermission past its own deadline (default transport timeout is 30 seconds, injected clients need not have a timeout); Stop/replacement queues a waiter cancellation but does not interrupt that HTTP call. Use a request lifecycle context/deadline that covers create as well as waiting, and arbitrate an answer arriving before a lost create acknowledgement consistently. Add delayed-create tests for expiry, Stop/replacement, and response loss after durable creation, with exact durable outcome/receipt assertions.

Additional integration concern to investigate in the same follow-up: permissionDeadline examines the inbound SDK callback context, but pinned acp-go-sdk v0.13.5 derives inbound request contexts from its independent background connection context. That context does not inherit the outgoing prompt deadline. Prove with the real ACP fixture that prompt deadline/cancel closes the matching permission promptly while the connection remains alive; bind it to the correct prompt attempt if needed, without allowing an old cancel to affect a new attempt.

This is source/SDK review with an executed SDK reproduction, not a rerun of the full repository suite. CI passing is acknowledged. Draft/no-merge/global-disabled constraints remain; coordinator-owned integrated permission-reversed staging is still required after fixes.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Coordinator source re-review of 96df904:

The two blocking findings and the SDK prompt-lifetime concern are addressed in the reviewed diff:

  • No-wake forwards through ctx.container.getTcpPort(...).fetch and never calls the auto-starting SDK wrapper. Regressions cover stale persisted-running/actual-stopped and stop-at-forwarding for capability and answer calls.
  • Create and answer waiting share a deadline/cancellation context; waiter arbitration cancels in-flight create and preserves an exact consumed answer when the create acknowledgement is lost.
  • Permissions inherit the outgoing prompt attempt lifetime. The real acp-go-sdk pipe fixture explicitly observes an inbound context without the prompt deadline, proves expiry/cancel settlement while the connection survives, and protects the next attempt from stale cancellation.
  • Settle now retries 404 within the bounded budget, with a test forcing first-settle-before-create-commit ordering.

No additional blocking source finding in this pass. I inspected the regression implementations; I did not rerun the suites locally. Final-head CI is currently running. This is not readiness/merge approval: final CI, integrated permission-reversed VM/Instant staging, and remaining review gates are outstanding. Keep draft and global enable false.

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Coordinator follow-up fixes are pushed to the existing branch at 96df904f7 (no force push). The PR remains draft; staging, readiness, merge, and production flags were not touched.

Review findings addressed:

  • Instant capability and answer delivery now use ctx.container.getTcpPort(...).fetch(...), never Containers 0.3.7 containerFetch(). Tests cover persisted-running/actual-stopped and stop-at-forward for both request shapes with exact 410/503/409 results and zero start calls.
  • Permission create and waiting are bound to the exact Prompt attempt, deadline, Stop/process loss, and connection generation. Tests cover delayed-create expiry (expired/no_waiter), Stop (session_stopped/no_waiter), replacement (connection_replaced/stale_generation), and answer-before-lost-ack (completed, then duplicate).
  • Specialist review found an additional settle-before-create ordering: the first settle can see 404 before a slow create commits. 7e6030174 retries that 404 inside the existing bounded settle context, and its regression forces first-settle 404 → create commit → second-settle success with the exact session_stopped reason.
  • The real pinned acp-go-sdk@v0.13.5 fixture observes that inbound RequestPermission has no Prompt deadline, then proves Prompt deadline settles expired, explicit cancel settles wrapper_cancelled, the same connection still handles Initialize and another Prompt, and stale cancellation cannot claim the newer attempt.

Validation at handoff:

  • pnpm lint, pnpm typecheck, and pnpm build passed.
  • Focused API boundary suite: 68 passed; isolated rerun of the two load-sensitive full-suite files: 76 passed.
  • go test ./..., go vet ./..., and go test -race ./internal/acp ./internal/server passed after the final fix.
  • Focused final permission/SDK race tests passed 5 repetitions.
  • Specialist reviews: Go/test ADDRESSED → PASS; Cloudflare/security/constitution PASS; task-completion/docs PASS after evidence refresh.
  • The prior pushed-head CI run was fully green. New-head CI is being observed; integrated staging remains delegated to the parent as requested.

@sonarqubecloud

Copy link
Copy Markdown

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Final-head validation for coordinator review follow-up

  • Head: 96df904f7f847b84a2cb86df98439a19634a8612
  • Final-head GitHub checks: all applicable checks passed; path-filtered checks skipped as expected. CodeRabbit reported Review skipped: excluded by label configuration; it was not retriggered because this PR remains draft and readiness is parent-owned.
  • Local: focused API no-wake/delivery suite (68 tests), isolated load-sensitive API files (76 tests), pnpm lint, pnpm typecheck, pnpm build, go test ./..., go vet ./..., and go test -race ./internal/acp ./internal/server passed.
  • Real pinned acp-go-sdk v0.13.5 fixture proved that inbound RequestPermission context does not inherit the outgoing prompt deadline. The attempt-owned prompt context now settles deadline/cancel correctly while the connection remains usable, and stale cancellation cannot affect a newer attempt.
  • Full local pnpm test ran 11,079 API tests with three load-sensitive timeout failures in two files; both files passed all 76 tests in isolation, and the final-head remote Test job passed.

PR remains draft. No staging mutation, deployment, readiness change, production flag change, force push, or merge was performed. Integrated staging remains explicitly delegated to the parent.

@simple-agent-manager simple-agent-manager Bot mentioned this pull request Sep 30, 2026
18 of 19 tasks
@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Superseded by integration PR #2202, which includes this exact reviewed runtime head 96df904. #2202 merged as 86e6c5b and production deployment 36744720219 succeeded. Permission creation remains disabled pending the separately reviewed activation rollout.

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.

1 participant