Skip to content

feat: return native typed exec outcomes - #92

Merged
monatis merged 1 commit into
mainfrom
review/pr-87-native-exec-results
Jul 22, 2026
Merged

feat: return native typed exec outcomes#92
monatis merged 1 commit into
mainfrom
review/pr-87-native-exec-results

Conversation

@efecnc

@efecnc efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Status

Replacement review PR for #87. The original PR was merged accidentally and cannot be reopened by GitHub. PR #91 removed its changes from main; this PR restores the exact original diff for review and must remain open until explicitly approved for merge.

Summary

  • derive exec success and failure directly from the child process status
  • return NonZeroExit as an authoritative typed tool result while preserving legacy output text
  • prevent successful command output that resembles an exit marker from being misclassified
  • preserve large-output truncation, UTF-8 safety, and grep advisory behavior

Validation

  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all-features (440 passed, 0 failed, 6 ignored)
  • git diff --check

Original PR: #87

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes successfully refactor the exec tool to derive execution success/failure directly from child process status rather than relying on parseable output markers. The implementation introduces the typed ToolResult with NonZeroExit error code while preserving backward compatibility.

Key improvements:

  • Prevents command output from spoofing success/failure by separating process status from output content
  • Introduces ShellExecOutcome struct and execute_with_approved_mutation_typed method for typed results
  • Maintains existing execute method for backward compatibility
  • Comprehensive test coverage including spoofing prevention tests

All validation passed (440 tests, clippy, fmt) and the code is ready for merge.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

@efecnc

efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Codex review

No blocking findings.

I traced both public execution paths: the reasoning loop reaches execute_with_approved_mutation_typed, where the child process status is authoritative, while the legacy execute() surface preserves its existing text contract. Successful output containing a forged Exit code: suffix stays successful, and genuine non-zero status retains its typed NonZeroExit root cause after advisory/truncation formatting. Unix edge tests, full tests, fmt, and clippy -D warnings are clean.

Review status: ready for human approval, but leave this PR open until explicitly authorized to merge.

@monatis monatis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@monatis
monatis merged commit d4743ea into main Jul 22, 2026
2 checks passed
@monatis
monatis deleted the review/pr-87-native-exec-results branch July 23, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants