Conversation
|
@all-hands-bot live Agent Canvas evidence is now included. Please review this current head and submit an explicit APPROVE review if there are no blocking findings. |
|
👋 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. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: delegate issue triage to subject agents
I reviewed the current head (factory/triage-subject-turns) against main, ran the full test suite, and checked catalog/bundle consistency.
What the PR does
The github-issue-triage extension is refactored from an SDK-coupled automation that ran its own conversation runtime into a lightweight scanner that delegates triage work to Automation's subject-turn API. The worker no longer imports openhands.sdk (RemoteConversation, RemoteWorkspace, register_default_tools) or manages AGENT_SERVER_URL / SESSION_API_KEY / AUTOMATION_CONVERSATION_ID. Instead, submit_subject_turn() posts each changed issue to AUTOMATION_SUBJECT_TURN_URL with a Bearer $AUTOMATION_RUN_TOKEN credential, keyed by GitHub's immutable repository ID + issue number, and idempotent via a content digest.
Verification performed
- Full test suite: 823 passed, 24 skipped (includes the 3 new triage delivery tests, all catalog/sync/schema tests).
- Ruff: clean on
worker.pyandtest_github_triage_delivery.py. - Generated catalog:
npm run build:automationsandnpm run build:skillsproduce no diff —bundle-index.jsandskills/index.jsare in sync with source files. - Bundle-index ↔ source: confirmed
worker.pyandgithub_client.pyinbundle-index.jsmatch the source files byte-for-byte.
Code quality observations (positive)
- Clean separation of concerns: the scanner is a plain host command; only the delegated agent receives a conversation runtime. SDK imports, workspace lifecycle, and session management are fully removed.
- Idempotency:
subject_keyuses{repository_id}:issue:{number}(immutable repo ID, survives renames) andidempotency_keyuses the content digest, so re-runs are safe. - Defensive improvements:
issue.get("labels", [])instead ofissue["labels"],sort_keys=Truein the digest serialization for deterministic ordering. - Prompt security: the delegated prompt passes only the env-var name (
token_name), never the value, instructs the agent never to print it, and scopes mutations to a single issue in a single repository. - Tests are real, not mocks of mocks: the tests exercise the actual
worker.pyentrypoint via thegithub_automation_helpersfixture (which reads the manifest and loads the real bundle), mocking only thesubmit_subject_turnandurlopenboundaries.
Non-blocking finding
SKILL.md throughput description is stale. The last paragraph still reads:
Each scheduled run triages at most one changed issue per repository.
The old code used break after the first changed issue (one per run). The new code uses continue to skip already-triaged issues and submits all changed eligible issues per repository per run. The PR description itself says "For every changed eligible issue, it submits an idempotent subject turn," which matches the code. The SKILL.md text should be updated to reflect this (e.g., "Each scheduled run triages all changed eligible issues per repository"). This is documentation-only and does not block merge.
Verdict
No blocking findings. The architectural change is sound, the code is clean and well-tested, and the generated assets are consistent. Approving.
Generated by OpenHands AI on behalf of the user.
|
@neubig I reviewed the current head and submitted an explicit APPROVE review (#584 (review)). Verification performed:
Non-blocking finding: SKILL.md's last paragraph still says "Each scheduled run triages at most one changed issue per repository," but the new code uses Generated by OpenHands AI on behalf of the user. |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
c0634a5 to
4c0c313
Compare
|
@all-hands-bot Please re-review the current rebased head and explicitly approve it if there are no blocking findings. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Re-review of rebased head (4c0c313)
I re-reviewed the current rebased head (factory/triage-subject-turns @ 4c0c313) against main, re-ran the full suite, and re-checked catalog/bundle consistency.
Verification performed
- Full test suite: 905 passed, 24 skipped (includes the 3 triage delivery tests).
- Ruff: clean on
worker.pyandtest_github_triage_delivery.py. - Generated catalog:
npm run build:automationsandnpm run build:skillsproduce no diff —bundle-index.jsandskills/index.jsare in sync with source. - Bundle-index ↔ source:
worker.pyandgithub_client.pyinbundle-index.jsmatch the source files byte-for-byte.
Re-review observations
The rebased head is content-identical to the head I approved previously — the rebase replayed cleanly onto the latest main with no conflicts or changes. The architectural refactor (scanner delegates to Automation's subject-turn API; no SDK imports, no workspace/conversation lifecycle in the worker) remains sound:
submit_subject_turn()posts{source, subject_key, turn, idempotency_key}toAUTOMATION_SUBJECT_TURN_URLwith aBearer $AUTOMATION_RUN_TOKEN— no conversation/session/workspace machinery leaked into the scanner.- Idempotency keys are
{repository_id}:issue:{number}(immutable repo ID, survives renames) + content digest withsort_keys=Truefor deterministic ordering. - The prompt passes only the env-var name (
token_name), never the value, and scopes mutations to a single issue in a single repository. - Tests exercise the real
worker.pyentrypoint via thegithub_automation_helpersfixture, mocking only thesubmit_subject_turn/urlopenboundaries.
Non-blocking finding (unchanged)
SKILL.md's last paragraph (and the inlined skills/index.js content) still says "Each scheduled run triages at most one changed issue per repository," but the code uses continue (not break) and submits all changed eligible issues per run. The PR description already states the correct behavior. Documentation-only; does not block merge.
Verdict
No blocking findings. Explicitly approving.
Generated by OpenHands AI on behalf of the user.
all-hands-bot
left a comment
There was a problem hiding this comment.
Re-review of rebased head (4c0c313)
I re-reviewed the current rebased head (factory/triage-subject-turns @ 4c0c313) against main, re-ran the full suite, and re-checked catalog/bundle consistency.
Verification performed
- Full test suite: 905 passed, 24 skipped (includes the 3 triage delivery tests).
- Ruff: clean on
worker.pyandtest_github_triage_delivery.py. - Generated catalog:
npm run build:automationsandnpm run build:skillsproduce no diff —bundle-index.jsandskills/index.jsare in sync with source. - Bundle-index ↔ source:
worker.pyandgithub_client.pyinbundle-index.jsmatch the source files byte-for-byte.
Re-review observations
The rebased head is content-identical to the head I approved previously — the rebase replayed cleanly onto the latest main with no conflicts or changes. The architectural refactor (scanner delegates to Automation's subject-turn API; no SDK imports, no workspace/conversation lifecycle in the worker) remains sound:
submit_subject_turn()posts{source, subject_key, turn, idempotency_key}toAUTOMATION_SUBJECT_TURN_URLwith aBearer $AUTOMATION_RUN_TOKEN— no conversation/session/workspace machinery leaked into the scanner.- Idempotency keys are
{repository_id}:issue:{number}(immutable repo ID, survives renames) + content digest withsort_keys=Truefor deterministic ordering. - The prompt passes only the env-var name (
token_name), never the value, and scopes mutations to a single issue in a single repository. - Tests exercise the real
worker.pyentrypoint via thegithub_automation_helpersfixture, mocking only thesubmit_subject_turn/urlopenboundaries.
Non-blocking finding (unchanged)
SKILL.md's last paragraph (and the inlined skills/index.js content) still says "Each scheduled run triages at most one changed issue per repository," but the code uses continue (not break) and submits all changed eligible issues per run. The PR description already states the correct behavior. Documentation-only; does not block merge.
Verdict
No blocking findings. Explicitly approving.
Generated by OpenHands AI on behalf of the user.
|
@neubig I re-reviewed the current rebased head ( Verification performed:
The rebased head is content-identical to the head I approved previously — the rebase replayed cleanly onto the latest Non-blocking finding (unchanged): SKILL.md's last paragraph still says "Each scheduled run triages at most one changed issue per repository," but the code uses No blocking findings. Approved. Generated by OpenHands AI on behalf of the user. |
Why
The existing issue-triage extension attaches its scanner to a conversation and runs the scanner inside that agent runtime. Scanning and deduplication are deterministic automation work; only the selected issue needs an agent and a Docker runtime.
Summary
Keep
github-issue-triageas a lightweight scheduled scanner. For every changed eligible issue, it submits an idempotent subject turn keyed by GitHub's immutable repository ID and the issue number. The delegated agent uses the selected profile to publish a readable triage comment, priority, andready-for-devlabel directly on GitHub.The extension no longer imports the SDK or handles Agent Server URLs, session keys, workspaces, conversations, result artifacts, or runtime lifecycle. Those responsibilities are provided once by the Automation service.
Issue Number
Closes #583
How to Test
priority:highandready-for-dev, and released the subject container.Dependencies
This is an independent
main-based PR. Its runtime behavior requires OpenHands/automation#468. Profile-scoped credentials require OpenHands/automation#466 and OpenHands/software-agent-sdk#5046; the delegated Docker agent uses the profile/runtime boundary from OpenHands/software-agent-sdk#5017 and #3403.