Warn instead of failing when issue fields can't be set by imkp1 · Pull Request #13899 · cli/cli · GitHub
Skip to content

Warn instead of failing when issue fields can't be set - #13899

Open
imkp1 wants to merge 9 commits into
cli:trunkfrom
imkp1:fix/13804-gh-issue-create-errors-after-successful
Open

imkp1 wants to merge 9 commits into
cli:trunkfrom
imkp1:fix/13804-gh-issue-create-errors-after-successful

Conversation

@imkp1

@imkp1 imkp1 commented Jul 16, 2026 •

Copy link
Copy Markdown

Summary

gh issue create creates the issue, then applies the deferred fields (type,
parent, blocked-by, blocking) in a second step. When one of those mutations
fails, createRun returns the error. The deferred PreserveInput sees a
non-nil error, writes a recovery file and prints:

X operation failed. To restore: gh issue create --recover /var/folders/.../gh2543392225.json

GraphQL: monalisa does not have the correct permissions to execute `UpdateIssueIssueType` (updateIssueIssueType)

The issue URL is never printed. But the issue does exist — api.IssueCreate
already returned successfully. The message is misleading, and the --recover
prompt invites a duplicate submission of an issue that was just created.

This is reproducible by creating an issue on a repository where you cannot set
the issue type: the UpdateIssueIssueType mutation fails with a permissions
error after the issue has been created.

This assigns the deferred-update failure to a separate updateErr instead of
the named return err, reports it as a warning on stderr, and still prints the
issue URL:

$ gh issue create --title "Bug" --type "Bug"
! Issue created, but not all fields could be set: GraphQL: monalisa does not have
  the correct permissions to execute `UpdateIssueIssueType` (updateIssueIssueType)
https://github.com/OWNER/REPO/issues/123

Using a separate variable (rather than clearing err) means the named return is
provably never set on this path, so PreserveInput cannot fire and no recovery
file is written.

The issue already exists once api.IssueCreate returns, and
api.DeferredUpdateIssue already joins its failures so that a single failing
mutation does not abort the rest — by the time it returns an error, every
mutation has been attempted.

Scope

Resolving the deferred fields is deliberately left alone. deferredUpdateIssueOptions
runs before any mutation and reports invalid input such as type "Bugz" not found;
that remains a fatal error, and the existing create with type not found test pins
this behaviour.

Testing

  • create with type that cannot be set — added to the Test_createRun table,
    stubbing UpdateIssueIssueType with a FORBIDDEN error. Asserts the issue URL
    is printed to stdout, the warning is printed to stderr, and that createRun
    returns no error (which is what stops PreserveInput from writing a recovery
    file). Verified to fail without the change and pass with it.
  • go test ./pkg/cmd/issue/..., go vet ./pkg/cmd/issue/create/... and gofmt
    all pass.

Fixes #13804

@github-actions github-actions Bot added external pull request originating outside of the CLI core team needs-triage needs to be reviewed labels Jul 16, 2026
`gh issue create` creates the issue and then applies the deferred fields
(type, parent, blocked-by, blocking) in a second step. When one of those
mutations failed, createRun returned the error. The deferred PreserveInput
saw a non-nil error, wrote a recovery file and printed

    X operation failed. To restore: gh issue create --recover <file>

and the issue URL was never printed. The issue did exist, so the message
was misleading and invited a duplicate submission. This is reproducible by
creating an issue on a repository where you cannot set the issue type: the
UpdateIssueIssueType mutation fails with a permissions error.

The issue already exists once api.IssueCreate returns, and
api.DeferredUpdateIssue already joins its failures so one failing mutation
does not abort the rest. Report the failure as a warning on stderr and
still print the issue URL.

Resolving the deferred fields is left alone: it runs before any mutation
and reports invalid input such as `type "Bugz" not found`.

Fixes cli#13804

@BagToad BagToad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @imkp1. I agree with the initial direction, but I want a bit more.

On top of not failing completely when we get an err from a deferred update, I think we shouldn't be showing the type picker at all when a user doesn't have permission. Today, it's gated by whether the repo has types, but clearly that's not enough. We need to gate types showing up based on repo.ViewerCanTriage(), I think. Though a test is in order to ensure I'm correct and that's the right permission.

@BagToad BagToad added gh-issue relating to the gh issue command and removed needs-triage needs to be reviewed labels Jul 17, 2026
The interactive type picker is gated only on the repository having issue
types. A viewer without triage access is offered the picker, picks a type,
and UpdateIssueIssueType then fails after the issue already exists.

Gate the picker on ViewerCanTriage. IssueRepoInfo already selects
viewerPermission, so this costs no extra request, and it skips the
RepositoryIssueTypes fetch for viewers who cannot use the result.

ConfirmIssueSubmission already gates the metadata option the same way.
@imkp1
imkp1 marked this pull request as ready for review July 17, 2026 05:21
@imkp1
imkp1 requested a review from a team as a code owner July 17, 2026 05:21
@imkp1
imkp1 requested a review from BagToad July 17, 2026 05:21
@github-actions

This comment was marked as outdated.

@imkp1

imkp1 commented Jul 17, 2026 •

Copy link
Copy Markdown
Author

@taralove0518-netizen

This comment has been minimized.

@imkp1
imkp1 marked this pull request as draft July 20, 2026 13:49
@imkp1

imkp1 commented Jul 20, 2026 •

Copy link
Copy Markdown
Author

Back in draft to clear the auto-close timer — not a status change, ready for another look whenever you have time.

The one thing I still can't resolve is whether ViewerCanTriage() is the right boundary. It reads the base role and ignores custom roles, so a custom role granting type-setting on a base-READ repo would lose the picker with no fallback. If you know the server-side rule for Issue.viewerCanType, I'll match it; otherwise I'd lean toward keeping the triage gate and letting the warn path catch the gap.

@BagToad

BagToad commented Jul 20, 2026

Copy link
Copy Markdown
Member

@imkp1 Triage permission is fine. Custom roles can be out of scope. The warning closes the gap enough IMO, leaving custom roles as an edge case.

Apologies I haven't had a chance to re-review, but I'll reopen this and remove it from the auto-close so it remains in my PR review queue.

@BagToad
BagToad marked this pull request as ready for review July 20, 2026 20:45
Copilot AI review requested due to automatic review settings July 20, 2026 20:45
@github-actions

This comment was marked as outdated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts gh issue create to treat failures that occur while applying deferred issue fields (type/parent/blocking relationships) as warnings rather than fatal errors, so the command still prints the created issue URL and avoids triggering interactive recovery-file behavior when the issue was already created successfully.

Changes:

  • Gate the interactive “Issue type” prompt behind repo.ViewerCanTriage() to avoid offering a selection that will predictably fail for low-permission viewers.
  • Convert deferred field-application failures (api.DeferredUpdateIssue) from a returned error into a stderr warning while continuing to print the issue URL to stdout.
  • Add table-driven tests covering the new interactive permission gating and the “created but deferred fields couldn’t be set” warning behavior.
Show a summary per file
File Description
pkg/cmd/issue/create/create.go Avoid prompting for issue type without triage access; warn (don’t fail) when deferred field mutations fail after issue creation.
pkg/cmd/issue/create/create_test.go Add tests for type prompt gating (READ vs TRIAGE) and for warning behavior when deferred type update is forbidden.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread pkg/cmd/issue/create/create.go
Resolution of --type/--parent/--blocked-by/--blocking ran after
api.IssueCreate, so a bad flag value failed fatally with the issue
already created: no URL printed, and a recovery file written in
interactive mode.

Resolve into DeferredUpdateIssueOptions before the create mutation and
attach the issue ID afterwards. Only the post-create mutations stay
downgraded to a warning.
@imkp1

imkp1 commented Jul 21, 2026

Copy link
Copy Markdown
Author

Thanks @BagToad — settled then, triage gate stays as-is and custom roles are out of scope. No further change for that thread; 8fbdb9b already has the gate plus READ/TRIAGE tests.

Also pushed 745dc68 for the Copilot review: deferred field resolution now runs before IssueCreate, so a bad --type or unresolvable ref fails without creating an issue. Only the post-create mutations remain warnings. Ready for re-review.

imkp1 added 2 commits July 21, 2026 08:01
Refs resolve in a loop, so cover a later ref failing after an earlier
one succeeded. Excludes the IssueCreate mutation to assert no issue is
left behind.
@pokui

pokui commented Aug 8, 2026

Copy link
Copy Markdown

I left a comment on #13120 but can the type picker also be modified to not select a type i.e none? The web interface doesn't force you to pick a type I don't think the cli tool should either.

@imkp1

imkp1 commented Aug 8, 2026

Copy link
Copy Markdown
Author

@pokui The picker options are just the repo's type names, with no "none" entry, and backing out of the prompt aborts the create. So if a repo has types you have to pick one. Looks like it arrived in v2.94.0, which matches your upgrade.

@BagToad happy to add a "none" option here if you want it in scope, it's small and lands in the block this PR already touches. Otherwise I'll keep this PR to the original fix.

@pokui

pokui commented Aug 8, 2026

Copy link
Copy Markdown

@pokui The picker options are just the repo's type names, with no "none" entry, and backing out of the prompt aborts the create. So if a repo has types you have to pick one. Looks like it arrived in v2.94.0, which matches your upgrade.

Correct. I am happy to create a separate issue if the devs prefer since there may be some discussion as to whether a 'none' type is needed or if the picker moves to the "additional items" section just like where the label picker etc are.

chrislacey89 added a commit to chrislacey89/skills that referenced this pull request Aug 12, 2026
…d wire /qa (#205) (#206)

* Anonymize the downstream client repo in the v1.19.0 changelog entry

The v1.19.0 entry named a client repo and its PR number as the
triggering incident for the review-currency gate. The incident detail
that carries the argument is the shape of what happened (a reviewed
branch growing tenfold post-review), not which repo it happened in.

Replaces the named repo/PR with "a downstream client repo". No other
occurrence remains anywhere in the tree.

* Swap /prd-to-issues §7 to gh's native relationship flags (#205)

gh v2.94.0 (2026-06-10) shipped `gh issue edit --add-blocked-by`, which
takes issue numbers and accepts several per call. #168 shipped on
2026-08-04 and hand-rolled the REST path anyway, so §7 carried a
database-id lookup and a warning about a footgun that only existed
because it bypassed the CLI.

- Replaces the BLOCKER_ID lookup + POST with one `gh issue edit` call.
- Removes the "gh issue list --json cannot see dependency data" claim.
  `--json blockedBy,blocking,parent,subIssues` all work; only
  `isBlocked` errors, and that is what was originally tested.
- Adds the create-vs-edit pitfall: `gh issue create --blocked-by`
  applies relationships as a deferred mutation and, on failure,
  suppresses the issue URL while offering a --recover prompt that
  invites a duplicate. Verified live in 2.97.0 (cli/cli#13899 open).
- Documents total_blocked_by, which distinguishes "blockers all closed"
  from "never wired" — an ambiguity /help and /execute each carry a
  fallback paragraph to cope with.

Reads that must show a per-edge `state` stay on the REST dependency-list
endpoint: the inner node shape of `--json blockedBy` could not be
verified against a live edge, and an unverified jq path does not belong
in a skill.

* Use total_blocked_by to tell "unblocked" from "never wired" (#205)

/execute's Blocked-slice gate, /help's frontier query, and both copies
of Ralph's FRONTIER selection each read only `blocked_by`. That field
counts open blockers, so it reports 0 both for a slice whose blockers
have all closed and for a repo where no edge was ever written. /help
and /execute each carried a prose paragraph conceding the ambiguity was
unresolvable.

It is resolvable: `total_blocked_by` counts all blockers regardless of
state, so total > 0 with blocked_by == 0 means "wired, blockers closed"
and total == 0 means "never wired". Both fallback paragraphs now
describe a check instead of a guess.

Also removes the "dependency data is REST-only / --json isBlocked
errors" claim from all three files. `--json blockedBy,blocking,parent,
subIssues` work as of gh 2.94.0; only the nonexistent `isBlocked`
errors. These reads stay on `gh api` for a narrower and still-true
reason: the summary counts have no --json equivalent and are absent
from the single-issue GET.

* Give /qa create-time labels and native dependency edges (#205)

/qa was the one issue-filing skill #168 never reached. It wrote prose
`## Blocked by` sections that no tool reads, and said nothing about
labels at any point — its single mention of the word was `--label
wontfix` for searching closed issues during the lookback.

The dependency gap is not cosmetic. /execute's Blocked-slice gate reads
the native edges, so a /qa bug filed as genuinely blocked returns an
empty edge list and reads as takeable. Two shipped skills disagreed
about what "blocked" means.

The labeling gap is load-bearing for a reason that only showed up on
verification: GitHub's `parent-issue:` search qualifier does not work.
The sub-issue progress badge links to a query the search backend
rejects, and `has:parent-issue` matched all 131 issues on a repo with
zero sub-issues. A shared label is currently the only way a batch of
related issues stays retrievable.

- New Step 4b reads `gh label list` plus recent issues' labels and
  matches the existing convention; asks before creating anything new.
  No fixed taxonomy — /qa runs in repos it does not own.
- Breakdown filing now wires one `gh issue edit --add-blocked-by` edge
  per prose entry, reconciles the two representations, and degrades to
  prose-only on GHES < 3.19 rather than failing the report.
- Carries the create-vs-edit duplicate-issue pitfall from
  /prd-to-issues §7.

Old 4b (Create the issue) becomes 4c; the one cross-reference at the
destination-check step is updated to match.

* State the gh >= 2.94.0 floor and record v1.20.0 (#205)

The relationship flags this change now depends on landed in gh 2.94.0
(2026-06-10), and issue dependencies need GHES 3.19+. Skill Kit had
never stated a tool version floor; using the flags unconditionally
requires one.

The alternative — keeping a REST fallback for older gh — was rejected
because it would preserve the exact mechanism this change exists to
delete, permanently, to serve a shrinking population.

* Move every dependency read to the CLI after verifying the node shape (#205)

The first pass left the reads on `gh api` for one reason: the inner node
shape of `--json blockedBy` had no live edge to verify against, and an
unverified jq path should not ship in a skill.

Verified it by wiring one throwaway edge on #205 and removing it:

  nodes:      {id, number, state, title, url}
  state:      GraphQL enum — uppercase OPEN/CLOSED (REST uses lowercase)
  totalCount: counts blockers in EVERY state (blocker was CLOSED, count 1)

totalCount is therefore the CLI equivalent of total_blocked_by, which
removed the last reason to keep raw REST anywhere on this path.

- §7 verify, §9 frontier, /execute's gate, /help's frontier, and both
  Ralph FRONTIER copies now use `gh issue list` / `gh issue view`.
- §9's second confirming call is deleted: reading edges directly is not
  subject to the post-write staleness window it existed to close.
- The uppercase/lowercase `state` split is documented as a sharp edge —
  a jq filter copied between the two matches nothing, silently.
- /help's classification ladder (signals #3, #4 and the note below it)
  moves from `blocked_by == 0` to an empty `open_blockers`, so the
  router does not describe a field the frontier no longer emits.

Every documented command was executed against this repo before commit.

* Sync SYSTEM-OVERVIEW's frontier query and its four bundled copies (#205)

SYSTEM-OVERVIEW.md carried a third copy of the Ralph FRONTIER query and
a verbatim restatement of the false `isBlocked` claim. Because it is
bundled into correct-course, help, improve-pipeline, and
setup-ralph-loop via scripts/skill-references.manifest, leaving it would
have shipped the corrected guidance in five skills and the stale
guidance in four references/ copies of the same repo.

This is the drift CLAUDE.md's inventory-sync rule exists to prevent —
found by sweeping every surface that describes the mechanism, not just
the skills that implement it.

Also corrects the v1.20.0 changelog bullet, which named the REST field
(`total_blocked_by`) after the implementation had moved to the CLI
equivalent (`blockedBy.totalCount`).

* Correct the v1.20.0 net-line claim (#205)

The entry said net line count across the five skills is negative. It is
not: the mechanism migration is net -9 across the four skills that had
it, but /qa adds +37 because there the change is new capability, not a
swap. Measured rather than asserted.

* Close the /triage-issue labeling bypass (#205)

/qa Step 3.5 delegates per-issue to /triage-issue, whose issue replaces
the lightweight one Step 4 would have filed — so it skips Step 4b's new
labeling along with the rest of Step 4. A QA session mixing lightweight
and deep issues would emit some labeled and some not.

That is worse than the uniform absence this PR started from, because
inconsistency reads as intentional rather than as a gap.

#205 scoped /triage-issue out on evidence grounds, which stands: this
adds no new capability, only the pointer to /qa Step 4b's rule so the
delegated path does not silently drop it. Found by /pre-merge Dim 9.

* docs: compound — a claim asserted as 'by construction' that nothing constructs

#168 probed `gh issue list --json isBlocked`, got Unknown JSON field,
and recorded the broader conclusion that `--json` cannot see dependency
data at all. That was false when written: gh 2.94.0 had shipped
`--json blockedBy,blocking,parent,subIssues` two months earlier. The
claim propagated to nine sites and survived a week, found only because a
/research pass happened to read `gh --help`.

Alongside it, #168 asserted the selection sites 'agree by construction
rather than by coincidence.' They are hand-maintained in five files with
no test relating them.

Two decays, one cause: verification has no provenance or expiry, and
asserting a consistency property is cheaper than enforcing it while
reading identically. A claim believed verified, asserted consistent, and
copied nine times is maximally protected from correction.

Records that PR #206 fixed the claims and NOT the mechanism — the idiom
still lives in five hand-maintained files. Prevention names the repo's
own precedent (test-review-currency-marker.sh extracts both halves and
round-trips them rather than restating either).

Also updates secondhand-source-proposal-specificity with a third
instance, which recurred while implementing the fix for the second, and
moves it from 'a pattern worth naming' to a systemic claim.

* docs: compound — record why a test, not a Claude hook, enforces this

The Prevention section named the extract-and-compare pattern without
saying why hook-based enforcement is the wrong tool — the first question
a reader asks, since Skill Kit ships hook enforcement via /init-pipeline
and reaching for it looks natural.

The line: hooks gate session state that does not survive to commit time
(was this classified BEFORE code was written — .claude/.tdd-active exists
only during the session). Tests gate anything verifiable from the tree.
Cross-file agreement is entirely tree-verifiable, so a hook narrows who
the check protects instead of widening it.

Three reasons, one of them observed rather than argued: the session that
produced this entry edited SKILL.md files four ways — Write, Edit,
python3 heredocs via Bash across three files, and gh pr edit. A
PostToolUse hook on Edit|Write would have missed every python3 edit,
which were the multi-site mechanical rewrites most likely to cause the
drift being guarded against.

* Construct the agreement the prose asserts (#205)

/help and SYSTEM-OVERVIEW both claim the selection sites "agree by
construction rather than by coincidence." Nothing constructed it — the
idiom is hand-maintained across five files, and the assertion is what
stopped anyone from checking.

scripts/test-selection-idiom-consistency.sh now does, wired into
lefthook pre-push and CI. Four checks, each extracting real text from
real files rather than restating it:

  1. the three FRONTIER copies normalize to one value
  2. no `.state == "open"` compared against a blockedBy read (the
     uppercase/lowercase split that matches nothing, silently)
  3. `isBlocked` never resurfaces as a capability claim in a skill
  4. every adjacent `/skill` Step N reference resolves to a real heading

Deliberately NOT asserted: byte-identity across all nine occurrences.
They legitimately differ — §9 tests for an empty set, /execute projects
{number,title}, /help projects .number. Forcing uniformity would get the
suite deleted the first time someone needed a different projection.

Mutation-tested before commit. Drifting a FRONTIER --limit, lowercasing
a state predicate, appending an isBlocked claim, and adding a dangling
Step 9 reference each produced exactly one failure; tree restored clean
after each. A contract suite observed only passing is itself an
unverified claim, which is the error this whole PR is about.

Check 4 earned its place immediately: it caught the /triage-issue ->
/qa "Step 4b" coupling introduced earlier in this same PR (pre-merge
pass-2 Concern 1), now referenced by name instead of number.

Also updates the compound entry: its prescription now ships enforced,
and its Shelf Life ties the suite and the claim so they retire together.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 17, 2026
Five things the repository asserted without backing: a license it never
included, tests nothing ran, a flag the prompt recognized and then dropped, a
cleanup scope stated nowhere, and a concurrency guarantee stronger than the
guard actually gives.

- LICENSE. The README said MIT and plugin.json declared it, but the text was
  never in the tree, so GitHub detected no license and the claim granted
  nothing. Root only: plugin.json already carries the SPDX identifier for the
  plugin itself, and a second copy under plugins/ is a second thing to drift.

- CI, in .github/workflows/tests.yml. The two offline suites gate every push
  and pull request across Node 18 — the documented floor — and 22, on Linux and
  macOS. That matters most for unit.mjs, which is the only thing holding six
  version literals in step across four files; a check nobody runs is one that is
  eventually wrong.

  The integration suite runs weekly and on demand instead. It needs the network
  and an upstream PR that still exists, and neither should be able to block a
  commit — but it is also the only test that proves the worktree diff is
  GitHub's diff, so leaving it to memory was the larger risk.

  It stubs `codex` there. The suite never invokes Codex by design; it needs the
  binary only because it gates on `doctor`, which refuses an unhealthy
  toolchain. Without the stub every CI run would have exited 0 as a silent SKIP,
  which is worse than no job at all. Verified before committing: the stub
  satisfies the preflight, and the suite passes 10/10 against cli/cli#13899 with
  an isolated XDG_CACHE_HOME.

- review.md listed --profile among the recognized flags in step 2 and left it
  out of the command line in step 5, so the script's -p support was reachable
  from the script and not from the command. Now forwarded.

- clean.md and the README now say that `--pr` with a bare number is not scoped
  to the current repository the way `review 42` is: it selects that number in
  every repository the manifest knows. Documented rather than changed — it is
  usually what is wanted when clearing up, and the dry run names the repository
  each entry lives in before anything is confirmed.

- The in-flight guard was oversold. The README read as unconditional and the
  shipped plugin README said a running review is left alone "entirely", while
  the window that stays open lived only in 0.7.0's commit message: a clean that
  took its snapshot before a review recorded itself cannot hold back a marker
  that does not exist yet. Both now say guard rather than lock, and the README
  states the residual race and why closing it means locking.

Also removes an empty 69955/ left in the root by an earlier session. Git does
not track empty directories, so it never appeared in `git status` and could not
have been committed away by accident.

No version bump: this is work, not a release. `doctor` hashes the installed copy
against its source and flags the review.md edit as stale regardless of the
number, which is the detector that applies here — the stamp closes the other
gap, a session holding prompts older than a cache that has since been refreshed,
and that one only means anything once the version has actually moved.

Known and unfixed: `post` has no comment-size guard. GitHub caps a comment body
at 65,536 characters, so a long review on a large PR fails at `gh` — cleanly,
with its error surfaced, but only after the paid run has already been spent.

Tests: unit and regression unchanged at 101 assertions and passing; integration
10/10 against a live PR.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 20, 2026
Caught by the suite itself, on the first dispatched run after the release: three
assertions failed and a fourth passed for the wrong reason.

`clean` now refuses a removal it was not shown, so the final cleanup was
declined and the worktree, refs, and manifest entry all survived a step that
claimed to remove them. The suite now takes the same two steps the command
takes. Reading the digest straight back from the dry run is right here and would
be wrong in clean.md, where the point is that a person sees the plan first.

The branch assertion looked for `codex-pr/13899`, which 0.8.0 renamed. It
reported success because nothing of that name existed — the failure mode where a
test that has stopped checking anything is indistinguishable from a test that
passes. Both branch names now come from the prepare JSON, so the suite cannot
drift from the naming again.

Verified against cli/cli#13899: 11 passed, 0 failed, and the run leaves the real
cache untouched — which is the property the isolation added in 0.8.0 exists for
and could not have been observed before it.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 20, 2026
The last of the external review's findings, plus the packaging it asked for.
None of these are on the publish path, because there is no longer one.

- `clean` fed `entry.worktree` straight to `rmSync(recursive)`. The manifest is
  a JSON file in a cache directory — hand-edited when something goes wrong,
  writable by anything running as this user — so a recursive delete trusting it
  was only safe while nothing had touched it. It now recomputes where a worktree
  for that entry must be and refuses any other path, keeping the entry for a
  retry. The regression fixtures had to become faithful for this: they invented
  convenient worktree paths and so had been exercising a layout `prepare` never
  produces.

- Manifest mutation is serialized by a lock file. Writing was already atomic —
  temp file, rename over — which is a different problem: every mutation reads
  the whole manifest, edits one entry and writes it all back, so two overlapping
  ones meant the second silently discarded the first. `sweep` prepares several
  PRs at once and a background review re-records its own entry mid-clean, so
  this was the normal case. The lock expires after ten seconds, because a lock
  that outlives its holder is its own failure and nothing here holds it for more
  than a read, an edit and a rename. Tested with eight concurrent writers.

- A manifest that cannot be read was indistinguishable from one that is not
  there. Only ENOENT means first run now; EACCES and friends say so, rather than
  presenting as an empty queue to a command that would then re-create branches
  it already owns.

- `review --no-prepare` took the manifest on trust. The gap between preparing
  and reviewing is where a clean runs and where a branch gets checked out
  somewhere else, and a review of whatever is there now — labelled with the head
  the manifest remembers — is wrong about which commits it read. It now verifies
  the worktree exists, is a worktree of its own, and sits at the recorded head
  and merge-base.

- The cache is created 0700 and reviews and the manifest 0600, with existing
  installs repaired by chmod, since mkdir ignores a mode for a directory that is
  already there. These were at the process umask: 0755 and 0644, world-readable
  for saved reviews of code that is often not public.

- `list` no longer turns a manifest entry missing a field into a stack trace.
  It is the command people reach for when things look wrong.

- `sweep` drew from one queue while `list` drew from two, so a batch could be
  chosen from a set the user was never shown. It now gathers review-requested
  and assigned, de-duplicates on `url` before taking `--limit` — two 50-item
  searches merge to as many as 100 rows, and a PR in both queues would otherwise
  spend two paid runs on itself — and fetches the `createdAt` that `--order
  newest` sorts on and never had. `list` says the same about its own merge.

- `review --dry-run` prepared a worktree before printing the command it "would"
  run, which is what the user was avoiding.

- The integration suite claimed the diff was exactly GitHub's while comparing
  file names and totals, which two different diffs can share. Measured first:
  against cli/cli#13899 the patches are byte-identical apart from blob-hash
  abbreviation, 11 characters from the API and 8 from local git. It now compares
  the whole patch with only that normalized — 13 assertions, up from 11.

- CI runs both plugin manifest validators, and the actions are pinned to commit
  SHAs. A major tag is mutable and this workflow has a token; the trailing
  version comment is what Dependabot rewrites, so updates still arrive as pull
  requests.

- Adds CHANGELOG.md, and the releases are tagged.

Tests: unit passing; regression 100, up from 97 — concurrent writers, the
recursive-delete refusal, and both halves of the --no-prepare check. Integration
13/13 against a live PR.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 21, 2026
Five things the repository asserted without backing: a license it never
included, tests nothing ran, a flag the prompt recognized and then dropped, a
cleanup scope stated nowhere, and a concurrency guarantee stronger than the
guard actually gives.

- LICENSE. The README said MIT and plugin.json declared it, but the text was
  never in the tree, so GitHub detected no license and the claim granted
  nothing. Root only: plugin.json already carries the SPDX identifier for the
  plugin itself, and a second copy under plugins/ is a second thing to drift.

- CI, in .github/workflows/tests.yml. The two offline suites gate every push
  and pull request across Node 18 — the documented floor — and 22, on Linux and
  macOS. That matters most for unit.mjs, which is the only thing holding six
  version literals in step across four files; a check nobody runs is one that is
  eventually wrong.

  The integration suite runs weekly and on demand instead. It needs the network
  and an upstream PR that still exists, and neither should be able to block a
  commit — but it is also the only test that proves the worktree diff is
  GitHub's diff, so leaving it to memory was the larger risk.

  It stubs `codex` there. The suite never invokes Codex by design; it needs the
  binary only because it gates on `doctor`, which refuses an unhealthy
  toolchain. Without the stub every CI run would have exited 0 as a silent SKIP,
  which is worse than no job at all. Verified before committing: the stub
  satisfies the preflight, and the suite passes 10/10 against cli/cli#13899 with
  an isolated XDG_CACHE_HOME.

- review.md listed --profile among the recognized flags in step 2 and left it
  out of the command line in step 5, so the script's -p support was reachable
  from the script and not from the command. Now forwarded.

- clean.md and the README now say that `--pr` with a bare number is not scoped
  to the current repository the way `review 42` is: it selects that number in
  every repository the manifest knows. Documented rather than changed — it is
  usually what is wanted when clearing up, and the dry run names the repository
  each entry lives in before anything is confirmed.

- The in-flight guard was oversold. The README read as unconditional and the
  shipped plugin README said a running review is left alone "entirely", while
  the window that stays open lived only in 0.7.0's commit message: a clean that
  took its snapshot before a review recorded itself cannot hold back a marker
  that does not exist yet. Both now say guard rather than lock, and the README
  states the residual race and why closing it means locking.

Also removes an empty 69955/ left in the root by an earlier session. Git does
not track empty directories, so it never appeared in `git status` and could not
have been committed away by accident.

No version bump: this is work, not a release. `doctor` hashes the installed copy
against its source and flags the review.md edit as stale regardless of the
number, which is the detector that applies here — the stamp closes the other
gap, a session holding prompts older than a cache that has since been refreshed,
and that one only means anything once the version has actually moved.

Known and unfixed: `post` has no comment-size guard. GitHub caps a comment body
at 65,536 characters, so a long review on a large PR fails at `gh` — cleanly,
with its error surfaced, but only after the paid run has already been spent.

Tests: unit and regression unchanged at 101 assertions and passing; integration
10/10 against a live PR.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 21, 2026
Caught by the suite itself, on the first dispatched run after the release: three
assertions failed and a fourth passed for the wrong reason.

`clean` now refuses a removal it was not shown, so the final cleanup was
declined and the worktree, refs, and manifest entry all survived a step that
claimed to remove them. The suite now takes the same two steps the command
takes. Reading the digest straight back from the dry run is right here and would
be wrong in clean.md, where the point is that a person sees the plan first.

The branch assertion looked for `codex-pr/13899`, which 0.8.0 renamed. It
reported success because nothing of that name existed — the failure mode where a
test that has stopped checking anything is indistinguishable from a test that
passes. Both branch names now come from the prepare JSON, so the suite cannot
drift from the naming again.

Verified against cli/cli#13899: 11 passed, 0 failed, and the run leaves the real
cache untouched — which is the property the isolation added in 0.8.0 exists for
and could not have been observed before it.
dumplingsensei added a commit to dumplingsensei/codex-pr-reviewer that referenced this pull request Aug 21, 2026
The last of the external review's findings, plus the packaging it asked for.
None of these are on the publish path, because there is no longer one.

- `clean` fed `entry.worktree` straight to `rmSync(recursive)`. The manifest is
  a JSON file in a cache directory — hand-edited when something goes wrong,
  writable by anything running as this user — so a recursive delete trusting it
  was only safe while nothing had touched it. It now recomputes where a worktree
  for that entry must be and refuses any other path, keeping the entry for a
  retry. The regression fixtures had to become faithful for this: they invented
  convenient worktree paths and so had been exercising a layout `prepare` never
  produces.

- Manifest mutation is serialized by a lock file. Writing was already atomic —
  temp file, rename over — which is a different problem: every mutation reads
  the whole manifest, edits one entry and writes it all back, so two overlapping
  ones meant the second silently discarded the first. `sweep` prepares several
  PRs at once and a background review re-records its own entry mid-clean, so
  this was the normal case. The lock expires after ten seconds, because a lock
  that outlives its holder is its own failure and nothing here holds it for more
  than a read, an edit and a rename. Tested with eight concurrent writers.

- A manifest that cannot be read was indistinguishable from one that is not
  there. Only ENOENT means first run now; EACCES and friends say so, rather than
  presenting as an empty queue to a command that would then re-create branches
  it already owns.

- `review --no-prepare` took the manifest on trust. The gap between preparing
  and reviewing is where a clean runs and where a branch gets checked out
  somewhere else, and a review of whatever is there now — labelled with the head
  the manifest remembers — is wrong about which commits it read. It now verifies
  the worktree exists, is a worktree of its own, and sits at the recorded head
  and merge-base.

- The cache is created 0700 and reviews and the manifest 0600, with existing
  installs repaired by chmod, since mkdir ignores a mode for a directory that is
  already there. These were at the process umask: 0755 and 0644, world-readable
  for saved reviews of code that is often not public.

- `list` no longer turns a manifest entry missing a field into a stack trace.
  It is the command people reach for when things look wrong.

- `sweep` drew from one queue while `list` drew from two, so a batch could be
  chosen from a set the user was never shown. It now gathers review-requested
  and assigned, de-duplicates on `url` before taking `--limit` — two 50-item
  searches merge to as many as 100 rows, and a PR in both queues would otherwise
  spend two paid runs on itself — and fetches the `createdAt` that `--order
  newest` sorts on and never had. `list` says the same about its own merge.

- `review --dry-run` prepared a worktree before printing the command it "would"
  run, which is what the user was avoiding.

- The integration suite claimed the diff was exactly GitHub's while comparing
  file names and totals, which two different diffs can share. Measured first:
  against cli/cli#13899 the patches are byte-identical apart from blob-hash
  abbreviation, 11 characters from the API and 8 from local git. It now compares
  the whole patch with only that normalized — 13 assertions, up from 11.

- CI runs both plugin manifest validators, and the actions are pinned to commit
  SHAs. A major tag is mutable and this workflow has a token; the trailing
  version comment is what Dependabot rewrites, so updates still arrive as pull
  requests.

- Adds CHANGELOG.md, and the releases are tagged.

Tests: unit passing; regression 100, up from 97 — concurrent writers, the
recursive-delete refusal, and both halves of the --no-prepare check. Integration
13/13 against a live PR.
@BagToad

BagToad commented Sep 1, 2026

Copy link
Copy Markdown
Member

@imkp1 apologies for the delays. There's a merge conflict on this PR that needs resolving now, but after that I can take another review of this.

@BagToad

BagToad commented Sep 1, 2026

Copy link
Copy Markdown
Member

Correct. I am happy to create a separate issue if the devs prefer since there may be some discussion as to whether a 'none' type is needed or if the picker moves to the "additional items" section just like where the label picker etc are.

@pokui good question. I'd lean to this being moved to the additional items section. WDYT? Probably best to have a separate issue.

@imkp1

imkp1 commented Sep 1, 2026

Copy link
Copy Markdown
Author

Merged trunk in 3eb4a08. The conflict was in create.go only, where the deferred field resolution now sits just above the params map that trunk switched to map[string]any. Two of this PR's tests also picked up trunk's new promptStubs signature. Build, vet and the issue create tests pass locally. Ready for another look.

On the type picker, agreed. I will leave that out of this PR and it can go in a separate issue.

@pokui

pokui commented Sep 1, 2026 •

Copy link
Copy Markdown

Correct. I am happy to create a separate issue if the devs prefer since there may be some discussion as to whether a 'none' type is needed or if the picker moves to the "additional items" section just like where the label picker etc are.

@pokui good question. I'd lean to this being moved to the additional items section. WDYT? Probably best to have a separate issue.

@BagToad I agree. It makes the most sense to find it where we find labels. I'll crate a fresh issue. Thanks.

@imkp1

imkp1 commented Sep 1, 2026

Copy link
Copy Markdown
Author

juanquijandria added a commit to juanquijandria/prisma-harness that referenced this pull request Oct 9, 2026
…for changes counts from anyone

Second half of ticket 4 of the 0.9.1 plan, plan record ready-verdict
(notas/planes/plan-0.9.1-b.md).

The cause of the Windows case. The probe of cb40d2f showed that jq on
Windows ends each line with a carriage return and that the shell drops
it only from the last line. A message that names two PRs sent the
first number to pr-ready.sh with the carriage return attached, which
is a usage error, so the claim went unmeasured and the stop went
through. The public CI of the previous commit showed the predicted
reds on Windows only, case 5 and 31 of the claim gate, cases 49 and 50
of pr-ready.sh and case 8 of the index gate. Case 9 of the index gate
passed there against the prediction, with the cause not verified, and
its line is kept.

Built here:
- Each list a hook reads line by line out of jq drops the carriage
  return, the claims, the CI runs, the other PRs of the author, and
  the Read calls and failed calls of the index gate. A carriage
  return injected at each of the five sites on this Mac turns its case
  red without the fix and leaves it green with it.
- After the fidelity review, a request for changes stops the verdict
  whoever made it, listed in PRISMA_PR_REVIEWERS or not, and names who
  asked and when (case 52). A draft with --at is not judged (case 51),
  and an English ready to merge beside a denied one stays a claim
  (case 32). CHANGES_REQUESTED is one constant, the denial cases have
  a function of their own, and the README says that with --at a review
  is read in the state it has today.

Measured on 09/10: on public PRs of today, cli/cli#14355 in draft and
cli/cli#13665 blocked went from READY with main to NOT READY with this
branch, each with its reason, and cli/cli#13899 now names the reviewer
whose request for changes stands. jqlang/jq#3664 and #3663 stay READY
with both. Thirteen mutations, each red on its own case on this Mac.

Left out on purpose: the draft and blocked state of the past through
the timeline, requested reviewers and code owners, the shape of each
element inside a list, and jq calls that return one value.

The comments check of the push counts the one-line headers of new
files and two case patterns of format-gate-tests.sh, as in the
previous commits. This commit adds no comment.

Claude-Session: https://claude.ai/code/session_01BBBntBuurTTiaaa1rSvZyP

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team gh-issue relating to the gh issue command ready-for-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh issue create errors after successful creation when issue type can't be set

5 participants