refactor: extract shared CLI utils, types, tool handlers, and constants

[rsdouglas]

Summary

• Extract shared CLI utilities into src/cli/cli-utils.ts — cliError, requireConfig, resolveEnvVar, parseEnvMap, handleCommandError, getErrorMessage removed from 14 command files, replaced with imports. Adds cli-utils.test.ts.
• Centralize shared types and constants in src/core/types.ts — APIRequest, APIResponse, DenialError/DenialDetails, DEFAULT_TIMEOUT_MS, REDACTED, MIN_SCRUB_LENGTH. Adds SerializedSession to core/sessions.ts.
• Extract MCP tool handlers into src/core/tool-handlers.ts — 6 handlers (execute, janee_exec, manage_credential, test_service, explain_access, whoami) pulled out of the monolithic createMCPServer switch statement (~600 lines).
• Deduplicate Authority REST routes — new mountAuthorityRoutes() in authority.ts replaces ~80 lines of copy-pasted Express routes in startMCPServerHTTP.

Net result: -444 lines (874 added, 1,318 removed). All 528 tests pass.

Test plan

• All 528 existing tests pass (npx vitest run)
• New cli-utils.test.ts covers extracted utility functions
• No lint errors introduced

Made with Cursor

[lucamorettibuilds]

Thorough refactor. The -444 lines are real — this isn't just shuffling code, it's genuine deduplication. A few things I noticed:

🔴 Behavioral change in parseTTL

The original in mcp-server.ts threw on invalid TTL:

if (!match) throw new Error(`Invalid TTL format: ${ttl}`);

The new version in tool-handlers.ts silently falls back:

if (!match) return 3600;

This swallows config errors. If someone typos ttl: "1x" in their config, the old code would surface it immediately; the new code silently defaults to 1 hour. I'd keep the throw — it's a config error that should be loud.

🟡 resolveEnvVar error path changed

The old add.ts version called console.error + process.exit(1) directly, producing:

❌ Environment variable FOO is not set (needed for bar)

The new centralized version throws an Error, which bubbles to handleCommandError, which produces:

❌ Error: Environment variable FOO is not set (needed for bar)

Minor format difference (extra "Error:"), but if anyone's parsing CLI output programmatically, it's a break. Probably fine, but worth noting.

✅ Authority route dedup is clean

The mountAuthorityRoutes extraction is well done. The old code used a global middleware that skipped /v1/health; the new code applies authMiddleware per-route. Functionally identical, cleaner to read.

✅ Re-exports preserve the public API

mcp-server.ts re-exports DenialError, APIRequest, etc. from types.ts — anything importing from mcp-server still works. Good.

✅ ToolHandlerContext interface is the right abstraction

The context bag gives tool handlers everything they need without coupling them to the closure. Makes the handlers independently testable, which was the main win here.

Overall: solid refactor. The parseTTL regression is the only thing I'd flag as must-fix before merge.

[lucamorettibuilds]

This silently defaults to 1h on invalid TTL. The original in mcp-server.ts threw new Error('Invalid TTL format: ${ttl}') — I'd keep that behavior so config typos surface early.

[rsdouglas]

Thanks for the thorough review. Addressing each point:

🔴 parseTTL regression — fixed in a2e8803

Good catch, this was a genuine bug I introduced. Restored the throw new Error(...) behavior so invalid TTL formats (like "1x") are surfaced immediately instead of silently defaulting to 1 hour.

🟡 resolveEnvVar error path format change

Acknowledged. The old path was console.error("❌ ...") + process.exit(1) inline, the new path throws which lands in handleCommandError producing "❌ Error: ...". The extra "Error:" prefix is a minor cosmetic difference. I think the consistency win (all commands use the same error pipeline) outweighs the format change, and anyone parsing CLI stderr is already in fragile territory. Happy to strip the "Error:" prefix from handleCommandError if you feel strongly about it.

✅ Authority route dedup / re-exports / ToolHandlerContext

Glad these landed well. The re-exports from mcp-server.ts were specifically to avoid breaking downstream imports.