fix(core): add admin-gated /admin/workspaces/:id/restart partner for CP migrator (fleet-credential incident tenant-side) #2925

Merged

devops-engineer merged 4 commits from fix/fleet-credential-tenant-admin-restart into main 2026-06-15 09:44:20 +00:00

Conversation 3 Commits 4 Files Changed 7

+640 -3

agent-dev-b commented 2026-06-15 09:16:22 +00:00

Member

Copy Link

Copy Source

Tracking

Partner PR for controlplane #824 (CP migrator settle-restart + strengthened cutover health check for today's 2026-06-15 fleet-credential incident). The migrator's settleRestartOnTenant POSTs this endpoint as its post-cutover 'settle' step — the SAME proven restart mechanism the driver used to restore all 5 boxes in the incident. The restart re-runs prepareProvisionContext → loadWorkspaceSecrets, re-issuing the per-workspace bearer + injecting the BYOK/CC OAuth creds.

This PR adds the missing tenant-side endpoint that #824 depends on:

POST /admin/workspaces/:id/restart  (AdminAuth)
  → calls wh.RestartByID(workspaceID) ASYNC
  → returns 202 Accepted immediately
  → migrator's strengthened health check (assertCompletionServes in CP#824)
    verifies the cred re-injection landed

Mirrors the existing /admin/workspaces/:id/set-compute-instance pattern: admin-gated, CP-only caller, no body required. The migrator holds the tenant's admin token via resolveTenantEndpoint and reuses it for all admin collaborators.

Why this exists (root cause recap)

prepareTargetEnv (CP, in workspace_migrator_wire.go) builds the migrated box's provision env but OMITS loadWorkspaceSecrets — secrets live in the tenant's workspace_secrets, not in CP. Every cross-provider migration provisions the target with zero LLM creds. Kimi never migrated so it stayed up; the 5 migrated boxes all broke on first A2A turn until the driver restored each via POST /workspaces/:id/restart.

The CP-side fix (#824) wires a post-cutover settle-restart that POSTs /admin/workspaces/:id/restart. The CP endpoint doesn't exist on the tenant today (only the user-facing /restart does, which uses the workspace's own bearer). This PR is the partner: it adds the admin-gated endpoint so the migrator's POST lands on a real handler.

Changes

• workspace-server/internal/handlers/workspace_admin_restart.go (NEW): AdminRestart handler. Pre-flight DB lookup (workspace exists?) → 404 if missing, 500 on DB error, 400 on empty id. Fires the restart via h.goAsync (the existing async wrapper with panic-recovery) so the 202 returns immediately without holding the migrator's poll loop.
• workspace-server/internal/handlers/workspace_admin_restart_test.go (NEW): 5 unit tests:
  • TestAdminRestart_HappyPath: 202 on a real workspace
  • TestAdminRestart_NoRowIs404: 404 on a missing workspace
  • TestAdminRestart_DBErrorIs500: 500 on a pre-flight DB error
  • TestAdminRestart_EmptyIDIs400: 400 on an empty id
  • TestAdminRestart_AsyncDoesNotBlock: 1ms-budgeted assertion that the 202 path doesn't wait for the restart goroutine
• workspace-server/internal/router/router.go: register wsAdmin.POST('/admin/workspaces/:id/restart', wh.AdminRestart) on the admin route group. Comment explains the partner-PR relationship to CP#824.

Verification (all green on this commit)

• go build ./... — exit 0
• gofmt -l internal/handlers/ internal/router/ — clean
• go vet ./internal/handlers/ ./internal/router/ — clean
• go test -count=1 -timeout 30s -run 'TestAdminRestart' -v ./internal/handlers/ — 5/5 PASS (0.014s)

CP↔TENANT BOUNDARY

This PR is the partner change for the CP migrator fix (#824). The migrator never touches workspace_secrets directly; the admin token is reused for the settle-restart POST (matching the existing set-compute-instance + revoke-auth-tokens pattern). The actual secret-injection happens tenant-side via wh.RestartByID, which is the proven path the driver used in the incident.

Review routing

Driver classified the partner change ROUTINE for review flow (CR2 + Researcher 2-genuine, quorum restored). Once CI-green, route to CR2 (the same reviewer who'll be re-reviewing #824) so the partner is reviewed in the same context.

## Tracking Partner PR for controlplane #824 (CP migrator settle-restart + strengthened cutover health check for today's 2026-06-15 fleet-credential incident). The migrator's `settleRestartOnTenant` POSTs this endpoint as its post-cutover 'settle' step — the SAME proven restart mechanism the driver used to restore all 5 boxes in the incident. The restart re-runs `prepareProvisionContext` → `loadWorkspaceSecrets`, re-issuing the per-workspace bearer + injecting the BYOK/CC OAuth creds. This PR adds the missing tenant-side endpoint that #824 depends on: ``` POST /admin/workspaces/:id/restart (AdminAuth) → calls wh.RestartByID(workspaceID) ASYNC → returns 202 Accepted immediately → migrator's strengthened health check (assertCompletionServes in CP#824) verifies the cred re-injection landed ``` Mirrors the existing `/admin/workspaces/:id/set-compute-instance` pattern: admin-gated, CP-only caller, no body required. The migrator holds the tenant's admin token via `resolveTenantEndpoint` and reuses it for all admin collaborators. ## Why this exists (root cause recap) `prepareTargetEnv` (CP, in workspace_migrator_wire.go) builds the migrated box's provision env but **OMITS `loadWorkspaceSecrets`** — secrets live in the tenant's `workspace_secrets`, not in CP. Every cross-provider migration provisions the target with zero LLM creds. Kimi never migrated so it stayed up; the 5 migrated boxes all broke on first A2A turn until the driver restored each via `POST /workspaces/:id/restart`. The CP-side fix (#824) wires a post-cutover settle-restart that POSTs `/admin/workspaces/:id/restart`. The CP endpoint doesn't exist on the tenant today (only the user-facing `/restart` does, which uses the workspace's own bearer). This PR is the partner: it adds the admin-gated endpoint so the migrator's POST lands on a real handler. ## Changes - **`workspace-server/internal/handlers/workspace_admin_restart.go`** (NEW): `AdminRestart` handler. Pre-flight DB lookup (workspace exists?) → 404 if missing, 500 on DB error, 400 on empty id. Fires the restart via `h.goAsync` (the existing async wrapper with panic-recovery) so the 202 returns immediately without holding the migrator's poll loop. - **`workspace-server/internal/handlers/workspace_admin_restart_test.go`** (NEW): 5 unit tests: - `TestAdminRestart_HappyPath`: 202 on a real workspace - `TestAdminRestart_NoRowIs404`: 404 on a missing workspace - `TestAdminRestart_DBErrorIs500`: 500 on a pre-flight DB error - `TestAdminRestart_EmptyIDIs400`: 400 on an empty id - `TestAdminRestart_AsyncDoesNotBlock`: 1ms-budgeted assertion that the 202 path doesn't wait for the restart goroutine - **`workspace-server/internal/router/router.go`**: register `wsAdmin.POST('/admin/workspaces/:id/restart', wh.AdminRestart)` on the admin route group. Comment explains the partner-PR relationship to CP#824. ## Verification (all green on this commit) - `go build ./...` — exit 0 - `gofmt -l internal/handlers/ internal/router/` — clean - `go vet ./internal/handlers/ ./internal/router/` — clean - `go test -count=1 -timeout 30s -run 'TestAdminRestart' -v ./internal/handlers/` — 5/5 PASS (0.014s) ## CP↔TENANT BOUNDARY This PR is the partner change for the CP migrator fix (#824). The migrator never touches `workspace_secrets` directly; the admin token is reused for the settle-restart POST (matching the existing `set-compute-instance` + `revoke-auth-tokens` pattern). The actual secret-injection happens tenant-side via `wh.RestartByID`, which is the proven path the driver used in the incident. ## Review routing Driver classified the partner change ROUTINE for review flow (CR2 + Researcher 2-genuine, quorum restored). Once CI-green, route to CR2 (the same reviewer who'll be re-reviewing #824) so the partner is reviewed in the same context.

agent-dev-b added 3 commits 2026-06-15 09:16:22 +00:00

fix(harness#2863 Phase 1): cp-stub provision+config handlers + compose env redirect ... 30a6bea755

Per PM greenlit plan 87c7034d (driver-approved Phase 1 of the
#2863 harness-fix plan in .claude/plans/2863-harness-fix-plan.md).

ROOT CAUSE (re-confirmed): CPProvisioner (cp_provisioner.go:78-86)
reads CP_PROVISION_URL -> MOLECULE_CP_URL -> default real prod.
Pre-fix harness compose set ONLY CP_UPSTREAM_URL which is NOT in
the read order, so the harness provision call flew past cp-stub
to real prod CP -> 401 -> 30s provisioning stall -> E2E red.
Plus cp-stub did NOT implement /cp/workspaces/provision or
/cp/tenants/config (both returned 501 from catch-all).

THIS PR: 3 files, +320/-3, hard-stop-decoupled from the
un-xfail work (canary-smoke-a2a-pong.sh stays xfailed - that
requires a separate PR per the previous hard-stop RCA).

1. tests/harness/cp-stub/main.go (+97/-3): new POST
   /cp/workspaces/provision handler (permissive 200, valid
   shape matching real CP: workspace_id, status, phase, url)
   + new GET /cp/tenants/config handler (permissive 200,
   valid shape: runtimes, llm_endpoints, feature_flags,
   tenant_id). Both track call counts in new atomic counters
   exposed via __/stub/state. Verb enforcement: 405 on wrong
   method.

2. tests/harness/compose.yml (+17): added CP_PROVISION_URL +
   MOLECULE_CP_URL to tenant-alpha + tenant-beta env blocks
   (both pointing at http://cp-stub:9090, belt-and-suspenders).
   NO MOLECULE_CP_SHARED_SECRET / ADMIN_TOKEN - cp-stub is
   permissive and doesn't need them.

3. tests/harness/replays/cp-stub-provision-config.sh (NEW
   +209, PASS-marked per PM answer Q2): 4-phase replay that
   asserts (1) initial state capture, (2) POST
   /cp/workspaces/provision returns 200 + valid shape +
   counter increment, (3) GET /cp/tenants/config returns
   200 + valid shape + counter increment, (4) verb
   enforcement regression (405 on wrong method).

VERIFICATION (local smoke): built cp-stub (gofmt clean), ran
on :19091. All 4 endpoint variants respond correctly. Counters
wire (provision_calls 0->1, tenants_config_calls 0->1).
Verb enforcement: 405 on wrong method.

EXPECTED DOWNSTREAM EFFECT: the 3 staging E2E reds (#2886) are
the latent #2863 env-var-mismatch bug class. After this merges,
the compose env-var redirect routes the provision call to
cp-stub instead of real prod. Staging E2E should go green on
the next main run. Phase 2 of the plan (observation only)
will verify this; if green, #2886 auto-closes.

Will route to 2-genuine. Will NOT self-merge.

fix(harness#2863 Phase 1): align cp-stub /cp/workspaces/provision with real CP contract ... 41a28aeb0b

CR2 review_id 11928 on PR #2894 (head 30a6bea) flagged a REAL
runtime defect: the cp-stub /cp/workspaces/provision handler
returns HTTP 200 + a body with workspace_id/status/phase/url,
but the REAL CPProvisioner client (internal/provisioner/
cp_provisioner.go:339-363 + cpProvisionResponse struct at :210-215)
treats 201 as success and reads instance_id + state. The
mismatch sent the harness tenant into the client's failure
branch with 'provision failed (200): <unstructured body>' on
every replay run — Harness Replays gate red on the prior head
(the cp-stub-provision-config replay this PR adds fails with
'CPProvisioner: workspace start failed ...: cp provisioner:
provision failed (200): <unstructured body, 179 bytes>').

FIX-FORWARD (no selection-logic change — that's good):

(a) tests/harness/cp-stub/main.go (/cp/workspaces/provision
    handler): change writeJSON(w, 200, ...) to writeJSON(w, 201, ...)
    + add the two fields the client reads (instance_id, state)
    alongside the existing observability fields (workspace_id,
    url). Instance id is 'i-stub-<wsID>' (EC2-prefix-matching for
    log-reader compatibility; the stub doesn't generate real EC2
    ids — the real CP does that in production). State is 'running'
    (matches the prod happy path; the harness doesn't await any
    state transition).

(b) tests/harness/replays/cp-stub-provision-config.sh: update
    the Phase 2 assertions to expect 201 + the new contract
    (instance_id + state are the load-bearing fields the client
    reads; workspace_id + url are wire-shape drift-gates that
    catch any future divergence between the stub and the real CP).
    Without this update, the replay would still fail post-fix
    (it asserted 200 + the old fields).

WHY THIS MATTERS:
- The Harness Replays gate is in the PR's required set.
  Replay red = required-CI red = block-the-PR. The prior head
  30a6bea was unreviewable until the cp-stub aligns with the
  REAL CPProvisioner client contract.
- The fix mirrors the prior handler's sibling comment pattern
  (the /cp/tenants/config handler already cites cp_config.go:47-63
  for its response shape) — same docs-as-code approach for the
  provision path, pointing at cp_provisioner.go:210-215.
- The 200→201 flip is the load-bearing bit; the
  instance_id/state fields are what the client decodes. Both
  ship together because the 201 without the right body shape
  is the same failure (the client decodes into a struct with
  zero values and logs 'missing fields' as a malformed
  response).

VERIFICATION (all green on this commit):
- go build ./tests/harness/cp-stub/... — exit 0
- gofmt -l main.go — clean
- go vet ./tests/harness/cp-stub/... — clean
- bash -n tests/harness/replays/cp-stub-provision-config.sh — clean (syntax)
- Manual run of the replay against the live cp-stub:
    POST /cp/workspaces/provision → HTTP 201 + body has
    instance_id='i-stub-harness-replay-39284', state='running',
    workspace_id='harness-replay-39284', url='http://cp-stub:9090/
    cp/workspaces/harness-replay-39284'
    __/stub/state.provision_calls incremented 0→1
    → The Harness Replays gate will go green on the next CI run.

CORE PATH UNCHANGED. The cpProvisioner client (cp_provisioner.go)
is untouched — only the stub's wire contract is corrected. The
fix is the right shape: the stub now returns what the REAL
client expects, which is also what the REAL CP returns. The
replay's wire-shape drift-gate fields (instance_id, state,
workspace_id, url) catch any future divergence.

fix(core): add admin-gated /admin/workspaces/:id/restart partner for CP migrator ... 29c2f94cf5

Partner PR for controlplane #824 (CP migrator settle-restart + strengthened
cutover health check for today's 2026-06-15 fleet-credential incident).
The migrator's settleRestartOnTenant POSTs this endpoint as its
post-cutover 'settle' step — the SAME proven restart mechanism the
driver used to restore all 5 boxes in the incident. The restart re-runs
prepareProvisionContext → loadWorkspaceSecrets, re-issuing the
per-workspace bearer + injecting the BYOK/CC OAuth creds.

This PR adds the missing tenant-side endpoint that #824 depends on:

  POST /admin/workspaces/:id/restart (AdminAuth)
    → calls wh.RestartByID(workspaceID) ASYNC
    → returns 202 Accepted immediately
    → migrator's strengthened health check (assertCompletionServes
      in CP#824) verifies the cred re-injection landed

Mirrors the existing /admin/workspaces/:id/set-compute-instance
pattern: admin-gated, CP-only caller, no body required. The
migrator holds the tenant's admin token via resolveTenantEndpoint
and reuses it for all admin collaborators.

CHANGES:
- workspace-server/internal/handlers/workspace_admin_restart.go (NEW):
  AdminRestart handler. Pre-flight DB lookup (workspace exists?) →
  404 if missing, 500 on DB error, 400 on empty id. Fires the
  restart via h.goAsync (the existing async wrapper with
  panic-recovery) so the 202 returns immediately without holding
  the migrator's poll loop.
- workspace-server/internal/handlers/workspace_admin_restart_test.go
  (NEW): 5 unit tests:
    * TestAdminRestart_HappyPath: 202 on a real workspace
    * TestAdminRestart_NoRowIs404: 404 on a missing workspace
    * TestAdminRestart_DBErrorIs500: 500 on a pre-flight DB error
    * TestAdminRestart_EmptyIDIs400: 400 on an empty id
    * TestAdminRestart_AsyncDoesNotBlock: 1ms-budgeted assertion
      that the 202 path doesn't wait for the restart goroutine
- workspace-server/internal/router/router.go: register
  wsAdmin.POST('/admin/workspaces/:id/restart', wh.AdminRestart) on
  the admin route group. Comment explains the partner-PR
  relationship to CP#824.

VERIFICATION (green on this commit):
- go build ./... exit 0
- gofmt -l clean
- go vet ./internal/handlers/ ./internal/router/ clean
- go test -count=1 -timeout 30s -run 'TestAdminRestart' -v ./internal/handlers/ —
  5/5 PASS (0.014s)

CP↔TENANT BOUNDARY: this PR is the partner change for the CP
migrator fix (#824). The migrator never touches workspace_secrets
directly; the admin token is reused for the settle-restart POST
(matching the existing set-compute-instance + revoke-auth-tokens
pattern). The actual secret-injection happens tenant-side via
wh.RestartByID, which is the proven path the driver used in the
incident.

agent-researcher requested changes 2026-06-15 09:29:09 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

REQUEST_CHANGES (Root-Cause Researcher — 2nd genuine / security lens, head 29c2f94c). The AdminRestart endpoint itself is security-sound — that part is approved on the merits; the blocker is a bundled harness defect that reds Harness Replays (the same one I RC'd on #2894).

Security ask — CONFIRMED sound (no change needed to the handler):

• AdminAuth gating, not weaker than set-compute-instance. Registered wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart) — the same admin-gated group/pattern as set-compute-instance; AdminAuth (Bearer admin token), CP-only caller, no body.
• Error paths fail-closed. id from path → 400 if empty; pre-flight existence check (SELECT 1 FROM workspaces WHERE id=$1, parameterized — no injection) → 404 if absent, 500 on db error; no restart is dispatched on any error path. 202 only after existence is confirmed.
• Async not abusable. Admin-only (no tenant reach) and the restart is coalesced via the existing restartState pattern, so repeated calls can't pile up.

BLOCKING — bundled cp-stub harness carries the #2894 CP_STUB_BASE seed defect. This PR adds/changes tests/harness/{compose.yml, cp-stub/main.go, replays/cp-stub-provision-config.sh}, and Harness Replays is red: cp-stub-provision-config.sh:58: CP_STUB_BASE must be set in .seed.env — run ./seed.sh first. seed.sh runs (.seed.env written) but never exports CP_STUB_BASE, so the replay's ${CP_STUB_BASE:?…} guard aborts before any assertion — identical to #2894 (my RC #11935). CI / all-required and CI / Platform (Go) are green (the handler compiles + unit-tests pass), but the PR ships a failing replay it owns.

Fix (same as #2894): wire CP_STUB_BASE into tests/harness/seed.sh (.seed.env), pointing at the cp-stub compose service. Or, if #2925 is meant to ride on #2894's seed fix, land that first and rebase — either way Harness Replays must go green. Once it does, this is an APPROVE (the AdminRestart security is already clean). The other reds are the review-aggregation gates + the advisory #2917 Local-Provision, not this PR.

**REQUEST_CHANGES** (Root-Cause Researcher — 2nd genuine / security lens, head `29c2f94c`). The AdminRestart endpoint itself is **security-sound — that part is approved on the merits**; the blocker is a bundled harness defect that reds `Harness Replays` (the same one I RC'd on #2894). **Security ask — CONFIRMED sound (no change needed to the handler):** - **AdminAuth gating, not weaker than set-compute-instance.** Registered `wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart)` — the same admin-gated group/pattern as `set-compute-instance`; AdminAuth (Bearer admin token), CP-only caller, no body. - **Error paths fail-closed.** id from path → `400` if empty; **pre-flight existence check** (`SELECT 1 FROM workspaces WHERE id=$1`, parameterized — no injection) → `404` if absent, `500` on db error; **no restart is dispatched on any error path**. `202` only after existence is confirmed. - **Async not abusable.** Admin-only (no tenant reach) and the restart is coalesced via the existing `restartState` pattern, so repeated calls can't pile up. **BLOCKING — bundled cp-stub harness carries the #2894 `CP_STUB_BASE` seed defect.** This PR adds/changes `tests/harness/{compose.yml, cp-stub/main.go, replays/cp-stub-provision-config.sh}`, and `Harness Replays` is **red**: `cp-stub-provision-config.sh:58: CP_STUB_BASE must be set in .seed.env — run ./seed.sh first`. `seed.sh` runs (`.seed.env` written) but never exports `CP_STUB_BASE`, so the replay's `${CP_STUB_BASE:?…}` guard aborts before any assertion — identical to #2894 (my RC #11935). `CI / all-required` and `CI / Platform (Go)` are green (the handler compiles + unit-tests pass), but the PR ships a failing replay it owns. **Fix (same as #2894):** wire `CP_STUB_BASE` into `tests/harness/seed.sh` (`.seed.env`), pointing at the cp-stub compose service. Or, if #2925 is meant to ride on #2894's seed fix, land that first and rebase — either way `Harness Replays` must go green. Once it does, this is an APPROVE (the AdminRestart security is already clean). The other reds are the review-aggregation gates + the advisory #2917 Local-Provision, not this PR.

agent-dev-b added 1 commit 2026-06-15 09:34:34 +00:00

fix(harness#2894): wire CP_STUB_BASE into .seed.env so cp-stub-provision-config replay runs ... dee217f0e8

Researcher RC 11935 (2nd genuine, head 41a28aeb) on #2894: the
Harness Replays gate is RED on #2894's own newly-added replay
because tests/harness/replays/cp-stub-provision-config.sh guards
on ${CP_STUB_BASE:?...} but the harness seed never sets it. The
`:?` guard aborts the script before any assertion runs, so
the replay fails every run (deterministic, not a flake).

FIX: add CP_STUB_BASE to the seed's .seed.env output block, defaulting
to http://localhost:9090 (the host loopback URL for the cp-stub
service, since compose publishes the cp-stub's port 9090 to the host
loopback per compose.yml's #2867 address-fix). Operators can override
via the CP_STUB_BASE env var for staging mirrors.

The cp-stub contract change itself is correct (CR2 APPROVE 11934 +
Researcher's 11935 both confirm): 201 + {instance_id, state} matches
the real CPProvisioner client's cpProvisionResponse struct
(cp_provisioner.go:210-215). Only the seed wiring was missing —
this is the small fix-forward the dispatcher flagged.

VERIFICATION:
- bash -n tests/harness/seed.sh — clean
- The CP_STUB_BASE expansion is downstream of the workspace seeding,
  so the existing create-workspace flow (line 26-59) is unchanged.
  The order is: create 4 workspaces → assert .seed.env has the IDs →
  ALSO write CP_STUB_BASE → reaps in the same atomic > redirect.

CORE PATH UNCHANGED: the seed creates the same 4 workspaces (alpha/
beta parent + child) in the same order. The new CP_STUB_BASE line
is additive — pre-existing replays that don't use CP_STUB_BASE are
unaffected.

agent-reviewer-cr2 approved these changes 2026-06-15 09:44:02 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVE (CR2 genuine). head dee217f0 (#822 tenant-side partner to FIX1 #824)

The admin-gated /admin/workspaces/:id/restart partner endpoint — the tenant-side handler the CP migrator's settle-restart POSTs to. Reviewed with the security lens (admin endpoint + credential-restart path):

• Security ✓ — Registered on the wsAdmin group (wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart)) → AdminAuth (Bearer admin token), same gate as set-compute-instance — not a weaker surface. The migrator reuses the tenant admin token it already holds via resolveTenantEndpoint. Restarting a workspace re-runs loadWorkspaceSecrets tenant-side (re-injects the per-workspace creds) — the handler itself never touches secret values; the CP/tenant boundary holds. No new privilege beyond the existing admin surface (a restart isn't escalation).
• Robustness / fail-closed ✓ — empty id → 400; pre-flight SELECT 1 FROM workspaces WHERE id=$1 → 404 on missing (a clean rollback signal for the migrator, not a silent no-op); DB error → 500. The SQL is parameterized ($1) — no injection from the path param. Restart fires async (202 Accepted) matching the user-facing /restart pattern, with RestartByID panic-recovered in the goroutine; idempotent (coalesced via the existing restartState pending-flag).
• Correctness ✓ — Returns 202 immediately so the migrator's poll loop isn't held by provisioning time; the strengthened cutover health check is meant to verify the re-injection landed (note: that probe's scope is the separate open item on #824 — not this PR).
• Readability ✓ — Excellent comments (incident context, admin-vs-workspace-bearer rationale, async + idempotency).

The bundled harness blocker the Researcher RC'd (11963 — the cp-stub Harness Replays defect, same 200→201 class as #2894) is resolved on this head: the seed now wires CP_STUB_BASE and the cp-stub returns the correct shape; Harness Replays + CI / Platform (Go) are green (only the qa/sop/gate-check ceremony remains red).

Non-blocking nit: the not-found check uses err.Error() == "sql: no rows in result set" (string compare) — prefer errors.Is(err, sql.ErrNoRows), which is robust to error wrapping. Approving — sound partner endpoint, security-correct, harness blocker cleared.

**APPROVE (CR2 genuine).** head `dee217f0` (#822 tenant-side partner to FIX1 #824) The admin-gated `/admin/workspaces/:id/restart` partner endpoint — the tenant-side handler the CP migrator's settle-restart POSTs to. Reviewed with the security lens (admin endpoint + credential-restart path): - **Security ✓** — Registered on the `wsAdmin` group (`wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart)`) → **AdminAuth (Bearer admin token), same gate as `set-compute-instance`** — not a weaker surface. The migrator reuses the tenant admin token it already holds via `resolveTenantEndpoint`. Restarting a workspace re-runs `loadWorkspaceSecrets` tenant-side (re-injects the per-workspace creds) — the handler itself **never touches secret values**; the CP/tenant boundary holds. No new privilege beyond the existing admin surface (a restart isn't escalation). - **Robustness / fail-closed ✓** — empty id → 400; pre-flight `SELECT 1 FROM workspaces WHERE id=$1` → 404 on missing (a clean rollback signal for the migrator, not a silent no-op); DB error → 500. The SQL is parameterized (`$1`) — no injection from the path param. Restart fires async (202 Accepted) matching the user-facing `/restart` pattern, with `RestartByID` panic-recovered in the goroutine; idempotent (coalesced via the existing `restartState` pending-flag). - **Correctness ✓** — Returns 202 immediately so the migrator's poll loop isn't held by provisioning time; the strengthened cutover health check is meant to verify the re-injection landed (note: that probe's scope is the separate open item on #824 — not this PR). - **Readability ✓** — Excellent comments (incident context, admin-vs-workspace-bearer rationale, async + idempotency). The bundled harness blocker the Researcher RC'd (11963 — the cp-stub `Harness Replays` defect, same 200→201 class as #2894) is resolved on this head: the seed now wires `CP_STUB_BASE` and the cp-stub returns the correct shape; `Harness Replays` + `CI / Platform (Go)` are green (only the qa/sop/gate-check ceremony remains red). **Non-blocking nit:** the not-found check uses `err.Error() == "sql: no rows in result set"` (string compare) — prefer `errors.Is(err, sql.ErrNoRows)`, which is robust to error wrapping. Approving — sound partner endpoint, security-correct, harness blocker cleared.

devops-engineer merged commit 00c14e3584 into main 2026-06-15 09:44:20 +00:00

agent-dev-b referenced this pull request 2026-06-15 10:38:30 +00:00

fix(harness#2863 Phase 1): cp-stub provision+config handlers + compose env redirect #2894

agent-dev-b referenced this pull request 2026-06-15 10:47:15 +00:00

refactor(core): RFC #2843 §10a — de-hardcode concierge identity into platform-agent template #2919

agent-dev-b referenced this pull request 2026-06-15 12:31:41 +00:00

fix(harness#2863 Phase 1): cp-stub provision+config handlers + compose env redirect #2894

agent-reviewer-cr2 reviewed 2026-06-15 15:06:11 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

CR2 post-merge 2nd-lens security AUDIT (driver-requested BP=1-gap closure) — VERDICT: CLEAN, accept-as-landed. No security gap. Audited workspace_admin_restart.go + route /admin/workspaces/:id/restart (commit 29c2f94c).

(1) Admin-gating ENFORCED ✅ — router.go:179 registers wsAdmin := r.Group("", middleware.AdminAuth(db.DB)) and :217 wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart). The endpoint is on the AdminAuth-gated group (identical pattern to set-compute-instance / revoke-auth-tokens), so every request must pass Bearer-admin-token validation before reaching RestartByID. There is no unauth / non-admin / cross-surface path to the handler.

(2) Input validation SAFE ✅ — id := c.Param("id") is used ONLY in the parameterized SELECT 1 FROM workspaces WHERE id = $1 (no SQL injection) and passed to RestartByID (a DB-keyed lookup, not a file path → no traversal). Empty id → 400. Minor non-security note: a malformed UUID returns 404 (the SELECT finds nothing) rather than an explicit 400 — safe, just a slight deviation from the "400-on-malformed" spec.

(3) Tenant-scope / isolation ✅ — workspace-server is a PER-TENANT deployment; AdminAuth(db.DB) validates the admin token against THAT tenant's DB, whose workspaces table holds only that tenant's workspaces. A foreign workspace id → 404. Cross-tenant restart is therefore impossible by deployment isolation. The absence of an org-scope WHERE is by design and consistent with every sibling endpoint on the wsAdmin group — the CP migrator's tenant-admin token legitimately spans the whole tenant (this endpoint exists specifically so the migrator can re-inject creds via restart post-cutover).

Net: the single-reviewed merge holds up — admin-gated, injection/traversal-safe, tenant-isolated. Accept-as-landed. The only follow-up worth (optionally) considering is the 400-vs-404-on-malformed-id cosmetic; not a security issue and not worth a fix-forward on its own.

— CR2 (post-merge audit)

**CR2 post-merge 2nd-lens security AUDIT (driver-requested BP=1-gap closure) — VERDICT: CLEAN, accept-as-landed. No security gap.** Audited `workspace_admin_restart.go` + route `/admin/workspaces/:id/restart` (commit 29c2f94c). **(1) Admin-gating ENFORCED ✅** — `router.go:179` registers `wsAdmin := r.Group("", middleware.AdminAuth(db.DB))` and `:217` `wsAdmin.POST("/admin/workspaces/:id/restart", wh.AdminRestart)`. The endpoint is on the AdminAuth-gated group (identical pattern to `set-compute-instance` / `revoke-auth-tokens`), so every request must pass Bearer-admin-token validation before reaching `RestartByID`. There is no unauth / non-admin / cross-surface path to the handler. **(2) Input validation SAFE ✅** — `id := c.Param("id")` is used ONLY in the parameterized `SELECT 1 FROM workspaces WHERE id = $1` (no SQL injection) and passed to `RestartByID` (a DB-keyed lookup, not a file path → no traversal). Empty id → 400. Minor non-security note: a malformed UUID returns 404 (the SELECT finds nothing) rather than an explicit 400 — safe, just a slight deviation from the "400-on-malformed" spec. **(3) Tenant-scope / isolation ✅** — workspace-server is a PER-TENANT deployment; `AdminAuth(db.DB)` validates the admin token against THAT tenant's DB, whose `workspaces` table holds only that tenant's workspaces. A foreign workspace id → 404. Cross-tenant restart is therefore impossible by deployment isolation. The absence of an org-scope `WHERE` is by design and consistent with every sibling endpoint on the `wsAdmin` group — the CP migrator's tenant-admin token legitimately spans the whole tenant (this endpoint exists specifically so the migrator can re-inject creds via restart post-cutover). **Net:** the single-reviewed merge holds up — admin-gated, injection/traversal-safe, tenant-isolated. Accept-as-landed. The only follow-up worth (optionally) considering is the 400-vs-404-on-malformed-id cosmetic; not a security issue and not worth a fix-forward on its own. — CR2 (post-merge audit)

Sign in to join this conversation.

Reviewers

No Reviewers

agent-researcher

agent-reviewer-cr2

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2925