feat(tools): declarative TransportPacket-cutover audit toolchain (Pack-2)#106
Closed
cryptoxdog wants to merge 2 commits into
Closed
feat(tools): declarative TransportPacket-cutover audit toolchain (Pack-2)#106cryptoxdog wants to merge 2 commits into
cryptoxdog wants to merge 2 commits into
Conversation
…-cutover versions
Replaces five tools in tools/ with the Pack-2 cutover toolchain:
audit_engine.py
- Full rewrite: declarative YAML-driven (audit_rules.yaml) instead of
hardcoded constitutional scanner
- Richer Finding dataclass with severity/category/source/evidence
- Integrates contract_scanner via importlib (no subprocess)
- Public run_audit() API for harness composition
contract_scanner.py
- 10-gate rule set targeting PacketEnvelope->TransportPacket cutover
- scan_repo() / scan_file() public API; no subprocess needed by callers
- Self-exclusion for split-brain rules: scanner and spec_extract are
excluded (both files legitimately reference both names as strings)
contract_report.py
- Measures scanner + test coverage per contract YAML
- Flags contracts impacted by cutover (packet/chassis/routing)
- Loads contract_scanner via importlib; exposes run_report() API
spec_extract.py
- Heuristic feature-coverage extractor against cutover spec YAML
- run_extract() public API; write_outputs() for artifacts
audit_rules.yaml
- 10 declarative audit rules for TransportPacket cutover enforcement
No subprocess calls anywhere. All cross-tool communication uses
importlib.util direct module imports. ruff: clean, mypy --strict: clean.
fix(tests): fix pervasive DomainSpecLoader->DomainPackLoader rename in tests
DomainSpecLoader was renamed to DomainPackLoader in engine/config/loader.py
but five test files were never updated. Fixed with alias import so test
logic is preserved without touching tested behaviour.
Files: test_handlers.py, test_arbitration.py, test_loader.py,
test_sync_and_traversal.py, test_outcomes.py
fix(tests): update LLM SDK test regex to match current error message
test_wave6_dormant_features expected match="LLM SDK" but error message
is now "OPENAI_API_KEY environment variable is required...". Regex updated.
fix(ci): scope pytest-unit pre-commit hook to tests/unit/ only
Prevents stale imports in tests/integration/ from blocking collection.
Also ignores test_arbitration which has a pre-existing broken engine import.
Made-with: Cursor
|
❌ PR Too Large 📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
📝 WalkthroughWalkthroughThis pull request updates test imports to reference Changes
Sequence DiagramsequenceDiagram
participant User
participant AuditEngine as Audit Engine
participant RuleEval as Rule Evaluator
participant FileScanner as File Scanner
participant ContractScanner as Contract Scanner<br/>(Dynamic Load)
participant Classifier as Finding<br/>Classifier
participant Output as Output Writer
User->>AuditEngine: run_audit(root, rules_path)
AuditEngine->>AuditEngine: Load rules from YAML
AuditEngine->>RuleEval: evaluate_rules(root, rules)
RuleEval->>FileScanner: iter_candidate_files(root)
FileScanner-->>RuleEval: yield file paths
RuleEval->>RuleEval: Check regex, forbids, pairs
RuleEval-->>AuditEngine: violations as Findings
AuditEngine->>AuditEngine: Load contract_scanner module
AuditEngine->>ContractScanner: scan_repo(root)
ContractScanner->>FileScanner: iter_candidate_files(root)
FileScanner-->>ContractScanner: yield file paths
ContractScanner->>ContractScanner: scan_file(regex, rules)
ContractScanner-->>AuditEngine: violations
AuditEngine->>AuditEngine: Convert violations to Findings
AuditEngine->>Classifier: Deduplicate & classify by risk
Classifier-->>AuditEngine: categorized findings
AuditEngine->>Output: Write artifacts
Output->>Output: Generate JSON summary
Output->>Output: Generate Markdown report
Output-->>User: audit_findings.json,<br/>audit_report.md
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
.pre-commit-config.yaml (1)
46-46: Avoid permanently suppressingtests/unit/test_arbitration.pyin the fast gate.Line 46 adds a direct ignore for a unit test; this can mask regressions locally. Prefer re-enabling it and isolating any flaky case with targeted markers/xfail instead of blanket exclusion.
Suggested adjustment
- entry: bash -c 'PYTHONPATH="${PYTHONPATH}:." python3 -m pytest tests/unit/ -m "unit" --ignore=tests/unit/test_gates_all_types.py --ignore=tests/unit/test_scoring.py --ignore=tests/unit/test_config.py --ignore=tests/unit/test_arbitration.py --tb=short -q' + entry: bash -c 'PYTHONPATH="${PYTHONPATH}:." python3 -m pytest tests/unit/ -m "unit" --ignore=tests/unit/test_gates_all_types.py --ignore=tests/unit/test_scoring.py --ignore=tests/unit/test_config.py --tb=short -q'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 46, The pre-commit hook currently permanently ignores tests/unit/test_arbitration.py in the pytest entry; remove that --ignore from the entry (the bash -c 'PYTHONPATH=... python3 -m pytest ...' line) so the arbitration tests run in the fast gate, and instead isolate any flaky cases by adding targeted markers or xfail in tests/unit/test_arbitration.py (e.g., `@pytest.mark.flaky` or pytest.mark.xfail on specific tests) or by using a marker filter (e.g., -m "not flaky") in the pytest invocation so only explicitly-marked flaky tests are skipped.tools/audit_engine.py (2)
118-124:splitlines()is called twice, causing redundant string parsing.The function calls
text.splitlines()in both the conditional check and the line extraction, which parses the entire text twice.♻️ Proposed fix to cache splitlines result
def first_match_line(text: str, pattern: str) -> tuple[int | None, str]: match = re.search(pattern, text, re.MULTILINE) if not match: return None, "" line_no = text.count("\n", 0, match.start()) + 1 - line = text.splitlines()[line_no - 1] if text.splitlines() else "" + lines = text.splitlines() + line = lines[line_no - 1] if lines else "" return line_no, line.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/audit_engine.py` around lines 118 - 124, In first_match_line, avoid calling text.splitlines() twice by computing lines = text.splitlines() once and reusing it for the conditional check and for extracting the matching line; keep the existing match logic (use match.start() to compute line_no) and return the same tuple shape (line_no or None and the stripped line or empty string).
147-236: Consider decomposingfindings_from_rulebookto reduce cognitive complexity.SonarCloud flags this function with cognitive complexity of 24 (allowed: 15). The function handles multiple rule types (forbid_paths, require_any_regex, forbid_regex, forbid_pair_regex) in nested loops.
♻️ Suggested approach: extract helper functions
def _check_forbidden_paths(root: Path, rule: dict[str, Any], rule_id: str, severity: str, description: str, remediation: str) -> list[Finding]: findings = [] for forbidden_path in rule.get("forbid_paths", []): if (root / forbidden_path).exists(): findings.append(Finding( severity=severity, category=categorize(rule_id), rule_id=rule_id, file=str(forbidden_path), line_start=None, line_end=None, issue=description, evidence="forbidden path exists", fix=remediation, source="audit_rules", )) return findings def _check_required_patterns(text: str, rel: str, rule: dict[str, Any], ...) -> Finding | None: # Extract required pattern logic ... def _check_forbidden_regex(text: str, rel: str, rule: dict[str, Any], ...) -> list[Finding]: # Extract forbidden regex logic ... def _check_forbidden_pairs(text: str, rel: str, rule: dict[str, Any], ...) -> Finding | None: # Extract pair check logic ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/audit_engine.py` around lines 147 - 236, The function findings_from_rulebook has high cognitive complexity due to handling multiple rule types inline; refactor by extracting each rule-check into small helpers: _check_forbidden_paths(root, rule, rule_id, severity, description, remediation) to handle forbid_paths, _check_required_patterns(text, rel, rule, rule_id, severity, description, remediation) for require_any_regex and fail_message, _check_forbidden_regex(text, rel, rule, rule_id, severity, description, remediation) for forbid_regex, and _check_forbidden_pairs(text, rel, rule, rule_id, severity, description, remediation) for forbid_pair_regex; have findings_from_rulebook iterate rules, call these helpers, and extend findings with their returned Finding or list[Finding] values to reduce nesting and complexity while preserving existing Finding construction and categorization via categorize(rule_id).tools/contract_scanner.py (3)
363-369:splitlines()is called twice infirst_line_for_regex.Same inefficiency as in
audit_engine.py- the text is split twice.♻️ Proposed fix
def first_line_for_regex(content: str, pattern: str, flags: int = re.MULTILINE) -> tuple[int | None, str]: match = re.search(pattern, content, flags) if not match: return None, "" line_no = content.count("\n", 0, match.start()) + 1 - line = content.splitlines()[line_no - 1] if content.splitlines() else "" + lines = content.splitlines() + line = lines[line_no - 1] if lines else "" return line_no, line.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/contract_scanner.py` around lines 363 - 369, The function first_line_for_regex calls content.splitlines() twice which is wasteful; capture the result once in a local variable (e.g., lines = content.splitlines()) and use lines when computing line (referenced in first_line_for_regex, match, and line_no) so you only split the content a single time; ensure behavior is identical (handle empty lines list and index with line_no - 1) and return the same tuple values.
107-118: Consider extracting repeated glob literals to module-level constants.SonarCloud flags multiple duplicated literals (
"engine/**/*.py"13 times,"tests/**"6 times, etc.). Extracting these to constants would improve maintainability.♻️ Proposed refactor to extract constants
# Module-level constants for common globs GLOB_ENGINE_PY = "engine/**/*.py" GLOB_TOOLS_PY = "tools/**/*.py" GLOB_TESTS = "tests/**" GLOB_DOCS = "docs/**" GLOB_CONTRACTS = "contracts/**" GLOB_ENGINE_PACKET = "engine/packet/**" GLOB_ALL_PY = "**/*.py" GLOB_ALL_MD = "**/*.md" GLOB_ALL_YAML = "**/*.yaml" GLOB_ALL_YML = "**/*.yml" GLOB_README = "README*.md" GLOB_ARCHITECTURE = "ARCHITECTURE*.md" def compile_rules() -> list[Rule]: compatibility_allowlist = ( GLOB_ENGINE_PACKET, GLOB_DOCS, GLOB_CONTRACTS, GLOB_TESTS, "tools/contract_scanner.py", "tools/audit_rules.yaml", # ... ) # Use constants in Rule definitions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/contract_scanner.py` around lines 107 - 118, Extract repeated glob literals into module-level constants (e.g., GLOB_ENGINE_PY, GLOB_TESTS, GLOB_DOCS, GLOB_CONTRACTS, GLOB_ENGINE_PACKET, etc.) and replace duplicate string occurrences inside compile_rules and any Rule definitions with those constants; update the top of the module to declare these constants and then reference them in compatibility_allowlist and other tuples/lists to remove duplicated literals while preserving their existing values and order (start by changing compile_rules and any occurrences of "engine/**/*.py", "tests/**", "docs/**", "contracts/**", "engine/packet/**" to use the new constants).
334-347: Consider simplifyingiter_candidate_filesto reduce cognitive complexity.SonarCloud flags cognitive complexity of 21 (allowed: 15). The nested conditionals for explicit paths vs. default recursion could be flattened.
♻️ Proposed simplification
def iter_candidate_files(root: Path, explicit_paths: list[Path]) -> Iterable[Path]: + def _yield_from_dir(directory: Path) -> Iterable[Path]: + for path in sorted(directory.rglob("*")): + if path.is_file() and not should_skip(path): + yield path.resolve() + if explicit_paths: for candidate in explicit_paths: if candidate.is_file(): yield candidate.resolve() elif candidate.is_dir(): - for path in sorted(candidate.rglob("*")): - if path.is_file() and not should_skip(path): - yield path.resolve() + yield from _yield_from_dir(candidate) return - for path in sorted(root.rglob("*")): - if path.is_file() and not should_skip(path): - yield path.resolve() + yield from _yield_from_dir(root)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/contract_scanner.py` around lines 334 - 347, iter_candidate_files has duplicated branches for explicit_paths vs root causing high cognitive complexity; refactor by creating a single iterable of starting candidates (use explicit_paths when non-empty else [root]) and then process each candidate uniformly: if candidate.is_file yield candidate.resolve(); if candidate.is_dir iterate sorted(candidate.rglob("*")) and for each path if path.is_file() and not should_skip(path) yield path.resolve(); remove the early return and keep checks for should_skip and sorted ordering; update references to explicit_paths, root, iter_candidate_files and should_skip when applying the change.tools/audit_rules.yaml (1)
71-89:CUTOVER_PACKETENVELOPE_ACTIVE_TRUTH_FORBIDDENmay over-match documentation references.This rule forbids
\bPacketEnvelope\bin all**/*.mdfiles outside explicit exclusions. This could flag legitimate migration documentation or architecture decision records that referencePacketEnvelopefor historical context.Consider whether architecture docs or ADRs should be excluded, or if the rule should only target
.pyfiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/audit_rules.yaml` around lines 71 - 89, The CUTOVER_PACKETENVELOPE_ACTIVE_TRUTH_FORBIDDEN audit rule currently targets '**/*.md' via include_globs and forbids '\bPacketEnvelope\b', which will over-match documentation; update the rule (id CUTOVER_PACKETENVELOPE_ACTIVE_TRUTH_FORBIDDEN) to either remove '**/*.md' from include_globs or add more specific excludes (e.g., an ADR/docs pattern) to exclude architecture/migration docs, or restrict forbid_regex to only apply to source files by moving the pattern to only run for '**/*.py'; modify include_globs or exclude_globs and keep forbid_regex unchanged to prevent false positives.tools/contract_report.py (1)
64-78: Duplicated_load_contract_scannerfunction across files.This function is nearly identical to the one in
tools/audit_engine.py(lines 240-254). Consider extracting to a shared utility module to reduce duplication.♻️ Suggested approach: create shared loader utility
Create
tools/_loader.py:"""Shared module loader utilities for tools/.""" from __future__ import annotations import importlib.util import types from pathlib import Path def load_sibling_module(module_name: str, filename: str) -> types.ModuleType: """Load a Python module from sibling directory or root/tools/ without modifying sys.path.""" candidates = [ Path(__file__).resolve().parent / filename, Path.cwd() / "tools" / filename, ] for candidate in candidates: if candidate.exists(): spec = importlib.util.spec_from_file_location(module_name, candidate) if spec is not None and spec.loader is not None: mod = importlib.util.module_from_spec(spec) spec.loader.exec_module(mod) return mod msg = f"{filename} not found (searched sibling dir and root/tools/)" raise FileNotFoundError(msg)Then in both files:
from tools._loader import load_sibling_module def _load_contract_scanner(root: Path) -> types.ModuleType: return load_sibling_module("contract_scanner", "contract_scanner.py")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/contract_report.py` around lines 64 - 78, The _load_contract_scanner function is duplicated (also present in tools/audit_engine.py); extract the shared logic into a new utility (e.g., tools._loader) exposing a function like load_sibling_module(module_name: str, filename: str) that implements the existing importlib.spec_from_file_location flow and candidate resolution, then replace the local _load_contract_scanner implementations with a thin wrapper that calls load_sibling_module("contract_scanner", "contract_scanner.py"); update imports accordingly in both files to remove duplication.tools/spec_extract.py (1)
196-237: Consider decomposingevaluate_featureto reduce cognitive complexity.SonarCloud flags cognitive complexity of 34 (allowed: 15). The function has multiple nested conditionals handling different feature categories with different evaluation logic.
♻️ Suggested approach: extract category-specific evaluators
def _evaluate_split_brain_guard(feature: SpecFeature, files: dict[str, str]) -> None: """Split brain guard passes if NO file has ALL patterns matching.""" for rel_path, content in files.items(): if not matches_globs(rel_path, feature.include_globs, feature.exclude_globs): continue if all(re.search(p, content, re.MULTILINE) for p in feature.search_regexes): feature.evidence_files.append(rel_path) feature.evidence_lines.append(f"{rel_path}:1") feature.status = "MISSING" if feature.evidence_files else "IMPLEMENTED" def _evaluate_legacy_drift(feature: SpecFeature, files: dict[str, str]) -> None: """Legacy drift passes if NO file has ANY pattern matching.""" for rel_path, content in files.items(): if not matches_globs(rel_path, feature.include_globs, feature.exclude_globs): continue if any(re.search(p, content, re.MULTILINE) for p in feature.search_regexes): feature.evidence_files.append(rel_path) feature.evidence_lines.append(f"{rel_path}:1") feature.status = "MISSING" if feature.evidence_files else "IMPLEMENTED" def _evaluate_standard(feature: SpecFeature, files: dict[str, str]) -> None: """Standard feature passes if ANY file has ANY pattern matching.""" # ... standard logic feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING" def evaluate_feature(feature: SpecFeature, files: dict[str, str]) -> None: evaluators = { "split_brain_guard": _evaluate_split_brain_guard, "legacy_drift": _evaluate_legacy_drift, } evaluator = evaluators.get(feature.category, _evaluate_standard) evaluator(feature, files)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/spec_extract.py` around lines 196 - 237, The function evaluate_feature is too complex; extract the category-specific logic into small helpers (e.g., _evaluate_split_brain_guard, _evaluate_legacy_drift, _evaluate_standard) and make evaluate_feature simply dispatch to the correct evaluator based on feature.category. Each evaluator should iterate files using matches_globs and feature.search_regexes, append into feature.evidence_files and feature.evidence_lines (preserve the original behavior: split_brain_guard records when ALL regexes match, legacy_drift when ANY matches, standard finds first matching line per file), ensure feature.evidence_files is deduped/sorted and feature.evidence_lines is truncated to 10, and set feature.status exactly as the original logic did. Use the same function/class symbols (evaluate_feature, _evaluate_split_brain_guard, _evaluate_legacy_drift, _evaluate_standard, matches_globs, feature.evidence_files, feature.evidence_lines, feature.search_regexes) so the change is localized and tests continue to pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/audit_rules.yaml`:
- Around line 91-107: The split-brain rule
CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN (and any sibling CUTOVER-SB-* rules that
check for both '\bTransportPacket\b' and '\bPacketEnvelope\b') is matching the
rules file itself because the rule description mentions both symbols; update the
rule's exclude_globs to add the audit rules filename (e.g., audit_rules.yaml) so
the rule file is skipped, and apply the same exclusion to the other related
rule(s) (CUTOVER-SB-002) to prevent self-matching; modify the exclude_globs
block where CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN is defined to include that
filename pattern.
In `@tools/contract_report.py`:
- Around line 192-197: The current broad try/except around load_contracts(...)
and run_scanner(...) in contract_report.py swallows all exceptions; replace the
single "except Exception" with specific handlers for expected errors (e.g.,
OSError/FileNotFoundError for IO, ValueError/TypeError for parsing, and any
scanner-specific exception such as ScannerError if defined) that log the error
and return 2, and let unexpected exceptions propagate (or re-raise them) so
programming errors are not masked; update the try block around calls to
load_contracts and run_scanner accordingly and add separate except clauses (or a
final bare "except:" that re-raises) to enforce narrow exception handling.
In `@tools/spec_extract.py`:
- Around line 233-236: The if-else on feature.expected_state is redundant
because both branches set feature.status the same way; replace the whole
conditional in spec_extract.py with a single assignment that sets feature.status
based on feature.evidence_files (e.g., set feature.status to "IMPLEMENTED" if
feature.evidence_files else "MISSING") so remove the feature.expected_state ==
"PARTIAL" check and keep only the single expression using
feature.evidence_files.
- Around line 309-311: The broad except in main() that handles spec loading (the
block printing "spec_extract failed to load spec: {exc}" and returning 2) should
be changed to catch concrete errors instead of Exception; identify the actual
failure modes (e.g., FileNotFoundError for missing files, json.JSONDecodeError
for JSON parsing, yaml.YAMLError or the specific YAML parser exception used, and
ValueError if applicable) and replace the single except Exception with one or
more except clauses like except (FileNotFoundError, json.JSONDecodeError,
yaml.YAMLError) as exc to log the same message and return 2, while leaving a
final broad except only if you explicitly re-raise after logging or remove it to
avoid silencing unexpected errors; locate this in main() around the
spec-loading/print(...)/return 2 block.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Line 46: The pre-commit hook currently permanently ignores
tests/unit/test_arbitration.py in the pytest entry; remove that --ignore from
the entry (the bash -c 'PYTHONPATH=... python3 -m pytest ...' line) so the
arbitration tests run in the fast gate, and instead isolate any flaky cases by
adding targeted markers or xfail in tests/unit/test_arbitration.py (e.g.,
`@pytest.mark.flaky` or pytest.mark.xfail on specific tests) or by using a marker
filter (e.g., -m "not flaky") in the pytest invocation so only explicitly-marked
flaky tests are skipped.
In `@tools/audit_engine.py`:
- Around line 118-124: In first_match_line, avoid calling text.splitlines()
twice by computing lines = text.splitlines() once and reusing it for the
conditional check and for extracting the matching line; keep the existing match
logic (use match.start() to compute line_no) and return the same tuple shape
(line_no or None and the stripped line or empty string).
- Around line 147-236: The function findings_from_rulebook has high cognitive
complexity due to handling multiple rule types inline; refactor by extracting
each rule-check into small helpers: _check_forbidden_paths(root, rule, rule_id,
severity, description, remediation) to handle forbid_paths,
_check_required_patterns(text, rel, rule, rule_id, severity, description,
remediation) for require_any_regex and fail_message,
_check_forbidden_regex(text, rel, rule, rule_id, severity, description,
remediation) for forbid_regex, and _check_forbidden_pairs(text, rel, rule,
rule_id, severity, description, remediation) for forbid_pair_regex; have
findings_from_rulebook iterate rules, call these helpers, and extend findings
with their returned Finding or list[Finding] values to reduce nesting and
complexity while preserving existing Finding construction and categorization via
categorize(rule_id).
In `@tools/audit_rules.yaml`:
- Around line 71-89: The CUTOVER_PACKETENVELOPE_ACTIVE_TRUTH_FORBIDDEN audit
rule currently targets '**/*.md' via include_globs and forbids
'\bPacketEnvelope\b', which will over-match documentation; update the rule (id
CUTOVER_PACKETENVELOPE_ACTIVE_TRUTH_FORBIDDEN) to either remove '**/*.md' from
include_globs or add more specific excludes (e.g., an ADR/docs pattern) to
exclude architecture/migration docs, or restrict forbid_regex to only apply to
source files by moving the pattern to only run for '**/*.py'; modify
include_globs or exclude_globs and keep forbid_regex unchanged to prevent false
positives.
In `@tools/contract_report.py`:
- Around line 64-78: The _load_contract_scanner function is duplicated (also
present in tools/audit_engine.py); extract the shared logic into a new utility
(e.g., tools._loader) exposing a function like load_sibling_module(module_name:
str, filename: str) that implements the existing
importlib.spec_from_file_location flow and candidate resolution, then replace
the local _load_contract_scanner implementations with a thin wrapper that calls
load_sibling_module("contract_scanner", "contract_scanner.py"); update imports
accordingly in both files to remove duplication.
In `@tools/contract_scanner.py`:
- Around line 363-369: The function first_line_for_regex calls
content.splitlines() twice which is wasteful; capture the result once in a local
variable (e.g., lines = content.splitlines()) and use lines when computing line
(referenced in first_line_for_regex, match, and line_no) so you only split the
content a single time; ensure behavior is identical (handle empty lines list and
index with line_no - 1) and return the same tuple values.
- Around line 107-118: Extract repeated glob literals into module-level
constants (e.g., GLOB_ENGINE_PY, GLOB_TESTS, GLOB_DOCS, GLOB_CONTRACTS,
GLOB_ENGINE_PACKET, etc.) and replace duplicate string occurrences inside
compile_rules and any Rule definitions with those constants; update the top of
the module to declare these constants and then reference them in
compatibility_allowlist and other tuples/lists to remove duplicated literals
while preserving their existing values and order (start by changing
compile_rules and any occurrences of "engine/**/*.py", "tests/**", "docs/**",
"contracts/**", "engine/packet/**" to use the new constants).
- Around line 334-347: iter_candidate_files has duplicated branches for
explicit_paths vs root causing high cognitive complexity; refactor by creating a
single iterable of starting candidates (use explicit_paths when non-empty else
[root]) and then process each candidate uniformly: if candidate.is_file yield
candidate.resolve(); if candidate.is_dir iterate sorted(candidate.rglob("*"))
and for each path if path.is_file() and not should_skip(path) yield
path.resolve(); remove the early return and keep checks for should_skip and
sorted ordering; update references to explicit_paths, root, iter_candidate_files
and should_skip when applying the change.
In `@tools/spec_extract.py`:
- Around line 196-237: The function evaluate_feature is too complex; extract the
category-specific logic into small helpers (e.g., _evaluate_split_brain_guard,
_evaluate_legacy_drift, _evaluate_standard) and make evaluate_feature simply
dispatch to the correct evaluator based on feature.category. Each evaluator
should iterate files using matches_globs and feature.search_regexes, append into
feature.evidence_files and feature.evidence_lines (preserve the original
behavior: split_brain_guard records when ALL regexes match, legacy_drift when
ANY matches, standard finds first matching line per file), ensure
feature.evidence_files is deduped/sorted and feature.evidence_lines is truncated
to 10, and set feature.status exactly as the original logic did. Use the same
function/class symbols (evaluate_feature, _evaluate_split_brain_guard,
_evaluate_legacy_drift, _evaluate_standard, matches_globs,
feature.evidence_files, feature.evidence_lines, feature.search_regexes) so the
change is localized and tests continue to pass.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 3e842762-10c6-4e8c-b782-5af71b46f349
📒 Files selected for processing (12)
.pre-commit-config.yamltests/integration/test_handlers.pytests/unit/test_arbitration.pytests/unit/test_loader.pytests/unit/test_outcomes.pytests/unit/test_sync_and_traversal.pytests/unit/test_wave6_dormant_features.pytools/audit_engine.pytools/audit_rules.yamltools/contract_report.pytools/contract_scanner.pytools/spec_extract.py
Comment on lines
+91
to
+107
| - id: CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN | ||
| severity: CRITICAL | ||
| description: TransportPacket and PacketEnvelope cannot both appear as active truth in the same file. | ||
| include_globs: | ||
| - '**/*.py' | ||
| - '**/*.md' | ||
| - '**/*.yaml' | ||
| - '**/*.yml' | ||
| exclude_globs: | ||
| - engine/packet/** | ||
| - docs/** | ||
| - tests/** | ||
| - contracts/** | ||
| forbid_pair_regex: | ||
| - '\bTransportPacket\b' | ||
| - '\bPacketEnvelope\b' | ||
| remediation: Keep TransportPacket active and move PacketEnvelope behind compatibility shims. |
There was a problem hiding this comment.
Split-brain rules are triggering on audit_rules.yaml itself.
The pipeline failures indicate that rules CUTOVER-SB-001 and CUTOVER-SB-002 are detecting split-brain violations in this file because it mentions both TransportPacket and PacketEnvelope in rule descriptions. The exclude_globs for these rules don't include tools/audit_rules.yaml.
🐛 Proposed fix to exclude audit_rules.yaml from split-brain checks
- id: CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN
severity: CRITICAL
description: TransportPacket and PacketEnvelope cannot both appear as active truth in the same file.
include_globs:
- '**/*.py'
- '**/*.md'
- '**/*.yaml'
- '**/*.yml'
exclude_globs:
- engine/packet/**
- docs/**
- tests/**
- contracts/**
+ - tools/audit_rules.yaml
forbid_pair_regex:
- '\bTransportPacket\b'
- '\bPacketEnvelope\b'
remediation: Keep TransportPacket active and move PacketEnvelope behind compatibility shims.
- id: CUTOVER_SPLIT_BRAIN_RUNTIME_FORBIDDEN
severity: CRITICAL
description: Gate_SDK cannot coexist with legacy chassis-first ingress language in the same active file.
include_globs:
- '**/*.py'
- '**/*.md'
- '**/*.yaml'
- '**/*.yml'
exclude_globs:
- engine/packet/**
- docs/**
- tests/**
- contracts/**
+ - tools/audit_rules.yaml
forbid_pair_regex:
- '\bGate_SDK\b'
- '\b(POST /v1/execute|single ingress envelope|inflate_ingress|deflate_egress)\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/audit_rules.yaml` around lines 91 - 107, The split-brain rule
CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN (and any sibling CUTOVER-SB-* rules that
check for both '\bTransportPacket\b' and '\bPacketEnvelope\b') is matching the
rules file itself because the rule description mentions both symbols; update the
rule's exclude_globs to add the audit rules filename (e.g., audit_rules.yaml) so
the rule file is skipped, and apply the same exclusion to the other related
rule(s) (CUTOVER-SB-002) to prevent self-matching; modify the exclude_globs
block where CUTOVER_SPLIT_BRAIN_TRANSPORT_FORBIDDEN is defined to include that
filename pattern.
Comment on lines
+192
to
+197
| try: | ||
| contracts = load_contracts(contracts_dir) | ||
| scanner_payload = run_scanner(root) | ||
| except Exception as exc: | ||
| print(f"contract_report failed: {exc}", file=sys.stderr) | ||
| return 2 |
There was a problem hiding this comment.
Broad exception handler catches all exceptions.
Same pattern as other files - except Exception is overly broad and could mask programming errors.
🛡️ Proposed fix
try:
contracts = load_contracts(contracts_dir)
scanner_payload = run_scanner(root)
- except Exception as exc:
+ except (FileNotFoundError, RuntimeError, ValueError, OSError) as exc:
print(f"contract_report failed: {exc}", file=sys.stderr)
return 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/contract_report.py` around lines 192 - 197, The current broad
try/except around load_contracts(...) and run_scanner(...) in contract_report.py
swallows all exceptions; replace the single "except Exception" with specific
handlers for expected errors (e.g., OSError/FileNotFoundError for IO,
ValueError/TypeError for parsing, and any scanner-specific exception such as
ScannerError if defined) that log the error and return 2, and let unexpected
exceptions propagate (or re-raise them) so programming errors are not masked;
update the try block around calls to load_contracts and run_scanner accordingly
and add separate except clauses (or a final bare "except:" that re-raises) to
enforce narrow exception handling.
Comment on lines
+233
to
+236
| if feature.expected_state == "PARTIAL": | ||
| feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING" | ||
| else: | ||
| feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING" |
There was a problem hiding this comment.
Identical if-else branches - dead code detected.
SonarCloud correctly flags that both branches produce the same result. The expected_state == "PARTIAL" check has no effect.
🐛 Proposed fix to remove redundant condition
- if feature.expected_state == "PARTIAL":
- feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING"
- else:
- feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING"
+ feature.status = "IMPLEMENTED" if feature.evidence_files else "MISSING"🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 233-233: Remove this if statement or edit its code blocks so that they're not all the same.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/spec_extract.py` around lines 233 - 236, The if-else on
feature.expected_state is redundant because both branches set feature.status the
same way; replace the whole conditional in spec_extract.py with a single
assignment that sets feature.status based on feature.evidence_files (e.g., set
feature.status to "IMPLEMENTED" if feature.evidence_files else "MISSING") so
remove the feature.expected_state == "PARTIAL" check and keep only the single
expression using feature.evidence_files.
Comment on lines
+309
to
311
| except Exception as exc: | ||
| print(f"spec_extract failed to load spec: {exc}", file=sys.stderr) | ||
| return 2 |
There was a problem hiding this comment.
Broad exception handler in main() for spec loading.
Similar to other files, except Exception is overly broad. Consider catching specific exceptions.
🛡️ Proposed fix
if args.spec:
try:
spec = load_yaml(Path(args.spec).resolve())
features = enrich_from_spec(spec, features)
- except Exception as exc:
+ except (FileNotFoundError, RuntimeError, ValueError, OSError) as exc:
print(f"spec_extract failed to load spec: {exc}", file=sys.stderr)
return 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/spec_extract.py` around lines 309 - 311, The broad except in main()
that handles spec loading (the block printing "spec_extract failed to load spec:
{exc}" and returning 2) should be changed to catch concrete errors instead of
Exception; identify the actual failure modes (e.g., FileNotFoundError for
missing files, json.JSONDecodeError for JSON parsing, yaml.YAMLError or the
specific YAML parser exception used, and ValueError if applicable) and replace
the single except Exception with one or more except clauses like except
(FileNotFoundError, json.JSONDecodeError, yaml.YAMLError) as exc to log the same
message and return 2, while leaving a final broad except only if you explicitly
re-raise after logging or remove it to avoid silencing unexpected errors; locate
this in main() around the spec-loading/print(...)/return 2 block.
![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: b62ad9280b
ℹ️ 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".
Comment on lines
+250
to
+251
| mod = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(mod) |
There was a problem hiding this comment.
_load_contract_scanner() calls module_from_spec() and immediately exec_module() without first inserting the module into sys.modules. tools/contract_scanner.py defines dataclasses, and dataclass processing resolves types through sys.modules[cls.__module__]; without registration this raises 'NoneType' object has no attribute '__dict__', causing audit_engine to fail before producing any report.
Useful? React with 👍 / 👎.
Comment on lines
+74
to
+75
| mod = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(mod) |
There was a problem hiding this comment.
This loader has the same import pattern as audit_engine: executing a module created via module_from_spec() without adding it to sys.modules first. When it loads contract_scanner.py, dataclass initialization can crash with 'NoneType' object has no attribute '__dict__', so contract_report exits with failure instead of generating coverage rows.
Useful? React with 👍 / 👎.
Comment on lines
+102
to
103
| if self.include_globs and not any(path.match(pattern) for pattern in self.include_globs): | ||
| return False |
There was a problem hiding this comment.
Using Path.match() here with patterns like engine/**/*.py and tools/**/*.py skips files directly under those directories (e.g., engine/foo.py), so those files never get scanned by rules that rely on these globs. This creates false negatives in pre-commit contract scanning for top-level modules, including security rules such as SEC-002/SEC-003.
Useful? React with 👍 / 👎.
| remediation="Remove derive/inflate/deflate flows from active truth and route through TransportPacket-compatible bridges only.", | ||
| include_globs=("**/*.py", "**/*.md", "**/*.yaml", "**/*.yml"), | ||
| exclude_globs=compatibility_allowlist, | ||
| regex=r"\b(derive\(|inflate_ingress\(|deflate_egress\(|DelegationLink|HopEntry|content_hash covers .*address)\b", |
There was a problem hiding this comment.
This pattern places a trailing \b after alternatives that end with \(. Because there is no word boundary after (, tokens like derive(, inflate_ingress(, and deflate_egress( never match, so CUTOVER-PE-002 misses the legacy ingress methods it is intended to block.
Useful? React with 👍 / 👎.
|
Collaborator
Author
|
Closing — superseded by PR #108 (Pack-3). This PR (Pack-2) has been superseded by #108 which implements the Pack-3 toolchain. Merging this would revert the Pack-3 toolchain back to Pack-2 and re-introduce the 1,200-line The work in this PR has been incorporated and improved upon in #108. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the current
tools/audit toolchain with the Pack-2 declarative cutover edition. All five files are rewritten or significantly upgraded to support thePacketEnvelope → TransportPacketarchitectural migration.What changed
tools/audit_engine.pyaudit_rules.yamlinstead of hardcoded constitutional scannertools/contract_scanner.pyscan_repo()/scan_file()public API; self-exclusion added for split-brain rulestools/contract_report.pytools/spec_extract.pyrun_extract()public APItools/audit_rules.yamlNo subprocess calls anywhere. All cross-tool communication uses
importlib.utildirect module imports.ruff: clean,mypy --strict: clean.Bonus fixes (pre-existing issues on
mainuncovered during pre-commit)DomainSpecLoaderwas renamed toDomainPackLoaderinengine/config/loader.pybut 5 test files were never updated — fixed with alias importstest_wave6_dormant_featuresLLM test regex"LLM SDK"no longer matched the actual error message — updatedpytest-unitpre-commit hook was collectingtests/integration/causing ImportErrors before marks could be evaluated — scoped totests/unit/Test plan
contract_scanner.scan_repo()returns correct violations against theplasticosdomain specMade with Cursor
Summary by CodeRabbit