fix(ai-vault): fall back to default Pi/OMP session roots for non-absolute env values

[smwbev]

Summary

normalizeAgentSessionsDir returned non-empty, non-absolute values unchanged, so a degenerate PI_CODING_AGENT_DIR / OMP_CODING_AGENT_DIR — rel/path, ., .., or the drive-relative C:/C:foo a Windows drive value strips to — became a scan root that resolves against the main-process cwd. walkSessionFiles has no depth, entry, or time caps, so that walk recursively scans whatever the cwd happens to be, on every scan cycle (measured in review: ~5.8s on a repo checkout, >20s without finishing at /, where Finder/Dock-launched apps start).

Filesystem-root values were already handled before this PR: / strips to '', which the OMP allowlist filter in session-scanner-roots.ts drops, and fs calls on '' fail with ENOENT — an empty scan, not a cwd walk. The genuine fix here is the non-empty, non-absolute forms; the empty-string forms now land on the same default root instead of relying on per-call-site filtering.

This was flagged during review of #12935 as pre-existing and out of scope there ("One thing for a separate PR"); this is that PR.

env value	before	after
/agents/.pi	/agents/.pi/agent/sessions	unchanged
rel/path, ., ..	cwd-relative scan root — the actual bug	default ~/.pi/agent/sessions
C:\, C:foo	drive-relative C: → cwd-on-drive scan root	default
/	'' — already dropped by the OMP allowlist filter; empty scan elsewhere	default
/.., /a/..	resolves to /	unchanged — absolute; pre-existing on main, out of scope here

What changed

One syntactic guard after trim + trailing-separator strip: any non-absolute result returns the default <home>/<agentHomeDirName>/agent/sessions root. isAbsolute('') is false on both posix and win32, so the guard is the single head of the function — the previous empty-string early returns were implied by it and are collapsed. Absolute prefix forms (…/.pi, …/agent, …/sessions) behave exactly as before, including absolute values that resolve to a filesystem root (/..) — rejecting those would be a behavior change beyond the flagged leak, and the code comment now says so explicitly.

Screenshots

No visual change.

Testing

• pnpm lint
• pnpm typecheck — the sole failure is the known pre-existing psl TS2307 module-resolution issue, reproducible on upstream/main; no errors in the changed files. pnpm build stops at that same typecheck step on this machine (it passed on the original head before that environment issue).
• npx vitest run on session-scanner-values.test.ts + session-scanner-roots.test.ts: 10/10 green
• Added or updated high-quality tests that would catch regressions

The degenerate-shape test pins every non-absolute form (/, //, whitespace, ., .., rel/path, C:\, C:/, C:foo) to the default fallback for both .pi and .omp, and now carries positive controls: an absolute custom root survives the guard unchanged, and untrimmed whitespace around an absolute root must not demote it to the fallback (kills the drop-.trim() mutation, which previously passed the suite). The existing prefix-expansion cases pin the unchanged branches.

AI Review Report

Reviewed the change against both path.posix and path.win32 semantics: every listed degenerate value is non-absolute under both, so the guard behaves identically on macOS, Linux, and Windows; genuine absolute roots (POSIX, drive-qualified C:\Users\…, UNC \\server\share\…) pass isAbsolute and keep their previous behavior. The guard checks the normalized string and returns the original — it never resolve()s the return value, since path.win32.resolve would introduce backslashes and remotePiSessionsSegments() splits on /. The one intentional divergence: a pathological env value now lands on the default root instead of the cwd — same trade the #12935 review accepted for Prime Agent.

Security Audit

• Input handling: the env value is user-owned configuration; the change narrows what it can do (no new parsing, no execution). The cwd-relative scan-root leak this closes was a read-only scan surface plus an unbounded-walk cost, not an exploit path.
• Known remainder: an absolute value that resolves to a filesystem root (/..) still passes the syntactic guard and reaches the renderer-path allowlist — identical on main, surfaced in review, deliberately not addressed here (see Notes).
• No new surface: no IPC, network, dependencies, or behavior changes beyond the fallback.

Notes

• Tilde expansion is deliberately not added here: Pi and OMP consume their env vars without it today, and adding it would be a behavior change beyond the flagged leak.
• Rejecting absolute-but-root values (isFilesystemRoot(resolve(normalized))) and emitting a scan notice when an override is rejected were both suggested in review; they are behavior additions beyond this PR's scope and overlap with the systemic sweep proposed in AI Vault: six env-derived scan roots accept relative values and walk the process cwd unbounded #13082, so they're left as separate decisions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8ec1f835-be16-4783-8bc7-8a7850e59586

📥 Commits

Reviewing files that changed from the base of the PR and between 600b0b1 and e50e2c5.

📒 Files selected for processing (2)

• src/main/ai-vault/session-scanner-values.test.ts
• src/main/ai-vault/session-scanner-values.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/ai-vault/session-scanner-values.test.ts
• src/main/ai-vault/session-scanner-values.ts

---

📝 Walkthrough

Walkthrough

normalizeAgentSessionsDir now uses isAbsolute to reject non-absolute values after normalization. Empty and non-absolute inputs fall back to user-home-based session directories. Tests cover fallback behavior for .pi and .omp values and preserve absolute custom roots.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fallback change for non-absolute Pi and OMP session roots.
Description check	✅ Passed	The description covers the required summary, visual change, testing, AI review, security audit, and platform-specific notes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Jinwoo-H]

Thanks for carrying this over from #12935 — the relative-path case is real and worth fixing.

Reviewed against latest main, reproduced end to end, and validated in the running app. Notes below; nothing here says don't land it, but two claims in the description need correcting and there's one gap worth deciding on.

The description's headline example isn't the bug

OMP_CODING_AGENT_DIR=/ was already handled before this PR. It normalizes to '', and ompSessionsRootDirs already drops empty roots via .filter(rootDir => rootDir.trim().length > 0), so the allowlist got []. I ran the real ompSessionsRootDirs against origin/main and this branch side by side:

env value	origin/main	this branch
/	roots=[] — already safe	default root
rel/path	<cwd>/rel/path — the actual bug	default root
/.., /., /a/..	resolves to /	resolves to / (unchanged)

So the genuine fix is the non-empty, non-absolute values (., .., rel/path, C:foo). Worth rewording the body and the table, since / reads as the motivating case and isn't.

It's also a bigger win than "an information-surface quirk"

walkSessionFiles has no depth cap, entry cap, or time budget. A relative value made it recursively walk the main-process cwd: 5.8s on a repo checkout, and >20s without finishing at / (where Finder/Dock-launched apps start), re-running each scan cycle. That's the strongest argument for the change.

The guard is syntactic, so /.. still passes

isAbsolute('/..') is true, so it clears the guard and resolve()s to /. Since that root feeds the renderer-path allowlist in src/main/ipc/ai-vault.ts, every path clears the gate — I confirmed /etc/ssh/… and ~/.ssh/id_rsa… would both be accepted. A wrapper doing OMP_CODING_AGENT_DIR="$AGENT_ROOT/.." with AGENT_ROOT unset produces exactly that.

This is pre-existing — identical on origin/main — so it's not a regression here. But the comments added in this diff assert an invariant that isn't established. Either soften them or close it:

if (!isAbsolute(normalized) || isFilesystemRoot(resolve(normalized))) {
  return join(homedir(), agentHomeDirName, 'agent', 'sessions')
}
// where isFilesystemRoot(p) is p === parse(p).root

Important: resolve for the check only, and keep returning the original string. path.win32.resolve('/.pi/agent/sessions') yields backslashes, and remotePiSessionsSegments() does .split('/') — resolving the return value would break remote scanning from a Windows client.

Two dead conditions

isAbsolute('') is false on both posix and win32, so !normalized || is implied by the isAbsolute half, and that in turn makes the pre-existing if (!trimmed) early return unreachable. The whole head collapses to:

const normalized = rawValue.trim().replace(/[\\/]+$/, '')
if (!isAbsolute(normalized)) {
  return join(homedir(), agentHomeDirName, 'agent', 'sessions')
}

Verified equivalent across 264 input × platform × agent combinations. It also drops the eager join(homedir(), …) hoist, and makes the ' ' case in your new test non-vacuous — today it hits the removed early return and never reaches the guard.

Test notes

• The new it.each cases all assert the same default root, so they'd pass even if OMP_CODING_AGENT_DIR support were deleted. A positive control (/custom/omp → ['/custom/omp']) fixes that; verified it fails if env support is removed.
• An 8-mutation battery leaves one survivor: dropping .trim() passes all tests. ' /agents/.pi ' would silently downgrade to the default. Unreachable today (all four callers pre-trim), but one assertion closes it.

Silent fallback

A rejected override is currently invisible — a typo'd value looks like it worked. AiVaultScanIssue already supports kind: 'notice' and the issues array is threaded into the discovery sites, so this is a small addition if you want it.

Validated

typecheck, lint, and the changed-code quality gate clean (0 findings). 426/426 tests across src/main/ai-vault + src/main/native-chat. Verified in a dev build with OMP_CODING_AGENT_DIR=/ and =rel/path: both fall back correctly, sessions render normally, full rescan 579ms.

Filing the sibling-agent follow-up (CODEX_HOME, COPILOT_HOME, OPENCLAW_STATE_DIR, DEVIN_HOME, KIMI_CODE_HOME, GROK_HOME share the identical shape) separately.

[smwbev]

Thanks for the thorough pass — reproductions confirmed on this end, and the actionable items are applied in 946647b. Point by point:

• Headline example: you're right — / strips to '', which the OMP allowlist filter already dropped, and fs calls on '' fail with ENOENT, so it never produced a cwd walk. The PR body is rewritten around the actual bug (non-empty, non-absolute values) with your before/after table shape, and the unbounded-walk cost is now the stated motivation.
• Dead conditions: confirmed (isAbsolute('') is false on both posix and win32) — the head is collapsed to the single isAbsolute guard exactly as you sketched, which also drops the eager fallback hoist and makes the ' ' test case reach the guard.
• /.. passing the syntactic guard: confirmed pre-existing on main. This PR's comment no longer implies the guard establishes a safe-root invariant — it now states explicitly that absolute values resolving to a filesystem root pass through unchanged. The isFilesystemRoot(resolve(…)) tightening (check-only resolve, return the original string — agreed on the win32/split('/') hazard) is a behavior addition beyond this PR's flagged leak and overlaps with your AI Vault: six env-derived scan roots accept relative values and walk the process cwd unbounded #13082 sweep, so it's left as a separate decision rather than folded in here; same for the rejected-override scan notice. Both are recorded in the PR body's Notes.
• Tests: both gaps closed — a positive control pins an absolute custom root surviving the guard unchanged, and a whitespace-wrapped absolute root pins .trim() (verified it fails with .trim() dropped, so the mutation survivor is dead).

Validation on the new head: pnpm lint clean, targeted vitest 10/10 (session-scanner-values + session-scanner-roots), pnpm typecheck's only failure is the known pre-existing psl TS2307 reproducible on upstream/main.

[AmethystLiang]

thanks for reporting. looking into it

[AmethystLiang]

thanks for reporting. looking into it

[smwbev]

Cross-referencing for whoever picks this up: #13118 landed in the meantime, implementing the #13082 sweep (Codex, Copilot, OpenClaw, Devin, Kimi, Grok) with a shared resolveAbsoluteDirOverride helper, using this PR as the template.

I checked how the two interact so it doesn't have to be re-derived at review time:

• No conflict with this PR. git merge-tree between the two branches is clean — fix(ai-vault): ignore non-absolute env overrides for agent scan roots #13118 touches session-scanner-agent-sources.ts, session-scanner-kimi-paths.ts, shared/grok-session-paths.ts and the new helper; this PR only touches session-scanner-values.ts and its test. Either order of merge works.
• Prime Agent isn't in the sweep and doesn't need to be — PRIME_AGENT_CODING_AGENT_DIR isn't on main yet, and in feat(agents): add Prime Agent as a supported TUI agent with session history #12935 it resolves through normalizeAgentSessionsDir, which this PR gives the same guard.

Pre-empting the obvious follow-up — should Pi/OMP just use the shared helper too? Not as a drop-in, because the order of operations differs and the difference is load-bearing. normalizeAgentSessionsDir trims, then strips trailing separators, then checks isAbsolute — that ordering is what maps C:\ to the drive-relative C: and rejects it. resolveAbsoluteDirOverride checks isAbsolute on the raw value, where C:\ is absolute and passes through. It also has the agent/sessions leaf expansion that the generic helper has no reason to carry. Delegating just the head of the function to the helper after #13118 merges would be a reasonable cleanup, but it is a behavior change for the drive-root case rather than a refactor, so I've left it out of this PR. Happy to do it as a follow-up if you'd prefer one code path.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[smwbev]

Gentle status ping — no urgency intended, and no action needed if this is simply queued.

This PR was split out of the #12935 review at the reviewer's request (the pi/OMP normalizer fix: non-absolute env values should fall back to the default root rather than being treated as a path). It has been green since 9 Aug — mergeable, CodeRabbit's pass closed with nothing outstanding, and the actionable points from @Jinwoo-H's 7 Aug review were applied in 946647b. There has been no review decision since.

Two things have changed since it was opened that may be worth weighing:

• The integration it was split from has shipped. feat(agents): add Prime Agent as a supported TUI agent with session history #12935 reached the stable channel in v1.4.182 and is in v1.4.184 today, so the normalizer this PR fixes is now running in released builds.
• The parallel sweep over the same normalizer family has stalled: AI Vault: six env-derived scan roots accept relative values and walk the process cwd unbounded #13082 (P0) has had no activity since 8 Aug, and fix(ai-vault): ignore non-absolute env overrides for agent scan roots #13118, which implements it, has been open without a merge since 10 Aug. This PR touches only session-scanner-values.ts and its test, so it does not conflict with either — verified again today.

If it is waiting on reviewer assignment rather than on the change itself, the cause looks structural: .github/CODEOWNERS currently has four rules, all under /src/renderer/src/i18n/ and /config/, with no catch-all — so nothing under /src/main/ai-vault/ gets an automatic reviewer. That is the same gap I noted for /src/shared/plugins/ in #14121.

Happy to rebase or re-run verification if anything has gone stale in the meantime.

[smwbev]

@nwparker — a process question rather than a nudge on this PR specifically, since the pattern now spans five PRs and two different causes.

This PR has no reviewer at all. Opened 7 Aug (34 days), green since 9 Aug, one substantive review from @Jinwoo-H on 7 Aug whose points were applied in 946647b, and no review decision since. .github/CODEOWNERS has grown to eight rules — /src/renderer/src/i18n/locales/, three /config/ entries, and the four /cloud/ entries added recently — but nothing covers /src/main/ai-vault/, and there is no catch-all. So this PR never got an automatic assignment.

The other four have a reviewer and are still waiting:

PR	opened	reviewer requested	days
#12505	4 Aug	@brennanb2025, 6 Aug (by @AmethystLiang)	35
#12533	4 Aug	@brennanb2025, 6 Aug (by @AmethystLiang)	35
#14121	12 Aug	@brennanb2025, 13 Aug (by @AmethystLiang)	28
#14127	13 Aug	@brennanb2025, 13 Aug	28

None of the five has a human review decision. All five are MERGEABLE today — I rebased #12505 onto current main yesterday after upstream replaced the bundled English catalog with en-runtime-required.json, and re-ran typecheck and lint clean.

I am not asking anyone to prioritise these over their own work, and I realise a reviewer being busy is not a defect. Two concrete things would unblock the queue from my side:

1. A CODEOWNERS entry for /src/main/ai-vault/ (and, if it matches how you route things, /src/shared/plugins/ — that is where fix(plugins): reject locale ids Intl cannot parse #12533 and feat(plugins): let language packs translate the install form and log controls #14121 live) so this class of PR gets an owner automatically instead of relying on someone noticing.
2. If @brennanb2025 is not the right router for the four above, a redirect to whoever is. A reassignment is more useful to me than an approval — I would rather the work sit with the right person than accumulate quietly.

Happy to split, rebase, or close any of them if they are not wanted; a "no" is a perfectly good answer and costs me nothing to act on.

[nwparker]

Superseded by merged 170dbdb / #13118 (fix(ai-vault): ignore non-absolute env overrides for agent scan roots), which generalizes the same OMP/Pi path-safety fix. Closing the stale branch.