Skip to content

ci: fix engineer bot formatting and publication checks - #570

Open
vuanhphung wants to merge 30 commits into
mainfrom
ci/engineer-bot-changelog-style-checks
Open

vuanhphung wants to merge 30 commits into
mainfrom
ci/engineer-bot-changelog-style-checks

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Engineer-bot edits in #568 missed formatting and changelog updates. A formatting request then changed a file through Bash, which the engine does not record for publication, and lint failed after dependencies were moved out of the parent checkout.

Require changelog entries and final style checks across bot prompts, apply formatting through edit_file, share installed dependencies, and isolate mention lint from parent configuration. Before author publication, check recorded edits, configuration, dependencies, suppressions, and a new Unreleased entry against the pre-agent commit. Preserve existing release history, explain unsupported deletions and suppression restrictions, and remove agent-run dependency reinstalls from the allowlist.

The gate catches accidental omissions and routine tool misuse on the author's shared runner; execution isolation against hostile shell code is outside its scope. Mention/follow-up enforcement remains prompt-only plus PR CI because equivalent enforcement requires an engine publishing hook. Directive coverage changes still need diff review.

Validation: 316 gate cases, 1,488 unit tests (with coding-agent environment variables cleared), Prettier, ESLint, YAML parsing, and shell syntax pass locally. CI checks dependency immutability on the bot's Node 22 unit leg. Earlier real-test checks confirmed that redirected CACHE_DIR preserves the dependency snapshot and direct style tools ignore unusable npm script-shell settings.

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 · 1 Low

The prompt and gate changes are reasonable, with one high security concern. The new validation step runs lint and Prettier on the tree the agent edited but does not clear the per-step OIDC request credentials the way the author step does. I also have one low note: the changelog gate is looser than what the prompts require.

Comment thread .github/workflows/engineer-bot.yml Resolved
Comment thread .github/workflows/engineer-bot.yml Resolved
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
@vuanhphung vuanhphung changed the title ci: require engineer bot changelog and style checks ci: fix engineer bot formatting and publication checks Nov 6, 2026

@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: 2 Medium

The direction is good, but there are 2 medium gaps in the new pre-publish gate. The config-tamper check misses nested lint/format config files. The checks also read the working tree rather than the edit_file-recorded files that actually get published, so a bash-written changelog or a bash formatter run can still pass the gate and then be left out of the PR.

Comment thread .github/workflows/engineer-bot.yml Resolved
Comment thread .github/workflows/engineer-bot.yml 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: 2 Low

Looks good overall: the publication gate is fail-closed, credentials are cleared, and the changelog diff logic correctly handles both creating ## Unreleased and adding to it. Two low-severity edge cases: the validation step wrongly fails fixes that delete a file, and the mention job's ESLint still merges the parent checkout's config.

Comment thread .github/workflows/engineer-bot.yml 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: 1 Medium

Mostly solid. The changelog, recorded-file and deletion checks hold up for this repo's ## <version> changelog layout. One medium concern: the new pre-publish gate runs npm and Node tooling that the agent can still tamper with (an unprotected .npmrc, unchecked node_modules) in the step just before the token-holding publish step.

Comment thread .github/workflows/engineer-bot.yml 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: 1 Medium · 1 Low · 1 Nit

The changes look sound, with one medium gap: the publish gate still lets the agent switch off lint or formatting with comments inside source files. The dependency digest and protected-path checks look right: the **/ globs also match root-level files, .gitignore covers the node_modules symlink, and nyc's find-cache-dir honours CACHE_DIR. The other two findings are low-severity: a misleading failure message and an unchecked Unreleased section position.

Other findings

  • ⚪ Nit — split_unreleased finds ## Unreleased anywhere in the file, and the 'released' comparison only checks that the rest of the text is unchanged. So an agent can add a new ## Unreleased section at the bottom of CHANGELOG.md, or between two released versions, and still pass. The prompts require it to sit above the newest version. If the position matters, also assert that the section is the first ## heading, e.g. re.search(r"(?m)^## ", current).start() == section.start().

Comment thread .github/workflows/engineer-bot.yml Resolved
Comment thread .github/workflows/engineer-bot.yml 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: 2 Low

Looks good overall, with two low-severity points: the changelog entry parser is stricter than it says (only - bullets count), and the dependency digest adds cost on every unit-test matrix leg. I checked the gate's changelog, recorded-edit, protected-config, index-flag and directive logic, the mention node_modules symlink (root .gitignore uses node_modules with no trailing slash, so the symlink is ignored too), and the follow-up workflow, and found no correctness defects. One thing I couldn't confirm: whether the engine's bash allowlist matches by prefix, which the mention prompt's npm run lint -- --no-eslintrc --config .eslintrc relies on. The existing npx mocha --config … entry suggests it does.

Comment thread .github/workflows/main.yml 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: 1 Low

Looks good overall: one low-severity point about keeping the CI dependency check matched to the bot's Node version. I checked the gate's protected-path, recorded-edit, deletion, changelog and suppression logic against the workflow wiring. Generated outputs (.nyc_output, coverage_*, dist, lib/version.ts) are all gitignored, the bash allowlist matches by prefix so the explicit ESLint command in the mention prompt is allowed, and publishing is correctly blocked when validation fails. Nit: GIT_NO_REPLACE_OBJECTS is set in both the validation step's env and the script itself.

Comment thread .github/workflows/main.yml 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: 1 Low

Looks good overall, with one low concern: the gate's lint and Prettier commands are copied from package.json and could drift from it. The gate loads its scripts from the pre-agent commit, .gitignore's node_modules entry (no trailing slash) also ignores the mention-flow symlink, and the CACHE_DIR redirect keeps nyc caches out of the dependency digest. I checked the changelog parsing, the suppression-directive counting and the workflow step conditions against the fixtures and found no correctness problems.

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, with one low concern: the author prompt doesn't tell the agent about one rule that the new publication gate enforces. I traced the gate's changelog, recorded-edit, dependency-digest and directive checks, plus the workflow step ordering (digest is taken after npm ci, publishing is skipped on gate failure, and the comment step's always() reports the failure). I found no correctness defects in the executed code paths.

Comment thread .bot/prompts/engineer/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: 1 Low

Looks good overall, with one low-severity concern: the changelog gate is stricter than the prompts describe. I checked step ordering in the workflows (the dependency snapshot is taken after npm ci and the credential scrub), whether the pr/node_modules symlink is ignored (root .gitignore uses node_modules with no trailing slash, so the symlink is ignored), whether Prettier ignores the coverage/nyc output, and the Python 3.11 requirements (Counter <=, is_relative_to); none of these turned up a problem.

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.

✅ No issues identified by the review bot.

This branch had an error being deployed

1 failed deployment
azure-prod — b48feaf0 Deployed Nov 7, 2026 by vuanhphung via e2e-test (24.22.1, 24_22_1) #1828
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