Fix multi-turn history and hand dependency versions#26
Conversation
📝 WalkthroughWalkthroughThis PR adds minimum version checking for hand requirements. It introduces a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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
🧹 Nitpick comments (1)
crates/clawreform-api/static/js/pages/hands.js (1)
212-222: Consider template literals for improved readability.Both helper functions use string concatenation. Template literals would be more readable and maintainable.
♻️ Optional refactor using template literals
getRequirementCheckLabel(req) { if (!req) return ''; - if (req.min_version) return req.check_value + ' >= ' + req.min_version; + if (req.min_version) return `${req.check_value || ''} >= ${req.min_version}`; return req.check_value || ''; }, getRequirementMetLabel(req) { if (!req) return 'Detected on your system'; - if (req.min_version) return 'Detected on your system and meets >= ' + req.min_version; + if (req.min_version) return `Detected on your system and meets >= ${req.min_version}`; return 'Detected on your system'; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/clawreform-api/static/js/pages/hands.js` around lines 212 - 222, Replace string concatenation in getRequirementCheckLabel and getRequirementMetLabel with template literals for readability: in getRequirementCheckLabel use a template like `${req.check_value} >= ${req.min_version}` when req.min_version is present and return `${req.check_value}` (or empty) otherwise; in getRequirementMetLabel use a template like `Detected on your system and meets >= ${req.min_version}` when req.min_version exists and keep the existing fallback string when it does not. Ensure null/undefined guards (if (!req)) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/clawreform-api/static/js/pages/hands.js`:
- Around line 212-216: The getRequirementCheckLabel function may emit "undefined
>= x" if req.min_version is set but req.check_value is missing; update
getRequirementCheckLabel to defensively check that req.check_value (or
req.checkValue) is a valid non-null/undefined string before concatenating with
req.min_version and fall back to an empty string or a safe placeholder when
check_value is absent; specifically modify the logic in getRequirementCheckLabel
to test req.check_value (or use String(req.check_value) only when not
null/undefined) before returning "req.check_value + ' >= ' + req.min_version",
otherwise return '' or req.min_version alone as appropriate.
In `@crates/clawreform-hands/src/registry.rs`:
- Around line 317-330: query_binary_version currently uses
std::process::Command::output() and parses stdout/stderr even when the child
exited non-zero and runs synchronously inside async callers like
check_requirements()/list_hands(); update query_binary_version to validate the
child's exit status (return None or an Err when status.success() is false)
before using stdout/stderr, and protect blocking/spawn operations by running the
process under Tokio: either switch to tokio::process::Command and call it inside
tokio::time::timeout(...) with an appropriate duration in the async callers, or
wrap the existing std::process::Command invocation in
tokio::task::spawn_blocking and apply tokio::time::timeout around the spawned
task to avoid wedging the runtime.
---
Nitpick comments:
In `@crates/clawreform-api/static/js/pages/hands.js`:
- Around line 212-222: Replace string concatenation in getRequirementCheckLabel
and getRequirementMetLabel with template literals for readability: in
getRequirementCheckLabel use a template like `${req.check_value} >=
${req.min_version}` when req.min_version is present and return
`${req.check_value}` (or empty) otherwise; in getRequirementMetLabel use a
template like `Detected on your system and meets >= ${req.min_version}` when
req.min_version exists and keep the existing fallback string when it does not.
Ensure null/undefined guards (if (!req)) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dab5c69d-52b8-4076-bbc3-6df461ac7e8c
📒 Files selected for processing (6)
crates/clawreform-api/src/routes.rscrates/clawreform-api/static/index_body.htmlcrates/clawreform-api/static/js/pages/hands.jscrates/clawreform-hands/src/lib.rscrates/clawreform-hands/src/registry.rscrates/clawreform-runtime/src/agent_loop.rs
| getRequirementCheckLabel(req) { | ||
| if (!req) return ''; | ||
| if (req.min_version) return req.check_value + ' >= ' + req.min_version; | ||
| return req.check_value || ''; | ||
| }, |
There was a problem hiding this comment.
Guard against undefined check_value in version display.
Line 214 concatenates req.check_value without verifying it exists. If req.min_version is truthy but req.check_value is undefined or null, the UI would display "undefined >= 1.2.3" or "null >= 1.2.3".
🛡️ Proposed fix to add defensive check
getRequirementCheckLabel(req) {
if (!req) return '';
- if (req.min_version) return req.check_value + ' >= ' + req.min_version;
+ if (req.min_version) return (req.check_value || '') + ' >= ' + req.min_version;
return req.check_value || '';
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/clawreform-api/static/js/pages/hands.js` around lines 212 - 216, The
getRequirementCheckLabel function may emit "undefined >= x" if req.min_version
is set but req.check_value is missing; update getRequirementCheckLabel to
defensively check that req.check_value (or req.checkValue) is a valid
non-null/undefined string before concatenating with req.min_version and fall
back to an empty string or a safe placeholder when check_value is absent;
specifically modify the logic in getRequirementCheckLabel to test
req.check_value (or use String(req.check_value) only when not null/undefined)
before returning "req.check_value + ' >= ' + req.min_version", otherwise return
'' or req.min_version alone as appropriate.
| let output = std::process::Command::new(name).args(&args).output().ok()?; | ||
| let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); | ||
| let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); | ||
|
|
||
| if stdout.is_empty() && stderr.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| Some(match (stdout.is_empty(), stderr.is_empty()) { | ||
| (false, true) => stdout, | ||
| (true, false) => stderr, | ||
| (false, false) => format!("{stdout}\n{stderr}"), | ||
| (true, true) => return None, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get its size
wc -l crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 105
🏁 Script executed:
# Read the specific lines mentioned in the review plus surrounding context
sed -n '300,340p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 1444
🏁 Script executed:
# Search for the function containing these lines and understand the context
cd crates/clawreform-hands && grep -n "min_version\|fn.*version" src/registry.rs | head -20Repository: aegntic/clawreform
Length of output: 978
🏁 Script executed:
# Read the extract_version function to understand how it parses version strings
sed -n '333,360p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 765
🏁 Script executed:
# Search for tests related to version handling and min_version
grep -n "test\|#\[test\]" crates/clawreform-hands/src/registry.rs | tail -20Repository: aegntic/clawreform
Length of output: 606
🏁 Script executed:
# Look for any timeout or threading code in the file
grep -n "timeout\|thread\|spawn\|await" crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 231
🏁 Script executed:
# Read the version-related tests to understand the expected behavior
sed -n '583,630p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 1267
🏁 Script executed:
# Search for how binary_meets_min_version is used in the codebase
grep -rn "binary_meets_min_version" crates/clawreform-hands/src/Repository: aegntic/clawreform
Length of output: 293
🏁 Script executed:
# Check if there are any async/await patterns or threading in the broader codebase
grep -n "async\|tokio\|thread\|spawn" crates/clawreform-hands/src/*.rs | head -20Repository: aegntic/clawreform
Length of output: 605
🏁 Script executed:
# Look at the file to understand the broader context where binary_meets_min_version is called
sed -n '255,275p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 840
🏁 Script executed:
# Read more context around check_requirement to understand the usage
sed -n '200,280p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 2767
🏁 Script executed:
# Look for where check_requirement is called to understand if there are timeout/threading concerns
grep -rn "check_requirement" crates/clawreform-hands/src/ | head -15Repository: aegntic/clawreform
Length of output: 1045
🏁 Script executed:
# Run cargo build for the workspace
cd crates/clawreform-hands && cargo build --workspace --lib 2>&1 | tail -50Repository: aegntic/clawreform
Length of output: 662
🏁 Script executed:
# Run cargo test
cd crates/clawreform-hands && cargo test --workspace 2>&1 | tail -100Repository: aegntic/clawreform
Length of output: 148
🏁 Script executed:
# Try a simpler cargo build without syncing the full toolchain
cd crates/clawreform-hands && cargo build --lib 2>&1 | head -50Repository: aegntic/clawreform
Length of output: 7251
🏁 Script executed:
# Try cargo test with a timeout to see if there's a hang issue
timeout 30 cargo test --lib binary_meets_min_version 2>&1 || echo "Exit code: $?"Repository: aegntic/clawreform
Length of output: 122
🏁 Script executed:
# Let me examine what happens when a command exits with non-zero status
# Create a test scenario to understand the extract_version behavior
cat > /tmp/test_version.txt << 'EOF'
Example error outputs that might contain version-like patterns:
"Error: option requires argument 1"
"Usage: mybin --version
Version 2.0.0"
"Failed at line 5"
"invalid version format 3.14"
EOF
cat /tmp/test_version.txtRepository: aegntic/clawreform
Length of output: 249
🏁 Script executed:
# Check if there are any error handling tests or examples that show the concern
grep -A 10 -B 5 "output.status\|exit\|non.zero\|failed" crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 44
🏁 Script executed:
# Look at the public API of the Hand to understand if this runs on the main thread
sed -n '176,190p' crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 565
🏁 Script executed:
# Search for "readiness" to understand the timeout concern mentioned in review
grep -rn "readiness\|timeout\|stall\|wedge" crates/clawreform-hands/src/Repository: aegntic/clawreform
Length of output: 3646
🏁 Script executed:
# Check if there are comments about the check_requirements being called from async/API context
grep -B 5 -A 5 "pub fn check_requirements" crates/clawreform-hands/src/registry.rsRepository: aegntic/clawreform
Length of output: 513
🏁 Script executed:
# Look at the HandResult error types to understand error handling
grep -A 10 "enum HandError\|type HandResult" crates/clawreform-hands/src/lib.rs | head -30Repository: aegntic/clawreform
Length of output: 741
🏁 Script executed:
# Let me check if there's any caller or context that would indicate async/API usage
grep -rn "check_requirements\|readiness" crates/ --include="*.rs" | grep -v "test\|crates/clawreform-hands"Repository: aegntic/clawreform
Length of output: 832
🏁 Script executed:
# Check the broader project structure to understand if this is used in an async context
ls -la crates/Repository: aegntic/clawreform
Length of output: 1042
🏁 Script executed:
# Look for kernel or server code that might call into this
find crates -name "*.rs" -type f | head -20 | xargs grep -l "check_requirements" 2>/dev/nullRepository: aegntic/clawreform
Length of output: 141
🏁 Script executed:
# Look at the API routes to see if they're async and how check_requirements is used
sed -n '3045,3065p' crates/clawreform-api/src/routes.rsRepository: aegntic/clawreform
Length of output: 852
🏁 Script executed:
# Check the async context
grep -B 5 "check_requirements" crates/clawreform-api/src/routes.rs | head -40Repository: aegntic/clawreform
Length of output: 928
🏁 Script executed:
# Check if there's any timeout handling in the API layer
grep -n "timeout\|tokio\|blocking" crates/clawreform-api/src/routes.rs | head -20Repository: aegntic/clawreform
Length of output: 1349
🏁 Script executed:
# Look at channel_bridge to understand the readiness check context
sed -n '235,250p' crates/clawreform-api/src/channel_bridge.rsRepository: aegntic/clawreform
Length of output: 704
🏁 Script executed:
# Let's see if there's an async wrapper or blocking implementation already in the codebase
grep -A 10 "tokio::task::spawn_blocking\|blocking" crates/clawreform-api/src/routes.rs | head -30Repository: aegntic/clawreform
Length of output: 44
🏁 Script executed:
# Check how other binaries are invoked with timeout protection in the API
sed -n '3300,3330p' crates/clawreform-api/src/routes.rsRepository: aegntic/clawreform
Length of output: 1234
🏁 Script executed:
# Verify the context: are these API handlers marked as async?
grep -B 3 "check_requirements(&d.id)" crates/clawreform-api/src/routes.rs | head -15Repository: aegntic/clawreform
Length of output: 188
🏁 Script executed:
# Let's see the full context of how check_requirements is called in the async handlers
sed -n '3040,3075p' crates/clawreform-api/src/routes.rsRepository: aegntic/clawreform
Length of output: 1394
🏁 Script executed:
# Check if this is indeed an async handler
grep -B 15 "check_requirements(&d.id)" crates/clawreform-api/src/routes.rs | grep -E "async|fn |#\[" | head -20Repository: aegntic/clawreform
Length of output: 144
🏁 Script executed:
# Verify the function signature to understand if it's an async route handler
sed -n '3000,3050p' crates/clawreform-api/src/routes.rs | head -60Repository: aegntic/clawreform
Length of output: 1781
Check exit status before parsing process output, and protect from hung binaries in async context.
output().ok()? only filters spawn failures. A child that exits non-zero still has its stdout/stderr parsed, so error/usage text with digits can incorrectly satisfy min_version. Additionally, check_requirements() is called from async handlers (e.g., list_hands()) but runs synchronously, so a wedged or slow binary blocks the tokio runtime. Needs both exit code validation and timeout protection.
Required fixes
Exit status check in query_binary_version:
- let output = std::process::Command::new(name).args(&args).output().ok()?;
+ let output = std::process::Command::new(name).args(&args).output().ok()?;
+ if !output.status.success() {
+ return None;
+ }Timeout protection: Use tokio::time::timeout() with tokio::process::Command in the async callers, similar to the installation handler pattern at line 3302, or move to tokio::task::spawn_blocking().
📝 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.
| let output = std::process::Command::new(name).args(&args).output().ok()?; | |
| let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); | |
| let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); | |
| if stdout.is_empty() && stderr.is_empty() { | |
| return None; | |
| } | |
| Some(match (stdout.is_empty(), stderr.is_empty()) { | |
| (false, true) => stdout, | |
| (true, false) => stderr, | |
| (false, false) => format!("{stdout}\n{stderr}"), | |
| (true, true) => return None, | |
| }) | |
| let output = std::process::Command::new(name).args(&args).output().ok()?; | |
| if !output.status.success() { | |
| return None; | |
| } | |
| let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); | |
| let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); | |
| if stdout.is_empty() && stderr.is_empty() { | |
| return None; | |
| } | |
| Some(match (stdout.is_empty(), stderr.is_empty()) { | |
| (false, true) => stdout, | |
| (true, false) => stderr, | |
| (false, false) => format!("{stdout}\n{stderr}"), | |
| (true, true) => return None, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/clawreform-hands/src/registry.rs` around lines 317 - 330,
query_binary_version currently uses std::process::Command::output() and parses
stdout/stderr even when the child exited non-zero and runs synchronously inside
async callers like check_requirements()/list_hands(); update
query_binary_version to validate the child's exit status (return None or an Err
when status.success() is false) before using stdout/stderr, and protect
blocking/spawn operations by running the process under Tokio: either switch to
tokio::process::Command and call it inside tokio::time::timeout(...) with an
appropriate duration in the async callers, or wrap the existing
std::process::Command invocation in tokio::task::spawn_blocking and apply
tokio::time::timeout around the spawned task to avoid wedging the runtime.
1 participant
Preserves repaired multi-turn session history in both streaming and non-streaming agent loops by replacing the old 20-message trim with repair logging and a higher emergency cap. Adds regression tests to ensure earlier turns survive past twenty messages while still trimming only in true overflow scenarios. Also adds optional minimum-version metadata for binary hand requirements, enforces those checks in the hands registry, and exposes the requirement/version status through the API. Updates the hands setup UI to show required versions and success messaging when the installed tool satisfies the minimum.
Summary by CodeRabbit
Release Notes
New Features
Improvements