review-criteria: recalibrate severity per #2128

shared/prompts/code-review-criteria.md

@@ -39,6 +39,10 @@
 - Are there tests for new functionality?
 - Do existing tests still pass?
 - Are edge cases covered?
+- **Tests must exercise the production code path.** The following patterns are blocking:
+  - *Self-seeding goldens* — a golden fixture generated by running the implementation under test (whether on first run or after a regeneration) pins whatever output that implementation produced. Broken detector → broken golden, pinned forever; "we regenerated it after a fix" does not rescue this. Goldens must be authored against an independently verified expectation.
+  - *Hand-built fixtures that bypass the production code path* — a test that directly constructs a manifest entry, serialised payload, or cache key rather than going through the production helper does not exercise that helper. A regression there would not break the test.
+  - *Name-vs-behaviour contradictions* — e.g. a test named `test_zero_major_hard_fails` that asserts `'0.1'` is accepted. Either the name is misleading or the assertion is wrong; resolve the contradiction before merging.
 
 **Documentation**:
 - Are significant changes documented?
@@ -57,7 +61,10 @@
 
 **Blocking** (request changes):
 - Security vulnerabilities
-- Non-functional features — the feature's core purpose does not work end-to-end
+- **Non-functional features** — the feature's core purpose does not work end-to-end. Two named sub-cases, both blocking:
+  - *Single-module break* — the unit responsible for the feature is itself broken (wrong logic, missing branch, mis-wired constant). Caught by reading the file under inspection.
+  - *Cross-module silent no-op* — every individual file looks internally consistent, but the producer's output is filtered, dropped, or defaulted by a downstream consumer such that the feature does nothing in its normal path. Synthetic-key dead-ends across modules are the canonical example (one module emits a synthetic sentinel; the consumer's filter excludes it). Each unit test passes; the cross-module wiring dead-ends. Caught only by tracing data flow across the changeset.
+- **Operator-facing misconfiguration produces no signal** — patterns that mask invalid **operator-supplied** input: silent exception fallbacks (bare `except Exception:` followed by a default), `None`-on-error returns, or no-op default branches. These same patterns are often legitimate for non-operator inputs (graceful degradation of optional config, defensive nulling of internal state); the qualifier is operator-facing. The safety floor holding ("no crash, no security violation") does not make this non-blocking when the operator gets no feedback that their **deliberately set** input was ignored. Symlinks, schema errors, and denied paths a user deliberately set should fail loudly, not silently.
 - Logic errors that produce incorrect results
 - Breaking changes to existing functionality
 - Resource leaks or crashes

shared/prompts/security-review-criteria.md

@@ -115,7 +115,8 @@ bypass the gateway. **But** a compromised wrapper can still:
 The role-level write filter does **not** block writes under
 `sandbox/scripts/` — the credential-routing invariant is enforced by
 this lens, not by `patterns.py`. Treat any diff that touches
-`sandbox/scripts/*` as a trust-boundary change.
+`sandbox/scripts/*` as a trust-boundary change. Read-only file access
+of agent-supplied paths is covered separately in §8.
 
 Verification recipe:
 
@@ -174,6 +175,60 @@ A line-by-line code reviewer often misses these because the file
 under inspection looks self-consistent. The security lens runs on the
 full changeset and is the natural seam to flag them.
 
+### 8. Agent-supplied paths flowing into read-only file access
+
+The lens has historically downgraded read-only file access
+(`Path(p).read_text()`, `open(p)`, `glob(p)`) of agent-supplied paths
+because there was no shell-out and no write. That is the wrong
+threat-model. PR [#2105](https://github.com/jwbron/egg/pull/2105)'s
+`_handle_validate_repo_config` shipped this way — `reviewer_security`
+approved it as "appropriate read-only delegation," and the GHA reviewer
+correctly flagged it as path traversal. **Read access to attacker-chosen
+workspace-readable targets is a path-traversal bug class regardless of
+whether the handler also writes or shells out.**
+
+Common shapes:
+
+- An MCP-tool / route handler accepts a path argument and passes it
+  to `Path(...).read_text()` without a workspace-root prefix check.
+- A skill writes to or reads from `<repo-path>/.egg/...` where
+  `<repo-path>` is agent-supplied and unvalidated against a workspace
+  root.
+- A validator that *rejects* an unsafe path on one entrypoint, while a
+  sibling entrypoint reads the same path before validation runs.
+
+Verification recipe:
+
+1. For every changed MCP tool, route, or skill that accepts a path
+   argument, find every place that path flows into a filesystem API.
+   The list is non-exhaustive — flag any API that opens the file or
+   returns metadata about it. Common shapes:
+   - **Reads / opens**: `open()`, `Path.read_text` / `read_bytes`,
+     `shutil.copy`.
+   - **Directory enumeration**: `os.scandir`, `os.walk`, `os.listdir`,
+     `glob`, `pathlib.Path.iterdir` — leaks names of files outside the
+     workspace.
+   - **Existence / metadata oracles**: `Path.exists()`, `Path.is_file()`,
+     `Path.is_dir()`, `Path.stat()`, `os.path.exists()`,
+     `os.path.isdir()` — a `True` / `False` return based on
+     `Path(agent_path).exists()` leaks filesystem layout.
+   - **Symlink inspection**: `Path.is_symlink()`, `os.readlink()` —
+     leaks the symlink target outside the workspace.
+   - **Loaders that take a `Path`**: `yaml.safe_load(Path(p).read_text())`,
+     `json.load(open(p))`, `tomllib.load(open(p, "rb"))`,
+     `configparser.read()`.
+2. Confirm a workspace-root prefix check
+   (`p.resolve().is_relative_to(WORKSPACE_ROOT.resolve())` or
+   equivalent) runs **before** the access. `.resolve()` must run before
+   the prefix check, so symlinks and `..` segments are collapsed first;
+   checking `is_relative_to` on the unresolved `Path` lets a symlink
+   inside the workspace point outside it.
+3. NACK on any agent-supplied read of an unconstrained path — even
+   when the handler does not write, does not shell out, and does not
+   return the contents to the caller. Reading
+   `/etc/shadow` / `~/.ssh/id_rsa` / `<other-repo>/.git/config` is the
+   bug.
+
 ## How to Review
 
 1. Read the full diff once at the security lens.