Repository navigation
fix(sandbox): route nested workspace status discovery safely - #4496
Conversation
yannickmonney
left a comment
There was a problem hiding this comment.
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, runb4dd405a-5597-4bd9-a017-4b4655cf39f3, card TALE-1143 (c49f756e-1190-4869-8d85-d5bf430c9c65). Routed by Fleet coordination #1, runac2294c3. - Implementer: the PR names no agent, task or issue. Commit
d19af1d8was 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-base27084814. - One commit, tree
248aac356bead8b7d878c28b8252e00d7e668cfb, 6 files, +179/−14. - Read through
compareandcontents?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.
- PR base
Findings
- [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)andCI 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 istests/manual/reference/automation.md, in a different row, and GitHub reports the PR mergeable. A fresh run also covers the moved base.
- 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:
- [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(thenot_grantedguidance) andbackend/core/node_only/sandbox/workspace_tools_bridge.ts:613(the unknown-tool refusal). - A model reads them right after a
workspace_toolrefusal, 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_statusMCP tool with{}". During image skew they also keep steering old bridges intonot_granted. - Optional follow-up: align both sentences.
- The two sentences are in
- [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 thetimed out after 200msisError result for both the nested and the dedicated name.
- The status mock answers at once. A mutant that gives the nested branch
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) matchesargs.tool === 'workspace_status'exactly and always callscallPlatform('/status', {}, TOOLS_BASE_URL):- URL:
TOOLS_BASE_URLis derived once fromTALE_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
{}.
- URL:
- 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/../executedoes 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.
- A traversal-style name such as
- 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,organizationIdandplatform.version. It asserts that the door received exactly{}withBearer session-key, and that no execute call was made.
- The new bridge test sends forged
- No new information.
workspace_statusis already listed to every session, and the door answers any live token with that token's own grants. - Discovery stays out of grants.
normalizeToolGrantsstripsworkspace_status(tool_names test). Execute still refuses it, while a grantedtask_getworks 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.
- A 401 is relayed as
- Existing shared
callPlatformbehaviour:- 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/listdescriptions forworkspace_toolandworkspace_status: the existing description test is green.- Guidance strings
KNOWLEDGE_TOOLS_GUIDANCE,imageGenerationGuidanceandgrantedToolsGuidance: text only. They are assembled into prompts bybackend/core/tasks/agent_run_host.tsandbackend/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.
- No test asserts the old sentences. Code search on main finds "workspace_status once" only in
- Server doors: unchanged.
- Docs and manual suites:
docs/{en,de,fr}/platform/agents/harnesses.mdandtests/manual/suites/governance.md:570remain accurate. Theautomation.mdreference 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 answersnot_granted.tool_names.test.ts, "names discovery as a separate MCP tool…": base still has the old guidance text. ItsnormalizeToolGrantshalf is a pin.
- Pass on both base and head: the near-miss and
task_getguard, the missing-name guard, and SBX-R6.
CI execution at this head
- Unit (workspaces), job
112907681573, artifactturbo-test-workspaces-37654977218-1:@tale/sandbox-runtime-daemon#testwas 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.
|
POST-MERGE REVIEW: REQUEST CHANGES — TALE-1150, independent API-only review. Reviewed head:
Reviewed head — latest checks, read 2026-10-07T18:24:34.176808+00:00
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 snapshotChecks/runs read 2026-10-07T18:24:33.989079+00:00; one snapshot, no polling loop.
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-checkPR metadata and base-to-head comparison contain one reviewed commit and six files. Squash-parent SECURITY/CODE spot-check: the exact nested tool name routes to constant 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 |
|
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 ( Failed run 37654977257 (tested merge
|
| 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.
Summary
An agent that nested
workspace_statusinsideworkspace_toolreached the domain-tool grant gate and received a misleadingnot_grantedresponse, 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 checkpasses: 49/49 tasks, 38 cache hits; affected checks executed.bun run lint:sastpasses with no findings; the full scanner reports 81 partially analyzed files and its configured/large-file skips.Test plan
task_getcall under the same token scope. Existing status-door tests preserve refused-session responses and server-owned grants.task_getin one live worker session.