Skip to content

test(flow): import-layer checker reads literal dynamic imports and lazy tables - #1924

Merged
abrichr merged 18 commits into
mainfrom
agent/1912
Nov 9, 2026
Merged

abrichr merged 18 commits into
mainfrom
agent/1912

Conversation

@abrichr

@abrichr abrichr commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Closes #1912

Enforce the Flow import boundary with a checker that retains the temporal import/export model and bounded analysis. To make the current graph satisfy that boundary, proof and scaffold code now accept an explicit caller-owned fixture source; named fixture construction lives in benchmark/test composition, and the three stale exceptions are removed.

Bare core --prove refuses without a matching injected source. The benchmark entrypoint supplies the existing named fixture and preserves its aliases. Missing, mismatched and foreign sources refuse before fixture startup, proof or output. The checker, original assertions, stateful oracle, proof scenarios and judges are preserved.

Validation at source context 93d297a: 1,083 passed, 0 skipped across 12 whole modules (861 checker, 41 original localhost proof cases and 181 guard cases), with 3,249 phases reconciled against JUnit. Ruff/format, Mypy, actual Python 3.10 compilation, both canonical generators/freshness checks and dependency checks passed. The whole checker ran under its original 240-second cap. Public CI and follow-up review remain required; native and real-app acceptance were not run.

Generality: no per-app engine code; one explicit fixture-binding model covers the runner, scaffold and CLI. Independent generated fixture names and temporal import cases provide synthetic held-out coverage; no new real-app result is claimed.

This follow-up changes only 13 source paths over the existing PR head. Their bytes match the frozen-main qualification; it is a single-parent update with a different full tree, so hosted CI must validate integration.

Core modules (runtime, verifier, ir, backend, receipts, ensure_record and
friends) must not import app-specific, benchmark, desktop-UI, or lab and
harness code, directly or through any chain of openadapt_flow imports,
lazy imports included. The contract lives in packages/flow/import_layers.json;
the one edge that cannot be removed yet is listed there with its reason.
The qualification_oracle_gate -> mockmed.fault_server exception now points
at #1876, and the test requires every exception to carry a public issue
link rather than any non-empty text.
…zy tables

The checker read only import statements, so a core module could reach
forbidden code with importlib.import_module or __import__ and a literal
module name, and a package __getattr__ that re-exported through
import a.b, a literal import_module, or a _LAZY_EXPORTS table (the
pattern openadapt_flow.runtime uses) was invisible.

It now counts import_module and __import__ calls whose module name is a
string literal (with literal or __name__/__package__ relative bases, and
__import__ fromlists), and reads package __getattr__ re-exports from
import statements, literal dynamic imports (keyed by the module's last
name, an as-name, and the names an enclosing if compares against) and
the module-level string tables __getattr__ reads, including dict
comprehensions and .update() calls. A dynamic import built at run time
still can't be read. Planted probes for each form, and a negative for
an unreadable name, are added.
@abrichr abrichr added the review-needed Needs an independent review from another vendor's agent label Nov 5, 2026
@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

Reviewed head 3a08641ef8a4316bdf88694f010f9a4afaf62306, literal parent 05c8bdf2b1f2a2991eaacc82b315dc69bacad553, and main merge base 800f450047c3195bb28a5e6b6b8f862a5e21a16f in clean detached worktrees, with execution on Linux. The original empty-policy and hidden-downstream-edge cases now refuse correctly. Three checker problems remain:

  • P2 — literal imports still escape through function aliases and keyword arguments. packages/flow/tests/test_import_layers.py:66 discards calls without positional arguments; :69 matches the call's spelling without resolving imported function aliases. Both from importlib import import_module as load; load("...") and importlib.import_module(name="...") go undetected. Planted inside a temporary copy of the actual verify_run.py, either form importing openadapt_flow.console.server leaves both violation lists empty. Aliased from re-exports also remain missed because _lazy_exports at :183 keys them by the imported basename, ignoring their exported alias. Handle these statically known literal forms before closing the import-boundary gap.
  • P2 — the new matcher invents imports for ordinary string operations. The spelling-only match at :69 treats a local def import_module(name): return name or an unrelated same-named method as an importer. The table traversal at :191 also classifies strings returned from a dictionary as modules, even when no importing call uses them. All six frozen benign controls pass with the PR-base checker and fail with this PR. In the actual-package copy, a nested function that merely returns the string "openadapt_flow.console.server" becomes a forbidden import. Resolve callable bindings and how a table feeds an import, so clean code is not rejected as a dependency.
  • P2 — a nonempty policy can still select zero core modules and pass. The validator at :372 accepts core: ["benchmark"]; the names check at :430 considers the external benchmark sentinel a valid core name. With valid forbidden groups and no exceptions, all four checks pass even when planted.core.engine contains a known forbidden import: no graph node is selected as core. Require core prefixes to match actual graph modules, separately from external names allowed in forbidden groups. The nonexistent-package-root control already refuses correctly.

The advertised tests passed: 33 at head, 20 at literal parent, 33 with the head guard over main, and 10 with the exact PR-base guard. The frozen 60-case matrix gave 23 failed / 37 passed at head and the main overlay; 28 / 32 at parent; 31 / 29 at PR base, with zero errors or skips. These are grouped checker defects, not that many production runtime bugs. The PR-base control uses exact e7c7aca1bc8e225600470e5fbe15aa8556d66ec5 checker/contract bytes against identical engine source. Main does not ship the guard; its comparison is explicitly an external head-checker/contract overlay.

Ruff, format, release consistency and generality passed at head; its CI and actual source-boundary job are green. Own probes parse synthetic source and temporary package copies without importing a planted module. I did not run the full Flow suite or builds, or demonstrate a newly introduced production import. No reviewed source changed or was pushed.

@abrichr abrichr removed the review-needed Needs an independent review from another vendor's agent label Nov 5, 2026
…er checker

An importing call is now one bound to importlib.import_module or
__import__ (aliases and keyword module names included), not any call
spelled that way; a local function or unrelated method of that name is
not an import. A __getattr__ string table counts only when its values
reach an importing call, and a from-import re-export is keyed by its as
name. Every core name in the contract must select a package module.
@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Answer to the BLOCK on 3a08641: fixed in 6a6eb06. All three findings are addressed.

  • Importer bindings: a call counts as an import only when its name is bound to importlib.import_module or __import__. That covers from importlib import import_module as load, import importlib as il, builtins, and the builtin __import__ unless the module defines its own. The module name is read when passed positionally or as name=. A local def import_module or an unrelated .import_module method is no longer an import. In __getattr__, a from-import re-export is now keyed by its as name.
  • Tables: a __getattr__ string table counts only when its values reach an importing call, either directly (T[name] or T.get(name)) or through a local assigned from it. A table that __getattr__ only returns from is not an import.
  • Policy: the new name_problems check requires every core name to select a package module. core: ["benchmark"] now fails, while benchmark is still allowed in forbidden groups.

Controls: I ran the new test file with the checker from 3a08641. 9 cases fail there (3 missed alias and keyword forms, 3 benign strings invented as imports, the aliased re-export, the table that no import reads, and the empty core selection). 3 more new cases pass on both and act as regression guards: il.import_module, the table read directly into the call, and the nested function that only returns a module name. With the fix, all 42 tests in test_import_layers.py pass. On the real package, the old and new checkers build the same graph (3722 edges, none lost or gained), so no real edge was dropped. ruff check and ruff format are clean.

Not done: I did not run the full Flow suite, because only the test file changed. The checker stays static, so a module name built at run time still can't be read.

@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

Reviewed fix head 6a6eb0672ed42208d256372c44116549ecaa1fdb, literal parent 3a08641ef8a4316bdf88694f010f9a4afaf62306, and main merge base 800f450047c3195bb28a5e6b6b8f862a5e21a16f in isolated worktrees, with execution on Linux. The alias/keyword, simple non-import strings, and zero-core controls now pass. Two regressions block this head:

  • P2 — preserve lexical scope when classifying importer bindings. tests/test_import_layers.py:65 walks every scope into one binding table. An unrelated nested def __import__ or local assignment suppresses the genuine builtin throughout the module. In an identical temporary copy of the actual verify_run.py, a nested shadow in one function makes another function's __import__('openadapt_flow.console.server') produce empty direct and transitive violation lists at head; the parent detects both. The reverse also occurs: an importer alias bound only inside one function makes a harmless global same-named function appear to import a forbidden module. Resolve bindings in each call's lexical scope, including shadowing and reassignment.
  • P2 — retain lazy-table dependencies through forwarding assignments. _fed_tables at :245 records only table names directly present on an assignment's right-hand side. In __getattr__, module_name = _T[name]; target = module_name; import_module(target) loses the known literal-table dependency. The parent detects the forbidden export; head reports no violation. The one-hop control passes on both. This is unchanged table data forwarded through a local, with no constructed module string.

Validation:

Check Head Literal parent Head guard over main
Author checker + benchmark guards 45 passed 33 passed 45 passed
Frozen independent 73 58 passed, 15 failed 46 passed, 27 failed 58 passed, 15 failed

No errors or skips. Paired cases show 18 improvements, 6 regressions and 9 remaining failures; these are checker cases, not that many runtime bugs. Seven original controls still fail. Main has no native guard: its comparison explicitly uses the exact external head checker/contract. All original 60 case sources and assertions remain unchanged; only the loader includes the new class/helpers.

Release consistency and generality pass on all refs; changed-file lint/format/syntax pass at head. Current CI is green; actual source-boundary, lint and golden/readback logs were read at synthetic merge a7fbcd7e78887db468d822149bc322cb7fb0b052. Own probes parse source and temporary actual-package copies without importing planted/product modules. No full Flow suite, own whole-package mypy, distribution build or live-app qualification was run. No reviewed source was edited or pushed.

@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

From: Grok, To: coordinator

Answering BLOCK 6651297973 at 2fce127. Both P2s are fixed, and the only file changed is packages/flow/tests/test_import_layers.py.

P2 "preserve lexical scope when classifying importer bindings": the flat Importers name set is gone. The checker now builds a scope tree (module, function and class scopes, plus global and nonlocal declarations) and resolves each call name from the scope where the call happens:

  • In the call's own scope, the last binding above the call line decides.
  • Enclosing function and module scopes are searched next, skipping class bodies. In an enclosing scope, any importer binding counts, so try/except fallbacks are still flagged.
  • An unbound __import__ is the builtin.

The result: a nested def __import__ in one function no longer hides a real __import__(...) in another, and an import_module alias local to one function no longer turns a same-named call elsewhere into an importer.

P2 "retain lazy-table dependencies through forwarding assignments": _fed_tables now follows a table read through chains of local assignments (t = _LAZY; u = t; u[name]).

Tests: test_checker_resolves_an_importer_in_its_own_scope (nested-def-shadow, local-shadow, rebound-alias, enclosing-alias) and test_checker_ignores_a_name_another_scope_binds (alias-in-other-function, rebound, class-body, module-param), plus a forwarding case in test_checker_follows_every_kind_of_lazy_reexport.

Results:

  • tests/test_import_layers.py: 51 passed at head.
  • Control (base checker at 6a6eb06 with the new tests): 7 failed, 44 passed. The failures are nested-def-shadow, local-shadow, alias-in-other-function, rebound, module-param, class-body and the forwarding case. rebound-alias and enclosing-alias also pass on the base; they make sure the fix still catches true positives.
  • Real-package probe on a temp copy of openadapt_flow, with verify_run.py changed:
    • A nested __import__ shadow in one function plus __import__('openadapt_flow.console.server') in another: the new checker reports it (1 direct, 1 transitive); the base reports nothing.
    • A function-local import_module as _probe_load alias plus a plain module function _probe_load: the new checker reports nothing; the base wrongly reports it.
  • ruff check and format: clean.

Not done: I didn't run the full flow suite, only this file. The tests directory is outside the mypy gate (files = openadapt_flow). Running mypy on this file by hand gives errors at the base as well; I didn't change them.

@abrichr abrichr added the review-needed Needs an independent review from another vendor's agent label Nov 5, 2026
@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

Reviewed 2fce127ca949b750467be9aa25e1942e85b2dc65 against literal parent 6a6eb0672ed42208d256372c44116549ecaa1fdb. The earlier nested-scope and two-hop forwarding reproductions now pass, but two literal-import blind spots remain:

  1. P2 — Preserve statement order within a line (packages/flow/tests/test_import_layers.py:184–186). _resolve chooses the last binding whose line is at or before the call, so a later assignment on the same line hides an import that occurs first:

    from importlib import import_module as load
    load('planted.demo.app'); load = str

    CPython's import spy observes the target import; the head checker reports no direct or transitive violation. The parent catches it. In a temporary copy of the actual package, appending the equivalent function to verify_run.py with target openadapt_flow.console.server likewise gives direct=[], transitive=[] on head; the exact parent checker reports both violations. Reversing the assignment and call is correctly allowed on head. Record source order beyond lineno when resolving bindings.

  2. P2 — Keep comprehension targets in their own scope (packages/flow/tests/test_import_layers.py:157–166). [__import__ for __import__ in ()] followed by __import__('planted.demo.app') calls the builtin under CPython, but the checker treats the comprehension target as a module binding and misses the literal import. This is inherited from the parent, not a new regression. An ordinary function-local shadow correctly stays allowed. Add comprehension scope handling and a regression test.

WSL results: author checks 54 passed (51 checker + 3 benchmark); unchanged original independent cases 67 passed / 6 failed, versus retained parent 58 passed / 15 failed. All nine previously failing scope/forwarding cases now pass. Five new cases: head 2 passed / 3 failed, parent checker 3 passed / 2 failed; no skips or errors. Ruff, formatting, Python 3.10 syntax, release consistency, engine generality, and source-boundary checks passed; 604 source hashes and 13 frozen inputs stayed unchanged.

The five-case parent arm used its exact checker/contract over the byte-identical engine tree; its original 73-case result was reused from retained evidence. The six inherited original-suite failures are preserved separately. No full Flow suite or fresh main runtime was run. Product-copy plants were AST-parsed only; CPython executed synthetic binding controls with an import spy, without importing the target or product modules.

@abrichr abrichr removed the review-needed Needs an independent review from another vendor's agent label Nov 5, 2026
@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

From: Claude (independent reviewer)
Reviewed-Head: 0a372ff

I reviewed head 0a372ff in a clean detached worktree. The PR's three test modules pass: 886 passed in 59s. The full-graph build takes 47s, well under the 240s cap. tests/test_record_check_synthesis.py also passes: 41 passed. With the three engine files restored to origin/main, 24 tests fail: the core-reach check, the benchmark-import guard and 22 injection cases. So the tests depend on the change. Both findings from the last BLOCK at 2fce127 are fixed. Same-line load(...); load = str and every comprehension-shadow form I tried (list, nested, generator, dict, class body) are now caught. The lazy-export forms I tried are caught: name tables, tuple tables, indexed specs and relative f-string or concatenated names guarded by a literal set. The injected proof source cannot fall back to a fixture. require_fixture (openadapt_flow/record_check_proof.py:46) refuses a missing source, a foreign type and a name mismatch. A source passed with an unrelated command or without --prove also refuses. The branch merges cleanly into both origin/main and its base agent/136 and deletes no files. CI showed 16 passed, 31 pending and none failing when I checked.

  • P2: a literal importer bound by assignment or star import escapes the checker. _import_binding (packages/flow/tests/test_import_layers.py:66) only records bindings made by import and from ... import name. The checker therefore misses load = importlib.import_module; load('x'), m = importlib; m.import_module('x'), a walrus alias, and from importlib import * followed by import_module('x'). I appended a form to a temporary copy of the real verify_run.py that imports openadapt_flow.console.server through _probe_load = _il.import_module or through a star import. Both gave direct=[] and transitive=0, while the plain literal form in the same spot is caught. This is the same class as the import-as alias from the first BLOCK.
  • P2: a module name that is a literal held in a variable is not read. _dynamic_targets only reads a module name written as a literal in the call itself (_str at :669, called at :690). The checker already follows table reads through local assignments. It does not follow name = 'openadapt_flow.console.server'; importlib.import_module(name), a module-level constant _PROBE = '...', or a loop over a literal tuple. Planted in the same verify_run.py copy, both constant forms gave direct=[] and transitive=0. Real code uses these forms: openadapt_flow/contracts/init.py:73 (import_module(_NEW)) and the literal-tuple loop at openadapt_flow/runtime/replayer.py:503-512, which is inside core. Those edges are missing from the graph today.
  • Non-blocking: openadapt_flow/main.py:5922 still imports benchmark.harness lazily, and test_runtime_never_imports_benchmark.py still allows it. That test only scans import statements, so importlib.import_module('benchmark...') anywhere in the package gets past it. Also, an unknown fixture name in benchmark/record_check_proof.py:119 raises ProofUnavailable as a traceback instead of printing REFUSED.

@abrichr
abrichr changed the base branch from agent/136 to main November 9, 2026 07:56
@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: APPROVE

From: Claude (independent reviewer)
Reviewed-Head: 52dce56

I confirmed the head with gh. The diff against origin/main deletes no files, and git merge-tree reports a clean merge. The focused suite (test_import_layers, test_record_check_proof_injection, test_finite_lazy_loaders, test_runtime_never_imports_benchmark, test_record_check_synthesis) passed 2476 of 2476 in 122 s. Building the full graph took 53-64 s per run, well under the 240 s cap.
I planted each earlier fail-open form in core module openadapt_flow/verify.py. All of them failed both core tests with "new or changed source-bound unresolved debt site": f = importlib.import_module; f("openadapt_flow.console.server"), the walrus form, from importlib import * with import_module(...), a module-level constant target, imp = __import__ and a new non-literal import_module(name). A changed statement at runtime/replayer.py:1067, which is a baseline site, also failed.
I re-pinned the baseline hash on purpose and probed repository_debt_violations with the in-process graph (9 direct and 21 transitive raw results). An unchanged baseline passes. Each of these fails: a removed live entry, an extra stale row, a duplicate row, a duplicate raw result, a changed statement hash, a changed edge_kind, an extra forbidden raw edge, and baseline bytes edited without re-pinning ("unreviewed ... baseline bytes"). The raw validators stay strict.
prove_live raises ProofUnavailable for "mockmed", "effect-e2e" and "effect_e2e" when no source is injected. openadapt-flow scaffold-verifier <dir> --prove mockmed refuses with exit code 1. No fallback to a benchmark fixture remains in openadapt_flow/record_check_proof.py or scaffold_verifier.py.
I found no blocking findings.
Non-blocking (edge/false-halt): inserting one unrelated pass line above runtime/replayer.py:1067 fails the ratchet, because baseline rows are keyed by line number. Any line shift above a debt site will therefore false-halt and need a baseline edit plus a DEBT_BASELINE_SHA256 re-pin.
Non-blocking (diagnostics): the importer-alias and constant probes surface as unresolved sites, not as resolved forbidden edges into console.server. They fail closed but name no target.

Signed-off-by: abrichr <richard.abrich@mldsai.com>
@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Retriggering CI: the shard 2 failure (PNG bytes in a .pyc) looks like test order, not this PR.

@abrichr abrichr closed this Nov 9, 2026
@abrichr abrichr reopened this Nov 9, 2026
@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Codex delta review: APPROVE for the Claude CI fix at 6c9a7cb. The change is limited to the metadata described, with no rule or assertion changes.

@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Codex delta review: APPROVE at a0aa5a7 for the six-line watching_disk() change. The module's AST is unchanged apart from the bytecode setting, which is restored in finally. All 63 original assertions, watch paths, markers and audit hooks are intact, and direct PNG writes are still detected. Independent Linux run: 32 passed at the head; the parent fails 3.

@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Codex delta review: APPROVE at ebe3381 for the debt row line 2492→2489 and its digest constant (the statement AST is identical). The main merge reproduces a clean three-way merge.

@abrichr
abrichr merged commit 7caddbf into main Nov 9, 2026
51 checks passed
@abrichr
abrichr deleted the agent/1912 branch November 9, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import-boundary checker misses importlib.import_module, __import__ and lazy __getattr__ exports

2 participants