feat: add independent GitHub issue triage - #570
Conversation
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
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.
Summary
This PR adds a GitHub issue triage automation (skill, catalog manifest, shared github_automation.py module, and contract tests). The automation prioritizes open issues, posts acceptance criteria, and optionally labels issues ready-for-dev. The design is clean and the shared base class is well-factored for reuse across the planned implementation/review/watchdog PRs.
CI-Breaking Issues (Must Fix)
1. Missing commands/github-issue-triage.md - sync_extensions.py --check will fail
The SKILL.md declares triggers: [/github-issue-triage], which is a slash trigger. The sync script auto-generates a corresponding commands/github-issue-triage.md file and CI enforces its presence. Every other skill with a slash trigger in this repo ships its command file. Run python scripts/sync_extensions.py to generate it, or per the AGENTS.md guidance, prefer adding the command file directly rather than relying on the deprecated slash-trigger auto-generation.
2. README.md catalog section is out of date
sync_extensions.py --check also reports the README catalog table is stale. The new github-issue-triage skill is not listed in the auto-generated catalog table. Run python scripts/sync_extensions.py to fix both issues at once.
Design Observations
3. Single-issue-per-run throughput
The for...break/else loop in worker.py triages exactly one issue per cron invocation. With the default */5 * * * * schedule, clearing a backlog of N untriaged issues takes 5*N minutes. This appears to be a deliberate rate-limiting choice (one LLM call per run), but it is worth documenting explicitly in the SKILL.md so operators understand the expected throughput when first deployed against an existing backlog.
4. popularityRank collision
github-issue-triage uses popularityRank: 80, which collides with github-agents-md-maintainer and upstream-fork-sync. This is non-blocking (the catalog likely sorts stably), but a unique value would avoid ambiguous ordering in the UI.
Positive Notes
- Token handling is solid: the token name (not value) is embedded in agent instructions,
shell()redacts the token from error output, and the SKILL.md explicitly warns against putting token values in definitions. - The
completed_dependencycache correctly deduplicates GitHub API calls within a run, and the 404 to False / 403 to raise behavior is correct for dependency resolution. - Contract tests exercise real code paths (label preservation, dependency states, permission errors) rather than asserting mock calls.
- Repository boundary is correct: skill, manifest, and shared scripts belong in this extensions registry.
Risk Assessment
- Overall PR Risk: MEDIUM
- The automation logic is sound and well-tested, but two CI-breaking sync issues must be resolved before merge. No security concerns identified - token handling follows least-privilege principles and issue content is treated as untrusted data in the agent prompt.
Verdict
Worth merging after fixing the two sync issues. The core logic is sound and the shared module is cleanly designed for reuse. Run python scripts/sync_extensions.py and commit the generated files.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 thumbs up or thumbs down to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
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.
Summary
This PR adds a GitHub issue triage automation skill with a shared github_automation.py module, a triage worker, catalog manifest, tests, and marketplace/sync updates. The skill definition, manifest, and catalog entries are well-structured and follow the repo's conventions. The triage worker's change-detection logic (SHA-256 digest of issue state + marker comment) is a clean idempotency mechanism.
Two functional issues in the shared github_automation.py module should be addressed before merge.
Key Findings
1. Missing fire_callback - the Automation Service won't know the run completed
The main() function in github_automation.py (line 210) returns the conversation ID but never calls fire_callback() to signal completion to the Automation Service. Every existing automation script in this repo (github-pr-reviewer, news-digest, github-issue-to-pr, github-repo-monitor, etc.) calls fire_callback("COMPLETED") on success and fire_callback("FAILED", str(e)) on error. Without the callback, the Automation Service leaves the run in RUNNING state until it times out, which blocks concurrency slots and pollutes run history.
2. No per-repository error isolation in main()
The per-repo loop (lines 202-209) does not catch exceptions from automation.run(). If one repository fails, the exception propagates and remaining repositories are never processed. The established pattern in this repo is: "One repository failing does not stop the others; the run fails only if every repository fails." The try/finally only closes the server connection but lets the exception escape the loop.
Risk Assessment
- Overall PR risk: MEDIUM
- The skill definition, manifest, catalog, marketplace, and sync artifacts are correct and consistent.
- The two functional issues are in the shared runtime module that will be reused by future automation PRs (implementation, review, watchdog), so fixing them here prevents propagating the same bugs.
- Security posture is good: the token name (not value) is passed to the agent, issue content is treated as untrusted data and serialized via
json.dumps, and error output redacts the token.
Verdict
Needs rework - Add fire_callback and per-repo error isolation to main() before merge.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 thumbs up or thumbs down to give feedback.
all-hands-bot
left a comment
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.
Summary
This PR adds a scheduled GitHub issue triage automation with a shared GitHubAutomation base class (github_automation.py), a triage worker (worker.py), catalog/manifest entries, marketplace registration, and contract tests. The design is clean: the shared base class handles GitHub API transport, agent conversation lifecycle, and pagination, while the worker implements triage-specific logic (label creation, dependency checking, content-digest deduplication, acceptance criteria validation, label management).
The repository boundary is correct - skills, automations, and shared scripts belong here. No SDK documentation is duplicated. The manifest validates against the catalog schema. The sync_extensions.py --check passes and tests pass (6/6).
Key Findings
1. Missing completion callback (important)
Every other bundle-based automation in this repo (github-issue-to-pr, github-agents-md-maintainer, github-pr-reviewer, news-digest) calls fire_callback("COMPLETED") / fire_callback("FAILED", ...) on every exit path. The shared main() in github_automation.py does not. The openhands-automation skill documentation states: "Wrap your main logic in try/except and call fire_callback(\"FAILED\", str(e)) in the except block" and "If you never fire the callback the run stays RUNNING until the watchdog marks it FAILED." Without the callback, the automation service may not know a run completed until a watchdog timeout fires, delaying run status visibility and potentially blocking concurrency slots.
This affects not just github-issue-triage but every future automation that reuses this shared base class (the PR description mentions implementation, review, and watchdog PRs will share it).
2. AgentServerClient import may not exist in released SDK (important)
The __init__ method does from openhands.sdk.client import AgentServerClient. In the currently released openhands-sdk (1.47.0), there is no openhands.sdk.client module. The PR description acknowledges "SDK release availability remains a merge prerequisite" and references OpenHands/software-agent-sdk#5010. This is fine as a merge prerequisite, but the import is inside __init__ (deferred), so it won't fail at import time - it will fail at runtime when the automation tries to construct the client. If the SDK PR hasn't shipped by the time this is tested live, the failure will be a runtime ModuleNotFoundError rather than an obvious install-time error.
3. agent() deadline (2400s) exceeds manifest timeout (1800s) (minor)
The agent() method waits up to 2400 seconds (40 min) for the conversation to finish, but the manifest declares "timeout": 1800 (30 min). The automation service will kill the process before the script's own timeout can fire cleanly. The try/finally: server.close() in main() mitigates this, but the mismatch means the script's TimeoutError path is effectively dead code in catalog deployments. Consider aligning the deadline to be shorter than the manifest timeout.
Risk Assessment
- Overall PR risk: MEDIUM
- The missing callback is the most impactful issue - it affects run lifecycle management across all automations using the shared base class.
- The SDK import dependency is explicitly called out as a merge prerequisite, which is the right approach.
- Security posture is good: untrusted issue content is treated as data, token values are redacted in error messages, the secret name (not value) is passed to the agent, and label changes preserve existing labels.
- Tests exercise real code paths (label preservation, dependency checking, priority validation) rather than just asserting mocked calls.
Verdict
The code is well-structured and the design is sound. The missing fire_callback is the one issue I'd recommend addressing before merge, as it establishes a pattern that will be inherited by the other three planned PRs sharing this base class. The SDK import is appropriately gated as a merge prerequisite.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 thumbs up or thumbs down to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
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.
Summary
This PR adds a scheduled GitHub issue triage automation with a shared GitHubAutomation base class, a triage worker, catalog manifest, marketplace registration, and contract tests. The design is sound: the shared base handles GitHub API transport, agent conversation lifecycle, and pagination, while the worker implements triage-specific logic (label creation, dependency checking, content-digest deduplication, acceptance criteria validation, label management).
All findings from the previous bot reviews (commit d188c5e) have been addressed in subsequent commits:
- Command file now exists (
commands/github-issue-triage.md) - Per-repository error isolation is implemented with
try/except/finallyinmain() - Manifest timeout (3000s) now exceeds the agent deadline (2400s)
- Single-issue-per-run throughput is documented in SKILL.md
fire_callbackomission is explained as intentional (profile workers rely on the Automation #453 runtime watchdog, not the service callback key)AgentServerClientimport is gated as an explicit SDK merge prerequisite
Repository Boundary
Correct. Skills, automations, shared scripts, and catalog entries belong in this extensions registry. No SDK documentation is duplicated. The manifest validates against catalog.schema.json.
Verification
sync_extensions.py --checkpasses (theissue-duplicate-checkercoverage warning is pre-existing and unrelated)test_github_triage_delivery.pypasses 7/7- Tests exercise real code paths: label preservation, dependency state checking, priority validation, and per-repository error isolation
Risk Assessment
- Overall PR risk: LOW
- Security posture is solid: untrusted issue content is treated as data in the agent prompt, token values are redacted in error output, the secret name (not value) is referenced in instructions, and label changes preserve existing labels.
- The content-digest deduplication correctly prevents re-triaging unchanged issues while allowing re-triage when discussion evolves.
- The
for/elseloop correctly selects the lowest-numbered untriaged issue per run. - SDK release availability remains an explicit cross-repository merge prerequisite, which is the right approach.
Verdict: No material findings. The change is ready for merge once the SDK prerequisite is satisfied.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 is merge-ready.Was this review helpful? React with thumbs up or thumbs down to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
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.
Review Summary
This PR adds a new github-issue-triage automation skill, extracts shared GitHub REST transport into skills/github/scripts/github_client.py, and refactors four existing automation scripts to import from it. The change belongs in this repository (skills, automations, shared scripts).
What was reviewed
- New skill (
github-issue-triage):worker.pyimplements anIssueTriageclass extendingGitHubRepository. It ensures labels exist, filters open non-PR issues withoutready-for-dev, checks dependency completion, uses a SHA-256 digest marker to avoid re-triaging unchanged issues, delegates triage reasoning to an LLM conversation, validates the result, posts a comment with acceptance criteria, and conditionally appliesready-for-dev+ priority labels. The for/else loop correctly picks the first un-triaged issue or returns when all are done. - Shared
github_client.py: Clean extraction ofgithub_request/github_paginateplus aGitHubRepositorybase class with token validation, repository name validation, shell redaction, dependency tracking with@cache, and amain()that iterates repositories with per-repo failure isolation. - Refactored scripts:
github-issue-to-pr,github-pr-reviewer,github-agents-md-maintainer, andgithub-repo-monitorall import from the shared module. The sharedgithub_requestaddstimeout=90(old inline versions had no timeout) andgithub_paginatecaps at 100 pages (old versions looped indefinitely). Both are safety improvements with no behavioral impact on normal operation. - Tests:
test_github_triage_delivery.pycovers label preservation, conditional ready-for-dev application, dependency state validation (completed/not_planned/open/403), and multi-repo failure isolation. Tests exercise real code paths via the bundle helper, not just mocks. - Catalog/marketplace:
manifest.json,catalog-index.js,bundle-index.js,skills/index.js, andmarketplaces/openhands-extensions.jsonall include the new entry consistently.
No material findings
The code is well-structured, the refactoring is a clean extraction with no behavioral regressions, input validation is thorough (repository name, token name, triage result schema), and the token is never exposed in error messages or logs. No bugs, security issues, or design flaws identified.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- New automation with no checkout or code execution; only reads issues and posts comments/labels via the GitHub API. Token scope is limited to Issues: read and write. The LLM conversation receives untrusted issue content as data, not instructions, and its only output channel is a JSON file that is schema-validated before any GitHub action is taken.
- The shared code refactoring is backward-compatible: all callers already used the same function signatures, and the added timeout/pagination cap are safety improvements.
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.- 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
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.
Summary
This PR adds a new github-issue-triage automation and extracts shared GitHub API transport into skills/github/scripts/github_client.py, refactoring four existing automation scripts to import from it. The design is clean: the GitHubRepository base class encapsulates token resolution, API calls, pagination, commenting, and dependency checking. The triage worker's digest-based change detection is well thought out - it re-triages only when issue content changes, and the for...else loop correctly selects the first un-triaged issue.
No critical bugs or security issues found. The findings below are cleanup items from the refactor.
Findings
Dead imports left by the refactor:
from contextlib import closinginworker.pyis imported but never used. The commit message ("fix: close attached SDK conversations with contextlib") suggests this was meant to wrap something, but thewithstatement already handles cleanup via__enter__/__exit__onRemoteWorkspaceandRemoteConversation.attach.from urllib.parse import urlencodeis now orphaned in all four refactored scripts (github-pr-reviewer,github-issue-to-pr,github-agents-md-maintainer,github-repo-monitor) since_github_request/_github_paginate- the only callers ofurlencode- moved togithub_client.py.
Minor design note: GitHubRepository.__init__ reads config.json (line 66) and main() also reads it independently (line 172). The constructor's self.repository = repository or self.config["repository"] will KeyError if repository is falsy and the config has no "repository" key. In practice main() always passes a non-None repository, so this works, but the fallback is fragile. Consider not reading config in __init__ when repository is provided, or using self.config.get("repository").
Performance note (documented trade-off): The triage run() method fetches comments for every issue (sorted by number) until it finds one whose digest doesn't match an existing marker. For a large backlog this is O(n) API calls before triaging starts. This aligns with the SKILL.md's stated design ("at most one changed issue per repository"), but operators with large backlogs should be aware of the per-run cost.
Risk Assessment
- Overall PR Risk: LOW
- No security vulnerabilities: tokens are resolved from env vars, never hardcoded; the
shell()method redacts the token in error output. - No breaking changes to existing automations: the four refactored scripts retain their own
urllibusage for non-GitHub operations; only the shared request/paginate functions were extracted. - Tests cover the key contract scenarios (multi-repo failure isolation, label preservation, dependency completion, permission error propagation).
- The PR is correctly placed in the extensions registry (skills + automation catalog).
- No security vulnerabilities: tokens are resolved from env vars, never hardcoded; the
VERDICT: Worth merging. Core logic is sound. The dead imports are cleanup nits, not blockers.
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 thumbs up or thumbs down to give feedback.
cafd117 to
5f0c5df
Compare
|
@all-hands-bot please review the current head after the stack split and explicitly approve it if there are no material 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. |
2 similar comments
|
👋 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. |
|
👋 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. |
Review: feat: add independent GitHub issue triage (#570)Head reviewed: Verdict: ✅ Approved — no material findingsI reviewed all 21 changed files on the current head, ran What was verified
Code assessmentThe worker is clean and well-structured:
Prior findings — all resolvedEvery finding from earlier reviews on this PR has been addressed in the current head:
Non-blocking observation
Generated by OpenHands AI on behalf of the user. |
|
@all-hands-bot Your current-head review found no material issues. Please submit a GitHub pull-request review with the APPROVE decision on this head rather than posting another issue comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: 5f0c5df (post stack split)
Verdict: ✅ Approved — no material findings
I reviewed all 21 changed files on the current head, ran sync_extensions.py --check (passes), and executed the full relevant test suite (143 tests pass, including triage delivery contracts, skill installation, catalog schema, and sync extensions).
What was verified
| Check | Result |
|---|---|
sync_extensions.py --check |
✅ Passes — README catalog and command file in sync |
commands/github-issue-triage.md |
✅ Present (prior finding addressed) |
| Skill installation test | ✅ Bundle files materialize as real files, import worker succeeds |
| Triage delivery contracts | ✅ Label preservation and ready-only-when-clear both pass |
| Catalog schema validation | ✅ Manifest validates against the schema |
worker.py syntax |
✅ Compiles cleanly |
| Symlink resolution | ✅ github_client.py → shared skills/github/scripts/github_client.py |
| README catalog counts | ✅ Updated (70 extensions, 60 skills) |
| Per-repo error isolation | ✅ run_repositories() catches per-repo exceptions (prior finding addressed) |
| Manifest vs. worker timeout | ✅ Manifest 3000s > worker 2400s (prior finding addressed) |
Prior findings — all resolved
Every finding from earlier reviews on this PR has been addressed in the current head:
- Missing command file → shipped
- Stale README catalog → refreshed
- Per-repo error isolation → implemented in shared
run_repositories() - Timeout mismatch → manifest timeout now exceeds the worker deadline
fire_callback/ completion lifecycle → owned by the Automation Service via therun_repositories+RemoteConversationpattern (confirmed by @neubig's audit)
Generated by OpenHands AI on behalf of the user.
|
@all-hands-bot Please review the current head after the final stack cleanup. This diff now contains only the triage extension, its catalog registration, and focused tests. If there are no material findings, submit a GitHub APPROVE review on this head. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: 96c81d4 (final stack cleanup)
Verdict: ✅ Approved — no material findings
The previous APPROVE was on 5f0c5df. The head has since advanced by three commits that remove PR-only artifacts (.pr/triage/), share the automation bundle loader in tests, and restack on shared test tooling. The test files themselves are unchanged from the previously approved head — only the .pr/triage/ scaffolding was deleted (101 lines removed, 0 added).
What was verified on this head
| Check | Result |
|---|---|
sync_extensions.py --check |
✅ Passes — README catalog and command file in sync |
| Full test suite (822 tests) | ✅ 822 passed, 24 skipped |
| Triage delivery contracts | ✅ Label preservation + ready-only-when-clear both pass |
| Skill installation test | ✅ Bundle files materialize as real files, import worker succeeds |
| Automation foundation tests | ✅ Per-repo isolation, pagination, dependency gating all pass |
worker.py syntax |
✅ Compiles cleanly |
github_client.py syntax |
✅ Compiles cleanly |
| Manifest vs. worker timeout | ✅ Manifest 3000s > worker 2400s |
Diff scope (vs. base factory/github-automation-foundation)
17 files changed, 391 insertions(+), 5 deletions(-):
- Triage extension:
skills/github-issue-triage/— SKILL.md, README.md, worker.py, command file, plugin metadata, symlinks - Catalog registration: manifest.json, bundle-index.js, catalog-index.js, marketplaces/openhands-extensions.json, skills/index.js, README.md catalog
- Tests:
tests/github_automation_helpers.py,tests/test_github_triage_delivery.py,tests/test_github_skill_installation.py
No PR-only artifacts remain. The diff is exactly the triage extension, its catalog registration, and focused tests.
Generated by OpenHands AI on behalf of the user.
Co-authored-by: openhands <openhands@all-hands.dev>
96c81d4 to
3e91247
Compare
|
@all-hands-bot Please review the current head |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: 3e912477d69fffdfc4db4bf9f666ee4ebabcf9ac
Verdict: ✅ Approved — no blocking findings
This head is a rebase of the previously approved 96c81d4 onto the merged #581 base (origin/main). The tree is identical (git diff 96c81d4..3e91247 is empty), so the code content is unchanged from the prior approval.
What was verified
| Check | Result |
|---|---|
sync_extensions.py --check |
✅ Passes — README catalog and command file in sync |
| Full test suite (822 tests) | ✅ 822 passed, 24 skipped |
| Triage delivery contracts | ✅ Label preservation + ready-only-when-clear both pass |
| Skill installation test | ✅ Bundle files materialize as real files, import worker succeeds |
worker.py syntax |
✅ Compiles cleanly |
github_client.py syntax |
✅ Compiles cleanly |
| Manifest vs. worker timeout | ✅ Manifest 3000s > worker 2400s |
| Per-repo error isolation | ✅ run_repositories() catches per-repo exceptions |
| Tree identity vs. prior approval | ✅ git diff 96c81d4..3e91247 — empty |
Diff scope (vs. origin/main)
16 files changed, 366 insertions(+), 5 deletions(-):
- Triage extension:
skills/github-issue-triage/— SKILL.md, README.md, worker.py, command file, plugin metadata, symlinks - Catalog registration: manifest.json, bundle-index.js, catalog-index.js, marketplaces/openhands-extensions.json, skills/index.js, README.md catalog
- Tests:
tests/github_automation_helpers.py(shared loader),tests/test_github_triage_delivery.py,tests/test_github_skill_installation.py
Code assessment
The worker is clean and well-structured:
- Idempotency: SHA-256 content digest over issue title, body, and non-triage discussion comments. The marker is checked against all comments, so new discussion re-triggers triage while unchanged issues are skipped. Correct
for/else/breakidiom. - Label handling: Existing labels are preserved, stale priority labels are stripped, and
ready-for-devis added only when the LLM result is validated (non-empty string criteria + valid priority). Issues already carryingready-for-devare filtered out before processing. - Dependency checking:
completed_dependencycorrectly distinguishesclosed+completedfromclosed+not_planned, returnsFalseon 404, and propagates other HTTP errors (e.g. 403). - Security: Token name (not value) is validated by regex and passed to the agent.
shell()redacts the token from error output. Untrusted issue content is serialized viajson.dumpsand treated as data in the LLM prompt. - Failure isolation:
run_repositories()processes each repository independently; one failure does not skip the rest.
Non-blocking observation
popularityRank: 80 collides with github-agents-md-maintainer and upstream-fork-sync. This is cosmetic (stable sort handles ordering) and was noted as non-blocking in prior reviews. No change required for approval.
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>
|
@all-hands-bot Please review the current head |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: efd9fa8 (current HEAD of factory/issue-triage)
Verdict: Approved � no blocking findings
What changed since the last reviewed head (3e91247)
Two commits:
b38b33b�fix: use released SDK attach signature: removes thevisualizer=Noneargument fromRemoteConversation.attach()inworker.py, aligning with the released SDK v1.44.1 constructor signature (wherevisualizerdefaults toDefaultConversationVisualizer).efd9fa8�chore: refresh triage automation bundle: regeneratesautomations/bundle-index.jsso the embeddedworker.pystring matches the updated source exactly.
What was verified
| Check | Result |
|---|---|
worker.py syntax |
Compiles cleanly |
github_client.py syntax |
Compiles cleanly |
| Bundle-index.js vs source worker.py | Byte-identical match |
| Manifest timeout ordering | Manifest 3000s > worker 2400s |
github_client.py symlink |
Resolves to shared skills/github/scripts/github_client.py |
| Catalog registration | Present in bundle-index, catalog-index, skills/index, marketplace, README |
sync_extensions.py --check |
Passes (only non-blocking coverage warning for unrelated plugin) |
| Triage delivery contracts | 2/2 pass (label preservation + ready-only-when-clear) |
| Skill installation test | Bundle files materialize as real files, import worker succeeds |
| Shared client installation | 4/4 GitHub skills install shared github_client.py correctly |
| Full test suite | 822 passed, 24 skipped |
Code assessment
The worker is clean and well-structured:
- Idempotency: SHA-256 content digest over issue title, body, and non-triage discussion comments. The marker is checked against all comments, so new discussion re-triggers triage while unchanged issues are skipped. Correct
for/else/breakidiom. - Label handling: Existing labels are preserved, stale priority labels are stripped, and
ready-for-devis added only when the LLM result is validated (non-empty string criteria + valid priority). Issues already carryingready-for-devare filtered out before processing. - Dependency checking:
completed_dependencycorrectly distinguishesclosed+completedfromclosed+not_planned, returnsFalseon 404, and propagates other HTTP errors (e.g. 403). - Security: Token name (not value) is validated by regex and passed to the agent.
shell()redacts the token from error output. Untrusted issue content is serialized viajson.dumpsand treated as data in the LLM prompt. - Failure isolation:
run_repositories()processes each repository independently; one failure does not block the rest. - SDK signature:
RemoteConversation.attach()now uses the released SDK signature withoutvisualizer=None, matching the v1.44.1 default.
Prior findings � all resolved
Every finding from earlier reviews has been addressed:
- Missing command file � shipped
- Stale README catalog � refreshed
- Per-repo error isolation � implemented in shared
run_repositories() - Timeout mismatch � manifest timeout now exceeds the worker deadline
fire_callback/ completion lifecycle � owned by the Automation Service via therun_repositories+RemoteConversationpattern
Non-blocking observation
popularityRank: 80 collides with github-agents-md-maintainer and upstream-fork-sync. This is cosmetic (stable sort handles ordering) and was noted as non-blocking in prior reviews. No change required for approval.
Generated by OpenHands AI on behalf of the user.
Review: feat: add independent GitHub issue triage (#570)Head reviewed: Verdict: Approved � no blocking findingsWhat changed since the last reviewed head (
|
| Check | Result |
|---|---|
worker.py syntax |
Compiles cleanly |
github_client.py syntax |
Compiles cleanly |
| Bundle-index.js vs source worker.py | Byte-identical match |
| Manifest timeout ordering | Manifest 3000s > worker 2400s |
github_client.py symlink |
Resolves to shared skills/github/scripts/github_client.py |
| Catalog registration | Present in bundle-index, catalog-index, skills/index, marketplace, README |
sync_extensions.py --check |
Passes (only non-blocking coverage warning for unrelated plugin) |
| Triage delivery contracts | 2/2 pass (label preservation + ready-only-when-clear) |
| Skill installation test | Bundle files materialize as real files, import worker succeeds |
| Shared client installation | 4/4 GitHub skills install shared github_client.py correctly |
| Full test suite | 822 passed, 24 skipped |
Code assessment
The worker is clean and well-structured:
- Idempotency: SHA-256 content digest over issue title, body, and non-triage discussion comments. The marker is checked against all comments, so new discussion re-triggers triage while unchanged issues are skipped. Correct
for/else/breakidiom. - Label handling: Existing labels are preserved, stale priority labels are stripped, and
ready-for-devis added only when the LLM result is validated (non-empty string criteria + valid priority). Issues already carryingready-for-devare filtered out before processing. - Dependency checking:
completed_dependencycorrectly distinguishesclosed+completedfromclosed+not_planned, returnsFalseon 404, and propagates other HTTP errors (e.g. 403). - Security: Token name (not value) is validated by regex and passed to the agent.
shell()redacts the token from error output. Untrusted issue content is serialized viajson.dumpsand treated as data in the LLM prompt. - Failure isolation:
run_repositories()processes each repository independently; one failure does not block the rest. - SDK signature:
RemoteConversation.attach()now uses the released SDK signature withoutvisualizer=None, matching the v1.44.1 default.
Prior findings � all resolved
Every finding from earlier reviews has been addressed:
- Missing command file � shipped
- Stale README catalog � refreshed
- Per-repo error isolation � implemented in shared
run_repositories() - Timeout mismatch � manifest timeout now exceeds the worker deadline
fire_callback/ completion lifecycle � owned by the Automation Service via therun_repositories+RemoteConversationpattern
Non-blocking observation
popularityRank: 80 collides with github-agents-md-maintainer and upstream-fork-sync. This is cosmetic (stable sort handles ordering) and was noted as non-blocking in prior reviews. No change required for approval.
Generated by OpenHands AI on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: efd9fa8 (current HEAD of factory/issue-triage)
Verdict: Approved � no blocking findings
What changed since the last reviewed head (3e91247)
Two commits:
b38b33b�fix: use released SDK attach signature: removes thevisualizer=Noneargument fromRemoteConversation.attach()inworker.py, aligning with the released SDK v1.44.1 constructor signature (wherevisualizerdefaults toDefaultConversationVisualizer).efd9fa8�chore: refresh triage automation bundle: regeneratesautomations/bundle-index.jsso the embeddedworker.pystring matches the updated source exactly.
What was verified
| Check | Result |
|---|---|
worker.py syntax |
Compiles cleanly |
github_client.py syntax |
Compiles cleanly |
| Bundle-index.js vs source worker.py | Byte-identical match |
| Manifest timeout ordering | Manifest 3000s > worker 2400s |
github_client.py symlink |
Resolves to shared skills/github/scripts/github_client.py |
| Catalog registration | Present in bundle-index, catalog-index, skills/index, marketplace, README |
sync_extensions.py --check |
Passes (only non-blocking coverage warning for unrelated plugin) |
| Triage delivery contracts | 2/2 pass (label preservation + ready-only-when-clear) |
| Skill installation test | Bundle files materialize as real files, import worker succeeds |
| Shared client installation | 4/4 GitHub skills install shared github_client.py correctly |
| Full test suite | 822 passed, 24 skipped |
Code assessment
The worker is clean and well-structured:
- Idempotency: SHA-256 content digest over issue title, body, and non-triage discussion comments. The marker is checked against all comments, so new discussion re-triggers triage while unchanged issues are skipped. Correct
for/else/breakidiom. - Label handling: Existing labels are preserved, stale priority labels are stripped, and
ready-for-devis added only when the LLM result is validated (non-empty string criteria + valid priority). Issues already carryingready-for-devare filtered out before processing. - Dependency checking:
completed_dependencycorrectly distinguishesclosed+completedfromclosed+not_planned, returnsFalseon 404, and propagates other HTTP errors (e.g. 403). - Security: Token name (not value) is validated by regex and passed to the agent.
shell()redacts the token from error output. Untrusted issue content is serialized viajson.dumpsand treated as data in the LLM prompt. - Failure isolation:
run_repositories()processes each repository independently; one failure does not block the rest. - SDK signature:
RemoteConversation.attach()now uses the released SDK signature withoutvisualizer=None, matching the v1.44.1 default.
Prior findings � all resolved
Every finding from earlier reviews has been addressed:
- Missing command file � shipped
- Stale README catalog � refreshed
- Per-repo error isolation � implemented in shared
run_repositories() - Timeout mismatch � manifest timeout now exceeds the worker deadline
fire_callback/ completion lifecycle � owned by the Automation Service via therun_repositories+RemoteConversationpattern
Non-blocking observation
popularityRank: 80 collides with github-agents-md-maintainer and upstream-fork-sync. This is cosmetic (stable sort handles ordering) and was noted as non-blocking in prior reviews. No change required for approval.
Generated by OpenHands AI on behalf of the user.
|
@all-hands-bot Your current-head review says “Approved — no blocking findings,” but it was posted only as an issue comment. Please submit a formal GitHub |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: add independent GitHub issue triage (#570)
Head reviewed: efd9fa80fb275633aa89b7ec2b89a6ee3b7802ba (current HEAD of factory/issue-triage)
Verdict: Approved — no blocking findings
What changed since the last reviewed head (3e91247)
Two commits:
b38b33b—fix: use released SDK attach signature: removes thevisualizer=Noneargument fromRemoteConversation.attach()inworker.py, aligning with the released SDK v1.44.1 constructor signature (wherevisualizerdefaults toDefaultConversationVisualizer).efd9fa8—chore: refresh triage automation bundle: regeneratesautomations/bundle-index.jsso the embeddedworker.pystring matches the updated source exactly.
What was verified
| Check | Result |
|---|---|
worker.py syntax |
Compiles cleanly |
github_client.py syntax |
Compiles cleanly |
| Bundle-index.js vs source worker.py | Byte-identical match |
| Manifest timeout ordering | Manifest 3000s > worker 2400s |
github_client.py symlink |
Resolves to shared skills/github/scripts/github_client.py |
| Catalog registration | Present in bundle-index, catalog-index, skills/index, marketplace, README |
sync_extensions.py --check |
Passes (only non-blocking coverage warning for unrelated plugin) |
| Triage delivery contracts | 2/2 pass (label preservation + ready-only-when-clear) |
| Skill installation test | Bundle files materialize as real files, import worker succeeds |
| Shared client installation | 4/4 GitHub skills install shared github_client.py correctly |
| Full test suite | 822 passed, 24 skipped |
Code assessment
The worker is clean and well-structured:
- Idempotency: SHA-256 content digest over issue title, body, and non-triage discussion comments. The marker is checked against all comments, so new discussion re-triggers triage while unchanged issues are skipped. Correct
for/else/breakidiom. - Label handling: Existing labels are preserved, stale priority labels are stripped, and
ready-for-devis added only when the LLM result is validated (non-empty string criteria + valid priority). Issues already carryingready-for-devare filtered out before processing. - Dependency checking:
completed_dependencycorrectly distinguishesclosed+completedfromclosed+not_planned, returnsFalseon 404, and propagates other HTTP errors (e.g. 403). - Security: Token name (not value) is validated by regex and passed to the agent.
shell()redacts the token from error output. Untrusted issue content is serialized viajson.dumpsand treated as data in the LLM prompt. - Failure isolation:
run_repositories()processes each repository independently; one failure does not block the rest. - SDK signature:
RemoteConversation.attach()now uses the released SDK signature withoutvisualizer=None, matching the v1.44.1 default.
Prior findings — all resolved
Every finding from earlier reviews has been addressed:
- Missing command file → shipped
- Stale README catalog → refreshed
- Per-repo error isolation → implemented in shared
run_repositories() - Timeout mismatch → manifest timeout now exceeds the worker deadline
fire_callback/ completion lifecycle → owned by the Automation Service via therun_repositories+RemoteConversationpattern
Non-blocking observation
popularityRank: 80 collides with github-agents-md-maintainer and upstream-fork-sync. This is cosmetic (stable sort handles ordering) and was noted as non-blocking in prior reviews. No change required for approval.
Generated by OpenHands AI on behalf of the user.
|
A formal GitHub
The review body carries the same assessment as the prior issue comment (no blocking findings, all prior findings resolved, one non-blocking Generated by OpenHands AI on behalf of the user. |
3 participants
Why
The factory needs a dedicated triage role that turns unclear GitHub issues into prioritized, testable work before a developer starts.
Summary
Add one
github-issue-triageskill and catalog automation. It examines one changed, unblocked issue per run, asks the run's assigned agent for acceptance criteria and priority, posts a readable triage comment, and addsready-for-devonly when the result is actionable. A digest prevents repeated triage of unchanged discussion.GitHub transport and repository iteration come from #581. This PR adds no general runtime, profile, scheduling, or GitHub support machinery and does not modify another automation.
Issue Number
Closes #566
How to Test
The recording shows the installed automation turning Airbnb issue #47 into ready work through this readable triage result; the role behavior is unchanged by the stack split.
Relationship
Independent
main-based PR. Shared GitHub support from #581 is merged; this change does not depend on the other factory role PRs.