Repository navigation
Optimize the gate and the plan helper after the Bun switch #89
Description
Goal
Find and implement code optimizations that make the gate and the plan-lifecycle helper faster on top of the Bun switch (#87). Prove each gain with before and after measurements.
Scope: the gate's scheduling in scripts/ci.mjs, the slowest suites it runs (scripts/tests/plan-cli.mjs and the unit tests), the CI workflow, and the shipped plan.mjs where an optimization lowers consumer latency without changing its contract. Keep the coverage and semantics of every check.
Yolo: the user delegated every decision except the merge to the agent.
Mode: plan-and-implement
Research
Where the time goes now
- Hosted baseline: a
workflow_dispatchrun onmainat the Run the repository's author tooling on Bun 1.4.4 #88 merge (run 39885296173) passed; its full gate took 29.9 s.node scripts/tests/plan-cli.mjstook 17.8 s (59%),bun run test:unit6.6 s,bun scripts/tests/ci-plugin-targeting.mjs --unit2.0 s, andbun scripts/tests/ci-observability.mjs1.5 s. Every other command took less than 0.5 s. This run also covered the dispatch leg that the Run the repository's author tooling on Bun 1.4.4 #88 pull request did not run. - Local baseline (20 cores, Node v24.22.1, Bun 1.4.4, medians of the gate's own
total_ms):--plugin plan-lifecycle13.7 s,--plugin docks13.8 s,--lane repo9.45 s, full gate 23.8 s. scripts/tests/plan-cli.mjs(12.4 s serial on a quiet machine) is one top-level script with about 32 cases that share one scratch git repository and one fake-gh state file. One run starts 198plan.mjsprocesses and 414 fake-gh processes; the spawns take 12.7 s of 13.05 s. Only one case depends on an earlier one: the status-transitions case reuses thefrozenplan of the frozen-steps case. A prototype that runs the 44 cases as isolated units passed every time: 3.5 s with 4 workers and 2.1 s with 8 workers, against 12.4 s serial (quiet machine; logs in$(git rev-parse --git-path docks-review)/yolo-89/PlanCliCost/).bun run test:unit(6.43 s on 4 cores) is bound by one file.scripts/tests/unit/ci-background-task.test.mjstakes 6.42 s because two tests block in turn inside one worker: a 3000 ms busy loop for the pipe-backpressure test and a 2500 ms busy loop for the blocked-lifetime test. The vacuity files run 7 and 4 real gates one after another (about 0.28 s each). The prototype that combines decisions 3 to 6 ran 72 of 72 tests in 1.72 s on 4 cores (3 runs; logs in$(git rev-parse --git-path docks-review)/yolo-89/UnitSuiteCost/).- The shipped
plan.mjsmakes 4 gh calls for each write (edit,status,step,archive) and 5 forretire: a snapshotgh api graphql, a pre-writegh issue view,gh issue edit, and a read-backgh issue view.listmakes 2 (a repository lookup andgh issue list). Internally,gh issue editfirst looks the issue up, and it looks up repository metadata when labels or assignees change, so one write sends about 5 to 6 HTTP requests. On this machine onegh api graphqlcall took about 380 ms and onegh issue view430 to 470 ms (log:$(git rev-parse --git-path docks-review)/yolo-89/gh-calls/).
Decisions
- Run the plan-CLI cases in parallel through Node's test runner. Move the cases into node:test files under
scripts/tests/plan-cli/. Eachtest()gets its own context from a shared harness: a fresh scratch git repository, a fake-gh state file seeded with the five plan labels, and its own environment. The frozen-steps and status-transitions cases become one test.scripts/tests/plan-cli.mjsstays the suite entry, so the gate invocation, the targeting call counts, the test-contract registry entry, and the validator-table row do not change. The entry runsnode --test --test-concurrency=<os.availableParallelism()> <case files>and exits with its status. Node's runner gives each file its own process ("each matching test file is executed in a separate child process", https://nodejs.org/docs/latest-v24.x/api/test.html#test-runner-execution-model). Runtime stays Node, soplan.mjsis still proven on the consumer runtime (Evaluate and adopt the Bun runtime for the repository .mjs tooling #87). Rejected: an async pool inside one file, because every one of the 198run()calls would needawait, and a missedawaitcould turn an assertion into a no-op. Rejected: a bespoke worker runner insideplan-cli.mjs, because Node's runner already gives isolation, concurrency, and reporting. - Teach the validator table walker in
scripts/tests/author-tooling.mjs(validatorTableErrors) to skipscripts/tests/plan-cli/, as it already skipsscripts/tests/unit/: the files run as one suite through thescripts/tests/plan-cli.mjsrow.test:coverageruns the case files directly withnode --test --experimental-test-coverage. Node passesNODE_V8_COVERAGEto child processes (https://nodejs.org/docs/latest-v24.x/api/cli.html#node_v8_coveragedir), soplan.mjscoverage stays measurable. - Replace the busy-loop blockers in the background-task tests with sleeping children (
setTimeout).spawnSyncitself freezes the parent's event loop (the synchronous methods "will block the Node.js event loop, pausing execution of any additional code until the spawned process exits", https://nodejs.org/docs/latest-v24.x/api/child_process.html#synchronous-process-creation), so the busy loop adds nothing but CPU starvation. Commit 692c061 records a flake from that starvation. - Shorten the two blocking windows and keep their margins. Blocked lifetime: a 300 ms child behind a 1000 ms block, assert at least 950 ms awaited, keep the 250 ms tolerance. Pipe backpressure: a 1500 ms block, a 750 ms threshold, best of 3. The writer took at most 190 ms under 4-core load, so 750 ms leaves about 4x margin. A mutant
startTaskthat relays the child's pipes and times the await failed every shortened variant. Rejected: tighter values (700/300 ms), because the margin drops to 1.6x. - Move the two blocker tests into their own files,
ci-background-task-blocked-lifetime.test.mjsandci-background-task-pipe-backpressure.test.mjs, so that Bun's parallel runner can place them on different workers, and register both files inscripts/config/test-contracts.jsonlike the other unit files. - In both vacuity files, start every gate when the module loads with async
spawnand let each test await its own result. The assertions and per-test reporting stay the same. - Cut gh round trips in the shipped
plan.mjswithout changing its contract. (a) Each write sends onegh api graphqlrequest. Its first mutation isupdateIssuewith the new body and, for a claim,assigneeIds(the assignees from the pre-write read plus the acting login), so the body and the claim are one mutation, as the contract's same-write claim requires (UpdateIssueInputhasbodyandassigneeIds, "The Node ID of assignees for this issue", https://docs.github.com/en/graphql/reference/issues). Label repair follows in the same request as discreteaddLabelsToLabelableandremoveLabelsFromLabelablemutations (same reference page), because a fulllabelIdsreplacement would drop a label added concurrently; gh does the same on purpose (cli/clipkg/cmd/pr/shared/editable_http.go,UpdateIssue: "Labels are updated through discrete mutations to avoid having to replace the entire list of labels and risking race conditions"). The mutation fields run serially (https://spec.graphql.org/October2021/: the mutation "selection set should be executed serially"). The last mutation selects the stored body and labels, which replace the read-backgh issue view; the post-write body and label checks, the same ones as today, catch a partial result. Today's helper is not atomic either: gh sends the label additions, the label removals, and the body update as parallel requests. The pre-writegh issue viewstays and also readslabelsandassignees, because the contract says "Read the issue again before each write" and a plan-CLI case tests the race. The body travels as-F body=@<file>from the existingwithBodyFile: gh returns the bytes of an@file unchanged, while any other-Fvalue gets type conversion and{owner},{repo},{branch},:owner,:repo, and:branchplaceholder filling (cli/clipkg/cmd/api/fields.go,magicFieldValue; https://cli.github.com/manual/gh_api). So a body that quotes such a token is stored as written, as with today's--body-file, and the body stays out of the command arguments. A phase label that a status write needs and the repository lacks is created withgh label create, asnewdoes, and then looked up. (b)listreads the repository name and the plan issues in one paged GraphQL query instead of a repository lookup plusgh issue list. Expected counts: 3 gh calls for each write, 4 forretire, and 1 forlistwhen the repository has at most 100 plan issues, plus one page call per further 100 up to the 500-plan cap (gh issue listalso pages internally). Refusal strings stay the same. GitHub failures keep the contract's two forms,gh <command> <subcommand> failed: ...andgh api graphql returned errors: .... Because only shippedscripts/change, step 3 bumpsmetadata.updatedinplan-manager/SKILL.md, asplugins/plan-lifecycle/skills/AGENTS.mdrequires. - Rejected: overlapping gate checks in
scripts/ci.mjs. After decisions 1 to 6, the estimated sequential repo lane is about 4.7 s and the full gate about 8.6 s, so overlap would save about 2 to 4 s. It would touch the timing schema, the phase and task assertions inscripts/tests/ci-plugin-targeting.mjs, the test-contract discovery ofnode([/bun([calls, and the inline failure reporting. - Rejected: Node's compile cache.
node plan.mjstook 25.7 ms without it and 25.0 ms with it;plan.mjscode is only about 2 ms of its start. - Rejected: seeding plan fixtures straight into the fake-gh state instead of running
new. It saved 21% serial time in the prototype, but it would tie fixtures to the stub's issue format, and with decision 1 it would save under 1 s. - Rejected:
retireclosing the issue inside the same mutation request, because a label mismatch would no longer stop the close. Rejected: dropping the pre-write read or the post-write check, because the contract requires both. Rejected: oneupdateIssuewith a fulllabelIdslist, because it would add the lost-update race that gh avoids (decision 7).
Request corrections
Asked: the Goal's scope names the gate's scheduling in scripts/ci.mjs and the CI workflow. Did: no change to either. Reason: decision 8; the workflow already runs the gate in parallel shards, and the gains come from the suites.
Engineering standards
- Step 1
unit_suite_windows:test-coverageprinciples (every assertion still fails when its behavior regresses; checked with the pipe-relaying mutant),code-clarity,commit-discipline. - Step 2
plan_cli_parallel:test-coverageprinciples (no case lost: the per-test count andplan.mjscoverage stay the same or rise),code-clarity,commit-discipline. - Step 3
plan_helper_round_trips:code-clarity(the mutation request names the guarantee it keeps),commit-discipline. - Before code review round 1:
code-reviewself-review withcode-clarity.
Steps
| # | Id | Task | Files | Depends | Effect | Status | Done when |
|---|---|---|---|---|---|---|---|
| 1 | unit_suite_windows | Cut the unit suite's critical path (decisions 3 to 6): sleeping blockers, the shorter windows, the two blocker tests in their own registered files, and gates started at module load in both vacuity files; update the verify note at the top of scripts/lib/ci-background-task.mjs |
scripts/tests/unit/ci-background-task.test.mjs, scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs, scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs, scripts/tests/unit/agent-score-vacuity.test.mjs, scripts/tests/unit/skill-score-vacuity.test.mjs, scripts/config/test-contracts.json, scripts/lib/ci-background-task.mjs | - | local | done | bun run test:unit reports 72 pass, 0 fail across 9 files; taskset -c 0-3 bun run test:unit finishes in at most 2.5 s; with a copy of scripts/lib/ci-background-task.mjs whose launcher relays the child's pipes and times the await, both blocker tests fail; bun scripts/tests/test-contracts.mjs passes |
| 2 | plan_cli_parallel | Split scripts/tests/plan-cli.mjs into node:test case files under scripts/tests/plan-cli/ with a shared per-test harness, keep scripts/tests/plan-cli.mjs as the entry that runs them through node --test in parallel, skip the directory in the validator-table walker, point test:coverage at the case files, and update the scripts/AGENTS.md rows (decisions 1 and 2) |
scripts/tests/plan-cli.mjs, scripts/tests/plan-cli/, scripts/tests/author-tooling.mjs, scripts/AGENTS.md, package.json | - | local | done | node scripts/tests/plan-cli.mjs exits 0 and reports at least one test() per former case; taskset -c 0-3 node scripts/tests/plan-cli.mjs finishes in at most 5 s; bun run test:coverage reports at least 99% line coverage for plan.mjs; bun scripts/tests/author-tooling.mjs passes |
| 3 | plan_helper_round_trips | Send each plan.mjs write as one GraphQL request whose first mutation, updateIssue, carries the body (as -F body=@<file>) and any claim, followed by discrete label mutations, and verify from the returned stored state; read list in one paged GraphQL query (decision 7); make the fake gh handle the new mutations and query and mirror gh's -F value handling (an @ file is read verbatim; any other value gets type conversion and placeholder filling); update the plan-CLI cases whose expected gh command or message changes; bump metadata.updated in plan-manager/SKILL.md |
plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs, plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md, scripts/tests/fixtures/gh-stub/gh, scripts/tests/plan-cli/ | 2 | local | done | node scripts/tests/plan-cli.mjs exits 0; a plan-CLI case edits a body that quotes {owner}, {repo}, {branch}, and :owner and reads it back unchanged; a logged plan-CLI run shows 3 gh calls for each successful edit, status, step, and archive, 4 for retire, and 1 for list (5 for the 500-plan case); plan-contract.md needs no change; plan-manager/SKILL.md metadata.updated names the change date and bun scripts/skills/content-hash.mjs --check-only plugins/plan-lifecycle/skills passes |
Acceptance
| ID | Command | Expected |
|---|---|---|
| a1 | bun run test:unit |
72 pass, 0 fail, across 9 files |
| a2 | node scripts/tests/plan-cli.mjs |
exits 0 |
| a3 | bun run test:coverage |
runs under Node and reports at least 99% line coverage for plan.mjs |
| a4 | bun scripts/ci.mjs |
exits 0 for the repo-wide checks and both plugins (root AGENTS.md: the full gate for repo-wide and multi-plugin changes) |
| a5 | bun scripts/tests/ci-plugin-targeting.mjs |
exits 0 (the non-unit targeting contract, .github/AGENTS.md) |
| a6 | Three interleaved runs each, origin/main in a temporary worktree against the branch, of bun scripts/ci.mjs --plugin plan-lifecycle, --lane repo, and the full gate, comparing the gate's total_ms |
branch medians at least 60% lower for --plugin plan-lifecycle, 35% lower for --lane repo, and 45% lower for the full gate |
| a7 | taskset -c 0-3 bun run test:unit and taskset -c 0-3 node scripts/tests/plan-cli.mjs |
at most 2.5 s and 5 s |
| a8 | The branch's plan.mjs runs this plan's own routine lifecycle commands on #89 (list, show, export, edit, step) with a logging gh wrapper first on PATH |
every command succeeds; each write logs 3 gh calls and list logs 1 |
| a9 | pull request checks | every check passes |
Do not touch
- The plan contract and its refusal strings in
plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md, the pre-write read, the post-write check, theretireclose order, andnew. - The runtime split from Evaluate and adopt the Bun runtime for the repository .mjs tooling #87:
plan.mjsand the plan-CLI suite run on Node; author tooling runs on Bun. - Check scheduling in
scripts/ci.mjsand.github/workflows/ci.yml(decision 8). - Plugin versions, manifests, and marketplace catalogs. This plan makes no release.
docs/point-in-time records.
Open questions
Verification Results
Local runs: Linux x64, 20 cores, Node v24.22.1, Bun 1.4.4. Node test runs set TMPDIR to a directory in the review scratch, because a stray package.json with "type": "commonjs" at the root of this workstation's system temporary directory makes Node load the tests' extensionless ESM shims as CommonJS (recorded in #87).
Step 1 unit_suite_windows (commit 1d2b35e8)
bun run test:unit: 72 pass, 0 fail, across 9 files.taskset -c 0-3 bun test --parallel=4 --timeout=180000 ./scripts/tests/unit/*.test.mjs, 3 runs: 1724, 1726, and 1704 ms (before: 6.43 s three times).- Mutant check: the two blocker test files, pointed at a
startTaskthat relays the child's pipes and times the await, fail both tests (blocked lifetime and pipe backpressure); pointed at the real library, both pass. - The commit passes on its own in a temporary worktree: unit suite 72 of 72,
bun scripts/tests/test-contracts.mjs(contracts=17), the old serialnode scripts/tests/plan-cli.mjs, and biome.
Step 2 plan_cli_parallel (commit 5efaf4ef)
node scripts/tests/plan-cli.mjs: 45 tests, 45 pass (44 former cases plus a newlistorder case).taskset -c 0-3 node scripts/tests/plan-cli.mjs, 3 runs: 3956, 3925, and 3844 ms (before: 12.4 s serial).bun run test:coverage:plan.mjs996 of 1006 lines, the same as before. Branch coverage moved from 282/371 to 280/369: the old shared state reachedstatusFromIssuewith a different mix of issues. No assertion was dropped. The newlistorder case restores theCOMPLETEDbranch and the sort over mixed open and closed plans.bun scripts/tests/author-tooling.mjs,bun scripts/tests/test-contracts.mjs, andbun scripts/tree/guard.mjs: pass.
Step 3 plan_helper_round_trips (commits 267482e5, 21e0faec)
node scripts/tests/plan-cli.mjs: 47 tests, 47 pass. The two new cases: aneditwhose body quotes{owner},{repo},{branch},:owner, and:reporeads back unchanged; a status write on a repository that lacksplan:plannedcreates the label and applies it.- gh calls per successful command in a logged plan-CLI run:
edit3 (24 runs),status3 (9 runs; 5 when labels page or the phase label is created),step3,archive3 (4 when closers page),retire4,list1 (5 for the 500-plan case),showandexport1,new2. Before: 4 for each write, 5 forretire, 2 forlist. bun run test:coverage:plan.mjs99.54% lines, 78.06% branches.bun scripts/skills/content-hash.mjs --check-only plugins/plan-lifecycle/skills: pass;plan-manager/SKILL.mdmetadata.updatedis 2026-10-23.- Real GitHub, through the branch helper with a logging
ghwrapper (a8): on this issue,list1 call,show1,export1,edit3,step3, all successful, and thelistoutput matches the released helper line for line. Throwaway probe issues Probe: plan.mjs GraphQL write path (#89) #91, Probe: plan.mjs write timing (#89) #92, and Probe: plan.mjs write timing breakdown (#89) #93 exercisedstatuswith a claim,edit,step, andretire: 3 calls for each write and 4 forretire; the claim, the phase labels, and a body quoting{owner} {repo} {branch} :owner :repo :branchwere stored correctly. The probes are closed as not planned, and theirplanlabel is removed, solistdoes not show them. - Real GitHub timing, medians: a
statuswrite took 3229 ms of gh time with the released helper (4 calls;gh issue editalone 1895 ms) and 2759 ms with the branch helper (3 calls; the mutation 1721 ms).listtook 1.10 to 1.42 s with the released helper and 0.51 to 0.78 s with the branch helper (5 runs each). - Self-review findings (3 LOW) are fixed in
21e0faec; plan review round 5 passed the one Research sentence that changed with it.
Code review round 1 fix (commit 1b2fe575)
- Finding: label removal ids came from
gh issue view, which reads onlylabels(first:100). Red baseline, review round 1: with the fake gh truncatingissue viewlabels at 100 like real gh,node --test scripts/tests/plan-cli/archive-closer.test.mjs scripts/tests/plan-cli/status.test.mjson21e0faec: 2 failed, 10 passed (the paged-label status case and the new archive case). - After the fix:
node scripts/tests/plan-cli.mjs48 of 48;bun run test:coverageplan.mjs99.54% lines, 78.21% branches;bun scripts/ci.mjsexit 0 (total_ms7201);bun scripts/tests/ci-plugin-targeting.mjsexit 0. - Real GitHub, throwaway probe issue Probe: plan.mjs label removal by snapshot id (#89) #94 through the branch helper:
statusplanned (claim, add, remove) 3 calls,statusongoing 3 calls,retire4 calls; the issue ended closed as not planned with only theplanlabel, which was then removed solistdoes not show it.
Acceptance (final tree 21e0faec; the round 1 fix 1b2fe575 reran a2 to a5)
- a1
bun run test:unit: 72 pass, 0 fail, across 9 files. - a2
node scripts/tests/plan-cli.mjs: exit 0, 47 of 47. - a3
bun run test:coverage: runsnode --test --experimental-test-coverage;plan.mjs99.54% lines. - a4
bun scripts/ci.mjs: exit 0, statuspassed,total_ms7313. It also passes without theTMPDIRsetting (7697 ms): since Run the repository's author tooling on Bun 1.4.4 #88 the unit tests run on Bun, so the straypackage.jsonin the system temporary directory no longer affects the gate. - a5
bun scripts/tests/ci-plugin-targeting.mjs: exit 0. - a6
python3 "$(git rev-parse --git-path docks-review)/yolo-89/bench/compare.py" <origin-main-worktree> 3 <gate-args>, both sides on Bun, three interleaved runs each, medians of the gate's owntotal_ms(measured at267482e5;21e0faecchanges no runtime path):
| Gate | origin/main |
Branch | Change |
|---|---|---|---|
--plugin plan-lifecycle |
12.92 s | 2.54 s | -80.4% |
--lane repo |
9.33 s | 4.44 s | -52.4% |
full gate (-q) |
22.41 s | 7.25 s | -67.7% |
- a7
taskset -c 0-3, 3 runs:bun run test:unit1730, 1739, and 1723 ms;node scripts/tests/plan-cli.mjs4188, 4080, and 4124 ms. - a8: see Step 3 (real GitHub runs).
- a9 pull request checks on Cut the gate to 7 s and a gh round trip from each plan write #95 at the reviewed head
1b2fe575: all 5 pass (resolve validation shards, validation shard (repo), validation shard (core), CI plugin-targeting contracts, validate). The hosted timing artifacts show the core lane's gate at 9.4 s and the repo lane's gate at 6.5 s, against 18.1 s and 10.0 s on Run the repository's author tooling on Bun 1.4.4 #88 (one run each). bun run check:js: exit 0 with 7 warnings and 3 infos, the same counts as onmain.- The Run the repository's author tooling on Bun 1.4.4 #88 runtime switch on
main: aworkflow_dispatchrun at merge commit37852a89(run 39885296173) passed, which covers the dispatch leg that Run the repository's author tooling on Bun 1.4.4 #88's pull-request CI did not run. The release-tag legs (bun scripts/ci-target.mjs release-tag, thenbun scripts/ci.mjs --lane repoand--plugin) first run on the next release tag.
Activity
DocksDocks commentedon Oct 23, 2026
Plan review round 1
Plan-review: repair
updateIssueinput carrying the applicable body, labels, and claim; GitHub’s documentedUpdateIssueInputsupports these fields. Preserve unrelated labels, pre-write comparison, stored-state validation, and verify-before-close ordering. Sources:plan-contract.md(Status, ownership, and frozen state), GraphQL error semantics, GitHub issue schema.metadata.updated, perplugins/plan-lifecycle/skills/AGENTS.md, but the required file is outside every step’s scope. Addplugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.mdand its metadata update to step 3.DocksDocks commentedon Oct 23, 2026
Rejected review findings, round 1
Finding 1 (one
updateIssueinput carrying body, labels, and claim) is accepted in part.updateIssuewith the new body and, for a claim,assigneeIds(the assignees from the pre-write read plus the acting login). The body and the claim then succeed or fail as one mutation.labelIds.labelIdsreplaces every label, so a label that someone adds between the pre-write read and the write would be dropped.gh issue edit, whichplan.mjsuses today, avoids this on purpose. Source: cli/clipkg/cmd/pr/shared/editable_http.go,UpdateIssue: "Labels are updated through discrete mutations to avoid having to replace the entire list of labels and risking race conditions." The repaired plan keeps discreteaddLabelsToLabelableandremoveLabelsFromLabelablemutations afterupdateIssuein the same request.errgroup), and on github.com the assignment goes through a separatereplaceActorsForAssignablerequest beforeupdateIssue. The repaired design runs the mutations serially in one request, and the post-write checks of body, labels, and assignee still catch any partial result.Finding 2 (
SKILL.mdmetadata.updated) is accepted without change: step 3 now ownsplugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.mdand bumpsmetadata.updated, asplugins/plan-lifecycle/skills/AGENTS.mdrequires for a change to shippedscripts/.DocksDocks commentedon Oct 23, 2026
Plan review round 2
Plan-review: repair
plan_helper_round_trips;plan.mjsgraphql()- The plan moves the plan body out ofgh issue edit --body-file, which sends the bytes unchanged. The body becomes agh api graphqlvariable instead. The research never says how the body reaches gh. Today the helper's only GraphQL path isgraphql(), and it passes every variable with-F. gh changes-Fvalues: the gh manual says placeholder values{owner},{repo}, and{branch}"get populated" (https://cli.github.com/manual/gh_api). The gh source shows thatfillPlaceholdersin cli/clipkg/cmd/api/api.gorewrites every{owner},{repo},{branch},:owner,:repo, and:branchsubstring anywhere in the value. It also fails the request on{branch}whenGH_REPOis set. A plan body that quotes such a token would be stored rewritten.plan.mjsitself contains one (owner={owner}). The post-write check would refuse only after the corrupted body is already on the issue, and the contract's retry would rewrite it again. The fake gh reads-Fvalues literally (formFields), so neither the plan-CLI suite nor a8 can catch this. Today's--body-filepath does not have this risk, so the "without changing its contract" claim is unproven for every body write. - In decision 7(a) and step 3, send the body as-F body=@<file>from the existingwithBodyFile. gh'smagicFieldValuein cli/clipkg/cmd/api/fields.goreturns the bytes of an@file exactly, without type conversion or placeholder filling. This also keeps the body out of the command arguments, as--body-filedoes today. Teach the fake gh inscripts/tests/fixtures/gh-stub/ghto read@<file>field values the same way.DocksDocks commentedon Oct 23, 2026
Plan review round 3
Plan-review: pass
None.
DocksDocks commentedon Oct 23, 2026
Plan review round 4
Plan-review: pass
None.
DocksDocks commentedon Oct 23, 2026
Self-review
Scope: the full candidate diff since the merge base with
main(3 commits), reviewed withcode-reviewandcode-clarity. Two-axis mode: the spec source is this issue.Standards
LOW · Maintainability ·
plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjssharedFilesAdvice,printAdvice(introduced)Evidence: both functions are only reformatted (line wrapping); the change does not touch them.
Why it is a problem: the gate lints this path but does not format it, so the reformatting is diff noise in a shipped file.
Suggested fix: restore the original formatting of both functions.
LOW · Maintainability ·
plan.mjsSTORED_ISSUE_FIELDSand the malformed check inwriteIssueIfUnchanged(introduced)Evidence: the write mutation selects
assignees(first:100){nodes{login}}and fails on a malformedassigneesshape, but no code reads the stored assignees.Why it is a problem: unused data adds a way for a write to fail with no benefit; the post-write checks read only the body and the labels.
Suggested fix: select only
bodyandlabels, and drop theassigneesshape check. Research decision 7 says the last mutation selects "body, labels, and assignees", so the sentence changes too and needs a plan-review round first.LOW · Maintainability ·
plan.mjsREPOSITORY_LABELS(introduced)Evidence: the constant changed from a joined string to an array that is joined at its only use site.
Why it is a problem: the change has no reason and widens the diff.
Suggested fix: keep it a joined string with
{name id}.Spec
No findings. Steps 1 to 3 implement decisions 1 to 7; real GitHub runs on this issue and on probe issue #91 show 3 gh calls for each write, 4 for
retire, and 1 forlist.Summary
DocksDocks commentedon Oct 23, 2026
Plan review round 5
Plan-review: pass
None.
DocksDocks commentedon Oct 23, 2026
Code review round 1
Code-review: pass
Standards
gh issue view, whereas real gh selectslabels(first:100)in itsIssueGraphQLquery builder. Consequently,status.test.mjs:69passes despite the shipped helper missing removal IDs beyond that first page. - Mirror gh’s 100-label limit in the stub and retain the regression’s complete stored-label assertion.Spec
statusleaves both phases attached and fails after writing. For an already-owned plan,archive/retirecan instead hit the early return at line 375, bypass verification, and report success or close without removing that phase. - Resolve removal IDs from a complete paginated label connection, preserving discrete mutations and verification before retirement closes the issue.DocksDocks commentedon Oct 23, 2026
Code review round 2
Code-review: pass
Standards
None.
Spec
None.
test: cut the unit suite's critical path from 6.4 s to 1.7 s
test: run the plan-CLI cases in parallel through node --test
perf(plan-lifecycle): cut a gh round trip from every plan write and list
fix(plan-lifecycle): close the three self-review findings on #89
fix(plan-lifecycle): take label removal ids from the paged snapshot
feat: add a project-local benchmark for the plan-lifecycle reviewers