fix(mention-reply): move post-reply logic to TypeScript; fix dedup & guards#207
Merged
derekmisler merged 4 commits intoMay 12, 2026
Merged
Conversation
… inline comments Signed-off-by: Derek Misler <derek.misler@docker.com>
Contributor
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity security concern was found in the added code. The overall architecture of the new post-mention-reply module is sound — guards, dedup logic, YAML wiring, and the tsup.config.ts entry point are all correctly implemented. The inline comment below describes the gap.
… in unit tests
The two mcp-gateway tests were making real fs.promises.copyFile calls to
~/.docker/cli-plugins/docker-mcp (and fs.chmodSync / fs.promises.mkdir on
the same path), which fails with EPERM in sandboxed or restricted CI
environments.
Add a targeted vi.mock('node:fs', ...) that:
- Spreads the real module so test helpers (existsSync, mkdirSync, mkdtemp,
etc.) continue to operate against real temp dirs as before.
- Replaces only the three calls that escape into the home directory:
fs.promises.copyFile, fs.promises.mkdir, and fs.chmodSync → no-ops.
No production code changed.
Contributor
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The PR correctly moves reply-posting logic from bash to TypeScript with proper guards, and test coverage for the new module is comprehensive. Two findings in the new src/post-mention-reply/index.ts are worth addressing before merge.
Fix 1 (MEDIUM): parseInt(inReplyToId, 10) could return NaN when IN_REPLY_TO_ID is a non-numeric string, letting invalid IDs slip past Guard 5 and bypassing dedup entirely. Add Guard 6 after Guard 5: parse inReplyToId once into inReplyToIdNum and reject with an early return if the result is not a finite positive integer. Use inReplyToIdNum everywhere downstream (dedup filter and createReplyForReviewComment call) instead of re-parsing. Fix 2 (LOW): a transient octokit.paginate failure silently dropped the reply because the unhandled rejection propagated out of run(). Wrap the dedup paginate call in a try/catch. On error, log a warning and fall through to posting rather than aborting. Tests added: - Guard 6: non-numeric string 'abc' with IS_INLINE=true → skips - Guard 6: '0' and negative '-5' with IS_INLINE=true → skips - Guard 6: IS_INLINE=false with non-numeric ID → does NOT skip (guard is inline-only) - Dedup paginate throws → logs warning and posts anyway (no throw, no skip)
derekmisler
added a commit
to derekmisler/cagent-action
that referenced
this pull request
May 12, 2026
Change 1 — self-review-pr.yml: switch reusable workflow call from pinned external SHA to local path (./.github/workflows/review-pr.yml) so every PR self-review tests the branch's own workflow orchestration. Removes secrets: passthrough (not needed; workflow uses OIDC/AWS). Change 2 — test-e2e.yml: add two new e2e jobs behind the same fork-check guard pattern as test-pirate-agent: - test-mention-reply-toplevel: exercises the issue_comment (top-level @docker-agent mention) path end-to-end against PR docker#207. Writes a synthetic event, runs local .github/actions/mention-reply, runs local review-pr/mention-reply, verifies a reply was posted, then cleans up. - test-mention-reply-inline: exercises the pull_request_review_comment (inline mention) path — the scenario that was silently broken. Creates a real anchor comment on PR docker#207, synthesizes the event with the real comment ID, asserts is-inline=true and in-reply-to-id match, verifies a thread reply was posted, then cleans up. Change 3 — test-e2e-reviewer.yml (new): manual workflow_dispatch with three scenarios (full-review, top-level-mention, inline-mention). full-review calls the local review-pr.yml as a proper reusable-workflow job. Mention scenarios mirror the test-e2e.yml jobs but skip verification (manual runs are for observation).
aheritier
approved these changes
May 12, 2026
derekmisler
added a commit
to derekmisler/cagent-action
that referenced
this pull request
May 12, 2026
Fix 1 & 2: Replace invalid 'gh api --jq --argjson' with pipe to jq - In test-mention-reply-inline 'Verify inline reply' step: pipe gh api output to 'jq --argjson' instead of using invalid '--jq --argjson' flag combo - Same fix in 'Cleanup anchor comment and thread replies' step Fix 3: Replace hardcoded PR docker#207 with dynamic TEST_PR_NUMBER - Add job-level 'if: github.event_name == pull_request' guard to both test-mention-reply-toplevel and test-mention-reply-inline jobs (prevents running on push-to-main where no PR number exists) - Add job-level 'env: TEST_PR_NUMBER: ${{ github.event.pull_request.number }}' to both jobs - Update all synthetic event JSON, gh api endpoint URLs, and assertions to use $TEST_PR_NUMBER - Remove hardcoded default '207' from test-e2e-reviewer.yml workflow_dispatch input Fix 4: Revert self-review-pr.yml local ref back to pinned SHA - Restore 'uses: docker/cagent-action@f208610 # v1.5.3' - Local './.github/workflows/review-pr.yml' ref is resolved from default branch (main), not the PR branch, so it doesn't achieve dogfooding
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move the post-reply logic from bash (in the agent prompt) to a dedicated TypeScript module. This simplifies the agent's job, fixes deduplication for inline replies, and adds proper guards to prevent posting when secrets are detected or required routing variables are missing.
Changes
post-mention-replymodule — TypeScript entrypoint that handles posting agent replies to GitHub. Replaces inline bashgh apicalls with proper error handling and deduplication.owner,repo,pr-number,is-inline, andin-reply-to-idto the post step, enabling the framework to route replies correctly.jqpatterns. Agent now just writes its reply and ends with the marker; the framework posts it automatically.Test plan