fix: support JSONC comments and monorepo tsconfig.json discovery#485
fix: support JSONC comments and monorepo tsconfig.json discovery#485aviavraham wants to merge 2 commits into
Conversation
…ient-code#431, ambient-code#383) The TypeScript type annotation assessor now finds all tsconfig.json files recursively (excluding node_modules) and strips // and /* */ comments before parsing, fixing false 0-scores on real-world TypeScript repos. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR fixes monorepo TypeScript assessment by discovering and scoring all ChangesMonorepo TypeScript Assessment
Possibly related issues
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 |
📈 Test Coverage Report
Coverage calculated from unit tests only |
![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
🤖 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 `@docs/attributes.md`:
- Around line 273-275: Update the "tsconfig strict" attribute description in
docs/attributes.md to list all excluded paths recognized by the assessor (not
just node_modules); specifically add vendor and testdata to the excluded-paths
documentation and ensure the scoring note about proportional strict-enabled
configs (e.g., "2/3 strict = 67%") and any partial-credit or pass/fail rules
remain consistent with the assessor logic. Match wording/format to the existing
bullets so the docs stay in sync with the assessor’s recognized paths and
scoring rules.
In `@src/agentready/assessors/code_quality.py`:
- Around line 336-344: The current _find_tsconfig_files method uses
repository.path.rglob which still descends into large excluded trees; change it
to a prunable walk (e.g., use os.walk(repository.path) or Path.iterdir with
recursive stack) and remove "node_modules", "vendor", and "testdata" from the
directories list before recursing so those trees are never traversed; collect
any tsconfig.json files you encounter into found (preserving return of sorted
list) and keep function name _find_tsconfig_files and repository.path as the
location to modify.
- Around line 187-214: The code currently skips malformed tsconfig files by
continuing inside the except block which prevents them from affecting
total_count; instead, in the except (OSError, json.JSONDecodeError) handler
(referencing the errors list, total_count, strict_count, evidence variables),
stop using continue and increment total_count for that file, append a failure
evidence entry (e.g. f"{rel_path}: parse error ({e})") and treat it as
non-strict (do not increment strict_count) so malformed configs count against
the measured ratio; ensure the later score/status calculation
(calculate_proportional_score and status logic) naturally penalizes these files,
or alternatively if you prefer to surface hard failures return a Finding.error
when errors is non-empty after processing all files (adjust the final
total_count==0 check accordingly).
- Around line 181-197: The evaluator currently reads each tsconfig file directly
and checks compilerOptions.strict without resolving "extends"; update the logic
that iterates over tsconfig_files (the loop using tsconfig_path, rel_path, raw,
cleaned and json.loads and calling self._strip_json_comments) to resolve and
merge extended configs so the effective compilerOptions.strict is computed
(handle relative extends, absolute paths, node-style package references, merge
parent compilerOptions into child, detect and prevent circular extends), then
use that effective compilerOptions.strict for strict_count/evidence; also add
unit tests in tests/unit/test_assessors_typescript.py that include a base
tsconfig with "strict": true and a child that extends it to assert the assessor
treats the child as strict.
🪄 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: 809945c2-3d31-4ed1-8556-91c4fdae42fa
📒 Files selected for processing (3)
docs/attributes.mdsrc/agentready/assessors/code_quality.pytests/unit/test_assessors_typescript.py
| for tsconfig_path in tsconfig_files: | ||
| rel_path = str(tsconfig_path.relative_to(repository.path)) | ||
| try: | ||
| raw = tsconfig_path.read_text(encoding="utf-8") | ||
| cleaned = self._strip_json_comments(raw) | ||
| tsconfig = json.loads(cleaned) | ||
| except (OSError, json.JSONDecodeError) as e: | ||
| errors.append(f"{rel_path}: parse error ({e})") | ||
| continue | ||
|
|
||
| total_count += 1 | ||
| strict = tsconfig.get("compilerOptions", {}).get("strict", False) | ||
|
|
||
| if strict: | ||
| return Finding( | ||
| attribute=self.attribute, | ||
| status="pass", | ||
| score=100.0, | ||
| measured_value="strict mode enabled", | ||
| threshold="strict mode enabled", | ||
| evidence=["tsconfig.json has strict: true"], | ||
| remediation=None, | ||
| error_message=None, | ||
| ) | ||
| strict_count += 1 | ||
| evidence.append(f"{rel_path}: strict: true") | ||
| else: | ||
| return Finding( | ||
| attribute=self.attribute, | ||
| status="fail", | ||
| score=50.0, | ||
| measured_value="strict mode disabled", | ||
| threshold="strict mode enabled", | ||
| evidence=["tsconfig.json missing strict: true"], | ||
| remediation=self._create_remediation(), | ||
| error_message=None, | ||
| ) | ||
| evidence.append(f"{rel_path}: strict mode disabled") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TypeScript assessor logic:"
sed -n '181,197p' src/agentready/assessors/code_quality.py
echo
echo 'Search for any tsconfig "extends" handling in the assessor and tests:'
rg -n --type=py '\bextends\b|compilerOptions.*strict|_strip_json_comments|_find_tsconfig_files' \
src/agentready/assessors/code_quality.py \
tests/unit/test_assessors_typescript.pyRepository: ambient-code/agentready
Length of output: 2357
Fix TypeScript strict scoring to account for tsconfig inheritance (extends).
src/agentready/assessors/code_quality.py only scores strict from each tsconfig.json’s direct compilerOptions.strict (tsconfig.get("compilerOptions", {}).get("strict", False), lines 181-197). There’s no "extends" handling in the assessor (or in tests/unit/test_assessors_typescript.py), so leaf configs that inherit strict from a base config will be incorrectly treated as non-strict. Add extends resolution (effective compilerOptions) and cover this with unit tests for inherited strict.
🤖 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 `@src/agentready/assessors/code_quality.py` around lines 181 - 197, The
evaluator currently reads each tsconfig file directly and checks
compilerOptions.strict without resolving "extends"; update the logic that
iterates over tsconfig_files (the loop using tsconfig_path, rel_path, raw,
cleaned and json.loads and calling self._strip_json_comments) to resolve and
merge extended configs so the effective compilerOptions.strict is computed
(handle relative extends, absolute paths, node-style package references, merge
parent compilerOptions into child, detect and prevent circular extends), then
use that effective compilerOptions.strict for strict_count/evidence; also add
unit tests in tests/unit/test_assessors_typescript.py that include a base
tsconfig with "strict": true and a child that extends it to assert the assessor
treats the child as strict.
| def _find_tsconfig_files(self, repository: Repository) -> list: | ||
| """Find all tsconfig.json files, excluding node_modules/vendor/testdata.""" | ||
| found = [] | ||
| for tsconfig in repository.path.rglob("tsconfig.json"): | ||
| parts = tsconfig.parts | ||
| if "node_modules" in parts or "vendor" in parts or "testdata" in parts: | ||
| continue | ||
| found.append(tsconfig) | ||
| return sorted(found) |
There was a problem hiding this comment.
Prune excluded directories during traversal.
This still walks node_modules / vendor / testdata and only filters matching files afterward. On large monorepos, descending into those trees can dominate assessor runtime and defeats the intended exclusion behavior. Use a prunable walk (os.walk/Path.walk) and remove excluded dirs before recursing.
🤖 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 `@src/agentready/assessors/code_quality.py` around lines 336 - 344, The current
_find_tsconfig_files method uses repository.path.rglob which still descends into
large excluded trees; change it to a prunable walk (e.g., use
os.walk(repository.path) or Path.iterdir with recursive stack) and remove
"node_modules", "vendor", and "testdata" from the directories list before
recursing so those trees are never traversed; collect any tsconfig.json files
you encounter into found (preserving return of sorted list) and keep function
name _find_tsconfig_files and repository.path as the location to modify.
- Document all excluded paths (node_modules, vendor, testdata) in attributes.md - Count malformed tsconfig.json as non-strict instead of silently skipping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the review! Addressed #1 and #3 in the latest push. For the remaining two: Re: Re: Pruning excluded dirs during traversal (#4): |
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 `@docs/attributes.md`:
- Around line 273-275: Update the tsconfig strict-mode scoring description in
the listed bullets so it states that only 100% strict-enabled tsconfig.json
files result in a "pass" (partial scores like "2/3 = 67%" are reported but do
not pass), and explicitly note that malformed tsconfig.json files are treated as
non-strict; edit the lines that currently read "Scoring: proportional to
strict-enabled configs (e.g., 2/3 strict = 67%)" to mention the proportional
score is shown but the assessor requires 100% to return pass and malformed
configs count as non-strict so readers see the implementation matches the docs.
🪄 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: 9b144d19-d675-4aa3-ba9a-c10e764f2bbd
📒 Files selected for processing (3)
docs/attributes.mdsrc/agentready/assessors/code_quality.pytests/unit/test_assessors_typescript.py
| - `strict` mode enabled in tsconfig.json (supports JSONC with comments) | ||
| - All tsconfig.json files checked (monorepo support: root + subdirectories, excluding node_modules, vendor, testdata) | ||
| - Scoring: proportional to strict-enabled configs (e.g., 2/3 strict = 67%) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document that partial strict scores still fail.
This now reads like 2/3 strict = 67% is the user-facing outcome, but the assessor only returns pass when every discovered tsconfig.json is strict and treats malformed configs as non-strict. Please add that 100% is required to pass so the docs match the implementation.
As per coding guidelines, "Keep docs/attributes.md in sync when changing assessor scoring logic, thresholds, partial credit rules, recognized paths, or pass/fail conditions"
🤖 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 `@docs/attributes.md` around lines 273 - 275, Update the tsconfig strict-mode
scoring description in the listed bullets so it states that only 100%
strict-enabled tsconfig.json files result in a "pass" (partial scores like "2/3
= 67%" are reported but do not pass), and explicitly note that malformed
tsconfig.json files are treated as non-strict; edit the lines that currently
read "Scoring: proportional to strict-enabled configs (e.g., 2/3 strict = 67%)"
to mention the proportional score is shown but the assessor requires 100% to
return pass and malformed configs count as non-strict so readers see the
implementation matches the docs.
Verification against reported reposRan the updated assessor against both repos from the original issues: renovatebot/renovate (#383): podman-desktop/extension-bootc (#431): Both previously scored 0/100. |
1 participant
Summary
Fixes #431 and #383.
//and/* */comments before parsingtsconfig.json(previouslyjson.load()would choke on comments, common in real-world TypeScript projects like renovatebot/renovate)tsconfig.jsonfiles recursively viarglob, excludingnode_modules/vendor/testdata(previously only checked repo root, failing monorepos like podman-desktop/extension-bootc)Test plan
test_assessors_typescript.pycovering root strict, JSONC, monorepo, node_modules exclusion, malformed JSONSummary by CodeRabbit
Documentation
Bug Fixes
Tests