POK-46: Normalize Windows subtree scope paths

[HYT-koota]

Summary

Implements the filesystem subtree scope normalization piece split out as POK-46 from the Windows compatibility work in PR #39.

• Adds a shared appendFsSubtreeSuffix() helper that always emits the stable internal target/** form instead of platform-specific target\**.
• Accepts Windows-style trailing \** filesystem scope inputs and normalizes them to the internal /** suffix.
• Replaces subtree scope construction in policy baselines, request permission expansion, and subagent initial path scopes.
• Keeps filesystem root subtree scopes rejected, including Windows roots such as C:\**.

Validation

• pnpm test -- tests/security/scope.test.ts tests/tools/permission-block.test.ts
• pnpm build
• pnpm exec biome lint --error-on-warnings src/security/scope.ts src/security/policy.ts src/tools/helpers/permission-block.ts src/orchestration/subagents.ts tests/security/scope.test.ts tests/tools/permission-block.test.ts

Note: full pnpm test / pre-commit preflight still fail on native Windows for existing platform issues unrelated to this PR, including POSIX path expectations, symlink permission failures, and sandbox-runtime unsupported-on-Windows paths.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3de4f8115d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Expand Windows home-relative hard-denies

On Windows, this default is built from path.join("~", ".ssh"), which produces ~\.ssh, and the new subtree helper turns it into ~\.ssh/**. normalizePolicyPath only expands ~/ and not ~\, so this hard-deny normalizes relative to process.cwd instead of the user's home; reads/writes under C:\Users\<user>\.ssh (and the same home-relative secret dirs below) are therefore not blocked by the system policy on Windows. Build these defaults with a form expandHomePrefix handles, or teach it to expand ~\.

Useful? React with 👍 / 👎.

[danielwpz]

@claude help me review

[claude]

Claude finished @danielwpz's task in 1m 37s —— View job

---

PR Review: POK-46 Windows Subtree Scope Paths

Summary

The implementation is solid overall. Here's my detailed analysis:

---

What's Done Well

1. Centralized appendFsSubtreeSuffix() helper (src/security/scope.ts:89-94) - Consolidates subtree suffix logic and ensures consistent use of the stable /** internal form across all call sites.

2. Cross-platform path handling - Functions like isPathSeparator(), isAbsoluteFsPath(), and isFsRootPath() correctly handle both POSIX and Windows paths using dual path.isAbsolute() / path.win32.isAbsolute() checks.

3. Proper Windows root rejection - The test at tests/security/scope.test.ts:74-78 confirms C:\** is correctly rejected as a filesystem root subtree.

4. Consistent normalization - All four call sites were updated:

   • src/security/policy.ts (policy baselines)
   • src/orchestration/subagents.ts (toSubtreePath())
   • src/tools/helpers/permission-block.ts (expandPermissionEntriesToScopes())

5. Test coverage - New tests in both tests/security/scope.test.ts and tests/tools/permission-block.test.ts cover the key scenarios.

---

Issues Found

1. Dead code: subtree() function removed but not deleted

File: src/security/policy.ts

The local subtree() helper function was replaced but the empty function definition was not removed from the diff. Looking at the diff, the old subtree() function is removed, which is correct - but you should verify the file is clean.

Fix: No fix needed - the function was properly removed.

---

2. P1: Windows home-relative hard-denies not expanded ⚠️

File: src/security/permissions.ts:60-68

This was also flagged by the automated review and is a legitimate concern.

The expandHomePrefix() function only expands ~ and ~/:

function expandHomePrefix(value: string): string {
  if (value === "~") {
    return process.env.HOME ?? value;
  }
  if (value.startsWith("~/")) {  // ← Only handles forward slash
    const homeDir = process.env.HOME;
    return homeDir == null ? value : path.join(homeDir, value.slice(2));
  }
  return value;
}

On Windows, path.join("~", ".ssh") produces ~\.ssh, which expandHomePrefix does not expand. This means:

• Default hard-denies like ~\.ssh/**, ~\.gnupg/**, etc. remain unexpanded
• They get resolved relative to process.cwd() instead of the user's home
• Security-sensitive directories under the actual home path are not protected

Impact: On Windows, reads/writes to C:\Users\<user>\.ssh would not be blocked by the system policy.

Fix needed: Update expandHomePrefix() to also handle ~\:

function expandHomePrefix(value: string): string {
  if (value === "~" || value === "~\\" || value === "~/") {
    return process.env.HOME ?? value;
  }
  if (value.startsWith("~/")) {
    const homeDir = process.env.HOME;
    return homeDir == null ? value : path.join(homeDir, value.slice(2));
  }
  if (value.startsWith("~\\")) {
    const homeDir = process.env.HOME;
    return homeDir == null ? value : path.join(homeDir, value.slice(2));
  }
  return value;
}

Or more elegantly with a regex that handles both separators.

Note: This is an existing bug that predates this PR, but the PR exposes it more clearly since appendFsSubtreeSuffix() now preserves the Windows-style paths from path.join("~", ".ssh").

---

Minor Observations

1. Constant naming (src/security/scope.ts:48): SUBTREE_MARKER = "**" is good, but consider if SUBTREE_SUFFIX and SUBTREE_MARKER names could be confused (one is /**, the other is just **).

2. Test on POSIX paths (tests/security/scope.test.ts:48-58): The tests verify Windows paths (C:\...) but not POSIX paths (/home/...). Consider adding:

   expect(appendFsSubtreeSuffix("/home/user/project")).toBe("/home/user/project/**");

---

Verdict

Approve with concerns. The core implementation is correct and well-tested. The Windows home-relative expansion issue should be addressed before merging, as it represents a real security gap on Windows systems. This could be fixed in a follow-up PR or as part of this one.

Suggested Fix

Fix expandHomePrefix to handle Windows home paths →

---

Validation Commands

pnpm test -- tests/security/scope.test.ts tests/tools/permission-block.test.ts
pnpm build
pnpm exec biome lint --error-on-warnings src/security/scope.ts src/security/policy.ts src/tools/helpers/permission-block.ts src/orchestration/subagents.ts tests/security/scope.test.ts tests/tools/permission-block.test.ts

[HYT-koota]

@claude Please take another pass on this Windows subtree-scope PR against the current upstream main. Focus on whether the subtree scope normalization is still correct with the latest code, and identify any concrete follow-up changes needed on this branch.

[danielwpz]

@claude Please review this Windows subtree-scope PR again. This is a maintainer-triggered retry after the Claude workflow routing change; treat this comment as the review request. Focus on whether Windows subtree scope normalization is still correct against current main, and call out any concrete code or test changes needed on this branch.

[claude]

Review: POK-46 Windows subtree scope normalization

1. No blocking correctness issues. The normalization is applied comprehensively at every entry point: validateFsScopePath (scope parsing), appendFsSubtreeSuffix (policy baselines, hard-denies, subagent paths, permission-block expansion). All subtree paths are now stored internally as base/** regardless of input separator style, and the logic in getFsSubtreeBase, isFsRootPath, and stripTrailingFsSeparatorsPreservingRoot correctly handles the relevant edge cases (trailing slashes, root rejection for both POSIX / and Windows C:\, idempotency).

2. Unchanged downstream consumers are safe. Files that weren't touched (sandbox.ts:132-133 and request-permissions.ts:282) still use endsWith("/**") and slice(0, -3) to detect and extract subtree bases. They remain correct because the PR ensures all subtree paths reaching them are /**-terminated — the normalization happens upstream at parse/construction time.

3. slice(0, -3) extraction is scattered across 5 files (permissions.ts:74,90, permission-block.ts:184,280, sandbox.ts:133, request-permissions.ts:282). Both /** and \** happen to be 3 characters, so the slice works regardless of separator, but the coupling between isFsSubtreeScopePath (which now accepts both separators) and the hardcoded -3 is implicit. Not a bug today, but if the subtree marker format ever changed, these sites would silently break. A follow-up that centralizes suffix stripping into a single helper (e.g., exporting getFsSubtreeBase and consuming it everywhere) would make the invariant explicit.

4. Test coverage is solid at the unit level but doesn't include an end-to-end integration test for the Windows path flow: parse a Windows-style subtree scope → normalize → run through buildEffectivePermissions / checkFilesystemPermission → verify the match. The current tests validate each layer in isolation, which is adequate for correctness, but an integration-level test would catch regressions if normalization is ever bypassed.

5. ~\\ home expansion (permissions.ts:64) is a good addition for Windows, though it depends on HOME being set. On native Windows, USERPROFILE is the more conventional env var for the home directory; HOME may not always be set. This isn't a regression (the existing code also only checked HOME), but is worth tracking as a follow-up if native Windows support deepens.

Verdict: Mergeable. The changes are correct, well-scoped, and the normalization chain is complete. The concerns above are maintainability/coverage notes, not correctness blockers.

[HYT-koota]

@claude Please take another pass on this Windows subtree-scope PR now that I have collaborator access. Focus on whether the subtree scope normalization is still correct with the latest code, and identify any concrete follow-up changes needed on this branch.

[claude]

Review: POK-46 Windows subtree scope normalization (second pass)

The core normalization is correct and complete. Every entry point normalizes to /**, and the prior review's main concern (~\\ home expansion) has been addressed. I have one concrete follow-up item and one minor observation.

---

1. Subtree normalization chain is complete and correct. validateFsScopePath normalizes Windows \** inputs to /** at parse time. appendFsSubtreeSuffix ensures all programmatic construction (policy baselines, hard-denies, subagent paths, permission-block expansion) uses the stable /** suffix. normalizePolicyPath at permissions.ts:73-75 re-normalizes after home expansion. Every downstream consumer that uses endsWith("/**") / slice(0, -3) — sandbox.ts, request-permissions.ts, permission-block.ts — receives only /**-terminated paths, so the hardcoded -3 remains correct.

2. The ~\\ home expansion fix in expandHomePrefix resolves the prior review's main concern. On Windows, path.join("~", ".ssh") produces ~\.ssh, and the new startsWith("~\\") check correctly expands it. The test in permissions.test.ts validates this end-to-end.

3. Concrete follow-up: centralize subtree base extraction. The slice(0, -3) pattern is scattered across 5 files: permissions.ts:74,90, sandbox.ts:133, permission-block.ts:184, request-permissions.ts:282. These implicitly depend on the suffix being exactly 3 characters long. Since getFsSubtreeBase in scope.ts already correctly strips the suffix regardless of separator style, the cleanest fix is to export getFsSubtreeBase from scope.ts and replace every isFsSubtreeScopePath(p) ? p.slice(0, -3) : p with getFsSubtreeBase(p) ?? p. This makes the invariant explicit instead of relying on the upstream normalization guarantee. Not a blocker — nothing breaks today — but it eliminates the implicit coupling and is a low-risk cleanup for this branch.

4. HOME vs USERPROFILE on native Windows is an existing limitation, not a regression. expandHomePrefix only reads process.env.HOME, and on native Windows, USERPROFILE is the more conventional env var. This predates the PR but is worth tracking if native Windows support is a goal.

Verdict: Mergeable. The normalization is correct, the previously flagged ~\\ gap is closed, and the one recommended follow-up (exporting getFsSubtreeBase) is a low-risk cleanup rather than a correctness fix.

[danielwpz]

Added a collaborator follow-up commit 8e23f05 to keep the Windows home hard-deny test aligned with runtime path normalization. This preserves the original contributor commits and only fixes the macOS realpath-sensitive assertion. Validation: local preflight passed on the PR branch; merged the updated PR head into current main in a temporary worktree with no conflicts; targeted security tests, build, and related lint all passed. GitHub CI is now green for lint/build, linux tests, and macOS tests.