Close yolo, contract, and helper gaps from #78

plugins/plan-lifecycle/agents/plan-reviewer.md

@@ -21,9 +21,11 @@ lifecycle change.
 <constraint>
 A plan-review finding is exactly one of `goal_fit`, `research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan passes.
 Report a `goal_fit` finding when the plan uses a temporary workaround instead of the root-cause fix, leaves a caller on a replaced path, or keeps an obsolete path, unless Open questions record user approval.
+Report a `goal_fit` finding when Acceptance lacks a check that the repository instructions require for the paths in the Steps Files cells.
 Perform one review invocation and return one verdict. Never demand style,
 naming, formatting, line counts, more citations, additional probes, mutation
-tests, extra acceptance rows, cosmetic work, or restructuring for its own sake.
+tests, acceptance rows beyond the checks that repository instructions require,
+cosmetic work, or restructuring for its own sake.
 </constraint>
 
 ## Workflow

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

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: <text>` 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: <path>`, once per distinct path.
@@ -240,6 +258,7 @@ stale export: <path> was exported from a superseded body; run `plan.mjs export <
 step state is frozen once work starts: <detail>
 plan #<n> is closed; status applies to open plans
 illegal plan status transition: <from> -> <to>
+status <status> requires a filled Research section
 illegal step status transition: <from> -> <to>
 plan #<n> is finished; reopen it before moving step <id> to <status>
 plan status is <status>; expected ongoing
@@ -258,6 +277,24 @@ blocked status requires --reason as single-line text
 reserved label namespace: <label>
 ```
 
+GitHub call failures exit 1 and print one of these messages on stderr. `edit`,
+`status`, and `step` print their stdout result only after the write succeeds.
+
+```text
+gh <command> <subcommand> failed: <gh stderr, or exit <code>>
+gh api graphql returned errors: <message>
+```
+
+A failure can be short-lived, such as
+`GraphQL: Something went wrong while executing your query`. After such a
+failure:
+
+1. Run `show <n>` and check whether the change is on the issue.
+2. If it is not, run the same command once more. For `edit`, use the same file.
+3. If it is, do not run the command again. Run `export <n>` before the next body
+   edit.
+4. If the second run fails, stop the phase and report the message.
+
 ## Landing and archive proof
 
 ### Linked branch
@@ -337,6 +374,7 @@ question. All other contract rules stay in force.
 | Contract question | Yolo rule |
 |---|---|
 | Path and Mode | Plan with `Mode: plan-and-implement` |
+| Request that names an existing plan, or an incomplete request | The yolo skill's Existing plans and incomplete requests rule |
 | Non-local Effect step before the landing merge | The effect table below |
 | Linked-branch timing | Right after `new`, while the plan is drafting |
 | Dirty checkout at linked-branch setup | A separate clean worktree, without a question |

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,
@@ -595,12 +619,23 @@ 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}`);
 }
+// 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
@@ -679,7 +714,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([
@@ -742,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(
@@ -782,14 +817,21 @@ 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)`);
   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,
@@ -803,6 +845,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',

plugins/plan-lifecycle/skills/productivity/plan-reviewer/SKILL.md

@@ -5,7 +5,7 @@ user-invocable: false
 metadata:
   pattern: tool-wrapper
   updated: "2026-10-19"
-  content_hash: "10bb17553f82450f69825d1f6b0e18b15ac9d76c135e0a38ce525f0fd5a9c7ee"
+  content_hash: "52a28e27f3849805a5044e9c6f16c95320abca5f6ab4aa2f5a19c4645461b94f"
 ---
 
 # Plan Reviewer
@@ -35,6 +35,7 @@ citations, probes, tests, or acceptance rows without a load-bearing defect.
 | `security_risk` | The plan introduces or ignores a security risk or destructive action without confirmation |
 
 Report a `goal_fit` finding when the plan uses a temporary workaround instead of the root-cause fix, leaves a caller on a replaced path, or keeps an obsolete path, unless Open questions record user approval.
+Report a `goal_fit` finding when Acceptance lacks a check that the repository instructions require for the paths in the Steps Files cells.
 Read enough surrounding code, callers, tests, and local instructions to verify
 claims. Check library and API claims against current official documentation.
 Do not demand additional sources when existing evidence establishes the claim.