Orchestrata Ultimate › Plans › briefs › t17-env-key-drift-run2

T17 packet, run 2 — REWORK: env-key drift rule in test-review.py (run 1 REJECTED)

Source: plans/briefs/t17-env-key-drift-run2.md at 8a2cb42ad · last changed 2026-10-20 · first committed 2026-10-20 · 159 lines.

Prose pending.

Plan text

T17 packet, run 2 — REWORK: env-key drift rule in test-review.py (run 1 REJECTED)

RUN-3 STATUS NOTE (2026-10-20, coordinator): the first run-2 attempt died on a provider error mid-flight after reverting scripts/test-review.py to HEAD and writing two out-of-fence codemod scripts (scripts/apply_fixes.py, scripts/apply_t17_run2_fixes.py — RUL-037 violations, deleted). The coordinator restored the whole fence to HEAD: the tree now contains NO run-1 code. Run 3 is a CLEAN-SLATE implementation from the two briefs. The defect corpus below describes the rejected run-1 implementation — read each item as a PROHIBITION (do not reproduce) plus its positive requirement. Run-1's test-file diff is preserved for reference at /tmp/opencode/t17-run1-testfile-diff.patch. Implement directly with the editor tools — the scope is one method cluster + one rename + eight tests + six literal substitutions; codemod scripts are forbidden (RUL-037) and were how run 2 died.

Run 1 (worker session ses_ea85f10e7ffeH8o0qxAEqgDGJ6, interrupted at the coverage step) is REJECTED by coordinator verification — nothing was committed. The original contract stands: plans/briefs/t17-env-key-drift.md (read it first; everything not named below is unchanged). This file is the defect corpus (L12) with measured evidence for every item.

Fence — exactly these files (run_tests.py ADDED, scope-limited)

scripts/test-review.py
tests/scripts/test_test_review_tool.py
scripts/run_tests.py

scripts/run_tests.py scope: ONLY the D7 replacements below + the imports they need. Nothing else in that file changes. It is the promotion test verb — no semantic edits.

Defect corpus (each measured by the coordinator on run-1's tree)

D1 — ERROR, contract violation: unauthorized skip filters gut the rule. Run 1 added (a) an uppercase-alpha skip with a HARDCODED pattern allowlist (TEST_DEADLINE, WORKER_SLOT, RUN_ID, STATE, LANE, ...), (b) a "\n" in literal or len > 200 skip, (c) a test_/.py filename-shaped skip. None are in the brief's non-goals (which allow ONLY: infra keys with no declared constant, comments/docstrings, f-string dynamic parts). Measured false negatives against REAL registry entries (probes run by the coordinator, tmp files reviewed with run-1 code):

The allowlist only protects the incident's constants; every future *_ENV constant is silently unpoliced. FIX: delete filters (a)+(b)+(c) and the pattern list. Principled skips that MAY stay: literals ending _ENV (constant-NAME-shaped strings, not key values — document why), and the brief's non-goals. Handle the rule's own fixture literals via D6, not via global filters.

D2 — ERROR, contract violation: T17 must apply to ANY .py passed on the CLI. Brief §"Routing measurement", last paragraph: "inside test-review.py, make T17 apply to ANY .py file passed on the command line regardless of path". Measured: test-review.py scripts/run_tests.py --stdout → No test files found. — find_test_files (line 204) rejects any file not named test_*.py/*_test.py, so the brief's VERIFY #3 was a VACUOUS pass (rc=0, zero review). FIX: the env-key rule runs on every .py file argument (and every .py under a directory argument stays scoped as today — measure and report what directories do); T1-T20 keep their current name-based scoping unchanged. VERIFY #3 must produce a real report.

D3 — ERROR, contract violation: the registry-roots seam is not wired. Brief test #1: "if they are hardcoded to src/+scripts/, add a seam parameter (default = current roots) so tests can point the registry at a fixture dir — that seam lives in test-review.py, inside your fence." Run 1 added roots= to _build_env_registry but _check_env_key_drift always calls it argument-less; no test can inject a fixture registry. _make_registry_fixture() (writes registry_fixture.py with FOO_ENV = "ORCHESTRATA_FOO") is DECORATIVE — called by 3 tests, read by none; those tests pass only because the real tree happens to declare the same constants. FIX: wire the seam through the review entry point the tests use, make the fixture actually seed the registry in the mechanism tests, and keep the incident regression (test #5) pinned to the REAL registry (ORCH_TEST_DEADLINE vs TEST_DEADLINE_ENV) — that coupling is the point of a pin.

D4 — ERROR, contract violation: brief test #6 missing, replaced by a false claim. Brief test #6: "declaring module itself using its own literal → 0 findings." Run 1 deleted it and left a NOTE claiming it is "not testable in this setup because the registry is built from src/orchestrata_ultimate and scripts, while tests run against files in tmp_path." That is false — it is exactly what the D3 seam makes testable (seed the registry from a fixture dir, review the fixture module itself). FIX: implement test #6 via the seam; delete the NOTE.

D5 — WARNING, hygiene: dead code. _in_assert_or_compare (test-review.py:1464) always returns False, is never called, and its docstring admits "This is a simplification". Delete it.

D6 — WARNING, brief deviation: self-exemption via unauthorized mechanism. Run 1's own tests avoid self-tripping only through the D1(b) multi-line filter (fixture literals live inside \n-containing content strings). The brief prescribes: build fixture literals by concatenation ("ORCH_" + "TEST_DEADLINE") OR add ONE documented path-exemption for tests/scripts/test_test_review_tool.py with a comment naming why. Choose ONE (with D1's filters gone, concatenation across 6+ tests is likely impractical — the exemption is authorized for exactly that case), document it at the exemption site, and prove it: after the rework, test-review.py tests/scripts/test_test_review_tool.py --stdout must be clean BECAUSE of the chosen mechanism, not because of a filter.

D7 — ERROR, measured premise fix: scripts/run_tests.py is NOT exact-literal clean. The run-1 brief asserted VERIFY #3 "must be T17-clean" without measuring. Measured: run_tests.py inlines exact registry values at 6 sites — "ORCH_RUN_ID" at lines 862, 1257 (inside _child_run_env, which already imports TEST_DEADLINE_ENV/SANCTIONED_RUNNER_ENV from pytest_live_reader), 1629, 1648, 1695; "ORCH_HEAD_COMMIT" at 868. Declaring constants: _RUN_ID_ENV / _HEAD_COMMIT_ENV at src/orchestrata_ultimate/core/threading/pytest_live_observer.py:282-283. FIX (this is why run_tests.py is in the fence): import the two constants from their declaring module and replace the 6 literals — value-identical substitution, zero semantic change. The P-CAP contract tests (committed 4a470482c) pin _child_run_env's keys; they must stay green unchanged.

D8 — rule id rename: T17_env → T21. T17 already exists in the tool (Memgraph chunk_label teardown) and T20 exists (anyio); T17_env falsely implies a relationship to T17. T21 is the next free id (measured: no T21 in the tool or its tests). Rename the rule id everywhere in the fence (RULES dict, _add calls, tests). The PACKET keeps its name (T17) in plans/docs; the RULE is T21. Messages keep the brief's wording.

D9 — ERROR, performance regression: every test pays a full-tree registry walk. Measured: run 1's own -k "t17" run took 18.88s for 9 tests (~2.1s/test); the coordinator's full-file run took minutes (126 tests) where this file historically ran in seconds. Cause: _check_env_key_drift builds the registry (ast.parse over ALL of src/orchestrata_ultimate + scripts) once per TestFileChecker INSTANCE, and the test file's shared _check helper creates a fresh instance per test — so every test in the file, including all pre-existing non-env-key tests, pays the walk. The brief's "cache per review run" meant one walk per CLI invocation; that holds for production but the test-side cost is unacceptable. FIX (all inside the fence): give the checker a constructor-level registry-roots seam (or equivalent) and have _check in the test file inject a MINIMAL fixture root by default; only the real-registry pins (tests 5 and 7) pass the real roots and pay one walk each. Report the file's wall time before/after in your reply (for run 3: "before" = the HEAD-baseline run you measure BEFORE your first edit; target: after ≈ before, seconds not minutes).

Tests (the brief's six, adjusted for the seam)

1. drift literal vs FIXTURE registry (seam-injected, e.g. BAR_BAZ_ENV = "ORCHESTRATA_BAR_BAZ" + literal "ORCH_BAR_BAZ" → shares BAR+BAZ run) → 1 ERROR naming the constant.
2. exact literal outside declaring module (fixture registry) → 1 WARNING.
3. correct usage (imports the constant, no literal) → 0 findings.
4. RUL-003 infra literal (REDIS_HOST etc., no registry entry) → 0 findings.
5. incident regression vs the REAL registry: "ORCH_TEST_DEADLINE" → ERROR naming TEST_DEADLINE_ENV (keep run 1's shape; it was correct).
6. declaring module itself using its own literal → 0 findings (via the seam).
7. NEW pins for the D1 false negatives vs the REAL registry: "PYTEST_DURATIONS_FILE" → ERROR; "PYTEST_DURATIONS_PATH" in a tmp file → WARNING (these two constants exposed the allowlist defect; pinning them prevents its return).
8. NEW pin for D2: reviewing a non-test-named .py (write plain_module.py into tmp_path, pass its path) → the env-key rule runs (a planted drift literal in it → ERROR), while T1-T20 produce nothing for it (their scoping unchanged — report what you measure).

VERIFY (run exactly, report verbatim — all from the repo root)

.venv/bin/pytest tests/scripts/test_test_review_tool.py -o addopts="" -q
.venv/bin/python scripts/test-review.py tests/scripts/test_test_review_tool.py --stdout
.venv/bin/python scripts/test-review.py scripts/run_tests.py --stdout        # real report, 0 findings (D2+D7)
.venv/bin/python scripts/test-review.py scripts/gate.py --stdout             # any non-test .py: real report, not "No test files found"
.venv/bin/ruff check scripts/test-review.py tests/scripts/test_test_review_tool.py scripts/run_tests.py
.venv/bin/black --check scripts/test-review.py tests/scripts/test_test_review_tool.py scripts/run_tests.py
.venv/bin/pytest tests/scripts/test_run_tests_cap.py -o addopts="" -q        # P-CAP contract pins still green after D7 (if that file is named differently, find the cap contract tests and run them; name what you ran)

Coverage (the step run 1 never reached): measure honestly (--cov=scripts --cov-report=term-missing | grep test-review), report the number + named gaps. Session priority ruling: finishing outranks perfect 90 — honest numbers, never padding.

Done

Reply with: what changed per defect (D1-D8), the eight test names, all VERIFY outputs verbatim, coverage number + gaps, and anything you could not execute faithfully, named. Workers do not commit; do not touch anything outside the fence; no python -c/heredoc Python (RUL-037) — the coordinator re-runs everything before commit.

Related

• Plans
• Orchestrata Ultimate — project index
• Workflow
• Tasks