feat(machines): normalize-and-accept branch/project names instead of rejecting

[2witstudios]

Sub-task 1 of the Terminal UX redesign (Terminal→Machine GA epic), node c6lmwa9at0372ac0icaq9ncc. Independent of the tree/pane rewrite and the Development surface — touches no UI.

The problem

Branch and project name handling validated and rejected. isValidBranchName (branch-session.ts) and isValidProjectName (project-paths.ts) are boolean gates, so a user who typed My Cool Feature got an error and had to guess the charset a git ref allows.

The change: normalize and accept

Type anything; it becomes a valid name.

you type	branch	project
My Cool Feature	my-cool-feature	my-cool-feature
feat/JIRA-123 Fix!!	feat/jira-123-fix	feat-jira-123-fix
../escape	escape	escape
a.lock/b	a-lock/b	a.lock-b
HEAD	head	HEAD
Release-2.0	Release-2.0 (untouched)	Release-2.0
日本語	xnbamha (distinct, never shared)	xnbamha
	rejected (400)	rejected (400)

Two pure functions — normalizeBranchName, normalizeProjectName — colocated with the predicates they must satisfy, over a shared slugifySegment primitive (name-slug.ts). Three rules do the real work, and each exists because of a bug found in review:

1. A name the predicate already accepts is returned untouched. Normalization exists to accept text that would be rejected, never to rewrite text that already worked.

2. When the ASCII charset destroys identity-bearing content, a digest keeps the name distinct. Only structural noise (.., ., ) is dropped — which is what still collapses ../escape to escape.

3. The digest hashes the name's identity (folded, lowercased, ASCII-structure collapsed), not its raw text — so 日本語 stays apart from 한국어 without also splitting MY 🚀 FEATURE from my 🚀 feature.

Six real bugs caught in review — every one a silent wrong answer

1. .lock on inner segments (Codex). Git forbids a .lock ending on every slash-separated component, not just the ref's tail, so feature/a.lock/foo normalized "cleanly", passed the predicate, then died at git checkout -b. Now rewritten per segment; FORBIDDEN_SEGMENT_RE tightened to \.lock(\/|$) to state git's real rule.

2. Case/underscore regression. Git refs are case-sensitive. Slugifying Release-2.0 → release-2.0 made git checkout -b release-2.0 origin/release-2.0 miss the real upstream branch, and the existing fallback then silently created a new empty branch off HEAD while origin/Release-2.0 sat untouched. Uppercase was valid pre-PR, so this PR would have introduced it. → Rule 1.

3. Non-ASCII names collapsed onto one branch — cross-attach. A branch name is a branch-terminal's identity (hashed into the Sprite session key, used as the store's lookup key). Every non-Latin name slugified to nothing and fell back to a shared branch, so spawnBranch('日本語') then spawnBranch('한국어') returned resumed: true and handed the second user the first user's Sprite and filesystem. The default outcome for every non-Latin-script user. → Rule 2.

4. Partial annihilation. 日本語 feature → feature — a different branch that may already exist and belong to someone else. Digesting only total annihilation wasn't enough. → Rule 2, generalized.

5. The digest split names it was meant to protect. It hashed raw text while the slug beside it was folded, so MY 🚀 FEATURE and my 🚀 feature minted two branches, two Sprites, two clones — for one name. → Rule 3.

6. Nameless names addressed real resources. Every nameless string normalizes to a fallback (branch / project) — and a fallback is a real name someone can own. So DELETE /api/machines/projects?name=.. rm -rf'd the project called project, and POST /api/machines/branches {branchName: "."} attached the caller to the branch-terminal called branch, handing back its Sprite. This family produced four separate bugs (blank → whitespace → ./../// → the DELETE path), because each fix used a narrower predicate than the normalizer itself. The guards now key on the normalizer's own hasNameContent, verified as a two-way property: every input that reaches a fallback is rejected, and no real name (!!!, ___, 日本語, 🚀) is over-blocked.

Also: HEAD is git's reserved symbolic ref and the only such name in our charset (head, Head, FETCH_HEAD are all legal — verified against git); the length cut was itself lossy (two over-long names cut to one ref, so the cut now reserves room for a digest); and untrimmed input ("Release-2.0 ") skipped the pass-through and downcased.

A deliberately accepted limitation, stated rather than silent

Our charset is narrower than git's. Git accepts _wip, fix#123, v1.0+build, 日本語; we don't, because the charset is a confinement boundary (the name lands in git checkout -b argv, a store key, and a scope key). So those names are still rewritten — and if one names an existing upstream branch, git's fallback creates a new empty branch instead.

spawnBranch therefore returns createdNew (surfaced on the API response and threaded through the hook): did we check out an existing upstream branch, or create a brand-new one? Nothing renders it yet — that belongs with the spawn-flow sub-task — but the outcome is now knowable rather than invisible. Widening the charset to git's full rule is a separate, security-reviewable change.

The invariant

The isValid* predicates remain the contract:

• isValid*(normalize*(x)) === true for every input
• normalization is idempotent
• every normalized project name resolves inside PROJECTS_ROOT
• distinct names stay distinct (no cross-attach); names differing only in case / ASCII punctuation / whitespace collapse to one

Server-side wiring (authoritative)

Normalization lives in the services, not the routes, so it cannot be bypassed. Every name-keyed path normalizes — spawnBranch, attachBranch, killBranch, listBranches (both projectName and branchName), addProject, removeProject — so whatever text created a thing can also find, attach to, and delete it. Canonical names re-fetched from the list APIs pass through unchanged, because normalization is idempotent.

Both POST routes echo the service's canonical name rather than the request's. The existing hooks already re-fetch and render the server's record, so no client change is needed.

planSpawnBranch and the invalid_branch_name denial are deleted (feature is behind CODE_EXECUTION_ENABLED, no compat shim). invalid_name survives in planAddProject only as the unreachable second confinement gate.

Validation

• Fuzzed 30k+ inputs (random unicode, ligatures, fullwidth, ㍿, combining marks, lone surrogates, CJK, emoji, both truncation boundaries): invariant, idempotency, and confinement all hold.
• Ground truth, not just our own predicate: every distinct normalized branch name accepted by real git check-ref-format --branch.
• No-collision and same-name-collapse both verified as properties, not just examples.
• 438 lib + 154 web machine tests. @pagespace/lib and apps/web typecheck clean; eslint clean.
• The tests are mutation-verified: each regression test was confirmed to FAIL when its own fix is reverted, so none of them is passing by accident.
• The final commit is a behaviour-preserving refactor (the digest-disambiguation rule and the truncate-with-digest block were each written twice; they now live once in name-slug.ts as disambiguateSlug / truncateWithDigest). Proved rather than asserted: byte-identical output across 514,760 differential inputs vs the pre-refactor implementation.

Known issues NOT fixed here (deliberately)

A pre-existing concurrency race in addProject. Two concurrent adds of the same project both clone into one path; the loser's clone fails because the winner's succeeded, and the cleanup rm -rfs the winner's checkout. Normalization widens the trigger from "two callers typed the same text" to "two callers typed the same name" (My Repo / my repo), so it's called out in-code.

I attempted a fix three times and each attempt introduced a worse bug (reserving the name before the clone makes the row visible mid-clone, so a delete-then-re-add leaves an orphan directory and returns ok: true). addProject's control flow is therefore restored exactly to master's — only the normalized name differs. The real fix is structural and earns its own PR: make the clone path unique per row (${name}-${id}) so no two operations can own one directory, and make every destructive store op id-scoped. removeProject has the identical by-name TOCTOU on master today and needs the same treatment.

The sibling agent-terminal / files / diff surfaces don't normalize their name params, so a direct API caller can POST /api/machines/branches {branchName: "My Cool Feature"} successfully but get branch_not_found from POST /api/machines/agent-terminals with the same string. Harmless for the UI (it always passes canonical names back from the list APIs). Closing it needs the realtime session-key path too, so it's a follow-up rather than a drive-by.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KPybRBwDvgaL7bKCfVYeUD

Summary by CodeRabbit

• New Features

  • Branch and project names now accept free text and are automatically normalized into safe, canonical names.
  • Responses display the normalized name and indicate whether a branch was newly created or resumed.
  • Existing branches and projects can be found, attached, or removed using their original free-text names.

• Bug Fixes

  • Blank or whitespace-only names are rejected with a clear validation error.
  • Unsafe names and naming collisions are handled consistently and securely.

[coderabbitai]

📝 Walkthrough

Walkthrough

Machine branch and project names now accept free text, normalize server-side into canonical slugs, use those values for lookup and persistence, and return them through API responses. Branch provisioning metadata, validation, project confinement, and related tests were updated.

Changes

Machine name normalization

Layer / File(s)	Summary
Project name normalization packages/lib/src/services/machines/name-slug.ts, packages/lib/src/services/machines/project-paths.ts, packages/lib/src/services/machines/machine-projects.ts, packages/lib/src/services/machines/__tests__/*project*	Adds shared slugification and project normalization, applies normalized names before path resolution and duplicate checks, persists canonical names, and validates path confinement and normalization invariants.
Branch name normalization packages/lib/src/services/machines/branch-session.ts, packages/lib/src/services/machines/__tests__/branch-session.test.ts	Adds branch normalization, deterministic digest disambiguation, fallback handling, length trimming, and stricter .lock validation with edge-case coverage.
Branch service integration packages/lib/src/services/machines/machine-branches.ts, packages/lib/src/services/machines/__tests__/machine-branches.test.ts	Normalizes names during spawning, attachment, listing, and removal; returns canonical branchName values; reports creation state; and reconciles normalized branch collisions.
API canonical responses apps/web/src/app/api/machines/branches/*, apps/web/src/app/api/machines/projects/*, apps/web/src/hooks/useMachineBranches.ts	Rejects blank names, forwards raw names to services, returns normalized names and creation metadata, and updates endpoint tests and client typing.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MachinesAPI
  participant MachineService
  participant NameNormalizer
  participant Store
  Client->>MachinesAPI: Submit free-text machine name
  MachinesAPI->>MachineService: Forward raw name
  MachineService->>NameNormalizer: Normalize branch or project name
  MachineService->>Store: Lookup or persist canonical name
  Store-->>MachineService: Canonical record
  MachineService-->>MachinesAPI: Normalized result and metadata
  MachinesAPI-->>Client: Canonical name in response

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: machines now normalize and accept branch/project names instead of rejecting them.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/machine-name-normalization

---

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db509d8b80

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Rewrite .lock on every branch path segment

When free text produces an internal .lock segment, such as feature/a.lock /foo normalizing to feature/a.lock/foo, this only rewrites a .lock suffix on the whole ref, so the normalized name is later passed to git checkout -b even though Git rejects it. Git's check-ref-format docs state that no slash-separated component may end with .lock (https://git-scm.com/docs/git-check-ref-format), and git check-ref-format --branch feature/a.lock/foo exits fatal, so these inputs still fail with checkout_failed instead of being normalized and accepted.

Useful? React with 👍 / 👎.

[2witstudios]

Confirmed and fixed in b05e949 — thank you, this was a real hole.

You're right that git check-ref-format forbids a .lock ending on every slash-separated component, not just the ref's tail. My rewrite was anchored on the whole name (\.lock$), so feature/a.lock/foo normalized "cleanly", passed the predicate, and then died at git checkout -b with checkout_failed — precisely the failure mode this PR exists to eliminate.

Two changes:

1. rewriteLockSuffix now runs per segment in the slugify map, and once more after the length cut (the cut only ever lands in the last surviving segment, and can itself mint a .lock out of hotfix.lockdown truncated at 200).
2. FORBIDDEN_SEGMENT_RE tightened from \.lock$ to \.lock(\/|$), so the predicate now states git's actual rule. This matters beyond the normalizer: the predicate is the contract the invariant is proven against, so a too-permissive predicate made the invariant weaker than it looked.

New pinned cases: a.lock/b → a-lock/b, feature/a.lock /foo → feature/a-lock/foo, plus both added to the gnarly corpus that sweeps validity + idempotency.

Verified against ground truth rather than just our own predicate: I ran every normalized name from the corpus (41 inputs, including x.lock/y.lock/z.lock, a@{b, ../../etc/passwd, 250-char strings) through real git check-ref-format --branch — 41/41 accepted.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

apps/web/src/app/api/machines/projects/route.ts (1)

127-127: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

removeProject does not normalize the name before lookup — DELETE with free text will 404.

The PR objectives state "Server-side services normalize names before … lookup." The graph context for removeProject (machine-projects.ts:226-250) shows it calls deps.store.findByName(machineId, name) with the raw, un-normalized name from the DELETE handler. A user who creates a project with "My Cool Feature" (persisted as "my-cool-feature") and then DELETEs using the original free text will get a 404 not_found — inconsistent with addProject and attachBranch, which both normalize before lookup.

Consider normalizing in removeProject (or in the route before calling it) so DELETE accepts the same free text as POST.

Proposed fix: normalize in removeProject

 export async function removeProject({
   machineId,
   name,
   deps,
 }: {
   machineId: string;
   name: string;
   deps: MachineProjectsDeps;
 }): Promise<RemoveProjectResult> {
-  const existing = await deps.store.findByName(machineId, name);
+  const normalized = normalizeProjectName(name);
+  const existing = await deps.store.findByName(machineId, normalized);
   if (!existing) return { ok: false, reason: 'not_found' };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/app/api/machines/projects/route.ts` at line 127, Normalize the
project name before lookup in removeProject, ensuring its deps.store.findByName
call uses the same normalization behavior as addProject and attachBranch.
Preserve the existing deletion and not-found handling while allowing DELETE
requests to resolve free-text names such as “My Cool Feature”.

apps/web/src/app/api/machines/branches/route.ts (1)

139-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Normalize the branch name before killBranch() lookup. The DELETE path passes raw branchName through, but killBranch() looks up the row without normalizing first. A request using the user-entered name instead of the canonical slug will return not_found; match attachBranch() and normalize here too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/app/api/machines/branches/route.ts` at line 139, Normalize the
branch name before the DELETE path calls killBranch(), matching the
normalization used by attachBranch(). Pass the canonical normalized value in the
killBranch lookup instead of raw branchName.value, while preserving the existing
not_found behavior for genuinely missing branches.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/lib/src/services/machines/machine-projects.ts`:
- Around line 37-49: The removeProject flow still looks up projects using the
raw caller-supplied name. Normalize the name with normalizeProjectName before
performing the lookup and deletion, so original free-text names resolve to the
same canonical slug used by addProject, duplicate checks, clone paths, and
persistence.

---

Outside diff comments:
In `@apps/web/src/app/api/machines/branches/route.ts`:
- Line 139: Normalize the branch name before the DELETE path calls killBranch(),
matching the normalization used by attachBranch(). Pass the canonical normalized
value in the killBranch lookup instead of raw branchName.value, while preserving
the existing not_found behavior for genuinely missing branches.

In `@apps/web/src/app/api/machines/projects/route.ts`:
- Line 127: Normalize the project name before lookup in removeProject, ensuring
its deps.store.findByName call uses the same normalization behavior as
addProject and attachBranch. Preserve the existing deletion and not-found
handling while allowing DELETE requests to resolve free-text names such as “My
Cool Feature”.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38edc0ca-05bd-4c23-b58e-ff7f25cf17ac

📥 Commits

Reviewing files that changed from the base of the PR and between 654ba81 and b05e949.

📒 Files selected for processing (13)

• apps/web/src/app/api/machines/branches/__tests__/route.test.ts
• apps/web/src/app/api/machines/branches/route.ts
• apps/web/src/app/api/machines/projects/__tests__/route.test.ts
• apps/web/src/app/api/machines/projects/route.ts
• packages/lib/src/services/machines/__tests__/branch-session.test.ts
• packages/lib/src/services/machines/__tests__/machine-branches.test.ts
• packages/lib/src/services/machines/__tests__/machine-projects.test.ts
• packages/lib/src/services/machines/__tests__/project-paths.test.ts
• packages/lib/src/services/machines/branch-session.ts
• packages/lib/src/services/machines/machine-branches.ts
• packages/lib/src/services/machines/machine-projects.ts
• packages/lib/src/services/machines/name-slug.ts
• packages/lib/src/services/machines/project-paths.ts

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/lib/src/services/machines/name-slug.ts (1)

126-158: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider widening the digest to reduce collision risk.

FNV-1a/32-bit gives a real (if small) chance that two distinct non-sluggable names collide on the same slugDigest. Per the file's own docstring, that specific failure mode is severe — it would make spawnBranch hand one user another user's Sprite/filesystem. Widening the hash (e.g., two FNV-1a passes with different seeds, concatenated) would meaningfully shrink that residual risk while staying sync/pure/dependency-free for the browser live-preview use case.

♻️ Example approach to widen the digest

+const FNV_OFFSET_BASIS_2 = 0x9dc5;
+
 export function slugDigest(input: string): string {
-  let hash = FNV_OFFSET_BASIS;
   const text = nameIdentity(input);
-  for (let i = 0; i < text.length; i += 1) {
-    hash ^= text.charCodeAt(i);
-    hash = Math.imul(hash, FNV_PRIME);
-  }
-  return (hash >>> 0).toString(36);
+  let h1 = FNV_OFFSET_BASIS;
+  let h2 = FNV_OFFSET_BASIS ^ FNV_OFFSET_BASIS_2;
+  for (let i = 0; i < text.length; i += 1) {
+    const code = text.charCodeAt(i);
+    h1 = Math.imul(h1 ^ code, FNV_PRIME);
+    h2 = Math.imul(h2 ^ code, FNV_PRIME + 2);
+  }
+  return `${(h1 >>> 0).toString(36)}${(h2 >>> 0).toString(36)}`;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/lib/src/services/machines/name-slug.ts` around lines 126 - 158,
Widen the digest produced by slugDigest beyond the current single 32-bit FNV-1a
pass to materially reduce collisions between distinct non-sluggable names. Keep
it synchronous, pure, dependency-free, and based on nameIdentity; use an
additional independently seeded FNV-1a pass and combine the unsigned base36
results into the returned token.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/web/src/app/api/machines/projects/route.ts`:
- Around line 84-90: Update the validation condition in the projects route to
require body.repoUrl to be a string with non-empty trimmed content, matching the
existing body.name validation and the error message. Preserve the current 400
response for invalid name or repoUrl values.

---

Nitpick comments:
In `@packages/lib/src/services/machines/name-slug.ts`:
- Around line 126-158: Widen the digest produced by slugDigest beyond the
current single 32-bit FNV-1a pass to materially reduce collisions between
distinct non-sluggable names. Keep it synchronous, pure, dependency-free, and
based on nameIdentity; use an additional independently seeded FNV-1a pass and
combine the unsigned base36 results into the returned token.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f17b6695-01f9-4875-9522-43cc44a827d0

📥 Commits

Reviewing files that changed from the base of the PR and between b05e949 and c3ee261.

📒 Files selected for processing (14)

• apps/web/src/app/api/machines/branches/__tests__/route.test.ts
• apps/web/src/app/api/machines/branches/route.ts
• apps/web/src/app/api/machines/projects/__tests__/route.test.ts
• apps/web/src/app/api/machines/projects/route.ts
• apps/web/src/hooks/useMachineBranches.ts
• packages/lib/src/services/machines/__tests__/branch-session.test.ts
• packages/lib/src/services/machines/__tests__/machine-branches.test.ts
• packages/lib/src/services/machines/__tests__/machine-projects.test.ts
• packages/lib/src/services/machines/__tests__/project-paths.test.ts
• packages/lib/src/services/machines/branch-session.ts
• packages/lib/src/services/machines/machine-branches.ts
• packages/lib/src/services/machines/machine-projects.ts
• packages/lib/src/services/machines/name-slug.ts
• packages/lib/src/services/machines/project-paths.ts

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

• apps/web/src/app/api/machines/projects/tests/route.test.ts
• packages/lib/src/services/machines/tests/project-paths.test.ts
• packages/lib/src/services/machines/tests/machine-projects.test.ts
• apps/web/src/app/api/machines/branches/tests/route.test.ts
• packages/lib/src/services/machines/project-paths.ts
• packages/lib/src/services/machines/tests/machine-branches.test.ts
• packages/lib/src/services/machines/tests/branch-session.test.ts
• apps/web/src/app/api/machines/branches/route.ts
• packages/lib/src/services/machines/machine-branches.ts