review: hand off out-of-lane observations instead of dropping them

.changeset/review-out-of-lane.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+Out-of-lane observations are handed off, never dropped. Observed on the review-v1.4.0 re-run lifecycle: the skill-auditor found a real eventual-consistency bug (dedup reads via a Query immediately after PutMulti) and discarded it, verbatim "that's a correctness concern, not a quotable skill-rule violation, so I'll leave it"; correct under quote-the-rule, but the observation died and the defect shipped unflagged. The skill-auditor and every specialist lens may now return `out_of_lane_observations[]` (path, optional line, the concern stated concretely, a required concrete `failure_scenario`, optional `suggested_lane`) for real concerns their own mandate does not let them report. The orchestrator converts each one into a candidate comment with the code-assigned label `question (non-blocking)` (a handoff is not a vetted finding and can never block on its own; the claim-validator never upgrades severity) and routes it through the identical provenance gate, scope filter, claims.json, and validation path as every other candidate. The shape is validated by the new `validateOutOfLaneObservation` in `lib/finding-schema.ts` (a sibling type, so `FINDING_SCHEMA_VERSION` stays 2: no serialized finding is invalidated).

workflows/review/README.md

@@ -37,6 +37,11 @@ read-only **sub-agents** (it makes every GitHub and comment call itself):
    blocking first, with the resolved count, and an approval that resolved the last
    open threads states that every prior thread is resolved — resolving some threads
    never leaves the rest silently open.
+   A reviewer that surfaces a real concern its own mandate does not let it report — a
+   correctness problem the skill-auditor cannot quote a rule for, or something outside
+   a specialist lens's domain — hands it off as an `out_of_lane_observations[]` entry
+   instead of dropping it; the orchestrator routes each one into claim validation as a
+   non-blocking candidate (label code-assigned, so a handoff can never block on its own).
 3. If those reviewers proposed any comments, **`claim-validator`** re-checks each one
    against the actual code (attacking the finding's stated failure scenario) and,
    for best-practice claims, against the relevant skill's

workflows/review/eval/corpus/live.ts

@@ -72,7 +72,19 @@ export type LiveDefectSpec = {
  */
 export type CaseLive = {
     prContext: LivePrContext;
-    /** Case-dir-relative path to the post-change tree (default `tree`). */
+    /**
+     * Case-dir-relative path to the post-change tree (default `tree`).
+     *
+     * Trees are deliberately minimal: validation requires only the changed
+     * files to exist. The staged copy of this tree is the sub-agent's whole
+     * world (its cwd, with no network), so the agent WILL often try to read
+     * an imported module or caller that is not there; that read returns an
+     * ordinary not-found tool error the agent tolerates and works around,
+     * not a run failure. The cost of a missing file is realism, not a crash:
+     * include the context files whose absence would change what a reviewer
+     * concludes (e.g. the decorator module a sibling handler imports), and
+     * nothing more.
+     */
     tree: string;
     /** Labeled defects a live run must catch. */
     mustCatchSpecs?: LiveDefectSpec[];

workflows/review/lib/finding-schema.test.ts

@@ -10,6 +10,8 @@ import {
     validateFinding,
     isValidFinding,
     assertFinding,
+    validateOutOfLaneObservation,
+    isValidOutOfLaneObservation,
 } from "./finding-schema.ts";
 
 /**
@@ -384,3 +386,89 @@ describe("assertFinding", () => {
         }
     });
 });
+
+describe("validateOutOfLaneObservation", () => {
+    const makeValidObservation = (
+        overrides: Record<string, unknown> = {},
+    ): Record<string, unknown> => ({
+        path: "services/foo/dedup.go",
+        line: 42,
+        observation:
+            "Dedup reads via an eventually-consistent Query immediately after PutMulti.",
+        failure_scenario:
+            "Two rapid submissions race the index; the second Query misses the first PutMulti and both rows persist.",
+        suggested_lane: "correctness",
+        ...overrides,
+    });
+
+    it("accepts a well-formed observation", () => {
+        const result = validateOutOfLaneObservation(makeValidObservation());
+        expect(result.ok).toBe(true);
+    });
+
+    it("accepts the minimal shape (no line, no suggested_lane)", () => {
+        const minimal = makeValidObservation();
+        delete minimal["line"];
+        delete minimal["suggested_lane"];
+        expect(validateOutOfLaneObservation(minimal).ok).toBe(true);
+    });
+
+    it("rejects a non-object", () => {
+        const result = validateOutOfLaneObservation("nope");
+        expect(result.ok).toBe(false);
+        if (!result.ok) {
+            expect(result.errors).toEqual(["observation: must be an object"]);
+        }
+    });
+
+    it("requires path, observation, and failure_scenario", () => {
+        const result = validateOutOfLaneObservation({});
+        expect(result.ok).toBe(false);
+        if (!result.ok) {
+            expect(result.errors).toContain("path: required non-empty string");
+            expect(result.errors).toContain(
+                "observation: required non-empty string",
+            );
+            expect(result.errors).toContain(
+                "failure_scenario: required non-empty string",
+            );
+        }
+    });
+
+    it("collects every violation rather than failing on the first", () => {
+        const result = validateOutOfLaneObservation(
+            makeValidObservation({
+                path: "",
+                line: 0,
+                observation: "",
+                suggested_lane: "",
+            }),
+        );
+        expect(result.ok).toBe(false);
+        if (!result.ok) {
+            expect(result.errors.length).toBe(4);
+        }
+    });
+
+    it("rejects a non-integer or non-positive line when present", () => {
+        expect(
+            validateOutOfLaneObservation(makeValidObservation({line: 1.5})).ok,
+        ).toBe(false);
+        expect(
+            validateOutOfLaneObservation(makeValidObservation({line: -3})).ok,
+        ).toBe(false);
+    });
+});
+
+describe("isValidOutOfLaneObservation", () => {
+    it("narrows well-formed input and rejects malformed input", () => {
+        expect(
+            isValidOutOfLaneObservation({
+                path: "a.go",
+                observation: "A real concern.",
+                failure_scenario: "Input X produces wrong output Y.",
+            }),
+        ).toBe(true);
+        expect(isValidOutOfLaneObservation({path: "a.go"})).toBe(false);
+    });
+});

workflows/review/lib/finding-schema.ts

@@ -170,6 +170,46 @@ export type Finding = {
     model_authored_prose: string;
 };
 
+/**
+ * An out-of-lane observation: a real concern a reviewer surfaced that its own
+ * mandate does not let it report as a finding — e.g. the skill-auditor notices
+ * a correctness problem while checking a rule, but quote-the-rule rightly
+ * forbids reporting it as a skill violation. Production motivation: on the
+ * review-v1.4.0 re-run lifecycle the skill-auditor found a real
+ * eventual-consistency bug (dedup reads via a Query immediately after
+ * PutMulti) and dropped it — "that's a correctness concern, not a quotable
+ * skill-rule violation, so I'll leave it". Correct per quote-the-rule, but the
+ * observation died.
+ *
+ * The skill-auditor and every specialist lens may return these alongside
+ * `findings[]`; the orchestrator routes each one into claim validation as a
+ * non-blocking candidate (label code-assigned, never model-chosen), so a
+ * declined-as-out-of-lane observation is validated and surfaced rather than
+ * discarded. An observation is a handoff, not a vetted finding: it can never
+ * block on its own.
+ */
+export type OutOfLaneObservation = {
+    /** Repo-relative path the observation anchors on. */
+    path: string;
+    /** RIGHT-side diff line, when the concern is line-specific. */
+    line?: number;
+    /** One sentence: the concern, stated concretely. Model-authored. */
+    observation: string;
+    /**
+     * One sentence: the concrete inputs/state and the wrong outcome they
+     * produce — the claim the claim-validator attacks, exactly as on a
+     * finding. An observation whose scenario cannot be stated concretely is
+     * not worth handing off.
+     */
+    failure_scenario: string;
+    /** The lane the producer thinks owns this, e.g. `correctness`. Optional. */
+    suggested_lane?: string;
+};
+
+export type ObservationValidationResult =
+    | {ok: true; observation: OutOfLaneObservation}
+    | {ok: false; errors: string[]};
+
 export type ValidationResult =
     | {ok: true; finding: Finding}
     | {ok: false; errors: string[]};
@@ -328,6 +368,60 @@ export const validateFinding = (input: unknown): ValidationResult => {
     return {ok: true, finding: input as Finding};
 };
 
+/**
+ * Validate an untrusted value against the out-of-lane observation shape.
+ * Same error-collecting contract as {@link validateFinding}: every problem is
+ * returned, so a producer's malformed handoff is diagnosable from the run
+ * artifact rather than silently dropped.
+ */
+export const validateOutOfLaneObservation = (
+    input: unknown,
+): ObservationValidationResult => {
+    const errors: string[] = [];
+
+    if (!isRecord(input)) {
+        return {ok: false, errors: ["observation: must be an object"]};
+    }
+
+    if (!isNonEmptyString(input["path"])) {
+        errors.push("path: required non-empty string");
+    }
+
+    const line = input["line"];
+    if (
+        line !== undefined &&
+        (!Number.isInteger(line) || (line as number) < 1)
+    ) {
+        errors.push("line: must be a positive integer when present");
+    }
+
+    if (!isNonEmptyString(input["observation"])) {
+        errors.push("observation: required non-empty string");
+    }
+
+    if (!isNonEmptyString(input["failure_scenario"])) {
+        errors.push("failure_scenario: required non-empty string");
+    }
+
+    if (
+        input["suggested_lane"] !== undefined &&
+        !isNonEmptyString(input["suggested_lane"])
+    ) {
+        errors.push("suggested_lane: must be a non-empty string when present");
+    }
+
+    if (errors.length > 0) {
+        return {ok: false, errors};
+    }
+
+    return {ok: true, observation: input as OutOfLaneObservation};
+};
+
+/** Narrowing boolean wrapper around {@link validateOutOfLaneObservation}. */
+export const isValidOutOfLaneObservation = (
+    input: unknown,
+): input is OutOfLaneObservation => validateOutOfLaneObservation(input).ok;
+
 /** Narrowing boolean wrapper around {@link validateFinding}. */
 export const isValidFinding = (input: unknown): input is Finding =>
     validateFinding(input).ok;