Skip to content

fix: always pass prompt via stdin to satisfy cagent exec's required prompt arg#45

Closed
derekmisler wants to merge 1 commit intodocker:mainfrom
derekmisler:just-testing-will-delete-this
Closed

fix: always pass prompt via stdin to satisfy cagent exec's required prompt arg#45
derekmisler wants to merge 1 commit intodocker:mainfrom
derekmisler:just-testing-will-delete-this

Conversation

@derekmisler
Copy link
Contributor

@derekmisler derekmisler commented Feb 17, 2026

Summary

Fixes a bug where cagent exec would fail when no prompt was provided, since the prompt argument is required. The action now always passes a prompt via stdin (even if empty) and simplifies the execution logic by removing conditional branching.

Changes

  • action.yml: Always append - to ARGS array to read prompt from stdin, and unconditionally pipe $PROMPT_INPUT (empty or not) to cagent, eliminating duplicate execution paths for timeout and non-timeout scenarios
  • review-pr/README.md: Removed example YAML configuration block (28 lines)

Breaking Changes

None

How to Test

  • Run the action with a prompt input and verify it executes successfully
  • Run the action without a prompt input and verify it no longer fails with "missing required prompt argument"

…rompt arg

Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler self-assigned this Feb 17, 2026
@derekmisler
Copy link
Contributor Author

/describe

@derekmisler derekmisler marked this pull request as ready for review February 17, 2026 15:28
@derekmisler derekmisler requested a review from a team as a code owner February 17, 2026 15:28
@docker-agent
Copy link

docker-agent bot commented Feb 17, 2026

✅ PR description has been generated and updated!

Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

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

PR Review Summary

⚠️ Critical Bug Found: Exit code capture in timeout path is incorrect.

This PR simplifies the logic to always pass prompt via stdin, which is good. However, there's a critical bug in the timeout path that will cause failed cagent executions to appear as successful.

Findings

  • 1 HIGH severity bug (exit code capture)
  • Documentation changes look good

timeout "$TIMEOUT" "$GITHUB_WORKSPACE/cagent" "${ARGS[@]}" 2>&1 | tee "$OUTPUT_FILE"
fi
printf '%s\n' "$PROMPT_INPUT" | timeout "$TIMEOUT" "$GITHUB_WORKSPACE/cagent" "${ARGS[@]}" 2>&1 | tee "$OUTPUT_FILE"
EXIT_CODE=$?
Copy link

Choose a reason for hiding this comment

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

🐛 CRITICAL BUG: Incorrect exit code capture in timeout path

The timeout path uses EXIT_CODE=$? which captures tee's exit code (the last command in the pipeline), not cagent's exit code.

The Bug:

printf '%s\n' "$PROMPT_INPUT" | timeout "$TIMEOUT" "$GITHUB_WORKSPACE/cagent" "${ARGS[@]}" 2>&1 | tee "$OUTPUT_FILE"
EXIT_CODE=$?  # BUG: Captures tee's exit (always 0), not cagent's exit

In bash, $? captures only the last command's exit code in a pipeline. Since tee is the last command and almost always exits with 0 (if file write succeeds), this will report success even when cagent fails or times out.

Impact:

  • Failed cagent commands with timeout will appear as successful (EXIT_CODE=0)
  • Timeouts (exit code 124) will be reported as 0
  • The workflow will not fail on actual cagent failures when timeout is enabled
  • Breaks error detection and workflow failure handling

The Fix:
Change line 512 to match the no-timeout path (line 518) which correctly uses ${PIPESTATUS[0]}:

EXIT_CODE=${PIPESTATUS[0]}  # Captures cagent's exit code correctly

Why PIPESTATUS[0]?

  • Pipeline: printf | timeout (cagent) | tee
  • ${PIPESTATUS[0]} = printf's exit code
  • ${PIPESTATUS[1]} = timeout/cagent's exit code ← This is what we need!
  • ${PIPESTATUS[2]} = tee's exit code
  • $? = tee's exit code (WRONG)

Actually, use ${PIPESTATUS[1]} since cagent is the second command in the pipeline:

EXIT_CODE=${PIPESTATUS[1]}

Or if you want to match the no-timeout path structure exactly, both should use the same index based on which command's exit code you're interested in. The no-timeout path uses [0] because cagent is piped from printf (making it the first meaningful command). With timeout, cagent is wrapped, so you need [1].

@derekmisler
Copy link
Contributor Author

wrong branch, go here instead: #46

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.

1 participant