Skip to content

refactor: extract review-file CLI logic out of github.go + audit for cwd-resolution bugs #425

@tomasz-tomczyk

Description

@tomasz-tomczyk

Context

PR #424 fixed a bulk-routing bug in `crit comment --json --reply-to`: with multiple daemons/review files in play, the bulk write would silently land in the cwd-resolved review instead of the file that actually contains the comment IDs. The fix extends the existing singular-reply fallback (scan `~/.crit/reviews/` by ID) to the bulk path.

The fix landed in `github.go` because that's where the existing helpers live — but `github.go` is supposed to be the GitHub PR sync layer (`gh pr` / push / pull). Comment authoring, bulk writes, review-file resolution, and ID-based file lookup have nothing to do with GitHub. They've accreted there because that's where `crit comment` was originally bolted on.

Refactor

Move CLI comment authoring + review-file I/O into their own files. Suggested split:

  • `comment_cli.go` — `runComment*`, `addCommentToCritJSON*`, `addReplyToCritJSON`, `bulkAddCommentsToCritJSONScoped`, `processBulkEntry`, `appendComment*`, `appendReply`, `BulkCommentEntry`
  • `review_file.go` — `resolveReviewPath`, `resolveReviewPathFromDaemon`, `pickReviewPath`, `loadCritJSON`, `saveCritJSON`, `writeCritJSON`, `clearCritJSON`, `findReviewFileByCommentID`, `reviewFileContainsComment`, `cjContainsCommentID`
  • `github.go` — keep only `gh`-CLI integration (`crit pull` / `crit push` / GitHub ID sync)

Tests should split alongside.

Audit

While in the area, audit other CLI subcommands for the same class of bug — silently operating on the cwd-resolved review file when the user's intent was a different review:

  • `crit fetch` — does it correctly target the review the comments were originally written against?
  • `crit pull` / `crit push` — same question, especially when multiple daemons exist for the same cwd
  • `crit unpublish` — picks the right review's `delete_token`?
  • `crit cleanup` — anything that could remove the wrong file?
  • Anything else that calls `resolveReviewPath` without an alt-lookup fallback

For each: either confirm the cwd resolution is correct for the operation, or extend the same ID-based / token-based fallback that `addReplyToCritJSON` and `bulkAddCommentsToCritJSONScoped` now use.

Why

`github.go` is now ~1700 lines and mixes three unrelated concerns. Fixing #424 made that obvious — the new bulk-routing logic doesn't belong next to GitHub PR REST helpers. And if we missed the same cwd-vs-intent class of bug in other commands, users will hit the same "silently wrote to the wrong review" surprise.

🤖 Generated with Claude Code

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions