Review for essential changes and single source of truth

.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.

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)

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.

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

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.

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
 

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?

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.