fix(auth): persist internal token across restarts

[dimakis]

Summary

• Read existing token from ~/.mitzo/internal-token on startup instead of generating a new one every time
• Only generates a fresh token when the file is missing or corrupt (not hex, wrong length)
• Fixes 401 errors for agents, hooks, and the Python mitzo_client after every Mitzo restart or crash recovery

Root cause

INTERNAL_TOKEN = randomBytes(32).toString('hex') ran on every import, overwriting the persisted token. Any external client (PR Shepherd, session hooks, CLI) holding the old token got 401'd after a restart. Tonight this broke the Task Board API entirely after a crash + launchd recovery + dev server token collision.

Test plan

• All 19 auth tests pass
• Restart Mitzo, verify token file unchanged
• Verify mitzo_client API calls work after restart
• Delete token file, restart, verify new one generated

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

server/internal-token.ts

Solid, low-risk fix that correctly persists the internal token across restarts. Main gap is missing unit tests for the new loadOrCreateToken function.

• 🟡 missing_tests (L11): The new loadOrCreateToken() function has no unit tests. The existing auth.test.ts tests cover isValidInternalToken and the auth middleware, but there are no tests for the load-or-create logic: reading an existing valid token from disk, rejecting a corrupted/truncated file and regenerating, creating a new token when the file is missing, or verifying file permissions (0o600). Since the CLAUDE.md mandates TDD, this function should have been test-first. [fixable]
• 🔵 unsafe_assumptions (L14): The regex /^[0-9a-f]+$/ only matches lowercase hex. randomBytes().toString('hex') always produces lowercase, but if a user or external tool manually wrote an uppercase hex token to the file (e.g. A1B2...), it would be silently rejected and overwritten. Consider using /^[0-9a-fA-F]+$/i or documenting that only lowercase is accepted. [fixable]

[dimakis]

🟡 missing_tests: The new loadOrCreateToken() function has no unit tests. The existing auth.test.ts tests cover isValidInternalToken and the auth middleware, but there are no tests for the load-or-create logic: reading an existing valid token from disk, rejecting a corrupted/truncated file and regenerating, creating a new token when the file is missing, or verifying file permissions (0o600). Since the CLAUDE.md mandates TDD, this function should have been test-first. [fixable]

[dimakis]

🔵 unsafe_assumptions: The regex /^[0-9a-f]+$/ only matches lowercase hex. randomBytes().toString('hex') always produces lowercase, but if a user or external tool manually wrote an uppercase hex token to the file (e.g. A1B2...), it would be silently rejected and overwritten. Consider using /^[0-9a-fA-F]+$/i or documenting that only lowercase is accepted. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

server/__tests__/internal-token.test.ts

Core logic is correct and the persistence fix is sound. Main concern is that the test duplicates the function rather than testing the real one, introducing subtle divergences (missing try/catch around writes, platform-specific path splitting).

• 🟡 bugs (L22): The test-local copy of loadOrCreateToken diverges from the production version in server/internal-token.ts. The production code wraps mkdirSync/writeFileSync in a try/catch (non-fatal), while the test copy does not. This means the test is validating a slightly different function than what ships. If a future fix touches the error handling path, the test won't catch regressions. Consider extracting the function with a configurable path parameter in the production module (or using vi.stubEnv/dependency injection) rather than duplicating the logic. [fixable]
• 🔵 bugs (L22): tokenPath.substring(0, tokenPath.lastIndexOf('/')) uses a hardcoded / separator. The production code uses join(homedir(), '.mitzo') which is platform-aware via path.join. While macOS is the only target today, this is a subtle divergence from the production implementation. Prefer path.dirname(tokenPath) for consistency. [fixable]
• 🔵 missing_tests: No test covers the non-fatal write failure path (production lines 23-27). The production code silently swallows write errors and still returns the generated token. The test copy doesn't have this try/catch, so this behavior is unverified. A test with a read-only directory would cover this. [fixable]

server/internal-token.ts

Core logic is correct and the persistence fix is sound. Main concern is that the test duplicates the function rather than testing the real one, introducing subtle divergences (missing try/catch around writes, platform-specific path splitting).

• 🔵 unsafe_assumptions (L14): The regex /^[0-9a-fA-F]+$/ accepts uppercase hex on read, but randomBytes(32).toString('hex') always generates lowercase. This is fine for accepting externally-edited files, but isValidInternalToken at line 37 uses timingSafeEqual on the raw string — a token written as uppercase would work on load but an external client sending the same uppercase value would match. No bug, but worth noting the case-sensitivity contract is intentionally loose on read.

[dimakis]

🟡 bugs: The test-local copy of loadOrCreateToken diverges from the production version in server/internal-token.ts. The production code wraps mkdirSync/writeFileSync in a try/catch (non-fatal), while the test copy does not. This means the test is validating a slightly different function than what ships. If a future fix touches the error handling path, the test won't catch regressions. Consider extracting the function with a configurable path parameter in the production module (or using vi.stubEnv/dependency injection) rather than duplicating the logic. [fixable]

[dimakis]

🔵 bugs: tokenPath.substring(0, tokenPath.lastIndexOf('/')) uses a hardcoded / separator. The production code uses join(homedir(), '.mitzo') which is platform-aware via path.join. While macOS is the only target today, this is a subtle divergence from the production implementation. Prefer path.dirname(tokenPath) for consistency. [fixable]

[dimakis]

🔵 unsafe_assumptions: The regex /^[0-9a-fA-F]+$/ accepts uppercase hex on read, but randomBytes(32).toString('hex') always generates lowercase. This is fine for accepting externally-edited files, but isValidInternalToken at line 37 uses timingSafeEqual on the raw string — a token written as uppercase would work on load but an external client sending the same uppercase value would match. No bug, but worth noting the case-sensitivity contract is intentionally loose on read.

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

server/internal-token.ts

Clean, well-tested change that correctly persists the internal token across restarts. No bugs — just minor hardening suggestions around permission testing, corruption logging, and tightening the hex regex to match actual output.

• 🟡 bugs (L15): loadOrCreateToken accepts uppercase hex tokens (regex allows a-fA-F), but isValidInternalToken uses timingSafeEqual which is case-sensitive. If someone manually writes an uppercase token to ~/.mitzo/internal-token, the loaded INTERNAL_TOKEN will be uppercase, but callers (chat.ts line 576) pass this value as MITZO_INTERNAL_TOKEN env var — the task-mcp-server then sends it back in headers — so it matches itself. This is internally consistent because the same INTERNAL_TOKEN is used on both sides. Not a bug in practice, but the uppercase acceptance is dead code since randomBytes().toString('hex') always produces lowercase — consider tightening the regex to /^[0-9a-f]+$/ to avoid a latent mismatch if a future caller normalizes case. [fixable]
• 🔵 unsafe_assumptions (L15): Corrupt file validation catches wrong length and non-hex, but doesn't log when it regenerates a token. A corrupted token file silently invalidates all existing hook/agent credentials with no audit trail. Consider logging at warn level when regenerating due to corruption (the logger module is already available in the project). [fixable]

server/__tests__/internal-token.test.ts

Clean, well-tested change that correctly persists the internal token across restarts. No bugs — just minor hardening suggestions around permission testing, corruption logging, and tightening the hex regex to match actual output.

• 🔵 missing_tests: No test verifies that the file is written with restrictive permissions (mode 0o600) or that the parent directory is created with mode 0o700. Since this is a security-sensitive token persisted to disk, a test asserting fs.statSync(tokenPath).mode & 0o777 === 0o600 would catch permission regressions. [fixable]
• 🔵 style (L12): Using Date.now() for temp directory uniqueness can collide if tests run in parallel within the same millisecond. crypto.randomUUID() or the node:fs mkdtempSync would be more robust, though this is unlikely to cause issues in practice. [fixable]

[dimakis]

🟡 bugs: loadOrCreateToken accepts uppercase hex tokens (regex allows a-fA-F), but isValidInternalToken uses timingSafeEqual which is case-sensitive. If someone manually writes an uppercase token to ~/.mitzo/internal-token, the loaded INTERNAL_TOKEN will be uppercase, but callers (chat.ts line 576) pass this value as MITZO_INTERNAL_TOKEN env var — the task-mcp-server then sends it back in headers — so it matches itself. This is internally consistent because the same INTERNAL_TOKEN is used on both sides. Not a bug in practice, but the uppercase acceptance is dead code since randomBytes().toString('hex') always produces lowercase — consider tightening the regex to /^[0-9a-f]+$/ to avoid a latent mismatch if a future caller normalizes case. [fixable]

[dimakis]

🔵 unsafe_assumptions: Corrupt file validation catches wrong length and non-hex, but doesn't log when it regenerates a token. A corrupted token file silently invalidates all existing hook/agent credentials with no audit trail. Consider logging at warn level when regenerating due to corruption (the logger module is already available in the project). [fixable]

[dimakis]

🔵 style: Using Date.now() for temp directory uniqueness can collide if tests run in parallel within the same millisecond. crypto.randomUUID() or the node:fs mkdtempSync would be more robust, though this is unlikely to cause issues in practice. [fixable]