From 9af4b09217f02e9ef2d79f6c046b3992d33a5245 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:12:25 -0300 Subject: [PATCH 01/16] fix(plan-lifecycle): refuse planned and ongoing without research A drafted plan could move to planned or ongoing while its Research section still held the placeholder. The helper only printed advice after the write. Now status planned and status ongoing refuse until the trimmed Research section is filled. The check compares the whole section, so Research prose that quotes the placeholder is not unfilled. Refs #78 --- .../plan-manager/scripts/plan.mjs | 14 ++++++++--- scripts/tests/plan-cli.mjs | 25 +++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index 841b615f..d3c874c1 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -595,10 +595,16 @@ export function parsePlan(text) { acceptance: table(sections.get('Acceptance'), ACCEPTANCE_HEADER).rows, }; } +// A drafted plan holds this placeholder until research is published (#78). +const RESEARCH_PLACEHOLDER = '_Not researched yet._'; +// Compare the whole section: research prose may quote the placeholder. +function researchFilled(plan) { + const research = plan.sections.get('Research').trim(); + return research !== '' && research !== RESEARCH_PLACEHOLDER; +} function printAdvice(text, status, extra = [], options = {}) { const result = normalizePlan(text, options); - if (status !== 'drafting' && parsePlan(text).sections.get('Research').includes('_Not researched yet._')) - result.advice.push('Research is not filled after drafting.'); + if (status !== 'drafting' && !researchFilled(parsePlan(text))) result.advice.push('Research is not filled after drafting.'); for (const message of [...result.advice, ...extra]) console.log(`advice: ${message}`); } // Reviewers write Markdown: read review keywords through emphasis, code marks, quotes, and list markers (#49). @@ -679,7 +685,7 @@ function createPlan(args) { return /^ {0,3}##[ \t]/.test(masked) ? line.replace(/^( {0,3})##/, '$1###') : line; }) .join('\n'); - const source = `## Goal\n${goalText}\n## Research\n_Not researched yet._\n## Steps\n${STEPS_HEADER}\n${STEPS_SEPARATOR}\n## Acceptance\n${ACCEPTANCE_HEADER}\n${ACCEPTANCE_SEPARATOR}`; + const source = `## Goal\n${goalText}\n## Research\n${RESEARCH_PLACEHOLDER}\n## Steps\n${STEPS_HEADER}\n${STEPS_SEPARATOR}\n## Acceptance\n${ACCEPTANCE_HEADER}\n${ACCEPTANCE_SEPARATOR}`; const { body } = normalizePlan(source, { mode: options['--mode'] }); const url = withBodyFile(body, (file) => runGh([ @@ -803,6 +809,8 @@ function setPlanStatus(args) { fail(`plan #${issue.number} is closed; status applies to open plans`); if (workStarted(issue, status) && !['ongoing', 'blocked'].includes(target)) fail(`illegal plan status transition: ${status} -> ${target}`); + if (['planned', 'ongoing'].includes(target) && !researchFilled(parsed)) + fail(`status ${target} requires a filled Research section`); const content = parsed.sections.get('Open questions').replace(/^Blocked:[^\n]*\n?\s*/i, ''); parsed.sections.set( 'Open questions', diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index ce999aca..d09a47bc 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -944,6 +944,31 @@ try { expectSuccess(edit(numericId), 'second normalization'); assert.equal(issue(numericId).body, once, 'a second normalization changes nothing'); + // A drafted plan enters planned or ongoing only after research replaces the placeholder (#78). + const unresearched = createPlan( + 'unresearched plan', + body().replace('The parser loses mixed-case records.', '_Not researched yet._'), + ); + refuse(run('status', String(unresearched), 'planned'), 'status planned requires a filled Research section'); + refuse(run('status', String(unresearched), 'ongoing'), 'status ongoing requires a filled Research section'); + assert.deepEqual(issue(unresearched).labels, ['plan', 'plan:drafting'], 'a refused status keeps the phase label'); + expectSuccess( + run('status', String(unresearched), 'blocked', '--reason', 'Waiting for research'), + 'an unresearched plan can still block', + ); + const emptyResearch = createPlan('empty research', body().replace('The parser loses mixed-case records.', '')); + refuse(run('status', String(emptyResearch), 'planned'), 'status planned requires a filled Research section'); + const quotedPlaceholder = createPlan( + 'research quotes the placeholder', + body().replace( + 'The parser loses mixed-case records.', + 'A new plan holds `_Not researched yet._` until research is published.', + ), + ); + const quotedStatus = run('status', String(quotedPlaceholder), 'planned'); + expectSuccess(quotedStatus, 'research prose may quote the placeholder'); + assert.doesNotMatch(quotedStatus.stdout, /Research is not filled/, 'quoted placeholder is not unfilled research'); + updateState((state) => { for (let i = 0; i < 500; i += 1) { const number = state.nextIssue++; From c44bc07864c523311218903fc4b09fcbac722e34 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:14:40 -0300 Subject: [PATCH 02/16] fix(plan-lifecycle): keep export provenance until the remote edit succeeds edit refreshed the .origin sidecar before it wrote the issue body. When gh issue edit failed, the sidecar already named a body that GitHub did not have, so a retry with the same file refused as a stale export. Now edit refreshes the sidecar only after the remote write succeeds. Refs #78 --- .../productivity/plan-manager/scripts/plan.mjs | 3 ++- scripts/tests/plan-cli.mjs | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index d3c874c1..84287814 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -788,9 +788,10 @@ function editPlan(args) { }); } fs.writeFileSync(file, body); + editIssueBodyIfUnchanged(issue, body); + // Refresh provenance only after the remote write, so a failed write can be retried with the same file (#78). fs.writeFileSync(originFile(file), `${bodyDigest(body)}\n`, { mode: 0o600 }); fs.chmodSync(originFile(file), 0o600); - editIssueBodyIfUnchanged(issue, body); const changes = changedLines(issue.body, body); console.log(headerStrip(issue, status)); console.log(`changed: ${changes.length} line(s)`); diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index d09a47bc..f011a111 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -969,6 +969,23 @@ try { expectSuccess(quotedStatus, 'research prose may quote the placeholder'); assert.doesNotMatch(quotedStatus.stdout, /Research is not filled/, 'quoted placeholder is not unfilled research'); + // A failed remote edit keeps the export valid, so the same file can be sent again (#78). + const retried = createPlan('retried edit'); + const retryFile = exported(retried); + fs.writeFileSync(retryFile, fs.readFileSync(retryFile, 'utf8').replace('Not run.', 'Smoke passed.')); + const transientError = 'GraphQL: Something went wrong while executing your query'; + updateState((state) => { + state.issueEditError = transientError; + }); + const failedEdit = run('edit', String(retried), '--file', retryFile); + refuse(failedEdit, `gh issue edit failed: ${transientError}`); + assert.equal(failedEdit.stdout, '', 'a failed edit prints no result'); + updateState((state) => { + delete state.issueEditError; + }); + expectSuccess(run('edit', String(retried), '--file', retryFile), 'retry with the same export'); + assert.match(issue(retried).body, /Smoke passed\./, 'the retry writes the edited body'); + updateState((state) => { for (let i = 0; i < 500; i += 1) { const number = state.nextIssue++; From fe3aba1e063cc297d920d6d31aae28da4f098b7c Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:16:29 -0300 Subject: [PATCH 03/16] fix(plan-lifecycle): advise on steps that share a path with no Depends chain Two steps that list the same file and have no Depends order can run in parallel and collide. The helper did not read the Files cell. Now it prints advice for each pair of steps that share an exact path, after it trims each entry and removes code marks, unless a Depends chain links the two steps. Advice, not a refusal: Files cells can hold folders and globs, which exact comparison cannot judge. Refs #78 --- .../plan-manager/scripts/plan.mjs | 24 +++++++++++++++++++ scripts/tests/plan-cli.mjs | 21 ++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index 84287814..63c2e44d 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -520,6 +520,29 @@ function replaceTable(sections, name, header, separator, rows) { ); sections.set(name, parsed.lines.join('\n')); } +// Steps that edit the same path need a Depends order, or parallel work collides (#78). +function sharedFilesAdvice(rows) { + const paths = rows.map( + (row) => new Set(row[3].split(',').map((part) => unquoteCode(part.trim())).filter((part) => part && part !== '-')), + ); + const dependencies = new Map( + rows.map((row) => [row[0], row[4] === '-' ? [] : row[4].split(',').map((part) => part.trim())]), + ); + const reaches = (from, to, seen = new Set()) => { + if (seen.has(from)) return false; + seen.add(from); + return (dependencies.get(from) ?? []).some((next) => next === to || reaches(next, to, seen)); + }; + const advice = []; + rows.forEach((row, index) => { + for (let other = index + 1; other < rows.length; other += 1) { + const shared = [...paths[index]].filter((entry) => paths[other].has(entry)); + if (shared.length && !reaches(row[0], rows[other][0]) && !reaches(rows[other][0], row[0])) + advice.push(`steps ${row[1]} and ${rows[other][1]} share ${shared.join(', ')} with no Depends link`); + } + }); + return advice; +} export function normalizePlan(text, { mode: explicitMode } = {}) { const advice = []; const cleaned = text @@ -559,6 +582,7 @@ export function normalizePlan(text, { mode: explicitMode } = {}) { }; for (const row of rows) if (row.length === 8) row[4] = row[4] === '-' ? '-' : row[4].split(',').map(dependency).join(', '); + advice.push(...sharedFilesAdvice(rows.filter((row) => row.length === 8))); replaceTable(sections, 'Steps', STEPS_HEADER, STEPS_SEPARATOR, rows); replaceTable( sections, diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index f011a111..2b321bc7 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -986,6 +986,27 @@ try { expectSuccess(run('edit', String(retried), '--file', retryFile), 'retry with the same export'); assert.match(issue(retried).body, /Smoke passed\./, 'the retry writes the edited body'); + // Steps that edit the same path need a Depends chain, or parallel work collides (#78). + const sharedFiles = createPlan('shared files'); + const sharedRows = [ + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + '| 2 | parse_dates | Parse dates | `src/parser.mjs`, src/dates.mjs | - | local | planned | Dates parse |', + '| 3 | format_dates | Format dates | src/dates.mjs | parse_dates | local | planned | Dates format |', + '| 4 | print_dates | Print dates | src/dates.mjs | format_dates | local | planned | Dates print |', + ].join('\n'); + const sharedEdit = edit(sharedFiles, (text) => + text.replace( + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + sharedRows, + ), + ); + expectSuccess(sharedEdit, 'edit with shared files'); + assert.deepEqual( + sharedEdit.stdout.split('\n').filter((line) => line.includes(' share ')), + ['advice: steps fix_parser and parse_dates share src/parser.mjs with no Depends link'], + 'only steps without a Depends chain get shared-path advice', + ); + updateState((state) => { for (let i = 0; i < 500; i += 1) { const number = state.nextIssue++; From 4db1f34dfb5471290c85990feb367387fad34010 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:19:02 -0300 Subject: [PATCH 04/16] fix(plan-lifecycle): advise a new plan review after planned-work edits A plan-review pass covers the body that the reviewer read. An edit after the pass could change Goal, Research, Steps, Acceptance, or Do not touch, and show still reported plan=pass. Now edit prints advice that names the changed sections when a trusted plan pass exists. Edits to Verification Results or Open questions print no such advice. Refs #78 --- .../productivity/plan-manager/scripts/plan.mjs | 15 +++++++++++++-- scripts/tests/plan-cli.mjs | 17 ++++++++++++++++- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index 63c2e44d..dd2c7e12 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -631,6 +631,11 @@ function printAdvice(text, status, extra = [], options = {}) { if (status !== 'drafting' && !researchFilled(parsePlan(text))) result.advice.push('Research is not filled after drafting.'); for (const message of [...result.advice, ...extra]) console.log(`advice: ${message}`); } +// A plan-review pass covers the body that the reviewer read (#78). +const PLANNED_WORK_SECTIONS = ['Goal', 'Research', 'Steps', 'Acceptance', 'Do not touch']; +function plannedWorkChanges(before, after) { + return PLANNED_WORK_SECTIONS.filter((name) => before.sections.get(name) !== after.sections.get(name)); +} // Reviewers write Markdown: read review keywords through emphasis, code marks, quotes, and list markers (#49). function reviewText(line) { return line @@ -772,7 +777,7 @@ function editPlan(args) { const [value, ...flags] = args, file = parseOptions(flags, new Set(['--file']))['--file']; if (!file) fail('edit requires an issue and --file'); - const { issue, parsed, status } = readPlanIssue(value, true, { history: true }), + const { issue, parsed, status } = readPlanIssue(value, true, { history: true, comments: true }), origin = readOrigin(file); if (!origin) fail( @@ -820,7 +825,13 @@ function editPlan(args) { console.log(headerStrip(issue, status)); console.log(`changed: ${changes.length} line(s)`); for (const line of changes) console.log(line); - printAdvice(source, status); + const changedWork = reviewVerdicts(issue).plan === 'pass' ? plannedWorkChanges(parsed, incomingPlan) : []; + const rereview = changedWork.length + ? [ + `plan review passed before this edit; changed sections: ${changedWork.join(', ')}; run a new plan-review round before code that depends on them`, + ] + : []; + printAdvice(source, status, rereview); } function setPlanStatus(args) { const [value, raw, ...flags] = args, diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index 2b321bc7..4a924f2e 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -1007,6 +1007,21 @@ try { 'only steps without a Depends chain get shared-path advice', ); + // A plan-review pass covers the body that the reviewer read (#78). + const reviewed = createPlan('reviewed plan'); + addIssueComment(reviewed, '### Plan review round 1\n\nPlan-review: pass\n\nNone.'); + const resultsEdit = edit(reviewed, (text) => text.replace('Not run.', 'Smoke passed.')); + expectSuccess(resultsEdit, 'edit verification results after a plan pass'); + assert.doesNotMatch(resultsEdit.stdout, /plan review passed before this edit/, 'evidence edits need no new round'); + const researchEdit = edit(reviewed, (text) => + text.replace('The parser loses mixed-case records.', 'The parser loses mixed-case records.\n\nDecision: keep the old date format.'), + ); + expectSuccess(researchEdit, 'edit research after a plan pass'); + assert.match( + researchEdit.stdout, + /^advice: plan review passed before this edit; changed sections: Research; run a new plan-review round before code that depends on them$/m, + ); + updateState((state) => { for (let i = 0; i < 500; i += 1) { const number = state.nextIssue++; @@ -1031,7 +1046,7 @@ try { assert.match(limitedList.stdout, /^advice: list stopped at 500 plans; more plans may exist$/m); console.log( - 'plan-cli smoke PASSED: labels, snapshot paging, normalization, ownership, race, provenance, step freeze, transitions, review trust, archive proof, list, retire, large output, Markdown review records, early closure, list limit, machine path advice', + 'plan-cli smoke PASSED: labels, snapshot paging, normalization, ownership, race, provenance, step freeze, transitions, review trust, archive proof, list, retire, large output, Markdown review records, early closure, list limit, machine path advice, research gate, edit retry, shared files advice, re-review advice', ); } finally { fs.rmSync(scratch, { recursive: true, force: true }); From d9ec659c1871d33483022cba439972e44004824a Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:20:02 -0300 Subject: [PATCH 05/16] docs(plan-lifecycle): state the research, retry, shared-path, and re-review rules The contract did not say when a passed plan needs a new review, how to recover from a failed GitHub call, what the Files cell holds, or that Acceptance must cover the checks that repository instructions require. It now states these rules, the Research gate on planned and ongoing, the new advice, and a pointer to the yolo rule for existing plans. Refs #78 --- .../skills/productivity/plan-manager/SKILL.md | 2 +- .../plan-manager/references/plan-contract.md | 54 ++++++++++++++++--- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md index 6a1f05ae..64857070 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md @@ -5,7 +5,7 @@ user-invocable: true metadata: pattern: tool-wrapper updated: "2026-10-19" - content_hash: "243b9e9f8f3a8755b28f902d400f4e0de2bf40f46d42ea6a4530e44f2ec5d6d8" + content_hash: "bb1ee37a5ff78641994eb70b3c785307086998dd40a97623a9700e47d24837ca" --- # Plan Manager diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md index d48307a2..12b3bd3c 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md @@ -67,11 +67,15 @@ Use these table headers: ``` Each Steps row describes one bounded task. `#` is its display number. `Id` is -its durable command identifier. `Files` defines scope. `Depends` is `-` or +its durable command identifier. `Files` defines scope as comma-separated +repository-relative paths; a path can be in backticks. `Depends` is `-` or comma-separated display numbers or ids. The helper resolves ids to display -numbers on write. `Done when` names observable completion, not activity. -Acceptance rows identify a command and its expected result. They cover the goal, -not just individual steps. Use repository-relative paths and portable commands. +numbers on write. Two steps that list the same path need a Depends chain +between them, or one of them must own the shared edit. `Done when` names +observable completion, not activity. Acceptance rows identify a command and its +expected result. They cover the goal, not just individual steps. Acceptance also +holds one row for each check that the repository instructions require for the +paths in Files. Use repository-relative paths and portable commands. | Token | Meaning | |---|---| @@ -115,6 +119,9 @@ may move to any open status. After it starts, only ongoing and blocked are legal targets. Status writes repair phase labels. Blocked requires a single-line reason at the start of Open questions. Leaving blocked removes that line. +`planned` and `ongoing` require a filled Research section. The trimmed section +must not be empty and must not equal `_Not researched yet._`. + An open plan must be ongoing for `step`. Its done and skipped steps are immutable. Other open step transitions are free. A finished plan permits only done or skipped targets for terminal repair. To move a step of a finished plan to another status, reopen the issue first. Other closed plans do not permit step transitions. @@ -150,6 +157,12 @@ Run this review loop for plan review and for code review: without a relevant change. If no answer is available, record the reason and block. A yolo run uses the round ceiling in Yolo runs instead. +A plan pass covers the body that the reviewer read. After `plan=pass`, an edit +that changes Goal, Research, Steps, Acceptance, or Do not touch needs a new +plan-review round before you write code that depends on the change. Edits to +Verification Results or Open questions, and `step` state changes, need no new +round. + Record user-only decisions in Open questions. Do not invent approval. The helper finds review records in comments with these rules. The reviewer @@ -202,7 +215,9 @@ Scratch lives at `git rev-parse --git-path docks-review`, with directory mode 0700. Export writes the body and a `.origin` sidecar containing its sha256 hex plus newline, with sidecar mode 0600. The digest detects staleness; it grants no permission. Edit validates provenance, normalizes the file, applies the freeze, -and refreshes the sidecar before the remote write. Re-export after remote changes. +writes the remote body, and then refreshes the sidecar. A failed remote write +leaves the sidecar unchanged, so the same file stays valid for a retry. +Re-export after remote changes. Keep scratch and machine paths out of durable records. No queue file is tool input. ## Normalization, advice, and refusals @@ -217,9 +232,12 @@ rewrite never drops the row. Once work has started, `edit` also refuses while the current record holds such a row. A duplicate or unknown `##` heading blocks every body write; a retired `## Review` heading is dropped on the next write. An explicit `--mode` on `new` outranks any Mode line inside `--goal`; otherwise the last unfenced, unindented Mode line in Goal is the decision. Advice prints `advice: ` on stdout and exits 0. It covers absolute machine -paths, `_Not researched yet._` after drafting, defaulted Mode, unknown Effect or -Status, an unfinished dependency when a step moves to in-flight or done, and -every non-local Effect as an in-session permission reminder. A machine path +paths, a Research section that holds only `_Not researched yet._` after drafting, +defaulted Mode, unknown Effect or Status, an unfinished dependency when a step +moves to in-flight or done, two steps that share a Files path with no Depends +chain, an `edit` that changes Goal, Research, Steps, Acceptance, or Do not touch +after a trusted plan pass, and every non-local Effect as an in-session +permission reminder. A machine path points into a user or root home, a mount, a named temporary or runtime directory, a Windows drive, or a network share. Its advice is `body contains an absolute machine path: `, once per distinct path. @@ -240,6 +258,7 @@ stale export: was exported from a superseded body; run `plan.mjs export < step state is frozen once work starts: plan # is closed; status applies to open plans illegal plan status transition: -> +status requires a filled Research section illegal step status transition: -> plan # is finished; reopen it before moving step to plan status is ; expected ongoing @@ -258,6 +277,24 @@ blocked status requires --reason as single-line text reserved label namespace: