Skip to content

The manual-review label is a one-way latch: nothing lifts it when the hold that applied it clears #9939

Description

@JSONbored

Observed live on #9935 (orb-v3.7.0-beta.1).

The PR was MERGEABLE / mergeStateStatus: CLEAN, CI fully green, and a fresh escalated review on the current head returned zero findings. It still would not merge:

agent.action.merge   | denied | manual-review label "manual-review" is present on the live PR — merge not executed
agent.action.approve | denied | manual-review label "manual-review" is present on the live PR — approve not executed

The label had been applied several passes earlier, when an ai_consensus_defect fired against a stale head (fb4ffe52, pre-rebase). The finding was legitimate at the time — the branch carried generated-schema drift picked up from a main that was itself stale. A rebase removed it, api-schemas.ts went to a zero diff against main, and the re-review came back clean. None of that lifted the label, so the PR sat blocked until a human removed it — in the exact autonomy mode that is supposed to have no human in it.

Why it is a latch

agent-actions.ts clears stale disposition labels (readyToMerge / migrationCollision / changesRequested) once the disposition resolves, but deliberately excludes this one:

Intentionally do NOT remove manualReview here: that same label is also the live maintainer safety hold/freeze, and the planner has no provenance bit proving it was only a stale bot disposition rather than a human-applied hold. Only a maintainer removing the label should lift it.

That reasoning is correct and must not be reverted — the label is overloaded. A human applying manual-review to freeze a PR must have that respected, and auto-removal without provenance would silently override a deliberate human hold, which is far worse than the current stall.

The gap is the missing provenance, not the caution.

Fix

Record who applied it, then let only the bot lift what the bot applied:

  • Persist the provenance when the PLANNER adds manualReview — head SHA plus the hold reason, alongside the existing per-PR markers (visualCaptureRetryPendingSha and friends).
  • On a later pass: if the label is live and provenance says the bot applied it and the disposition no longer holds it for manual review, remove it and say why in the audit trail.
  • No provenance ⇒ never auto-removed. A human-applied label, or one predating this change, keeps exactly today’s behaviour. That is the property that makes this safe.
  • The removal must be scoped to the reason that applied it, so a label applied for reason A is not lifted by reason B clearing.

Same structural class as #9876: a latch whose release path did not exist, freezing PRs indefinitely. #9869 fixed the planner emitting the label alongside a merge in one pass; this is the other half — lifting one already applied.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions