Skip to content

fix(rewrite): preserve quoted assignment data - #3462

Open
zhiwuyazhe-fjr wants to merge 3 commits into
rtk-ai:developfrom
zhiwuyazhe-fjr:codex/fix-3262-quoted-literal-rewrite
Open

zhiwuyazhe-fjr wants to merge 3 commits into
rtk-ai:developfrom
zhiwuyazhe-fjr:codex/fix-3262-quoted-literal-rewrite

Conversation

@zhiwuyazhe-fjr

@zhiwuyazhe-fjr zhiwuyazhe-fjr commented Aug 7, 2026 •

Copy link
Copy Markdown

Summary

  • prevent the environment-prefix regex from backtracking into quoted assignment values and exposing literal data as a command
  • preserve valid shell values composed from quoted and unquoted fragments
  • add regressions for the original shellcheck corruption and the latest reported single-/double-quoted PR-title and prose cases
  • rebase the branch onto current develop

Root cause

The lexer kept quote boundaries and byte offsets correctly. The corruption happened later in ENV_PREFIX: when a complete quoted assignment did not satisfy the trailing-whitespace requirement at a compound-command boundary, the regex could fall back to the broad unquoted alternative and match only the start of the assignment. The remaining literal text then looked like a command and was rewritten.

The unquoted fragment now excludes quote characters, while the environment value accepts a sequence of complete quoted or unquoted fragments. This prevents partial reinterpretation without regressing valid forms such as FOO='bar baz'qux git status.

This is independent of #3202's case-terminator lexer fix.

Security behavior

The original #3262 payload now passes through unchanged. A supported command outside the quoted assignment is still rewritten:

D='# shellcheck | git status; $(whoami)' ; cargo test
  -> D='# shellcheck | git status; $(whoami)'; rtk cargo test

The latest reported shapes are covered too:

  • T="... make ..."; echo "len=${#T}" preserves the double-quoted value
  • T='please make a cake'; printf '%s' "$T" preserves the single-quoted value
  • the longer T=...; echo ...; [ ... ] && gh pr edit ... --title "$T" form preserves T while rewriting only the external gh command
  • a simple gh pr edit --title "... make ..." still takes the normal rewrite path and preserves its title bytes

Test plan

  • cargo fmt --all
  • cargo clippy --all-targets
  • cargo test --all: 2583 passed, 0 failed, 8 ignored; all integration suites passed
  • targeted regressions for the four latest reported command shapes

Closes #3262

@bharvey42

Copy link
Copy Markdown

Carried in a local fork build since 2026-08-08 (Windows 11, daily use), no regressions.

Still merges cleanly with develop as of today (aa40853) — including after #3749 landed a few hours ago and reworked exclude_commands matching in this same file (src/discover/registry.rs, ~230 lines changed). git merge-tree reports no conflict against that, but given the overlap it may be worth a second look before merge.

@zhiwuyazhe-fjr
zhiwuyazhe-fjr force-pushed the codex/fix-3262-quoted-literal-rewrite branch from b8bd614 to a3cdf3a Compare September 1, 2026 09:17
@zhiwuyazhe-fjr

Copy link
Copy Markdown
Author

Thanks @bharvey42 for the report and for carrying this in a daily-use fork build since 2026-08-08 — glad to hear no regressions.

Re: the overlap with #3749 — rebased onto develop aa40853 (which includes it) and took that second look:

Verification on Windows after the rebase:

Ready for review.

@zhiwuyazhe-fjr
zhiwuyazhe-fjr force-pushed the codex/fix-3262-quoted-literal-rewrite branch from a3cdf3a to 234dacc Compare September 13, 2026 09:07
@zhiwuyazhe-fjr

Copy link
Copy Markdown
Author

Fresh rebase onto the current develop tip (0f8b26c) pushed — still conflict-free, including against the ~365 commits that landed since the last check (the arg_tokenizer work touches src/discover/registry.rs heavily, and this rebase stayed clean).

Verification on Windows after the rebase:

  • cargo fmt --all clean, cargo clippy --all-targets zero warnings
  • Full suite: 3532 passed, 0 failed (8 ignored) — the quoted-literal tests from this PR and the wrapper-form exclude_commands tests from fix(hooks): match exclude_commands against the peeled command form #3749 all pass together
  • Only failure is find_cmd::run_from_args_propagates_find_exit_status, which fails identically on a clean develop checkout under Windows (find.exe vs GNU find exit semantics) — pre-existing and unrelated

One heads-up for reviewers: workflow runs from this fork are approval-gated, so the CI run for this push is sitting in action_required — a maintainer approval from the Actions tab is needed for the checks to actually execute.

@bharvey42

Copy link
Copy Markdown

Re-verified today, since a lot has landed since my last note:

  • bug(hook): rewrite edits inside quoted string literals — 'shellcheck' in a single-quoted sed replacement became 'rtk shellcheck' (data corruption) #3262 still reproduces on stable v0.50.0 and on develop 3a12dfe. Through rtk hook claude, D='# shellcheck disable=SC2034 # c'; for f in h/*.sh; do sed -i -e "s|^X=|${D}\nX=|" "$f"; done comes back as D='# rtk shellcheck disable=…'.
  • With this PR's three commits: that payload passes through byte-identical, and a real command after a quoted assignment (export M='cargo test'; git status, V='ls -la' git status) is still rewritten.
  • Develop 3a12dfe + this PR on Windows (GNU toolchain): clippy clean; full suite 3942 passed / 2 failed. Both failures are pre-existing and Windows-only (find_cmd exit status, stream invalid UTF-8), in modules this PR doesn't touch.
  • Applied to the v0.50.0 tag: also clean. Hook output is identical to the official v0.50.0 binary on 34 everyday command shapes (env prefixes, pipes, loops, $( )).

CI has never run on this PR; it is still waiting on "Approve and run workflows". This is the silent kind of failure: exit 0 and wrong bytes on disk. Could a maintainer approve the run?

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.

bug(hook): rewrite edits inside quoted string literals — 'shellcheck' in a single-quoted sed replacement became 'rtk shellcheck' (data corruption)

2 participants