From 948bd044a15c66678922f3e29632158b5549a0a9 Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Wed, 7 Oct 2026 14:29:37 +0000 Subject: [PATCH 1/6] Review for essential changes and single source of truth - simplicity: also flags changes the task does not need - new single-source reviewer: one home per fact, missed homes - code-structure: duplication moves to single-source - code-functionality: checks other call paths to changed behaviour - fixer changes only what a finding needs --- .orca/reviewers/orca.md | 6 +-- adr/0011-reviewer-roster.md | 15 ++++++ docs/using/reviewers.md | 6 +-- .../main/resources/orca/review/prompts/fix.md | 3 +- .../orca/review/prompts/initial-review.md | 4 +- .../prompts/reviewers/code-functionality.md | 3 ++ .../prompts/reviewers/code-structure.md | 14 ++---- .../review/prompts/reviewers/simplicity.md | 32 +++++++++---- .../review/prompts/reviewers/single-source.md | 47 +++++++++++++++++++ .../main/scala/orca/review/Reviewers.scala | 2 + 10 files changed, 106 insertions(+), 26 deletions(-) create mode 100644 flow/src/main/resources/orca/review/prompts/reviewers/single-source.md diff --git a/.orca/reviewers/orca.md b/.orca/reviewers/orca.md index e606d4a8d..9c623f658 100644 --- a/.orca/reviewers/orca.md +++ b/.orca/reviewers/orca.md @@ -42,9 +42,9 @@ shipped reviewers running beside you. exhaustively downstream. - **Modelling**: three or more same-typed adjacent parameters — or two whose swap compiles — take named arguments or a case class; a wire field's absence - is decided once, at decode, never re-defaulted per call site; one decision, - one home, so display and summary code consumes the production resolver - instead of mirroring the rule. + is decided once, at decode, never re-defaulted per call site. +- **Deliberate duplication**: `README.md` carries its own API reference for the + public surface, restating scaladoc on purpose — never report it as a copy. - **Invisible in a diff**: terminal escapes are written as explicit unicode escapes (`\u001b`), never raw bytes; code that generates code — prompts, skeletons, templates — is tested by compiling or running the artifact, not by diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index 29e8f3895..8f5987671 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -242,3 +242,18 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > ADR 0023 also moves a reviewer's description and `files:` pattern off > `ReviewerPrompts`' by-name maps onto the reviewer itself, so a discovered > reviewer cannot reach the picker as a bare name — see it for both. + +> **Amendment (2026-10-07).** The roster is nine reviewers: +> - **single-source** is new. It owns duplicated knowledge — whether each fact +> the change touches (a rule, default, mapping, decision) has one home, in +> code, prompts or docs — and the homes a change failed to update. It is the +> one reviewer that searches the repository beyond the diff, limited to the +> facts the diff touches. Duplication moves to it from **code-structure**, +> which keeps abstraction quality, including the warning against premature +> abstraction. +> - **simplicity** also owns whether each change is needed for the task at all: +> it traces every hunk to the task and flags drive-by refactors, renames, +> reformatting and unrelated fixes. Its slug is unchanged, so a project file +> named `simplicity.md` still shadows it. +> - **code-functionality** checks that a behaviour change reaches every call +> path to that behaviour, not only the one the diff edits. diff --git a/docs/using/reviewers.md b/docs/using/reviewers.md index 55d70725a..ce77b3a35 100644 --- a/docs/using/reviewers.md +++ b/docs/using/reviewers.md @@ -1,8 +1,8 @@ # Custom reviewers A reviewer is a prompt that says what to look for, paired with a read-only -agent. Orca ships eight of them: code-functionality, test, readability, -code-structure, simplicity, performance, security and scala-fp. You can add +agent. Orca ships nine of them: code-functionality, test, readability, +code-structure, single-source, simplicity, performance, security and scala-fp. You can add your own, or retune a shipped one, by writing a Markdown file; no code changes are needed. @@ -16,7 +16,7 @@ the repository: |---|---|---| | project | `.orca/reviewers/*.md` | committed with the repository | | global | `~/.config/orca/reviewers/*.md` (`$XDG_CONFIG_HOME/orca/reviewers/`) | your own, in every project | -| built-in | shipped with Orca | the eight above | +| built-in | shipped with Orca | the nine above | The tiers merge into one **catalog**. A reviewer's name is its filename stem, compared case-insensitively, so `.orca/reviewers/orca.md` is the reviewer diff --git a/flow/src/main/resources/orca/review/prompts/fix.md b/flow/src/main/resources/orca/review/prompts/fix.md index 6b8329ad6..3cfd31079 100644 --- a/flow/src/main/resources/orca/review/prompts/fix.md +++ b/flow/src/main/resources/orca/review/prompts/fix.md @@ -2,4 +2,5 @@ For each finding below: fix it directly in the codebase if you can. Otherwise when the finding is environmental, out of scope, or a false positive — decline it with a brief reason. -Prefer minimal, scoped fixes. +Change only what the finding needs. When a finding names several locations, fix +all of them. diff --git a/flow/src/main/resources/orca/review/prompts/initial-review.md b/flow/src/main/resources/orca/review/prompts/initial-review.md index 6b62b68c9..d536084e9 100644 --- a/flow/src/main/resources/orca/review/prompts/initial-review.md +++ b/flow/src/main/resources/orca/review/prompts/initial-review.md @@ -3,7 +3,9 @@ Task: {{taskTitle}}{{taskContext}} Review the change below — do NOT survey unrelated files in the project. Start from what the diff modifies, and follow it into the code that has to keep working now that it has changed: unchanged code is in scope precisely when the -change alters what it can be handed. +change alters what it can be handed. Your scope may name other code to read — +such as other places that encode a fact the change touches; reading that is +following the change, not surveying. {{diffIntro}} diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md index 665e48bfc..c963c403b 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md @@ -12,6 +12,9 @@ performance, tests, structure) belong to other reviewers. - **Intent vs. behaviour**: trace the code; does it produce the documented/intended result for typical inputs? +- **Other call paths**: when the change alters behaviour reached from one + entry point, find the other callers and entry points to the same behaviour + and check they get it too. - **Edge cases**: empty collections, zero, negative, max/min, boundary indices, unicode, missing/null, malformed input. Pick the ones that apply. - **Failure modes**: every external call, parse, or shell-out has a sad path — diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md index 0a7f5c366..058e9c04c 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md @@ -1,24 +1,20 @@ --- name: code-structure-reviewer -description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, dependency direction, abstraction quality, and duplication. Flags catch-all files, leaky internals, over-exposed APIs, premature abstractions, missed extractions, cycles, and stable code that depends on volatile concretions. +description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, dependency direction, and abstraction quality. Flags catch-all files, leaky internals, over-exposed APIs, premature abstractions, cycles, and stable code that depends on volatile concretions. --- ## Scope Structure only — how the pieces fit together. Language- and framework- -agnostic. Other dimensions (correctness, naming, performance, tests) belong -to other reviewers. +agnostic. Other dimensions (correctness, naming, performance, tests, +duplicated knowledge) belong to other reviewers. ## Aspects -- **Duplication**: semantic duplication (same logic, different syntax) - repeated 2+ times. Suggest a name and a home for the extracted unit. Three - similar lines is better than a premature abstraction — flag only when the - duplication is load-bearing or likely to drift. - - **Abstraction quality**: extractions that genuinely simplify vs. premature ones that just add indirection. Each extracted unit needs a single - coherent responsibility. Don't propose abstractions for one-off code. + coherent responsibility. Don't propose abstractions for one-off code — + three similar lines is better than a premature abstraction. - **Cohesion**: a module or package should hold types and functions that change for the same reason and are typically used together. Things that diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md index d2f0acc9d..7de602fdf 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md @@ -1,24 +1,38 @@ --- name: simplicity-reviewer -description: Reviews whether the change is more complex than the problem requires — speculative generality, gold-plating, options or indirection nothing uses, handling of cases that can't occur, and convoluted logic that could be markedly simpler or removed. The question the other reviewers don't ask: could this do the same job with less? +description: Reviews whether every change is needed for the task and no bigger than it must be — edits the task does not need (drive-by refactors, renames, reformatting, unrelated fixes, new surface or config), speculative generality, gold-plating, options or indirection nothing uses, handling of cases that can't occur, and convoluted logic that could be markedly simpler or removed. --- ## Scope -One question only: is this more complex than the problem in front of it -requires? Judge against what the change actually needs to do, not a hypothetical -future. Correctness, naming, performance, and structural layout belong to other -reviewers — flag complexity, not bugs or style. +Two questions: does the task need this change at all, and if so, could it do the +same job with less? Judge against what the user asked for and what this task must +deliver, not a hypothetical future. Correctness, naming, performance, and +structural layout belong to other reviewers — flag what can go, not bugs or style. ## Aspects +- **Not needed for the task**: for each hunk, name the part of the task it + serves. Flag hunks that serve none — refactors, renames, reordering or + reformatting of code the task did not have to touch, comment or doc edits + unrelated to the change, fixes of unrelated defects, dependency or config + changes. Suggest reverting them; an unrelated defect worth fixing is a + separate task, not part of this change. + +- **What counts as needed**: what the task's behaviour requires, and what keeps + the repository consistent with it — updated callers, tests for the new + behaviour, docs and other homes of a fact the change altered, merging the + copies of such a fact into one home, formatter output, and edits that resolve + review findings. Work that belongs to another task of the same plan is not + this task's to judge. Do not flag any of these. + - **Speculative generality**: abstractions, type parameters, traits, or config knobs with a single current use. Generality earns its place at the second real caller, not the first imagined one — until then the concrete form wins. -- **Gold-plating**: behaviour beyond what was asked — extra options, modes, or - configurability nothing exercises; solving a more general problem than the one - posed. Flag the unused surface. +- **Gold-plating**: behaviour beyond what was asked — extra options, modes, + public members, or configurability nothing exercises; solving a more general + problem than the one posed. Flag the unused surface. - **Impossible cases**: branches, guards, or fallbacks for inputs the types or callers already rule out. The counterpart to the correctness reviewer's @@ -36,4 +50,4 @@ reviewers — flag complexity, not bugs or style. The strongest simplification is often deletion: when code, a parameter, or a whole abstraction can go without losing required behaviour, say so. Don't mistake terseness for simplicity — clarity still wins. Cap at the 3–5 most -valuable simplifications when the change is large. +valuable findings when the change is large. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md new file mode 100644 index 000000000..9ab06fe0f --- /dev/null +++ b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md @@ -0,0 +1,47 @@ +--- +name: single-source-reviewer +description: Reviews whether each fact the change touches — a rule, default, constant, mapping, format, invariant, or decision — has one authoritative home. Searches the repository for other code, prompts, and docs encoding the same fact, and flags new copies, copies the change failed to update, and consumers that re-derive what an owner already decides. +--- + +## Scope + +Where knowledge lives, not how code is shaped. A fact is something that would +have to change in every place it appears if it changed once. Code that merely +looks alike but would change for different reasons is not duplication — leave +it. + +Only facts the change adds, alters, or relies on are in scope; spread that +exists independently of this change is not. Merging the copies of an in-scope +fact into one home is part of the task. Historical records — plans, research +notes, dated or superseded ADR text — are not homes; never ask to update them. + +Abstraction quality, layout, and naming belong to other reviewers, and duplicate +tests to the test reviewer. + +## Method + +List the facts the change adds or alters. For each, search the repository for +its other encodings — identifiers, literals, enum cases, matches over the same +type, phrases in prompts and docs — including code the diff never touches. This +search is your scope: do not review the files it finds for anything else. + +## Aspects + +- **New copy**: the change restates a fact that already has an owner — a second + match over the same cases, a re-declared constant or default, a hand-rolled + version of an existing helper, a rule re-worded in a second doc. Name the + owner and how the copy should use it. +- **Missed home**: the change altered a fact in one place while another still + encodes the old version — a parallel match, another entry point, a test + fixture, a docs table, a prompt, a current ADR. Name each one. +- **Spread decision**: the change adds a site to a decision split across places + that must agree by convention — parallel lists or matches a new case has to be + added to in step. Suggest the single place that should produce the rest. +- **Re-derived values**: a consumer re-parses, re-defaults, or re-computes what + a producer already decided, instead of taking its result. +- **Prose duplication**: the same explanation in several comments, docs, or + prompts. Keep it in the home closest to what it describes; the others point to + it or say nothing. + +Report each finding with every location of the fact, and say which one should +stay. diff --git a/flow/src/main/scala/orca/review/Reviewers.scala b/flow/src/main/scala/orca/review/Reviewers.scala index 54cd1e3a6..d621394f5 100644 --- a/flow/src/main/scala/orca/review/Reviewers.scala +++ b/flow/src/main/scala/orca/review/Reviewers.scala @@ -240,6 +240,7 @@ object ReviewerPrompts: val ScalaFp: Reviewer = load("scala-fp") val Security: Reviewer = load("security") val Simplicity: Reviewer = load("simplicity") + val SingleSource: Reviewer = load("single-source") val Test: Reviewer = load("test") /** Every reviewer the library ships with. Order matches how `allReviewers` @@ -250,6 +251,7 @@ object ReviewerPrompts: Test, Readability, CodeStructure, + SingleSource, Simplicity, Performance, Security, From c9a321e94ad326d366e2f91c3c765a9cb52f3364 Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Wed, 7 Oct 2026 15:01:29 +0000 Subject: [PATCH 2/6] code-functionality checks the task is delivered; single-source flags contradictions --- adr/0011-reviewer-roster.md | 5 +++-- .../orca/review/prompts/reviewers/code-functionality.md | 4 ++++ .../resources/orca/review/prompts/reviewers/single-source.md | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index 8f5987671..6dd6b5cd0 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -255,5 +255,6 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > it traces every hunk to the task and flags drive-by refactors, renames, > reformatting and unrelated fixes. Its slug is unchanged, so a project file > named `simplicity.md` still shadows it. -> - **code-functionality** checks that a behaviour change reaches every call -> path to that behaviour, not only the one the diff edits. +> - **code-functionality** checks that the change delivers everything its task +> asks, and that a behaviour change reaches every call path to that +> behaviour, not only the one the diff edits. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md index c963c403b..6e9069d29 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md @@ -12,6 +12,10 @@ performance, tests, structure) belong to other reviewers. - **Intent vs. behaviour**: trace the code; does it produce the documented/intended result for typical inputs? +- **Delivers the task**: each part of what this change must deliver is + delivered — the task's description, or the user's request when the change + under review is the whole planned change. Parts that belong to other tasks + are not this task's to judge. - **Other call paths**: when the change alters behaviour reached from one entry point, find the other callers and entry points to the same behaviour and check they get it too. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md index 9ab06fe0f..700480b1c 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md @@ -39,6 +39,8 @@ search is your scope: do not review the files it finds for anything else. added to in step. Suggest the single place that should produce the rest. - **Re-derived values**: a consumer re-parses, re-defaults, or re-computes what a producer already decided, instead of taking its result. +- **Contradiction**: the change leaves two related rules a reader cannot both + follow — in code, prompts, or docs. Say which one should win. - **Prose duplication**: the same explanation in several comments, docs, or prompts. Keep it in the home closest to what it describes; the others point to it or say nothing. From 2c72b6901454ee60c236b7b69c3e281f2f571b33 Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Thu, 8 Oct 2026 10:58:41 +0000 Subject: [PATCH 3/6] Remove overlaps between reviewer prompts Each concern now has one reviewer: concurrency stays with performance, boolean parameters and long functions with readability, re-derived values with single-source, other entry points with code-functionality. Drops repeated text within prompts and the stale README exception in orca.md. --- .orca/reviewers/orca.md | 11 +++----- adr/0011-reviewer-roster.md | 8 +++--- .../orca/review/prompts/initial-review.md | 3 ++- .../prompts/reviewers/code-functionality.md | 18 +++++-------- .../prompts/reviewers/code-structure.md | 8 +++--- .../review/prompts/reviewers/performance.md | 2 +- .../review/prompts/reviewers/readability.md | 5 ++-- .../orca/review/prompts/reviewers/scala-fp.md | 9 ++----- .../review/prompts/reviewers/simplicity.md | 16 ++++++------ .../review/prompts/reviewers/single-source.md | 25 ++++++++----------- .../orca/review/prompts/reviewers/test.md | 7 +++--- 11 files changed, 44 insertions(+), 68 deletions(-) diff --git a/.orca/reviewers/orca.md b/.orca/reviewers/orca.md index 9c623f658..23397fbc1 100644 --- a/.orca/reviewers/orca.md +++ b/.orca/reviewers/orca.md @@ -25,8 +25,7 @@ shipped reviewers running beside you. with a reason it wrote; `OpenFinding(s)` is what the run leaves unresolved, each with an `OpenReason`. `issue` means a GitHub issue. Don't name the open set after one of its reasons, and don't rename inside a dated record. -- **Comments are present-tense facts**: no history ("no longer", "renamed - from"), no plan or epic labels, no teaching Scala mechanics; in `flows/*.sc`, +- **Comments are present-tense facts**: no plan or epic labels, no teaching Scala mechanics; in `flows/*.sc`, only facts about that file. - **Capability discipline**: `InStage.unsafe`/`WorkspaceWrite.unsafe` only in `RuntimeInStage` and tests; never drop a `(using InStage)` or `(using @@ -36,15 +35,11 @@ shipped reviewers running beside you. over `os.write.over`. - **Subprocesses capture stderr** — `QuietProc.call` or a `CliRunner`. - **Enums, not flags**: a domain mode is an enum, never a `Boolean` or a raw - string compared to literals; two flags or `Option`s whose combinations - include impossible states are one ADT. Protocol strings are parsed into an + string compared to literals. Protocol strings are parsed into an enum at the boundary (`Unknown(raw)` for unrecognised values) and matched exhaustively downstream. - **Modelling**: three or more same-typed adjacent parameters — or two whose - swap compiles — take named arguments or a case class; a wire field's absence - is decided once, at decode, never re-defaulted per call site. -- **Deliberate duplication**: `README.md` carries its own API reference for the - public surface, restating scaladoc on purpose — never report it as a copy. + swap compiles — take named arguments or a case class. - **Invisible in a diff**: terminal escapes are written as explicit unicode escapes (`\u001b`), never raw bytes; code that generates code — prompts, skeletons, templates — is tested by compiling or running the artifact, not by diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index 6dd6b5cd0..4bcf812d0 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -248,9 +248,8 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > the change touches (a rule, default, mapping, decision) has one home, in > code, prompts or docs — and the homes a change failed to update. It is the > one reviewer that searches the repository beyond the diff, limited to the -> facts the diff touches. Duplication moves to it from **code-structure**, -> which keeps abstraction quality, including the warning against premature -> abstraction. +> facts the diff touches. Duplication moves to it from **code-structure**; +> premature abstraction is left to **simplicity**. > - **simplicity** also owns whether each change is needed for the task at all: > it traces every hunk to the task and flags drive-by refactors, renames, > reformatting and unrelated fixes. Its slug is unchanged, so a project file @@ -258,3 +257,6 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > - **code-functionality** checks that the change delivers everything its task > asks, and that a behaviour change reaches every call path to that > behaviour, not only the one the diff edits. +> It no longer checks concurrency, which stays with **performance**. +> - **readability** alone owns unclear boolean parameters and long functions; +> **scala-fp** no longer repeats them. diff --git a/flow/src/main/resources/orca/review/prompts/initial-review.md b/flow/src/main/resources/orca/review/prompts/initial-review.md index d536084e9..adc8c28ff 100644 --- a/flow/src/main/resources/orca/review/prompts/initial-review.md +++ b/flow/src/main/resources/orca/review/prompts/initial-review.md @@ -43,7 +43,8 @@ looks wrong, report it as a finding against that choice — say which part of th task you mean. What the user asked for is what the work has to satisfy. Where that and what was -planned differ, what the user asked for wins. +planned differ, what the user asked for wins. Work that belongs to another task +of the plan is not this review's to judge. ## Always report these diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md index 6e9069d29..ae3f687f6 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md @@ -1,21 +1,18 @@ --- name: code-functionality-reviewer -description: Verifies code correctly implements its intent, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, and broken concurrency invariants. +description: Verifies code correctly implements its intent and the whole task, reaches every call path to the behaviour it changes, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, and entry points left on the old behaviour. --- ## Scope Correctness only — what the code does and how it fails. Other dimensions (style, -performance, tests, structure) belong to other reviewers. +performance and concurrency, tests, structure) belong to other reviewers. ## Aspects - **Intent vs. behaviour**: trace the code; does it produce the - documented/intended result for typical inputs? -- **Delivers the task**: each part of what this change must deliver is - delivered — the task's description, or the user's request when the change - under review is the whole planned change. Parts that belong to other tasks - are not this task's to judge. + documented/intended result for typical inputs, and deliver every part the + task asks for? - **Other call paths**: when the change alters behaviour reached from one entry point, find the other callers and entry points to the same behaviour and check they get it too. @@ -23,8 +20,5 @@ performance, tests, structure) belong to other reviewers. unicode, missing/null, malformed input. Pick the ones that apply. - **Failure modes**: every external call, parse, or shell-out has a sad path — is it caught at the right boundary, logged with enough context, surfaced to - the caller, or deliberately ignored with a reason? -- **Error swallowing**: a `try/catch` that drops the exception or returns a - default silently is almost always wrong. Flag it. -- **Concurrent access**: if shared state crosses threads, are the invariants - still safe? + the caller, or deliberately ignored with a reason? Silently dropping an + exception or returning a default is almost always wrong. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md index 058e9c04c..80242b027 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md @@ -1,6 +1,6 @@ --- name: code-structure-reviewer -description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, dependency direction, and abstraction quality. Flags catch-all files, leaky internals, over-exposed APIs, premature abstractions, cycles, and stable code that depends on volatile concretions. +description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, dependency direction, and abstraction quality. Flags catch-all files, leaky internals, over-exposed APIs, cycles, and stable code that depends on volatile concretions. --- ## Scope @@ -11,10 +11,8 @@ duplicated knowledge) belong to other reviewers. ## Aspects -- **Abstraction quality**: extractions that genuinely simplify vs. premature - ones that just add indirection. Each extracted unit needs a single - coherent responsibility. Don't propose abstractions for one-off code — - three similar lines is better than a premature abstraction. +- **Abstraction quality**: each extracted unit has a single coherent + responsibility. - **Cohesion**: a module or package should hold types and functions that change for the same reason and are typically used together. Things that diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md index 79722a289..15617e840 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md @@ -5,7 +5,7 @@ description: Reviews CPU/memory efficiency, algorithmic complexity, I/O and netw ## Scope -Performance and concurrent safety only. Other dimensions (correctness, style, +Performance and concurrency only. Other dimensions (correctness, style, tests) belong to other reviewers. If the change has no performance implications (startup, one-shot, trivially-small data), report no findings. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/readability.md b/flow/src/main/resources/orca/review/prompts/reviewers/readability.md index 3bf8a0588..4e179ee3b 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/readability.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/readability.md @@ -16,9 +16,8 @@ reviewers. Don't chase formatting the project's formatter handles. meaning isn't obvious at the call site. - **Comments**: explain *why*, not *what*, in as few words as the fact needs. Flag comments that restate the code, narrate change history ("used to be X", - "no longer does Y"), assert something unverified, or repeat a fact already - stated elsewhere in the change. Flag absent comments where non-obvious - reasoning is needed. + "no longer does Y"), or assert something unverified. Flag absent comments + where non-obvious reasoning is needed. - **Control flow**: deep nesting, long methods, dense conditionals. Suggest early returns, named helpers, or pattern matching when they'd clarify. - **Magic values**: unexplained literals/strings/numbers in the middle of logic. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md index dcf09b6da..42deea449 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md @@ -1,6 +1,6 @@ --- name: scala-fp-reviewer -description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, no boolean blindness, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives. +description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, explicit dependencies, braceless syntax, Ox concurrency primitives. files: \.scala$ --- @@ -19,10 +19,6 @@ Review only the FP idioms below; other dimensions belong to other reviewers. - **Pure functions**: parameters in, value out, no hidden `Clock.now`/`UUID.randomUUID`/`Random` — inject those. Use pattern matching/ADTs for control flow, not if/else cascades. -- **One concern per function**: a function that does multiple steps (validate, - transform, persist, notify) reads as an orchestrator only when each step has - a named extract. Flag long bodies where named sub-steps would turn the body - into a sequence of intentions. - **Immutable data**: `case class` / `enum` / sealed traits, immutable collections only. Different states of an entity → different types, not `Option` fields (`PendingOrder` / `ConfirmedOrder`, not `Order` with an @@ -31,8 +27,7 @@ Review only the FP idioms below; other dimensions belong to other reviewers. `String`/`Int`/`Long`/`Boolean` domain values (`OrderId`, `Port`). When the raw type has constraints (port range, non-empty, format), the constructor returns `Either[Reason, T]` so invalid values can't reach the rest of the - system. No boolean blindness — two-case enums for parameters whose - `true`/`false` isn't self-evident at the call site. + system. - **Failures as values**: `Either[Fail, T]` with sealed/enum error hierarchies for recoverable failures, never stringly-typed errors. In direct-style bodies, prefer an `either { ... .ok() ... }` block over manual diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md index 7de602fdf..92a251507 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md @@ -1,6 +1,6 @@ --- name: simplicity-reviewer -description: Reviews whether every change is needed for the task and no bigger than it must be — edits the task does not need (drive-by refactors, renames, reformatting, unrelated fixes, new surface or config), speculative generality, gold-plating, options or indirection nothing uses, handling of cases that can't occur, and convoluted logic that could be markedly simpler or removed. +description: Reviews whether every change is needed for the task and no bigger than it must be — drive-by refactors, renames, reformatting and unrelated fixes, speculative generality, gold-plating, options or indirection nothing uses, handling of cases that can't occur, and convoluted logic that could be markedly simpler or removed. --- ## Scope @@ -23,15 +23,14 @@ structural layout belong to other reviewers — flag what can go, not bugs or st the repository consistent with it — updated callers, tests for the new behaviour, docs and other homes of a fact the change altered, merging the copies of such a fact into one home, formatter output, and edits that resolve - review findings. Work that belongs to another task of the same plan is not - this task's to judge. Do not flag any of these. + review findings. Do not flag any of these. -- **Speculative generality**: abstractions, type parameters, traits, or config - knobs with a single current use. Generality earns its place at the second real +- **Speculative generality**: abstractions, type parameters, or traits with a + single current use. Generality earns its place at the second real caller, not the first imagined one — until then the concrete form wins. - **Gold-plating**: behaviour beyond what was asked — extra options, modes, - public members, or configurability nothing exercises; solving a more general + or configurability nothing exercises; solving a more general problem than the one posed. Flag the unused surface. - **Impossible cases**: branches, guards, or fallbacks for inputs the types or @@ -43,11 +42,10 @@ structural layout belong to other reviewers — flag what can go, not bugs or st forwards, a parameter that's always the same value, state threaded through that nothing reads. -- **Convoluted logic**: a body that could be markedly shorter or flatter — +- **Convoluted logic**: a body that could be markedly shorter — several steps one expression covers, a hand-rolled loop a library call replaces. Suggest the simpler form concretely. The strongest simplification is often deletion: when code, a parameter, or a whole abstraction can go without losing required behaviour, say so. Don't -mistake terseness for simplicity — clarity still wins. Cap at the 3–5 most -valuable findings when the change is large. +mistake terseness for simplicity — clarity still wins. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md index 700480b1c..70d1b77e9 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md @@ -10,37 +10,32 @@ have to change in every place it appears if it changed once. Code that merely looks alike but would change for different reasons is not duplication — leave it. -Only facts the change adds, alters, or relies on are in scope; spread that -exists independently of this change is not. Merging the copies of an in-scope -fact into one home is part of the task. Historical records — plans, research +Only facts the change adds or alters are in scope. Historical records — plans, research notes, dated or superseded ADR text — are not homes; never ask to update them. -Abstraction quality, layout, and naming belong to other reviewers, and duplicate -tests to the test reviewer. +Duplicate tests belong to the test reviewer. ## Method List the facts the change adds or alters. For each, search the repository for its other encodings — identifiers, literals, enum cases, matches over the same -type, phrases in prompts and docs — including code the diff never touches. This -search is your scope: do not review the files it finds for anything else. +type, phrases in prompts and docs — including code the diff never touches. Do not +review the files it finds for anything else. ## Aspects - **New copy**: the change restates a fact that already has an owner — a second - match over the same cases, a re-declared constant or default, a hand-rolled - version of an existing helper, a rule re-worded in a second doc. Name the - owner and how the copy should use it. + match over the same cases, a re-declared constant or default, a value + re-parsed or re-computed instead of taken from its producer, a hand-rolled + version of an existing helper. Name the owner and how the copy should use it. - **Missed home**: the change altered a fact in one place while another still - encodes the old version — a parallel match, another entry point, a test - fixture, a docs table, a prompt, a current ADR. Name each one. + encodes the old version — a parallel match, a test fixture, a docs + table, a prompt, a current ADR. - **Spread decision**: the change adds a site to a decision split across places that must agree by convention — parallel lists or matches a new case has to be added to in step. Suggest the single place that should produce the rest. -- **Re-derived values**: a consumer re-parses, re-defaults, or re-computes what - a producer already decided, instead of taking its result. - **Contradiction**: the change leaves two related rules a reader cannot both - follow — in code, prompts, or docs. Say which one should win. + follow — in code, prompts, or docs. - **Prose duplication**: the same explanation in several comments, docs, or prompts. Keep it in the home closest to what it describes; the others point to it or say nothing. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/test.md b/flow/src/main/resources/orca/review/prompts/reviewers/test.md index 1e57c6f51..ba2b2eb1a 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/test.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/test.md @@ -11,10 +11,9 @@ bug. ## Aspects -- **Minimality**: every test must justify its existence by covering a distinct - property no other test covers. Flag redundant tests for removal. -- **No duplication**: two tests exercising the same path with different literals - are duplicates. Pick one. +- **Minimality**: every test covers a property no other test covers; two tests + exercising the same path with different literals are duplicates. Flag + redundant tests for removal. - **Single property per test**: one behaviour per test. Multi-property tests get split; tightly-coupled facets of one scenario are fine. - **Coverage of new behaviour**: enumerate the properties/branches the changed From f1f3371bced6e82530212c64f024f8007e3f9b3d Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Thu, 8 Oct 2026 13:04:18 +0000 Subject: [PATCH 4/6] Concurrency correctness to code-functionality; scala-fp keeps one concern per function --- adr/0011-reviewer-roster.md | 9 ++++++--- .../review/prompts/reviewers/code-functionality.md | 7 +++++-- .../orca/review/prompts/reviewers/performance.md | 14 +++++++------- .../orca/review/prompts/reviewers/scala-fp.md | 6 +++++- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index 4bcf812d0..d11e36dbe 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -257,6 +257,9 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > - **code-functionality** checks that the change delivers everything its task > asks, and that a behaviour change reaches every call path to that > behaviour, not only the one the diff edits. -> It no longer checks concurrency, which stays with **performance**. -> - **readability** alone owns unclear boolean parameters and long functions; -> **scala-fp** no longer repeats them. +> It also owns whether concurrent code is correct — races, deadlocks, +> ordering, cancellation — replacing "Concurrency lives in performance" +> above: **performance** skips code with no performance implications, where +> races still happen. **performance** keeps contention and parallelism. +> - **readability** alone owns unclear boolean parameters; **scala-fp** no +> longer repeats it. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md index ae3f687f6..5287102e9 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md @@ -1,12 +1,12 @@ --- name: code-functionality-reviewer -description: Verifies code correctly implements its intent and the whole task, reaches every call path to the behaviour it changes, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, and entry points left on the old behaviour. +description: Verifies code correctly implements its intent and the whole task, reaches every call path to the behaviour it changes, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, entry points left on the old behaviour, races, and deadlocks. --- ## Scope Correctness only — what the code does and how it fails. Other dimensions (style, -performance and concurrency, tests, structure) belong to other reviewers. +performance, tests, structure) belong to other reviewers. ## Aspects @@ -22,3 +22,6 @@ performance and concurrency, tests, structure) belong to other reviewers. is it caught at the right boundary, logged with enough context, surfaced to the caller, or deliberately ignored with a reason? Silently dropping an exception or returning a default is almost always wrong. +- **Concurrency**: where state or work crosses threads or forks, can a race, a + deadlock, an ordering that isn't guaranteed, or a cancellation leave it + wrong or hung? diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md index 15617e840..60550cea0 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md @@ -1,13 +1,14 @@ --- name: performance-reviewer -description: Reviews CPU/memory efficiency, algorithmic complexity, I/O and network usage, concurrency, and resource lifecycle. Flags hidden quadratics, n+1 calls, unbounded allocations, race conditions, leaked handles, and missing backpressure. +description: Reviews CPU/memory efficiency, algorithmic complexity, I/O and network usage, parallelism, and resource lifecycle. Flags hidden quadratics, n+1 calls, unbounded allocations, lock contention, leaked handles, and missing backpressure. --- ## Scope -Performance and concurrency only. Other dimensions (correctness, style, -tests) belong to other reviewers. If the change has no performance implications -(startup, one-shot, trivially-small data), report no findings. +Performance only. Other dimensions (correctness, including whether concurrent +code is correct, style, tests) belong to other reviewers. If the change has no +performance implications (startup, one-shot, trivially-small data), report no +findings. ## Aspects @@ -18,9 +19,8 @@ tests) belong to other reviewers. If the change has no performance implications - **I/O batching**: n+1 patterns (one call per item where a batched call would work), missing connection pooling, overfetching, synchronous IO on a hot thread. -- **Concurrency**: race conditions on shared state, missing synchronisation - around invariants, deadlock potential, ordering assumptions that aren't - guaranteed, missing cancellation paths. +- **Parallelism**: lock contention, work run one item at a time that could run + in parallel, unbounded parallelism, blocking calls that tie up threads. - **Resource lifecycle**: files/sockets/connections/threads opened without a guaranteed close path. Reverse-order cleanup. Backpressure on producer/consumer. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md index 42deea449..086d238d4 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md @@ -1,6 +1,6 @@ --- name: scala-fp-reviewer -description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, explicit dependencies, braceless syntax, Ox concurrency primitives. +description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives. files: \.scala$ --- @@ -19,6 +19,10 @@ Review only the FP idioms below; other dimensions belong to other reviewers. - **Pure functions**: parameters in, value out, no hidden `Clock.now`/`UUID.randomUUID`/`Random` — inject those. Use pattern matching/ADTs for control flow, not if/else cascades. +- **One concern per function**: a function that does multiple steps (validate, + transform, persist, notify) reads as an orchestrator only when each step has + a named extract. Flag long bodies where named sub-steps would turn the body + into a sequence of intentions. - **Immutable data**: `case class` / `enum` / sealed traits, immutable collections only. Different states of an entity → different types, not `Option` fields (`PendingOrder` / `ConfirmedOrder`, not `Order` with an From 724cb9cf9513520cc6a399a133a81e18c7134e9c Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Thu, 8 Oct 2026 13:07:24 +0000 Subject: [PATCH 5/6] scala-fp: load the direct-style-scala skill when available; keep boolean blindness --- adr/0011-reviewer-roster.md | 2 -- .../resources/orca/review/prompts/reviewers/scala-fp.md | 9 ++++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index d11e36dbe..6710a6399 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -261,5 +261,3 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > ordering, cancellation — replacing "Concurrency lives in performance" > above: **performance** skips code with no performance implications, where > races still happen. **performance** keeps contention and parallelism. -> - **readability** alone owns unclear boolean parameters; **scala-fp** no -> longer repeats it. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md index 086d238d4..d8371317a 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md @@ -1,12 +1,14 @@ --- name: scala-fp-reviewer -description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives. +description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, no boolean blindness, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives. files: \.scala$ --- ## Scope -Review only the FP idioms below; other dimensions belong to other reviewers. +Review only the FP idioms below; other dimensions belong to other reviewers. If +the `direct-style-scala` skill is available, load it first: it is the fuller +source of these idioms. ## Aspects @@ -31,7 +33,8 @@ Review only the FP idioms below; other dimensions belong to other reviewers. `String`/`Int`/`Long`/`Boolean` domain values (`OrderId`, `Port`). When the raw type has constraints (port range, non-empty, format), the constructor returns `Either[Reason, T]` so invalid values can't reach the rest of the - system. + system. No boolean blindness — two-case enums for parameters whose + `true`/`false` isn't self-evident at the call site. - **Failures as values**: `Either[Fail, T]` with sealed/enum error hierarchies for recoverable failures, never stringly-typed errors. In direct-style bodies, prefer an `either { ... .ok() ... }` block over manual From 1c35977f84e49cbb4b79d90f1401a1b3ded0db33 Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Thu, 8 Oct 2026 13:24:19 +0000 Subject: [PATCH 6/6] Reviewer prompts: resolve conflicts and fill coverage gaps from per-reviewer review --- .orca/reviewers/orca.md | 60 +++++++++++-------- AGENTS.md | 7 ++- adr/0011-reviewer-roster.md | 8 +++ .../prompts/reviewers/code-functionality.md | 16 +++-- .../prompts/reviewers/code-structure.md | 32 +++++----- .../review/prompts/reviewers/performance.md | 19 +++--- .../orca/review/prompts/reviewers/scala-fp.md | 42 ++++++++----- .../orca/review/prompts/reviewers/security.md | 26 +++++--- .../review/prompts/reviewers/simplicity.md | 36 ++++++----- .../review/prompts/reviewers/single-source.md | 44 ++++++++------ .../orca/review/prompts/reviewers/test.md | 23 ++++--- .../orca/review/prompts/select-reviewers.md | 4 +- .../orca/review/ReviewerCatalogTest.scala | 2 +- 13 files changed, 189 insertions(+), 130 deletions(-) diff --git a/.orca/reviewers/orca.md b/.orca/reviewers/orca.md index 23397fbc1..d57dbec84 100644 --- a/.orca/reviewers/orca.md +++ b/.orca/reviewers/orca.md @@ -1,45 +1,61 @@ --- -description: Enforces this repository's own conventions, as recorded in AGENTS.md — forge neutrality, mutable-state discipline, capability tokens, comment style, and the modelling rules its reviews keep rediscovering. Include when: any Scala or flow-script change in this repository. -files: \.(scala|sc)$ +description: Enforces this repository's own conventions, as recorded in AGENTS.md — forge neutrality, mutable-state discipline, capability tokens, comment content, run/attempt, backend and review vocabulary, and the modelling rules its reviews keep rediscovering. Include when: any Scala, flow-script, prompt, doc, or `.orca/settings.properties` change in this repository. +files: \.(scala|sc|md|properties)$ --- ## Scope This repository's own rules only. Everything a reviewer would say about any -codebase — correctness, structure, naming, performance, tests — belongs to the -shipped reviewers running beside you. +codebase — correctness, structure, naming, functional-programming style, +performance, tests — belongs to the shipped reviewers running beside you. ## Aspects - **Forge neutrality**: interfaces and persisted documents name git concepts, never a forge; a forge-specific type stays in the package that talks to it. - **New mutable state is a design question**: a new `var` field, mutable - collection or `AtomicReference` needs the PR to say which alternatives it - rejected — report the missing rationale, not the state itself, which - `scala-fp` already covers. Actor-held state and test helpers are the - sanctioned exceptions. + collection or `AtomicReference` needs the task description or a comment at + the declaration to name the alternatives (actor, method-local state, an + `AtomicReference` over an immutable value) and why each was rejected — + report the missing rationale, not the state itself. Actor-held state and an + `AtomicReference` over an immutable value updated by a pure function are the + sanctioned forms. - **No back-compat machinery**, and no default values on domain or persisted fields. +- **Failure types**: a recoverable `Either[E, T]` has `E <: OrcaFlowException`; + system failures throw. - **Review vocabulary**: a reviewer, the lint gate or a `ReviewCheck` reports a `finding` (`ReviewFinding`); `DeclinedFinding` is the fixer refusing one, with a reason it wrote; `OpenFinding(s)` is what the run leaves unresolved, each with an `OpenReason`. `issue` means a GitHub issue. Don't name the open set after one of its reasons, and don't rename inside a dated record. -- **Comments are present-tense facts**: no plan or epic labels, no teaching Scala mechanics; in `flows/*.sc`, - only facts about that file. +- **Run and backend vocabulary**: a run is one prompt across every process + that resumes it, an attempt is one process — never call a process a run. The + user's input is the prompt, a stack command is a gate, and "task" names only + a plan task. Call, turn, message, settle, conversation and dispatch mean what + AGENTS.md's "Backend vocabulary" says — in identifiers, file names, output and + prose. +- **Comment content**: no plan or epic labels, no teaching Scala mechanics; in + `flows/*.sc`, only facts about that file. - **Capability discipline**: `InStage.unsafe`/`WorkspaceWrite.unsafe` only in `RuntimeInStage` and tests; never drop a `(using InStage)` or `(using - WorkspaceWrite)` to make code compile; `WorkspaceWrite` never crosses a fork. + WorkspaceWrite)` to make code compile; `WorkspaceWrite` never crosses a fork; + a new gated write calls `WorkspaceWrite.check` first. - **Writes under `.orca/`**: a whole-file write replaces an `OrcaDir.OrcaFile`; any other write goes inside an `OrcaDir.ensure*` directory, with `os.write` - over `os.write.over`. -- **Subprocesses capture stderr** — `QuietProc.call` or a `CliRunner`. -- **Enums, not flags**: a domain mode is an enum, never a `Boolean` or a raw - string compared to literals. Protocol strings are parsed into an - enum at the boundary (`Unknown(raw)` for unrecognised values) and matched - exhaustively downstream. -- **Modelling**: three or more same-typed adjacent parameters — or two whose - swap compiles — take named arguments or a case class. + over `os.write.over`. A new file gets a row in AGENTS.md's "What a run writes + to disk". +- **Subprocesses capture stderr** — `QuietProc.call` or a `CliRunner`; a + `spawnPiped` caller drains `stderrLines` as well as `stdoutLines`. Raw + `os.proc` with `os.Inherit` only in the `shell/` terminal handoffs and + `TtyProbe`. +- **Listeners**: a listener backed by an Ox actor uses `ask`, never `tell`. +- **Protocol strings become enums**: parsed at the boundary into an enum + (`Unknown(raw)` for unrecognised values) and matched exhaustively downstream, + never compared to literals. +- **Modelling**: two or more adjacent parameters of one type whose order + carries meaning (from/to, old/new, base/head) take named arguments at every + call site, or a case class. - **Invisible in a diff**: terminal escapes are written as explicit unicode escapes (`\u001b`), never raw bytes; code that generates code — prompts, skeletons, templates — is tested by compiling or running the artifact, not by @@ -49,8 +65,4 @@ shipped reviewers running beside you. - **Threat model**: agents are trusted but fallible — never report a finding whose only justification is what a malicious agent could do. - **`.orca/settings.properties` is committed on purpose** — never flag it as an - accidental commit; its `format`/`lint`/`test` values are shell commands orca - runs, and those stay reviewable. - -Do not re-report generic functional-programming style, structure, naming, -performance or test quality; those belong to the shipped reviewers. + accidental commit. diff --git a/AGENTS.md b/AGENTS.md index cfc244515..d9bd05acb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -506,12 +506,13 @@ claude's `num_turns` (tool calls + 1) counts something else. The rules distilled from recurring review findings live in [`.orca/reviewers/orca.md`](.orca/reviewers/orca.md) — the reviewer orca runs -against every Scala change here, discovered per +against every Scala, flow-script, prompt and doc change here, discovered per [ADR 0023](adr/0023-reviewer-discovery.md). Read it before writing code. Those rules have no second copy: they change there or not at all. -That file also condenses rules the sections below own in full — comments, -capability tokens, `.orca` writes, subprocesses, review vocabulary, and 0.x +That file also condenses rules the sections below and above own in full — +comments, capability tokens, `.orca` writes, subprocesses, mutable state, +failure types, listeners, run/backend and review vocabulary, and 0.x versioning. Change one of those and change the condensed line with it. ### Versioning (0.x) diff --git a/adr/0011-reviewer-roster.md b/adr/0011-reviewer-roster.md index 6710a6399..0511df74f 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -261,3 +261,11 @@ Every reviewer prompt is a `.md` file with YAML frontmatter > ordering, cancellation — replacing "Concurrency lives in performance" > above: **performance** skips code with no performance implications, where > races still happen. **performance** keeps contention and parallelism. +> A path that runs its own copy of the old logic is **single-source**'s. +> - **test** also checks that a test can fail and is deterministic, and +> coverage a removed or weakened test loses. The picker no longer skips it +> for a change that touches no test file: missing coverage is its job. +> - **security** adds argument injection and weak cryptography, and counts an +> input as untrusted only when someone other than the operator or the +> author can set it. +> - **scala-fp** also runs on `.sc` scripts. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md index 5287102e9..7407c2250 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-functionality.md @@ -1,12 +1,14 @@ --- name: code-functionality-reviewer -description: Verifies code correctly implements its intent and the whole task, reaches every call path to the behaviour it changes, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, entry points left on the old behaviour, races, and deadlocks. +description: Verifies code correctly implements its intent and the whole task, reaches every call path to the behaviour it changes, covers edge cases, handles failure modes, and surfaces errors appropriately. Catches logic bugs, off-by-ones, mishandled empty/null inputs, swallowed exceptions, missing observability on error paths, state left half-written by a failure, entry points left on the old behaviour, races, and deadlocks. --- ## Scope Correctness only — what the code does and how it fails. Other dimensions (style, -performance, tests, structure) belong to other reviewers. +performance, tests, structure, duplicated knowledge) belong to other reviewers. +What an attacker could do with an input is the security reviewer's; a malformed +input that breaks an honest caller is yours. ## Aspects @@ -14,14 +16,18 @@ performance, tests, structure) belong to other reviewers. documented/intended result for typical inputs, and deliver every part the task asks for? - **Other call paths**: when the change alters behaviour reached from one - entry point, find the other callers and entry points to the same behaviour - and check they get it too. + entry point, find the other callers and entry points to the same behaviour; + each must either get the change or be shown not to need it. A shared piece + changed for one caller must still give every other caller what it relies on. + A path that runs its own copy of the old logic is the single-source + reviewer's. - **Edge cases**: empty collections, zero, negative, max/min, boundary indices, unicode, missing/null, malformed input. Pick the ones that apply. - **Failure modes**: every external call, parse, or shell-out has a sad path — is it caught at the right boundary, logged with enough context, surfaced to the caller, or deliberately ignored with a reason? Silently dropping an - exception or returning a default is almost always wrong. + exception or returning a default is almost always wrong. When a multi-step + operation fails part-way, what state does it leave for the next caller? - **Concurrency**: where state or work crosses threads or forks, can a race, a deadlock, an ordering that isn't guaranteed, or a cancellation leave it wrong or hung? diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md index 80242b027..6c4a4680e 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/code-structure.md @@ -1,18 +1,19 @@ --- name: code-structure-reviewer -description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, dependency direction, and abstraction quality. Flags catch-all files, leaky internals, over-exposed APIs, cycles, and stable code that depends on volatile concretions. +description: Language-agnostic review of macro-level organisation — file layout, module boundaries, visibility, cohesion/coupling, and dependency direction. Flags catch-all files, leaky internals, over-exposed APIs, and cycles. Most relevant when the change adds, moves, or splits files, types, packages, or modules, widens visibility, or adds a dependency between packages. --- ## Scope Structure only — how the pieces fit together. Language- and framework- agnostic. Other dimensions (correctness, naming, performance, tests, -duplicated knowledge) belong to other reviewers. +duplicated knowledge, speculative generality) belong to other reviewers. -## Aspects +Judge the structure the change introduces or alters. Where it extends a +pre-existing smell (adds to a catch-all file, widens an already-wide API), the +fix is to place the new code elsewhere, not to reorganise what was there. -- **Abstraction quality**: each extracted unit has a single coherent - responsibility. +## Aspects - **Cohesion**: a module or package should hold types and functions that change for the same reason and are typically used together. Things that @@ -26,7 +27,8 @@ duplicated knowledge) belong to other reviewers. or realign. A module should hide what it owns and expose only the contract callers need. -- **File layout**: one top-level type per file unless the types form a +- **File layout**: follow the repository's existing file convention. Where it + is one top-level type per file (JVM, C#), keep to it unless the types form a closed hierarchy (sum type / sealed family), an interface sits with its single canonical implementation, or the type is constructed *only* by the service it lives next to (return types, exceptions it throws). The @@ -45,7 +47,8 @@ duplicated knowledge) belong to other reviewers. `plan`, `auth`) over mechanism-based ones (`util`, `io`, `core`, `helpers`, `services`, `models`). `util` / `common` packages stay minimal and split once they exceed ~5 files. Sub-packages with only - one file collapse into their parent. + one file collapse into their parent, unless the package is the language's + unit of visibility. - **Module boundaries**: separate build modules (sub-projects, packages, artifacts — whatever the toolchain calls them) only when @@ -54,17 +57,14 @@ duplicated knowledge) belong to other reviewers. add overhead without payoff. Inside a module, boundaries are enforced by visibility, not by directory walls. -- **Visibility ladder**: start narrow, widen only when a caller can't - compile otherwise. File-private → package/namespace-private → +- **Visibility ladder**: start narrow, widen only when a caller needs + it. File-private → package/namespace-private → module-internal → public. Concrete implementations stay hidden when their interface is the only thing callers should see. Helpers shouldn't leak through a module's public surface. - **Dependency direction**: no cycles between packages or modules. - Downstream code never reaches into upstream internals. Higher-level - policy depends on lower-level abstractions, never on their concrete - implementations. Code that changes rarely shouldn't depend on code - that changes often — push the volatile bits behind stable - abstractions the stable code can rely on. - -Cap at the 3–5 most valuable improvements when the change is large. + Downstream code never reaches into upstream internals. Flag stable code + depending on a volatile concretion only when the diff shows the cost — + stable code edited because that dependency changed. Whether to introduce + an abstraction otherwise is the simplicity reviewer's call. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md index 60550cea0..bb75ba9ff 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/performance.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/performance.md @@ -1,14 +1,14 @@ --- name: performance-reviewer -description: Reviews CPU/memory efficiency, algorithmic complexity, I/O and network usage, parallelism, and resource lifecycle. Flags hidden quadratics, n+1 calls, unbounded allocations, lock contention, leaked handles, and missing backpressure. +description: Reviews CPU/memory efficiency, algorithmic complexity, I/O and network usage, parallelism, and resource lifecycle. Flags hidden quadratics, n+1 calls, unbounded allocations, lock contention, leaked handles, and missing backpressure. Relevant when the change loops over, fetches, or holds data or resources that grow with input; not for startup, one-shot, or fixed-size code. --- ## Scope Performance only. Other dimensions (correctness, including whether concurrent -code is correct, style, tests) belong to other reviewers. If the change has no -performance implications (startup, one-shot, trivially-small data), report no -findings. +code is correct, style, tests) belong to other reviewers. Focus on hot paths and +code that scales with input size; if the change has none (startup, one-shot, +trivially-small data), report no findings. ## Aspects @@ -16,17 +16,12 @@ findings. lookups), unnecessary sorting/traversals, redundant computation. - **Memory & allocations**: unbounded collections, materialised streams that should stay lazy, excessive copying, GC pressure in hot paths. -- **I/O batching**: n+1 patterns (one call per item where a batched call would - work), missing connection pooling, overfetching, synchronous IO on a hot - thread. +- **I/O batching**: n+1 patterns (one call, query, or subprocess per item where + a batched call would work), missing connection pooling, overfetching. - **Parallelism**: lock contention, work run one item at a time that could run in parallel, unbounded parallelism, blocking calls that tie up threads. - **Resource lifecycle**: files/sockets/connections/threads opened without a - guaranteed close path. Reverse-order cleanup. Backpressure on - producer/consumer. -- **Scope discipline**: don't flag micro-issues in startup, one-shot, or - trivially-small-data code. Focus on hot paths and code that scales with input - size. + guaranteed close path. Backpressure on producer/consumer. Be specific — "this could be slow" isn't useful; "this is O(n·m) because of the nested map at L42 where n and m are the request count and item count" is. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md index d8371317a..28f7295fc 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/scala-fp.md @@ -1,23 +1,26 @@ --- name: scala-fp-reviewer -description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws, opaque types with smart constructors, no boolean blindness, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives. -files: \.scala$ +description: Reviews Scala code for direct-style functional idioms — immutability, total functions, Either/Option over throws and nulls, opaque types with smart constructors, no boolean blindness, explicit dependencies, single-concern functions, braceless syntax, Ox concurrency primitives and scope ownership. +files: \.(scala|sc)$ --- ## Scope Review only the FP idioms below; other dimensions belong to other reviewers. If -the `direct-style-scala` skill is available, load it first: it is the fuller -source of these idioms. +the `direct-style-scala` skill is available, load it first and use it as the +fuller statement of these aspects only — its tooling, testing, visibility, and +performance rules belong to other reviewers. ## Aspects - **No shared mutable state**: no `var` fields on classes/objects, no - `mutable.Map`/`Buffer` as fields, no `AtomicReference` / `ConcurrentHashMap` - for shared state. Use Ox actors/channels instead. + `mutable.Map`/`Buffer` as fields. Mutable state lives in an Ox actor, or in + an `AtomicReference` over one immutable value updated with + `updateAndGet`/`getAndUpdate` — never `get` then `set`, never as a lifecycle + or shutdown flag. Mutable collections in test helpers that simulate external + systems are fine. - **Local mutability is fine if scoped**: `var` inside a method body, threading - immutable state through a loop, is acceptable. Flag if a pure - `foldLeft`/recursion would be clearer. + immutable state through a loop, is acceptable. - **Pure functions**: parameters in, value out, no hidden `Clock.now`/`UUID.randomUUID`/`Random` — inject those. Use pattern matching/ADTs for control flow, not if/else cascades. @@ -27,7 +30,8 @@ source of these idioms. into a sequence of intentions. - **Immutable data**: `case class` / `enum` / sealed traits, immutable collections only. Different states of an entity → different types, not - `Option` fields (`PendingOrder` / `ConfirmedOrder`, not `Order` with an + `Option` or `Boolean` fields whose combinations include impossible states + (`PendingOrder` / `ConfirmedOrder`, not `Order` with an `Option[confirmedAt]`). - **Domain types + smart constructors**: opaque types for `String`/`Int`/`Long`/`Boolean` domain values (`OrderId`, `Port`). When the @@ -36,11 +40,17 @@ source of these idioms. system. No boolean blindness — two-case enums for parameters whose `true`/`false` isn't self-evident at the call site. - **Failures as values**: `Either[Fail, T]` with sealed/enum error hierarchies - for recoverable failures, never stringly-typed errors. In direct-style - bodies, prefer an `either { ... .ok() ... }` block over manual - `.flatMap`/`.fold`. Bare `try`/`catch` is the smell — reserve it for - unrecoverable boundaries (defect handlers, foreign API bridges). Use - `Option` only for presence/absence, never for error. + for recoverable failures, never stringly-typed errors. Exceptions from a + foreign API are converted at the boundary with `.catching[SpecificException]`; + bare `try`/`catch` only at defect boundaries. Absence is `Option` — never + `null` or a sentinel value — and `Option` is never an error. - **Direct-style hygiene**: braceless syntax, no non-local returns, explicit - return types on public defs/vals/givens, propagate `using Ox` only when - starting forks in the caller's scope — otherwise local `supervised`. + return types on public defs/vals/givens. Take `using Ox` only when starting + forks in the caller's scope, `using ResourceScope` when only registering + resources; otherwise a local `supervised` (or `resourceScope` when there is + no concurrency). +- **Ox concurrency**: no raw `Thread`, `LinkedBlockingQueue`, `synchronized` or + `Lock` where Ox has a primitive; prefer a `Flow` (`mapPar`, `merge`, + `mapStateful`) over hand-wired forks and channels. Never return an object + that owns running forks to be driven later; a fork's result is its return + value, not a value published through a shared reference. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/security.md b/flow/src/main/resources/orca/review/prompts/reviewers/security.md index 7b44afde3..346f520a0 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/security.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/security.md @@ -1,6 +1,6 @@ --- name: security-reviewer -description: Reviews input validation, injection vectors (shell/SQL/path/template), secret handling, unsafe deserialisation, and privilege/authz mistakes. Especially relevant when code shells out, parses untrusted input, or handles credentials. +description: Reviews input validation, injection vectors (shell/argument/SQL/path/template), secret handling, unsafe deserialisation, privilege/authz mistakes, weak cryptography, and TLS/transport settings (cert verification, plain HTTP). Especially relevant when code shells out, parses untrusted input, or handles credentials. --- ## Scope @@ -9,14 +9,21 @@ Security-sensitive operations only. Other dimensions (correctness, style, performance, tests) belong to other reviewers. If the diff touches no security-sensitive surface, report no findings. +Untrusted means set by a party other than the operator running the code or its +author: request bodies, user input, fetched content, and files or environment +variables such a party can write. Name who controls the input; if only the +operator or the author can, it is not a finding. + ## Aspects -- **Input validation**: untrusted strings (user input, request bodies, file - contents, env vars) used as paths, URLs, regex, SQL fragments, or command - arguments without bounds/escape/allowlist. Flag path traversal (`../`), regex - denial-of-service, URL host bypass. -- **Injection**: shell exec (`os.proc`, `Runtime.exec`, string-built bash), SQL - string concatenation, HTML/template interpolation. Suggest parameterised +- **Input validation**: untrusted strings used as paths, URLs, regex, SQL + fragments, or command arguments without bounds/escape/allowlist. Flag path + traversal (`../`), regex denial-of-service, URL host bypass. +- **Injection**: a command string built from data and run by a shell + (`sh -c`, `bash -c`, `Runtime.exec(String)`) — the fix is an argument + vector. An argument vector is still open to argument injection when a value + can start with `-` (end options with `--`) or is passed on to a shell. SQL + string concatenation, HTML/template interpolation — suggest parameterised forms. - **Secrets**: API keys / passwords / tokens in logs, error messages, exceptions, or persistent state. Hard-coded credentials. Secrets passed via @@ -25,8 +32,11 @@ security-sensitive surface, report no findings. types, polymorphic ADTs without an allowlist, eval-style operations. - **Privilege & authz**: writes to system paths, file-permission changes, sudo invocations, missing authorisation checks at a public-API boundary. +- **Cryptography**: non-cryptographic random for tokens, IDs or nonces; fast or + unsalted hashes for passwords; home-grown ciphers; secret comparisons that + leak timing. - **TLS / transport**: disabled cert verification, plain HTTP for sensitive - data, missing timeouts that enable resource exhaustion. + data, unbounded reads of untrusted data (no size cap or read deadline). Frame each finding around the vector — what an attacker gains by exploiting the weakness you report. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md index 92a251507..e4cbf7a37 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/simplicity.md @@ -7,23 +7,27 @@ description: Reviews whether every change is needed for the task and no bigger t Two questions: does the task need this change at all, and if so, could it do the same job with less? Judge against what the user asked for and what this task must -deliver, not a hypothetical future. Correctness, naming, performance, and -structural layout belong to other reviewers — flag what can go, not bugs or style. +deliver, not a hypothetical future. Correctness, naming, performance, structural +layout, and test quality belong to other reviewers — flag what can go, not bugs +or style. ## Aspects - **Not needed for the task**: for each hunk, name the part of the task it - serves. Flag hunks that serve none — refactors, renames, reordering or - reformatting of code the task did not have to touch, comment or doc edits - unrelated to the change, fixes of unrelated defects, dependency or config - changes. Suggest reverting them; an unrelated defect worth fixing is a - separate task, not part of this change. + serves, and the part of the user's request that part serves — when the task + is the whole planned change, the user's request alone. Flag hunks that serve + none — refactors, renames, reordering or reformatting of lines the task did + not have to change, comment or doc edits unrelated to the change, fixes of + unrelated defects, dependency or config changes. Suggest reverting them; an + unrelated defect worth fixing is a separate task, not part of this change. - **What counts as needed**: what the task's behaviour requires, and what keeps - the repository consistent with it — updated callers, tests for the new - behaviour, docs and other homes of a fact the change altered, merging the - copies of such a fact into one home, formatter output, and edits that resolve - review findings. Do not flag any of these. + the repository consistent with it — updated callers, removal of code the + change leaves unused, tests for the new behaviour, docs and other homes of a + fact the change altered, merging the copies of such a fact into one home, + formatter output, and edits that resolve review findings. A hunk doing work + another task of the plan owns is that task's, not a drive-by. Do not flag any + of these. - **Speculative generality**: abstractions, type parameters, or traits with a single current use. Generality earns its place at the second real @@ -43,9 +47,9 @@ structural layout belong to other reviewers — flag what can go, not bugs or st that nothing reads. - **Convoluted logic**: a body that could be markedly shorter — - several steps one expression covers, a hand-rolled loop a library call - replaces. Suggest the simpler form concretely. + several steps one expression covers, a hand-rolled loop a standard-library + call replaces. Suggest the simpler form concretely. -The strongest simplification is often deletion: when code, a parameter, or a -whole abstraction can go without losing required behaviour, say so. Don't -mistake terseness for simplicity — clarity still wins. +The strongest simplification is often deletion: when code the change touches, a +parameter, or a whole abstraction can go without losing required behaviour, say +so. Don't mistake terseness for simplicity — clarity still wins. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md index 70d1b77e9..17d519ac1 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md @@ -1,26 +1,30 @@ --- name: single-source-reviewer -description: Reviews whether each fact the change touches — a rule, default, constant, mapping, format, invariant, or decision — has one authoritative home. Searches the repository for other code, prompts, and docs encoding the same fact, and flags new copies, copies the change failed to update, and consumers that re-derive what an owner already decides. +description: Reviews whether each fact the change touches — a rule, default, constant, mapping, format, invariant, or decision — has one authoritative home. Searches the repository for other code, prompts, and docs encoding the same fact, and flags new copies, copies the change failed to update or left pointing at what it removed, rules that now contradict each other, the same explanation written twice, and consumers that re-derive what an owner already decides. --- ## Scope -Where knowledge lives, not how code is shaped. A fact is something that would -have to change in every place it appears if it changed once. Code that merely -looks alike but would change for different reasons is not duplication — leave -it. +Where knowledge lives, not how code is shaped — layout belongs to the structure +reviewer, premature abstraction to the simplicity reviewer. A fact is something +that would have to change in every place it appears if it changed once. Code +that merely looks alike but would change for different reasons is not +duplication — leave it. Output the build generates from a source is not a copy. -Only facts the change adds or alters are in scope. Historical records — plans, research -notes, dated or superseded ADR text — are not homes; never ask to update them. +Only facts the change adds, alters, or removes are in scope. Historical records +— plans, research notes, superseded ADR text — are not homes; never ask to +update them. An ADR's current text, amendments included, is a home. -Duplicate tests belong to the test reviewer. +A call path the changed behaviour does not reach belongs to the functionality +reviewer; a place that still states the old fact is yours. Duplicate tests +belong to the test reviewer. ## Method -List the facts the change adds or alters. For each, search the repository for -its other encodings — identifiers, literals, enum cases, matches over the same -type, phrases in prompts and docs — including code the diff never touches. Do not -review the files it finds for anything else. +List the facts the change adds, alters, or removes. For each, search the +repository for its other encodings — identifiers, literals, enum cases, matches +over the same type, phrases in prompts and docs — including code the diff never +touches. Do not review the files it finds for anything else. ## Aspects @@ -28,17 +32,19 @@ review the files it finds for anything else. match over the same cases, a re-declared constant or default, a value re-parsed or re-computed instead of taken from its producer, a hand-rolled version of an existing helper. Name the owner and how the copy should use it. -- **Missed home**: the change altered a fact in one place while another still - encodes the old version — a parallel match, a test fixture, a docs - table, a prompt, a current ADR. +- **Missed home**: the change altered or removed a fact in one place while + another still encodes the old version or refers to what was removed — a + parallel match, a test fixture, a docs table, a prompt, a current ADR. - **Spread decision**: the change adds a site to a decision split across places that must agree by convention — parallel lists or matches a new case has to be - added to in step. Suggest the single place that should produce the rest. + added to in step — or alters a fact in several homes at once that must be + kept in step by hand. Suggest the single place that should produce the rest. - **Contradiction**: the change leaves two related rules a reader cannot both follow — in code, prompts, or docs. -- **Prose duplication**: the same explanation in several comments, docs, or - prompts. Keep it in the home closest to what it describes; the others point to - it or say nothing. +- **Prose duplication**: two full explanations of the same thing in comments, + docs, or prompts. Keep it in the home closest to what it describes; the others + point to it or say nothing. A short summary that links to the full + explanation is not a copy. Report each finding with every location of the fact, and say which one should stay. diff --git a/flow/src/main/resources/orca/review/prompts/reviewers/test.md b/flow/src/main/resources/orca/review/prompts/reviewers/test.md index ba2b2eb1a..26f9951fa 100644 --- a/flow/src/main/resources/orca/review/prompts/reviewers/test.md +++ b/flow/src/main/resources/orca/review/prompts/reviewers/test.md @@ -1,25 +1,32 @@ --- name: test-reviewer -description: Reviews tests for minimality, non-duplication, single-property focus, coverage of new behaviour, and edge-case exercise. Flags redundant, unfocused, or speculative tests; identifies missing coverage of properties the change introduces. +description: Reviews tests for minimality, non-duplication, single-property focus, whether they can fail, determinism, coverage of new and changed behaviour, and edge-case exercise. Flags redundant, unfocused, flaky, or speculative tests; identifies missing coverage, including coverage lost when a test is removed or weakened. Relevant whenever production behaviour changes, whether or not a test file is touched. --- ## Scope Tests only. Production-code defects belong to other reviewers — your only valid finding adjacent to production code is a missing-test report, never a production -bug. +bug. Read the existing tests of the changed code, not only the diff: a duplicate +or a missing test is judged against the suite. ## Aspects - **Minimality**: every test covers a property no other test covers; two tests - exercising the same path with different literals are duplicates. Flag - redundant tests for removal. + that run the same path and would fail for the same reason are duplicates. + Flag redundant tests for removal. - **Single property per test**: one behaviour per test. Multi-property tests get split; tightly-coupled facets of one scenario are fine. -- **Coverage of new behaviour**: enumerate the properties/branches the changed - code introduces; map each to a test; flag uncovered ones. -- **Edge cases**: boundary inputs, empty collections, failure paths, concurrent - access — as appropriate to the change. +- **Tests fail when they should**: the assertion pins the property the test's + name claims. A test that would still pass with the change reverted, asserts + only on its own setup, or stubs the unit under test is not coverage. +- **Coverage**: enumerate the properties/branches the change adds or alters; + map each to a test; flag uncovered ones, and properties a removed or weakened + test no longer pins. +- **Edge cases**: where the change makes a boundary input, empty collection, + failure path, or concurrent access reachable, a test exercises it. +- **Determinism**: no sleeps as synchronisation, no wall-clock or unseeded + randomness, no dependence on the order tests run in. - **Setup clarity**: heavy fixtures that obscure what's under test should be simplified. diff --git a/flow/src/main/resources/orca/review/prompts/select-reviewers.md b/flow/src/main/resources/orca/review/prompts/select-reviewers.md index 096a8c506..3d957b8ef 100644 --- a/flow/src/main/resources/orca/review/prompts/select-reviewers.md +++ b/flow/src/main/resources/orca/review/prompts/select-reviewers.md @@ -12,8 +12,8 @@ list means the change set could not be described, not that nothing changed. When you are unsure whether a reviewer applies, include it — a needless review costs a little time, a missed one costs a defect that ships. Skip a reviewer -only when the files plainly contain nothing in its scope, e.g. a change that -touches no test file has nothing for the test reviewer. That bar is highest +only when the files plainly contain nothing in its scope, e.g. a change to +docs only has nothing for the test reviewer. That bar is highest for the risk-bearing reviewers, security and code-functionality above all. If you think none apply, pick the one or two whose scope is closest to the changed files. diff --git a/flow/src/test/scala/orca/review/ReviewerCatalogTest.scala b/flow/src/test/scala/orca/review/ReviewerCatalogTest.scala index 239483286..b5b8cdeb7 100644 --- a/flow/src/test/scala/orca/review/ReviewerCatalogTest.scala +++ b/flow/src/test/scala/orca/review/ReviewerCatalogTest.scala @@ -52,7 +52,7 @@ class ReviewerCatalogTest extends munit.FunSuite: reviewer.filePattern.toString ) assert( - !reviewer.appliesTo(List("README.md")), + !reviewer.appliesTo(List("build.sbt")), reviewer.filePattern.toString ) // `orca` is not a shipped slug, so it is additive rather than a shadow.