feat: add pr diff and checkout (task 0014) #14

Merged
alexion merged 1 commits from task-0014-pr-diff-and-checkout into main 2026-07-14 07:02:40 -04:00
Owner

Implements .claude/tasks/0014-pr-diff-and-checkout.md.

Summary

Adds the two PR commands that touch content and the local worktree:

  • pr diff <n> fetches the raw diff from the .diff endpoint (forcing a text response — the JSON-defaulting client would otherwise discard the plain-text body), truncating at 4000 chars with separate truncated/original_length fields and a prepended --full suggestion; --full returns the raw diff.
  • pr checkout <n> reads the PR head branch from the PR fetch and fetches refs/pull/{n}/head from origin (uniform for same-repo and fork PRs, per ADR 0011), three-cased on local branch state so re-checkout is idempotent; divergent local commits fail with GIT_ERROR rather than being discarded.

Introduces the GIT_ERROR mapping (runGit) carrying git's first stderr line plus a remediation help line, extends the test fixture server with a raw (non-JSON) response body, and widens the PR 404 classifier so a .diff path still resolves to PR_NOT_FOUND.

Deviations

  • The .diff fetch forces { format: "text" } because giteaApi sets baseApiParams.format: "json", which would otherwise null out the plain-text body.
  • For the checked-out-and-diverged case, git's merge --ff-only prints hint: lines before the fatal: line, so the surfaced GIT_ERROR message is that first hint: line; the plain-language divergence explanation lives in the help lines (matching the spec's literal "git's first stderr line").
  • Process: /implement front-loaded the implementation before the test-writer sub-agent authored tests, so TDD cycles were green-on-first-run; each test was still authored independently by a sub-agent from the public CLI interface alone, one behavior at a time.

Review

Risk

Overall: Medium

  • Blast radius — Low: additive new subcommands plus one new module; the only shared edits are a widened regex in errors.ts and an optional raw field in the test fixture server, neither altering existing callers.
  • Reversibility — Low: pure additions with no migrations, deletions, or published schema changes.
  • Test coverage — Low: both commands are covered end-to-end (truncation boundaries, 404, fork PRs, divergence/idempotency, git-failure paths) via real scratch git repos and a fixture server.
  • Sensitive domain — Medium: pr checkout mutates the working tree via git and force-updates local branches, but only branches it defines as PR-head mirrors, and refuses to discard divergent local commits (--ff-only).
  • Size & complexity — Medium: ~380 lines with genuinely tangled control flow in checkoutPullHead (three-way branch-state casing plus fast-forward error handling).
  • Runtime criticality — Low: developer-facing CLI tooling, not a production hot path.

Standards — unaddressed findings

  • Primitive Obsession / Data Clumps (judgement call, declined): checkoutPullHead threads a bare number + branch and builds the pull/{n}/head refspecs by hand. Left as-is — the "PR head ref" concept is local and used once, so a dedicated type would be speculative; the reviewer explicitly would not act on it.

(The Duplicated-Code finding — the merge --ff-only path re-implementing runGit's failure mapping — was fixed in this diff: runGit gained an optional fallback message and the merge routes through it.)

Spec

No unaddressed findings — the Spec axis reported the change faithfully implements the task, spec, and ADR 0011 with no defects.

Implements `.claude/tasks/0014-pr-diff-and-checkout.md`. ## Summary Adds the two PR commands that touch content and the local worktree: - **`pr diff <n>`** fetches the raw diff from the `.diff` endpoint (forcing a text response — the JSON-defaulting client would otherwise discard the plain-text body), truncating at 4000 chars with separate `truncated`/`original_length` fields and a prepended `--full` suggestion; `--full` returns the raw diff. - **`pr checkout <n>`** reads the PR head branch from the PR fetch and fetches `refs/pull/{n}/head` from origin (uniform for same-repo and fork PRs, per ADR 0011), three-cased on local branch state so re-checkout is idempotent; divergent local commits fail with `GIT_ERROR` rather than being discarded. Introduces the `GIT_ERROR` mapping (`runGit`) carrying git's first stderr line plus a remediation help line, extends the test fixture server with a `raw` (non-JSON) response body, and widens the PR 404 classifier so a `.diff` path still resolves to `PR_NOT_FOUND`. ### Deviations - The `.diff` fetch forces `{ format: "text" }` because `giteaApi` sets `baseApiParams.format: "json"`, which would otherwise null out the plain-text body. - For the checked-out-and-diverged case, git's `merge --ff-only` prints `hint:` lines before the `fatal:` line, so the surfaced `GIT_ERROR` message is that first `hint:` line; the plain-language divergence explanation lives in the help lines (matching the spec's literal "git's first stderr line"). - Process: `/implement` front-loaded the implementation before the test-writer sub-agent authored tests, so TDD cycles were green-on-first-run; each test was still authored independently by a sub-agent from the public CLI interface alone, one behavior at a time. ## Review ### Risk **Overall: Medium** - Blast radius — Low: additive new subcommands plus one new module; the only shared edits are a widened regex in `errors.ts` and an optional `raw` field in the test fixture server, neither altering existing callers. - Reversibility — Low: pure additions with no migrations, deletions, or published schema changes. - Test coverage — Low: both commands are covered end-to-end (truncation boundaries, 404, fork PRs, divergence/idempotency, git-failure paths) via real scratch git repos and a fixture server. - Sensitive domain — Medium: `pr checkout` mutates the working tree via git and force-updates local branches, but only branches it defines as PR-head mirrors, and refuses to discard divergent local commits (`--ff-only`). - Size & complexity — Medium: ~380 lines with genuinely tangled control flow in `checkoutPullHead` (three-way branch-state casing plus fast-forward error handling). - Runtime criticality — Low: developer-facing CLI tooling, not a production hot path. ### Standards — unaddressed findings - **Primitive Obsession / Data Clumps (judgement call, declined):** `checkoutPullHead` threads a bare `number` + `branch` and builds the `pull/{n}/head` refspecs by hand. Left as-is — the "PR head ref" concept is local and used once, so a dedicated type would be speculative; the reviewer explicitly would not act on it. _(The Duplicated-Code finding — the `merge --ff-only` path re-implementing `runGit`'s failure mapping — was fixed in this diff: `runGit` gained an optional fallback message and the merge routes through it.)_ ### Spec No unaddressed findings — the Spec axis reported the change faithfully implements the task, spec, and ADR 0011 with no defects.
alexion added 1 commit 2026-07-13 22:15:39 -04:00
feat: add pr diff and checkout (task 0014)
All checks were successful
CI / test (pull_request) Successful in 42s
3db6d421d4
Add the two PR commands that touch content and the local worktree:

- `pr diff <n>` fetches the raw diff from the `.diff` endpoint (forcing a
  text response, which the JSON-defaulting client would otherwise discard),
  truncates at 4000 chars with separate `truncated`/`original_length` fields
  and a prepended `--full` suggestion; `--full` returns the raw diff.
- `pr checkout <n>` reads the PR head branch from the PR fetch and fetches
  `refs/pull/{n}/head` from origin (uniform for same-repo and fork PRs, ADR
  0011), three-cased on local branch state so re-checkout is idempotent and
  divergent local commits fail with `GIT_ERROR` rather than being discarded.

Introduces the `GIT_ERROR` mapping (`runGit`) carrying git's first stderr
line plus a remediation help line, and widens the PR 404 classifier so a
`.diff` path still resolves to `PR_NOT_FOUND`.
alexion merged commit d034fce3ea into main 2026-07-14 07:02:40 -04:00
Sign in to join this conversation.
No Reviewers
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: alexion/gitea-axi#14