feat(agent): add favorites and recent-projects quick-nav header#46
Conversation
Add a quick-nav header to ws agent surfacing up to five favorites and five recently-touched projects, sorted by last activity. Activity is recorded by stamping the launched project's branch on every enter / p / l / ctrl+s, creating a minimal [[branches]] entry on first launch without setting CreatedBy/CreatedAt (the launcher is not a creation event). Favorites live on Project.Favorite in workspace.toml and sync across machines like any other project metadata. The agent view (all | favorites) persists to [agent].default_view so the next ws agent invocation opens in the same mode on any machine. New CLI surface: ws favorite add|rm|list <project>. New TUI hotkeys: f toggles favorite on a project row; space v flips the view.
Fold the project-level CLAUDE.md content into AGENTS.md so contributors and agents have a single source of truth. Also refresh two stale sections: the sidecar pre-check now lists all four kinds (bootstrap, migrate, add, create), and the reconciler description drops the non-existent Phase 0/3 numbering.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds Explorer TUI with pinned header chips, favorites, flash search, and which-key; implements activity stamping; extends config schema (branches, agent default view); introduces CLI worktree add/list/push/rm and favorites; splits daemon reconciliation and TOML sync; updates docs and tests. ChangesExplorer TUI, activity, and CLI/daemon updates
Sequence Diagram(s)(skipped) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d340c32116
ℹ️ 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".
| m.statusMsg = "favorite: " + err.Error() | ||
| return | ||
| } | ||
| proj.Favorite = target |
There was a problem hiding this comment.
When the selected row comes from the Favorites/Recent header, proj points to a copy produced by headerSections(allProjects(...)), not the element stored in m.workspaces. Updating only this copy means rebuildItems() recomputes the header from stale workspace data, so toggling a header row (especially in favorites-only view) saves workspace.toml but the row remains in the old state until restarting the TUI. Update the matching project in m.workspaces before rebuilding.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/agent/tui.go (1)
453-458:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
g/Gshould respect non-selectable section rows too.Lines 453-458 set the cursor directly and can land on
KindSection, which breaks the “cursor skips headers” behavior implemented elsewhere.💡 Suggested fix
@@ case "G": m.cursor = len(m.items) - 1 + m.clampCursor() m.ensureVisible() case "g": m.cursor = 0 + m.clampCursor() m.scroll = 0🤖 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 `@internal/agent/tui.go` around lines 453 - 458, The G/g handlers set m.cursor directly which can land on header rows (KindSection) and break the “skip headers” behavior; change the logic in the "G" and "g" cases to advance m.cursor from the target index (len(m.items)-1 for "G", 0 for "g") forward/backward until m.items[m.cursor].Kind != KindSection (or bounds reached), set m.scroll appropriately, and then call m.ensureVisible(); use the existing m.items, m.cursor, ensureVisible, and KindSection symbols to locate and implement the loop that finds the first/last selectable row instead of assigning the raw index.
🤖 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 `@AGENTS.md`:
- Around line 57-63: Add a language identifier to the fenced code block
containing the directory tree (the triple-backtick block with "personal/ ├──
myapp/ ..." ) so markdownlint MD040 is satisfied; change the opening fence from
``` to ```text (or another appropriate language like `bash`/`text`) while
leaving the block contents unchanged.
In `@internal/agent/stamp_test.go`:
- Around line 147-148: The test currently sets deep := filepath.Join(mainPath,
".") which normalizes to mainPath and doesn't exercise root discovery; change
the test to join a real nested subpath (e.g., filepath.Join(mainPath, "subdir")
or "a/b") and create that directory before invoking StampLaunchFromPath so
StampLaunchFromPath(deep) runs from a deeper directory and validates upward root
discovery; update references to the deep variable and ensure any cleanup or
MkdirAll is used so the nested path exists during the test.
In `@internal/agent/tui.go`:
- Around line 1166-1175: Header quick-nav rows are storing pointers to temporary
values returned by headerSections, so calling appendHeaderProject (which takes
&favs[i]) and later toggleFavoriteFor mutates those temporaries and gets
overwritten when rebuildItems rehydrates from m.workspaces; fix by making header
items reference the canonical workspace entries instead of temp copies (e.g.,
change appendHeaderProject and any header creation in headerSections to record a
stable identifier or index and resolve to the real workspace in m.workspaces
before taking a pointer, or have appendHeaderProject accept a workspace ID and
look up the workspace in m.workspaces to call toggleFavoriteFor), adjust code
paths that create header entries (the places noted around headerSections,
appendHeaderProject, and where toggles call toggleFavoriteFor/rebuildItems) so
pointers always point at entries inside m.workspaces rather than ephemeral slice
elements.
- Around line 1562-1569: The branch handling p.Favorite can panic or corrupt
UTF-8 because it slices body with body[len(" * "):] and computes spaces as
w-lipgloss.Width(left) without clamping; fix by avoiding byte-index slicing and
by clamping the padding to >=0. Replace the current construction so you render
the star and the rest separately (e.g., favoriteStarStyle.Render(" * ") +
itemStyle.Render(" "+iconProject+" "+name) instead of slicing body), compute
padding := w - lipgloss.Width(left) and if padding < 0 set padding = 0 before
calling strings.Repeat, and use those symbols (p.Favorite, favoriteStarStyle,
itemStyle, body, lipgloss.Width(left)) to locate and update the code.
In `@internal/cli/agent.go`:
- Around line 13-27: Add a microbenchmark for the new loadAgentDefaultView
function by creating a *_bench_test.go file in the internal/cli package and
implementing at least one Benchmark function (e.g.,
BenchmarkLoadAgentDefaultView_Empty) that calls loadAgentDefaultView(nil) inside
the b.N loop; optionally add a second benchmark that constructs a minimal
agent.WorkspaceData slice (one workspace) and calls loadAgentDefaultView with it
to exercise the config.Load path. Ensure the file uses package cli and imports
testing so the benchmark runs with go test -bench.
---
Outside diff comments:
In `@internal/agent/tui.go`:
- Around line 453-458: The G/g handlers set m.cursor directly which can land on
header rows (KindSection) and break the “skip headers” behavior; change the
logic in the "G" and "g" cases to advance m.cursor from the target index
(len(m.items)-1 for "G", 0 for "g") forward/backward until
m.items[m.cursor].Kind != KindSection (or bounds reached), set m.scroll
appropriately, and then call m.ensureVisible(); use the existing m.items,
m.cursor, ensureVisible, and KindSection symbols to locate and implement the
loop that finds the first/last selectable row instead of assigning the raw
index.
🪄 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: 00b76611-434c-43ae-b78f-df9e31373357
📒 Files selected for processing (16)
AGENTS.mdCLAUDE.mddocs/agent-tui.mdinternal/agent/header.gointernal/agent/header_test.gointernal/agent/persist.gointernal/agent/source.gointernal/agent/stamp.gointernal/agent/stamp_test.gointernal/agent/tui.gointernal/agent/types.gointernal/cli/agent.gointernal/cli/favorite.gointernal/cli/root.gointernal/config/config.gointernal/config/config_test.go
💤 Files with no reviewable changes (1)
- CLAUDE.md
| // loadAgentDefaultView returns the workspace.toml-stored agent.default_view | ||
| // for the first workspace in the list. The TUI is single-view (no per- | ||
| // workspace switching), so we pick the active workspace's preference and | ||
| // use it for the whole session. Returns "all" on any read error so the | ||
| // launcher never fails on a corrupt or missing workspace.toml. | ||
| func loadAgentDefaultView(workspaces []agent.WorkspaceData) string { | ||
| if len(workspaces) == 0 { | ||
| return config.AgentViewAll | ||
| } | ||
| ws, err := config.Load(workspaces[0].Root) | ||
| if err != nil { | ||
| return config.AgentViewAll | ||
| } | ||
| return ws.AgentDefaultView() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add at least one microbenchmark for the new internal/cli logic.
This change adds new execution-path code in internal/cli without accompanying *_bench_test.go coverage.
Starter benchmark skeleton
package cli
import "testing"
func BenchmarkLoadAgentDefaultView_Empty(b *testing.B) {
for i := 0; i < b.N; i++ {
_ = loadAgentDefaultView(nil)
}
}Also applies to: 143-151
🤖 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 `@internal/cli/agent.go` around lines 13 - 27, Add a microbenchmark for the new
loadAgentDefaultView function by creating a *_bench_test.go file in the
internal/cli package and implementing at least one Benchmark function (e.g.,
BenchmarkLoadAgentDefaultView_Empty) that calls loadAgentDefaultView(nil) inside
the b.N loop; optionally add a second benchmark that constructs a minimal
agent.WorkspaceData slice (one workspace) and calls loadAgentDefaultView with it
to exercise the config.Load path. Ensure the file uses package cli and imports
testing so the benchmark runs with go test -bench.
Add four rules borrowed from the Sunny agent playbook, adapted to Go: - No big files: 500-line soft split, 800-line hard split. - No decorative section separators — extract instead. - Comments are a last resort. - Architectural changes ask first, with explicit counter-examples so split-on-cohesive-seam stays a just-do-it. The Rust-specific 'pub or nothing' rule is dropped — Go visibility is already binary by identifier case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7a353fa29
ℹ️ 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".
| p.Branches = append(p.Branches, BranchMeta{ | ||
| Name: name, | ||
| Machines: []string{machine}, | ||
| LastActiveMachine: machine, | ||
| LastActiveAt: stamp, | ||
| }) |
There was a problem hiding this comment.
When ws agent shell/launch runs inside a sibling worktree whose branch is not already in [[branches]] (for example a legacy or out-of-band worktree), this append persists a new registry entry with Machines set. That silently re-registers branches even though unregistered legacy worktrees are supposed to remain no-ops until ws worktree add, and the reconciler/list UI will now treat the branch as managed; stamp only existing branch entries, or explicitly limit auto-creation to the default branch used for Recent.
Useful? React with 👍 / 👎.
| // scanability without using a different background. | ||
| if p.Favorite { | ||
| body := fmt.Sprintf(" %s %s", iconProject, name) | ||
| return favoriteStarStyle.Render(" * ") + itemStyle.Render(body[len(" * "):]) + strings.Repeat(" ", w-lipgloss.Width(left)) |
There was a problem hiding this comment.
When a favorite has no recorded activity (right == "") and its header label is wider than the list width (long project name or a narrow terminal), w-lipgloss.Width(left) becomes negative and strings.Repeat panics, crashing ws agent while rendering the Favorites header. Clamp the padding or reuse the existing padRight/truncation path before repeating spaces.
Useful? React with 👍 / 👎.
Break the 691-line config.go into Project (project.go), BranchMeta and its methods (branch.go), Validate/ValidationIssue (validate.go), and legacy [[autopush]] migration (legacy.go). config.go retains Workspace, Meta, Group, Daemon, AgentConfig, path resolution, Load/Save, and the package-private contains/sortedDedup helpers. Pure move; no behaviour change. All existing tests pass.
Break the 531-line worktree.go into per-subcommand files. worktree.go keeps the root cobra wiring (newWorktreeCmd) and three shared helpers (resolveProject, locateWorktreeForBranch, validateBranchName). worktree_add.go / worktree_list.go / worktree_rm.go / worktree_push.go own one cobra command each. worktreeStateString moves with list since it is its only caller. Pure move; no behaviour change.
Break the 617-line migrate.go into check.go (Check + CheckResult), hooks.go (listActiveHooks, copyHooks, copyFilePreservingMode), and resolve.go (commitReachableFromAnyBranch, resolveDefaultBranch). migrate.go retains the MigrateProject core, Options, Result, ErrAlreadyMigrated, and the 2-line rollbackBare (its only caller is MigrateProject). Pure move; no behaviour change.
Pull the bubbletea model (state, Update, decision flow) into migrate_model.go and the View renderers into migrate_view.go. migrate_tui.go retains the runMigrateTUI entry point, buildMigratePlan, commitMigrate, and the migratePlanItem/migrateState plan types. Pure move; no behaviour change.
Pull the bubbletea model (state, Update, branch-prompt routing, startClone goroutine) into bootstrap_model.go and the View renderers plus the cli-package lipgloss styles into bootstrap_view.go. bootstrap.go retains newBootstrapCmd, runBootstrap, commitBootstrap, and printPlanText. Pure move; no behaviour change.
Pull the workspace.toml sync (Phase 1) into toml.go, the per-project reconcile loop and auto-clone path (Phase 2) into projects.go, conflict- store and backoff bookkeeping into conflicts.go, and the git/fs helpers into git.go. reconciler.go retains the Reconciler type, NewReconciler, SetAutoBootstrap, Run, and the Tick driver that wires the phases together. Pure move; no behaviour change.
Pull the Bubbletea commands (fetchOwnersCmd, createCmd) and msg types into cmd.go, the View renderers and lipgloss styles into render.go, and the standalone runTUI entry point into runner.go. tui.go retains the Model, NewCreateModel, Init, Update, and the key/focus state machine. Pure move; no behaviour change.
Pull each lifecycle stage of the add TUI into its own file (gather, browse, manual, edit + confirm + bulk, clone + branchprompt + done), move messages into msg.go, generic text/source-chip helpers into format.go, and lipgloss styles into styles.go. tui.go retains the Model definition, NewAddModel, Init, the streaming startSource cmd, Update and View dispatchers, and the state-machine helpers. Pure move; no behaviour change.
Pull each piece of the agent TUI into its own file: - list.go owns updateList (the cursor's main dispatch loop). - whichkey.go owns the action panel: actions table, updateWhichKey, toggleAgentView, toggleFavoriteFor, whichKeyTitle, viewWhichKey. - forms.go owns the worktree-create and prompt-input forms. - flash.go owns the flash-search state machine and label assignment. - items.go owns rebuildItems and the list-construction helpers (clampCursor, addProjectItem, the section append*). - render.go owns View dispatch, viewList, and every per-kind renderer (group/project/header-project/worktree/session/section) plus the rendering helpers (truncateStr, renderSelected, padRight). - styles.go owns the lipgloss palette. tui.go retains the Model, listItem, LaunchRequest, NewModel, Init, Update, View dispatcher, and the cursor/scroll/breadcrumb helpers that every other file calls into. Pure move; no behaviour change.
Production .go functions over gocyclo 15 are split now; functions over 10 are split on the next touch. Mirrors the file-length rule's hard/soft split. gocyclo invocation included so reviewers can run the gate locally.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1accb3eb
ℹ️ 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".
| // scanability without using a different background. | ||
| if p.Favorite { | ||
| body := fmt.Sprintf(" %s %s", iconProject, name) | ||
| return favoriteStarStyle.Render(" * ") + itemStyle.Render(body[len(" * "):]) + strings.Repeat(" ", w-lipgloss.Width(left)) |
There was a problem hiding this comment.
For a favorite with no recorded activity, right == "" takes this path; body is built without the " * " prefix, so body[len(" * "):] starts in the middle of the UTF-8 iconProject rune. Newly favorited zero-activity projects therefore render the Favorites header with a corrupted/missing icon or label; render body from a valid boundary (or avoid slicing this string) before padding.
Useful? React with 👍 / 👎.
Drop the in-list Favorites/Recent section rows and render up to nine numbered chips on one or two pinned lines above the scrollable tree. The chip header stays visible while the tree scrolls under it, fixing the short-Hyprland-tile case where favorites disappeared off the top. Also drops the all/favorites view toggle and the [agent].default_view preference plumbing — the pinned header makes the toggle redundant. KindSection, inHeader, sectionTitle, view-toggle code, isSelectable and nextSelectable are removed; every row in m.items is now a real selectable item. Tree rows now indent four spaces per level instead of two for clearer parent/child relationship at a glance.
Detect each project's primary language via marker files (go.mod, Cargo.toml, pyproject.toml, tsconfig.json, package.json, Gemfile, pom.xml, build.gradle, Dockerfile) and render its Nerd Font glyph in the tree. Falls back to a top-level extension scan when no marker fires, then to a Markdown glyph for README-only repos, then to the generic package icon. Results are memoized per project path. Marker order disambiguates polyglot repos correctly: a Go project that also ships a Dockerfile reads as Go because go.mod is checked before Dockerfile.
Press 1-9 in the tree to launch the corresponding pinned chip project. Digits never collide with the tree navigation keys (j/k/h/l/ g/G/q) and are checked before the navigation switch so a chip launch beats any future single-digit binding.
The TUI is an explorer over projects/worktrees/sessions, not an agent. Rename the cobra command, the entry point, and the docs file to match. `ws agent` keeps working as a backwards-compat alias. Source files: internal/cli/agent.go → internal/cli/explorer.go, docs/agent-tui.md → docs/explorer.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483ff103da
ℹ️ 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".
| newWorktreeCmd(), | ||
| newBootstrapCmd(), | ||
| newAgentCmd(), | ||
| newExplorerCmd(), |
There was a problem hiding this comment.
Registering the TUI as explorer (with agent only as an alias) leaves PersistentPreRunE checking only cmd.Name() == "agent", but Cobra exposes the command's canonical name here. As a result, ws agent, ws explorer, and their launch/shell/resume subcommands still run config.FindRoot() and fail when invoked from $HOME or any non-workspace directory, before the explorer can load daemon-registered workspaces as intended. Include the canonical explorer command (and its children) in the preload skip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cli/root.go (1)
19-99: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
NewRootCmdon this touch to satisfy complexity policy.This function is carrying command wiring plus preload policy plus default execution behavior in one block. It has already crossed the “split on next touch before adding new branches” threshold.
As per coding guidelines
**/*.go: “Cyclomatic complexity must not exceed 15 in production Go functions; extract now. Functions over 10 must be split on next touch before adding new branches.”🤖 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 `@internal/cli/root.go` around lines 19 - 99, NewRootCmd is too complex: extract the large PersistentPreRunE workspace-loading branch and the RunE TTY check into small helpers to reduce cyclomatic complexity. Create a helper like shouldSkipWorkspaceLoad(cmd *cobra.Command) bool that encapsulates the early returns (help, completion, docs, agent, ws, daemon parent, auth parent) and two workspace helpers: loadOrCreateWorkspaceForSetup(wsRoot *string) error (handles the "setup" branch, os.Getwd and config.LoadOrCreate) and loadWorkspace(wsRoot *string) error (handles FindRoot + config.Load). Replace the inline logic in PersistentPreRunE with calls to shouldSkipWorkspaceLoad and then one call to either loadOrCreateWorkspaceForSetup or loadWorkspace; also move the RunE body into a small runRootBehavior(cmd *cobra.Command, args []string) error that calls runExplorerTUI or cmd.Help. Update NewRootCmd to wire these helpers and keep only minimal orchestration to register flags/commands.internal/cli/explorer.go (1)
105-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale CLI prefix in diagnostics.
At Line 105 and Line 136, logs still use
ws agenteven though this path is nowws explorer, which is confusing in user-facing output.Suggested fix
- fmt.Fprintf(os.Stderr, "ws agent: %s\n", d) + fmt.Fprintf(os.Stderr, "ws explorer: %s\n", d) ... - fmt.Fprintf(os.Stderr, "ws agent: stamp activity: %v\n", err) + fmt.Fprintf(os.Stderr, "ws explorer: stamp activity: %v\n", err)Also applies to: 136-136
🤖 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 `@internal/cli/explorer.go` around lines 105 - 106, Update the stale CLI prefix "ws agent" used in diagnostic logs to "ws explorer": locate the fmt.Fprintf call that writes "ws agent: %s\n" (and the other matching occurrence around the same area) and change the literal prefix to "ws explorer" so user-facing output reflects the current path name; ensure both instances are updated consistently.
🟡 Minor comments (11)
internal/migrate/hooks.go-30-33 (1)
30-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t silently ignore hook metadata read failures.
If Line 30 fails, the hook is dropped from migration with no visibility. Returning an error here makes partial migrations explicit and debuggable.
Suggested patch
info, err := e.Info() if err != nil { - continue + return nil, fmt.Errorf("stat hook %s: %w", filepath.Join(dir, name), err) }🤖 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 `@internal/migrate/hooks.go` around lines 30 - 33, The code silently ignores errors from e.Info() in internal/migrate/hooks.go which drops hooks without visibility; update the surrounding function (the migration hook processing routine that calls e.Info()) to propagate the error instead of continuing: when e.Info() returns a non-nil err, return that error (or wrap it with context mentioning the hook entry) so the caller sees the failure and partial migrations become explicit; keep the reference to e.Info() and the enclosing function name to locate and change the behavior.internal/cli/worktree_push.go-63-63 (1)
63-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle upstream-setting failures explicitly.
Line 63 drops a non-trivial error path. If tracking setup fails after a successful push, users get no signal and later ahead/behind behavior can be confusing.
Suggested patch
- _ = git.SetBranchUpstream(wtPath, branch, "origin") + if err := git.SetBranchUpstream(wtPath, branch, "origin"); err != nil { + fmt.Fprintf(os.Stderr, "warning: pushed %s but failed to set upstream to origin: %v\n", branch, err) + }🤖 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 `@internal/cli/worktree_push.go` at line 63, The call to git.SetBranchUpstream(wtPath, branch, "origin") currently ignores its returned error; update the code to capture and handle that error instead of assigning to "_" — e.g., check the error from SetBranchUpstream (using wtPath and branch) and either return it to the caller or log a clear warning via the function's logger so users are informed when tracking setup fails after a push; ensure you include context (branch name and wtPath) in the message and preserve existing behavior when no error occurs.internal/config/validate.go-50-64 (1)
50-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate-detail accounting uses the wrong index source.
Line 56 stores
len(seen)instead of the branch slice index, so Line 63 can report incorrect “first at index …” and misleading entry counts.Suggested patch
-func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue { - seen := make(map[string]int, len(branches)) +func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue { + firstIdx := make(map[string]int, len(branches)) + count := make(map[string]int, len(branches)) var out []ValidationIssue - for _, b := range branches { + for i, b := range branches { if b.Name == "" { continue } - prev, isDup := seen[b.Name] - if !isDup { - seen[b.Name] = len(seen) + if _, ok := firstIdx[b.Name]; !ok { + firstIdx[b.Name] = i + count[b.Name] = 1 continue } + count[b.Name]++ + prev := firstIdx[b.Name] out = append(out, ValidationIssue{ Kind: ValidationDuplicateBranch, Project: projName, Branch: b.Name, - Detail: fmt.Sprintf("branch %q has %d entries (first at index %d)", b.Name, prev+1, prev), + Detail: fmt.Sprintf("branch %q has %d entries (first at index %d)", b.Name, count[b.Name], prev), }) } return out }🤖 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 `@internal/config/validate.go` around lines 50 - 64, The duplicate tracking stores seen[b.Name] = len(seen) which records the number of unique keys instead of the actual slice index, causing wrong "first at index" and counts; update the loop to use the branch slice index (e.g. use for i, b := range branches and set seen[b.Name] = i) so prev reflects the true first-occurrence index, and keep the ValidationIssue construction (ValidationDuplicateBranch, Project, Branch, Detail) unchanged.internal/agent/lang.go-39-48 (1)
39-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
pathbefore cache lookup/store.
DetectIconclaims absolute-path caching, but it currently keys by raw input. Relative inputs can return incorrect cached icons across different working directories.Proposed fix
func DetectIcon(path string) string { if path == "" { return iconProject } + if abs, err := filepath.Abs(path); err == nil { + path = abs + } if v, ok := projectIconCache.Load(path); ok { return v.(string) }🤖 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 `@internal/agent/lang.go` around lines 39 - 48, DetectIcon currently keys projectIconCache by the raw input path which allows relative paths to collide; update DetectIcon to normalize the path to an absolute, cleaned form before any cache operations: call filepath.Abs (and filepath.Clean as needed) on the incoming path at the top of DetectIcon, use the resolved path for projectIconCache.Load and Store, and fall back to the original/cleaned path if Abs returns an error; ensure detectIconUncached is invoked with the same normalized path.internal/add/manual.go-39-40 (1)
39-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
manualErronce the input becomes non-empty.The error state persists while the user edits after a failed submit, so the form keeps showing a stale error until the next Enter.
Proposed fix
var cmd tea.Cmd m.manualInput, cmd = m.manualInput.Update(msg) +if strings.TrimSpace(m.manualInput.Value()) != "" { + m.manualErr = "" +} return m, cmd🤖 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 `@internal/add/manual.go` around lines 39 - 40, After calling m.manualInput.Update(msg) in the Update handler, clear the stale error by setting m.manualErr to its zero value whenever the input is non-empty; i.e., after m.manualInput, cmd = m.manualInput.Update(msg) check the current text in m.manualInput (e.g. via m.manualInput.Value() or the appropriate accessor) and if it contains any characters set m.manualErr = nil (or empty string) so the error message is removed while the user edits.internal/cli/worktree_list.go-47-55 (1)
47-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t silently swallow invalid project names or unexpected stat errors.
Right now
list [project]can exit successfully with only a header when the project is unknown, and permission/I/O errors onbarePathare reported as “not migrated”.Proposed fix
for _, name := range names { proj, ok := ws.Projects[name] if !ok { - continue + if len(args) == 1 { + return fmt.Errorf("unknown project %q", name) + } + continue } mainPath := filepath.Join(wsRoot, proj.Path) barePath := layout.BarePath(mainPath) - if _, err := os.Stat(barePath); err != nil { - fmt.Printf("%-20s %s\n", name, "(not migrated)") - continue + if _, err := os.Stat(barePath); err != nil { + if os.IsNotExist(err) { + fmt.Printf("%-20s %s\n", name, "(not migrated)") + continue + } + return fmt.Errorf("stat bare repo for %q: %w", name, err) }🤖 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 `@internal/cli/worktree_list.go` around lines 47 - 55, The code currently ignores unknown project names and treats any os.Stat error on layout.BarePath as "(not migrated)"; update the logic in the function handling the list [project] path (around ws.Projects lookup and the os.Stat call) so that if the project name key lookup (ws.Projects[name]) fails you print a clear error (e.g., "unknown project: <name>") and return a non-zero/error exit, and when checking layout.BarePath use os.Stat error inspection: if os.IsNotExist(err) print "(not migrated)" as before, otherwise surface the real error (print or return it) so permission/I/O errors aren’t masked. Ensure you reference the same symbols (ws.Projects, name, layout.BarePath, os.Stat) when making the changes.internal/daemon/projects.go-236-240 (1)
236-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
path-blockedconflict on successful clone.After a successful auto-clone,
KindPathBlockedcan remain stale because it isn’t cleared in the success path.Suggested fix
// Clear any previously recorded clone failure for this project. _ = r.clearProjectConflict(name, "", conflict.KindCloneFailed) _ = r.clearProjectConflict(name, "", conflict.KindNeedsBootstrap) + _ = r.clearProjectConflict(name, "", conflict.KindPathBlocked)🤖 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 `@internal/daemon/projects.go` around lines 236 - 240, After a successful clone (the code block that calls r.logger.Printf("reconciler: cloned ...") and then _ = r.clearProjectConflict(..., conflict.KindCloneFailed) and _ = r.clearProjectConflict(..., conflict.KindNeedsBootstrap)), also call r.clearProjectConflict(name, "", conflict.KindPathBlocked) to remove any stale path-blocked conflicts; update the success path in the reconciler (where clearProjectConflict is invoked) to include conflict.KindPathBlocked so that successful auto-clone clears that conflict type as well.internal/agent/render.go-231-234 (1)
231-234:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse display width (not rune count) for truncation budget.
At Line 232,
len([]rune(prefix))can miscalculate visible width for glyphs/icons and cause row wrapping.Suggested fix
- maxTitle := w - len([]rune(prefix)) - 1 + maxTitle := w - lipgloss.Width(prefix) - 1🤖 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 `@internal/agent/render.go` around lines 231 - 234, The truncation budget is computed using rune count which miscalculates visible width for icons/glyphs; in the block that builds prefix (using indent and iconSession) replace len([]rune(prefix)) with the display width (e.g. runewidth.StringWidth(prefix)) so maxTitle = w - prefixDisplayWidth - 1, and ensure truncateStr is called with/updated to truncate by display width (or provide a new truncateByWidth helper) so the title is trimmed based on visible columns rather than rune count.internal/config/branch.go-217-232 (1)
217-232:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd empty-input guard in
MarkPushedfor metadata integrity.
MarkPushedcurrently accepts emptyname/machineand can stamp blank provenance fields; other mutators already reject this.Proposed fix
func (p *Project) MarkPushed(name, machine string, when time.Time) bool { + if name == "" || machine == "" { + return false + } b := p.LookupBranch(name) if b == nil { return false }🤖 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 `@internal/config/branch.go` around lines 217 - 232, MarkPushed allows empty name or machine and can write blank provenance fields; add an early guard in Project.MarkPushed to return false if name == "" or machine == "" before calling LookupBranch so no blank LastPushedMachine/LastPushedAt/LastActiveMachine/LastActiveAt are set; keep existing equality check and stamping logic unchanged and reference LookupBranch/B.LastPushedMachine/LastPushedAt/LastActiveMachine/LastActiveAt when locating the change.internal/agent/list.go-15-16 (1)
15-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDelete confirmation should accept uppercase
Y(and ideally Enter) too.This branch currently confirms only
"y", which is inconsistent with other confirmation handlers and easy to miss.Proposed fix
- if msg.String() == "y" && m.deleteItem != nil { + if (msg.String() == "y" || msg.String() == "Y" || msg.String() == "enter") && m.deleteItem != nil {🤖 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 `@internal/agent/list.go` around lines 15 - 16, Update the confirmation branch that currently checks msg.String() == "y" to accept uppercase Y and Enter as well: normalize the input (e.g., strings.TrimSpace(strings.ToLower(msg.String()))) and treat it as confirmation if it equals "y" or if the raw input is an empty string (Enter), while still verifying m.deleteItem != nil; update the condition around m.deleteItem and msg.String() accordingly and add a strings import if missing.AGENTS.md-726-728 (1)
726-728:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a fence language for the gocyclo command block.
Line 726 uses a bare triple-backtick block; this can fail MD040 lint checks.
Suggested fix
-``` +```bash go run github.com/fzipp/gocyclo/cmd/gocyclo@latest -over 10 -ignore '_test\.go' .</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@AGENTS.mdaround lines 726 - 728, The Markdown code fence containing the
gocyclo command (the triple-backtick block with "go run
github.com/fzipp/gocyclo/cmd/gocyclo@latest -over 10 -ignore '_test.go' .")
should include a language specifier to satisfy MD040; update the fence to use
"bash" instead of a bare "" so the block becomes a bash code block.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>internal/config/validate.go (1)</summary><blockquote> `30-42`: _⚡ Quick win_ **Add a microbenchmark for `Workspace.Validate()` in `internal/config`.** This package is in your perf-gated set, and this introduces new scan/sort work over projects/branches. A small benchmark here helps protect against regressions as config size grows. Based on learnings: “Add at least one microbenchmark in *_bench_test.go files when adding new code to hot-path packages: internal/config, internal/cli, internal/conflict, internal/git, internal/clone, cmd/ws”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/validate.go` around lines 30 - 42, Add a microbenchmark for Workspace.Validate by creating an internal/config file named validate_bench_test.go that benchmarks the Validate() method; write a BenchmarkValidate(b *testing.B) that constructs representative Workspace instances (small and larger sizes) — e.g., helper funcs to build a Workspace with N projects each with M branches — call b.ReportAllocs(), reset the timer after setup, and loop b.N times invoking w.Validate() to measure CPU/allocations; keep the benchmark in the same package (internal/config) and use sub-benchmarks (b.Run) for different sizes so CI perf-gating catches regressions on Validate, duplicateBranchIssues, and the sort path. ``` </details> </blockquote></details> <details> <summary>internal/config/config.go (1)</summary><blockquote> `57-111`: _⚡ Quick win_ **Add a microbenchmark for the new `internal/config` helpers.** Given this package is on the cold-start path, please add at least one `*_bench_test.go` benchmark for the new view/root helpers. Based on learnings: Add at least one microbenchmark in `*_bench_test.go` files when adding new code to hot-path packages: `internal/config`, `internal/cli`, `internal/conflict`, `internal/git`, `internal/clone`, `cmd/ws`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 57 - 111, Add a microbenchmark file for internal/config that exercises the new hot-path helpers (e.g. AgentDefaultView, SetAgentDefaultView, FindRoot, FindRootFrom): create an internal/config/config_bench_test.go and add at least one BenchmarkAgentDefaultView/BenchmarkSetAgentDefaultView and one BenchmarkFindRootFrom (mocking/stubbing env/working-dir where needed) to measure cold-start behavior; ensure benchmarks run fast by using b.ResetTimer and b.N loops and isolate filesystem calls (use temporary dirs or in-memory stubs) so CI-friendly microbenchmarks capture realistic perf of these functions. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@internal/add/edit.go:
- Around line 74-87: The backspace handling in applyEditBackspace trims strings
by byte slices (m.editFields.Name/URL/Group) which can break multi-byte UTF-8
runes; update applyEditBackspace to remove the last rune (not the last byte) for
each case by decoding runes (e.g., using rune iteration or last rune index)
similar to how applyEditRunes accepts multi-byte input so that deleting handles
emoji/accents correctly and preserves valid UTF-8.In
@internal/agent/styles.go:
- Around line 52-59: sectionStyle is declared but unused (golangci-lint) —
either remove the declaration or apply it to the place where quick-nav section
labels are rendered; locate the rendering code that outputs the
"Favorites"/"Recent"/divider labels (the function/method that composes quick-nav
or workspace tree headings) and wrap those label strings with
sectionStyle.Render(...) or delete the sectionStyle variable and its comment if
you choose the minimal fix.In
@internal/agent/tui.go:
- Around line 217-228: The unused helper Model.primaryWorkspaceRoot is causing a
lint failure; remove the function declaration (the primaryWorkspaceRoot receiver
method that accesses m.workspaces[0].Root) if it isn't needed, or alternatively
wire it into existing logic such as loadAgentDefaultView or any place that needs
the session workspace root by replacing direct m.workspaces indexing with a call
to primaryWorkspaceRoot to centralize the behavior; ensure you update any
callers and imports accordingly so no unused symbol remains.In
@internal/cli/bootstrap_model.go:
- Around line 184-193: The PromptDefaultBranch callback can nil-deref if program
is unset; guard the program.Send call by checking program != nil before calling
program.Send(needsBranchMsg{...}) and, if nil, return a sensible error via
ans.err (or construct and return an error immediately) so the clone worker
doesn't panic; update the PromptDefaultBranch function (referencing
PromptDefaultBranch, program.Send, needsBranchMsg, and ch) to perform this nil
check and return an error when program is nil instead of calling program.Send.- Around line 155-157: The switch case handling the Escape key in the Update
logic uses the wrong key string ("escape"), so update the case labels in the
switch that set m.canceled = true and return tea.Quit (the branch currently
written as case "n", "N", "escape":) to use "esc" instead of "escape" (i.e.,
case "n", "N", "esc":) so Bubble Tea will correctly recognize the Escape key;
keep the m.canceled assignment and tea.Quit return as-is.In
@internal/cli/migrate_model.go:
- Around line 264-266: The calls to m.sidecar.MarkDone and migrate.Save
currently ignore returned errors; capture and handle both errors (from
m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) and
migrate.Save(m.sidecar)) instead of assigning to blank identifier: check each
error and propagate or surface it (e.g., return a wrapped error with context or
log and terminate) so sidecar persistence failures are not silently dropped;
update the function containing these calls to return an error if either
operation fails.In
@internal/cli/root.go:
- Around line 93-94: The explorer command is being added via newExplorerCmd()
but not included in the preload-skip branch, so running "ws explorer" can
trigger config.FindRoot() in non-workspace dirs; update the preload bypass logic
(the branch that currently skips preload for root-mode "ws"/root commands) to
also skip when the active subcommand is "explorer" (or when newExplorerCmd() is
detected), i.e., add "explorer" to the list/condition that prevents
workspace.toml preload so explorer loads lazily like the root "ws" mode.In
@internal/daemon/git.go:
- Around line 32-35: The current error handling returns true when err != nil,
which falsely reports a clean repo; update the handler so that when the git
command returns an error (err != nil) the function treats the repo as not-clean
(return false) or refactor the function to propagate the error to callers
(change signature to return (bool, error) and return the err). Locate the block
using the variables err and out and the expression
strings.TrimSpace(string(out)) == "" and replace the early return true with
either a false return or an error propagation consistent with the function's
API.In
@internal/daemon/toml.go:
- Around line 85-100: The bug is that TOML conflicts are recorded under repoRoot
but cleared using r.root, so they can remain stuck; update the clear path to use
the same workspace key (repoRoot) wherever conflicts are cleared — e.g., change
calls to r.clearTOMLConflicts() (and any other clearing call around the push
retry handling and the similar block at lines 131-137) to use the repoRoot-based
clear method or overload that accepts a workspace key so it clears the exact key
used by r.recordTOMLConflict(repoRoot, ...); ensure all calls referencing r.root
are replaced to consistently use repoRoot and keep
conflict.KindTOMLMerge/KindTOMLPushFailed usage unchanged.In
@internal/migrate/resolve.go:
- Around line 24-27: The current loop calling runGit(repoPath, "merge-base",
"--is-ancestor", sha, b) treats every error as "not an ancestor" and swallows
real failures; update the check in the loop inside resolve (or the function
containing this merge-base logic) to discriminate errors using errors.As(err,
&exec.ExitError{}): if err == nil return true,nil; if the error is an
exec.ExitError with ExitCode() == 1 then continue (commit not an ancestor); for
any other error return false, err so real git failures (permissions, repository
corruption, etc.) are propagated. Ensure you import "errors" and "os/exec" if
not already present.
Outside diff comments:
In@internal/cli/explorer.go:
- Around line 105-106: Update the stale CLI prefix "ws agent" used in diagnostic
logs to "ws explorer": locate the fmt.Fprintf call that writes "ws agent: %s\n"
(and the other matching occurrence around the same area) and change the literal
prefix to "ws explorer" so user-facing output reflects the current path name;
ensure both instances are updated consistently.In
@internal/cli/root.go:
- Around line 19-99: NewRootCmd is too complex: extract the large
PersistentPreRunE workspace-loading branch and the RunE TTY check into small
helpers to reduce cyclomatic complexity. Create a helper like
shouldSkipWorkspaceLoad(cmd *cobra.Command) bool that encapsulates the early
returns (help, completion, docs, agent, ws, daemon parent, auth parent) and two
workspace helpers: loadOrCreateWorkspaceForSetup(wsRoot *string) error (handles
the "setup" branch, os.Getwd and config.LoadOrCreate) and loadWorkspace(wsRoot
*string) error (handles FindRoot + config.Load). Replace the inline logic in
PersistentPreRunE with calls to shouldSkipWorkspaceLoad and then one call to
either loadOrCreateWorkspaceForSetup or loadWorkspace; also move the RunE body
into a small runRootBehavior(cmd *cobra.Command, args []string) error that calls
runExplorerTUI or cmd.Help. Update NewRootCmd to wire these helpers and keep
only minimal orchestration to register flags/commands.
Minor comments:
In@AGENTS.md:
- Around line 726-728: The Markdown code fence containing the gocyclo command
(the triple-backtick block with "go run
github.com/fzipp/gocyclo/cmd/gocyclo@latest -over 10 -ignore '_test.go' .")
should include a language specifier to satisfy MD040; update the fence to use
"bash" instead of a bare "" so the block becomes a bash code block.In
@internal/add/manual.go:
- Around line 39-40: After calling m.manualInput.Update(msg) in the Update
handler, clear the stale error by setting m.manualErr to its zero value whenever
the input is non-empty; i.e., after m.manualInput, cmd =
m.manualInput.Update(msg) check the current text in m.manualInput (e.g. via
m.manualInput.Value() or the appropriate accessor) and if it contains any
characters set m.manualErr = nil (or empty string) so the error message is
removed while the user edits.In
@internal/agent/lang.go:
- Around line 39-48: DetectIcon currently keys projectIconCache by the raw input
path which allows relative paths to collide; update DetectIcon to normalize the
path to an absolute, cleaned form before any cache operations: call filepath.Abs
(and filepath.Clean as needed) on the incoming path at the top of DetectIcon,
use the resolved path for projectIconCache.Load and Store, and fall back to the
original/cleaned path if Abs returns an error; ensure detectIconUncached is
invoked with the same normalized path.In
@internal/agent/list.go:
- Around line 15-16: Update the confirmation branch that currently checks
msg.String() == "y" to accept uppercase Y and Enter as well: normalize the input
(e.g., strings.TrimSpace(strings.ToLower(msg.String()))) and treat it as
confirmation if it equals "y" or if the raw input is an empty string (Enter),
while still verifying m.deleteItem != nil; update the condition around
m.deleteItem and msg.String() accordingly and add a strings import if missing.In
@internal/agent/render.go:
- Around line 231-234: The truncation budget is computed using rune count which
miscalculates visible width for icons/glyphs; in the block that builds prefix
(using indent and iconSession) replace len([]rune(prefix)) with the display
width (e.g. runewidth.StringWidth(prefix)) so maxTitle = w - prefixDisplayWidth- 1, and ensure truncateStr is called with/updated to truncate by display width
(or provide a new truncateByWidth helper) so the title is trimmed based on
visible columns rather than rune count.In
@internal/cli/worktree_list.go:
- Around line 47-55: The code currently ignores unknown project names and treats
any os.Stat error on layout.BarePath as "(not migrated)"; update the logic in
the function handling the list [project] path (around ws.Projects lookup and the
os.Stat call) so that if the project name key lookup (ws.Projects[name]) fails
you print a clear error (e.g., "unknown project: ") and return a
non-zero/error exit, and when checking layout.BarePath use os.Stat error
inspection: if os.IsNotExist(err) print "(not migrated)" as before, otherwise
surface the real error (print or return it) so permission/I/O errors aren’t
masked. Ensure you reference the same symbols (ws.Projects, name,
layout.BarePath, os.Stat) when making the changes.In
@internal/cli/worktree_push.go:
- Line 63: The call to git.SetBranchUpstream(wtPath, branch, "origin") currently
ignores its returned error; update the code to capture and handle that error
instead of assigning to "_" — e.g., check the error from SetBranchUpstream
(using wtPath and branch) and either return it to the caller or log a clear
warning via the function's logger so users are informed when tracking setup
fails after a push; ensure you include context (branch name and wtPath) in the
message and preserve existing behavior when no error occurs.In
@internal/config/branch.go:
- Around line 217-232: MarkPushed allows empty name or machine and can write
blank provenance fields; add an early guard in Project.MarkPushed to return
false if name == "" or machine == "" before calling LookupBranch so no blank
LastPushedMachine/LastPushedAt/LastActiveMachine/LastActiveAt are set; keep
existing equality check and stamping logic unchanged and reference
LookupBranch/B.LastPushedMachine/LastPushedAt/LastActiveMachine/LastActiveAt
when locating the change.In
@internal/config/validate.go:
- Around line 50-64: The duplicate tracking stores seen[b.Name] = len(seen)
which records the number of unique keys instead of the actual slice index,
causing wrong "first at index" and counts; update the loop to use the branch
slice index (e.g. use for i, b := range branches and set seen[b.Name] = i) so
prev reflects the true first-occurrence index, and keep the ValidationIssue
construction (ValidationDuplicateBranch, Project, Branch, Detail) unchanged.In
@internal/daemon/projects.go:
- Around line 236-240: After a successful clone (the code block that calls
r.logger.Printf("reconciler: cloned ...") and then _ =
r.clearProjectConflict(..., conflict.KindCloneFailed) and _ =
r.clearProjectConflict(..., conflict.KindNeedsBootstrap)), also call
r.clearProjectConflict(name, "", conflict.KindPathBlocked) to remove any stale
path-blocked conflicts; update the success path in the reconciler (where
clearProjectConflict is invoked) to include conflict.KindPathBlocked so that
successful auto-clone clears that conflict type as well.In
@internal/migrate/hooks.go:
- Around line 30-33: The code silently ignores errors from e.Info() in
internal/migrate/hooks.go which drops hooks without visibility; update the
surrounding function (the migration hook processing routine that calls e.Info())
to propagate the error instead of continuing: when e.Info() returns a non-nil
err, return that error (or wrap it with context mentioning the hook entry) so
the caller sees the failure and partial migrations become explicit; keep the
reference to e.Info() and the enclosing function name to locate and change the
behavior.
Nitpick comments:
In@internal/config/config.go:
- Around line 57-111: Add a microbenchmark file for internal/config that
exercises the new hot-path helpers (e.g. AgentDefaultView, SetAgentDefaultView,
FindRoot, FindRootFrom): create an internal/config/config_bench_test.go and add
at least one BenchmarkAgentDefaultView/BenchmarkSetAgentDefaultView and one
BenchmarkFindRootFrom (mocking/stubbing env/working-dir where needed) to measure
cold-start behavior; ensure benchmarks run fast by using b.ResetTimer and b.N
loops and isolate filesystem calls (use temporary dirs or in-memory stubs) so
CI-friendly microbenchmarks capture realistic perf of these functions.In
@internal/config/validate.go:
- Around line 30-42: Add a microbenchmark for Workspace.Validate by creating an
internal/config file named validate_bench_test.go that benchmarks the Validate()
method; write a BenchmarkValidate(b *testing.B) that constructs representative
Workspace instances (small and larger sizes) — e.g., helper funcs to build a
Workspace with N projects each with M branches — call b.ReportAllocs(), reset
the timer after setup, and loop b.N times invoking w.Validate() to measure
CPU/allocations; keep the benchmark in the same package (internal/config) and
use sub-benchmarks (b.Run) for different sizes so CI perf-gating catches
regressions on Validate, duplicateBranchIssues, and the sort path.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `52df4671-db6b-44a2-865d-85b49130ec43` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d340c3211636567cb3619860878fbed40cea982a and 483ff103da7af0340da4bbd511ff4fdd1b51212e. </details> <details> <summary>📒 Files selected for processing (53)</summary> * `AGENTS.md` * `docs/explorer.md` * `internal/add/browse.go` * `internal/add/clone.go` * `internal/add/edit.go` * `internal/add/format.go` * `internal/add/gather.go` * `internal/add/manual.go` * `internal/add/msg.go` * `internal/add/styles.go` * `internal/add/tui.go` * `internal/agent/flash.go` * `internal/agent/forms.go` * `internal/agent/header.go` * `internal/agent/header_test.go` * `internal/agent/items.go` * `internal/agent/lang.go` * `internal/agent/list.go` * `internal/agent/render.go` * `internal/agent/styles.go` * `internal/agent/tui.go` * `internal/agent/whichkey.go` * `internal/cli/bootstrap.go` * `internal/cli/bootstrap_model.go` * `internal/cli/bootstrap_view.go` * `internal/cli/explorer.go` * `internal/cli/migrate_model.go` * `internal/cli/migrate_tui.go` * `internal/cli/migrate_view.go` * `internal/cli/root.go` * `internal/cli/worktree.go` * `internal/cli/worktree_add.go` * `internal/cli/worktree_list.go` * `internal/cli/worktree_push.go` * `internal/cli/worktree_rm.go` * `internal/config/branch.go` * `internal/config/config.go` * `internal/config/legacy.go` * `internal/config/project.go` * `internal/config/validate.go` * `internal/create/cmd.go` * `internal/create/render.go` * `internal/create/runner.go` * `internal/create/tui.go` * `internal/daemon/conflicts.go` * `internal/daemon/git.go` * `internal/daemon/projects.go` * `internal/daemon/reconciler.go` * `internal/daemon/toml.go` * `internal/migrate/check.go` * `internal/migrate/hooks.go` * `internal/migrate/migrate.go` * `internal/migrate/resolve.go` </details> <details> <summary>💤 Files with no reviewable changes (7)</summary> * internal/cli/worktree.go * internal/daemon/reconciler.go * internal/cli/migrate_tui.go * internal/create/tui.go * internal/migrate/migrate.go * internal/cli/bootstrap.go * internal/add/tui.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| func (m *AddModel) applyEditBackspace() { | ||
| switch m.editFocus { | ||
| case 0: | ||
| if len(m.editFields.Name) > 0 { | ||
| m.editFields.Name = m.editFields.Name[:len(m.editFields.Name)-1] | ||
| } | ||
| case 1: | ||
| if len(m.editFields.URL) > 0 { | ||
| m.editFields.URL = m.editFields.URL[:len(m.editFields.URL)-1] | ||
| } | ||
| case 3: | ||
| if len(m.editFields.Group) > 0 { | ||
| m.editFields.Group = m.editFields.Group[:len(m.editFields.Group)-1] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual code in the file
cat -n internal/add/edit.go | sed -n '70,90p'Repository: kuchmenko/workspace
Length of output: 816
🏁 Script executed:
# Also check what the file structure looks like around this function
wc -l internal/add/edit.goRepository: kuchmenko/workspace
Length of output: 88
🏁 Script executed:
# Search for similar string trimming patterns in the codebase to understand if this is a widespread issue
rg 'editFields\.(Name|URL|Group).*:len' -A 2 -B 2Repository: kuchmenko/workspace
Length of output: 684
🏁 Script executed:
# Check if there are any existing rune-handling utilities in the codebase
rg '(rune|utf8|UTF-8)' internal/add/ -t goRepository: kuchmenko/workspace
Length of output: 635
🏁 Script executed:
# Find the type definition of editFields and the AddModel struct
rg 'type AddModel struct' -A 20 internal/add/Repository: kuchmenko/workspace
Length of output: 1146
🏁 Script executed:
# Check if there are any tests with non-ASCII characters
rg '(ä|é|€|😀|\\u|utf)' internal/add/ -t goRepository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# Look at the applyEditRunes function to understand how runes are handled on input
rg -A 15 'func \(m \*AddModel\) applyEditRunes' internal/add/edit.goRepository: kuchmenko/workspace
Length of output: 521
🏁 Script executed:
# Check what the KeyRunes input source is (Bubble Tea TUI)
rg 'KeyRunes|key\.Runes' -B 3 -A 3 internal/add/edit.goRepository: kuchmenko/workspace
Length of output: 268
Use rune-safe backspace to avoid corrupting UTF-8 input.
Lines 78, 82, and 86 trim by bytes, so deleting characters from non-ASCII input (e.g., emoji, accented letters) can leave invalid UTF-8. This is reachable: the code accepts multi-byte runes via applyEditRunes() but deletes them as raw bytes.
Proposed fix
func (m *AddModel) applyEditBackspace() {
+ trimLastRune := func(s string) string {
+ r := []rune(s)
+ if len(r) == 0 {
+ return s
+ }
+ return string(r[:len(r)-1])
+ }
switch m.editFocus {
case 0:
- if len(m.editFields.Name) > 0 {
- m.editFields.Name = m.editFields.Name[:len(m.editFields.Name)-1]
- }
+ m.editFields.Name = trimLastRune(m.editFields.Name)
case 1:
- if len(m.editFields.URL) > 0 {
- m.editFields.URL = m.editFields.URL[:len(m.editFields.URL)-1]
- }
+ m.editFields.URL = trimLastRune(m.editFields.URL)
case 3:
- if len(m.editFields.Group) > 0 {
- m.editFields.Group = m.editFields.Group[:len(m.editFields.Group)-1]
- }
+ m.editFields.Group = trimLastRune(m.editFields.Group)
}
m.editFields.Path = buildPath(m.editFields.Group, m.editFields.Category, m.editFields.Name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *AddModel) applyEditBackspace() { | |
| switch m.editFocus { | |
| case 0: | |
| if len(m.editFields.Name) > 0 { | |
| m.editFields.Name = m.editFields.Name[:len(m.editFields.Name)-1] | |
| } | |
| case 1: | |
| if len(m.editFields.URL) > 0 { | |
| m.editFields.URL = m.editFields.URL[:len(m.editFields.URL)-1] | |
| } | |
| case 3: | |
| if len(m.editFields.Group) > 0 { | |
| m.editFields.Group = m.editFields.Group[:len(m.editFields.Group)-1] | |
| } | |
| func (m *AddModel) applyEditBackspace() { | |
| trimLastRune := func(s string) string { | |
| r := []rune(s) | |
| if len(r) == 0 { | |
| return s | |
| } | |
| return string(r[:len(r)-1]) | |
| } | |
| switch m.editFocus { | |
| case 0: | |
| m.editFields.Name = trimLastRune(m.editFields.Name) | |
| case 1: | |
| m.editFields.URL = trimLastRune(m.editFields.URL) | |
| case 3: | |
| m.editFields.Group = trimLastRune(m.editFields.Group) | |
| } | |
| m.editFields.Path = buildPath(m.editFields.Group, m.editFields.Category, m.editFields.Name) | |
| } |
🤖 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 `@internal/add/edit.go` around lines 74 - 87, The backspace handling in
applyEditBackspace trims strings by byte slices (m.editFields.Name/URL/Group)
which can break multi-byte UTF-8 runes; update applyEditBackspace to remove the
last rune (not the last byte) for each case by decoding runes (e.g., using rune
iteration or last rune index) similar to how applyEditRunes accepts multi-byte
input so that deleting handles emoji/accents correctly and preserves valid
UTF-8.
| case "n", "N", "escape": | ||
| m.canceled = true | ||
| return m, tea.Quit |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/cli/bootstrap_model.go | head -170 | tail -30Repository: kuchmenko/workspace
Length of output: 1289
🏁 Script executed:
rg -A 5 -B 5 'escape|esc' internal/cli/bootstrap_model.goRepository: kuchmenko/workspace
Length of output: 381
🌐 Web query:
Bubble Tea key names Escape esc tea.KeyMsg
💡 Result:
In Bubble Tea (charmbracelet/bubbletea), the escape key is represented by the key string "esc" (i.e., msg.String == "esc").[1][2] So, in your Update loop you typically check: - case tea.KeyMsg: - switch msg.String: case "esc":...[1][2] Why this works: Bubble Tea’s key handling maps an ESC key event to the text "esc" in its internal key name mapping (keyNames includes keyESC: "esc").[3]
Citations:
- 1: Bug: Nested Model Fails to Exit/Switch State via Custom Command charmbracelet/bubbletea#1522
- 2: https://stackoverflow.com/questions/79232977/how-to-avoid-improper-view-updates-with-bubble-tea-tui
- 3: https://github.com/charmbracelet/bubbletea/blob/cc2f03d4/key.go
Change "escape" to "esc" to properly handle Escape key in Bubble Tea.
Bubble Tea maps the Escape key to the string "esc", not "escape". Using "escape" will cause the Escape key handler to silently fail.
Proposed fix
- case "n", "N", "escape":
+ case "n", "N", "esc":
m.canceled = true
return m, tea.Quit🤖 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 `@internal/cli/bootstrap_model.go` around lines 155 - 157, The switch case
handling the Escape key in the Update logic uses the wrong key string
("escape"), so update the case labels in the switch that set m.canceled = true
and return tea.Quit (the branch currently written as case "n", "N", "escape":)
to use "esc" instead of "escape" (i.e., case "n", "N", "esc":) so Bubble Tea
will correctly recognize the Escape key; keep the m.canceled assignment and
tea.Quit return as-is.
| PromptDefaultBranch: func(name string, candidates []string) (string, error) { | ||
| // Send a request into the bubbletea queue and block until | ||
| // the model writes back into ch. | ||
| program.Send(needsBranchMsg{ | ||
| project: name, | ||
| candidates: candidates, | ||
| answer: ch, | ||
| }) | ||
| ans := <-ch | ||
| return ans.branch, ans.err |
There was a problem hiding this comment.
Guard program.Send to prevent nil-pointer panic in clone worker.
If program is not set (or gets reset), Line 187 dereferences nil and crashes the bootstrap flow from a worker goroutine.
Proposed fix
PromptDefaultBranch: func(name string, candidates []string) (string, error) {
+ if program == nil {
+ return "", errors.New("bootstrap UI is not initialized")
+ }
// Send a request into the bubbletea queue and block until
// the model writes back into ch.
program.Send(needsBranchMsg{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PromptDefaultBranch: func(name string, candidates []string) (string, error) { | |
| // Send a request into the bubbletea queue and block until | |
| // the model writes back into ch. | |
| program.Send(needsBranchMsg{ | |
| project: name, | |
| candidates: candidates, | |
| answer: ch, | |
| }) | |
| ans := <-ch | |
| return ans.branch, ans.err | |
| PromptDefaultBranch: func(name string, candidates []string) (string, error) { | |
| if program == nil { | |
| return "", errors.New("bootstrap UI is not initialized") | |
| } | |
| // Send a request into the bubbletea queue and block until | |
| // the model writes back into ch. | |
| program.Send(needsBranchMsg{ | |
| project: name, | |
| candidates: candidates, | |
| answer: ch, | |
| }) | |
| ans := <-ch | |
| return ans.branch, ans.err |
🤖 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 `@internal/cli/bootstrap_model.go` around lines 184 - 193, The
PromptDefaultBranch callback can nil-deref if program is unset; guard the
program.Send call by checking program != nil before calling
program.Send(needsBranchMsg{...}) and, if nil, return a sensible error via
ans.err (or construct and return an error immediately) so the clone worker
doesn't panic; update the PromptDefaultBranch function (referencing
PromptDefaultBranch, program.Send, needsBranchMsg, and ch) to perform this nil
check and return an error when program is nil instead of calling program.Send.
| _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) | ||
| _ = migrate.Save(m.sidecar) | ||
| } |
There was a problem hiding this comment.
Don’t swallow sidecar persistence failures.
At Line 264 and Line 265, errors from MarkDone/Save are ignored, which can silently lose migration progress metadata.
Suggested fix
- if msg.res != nil {
- _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch)
- _ = migrate.Save(m.sidecar)
- }
+ if msg.res != nil {
+ if err := m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch); err != nil {
+ m.errors = append(m.errors, migrateError{project: msg.project, err: err})
+ } else if err := migrate.Save(m.sidecar); err != nil {
+ m.errors = append(m.errors, migrateError{project: msg.project, err: err})
+ }
+ }🤖 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 `@internal/cli/migrate_model.go` around lines 264 - 266, The calls to
m.sidecar.MarkDone and migrate.Save currently ignore returned errors; capture
and handle both errors (from m.sidecar.MarkDone(msg.project,
msg.res.DefaultBranch) and migrate.Save(m.sidecar)) instead of assigning to
blank identifier: check each error and propagate or surface it (e.g., return a
wrapped error with context or log and terminate) so sidecar persistence failures
are not silently dropped; update the function containing these calls to return
an error if either operation fails.
| newExplorerCmd(), | ||
| newFavoriteCmd(), |
There was a problem hiding this comment.
ws explorer should bypass workspace.toml preload like ws root mode.
Adding newExplorerCmd() without adding "explorer" to the preload skip branch means ws explorer may fail on config.FindRoot() in directories outside a workspace, even though explorer is designed to load lazily.
Suggested patch
- if cmd.Name() == "agent" || cmd.Name() == "ws" {
+ if cmd.Name() == "agent" || cmd.Name() == "explorer" || cmd.Name() == "ws" {
return nil
}🤖 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 `@internal/cli/root.go` around lines 93 - 94, The explorer command is being
added via newExplorerCmd() but not included in the preload-skip branch, so
running "ws explorer" can trigger config.FindRoot() in non-workspace dirs;
update the preload bypass logic (the branch that currently skips preload for
root-mode "ws"/root commands) to also skip when the active subcommand is
"explorer" (or when newExplorerCmd() is detected), i.e., add "explorer" to the
list/condition that prevents workspace.toml preload so explorer loads lazily
like the root "ws" mode.
| if err != nil { | ||
| return true | ||
| } | ||
| return strings.TrimSpace(string(out)) == "" |
There was a problem hiding this comment.
Treat git status errors as not-clean (or propagate).
At Line 33, returning true on command failure can mask repository/state errors as “clean”.
Suggested fix
out, err := cmd.Output()
if err != nil {
- return true
+ return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return true | |
| } | |
| return strings.TrimSpace(string(out)) == "" | |
| if err != nil { | |
| return false | |
| } | |
| return strings.TrimSpace(string(out)) == "" |
🤖 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 `@internal/daemon/git.go` around lines 32 - 35, The current error handling
returns true when err != nil, which falsely reports a clean repo; update the
handler so that when the git command returns an error (err != nil) the function
treats the repo as not-clean (return false) or refactor the function to
propagate the error to callers (change signature to return (bool, error) and
return the err). Locate the block using the variables err and out and the
expression strings.TrimSpace(string(out)) == "" and replace the early return
true with either a false return or an error propagation consistent with the
function's API.
| r.recordTOMLConflict(repoRoot, conflict.KindTOMLMerge, err) | ||
| return false, err | ||
| } | ||
| _ = r.clearTOMLConflicts() | ||
| } | ||
|
|
||
| // Push if anything to push. | ||
| if ahead > 0 || behind > 0 { | ||
| if err := git.Push(repoRoot); err != nil { | ||
| // One retry: fetch + rebase + push, mirror of the legacy syncer. | ||
| if perr := runIn(repoRoot, "git", "pull", "--rebase"); perr != nil { | ||
| r.recordTOMLConflict(repoRoot, conflict.KindTOMLMerge, perr) | ||
| return false, perr | ||
| } | ||
| if perr := git.Push(repoRoot); perr != nil { | ||
| r.recordTOMLConflict(repoRoot, conflict.KindTOMLPushFailed, perr) |
There was a problem hiding this comment.
Use a consistent workspace key when recording vs clearing TOML conflicts.
Conflicts are recorded with repoRoot but cleared with r.root. If those differ, TOML conflicts can be permanently “stuck” in the store.
Proposed fix
- r.recordTOMLConflict(repoRoot, conflict.KindTOMLMerge, err)
+ r.recordTOMLConflict(r.root, conflict.KindTOMLMerge, err)
return false, err
}
@@
- r.recordTOMLConflict(repoRoot, conflict.KindTOMLMerge, perr)
+ r.recordTOMLConflict(r.root, conflict.KindTOMLMerge, perr)
return false, perr
}
if perr := git.Push(repoRoot); perr != nil {
- r.recordTOMLConflict(repoRoot, conflict.KindTOMLPushFailed, perr)
+ r.recordTOMLConflict(r.root, conflict.KindTOMLPushFailed, perr)
return false, perr
}Also applies to: 131-137
🤖 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 `@internal/daemon/toml.go` around lines 85 - 100, The bug is that TOML
conflicts are recorded under repoRoot but cleared using r.root, so they can
remain stuck; update the clear path to use the same workspace key (repoRoot)
wherever conflicts are cleared — e.g., change calls to r.clearTOMLConflicts()
(and any other clearing call around the push retry handling and the similar
block at lines 131-137) to use the repoRoot-based clear method or overload that
accepts a workspace key so it clears the exact key used by
r.recordTOMLConflict(repoRoot, ...); ensure all calls referencing r.root are
replaced to consistently use repoRoot and keep
conflict.KindTOMLMerge/KindTOMLPushFailed usage unchanged.
| if err := runGit(repoPath, "merge-base", "--is-ancestor", sha, b); err == nil { | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect helper behavior to distinguish exit-code=1 from real git failures.
rg -n -C4 --type go 'func runGit|merge-base|is-ancestor|ExitCode|exec\.ExitError'
rg -n -C4 --type go 'func commitReachableFromAnyBranch'
rg -n -C4 --type go 'func .*IsAncestor|IsAncestor\('Repository: kuchmenko/workspace
Length of output: 4298
🏁 Script executed:
cat -n internal/migrate/git_helpers.goRepository: kuchmenko/workspace
Length of output: 915
🏁 Script executed:
rg -n "merge-base|is-ancestor|Ancestor" internal/git/ --type goRepository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
fd -t f . internal/git/ | head -20Repository: kuchmenko/workspace
Length of output: 185
🏁 Script executed:
rg -n "^func " internal/git/ --type go | head -30Repository: kuchmenko/workspace
Length of output: 2451
🏁 Script executed:
cat -n internal/git/*.go | grep -A 10 "func.*Branch\|func.*Ancestor\|ExitError"Repository: kuchmenko/workspace
Length of output: 4605
🏁 Script executed:
cat -n internal/migrate/resolve.go | head -40Repository: kuchmenko/workspace
Length of output: 1577
Add exit code discrimination to merge-base --is-ancestor check.
The loop treats all errors from runGit as "not an ancestor". Git's merge-base --is-ancestor exits with code 1 when the commit is not an ancestor, but exits with other codes (128+) for real failures like repository corruption or permission denied. Currently both cases continue the loop, causing real errors to be silently suppressed and the function to return false instead of surfacing the failure.
Use errors.As(..., &exec.ExitError) to discriminate: exit code 1 should continue the loop, but other errors should be returned.
🤖 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 `@internal/migrate/resolve.go` around lines 24 - 27, The current loop calling
runGit(repoPath, "merge-base", "--is-ancestor", sha, b) treats every error as
"not an ancestor" and swallows real failures; update the check in the loop
inside resolve (or the function containing this merge-base logic) to
discriminate errors using errors.As(err, &exec.ExitError{}): if err == nil
return true,nil; if the error is an exec.ExitError with ExitCode() == 1 then
continue (commit not an ancestor); for any other error return false, err so real
git failures (permissions, repository corruption, etc.) are propagated. Ensure
you import "errors" and "os/exec" if not already present.
The project row had a leading ▸/▾ arrow before the language icon indicating whether worktree/session children were expanded; the information was redundant with tab and read as visual noise. Drop it. On the right side, prefix the worktree count badge with ⚡ so the "branches in flight" signal reads at a glance.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca8bce16ff
ℹ️ 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".
| // DefaultView is the view `ws agent` opens with: "all" (favorites | ||
| // + recent header above the full nested tree) or "favorites" (only | ||
| // the favorites section, flat). Empty string means "all". | ||
| DefaultView string `toml:"default_view,omitempty"` |
There was a problem hiding this comment.
When [agent].default_view = "favorites" is present in workspace.toml, the explorer still opens the full tree because this new field is not carried into WorkspaceData/Model and no internal/agent or internal/cli code calls AgentDefaultView before rendering. Users who set the documented preference therefore get no behavior change; load this setting into the TUI and filter the initial item list to favorites when requested.
Useful? React with 👍 / 👎.
| idx := strings.Index(lower, q) | ||
| if idx < 0 { | ||
| return name | ||
| } | ||
| matchEnd := idx + len(q) | ||
| runes := []rune(name) |
There was a problem hiding this comment.
For non-ASCII project, worktree, or session titles, strings.Index and len(q) return byte offsets, but the result is used as indices into []rune(name). If a user searches for a multibyte character such as é in a matching title, runes[idx:matchEnd] can be out of range or slice through the wrong characters, crashing the TUI; convert the byte offsets to rune offsets before slicing.
Useful? React with 👍 / 👎.
Group favorites: a [groups.<name>].favorite=true flag pins a group to the quick-nav chips alongside project favorites. The TUI 'f' hotkey on a group row toggles it, ws favorite add @<name> and ws favorite list also accept groups. Group chips render as @name with the same * marker that projects use. Chip action modal: pressing 1-9 on a header chip now opens a small modal asking what to do — c/enter for claude, p for claude+prompt, s/l for shell, w for new worktree (project chips only), esc to cancel. The chip itself stays a hotkey shortcut but the launch intent is explicit instead of always-claude-always-cwd. Internally, headerProjects []Project becomes headerChips []Chip with a {Kind, Name, Path, Favorite, Project*, WorkspaceRoot} struct so group and project chips share one rendering path. KindSection is dropped (was already dead code after the pinned-header commit).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e204e6ffd
ℹ️ 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".
| err := MutateAndSave(root, func(ws *config.Workspace) bool { | ||
| return ws.SetGroupFavorite(group, target) | ||
| }) |
There was a problem hiding this comment.
When the selected group exists only because a project references group = ... (no [groups.<name>] table yet), ws.SetGroupFavorite returns false, so MutateAndSave skips writing workspace.toml; the code below still updates m.workspaces and shows a favorited status, but the pin disappears on restart. The CLI path auto-registers missing groups before setting the flag, so the TUI should do the same for implicit group rows before calling SetGroupFavorite.
Useful? React with 👍 / 👎.
CI was failing with gocognit min-complexity=7, which contradicted the AGENTS.md rule (10 soft / 15 hard). Bump both gocognit and gocyclo to 15 to match the policy doc. Also clean three stale lint hits from the explorer rework: - replace a 2-line append loop with a single append-spread (S1011) - drop the unused sectionStyle (Favorites/Recent section headers were removed when the header became pinned chips) - drop the unused primaryWorkspaceRoot (last caller toggleAgentView was removed in the view-toggle deletion) PR feedback (CodeRabbit): - AGENTS.md fenced layout block now declares 'text' for MD040. - stamp_test subpath test walks into a real subdirectory instead of filepath.Join(mainPath, '.') which normalized to mainPath.
The earlier refactor commits split four big files:
- internal/cli/worktree.go → worktree_{add,list,rm,push}.go
- internal/cli/bootstrap.go → bootstrap_{model,view}.go (+ original)
- internal/cli/migrate_tui.go → migrate_{model,view}.go (+ original)
- internal/daemon/reconciler.go → projects.go, toml.go, conflicts.go, git.go
The .golangci.yml exclusions still pointed at the pre-split filenames,
so the cognitive-complexity allowances no longer applied to the
moved code. Widen the globs:
- 'internal/cli/.*(tui|model|view|clone).*\.go' covers every
Bubbletea reducer split.
- 'internal/cli/worktree.*\.go' covers all four worktree commands.
- 'internal/daemon/.*\.go' covers the reconciler family.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0fef1ca3d
ℹ️ 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".
| Workspace: m.ws, | ||
| Save: m.saveFn, | ||
| Mode: ModeHeadless, | ||
| NoClone: job.FromDisk != "", // disk-found → register only |
There was a problem hiding this comment.
When a suggestion came from the disk scan, this sets NoClone but still calls Register, which derives Project.Path from category/group/name instead of job.FromDisk. If the repo was found at an existing location such as work/acme/api or tools/api and the user accepts the TUI defaults, workspace.toml records a different path (often personal/api) while the UI says it is registering the local repo; the daemon will then treat the real checkout as unregistered and may bootstrap a duplicate at the recorded path.
Useful? React with 👍 / 👎.
| regOpts := add.Options{ | ||
| Category: category, | ||
| Group: group, | ||
| Name: projectName, | ||
| WsRoot: wsRoot, | ||
| Workspace: ws, | ||
| Save: saveFn, | ||
| } |
There was a problem hiding this comment.
In the interactive ws create path, selecting category work and leaving Group blank passes an empty group through to add.Register, whose fallback is the literal work group. The headless path uses buildRegisterOpts to default an empty work group to the selected owner login, and the CLI help documents that default; without the same normalization here, TUI-created org repos are saved under the wrong group/path unless the user manually retypes the owner.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03a8b976e4
ℹ️ 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".
| Workspace: m.ws, | ||
| Save: m.saveFn, | ||
| Mode: ModeHeadless, | ||
| NoClone: job.FromDisk != "", // disk-found → register only |
There was a problem hiding this comment.
When a queued suggestion came from the disk source, this only sets NoClone and drops job.FromDisk; Register then computes Project.Path from the selected group/category/name instead of the existing checkout path. For an unregistered repo found at work/acme/api or playground/api, the TUI can save a different path (for example personal/api or acme/api) while also skipping the clone, leaving workspace.toml pointing at a directory that does not exist. Pass the disk path through to registration or derive the relative path from FromDisk for disk-sourced jobs.
Useful? React with 👍 / 👎.
2 participants
Problem
ws agentlists every project in every workspace as a flat tree. Pickingthe project I touched 30 seconds ago means scanning the same alphabetical
list each time. There is no concept of pinned projects, and no notion of
recency — even though the data exists in
[[branches]].last_active_at.Solution
Surface up to five favorites + five recently-touched projects in a
header above the tree, sorted by activity desc with name-asc tiebreak.
Project.Favorite boolinworkspace.toml(omitempty). Syncs acrossmachines like any project metadata.
[agent].default_view(all|favorites) persists the view choicecross-machine.
internal/agent/header.go—headerSections(projects)returns(favs, recent)capped atHeaderCap = 5. Favorites with zeroactivity sort to the end of the favorites section; non-favorites with
zero activity never appear in Recent.
internal/agent/stamp.go—StampLaunchFromPath(cwd)walks up tofind
workspace.toml, resolves the project by path, reads HEAD viagit rev-parse, and bumpslast_active_atfor that branch. Createsa minimal
[[branches]]entry on first launch — withoutCreatedBy/CreatedAt, because the launcher is not a creation event(unlike
ws worktree add).enter,p,l,ctrl+s.ws favorite add|rm|list <project>.ftoggles favorite on a project row;space vflips the view.
Also folds
CLAUDE.mdintoAGENTS.mdso there is one source of truth.Blast radius
internal/config—Project.Favorite,AgentConfig,SetFavorite,AgentDefaultView,SetAgentDefaultView,StampActivity,FindRootFrom.internal/agent— new files for header, stamp, persist;tui.gogrows view state, section rows,
f+space vkeybinds, and thenextSelectable/firstSelectableIndexhelpers so the cursor skipssection headings.
internal/cli—favorite.gosubcommands,agent.golaunchstamps +
loadAgentDefaultView,root.goregistersnewFavoriteCmd.AGENTS.md— absorbed CLAUDE.md; refreshed sidecar pre-check(4 kinds) and reconciler phase numbering.
must never break a launch.
StampLaunchFromPathreturns nil forany path that is not inside a workspace; tests cover that case.
Test plan
(favorites currently scroll off the top — header is not pinned;
pinning is part of the follow-up explorer rework)
Favoriteanddefault_viewPerformance
bench-skip: visual review draft — bench gate not run yet, will run
before flipping to ready.
Follow-ups (not in this PR)
Per design interview, the next PR on this branch will rework the
launcher into
ws explorerwith a pinned compact-chip header, 4-spaceindent tree, and language-based icons. This PR ships the data + CLI +
basic TUI integration so the reviewer can validate the model before the
UI rework.
Summary by CodeRabbit
Release Notes
New Features
ws favoritecommand to mark/unmark projects as favorites for quick access.ws worktree add,ws worktree list,ws worktree push, andws worktree rmcommands for branch management.?) for context-aware help and commands.Documentation