fix: A-tier spec drift for Cursor, Copilot and Cline (#1288) - #1293
Conversation
Closes #1288. Audited the six A-tier sources that had never been reviewed and whose baselines were left stale in #1287 for exactly that reason. Nine false positives, each reproduced against the doc's own example before fixing and re-verified after. Copilot: - COP-005 listed `coding-agent`, which was never an upstream value. The documented pair is `code-review` / `cloud-agent`, so the doc's own value errored - and the unsafe autofix rewrote a correct value into the invalid one via find_closest_value. `coding-agent` stays as a deprecated alias so configs written against agnix's own wrong advice keep working, ordered last so the fixer prefers the documented spelling. 56 occurrences across 22 files: fixing only the const would have left every user-facing message wrong. - COP-003 rejected comma-separated `applyTo`, which the doc documents with `"**/*.ts,**/*.tsx"` as its example. A correct brace/bracket-aware splitter already existed with 18 passing tests, marked `#[allow(dead_code)] // reserved for future use` and never called. The fix was wiring, not new logic. Cursor: - CUR-004 had the same comma bug. Notable because it was *intermittently* wrong: the doc's literal row parses as one pattern while `src/**, tests/**` does not, so a fixture built from the doc's own example would have passed. - CUR-010 required `version`, documented with a default of 1 and omitted by several of the doc's examples. Now optional, and typed number rather than integer. - CUR-012 required `command` on every hook entry, so the documented prompt hook - `type`, `prompt`, `timeout`, no `command` - errored. CUR-018 already covers `prompt` for those. - CUR-016 required `install`. Fetched the published schema: no `required` array in `definitions.common`, `required: []` in `definitions.container`, and the setup page's snapshot example omits it. Terminal entries now need only `command`, the array `oneOf` branch is accepted, and `update` - absent from a schema that sets unevaluatedProperties: false - is reported as renamed rather than blessed. - Two timeout suggestions said milliseconds; the doc says seconds. Cross-tool: - CC-MEM-014 fired on `.cursor/rules/*.mdc`, reporting a Cursor file as "CLAUDE.md" against Claude's 200-line limit where Cursor documents 500. ClaudeMdValidator is registered for CursorRule for its generic prose checks; the Claude-specific line rule is now gated on the file actually being a Claude memory file. - 18 rules carried evidence URLs that redirect (Copilot 301, Cline 308). Refreshed in rules.json and spec-baselines.json so those two sentinel sources can match again. Cline needed no rule change: `paths` is documented as a YAML array and the scalar form is already reported, so comma-splitting there would be wrong - the two audits disagreed on this and the docs settled it per tool rather than as a blanket change. Deliberately not done: user-global paths (`~/.copilot/`, `~/Documents/Cline/ Rules`, `~/.agents/AGENTS.md`) are outside the project tree agnix walks, so adding rules there is a scope decision rather than a drift fix.
|
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 12m 13s —— View job Review: A-tier spec drift (#1293)Nine claimed false positives; I re-fetched every upstream source rather than taking the PR body's quotes on trust, and re-ran the affected paths against the built CLI. Seven of the nine are correct and well-evidenced. Two changes are wrong in a way that reintroduces the exact class of bug being fixed, and one has no test at all.
Findings1. COP-005: the unsafe autofix can still write
Related, softer: the alias is accepted with no diagnostic at all, so a user who followed agnix's old advice ships a file Copilot won't honour and never hears about it - and 2. CUR-016 terminals: the I fetched
So a false negative was added and a false positive of exactly the class this PR targets was left in place. The 3. CC-MEM-014 gating has no test, and introduces a second definition of "Claude memory file" (inline) - CONFIRMED by reading Nothing asserts a >200-line 4. Same class, not covered: CC-MEM-009 - PLAUSIBLE, by reading CC-MEM-009 ( 5. Nits (behavioural only where noted)
What I verified as correctUpstream re-fetched today, all four sources back the PR's claims verbatim:
Also checked:
Not verified: the two redirect claims (301/308) - I did not re-run Findings 1 and 2 are the blocking ones; 3 is required by CLAUDE.md rule 7. The rest of the diff is accurate work with unusually good evidence discipline. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // here only so configs written against agnix's own previous | ||
| // wrong advice keep validating, and it is deliberately last | ||
| // so `find_closest_value` prefers the documented spelling. | ||
| const VALID_AGENTS: &[&str] = &["code-review", "cloud-agent", "coding-agent"]; |
There was a problem hiding this comment.
The unsafe autofix can still write coding-agent - the same failure mode this PR is fixing, just reached by a different input.
VALID_AGENTS is used both as the acceptance list here and as the fix-candidate list at line 1331. find_closest_value falls back to a substring match, so any invalid value that matches only the deprecated alias gets rewritten to it. The "ordered last" comment only decides ties.
Reproduced on this branch with the built CLI:
$ cat .github/instructions/x.instructions.md
excludeAgent: "coding"
$ agnix . --fix-unsafe
error: Invalid excludeAgent value 'coding'. Valid values: 'code-review', 'cloud-agent' [fixable]
Fixed: - Change excludeAgent to 'coding-agent'
$ cat .github/instructions/x.instructions.md
excludeAgent: "coding-agent"
So the message names the two documented values and the fix then writes the third, undocumented one. Splitting the lists fixes it:
/// Documented upstream - the only values the fixer may produce.
const DOCUMENTED_AGENTS: &[&str] = &["code-review", "cloud-agent"];
/// Additionally tolerated on input: written against agnix's own previous wrong advice.
const ACCEPTED_AGENTS: &[&str] = &["code-review", "cloud-agent", "coding-agent"];then ACCEPTED_AGENTS.contains(...) at the check and find_closest_value(value, DOCUMENTED_AGENTS) at the fix. A test asserting the replacement text (not just that a diagnostic exists) would have caught this - the three new COP-005 tests all assert presence/absence only.
Separately: accepting coding-agent silently means a user who followed the old advice gets no signal that Copilot won't honour their file, and rules.json cites no source for the value. A LOW/INFO deprecation diagnostic would keep them working and tell them.
| } else if terminal.as_array().is_none() { | ||
| // The `oneOf`'s other branch is an array of | ||
| // command-bearing objects, so a nested list is valid | ||
| // even though it reads oddly. |
There was a problem hiding this comment.
The oneOf is on the terminals value, not on its items - so this branch accepts what the schema rejects, while the form the schema accepts is still rejected at line 909.
I fetched cursor.com/schemas/environment.schema.json to check:
- branch 1:
terminalsis an array of objects, each requiringcommand - branch 2:
terminalsis a single object requiringcommand
There is no "array of arrays" branch. Both directions verified against the built CLI on this branch:
// invalid per schema - agnix reports nothing at all
{ "terminals": [ [ { "name": "no-command-here" } ] ] }
// valid per schema (branch 2) - agnix: error "Field 'terminals' must be an array, got object"
{ "install": "npm ci", "terminals": { "command": "npm run dev", "name": "dev" } }The name/description relaxation above is right; this escape hatch is not. Suggested shape: drop the as_array() exemption (a nested array is terminal_not_object), and handle JsonValue::Object at line 909 by running the same command check instead of erroring. The comments at lines 874-877 and 893-895, the CHANGELOG line ("the array branch is accepted") and VALIDATION-RULES.md CUR-016 ("the oneOf also permits an array branch") all repeat the same misreading and need the same correction.
Neither form has a test - test_cur_016_terminal_name_is_optional and test_cur_016_terminal_command_still_required both use the array-of-objects branch only.
| // file was reported as "CLAUDE.md has N lines" against Claude's limit - | ||
| // wrong filename, wrong threshold (Cursor documents 500), wrong tool. | ||
| // Gated on the file actually being a Claude memory file. | ||
| let is_claude_memory = matches!(crate::detect_file_type(path), crate::FileType::ClaudeMd); |
There was a problem hiding this comment.
The gate is correct today, but it is the second, differently-derived notion of "Claude memory file" in this one function.
Line 69 already computed is_claude_md = matches!(filename, "CLAUDE.md" | "CLAUDE.local.md"), and the early return at line 71 is the only reason this new gate agrees with it: detect_file_type also maps AGENTS.md, AGENTS.local.md and AGENTS.override.md to FileType::ClaudeMd (file_types/detection.rs:351), and the comment at line 67 says CC-MEM rules deliberately skip AGENTS.*. If line 69 ever gains a filename, CC-MEM-014's scope changes silently in the opposite direction. if is_claude_md && config.is_rule_enabled(...) says the same thing with no second source of truth and no path re-parse.
More important: there is no test for this change in either direction. Nothing asserts that a .cursor/rules/*.mdc over 200 lines no longer reports CC-MEM-014, and nothing asserts CLAUDE.local.md still does - the three existing tests (lines 1099-1153) all pass Path::new("CLAUDE.md"), which was passing before the change too. Per CLAUDE.md rule 7 this needs the pair.
…als (#1288) Review found the COP-005 fix reintroduced its own bug through a different input, and a real gap in the terminals branch. VALID_AGENTS doubled as the acceptance list and the fix-candidate list, and find_closest_value also does substring matching. So `excludeAgent: "coding"` was "fixed" to `coding-agent` while the message on the same diagnostic said the valid values are code-review/cloud-agent - exactly the bug this rule was corrected for. Reproduced with --fix-unsafe before fixing. Split into DOCUMENTED_AGENTS (fix candidates) and ACCEPTED_AGENTS (input tolerance); `coding` now reports without a fix rather than being guessed at, which is the right outcome for an input that matches no documented value. The three new COP-005 tests all asserted that a diagnostic fired and none asserted its replacement text, which is why this survived. Added a test that checks the fix text directly. The legacy spelling is also now reported at info level instead of accepted silently. A user who followed agnix's old advice was shipping a file Copilot will not honor and hearing nothing about it. Terminals: the reviewer read the `oneOf` as sitting on the `terminals` value rather than on `items`. I re-fetched the schema and the original reading was right - branch 1 is an array of command-bearing objects, branch 2 is one such object - so the branches are not inverted. But accepting the array branch without checking inside was a real gap: `[[{"name":"x"}]]` passed with no `command`. Its contents are now validated, with a test for both directions and one for the scalar case that matches no branch.
|
Fixed in 6750bf1. One of your two "wrong" findings was right and one wasn't — details below, with the measurement for each. 1, the autofix could still write Split into Your diagnosis of why it survived was the useful part: all three new COP-005 tests asserted a diagnostic fired, none asserted the replacement text. Added a test that checks the fix text directly. Also took the softer point — the legacy spelling now reports at info level instead of being accepted silently. A user who followed agnix's old advice was shipping a file Copilot won't honor and hearing nothing. 2, terminals "terminals": { "type": "array", "items": { "oneOf": [
{ "type": "array", "items": { "type":"object", "required":["command"] } },
{ "type": "object", "required": ["command"] } ] } }The But you found a real gap next to it: I accepted the array branch without checking inside, so If you're reading the schema differently, point me at the path — I'm working from Verified: 37 binaries green, eval 61/61, fmt + clippy |
|
Claude finished @avifenesh's task in 4m 38s —— View job Review: follow-up push
|
Closes #1288 — the six A-tier sources that had never been audited, and whose baselines were deliberately left stale in #1287 for exactly that reason.
Nine false positives, each reproduced against the doc's own example before fixing and re-verified after.
Copilot
COP-005 rejected the documented value and autofixed it wrong.
agnix listed
coding-agent, which was never upstream. So the doc's own value errored — and worse, the unsafe autofix rewrote a correct value into the invalid one viafind_closest_value.56 occurrences across 22 files. Fixing only the const would have left every user-facing message wrong, so this covers the validator, all 12 locale files, both
rules.jsoncopies, and the knowledge-base docs.coding-agentis retained as a deprecated alias — users who followed agnix's own wrong advice shouldn't break — ordered last so the fixer prefers the documented spelling.COP-003 rejected comma-separated
applyTo.The whole string went to
glob::Pattern. The notable part: a correct brace/bracket-aware splitter already existed with 18 passing tests, marked#[allow(dead_code)] // reserved for future useand never called. The fix was wiring, not new logic.Cursor
CUR-004 — same comma bug, same upstream wording ("Separate multiple patterns with commas"). Worth calling out why it survived: it was intermittently wrong. The doc's literal row
docs/**/*.md, docs/**/*.mdxhappens to parse as one pattern, whilesrc/**, tests/**does not — so a fixture built from the doc's own example would have passed.CUR-010 required
version— documented as| version | number | 1 | Config schema version |, and several of the doc's examples omit it. Now optional, and typed number rather than integer, so1.0is valid.CUR-012 required
commandon prompt hooks. The doc's prompt example has onlytype,prompt,timeout— "Prompt hooks use an LLM to evaluate a natural language condition". CUR-018 already checkedpromptfor those.CUR-016 required
install. I fetched the published schema rather than trusting the page:So: nothing is required, the setup page's snapshot example legitimately omits
install, terminal entries need onlycommand(bothoneOfbranches), andupdateis not in the schema — which setsunevaluatedProperties: false, making it invalid. It's now reported as renamed toinstallrather than silently accepted.Timeout units — two suggestions said milliseconds; the doc says "Execution timeout in seconds".
Cross-tool
CC-MEM-014 fired on Cursor
.mdcfiles, reporting them asCLAUDE.md has N non-empty lines, exceeding the recommended 200 line limit— wrong filename, wrong threshold (Cursor documents 500), wrong tool attribution.ClaudeMdValidatoris registered forFileType::CursorRulefor its generic prose checks; the Claude-specific line rule is now gated on the file actually being a Claude memory file.18 rules had redirecting evidence URLs — Copilot 301s to
/how-tos/provide-context, Cline 308s to/customization/cline-rules. Both verified withcurland refreshed inrules.jsonandspec-baselines.json, so those two sentinel sources can match again.Where the two audits disagreed
One auditor reported comma-globs as a five-tool class including Cline and Claude rules. I checked each upstream doc separately rather than applying it as a blanket change:
globsapplyTopathspathsSplitting on commas for Cline or Claude would have been wrong. The docs settled it per tool.
Deliberately not done
User-global paths (
~/.copilot/,~/Documents/Cline/Rules,~/.agents/AGENTS.md) are outside the project tree agnix walks. Adding rules there is a scope decision, not a drift fix — worth a separate issue if wanted.Baseline hashes stay stale until this merges; re-cutting them is a separate reviewable change, same as #1287.
Verification
5060+ tests / 37 binaries green · eval 61/61 · self-lint clean ·
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean · bookkeeping, locale sync and rule counts in sync at 442 ·actionlint+shellcheckclean.Every fix reproduced before and re-verified after; new tests assert both directions (documented value accepted and the genuinely invalid one still rejected).