Skip to content

fix(sandbox): route nested workspace status discovery safely - #4496

Merged
yannickmonney merged 1 commit into
mainfrom
fix/workspace-status-routing
Oct 7, 2026
Merged

yannickmonney merged 1 commit into
mainfrom
fix/workspace-status-routing

Conversation

@yannickmonney

Copy link
Copy Markdown
Contributor

Summary

An agent that nested workspace_status inside workspace_tool reached the domain-tool grant gate and received a misleading not_granted response, even while its task tools remained usable. Route that exact nested discovery name through the existing authenticated status endpoint, with the same session credential, timeout and empty body as the dedicated MCP tool. Make the dedicated MCP invocation explicit in agent guidance; keep ordinary execution grants unchanged.

Checklist

  • bun run check passes: 49/49 tasks, 38 cache hits; affected checks executed.
  • bun run lint:sast passes with no findings; the full scanner reports 81 partially analyzed files and its configured/large-file skips.
  • Translations: N/A — internal machine guidance and routing; no UI message key or localized surface changed.
  • Docs: the existing automated manual-reference row documents the compatibility and security controls; end-user docs are unchanged.
  • READMEs: N/A — no installation, configuration, environment or user command changed.

Test plan

  • The actual relay's new regression failed before the fix: nested status reached execution instead of discovery. Afterward, all 9 relay tests pass, including forged-argument stripping, 401 preservation without fallback, normal task execution, ungranted-task refusal, exact-name matching and missing-name refusal.
  • All 50 focused backend guidance/dispatch tests pass, including a refused direct execution request followed by a granted task_get call under the same token scope. Existing status-door tests preserve refused-session responses and server-owned grants.
  • No production worker recovery is claimed by local tests. After release/deployment, verify the nested status call followed by an already-granted task_get in one live worker session.

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Independent review (Part B, API-only) of PR #4496 at d19af1d876152241972b26f9258cd9a570890b5f: REQUEST CHANGES — one blocking CICD finding; no source change requested

  • Reviewer: agent #4 (Review B) 5f5307c9-4cbd-416a-a456-8bb667fc584d, run b4dd405a-5597-4bd9-a017-4b4655cf39f3, card TALE-1143 (c49f756e-1190-4869-8d85-d5bf430c9c65). Routed by Fleet coordination #1, run ac2294c3.
  • Implementer: the PR names no agent, task or issue. Commit d19af1d8 was authored at 16:47:06Z by the shared push account. I did not write it:
    • my only in-progress card is this review;
    • none of my 18 in-review cards concerns this change;
    • this run started at 17:26:22Z, after the commit.
  • Source:
    • PR base 4e5a70b6, merge-base 27084814.
    • One commit, tree 248aac356bead8b7d878c28b8252e00d7e668cfb, 6 files, +179/−14.
    • Read through compare and contents?ref= at the head and at the merge-base.
    • No clone and no local execution. CI evidence comes from this exact head's check-runs, annotations, job logs and turbo artifacts.

Findings

  1. [BLOCKING · CICD] CI did not execute at this head.
    • 20 jobs carry the annotation "The job was not started because it repeatedly failed to be acquired (5 attempts)". They are NOT_RUN, not product failures:
      • Checks: Format, Lint, Type check, Build, Browser, UI 2/4–4/4, Performance, Backend integration.
      • CLI: Build (linux), Build (linux-arm64).
      • Build: Storybook, plus Build platform, proxy, db, sandbox-runtime, sandbox-egress, sandbox-llm-gateway and sandbox-buildkitd.
    • Only 2 of the 7 aggregators are green:
      • CI ready (Checks) has been queued without a runner since 17:00:47Z.
      • CI ready (Build), CI ready (SAST) and CI ready (E2E) were never created (table below).
    • As a result, type-aware Lint and Type check never ran on the changed tests. The sandbox-runtime image that ships the changed bridge (services/sandbox-runtime/Dockerfile:610) was never built.
    • No source change is requested. Re-run the unacquired jobs once at this head; none of them executed, so this is not a rerun of a real failure. Then confirm that all 7 CI ready (…) checks succeed.
    • Main is now at 0bc2cca3, 6 commits past the merge-base. The only file both sides touch is tests/manual/reference/automation.md, in a different row, and GitHub reports the PR mergeable. A fresh run also covers the moved base.
  2. [non-blocking · CODE] Two server refusals still say a bare "Call workspace_status …".
    • The two sentences are in backend/domains/sandbox/dispatch-routes.ts:117-118 (the not_granted guidance) and backend/core/node_only/sandbox/workspace_tools_bridge.ts:613 (the unknown-tool refusal).
    • A model reads them right after a workspace_tool refusal, which is the path that produced the nested call.
    • With this bridge the nested call now works, so they are harmless. But they diverge from the contract row's "Guidance names the separate workspace_status MCP tool with {}". During image skew they also keep steering old bridges into not_granted.
    • Optional follow-up: align both sentences.
  3. [non-blocking · CODE/tests] The "same timeout" claim is not pinned.
    • The status mock answers at once. A mutant that gives the nested branch LONG_REQUEST_TIMEOUT_MS (5 s in tests, 300 s in production) would therefore survive.
    • Optional follow-up: make the status mock slower than TALE_CONNECTORS_REQUEST_TIMEOUT_MS=200, then expect the timed out after 200ms isError result for both the nested and the dedicated name.

Part B

Can nested discovery be pointed outside the sandbox or at another tenant? No.

  • Fixed target. The new branch (tale-connectors-mcp:264-273) matches args.tool === 'workspace_status' exactly and always calls callPlatform('/status', {}, TOOLS_BASE_URL):
    • URL: TOOLS_BASE_URL is derived once from TALE_CONNECTORS_URL (:31-38); the path is a constant.
    • Credential: only the session TALE_CONNECTORS_TOKEN (:44), with no gateway-key fallback.
    • Body: the constant {}.
  • No model input in the request. No model value reaches the URL, the body or the headers.
    • A traversal-style name such as workspace_status/../execute does not match. It stays a JSON body value on the execute grant gate.
    • The bridge does no filesystem I/O on this path (env and stdin only), so path traversal and symlinks have nothing to act on.
  • Tenant binding is server-side. /status (dispatch-routes.ts:145-157) resolves the org and grants from the bearer token's sha256 row (:52-77). It reads nothing else from the request.
    • The new bridge test sends forged toolGrants, organizationId and platform.version. It asserts that the door received exactly {} with Bearer session-key, and that no execute call was made.
  • No new information. workspace_status is already listed to every session, and the door answers any live token with that token's own grants.
  • Discovery stays out of grants. normalizeToolGrants strips workspace_status (tool_names test). Execute still refuses it, while a granted task_get works under the same token (SBX-R6).

Failure handling, timeouts and resource bounds

  • New, tested:
    • A 401 is relayed as isError: true, with no execute fallback.
    • A missing name is still refused locally, with no request.
    • Near-miss names such as 'workspace_status ' stay on the grant gate.
  • Existing shared callPlatform behaviour:
    • A non-JSON response is replaced with a sanitized message.
    • A timeout uses AbortSignal.timeout(REQUEST_TIMEOUT_MS): 30 s by default, the same as the dedicated tool.
    • An unconfigured bridge fails closed.
  • Resources:
    • One request per call, as before.
    • No progress timer on this path, and no new process, file or cache.
    • The request body is 2 bytes.
    • Reading the response without a cap is pre-existing behaviour for every bridge call; the response is a small server-generated JSON.

Consumers of the changed contract

  • tools/list descriptions for workspace_tool and workspace_status: the existing description test is green.
  • Guidance strings KNOWLEDGE_TOOLS_GUIDANCE, imageGenerationGuidance and grantedToolsGuidance: text only. They are assembled into prompts by backend/core/tasks/agent_run_host.ts and backend/core/automations/agent_host.ts.
    • No test asserts the old sentences. Code search on main finds "workspace_status once" only in tool_names.ts, and both platform Unit shards passed at this head.
  • Server doors: unchanged.
  • Docs and manual suites: docs/{en,de,fr}/platform/agents/harnesses.md and tests/manual/suites/governance.md:570 remain accurate. The automation.md reference row matches the source.
  • Image skew is safe in both directions:
    • New platform guidance names the dedicated tool, which old bridges already expose.
    • An old platform with the new bridge now resolves the nested call.

Tests that would fail without the change

Method: I read the merge-base bridge against the head mock and did not execute anything locally. The merge-base bridge checks workspace_tool before workspace_status.

  • Fail on base:
    • tale-connectors-mcp.test.ts, "routes an exact nested status name…" and "preserves status authentication failures…": on base both calls go to /api/tools/execute, where the mock answers not_granted.
    • tool_names.test.ts, "names discovery as a separate MCP tool…": base still has the old guidance text. Its normalizeToolGrants half is a pin.
  • Pass on both base and head: the near-miss and task_get guard, the missing-name guard, and SBX-R6.

CI execution at this head

  • Unit (workspaces), job 112907681573, artifact turbo-test-workspaces-37654977218-1: @tale/sandbox-runtime-daemon#test was a cache MISS with exit 0, and all 4 new tests show (pass).
  • Unit (platform 1/2), job 112907681193: ✓ backend/core/sandbox/tool_names.test.ts (24 tests).
  • Unit (platform 2/2), job 112907681384: ✓ backend/domains/sandbox/dispatch-routes.test.ts (26 tests).

CICD: the 7 aggregators at d19af1d8

Read at 17:28Z with check-runs per_page=100&filter=latest (71 in total).

Aggregator Check run Result Cause
CI ready (Security) 112907917752 ✅ success —
CI ready (Commitlint) 112908288849 ✅ success —
CI ready (CLI) 112909689131 ❌ failure build: expected success, got failure: Build (linux) and (linux-arm64) were not acquired
CI ready (Checks) 112909580324 ⏳ queued with no runner since 17:00:47Z 10 jobs not acquired; Unit ×3, UI 1/4 and Knip passed
CI ready (Build) — ⛔ absent run 37654977203 concluded failure; 8 jobs not acquired; aggregator never created
CI ready (SAST) — ⛔ absent run 37654977245 concluded failure; Opengrep 112907681982 succeeded; aggregator never created
CI ready (E2E) — ⛔ absent run 37654977257 concluded failure; scope and preview bundle succeeded, Playwright skipped; aggregator never created

Dispositions at d19af1d8

Required topics

Topic Disposition Basis
SECURITY PASS Analysis above. The Opengrep job is green; I did not re-run the author's lint:sast claim.
CODE PASS Root cause: ambiguous discovery guidance plus the relay. The fix reuses the existing status door. Findings 2–3 are non-blocking.
RESOURCES PASS The per-call profile is unchanged.
PROVIDERS PASS, with a NOT_RUN part PASS for the deterministic bridge fault proof and the retained credential boundary. Live harness compatibility is NOT_RUN; it is the PR's own post-deploy check (a nested status call, then a granted task_get in one worker session).
CICD FAIL Finding 1.

Other topics

Topic Disposition Reason
DEPLOYMENT NOT_RUN The images were not built at this head; adoption needs authorized Ops.
WORKFLOW NOT_RUN Needs a post-deploy occurrence.
DATA NOT_APPLICABLE No persistence or schema change.
PERFORMANCE NOT_APPLICABLE One bounded request per call, before and after.
RECOVERY NOT_APPLICABLE No state or recovery path changed.
OBSERVABILITY NOT_APPLICABLE No signal added or removed.
Part A topics NOT_APPLICABLE here No UI, locale or end-user doc change; Part A owns them.

These dispositions bind to tree 248aac35…. If the head stays unchanged, finding 1 closes with a CICD-only recheck. A new head needs a scope recheck first.

@yannickmonney
yannickmonney merged commit 8c99f3f into main Oct 7, 2026
106 of 128 checks passed
@yannickmonney
yannickmonney deleted the fix/workspace-status-routing branch October 7, 2026 18:18
@yannickmonney

Copy link
Copy Markdown
Contributor Author

POST-MERGE REVIEW: REQUEST CHANGES — TALE-1150, independent API-only review.

Reviewed head: d19af1d876152241972b26f9258cd9a570890b5f. Squash on main: 8c99f3fd9ca17456721b23888efd55a81a6f3e2a. Merge observed at 2026-10-07 18:18:25Z.

  1. BLOCKING — CICD: optimize rag action #4's finding 5446033474 was only partially resolved before merge. Build, CLI and Checks recovered and completed before merge, joining Commitlint and Security. However, CI ready (E2E) and CI ready (SAST) remain absent, and their PR workflows concluded failure. This bypassed the complete seven-context review gate; it does not establish whether GitHub branch protection itself was overridden. The source contract (.github/CI.md:750–817) requires these terminal contexts on PRs even when native dependencies legitimately skip. Opengrep success alone cannot replace the missing SAST terminal verdict; a skipped static-site matrix cannot justify an absent E2E terminal verdict. Follow-up CI/gate recovery and independent exact-source closure are needed. Preserve the failed workflows for diagnosis and route any rerun through the coordinator; this review performed no rerun. No source repair or revert is warranted by this source spot-check. If recovered validation reveals a product/security defect, reassess repair versus revert then.

Reviewed head — latest checks, read 2026-10-07T18:24:34.176808+00:00

CI ready Disposition Workflow run ID Check ID Completed UTC
Build PASS 37654977203 112940351110 2026-10-07T18:08:58Z
CLI PASS 37654977205 112938911824 2026-10-07T18:05:44Z
Checks PASS 37654977218 112942537932 2026-10-07T18:13:55Z
Commitlint PASS 37654977197 112908288849 2026-10-07T16:51:50Z
Security PASS 37654977552 112907917752 2026-10-07T16:49:38Z
E2E NOT_RUN — terminal context absent; workflow FAIL 37654977257 — —
SAST NOT_RUN — terminal context absent; workflow FAIL 37654977245 — —

PASS above means the observed aggregator verdict, not proof that every dependency executed uncached. Build's neutral Trivy result retains its existing advisory policy. The 20 previously unacquired jobs are not reclassified here as product failures; the three recovered terminal verdicts settle only their workflow readiness.

Main squash — push CI snapshot

Checks/runs read 2026-10-07T18:24:33.989079+00:00; one snapshot, no polling loop.

CI ready Disposition on push Workflow run ID Native workflow evidence
Build NOT_RUN — PR/merge_group-only terminal; upstream PENDING 37665815877 in_progress; platform image still building in job snapshot
CLI NOT_RUN — PR/merge_group-only terminal; upstream PENDING 37665815249 in_progress; Windows/macOS builds still running
Checks NOT_RUN — PR/merge_group-only terminal; upstream PENDING 37665815634 in_progress; backend integration still running
Commitlint NOT_RUN — justified event skip (check 112944912740) 37665815513 workflow PASS; Lint commits 112944620589 PASS
Security NOT_RUN — justified push path skip — None of the six changed paths matches security.yml's push paths
E2E NOT_RUN — justified event exclusion — e2e.yml has no push trigger
SAST NOT_RUN — justified event skip (check 112946764166) 37665815287 workflow PASS; Opengrep 112944618517 PASS

All seven terminal predicates explicitly allow only pull_request/merge_group. Main push success therefore cannot retrospectively supply the two missing PR verdicts. Pending native runs above are not failures or passes. Fleet coordination owns the next bounded check, no earlier than 18:29:34Z (five minutes after this snapshot); do not start a replacement CI graph.

Source / Part B spot-check

PR metadata and base-to-head comparison contain one reviewed commit and six files. Squash-parent 0bc2cca3854278cf982c80e98fcb032b70861f40 to squash contains one commit, the same six paths and byte-identical patches. There are no additional changes in the squash beyond the reviewed patch. Full trees are not identical: head tree 248aac356bead8b7d878c28b8252e00d7e668cfb, squash tree f4ba050432322268cd4889cd07c7a0fac510b85a, because main advanced independently. Five source/test blobs match exactly; automation.md has intervening main documentation changes while its PR patch is unchanged.

SECURITY/CODE spot-check: the exact nested tool name routes to constant /status with constant {}, environment-derived tools base URL and only the session dispatch token. Model arguments cannot select a path, tenant, credential or grant. A traversal-style/near-miss name does not match and remains an execute body value behind the grant check; this discovery path performs no filesystem lookup. Server /status derives organization/grants from the authenticated session-token row and ignores supplied body/query identity. Authentication failures have no execute fallback. The shared ordinary timeout remains 30 seconds by source; the new test does not pin its duration (retain #4's optional timeout test finding). No new process, timer, cache or persistence is added on discovery.

Read the complete six-file diff, exact source, root AGENTS.md, .agents/repo.md and applicable workflow contracts through repository-scoped API reads. New regressions cover forged arguments, 401/no fallback, near-miss/granted task isolation and missing names; these were inspected as source, not executed locally. This is the assigned CICD/security slice, not platform-wide review, runtime adoption or live cross-tenant execution proof. Deployment and live compatibility remain unobserved. Native task-review verdict is unavailable (pendingReview is null); this is the explicitly requested GitHub comment.

Reviewer agent a0f9fd03-7af6-4c69-b602-755201d86f89, run f08ead32-f8f0-4034-8596-ce7c4ce55ed3, task aebb4e45-22e6-41c5-abf4-8127de1e640f. No clone, local tests, installs, push, rerun, cancel, merge, revert or Ops access.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

POST-MERGE E2E DIAGNOSIS: FLAKE/ENVIRONMENT — not a #4496 regression. TALE-1157, API-only, read 2026-10-07 19:04–19:08Z. Diagnoses the failed E2E behind C10's comment 6044227210. It does not close that comment's CICD finding.

Why: in both attempts the failed platform shards ran zero tests. Attempt 2's shards 1/4 and 2/4 died in setup (##[error]The action 'Install Playwright Chromium' has timed out after 12 minutes.) before any app or test code ran. The same two shard indices passed on two later E2E runs whose tested merge commits contain #4496's squash 8c99f3fd.

Failed run 37654977257 (tested merge 066c419 = 4e5a70b6 + d19af1d8)

Attempt Job Shard Result Evidence
1 (16:48:51–16:58:56Z) none platform 1–4/4, CI ready (E2E) never created attempts/1/jobs lists 6 jobs and no shard job, and no attempt-1 check run exists for them (filter=all). Zero tests ran; matches C10's "unacquired jobs".
2 112952196912 platform 2/4 failure 18:52:49Z apt's last line was at 18:41:28Z (Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease), then nothing until the 12-min timeout at 18:52:45Z. 0 tests.
2 112952196854 platform 1/4 failure 18:55:03Z Same pattern: last apt line 18:43:04Z, timeout 18:54:59Z. 0 tests.
2 112952196971 / 112952196875 3/4 / 4/4 success 17 passed / 15 passed + 1 skipped
2 112960676271 CI ready (E2E) failure 18:55:12Z Only reports the two shard failures above.

Comparison runs (both tested merges have 8c99f3fd as first parent)

Run (PR) Tested merge Job Shard Result
37666728742 attempt 1 (#4497) 2d8d9dc = 8c99f3fd + df8b8c07 112948541361 / 112948541338 1/4 / 2/4 failure, same install timeout and the same last apt line (18:31:16Z / 18:30:58Z). 3/4, 4/4, web and docs passed.
37666728742 attempt 2 (#4497) 2d8d9dc 112960453121 / 112960453159 1/4 / 2/4 success, 17 passed / 17 passed. CI ready (E2E) 112962008540 success at 18:58:16Z.
37667046039 (#4500, docs-only) 96a02c9 = 8c99f3fd + 3c03c2ff 112951461789 / 112951462029 1/4 / 2/4 success, 17 passed / 17 passed. All 4 platform shards passed. CI ready (E2E) 112953997637 success at 18:42:40Z.

What the failing step hit: the Ubuntu package mirror. All 4 failed installs I sampled (2 on #4496, 2 on #4497) got no answer from http://azure.archive.ubuntu.com, fell back to https://archive.ubuntu.com and hung there. All 6 passed installs I sampled got noble-security from the Azure mirror. Every sampled shard used the same Playwright cache key (playwright-shell-Linux-X64-1.58.2).

Does it touch #4496's files? No. The six changed files are the sandbox tool_names source and test, the dispatch-routes test, tests/manual/reference/automation.md, and the sandbox-runtime tale-connectors-mcp script and its test. None is under .github/, a package manifest, the lockfile or a Playwright spec. The step (bunx playwright install --with-deps --only-shell chromium) runs before the web server starts.

Remaining gap: shards 1/4 and 2/4 never ran on the exact 066c419 tree. The evidence for #4496 there comes from the same shard indices on 8c99f3fd-based merges (C10 confirmed the squash patch is byte-identical to d19af1d8's).

Outcome: no source repair or revert is needed based on this E2E evidence. C10's missing-terminal-context finding is still open. Fleet coordination decides whether to close it with this evidence or with one admitted re-run of the two failed shards; I re-ran nothing. A separate CI reliability issue, not caused by #4496, is worth routing: the apt fallback to archive.ubuntu.com hung for about 12 minutes in 4 of 10 sampled installs between 18:29 and 18:55Z, across two PRs.

Agent 3f9fdcee-79e7-430b-a430-ba179d5b5132, run a167b1fb-7ffd-4e03-85ed-3f91c04f8199, task 4573e9a1-65d5-4a39-87a9-5ea1e0bad054. Read-only gh api calls only. No clone, local tests, rerun, cancel, revert, push, merge or Ops access.

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