[sroussey]

Every task class in libs (178 across 9 packages) was evaluated for developer friendliness, correctness and performance. This branch holds what came out of it. The findings, grades and decision records are in the prd repo under analysis/task-evals/2026-11-05/.

Each commit is one change, with a test written to fail first.

Fixes made during the evaluation (21 commits, 9a88e17…11e6a96)

Bugs with no product question attached. The ones with the most effect:

• task-graph
  • Dynamic input schemas are compiled once per class, not on every validation (about 65–87 µs down to about 5 µs per validation).
  • A blank condition operand is no longer read as 0.
  • A streaming group stops its subgraph when the reader leaves, and its failure no longer becomes an unhandled rejection that kills a Node process.
  • Entitlements are enforced inside MapTask, ReduceTask and WhileTask.
• javascript: a script that never ends no longer holds the event loop; it runs in time slices, so abort works.
• mcp
  • A failed tool call can no longer be reported as a success by a tool field named isError.
  • A tool's schemas are discovered once; before, every call opened a second connection.
• tasks
  • String templates substitute once, so user text such as {{secret}} cannot pull in another key.
  • ScalarMax/Min no longer throw a RangeError on 10⁶ values.
  • Plus input-error fixes in DateFormat, Json, Lambda and Delay.
• ai
  • The ai:model entitlement is scoped to the model a run actually uses.
  • A seeded image generation honours cacheable: false.
  • Several refusals are now task errors instead of bare Errors.
• browser-control and a2ui: bad input and missing setup now raise task errors.

The 19 decided product changes (548270b…ec49928)

Changes that refuse or alter things that work today:

• StringTemplateTask is removed. Saved graphs naming it no longer load; TemplateTask replaces it.
• MCP output is namespaced. McpToolCallTask returns { content, isError, structured }. Wiring that read a tool field from the top level reads structured.<field> instead.
• ArrayTask requires maxIterations. The number of combinations is checked against it before any task is built.
• Map output stays index-aligned. Each port gets one slot per item, with null where an iteration produced nothing; a declared port no iteration produced still gets one null per item.
• ConditionalTask: a branch condition that throws now fails the task instead of reading as "not taken".
• Function-built graphs (QuickTask, ParallelTask) refuse to serialize, where before they saved and then failed to load.
• Not cached any more:
  • Readers of a changing source: file loader, grep and sed, KB search and retrieval, web search, and the fetch each of them owns.
  • LambdaTask, always.
  • JavaScriptTask, unless config.cacheable is set.
• Browser-control
  • Navigate and login refuse non-http(s) URLs and URLs the classifier cannot place (such as ones carrying credentials). They declare browser:navigate and network:private scoped to the destination.
  • A navigation that lands on an undeclared private host is refused, and the page is cleared.
  • Upload is confined to configured roots, and declares the real file each path resolves to.
  • BrowserLoginTask is a real manual login: it navigates, then waits on a confirm.
• Refused inputs:
  • an empty list for max/min;
  • anything other than exactly two vectors for distance;
  • a zero divisor element in vector divide;
  • a chunk overlap at least as large as the chunk;
  • resize/border output past the image pixel budget (schema maxima 16384 per side for resize, 8192 for a border);
  • a JavaScript run past its step budget. Time spent waiting on a worker or a timer is not counted, and a preview stops once only timers are left.
• HumanApprovalTask asks with a confirm rather than a form.
• A schema that will not compile fails closed with TaskConfigurationError, for input, config and GraphAsTask alike.
• Fan-in: a port that admits an array (T[] | null, T | T[]) collects multiple incoming dataflows. On union ports collection starts at the second delivery, so a single value is unchanged.
• DocumentEnricherTask bounds its model calls (concurrency, default 4); RerankerTask and QueryExpanderTask require no model capability.
• New settings: StringLengthTask unit (code units / code points / graphemes); StringConcatTask joins in declared-port order.

Fixes from review

• An aborted Map/iterator run fails. Before, it read the abort signal through a context the abort had cleared, and resolved with full results.
• Entitlement enforcement reaches nested work when a task is run directly with enforceEntitlements, scoped to that run.
• config.outputSchema holds an MCP node's ports, and mcpToolOutputPortSchema is idempotent, so builder and libs agree on them.
• The typecheck budget and the co-located test typecheck are back within limits.

Fixes from a second review and the deferred list (7912e90…e9cad38)

• BrowserNewTabTask follows the navigation reach policy (7912e90). A tab given a URL navigates as surely as BrowserNavigateTask does, but it checked nothing. It now refuses what navigate refuses and declares browser:navigate scoped to its URL. A tab that lands on an undeclared private host is closed; on a single-page backend (Electron, bun-webview) closing it ends the session. Navigate and login no longer let a failing page reset replace the refusal it follows.
• DocumentEnricherTask stays within its concurrency (86b11a2). A slot that changed hands was briefly open to a second caller. The slot is now handed straight to the waiter; the test fails on the old code under both runners.
• A filesystem root contains the files beneath it (ae3c704). LocalFilePath checked a root of / as the prefix //, so FileLoader, FileGrep and FileSed refused every file under it.
• FallbackTask alternatives run inside the run (f482d33). In task mode an alternative got none of the run's context: cancelling the fallback left the attempt in flight running, and it used the global registry and its own resource scope. It now gets the run's signal, registry, resource scope, output cache and entitlement enforcement. Data mode gains the scope and enforcement it also lacked.
• A while condition that can never hold is refused (e9cad38). An ordering comparison against a non-numeric value is always false, which ended the loop after one pass with the run reported as successful. It is now a TaskConfigurationError when the condition is built.
• Two comment fixes (87e5512).

Also fixes the nightly Bun parity failure on main

9928a14: the nightly Bun parity job on main (run 42235386392) failed Tabular migration contract: FsFolder > backfill: every existing row is rewritten on its retry.

• The cause was the test, not storage. Bun retries a failed test (retry = 1) while the attempt that timed out may still be running. The FsFolder test harness read its shared "current directory" state after awaits, so the stale attempt followed the retry into its directory and applied the migration there first. The retry's backfill was then skipped as already applied.
• The fix: the harness keeps the directory it chose in a local and records pre-population per directory.
• Regression test: it fails every time against the old harness and passes against the new one.

Known gap: browser request interception

Browser navigation is checked against the URL asked for and the URL the page lands on. It does not stop these:

• Redirects: the request to a private host has already been sent by the time the landing check runs.
• Later navigations: ones a page makes on its own, by script or by a click task.
• Subresources: images, fetches and iframes the page loads.
• DNS: a public hostname that resolves to a private address.

Refusing those before they're sent needs request interception in each browser backend (page.route in Playwright, webRequest in Electron, and so on), checked against the declared scope and the resolved address. That changes the IBrowserContext contract across all four backends, so it is a separate follow-up by decision.

Verification

• bun run build:types, bun run typecheck:tests, bun scripts/typecheck-budget.ts and bun run lint are clean.
• These test sections pass under Vitest: graph, task-graph, task, ai, rag, mcp, browser, web-search.
• Bun parity: the nightly job's own command, bun scripts/test.ts bun unit --except rag,browser,provider-hft,provider-nodellama,provider-api,provider-cactus, passes: 9,611 tests across 691 files, 0 failing. The touched sections run under Bun also pass: 4,604 tests across 370 files. The later commits' tests pass under both runners.
• Not verified: the real-browser path of the browser-control changes. The Playwright integration tests cannot launch in the environment these were written in (chromium_headless_shell-1248 is missing), with or without these changes.

Companion PRs

• workglow-dev/builder#542: draws MCP nodes' ports from the task. It works on the pinned @workglow/mcp 0.6.15 and after this lands.
• workglow-dev/prd#119: the evaluation run and the decision records.

🤖 Generated with Claude Code

https://claude.ai/code/session_015q4aqYCbFQNwRKftssinzQ

[mergestorm-vortex]

Starting the review on the remaining files now. The last Vortex pass at 347b4d4 reviewed 57 of 122 files and posted 6 findings, and the head hasn't moved since. Findings from this run will appear on the PR as usual.

[mergestorm-vortex]

Review progress ██████████ 118/122 files

Comment — found 2 issue(s) at 8a7acd8.

Actionable comments posted: 2

Coverage is incomplete: reviewed 118/122 files; 0 skipped; 4 deferred and still unreviewed.

Comment @mergestorm-vortex review on this PR to review the remaining files.

🤖 Prompt for AI agents

Pull request workglow-dev/libs#1047: https://github.com/workglow-dev/libs/pull/1047 (head branch `claude/confident-lovelace-4cw5ln`)

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@packages/tasks/src/task/FileLoaderTask.ts` (line 130, Important):
   `cachePolicy: { kind: "none" }` disables only this task's output cache. `execute()` still owns a `FetchUrlTask` at line 174, which has no `cachePolicy` override and therefore uses the default deterministic policy. With a `CacheRegistry` configured, running the loader twice for the same URL can still return the first fetch's cached body after the remote file changes, defeating this change's no-cache behavior. `FileGrepTask` has the same owned-fetch path.

2. In `@packages/tasks/src/task/vector/VectorMultiplyTask.ts` (line 90, Important):
   `return this.execute(input)` makes `runPreview()` perform the complete O(vector length × vector count) multiplication and allocate the result synchronously; vector inputs have no size bound. Large vectors can therefore block the preview/UI path, contrary to the lightweight-preview contract. The same forwarding pattern is used by the other vector preview methods in this diff.

---

[mergestorm-vortex]

Review progress ██████████ 124/124 files

Request changes — found 4 issue(s) at 8f7dd7a.

Actionable comments posted: 4

🤖 Prompt for AI agents

Pull request workglow-dev/libs#1047: https://github.com/workglow-dev/libs/pull/1047 (head branch `claude/confident-lovelace-4cw5ln`)

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 79, Blocker):
   `classifyUrl` returns `invalid` for URLs with username/password, but this branch adds `network:private` only for `kind === "private"`; `assertNavigableUrl` accepts such HTTP(S) URLs. For example, `http://user:pass@169.254.169.254/latest/meta-data` therefore declares only scoped `browser:navigate`, so a policy granting that scope while denying `network:private` passes the preflight and the browser contacts the private host. Refuse invalid classifications or require the private entitlement for them.

2. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 101, Blocker):
   Both navigation tasks call this only after `ctx.navigate()` returns. The Playwright backend implements that with `page.goto(..., { waitUntil: "load" })`, which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to `about:blank`. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.

3. In `@packages/task-graph/src/task/IteratorTaskRunner.ts` (line 355, Blocker):
   `enforceEntitlements: this.task.runConfig?.enforceEntitlements`
   When a caller invokes `map.run(input, { registry, enforceEntitlements: true })`, `TaskRunner` checks the MapTask using that run's `effectiveConfig` but does not copy the option into `task.runConfig`. This therefore passes `undefined` to the cloned subgraph's `run()`, which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

4. In `@packages/javascript/src/task/JavaScriptTask.ts` (line 71, Important):
   `if (steps >= maxSteps)` counts every loop iteration, including iterations where `interpreter.step()` makes no progress because `getStatus()` is `TASK` while a `setTimeout` callback is pending. The interpreter supports `setTimeout`; for example, a script scheduling a callback after 6 seconds can exhaust the default 50M-step budget (and fail as `TaskInvalidInputError`) before the callback runs, despite executing very little user code. The same issue can make shorter timers exceed the much smaller preview budget.

---

[mergestorm-vortex]

Review progress ██████████ 125/125 files

Request changes — found 6 issue(s) at 0421952.

Actionable comments posted: 6

🤖 Prompt for AI agents

Pull request workglow-dev/libs#1047: https://github.com/workglow-dev/libs/pull/1047 (head branch `claude/confident-lovelace-4cw5ln`)

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 141, Blocker):
   `resources: filePaths as string[]`
   The entitlement is checked against these original path strings, but `containUploadPaths()` resolves symlinks and passes the resolved paths to `ctx.uploadFile()` afterward. For example, a grant scoped to `/work/input.txt` allows that path when it is a symlink to `/work/.env`; if both are inside the configured root, the root check passes and the upload reads `.env` without a grant for that resolved resource. Ensure the entitlement scope covers the canonical paths that are actually uploaded.

2. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 193, Important):
   `p.startsWith(root + path.sep)` turns a configured root of `/` into the prefix `//`, so paths such as `/tmp/file.txt` fail containment even though they are inside `/`. Windows drive roots have the same trailing-separator issue. Handle filesystem roots as containing their descendants (or otherwise normalize this boundary) so a valid root configuration does not reject every upload.

3. In `@packages/javascript/src/task/JavaScriptTask.ts` (line 183, Important):
   `JavaScriptTaskConfig` now declares `maxSteps`, but `javaScript()` and `Workflow.prototype.javaScript` still type their config argument as the base `TaskConfig`. TypeScript callers therefore cannot pass `{ maxSteps: ... }` through these public entry points (an excess-property error); only constructing `JavaScriptTask` directly exposes the new setting.

4. In `@packages/task-graph/src/task/TaskRunner.ts` (line 429, Important):
   `this.task.runConfig` outlives this call, so a direct `task.run(..., { enforceEntitlements: true })` permanently sets this flag. A later `task.run(..., { enforceEntitlements: false })` makes the outer run's `effectiveConfig` false, but `GraphAsTaskRunner`, `IteratorTaskRunner`, and `WhileTask` pass `this.task.runConfig?.enforceEntitlements` to nested runs, so they still enforce (and can reject work or fail when no enforcer is registered). Keep this propagation scoped to the current run rather than leaving it on the task instance.

Additional findings outside changed lines:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 117, Blocker):
   The redirect check runs after the private request
   Both navigation tasks call this only after `ctx.navigate()` returns. The Playwright backend implements that with `page.goto(..., { waitUntil: "load" })`, which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to `about:blank`. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.

2. In `@packages/task-graph/src/task/IteratorTaskRunner.ts` (line 355, Blocker):
   Nested map run drops per-run entitlement enforcement
   `enforceEntitlements: this.task.runConfig?.enforceEntitlements`
   When a caller invokes `map.run(input, { registry, enforceEntitlements: true })`, `TaskRunner` checks the MapTask using that run's `effectiveConfig` but does not copy the option into `task.runConfig`. This therefore passes `undefined` to the cloned subgraph's `run()`, which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

Still open from earlier reviews (2) — the lines they point at have not changed:

• 🔴 Blocker · The redirect check runs after the private request · packages/browser-control/src/task/BrowserReachPolicy.ts line 117 · from 8f7dd7a
  Both navigation tasks call this only after ctx.navigate() returns. The Playwright backend implements that with page.goto(..., { waitUntil: "load" }), which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to about:blank. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.
• 🔴 Blocker · Nested map run drops per-run entitlement enforcement · packages/task-graph/src/task/IteratorTaskRunner.ts line 355 · from 8f7dd7a
enforceEntitlements: this.task.runConfig?.enforceEntitlements
  When a caller invokes map.run(input, { registry, enforceEntitlements: true }), TaskRunner checks the MapTask using that run's effectiveConfig but does not copy the option into task.runConfig. This therefore passes undefined to the cloned subgraph's run(), which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

[mergestorm-vortex]

Review progress ██████████ 125/125 files

Request changes — found 6 issue(s) at 4d32fab.

Actionable comments posted: 6

🤖 Prompt for AI agents

Pull request workglow-dev/libs#1047: https://github.com/workglow-dev/libs/pull/1047 (head branch `claude/confident-lovelace-4cw5ln`)

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 89, Blocker):
   `classifyUrl(url)` only classifies the URL's literal hostname; it does not resolve DNS. A URL such as `http://attacker.example` therefore gets only `browser:navigate` even when its A/AAAA record resolves to `127.0.0.1` or `10.0.0.5`, and the browser backend resolves and connects to that private address without `network:private`. The landing check has the same limitation because it classifies the URL text. Unlike `SafeFetch.server.ts`, this path does not check the resolved addresses, so private services reachable from the browser host can be accessed without their entitlement.

2. In `@packages/task-graph/src/task/TaskRunner.ts` (line 434, Blocker):
   `this.task.runConfig` is stamped here, but `FallbackTaskRunner` does not pass that value into its nested runs: task mode calls `alternativeTask.run(input, this.streamRunOptions)` and data mode calls `subGraph.run(...)` without `enforceEntitlements`. With a runtime-only URL such as `http://169.254.169.254/...` reaching a fallback fetch from upstream, the child runs without the parent enforcer and can make the denied request. The added FallbackTask tests exercise these paths, but this stamp alone cannot make their nested executions enforce the policy.

3. In `@packages/javascript/src/task/JavaScriptTask.ts` (line 89, Important):
   `interpreter.step()` has already executed at line 88 before this check. When `steps === maxSteps`, the next interpreter instruction still runs and only then throws, so a run configured for N steps can execute N+1 steps (including any work or side effects in that extra step) before rejection. Check the budget before advancing the interpreter when more work is pending.

Additional findings outside changed lines:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 145, Blocker):
   The file grant can name a symlink, not the file actually uploaded
   `resources: filePaths as string[]`
   The entitlement is checked against these original path strings, but `containUploadPaths()` resolves symlinks and passes the resolved paths to `ctx.uploadFile()` afterward. For example, a grant scoped to `/work/input.txt` allows that path when it is a symlink to `/work/.env`; if both are inside the configured root, the root check passes and the upload reads `.env` without a grant for that resolved resource. Ensure the entitlement scope covers the canonical paths that are actually uploaded.

2. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 117, Blocker):
   The redirect check runs after the private request
   Both navigation tasks call this only after `ctx.navigate()` returns. The Playwright backend implements that with `page.goto(..., { waitUntil: "load" })`, which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to `about:blank`. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.

3. In `@packages/task-graph/src/task/IteratorTaskRunner.ts` (line 355, Blocker):
   Nested map run drops per-run entitlement enforcement
   `enforceEntitlements: this.task.runConfig?.enforceEntitlements`
   When a caller invokes `map.run(input, { registry, enforceEntitlements: true })`, `TaskRunner` checks the MapTask using that run's `effectiveConfig` but does not copy the option into `task.runConfig`. This therefore passes `undefined` to the cloned subgraph's `run()`, which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

Still open from earlier reviews (3) — the lines they point at have not changed:

• 🔴 Blocker · The file grant can name a symlink, not the file actually uploaded · packages/browser-control/src/task/BrowserReachPolicy.ts line 145 · from 0421952
resources: filePaths as string[]
  The entitlement is checked against these original path strings, but containUploadPaths() resolves symlinks and passes the resolved paths to ctx.uploadFile() afterward. For example, a grant scoped to /work/input.txt allows that path when it is a symlink to /work/.env; if both are inside the configured root, the root check passes and the upload reads .env without a grant for that resolved resource. Ensure the entitlement scope covers the canonical paths that are actually uploaded.
• 🔴 Blocker · The redirect check runs after the private request · packages/browser-control/src/task/BrowserReachPolicy.ts line 117 · from 8f7dd7a
  Both navigation tasks call this only after ctx.navigate() returns. The Playwright backend implements that with page.goto(..., { waitUntil: "load" }), which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to about:blank. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.
• 🔴 Blocker · Nested map run drops per-run entitlement enforcement · packages/task-graph/src/task/IteratorTaskRunner.ts line 355 · from 8f7dd7a
enforceEntitlements: this.task.runConfig?.enforceEntitlements
  When a caller invokes map.run(input, { registry, enforceEntitlements: true }), TaskRunner checks the MapTask using that run's effectiveConfig but does not copy the option into task.runConfig. This therefore passes undefined to the cloned subgraph's run(), which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

[mergestorm-vortex]

Review progress ██████████ 125/125 files

Request changes — found 6 issue(s) at 8f94ee5.

Actionable comments posted: 6

🤖 Prompt for AI agents

Pull request workglow-dev/libs#1047: https://github.com/workglow-dev/libs/pull/1047 (head branch `claude/confident-lovelace-4cw5ln`)

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@packages/test/src/test/task/NestedEntitlementEnforcement.test.ts` (line 286, Important):
   `expectDenied()` is also used by the data-mode fallback test, but `FallbackTaskRunner` catches each alternative's `TaskEntitlementError` and throws a `TaskFailedError` from `buildAggregateError()` after attempts are exhausted. Thus this assertion (and the direct fallback assertions at lines 330 and 344) fails on a correctly denied request. Assert the aggregate failure while retaining the `fetched` check, or otherwise inspect the wrapped cause/message.

Additional findings outside changed lines:
1. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 89, Blocker):
   DNS-resolved private destinations bypass the private entitlement
   `classifyUrl(url)` only classifies the URL's literal hostname; it does not resolve DNS. A URL such as `http://attacker.example` therefore gets only `browser:navigate` even when its A/AAAA record resolves to `127.0.0.1` or `10.0.0.5`, and the browser backend resolves and connects to that private address without `network:private`. The landing check has the same limitation because it classifies the URL text. Unlike `SafeFetch.server.ts`, this path does not check the resolved addresses, so private services reachable from the browser host can be accessed without their entitlement.

2. In `@packages/task-graph/src/task/TaskRunner.ts` (line 434, Blocker):
   Fallback executions still drop entitlement enforcement
   `this.task.runConfig` is stamped here, but `FallbackTaskRunner` does not pass that value into its nested runs: task mode calls `alternativeTask.run(input, this.streamRunOptions)` and data mode calls `subGraph.run(...)` without `enforceEntitlements`. With a runtime-only URL such as `http://169.254.169.254/...` reaching a fallback fetch from upstream, the child runs without the parent enforcer and can make the denied request. The added FallbackTask tests exercise these paths, but this stamp alone cannot make their nested executions enforce the policy.

3. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 117, Blocker):
   The redirect check runs after the private request
   Both navigation tasks call this only after `ctx.navigate()` returns. The Playwright backend implements that with `page.goto(..., { waitUntil: "load" })`, which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to `about:blank`. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.

