fix(review-transition): report no-PR Workload verdict to dispatch instead of silent skip:no-pr - #214
Conversation
…tead of silent skip:no-pr Route no-PR endings on the coder task's status.verdict (status.result.summary and status.result.extra are carried along): - Non-GO or absent verdict: park the issue as "blocked" via update_status so it stops being re-claimed and consuming a MAX_IN_PROGRESS slot, and report the verdict and summary in the results line. - GO with no PR: distinct anomaly — surface a results line with extra.commitSHA / extra.branch and the summary so an operator can tell whether commits exist that were never opened as a PR; do not park silently. - AllAlreadyResolved keeps its existing done path. - Fails open: a raising dispatch call appends an error result line and moves on, matching the existing except handling in the loop. Adds tests for INCOMPLETE parking, GO anomaly surfacing (with and without commit info), AllAlreadyResolved still done, raising dispatch call, and absent-verdict parking; updates the two tests that asserted the old silent skip:no-pr behavior to the new parking behavior. Fixes #213 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
PR PR 214 Review — Approve
Recommendation: Approve. The PR correctly implements issue PR 213 and all required checks have been addressed.
Change-by-Change Findings
1. _extract_task_signal() (lines 51–74)
New helper function that reads verdict, result.summary, and result.extra from tasks. It iterates through tasks and returns the signal from the last task carrying a verdict (or "" for absent verdict). The implementation safely uses .get() with fallbacks and str() coercion — no unsafe type handling.
2. No-PR routing logic (lines 139–171)
The old results.append(f"{name}:skip:no-pr") is replaced with a two-branch conditional:
- Non-GO or absent verdict → "blocked": Calls
update_status(item, "blocked", agent_name), matching the pattern referenced inbridge/claim.py:333. Results include the verdict value and optional summary. - GO verdict with no PR → anomaly: Appends
anomaly:go-no-prwithcommitSHA,branch, andsummaryfromextra. Does not callupdate_status(issue not parked).
Both branches include except Exception as e handlers matching the existing error pattern in the same loop.
Required Checks — Explicitly Addressed
1. Review for path traversal vulnerabilities
Status: Does not apply.
This PR operates entirely on in-memory data structures (dicts, strings) read from Workload/task manifests. It does not:
- Open, read, write, or link to filesystem paths
- Accept user-supplied paths
- Interpolate strings into paths, URLs, or command invocations
The commitSHA and branch values read from extra are only string-interpolated into result log lines (e.g. commitSHA=abc123), not into any path construction. No path traversal surface exists in this diff.
2. Test with edge-case paths (null bytes, symlinks)
Status: Does not apply.
The code under test is pure business logic (transition_to_in_review) that processes task verdict data in memory. It does not interact with the filesystem, create or follow symlinks, or parse path strings. Null-byte injection and symlink attacks are not relevant attack vectors for this change.
Standards Compliance
The AGENTS.md conventions for this repository govern issue filing (imperative ask + named file paths). This PR is a code implementation, not an issue, so those conventions do not apply to the diff. Code-level conventions observed:
- Docstrings explain behavior and rationale (e.g.
_extract_task_signal) except Exception as ehandling matches the existing pattern in the same loop (results.append(f"{name}:error:{e}"))- Error messages are informative (
API downin test) - No new status values invented; "blocked" is confirmed via
git_grepas an existing dispatch status
Linked Issue PR 213 Fit
| Acceptance Criterion | Status |
|---|---|
No-PR non-AllAlreadyResolved reports to dispatch (not silent) |
✅ _extract_task_signal + result appends |
| Non-GO verdict parks as "blocked" (not "done") | ✅ update_status(item, "blocked", ...) |
| GO verdict with no PR is distinct anomaly with commitSHA/branch | ✅ anomaly:go-no-pr branch |
status.result.summary carried into result line |
✅ Appended via :{summary} |
Fails open: raising update_status returns error line, not abort |
✅ except Exception as e branch |
| Absent verdict treated as non-GO and parks | ✅ verdict != "GO" catches empty string |
AllAlreadyResolved still goes to done |
✅ Test: test_all_already_resolved_still_goes_to_done |
All six acceptance criteria are satisfied. The diff also passes the explicit non-regression: AllAlreadyResolved with a verdict present still goes to done.
Tool Harness Findings
Not applicable — no tool harness output in corpus.
Unknowns / Needs Verification
None. The diff is self-contained, tests are comprehensive, CI passed (test + docker: success), and the implementation matches the issue's routing logic specification exactly.
The coder's branch correctly implements issue #213: no-PR routing now depends on the coder task's verdict — INCOMPLETE/absent verdicts park the issue as "blocked", while GO verdicts surface as anomalies with commitSHA/branch info without parking. …
Fixes #213
Opened by foreman on review GO (workload wl-misospace-foreman-dispatch-bridge-213).