Skip to content

Read a pull request's diff from T3 and split its patch into files - #65

Merged
krishhgg merged 13 commits into
masterfrom
t3term/pull-request-diff-read
Oct 18, 2026
Merged

krishhgg merged 13 commits into
masterfrom
t3term/pull-request-diff-read

Conversation

@krishhgg

@krishhgg krishhgg commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Adds pull_request::diff, which reads a pull request's diff from T3, and pull_request::patch, which splits the patch into files, hunks and numbered lines. Nothing in the TUI calls them yet. The next pull request, stacked on this one, adds the Code tab to a link's details.

  • diff POSTs to /api/pull-requests/diff with the session's token, as the desktop does. T3 sends the diff over HTTP, not RPC, because it is often the largest answer. It takes a cursor for the next slice and reads at most 16 MiB of the answer, a chunk at a time, with or without a length up front.
  • Each failure the route encodes reads as its kind. A host without credentials and a pull request the host can't find read as Unavailable and NotFound, as they do over RPC. A refused token or scope, T3's own failure, another status, a redirect and no answer read as the new PullRequestError::Http with a sentence of t3term's.
  • Diff keeps the patch, truncated, next_cursor and the host's counts for files it left out.
  • Patch::read reads git's format and T3's own patch for a page of files: added, deleted, renamed and binary files, quoted paths with octal escapes, paths with spaces, a file with a header and no hunks, \ No newline at end of file, Windows line ends and a plain unified diff. Each line's text is a range of the patch, so nothing is copied. A broken or short hunk marks its file unreadable, and the rest still reads.
  • The test server in src/http.rs now reads and records request bodies, so a test can check what t3term sent.
  • docs/pull-request-reads.md gets a section on the diff, its bounds and its failures.

Stacked on #64.

Decision log

What I noticed. The next step for PR reviews is the desktop's Code tab, which shows a pull request's changed files and their diff. t3term had no way to read a diff: src/pull_request.rs reads only over RPC, and T3 serves the diff only over HTTP.

How I confirmed it. These files in the pinned T3 source define the read:

  • packages/contracts/src/environmentHttp.ts:650-663 defines the route and its five errors.
  • packages/contracts/src/pullRequest.ts:945-988 defines PullRequestDiffInput and PullRequestDiffResult, and says a slice holds whole files.
  • apps/server/src/pullRequest/http.ts checks orchestration:read, which a saved login carries.
  • GitHubPullRequestCli.ts:2206-2259 reads GitHub's whole diff up to 8 MiB and falls back to 100 files a slice.
  • gitHubPullRequestJson.ts:3219-3266 writes T3's own headers for that fallback.

Options.

  • Chosen: an HTTP read beside the RPC ones, through the Api the client already holds, with the RPC error classes reused for the two pull request failures.
  • Chosen: a hand-written parser that keeps ranges into the patch. A 16 MiB patch would otherwise be copied line by line.
  • Rejected: a diff-parsing dependency. I didn't check whether one reads T3's header-only files and keeps ranges rather than copies, and git's format is small enough to read in one file.
  • Rejected: reading through Api::request. It reads the whole body with no bound and turns every error status into its own error, so the route's failures couldn't be told apart.
  • Rejected: skipping binary patch data by state. Its lines start with a letter and have no space, so no header test matches them. A mutation test showed the skip changed nothing.

What review changed. Greptile found two gaps in the parser, both fixed in f70828f. First, a patch that ended inside a hunk read as whole, and a line past one side's count joined the hunk. Now a line joins only while each side it is on has lines left, and a short hunk, an overfull one or a hunk-like line after a file's last hunk marks the file unreadable. Second, hunk headers didn't count against MOST_LINES, so a patch of headers alone could keep a record for each. Now each kept hunk counts as a line.

The merges of #64 brought #63's steady live read test (15b07f8), master at 47e50cd (95b00ab) and the stack recipes (42f59c1, c48f756). Two conflicted. In e0691b9 and 95b00ab, README.md had changes on both sides, and each keeps both. In 95b00ab, master's rejected and this branch's RpcError::failed were added at the same place in src/rpc.rs, and both stay. The diff from #64 is still the same 8 files.

GPT-6.1 Sol's first verdict on c48f756 was FAIL for one thing. hunk_header took any u64 start and count, and each line then moved the number on unchecked. @@ -18446744073709551615 +1 @@ with one context line panicked in a debug build and wrapped to 0 in a release one, without marking the file. In e98add4 a range whose start plus count passes u64::MAX isn't a header, so the file is marked unreadable as for other broken headers. A range that ends at the highest number still reads. The new cases, on either side, panic at patch.rs:343 against the old parser. #62 to #64 have since landed, so this pull request now targets master. 3995c30 merges master at f043b63, which brings #48's environment switcher. Only README.md's unit test sentence conflicted, and the merge keeps both sides.

Known limits.

  • Nothing calls diff until the next pull request.
  • It doesn't read one commit's diff (commit), a file's whole contents (pullRequests.diffFileContents) or viewed files.
  • It doesn't ask for compression, as T3 runs on the same machine.
  • A path git quoted with bytes that aren't UTF-8 shows U+FFFD for them.

Tests

  • The patch tests read hunks with every line numbered and a blank context line, each file status, binary files, quoted and spaced paths, a path past 4,096 bytes, Windows line ends, header-like lines inside a hunk, broken and short hunks, headers whose lines would be numbered past u64::MAX on either side, T3's header-only files, a plain unified diff and the file and line bounds.
  • The diff tests run diff against a fake T3 on 127.0.0.1. They check the exact request with and without a cursor, each failure the route encodes, an error that isn't JSON, a redirect, no answer, and an answer one byte past its bound with and without its length up front. Unit tests read a full answer, a last slice, counts T3's schema allows but GitHub never sends, more omitted files than the bound and each broken field.
  • 24 mutations each fail a test, among them dropping the redirect check, the bound on either side of the length, the cursor, the second --- rule, the octal base and the tab before a path's end.
  • On 3995c30, fmt, clippy and all 797 tests pass (716 lib, 2 main, 31 CLI, 48 RPC). Nothing in the TUI calls the new reads, and smoke passes 12 of 12 recipes on its build. e98add4 changes only the parser.

🤖 Generated with Claude Code

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge, with the earlier caller fix supplied by PR #66.

Summary

Adds pull_request::diff to read bounded diff slices over HTTP and Patch::read to split patches into files, hunks and numbered lines.

  • Pull-request diffs arrive in bounded slices that callers can read one after another.
  • Pull-request patches split into files, hunks, and numbered lines.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["Reference and optional cursor"] --> B["POST /api/pull-requests/diff"]
  B --> C["Read up to 16 MiB"]
  C --> D["Diff: patch and next cursor"]
  D --> E["Patch::read"]
  E --> F["Files, hunks and numbered lines"]
Loading

Reviews (7) · Last reviewed commit: "Merge master at f043b63, which adds the ..." · Reviewed by Greptile

krishhgg and others added 6 commits October 15, 2026 01:38
…nto files

pull_request::diff posts to /api/pull-requests/diff, as the desktop does, and
reads at most 16 MiB of the answer. Each failure the route encodes reads as its
kind. pull_request::patch splits the unified diff into files, hunks and
numbered lines, within bounds on files, lines and path length. Nothing in the
TUI calls either yet.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…eir prefix

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nly files

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…into t3term/pull-request-diff-read

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… into t3term/pull-request-diff-read

# Conflicts:
#	README.md
@krishhgg
krishhgg marked this pull request as ready for review October 15, 2026 08:56
Comment thread src/pull_request/patch.rs
Comment thread src/pull_request/patch.rs Outdated
…e line bound

Greptile on #65: a patch that ended inside a hunk read as whole, a line past
one side's count was taken into the hunk, and a patch of hunk headers alone
could keep a hunk for every header past MOST_LINES. Now a line joins a hunk
only while each side it is on has lines left, the end of the patch inside a
hunk and a hunk-like line after a file's last hunk mark the file unreadable,
and each kept hunk counts as a line of the bound.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
krishhgg added a commit that referenced this pull request Oct 15, 2026
After #65's parser fix a file past the line bound keeps no hunk, and a file
whose only hunk header is broken never had one. Both read "No lines
changed." Now they show the cut or unreadable note instead, with a test
for each.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread src/pull_request/patch.rs
krishhgg and others added 3 commits October 17, 2026 14:02
…e50cd

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…page recipe

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n the page recipe

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@krishhgg

Copy link
Copy Markdown
Owner Author

FAIL for head c48f756975403a8cffb31e2e7e51ee4675dc4891, patch id aef82541250a8a61b6a741d059f4fca4d1bb6fdd.
Verifier: GPT-6.1 Sol (xhigh), not the author. Base 5ff1c6cb555d7c53b310ade01b09310a92017c55. Scenario links and every feature recipe's scenario, size and truecolor setting.

Claim Base Head
Nothing a user sees changes All 11 branch recipes pass All 11 branch recipes pass; settled saved-stack captures and complete request maps match the base
Diff HTTP read matches T3's contract and bounded failure handling Reader absent Pinned route, payload, result and errors match; fake HTTP tests pass
Patch parser preserves its bounds and treats malformed hunks safely Parser absent Existing tests pass, but maximum line numbers panic or wrap

Finding, P2: src/pull_request/patch.rs:343, also line 347. hunk_header accepts any u64 start and count, but hunk_line increments old_at and new_at without checking overflow. diff --git a/a b/a\n@@ -18446744073709551615 +1 @@\n same\n panics on the first context line with overflow checks. The equivalent new-side header panics at line 347. With overflow checks off, a two-line hunk starting at 18446744073709551615 returns old line numbers [18446744073709551615, 0] and unreadable: false. This is untrusted patch text and contradicts safe malformed-hunk handling. Reject an overflowing range or use checked increments and mark it unreadable, with regression coverage for both sides. Fixes remain with the author.

The request matches the pinned diff route and input/result schemas. I also checked the scope handler, GitHub fallback and T3-generated patch headers. The declared errors use 503, 502, 401, 403 and 500, and the reader recognizes them. Optional commit reads remain an explicit known limit.

Tests: all 27 pull_request:: library tests pass. The six isolated parser tests pass unchanged. In a detached scratch worktree, reverting only the non-test parser to its pre-f70828f implementation makes the retained short/overfull-hunk test and hunk-budget test fail. Three independent mutations to line numbering, the hunk budget and file budget each fail existing tests. The new overflow probes reproduce on the unmodified head. Evidence is in parser-experiments.json, parser-fix-reverted.log, parser-probe.log and parser-release-probe.log under /tmp/t3v-verdict/65/round1/.

Regression sweep: all 11 recipes pass using each side's own checkout. The additional current-master sweep passes 10 of 11 on both sides. Its older page recipe uses 200x60; the stack rows inherited from #63 and #64 put the conversation below that pane. Both branches already contain the updated 200x69 recipe, and it passes. This is an inherited map mismatch, not a regression introduced by the library layer. Connection, environments, appearance, sidebar, lineage, linked PRs, page, live refresh, thread details, transcript and CLI were covered. The source map and request comparisons are in claims.md, 65-own-comparison.json and 65-master-comparison.json.
Rules and requests: doctor and the before/after-stop check calls pass. The complete request maps match for the same completed branch recipes. The old master linked-PR recipe can replace links while its host read is still running, so its final stack/subscription count varies on both builds. A paired replay that waits for r refresh before leaving the details has identical maps. The master appearance captures also raced the conversation answer; waiting for completion gives byte-identical dark-again.txt captures and identical maps. Run addresses, environment ids, clocks and spinner phases account for the other capture differences. A separate paired 100x30 saved-stack drive has byte-identical before-open, saved, host, first-failure and recovered captures, with identical complete maps. See settled-linked-comparison.json, settled-appearance-comparison.json, base-100x30 and head-100x30.
CI: all checks on the pinned head passed, including macOS and E2E smoke. Greptile: review passed; all 3 earlier threads are resolved and answered; no new finding on this head. The earlier Code-tab caller finding is resolved in the caller PR, as Greptile acknowledged, and no caller exists in this layer.

Binary identity, read with otool -l:

Commit LC_UUID Build
5ff1c6cb555d7c53b310ade01b09310a92017c55 679C0FC5-6B78-304A-9BBD-C3DD11B311B6 Base build confirms Compiling t3term
c48f756975403a8cffb31e2e7e51ee4675dc4891 2C224490-F736-32B1-B9E3-760CDCC05D5A Head build confirms Compiling t3term

Tools and the additional master recipes came from origin/master at abf402b3dd9d5bf5b94837df40081e234c0bbc0f. All evidence is under /tmp/t3v-verdict/65/round1/. No PR code, branch or remote ref was changed.

@krishhgg
krishhgg changed the base branch from t3term/pull-request-saved-stack to master October 18, 2026 07:51
krishhgg and others added 2 commits October 18, 2026 00:53
A hunk header took any u64 start and count, and each line then moved
the number on unchecked, so a header such as -18446744073709551615 with
one line panicked in a debug build and wrapped to 0 in a release one.
A range whose start plus count overflows now isn't a header, which
marks the file unreadable as other broken headers do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
README.md's unit test sentence conflicted, and the merge keeps both sides.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@krishhgg

Copy link
Copy Markdown
Owner Author

PASS for head 3995c30ca58e0cb11c2963116edf97ec0e0770c7, patch id eb8b2cd4dbfea3bfb4bf881562e3db95bf00655e.
Verifier: GPT-6.1 Sol (xhigh), not the author. Base f043b63983f6d5944146c4a12bf00517a309128b. Scenarios links, lineage and terminal, recipe sizes, truecolor.

Claim Base or negative control Head
Nothing a user sees changes Master's recipes and captured screens Same recipes, settled screens and request counts
Hunk ranges cannot overflow on either side Reverting e98add4 reproduces the panic on both sides, twice from fresh parser state 61 independent boundary inputs, each run twice, pass with no panic; includes counts of 0, 1, 2, 3 and u64::MAX, valid upper boundaries and a following file
The regression test detects the fix With only e98add4's non-test parser change reverted, one test runs and fails at patch.rs:343 The same test passes in the full head suite
The bounded HTTP read and patch parser meet their contracts These APIs are absent on master Fake HTTP tests and parser tests pass

Regression sweep: 12 of 12 recipes pass on master and on head. The 69 paired recipe captures match after accounting for run paths and loopback ports, spinner/elapsed time and one transient feed-reconnect frame. The settled paired recovery capture matches exactly apart from the run path and port. Immediate-stop linked-list tallies vary by one capability resubscription; settling the same actions gives identical RPC counts on both sides, as detailed below. The affected-feature map is claims-and-map.md. The verify-t3term tree is unchanged from master. The merge retains the environment-switcher and diff-reader coverage in README.md.

Rules and requests: check passed before and after stop. The paired settled drive captured sidebar, thread details, links, Summary, Timeline, capability changes and malformed-frame recovery. Its seven captures match after replacing only the run's path and loopback port. Both sides sent two each of orchestration.subscribeShell, subscribeServerConfig, orchestration.subscribeThread and pullRequests.subscribeRefreshes, and one each of pullRequests.detail, pullRequests.activity and pullRequests.stack. A deliberately absent screen expectation failed as required.

Local checks: fmt, clippy and all 797 tests pass. CI: macOS, Linux and E2E smoke are green on the pinned head. Greptile: the head check passed, all three earlier threads are resolved and answered, and there are no new head findings.

Binary driven LC_UUID
f043b63983f6d5944146c4a12bf00517a309128b 8A620DF7-4925-3BA5-A529-B9C4DF956662
3995c30ca58e0cb11c2963116edf97ec0e0770c7 889ACCF0-996A-3EDC-97FD-BEC7EE0CDC90

Both builds logged Compiling t3term. Evidence: /tmp/t3v-verdict/65/round2/, including smoke-base-isolated/, smoke-head/, sweep-comparison.json, parser-probe-head.log, parser-probe-old.log, reverted-test.log, unchanged-base-2/, unchanged-head-2/ and unchanged-comparison.json. Run paths are retained in the probe evidence and smoke logs. Every run was stopped with t3v and checked after stop.
Notes: no findings. The overflow in #66's new renderer is reported on #66, where that code is introduced.

@krishhgg
krishhgg merged commit 83126f5 into master Oct 18, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant