refactor: cross-app import boundaries and import-linter#240
Merged
wpak-ai merged 4 commits intoMay 26, 2026
Merged
Conversation
…ross-app dependencies
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR resolves five documented cross-app import tech-debt edges by introducing service-layer façades ( ChangesCross-app import boundary refactor and enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
github_activity_tracker/tests/test_sync_api.py (1)
6-29: ⚡ Quick winAssert the full
sync_apiexport contract explicitly.Current tests validate only a subset of symbols. Adding one exact
__all__assertion will better protect the cross-app boundary from accidental API drift.Proposed test hardening
from github_activity_tracker import sync_api +def test_sync_api_public_contract_is_stable(): + expected = { + "build_issue_document", + "build_pr_document", + "fetcher", + "get_commit_json_path", + "get_issue_json_path", + "get_pr_json_path", + "get_raw_source_issue_path", + "get_raw_source_pr_path", + "iter_existing_commit_jsons", + "iter_existing_issue_jsons", + "iter_existing_pr_jsons", + "normalize_issue_json", + "normalize_pr_json", + "save_commit_raw_source", + "save_issue_raw_source", + "save_pr_raw_source", + } + assert set(sync_api.__all__) == expected + for name in expected: + assert hasattr(sync_api, name) + def test_sync_api_exports_fetcher(): assert sync_api.fetcher is not None🤖 Prompt for 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. In `@github_activity_tracker/tests/test_sync_api.py` around lines 6 - 29, Add a strict export contract assertion: check sync_api.__all__ exactly matches the expected list of public symbols to prevent accidental API drift. In the test_sync_api.py add an assertion like comparing tuple(getattr(sync_api, "__all__", None)) or set(sync_api.__all__) to the explicit list: ["fetcher","normalize_issue_json","normalize_pr_json","save_commit_raw_source","save_issue_raw_source","save_pr_raw_source","get_commit_json_path","get_raw_source_issue_path","iter_existing_pr_jsons","build_issue_document","build_pr_document"]; if __all__ is missing make the test fail so authors add/maintain it.core/_version.py (1)
1-2: 💤 Low valueAuto-generated version file in version control.
This file is generated by
setuptools-scm(per line 1 comment andpyproject.tomllines 17-22). Modern Python packaging best practices typically exclude generated version files from version control, adding them to.gitignoreand letting the build process generate them.However,
pyproject.tomlexplicitly configuresversion_file = "core/_version.py", indicating the project intends to commit this file. If this is intentional (e.g., for environments without git metadata), this is acceptable but will cause version string changes in every commit.Consider whether this file should be in
.gitignoreor if the current approach serves a specific deployment need.🤖 Prompt for 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. In `@core/_version.py` around lines 1 - 2, The repo contains an auto-generated version file core/_version.py with the variable version = "0.1.1.dev0+gfcfdda0ab.d20260525" committed; decide whether to keep committing this generated file or ignore it: if you want builds to generate the version at build time, remove core/_version.py from version control and add it to .gitignore (and ensure pyproject.toml no longer requires a committed file), otherwise document and accept that pyproject.toml's version_file = "core/_version.py" is intentional (keep the file committed) and add a note to the repo README explaining why the generated file is checked in for environments lacking git metadata..importlinter (1)
2-17: 💤 Low valueConsider adding
slack_event_handlerto root_packages.The app inventory in
docs/cross-app-dependencies.mdlistsslack_event_handleras an installed app. While it has no domain models and no current import contracts, including it inroot_packageswould make the configuration complete and future-proof against accidental cross-app imports.📦 Proposed addition
github_activity_tracker + slack_event_handler wg21_paper_tracker🤖 Prompt for 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. In @.importlinter around lines 2 - 17, The .importlinter configuration's root_packages list is missing slack_event_handler; add slack_event_handler to the root_packages block (the root_packages key in the .importlinter file) so the package list includes "slack_event_handler" to match docs/cross-app-dependencies.md and prevent accidental cross-app import issues.
🤖 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.
Nitpick comments:
In @.importlinter:
- Around line 2-17: The .importlinter configuration's root_packages list is
missing slack_event_handler; add slack_event_handler to the root_packages block
(the root_packages key in the .importlinter file) so the package list includes
"slack_event_handler" to match docs/cross-app-dependencies.md and prevent
accidental cross-app import issues.
In `@core/_version.py`:
- Around line 1-2: The repo contains an auto-generated version file
core/_version.py with the variable version = "0.1.1.dev0+gfcfdda0ab.d20260525"
committed; decide whether to keep committing this generated file or ignore it:
if you want builds to generate the version at build time, remove
core/_version.py from version control and add it to .gitignore (and ensure
pyproject.toml no longer requires a committed file), otherwise document and
accept that pyproject.toml's version_file = "core/_version.py" is intentional
(keep the file committed) and add a note to the repo README explaining why the
generated file is checked in for environments lacking git metadata.
In `@github_activity_tracker/tests/test_sync_api.py`:
- Around line 6-29: Add a strict export contract assertion: check
sync_api.__all__ exactly matches the expected list of public symbols to prevent
accidental API drift. In the test_sync_api.py add an assertion like comparing
tuple(getattr(sync_api, "__all__", None)) or set(sync_api.__all__) to the
explicit list:
["fetcher","normalize_issue_json","normalize_pr_json","save_commit_raw_source","save_issue_raw_source","save_pr_raw_source","get_commit_json_path","get_raw_source_issue_path","iter_existing_pr_jsons","build_issue_document","build_pr_document"];
if __all__ is missing make the test fail so authors add/maintain it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e61a19b-9577-46a5-a2f5-313ff90d2d70
⛔ Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.importlinter.pre-commit-config.yamlCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdboost_library_docs_tracker/management/commands/run_boost_library_docs_tracker.pyboost_library_docs_tracker/tests/test_run_boost_library_docs_tracker_command.pyboost_library_usage_dashboard/models.pyboost_library_usage_dashboard/tests/test_models.pyboost_usage_tracker/update_repository_from_csv.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/sync_raw.pycore/_version.pycppa_pinecone_sync/services.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/tests/test_run_command.pycppa_user_tracker/services.pycppa_user_tracker/tests/test_services.pydocs/Development_guideline.mddocs/cross-app-dependencies.mddocs/service_api/cppa_pinecone_sync.mddocs/service_api/cppa_user_tracker.mddocs/service_api/github_activity_tracker.mdgithub_activity_tracker/sync_api.pygithub_activity_tracker/tests/test_sync_api.pypyproject.tomlpyrightconfig.jsonrequirements-dev.inscripts/validate_collector_scaffold.py
💤 Files with no reviewable changes (1)
- boost_library_usage_dashboard/models.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@cppa_pinecone_sync/services.py`:
- Around line 22-47: sync_source_to_pinecone currently forwards the instance
verbatim; validate and normalize it before calling sync_to_pinecone: import
PineconeInstance, set pinecone_instance = PineconeInstance.PUBLIC when instance
is None, allow string names by mapping them to PineconeInstance (e.g.
PineconeInstance[instance] or by value) and if the provided instance is not a
PineconeInstance or mappable string raise a clear ValueError indicating allowed
values; finally pass the normalized pinecone_instance into sync_to_pinecone
(refer to sync_source_to_pinecone, PineconeInstance.PUBLIC, parameter instance,
and sync_to_pinecone).
In `@docs/cross-app-dependencies.md`:
- Around line 170-183: Update this page so the summary table and the earlier
detailed rows are consistent: find the earlier section(s) that still list
boost_library_usage_dashboard as a shim and the script
update_repository_from_csv.py as unresolved tech-debt and change their state to
match the "resolved" summary (i.e., remove/mark the shim and
update_repository_from_csv.py entries as resolved), ensuring mentions of
boost_library_docs_tracker, cppa_slack_tracker, boost_usage_tracker (CSV
import/GitHubAccount), and clang_github_tracker align with the summary; keep the
resolved wording and CI/import-linter note unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 532a41f3-4ff6-4482-93eb-56313384d881
⛔ Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.importlinter.pre-commit-config.yamlCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdboost_library_docs_tracker/management/commands/run_boost_library_docs_tracker.pyboost_library_docs_tracker/tests/test_run_boost_library_docs_tracker_command.pyboost_library_usage_dashboard/models.pyboost_library_usage_dashboard/tests/test_models.pyboost_usage_tracker/update_repository_from_csv.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/sync_raw.pycore/_version.pycppa_pinecone_sync/services.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/tests/test_run_command.pycppa_user_tracker/services.pycppa_user_tracker/tests/test_services.pydocs/Development_guideline.mddocs/cross-app-dependencies.mddocs/service_api/cppa_pinecone_sync.mddocs/service_api/cppa_user_tracker.mddocs/service_api/github_activity_tracker.mdgithub_activity_tracker/sync_api.pygithub_activity_tracker/tests/test_sync_api.pypyproject.tomlpyrightconfig.jsonrequirements-dev.inscripts/validate_collector_scaffold.py
💤 Files with no reviewable changes (1)
- boost_library_usage_dashboard/models.py
…ameter validation
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.importlinter (1)
2-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
coreto.importlinterroot_packages
core/exists, and many packages in the currentroot_packageslist import fromcore.*(collectors/operations/utils/protocols/errors, etc.). Leavingcoreout ofroot_packagesprevents import-linter from analyzing/flagging cross-boundary violations involvingcore.🤖 Prompt for 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. In @.importlinter around lines 2 - 17, Add "core" to the root_packages list in the .importlinter config so import-linter will analyze imports to and from core; update the root_packages block (the list containing boost_collector_runner, boost_library_docs_tracker, etc.) to include core as a top-level package name to ensure cross-boundary violations involving core.* are detected.
🧹 Nitpick comments (1)
.importlinter (1)
29-29: 💤 Low valueConsider eventually tightening
allow_indirect_importstofalse.All three contracts currently set
allow_indirect_imports = true, which permits violations through transitive imports (e.g., ifapp_aimportshelper_b, andhelper_bimports the forbidden module, the violation is not caught). While this setting is appropriate during the initial migration to reduce false positives, you may want to tighten these contracts toallow_indirect_imports = falseonce the direct imports are fully resolved and the service boundaries are stable. This will provide stronger enforcement against future violations.Also applies to: 38-38, 50-50
🤖 Prompt for 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. In @.importlinter at line 29, The import linter config currently sets the boolean option allow_indirect_imports = true in three contract blocks which permits transitive import violations; change those three occurrences of allow_indirect_imports from true to false to enforce direct+indirect import restrictions, run the import-linter check suite to surface transitive violations, and iteratively fix or explicitly whitelist any legitimate exceptions (or add narrow contract rules) until the linter is clean.
🤖 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 `@cppa_pinecone_sync/types.py`:
- Around line 21-25: The enum coercion currently lowercases string inputs but
doesn't trim whitespace, causing valid inputs like " PRIVATE " to raise
ValueError; update the coercion in the relevant enum class method (the block
that calls cls(instance.lower())) to normalize the input by stripping
surrounding whitespace first (use instance.strip().lower()) before attempting
cls(...), and keep the existing ValueError raising behavior on failure so inputs
with only surrounding whitespace are handled correctly.
---
Outside diff comments:
In @.importlinter:
- Around line 2-17: Add "core" to the root_packages list in the .importlinter
config so import-linter will analyze imports to and from core; update the
root_packages block (the list containing boost_collector_runner,
boost_library_docs_tracker, etc.) to include core as a top-level package name to
ensure cross-boundary violations involving core.* are detected.
---
Nitpick comments:
In @.importlinter:
- Line 29: The import linter config currently sets the boolean option
allow_indirect_imports = true in three contract blocks which permits transitive
import violations; change those three occurrences of allow_indirect_imports from
true to false to enforce direct+indirect import restrictions, run the
import-linter check suite to surface transitive violations, and iteratively fix
or explicitly whitelist any legitimate exceptions (or add narrow contract rules)
until the linter is clean.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33722aee-fd97-406e-9e14-b15c7a17d740
📒 Files selected for processing (20)
.importlinterboost_library_docs_tracker/management/commands/run_boost_library_docs_tracker.pyboost_library_docs_tracker/tests/test_run_boost_library_docs_tracker_command.pycore/_version.pycppa_pinecone_sync/README.mdcppa_pinecone_sync/__init__.pycppa_pinecone_sync/ingestion.pycppa_pinecone_sync/management/commands/run_cppa_pinecone_sync.pycppa_pinecone_sync/sync.pycppa_pinecone_sync/sync_api.pycppa_pinecone_sync/tests/test_ingestion.pycppa_pinecone_sync/tests/test_sync.pycppa_pinecone_sync/tests/test_sync_api.pycppa_pinecone_sync/tests/test_types.pycppa_pinecone_sync/types.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/tests/test_run_command.pydocs/Pinecone_preprocess_guideline.mddocs/cross-app-dependencies.mddocs/service_api/cppa_pinecone_sync.md
✅ Files skipped from review due to trivial changes (7)
- cppa_pinecone_sync/tests/test_ingestion.py
- cppa_pinecone_sync/init.py
- core/_version.py
- cppa_pinecone_sync/README.md
- cppa_pinecone_sync/sync_api.py
- docs/Pinecone_preprocess_guideline.md
- docs/cross-app-dependencies.md
…eject empty strings
jonathanMLDev
approved these changes
May 26, 2026
wpak-ai
approved these changes
May 26, 2026
8 tasks
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
3 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
Resolves the five cross-app import tech-debt edges documented in
docs/cross-app-dependencies.mdand enforces them in CI with import-linter.boost_library_docs_tracker,cppa_slack_tracker) callcppa_pinecone_sync.services.sync_source_to_pineconeinstead of importingsync/ingestioninternals.boost_usage_tracker.update_repository_from_csvusescppa_user_tracker.services.get_github_account_by_usernameinstead of queryingGitHubAccountdirectly.clang_github_trackerimports orchestration helpers only from newgithub_activity_tracker.sync_api(notfetcher,sync.*,workspace, orpreprocessors).boost_library_usage_dashboard/models.pyre-export shim; callers useboost_usage_tracker.modelsdirectly..importlintercontracts,import-linterdev dependency, and pre-commitlint-importshook.pyproject.toml, README, CONTRIBUTING).Test plan
uv run lint-imports(or pre-commitimport-linterhook) passesuv run pyrightpassespytest github_activity_tracker/tests/test_sync_api.pypytest cppa_user_tracker/tests/test_services.pypytest cppa_slack_tracker/tests/test_run_command.pypytest boost_library_docs_tracker/tests/test_run_boost_library_docs_tracker_command.pypytest boost_library_usage_dashboard/tests/test_models.pyrun_boost_library_docs_tracker/run_cppa_slack_trackerPinecone sync path (if Pinecone env available)clang_github_trackersync/backfill still runs againstsync_apiimportsCloses #229
Summary by CodeRabbit
New Features
Documentation
Chores