Skip to content

fix(review): clear stale disposition labels once the underlying hold resolves - #3575

Merged
JSONbored merged 1 commit into
mainfrom
claude/stale-disposition-label-cleanup
Jul 5, 2026
Merged

fix(review): clear stale disposition labels once the underlying hold resolves#3575
JSONbored merged 1 commit into
mainfrom
claude/stale-disposition-label-cleanup

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #3573

Summary

  • The review_state_label disposition ternary (ready-to-merge / manual-review /
    migration-collision / changes-requested) in planAgentMaintenanceActions
    (src/settings/agent-actions.ts) only ever ADDED the currently-correct label — it
    never removed a sibling label left over from a prior pass. A PR held once (e.g.
    changes-requested while CI was red) kept that label forever, even after becoming
    healthy again, alongside whatever the bot added next.
  • This has a real second-order effect: the AI-review freeze (isFrozenForManualReview
    in src/queue/processors.ts) checks whether the manual-review label is live on the
    PR to decide whether to reuse the last-published AI verdict instead of spending a
    fresh call. A manual-review label that never clears means that freeze can stay
    engaged indefinitely — producing a review comment whose deterministic parts (checks,
    gate result, linked-issue status) are fresh while the AI-authored "Review
    summary"/"Blockers" prose is stale, because the freeze never has a reason to lift.
    This is exactly what was observed on feat(review): hold PRs that solve an unlinked open issue #3513 after its own blockers were fixed.
  • Fix: add a cleanup pass, in the same review_state_label-gated section, that removes
    any of the three OTHER mutually-exclusive disposition labels still live on the PR
    once the ternary resolves to a different one this pass — mirroring the existing
    clearLinkedIssueFlag pattern already used for the pending-closure label.
    manual-review is excluded from this cleanup specifically when the separate
    owner/automation "not reviewGood" fallback hold (manualHoldReason, hoisted earlier
    in the function so this cleanup can see it) still independently wants it this same
    pass — that fallback can re-add the label later in the same call, and removing it
    here would race against that add.
  • manualHoldReason's computation was hoisted from the bottom of the function to
    right after ciReason (its own dependencies — guardrailHit, ciUnverified,
    conclusion, reviewGood, willClose, closeEligible — are all already available
    there); its later use is unchanged, just no longer redeclared.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused — one file changed (plus its test file), no apps/gittensory-ui/**,
    no generated-artifact changes needed (no OpenAPI/schema/migration/wrangler-binding
    changes).
  • Follows CONTRIBUTING.md; no site//CNAME/VitePress changes.
  • Linked issue: Closes #3573.

Validation

  • git diff --check
  • npm run actionlint — not run; no workflow files touched.
  • npm run typecheck (clean)
  • npm run test:coverage (full/unsharded) — not run locally; ran the specific
    affected files instead (agent-actions.test.ts, 229 tests; the full
    queue.test.ts integration suite, 572 tests) — all green. Cross-referenced the
    exact new-diff line/branch ranges against a fresh coverage-final.json
    (--coverage.include='src/settings/agent-actions.ts'): 100% of the new
    statements and branches are covered. GitHub CI runs the full suite/gate on push.
  • npm run test:workers / npm run build:mcp / npm run test:mcp-pack — not run;
    nothing in those surfaces touched.
  • npm run ui:openapi:check / ui:lint / ui:typecheck / ui:build — not run; no
    API/schema or apps/gittensory-ui/** changes.
  • npm audit --audit-level=moderate — not run; no dependency changes.
  • New/changed behavior has unit tests for every new branch: clearing a stale
    changes-requested/manual-review/ready-to-merge label individually, clearing
    multiple stale siblings at once, idempotency (no remove when only the correct
    label is present), a null-configured sibling label never producing a remove, the
    manualHoldReason-still-warranted exclusion (ciUnverified case), and confirming
    the pending-closure label's own dedicated clear path is untouched (no duplicate
    remove from the new sibling-cleanup loop).

If any required check was skipped, explain why:

  • Local validation here was typecheck + the specific affected test files (agent-actions.test.ts
    plus the full queue.test.ts integration suite) rather than a full local
    test:ci/test:coverage/npm audit pass, since GitHub CI runs the complete gate on
    push and re-running the whole suite by hand for every change is redundant.

Safety

  • No secrets, wallets, hotkeys, coldkeys, trust scores, or private scoring values are
    touched or exposed.
  • Public-facing text (the new "clearing the stale ... label" reason string) stays
    factual, no compensation/optimization claims — it's a reason string on an internal
    label action, not posted as a PR comment.
  • Not an auth/CORS/session change; no negative-path tests needed for that reason.
  • Not an API/OpenAPI change.
  • Not a UI change (N/A) — no UI Evidence section included.
  • No changelog edit.

Notes

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

Base automatically changed from claude/zealous-herschel-f6bfba to main July 5, 2026 17:35
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 5, 2026
@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-05 17:45:43 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
This PR adds a cleanup pass in planAgentMaintenanceActions that removes stale sibling disposition labels (readyToMerge/manualReview/migrationCollision/changesRequested) once the ternary picks a different one, fixing the described bug where a label like changes-requested lingered forever after a PR became healthy. The manualReview exclusion when manualHoldReason is non-null correctly avoids racing against the separate owner/automation fallback that can re-add manualReview later in the same pass, and the accompanying tests exercise each sibling-clearing path, the null-config case, the fallback-exclusion case, and non-interference with the pending-closure label. The main open question is whether manualHoldReason's new hoisted position still has all its free variables (reviewGood, willClose, closeEligible, acting, conclusion) already declared above it in the full file, since that isn't visible in the diff/omitted full file content.

Nits — 6 non-blocking
  • src/settings/agent-actions.ts: confirm reviewGood, willClose, closeEligible, and acting are all declared before the new hoisted manualHoldReason position (right after ciReason) — if any of them are declared later in the original file, this would be a TDZ ReferenceError, but this can't be confirmed from the diff alone.
  • The dispositionLabelSiblings loop iterates over input.pr.labels via hasLabel for each candidate rather than precomputing a Set, which is a minor style point given the array only ever has 3-4 entries.
  • If two disposition labels are ever misconfigured to the same string value, the loop would push a duplicate remove action for that label; worth a defensive dedupe or an assertion, though this is a config-time edge case.
  • Consider adding a short inline test comment cross-referencing feat(review): hold PRs that solve an unlinked open issue #3513 (the observed incident) in the test file the way the source comment does, to make the regression traceable from the test suite alone.
  • test/unit/agent-actions.test.ts: the 'never touches pending-closure label' test is a good regression guard — consider also asserting the sibling loop's array literal doesn't accidentally include pendingClosure by construction (e.g., via a type-level exhaustiveness check) rather than only via a behavioral test.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3573
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 46 merged, 521 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 56 PR(s), 521 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 56 PR(s), 521 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.15%. Comparing base (626b580) to head (1c6db66).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3575   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files         311      311           
  Lines       31702    31707    +5     
  Branches    11579    11581    +2     
=======================================
+ Hits        29533    29538    +5     
  Misses       1517     1517           
  Partials      652      652           
Files with missing lines Coverage Δ
src/settings/agent-actions.ts 97.53% <100.00%> (+0.06%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…resolves

The review_state_label ternary (ready-to-merge / manual-review /
migration-collision / changes-requested) only ever ADDED the current
state's label, never removed a sibling left over from a prior pass —
so a PR held once (e.g. changes-requested while CI was red) kept that
label forever even after becoming healthy again, alongside whatever
got added next. This also meant a stale manual-review label could
keep the AI-review freeze (isFrozenForManualReview) engaged forever
once a PR resolved, since nothing ever cleared it.

Adds a cleanup pass that removes any of the three mutually-exclusive
sibling labels still live on the PR once the ternary picks something
else, while leaving the owner/automation "not reviewGood" fallback
hold's own manual-review label alone when it's still independently
warranted this same pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

fix(review): stale disposition labels never clear once a hold resolves

1 participant