Knowledge Wiki Orchestrata Ultimate

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

T17 — env-key drift detection in test-review.py — brief for one qwen3-coder-next run

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

Prose pending.

Plan text

T17 — env-key drift detection in test-review.py — brief for one qwen3-coder-next run

Ordered by Mohamed 2026-10-20, verbatim: "can you add a way to detect it from test review code?" — "it" = the bug class just measured and fixed in the P-CAP packet (commit 4a470482c): scripts/run_tests.py ::_child_run_env FABRICATED env keys — it stamped ORCH_TEST_DEADLINE while the observer reads TEST_DEADLINE_ENV = "ORCHESTRATA_TEST_DEADLINE", and set no ORCHESTRATA_SANCTIONED_RUNNER stamp at all. The cap mechanism was dead on arrival and the multi-file guard would have refused every batched child. Nothing mechanical caught it; a hand audit did. This packet makes test-review.py catch it.

Model-routing note (RUL-037): .py-fenced brief on qwen3-coder-next by Mohamed's session instruction (stated override; M2 not landed). Coordinator verifies before commit. Workers do not commit.

Fence — exactly these files

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

Read (READ-ONLY, for measurement — do not edit): scripts/gate.py (the review-staged subcommand — how it selects which staged files go to test-review), src/orchestrata_ultimate/prolog/commit_review.pl (the Prolog policy deciding which reviews are required/blocking), scripts/hooks/pre-commit (lines 95-145: STAGED_PY collection), and the constants block src/orchestrata_ultimate/core/threading/ pytest_live_reader.py:295-310 (the incident's authoritative constants).

The rule (T17 — env-key drift / fabrication)

Two levels, driven by a REGISTRY the check builds itself:

Registry: walk src/orchestrata_ultimate/**/*.py and scripts/*.py with stdlib ast; collect every module-level assignment (plain or annotated) whose TARGET name ends with _ENV and whose value is a string Constant → entries (constant_name, value, declaring_module, lineno). (Measured examples: TEST_DEADLINE_ENV="ORCHESTRATA_TEST_DEADLINE", SANCTIONED_RUNNER_ENV="ORCHESTRATA_SANCTIONED_RUNNER", _RUN_ID_ENV="ORCH_RUN_ID" — private names included.) Cache per review run; the walk must not re-parse per file reviewed.

T17-error (drift/fabrication): any string literal ANYWHERE in the reviewed file (ast.walk over ast.Constant where the value is str) that is SIMILAR-BUT-NOT-EQUAL to a registry value. Similarity (token-based, deterministic — no fuzzy libs): split both on _, drop empties; flag when the literal shares a run of >=2 consecutive tokens with a registry value (case-insensitive) but != it — e.g. ORCH_TEST_DEADLINE vs ORCHESTRATA_TEST_DEADLINE shares TEST+DEADLINE → error. Message must name the suspected constant: T17 possible fabricated/drifted env key 'X' — declared constant NAME='Y' at file:line; import the constant, do not retype keys.

T17-warning (hardcoded exact key): a literal EQUAL to a registry value, used in the reviewed file outside its declaring module → warning: T17 env key 'X' has a declared constant NAME at file:line — import it instead of retyping. Warnings block at pre-commit (the hook's policy: test review WARNING and up blocks) — that is intended.

Non-goals (do NOT flag): infra keys with NO declared constant (REDIS_HOST, KG_DATABASE, ...) — inline literals there are the RUL-003 house style; comments/docstrings (not ast Constants in code position — ast.walk naturally excludes comment text); f-strings' dynamic parts.

Self-exemption: the rule's OWN tests must not trip it — 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 concatenation if practical; exemption only if it isn't.

Routing measurement (report, do not change yet)

Measure and REPORT in your reply (no edits outside the fence): which staged files does gate.py review-staged hand to test-review.py today (tests only, or all staged .py)? The incident file was scripts/run_tests.py (SOURCE). If test-review only ever receives tests/**, say so explicitly and describe the minimal routing change that would get source .py files T17-reviewed (gate.py selection vs commit_review.pl policy) — the coordinator will rule on it as a follow-up packet. ALSO: inside test-review.py, make T17 apply to ANY .py file passed on the command line regardless of path (the test-standards rules T1-T16 keep their current scoping — measure how they scope today and leave that behaviour unchanged for non-test paths if they already skip, or note what happens).

Tests (in tests/scripts/test_test_review_tool.py, follow its existing patterns)

Real end-to-end runs of the reviewer over tmp_path fixture files (write fixtures with the editor tools... fixtures are created BY the tests at runtime via tmp_path — that is test data, allowed; the no-shell-write rule binds YOU, not the test code):

  1. drift literal → 1 T17 error naming the constant (registry seeded from a tmp fixture module with a FOO_ENV = "ORCHESTRATA_FOO"-style constant — check whether the reviewer's registry roots are injectable for tests; 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).
  2. exact literal outside declaring module → 1 T17 warning.
  3. correct usage (imports the constant, no literal) → 0 T17 findings.
  4. RUL-003 infra literal (os.environ.get("REDIS_HOST") with no registry entry) → 0 T17 findings.
  5. the incident regression pin: a fixture reproducing the _child_run_env shape (dict literal keyed "ORCH_" + "TEST_DEADLINE" built by concatenation at fixture-write time, next to a registry entry ORCHESTRATA_TEST_DEADLINE) → T17 error fires.
  6. declaring module itself using its own literal → 0 findings.

VERIFY (run exactly, report verbatim)

.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     # the FIXED file: must be T17-clean now
.venv/bin/ruff check scripts/test-review.py tests/scripts/test_test_review_tool.py
.venv/bin/black --check scripts/test-review.py tests/scripts/test_test_review_tool.py

Coverage: measure honestly (--cov=scripts --cov-report=term-missing | grep test-review), report the number + named gaps (session priority: finishing outranks perfect 90).

Done

Reply with: the rule implementation summary (registry walk + similarity function, exact token rule), the routing measurement answer (which files reach test-review today + the minimal source-file wiring proposal, NOT implemented), the six test names, all VERIFY outputs verbatim, coverage number + gaps, and anything you could not execute faithfully, named.