working out the worker/execution interface
This commit is contained in:
243
work-summary/2026-02-action-execution-fixes.md
Normal file
243
work-summary/2026-02-action-execution-fixes.md
Normal file
@@ -0,0 +1,243 @@
|
||||
# Action Execution Fixes - 2026-02-04
|
||||
|
||||
## Summary
|
||||
|
||||
Fixed three critical issues with action execution identified during testing:
|
||||
|
||||
1. **Correct result format for stdout/stderr** - stdout content included directly in result JSON, stderr written to log file with path included only if non-empty/non-whitespace
|
||||
2. **Fixed parameter name case sensitivity** - Worker now preserves exact parameter names from schema (lowercase) instead of uppercasing them
|
||||
3. **Eliminated jq dependency in core shell actions** - Converted echo, noop, and sleep actions to use pure POSIX shell with DOTENV parameter format
|
||||
|
||||
## Issues Addressed
|
||||
|
||||
### Issue 1: Correct Result Format for stdout/stderr
|
||||
|
||||
**Problem**: Initial implementation had stdout/stderr content duplicated in both result JSON and log files, causing confusion about which was the source of truth.
|
||||
|
||||
**Correct Specification**:
|
||||
- **stdout** → included directly in the result JSON payload (primary output)
|
||||
- **stderr** → written to log file; log file path included in result JSON ONLY if stderr is non-empty and non-whitespace
|
||||
- Both stdout and stderr are always written to artifact log files for persistence
|
||||
|
||||
**Solution**: Modified `executor.rs` to implement correct result format:
|
||||
- `handle_execution_success()`: Include stdout content in result, stderr_log path only if stderr has content
|
||||
- `handle_execution_failure()`: Include stdout content in result, stderr_log path only if stderr has content
|
||||
- Result structure:
|
||||
```json
|
||||
{
|
||||
"exit_code": 0,
|
||||
"duration_ms": 5,
|
||||
"succeeded": true,
|
||||
"stdout": "Action output here\n",
|
||||
"stderr_log": "/tmp/attune/artifacts/execution_123/stderr.log" // only if stderr non-empty
|
||||
}
|
||||
```
|
||||
|
||||
**Benefits**:
|
||||
- stdout immediately available in result for quick access
|
||||
- stderr log path provided only when there's actual error output to review
|
||||
- Clear separation: stdout for primary output, stderr for diagnostics
|
||||
- Artifact log files preserve complete history
|
||||
|
||||
### Issue 2: Parameter Name Case Sensitivity
|
||||
|
||||
**Problem**: The worker was converting parameter names to uppercase in DOTENV format:
|
||||
```rust
|
||||
let key_upper = key.to_uppercase(); // Wrong!
|
||||
lines.push(format!("{}='{}'", key_upper, escaped_value));
|
||||
```
|
||||
|
||||
This broke shell scripts that expected lowercase parameter names matching the schema:
|
||||
- Schema: `message` (lowercase)
|
||||
- Worker sent: `MESSAGE='value'` (uppercase)
|
||||
- Script expected: `message='value'` (lowercase)
|
||||
|
||||
**Root Cause**: Parameter names are case-sensitive in shell. The action YAML schemas define exact parameter names (e.g., `message`, `exit_code`, `seconds`) which must be preserved exactly.
|
||||
|
||||
**Solution**: Modified `parameter_passing.rs` to preserve original parameter names:
|
||||
```rust
|
||||
// Before
|
||||
let key_upper = key.to_uppercase();
|
||||
lines.push(format!("{}='{}'", key_upper, escaped_value));
|
||||
|
||||
// After
|
||||
lines.push(format!("{}='{}'", key, escaped_value));
|
||||
```
|
||||
|
||||
Updated tests to expect lowercase parameter names.
|
||||
|
||||
**Impact**:
|
||||
- Parameter names now match exactly what's defined in action schemas
|
||||
- Shell scripts can use correct lowercase variable names
|
||||
- No transformation applied - what's in the schema is what gets passed
|
||||
|
||||
### Issue 3: jq Dependency in Core Shell Actions
|
||||
|
||||
**Problem**: Core shell actions (echo, noop, sleep) used `jq` for JSON parsing:
|
||||
```bash
|
||||
MESSAGE=$(echo "$INPUT" | jq -r '.message // ""')
|
||||
```
|
||||
|
||||
But `jq` is not available in the Docker worker containers, causing execution failures:
|
||||
```
|
||||
exit_code: 127
|
||||
error: "/opt/attune/packs/core/actions/echo.sh: line 12: jq: command not found"
|
||||
```
|
||||
|
||||
**Principle**: Built-in shell scripts should be independent from the installed system and prefer POSIX-compliant shell operators over external utilities.
|
||||
|
||||
**Solution**:
|
||||
1. Rewrote actions to parse DOTENV format using pure POSIX shell:
|
||||
- `echo.sh`: Parse `message` parameter
|
||||
- `noop.sh`: Parse `message` and `exit_code` parameters
|
||||
- `sleep.sh`: Parse `seconds` and `message` parameters
|
||||
|
||||
2. Changed parameter format from `json` to `dotenv` in action YAML files:
|
||||
- `echo.yaml`
|
||||
- `noop.yaml`
|
||||
- `sleep.yaml`
|
||||
|
||||
3. Implementation uses pure POSIX shell features:
|
||||
- `#!/bin/sh` instead of `#!/bin/bash`
|
||||
- No bashisms (no `set -o pipefail`, no `[[` tests)
|
||||
- Pattern matching with `case` statements
|
||||
- String manipulation with parameter expansion (`${var#prefix}`, `${var%suffix}`)
|
||||
|
||||
**Parsing Logic**:
|
||||
```sh
|
||||
# Read DOTENV-formatted parameters from stdin until delimiter
|
||||
while IFS= read -r line; do
|
||||
case "$line" in
|
||||
*"---ATTUNE_PARAMS_END---"*)
|
||||
break
|
||||
;;
|
||||
message=*)
|
||||
message="${line#message=}"
|
||||
# Remove quotes if present
|
||||
case "$message" in
|
||||
\"*\") message="${message#\"}" ; message="${message%\"}" ;;
|
||||
\'*\') message="${message#\'}" ; message="${message%\'}" ;;
|
||||
esac
|
||||
;;
|
||||
esac
|
||||
done
|
||||
```
|
||||
|
||||
**Benefits**:
|
||||
- Zero external dependencies (no jq, yq, or other tools required)
|
||||
- Works in minimal container environments
|
||||
- Faster execution (no subprocess spawning for jq)
|
||||
- More portable across different shell environments
|
||||
- POSIX-compliant for maximum compatibility
|
||||
|
||||
**Note**: Complex actions like `http_request` legitimately need JSON and external tools (curl, jq) for sophisticated processing. Only simple utility actions were converted to DOTENV format.
|
||||
|
||||
## Additional Improvements
|
||||
|
||||
### Enhanced Debug Logging
|
||||
|
||||
Added detailed logging to help diagnose execution status issues:
|
||||
|
||||
```rust
|
||||
debug!(
|
||||
"Execution {} result: exit_code={}, error={:?}, is_success={}",
|
||||
execution_id,
|
||||
result.exit_code,
|
||||
result.error,
|
||||
is_success
|
||||
);
|
||||
```
|
||||
|
||||
This helps verify that `is_success()` correctly evaluates to false when exit_code is non-zero.
|
||||
|
||||
## Files Modified
|
||||
|
||||
### Worker Service
|
||||
- `crates/worker/src/executor.rs` - Removed stdout/stderr content from result JSON, added debug logging
|
||||
- `crates/worker/src/runtime/parameter_passing.rs` - Fixed case sensitivity (preserve original parameter names)
|
||||
|
||||
### Core Pack Actions (Shell Scripts)
|
||||
- `packs/core/actions/echo.sh` - Rewrote to use pure POSIX shell + DOTENV parsing
|
||||
- `packs/core/actions/noop.sh` - Rewrote to use pure POSIX shell + DOTENV parsing
|
||||
- `packs/core/actions/sleep.sh` - Rewrote to use pure POSIX shell + DOTENV parsing
|
||||
|
||||
### Core Pack Action Definitions (YAML)
|
||||
- `packs/core/actions/echo.yaml` - Changed parameter_format from json to dotenv
|
||||
- `packs/core/actions/noop.yaml` - Changed parameter_format from json to dotenv
|
||||
- `packs/core/actions/sleep.yaml` - Changed parameter_format from json to dotenv
|
||||
|
||||
## Testing
|
||||
|
||||
Manual testing verified:
|
||||
1. POSIX shell scripts correctly parse lowercase parameter names
|
||||
2. Quote handling works (single quotes, double quotes, no quotes)
|
||||
3. Empty parameters handled correctly
|
||||
4. Parameter validation works (numeric checks, range checks)
|
||||
|
||||
Example test:
|
||||
```sh
|
||||
echo "message='Hello World!'" > test.txt
|
||||
echo "---ATTUNE_PARAMS_END---" >> test.txt
|
||||
cat test.txt | sh packs/core/actions/echo.sh
|
||||
# Output: Hello World!
|
||||
```
|
||||
|
||||
## Design Decisions
|
||||
|
||||
### Why DOTENV Over JSON for Simple Actions?
|
||||
|
||||
1. **Dependency-free**: No need for jq, python, or other parsers
|
||||
2. **Simple actions = simple format**: Key-value pairs are natural for basic parameters
|
||||
3. **Shell-friendly**: Direct variable assignment pattern familiar to shell scripters
|
||||
4. **Portable**: Pure POSIX shell works everywhere
|
||||
|
||||
### Why Keep JSON for Complex Actions?
|
||||
|
||||
Actions like `http_request`, `download_packs`, and `build_pack_envs` legitimately need:
|
||||
- Nested data structures (headers, query params)
|
||||
- Array handling
|
||||
- Complex object manipulation
|
||||
- Integration with JSON APIs
|
||||
|
||||
For these, JSON + jq is the right tool.
|
||||
|
||||
### Parameter Format Selection Guidelines
|
||||
|
||||
**Use DOTENV when**:
|
||||
- Action has simple key-value parameters (strings, numbers, booleans)
|
||||
- No nested structures needed
|
||||
- Want to avoid external dependencies
|
||||
- Writing pure shell scripts
|
||||
|
||||
**Use JSON when**:
|
||||
- Need nested objects or arrays
|
||||
- Integrating with JSON APIs
|
||||
- Complex data manipulation required
|
||||
- Using Python/Node.js runtimes that parse JSON natively
|
||||
|
||||
## Impact
|
||||
|
||||
### Positive
|
||||
- ✅ Core actions now work in minimal Docker environments
|
||||
- ✅ Faster execution (no subprocess spawning for jq)
|
||||
- ✅ Cleaner result JSON (no log content duplication)
|
||||
- ✅ Correct parameter name handling (case-sensitive)
|
||||
- ✅ Better debugging (enhanced logging)
|
||||
|
||||
### Compatibility
|
||||
- ⚠️ Breaking change: Actions using lowercase parameter names will now receive them correctly
|
||||
- ⚠️ Breaking change: Result JSON format changed - stdout now in result, stderr_log only if non-empty
|
||||
- ✅ No impact on JSON-based actions (http_request, etc.)
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. Test updated actions in Docker environment
|
||||
2. Update any custom packs that relied on uppercase parameter names
|
||||
3. Update Web UI to display stdout from result JSON, stderr via log file endpoint if stderr_log path present
|
||||
4. Consider adding parameter format documentation to pack development guide
|
||||
|
||||
## Related Documentation
|
||||
|
||||
- `docs/packs/pack-structure.md` - Parameter delivery and format options
|
||||
- `docs/architecture/worker-service.md` - Action execution flow
|
||||
- `AGENTS.md` - Project rules on parameter passing and shell scripts
|
||||
Reference in New Issue
Block a user