fix(cap-discord): expand ~/ in tokenFile path before reading#57
Conversation
readToken() passed the configured tokenFile path literally to fs.readFileSync, which does NOT expand a leading ~/ (only the shell does). A bob.yaml with `tokenFile: ~/agents/<name>/.pi-agent/discord.token` therefore threw ENOENT. Because this throws at the top of the cap-discord factory and pi's extension loader swallows extension-load errors, the discord capability silently never loaded — no gateway, no error. The config schema already documents "Absolute (or ~-relative) path" as supported, a contract readToken did not honor. cap-flair (client.ts) and cap-observatory (reader.ts) already expand ~/; cap-discord was the only file-reading capability missing it. This mirrors their convention exactly. Found in the Rivet Flair/Discord dogfood. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tps-sherlock
left a comment
There was a problem hiding this comment.
Security Review — APPROVE ✅
This is a path resolution fix for readToken() in cap-discord. No security regression, no new attack surface.
What's happening
readToken() passed the configured tokenFile path literally to fs.readFileSync. Node's fs does NOT expand ~/ (only the shell does), so a config with tokenFile: ~/agents/rivet/.pi-agent/discord.token threw ENOENT. Because pi's extension loader swallows extension-load errors, the discord capability silently never loaded.
The fix expands a leading ~/ to homedir() before reading:
const resolvedPath = tokenFile.startsWith("~/")
? `${homedir()}/${tokenFile.slice(2)}`
: tokenFile;This mirrors the existing convention in cap-flair (client.ts) and cap-observatory (reader.ts), which already expand ~/. cap-discord was the only file-reading capability missing it.
Security assessment
No new risk. The ~/ expansion uses os.homedir(), a standard Node API. It only applies to paths starting with ~/ — absolute paths pass through unchanged. There's no arbitrary expansion, no path traversal vector. The tokenFile value still comes from config; this just makes the config schema's documented contract ("Absolute (or ~-relative) path") actually work.
Error messages use the resolved path. The empty-file error now references resolvedPath instead of the raw tokenFile. This is correct — the error should show the actual path that failed, not the unexpanded ~/ form. No token leakage — the existing test already verifies tokens aren't echoed in error messages.
Tests are clean. Two new tests use the injected read seam to capture the path actually opened (never a real token file):
~/-prefixed path resolves to${homedir()}/...and the~/prefix is stripped- Absolute path is left unchanged
CI: Build (TypeScript strict) green, Socket Supply Chain green. Dependency Audit failure is the same pre-existing one from #56 (pi-coding-agent/protobufjs/undici advisories with no upstream fixes — not this PR's scope). Unit Tests and CodeQL still in progress at review time but build passed so no reason to expect failures.
Verdict
APPROVE. Simple, correct fix that brings cap-discord in line with cap-flair and cap-observatory. No security regression, no new attack surface, well-tested.
tps-kern
left a comment
There was a problem hiding this comment.
Review: fix(cap-discord): expand ~/ in tokenFile path before reading
Verdict: APPROVE
Architecture & Correctness
Straightforward fix. Node's fs module does not expand ~/ (that's a shell convention), so a bob.yaml with tokenFile: ~/agents/.../discord.token would throw ENOENT and silently kill the cap-discord extension (pi's loader swallows extension-load errors).
The fix is correct:
tokenFile.startsWith("~/")→${homedir()}/${tokenFile.slice(2)}— standard tilde expansion, matches the pattern already incap-flair(client.ts) andcap-observatory(reader.ts)- Absolute paths pass through unchanged
- Error messages updated to reference
resolvedPath— good, this shows the actual path that failed, not the unexpanded shorthand
The injected read seam is preserved — readToken accepts a read function parameter, so tests can capture the path without touching real files. Clean.
Tests
Two new tests:
~/-prefixed path resolves to${homedir()}/...— verifies the expansion- Absolute path left unchanged — verifies no over-expansion
Both use the read seam to capture opened path. Good coverage of the fix without requiring real files.
CI
- Build (TypeScript strict): ✓
- Dependency Audit: ✗ — pre-existing on
main(same Socket.dev advisory check failing as PR #56; out of scope for this PR) - Unit Tests and CodeQL still in progress at review time, but the change is minimal and the build already passed
Consistency
This brings cap-discord in line with the tilde-expansion convention already established in cap-flair and cap-observatory. Good consistency across capabilities.
Ship it.
tps-kern
left a comment
There was a problem hiding this comment.
Review — architecture + correctness lane
Verdict: APPROVE
Bug analysis ✅
readToken() passed tokenFile literally to fs.readFileSync. Node's fs does NOT expand ~/ (only the shell does), so a config with tokenFile: ~/agents/.../discord.token threw ENOENT. Because this throws at the top of the cap-discord factory and pi's extension loader swallows extension-load errors, the discord capability silently never loaded — no gateway, no error. The config schema already documented "Absolute (or ~-relative) path" as supported — an unfulfilled contract.
Fix architecture ✅
- Simple, correct expansion:
tokenFile.startsWith("~/")→homedir() + "/" + tokenFile.slice(2). Otherwise pass through unchanged. - Error messages now reference
resolvedPathinstead of the rawtokenFile— better debugging for ENOENT cases, no secret exposure risk change. - Mirrors the existing convention in
cap-flair(client.ts) andcap-observatory(reader.ts). cap-discord was the only file-reading capability missing this. Consistency restored. - The injected
readseam is preserved, keeping the function testable.
Test coverage ✅
- New test:
~/-prefixed path resolves to${homedir()}/...— verified via thereadseam capturing the actual path opened. - New test: absolute path left unchanged.
- Both use the seam (never touch a real token file).
- Existing "never echoes token" test still passes (confirmed via green Unit Tests CI).
CI ✅
- Build (TypeScript strict): pass
- Unit Tests: pass (40/0)
- CodeQL: pass
- Socket.dev: pass
- Dependency Audit: fail — pre-existing (same pi-coding-agent/protobufjs/undici advisories from PR #56; not introduced by this change).
Minor note (non-blocking)
~user/ (tilde with username) is not handled, but that's an extremely rare use case, not documented in the schema, and not used in any known config. ~/ is the standard form.
— Kern 📐
The bug
readToken()inpackages/cap-discord/src/config.tspassed the configuredtokenFilepath literally tofs.readFileSync. Node'sfsdoes NOT expand a leading~/(only the shell does), so abob.yamlwith:threw
ENOENT: ... open '~/agents/...'.Because this throws at the very top of the cap-discord factory (
index.ts), and pi's extension loader swallows extension-load errors, the discord capability silently never loaded — no gateway, no error, nothing. (Cost a long debug.)The config schema docstring already documents "Absolute (or ~-relative) path" as supported — a contract
readTokendidn't honor.The fix
Expand a leading
~/tohomedir()before reading, and reference the resolved path in the empty-file / error messages. This mirrors the existing convention incap-flair(client.ts~L90) andcap-observatory(reader.ts~L24), which already expand~/. cap-discord was the only file-reading capability missing it.Tests
Two unit tests added to
test/config.test.ts(they use the injectedreadseam to capture the path actually opened — never a real token file):~/-prefixedtokenFileresolves to${homedir()}/...bun test(40 pass / 0 fail),bun run typecheck, andbun run buildall green for cap-discord.Found in the Rivet Flair/Discord dogfood; matches the cap-flair/cap-observatory tilde handling.
🤖 Generated with Claude Code