test(devtools): guard proofs with production reachability - #3913
Conversation
Problem: a proof test can call a helper that is no longer on the production route and still certify archive behavior. What changed: add an AST import/call graph oracle with structured seam metadata, a red unreachable-helper fixture, and bindings for the three reindex replay/convergence proof tests. The bindings require the real rebuild entrypoint to reach raw replay and terminal insight convergence. Compatibility/migration: verification-only. No production code or archive state changes. Ref polylogue-4v2d3 Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded an AST-based production reachability checker with structured diagnostics. Added fixture tests for reachable and unreachable symbols. Integrated seam checks into selected rebuild tests. ChangesProduction Reachability Verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestSeam
participant check_production_seam
participant ASTCallGraph
participant ProductionReachabilityReport
TestSeam->>check_production_seam: submit ProductionSeamSpec
check_production_seam->>ASTCallGraph: load production and test sources
ASTCallGraph-->>check_production_seam: return reachable symbols
check_production_seam->>ProductionReachabilityReport: record seam violations
ProductionReachabilityReport-->>TestSeam: return validation status and JSON diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@circleci run |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/production_reachability.py`:
- Around line 110-130: Update _index_edges to traverse only the current function
scope, excluding nested FunctionDef, AsyncFunctionDef, and lambda bodies when
collecting calls for each enclosing function. Preserve calls made directly by
the current function and add a regression fixture where an uninvoked nested
function calls a live helper without creating an edge from the enclosing
function.
- Around line 171-191: Preserve the package-initializer state from _ParsedModule
and pass it into the relative-import resolution flow. Update
_resolve_relative_module and its callers, including _imports_from_nodes, so
level-one imports from package __init__.py files retain the package name (for
example, package.child), while existing relative-import behavior for regular
modules remains unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e05b2520-e7fb-4172-9601-eed5f4f42493
📒 Files selected for processing (7)
.beads/issues.jsonldevtools/production_reachability.pytests/fixtures/production_reachability/__init__.pytests/fixtures/production_reachability/fixture_test.pytests/fixtures/production_reachability/routes.pytests/unit/devtools/test_production_reachability.pytests/unit/maintenance/test_rebuild_parse_apply_split.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1d2871cd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| references = [call.func, *call.args, *(keyword.value for keyword in call.keywords)] | ||
| for reference in references: | ||
| for expression in ast.walk(reference): | ||
| target = _resolve_call_target(expression, bindings, self.nodes) |
There was a problem hiding this comment.
When a proof test merely passes a production symbol as an argument, such as assert callable(production_entrypoint), this loop resolves the argument and records it as a call target even though the symbol is never invoked. check_production_seam therefore reports the test as production-wired, allowing exactly the vacuous proof this guard is meant to reject; test-call edges should come from each ast.Call.func, not arbitrary argument expressions.
Useful? React with 👍 / 👎.
| for call in ast.walk(function.node): | ||
| if not isinstance(call, ast.Call): | ||
| continue | ||
| references = [call.func, *call.args, *(keyword.value for keyword in call.keywords)] | ||
| for reference in references: | ||
| for expression in ast.walk(reference): | ||
| target = _resolve_call_target(expression, bindings, self.nodes) |
There was a problem hiding this comment.
When an entrypoint defines a nested function or lambda that calls a required symbol but never invokes that closure, ast.walk(function.node) still descends into the nested body and attributes its calls directly to the entrypoint. The required symbol is consequently reported reachable despite having no executable path from the declared root, so traversal needs to stop at nested function, lambda, and class scopes and analyze those as separate nodes where appropriate.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Import): | ||
| for alias in node.names: | ||
| bindings[alias.asname or alias.name.split(".")[0]] = alias.name | ||
| elif isinstance(node, ast.ImportFrom): |
There was a problem hiding this comment.
For a valid import such as import prod.routes, Python binds the local name prod, but this code binds prod to prod.routes; resolving prod.routes.production_entrypoint() then constructs prod.routes.routes.production_entrypoint and incorrectly emits test_symbol_not_called. Any proof test using the standard unaliased dotted-import form is therefore rejected, so unaliased imports should bind the first component to itself while preserving the full path only for an explicit alias.
Useful? React with 👍 / 👎.
| def _resolve_call_target(function: ast.AST, bindings: dict[str, str], nodes: dict[str, _FunctionNode]) -> str | None: | ||
| parts = _attribute_parts(function) | ||
| if parts is None or not parts: | ||
| return None | ||
| bound = bindings.get(parts[0]) | ||
| if bound is None: | ||
| return None | ||
| target = ".".join((bound, *parts[1:])) |
There was a problem hiding this comment.
When a declared route is a class method, calls such as Runner().production_entrypoint() and self.live_helper() cannot be resolved because _attribute_parts only accepts name/attribute chains and self has no binding. Although class methods are indexed as nodes, a valid class-based production path is therefore reported with test_symbol_not_called and required_symbol_unreachable; the graph needs class/instance context for constructor and self/cls dispatch.
Useful? React with 👍 / 👎.
| @lru_cache(maxsize=8) | ||
| def _call_graph(source_root: Path, test_path: Path) -> _CallGraph: | ||
| production_root = source_root / "polylogue" | ||
| graph_roots = (production_root if production_root.is_dir() else source_root, test_path) | ||
| return _CallGraph(_parse_modules(source_root, graph_roots)) |
There was a problem hiding this comment.
When a mutation test or long-lived verifier edits a production file and calls check_production_seam again for the same root and test path, this cache returns the pre-edit AST because its key contains no source fingerprint or mtime. Removing a required call after an initial green check therefore still produces a green report in the same process, undermining controlled dead-symbol mutation coverage; cache entries must incorporate source state or be explicitly invalidated before each verification run.
Useful? React with 👍 / 👎.
| for function in tuple(node for node in self.nodes.values() if node.module == module.name): | ||
| bindings = {**module_imports, **local_functions, **_imports_from_nodes(function.node.body, module.name)} | ||
| targets: set[str] = set() |
There was a problem hiding this comment.
When a test parameter or local assignment shadows an imported production symbol, the bindings table still maps that name to the import because parameters and assignments are never analyzed. For example, a test that assigns production_entrypoint = lambda: None and invokes that lambda is reported as directly calling the real production entrypoint, so a test double or fixture can produce a false-green reachability proof; lexical bindings must override module imports before call targets are resolved.
Useful? React with 👍 / 👎.
| @lru_cache(maxsize=8) | ||
| def _call_graph(source_root: Path, test_path: Path) -> _CallGraph: | ||
| production_root = source_root / "polylogue" | ||
| graph_roots = (production_root if production_root.is_dir() else source_root, test_path) | ||
| return _CallGraph(_parse_modules(source_root, graph_roots)) |
There was a problem hiding this comment.
As soon as seam specs span multiple test files, test_path makes every cache entry parse and retain a separate full AST/call graph for the entire polylogue package. The eight-entry cache can therefore hold eight near-identical repository-sized graphs, causing verification latency and memory usage to scale with test-file count rather than changed code; cache the production graph once per source state and layer each small test module onto it.
Useful? React with 👍 / 👎.
|
@circleci run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/production_reachability.py`:
- Around line 306-309: Update _calls_in_function to visit only function.body
rather than the complete FunctionDef or AsyncFunctionDef, excluding decorators
and default-value expressions from call indexing. Add a regression fixture where
a decorator or default argument invokes dead_helper, and verify that dead_helper
is not marked reachable from the entrypoint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d6bc346-772c-4f5d-9e64-ade9291727ac
📒 Files selected for processing (6)
devtools/production_reachability.pytests/fixtures/production_reachability/fixture_test.pytests/fixtures/production_reachability/nestedpkg/__init__.pytests/fixtures/production_reachability/nestedpkg/child.pytests/fixtures/production_reachability/routes.pytests/unit/devtools/test_production_reachability.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc0aac908f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if bound in nodes or any(name.startswith(f"{bound}.") for name in nodes): | ||
| return bound |
There was a problem hiding this comment.
When a proof calls an attribute of an imported production function, such as production_entrypoint.__str__(), the constructed attribute target is absent but this fallback returns the bound function itself. The report therefore records a direct call to production_entrypoint even though only its attribute ran, allowing a passing test to satisfy tested_symbols without executing the production entrypoint; the fallback to bound should apply only to constructor calls represented by a bare name.
Useful? React with 👍 / 👎.
| for statement in node.body: | ||
| self.visit(statement) |
There was a problem hiding this comment.
When a required helper is mentioned only under if False: or after an unconditional return, this unconditional statement walk still records the call and reports the helper reachable. Removing the real production invocation can therefore leave the proof green whenever a dead reference remains, defeating the controlled dead-symbol mutation this oracle is intended to enforce; the scanner should at least prune statically false branches and statements after unconditional terminators.
Useful? React with 👍 / 👎.
| for statement in module.tree.body: | ||
| if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| qualified_name = f"{module.name}.{statement.name}" | ||
| self.nodes[qualified_name] = _FunctionNode(qualified_name, statement, module.name) |
There was a problem hiding this comment.
When a production entrypoint is re-exported from a package, the index contains only the defining function and never the public alias. For example, polylogue/daemon/__init__.py re-exports polylogue.daemon.cli.main as polylogue.daemon.main, so a proof that imports and declares the public daemon entrypoint receives missing_production_entrypoint and missing_tested_symbol even though that is the runtime API; imported callable aliases need to resolve to their defining nodes.
Useful? React with 👍 / 👎.
| def visit_Call(self, node: ast.Call) -> None: | ||
| self.calls.append(node) | ||
| self.generic_visit(node) |
There was a problem hiding this comment.
When a test merely creates and closes a coroutine with pending = async_entrypoint(); pending.close(), this visitor records the call and treats the async function's body as reachable even though none of it executed. The same false green occurs inside production when an async required symbol is called without await, so the oracle can certify a route whose required work never runs; edges to AsyncFunctionDef nodes should require an awaited call context.
Useful? React with 👍 / 👎.
1 participant
Summary
Add a structured production-reachability oracle for proof tests. The oracle verifies that declared production seams call the tested production symbol and that the symbol is reachable from the declared production entrypoint.
Problem
The previous proof gate could certify orphan helpers, over-approximate routes through nested function bodies or passed callable arguments, and resolve Python imports/caches incorrectly. The hermetic filesystem-boundary half remains a named successor.
Solution
polylogue-jdesfsuccessor for hermetic path boundaries.Verification
devtools test tests/unit/devtools/test_production_reachability.py— 7 passed.devtools verify --quick— all 24 steps exit 0 at93ab11703c87ac777b9c8513dfa764be5e1a7de1.Scope and residuals
This PR is partial for
polylogue-4v2d3; it does not claim hermetic test-path enforcement, which remainspolylogue-jdesf. It does not mutate production or the live archive.Ref #4v2d3