Repository navigation
ci: ground bot changes and reviews in scope and evidence - #571
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
| Scope and stopping rules: | ||
| - The original task and explicit maintainer direction define the acceptance | ||
| criteria. Use the supplied context and relevant tests to establish them; | ||
| review suggestions alone do not expand the task. |
There was a problem hiding this comment.
🔵 Low — The follow-up prompt runs on review comments, but "review suggestions alone do not expand the task" doesn't say whose review it means. A human maintainer's review comment is "explicit maintainer direction", while a review-bot suggestion is not. As written, the model could treat both the same way. It might refuse a maintainer's in-thread request as out of scope and report blocked, or it might go along with a bot's request to broaden the change. The PR also deletes the explicit public-API guardrail ("keep public API changes out of scope unless the reviewer explicitly asks"), so this sentence is now the only thing deciding when a public-API change is allowed. Suggest separating the two cases, e.g. "automated/bot review suggestions alone do not expand the task; a maintainer's explicit request in the thread counts as maintainer direction", and keeping a short reminder that changes to exported signatures need that direction.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good: one low finding about a sentence left with no instruction after an edit. The changes only touch prompts and match the outcome names the prompts already use (blocked, out_of_scope). Nit: in the follow-up prompt, line 16 (findings. If the scope is unclear or a broader change or policy decision is needed,) is wider than the rest of the block. Re-wrapping it would match the surrounding style.
| (`printWidth 120`, single quotes, trailing commas). TypeScript is pinned to | ||
| 5.5.4 — don't rely on newer syntax. This is a widely-consumed connector — keep | ||
| public API changes out of scope unless the reviewer explicitly asks. | ||
| 5.5.4 — don't rely on newer syntax. This is a widely-consumed connector. |
There was a problem hiding this comment.
🔵 Low — Removing the trailing clause leaves This is a widely-consumed connector. as a bare sentence with no instruction attached. The guidance now lives in the new "Scope and stopping rules" block (Preserve behavior and public contracts outside the task). That block also says only a human maintainer can widen scope, where the old text said "the reviewer", which is a reasonable tightening. Either delete the leftover sentence or point it back to the new rule (e.g. This is a widely-consumed connector — see the scope rules above for public API changes.) so the model doesn't read it as context with no instruction.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
vuanhphung
changed the title
There was a problem hiding this comment.
Verdict: 1 Low
Looks good: these are prompt-only changes, and the scope and evidence rules agree with each other and with the existing blocked / out_of_scope outcomes (.bot/config.yaml:107; follow-up L49 already uses blocked). One low concern: the follow-up prompt's public-API guard got weaker. Nit: line 15 of engineer-followup/system.md is longer than the ~80-column wrap the rest of the file uses.
| (`printWidth 120`, single quotes, trailing commas). TypeScript is pinned to | ||
| 5.5.4 — don't rely on newer syntax. This is a widely-consumed connector — keep | ||
| public API changes out of scope unless the reviewer explicitly asks. | ||
| 5.5.4 — don't rely on newer syntax. This is a widely-consumed connector. |
There was a problem hiding this comment.
🔵 Low — The PR removes the explicit public-API guard from the follow-up prompt: "keep public API changes out of scope unless the reviewer explicitly asks". The new wording at L14-15 ("Preserve behavior and public contracts outside the task") is weaker. It only protects contracts outside the task, so a follow-up edit could now change an exported signature whenever it judges that change to be inside the task, with no explicit request needed.
The author prompt still says "avoid changing exported signatures … unless the bug is squarely there", so the two flows no longer match on how strict they are about the public API. If you meant to keep the old guarantee, add a short clause to the scope rules, e.g. "Do not change exported signatures or documented behavior unless a human maintainer explicitly asks." That would also fit the new rule that only human maintainers can revise scope.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Successive bot reviews can broaden a focused fix, repeat settled arguments, or propose changes based on unverified assumptions. Anchor implementation to the original acceptance criteria and explicit maintainer direction, preserving behavior outside the task.
Require engineers and reviewers to verify execution paths, capabilities, and downstream contracts before acting or raising findings. Use available review history and require new evidence to reopen settled concerns. Keep tests focused on intended behavior and relevant boundary interactions, and report verification and deferred work accurately.
Validation:
git diff --checkand commit hooks passed. Prompt-only changes; no live bot evaluation. Supplying additional review history remains an engine concern.