Skip to content

fix(config): [limits] section is ignored, and a partial section silently resets the whole config - #3368

Closed
seattled23 wants to merge 8 commits into
rtk-ai:masterfrom
seattled23:fix/limits-config-ignored
Closed

seattled23 wants to merge 8 commits into
rtk-ai:masterfrom
seattled23:fix/limits-config-ignored

Conversation

@seattled23

Copy link
Copy Markdown

Summary

Two related defects mean the [limits] config section does not work, and setting it partially breaks the rest of your config too.

1. grep_max_results is dead config

It is declared, defaulted, and printed by rtk config — but never read anywhere in the codebase:

$ rg 'grep_max_results' src/
src/core/config.rs:125:    pub grep_max_results: usize,
src/core/config.rs:139:            grep_max_results: 200,

(grep_max_per_file is read, in search.rs, so the section looks like it works.)

The cap actually applied comes from clap's hardcoded default on rtk grep:

#[arg(short, long, default_value = "200")]
max: usize,

So setting the documented key has no effect. Commands::Rg hardcoded 200 the same way.

Before:

$ printf '[limits]\ngrep_max_results = 5000\ngrep_max_per_file = 500\n' > ~/.config/rtk/config.toml
$ rtk grep -n 'the' ./skills | wc -l
202                      # still capped at 200
$ rtk grep -n 'the' ./skills -m 5000 | wc -l
4869                     # only the CLI flag works

After — max becomes Option<usize> and falls back to the config value, so precedence is CLI > config > default:

$ # grep_max_results = 3   -> 3 results
$ # grep_max_results = 400 -> all 182 results
$ # grep_max_results = 3, with --max 50 -> 50 results   (CLI still wins)

2. A partial [limits] section breaks the whole config

LimitsConfig has no serde defaults, so every one of its five fields is mandatory. A config that sets only one limit fails to deserialize — and because that fails the entire Config::load(), every other section silently reverts to defaults, including [hooks] transparent_prefixes and exclude_commands.

$ printf '[limits]\ngrep_max_results = 5000\n' > ~/.config/rtk/config.toml
$ rtk config
rtk: TOML parse error at line 1, column 1
     missing field `status_max_files`

That is a confusing failure on its own, but the dangerous part is the silent blast radius onto unrelated sections.

#[serde(default)] on the struct fixes it, and matches what TelemetryConfig, HooksConfig, and every field of Config already do.

Changes

  • src/core/config.rs — #[serde(default)] on LimitsConfig
  • src/main.rs — rtk grep --max is now Option<usize>, falling back to limits.grep_max_results; Commands::Rg uses the config value instead of a hardcoded 200
  • two regression tests: a partial [limits] parses and keeps other fields' defaults, and it no longer clobbers [hooks]

Verification

cargo fmt --all -- --check clean, cargo clippy --all-targets no warnings.

cargo test --bin rtk → 2526 passed.

Two pre-existing failures in hooks::rewrite_cmd::tests::unattestable_passthrough (test_plain_command_still_rewrites, test_fd_dup_redirect_still_rewrites) reproduce identically on pristine master with my changes stashed. They read the real user config, so they pass or fail depending on whether ~/.config/rtk/config.toml exists on the machine running them — worth isolating separately, but out of scope here.

Why this mattered

Found while debugging what looked like rtk grep silently dropping matches. It does not drop them — but with grep_max_per_file = 25 unfixable via config, a file with 30 matches shows 25, and the total in the header stays honest, so nothing looks wrong. For an LLM agent consuming the output, a truncated search reads as "no more matches" and becomes a wrong conclusion rather than a smaller one.

KuSh and others added 8 commits July 27, 2026 13:59
…AskRewrite

handle_vscode (rtk hook copilot's PascalCase path, shared by VS Code Copilot
Chat and Copilot CLI's Claude-compat entry) always set permissionDecision to
"allow" or "ask", diverging from process_claude_payload which only asserts
"allow" for an explicit user-configured Allow rule and stays silent otherwise.
Asserting "ask" is what caused #3037: Copilot CLI 1.0.66+ treats it as
authoritative and forces a blocking dialog with no "remember" option on every
rewritten command.

84aa4d6/0df6929 patched the wrong path (the camelCase native handler, which
never had this problem) with an auto-allow heuristic gated on an `explicit`
flag, instead of fixing handle_vscode itself. Replace both with the same rule
Claude's own hook and Droid's hook already use: never assert a decision for
AskRewrite, regardless of whether the verdict was Default or an explicit Ask
rule. This removes the Copilot-specific heuristic (and the now-unused
`explicit` field) in favor of one behavior shared across all hosts.

Also extracts vscode_response_from_decision as a pure, testable function
(mirroring copilot_cli_response_from_decision), closing a prior gap where
handle_vscode's decision output had no direct unit test coverage.
…ok config

rtk init --copilot registered both a PascalCase PreToolUse entry and a
camelCase preToolUse entry in the same rtk-rewrite.json, on the assumption
that VS Code Copilot Chat needs the former and Copilot CLI needs the latter.

Live testing showed Copilot CLI treats PreToolUse/preToolUse as two
independent, sequentially-run hooks — a redundant second `rtk hook copilot`
process spawn per tool call, chaining the first hook's rewrite into the
second's input (confirmed via raw stdin capture, and confirmed independent
of declaration order in the file). Also confirmed Copilot CLI honors the
PascalCase-only schema perfectly well on its own, receiving the same
tool_name/tool_input.command shape either way — so the camelCase entry buys
nothing for Copilot CLI, while adding process overhead and an extra,
harder-to-reason-about execution path.

Drop the camelCase preToolUse entry, keeping the single PascalCase
PreToolUse entry shared by both hosts. Existing installs are not upgraded
automatically — re-running `rtk init --copilot` / `rtk init --global
--copilot` overwrites the old dual-schema file with the new one
(write_if_changed overwrites unconditionally on content diff), verified
by test_copilot_init_upgrades_old_dual_schema_install and
test_copilot_global_install_upgrades_old_dual_schema_install, which seed
the old dual-schema content and assert it gets replaced.
detect_format only matched tool_name values "runTerminalCommand", "Bash",
and "bash" for the VsCode hook format. Live payload capture from a real VS
Code Copilot Chat session (agent mode, GitHub.copilot-chat) showed it
actually sends "run_in_terminal" — none of the previously recognized
values — so detect_format fell through to PassThrough and the hook never
fired at all for VS Code Copilot Chat: no rewrite, no permissionDecision,
nothing.

Add "run_in_terminal" to the recognized tool_name set. Verified against the
real captured payload end-to-end: a compound "cd <dir> && git status
--short --branch" command now correctly rewrites only the git segment to
"rtk git status --short --branch", leaving "cd" untouched, with no
permissionDecision asserted (consistent with the Default-verdict behavior
fixed in 042aeaf).
…ot generated

The doc comments described the camelCase toolName/toolArgs schema as "GitHub
Copilot CLI"'s format, which was accurate when rtk init --copilot registered
it. Now that only the PascalCase schema is generated, this path is reachable
only via not-yet-upgraded installs' leftover registration, or hosts that use
this schema under a different toolName value on their own (JetBrains/
IntelliJ's Copilot plugin sends "run_in_terminal", tracked in #2443/#3093).
Note both call sites accordingly so the code isn't mistaken for dead weight.
…owershell

Contributor testing on Windows 11 with Copilot CLI 1.0.73 (#3179) confirmed
Copilot CLI remaps its native bash/powershell shell tool to tool_name: "Bash"
under the PascalCase PreToolUse schema, and that its updatedInput is honored
end-to-end there. Since rtk init --copilot now only registers that schema,
Windows already works standalone through it — the camelCase toolName
"powershell" case (#3178/#3179) becomes legacy-only, relevant solely to
un-upgraded installs. Document that on both HookFormat variants.

Also fixes a HookDecision::AskRewrite struct-pattern leftover in
copilot_ide_response_from_decision (added by #3093, merged into develop
after this branch's own struct-to-tuple AskRewrite refactor), surfaced as a
compile error by rebasing onto develop to pick up #3179.
…on-decision

fix(hooks): stop Copilot from silently deciding permission on unconfigured commands
…ons broke the whole config

Two related defects in LimitsConfig.

1. grep_max_results was dead config. It is declared, defaulted and printed
   by `rtk config`, but never read anywhere. The cap actually applied comes
   from clap's hardcoded `default_value = "200"` on `rtk grep --max`, so
   setting the documented config key had no effect at all. Commands::Rg
   hardcoded 200 the same way. Both now fall back to the config value, while
   an explicitly passed --max still wins (CLI > config > default).

2. LimitsConfig had no serde defaults, so every field was mandatory. A
   config setting only one limit failed to deserialize, which failed the
   whole Config::load() and silently reverted EVERY other section --
   including [hooks] transparent_prefixes -- back to defaults. Adding
   #[serde(default)] matches how TelemetryConfig and HooksConfig already
   behave.

Verified against a debug build:
  grep_max_results=3   -> 3 results
  grep_max_results=400 -> all 182 results
  grep_max_results=3 with --max 50 -> 50 results (CLI still wins)
  partial [limits] now parses; on 0.42.0 the same file gives
  'TOML parse error at line 1, column 1'

Adds two regression tests. Full suite: 2526 pass. The two
hooks::rewrite_cmd::unattestable_passthrough failures reproduce on
pristine master -- they read the real user config, so they depend on
whether ~/.config/rtk/config.toml exists.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ KuSh
❌ seattled23
You have signed the CLA already but the status is still pending? Let us recheck it.

@rtk-release-bot

Copy link
Copy Markdown
Contributor

Automatic message from CI checks : It seems like this branch is targeting the wrong branch, any contribution should target develop branch.

See CONTRIBUTING.md for details.

@rtk-release-bot rtk-release-bot Bot added the wrong-base PR targets master instead of develop label Aug 3, 2026
@HetCreep

HetCreep commented Aug 4, 2026 •

Copy link
Copy Markdown

This PR matches what an independent audit of v0.44.2 (700bdde) found, including the #[serde(default)] root cause — the diagnosis in the PR body is correct and the regression test is the right shape. Three things that look like they fall just outside its scope.

Provenance: AI-assisted source audit, human-directed. The deserialization behaviour below was reproduced by building a replica of these exact struct attributes against serde 1 + toml 0.8; the grep counts are from 700bdde.

1. Three sibling structs have the same defect, and the PR only fixes LimitsConfig

At 700bdde, four config structs derive Deserialize with neither Default nor #[serde(default)]:

struct line has Default? #[serde(default)]?
TrackingConfig config.rs:56-57 no no
DisplayConfig config.rs:74-75 no no
FilterConfig config.rs:91-92 no no
LimitsConfig config.rs:122-123 no fixed by this PR

(Config, HooksConfig and TelemetryConfig all derive Default, which is why they behave.)

So the failure mode this PR describes — "a config that sets only one field fails to deserialize, which fails the WHOLE Config::load()" — still applies to [tracking], [display] and [filters] after it lands. Reproduced on the replica:

[tracking]
enabled = false          -> TOML parse error
[display]
colors = false           -> TOML parse error

Adding #[serde(default)] to those three as well would make the fix uniform, and the existing regression test generalises to them directly.

2. Six more keys are still declared, serialized, and never read

grep_max_results was the one this PR wires up. Grepping src/ at 700bdde, excluding config.rs itself, these return zero hits:

  • tracking.history_days
  • display.max_width
  • filters.ignore_dirs
  • filters.ignore_files
  • limits.status_max_files
  • limits.status_max_untracked

Since Config::save() and rtk config serialize every field, the written config file advertises these as working settings. (I checked colors, emoji and enabled too — those do appear elsewhere in src/, so I am not claiming them.) Also worth a look: src/cmds/system/README.md:8 states that search.rs reads grep_max_results, which is what this PR is making true — the doc was ahead of the code.

3. An unknown key is still accepted silently

The inverse of the partial-section problem, and untouched by this PR:

Correction (2026-08-06): the example I originally gave here was wrong, and I have replaced it. I wrote that histroy_days = 7 "parses fine, silently ignored". It does not — re-running the replica, a lone misspelled key yields Err: missing field history_days, because the correctly-spelled field is then absent and there is no serde(default) to supply it. My claim that a misspelled key produces silence was false for the current code. What is actually true is narrower, and points the other way:

no serde(default) -- the shape shipping today
  all fields present + unknown key   -> Ok   (unknown key silently ignored)
  partial section + unknown key      -> Err  missing field history_days
  misspelled key only                -> Err  missing field history_days

WITH serde(default) -- what #1483 / #1544 would make them
  all fields present + unknown key   -> Ok   (unknown key silently ignored)
  misspelled key only                -> Ok   history_days silently = 0

So unknown keys are ignored today only when every declared field is present. The part worth acting on is the second block: once serde(default) lands, a misspelled key stops erroring and silently reverts the real setting to its default — a silent-misconfiguration failure mode that does not exist today. No struct carries deny_unknown_fields; adding it alongside serde(default) would close that regression before it ships, and a warning on unrecognised keys would get most of the benefit if outright rejection is too strict for forward compatibility.

None of this is a blocker for the PR as written; it is scoped and correct. Posting so the remaining three structs and the six keys are on the record somewhere, since they will look fixed once [limits] starts working.

@HetCreep

HetCreep commented Aug 6, 2026

Copy link
Copy Markdown

For the record, since this closed without a technical objection: the diagnosis here is still correct at develop HEAD today.

src/core/config.rs
  TrackingConfig  :57   #[derive(Debug, Serialize, Deserialize)]  — no Default, no serde(default)
  DisplayConfig   :75   same
  FilterConfig    :92   same
  LimitsConfig   :123   same

grep_max_results still appears only in src/core/config.rs and two READMEs — no code reads it — so the [limits] behaviour this PR described is unchanged.

The blockers looked procedural rather than technical: the CI bot flagged on day one that this targets master when contributions go to develop, and the CLA shows 1 of 2 committers signed. If you do pick it back up, worth coordinating with #1483, which already targets develop and adds struct-level #[serde(default)] to all five sub-configs — it has been open since April and covers the partial-section half of this. #1482 is the tracking issue for that behaviour.

Provenance: AI-assisted source audit, human-directed. The line numbers above were read from develop HEAD via the API today; the deserialization behaviour was previously reproduced on a replica of these struct attributes, not on an rtk build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wrong-base PR targets master instead of develop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants