Skip to content

fix(find): never-worse guard, recovery hint, and dispatch on find's grammar - #3603

Merged
aeppling merged 14 commits into
developfrom
fix/find-never-worse
Aug 26, 2026
Merged

aeppling merged 14 commits into
developfrom
fix/find-never-worse

Conversation

@aeppling

@aeppling aeppling commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Started as the benchmark negative on find --max 10; grew into making rtk find correct on every input shape, per review.

rtk find now parses arguments like find: options, then paths, then the expression. So find src lists src instead of matching a file named src. rtk walks only the subset it models (-name/-iname globs, -type f|d, -maxdepth, each once). Any other listing predicate (-path, -mtime, -not, -o, -type l, brackets, -mindepth, -L ...) runs the real find and its results go through the same compressed renderer. Actions (-exec, -delete, -print0, -ok ...) run through runner::run_passthrough: live output, stdin inherited, exit code kept.

rtk's own -m/-t are read only as trailing tokens and apply to both native and compressed runs; combined with an action they are refused rather than dropped. Legacy 'find ...' is rewritten to -name form and dispatched, never errors. The tee hint goes through runner::emit_guarded; default cap is CAP_INVENTORY; nonexistent path exits 1; no panic on long non-ASCII dir names.

Not changed, for the maintainer to decide: the native walker still skips hidden and gitignored entries and defaults to files (documented rtk behavior), and prints paths relative to the search root.

Verified: ~8k fuzzed invocations per iteration against GNU find and the pre-PR binary, 2713 tests, benchmark find cases all positive. The remaining CI red is the golangci-lint benchmark case, unrelated to this PR.

The guard compared the grouped summary to the full match list, so a
--max-capped run could emit more tokens than plain paths (benchmark
'find --max 10' negative case). Baseline is now the plain listing
truncated at max_results with a +N more marker.
Per src/cmds/README.md truncation-recovery rule: when the cap comes
from the default (agent never asked to truncate), tee the full flat
listing and emit force_tee_tail_hint so hidden items are recoverable
without re-running. An explicit -m/--max keeps the bare +N more
marker: the agent asked for less.
@aeppling aeppling changed the title fix(find): apply never-worse guard against the capped listing fix(find): apply never-worse guard against the capped listing + tee tail hint Aug 19, 2026
The hook rewrites any find command, but rtk find bailed on compound
predicates and actions (-not, -exec, ...) with advice to 'use find
directly' that the hook makes impossible to follow: the retry gets
rewritten again. Now such commands execute the real find from PATH
with the original args, output and exit code untouched, tracked at
0% savings.
@aeppling
aeppling marked this pull request as ready for review August 19, 2026 12:06
@aeppling aeppling mentioned this pull request Aug 20, 2026
aeppling and others added 4 commits August 25, 2026 19:11
track() with stdout as both input and output recorded real token
counts at 0% savings, diluting the global gain percentage for
commands rtk never tried to filter. track_passthrough records 0/0,
neutral by design.
Flat path sort and display dir sort diverge ('logs-old/f' < 'logs/f'
as paths, 'logs' < 'logs-old' as dirs), so tail on the flat-sorted
tee could return files already shown while the hidden ones stayed
buried. One display-ordered list now feeds the summary, the capped
listing, and the tee.
The passthrough decoded find's output through the lossy text decoder,
so non-UTF-8 filenames came out with U+FFFD, including under -print0
where the bytes feed xargs -0. Write the captured stdout/stderr bytes
verbatim instead; exit code and signal diagnostic unchanged.
@pszymkowiak

Copy link
Copy Markdown
Collaborator

Tested the current head (8942e75) — the 3 previous fixes (tee-hint ordering, tracking dilution, stdin) all hold up. But the underlying dispatch logic (native vs RTK syntax vs unsupported passthrough) still has real bugs. All repro'd locally:

1. Crash on non-ASCII directory names (find_cmd.rs:388)

mkdir -p 'a😀aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
rtk find . -name "*.rs"
thread 'main' panicked at src/cmds/system/find_cmd.rs:388:34:
start byte index 4 is not a char boundary; it is inside '😀' (bytes 1..5 of string)

&dir[dir.len()-47..] slices at a fixed byte offset with no char-boundary check. Any dir name >50 bytes with a multi-byte char near that offset crashes the whole process instead of falling back to raw output.

2. Native flags outside the allowlist get silently misparsed as an RTK query

rtk find -mindepth 2

→ prints nothing, exit 0, no error. -mindepth isn't in UNSUPPORTED_FIND_FLAGS and isn't in the native-flag allowlist (-name/-type/-maxdepth/-iname), so it falls through to parse_rtk_find_args, which treats "-mindepth" as the glob pattern and "2" as the path. Silent wrong result, not an error.

3. Same root cause, but silently widens results instead of narrowing

find . -path "*/target/*" -name "*.rs"   # real find: only matches under target/
rtk find . -path "*/target/*" -name "*.rs"   # rtk: -path dropped, returns every *.rs file

-path/-user/-prune/-samefile etc. hit the catch-all unknown flag, ignored branch — the constraint silently disappears instead of narrowing or triggering passthrough.

4. -print0/-exec passthrough corrupts non-UTF8 bytes

printf 'binaire:\x00\x01\xff\xfe:fin\n' > bin.txt
rtk find . -name bin.txt -exec cat {} \;

Raw bytes ff fe come back as ef bf bd ef bf bd (double U+FFFD) — exec_capture_stdin decodes via from_utf8_lossy. Defeats exactly the byte-safety guarantee -print0 exists for.

5. Exit code depends on which path was taken

rtk find /does/not/exist -mtime -1   # passthrough → exit 1 (correct)
rtk find "*.rs" /does/not/exist      # normal path → exit 0 (wrong — main.rs hardcodes 0)

6. RTK flags + unsupported native flag together → forwarded verbatim, real find errors

rtk find "*.rs" . -m 5 -mtime +7
find: -m: unknown primary or operator

-m is RTK-only syntax; once -mtime triggers passthrough, the whole arg list (including -m 5) goes straight to the real find binary, which doesn't understand it.

Root cause across 1/2/3/6 feels like the same thing: UNSUPPORTED_FIND_FLAGS is a denylist of known-bad flags, so anything else defaults to "treat as RTK syntax" — the wrong default. Given how large real find's flag surface is, an allowlist of what RTK actually knows how to parse (falling through to passthrough for everything else) would close this whole class at once instead of adding flags to the denylist one at a time.

Also still open from earlier: -ok/-okdir missing from UNSUPPORTED_FIND_FLAGS (same silent-ignore behavior as #3 above, but for the interactive-confirm actions).

…deled predicates

Paths are the tokens before the first expression token, as in find, so
'find src' lists src instead of matching a file named src. rtk walks
only the subset it models (-name/-iname globs, -type f|d, -maxdepth);
any other listing predicate runs the real find and its results go
through the same compressed renderer; actions (-exec, -delete,
-print*, ...) stay verbatim. Also: nonexistent path exits 1, long
unicode dir labels no longer panic, rtk -m/-t mixed with actions is a
clear error.

Known gaps from fuzzing, to fix before merge: leading -H/-L/-P/-O/-D
options, rtk flag scan reaching into -exec args, repeated subset
predicates, -t precedence under -o, root '.' entry rendered empty.
…ions forwarded

Leading -H/-L/-P/-D/-O options are recognized and forwarded ahead of
the paths. rtk's -m/-t are interpreted only when the whole expression
is the modeled subset, each predicate at most once; everything else
reaches find untouched, so -exec arguments and predicate values are
never misread and -t can no longer change precedence under -o. The
root '.' entry keeps its name in compressed output.
@aeppling aeppling changed the title fix(find): apply never-worse guard against the capped listing + tee tail hint fix(find): never-worse guard, recovery hint, and dispatch on find's grammar Aug 26, 2026
Explicit remove_dir_all trips the filesystem-deletion semgrep rule;
TempDir cleans up on drop like the other test modules.
…ck on legacy syntax

Verbatim runs go through runner::run_passthrough (inherited stdio,
live output). The tee hint is emitted through runner::emit_guarded so
body plus hint never exceed the plain listing. rtk's -m/-t are read
only as trailing tokens and apply to native and compressed runs
(( expr ) -type T keeps precedence); combined with an action they are
refused rather than dropped. Legacy 'find <pattern> ...' is rewritten
to -name form and dispatched. Default cap is CAP_INVENTORY.
…efore actions; paths like find

The never-worse baseline now carries the tee hint too, so a truncated
listing always keeps its recovery path. Trailing -m/-t in front of an
action are forwarded untouched and find rejects them itself, no rtk
error string. The native walker prints paths from the search root as
find does. File formatted with rustfmt directly: automod hides
src/cmds from cargo fmt.
Truncating labels over 50 chars to ...tail left grouped output
unresolvable whenever the search root was absolute; a label is printed
once per directory, so the saving was negligible.
Restores the pre-PR output shape: paths relative to the search root
and long directory labels shortened, both cheaper in tokens. A bare
positional is a path only when it is an existing directory, so
'find src' lists src while 'rtk find Cargo.toml' still searches by
name as it always did.
@pszymkowiak

Copy link
Copy Markdown
Collaborator

Re-tested at current head (988f2e3) — all 6 issues from the previous comment are fixed, verified locally:

  1. Crash on non-ASCII dirs — directory labels now print in full, no more fixed-offset truncation. No panic.
  2. -mindepth silently misparsed — now passthroughs to real find, which gives its own clear error instead of a silent empty result.
  3. -path silently dropped — rtk find . -path "*/target/*" -name "*.rs" now returns exactly what real find returns.
  4. -print0/-exec byte corruption — raw non-UTF8 bytes (ff fe) now come through untouched, no more lossy decode.
  5. Inconsistent exit codes — both the passthrough and normal paths now exit 1 on a bad path (previously the normal path silently exited 0).
  6. Mixed RTK+native flags — now surfaces real find's own error message instead of silently misbehaving. Not the smoothest UX, but honest and correct.

Also -ok/-okdir (flagged earlier, before this comment) now correctly triggers passthrough — the real confirmation prompt shows up.

Nice work tracking all of these down. LGTM on the correctness front — the remaining points from earlier (dead bail! branch in parse_find_args, group_by_dir computed twice, hand-rolled passthrough instead of run_passthrough) are just cleanup, not blockers.

@pszymkowiak pszymkowiak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All correctness issues found in review are fixed and re-verified locally (crash on non-ASCII dirs, -mindepth/-path silent misparsing, -print0/-exec byte corruption, inconsistent exit codes, -ok/-okdir passthrough). LGTM.

@aeppling
aeppling merged commit 203948b into develop Aug 26, 2026
1 check passed
mariuszs added a commit to mariuszs/rtk-java that referenced this pull request Aug 26, 2026
Conflict resolutions:

- src/core/stream.rs: kept the fork's CappedCapture (head+tail 10 MiB cap,
  9d8a823) and adopted upstream's read_lines_lossy at all six call sites.
  The two are orthogonal: upstream fixes lines().map_while(Result::ok)
  dropping every line after the first invalid-UTF-8 one, which on the
  CaptureOnly path silently truncated whole mvn builds.
- src/cmds/system/find_cmd.rs: took upstream wholesale. Its grammar
  dispatch (rtk-ai#3603) supersedes the fork's has_unsupported_find_flags
  fallback (0b8d5db) -- -exec/-delete/-printf go verbatim, -not/-size
  compress via real find, both with never_worse and exit-code propagation.
- src/main.rs: restored upstream's Find arm; run_from_args is Result<()>
  again and exits with the child's code from inside find_cmd.
- Cargo.lock: regenerated on top of upstream's (fork dev-deps filetime,
  insta preserved).

Gate: fmt clean, clippy --all-targets clean, cargo test --all 2896 passed.
Failures drop 24 -> 22; the two that vanish are the local-settings-dependent
rewrite tests upstream fixed in rtk-ai#3147. No new failures.
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
rtk 0.46.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>## [0.46.0](rtk-ai/rtk@v0.45.0...v0.46.0) (2026-08-26)


### Features

- find: dispatch on find's grammar; compress find output for unmodeled predicates ([#3603](rtk-ai/rtk#3603))
- find: tee tail hint when rtk imposes the result cap ([#3603](rtk-ai/rtk#3603))

### Bug Fixes

- find: never-worse guard, recovery hint, and dispatch on find's grammar ([#3603](rtk-ai/rtk#3603))
- git: don't misdetect a value-taking option's argument as a patch flag ([#3575](rtk-ai/rtk#3575))
- cicd: stop benchmark.sh deleting the tracked scripts/benchmark harness ([#3595](rtk-ai/rtk#3595))
- tee: hash long recovery-file slugs to prevent collisions and shorten hints ([#3266](rtk-ai/rtk#3266))
- benchmark: avoid negative curl/cargo cases that fail the benchmark job ([#3430](rtk-ai/rtk#3430))
- test: accept both Ask and Allow verdicts in rewrite tests ([#3147](rtk-ai/rtk#3147)) — Closes [#3146](rtk-ai/rtk#3146)
- core: decode process output using Windows console code page ([#2717](rtk-ai/rtk#2717)) — Closes [#2452](rtk-ai/rtk#2452)
- git: preserve patch output from log commands ([#2951](rtk-ai/rtk#2951)) — Closes [#2944](rtk-ai/rtk#2944)
- discover: sanitize drive-letter colon so Windows discover finds sessions ([#2952](rtk-ai/rtk#2952)) — Closes [#2919](rtk-ai/rtk#2919)
- stream: decode lossily instead of dropping lines on invalid UTF-8 ([#2997](rtk-ai/rtk#2997)) — Closes [#2994](rtk-ai/rtk#2994)

### Other

- test(find): use the platform temp dir instead of /tmp ([#3717](https://github.com/rtk-ai/rtk/pull/3717))</pre>
  <p>View the full release notes at <a href="https://github.com/rtk-ai/rtk/releases/tag/v0.46.0">https://github.com/rtk-ai/rtk/releases/tag/v0.46.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!17826
tianjianjiang added a commit to tianjianjiang/smith that referenced this pull request Sep 24, 2026
…nd later

rtk 0.46.0 forwards -H/-L/-P to native find (rtk-ai/rtk#3603), so the
guard now reads `rtk --version` and advises only below 0.46.0 or when the
version cannot be parsed. Verified 2026-09-24 that rtk 0.49.0 returns the
symlinked file for `find -L`.

Assisted-by: Claude:claude-opus-5-5
tianjianjiang added a commit to tianjianjiang/smith that referenced this pull request Sep 24, 2026
* fix(smith-ctx-claude): silence rtk-find-symlink-guard on rtk 0.46.0 and later

rtk 0.46.0 forwards -H/-L/-P to native find (rtk-ai/rtk#3603), so the
guard now reads `rtk --version` and advises only below 0.46.0 or when the
version cannot be parsed. Verified 2026-09-24 that rtk 0.49.0 returns the
symlinked file for `find -L`.

Assisted-by: Claude:claude-opus-5-5

* fix(smith-ctx-claude): address review of rtk find guard version gate

Remove the contradictory "verified open" note on rtk-ai/rtk#2821 in
HOOKS.md, scope RTK_FAKE_VERSION to subshells in the tests, add a
major-version case, and fold the version check into one expression.

Assisted-by: Claude:claude-opus-5-5

* test(smith-ctx-claude): fail the run when a version-gate case fails

fail() inside a subshell only exits that subshell, so the five
version-gate cases could print FAIL and still report PASS with exit 0.
Propagate each subshell's status with `|| exit 1`.

Assisted-by: Claude:claude-opus-5-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants