Repository navigation
feat: open a menu of fixes on a pull request GitHub will not merge - #17
Conversation
![datapoints-fleet-automation[bot]](https://avatars.githubusercontent.com/u/49924492?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR review — system-design
Used: Cloned PR 17 at its head. Read merge_worktree.go, the fix_menu.go resolve and push paths, fetch.go, and the GitHub listing fields. Ran no code against the change.
Standards: No standards cited. I ran no fleet standards search, because the one finding is a direct trust-boundary defect.
Findings: 1 correctness · 0 design · 0 prose
Correctness
system-design.1 internal/pullrequests/merge_worktree.go:168 · internal/board/fix_menu.go:181 · internal/pullrequests/github.go:67
Reach: A user with write access to the base repo opens a fork pull request that GitHub marks DIRTY. The fork's head branch is named main, which is the default for many forks. The user presses f, picks the merge fix, resolves, and confirms the push. git push origin HEAD:refs/heads/main then runs against the base repo. The merge commit contains origin/main, so the push is a fast-forward and it succeeds. If branch protection is off, the fork's unreviewed code lands on upstream main. Where the fork's branch is named feature, the push creates a stray feature branch on the base repo instead, and the pull request is not updated.
The push target ignores which repo owns the head branch.
CommitAndPush pushes to origin as HEAD:refs/heads/<pr.Branch>. pr.Branch comes from headRefName. The head is fetched via the provider's Head(pr) ref, which is the pull request's own head ref, so it works for forks. The push does not follow it.
The code never reads isCrossRepository, headRepository or any fork flag. I searched the tree for fork, isCrossRepository and headRepository. Nothing matches outside this diff.
The contract on the far side is that headRefName names a branch in the head repo. For a fork, that repo is not origin. The fix menu opens for every row blockerOf names, so fork pull requests reach it.
Fix: Read isCrossRepository and the head repository in the GitHub listing and carry them on PullRequest. For a cross-repository pull request, do not offer the commit-and-push step. Leave the merge in the worktree and say so. Or push to the head repository's URL, and only where maintainerCanModify allows it. Bitbucket needs the same check.
Findings as JSON
{
"used": "Cloned PR 17 at its head. Read merge_worktree.go, the fix_menu.go resolve and push paths, fetch.go, and the GitHub listing fields. Ran no code against the change.",
"standards": "No standards cited. I ran no `fleet standards search`, because the one finding is a direct trust-boundary defect.",
"findings": [
{
"number": "system-design.1",
"section": "correctness",
"sites": [
"internal/pullrequests/merge_worktree.go:168",
"internal/board/fix_menu.go:181",
"internal/pullrequests/github.go:67"
],
"reach": "A user with write access to the base repo opens a fork pull request that GitHub marks DIRTY. The fork's head branch is named `main`, which is the default for many forks. The user presses `f`, picks the merge fix, resolves, and confirms the push. `git push origin HEAD:refs/heads/main` then runs against the base repo. The merge commit contains `origin/main`, so the push is a fast-forward and it succeeds. If branch protection is off, the fork's unreviewed code lands on upstream `main`. Where the fork's branch is named `feature`, the push creates a stray `feature` branch on the base repo instead, and the pull request is not updated.",
"body": "**The push target ignores which repo owns the head branch.**\n\n`CommitAndPush` pushes to `origin` as `HEAD:refs/heads/<pr.Branch>`. `pr.Branch` comes from `headRefName`. The head is fetched via the provider's `Head(pr)` ref, which is the pull request's own head ref, so it works for forks. The push does not follow it.\n\nThe code never reads `isCrossRepository`, `headRepository` or any fork flag. I searched the tree for `fork`, `isCrossRepository` and `headRepository`. Nothing matches outside this diff.\n\nThe contract on the far side is that `headRefName` names a branch in the head repo. For a fork, that repo is not `origin`. The fix menu opens for every row `blockerOf` names, so fork pull requests reach it.\n\n**Fix:** Read `isCrossRepository` and the head repository in the GitHub listing and carry them on `PullRequest`. For a cross-repository pull request, do not offer the commit-and-push step. Leave the merge in the worktree and say so. Or push to the head repository's URL, and only where `maintainerCanModify` allows it. Bitbucket needs the same check."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — failure-paths
Used: Read the diff, fix_menu.go and merge_worktree.go at head. Traced each git, gh and tmux call to its failure outcome. I could not run the push-failure repro, because a hook blocks git merge in this sandbox.
Standards: Searched fleet standards search for "push fails after commit leaves local state, error message promises recovery". The nearest hit was help.md "An error message names what failed, what was expected, and the command that fixes it". No code-quality.md rule on partial results came up.
Findings: 2 correctness · 0 design · 0 prose
Correctness
failure-paths.1 internal/pullrequests/merge_worktree.go:163 · internal/pullrequests/merge_worktree.go:78 · internal/board/fix_menu.go:604
Reach: A push to the head branch is refused after the merge commit is made. Cause: someone else pushed to the branch since the fetch, or a hook or network failure. The user then presses f, then n. Traced only. I could not run it, because a hook blocks git merge here.
A failed push throws away the user's conflict resolution, and the message says it will be reopened.
CommitAndPush runs git commit first and git push second. When the push fails, the commit already exists. MERGE_HEAD is gone, because the commit consumed it.
commitAndPush's failed handler (fix_menu.go:604) then says "the merge waits in , and f, then n, opens it again".
The recovery path does not match that. f then n calls Start, which finds the worktree registered, with no merge under way and a clean git status. It takes the registered branch, which runs git checkout --detach <fresh head>. That discards the resolved merge commit, which is now unreferenced. Start then re-merges from scratch and the user resolves the conflicts again.
This is most likely when the push is refused because someone pushed to the branch meanwhile. The message promises the resolved state and the code deletes it without telling the user.
conflictsOf also reports waiting as false in this state, because Merging checks MERGE_HEAD. The menu therefore never shows a pending merge.
Fix: on push failure, say the merge is committed locally and unpushed, and offer to retry the push. Alternatively make Start detect a worktree whose HEAD is ahead of the head, and refuse or offer a push rather than resetting it. Alternatively git reset --soft HEAD^ after a failed push, to restore the merge-in-progress state the message describes.
failure-paths.2 internal/board/fix_menu.go:521 · internal/board/fix_menu.go:511
Reach: A pull request has two or more failing GitHub Actions runs, and a later gh run rerun fails, for example from a rate limit, a permission error, or a run that is still in progress. Traced only.
A rerun that fails partway reports one error and does not say which runs were rerun.
rerunFailed loops over the runs and returns on the first failure. Earlier runs were already rerun on GitHub. The failed handler is the default, so it shows only the single run's error.
The user does not learn that runs 1..k-1 were restarted. The checks column changes after the next read with no explanation. A retry via r re-reruns the earlier runs.
Fix: keep going through the remaining runs. Collect the failures per run. Report which runs were rerun and which were not. A failure should cost only the run that failed.
Findings as JSON
{
"used": "Read the diff, fix_menu.go and merge_worktree.go at head. Traced each git, gh and tmux call to its failure outcome. I could not run the push-failure repro, because a hook blocks `git merge` in this sandbox.",
"standards": "Searched `fleet standards search` for \"push fails after commit leaves local state, error message promises recovery\". The nearest hit was help.md \"An error message names what failed, what was expected, and the command that fixes it\". No code-quality.md rule on partial results came up.",
"findings": [
{
"number": "failure-paths.1",
"section": "correctness",
"sites": [
"internal/pullrequests/merge_worktree.go:163",
"internal/pullrequests/merge_worktree.go:78",
"internal/board/fix_menu.go:604"
],
"reach": "A push to the head branch is refused after the merge commit is made. Cause: someone else pushed to the branch since the fetch, or a hook or network failure. The user then presses `f`, then `n`. Traced only. I could not run it, because a hook blocks `git merge` here.",
"body": "**A failed push throws away the user's conflict resolution, and the message says it will be reopened.**\n\n`CommitAndPush` runs `git commit` first and `git push` second. When the push fails, the commit already exists. `MERGE_HEAD` is gone, because the commit consumed it.\n\n`commitAndPush`'s `failed` handler (fix_menu.go:604) then says \"the merge waits in <path>, and f, then n, opens it again\".\n\nThe recovery path does not match that. `f` then `n` calls `Start`, which finds the worktree registered, with no merge under way and a clean `git status`. It takes the `registered` branch, which runs `git checkout --detach <fresh head>`. That discards the resolved merge commit, which is now unreferenced. `Start` then re-merges from scratch and the user resolves the conflicts again.\n\nThis is most likely when the push is refused because someone pushed to the branch meanwhile. The message promises the resolved state and the code deletes it without telling the user.\n\n`conflictsOf` also reports `waiting` as false in this state, because `Merging` checks `MERGE_HEAD`. The menu therefore never shows a pending merge.\n\n**Fix:** on push failure, say the merge is committed locally and unpushed, and offer to retry the push. Alternatively make `Start` detect a worktree whose HEAD is ahead of the head, and refuse or offer a push rather than resetting it. Alternatively `git reset --soft HEAD^` after a failed push, to restore the merge-in-progress state the message describes."
},
{
"number": "failure-paths.2",
"section": "correctness",
"sites": [
"internal/board/fix_menu.go:521",
"internal/board/fix_menu.go:511"
],
"reach": "A pull request has two or more failing GitHub Actions runs, and a later `gh run rerun` fails, for example from a rate limit, a permission error, or a run that is still in progress. Traced only.",
"body": "**A rerun that fails partway reports one error and does not say which runs were rerun.**\n\n`rerunFailed` loops over the runs and returns on the first failure. Earlier runs were already rerun on GitHub. The `failed` handler is the default, so it shows only the single run's error.\n\nThe user does not learn that runs 1..k-1 were restarted. The checks column changes after the next read with no explanation. A retry via `r` re-reruns the earlier runs.\n\n**Fix:** keep going through the remaining runs. Collect the failures per run. Report which runs were rerun and which were not. A failure should cost only the run that failed."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — stale-prose
Used: I cloned PR 17 at 7d607f0 and read every sentence it adds in README.md, CLAUDE.md, the prs config path help text, the Go comments, the error messages and the menu labels. I checked each one against the code at the head. I built prs and ran prs config show against a scratch config whose merge fix sits on key a. It printed refused, key "a" is the fix menu's own, and it keeps n x u l r m a and esc.
Standards: I applied pull-requests.md/Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite. fleet standards search "value the code holds written out in docs goes stale" also surfaced documentation.md/A comment describes the code as it is, never the change or history that produced it.
Findings: 0 correctness · 0 design · 2 prose
Prose
stale-prose.1 README.md:346 · README.md:337 · internal/board/fix_menu.go:23
These sentences copy values the code holds. Each is true at the head. The next change to the list or the constants makes it wrong, and nothing ties it back.
- README.md:346: "A fix may not take a key the menu offers itself:
n,x,u,l,r,mora." This copiesmenuKeysininternal/board/fix_menu.go:77. The sentence also leaves outescandctrl+c, whichRefusedFixrefuses too.prs config showalready prints the live list: on a fix keyedait returnedrefused, key "a" is the fix menu's own, and it keeps n x u l r m a and esc.
Rewrite: "A fix may not take a key the menu offers itself, andprs config showsays which keys those are." - README.md:337: "
blockersnames some ofconflicts,behind,failing-checksanddraft." This copiesconfig.BlockerKinds. The help text forprs config pathalready builds the same list fromstrings.Join(config.BlockerKinds, ", ").
Rewrite: "blockersnames the kinds of blocker the fix is offered for, whichprs config path --helplists." - internal/board/fix_menu.go:23: "The first four are the kinds a merge fix names in its blockers and receives in {blocker}." The count and the order both go stale when a kind is added or a constant moves.
Rewrite: "The ones taken from config are the kinds a merge fix names in its blockers and receives in {blocker}."
stale-prose.2 README.md:267
This sentence narrates the change. README.md:267 says "so a Bitbucket row has no menu, and m there asks to merge as before." The phrase "as before" points at the repo's state before this PR. A reader of the README has no "before".
Rewrite: "so a Bitbucket row has no menu, and m there asks to merge."
Findings as JSON
{
"used": "I cloned PR 17 at 7d607f0 and read every sentence it adds in README.md, CLAUDE.md, the `prs config path` help text, the Go comments, the error messages and the menu labels. I checked each one against the code at the head. I built `prs` and ran `prs config show` against a scratch config whose merge fix sits on key `a`. It printed `refused, key \"a\" is the fix menu's own, and it keeps n x u l r m a and esc`.",
"standards": "I applied `pull-requests.md/Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite`. `fleet standards search \"value the code holds written out in docs goes stale\"` also surfaced `documentation.md/A comment describes the code as it is, never the change or history that produced it`.",
"findings": [
{
"number": "stale-prose.1",
"section": "prose",
"sites": [
"README.md:346",
"README.md:337",
"internal/board/fix_menu.go:23"
],
"reach": "",
"body": "**These sentences copy values the code holds.** Each is true at the head. The next change to the list or the constants makes it wrong, and nothing ties it back.\n\n- README.md:346: \"A fix may not take a key the menu offers itself: `n`, `x`, `u`, `l`, `r`, `m` or `a`.\" This copies `menuKeys` in `internal/board/fix_menu.go:77`. The sentence also leaves out `esc` and `ctrl+c`, which `RefusedFix` refuses too. `prs config show` already prints the live list: on a fix keyed `a` it returned `refused, key \"a\" is the fix menu's own, and it keeps n x u l r m a and esc`.\n Rewrite: \"A fix may not take a key the menu offers itself, and `prs config show` says which keys those are.\"\n- README.md:337: \"`blockers` names some of `conflicts`, `behind`, `failing-checks` and `draft`.\" This copies `config.BlockerKinds`. The help text for `prs config path` already builds the same list from `strings.Join(config.BlockerKinds, \", \")`.\n Rewrite: \"`blockers` names the kinds of blocker the fix is offered for, which `prs config path --help` lists.\"\n- internal/board/fix_menu.go:23: \"The first four are the kinds a merge fix names in its blockers and receives in {blocker}.\" The count and the order both go stale when a kind is added or a constant moves.\n Rewrite: \"The ones taken from config are the kinds a merge fix names in its blockers and receives in {blocker}.\""
},
{
"number": "stale-prose.2",
"section": "prose",
"sites": [
"README.md:267"
],
"reach": "",
"body": "**This sentence narrates the change.** README.md:267 says \"so a Bitbucket row has no menu, and `m` there asks to merge as before.\" The phrase \"as before\" points at the repo's state before this PR. A reader of the README has no \"before\".\n\nRewrite: \"so a Bitbucket row has no menu, and `m` there asks to merge.\""
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — useless-test
Used: I cloned PR 17 and checked it out detached, then read the diff to cmd/prs/tui_test.go beside internal/board/fix_menu.go, rows.go, work.go and config.go. go test -count=1 ./... passed internal/board. Every test in cmd/prs failed at setup with bind: invalid argument, because the stand-in's gh socket path under my own TMPDIR is longer than the unix socket limit, and unshare was not permitted, so I judged those tests from reading them, not from a run.
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 · 0 design · 0 prose
No findings.
Findings as JSON
{
"used": "I cloned PR 17 and checked it out detached, then read the diff to `cmd/prs/tui_test.go` beside `internal/board/fix_menu.go`, `rows.go`, `work.go` and `config.go`. `go test -count=1 ./...` passed `internal/board`. Every test in `cmd/prs` failed at setup with `bind: invalid argument`, because the stand-in's gh socket path under my own TMPDIR is longer than the unix socket limit, and `unshare` was not permitted, so I judged those tests from reading them, not from a run.",
"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": []
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — code-architecture
Used: I cloned the repo and checked out PR 17 at 7d607f0. I read the whole diff and then the head versions of fix_menu.go, actions.go, rows.go, tmux_window_watch.go and config.go. I used rg to find every other site that reads MergeStateStatus, Mergeable, BlockerKinds, ShellQuotedPlaceholders, menuKeys, mergeable and pinFor. I did not run the binary.
Standards: I ran fleet standards search four times. It found two rules that apply: code-quality.md § "Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site", and go.md § "Dispatch over a closed vocabulary names every member". The search for a security property kept in a parallel list returned no matching rule. The search for a reserved-key list kept beside its constants also returned no matching rule.
Findings: 0 correctness · 5 design · 0 prose
Design and architecture
code-architecture.1 internal/board/fix_menu.go:42 · internal/board/fix_menu.go:48 · internal/board/fix_menu.go:50 · internal/board/fix_menu.go:56 · internal/board/fix_menu.go:301 · internal/board/rows.go:106 · internal/board/rows.go:129 · internal/board/actions.go:144
Reach: The next GitHub merge state, or a change to what an existing state means, has to edit mergeMeanings and blockerOf together. The code itself expects new states. stateMeaning's comment says "because GitHub adds states". This PR is already that kind of change: it adds a precedence (draft, then conflict, then behind, then unknown, then failing checks) that the row's precedence in stateMeaning does not hold.
Two places decide what GitHub's merge state means. mergeMeanings in rows.go is the table that maps each state to its word, ready and computing. Before this PR, stateMeaning and mergeBlocker read only that table. The new blockerOf decides the same states again with string literals: "DRAFT", "DIRTY", "BEHIND", "UNSTABLE" and "BLOCKED". It also repeats the draft-then-CONFLICTING override from stateMeaning (rows.go:133-136). fixesFor compares against "UNSTABLE" a third time (fix_menu.go:301) to decide whether "merge anyway" is offered.
The two classifications already differ. blockerOf treats MergeStateStatus == "DRAFT" as a draft, but stateMeaning checks only pr.Draft. A state missing from the table reads as its own words in the row, but blockerOf treats it as needing no fix. The table and the switch carry the same facts, so the next edit to one has to be made to the other by hand. This is what code-quality.md § "Ask whatever owns a fact…" forbids.
Fix: give each mergeMeanings entry its blocker kind and a mergesAnyway flag. Put the draft/conflict override in one shared mergeStateOf(pr) string that both stateMeaning and blockerOf call. blockerOf then becomes a table lookup plus the failing-checks rule. The "UNSTABLE" check at line 301 reads the flag.
code-architecture.2 internal/config/config.go:189 · internal/config/config.go:227 · internal/config/config.go:232 · internal/config/config.go:514
Reach: The next placeholder whose value comes from outside prs needs two edits that must agree: an entry in fixPlaceholders and an entry in ShellQuotedPlaceholders. A pull request's title or an author login would be such a placeholder. This PR already added five quoted names and one unquoted name ({blocker}), so placeholders are growing. If the second edit is missed, the shell receives an unquoted branch-controlled string, and nothing fails at load or in a test.
Whether a placeholder gets shell-quoted is kept in a list separate from the placeholder itself. fixPlaceholders maps each name to its value. ShellQuotedPlaceholders is a hand-sorted string slice of the names to quote. filled checks membership at fill time, and insideQuotes checks the same list at load time. Nothing ties the two together. A new map entry is filled bare by default, so a missed edit leaves the command open to shell injection, which is exactly the risk the PR body says this design prevents.
Fix: make each placeholder entry a struct {fill func(Fields) string; outside bool}. Derive ShellQuotedPlaceholders from the map in the same way FixPlaceholders() is already derived. Then one entry decides both the value and whether it is quoted.
code-architecture.3 internal/config/config.go:131 · internal/config/config.go:139 · internal/board/fix_menu.go:27 · internal/board/fix_menu.go:42 · internal/board/fix_menu.go:253 · cmd/prs/config.go:84 · README.md:337
Reach: The next fixable blocker kind would cost edits in five places. heldBlocker (a required review) is the likeliest next kind, because the menu already names it but offers it no choices. Adding it means editing the constants and BlockerKinds in config, the alias constants in board, a case in blockerOf, a case in fixesFor, and the help text in cmd/prs/config.go and README.
The list of blocker kinds is owned by the config package, but what each kind means is decided in board. config.go declares Conflicts, Behind, FailingChecks and Draft, plus BlockerKinds for validating blockers: at load. board redeclares each of them as a blocker constant (fix_menu.go:27-30). board also adds two more kinds that config does not know about: computingBlocker and heldBlocker. As a result, the list config accepts and the list board produces are maintained separately, and no compile-time check catches a kind that one side has and the other lacks.
Fix: move the blocker vocabulary next to blockerOf, in board or pullrequests. Each entry should say whether a merge fix may name it. Have config validate blockers: against a list passed in, the same way board.RefusedFix already checks keys after load. Then adding a kind means adding one entry and one fixesFor case.
code-architecture.4 internal/board/fix_menu.go:66 · internal/board/fix_menu.go:77 · internal/board/fix_menu.go:82 · internal/board/fix_menu.go:233
Reach: The next built-in choice in the menu needs both a new key constant and a matching entry in menuKeys. This PR adds eight such choices, so more are likely, for example 'open in browser' or 'rerun all'. If the menuKeys edit is missed, a configured fix can take the same key. askFix then runs whichever choice comes first in the slice, and nothing reports the clash.
The menu's reserved keys are a hand-kept list beside the choices that use them. menuKeys repeats seven of the eight key constants. It leaves out readyKey only because that key equals rerunKey. RefusedFix checks configured fixes against this list. The choices themselves are built in fixesFor, checkFixes, conflictFixes and unmergedMenu, and none of them reads menuKeys.
Fix: check for duplicates where the choices are put together. One check over the final []fixChoice, inside askFix or fixesFor, would refuse a key that appears twice. Alternatively, build menuKeys from a single table of the built-in choices, so that declaring a choice also reserves its key.
code-architecture.5 internal/board/fix_menu.go:303 · internal/board/actions.go:403 · internal/board/actions.go:602
Reach: The next change to how prs merges has to be made at three sites. Examples are a merge-method option, a change to which head pinFor pins, or telling the person where the pin came from. This PR is the change that created the third copy.
The steps of a merge are written out three times. "Merge anyway" (fix_menu.go:303-312) calls b.mergeable(pr), then refused, then b.pinFor(pr), then b.start(b.mergeWork(...)). askMerge (actions.go:403-411) does the same sequence with a confirmation question added. The batch path (actions.go:602-606) is a third copy. Each copy decides for itself which refusal stops the merge and which head gets pinned.
Fix: pull out one mergeNow(m, pr) tea.Cmd or mergeCmd(pr) (work, error) that does the provider check, the refusal and the pin. askMerge asks first and then calls it. "Merge anyway" calls it directly.
Findings as JSON
{
"used": "I cloned the repo and checked out PR 17 at 7d607f0. I read the whole diff and then the head versions of fix_menu.go, actions.go, rows.go, tmux_window_watch.go and config.go. I used rg to find every other site that reads MergeStateStatus, Mergeable, BlockerKinds, ShellQuotedPlaceholders, menuKeys, mergeable and pinFor. I did not run the binary.",
"standards": "I ran `fleet standards search` four times. It found two rules that apply: code-quality.md § \"Ask whatever owns a fact, the function or enum that computes it, never duplicate its logic at a second site\", and go.md § \"Dispatch over a closed vocabulary names every member\". The search for a security property kept in a parallel list returned no matching rule. The search for a reserved-key list kept beside its constants also returned no matching rule.",
"findings": [
{
"number": "code-architecture.1",
"section": "design",
"sites": [
"internal/board/fix_menu.go:42",
"internal/board/fix_menu.go:48",
"internal/board/fix_menu.go:50",
"internal/board/fix_menu.go:56",
"internal/board/fix_menu.go:301",
"internal/board/rows.go:106",
"internal/board/rows.go:129",
"internal/board/actions.go:144"
],
"reach": "The next GitHub merge state, or a change to what an existing state means, has to edit `mergeMeanings` and `blockerOf` together. The code itself expects new states. `stateMeaning`'s comment says \"because GitHub adds states\". This PR is already that kind of change: it adds a precedence (draft, then conflict, then behind, then unknown, then failing checks) that the row's precedence in `stateMeaning` does not hold.",
"body": "**Two places decide what GitHub's merge state means.** `mergeMeanings` in rows.go is the table that maps each state to its word, `ready` and `computing`. Before this PR, `stateMeaning` and `mergeBlocker` read only that table. The new `blockerOf` decides the same states again with string literals: `\"DRAFT\"`, `\"DIRTY\"`, `\"BEHIND\"`, `\"UNSTABLE\"` and `\"BLOCKED\"`. It also repeats the draft-then-`CONFLICTING` override from `stateMeaning` (rows.go:133-136). `fixesFor` compares against `\"UNSTABLE\"` a third time (fix_menu.go:301) to decide whether \"merge anyway\" is offered.\n\nThe two classifications already differ. `blockerOf` treats `MergeStateStatus == \"DRAFT\"` as a draft, but `stateMeaning` checks only `pr.Draft`. A state missing from the table reads as its own words in the row, but `blockerOf` treats it as needing no fix. The table and the switch carry the same facts, so the next edit to one has to be made to the other by hand. This is what code-quality.md § \"Ask whatever owns a fact…\" forbids.\n\n**Fix:** give each `mergeMeanings` entry its blocker kind and a `mergesAnyway` flag. Put the draft/conflict override in one shared `mergeStateOf(pr) string` that both `stateMeaning` and `blockerOf` call. `blockerOf` then becomes a table lookup plus the failing-checks rule. The `\"UNSTABLE\"` check at line 301 reads the flag."
},
{
"number": "code-architecture.2",
"section": "design",
"sites": [
"internal/config/config.go:189",
"internal/config/config.go:227",
"internal/config/config.go:232",
"internal/config/config.go:514"
],
"reach": "The next placeholder whose value comes from outside prs needs two edits that must agree: an entry in `fixPlaceholders` and an entry in `ShellQuotedPlaceholders`. A pull request's title or an author login would be such a placeholder. This PR already added five quoted names and one unquoted name (`{blocker}`), so placeholders are growing. If the second edit is missed, the shell receives an unquoted branch-controlled string, and nothing fails at load or in a test.",
"body": "**Whether a placeholder gets shell-quoted is kept in a list separate from the placeholder itself.** `fixPlaceholders` maps each name to its value. `ShellQuotedPlaceholders` is a hand-sorted string slice of the names to quote. `filled` checks membership at fill time, and `insideQuotes` checks the same list at load time. Nothing ties the two together. A new map entry is filled bare by default, so a missed edit leaves the command open to shell injection, which is exactly the risk the PR body says this design prevents.\n\n**Fix:** make each placeholder entry a struct `{fill func(Fields) string; outside bool}`. Derive `ShellQuotedPlaceholders` from the map in the same way `FixPlaceholders()` is already derived. Then one entry decides both the value and whether it is quoted."
},
{
"number": "code-architecture.3",
"section": "design",
"sites": [
"internal/config/config.go:131",
"internal/config/config.go:139",
"internal/board/fix_menu.go:27",
"internal/board/fix_menu.go:42",
"internal/board/fix_menu.go:253",
"cmd/prs/config.go:84",
"README.md:337"
],
"reach": "The next fixable blocker kind would cost edits in five places. `heldBlocker` (a required review) is the likeliest next kind, because the menu already names it but offers it no choices. Adding it means editing the constants and `BlockerKinds` in config, the alias constants in board, a case in `blockerOf`, a case in `fixesFor`, and the help text in cmd/prs/config.go and README.",
"body": "**The list of blocker kinds is owned by the config package, but what each kind means is decided in board.** config.go declares `Conflicts`, `Behind`, `FailingChecks` and `Draft`, plus `BlockerKinds` for validating `blockers:` at load. board redeclares each of them as a `blocker` constant (fix_menu.go:27-30). board also adds two more kinds that config does not know about: `computingBlocker` and `heldBlocker`. As a result, the list config accepts and the list board produces are maintained separately, and no compile-time check catches a kind that one side has and the other lacks.\n\n**Fix:** move the blocker vocabulary next to `blockerOf`, in board or pullrequests. Each entry should say whether a merge fix may name it. Have config validate `blockers:` against a list passed in, the same way `board.RefusedFix` already checks keys after load. Then adding a kind means adding one entry and one `fixesFor` case."
},
{
"number": "code-architecture.4",
"section": "design",
"sites": [
"internal/board/fix_menu.go:66",
"internal/board/fix_menu.go:77",
"internal/board/fix_menu.go:82",
"internal/board/fix_menu.go:233"
],
"reach": "The next built-in choice in the menu needs both a new key constant and a matching entry in `menuKeys`. This PR adds eight such choices, so more are likely, for example 'open in browser' or 'rerun all'. If the `menuKeys` edit is missed, a configured fix can take the same key. `askFix` then runs whichever choice comes first in the slice, and nothing reports the clash.",
"body": "**The menu's reserved keys are a hand-kept list beside the choices that use them.** `menuKeys` repeats seven of the eight key constants. It leaves out `readyKey` only because that key equals `rerunKey`. `RefusedFix` checks configured fixes against this list. The choices themselves are built in `fixesFor`, `checkFixes`, `conflictFixes` and `unmergedMenu`, and none of them reads `menuKeys`.\n\n**Fix:** check for duplicates where the choices are put together. One check over the final `[]fixChoice`, inside `askFix` or `fixesFor`, would refuse a key that appears twice. Alternatively, build `menuKeys` from a single table of the built-in choices, so that declaring a choice also reserves its key."
},
{
"number": "code-architecture.5",
"section": "design",
"sites": [
"internal/board/fix_menu.go:303",
"internal/board/actions.go:403",
"internal/board/actions.go:602"
],
"reach": "The next change to how prs merges has to be made at three sites. Examples are a merge-method option, a change to which head `pinFor` pins, or telling the person where the pin came from. This PR is the change that created the third copy.",
"body": "**The steps of a merge are written out three times.** \"Merge anyway\" (fix_menu.go:303-312) calls `b.mergeable(pr)`, then `refused`, then `b.pinFor(pr)`, then `b.start(b.mergeWork(...))`. `askMerge` (actions.go:403-411) does the same sequence with a confirmation question added. The batch path (actions.go:602-606) is a third copy. Each copy decides for itself which refusal stops the merge and which head gets pinned.\n\n**Fix:** pull out one `mergeNow(m, pr) tea.Cmd` or `mergeCmd(pr) (work, error)` that does the provider check, the refusal and the pin. `askMerge` asks first and then calls it. \"Merge anyway\" calls it directly."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — contracts
Used: I built the head into a sandbox binary and ran prs config show and prs config path --help against configs inside my own directory. The first config was a copy of the live ~/.config/prs/config.yml with a merge_fixes entry added. The second bound a window action to f. I searched all 115 checkouts in the registry for window_actions, merge_fixes, worktree_root, WORKTREE_ROOT and make-mergeable. I read dotfiles apps/common/worktree for the fleet's existing worktree root. Other surfaces break no existing caller: the live config binds window actions only to r and C, so the newly reserved f touches nothing. Window commands filled through FillCommand leave {number}, {repo} and {slug} bare, as before. cmd/prs/list.schema.json did not change.
Standards: data.md § "A shared file is named in config; only the tools own default is compiled in" (resolve env var, then config key, then own-XDG default); configuration.md § "Every path a tool writes is an XDG base directory"; git-workflow.md § "Concurrent sessions isolate with a worktree, and it lands on main" (~/.worktrees/<repo>/<slug>).
Findings: 0 correctness · 1 design · 0 prose
Design and architecture
contracts.1 internal/config/config.go:393 · cmd/prs/config.go:41 · README.md:326 · dotfiles:apps/common/worktree:534
Reach: The next caller to misread it is a machine that sets WORKTREE_ROOT or keeps its worktrees in ~/.worktrees. On that machine prs puts its worktrees under $XDG_DATA_HOME/prs/worktrees. worktree list scans only $WORKTREE_ROOT and cannot see them. This PR's own risk section relies on git worktree list to find worktrees prs leaves behind after a revert. The next change it costs is a migration of every worktree already made, once prs adopts the variable.
prs adds a second worktree root and never reads the fleet's WORKTREE_ROOT.
The fleet already has a worktree root. dotfiles apps/common/worktree resolves it as $WORKTREE_ROOT, then ~/.worktrees. Its docstring calls that root "the only index of them". Its layout is <root>/<repo>/<slug>. This PR's worktree_root key uses the same layout, <root>/<checkout's directory>/pr-<n>. Its resolution skips the variable and goes straight from the config key to $XDG_DATA_HOME/prs/worktrees.
I ran it with WORKTREE_ROOT=/x/wt XDG_DATA_HOME=…/sb/data prs config show. It printed:
worktree …/sb/data/prs/worktrees from the default
The variable was ignored. One meaning, "where worktrees live", now has two names and two defaults. The help text adds a third path: the example in prs config path --help and the README reads worktree_root: ~/worktrees, not ~/.worktrees.
data.md § "A shared file is named in config; only the tools own default is compiled in" covers this case: "a directory tree one indexes". It says to resolve "environment variable, then config key, then a default under the tool's own XDG directory". The default here complies. The environment-variable layer is missing.
Fix:
- Resolve
WORKTREE_ROOTahead ofworktree_root, and keep$XDG_DATA_HOME/prs/worktreesas the default. - Have
config showreport "from WORKTREE_ROOT" when the variable supplies the value. - Change the example in
cmd/prs/config.go:41andREADME.md:326to~/.worktrees, or to a path that does not look like a typo of it. - Make this change before the release ships, while no worktree exists under the old default.
Findings as JSON
{
"used": "I built the head into a sandbox binary and ran `prs config show` and `prs config path --help` against configs inside my own directory. The first config was a copy of the live `~/.config/prs/config.yml` with a `merge_fixes` entry added. The second bound a window action to `f`. I searched all 115 checkouts in the registry for `window_actions`, `merge_fixes`, `worktree_root`, `WORKTREE_ROOT` and `make-mergeable`. I read dotfiles `apps/common/worktree` for the fleet's existing worktree root. Other surfaces break no existing caller: the live config binds window actions only to `r` and `C`, so the newly reserved `f` touches nothing. Window commands filled through `FillCommand` leave `{number}`, `{repo}` and `{slug}` bare, as before. `cmd/prs/list.schema.json` did not change.",
"standards": "data.md § \"A shared file is named in config; only the tools own default is compiled in\" (resolve env var, then config key, then own-XDG default); configuration.md § \"Every path a tool writes is an XDG base directory\"; git-workflow.md § \"Concurrent sessions isolate with a worktree, and it lands on main\" (`~/.worktrees/<repo>/<slug>`).",
"findings": [
{
"number": "contracts.1",
"section": "design",
"sites": [
"internal/config/config.go:393",
"cmd/prs/config.go:41",
"README.md:326",
"dotfiles:apps/common/worktree:534"
],
"reach": "The next caller to misread it is a machine that sets `WORKTREE_ROOT` or keeps its worktrees in `~/.worktrees`. On that machine prs puts its worktrees under `$XDG_DATA_HOME/prs/worktrees`. `worktree list` scans only `$WORKTREE_ROOT` and cannot see them. This PR's own risk section relies on `git worktree list` to find worktrees prs leaves behind after a revert. The next change it costs is a migration of every worktree already made, once prs adopts the variable.",
"body": "**prs adds a second worktree root and never reads the fleet's `WORKTREE_ROOT`.**\n\nThe fleet already has a worktree root. dotfiles `apps/common/worktree` resolves it as `$WORKTREE_ROOT`, then `~/.worktrees`. Its docstring calls that root \"the only index of them\". Its layout is `<root>/<repo>/<slug>`. This PR's `worktree_root` key uses the same layout, `<root>/<checkout's directory>/pr-<n>`. Its resolution skips the variable and goes straight from the config key to `$XDG_DATA_HOME/prs/worktrees`.\n\nI ran it with `WORKTREE_ROOT=/x/wt XDG_DATA_HOME=…/sb/data prs config show`. It printed:\n\n```\nworktree …/sb/data/prs/worktrees from the default\n```\n\nThe variable was ignored. One meaning, \"where worktrees live\", now has two names and two defaults. The help text adds a third path: the example in `prs config path --help` and the README reads `worktree_root: ~/worktrees`, not `~/.worktrees`.\n\ndata.md § \"A shared file is named in config; only the tools own default is compiled in\" covers this case: \"a directory tree one indexes\". It says to resolve \"environment variable, then config key, then a default under the tool's own XDG directory\". The default here complies. The environment-variable layer is missing.\n\n**Fix:**\n- Resolve `WORKTREE_ROOT` ahead of `worktree_root`, and keep `$XDG_DATA_HOME/prs/worktrees` as the default.\n- Have `config show` report \"from WORKTREE_ROOT\" when the variable supplies the value.\n- Change the example in `cmd/prs/config.go:41` and `README.md:326` to `~/.worktrees`, or to a path that does not look like a typo of it.\n- Make this change before the release ships, while no worktree exists under the old default."
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — fleet-patterns
Used: I checked out PR 17 at 7d607f0 and read three files: merge_worktree.go, provider.go and FillCommand/shellQuoted in config.go. I compared them with the make-pr-mergeable skill, dotfiles apps/common/worktree, fleet fix/worktree.go and audit/checkout.go, fleet's ci log reader, gotermboard 056debb, and every shellQuote copy in fleet, slate and runthrough. I used indy search and rg across the registry checkouts to find them.
Standards: I cited git-workflow.md/A branch catches up only when something forces it, and then by rebasing and code-quality.md/Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the formats grammar, never a regex of the values seen so far. Searches for a tmux-window rule and a worktree-for-a-PR rule found no rule that applies.
Findings: 0 correctness · 3 design · 0 prose
Design and architecture
fleet-patterns.1 internal/pullrequests/merge_worktree.go:97 · internal/pullrequests/provider.go:111 · claude:skills/make-pr-mergeable/SKILL.md:19 · claude:skills/make-pr-mergeable/SKILL.md:52 · claude:skills/make-pr-mergeable/SKILL.md:80
Reach: Every conflicting or behind GitHub row someone fixes from the menu (n then push, or u). The same pull request can also reach /make-pr-mergeable, which the conflict test hands off to through c, and that skill rebases. Any later change to how a blocked branch catches up has to be made in prs and in the skill.
prs merges the base into a blocked branch, and the fleet's existing fix rebases it.
Chris already has a solution for making a pull request mergeable: the make-pr-mergeable skill. It uses the same three blockers this menu uses: conflicts, behind and failing checks. For a conflict it runs git rebase origin/<base> and then git push --force-with-lease --force-if-includes. For a branch behind it runs gh pr update-branch <n> --rebase. Its "never" list says it "never merges the base into the head branch".
The git-workflow.md rule "A branch catches up only when something forces it, and then by rebasing" says the same thing: "Never git merge main into a branch".
This PR does the opposite at both sites:
MergeWorktree.Startrunsgit merge --no-ff --no-commit origin/<base>and pushes a merge commit.UpdateBranchrunsgh pr update-branchwith no--rebase, so GitHub writes a merge commit too.
The PR body gives a reason: a rebase "rewrites a branch a reviewer has read and needs a force push". The standard already weighs that reason and rejects it. It says "The force push is not a workaround for the rebase; it is the rebase", and it requires the lease flags so the push stays safe. The body neither cites the rule nor argues against it.
The cost is that one pull request now gets two different histories depending on who fixes it. If n resolves a conflict in prs and the skill later rebases the same branch, git drops the merge commit and replays the commits from before it.
Fix: follow the skill and the rule. Use gh pr update-branch --rebase for a branch behind. For a conflict, rebase in the worktree and push with --force-with-lease --force-if-includes. If merging really is the better choice for prs, change the rule and the skill in the same change, so the fleet keeps one answer.
fleet-patterns.2 internal/config/config.go:393 · internal/pullrequests/merge_worktree.go:52 · internal/pullrequests/merge_worktree.go:79 · dotfiles:apps/common/worktree:39 · dotfiles:apps/common/worktree:536 · claude:skills/make-pr-mergeable/SKILL.md:70
Reach: There are two cases. In the first, worktree_root is left at its default. Then every worktree prs adds sits outside the index that worktree list, show, choose and sweep read, so an abandoned one is only found by hand. The PR body's rollback note says exactly that. In the second, worktree_root is pointed at ~/.worktrees to join that index. Then prs and the skill both claim <root>/<repo>/pr-<n>, and Start runs checkout --detach in the skill's worktree.
prs starts a second worktree index rather than using $WORKTREE_ROOT.
The fleet already has one place for worktrees. In dotfiles apps/common/worktree, the index is $WORKTREE_ROOT, which defaults to ~/.worktrees (line 536). Line 39 states that "$WORKTREE_ROOT is the whole index — a repo has a directory there exactly when it has a worktree". Its sweep command is what collects worktrees that are finished. The make-pr-mergeable skill adds its pull-request worktree at ${WORKTREE_ROOT}/<repo>/pr-<n> and leaves it in place.
prs uses the same layout, <root>/<checkout dir>/pr-<n> (MergeWorktreeAt). Its default root is a new one, $XDG_DATA_HOME/prs/worktrees (worktreeRoot), and the body gives no reason for that. The layout matches the skill but the root does not, which causes two problems:
- At the default, these worktrees are missing from every
worktreelisting and fromsweep. The PR's rollback section has to tell the reader to rungit worktree listby hand. - Pointed at the shared index,
pr-<n>collides with the skill's worktree. The skill leaves that worktree clean on the head branch.Startsees a registered, clean worktree with no merge under way and runscheckout --quiet --detach <head>in it (merge_worktree.go:83). That moves the worktree the skill left off its branch.
Fix: default worktree_root to $WORKTREE_ROOT, falling back to ~/.worktrees, so the existing tools see these worktrees. Give prs's worktree a name the skill does not use, such as pr-<n>-merge. Alternatively, make the skill and prs share one convention and have each refuse a worktree that is on a branch.
fleet-patterns.3 internal/config/config.go:268 · slate:cli/internal/cli/argument_values.go:29 · fleet:syncthing/fleet.go:513 · fleet:eval/fleettool.go:249 · fleet:cmd/standards_address.go:426 · runthrough:src/runthrough/config.py:82
Reach: The next bug fix in one shell quoter will not reach the others. The copies already disagree on which characters they leave unquoted. prs is the one copy whose output gets executed, with branch names chosen by whoever opens a pull request.
shellQuoted is a sixth hand-written shell quoter in the fleet, and the copies already disagree.
The Go copies are these four:
- fleet has three private
shellQuotefunctions, insyncthing/fleet.go:513,eval/fleettool.go:249andcmd/standards_address.go:426. Each always single-quotes. - slate's copy (
argument_values.go:29) leaves a value bare unless it contains a space, a tab, a quote, a backslash,$or a backtick. That meansa;b,a|banda&bstay bare. - prs's new
shellQuotedfollowsshlex.quoteand uses an allowlist regex of safe characters.
runthrough uses Python's shlex.quote directly.
The prs version is the correct shape, and the code-quality.md rule on escaping a shell word through the format's own encoder backs it. It is also the only copy whose output is run rather than printed as a hint. That is where it matters most that every copy is the same.
This is not a contract between a client and a server. It is evidence that Go code in the fleet has no shared encoder for a shell word, so each repo writes its own and they drift apart. slate's copy shows the drift already.
Fix: move one ShellQuote (shlex semantics) into goclikit, which every Go tool here already imports. Use it in prs. Then replace the fleet and slate copies with it. Until that happens, the prs copy is the one the others should match.
Findings as JSON
{
"used": "I checked out PR 17 at 7d607f0 and read three files: merge_worktree.go, provider.go and FillCommand/shellQuoted in config.go. I compared them with the make-pr-mergeable skill, dotfiles apps/common/worktree, fleet fix/worktree.go and audit/checkout.go, fleet's ci log reader, gotermboard 056debb, and every shellQuote copy in fleet, slate and runthrough. I used indy search and rg across the registry checkouts to find them.",
"standards": "I cited `git-workflow.md/A branch catches up only when something forces it, and then by rebasing` and `code-quality.md/Parsing or escaping JSON, YAML, CSV, a URL or a shell word follows the formats grammar, never a regex of the values seen so far`. Searches for a tmux-window rule and a worktree-for-a-PR rule found no rule that applies.",
"findings": [
{
"number": "fleet-patterns.1",
"section": "design",
"sites": [
"internal/pullrequests/merge_worktree.go:97",
"internal/pullrequests/provider.go:111",
"claude:skills/make-pr-mergeable/SKILL.md:19",
"claude:skills/make-pr-mergeable/SKILL.md:52",
"claude:skills/make-pr-mergeable/SKILL.md:80"
],
"reach": "Every conflicting or behind GitHub row someone fixes from the menu (`n` then push, or `u`). The same pull request can also reach `/make-pr-mergeable`, which the conflict test hands off to through `c`, and that skill rebases. Any later change to how a blocked branch catches up has to be made in prs and in the skill.",
"body": "**prs merges the base into a blocked branch, and the fleet's existing fix rebases it.**\n\nChris already has a solution for making a pull request mergeable: the `make-pr-mergeable` skill. It uses the same three blockers this menu uses: conflicts, behind and failing checks. For a conflict it runs `git rebase origin/<base>` and then `git push --force-with-lease --force-if-includes`. For a branch behind it runs `gh pr update-branch <n> --rebase`. Its \"never\" list says it \"never merges the base into the head branch\".\n\nThe `git-workflow.md` rule \"A branch catches up only when something forces it, and then by rebasing\" says the same thing: \"Never `git merge main` into a branch\".\n\nThis PR does the opposite at both sites:\n- `MergeWorktree.Start` runs `git merge --no-ff --no-commit origin/<base>` and pushes a merge commit.\n- `UpdateBranch` runs `gh pr update-branch` with no `--rebase`, so GitHub writes a merge commit too.\n\nThe PR body gives a reason: a rebase \"rewrites a branch a reviewer has read and needs a force push\". The standard already weighs that reason and rejects it. It says \"The force push is not a workaround for the rebase; it **is** the rebase\", and it requires the lease flags so the push stays safe. The body neither cites the rule nor argues against it.\n\nThe cost is that one pull request now gets two different histories depending on who fixes it. If `n` resolves a conflict in prs and the skill later rebases the same branch, git drops the merge commit and replays the commits from before it.\n\n**Fix:** follow the skill and the rule. Use `gh pr update-branch --rebase` for a branch behind. For a conflict, rebase in the worktree and push with `--force-with-lease --force-if-includes`. If merging really is the better choice for prs, change the rule and the skill in the same change, so the fleet keeps one answer."
},
{
"number": "fleet-patterns.2",
"section": "design",
"sites": [
"internal/config/config.go:393",
"internal/pullrequests/merge_worktree.go:52",
"internal/pullrequests/merge_worktree.go:79",
"dotfiles:apps/common/worktree:39",
"dotfiles:apps/common/worktree:536",
"claude:skills/make-pr-mergeable/SKILL.md:70"
],
"reach": "There are two cases. In the first, `worktree_root` is left at its default. Then every worktree prs adds sits outside the index that `worktree list`, `show`, `choose` and `sweep` read, so an abandoned one is only found by hand. The PR body's rollback note says exactly that. In the second, `worktree_root` is pointed at `~/.worktrees` to join that index. Then prs and the skill both claim `<root>/<repo>/pr-<n>`, and `Start` runs `checkout --detach` in the skill's worktree.",
"body": "**prs starts a second worktree index rather than using `$WORKTREE_ROOT`.**\n\nThe fleet already has one place for worktrees. In dotfiles `apps/common/worktree`, the index is `$WORKTREE_ROOT`, which defaults to `~/.worktrees` (line 536). Line 39 states that \"$WORKTREE_ROOT is the whole index — a repo has a directory there exactly when it has a worktree\". Its `sweep` command is what collects worktrees that are finished. The `make-pr-mergeable` skill adds its pull-request worktree at `${WORKTREE_ROOT}/<repo>/pr-<n>` and leaves it in place.\n\nprs uses the same layout, `<root>/<checkout dir>/pr-<n>` (`MergeWorktreeAt`). Its default root is a new one, `$XDG_DATA_HOME/prs/worktrees` (`worktreeRoot`), and the body gives no reason for that. The layout matches the skill but the root does not, which causes two problems:\n- At the default, these worktrees are missing from every `worktree` listing and from `sweep`. The PR's rollback section has to tell the reader to run `git worktree list` by hand.\n- Pointed at the shared index, `pr-<n>` collides with the skill's worktree. The skill leaves that worktree clean on the head branch. `Start` sees a registered, clean worktree with no merge under way and runs `checkout --quiet --detach <head>` in it (merge_worktree.go:83). That moves the worktree the skill left off its branch.\n\n**Fix:** default `worktree_root` to `$WORKTREE_ROOT`, falling back to `~/.worktrees`, so the existing tools see these worktrees. Give prs's worktree a name the skill does not use, such as `pr-<n>-merge`. Alternatively, make the skill and prs share one convention and have each refuse a worktree that is on a branch."
},
{
"number": "fleet-patterns.3",
"section": "design",
"sites": [
"internal/config/config.go:268",
"slate:cli/internal/cli/argument_values.go:29",
"fleet:syncthing/fleet.go:513",
"fleet:eval/fleettool.go:249",
"fleet:cmd/standards_address.go:426",
"runthrough:src/runthrough/config.py:82"
],
"reach": "The next bug fix in one shell quoter will not reach the others. The copies already disagree on which characters they leave unquoted. prs is the one copy whose output gets executed, with branch names chosen by whoever opens a pull request.",
"body": "**`shellQuoted` is a sixth hand-written shell quoter in the fleet, and the copies already disagree.**\n\nThe Go copies are these four:\n- fleet has three private `shellQuote` functions, in `syncthing/fleet.go:513`, `eval/fleettool.go:249` and `cmd/standards_address.go:426`. Each always single-quotes.\n- slate's copy (`argument_values.go:29`) leaves a value bare unless it contains a space, a tab, a quote, a backslash, `$` or a backtick. That means `a;b`, `a|b` and `a&b` stay bare.\n- prs's new `shellQuoted` follows `shlex.quote` and uses an allowlist regex of safe characters.\n\nrunthrough uses Python's `shlex.quote` directly.\n\nThe prs version is the correct shape, and the `code-quality.md` rule on escaping a shell word through the format's own encoder backs it. It is also the only copy whose output is run rather than printed as a hint. That is where it matters most that every copy is the same.\n\nThis is not a contract between a client and a server. It is evidence that Go code in the fleet has no shared encoder for a shell word, so each repo writes its own and they drift apart. slate's copy shows the drift already.\n\n**Fix:** move one `ShellQuote` (shlex semantics) into goclikit, which every Go tool here already imports. Use it in prs. Then replace the fleet and slate copies with it. Until that happens, the prs copy is the one the others should match."
}
]
}This review does not gate merge.
There was a problem hiding this comment.
PR review — G1
Used: Built the branch with go build ./cmd/prs. Drove MergeWorktree.Start, CommitAndPush and a second Start from a scratch Go test against a local bare origin, a checkout and a conflicting PR branch, with git config pointed at a scratch file. Ran go vet ./... and go test -count=1 ./.... Then ran the failing tuidrive test alone twice at load average 52.
Standards: Cited help.md § "An error message names what failed, what was expected, and the command that fixes it", documentation.md § "A comment describes the code as it is, never the change or history that produced it" and pull-requests.md § "Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite". Searches for a rule on TUI tests that race an async key found none.
Findings: 2 correctness · 1 design · 1 prose
Correctness
G1.1 internal/pullrequests/merge_worktree.go:166 · internal/pullrequests/merge_worktree.go:79 · internal/pullrequests/merge_worktree.go:83 · internal/board/fix_menu.go:685 · internal/pullrequests/pullrequest.go:176
Reach: Someone pushes to the head branch while the user resolves in nvim, or a pre-push hook fails. The PR body expects this: "The push is a plain git push origin HEAD:refs/heads/<branch>, so it fails where the branch moved." The user then follows prs's own advice, f then n.
A refused push throws away the user's conflict resolution, and the footer says the opposite.
CommitAndPush commits before it pushes. When the push fails, the commit has already used up MERGE_HEAD. Merging then reads false. The footer still says …; the merge waits in <path>, and f, then n, opens it again. Pressing f then n runs Start, which hits case registered:. The tree is clean, so Start runs git checkout --detach <new head> and merges again. The resolved merge commit becomes a detached orphan that only the reflog can find, and the conflict is back.
What I ran: a scratch test against a local bare origin. Steps:
Start→merging=true.- Resolved
fby hand and staged it. - Pushed a new commit to
featfrom another clone. CommitAndPush→ error.Merging→false. The worktree'sgit logshowsMerge branch 'main' into feat.Fetch+Start→merging=true.freads<<<<<<< HEAD\nfeat\n=======\nmain\n>>>>>>> origin/main.git statusreadsUU f.
The push error also drops git's reason. Run keeps only the first stderr line. For a rejected push that line is To <remote>. The user saw git could not push the merge in …/pr-7 to feat: To …/origin.git, which names neither "rejected" nor "fetch first". help.md § "An error message names what failed, what was expected, and the command that fixes it" also reaches this.
The same loss happens when an earlier n left a merge waiting on a head that has since moved. Start returns at registered && merging without checking the head. The push is refused, and the next n starts over.
Fix:
- When the push fails, undo the commit with
git reset --soft HEAD^plusgit update-ref MERGE_HEAD origin/<base>, so the merge is still under way. Or keep the commit and haveStartoffer to push a worktree whoseHEADalready hasorigin/<base>as a second parent. - Make the footer say what is actually kept.
- For
git push, report the!orerror:line rather than the first one.
G1.2 internal/board/fix_menu.go:633 · internal/board/fix_menu.go:260 · internal/board/fix_menu.go:123 · internal/pullrequests/merge_worktree.go:174
Reach: The unmerged menu offers merge fixes with {worktree} set to the merge under way, and the README's example c hand-to-claude fix names conflicts. Such a session finishes the merge in the worktree and pushes. The worktree is also stranded when the user leaves a merge waiting and the conflict is then fixed anywhere else, or the PR is merged or closed.
A merge that ends any way but prs's own push or abort leaves its worktree behind, and no key in prs removes it.
Only two prs actions call MergeWorktree.Remove:
- the commit-and-push question after
n(CommitAndPush) - the
aabort choice in the unmerged menu (Abort)
Both are reached only from the n flow. Once GitHub stops reading the PR as conflicting, the n and a choices are gone:
fixesForbuilds conflict choices only undercase conflictsBlocker.fon a now-clean row saysGitHub reads … as ready, which needs no fix; m merges it.
So a worktree that a configured fix committed and pushed from stays at <worktree_root>/<checkout>/pr-<n> for good. The same goes for one the user left with esc before the conflict was fixed elsewhere. Each one shows in the checkout's git worktree list. The PR body admits leftover worktrees only for a revert, yet they pile up in ordinary use.
Fix: have the menu and f find a worktree at MergeWorktreeAt(...) whatever the blocker. Remove it when no merge is under way and HEAD is on origin. Otherwise offer a there. Or remove it once the row leaves the open listings.
Design and architecture
G1.3 cmd/prs/tui_test.go:1000
Reach: Any go test ./cmd/prs on a loaded machine. I ran it twice at load average 52 and it failed both times. The next change that adds latency to m's read makes it fail on idle machines too.
TestEveryKeyThatActsLeavesPrsOnTheListAndAMergedRowMovesToRecentlyClosedAtOnce now races the read that m starts.
The listing has #13 as DIRTY. With this branch, m there starts readThenFix in the background. The branch added readsAs(13, "CLEAN") so that the read ends at the merge question. Line 1000 still sends Press("g", "j", "j", "j", "j", "m", "enter") in one go. When enter arrives before the read lands, it opens #13's document. The test then times out waiting for gh could not merge #13: …. The screen dump shows the document's Description and Checks sections.
The PR body says the suite passed. It passes only where the stand-in read finishes before tuidrive's next keystroke.
Fix: send m alone, then term.WaitFor("merge acme/widget #13 — Count cache hits? [Y/n]"), then enter. Other steps that press m on a row the listing calls blocked need the same treatment.
Prose
G1.4 README.md:267 · README.md:346 · CLAUDE.md:131 · README.md:278
Three README and CLAUDE.md sentences will go stale.
-
README.md:267 says
`m` there asks to merge as before. "As before" describes the repo before this branch, which a later reader cannot see.documentation.md§ "A comment describes the code as it is, never the change or history that produced it" covers this. Rewrite: "prs reads no merge state from Bitbucket, so a Bitbucket row has no menu, andmthere asks to merge." -
README.md:346 says
A fix may not take a key the menu offers itself: `n`, `x`, `u`, `l`, `r`, `m` or `a`.This copiesmenuKeysby hand. The next menu choice makes it wrong, andRefusedFixandprs config showalready print the live list. Rewrite: "A fix may not take a key the menu offers itself, andprs config shownames the fix and the keys it clashes with." -
CLAUDE.md:131 says "A merge left unmerged waits in the worktree, and
fthennopens it again rather than merging twice". README.md:278 says the same thing. Both are true only until the commit runs. After a refused push, the nextnmerges a second time (see the first correctness finding). Once that is fixed, rewrite: "A merge not yet pushed waits in the worktree, committed or not, andfthennopens it again rather than merging twice."
Findings as JSON
{
"used": "Built the branch with `go build ./cmd/prs`. Drove `MergeWorktree.Start`, `CommitAndPush` and a second `Start` from a scratch Go test against a local bare origin, a checkout and a conflicting PR branch, with git config pointed at a scratch file. Ran `go vet ./...` and `go test -count=1 ./...`. Then ran the failing tuidrive test alone twice at load average 52.",
"standards": "Cited `help.md` § \"An error message names what failed, what was expected, and the command that fixes it\", `documentation.md` § \"A comment describes the code as it is, never the change or history that produced it\" and `pull-requests.md` § \"Sentences the diff adds are checked for what goes stale, and the finding carries the rewrite\". Searches for a rule on TUI tests that race an async key found none.",
"findings": [
{
"number": "G1.1",
"section": "correctness",
"sites": [
"internal/pullrequests/merge_worktree.go:166",
"internal/pullrequests/merge_worktree.go:79",
"internal/pullrequests/merge_worktree.go:83",
"internal/board/fix_menu.go:685",
"internal/pullrequests/pullrequest.go:176"
],
"reach": "Someone pushes to the head branch while the user resolves in nvim, or a pre-push hook fails. The PR body expects this: \"The push is a plain `git push origin HEAD:refs/heads/<branch>`, so it fails where the branch moved.\" The user then follows prs's own advice, `f` then `n`.",
"body": "**A refused push throws away the user's conflict resolution, and the footer says the opposite.**\n\n`CommitAndPush` commits before it pushes. When the push fails, the commit has already used up `MERGE_HEAD`. `Merging` then reads false. The footer still says `…; the merge waits in <path>, and f, then n, opens it again`. Pressing `f` then `n` runs `Start`, which hits `case registered:`. The tree is clean, so `Start` runs `git checkout --detach <new head>` and merges again. The resolved merge commit becomes a detached orphan that only the reflog can find, and the conflict is back.\n\nWhat I ran: a scratch test against a local bare origin. Steps:\n\n1. `Start` → `merging=true`.\n2. Resolved `f` by hand and staged it.\n3. Pushed a new commit to `feat` from another clone.\n4. `CommitAndPush` → error. `Merging` → `false`. The worktree's `git log` shows `Merge branch 'main' into feat`.\n5. `Fetch` + `Start` → `merging=true`. `f` reads `<<<<<<< HEAD\\nfeat\\n=======\\nmain\\n>>>>>>> origin/main`. `git status` reads `UU f`.\n\n**The push error also drops git's reason.** `Run` keeps only the first stderr line. For a rejected push that line is `To <remote>`. The user saw `git could not push the merge in …/pr-7 to feat: To …/origin.git`, which names neither \"rejected\" nor \"fetch first\". `help.md` § \"An error message names what failed, what was expected, and the command that fixes it\" also reaches this.\n\nThe same loss happens when an earlier `n` left a merge waiting on a head that has since moved. `Start` returns at `registered && merging` without checking the head. The push is refused, and the next `n` starts over.\n\nFix:\n\n- When the push fails, undo the commit with `git reset --soft HEAD^` plus `git update-ref MERGE_HEAD origin/<base>`, so the merge is still under way. Or keep the commit and have `Start` offer to push a worktree whose `HEAD` already has `origin/<base>` as a second parent.\n- Make the footer say what is actually kept.\n- For `git push`, report the `!` or `error:` line rather than the first one."
},
{
"number": "G1.2",
"section": "correctness",
"sites": [
"internal/board/fix_menu.go:633",
"internal/board/fix_menu.go:260",
"internal/board/fix_menu.go:123",
"internal/pullrequests/merge_worktree.go:174"
],
"reach": "The unmerged menu offers merge fixes with `{worktree}` set to the merge under way, and the README's example `c` hand-to-claude fix names `conflicts`. Such a session finishes the merge in the worktree and pushes. The worktree is also stranded when the user leaves a merge waiting and the conflict is then fixed anywhere else, or the PR is merged or closed.",
"body": "**A merge that ends any way but prs's own push or abort leaves its worktree behind, and no key in prs removes it.**\n\nOnly two prs actions call `MergeWorktree.Remove`:\n\n- the commit-and-push question after `n` (`CommitAndPush`)\n- the `a` abort choice in the unmerged menu (`Abort`)\n\nBoth are reached only from the `n` flow. Once GitHub stops reading the PR as conflicting, the `n` and `a` choices are gone:\n\n- `fixesFor` builds conflict choices only under `case conflictsBlocker`.\n- `f` on a now-clean row says `GitHub reads … as ready, which needs no fix; m merges it`.\n\nSo a worktree that a configured fix committed and pushed from stays at `<worktree_root>/<checkout>/pr-<n>` for good. The same goes for one the user left with esc before the conflict was fixed elsewhere. Each one shows in the checkout's `git worktree list`. The PR body admits leftover worktrees only for a revert, yet they pile up in ordinary use.\n\nFix: have the menu and `f` find a worktree at `MergeWorktreeAt(...)` whatever the blocker. Remove it when no merge is under way and `HEAD` is on origin. Otherwise offer `a` there. Or remove it once the row leaves the open listings."
},
{
"number": "G1.3",
"section": "design",
"sites": [
"cmd/prs/tui_test.go:1000"
],
"reach": "Any `go test ./cmd/prs` on a loaded machine. I ran it twice at load average 52 and it failed both times. The next change that adds latency to `m`'s read makes it fail on idle machines too.",
"body": "**`TestEveryKeyThatActsLeavesPrsOnTheListAndAMergedRowMovesToRecentlyClosedAtOnce` now races the read that `m` starts.**\n\nThe listing has `#13` as `DIRTY`. With this branch, `m` there starts `readThenFix` in the background. The branch added `readsAs(13, \"CLEAN\")` so that the read ends at the merge question. Line 1000 still sends `Press(\"g\", \"j\", \"j\", \"j\", \"j\", \"m\", \"enter\")` in one go. When `enter` arrives before the read lands, it opens #13's document. The test then times out waiting for `gh could not merge #13: …`. The screen dump shows the document's Description and Checks sections.\n\nThe PR body says the suite passed. It passes only where the stand-in read finishes before tuidrive's next keystroke.\n\nFix: send `m` alone, then `term.WaitFor(\"merge acme/widget #13 — Count cache hits? [Y/n]\")`, then `enter`. Other steps that press `m` on a row the listing calls blocked need the same treatment."
},
{
"number": "G1.4",
"section": "prose",
"sites": [
"README.md:267",
"README.md:346",
"CLAUDE.md:131",
"README.md:278"
],
"reach": "",
"body": "**Three README and CLAUDE.md sentences will go stale.**\n\n1. README.md:267 says `` `m` there asks to merge as before``. \"As before\" describes the repo before this branch, which a later reader cannot see. `documentation.md` § \"A comment describes the code as it is, never the change or history that produced it\" covers this. Rewrite: \"prs reads no merge state from Bitbucket, so a Bitbucket row has no menu, and `m` there asks to merge.\"\n\n2. README.md:346 says ``A fix may not take a key the menu offers itself: `n`, `x`, `u`, `l`, `r`, `m` or `a`.`` This copies `menuKeys` by hand. The next menu choice makes it wrong, and `RefusedFix` and `prs config show` already print the live list. Rewrite: \"A fix may not take a key the menu offers itself, and `prs config show` names the fix and the keys it clashes with.\"\n\n3. CLAUDE.md:131 says \"A merge left unmerged waits in the worktree, and `f` then `n` opens it again rather than merging twice\". README.md:278 says the same thing. Both are true only until the commit runs. After a refused push, the next `n` merges a second time (see the first correctness finding). Once that is fixed, rewrite: \"A merge not yet pushed waits in the worktree, committed or not, and `f` then `n` opens it again rather than merging twice.\""
}
]
}This review does not gate merge.
datapointchris
left a comment
datapointchris
left a comment
There was a problem hiding this comment.
PR review — synthesis
Used: the 18 findings of round 1, from 8 review(s), and nothing else
Standards: none, because this step reads only the findings
Findings: 0 correctness · 3 design · 0 prose
Design and architecture
synthesis.1 explains code-architecture.1 · code-architecture.2 · code-architecture.3 · code-architecture.4 · stale-prose.1 · G1.4
A fact is kept twice, in a hand-written copy beside the code or list that owns it. Each copy goes stale or disagrees when the owner changes. The copies: the menu's reserved keys (menuKeys) and the README list of them; the blocker kinds (config.BlockerKinds) and the README list; the GitHub merge-state meaning in mergeMeanings and blockerOf; the shell-quoted placeholder list and fixPlaceholders; the blocker vocabulary in config and board.
synthesis.2 explains fleet-patterns.2 · contracts.1
prs invents its own worktree root and layout instead of using the fleet's $WORKTREE_ROOT index. The default root differs from the fleet's. The pr-<n> name collides with the skill's worktree. The fleet's worktree tools cannot see prs's worktrees.
synthesis.3 explains G1.1 · failure-paths.1
CommitAndPush commits before it pushes. A failed push leaves a committed merge that Start and the footer treat as if no merge were under way. The resolved merge is lost, and the message promises it is kept.
One defect, raised more than once
G1.1 · failure-paths.1: A failed push after CommitAndPush has committed the merge consumes MERGE_HEAD. f then n makes Start detach and re-merge, which discards the resolved merge, while the footer says the merge waits.
G1.4 · stale-prose.1: README.md:346 copies the menu's reserved-key list by hand, and it goes stale when the menu changes.
fleet-patterns.2 · contracts.1: prs ignores the fleet's WORKTREE_ROOT and defaults its worktree root to $XDG_DATA_HOME/prs/worktrees, so the fleet's worktree tools cannot see prs's worktrees.
Shapes as JSON
{
"shapes": [
{
"number": "synthesis.1",
"section": "design",
"explains": [
"code-architecture.1",
"code-architecture.2",
"code-architecture.3",
"code-architecture.4",
"stale-prose.1",
"G1.4"
],
"body": "A fact is kept twice, in a hand-written copy beside the code or list that owns it. Each copy goes stale or disagrees when the owner changes. The copies: the menu's reserved keys (`menuKeys`) and the README list of them; the blocker kinds (`config.BlockerKinds`) and the README list; the GitHub merge-state meaning in `mergeMeanings` and `blockerOf`; the shell-quoted placeholder list and `fixPlaceholders`; the blocker vocabulary in config and board."
},
{
"number": "synthesis.2",
"section": "design",
"explains": [
"fleet-patterns.2",
"contracts.1"
],
"body": "prs invents its own worktree root and layout instead of using the fleet's `$WORKTREE_ROOT` index. The default root differs from the fleet's. The `pr-<n>` name collides with the skill's worktree. The fleet's `worktree` tools cannot see prs's worktrees."
},
{
"number": "synthesis.3",
"section": "design",
"explains": [
"G1.1",
"failure-paths.1"
],
"body": "`CommitAndPush` commits before it pushes. A failed push leaves a committed merge that `Start` and the footer treat as if no merge were under way. The resolved merge is lost, and the message promises it is kept."
}
],
"duplicates": [
{
"defect": "A failed push after `CommitAndPush` has committed the merge consumes MERGE_HEAD. `f` then `n` makes `Start` detach and re-merge, which discards the resolved merge, while the footer says the merge waits.",
"findings": [
"G1.1",
"failure-paths.1"
]
},
{
"defect": "README.md:346 copies the menu's reserved-key list by hand, and it goes stale when the menu changes.",
"findings": [
"G1.4",
"stale-prose.1"
]
},
{
"defect": "prs ignores the fleet's `WORKTREE_ROOT` and defaults its worktree root to `$XDG_DATA_HOME/prs/worktrees`, so the fleet's worktree tools cannot see prs's worktrees.",
"findings": [
"fleet-patterns.2",
"contracts.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/prs",
"pr": 17,
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"experiment": "none",
"dispatch": "20261108T165856Z-pr-review",
"roster": "44e66c70a21627e0922b633d0e7db86363a01f6c",
"harness": "d046ba6e84043927ddeed9c2ffe22b851426a206",
"machine": "archlinux",
"started": "2026-11-08T16:59:52Z",
"finished": "2026-11-08T17:12:49Z",
"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": "00271016dd7d",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 605,
"tokens": {
"input": 54,
"cache_write": 95973,
"cache_read": 1818487,
"output": 17800
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 54,
"cache_write": 95973,
"cache_read": 1818487,
"output": 17800
}
},
"session": "01a1bc1a-b43b-7d0f-a0c7-adf76e0c31ab",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818804937",
"review_id": 5818804937,
"findings": {
"correctness": 2,
"design": 1,
"prose": 1
},
"numbers": [
"G1.1",
"G1.2",
"G1.3",
"G1.4"
],
"rules": {
"proposed": 1,
"keys": [
"rule/testing/a-tui-test-waits-for-what-a-key-opens-before-it-presses-the-next-key"
],
"filed": true,
"why": "",
"path": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records/G1.rules.json"
},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:01:53Z",
"finished": "2026-11-08T17:12:06Z",
"account": "fleetizen",
"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": "f0b52bbf8a48",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 102,
"tokens": {
"input": 22,
"cache_write": 31318,
"cache_read": 288784,
"output": 4346
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 22,
"cache_write": 31318,
"cache_read": 288784,
"output": 4346
}
},
"session": "01a1bc1a-04e0-7e11-b745-9e9dd1bbc854",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818772491",
"review_id": 5818772491,
"findings": {
"correctness": 0,
"design": 0,
"prose": 0
},
"numbers": [],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:00:22Z",
"finished": "2026-11-08T17:02:08Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"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": "283f4014efc3",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 85,
"tokens": {
"input": 18,
"cache_write": 45107,
"cache_read": 272787,
"output": 7069
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 18,
"cache_write": 45107,
"cache_read": 272787,
"output": 7069
}
},
"session": "01a1bc1b-2e61-7170-82bb-d3206a0e77ad",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818773707",
"review_id": 5818773707,
"findings": {
"correctness": 0,
"design": 5,
"prose": 0
},
"numbers": [
"code-architecture.1",
"code-architecture.2",
"code-architecture.3",
"code-architecture.4",
"code-architecture.5"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:01:04Z",
"finished": "2026-11-08T17:02:31Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"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": "efa99c642e6d",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 259,
"tokens": {
"input": 48,
"cache_write": 53953,
"cache_read": 910709,
"output": 11885
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 48,
"cache_write": 53953,
"cache_read": 910709,
"output": 11885
}
},
"session": "01a1bc1b-3393-7d0d-9310-507b0aabd3c4",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818783174",
"review_id": 5818783174,
"findings": {
"correctness": 0,
"design": 3,
"prose": 0
},
"numbers": [
"fleet-patterns.1",
"fleet-patterns.2",
"fleet-patterns.3"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:01:07Z",
"finished": "2026-11-08T17:05:28Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"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": "bc2eb8615f41",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 48,
"tokens": {
"input": 12,
"cache_write": 16767,
"cache_read": 101884,
"output": 2565
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 12,
"cache_write": 16767,
"cache_read": 101884,
"output": 2565
}
},
"session": "01a1bc19-76a1-7132-a244-4e0da16bc0ea",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818768120",
"review_id": 5818768120,
"findings": {
"correctness": 1,
"design": 0,
"prose": 0
},
"numbers": [
"system-design.1"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T16:59:52Z",
"finished": "2026-11-08T17:00:45Z",
"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": "4359b874a436",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 56,
"tokens": {
"input": 18,
"cache_write": 29067,
"cache_read": 198041,
"output": 3860
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 18,
"cache_write": 29067,
"cache_read": 198041,
"output": 3860
}
},
"session": "01a1bc1b-30ba-77b5-a1c7-108f8a967130",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818772306",
"review_id": 5818772306,
"findings": {
"correctness": 0,
"design": 0,
"prose": 2
},
"numbers": [
"stale-prose.1",
"stale-prose.2"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:01:06Z",
"finished": "2026-11-08T17:02:04Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"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": "4239ed2be477",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 64,
"tokens": {
"input": 16,
"cache_write": 24052,
"cache_read": 141487,
"output": 3868
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 16,
"cache_write": 24052,
"cache_read": 141487,
"output": 3868
}
},
"session": "01a1bc19-799e-73e9-890e-e199b258e17c",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818769083",
"review_id": 5818769083,
"findings": {
"correctness": 2,
"design": 0,
"prose": 0
},
"numbers": [
"failure-paths.1",
"failure-paths.2"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/fleet/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T16:59:52Z",
"finished": "2026-11-08T17:01:04Z",
"account": "live",
"api_key_source": "none",
"machine": "queue"
},
{
"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": "3cf737a7c111",
"status": "ran",
"why": "",
"attempts": 1,
"seconds": 125,
"tokens": {
"input": 40,
"cache_write": 52475,
"cache_read": 695713,
"output": 7604
},
"tokens_from": "result",
"tokens_by_model": {
"claude-opus-5-5": {
"input": 40,
"cache_write": 52475,
"cache_read": 695713,
"output": 7604
}
},
"session": "01a1bc1a-af78-7323-aeb3-e62bf1a13176",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818774045",
"review_id": 5818774045,
"findings": {
"correctness": 0,
"design": 1,
"prose": 0
},
"numbers": [
"contracts.1"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:00:30Z",
"finished": "2026-11-08T17:02:38Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
},
{
"label": "synthesis",
"agent": null,
"model": "sonnet",
"effort": "medium",
"experiment": "none",
"arm": "default",
"reason": "STATIC",
"prompt_profile": "pr-reviewer-prompts/synthesis.md",
"prompt_sha": "8832c4b84e09",
"status": "ran",
"why": "",
"attempts": 2,
"seconds": 16,
"tokens": {
"input": 6,
"cache_write": 14183,
"cache_read": 16203,
"output": 2020
},
"tokens_from": "result",
"tokens_by_model": {
"claude-sonnet-5-5": {
"input": 6,
"cache_write": 14183,
"cache_read": 16203,
"output": 2020
}
},
"session": "01a1bc25-9951-7353-85dd-4e871cb86167",
"worklog": "1335.1",
"worklog_refused": "",
"claude_code": "2.1.319",
"review_url": "https://github.com/datapointchris/prs/pull/17#pullrequestreview-5818807487",
"review_id": 5818807487,
"findings": {
"correctness": 0,
"design": 3,
"prose": 0
},
"numbers": [
"synthesis.1",
"synthesis.2",
"synthesis.3"
],
"rules": {},
"head": "7d607f0eabc71e3f94acae361f1a6fdfeef311e0",
"kept": false,
"records": "/home/chris/.cache/run-pr-reviewers/datapointchris-prs-pr17-round1-20261108T165856Z/records",
"started": "2026-11-08T17:12:30Z",
"finished": "2026-11-08T17:12:49Z",
"account": "fleetizen",
"api_key_source": "none",
"machine": "archlinux"
}
],
"rules": {
"proposed": 1,
"keys": [
"rule/testing/a-tui-test-waits-for-what-a-key-opens-before-it-presses-the-next-key"
],
"unfiled": []
},
"kept": []
} |
m on a row whose merge state says GitHub refuses it reads the pull request again, and where it is still blocked opens a menu of fixes rather than asking to merge. A merge GitHub refuses anyway opens the same menu under gh's words, and f opens it on any row. Each choice is one key, so a reflex enter chooses nothing, and no choice exits prs. - Conflicts name the conflicting files from git merge-tree before anything is checked out. n merges origin/<base> into a worktree detached at the head, under worktree_root, and hands it to nvim's diffview. With nothing left unmerged it asks to commit the merge and push it to the head branch as a plain push, then removes the worktree. With paths left unmerged it offers nvim again, git merge --abort, or a configured fix. - A branch behind its base updates with gh pr update-branch. - Failing checks show the failed jobs' log on the Reader, rerun with gh run rerun --failed, or, where GitHub would merge them failing, merge anyway. - A draft is marked ready with gh pr ready. merge_fixes in the config adds a choice per blocker kind. Its command runs in a tmux window opened behind the board, and the row reads row_reads while that window is open, watched by the same tmux poll a review round's window uses. A command's paths and branches are quoted for the shell, because anyone who can open a pull request names its branch, and a command that puts one inside quotes of its own is refused. Bitbucket reports no merge state, so a Bitbucket row keeps asking to merge as before. The gotermboard pin is the head of the pull request adding Question.Choices and OpenWindowBehind, until it releases. Agent-Name: claude-code-unattended Agent-Session: 0ed7f52a-bc57-4308-b45d-2889466add5f Agent-Run: 01a1bbea-ca97-7b16-957f-bd44a01039af Worklog: 1335.1
…ny row A push origin refused, because someone pushed to the branch since, used to leave the merge committed while the footer said it waited, and f then n merged again over the resolution. CommitAndPush now takes the commit back and leaves the merge under way, resolved. git refuses a soft reset during a merge and refuses update-ref on MERGE_HEAD, so the reset comes first and MERGE_HEAD is written where git merge writes it. A failing client's message names git's refused-ref line, as ! [rejected] HEAD -> feat (fetch first), rather than the To <remote> line above it. - Every fix menu looks for a worktree an earlier n left, whatever blocks the pull request or where nothing does, and a aborts its merge and removes it. f reads a ready row again to look for one. - The worktree is pr-<n>-merge, apart from a pr-<n> worktree another tool keeps, and Start leaves alone a worktree it meets on a branch. - A conflict whose head branch is in a fork gets no n, because the push goes to origin. The row reads isCrossRepository, kept out of --json since Bitbucket does not say. - A rerun goes on past a run that fails, and names each run that reran. - mergeMeanings says each merge state's blocker, so blockerOf and the row's word read one table, through mergeStateOf. - The shell-quoted placeholders are a set of their own, which the merge fix's set joins, so declaring one decides whether it is quoted, and ShellQuotedPlaceholders names that set. - Every merge key starts the merge mergeOf pins. - A fix menu offering one key twice opens on neither and says so. - The key test waits for the merge question before pressing enter, since m reads the row again first.
1b48e7c to
bd0f484
Compare
… rebasing n rebases the pull request's head onto origin/<base> in its worktree, rather than merging the base in, so the branch's history stays linear. The worktree is pr-<n>-rebase, added at the head branch fetched into origin/<branch>. Each stop opens nvim, and nvim exiting with nothing unmerged runs git rebase --continue, then opens nvim again at the next stop. Paths left unmerged, or a stop git cannot go on from, offer nvim again, git rebase --abort, or a merge fix. - A finished rebase lists its commits and asks to force-push them, enter meaning yes. The lease is on ORIG_HEAD, the head the rebase started from, so origin refuses the push where anyone pushed since, and the rebase waits in the worktree for p. git's --force-if-includes reads the reflog of a local branch named for the head, which a detached worktree pushes from none of, so it refuses even where nobody pushed. - Every client runs with GIT_EDITOR=true, so rebase --continue keeps each commit's message rather than wait on an editor with no terminal to draw on. - u runs gh pr update-branch --rebase. - A menu on any row offers p for a finished rebase its worktree holds, beside a. Agent-Name: claude-code-unattended Agent-Session: 0ed7f52a-bc57-4308-b45d-2889466add5f Agent-Run: 01a1bc6a-675d-78c1-9fa5-bb9bd307fb23 Worklog: 1335.1
…u and OpenWindowBehind The fix menu was pinned to the library pull request's head commit. v0.42.0 is the first tag holding that commit, with the choice line drawn through the board's painter. Agent-Name: claude-code Agent-Session: 60bee577-11ae-4c13-9517-75a25f6211a6
prs is gaining a menu of fixes for a pull request GitHub will not merge (datapointchris/prs#17). Its conflict fix needs a worktree root, and its Claude choice needs declaring, the way the review keys are. Without this file the menu still works on prs's defaults: worktrees under `$XDG_DATA_HOME/prs/worktrees`, and no Claude choice. This sets `worktree_root` to `~/.worktrees` and adds a `merge_fixes` entry. Key `c` runs `/make-pr-mergeable {number} {blocker}` in a tmux window behind the board, for conflicts, a branch behind and failing checks. It is offered only where `claude` is on PATH. ## What to look at - `configs/common/.config/prs/config.yml.tmpl`: the `command` keeps `{number}` and `{blocker}` inside single quotes. prs fills those two bare, and refuses a command that quotes a path or branch placeholder. `make-pr-mergeable`'s argument hint is `<pr number> [conflicts | behind | failing-checks] [repo]`, and the window opens in the checkout, so the repo needs no argument. ## How it was verified The template, rendered with this machine's registry, loads in prs built from datapointchris/prs#17. `prs config show` printed: ```text worktree /home/chris/.worktrees from worktree_root in the config file fix c hand to claude for conflicts, behind, failing-checks offered ``` The installed prs 0.15.0 refuses the same file, with `field worktree_root not found in type config.file` and `field merge_fixes not found in type config.file`. ## What changes - Once the prs release reads them, `c` in the fix menu opens a Claude window running `/make-pr-mergeable`, and the row reads `claude fixing` while that window is open. - A conflict resolved in nvim adds `~/.worktrees/<repo>/pr-<number>-rebase`, which `worktree list` shows until the rebase is pushed or aborted. ## Decisions, and what they rejected - **`~/.worktrees`, literal, rather than a template value.** `worktree` reads `$WORKTREE_ROOT` and falls back to `~/.worktrees`, and no machine sets `$WORKTREE_ROOT`, so no flag exists to render from. - **No `draft` in the fix's blockers.** The menu's own `r` readies a draft through `gh pr ready`, and `make-pr-mergeable` names no draft case. ## Risk and rollback This merges only after the prs release carrying the fix menu is installed. On an older prs, every `prs` command refuses this config at load. A revert removes the keys, and prs falls back to its defaults. Session 0ed7f52a-bc57-4308-b45d-2889466add5f wrote this pull request.
A pull request GitHub will not merge leaves prs with nothing to offer.
msends the merge, gh refuses it, and the footer prints gh's reason. The fix then happens somewhere else: a checkout, a browser tab or another terminal, each found and opened by hand.This opens a menu of fixes instead.
mon a row whose merge state says GitHub refuses it reads the pull request again. Where it is still blocked, the menu opens rather than the merge question. A merge GitHub refuses anyway opens the same menu under gh's words, andfopens it on any row. Each choice is one key, and no choice exits prs.CONFLICTING,DIRTY)nrebase onto the base and resolve each stop in nvim ·xclose, after askingugh pr update-branch --rebaseUNSTABLE, orBLOCKEDwith a check failing)lread the failed log ·rgh run rerun --failed·mmerge anyway,UNSTABLEonlyrgh pr readyppush a finished rebase ·aabort a stopped one, or remove the worktreemerge_fixesin the config adds choices of your own per blocker kind. Each runs in a tmux window opened behind the board, and the row reads itsrow_readswhile that window is open.What to look at
internal/board/rows.go,mergeMeaningsandmergeStateOf: one table says what each merge state means, its word and the blocker the menu offers fixes for.blockerOfininternal/board/fix_menu.gois a lookup in it plus the failing-checks rule. A draft wins over a conflict, and a conflict over everything else.internal/pullrequests/rebase_worktree.go,Start: the head branch is fetched intoorigin/<branch>, and the worktree is added there, detached. A registered worktree with a rebase stopped, or finished and not pushed, is kept as it is. One on a branch is refused, and one with neither must be clean beforecheckout --detach. A path that is not a worktree is refused rather than replaced.internal/pullrequests/rebase_worktree.go,Continue:git rebase --continueeither stops at the next commit, finishes, or stops where git cannot go on with nothing unmerged. That last one stays stopped, with git's words.internal/pullrequests/rebase_worktree.go,Push: the lease is--force-with-lease=refs/heads/<branch>:<ORIG_HEAD>, the head the rebase started from. git's--force-if-includesreads the reflog of a local branch named for the head, and a detached worktree pushes from none. So git refused that push even where nobody else had pushed.internal/pullrequests/pullrequest.go,runFor: every client runs withGIT_EDITOR=true. Without it,rebase --continuewaits on an editor in a session with no terminal.internal/pullrequests/rebase_worktree.go,ConflictingFiles:git merge-tree --write-tree --name-onlyexits 1 both for a conflict and for a ref it cannot read. The tree id on stdout's first line is what tells a conflict apart.internal/config/config.go,quotedPlaceholders,FillCommandandinsideQuotes: a placeholder declared inquotedPlaceholders({path},{worktree},{head},{base},{log}) is quoted for the shell, and a command that puts one inside quotes of its own is refused at load.internal/board/actions.go,mergeOf,mergeandmergeWork: every merge key starts the mergemergeOfpins. A refused merge reads the pull request again and opens the menu, except where the refusal was a timeout or the host has no merge state.internal/board/tmux_window_watch.go: the tmux poll moved out ofreview_rounds.go, and serves both review windows and fix windows.How it was verified
go test -count=1 ./...passed, withcmd/prsin 11.9s. Two tests drive the built binary through tuidrive against the stand-in GitHub, gh, git, tmux and nvim:TestARefusedMergeOpensTheFixMenuOnItsConflictsWhichRebaseInAWorktreeAndPushstarts with a merge that reads ready and is refused by gh. The second read finds it conflicting, and the test checks that the menu opens under gh's words, naming both conflicting files. The test then walks the rebase:nfetches+refs/heads/cache-metrics:refs/remotes/origin/cache-metricsand adds a detached worktree there atpr-13-rebase. It runsrebase --quiet origin/add-cacheand opens nvim in the worktree.copensnew-window -dwith the worktree handed over, and the row readsclaude fixing.fandnreopen nvim, then runrebase --continue. The stand-in rebase stops once more, and nvim opens again at that stop, three nvim runs in all.--force-with-lease=refs/heads/cache-metrics:<head>and removes the worktree.foffersa, which runsrebase --abortand removes the worktree.TestTheFixMenuUpdatesABranchReadiesADraftAndReadsOrRerunsFailingCheckscoversuon a branch behind, which runsupdate-branch --rebase, andron a draft. Then it coversl,randcon failing checks. Each step checks the arguments the stand-in gh or tmux received.Two assertions were proved able to fail:
Fillin place ofFillCommand, the conflict test fails on the missingFIX_LOG=''.Three scratch checks ran once and were deleted:
RebaseWorktreeran withGIT_EDITOR=falsein the environment:Startstopped at the first commit, andContinuestopped at the second.Continuefinished, andORIG_HEADwas the head the rebase started from.Pushwas refused withstale info. The rebase stayed in the worktree.Abortremoved it. A newStartrebased the newest head, andPushput it on origin over the other clone's commit.GIT_EDITOR=truetaken out ofrunFor,Continuefailed onthere was a problem with the editor 'false'.candx.insideQuoteswas checked on six commands.FillCommandwas checked on a branch nameda;b'c, and on one namedx;rm {number}, which fills as one quoted word and is not filled again.gh api graphqlconfirmedisCrossRepositoryon this pull request.What changes
mon a blocked GitHub row opens the menu rather than asking to merge, after one more read of the pull request. A ready row asks to merge, and so does every Bitbucket row.fis a new key: the fix menu on the row under the cursor, after one read of the pull request.<worktree_root>/<checkout's directory>/pr-<n>-rebase, detached at the head branch.worktree_rootdefaults to$XDG_DATA_HOME/prs/worktrees. The worktree goes once its rebase is pushed or aborted. Until then every menu on that row names it, and offerspfor a finished rebase anda.nmovesorigin/<branch>in the checkout, as the person's owngit fetchwould.n, and the menu says why.urebases the branch on GitHub, throughgh pr update-branch --rebase.GIT_EDITOR=true.merge_fixesandworktree_rootare new config keys.prs config showprints the worktree root and each fix, offered, refused, or not offered for a missing program.prs config path --helpdocuments both.FillCommand. Its placeholders are{number},{repo}and{slug}, none quoted, so an existing command fills unchanged.$TMPDIR/prs-failing-log-<repo>-<n>.txt, which{log}names.Question.ChoicesandOpenWindowBehind, until that releases.Decisions, and what they rejected
ORIG_HEAD. Rejected:--force-with-lease --force-if-includes. From a detached worktree with no local branch of the head's name, real git refused that push withremote ref updated since checkoutalthough nobody had pushed. A lease on the head the rebase started from refuses exactly where someone pushed since, and needs no local branch.git worktree add <path> <branch>. The branch is often checked out in another worktree, where git refuses a second, and a local branch can hold commits nobody pushed.pr-<n>-rebase. A tool keeping its own worktree of a pull request beside it more likely names itpr-<n>, andStartwould otherwise move that one off its branch.mreads the pull request again before choosing the menu over the merge. Rejected: deciding on the listing's merge state, which can be minutes old. GitHub often readsUNKNOWNorBEHINDfor a moment after a push.UNSTABLEwith checks failing offers "merge anyway". GitHub merges anUNSTABLEpull request, because no failing check is required, and prs merged those before this change. OnlyBLOCKEDtakes the merge away.x;rm -rf ~run its tail. Rejected: quoting every value, which breaks'/review-pr {number}', the shape existing commands take. Quoting only what comes from outside prs, and refusing a quoted placeholder, keeps both.worktree_root.OpenWindow, which takes the terminal away from the board as soon as a fix starts.Risk and rollback
A release ships the binary, and a revert removes the menu and the two config keys. A config naming
merge_fixesorworktree_rootis refused by any release before this one, because prs refuses unknown keys. So a config change adding them waits for this release. A pushed rebase rewrites the head branch, and GitHub keeps review comments on the old commits as outdated. A revert leaves any worktree prs added underworktree_root.git worktree listin the checkout names each one, andgit worktree removetakes it.The review
Round 1: #17 (comment)
system-design.1— fixed. The row fragment readsisCrossRepository, carried asHeadInForkand kept out of--json, because Bitbucket's listing does not say. A conflict whose head is in a fork gets non, and its menu says why. Configured fixes andxstay.failure-paths.1— fixed — same asG1.1.failure-paths.2— fixed.rerunFailedgoes on past a run that fails, and the footer names each run that reran, then each failure.stale-prose.1— fixed. The README points atprs config path --helpfor the blocker kinds and atprs config showfor the menu's keys. The comment onblockerno longer counts its kinds.stale-prose.2— fixed — same asG1.4.code-architecture.1— fixed. EachmergeMeaningsentry carries itsblocker,checksBlockandmergesFailing.mergeStateOfholds the draft and conflict override for bothstateMeaningandblockerOf, and merge anyway readsmergesFailing.code-architecture.2— fixed. The quoted placeholders are a set of their own,quotedPlaceholders, which the merge fix's set joins. Declaring a placeholder in one set or the other decides whether it is quoted, andShellQuotedPlaceholders()names that set.code-architecture.3— rejected. The kinds are the config's vocabulary: the words a file writes, refused at load and handed to{blocker}. The board's kinds are defined from the config's constants, so each word lives once.unknownandheldare the board's alone, because no fix is offered for them. Withcode-architecture.1fixed, a new kind costs a constant, amergeMeaningsentry and afixesForcase, and the help text derives the list.code-architecture.4— fixed.askFixopens no menu offering one key twice, and names both choices on the footer. So a built-in keymenuKeysmisses shows, rather than hiding a choice.code-architecture.5— fixed.mergeOfdoes the provider check, the refusal and the pin, andaskMerge, merge anyway and the batch each call it.contracts.1— rejected, for the variable. The help and README example now read~/.worktrees. The standard the finding cites has a tool implement the layers it needs, and forbids a default outside the tool's own XDG directories.WORKTREE_ROOTbelongs to another tool, and prs reads nothing of one machine's setup. A machine whose worktrees live elsewhere names the place inworktree_root, as feat(prs): give the fix menu its worktree root and a Claude fix dotfiles#109 does.fleet-patterns.1— fixed. The design's owner settled on rebasing after the round.nrebases the head onto the base in its worktree, andurunsgh pr update-branch --rebase.fleet-patterns.2— fixed. The worktree ispr-<n>-rebase, andStartleaves alone a worktree it finds on a branch. The default root stays under prs's own data directory, for the reasoncontracts.1gives.fleet-patterns.3— deferred. One quoter shared across tools, and replacing the copies in other repos, is work outside this repo, filed where those repos' work is tracked. prs's copy is the shape the others should match.G1.1— fixed. A refused push leaves the finished rebase in the worktree, andppushes it again.failureLinemakes the footer name git's refused-ref line, such as! [rejected] HEAD -> feat (stale info), rather thanTo <remote>. Real git confirmed both, as "How it was verified" says.G1.2— fixed. Every menu finds the worktree, whatever blocks the pull request or where nothing does.aaborts its rebase and removes it, andppushes a finished one.fon a ready row reads the pull request again to find one. The conflict test covers it.G1.3— fixed. The step pressesm, waits for the merge question, then presses enter.G1.4— fixed. "as before" is gone, and the key list points atprs config show. The sentence on a waiting rebase now covers a refused push.synthesis.1— fixed — same ascode-architecture.1,code-architecture.2andcode-architecture.4.code-architecture.3is rejected, as its line says.synthesis.2— fixed — same asfleet-patterns.2.synthesis.3— fixed — same asG1.1.Session 0ed7f52a-bc57-4308-b45d-2889466add5f wrote this pull request.