navicore/patch-rexx

Fork 0

In patch-rexx/src/eval.rs: #23

Merged

navicore merged 4 commits from set-cmd into main

2026-02-13 06:16:41 +00:00

navicore commented

2026-02-13 05:52:36 +00:00

(Migrated from github.com)

Added a command_handler field to Evaluator: Option<Box<dyn FnMut(&str, &str) -> Option>>
Added set_command_handler() public method
Modified exec_host_command() to try the custom handler first — if it returns Some(rc), use that; if None, fall through to the existing sh -c behavior
Extracted fire_failure_trap() as a helper during the refactor

It's fully backward-compatible — without calling set_command_handler, behavior is identical to 0.9.2. All 556 existing tests pass.

- Added a command_handler field to Evaluator: Option<Box<dyn FnMut(&str, &str) -> Option<i32>>> - Added set_command_handler() public method - Modified exec_host_command() to try the custom handler first — if it returns Some(rc), use that; if None, fall through to the existing sh -c behavior - Extracted fire_failure_trap() as a helper during the refactor It's fully backward-compatible — without calling set_command_handler, behavior is identical to 0.9.2. All 556 existing tests pass.

claude[bot] commented

2026-02-13 05:54:42 +00:00

(Migrated from github.com)

PR Review - Custom Command Handler

I've reviewed PR #23 which adds a custom command handler mechanism to the REXX evaluator. Overall, the implementation is solid and backward-compatible. I identified one potential correctness issue and have some observations:

Critical Issue

Potential panic in custom handler (src/eval.rs:492-497)

The custom command handler is called without any panic catching or error handling:

let custom_rc = if let Some(ref mut handler) = self.command_handler {
    let addr = self.env.address().to_string();
    handler(&addr, command)  // <-- Could panic
} else {
    None
};

If the custom handler panics, it will unwind through the evaluator. REXX semantics require that the interpreter should be resilient to errors - malformed input should never crash the interpreter. Consider either:

Document that panics in the handler are the caller's responsibility
Use std::panic::catch_unwind to convert panics into a FAILURE condition
Change the handler signature to return Result<Option<i32>, Box<dyn Error>>

The choice depends on the intended use case. For embedding scenarios (like XEDIT mentioned in the docs), option 1 with clear documentation is probably sufficient since the embedding application controls the handler.

Observations (not issues, but worth noting)

Handler state and reentrancy (src/eval.rs:189)

The handler is Box<dyn FnMut>, which allows stateful handlers. However:

There's no documentation about whether the handler might be called reentrantly (e.g., if the handler itself triggers REXX evaluation via INTERPRET)
The handler isn't Send, so it can't be shared across threads, which is correct for this single-threaded evaluator

Condition handling is preserved correctly (src/eval.rs:522-534)

The ERROR condition is correctly triggered for non-zero return codes from both custom handlers and shell execution. The trap removal (self.traps.remove(&Condition::Error)) follows REXX semantics where a trap fires once and is then disabled.

FAILURE trap helper extraction (src/eval.rs:537-550)

The fire_failure_trap refactoring is clean and improves readability. The early return pattern is correct - if there's no FAILURE trap installed, execution continues normally with RC=-1.

Correctness Verified

✓ ADDRESS environment is correctly passed to the handler (the environment is switched before calling the handler for temporary commands, which is correct REXX semantics)
✓ RC is set before condition traps fire (correct order per ANSI X3.274-1996)
✓ Fall-through behavior preserves existing shell execution
✓ Backward compatibility - all 556 tests pass

Recommendation

Document the panic behavior of the custom handler in the function's documentation comment (src/eval.rs:912-922). Suggest adding something like:

/// # Safety
/// The handler must not panic. If it does, the panic will propagate through
/// the evaluator and potentially terminate the REXX program. Handlers should
/// handle all error cases internally and return appropriate return codes.

Otherwise, LGTM! The implementation is clean, follows REXX semantics correctly, and the refactoring improves code organization.

## PR Review - Custom Command Handler I've reviewed PR #23 which adds a custom command handler mechanism to the REXX evaluator. Overall, the implementation is solid and backward-compatible. I identified one potential correctness issue and have some observations: ### Critical Issue **Potential panic in custom handler (src/eval.rs:492-497)** The custom command handler is called without any panic catching or error handling: ```rust let custom_rc = if let Some(ref mut handler) = self.command_handler { let addr = self.env.address().to_string(); handler(&addr, command) // <-- Could panic } else { None }; ``` If the custom handler panics, it will unwind through the evaluator. REXX semantics require that the interpreter should be resilient to errors - malformed input should never crash the interpreter. Consider either: 1. Document that panics in the handler are the caller's responsibility 2. Use `std::panic::catch_unwind` to convert panics into a FAILURE condition 3. Change the handler signature to return `Result<Option<i32>, Box<dyn Error>>` The choice depends on the intended use case. For embedding scenarios (like XEDIT mentioned in the docs), option 1 with clear documentation is probably sufficient since the embedding application controls the handler. ### Observations (not issues, but worth noting) **Handler state and reentrancy (src/eval.rs:189)** The handler is `Box<dyn FnMut>`, which allows stateful handlers. However: - There's no documentation about whether the handler might be called reentrantly (e.g., if the handler itself triggers REXX evaluation via INTERPRET) - The handler isn't `Send`, so it can't be shared across threads, which is correct for this single-threaded evaluator **Condition handling is preserved correctly (src/eval.rs:522-534)** The ERROR condition is correctly triggered for non-zero return codes from both custom handlers and shell execution. The trap removal (`self.traps.remove(&Condition::Error)`) follows REXX semantics where a trap fires once and is then disabled. **FAILURE trap helper extraction (src/eval.rs:537-550)** The `fire_failure_trap` refactoring is clean and improves readability. The early return pattern is correct - if there's no FAILURE trap installed, execution continues normally with RC=-1. ### Correctness Verified ✓ ADDRESS environment is correctly passed to the handler (the environment is switched before calling the handler for temporary commands, which is correct REXX semantics) ✓ RC is set before condition traps fire (correct order per ANSI X3.274-1996) ✓ Fall-through behavior preserves existing shell execution ✓ Backward compatibility - all 556 tests pass ### Recommendation Document the panic behavior of the custom handler in the function's documentation comment (src/eval.rs:912-922). Suggest adding something like: ```rust /// # Safety /// The handler must not panic. If it does, the panic will propagate through /// the evaluator and potentially terminate the REXX program. Handlers should /// handle all error cases internally and return appropriate return codes. ``` Otherwise, LGTM! The implementation is clean, follows REXX semantics correctly, and the refactoring improves code organization.

claude[bot] commented

2026-02-13 06:02:38 +00:00

(Migrated from github.com)

Code Review

I reviewed PR #23 for the custom command handler feature. The implementation is mostly solid, but I found one critical issue that needs to be fixed:

Critical Issue: Documentation Corruption

src/eval.rs:187 and src/eval.rs:911 - The documentation comments are corrupted with garbled text:

/// Called as `handler(address_env``command_string`ng)`return_code`code.

Should be:

/// Called as `handler(address_environment, command_string)` and returns a return code.

This appears in two places:

Line 187: Field documentation
Line 911: Method documentation

What I Verified ✓

Correctness:

ERROR trap logic preserved correctly (fires when rc != 0, lines 520-530)
FAILURE trap logic correct (only fires when shell spawn fails, line 516)
Custom handler bypasses shell execution when it returns Some(rc) - this is correct behavior
RC variable set properly in all code paths (lines 502, 512, 515)

REXX Semantics:

ERROR condition fires on non-zero return codes ✓
FAILURE condition fires on command execution failure ✓
Condition info data structure populated correctly ✓
Traps are removed after firing (one-shot behavior) ✓

Error Handling:

No panics on malformed input ✓
Proper handling of status.code() returning None (uses -1) ✓
Early return prevents double trap firing ✓

Safety & Design:

FnMut allows stateful handlers ✓
ref mut borrow prevents move ✓
Backward compatible (None handler = original behavior) ✓
fire_failure_trap extraction is clean refactoring ✓

Minor Observations (not blocking)

The #[allow(clippy::type_complexity)] is appropriate here
The refactoring maintains identical behavior to 0.9.2 when no handler is set
All 556 tests passing confirms backward compatibility

Action Required: Fix the documentation corruption at lines 187 and 911 before merging.

## Code Review I reviewed PR #23 for the custom command handler feature. The implementation is mostly solid, but I found **one critical issue** that needs to be fixed: ### Critical Issue: Documentation Corruption **src/eval.rs:187** and **src/eval.rs:911** - The documentation comments are corrupted with garbled text: ```rust /// Called as `handler(address_env``command_string`ng)`return_code`code. ``` Should be: ```rust /// Called as `handler(address_environment, command_string)` and returns a return code. ``` This appears in two places: - Line 187: Field documentation - Line 911: Method documentation ### What I Verified ✓ **Correctness:** - ERROR trap logic preserved correctly (fires when rc != 0, lines 520-530) - FAILURE trap logic correct (only fires when shell spawn fails, line 516) - Custom handler bypasses shell execution when it returns `Some(rc)` - this is correct behavior - RC variable set properly in all code paths (lines 502, 512, 515) **REXX Semantics:** - ERROR condition fires on non-zero return codes ✓ - FAILURE condition fires on command execution failure ✓ - Condition info data structure populated correctly ✓ - Traps are removed after firing (one-shot behavior) ✓ **Error Handling:** - No panics on malformed input ✓ - Proper handling of `status.code()` returning None (uses -1) ✓ - Early return prevents double trap firing ✓ **Safety & Design:** - `FnMut` allows stateful handlers ✓ - `ref mut` borrow prevents move ✓ - Backward compatible (None handler = original behavior) ✓ - `fire_failure_trap` extraction is clean refactoring ✓ ### Minor Observations (not blocking) - The `#[allow(clippy::type_complexity)]` is appropriate here - The refactoring maintains identical behavior to 0.9.2 when no handler is set - All 556 tests passing confirms backward compatibility --- **Action Required:** Fix the documentation corruption at lines 187 and 911 before merging.

claude[bot] commented

2026-02-13 06:07:20 +00:00

(Migrated from github.com)

Code Review - PR #23: Custom Command Handler

I've reviewed this PR for correctness, REXX semantics compliance, and safety. The implementation is clean and backward-compatible. I found one issue that must be fixed before merging.

CRITICAL: Documentation Corruption

src/eval.rs:187 and src/eval.rs:912 - Doc comments are corrupted with garbled backticks:

/// Called as `handler(address_env``command_string`ng)`return_code`code.

Should be something like:

/// Called as `handler(address_environment, command_string)` returning `Option<i32>`.

This appears in two locations:

Line 187: Field documentation in the Evaluator struct
Line 912: Method documentation for set_command_handler()

Impact: Rustdoc output will be malformed. This affects users trying to understand the API.

Correctness Analysis ✓

ERROR/FAILURE semantics (lines 491-548):

ERROR correctly fires when rc != 0 from either custom handler or shell (lines 520-531) ✓
FAILURE correctly fires when shell spawn fails (line 516) ✓
Custom handler returning Some(rc) bypasses shell execution - correct design ✓
Condition info properly populated in both cases ✓
Traps are removed after firing (REXX one-shot semantics) ✓

RC variable handling:

Set correctly for custom handler path (line 502) ✓
Set correctly for shell execution success (line 512) ✓
Set to -1 for shell spawn failure (line 515) ✓
All paths set RC before firing traps (correct order per ANSI X3.274-1996) ✓

Refactoring quality:

fire_failure_trap() extraction improves readability ✓
No behavior change from original code ✓
All 556 tests pass (backward compatibility confirmed) ✓

Safety Analysis ✓

No panics on malformed input:

Custom handler could panic, but that's caller's responsibility - this is acceptable for an embedding API ✓
status.code() returns None is handled with unwrap_or(-1) (line 511) ✓
Early return prevents double trap firing ✓

Type safety:

Box<dyn FnMut(&str, &str) -> Option<i32>> signature is appropriate ✓
ref mut prevents moving the handler ✓
#[allow(clippy::type_complexity)] is justified here ✓

REXX Semantics Compliance ✓

Per ANSI X3.274-1996 and test suite analysis:

