fix(daemon): allowlist the active skill root for external_directory - #8178

Thanks @aron-intframe — the description clearly connects the permission prompt to the active skill root. The change is now in the review queue.

Queued for QA validation — this change affects a live skill-execution path and needs a manual pass before merge. Nothing needed from you right now; we’ll update here once it is validated.

Thanks for the approval. I re-ran both of your claims on a clean clone of the reviewed SHA (0d0771d) instead of taking them at face value, and they hold: Node 24.21 / pnpm 10.33.2, full tests/chat-route.test.ts plus the new suite.

      Tests  81 passed (81)
 ✓ tests/skill-root-external-directory.test.ts > run-scoped external_directory allowlist — skill roots > still hands Codex no read-only resource directory 0ms

Falsification, one half at a time. Deleting activeSkillDirs, from the resolveChatExtraAllowedDirs({ ... }) call in apps/daemon/src/server.ts reds only the chat-route case:

 FAIL  tests/chat-route.test.ts > /api/chat > allowlists the active skill root that lives outside the repository skills dir
AssertionError: expected undefined to be truthy

Restoring that and instead dropping the ...(Array.isArray(activeSkillDirs) ? activeSkillDirs : []) spread inside chat-prompt-inputs.ts reds the plugin-local case instead:

 FAIL  tests/skill-root-external-directory.test.ts > run-scoped external_directory allowlist — skill roots > allows every side file a plugin-local skill advertises
AssertionError: expected [] to deeply equal [ …(4) ]
      Tests  1 failed | 2 passed (3)

A correction to my own PR description. I described the new suite as "red before the fix: 0/4 manifest-declared assets allowed". Only one of its three cases is actually falsifiable against the change. ships the web-prototype assets its manifest declares stays green with the fix reverted, because it asserts that the manifest's four od.context.assets entries exist on disk, not that they are allowlisted. It is a fixture guard, not regression cover, so 2 of 3 cases carry the fix.

The caveat I filed does not reproduce. I wrote that chat-route.test.ts aborts mid-file on a better-sqlite3 teardown assertion in my sandbox. On a fresh pnpm install --frozen-lockfile the whole file runs, 81/81 as quoted above, so that was my locally built native module and not something the queued QA pass needs to work around. pnpm --filter @open-design/daemon typecheck is clean on the same checkout.

Where "every resolved active skill directory" has an edge, since QA is about to exercise this path: two other sites deliberately pass an empty list, activeSkillDirs: odNextStrategyRecipe ? [] : activeSkillDirs in the prompt-composer return and activeSkillDirs: [] in the strategyTaskAtStart branch. I read both. The first composes its prompt through composeOdNextStrategyCorePromptV2(recipe) and the second returns prompt: '', so neither routes a skill body through withSkillRootPreamble() and neither advertises an absolute skill root, which keeps the invariant intact. I did not run an OD Next strategy turn end to end, so that one is read, not run.

Also not verified: a real opencode binary. Both tests use a fake agent on PATH and read back OPENCODE_CONFIG_CONTENT, and the allowsRead helper in the new suite mirrors OpenCode's external_directory prefix matching rather than calling OpenCode itself. Nothing needed changing, so no new commits on the branch.

QA Acceptance Record

@aron-intframe QA passed for the verified scope at head 0d0771d6d829dde46a332c117dba2ff9df9c56ad, following the requester’s explicit acceptance decision.

• All 84 tests in the touched suites passed; daemon typecheck and repository guard passed. Reverting the fix failed the two targeted regression cases; restoring it passed both.
• Real OpenCode read bundled skill resources (4/4) and a user-installed skill resource outside the project directory. Unrelated paths and a skill from another project were denied; a missing resource produced an explicit error.
• Electron development runtime: real OpenAI generation, rendered prototype preview, continued edits, and a subsequent run with Linear bound as the design system succeeded. Claude credit was not required.
• Packaged installer verification is explicitly deferred by the requester until packaging. Session-restart pending-permission recovery and the separate memory-llm issue are outside this PR’s scope.

Environment record: the QA Electron/web/daemon namespace was stopped, its ports closed, and the disposable runtime data and copied credential removed. Durable evidence and the QA worktree/dependencies were retained. An earlier unsandboxed test invocation touched shared OpenCode log/database/lock state; additional skill-file changes were observed but cannot be attributed. Full restoration is unproven, and no unsafe rollback of concurrent user state was attempted. This remains a separately disclosed local environment incident, not a confirmed PR defect.

Associated acceptance suggestion: perform the planned packaged-app smoke for bundled and user-installed skill resource reads.

🎉 📡 You just leveled up to Giotto

📡 ✨ Sending steady signals.

🙌 Your contributions are sending a clear signal across the network: you care about making Open Design better. Keep transmitting.

💛 Thanks for helping Open Design move forward. Keep building in the open. 🚀

📊 Rank #206 among 800+ contributors

🔗 Share on X (English) · 分享到 X（中文）

Thanks for the record. I re-ran it against merged main (e38462f) rather than the reviewed SHA, since 59 commits landed in between and apps/daemon/src/server.ts alone moved 378+/122- around the call site. Your numbers reproduce there.

The test count matches exactly. Node 24.21 / pnpm 10.33.2, both touched suites:

 Test Files  2 passed (2)
      Tests  84 passed (84)
   Duration  268.09s (transform 3.20s, setup 22ms, import 4.14s, tests 263.79s, environment 0ms)

pnpm --filter @open-design/daemon typecheck and pnpm guard are both clean on the same checkout, three of the four files the PR touched are byte-identical to the reviewed head, and in server.ts, which other PRs have since rewritten heavily, the one activeSkillDirs, line survived the drift intact.

"Two targeted regression cases" is the right count, and each half of the fix carries exactly one. Removing activeSkillDirs, from the resolveChatExtraAllowedDirs({ ... }) call in server.ts reds only the chat-route case:

 FAIL  tests/chat-route.test.ts > /api/chat > allowlists the active skill root that lives outside the repository skills dir
AssertionError: expected undefined to be truthy
      Tests  1 failed | 80 skipped (81)

Restoring that and instead dropping the ...(Array.isArray(activeSkillDirs) ? activeSkillDirs : []) spread inside chat-prompt-inputs.ts reds only the other one:

 FAIL  tests/skill-root-external-directory.test.ts > run-scoped external_directory allowlist — skill roots > allows every side file a plugin-local skill advertises
AssertionError: expected [] to deeply equal [ …(4) ]
      Tests  1 failed | 2 passed (3)

The third case in that suite is a fixture guard rather than regression cover, as I corrected earlier, so your count of two is the honest one.

On the deferred packaged smoke, I can rule one failure mode out for you statically. The risk worth naming is that a bundled skill root is unreadable by the external opencode process even once it is allowlisted, because it sits inside app.asar. It does not: ELECTRON_BUILDER_ASAR = false in both tools/pack/src/mac/constants.ts and tools/pack/src/win/constants.ts, asar: false in tools/pack/src/linux.ts, and plugins/_official ships as a whole recursive tree in BUNDLED_RESOURCE_TREES. I called the real copyBundledResourceTrees into a temporary resource root:

staged skill root: <resources>/plugins/_official/examples/web-prototype
declared assets: 4
  PRESENT  ./example.html
  PRESENT  ./assets/template.html
  PRESENT  ./references/checklist.md
  PRESENT  ./references/layouts.md
plain-file assets under the staged skill root: 4/4
skill root is a real directory (not inside app.asar): true

BUNDLED_PLUGINS_DIR resolves through resolveDaemonResourceDir(DAEMON_RESOURCE_ROOT, path.join('plugins', '_official'), ...) to that same <resourcesPath>/plugins/_official, so the root the allowlist receives in a packaged build is a plain directory holding all four declared side files. The smoke still has to confirm the runtime leg, but it is not exposed to an asar-opacity failure.

What I did not verify. There is no real OpenCode or Codex binary in this environment (opencode: NOT on PATH), so your 4/4 real-agent read, the denials for unrelated paths and another project's skill, and the explicit missing-resource error are taken on your word rather than reproduced; the same goes for the Electron dev run with real OpenAI generation. I also cannot speak to the shared OpenCode log/database/lock state your environment note discloses, and I did not attempt to reconstruct it. I found no new defect, so there are no new commits on the branch.