refactor: shrink production file count from 150 to 45 (-70%)#50
Merged
Conversation
📝 WalkthroughWalkthroughLarge refactor consolidates CLI commands into root.go, rebuilds add and agent TUIs and sources, integrates GitHub auth and bootstrap sidecars, unifies config branch/machine models, adds conflict notifications, and updates L2 benchmark paths to bench/benchfixture. ChangesBench L2 fixture path switch
Add flow refactor and sources/TUI integration
Agent model/view/TUI consolidation
Alias generation/manager integration
Auth/device flow and bootstrap
CLI consolidation
Config and conflict notifications
Sequence Diagram(s)(none) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
internal/add/add.go (1)
268-277: 💤 Low valueUnused parameter in
inferGroup.The
urlparameter is explicitly ignored with_. If this is intentional as a placeholder for future use, consider removing the parameter entirely until needed (YAGNI), or add a// TODO:marker explaining the intent.♻️ Suggested simplification
-func inferGroup(_ string, cat config.Category) string { +func inferGroup(cat config.Category) string { return string(cat) }Then update the call site at line 228:
- group = inferGroup(url, cat) + group = inferGroup(cat)🤖 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/add.go` around lines 268 - 277, Remove the unused url parameter from inferGroup and update all call sites to use the new signature; specifically, change func inferGroup(_ string, cat config.Category) string to func inferGroup(cat config.Category) string (or alternatively keep the parameter but add a clear TODO comment if you intend to use it), and ensure callers that currently pass a URL argument are adjusted to pass only the category. Also verify buildPath remains unchanged and still accepts group, cat, name.internal/add/sources.go (1)
88-93: 💤 Low valueRedundant error wrapping branches.
Both branches of the
ErrNotAuthedcheck wrap the error identically. The special case has no distinct handling.♻️ Simplify error handling
repos, err := s.Provider.SuggestRepos(ctx, limit) if err != nil { - if errors.Is(err, github.ErrNotAuthed) { - return nil, fmt.Errorf("github source: %w", err) - } return nil, fmt.Errorf("github source: %w", 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/add/sources.go` around lines 88 - 93, The code checks errors.Is(err, github.ErrNotAuthed) but both branches return the same wrapped error, creating redundant logic; simplify by removing the special-case branch and replace the whole conditional with a single return that wraps err (e.g., return nil, fmt.Errorf("github source: %w", err)), keeping the errors.Is/github.ErrNotAuthed check only if you intend to handle that case differently in functions dealing with err.internal/add/tui.go (1)
1-912: ⚖️ Poor tradeoffFile exceeds 800 LOC extraction threshold.
At 912 lines, this file exceeds the 800 LOC limit specified in the coding guidelines: "extract immediately if beyond 800 lines." Consider splitting the style definitions (lines 863-911) into a separate
styles.gofile, or extracting the browse/edit/confirm view logic intotui_views.go.As per coding guidelines: "Split
.gofiles larger than 500 lines on the next touch, and extract immediately if beyond 800 lines."🤖 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/tui.go` around lines 1 - 912, File exceeds size threshold; split large tui.go by extracting the style constants (addTitle, addDim, addHelp, addCursor, addAccent, addErr, addCheck, addChip, addGroupHdr, addItemName, addExists, addExistsTag, addPreviewName, addCursorRow) into a new styles.go in package add (or alternatively extract view functions like viewBrowse, viewEdit, viewConfirm into tui_views.go). Create the new file with the same package declaration, move the var block there, run `go vet`/`go build` to ensure no symbol changes, and keep all references unchanged (e.g., viewBrowse(), viewEdit(), renderSelectionPreview(), renderItemLine(), buildBrowseRows()) so callers continue to compile.internal/cli/bootstrap.go (2)
264-264: 💤 Low valueGlobal mutable
programpointer requires caution.This global is a common bubbletea pattern for sending messages from goroutines, but it creates implicit coupling. The current usage is safe since
runBootstrapsets and clears it within a single call, but consider documenting the invariant or eventually passing the program reference through the model.🤖 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.go` at line 264, The global variable program (var program *tui.Program) is a shared mutable pointer used by runBootstrap to send messages from goroutines, which creates implicit coupling; either document the invariant (that runBootstrap sets program and clears it within its call and no other goroutines access it outside that window) or refactor to remove the global by adding a program field to the bootstrap model and passing that model (or the *tui.Program) into any goroutines that need to send messages; update runBootstrap to set model.program (or accept the program as an argument) and remove/clear the package-level program variable to ensure the coupling is explicit and thread-safe.
577-615: ⚖️ Poor tradeoffFile exceeds 500 LOC guideline.
At 615 lines, this file crosses the 500-line threshold for splitting on next touch. The style variables (lines 577-615) and view helpers (
renderProgressBar,indent) are natural extraction candidates into abootstrap_styles.goor sharedstyles.go. Not blocking for this mechanical refactor PR, but flagging for future.As per coding guidelines: "Split
.gofiles larger than 500 lines on the next touch".🤖 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.go` around lines 577 - 615, This file exceeds the 500-line limit and should be split: extract the style variables (e.g., bsTitleStyle, bsHeaderStyle, bsDimStyle, bsHelpStyle, bsCheckStyle, bsWarnStyle, bsErrStyle, bsArrowStyle, bsBarFilledStyle, bsBarEmptyStyle, errorBannerStyle) and related small view helpers (e.g., renderProgressBar, indent) into a new file such as bootstrap_styles.go or styles.go, update package-level visibility if needed, and ensure any callers in bootstrap.go reference the moved symbols; keep behavior unchanged and run `go vet`/`go test` to verify no import or visibility regressions.internal/cli/worktree.go (2)
140-144: 💤 Low valueSilent error suppression on git operations.
Lines 141, 144, and 206 discard errors from
SetFetchRefspec,FetchRefspec, andSetBranchUpstream. While these may be intentional for non-critical auxiliary operations, they could mask connectivity or permission issues.Consider logging at debug level or returning early only when the operation is truly required for correctness.
🤖 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.go` around lines 140 - 144, The code currently swallows errors from git.SetFetchRefspec, git.FetchRefspec, and git.SetBranchUpstream called in the worktree flow (see HasFetchRefspec, SetFetchRefspec, FetchRefspec, SetBranchUpstream), which can hide connectivity/permission failures; update those calls to capture returned errors and either log them at debug/trace level (via the existing logger) or return the error upstream when the operation is required for correctness (e.g., if FetchRefspec fails and subsequent operations depend on the fetch), ensuring each call checks its error and handles it appropriately rather than discarding it.
121-238: ⚖️ Poor tradeoffCyclomatic complexity exceeds threshold.
The
RunEclosure handles three distinct flows (re-register existing worktree, attach to local/remote branch, create new branch) with approximately 27 decision points. This exceeds the 15-point threshold.Consider extracting helpers such as
handleExistingWorktree,createWorktreeFromExisting, andcreateWorktreeFromBaseto reduce complexity and improve testability.As per coding guidelines: "Cyclomatic complexity must not exceed 15 in production Go functions; extract immediately if over 15."
🤖 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.go` around lines 121 - 238, The RunE closure in worktree.go has excessive cyclomatic complexity; extract logical sections into small helper functions to reduce decision points: implement handleExistingWorktree(projectName, branch, machine, barePath, ws, saveWorkspace) to contain the existing-worktree branch re-registration logic (uses locateWorktreeForBranch, p.ClaimBranch, p.MarkPushed, saveWorkspace), implement createWorktreeFromExisting(barePath, wtPath, branch, remoteExists, fromBase) to encapsulate the local/remote attach flow (uses git.WorktreeAdd, git.SetBranchUpstream) and implement createWorktreeFromBase(barePath, wtPath, branch, base) to handle creating a new branch from a base (uses proj.DefaultBranch and git.WorktreeAdd); have RunE call these helpers and handle updating ws.Projects and printing output so the closure's decision points drop well below 15.internal/aliasmgr/aliasmgr.go (1)
1-12: ⚖️ Poor tradeoffFile exceeds 500 LOC guideline (635 lines).
Per coding guidelines,
.gofiles larger than 500 lines should be split on the next touch. Consider extracting the tree-building logic (buildTree,treeRow, helper functions) or the view rendering (viewManage,viewConfirm) into separate files.As per coding guidelines: "Split
.gofiles larger than 500 lines on the next touch, and extract immediately if beyond 800 lines"🤖 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/aliasmgr/aliasmgr.go` around lines 1 - 12, The file is too large (>500 LOC); extract cohesive parts into separate files: move the tree-building logic (buildTree, treeRow and any helper functions/types they depend on) into a new file (e.g., tree.go) and move view rendering (viewManage, viewConfirm and their helpers) into another file (e.g., view.go), keeping them in the same package aliasmgr; update imports and adjust function/variable visibility (exported vs unexported) so references remain valid, and ensure any shared helper types/constants remain in a small shared file or kept at top of aliasmgr.go to avoid import cycles.internal/auth/auth.go (1)
138-167: ⚡ Quick winHTTP request lacks explicit timeout.
http.DefaultClienthas no timeout, which could cause the CLI to hang indefinitely on network issues. Consider using a client with a timeout for better UX.Suggested approach
var httpClient = &http.Client{Timeout: 30 * time.Second}Then use
httpClient.Do(req)instead ofhttp.DefaultClient.Do(req)throughout the file.🤖 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/auth/auth.go` around lines 138 - 167, The HTTP call in requestDeviceCode (and other auth-related requests) uses http.DefaultClient which has no timeout; create a package-level http client (e.g., var httpClient = &http.Client{Timeout: 30*time.Second}) and replace http.DefaultClient.Do(...) with httpClient.Do(...) in requestDeviceCode and any other functions in internal/auth/auth.go that make HTTP requests so calls can't hang indefinitely; ensure you import time if not already imported.
🤖 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 `@internal/add/clipboard_test.go`:
- Around line 43-52: Update TestSystemClipboardReader_DeadlineExceeded to assert
the specific error type instead of only checking err != nil: call
DefaultClipboardReader.Read(ctx) and use errors.Is to verify the returned err is
either context.DeadlineExceeded or ErrClipboardUnavailable (matching the pattern
used in TestSystemClipboardReader_ContextCancelled). Reference the test function
name TestSystemClipboardReader_DeadlineExceeded and the Read method on
DefaultClipboardReader; replace the weak t.Fatal("expected error") with an
assertion that uses errors.Is(err, context.DeadlineExceeded) || errors.Is(err,
ErrClipboardUnavailable) and fail the test if neither holds.
In `@internal/add/sources.go`:
- Around line 251-305: The shorthand SSH branch in looksLikeGitURL currently
accepts any SSH shorthand because the shorthandRegex block returns true
unconditionally; update that block so it only returns true when the extracted
host is allowed (i.e., keep the existing if allowedHosts[host] { return true }
and remove the unconditional return true), or if accepting all SSH shorthands is
intentional add a short DECISION comment above the shorthandRegex handling
describing that choice and why; ensure you modify only the shorthandRegex branch
(the code that extracts host via m := shorthandRegex.FindStringSubmatch(s)) and
leave the HTTPS/URL logic unchanged.
In `@internal/agent/tui.go`:
- Around line 1-19: The file internal/agent/tui.go is far too large (1,717
lines) and must be split; refactor by extracting related types and functions
into separate files: move Worktree, WorktreeResult, WorktreeCache,
CreateWorktree, DeleteWorktreeWithRegistry, and LoadWorktrees into a new
worktree.go; move Session, SessionCache, LoadSessions, FindSession, and
extractTitle into session.go; move LaunchClaude and LaunchShell into launch.go;
move the Edit project UI handlers (the functions handling edit UI logic
referenced around lines where edit functions appear) into edit.go; ensure you
update package-level imports and any unexported helper references (keep names
and signatures unchanged) and run go vet/go build to fix any missing references.
- Around line 865-867: The current encodeCwd function uses
strings.ReplaceAll(path, "/", "-"), which is lossy; replace it with a reversible
encoding (e.g., base64 RawURLEncoding or url.PathEscape) so encoded names are
unique and reversible. Update encodeCwd to return a reversible encoding (suggest
base64.RawURLEncoding.EncodeToString([]byte(path)) or url.PathEscape(path)), and
update all places that decode or parse these names—specifically FindSession and
LoadSessions—to decode using the matching decoder
(base64.RawURLEncoding.DecodeString or url.PathUnescape) and use the decoded
path for comparisons and session directory lookup so hyphenated paths no longer
collide. Ensure session directory naming and any lookup logic consistently use
the new encoder/decoder pair.
In `@internal/agent/view.go`:
- Around line 488-503: formatInt currently discards the sign for negative
inputs; update formatInt to preserve and emit a leading '-' for negative n by
recording sign (e.g., neg := n < 0), converting n to its absolute value for
digit extraction, and then prefixing '-' to the returned string when neg is
true; alternatively, if negatives are truly invalid, change the API to take a
unsigned type or add an assertion/panic in formatInt to fail fast—but the
quickest fix is to detect negative n and include the minus sign in the returned
string from formatInt.
In `@internal/aliasmgr/aliasmgr.go`:
- Around line 78-82: In buildItems, aliasFor currently maps target->single alias
which overwrites when multiple aliases point to the same target; change aliasFor
from map[string]string to map[string][]string, populate it with aliasFor[t] =
append(aliasFor[t], n) for each n,t in ws.Aliases, and then update any
subsequent code in buildItems that consumes aliasFor to iterate the slice of
aliases (e.g., for _, alias := range aliasFor[target] ...) so all aliases for
the same target are preserved and rendered/checked individually.
In `@internal/cli/bootstrap.go`:
- Around line 397-400: The code currently discards errors from
m.sidecar.MarkDone(...) and bootstrap.Save(m.sidecar); capture both return
values and handle non-nil errors instead of ignoring them: call
m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) and if it returns err log
a warning or error (using the existing logger on the manager struct, e.g.
m.logger or m.log) with context including msg.project and msg.res.DefaultBranch;
then call bootstrap.Save(m.sidecar) and likewise log any error returned (or
accumulate/return the error if the calling flow supports it) so disk/permission
failures are visible and do not silently lose resume state.
- Around line 296-304: When handling tui.KeyMsg for ctrl+c in
bsStepBranchPrompt, avoid returning early and leaking the goroutine waiting on
the branch-response channel; instead ensure the branch prompt is resolved before
quitting by invoking the same cleanup that unblocks the waiter (e.g., call
updateBranchPrompt or directly call resolveBranch with an error/ cancellation)
so ans := <-ch in the clone goroutine can receive and exit, then set m.canceled
and return tui.Quit.
In `@internal/cli/migrate.go`:
- Around line 528-534: The code currently appends to m.successes and then
discards errors from m.sidecar.MarkDone and migrate.Save; instead capture their
returned errors (from m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) and
migrate.Save(m.sidecar)), append any non-nil errors to a collector on the
migrator (e.g., m.sidecarErrors or m.errors) or log them immediately via the
existing logger, and ensure these collected errors are surfaced in the final
summary/report so users know which projects may be out of sync; do this inside
the same success branch that handles msg.project and msg.res to preserve
context.
In `@internal/cli/root.go`:
- Around line 607-617: The JSON decoding of resp.Body into the local user struct
is ignoring errors; change the json.NewDecoder(resp.Body).Decode(&user) call to
capture its error, handle it (return or print a clear error) and avoid printing
an empty user.Login. Specifically, in the same block that prints "Authenticated
as", check the Decode error returned from
json.NewDecoder(resp.Body).Decode(&user), and on error either return that error
(so root command fails) or set a safe fallback string for user.Login and log the
decode failure before printing token/CreatedAt; ensure you reference the
existing user variable and resp.Body and do not swallow the decode error.
In `@internal/cli/sync.go`:
- Around line 404-416: findWorktreePath currently ignores the branch parameter
and always returns the main project path; update the function (findWorktreePath)
to use the branch argument to resolve branch-specific worktrees by checking the
project's worktree mapping (e.g., ws.Projects[project].Worktrees or similar) and
return the branch worktree path when present, falling back to the main proj.Path
when not; also replace the manual string concatenation that builds mainPath with
filepath.Join(workspace, proj.Path) for portability and consistency so
resolveProjectConflict will open the correct directory for branch worktrees.
In `@internal/config/config.go`:
- Around line 596-616: duplicateBranchIssues currently stores only the first
index in seen and then uses prev+1 as a "count", which is wrong; change the
logic to track both the first occurrence index and the total occurrence count
for each branch (e.g., add a separate counts map[string]int or change seen to
map[string]struct{first int; count int}), set first when you see a name the
first time and initialize count=1, increment count on subsequent occurrences,
and when emitting ValidationIssue use the actual count for "has %d entries" and
the stored first index for the "first at index %d" (use those symbols:
duplicateBranchIssues, seen/first/count, prev/ValidationIssue).
---
Nitpick comments:
In `@internal/add/add.go`:
- Around line 268-277: Remove the unused url parameter from inferGroup and
update all call sites to use the new signature; specifically, change func
inferGroup(_ string, cat config.Category) string to func inferGroup(cat
config.Category) string (or alternatively keep the parameter but add a clear
TODO comment if you intend to use it), and ensure callers that currently pass a
URL argument are adjusted to pass only the category. Also verify buildPath
remains unchanged and still accepts group, cat, name.
In `@internal/add/sources.go`:
- Around line 88-93: The code checks errors.Is(err, github.ErrNotAuthed) but
both branches return the same wrapped error, creating redundant logic; simplify
by removing the special-case branch and replace the whole conditional with a
single return that wraps err (e.g., return nil, fmt.Errorf("github source: %w",
err)), keeping the errors.Is/github.ErrNotAuthed check only if you intend to
handle that case differently in functions dealing with err.
In `@internal/add/tui.go`:
- Around line 1-912: File exceeds size threshold; split large tui.go by
extracting the style constants (addTitle, addDim, addHelp, addCursor, addAccent,
addErr, addCheck, addChip, addGroupHdr, addItemName, addExists, addExistsTag,
addPreviewName, addCursorRow) into a new styles.go in package add (or
alternatively extract view functions like viewBrowse, viewEdit, viewConfirm into
tui_views.go). Create the new file with the same package declaration, move the
var block there, run `go vet`/`go build` to ensure no symbol changes, and keep
all references unchanged (e.g., viewBrowse(), viewEdit(),
renderSelectionPreview(), renderItemLine(), buildBrowseRows()) so callers
continue to compile.
In `@internal/aliasmgr/aliasmgr.go`:
- Around line 1-12: The file is too large (>500 LOC); extract cohesive parts
into separate files: move the tree-building logic (buildTree, treeRow and any
helper functions/types they depend on) into a new file (e.g., tree.go) and move
view rendering (viewManage, viewConfirm and their helpers) into another file
(e.g., view.go), keeping them in the same package aliasmgr; update imports and
adjust function/variable visibility (exported vs unexported) so references
remain valid, and ensure any shared helper types/constants remain in a small
shared file or kept at top of aliasmgr.go to avoid import cycles.
In `@internal/auth/auth.go`:
- Around line 138-167: The HTTP call in requestDeviceCode (and other
auth-related requests) uses http.DefaultClient which has no timeout; create a
package-level http client (e.g., var httpClient = &http.Client{Timeout:
30*time.Second}) and replace http.DefaultClient.Do(...) with httpClient.Do(...)
in requestDeviceCode and any other functions in internal/auth/auth.go that make
HTTP requests so calls can't hang indefinitely; ensure you import time if not
already imported.
In `@internal/cli/bootstrap.go`:
- Line 264: The global variable program (var program *tui.Program) is a shared
mutable pointer used by runBootstrap to send messages from goroutines, which
creates implicit coupling; either document the invariant (that runBootstrap sets
program and clears it within its call and no other goroutines access it outside
that window) or refactor to remove the global by adding a program field to the
bootstrap model and passing that model (or the *tui.Program) into any goroutines
that need to send messages; update runBootstrap to set model.program (or accept
the program as an argument) and remove/clear the package-level program variable
to ensure the coupling is explicit and thread-safe.
- Around line 577-615: This file exceeds the 500-line limit and should be split:
extract the style variables (e.g., bsTitleStyle, bsHeaderStyle, bsDimStyle,
bsHelpStyle, bsCheckStyle, bsWarnStyle, bsErrStyle, bsArrowStyle,
bsBarFilledStyle, bsBarEmptyStyle, errorBannerStyle) and related small view
helpers (e.g., renderProgressBar, indent) into a new file such as
bootstrap_styles.go or styles.go, update package-level visibility if needed, and
ensure any callers in bootstrap.go reference the moved symbols; keep behavior
unchanged and run `go vet`/`go test` to verify no import or visibility
regressions.
In `@internal/cli/worktree.go`:
- Around line 140-144: The code currently swallows errors from
git.SetFetchRefspec, git.FetchRefspec, and git.SetBranchUpstream called in the
worktree flow (see HasFetchRefspec, SetFetchRefspec, FetchRefspec,
SetBranchUpstream), which can hide connectivity/permission failures; update
those calls to capture returned errors and either log them at debug/trace level
(via the existing logger) or return the error upstream when the operation is
required for correctness (e.g., if FetchRefspec fails and subsequent operations
depend on the fetch), ensuring each call checks its error and handles it
appropriately rather than discarding it.
- Around line 121-238: The RunE closure in worktree.go has excessive cyclomatic
complexity; extract logical sections into small helper functions to reduce
decision points: implement handleExistingWorktree(projectName, branch, machine,
barePath, ws, saveWorkspace) to contain the existing-worktree branch
re-registration logic (uses locateWorktreeForBranch, p.ClaimBranch,
p.MarkPushed, saveWorkspace), implement createWorktreeFromExisting(barePath,
wtPath, branch, remoteExists, fromBase) to encapsulate the local/remote attach
flow (uses git.WorktreeAdd, git.SetBranchUpstream) and implement
createWorktreeFromBase(barePath, wtPath, branch, base) to handle creating a new
branch from a base (uses proj.DefaultBranch and git.WorktreeAdd); have RunE call
these helpers and handle updating ws.Projects and printing output so the
closure's decision points drop well below 15.
🪄 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: 8f726a84-cd18-4829-8a41-b7ea35b9b99c
📒 Files selected for processing (150)
AGENTS.mdbench/README.mdbench/benchfixture/fixture.gobench/benchfixture/reconciler_bench_test.gobench/benchfixture/scan_bench_test.gobench/scripts/run-l2.shinternal/add/add.gointernal/add/browse.gointernal/add/clipboard.gointernal/add/clipboard_test.gointernal/add/clone.gointernal/add/dedup.gointernal/add/disk.gointernal/add/edit.gointernal/add/format.gointernal/add/gather.gointernal/add/github_source.gointernal/add/manual.gointernal/add/msg.gointernal/add/options.gointernal/add/register.gointernal/add/sidecar.gointernal/add/sources.gointernal/add/styles.gointernal/add/suggest.gointernal/add/suggestions.gointernal/add/tui.gointernal/agent/chip_action.gointernal/agent/edit_project.gointernal/agent/flash.gointernal/agent/forms.gointernal/agent/header.gointernal/agent/items.gointernal/agent/lang.gointernal/agent/launcher.gointernal/agent/list.gointernal/agent/model.gointernal/agent/persist.gointernal/agent/render.gointernal/agent/sessions.gointernal/agent/source.gointernal/agent/stamp.gointernal/agent/styles.gointernal/agent/tui.gointernal/agent/types.gointernal/agent/view.gointernal/agent/whichkey.gointernal/agent/worktrees.gointernal/alias/alias.gointernal/alias/conflict.gointernal/alias/generate.gointernal/alias/install.gointernal/alias/resolve.gointernal/alias/shell_zsh.gointernal/aliasmgr/aliasmgr.gointernal/aliasmgr/model.gointernal/aliasmgr/step_confirm.gointernal/auth/auth.gointernal/auth/device_flow.gointernal/auth/pat.gointernal/bootstrap/bootstrap.gointernal/bootstrap/sidecar.gointernal/branchprompt/branchprompt.gointernal/branchprompt/messages.gointernal/cli/add.gointernal/cli/alias.gointernal/cli/auth.gointernal/cli/bootstrap.gointernal/cli/bootstrap_model.gointernal/cli/bootstrap_view.gointernal/cli/create.gointernal/cli/daemon.gointernal/cli/docs.gointernal/cli/docs_test.gointernal/cli/doctor.gointernal/cli/explorer.gointernal/cli/favorite.gointernal/cli/migrate.gointernal/cli/migrate_model.gointernal/cli/migrate_tui.gointernal/cli/migrate_view.gointernal/cli/path.gointernal/cli/root.gointernal/cli/scan.gointernal/cli/setup.gointernal/cli/status.gointernal/cli/sync.gointernal/cli/sync_resolve.gointernal/cli/worktree.gointernal/cli/worktree_add.gointernal/cli/worktree_list.gointernal/cli/worktree_push.gointernal/cli/worktree_rm.gointernal/clipboard/clipboard.gointernal/clipboard/clipboard_test.gointernal/clone/clone.gointernal/config/branch.gointernal/config/config.gointernal/config/legacy.gointernal/config/machine.gointernal/config/project.gointernal/config/validate.gointernal/conflict/conflict.gointernal/conflict/notify.gointernal/create/cmd.gointernal/create/create.gointernal/create/gh.gointernal/create/options.gointernal/create/render.gointernal/create/runner.gointernal/create/sidecar.gointernal/create/tui.gointernal/daemon/config.gointernal/daemon/conflicts.gointernal/daemon/daemon.gointernal/daemon/git.gointernal/daemon/ipc.gointernal/daemon/projects.gointernal/daemon/reconciler.gointernal/daemon/socket.gointernal/daemon/toml.gointernal/daemon/watcher.gointernal/docs/agent.gointernal/docs/schema.gointernal/doctor/doctor.gointernal/doctor/format.gointernal/doctor/project.gointernal/doctor/system.gointernal/git/bare.gointernal/git/clone_test.gointernal/git/git.gointernal/git/worktree.gointernal/github/app_provider.gointernal/github/cache.gointernal/github/client.gointernal/github/gh_client.gointernal/github/github.gointernal/github/http_client.gointernal/github/provider.gointernal/github/resolve.gointernal/migrate/check.gointernal/migrate/git_helpers.gointernal/migrate/hooks.gointernal/migrate/migrate.gointernal/migrate/resolve.gointernal/migrate/sidecar.gointernal/setup/setup.gointernal/setup/step_confirm.gointernal/setup/step_group.gointernal/setup/step_select.go
💤 Files with no reviewable changes (77)
- internal/add/options.go
- internal/agent/styles.go
- internal/auth/pat.go
- internal/cli/alias.go
- internal/clipboard/clipboard_test.go
- internal/alias/resolve.go
- internal/add/github_source.go
- internal/agent/whichkey.go
- internal/branchprompt/messages.go
- internal/agent/sessions.go
- internal/add/manual.go
- internal/conflict/notify.go
- internal/agent/persist.go
- internal/clipboard/clipboard.go
- internal/cli/migrate_view.go
- internal/bootstrap/sidecar.go
- internal/alias/install.go
- internal/agent/stamp.go
- internal/clone/clone.go
- internal/agent/types.go
- internal/cli/doctor.go
- internal/agent/source.go
- internal/create/cmd.go
- internal/cli/bootstrap_model.go
- internal/cli/worktree_rm.go
- internal/agent/chip_action.go
- internal/cli/create.go
- internal/alias/shell_zsh.go
- internal/agent/lang.go
- internal/alias/conflict.go
- internal/add/gather.go
- internal/cli/worktree_list.go
- internal/config/machine.go
- internal/alias/generate.go
- internal/agent/worktrees.go
- internal/config/legacy.go
- internal/add/sidecar.go
- internal/agent/launcher.go
- internal/agent/header.go
- internal/cli/auth.go
- internal/config/branch.go
- internal/aliasmgr/model.go
- internal/agent/forms.go
- internal/agent/list.go
- internal/cli/favorite.go
- internal/cli/status.go
- internal/config/validate.go
- internal/add/disk.go
- internal/add/dedup.go
- internal/add/browse.go
- internal/cli/worktree_add.go
- internal/add/msg.go
- internal/cli/migrate_model.go
- internal/cli/migrate_tui.go
- internal/agent/edit_project.go
- internal/add/suggestions.go
- internal/agent/items.go
- internal/add/styles.go
- internal/auth/device_flow.go
- internal/add/clone.go
- internal/cli/setup.go
- internal/cli/add.go
- internal/cli/scan.go
- internal/cli/explorer.go
- internal/add/edit.go
- internal/cli/sync_resolve.go
- internal/cli/worktree_push.go
- internal/cli/daemon.go
- internal/add/clipboard.go
- internal/agent/render.go
- internal/config/project.go
- internal/aliasmgr/step_confirm.go
- internal/cli/path.go
- internal/cli/bootstrap_view.go
- internal/add/format.go
- internal/add/register.go
- internal/agent/flash.go
Comment on lines
+43
to
+52
| func TestSystemClipboardReader_DeadlineExceeded(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) | ||
| defer cancel() | ||
| time.Sleep(10 * time.Millisecond) | ||
|
|
||
| _, err := DefaultClipboardReader.Read(ctx) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify specific error type for deadline exceeded.
The error assertion only checks err != nil without verifying the error type. For consistency with TestSystemClipboardReader_ContextCancelled (which checks for specific error types), this should verify that the error is either context.DeadlineExceeded or ErrClipboardUnavailable. The current weak assertion means the test could pass even if the wrong error is returned, or if the clipboard tool is simply unavailable rather than the deadline being exceeded.
🔍 Proposed fix to verify error type
_, err := DefaultClipboardReader.Read(ctx)
if err == nil {
t.Fatal("expected error")
}
+ if !errors.Is(err, ErrClipboardUnavailable) && !errors.Is(err, context.DeadlineExceeded) {
+ t.Errorf("unexpected error: %v", err)
+ }
}📝 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.
Suggested change
| func TestSystemClipboardReader_DeadlineExceeded(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) | |
| defer cancel() | |
| time.Sleep(10 * time.Millisecond) | |
| _, err := DefaultClipboardReader.Read(ctx) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| } | |
| func TestSystemClipboardReader_DeadlineExceeded(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) | |
| defer cancel() | |
| time.Sleep(10 * time.Millisecond) | |
| _, err := DefaultClipboardReader.Read(ctx) | |
| if err == nil { | |
| t.Fatal("expected error") | |
| } | |
| if !errors.Is(err, ErrClipboardUnavailable) && !errors.Is(err, context.DeadlineExceeded) { | |
| t.Errorf("unexpected error: %v", 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/add/clipboard_test.go` around lines 43 - 52, Update
TestSystemClipboardReader_DeadlineExceeded to assert the specific error type
instead of only checking err != nil: call DefaultClipboardReader.Read(ctx) and
use errors.Is to verify the returned err is either context.DeadlineExceeded or
ErrClipboardUnavailable (matching the pattern used in
TestSystemClipboardReader_ContextCancelled). Reference the test function name
TestSystemClipboardReader_DeadlineExceeded and the Read method on
DefaultClipboardReader; replace the weak t.Fatal("expected error") with an
assertion that uses errors.Is(err, context.DeadlineExceeded) || errors.Is(err,
ErrClipboardUnavailable) and fail the test if neither holds.
Comment on lines
+251
to
+305
| func looksLikeGitURL(s string, allowedHosts map[string]bool) bool { | ||
| s = strings.TrimSpace(s) | ||
|
|
||
| if strings.ContainsAny(s, " \t\n\r") { | ||
| return false | ||
| } | ||
|
|
||
| if m := shorthandRegex.FindStringSubmatch(s); m != nil { | ||
| host := strings.ToLower(m[1]) | ||
|
|
||
| if allowedHosts[host] { | ||
| return true | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| u, err := url.Parse(s) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| switch strings.ToLower(u.Scheme) { | ||
| case "https", "http", "ssh", "git": | ||
|
|
||
| default: | ||
| return false | ||
| } | ||
| if u.Host == "" { | ||
| return false | ||
| } | ||
|
|
||
| host := strings.ToLower(u.Host) | ||
|
|
||
| if i := strings.LastIndex(host, ":"); i >= 0 { | ||
| host = host[:i] | ||
| } | ||
|
|
||
| pathTrimmed := strings.TrimSuffix(u.Path, "/") | ||
|
|
||
| if strings.HasSuffix(pathTrimmed, ".git") { | ||
| return true | ||
| } | ||
|
|
||
| if allowedHosts[host] { | ||
| if ownerRepoPath.MatchString(pathTrimmed+"/") || ownerRepoPath.MatchString(pathTrimmed) { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| if ownerRepoPath.MatchString(pathTrimmed) || ownerRepoPath.MatchString(pathTrimmed+"/") { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Inconsistent host validation for SSH shorthand URLs.
At lines 258-266, SSH shorthand URLs (user@host:owner/repo) are accepted regardless of whether the host is in allowedHosts. The check at line 261 is performed but line 265 returns true unconditionally. This differs from HTTPS URLs where unknown hosts require a .git suffix or matching path pattern.
If accepting any SSH shorthand is intentional (since SSH access implies user trust), consider adding a brief // DECISION: comment. Otherwise, this may accept clipboard content from untrusted hosts.
🛡️ Suggested fix if strict host checking is desired
if m := shorthandRegex.FindStringSubmatch(s); m != nil {
host := strings.ToLower(m[1])
-
- if allowedHosts[host] {
- return true
- }
-
- return true
+ return allowedHosts[host]
}📝 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.
Suggested change
| func looksLikeGitURL(s string, allowedHosts map[string]bool) bool { | |
| s = strings.TrimSpace(s) | |
| if strings.ContainsAny(s, " \t\n\r") { | |
| return false | |
| } | |
| if m := shorthandRegex.FindStringSubmatch(s); m != nil { | |
| host := strings.ToLower(m[1]) | |
| if allowedHosts[host] { | |
| return true | |
| } | |
| return true | |
| } | |
| u, err := url.Parse(s) | |
| if err != nil { | |
| return false | |
| } | |
| switch strings.ToLower(u.Scheme) { | |
| case "https", "http", "ssh", "git": | |
| default: | |
| return false | |
| } | |
| if u.Host == "" { | |
| return false | |
| } | |
| host := strings.ToLower(u.Host) | |
| if i := strings.LastIndex(host, ":"); i >= 0 { | |
| host = host[:i] | |
| } | |
| pathTrimmed := strings.TrimSuffix(u.Path, "/") | |
| if strings.HasSuffix(pathTrimmed, ".git") { | |
| return true | |
| } | |
| if allowedHosts[host] { | |
| if ownerRepoPath.MatchString(pathTrimmed+"/") || ownerRepoPath.MatchString(pathTrimmed) { | |
| return true | |
| } | |
| return false | |
| } | |
| if ownerRepoPath.MatchString(pathTrimmed) || ownerRepoPath.MatchString(pathTrimmed+"/") { | |
| return true | |
| } | |
| return false | |
| } | |
| func looksLikeGitURL(s string, allowedHosts map[string]bool) bool { | |
| s = strings.TrimSpace(s) | |
| if strings.ContainsAny(s, " \t\n\r") { | |
| return false | |
| } | |
| if m := shorthandRegex.FindStringSubmatch(s); m != nil { | |
| host := strings.ToLower(m[1]) | |
| return allowedHosts[host] | |
| } | |
| u, err := url.Parse(s) | |
| if err != nil { | |
| return false | |
| } | |
| switch strings.ToLower(u.Scheme) { | |
| case "https", "http", "ssh", "git": | |
| default: | |
| return false | |
| } | |
| if u.Host == "" { | |
| return false | |
| } | |
| host := strings.ToLower(u.Host) | |
| if i := strings.LastIndex(host, ":"); i >= 0 { | |
| host = host[:i] | |
| } | |
| pathTrimmed := strings.TrimSuffix(u.Path, "/") | |
| if strings.HasSuffix(pathTrimmed, ".git") { | |
| return true | |
| } | |
| if allowedHosts[host] { | |
| if ownerRepoPath.MatchString(pathTrimmed+"/") || ownerRepoPath.MatchString(pathTrimmed) { | |
| return true | |
| } | |
| return false | |
| } | |
| if ownerRepoPath.MatchString(pathTrimmed) || ownerRepoPath.MatchString(pathTrimmed+"/") { | |
| return true | |
| } | |
| 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/add/sources.go` around lines 251 - 305, The shorthand SSH branch in
looksLikeGitURL currently accepts any SSH shorthand because the shorthandRegex
block returns true unconditionally; update that block so it only returns true
when the extracted host is allowed (i.e., keep the existing if
allowedHosts[host] { return true } and remove the unconditional return true), or
if accepting all SSH shorthands is intentional add a short DECISION comment
above the shorthandRegex handling describing that choice and why; ensure you
modify only the shorthandRegex branch (the code that extracts host via m :=
shorthandRegex.FindStringSubmatch(s)) and leave the HTTPS/URL logic unchanged.
Comment on lines
1
to
19
| package agent | ||
|
|
||
| import ( | ||
| "bufio" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "sort" | ||
| "strings" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "github.com/kuchmenko/workspace/internal/config" | ||
| "github.com/kuchmenko/workspace/internal/git" | ||
| "github.com/kuchmenko/workspace/internal/layout" | ||
| "github.com/kuchmenko/workspace/internal/tui" | ||
| ) |
There was a problem hiding this comment.
File exceeds 800 LOC limit and must be split immediately.
This file is 1,717 lines, well beyond the 800-line threshold. Consider extracting into separate files by concern:
worktree.go:Worktree,WorktreeResult,WorktreeCache,CreateWorktree,DeleteWorktreeWithRegistry,LoadWorktrees(lines 541-753)session.go:Session,SessionCache,LoadSessions,FindSession,extractTitle(lines 755-941)launch.go:LaunchClaude,LaunchShell(lines 1685-1717)edit.go: Edit project UI handlers (lines 1126-1368)
As per coding guidelines: "Split .go files larger than 500 lines on the next touch, and extract immediately if beyond 800 lines"
🤖 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 1 - 19, The file internal/agent/tui.go is
far too large (1,717 lines) and must be split; refactor by extracting related
types and functions into separate files: move Worktree, WorktreeResult,
WorktreeCache, CreateWorktree, DeleteWorktreeWithRegistry, and LoadWorktrees
into a new worktree.go; move Session, SessionCache, LoadSessions, FindSession,
and extractTitle into session.go; move LaunchClaude and LaunchShell into
launch.go; move the Edit project UI handlers (the functions handling edit UI
logic referenced around lines where edit functions appear) into edit.go; ensure
you update package-level imports and any unexported helper references (keep
names and signatures unchanged) and run go vet/go build to fix any missing
references.
Comment on lines
+865
to
+867
| func encodeCwd(path string) string { | ||
| return strings.ReplaceAll(path, "/", "-") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Claude uses slash-to-hyphen encoding for project directories
if [ -d "$HOME/.claude/projects" ]; then
echo "Claude projects directory contents:"
ls -la "$HOME/.claude/projects" | head -20
echo ""
echo "Sample directory names that might reveal encoding scheme:"
ls "$HOME/.claude/projects" 2>/dev/null | head -5
fiRepository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# First, check file size and read the relevant sections
wc -l internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 90
🏁 Script executed:
# Read around line 865-867 (encodeCwd function)
sed -n '860,875p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 366
🏁 Script executed:
# Read around line 903 to see the decoding logic
sed -n '898,910p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 299
🏁 Script executed:
# Search for all encodeCwd and decodeCwd usages to understand the full context
rg -n "encodeCwd|decodeCwd" internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 134
🏁 Script executed:
# Read around line 770 where encodeCwd is used
sed -n '765,780p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 313
🏁 Script executed:
# Search for decodeCwd function (might exist but not be used)
rg -n "decodeCwd|func decode" internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# Look at the broader FindSession function context
sed -n '880,920p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 762
🏁 Script executed:
# Search for pathLookup usage after line 770
sed -n '770,850p' internal/agent/tui.go | grep -n "pathLookup"Repository: kuchmenko/workspace
Length of output: 137
🏁 Script executed:
# Check the SaveSession function to see if it uses encodeCwd
rg -n "SaveSession" internal/agent/tui.go -A 30Repository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# Find where sessions are created or written to disk
rg -n "Session{" internal/agent/tui.go -B 3 -A 3Repository: kuchmenko/workspace
Length of output: 143
🏁 Script executed:
# Look for the full context around pathLookup usage (lines 770-800)
sed -n '770,800p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 656
🏁 Script executed:
# Find where directories are created in claudeRoot
rg -n "mkdir|MkdirAll" internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# Look for where session directories are written
rg -n "claudeRoot\|claudeProjectsDir" internal/agent/tui.go -A 2 | head -40Repository: kuchmenko/workspace
Length of output: 45
🏁 Script executed:
# Get more context on how encoded paths get written to disk
sed -n '750,800p' internal/agent/tui.goRepository: kuchmenko/workspace
Length of output: 997
Replace lossy path encoding with reversible encoding.
The encoding strings.ReplaceAll(path, "/", "-") is not reversible for paths containing hyphens. Paths /foo/bar and /foo-bar both encode to foo-bar, causing:
- Session directories to collide in
~/.claude/projects - FindSession to return incorrect sessions when decoding
- LoadSessions to fail lookups for hyphenated paths
Use base64 or percent-encoding instead:
Proposed fix
+import (
+ "encoding/base64"
+)
+
func encodeCwd(path string) string {
- return strings.ReplaceAll(path, "/", "-")
+ return base64.URLEncoding.EncodeToString([]byte(path))
}
+
+func decodeCwd(encoded string) (string, error) {
+ b, err := base64.URLEncoding.DecodeString(encoded)
+ if err != nil {
+ return "", err
+ }
+ return string(b), nil
+}Update FindSession (line 903):
- cwd := strings.ReplaceAll(entry.Name(), "-", "/")
+ cwd, err := decodeCwd(entry.Name())
+ if err != nil {
+ continue
+ }🤖 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 865 - 867, The current encodeCwd function
uses strings.ReplaceAll(path, "/", "-"), which is lossy; replace it with a
reversible encoding (e.g., base64 RawURLEncoding or url.PathEscape) so encoded
names are unique and reversible. Update encodeCwd to return a reversible
encoding (suggest base64.RawURLEncoding.EncodeToString([]byte(path)) or
url.PathEscape(path)), and update all places that decode or parse these
names—specifically FindSession and LoadSessions—to decode using the matching
decoder (base64.RawURLEncoding.DecodeString or url.PathUnescape) and use the
decoded path for comparisons and session directory lookup so hyphenated paths no
longer collide. Ensure session directory naming and any lookup logic
consistently use the new encoder/decoder pair.
Comment on lines
+488
to
+503
| func formatInt(n int) string { | ||
| if n == 0 { | ||
| return "0" | ||
| } | ||
| if n < 0 { | ||
| n = -n | ||
| } | ||
| var buf [20]byte | ||
| i := len(buf) | ||
| for n > 0 { | ||
| i-- | ||
| buf[i] = byte('0' + n%10) | ||
| n /= 10 | ||
| } | ||
| return string(buf[i:]) | ||
| } |
There was a problem hiding this comment.
formatInt silently discards the sign of negative numbers.
If a negative value is ever passed, this returns the absolute value without a minus sign, which could cause subtle display bugs.
Proposed fix
func formatInt(n int) string {
if n == 0 {
return "0"
}
+ neg := n < 0
if n < 0 {
n = -n
}
var buf [20]byte
i := len(buf)
for n > 0 {
i--
buf[i] = byte('0' + n%10)
n /= 10
}
+ if neg {
+ i--
+ buf[i] = '-'
+ }
return string(buf[i:])
}Alternatively, if negative values are never expected, consider using uint or adding a panic/assertion for defensive clarity.
📝 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.
Suggested change
| func formatInt(n int) string { | |
| if n == 0 { | |
| return "0" | |
| } | |
| if n < 0 { | |
| n = -n | |
| } | |
| var buf [20]byte | |
| i := len(buf) | |
| for n > 0 { | |
| i-- | |
| buf[i] = byte('0' + n%10) | |
| n /= 10 | |
| } | |
| return string(buf[i:]) | |
| } | |
| func formatInt(n int) string { | |
| if n == 0 { | |
| return "0" | |
| } | |
| neg := n < 0 | |
| if n < 0 { | |
| n = -n | |
| } | |
| var buf [20]byte | |
| i := len(buf) | |
| for n > 0 { | |
| i-- | |
| buf[i] = byte('0' + n%10) | |
| n /= 10 | |
| } | |
| if neg { | |
| i-- | |
| buf[i] = '-' | |
| } | |
| return string(buf[i:]) | |
| } |
🤖 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/view.go` around lines 488 - 503, formatInt currently discards
the sign for negative inputs; update formatInt to preserve and emit a leading
'-' for negative n by recording sign (e.g., neg := n < 0), converting n to its
absolute value for digit extraction, and then prefixing '-' to the returned
string when neg is true; alternatively, if negatives are truly invalid, change
the API to take a unsigned type or add an assertion/panic in formatInt to fail
fast—but the quickest fix is to detect negative n and include the minus sign in
the returned string from formatInt.
Comment on lines
+397
to
+400
| if msg.res != nil { | ||
| _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) | ||
| _ = bootstrap.Save(m.sidecar) | ||
| } |
There was a problem hiding this comment.
Ignored errors from MarkDone and Save could silently lose progress.
Both MarkDone and Save errors are discarded. If the sidecar file cannot be written (e.g., disk full, permissions), the user loses crash-safe resume capability without any indication.
🛡️ Proposed fix to log or accumulate warnings
if msg.res != nil {
- _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch)
- _ = bootstrap.Save(m.sidecar)
+ if err := m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch); err != nil {
+ // Log or accumulate warning; sidecar is best-effort
+ }
+ if err := bootstrap.Save(m.sidecar); err != nil {
+ // Log or accumulate warning
+ }
}🤖 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.go` around lines 397 - 400, The code currently
discards errors from m.sidecar.MarkDone(...) and bootstrap.Save(m.sidecar);
capture both return values and handle non-nil errors instead of ignoring them:
call m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) and if it returns
err log a warning or error (using the existing logger on the manager struct,
e.g. m.logger or m.log) with context including msg.project and
msg.res.DefaultBranch; then call bootstrap.Save(m.sidecar) and likewise log any
error returned (or accumulate/return the error if the calling flow supports it)
so disk/permission failures are visible and do not silently lose resume state.
Comment on lines
+528
to
+534
| } else { | ||
| m.successes = append(m.successes, msg.project) | ||
| if msg.res != nil { | ||
| _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) | ||
| _ = migrate.Save(m.sidecar) | ||
| } | ||
| } |
There was a problem hiding this comment.
Silently ignoring sidecar save errors could lose migration state.
If MarkDone or Save fails, the user won't know the sidecar is out of sync. On TUI crash or unexpected exit, resume won't accurately reflect which projects completed.
Consider accumulating these errors to display in the summary, or at minimum logging them:
} else {
m.successes = append(m.successes, msg.project)
if msg.res != nil {
- _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch)
- _ = migrate.Save(m.sidecar)
+ if err := m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch); err != nil {
+ m.errors = append(m.errors, migrateError{project: msg.project + " (sidecar)", err: err})
+ } else if err := migrate.Save(m.sidecar); err != nil {
+ m.errors = append(m.errors, migrateError{project: "<sidecar>", err: err})
+ }
}
}📝 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.
Suggested change
| } else { | |
| m.successes = append(m.successes, msg.project) | |
| if msg.res != nil { | |
| _ = m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) | |
| _ = migrate.Save(m.sidecar) | |
| } | |
| } | |
| } else { | |
| m.successes = append(m.successes, msg.project) | |
| 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 + " (sidecar)", err: err}) | |
| } else if err := migrate.Save(m.sidecar); err != nil { | |
| m.errors = append(m.errors, migrateError{project: "<sidecar>", 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.go` around lines 528 - 534, The code currently appends
to m.successes and then discards errors from m.sidecar.MarkDone and
migrate.Save; instead capture their returned errors (from
m.sidecar.MarkDone(msg.project, msg.res.DefaultBranch) and
migrate.Save(m.sidecar)), append any non-nil errors to a collector on the
migrator (e.g., m.sidecarErrors or m.errors) or log them immediately via the
existing logger, and ensure these collected errors are surfaced in the final
summary/report so users know which projects may be out of sync; do this inside
the same success branch that handles msg.project and msg.res to preserve
context.
Comment on lines
+607
to
+617
| var user struct { | ||
| Login string `json:"login"` | ||
| } | ||
| json.NewDecoder(resp.Body).Decode(&user) | ||
|
|
||
| path, _ := auth.TokenPath() | ||
| fmt.Printf(" Authenticated as: %s\n", user.Login) | ||
| fmt.Printf(" Token: %s\n", path) | ||
| fmt.Printf(" Scopes: %s\n", token.Scope) | ||
| fmt.Printf(" Created: %s\n", token.CreatedAt.Format("2006-01-02 15:04")) | ||
| return nil |
There was a problem hiding this comment.
JSON decode error ignored could produce confusing output.
If Decode fails, user.Login will be empty, resulting in output like "Authenticated as: " with no username.
Suggested fix
- json.NewDecoder(resp.Body).Decode(&user)
-
- path, _ := auth.TokenPath()
- fmt.Printf(" Authenticated as: %s\n", user.Login)
+ if err := json.NewDecoder(resp.Body).Decode(&user); err != nil || user.Login == "" {
+ fmt.Println(" Token valid but could not read username from API response.")
+ } else {
+ fmt.Printf(" Authenticated as: %s\n", user.Login)
+ }
+ path, _ := auth.TokenPath()🤖 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 607 - 617, The JSON decoding of resp.Body
into the local user struct is ignoring errors; change the
json.NewDecoder(resp.Body).Decode(&user) call to capture its error, handle it
(return or print a clear error) and avoid printing an empty user.Login.
Specifically, in the same block that prints "Authenticated as", check the Decode
error returned from json.NewDecoder(resp.Body).Decode(&user), and on error
either return that error (so root command fails) or set a safe fallback string
for user.Login and log the decode failure before printing token/CreatedAt;
ensure you reference the existing user variable and resp.Body and do not swallow
the decode error.
Comment on lines
+404
to
+416
| func findWorktreePath(workspace, project, branch string) (string, error) { | ||
| if ws == nil { | ||
| return "", fmt.Errorf("workspace not loaded") | ||
| } | ||
| proj, ok := ws.Projects[project] | ||
| if !ok { | ||
| return "", fmt.Errorf("project %s not in workspace.toml", project) | ||
| } | ||
| mainPath := workspace + string(os.PathSeparator) + proj.Path | ||
|
|
||
| _ = branch | ||
| return mainPath, nil | ||
| } |
There was a problem hiding this comment.
Bug: branch parameter is ignored, breaking branch worktree resolution.
findWorktreePath accepts a branch parameter but explicitly discards it on line 414. The function always returns mainPath, so resolveProjectConflict cannot resolve divergence conflicts on branch worktrees—it will open a shell in the wrong directory.
Additionally, line 412 uses string concatenation for path construction instead of filepath.Join, which is inconsistent with line 250 and less portable.
🐛 Proposed fix to use branch for worktree lookup
func findWorktreePath(workspace, project, branch string) (string, error) {
if ws == nil {
return "", fmt.Errorf("workspace not loaded")
}
proj, ok := ws.Projects[project]
if !ok {
return "", fmt.Errorf("project %s not in workspace.toml", project)
}
- mainPath := workspace + string(os.PathSeparator) + proj.Path
-
- _ = branch
- return mainPath, nil
+ mainPath := filepath.Join(workspace, proj.Path)
+ if branch == "" || branch == "main" || branch == "master" {
+ return mainPath, nil
+ }
+ barePath := layout.BarePath(mainPath)
+ wtPath := locateWorktreeForBranch(barePath, branch)
+ if wtPath == "" {
+ return "", fmt.Errorf("no worktree found for branch %s", branch)
+ }
+ return wtPath, 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/sync.go` around lines 404 - 416, findWorktreePath currently
ignores the branch parameter and always returns the main project path; update
the function (findWorktreePath) to use the branch argument to resolve
branch-specific worktrees by checking the project's worktree mapping (e.g.,
ws.Projects[project].Worktrees or similar) and return the branch worktree path
when present, falling back to the main proj.Path when not; also replace the
manual string concatenation that builds mainPath with filepath.Join(workspace,
proj.Path) for portability and consistency so resolveProjectConflict will open
the correct directory for branch worktrees.
Comment on lines
+596
to
+616
| func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue { | ||
| seen := make(map[string]int, len(branches)) | ||
| var out []ValidationIssue | ||
| for _, b := range branches { | ||
| if b.Name == "" { | ||
| continue | ||
| } | ||
| prev, isDup := seen[b.Name] | ||
| if !isDup { | ||
| seen[b.Name] = len(seen) | ||
| continue | ||
| } | ||
| 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), | ||
| }) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Incorrect count in validation detail message.
The prev+1 value represents the 1-indexed position of the first occurrence, not the total count of duplicate entries. When a duplicate is found, the message "branch %q has %d entries" will incorrectly report the position as a count.
For example, with branches ["beta", "alpha", "gamma", "alpha"], the message for "alpha" would say "has 2 entries" (because prev=1, so prev+1=2), but this happens to be coincidentally correct only because the first occurrence was at index 1.
Proposed fix
func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue {
- seen := make(map[string]int, len(branches))
+ seen := make(map[string]int, len(branches)) // stores first-seen index
+ count := make(map[string]int, len(branches)) // stores occurrence count
var out []ValidationIssue
for _, b := range branches {
if b.Name == "" {
continue
}
- prev, isDup := seen[b.Name]
- if !isDup {
- seen[b.Name] = len(seen)
+ firstIdx, wasSeen := seen[b.Name]
+ count[b.Name]++
+ if !wasSeen {
+ seen[b.Name] = len(seen)
continue
}
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 appears %d times (first at index %d)", b.Name, count[b.Name], firstIdx),
})
}
return out
}📝 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.
Suggested change
| func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue { | |
| seen := make(map[string]int, len(branches)) | |
| var out []ValidationIssue | |
| for _, b := range branches { | |
| if b.Name == "" { | |
| continue | |
| } | |
| prev, isDup := seen[b.Name] | |
| if !isDup { | |
| seen[b.Name] = len(seen) | |
| continue | |
| } | |
| 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), | |
| }) | |
| } | |
| return out | |
| } | |
| func duplicateBranchIssues(projName string, branches []BranchMeta) []ValidationIssue { | |
| seen := make(map[string]int, len(branches)) | |
| count := make(map[string]int, len(branches)) | |
| var out []ValidationIssue | |
| for _, b := range branches { | |
| if b.Name == "" { | |
| continue | |
| } | |
| firstIdx, wasSeen := seen[b.Name] | |
| count[b.Name]++ | |
| if !wasSeen { | |
| seen[b.Name] = len(seen) | |
| continue | |
| } | |
| out = append(out, ValidationIssue{ | |
| Kind: ValidationDuplicateBranch, | |
| Project: projName, | |
| Branch: b.Name, | |
| Detail: fmt.Sprintf("branch %q appears %d times (first at index %d)", b.Name, count[b.Name], firstIdx), | |
| }) | |
| } | |
| 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/config.go` around lines 596 - 616, duplicateBranchIssues
currently stores only the first index in seen and then uses prev+1 as a "count",
which is wrong; change the logic to track both the first occurrence index and
the total occurrence count for each branch (e.g., add a separate counts
map[string]int or change seen to map[string]struct{first int; count int}), set
first when you see a name the first time and initialize count=1, increment count
on subsequent occurrences, and when emitting ValidationIssue use the actual
count for "has %d entries" and the stored first index for the "first at index
%d" (use those symbols: duplicateBranchIssues, seen/first/count,
prev/ValidationIssue).
Criterion: a file earns its size if it reads top-to-bottom as one narrative. Not by LOC. Big is fine when there's a through-line; small is wrong when the next thing you need is in another file. By that yardstick most of the 150 files were small *and* wrong — split per-view-mode, per-syscall, per-subcommand. Mechanical consolidation, no behavior change. Results: 150 -> 45 production .go files (-70%) 25 -> 21 folders under internal/ 20,468 -> 19,592 LOC (dedup'd imports) binary size unchanged (14 MB) zero charmbracelet imports outside internal/tui/ (PR #49 seam intact) Major package collapses: cli 28 -> 5 (root + bootstrap + migrate + sync + worktree + docs) agent 19 -> 3 (model -> view -> tui) add 18 -> 4 (add -> sources -> suggest -> tui) daemon 10 -> 3 (daemon -> reconciler -> ipc) create 8 -> 2 (create -> tui) Ten small packages each collapse to one file at internal/<pkg>/<pkg>.go: alias, aliasmgr, auth, bootstrap, config, doctor, git, github, migrate, setup. Four folders deleted: clipboard folds into add, clone folds into git, benchfixture moves to bench/, docs folds into cli. Two intentional outliers: cli/root.go (1,727 LOC) and agent/tui.go (1,774 LOC) both walk top-to-bottom — root.go is 13 declarative Cobra builders in a row; agent/tui.go is state -> update -> panels. Splitting either would cut a sentence in half. Queued, not in this PR: bubbletea eviction (drops internal/tui/ 10 -> ~3) and a few cli-only folds (aliasmgr, setup, doctor, create, bootstrap, migrate could fold into cli). Both deferred so this PR stays a pure mechanical shrink.
kuchmenko
force-pushed
the
refactor/file-count-shrink
branch
from
9b07a22 to
234cf5d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Criterion: a file earns its size if it reads top-to-bottom as one narrative. Big is fine when there's a through-line; small is wrong when the next thing you need is in another file.
By that yardstick most of the 150 files were small and wrong — split per-view-mode, per-syscall, per-subcommand. Mechanical consolidation, no behavior change: 150 → 45 production .go files (−70%), 25 → 21 folders, LOC unchanged (just dedup'd imports), 14 MB binary unchanged, PR #49 TUI seam intact.
The heavy collapses:
cli28 → 5,agent19 → 3,add18 → 4,daemon10 → 3,create8 → 2. Ten small packages each collapse to one file atinternal/<pkg>/<pkg>.go. Four folders deleted (clipboard→add,clone→git,benchfixture→bench/,docs→cli).Two intentional outliers:
cli/root.go(1,727 LOC) andagent/tui.go(1,774 LOC). Both walk top-to-bottom —root.gois 13 declarative Cobra builders in a row,agent/tui.gois state → update → panels. Splitting either would cut a sentence in half.go build,go test,go vet,golangci-lint,gofmtall clean. Smoke remaining:ws agent,ws alias,ws add,ws status,ws syncon a real workspace.