Skip to content

fix(review-pr): download artifact directly in reply-to-feedback to bypass secret masking#167

Merged
derekmisler merged 2 commits into
mainfrom
fix/reply-to-feedback-secret-masking
May 4, 2026
Merged

fix(review-pr): download artifact directly in reply-to-feedback to bypass secret masking#167
derekmisler merged 2 commits into
mainfrom
fix/reply-to-feedback-secret-masking

Conversation

@docker-agent
Copy link
Copy Markdown
Contributor

Problem

GitHub Actions' secret masking silently drops job outputs whose values resemble secrets. The comment-json output from the resolve-context job was being suppressed with the annotation:

Skip output 'comment-json' since it may contain secret.

This caused the reply-to-feedback job to always enter the else branch and fail with:

No comment data available

Failing run: https://github.com/docker/cagent-action/actions/runs/25337752257

Fix

Instead of relying on needs.resolve-context.outputs.comment-json (which is silently dropped), the reply-to-feedback job now downloads the pr-review-context artifact directly from inputs.trigger-run-id — the same artifact that was already successfully produced by the trigger workflow.

Changes in reply-to-feedback job

  • Add actions: read permission — required for cross-run artifact download
  • New Setup credentials step (first step, gated on trigger-run-id != '') — fetches GITHUB_APP_TOKEN via OIDC before the artifact download needs it
  • New Download trigger context step — downloads pr-review-context artifact directly from the trigger run using actions/download-artifact@v8.0.1, bypassing job-output secret masking entirely
  • Updated Parse comment context step — checks /tmp/context/comment.json first (new trigger path), then falls through to the direct event path; removes COMMENT_JSON/CTX_PR_NUMBER/CTX_PR_HEAD_SHA from the step's env block

Not changed

The resolve-context job and its comment-json output are unchanged for backward compatibility with any other callers.

The standalone reply-to-feedback.yml workflow is not touched — it already works correctly.

Testing

  • actionlint: no errors
  • YAML syntax: valid

…pass secret masking

GitHub Actions' secret masking silently drops job outputs whose values
resemble secrets. The `comment-json` output from `resolve-context` was
being suppressed with annotation 'Skip output comment-json since it may
contain secret', causing `reply-to-feedback` to always hit the 'No
comment data available' error branch.

Changes in `reply-to-feedback` job:
- Add `actions: read` permission (needed for cross-run artifact download)
- Add early `Setup credentials` step (gated on trigger-run-id) so
  GITHUB_APP_TOKEN is available before the artifact download
- Add `Download trigger context` step that downloads the artifact
  directly from `inputs.trigger-run-id` using download-artifact v8.0.1,
  bypassing the job-output secret-masking entirely
- Update `Parse comment context` to check /tmp/context/comment.json
  first (new trigger path), then fall through to the direct event path
- Remove COMMENT_JSON / CTX_PR_NUMBER / CTX_PR_HEAD_SHA from the step
  env block (no longer the primary path)

The `resolve-context` job and its `comment-json` output are unchanged
for backward compatibility.

Fixes: https://github.com/docker/cagent-action/actions/runs/25337752257

Assisted-By: docker-agent
@docker-agent docker-agent requested a review from a team as a code owner May 4, 2026 19:14
@docker-agent docker-agent requested a review from derekmisler May 4, 2026 19:14
Copy link
Copy Markdown
Contributor

@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.

Assessment: 🟡 NEEDS ATTENTION

The artifact-download approach correctly bypasses GitHub Actions' secret masking on job outputs. However, there is a logic inconsistency in the fallback handling.

1 NOTABLE finding (see inline comment).

Comment thread .github/workflows/review-pr.yml Outdated
Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

Two medium findings and one low finding in the new steps added to reply-to-feedback. The core fix (artifact download bypassing secret masking) is correct and the step ordering is right.

Comment thread .github/workflows/review-pr.yml Outdated
Comment thread .github/workflows/review-pr.yml Outdated
Comment thread .github/workflows/review-pr.yml
F1: Delete dead legacy elif COMMENT_JSON branch — env vars COMMENT_JSON,
CTX_PR_NUMBER, CTX_PR_HEAD_SHA were already removed from the step env block
so the condition was permanently unreachable.

F2: Guard pr_number.txt read with a hard-fail so a missing file produces
an actionable ::error:: instead of silently writing an empty pr-number
output and causing cryptic downstream failures.

F3: Add 'Verify token for cross-run artifact download' step (gated on
trigger-run-id) after 'Setup credentials', matching the same guard that
already exists in the resolve-context job, for consistency and early
failure on OIDC/Secrets Manager misconfiguration.

Assisted-By: docker-agent
@docker-agent
Copy link
Copy Markdown
Contributor Author

All three review items addressed in 136d4f8:

F1 — Deleted the dead elif [ -n "$COMMENT_JSON" ] block. Since COMMENT_JSON, CTX_PR_NUMBER, and CTX_PR_HEAD_SHA were already removed from the step's env: block, the condition could never be true; the branch was unreachable dead code.

F2 — Added a hard-fail guard on pr_number.txt:

echo "pr-number=$(cat /tmp/context/pr_number.txt || { echo '::error::pr_number.txt missing from artifact'; exit 1; })" >> $GITHUB_OUTPUT

A missing file now surfaces a clear ::error:: annotation instead of silently writing an empty output.

F3 — Added a Verify token for cross-run artifact download step (gated on inputs.trigger-run-id != '') immediately after Setup credentials, matching the identical guard already present in the resolve-context job. Ensures OIDC/Secrets Manager misconfiguration fails early with a helpful error message rather than a cryptic download failure.

actionlint: clean ✅

@derekmisler derekmisler merged commit 8088333 into main May 4, 2026
14 checks passed
@derekmisler derekmisler deleted the fix/reply-to-feedback-secret-masking branch May 4, 2026 19:34
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