Skip to content

fix(agent-actions): recheck non-ci staged closes#2902

Merged
JSONbored merged 4 commits into
mainfrom
codex/propose-fix-for-non-ci-close-issue
Jul 4, 2026
Merged

fix(agent-actions): recheck non-ci staged closes#2902
JSONbored merged 4 commits into
mainfrom
codex/propose-fix-for-non-ci-close-issue

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Fix a safety gap where fresh non-CI heuristic closes persisted with closeRequiresCiState: "not_required" could be accepted and executed without any accept-time live revalidation, allowing a stale justification (conflict/duplicate/gate) to be acted on.
  • No linked issue: this is a small, self-evident safety-predicate fix in code the AI reviewer's own gate verdict on this PR pointed at directly (src/services/agent-approval-queue.ts:205), not a pre-planned feature.

Description

  • Reintroduced an accept-time live-disposition recheck in decidePendingAgentAction (in src/services/agent-approval-queue.ts) that runs the same fetchLiveCiAggregate/fetchLivePullRequestMergeState/fetchLivePullRequestReviewDecision checks used for merges when the pending row is a staged merge or a non-CI heuristic close with closeRequiresCiState === "not_required".
  • Bases the close-path staleness check only on mergeableState/reviewDecision, dropping the ciState === "passed" requirement: closeRequiresCiState: "not_required" already means CI was never this close's justification, so gating staleness on CI meant a cleared conflict on an otherwise-clean PR would still execute the close whenever live CI happened to be pending, failed, or unverified at accept time.
  • Added regression/unit tests in test/unit/agent-approval-queue.test.ts covering: live-CI-passing + clean → superseded; live conflict remains → still executes; clean mergeability with live CI merely pending (not passed) → still superseded (the case the gate's own AI review flagged as reachable and uncovered); a reviewer requesting changes → still executes.
  • Rebased onto current main (branch was ~280 commits behind).

Testing

  • npm run typecheck — clean.
  • git diff --check — clean.
  • npx vitest run test/unit/agent-approval-queue.test.ts — 62/62 passed.
  • Verified the new "clean mergeability + non-passed CI" regression test fails against the pre-fix predicate and passes against the fix (confirms it pins the fix, not a tautology).
  • Scoped vitest --coverage on src/services/agent-approval-queue.ts via this test file: 100% branch/line/statement coverage.

Codex Task

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.06%. Comparing base (fdda351) to head (fbd20aa).
⚠️ Report is 17 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2902   +/-   ##
=======================================
  Coverage   96.06%   96.06%           
=======================================
  Files         259      259           
  Lines       28655    28660    +5     
  Branches    10428    10432    +4     
=======================================
+ Hits        27528    27533    +5     
  Misses        490      490           
  Partials      637      637           
Files with missing lines Coverage Δ
src/services/agent-action-executor.ts 96.44% <100.00%> (+0.01%) ⬆️
src/services/agent-approval-queue.ts 100.00% <100.00%> (ø)
src/settings/agent-actions.ts 95.59% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 4, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-04 08:30:01 UTC

6 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI green · unknown

⏸️ Suggested Action - Manual Review

  • AI reviewers agree on a likely critical defect: src/services/agent-approval-queue.ts:205 in decidePendingAgentAction: a conflict-justified close is still accepted after the conflict clears when live reviewDecision is CHANGES_REQUESTED, so the stale base-conflict close can execute even though the persisted close justification no longer holds. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
The change correctly introduces and round-trips a dedicated discriminator for conflict-justified heuristic closes instead of applying the live mergeability recheck to every non-CI close. However, the close accept-time predicate still treats an unrelated live review decision as sufficient to execute a stale conflict close, and it leaves already-staged rows without the new discriminator on the old unsafe path despite these rows having no expiry.

Blockers

  • src/services/agent-approval-queue.ts:205 in decidePendingAgentAction: a conflict-justified close is still accepted after the conflict clears when live reviewDecision is CHANGES_REQUESTED, so the stale base-conflict close can execute even though the persisted close justification no longer holds.
  • src/services/agent-approval-queue.ts:179 in decidePendingAgentAction: any already-pending conflict-justified heuristic close created before closeRequiresMergeableState existed has closeRequiresMergeableState undefined, so shouldRecheckLiveDisposition skips the live recheck and preserves the no-expiry stale-close gap this PR is meant to close.
Nits — 5 non-blocking
  • nit: src/services/agent-approval-queue.ts:179 would read more clearly as an explicit boolean predicate instead of relying on pr?.headSha returning a string-or-falsey value.
  • nit: src/services/agent-approval-queue.ts:186 still fetches live CI for conflict-close acceptance even though the close branch intentionally ignores CI for the stale predicate, which spends rate limit only for audit metadata.
  • In src/services/agent-approval-queue.ts, make conflict-close staleness depend on the conflict signal itself: if the live mergeable-state fetch succeeds with mergeableState === "clean", reject as stale regardless of reviewDecision, and let any remaining review issues be replanned separately.
  • In src/services/agent-approval-queue.ts, add an explicit legacy-row policy for heuristic closes with closeRequiresCiState === "not_required" and closeRequiresMergeableState === undefined, such as fail-closed/manual replan or a narrowly documented backfill path.
  • In test/unit/agent-approval-queue.test.ts, replace the current "reviewer has since requested changes" expectation with a regression test proving a cleared conflict is superseded even when reviewDecision is CHANGES_REQUESTED.

Concerns raised — review before merging

  • src/services/agent-approval-queue.ts:205 in decidePendingAgentAction: a conflict-justified close is still accepted after the conflict clears when live reviewDecision is CHANGES_REQUESTED, so the stale base-conflict close can execute even though the persisted close justification no longer holds.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 60 registered-repo PR(s), 51 merged, 421 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 421 issue(s).
Gate result ❌ Blocking Repo-configured hard 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: 60 PR(s), 421 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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

@gittensory-orb gittensory-orb Bot added the gittensor Gittensor contributor context label Jul 4, 2026
@JSONbored JSONbored self-assigned this Jul 4, 2026
JSONbored added 2 commits July 4, 2026 00:34
…staleness

closeRequiresCiState: "not_required" means CI was never the justification
for a heuristic close, so gating the accept-time staleness check on
ciState === "passed" left the safety gap open whenever live CI was
pending, failed, or unverified: a cleared conflict on an otherwise-clean,
non-conflicting PR would still execute the close. Base staleness only on
mergeableState and reviewDecision, matching the non-CI signals the close
actually depended on.
@JSONbored JSONbored force-pushed the codex/propose-fix-for-non-ci-close-issue branch from 6a9984d to f2bdf81 Compare July 4, 2026 07:39
…tified closes

closeRequiresCiState === "not_required" covers every non-CI heuristic
close reason (duplicate PR, slop score, a gate blocker, or a base
conflict), but the accept-time recheck only has a cheap live signal for
the conflict case. Gating staleness on mergeableState/reviewDecision
for ALL of them meant a duplicate- or slop-justified close was
superseded merely because the PR happened to read clean, even though
its actual justification was still live.

Add closeRequiresMergeableState, set at plan time only when a base
conflict was part of the close's reasons, and scope the recheck to
that. Also stop treating a failed live review-decision read as
equivalent to "no changes requested" -- a rejected fetch now fails
open instead of silently satisfying the staleness check.
… level

Prove the planner sets closeRequiresMergeableState: true only when a
base conflict is part of the close's reasons, and false for a
duplicate-justified close with no conflict -- the approval queue's
accept-time recheck depends on this discriminator being planned
correctly, not just consumed correctly.
@JSONbored JSONbored merged commit 706d1c1 into main Jul 4, 2026
11 checks passed
@JSONbored JSONbored deleted the codex/propose-fix-for-non-ci-close-issue branch July 4, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. gittensor Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant