Repository navigation
feat(nvim): show the pull request beside a diff prs opened - #106
Conversation
prs starts nvim's diff with PRS_DIFF_DOCUMENT naming the pull request's markdown document and PRS_DIFF_THREADS naming its review threads. core/pull-request-panel reads them, and does nothing where the first is unset, so every other nvim is untouched. - <leader>gr shows the document in a split at the right edge, and hides it. <leader>gR does the same in a float. - <CR> or gf on a path or path:line mention in the document selects that file in the open diffview through its Lua API, and puts the cursor on the line, on the side the thread there names. Where the cursor is on no file the diff changes, the first backticked one on the line is taken, so a thread's heading or a row of the files table jumps from anywhere on it. - Each review thread shows as virtual text at the end of its line, on the base's side for LEFT and the head's for RIGHT. tests/nvim/pull_request_panel_spec.lua drives it in a headless Neovim against a real git repo and the diffview.nvim lazy installed. It clears the variables git exports to a hook that name the repo being committed, as tests/conftest.py does, so its git and diffview's act on the repo it builds. task test:nvim runs every spec there, and a commit hook runs that task when the nvim config or the specs change. Agent-Name: claude-code-unattended Agent-Session: 6ab7d888-63b1-4910-ab41-c89e8e170c7a Agent-Run: 01a1b9ba-16f0-7b20-87db-b565f27ef2a8
prs starts nvim in the repo's checkout with only -c arguments, which auto-session reads as a session start. At exit it saved the diff over the repo's session: the next nvim there reopened a diffview:// buffer, and the pull request's document, both gone once that nvim exited. auto-session now stays unloaded where PRS_DIFF_DOCUMENT is set, so the diff neither saves over the session nor restores it around the diff. Agent-Name: claude-code-unattended Agent-Session: 6ab7d888-63b1-4910-ab41-c89e8e170c7a Agent-Run: 01a1b9ba-16f0-7b20-87db-b565f27ef2a8
There was a problem hiding this comment.
PR review — system-design
Used: Read the full diff, the CI hooks job in validate.yml, and the prs repo (default branch and its open PR list) for the far side of PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. I did not run nvim.
Standards: No fleet standards search run. I cited no rule.
Findings: 1 correctness · 0 design · 0 prose
Correctness
system-design.1 .pre-commit-config.yaml:426 · tests/nvim/pull_request_panel_spec.lua:14
Reach: A commit on any machine that lacks the lazy-installed diffview.nvim or task blocks. That covers a fresh machine, a server, a container, or a clone where nvim has not yet started with this config. The sequence is: the user stages a file under configs/common/.config/nvim/ or tests/nvim/, the new hook runs task test:nvim, and the spec calls cquit because stdpath('data')/lazy/diffview.nvim is missing. The commit is rejected. The hook's own comment and the README admit the machine must have run nvim once. CI is not affected: the explicit hooks=(...) list in .github/workflows/validate.yml omits nvim-specs.
The nvim-specs hook is language: system. It depends on task, nvim and a plugin directory that only a prior interactive nvim start creates. Nothing in the repo provisions that directory. When the directory is missing, the spec reports FAIL, so the user cannot commit a change to the nvim config until they have started nvim by hand. The hook also reads the real stdpath('data') of whoever runs it, so its result depends on that machine's plugin state.
Fix: make the spec skip with a clear message when diffview is absent, as tests/shell does for missing interpreters. Alternatively, have the Taskfile target bootstrap the plugin into a directory under the repo or $TMPDIR with nvim --headless and Lazy! sync. Alternatively, set stages: [manual] and run it deliberately. Whichever is chosen, say whether it also belongs in the CI hooks list. It is currently absent, so CI never exercises the spec.
Findings as JSON
{
"used": "Read the full diff, the CI hooks job in validate.yml, and the prs repo (default branch and its open PR list) for the far side of PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. I did not run nvim.",
"standards": "No `fleet standards search` run. I cited no rule.",
"findings": [
{
"number": "system-design.1",
"section": "correctness",
"sites": [
".pre-commit-config.yaml:426",
"tests/nvim/pull_request_panel_spec.lua:14"
],
"reach": "A commit on any machine that lacks the lazy-installed diffview.nvim or `task` blocks. That covers a fresh machine, a server, a container, or a clone where nvim has not yet started with this config. The sequence is: the user stages a file under `configs/common/.config/nvim/` or `tests/nvim/`, the new hook runs `task test:nvim`, and the spec calls `cquit` because `stdpath('data')/lazy/diffview.nvim` is missing. The commit is rejected. The hook's own comment and the README admit the machine must have run nvim once. CI is not affected: the explicit `hooks=(...)` list in `.github/workflows/validate.yml` omits `nvim-specs`.",
"body": "The `nvim-specs` hook is `language: system`. It depends on `task`, `nvim` and a plugin directory that only a prior interactive nvim start creates. Nothing in the repo provisions that directory. When the directory is missing, the spec reports FAIL, so the user cannot commit a change to the nvim config until they have started nvim by hand. The hook also reads the real `stdpath('data')` of whoever runs it, so its result depends on that machine's plugin state.\n\n**Fix:** make the spec skip with a clear message when diffview is absent, as `tests/shell` does for missing interpreters. Alternatively, have the Taskfile target bootstrap the plugin into a directory under the repo or `$TMPDIR` with `nvim --headless` and `Lazy! sync`. Alternatively, set `stages: [manual]` and run it deliberately. Whichever is chosen, say whether it also belongs in the CI `hooks` list. It is currently absent, so CI never exercises the spec."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — stale-prose
Used: I checked out PR 106 at 2561ac6 and read every comment, help string, error message and README line the diff adds, each beside its code. I ran setup() in headless nvim from that checkout with XDG paths inside my directory, once with only PRS_DIFF_THREADS set and once with only PRS_DIFF_DOCUMENT set. I compared the spec's git list with tests/conftest.py. I cloned datapointchris/prs and read prs#9's diff.
Standards: I applied pull-requests.md/Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite. Searches also surfaced documentation.md/A comment states what this repos code enforces, never another repos deployment as fact and documentation.md/A doc points at the source instead of restating its content or constants.
Findings: 0 correctness · 0 design · 3 prose
Prose
stale-prose.1 configs/common/.config/nvim/lua/core/pull-request-panel.lua:4
False: the module header says both variables gate setup(). Only PRS_DIFF_DOCUMENT does.
The sentence is -- files, and names them in PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. Where neither is set, setup() does nothing, so every other nvim is untouched. (pull-request-panel.lua:4-5).
A reader takes it to mean either variable alone turns the panel on. setup() checks only vim.env.PRS_DIFF_DOCUMENT. I ran setup() in headless nvim from the head checkout:
- With only
PRS_DIFF_THREADS=/nonexistentset, it returnedthreads only: false. - With only
PRS_DIFF_DOCUMENT=/xset, it returneddocument only: true.
The docstring on M.setup already says this correctly: "does nothing where the document's is unset". The header contradicts it.
Rewrite: -- files, and names them in PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. Where PRS_DIFF_DOCUMENT is unset, setup() does nothing, so every other nvim is untouched.
stale-prose.2 configs/common/.config/nvim/lua/core/pull-request-panel.lua:18 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:1 · tests/nvim/pull_request_panel_spec.lua:100
Will go stale: three comments state prs's files and keys as fact, and nothing in this repo checks them against prs.
The sentences:
-- The threads file's shape is prs's cmd/prs/diff-threads.schema.json.(pull-request-panel.lua:18). prs's default branch has no such file today. A shallow clone of datapointchris/prs has onlycmd/prs/list.schema.json. The file exists only on prs#9's branchnvim-pr-panel, and prs#9 is OPEN. If that PR changes the path before it merges, nothing here notices.-- The pull request beside its diff, in an nvim that prs started withd.(pull-request-panel.lua:1). The key is a binding in prs'sinternal/board/actions.go. Nothing in this module depends on which key started nvim.-- The two files prs writes, in the shapes it writes them.(pull_request_panel_spec.lua:100). The markdown document and the threads JSON below it are written by hand. Nothing checks them against prs's schema or its writer. The comment stays true only while someone edits both repos together.
documentation.md § "A comment states what this repos code enforces, never another repos deployment as fact" also covers these sentences.
Rewrites:
- Line 18:
-- PRS_DIFF_THREADS names the JSON prs writes for a diff. This module reads each thread's path, line, side, resolved, comments and comment_count.That names the contract from this file's side, and the code can be checked against it. - Line 1:
-- The pull request beside its diff, in an nvim prs started to show a pull request's diff.Drop the key. - Spec line 100:
-- A document and a threads file shaped like the ones prs writes. Nothing checks them against prs's schema.Or delete the sentence.
stale-prose.3 tests/nvim/pull_request_panel_spec.lua:42
Will go stale: the spec says its list of git variables matches tests/conftest.py, and nothing keeps the two lists the same.
The sentence is -- one built here. tests/conftest.py clears the same list for the Python suite. (pull_request_panel_spec.lua:42).
Today it is true. tests/conftest.py:121-129 (GIT_LOCATION) holds the same eight names in the same order: GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_PREFIX and GIT_NAMESPACE. The two lists are separate literals in two languages, and nothing compares them. The next variable added to GIT_LOCATION makes this comment false, and nothing will flag it.
Rewrite: delete the sentence. The two sentences before it already say why the variables are cleared. If the link should stay, make it a pointer that asks for an edit, not a claim of sameness: -- tests/conftest.py's GIT_LOCATION is the Python suite's copy of this list; a name added to one belongs in both.
Findings as JSON
{
"used": "I checked out PR 106 at 2561ac6c and read every comment, help string, error message and README line the diff adds, each beside its code. I ran `setup()` in headless nvim from that checkout with XDG paths inside my directory, once with only PRS_DIFF_THREADS set and once with only PRS_DIFF_DOCUMENT set. I compared the spec's git list with `tests/conftest.py`. I cloned datapointchris/prs and read prs#9's diff.",
"standards": "I applied `pull-requests.md/Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite`. Searches also surfaced `documentation.md/A comment states what this repos code enforces, never another repos deployment as fact` and `documentation.md/A doc points at the source instead of restating its content or constants`.",
"findings": [
{
"number": "stale-prose.1",
"section": "prose",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:4"
],
"reach": "",
"body": "**False: the module header says both variables gate `setup()`. Only PRS_DIFF_DOCUMENT does.**\n\nThe sentence is `-- files, and names them in PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. Where neither is set, setup() does nothing, so every other nvim is untouched.` (`pull-request-panel.lua:4-5`).\n\nA reader takes it to mean either variable alone turns the panel on. `setup()` checks only `vim.env.PRS_DIFF_DOCUMENT`. I ran `setup()` in headless nvim from the head checkout:\n\n- With only `PRS_DIFF_THREADS=/nonexistent` set, it returned `threads only: false`.\n- With only `PRS_DIFF_DOCUMENT=/x` set, it returned `document only: true`.\n\nThe docstring on `M.setup` already says this correctly: \"does nothing where the document's is unset\". The header contradicts it.\n\n**Rewrite:** `-- files, and names them in PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS. Where PRS_DIFF_DOCUMENT is unset, setup() does nothing, so every other nvim is untouched.`"
},
{
"number": "stale-prose.2",
"section": "prose",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:18",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:1",
"tests/nvim/pull_request_panel_spec.lua:100"
],
"reach": "",
"body": "**Will go stale: three comments state prs's files and keys as fact, and nothing in this repo checks them against prs.**\n\nThe sentences:\n\n- `-- The threads file's shape is prs's cmd/prs/diff-threads.schema.json.` (`pull-request-panel.lua:18`). prs's default branch has no such file today. A shallow clone of datapointchris/prs has only `cmd/prs/list.schema.json`. The file exists only on prs#9's branch `nvim-pr-panel`, and prs#9 is OPEN. If that PR changes the path before it merges, nothing here notices.\n- `-- The pull request beside its diff, in an nvim that prs started with `d`.` (`pull-request-panel.lua:1`). The key is a binding in prs's `internal/board/actions.go`. Nothing in this module depends on which key started nvim.\n- `-- The two files prs writes, in the shapes it writes them.` (`pull_request_panel_spec.lua:100`). The markdown document and the threads JSON below it are written by hand. Nothing checks them against prs's schema or its writer. The comment stays true only while someone edits both repos together.\n\n`documentation.md` § \"A comment states what this repos code enforces, never another repos deployment as fact\" also covers these sentences.\n\n**Rewrites:**\n\n- Line 18: `-- PRS_DIFF_THREADS names the JSON prs writes for a diff. This module reads each thread's path, line, side, resolved, comments and comment_count.` That names the contract from this file's side, and the code can be checked against it.\n- Line 1: `-- The pull request beside its diff, in an nvim prs started to show a pull request's diff.` Drop the key.\n- Spec line 100: `-- A document and a threads file shaped like the ones prs writes. Nothing checks them against prs's schema.` Or delete the sentence."
},
{
"number": "stale-prose.3",
"section": "prose",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:42"
],
"reach": "",
"body": "**Will go stale: the spec says its list of git variables matches `tests/conftest.py`, and nothing keeps the two lists the same.**\n\nThe sentence is `-- one built here. tests/conftest.py clears the same list for the Python suite.` (`pull_request_panel_spec.lua:42`).\n\nToday it is true. `tests/conftest.py:121-129` (`GIT_LOCATION`) holds the same eight names in the same order: `GIT_DIR`, `GIT_WORK_TREE`, `GIT_COMMON_DIR`, `GIT_INDEX_FILE`, `GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_PREFIX` and `GIT_NAMESPACE`. The two lists are separate literals in two languages, and nothing compares them. The next variable added to `GIT_LOCATION` makes this comment false, and nothing will flag it.\n\n**Rewrite:** delete the sentence. The two sentences before it already say why the variables are cleared. If the link should stay, make it a pointer that asks for an edit, not a claim of sameness: `-- tests/conftest.py's GIT_LOCATION is the Python suite's copy of this list; a name added to one belongs in both.`"
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — code-architecture
Used: I cloned the repo and checked out PR 106 at 2561ac6. I read the whole nvim config, core/profiles.lua, core/lazy.lua, the spec, the Taskfile, the pre-commit hook and tests/conftest.py. I loaded auto-session.lua's spec and pull-request-panel.setup() in a headless nvim 0.10.4 with a scratch HOME. I read lazy.nvim's lua/lazy/core/plugin.lua to see how defaults.cond is applied. The spec itself did not run here, because diffview.nvim is not installed on this pod.
Standards: code-quality.md/ask-whatever-owns-a-fact-the-function-or-enum-that-computes-it-never-duplicate-its-logic-at-a-second-site; testing.md/a-test-suite-that-runs-git-starts-with-gits-repository-variables-cleared; no rule found for "lazy.nvim plugin cond replaces defaults cond profile".
Findings: 0 correctness · 2 design · 0 prose
Design and architecture
code-architecture.1 configs/common/.config/nvim/lua/plugins/auto-session.lua:7 · configs/common/.config/nvim/lua/core/profiles.lua:35 · configs/common/.config/nvim/lua/core/lazy.lua:22 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:315
Reach: Two changes are coming. The first is the next plugin that has to stay unloaded in a prs diff nvim. The second is the next profile rule for auto-session. This PR is already the first case: its second commit added the gate after a session got overwritten. profiles.lua shows the second case already exists, because it gates every plugin for VSCode. Each further gated plugin adds one more per-spec cond and one more copy of the env-var test. Any plugin that gets one also drops out of the VSCode allowlist without anyone noticing.
Which plugins load in a prs diff nvim is decided outside core/profiles
core/profiles.lua owns which plugins load in which kind of nvim. core/lazy.lua:22 installs profiles.plugin_enabled as lazy's defaults.cond. This PR adds a second owner: a cond on the auto-session spec that tests vim.env.PRS_DIFF_DOCUMENT.
lazy.nvim applies defaults.cond only where a spec sets no cond. In lua/lazy/core/plugin.lua:267-270, local cond = plugin.cond; if cond == nil then cond = Config.options.defaults.cond end. The spec's own cond therefore replaces the profile function for auto-session. It does not combine with it. Two modules now decide whether auto-session loads, and only one of them runs.
The predicate "this nvim is a prs diff" also has two owners. pull-request-panel.setup() (line 315-316) tests non-nil and non-empty. auto-session.lua:8 tests not vim.env.PRS_DIFF_DOCUMENT. They agree today only because nvim's vim.env returns nil for an empty variable. I loaded both in a headless nvim with PRS_DIFF_DOCUMENT='': cond returned true and setup returned false. Nothing compares the two tests. code-quality.md § "Ask whatever owns a fact" reaches this.
Fix: make the prs diff nvim a profile in core/profiles.lua:
- add
M.is_prs_diff, computed once fromPRS_DIFF_DOCUMENT; - have
plugin_enabledreturn false for the plugins on aprs_diff_disabledlist, with auto-session as its one member; - delete the
condfromauto-session.lua; - have
pull-request-panel.setup()readprofiles.is_prs_diffinstead of the env var.
The next gated plugin is then one line in one table, and it keeps the VSCode allowlist.
code-architecture.2 tests/nvim/pull_request_panel_spec.lua:43 · tests/conftest.py:121
Reach: The next variable git adds to its repository set, or the next one found to leak, has to be added in both lists, in two languages. The conftest docstring records that this list has been incomplete before: it "held only by accident until worktrees". Each new tests/nvim/*_spec.lua that runs git would add a third copy. task test:nvim is a glob over specs, so more of them are expected.
The list of git variables to clear has two owners
The spec hard-codes eight names: GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_PREFIX and GIT_NAMESPACE. Its comment says it copies tests/conftest.py's GIT_LOCATION. Nothing checks that the two lists stay equal.
Neither list owns the fact. git owns it. On this pod, git rev-parse --local-env-vars names 15 variables, including GIT_CONFIG, GIT_CONFIG_PARAMETERS, GIT_CONFIG_COUNT, GIT_IMPLICIT_WORK_TREE, GIT_GRAFT_FILE, GIT_NO_REPLACE_OBJECTS, GIT_REPLACE_REF_BASE and GIT_SHALLOW_FILE. testing.md § "A test suite that runs git starts with git's repository variables cleared" says a suite unsets the names that command returns.
Fix: have the spec's first lines run vim.fn.systemlist({'git', 'rev-parse', '--local-env-vars'}) and clear each name it returns. Have tests/conftest.py derive GIT_LOCATION the same way. Then no hand-written list exists to keep in step.
Findings as JSON
{
"used": "I cloned the repo and checked out PR 106 at 2561ac6c. I read the whole nvim config, `core/profiles.lua`, `core/lazy.lua`, the spec, the Taskfile, the pre-commit hook and `tests/conftest.py`. I loaded `auto-session.lua`'s spec and `pull-request-panel.setup()` in a headless nvim 0.10.4 with a scratch HOME. I read lazy.nvim's `lua/lazy/core/plugin.lua` to see how `defaults.cond` is applied. The spec itself did not run here, because diffview.nvim is not installed on this pod.",
"standards": "code-quality.md/ask-whatever-owns-a-fact-the-function-or-enum-that-computes-it-never-duplicate-its-logic-at-a-second-site; testing.md/a-test-suite-that-runs-git-starts-with-gits-repository-variables-cleared; no rule found for \"lazy.nvim plugin cond replaces defaults cond profile\".",
"findings": [
{
"number": "code-architecture.1",
"section": "design",
"sites": [
"configs/common/.config/nvim/lua/plugins/auto-session.lua:7",
"configs/common/.config/nvim/lua/core/profiles.lua:35",
"configs/common/.config/nvim/lua/core/lazy.lua:22",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:315"
],
"reach": "Two changes are coming. The first is the next plugin that has to stay unloaded in a prs diff nvim. The second is the next profile rule for auto-session. This PR is already the first case: its second commit added the gate after a session got overwritten. `profiles.lua` shows the second case already exists, because it gates every plugin for VSCode. Each further gated plugin adds one more per-spec `cond` and one more copy of the env-var test. Any plugin that gets one also drops out of the VSCode allowlist without anyone noticing.",
"body": "### Which plugins load in a prs diff nvim is decided outside `core/profiles`\n\n`core/profiles.lua` owns which plugins load in which kind of nvim. `core/lazy.lua:22` installs `profiles.plugin_enabled` as lazy's `defaults.cond`. This PR adds a second owner: a `cond` on the auto-session spec that tests `vim.env.PRS_DIFF_DOCUMENT`.\n\nlazy.nvim applies `defaults.cond` only where a spec sets no `cond`. In `lua/lazy/core/plugin.lua:267-270`, `local cond = plugin.cond; if cond == nil then cond = Config.options.defaults.cond end`. The spec's own `cond` therefore replaces the profile function for auto-session. It does not combine with it. Two modules now decide whether auto-session loads, and only one of them runs.\n\nThe predicate \"this nvim is a prs diff\" also has two owners. `pull-request-panel.setup()` (line 315-316) tests non-nil and non-empty. `auto-session.lua:8` tests `not vim.env.PRS_DIFF_DOCUMENT`. They agree today only because nvim's `vim.env` returns nil for an empty variable. I loaded both in a headless nvim with `PRS_DIFF_DOCUMENT=''`: cond returned `true` and setup returned `false`. Nothing compares the two tests. code-quality.md § \"Ask whatever owns a fact\" reaches this.\n\n**Fix:** make the prs diff nvim a profile in `core/profiles.lua`:\n- add `M.is_prs_diff`, computed once from `PRS_DIFF_DOCUMENT`;\n- have `plugin_enabled` return false for the plugins on a `prs_diff_disabled` list, with auto-session as its one member;\n- delete the `cond` from `auto-session.lua`;\n- have `pull-request-panel.setup()` read `profiles.is_prs_diff` instead of the env var.\n\nThe next gated plugin is then one line in one table, and it keeps the VSCode allowlist."
},
{
"number": "code-architecture.2",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:43",
"tests/conftest.py:121"
],
"reach": "The next variable git adds to its repository set, or the next one found to leak, has to be added in both lists, in two languages. The conftest docstring records that this list has been incomplete before: it \"held only by accident until worktrees\". Each new `tests/nvim/*_spec.lua` that runs git would add a third copy. `task test:nvim` is a glob over specs, so more of them are expected.",
"body": "### The list of git variables to clear has two owners\n\nThe spec hard-codes eight names: `GIT_DIR`, `GIT_WORK_TREE`, `GIT_COMMON_DIR`, `GIT_INDEX_FILE`, `GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_PREFIX` and `GIT_NAMESPACE`. Its comment says it copies `tests/conftest.py`'s `GIT_LOCATION`. Nothing checks that the two lists stay equal.\n\nNeither list owns the fact. git owns it. On this pod, `git rev-parse --local-env-vars` names 15 variables, including `GIT_CONFIG`, `GIT_CONFIG_PARAMETERS`, `GIT_CONFIG_COUNT`, `GIT_IMPLICIT_WORK_TREE`, `GIT_GRAFT_FILE`, `GIT_NO_REPLACE_OBJECTS`, `GIT_REPLACE_REF_BASE` and `GIT_SHALLOW_FILE`. testing.md § \"A test suite that runs git starts with git's repository variables cleared\" says a suite unsets the names that command returns.\n\n**Fix:** have the spec's first lines run `vim.fn.systemlist({'git', 'rev-parse', '--local-env-vars'})` and clear each name it returns. Have `tests/conftest.py` derive `GIT_LOCATION` the same way. Then no hand-written list exists to keep in step."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — fleet-patterns
Used: I cloned dotfiles at PR 106's head and read the panel, the auto-session cond, the spec, the Taskfile task and the hook. I also cloned prs at PR #9's head and read diff_files.go, diff-threads.schema.json, document.go and tui_test.go. I read homelab's tests/runner-update.bats and git-graph's tests/conftest.py through gh api. I ran gh search code --owner datapointchris local-env-vars and git rev-parse --local-env-vars. I built nothing and ran nothing in nvim.
Standards: These rules apply: testing.md § "A test suite that runs git starts with git's repository variables cleared", cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes" ("A reader points at that contract and never copies the fields"), testing.md § "A stand-in carries the shapes of the real thing", and shell.md § "A generated artifact is validated against its new consumer, not its producer".
Findings: 0 correctness · 2 design · 0 prose
Design and architecture
fleet-patterns.1 tests/nvim/pull_request_panel_spec.lua:42 · tests/conftest.py:121 · dotfiles:.pre-commit-config.yaml:113 · homelab:tests/runner-update.bats:12 · git-graph:tests/conftest.py:14
Reach: The spec runs under the new nvim-specs commit hook, which is where git exports these variables. Its own comment, "tests/conftest.py clears the same list", is a fix-both note between two hard-coded copies. If git exports a variable outside the eight, both copies miss it. If one copy gains a name, the other has nothing tying it to the change.
The spec copies a hard-coded list of git variables when three other suites ask git for it.
The spec unsets eight names it typed out, copied from GIT_LOCATION in tests/conftest.py. Three other sites ask git at run time with git rev-parse --local-env-vars:
- homelab's bats
setup()does it withmapfile -t git_vars < <(git rev-parse --local-env-vars). - git-graph's
conftest.pybuildsGIT_REPOSITORY_VARIABLESfrom the same call. - This repo's own
batshook at.pre-commit-config.yaml:113runsunset $(git rev-parse --local-env-vars).
On this machine, git rev-parse --local-env-vars returns 15 names. The eight-name list leaves out seven of them:
GIT_CONFIGGIT_CONFIG_PARAMETERSGIT_CONFIG_COUNTGIT_IMPLICIT_WORK_TREEGIT_GRAFT_FILEGIT_SHALLOW_FILEGIT_NO_REPLACE_OBJECTS
A hook started by git -c … commit inherits GIT_CONFIG_PARAMETERS. That variable then reaches every git call the spec and diffview make.
testing.md § "A test suite that runs git starts with git's repository variables cleared" names homelab's --local-env-vars call as the canonical source.
Fix: in the spec, replace the literal table with vim.fn.systemlist({'git', 'rev-parse', '--local-env-vars'}) and drop the "same list" comment. A follow-up can do the same to GIT_LOCATION in tests/conftest.py, as git-graph already does. Both suites would then take the list from git, and neither would hold a copy.
fleet-patterns.2 tests/nvim/pull_request_panel_spec.lua:100 · tests/nvim/pull_request_panel_spec.lua:131 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:18 · prs:cmd/prs/diff-threads.schema.json:1 · prs:internal/board/diff_files.go:16 · prs:internal/board/document.go:239 · prs:cmd/prs/tui_test.go:495
Reach: prs#9 is still open, so its schema and document headings can change before it merges. Some changes to prs's threads shape would leave this spec passing while the panel shows nothing: - renaming side - changing the ### path:line`` heading that M.mentions matches - emitting `line` differently The owner checks its own output against the schema. Nothing checks the copy here.
The spec hand-writes both prs files instead of reading what prs actually writes.
prs owns two outputs:
- the threads file, whose shape is
cmd/prs/diff-threads.schema.json($id urn:prs:diff-threads:v1) - the markdown document, written by
MarkdownDocument, which emits###path:line· stateheadings and the| File | Change |table
prs's tui_test.go:495 validates its real output against that schema. This spec writes both files by hand at lines 100–170. The comment there says "in the shapes it writes them", but nothing checks that claim.
cli-design.md § "The shape a --json read or an endpoint returns is a JSON Schema file…" says a reader "points at that contract and never copies the fields". testing.md § "A stand-in carries the shapes of the real thing" says a consumer is tested with real output, or through a test that runs the real producer into it.
prs's own test already has the real output in hand. It copies both files to handed.md and handed.threads.json.
The two copies agree today. This is one producer and one consumer, so the cross-repo evidence is a single pair. The cost comes from the timing: prs#9 is open and can change both formats.
Fix, in order of preference:
- Have prs commit the
handed.mdandhanded.threads.jsonits TUI test produces as golden testdata, and have this spec read those files. - Otherwise, vendor the schema with its
$idand validate the spec's fixture against it intask test:nvim. A schema change then fails here too.
Findings as JSON
{
"used": "I cloned dotfiles at PR 106's head and read the panel, the auto-session cond, the spec, the Taskfile task and the hook. I also cloned prs at PR #9's head and read `diff_files.go`, `diff-threads.schema.json`, `document.go` and `tui_test.go`. I read homelab's `tests/runner-update.bats` and git-graph's `tests/conftest.py` through `gh api`. I ran `gh search code --owner datapointchris local-env-vars` and `git rev-parse --local-env-vars`. I built nothing and ran nothing in nvim.",
"standards": "These rules apply: testing.md § \"A test suite that runs git starts with git's repository variables cleared\", cli-design.md § \"The shape a --json read or an endpoint returns is a JSON Schema file that the data catalog publishes\" (\"A reader points at that contract and never copies the fields\"), testing.md § \"A stand-in carries the shapes of the real thing\", and shell.md § \"A generated artifact is validated against its new consumer, not its producer\".",
"findings": [
{
"number": "fleet-patterns.1",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:42",
"tests/conftest.py:121",
"dotfiles:.pre-commit-config.yaml:113",
"homelab:tests/runner-update.bats:12",
"git-graph:tests/conftest.py:14"
],
"reach": "The spec runs under the new `nvim-specs` commit hook, which is where git exports these variables. Its own comment, \"tests/conftest.py clears the same list\", is a fix-both note between two hard-coded copies. If git exports a variable outside the eight, both copies miss it. If one copy gains a name, the other has nothing tying it to the change.",
"body": "**The spec copies a hard-coded list of git variables when three other suites ask git for it.**\n\nThe spec unsets eight names it typed out, copied from `GIT_LOCATION` in `tests/conftest.py`. Three other sites ask git at run time with `git rev-parse --local-env-vars`:\n- homelab's bats `setup()` does it with `mapfile -t git_vars < <(git rev-parse --local-env-vars)`.\n- git-graph's `conftest.py` builds `GIT_REPOSITORY_VARIABLES` from the same call.\n- This repo's own `bats` hook at `.pre-commit-config.yaml:113` runs `unset $(git rev-parse --local-env-vars)`.\n\nOn this machine, `git rev-parse --local-env-vars` returns 15 names. The eight-name list leaves out seven of them:\n- `GIT_CONFIG`\n- `GIT_CONFIG_PARAMETERS`\n- `GIT_CONFIG_COUNT`\n- `GIT_IMPLICIT_WORK_TREE`\n- `GIT_GRAFT_FILE`\n- `GIT_SHALLOW_FILE`\n- `GIT_NO_REPLACE_OBJECTS`\n\nA hook started by `git -c … commit` inherits `GIT_CONFIG_PARAMETERS`. That variable then reaches every `git` call the spec and diffview make.\n\ntesting.md § \"A test suite that runs git starts with git's repository variables cleared\" names homelab's `--local-env-vars` call as the canonical source.\n\n**Fix:** in the spec, replace the literal table with `vim.fn.systemlist({'git', 'rev-parse', '--local-env-vars'})` and drop the \"same list\" comment. A follow-up can do the same to `GIT_LOCATION` in `tests/conftest.py`, as git-graph already does. Both suites would then take the list from git, and neither would hold a copy."
},
{
"number": "fleet-patterns.2",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:100",
"tests/nvim/pull_request_panel_spec.lua:131",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:18",
"prs:cmd/prs/diff-threads.schema.json:1",
"prs:internal/board/diff_files.go:16",
"prs:internal/board/document.go:239",
"prs:cmd/prs/tui_test.go:495"
],
"reach": "prs#9 is still open, so its schema and document headings can change before it merges. Some changes to prs's threads shape would leave this spec passing while the panel shows nothing:\n- renaming `side`\n- changing the `### `path:line`` heading that `M.mentions` matches\n- emitting `line` differently\n\nThe owner checks its own output against the schema. Nothing checks the copy here.",
"body": "**The spec hand-writes both prs files instead of reading what prs actually writes.**\n\nprs owns two outputs:\n- the threads file, whose shape is `cmd/prs/diff-threads.schema.json` (`$id urn:prs:diff-threads:v1`)\n- the markdown document, written by `MarkdownDocument`, which emits `### `path:line` · state` headings and the `| File | Change |` table\n\nprs's `tui_test.go:495` validates its real output against that schema. This spec writes both files by hand at lines 100–170. The comment there says \"in the shapes it writes them\", but nothing checks that claim.\n\ncli-design.md § \"The shape a --json read or an endpoint returns is a JSON Schema file…\" says a reader \"points at that contract and never copies the fields\". testing.md § \"A stand-in carries the shapes of the real thing\" says a consumer is tested with real output, or through a test that runs the real producer into it.\n\nprs's own test already has the real output in hand. It copies both files to `handed.md` and `handed.threads.json`.\n\nThe two copies agree today. This is one producer and one consumer, so the cross-repo evidence is a single pair. The cost comes from the timing: prs#9 is open and can change both formats.\n\n**Fix, in order of preference:**\n1. Have prs commit the `handed.md` and `handed.threads.json` its TUI test produces as golden testdata, and have this spec read those files.\n2. Otherwise, vendor the schema with its `$id` and validate the spec's fixture against it in `task test:nvim`. A schema change then fails here too."
}
]
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — failure-paths
Used: Cloned the PR head. Ran core.pull-request-panel setup() headless with an isolated HOME against three threads files: threads_read=false, truncated JSON and a missing path.
Standards: code-quality.md § "A result that can come back partial carries the signal that tells it from a whole one". Searched "partial result carries signal distinguishing it from whole".
Findings: 1 correctness · 0 design · 0 prose
Correctness
failure-paths.1 configs/common/.config/nvim/lua/core/pull-request-panel.lua:38 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:39 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:47
Reach: prs writes the threads file in the shape of its diff-threads.schema.json. The spec fixture at tests/nvim/pull_request_panel_spec.lua:140 shows a threads_read field. A prs run whose thread fetch failed gives threads_read=false and an empty list. A deleted or never-written file also reaches the same branch. I ran setup() with {"threads_read":false,"threads":[]} and with a nonexistent threads path. Both returned true with 0 notifications. Truncated JSON did notify.
The panel ignores threads_read. read_threads returns {} with no message in two cases. One is a path that is unset, empty or unreadable. The other is a file that decodes with threads_read=false. The diff then shows no thread markers, which reads as a PR with no review comments. A reviewer can approve while unresolved threads exist.
Fix: in read_threads, check decoded.threads_read == false and say(..., WARN) that the threads could not be fetched. Do the same when PRS_DIFF_THREADS is set but its file is unreadable. Keep a flag in state so decorate or the panel can mark the view as incomplete. Add a spec case for each. The spec only covers threads_read=true. This follows code-quality.md § "A result that can come back partial carries the signal that tells it from a whole one".
Findings as JSON
{
"used": "Cloned the PR head. Ran core.pull-request-panel setup() headless with an isolated HOME against three threads files: threads_read=false, truncated JSON and a missing path.",
"standards": "code-quality.md § \"A result that can come back partial carries the signal that tells it from a whole one\". Searched \"partial result carries signal distinguishing it from whole\".",
"findings": [
{
"number": "failure-paths.1",
"section": "correctness",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:38",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:39",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:47"
],
"reach": "prs writes the threads file in the shape of its diff-threads.schema.json. The spec fixture at tests/nvim/pull_request_panel_spec.lua:140 shows a `threads_read` field. A prs run whose thread fetch failed gives `threads_read=false` and an empty list. A deleted or never-written file also reaches the same branch. I ran setup() with `{\"threads_read\":false,\"threads\":[]}` and with a nonexistent threads path. Both returned true with 0 notifications. Truncated JSON did notify.",
"body": "The panel ignores `threads_read`. `read_threads` returns `{}` with no message in two cases. One is a path that is unset, empty or unreadable. The other is a file that decodes with `threads_read=false`. The diff then shows no thread markers, which reads as a PR with no review comments. A reviewer can approve while unresolved threads exist.\n\nFix: in `read_threads`, check `decoded.threads_read == false` and `say(..., WARN)` that the threads could not be fetched. Do the same when `PRS_DIFF_THREADS` is set but its file is unreadable. Keep a flag in `state` so `decorate` or the panel can mark the view as incomplete. Add a spec case for each. The spec only covers `threads_read=true`. This follows code-quality.md § \"A result that can come back partial carries the signal that tells it from a whole one\"."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — contracts
Used: I cloned the PR head and datapointchris/prs#9, the producer side of the contract, which is still open. I compared each field pull-request-panel.lua reads against prs's cmd/prs/diff-threads.schema.json, internal/board/diff_files.go and document.go. I cloned diffview.nvim into an XDG data dir inside my directory and ran task test:nvim there: 15 ok, exit 0. I confirmed the DiffviewDiffBufWinEnter and DiffviewViewEnter events exist in diffview's init.lua. I checked <leader>gr and <leader>gR against every existing <leader>g mapping and found no clash. I confirmed nvim-specs is absent from CI's fixed hook list in validate.yml, so CI is unaffected. I ran a headless nvim with PRS_DIFF_DOCUMENT set to show that a child process still sees the variable after setup().
Standards: I searched "environment variable inherited by child processes contract" and "cross-repo contract schema producer consumer" and "keymap mapped only in one mode environment variable marks process". No cli-design.md, api-design.md or configuration.md rule governs an env var handed to a single program or a markdown mention format. testing.md "A stand-in carries the shapes of the real thing" is met, because the spec's threads fixture matches prs's schema, including line: null.
Findings: 0 correctness · 2 design · 0 prose
Design and architecture
contracts.1 configs/common/.config/nvim/lua/core/pull-request-panel.lua:159 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:175 · prs:internal/board/document.go:272
Reach: Any pull request with a LEFT thread and a RIGHT thread on the same path and line number. prs's document sorts unresolved threads first, so the first match is often the wrong thread. I read this from the code and did not run it.
A path:line mention in the document does not say which side of the diff it is on. prs writes each thread heading as ### `path:line` · state (document.go:272-276). The thread's side exists only in the threads file. In nvim, side_of(path, line) returns the side of the first thread whose path and line match, and place_cursor uses that to pick the base window or the head window.
Take two threads on cache/cache.go:14, one LEFT (base line 14) and one RIGHT (head line 14). Both headings read `cache/cache.go:14`. <CR> on either heading lands on whichever thread comes first in the file. So one of the two headings always opens the wrong copy of the file.
The ambiguity is in the format the two repos share, so fixing it touches both. Either fix works:
- prs writes the side into the heading, for example
`path:line` (base), andM.mentionsparses it. - nvim maps each heading to its thread by order. The headings and
threadscome from the same list, though the document re-sorts it unresolved first.
The first fix is cheaper while prs#9 is still open, before anything else depends on the format.
contracts.2 configs/common/.config/nvim/lua/core/pull-request-panel.lua:314 · configs/common/.config/nvim/lua/plugins/auto-session.lua:7 · configs/common/.config/lazygit/config.yml:13
Reach: Any nvim started from inside the diff nvim. Pressing e in lazygit (<leader>gg, editPreset: nvim) starts one, and so does git commit in :terminal or the floaterminal. I ran a headless nvim with PRS_DIFF_DOCUMENT set and called setup(): it returned true, and system('printenv PRS_DIFF_DOCUMENT') still printed the path in the child.
PRS_DIFF_DOCUMENT stays set for every process the diff nvim starts. This change gives the variable a meaning: "this nvim is the one prs opened for a diff". Two places act on it:
auto-session.lua'scondkeeps auto-session unloaded.setup()maps<leader>grand<leader>gR.
setup() reads the variable but never clears it, so every child process inherits it. This config makes nested nvims easy to start: lazygit's editPreset: nvim opens one, and so does a git commit in the floaterminal. Each nested nvim then counts as prs's diff nvim. It loads no auto-session. It maps <leader>gr to the same document. In that nvim, <CR>/gf in the panel reports "No diffview is open to jump in."
Fix: unset both variables in setup() once they are read, with vim.env.PRS_DIFF_DOCUMENT = nil and vim.env.PRS_DIFF_THREADS = nil. Then only the nvim prs started has the meaning. lazy decides auto-session's cond inside require('core.lazy'), which runs before setup() in init.lua, so clearing the variables there does not change that decision.
Findings as JSON
{
"used": "I cloned the PR head and datapointchris/prs#9, the producer side of the contract, which is still open. I compared each field `pull-request-panel.lua` reads against prs's `cmd/prs/diff-threads.schema.json`, `internal/board/diff_files.go` and `document.go`. I cloned diffview.nvim into an XDG data dir inside my directory and ran `task test:nvim` there: 15 ok, exit 0. I confirmed the `DiffviewDiffBufWinEnter` and `DiffviewViewEnter` events exist in diffview's `init.lua`. I checked `<leader>gr` and `<leader>gR` against every existing `<leader>g` mapping and found no clash. I confirmed `nvim-specs` is absent from CI's fixed hook list in validate.yml, so CI is unaffected. I ran a headless nvim with `PRS_DIFF_DOCUMENT` set to show that a child process still sees the variable after `setup()`.",
"standards": "I searched \"environment variable inherited by child processes contract\" and \"cross-repo contract schema producer consumer\" and \"keymap mapped only in one mode environment variable marks process\". No cli-design.md, api-design.md or configuration.md rule governs an env var handed to a single program or a markdown mention format. testing.md \"A stand-in carries the shapes of the real thing\" is met, because the spec's threads fixture matches prs's schema, including `line: null`.",
"findings": [
{
"number": "contracts.1",
"section": "design",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:159",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:175",
"prs:internal/board/document.go:272"
],
"reach": "Any pull request with a LEFT thread and a RIGHT thread on the same path and line number. prs's document sorts unresolved threads first, so the first match is often the wrong thread. I read this from the code and did not run it.",
"body": "**A `path:line` mention in the document does not say which side of the diff it is on.** prs writes each thread heading as `` ### `path:line` · state `` (`document.go:272-276`). The thread's `side` exists only in the threads file. In nvim, `side_of(path, line)` returns the `side` of the first thread whose path and line match, and `place_cursor` uses that to pick the base window or the head window.\n\nTake two threads on `cache/cache.go:14`, one LEFT (base line 14) and one RIGHT (head line 14). Both headings read `` `cache/cache.go:14` ``. `<CR>` on either heading lands on whichever thread comes first in the file. So one of the two headings always opens the wrong copy of the file.\n\nThe ambiguity is in the format the two repos share, so fixing it touches both. Either fix works:\n- prs writes the side into the heading, for example `` `path:line` (base) ``, and `M.mentions` parses it.\n- nvim maps each heading to its thread by order. The headings and `threads` come from the same list, though the document re-sorts it unresolved first.\n\nThe first fix is cheaper while prs#9 is still open, before anything else depends on the format."
},
{
"number": "contracts.2",
"section": "design",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:314",
"configs/common/.config/nvim/lua/plugins/auto-session.lua:7",
"configs/common/.config/lazygit/config.yml:13"
],
"reach": "Any nvim started from inside the diff nvim. Pressing `e` in lazygit (`<leader>gg`, `editPreset: nvim`) starts one, and so does `git commit` in `:terminal` or the floaterminal. I ran a headless nvim with `PRS_DIFF_DOCUMENT` set and called `setup()`: it returned `true`, and `system('printenv PRS_DIFF_DOCUMENT')` still printed the path in the child.",
"body": "**`PRS_DIFF_DOCUMENT` stays set for every process the diff nvim starts.** This change gives the variable a meaning: \"this nvim is the one prs opened for a diff\". Two places act on it:\n- `auto-session.lua`'s `cond` keeps auto-session unloaded.\n- `setup()` maps `<leader>gr` and `<leader>gR`.\n\n`setup()` reads the variable but never clears it, so every child process inherits it. This config makes nested nvims easy to start: lazygit's `editPreset: nvim` opens one, and so does a `git commit` in the floaterminal. Each nested nvim then counts as prs's diff nvim. It loads no auto-session. It maps `<leader>gr` to the same document. In that nvim, `<CR>`/`gf` in the panel reports \"No diffview is open to jump in.\"\n\nFix: unset both variables in `setup()` once they are read, with `vim.env.PRS_DIFF_DOCUMENT = nil` and `vim.env.PRS_DIFF_THREADS = nil`. Then only the nvim prs started has the meaning. lazy decides auto-session's `cond` inside `require('core.lazy')`, which runs before `setup()` in `init.lua`, so clearing the variables there does not change that decision."
}
]
}This review does not gate merge.
datapointchris
left a comment
There was a problem hiding this comment.
PR review — useless-test
Used: I cloned PR 106 at 2561ac6 and ran tests/nvim/pull_request_panel_spec.lua in headless nvim against a copy of diffview.nvim, with every XDG path inside my directory. All 15 checks passed. I then made four mutations to pull-request-panel.lua: I reworded the jump notice, made setup() return nil, deleted the <leader>gR mapping and removed the unknown-path bail. I recorded which checks failed for each mutation and restored the file.
Standards: testing.md § "Test behavior the user cares about, not what this commit happened to change"; pull-requests.md § "Tests the diff adds are checked for whether they test the program or the change"; testing.md § "Never assert on rendered output — assert the value it was built from".
Findings: 0 correctness · 3 design · 0 prose
Design and architecture
useless-test.1 tests/nvim/pull_request_panel_spec.lua:291 · tests/nvim/pull_request_panel_spec.lua:294 · tests/nvim/pull_request_panel_spec.lua:296
Reach: The <CR>/gf mapping on the document buffer calls M.jump. Given a mention of a path the diff does not change, M.jump calls say(named .. ' is not a file this diff changes.') and returns without moving (configs/common/.config/nvim/lua/core/pull-request-panel.lua:202-205). A user presses <CR> on docs/design.md in the panel and reaches it.
Delete the check a path the diff does not change says so and stays on the file shown and replace it. It compares the whole notice sentence byte for byte.
The check asserts said[1] == 'docs/design.md is not a file this diff changes.'. Everything after the path is a literal typed into M.jump (pull-request-panel.lua:204), so that part of the assertion only checks that the literal equals itself. testing.md § "Never assert on rendered output — assert the value it was built from" names an error message as rendered output.
I reworded the literal to ' is not changed in this diff.' without touching any logic. Only this check failed: FAIL a path the diff does not change says so and stays on the file shown.
The check also pins a real behavior that no other check covers. On a path outside the diff, the jump refuses and the diff stays on the file it shows. With the if not path then bail removed, this check is the only one that failed.
Replace it with a check that asserts:
view().cur_entry.path == 'cache/cache.go'after the press;- a notice was raised (
#said == 1); - the notice carries the computed mention:
said[1]:find('docs/design.md', 1, true).
The path is the interpolated value that the mention parser produced. The rule's exception allows asserting it. The sentence around it is then free to change.
useless-test.2 tests/nvim/pull_request_panel_spec.lua:176 · tests/nvim/pull_request_panel_spec.lua:180 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:312-330 · configs/common/.config/nvim/init.lua:23
Reach: The only production caller of M.setup() is init.lua:23, and it discards the return value. That boolean is read by the spec and by nothing else. The <leader>gr/<leader>gR mappings it installs are what a user reaches. The spec already presses them at lines 229, 240, 243 and 246.
Delete the check with PRS_DIFF_DOCUMENT set, setup maps both keys, and drop panel.setup() == false from outside a diff prs opened, setup sets up nothing. Both assert a return value no caller reads.
The second check combines two things:
panel.setup() == true.init.luacallsrequire('core.pull-request-panel').setup()and ignores the result, so this is a value one function hands a caller that throws it away. testing.md § "Test behavior the user cares about…" names that shape.maparg(' gR', 'n') ~= ''. This is a weaker copy of<leader>gR shows the document in a float, which presses the key and checks that a float opened.
I made setup() return nil in both branches. Nothing a user sees changed. Both setup checks failed anyway.
I deleted the <leader>gR mapping. The press-the-key check <leader>gR shows the document in a float failed as well, so the maparg half of check two adds nothing.
In the first check, keep vim.fn.maparg(' gr', 'n') == '' alone. It is the one assertion that pins the behavior the user cares about: an nvim that prs did not start gets no mapping. The setup() call on line 180 stays as plain setup without a check around it.
useless-test.3 tests/nvim/pull_request_panel_spec.lua:196
Reach: :DiffviewOpen main...cache-metrics choosing its first file is diffview.nvim's behavior. No code in core/pull-request-panel takes part in it. The spec uses the wait as setup before reading extmarks at line 198.
Delete the named check the diff opens on cache/cache.go. It tests diffview.nvim, not the panel.
The check runs DiffviewOpen and asserts that diffview selected cache/cache.go and loaded its buffer. Nothing in pull-request-panel.lua runs on that path. M.decorate only hangs off the DiffviewDiffBufWinEnter/DiffviewViewEnter autocmds after the selection. testing.md § "Test behavior the user cares about…" names a dependency's documented behavior as a snapshot shape.
In the four mutations I ran against the panel, this check never failed. It can only fail when diffview or the fixture changes.
The spec still needs the wait. Keep showing('cache/cache.go') as a precondition that aborts with cquit and a FAIL line, the way the fixture's git init guard at line 87 does. It should not be reported as one of the panel's checks. Nothing replaces it as a behavior test. Both virtual-text checks already depend on the diff being open on that file.
Findings as JSON
{
"used": "I cloned PR 106 at 2561ac6c and ran `tests/nvim/pull_request_panel_spec.lua` in headless nvim against a copy of diffview.nvim, with every XDG path inside my directory. All 15 checks passed. I then made four mutations to `pull-request-panel.lua`: I reworded the jump notice, made `setup()` return nil, deleted the `<leader>gR` mapping and removed the unknown-path bail. I recorded which checks failed for each mutation and restored the file.",
"standards": "testing.md § \"Test behavior the user cares about, not what this commit happened to change\"; pull-requests.md § \"Tests the diff adds are checked for whether they test the program or the change\"; testing.md § \"Never assert on rendered output — assert the value it was built from\".",
"findings": [
{
"number": "useless-test.1",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:291",
"tests/nvim/pull_request_panel_spec.lua:294",
"tests/nvim/pull_request_panel_spec.lua:296"
],
"reach": "The `<CR>`/`gf` mapping on the document buffer calls `M.jump`. Given a mention of a path the diff does not change, `M.jump` calls `say(named .. ' is not a file this diff changes.')` and returns without moving (`configs/common/.config/nvim/lua/core/pull-request-panel.lua:202-205`). A user presses `<CR>` on `docs/design.md` in the panel and reaches it.",
"body": "**Delete the check `a path the diff does not change says so and stays on the file shown` and replace it. It compares the whole notice sentence byte for byte.**\n\nThe check asserts `said[1] == 'docs/design.md is not a file this diff changes.'`. Everything after the path is a literal typed into `M.jump` (`pull-request-panel.lua:204`), so that part of the assertion only checks that the literal equals itself. testing.md § \"Never assert on rendered output — assert the value it was built from\" names an error message as rendered output.\n\nI reworded the literal to `' is not changed in this diff.'` without touching any logic. Only this check failed: `FAIL a path the diff does not change says so and stays on the file shown`.\n\nThe check also pins a real behavior that no other check covers. On a path outside the diff, the jump refuses and the diff stays on the file it shows. With the `if not path then` bail removed, this check is the only one that failed.\n\nReplace it with a check that asserts:\n- `view().cur_entry.path == 'cache/cache.go'` after the press;\n- a notice was raised (`#said == 1`);\n- the notice carries the computed mention: `said[1]:find('docs/design.md', 1, true)`.\n\nThe path is the interpolated value that the mention parser produced. The rule's exception allows asserting it. The sentence around it is then free to change."
},
{
"number": "useless-test.2",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:176",
"tests/nvim/pull_request_panel_spec.lua:180",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:312-330",
"configs/common/.config/nvim/init.lua:23"
],
"reach": "The only production caller of `M.setup()` is `init.lua:23`, and it discards the return value. That boolean is read by the spec and by nothing else. The `<leader>gr`/`<leader>gR` mappings it installs are what a user reaches. The spec already presses them at lines 229, 240, 243 and 246.",
"body": "**Delete the check `with PRS_DIFF_DOCUMENT set, setup maps both keys`, and drop `panel.setup() == false` from `outside a diff prs opened, setup sets up nothing`. Both assert a return value no caller reads.**\n\nThe second check combines two things:\n- `panel.setup() == true`. `init.lua` calls `require('core.pull-request-panel').setup()` and ignores the result, so this is a value one function hands a caller that throws it away. testing.md § \"Test behavior the user cares about…\" names that shape.\n- `maparg(' gR', 'n') ~= ''`. This is a weaker copy of `<leader>gR shows the document in a float`, which presses the key and checks that a float opened.\n\nI made `setup()` return nil in both branches. Nothing a user sees changed. Both setup checks failed anyway.\n\nI deleted the `<leader>gR` mapping. The press-the-key check `<leader>gR shows the document in a float` failed as well, so the `maparg` half of check two adds nothing.\n\nIn the first check, keep `vim.fn.maparg(' gr', 'n') == ''` alone. It is the one assertion that pins the behavior the user cares about: an nvim that prs did not start gets no mapping. The `setup()` call on line 180 stays as plain setup without a `check` around it."
},
{
"number": "useless-test.3",
"section": "design",
"sites": [
"tests/nvim/pull_request_panel_spec.lua:196"
],
"reach": "`:DiffviewOpen main...cache-metrics` choosing its first file is diffview.nvim's behavior. No code in `core/pull-request-panel` takes part in it. The spec uses the wait as setup before reading extmarks at line 198.",
"body": "**Delete the named check `the diff opens on cache/cache.go`. It tests diffview.nvim, not the panel.**\n\nThe check runs `DiffviewOpen` and asserts that diffview selected `cache/cache.go` and loaded its buffer. Nothing in `pull-request-panel.lua` runs on that path. `M.decorate` only hangs off the `DiffviewDiffBufWinEnter`/`DiffviewViewEnter` autocmds after the selection. testing.md § \"Test behavior the user cares about…\" names a dependency's documented behavior as a snapshot shape.\n\nIn the four mutations I ran against the panel, this check never failed. It can only fail when diffview or the fixture changes.\n\nThe spec still needs the wait. Keep `showing('cache/cache.go')` as a precondition that aborts with `cquit` and a `FAIL` line, the way the fixture's `git init` guard at line 87 does. It should not be reported as one of the panel's checks. Nothing replaces it as a behavior test. Both virtual-text checks already depend on the diff being open on that file."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — G1
Used: Ran the PR's spec under nvim --headless -u NONE (v0.10.4) with diffview.nvim cloned into an XDG_DATA_HOME inside my directory: 15/15 pass. Then ran scratch scripts against scratch git repos: moving between files with diffview's next_file, a renamed file's LEFT thread, <CR> on a comment body, a blank line and an author line, and a root main.go beside cmd/a/main.go and cmd/b/main.go. Also read prs PR #9's threads schema, diffFiles.write and threadsOf to see what nvim is handed.
Standards: The diff follows lua.md § "A plugin's Lua is tested in a headless Neovim that the commit hook runs". ci.md § "CI runs the repo's pre-commit hooks, never copies of their checks" leaves custom-section hooks like nvim-specs out of CI by design. pull-requests.md § "Sentences the diff adds are checked for what goes stale" found no stale sentence.
Findings: 2 correctness · 0 design · 0 prose
Correctness
G1.1 configs/common/.config/nvim/lua/core/pull-request-panel.lua:143 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:150
Reach: A pull request that changes a root file and two or more nested files with the same name, where diffview lists the nested ones first. Examples: main.go with cmd/a/main.go and cmd/b/main.go, or go.mod and pyproject.toml beside per-module copies. The reader presses <CR> on the root file's row in the Files table, or on its ### path:line`` thread heading. prs's threadsOf writes exactly those headings.
resolve refuses an exact path when two longer paths end in the same name and are listed first. The loop returns nil at the second suffix match. It returns before it reaches the file whose path equals the mention.
I ran a scratch repo whose diff changes main.go, cmd/a/main.go and cmd/b/main.go. diffview iterates them as cmd/a/main.go, cmd/b/main.go, main.go. <CR> on | `main.go` | modified | and on ### `main.go:2` · unresolved each said main.go is not a file this diff changes. The diff stayed on cmd/a/main.go. The same document with README.md, docs/README.md and tests/README.md worked, only because uppercase R sorts the root file first.
Fix: walk every file before deciding. Return the exact match wherever it appears. Return the suffix match only after the loop, and only where exactly one file had one. A spec check with a root main.go and two nested main.go files pins it.
G1.2 configs/common/.config/nvim/lua/core/pull-request-panel.lua:202 · configs/common/.config/nvim/lua/core/pull-request-panel.lua:111
Reach: A reader in the panel's Review threads section presses <CR> or gf on a thread's comment lines or a blank line. prs's threadsOf writes an author line and an indented body under each ### heading, so most lines in that section are these.
<CR> anywhere in a thread except its heading warns with an arbitrary word, and on a blank line with a broken sentence. M.jump reports mentions[1] as "not a file this diff changes". That mention is whatever word sits under the cursor.
I ran a document shaped like prs's, with a ### a.txt:14`` heading, an author line and a comment body. Pressing jump on each line returned:
- body
This expiry reads the wall clock.→expiry is not a file this diff changes. - author line
**mona** · 3h ago→mona is not a file this diff changes. - blank line →
Nothing under the cursor is not a file this diff changes.
A reader tracking a comment presses <CR> while reading it. The body says the heading jumps "from anywhere on it", but none of the comment's own lines do.
Fix: where the line names no file the diff changes, take the nearest ### heading above the cursor, which is the thread's own path:line. Where nothing resolves, say No file this diff changes is named on this line. rather than naming a word that was never a path.
Findings as JSON
{
"used": "Ran the PR's spec under `nvim --headless -u NONE` (v0.10.4) with diffview.nvim cloned into an XDG_DATA_HOME inside my directory: 15/15 pass. Then ran scratch scripts against scratch git repos: moving between files with diffview's next_file, a renamed file's LEFT thread, `<CR>` on a comment body, a blank line and an author line, and a root `main.go` beside `cmd/a/main.go` and `cmd/b/main.go`. Also read prs PR #9's threads schema, `diffFiles.write` and `threadsOf` to see what nvim is handed.",
"standards": "The diff follows `lua.md` § \"A plugin's Lua is tested in a headless Neovim that the commit hook runs\". `ci.md` § \"CI runs the repo's pre-commit hooks, never copies of their checks\" leaves custom-section hooks like `nvim-specs` out of CI by design. `pull-requests.md` § \"Sentences the diff adds are checked for what goes stale\" found no stale sentence.",
"findings": [
{
"number": "G1.1",
"section": "correctness",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:143",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:150"
],
"reach": "A pull request that changes a root file and two or more nested files with the same name, where diffview lists the nested ones first. Examples: `main.go` with `cmd/a/main.go` and `cmd/b/main.go`, or `go.mod` and `pyproject.toml` beside per-module copies. The reader presses `<CR>` on the root file's row in the Files table, or on its `### `path:line`` thread heading. prs's `threadsOf` writes exactly those headings.",
"body": "**`resolve` refuses an exact path when two longer paths end in the same name and are listed first.** The loop returns `nil` at the second suffix match. It returns before it reaches the file whose path equals the mention.\n\nI ran a scratch repo whose diff changes `main.go`, `cmd/a/main.go` and `cmd/b/main.go`. diffview iterates them as `cmd/a/main.go, cmd/b/main.go, main.go`. `<CR>` on `` | `main.go` | modified | `` and on `` ### `main.go:2` · unresolved `` each said `main.go is not a file this diff changes.` The diff stayed on `cmd/a/main.go`. The same document with `README.md`, `docs/README.md` and `tests/README.md` worked, only because uppercase `R` sorts the root file first.\n\nFix: walk every file before deciding. Return the exact match wherever it appears. Return the suffix match only after the loop, and only where exactly one file had one. A spec check with a root `main.go` and two nested `main.go` files pins it."
},
{
"number": "G1.2",
"section": "correctness",
"sites": [
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:202",
"configs/common/.config/nvim/lua/core/pull-request-panel.lua:111"
],
"reach": "A reader in the panel's Review threads section presses `<CR>` or `gf` on a thread's comment lines or a blank line. prs's `threadsOf` writes an author line and an indented body under each `###` heading, so most lines in that section are these.",
"body": "**`<CR>` anywhere in a thread except its heading warns with an arbitrary word, and on a blank line with a broken sentence.** `M.jump` reports `mentions[1]` as \"not a file this diff changes\". That mention is whatever word sits under the cursor.\n\nI ran a document shaped like prs's, with a `### `a.txt:14`` heading, an author line and a comment body. Pressing jump on each line returned:\n- body ` This expiry reads the wall clock.` → `expiry is not a file this diff changes.`\n- author line `**mona** · 3h ago` → `mona is not a file this diff changes.`\n- blank line → `Nothing under the cursor is not a file this diff changes.`\n\nA reader tracking a comment presses `<CR>` while reading it. The body says the heading jumps \"from anywhere on it\", but none of the comment's own lines do.\n\nFix: where the line names no file the diff changes, take the nearest `### ` heading above the cursor, which is the thread's own `path:line`. Where nothing resolves, say `No file this diff changes is named on this line.` rather than naming a word that was never a path."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — synthesis
Used: the 16 findings of round 1, from 8 review(s), and nothing else
Standards: none, because this step reads only the findings
Findings: 0 correctness · 4 design · 0 prose
Design and architecture
synthesis.1 explains code-architecture.2 · fleet-patterns.1 · fleet-patterns.2
The spec hand-copies facts that another owner defines, with nothing checking the copy. The git variable list copies tests/conftest.py and ignores what git rev-parse --local-env-vars returns. The prs document and threads fixtures are hand-written, not read from prs output or its schema.
synthesis.2 explains code-architecture.1 · contracts.2
The prs-diff-nvim predicate on PRS_DIFF_DOCUMENT is decided at several sites. auto-session.lua, setup() and the inherited child environment each treat it separately. No single owner exists, so they can disagree or leak to nested nvims.
synthesis.3 explains stale-prose.1 · stale-prose.2 · stale-prose.3
Comments in the panel module and spec state prs's files, keys and variable gating as fact. They can be false or go stale, and nothing in this repo checks them.
synthesis.4 explains G1.1 · G1.2
M.jump and resolve handle path lookup badly in the panel. The first resolves the wrong thing or refuses an exact path. The second names a stray word for lines that name no file.
One defect, raised more than once
code-architecture.2 · fleet-patterns.1: The spec's hard-coded eight-name git variable list duplicates tests/conftest.py and omits names that git rev-parse --local-env-vars returns.
Shapes as JSON
{
"shapes": [
{
"number": "synthesis.1",
"section": "design",
"explains": [
"code-architecture.2",
"fleet-patterns.1",
"fleet-patterns.2"
],
"body": "The spec hand-copies facts that another owner defines, with nothing checking the copy. The git variable list copies `tests/conftest.py` and ignores what `git rev-parse --local-env-vars` returns. The prs document and threads fixtures are hand-written, not read from prs output or its schema."
},
{
"number": "synthesis.2",
"section": "design",
"explains": [
"code-architecture.1",
"contracts.2"
],
"body": "The prs-diff-nvim predicate on `PRS_DIFF_DOCUMENT` is decided at several sites. `auto-session.lua`, `setup()` and the inherited child environment each treat it separately. No single owner exists, so they can disagree or leak to nested nvims."
},
{
"number": "synthesis.3",
"section": "design",
"explains": [
"stale-prose.1",
"stale-prose.2",
"stale-prose.3"
],
"body": "Comments in the panel module and spec state prs's files, keys and variable gating as fact. They can be false or go stale, and nothing in this repo checks them."
},
{
"number": "synthesis.4",
"section": "design",
"explains": [
"G1.1",
"G1.2"
],
"body": "`M.jump` and `resolve` handle path lookup badly in the panel. The first resolves the wrong thing or refuses an exact path. The second names a stray word for lines that name no file."
}
],
"duplicates": [
{
"defect": "The spec's hard-coded eight-name git variable list duplicates `tests/conftest.py` and omits names that `git rev-parse --local-env-vars` returns.",
"findings": [
"code-architecture.2",
"fleet-patterns.1"
]
}
]
}This review does not gate merge.
PR review round 1Ran: G1, useless-test, code-architecture, fleet-patterns, system-design, stale-prose, failure-paths, contracts, synthesis
Round record{
"grammar": 6,
"round": 1,
"repo": "datapointchris/dotfiles",
"pr": 106,
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"experiment": "none",
"dispatch": "20261108T063038Z-pr-review",
"roster": "1ee22e0694913fcfb159f68268741c1851bff2a5",
"harness": "ef03d584c604b56cf3bed068b781233667e9512e",
"machine": "queue",
"started": "2026-11-08T06:32:13Z",
"finished": "2026-11-08T06:39:42Z",
"reviewers": [
{
"label": "G1",
"agent": "pr-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/general.md",
"prompt_sha": "a0f85e3b82b4",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 349,
"tokens": {
"input": 56,
"cache_write": 70573,
"cache_read": 1351575,
"output": 24632
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 56,
"cache_write": 70573,
"cache_read": 1351575,
"output": 24632
}
},
"session": "01a1b9db-3a56-7d25-967f-9c245fe9d1f4",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816923639",
"review_id": 5816923639,
"findings": {
"correctness": 2,
"design": 0,
"prose": 0
},
"numbers": [
"G1.1",
"G1.2"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:38Z",
"finished": "2026-11-08T06:38:32Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "useless-test",
"agent": "pr-useless-test-reviewer",
"model": "opus",
"effort": "high",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/useless-test.md",
"prompt_sha": "3700d72236e3",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 72,
"tokens": {
"input": 14,
"cache_write": 25943,
"cache_read": 146935,
"output": 6070
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 14,
"cache_write": 25943,
"cache_read": 146935,
"output": 6070
}
},
"session": "01a1b9dd-9ad9-73ae-9921-8ddad7575a4b",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816917709",
"review_id": 5816917709,
"findings": {
"correctness": 0,
"design": 3,
"prose": 0
},
"numbers": [
"useless-test.1",
"useless-test.2",
"useless-test.3"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:34:37Z",
"finished": "2026-11-08T06:35:52Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "code-architecture",
"agent": "pr-code-architecture-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "d9be86eb4324",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 111,
"tokens": {
"input": 30,
"cache_write": 28353,
"cache_read": 398788,
"output": 7233
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 30,
"cache_write": 28353,
"cache_read": 398788,
"output": 7233
}
},
"session": "01a1b9da-ed2d-7c74-9bec-73d958739dad",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816913939",
"review_id": 5816913939,
"findings": {
"correctness": 0,
"design": 2,
"prose": 0
},
"numbers": [
"code-architecture.1",
"code-architecture.2"
],
"rules": {
"proposed": 1,
"keys": [
"rule/lua/a-lazynvim-specs-own-cond-replaces-defaultscond-so-every-load-gate-lives-in-the-function-defaultscond-names"
],
"filed": true,
"why": "",
"path": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records/code-architecture.rules.json"
},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:14Z",
"finished": "2026-11-08T06:34:12Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "fleet-patterns",
"agent": "pr-fleet-patterns-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "eae343d1d8d5",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 131,
"tokens": {
"input": 36,
"cache_write": 35897,
"cache_read": 481593,
"output": 7376
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 36,
"cache_write": 35897,
"cache_read": 481593,
"output": 7376
}
},
"session": "01a1b9db-4074-7d78-9345-1b44e93d7556",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816915764",
"review_id": 5816915764,
"findings": {
"correctness": 0,
"design": 2,
"prose": 0
},
"numbers": [
"fleet-patterns.1",
"fleet-patterns.2"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:38Z",
"finished": "2026-11-08T06:34:58Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "system-design",
"agent": "pr-system-design-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "c284e39c894a",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 54,
"tokens": {
"input": 16,
"cache_write": 17022,
"cache_read": 152197,
"output": 2652
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 16,
"cache_write": 17022,
"cache_read": 152197,
"output": 2652
}
},
"session": "01a1b9da-e524-78e7-93c8-80d1dd41f1a6",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816911716",
"review_id": 5816911716,
"findings": {
"correctness": 1,
"design": 0,
"prose": 0
},
"numbers": [
"system-design.1"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:14Z",
"finished": "2026-11-08T06:33:14Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "stale-prose",
"agent": "pr-stale-prose-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "50506a05f936",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 98,
"tokens": {
"input": 20,
"cache_write": 28754,
"cache_read": 258360,
"output": 6385
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 20,
"cache_write": 28754,
"cache_read": 258360,
"output": 6385
}
},
"session": "01a1b9da-e903-7e22-9ea8-f8c638123d2f",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816913376",
"review_id": 5816913376,
"findings": {
"correctness": 0,
"design": 0,
"prose": 3
},
"numbers": [
"stale-prose.1",
"stale-prose.2",
"stale-prose.3"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:13Z",
"finished": "2026-11-08T06:33:57Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "failure-paths",
"agent": "pr-failure-paths-reviewer",
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "56bdd640764b",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 36,
"tokens": {
"input": 12,
"cache_write": 23079,
"cache_read": 94751,
"output": 2834
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 12,
"cache_write": 23079,
"cache_read": 94751,
"output": 2834
}
},
"session": "01a1b9dd-9bb6-7b36-a74c-ec30ac09c093",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816916337",
"review_id": 5816916337,
"findings": {
"correctness": 1,
"design": 0,
"prose": 0
},
"numbers": [
"failure-paths.1"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:34:37Z",
"finished": "2026-11-08T06:35:16Z",
"account": "live",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "contracts",
"agent": "pr-contracts-reviewer",
"model": "opus",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/persona.md",
"prompt_sha": "88b65626bceb",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 163,
"tokens": {
"input": 40,
"cache_write": 41271,
"cache_read": 628989,
"output": 9824
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 40,
"cache_write": 41271,
"cache_read": 628989,
"output": 9824
}
},
"session": "01a1b9db-376e-7983-b891-5d9819411df1",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816916701",
"review_id": 5816916701,
"findings": {
"correctness": 0,
"design": 2,
"prose": 0
},
"numbers": [
"contracts.1",
"contracts.2"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:32:38Z",
"finished": "2026-11-08T06:35:27Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"label": "synthesis",
"agent": null,
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/synthesis.md",
"prompt_sha": "40dcb5a50df0",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 8,
"tokens": {
"input": 2,
"cache_write": 13490,
"cache_read": 0,
"output": 589
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 2,
"cache_write": 13490,
"cache_read": 0,
"output": 589
}
},
"session": "01a1b9e1-7690-77b6-bd58-061fba33d6cf",
"worklog": "1326.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/dotfiles/pull/106#pullrequestreview-5816927165",
"review_id": 5816927165,
"findings": {
"correctness": 0,
"design": 4,
"prose": 0
},
"numbers": [
"synthesis.1",
"synthesis.2",
"synthesis.3",
"synthesis.4"
],
"rules": {},
"head": "2561ac6cff5608b9f18898771232ae2799a4c153",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-dotfiles-pr106-round1-20261108T063038Z/records",
"started": "2026-11-08T06:39:27Z",
"finished": "2026-11-08T06:39:42Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
}
],
"rules": {
"proposed": 1,
"keys": [
"rule/lua/a-lazynvim-specs-own-cond-replaces-defaultscond-so-every-load-gate-lives-in-the-function-defaultscond-names"
],
"unfiled": []
},
"kept": []
} |
… from any thread line core.profiles reads PRS_DIFF_DOCUMENT and PRS_DIFF_THREADS once and clears them, so an nvim started inside the diff, such as lazygit's editor, is an ordinary one. Its plugin_enabled leaves auto-session out of a prs diff. A cond on the plugin's own spec replaced lazy's default cond, which let auto-session load inside VSCode. The panel takes the file a mention names exactly before any file whose path ends in it, so a root main.go beside cmd/a/main.go and cmd/b/main.go selects itself. A line naming no changed file takes the heading above it, so each line of a thread's comments jumps to the thread's line. A refused jump names only a mention shaped like a path. A threads file that is missing, unreadable or written before prs read the threads now warns that none show on the diff. The spec passes with SKIP where lazy has not installed diffview.nvim, and clears the repository variables git rev-parse --local-env-vars lists. Agent-Name: claude-code-unattended Agent-Session: 6ab7d888-63b1-4910-ab41-c89e8e170c7a Agent-Run: 01a1b9da-2b49-72dc-aa5e-712514e0b4ee
…f eight git exports its repository variables to a hook, and a test that builds a throwaway repo under one acts on the repo being committed. The hand-kept list named eight and missed seven git prints, among them GIT_CONFIG_PARAMETERS and GIT_IMPLICIT_WORK_TREE. git rev-parse --local-env-vars is git's own list. Agent-Name: claude-code-unattended Agent-Session: 6ab7d888-63b1-4910-ab41-c89e8e170c7a Agent-Run: 01a1b9da-2b49-72dc-aa5e-712514e0b4ee Worklog: 1326.1
A diff prs opens in nvim shows the code and nothing of the pull request. A reviewer leaves nvim to read the description and the review threads, and cannot see which comment sits on the line in front of them. datapointchris/prs#9 starts that nvim with
PRS_DIFF_DOCUMENTnaming the pull request's markdown document andPRS_DIFF_THREADSnaming its review threads.core/profilesreads and clears both, andcore/pull-request-panelshows the pull request beside the diff. Every other nvim is untouched, because without the first variable nothing is set up.auto-session stays unloaded in that nvim. Before that, closing a diff saved the diff over the repo's own session.
Opened by Claude Code session 6ab7d888-63b1-4910-ab41-c89e8e170c7a.
What to look at
configs/common/.config/nvim/lua/core/pull-request-panel.lua, in4fc0cdeeand568d8cfb:M.decorateputs each thread on the shown entry's files, matched by the entry's path,LEFTonaandRIGHTonb. Check that the head's path is the right key for a renamed file's base side.mentions,resolveandM.jumpdecide what a mention is: the word under the cursor, then each backticked span on the line, then the nearest heading above. A path the diff lists wins over a file whose path ends in the mention, and an ending counts only where one file alone has it.place_cursorruns after awaitingset_file_by_path's Future insidediffview.async. It moves the cursor inview.cur_layout's window, because an entry's own layout holds its files but no window ids.read_threadswarns for each way the diff ends up with no threads to show.configs/common/.config/nvim/lua/core/profiles.lua, in568d8cfb:pull_request_diffis read once when the module loads, and both variables are cleared.plugin_enabled, lazy's defaultcond, leaves auto-session out.init.luarequirescore.profilesbeforecore.lazy, so the decision is made before any plugin loads.tests/nvim/pull_request_panel_spec.lua, thetest:nvimtask and thenvim-specshook.tests/conftest.py, in3c6c2c7f:GIT_LOCATIONcomes from git rather than a hand-kept list.2561ac6cgated auto-session with acondon its own spec, and568d8cfbmoves that gate intocore/profiles.How it was verified
task test:nvimpasses 16 checks innvim --headless -u NONE, against a real git repo the spec builds and the diffview.nvim lazy installed. Thenvim-specshook ran on every commit.Four mutations in one run each failed their own check and no other:
core/profilesleaving the variables set faileda program the diff nvim starts is an ordinary one, with no PRS_DIFF_DOCUMENT.threads_readwarning faileda diff opened before prs read the review threads warns that none show.resolvegiving up at the second file ending in the mention faileda root file selects itself where nested files share its name.a RIGHT thread's comment puts the cursor on its heading's line of the head's copy.Swapping
LEFTandRIGHTinM.decoratefailed both virtual-text checks.With an empty data directory the spec prints
SKIP diffview.nvim is not at /tmp/tmp.zaOXz6biJC/nvim/lazy/diffview.nvim, where lazy installs it the first time nvim startsand exits 0.Under the full config, with
XDG_CONFIG_HOMEpointing at this tree, started headless from an empty directory:prssetPRS_DIFF_DOCUMENT, andvscodestarted with--cmd 'let g:vscode=1'. Space is the leader there, the thread lands on row 13 of the head's copy, and the panel opens as markdown with treesitter highlighting anddiffoff.auto-session, in a real terminal on a private tmux server, because it skips headless runs. Before the change, a diff opened as prs opens it rewrote the scratch repo's saved session to edit a
diffview://buffer. With auto-session unloaded the session file stays byte-identical.The spec's first run under the commit hook acted on this repo. git exports
GIT_DIRandGIT_INDEX_FILEto a hook, and the spec'sgit initand commits followed them. The spec now clears every namegit rev-parse --local-env-varsprints, and refuses to write to a repogit initmade anywhere but where it asked. WithGIT_DIR,GIT_INDEX_FILEandGIT_WORK_TREEnaming a decoy repo:pytest tests/apps/test_agent_commit_trailers.py tests/apps/test_worktree.py, whose fixtures build repos, passed 163 and left the decoy unchanged.What changes
<leader>grshows the pull request in a split at the right edge, and hides it.<leader>gRshows it in a float, and hides it.<CR>orgfon apathorpath:linemention selects that file in the diff and puts the cursor on the line. On a line naming no changed file, such as a thread's comment, it takes the heading above. A jump from the float closes it.<leader>wrthere reports thatSessionRestoreis not a command.configs/common/.config/nvim/ortests/nvim/runstask test:nvim. Where nvim has never started, so lazy installed no diffview.nvim, the spec prints SKIP and passes.git rev-parse --local-env-varslists. It cleared eight before, and missedGIT_CONFIG_PARAMETERS, which a hook undergit -c … commitinherits.Decisions, and what they rejected
<leader>grand<leader>gR, free in the Git group and mapped only in a prs diff.<CR>andgfboth jump.gfis nvim's own key for the file under the cursor, and<CR>is the one a reader presses on a link.core/profilesdecides whether this nvim is a prs diff, and reads the variables once and clears them. The panel and the plugin set then read one answer, and a nested nvim does not inherit it.plugin_enabled, lazy's defaultcond. Rejected: acondon auto-session's own spec, which replaces the defaultcondand so let auto-session load inside VSCode. Rejected: closing the panel onVimLeavePre. auto-session registers its own handler first, so the session is saved before the panel closes, and the diff's buffers are saved either way.:vsplit $PRS_DIFF_DOCUMENT. It cannot select a file in the diff or place a thread on its line, which is what a reviewer tracking a comment needs.What this does not do
LEFTand aRIGHTthread on one file at the same line number share a heading,`path:line`. A jump from either lands on the side of whichever the threads file lists first.The review
#106 (comment)
system-design.1fixed —568d8cfb. Where lazy has not installed diffview.nvim, the spec prints SKIP and exits 0, so a commit on a machine where nvim never started goes through. It stays out of CI's hook list, where it could only skip.stale-prose.1fixed —568d8cfb. The header sayssetup()does nothing wherePRS_DIFF_DOCUMENTis unset.stale-prose.2fixed —568d8cfb. The header drops prs's key and names the fields this module reads from the threads JSON. The spec says its files are shaped like the ones prs writes.stale-prose.3fixed — same asfleet-patterns.1.code-architecture.1fixed —568d8cfb.core/profilesowns "this nvim is a prs diff", andplugin_enabledleaves auto-session out. Thecondon auto-session's spec had also let it load in VSCode, and the probe above shows it unloaded there.code-architecture.2fixed — same asfleet-patterns.1.fleet-patterns.1fixed —568d8cfbfor the spec,3c6c2c7ffortests/conftest.py. Both clear whatgit rev-parse --local-env-varsprints.fleet-patterns.2rejected — reading prs's own test output means locating prs's checkout, and a machine committing to dotfiles need not have prs cloned. A vendored schema or golden file is a second copy nothing refreshes, the same as the fixture it would check. prs's own test holds what it writes to its schema, and the panel's header names the fields it reads.failure-paths.1fixed —568d8cfb.read_threadswarns where prs named no threads file, the file is gone, it does not parse, or prs wrote it before reading the threads. A spec check covers the last, which a reader reaches by pressingdbefore prs's detail read lands. The warning reaches fidget through the startup queue, so no state flag marks the view.contracts.1dropped — telling the two threads apart needs the side in prs's thread headings, and prs's half of this pair is kept to the handover and the file write. The case needs a comment on a removed line and another on a head line with the same number, in one file. "What this does not do" names it.contracts.2fixed —568d8cfb.core/profilesreads both variables once and clears them. A spec check runsprintenv PRS_DIFF_DOCUMENTin a child.useless-test.1fixed —568d8cfb. The check asserts one notice namingdocs/design.mdand an unchanged entry, rather than the sentence.useless-test.2fixed —568d8cfb.setup()returns nothing, both return-value checks are gone, and the outside-a-diff check asserts only that<leader>gris unmapped.useless-test.3fixed —568d8cfb. Opening the diff is a precondition that stops the spec, not one of its checks.G1.1fixed —568d8cfb.resolvewalks every file, returns an exact match wherever diffview lists it, and an ending only where one file alone has it. A spec check selects a rootmain.gobesidecmd/a/main.goandcmd/b/main.go.G1.2fixed —568d8cfb. A line naming no changed file takes the nearest heading above, so a comment line jumps to its thread's line. A refusal names a mention only where it is shaped like a path. A spec check presses<CR>on a comment's second line.synthesis.1fixed — same asfleet-patterns.1, for the git list. Its fixture half isfleet-patterns.2, rejected above.synthesis.2fixed — same ascode-architecture.1.synthesis.3fixed — same asstale-prose.1,stale-prose.2andstale-prose.3.synthesis.4fixed — same asG1.1andG1.2.