Repository navigation
Conversation
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…d rules dir, claude_rules key Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…irst Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
🔍 OpenCodeReview found 3 issue(s) in this PR. By severity: 3 medium
Review details
|
| > `.claude/rules/**/*.md` files are `rule` entries: at start in the root `.claude/rules/` unless | ||
| > their frontmatter's `paths` names a pattern, lazily then and in a subdirectory's `.claude/rules/`; | ||
| > an `@` path in a rule is not followed. The `start` total is not an upper bound: it is too low by |
There was a problem hiding this comment.
Must-fix: these rules omit the case where guidance imports a rule. For example, with CLAUDE.md containing @.claude/rules/api.md and api.md containing paths: src/** frontmatter, repo budget reports the rule as start, not lazy (also asserted by test_a_rule_a_root_claude_md_imports_is_one_start_entry). If that rule contains @extra.md, _imports also follows it transitively. Qualify these statements to distinguish automatic rule discovery from guidance imports, or adjust the implementation if the unconditional no-import behavior is intended. The same missing start-classification exception appears in docs/architecture.md and the changelog; the changelog also repeats the unconditional no-import claim.
There was a problem hiding this comment.
Waiting on a decision before I change this: is the harness's @ expansion inside a rule that a root CLAUDE.md imports part of the behaviour we document? Context: the code follows those @ paths transitively, as it does for any imported file, and counts them as start imports; the docs say an @ path in a rule is not followed, without the imported-rule case. Options: (a) keep the current behaviour and state it in the docs, with the start classification of imported rules; (b) stop expansion at an imported rule. My recommendation: measure it on Claude Code with repo reach before choosing; I can make the doc change in the next round once it is answered. The thread stays open until then.
There was a problem hiding this comment.
On the question in this thread: I would keep the current code (option a) and fix the docs. Claude Code's memory docs say an imported file can import more files, up to four hops; a file a CLAUDE.md @-imports is loaded as an import, whatever directory it is in. What 2.1.304 measured is a rule loaded as a rule (@extra.md not expanded), and that does not cover a rule a CLAUDE.md imports. With the PR code, CLAUDE.md = @.claude/rules/api.md, api.md with paths: src/** and see @../../extra.md gives api.md start and extra.md a start import, which matches the docs. So the docs should say: "an @ path in a rule is not followed, unless a CLAUDE.md imports the rule: then the rule loads at start and its imports are followed" (here, in architecture.md and in the changelog fragment). One thing this does not settle: when imported, the rule's frontmatter is probably not removed, but it is not counted now. repo reach on a probe repo would measure both (a few haiku calls, cents).
| unreadable_warnings.append(warning) | ||
| entries.append({"path": f, "kind": "guidance", "when": when, "chars": chars}) | ||
| counted = {_target(tree, f) for f in guidance} | ||
| entries += _rules(tree, files if rules else [], counted, start_t, unreadable_warnings) |
There was a problem hiding this comment.
Must-fix: rule discovery happens after _imports has already traversed the rule's body, so the promised non-expansion of a rule's @ paths is not enforced for imported rules. For example, CLAUDE.md containing @.claude/rules/api.md, .claude/rules/api.md containing @../../extra.md, and extra.md containing extra\n produces a startup import entry for extra.md. _rules only deduplicates the rule itself; it cannot remove that transitive import or its hop-limit warnings. Make import traversal aware of rules when this profile feature is enabled, stopping expansion at a rule while still counting the explicitly imported rule, and add a regression test for this chain.
There was a problem hiding this comment.
Waiting on a decision before I change this: the same question as in the docs thread, since this one asks for the non-expansion to be enforced. Should a @ path in a rule that a root CLAUDE.md imports be followed (current code, transitively, hop-limited) or not (your proposal)? My recommendation: measure it on Claude Code first; I will make the change and add the regression test the same round once you pick. The thread stays open until then.
| if _target(tree, f) in counted: | ||
| continue # a link to a file already counted, or a rule that is also a guidance file |
There was a problem hiding this comment.
Must-fix: deduplicating against guidance before considering the rule's loading time loses startup context. For example, create CLAUDE.md with no imports, AGENTS.md containing agents\n, and .claude/rules/agents.md symlinked to ../../AGENTS.md. The guidance entry is classified off, then this branch skips the unconditional root rule, leaving its 7 characters out of start and incorrectly warning that none of AGENTS.md is read. Similarly, a root rule linked to sub/CLAUDE.md leaves that content classified lazy. Merge the loading conditions of rule and guidance aliases before deduplicating (promoting the retained entry to start when appropriate), and add regression tests for both cases.
There was a problem hiding this comment.
Fixed in 21e287c: a rule linked to a guidance file is the same bytes, so the guidance entry now takes the earlier of the two loading conditions instead of the rule being dropped. The AGENTS.md a root rule links to is start, and its unread warning is gone; a rule linked to sub/CLAUDE.md counts that file at start. Pinned by test_a_root_rule_linked_to_an_off_agents_md_loads_it_at_start and test_a_root_rule_linked_to_a_nested_guidance_file_loads_it_at_start.
| """True if ``paths`` names at least one pattern: a list item or an inline value not ``[]`` or ``""``.""" | ||
| try: | ||
| meta = yaml.safe_load(frontmatter) | ||
| except (yaml.YAMLError, RecursionError): # RecursionError: nesting too deep for the parser |
There was a problem hiding this comment.
Must-fix: yaml.safe_load can also raise ValueError when constructing an oversized decimal integer. On supported Python 3.11+ with the default conversion limit, a rule whose frontmatter is "unused: " + "9" * 5000 + "\n" crashes repo budget, even though it has no paths field. Neither _rule nor the command catches this parser exception. Include ValueError in the fallback handling and add a regression test with an oversized scalar unrelated to paths.
Suggestion:
| except (yaml.YAMLError, RecursionError): # RecursionError: nesting too deep for the parser | |
| except (yaml.YAMLError, RecursionError, ValueError): # invalid YAML, excessive nesting or oversized integer |
There was a problem hiding this comment.
Fixed in 21e287c: the frontmatter parser's fallback catches ValueError too (yaml raises it for an integer over the digit limit), so an oversized scalar unrelated to paths no longer crashes budget. Pinned by oversized-integer-unrelated-to-paths.
| values = [m["inline"]] + [line.split("-", 1)[1] for line in m["items"].splitlines() if line.strip()] | ||
| return any(v.strip().strip("[]").strip(" \t\"'") for v in values) |
There was a problem hiding this comment.
Must-fix: the malformed-YAML fallback treats comments as patterns and stops collecting list items at comment lines. For example, paths: # none yet\nfoo: [unclosed\n returns True despite naming no pattern, whereas paths:\n # the api\n - "src/**"\nfoo: [unclosed\n returns False despite naming one. Root rules therefore land in the wrong start/lazy totals. Make fallback extraction ignore YAML comments (without stripping quoted # characters) and allow blank/comment lines between list items; add both malformed-frontmatter cases to the parametrized tests.
There was a problem hiding this comment.
Fixed in 21e287c: when the frontmatter does not parse, the paths fallback cuts YAML comments from each value (a # that starts a word, outside quotes, so "src/#x" keeps it) and skips comment and blank lines between items. paths: # none yet is start; a comment line before an item is lazy. Pinned by the unparsed-* cases of test_only_a_paths_value_that_names_a_pattern_scopes_a_rule.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
+ Coverage 94.05% 94.06% +0.01%
==========================================
Files 297 297
Lines 38018 38101 +83
Branches 6424 6443 +19
==========================================
+ Hits 35756 35838 +82
- Misses 1452 1453 +1
Partials 810 810
🚀 New features to boost your workflow:
|
…ng time A root rule that links to a guidance file (or to a file a root CLAUDE.md turns off) is the same bytes: the guidance entry now takes the earlier of the two loading conditions instead of the rule being dropped, so the file is counted at start and no longer warned as unread. Frontmatter that fails to parse no longer crashes budget on an oversized integer (ValueError), and the fallback that reads the paths key ignores YAML comments and blank lines between items. Quoted # is kept. Thread: PRRT_kwDOVBg7g86ySxKH Thread: PRRT_kwDOVBg7g86ySxKK Thread: PRRT_kwDOVBg7g86ySxKP Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Nothing to change in this review itself: it carries no finding of its own. Its five findings are the threads above, and each has its answer there. |
|
This summary restates the five threads above, and claims nothing of its own. Each finding has its answer in its thread. |
|
The report is for 82fd9bf and names neither line: one missing line and one partial branch in budget.py. Each branch this round adds runs in a regression case (listed in the thread replies). If the report on the new head still flags a line, I will cover it in the next round. |
| if target in by_target and rank[when] > rank[by_target[target]["when"]]: | ||
| by_target[target]["when"] = when |
There was a problem hiding this comment.
Must-fix: promoting an off guidance entry changes only when, retaining the full _chars() count even though it is loaded exclusively as a rule, whose frontmatter is stripped. For example, use CLAUDE.md = "claude\n", AGENTS.md = "---\npaths: []\n---\nagents\n", and .claude/rules/agents.md symlinked to AGENTS.md. Startup is charged for the frontmatter as well as the body. Reconcile the character count with the loading route when promoting an off entry, and extend the off-AGENTS alias test with frontmatter. Also consider lazy guidance promoted to start through a rule: its full guidance text should not all be charged at startup.
| counted.add(target) | ||
| if warning: | ||
| warnings.append(warning) | ||
| out.append({"path": f, "kind": "rule", "when": when, "chars": chars}) |
There was a problem hiding this comment.
Must-fix: by_target contains only guidance entries, so duplicate rule targets do not receive the loading-condition reconciliation. For example, create an unscoped !sub/.claude/rules/r.md and a root .claude/rules/r.md symlink to it. The nested path sorts first and produces a lazy entry; the root alias is then skipped because its target is counted but absent from by_target. The result reports zero startup characters despite the unconditional root rule. Add newly emitted rule entries to the target-to-entry map and test a nested alias that sorts before its root alias.
| values = _fallback_paths(frontmatter) | ||
| if values is None: | ||
| return False | ||
| return any(v.strip().strip("[]").strip(" \t\"'") for v in values) |
There was a problem hiding this comment.
Must-fix: the malformed-YAML fallback does not preserve the empty paths cases handled by the normal parser. For example, ---\npaths: null\nunused: [unclosed\n---\nbody\n is classified as lazy: the fallback returns "null", which this expression treats as a pattern. Without the unrelated malformed unused field, the same paths: null is correctly start. Similarly, paths: ["", ""] becomes scoped only when unrelated YAML is malformed. Normalize null and parse empty inline list items in the fallback so unrelated malformed metadata does not change the loading condition; add regression cases alongside the existing malformed-frontmatter tests.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 21e287c. Counting .claude/rules closes a real gap in start, and the loading rules match what the PR measured. One problem should be fixed before merge: paths values go through str(), so a short alias chain in a rule's frontmatter hangs repo budget. The rest are smaller edge cases, in the line comments.
Agreed with @exai-bot: a rule-to-rule link that sorts first keeps lazy; a guidance file promoted through a rule is charged its frontmatter; the YAML fallback reads paths: null and ["", ""] as patterns. On the open question about a rule that a CLAUDE.md imports, I replied in its thread: keep the code and fix the docs.
What I ran, at 21e287c:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network and Docker jobs only).- Scratch repros with the PR's
budget(): the alias chain (343 bytes 0.6 s, 393 bytes 4.3 s, 10x per level),paths: true/5/{a: 1}, a BOM rule,.claudeas a directory link, a rule-to-rule link, a danglingnotes.txtlink, and an imported rule with its own@path.
Checked, no issues
- Earlier threads: the
ValueErrorfallback, comments in the unparsedpaths, and a root rule linked to an off AGENTS.md or a nested CLAUDE.md are fixed, and each has a test. - Schemas:
repo_budget.v1only addsruletokindandharness_profile.v1only adds an optionalclaude_rules, so both widen in place; the Codex and Copilot profiles leave rules off. - Changelog: the fragment says that
startwas too low by every root rule before, so earlier budgets are not comparable with new ones.
| if isinstance(meta, dict): | ||
| value = meta.get("paths") | ||
| items = value if isinstance(value, list) else str(value or "").split(",") | ||
| return any(str(v).strip() for v in items if v is not None) |
There was a problem hiding this comment.
_has_paths calls str() on whatever YAML gives for paths, so a few hundred bytes of YAML aliases make repo budget hang or run out of memory.
yaml.safe_load keeps aliases as shared references, so it is fast, but str(v) on a list item (or on a dict value of paths) expands them. Repro with the PR code: a rule whose frontmatter is a0: &a0 [x,x,...] (10 items), then a1: &a1 [*a0,*a0,...] and so on, ending with paths: [*aN]. 343 bytes (7 levels) take 0.6 s, 393 bytes (8 levels) 4.3 s, and each level costs 10x more time and memory, so 10 levels is minutes and gigabytes. repo budget reads repositories you do not control.
The same str() also makes paths: true, paths: 5 and paths: {a: 1} scope the rule (all give lazy), while false and null give start; nothing measured says Claude Code reads such a value as a pattern.
Suggestion: only count strings, and never str() a container:
items = value if isinstance(value, list) else [value]
return any(isinstance(v, str) and v.strip() for v in items)(with the comma split for an inline string kept as now), plus a test with a nested-alias paths and one with paths: true.
| text = tree.read_text(path) | ||
| except (OSError, ValueError, RuntimeError) as err: # UnicodeDecodeError is a ValueError; RuntimeError a loop | ||
| return 0, False, f"{path}: not readable ({_unreadable_reason(err)}), counted as 0" | ||
| m = _FRONTMATTER.match(text) |
There was a problem hiding this comment.
A rule that starts with a UTF-8 BOM is read as having no frontmatter, so its paths is ignored.
_FRONTMATTER is anchored at \A---. .claude/rules/r.md = "\ufeff---\npaths: src/**\n---\nbody\n" gives start with 28 chars, instead of lazy with 5. dokimi.utils.parse_frontmatter, which _skills in this module uses for SKILL.md, strips the BOM first, so the same frontmatter in a skill is parsed. A rule saved by a Windows editor is enough to move it into start.
Suggestion: strip a leading \ufeff before matching (and count the body after it), as parse_frontmatter does; one test case with a BOM.
| try: | ||
| if tree.is_dir(tree.resolve(f, dirs=True)): # a tree lists a link to a directory as one entry | ||
| warnings.append(f"{f}: a link to a directory; the rules behind it are not counted") | ||
| except (OSError, ValueError, RuntimeError): # out of the tree (a shared rules dir), dangling or a loop |
There was a problem hiding this comment.
Nit: A dangling or out-of-tree link to a non-Markdown file under .claude/rules warns that "any rules behind it are not counted".
.claude/rules/notes.txt -> nowhere.txt gives that warning, and repo budget --fail-on-warning fails a CI gate on a file that can never be a rule (a regular notes.txt gives none). Suggestion: in the except branch, warn only when the name has no suffix (it may be a directory link) and skip other suffixes.
| hop limit, each of which also trips | ||
| `--fail-on-warning`. `start` is not an upper bound: it is too low by what it does not count | ||
| (`.claude/rules/*.md` files; only `@path.md` imports are followed, so `@notes.txt` adds nothing; | ||
| `--fail-on-warning`. For a profile with `guidance.claude_rules` (Claude Code), |
There was a problem hiding this comment.
Nit: The list of warnings that "each ... trips --fail-on-warning" just above does not name the new one, a link under .claude/rules to a directory or out of the tree. The module docstring now says six checks. Suggestion: add it to that list.
Summary
repo budgetnow counts Claude Code.claude/rules/**/*.md. Before this, the docs admitted thatstartwas "too low by.claude/rules/*.md" for every repo that uses rules.guidance.claude_rulesis a new optional key.harness_profile.v1is widened in place,claude-code.yamlsets it totrue, and the Codex and Copilot profiles do not count rules.kindenum inrepo_budget.v1gainsrule.startrule. It is not also counted as an import.paths: [],"",nulland an empty key.paths:lines are still read, which matches what was measured. Frontmatter nested too deep for the parser falls back the same way and no longer crashes.Docs updated: the evaluation contract, architecture and what-it-measures. A changelog fragment is included.
Evidence
Before:
repo budgeton a repo with.claude/rules/plain.mdlisted no entry for it, sostartwas missing about 8k tokens.After: that file is reported as
kind: rule,when: start. The classification matches what Claude Code 2.1.304 actually loads:.claude/rules/plain.md.claude/rules/deep/x.mdpaths: ["src/**"]sub/.claude/rules/n.mdSee @extra.mdpathsLazy side: tokens on the second call, after a Read. The control is a scoped rule plus a non-matching Read, which adds +223.
paths: src/**, Readsrc/a.pysub/.claude/rules, Readsub/a.pypaths: [unclosed(invalid glob)paths: "src/**"+ broken YAML elsewherepaths: []Tests:
tests/test_repo_budget_rules.py(new, 20 cases). Eachpathsshape, each link warning and the too-deep frontmatter case failed before its fix and passes now. All 120 budget tests pass.Merge Danger
Door: two-way
Both schemas are widened in place: a new optional profile key and a new
kindvalue.Blast Radius: budget-consumers
repo budgettotals for claude-code go up on repos that have rules, so any stored baseline compared against them will show growth.kindvalues will now seerule.repo reachandrepo scanstill do not look at rules (follow-up).🤖 Generated with Claude Code