feat: introducing coder worker - #189
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces a complete "coder" worker implementation for the iii framework: a path-jailed filesystem access worker with seven core functions (read, create, delete, list-folder, tree, search, update). It includes security guarantees via canonical path resolution and glob-based access control, atomic file writes via temp+rename, comprehensive BDD test coverage with feature files and step definitions, and full operator/user documentation. ChangesCoder Worker Implementation
🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
coder/Cargo.toml (2)
1-2: 💤 Low valueClarify or remove the empty workspace definition.
An empty
[workspace]section is unusual. If this crate is standalone, the section can be omitted entirely. If it's meant to define a workspace,membersshould be listed. Empty workspace sections can sometimes cause unexpected behavior with cargo commands.♻️ Suggested fix
If standalone:
-[workspace] - [package]Or if this should be a workspace root:
[workspace] +members = ["coder"]🤖 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 `@coder/Cargo.toml` around lines 1 - 2, The Cargo.toml contains an empty [workspace] table which is either unnecessary or incomplete; either remove the empty [workspace] section if this crate is standalone, or convert it into a proper workspace root by adding a members = [...] entry listing the workspace crates (and any optional workspace keys like exclude) so Cargo recognizes the workspace correctly; update the [workspace] table (or delete it) accordingly.
17-17: Confirm rationale for exactiii-sdkpre-release pin (=0.13.0-next.1)
coder/Cargo.tomlpinsiii-sdk = "=0.13.0-next.1", and the same exact constraint (and resolved version inCargo.lock) is used across all other workers—this looks intentionally coordinated rather than an accidental drift. Add a brief comment documenting why the exact pre-release pin is required (compatibility/testing), or relax the constraint if automatic updates are expected.🤖 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 `@coder/Cargo.toml` at line 17, The Cargo.toml currently pins the iii-sdk dependency exactly to "=0.13.0-next.1"; either document why this exact pre-release pin is required (compatibility/testing/ABI lockstep across workers) by adding a brief comment next to the iii-sdk = "=0.13.0-next.1" line explaining the rationale, or relax the constraint to allow updates (for example use "^0.13.0-next.1" or "0.13.0-next" per your intended update policy) so automated updates won't be blocked; update the line referencing the iii-sdk crate and include the comment or new constraint consistently across the other worker Cargo.toml entries to keep behavior coordinated.coder/src/main.rs (1)
45-63: ⚡ Quick winConsider whether config load failures should be fatal.
The worker falls back to defaults if config loading fails, which allows it to start even with a missing or corrupt config file. While the warning is logged, an operator might not notice that their custom configuration isn't being applied.
Consider making config load failures fatal (like PathResolver failures) to ensure operators are aware of configuration issues at startup.
🔒 Alternative: fail fast on config errors
- let cfg = match config::load_config(&cli.config) { - Ok(c) => { - tracing::info!( - base_path = %c.base_path.display(), - non_accessible_globs = c.non_accessible_globs.len(), - "loaded config from {}", - cli.config - ); - c - } - Err(e) => { - tracing::warn!( - error = %e, - path = %cli.config, - "failed to load config, using defaults" - ); - config::CoderConfig::default() - } - }; + let cfg = config::load_config(&cli.config).map_err(|e| { + anyhow::anyhow!( + "failed to load config from {}: {}", + cli.config, + e + ) + })?; + tracing::info!( + base_path = %cfg.base_path.display(), + non_accessible_globs = cfg.non_accessible_globs.len(), + "loaded config from {}", + cli.config + );🤖 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 `@coder/src/main.rs` around lines 45 - 63, The code currently swallows config::load_config errors and falls back to config::CoderConfig::default(), which lets the process continue with defaults; change the Err branch to fail fast instead: replace the current tracing::warn + default return with a tracing::error that includes the error and path (use error = %e, path = %cli.config) and then terminate startup (e.g., by returning an Err from main or calling std::process::exit(1) / panic!) so the program does not continue with defaults—update the match on config::load_config accordingly.coder/tests/steps/security.rs (2)
1-21: 💤 Low valueRemove unnecessary
asynckeyword.The
given_symlink_escapefunction doesn't perform any async operations (no.awaitcalls), so theasynckeyword is unnecessary. While harmless, removing it would make the code clearer.♻️ Proposed fix
#[given(regex = r#"^a symlink at "([^"]+)" pointing to a path outside base$"#)] -async fn given_symlink_escape(world: &mut CoderWorld, rel: String) { +fn given_symlink_escape(world: &mut CoderWorld, rel: String) { if world.iii.is_none() {🤖 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 `@coder/tests/steps/security.rs` around lines 1 - 21, The function given_symlink_escape is declared async but contains no await and should be synchronous; change its signature from "async fn given_symlink_escape(world: &mut CoderWorld, rel: String)" to a plain "fn given_symlink_escape(...)" (keeping the cucumber #[given(...)] attribute and parameter types intact) and leave the body unchanged (still returning early on None). Also verify no call sites expect an async handler for given_symlink_escape.
25-25: ⚡ Quick winReplace deprecated
.keep()with.into_path().The
.keep()method was deprecated in tempfile 3.0 in favor of.into_path(). Both have the same behavior (consume theTempDirand returnPathBufwithout cleanup), but using the non-deprecated API prevents potential compiler warnings.♻️ Proposed fix
- let outside = tempfile::tempdir().expect("escape tempdir").keep(); + let outside = tempfile::tempdir().expect("escape tempdir").into_path();🤖 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 `@coder/tests/steps/security.rs` at line 25, The code uses the deprecated TempDir::keep() call when constructing the outside PathBuf; update the expression that creates outside (tempfile::tempdir().expect("escape tempdir").keep()) to call .into_path() instead of .keep() so the TempDir is consumed and a PathBuf is returned without triggering deprecation warnings.
🤖 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 `@coder/src/functions/create_file.rs`:
- Around line 110-121: The current create_file path checks abs.exists() and then
calls std::fs::write which is not atomic and allows a race; instead, for the
branch where spec.overwrite is false use std::fs::OpenOptions with
create_new(true) (and write/append/truncate as appropriate) to open the file
atomically and fail if it already exists, replacing the exists() check and the
std::fs::write call in that branch; keep the spec.parents handling and use
OpenOptions::new().write(true).create_new(true) to write the bytes and propagate
errors via CoderError::from.
- Around line 121-132: Parse and validate the mode string before writing the
file so bad mode strings or parse errors fail before any disk side-effect;
specifically, in create_file validate/convert spec.mode to a numeric u32 (handle
"0000" by treating an empty trimmed string as "0" or otherwise accept leading
zeros) and return Err on parse failure, then call std::fs::write and finally
call apply_mode with the parsed numeric mode (or adjust apply_mode to accept a
u32 instead of re-parsing the string) so set_permissions is only attempted after
a known-good mode.
In `@coder/src/functions/list_folder.rs`:
- Around line 101-112: The code currently uses e.metadata() (which follows
symlinks) to classify entries, causing broken symlinks to be skipped and symlink
kinds to reflect their targets; in list_folder change the kind determination to
use e.file_type() or e.symlink_metadata() (e.g., call e.file_type() and pass
that result into classify or adjust classify to accept FileType) so symlinks are
classified correctly, and only call e.metadata() when you need target-specific
size/mtime (wrap that in its own match so broken symlinks don’t cause continue);
update the DirEntry construction (symbols: DirEntry, classify, unix_mtime,
resolver.is_non_accessible) to use file_type for kind and keep metadata-derived
size/mtime optional.
In `@coder/src/functions/read_file.rs`:
- Around line 51-67: The current code checks md.len() but then uses
std::fs::read(&abs) which can race; replace the unbounded read with a
size-limited read or at minimum validate the actual bytes length after reading.
Open the file at abs and read via a capped reader (std::io::Read::take using
cfg.max_read_bytes + 1) or call std::fs::read and then immediately check
bytes.len() against cfg.max_read_bytes and return CoderError::TooLarge
(including req.path and sizes) before converting to String in the
String::from_utf8 branch; ensure you reference md, abs, bytes,
cfg.max_read_bytes and req.path in the fix.
In `@coder/src/functions/search.rs`:
- Around line 165-214: The code uses a single shared truncated flag that both
the path-matching branch and the content-matching branch set, which causes one
capped list (path_matches or content_matches) to prematurely stop the entire
walk and corrupt the other list; introduce two separate flags (e.g.,
path_truncated and content_truncated) and update the path-matcher block (where
PathMatch is pushed) to set path_truncated when path_matches.len() >=
max_matches without affecting content processing, and update the content-matcher
block to set and check content_truncated (instead of truncated) when
content_matches reaches max_matches and only break/stop the content loop or
outer walk based on content_truncated; replace uses of truncated in loop-break
logic with the appropriate per-list flag and keep the existing symbols
path_matcher, content_matcher, path_matches, content_matches, and the truncation
checks aligned to each list.
In `@coder/src/functions/tree.rs`:
- Around line 180-205: The code is incorrectly following symlinks by calling
e.metadata() for classification and skipping dangling symlinks on metadata
errors; update the logic in the walk_dir/TreeNode construction to use
e.file_type() and e.symlink_metadata() instead of e.metadata() so you can detect
and preserve NodeKind::Symlink (and base size/mtime on the symlink's own
metadata when available), avoid continuing on e.metadata() errors for symlinks,
and apply the same change at the other occurrence around the 220-231 block;
reference functions/values: walk_dir, TreeNode, classify, e.file_type(),
e.symlink_metadata(), and resolver.is_non_accessible to implement this behavior.
In `@coder/src/functions/update_file.rs`:
- Around line 376-395: atomic_write currently writes a temp file and renames it
over target but loses the original file mode; before renaming in atomic_write,
obtain the target's permissions via std::fs::metadata(target)?.permissions()
(guarding for target's absence) and apply them to the tmp_path with
std::fs::set_permissions(&tmp_path, perms). Ensure you handle and propagate
errors similarly to the existing write/rename error handling (clean up tmp file
on failure) so the temp file inherits the target's mode prior to
std::fs::rename.
In `@coder/src/path/mod.rs`:
- Around line 63-89: resolve() currently calls canonicalize_with_fallback() on
the raw joined path which allows inputs with `..` to bypass the symlink-escape
check; before canonicalization, lexically normalize the relative components
(collapse "." and ".." without following symlinks) by walking
joined.components() into a new PathBuf: skip "." components, pop on ".." and if
a pop would escape the base_root_canon (i.e., you pop past the joined prefix)
return an appropriate CoderError (e.g., OutsideBase or BadInput), otherwise push
normal components; then call canonicalize_with_fallback() on that normalized
PathBuf (instead of joined) and keep the existing canonical-starts_with
base_root_canon check. Ensure you update references to joined -> normalized when
calling canonicalize_with_fallback and in subsequent checks.
In `@coder/tests/common/engine.rs`:
- Around line 55-60: The worker registration error is being swallowed by .ok()?
on the register_all(&iii).await call; change that to propagate the error instead
of converting it to None so failures surface as hard test errors — replace
register_all(&iii).await.ok()? with a propagation (e.g.,
register_all(&iii).await? or propagate the Result from register_all directly) in
the get_or_init closure so registration failures fail fast (check types around
get_or_init, try_connect_raw, and register_all to adjust the return/Result
handling as needed).
In `@coder/tests/integration.rs`:
- Around line 35-76: boot() currently conflates "iii missing" and post-discovery
failures by returning None after iii has started (variables iii and worker),
which causes later tests to be skipped and can leave orphaned processes; change
boot() to return a Result<Harness, BootOutcome> (or similar) so that
which::which("iii") still maps to Err(BootOutcome::MissingEngine) (skip), but
any failures after spawning iii (e.g., Command::spawn() for worker failing)
return an Err indicating a real boot failure; ensure you properly kill and wait
on iii before returning that error (use the existing iii.kill()/iii.wait()
cleanup) and update call sites to only skip on BootOutcome::MissingEngine.
---
Nitpick comments:
In `@coder/Cargo.toml`:
- Around line 1-2: The Cargo.toml contains an empty [workspace] table which is
either unnecessary or incomplete; either remove the empty [workspace] section if
this crate is standalone, or convert it into a proper workspace root by adding a
members = [...] entry listing the workspace crates (and any optional workspace
keys like exclude) so Cargo recognizes the workspace correctly; update the
[workspace] table (or delete it) accordingly.
- Line 17: The Cargo.toml currently pins the iii-sdk dependency exactly to
"=0.13.0-next.1"; either document why this exact pre-release pin is required
(compatibility/testing/ABI lockstep across workers) by adding a brief comment
next to the iii-sdk = "=0.13.0-next.1" line explaining the rationale, or relax
the constraint to allow updates (for example use "^0.13.0-next.1" or
"0.13.0-next" per your intended update policy) so automated updates won't be
blocked; update the line referencing the iii-sdk crate and include the comment
or new constraint consistently across the other worker Cargo.toml entries to
keep behavior coordinated.
In `@coder/src/main.rs`:
- Around line 45-63: The code currently swallows config::load_config errors and
falls back to config::CoderConfig::default(), which lets the process continue
with defaults; change the Err branch to fail fast instead: replace the current
tracing::warn + default return with a tracing::error that includes the error and
path (use error = %e, path = %cli.config) and then terminate startup (e.g., by
returning an Err from main or calling std::process::exit(1) / panic!) so the
program does not continue with defaults—update the match on config::load_config
accordingly.
In `@coder/tests/steps/security.rs`:
- Around line 1-21: The function given_symlink_escape is declared async but
contains no await and should be synchronous; change its signature from "async fn
given_symlink_escape(world: &mut CoderWorld, rel: String)" to a plain "fn
given_symlink_escape(...)" (keeping the cucumber #[given(...)] attribute and
parameter types intact) and leave the body unchanged (still returning early on
None). Also verify no call sites expect an async handler for
given_symlink_escape.
- Line 25: The code uses the deprecated TempDir::keep() call when constructing
the outside PathBuf; update the expression that creates outside
(tempfile::tempdir().expect("escape tempdir").keep()) to call .into_path()
instead of .keep() so the TempDir is consumed and a PathBuf is returned without
triggering deprecation warnings.
🪄 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: ea15827e-6880-4bc6-a7b2-354b30aeab12
⛔ Files ignored due to path filters (1)
coder/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
coder/Cargo.tomlcoder/README.mdcoder/build.rscoder/config.yamlcoder/iii.worker.yamlcoder/skills/coder.mdcoder/skills/index.mdcoder/src/config.rscoder/src/error.rscoder/src/functions/create_file.rscoder/src/functions/delete_file.rscoder/src/functions/list_folder.rscoder/src/functions/mod.rscoder/src/functions/read_file.rscoder/src/functions/search.rscoder/src/functions/tree.rscoder/src/functions/update_file.rscoder/src/lib.rscoder/src/main.rscoder/src/manifest.rscoder/src/path/mod.rscoder/tests/bdd.rscoder/tests/common/engine.rscoder/tests/common/helpers.rscoder/tests/common/mod.rscoder/tests/common/workers.rscoder/tests/common/world.rscoder/tests/features/create_file.featurecoder/tests/features/delete_file.featurecoder/tests/features/lifecycle.featurecoder/tests/features/list_folder.featurecoder/tests/features/path_security.featurecoder/tests/features/read_file.featurecoder/tests/features/search.featurecoder/tests/features/tree.featurecoder/tests/features/update_file.featurecoder/tests/integration.rscoder/tests/manifest.rscoder/tests/path_jail.rscoder/tests/steps/common.rscoder/tests/steps/create.rscoder/tests/steps/delete.rscoder/tests/steps/lifecycle.rscoder/tests/steps/list.rscoder/tests/steps/mod.rscoder/tests/steps/read.rscoder/tests/steps/search.rscoder/tests/steps/security.rscoder/tests/steps/tree.rscoder/tests/steps/update.rscoder/tests/update_ops.rs
Comment on lines
+110
to
+121
| if abs.exists() && !spec.overwrite { | ||
| return Err(CoderError::AlreadyExists(format!( | ||
| "{} already exists; pass overwrite=true to replace", | ||
| spec.path | ||
| ))); | ||
| } | ||
| if spec.parents { | ||
| if let Some(parent) = abs.parent() { | ||
| std::fs::create_dir_all(parent).map_err(CoderError::from)?; | ||
| } | ||
| } | ||
| std::fs::write(&abs, bytes).map_err(CoderError::from)?; |
There was a problem hiding this comment.
Make the overwrite: false path atomic.
Lines 110-121 do exists() and then write(), so another writer can create the file between those calls and still get overwritten even though overwrite is false. Use OpenOptions::create_new(true) for the non-overwrite branch.
🤖 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 `@coder/src/functions/create_file.rs` around lines 110 - 121, The current
create_file path checks abs.exists() and then calls std::fs::write which is not
atomic and allows a race; instead, for the branch where spec.overwrite is false
use std::fs::OpenOptions with create_new(true) (and write/append/truncate as
appropriate) to open the file atomically and fail if it already exists,
replacing the exists() check and the std::fs::write call in that branch; keep
the spec.parents handling and use
OpenOptions::new().write(true).create_new(true) to write the bytes and propagate
errors via CoderError::from.
Comment on lines
+121
to
+132
| std::fs::write(&abs, bytes).map_err(CoderError::from)?; | ||
| apply_mode(&abs, &spec.mode)?; | ||
| Ok(bytes.len() as u64) | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| fn apply_mode(path: &Path, mode_str: &str) -> Result<(), CoderError> { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let mode = u32::from_str_radix(mode_str.trim_start_matches('0'), 8) | ||
| .map_err(|e| CoderError::BadInput(format!("bad mode {mode_str:?}: {e}")))?; | ||
| let perms = std::fs::Permissions::from_mode(mode & 0o777); | ||
| std::fs::set_permissions(path, perms).map_err(CoderError::from) |
There was a problem hiding this comment.
Validate mode before the write is committed.
A bad mode string—or a set_permissions failure—returns an error after the bytes are already on disk, so the caller sees a failure even though the file was created/overwritten. "0000" also currently fails because trim_start_matches('0') can leave an empty string. Parse/validate the mode first, then write/apply it so failed results stay side-effect free.
🤖 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 `@coder/src/functions/create_file.rs` around lines 121 - 132, Parse and
validate the mode string before writing the file so bad mode strings or parse
errors fail before any disk side-effect; specifically, in create_file
validate/convert spec.mode to a numeric u32 (handle "0000" by treating an empty
trimmed string as "0" or otherwise accept leading zeros) and return Err on parse
failure, then call std::fs::write and finally call apply_mode with the parsed
numeric mode (or adjust apply_mode to accept a u32 instead of re-parsing the
string) so set_permissions is only attempted after a known-good mode.
Comment on lines
+101
to
+112
| let entry_md = match e.metadata() { | ||
| Ok(m) => m, | ||
| Err(_) => continue, | ||
| }; | ||
| let abs_entry = e.path(); | ||
| all.push(DirEntry { | ||
| name, | ||
| kind: classify(&entry_md), | ||
| size: entry_md.len(), | ||
| mtime: unix_mtime(&entry_md), | ||
| non_accessible: resolver.is_non_accessible(&abs_entry), | ||
| }); |
There was a problem hiding this comment.
Fix symlink classification in list_folder
In coder/src/functions/list_folder.rs (lines 101-112; also 136-147), DirEntry::metadata() follows symlinks and fails for broken symlinks, so classify(&entry_md) uses the target type (and broken links are skipped via continue). Use e.file_type() (or e.symlink_metadata()) for kind, and only read target metadata separately if you still want target size/mtime.
🤖 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 `@coder/src/functions/list_folder.rs` around lines 101 - 112, The code
currently uses e.metadata() (which follows symlinks) to classify entries,
causing broken symlinks to be skipped and symlink kinds to reflect their
targets; in list_folder change the kind determination to use e.file_type() or
e.symlink_metadata() (e.g., call e.file_type() and pass that result into
classify or adjust classify to accept FileType) so symlinks are classified
correctly, and only call e.metadata() when you need target-specific size/mtime
(wrap that in its own match so broken symlinks don’t cause continue); update the
DirEntry construction (symbols: DirEntry, classify, unix_mtime,
resolver.is_non_accessible) to use file_type for kind and keep metadata-derived
size/mtime optional.
Comment on lines
+51
to
+67
| let md = std::fs::metadata(&abs)?; | ||
| if !md.is_file() { | ||
| return Err(CoderError::BadInput(format!( | ||
| "not a regular file: {}", | ||
| req.path | ||
| ))); | ||
| } | ||
| if md.len() > cfg.max_read_bytes { | ||
| return Err(CoderError::TooLarge(format!( | ||
| "{} is {} bytes; max_read_bytes is {}", | ||
| req.path, | ||
| md.len(), | ||
| cfg.max_read_bytes | ||
| ))); | ||
| } | ||
| let bytes = std::fs::read(&abs)?; | ||
| let (content, is_utf8) = match String::from_utf8(bytes.clone()) { |
There was a problem hiding this comment.
Enforce max_read_bytes on the bytes actually read.
Lines 51-66 trust the earlier metadata size, but the file can grow before Line 66 runs. That lets a racing writer bypass the configured cap and pull a larger blob into memory/response. Read through a capped handle, or at least fail on bytes.len() after the read.
🤖 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 `@coder/src/functions/read_file.rs` around lines 51 - 67, The current code
checks md.len() but then uses std::fs::read(&abs) which can race; replace the
unbounded read with a size-limited read or at minimum validate the actual bytes
length after reading. Open the file at abs and read via a capped reader
(std::io::Read::take using cfg.max_read_bytes + 1) or call std::fs::read and
then immediately check bytes.len() against cfg.max_read_bytes and return
CoderError::TooLarge (including req.path and sizes) before converting to String
in the String::from_utf8 branch; ensure you reference md, abs, bytes,
cfg.max_read_bytes and req.path in the fix.
Comment on lines
+165
to
+214
| if let Some(matcher) = &path_matcher { | ||
| if matcher.is_match(&rel) { | ||
| if path_matches.len() >= max_matches { | ||
| truncated = true; | ||
| } else { | ||
| path_matches.push(PathMatch { path: rel.clone() }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Some(matcher) = &content_matcher { | ||
| // Skip files larger than max_read_bytes during a search — we | ||
| // don't want to load multi-GB blobs into memory by accident. | ||
| if let Ok(md) = std::fs::metadata(abs) { | ||
| if md.len() > cfg.max_read_bytes { | ||
| continue; | ||
| } | ||
| } | ||
| let bytes = match std::fs::read(abs) { | ||
| Ok(b) => b, | ||
| Err(_) => continue, | ||
| }; | ||
| // Cheap binary heuristic: presence of any NUL byte. Skip | ||
| // binary files so the response stays human-readable. | ||
| if bytes.contains(&0) { | ||
| continue; | ||
| } | ||
| let text = String::from_utf8_lossy(&bytes); | ||
| for (line_idx, line) in text.lines().enumerate() { | ||
| let truncated_line = if line.len() > max_line_bytes { | ||
| &line[..max_line_bytes] | ||
| } else { | ||
| line | ||
| }; | ||
| if let Some(m) = matcher.find(truncated_line) { | ||
| if content_matches.len() >= max_matches { | ||
| truncated = true; | ||
| break; | ||
| } | ||
| content_matches.push(ContentMatch { | ||
| path: rel.clone(), | ||
| line: (line_idx as u32) + 1, | ||
| column: (m.start as u32) + 1, | ||
| text: truncated_line.to_string(), | ||
| }); | ||
| } | ||
| } | ||
| if truncated { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Don't let one capped list stop the other search mode.
truncated is shared across path and content matches. Once the path list hits max_matches, Line 212 breaks the outer walk the next time content search runs, so search_content=true can return an arbitrarily incomplete content_matches list even when that cap was never reached. Track truncation per list, or stop only the branch that hit its own cap.
🤖 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 `@coder/src/functions/search.rs` around lines 165 - 214, The code uses a single
shared truncated flag that both the path-matching branch and the
content-matching branch set, which causes one capped list (path_matches or
content_matches) to prematurely stop the entire walk and corrupt the other list;
introduce two separate flags (e.g., path_truncated and content_truncated) and
update the path-matcher block (where PathMatch is pushed) to set path_truncated
when path_matches.len() >= max_matches without affecting content processing, and
update the content-matcher block to set and check content_truncated (instead of
truncated) when content_matches reaches max_matches and only break/stop the
content loop or outer walk based on content_truncated; replace uses of truncated
in loop-break logic with the appropriate per-list flag and keep the existing
symbols path_matcher, content_matcher, path_matches, content_matches, and the
truncation checks aligned to each list.
Comment on lines
+180
to
+205
| let ft = e.file_type().ok(); | ||
| if ft.as_ref().is_some_and(|t| t.is_dir()) { | ||
| let sub = walk_dir( | ||
| resolver, | ||
| &child_abs, | ||
| child_rel, | ||
| depth + 1, | ||
| max_depth, | ||
| per_folder_limit, | ||
| )?; | ||
| children.push(sub); | ||
| } else { | ||
| let cmd = match e.metadata() { | ||
| Ok(m) => m, | ||
| Err(_) => continue, | ||
| }; | ||
| children.push(TreeNode { | ||
| name: e.file_name().to_string_lossy().into_owned(), | ||
| path: child_rel, | ||
| kind: classify(&cmd), | ||
| size: cmd.len(), | ||
| mtime: unix_mtime(&cmd), | ||
| non_accessible: resolver.is_non_accessible(&child_abs), | ||
| children: None, | ||
| truncated: None, | ||
| }); |
There was a problem hiding this comment.
Preserve Symlink nodes in tree.rs by avoiding e.metadata() for classification
coder/src/functions/tree.rs(around 180-205; also 220-231) computeskind: classify(&cmd)wherecmdcomes frome.metadata(), which follows symlinks—so a symlink-to-dir/file is classified asDir/File(and won’t produce children due to the earliere.file_type().is_dir()recursion gate).- Dangling symlinks get skipped because
e.metadata()fails and the codecontinues.
Usee.file_type()and/ore.symlink_metadata()to deriveNodeKind::Symlink(and basesize/mtimeon that same metadata if needed) instead ofe.metadata().
🤖 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 `@coder/src/functions/tree.rs` around lines 180 - 205, The code is incorrectly
following symlinks by calling e.metadata() for classification and skipping
dangling symlinks on metadata errors; update the logic in the walk_dir/TreeNode
construction to use e.file_type() and e.symlink_metadata() instead of
e.metadata() so you can detect and preserve NodeKind::Symlink (and base
size/mtime on the symlink's own metadata when available), avoid continuing on
e.metadata() errors for symlinks, and apply the same change at the other
occurrence around the 220-231 block; reference functions/values: walk_dir,
TreeNode, classify, e.file_type(), e.symlink_metadata(), and
resolver.is_non_accessible to implement this behavior.
Comment on lines
+376
to
+395
| /// Write atomically via sibling temp file + rename. | ||
| fn atomic_write(target: &Path, bytes: &[u8]) -> Result<(), CoderError> { | ||
| let parent = target | ||
| .parent() | ||
| .ok_or_else(|| CoderError::Io(format!("no parent for {}", target.display())))?; | ||
| let mut tmp = std::ffi::OsString::from(target.file_name().unwrap_or_default()); | ||
| tmp.push(".coder-tmp-"); | ||
| tmp.push(format!("{}", std::process::id())); | ||
| tmp.push("-"); | ||
| tmp.push(format!("{}", rand_suffix())); | ||
| let tmp_path = parent.join(tmp); | ||
| std::fs::write(&tmp_path, bytes).map_err(|e| { | ||
| let _ = std::fs::remove_file(&tmp_path); | ||
| CoderError::Io(format!("tmp write: {e}")) | ||
| })?; | ||
| std::fs::rename(&tmp_path, target).map_err(|e| { | ||
| let _ = std::fs::remove_file(&tmp_path); | ||
| CoderError::Io(format!("rename: {e}")) | ||
| })?; | ||
| Ok(()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate atomic_write and relevant call sites
rg -n "fn atomic_write|atomic_write\(" coder/src/functions/update_file.rs coder/src/functions -S
# Show surrounding code where atomic_write is defined and used
sed -n '330,460p' coder/src/functions/update_file.rs
# Search for any follow-up chmod/fset permissions after atomic_write
rg -n "chmod|set_permissions|PermissionsExt|fchmod|mode\\b" coder/src/functions/update_file.rs -S
# Check whether there are tests covering permission preservation for updates
rg -n "permission|mode\\b|executable|chmod" coder/src -SRepository: iii-hq/workers
Length of output: 4075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect call site around atomic_write usage
sed -n '120,240p' coder/src/functions/update_file.rs
# 2) Search for any permission copying / chmod after writing temp file
rg -n "set_permissions|permissions\\(|metadata\\(|mode\\b|chmod\\b|PermissionsExt|fchmod" coder/src/functions/update_file.rs coder/src/functions -S
# 3) Check for tests mentioning permissions/mode/executable
rg -n "permission|permissions|mode\\b|chmod|executable" coder/src/functions/update_file.rs coder/src -S
# 4) Show remainder of tests module (up to end of file or at least a good range)
sed -n '240,520p' coder/src/functions/update_file.rsRepository: iii-hq/workers
Length of output: 17976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm atomic_write only used in update_file.rs (or find other uses)
rg -n "atomic_write\\(" coder/src -S
# 2) Look for any chmod / mode / permissions logic inside update_file.rs
rg -n "chmod|set_permissions|PermissionsExt|permissions\\(|metadata\\(|mode\\b|executable|file mode" coder/src/functions/update_file.rs -S
# 3) Search tests for permission/mode-related assertions
rg -n "permission|permissions|mode\\b|chmod|executable" coder/src/functions/update_file.rs -S
# 4) Show the remainder of update_file.rs tests section end (small tail)
tail -n 120 coder/src/functions/update_file.rsRepository: iii-hq/workers
Length of output: 289
🏁 Script executed:
#!/bin/bash
set +euo pipefail
# 1) Find all uses/definition of atomic_write
rg -n "atomic_write\\(" coder/src/functions/update_file.rs coder/src -S
# 2) Look for any permission/mode copying logic inside update_file.rs
rg -n "chmod|set_permissions|PermissionsExt|permissions\\(|metadata\\(|mode\\b|executable" coder/src/functions/update_file.rs -S
# 3) Search tests for permission/mode-related assertions in update_file.rs
rg -n "permission|permissions|mode\\b|chmod|executable" coder/src/functions/update_file.rs -S
# 4) Show end of file (tests section) to ensure nothing about permissions is omitted
tail -n 160 coder/src/functions/update_file.rsRepository: iii-hq/workers
Length of output: 5573
Preserve target file permissions when doing atomic replacement.
atomic_write (lines 376-395) creates a new temp file via std::fs::write and then renames it over the existing target, so the final file’s mode comes from the temp file/umask, not the original target. Copy std::fs::metadata(target)?.permissions() onto tmp_path (e.g., std::fs::set_permissions) before the rename.
🤖 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 `@coder/src/functions/update_file.rs` around lines 376 - 395, atomic_write
currently writes a temp file and renames it over target but loses the original
file mode; before renaming in atomic_write, obtain the target's permissions via
std::fs::metadata(target)?.permissions() (guarding for target's absence) and
apply them to the tmp_path with std::fs::set_permissions(&tmp_path, perms).
Ensure you handle and propagate errors similarly to the existing write/rename
error handling (clean up tmp file on failure) so the temp file inherits the
target's mode prior to std::fs::rename.
Comment on lines
+63
to
+89
| pub fn resolve(&self, rel: &str) -> Result<PathBuf, CoderError> { | ||
| let rel_path = Path::new(rel); | ||
| if rel_path.is_absolute() { | ||
| return Err(CoderError::BadInput(format!( | ||
| "path must be relative to base_path: {rel}" | ||
| ))); | ||
| } | ||
| let joined = self.base_root_canon.join(rel_path); | ||
| let canon = canonicalize_with_fallback(&joined).map_err(|e| { | ||
| let msg = e.to_string(); | ||
| if msg.contains("dangling symlink in path") { | ||
| CoderError::OutsideBase(format!("{rel}: {msg}")) | ||
| } else if e.kind() == std::io::ErrorKind::InvalidInput | ||
| || e.kind() == std::io::ErrorKind::NotFound | ||
| { | ||
| CoderError::NotFoundOrDenied(format!("{rel}: {msg}")) | ||
| } else { | ||
| CoderError::Io(format!("canonicalize {rel}: {e}")) | ||
| } | ||
| })?; | ||
| if !canon.starts_with(&self.base_root_canon) { | ||
| return Err(CoderError::OutsideBase(format!( | ||
| "path escapes base_path: {rel}" | ||
| ))); | ||
| } | ||
| Ok(canon) | ||
| } |
There was a problem hiding this comment.
Normalize relative components before fallback canonicalization to prevent symlink-escape bypass.
resolve() currently feeds raw relative paths into canonicalize_with_fallback(). Inputs like missing/../escape/passwd can bypass symlink escape checks when escape is a symlink out of base_path, because the symlink probe runs on the unnormalized suffix path. This breaks the jail boundary.
Suggested fix
pub fn resolve(&self, rel: &str) -> Result<PathBuf, CoderError> {
let rel_path = Path::new(rel);
if rel_path.is_absolute() {
return Err(CoderError::BadInput(format!(
"path must be relative to base_path: {rel}"
)));
}
- let joined = self.base_root_canon.join(rel_path);
+ // Collapse "."/".." first so fallback checks the real lexical target.
+ let mut normalized_rel = PathBuf::new();
+ for c in rel_path.components() {
+ match c {
+ Component::CurDir => {}
+ Component::Normal(seg) => normalized_rel.push(seg),
+ Component::ParentDir => {
+ if !normalized_rel.pop() {
+ return Err(CoderError::OutsideBase(format!(
+ "path escapes base_path: {rel}"
+ )));
+ }
+ }
+ Component::RootDir | Component::Prefix(_) => {
+ return Err(CoderError::BadInput(format!(
+ "path must be relative to base_path: {rel}"
+ )));
+ }
+ }
+ }
+ let joined = self.base_root_canon.join(&normalized_rel);
let canon = canonicalize_with_fallback(&joined).map_err(|e| {
let msg = e.to_string();
if msg.contains("dangling symlink in path") {
CoderError::OutsideBase(format!("{rel}: {msg}"))Also applies to: 127-150
🤖 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 `@coder/src/path/mod.rs` around lines 63 - 89, resolve() currently calls
canonicalize_with_fallback() on the raw joined path which allows inputs with
`..` to bypass the symlink-escape check; before canonicalization, lexically
normalize the relative components (collapse "." and ".." without following
symlinks) by walking joined.components() into a new PathBuf: skip "."
components, pop on ".." and if a pop would escape the base_root_canon (i.e., you
pop past the joined prefix) return an appropriate CoderError (e.g., OutsideBase
or BadInput), otherwise push normal components; then call
canonicalize_with_fallback() on that normalized PathBuf (instead of joined) and
keep the existing canonical-starts_with base_root_canon check. Ensure you update
references to joined -> normalized when calling canonicalize_with_fallback and
in subsequent checks.
Comment on lines
+55
to
+60
| pub async fn get_or_init() -> Option<Arc<III>> { | ||
| ENGINE | ||
| .get_or_init(|| async { | ||
| let iii = try_connect_raw().await?; | ||
| crate::common::workers::register_all(&iii).await.ok()?; | ||
| Some(iii) |
There was a problem hiding this comment.
Fail fast when worker registration fails (don’t soft-skip).
On Line 59, .ok()? collapses registration errors into None, so engine-tagged assertions can silently skip and pass even when coder registration is broken. This should be a hard test failure.
Suggested fix
pub async fn get_or_init() -> Option<Arc<III>> {
ENGINE
.get_or_init(|| async {
let iii = try_connect_raw().await?;
- crate::common::workers::register_all(&iii).await.ok()?;
+ if let Err(e) = crate::common::workers::register_all(&iii).await {
+ panic!("failed to register coder worker in BDD harness: {e}");
+ }
Some(iii)
})
.await
.clone()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn get_or_init() -> Option<Arc<III>> { | |
| ENGINE | |
| .get_or_init(|| async { | |
| let iii = try_connect_raw().await?; | |
| crate::common::workers::register_all(&iii).await.ok()?; | |
| Some(iii) | |
| pub async fn get_or_init() -> Option<Arc<III>> { | |
| ENGINE | |
| .get_or_init(|| async { | |
| let iii = try_connect_raw().await?; | |
| if let Err(e) = crate::common::workers::register_all(&iii).await { | |
| panic!("failed to register coder worker in BDD harness: {e}"); | |
| } | |
| Some(iii) | |
| }) | |
| .await | |
| .clone() | |
| } |
🤖 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 `@coder/tests/common/engine.rs` around lines 55 - 60, The worker registration
error is being swallowed by .ok()? on the register_all(&iii).await call; change
that to propagate the error instead of converting it to None so failures surface
as hard test errors — replace register_all(&iii).await.ok()? with a propagation
(e.g., register_all(&iii).await? or propagate the Result from register_all
directly) in the get_or_init closure so registration failures fail fast (check
types around get_or_init, try_connect_raw, and register_all to adjust the
return/Result handling as needed).
Comment on lines
+35
to
+76
| async fn boot() -> Option<Harness> { | ||
| let iii_bin = which::which("iii").ok()?; | ||
|
|
||
| let mut iii = Command::new(&iii_bin) | ||
| .arg("--use-default-config") | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; | ||
|
|
||
| sleep(Duration::from_millis(800)).await; | ||
|
|
||
| let base = tempfile::tempdir().ok()?; | ||
| let cfg_path = base.path().join("coder-config.yaml"); | ||
| let yaml = format!( | ||
| "base_path: {}\nnon_accessible_globs:\n - \"**/.env\"\n", | ||
| base.path().display() | ||
| ); | ||
| std::fs::write(&cfg_path, yaml).ok()?; | ||
|
|
||
| let worker_bin = env!("CARGO_BIN_EXE_coder"); | ||
| let worker = match Command::new(worker_bin) | ||
| .args([ | ||
| "--url", | ||
| ENGINE_WS, | ||
| "--config", | ||
| cfg_path.to_str().expect("utf-8 cfg path"), | ||
| ]) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| { | ||
| Ok(w) => w, | ||
| Err(_) => { | ||
| let _ = iii.kill(); | ||
| let _ = iii.wait(); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| Some(Harness { iii, worker, base }) | ||
| } |
There was a problem hiding this comment.
Do not treat post-discovery boot failures as “skip”.
Line 47 and Line 53 can return None after iii has already started, and Line 119 then reports “iii not on PATH”. That masks real failures and can leave an orphan engine process (port contention/flaky follow-up tests).
Suggested direction
-async fn boot() -> Option<Harness> {
+enum BootOutcome {
+ MissingEngine,
+ Ready(Harness),
+}
+
+async fn boot() -> Result<BootOutcome, String> {
- let iii_bin = which::which("iii").ok()?;
+ let iii_bin = match which::which("iii") {
+ Ok(bin) => bin,
+ Err(_) => return Ok(BootOutcome::MissingEngine),
+ };
let mut iii = Command::new(&iii_bin)
// ...
- .spawn()
- .ok()?;
+ .spawn()
+ .map_err(|e| format!("spawn iii: {e}"))?;
- let base = tempfile::tempdir().ok()?;
+ let base = tempfile::tempdir().map_err(|e| format!("tempdir: {e}"))?;
// ...
- std::fs::write(&cfg_path, yaml).ok()?;
+ std::fs::write(&cfg_path, yaml).map_err(|e| format!("write config: {e}"))?;
- Some(Harness { iii, worker, base })
+ Ok(BootOutcome::Ready(Harness { iii, worker, base }))
}Then fail the test on Err(_), and only skip on BootOutcome::MissingEngine.
Also applies to: 119-122
🤖 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 `@coder/tests/integration.rs` around lines 35 - 76, boot() currently conflates
"iii missing" and post-discovery failures by returning None after iii has
started (variables iii and worker), which causes later tests to be skipped and
can leave orphaned processes; change boot() to return a Result<Harness,
BootOutcome> (or similar) so that which::which("iii") still maps to
Err(BootOutcome::MissingEngine) (skip), but any failures after spawning iii
(e.g., Command::spawn() for worker failing) return an Err indicating a real boot
failure; ensure you properly kill and wait on iii before returning that error
(use the existing iii.kill()/iii.wait() cleanup) and update call sites to only
skip on BootOutcome::MissingEngine.
Contributor
skill-check — worker0 verified, 13 skipped (no docs/).
Three for three. Nicely done. |
6 tasks
This was referenced Jun 24, 2026
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
coder
A path-jailed code worker for iii agents.
coder::*lets agents read,search, edit, create, and delete files inside a single configured
base_path— without ever escaping it via.., absolute paths, orsymlinks. A glob-based
non_accessiblelist keeps sensitive files(
.env,*.pem, anything undersecrets/) visible to directorylistings but unreadable and unwritable.
Install
iii worker addfetches the binary, writes a config block into~/.iii/config.yaml, and the engine starts the worker on the nextiii start.Quickstart
Functions
coder::read-filemax_read_bytes).coder::searchbase_path.coder::update-fileinsert/remove/update_lines/ regexreplaceops across one or more files. Line ops bottom-up; atomic per file.coder::create-fileoverwriteandparentsflags.coder::delete-filerecursive: truerequired for non-empty dirs.coder::list-foldercoder::treemax_depthandper_folder_limit.coder::update-filesemanticsLine ops (
insert,remove,update_lines) use 1-based inclusiveline numbers and are applied bottom-up (highest affected line
first), so each op still references the original line numbers from the
caller's perspective. Overlapping line ops are rejected (
C210).Regex
replaceops run after line ops on the full file body. Thewhole batch is committed via a sibling temp file + rename, so a failure
mid-write leaves the original file intact.
{ "files": [{ "path": "schema.sql", "ops": [ { "op": "insert", "at_line": 1, "content": "-- header\n-- v2" }, { "op": "remove", "from_line": 5, "to_line": 12 }, { "op": "update_lines", "from_line": 30, "to_line": 30, "content": "PRIMARY KEY (id)" }, { "op": "replace", "pattern": "OLD_", "replacement": "NEW_" } ] }] }Error codes
All errors return as JSON strings of the form
{"code":"C2xx","message":"..."}.C210C211non_accessible_globsentryC213max_read_bytesormax_write_bytesC215base_pathlexically or through a symlinkC216C217coder::create-filesaw an existing file withoverwrite=falseConfiguration
non_accessible_globsuses the same syntax as theglobsetcrate (so**/,*,?, character classes, …). Matching is done against therelative path from
base_path, so**/.envblocks.env,a/.env, anda/b/.env.Security boundary
base_pathis canonicalised at startup; the worker refuses to startif it can't be reached.
base_path; absolute pathsreturn
C210rather than being silently re-jailed...and symlinks are resolved against the longest existing ancestorand rejected if they leave
base_path(C215). Dangling symlinksin the tail are also rejected because the kernel would otherwise
follow them on the next syscall.
glob hides the file from
coder::read-file,coder::update-file,coder::create-file,coder::delete-file, and fromcoder::search's content/path matches.coder::delete-filerefuses to descend through a subtreethat contains a non-accessible entry rather than removing it.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores