docs(review): define Automation Service checkpoints - #495
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟢 Good taste — docs-only change that converts observed review gaps into stable, mechanism-grounded checkpoints.
Verification performed:
- Confirmed the diff touches only
.agents/skills/custom-codereview-guide.md(+78/-17) andAGENTS.md(+22/-0). - Spot-checked the checkpoints against the real service code at this head:
FOR UPDATE SKIP LOCKEDmulti-worker polling exists in bothscheduler.pyanddispatcher.py, andwatchdog.pyperforms terminal verification with sandbox pause/cleanup — so the run-lifecycle, overlap/capacity, and staleness checkpoints describe existing mechanisms, not aspirational ones. - The skill frontmatter keeps the required
triggers: [/codereview]and addsname/description, matching the skill format. - The approval-mode guidance from the old "Review Submission Mode" section is preserved in the new wording ("submit APPROVE rather than a COMMENT that says the PR is ready"), so no behavioral guidance was lost in the rewrite.
- The AGENTS.md checklist mirrors the guide's five checkpoint areas consistently.
[CRITICAL ISSUES] None.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Documentation-only change to review guidance and the contributor-facing checklist. No code paths, schemas, migrations, or contracts are affected. The checkpoints are grounded in mechanisms that exist in the repository today.
VERDICT:
✅ Worth merging: No material correctness, security, compatibility, or acceptance-criterion defects found.
KEY INSIGHT:
The checkpoints are traceable to real service machinery (SKIP LOCKED claiming, callback/watchdog optimistic locking, sandbox pause/delete on terminal verification), which is exactly what makes them stable review criteria rather than a generic checklist.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Taste Rating: 🟢 Good taste
Docs-only change that reorganizes the repository-specific review guide into stable, mechanism-based checkpoints and mirrors them as a pre-PR implementation checklist in AGENTS.md. I verified the checkpoints against the repository's actual architecture as documented in AGENTS.md, and they are accurate and well-grounded:
- Run state and lifecycle matches the documented callback/watchdog optimistic-locking pattern (
UPDATE ... WHERE status = 'RUNNING'withrowcountchecks) and the fire-and-forget sandbox cleanup path. - Scheduling, overlap, and capacity matches the cross-replica lease design and per-user sandbox limit behavior.
- Database and migration parity matches the documented PostgreSQL/SQLite split,
alembic upgrade head, the cross-database migration rules, and theFOR UPDATE SKIP LOCKED/using_sqlite()fallback pattern. - The new frontmatter (
name,description,triggers: [/codereview]) matches the required skill format, and the removed "submission mode" section is fully covered by the new APPROVE/COMMENT guidance — no guidance was lost.
No material correctness, security, or compatibility findings. The guide and the AGENTS.md checklist are consistent with each other and with the service's real mechanisms.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Documentation-only change to review guidance and contributor checklist; no runtime code, schema, or configuration is touched. The content was cross-checked against the repository's documented behavior and found accurate.
VERDICT:
✅ Worth merging: No material findings.
KEY INSIGHT:
Encoding verified missed-review patterns as explicit, mechanism-grounded checkpoints (rather than incident-specific rules) is the right abstraction — it generalizes without over-fitting to past failures.
|
@all-hands-bot Your review of current head |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Docs-only change that converts observed review gaps into mechanism-grounded checkpoints and mirrors them as a pre-PR implementation checklist in AGENTS.md. I verified the checkpoints against the service code at this head and they describe existing machinery, not aspirations:
- Run state and lifecycle matches the callback/watchdog optimistic-locking pattern (
UPDATE ... WHERE status = ``RUNNING``` withrowcountchecks) and the pause-then-cleanup janitor path inwatchdog.py`. - Scheduling, overlap, and capacity matches
FOR UPDATE SKIP LOCKEDmulti-worker polling in bothscheduler.pyanddispatcher.py, and the per-user sandbox handling. - Database parity matches the
using_sqlite()fallback guard used throughout queries/utilities, and the single linear Alembic head (025_add_agent_profile.py,down_revision = "024"). - The skill frontmatter keeps
triggers: [/codereview]and addsname/description, matching the skill format; the old "Review Submission Mode" approval guidance is preserved in the new APPROVE/COMMENT wording, so no behavioral guidance was lost.
No material correctness, security, or compatibility findings. CI is passing for this head.
✅ APPROVED
|
🚀 Released in 1.13.3. |
Why
The repository-specific guide only explained how to submit an approval. A review of 134 Automation PR records found six verified missed findings across four approved PRs, centered on state transitions, overlapping runs, sandbox capacity/liveness, and configuration parity. None showed that a missing incident-specific rule was the root cause, so this organizes the service's existing mechanisms into stable review checkpoints.
Summary
AGENTS.mdso implementations can satisfy review on the first pass.Closes #494
Validation
uv run pre-commit run --files AGENTS.md .agents/skills/custom-codereview-guide.md