fix(pi): require confirmation for external scan inputs

[yashrajp22]

The Pi scan tool could read an external target or YARA rules directory without asking the user. It now requests approval before resolving external paths, rechecks paths after approval, and preserves the CLI's local symlink checks. Remote targets require approval too. Headless sessions reject requests that need approval; cancellation closes the dialog.

YARA includes are disabled in all compile paths, so rules cannot read arbitrary included files. Valid rules remain usable, and rejected include rules produce a visible read error. Invalid output paths fail before any approval prompt.

Validation: 93 Pi/OpenCode tests and 369 Python YARA tests passed; the 369 Python tests also passed against a freshly installed wheel. Coverage includes canceled calls, workspace swaps, external-path probing, absolute/relative includes, and valid rules. Extension CLI execution is mocked. These focused tests made no live provider calls.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for closing the Pi tool's unapproved external-read path! The gate canonicalises local inputs before the workspace check, rechecks them after the dialog, and fails closed when no dialog UI exists. The new tests also run against the real extension source.

Value and readiness: The problem is real on main. There, extensions/skillspector.ts:30-38 and :105-115 pass target and yaraRulesDir to the CLI without asking the user, and isLikelyUrl sends any a/b string through unchanged. I ran main's CLI with the exact arguments that extension builds for an external file (scan <outside>/private.md --format json --no-llm, cwd = workspace). It exited 0, and the file's marker string appeared twice in the JSON report that the tool returns to the agent.

This PR puts the following behind a confirm dialog that shows the canonical path or URL:

• absolute, ../ and symlink-escaping paths
• external YARA directories
• every https:// and git@...git target

It also rejects a remote yaraRulesDir and keeps workspace scans prompt-free. Its remote classification matches the CLI's on 14 sample inputs (transcription).

All findings below are non-blocking. The most important one, finding 1, is an existing CLI gap that limits how far the new gate reaches. It is not a regression, and I recommend fixing it here or in a tracked follow-up. The PR is ready for final maintainer review. CI still has to run before merge.

Material findings

1. [Non-blocking] extensions/skillspector.ts:91: a workspace-local yaraRulesDir can load external YARA files through include without approval.
   • A rules dir whose realpath is inside the workspace adds no approval request (lines 87-93). The PR's own test asserts this with hasUI: false and zero prompts (tests/unit/test_pi_extension.mjs:150-160).
   • The CLI compiles user rules at static_yara.py:401, :413 and :420, using yara.compile(sources=...) and (source=...) without includes=False. libyara therefore opens include targets itself, outside the no-follow collector that the comment at lines 106-107 relies on.
   • I reproduced this with main's CLI. The PR changes no Python, and static_yara.py is byte-identical. I used the args the PR builds, with cwd = workspace:
     • rules/inc.yar = include "<abs>/outside/private.yar": exit 0. The report contained YARA rule 'external_private_rule': EXTERNAL_RULE_LOADED_WITHOUT_APPROVAL [inc].
     • include "../outside/rel.yar": also loaded. Relative includes resolve against the CLI process cwd, which the extension sets to the workspace (line 187).
   • Scope of the leak:
     • Only YARA-parseable files load. Including a KEY=value file produces only a syntax error, with none of the file's content.
     • The names and meta of matching rules appear in findings.
     • With verbose: true, compile errors go to stderr, which the tool returns to the agent (lines 195, 217). Including the same file twice gives duplicated identifier "<first rule name>", which reveals a rule name even when that rule does not match.
   • Consequence: the claim "requires approval before ... reads external ... YARA rule directories" does not cover rule files reached through include. A malicious repo can ship a rules/ dir and steer the agent to scan with it.
   • Expected fix:
     • Pass includes=False to all three yara.compile calls together. If only line 401 changes, the per-source fallback at 413 still accepts the including file.
     • In yara-python 4.5.4, this raises SyntaxError: line 1: includes are disabled. The existing handlers already catch it, so the file is skipped and the other rules keep loading.
     • The built-in rules use no include. I checked the four .yar files and the decoded malware.yar.b64.
     • Add a regression test with one absolute and one relative external include.
     • If Python changes are out of scope here, reword the comment at lines 106-107, the schema text at line 24 and the PR description so they do not claim to cover included rule files. Then track includes=False as a follow-up.
2. [Non-blocking] extensions/skillspector.ts:89: sending the canonical target to the CLI turns its symlink refusal into a prompt, or into a silent follow inside the workspace.
   • Lines 87-89 store realpathSync(input), and buildScanArgs (line 146) sends it to the CLI. main's CLI refuses symlinked inputs (cli.py:717 → input_handler.py:293, :298).
   • Measured with main's CLI from the workspace:
     • linked.md -> real/SKILL.md and realdir/SKILL.md (realdir -> real) both exited 2, with "Refusing to resolve a symlinked input" and "...with a symlinked parent".
     • With the arguments the PR builds (transcription), both run with exit 0 and no prompt.
     • out.md -> ../outside/SKILL.md was refused on main. With the PR it becomes one dialog and, if approved, a read.
   • The OpenCode tool keeps the CLI's refusal on purpose (.opencode/tools/skillspector_scan_lib.ts:611-616).
   • The test's linked.md case (test_pi_extension.mjs:173) presents that file as a main exposure, but the real CLI refused it; the mocked pi.exec hides the refusal. The other cases in that test are genuine main exposures.
   • Exposure is not higher than on main. An agent could already pass the absolute external path and have it read with no prompt, and the new dialog names the canonical path. What changes is policy: Pi, the CLI and OpenCode now handle symlinks differently, and this is not documented.
   • Side effect: the CLI now classifies by canonical name, so an x.zip symlink to y.dat is scanned as a single file.
   • Expected fix:
     • Either reject a local target with a symlinked or junctioned component before prompting, as the OpenCode tool does, or use realpath only for the inside/outside decision and the dialog text and pass the lexical input to the CLI.
     • Keep canonicalising yaraRulesDir, which the CLI resolves anyway (cli.py:737).
     • Update the linked.md case at :173 and the target assertions at :195-199.
     • If following symlinks is intended, say so in the PR description and the docs.
3. [Non-blocking] docs/PI_EXTENSION.md:42: the new permission behaviour is not documented.
   • Line 98 rejects every request when !ctx.hasUI, and every remote target adds a request (line 85). Pi's ExtensionMode is tui | rpc | json | print, and hasUI is true only in TUI and RPC. So pi -p and --mode json can no longer scan any URL, or any path outside the session cwd.
   • The doc's target bullet (line 42) and yaraRulesDir bullet (line 48) are unchanged. Only the agent-facing schema strings (lines 10, 24) were updated.
   • This is a documentation gap, not a functional one:
     • The runtime error states the cause.
     • Workspace targets still scan in print/JSON mode, and users can run the CLI directly.
     • Failing closed without a UI is intentional and matches Pi's permission-gate example.
   • Expected fix: add one short paragraph modelled on docs/OPENCODE_EXTENSION.md:43-46, saying:
     • Inputs inside the Pi session's working directory run without a prompt.
     • Remote targets and paths that resolve outside it open a dialog showing the canonical path or URL.
     • Print and JSON sessions reject them. Use TUI or RPC mode, move the skill into the working directory, or run the CLI.
   • Whether to add an operator opt-in for automation is a maintainer choice.
4. [Non-blocking] extensions/skillspector.ts:88: realpathSync runs on external paths before the approval check, so the agent can probe paths without approval.
   • I transcribed lines 69-104 and ran them with hasUI: false, which gave three outcomes the agent can tell apart, all without a prompt:
     • /etc/hosts → "require user confirmation"
     • /etc/definitely-not-here → ENOENT
     • /private/var/root/.ssh → EACCES
   • With a UI, a missing path fails silently, while an existing one opens a dialog the user sees.
   • This adds nothing in Pi's default configuration, where read and bash are enabled without per-call approval. It matters only in sessions where this tool is the only filesystem access, for example --tools skillspector_scan.
   • On Windows, realpath on a \\host\share\x target may contact the SMB host before the dialog. This is reasoning only and not measured.
   • Not a regression.
   • Expected fix: classify lexically first.
     • If resolve(ctx.cwd, value) is outside the workspace or is a UNC/device path, request approval (or reject when !hasUI) before any filesystem call. Then realpath and recheck.
     • Alternatively, map every pre-approval realpath error to the same generic error.
     • Add a test asserting that a missing external path with hasUI: false fails the same way as an existing one.
5. [Non-blocking] extensions/skillspector.ts:99: the confirm dialog is not tied to the tool's AbortSignal.
   • Lines 99-102 call ctx.ui.confirm(title, message) with no options. Pi's confirm(title, message, opts?) (types.ts:154) accepts a signal that dismisses the dialog.
   • Without it, an abort is only noticed at line 105, after someone answers the dialog. A canceled call never launches the CLI, so this is not a security issue.
   • In RPC mode, the abort command awaits session.abort(), which waits for the tool call to settle. The client therefore gets no response until it answers the stale confirm request.
     • In my transcription, without the signal the abort was still pending after 1.0 s. With it, the abort completed.
     • In the TUI, Esc still cancels the dialog; only programmatic aborts are affected.
   • The test at test_pi_extension.mjs:219-230 ("...after a canceled dialog") only settles because it calls approve(true) after controller.abort(). The mock confirm(title, message) at line 70 also ignores opts.
   • Expected fix:
     • Pass { signal }, and call signal?.throwIfAborted() before the "not approved" check so that a dismissed dialog reports an abort.
     • Make the mock honour opts.signal, and assert that the scan rejects after abort() without calling approve().
6. [Non-blocking] extensions/skillspector.ts:173: the output path is validated only after approval.
   • On main, reportOutputPath ran right after findSkillSpectorBin. The PR inserts approveScanInputs between them (lines 172-174), although reportOutputPath does not depend on the prepared inputs.
   • Transcription with an external target and output: "../report.json": one dialog, then "Report output must be a file within the current workspace." With the order swapped, no dialog appears.
   • Without a UI, the same input fails with the misleading "require user confirmation" error instead.
   • Expected fix:
     • Call reportOutputPath before approveScanInputs.
     • Keep mkdtempSync (line 175) after approval, so a denied dialog leaves no temp directory behind.
     • The post-scan recheck at lines 202-204 still catches changes made while the dialog is open.
7. [Non-blocking] extensions/skillspector.ts:108: some of the new branches have no test.
   • Workspace re-check (first half of line 108):
     • No test changes the workspace itself. In a transcription mutant without this check, every transcribed test still passed, and a workspace swap with a remote target launched the CLI.
     • A covering test needs a remote target and no yaraRulesDir; otherwise the per-input check catches the swap.
   • Pre-dialog abort (line 96): removing it still ends in an abort, but only after one needless dialog. A test must assert prompts.length === 0.
   • Missing target or yaraRulesDir (line 88):
     • Both now fail with Node's raw ENOENT before launch.
     • On main, a missing target gave the CLI's exit-2 "Cannot determine input type".
     • On main, a missing --yara-rules-dir only logged a warning, and the scan finished with exit 0. The YARA case is therefore a behaviour change.
     • Consider a clearer "not found" message, and decide whether the YARA case should keep warn-and-continue.

PIC tradeoffs:

• Workspace boundary. The workspace is realpath(ctx.cwd) alone. A Pi session started in $HOME or / therefore treats ~/.ssh and ~/.aws as workspace and scans them without a prompt. The OpenCode tool instead uses directory plus worktree, and ignores a filesystem-root worktree.
• Unattended scanning. Failing closed in print/JSON mode stops unattended remote scanning, which the tool's own guideline (line 167) encourages. There is no operator opt-in.
• Symlink policy. This PR canonicalises and confirms; the CLI and OpenCode refuse symlinked targets. The first lets symlinked layouts scan. The second keeps one policy across all three entry points (finding 2).
• Repeated prompts. Pi's confirm has no "remember" option, so repeated scans of the same external directory, or of several URLs, prompt every time.

Verification and gaps:

• Baseline on main:
  • I ran the CLI with the extension's arguments for an external file: exit 0, with the marker twice in the report.
  • A transcription of isLikelyUrl showed ../private.md, ./SKILL.md and external/private.md passing through unchanged.
  • main follows --yara-rules-dir symlinks (cli.py:737).
• PR logic, checked by code trace and by my own Python transcriptions of approveScanInputs, the execute order and Pi's RPC confirm/abort path:
  • Remote classification matches the CLI on 14 inputs.
  • Noninteractive sessions reject before any launch.
  • The post-dialog realpath recheck, together with the CLI's no-follow reads, covers swaps made during the dialog.
  • For the target, the approved scope matches what the CLI reads, apart from finding 1. The directory walk does not follow symlinks, and the extension never passes --transitive.
• Reproductions: findings 1 and 2 were reproduced with main's real CLI. cli.py, input_handler.py and static_yara.py are identical on the head and main.
• Tests:
  • By trace, every new or changed case in tests/unit/test_pi_extension.mjs would fail on main, where nothing prompts or rejects and owner/repo and ./SKILL.md are sent unchanged.
  • The tests exercise the real extension source; only pi.exec, ctx.ui.confirm and the schema helpers are mocked.
  • Gaps are in findings 5 and 7. CI runs this file on Node 22.20.0 (ci.yml:106).
• CI: no checks have run. The workflow run on 511b215 is action_required (waiting for a maintainer), so the PR description's "88 Pi/OpenCode tests passed" is unverified, and CI must pass before merge.
• Conflicts: none. The PR is current with main (a0d489e), and no other PR in this batch touches extensions/ or the Pi test.
• Head update: I reviewed 511b215e77db39f8c7b03480ec391c7c92641628. The current head 14861342a4d78fd52867c548974fbaec0952fdcc only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files; #608 adds code to static_yara.py, so the yara.compile references below are to the current head (still no includes=False). The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Unverified areas:
  • Pi behaviour (hasUI per mode, confirm options, abort handling) comes from earendil-works/pi source on its default branch, read with GET requests only. Pi itself was not run.
  • Windows behaviour (UNC paths, junctions) was assessed by reasoning only, and CI runs the symlink tests only on Linux.
  • The YARA demonstration used Python 3.13 with yara-python 4.5.4; CI uses 3.12.

---

Decision: Approved (reviewed head 511b215e77db39f8c7b03480ec391c7c92641628; current head 14861342a4d78fd52867c548974fbaec0952fdcc only adds merges of main)

[yashrajp22]

Thanks for the careful review! I closed the remaining gaps: external paths are approved before they are resolved, local targets keep the CLI’s symlink checks, and YARA includes can no longer read files outside the selected rules. Canceling now closes the confirmation dialog, invalid output paths fail before prompting, and the docs explain headless sessions and missing rule directories. I also added checks for canceled calls, workspace swaps, external-path probing, and absolute/relative YARA includes.