Fix detection of Golang project layout#435
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughBaseAssessor._primary_language() now prefers root-level manifests (expanded mappings include TS and C#), with JS/TS tie-breaks and refined fallbacks. Unit tests and a Go layout integration test validate manifest precedence and file-count fallback behavior. ChangesLanguage Primary-Detection Enhancement with Manifest-Based Prioritization
Possibly Related PRs
Suggested Labels
🚥 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)
✨ Simplify code
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: 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 `@tests/unit/test_assessors_structure.py`:
- Around line 908-912: The current assertion using evidence_str is too
permissive; update the check so that when "src/" appears it must be accompanied
by clear Go indicators. Replace the single permissive assert on evidence_str
with logic: if "src/" in evidence_str.lower(): assert "source" not in
evidence_str.lower() and ( "go.mod" in evidence_str.lower() or "module " in
evidence_str.lower() or re.search(r"\bpackage\s+\w+", evidence_str.lower()) ),
otherwise allow evidence_str to omit "src/". This uses the existing evidence_str
variable to ensure "src/" presence actually reflects a Go layout.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 3abdf512-a64f-4f94-b0f6-60dd342d48db
📒 Files selected for processing (2)
src/agentready/assessors/structure.pytests/unit/test_assessors_structure.py
mmorhun
force-pushed
the
fix-golang-dir-layout
branch
from
984d11e to
58a4921
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
mmorhun
force-pushed
the
fix-golang-dir-layout
branch
from
58a4921 to
24981e9
Compare
|
Rebased and resolved merge conflicts. |
natifridman
left a comment
natifridman
left a comment
There was a problem hiding this comment.
Code Review
Overall: Good bug fix with solid tests. The core idea — using go.mod as a definitive signal instead of file counts — is the right call.
The bug (confirmed valid)
_primary_language() uses file counts with a 30% threshold for tiebreaking. In a Go operator repo with many Python scripts, if Python files outnumber Go by more than 30%, the manifest tiebreaker never fires, and the assessor evaluates Python layout rules (src/ + tests/) against a Go project — guaranteed fail.
The fix
Replacing _primary_language(repository, {"Python", "Go"}) == "Go" with _find_go_module_roots(repository) bypasses the file-count heuristic entirely. go.mod presence is treated as definitive. This is correct — go.mod is the canonical Go project marker.
Concerns
1. Overly broad go.mod detection — _find_go_module_roots() finds go.mod at any depth (excluding vendor/testdata). A Python project with a Go tool in a subdirectory (e.g., tools/linter/go.mod) would now route to Go assessment, which is likely wrong. A safer fix might be to check root-level go.mod only:
if (repository.path / "go.mod").exists():
return self._assess_go_layout(repository)This still fixes the reported bug (Go operators have go.mod at root) while avoiding false positives from subdirectory Go modules in non-Go projects.
2. Other assessors have the same vulnerability — _primary_language() is still used by code_quality.py (lines 62, 445, 719), documentation.py (line 1203), and testing.py (line 59). Those assessors face the same Python-files-outnumber-Go-files issue for the same types of repos. This PR fixes structure.py only. Not a blocker, but worth tracking.
3. Overly complex test assertion — The last assertion block in test_go_project_with_python_scripts_uses_go_assessment (lines ~908-912) is hard to follow and tests a condition that can't actually trigger — the Go assessor doesn't produce evidence containing "src/". The assertion will always be vacuously true. Consider dropping it for clarity, or simplifying to just asserting "source" not in evidence_str.lower().
What's good
- The bug is real and well-demonstrated (before/after in PR description)
- Tests cover both the simple case and the exact bug scenario
- Minimal diff in production code (3 lines changed)
🤖 Generated with Claude Code
Signed-off-by: Mykola Morhun <mmorhun@redhat.com> Assisted-by: Claude
mmorhun
force-pushed
the
fix-golang-dir-layout
branch
from
24981e9 to
0d09a0b
Compare
|
@coderabbitai review |
✅ Actions performedReview 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 `@tests/unit/test_assessors_base.py`:
- Line 30: The test currently shells out to "git init" which is unnecessary for
testing _primary_language(); remove the subprocess.run(["git", "init"], ...)
call and instead stub/mock the code path that queries git: patch the
helper/function that invokes git (the _primary_language() call or the
subprocess.run wrapper it uses) to return the expected language value, or create
the minimal file(s) the function inspects directly so no git command is
executed; apply this same replacement for all other instances of
subprocess.run(["git", "init"], ...) in the file so tests no longer depend on an
external git binary.
- Around line 21-23: The assess method in ConcreteAssessor (def assess(self,
repository: Repository) -> Finding) references self.attribute which doesn't
exist on BaseAssessor; update the method to use the correct attribute name
self.attribute_id when calling Finding.create_pass (or otherwise pass the
intended attribute identifier) so the call uses a defined property and doesn't
raise an AttributeError.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 4cb62baf-5961-4f52-9c37-e8804e3fdbe8
📒 Files selected for processing (3)
src/agentready/assessors/base.pytests/unit/test_assessors_base.pytests/unit/test_assessors_structure.py
📈 Test Coverage Report
Coverage calculated from unit tests only |
Signed-off-by: Mykola Morhun <mmorhun@redhat.com> Assisted-by: Claude
mmorhun
force-pushed
the
fix-golang-dir-layout
branch
from
0d09a0b to
d4295f2
Compare
|
@coderabbitai review |
✅ Actions performedReview 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 `@src/agentready/assessors/base.py`:
- Line 77: The TypeScript entry in the language detection map is being triggered
by package.json alone causing false positives; update the detection logic that
builds the candidates set so that "TypeScript" is only added when tsconfig.json
is present or when .ts/.tsx source files are detected (not when only
package.json exists). Specifically, change the code that uses the mapping
containing "TypeScript": ["package.json", "tsconfig.json"] and the branch that
checks the resulting candidates set (including the block referencing candidates
== {"TypeScript"}) to exclude TypeScript if the only match is package.json, and
mirror the same fix in the second occurrence around the other candidates-check.
In `@tests/unit/test_assessors_base.py`:
- Around line 71-136: Add a regression test that covers the TS-only candidate
edge case: create a new test (e.g.,
test_typescript_only_candidate_without_tsconfig) in
tests/unit/test_assessors_base.py that writes only package.json (no
tsconfig.json), patches Repository.__post_init__ as in the other tests,
constructs a Repository with languages={"TypeScript": N} (no "JavaScript"), and
calls ConcreteAssessor()._primary_language(repo, {"TypeScript"}) and assert it
returns "TypeScript"; use the same Repository construction pattern and patching
used by the surrounding tests to locate the correct setup.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: f236282d-2906-493a-a2c0-1e5ad8cf2b75
📒 Files selected for processing (3)
src/agentready/assessors/base.pytests/unit/test_assessors_base.pytests/unit/test_assessors_structure.py
💤 Files with no reviewable changes (1)
- tests/unit/test_assessors_structure.py
|
I'll deal with the above next day. |
2 participants
Assisted-by: Claude
Description
Type of Change
Related Issues
Relates to #378
Changes Made
go.mod(or any other language project files) found in the project root, assume we deal with a project in that language. If none or a few markers found, rely on old logic.Testing
pytest)Checklist
Screenshots (if applicable)
Additional Notes
Before the fix on an Golang k8s operator:
After the fix:
Summary by CodeRabbit
Bug Fixes
Tests