From b36f088b87e8973d00ee19db85a75db9a08833b5 Mon Sep 17 00:00:00 2001 From: Paulo Date: Sun, 26 Jul 2026 01:29:50 +0200 Subject: [PATCH] The code reviewer reads the diff, not the plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven build roles shared one header, so the reviewer arrived holding the plan, the acceptance criteria, every prior evaluation, and an order to treat the ticket as binding truth — and graded plan conformance, which is the evaluator's job it is told not to redo. The contract now lives in its own partial that the six contract-bearing roles include; the reviewer gets the workflow context and the SHAs, and is ordered to diff them and read every changed file end to end before it writes a finding. Its severity list gains out-of-scope edits at high — the one class only a reader of the diff can see — and loses the bullet that waved off style as long as the repo did not enforce it, which read as licence to ignore house style. SIM108 is ignored: it rewrites if/else into a ternary, and plain if/else is the house form. --- .../prompts/build/build_workflow/_contract.md | 64 ++++++++++++++++ .../prompts/build/build_workflow/_header.md | 64 +--------------- .../build_workflow/evaluate_implementation.md | 1 + .../build/build_workflow/generate_plan.md | 1 + .../prompts/build/build_workflow/implement.md | 1 + .../build/build_workflow/review_code.md | 9 ++- .../build/build_workflow/review_plan.md | 1 + .../build/build_workflow/revise_contract.md | 1 + .../build_workflow/triage_human_feedback.md | 1 + backend/tests/test_build_prompts.py | 73 ++++++++++++++++++- pyproject.toml | 2 + 11 files changed, 148 insertions(+), 70 deletions(-) create mode 100644 backend/templates/prompts/build/build_workflow/_contract.md diff --git a/backend/templates/prompts/build/build_workflow/_contract.md b/backend/templates/prompts/build/build_workflow/_contract.md new file mode 100644 index 0000000..af9b468 --- /dev/null +++ b/backend/templates/prompts/build/build_workflow/_contract.md @@ -0,0 +1,64 @@ +{% if build.ticket_ref %} +**MANDATORY FIRST ACTION — fetch the ticket. This is not a suggestion.** Your very first tool call MUST be to fetch `{{ build.ticket_ref }}` from {{ build.source | default('the tracker', true) | capitalize }} using your available tools, then read its full description and **every** comment before you read the codebase, write a plan, edit a file, or emit any output. Do not begin from the ticket reference, title, or the rendered plan alone — those are derived; the ticket and its operator comments are the binding source of truth, and frequently carry exact decisions you must honor verbatim. The ONLY acceptable reason to proceed without the ticket's full text is a genuine tool failure, which you must report as a blocker — never guess or fabricate the requirements. If the source materially contradicts a plan or acceptance criteria rendered below, surface the conflict rather than silently proceeding. + +{% endif %} +{% if build.journal.plan.plan_markdown %} +## Current plan + +{{ build.journal.plan.plan_markdown }} + +{% endif %} +{% if build.journal.plan.acceptance_criteria %} +## Acceptance criteria + +{% for ac in build.journal.plan.acceptance_criteria %} +### {{ ac.id }} + +**Description:** {{ ac.description.strip() }} + +{% if ac.verification %} +**Verification:** {{ ac.verification.strip() }} + +{% endif %} +{% endfor %} +{% endif %} +{% if build.journal.plan.rejected_approaches %} +## Ruled out + +{% for approach in build.journal.plan.rejected_approaches %} +- {{ approach.strip() }} +{% endfor %} + +{% endif %} +{% if build.journal.evaluations %} +## Prior implementation review + +{% for review in build.journal.evaluations %} +### Round {{ loop.index }} — verdict: {{ review.verdict.value if review.verdict.value is defined else review.verdict }} + +{% if review.body %} +{{ review.body.strip() }} + +{% endif %} +{% endfor %} +{% endif %} +{% if build.journal.human_feedback %} +## Human feedback + +{% for fb in build.journal.human_feedback %} +### {{ fb.reviewer }} + +{% if fb.body %} +**Body:** {{ fb.body.strip() }} + +{% endif %} +{% if fb.question %} +**Question:** {{ fb.question.strip() }} + +{% endif %} +{% if fb.implementation_instructions %} +**Implementation instructions:** {{ fb.implementation_instructions.strip() }} + +{% endif %} +{% endfor %} +{% endif %} diff --git a/backend/templates/prompts/build/build_workflow/_header.md b/backend/templates/prompts/build/build_workflow/_header.md index 98d9a2e..1048745 100644 --- a/backend/templates/prompts/build/build_workflow/_header.md +++ b/backend/templates/prompts/build/build_workflow/_header.md @@ -10,9 +10,7 @@ - **Repo:** {{ build.repo }} · branch `{{ build.branch or '(none)' }}` · PR #{{ build.pr_number or '?' }}{% if build.issue_number %} · issue #{{ build.issue_number }}{% endif %} {% endif %} {% if build.ticket_ref %} -- **Ticket:** {{ build.ticket_ref }} - - **MANDATORY FIRST ACTION — fetch the ticket. This is not a suggestion.** Your very first tool call MUST be to fetch `{{ build.ticket_ref }}` from {{ build.source | default('the tracker', true) | capitalize }} using your available tools, then read its full description and **every** comment before you read the codebase, write a plan, edit a file, or emit any output. Do not begin from the ticket reference, title, or the rendered plan alone — those are derived; the ticket and its operator comments are the binding source of truth, and frequently carry exact decisions you must honor verbatim. The ONLY acceptable reason to proceed without the ticket's full text is a genuine tool failure, which you must report as a blocker — never guess or fabricate the requirements. If the source materially contradicts a plan or acceptance criteria rendered below, surface the conflict rather than silently proceeding. +- **Ticket:** {{ build.ticket_ref }} on {{ build.source | default('the tracker', true) | capitalize }} {% endif %} - **Plan revision:** {{ build.journal.plan_revision }} - **Implementation revision:** {{ build.journal.implementation_revision }}{% if build.journal.implementation_revision == 0 %} (first attempt){% endif %} @@ -26,63 +24,3 @@ - ``workspace_root``: `{{ workspace.workspace_root }}` — clone related repos you need as ``workspace_root/related/`` {{ verification }} -{% if build.journal.plan.plan_markdown %} -## Current plan - -{{ build.journal.plan.plan_markdown }} - -{% endif %} -{% if build.journal.plan.acceptance_criteria %} -## Acceptance criteria - -{% for ac in build.journal.plan.acceptance_criteria %} -### {{ ac.id }} - -**Description:** {{ ac.description.strip() }} - -{% if ac.verification %} -**Verification:** {{ ac.verification.strip() }} - -{% endif %} -{% endfor %} -{% endif %} -{% if build.journal.plan.rejected_approaches %} -## Ruled out - -{% for approach in build.journal.plan.rejected_approaches %} -- {{ approach.strip() }} -{% endfor %} - -{% endif %} -{% if build.journal.evaluations %} -## Prior implementation review - -{% for review in build.journal.evaluations %} -### Round {{ loop.index }} — verdict: {{ review.verdict.value if review.verdict.value is defined else review.verdict }} - -{% if review.body %} -{{ review.body.strip() }} - -{% endif %} -{% endfor %} -{% endif %} -{% if build.journal.human_feedback %} -## Human feedback - -{% for fb in build.journal.human_feedback %} -### {{ fb.reviewer }} - -{% if fb.body %} -**Body:** {{ fb.body.strip() }} - -{% endif %} -{% if fb.question %} -**Question:** {{ fb.question.strip() }} - -{% endif %} -{% if fb.implementation_instructions %} -**Implementation instructions:** {{ fb.implementation_instructions.strip() }} - -{% endif %} -{% endfor %} -{% endif %} diff --git a/backend/templates/prompts/build/build_workflow/evaluate_implementation.md b/backend/templates/prompts/build/build_workflow/evaluate_implementation.md index 24412c7..1e89ba1 100644 --- a/backend/templates/prompts/build/build_workflow/evaluate_implementation.md +++ b/backend/templates/prompts/build/build_workflow/evaluate_implementation.md @@ -31,6 +31,7 @@ plan should have asked for. You verify what it did ask for, exhaustively, in a s AWARENESS rule below. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} Evaluate the implementation against the **Current plan** above, the issue, and the current PR diff. When `base_sha` and `head_sha` are listed in the **Workflow context** section above, use them as the authoritative diff range and evaluate `head_sha` against `base_sha`. If branch names disagree with those SHAs, trust the SHAs and mention metadata drift only when it affects the PR. Return blocked if an authoritative SHA is unavailable locally after fetching. Evaluate every acceptance criterion from the PR state and report one result per criterion. Inspection commands such as git diff/show, rg, and sed are allowed for review. Verification commands are different: run only the configured verification profile commands when feasible and report their results in checks. Do not invent repo-specific smoke tests or package install commands. Return exactly one final result object. Return pass only when the work is ready for a human final PR review. Return fail for actionable implementation changes. diff --git a/backend/templates/prompts/build/build_workflow/generate_plan.md b/backend/templates/prompts/build/build_workflow/generate_plan.md index fcc6457..1ccdcfa 100644 --- a/backend/templates/prompts/build/build_workflow/generate_plan.md +++ b/backend/templates/prompts/build/build_workflow/generate_plan.md @@ -48,6 +48,7 @@ your acceptance criteria. Wrong shape here compounds into every step that follow - Preserve the source's explicit out-of-scope statements near-verbatim. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} {% if answered_questions %} diff --git a/backend/templates/prompts/build/build_workflow/implement.md b/backend/templates/prompts/build/build_workflow/implement.md index 069f707..b626d47 100644 --- a/backend/templates/prompts/build/build_workflow/implement.md +++ b/backend/templates/prompts/build/build_workflow/implement.md @@ -32,6 +32,7 @@ acceptance criterion — nothing more, nothing less. catch it and the loop cost is the same — except the operator never sees your explanation. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} Implement the approved plan (rendered above as **Current plan**) on the work branch (see "Delivering your work" below). If the **Human feedback** section above carries an entry with implementation instructions, apply those instructions as the current revision request. Do not run ad hoc install, lint, test, build, or smoke commands during implementation unless the plan explicitly requires changing those commands or generated outputs. The evaluator runs and adjudicates the verification profile — you do not run or judge it; report every check as structured evidence (`not_run` or an observed baseline failure). Never leave dependency lockfile, generated, or cache changes unless they are part of the requested implementation. Return structured evidence for every acceptance criterion, every check you ran or intentionally did not run, changed files, and known risks. If a check was not run, include status not_run and a reason. diff --git a/backend/templates/prompts/build/build_workflow/review_code.md b/backend/templates/prompts/build/build_workflow/review_code.md index c5f23a2..0287e1a 100644 --- a/backend/templates/prompts/build/build_workflow/review_code.md +++ b/backend/templates/prompts/build/build_workflow/review_code.md @@ -27,6 +27,9 @@ write it? there is at least one medium or high finding. {% include "build/build_workflow/_header.md" %} + +**MANDATORY FIRST ACTION — read the diff. This is not a suggestion.** Your very first tool call MUST be `git diff ..` using the SHAs rendered above. Then read every changed file END TO END — the whole file, not the changed hunks — before writing any finding. You are reviewing code, not a plan: you have no plan and no acceptance criteria here by design, because correctness against the contract was already adjudicated by the evaluator. Fetch the ticket when you need its scope to judge a finding, and to file the follow-up sub-issue below. + {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} Review the implementation as a code reviewer, AFTER the evaluator has already passed on correctness against the acceptance criteria. Your job is the holistic quality pass: would you be happy maintaining this code in 6 months? @@ -43,15 +46,15 @@ WHAT TO LOOK FOR — beyond AC correctness, which is the evaluator's job, not yo - Foot-guns: surprising default values, unsafe casts, swallowed exceptions, missing input validation on a public boundary, log-then-continue patterns where the caller can't tell something failed. - Secret leaks, obvious injection paths, log lines that include sensitive data. - Comments and naming that lie or mislead — drift between what the comment claims and what the code does. +- Edits outside the ticket scope: a file the change touches that the ticket never asked for is a finding; name it by file path. WHAT NOT TO FLAG: - Anything the evaluator already covered (AC correctness). Don't relitigate. -- Style nits that don't change behavior or maintainability (whitespace, alphabetical import order, single vs double quote unless the repo enforces one). - "I would have done this differently" without a concrete reason tied to maintainability, performance, or correctness. - Pre-existing issues in unchanged code. You're reviewing the diff, not the codebase. SEVERITY CALIBRATION: -- high: correctness bug, security flaw, data loss path, or a duplicate of an existing helper that's already used elsewhere in the repo (someone will fix the other usages later and miss this one). +- high: correctness bug, security flaw, data loss path, an out-of-scope edit, or a duplicate of an existing helper that's already used elsewhere in the repo (someone will fix the other usages later and miss this one). - medium: test shape problem (asserting against implementation details), missing test for a non-trivial new behavior, idiomatic mismatch that will confuse future readers, log-then-continue pattern, unsafe default. - low: naming clarity, comment drift, a small refactor that would simplify the next change but isn't required now. @@ -63,7 +66,7 @@ You do the writing yourself — druks records only your one-line `summary`. 2. **Post one PR comment** on PR #{{ build.pr_number }} (`gh pr comment {{ build.pr_number }} --body ...`; the checkout is authenticated): `Code review: `. -3. **File a follow-up sub-issue — only when step 1 found real work.** Open it on the same tracker as the parent ticket (the one you fetched at the start), as a child of that ticket, using the same tracker tools. Give it a concise verb-first title (e.g. "Extract duplicate validation helper", "Add integration test for refund path"). In the body write one section per finding — the follow-up implementer reads it as spec — each covering: +3. **File a follow-up sub-issue — only when step 1 found real work.** Open it on the same tracker as the parent ticket named in **Workflow context**, as a child of that ticket, using the same tracker tools. Give it a concise verb-first title (e.g. "Extract duplicate validation helper", "Add integration test for refund path"). In the body write one section per finding — the follow-up implementer reads it as spec — each covering: - severity: high / medium / low - what's wrong, why it matters, and what good would look like - the file path and anchor line it applies to, when it has one diff --git a/backend/templates/prompts/build/build_workflow/review_plan.md b/backend/templates/prompts/build/build_workflow/review_plan.md index cc4affe..ba83c4b 100644 --- a/backend/templates/prompts/build/build_workflow/review_plan.md +++ b/backend/templates/prompts/build/build_workflow/review_plan.md @@ -31,6 +31,7 @@ implementation — your verdict is the gate. - Do not require changes beyond what the issue and the existing codebase support. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} Review the current plan in one complete pass. Batch every blocking issue and every required clarification into a single response — there is no second automatic round. diff --git a/backend/templates/prompts/build/build_workflow/revise_contract.md b/backend/templates/prompts/build/build_workflow/revise_contract.md index 07d6ab4..523a1e9 100644 --- a/backend/templates/prompts/build/build_workflow/revise_contract.md +++ b/backend/templates/prompts/build/build_workflow/revise_contract.md @@ -22,6 +22,7 @@ everything the original plan got right. - Do not add scope beyond what the human feedback introduced. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} The human reviewer's feedback contradicts the current acceptance criteria. Revise the plan and acceptance criteria to incorporate the feedback while preserving the original issue intent. The latest triaged human feedback is in the **Human feedback** section above; the current acceptance criteria are in the **Acceptance criteria** section above. Return the full revised plan markdown, the complete updated acceptance criteria list, and concise implementation instructions describing what changed so the implementer knows what to redo. diff --git a/backend/templates/prompts/build/build_workflow/triage_human_feedback.md b/backend/templates/prompts/build/build_workflow/triage_human_feedback.md index 3f5b293..50a6ab8 100644 --- a/backend/templates/prompts/build/build_workflow/triage_human_feedback.md +++ b/backend/templates/prompts/build/build_workflow/triage_human_feedback.md @@ -19,6 +19,7 @@ without ambiguity. - Do not edit files. {% include "build/build_workflow/_header.md" %} +{% include "build/build_workflow/_contract.md" %} {% include "build/build_workflow/_related_repos.md" %} {% include "build/build_workflow/_skills.md" %} Triage the latest pending entry in the **Human feedback** section above. Decide whether the feedback requires code changes, is incorrect or already addressed, needs a follow-up question, or means the PR should be closed/cancelled. Do not edit files. Use action `no_change` when no code change is needed and explain why in body. Use `change_required` only for actionable implementation work and put precise instructions for the implementer in implementation_instructions. Use `contract_change_required` when the feedback contradicts or invalidates one or more acceptance criteria — for example the human rejects a design choice that the criteria explicitly required. In that case put revised instructions in implementation_instructions explaining what changed. Use `question` when human input is needed before acting and put the exact question in question. Use `close` only when the correct action is to stop the PR. diff --git a/backend/tests/test_build_prompts.py b/backend/tests/test_build_prompts.py index ccc310b..dcd6303 100644 --- a/backend/tests/test_build_prompts.py +++ b/backend/tests/test_build_prompts.py @@ -21,6 +21,10 @@ "triage_human_feedback.md", ] +# The roles that carry the plan, the acceptance criteria, and the ticket behind +# them. The code reviewer is deliberately outside this set: it grades the diff. +_CONTRACT_TEMPLATES = [name for name in _OP_TEMPLATES if name != "review_code.md"] + # The kwargs the workflow passes at each template's agent call site. _CALL_KWARGS = { "generate_plan.md": {"answered_questions": [], "operator_note": "", "reviewer_notes": ""}, @@ -156,11 +160,12 @@ async def test_the_planner_resolves_the_assignee_not_the_reviewer(): assert "assignee_github_login" not in reviewer -@pytest.mark.parametrize("template", _OP_TEMPLATES) +@pytest.mark.parametrize("template", _CONTRACT_TEMPLATES) async def test_build_prompt_orders_the_ticket_fetch(template): - """Every build agent is ordered to fetch the ticket from its source before - acting — a mandatory first step, not a suggestion. Regression guard for the - silently-skipped-fetch bug (agents working off the ticket ref alone).""" + """Every agent that works the contract is ordered to fetch the ticket from its + source before acting — a mandatory first step, not a suggestion. Regression + guard for the silently-skipped-fetch bug (agents working off the ticket ref + alone). The code reviewer is ordered to read the diff instead.""" build = _build() build.source = "linear" output = await render_prompt( @@ -193,6 +198,66 @@ async def test_review_code_prompt_owns_its_followup_subissue(): assert '"summary"' in output # the only thing it returns +async def test_contract_context_is_omitted_only_from_code_review(): + build = _build() + build.journal = SimpleNamespace( + plan_revision=1, + implementation_revision=1, + last_implementation=SimpleNamespace(base_sha="base123", head_sha="head456"), + plan=SimpleNamespace( + plan_markdown="PLAN-MARKER", + acceptance_criteria=[ + SimpleNamespace( + id="AC-1", + description="AC-MARKER", + verification="VERIFY-MARKER", + ) + ], + rejected_approaches=["RULED-OUT-MARKER"], + ), + evaluations=[SimpleNamespace(verdict="pass", body="EVALUATION-MARKER")], + human_feedback=[ + SimpleNamespace( + reviewer="REVIEWER-MARKER", + body="FEEDBACK-MARKER", + question="QUESTION-MARKER", + implementation_instructions="INSTRUCTIONS-MARKER", + ) + ], + ) + headings = ( + "## Current plan", + "## Acceptance criteria", + "## Prior implementation review", + "## Human feedback", + ) + markers = headings + ( + "MANDATORY FIRST ACTION — fetch the ticket", + "PLAN-MARKER", + "AC-MARKER", + "RULED-OUT-MARKER", + "EVALUATION-MARKER", + "FEEDBACK-MARKER", + "QUESTION-MARKER", + "INSTRUCTIONS-MARKER", + ) + for template in _OP_TEMPLATES: + output = await render_prompt( + f"build/build_workflow/{template}", + build=build, + verification="VERIFICATION-BLOCK", + workspace=_workspace(), + **_CALL_KWARGS.get(template, {}), + ) + if template == "review_code.md": + assert all(marker not in output for marker in markers) + assert "`git diff ..`" in output + else: + positions = [output.index(heading) for heading in headings] + assert positions == sorted(positions) + assert all(marker in output for marker in markers) + + def test_build_prompt_context_covers_template_attrs(): # Every build prompt reads build.; assert BuildPromptContext carries them # all, so a template ref can never outrun the context contract. diff --git a/pyproject.toml b/pyproject.toml index 3f0358c..04f871a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -97,6 +97,8 @@ extend-exclude = ["backend/migrations/versions"] [tool.ruff.lint] select = ["E", "F", "I", "UP", "B", "SIM"] +# SIM108 rewrites if/else into a ternary; formatter-wrapped ternaries are banned here, and plain if/else is the house form. +ignore = ["SIM108"] [tool.ruff.lint.flake8-bugbear] # FastAPI dependency markers are the documented way to declare injected