Some checks failed
CI / Rustfmt (push) Successful in 23s
CI / Cargo Audit & Deny (push) Successful in 30s
CI / Web Blocking Checks (push) Successful in 48s
CI / Security Blocking Checks (push) Successful in 8s
CI / Clippy (push) Failing after 1m55s
CI / Web Advisory Checks (push) Successful in 35s
CI / Security Advisory Checks (push) Successful in 37s
CI / Tests (push) Successful in 8m5s
238 lines
8.8 KiB
Markdown
238 lines
8.8 KiB
Markdown
# 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 EOF
|
|
while IFS= read -r line; do
|
|
case "$line" in
|
|
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!'" | 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 |