refactor: bundled cleanup — wrappers, mustGetwd, browser.go, error surfacing#428
Merged
tomasz-tomczyk merged 1 commit intomainfrom May 2, 2026
Merged
refactor: bundled cleanup — wrappers, mustGetwd, browser.go, error surfacing#428tomasz-tomczyk merged 1 commit intomainfrom
tomasz-tomczyk merged 1 commit intomainfrom
Conversation
…rfacing
- Delete 7 unscoped wrapper functions in github.go (per .claude/rules/go.md)
that were one-line delegates to *Scoped variants with inheritedScope{}.
Update test callers to call Scoped versions directly.
- Add mustGetwd() helper; replace 7 ignored-error os.Getwd() sites in
main.go and share.go that silently kept "" as cwd on failure.
- Extract browser.go from main.go (browser_test.go already existed in the
package — pure code move, no test changes).
- Distinguish ENOENT from parse/IO errors:
- share.go loadExistingShareCfg now returns (CritJSON, bool, error);
callers surface parse errors with diagnostics instead of treating
corrupt files as missing.
- focus_cli.go loadCritJSONForOutputDir logs parse/IO errors loudly
to stderr rather than swallowing them.
- Inline pluralReply at its single call site; delete the helper and its
test.
Builds clean, vet clean, gofmt clean, tests pass (go test ./... — ok 35s).
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (59.37%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 67.38% 67.26% -0.12%
==========================================
Files 26 27 +1
Lines 9747 9748 +1
==========================================
- Hits 6568 6557 -11
- Misses 2663 2675 +12
Partials 516 516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tomasz-tomczyk
added a commit
that referenced
this pull request
May 2, 2026
- review_file.go: document gh-CLI repo-scoping assumption that keeps cross-repo branch-name collisions from being a real-world hazard. - main.go: extend runPull comment to note that a corrupt cwd file also produces empty cj.Branch and triggers branch-based redirect. - .golangci.yml: update stale gocyclo exclusion to reference the *Scoped function name (post-#428 wrapper deletion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tomasz-tomczyk
added a commit
that referenced
this pull request
May 2, 2026
* refactor: split github.go into github / comment_cli / review_file github.go had grown to ~1774 lines mixing three unrelated concerns: gh-CLI / PR push-pull, the comment-authoring CLI, and review-file I/O. PR #424's bulk-routing fallback made the entanglement obvious. Move whole functions, no behavior changes, no signature changes: - review_file.go: resolveReviewPath, resolveReviewPathFromDaemon, pickReviewPath, resolveReviewPathFromSessions, writeCritJSON, loadCritJSON, saveCritJSON, clearCritJSON, findReviewFileByCommentID, reviewFileContainsComment, cjContainsCommentID - comment_cli.go: appendComment[Scoped], readAnchorFromDisk, appendReply, addCommentToCritJSON[Scoped], addReplyToCritJSON, BulkCommentEntry (incl. UnmarshalJSON), processBulkEntry and helpers, parseLineSpec, bulkAddCommentsToCritJSON[Scoped], resolveBulkTarget, addReviewCommentToCritJSON[Scoped], addFileCommentToCritJSON[Scoped], appendReviewComment[Scoped], appendFileComment[Scoped] - github.go: keeps only the gh-CLI / PR sync surface (detectPR, fetchPRComments, mergeGHComments*, createGHReview, push/pull plumbing, truncateStr). Tests stay in github_test.go for this commit — they all live in package main and continue to compile and pass against the moved functions. Splitting the 2700-line test file along the same lines is mechanical follow-up work and not required for the refactor to be useful. go build ./... and go test ./... both pass. * fix: route crit pull --pr to review file matching the PR's branch When a user runs `crit pull <pr>` from a worktree whose cwd-resolved review file is for a different branch than the named PR, comments were silently merged into the wrong review file — same class of cwd-vs-intent mismatch that PR #424 fixed for `crit comment`. The trigger is narrow: only kicks in when the user passed an explicit PR number, didn't override --output, and the existing review file's "branch" field disagrees with the PR's headRefName. Under those conditions we scan ~/.crit/reviews/ for exactly one review file matching the PR's branch and route to it, with a stderr note. If zero or multiple match, we fall back to the cwd-resolved path (today's behavior). Adds findReviewFileByBranch helper to review_file.go alongside findReviewFileByCommentID and a redirectReviewPathForPR shim in main.go that wires the fetch+match together. * fix: route crit push --pr to review file matching the PR's branch Mirrors the previous commit for `crit pull`. When a user runs `crit push <pr>` from a worktree whose cwd-resolved review file is for a different branch than the named PR, we'd silently post the wrong comments to that PR — strictly worse than the pull case because pushing creates side effects on GitHub that are visible to reviewers. Trigger conditions are identical to the pull fix: explicit PR number, no --output override, existing review file's branch disagrees with the PR's headRefName, and exactly one alt review file matches the PR's branch. Reuses redirectReviewPathForPR. * fix: log Note when multiple review files match PR branch in redirect When a multi-worktree user runs crit pull/push with --pr and several review files share the PR's head branch, findReviewFileByBranch returned a generic error and redirectReviewPathForPR swallowed it — leaving the user with the cwd file and no signal of why. Distinguish "no match" (silent fallback) from "ambiguous" (stderr Note) via sentinel errors. The Note tells the user to pass --output to disambiguate. * fix: reroute crit pull/push --pr when cwd review file missing Previously the gate in runPull/runPush required cj.Branch != "" before attempting to redirect to a branch-matching review file. When the cwd review file didn't exist, cj.Branch was empty and the redirect never fired — the user got a fresh wrong-location file silently. For runPull: drop the cj.Branch != "" gate; pass empty cwdBranch through to redirectReviewPathForPR, which now skips its cwd-vs-PR-branch early-out when cwdBranch is empty (no false-positive risk without cwd state). For runPush: tolerate ENOENT on the initial review file read so an explicit --pr from a clean checkout can find the right file by branch via the redirect. Only fail with "no review file found" if the redirect also misses. * test: cover findReviewFileByBranch + redirectReviewPathForPR Table-driven tests for both helpers using the existing fetchPRByNumberFn test seam (via withFetchPRByNumber) and a temp HOME for isolated ~/.crit/reviews/ fixtures. Covers all redirect branches: cwd-matches-PR, unique-alt, no-alt, ambiguous-alt (asserts the new stderr Note), fetch error, empty HeadRefName, and empty cwdBranch (Fix 1: redirect must fire even when the cwd review file is missing). Also covers the four findReviewFileByBranch outcomes (single, none, ambiguous, exclude). * docs: address second-pass review notes - review_file.go: document gh-CLI repo-scoping assumption that keeps cross-repo branch-name collisions from being a real-world hazard. - main.go: extend runPull comment to note that a corrupt cwd file also produces empty cj.Branch and triggers branch-based redirect. - .golangci.yml: update stale gocyclo exclusion to reference the *Scoped function name (post-#428 wrapper deletion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Bundled refactor PR addressing four small cleanup opportunities surfaced by an audit (separate from #425, which covers the comment-CLI / review-file extraction).
github.gothat were one-line delegates to*Scopedvariants withinheritedScope{}. Removed:appendComment,appendReviewComment,appendFileComment,addCommentToCritJSON,addReviewCommentToCritJSON,addFileCommentToCritJSON,bulkAddCommentsToCritJSON. No production callers; tests updated to call the*Scopedversions directly. Aligns with.claude/rules/go.mdwhich forbids same-signature wrappers.mustGetwd()helper and replace 7 ignored-erroros.Getwd()sites inmain.go(×6) andshare.go(×1). Previously these silently kept""as cwd on failure, leading to bizarre downstream paths in the unlikely deleted-cwd scenario.browser.gofrommain.go.browser_test.goalready existed in the package — pure code move, no test changes. Resolves the smell of a test file with no corresponding source file.share.goloadExistingShareCfgnow returns(CritJSON, bool, error). ENOENT →(zero, false, nil); parse/IO → wrapped error so the caller can surface a diagnostic. Single production caller updated.focus_cli.goloadCritJSONForOutputDirlogs parse/IO errors loudly to stderr instead of swallowing them as missing-file. Signature unchanged because the underlying loader synthesizes a fresh struct on ENOENT.pluralReplyat its single call site; delete the helper and its test.Review
Test plan
🤖 Generated with Claude Code