feat: add XP-009 for Codex's cumulative instruction-chain cap (#1289) - #1292
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Claude finished @avifenesh's task in 8m 4s —— View job Review: XP-009 cumulative instruction-chain cap
The rule models a real gap and the discovery order it implements is right on the dimensions the PR describes. Two behavioural problems and a doc-generation defect need fixing before merge. Everything below was verified by building and running probe tests against this branch ( Blocking1. The chain sums across sibling subtrees - false positives on the recommended layout.
It scales the wrong way: a monorepo with N packages fires unconditionally once N × package-size + root passes 32 KiB, and the blamed file can sit in a subtree that shares nothing with the bytes that pushed it over. It also contradicts the advice AGM-006 gives in this same file ("Codex ... recommends splitting across nested directories to stay under the byte cap"). Fix: sum each root-to-leaf directory chain independently, take crossings per chain, dedupe per file. 2. The function's only two non-test callers are That is the override doing exactly what Codex documents. XP-009 correctly excludes the shadowed file; XP-004/005/006 need the same shadowing, or the pair excluded. Should fix3. 4. Double-escaped newlines in 5. Test gaps. 6. Read path diverges from the rest of the module. 7. Stale rule lists. The module header was updated to include XP-009, but the same enumeration appears unchanged in four other places:
And 8. CHANGELOG. Minor
What I verified, and what I did notVerified by execution on this branch: the sibling false positive, the Not verified: the full suite and the eval numbers in the PR body - I ran only targeted The Cline v4.1.2 half of the PR looks right - the three releases are described accurately against this tool's declared-irrelevant list, and the baseline plus review date move together. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Group the candidates by directory, keeping only Codex-discoverable names. | ||
| let mut by_dir: BTreeMap<PathBuf, Vec<&PathBuf>> = BTreeMap::new(); | ||
| for path in instruction_file_paths { | ||
| let Some(name) = path.file_name().and_then(|n| n.to_str()) else { | ||
| continue; | ||
| }; | ||
| if !precedence.contains(&name) { | ||
| continue; | ||
| } | ||
| let Some(dir) = path.parent() else { continue }; | ||
| // Only directories from the root down: the walk stops at the working | ||
| // directory, and anything outside the root is not part of this chain. | ||
| if !dir.starts_with(root_dir) { | ||
| continue; | ||
| } | ||
| by_dir.entry(dir.to_path_buf()).or_default().push(path); | ||
| } | ||
|
|
||
| // BTreeMap orders shallow-to-deep for nested paths, matching root-down | ||
| // concatenation. Within a directory, take the highest-precedence name only. | ||
| by_dir | ||
| .into_values() | ||
| .filter_map(|candidates| { | ||
| precedence.iter().find_map(|wanted| { | ||
| candidates | ||
| .iter() | ||
| .find(|p| p.file_name().and_then(|n| n.to_str()) == Some(*wanted)) | ||
| .map(|p| (*p).clone()) | ||
| }) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
Correctness (blocking): the chain sums across sibling subtrees, so this over-reports on exactly the layout Codex recommends.
by_dir collects every directory under the root that holds an AGENTS.md, and the loop at line 331 sums all of them into one running total. But a Codex chain is a single root→cwd path, not the whole tree - two sibling packages are never concatenated together.
Verified with a probe test on this branch (root 10 KB + pkg-a/ 12 KB + pkg-b/ 12 KB):
XP-009 pkg-b/AGENTS.md
"Codex instruction chain reaches 34816 bytes across 3 file(s), past the 32768-byte ... default"
Neither real chain exceeds the cap: root→pkg-a is 22 KB and root→pkg-b is 22 KB. This is a false positive, and it gets worse the more packages a monorepo has - a 20-package repo with a 2 KB AGENTS.md each plus a root file fires unconditionally. It also contradicts the advice AGM-006 gives a few hundred lines up in this same file ("Codex ... recommends splitting across nested directories to stay under the byte cap").
The fix is to score chains, not the set: for each directory that holds a chain file, sum only that directory's ancestors (root-down), and report the first crossing over the worst chain. Something like:
// chain files, root-first, one per directory (as today)
let files = codex_instruction_chain(instruction_file_paths, root_dir);
// each leaf directory defines one chain: itself plus its ancestors
for leaf in &files {
let leaf_dir = leaf.parent().unwrap_or(root_dir);
let mut total = 0usize;
for f in files.iter().filter(|f| leaf_dir.starts_with(f.parent().unwrap_or(root_dir))) {
total += size_of(f);
if total > CODEX_BYTE_LIMIT { /* record crossing on f */ break }
}
}then dedupe the recorded crossings so one file is reported once.
Related: the comment on 828 says "BTreeMap orders shallow-to-deep for nested paths". That holds for a single chain, but across siblings the order is lexicographic DFS (a/b/c before z), so today the reported file can be one in an unrelated subtree - as in the probe above, where pkg-b is blamed for pkg-a's bytes.
| // `AGENTS.override.md` first, then `AGENTS.md`. Codex also honors any | ||
| // `project_doc_fallback_filenames`, but those live in the user's Codex | ||
| // `config.toml` rather than anything agnix reads, so the chain is built from | ||
| // the two default names. A project relying on fallbacks would have a longer | ||
| // real chain than this, which makes the check conservative: it can | ||
| // under-report, never over-report. | ||
| let precedence: [&str; 2] = ["AGENTS.override.md", "AGENTS.md"]; |
There was a problem hiding this comment.
Evidence: "it can under-report, never over-report" is not true as written, and the same claim is repeated in three other places that all need correcting together once the sibling bug above is fixed:
- this comment (line 806-807)
knowledge-base/VALIDATION-RULES.mdXP-009 Detection: "this check under-reports rather than over-reports"- the PR body ("makes the check under-report rather than over-report - the right direction for a SHOULD-level warning")
CHANGELOG.mdimplies the same by describing only the truncation direction
The fallback-filenames argument is sound in isolation, but the sibling-summing behaviour over-reports independently of it, so the net direction is currently both. Worth stating the residual limitation precisely rather than as an absolute.
| t!( | ||
| "rules.xp_009.message", | ||
| bytes = running_total, | ||
| limit = CODEX_BYTE_LIMIT, | ||
| count = chain.len() | ||
| ), |
There was a problem hiding this comment.
Correctness: bytes and count describe different sets, so the message can be arithmetically impossible.
running_total is the sum up to and including the crossing file; chain.len() is the length of the whole chain. When the crossing happens before the end, the message attributes a partial byte total to every file.
Verified on this branch with root 20 KB + api/ 20 KB + api/v2/ 5 KB:
file = api/AGENTS.md
"Codex instruction chain reaches 40960 bytes across 3 file(s) ..."
40960 is 2 files, not 3 - and the third file's 5 KB is precisely the content Codex drops. Either count the files actually summed, or report the full chain total alongside the crossing point:
truncated_at = Some((path.clone(), total, idx + 1)); // files counted so farA test asserting the reported file and the numbers in the message would have caught this - see the top-level comment on test coverage.
| for path in &chain { | ||
| let size = match config.fs().read_to_string(path) { | ||
| Ok(content) => content.len(), | ||
| Err(_) => continue, | ||
| }; | ||
| total += size; | ||
| if total > CODEX_BYTE_LIMIT && truncated_at.is_none() { | ||
| truncated_at = Some((path.clone(), total)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Read path is inconsistent with the rest of this module. This is the only config.fs().read_to_string(...) call in project_level.rs (verified by grep); XP-004/005/006 above and VER-001 below all go through file_utils::safe_read_file, which rejects non-regular files (a FIFO named AGENTS.md would hang this loop) and enforces DEFAULT_MAX_FILE_SIZE before allocating. Since XP-009 is about sizes, an unbounded read of an oversized file is the wrong place to skip the guard.
It also re-reads files XP-004 already loaded into file_contents a few lines up - the doc comment's "adds no filesystem traversal" is true for the walk, but every chain file is now read twice on a default run. Reusing file_contents (or fs::metadata().len(), which is what the byte cap actually measures and avoids reading content at all) would fix both.
Note metadata().len() is also more faithful than content.len(): read_to_string fails on non-UTF-8 and the Err(_) => continue arm then silently drops that file's bytes from the total.
| if file_name.eq_ignore_ascii_case("claude.md") | ||
| || file_name.eq_ignore_ascii_case("agents.md") | ||
| // Codex checks `AGENTS.override.md` before `AGENTS.md` in each | ||
| // directory, so it is part of the instruction set and counts toward | ||
| // `project_doc_max_bytes` (XP-007, XP-009). | ||
| || file_name.eq_ignore_ascii_case("agents.override.md") |
There was a problem hiding this comment.
This one-line allowlist change has three knock-on effects, none of them tested.
-
New false positives in XP-004/005/006.
is_instruction_file()has exactly two non-test callers (pipeline.rs:720andpipeline.rs:877), both feedinginstruction_file_paths- which drives XP-004/005/006 as well as the new XP-009. AddingAGENTS.override.mdmeans an override file and theAGENTS.mdit shadows are now compared as two conflicting layers. Verified on this branch with a rootAGENTS.mdsaying "Use npm run build" and anAGENTS.override.mdsaying "Use pnpm build":XP-004 AGENTS.override.md uses pnpm but AGENTS.md uses npm for build commandsThat is the file doing its job. XP-009 correctly excludes the shadowed file from the chain (line 830 of
project_level.rs); XP-004/005/006 should apply the same one-file-per-directory shadowing, or the override pair should be excluded from conflict detection. -
No unit test for the new entry.
test_is_instruction_fileis right here at line ~2044 and assertsCLAUDE.md,AGENTS.md,.clinerules, etc.AGENTS.override.mdwas not added to it, so the allowlist entry is only exercised indirectly through one XP-009 integration test. -
The same list is still missing its siblings. It carries
gemini.local.mdbut neitherclaude.local.mdnoragents.local.md, even thoughCLAUDE.local.mdis a first-class layer elsewhere in this file (XP-008 matches"CLAUDE.md" | "CLAUDE.local.md") anddetection.rs:129treatsAGENTS.md | AGENTS.local.md | AGENTS.override.mdas one set. Pre-existing, but it is the same list and the same class of gap - worth closing in one pass or documenting why.localvariants are deliberately out of the cross-layer analysis.
Separately: the CHANGELOG's claim that this made XP-007's override handling "unreachable in practice" does not hold - see the top-level comment.
| }, | ||
| "good_example": "# Root AGENTS.md (8 KB)\\n\\nProject conventions.\\n\\n# nested/AGENTS.md (6 KB)\\n\\nModule specifics. Combined 14 KB, under the 32768-byte cap.", |
There was a problem hiding this comment.
Bookkeeping: the newlines in these examples are double-escaped, so the generated docs render literal \n.
Raw bytes here are \\n (JSON-escaped backslash + n), not \n. Every other rule in the file uses \n - compare XP-007 two entries up at line 11578. The result is visible in the file this PR generated, website/docs/rules/generated/xp-009.md:
# Root AGENTS.md (20 KB)\n\n# api/AGENTS.md (10 KB)\n\n# api/v2/AGENTS.md (8 KB)\n\nCombined 38 KB: ...versus xp-007.md, which renders real line breaks. Applies to both good_example and bad_example, and to both copies of the file - crates/agnix-rules/rules.json is byte-identical (verified with git diff --no-index), so the mirror carries the same defect. Re-running node scripts/sync-rule-bookkeeping.js after fixing the escapes will refresh the website doc.
Two smaller things in this same entry:
- It has no
"description"field, while XP-007 and the other rules with prose docs do. - It is inserted between XP-007 and XP-008, which is why
website/docs/rules/index.mdnow lists XP-009 above XP-008. Moving it after XP-008 keeps both the JSON and the generated index in order.
| ### Added | ||
| - **XP-009: Codex Instruction Chain Exceeds project_doc_max_bytes**. Codex's 32 KiB `project_doc_max_bytes` cap is cumulative across the instruction chain, not per-file - it "stops adding files once the combined size reaches the limit". XP-007 checks each `AGENTS.md` in isolation, so a project split across several mid-size files was truncated with no diagnostic at all. The new project-level rule builds the chain the way Codex discovers it (root down, at most one file per directory, `AGENTS.override.md` before `AGENTS.md`), sums root-first, and reports on the file where the running total crosses the limit - that file and everything deeper is what Codex drops. Rule count 442 -> 443 (closes #1289). | ||
|
|
||
| ### Fixed | ||
| - **`AGENTS.override.md` was not recognized as an instruction file**. Codex checks it before `AGENTS.md` in each directory, but `is_instruction_file()` did not list it, so it was invisible to every cross-platform rule. This also made the `AGENTS.override.md` handling added to XP-007 in the previous release unreachable in practice. | ||
|
|
There was a problem hiding this comment.
Two issues in this block.
-
The "unreachable" claim is wrong - "This also made the
AGENTS.override.mdhandling added to XP-007 in the previous release unreachable in practice." XP-007 is a per-file validator dispatched fromdetect_file_type(), andcrates/agnix-core/src/file_types/detection.rs:352has mappedAGENTS.override.md -> FileType::ClaudeMdsince before this PR (that file is untouched by this diff).is_instruction_file()has only two non-test callers, both inpipeline.rs, and both feed project-level path collection only - it never gated XP-007. Verified on this branch: a 40 KBAGENTS.override.mdproducesXP-007(plus AGM-002/003/004, CDX-003, XP-002), which is the same path that existed onmain. Theis_instruction_file()addition is still a genuine fix for the XP-004/005/006/XP-009 side; it just was not what was blocking XP-007. -
Duplicate
### Addedheading -[Unreleased]now has two### Addedsections (this one and the pre-existingargs-on-command-hooks entry immediately below). Merge into the single existing section per keep-a-changelog.
| /// XP-009: Codex's `project_doc_max_bytes` cap is cumulative across the | ||
| /// instruction chain, not per-file. XP-007 checks each file alone, so a project | ||
| /// split into several mid-size AGENTS.md files was silently truncated with no | ||
| /// diagnostic at all (issue #1289). | ||
| #[test] | ||
| fn xp009_flags_cumulative_chain_over_the_cap() { | ||
| let temp = tempfile::TempDir::new().unwrap(); | ||
| // Three files, each well under the 32 KiB per-file limit, 14 KB each. | ||
| // Combined 42 KB, so Codex stops appending partway through. | ||
| let body = "x".repeat(14 * 1024); | ||
| fs::write(temp.path().join("AGENTS.md"), &body).unwrap(); | ||
| fs::create_dir_all(temp.path().join("api")).unwrap(); | ||
| fs::write(temp.path().join("api").join("AGENTS.md"), &body).unwrap(); | ||
| fs::create_dir_all(temp.path().join("api").join("v2")).unwrap(); | ||
| fs::write(temp.path().join("api").join("v2").join("AGENTS.md"), &body).unwrap(); | ||
|
|
||
| let result = validate_project(temp.path(), &LintConfig::default()).expect("validate_project"); | ||
|
|
||
| let xp007: Vec<_> = result | ||
| .diagnostics | ||
| .iter() | ||
| .filter(|d| d.rule == "XP-007") | ||
| .collect(); | ||
| assert!( | ||
| xp007.is_empty(), | ||
| "each file is under the per-file limit, so XP-007 must stay quiet - that is the gap XP-009 fills: {xp007:?}" | ||
| ); | ||
|
|
||
| let xp009: Vec<_> = result | ||
| .diagnostics | ||
| .iter() | ||
| .filter(|d| d.rule == "XP-009") | ||
| .collect(); | ||
| assert_eq!( | ||
| xp009.len(), | ||
| 1, | ||
| "a 42 KB chain exceeds the 32768-byte cap and must be reported once: {:?}", | ||
| result.diagnostics | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test coverage: the four tests cover the happy path and one suppression path, but not the cases where the implementation is actually wrong. Missing, in rough priority order:
- Sibling subtrees -
root 10 KB + pkg-a/ 12 KB + pkg-b/ 12 KBmust stay quiet. It does not today (see theproject_level.rscomment); every fixture here is a single linear chain, which is why the bug survived. - Which file is reported - this test asserts
xp009.len() == 1but never checksd.file. Reporting on the wrong file is the exact failure mode the sibling bug produces, and the assertion cannot see it. Assertd.fileends withapi/AGENTS.mdfor a20 + 20 + 5 KBchain, and assert the numbers ind.message. - Boundary - a chain summing to exactly 32768 must stay quiet; 32769 must fire.
check_byte_limithas these for XP-007 (schemas/cross_platform.rs:2852-2864); XP-009's own>comparison has none, so an off-by-one here would pass. - Nested
AGENTS.override.md- the override test only covers an override at the root. I probedroot AGENTS.md 20 KB + api/AGENTS.override.md 20 KBand it fires correctly, but nothing in the suite pins that. - Rule-level disable -
disable_rule("XP-009")(as opposed to a per-file[[overrides]]) is untested; theconfig.is_rule_enabledguard at line 326 has no coverage.
Also worth noting: three of these four tests are not about [[overrides]] at all, and this file's header describes it as the [[overrides]] end-to-end suite. xp009_respects_per_file_overrides belongs here; the other three read more naturally next to the other project-level rule tests.
|
Claude finished @avifenesh's task in 3m 42s —— View job Re-review after
|
| fixture | real Codex chains | XP-009 on this branch |
|---|---|---|
root 10 KB + pkg-a/ 12 KB + pkg-b/ 12 KB |
22 KB, 22 KB | fires on pkg-b/AGENTS.md, "34816 bytes across 3 file(s)" |
root 2 KB + 20 × pkgNN/ 2 KB |
every chain 4 KB | fires on pkg15/AGENTS.md, "34816 bytes across 21 file(s)" |
The second row is the shape this rule will actually meet: a 21-file monorepo where no chain is within 8× of the cap, blamed on an arbitrary middle package. And it applies to this repo - agnix's own 19 AGENTS.md files sum to 27,741 bytes, so agnix is ~5 KB of fixtures away from XP-009 failing its own self-lint on a file that has nothing to do with any other.
2. is_instruction_file() + AGENTS.override.md makes XP-004/005/006 flag override pairs (blocking). Confirmed: root AGENTS.md "Use npm run build" + AGENTS.override.md "Use pnpm build" →
XP-004 AGENTS.override.md uses pnpm but AGENTS.md uses npm for build commands. XP-009 shadows correctly at project_level.rs:415; the conflict rules do not.
3. bytes and count describe different sets. Confirmed on 20 + 20 + 5 KB: "reaches 40960 bytes across 3 file(s)" - 40960 is two files.
4. Double-escaped \\n in the XP-009 rules.json examples, in both copies, shipping literal \n into website/docs/rules/generated/xp-009.md. Same entry still has no "description" and still sits between XP-007 and XP-008.
5. Test gaps - no sibling fixture, no assertion on d.file or on the message numbers, no boundary test, no nested-override case, disable_rule("XP-009") untested, AGENTS.override.md never added to test_is_instruction_file.
6. config.fs().read_to_string bypasses file_utils::safe_read_file - still the only such call in project_level.rs.
7. Stale rule enumerations - project_level.rs:36-42, pipeline.rs:630, pipeline.rs:959, agnix-lsp/src/backend.rs:257, rules/mod.rs:196-197 all still say "AGM-006, XP-004/005/006, VER-001", and rules/cross_platform.rs:127 still reads "needs project-level accounting; tracked separately".
8. CHANGELOG - the "unreachable in practice" claim is still there and still wrong (detection.rs:351-352 maps AGENTS.override.md -> FileType::ClaudeMd and is untouched by this diff), and [Unreleased] still carries two ### Added headings.
Two things I can confirm work correctly, and that only need pinning with tests rather than fixing: the 32768/32769 boundary behaves right (quiet at exactly the limit, fires at +1), and lowercase agents.md is skipped by the chain builder as intended.
What I verified, and how
Executed on this branch (cargo test -p agnix-core, probe fixtures, since removed): the two sibling false positives, the bytes/count mismatch, the XP-004 override-pair firing, the new XP-007/XP-009 double report, and the boundary and lowercase cases. Read-only: the stale enumerations, the caller set for is_instruction_file, the rules.json raw escapes and entry ordering, detection.rs, and the repo's own AGENTS.md byte total. Not run: the full 5060-test suite or the eval - CI covers those, and none of the findings above depend on them.
Findings 1, 2 and the new double-report are behavioural and I would not merge without them; the rest are correctness-of-record and bookkeeping.
· branch fix/xp009-cumulative-cap-and-cline-baseline
| if total > CODEX_BYTE_LIMIT && truncated_at.is_none() { | ||
| truncated_at = Some((path.clone(), total)); | ||
| } | ||
| } |
There was a problem hiding this comment.
New finding (not in the previous pass): XP-009 double-reports every file XP-007 already flags.
A chain of length 1 whose single file is over the cap satisfies total > CODEX_BYTE_LIMIT on the first iteration, so XP-009 fires on the same file XP-007 just flagged, with a differently-worded message. Verified on this branch with a single 40 KB root AGENTS.md:
XP-007 AGENTS.md :: AGENTS.md exceeds Codex CLI byte limit (40960 bytes, max 32768)
XP-009 AGENTS.md :: Codex instruction chain reaches 40960 bytes across 1 file(s), ...
Two diagnostics, one file, one cause. "instruction chain ... across 1 file(s)" also reads wrong - there is no chain. Every project that today gets a single XP-007 gains a duplicate on this PR, which is a real regression in output noise for the most common oversized-AGENTS.md case, and it is not covered by any test here (xp009_flags_cumulative_chain_over_the_cap asserts XP-007 is silent, i.e. only the disjoint case).
The gap XP-009 exists to close is chains where no single file is over. Suggest skipping when the crossing file is itself over the limit - XP-007 owns that - so the two rules partition the space instead of overlapping:
// XP-007 already reports a single file over the cap; XP-009 covers only
// the cumulative case no per-file check can see.
if total > CODEX_BYTE_LIMIT && truncated_at.is_none() && size <= CODEX_BYTE_LIMIT {
truncated_at = Some((path.clone(), total));
}(and pin it with a test asserting exactly one diagnostic for the single-oversized-file case).
a13210f to
17e5cae
Compare
|
Claude finished @avifenesh's task in 7m 55s —— View job Third review - branch unchanged, all findings stand
Nothing has been pushed since the last review. Blocking1. The chain sums across sibling subtrees.
This repo is close to tripping it. agnix's own 19 Fix: sum each root-to-leaf directory chain independently, record the crossing per chain, dedupe per file. 2. The function's only two non-test callers are That is the override file doing precisely what Codex documents, reported twice as a conflict. This is new with this PR - on 3. XP-009 double-reports every file XP-007 already flags. A chain of length 1 whose single file is over the cap satisfies Two diagnostics, one file, one cause - and "instruction chain ... across 1 file(s)" reads wrong, there is no chain. This is the single most common oversized- Should fix4. 5. Read path. 6. 7. Stale rule enumerations - eight sites, one more than previously reported. The module header was updated; these were not:
And 8. Test gaps. Every fixture is a single linear chain, which is why finding 1 survived two pushes. Missing: sibling-subtree regression; an assertion on 9. CHANGELOG. The claim that this made XP-007's override handling "unreachable in practice" does not hold. XP-007 is dispatched from 10. Evidence wording. "it can under-report, never over-report" appears in Minor
Two behaviours that are already correct and only need pinning with tests: the 32768/32769 boundary (quiet at the limit, fires at +1) and the lowercase skip. What is fine
How I verifiedBuilt Findings 1, 2 and 3 are behavioural and I would not merge without them. Note: |
| let mut truncated_at: Option<(PathBuf, usize)> = None; | ||
|
|
||
| for path in &chain { | ||
| let size = match config.fs().read_to_string(path) { |
There was a problem hiding this comment.
New this pass: XP-009 and XP-007 measure the same file with different rulers on CRLF checkouts.
pipeline.rs:459 normalizes content (let content = normalize_line_endings(&raw_content);) before any per-file validator runs, so XP-007's check_byte_limit sees LF-only bytes. XP-009 here reads raw through config.fs().read_to_string, so it counts the \rs. XP-004/005/006 sixty lines up normalize too (project_level.rs:131-137), so this is the only unnormalized size measurement in the crate.
On a CRLF working copy a 1000-line file contributes 1000 more bytes to XP-009's total than to XP-007's. Which one is right depends on what Codex reads off disk - raw, so XP-009's number is arguably the faithful one and XP-007 is the one under-counting - but the two rules must not disagree about the same file's size when their messages both quote project_doc_max_bytes. Pick one and state it.
This compounds the read-path point from the previous review (this is still the only config.fs().read_to_string in the module; file_utils::safe_read_file is what everything else uses, and fs::metadata().len() would sidestep both the normalization question and the Err(_) => continue hole that silently drops non-UTF-8 files from the total).
Verified by reading pipeline.rs:455-462 and project_level.rs:125-140; not exercised with a CRLF fixture.
| // onward is what Codex drops. | ||
| if config.for_path(&report_path).is_rule_enabled("XP-009") { | ||
| diagnostics.push( | ||
| Diagnostic::warning( |
There was a problem hiding this comment.
Minor, new this pass: column convention. This is Diagnostic::warning(report_path, 1, 1, ...). Every other diagnostic emitted from this module uses column 0 - AGM-006 at line 91, XP-004 at 138, XP-005 at 181/224, XP-006 at 269, VER-001 at 306. Confirmed in the JSON output of a probe run: XP-009 lands at "column": 1 while AGM-006 on the same file lands at "column": 1 in the CLI's 1-based rendering of 0, i.e. the two are actually off by one against each other in the LSP's 0-based character positions.
1, 0 matches the module; 1, 1 matches XP-007 in the per-file validator. Since this rule lives here and is merged into the LSP's project-level diagnostic cache (agnix-lsp/src/backend.rs:257), 0 is the consistent choice.
While in this block, the two items already raised still apply: the count = chain.len() / bytes = running_total mismatch (measured "reaches 40960 bytes across 3 file(s)" on a 20+20+5 KB chain, where 40960 is two files), and the missing size <= CODEX_BYTE_LIMIT guard that makes this fire alongside XP-007 on every single oversized AGENTS.md.
|
Both blocking findings fixed in 9617d24. Reproduced each before touching code. 1, sibling subtrees. You were right, and the reproduction matched your numbers exactly: The argument that settled it was yours: it contradicts AGM-006 two hundred lines above in the same file, which recommends splitting across nested directories precisely to stay under this cap. A rule punishing the layout its sibling recommends is wrong regardless of the arithmetic. Also confirmed your measurement that this repo was ~5 KB from tripping its own rule — 19 files, 27,641 of 32,768. It now reports nothing. Now one chain per leaf directory, summed independently, reported on the file where that chain crosses, deduped so a shared root isn't reported once per descendant. 2, override pairs. Also correct, and a consequence of my own fix: adding Verified a genuine Four regression tests cover both directions of each. One note on process: I'd initially pushed the A-tier work (#1288) onto this branch, which conflated two issues. Split it out to #1293 so each is reviewable on its own — this PR is back to XP-009 plus the Cline baseline only. |
|
Claude finished @avifenesh's task in 6m 55s —— View job Fourth review -
|
| finding | fixture | result |
|---|---|---|
| 1. sibling subtrees | root 10 KB + pkg-a 12 KB + pkg-b 12 KB |
0 XP-009 ✅ |
root 2 KB + 20 × pkgNN 2 KB |
0 XP-009 ✅ | |
| 2. override pairs in XP-004/005/006 | AGENTS.md npm + AGENTS.override.md pnpm |
0 XP-004/005/006 ✅ |
CLAUDE.md npm vs AGENTS.override.md pnpm |
XP-004 still fires ✅ - exclusion, not disablement |
Also confirmed still correct: 32768 quiet / 32769 fires; a deep 20+20+5 chain reports on api/AGENTS.md, the crossing file; the new tests assert d.file, which they did not before.
The codex_instruction_chains rewrite is the right model - one chain per leaf, ancestors only, depth-sorted, per-chain sums. The VALIDATION-RULES.md Detection text was updated to match, so evidence and code agree again.
New: the dedup does not do what the commit message claims
"deduped across chains so a shared root file is not reported once per descendant"
That holds only when the root itself is the crossing file. When the root is large but under the cap and each sibling tips its own chain over, the crossing file differs per chain, so reported gets one entry per sibling:
root 30 KB + 3 x pkg/5 KB
XP-009 pkg0/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
XP-009 pkg1/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
XP-009 pkg2/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
root 30 KB + 20 x pkg/5 KB -> XP-009 diagnostic count = 20
These are not false positives - every chain really does truncate. The attribution is wrong. Twenty 5 KB files are blamed for a 30 KB root, and the suggestion tells the reader to trim the file it points at; trimming any one of those twenty removes 5 KB from a chain that needs 30 KB removed. The one edit that fixes all twenty - trim the root - is never named. This is the previous bug one step along: it used to fire once with the wrong total, it now fires N times against the wrong file, and it still scales with package count rather than with the number of distinct causes.
Not blocking on its own, but it is the same class as the finding the fix was for, and no test can see it: xp009_does_not_sum_across_sibling_subtrees uses siblings that stay under the cap, so the sibling case that does cross is unexercised.
New: three places now disagree about AGENTS.override.md
The new shadow filter matches == Some("AGENTS.override.md") exactly; codex_instruction_chains's precedence matches exactly; is_instruction_file() matches eq_ignore_ascii_case. So a lowercase agents.override.md is collected as an instruction file, shadows nothing, and is absent from the chain - measured: the 40 KB AGENTS.md it should shadow is still summed and XP-009 fires on it. Pick one convention across all three.
Related and still true: XP-007 does not share the shadowing model this PR establishes. Small AGENTS.override.md + 40 KB shadowed AGENTS.md still yields XP-007 AGENTS.md, for bytes Codex never reads - while xp009_counts_one_file_per_directory_preferring_override in this same PR asserts those bytes "must not count". Two rules, one PR, opposite positions on the same file. Not fixable from a per-file validator; worth a line in the XP-007 docs.
Previously reported, unchanged - re-verified
XP-007 / XP-009 double report. A single 40 KB root AGENTS.md still emits both, and "instruction chain ... across 1 file(s)" still reads wrong:
XP-007 AGENTS.md "AGENTS.md exceeds Codex CLI byte limit (40960 bytes, max 32768)"
XP-009 AGENTS.md "Codex instruction chain reaches 40960 bytes across 1 file(s), ..."
&& size <= CODEX_BYTE_LIMIT on the guard partitions the two rules.
bytes vs count. 20 + 20 + 5 KB still gives "reaches 40960 bytes across 3 file(s)" - 40960 is two files. idx + 1, not chain.len().
Read path. project_level.rs:361 is still the only config.fs().read_to_string in the module and still the only unnormalized size measurement in the crate (pipeline.rs:459 and project_level.rs:127-133 both normalize), so XP-007 and XP-009 measure the same file differently on a CRLF checkout. Err(_) => continue still drops non-UTF-8 files silently. fs::metadata().len() resolves all three.
rules.json bookkeeping. Still \\n rather than \n in both good_example and bad_example (line 11608-11609), in both copies of the file; website/docs/rules/generated/xp-009.md:41,47 still ships literal \n in its code fences. Entry still has no "description", and still sits between XP-007 and XP-008, which is why the generated index lists them out of order. Re-run node scripts/sync-rule-bookkeeping.js after fixing.
Stale rule enumerations - all eight still present. project_level.rs:34-42 (the run_project_level_checks doc comment lists AGM-006/XP-004/005/006/VER-001, no XP-009); pipeline.rs:630, :647, :959; agnix-lsp/src/backend.rs:257; rules/mod.rs; agnix-cli/tests/rule_parity.rs:289. And rules/cross_platform.rs:127 still reads "Catching that needs project-level accounting; tracked separately" - VALIDATION-RULES.md:3341 was pointed at XP-009, the code comment it mirrors was not.
CHANGELOG. The "unreachable in practice" claim is still there and still wrong - file_types/detection.rs:352 maps AGENTS.override.md -> FileType::ClaudeMd and is untouched by this diff, so XP-007 reaches override files through a path that never calls is_instruction_file(). [Unreleased] still carries two ### Added headings.
Column convention. XP-009 emits column 1; AGM-006/XP-004/005/006/VER-001 in the same function all emit 0.
Test gaps - four new tests close the two biggest ones (sibling regression, d.file assertion). Still missing: siblings that do cross (the new finding above); an assertion on the numbers in d.message; the exact-32768 boundary (behaviour is correct, just unpinned); a nested AGENTS.override.md; disable_rule("XP-009"); AGENTS.override.md in test_is_instruction_file (cross_platform.rs:2044, still asserts only CLAUDE.md/AGENTS.md/.clinerules/cursor/copilot).
Assessment
The two blocking findings are genuinely fixed and I verified both by execution - the chain model is now right, and the exclusion is an exclusion rather than a mute. Nothing here is worse than what was on the branch before.
What I would still want before merge: the XP-007/XP-009 overlap (a duplicate diagnostic on the most common oversized-AGENTS.md shape, one line to fix) and the bytes/count mismatch (an arithmetically impossible message, one line to fix). The N-per-sibling attribution and the three-way AGENTS.override.md case-sensitivity split are new and real but smaller. The bookkeeping items - \\n, the eight stale enumerations, the CHANGELOG claim - are correctness-of-record and cheap.
The Cline v4.1.2 half remains correct and needs nothing.
This was my third and final review of this PR, so the remaining items are listed in full above rather than held back for a next pass.
Note: CLAUDE.md shows modified in the working tree with a 443→442 rule-count revert. It is a checkout artifact, predates this session, and is not in the branch - the committed CLAUDE.md correctly says 443.
· branch fix/xp009-cumulative-cap-and-cline-baseline
| if total > CODEX_BYTE_LIMIT { | ||
| // Report on the file that crosses the limit: it and anything | ||
| // deeper in this chain is what Codex drops. Deduped across | ||
| // chains, keeping the largest total, so a shared root file | ||
| // is not reported once per descendant. | ||
| reported | ||
| .entry(path.clone()) | ||
| .and_modify(|entry| { | ||
| if total > entry.0 { | ||
| *entry = (total, chain.len()); | ||
| } | ||
| }) | ||
| .or_insert((total, chain.len())); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
New this pass: the dedup does not do what the commit message says, and the noise now scales with package count instead of the byte total.
The commit says the crossings are "deduped across chains so a shared root file is not reported once per descendant." That holds only when the root itself is the crossing file. When the root is large but under the cap and each sibling tips its own chain over, the crossing file is a different file in every chain, so reported gets one entry per sibling. Measured on this branch:
root 30 KB + 3 x pkg/5 KB
XP-009 pkg0/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
XP-009 pkg1/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
XP-009 pkg2/AGENTS.md "reaches 35840 bytes across 2 file(s) ..."
root 30 KB + 20 x pkg/5 KB
XP-009 diagnostic count = 20
Every chain genuinely truncates, so these are not false positives - but the attribution is. Twenty 5 KB files are blamed for a 30 KB root, and rules.xp_009.suggestion tells the reader to trim the file it points at, which for any of those twenty removes 5 KB from a chain that needs 30 KB removed. Trimming the root fixes all twenty at once and is never named.
This is the same "scales the wrong way" shape as the sibling bug just fixed, one level along: the old version fired once with the wrong total, this one fires N times with the wrong file. Suggest attributing the crossing to the largest contributor at or above the crossing point in that chain (or emitting one diagnostic per shared prefix rather than per leaf) so the count tracks distinct causes, not distinct packages.
Two previously-reported items in this same block are unchanged and still reproduce:
bytesvscount.running_totalstops at the crossing file,chain_lenis the whole chain.20 + 20 + 5 KBstill yields "reaches 40960 bytes across 3 file(s)" onapi/AGENTS.md- 40960 is two files, and the third file's 5 KB is exactly what Codex drops.idx + 1instead ofchain.len().- XP-007 double-report. A chain of length 1 crosses on its first element, so a single 40 KB root
AGENTS.mdstill emits both:AddingXP-007 AGENTS.md "AGENTS.md exceeds Codex CLI byte limit (40960 bytes, max 32768)" XP-009 AGENTS.md "Codex instruction chain reaches 40960 bytes across 1 file(s), ..."&& size <= CODEX_BYTE_LIMITto the guard at line 364 makes the two rules partition the space, and "chain ... across 1 file(s)" stops appearing.
| // Drop any `AGENTS.md` shadowed by an `AGENTS.override.md` in the same | ||
| // directory. Codex "includes at most one file per directory", checking | ||
| // the override first, so the shadowed file is never loaded - the doc's | ||
| // own tree labels it "Ignored because an override exists". Comparing the | ||
| // two as peers reported a conflict between a file and the thing whose | ||
| // whole purpose is to differ from it. | ||
| let shadowed: std::collections::BTreeSet<PathBuf> = file_contents | ||
| .iter() | ||
| .filter(|(path, _)| { | ||
| path.file_name().and_then(|n| n.to_str()) == Some("AGENTS.override.md") | ||
| }) | ||
| .filter_map(|(path, _)| path.parent().map(|dir| dir.join("AGENTS.md"))) | ||
| .collect(); | ||
| if !shadowed.is_empty() { | ||
| file_contents.retain(|(path, _)| !shadowed.contains(path)); | ||
| } |
There was a problem hiding this comment.
The shadowing fix works - verified - but it is case-sensitive while the list it filters is not.
Confirmed working: root AGENTS.md "npm run build" + AGENTS.override.md "pnpm build" now produces no XP-004/005/006, and a genuine CLAUDE.md-vs-override conflict still fires. So this is an exclusion, not a disablement, as claimed.
Three follow-ups, all in the same class - this PR now has three independent places that decide "is this an override / is this shadowed", and they disagree:
- Here (
== Some("AGENTS.override.md")) - exact match. codex_instruction_chainsprecedence- exact match.is_instruction_file()(schemas/cross_platform.rs:1011) -eq_ignore_ascii_case("agents.override.md").
So a lowercase agents.override.md is collected as an instruction file but shadows nothing and is absent from the chain. Measured on a fixture with agents.override.md + a 40 KB AGENTS.md: the shadowed 40 KB file is still summed and XP-009 fires on it. Pick one convention across all three - exact match everywhere (and drop the eq_ignore_ascii_case for this name) is the simpler resolution and matches XP-007.
-
XP-007 does not share this shadowing model. Verified: small
AGENTS.override.md+ 40 KB shadowedAGENTS.mdstill yieldsXP-007 AGENTS.md, for bytes Codex never reads - whilexp009_counts_one_file_per_directory_preferring_overridein this same PR asserts those bytes "must not count". Two rules in one PR now hold opposite positions on the same file. Not fixable from a per-file validator, but it should be stated in the XP-007 docs rather than left to be discovered. -
Ordering nit with teeth: the XP-004 read-error diagnostic at line 138 is emitted for the shadowed
AGENTS.mdbefore thisretaindrops it, so an unreadable shadowed file still produces a diagnostic about a file Codex never opens.
|
All three fixed in fedcaa3. 1, attribution. Reproduced your exact numbers — and confirming your framing, this was the sibling bug one step along: N reports against the wrong file instead of one report with the wrong total. Now attributed to the largest contributor in the chain, tie-broken toward the shallowest. A deep file that dominates its chain is still named, so it isn't just "always blame the root" — there's a test for that direction too. Your point that no test could see it was the useful half: 2, three conventions for one filename. Correct. 3, XP-007 asymmetry. Recorded in XP-007's rule docs rather than left implicit. You're right that two rules in one PR were taking opposite positions on the same file, and right that it isn't fixable from a per-file validator — it needs the project-level shadowing model XP-009 has. One process note: my first attempt to reproduce (1) through the CLI showed zero diagnostics and I nearly wrote it off. The temp fixtures were being excluded by the directory walk; reproducing through Verified: 37 binaries green, eval 61/61, fmt + clippy |
|
[INFO] Claude Code review has already run 3 times for this pull request, so further pushes will not be reviewed automatically. Comment |
Closes #1289. Also clears #1282 (Cline v4.1.2 triage). XP-007 checks each AGENTS.md against the 32 KiB limit in isolation, but the documented cap is cumulative: Codex "stops adding files once the combined size reaches the limit defined by project_doc_max_bytes". A project split across several mid-size files is therefore truncated with every per-file check passing - the gap recorded in XP-007's rule docs when that dimension was deferred. XP-009 is a project-level rule modelled on the documented discovery order: start at the project root, walk down to the working directory, take at most one file per directory preferring AGENTS.override.md over AGENTS.md, sum root-first and report on the file where the running total crosses the limit, since that file and everything deeper is what Codex drops. It reuses the paths the project walk already collected, so it adds no traversal. Deliberately conservative: project_doc_fallback_filenames lives in the user's Codex config rather than anything agnix reads, so a project using fallbacks has a longer real chain than this models. That makes the check under-report rather than over-report, which is the right direction for a SHOULD-level warning. Writing the tests surfaced a second bug. AGENTS.override.md was not in is_instruction_file(), so it was invisible to every cross-platform rule - which means the AGENTS.override.md handling added to XP-007 in the previous release could never fire. A test asserting that a small override shadows a 40 KB AGENTS.md failed because only the AGENTS.md was ever collected. Fixed at the source. Tests cover both directions: a 42 KB three-file chain fires while XP-007 stays quiet (the gap), a 12 KB chain stays clean, a shadowed AGENTS.md does not inflate the total, and [[overrides]] suppresses it. Cline v4.1.2 (#1282): reviewed all three releases since the v4.0.12 baseline, not just the newest. v4.1.0 is an A/B packaging change, v4.1.1 removes internal MCP server-key machinery, v4.1.2 adds a settings-page label - all matching the declared-irrelevant list for this tool, and none touching .clinerules parsing, workflow/hook/skill layouts, or skill frontmatter. Baseline and the RESEARCH-TRACKING date advanced with no rule changes.
CI clippy runs with -D warnings, which promotes clippy::items-after-test-module to an error. My local run used --all-targets without -D warnings, so it passed. Using the strict form locally from here.
Two blocking findings from review, both reproduced before fixing. XP-009 summed every discovered AGENTS.md into one whole-tree total, but a Codex chain is a single root-to-cwd path - sibling subtrees are never concatenated. Measured: root 10 KB + pkg-a 12 KB + pkg-b 12 KB reported pkg-b at 34 KB, when the real chains are 22 KB each and both fit. It also scaled the wrong way, since enough packages cross the cap however small each one is, and it contradicted AGM-006 two hundred lines above in the same file, which recommends splitting across nested directories precisely to stay under this cap. The reviewer also measured that this repo was about 5 KB from tripping its own rule: 19 AGENTS.md files totalling 27,641 bytes against a 32,768 cap. Confirmed, and it now reports nothing. Now one chain per leaf directory, each summed independently, reported on the file where that chain crosses, deduped across chains so a shared root file is not reported once per descendant. Second finding: adding AGENTS.override.md to is_instruction_file() (needed for XP-007 and XP-009) also fed it to XP-004/005/006, which then compared an override against the file it shadows and reported a conflict. Codex "includes at most one file per directory" and checks the override first, so the shadowed file is never loaded - the doc's own tree labels it "Ignored because an override exists". Shadowed AGENTS.md files are now dropped from those detectors' candidate set, the same filter-at-input approach the [[overrides]] handling already uses. Verified a genuine CLAUDE.md-vs-AGENTS.md conflict still fires, so this is an exclusion, not a disablement. Four regression tests: siblings stay clean, a deep chain reports once on the crossing file, an override pair produces no conflict, and a real cross-file conflict still does.
…ng (#1289) Three findings from the fourth review, all in my own fix. Attribution followed the file where the running total crossed, which is not the file that caused it. Reproduced: a 30 KB root with twenty 5 KB packages crosses inside each package, so it emitted twenty diagnostics against twenty 5 KB files - and the suggestion tells the reader to trim the file it names. Trimming any one of them removes 5 KB from a chain that needs 30 KB removed; the single edit that fixes all twenty was never named. As the reviewer put it, this was the sibling bug one step along: N reports against the wrong file instead of one report with the wrong total. Now attributed to the largest contributor in the chain, tie-broken toward the shallowest file. Twenty reports collapse to one, on the root. A deep file that dominates its chain is still named, so this is not just "always blame the root". The reviewer also noted no test could see it: the existing sibling test uses siblings that stay under the cap, so the crossing case was unexercised. Added both directions. Three places named AGENTS.override.md with two conventions: is_instruction_file() matched case-insensitively while the new shadow filter and the chain precedence matched exactly. So a lowercase agents.override.md was collected as an instruction file, shadowed nothing, and was absent from the chain - the 40 KB file it should have hidden was still summed. All three now use eq_ignore_ascii_case, with a test covering both spellings. Recorded the XP-007 asymmetry in its rule docs rather than leaving two rules in one PR taking opposite positions on the same file: XP-007 is a per-file validator with no view of what Codex loads, so it reports a shadowed AGENTS.md on size alone while XP-009 correctly excludes it. Fixing that needs the project-level shadowing model, which a per-file rule cannot reach.
…hangelog] macOS resolves a TempDir under /var to /private/var, so comparing `file.parent()` against `temp.path()` failed on macOS and Windows while passing on Linux - a test bug, not a code bug: the attribution was correct on all three. Both assertions now check path structure (absence of an `api` or `pkgNN` component) rather than equality with the temp root.
The previous commit converted one of the two `parent() == temp.path()` comparisons; the other did not match my replace and survived, so macOS and Windows would still have failed. Both are now structural, and a grep confirms none remain.
b71e8ec to
10bca39
Compare
Closes #1289. Also clears #1282 (Cline v4.1.2 triage) since it needed no rule change.
XP-009: the cap is cumulative, not per-file
From the Codex AGENTS.md guide:
XP-007 checks each
AGENTS.mdin isolation, so a project split across several mid-size files gets truncated with every per-file check passing. This was the dimension deferred in #1286 and recorded in XP-007's own docs rather than left implicit.The new project-level rule models the documented discovery order:
So: root-down, one file per directory, override preferred, summed root-first, reported on the file where the running total crosses the limit — that file and everything deeper is what Codex drops. It reuses the paths the project walk already collected, so it adds no traversal.
Deliberately conservative.
project_doc_fallback_filenameslives in the user's Codexconfig.toml, not anything agnix reads, so a project using fallbacks has a longer real chain than this models. That makes the check under-report rather than over-report — the right direction for a SHOULD-level warning, and stated in the rule docs rather than left as a surprise.The tests found a second bug
AGENTS.override.mdwas not inis_instruction_file(), so it was invisible to every cross-platform rule. Which means theAGENTS.override.mdhandling added to XP-007 in #1286 could never fire — I shipped an unreachable fix.Caught by a test asserting a small override shadows a 40 KB
AGENTS.md: it failed because only theAGENTS.mdwas ever collected. Fixed at the source, so XP-007's override handling now works too.Tests assert both directions:
AGENTS.md[[overrides]]on the reported fileCline v4.1.2 (#1282)
Reviewed all three releases since the
v4.0.12baseline, not just the newest one the issue named:v4.1.0— A/B packaging: one VSIX with both extensions plus a loaderv4.1.1— removes internal MCP server-key machinery fromMcpHubv4.1.2— settings-page label showing which variant is activeAll three match this tool's declared-irrelevant list (
Webview UI updates,cline-core internal…,Provider plumbing). None touches.clinerulesparsing, workflow/hook/skill layouts, or skill frontmatter. Confirmed agnix validates.clinerulesand skill files, not Cline's MCP internals, so v4.1.1 is out of scope too.Baseline advanced to
v4.1.2and theRESEARCH-TRACKING.mdreview date updated. No rule changes.Verification
5060+ tests / 37 binaries green · eval 61/61 · self-lint clean ·
cargo fmt --checkandcargo clippy --all-targetsclean · bookkeeping, locale sync and rule counts in sync at 443 ·actionlint+shellcheckclean.Rule count 442 → 443.