Fixes from ralph-tmux-codex-test exploratory campaign

[agent-era-ai]

Summary

Three fixes that close 4 of the 9 confirmed bugs from ralph-tmux-codex-test's exploratory tmux+Codex run:

• fix(git) — GitService.createWorktree now works on a brand-new local-only repo with no origin remote. Skips git fetch origin when no origin exists; passes the resolved base branch through unchanged instead of synthesising a non-existent origin/<base> ref. (Closes finding Rename CleanDiffView to DiffView #1.)
• fix(sessions) — Threads an opts.freshWorktree flag from createFeature / recreateImplementWorktree through attachSession / launchSessionBackground into the launch helpers. When set, codex/claude/gemini launch fresh (no resume --last / --continue) so first-launch into a just-created worktree never tries to resume a session that doesn't exist. (Closes findings Fix slow navigation after archiving/creating branches #2 + Add auto-attach functionality for new feature branches #3.)
• fix(tracker) — Adds TrackerService.renameItem and wires it into App.ensureItemWorktree so when a worktree-create lands on a suffixed name (foo-2 because branch foo already exists), the tracker slug, tracker/index.json buckets, sessions key, on-disk tracker/items/ directory, and slug: frontmatter all adopt the suffix in lockstep — no silent drift between tracker, worktree, and tmux session names. (Closes finding Restore execute session feature (x/X shortcuts) #8.)

The original 5th fix (codex pane-state detector for trust / sandbox-retry / usage-limit / model-switch modals) was dropped during rebase: PR #228 (33c4a71 terminal ui state detection) landed in main while this branch was open and reorganised getStatusForTool around picker-based detection. My substring regex against the modal text reproduced as a false-positive on the existing fixtures (those modals stay in scrollback after dismissal), and the picker-based shape from #228 is the right primitive.

Findings #4, #5, #7 are partially open after this PR — main's picker patterns catch active codex consent/permission dialogs, but trust-prompt-only (Press enter to continue) and usage-limit/model-switch modals aren't matched today. Recommended follow-up: capture proper fixtures for those active states and extend the picker patterns. Findings #6 and #9 stay out of scope per requirements (agent-side status.json write race; PTY-harness keystroke flakiness).

Test plan

• npx tsc -p tsconfig.test.json --noEmit — clean
• npx tsc -p . --noEmit — clean
• npm test — 757 tests pass across 76 suites
• npm run build — clean
• Manual smoke: create a fresh local-only repo, attach via tracker, confirm worktree creation succeeds and codex launches without resume noise

🤖 Generated with Claude Code

[claude]

Code Review: PR #231 — Fixes from ralph-tmux-codex-test

Overall: Well-scoped bug-fix bundle with clear intent and good test coverage. A few issues worth addressing before merge.

---

fix(git): GitService.createWorktree (#1)

Good:

• Correctly skips git fetch origin on local-only repos
• Removes the incorrect synthetic origin/ prefix that broke local-only worktree creation
• New hasOriginRemote() helper is small and focused

Concern — fragile string matching in hasOriginRemote:

const out = runCommandQuick(['git', '-C', repoPath, 'remote', 'get-url', 'origin']);
return !!out && !/fatal/i.test(out);

This relies on the English text "fatal" appearing in git's stderr. If runCommandQuick captures stdout but not stderr (or silently swallows the non-zero exit code), this will return true even when there's no remote. Consider checking whether runCommandQuick returns null/empty on command failure vs. printing the error message — if it already does, add a comment explaining why the fatal check is needed. If not, checking the git exit code directly would be more robust.

---

fix(sessions): freshWorktree flag (#2)

Good:

• Clean approach: thread an explicit freshWorktree flag from the two creator paths rather than probing disk state
• Correctly removes the noisy || fallback chain on first launch

Gap — showAIToolSelection doesn't receive freshWorktree:

In CreateFeatureScreen.tsx:

if (needsSelection) {
  showAIToolSelection(created);          // ← freshWorktree not passed
} else {
  await attachSession(created, undefined, undefined, {freshWorktree: true});
}

And in App.tsx:

if (needsSelection) {
  showAIToolSelection(prepared.worktree, {initialPrompt: prepared.prompt, ...}); // ← freshWorktree not passed
} else {
  await attachSession(..., {freshWorktree: prepared.fresh});
}

When a brand-new worktree requires tool selection, the session launched after the user picks a tool will still run claude --continue (or codex resume --last), which is exactly the bug this fix targets. If showAIToolSelection can accept an opts bag, thread freshWorktree through that path too.

Behavior change for existing worktrees:
Removing the || fresh fallback from the non-fresh path means a user attaching to an existing worktree whose tmux session was killed (e.g. after reboot) will get a failing claude --continue with no recovery. This is an intentional tradeoff per the PR description, but it would be worth a brief comment near launchClaudeSessionWithFallback noting that callers that need the old recovery behaviour should pass freshWorktree: true instead of relying on a chain.

---

fix(tracker): TrackerService.renameItem (#8)

Good:

• Comprehensive: updates index buckets, sessions metadata, on-disk directory, and frontmatter in one atomic operation
• writeJSONAtomic for the index write is correct
• Test coverage is solid (4 cases: happy path, dir+frontmatter, collision, unknown slug)

Concern — unescaped slug in regex:

const slugLineRe = new RegExp(`^slug:\\s*${oldSlug}\\s*$`, 'm');

oldSlug is interpolated directly without escaping. A slug like feature-2 is fine, but if isValidSlug permits characters like . or +, this can produce an incorrect match. Either call escapeRegExp(oldSlug) or add a comment asserting that isValidSlug guarantees slug chars are regex-safe ([a-z0-9-] only).

Minor — renameItem returns false when oldSlug === newSlug:
This makes the check in ensureItemWorktree slightly misleading:

if (created.feature !== item.slug) {
  const renamed = tracker.renameItem(...);
  if (renamed) activeItem = {...item, slug: created.feature};
}

The guard created.feature !== item.slug already prevents calling with equal slugs, so this is safe — but consider returning true (success, no-op) for the equal case to match caller expectations.

---

Tracker files in source tree

The PR adds tracker/items/ralph-tmux-codex-fixes/ (implementation.md, notes.md, requirements.md, status.json) to the repository. These are project-management artefacts, not source code. If tracker items for the project's own development work are intentionally committed, that's fine — but confirm this is the expected convention. If not, tracker/items/ should be in .gitignore.

---

Tests

• WorktreeCoreAutoResume.test.ts — new freshWorktree=true cases for claude and codex are good; the updated existing-worktree test now correctly expects only claude --continue (no fallback)
• git-worktree-creation.test.ts — the new local-only-repo test and the inverted "no synthetic prefix" assertion directly cover the fixed behaviour
• tracker.test.ts — four renameItem cases cover the key paths

Missing: There's no test for the case where renameItem fails (returns false) inside ensureItemWorktree — the current code silently falls back to the original slug. A test that asserts the returned item still has the original slug in that case would prevent a future regression.

---

Summary

Item	Severity
showAIToolSelection doesn't pass freshWorktree	Bug — the fix is incomplete for the tool-selection path
Unescaped oldSlug in regex	Low risk (depends on slug charset); should be documented or fixed
hasOriginRemote relies on "fatal" text	Low risk; worth a comment or exit-code check
Tracker files committed	Convention question — confirm intentional
Missing test for renameItem failure path	Minor

🤖 Generated with Claude Code

[claude]

Code Review: PR #231 — Fixes from ralph-tmux-codex-test exploratory campaign

Overview: Three focused bug fixes covering local-only repo worktree creation, fresh-session launch noise, and tracker slug drift. The PR description is thorough, the scope is tight, and the test plan was followed. 757 tests passing is a good baseline signal.

---

Fix 1 — GitService.createWorktree (local-only repos)

src/services/GitService.ts

The logic is correct. The removed origin/ re-prefix was the real root cause, and the conditional fetch is a clean guard.

Minor concern — hasOriginRemote fragility:

private hasOriginRemote(repoPath: string): boolean {
  const out = runCommandQuick(['git', '-C', repoPath, 'remote', 'get-url', 'origin']);
  return !!out && !/fatal/i.test(out);
}

Parsing fatal from stderr-mixed output works in practice but is brittle. A slightly more robust alternative would be checking git remote output for the string origin, or relying on exit code if runCommandQuick surfaces it. Not a blocker, but worth noting for future maintenance.

Test coverage: The two updated/new tests in git-worktree-creation.test.ts cover the happy path, the local-fallback case, and the no-fetch assertion for local-only repos. Good.

---

Fix 2 — freshWorktree flag threading

src/cores/WorktreeCore.ts, src/contexts/WorktreeContext.tsx, src/contexts/UIContext.tsx, src/App.tsx, src/screens/CreateFeatureScreen.tsx

The threading is clean and the flag defaults correctly to false/undefined everywhere it isn't set. Call sites that don't need the flag are unchanged in behavior.

Behavior change worth understanding — removal of the || fallback chain:

The old non-fresh-with-prompt path was:

codex resume --last <prompt> || codex <prompt>

The new non-fresh path (with or without prompt) is just:

codex resume --last [<prompt>]

No fallback. The PR author argues this is correct because the existing-worktree path should always have a prior session. That reasoning is sound for the happy path, but if a codex session directory is wiped externally (e.g., ~/.codex/ cleared), the session will silently fail on re-attach with no recovery path. This is an acceptable trade-off, but it's worth documenting as a known limitation so future maintainers understand why the fallback was intentionally removed.

Similarly, launchClaudeSessionWithFallback now issues a plain claude --continue for non-fresh worktrees with no fallback. Same trade-off applies.

Test coverage: The renamed test (resumes claude) + two new freshWorktree=true tests cover the new branching. The old fallback-chain assertion is correctly removed. ✓

---

Fix 3 — TrackerService.renameItem + slug propagation

src/services/TrackerService.ts, src/App.tsx

The implementation is thorough — index buckets, sessions metadata, on-disk directory, and frontmatter all move together.

Potential atomicity gap in renameItem:

writeJSONAtomic(this.getIndexPath(projectPath), index);   // ← index updated
// ...
if (fs.existsSync(oldDir) && !fs.existsSync(newDir)) {
  fs.renameSync(oldDir, newDir);                           // ← could fail here
  this.rewriteSlugFrontmatter(newDir, oldSlug, newSlug);
}

If fs.renameSync throws after writeJSONAtomic succeeds, the index will reference newSlug but the directory will still be at oldSlug. This is unlikely on the same filesystem, but it's worth considering for error handling. A silent try/catch around the rename block (logging the error) would limit the blast radius.

ensureItemWorktree rename path in App.tsx:

const renamed = tracker.renameItem(project.path, item.slug, created.feature);
if (renamed) activeItem = {...item, slug: created.feature};

If renameItem returns false (collision or unknown slug), activeItem keeps the original slug, but created.feature is the suffixed name. These now diverge — ensureItemFiles will be called with the old slug against the suffixed worktree path. This edge case may be low-probability in practice, but it could leave the tracker in an inconsistent state silently. Adding a log or a UI notice when renamed === false and created.feature !== item.slug would make debugging easier.

Test coverage: Four new unit tests in tracker.test.ts cover happy path, collision rejection, unknown-slug rejection, and frontmatter rewrite. The directory-rename test seeds the directory manually to work around createItem not materialising it — that's a reasonable approach. ✓

---

rewriteSlugFrontmatter — minor

The comment correctly notes that oldSlug is regex-unescaped but constrained to [a-z0-9-]. The double-check (parseFrontmatter → frontmatter.slug !== oldSlug) before the regex replace is a nice guard against false positives in the body.

---

Tracker meta-files

tracker/items/ralph-tmux-codex-fixes/ (implementation.md, notes.md, requirements.md, status.json) are committed as part of the PR. This is consistent with how the project manages itself, so no issue — just flagging for reviewers unfamiliar with the pattern.

---

What's missing

1. E2E test for slug drift end-to-end. The unit tests cover renameItem and the git fix in isolation, but there's no mock-rendered E2E test that walks handleAttachSession → ensureItemWorktree → renameItem when the worktree lands on a suffixed name. Given the multi-component nature of fix 3, an integration-level test would give higher confidence. Not a blocker, but recommended as a follow-up.

2. Manual smoke test not checked. The PR's own test plan has the manual smoke test unchecked. Noting this so the reviewer can confirm before merging.

---

Summary

Area	Assessment
Code correctness	✅ Fixes are correct for the stated bugs
Conventions	✅ Follows project patterns (service layer, ESM imports, TypeScript strict)
Test coverage	✅ Good unit coverage; E2E gap noted above
Security	✅ No new attack surface; shellQuote used on prompt args
Performance	✅ Conditional fetch avoids unnecessary network call
Risk	⚠️ Fallback chain removal + renameItem atomicity gap — both low-probability, documented above

Overall this is a clean, well-scoped set of fixes. Address the atomicity gap and the silent-failure path in ensureItemWorktree before merging, and the PR is in good shape.

[claude]

Code Review — PR #231: Fixes from ralph-tmux-codex-test exploratory campaign

Overview

Three targeted bug fixes covering: (1) worktree creation on local-only repos without an origin remote, (2) fresh AI launch in just-created worktrees (no stale resume --last), and (3) slug/worktree name drift when GitService auto-suffixes branch names. 757 tests pass; typecheck and build are clean. Well-scoped and well-documented.

---

Fix 1 — GitService.createWorktree on local-only repos

hasOriginRemote:

const out = runCommandQuick(['git', '-C', repoPath, 'remote', 'get-url', 'origin']);
return !!out && !/fatal/i.test(out);

• Functionally correct for the common case.
• The fatal string check is slightly fragile — it relies on git's English error output. A more robust approach would be to check the exit code (non-zero when origin doesn't exist), but given the current runCommandQuick API that returns output rather than exit codes, this is a reasonable workaround. Worth a comment explaining the limitation.
• The test correctly covers both the origin-present and origin-absent paths. ✓
• The removed origin/ re-prefix was the root cause; trusting findBaseBranch's return value is the right fix. ✓

---

Fix 2 — freshWorktree flag threading

Behavioral change: the old || fallback chain (resume || fresh) is completely removed for existing-worktree paths. The new code trusts that an existing worktree always has a resumable session and uses the resume form directly.

This is correct by design, but it introduces a regression risk: if an existing worktree's session state is corrupted (e.g. codex lost its session ID, or the user manually cleared session data), the resume command will fail with no fallback. Previously, the || chain would silently recover.

Suggestion: document this trade-off in a comment on launchAISessionWithFallback — the name still says "WithFallback" but the fallback is gone. Either rename the method or add a note explaining the fallback was deliberately removed.

pendingWorktreeFresh reset: the state is correctly set every time showAIToolSelection is called, so no stale true value can persist across calls. ✓

Default false: all existing call sites that don't pass freshWorktree default to false which preserves prior behavior. ✓

Subtle issue in handleAttachSession: when the user manually re-attaches to an existing tracker item, fresh = ensured.fresh. But ensureItemWorktree only returns fresh: true when recreateImplementWorktree was called (i.e. the worktree didn't exist before this call). For an existing worktree, fresh = false, so resume is used. That's correct. ✓

---

Fix 3 — TrackerService.renameItem + slug propagation

renameItem implementation:

• All the right places are updated: index buckets, sessions key, on-disk directory, frontmatter. ✓
• The writeJSONAtomic call for index is good. However, fs.writeFileSync in rewriteSlugFrontmatter is non-atomic — a crash mid-write would corrupt the markdown file. Low probability in practice, but writeFileSync with a temp-file-and-rename pattern would be safer if this is ever used in a hot path.
• The regex-safety comment for isValidSlug constraining to [a-z0-9-] is appropriate. ✓

ensureItemWorktree in App.tsx:

const renamed = tracker.renameItem(project.path, item.slug, created.feature);
if (renamed) activeItem = {...item, slug: created.feature};

If renameItem returns false (which it can — collision or unknown slug), activeItem stays as the original item.slug, but created.path corresponds to created.feature. The subsequent ensureItemFiles call uses activeItem.slug (old slug) while the worktree is at the new location. This mismatch would be silent. Consider logging a warning when renamed === false after created.feature !== item.slug.

Test coverage: the four new renameItem tests are thorough and cover both the happy path and failure modes. ✓

---

Tracker files committed

The tracker/items/ralph-tmux-codex-fixes/ docs (requirements, notes, implementation, status.json) look like internal workflow artifacts. These are fine to commit per the project's tracker convention, but status.json with "state": "working" at merge time should ideally be updated to "done" before the PR lands.

---

Summary

Good fixes with clear rationale. Items to address before merge:

1. Rename or document launchAISessionWithFallback — the fallback is gone; the name is misleading.
2. Log a warning in ensureItemWorktree when renameItem returns false after detecting a feature name mismatch — silent failure here is hard to debug.
3. (Minor) rewriteSlugFrontmatter uses fs.writeFileSync non-atomically — acceptable for now, but worth noting.
4. Update tracker/items/ralph-tmux-codex-fixes/status.json to "done" before merging.

[claude]

Code Review — PR #231: Fixes from ralph-tmux-codex-test exploratory campaign

Overview

Three targeted fixes closing 4 of 9 confirmed bugs. The PR description is unusually transparent about what's still open and why — that's a model to keep.

---

Fix 1 — fix(git): no-origin worktree creation ✅

Correct fix. Skipping git fetch origin when no origin exists and passing the resolved base branch through unchanged both make sense. The previous synthetic origin/<base> ref construction was only valid for repos with remotes.

Test coverage is good. The new test skips the origin fetch on a local-only repo correctly verifies the fetch is absent and that the worktree-add still runs with the local base branch.

Minor observation: The existing test was renamed from "should handle local base branch by prefixing with origin/" to "passes the resolved base branch through unchanged" — the new name correctly documents the corrected behavior. Worth confirming the underlying createWorktree implementation has the removal of the prefix synthesis, not just the test update. The test change from origin/main to main in the expected call looks right.

---

Fix 2 — fix(sessions): freshWorktree flag threading ✅

Clean implementation. The optional opts?: {freshWorktree?: boolean} parameter threads through createSessionIfNeeded → launchClaudeSessionWithFallback / launchAISessionWithFallback without touching callers that don't need it.

Behavior change to flag: The existing test was updated:

- `claude --continue -n 'feat - proj' || claude -n 'feat - proj'`
+ `claude --continue -n 'feat - proj'`

The fallback chain (|| claude ...) has been removed for the non-fresh attach path. The comment in launchClaudeSessionWithFallback explains this ("trusts that a prior session exists"). This is the right call — the fallback was masking --continue failures in normal attach — but it's a behavior change for existing worktrees and deserves explicit attention from reviewers. If --continue ever fails silently (e.g., corrupted session state), the user will now see a broken session instead of a quietly recovered one. Intentional tradeoff, but worth documenting.

Test coverage is good. Two new tests for freshWorktree=true cover claude and codex paths. gemini is not tested but follows the same launchAISessionWithFallback path as codex so the risk is low.

---

Fix 3 — fix(tracker): renameItem and slug synchronisation ✅

renameItem is thorough:

• Updates all index buckets (iterates index.backlog, index.active, index.done)
• Migrates the sessions metadata key
• Renames the on-disk directory
• Rewrites slug: frontmatter in all .md files

rewriteSlugFrontmatter is correct. The unescaped regex is safe given [a-z0-9-]-constrained slugs, and the comment calls this out explicitly.

One potential gap: renameItem returns false when newSlug already exists in the index — but it doesn't guard against newDir existing on disk when the index doesn't yet know about newSlug. The existing !fs.existsSync(newDir) guard in the dir-rename block handles this silently (skips the rename), so the slug in index.json would be updated but the disk directory would remain at oldDir. In practice this shouldn't happen since renameItem is only called when created.feature !== item.slug (a worktree-create that returned a suffix), but it's a subtle inconsistency if ever called in other contexts.

Test coverage is good. Four unit tests cover the happy path, disk rename + frontmatter rewrite, collision guard, and unknown-old-slug guard. These are the right cases.

ensureItemWorktree refactor is clean. Returning {worktree, item, fresh} is more expressive than the previous implicit bool and avoids the caller having to re-derive item.slug from the worktree name.

---

General

• pendingWorktreeFresh in UIContext defaults to false and is reset on every showAIToolSelection call — no stale-flag leak.
• The dropped codex pane-state detector fix is well-explained. Not a concern.
• 757 passing tests is a healthy signal, but there are no E2E tests covering the create-with-suffix → rename flow end-to-end. Recommended follow-up when terminal tests are extended.

---

Verdict: Approve with minor notes. All three fixes are correct, appropriately scoped, and tested. The removed fallback chain (fix 2) is the one change worth a second look from anyone who owns the session-launch path.