fix(cli): branch and memory refuse without a configured identity (#499)

[tps-anvil]

Closes #499

tps branch start and memory governance actions (review, approve, reject, archive, unarchive, purge, list, show, search) use requireLocalAgentId and refuse by name without an identity. The branch hostname-fragment fallback and memory admin fallback are removed. Re-adding either removed expression (hostname().split(".")[0] in branch or ?? "admin" in memory) turns the guard red.

Counts measured on e69bb1d (main 77ee454):

• New tests: packages/cli/test/no-identity-defaults-499.test.ts (4 tests) and the rewritten packages/cli/test/branch-mail-identity.test.ts fail on origin/main (4 fail / 1 file error) and pass on this head.
• Guard mutation: re-adding hostname().split(".")[0] in src/commands/branch.ts or ?? "admin" in src/commands/memory.ts makes no-identity-in-src.test.ts red (measured one file at a time); restoring makes it pass.
• bun run lint:ci: exit 0.
• bun run build (tsc, all workspace packages): exit 0.
• CLI unit suite: 2239 pass / 4 skip / 0 fail across 161 files on e69bb1d; baseline on main 77ee454 is 2232 pass / 4 skip / 0 fail across 160 files. The 7 added tests are the new file (4) and the guard coverage (3).

Merged main 883f889 (#496, #497); branch.ts keeps #497's OutboxSendTracker unchanged and adds this PR's requireLocalAgentId import. On b8a99c3: bun run build exit 0, bun run lint:ci exit 0; branch-mail-identity 5/0, no-identity-defaults-499 4/0, no-identity-in-src 7/0, memory-cli 11/0, relay-delivery-loss 16/0, relay 16/0 (pass/fail).

On 9cd4676:

• bun run build, bun run lint:ci, and CLI bun x tsc --noEmit: exit 0.
• Focused branch/memory/identity tests: 79 pass / 0 fail across 8 files.
• CLI regression tests (memory-entry and branch-daemon) applied to b8a99c3: 2 pass / 13 fail; all pass in the focused run on 9cd4676.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 37 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: f4bc995c-6d3a-415a-b6c3-72c434215ee3

📥 Commits

Reviewing files that changed from the base of the PR and between 883f889 and 9cd4676.

📒 Files selected for processing (11)

• .changelog/unreleased/fixed-499-identity-fallbacks.md
• packages/cli/bin/tps.ts
• packages/cli/src/commands/branch.ts
• packages/cli/src/commands/memory.ts
• packages/cli/test/branch-daemon.test.ts
• packages/cli/test/branch-mail-identity.test.ts
• packages/cli/test/memory-cli.test.ts
• packages/cli/test/memory-entry.test.ts
• packages/cli/test/no-identity-defaults-499.test.ts
• packages/cli/test/no-identity-in-src.test.ts
• pr-body.new.md

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[tps-anvil]

Pre-delivery sweep (cli#499) — measured on e69bb1d (main 77ee454)

Every sentence this PR adds or changes, checked against the code: outcomes on every branch, scope words, and coverage claims.

• branch.ts branchAgentId doc — "refusing by name when none is set": requireLocalAgentId throws when TPS_AGENT_ID and confAgentId are both absent. OK. "TPS_AGENT_ID wins": process.env.TPS_AGENT_ID ?? confAgentId resolves env first. OK. "the id persisted by tps branch init --agent": writeBranchConf(..., args.agent) persists conf.agentId; readBranchConf returns it. OK. "There is no fallback to the hostname": grep -c 'hostname().split' src/commands/branch.ts = 0. OK.
• branch.ts getLocalAgentId comment — describes env → conf → refuse; matches branchAgentId(conf.agentId); the storage/alias rationale is unchanged behaviour. OK.
• memory.ts comment — "the CLI operator's configured agent (TPS_AGENT_ID, or the explicit agentId); with neither, refuse by name": code is requireLocalAgentId("memory operator id", process.env.TPS_AGENT_ID ?? args.agentId), which throws only when both are absent. OK.
• branch-mail-identity.test.ts header — "stores incoming mail under its own local identity ... and refuses by name when neither is configured": asserted by the rewritten tests against the real branchAgentId. OK.
• no-identity-in-src.test.ts new comment — "refuse by name when none is set; neither may fall back to an identity ... re-adding one turns this guard red": mutation run one file at a time (re-add hostname().split(".")[0] in branch.ts → red; re-add ?? "admin" in memory.ts → red; restore → green). OK.
• no-identity-defaults-499.test.ts header — "refuse by name when none is set ... fallbacks are gone": 4 tests fail on main, pass on this head. OK.
• Changelog fragment — lede describes main's behaviour (hostname / admin fallback) vs this release; body names the configured sources per command. Both match the code. OK.

Scope words (every, all, only, never, none, neither): the new comments use "neither" for the two named files and "no fallback"; both are true of the code. No coverage claim is made that a test does not assert.

[tps-flint]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tps-flint]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tps-kern]

Review of head 9cd4676. Repo visibility checked before writing: repos/tpsdev-ai/cli .visibility = public; no findings below require private handling. Internal author (tps-anvil): I built and ran the touched suites in a review worktree.

What I verified

• Build (bun install --frozen-lockfile && bun run build) succeeds. Targeted suites, via the isolated launcher: branch-daemon, branch-mail-identity, memory-cli, memory-entry, no-identity-defaults-499, no-identity-in-src — 42/42 pass.
• Same configuration pattern as #485/#498: both new call sites go through requireLocalAgentId from utils/local-agent.ts (the #397 helper — explicit arg or TPS_AGENT_ID, else a named refusal, no principal fallback). branchAgentId() adds the branch init --agent persisted value in the documented precedence (TPS_AGENT_ID wins); routeHandlerAction's default refuses with only the env, and the daemon passes the fully-resolved id explicitly (branch.ts:389), so no path silently re-derives one.
• No remaining identity fallback for these surfaces: grepped packages/cli/src — the hostname-fragment fallback in branch.ts and the "admin" literal in memory.ts are gone, and the guard (no-identity-in-src.test.ts) pins the exact removed expressions with a self-check that each pattern matches the expression it guards, so a re-added fallback turns red.
• Refusal ordering is before side effects: runStart resolves identity and requires branch.conf.json before the daemonization spawn — the new tests assert no spawn, no pid file, no log on refusal, and the identity error is named ("no branch agent id"). For memory, the refusal happens before any Flair client construction, and each CLI action still validates its own missing positional (agentId/memoryId) before any network call.
• CLI dispatch: meow strips flags from cli.input, so rest[1] can never swallow a --limit/--json; the durability raw-value flag registration matches runMemory's expectations.
• Memory test hygiene: env mutation is saved/restored around each case, and the entry tests spawn the built binary under a fresh HOME with TPS_AGENT_ID deleted.

Observations (non-blocking)

1. [packages/cli/src/utils/identity.ts:412-419] (observation, out of #499 scope) loadHostIdentityId() still returns hostname().split(".")[0] unconditionally. That is the host principal's definition (feeds resolveCliSenderId and mail.ts's sender resolution), not an agent-principal fallback, so the no-fallback claim holds for what #499 covers — but note the brief-level phrasing "no remaining identity fallback in packages/cli/src" is only true in that narrower sense. Its downstream || "unknown" (sender-id.ts:32, mail.ts:145) means mail with nothing configured signs as the literal principal "unknown"; pre-existing, worth its own issue rather than this PR.
2. [packages/cli/src/commands/memory.ts:47] (nit, documented) For review/list/search with no TPS_AGENT_ID, the operator resolves to the target agent id. The changelog states this, and it is safe because the client then needs that agent's key at defaultFlairKeyPath(operatorId) — no key, no request — but it does mean tps memory review flint on a machine holding flint's key acts as flint, not as an admin. Server-side authorization is the real boundary here; fine to leave.

Verdict: APPROVE.

[tps-sherlock]

Reviewed head 9cd46766 (author tps-anvil, a tps-* agent → internal). Visibility checked: repos/tpsdev-ai/cli .visibility = public (PUBLIC). Built and ran in my own worktree ~/work/review-501-sherlock: bun install --frozen-lockfile && bun run build → exit 0; focused suites (no-identity-in-src, no-identity-defaults-499, branch-mail-identity, branch-daemon, memory-cli, memory-entry) through scripts/test-suite.mjs with HOME on an empty dir → 42 pass / 0 fail (127 expect). I also ran three guard mutations.

Sherlock focus — an unset identity refuses (never falls back to a principal); the guard catches a re-added fallback.

1. Refusal holds at both sites. requireLocalAgentId (packages/cli/src/utils/local-agent.ts:9-14) returns the explicit arg or TPS_AGENT_ID and otherwise throws no <what>: pass an explicit agent id or set TPS_AGENT_ID. branchAgentId (packages/cli/src/commands/branch.ts:41-42) passes process.env.TPS_AGENT_ID ?? confAgentId — no hostname; runMemory (packages/cli/src/commands/memory.ts:47) passes process.env.TPS_AGENT_ID ?? args.agentId. Behavior tests confirm: "mail routing refuses when no identity is configured", "refuses by name when no operator identity is configured", and branch-daemon "refuses without identity or branch configuration before spawning" (spawned/pidWritten/logWritten all false — the identity check precedes the daemon spawn at branch.ts:76-77). An empty TPS_AGENT_ID also refuses because !id catches "" (tested for "" in branch-daemon).

2. The guard catches re-adding the two removed expressions — verified by mutation. Re-adding hostname().split(".")[0] in branch.ts turns "each covered file resolves its identity without a fallback" red; re-adding ?? "admin" in memory.ts turns that test and the behavioral refusal test red.

3. Non-blocking — the guard is expression-specific. packages/cli/test/no-identity-in-src.test.ts:32-35 pins two regexes (/hostname\(\)\.split\(/ in branch.ts, /"admin"/ in memory.ts). A fallback written differently is invisible to the static guard: I added ?? "operator" to memory.ts:47 and the guard stayed green (only the behavioral test "refuses by name when no operator identity is configured" went red). So the issue's "add both files … so a fallback cannot return" holds only for those two spellings; the behavioral tests, not the guard, are the real backstop. A comment stating that reliance would help the next reader.

4. Non-blocking, outside #499's two-file scope — other hostname-derived identity fallbacks remain in packages/cli/src. packages/cli/src/utils/identity.ts:413 const safeHostname = () => hostname().split(".")[0]!; is returned by loadHostIdentityId() on every path (:415-422), and packages/cli/src/commands/mail.ts:133-152 (resolveAgentId) reaches it as the sender fallback (const id = await loadHostIdentityId() || "unknown"). Neither is covered by the guard (the literal scan flags only known agent ids; the new pattern list covers only branch.ts and memory.ts). Pre-existing and not what #499 asked to change — flagged only because this lane asks whether any identity fallback remains in src.

Minor notes (non-blocking). requireLocalAgentId tests !id only, so a whitespace-only TPS_AGENT_ID/argument passes as the principal (empty is refused; whitespace is untested). packages/cli/src/commands/branch.ts:209-212 still carries the comment describing the removed order "(TPS_AGENT_ID env → conf.agentId → hostname)" while the code persists args.agent (writeBranchConf(port, advertiseHost, transport, undefined, args.agent)), so that comment now references a fallback this PR deleted.

No blocking findings. Verdict: APPROVE.