(MOT-3873) feat(shell): str_replace update op + coder::context workspace snapshot#409
(MOT-3873) feat(shell): str_replace update op + coder::context workspace snapshot#409ytallo wants to merge 1 commit into
Conversation
- coder::update-file gains { op: "str_replace", old_str, new_str,
replace_all? }: exact-literal replacement desugared onto the existing
content-op machinery (escaped-literal regex + $$-escaped replacement).
Without replace_all the op requires exactly one occurrence and fails
C210 (nothing written) on 0 or N matches with prescriptive recovery;
replace_all replaces every occurrence (>=1 required).
- new coder::context: one-call workspace snapshot (allowed roots,
platform, bounded git branch/status/log, AGENTS.md / CLAUDE.md
instruction files capped at 16 KiB) for seeding coding sessions;
git state degrades to null on non-repo roots or missing git binary.
- wire-schema goldens regenerated; catalog grows to 10 functions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 32 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughThis PR adds a new Changescoder::context endpoint
update-file str_replace operation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant register_context
participant handle
participant PathResolver
participant GitCLI
Caller->>register_context: coder::context request
register_context->>PathResolver: scope_root/scope_grants(fs_scope)
register_context->>handle: handle(resolver, cfg, req)
handle->>PathResolver: resolve primary_root, base_paths
handle->>GitCLI: git rev-parse/status/log (2s timeout)
GitCLI-->>handle: branch, status, commits or failure
handle->>PathResolver: resolve AGENTS.md/CLAUDE.md
handle-->>register_context: ContextOutput
register_context-->>Caller: response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shell/src/code/functions/update_file.rs (1)
501-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect implementation of literal
str_replacevia escaped-literal regex.The
regex::escape(old_str)pattern plusnew_str.replace('$', "$$")replacement correctly produces verbatim expansion —Captures::expandtreats$$as a literal-$escape, so a$innew_str(e.g.${name}) survives untouched. TheCountRuleunification cleanly enforcesstr_replace's "found-and-unique orreplace_all" contract alongsideReplace'sexpect_matches, and matches the unit tests (str_replace_dollar_in_new_str_lands_verbatim,str_replace_ambiguous_without_replace_all_is_c210, etc.).One readability note: this function now handles two fairly different op kinds' setup logic plus the shared match/count-guard machinery in a single ~150-line body. Extracting the per-op-kind setup (the two match arms building
(Regex, Cow<str>, CountRule)) into small helper functions (e.g.compile_replace_op,compile_str_replace_op) would reduce the cognitive load of this function without touching behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/code/functions/update_file.rs` around lines 501 - 670, The logic in apply_regex_ops is correct, but the per-op setup inside the match on UpdateOp is doing too much in one long function. Extract the two branches that build (Regex, Cow<str>, CountRule) into small helpers like compile_replace_op and compile_str_replace_op, then keep the shared replacement/count-guard flow in apply_regex_ops. This will reduce cognitive load without changing behavior and makes the CountRule and regex compilation paths easier to follow.shell/src/code/functions/context.rs (1)
143-165: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential git invocations add up to ~6s worst-case latency for a "one-call" snapshot.
branch,status, andrecent_commitsare fetched with three sequential.awaits, each bounded by a 2sGIT_TIMEOUT. In a degenerate case (e.g., slow FS, large repo) the handler could take up to ~6s even thoughstatusandrecent_commitsdon't depend on each other. Since the doc comment frames this as a fast one-call bootstrap primitive, running these concurrently would meaningfully cut worst-case latency.♻️ Proposed refactor: run status/log concurrently after the repo probe
let branch = git_lines(root, &["rev-parse", "--abbrev-ref", "HEAD"]) .await? .into_iter() .next()?; - let mut status = git_lines(root, &["status", "--porcelain"]) - .await - .unwrap_or_default(); + let (status_res, commits_res) = tokio::join!( + git_lines(root, &["status", "--porcelain"]), + git_lines(root, &["log", "--oneline", &format!("-{RECENT_COMMITS}")]) + ); + let mut status = status_res.unwrap_or_default(); let status_truncated = status.len() > STATUS_MAX_ENTRIES; status.truncate(STATUS_MAX_ENTRIES); - // Fails on a repo with no commits yet — an empty history, not an error. - let recent_commits = git_lines(root, &["log", "--oneline", &format!("-{RECENT_COMMITS}")]) - .await - .unwrap_or_default(); + let recent_commits = commits_res.unwrap_or_default();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/code/functions/context.rs` around lines 143 - 165, The git snapshot in git_context is doing branch, status, and recent_commits fetches sequentially, which unnecessarily stretches the one-call latency. Keep the repo probe via git_lines(... "rev-parse" ...) first, then run the independent status and log calls concurrently (for example by starting both futures after branch is known and awaiting them together), and preserve the existing truncation/default handling for STATUS_MAX_ENTRIES and RECENT_COMMITS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shell/src/code/functions/context.rs`:
- Around line 124-141: The git command in git_lines can outlive the timeout
because dropping the output() future does not stop the spawned process; update
the tokio::process::Command setup in git_lines to enable kill_on_drop(true) so
timed-out git invocations are terminated instead of leaving background children
running.
---
Nitpick comments:
In `@shell/src/code/functions/context.rs`:
- Around line 143-165: The git snapshot in git_context is doing branch, status,
and recent_commits fetches sequentially, which unnecessarily stretches the
one-call latency. Keep the repo probe via git_lines(... "rev-parse" ...) first,
then run the independent status and log calls concurrently (for example by
starting both futures after branch is known and awaiting them together), and
preserve the existing truncation/default handling for STATUS_MAX_ENTRIES and
RECENT_COMMITS.
In `@shell/src/code/functions/update_file.rs`:
- Around line 501-670: The logic in apply_regex_ops is correct, but the per-op
setup inside the match on UpdateOp is doing too much in one long function.
Extract the two branches that build (Regex, Cow<str>, CountRule) into small
helpers like compile_replace_op and compile_str_replace_op, then keep the shared
replacement/count-guard flow in apply_regex_ops. This will reduce cognitive load
without changing behavior and makes the CountRule and regex compilation paths
easier to follow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41b96137-b7e6-4abd-a061-227b31db6088
📒 Files selected for processing (8)
shell/src/code/functions/context.rsshell/src/code/functions/mod.rsshell/src/code/functions/update_file.rsshell/tests/code_context.rsshell/tests/code_golden_schemas.rsshell/tests/code_update_ops.rsshell/tests/golden/schemas/coder.context.jsonshell/tests/golden/schemas/coder.update-file.json
| async fn git_lines(root: &Path, args: &[&str]) -> Option<Vec<String>> { | ||
| let fut = tokio::process::Command::new("git") | ||
| .arg("-C") | ||
| .arg(root) | ||
| .args(args) | ||
| .stdin(std::process::Stdio::null()) | ||
| .output(); | ||
| let out = tokio::time::timeout(GIT_TIMEOUT, fut).await.ok()?.ok()?; | ||
| if !out.status.success() { | ||
| return None; | ||
| } | ||
| Some( | ||
| String::from_utf8_lossy(&out.stdout) | ||
| .lines() | ||
| .map(str::to_string) | ||
| .collect(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does tokio::process::Command::output() kill the child process on timeout/drop by default, or does it require kill_on_drop(true)?
💡 Result:
By default, tokio::process::Command::output() does not kill the child process if the future is dropped before it completes [1][2][3]. To enable this behavior, you must explicitly call kill_on_drop(true) on the Command object before spawning or running the output command [4][2][3]. In the tokio library, dropping a Child process handle (or the future returned by output()) does not trigger cancellation by default, a design choice intended to mirror the behavior of the standard library's std::process::Command [1][2][3]. If kill_on_drop(true) is set, the destructor of the future returned by output() will attempt to kill the child process if it is still running when the future is dropped [2][3]. Because of the potential for zombie processes and the non-guaranteed nature of automatic reaping, it is generally recommended to explicitly manage the child process lifecycle using child.wait().await or child.kill().await when strict cleanup guarantees are required [2][5][6].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/process/index.html
- 2: https://docs.rs/tokio/1.32.0/tokio/process/struct.Command.html
- 3: https://docs.rs/tokio/1.48.0/tokio/process/struct.Command.html
- 4: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 5: https://docs.rs/tokio/latest/%20tokio/process/struct.Child.html
- 6: https://github.com/tokio-rs/tokio/blob/master/tokio/src/process/mod.rs
Kill timed-out git children in shell/src/code/functions/context.rs:124-131 tokio::time::timeout drops the output() future, but without kill_on_drop(true) the spawned git process can keep running after the deadline. Add kill_on_drop(true) here so timed-out calls don't leave background processes behind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/src/code/functions/context.rs` around lines 124 - 141, The git command
in git_lines can outlive the timeout because dropping the output() future does
not stop the spawned process; update the tokio::process::Command setup in
git_lines to enable kill_on_drop(true) so timed-out git invocations are
terminated instead of leaving background children running.
|
Consolidated into #423. |
1 participant
replace_all? }: exact-literal replacement desugared onto the existing
content-op machinery (escaped-literal regex + $$-escaped replacement).
Without replace_all the op requires exactly one occurrence and fails
C210 (nothing written) on 0 or N matches with prescriptive recovery;
replace_all replaces every occurrence (>=1 required).
platform, bounded git branch/status/log, AGENTS.md / CLAUDE.md
instruction files capped at 16 KiB) for seeding coding sessions;
git state degrades to null on non-repo roots or missing git binary.
Refs MOT-3873
Summary by CodeRabbit
New Features
Bug Fixes