diff --git a/.orca/reviewers/orca.md b/.orca/reviewers/orca.md index e606d4a8d..d57dbec84 100644 --- a/.orca/reviewers/orca.md +++ b/.orca/reviewers/orca.md @@ -1,50 +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 history ("no longer", "renamed - from"), 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; two flags or `Option`s whose combinations - include impossible states are one ADT. 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; one decision, - one home, so display and summary code consumes the production resolver - instead of mirroring the rule. + 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 @@ -54,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 29e8f3895..0511df74f 100644 --- a/adr/0011-reviewer-roster.md +++ b/adr/0011-reviewer-roster.md @@ -242,3 +242,30 @@ 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**; +> 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 +> named `simplicity.md` still shadows it. +> - **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 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. +> 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/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..adc8c28ff 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}} @@ -41,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 665e48bfc..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,23 +1,33 @@ --- 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, 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 - **Intent vs. behaviour**: trace the code; does it produce the - documented/intended result for typical inputs? + 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; + 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? -- **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. 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 0a7f5c366..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,24 +1,19 @@ --- 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, 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) belong -to other reviewers. +agnostic. Other dimensions (correctness, naming, performance, tests, +duplicated knowledge, speculative generality) 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. +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**: 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. +## Aspects - **Cohesion**: a module or package should hold types and functions that change for the same reason and are typically used together. Things that @@ -32,7 +27,8 @@ 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 @@ -51,7 +47,8 @@ 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 @@ -60,17 +57,14 @@ 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 79722a289..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,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. 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 and concurrent safety 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. 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 @@ -15,18 +16,12 @@ tests) belong to other reviewers. If the change has no performance implications 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. -- **Concurrency**: race conditions on shared state, missing synchronisation - around invariants, deadlock potential, ordering assumptions that aren't - guaranteed, missing cancellation paths. +- **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/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..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,21 +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. +Review only the FP idioms below; other dimensions belong to other reviewers. If +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. @@ -25,7 +30,8 @@ Review only the FP idioms below; other dimensions belong to other reviewers. 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 @@ -34,11 +40,17 @@ Review only the FP idioms below; other dimensions belong to other reviewers. 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 d2f0acc9d..e4cbf7a37 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,41 @@ --- 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 — 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 -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, structural +layout, and test quality belong to other reviewers — flag what can go, not bugs +or style. ## Aspects -- **Speculative generality**: abstractions, type parameters, traits, or config - knobs with a single current use. Generality earns its place at the second real +- **Not needed for the task**: for each hunk, name the part of the task it + 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, 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 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, + 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 @@ -29,11 +46,10 @@ reviewers — flag complexity, not bugs or style. 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 — - several steps one expression covers, a hand-rolled loop a library call - replaces. Suggest the simpler form concretely. +- **Convoluted logic**: a body that could be markedly shorter — + 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. Cap at the 3–5 most -valuable simplifications when the change is large. +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 new file mode 100644 index 000000000..17d519ac1 --- /dev/null +++ b/flow/src/main/resources/orca/review/prompts/reviewers/single-source.md @@ -0,0 +1,50 @@ +--- +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 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 — 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, 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. + +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, 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 + +- **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 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 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 — 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**: 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 1e57c6f51..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,26 +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 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 + 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/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, 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.