Skip to content

test(docker-model): admit workspace setup revisions for unprotected execution - #3801

Merged
freeqaz merged 1 commit into
mainfrom
fix/docker-model-execution-admission
Oct 27, 2026
Merged

freeqaz merged 1 commit into
mainfrom
fix/docker-model-execution-admission

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Problem

The docker-model lane (Full Integration, dispatch-only, so pull-request CI never runs it) fails on main. Both Docker workspace setup tests stop before Docker does any work:

Missing or unsupported durable execution admission. (at assertExecutionAdmission (packages/contracts/src/credential-proxy.ts:483:11))

Since the Credential Proxy change (#2487), every Compute Driver refuses a revision without a durable executionAdmission; Docker Compute calls assertExecutionReleaseAllowed in prepareRevision. These two tests build their AgentRevision by hand and never got the field.

Change

Give the hand-built revision the admission OCC records for Docker Compute, { version: 2, mode: "unprotected" } (Docker Compute refuses protected execution), as the other hand-built revision fixtures do (tests/helpers/kubernetes-compute-real.mjs, compute-singleton-worker.mjs). Test-only; no product change.

Not changed here (the lane stays red after this PR; each item is separate):

  • The Compose Agent journey ("Docker Compose development drives Docker Compute networks, containers, auth, cleanup, and real model turns") fails with 409 Deployment requires an explicit Harness authentication binding. That is the documented support boundary, not fixture drift: OCC requires a Harness authentication binding at deployment and Docker Compute rejects every binding (Docker Compute; testing guide: "Selected Agent cases currently fail at admission").
  • Since feat: add Gateway managed mode to Agent deployments #2982 moved the OpenClaw pin, the embedded test's replacement Gateway (after stopRevision) waits for the previous Gateway's state lock and exits: Docker Compute force-removes the old Gateway, so its lock is never released. feat(compute): isolate Docker Gateway state and verify ownership before recovery #3355 works in this area.
  • The dedicated test needs a real model key: the Codex Harness model probe must succeed before the Harness is ready.

Verification

The lane is dispatch-only, so it ran with the CI lane steps (prepare.mjs, run-tests.mjs run docker-model, cleanup.mjs) on a remote test VM with Docker 29.9.2 and a dummy model key:

  • main (110aea3, without this change): the cleanup test passes; both workspace setup tests fail with the admission refusal above; the journey fails with the 409. This is also the mutant: the change is the one added field.
  • this branch on main (110aea3): both workspace setup tests get past admission. The embedded test initializes the workspace, verifies the files and fails later at the Gateway replacement (lock, see above); the dedicated test's Harness fails its model probe with the dummy key (HTTP 401).
  • this change on the commit before feat: add Gateway managed mode to Agent deployments #2982 (aaf04ff), 3 lane runs: the embedded workspace setup test and the cleanup test pass in each run (3/3); the dedicated test fails at the model probe (dummy key) and the journey at the 409.

Risks

Low. Test-only, one fixture field. The tests still drive the real Docker Compute Driver against real containers.

…xecution

The Credential Proxy change (#2487) made every Compute Driver refuse a
revision without a durable execution admission. The Docker workspace setup
tests build their revisions by hand and had none, so both failed with
'Missing or unsupported durable execution admission' before Docker did any
work. Give them the unprotected admission OCC records for Docker Compute,
as the other hand-built revision fixtures do.
@freeqaz
freeqaz requested a review from a team as a code owner October 26, 2026 23:59
@clawsweeper

clawsweeper Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 27, 2026
@clawsweeper

clawsweeper Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR adds the required unprotected execution admission to the shared Docker workspace setup revision fixture.

Example: Run the Docker embedded workspace setup case.

  • Before: prepareRevision rejects the fixture with “Missing or unsupported durable execution admission.”
  • After: The fixture supplies { version: 2, mode: "unprotected" }, allowing execution to reach workspace initialization.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The minimal patch is source-verified and correct; supplied validation reports partial real-Docker execution rather than a completed current-baseline workflow.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored, test-only repair is outside the ordinary contributor proof gate; its detailed Docker report supports the narrow admission repair without claiming a passing model lane. The prepared screenshot concerns the separate managed-mode change and provides no evidence for this fixture.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Maintainers cannot exercise Docker workspace initialization because stale fixture revisions fail admission first.
Reason: This maintainer-authored repair restores existing test intent using the current contract and makes no new product choice.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

Keep open: this PR makes a focused, correct fixture repair that current main still needs, with no introduced defects found.

Priority: P2
Reviewed head: c733bff4dc317306bf6e5d46241f182635e66ba8

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The reported current-baseline runs establish progress past admission, but neither workspace case completed end to end because of later runtime or credential failures.
Agent review details

How this fits together

The integration suite constructs Agent revisions and passes them directly to DockerComputeDriver, which validates admission before creating runtime containers and initializing workspace files.

flowchart LR
  A[Workspace integration cases] --> B[Shared revision fixture]
  B --> C[Docker Compute admission validation]
  C --> D[Workspace initializer]
  D --> E[Runtime containers]
  E --> F[File and lifecycle assertions]
Loading

Technical review

Best possible solution:

Land the fixture admission field while keeping unrelated Docker runtime and authentication repairs separate.

Do we have a high-confidence way to reproduce the issue?

Current-main source establishes the failure directly: both cases omit executionAdmission and call a Driver that rejects its absence; the PR reports matching before-and-after Docker results, but this review ran no tests.

Is this the best way to solve the issue?

The added value matches OCC admission and existing revision fixtures, repairs both cases through their shared owner, and preserves all workspace and fail-closed assertions.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 36fb7180e5d1.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Pinned change: The introduced diff adds one admission field and two explanatory comment lines to the fixture shared by embedded and dedicated cases; replacement revisions inherit the field. (tests/integration/docker-compute-real.test.mjs:1737, c733bff4dc31)
  • Current-main failure path: Current main still omits admission from this fixture, while Docker prepareRevision calls assertExecutionReleaseAllowed before runtime work. (apps/controller/src/drivers/compute/docker/index.ts:602, 36fb7180e5d1)
  • Admission matches the existing owner: OCC assigns version 2 unprotected admission when no egress plan exists; the contracts reject missing admission, and Docker rejects protected execution. (packages/occ/src/index.ts:10188, c733bff4dc31)
  • Verification scope: The full PR body reports admission failures before the change and progress beyond admission afterward on Docker 29.9.2; it explicitly distinguishes later Gateway-lock and dummy-key failures from this repair. (c733bff4dc31)
  • Responsible admission change: The merged Credential Proxy change added assertExecutionReleaseAllowed to Docker prepareRevision: Credential Proxy Implementation #2487. (apps/controller/src/drivers/compute/docker/index.ts, 31156790238d)
  • Policy and inspection limits: Read the root AGENTS.md; no nested test instructions, maintainer notes directory, or pr-simplify skill were present. Some historical production blobs were unavailable locally; the admission introduction was independently verified through GitHub. The checkout remained clean, and no tests or review helpers ran. (AGENTS.md, c733bff4dc31)

Likely related people:

  • kevinlin-openai: Raw commit cee299c adds tests/integration/docker-compute-real.test.mjs:1683 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: cee299ce8147; files: tests/integration/docker-compute-real.test.mjs)
  • freeqaz: Raw commit aecbefd adds tests/integration/docker-compute-real.test.mjs:21 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: aecbefdff7af; files: tests/integration/docker-compute-real.test.mjs)

Review metrics

Metric Value Why it matters
Patch scope Production +0; tests +3, -0 One fixture field restores admission for two existing cases without adding machinery or weakening assertions.

Labels

Label changes:

  • add P2: Restores meaningful execution of two selected Docker integration cases with no production behavior change.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Restores meaningful execution of two selected Docker integration cases with no production behavior change.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 26, 2026, 8:12 PM ET / October 27, 2026, 00:12 UTC.

@freeqaz
freeqaz merged commit 09e9cb6 into main Oct 27, 2026
74 checks passed
@freeqaz
freeqaz deleted the fix/docker-model-execution-admission branch October 27, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant