Skip to content

fix(tokenless): anchor rtk prefix in rewrites - #1975

Merged
casparant merged 1 commit into
alibaba:mainfrom
casparant:fix/tokenless/rtk-absolute-rewrite
Aug 1, 2026
Merged

fix(tokenless): anchor rtk prefix in rewrites#1975
casparant merged 1 commit into
alibaba:mainfrom
casparant:fix/tokenless/rtk-absolute-rewrite

Conversation

@casparant

Copy link
Copy Markdown
Collaborator

Description

rtk emits rewritten commands with a bare rtk prefix, which only resolves when the shell executing the tool call has the rtk location on its PATH. Agent runtimes with a trimmed PATH — observed live in Qoder IDE tool sessions, where PATH lacks ~/.local/bin — fail every rewritten command with exit 127 (command not found: rtk), even though the hook itself resolves rtk successfully via the fallback layouts. The 0.7.3 cross-layout discovery covered the hook process, not the rewrite it hands back.

Fix: anchor the rewrite to the resolved absolute rtk_bin in rewrite_hook.py. A segment-start regex (command start and after && / || / ; / |) swaps the bare prefix for the shlex-quoted resolved path, preserving separator whitespace. Anchoring is unconditional — the absolute path is correct whether or not rtk was on PATH, so behavior in qodercli terminal sessions is unchanged (absolute path instead of bare name).

Related Issue

closes #1974

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Scope

  • tokenless (tokenless)

Checklist

  • I have read the Contributing Guide
  • My code follows the project's code style
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly (N/A — behavior-only fix; the documented rewrite behavior now also holds in trimmed-PATH environments)
  • For tokenless: cargo clippy -- -D warnings and cargo fmt --check pass (no Rust changes; Python hook + test only)
  • Lock files are up to date (package-lock.json / Cargo.lock) (unchanged)

Testing

New tests/test_rewrite_hook.py (wired into make test under test-integration):

  • Stages a fake rtk/tokenless pair in the fallback layout (~/.local/share/anolisa/tokenless/) under a sandboxed HOME, runs the hook with PATH=/usr/bin:/bin — the exact affected shape — and asserts every rewritten segment starts with the executable absolute rtk path, and that updatedInput matches tool_input.
  • 2/2 pass against the fixed hook.
  • Negative control: the same test against the pre-fix hook (from origin/main) fails with the production symptom (rtk grep --cached foo && rtk git status emitted unanchored).

Also verified: tests/test_compress_response_hook.py still passes, hook compiles clean (py_compile).

Additional Notes

Root-caused from a live Qoder IDE session: the hook rewrote git/grep tool calls to rtk git ... / rtk grep ..., all failing 127 because the IDE tool shell PATH lacks ~/.local/bin. With this fix the emitted rewrite is self-contained.

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

本次审查范围内未发现需要修改的问题。


🤖 Generated by QoderView workflow run

@kongche-jbw kongche-jbw 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.

Thanks for addressing the trimmed-PATH failure. I found two edge cases around locating the inserted rtk token that seem worth covering before merge; I’ve left details inline.

Comment thread src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py Outdated
Comment thread src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py Outdated
@Forrest-ly

Copy link
Copy Markdown
Collaborator

Code Review — commit dfb600d

Overall: the fix is correct, well-scoped, and well-tested. Verified locally at dfb600d: python3 tests/test_rewrite_hook.py passes 2/2, and test_compress_response_hook.py is unaffected. The approach — anchoring at segment starts with the resolved rtk_bin, shlex.quoted, applied to both tool_input and updatedInput — directly addresses the exit-127 failure in trimmed-PATH runtimes.

One non-blocking edge case worth noting, plus a minor observation:

1. _RTK_PREFIX_RE can match inside quoted arguments (rewrite_hook.py:50)

The regex (^|&&|\|\||[;|])(\s*)rtk(?=\s) is quote-unaware, so a connective character inside a quoted argument followed by rtk gets rewritten. Reproduced against this branch:

anchor("rtk grep -E 'foo|rtk bar' src/")
# => "/abs/rtk grep -E 'foo|/abs/rtk bar' src/"   <- pattern corrupted

anchor('rtk grep "install; rtk setup" docs/')
# => '/abs/rtk grep "install; /abs/rtk setup" docs/'

Since rtk primarily rewrites grep/git invocations, a user search pattern containing |rtk or ; rtk is plausible (regex alternation is the most likely shape). The result is a silently corrupted pattern rather than a hard failure.

This should not block the merge — the pre-existing behavior (every rewrite failing with 127) is strictly worse, and the trigger is narrow. A cheap hardening for a follow-up: require at least one whitespace between the connective and rtk (e.g. (^|(?:&&|\|\||[;|])(?=\s))(\s*)rtk(?=\s)), which rejects the tight |rtk shape while still matching everything rtk actually emits (a && rtk b, a | rtk b). It doesn't close the quoted "; rtk " case, but shrinks the realistic false-positive surface (regex alternations) to near zero. Fully closing it would need quote-aware scanning, which is likely over-engineering here.

2. shutil.which can return a relative path (informational)

resolve_binary (hook_utils.py:366) returns shutil.which(name) verbatim; with a relative entry on PATH this could be a relative path, and the anchored command would then depend on the tool-shell CWD. In practice PATH entries are absolute and, when which succeeds, the bare rtk would have resolved anyway — so no action needed, just noting the assumption.

Test coverage is good: sandboxed HOME, fallback-layout staging, the exact trimmed-PATH shape (PATH=/usr/bin:/bin), executability assertion on the anchored first word, and tool_input/updatedInput parity. Wiring into make test-integration is appreciated.

@Forrest-ly Forrest-ly 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.

Code review 完成(commit dfb600d):修复正确且测试充分,本地验证 tests/test_rewrite_hook.py 2/2 通过。有一个非阻塞建议 —— _RTK_PREFIX_RE 对引号内容不感知,引号内的连接符后跟 rtk 会被误改写(如 grep -E 'foo|rtk bar'),详见上方评论中的复现和加固建议。不阻塞合并。

Copy link
Copy Markdown
Collaborator

One additional coverage gap: this patch anchors the RTK prefix only in the shared common/hooks/rewrite_hook.py, while Hermes and OpenClaw have independent rewrite paths that remain unchanged.

  • Hermes resolves and invokes rtk_bin directly, but then returns proc.stdout.strip() unchanged in Re-execute as: {rewritten}.
  • OpenClaw resolves and invokes rtkPath directly, but returns result.stdout.trim() unchanged and later assigns it to event.params.command.

Under the same trimmed-PATH condition, RTK still emits a bare rtk ... command in both paths, so the subsequent agent/tool-shell execution can still fail to locate rtk. This is a pre-existing analogous risk rather than a regression introduced by this diff, but it remains outside the coverage of this PR if the intended fix is tokenless-wide.

@Forrest-ly Forrest-ly 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.

Code review 通过(commit dfb600d)。修复思路正确、实现克制、测试扎实。

已独立验证

  • tests/test_rewrite_hook.py 2/2 通过;test_compress_response_hook.py 不受影响;py_compile 干净。
  • 负向对照复现成功:将 rewrite_hook.py 回退到 origin/main 版本后,新测试如预期失败,报出的正是生产症状(rtk grep --cached foo && rtk git status 未锚定)。
  • 测试 staged 的 fallback 路径 ~/.local/share/anolisa/tokenless/hook_utils.py_RTK_LOCAL_SHARE 常量一致,且沙箱 HOME + PATH=/usr/bin:/bin 精确复现了受影响环境的形态。

实现要点确认

  • _anchor_rtk_prefix 无条件锚定的决策合理——rtk_bin 是 hook 已解析成功的绝对路径,无论 rtk 是否在 PATH 上都正确,qodercli 终端场景行为等价。
  • shlex.quote(rtk_bin) 正确处理了含空格/特殊字符的安装路径(已验证 /opt/my dir/rtk'/opt/my dir/rtk')。
  • 正则的 (?=\s) lookahead 保证只改带参数的 rtk 前缀,分隔符空白通过捕获组原样保留。

一个非阻塞的观察(不要求本 PR 修改):

_RTK_PREFIX_RE 是纯文本匹配,不感知 shell 引号。若 rtk 输出的 rewrite 中某个带引号参数内部恰好出现 && rtk (如 rtk git commit -m "bump rtk && rtk docs"),引号内的 rtk 也会被替换为绝对路径。由于输入域是 rtk 自身生成的 rewrite(前缀形态受控),实际风险很低;但如果未来 rtk rewrite 开始透传用户提供的字符串参数,值得在 rtk 侧输出结构化 segment 或在此处引入 shlex 级别的解析。建议作为已知限制记录即可。

@ikunkun-sys ikunkun-sys 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.

Requesting changes at dfb600d because two confirmed correctness issues remain unresolved:

  • Real RTK 0.43.0 output can place a bare rtk after wrappers, environment assignments, and a single &; _RTK_PREFIX_RE does not anchor those positions, so the trimmed-PATH exit-127 failure remains for those commands.
  • The regex is quote-unaware and rewrites user data such as the rtk text in grep -E 'foo|rtk bar', silently changing command semantics even when PATH already contains RTK.

Both behaviors were reproduced against this exact revision in the existing inline threads. In addition, the independent Hermes and OpenClaw rewrite paths still pass RTK's bare output to the subsequent execution environment, so they remain outside this PR's coverage.

@Forrest-ly Forrest-ly 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.

总体评价

本 PR 正确解决了"trimmed-PATH 环境中 rtk bare prefix 导致 exit-127"的核心问题,思路清晰、实现克制。但 ikunkun-sys 在 2026-07-30 提出的两个正确性问题经过独立验证是真实的,目前未被修复,阻塞合并。

审查结论

request changes


详细意见

🔴 必须修改(阻塞合并)

问题 1:_RTK_PREFIX_RE 未覆盖 wrapper 和环境变量赋值形式的 rtk 前缀

  • [src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py:50] — _RTK_PREFIX_RE 只在行首、&&||;| 后锚定 rtk。RTK 0.43.0 实际输出包含以下形式:

    • sudo rtk git status(wrapper 后)
    • RUST_BACKTRACE=1 rtk cargo test(环境变量赋值后)
    • git status & rtk grep foo(单 & 后)

    这些形式下 _anchor_rtk_prefix 不替换,trimmed-PATH 环境中仍然 exit-127,PR 要修复的问题仍然存在。ikunkun-sys 已在内联 thread 中通过 RTK 0.43.0 复现确认(comment id: 3670646132)。

    修复建议:正则需扩展覆盖:

    • &(非 &&):(?<!\&)\&(?!\&)
    • 环境变量赋值序列:(?:[A-Za-z_]\w*=[^\s]* +)+(可能需要多次 pass 或更完整的 tokenization)
    • Wrapper 命令(sudoenv 等):最简可行方案是改用 rtk rewrite --output-structured 获取结构化 segment 列表(若 RTK 0.43.0 支持),否则需要递归/迭代匹配或基于 shlex 的 token 级处理。

    若上述扩展实现成本过高,至少应补充单 & 的覆盖,并在 docstring 中明确记录 wrapper/env-assignment 形式为已知未覆盖场景,待后续处理。

问题 2:_RTK_PREFIX_RE 不感知引号,会误改写 shell 字符串字面量内的内容

  • [src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py:50] — 当 RTK 将 grep -E 'foo|rtk bar' src/ 重写为 rtk grep -E 'foo|rtk bar' src/ 后,当前正则同时会将单引号内的 | rtk 替换为 | /resolved/rtk,导致:

    • shell 实际执行的命令语义发生改变(传给 grep -E 的 pattern 被污染)
    • 即使 PATH 中已有 rtk 也会静默改变语义,是无条件的正确性回归

    ikunkun-sys 在内联 thread 中通过此 PR 的精确版本(dfb600d)复现确认(comment id: 3670645830)。

    修复建议:引入基于 shlex.split + token 位置的替换,仅替换 token 边界上的 rtk,跳过引号内内容。或等待 RTK 提供结构化 segment 输出(推荐长期方案)。最小可行修复示例:

    import shlex
    
    def _anchor_rtk_prefix_safe(rewritten: str, rtk_bin: str) -> str:
        """Quote-aware replacement using shlex token positions."""
        quoted = shlex.quote(rtk_bin)
        try:
            tokens = shlex.split(rewritten, posix=False)
        except ValueError:
            # 解析失败时退回原始字符串,不做替换(保守策略)
            return rewritten
        # 找出所有 segment 起始 token 的位置,仅替换这些位置的 bare `rtk`
        # (详细实现需跟踪 token 在原始字符串中的偏移量)
        ...

    注意:shlex 对复杂的多级引号/heredoc 仍有局限;若 RTK 未来提供结构化输出,应优先切换。

🟡 建议修改(不阻塞但推荐)

  • Hermes/OpenClaw 路径覆盖:issue 描述中提到独立的 Hermes 和 OpenClaw rewrite 路径仍向后续执行环境传递 bare rtk 输出。本 PR 未涉及这些路径,范围上是合理的,但建议在 PR 描述或代码注释中明确标注这是已知遗留问题,并开 follow-up issue 跟踪。

  • [src/tokenless/tests/test_rewrite_hook.py] — 现有测试覆盖了行首和 && 两种形式,但未覆盖问题 1 和问题 2 的回归场景。修复上述问题后,建议补充:

    • wrapper 形式:sudo git statussudo rtk git status 的锚定
    • &git status & grep foortk git status & rtk grep foo
    • quote-aware:grep -E 'foo|rtk bar' 被 RTK 重写后,引号内容不被修改
    • 环境变量赋值:RUST_BACKTRACE=1 cargo testRUST_BACKTRACE=1 rtk cargo test

🟢 值得肯定

  • 核心问题(行首和 &&/||/;/| 后 bare rtk 在 trimmed-PATH 下 exit-127)已正确修复,shlex.quote 处理了含空格/特殊字符安装路径的 edge case。
  • 测试设计扎实:fallback 布局 staging + 沙箱 HOME + 精确 PATH 复现受影响环境形态,负向对照验证有效。
  • _anchor_rtk_prefix 无条件锚定的决策合理,(?=\s) lookahead 防止误匹配裸尾部 rtk token。
  • 修改范围克制,只改了 3 个文件,未引入不必要的依赖。

审查基于 commit dfb600d,聚焦 tokenless 组件相关改动。

@Forrest-ly Forrest-ly 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.

总体评价

9a33e45 正确地用 shlex token 级实现替代了 dfb600d 的正则方案,解决了上一轮两个阻塞问题(引号感知、wrapper/env/单 & 覆盖)。但新的实现对所有 token 做 shlex.split(posix=True) + shlex.quote 往返重引号,静默破坏了 glob 展开、重定向、双引号变量/命令替换、tilde 展开等常见 shell 语义——这是比原正则方案更严重的正确性回归。必须修改后再合并。

审查结论

request changes


详细意见

🔴 必须修改(阻塞合并)

问题 1:shlex.quote 重引号破坏 glob / 重定向 / 变量展开 / 命令替换 / tilde

  • [rewrite_hook.py:101-103] — " ".join(token if token in _CONTROL_OPS else shlex.quote(token) for token in result) 对每个非控制符 token 调用 shlex.quote,而 shlex.split(posix=True) 已剥离原始引号。往返后所有含特殊字符的 token 都被单引号包裹,以下常见 shell 特性被静默禁用(均已用本 PR 实际代码复现):

    输入(rtk rewrite 输出) 实际输出 后果
    rtk git diff *.py … git diff '*.py' glob 不展开
    rtk git status > /dev/null … status '>' /dev/null 重定向变成字面参数
    rtk git log 2>&1 | head … log '2>&1' | head fd 重定向变成字面参数
    rtk git commit -m "release $VERSION" … -m 'release $VERSION' 变量不展开
    rtk git commit -m "$(date)" … -m '$(date)' 命令替换不执行
    rtk cat ~/.bashrc … cat '~/.bashrc' tilde 不展开

    dfb600d 的正则方案只替换 rtk 前缀、其余原样保留,不存在此问题;9a33e45 的重引号是新引入的回归。这些是 trimmed-PATH 场景下用户每天都会用到的命令形态,且由于 shlex.quote 是无条件的,即使 PATH 中已有 rtk 也会发生——与 PR"行为等价"的承诺矛盾。

    修复建议:改用 shlex.split(rewritten, posix=False)(保留原始引号作为 token 的一部分),仅替换裸 rtk token 为 shlex.quote(rtk_bin),其余 token 原样拼接,不做 shlex.quote

    tokens = shlex.split(rewritten, posix=False)   # 保留原始引号
    ...
    # 替换裸 rtk(posix=False 下 'rtk' 与 'rtk' 可区分)
    if not rtk_replaced and token == "rtk":
        result[i] = shlex.quote(rtk_bin)
        rtk_replaced = True
    ...
    return " ".join(result)   # 不对非 rtk token 重引号

    我已验证此方案保留 glob / 重定向 / 双引号 / tilde 语义,同时通过现有全部 6 个测试用例。

🟡 建议修改(不阻塞但推荐)

问题 2:in_segment 为死代码,裸 rtk 参数会被误替换

  • [rewrite_hook.py:85,90,94] — in_segment 初始化为 True、在控制符处重置为 True,但从未被置为 Falseif in_segment: 守卫恒为真。后果:当某段不以 rtk 开头时(rtk 透传的非 rewrite 段),段内作为参数出现的裸 rtk 会被误替换为绝对路径。已复现:

    rtk git status && echo rtk done
    → /resolved/rtk git status && echo /resolved/rtk done   # echo 的参数被改写
    

    这与上一轮阻塞的"引号内 rtk 被改写"属同一类静默语义变更。在本 PR 输入域(rtk rewrite 输出)中触发概率较低,建议作为已知限制处理。

    修复方向:在遇到第一个非 env-assignment、非控制符的命令 token 后置 in_segment = False;但需配合一个 wrapper 白名单(sudo/env/nice/nohup/exec 等)使其不触发置 False,否则 sudo rtk ... 的 rtk 将无法被锚定(会重新引入上一轮阻塞的 wrapper 问题)。若短期不做,至少在 docstring 中补充此已知限制。

问题 3:测试未覆盖重引号引入的回归场景

  • [test_rewrite_hook.py] — 现有 6 个测试覆盖了上一轮反馈的 wrapper / env / 单 & / 引号场景,但未覆盖问题 1 引入的回归。修复问题 1 后建议补充:
    • glob:rtk git diff *.py → 输出含裸 *.py(未被引号包裹)
    • 重定向:rtk git status > /dev/null> 保持为重定向符
    • 双引号变量:rtk git commit -m "$VAR"$VAR 可展开(双引号保留)
    • tilde:rtk cat ~/file~ 保持可展开

🟢 值得肯定

  • shlex token 级方案正确解决了 dfb600d 的两个阻塞问题:引号内容不再被修改,wrapper/env/单 & 后的 rtk 可被锚定。
  • ValueError 时退回原字符串的保守策略是好的安全网。
  • _is_env_assignment 对变量名的校验严谨(首字符 + 余字符白名单)。
  • 测试基础设施(沙箱 HOME + fallback layout staging + 精确 PATH 复现)设计扎实,负向对照思路正确。

审查基于 commit 9a33e45,聚焦 tokenless 组件相关改动。

@casparant

Copy link
Copy Markdown
Collaborator Author

Verified the claim against the code — confirmed on both paths:

  • hermes/__init__.py _try_rewrite resolves rtk_bin then returns proc.stdout.strip() verbatim
  • openclaw/index.ts uses result.stdout?.trim() verbatim after spawnSync(rtkPath, ["rewrite", command])

Same latent failure mode as #1974 in trimmed-PATH runtimes. Kept out of this PR to hold its scope to the shared hook path that produced the observed Qoder IDE symptom; tracked as follow-up #2123 with a concrete fix plan (share the anchor helper via hook_utils.py for Hermes, port it for the OpenClaw TS plugin with the same test matrix).

Also noting for reviewers: the quote-aware rework in the previous revision (9a33e45) re-quoted every token on re-join, which neutered unquoted globs (rtk grep foo *.txtgrep foo '*.txt') and #-leading arguments. Fixed in the latest revision (2d28a89) by lexing with posix=False so quoting/metacharacters survive verbatim and disabling comment stripping; two regression tests added (test_rewrite_preserves_unquoted_glob, test_rewrite_preserves_hash_argument), 8/8 pass, and both new tests fail against 9a33e45 as expected.

@casparant
casparant force-pushed the fix/tokenless/rtk-absolute-rewrite branch from 2d28a89 to f0f3a30 Compare August 1, 2026 10:28
@casparant

casparant commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

Branch hygiene note (re-review requested)

A maintainer-authored commit was pushed directly to this PR branch during the first review round (replacing the original implementation, later seen as 9a33e45). The PR author was not comfortable with direct pushes to their branch, so that commit has been dropped entirely, and maintainer_can_modify is now disabled on this PR.

The branch is back to a single author-owned commit (f0f3a30). All valid points from the first review round were re-processed and addressed by the author in this revision — see the two thread replies for specifics. The final implementation is a token-level pass (shlex posix=False lexer, comment stripping disabled) that anchors the first unquoted rtk token per segment while quoted patterns, unquoted globs, and #-leading arguments survive verbatim; 8 exact-match regression tests cover the full matrix (all 8 fail against origin/main, which emits the unanchored bare prefix).

Fresh review welcome.

@Forrest-ly Forrest-ly 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.

总体评价

2d28a89 正确解决了 9a33e45 的两个阻塞问题(重引号回归、死代码),但 punctuation_chars=True 引入了新的正确性回归:2>&12>/dev/null 等无空格 fd 重定向被拆分为多个 token 后重拼接,改变了 shell 语义。已验证一行修复即可解决。

审查结论

request changes

详细意见

🔴 必须修改(阻塞合并)

问题 1:punctuation_chars=True 破坏 2>&12>/dev/null 等 fd 重定向

  • [rewrite_hook.py:78] — shlex.shlex(rewritten, posix=False, punctuation_chars=True) 使 >& 等字符即使在 whitespace_split=True 下也被拆分为独立 token。2>&1(无空格)被拆为 ['2', '>&', '1']2>/dev/null 被拆为 ['2', '>', '/dev/null']" ".join(result) 重拼接后变成 2 >& 12 > /dev/null,shell 语义改变:

    • 2>&1(将 stderr 重定向到 stdout)→ 2 >& 12 变成命令参数,>& 1 变成无意义的 stdout 重定向)
    • 2>/dev/null(丢弃 stderr)→ 2 > /dev/null2 变成命令参数,stdout 被重定向到 /dev/null)

    已用本 PR 实际代码复现:

    输入: rtk git log 2>&1 | rtk head
    输出: /path/rtk git log 2 >& 1 | /path/rtk head   ← 语义改变
    
    输入: rtk git status 2>/dev/null
    输出: /path/rtk git status 2 > /dev/null   ← 语义改变
    

    这与 9a33e45 review 阻塞的"重引号破坏 shell 语义"属同类回归——对非 rtk token 的文本做了不必要的改写。dfb600d 的正则方案只替换 rtk 前缀,其余原样保留,不存在此问题。

    修复建议:移除 punctuation_chars=True,仅使用 posix=False + whitespace_split=True

    lexer = shlex.shlex(rewritten, posix=False)   # 移除 punctuation_chars=True
    lexer.whitespace_split = True
    lexer.commenters = ""

    已独立验证此修复后:

    • 2>&12>/dev/null>/dev/null 全部保留 ✓
    • glob *.py、tilde ~/.bashrc、双引号变量 "release $VER"、引号内 rtk 'foo|rtk bar' 全部保留 ✓
    • 现有全部 8 个测试用例的 tokenization 结果不变 ✓
    • wrapper(sudo rtk)、env assignment、单 &# 参数全部正常 ✓

    唯一代价:无空格分隔的 ;(如 cmd1;cmd2)不再被拆分为独立 token,但 rtk rewrite 输出中分号两侧几乎总有空格,且即使不拆分也不会导致错误——只是该段的 rtk 不被锚定(保守失败,不会改变语义)。

🟡 建议修改(不阻塞但推荐)

问题 2:裸 rtk 作为其他命令的参数仍会被误替换

  • [rewrite_hook.py:90-93] — 当前逻辑在同一 segment 中,只要还没替换过 rtkrtk_replaced == False),遇到任何值为 "rtk" 的 token 都会替换。这意味着非 segment 首位置的 rtk 参数也会被替换:

    rtk git status && echo rtk done
    → /path/rtk git status && echo /path/rtk done   ← echo 的参数被改写
    

    这是支持 wrapper 命令(sudo rtk ...)的必要代价——无法区分"wrapper 命令后面的 rtk"和"普通命令参数中的 rtk"。在 rtk rewrite 输出的输入域中触发概率很低(rtk 不太可能将 echo rtk done 作为 rewrite 输出),可作为已知限制处理。建议在 docstring 中补充说明。

  • [test_rewrite_hook.py] — 建议补充 fd 重定向的回归测试(本次 review 发现的新问题):

    • 2>&1rtk git log 2>&1 → 输出含裸 2>&1(未被拆分)
    • 2>/dev/nullrtk git status 2>/dev/null2>/dev/null 保持原样

🟢 值得肯定

  • posix=False + whitespace_split=True 的组合正确保留了引号、glob、tilde、变量展开等 shell 语义,解决了 9a33e45 的重引号回归。
  • rtk_replaced per-segment 标志替代了 9a33e45 的死代码 in_segment,逻辑更清晰。
  • lexer.commenters = "" 正确处理了 # 参数(如 #include),避免被截断为注释。
  • 测试覆盖扎实:8 个测试用例覆盖了 wrapper、env assignment、单 &、引号、glob、hash 等场景。
  • ValueError 退回原字符串的保守策略是好的安全网。
  • _is_env_assignment 对变量名的校验严谨。

审查基于 commit 2d28a89,聚焦 tokenless 组件相关改动。

@Forrest-ly Forrest-ly 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.

总体评价

本 review 为对 Forrest-ly 上一轮 review(commit f0f3a30)的增量补充,不重复其已列出的 2>&1/2>/dev/null 与 bare-rtk 参数问题。补充一点:punctuation_chars=True 还会破坏 $(...) 命令替换,Forrest-ly 的复现与修复验证清单均未覆盖此形态。根因与修复同 Forrest-ly(移除 punctuation_chars=True),本 review 独立验证了该修复同时解决命令替换。force-push(2d28a89f0f3a30)仅 squash,代码内容一致。

审查结论

request changes

详细意见

🔴 必须修改(阻塞合并)— 补充 Forrest-ly review

[src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py:80] punctuation_chars=True 还破坏 $(...) 命令替换(Forrest-ly 未列出)

shlex.shlex(rewritten, posix=False, punctuation_chars=True) 启用默认标点集 ();<>|&,使 $(date) 被拆为 ['$', '(', 'date', ')']" ".join 后变成 $ ( date ),命令替换语义被破坏:

输入(rtk rewrite 输出) _anchor_rtk_prefix 实际输出 后果
rtk echo $(date) …/rtk echo $ ( date ) $ 变字面量,( date ) 变独立子shell,命令替换输出不被捕获
rtk git log 2>&1 | rtk head … log 2 >& 1 | … head stderr 重定向失效(Forrest-ly 已列)
rtk git status 1>&2 … status 1 >& 2 fd 重定向失效(同根因)

命令替换是 rtk rewrite 输出中的现实场景:用户命令含 $(...) 时 rtk 透传该文本(hook 是 pre-call,$(...) 未展开)。这与 Forrest-ly 列出的 2>&1 同根因——punctuation_chars 把不应拆分的 shell 元字符拆成独立 token,join 再插空格。

修复:同 Forrest-ly 建议,移除 punctuation_chars=True

lexer = shlex.shlex(rewritten, posix=False)
lexer.whitespace_split = True
lexer.commenters = ""

已独立验证(移除 punctuation_chars 后):$(date)2>&12>/dev/null1>&2 全部原样保留;现有 8 个测试用例(wrapper/env/单&/引号/glob/hash/&&)输出不变;glob *.py、tilde ~/.bashrc、双引号变量 "$VERSION">>out.log&>file 均保留。Forrest-ly 的验证清单未含 $(...),本 review 补齐该验证。

🟡 建议修改(不阻塞但推荐)

  • [src/tokenless/tests/test_rewrite_hook.py] — Forrest-ly 已建议补 fd 重定向回归测试;同建议追加命令替换用例:rtk echo $(date) → 输出含裸 $(date)(未被拆分)。未来回归此 bug 时可立即捕获。

🟢 值得肯定

  • token 级 posix=False 方案正确保留引号/glob/tilde/变量展开,解决了前两轮 review 的阻塞问题。
  • ValueError 退回原字符串的保守策略是好的安全网。
  • 测试基础设施(沙箱 HOME + fallback layout + 精确 PATH 复现受影响环境)设计扎实。

审查基于 commit f0f3a30(与 Forrest-ly 上轮 2d28a89 内容一致,force-push 仅 squash),聚焦 tokenless 组件相关改动。本 review 为增量补充,不重复 Forrest-ly 已覆盖的 2>&1/2>/dev/null 与 bare-rtk 参数问题。

@casparant
casparant removed the request for review from shiloong August 1, 2026 10:49
rtk emits rewritten commands with a bare `rtk` prefix, which only
resolves when the shell executing the tool call has the rtk location
on its PATH. Agent runtimes with a trimmed PATH (e.g. Qoder IDE tool
environments without ~/.local/bin) failed every rewritten command
with exit 127 even though the hook resolved rtk successfully via the
fallback layouts — the 0.7.3 cross-layout discovery only covered the
hook process itself.

Anchor the rewrite to the resolved absolute rtk_bin. A token-level
pass (shlex posix=False lexer, no punctuation splitting, comment
stripping disabled) replaces the first unquoted `rtk` token in each
shell segment — at command start and after whitespace-delimited
&&, ||, ;, |, single &, optionally behind leading environment
assignments or wrappers such as sudo — while quoted patterns,
unquoted globs, #-leading arguments, fd redirections (2>&1,
2>/dev/null), and command substitutions ($(date)) all survive
verbatim. Anchoring is unconditional: the absolute path is correct
whether or not rtk was on PATH. Unspaced connectives are left
unsplit (conservative, never corrupts); unparseable shell is
returned untouched.

Adds tests/test_rewrite_hook.py wired into `make test`
(test-integration): stages a fake rtk/tokenless pair in the fallback
layout under a sandboxed HOME, runs the hook with a PATH lacking the
rtk location, and asserts segment anchoring plus
quote/glob/hash/fd-redirection/substitution preservation across 11
exact-match cases.

Assisted-by: Qoder:1.18.2
Signed-off-by: Caspar Zhang <caspar@linux.alibaba.com>
@casparant
casparant force-pushed the fix/tokenless/rtk-absolute-rewrite branch from f0f3a30 to 99d0a97 Compare August 1, 2026 10:58
@casparant

Copy link
Copy Markdown
Collaborator Author

Addressed in 99d0a97 — both punctuation_chars regressions from the latest two reviews:

  1. fd redirection (@Forrest-ly review 10:30): removed punctuation_chars=True per your suggested one-line fix. Verified: rtk git log 2>&1 | rtk head…/rtk git log 2>&1 | …/rtk head and rtk git status 2>/dev/null stay byte-for-byte intact; spaced connectives still anchor correctly; the pre-existing 8 tests are unaffected.
  2. Command substitution (@Forrest-ly review 10:42): same root cause, same fix — rtk echo $(date) no longer splits into $ ( date ).

Added 3 exact-match regression tests (test_fd_merging_preserved, test_fd_redirection_preserved, test_command_substitution_preserved); 11/11 pass, and exactly these 3 fail against the previous revision as expected. The known limitation on argument-position bare rtk is now documented in the _anchor_rtk_prefix docstring per your 🟡 suggestion.

Re-review requested.

@casparant
casparant dismissed stale reviews from ikunkun-sys, Forrest-ly, and Forrest-ly August 1, 2026 11:00

Resolved in later revisions: wrapper/env-assignment/single-& anchoring and quote-aware substitution are all fixed on the current head (99d0a97), with regression tests. Hermes/OpenClaw coverage is tracked separately in #2123.

@Forrest-ly Forrest-ly 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.

总体评价

99d0a97 正确解决了上一轮 review(f0f3a30)的全部阻塞问题。移除 punctuation_chars=True 后,2>&12>/dev/null$(date) 等 shell 结构不再被拆分;posix=False + whitespace_split=True + commenters="" 的组合精确实现了 review 建议的方案。11 个测试全部通过,实现克制、保守、文档完善。

审查结论

approve

详细意见

必须修改(阻塞合并)

无。

建议修改(不阻塞但推荐)

无新增建议。上一轮的非阻塞观察(裸 rtk 作为其他命令参数会被误替换)已在 docstring 中作为已知限制记录,输入域(rtk rewrite 输出)中该形态不出现,接受。

值得肯定

  • 阻塞问题全部修复shlex.shlex(rewritten, posix=False) 不再使用 punctuation_chars=True2>&12>/dev/null$(date)" ".join(result) 后保持原样。已逐项验证。
  • 仅锚定 rtk token:只有 rtk_binshlex.quote 包裹,其余 token 原样拼接,不引入 glob/redirect/variable/tilde 的重引号回归(9a33e45 问题已解决)。
  • wrapper/env/单 & 覆盖完整wrapped 标志在 segment op 处重置、env assignment 处跳过、首个 rtk 处置位,sudo rtkVAR=1 rtkcmd & rtk 均正确锚定(dfb600d 问题已解决)。
  • 引号感知posix=False 保留原始引号作为 token 一部分,grep -E 'foo|rtk bar' 中引号内 rtk 不被替换(dfb600d 问题已解决)。
  • 保守的失败处理ValueError 时返回原始字符串不做替换,不破坏命令。
  • 测试覆盖扎实:11 个 exact-match 用例覆盖 wrapper、env assignment、单 &、引号、glob、# 参数、2>&12>/dev/null$(date),沙箱 HOME + 精确 PATH 复现受影响环境形态。
  • 已知限制文档化:docstring 明确记录裸 rtk 参数误替换和 unspaced connective 不拆分两个限制,均为保守行为(不破坏语义)。

审查基于 commit 99d0a97,聚焦 tokenless 组件改动(rewrite_hook.py + test_rewrite_hook.py + Makefile)。

@Forrest-ly Forrest-ly 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.

总体评价

commit 99d0a97 已完整回应 f0f3a30 轮 review 的两个阻塞问题:移除 punctuation_chars=True2>&12>/dev/null$(...) 等 shell 元字符不再被拆分;shlex token 级方案正确覆盖了 wrapper、env 赋值、单 &、引号、glob、# 参数、fd 重定向和命令替换场景。11 个精确匹配测试全部通过,实现克制,测试扎实。

审查结论

approve

详细意见

🟡 建议修改(不阻塞但推荐)

docstring 中 "never corrupts" 表述略强于实际行为

  • [src/tokenless/adapters/tokenless/common/hooks/rewrite_hook.py:80-81] 文档称未空格分隔的连接符 "never corrupts",但当输入形如 rtk cmd1;rtk cmd2; 前无空格)时,shlex 会将 cmd1;rtk 视为单个 token,拼接后变成 /path/rtk cmd1;rtk cmd2,shell 语义从 "两个命令" 变为 "一个命令带奇怪参数。rtk rewrite 输出大概率 always 对连接符加空格,因此不构成阻塞,但建议将措辞收敛为 "unspaced connectives are left unsplit, so only that segment's rtk` remains unanchored" 之类的描述,避免读者过度依赖 "never corrupts"。

🟢 值得肯定

  • posix=False + whitespace_split=True 且不带 punctuation_chars 的 shlex 方案,正确保留引号、glob、tilde、变量展开、fd 重定向和命令替换语义。
  • 对不可解析输入回退原字符串的保守策略合理。
  • commenters="" 避免 # 参数被截断。
  • wrapped per-segment 标志逻辑清晰,仅替换每段第一个裸 rtk
  • 沙箱 HOME + fallback layout + 精确 PATH 的测试基础设施精确复现了受影响环境,负向对照有效。
  • 11 个测试用例覆盖了上一轮 review 要求的 fd 重定向和命令替换回归场景。

@casparant
casparant merged commit ae83f7d into alibaba:main Aug 1, 2026
19 checks passed
Forrest-ly added a commit to Forrest-ly/anolisa that referenced this pull request Aug 9, 2026
Both adapters returned rtk's rewrite output verbatim, so bare `rtk`
tokens in the result failed with exit 127 in agent runtimes whose PATH
lacked the rtk location — the same failure mode fixed for the shared
hook in PR alibaba#1975.

- Move _anchor_rtk_prefix/_is_env_assignment/_SEGMENT_OPS into
  hook_utils.py so rewrite_hook.py and hermes/__init__.py share one
  implementation; rewrite_hook.py now imports rather than defines them.
- hermes/__init__.py _try_rewrite: call _anchor_rtk_prefix on the rtk
  output before building the block directive.
- openclaw/index.ts: port the anchor logic (shellTokenize +
  anchorRtkPrefix + isEnvAssignment + SEGMENT_OPS) with posix=False
  semantics — quoted strings, globs, fd redirections, and command
  substitutions are preserved verbatim; call anchorRtkPrefix in
  tryRtkRewrite before returning the result.
- Add tests/test_openclaw_anchor.mjs covering the full case matrix:
  simple rewrite, multiple &&-separated segments, sudo wrapper, env
  assignments, single &, quoted rtk pattern, unquoted glob, hash arg,
  fd merge (2>&1), fd redirect (2>/dev/null), command substitution
  $(date), spaced path quoting, no-rtk passthrough, unmatched quote.

Co-authored-by: multica-agent <github@multica.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:tokenless src/tokenless/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rewrite hook emits bare rtk prefix — rewritten commands fail (exit 127) when the tool shell's PATH lacks the rtk location

4 participants