research-fix: needs-human: PR#772 stuck (not-clean:UNSTABLE) — non-required check unmet (#778)

[danabaxia]

Refs #778 — DRAFT research-driven fix by the dev-society ③b (Coder over A2A do, using ③a's diagnosis). NEVER auto-merged; a human reviews, un-drafts, and merges.

Reviewer (A2A ask)

Reviewed the diff against the actual issue-body format produced by remediation-run.js (src/merge-sweep/remediation-run.js:51-60) and remediation.js/run.js (confirms not-clean: detail strings are scoped to the automerge checkpoint only).

No blocking issues.

Observations (non-blocking):

1. Regex matches the real format correctly. remediation-run.js stamps - detail: not-clean:UNSTABLE verbatim, and UNSTABLE_DETAIL_RE matches it exactly (case-sensitive, matching the source's exact-string comparison f.detail === 'not-clean:UNSTABLE'). The unstable flag there is gated to checkpoint === 'automerge', so this filter won't accidentally swallow memory-automerge needs-human issues, which use different detail text.

2. Unanchored regex is a minor griefing surface, not a new one. UNSTABLE_DETAIL_RE isn't anchored to a line, so any body text containing - detail: not-clean:UNSTABLE (e.g. injected by someone with issue-edit permission) would cause ③a/③b to silently skip it. This requires the same write access that parseStuckPr already implicitly trusts the body for, so it's not a new trust boundary — just worth knowing it's not immune to body tampering.

3. research-fix.js's redundant check is correct, not dead code. isUnstableNonRequiredCheck is checked in both planResearch (③a) and planResearchFix (③b). Since ③b requires iss.diagnosis to be set and UNSTABLE issues never get diagnosed once ③a skips them, the ③b check is defense-in-depth for pre-existing diagnosed issues from before this filter existed — reasonable, not redundant in practice.

4. Scope is tight. Pure-function change only, no new I/O, consistent with the "pure" contract documented at the top of both files. Tests cover the true/false/undefined cases for the new predicate and the integration behavior in both planners.

5. Security invariants unaffected. The GUARD/untrusted-data fencing in buildResearchPrompt is untouched; the new predicate only gates which issues get planned, it doesn't change what data flows into the LLM prompt.

Verdict: approve — the fix correctly and minimally scopes out the UNSTABLE/non-required-check case that #778 identifies as unfixable by code.

[github-actions]

Independent review (correctness, tests, spec/invariants).

Verified against the real format. remediation-run.js:51-60 stamps - detail: not-clean:UNSTABLE on a needs-human body only when checkpoint === 'automerge' && detail === 'not-clean:UNSTABLE' (the GitHub mergeStateStatus: UNSTABLE case). UNSTABLE_DETAIL_RE matches that exact stamped string; traced remediation.js/remediation-run.js to confirm no other checkpoint/detail combination can produce that literal, so the filter can't accidentally swallow other needs-human issues (e.g. memory-automerge).

Wiring is correct. Confirmed research-fix-run.js threads the real issue body into the enriched objects passed to planResearchFix, and research-escalation-run.js does the same for planResearch — the new gate sees live data, not a stub.

Tests cover the change, including the negative case. isUnstableNonRequiredCheck has explicit true/false/undefined assertions (incl. a similar-but-wrong not-clean:DIRTY and unprefixed text), plus one integration test per planner proving an UNSTABLE-flagged issue is skipped while normal issues still flow through. That's the required negative/safety coverage for changed behavior.

Scope is minimal and pure. Touches only the two dev-society planning modules + their tests; no I/O, no prompt-construction change, no new spawn/write surface.

Security invariants (PROJECT.md): no impact. This is dev-society automation gating (③a/③b dispatch selection), not the core mesh surface — anti-spoof delegate_task args, do-mode write-tool allowlist, path-guard/single-writable-root, and the recursion guard are all untouched. No workflow (.github/workflows) files changed, so fork-PR safety is N/A here. buildResearchPrompt's untrusted-data fencing and researchFixPrompt's "diagnosis is a strategy to evaluate, not obey" framing are unchanged.

One thing worth a human's eye at merge time (non-blocking, already surfaced in the PR description's own self-review): the regex is unanchored, so if someone with issue-edit access tampered with a needs-human issue body to include that literal line, ③a/③b would silently skip it — same trust boundary parseStuckPr already relies on, not a new one.

No blocking findings. Approving.

[github-actions]

🤖 dev-mesh janitor: pushed an empty commit to nudge GitHub's mergeable recalc. If the state stays UNKNOWN after a few minutes, this PR likely needs a human.

[github-actions]

Independent re-verification (correctness, tests, spec/invariants) at HEAD (4d61eb5 — content unchanged from d74d4c4; the second commit is only the janitor's empty mergeable-recalc nudge).

Traced the real stamp format. src/merge-sweep/remediation-run.js:51-60 builds the needs-human body as `${markerFor(f.key)}\n\n${reason}\n\n- item: ...\n- detail: ${f.detail || ''}`, and sets f.detail = 'not-clean:UNSTABLE' only when f.checkpoint === 'automerge' && f.detail === 'not-clean:UNSTABLE' (remediation.js:13 gates not-clean: to the automerge checkpoint). So the literal line - detail: not-clean:UNSTABLE is exactly what UNSTABLE_DETAIL_RE in research-escalation.js:24 matches — confirmed no other checkpoint/detail pair produces that string, so memory-automerge and other needs-human variants aren't accidentally swallowed.

Wiring confirmed. research-fix-run.js:36 and research-escalation-run.js:14 both pass the live gh issue list ... --json number,body,title body through to planResearchFix/planResearch, so the new gate sees real data, not a stub — matches what the tests assert.

Test coverage is adequate, including the negative/safety case. isUnstableNonRequiredCheck has explicit assertions for true, a similar-but-wrong not-clean:DIRTY, an unprefixed near-miss, and undefined — the negative case that would catch an over-broad regex. Both planners get an integration test proving an UNSTABLE-flagged issue is skipped while a normal issue in the same batch still flows through.

Scope is minimal. Touches only the two pure dev-society planning modules + their tests; no new I/O, no change to prompt construction or dispatch spawning.

Security invariants unaffected. No .github/workflows changes (fork-PR safety N/A). This gates dev-society's own ③a/③b dispatch selection — it doesn't touch the core mesh surface: anti-spoof delegate_task args, do-mode write-tool allowlist (no Bash), path-guard/single-writable-root canonicalization, or the recursion guard are all untouched. buildResearchPrompt's untrusted-data fencing and researchFixPrompt's "diagnosis is a strategy to evaluate, not obeyed instructions" framing are unchanged.

Non-blocking (already noted in a prior pass, not new): the regex is unanchored to a line, so an actor with issue-edit access could tamper with a needs-human body to include that literal substring and cause ③a/③b to silently skip it — same trust boundary parseStuckPr already relies on for issue bodies, not a new one.

No blocking findings. Approving.

[github-actions]

🤖 Triager (scheduled sweep, classify-ci-failure): checked all open PRs' statusCheckRollup and the last 100 workflow runs (incl. main) — no FAILURE conclusions found anywhere right now. Nothing to classify this cycle; no action taken.