Conversation
Overhauls the test suite by separating web server tests into dedicated scripts. These new scripts start a server process and perform HTTP requests to validate functionality. The main integration test runner is updated to skip these specialized tests and now includes timeouts to prevent individual test programs from hanging. Test reporting is also improved to show pass, fail, skip, and timeout statuses. Fixes a parser bug to correctly handle expressions in named function arguments that are separated by the 'and' keyword.
Simplifies the 'faulty' action by removing intermediate variables to directly trigger the division-by-zero error. Adds parentheses to a compound conditional check to improve clarity and ensure correct logical grouping.
Replaces the previously commented-out placeholder for pattern matching tests with a functional and comprehensive test suite. The new tests use the working `create pattern` syntax to validate various scenarios, including exact digit counts, word matching, and numeric ranges.
Adds logic to delete temporary files created during the async I/O tests. This finalizes the test suite by ensuring it leaves the environment clean, resolving a previous TODO.
Replaces the runtime check for `sync_all()` failures on Windows with compile-time conditional compilation. On Windows, the result of `sync_all()` is now ignored to prevent failures in concurrent access scenarios, as `flush()` provides sufficient durability. This also removes the previous warning message. On non-Windows platforms, `sync_all()` is still enforced to guarantee data is fully written to disk. This change simplifies the code and provides a cleaner approach to platform-specific behavior.
Adds integration tests to verify the auto-call behavior of zero-argument actions, specifically focusing on error handling. This addresses a bug where assigning a faulty action to a variable would not execute the action, preventing errors from being caught by a try-catch block. The tests confirm that such actions are now correctly invoked, and any resulting errors are properly propagated and caught. A second test also validates the auto-call behavior for successful actions.
Automatically executes a user-defined function when it is referenced if it accepts no arguments. This provides a more convenient syntax by not requiring empty parentheses for simple function calls. Functions that require arguments will return the function object itself, preserving existing behavior.
Implements the modulo operator (`%`) for arithmetic operations. This change integrates the new operator throughout the entire language pipeline, including the lexer, parser, type checker, and interpreter. The implementation includes error handling for modulo-by-zero scenarios. An example program is updated to use the new operator, demonstrating its utility for simplifying expressions like even/odd number checks.
Introduces the modulo operator (%) for remainder calculations. A new test suite is added to verify basic operations, its use in even/odd checks, and correct error handling for division by zero. Additionally, clarifies that `exit loop` now breaks out of all nested loops, and updates the corresponding test to reflect this behavior.
|
Caution Review failedThe pull request is closed. 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. WalkthroughThis PR adds modulo (%) support across lexer, parser, AST, typechecker, interpreter, and fixer; implements Windows-aware file sync suppression; enables auto-calling zero-arg user-defined functions when referenced as variables; expands Nexus tests; adds multiple unit/integration tests; and introduces test runners and web-server test orchestration with timeouts, skipping, and reporting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Test Runner
participant Proc as WFL Server Process
participant HTTP as HTTP Client/Layer
participant FS as File System
Runner->>Proc: Start wfl.exe with TestFile & Port (background)
Note over Proc: Server initializes and reads TestFile
loop Poll (every 500ms up to Timeout)
Runner->>HTTP: GET http://localhost:Port/
HTTP-->>Runner: Connection refused / timeout
end
HTTP->>Proc: GET request accepted
Proc->>FS: Read test configuration / content
FS-->>Proc: File data
Proc-->>HTTP: HTTP 200 + body
HTTP-->>Runner: Response received
Runner->>Runner: Validate response contains expected string
alt Match
Runner->>Runner: Record PASS
else No match
Runner->>Runner: Record FAIL
end
Runner->>Proc: Terminate process (cleanup)
Proc-->>Runner: Process exited
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 PR Review Complete ✅I've conducted a thorough review of this substantial PR that introduces language enhancements and testing improvements. Here's my detailed analysis: Review Checklist:
🎯 Overall Assessment: EXCELLENTThis PR demonstrates high-quality implementation with thoughtful design decisions, comprehensive testing, and good adherence to existing codebase patterns. ✨ StrengthsLanguage Enhancements
Testing Infrastructure
⚡ Performance Considerations
🔒 Security Assessment
🧪 Test CoverageExcellent - The PR adds comprehensive test coverage:
🔧 Code Quality
🔍 Minor Observations
✅ RecommendationAPPROVE - This PR is well-implemented, thoroughly tested, and ready for merge. It:
The modulo operator and auto-call functionality are valuable language improvements, and the testing infrastructure changes will benefit long-term development quality. |
Applies standard Rust formatting across interpreter and test files to improve code style consistency. This commit contains only stylistic changes and does not alter program logic.
|
Claude finished @logbie's task —— View job PR Review: Language Features and Testing InfrastructureI've completed a comprehensive review of this substantial PR that introduces multiple language enhancements and significantly improves the testing infrastructure. Here's my detailed feedback: Review Checklist:
✅ StrengthsLanguage Enhancements
Testing Infrastructure
|
![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".
| #[cfg(windows)] | ||
| { | ||
| // On Windows, skip sync_all due to filesystem limitations with concurrent access | ||
| // flush() provides adequate durability by ensuring data reaches OS buffers | ||
| let _ = file_clone.sync_all().await; |
There was a problem hiding this comment.
On Windows we now swallow every error from sync_all() by assigning the result to _ and immediately returning Ok(()). That means real I/O failures (disk full, access errors other than the previously handled PermissionDenied, etc.) no longer surface; the write/close path reports success even though data may not have been persisted. This is a regression from the prior logic that only suppressed the known spurious PermissionDenied but still bubbled other errors. The same silent failure pattern was introduced in the corresponding close/append paths, so Windows users can get false positives and data loss when file syncing actually fails.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/interpreter/mod.rs (1)
4758-4801: Auto-calling zero-arg user functions inExpression::Variablebreaksfoo()callsThe new branch auto-invoking
Value::Functionwhenfunc.params.is_empty()causes a critical failure inExpression::FunctionCall:
- When
foo()is parsed,FunctionCallevaluates its callee viaself.evaluate_expression(function, ...)on theExpression::Variable("foo", ..)node.- The
Variablebranch now executesself.call_function(func, vec![], ...).awaitfor zero-arg functions, returning the function's result (e.g.,Value::String) instead of the function object.- Back in
FunctionCall, the match onfunction_valexpectsValue::FunctionorValue::NativeFunction. Sincefunction_valis now the result value, it hits the catch-all arm and errors:"Cannot call {type}".This breaks all zero-argument function calls like
foo(). The special-case handling inStatement::ExpressionStatement(which detects bare action names beforeevaluate_expression) cannot compensate becauseFunctionCallalways usesevaluate_expression.Fix: Bypass auto-call when resolving the callee in
FunctionCall. One approach is to fetch the function value directly forExpression::Variabletargets without triggering auto-call:- } => { - let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?; + } => { + let function_val = match function.as_ref() { + // Bypass auto-call for call targets; fetch value directly + Expression::Variable(name, _, _) => env + .borrow() + .get(name) + .ok_or_else(|| RuntimeError::new( + format!("Undefined variable '{name}'"), + *line, + *column, + ))?, + _ => self.evaluate_expression(function, Rc::clone(&env)).await?, + };
🧹 Nitpick comments (9)
Nexus/nexus.wfl (2)
468-474: Good cleanup practice, consider optional error handling for robustness.The file deletion correctly cleans up temporary test artifacts. For added robustness, you might wrap deletions in error handling to ensure the final log messages are written even if deletion fails (e.g., file locked by another process).
// Clean up temporary files created during async I/O tests -delete file at "temp1.txt" -delete file at "temp2.txt" +try: + delete file at "temp1.txt" + delete file at "temp2.txt" +catch: + log_message with "Warning: Could not delete temporary files" +end tryAs per coding guidelines, WFL programs should use comprehensive try/when/otherwise error handling. However, since these files were just created in the same script run, the current approach is acceptable for a test script.
425-428: Minor: Section numbering inconsistency.The comment at line 426 says "6. Asynchronous I/O and Concurrency Tests (formerly section 7)" but line 356 already declares section 6 as "Pattern Matching Tests". Consider updating this to section 7.
/////////////////////////////////////////////////////////////////////////// -// 6. Asynchronous I/O and Concurrency Tests (formerly section 7) +// 7. Asynchronous I/O and Concurrency Tests ///////////////////////////////////////////////////////////////////////////scripts/run_web_tests.sh (2)
77-77: Declare and assign separately to avoid masking return values.Per ShellCheck SC2155, combining
localwith command substitution can mask the exit status ofbasename.Apply this diff:
- local test_name=$(basename "$test_file") + local test_name + test_name=$(basename "$test_file")
144-145:grep -Pmay not be portable across all Unix systems.The
-P(Perl regex) flag is not available on macOS's default grep. Consider usinggrep -Ewith capture viasedorawkfor broader compatibility.Apply this diff for better portability:
# Read the file to find the port - port=$(grep -oP 'port\s+\K\d+' "TestPrograms/web_server_test.wfl" 2>/dev/null || echo "") + port=$(grep -E 'port\s+[0-9]+' "TestPrograms/web_server_test.wfl" 2>/dev/null | grep -oE '[0-9]+' | head -1 || echo "")tests/zero_arg_action_error_propagation.rs (1)
11-30: Extract common test setup logic to reduce duplication.Both test functions duplicate the binary path resolution and verification logic. Consider extracting this into a helper function or using a test fixture.
fn get_wfl_binary_path() -> std::path::PathBuf { let wfl_binary = if cfg!(target_os = "windows") { "target/release/wfl.exe" } else { "target/release/wfl" }; let binary_path = env::current_dir() .unwrap() .join(wfl_binary); if !binary_path.exists() { panic!( "WFL binary not found at {:?}. Run 'cargo build --release' first.", binary_path ); } binary_path }Then use it in tests:
#[test] fn test_zero_arg_action_error_propagation() { let binary_path = get_wfl_binary_path(); // ... rest of test }Also applies to: 95-111
tests/modulo_operator_test.rs (1)
11-18: Extract common test setup logic to reduce duplication.All three test functions duplicate the binary path resolution and verification logic. Consider extracting this into a helper function to improve maintainability.
fn get_wfl_binary_path() -> std::path::PathBuf { let wfl_binary = if cfg!(target_os = "windows") { "target/release/wfl.exe" } else { "target/release/wfl" }; let binary_path = env::current_dir().unwrap().join(wfl_binary); assert!( binary_path.exists(), "WFL binary not found at {:?}. Run 'cargo build --release' first.", binary_path ); binary_path }Then simplify each test:
#[test] fn test_modulo_operator_basic() { let binary_path = get_wfl_binary_path(); // ... rest of test }Also applies to: 76-83, 125-132
src/parser/mod.rs (1)
5460-5492: Argument parsing viaparse_binary_expression(1)fixes theandseparator issue but may block other low‑precedence operators as argumentsUsing
parse_binary_expression(1)forarg_valueensures that:
- Arithmetic (
+,-,times,divided by,%) still parse inside a single argument.- All precedence‑0 operators (including
and) are not parsed into the argument, soandcan safely act as an argument separator, matching the intended grammar (e.g.foo with x and y and z).However, this also means precedence‑0 operators like
or,equals, comparison operators (is greater than,is less than, etc.), andcontainswill not be parsed inside a single argument expression; they’ll be left for outer parsing. If the language is expected to support calls likecheck with value is greater than limitorfn with cond1 or cond2as a single argument, this change may be surprising or a silent behavior change.If the intent is to special‑case only
andas an argument separator, consider (in a follow‑up) either:
- A dedicated
parse_argument_expressionthat treatsandas a hard terminator but still allows other precedence‑0 operators, or- Additional tests plus documentation that boolean/comparison expressions are not supported directly inside argument lists without extra structuring.
Please:
- Confirm via tests whether boolean/comparison expressions as arguments are (or should be) supported.
- If they should be, we can sketch a small refactor to limit the special behavior to
andonly.src/interpreter/mod.rs (2)
491-520: Consolidate platform-specificsync_alllogic and address rustfmt failuresThe new platform-specific fsync blocks in
write_file,close_file, andappend_filelook logically sound (flush first, thensync_allon Unix, ignore errors on Windows), but there are a couple of points worth tightening:
- On Windows you still call
sync_all()but ignore its result, while the comment says “skipsync_all”. Either actually skip the call or adjust the comment to match the behavior so future readers aren’t misled.- The cfg-gated
flush/sync_allpattern is duplicated in three places; consider extracting a small helper likeasync fn flush_and_sync(file: &mut File) -> Result<(), String>with#[cfg]inside, to centralize the behavior and error messages.- CI is flagging rustfmt issues precisely on these blocks (lines 500, 543, 581). Please run
cargo fmt --allso formatting stays consistent with the rest of the file.Also applies to: 540-559, 575-595
4818-4850: Modulo operator wiring and helper look correct; consider semantics & testsHooking
Operator::Modulothroughself.modulo(...)and the newfn modulomirrors thedivideimplementation nicely:
- Only numeric–numeric operands are accepted; everything else produces a clear runtime error.
- Division-by-zero equivalent (
b == 0.0) is handled explicitly.- You guard against non-finite results (
!result.is_finite()), which matches the robustness ofdivide.Two non-blocking suggestions:
- Clarify/test behavior for negative operands (e.g.,
-5 % 2,5 % -2,-5 % -2) and ensure it matches the language spec you want. Right now you are relying on Rust’s%forf64; if you expect “mathematical modulo” rather than “remainder” semantics, you may wantrem_euclid-style behavior instead.- CI reports a rustfmt failure near this region (around line 5815); after finalizing the implementation, please run
cargo fmt --allso the new helper and its match arm conform to the repo’s formatting rules.Also applies to: 5782-5823
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.claude/settings.local.json(1 hunks)Nexus/nexus.wfl(6 hunks)scripts/run_integration_tests.ps1(2 hunks)scripts/run_integration_tests.sh(1 hunks)scripts/run_web_tests.ps1(1 hunks)scripts/run_web_tests.sh(1 hunks)src/fixer/mod.rs(1 hunks)src/interpreter/mod.rs(6 hunks)src/lexer/token.rs(1 hunks)src/parser/ast.rs(1 hunks)src/parser/mod.rs(4 hunks)src/typechecker/mod.rs(1 hunks)tests/modulo_operator_test.rs(1 hunks)tests/zero_arg_action_error_propagation.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.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 codeUse Rust edition 2024 for all Rust source files
Files:
src/lexer/token.rssrc/typechecker/mod.rstests/zero_arg_action_error_propagation.rssrc/parser/mod.rssrc/parser/ast.rstests/modulo_operator_test.rssrc/fixer/mod.rssrc/interpreter/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Provide component documentation for all major modules in Rust source files
Implement comprehensive error diagnostics using codespan-reporting
Files:
src/lexer/token.rssrc/typechecker/mod.rssrc/parser/mod.rssrc/parser/ast.rssrc/fixer/mod.rssrc/interpreter/mod.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/zero_arg_action_error_propagation.rstests/modulo_operator_test.rs
src/parser/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Update bytecode when modifying parser in Rust source code
Files:
src/parser/mod.rssrc/parser/ast.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/modulo_operator_test.rs
src/interpreter/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Apply security sanitization to subprocess execution in Rust implementation
Files:
src/interpreter/mod.rs
**/*.wfl
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.wfl: Use natural language syntax in WFL programs:store name as "value",check if x is greater than 5
Use comprehensive try/when/otherwise error handling in WFL programs
Utilize async/await in WFL programs for concurrent operations
Use containers (classes) in WFL for object-oriented programming when appropriate
Files:
Nexus/nexus.wfl
🧠 Learnings (15)
📚 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
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to src/interpreter/**/*.rs : Apply security sanitization to subprocess execution in Rust implementation
Applied to files:
.claude/settings.local.jsonsrc/interpreter/mod.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to TestPrograms/**/*.wfl : All TestPrograms/*.wfl files MUST pass after any change
Applied to files:
scripts/run_integration_tests.ps1tests/zero_arg_action_error_propagation.rsscripts/run_web_tests.shscripts/run_web_tests.ps1Nexus/nexus.wflscripts/run_integration_tests.sh
📚 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:
scripts/run_integration_tests.ps1scripts/run_web_tests.shscripts/run_integration_tests.sh
📚 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:
scripts/run_integration_tests.ps1scripts/run_web_tests.shscripts/run_web_tests.ps1scripts/run_integration_tests.sh
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Use provided scripts for integration tests: `scripts/run_integration_tests.ps1` or `scripts/run_integration_tests.sh`
Applied to files:
scripts/run_integration_tests.ps1scripts/run_web_tests.shscripts/run_web_tests.ps1scripts/run_integration_tests.sh
📚 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:
scripts/run_integration_tests.ps1scripts/run_integration_tests.sh
📚 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/zero_arg_action_error_propagation.rstests/modulo_operator_test.rsscripts/run_web_tests.sh
📚 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:
tests/zero_arg_action_error_propagation.rsNexus/nexus.wfl
📚 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:
tests/zero_arg_action_error_propagation.rstests/modulo_operator_test.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to src/parser/**/*.rs : Update bytecode when modifying parser in Rust source code
Applied to files:
src/parser/mod.rssrc/interpreter/mod.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Always run `cargo build --release` before integration tests
Applied to files:
scripts/run_web_tests.sh
📚 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:
scripts/run_web_tests.sh
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Confirm tests fail before writing implementation
Applied to files:
Nexus/nexus.wfl
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Run ALL TestPrograms after changes to verify backward compatibility
Applied to files:
scripts/run_integration_tests.sh
🧬 Code graph analysis (1)
src/interpreter/mod.rs (2)
src/interpreter/error.rs (1)
new(25-32)src/interpreter/value.rs (1)
type_name(158-180)
🪛 GitHub Actions: CI
tests/zero_arg_action_error_propagation.rs
[error] 1-1: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 4-4: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 18-18: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
tests/modulo_operator_test.rs
[error] 1-1: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 4-4: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 15-15: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 66-66: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 116-116: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
[error] 158-158: rustfmt formatting check failed in test file. Run 'cargo fmt' to fix.
src/interpreter/mod.rs
[error] 500-500: rustfmt formatting check failed in file due to formatting differences. Run 'cargo fmt' to fix.
[error] 543-543: rustfmt formatting check failed in file due to formatting differences. Run 'cargo fmt' to fix.
[error] 581-581: rustfmt formatting check failed in file due to formatting differences. Run 'cargo fmt' to fix.
[error] 5815-5815: rustfmt formatting check failed in file due to formatting differences. Run 'cargo fmt' to fix.
🪛 Shellcheck (0.11.0)
scripts/run_web_tests.sh
[warning] 77-77: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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 (25)
.claude/settings.local.json (1)
34-35: LGTM!The new permission for
./target/release/wfl.exe:*is consistent with existing bash patterns in the allow list and aligns with the testing infrastructure improvements mentioned in this PR. The pattern properly enables direct execution of the compiled binary for test runners.Nexus/nexus.wfl (5)
185-196: LGTM! Modulo operator correctly used for even number detection.The
count2 % 2expression properly identifies even numbers to skip, and the expected sum of odd numbers (1+3+5=9) is correctly validated.
255-268: LGTM! Test correctly validates the updatedexit loopsemantics.The test properly verifies that
exit loopnow exits all enclosing loops (both inner and outer), withexit_outer_counterremaining 0 since the code after the inner loop is never reached. This contrasts well with thebreaktest above that only exits the innermost loop.
306-310: LGTM! Simplified error-triggering implementation.The direct
give back 1 divided by 0is a clean approach to trigger the division-by-zero error, and the corresponding try/catch block at lines 342-351 properly validates error handling.
356-423: LGTM! Comprehensive pattern matching test coverage.The test suite covers three distinct patterns with both positive and negative cases:
three_digits(exactly 3 digit) - validates consecutive digit detectionword_pattern(one or more letter) - validates letter-only matchingbetween_pattern(2 to 5 digit) - validates range quantifiersGood job including edge cases like strings with insufficient digits/letters.
456-460: LGTM! Compound condition correctly validates both file reads.The use of
andwith proper parentheses grouping ensures both file contents are verified in a single assertion.scripts/run_integration_tests.ps1 (3)
106-115: Skip list and timeout configuration look good.The skip list appropriately excludes web server tests that require dedicated test runners, and the 30-second timeout is reasonable for preventing test hangs.
138-152: Timeout handling logic is well-implemented.The timeout mechanism correctly converts seconds to milliseconds and properly terminates hung processes. The exit code reporting is helpful for debugging failures.
154-163: Results summary is clear and informative.The summary line provides a quick overview of test outcomes. Consider adding an explicit
$passedProgramscounter for consistency with the shell script version, though the current calculation is correct.scripts/run_integration_tests.sh (2)
80-100: Skip mechanism and timeout configuration are well-designed.The
should_skipfunction correctly iterates through the skip list array. The approach is consistent with the PowerShell version.
119-131: Test discovery and counter initialization are correct.The
maxdepth 1appropriately limits discovery to the TestPrograms directory, and the counter variables are properly initialized.scripts/run_web_tests.ps1 (2)
44-100: Web server test function is well-structured.The
Test-WflWebServerfunction properly:
- Starts the server process in background
- Implements retry logic for server readiness
- Uses try/finally for reliable process cleanup
- Validates response content
113-126: Port extraction with fallback to skip is a good defensive approach.The regex-based port extraction handles the case where the port cannot be determined by skipping the test rather than failing. Decrementing
$totalTestskeeps the pass/total ratio accurate.scripts/run_web_tests.sh (1)
129-154: Test execution and summary logic are correct.The test execution mirrors the PowerShell version appropriately, with proper handling of missing files and port extraction failures.
src/lexer/token.rs (1)
361-362: LGTM! Percent token addition is correct.The new
Percenttoken is properly defined and follows the same pattern as other arithmetic operator tokens. This aligns with the modulo operator implementation in the parser and AST.src/parser/ast.rs (1)
708-708: LGTM! Modulo operator variant added correctly.The
Modulovariant is properly added to theOperatorenum, positioned logically with other arithmetic operators. This complements the lexer and typechecker changes for modulo support.src/fixer/mod.rs (1)
848-848: LGTM! Modulo operator formatting is consistent.The pretty-printer correctly formats the modulo operator with surrounding spaces, matching the style of other arithmetic operators.
src/typechecker/mod.rs (1)
1592-1612: LGTM! Modulo type checking is correctly implemented.The modulo operator is properly grouped with other arithmetic operators that require numeric operands. The type checker will correctly validate that both operands are
Type::Numberand returnType::Number, with appropriate error messages for type mismatches.tests/modulo_operator_test.rs (4)
20-54: Excellent test coverage for basic modulo operations.The test program validates multiple modulo cases (5%2, 10%3, 7%7, 6%4) with clear expected results. This provides good coverage of the modulo operator's basic behavior.
85-104: Smart test for practical modulo usage.Testing modulo in the context of even/odd checking demonstrates a real-world use case and validates that the operator integrates correctly with control flow structures.
134-146: Good error handling validation for modulo by zero.Testing that modulo by zero properly raises an error and is caught by try/catch ensures the error handling path works correctly. This is essential for runtime safety.
1-163: Fix rustfmt formatting issues before merge.The pipeline reports rustfmt formatting failures on lines 1, 4, 15, 66, 116, and 158. Run
cargo fmtto resolve these issues.As per coding guidelines, Rust code must be formatted using
cargo fmt --all.⛔ Skipped due to learnings
Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Format Rust code using `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 **/*.rs : Format Rust code using `cargo fmt --all` (see `.rustfmt.toml`)Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Applies to **/*.rs : Lint clean: run `cargo clippy --all-targets --all-features -- -D warnings` with no warningsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Maintain minimum Rust version 1.75+ compatibilityLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Run clippy linter with `cargo clippy --all-targets --all-features -- -D warnings` to enforce all warnings as errorsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Applies to src/**/*.rs : Implement comprehensive error diagnostics using codespan-reportingLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Always run `cargo build --release` before integration testsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Applies to src/**/*.rs : Provide component documentation for all major modules in Rust source filesLearnt 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: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Applies to **/*.rs : Use Rust edition 2024 for all Rust source filesLearnt 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.tests/zero_arg_action_error_propagation.rs (1)
1-166: Fix rustfmt formatting issues before merge.The pipeline reports rustfmt formatting failures on lines 1, 4, and 18. Run
cargo fmtto resolve these issues.As per coding guidelines, Rust code must be formatted using
cargo fmt --all.⛔ Skipped due to learnings
Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Format Rust code using `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 **/*.rs : Format Rust code using `cargo fmt --all` (see `.rustfmt.toml`)Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Applies to **/*.rs : Lint clean: run `cargo clippy --all-targets --all-features -- -D warnings` with no warningsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Run clippy linter with `cargo clippy --all-targets --all-features -- -D warnings` to enforce all warnings as errorsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Applies to **/*.rs : Use Rust edition 2024 for all Rust source filesLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Maintain minimum Rust version 1.75+ compatibilityLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Applies to src/**/*.rs : Implement comprehensive error diagnostics using codespan-reportingLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Always run `cargo build --release` before integration testsLearnt 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: 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.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.src/parser/mod.rs (2)
1763-1783: Modulo (%) operator wiring looks consistent; just ensure downstream bytecode/emitter support is in placeMapping
Token::PercenttoOperator::Modulowith precedence2(same as multiply/divide) and adding the corresponding consumption arm in the operator-consumption match keeps arithmetic precedence coherent and avoids parser stalls. As long asOperator::Modulois handled in the AST → bytecode / interpreter path and in the typechecker (which PR context suggests), this change is sound. Please double‑check the bytecode/emitter layer for a matchingModulocase, per the parser guidelines.As per coding guidelines, please confirm the bytecode / codegen for
Operator::Modulohas been updated and covered by tests.Also applies to: 2228-2255
1973-1977: Deferringandconsumption correctly aligns with precedence checks and argument‑separator useChanging the
Token::KeywordAndbranch to (a) avoid consuming the token before precedence comparison and (b) consume it only in the post‑precedence operator‑consumption match prevents premature token loss whenparse_binary_expressionis called withprecedence > 0(notably fromparse_argument_list). This makesandbehave like other binary operators and is required for the new argument‑list parsing strategy to work without dropping or misclassifyingandtokens.Please ensure there are tests covering both:
a and bas a normal boolean expression (precedence 0).action with x and ywhereandacts as an argument separator rather than a logical operator.Also applies to: 2258-2260
| # Run with timeout to prevent hangs | ||
| if timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1; then | ||
| print_success "PASS $test_name" | ||
| ((passed_programs++)) | ||
| else | ||
| print_error "✗ $wfl_file" | ||
| exit_code=$? | ||
| if [ $exit_code -eq 124 ]; then | ||
| print_error "TIMEOUT $test_name (exceeded ${TEST_TIMEOUT}s)" | ||
| else | ||
| print_error "FAIL $test_name (exit code: $exit_code)" | ||
| fi | ||
| ((failed_programs++)) | ||
| fi |
There was a problem hiding this comment.
Exit code capture is incorrectly placed.
The exit_code=$? on line 151 captures the exit status of the if condition evaluation, not the timeout command. Move the capture to preserve the actual exit code.
Apply this diff to fix the exit code capture:
# Run with timeout to prevent hangs
- if timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1; then
+ timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1
+ exit_code=$?
+ if [ $exit_code -eq 0 ]; then
print_success "PASS $test_name"
((passed_programs++))
else
- exit_code=$?
if [ $exit_code -eq 124 ]; then
print_error "TIMEOUT $test_name (exceeded ${TEST_TIMEOUT}s)"
else📝 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.
| # Run with timeout to prevent hangs | |
| if timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1; then | |
| print_success "PASS $test_name" | |
| ((passed_programs++)) | |
| else | |
| print_error "✗ $wfl_file" | |
| exit_code=$? | |
| if [ $exit_code -eq 124 ]; then | |
| print_error "TIMEOUT $test_name (exceeded ${TEST_TIMEOUT}s)" | |
| else | |
| print_error "FAIL $test_name (exit code: $exit_code)" | |
| fi | |
| ((failed_programs++)) | |
| fi | |
| # Run with timeout to prevent hangs | |
| timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1 | |
| exit_code=$? | |
| if [ $exit_code -eq 0 ]; then | |
| print_success "PASS $test_name" | |
| ((passed_programs++)) | |
| else | |
| if [ $exit_code -eq 124 ]; then | |
| print_error "TIMEOUT $test_name (exceeded ${TEST_TIMEOUT}s)" | |
| else | |
| print_error "FAIL $test_name (exit code: $exit_code)" | |
| fi | |
| ((failed_programs++)) | |
| fi |
🤖 Prompt for AI Agents
In scripts/run_integration_tests.sh around lines 146 to 158, the exit_code=$? is
placed after the if which captures the status of the shell's conditional
evaluation rather than the timeout command; move the exit_code=$? to immediately
after the timeout "./$WFL_BINARY" "$wfl_file" invocation (before the if) so you
preserve the actual exit status from timeout, then adjust the if to test that
saved exit_code (e.g., check if exit_code == 0) to decide PASS vs
failure/timeouts and keep the existing timeout-vs-other-exit handling.
| # Cleanup function | ||
| cleanup() { | ||
| if kill -0 $server_pid 2>/dev/null; then | ||
| kill $server_pid 2>/dev/null || true | ||
| echo -e "${GRAY}[INFO] Server process terminated${NC}" | ||
| fi | ||
| } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
Trap persists globally and may reference stale PIDs.
The trap cleanup EXIT set inside the function persists after the function returns. If the script exits unexpectedly between tests, it may attempt to kill a stale or reused PID. Consider resetting the trap after each test or using a subshell for isolation.
Apply this diff to reset the trap after cleanup:
if [ "$server_ready" = false ]; then
echo -e "${RED}[ERROR]${NC} TIMEOUT: Server did not start within ${timeout_seconds}s"
cleanup
+ trap - EXIT
return 1
fi
# Server is ready, check response
if [[ "$response" == *"$expected_response"* ]]; then
echo -e "${GREEN}[SUCCESS]${NC} PASS: Got expected response"
cleanup
+ trap - EXIT
return 0
else
echo -e "${RED}[ERROR]${NC} FAIL: Unexpected response"
echo -e "${GRAY} Expected: $expected_response${NC}"
echo -e "${GRAY} Got: $response${NC}"
cleanup
+ trap - EXIT
return 1
fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/run_web_tests.sh around lines 85 to 92, the global trap setup `trap
cleanup EXIT` can fire later against stale PIDs; update cleanup to unset the
trap after it runs or scope the server to a subshell. Specifically, keep the
existing cleanup body but add a call to `trap - EXIT` at the start or end of
cleanup to remove the EXIT trap after executing, and ensure the trap is set only
around the lifecycle of the spawned server (or run the server/test in a subshell
so the trap cannot persist globally).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/zero_arg_action_error_propagation.rs (1)
12-27: Extract helper function to reduce duplication.The binary path setup and verification logic is duplicated between both test functions. Consider extracting a helper function to improve maintainability.
fn get_wfl_binary_path() -> std::path::PathBuf { let wfl_binary = if cfg!(target_os = "windows") { "target/release/wfl.exe" } else { "target/release/wfl" }; let binary_path = env::current_dir().unwrap().join(wfl_binary); if !binary_path.exists() { panic!( "WFL binary not found at {:?}. Run 'cargo build --release' first.", binary_path ); } binary_path }Similarly, consider extracting the test execution and validation logic into a helper function that accepts the test program content and expected output conditions.
Also applies to: 92-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/interpreter/mod.rs(6 hunks)tests/modulo_operator_test.rs(1 hunks)tests/zero_arg_action_error_propagation.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/modulo_operator_test.rs
- src/interpreter/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.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 codeUse Rust edition 2024 for all Rust source files
Files:
tests/zero_arg_action_error_propagation.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/zero_arg_action_error_propagation.rs
🧠 Learnings (5)
📚 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/zero_arg_action_error_propagation.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to TestPrograms/**/*.wfl : All TestPrograms/*.wfl files MUST pass after any change
Applied to files:
tests/zero_arg_action_error_propagation.rs
📚 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:
tests/zero_arg_action_error_propagation.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:
tests/zero_arg_action_error_propagation.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:
tests/zero_arg_action_error_propagation.rs
| let test_file = "test_zero_arg_error.wfl"; | ||
| fs::write(test_file, test_program).expect("Failed to write test file"); |
There was a problem hiding this comment.
Use temp directories to avoid race conditions in parallel test execution.
Both tests write to fixed filenames in the current directory (test_zero_arg_error.wfl and test_zero_arg_autocall.wfl). Since Rust's test runner executes tests in parallel by default, this can cause race conditions if multiple test runs occur simultaneously or if the same test is invoked concurrently in CI environments.
Consider using std::env::temp_dir() combined with a unique identifier, or better yet, the tempfile crate for automatic cleanup:
use std::env;
let test_file = env::temp_dir().join(format!("test_zero_arg_error_{}.wfl", std::process::id()));Or with the tempfile crate:
let mut test_file = tempfile::NamedTempFile::new().unwrap();
test_file.write_all(test_program.as_bytes()).unwrap();
let test_path = test_file.path();Also applies to: 125-126
🤖 Prompt for AI Agents
In tests/zero_arg_action_error_propagation.rs around lines 51-52 (also apply
same fix at lines 125-126): tests write fixed filenames into the current
directory which causes race conditions when tests run in parallel; switch to
creating unique temp files/paths (use std::env::temp_dir with a unique suffix
such as process id or a UUID) or use the tempfile crate to create a
NamedTempFile, write the test_program into that temp file, and use its path for
the test so files are unique and cleaned up automatically.
Improves the reliability of file I/O operations on Windows by selectively ignoring spurious `PermissionDenied` errors from `sync_all()`. On Windows, concurrent file access from other processes like antivirus software can cause `sync_all()` to fail unexpectedly. Since the preceding `flush()` call already ensures data is safe in OS buffers, this specific error can be safely ignored. This change introduces a handler that catches and suppresses only the `PermissionDenied` error on Windows, logging a warning instead of failing the operation. All other critical I/O errors (e.g., disk full) are still propagated on all platforms to prevent data loss. The platform-specific logic is centralized into a new helper function, and new integration tests are added to verify the fix and ensure data integrity.
|
Claude finished @logbie's task —— View job PR Review: Language Features and Testing InfrastructureI've completed a comprehensive review of this substantial PR that introduces multiple language enhancements and significantly improves the testing infrastructure. Here's my detailed feedback: Review Checklist:
✅ StrengthsLanguage Enhancements
Testing Infrastructure
|
Updates integration tests to use unique file names for temporary script files and artifacts. Appending the process ID to file names ensures each test instance is isolated, preventing file access conflicts and race conditions when tests are run concurrently. This change improves the stability and reliability of the test suite, particularly in CI environments.
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/interpreter/mod.rs (1)
1-6102: Runcargo fmtto clear formatting issues in this moduleCI reports
cargo fmt --all -- --checkdiffs for this file. Given the size ofsrc/interpreter/mod.rs, please runcargo fmt --allso the new helper, modulo function, and auto‑call changes conform to the repo’s Rust formatting rules and unblock the pipeline.
🧹 Nitpick comments (2)
tests/file_io_windows_sync_errors_test.rs (1)
85-280: Cross‑platform integrity / append / multi‑cycle tests are well‑structuredThese three tests exercise:
- Basic write/read integrity,
- Append semantics with sync handling,
- Multiple write/close cycles,
via real WFL programs and the release binary. The PASS‑string assertions and explicit cleanup of temp files look sound and non‑flaky.
If you find yourself adding more of these, consider extracting a small helper (e.g. “run_wfl_script_and_capture”) to de‑duplicate the binary resolution + script file creation + execution + cleanup pattern, but it’s not required for this PR.
src/interpreter/mod.rs (1)
4840-4858: Modulo operator integration and implementation look correct
- Binary operation dispatch was extended with:
Operator::Modulo => self.modulo(left_val, right_val, *line, *column),
fn modulomirrorsdivide:
- Accepts only
Value::Numberoperands,- Rejects zero divisor with
"Modulo by zero",- Computes
a % b,- Ensures the result is finite (
is_finite()), otherwise raises a runtime error,- Produces clear type‑error messaging for non‑numeric operands.
This is consistent with existing arithmetic helpers and should integrate cleanly with the rest of the interpreter. You may want tests that cover negative operands to lock in the chosen sign semantics, but the core implementation is solid.
Also applies to: 5790-5835
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interpreter/mod.rs(7 hunks)tests/file_io_windows_sync_errors_test.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.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 codeUse Rust edition 2024 for all Rust source files
Files:
tests/file_io_windows_sync_errors_test.rssrc/interpreter/mod.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/file_io_windows_sync_errors_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/file_io_windows_sync_errors_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Provide component documentation for all major modules in Rust source files
Implement comprehensive error diagnostics using codespan-reporting
Files:
src/interpreter/mod.rs
src/interpreter/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Apply security sanitization to subprocess execution in Rust implementation
Files:
src/interpreter/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/file_io_windows_sync_errors_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/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)
Applied to files:
tests/file_io_windows_sync_errors_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/file_io_windows_sync_errors_test.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to src/interpreter/**/*.rs : Apply security sanitization to subprocess execution in Rust implementation
Applied to files:
src/interpreter/mod.rs
🧬 Code graph analysis (1)
src/interpreter/mod.rs (2)
src/interpreter/error.rs (1)
new(25-32)src/interpreter/value.rs (1)
type_name(158-180)
🪛 GitHub Actions: CI
tests/file_io_windows_sync_errors_test.rs
[error] 1-1: Code formatting required: cargo fmt --all -- --check reported diffs in this file. Run 'cargo fmt' to fix.
src/interpreter/mod.rs
[error] 1-1: Code formatting required: cargo fmt --all -- --check reported diffs in this file. Run 'cargo fmt' to fix.
⏰ 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 (6)
tests/file_io_windows_sync_errors_test.rs (2)
13-83: Windows PermissionDenied sync test matches intended behaviorThe test correctly:
- Resolves the Windows
wfl.exepath.- Exercises write → read → verify through the interpreter.
- Treats
PermissionDeniedonsync_all()as non-fatal while still requiring aWarningin stderr when it occurs.This aligns with the documented Windows sync semantics and gives good coverage of the new behavior.
1-280: Fixcargo fmtviolations in this test moduleCI is failing with
cargo fmt --all -- --checkdiffs for this file. Please runcargo fmt --alllocally (or equivalent in your workflow) so the tests compile cleanly in CI.⛔ Skipped due to learnings
Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Format Rust code using `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 **/*.rs : Format Rust code using `cargo fmt --all` (see `.rustfmt.toml`)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 **/tests/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)Learnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Always run `cargo build --release` before integration testsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-01T18:28:13.615Z Learning: Applies to **/*.rs : Lint clean: run `cargo clippy --all-targets --all-features -- -D warnings` with no warningsLearnt from: CR Repo: WebFirstLanguage/wfl PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-05T10:17:06.447Z Learning: Run clippy linter with `cargo clippy --all-targets --all-features -- -D warnings` to enforce all warnings as errorsLearnt 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.src/interpreter/mod.rs (4)
465-509: Windows‑aware sync helper correctly restores error propagation semanticsThe new
sync_file_with_windows_handlinghelper centralizes platform‑specific behavior and:
- Propagates all
sync_all()errors by default.- On Windows, selectively swallows only
ErrorKind::PermissionDenied, emitting a warning to stderr and returningOk(()).This addresses the earlier regression where all sync errors were ignored and preserves the documented requirement that real I/O failures (disk full, read‑only FS, etc.) still surface.
545-548: write_file now syncs with correct Windows error handlingAfter flushing,
write_filedelegates tosync_file_with_windows_handling("write"), so:
- Data is flushed then fsync’d,
- Spurious Windows
PermissionDeniedis tolerated with a warning,- All other sync failures cause the write to fail.
This matches the helper’s contract and restores correct reporting for real errors.
571-578: close_file flush+sync flow is consistent with new helperOn close you now:
- Flush the file,
- Then call
sync_file_with_windows_handling("close")and propagate any non‑PermissionDeniederrors.Behavior is consistent with
write_fileand avoids the previous “swallow all sync errors” problem on Windows.
595-603: append_file syncs appended data with Windows‑aware behavior
append_filenow:
- Seeks to end, writes, flushes,
- Then uses
sync_file_with_windows_handling("append").This ensures appended data is durably reported while still tolerating known‑spurious Windows
PermissionDeniedsync errors.
| Value::Function(func) => { | ||
| if func.params.is_empty() { | ||
| // Auto-call zero-argument user-defined functions | ||
| self.call_function(func, vec![], *line, *column).await | ||
| } else { | ||
| // Return function object for functions with arguments | ||
| Ok(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Zero‑arg function auto‑call breaks explicit calls like my_action()
Expression::Variable now auto‑invokes zero‑argument user functions:
Expression::Variable(name, line, column) => {
…
if let Some(value) = env.borrow().get(name) {
match &value {
Value::NativeFunction(..) => { /* auto‑call if arity 0 */ }
Value::Function(func) => {
if func.params.is_empty() {
self.call_function(func, vec![], *line, *column).await
} else {
Ok(value)
}
}
_ => Ok(value),
}
}
…
}However, Expression::FunctionCall still does:
let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?;
…
match function_val {
Value::Function(func) => self.call_function(&func, arg_values, *line, *column).await,
…
}For a zero‑arg action called as my_action(), the function expression is Expression::Variable("my_action", …). With the new logic:
evaluate_expression(function)auto‑callsmy_actionwith no args and returns its result (e.g.,Value::Null),Expression::FunctionCallthen attempts to treat that result as a function and errors with “Cannot call Null…” instead of executing the action once.
This is a behavioral regression for zero‑argument functions invoked with parentheses.
A targeted fix is to avoid using evaluate_expression for the callee when it is a simple variable, and instead resolve it directly from the environment so the auto‑call path is skipped for the callee:
- } => {
- let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?;
+ } => {
+ // Resolve the callee without triggering zero‑arg auto‑call when it's a bare variable
+ let function_val = match function.as_ref() {
+ Expression::Variable(name, _fn_line, _fn_col) => {
+ env.borrow()
+ .get(name)
+ .ok_or_else(|| RuntimeError::new(
+ format!("Undefined function '{}'", name),
+ *line,
+ *column,
+ ))?
+ }
+ _ => self.evaluate_expression(function, Rc::clone(&env)).await?,
+ };This preserves the new “auto‑call on bare reference” behavior for general expressions while keeping explicit calls like my_action() working as before.
Also applies to: 4885-4937
There was a problem hiding this comment.
Please open an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the zero-arg function auto-call bug
gh issue create \
--title "Zero-argument function auto-call breaks explicit calls with parentheses" \
--body "## Problem Description
The new zero-argument function auto-call feature introduced in PR #191 causes a behavioral regression for zero-argument functions invoked with explicit parentheses.
## Current Behavior
When a zero-argument user-defined function is called explicitly with parentheses (e.g., \`my_action()\`), the following happens:
1. The function expression \`Expression::Variable(\"my_action\", ...)\` is evaluated
2. The new auto-call logic in \`Expression::Variable\` immediately invokes the function and returns its result (e.g., \`Value::Null\`)
3. \`Expression::FunctionCall\` then attempts to treat that result as a function
4. This results in an error like \"Cannot call Null\" instead of executing the action once
## Expected Behavior
Zero-argument functions should work correctly whether:
- Referenced as a bare variable (auto-call): \`my_action\`
- Called explicitly with parentheses: \`my_action()\`
Both should execute the function once and return its result.
## Impact
This is a critical regression that breaks existing code using explicit function calls with parentheses for zero-argument functions.
## References
- Related PR: https://github.com/WebFirstLanguage/wfl/pull/191
- Code review comment: https://github.com/WebFirstLanguage/wfl/pull/191#discussion_r2593337324
- Requested by: @logbie
## Affected Code
- \`src/interpreter/mod.rs\`: \`Expression::Variable\` (auto-call logic)
- \`src/interpreter/mod.rs\`: \`Expression::FunctionCall\` (callee evaluation)
---
_This issue was created as a follow-up to code review feedback._"
Length of output: 50
@logbie, I've opened issue #193 for this: #193
The issue documents the zero-argument function auto-call regression and includes:
- Clear description of the problem and its impact
- References back to this PR and comment
- Details about the affected code locations
🧬 Code Graph Analysis Results
- File: src/interpreter/error.rs, Lines: 25-32
pub fn new(message: String, line: usize, column: usize) -> Self {
RuntimeError {
message,
line,
column,
kind: ErrorKind::General,
}
}
- File: src/interpreter/error.rs, Lines: 34-41
pub fn with_kind(message: String, line: usize, column: usize, kind: ErrorKind) -> Self {
RuntimeError {
message,
line,
column,
kind,
}
}
- File: src/interpreter/environment.rs, Lines: 14-23
pub fn new_global() -> Rc<RefCell<Self>> {
#[cfg(feature = "dhat-ad-hoc")]
dhat::ad_hoc_event(1);
Rc::new(RefCell::new(Environment {
values: HashMap::new(),
constants: HashSet::new(),
parent: None,
}))
}
- File: src/interpreter/environment.rs, Lines: 25-34
pub fn new(parent: &Rc<RefCell<Environment>>) -> Rc<RefCell<Self>> {
#[cfg(feature = "dhat-ad-hoc")]
dhat::ad_hoc_event(1);
Rc::new(RefCell::new(Environment {
values: HashMap::new(),
constants: HashSet::new(),
parent: Some(Rc::downgrade(parent)),
}))
}
- File: src/interpreter/value.rs, Lines: 158-180
pub fn type_name(&self) -> &'static str {
match self {
Value::Number(_) => "Number",
Value::Text(_) => "Text",
Value::Bool(_) => "Boolean",
Value::List(_) => "List",
Value::Object(_) => "Object",
Value::Function(_) => "Function",
Value::NativeFunction(_, _) => "NativeFunction",
Value::Future(_) => "Future",
Value::Date(_) => "Date",
Value::Time(_) => "Time",
Value::DateTime(_) => "DateTime",
Value::Pattern(_) => "Pattern",
Value::Null => "Null",
Value::Nothing => "Nothing",
Value::ContainerDefinition(_def) => "Container",
Value::ContainerInstance(_) => "ContainerInstance",
Value::ContainerMethod(_) => "ContainerMethod",
Value::ContainerEvent(_) => "ContainerEvent",
Value::InterfaceDefinition(_) => "Interface",
}
}
- File: src/interpreter/value.rs, Lines: 182-201
pub fn is_truthy(&self) -> bool {
match self {
Value::Bool(b) => *b,
Value::Null => false,
Value::Number(n) => *n != 0.0,
Value::Text(s) => !s.is_empty(),
Value::List(list) => !list.borrow().is_empty(),
Value::Object(obj) => !obj.borrow().is_empty(),
Value::Function(_) | Value::NativeFunction(_, _) => true,
Value::Future(future) => future.borrow().completed,
Value::Date(_) | Value::Time(_) | Value::DateTime(_) => true,
Value::Pattern(_) => true,
Value::Nothing => false,
Value::ContainerDefinition(_) => true,
Value::ContainerInstance(_) => true,
Value::ContainerMethod(_) => true,
Value::ContainerEvent(_) => true,
Value::InterfaceDefinition(_) => true,
}
}
- File: src/interpreter/command_sanitizer.rs, Lines: 40-143
pub fn parse_command(command: &str) -> Result<(String, Vec<String>), String> {
let trimmed = command.trim();
if trimmed.is_empty() {
return Err("Empty command".to_string());
}
#[derive(Debug, Clone, Copy, PartialEq)]
enum State {
Normal, // Outside quotes
InDoubleQuote, // Inside "..."
InSingleQuote, // Inside '...'
Escape, // After backslash outside quotes
EscapeInDouble, // After backslash inside double quotes
}
let mut parts = Vec::new();
let mut current = String::new();
let mut state = State::Normal;
let mut in_quoted_context = false; // Track if we just closed quotes (for empty strings)
for ch in trimmed.chars() {
match state {
State::Normal => match ch {
'"' => {
state = State::InDoubleQuote;
in_quoted_context = true;
}
'\'' => {
state = State::InSingleQuote;
in_quoted_context = true;
}
'\\' => state = State::Escape,
' ' | '\t' => {
if !current.is_empty() || in_quoted_context {
parts.push(current.clone());
current.clear();
in_quoted_context = false;
}
}
_ => {
current.push(ch);
in_quoted_context = false;
}
},
State::InDoubleQuote => match ch {
'"' => state = State::Normal,
'\\' => state = State::EscapeInDouble,
_ => current.push(ch),
},
State::InSingleQuote => match ch {
'\'' => state = State::Normal,
_ => current.push(ch), // Single quotes preserve everything literally
},
State::Escape => {
current.push(ch); // Backslash outside quotes escapes next char
state = State::Normal;
in_quoted_context = false;
}
State::EscapeInDouble => {
// Handle escape sequences in double quotes
match ch {
'n' => current.push('\n'),
't' => current.push('\t'),
'r' => current.push('\r'),
'\\' => current.push('\\'),
'"' => current.push('"'),
'0' => current.push('\0'),
_ => {
current.push('\\');
current.push(ch);
}
}
state = State::InDoubleQuote;
}
}
}
// Check for unclosed quotes or trailing escape
match state {
State::InDoubleQuote => return Err("Unclosed double quote".to_string()),
State::InSingleQuote => return Err("Unclosed single quote".to_string()),
State::Escape | State::EscapeInDouble => {
return Err("Trailing escape character".to_string());
}
State::Normal => {
if !current.is_empty() || in_quoted_context {
parts.push(current);
}
}
}
if parts.is_empty() {
return Err("No program specified".to_string());
}
let program = parts[0].clone();
let args = parts[1..].to_vec();
Ok((program, args))
}
- File: src/interpreter/command_sanitizer.rs, Lines: 146-165
pub fn contains_shell_metacharacters(command: &str) -> bool {
const SHELL_METACHARACTERS: &[char] = &[
';', '|', '&', '<', '>', '$', '`', '(', ')', '{', '}', '[', ']', '*', '?', '~', '!',
'\\', '\n', '\r',
];
// Check for metacharacters
for ch in SHELL_METACHARACTERS {
if command.contains(*ch) {
return true;
}
}
// Check for command substitution patterns
if command.contains("$(") || command.contains("${") {
return true;
}
false
}
Updates the test file name to follow the project's naming convention, making it more explicit that the file contains tests.
Formats Rust code in the interpreter and Windows file I/O tests using rustfmt. This is a purely stylistic change to improve readability and maintain a consistent code style, with no impact on functionality.
|
Claude finished @logbie's task —— View job PR Review: Language Features and Testing InfrastructureI've completed a comprehensive review of this substantial PR that introduces multiple language enhancements and significantly improves the testing infrastructure. Review Checklist:
🎯 Overall Assessment: NEEDS CRITICAL FIXThis PR implements valuable language features and testing improvements, but contains a critical bug that breaks zero-argument function calls and several other issues that need addressing before merge. 🚨 Critical Issues1. CRITICAL: Zero-Argument Function Auto-Call Breaks Explicit CallsLocation: The auto-call implementation in Problem: When calling
Impact: All zero-argument function calls with parentheses will fail with "Cannot call Null" errors. Fix Required: Modify // In FunctionCall handler, replace:
let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?;
// With:
let function_val = match function.as_ref() {
Expression::Variable(name, _fn_line, _fn_col) => {
// Bypass auto-call for function callees
env.borrow()
.get(name)
.ok_or_else(|| RuntimeError::new(
format!("Undefined function '{}'", name),
*line, *column,
))?
}
_ => self.evaluate_expression(function, Rc::clone(&env)).await?,
};2. CRITICAL: Exit Code Capture Bug in Shell ScriptLocation: Exit code capture happens after the Fix Required: timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1
exit_code=$?
if [ $exit_code -eq 0 ]; then
# ... rest of logic3. File Sync Error Handling RegressionLocation: While the Windows-specific sync handling is well-intentioned, there are documentation/implementation mismatches that could confuse future maintainers. ✅ StrengthsLanguage Enhancements
Testing Infrastructure
Platform-Specific Improvements
⚡ Performance ConsiderationsGood:
Concerns:
🔒 Security AssessmentSafe:
Minor Concerns:
🧪 Test Coverage AssessmentExcellent Coverage:
Missing Coverage:
📋 Additional IssuesCode Quality
Minor Improvements
🎯 Recommendation: REQUIRES FIXES BEFORE MERGEMust Fix:
Should Fix: Nice to Have: The modulo operator and testing infrastructure improvements are well-implemented, but the function auto-call bug is a showstopper that needs immediate attention. |
Introduces a mutex to serialize tests that modify shared environment variables. This prevents interference and flaky results when tests are executed in parallel, ensuring their stability and reliability.
|
Claude finished @logbie's task —— View job PR Review: Language Features and Testing InfrastructureI've completed a comprehensive review of this substantial PR that introduces multiple language enhancements and significantly improves the testing infrastructure. Review Checklist:
🎯 Overall Assessment: NEEDS CRITICAL FIXThis PR implements valuable language features and testing improvements, but contains a critical bug that breaks zero-argument function calls and several other issues that need addressing before merge. 🚨 Critical Issues1. CRITICAL: Zero-Argument Function Auto-Call Breaks Explicit CallsLocation: The auto-call implementation in Problem: When calling
Impact: All zero-argument function calls with parentheses will fail with "Cannot call Null" errors. Fix Required: Modify // In FunctionCall handler, replace:
let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?;
// With:
let function_val = match function.as_ref() {
Expression::Variable(name, _fn_line, _fn_col) => {
// Bypass auto-call for function callees
env.borrow()
.get(name)
.ok_or_else(|| RuntimeError::new(
format!("Undefined function '{}'", name),
*line, *column,
))?
}
_ => self.evaluate_expression(function, Rc::clone(&env)).await?,
};2. CRITICAL: Exit Code Capture Bug in Shell ScriptLocation: Exit code capture happens after the Fix Required: timeout "${TEST_TIMEOUT}s" "./$WFL_BINARY" "$wfl_file" > /dev/null 2>&1
exit_code=$?
if [ $exit_code -eq 0 ]; then
# ... rest of logic3. File Sync Error Handling RegressionLocation: While the Windows-specific sync handling is well-intentioned, there are documentation/implementation mismatches that could confuse future maintainers. ✅ StrengthsLanguage Enhancements
Testing Infrastructure
Platform-Specific Improvements
⚡ Performance ConsiderationsGood:
Concerns:
🔒 Security AssessmentSafe:
Minor Concerns:
🧪 Test Coverage AssessmentExcellent Coverage:
Missing Coverage:
📋 Additional IssuesCode Quality
Minor Improvements
🎯 Recommendation: REQUIRES FIXES BEFORE MERGEMust Fix:
Should Fix: Nice to Have: The modulo operator and testing infrastructure improvements are well-implemented, but the function auto-call bug is a showstopper that needs immediate attention. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/interpreter/mod.rs (1)
4767-4825: Zero‑arg auto‑call still breaks explicit calls likemy_action()/display()
Expression::Variablenow auto‑invokes zero‑argumentValue::FunctionandValue::NativeFunctionvalues, butExpression::FunctionCallstill obtains the callee viaevaluate_expression(function, ...). For a zero‑arg callee such asget_value()orfaulty_action():
evaluate_expression(function)auto‑calls it and returns the result (oftenNullor a number),FunctionCallthen attempts to treat that result as a function and errors with “Cannot call Null/Number…”, after having already executed the body once.This is a behavioral regression for existing code that uses explicit parentheses (and for zero‑arg natives like
display()). The previous review already flagged this; the issue remains.A minimal fix is to resolve simple variable callees directly from the environment in the
FunctionCallarm, bypassing the auto‑call path, and only useevaluate_expressionfor more complex callee expressions:Expression::FunctionCall { function, arguments, line, column, } => { - let function_val = self.evaluate_expression(function, Rc::clone(&env)).await?; + let function_val = match function.as_ref() { + // Avoid triggering zero‑arg auto‑call for bare function names + Expression::Variable(name, _fn_line, _fn_col) => { + env.borrow() + .get(name) + .ok_or_else(|| RuntimeError::new( + format!("Undefined function '{name}'"), + *line, + *column, + ))? + } + _ => self.evaluate_expression(function, Rc::clone(&env)).await?, + }; // … rest of arm unchanged …This keeps the new “bare reference auto‑call” behavior while restoring correct semantics for
foo()and native zero‑arg calls.Also applies to: 4885-4938
🧹 Nitpick comments (3)
src/interpreter/mod.rs (1)
4827-4860: Modulo operator wiring and runtime semantics look consistent
Operator::Modulois correctly routed throughExpression::BinaryOperationto the newmodulohelper, which mirrorsdivide’s structure: it accepts onlyValue::Numberoperands, rejects modulo by zero with a clear runtime error, checks the result withis_finite(), and otherwise returnsValue::Number(result). Error messaging for non‑numeric operands is also consistent with the other arithmetic operators. If you later want strictly integer semantics or specific behavior for negatives, that can be tightened in the type checker without changing this runtime path.Also applies to: 5791-5837
tests/modulo_operator_test.rs (1)
1-81: Modulo integration tests are solid; consider a small helper to DRY binary resolutionThe three tests nicely cover normal
%behavior (including control‑flow usage) and the modulo‑by‑zero error path viatry/catch, using temp files to avoid interference between runs. To reduce duplication and keep future path changes in one place, you could extract the repeatedwfl_binary/binary_pathlogic into a small helper likefn wfl_binary_path() -> PathBufand reuse it in each test (and other integration tests that launch the binary).Also applies to: 83-137, 139-186
tests/zero_arg_action_error_propagation_test.rs (1)
1-89: Good coverage of zero‑arg auto‑call; add a regression test foraction()with parenthesesThese tests nicely cover the new semantics where a zero‑arg action is auto‑called when used as a bare expression (
store res as faulty_action,store result as get_value) and that errors propagate intotry/catch. Given the interpreter’s current implementation, explicit calls likeget_value()/faulty_action()are handled by a different code path (Expression::FunctionCall) that is currently broken by the auto‑call change. It would be worth adding an extra integration test that:
- defines a zero‑arg action,
- calls it as
action_name(),- asserts the result (and error propagation where appropriate),
so future refactors can’t regress the explicit‑call behavior without being caught.
Also applies to: 91-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/interpreter/mod.rs(7 hunks)tests/file_io_windows_sync_errors_test.rs(1 hunks)tests/modulo_operator_test.rs(1 hunks)tests/zero_arg_action_error_propagation_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/file_io_windows_sync_errors_test.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.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 codeUse Rust edition 2024 for all Rust source files
Files:
tests/zero_arg_action_error_propagation_test.rstests/modulo_operator_test.rssrc/interpreter/mod.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/zero_arg_action_error_propagation_test.rstests/modulo_operator_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/zero_arg_action_error_propagation_test.rstests/modulo_operator_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Provide component documentation for all major modules in Rust source files
Implement comprehensive error diagnostics using codespan-reporting
Files:
src/interpreter/mod.rs
src/interpreter/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Apply security sanitization to subprocess execution in Rust implementation
Files:
src/interpreter/mod.rs
🧠 Learnings (10)
📚 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/zero_arg_action_error_propagation_test.rstests/modulo_operator_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/zero_arg_action_error_propagation_test.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to TestPrograms/**/*.wfl : All TestPrograms/*.wfl files MUST pass after any change
Applied to files:
tests/zero_arg_action_error_propagation_test.rs
📚 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:
tests/zero_arg_action_error_propagation_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/**/*_test.rs : Write failing tests first (TDD approach); feature-oriented test names (e.g., `*_test.rs`)
Applied to files:
tests/zero_arg_action_error_propagation_test.rstests/modulo_operator_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:
tests/zero_arg_action_error_propagation_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/zero_arg_action_error_propagation_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 **/*.rs : Use `snake_case` for function and file names in Rust
Applied to files:
tests/zero_arg_action_error_propagation_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 **/*.rs : Review `SECURITY.md`; avoid logging secrets and prefer zeroization for sensitive data in Rust code
Applied to files:
tests/zero_arg_action_error_propagation_test.rs
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to src/interpreter/**/*.rs : Apply security sanitization to subprocess execution in Rust implementation
Applied to files:
src/interpreter/mod.rs
🧬 Code graph analysis (1)
src/interpreter/mod.rs (2)
src/interpreter/error.rs (1)
new(25-32)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). (3)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: claude-review
🔇 Additional comments (1)
src/interpreter/mod.rs (1)
465-509: Windows sync helper now correctly suppresses only spurious PermissionDenied errorsThe new
sync_file_with_windows_handlinghelper centralizessync_all()handling, suppressingPermissionDeniedonly undercfg(windows)and propagating all other errors. Using it fromwrite_file,close_file, andappend_fileafter a successfulflush()restores proper error reporting while still working around the Windows-specific quirk called out in the doc comment. This addresses the earlier regression where all sync errors were swallowed.Also applies to: 545-548, 577-579, 602-603
1 participant
This update introduces several new language features and significantly improves the robustness and structure of the testing suite.
Language Enhancements
%operator for remainder calculations, along with corresponding integration tests.Testing Improvements
Fixes
andwere not parsed correctly.Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.