Skip to content

ci: port engineer bot mention workflow from Go driver - #566

Merged
vuanhphung merged 7 commits into
mainfrom
vu/port-engineer-bot-mention
Nov 5, 2026
Merged

vuanhphung merged 7 commits into
mainfrom
vu/port-engineer-bot-mention

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Port the Go driver's engineer-bot mention workflow so maintainers with write access can start a review, ask a question, or request changes with a top-level @bot comment on an open, same-repository PR.

Load bot setup and dependencies from the trusted default branch, clear registry credentials before the PR checkout, and reuse dependencies with npm offline. Require a PR build before verification, clear runner-injected OIDC variables inside each engineer agent shell, and serialize mention/follow-up jobs through the existing per-PR queue. Preserve authorization and the checked-out SHA/diff reference.

Validation: formatting, YAML/shell/Python checks, authorization and dependency-transfer checks, and credential-inheritance checks passed. A clean checkout built successfully and passed 119 affected unit tests. Live mention execution requires merging to the default branch.


This PR was created with GitHub MCP.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 1 High

The workflow's authorization, credential scrubbing and diff-reference handling look sound, with one high issue. The PR checkout only receives node_modules from the trusted install, so the generated, gitignored lib/version.ts is missing there. The agent's first unit-test or type-check run will fail until it builds.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 1 Medium

Mostly solid: one medium concern. The authorization gating, credential cleanup before the PR checkout, and the trusted-dependency handoff all look correct. The mention workflow and the follow-up workflow don't share a concurrency group, so both can push to the same PR branch at once.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 1 Medium · 1 Low

The port looks mostly sound: the default-branch dependency install, the credential scrub before the PR checkout, and the diff reference are wired correctly. One medium concern: the scrub may not actually remove the OIDC token request variables while the job still has id-token: write. One low note: dropped @bot requests get no feedback on the PR.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 1 Low

Looks good. There's one low-severity robustness note on how the diff reference file is created. Authorization runs in layers (author association, then a live permission check, then a same-repo/open check repeated after the queue wait), dependencies install from the default branch before any PR code is checked out, and credential scrubbing matches the existing follow-up workflow. All npm scripts the prompt references (build, test, type-check, lint, prettier) exist in package.json.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 1 Low

Looks good overall, with one low-severity concern: the bash allowlist still permits npm ci and npm run e2e, which the mention prompt forbids. Authorization, the PR-eligibility recheck, the trusted-dependency handoff and the guards on the diff reference file all hold up. Nit: the new workflow has no explanatory header comment, unlike its sibling workflows.

Comment thread .bot/prompts/engineer-mention/system.md Resolved
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot 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.

Verdict: 3 Low

Overall the workflow looks sound: authorization is checked before any secrets are requested, dependencies are installed from the trusted default branch and credentials are cleared before the PR checkout, and the diff-reference file is handled safely. There are three low-severity concerns: the concurrency.queue key may not be supported (which would also break the existing follow-up workflow), only the prompt stops the destructive npm ci, and rejected @bot requests get no feedback.

Comment thread .bot/prompts/engineer-mention/system.md Resolved
@vuanhphung
vuanhphung added this pull request to the merge queue Nov 5, 2026
Merged via the queue into main with commit 8356ac6 Nov 5, 2026
38 of 43 checks passed

This branch was successfully deployed

1 active deployment
azure-prod — 6f65c3ca Deployed Nov 5, 2026 by vuanhphung via e2e-test (22.23.4, 22_23_4) #1761
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant