Skip to content

refactor: extract addon source scanner - #51

Open
barry166 wants to merge 2 commits into
erpipe-org:mainfrom
barry166:barry166/extract-addon-scanner-31
Open

refactor: extract addon source scanner#51
barry166 wants to merge 2 commits into
erpipe-org:mainfrom
barry166:barry166/extract-addon-scanner-31

Conversation

@barry166

@barry166 barry166 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • move the local addon AST/XML scanner into a focused core addon_scanner module
  • preserve addon-scanner and field-ranking imports through compatibility re-exports in agent_tools
  • keep legacy agent_tools helper monkeypatches effective through a path-resolving compatibility wrapper
  • include the new module beside existing core helpers in the import-linter contract

Closes #31

Coverage

  • Odoo versions tested: N/A (pure module extraction)
  • Transports tested: N/A (no transport behavior changed)

Verification

uv run python -m pytest -q
uv run python -m ruff check .
uv run python -m mypy src
uv run python -m black --check src/odoo_mcp/addon_scanner.py src/odoo_mcp/agent_tools.py tests/test_addon_scanner_module.py
PYTHONPATH=src uvx --from import-linter lint-imports
git diff --check

Result: 911 tests passed; both import-linter contracts were kept.

Checklist

  • README/docs are unchanged because runtime behavior and public usage are unchanged
  • CHANGELOG is unchanged because this is an internal module extraction
  • No credentials, private database names, or production debug traces are present
  • Write-execution safety gates are unchanged

Notes for reviewers

The compatibility coverage now verifies both direct helper re-exports and the end-to-end late-bound lookup used when downstream code patches agent_tools._normalize_scan_paths.

@tuanle96

Copy link
Copy Markdown
Collaborator

@barry166 heads-up: today main took #48 (new _normalize_numbers near canonical_json), #42 (field-ranking helpers extracted to field_ranking.py), and the v1.2.0 release, so this branch now conflicts. Could you rebase? Two things to fold in while you're at it: (1) the scanner extraction should account for agent_tools now also re-exporting the field-ranking helpers — keep both re-export blocks; (2) .importlinter already gained field_ranking in source_modules, so your addition of the scanner module lands next to it. Review will be quick once it's green — the #42 extraction merged smoothly and this one follows the same pattern.

The source scanner has an independent AST/XML boundary and should remain
usable without coupling its implementation to the broader agent helper
module. Compatibility re-exports preserve existing callers while the new
module boundary is covered directly.

Constraint: Preserve both addon-scanner and field-ranking compatibility re-exports after rebasing.
Confidence: high
Scope-risk: narrow
Directive: Keep addon scanner helpers isolated from MCP surface modules and retain agent_tools compatibility imports.
Tested: 910 pytest tests; Ruff; mypy; Black; import-linter; git diff checks.
Not-tested: Live Odoo addon directory smoke test.
@barry166
barry166 force-pushed the barry166/extract-addon-scanner-31 branch from da1b566 to 9ab75d7 Compare July 16, 2026 08:13
@barry166

Copy link
Copy Markdown
Contributor Author

Rebased onto 89198e2 and kept both compatibility surfaces in agent_tools: the addon-scanner re-exports and the field-ranking re-exports. The new addon_scanner entry also sits beside the existing core helpers in .importlinter.

Local verification: 910 tests, Ruff, mypy, Black, and both import-linter contracts. The branch is mergeable and CI is green.

@tuanle96 tuanle96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rebasing and preserving both re-export blocks. The branch is clean and the broad gates are healthy: 910 tests pass in an isolated HOME/XDG environment, Ruff and mypy pass, both import-linter contracts are kept, and the direct structural/architecture checks pass.

One compatibility blocker remains from the previous review: the legacy agent_tools monkeypatch surface still does not work end-to-end.

The new test only proves import identity:

assert getattr(agent_tools, name) is getattr(addon_scanner, name)

That does not preserve late-bound helper lookup. scan_addons_source_report.__globals__ now belongs to odoo_mcp.addon_scanner, so a test or downstream consumer that patches agent_tools._normalize_scan_paths no longer affects the re-exported function. Reproduction on current head 9ab75d7:

from odoo_mcp import agent_tools

sentinel = "/tmp/legacy-agent-tools-monkeypatch-sentinel"
agent_tools._normalize_scan_paths = lambda _: [sentinel]
result = agent_tools.scan_addons_source_report(addons_paths=None, max_files=1)
assert result["paths"] == [sentinel]

Current result:

expected: ['/tmp/legacy-agent-tools-monkeypatch-sentinel']
actual:   []
function globals: odoo_mcp.addon_scanner

Please add an end-to-end regression that patches the legacy agent_tools namespace and proves the patched helper is actually used, then implement a compatibility shim (or explicitly revise the issue's compatibility contract if preserving monkeypatch behavior is no longer intended). Import identity alone is not sufficient.

The review driver also printed a structural PASS while saying the Python structural entry point was skipped; I did not use that as proof. The direct repository runner was executed separately and passed.

The extracted scanner now exposes a private path-resolved core so the agent_tools compatibility wrapper can continue resolving patched helpers in its original namespace without moving scanner logic back across the module boundary.

Constraint: Keep addon scanning implemented outside agent_tools while retaining downstream monkeypatch behavior.

Rejected: Synchronize module globals during each scan | thread-unsafe and obscures ownership.

Confidence: high

Scope-risk: narrow

Directive: Keep path normalization late-bound in each public module wrapper.

Tested: 911 pytest tests; Ruff; mypy; Black; import-linter; git diff check.
@barry166

Copy link
Copy Markdown
Contributor Author

Fixed the remaining compatibility blocker in 6e05cfd.

agent_tools.scan_addons_source_report now resolves the legacy _normalize_scan_paths symbol before entering the extracted scanner core, and the regression patches the old namespace end to end. Full verification passes: 911 tests, Ruff, mypy, Black, both import-linter contracts, and the diff check.

@tuanle96 tuanle96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head 6e05cfdfd967d927b06f28fa3c5591fb5f042ebc against the current MCP 2026-07-28 / SDK v2 main.

The previous compatibility blocker is addressed by the end-to-end legacy agent_tools._normalize_scan_paths monkeypatch regression. I also tested a temporary merge with current origin/main:

  • 917 tests passed
  • Ruff passed
  • mypy passed across 35 source files
  • both import-linter contracts were kept

Approved. Issue #31 can close when this PR merges.

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.

Split agent_tools.py (1,317 LOC): extract the AST addon scanner

2 participants