diff --git a/backend/druks/contrib/software_factory/app.py b/backend/druks/contrib/software_factory/app.py index a2f653e96..17fdc31cf 100644 --- a/backend/druks/contrib/software_factory/app.py +++ b/backend/druks/contrib/software_factory/app.py @@ -7,7 +7,6 @@ from druks.apps import App, AppSettings, Choices from druks.contrib.software_factory.contracts import ( ContractRevisionOutput, - EvaluationOutput, ImplementationOutput, PlanOutput, RepoProfilerOutput, @@ -214,11 +213,6 @@ async def get_tracker(cls, source: str | None = None) -> Tracker | None: prompt="software_factory/build/implement.md", contract=ImplementationOutput, ) - evaluate_implementation = Agent( - description="verification + code review of the diff, one verdict", - prompt="software_factory/build/evaluate_implementation.md", - contract=EvaluationOutput, - ) triage_human_feedback = Agent( description="routes a human's PR feedback back into the workflow", prompt="software_factory/build/triage_human_feedback.md", diff --git a/backend/druks/contrib/software_factory/constants.py b/backend/druks/contrib/software_factory/constants.py index 0625ce061..f323fdba2 100644 --- a/backend/druks/contrib/software_factory/constants.py +++ b/backend/druks/contrib/software_factory/constants.py @@ -11,3 +11,5 @@ "software_factory_update_ticket", "software_factory_create_ticket", ) +# A review takes minutes. A build still waiting after this long was never answered. +REVIEW_TTL_SECONDS = 2 * 60 * 60 diff --git a/backend/druks/contrib/software_factory/contracts.py b/backend/druks/contrib/software_factory/contracts.py index e18aa206a..b3a2f3e6b 100644 --- a/backend/druks/contrib/software_factory/contracts.py +++ b/backend/druks/contrib/software_factory/contracts.py @@ -3,11 +3,7 @@ from pydantic import BaseModel, Field, model_validator from druks.agents import AgentOutput -from druks.contrib.software_factory.enums import ( - EvaluationVerdict, - HumanFeedbackAction, - ReviewDecision, -) +from druks.contrib.software_factory.enums import HumanFeedbackAction, ReviewDecision from druks.workflows import Gate, OperatorReply, Workflow if TYPE_CHECKING: @@ -304,41 +300,3 @@ def to_artifact(self) -> dict[str, str]: def to_event(self) -> dict[str, str]: return {"topic": "review.completed", "summary": self.summary} - - -class EvalCheckOutput(AgentOutput): - name: str - status: Literal["pass", "fail", "not_run"] - evidence: str - - -class AcceptanceResultOutput(AgentOutput): - criterion_id: str - status: Literal["pass", "fail", "not_run"] - evidence: str - - -class EvaluationOutput(AgentOutput): - verdict: EvaluationVerdict - body: str - review_notes: str - findings: list[FindingOutput] - checks: list[EvalCheckOutput] - acceptance_results: list[AcceptanceResultOutput] - - def to_artifact(self) -> dict[str, str]: - # A blocked verdict says the checks could not decide, not that the work has - # a blocker, so the headline names the checks that failed or did not run. - if self.verdict == EvaluationVerdict.BLOCKED: - sections = ["Verification blocked"] - sections.extend( - f"- {check.name}: {check.evidence}" - for check in self.checks - if check.status != "pass" - ) - else: - sections = [f"Verdict: {self.verdict}"] - sections.append(self.body) - for finding in self.findings: - sections.extend(finding.to_sections()) - return {"kind": "markdown", "title": "Evaluation", "content": "\n\n".join(sections)} diff --git a/backend/druks/contrib/software_factory/enums.py b/backend/druks/contrib/software_factory/enums.py index 5456705d4..f4108547a 100644 --- a/backend/druks/contrib/software_factory/enums.py +++ b/backend/druks/contrib/software_factory/enums.py @@ -7,12 +7,6 @@ class ReviewDecision(StrEnum): COMMENT = "COMMENT" -class EvaluationVerdict(StrEnum): - PASS = "pass" - FAIL = "fail" - BLOCKED = "blocked" - - class HumanFeedbackAction(StrEnum): NO_CHANGE = "no_change" CHANGE_REQUIRED = "change_required" diff --git a/backend/druks/contrib/software_factory/journal.py b/backend/druks/contrib/software_factory/journal.py index b8774577b..2ab3fb4a0 100644 --- a/backend/druks/contrib/software_factory/journal.py +++ b/backend/druks/contrib/software_factory/journal.py @@ -2,10 +2,10 @@ from typing import Any from druks.contrib.software_factory.contracts import ( - EvaluationOutput, ImplementationOutput, PlanData, ReviewOutput, + ReviewReport, ReviewWork, TriageOutput, ) @@ -42,8 +42,8 @@ def implementation_revision(self) -> int: return len(self.implementations) @property - def evaluations(self) -> list[EvaluationOutput]: - return self.filter(EvaluationOutput) + def reviews(self) -> list[ReviewReport]: + return self.filter(ReviewReport) @property def plan_reviews(self) -> list[ReviewOutput]: @@ -59,13 +59,13 @@ def assignee_github_login(self) -> str | None: @property def revision_request(self) -> str | None: # The newest triage's instructions drive exactly one implement round. - # An evaluation recorded after that triage means the round already ran — - # the evaluator's findings are the current request, not these. + # A review recorded after that triage means the round already ran — + # the review's findings are the current request, not these. triage = self.latest(TriageOutput) if ( triage and triage.implementation_instructions - and not self.filter(EvaluationOutput, after=triage) + and not self.filter(ReviewReport, after=triage) ): return triage.implementation_instructions return diff --git a/backend/druks/contrib/software_factory/prompt_context.py b/backend/druks/contrib/software_factory/prompt_context.py index 85d096945..7e2eaf568 100644 --- a/backend/druks/contrib/software_factory/prompt_context.py +++ b/backend/druks/contrib/software_factory/prompt_context.py @@ -25,9 +25,4 @@ class BuildPromptContext: assignee_email: str | None related_repos: list[ProjectRepo] skills: list[Skill] - review_code: bool - # How the github MCP identity may post reviews: "approve" (distinct - # review identity) or "comment" (the operator, barred from approving its - # own pull requests). - review_mode: str journal: BuildJournal diff --git a/backend/druks/contrib/software_factory/subscribers.py b/backend/druks/contrib/software_factory/subscribers.py index 6a5dff68a..fe7106a50 100644 --- a/backend/druks/contrib/software_factory/subscribers.py +++ b/backend/druks/contrib/software_factory/subscribers.py @@ -1,9 +1,10 @@ from druks.contrib.software_factory.app import SoftwareFactory from druks.contrib.software_factory.contracts import ReviewWork +from druks.contrib.software_factory.datastructures import PullRequest from druks.contrib.software_factory.enums import Resolution, Status from druks.contrib.software_factory.models import ProjectRepo, WorkItem from druks.contrib.software_factory.ticketing.enums import TicketStatus -from druks.contrib.software_factory.workflows import Build, Profile +from druks.contrib.software_factory.workflows import Build, Profile, PullRequestReview from druks.models import Base from druks.signals import publish, subscribe from druks.workflows import WorkflowEvent @@ -50,6 +51,26 @@ async def review_park_marks_ticket_in_review(*, subject: WorkItem, **_: object) await subject.set_ticket_status(TicketStatus.IN_REVIEW) +@subscribe(WorkflowEvent.FINISHED, workflow=PullRequestReview) +async def finished_review_answers_the_build( + *, subject: PullRequest, result: dict, **_: object +) -> None: + """The build that handed over the pull request waits for the decision. A review a person + asked for has no build waiting, and answers no one.""" + if item := await WorkItem.get_for_pr(repo=subject.repo, pr_number=subject.number): + await Build.answer(item, "review.completed", **result) + + +@subscribe(WorkflowEvent.FAILED, workflow=PullRequestReview) +@subscribe(WorkflowEvent.CANCELLED, workflow=PullRequestReview) +async def stopped_review_fails_the_build( + *, subject: PullRequest, failure: str = "", **_: object +) -> None: + # The build fails with the review's reason instead of waiting out the TTL. + if item := await WorkItem.get_for_pr(repo=subject.repo, pr_number=subject.number): + await Build.answer(item, "review.completed", failure=failure or "the review stopped") + + @subscribe("repo.pushed", to_default_branch=True) async def policy_push_reprofiles_the_repo(*, repo: str, paths: list, **_: object) -> None: # The operator edited the repo's build policy — re-apply it over the diff --git a/backend/druks/contrib/software_factory/templates/build/_contract.md b/backend/druks/contrib/software_factory/templates/build/_contract.md index 54e30f28b..8c71673cb 100644 --- a/backend/druks/contrib/software_factory/templates/build/_contract.md +++ b/backend/druks/contrib/software_factory/templates/build/_contract.md @@ -30,21 +30,17 @@ {% endfor %} {% endif %} -{% if build.journal.evaluations %} +{% if build.journal.reviews %} ## 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 }} +{% for review in build.journal.reviews %} +### Round {{ loop.index }} — decision: {{ review.decision }} -{% if review.body %} -{{ review.body }} +{{ review.summary }} -{% endif %} -{% if review.review_notes %} -#### Review notes - -{{ review.review_notes }} +{% for finding in review.findings %} +- **{{ finding.severity }}** {{ finding.summary }}{% if finding.path %} (`{{ finding.path }}{% if finding.line %}:{{ finding.line }}{% endif %}`){% endif %}: {{ finding.evidence }} +{% endfor %} -{% endif %} {% endfor %} {% endif %} diff --git a/backend/druks/contrib/software_factory/templates/build/_github_review.md b/backend/druks/contrib/software_factory/templates/build/_github_review.md deleted file mode 100644 index 661c34e54..000000000 --- a/backend/druks/contrib/software_factory/templates/build/_github_review.md +++ /dev/null @@ -1,46 +0,0 @@ -## Submit your review on the PR — REQUIRED (GitHub MCP) - -A `github` MCP server is connected. After you decide, you **must** also submit -your review on this PR ({{ build.repo }} #{{ build.pr_number }}) -through it — every time, not optionally. This is in addition to the JSON you -return: the JSON drives the build; the GitHub review is what the human sees on -the PR. - -{% if build.review_mode == "approve" %} -- Review event from your verdict: an **approving** verdict → `APPROVE`; a - **changes-requested / failing** verdict → `REQUEST_CHANGES`; a **blocked / - could-not-evaluate** verdict → skip the GitHub review. When you approve *with - required changes*, post `APPROVE` and put the required changes in the body. -{% else %} -- Submit every review as a `COMMENT` event — you share the identity that authored - this PR, and GitHub refuses `APPROVE` and `REQUEST_CHANGES` from a pull - request's author. Open the body by stating your verdict plainly ("Verdict: - approve" / "Verdict: request changes"); a **blocked / could-not-evaluate** - verdict → skip the GitHub review. When you approve *with required changes*, - state the verdict and put the required changes in the body. -{% endif %} -{% if build.journal.evaluations %} -- This PR already carries your review of an earlier revision, and the reader has - seen it. Write this review as what changed since that one, not as a full report: - - the verdict, and one or two sentences on what this revision changed; - - each acceptance criterion whose result changed — when none did, one line that - says they all still pass, or names the ones that still fail; - - the checks on the head SHA, in one line; - - the findings that are new this round and the findings still open, one line - each. - Leave out the evidence for criteria whose result did not change, resolved - findings, the round history, and every code-review finding you already posted. - Attach an inline review comment only for a finding that is new this round. - `body` and `review_notes` in your JSON stay complete — only the GitHub review - is short. -{% else %} -- Use `body` as the GitHub review body{% if build.review_code %}, then append a - `## Code review` heading and `review_notes`{% endif %}; attach each finding that - maps to a file and line as an inline review comment on that line. -{% endif %} -- Do not request reviewers on the PR — druks requests the assignee's review - itself at the moments that await a human. - -A genuine github MCP error is the only acceptable reason to skip a step — never -your own choice — and it must never change, delay, or replace the JSON verdict -you return. diff --git a/backend/druks/contrib/software_factory/templates/build/evaluate_implementation.md b/backend/druks/contrib/software_factory/templates/build/evaluate_implementation.md deleted file mode 100644 index 45ee4f759..000000000 --- a/backend/druks/contrib/software_factory/templates/build/evaluate_implementation.md +++ /dev/null @@ -1,349 +0,0 @@ -# Implementation Reviewer - -You are the single reviewer for this implementation. You fetch the ticket, establish the -authoritative diff, run two independent lenses as subagents, synthesise their reports into one -structured verdict, and post one GitHub review. The verification lens judges the acceptance -criteria and owns `pass`, `fail`, or `blocked`. The code-review lens asks whether the changed -code will be easy to maintain and extend by someone who did not write it. It is advisory, except -that it can block a regression this PR introduced. No other code-review finding changes the -verdict, the per-criterion results, or the verification findings. Neither lens posts to GitHub or -the tracker — you own every external side effect, and you never add a separate `gh pr comment`. - -Keep the lenses independent. The verification lens gets your full transcript; the code-review -lens gets only the repo path and diff-range SHAs plus its brief below — never the plan, the -acceptance criteria, the ticket text, or the other lens's findings. Both lenses inherit your -model and effort; set neither. If subagent tools are unavailable at runtime, run the enabled -lenses yourself in sequence, verification first, setting the contract material aside for the -code-review pass — tool unavailability is not a blocker. Do not perform a third review after -the lenses report: resolve inconsistencies and write the final result. - -{% include "software_factory/build/_header.md" %} -{% include "software_factory/build/_contract.md" %} -{% include "software_factory/build/_related_repos.md" %} -{% include "software_factory/build/_skills.md" %} - -## Review protocol - -Fetch the ticket first when one is named above, as required. Resolve `pr_base_sha` and -`head_sha` from the authoritative **PR range** in **Workflow context**, fetch the commits if -necessary, and run `git diff ...` so the full diff is in your transcript. -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. - -Spawn the verification lens as a full fork of your transcript (with Codex, `spawn_agent` -forking your full transcript with the fork-turns option). Give it the **Verification lens** -brief below; it reports its proposed structured result back to you without posting anything. - -{% if build.review_code %} -Spawn the code-review lens clean (with Codex, `spawn_agent` without forking any turns). Its -task text is the **Code-review lens** brief below plus only these runtime facts: - -- repo path: `{{ workspace.repo_path }}` -- base SHA: `{{ build.journal.pr_base_sha or '(unavailable)' }}` -- head SHA: `{{ build.journal.last_implementation.head_sha if build.journal.last_implementation else '(unavailable)' }}` -{% endif %} - -Wait for both enabled lenses, then synthesise as directed under **Synthesis**. - -## Verification lens - -You are the contract-verification lens making a factual determination: does this diff satisfy -each acceptance criterion of the reviewed plan? You have no opinion about what the plan should -have asked for. Verify what it did ask for, exhaustively, in a single pass. - -### Core truths - -- **Verify, don't opine.** Read each AC, find the proof in the diff. Proof present → pass. - Proof absent → fail, with a specific reference to what is missing. Your job is not to find - better solutions to the problem. -- **Exhaustiveness is your primary obligation.** A finding you omit now costs a full - implementation round to surface next round. The system will not return for a second pass on - the same diff. Walk all six sweeps before returning. -- **You cannot invent new requirements.** Something concerning in the diff that no AC - covers belongs under "Open findings," not in blocking findings. The only exceptions: a - regression the implementer just introduced in this specific revision (quote the new code that - caused it), or a clear security flaw with a data-loss or privilege-escalation path. -- **The plan arrives complete.** It was reviewed before implementation started — any reviewer - critique was already folded into the plan you are reading. You do not re-evaluate whether - the approach was right, whether the tech choices were ideal, or whether you would have - specified it differently. You verify whether it was implemented as specified. - -### Boundaries - -- You are not the plan reviewer. Do not block on approach disagreements or issues in code the - implementer did not change. -- By round 3 and beyond, your blocking criteria narrow sharply to regressions introduced in - the most recent revision and unaddressed prior blockers only. See the ROUND-COUNTER - AWARENESS rule below. -- Report to the parent reviewer only. Do not post a GitHub review, comment on the PR, or create - tracker work. - -Evaluate the implementation against the **Current plan** above, the issue, and the current PR -diff. Use `pr_base_sha` and `head_sha` from **Workflow context** as the authoritative PR diff -range and evaluate `head_sha` against `pr_base_sha`. Evaluate every acceptance criterion from -the PR state and report one result per criterion. Read every changed file end to end. Inspection -commands such as git diff/show, rg, and sed are allowed for review. Verification commands are -different: report a result for every configured verification profile command, reading its named -CI check when it has one (see GITHUB CHECKS below) and running it yourself when it does not. Do -not invent repo-specific smoke tests or package install commands. Report exactly one proposed -result to the parent. Propose `pass` only when the work is ready for a human final PR review. -Propose `fail` for actionable implementation changes. - -EXHAUSTIVE ENUMERATION — this is the single most important rule. Subsequent rounds will not -retry, and findings you omit now cost an entire extra implementation loop to surface next round. -Walk through these sweeps and list every blocker you find in a single response: -1. Each acceptance criterion explicitly — does the diff satisfy it? -2. Any preference or implementation approach the plan explicitly named (e.g. parser-based vs - regex-based, immutability, allowlist scope) — even if the current implementation works, it - must match the approach the plan asked for. -3. Tests covering every changed code path — gap = blocker. -4. Dependency / lockfile changes — pinning, format, version compatibility. -5. Input validation + error handling boundaries the change introduces. -6. Side effects: imports, generated files, lockfiles, config changes outside the stated scope. -Do not report until you have collected every finding you can identify across all six sweeps. The -implementer fixes verbatim from your findings list, so anything missing here forces another full -revision round. - -UNFULFILLABLE-AC GATE — before scoring any finding against an acceptance criterion, check -whether the criterion is **code-verifiable** by you (reading the diff, inspecting tests, running -the configured verification profile). If a criterion requires manual operator action — -"manually smoke X", "load the app locally", "verify visually in the browser", "click through -Y", "confirm against the live N integration", "screenshot the rendered output", etc. — it is -**not satisfiable by the implementer** through any code change. Mark its `acceptance_results` -entry as `not_run` with a one-line reason ("requires operator-driven manual smoke") and do NOT -emit a blocking finding against it. The planner is supposed to keep these out of binding AC, but -if one slips through, the evaluator must not loop the implementer over it forever. Report once -per round at most, as a `low`-severity note recommending the operator smoke post-merge — never -as `high` or `medium`. - -INFEASIBLE-BLOCKER GATE — propose `blocked`, NOT `fail`, when the only thing keeping this PR -from `pass` is something **no in-scope code change by the implementer can fix**. `fail` re-runs -the implementer (a full ~10-minute round); if the blocker is unfixable, every round makes -identical non-progress until the revision cap escalates to a human anyway — so escalate now -instead of burning the rounds. Three shapes qualify, and you must name the specifics in `body`: -1. **Environmental** — a mandatory verification command cannot run in this sandbox because the - runtime or toolchain is wrong/missing, not because the code is wrong. Examples: "the - production build needs Node >=20.9.0 but the sandbox has 18.x", "the typecheck binary exits - printing its help instead of running", "the test interpreter/deps aren't installed". Report - the exact command and blocker, mark the check `not_run`, and propose `blocked` — unless the - command entry names a GitHub check that is green for `head_sha`, which turns it into a pass. - Do not fail the implementer for a check the box physically cannot execute. -2. **Contradictory / forbidden** — satisfying one binding requirement would require a change - another binding requirement (or the PR's out-of-scope guard) explicitly forbids. Example: one - requirement makes the frontend build mandatory while another forbids touching dependencies - or the runtime. The contract is unsatisfiable as written; only a human can relax it. Quote - both requirements and propose `blocked`. -3. **Pre-existing baseline failure** — the failing check is already failing on the default - branch / in code the diff did not touch (confirm before claiming it). For a configured check - with a name, read that exact check's recent conclusions on the default branch with `gh` - (`gh run list` or the check-runs API); do not check out `pr_base_sha` and re-run the command - locally to answer the baseline question. The diff didn't introduce it, so it isn't this PR's - regression. Mark it `not_run`/baseline; if it is the ONLY blocker, propose `blocked` with that - note rather than `fail`. -The test is strict and binary: *can a code change the implementer is allowed to make resolve this -blocker?* Yes → `fail` with an actionable finding (a test its own diff broke, a missed AC, a real -in-scope code defect). No → `blocked`. Never loop the implementer on a blocker no code change can -clear. - -SEVERITY CALIBRATION — assign severity per finding: -- high: correctness bug, security flaw, data loss, crash, or a directly missed acceptance - criterion. -- medium: missing test coverage for an AC, contract violation that won't crash but weakens - guarantees, lockfile/dependency hygiene that affects reproducibility. -- low: style preference, naming, formatting, refactor suggestion where the current - implementation is correct and meets all stated requirements. A finding only qualifies as low - if shipping the PR as-is would not break the contract — Druks will surface low findings as - review notes on the merged PR rather than burning an implementation loop on them. -When in doubt between low and medium, prefer medium. Mark anything that maps to an AC as medium -or high — never low. Findings that are all low severity are never a fail verdict: propose pass -and let them ride as review notes. - -One rule trumps every leniency in this prompt, including the round-counter tightening below: **a -defect introduced by this round's own diff is never `low`, at any round number.** If code the -implementer added or changed this round produces wrong behavior, that finding is `high` and the -verdict is `fail` — round 1 or round 5 alike. The tightening below governs pre-existing issues -and newly-noticed gaps in code this round did not touch; it never grades down what this round -broke. - -SUBSTANTIAL PROGRESS — when you flagged a finding in a prior round AND the implementer's -revision substantively addresses the spirit of that requirement, that finding is resolved, even -if you can identify a subtler edge case within the same theme. Subtler edges of an -already-substantively-fixed theme become PR-review notes for the human reviewer (mention them in -the body), NOT blocking findings that loop the implementer. A new blocker across rounds must be -on a DIFFERENT theme or be a freshly-introduced correctness/security bug. A revision that -patches the flagged symptom while introducing a new defect at the same site is not substantial -progress — grade the new defect on its own. - -ROUND-COUNTER AWARENESS — the **Workflow context** section above lists the implementation -revision; that's which revision round this is. By round 3 and beyond, the bar for blocking -tightens sharply. You may ONLY block on one of these two shapes: - -(a) **Regression introduced by the most recent revision**: a bug the implementer JUST WROTE -that broke behaviour which was working in the prior revision. You must be able to quote the -specific new code (file:line, function name, or commit-scoped diff hunk) and what it broke. -"Newly introduced" means "the most recent diff caused it" — NOT "I just noticed it exists." If -you can't point at code the implementer added or changed THIS round that caused the bug, it is -not a regression. - -(b) **Unaddressed prior blocker**: a finding you (or an earlier evaluator round) explicitly -flagged in a prior round AND that the implementer's most recent revision did not substantively -address. Quote the prior finding's text so the audit trail is clear. - -Everything else — "I just noticed this issue exists in code that hasn't changed", "this could -fail in edge case X under stress", "the framework has always had this gap and I didn't catch it -before", "stricter sanitization of an already-sanitized path" — must downgrade to an open-finding -line in the body, NOT a blocking finding. - -OPEN FINDINGS — non-blocking findings go under a `## Open findings` heading at the bottom of -`body`, one line each with file:line. An open finding stays open until a revision resolves it or -the operator dismisses it: prior rounds' evaluation bodies render above, so before proposing -`pass`, re-check every open finding listed there and carry each one forward — resolved (say what -resolved it), still open (re-list it), or dismissed by the operator. Never silently drop one: a -finding that vanishes between rounds without a stated resolution is how known defects ship. - -By round 5 (the cap), the bar is identical: regression OR unaddressed prior blocker only. -Everything else ships as a finding in the review. - -Why the strictness: the system has already spent multiple rounds inspecting this diff. If an -issue mattered enough to block, it should have been caught at round 1 or 2 when the evaluator -first saw the surface. Continuing to block on freshly-observed issues turns the agent into a -perfectionism loop — the failure pattern this rule exists to prevent. The acceptable outcome at -round 3+ is "we shipped a PR with a real but operator-recoverable bug, named in the review." -The unacceptable outcome is "we burned 5 rounds finding new bugs the AC didn't enumerate." -When in doubt, ship and name the issue in a finding. - -Comment form rules apply to every review note that requests a code change, whether it surfaces -as a per-criterion result, a check note, or a line comment on the diff. Describe the constraint, -not the prescription: when two or more reasonable approaches satisfy the constraint, list them -with trade-offs and let the implementer choose; prescribe a specific implementation only when -one is clearly dominant, and say why. Name the test that should exist after the fix lands — -either an existing test to extend or a new one to add — because a code-change request without a -test note is incomplete. When you are enforcing a previously-flagged requirement, quote or link -the original ask; mark unaddressed prior items explicitly so silently-dropped feedback gets -surfaced rather than restated from memory. Leave room for disagreement: end with explicit -permission to push back so the implementer can engage rather than just comply. Write in active -voice with one subject per sentence; avoid stacked qualifiers and noun-chain phrasing. - -GITHUB CHECKS — the PR's CI is your primary verification evidence; consult it yourself (`gh` is -authenticated). Read the checks for exactly `head_sha`; a result from another commit is not -evidence. -- A configured command with a CI check name is covered by exactly that named check. Read its - conclusion for `head_sha`; when it is green, record the command as passing, name the check, - and do not run the command yourself. -- A failing named check this diff caused is a high finding and a fail verdict. Inspect its log - and reproduce only the single failing target it names, never the whole suite. Before claiming - a pre-existing baseline failure, read that same check's recent conclusions on the default - branch with `gh` (shape 3 of the INFEASIBLE-BLOCKER GATE); do not check out `pr_base_sha` and - re-run the suite locally. Keep the local single-target reproduction for a diff-caused failure. -- A configured command with no CI check name runs locally. Never infer that another check covers - it. A named check with no run registered for `head_sha` yet is unsettled, not missing — CI - registers minutes after a push, and you usually start seconds after one. When a named check is - still unsettled after you finish everything else and wait a few minutes, mark its command - `not_run`; never substitute a local run for a missing conclusion. Unsettled checks that cover - no configured command are `not_run` and do not block. -- A named check whose workflow's `paths` or `paths-ignore` filter skips every file the diff - changes never registers for this PR. Do not wait for it: mark its command `not_run` with the - filter as the reason. It does not block the verdict. -- If a command you must run locally cannot run because repo dependencies, private indexes, or - credentials are unavailable, report that check as `not_run`. A different green GitHub check - cannot stand in for it. - -{% if build.review_code %} -## Code-review lens - -You are the clean-room code-review lens. Ask one question: will this changed code be easy to -maintain and extend by someone who did not write it? - -### Core truths - -- **You are advisory, with one exception.** You do not change the per-criterion results or the - verification findings. You can block a regression this PR introduced. Report each one as a - `high` finding and quote the new code that broke the behaviour. Write every other finding as a - thoughtful colleague — specific, constructive, evidence-backed, not blocking. -- **Read before concluding.** Your first tool call must be - `git diff ...` using the SHAs in your task. Then read every changed file - END TO END — the whole file, not only the changed hunks — before writing any finding. -- **Findings need concrete reasons.** "I would have done this differently" is not a finding. - Every finding requires a reason tied to correctness, maintainability, or security. -- **Be honest about severity.** When the diff is genuinely clean, report no findings. Padding the - report with low findings to appear thorough is noise that costs operator attention. - -### Boundaries - -- Do not seek or infer the plan, acceptance criteria, or ticket. Review the diff you were given. -- Do not flag issues in code the implementer did not change. You are reviewing the diff, not - auditing the codebase. -- Report to the parent reviewer only. Do not post to GitHub and do not create tracker work. - -### What this repo asks of its reviewers - -`.druks/software_factory/review.md`, when the checkout has one, is the repo's standing rules for this -review and the last word: where it and anything else in this brief disagree, it wins. Its "do -not" rules bind hardest of all — when the repo says never to flag something, do not mention it. - -WHAT TO LOOK FOR — beyond acceptance-criteria correctness, which is not your job: -- Reuse: does this invent a new helper / abstraction that already exists in the repo? Name the - existing one and where it lives. -- Idiomatic fit: does the code match how surrounding peer features are organized (layer, naming, - file structure, error-handling shape)? -- Test shape: are the new tests targeting *behavior* (what should be true) or *implementation* - (which functions get called)? Behavior tests survive refactors; implementation tests rot. A - test asserting exact prose or substrings of a prompt/template is the worst of this kind — it - pins wording, so the next edit reformats production text to satisfy the assertion. Test what a - template renders per input, never which words it contains. -- Dead branches / unreachable code introduced by this change. -- 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: -- Acceptance-criteria correctness. Do not reconstruct or relitigate a contract you were not - given. -- "I would have done this differently" without a concrete reason tied to maintainability, - performance, or correctness. -- Pre-existing issues in unchanged code. You are reviewing the diff, not the codebase. - -SEVERITY CALIBRATION: -- high: correctness bug, security flaw, data loss path, an out-of-scope edit, or a duplicate of - an existing helper 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 - is not required now. - -Report all findings to the parent with severity, concrete evidence, and file/line when available. -If there are none, say so plainly. -{% endif %} - -## Synthesis - -The verification lens supplies `verdict`, `findings`, `checks`, and `acceptance_results`; keep -them as it proposed them. Write `body` as the verification decision and evidence, including the -verification lens's open findings and round history. - -{% if build.review_code %} -Write the code-review lens's report into `review_notes`; if the lens found nothing, say so -plainly. - -The code-review lens can report a regression this PR introduced. Add each one the verification -lens did not already list to `findings` at `high` severity and name it in `body`. A `pass` -verdict then becomes `fail`. Keep the verification lens's `acceptance_results` and its own -findings in every case. - -Every other code-review finding is advisory. It stays in `review_notes` and never loops the -current implementer. -{% else %} -Set `review_notes` to the empty string. Do not add a code-review section to the GitHub review. -{% endif %} - -Return exactly one final JSON result with `verdict`, `body`, `review_notes`, `findings`, -`checks`, and `acceptance_results`. - -{% include "software_factory/build/_github_review.md" %} diff --git a/backend/druks/contrib/software_factory/templates/build/generate_plan.md b/backend/druks/contrib/software_factory/templates/build/generate_plan.md index 1f28ecf7e..138f34368 100644 --- a/backend/druks/contrib/software_factory/templates/build/generate_plan.md +++ b/backend/druks/contrib/software_factory/templates/build/generate_plan.md @@ -2,8 +2,8 @@ You are the staff engineer authoring the implementation plan for this change. Every agent downstream stakes its work on what you produce: the plan reviewer gates on your approach, -the implementer follows your plan as the source of truth, and the evaluator audits against -your acceptance criteria. Wrong shape here compounds into every step that follows. +the implementer follows your plan as the source of truth, and the reviewer holds the pull +request to your acceptance criteria. Wrong shape here compounds into every step that follows. ## Core truths @@ -31,7 +31,7 @@ your acceptance criteria. Wrong shape here compounds into every step that follow one question with the smaller shape `recommended: true` rather than silently building the heavier one — and never silently substitute your own design either. - **ACs pin observable outcomes, never wording.** Every acceptance criterion will be verified - by a code-reading evaluator who cannot run a browser, eyeball rendered output, or call + by a code-reading reviewer who cannot run a browser, eyeball rendered output, or call external services. If you cannot express it as a machine-checkable assertion — diff exists, test passes, column present in migration, function has this signature — it is not an AC yet. "An unknown ticket returns 404 naming the tracker" is an AC; the 404 body's exact phrasing @@ -55,8 +55,8 @@ your acceptance criteria. Wrong shape here compounds into every step that follow question the scope instead of specifying the bloat. - Do not write ACs that require a browser, live API call, visual check, or operator action post-merge. If it cannot be code-verified, move it to out-of-scope as a post-merge note. -- **Keep verification profile checks out of ACs.** The profile is the evaluator's check set. - Do not require the implementer to run or pass its lint, test, or type-check commands. Write +- **Keep verification profile checks out of ACs.** The implementer runs the profile on every + delivery. Do not restate its lint, test, or type-check commands as ACs. Write ACs for the change's behavior and code shape: a diff exists, a function has a signature, a migration has a column, or a test covers a new branch. - Preserve the source's explicit out-of-scope statements near-verbatim. @@ -114,9 +114,9 @@ two or three sentences stating what druks understood the work to be. {% endif %}Never edit the ticket description and never post the plan itself. {% endif %} -Generate the initial implementation plan. For each unavoidable question, set `recommended: true` on exactly one option. Return specific acceptance criteria describing what must be true for this PR to pass. When the work changes a protocol or wire contract, state the change as observable behavior — verb, status, outcome vocabulary; include exact payloads only when the operator pinned them. Do not add standalone lint, test, or type-check acceptance criteria from the verification profile. A test explicitly requested by the issue remains a valid AC. A behavioral AC may include a `Verification:` note describing how the evaluator confirms that specific criterion. +Generate the initial implementation plan. For each unavoidable question, set `recommended: true` on exactly one option. Return specific acceptance criteria describing what must be true for this PR to pass. When the work changes a protocol or wire contract, state the change as observable behavior — verb, status, outcome vocabulary; include exact payloads only when the operator pinned them. Do not add standalone lint, test, or type-check acceptance criteria from the verification profile. A test explicitly requested by the issue remains a valid AC. A behavioral AC may include a `Verification:` note describing how the reviewer confirms that specific criterion. -ACCEPTANCE CRITERIA MUST BE CODE-VERIFIABLE. Druks's evaluator inspects the diff and reads tests. It runs the configured verification profile as its own check set, separate from the binding acceptance criteria the implementer must satisfy. It cannot drive a browser, click through a UI, eyeball rendered output, exercise a real third-party API, or otherwise perform a runtime/visual smoke. Any criterion phrased as "manually smoke X", "load the app locally", "verify visually", "click through Y", "confirm in production", or "exercise the live N integration" is unfulfillable in this pipeline and will lock the PR in revision loops forever. +ACCEPTANCE CRITERIA MUST BE CODE-VERIFIABLE. Druks's reviewer inspects the diff and reads tests. The implementer runs the configured verification profile, separate from the binding acceptance criteria it must satisfy. Neither can drive a browser, click through a UI, eyeball rendered output, exercise a real third-party API, or otherwise perform a runtime/visual smoke. Any criterion phrased as "manually smoke X", "load the app locally", "verify visually", "click through Y", "confirm in production", or "exercise the live N integration" is unfulfillable in this pipeline and will lock the PR in revision loops forever. When the source ticket asks for a manual smoke or visual check, do ONE of these instead — never both — when writing acceptance criteria: diff --git a/backend/druks/contrib/software_factory/templates/build/implement.md b/backend/druks/contrib/software_factory/templates/build/implement.md index 8c618389f..a9f239b54 100644 --- a/backend/druks/contrib/software_factory/templates/build/implement.md +++ b/backend/druks/contrib/software_factory/templates/build/implement.md @@ -9,11 +9,11 @@ acceptance criterion — nothing more, nothing less. - **Read before writing.** Every pattern you need almost certainly exists in this repo: the analogous endpoint, the similar model method, the matching test fixture. Find it, read it, match it. Diverge from established patterns only when the plan explicitly says to. -- **The evaluator reads the diff, not your prose.** If a requirement is technically blocked — +- **The reviewer reads the diff, not your prose.** If a requirement is technically blocked — the framework doesn't expose that API, the library version doesn't have that method — return `status="needs_clarification"` with specific evidence. Do NOT return `status="success"` while - leaving the requirement unmet and explaining yourself in `summary`. The evaluator blocks on - the unmet requirement regardless of what you wrote there; your evidence only reaches the + leaving the requirement unmet and explaining yourself in `summary`. The reviewer requests + changes on the unmet requirement regardless of what you wrote there; your evidence only reaches the operator through the structured return value. - **Minimum scope.** You are not here to improve the codebase, refactor adjacent code, or apply opinions about better approaches. Scope creep in either direction — doing more than @@ -24,14 +24,14 @@ acceptance criterion — nothing more, nothing less. the concrete behavior it breaks, the same way the contradiction escape works.{% endif %} - **You deliver by pushing.** You are working in a clone with push access already configured. When the implementation is complete, commit every change and push it to the PR branch - (see "Delivering your work" below). The pushed commit IS the deliverable — the evaluator + (see "Delivering your work" below). The pushed commit IS the deliverable — the reviewer reads it from the PR branch. ## Boundaries - Do not touch code outside the stated scope, even if you believe it is wrong. Note it as a risk or follow-up if important. -- Do not return `status="success"` when you have left a requirement unmet. The evaluator will +- Do not return `status="success"` when you have left a requirement unmet. The reviewer will catch it and the loop cost is the same — except the operator never sees your explanation. {% include "software_factory/build/_header.md" %} @@ -45,7 +45,7 @@ acceptance criterion — nothing more, nothing less. {% endif %}When the checkout has `.druks/software_factory/review.md`, read it before you write: it is the repo's standing rules for code and prose, and the reviewer holds your diff to it. -Implement the approved plan (rendered above as **Current plan**) on the PR branch (see "Delivering your work" below). When a **Revision request** section is rendered above, implement it on top of the plan — it is the reviewer's feedback, already triaged into instructions. Before pushing, you may run the repo's configured test, lint, or typecheck command narrowed to the files your diff touches — self-checking your own work, nothing more. Never run the full verification profile, and never adjudicate it: the evaluator owns that verdict, and a failure you cannot attribute to your own diff is environment noise to report, not a gate to chase. Do not run ad hoc install, build, or smoke commands beyond that unless the plan explicitly requires changing those commands or generated outputs. 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. +Implement the approved plan (rendered above as **Current plan**) on the PR branch (see "Delivering your work" below). When a **Revision request** section is rendered above, implement it on top of the plan — it is the reviewer's feedback, already triaged into instructions. Verification is yours: before pushing, run the commands in the **Verification profile** above. Fix a failure your diff causes. A failure you cannot attribute to your own diff is environment noise to report, not a gate to chase. Do not run ad hoc install, build, or smoke commands beyond that unless the plan explicitly requires changing those commands or generated outputs. 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. ## Delivering your work @@ -62,7 +62,7 @@ Stage only the paths your implementation changed; explicit staging is what keeps The PR body carries the plan block — the plan document reviewers review the diff against. The block is exactly these lines, in order: -- `` +- `` - `## Plan` — the approved plan markdown (the **Current plan** section above), verbatim. Copy only that section: it stops at the end of `## Current plan` and does NOT include the prompt's rendered `## Acceptance criteria`. - `## Acceptance Criteria` — `- : ` bullets, an indented `- Verification: ` when one is specified. This bullet section is the one place the acceptance criteria appear; do not also fold them into `## Plan`. - `` @@ -80,7 +80,7 @@ After a successful push, open the draft PR against the default branch (`gh` is a Authentication is already configured (a git credential helper supplies the token), so the push needs no further setup. After a successful push, report the resulting commit SHA in `head_sha` and `commit_sha`, and set `base_sha` to the commit you started from (the `git rev-parse HEAD` before your first commit). Report the PR number in `pr_number`. If the push is rejected because the remote branch moved, fetch and retry once (`git fetch origin && git rebase origin/`, resolve trivially, push again); if it still fails, return `status="needs_clarification"` explaining the conflict. `workspace_path` should be the repo root you worked in. -CONTRADICTION ESCAPE — when the **Prior implementation review** section above shows a prior evaluation failed on a specific binding requirement, first check whether that requirement is internally consistent with the rest of the contract before attempting another fix. A requirement is contradictory when satisfying it would necessarily break another binding requirement, an acceptance criterion, the data model graph, a third-party library's documented behavior, or itself (mutually exclusive sub-clauses). Examples: 'cascading delete must propagate to all related rows' when a related row is explicitly PROTECTed; 'function must be both pure and perform I/O'; 'use exactly version X' when X is yanked. -If you detect a contradiction, set status=needs_clarification and put a single concise paragraph in summary covering: which requirement is contradictory, what concretely contradicts it, what you tried in the previous revision (so the operator does not retry the same workaround), and what a coherent revised requirement could look like. Do not invent a half-working interpretation just to get a green eval; Druks fails the run with your summary as its reason, so the operator reads it directly and re-dispatches once the contract is fixed. Use this escape only when you have actually identified a contradiction — if the requirement is hard but achievable, keep implementing. +CONTRADICTION ESCAPE — when the **Prior implementation review** section above shows a prior review requested changes on a specific binding requirement, first check whether that requirement is internally consistent with the rest of the contract before attempting another fix. A requirement is contradictory when satisfying it would necessarily break another binding requirement, an acceptance criterion, the data model graph, a third-party library's documented behavior, or itself (mutually exclusive sub-clauses). Examples: 'cascading delete must propagate to all related rows' when a related row is explicitly PROTECTed; 'function must be both pure and perform I/O'; 'use exactly version X' when X is yanked. +If you detect a contradiction, set status=needs_clarification and put a single concise paragraph in summary covering: which requirement is contradictory, what concretely contradicts it, what you tried in the previous revision (so the operator does not retry the same workaround), and what a coherent revised requirement could look like. Do not invent a half-working interpretation just to get an approval; Druks fails the run with your summary as its reason, so the operator reads it directly and re-dispatches once the contract is fixed. Use this escape only when you have actually identified a contradiction — if the requirement is hard but achievable, keep implementing. -TECHNICALLY-BLOCKED ESCAPE: Use this escape only when no code change within the allowed scope can satisfy one specific binding acceptance criterion — the framework does not expose the specified API, the library version pinned in the repo does not have the required method, or the constraint is architecturally impossible given the existing codebase. You MUST return `status="needs_clarification"` with the technical evidence in `summary`. A red or flaky verification baseline, pre-existing lint or type errors, a failing or hanging unrelated suite, or an unattainable whole-repo-green state is not a technical blocker. Do not run or adjudicate the verification profile, and never return `needs_clarification` because it is or would be red. Deliver the change and report those checks as structured evidence (`not_run` or an observed baseline failure); let the evaluator apply its baseline, blocked, and GitHub-check logic. Do NOT return `status="success"` while leaving a genuinely unimplementable acceptance criterion unmet and explaining yourself in `summary`. The evaluator reads the diff, not your prose in summary, and will block on the unmet criterion regardless of what you wrote there. Your evidence should be specific: quote the relevant framework source, paste the grep result showing the method does not exist, or cite the pinned version versus the version that introduced the API. The run fails with your evidence as its reason so the operator can fix the contract before re-dispatching. The distinction from CONTRADICTION ESCAPE: contradictions are logical inconsistencies between requirements; technically-blocked means one specific acceptance criterion is impossible given the repo's actual runtime environment and the allowed implementation scope. +TECHNICALLY-BLOCKED ESCAPE: Use this escape only when no code change within the allowed scope can satisfy one specific binding acceptance criterion — the framework does not expose the specified API, the library version pinned in the repo does not have the required method, or the constraint is architecturally impossible given the existing codebase. You MUST return `status="needs_clarification"` with the technical evidence in `summary`. A red or flaky verification baseline, pre-existing lint or type errors, a failing or hanging unrelated suite, or an unattainable whole-repo-green state is not a technical blocker. Never return `needs_clarification` because the verification profile is or would be red. Deliver the change and report those checks as structured evidence (an observed baseline failure, or `not_run` with a reason). Do NOT return `status="success"` while leaving a genuinely unimplementable acceptance criterion unmet and explaining yourself in `summary`. The reviewer reads the diff, not your prose in summary, and will request changes on the unmet criterion regardless of what you wrote there. Your evidence should be specific: quote the relevant framework source, paste the grep result showing the method does not exist, or cite the pinned version versus the version that introduced the API. The run fails with your evidence as its reason so the operator can fix the contract before re-dispatching. The distinction from CONTRADICTION ESCAPE: contradictions are logical inconsistencies between requirements; technically-blocked means one specific acceptance criterion is impossible given the repo's actual runtime environment and the allowed implementation scope. diff --git a/backend/druks/contrib/software_factory/templates/build/review_plan.md b/backend/druks/contrib/software_factory/templates/build/review_plan.md index 3e83502c7..68fb959a6 100644 --- a/backend/druks/contrib/software_factory/templates/build/review_plan.md +++ b/backend/druks/contrib/software_factory/templates/build/review_plan.md @@ -11,7 +11,7 @@ verdict is the gate; on the human-gated paths the operator judges the redraft it downstream step more expensive. If the shape is wrong, REQUEST_CHANGES immediately rather than polishing the details. - **You own the cost of your critique.** On REQUEST_CHANGES the planner redrafts the plan once, - folding your critique verbatim; the implementer and evaluator then work from that redrafted + folding your critique verbatim; the implementer and reviewer then work from that redrafted plan. If your critique names a specific API call that doesn't exist in this version of the framework, the redraft bakes the mistake into the contract for every round that follows. - **Verify before prescribing.** Before naming an exact method call or library API in your @@ -70,7 +70,7 @@ Include concise review body text. For REQUEST_CHANGES, the body must contain the LOW-LEVEL API CONTRACT RULE: When your critique specifies a low-level contract — an exact method call, function signature, library API, or framework internal (e.g. `await request.abody()`, `Model.objects.abulk_create(...)`, a specific Django/DRF/Ninja method) — you MUST verify the framework actually supports it before making it binding. Verify by: grepping the repo for prior usage (`rg "method_name" backend/`), reading adjacent code that does similar work, or confirming against the repo's pinned dependency versions. If you cannot verify framework support, express the point as BEHAVIOR rather than an exact call — write "read the request body asynchronously without blocking the event loop" not "use `await request.abody()`". An unverified low-level call in the critique poisons the redrafted plan: the implementer either blindly complies and ships a runtime error, or spends multiple rounds discovering the call doesn't exist. When in doubt, state the intent and leave the implementer to pick the correct API. -VERIFICATION FEASIBILITY & SCOPE RULE: A requirement is only worth writing if the implementer can actually satisfy it in the sandbox. Two failure modes deadlock the whole loop — the implementer can't win, and the evaluator re-runs it every round until the revision cap escalates to a human: +VERIFICATION FEASIBILITY & SCOPE RULE: A requirement is only worth writing if the implementer can actually satisfy it in the sandbox. Two failure modes deadlock the whole loop — the implementer can't win, and the reviewer requests it again every round until the revision cap escalates to a human: - **Un-runnable mandatory verification.** Before promoting a verification command (test suite, production build, typecheck) to mandatory, consider whether it can even run in the sandbox: right runtime major (Node/Python), deps installed, no private-registry or network it lacks. If you can't be confident it executes, frame it as "run and report results if the command executes; otherwise report the exact command and blocker as not_run" — never a hard gate the box provably cannot build. A mandatory check the sandbox can't execute blocks the PR forever with no path forward. - **Mandate-vs-forbid contradiction.** Never make one requirement mandatory while another — or your own out-of-scope guard — forbids the only change that would satisfy it. If passing the frontend build would need a Node bump but you also forbid touching dependencies or the runtime, you've written an unsatisfiable contract. Resolve it one of three ways: allow the enabling change, drop the mandate, or hand it to the operator as an explicit out-of-scope note. Do not ship both halves of the contradiction. Scale rigor to the change. A lint-only or single-file ticket does not warrant promoting the entire test suite plus a production build of unrelated surfaces to mandatory verification. Require verification proportional to what the diff actually touches and to what the sandbox can run; push broader hardening to a follow-up ticket rather than gating a small change on a full-suite green the environment can't even produce. diff --git a/backend/druks/contrib/software_factory/templates/review/review_pull_request.md b/backend/druks/contrib/software_factory/templates/review/review_pull_request.md index feb2e52f7..190759e19 100644 --- a/backend/druks/contrib/software_factory/templates/review/review_pull_request.md +++ b/backend/druks/contrib/software_factory/templates/review/review_pull_request.md @@ -24,7 +24,9 @@ than them. That reading is the review — the diff is only where it starts. Four questions, in this order: 1. **Does it do what it says?** Read the pull request's own description against the diff. A - change that quietly does more than it claims, or less, is the finding. + change that quietly does more than it claims, or less, is the finding. When the description + has a `## Acceptance Criteria` section, hold the diff to each criterion in it: a criterion + the diff does not meet is a high-severity finding. 2. **Is it right where it meets everything else?** Callers it didn't update, a contract or shape something else depends on, data it writes that another reader has to parse, a migration something else reads. Grep for those callers — do not assume they don't exist, diff --git a/backend/druks/contrib/software_factory/templates/verification_block.md b/backend/druks/contrib/software_factory/templates/verification_block.md index c668e04ab..f00f9b2fb 100644 --- a/backend/druks/contrib/software_factory/templates/verification_block.md +++ b/backend/druks/contrib/software_factory/templates/verification_block.md @@ -7,7 +7,7 @@ {% endfor %} {% endif -%} {% if not has_commands -%} -No lint / typecheck / test / smoke commands are configured for this repo. Take verification conventions from the repo's AGENTS.md if it has one; otherwise the evaluator is acceptance-criteria-driven. Never invent commands (pytest, npm test, …) the project doesn't actually use. +No lint / typecheck / test / smoke commands are configured for this repo. Take verification conventions from the repo's AGENTS.md if it has one; otherwise verification is acceptance-criteria-driven. Never invent commands (pytest, npm test, …) the project doesn't actually use. {% else -%} {% for section in sections if section.command_entries -%} **{{ section.label }}:** diff --git a/backend/druks/contrib/software_factory/workflows.py b/backend/druks/contrib/software_factory/workflows.py index 2b0002f67..b42179de5 100644 --- a/backend/druks/contrib/software_factory/workflows.py +++ b/backend/druks/contrib/software_factory/workflows.py @@ -5,13 +5,12 @@ from pydantic import BaseModel, Field from druks.accounts import Account -from druks.contrib.software_factory.contracts import ImplementationOutput, ReviewWork -from druks.contrib.software_factory.enums import ( - EvaluationVerdict, - HumanFeedbackAction, - Resolution, - ReviewDecision, +from druks.contrib.software_factory.contracts import ( + ImplementationOutput, + ReviewReport, + ReviewWork, ) +from druks.contrib.software_factory.enums import HumanFeedbackAction, Resolution, ReviewDecision from druks.contrib.software_factory.models import ProjectRepo, WorkItem from druks.contrib.software_factory.ticketing.enums import TicketStatus from druks.core.services import Github @@ -26,7 +25,7 @@ from druks.workspaces import RepoWorkspace from .app import SoftwareFactory -from .constants import GITHUB_MCP_NAME, GITHUB_MCP_URL, TICKET_TOOLS +from .constants import GITHUB_MCP_NAME, GITHUB_MCP_URL, REVIEW_TTL_SECONDS, TICKET_TOOLS from .datastructures import PullRequest from .github import get_review_actor from .journal import BuildJournal @@ -67,12 +66,10 @@ async def get_secrets(cls, subject: Any) -> list[SandboxSecret]: @classmethod async def get_mcp_servers(cls, subject: Any) -> tuple[SandboxMcpServer, ...]: - # GitHub MCP acts as the review actor. The clone acts as the operator. - actor = await get_review_actor() github = SandboxMcpServer( name=GITHUB_MCP_NAME, url=GITHUB_MCP_URL, - secret_id=(await actor.service.get()).id, + secret_id=(await cls.github.get()).id, resources=await get_project_repos(cls.get_repo(subject)), ) if (await SoftwareFactory.settings()).tracker == "druks": @@ -139,11 +136,6 @@ class Settings(BaseModel): title="Max implementation revisions", description="Implement/review round-trips before parking for a human.", ) - review_code: bool = Field( - default=True, - title="Code review", - description="Include the advisory code-review lens in the implementation review.", - ) @classmethod async def dispatch(cls, *, ticket: dict) -> str | None: @@ -247,8 +239,6 @@ async def get_prompt_context(self, **context: Any) -> dict[str, Any]: skills=await Skill.list_delivered( db_session(), self._profile.get("recommended_skills", []) ), - review_code=self._settings.review_code, - review_mode=(await get_review_actor()).mode, journal=self.journal, ) return { @@ -321,14 +311,27 @@ async def _implement_phase(self) -> None: await self._create_branch() while True: await self.implement() - evaluation = await SoftwareFactory.evaluate_implementation() - if evaluation.verdict == EvaluationVerdict.FAIL and ( + review = await self._review_work() + if review.decision == "request_changes" and ( self.journal.implementation_revision < self._settings.max_implementation_revisions ): continue if await self._work_gate(): return + async def _review_work(self) -> ReviewReport: + """Hand the pull request to the standalone review and wait for its decision.""" + # The body starts the child run, so a replay gets back the review it started. + await PullRequestReview.dispatch( + repo=(await self.subject).repo, pr_number=self.pr_number, account=self.account + ) + facts = await self.wait( + "review.completed", label="Waiting for the review", ttl_seconds=REVIEW_TTL_SECONDS + ) + review = ReviewReport.model_validate(facts) + self.journal.add(review) + return review + async def _work_gate(self) -> bool: """Park for work approval. Return True when the work is done, or False when it goes back to implementation.""" @@ -551,8 +554,9 @@ async def dispatch(cls, *, repo: str, pr_number: int, account: Account, note: st subject=PullRequest(id=f"{repo}#{pr_number}"), account_id=account.id, note=note ) - async def run(self, note: str = "") -> None: - await SoftwareFactory.review_pull_request() + async def run(self, note: str = "") -> ReviewReport: + # The finished event carries this, so a build waiting on the review reads it there. + return await SoftwareFactory.review_pull_request() async def get_prompt_context(self, **context: Any) -> dict[str, Any]: project_repo = await ProjectRepo.get_for_repo( diff --git a/backend/tests/software_factory/test_build_journal.py b/backend/tests/software_factory/test_build_journal.py index 1b9aecaeb..e3ba5d8e9 100644 --- a/backend/tests/software_factory/test_build_journal.py +++ b/backend/tests/software_factory/test_build_journal.py @@ -1,11 +1,11 @@ from druks.contrib.software_factory.contracts import ( - EvaluationOutput, ImplementationOutput, PlanData, + ReviewReport, ReviewWork, TriageOutput, ) -from druks.contrib.software_factory.enums import EvaluationVerdict, HumanFeedbackAction +from druks.contrib.software_factory.enums import HumanFeedbackAction from druks.contrib.software_factory.journal import BuildJournal @@ -39,17 +39,6 @@ def _implementation( ) -def _evaluation() -> EvaluationOutput: - return EvaluationOutput( - verdict=EvaluationVerdict.FAIL, - body="", - review_notes="", - findings=[], - checks=[], - acceptance_results=[], - ) - - def test_plan_is_the_latest_or_an_empty_stand_in(): assert _journal().plan.plan_markdown == "" journal = _journal(PlanData(plan_markdown="v1"), PlanData(plan_markdown="v2")) @@ -85,7 +74,7 @@ def test_assignee_scan_falls_through_unresolved_revisions(): assert _journal(PlanData()).assignee_github_login is None -def test_revision_request_is_the_newest_triage_until_an_evaluation_supersedes_it(): +def test_revision_request_is_the_newest_triage_until_a_review_supersedes_it(): triage = TriageOutput( action=HumanFeedbackAction.CHANGE_REQUIRED, body="rename the flag", @@ -93,7 +82,10 @@ def test_revision_request_is_the_newest_triage_until_an_evaluation_supersedes_it implementation_instructions="rename X to Y", ) assert _journal(triage).revision_request == "rename X to Y" - assert _journal(triage, _evaluation()).revision_request is None + review = ReviewReport( + decision="request_changes", summary="Rename Y too.", findings=[], context_repos=[] + ) + assert _journal(triage, review).revision_request is None assert _journal().revision_request is None diff --git a/backend/tests/software_factory/test_build_plan_phase.py b/backend/tests/software_factory/test_build_plan_phase.py index 677f0dd6d..52dc6c056 100644 --- a/backend/tests/software_factory/test_build_plan_phase.py +++ b/backend/tests/software_factory/test_build_plan_phase.py @@ -9,11 +9,13 @@ QuestionOptionOutput, QuestionOutput, ReviewOutput, + ReviewReport, ReviewWork, ) from druks.contrib.software_factory.enums import ReviewDecision +from druks.contrib.software_factory.journal import BuildJournal from druks.contrib.software_factory.policy import PlanGate, RepoPolicy -from druks.contrib.software_factory.workflows import Build +from druks.contrib.software_factory.workflows import Build, PullRequestReview from druks.workflows import FatalError, OperatorReply, Run @@ -701,3 +703,77 @@ async def fake_work_gate(): assert await flow._approved_work() is True assert reparked == [True] + + +def _reviewing_flow(monkeypatch, facts: dict) -> tuple[Build, list[dict]]: + flow = Build() + flow.subject = SimpleNamespace(repo="acme/widget") + flow.account = SimpleNamespace(id="acct-1") + flow.journal = BuildJournal() + monkeypatch.setattr(Build, "pr_number", property(lambda self: 42)) + dispatched: list[dict] = [] + + async def dispatch(cls, **kwargs): + dispatched.append(kwargs) + + async def wait(topic, *, label, ttl_seconds): + assert topic == "review.completed" + return facts + + monkeypatch.setattr(PullRequestReview, "dispatch", classmethod(dispatch)) + monkeypatch.setattr(flow, "wait", wait) + return flow, dispatched + + +async def test_a_build_hands_its_pull_request_to_the_review(monkeypatch): + flow, dispatched = _reviewing_flow( + monkeypatch, + { + "decision": "request_changes", + "summary": "Rename the flag.", + "findings": [], + "context_repos": [], + }, + ) + + review = await flow._review_work() + + assert dispatched == [{"repo": "acme/widget", "pr_number": 42, "account": flow.account}] + assert review == ReviewReport( + decision="request_changes", summary="Rename the flag.", findings=[], context_repos=[] + ) + assert flow.journal.reviews == [review] + + +@pytest.mark.parametrize(("max_revisions", "implemented"), [(5, 2), (1, 1)]) +async def test_requested_changes_go_back_to_implement_until_the_cap( + monkeypatch, max_revisions, implemented +): + flow = Build() + flow._settings = Build.Settings(max_implementation_revisions=max_revisions) + flow.journal = SimpleNamespace(implementation_revision=0) + decisions = iter(["request_changes", "approve"]) + calls: list[str] = [] + + async def create_branch(): + return None + + async def implement(): + flow.journal.implementation_revision += 1 + calls.append("implement") + + async def review_work(): + return ReviewReport(decision=next(decisions), summary="", findings=[], context_repos=[]) + + async def work_gate(): + calls.append("work_gate") + return True + + monkeypatch.setattr(flow, "_create_branch", create_branch) + monkeypatch.setattr(flow, "implement", implement) + monkeypatch.setattr(flow, "_review_work", review_work) + monkeypatch.setattr(flow, "_work_gate", work_gate) + + await flow._implement_phase() + + assert calls == ["implement"] * implemented + ["work_gate"] diff --git a/backend/tests/software_factory/test_build_prompts.py b/backend/tests/software_factory/test_build_prompts.py index 3bb40a524..c24f7ed2a 100644 --- a/backend/tests/software_factory/test_build_prompts.py +++ b/backend/tests/software_factory/test_build_prompts.py @@ -4,6 +4,7 @@ import pytest from druks.contrib import software_factory +from druks.contrib.software_factory.contracts import FindingOutput, ReviewReport from druks.contrib.software_factory.journal import BuildJournal from druks.contrib.software_factory.models import Project, ProjectRepo from druks.contrib.software_factory.policy import RepoPolicy @@ -16,7 +17,6 @@ "review_plan.md", "revise_contract.md", "implement.md", - "evaluate_implementation.md", "triage_human_feedback.md", ] @@ -26,7 +26,7 @@ } -def _build(*, review_code: bool = True) -> SimpleNamespace: +def _build() -> SimpleNamespace: """A stand-in BuildPromptContext exposing the fields the templates read — identity facts faked, the journal real and empty.""" return SimpleNamespace( @@ -45,8 +45,6 @@ def _build(*, review_code: bool = True) -> SimpleNamespace: description="Apply the Python house rules.", ) ], - review_code=review_code, - review_mode="approve", journal=BuildJournal(), ) @@ -137,48 +135,36 @@ async def test_verification_profile_renders_ci_provenance_per_command(): ] -async def test_evaluation_prompt_renders_code_review_lens_only_when_enabled(): - headings = {} - for review_code in (True, False): - prompt = await render_prompt( - "software_factory/build/evaluate_implementation.md", - build=_build(review_code=review_code), - verification="VERIFICATION-BLOCK", - workspace=_workspace(), +async def test_implement_prompt_renders_the_prior_review(): + build = _build() + finding = FindingOutput( + severity="high", + summary="The flag keeps its old name.", + evidence="Rename it everywhere.", + path="app/flags.py", + line=12, + start_line=None, + ) + build.journal.add( + ReviewReport( + decision="request_changes", + summary="Rename the flag.", + findings=[finding], + context_repos=[], ) - headings[review_code] = {line for line in prompt.splitlines() if line.startswith("## ")} - - assert headings[False] < headings[True] - assert len(headings[True] - headings[False]) == 1 - - -async def test_approve_mode_posts_verdict_reviews(): - prompt = await render_prompt( - "software_factory/build/evaluate_implementation.md", - build=_build(), - verification="VERIFICATION-BLOCK", - workspace=_workspace(), ) - assert "an **approving** verdict → `APPROVE`" in prompt - assert "Submit every review as a `COMMENT` event" not in prompt - - -async def test_comment_mode_swaps_the_review_event_mapping(): - # The operator authored the PR, and GitHub refuses an author's APPROVE / - # REQUEST_CHANGES — the prompt swaps the event mapping for comment-mode. - build = _build() - build.review_mode = "comment" - prompt = await render_prompt( - "software_factory/build/evaluate_implementation.md", + "software_factory/build/implement.md", build=build, verification="VERIFICATION-BLOCK", workspace=_workspace(), ) - assert "Submit every review as a `COMMENT` event" in prompt - assert "an **approving** verdict → `APPROVE`" not in prompt + assert "## Prior implementation review" in prompt + assert "### Round 1 — decision: request_changes" in prompt + assert "Rename the flag." in prompt + assert "- **high** The flag keeps its old name. (`app/flags.py:12`)" in prompt def test_build_prompt_context_covers_template_attrs(): diff --git a/backend/tests/software_factory/test_build_workspace.py b/backend/tests/software_factory/test_build_workspace.py index 0b3ca2a64..3289ea691 100644 --- a/backend/tests/software_factory/test_build_workspace.py +++ b/backend/tests/software_factory/test_build_workspace.py @@ -69,11 +69,12 @@ async def base_run_agent(self: Any, **kwargs: Any) -> str: assert execs == [["mkdir", "-p", get_related_root("exedev")], ["run_agent"]] -async def test_build_workspace_declares_its_github_mcp_as_the_review_actor(druks_db): +async def test_build_workspace_declares_its_github_mcp_as_the_operator(druks_db): + # The review runs as the reviewer in its own workflow, so a build never acts as it. operator = await connect_service( "github", identity={"app_id": "1", "slug": "druks-operator"}, secrets={"private_key": "pem"} ) - reviewer = await connect_service( + await connect_service( "github_reviewer", identity={"app_id": "2", "slug": "druks-reviewer"}, secrets={"private_key": "reviewer-pem"}, @@ -84,7 +85,7 @@ async def test_build_workspace_declares_its_github_mcp_as_the_review_actor(druks assert github.url == GITHUB_MCP_URL assert github.bearer_token_env_var == get_bearer_token_env_var(GITHUB_MCP_NAME) - assert ref.key == ("mcp_github_token", reviewer.id, ("o/main",), "api.githubcopilot.com") + assert ref.key == ("mcp_github_token", operator.id, ("o/main",), "api.githubcopilot.com") [clone] = await BuildWorkspace.get_secrets(subject) assert clone == SandboxSecret( name="github", secret_id=operator.id, resources=("o/main",), host="github.com" diff --git a/backend/tests/software_factory/test_contracts.py b/backend/tests/software_factory/test_contracts.py index 29cea52fe..0c772579e 100644 --- a/backend/tests/software_factory/test_contracts.py +++ b/backend/tests/software_factory/test_contracts.py @@ -14,7 +14,6 @@ O.ReviewOutput, O.TriageOutput, O.ImplementationOutput, - O.EvaluationOutput, O.ContractRevisionOutput, ] @@ -207,29 +206,6 @@ def test_ask_contracts_cap_identity_and_cardinality(): ) -def test_evaluation_artifact_names_the_checks_that_did_not_decide(): - checks = [ - O.EvalCheckOutput(name="make lint", status="fail", evidence="make: command not found"), - O.EvalCheckOutput(name="tests", status="not_run", evidence="No database is available."), - O.EvalCheckOutput(name="CI", status="pass", evidence="CI passed for this commit."), - ] - blocked = O.EvaluationOutput( - verdict="blocked", - body="The required local check could not run.", - review_notes="", - findings=[], - checks=checks, - acceptance_results=[], - ) - content = blocked.to_artifact()["content"] - assert content.startswith("Verification blocked\n\n- make lint: make: command not found") - assert "- tests: No database is available." in content - assert "CI passed" not in content - assert content.endswith("The required local check could not run.") - passed = blocked.model_copy(update={"verdict": "pass"}) - assert passed.to_artifact()["content"].startswith("Verdict: pass\n\n") - - def test_review_output_records_no_artifact(): # An artifact would displace the plan as the parked ask's document. grade = O.ReviewOutput(decision=ReviewDecision.REQUEST_CHANGES, body="name the wire schema") diff --git a/backend/tests/software_factory/test_lane_reactions.py b/backend/tests/software_factory/test_lane_reactions.py index be16cb7ce..9a4b52ae3 100644 --- a/backend/tests/software_factory/test_lane_reactions.py +++ b/backend/tests/software_factory/test_lane_reactions.py @@ -5,10 +5,11 @@ import pytest from druks.accounts.models import Account from druks.contrib.software_factory.contracts import ReviewWork +from druks.contrib.software_factory.datastructures import PullRequest from druks.contrib.software_factory.enums import Resolution from druks.contrib.software_factory.models import WorkItem from druks.contrib.software_factory.ticketing.enums import TicketStatus -from druks.contrib.software_factory.workflows import Build +from druks.contrib.software_factory.workflows import Build, PullRequestReview from druks.durable import Run from druks.durable.enums import WorkflowEvent from druks.events.models import Event @@ -236,3 +237,64 @@ async def test_merge_before_park_answers_review(druks_db, monkeypatch): answer.assert_awaited_once_with(item, workflow=Build, action="merged") push.assert_not_awaited() + + +async def _answers_to_the_build(monkeypatch) -> list[tuple]: + answers: list[tuple] = [] + + async def answer(cls, subject, topic, **facts): + answers.append((subject.id, topic, facts)) + + monkeypatch.setattr(Build, "answer", classmethod(answer)) + return answers + + +async def _item_with_pr(ticket_key: str, pr_number: int) -> WorkItem: + item = await make_test_work_item( + repo="acme/widget", title="t", source="linear", ticket_key=ticket_key + ) + await item.update(pr_number=pr_number, branch=f"agent/{ticket_key.lower()}") + return item + + +async def test_a_finished_review_answers_the_build_of_its_pull_request(druks_db, monkeypatch): + item = await _item_with_pr("ACME-30", 42) + answers = await _answers_to_the_build(monkeypatch) + result = {"decision": "approve", "summary": "Looks right.", "findings": [], "context_repos": []} + + await publish( + WorkflowEvent.FINISHED, + subject=PullRequest(id="acme/widget#42").identity, + kind=PullRequestReview.kind, + result=result, + ) + + assert answers == [(item.id, "review.completed", result)] + + +@pytest.mark.parametrize("event", [WorkflowEvent.FAILED, WorkflowEvent.CANCELLED]) +async def test_a_stopped_review_fails_the_build_with_its_failure(druks_db, monkeypatch, event): + item = await _item_with_pr("ACME-31", 43) + answers = await _answers_to_the_build(monkeypatch) + + await publish( + event, + subject=PullRequest(id="acme/widget#43").identity, + kind=PullRequestReview.kind, + failure="druks-review has no access", + ) + + assert answers == [(item.id, "review.completed", {"failure": "druks-review has no access"})] + + +async def test_a_review_of_a_pull_request_no_build_owns_answers_no_one(druks_db, monkeypatch): + answers = await _answers_to_the_build(monkeypatch) + + await publish( + WorkflowEvent.FINISHED, + subject=PullRequest(id="acme/widget#99").identity, + kind=PullRequestReview.kind, + result={"decision": "approve", "summary": "", "findings": [], "context_repos": []}, + ) + + assert answers == [] diff --git a/backend/tests/test_api_settings.py b/backend/tests/test_api_settings.py index bc64310da..f8212a9d6 100644 --- a/backend/tests/test_api_settings.py +++ b/backend/tests/test_api_settings.py @@ -252,7 +252,7 @@ def test_apps_surface_build_agents_and_workflow_defaults(tmp_path: Path): "timeoutSource": "default", } assert agents["software_factory.implement"]["model"] == "anthropic/claude-opus-4-7" - assert agents["software_factory.evaluate_implementation"]["effortSource"] == "default" + assert agents["software_factory.triage_human_feedback"]["effortSource"] == "default" fields = {field["name"]: field for field in software_factory["workflows"][0]["fields"]} assert fields["max_implementation_revisions"]["value"] == 5 assert fields["plan_gate"] == { @@ -602,24 +602,24 @@ def test_apps_reject_non_positive_timeout(tmp_path: Path): assert response.status_code == 422 -def test_build_review_code_is_a_workflow_setting(tmp_path: Path): +def test_build_revision_cap_is_a_workflow_setting(tmp_path: Path): with settings_client(tmp_path) as client: workflow = _software_factory_app(client)["workflows"][0] fields = {field["name"]: field for field in workflow["fields"]} - assert fields["review_code"]["value"] - assert not fields["review_code"]["overridden"] + assert fields["max_implementation_revisions"]["value"] == 5 + assert not fields["max_implementation_revisions"]["overridden"] patch = client.patch( "/api/settings/apps", - json={"workflowSettings": {workflow["kind"]: {"review_code": False}}}, + json={"workflowSettings": {workflow["kind"]: {"max_implementation_revisions": 3}}}, ) assert patch.status_code == 200 fields = { field["name"]: field for field in _software_factory_app(client)["workflows"][0]["fields"] } - assert not fields["review_code"]["value"] - assert fields["review_code"]["overridden"] + assert fields["max_implementation_revisions"]["value"] == 3 + assert fields["max_implementation_revisions"]["overridden"] def test_apps_clearing_an_override_reverts_to_the_operator_default(tmp_path: Path): diff --git a/docs/configuration.md b/docs/configuration.md index 70fb7d4e1..a413f77d6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -488,9 +488,9 @@ sandbox's repo, with the expiry GitHub gives it, and fetches a new one before it expires. The token reaches that repo and the project's other repos that the same installation covers, and no other repo. The `software_factory` build clones and pushes as the operator App. A review clones as the reviewer App when -one is connected. Builds and reviews -also get the GitHub MCP server for `api.githubcopilot.com`. It acts as the -review identity through a second entry. +one is connected. Builds and reviews also get the GitHub MCP server for +`api.githubcopilot.com`. The build's server acts as the operator App. The +review's acts as the review identity. **To upgrade an existing installation**, paste the credentials one time on each active host. Open **Settings → Connections → Services**. Connect GitHub with the @@ -531,8 +531,9 @@ Connect a second App to let reviews approve pull requests and request changes. Open **Settings → Connections → Services → Github Reviewer** and select **Create App**. The flow is the same as for the operator App. Install the -reviewer App on every repository that has the operator App, because builds also -post their reviews through it. +reviewer App on every repository that has the operator App, because a build +hands its pull request to a review that runs as the reviewer App. When that +review fails, the build fails with the review's reason. To register the reviewer App manually: diff --git a/frontend/src/apps/software_factory/WorkItemPage.tsx b/frontend/src/apps/software_factory/WorkItemPage.tsx index 9a8be71b6..3752fa083 100644 --- a/frontend/src/apps/software_factory/WorkItemPage.tsx +++ b/frontend/src/apps/software_factory/WorkItemPage.tsx @@ -611,7 +611,7 @@ function RunInspector({ const external = run.state === 'parked' && run.inputRequest?.presentation === 'external' const requestedReview = expected && !external const isNewestCall = call == null || call.id === run.agentCalls.at(-1)?.id - // A finished call's saved result (an evaluation's verdict, a plan) is what the + // A finished call's saved result (a plan, a delivery) is what the // operator came for; the transcript is how it got there. const files = useQuery({ queryKey: ['agent-call-files', call?.id],