4. In `@packages/browser-control/src/task/BrowserReachPolicy.ts` (line 145, Blocker):
   The file grant can name a symlink, not the file actually uploaded
   `resources: filePaths as string[]`
   The entitlement is checked against these original path strings, but `containUploadPaths()` resolves symlinks and passes the resolved paths to `ctx.uploadFile()` afterward. For example, a grant scoped to `/work/input.txt` allows that path when it is a symlink to `/work/.env`; if both are inside the configured root, the root check passes and the upload reads `.env` without a grant for that resolved resource. Ensure the entitlement scope covers the canonical paths that are actually uploaded.

5. In `@packages/task-graph/src/task/IteratorTaskRunner.ts` (line 355, Blocker):
   Nested map run drops per-run entitlement enforcement
   `enforceEntitlements: this.task.runConfig?.enforceEntitlements`
   When a caller invokes `map.run(input, { registry, enforceEntitlements: true })`, `TaskRunner` checks the MapTask using that run's `effectiveConfig` but does not copy the option into `task.runConfig`. This therefore passes `undefined` to the cloned subgraph's `run()`, which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

Still open from earlier reviews (5) — the lines they point at have not changed:

• 🔴 Blocker · DNS-resolved private destinations bypass the private entitlement · packages/browser-control/src/task/BrowserReachPolicy.ts line 89 · from 4d32fab
classifyUrl(url) only classifies the URL's literal hostname; it does not resolve DNS. A URL such as http://attacker.example therefore gets only browser:navigate even when its A/AAAA record resolves to 127.0.0.1 or 10.0.0.5, and the browser backend resolves and connects to that private address without network:private. The landing check has the same limitation because it classifies the URL text. Unlike SafeFetch.server.ts, this path does not check the resolved addresses, so private services reachable from the browser host can be accessed without their entitlement.
• 🔴 Blocker · Fallback executions still drop entitlement enforcement · packages/task-graph/src/task/TaskRunner.ts line 434 · from 4d32fab
this.task.runConfig is stamped here, but FallbackTaskRunner does not pass that value into its nested runs: task mode calls alternativeTask.run(input, this.streamRunOptions) and data mode calls subGraph.run(...) without enforceEntitlements. With a runtime-only URL such as http://169.254.169.254/... reaching a fallback fetch from upstream, the child runs without the parent enforcer and can make the denied request. The added FallbackTask tests exercise these paths, but this stamp alone cannot make their nested executions enforce the policy.
• 🔴 Blocker · The redirect check runs after the private request · packages/browser-control/src/task/BrowserReachPolicy.ts line 117 · from 8f7dd7a
  Both navigation tasks call this only after ctx.navigate() returns. The Playwright backend implements that with page.goto(..., { waitUntil: "load" }), which follows redirects and runs the landing page before this check; a public URL redirecting to a private service can therefore reach it and execute its response (including script-triggered requests) before the task throws and navigates to about:blank. Clearing the page prevents downstream extraction but cannot undo that access; the private destination must be blocked before its redirected request is sent.
• 🔴 Blocker · The file grant can name a symlink, not the file actually uploaded · packages/browser-control/src/task/BrowserReachPolicy.ts line 145 · from 0421952
resources: filePaths as string[]
  The entitlement is checked against these original path strings, but containUploadPaths() resolves symlinks and passes the resolved paths to ctx.uploadFile() afterward. For example, a grant scoped to /work/input.txt allows that path when it is a symlink to /work/.env; if both are inside the configured root, the root check passes and the upload reads .env without a grant for that resolved resource. Ensure the entitlement scope covers the canonical paths that are actually uploaded.
• 🔴 Blocker · Nested map run drops per-run entitlement enforcement · packages/task-graph/src/task/IteratorTaskRunner.ts line 355 · from 8f7dd7a
enforceEntitlements: this.task.runConfig?.enforceEntitlements
  When a caller invokes map.run(input, { registry, enforceEntitlements: true }), TaskRunner checks the MapTask using that run's effectiveConfig but does not copy the option into task.runConfig. This therefore passes undefined to the cloned subgraph's run(), which disables its entitlement checks; an inner task requiring a denied entitlement can execute unchecked. Propagate the active run's enforcement setting here.

---

[mergestorm-vortex]

Mergestorm reviews this repository's pull requests

Every PR gets AI code review that flags real bugs before they merge. This account's 30 free credits for this monthly window are used, so reviews are paused at e9cad38 and resume automatically on Dec 1 (UTC).

The Starter plan includes 120 credits a month.

Upgrade for more reviews →