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 1 issue(s) in this PR. By severity: 1 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).
There was a problem hiding this comment.
Waiting on a decision before I change this, still open. The reviewer's comment is one reading (keep following @ paths, and state it), not the author's decision.
Options: (a) keep following them, transitively and hop-limited, and state that in these docs, architecture.md and the changelog, as the comment proposes; (b) stop expansion at an imported rule.
My recommendation: (a), with the wording from the comment, after repo reach measures the imported-rule case on Claude Code. The comment also raises a new question for that measurement: whether an imported rule keeps its frontmatter. The count strips it on this route, as on the rule route, and that is not measured yet. The thread stays open until then.
| 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.20% 94.21%
========================================
Files 304 304
Lines 38748 38864 +116
Branches 6561 6588 +27
========================================
+ Hits 36504 36616 +112
- Misses 1430 1432 +2
- Partials 814 816 +2
🚀 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.
There was a problem hiding this comment.
Fixed in cc17f0d. A guidance file a rule links to takes the rule's count when the rule loads it earlier, since the rule route strips the frontmatter. In your example AGENTS.md is 7 characters at start (agents\n), not 25, and the warning is gone. Pinned by test_a_root_rule_linked_to_an_off_agents_md_is_charged_its_body. A lazy nested file promoted to start through a rule is charged its body: the rule loads the whole body at start, and nothing else of the file.
| 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.
There was a problem hiding this comment.
Fixed in cc17f0d. A rule entry now goes in the target map, so a root alias that sorts after a nested real file upgrades that entry to start instead of being skipped. Your case (!sub/.claude/rules/r.md with a root symlink to it) reports 10 characters at start, not 0. Pinned by test_a_root_rule_linked_to_a_nested_rule_that_sorts_first_still_loads_at_start.
| 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.
There was a problem hiding this comment.
Fixed in cc17f0d. The unparsed-YAML fallback reads null, ~ and empty items as no pattern, so paths: null and paths: ["", ""] stay start even when unrelated metadata is malformed. Pinned by unparsed-null-unrelated-malformed and unparsed-empty-items-unrelated-malformed in the parametrized test.
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.
There was a problem hiding this comment.
Fixed in cc17f0d, with your suggestion. A paths value is read as strings only and is never str()-ed, so an alias chain no longer expands: a 6-level chain took 0.66 s on head and parses at once now. Pinned by test_a_paths_value_is_never_stringified, which fails if a container is converted to text. Side effect: paths: true, paths: 5 and paths: {a: 1} now count as naming no pattern (start), as suggested. Nothing measured says Claude Code reads them as a pattern; a probe that shows otherwise would be a follow-up. Pinned in the parametrized test.
| 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.
There was a problem hiding this comment.
Fixed in cc17f0d. A leading UTF-8 BOM is stripped before the frontmatter match, as parse_frontmatter does for SKILL.md. Your example is now lazy with 5 characters. Pinned by test_a_rule_with_a_utf8_bom_reads_its_frontmatter.
| 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.
There was a problem hiding this comment.
Fixed in cc17f0d. A dangling or out-of-tree link with a suffix (notes.txt) is no longer warned, since no rule can be behind it. A name without a suffix is still warned, because it may be a directory link. Pinned by test_a_dangling_link_to_a_non_markdown_file_under_rules_is_not_warned. A dangling .md rule is still named and counted as 0, pinned by test_a_dangling_link_to_a_markdown_rule_is_named_and_counted_as_zero.
| 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.
There was a problem hiding this comment.
Fixed in cc17f0d. The list of checks that trip --fail-on-warning in architecture.md now names the link under .claude/rules to a directory or out of the tree.
… the harness does - A rule with a UTF-8 BOM has its frontmatter read, as a SKILL.md does. - `paths` is a pattern only when it is a string or a list of strings; the value is never str()-ed, so a YAML alias chain cannot expand into a hang. Non-string values (true, 5, a mapping) name no pattern. - The unparsed-YAML fallback treats null, ~ and empty items as no pattern, so unrelated malformed metadata does not change the loading condition. - A rule linked to a file already counted takes the earlier loading condition and that route's count (a rule's frontmatter is removed before it loads). - A rule entry joins the target map, so a root alias that sorts after a nested real file still promotes it to start. - A dangling link to a suffixed non-Markdown file under .claude/rules is not warned; a name without a suffix still is (it may be a directory link). - architecture.md lists the rules-link warning among the checks that trip --fail-on-warning. Thread: PRRT_kwDOVBg7g86yUV4N Thread: PRRT_kwDOVBg7g86yUV4R Thread: PRRT_kwDOVBg7g86yUV4U Thread: PRRT_kwDOVBg7g86yVf4V Thread: PRRT_kwDOVBg7g86yVf4Y Thread: PRRT_kwDOVBg7g86yVf4b Thread: PRRT_kwDOVBg7g86yVf4c Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| values = _fallback_paths(frontmatter) | ||
| if values is None: | ||
| return False | ||
| return any(_names_pattern(v) for v in values) |
There was a problem hiding this comment.
Must-fix: the fallback does not preserve the paths classification when unrelated metadata is malformed. For example, paths: true\n is unscoped, but paths: true\nunused: [unclosed\n becomes scoped because _names_pattern treats true as a string pattern; numbers and mappings have the same problem. Conversely, paths: [\n "src/**"]\nunused: [unclosed\n becomes unscoped because _fallback_paths collects only [ and stops before the pattern. These produce incorrect start/lazy totals. Parse the isolated paths value (including flow-list continuation lines) while retaining the documented non-string handling, and add paired tests with and without unrelated malformed YAML.
There was a problem hiding this comment.
Fixed in f2da52f. A paths key that the rest of the frontmatter does not parse is now read on its own, with its flow-list continuation lines, so paths: true stays unscoped and a paths: [ split over lines scopes the rule. Pinned by four new cases in test_only_a_paths_value_that_names_a_pattern_scopes_a_rule (new ones red before).
There was a problem hiding this comment.
🟡 Changes recommended
Directory symlink handling can silently omit rules or report fictitious entries, while rule imports are still expanded despite the no-follow contract.
3 open issues
What changed in this PR
3 new findings.
This PR extends repo budget to include Claude Code rules, reducing undercounting of repository context.
Changes:
- Counts rule bodies as startup or lazy context, with import deduplication and link warnings.
- Enables rules through an optional harness-profile setting and widens the budget schema.
- Adds regression tests and updates documentation and release notes.
| File | Description |
|---|---|
dokimi/tests/test_repo_budget_schema.py |
Validates artifacts containing rule entries. |
dokimi/tests/test_repo_budget_rules.py |
Tests rule classification, parsing, imports, and links. |
dokimi/dokimi/schemas/repo_budget.v1.json |
Adds the rule entry kind. |
dokimi/dokimi/schemas/harness_profile.v1.json |
Allows the optional claude_rules setting. |
dokimi/dokimi/repos/profiles/claude-code.yaml |
Enables rule counting for Claude Code. |
dokimi/dokimi/repos/harness_profile.py |
Exposes the profile’s rule-counting setting. |
dokimi/dokimi/repos/budget.py |
Implements rule discovery, counting, and warnings. |
dokimi/dokimi/commands/repo.py |
Passes the profile setting into budget calculation. |
dokimi/docs/what-it-measures.md |
Includes rules in the budget description. |
dokimi/docs/repo-evaluation-contract.md |
Documents rule-loading conditions and limitations. |
dokimi/docs/architecture.md |
Describes rule accounting and link warnings. |
dokimi/changelog.d/20261030_180000_napetrov_budget-claude-rules.md |
Announces rule support and schema changes. |
Review effort: Balanced
| 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, entries) |
There was a problem hiding this comment.
Fixed in f2da52f. An @ path in a rule is not followed, also when a CLAUDE.md imports the rule (matches docs/repo-evaluation-contract.md). A file named CLAUDE.md under .claude/rules is a rule, not guidance. Pinned by test_a_rule_does_not_expand_its_at_imports_even_when_claude_md_imports_it and test_a_file_named_claude_md_under_rules_is_a_rule_not_guidance.
| if not PurePosixPath(f).suffix: # a file with a suffix (notes.txt) is never a rule; a name without one may be a directory | ||
| warnings.append(f"{f}: a link that does not resolve in the tree; any rules behind it are not counted") |
There was a problem hiding this comment.
Fixed in f2da52f. A link under .claude/rules that does not resolve is now named whatever its suffix: the tree cannot tell a dangling file from an external directory. The old test that stayed silent on notes.txt is replaced by test_a_dangling_link_under_rules_that_is_not_markdown_is_named.
| for f in sorted(f for f in files if _RULES_ENTRY.match(f) and not RULE_GLOB.match(f)): | ||
| 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 | ||
| if not PurePosixPath(f).suffix: # a file with a suffix (notes.txt) is never a rule; a name without one may be a directory | ||
| warnings.append(f"{f}: a link that does not resolve in the tree; any rules behind it are not counted") | ||
| for f in sorted(f for f in files if RULE_GLOB.match(f)): |
There was a problem hiding this comment.
Fixed in f2da52f. An entry that resolves to a directory is named as a linked directory before it is classified as a rule, so shared.md no longer yields a zero-character rule. Pinned by test_a_linked_directory_named_like_a_rule_is_named_not_counted_as_a_rule.
…e import walk, names unresolved rule links - A `paths` key that the rest of the frontmatter fails to parse is read as its own block, with its flow-list continuation lines, so `paths: true` and `paths: [` over several lines classify the same as the parsed key (Thread: PRRT_kwDOVBg7g86yWM2k). - An @ path in a rule is not followed, also when a CLAUDE.md imports the rule, and a file named CLAUDE.md under .claude/rules is a rule, not guidance (Thread: PRRT_kwDOVBg7g86yWNjN). - A link under .claude/rules that does not resolve is named whatever its suffix, since the tree cannot tell a dangling file from an external directory (Thread: PRRT_kwDOVBg7g86yWNjn). - An entry that resolves to a directory is named as a linked directory before it is classified as a rule, also for a name ending in .md (Thread: PRRT_kwDOVBg7g86yWNj0). Thread: PRRT_kwDOVBg7g86yWM2k Thread: PRRT_kwDOVBg7g86yWNjN Thread: PRRT_kwDOVBg7g86yWNjn Thread: PRRT_kwDOVBg7g86yWNj0 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| names = set(guidance_names) | ||
| every = [f for f in files if f.rsplit("/", 1)[-1] in names] | ||
| # a file under .claude/rules is a rule whatever its name (a rules/CLAUDE.md is not guidance) | ||
| every = [f for f in files if f.rsplit("/", 1)[-1] in names and not RULE_GLOB.match(f)] |
There was a problem hiding this comment.
Must-fix: Apply this exclusion only when rules is enabled. With the Codex profile (reads_claude_rules == False), a tree containing .claude/rules/AGENTS.md now loses that recognized guidance file entirely: it is excluded here and _rules receives no files. The unconditional RULE_GLOB check in _imports likewise truncates explicit import chains for profiles that do not read Claude rules. Gate both special cases on the profile flag and add disabled-rules regression tests; the existing profile test only verifies that no rule entries appear.
There was a problem hiding this comment.
Fixed in 6cc0717: the skip of .claude/rules files (as rules, and in _imports) now applies only when the profile reads rules.
For codex, .claude/rules/AGENTS.md counts as guidance and its @imports are followed.
Test: test_a_profile_that_does_not_read_rules_still_counts_a_rules_file_as_guidance_and_imports (red on f2da52f).
| if not item.strip() or item[:1] in (" ", "\t", "-", "#"): | ||
| block.append(item) | ||
| else: | ||
| break |
There was a problem hiding this comment.
Must-fix: The fallback drops unindented flow-list continuation lines. For frontmatter paths: [\n"src/**"]\nunused: [unclosed\n, parsing the full document fails on unused, but the paths value itself names a pattern. This loop stops before "src/**"], leaving only paths: [; _names_pattern then returns false and the rule is incorrectly charged at start. Track open flow collections when collecting the block, and add a malformed-unrelated-field test with an unindented continuation (the current tests only cover indented continuations).
… keeps its continuation A profile that does not read .claude/rules (codex) still counts a file there as guidance and follows its @imports. An unindented continuation line of a flow list that the paths key opens is part of the key's block. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| for i, line in enumerate(lines): | ||
| if _PATHS_LINE.match(line): | ||
| block = [line] | ||
| depth = line.count("[") - line.count("]") # a flow list open on the key runs on, unindented or not |
There was a problem hiding this comment.
Must-fix: Brackets in YAML comments are counted as flow-list delimiters. For frontmatter paths: [] # [optional]\nunused: [unclosed\n (with an unmatched [ in the comment, e.g. change the comment to # [optional), full-document parsing fails on unused; this depth calculation then consumes that unrelated key into the paths block. Parsing the block fails too, and the text fallback treats unused: [unclosed as a pattern, reporting the rule as lazy despite paths: []. Ignore comments when tracking flow-list depth, including continuation lines, and add a regression test for an empty paths list with a bracket-bearing comment and unrelated malformed YAML.
| if rules and RULE_GLOB.match(f): | ||
| continue # an @ path in a rule is not followed (docs/repo-evaluation-contract.md) |
There was a problem hiding this comment.
Must-fix: This check only recognizes the import's spelling, not its resolved target. With CLAUDE.md containing @docs/memory/alias.md, docs/memory/alias.md symlinked to .claude/rules/r.md, and that rule containing @../../extra.md, _imports reads the alias and incorrectly adds an existing extra.md as a start import. _rules already deduplicates the alias against the rule by resolved target, so the output treats this as a rule while still expanding its imports, contrary to the new contract. Check both the literal path and _target(tree, f) for rule membership, and add an outside-rules alias regression test.
Suggestion:
| if rules and RULE_GLOB.match(f): | |
| continue # an @ path in a rule is not followed (docs/repo-evaluation-contract.md) | |
| if rules and (RULE_GLOB.match(f) or RULE_GLOB.match(_target(tree, f))): | |
| continue # an @ path in a rule is not followed (docs/repo-evaluation-contract.md) |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6cc0717. The new rules accounting holds up against the cases I tried. Two risks remain, in the malformed-YAML fallback and in import traversal through a link; both reproduce at this head. One nit on the docs.
What I ran, on a Linux review host at 6cc0717:
make ci-local: 28 passed, 2 failed (gitleaks download returned HTTP 504, so its scan did not run), 7 skipped; 12,019 tests; changed-line coverage 94%.- Reproductions for the findings below, on trees built the way
tests/test_repo_budget_rules.pybuilds them. - Mutation check on the fallback: dropping
"null"and"~"from_names_patternstill passes all 49 tests intest_repo_budget_rules.py. Those items only reach_names_patternwhen thepathskey itself does not parse, so the cases that are named as pinning them go through the YAML parse of the key instead.
Checked, no issues
- Schemas:
ruleadded to thekindenum and optionalclaude_rulesadded to the profile schema both widen in place; the schema compatibility check passes against the base. - Profiles: Codex and Copilot leave
claude_rulesoff, and.claude/rules/CLAUDE.mdis then plain guidance. - Dedup and links: a root alias of a nested rule, a rule linked to an off AGENTS.md, directory links and dangling links each give one entry and the expected warning.
- Commits: all eight non-merge commits carry a
Signed-off-by.
Bot points I checked that no longer apply at this head (13)
- Copilot's and exai-bot's point on an imported rule's
@paths being followed does not apply: at this head_importsstops at a rule, and a test pins thatextra.mdis not charged. - exai-bot's point on a root rule linked to an off AGENTS.md losing startup context does not apply: the guidance entry takes the earlier loading condition, and tests pin it.
- exai-bot's point on an oversized integer crashing the frontmatter parser does not apply:
ValueErroris caught in_has_paths. - exai-bot's point on comments in the unparsed
pathsfallback does not apply:_without_commentcuts them and comment lines between items are skipped. - exai-bot's point on a promoted guidance entry keeping its frontmatter does not apply: the entry takes the rule's count when the rule loads it earlier.
- exai-bot's point on duplicate rule targets not being reconciled does not apply: rule entries are added to the target map and the nested-first case loads at start.
- exai-bot's point on
paths: nulland empty items in the fallback does not apply:_names_patternreadsnull,~and empty items as no pattern. - exai-bot's point on
paths: trueand a split flow list in the fallback does not apply: the key is parsed on its own with its continuation lines. - Copilot's point on a CLAUDE.md-imported rule expanding its imports and on a
rules/CLAUDE.mdfile does not apply: both are handled at this head and tested. - Copilot's point on an unresolved external link with a suffix suppressing the warning does not apply: any non-Markdown entry that does not resolve is named.
- Copilot's point on a directory link named like a rule does not apply: an entry that resolves to a directory is named before it is classified.
- exai-bot's point on gating the rules exclusion on the profile flag does not apply: both the exclusion and the
_importsskip are gated onrules, and a test covers the Codex profile. - exai-bot's point on unindented flow-list continuation lines does not apply:
_paths_blockkeeps them while a flow list is open.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to b1f2624, which fix the nit from my review:
b1f2624:dokimi/docs/repo-evaluation-contract.md:51: added that a rule a root CLAUDE.md or AGENTS.md imports isstartwhatever itspaths, with the same sentence inarchitecture.mdand the changelog fragment.
make ci-local at b1f2624: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the two risks from my review.
…s or imports read A bracket inside a YAML comment on the paths line (or on a comment line under it) counted toward the flow-list depth, so an unrelated malformed key below was pulled into the paths block and an empty paths was read as scoped. Comments are now left out of the depth. An @ import of an alias that links to a rule was still followed, though an @ path inside a rule is not. The rule check now also looks at the file the import resolves to, as the rule dedup already does. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if s and not s.startswith("#"): | ||
| values.append(_without_comment(s[1:] if s.startswith("-") else s)) |
There was a problem hiding this comment.
Must-fix: For a root rule containing ---\npaths: []\n unused: [unclosed\n---\nbody\n, _paths_block() includes the indented unused line. Both YAML parses fail, and this fallback treats unused: [unclosed as a paths pattern, reporting the rule as lazy even though paths: [] names no pattern. This drops the body from the startup total. Keep unrelated indented fields out of the fallback pattern values (including after a complete inline value), and add this case to the parametrized frontmatter tests with when="start" and body-only character counting.
There was a problem hiding this comment.
Fixed in adbfd73. An indented line that is a mapping key ( unused: [unclosed) now ends the paths block when no flow list is open, so it is no longer read as a pattern. Your example (paths: [] plus that line) now loads at start with body-only counting.
Tests: four new cases in the frontmatter table. The paths: [] and paths: null ones are red on cd427e9; the two after a pattern and after a list pass on both (guards).
Local: test_repo_budget_rules.py + test_repo_budget.py 137 passed; full suite rc 0; ruff clean. Self-review: 0 blocking.
When the frontmatter does not parse, the paths key was read with its indented continuation lines, so ` unused: [unclosed` under `paths: []` counted as a pattern and the rule was reported as lazy. A line that is an indented mapping key now ends the block, outside an open flow list. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| # A guidance file or a rule is an entry of its own, so a cut import of it is already loaded (not an import). | ||
| loaded = {_target(tree, p) for p in at_start | lazy_all} | { | ||
| _target(tree, e["path"]) for e in entries if e["kind"] == "guidance" and e["when"] != "off"} | ||
| _target(tree, e["path"]) for e in entries if e["kind"] in ("guidance", "rule") and e["when"] != "off"} |
There was a problem hiding this comment.
Must-fix: This treats every scoped rule as loaded when suppressing hop-limit warnings, even though its loading depends on a matching file read. For example, use CLAUDE.md -> c1.md -> c2.md -> c3.md -> c4.md -> .claude/rules/r.md, import_max_depth=4, and r.md containing ---\npaths: src/**\n---\nbody\n. The startup import is skipped and the rule remains lazy, but warnings is empty, so --fail-on-warning succeeds without naming the skipped context. Exclude conditionally loaded rules from this suppression unless another import chain actually reaches them, and add a scoped counterpart to test_a_cut_import_of_a_counted_rule_does_not_warn.
There was a problem hiding this comment.
Fixed in 6d372ac. A scoped rule (lazy) no longer counts as loaded when a cut import is checked, so your chain (c4.md -> .claude/rules/r.md, import_max_depth=4) now warns and names c4.md. An unscoped rule still suppresses it, and a scoped rule that another chain does reach is still counted.
Test: test_a_cut_import_of_a_scoped_rule_warns, red on adbfd73. Local: budget tests 138 passed, full suite rc 0.
A cut import of a rule that loads only when a matching file is read was suppressed as already loaded, so the hop-limit warning was lost. Only a rule that loads at start now counts as loaded for that check. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| _FRONTMATTER = re.compile(r"\A---[ \t]*\r?\n(.*?)^---[ \t]*(?:\r?\n|\Z)", re.S | re.M) | ||
| # The `paths:` key of frontmatter that does not parse: its line, read on its own (see _paths_block). | ||
| _PATHS_LINE = re.compile(r"^paths[ \t]*:(.*)$") | ||
| _INDENTED_KEY = re.compile(r"^[ \t]+[A-Za-z0-9_][^:#]*:(?:[ \t]|$)") # ` unused: ...`: a key, not a paths continuation |
There was a problem hiding this comment.
Must-fix: The indented-key guard misses quoted YAML keys. For a root rule containing ---\npaths: []\n "unused": [unclosed\n---\nbody\n, both YAML parses fail, _INDENTED_KEY does not match the unrelated key, and the fallback treats "unused": [unclosed as a paths pattern. The rule is consequently reported as lazy rather than start, understating the startup budget. The same happens with 'unused'. Extend the block-boundary detection to quoted mapping keys and add both variants to the existing empty-paths regression tests.
There was a problem hiding this comment.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
One risk and one nit in the new commits. The fixes for the bracket-in-comment and alias points are right, but the alias check now also fires on a root guidance file that is a link to a rule.
What I ran, on a Linux review host at 6d372ac:
make ci-local: 30 passed, 0 failed, 7 skipped (docker, network and org-level jobs only); 12,027 tests; changed-line coverage 97%.- Reproductions for both findings, with no model or network involved; the fixes below pass
test_repo_budget_rules.pyandtest_repo_budget.py(138 passed). - Mutants of the commits since b1f2624 (21): 14 killed. The survivors are variants of the
_INDENTED_KEYcharacter class (digit- or underscore-led keys, tab indentation, a barekey:line), which only a hand-built frontmatter reaches, so I list them as nothing to fix.
Checked, no issues
- Delta:
_flow_depthignores brackets in comments on the key line and on continuation lines; theloadedset no longer lets a scoped rule cover a cut import. - Schemas and docs:
ruleadded to thekindenum and optionalclaude_rulesadded to the profile widen in place;schema_compatagainst the base passes. - Commits: all 12 non-merge commits carry a
Signed-off-by.
Bot points I checked that no longer apply at this head (8)
- exai-bot's and Copilot's points on an imported rule's
@paths do not apply:_importsstops at a rule, and the contract, the architecture page and the changelog say so and name the start case. - exai-bot's points on a rule linked to a guidance file or to a nested rule do not apply: the entry takes the earlier loading condition and the rule's body count, and tests pin both.
- exai-bot's point on an oversized integer in frontmatter does not apply:
ValueErroris caught and the rule falls back to thepathslines. - exai-bot's points on the unparsed-YAML
pathsfallback (comments,null, empty items,true, a flow list over several lines, brackets in comments, an indented field of another key) do not apply: each case reads as the parsed YAML would, and the commits pin them. - Copilot's points on dangling links and a directory link named like a rule do not apply: an entry that does not resolve, or resolves to a directory, is named before it is classified.
- exai-bot's point on gating the
.claude/rulesexclusion on the profile flag does not apply: both the exclusion and the_importsskip depend onrules, and a Codex-profile test covers it. - exai-bot's point on an alias of a rule being import-expanded does not apply in the imported-alias case:
_importsnow matches the resolved target as well (see the inline comment for what that change broke). - exai-bot's point on a scoped rule suppressing a cut-import warning does not apply: only a
startrule counts as loaded, and a test pins the scoped case.
| while level: | ||
| nxt: list[str] = [] | ||
| for f in level: | ||
| if rules and (RULE_GLOB.match(f) or RULE_GLOB.match(_target(tree, f))): |
There was a problem hiding this comment.
A root CLAUDE.md that is a link to a rule loses its @ imports. The new RULE_GLOB.match(_target(tree, f)) also runs on the roots, which _imports starts from, so a guidance root whose target sits under .claude/rules/ is skipped.
CLAUDE.mdsymlinked to.claude/rules/shared.md(contentshared @extra.md),extra.md500 chars:budget(..., rules=True)gives onlyCLAUDE.md guidance start 17, no warning. Withrules=Falsethe same tree givesCLAUDE.mdplusextra.md import start 500.startis understated by 500 on the Claude Code profile.- Claude Code reads
CLAUDE.mdas memory and expands its imports, so the root must keep expanding. The alias case from the earlier thread (CLAUDE.md->@docs/alias.md-> rule) must still be skipped.
Suggestion: apply the target check only to files reached through an import:if rules and (RULE_GLOB.match(f) or (f not in roots and RULE_GLOB.match(_target(tree, f)))):. With it the root keepsextra.mdand the alias stays skipped; add a test of the root link next totest_an_alias_of_a_rule_is_not_followed_as_an_import.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| loaded = {_target(tree, p) for p in at_start | lazy_all} | { | ||
| _target(tree, e["path"]) for e in entries if e["kind"] == "guidance" and e["when"] != "off"} | ||
| _target(tree, e["path"]) for e in entries | ||
| if (e["kind"] == "guidance" and e["when"] != "off") or (e["kind"] == "rule" and e["when"] == "start")} |
There was a problem hiding this comment.
Must-fix: Preserve loading provenance when a rule promotes an aliased guidance entry. Reproducer: CLAUDE.md imports c1.md, the chain continues through c4.md, and c4.md imports AGENTS.md; set import_max_depth=4, give AGENTS.md frontmatter paths: src/**, and symlink .claude/rules/agents.md to it. _rules() promotes the otherwise-off AGENTS entry to lazy but leaves its kind as guidance. This predicate then includes its target in loaded, suppressing the fifth-hop warning even though only reading a matching file loads it. Consequently --fail-on-warning succeeds incorrectly. Build the independently-loaded guidance target set before rule promotion, or retain the loading route separately, and add a regression test combining a scoped alias with a cut import.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to b484dc8, which fix the nit from my review:
b484dc8:dokimi/dokimi/repos/budget.py:52:_INDENTED_KEYnow accepts double- and single-quoted keys, and the frontmatter table has both quoted cases afterpaths: [].
make ci-local at b484dc8: 30 passed, 0 failed, 7 skipped.
The static job's schema_compat step fails: it reports eval_coverage.v1.json and need_graph.v1.json as removed. Both came into main with #340 after this branch was cut, so the branch only lacks them; the fix commit touches budget.py's _INDENTED_KEY and its tests. Merging main into the branch clears it.
Not approving yet. Still open: the risk from my review.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if not RULE_GLOB.match(f): | ||
| warnings.append(f"{f}: a link that does not resolve in the tree; any rules behind it are not counted") |
There was a problem hiding this comment.
Must-fix regression: the previously reported notes.txt case is still present. With .claude/rules/notes.txt -> nowhere.txt, this branch emits a rules-directory warning even though the link is named as a non-Markdown file. Consequently, repo budget --fail-on-warning exits 1 on this otherwise irrelevant entry. The suffix guard and test_a_dangling_link_to_a_non_markdown_file_under_rules_is_not_warned mentioned in the earlier fix are absent. Restore the agreed filtering and regression test, or explicitly resolve the earlier thread if warning on all unresolved links is now intentional.
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