ERROR condition: Fires when command executes but returns non-zero exit code ✓
FAILURE condition: Fires when command cannot be executed (spawn failure) ✓
RC variable: Set before condition traps fire ✓
Trap auto-disable: Both ERROR and FAILURE traps removed after firing ✓
Condition info: Populated with correct condition, description, instruction, status ✓
ADDRESS environment: Passed to handler correctly (line 494) ✓

Test coverage from tests/phase8.rs confirms all behaviors are preserved.

Design Notes (not issues)

Custom handler semantics: Handler returns Some(rc) to handle command, None to fall through to shell. Clean design that avoids breaking changes.
Reentrancy: Handler is FnMut, allowing state. Documentation doesn't address reentrancy (e.g., if handler triggers INTERPRET that calls another command). This is probably fine for initial use case.
FAILURE from custom handler: If a custom handler wants to signal "I couldn't execute this", it has no way to trigger FAILURE condition. Returning Some(-1) triggers ERROR, not FAILURE. This is probably correct - the handler either handles the command or doesn't; spawn failures are shell-specific.

Summary

Must fix: Documentation corruption at lines 187 and 912.

Otherwise: LGTM! The implementation correctly preserves REXX semantics, handles all error conditions properly, maintains backward compatibility, and provides a clean API for embedding applications.

File references:

src/eval.rs:187 (field doc)
src/eval.rs:491-548 (exec_host_command, fire_failure_trap)
src/eval.rs:910-921 (set_command_handler doc and implementation)

## Code Review - PR #23: Custom Command Handler I've reviewed this PR for correctness, REXX semantics compliance, and safety. The implementation is clean and backward-compatible. I found **one issue that must be fixed** before merging. --- ### CRITICAL: Documentation Corruption **src/eval.rs:187** and **src/eval.rs:912** - Doc comments are corrupted with garbled backticks: ```rust /// Called as `handler(address_env``command_string`ng)`return_code`code. ``` Should be something like: ```rust /// Called as `handler(address_environment, command_string)` returning `Option<i32>`. ``` This appears in two locations: - Line 187: Field documentation in the `Evaluator` struct - Line 912: Method documentation for `set_command_handler()` **Impact:** Rustdoc output will be malformed. This affects users trying to understand the API. --- ### Correctness Analysis ✓ **ERROR/FAILURE semantics (lines 491-548):** - ERROR correctly fires when `rc != 0` from either custom handler or shell (lines 520-531) ✓ - FAILURE correctly fires when shell spawn fails (line 516) ✓ - Custom handler returning `Some(rc)` bypasses shell execution - correct design ✓ - Condition info properly populated in both cases ✓ - Traps are removed after firing (REXX one-shot semantics) ✓ **RC variable handling:** - Set correctly for custom handler path (line 502) ✓ - Set correctly for shell execution success (line 512) ✓ - Set to -1 for shell spawn failure (line 515) ✓ - All paths set RC before firing traps (correct order per ANSI X3.274-1996) ✓ **Refactoring quality:** - `fire_failure_trap()` extraction improves readability ✓ - No behavior change from original code ✓ - All 556 tests pass (backward compatibility confirmed) ✓ --- ### Safety Analysis ✓ **No panics on malformed input:** - Custom handler could panic, but that's caller's responsibility - this is acceptable for an embedding API ✓ - `status.code()` returns None is handled with `unwrap_or(-1)` (line 511) ✓ - Early return prevents double trap firing ✓ **Type safety:** - `Box<dyn FnMut(&str, &str) -> Option<i32>>` signature is appropriate ✓ - `ref mut` prevents moving the handler ✓ - `#[allow(clippy::type_complexity)]` is justified here ✓ --- ### REXX Semantics Compliance ✓ Per ANSI X3.274-1996 and test suite analysis: - **ERROR condition**: Fires when command executes but returns non-zero exit code ✓ - **FAILURE condition**: Fires when command cannot be executed (spawn failure) ✓ - **RC variable**: Set before condition traps fire ✓ - **Trap auto-disable**: Both ERROR and FAILURE traps removed after firing ✓ - **Condition info**: Populated with correct condition, description, instruction, status ✓ - **ADDRESS environment**: Passed to handler correctly (line 494) ✓ Test coverage from `tests/phase8.rs` confirms all behaviors are preserved. --- ### Design Notes (not issues) 1. **Custom handler semantics**: Handler returns `Some(rc)` to handle command, `None` to fall through to shell. Clean design that avoids breaking changes. 2. **Reentrancy**: Handler is `FnMut`, allowing state. Documentation doesn't address reentrancy (e.g., if handler triggers INTERPRET that calls another command). This is probably fine for initial use case. 3. **FAILURE from custom handler**: If a custom handler wants to signal "I couldn't execute this", it has no way to trigger FAILURE condition. Returning `Some(-1)` triggers ERROR, not FAILURE. This is probably correct - the handler either handles the command or doesn't; spawn failures are shell-specific. --- ### Summary **Must fix:** Documentation corruption at lines 187 and 912. **Otherwise:** LGTM! The implementation correctly preserves REXX semantics, handles all error conditions properly, maintains backward compatibility, and provides a clean API for embedding applications. --- **File references:** - src/eval.rs:187 (field doc) - src/eval.rs:491-548 (exec_host_command, fire_failure_trap) - src/eval.rs:910-921 (set_command_handler doc and implementation)

claude[bot] commented

2026-02-13 06:12:43 +00:00

(Migrated from github.com)

Code Review: Custom Command Handler for ADDRESS Environments

I've reviewed PR #23 which adds a custom command handler mechanism for ADDRESS environments. The implementation is generally solid and maintains backward compatibility. All 556 tests pass. Here are my findings:

✅ Correct Implementation

Proper control flow: The refactoring correctly preserves the original behavior - custom handler is tried first, then falls back to shell execution
RC variable handling: Both paths correctly set the RC variable before checking traps
ERROR/FAILURE distinction: Properly maintained - ERROR for non-zero return codes, FAILURE for execution failures
Backward compatibility: The Option<CommandHandler> field with default None ensures zero impact on existing code
Test coverage: All existing tests pass, confirming behavioral preservation

⚠️ Potential Issue: FAILURE Condition for Custom Handlers

Location: src/eval.rs:502-543

The current implementation only fires the FAILURE trap when shell execution fails (lines 526-528). However, there's an ambiguity in the contract for custom command handlers:

Current behavior:

Custom handler returns Some(rc) → sets RC, fires ERROR trap if rc ≠ 0
Custom handler returns None → falls through to shell
Shell execution fails → fires FAILURE trap

Missing case:
What if the custom handler itself experiences an execution failure (not just a non-zero return code, but an inability to execute the command at all)?

In REXX semantics per ANSI X3.274-1996:

ERROR condition: Command executed but returned non-zero
FAILURE condition: Command could not be executed at all

The custom handler API uses Option<i32> where:

Some(rc) means "I handled it, here's the return code"
None means "I don't handle this environment"

There's no way for a custom handler to signal "I should handle this but execution failed." The handler must choose between:

Returning Some(-1) (fires ERROR trap)
Panicking (violates the documented contract at line 172-176)
Handling all errors internally and always returning a code

Recommendation:

Consider whether the CommandHandler type should be:

type CommandHandler = Box<dyn FnMut(&str, &str) -> Result<Option<i32>, ()>>;

Where:

Ok(Some(rc)) = handled successfully with return code rc
Ok(None) = don't handle this environment
Err(()) = attempted to handle but execution failed (fire FAILURE)

This would allow custom handlers to properly distinguish between "command failed" (ERROR) and "couldn't execute command" (FAILURE).

Alternatively, document explicitly in the handler contract that handlers must return Some(-1) for execution failures and accept that FAILURE traps won't fire for custom handler execution failures. This is a reasonable design choice if custom handlers are expected to be well-behaved.

📝 Minor Observations

Line 505: self.env.address().to_string() - unavoidable allocation, but custom handlers that don't use the address parameter will pay this cost. Consider if the address should be passed by reference.
Documentation clarity: The panic warning at lines 172-176 is good, but consider adding a note about FAILURE vs ERROR semantics and when handlers should return which codes.

🔍 Safety Check

✅ No panics on malformed input (handler panics are caller's responsibility per docs)
✅ No unsafe code introduced
✅ Proper ownership with ref mut handler for FnMut
✅ No unnecessary clones in hot path (just the address string)

Verdict

The implementation is functionally correct for the stated use case (allowing embedding applications to intercept commands). The FAILURE condition ambiguity is a design question rather than a bug - it depends on whether custom handlers need the ability to signal execution failures separately from non-zero return codes.

If the current design is intentional (handlers always "succeed" in executing, even if they return non-zero codes), then this is good to merge as-is. Otherwise, consider the API refinement suggested above.

Recommendation: Merge as-is if custom handlers are trusted to handle failures internally, or refine the API to support FAILURE condition for handler execution failures.

## Code Review: Custom Command Handler for ADDRESS Environments I've reviewed PR #23 which adds a custom command handler mechanism for ADDRESS environments. The implementation is generally solid and maintains backward compatibility. All 556 tests pass. Here are my findings: ### ✅ Correct Implementation 1. **Proper control flow**: The refactoring correctly preserves the original behavior - custom handler is tried first, then falls back to shell execution 2. **RC variable handling**: Both paths correctly set the RC variable before checking traps 3. **ERROR/FAILURE distinction**: Properly maintained - ERROR for non-zero return codes, FAILURE for execution failures 4. **Backward compatibility**: The `Option<CommandHandler>` field with default `None` ensures zero impact on existing code 5. **Test coverage**: All existing tests pass, confirming behavioral preservation ### ⚠️ Potential Issue: FAILURE Condition for Custom Handlers **Location**: `src/eval.rs:502-543` The current implementation only fires the FAILURE trap when shell execution fails (lines 526-528). However, there's an ambiguity in the contract for custom command handlers: **Current behavior:** - Custom handler returns `Some(rc)` → sets RC, fires ERROR trap if rc ≠ 0 - Custom handler returns `None` → falls through to shell - Shell execution fails → fires FAILURE trap **Missing case:** What if the custom handler itself experiences an execution failure (not just a non-zero return code, but an inability to execute the command at all)? In REXX semantics per ANSI X3.274-1996: - **ERROR condition**: Command executed but returned non-zero - **FAILURE condition**: Command could not be executed at all The custom handler API uses `Option<i32>` where: - `Some(rc)` means "I handled it, here's the return code" - `None` means "I don't handle this environment" There's no way for a custom handler to signal "I should handle this but execution failed." The handler must choose between: 1. Returning `Some(-1)` (fires ERROR trap) 2. Panicking (violates the documented contract at line 172-176) 3. Handling all errors internally and always returning a code **Recommendation:** Consider whether the `CommandHandler` type should be: ```rust type CommandHandler = Box<dyn FnMut(&str, &str) -> Result<Option<i32>, ()>>; ``` Where: - `Ok(Some(rc))` = handled successfully with return code rc - `Ok(None)` = don't handle this environment - `Err(())` = attempted to handle but execution failed (fire FAILURE) This would allow custom handlers to properly distinguish between "command failed" (ERROR) and "couldn't execute command" (FAILURE). **Alternatively**, document explicitly in the handler contract that handlers must return `Some(-1)` for execution failures and accept that FAILURE traps won't fire for custom handler execution failures. This is a reasonable design choice if custom handlers are expected to be well-behaved. ### 📝 Minor Observations 1. **Line 505**: `self.env.address().to_string()` - unavoidable allocation, but custom handlers that don't use the address parameter will pay this cost. Consider if the address should be passed by reference. 2. **Documentation clarity**: The panic warning at lines 172-176 is good, but consider adding a note about FAILURE vs ERROR semantics and when handlers should return which codes. ### 🔍 Safety Check - ✅ No panics on malformed input (handler panics are caller's responsibility per docs) - ✅ No unsafe code introduced - ✅ Proper ownership with `ref mut handler` for FnMut - ✅ No unnecessary clones in hot path (just the address string) ### Verdict The implementation is **functionally correct** for the stated use case (allowing embedding applications to intercept commands). The FAILURE condition ambiguity is a design question rather than a bug - it depends on whether custom handlers need the ability to signal execution failures separately from non-zero return codes. If the current design is intentional (handlers always "succeed" in executing, even if they return non-zero codes), then this is good to merge as-is. Otherwise, consider the API refinement suggested above. **Recommendation**: Merge as-is if custom handlers are trusted to handle failures internally, or refine the API to support FAILURE condition for handler execution failures.