Repository navigation
ci: fix engineer bot formatting and publication checks - #570
vuanhphung wants to merge 19 commits into
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
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_unreleasedfinds## Unreleasedanywhere in the file, and the 'released' comparison only checks that the rest of the text is unchanged. So an agent can add a new## Unreleasedsection at the bottom ofCHANGELOG.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().
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Mostly solid: the publication gate is well thought out and covered by many regression cases. I found one medium gap, a Prettier config file type the protected list misses, and one low gap: inline disable comments can still get the gate to pass.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
The gate and prompt changes look solid overall, with one medium gap: a self-ignoring nested .gitignore can get past the recorded-edit and protected-config checks. There is also one low issue: the changelog check never verifies that existing Unreleased entries are kept.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 High · 1 Medium
One high-severity gap: the new suppression-directive gate can be bypassed by putting any plain comment before the directive. One medium gap: inline ESLint config comments aren't covered. The rest of the gate looks sound: HEAD/index-flag checks, config and dependency integrity, recorded-edit coverage, changelog append rules, and credential scrubbing.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
The gate design looks sound overall, with one medium hardening gap and one low false-positive risk. The validator trusts the repo-local Git config, which the agent can change through bash (for example by setting core.fsmonitor). The suppression-comment matcher is broad enough to reject ordinary prose comments.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good, with 2 low-severity robustness concerns about the publication gate: it can fail correct fixes because of baseline changelog quirks or tool caches written into node_modules. The gate's checks themselves are consistent: validators are loaded from the captured commit, HEAD and index flags are checked, recorded edits are matched against the diff, and directive detection is TypeScript-AST based. The tests cover these well.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good, with one low-severity false-positive risk in the new directive check. The validation gate fails closed, takes its validators from the pre-agent commit, and its regression fixture covers hidden-edit, config, dependency-integrity and changelog cases well. The ln -s change works because the root .gitignore uses node_modules with no trailing slash, so the symlink is ignored.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Medium · 1 Low
The gate design is solid and well tested, with 2 medium concerns: the gate's Python step and its final npm style checks can both be subverted by files the agent writes outside the protected-path checks. There is also 1 low gap in how the directive check counts suppressions.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good, with one low concern: the new pre-publication gate covers only the author flow. The mention and follow-up flows, where the #568 problem came from, still rely on prompt instructions alone. The validator logic I checked against the repo's CHANGELOG layout, .gitignore/.prettierignore, and existing directive usage looks correct: changelog diffing, recorded-edit coverage, the directive-count check and the dependency digest.
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall, with one medium concern: an agent can rewrite the step environment through the runner's GITHUB_ENV/GITHUB_PATH files, which gets around the pre-publication gate. I found no defects in the changelog parsing, the recorded-edit and dependency-digest checks, the directive counting, the symlink in the mention workflow, or the prompt changes.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Mostly solid. The gate, workflow wiring and tests are thorough, and the ordering checks out: the digest is taken after npm ci, the publish step is gated on validation, and the comment step distinguishes a validation failure. One medium gap: block-region eslint-enable directives are checked only by total count, so a disabled region can be widened without adding a new directive.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall: the publication gate is thorough and backed by a large regression suite. I found two low-severity issues: the Unreleased changelog parser can wrongly reject a valid new entry when that section contains a ### sub-heading or prose, and the prompts never tell the agent that file deletions will fail the gate.
| raise SystemExit("::error::Place ## Unreleased above the released versions.") | ||
| released = text[:section.start()] + text[section.end():] | ||
| blocks = [entry.rstrip() for entry in re.findall(r"(?ms)^- (\S.*?)(?=^- |\Z)", section.group(1))] | ||
| return released, Counter(blocks) |
There was a problem hiding this comment.
🔵 Low — The bullet-block regex treats everything up to the next - line as part of the current entry. That includes ### sub-headings and any prose paragraphs. This changelog already uses that layout in released sections (for example ## 1.8.0 / ## 1.6.0 have ### Highlights followed by prose). Suppose a future ## Unreleased section looks like `- A
Notes
text. When the agent appends - Bafter- A, as the prompt tells it to, the text that belonged to block Amoves into blockB. previous_entries <= current_entries` then fails, and a valid fix is rejected with "preserve existing entries". You could split blocks at sub-headings and blank-line paragraphs as well, or compare only bullet title lines plus their indented continuations. At minimum, add a fixture for this case so the limitation is explicit.
| | git_paths("diff", "--cached", "--no-ext-diff", "--no-renames", "--name-only", "-z", base_sha, "--") | ||
| | git_paths("ls-files", "--others", "--exclude-per-directory=.gitignore", "-z") | ||
| ) | ||
| deleted = git_paths("diff", "--no-ext-diff", "--no-renames", "--diff-filter=D", "--name-only", "-z", base_sha, "--") |
There was a problem hiding this comment.
🔵 Low — The gate fails any run that deletes a tracked file (The pinned bot publisher cannot stage deleted files). However, the engineer and follow-up prompts never tell the agent that deletions aren't allowed. An agent that removes a dead helper or obsolete test as part of a minimal fix will finish successfully, then lose the whole run at publication. Add one line to the engineer prompt's RULES (for example: "Do not delete files; the publisher cannot stage deletions — note them in your reason instead") so the agent avoids this instead of hitting an opaque gate failure.
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, validate recorded edits, configuration, dependencies, and a new Unreleased entry against the pre-agent commit. Isolate imports and Git settings, invoke verified style binaries, and preserve existing changelog entries. Directive counts reject added suppressions and disabling-rule changes, and preserve enable keys unless matching disable blocks are removed; movement and changes to covered code still need diff review.The gate catches accidental omissions and tool misuse on the author's shared runner. Hostile shell code can tamper with that user/environment; execution isolation is outside this gate's scope. Mention/follow-up enforcement is prompt-only, followed by standard PR CI, because the pinned engine publishes inside its run step; enforcing the same gate there requires an engine hook.
Validation: 294 gate cases, Prettier, ESLint, shell syntax, and 1,485 unit tests pass locally. Real unit tests preserve the dependency digest with the pinned engine’s scrubbed CACHE_DIR; CI now checks that invariant. Direct style checks also pass with unusable user/global npm script-shell settings.