ci: pin the provenance filters on the writer that actually runs

[jason931225]

G008 carried twelve pins on the staging script and none on roster.rs. That was right while the script was the only writer of payroll_draft_lines. It stopped being right at #846 — roster::materialise_roster_in_tx now runs on every payroll.create_run, and the pinned file is no longer on the production path.

So the mechanical proof of "what may become a payroll roster" sat entirely on one encoding while a second encoding did the actual work. Nothing stopped the two drifting, and G008 would have stayed green while the executed one weakened.

The tests from #846 do bind these predicates — but from the outside. A text pin is what makes a reviewer see the divergence in the diff.

Six pins, each mutation-proven against the writer

mutation	result
drop run.status = 'APPLIED'	exit 1
drop r.row_status <> 'ERROR'	exit 1
equality → overlap on the period	exit 1
weaken one of four non-blank flags	exit 1
reintroduce `?	array`
add a reconciliation DELETE	exit 1
restored	exit 0, 29 checks

The non-blank pin is counted, not includes — a plain includes passes while three of the four flags are weakened. That's the same hole I shipped once already in #836 and had to fix.

Deliberately not re-pointed from the script

Two of the script's pins would be actively harmful here. requireMatches(/raw_row\?\|array\[/) asserts the key-presence idiom — the exact fabrication vector where a blank 출근 cell counts as attendance material. Re-pointing it at the writer would make CI require the bug. The writer is pinned on the non-blank form instead, plus the absence of ?|array.

The DELETE pin isn't stylistic: 0222 revoked DELETE on this table from console_rt and asserts the revocation, so a reconciliation delete raises 42501 at plan time — killing every payroll.create_run, not just the re-stage that introduced it.

The script keeps its twelve pins

It's still the operational hand-run path. Retiring it is a separate change, and not a trivial one: reducing it to a read-only query would leave all twelve pins passing over a file that writes nothing — a false green rather than a removal.

🤖 Generated with Claude Code

[jason931225]

Anvil Adversarial Pull Request Review — PR #848

Verdict: REQUEST_CHANGES
Target: scripts/check-g008-payroll-readiness.mjs (Base: main, Head: d3d19cc5781c618563b7016c77e6a3b057184f7f)

---

Executive Overview

PR #848 addresses a real and critical architectural gap: following PR #846, payroll_draft_lines is actively written in production by roster::materialise_roster_in_tx (backend/crates/payroll/adapter-postgres/src/roster.rs), while G008's mechanical pins remained anchored entirely to the legacy staging script. This created a dual-encoding hazard where production roster generation could weaken while CI remained falsely green.

However, while the intent of pinning roster.rs is fundamentally sound, the implementation of the guardrails contains severe adversarial bypass vulnerabilities and brittle query couplings:

1. Negative Assertion Bypass (?|array): The check requireNotIncludes(rosterWriter, "?|array", ...) uses exact substring matching. Standard Postgres SQL formatting with whitespace (raw_row ?| array[...] or raw_row ?| ARRAY[...]) will completely bypass this assertion, rendering the sentinel blind to reintroductions of the key-presence fabrication bug.
2. Case & Multi-line SQL Bypass (DELETE FROM): Substring pin "DELETE FROM payroll_draft_lines" is bypassed by multi-line formatting (DELETE\nFROM) or lowercase delete from, failing to prevent 42501 plan-time crashes.
3. Brittle Positional Parameter Coupling ($3): Pinning run.pay_period_start = $3 couples CI to arbitrary parameter ordering in Rust query construction rather than the semantic equality constraint.

---

16-Lens Canonical Reasoning Matrix

Lens	Classification	Adversarial Analysis & Findings
1. Cartesian Doubt	FAILED	Assumes raw substring matching effectively enforces semantic negative constraints on SQL in Rust. In reality, whitespace and casing variations create false-negative security escapes.
2. Essentialism / YAGNI	PASSED	Minimal, zero-dependency additions to the existing G008 test harness. No unnecessary abstractions introduced.
3. Chesterton's Fence	PASSED	Correctly retains the 12 staging script pins for the operational hand-run path while establishing dedicated pins for the new production writer path from #846.
4. Contrarian / 10x	OBSERVATION	Long-term, query validation should be enforced via sqlx compile-time query verification and Cedar authorization policies rather than regex source-scraping.
5. Socratic	FAILED	Why is $3 pinned as a literal index? If a developer prepends a tenant or run filter, why should period equality validation fail?
6. Pragmatism	PASSED	Text pins in pre-commit/CI provide immediate, sub-second reviewer visibility during PR diffs without heavy DB harness overhead.
7. Red Team	FAILED	An attacker/developer can re-introduce the key-presence bug via `raw_row ?
8. Systems Thinking	CONCERN	Dual-path reality: the staging script still permits `?
9. Operability / Day-2	PASSED	Assertion failure messages are descriptive, actionable, and explain the underlying invariant failure clearly.
10. Opportunity Cost	PASSED	Fixing regex robustness now takes trivial effort and prevents future false CI breakages during legitimate query refactorings.
11. Blast-radius	PASSED	Scope is strictly confined to local CI/pre-commit test execution (check:g008-payroll-readiness).
12. Constant-work	PASSED	Static string and regex checks execute in microsecond time over single source files.
13. Shared-nothing	PASSED	Stateless, self-contained CI gate execution.
14. FinOps / Unit-cost	PASSED	Zero compute overhead or cloud egress impact.
15. Telemetry-first	N/A	Static linting / CI gate; runtime telemetry not applicable.
16. Zero-trust / Defense-in-depth	FAILED	Negative security pins do not defend against standard formatting permutations (whitespace, case sensitivity). Defense-in-depth is compromised by brittle patterns.

---

Critical Risks & Required Remediation

1. Replace Substring Negative Checks with Case-Insensitive, Whitespace-Agnostic Regexes:
   • For ?|array: Pin against ?| entirely (requireNotIncludes(rosterWriter, "?|", ...)) or use regex requireNotMatches(rosterWriter, /\?\|\s*array/i, ...). In PostgreSQL JSONB, ?| has no legitimate place in roster.rs if existence-only checks are forbidden.
   • For DELETE: Use requireNotMatches(rosterWriter, /delete\s+from\s+payroll_draft_lines/i, ...).
2. Decouple Parameter Indexing:
   • Change run.pay_period_start = $3 to a parameterized equality regex: requireMatches(rosterWriter, /run\.pay_period_start\s*=\s*\$\d+/, ...).

Findings not addressable in the diff

• scripts/check-g008-payroll-readiness.mjs:172 [RIGHT] -- diff unavailable or unparseable: ### Brittle Positional Parameter Index Pin

Pinning the literal parameter $3 (run.pay_period_start = $3) tightly couples this invariant check to the arbitrary parameter ordering of the SQL query in roster.rs. If a developer refactors the query (e.g. adding a preceding parameter like tenant_id = $1 or reordering bindings), G008 will fail even though the pay period equality invariant remains strictly enforced.

Recommendation:
Use regex matching to enforce parameter equality without coupling to positional index:

requireMatches(
  rosterWriter,
  /run\.pay_period_start\s*=\s*\$\d+/,
  "production roster writer scopes by the declared pay period, by equality",
);

• scripts/check-g008-payroll-readiness.mjs:184 [RIGHT] -- diff unavailable or unparseable: ### Critical Security / Sentinel Bypass: ?|array Negative Pin

requireNotIncludes(rosterWriter, "?|array", ...) performs a literal substring check with no whitespace tolerance and case sensitivity. Standard SQL formatting frequently includes whitespace (e.g. raw_row ?| array[...] or raw_row ?| ARRAY[...]).

Because "raw_row ?| array[".includes("?|array") evaluates to false, any reintroduction of the key-presence fabrication vector with standard spacing will completely evade G008, creating a dangerous false sense of security in CI.

Recommendation:
Forbid the ?| operator entirely or match with whitespace and case insensitivity:

requireNotIncludes(
  rosterWriter,
  "?|",
  "production roster writer never uses key-presence operator ?|",
);

Or (if requireNotMatches is available/implemented):

requireNotMatches(
  rosterWriter,
  /\?\|\s*array/i,
  "production roster writer never treats a merely PRESENT column as material",
);

• scripts/check-g008-payroll-readiness.mjs:192 [RIGHT] -- diff unavailable or unparseable: ### Formatting & Case Sensitivity Bypass on DELETE FROM

Checking for the exact substring "DELETE FROM payroll_draft_lines" can be bypassed by multi-line SQL formatting (e.g. DELETE\nFROM payroll_draft_lines) or lowercase keywords (delete from payroll_draft_lines).

Since console_rt has DELETE revoked (triggering a 42501 plan-time failure on payroll.create_run), this negative assertion must be resilient to formatting variance.

Recommendation:
Use a whitespace-agnostic and case-insensitive check:

requireNotMatches(
  rosterWriter,
  /delete\s+from\s+payroll_draft_lines/i,
  "production roster writer never deletes: console_rt holds no DELETE on this table",
);

---

🤖 [Reviewed] by Oyatie Anvil

[jason931225]

❌ Blocked — 7 finding(s) across 68 gates; 6 gate(s) produced no measurement.

• coverage — not measured: the synchronous entry point cannot run a coverage tool; call CoverageGuard::measure_diff_coverage
  • fix: add tests covering the lines this PR adds
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• slo — not measured: no Prometheus or OpenTelemetry endpoint is configured, so error budget consumption over any window was never queried
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• cluster-audit — not measured: no Kubernetes API or ArgoCD cluster access is configured, so no live state was read back and no comparison against Git was performed
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• ci-wallclock — not measured: no GitHub Actions workflow-run timing API access is configured, so neither this PR's CI duration nor its billable compute was read
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• remote-cache — not measured: no sccache or Buck2 CAS statistics endpoint is configured, so no cache hit rate was read and no lockfile was hashed
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• shadow-traffic — not measured: no traffic mirror and no replay target are configured, so no production requests were sampled and no responses were compared
  • note: this gate is aspirational fidelity and does not fully measure what its name implies
• schema-evolution — failed: Detected 1 breaking wire schema changes or tag renumberings.

---

🤖 [Blocked] by Oyatie Anvil