Move plan record state into GitHub fields - #15
Merged
Conversation
The plan record carried a frontmatter block that duplicated fields GitHub already owns. Title, status, owner, and both timestamps had two writers and one arbiter, so a closed issue could still display `plan:ongoing`. The body now opens with `<!-- plan-contract: v3 -->` and holds only the human-authored plan. Title, assignee, and timestamps are read from the issue. Status is read from the phase label while the issue is open, and completion is derived from `state` and `stateReason` once it closes, so no field has two writers. Archive stopped writing status and became a verifier: it accepts a closing commit only when an associated pull request merged into the default branch, which closes the direct-push loophole. The v1 and v2 paths are deleted rather than deprecated. Both live records were converted first, so no reader needs a frontmatter parser; `plan.mjs` is 926 lines with a two-outcome classifier. The 120 archived markdown files stay untouched in git history and nothing parses them. Two defects surfaced while driving the tool through its own migration: `check` counted every occurrence of the version marker, so a plan about this contract could not quote its own marker. It now counts standalone marker lines. Applying an export taken before an intervening body write silently reverted recorded step statuses; it cost verified state twice here. An export now records the digest of the body it copied, and an edit whose file came from a superseded body is refused. A first design compared the issue timestamp and was discarded: a phase-only status change is a label write, which moves that timestamp without touching the body, so it would have refused safe edits. A damaged provenance file now stops the edit instead of silently disabling the guard. Also deletes the two dead labels, `plan:finished` and `plan-scheduled`. Closes #14
The v3 cutover edited `plugins/docks/` and `scripts/`, so the selected-plugin gate was the wrong scope: it never lints `scripts/` and never backfills the docks skill hashes. Remote CI failed three checks the local run could not see. Deletes a dead `node:stream/promises` import that predates this branch and was only latent because no lane linted it, orders the new `node:crypto` import as organize-imports requires, and backfills the scaffold reference hash. The stale-export guard also contradicted the contract's "no hashes" sentence. That sentence now says what it means: the record carries no hash, and the one digest that exists detects a stale export copy without authorizing anything. Root AGENTS.md and the shipped template carry byte-identical wording.
… guard Round-1 code review found the archive verifier and the export guard weaker than the record claims. The closing-proof query returned manually linked pull requests, so a collaborator could link any merged pull request and pass the verifier; it now passes `excludeUserLinked:true`, which keeps keyword closers. Verified against the live API: microsoft/vscode#331368 returns its manual link in the plain connection and cli/cli#14073 keeps its keyword closer under exclusion. The reviewer's paired `includeClosedPrs` claim is false and the default stays: cli/cli#14073 returns a merged closer without it, and cli/cli#14156 shows it only adds the closed-unmerged references this verifier must reject. The commit fallback took the last commit closer rather than the current closure, so an issue closed by a commit, reopened, then closed by hand kept stale proof. It now reads only the latest ClosedEvent. A missing sidecar disabled the export guard, which made a copy or a deleted sidecar a silent bypass. Every edit now requires provenance, matching the documented export, edit, check, delete flow. The refreshed digest is staged before the remote write, so a local failure costs one re-export instead of leaving a digest that names a body GitHub already replaced. Check 2 gained the inverse blocked rule: only a blocked plan may open Open questions with a Blocked line. Each fix is mutation-checked; reverting any one of the five fails scripts/tests/plan-cli.mjs. The phase pins now hold whole clauses instead of the words 'byte' and 'unreadable'.
…roof Round-2 code review found both round-1 repairs over-reaching. checkPlan guessed `drafting` whenever it ran without issue context, so a legitimate blocked plan's export failed `check --file` on the very line the contract requires. Phase lives in the labels, so a body alone now derives no status and skips the phase-label rule, both `Blocked:` rules, and the filled-Research rule; `check <issue>` still enforces all four. archivePlan branched exclusively on a non-empty closing-reference connection, so one open keyword reference suppressed a valid closing-commit proof. It now seeks an eligible keyword closer first and consults the commit proof whenever that search finds none. Every refusal message is unchanged. Five compositions gained fixtures: a blocked body under `check --file`, a placeholder-Research body under `check --file`, an ineligible keyword reference beside a valid commit proof, a latest closer that is a pull request, and a merged closer on the second page of closing references. Each was proven load-bearing by reverting the behaviour and re-running the suite. Acceptance row a28 was issue-bound, since file bytes carry no phase, and a30 covers the file-only path it wrongly claimed.
…port Round-3 review found the fixture wrote the issue body straight to a file while acceptance row a30 claims the file came from `plan.mjs export`, so a broken export path would still have satisfied the row. The fixture now runs `export`, asserts the exported bytes equal the issue body, and checks that exported path.
…stic The self-test asserted the manager skill links `references/plan-contract.md` but reported a failure as a missing "v2 plan contract reference", which is the last retired-version wording in the plugin payload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The plan record carried a frontmatter block that duplicated fields GitHub already owns. Title, status, owner, and both timestamps had two writers and one arbiter, so a closed issue could still display
plan:ongoing— which it did, live, on issue #12 after its merge.The body now opens with
<!-- plan-contract: v3 -->and carries only the human-authored plan. Title, assignee, and timestamps are read from the issue. Status is read from the phase label while the issue is open, and completion is derived fromstateandstateReasononce it closes, so no field has two writers.archivestopped writing status and became a verifier: it accepts a closing commit only when an associated pull request merged into the default branch, which closes the direct-push loophole.Legacy deleted, not deprecated
Both live records were converted first, so no reader needs a frontmatter parser.
plan.mjsis 926 lines and its classifier has two outcomes: marker, orunreadablewith no parser attempted. The 120 archived markdown files underdocs/plans/finished/stay byte-untouched in git history, and nothing parses them.Two defects the migration found
Both were found by driving this tool through its own cutover, not by review.
checkcounted every occurrence of the version marker, so a plan about this contract could not quote its own marker. It now counts standalone marker lines.updatedAtand was discarded: a phase-only status change is a label write, which advances that timestamp without touching the body, so it would have refused safe edits. The suite pins that false positive as a regression fixture. A damaged provenance file stops the edit rather than silently disabling the guard.Verification
24 acceptance rows, run against this repository and the installed CLI. Live: #12 reads
finishedfromCLOSED/COMPLETEDwhile carrying onlyplan;archive 12accepted commitadbb40athrough merged PR #13 and stripped the stale phase label;status 12 ongoingrefused on a closed issue;blockedstill demands a reason. Suites:plan-cli.mjsexit 0 with 199 assertion sites,plan-skill-phases.mjsexit 0 withdocs/PLAN.mdbyte-identical to the shipped template fence. Gate:node scripts/ci.mjs --plugin plan-lifecycleexit 0.Also deletes the two dead labels,
plan:finishedandplan-scheduled.Out of scope
plan-lifecycleis bumped to 0.6.0 across both manifests and the versioned catalog, but releasing frommainneeds this to merge first and is deliberately a follow-up plan. No commit here creates itself; the lifecycle still creates zero commits and never pushes.Closes #14