feat(prompts): weave Pike's rules into develop + research prompts - #321
Merged
Conversation
Extends the existing prompts so Rob Pike's 5 Rules of Programming sit alongside the current red/green TDD guidance. Three gaps were closed: - Rules 1 & 2 (can't guess bottlenecks, measure first): planning, plan-review, implementation, and quality-review now all flag speculative optimizations that lack a benchmark. - Rule 3 (n is usually small): planning asks the planner to estimate n per loop and prefer brute force when it's bounded; issue-synthesis requires a measurement target before accepting optimization issues. - Rule 5 (data dominates): planning introduces a data-first paragraph, plan-review adds a Data Structure Review section, quality-review checks whether code is fighting the data, and spec-architect requires domain ADRs to name core data structures. No section-heading renames that tests pin on (Plan Adherence, Scope Conformance, Phase 1, analysis-brief.json, explore the codebase all preserved). The only structural change is the plan-review verdict moving from #5 to #6, which parsePlanVerdict handles via heading-name matching. 6 files, +60/-13 lines. No mechanism changes.
Collapse within-prompt redundancy in the longest prompts. Each target was a single agent reading a single message where the same concept appeared 2-3 times under different headings (Complexity Budget + Anti-Patterns + Constraints in planning; Match Existing Patterns + Minimize Changes + NO Over-Engineering + Scope Discipline in implementation; etc). Cross-prompt repetition is preserved — different agents at different steps still get the full statement. - planning.machine.js: merge Complexity Budget + Anti-Patterns + Constraints into one House Rules block; drop the duplicate Pike bullet that previously appeared in two sections. - implementation.machine.js: merge RED/GREEN and non-TDD Step 2 branches into one conditional; collapse 6 STRICT Requirements subsections into 6 House Rules lines; drop FORBIDDEN/ALLOWED symmetric listings. - quality-review.machine.js: tighten each checklist bullet and the Output Format template. Preserves every pinned test substring (Scope Conformance, Plan Adherence, tentative guide, revised approach, NOT approved, paths.plan in Scope Conformance). - issue-draft.machine.js: tighten EARS pattern listing and TDD conditional. - research/issue-synthesis.machine.js: tighten Phase 1 exploration and Rules list. Preserves 'explore the codebase' and the briefPath interpolation (pinned by research-cancel.test.js). Net: -97 lines across 5 files. 804/805 tests green (the 1 failure is pre-existing spec-architect.test.js Node version incompat, unrelated). No mechanism changes.
- implementation.machine: Step 1 now addresses every critique section (Critical Issues, Over-Engineering, Data Structure Review, Concerns), closing a stale cross-prompt contract; Step 3 skip path is explicit about keeping the existing suite green for pure refactors. - planning.machine: restore explicit "do not modify any tracked files" guardrail that was collapsed into the "do not implement code" bullet. - issue-draft.machine: restore "Output ONLY markdown" phrasing and reinstate the concrete EARS requirement templates the earlier version had spelled out. - quality-review.machine: add a compact REVIEW_FINDINGS.md skeleton so round-N context and the programmer-fix prompt see a consistent shape.
- issue-draft.machine: clarify that EARS templates are sentence forms where <...> placeholders must be substituted with concrete text, not emitted verbatim. - implementation.machine: broaden Step 1 to include the Questions critique section — answer from repo/ISSUE.md or record assumptions in the plan before continuing. - implementation.machine: rewrite the skip path so it only waives Step 2's failing-test-first requirement; Step 3 still runs and, on the skip path, the existing suite is run before and after.
- issue-draft.machine: restore the "if failing-test-first isn't practical" qualifier for low-complexity issues, realigning the test-after caveat with implementation.machine's non-TDD path. Also add an explicit negative instruction so the EARS templates' angle brackets and backticks don't leak into ISSUE.md verbatim. - planning.machine: same qualifier restored for difficulty < 3.
- implementation.machine: questions now have blocker semantics — the agent answers only from explicit repo/ISSUE.md evidence; otherwise the question + working assumption is recorded visibly under an "Open Questions" plan section so reviewers can catch it. - planning.machine: re-split "Files to Modify / Create" into separate headings so the plan-to-implementation scope contract can still distinguish edits from new-file creates (and reviewers can spot unjustified new files). - issue-synthesis.machine: brute-force-first heuristic is now gated by "unless the brief/validation names an explicit performance or scale requirement," so artifact-backed scale constraints aren't silently downgraded into optimization follow-ups. - issue-draft.machine: restore explicit append-only scratchpad semantics so iterative drafting doesn't overwrite prior-pass notes.
- implementation.machine: Step 1 now distinguishes blocking from non-blocking critique questions. Blocking questions (requirements, acceptance criteria, API/data shape) halt the implementation path and get recorded under an "## Open Questions (BLOCKING)" heading so the workflow surfaces the blocker instead of shipping a guess. Non-blocking questions still get recorded with a working assumption and proceed. - issue-synthesis.machine: brute-force-first exception now also counts user clarifications as a source of explicit performance or scale constraints, not just the brief/validation.
Two classes of lingering warnings unrelated to the prompt refactor but surfaced while working on this branch: - 4 biome useOptionalChain warnings in pr-creation.machine.js, develop-git.js, and failure-monitor.js — all `!x || !x.foo` collapsed into `!x?.foo`. - 4 TypeScript "await has no effect" warnings in implementation.machine.js and issue-draft.machine.js, caused by two async functions declaring @returns {T} instead of @returns {Promise<T>} in their JSDoc — fixed on discoverCodexSessionId and fetchIssueBody. The JSDoc was actively overriding TS's correct inference, so this was a real bug, not a false positive suppression.
canesin
force-pushed
the
feat/pike-rules-in-prompts
branch
from
April 11, 2026 05:53
4ff190f to
955ba5f
Compare
Closed
5 tasks
canesin
added a commit
that referenced
this pull request
Apr 11, 2026
* feat(prompts): weave Pike's rules into develop + research prompts Extends the existing prompts so Rob Pike's 5 Rules of Programming sit alongside the current red/green TDD guidance. Three gaps were closed: - Rules 1 & 2 (can't guess bottlenecks, measure first): planning, plan-review, implementation, and quality-review now all flag speculative optimizations that lack a benchmark. - Rule 3 (n is usually small): planning asks the planner to estimate n per loop and prefer brute force when it's bounded; issue-synthesis requires a measurement target before accepting optimization issues. - Rule 5 (data dominates): planning introduces a data-first paragraph, plan-review adds a Data Structure Review section, quality-review checks whether code is fighting the data, and spec-architect requires domain ADRs to name core data structures. No section-heading renames that tests pin on (Plan Adherence, Scope Conformance, Phase 1, analysis-brief.json, explore the codebase all preserved). The only structural change is the plan-review verdict moving from #5 to #6, which parsePlanVerdict handles via heading-name matching. 6 files, +60/-13 lines. No mechanism changes. * refactor(prompts): tighten develop + research prompts Collapse within-prompt redundancy in the longest prompts. Each target was a single agent reading a single message where the same concept appeared 2-3 times under different headings (Complexity Budget + Anti-Patterns + Constraints in planning; Match Existing Patterns + Minimize Changes + NO Over-Engineering + Scope Discipline in implementation; etc). Cross-prompt repetition is preserved — different agents at different steps still get the full statement. - planning.machine.js: merge Complexity Budget + Anti-Patterns + Constraints into one House Rules block; drop the duplicate Pike bullet that previously appeared in two sections. - implementation.machine.js: merge RED/GREEN and non-TDD Step 2 branches into one conditional; collapse 6 STRICT Requirements subsections into 6 House Rules lines; drop FORBIDDEN/ALLOWED symmetric listings. - quality-review.machine.js: tighten each checklist bullet and the Output Format template. Preserves every pinned test substring (Scope Conformance, Plan Adherence, tentative guide, revised approach, NOT approved, paths.plan in Scope Conformance). - issue-draft.machine.js: tighten EARS pattern listing and TDD conditional. - research/issue-synthesis.machine.js: tighten Phase 1 exploration and Rules list. Preserves 'explore the codebase' and the briefPath interpolation (pinned by research-cancel.test.js). Net: -97 lines across 5 files. 804/805 tests green (the 1 failure is pre-existing spec-architect.test.js Node version incompat, unrelated). No mechanism changes. * fix: address codex review feedback (round 1) - implementation.machine: Step 1 now addresses every critique section (Critical Issues, Over-Engineering, Data Structure Review, Concerns), closing a stale cross-prompt contract; Step 3 skip path is explicit about keeping the existing suite green for pure refactors. - planning.machine: restore explicit "do not modify any tracked files" guardrail that was collapsed into the "do not implement code" bullet. - issue-draft.machine: restore "Output ONLY markdown" phrasing and reinstate the concrete EARS requirement templates the earlier version had spelled out. - quality-review.machine: add a compact REVIEW_FINDINGS.md skeleton so round-N context and the programmer-fix prompt see a consistent shape. * fix: address codex review feedback (round 2) - issue-draft.machine: clarify that EARS templates are sentence forms where <...> placeholders must be substituted with concrete text, not emitted verbatim. - implementation.machine: broaden Step 1 to include the Questions critique section — answer from repo/ISSUE.md or record assumptions in the plan before continuing. - implementation.machine: rewrite the skip path so it only waives Step 2's failing-test-first requirement; Step 3 still runs and, on the skip path, the existing suite is run before and after. * fix: address codex review feedback (round 3) - issue-draft.machine: restore the "if failing-test-first isn't practical" qualifier for low-complexity issues, realigning the test-after caveat with implementation.machine's non-TDD path. Also add an explicit negative instruction so the EARS templates' angle brackets and backticks don't leak into ISSUE.md verbatim. - planning.machine: same qualifier restored for difficulty < 3. * fix: address codex review feedback (round 4) - implementation.machine: questions now have blocker semantics — the agent answers only from explicit repo/ISSUE.md evidence; otherwise the question + working assumption is recorded visibly under an "Open Questions" plan section so reviewers can catch it. - planning.machine: re-split "Files to Modify / Create" into separate headings so the plan-to-implementation scope contract can still distinguish edits from new-file creates (and reviewers can spot unjustified new files). - issue-synthesis.machine: brute-force-first heuristic is now gated by "unless the brief/validation names an explicit performance or scale requirement," so artifact-backed scale constraints aren't silently downgraded into optimization follow-ups. - issue-draft.machine: restore explicit append-only scratchpad semantics so iterative drafting doesn't overwrite prior-pass notes. * fix: address codex review feedback (round 5) - implementation.machine: Step 1 now distinguishes blocking from non-blocking critique questions. Blocking questions (requirements, acceptance criteria, API/data shape) halt the implementation path and get recorded under an "## Open Questions (BLOCKING)" heading so the workflow surfaces the blocker instead of shipping a guess. Non-blocking questions still get recorded with a working assumption and proceed. - issue-synthesis.machine: brute-force-first exception now also counts user clarifications as a source of explicit performance or scale constraints, not just the brief/validation. * chore: clear pre-existing lint + type-check warnings Two classes of lingering warnings unrelated to the prompt refactor but surfaced while working on this branch: - 4 biome useOptionalChain warnings in pr-creation.machine.js, develop-git.js, and failure-monitor.js — all `!x || !x.foo` collapsed into `!x?.foo`. - 4 TypeScript "await has no effect" warnings in implementation.machine.js and issue-draft.machine.js, caused by two async functions declaring @returns {T} instead of @returns {Promise<T>} in their JSDoc — fixed on discoverCodexSessionId and fetchIssueBody. The JSDoc was actively overriding TS's correct inference, so this was a real bug, not a false positive suppression.
canesin
added a commit
that referenced
this pull request
Apr 11, 2026
Adds src/machines/prompt-contracts.js as the single source of truth for artifact section names shared between producer and consumer prompts across develop and research machines. Rebased onto origin/dev, integrating Pike's rules prose from PR #321: - Adds "Data Structure Review" to PLANREVIEW.md contract sections. - Extends the contract with optional sectionDescriptions so producer prompts preserve their rich per-section prose while consumers still pull just the section-name list via renderCritiqueSectionList. - Adds findingFieldExamples + renderFindingExample() for the REVIEW_FINDINGS.md example block in quality-review. Producers (plan-review, planning, issue-draft, quality-review, and the research machines) now source section names from the contract so any future drift fails the test suite instead of silently breaking downstream verdict parsers.
canesin
added a commit
that referenced
this pull request
Apr 12, 2026
Adds src/machines/prompt-contracts.js as the single source of truth for artifact section names shared between producer and consumer prompts across develop and research machines. Rebased onto origin/dev, integrating Pike's rules prose from PR #321: - Adds "Data Structure Review" to PLANREVIEW.md contract sections. - Extends the contract with optional sectionDescriptions so producer prompts preserve their rich per-section prose while consumers still pull just the section-name list via renderCritiqueSectionList. - Adds findingFieldExamples + renderFindingExample() for the REVIEW_FINDINGS.md example block in quality-review. Producers (plan-review, planning, issue-draft, quality-review, and the research machines) now source section names from the contract so any future drift fails the test suite instead of silently breaking downstream verdict parsers.
canesin
added a commit
that referenced
this pull request
Apr 12, 2026
…velop + research machines (#324) * fix: use login shell in runShellSync fallback for full user PATH (#313) * chore(wip): checkpoint [skip ci] * fix: use login shell in runShellSync fallback for full user PATH Change bash -c to bash -lc in the runShellSync fallback path so that user profile additions (go, cargo, etc.) are available when the MCP server runs from a non-login context (systemd, VS Code, Claude Code). Closes #306 * fix: pre-review cleanup — correct cmd field to match -lc flag * feat(prompts): formalize cross-prompt contract registry Adds src/machines/prompt-contracts.js as the single source of truth for artifact section names shared between producer and consumer prompts across develop and research machines. Rebased onto origin/dev, integrating Pike's rules prose from PR #321: - Adds "Data Structure Review" to PLANREVIEW.md contract sections. - Extends the contract with optional sectionDescriptions so producer prompts preserve their rich per-section prose while consumers still pull just the section-name list via renderCritiqueSectionList. - Adds findingFieldExamples + renderFindingExample() for the REVIEW_FINDINGS.md example block in quality-review. Producers (plan-review, planning, issue-draft, quality-review, and the research machines) now source section names from the contract so any future drift fails the test suite instead of silently breaking downstream verdict parsers. * docs: document sectionDescriptions and renderFindingExample helpers * fix: address codex review feedback (round 1) - REVIEW_FINDINGS.md contract: add renderReviewFindingsTemplate() that sources the full block (title, finding heading, verdict line) from the registry so the quality-review producer template no longer hardcodes headings that could drift from findingFields. - issue-draft, planning: replace fragile secs[N] positional indexing with renderSectionsWithDescriptions(), a name-keyed helper that throws on missing or unknown section keys so reordering fails loudly instead of silently misaligning prose to the wrong heading. - renderSpecArchitectExample: throw on unknown mode instead of silently dropping required top-level sections. - Add tests covering the new helpers and error paths. * fix: address codex review feedback (round 2) - renderIssueBacklogExample: iterate named sections instead of sections[0..2] positional indexing; throw if the 'issues' slot is removed so silent key reordering cannot move issue objects into the wrong top-level field. - renderCritiqueSectionList: join the final item with 'and' instead of 'or' — consumers must address every critique section, not any one of them. The previous 'or' wording weakened the implementation prompt's reach over the critique. - Tests: - new producer-import check asserts each registered producer file imports from prompt-contracts.js, catching regressions to hardcoded section prose - renderIssueBacklogExample test now asserts the issue array lives at json.issues (by name) rather than json[sections[0]] - renderCritiqueSectionList test now asserts ' and ' conjunction and the absence of ' or ' Dismissed: (2) renderSpecArchitectExample hardcoded 'issueSpecs' key is already protected by the existing "all build/ingest sections are present in JSON" test; (4) renderIdeaIssueMarkdown flat-marker test scope is out of this PR's drift-prevention goal. * fix: address codex review feedback (round 3) - Tighten renderSpecArchitectExample tests to exact top-level key equality for both build and ingest modes. Previously the tests only asserted required keys were present; a stale key left in modeExamples after a section was removed from modes.*.sections would slip through silently. Now any drift between the two sources of truth fails the test. - Add a prompt-consumer import check: every consumer listed in a contract's consumers[] must import from prompt-contracts.js, with an inline allowlist for the two research runtime consumers (issue-publish, spec-render) that only destructure JS fields from the artifact and never embed section names in their own prompts. The rationale is documented in the test body so the allowlist cannot silently expand. Dismissed (repeat): renderIdeaIssueMarkdown nested-field assertions remain out of this PR's section-name-drift scope — that function predates the contract work and its test strategy is a separate concern. * fix: address codex review feedback (round 4) - implementation.machine.js: rewrite Step 1 prose so the Question section name isn't duplicated outside the render helper. The renamed "open question" / "open item" language no longer hardcodes any section label, so PLANREVIEW.md's Questions section can be renamed without updating this consumer. - prompt-contracts.js: buildIssueFieldsExample() now throws when any declared issueField lacks an entry in issueFieldExamples instead of silently falling back to "string" — schema drift for array and object-typed fields will fail at load time. - research/issue-backlog and research/spec-architect contracts: add explicit examples for every declared issueField. This uncovered and fixed real gaps (title, objective, problem, verification, estimated_effort, notes, domain were missing). - Tests: - new test asserts every issueField has an explicit example - producer import test now also requires the literal contract key (or a dedicated helper for the artifact) to appear in source, catching producers that import the registry but then re-hardcode their section names - consumer import test relabels RUNTIME_CONSUMERS as PASSIVE_CONSUMERS and documents the rationale more clearly Dismissed (repeat): renderSectionsWithDescriptions requires a description map by design — producers own dynamic per-section prose while the registry owns canonical section names, and the helper's loud-failure behavior is the intended drift guard. * fix: address codex review feedback (round 5) - renderRequiredSections, renderCritiqueSectionList, getSections now guard against contract entries without a flat 'sections' array (REVIEW_FINDINGS.md, research/spec-architect.json) and throw descriptive errors instead of a cryptic .map/undefined TypeError. - parseReviewVerdict tests now assert that every declared finding field survives round-tripping through the parser, not just that result.findings is truthy. assert.ok(findings) passed for any non-empty string, which is too weak a guarantee. - Consumer test restructure. The previous "all consumers must import the registry" test could pass when quality-review imported for its producer role while leaving PLAN.md/ISSUE.md references untouched. Replaced it with an ACTIVE_CONSUMER_PAIRS allowlist that documents the one (file, contract) pair that actually embeds contract section names in prompts — implementation.machine.js consuming PLANREVIEW.md — and requires the literal key on that single pair. All other consumer-contract pairs are passive file readers and don't need the stricter check. Dismissed (4th repeat): renderIdeaIssueMarkdown nested-field checks remain out of this PR's section-name-drift scope. * fix: address codex review feedback (round 6) - prompt-contracts.test.js: resolve contract producer/consumer file paths against the repo root (derived from import.meta.url) instead of process.cwd(). Tests now pass when invoked from any working directory, not just the repo root. Dismissed: the systemd-run.js `bash -c` → `bash -lc` change and its accompanying test are from PR #313 (already merged to main), not part of this PR's cross-prompt-contract scope. Any regression concerns about login-shell side effects belong in a separate PR against that change, not this one.
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.
Summary
Complexity Budget,Anti-Patterns,NO Over-Engineeringare already hardcoded inline). No new helpers, no steering-file mechanism changes, no exports added.What's covered
Files changed
src/machines/develop/planning.machine.js— Phase 2 gets a data-first paragraph + per-approach "expected n" bullet; Complexity Budget and Anti-Patterns each get one new bullet.src/machines/develop/plan-review.machine.js—basePromptBodyandbuildCritiqueRetryPromptboth get a new Data Structure Review required section and expanded Over-Engineering Concerns wording. Verdict renumbers from Expose stage heartbeat/liveness in coder_status and loop state #5 → Surface MCP startup health in structured results (with strict mode option) #6 (safe:parsePlanVerdictparses by heading name).src/machines/develop/implementation.machine.js— NO Over-Engineering gets a new forbidden pattern (unmeasured perf tuning); Scope Discipline gets a brute-force-first line.src/machines/develop/quality-review.machine.js—buildReviewerPrompt§3 Code Quality expanded with unmeasured-optimization and data-structure-fight checks. No renumbering — kept §3-only to preserve every existing test assertion.src/machines/research/spec-architect.machine.js— build-mode task list now requires a per-domain ADR naming core data structures.src/machines/research/issue-synthesis.machine.js— draft prompt now bars speculative optimization issues without a measurement target.Test plan
node --test test/quality-review.test.js test/research-cancel.test.js test/prompt-injection.test.js— 46/46 passingnode --test test/*.test.js— 804/805 passing (the one failure istest/spec-architect.test.jswhich usesmock.module, a Node API unrelated to my changes; verified failing on dev without this PR's changes too)npx biome check src/machines/develop/ src/machines/research/— no new warnings from this PR (two pre-existing warnings inpr-creation.machine.js, untouched here)Plan Adherence,Scope Conformance,tentative guide,revised approach,NOT approved,Spec Delta Summary,Additions,Refinements,Omissions,Phase 1,analysis-brief.json,explore the codebaseChange surface
+60 / -13 across 6 files. No mechanism changes, no new exports, no test updates needed.