diff --git a/CLAUDE.md b/CLAUDE.md index 5f79762..3010b9c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,6 +51,9 @@ knob with a sane default — not an operator toggle. Prefer a good default over ## Workflow - Branch + PR; never commit directly to `main`. Merge on green CI. +- Review checklist: a PR that removes a consumer of a shared layer states the + layer's remaining live-consumer count in one line; a drop-to-one is a refactor + smell to flag, not merge silently ([ADR-0042](docs/adr/0042-flag-remaining-consumer-count-on-consumer-removal.md)). - **Releases follow strict semver**, computed from the conventional-commit subjects since the last tag: `feat` → **minor**; `fix`/`perf`/`refactor`/`chore`/`docs`/`ci`/`build` → **patch**; `!` or `BREAKING CHANGE` → **major** (pre-1.0: breaking → minor). Do not diff --git a/docs/adr/0042-flag-remaining-consumer-count-on-consumer-removal.md b/docs/adr/0042-flag-remaining-consumer-count-on-consumer-removal.md new file mode 100644 index 0000000..abe65f4 --- /dev/null +++ b/docs/adr/0042-flag-remaining-consumer-count-on-consumer-removal.md @@ -0,0 +1,105 @@ +# 0042. Flag a shared layer's remaining live-consumer count when a PR removes a consumer + +- Status: Accepted +- Date: 2026-08-08 +- Relates to: [0024](0024-no-redundant-by-construction-predicates.md) + +## Context + +PR #189 (`perf(model): cache all ollama completions in one bounded client middleware`) +added a bounded LRU cache in front of `chat()`, deliberately keyed at the model-client +boundary "so EVERY model consumer is covered by one mechanism" — its two consumers at +the time were the adjudicator (ADR-0013) and the model-backed hypothesis stage +(`ModelHypothesizer`). Reviewed alone, it is a clean shared-layer change: one cache, +two call sites, no gap. + +Eight minutes later, PR #190 (`refactor(engine): remove the model-backed hypothesis +stage — deterministic proof only`) deleted `ModelHypothesizer` entirely: `proof::prove` +already enumerated every structurally-proven chain by exhaustive walk, so the +hypothesis stage discovered nothing and only burned a completion per pass. Reviewed +alone, it is also a clean deletion — the commit message even names the consequence +correctly ("The ONLY remaining Ollama consumer is the adjudicator"). + +Neither PR was wrong on its own, and neither review caught what their conjunction +did: PR #190 deleted one of the cache's two consumers, dropping PR #189's +just-added shared layer to a single caller. With one consumer, the LRU stopped +being a *shared* cache and became a redundant wrapper around a cache that already +existed one layer up — the deterministic verdict store (ADR keyed on the exact +prompt hash) already short-circuited `chat()` on a hit; the LRU added nothing on a +miss but a *correctness hazard*, because it cached any HTTP 200 including replies +that parse to `Uncertain`, while the verdict store deliberately never caches +`Uncertain` (it must retry). The bug shipped, then had to be found and reverted in +PR #191 (`revert(model): drop the ollama completion cache — redundant + +Uncertain-pinning hazard`), less than an hour after PR #190 merged. + +The root cause was not a defect in either diff. It was that **per-PR review has no +view of a neighboring PR's effect on the same abstraction.** PR #189's reviewer +had no reason to open `hypothesis.rs`; PR #190's reviewer had no reason to open +`model/cache.rs`. The redundancy existed only in the two PRs' conjunction, and nothing +in either diff, or in CI, surfaced it. This is the same failure family +[ADR-0024](0024-no-redundant-by-construction-predicates.md) names for a single PR +(a predicate whose result is already fixed by an existing arm is dead code, however +tidy and tested) — except here the fixing arm and the redundant layer landed as *two +separate, individually-reasonable* PRs, which is exactly the case ADR-0024's per-PR +framing cannot catch. + +## Decision + +**When a PR removes a consumer of a shared layer or abstraction (a cache, a port, a +shared middleware, a common helper used by more than one call site), the PR +description states the layer's remaining live-consumer count in one line** — e.g. +"the model-client cache now has 1 remaining consumer (the adjudicator)." This is a +statement of fact the author already has (they just read the call sites to make the +deletion), not new analysis. + +A remaining count of **one** is flagged in review as a refactor smell, not merged +silently: a layer built to serve multiple consumers that now serves one is either (a) +no longer earning its abstraction — inline it or fold it into its sole caller, or (b) +still justified (a real seam the count will grow back into) — in which case the PR +description says so in the same line. Either way the reviewer sees the number and +makes the call explicitly, instead of the drop-to-one passing unnoticed because the +diff itself looks like a clean, unrelated deletion. + +This is a review-time discipline, not a new automated check: no tool here reliably +knows what counts as "a shared layer" or enumerates its call sites across the whole +tree on every consumer-deletion diff. It is enforced the way ADR-0024's rule is +enforced — a reviewer checklist item and a citable ADR to point at. + +## Consequences + +Easier: +- The exact failure PR #189/#190/#191 hit is caught at review time on the + consumer-removal PR, before merge, instead of being found by a later revert. +- Reviewers get a one-line, low-cost signal (a count, not a design review) that + surfaces the cross-PR case ADR-0024 already covers for the single-PR case. +- A drop-to-one that *is* still justified gets its justification recorded in the PR + description at the moment it happens, rather than reconstructed later from git + archaeology. + +Harder / accepted: +- This depends on the PR author actually running the count and writing the line; + nothing in CI enforces it (see Scope, below). Accepted: this repo's review + discipline is documented as reviewer checklist items elsewhere (ADR-0024, this + ADR) rather than as tooling, and a one-line prompt is cheap enough to expect a + reviewer to ask for it when it's missing. +- Does not by itself detect the case in reverse — a PR that *adds* a second consumer + to a layer built for one is not this ADR's concern; that is ordinary code reuse and + is not the redundancy failure mode observed here. +- Scope: catching redundancy that spans a whole review *batch* (not just a single + consumer-removal PR against the tree at merge time) is a workflow-tooling concern, + not a repo convention — out of scope for this ADR. + +## References + +- [0024](0024-no-redundant-by-construction-predicates.md) — the sibling rule for the + single-PR case (a predicate whose result is already fixed by an existing arm is not + defense in depth, it is dead code); this ADR extends the same discipline across a + two-PR conjunction that no single diff shows. +- PR #189 (`perf(model): cache all ollama completions in one bounded client + middleware`) — added the shared cache "so EVERY model consumer is covered." +- PR #190 (`refactor(engine): remove the model-backed hypothesis stage — + deterministic proof only`) — deleted one of the cache's two consumers eight + minutes later, dropping it to one. +- PR #191 (`revert(model): drop the ollama completion cache — redundant + + Uncertain-pinning hazard`) — the revert that found the conjunction, less than an + hour after PR #190 merged. diff --git a/docs/adr/README.md b/docs/adr/README.md index c393181..ddaa688 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -47,5 +47,6 @@ Copy [`0000-template.md`](0000-template.md) to start one. | [0040](0040-node-scoped-containment-mechanism-escalation.md) | Node-scoped containment is a **deterministic mechanism escalation** of a model-decided target: when typed evidence proves the adversary broke the *pod* boundary (host-cred read; root + escape edge; ptrace/module-load; or ≥2 model-confirmed actively-exploited pods co-resident), the resolver escalates the model-named workload's cut from a pod NetworkPolicy to `ContainNode` (cordon + co-resident default-deny) — the model keeps *what*, determinism owns *how* (rejects a model-selectable node menu line: highest blast on the weakest measured axis). Own `node` action class at ladder rung 3 (above quarantine), propose-first by construction (a node always has alive collateral), reversible + ownership-marked + one-node-cap + control-plane-excluded + worker-floor; posture-derived `nodes` RBAC; ships shadow-first (trigger/proposal, then actuator). Also records: adversary-reach is a **presentation annotation, never judge context** (context-not-evidence enforced by absence from the prompt), and off-path auto-containment is a **won't-build** (reaffirms 0032 §6, strengthened by 0038). Refines 0032/0034; reuses 0010/0017/0021/0035/0036 rails | Proposed | | [0041](0041-narrow-blanket-notable-exec-corroboration.md) | Narrow the blanket notable-exec corroboration arm to shapes: a bare interactive-shell/pkg-mgr `ProcessExec` stops blanket-corroborating every objective (`Behavior::ProcessExec → false` in the flat arm — symmetric with PrivilegeChange/PtraceAttach/ModuleLoad, restoring the 0011 on-call-engineer FP guard), and exec-based corroboration re-homes in an entry-scoped `reverse_shell_on_foothold` shape (interactive-shell exec + internet NetworkConnection within a symmetric 60s window, foothold-gated) — unmasking the deferred reverse-shell shape per 0024. Corrects the framing: the model never sees `corroborated` (removed from the prompt by 0034); narrowing buys deterministic auto-apply-gate precision + honest latent/live presentation + 0024 unmasking, NOT model-input precision. Safe **now** because Falco's `Alert` arm still backstops (`falco.enabled: true`), pre-arming in `audit`. One real behavior change (inert under audit): a shell-only + decisive-attack incident becomes a proposal not an auto-cut in enforce (0011-correct). Package managers excluded (always egress); `is_alarming_now`/menu seeding untouched; a narrowing-delta counter feeds the retire-Falco parity bake (0037). Amends 0009's action-bar input | Proposed | +| [0042](0042-flag-remaining-consumer-count-on-consumer-removal.md) | Flag a shared layer's remaining live-consumer count when a PR removes a consumer: a consumer-removal PR states the layer's remaining live-consumer count in one line, and a drop-to-one is flagged in review as a refactor smell rather than merged silently — the discipline PR #189's shared model-completion cache and PR #190's hypothesis-stage deletion needed (their conjunction, invisible to either single-PR review, left a redundant, `Uncertain`-pinning cache that PR #191 had to revert). Extends 0024's single-PR redundant-by-construction rule across a two-PR conjunction | Accepted | See also [`../VISION.md`](../VISION.md) for the longer-form narrative this ADR realizes.