From cca62ff8b633a0010d9363675bdeac2d87501f88 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:27:16 -0300 Subject: [PATCH] feat(plan-lifecycle): harden CLI writes and validation Implements reviewed plan #23: - claim/new verify the assignee by post-write read; claim's assignee write is guarded body-unchanged - retire retries on a closed not-planned issue with a stale phase label and finishes cleanup; fully retired plans stay refused - new prevalidates a trimmed 1-70 title and non-empty goal before any write - machine-path citations catch any POSIX absolute, drive-letter, and UNC path, backticked included; link destinations and exactly /dev/null exempt - status/step/claim refuse malformed v3 records via the full 13 checks Code review round 1: pass with two recorded MEDIUM advisories (phase-label post-read set check; /dev/null token exactness and bare drive roots). Closes #23 --- .../plan-manager/scripts/plan.mjs | 64 +++++- scripts/tests/fixtures/gh-stub/gh | 15 +- scripts/tests/plan-cli.mjs | 196 +++++++++++++++++- 3 files changed, 261 insertions(+), 14 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 904d0045..aec7961f 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -157,9 +157,18 @@ function issueIdentity(planRef) { } export function machinePathCitations(planText) { + // `/dev/null` is the portable bit bucket, not a machine location; every other `/dev/` path + // (such as `/dev/shm/...`) names a machine filesystem and stays reported. + const machinePath = /(?:^|[\s("'`[{=:;,>])(?:\/(?!\/|\s|dev\/null\b)|[A-Za-z]:[\\/](?=\S)|\\\\(?=[^\\/\s]+[\\/][^\s]))/; return planText .split('\n') - .filter((line) => !/^[A-Z][A-Za-z0-9-]*: *\{/.test(line) && /\/home\/[a-z]|\/Users\/[A-Za-z]/.test(line)); + .filter((line) => { + if (/^[A-Z][A-Za-z0-9-]*: *\{/.test(line)) return false; + // Markdown link destinations are URL space, not filesystem citations; inline code stays + // detectable because a backticked machine path is still a machine path. + const prose = line.replace(/\]\([^)\n]*\)/g, ']()'); + return machinePath.test(prose); + }); } export function checkPlan(planText, planRef) { @@ -581,6 +590,12 @@ function editIssueLabelsIfBodyUnchanged(issue, labels) { for (const label of remove) argv.push('--remove-label', label); argv.push('--repo', repository.nameWithOwner); runGh(argv); + if (remove.length > 0 || labels.add) { + const stored = labelNames(issueView(issue.number, 'labels').labels); + if ((labels.add && !stored.includes(labels.add)) || remove.some((label) => stored.includes(label))) { + fail('plan issue labels differ after edit'); + } + } } function parseOptions(args, allowed, repeatable = new Set()) { @@ -640,9 +655,26 @@ function labelsCommand(args) { } } +function requireValidPlanIssue(issue) { + const failures = checkPlan(issue.body, issue); + if (failures.length > 0) fail(failures.map((message) => `#${issue.number}: ${message}`).join('\n')); +} + +function verifySoleAssignee(number, login) { + const stored = issueView(number, 'number,assignees'); + const owners = (stored.assignees ?? []).map((assignee) => assignee.login).filter(Boolean); + if (owners.length !== 1 || owners[0] !== login) { + fail(`plan #${number} assignee verification failed: expected sole assignee ${login}`); + } +} + function createPlan(args) { const options = parseOptions(args, new Set(['--title', '--goal', '--mode', '--label']), new Set(['--label'])); - if (!options['--title'] || !options['--goal']) fail('new requires --title and --goal'); + if (options['--title'] === undefined || options['--goal'] === undefined) fail('new requires --title and --goal'); + const title = options['--title'].trim(); + const goal = options['--goal'].trim(); + if (!title || title.length > 70) fail('title must contain 1 to 70 characters after trimming'); + if (!goal) fail('goal must be non-empty after trimming'); const mode = options['--mode'] ?? 'plan-and-implement'; if (!new Set(['plan-and-implement', 'plan-only']).has(mode)) fail(`invalid plan mode: ${mode}`); if ([options['--title'], options['--goal']].some((value) => /[\r\n]/.test(value))) fail('title and goal must be single-line text'); @@ -650,13 +682,14 @@ function createPlan(args) { if (!label.trim() || /[\r\n]/.test(label)) fail('label names must be non-empty single-line text'); if (/^plan(?::|$)/.test(label)) fail(`reserved label namespace: ${label}`); } - const body = planTemplate(options['--goal'], mode); + const login = resolveActingLogin(); + const body = planTemplate(goal, mode); const output = withBodyFile(body, (bodyFile) => { const argv = [ 'issue', 'create', '--title', - options['--title'], + title, '--body-file', bodyFile, '--label', @@ -672,7 +705,9 @@ function createPlan(args) { }); const match = /\/issues\/([1-9]\d*)\/?$/.exec(output); if (!match) fail('gh issue create returned an invalid issue URL'); - console.log(`plan created: #${match[1]} ${output}`); + const number = Number(match[1]); + verifySoleAssignee(number, login); + console.log(`plan created: #${number} ${output}`); } function claimPlan(args) { @@ -686,7 +721,10 @@ function claimPlan(args) { console.log(`plan #${issue.number} already claimed: ${login}`); return; } - runGh(['issue', 'edit', String(issue.number), '--add-assignee', '@me', '--repo', repository.nameWithOwner]); + requireValidPlanIssue(issue); + issue.claimLogin = login; + editIssueLabelsIfBodyUnchanged(issue, {}); + verifySoleAssignee(issue.number, login); console.log(`plan #${issue.number} claimed: ${login}`); } @@ -837,6 +875,7 @@ function setPlanStatus(args) { if (String(issue.state).toUpperCase() === 'CLOSED') { fail(`plan #${issue.number} is closed; status applies to open plans`); } + requireValidPlanIssue(issue); const current = record.status; if (!new Set(['unreadable', 'unlabelled']).has(current) && !STATUS_TRANSITIONS[current]?.has(target)) { fail(`illegal plan status transition: ${current} -> ${target}`); @@ -854,6 +893,7 @@ function setStepStatus(args) { if (!value || !stepId || !target || args.length !== 3) fail('step requires an issue, step id, and status'); if (!STEP_STATUSES.has(target)) fail(`unknown step status: ${target}`); const { issue, parsed, record } = readPlanIssue(value, true); + requireValidPlanIssue(issue); if (record.status !== 'ongoing') fail(`plan status is ${record.status}; expected ongoing`); const { sections } = sectionMap(parsed.body); const table = parseRows(sections.get('Steps') ?? '', STEPS_HEADER, STEPS_SEPARATOR, 8); @@ -970,7 +1010,12 @@ function archivePlan(args, retired = false) { const options = parseOptions(flags, retired ? new Set(['--reason']) : new Set()); if (retired && (!String(options['--reason'] ?? '').trim() || /[\r\n]/.test(options['--reason']))) fail('retire requires a single-line --reason'); const { issue, parsed, record } = readPlanIssue(value, true); - if (retired && String(issue.state).toUpperCase() === 'CLOSED') fail(`cannot retire a ${record.status} plan`); + const closed = String(issue.state).toUpperCase() === 'CLOSED'; + const recovery = retired && + closed && + String(issue.stateReason).toUpperCase() === 'NOT_PLANNED' && + labelNames(issue.labels).some((label) => label.startsWith('plan:') && PLAN_STATUSES.has(label.slice(5))); + if (retired && closed && !recovery) fail(`cannot retire a ${record.status} plan`); if (!retired && record.status !== 'finished') fail(`archive requires finished status, found ${record.status}`); let closingPullRequest; @@ -1012,7 +1057,7 @@ function archivePlan(args, retired = false) { } } - if (retired) { + if (retired && !recovery) { runGh([ 'issue', 'close', @@ -1026,7 +1071,8 @@ function archivePlan(args, retired = false) { ]); } editIssueLabelsIfBodyUnchanged(issue, { remove: labelsToRemove(issue) }); - if (retired) console.log(`plan #${issue.number} retired`); + if (recovery) console.log(`plan #${issue.number} retired (recovered label cleanup)`); + else if (retired) console.log(`plan #${issue.number} retired`); else { const url = closingPullRequest.url ?? `https://github.com/${repository.nameWithOwner}/pull/${closingPullRequest.number}`; console.log(`plan #${issue.number} finished (closed by ${url})`); diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index 3c192521..be72b439 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -358,7 +358,9 @@ if (group === 'issue' && command === 'create') { state: 'OPEN', stateReason: null, labels: values('--label'), - assignees: values('--assignee').map((login) => login === '@me' ? state.viewerLogin : login), + assignees: state.dropCreateAssignee + ? [] + : values('--assignee').map((login) => login === '@me' ? state.viewerLogin : login), url: `https://github.com/${state.repo.nameWithOwner}/issues/${number}`, createdAt: now, updatedAt: now, @@ -396,14 +398,21 @@ if (group === 'issue' && command === 'edit') { const addedAssignees = values('--add-assignee'); const addedLabels = values('--add-label'); const removedLabels = values('--remove-label'); + if (removedLabels.length > 0 && state.labelRemovalErrorOnce) { + const message = state.labelRemovalErrorOnce; + delete state.labelRemovalErrorOnce; + die(message); + } if (!bodyFile && addedAssignees.length === 0 && addedLabels.length === 0 && removedLabels.length === 0) die('issue edit requires a body, label, or assignee change'); if (bodyFile) issue.body = fs.readFileSync(bodyFile, 'utf8'); for (const name of removedLabels) issue.labels = issue.labels.filter((label) => label !== name); for (const name of addedLabels) if (!issue.labels.includes(name)) issue.labels.push(name); issue.assignees ??= []; - for (const login of addedAssignees.map((name) => name === '@me' ? state.viewerLogin : name)) { - if (!issue.assignees.includes(login)) issue.assignees.push(login); + if (!state.dropEditAssignee) { + for (const login of addedAssignees.map((name) => name === '@me' ? state.viewerLogin : name)) { + if (!issue.assignees.includes(login)) issue.assignees.push(login); + } } touch(issue); save(); diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index fa21befc..092633d1 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -239,6 +239,40 @@ try { assert.equal(reservedNew.stderr.trim(), 'reserved label namespace: plan:ongoing'); assert.equal(loadState().nextIssue, beforeReservedNew.nextIssue); assert.deepEqual(loadState().issues, beforeReservedNew.issues); + const beforeOversizedTitle = loadState(); + const oversizedTitle = run('new', '--title', 'x'.repeat(71), '--goal', 'An oversized title creates no issue'); + assert.equal(oversizedTitle.status, 1); + assert.equal(oversizedTitle.stderr.trim(), 'title must contain 1 to 70 characters after trimming'); + assert.equal(loadState().nextIssue, beforeOversizedTitle.nextIssue); + assert.deepEqual(loadState().issues, beforeOversizedTitle.issues); + const beforeWhitespaceGoal = loadState(); + const whitespaceGoal = run('new', '--title', 'Whitespace goal', '--goal', ' \t '); + assert.equal(whitespaceGoal.status, 1); + assert.equal(whitespaceGoal.stderr.trim(), 'goal must be non-empty after trimming'); + assert.equal(loadState().nextIssue, beforeWhitespaceGoal.nextIssue); + assert.deepEqual(loadState().issues, beforeWhitespaceGoal.issues); + + const failedCreateAssignmentNumber = loadState().nextIssue; + updateState((state) => { + state.dropCreateAssignee = true; + }); + const failedCreateAssignment = run( + 'new', + '--title', + 'Missing create assignment', + '--goal', + 'The silently dropped assignment must be reported', + ); + assert.equal(failedCreateAssignment.status, 1); + assert.equal(failedCreateAssignment.stdout, ''); + assert.equal( + failedCreateAssignment.stderr.trim(), + `plan #${failedCreateAssignmentNumber} assignee verification failed: expected sole assignee plan-agent`, + ); + assert.deepEqual(issue(failedCreateAssignmentNumber).assignees, []); + updateState((state) => { + delete state.dropCreateAssignee; + }); const topicNumber = createPlan('topic-label', { labels: ['security'] }); assert.deepEqual(issue(topicNumber).labels, ['plan', 'plan:drafting', 'security']); @@ -269,6 +303,7 @@ try { '@me', ]); const claimNumber = createPlan('claim'); + makeValid(claimNumber); updateIssue(claimNumber, (entry) => { entry.assignees = []; }); @@ -280,6 +315,65 @@ try { expectSuccess(claimedAgain, 'claim idempotently'); assert.equal(claimedAgain.stdout.trim(), `plan #${claimNumber} already claimed: plan-agent`); assert.deepEqual(issue(claimNumber).assignees, ['plan-agent']); + const failedClaimAssignmentNumber = createPlan('failed-claim-assignment'); + makeValid(failedClaimAssignmentNumber); + updateIssue(failedClaimAssignmentNumber, (entry) => { + entry.assignees = []; + }); + updateState((state) => { + state.dropEditAssignee = true; + }); + const failedClaimAssignment = run('claim', String(failedClaimAssignmentNumber)); + assert.equal(failedClaimAssignment.status, 1); + assert.equal(failedClaimAssignment.stdout, ''); + assert.equal( + failedClaimAssignment.stderr.trim(), + `plan #${failedClaimAssignmentNumber} assignee verification failed: expected sole assignee plan-agent`, + ); + assert.deepEqual(issue(failedClaimAssignmentNumber).assignees, []); + updateState((state) => { + delete state.dropEditAssignee; + }); + + const concurrentClaimNumber = createPlan('concurrent-claim'); + makeValid(concurrentClaimNumber); + updateIssue(concurrentClaimNumber, (entry) => { + entry.assignees = []; + }); + const concurrentClaimBody = issue(concurrentClaimNumber).body; + updateState((state) => { + state.remoteChange = { + issue: concurrentClaimNumber, + viewsBeforeChange: 1, + body: `${concurrentClaimBody}\nremote claim change\n`, + }; + }); + const concurrentClaim = run('claim', String(concurrentClaimNumber)); + assert.equal(concurrentClaim.status, 1); + assert.equal(concurrentClaim.stdout, ''); + assert.equal(concurrentClaim.stderr.trim(), 'plan issue changed remotely; re-read and retry'); + assert.deepEqual(issue(concurrentClaimNumber).assignees, []); + + const malformedClaimNumber = createPlan('malformed-claim'); + makeValid(malformedClaimNumber); + updateIssue(malformedClaimNumber, (entry) => { + entry.assignees = []; + entry.body = entry.body.replace('| A1 | `node --version` | Exit 0 |\n', ''); + }); + const malformedClaimBefore = issue(malformedClaimNumber); + const malformedClaimEditsBefore = loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(malformedClaimNumber), + ).length; + const malformedClaim = run('claim', String(malformedClaimNumber)); + assert.equal(malformedClaim.status, 1); + assert.match(malformedClaim.stderr, new RegExp(`#${malformedClaimNumber}: check 9:`)); + assert.deepEqual(issue(malformedClaimNumber), malformedClaimBefore); + assert.equal( + loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(malformedClaimNumber), + ).length, + malformedClaimEditsBefore, + ); const foreignClaimNumber = createPlan('foreign-claim'); updateIssue(foreignClaimNumber, (entry) => { entry.assignees = ['other-agent']; @@ -477,6 +571,41 @@ try { /check 12:/, ); } + for (const machinePath of [ + '/usr/local/x', + '/workspace/x', + '/tmp/x', + 'C:\\Users\\x', + '\\\\host\\share', + '`/home/user/x`', + '`C:\\Users\\x`', + '`\\\\host\\share`', + '/dev/shm/x', + ]) { + mutateAndRestore( + validNumber, + `machine path ${machinePath} introduced`, + (entry) => { + entry.body = entry.body.replace('_Not researched yet._', `_Not researched yet._ ${machinePath}`); + }, + /check 10:/, + ); + } + + const portableCitationsFile = path.join(scratch, 'portable-citations-plan.md'); + fs.writeFileSync( + portableCitationsFile, + issue(validNumber).body.replace( + '_Not researched yet._', + [ + 'Repository facts are in src/example.mjs and DocksDocks/docks#23.', + 'See https://example.com/usr/local/x and [the lifecycle guide](/docs/plan-lifecycle).', + 'Command: {/tmp/example}', + 'Cover each untracked path with `git diff --no-index /dev/null docs/PLAN.md`.', + ].join('\n'), + ), + ); + expectSuccess(run('check', '--file', portableCitationsFile), 'check portable path citations'); const validFile = path.join(scratch, 'valid-plan.md'); fs.writeFileSync(validFile, issue(validNumber).body); @@ -1224,8 +1353,20 @@ try { assert.match(bogusListing.stdout, new RegExp(`^drafting\\t#${transitionNumber}\\t`, 'm')); assert.doesNotMatch(bogusListing.stdout, /plan:bogus/); updateIssue(transitionNumber, (entry) => entry.labels.push('plan:blocked')); - expectSuccess(run('status', String(transitionNumber), 'planned'), 'conflicting labels repaired'); - assert.deepEqual(issue(transitionNumber).labels.sort(), ['plan', 'plan:planned', 'security']); + const malformedStatusBefore = issue(transitionNumber); + const malformedStatusEditsBefore = loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(transitionNumber), + ).length; + const malformedStatus = run('status', String(transitionNumber), 'planned'); + assert.equal(malformedStatus.status, 1); + assert.match(malformedStatus.stderr, new RegExp(`#${transitionNumber}: check 2:`)); + assert.deepEqual(issue(transitionNumber), malformedStatusBefore); + assert.equal( + loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(transitionNumber), + ).length, + malformedStatusEditsBefore, + ); const stepTransitions = { planned: new Set(['in-flight', 'done', 'blocked', 'skipped']), @@ -1252,6 +1393,26 @@ try { } } } + const malformedStepNumber = createPlan('malformed-step'); + makeValid(malformedStepNumber); + setIssueStatus(malformedStepNumber, 'ongoing'); + updateIssue(malformedStepNumber, (entry) => { + entry.body = entry.body.replace('| A1 | `node --version` | Exit 0 |\n', ''); + }); + const malformedStepBefore = issue(malformedStepNumber); + const malformedStepEditsBefore = loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(malformedStepNumber), + ).length; + const malformedStep = run('step', String(malformedStepNumber), 'implement_contract', 'done'); + assert.equal(malformedStep.status, 1); + assert.match(malformedStep.stderr, new RegExp(`#${malformedStepNumber}: check 9:`)); + assert.deepEqual(issue(malformedStepNumber), malformedStepBefore); + assert.equal( + loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'edit' && call[2] === String(malformedStepNumber), + ).length, + malformedStepEditsBefore, + ); const dependencyNumber = createPlan('dependency'); makeValid(dependencyNumber, { @@ -1928,6 +2089,37 @@ try { ); assert.deepEqual(issue(staleCommitNumber), beforeStaleCommitArchive); + const retireRecoveryNumber = createPlan('retire-cleanup-recovery'); + makeValid(retireRecoveryNumber); + setIssueStatus(retireRecoveryNumber, 'planned'); + updateState((state) => { + state.labelRemovalErrorOnce = 'stubbed label cleanup failure'; + }); + const failedRetireCleanup = run('retire', String(retireRecoveryNumber), '--reason', 'The request was withdrawn'); + assert.equal(failedRetireCleanup.status, 1, 'retire must expose a failed label cleanup'); + assert.equal(failedRetireCleanup.stderr.trim(), 'gh issue edit failed: stubbed label cleanup failure'); + assert.equal(issue(retireRecoveryNumber).state, 'CLOSED'); + assert.equal(issue(retireRecoveryNumber).stateReason, 'NOT_PLANNED'); + assert.deepEqual( + issue(retireRecoveryNumber).labels, + ['plan', 'plan:planned'], + 'failed retire cleanup must preserve the recoverable phase label', + ); + const recoveredRetire = run('retire', String(retireRecoveryNumber), '--reason', 'Retry the label cleanup'); + expectSuccess(recoveredRetire, 'retire label cleanup recovery'); + assert.equal(recoveredRetire.stdout.trim(), `plan #${retireRecoveryNumber} retired (recovered label cleanup)`); + assert.deepEqual(issue(retireRecoveryNumber).labels, ['plan']); + assert.equal( + loadState().calls.filter( + (call) => call[0] === 'issue' && call[1] === 'close' && call[2] === String(retireRecoveryNumber), + ).length, + 1, + 'retire recovery must not close the issue again', + ); + const completedRecoveryRetry = run('retire', String(retireRecoveryNumber), '--reason', 'A second retry'); + assert.equal(completedRecoveryRetry.status, 1, 'a fully retired plan must remain refused'); + assert.equal(completedRecoveryRetry.stderr.trim(), 'cannot retire a retired plan'); + const retiredNumber = createPlan('retired'); makeValid(retiredNumber); setIssueStatus(retiredNumber, 'planned');