Repository navigation
Conversation
`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
d0891dd to
c82266c
Compare
BagToad
left a comment
There was a problem hiding this comment.
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.
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.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
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 |
|
@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. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
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
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.
|
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. |
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.
|
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. |
|
@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. |
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. |
…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.
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.
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.
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.
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.
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.
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.
|
@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. |
@pokui good question. I'd lean to this being moved to the additional items section. WDYT? Probably best to have a separate issue. |
|
Merged trunk in 3eb4a08. The conflict was in On the type picker, agreed. I will leave that out of this PR and it can go in 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. |
…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

Summary
gh issue createcreates the issue, then applies the deferred fields (type,parent, blocked-by, blocking) in a second step. When one of those mutations
fails,
createRunreturns the error. The deferredPreserveInputsees anon-nil error, writes a recovery file and prints:
The issue URL is never printed. But the issue does exist —
api.IssueCreatealready returned successfully. The message is misleading, and the
--recoverprompt 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
UpdateIssueIssueTypemutation fails with a permissionserror after the issue has been created.
This assigns the deferred-update failure to a separate
updateErrinstead ofthe named return
err, reports it as a warning on stderr, and still prints theissue URL:
Using a separate variable (rather than clearing
err) means the named return isprovably never set on this path, so
PreserveInputcannot fire and no recoveryfile is written.
The issue already exists once
api.IssueCreatereturns, andapi.DeferredUpdateIssuealready joins its failures so that a single failingmutation 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.
deferredUpdateIssueOptionsruns 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 foundtest pinsthis behaviour.
Testing
create with type that cannot be set— added to theTest_createRuntable,stubbing
UpdateIssueIssueTypewith aFORBIDDENerror. Asserts the issue URLis printed to stdout, the warning is printed to stderr, and that
createRunreturns no error (which is what stops
PreserveInputfrom writing a recoveryfile). Verified to fail without the change and pass with it.
go test ./pkg/cmd/issue/...,go vet ./pkg/cmd/issue/create/...andgofmtall pass.
Fixes #13804