Skip to content

fixes for running the review agent locally trying to find non-existent files#159

Merged
derekmisler merged 1 commit into
docker:mainfrom
derekmisler:fixes-for-running-the-review-agent-locally-trying
Apr 29, 2026
Merged

fixes for running the review agent locally trying to find non-existent files#159
derekmisler merged 1 commit into
docker:mainfrom
derekmisler:fixes-for-running-the-review-agent-locally-trying

Conversation

@derekmisler
Copy link
Copy Markdown
Contributor

@derekmisler derekmisler commented Apr 29, 2026

Summary

Fixes the PR review agent to correctly handle both GitHub Actions (CI) and local console execution modes. The agent was trying to access files in /tmp/ that don't exist when running locally, causing failures. This PR clarifies the execution context and provides separate, mode-specific instructions for file paths and diff handling.

Changes

  • Clarified execution modes: Added explicit documentation distinguishing GitHub Actions mode (GITHUB_ACTIONS=true) from console mode (GITHUB_ACTIONS empty/unset)
  • Fixed file path handling:
    • GitHub Actions mode uses /tmp/refs/ for domain guides and /tmp/drafter_chunk_*.diff for pre-split diffs
    • Console mode uses /refs/ (if available) for domain guides and generates diffs locally with git diff
  • Updated diff generation: In console mode, the agent now generates diffs itself using git diff and saves to ./pr-review.diff instead of looking for non-existent /tmp/ files
  • Improved step 0 logic: Added explicit path variable setup (REFS_DIR) based on execution mode to avoid hardcoded /tmp/ references throughout
  • Clarified drafter delegation: Separated instructions for CI vs. console modes, with explicit guidance on which files to pass and which to skip
  • Fixed domain guide lookup: Updated to use $REFS_DIR variable instead of hardcoded /tmp/refs/ path
  • Removed verifier diff paths: Clarified that verifier delegation should never include diff file paths (CI or console)
  • Added mode-specific notes: Documented that posting-format.md and risk score files only exist in GitHub Actions mode

Closes: https://github.com/docker/gordon/issues/470

@derekmisler derekmisler self-assigned this Apr 29, 2026
@derekmisler derekmisler requested a review from a team April 29, 2026 14:48
@derekmisler derekmisler marked this pull request as ready for review April 29, 2026 14:48
@derekmisler derekmisler force-pushed the fixes-for-running-the-review-agent-locally-trying branch from 4d93454 to 0a8b171 Compare April 29, 2026 14:51
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

Comment thread review-pr/agents/pr-review.yaml Outdated
**Console output mode** (GITHUB_ACTIONS empty/unset):
a. Do NOT run `ls /tmp/drafter_chunk_*.diff` — there are no pre-staged files
and `/tmp/` is irrelevant in this mode.
b. You already obtained the unified diff from `git diff "$BASE" HEAD` in step 1.
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.

[MEDIUM] $BASE variable will be empty when step 4b runs in a separate shell invocation

The console-mode instructions in step 4b tell the agent to run:

git diff "$BASE" HEAD > ./pr-review.diff

However, the agent's shell tool creates a fresh shell for every invocation — shell variables do not persist between calls. $BASE is computed in step 1's shell block, but by the time step 4b runs (a separate shell call), $BASE is unset. The shell silently expands it to an empty string, so the command becomes:

git diff  HEAD > ./pr-review.diff

which diffs against the first parent commit (HEAD~1) rather than the true merge-base. This means the drafter receives the wrong set of changes to review.

Suggested fix: Combine the merge-base computation and the git diff … > ./pr-review.diff redirect into a single shell invocation in step 4b, rather than relying on $BASE surviving from step 1. For example:

BASE=$(git merge-base upstream/main HEAD 2>/dev/null \
  || git merge-base origin/main HEAD 2>/dev/null \
  || git merge-base main HEAD 2>/dev/null)
git diff "$BASE" HEAD > ./pr-review.diff

Or instruct the agent to capture the diff in one shot during step 1:

git diff "$BASE" HEAD | tee ./pr-review.diff

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

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.

Perfect! I can confirm the fix resolves the issue. The merge-base computation and git diff are now combined in a single shell invocation (lines 81-95), so $BASE is computed and used within the same shell session. The tee also writes the diff to ./pr-review.diff for the drafter while echoing it to stdout, which is a nice touch for visibility.

Thanks for addressing this quickly!

@@ -274,12 +313,17 @@ agents:

## Domain-Specific Review Guides
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.

[LOW] Cross-mode fallback for refs directory could silently use wrong guides in CI

The drafter is instructed:

"if /tmp/refs/ does not exist, try /refs/ instead"

This fallback contradicts the strict mode separation defined elsewhere. In GitHub Actions (CI) mode, if /tmp/refs/ is unexpectedly absent, the drafter silently falls back to /refs/ — which may contain stale guides, unrelated guides, or simply not exist (silent no-op). Either way, there's no error signal and the review quality degrades without notice.

In console mode, the drafter is directed to try /tmp/refs/ first (it won't exist there), wasting a tool call before the fallback succeeds.

Suggested fix: Replace the cross-mode fallback with a fail-loud approach. The drafter already knows its mode from the diff path it was given (CI: /tmp/drafter_chunk_N.diff; console: ./pr-review.diff). Instruct it to use the correct directory directly and report an error if it is missing, rather than silently falling back to the other mode's path:

- GitHub posting mode (GITHUB_ACTIONS=true) → `/tmp/refs/`
- Console mode (GITHUB_ACTIONS empty/unset) → `/refs/`
If the expected directory does not exist, skip domain guide lookup and note it in the summary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is fixed now.

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.

Verified — the cross-mode fallback is gone. The drafter now uses mode-specific paths (/tmp/refs/ in CI, /refs/ in console with best-effort fallback), and gracefully skips domain guidance when the directory doesn't exist rather than silently falling back to the wrong mode's path. This prevents the silent quality degradation I flagged. Thanks for addressing it!

…t files

Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler force-pushed the fixes-for-running-the-review-agent-locally-trying branch from 0a8b171 to e9cfd4c Compare April 29, 2026 14:54
@derekmisler derekmisler merged commit 2bb933c into docker:main Apr 29, 2026
17 checks passed
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