mirror of
https://github.com/garrytan/gstack.git
synced 2026-10-31 14:06:50 +01:00
F1: bind the plan audit to this branch's plan, never the newest file
Plan File Discovery now prefers an explicit binding (the plan-mode file
or /autoplan ACTIVE_PLAN from the conversation, then a "Plan: <path>"
line in the PR body), then lists candidates: repo-committed
docs/designs/ files this branch changed or that name it, and personal
plan files that name the branch. Candidates are offered, never picked
silently. The repo-name match and the newest-file-of-the-day fallback,
which audited PRs against unrelated plans, are gone. With nothing bound
or chosen it prints the exact line 'Plan completion audit: not run (...).
Fix: add "Plan: <path>" to the PR body, or run /autoplan.'
/ship's parent now binds the plan before dispatching the audit
subagent ({{PLAN_COMPLETION_GATE_SHIP:discovery}}), so the child never
searches. Migrates three tests that pinned the old markers and notice.
Co-authored-by: loulanyue <260355617@qq.com>
This commit is contained in:
1 parent
3e55fb8d8a
commit
e99c1fc7a0
5 files changed
+154
-27
No files matched your search
@@ -186,36 +186,52 @@ table-bearing section that must be the file's terminal heading.`;
|
||||
|
||||
// ─── Plan File Discovery (shared helper) ──────────────────────────────
|
||||
|
||||
export const PLAN_AUDIT_NOT_RUN =
|
||||
'Plan completion audit: not run (no plan is bound to this branch and no docs/designs/ file matches). Fix: add "Plan: <path>" to the PR body, or run /autoplan.';
|
||||
|
||||
function generatePlanFileDiscovery(ship = false): string {
|
||||
return `### Plan File Discovery
|
||||
|
||||
1. **Conversation context (primary):** Use the active plan file from this conversation or its plan-mode system context.
|
||||
Audit the plan this branch was built from, never a plan that is merely the newest file.
|
||||
|
||||
2. **Content-based search (fallback):** Without a conversation-supplied path, search by content:
|
||||
1. **Conversation context (primary):** the plan-mode file in this conversation's system context, or the \`ACTIVE_PLAN\` of a \`/autoplan\` run in this conversation. Either is a binding.
|
||||
2. **PR body binding:** a \`Plan: <path>\` line in this branch's open PR body, printed below as \`PLAN_BINDING:\`. A relative path resolves against the repository root.
|
||||
3. **Content-based search (fallback):** without a binding, list candidates; never pick one silently.
|
||||
|
||||
\`\`\`bash
|
||||
setopt +o nomatch 2>/dev/null || true # zsh compat
|
||||
BRANCH=$(git branch --show-current 2>/dev/null | tr '/' '-' | tr -cd 'a-zA-Z0-9._-')
|
||||
REPO=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)")
|
||||
_REPOTOP=$(git rev-parse --show-toplevel 2>/dev/null)
|
||||
_BOUND=$(gh pr view --json body -q .body 2>/dev/null | tr -d '\\r\`' | sed -n 's/^[[:space:]]*Plan:[[:space:]]*\\([^[:space:]]*\\).*/\\1/p' | head -1)
|
||||
[ -n "$_BOUND" ] && echo "PLAN_BINDING: $_BOUND"
|
||||
if [ -n "$_REPOTOP" ]; then
|
||||
_BASE=$(git merge-base "origin/<base>" HEAD 2>/dev/null)
|
||||
{ [ -n "$_BASE" ] && git -C "$_REPOTOP" diff --name-only --diff-filter=AM "$_BASE" -- 'docs/designs/*.md'
|
||||
[ -n "$BRANCH" ] && git -C "$_REPOTOP" grep -l -F -e "$BRANCH" -- 'docs/designs/*.md'
|
||||
} 2>/dev/null | sort -u | sed "s|^|PLAN_CANDIDATE: $_REPOTOP/|"
|
||||
fi
|
||||
GSTACK_STATE_ROOT=$(~/.claude/skills/gstack/bin/gstack-paths --get GSTACK_STATE_ROOT); : "\${GSTACK_STATE_ROOT:?gstack-paths failed; reinstall with ./setup or /gstack-upgrade}"
|
||||
_PLAN_SLUG=$(~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null | sed -n 's/^SLUG=//p') || true
|
||||
_PLAN_SLUG="\${_PLAN_SLUG:-$(basename "$PWD" | tr -cd 'a-zA-Z0-9._-')}"
|
||||
for PLAN_DIR in "$GSTACK_STATE_ROOT/projects/$_PLAN_SLUG" "$HOME/.claude/plans" "$HOME/.codex/plans" ".gstack/plans"; do
|
||||
[ -d "$PLAN_DIR" ] || continue
|
||||
PLAN=$(ls -t "$PLAN_DIR"/*.md 2>/dev/null | xargs grep -l "$BRANCH" 2>/dev/null | head -1)
|
||||
[ -z "$PLAN" ] && PLAN=$(ls -t "$PLAN_DIR"/*.md 2>/dev/null | xargs grep -l "$REPO" 2>/dev/null | head -1)
|
||||
[ -z "$PLAN" ] && PLAN=$(find "$PLAN_DIR" -name '*.md' -mmin -1440 -maxdepth 1 2>/dev/null | xargs -r ls -t 2>/dev/null | head -1)
|
||||
[ -n "$PLAN" ] && break
|
||||
[ -d "$PLAN_DIR" ] && [ -n "$BRANCH" ] || continue
|
||||
grep -l -F -e "$BRANCH" "$PLAN_DIR"/*.md 2>/dev/null | sed 's|^|PLAN_CANDIDATE: |'
|
||||
done
|
||||
[ -n "$PLAN" ] && echo "PLAN_FILE: $PLAN" || echo "NO_PLAN_FILE"
|
||||
\`\`\`
|
||||
|
||||
3. **Validation:** For search results, read the first 20 lines and verify the project, feature and current branch. A mismatch means "no plan file found." Conversation-supplied paths bypass this search-result check.
|
||||
\`PLAN_CANDIDATE:\` lines are repo-committed \`docs/designs/\` files this branch changed or that name the branch, then personal plan files that name it. Offer them with AskUserQuestion: one option per candidate (at most four), plus "No plan: skip the audit". Recommend the candidate only when exactly one \`docs/designs/\` file changed on this branch; otherwise recommend skipping. Spawned or non-interactive runs take the recommendation. Read the chosen file's first 20 lines to confirm the project and feature.
|
||||
|
||||
**Error handling:**
|
||||
${ship ? `- No plan file found → skip with "No plan file detected — skipping."
|
||||
- Plan file found but unreadable (permissions, encoding) → return an audit error to the parent. Do not report no plan or successful zero counts; the parent applies its audit-failure recovery and skip/stop decision.` : `- No plan file found → say "No plan file detected." and use the Fallback Intent Sources below.
|
||||
- Plan file found but unreadable (permissions, encoding) → say "Plan file found but unreadable." and use the Fallback Intent Sources below; never report plan items as verified.`}`;
|
||||
4. **No binding and no chosen candidate:** print exactly this line, then ${ship ? 'skip dispatch and record zero counts with this line as the summary' : 'use the Fallback Intent Sources below'}:
|
||||
\`${PLAN_AUDIT_NOT_RUN}\`
|
||||
|
||||
**Error handling:** a bound or chosen plan file that is unreadable (permissions, encoding) ${ship ? 'is an audit error, not "no plan": the parent applies its audit-failure recovery and skip/stop decision.' : '→ say "Plan file found but unreadable." and use the Fallback Intent Sources below; never report plan items as verified.'}`;
|
||||
}
|
||||
|
||||
/** Ship's child audit: the parent already bound the plan, so the child never searches. */
|
||||
function planInputShip(): string {
|
||||
return `### Plan input
|
||||
|
||||
Audit only the plan the parent supplied (path or full text). Do not search for another plan. If the supplied file is unreadable (permissions, encoding), return an audit error: do not report no plan or successful zero counts.`;
|
||||
}
|
||||
|
||||
// ─── Plan Completion Audit ────────────────────────────────────────────
|
||||
@@ -483,8 +499,8 @@ function generatePlanCompletionAuditInner(mode: PlanCompletionMode, part: 'audit
|
||||
const sections: string[] = [];
|
||||
let gate = '';
|
||||
|
||||
// ── Plan file discovery (shared) ──
|
||||
sections.push(generatePlanFileDiscovery(mode === 'ship'));
|
||||
// ── Plan input: ship's parent binds the plan before dispatch (F1) ──
|
||||
sections.push(mode === 'ship' ? planInputShip() : generatePlanFileDiscovery(false));
|
||||
|
||||
// ── Item extraction ──
|
||||
sections.push(planItemExtraction(mode));
|
||||
@@ -513,7 +529,9 @@ export function generatePlanCompletionAuditShip(_ctx: TemplateContext): string {
|
||||
return generatePlanCompletionAuditInner('ship');
|
||||
}
|
||||
|
||||
export function generatePlanCompletionGateShip(_ctx: TemplateContext): string {
|
||||
/** `{{PLAN_COMPLETION_GATE_SHIP:discovery}}` renders the parent-side plan binding that runs before dispatch. */
|
||||
export function generatePlanCompletionGateShip(_ctx: TemplateContext, args?: string[]): string {
|
||||
if (args?.includes('discovery')) return generatePlanFileDiscovery(true);
|
||||
return generatePlanCompletionAuditInner('ship', 'gate');
|
||||
}
|
||||
|
||||
|
||||
@@ -11,10 +11,13 @@ and `run_in_background: false`. Use Step 7's shared foreground-dispatch rule.
|
||||
The child reads the plan and every referenced
|
||||
code file; the parent validates its report and applies the gates below.
|
||||
|
||||
**Subagent prompt:** Substitute `<base>` and supply the active plan's absolute path
|
||||
or complete text, including user-approved scope changes. If none is known, say
|
||||
so; the child runs the fallback search below. If discovery found no plan, skip
|
||||
dispatch. The child does not inherit the parent's conversation.
|
||||
**Before dispatch, the parent binds the plan:**
|
||||
|
||||
{{PLAN_COMPLETION_GATE_SHIP:discovery}}
|
||||
|
||||
**Subagent prompt:** Substitute `<base>` and supply the bound plan's absolute path
|
||||
or complete text, including user-approved scope changes. When discovery printed the
|
||||
not-run line, skip dispatch. The child does not inherit the parent's conversation.
|
||||
|
||||
````text
|
||||
You are running a ship-workflow plan completion audit. The base branch is `<base>`. Use `git diff origin/<base>` and inspect untracked files from `git status` to see the full proposed change. Do not commit or push. Report only: classify every item, but do not execute Gate Logic, ask the user, or advance the workflow. The parent applies those gates to your report.
|
||||
|
||||
@@ -138,7 +138,8 @@ describe('every resolver files a nested-group repo under the same slug', () => {
|
||||
encoding: 'utf-8',
|
||||
timeout: 30_000,
|
||||
});
|
||||
expect(r.stdout).toContain(`PLAN_FILE: ${path.join(planDir, 'plan.md')}`);
|
||||
// F1 (#2768): search results are offered as candidates, never auto-bound.
|
||||
expect(r.stdout).toContain(`PLAN_CANDIDATE: ${path.join(planDir, 'plan.md')}`);
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
@@ -3,7 +3,7 @@ import { readFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { generateReviewArmy } from '../scripts/resolvers/review-army';
|
||||
import { generateCrossReviewDedup, generateSharedCodeReuse, generateScopeDrift } from '../scripts/resolvers/review-scope';
|
||||
import { generatePlanCompletionAuditReview, generatePlanCompletionAuditShip } from '../scripts/resolvers/plan-gates';
|
||||
import { generatePlanCompletionAuditReview, generatePlanCompletionAuditShip, generatePlanCompletionGateShip, PLAN_AUDIT_NOT_RUN } from '../scripts/resolvers/plan-gates';
|
||||
import { generateQAReview } from '../scripts/resolvers/qa';
|
||||
import { generateConfidenceCalibration } from '../scripts/resolvers/confidence';
|
||||
import { HOST_PATHS, type TemplateContext } from '../scripts/resolvers/types';
|
||||
@@ -468,12 +468,15 @@ test('review keeps its smoke-clock, setup-authority, plan-gate and findings-sour
|
||||
expect(qa).not.toContain('Ask for setup/permission');
|
||||
expect(generateQAReview({ ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' })).not.toContain('/review sets none');
|
||||
const audit = generatePlanCompletionAuditReview(ctx).replace(/\s+/g, ' ');
|
||||
expect(audit).toContain('"No plan file detected."');
|
||||
expect(audit).toContain('Fallback Intent Sources');
|
||||
expect(audit).not.toContain('skip with "No plan file detected — skipping."');
|
||||
// F1 (#2768): no bound plan prints the exact not-run line instead of the old notice.
|
||||
expect(audit).toContain(PLAN_AUDIT_NOT_RUN);
|
||||
expect(audit).toMatch(/use the Fallback Intent Sources below/);
|
||||
expect(audit).not.toMatch(/skip dispatch/);
|
||||
expect(audit).toContain('**HIGH-impact plan-file discrepancies** trigger AskUserQuestion');
|
||||
expect(audit).toMatch(/derived only from fallback sources[\s\S]{0,80}never trigger this question/i);
|
||||
expect(generatePlanCompletionAuditShip({ ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' })).toContain('skip with "No plan file detected — skipping."');
|
||||
const shipCtx = { ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' };
|
||||
expect(generatePlanCompletionGateShip(shipCtx, ['discovery'])).toMatch(/skip dispatch and record zero counts/);
|
||||
expect(generatePlanCompletionAuditShip(shipCtx)).toContain('Do not search for another plan');
|
||||
const persist = skill.slice(skill.indexOf('### 2. Fill the record')).replace(/\s+/g, ' ');
|
||||
expect(persist).toMatch(/combined final-pass findings \(core, specialist, adversarial, actionable Greptile, verified exploratory QA findings\)/i);
|
||||
});
|
||||
@@ -238,3 +238,105 @@ test('push idempotency requires the live remote SHA and fails closed on transpor
|
||||
expect(unavailable.stdout).toContain('BLOCKED');
|
||||
} finally { fs.rmSync(cwd, { recursive: true, force: true }); }
|
||||
}, 120_000);
|
||||
|
||||
// F1 (#2768, #2800): the Plan Completion Audit used to fall back to "the newest
|
||||
// plan file from the last day" and to any plan naming the repo, so a branch
|
||||
// with no plan was audited against an unrelated one, and repo-committed
|
||||
// docs/designs/ was never searched. The rendered discovery block now binds
|
||||
// explicitly, lists docs/designs/ candidates, and never auto-picks.
|
||||
describe('F1: plan binding, docs/designs candidates, exact not-run line', () => {
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const NOT_RUN = 'Plan completion audit: not run (no plan is bound to this branch and no docs/designs/ file matches). Fix: add "Plan: <path>" to the PR body, or run /autoplan.';
|
||||
const sections = {
|
||||
ship: path.join(ROOT, 'ship', 'sections', 'plan-completion.md'),
|
||||
review: path.join(ROOT, 'review', 'sections', 'plan-completion.md'),
|
||||
};
|
||||
|
||||
function discovery(file: string): string {
|
||||
const md = fs.readFileSync(file, 'utf8');
|
||||
return md.slice(md.indexOf('### Plan File Discovery')).match(/```bash\n([\s\S]*?)```/)![1]
|
||||
.replaceAll('~/.claude/skills/gstack', ROOT).replaceAll('<base>', 'main');
|
||||
}
|
||||
|
||||
function fixture(opts: { design?: boolean; prBody?: string }) {
|
||||
const tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'plan-bind-')));
|
||||
const repo = path.join(tmp, 'repo');
|
||||
fs.mkdirSync(repo);
|
||||
const env = { ...process.env, HOME: tmp, GSTACK_HOME: path.join(tmp, '.gstack'), GIT_CONFIG_GLOBAL: '/dev/null', GIT_AUTHOR_NAME: 't', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t' } as Record<string, string>;
|
||||
delete env.GSTACK_STATE_ROOT; delete env.GSTACK_PROJECT_SLUG;
|
||||
const git = (...a: string[]) => spawnSync('git', a, { cwd: repo, env, encoding: 'utf8', timeout: 30_000 });
|
||||
git('init', '-q', '-b', 'main');
|
||||
fs.mkdirSync(path.join(repo, 'docs', 'designs'), { recursive: true });
|
||||
fs.writeFileSync(path.join(repo, 'docs', 'designs', 'OLD_FEATURE.md'), '# an older, unrelated design\n');
|
||||
git('add', '-A'); git('commit', '-qm', 'base');
|
||||
git('update-ref', 'refs/remotes/origin/main', 'HEAD');
|
||||
git('checkout', '-qb', 'feat-pricing');
|
||||
fs.writeFileSync(path.join(repo, 'src.ts'), 'x\n');
|
||||
if (opts.design) fs.writeFileSync(path.join(repo, 'docs', 'designs', 'PRICING.md'), '# pricing plan\n');
|
||||
git('add', '-A'); git('commit', '-qm', 'work');
|
||||
// An unrelated plan-mode file from today, naming neither branch nor feature.
|
||||
fs.mkdirSync(path.join(tmp, '.claude', 'plans'), { recursive: true });
|
||||
fs.writeFileSync(path.join(tmp, '.claude', 'plans', 'recent-unrelated.md'), '# refactor the billing page for repo\n');
|
||||
const bin = path.join(tmp, 'bin');
|
||||
fs.mkdirSync(bin);
|
||||
if (opts.prBody !== undefined) fs.writeFileSync(path.join(tmp, 'pr-body.txt'), opts.prBody);
|
||||
fs.writeFileSync(path.join(bin, 'gh'), opts.prBody === undefined ? '#!/bin/sh\nexit 1\n' : `#!/bin/sh\ncat '${path.join(tmp, 'pr-body.txt')}'\n`, { mode: 0o755 });
|
||||
env.PATH = `${bin}:${process.env.PATH}`;
|
||||
return { tmp, repo, env };
|
||||
}
|
||||
|
||||
function run(which: 'ship' | 'review', f: ReturnType<typeof fixture>) {
|
||||
return spawnSync('bash', ['-c', discovery(sections[which])], { cwd: f.repo, env: f.env, encoding: 'utf8', timeout: 30_000 });
|
||||
}
|
||||
|
||||
for (const which of ['ship', 'review'] as const) {
|
||||
test(`${which}: a docs/designs file changed on this branch is a candidate; an unrelated recent plan is not`, () => {
|
||||
const f = fixture({ design: true });
|
||||
try {
|
||||
const out = run(which, f).stdout;
|
||||
expect(out).toContain(`PLAN_CANDIDATE: ${path.join(f.repo, 'docs', 'designs', 'PRICING.md')}`);
|
||||
expect(out).not.toContain('OLD_FEATURE.md');
|
||||
expect(out).not.toContain('recent-unrelated.md');
|
||||
expect(out).not.toContain('PLAN_FILE:');
|
||||
} finally { fs.rmSync(f.tmp, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test(`${which}: nothing bound and nothing matching yields no plan at all`, () => {
|
||||
const f = fixture({});
|
||||
try {
|
||||
const out = run(which, f).stdout;
|
||||
expect(out).not.toContain('PLAN_CANDIDATE:');
|
||||
expect(out).not.toContain('PLAN_FILE:');
|
||||
expect(out).not.toContain('recent-unrelated.md');
|
||||
} finally { fs.rmSync(f.tmp, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test(`${which}: a "Plan:" line in the PR body is printed as the binding`, () => {
|
||||
const f = fixture({ prBody: 'Summary\n\nPlan: `docs/designs/PRICING.md`\n' });
|
||||
try {
|
||||
expect(run(which, f).stdout).toContain('PLAN_BINDING: docs/designs/PRICING.md');
|
||||
} finally { fs.rmSync(f.tmp, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test(`${which}: binding precedence and the exact not-run line`, () => {
|
||||
const md = fs.readFileSync(sections[which], 'utf8');
|
||||
const text = md.slice(md.indexOf('### Plan File Discovery')).replace(/\s+/g, ' ');
|
||||
expect(text).toContain(NOT_RUN);
|
||||
const order = ['Conversation context (primary)', 'PR body binding', 'Content-based search (fallback)', 'No binding and no chosen candidate'];
|
||||
const at = order.map(o => text.indexOf(o));
|
||||
expect(at.every(i => i >= 0)).toBe(true);
|
||||
expect([...at].sort((a, b) => a - b)).toEqual(at);
|
||||
expect(text).toMatch(/never pick one silently/i);
|
||||
expect(text).toContain('No plan: skip the audit');
|
||||
});
|
||||
}
|
||||
|
||||
test('ship binds the plan in the parent before dispatch; the child never searches', () => {
|
||||
const md = fs.readFileSync(sections.ship, 'utf8');
|
||||
expect(md.indexOf('### Plan File Discovery')).toBeLessThan(md.indexOf('````text'));
|
||||
const child = md.slice(md.indexOf('````text'), md.lastIndexOf('````'));
|
||||
expect(child).not.toContain('### Plan File Discovery');
|
||||
expect(child).toMatch(/Do not search for another plan/);
|
||||
expect(md).toMatch(/not-run line, skip dispatch/);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user