feat(mention-review): Phase 0 + Phase 1 of the PR-review TUI, plus a third gate tier - #1698
Merged
Merged
Conversation
ZacxDev
force-pushed
the
feat/mention-review-phase1
branch
from
September 15, 2026 03:39
1b52498 to
21e5683
Compare
…third gate tier
Implements `claudedocs/proposal-pr-review-tui.md` Phases 0 and 1. Read-only, and
`nvim-octo` is untouched — `REVIEW_EXE` still spawns it, so the click path is
unchanged and the rollback is one line that does not exist yet.
PHASE 0 — three kill criteria, none tripped
1. THE QUERY. One `issueOrPullRequest` GraphQL read returns every field the
four panels need plus `viewer { login }`. MEASURED in Go against the public
`innovation-upstream/devrc` with a counting RoundTripper: 1 round trip,
median 663 ms on a 23-file PR, 536 ms small, 402 ms on an issue. No field
was ABSENT; the only null was `reviewDecision`, a legitimate state rendered
as the WORD `NONE`. ⚠ `files(first:100)` caps at one page — surfaced as
`TRUNCATED`, never a silently short list.
2. AUTH. go-gh v2.16.0 (floor v2.12.1 for CVE-2024-53859 / CVE-2025-48938).
OK / NO TOKEN / TOKEN REJECTED all distinguishable, measured as a triple.
§10.1's rung 4 reproduced exactly: `("", "default")` with NO error.
3. THE RENDERER. `viewport.View()` with `SoftWrap=false` + `SetContentLines`
is FLAT in buffer size — 318 µs @1k, 347 µs @4k (the p99), 342 µs @10k.
The SoftWrap control proves the benchmark reads the buffer: 2.45 ms / 8.46
ms / 21.55 ms, i.e. 63x slower at 10k and past a 60 fps budget on its own.
bubbletea#1724's 300-800 µs band is consistent with what was measured and
is ~2% of the frame budget. No kill.
PHASE 1 — what shipped
argv 64/65/66 ported from `nvim-octo.sh` case for case · one GraphQL fetch ·
four panels with focus cycling · a GENERATED footer (`bubbles/help` over the
same `key.Binding` values the dispatcher matches on) · API-only unified diff
via go-gitdiff · the issue card · gruvbox pinned two-way against the Alacritty
config · `o`/`q` · nix packaging with the version read OUT OF the Go source.
The command seam (§3.3) is in from day one: `Step(msg) (App, []Intent)` is
pure, intents are data, and one ~40-line runner turns them into `tea.Cmd`s.
That is what makes "zero network intents" a mechanical assertion. No golden
frames anywhere — §5.0's four reasons all hold under v2.
🔴 THE PHASE-1 SCROLL RE-MEASUREMENT FOUND A REAL DEFECT PHASE 0 COULD NOT SEE
The first implementation styled the WHOLE buffer on every cursor move while
carrying a comment claiming it styled only the visible window — §6.2 hazard 2,
with a comment the code contradicted. On the real four-panel screen:
diff lines CPU/frame 60fps budget after the fix
200 4.1 ms 24.6 % 3.4 ms
4000 20.5 ms 123.1 % MISSES 3.5 ms
10000 46.1 ms 276.3 % MISSES 3.1 ms
Phase 0's bare viewport was flat across the same sizes because it styled
nothing — the defect lived in the SEAM. `TestScrollingTheRealPanel...` is a
genuine regression test: RED at the pre-fix implementation, GREEN at HEAD, and
it guards the 60 fps budget itself rather than a number copied off a pass.
A THIRD GATE TIER (§5.6), wired rather than mentioned
`scripts/run-go-tests.sh` + `gate.sh --tier go` + `checks.gotests` + `pkgs.go`
in `gateTools`. Same guard shape as the other two runners, plus two of its
own: exit 3 on a missing toolchain (never a pass) and a SKIP budget of 0.
`--tier both` is now an ALIAS for `all` — a caller who typed it before Go
existed wanted the whole gate.
`doCheck = false` on the deploy derivation is a safety property: verified in
the pinned nixpkgs, `module.nix` defaults it TRUE, so a red Go test would fail
a `home-manager switch` — the skipped-host failure mode.
Three defects the floors caught, each of which would otherwise have printed
`RESULT: PASS` over zero tests: a JSON field-order grep that matched nothing
on go 1.25; a second that matched on 1.25 and NOT on the sandbox's 1.26; and
four packages failing to build for want of a C compiler while the runner
printed not one word of why.
VERIFICATION
· Go: 119 tests, 0 skips — GREEN on BOTH tiers (dev host go 1.25.14; nix
sandbox go 1.26.7 via `nix build .#checks.x86_64-linux.gotests`).
· Mutation sweep, 13 mutants + a no-op control: 13 KILLED BY THEIR OWN GUARD'S
ERROR, control SURVIVED. Two rounds — the first found a guard that could not
see its own mutant (every rejection test compared against the CONSTANT
`ExitBadRepo`, so moving it moved the expectation too) and a colour-only
encoding in the diff pane that nothing covered. Both closed.
· Packaging: version mechanism mutation-tested — 0 matches -> NULL,
2 matches -> NULL, control -> 0.1.0. Packaged binary verified: `--version`
equals the store-path label, exit codes 64/65/66 hold.
· gate.sh's new tier validated in both directions: FAIL->1, SCOPED->91,
silent->90, real runner->PASS 0.
· 499 + 142 + 80 + 45 existing pytest tests pass against the flake/gate/tools
changes. `test_no_real_launchers.py`'s PATH-clobber ledger caught the new
test's `PATH` replacement and now carries its justification.
NOT DONE, deliberately: local git, caching, write actions, syntax highlighting
(§6.3 — lazygit does none either), and the click-path flip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad
…test — plus the go tier's two missing registrations, and delete a provably-vacuous ledger
Four review findings from running this PR through `the-algorithm`, in the order
they were found. The fourth is the only one that was a real defect.
1. CI-RED, ONE LINE. `scripts/run-go-tests.sh` declares the
`DEVRC_GIT_REPO_POINTERS` array but was never added to
`test_git_repo_isolation.py`'s ledger, so `POINTER_CLEARERS` disagreed with
the files that actually declare it:
declares it but NOT pinned: ['scripts/run-go-tests.sh']
Registered in `RUNNERS`, not in `POINTER_CLEARERS` directly — it resolves a
ROOT with `git rev-parse --show-toplevel` and already places its `unset`
before that block, so the ordering pin (`COMMIT_SH`/`CLAIM_WORK` are held out
of `RUNNERS` precisely because it can assert nothing about them) validates it
for free.
🔴 AND REGISTERING IT SURFACED A SECOND, REAL GAP. Two more arms of the
ledger then went red: `run-go-tests.sh` spelled
`DEVRC_GITENV_CONTROL_VARS=(A B)` on ONE LINE, and the pins parse the
multi-line form the other four runners use. An unparseable declaration is an
UNCHECKED one — the CONTROL ledger and the duplicate check could say nothing
about this runner at all. Reformatted to match its siblings, so all 120 tests
in that file now actually cover it.
The prose counts moved with it: `RUNNERS` is five, and the SPELLINGS count in
both `test_git_repo_isolation.py` and `testlib/gitenv.py` was already stale by
one (it said FIVE/SIX when `claim-work.sh` made it six) and is now SEVEN.
2. THE GO TIER WAS INVISIBLE TO THE DEADMAN. `main-green-check.sh` — the
4-hourly reproduce-before-alerting guard that exists because branch
protection is off — looped `for tier in pytests nodetests`. A break in
`internal/ui` on `main` would never have been looked at by it. The tier list
is pinned two-way, so: the loop, the behavioural pin (`_calls(world)`), the
static pin (the regex over the source), and the three call-COUNT assertions
that encode "2 tiers x 2 attempts" (now 3 x 2 = 6, and +3 for a moved memo /
`--force`). `nix/home.nix`'s `TimeoutStartSec` comment said two tiers too.
3. DELETE THE WRITE-INTENT LEDGER (-184 lines net).
`LedgerViolations(KnownIntents())` was a PROVABLE CONSTANT `nil`: all three
registered intents return `Write() == false`, and `Confirmed`/`NotConfirmed`
are empty map literals. The test's own log said so —
`checked 3 registered intents, 0 of them writes`.
⚠ THIS IS A PHASING JUDGEMENT, NOT A DEFECT REMOVAL, AND IT WAS APPROVED AS
ONE. The emptiness was documented deliberately as Phase-2 scaffolding. The
argument for deleting it anyway: the Intent SEAM must exist from day one
because retrofitting it means rewriting every handler — the LEDGER over that
seam is purely additive and can be added in Phase 2 with no handler touched.
🔴 PHASE 2 MUST REINTRODUCE IT, with the positive control the deleted version
carried (a test-local write intent in neither set, reported with the guard's
own error string). Both surviving comments say so.
Gone: `Confirmed`, `NotConfirmed`, `LedgerViolations`, `Write()` from the
interface and its three implementations, the five ledger tests, and
`countWrites`/`containsString`. `IntentTypeName` went with them — exported,
with ZERO call sites anywhere in the module.
🔴 KEPT: `TestEveryRegisteredIntentIsHandledByRun` and
`TestEveryIntentStepCanEmitIsRegistered`. Neither was ever vacuous — the
dispatch walk emits 2 distinct intents and logs which. The test-local intent
survives as `unregisteredIntent`, still the positive control for `Run`'s
panic. While in there: `run.go` cited a `run_test.go` that has never existed;
the guarantee is real and lives in `intents_test.go`, so the citation now
names the test rather than an absent file.
4. 🔴 THE REAL DEFECT — FIVE ACTIONS HAD NO BEHAVIOURAL TEST AT ALL.
PageUp, PageDown, Top, Bottom and NextFile were never pressed. `keys_test.go`
walks `Dispatch()` and compares it to `FullHelp()`/`ShortHelp()` — both built
from the SAME literal — so it asserts the keymap agrees with ITSELF and never
looks at where the cursor lands.
MEASURED at 4a6c6f8: inverting all four arms of `moveIn`'s switch
(`-= page`/`+= page`, `= 0`/`= n-1` swapped pairwise) left ALL FIVE Go
packages GREEN under `-count=1`. The suite is not uniformly weak — mutating
`udiff.go:309`'s `h.LineIndex > from` to `>= from` kills three tests across
two packages — the hole was exactly these five actions.
`movement_test.go` presses them through `Step` and asserts literal cursor
positions. Re-applying the same 4-arm inversion now fails three tests with
nine assertions, each arm killed by its own:
one ctrl+d from the top -> diffCur = 0, want 10
ctrl+u at the top of the buffer -> diffCur = 10, want 0
ctrl+d on the last line -> diffCur = 191, want 201
g from line 100 -> diffCur = 201, want 0
G from line 100 -> diffCur = 0, want 201
`FileStart(f+1)` -> `FileStart(f)` kills the NextFile test on its own.
🔴 NO EXPECTATION IS DERIVED FROM `moveIn`. The test PINS the viewport height
(`a.vp.SetHeight(20)`), so "half a page" is 10 because the test made the page
20 — the help text's own words (`C-u` "half page up") are the contract. The
boundary cases carry the inversions: at the top of the buffer a correct
`ctrl+u` is INERT, while an arm that added would move ten lines the wrong way
and still look like a page. Fixture sizes are pinned as literals first, so a
fixture that changed size cannot quietly move every expectation with it.
⚠ THE SECOND MOVEMENT SWITCH IS COVERED TOO. `move()` has a separate switch
for the Overview panel's body viewport (`HalfPageUp`/`HalfPageDown`/
`GotoTop`/`GotoBottom`) that no mutation run had touched.
`TestTheOverviewBodyPagesAndJumpsToItsEnds` presses the same five keys there;
inverting that switch fails it with six assertions.
FLOORS DID NOT MOVE. `internal/ui` is 54 tests against its pinned floor of 51
(five ledger tests out, five movement tests in), and `run-go-tests.sh` printed
no replacement number. `RESULT: PASS`, `TOTAL: pass=119 fail=0 skip=0
(global floor 111)`, `SCOPE: FULL`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad
…nd adding them BLINDED the ledger to the two it was built to catch
🔴 NOT IN THE REVIEW BRIEF — found by running the authoritative `nix build`
sandbox tier, which is the only tier that had ever run this PR's full pytest
suite. `tekton/devrc-pytests` reported `failed=2` and its 140-char description
named only one of them.
THE DEFECT. `scripts/tests/test_gate_reexec.py::_env_vars_gate_sh_reads()`
derives every environment variable gate.sh reads, and a two-way pin asserts the
ambient-scrub tuple equals it. It harvests indirect reads out of the refusal
loop with a regex anchored on `^…for … in … ; do` — ONE line. This PR added
three more names to that loop (`DEVRC_GATE_GO_RUNNER`, `MIN_GO_TESTS`,
`MAX_GO_SKIPS`), which pushed it onto a second line with a backslash. The regex
then matched NOTHING, so the harvester stopped seeing the ENTIRE list —
including `DEVRC_TARGETS` and `MIN_TESTS`, the two the comment directly above it
says it was added to catch after they sat unscrubbed once before.
🔴 IT IS THE SAME FAILURE THE FUNCTION'S OWN DOCSTRING WARNS ABOUT, ONE
INDIRECTION DEEPER, AND IT IS INVISIBLE BY CONSTRUCTION: "the regex found no
loop" and "the loop reads nothing" are the same empty set, so a blind harvester
reports FULL COVERAGE. The only reason this failed loudly rather than silently
is an accident — `DEVRC_GATE_GO_RUNNER` is read a second time as
`${DEVRC_GATE_GO_RUNNER:-…}`, so it stayed visible and made the two sides
disagree. `MIN_GO_TESTS` and `MAX_GO_SKIPS` appear NOWHERE else in gate.sh
except comments; had the go tier not happened to have a runner override, all
three would have been added to the refusal and none would have been pinned,
green.
THE FIX, IN THREE PARTS.
1. THE INSTRUMENT. Join backslash continuations before scanning, so the
harvester sees what the SHELL sees. Reformatting the loop can no longer
retire the ledger.
2. THE CONTROL THAT WOULD HAVE CAUGHT IT —
`test_the_derivation_sees_a_refusal_list_WRAPPED_ACROSS_LINES`, pinned
against a SYNTHETIC script, not against gate.sh, so gate.sh reformatting its
own loop cannot quietly make the control vacuous. `WRAPPED_C` is reachable
ONLY past the continuation; a harvester that stops at the backslash returns
the first two and looks fine. It carries its own negative control: a name
mentioned only in a COMMENT must not be derived.
🔴 MUTATION-TESTED, AND THE EXISTING POSITIVE CONTROL SURVIVES THE MUTANT.
With the join replaced by a regex that cannot match, the new control FAILS
and the two-way pin FAILS (`scrubbed but not in gate.sh: ['DEVRC_TARGETS',
'MAX_GO_SKIPS', 'MIN_GO_TESTS', 'MIN_TESTS']`) — while
`test_the_env_var_derivation_can_actually_see_a_variable` PASSES. That is the
measurement showing the old control could never have seen this.
3. THE TUPLE. `DEVRC_GATE_GO_RUNNER`, `MIN_GO_TESTS` and `MAX_GO_SKIPS` added to
`_AMBIENT_GATE_VARS`. Until now an exported value for any of them rode
straight into every re-exec test.
🔴 AND THE REPAIRED INSTRUMENT IMMEDIATELY FOUND A SECOND CONSEQUENCE.
`test_the_help_text_documents_every_env_var_the_script_reads` is built on the
same derivation, so it was blind in the same way: `MIN_GO_TESTS` and
`MAX_GO_SKIPS` are read by gate.sh and were documented in `--help` NOWHERE. Both
now have an `Env:` entry. A flag documented nowhere the operator looks is a flag
that does not exist — and CLAUDE.md tells readers `--help` lists every variable
it reads, which was false for as long as the go tier had existed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad
… ANY, not just /usr/bin/env c5570bb fixed a blind harvester and tripped a different guard doing it: its synthetic script wrote `#!/usr/bin/env bash` at runtime, which `test_runtime_shebangs.py` exists to forbid — `patchShebangs` fixes the source tree and cannot reach a file a test writes while running, so such a shebang breaks in the nix sandbox. 🔴 THE GUARD IS WIDER THAN ITS NAME. `test_no_test_writes_a_usr_bin_env_shebang_ at_runtime` rejects a self-written `#!/bin/sh` too — measured, after trying that as the fix. The remedy it names is the only one: `testlib.mockbin.write_exec`, which owns the shebang so a call site cannot reintroduce one. The shebang could not simply be dropped: `_env_vars_gate_sh_reads` computes `body_start` over `range(1, …)`, so line 0 is skipped by construction and a script without one would lose its first real line. RE-PROVED THE CONTROL AFTER CHANGING THE FIXTURE. Removing the continuation-join still fails `test_the_derivation_sees_a_refusal_list_WRAPPED_ACROSS_LINES` (and `test_the_ambient_scrub_list_covers_every_gate_variable`), so the guard c5570bb added is still load-bearing through the rewrite — that it passes is not the same claim as that it can still go red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad
ZacxDev
force-pushed
the
feat/mention-review-phase1
branch
from
September 15, 2026 05:47
c5570bb to
f6103dc
Compare
ZacxDev
added a commit
that referenced
this pull request
Sep 15, 2026
…mation ledger that is no longer vacuous (#1723) * feat(mention-review): Phase 2 — the five write verbs, behind a confirmation ledger that is no longer vacuous Comment (PR-level) · approve · request changes · submit review · merge. Four of the five prompt first; a PR-level comment does not, per proposal §3.7, and the reason is stated per entry in NOT_CONFIRMED rather than left to be re-derived. 🔴 PR-LEVEL COMMENTS ONLY. The operator settled proposal §12.4: no inline diff-line positioning. Nothing here computes a diff position, and the endpoint comment in ghapi/write.go says why /issues/{n}/comments is the correct one. 🔴 THE WRITE-INTENT LEDGER IS BACK, AND NON-VACUOUS THIS TIME. #1698 deleted Confirmed / NotConfirmed / LedgerViolations / Write() as a PHASING judgement — in Phase 1 every intent returned a constant false, so the ledger compared two empty maps and read as coverage while providing none. It is reinstated with the assertion that makes it mean something: TestTheDispatchWalkEmitsWriteIntents asserts the keyboard walk PRODUCES each of the five write verbs by name. That test found its own gap on the first run — the walk reached four of five, and SubmitReview was unreachable from any state it drove. 🔴 §10.2 IS THE REASON THE PROMPT IS SHAPED THE WAY IT IS. cli/cli#14370: the OS keyring is not partitioned by account, so the resolved token may belong to a different github.com user than the config calls active, and this host's hosts.yml carries two. So `as <login>` is inside every pinned confirmation string, the compose bar names it too (a comment is outward and does not prompt), and a snapshot with NO login refuses every write verb rather than acting with the clause omitted. 🔴 THE MERGE METHOD IS READ, NEVER GUESSED. internal/cfg splits ABSENT (the declared default, spelled once) from PRESENT-AND-UNTRUSTWORTHY (refuse). An unknown method is the MERGE_METHOD_UNKNOWN sentinel carried over from the Lua — where a mutation replacing it with "squash" survived a fully green suite because nothing reached the arm. Here one keypress reaches it, and a test presses it. Modes are DISJOINT and the keymap ledger is now PER MODE. A ledger that walked only the browse table would report "every binding is helped" while every key that composes or confirms was invisible in the footer — the defect the preceding arc existed to fix, reintroduced inside a mode. Also closes one of the three over-claiming guard descriptions the handoff batched: MeaningBearingStates' doc comment said the ledger covers every constructor in words.go and nothing checked it. words_ledger_test.go now does, by AST rather than by grep. Phase 2 added ModeWord to that file, so the gap was about to grow rather than merely persist. No test can reach the live API: App.runner is nil in every pure test, the one end-to-end test installs a recording fake, and http.DefaultTransport is replaced in both network-reaching packages by one that refuses any non-loopback host — with a negative control proving it rejects api.github.com and a positive control proving it allows an httptest server. A Python guard derives WHICH packages must carry that from their imports, so a new one cannot be added without it. The click path is untouched: REVIEW_EXE still spawns nvim-octo. Phase 4 is gated on the operator using this for a real review. Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad * test(mention-review): pin the intent -> API-call mapping by RUNNING the tea.Cmd Three different intents collapse onto ONE endpoint, distinguished only by an event string. A swap there approves where the operator asked to BLOCK, and every other test in the package passes: Step emitted the right intent, the ledger agreed, the prompt named the right verb, and the wrong verdict went to GitHub. A tea.Cmd is opaque, so the mapping is only observable by CALLING the returned closure against a recording runner. Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad * test(mention-review): three findings from the mutation battery, fixed rather than recorded 1. The browse-mode sweep shared its fixture list with the MODAL states, which are built by pressing keys — so the very mutation it exists to catch (merge fires with no confirmation) ALSO broke the chain building the confirm-merge fixture, and the mutant died with the FIXTURE's error instead of the sweep's assertion. A kill by a different arm is green for the wrong reason and stays green with the arm deleted. The sweep now drives a list built without pressing anything. 2. Disabling proposeMerge's UNKNOWN-method check let a mutant SURVIVE: a second lock catches it (ConfirmPrompt cannot name an empty method), so no merge went out — but the notice changed to "no confirmation prompt could be built … this is a bug", which does not tell the operator what to fix. The test asserted only the REFUSED prefix; it now asserts THIS refusal. 3. The footer's mode was a field nobody branched on in any test: keys_test.go compares the dispatched and helped SETS per mode and says nothing about which mode renderFooter asks for. A footer pinned to ModeBrowse passed every ledger assertion while showing "q quit" over a merge prompt. TestTheFooterFollowsTheMode presses into each mode and asserts both directions — this mode's help present, the others' absent. Floors re-measured by the runner and copied from what it printed. Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad * docs(flake): the mention-review overlay comment said PHASE 1 (read-only) It can write now. Everything else in that block still holds: it is still not in the Alacritty wrapper's makeBinPath, because REVIEW_EXE still spawns nvim-octo and test_mention_open.py pins that list two-way. Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad * fix(mention-review): the write bar was not part of the layout, so the frame overflowed the terminal Found by re-reading the diff, not by a test — which is the point of the guard this adds. `render` subtracted the bar's height from the body; `relayout`, which is what actually SIZES the diff viewport, did not. So opening a compose buffer or raising a confirmation left the viewport sized for a bar-less frame and pushed the bottom rows off the bottom of the terminal. Every existing assertion passed through it. They are about STATE (the right intent, the right mode, the right pending prompt) or about a SUBSTRING of the frame — and the state was correct and the substring was present. Only counting the frame's LINES finds it, which layout_test.go now does, at two terminal heights because the layout branches on available space. Three parts to the fix, each with its own mutant: - relayout subtracts lipgloss.Height(renderBar()), the same call render makes, so the two cannot disagree; - every mode or notice change goes through settled(), because both change the bar's height and neither previously triggered a relayout; - the compose bar is a fixed composeBarLines-row window that FOLLOWS the cursor, so its height cannot move while the operator types — which is the property relayout depends on, since it runs when compose opens. Also drops a duplicated COMPOSING word: ComposeHeader already leads with it, and a doubled word reads as a rendering fault rather than as emphasis. Claude-Session-Id: 09f2cb96-627d-425c-b6c1-7999c68c48ad
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.
Implements
claudedocs/proposal-pr-review-tui.mdPhases 0 and 1. Read-only, andnvim-octois untouched — Phase 4 is out of scope.Phase 0 — three kill criteria, none tripped
innovation-upstream/devrcwith a countingRoundTripper: median 663 ms (23-file PR), 536 ms (small PR), 402 ms (issue) over 5 runs each.viewer { login }present. Every panel's data present — no field was ABSENT; the onlynullwasreviewDecision, a legitimate "nobody has reviewed it" state now rendered as the WORDNONE.OK/NO TOKEN/TOKEN REJECTEDmeasured as a triple, all distinguishable. §10.1's rung 4 reproduced exactly:("", "default")with no error.viewport.View()withSoftWrap=false+SetContentLinesis flat in buffer size: 318 µs @1k, 347 µs @4k (the p99), 342 µs @10k.Probe 3's instrument control. A flat number means nothing unless the benchmark can move.
SoftWrap = true— §6.2 hazard 1 — gives 2.45 ms / 8.46 ms / 21.55 ms at the same sizes: 63× slower at 10,000 lines, and past a 60 fps budget on its own. §6.2 hazard 1 is reproduced and quantified.347 µs sits inside bubbletea#1724's reported 300–800 µs band, i.e. the issue's numbers are consistent with what I measured — and that is ~2% of a 60 fps frame. End-to-end, scrolling is frame-clock bound, not compute bound: wall latency is exactly
1/fps(16.7 ms at 60, 8.2 ms at 120) and identical at 4,000 and 10,000 lines.🔴 The Phase-1 re-measurement found a real defect Phase 0 could not see
The proposal insisted this be re-measured on the real panel, not Phase 0's bare buffer. It was right to.
My first implementation styled the whole buffer on every cursor move — §6.2 hazard 2 — while carrying a comment claiming it styled only the visible window.
Phase 0's bare viewport was flat across those same sizes because it styled nothing. The defect lived in the seam between two things that were each fine alone. Fixed with the viewport's own
StyleLineFunc, which is called on the slice it is about to render — work now bounded by pane height (~36 rows), not buffer size. 14.6× at 10,000 lines.TestScrollingTheRealPanelMeetsTheFrameBudgetis the one genuine regression test here: red at the pre-fix implementation, green at HEAD, and it guards the frame budget itself rather than a number copied off a passing run.What Phase 1 ships
argv contract + exit codes 64/65/66 ported from
nvim-octo.shcase for case · one GraphQL fetch · four panels with focus cycling · a generated help footer · API-only unified diff (go-gitdiff) · the issue card · gruvbox pinned two-way against the Alacritty config ·obrowser,qquit · nix packaging with the version read out of the Go source.The command seam (§3.3) is in from day one.
Step(msg) (App, []Intent)is pure, intents are data, and one ~40-line runner converts them totea.Cmds. No golden frames anywhere — §5.0's four reasons all hold under v2.§10.2 is honoured. I independently confirmed this host's
ghconfig carries two github.com users.viewer { login }rides the same round trip and is rendered in the Overview panel and the footer, with mutants for both.A third gate tier (§5.6), wired rather than mentioned
scripts/run-go-tests.sh+gate.sh --tier go+checks.gotests+pkgs.goingateTools. Same guard shape as the other two runners, plus two of its own: exit 3 on a missing toolchain (never a pass) and a SKIP budget of 0.--tier bothis now an alias forall— a caller who typed it before Go existed wanted the whole gate, and silently excluding a language is the "list that only grows when somebody remembers" defect.doCheck = falseon the deploy derivation is a safety property, verified in the pinned nixpkgs rather than assumed:module.nixdefaults ittrue, so a red Go test would fail ahome-manager switch— the skipped-host failure mode.Three defects the floors caught, each of which would otherwise have printed
RESULT: PASSover zero tests:"Package"sits betweenActionandTest);Verification
nix build .#checks.x86_64-linux.gotests).err.Codeagainst the constantExitBadRepo, so changing65 → 64moved the expectation with the behaviour. Now pinned as literals.+marker from a diff row makes an add indistinguishable from context once colour is stripped, and it survived the whole suite.MeaningBearingStates()covered the Overview and not one row of the pane the tool is for.NULL, 2 matches →NULL, control →0.1.0. Packaged binary verified:--versionequals the store-path label; 64/65/66 hold.test_no_real_launchers.py's PATH-clobber ledger caught my new test and now carries its justification — the system working.Honest notes
REVIEW_EXEstill spawnsnvim-octo.test_mention_open.py's two-way wrapper ledger already enforces this — addingpkgs.mention-reviewto the Alacritty PATH before flipping would fail as "dead weight in the closure".Full detail, including where reality diverged from the proposal, is in the commit message.
🤖 Generated with Claude Code