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 written on the test's first run from the implementation under test pins whatever bug the first run had (broken detector → broken golden, pinned forever). 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?
@@ -58,6 +62,8 @@
 **Blocking** (request changes):
 - Security vulnerabilities
 - Non-functional features — the feature's core purpose does not work end-to-end
+- **Primary advertised use case silently no-ops** — the producer's output is filtered, dropped, or defaulted by a downstream consumer such that the feature does nothing in its normal path, while every individual file looks internally consistent. Distinct from the bullet above: each unit test passes, but the cross-module wiring dead-ends. Synthetic-key dead-ends across modules are the canonical example (one module emits a synthetic sentinel; the consumer's filter excludes it).
+- **Operator-facing misconfiguration produces no signal** — silent exception fallbacks (bare `except Exception:` followed by a default), `None`-on-error returns, or no-op default branches that mask invalid operator input. The safety floor holding ("no crash, no security violation") does not make this non-blocking when the operator gets no feedback that their 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,45 @@ 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 `open()`,
+   `Path.read_text` / `read_bytes`, `os.scandir`, `glob`,
+   `pathlib.Path.iterdir`, `shutil.copy`, etc.
+2. Confirm a workspace-root prefix check
+   (`p.resolve().is_relative_to(WORKSPACE_ROOT.resolve())` or
+   equivalent) runs **before** the access. Symlink resolution must
+   happen on the resolved path, not on the raw string the agent
+   supplied.
+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.