Skip to content

fix(review): wire resolveDispositionReason into the action-executor notification (#6636) - #6686

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
nickmopen:fix/wire-resolveDispositionReason-6636
Jul 16, 2026
Merged

fix(review): wire resolveDispositionReason into the action-executor notification (#6636)#6686
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
nickmopen:fix/wire-resolveDispositionReason-6636

Conversation

@nickmopen

Copy link
Copy Markdown
Contributor

Summary

resolveDispositionReason (src/review/outcomes-wire.ts) was fully implemented and had a dedicated test suite, but zero call sites — the action executor emitted its Discord/Slack disposition notification with only the plain action.reason, so the enriched, verdict-aware reason the function was built to supply never reached users. Its own doc comment has promised that behavior since it was built. Closes #6636.

Fix

At the notify site in src/services/agent-action-executor.ts, resolve the enriched reason before building the notification params:

const summary = await resolveDispositionReason(env, `${ctx.repoFullName}#${ctx.pullNumber}`, action.reason);

resolveDispositionReason returns the latest recorded gate_decision summary for the PR (review_audit keys those rows by ${repoFullName}#${pullNumber}), and falls back to action.reason when no verdict is on record or the read fails — so the notification is byte-identical when there's nothing to enrich with. resolveDispositionReason's own logic is unchanged (it's already correct + tested); this is purely the missing wire-up.

Scope / Validation / Safety

  • Conventional Commit; focused; Closes #6636.
  • npm run typecheck (0 errors), engine build clean, git diff --check clean, no attribution/secrets.
  • Regression test: spies on notifyActionToDiscord and asserts the notification carries the enriched gate-verdict summary for a PR with a recorded verdict, and the plain disposition reason for a PR without one — covering both branches at the new call site.
  • The changed source line is covered (hit via existing merge/close executor tests + the new regression test); test/unit/agent-action-executor.test.ts + outcomes-wire.test.ts green (219 tests).
  • resolveDispositionReason's logic untouched; no behavior change when no verdict is recorded.

Closes #6636

…otification (JSONbored#6636)

resolveDispositionReason (outcomes-wire.ts) was fully implemented + tested but had
zero call sites: the action executor emitted disposition notifications with only the
plain `action.reason`, never the enriched, verdict-aware reason the function builds.

Call it at the Discord/Slack notify site so the notification shows the AI's actual
recorded gate-verdict reasoning (latest gate_decision summary for the PR) when one is
on record, falling back to the plain reason otherwise — byte-identical when there's
nothing to enrich with. Adds a regression test asserting the enriched reason (not the
plain reason) reaches the notification, plus the plain-reason fallback path.
@nickmopen
nickmopen requested a review from JSONbored as a code owner July 16, 2026 18:46
@superagent-security

Copy link
Copy Markdown
Contributor

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

1 similar comment
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.65%. Comparing base (986a12a) to head (5b481a5).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6686      +/-   ##
==========================================
- Coverage   93.65%   93.65%   -0.01%     
==========================================
  Files         681      682       +1     
  Lines       68114    68107       -7     
  Branches    18706    18707       +1     
==========================================
- Hits        63791    63784       -7     
  Misses       3347     3347              
  Partials      976      976              
Flag Coverage Δ
shard-1 43.90% <100.00%> (-0.07%) ⬇️
shard-2 36.98% <100.00%> (+0.11%) ⬆️
shard-3 32.39% <0.00%> (-0.02%) ⬇️
shard-4 34.24% <100.00%> (-0.44%) ⬇️
shard-5 31.38% <0.00%> (+0.47%) ⬆️
shard-6 45.76% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/services/agent-action-executor.ts 97.01% <100.00%> (+<0.01%) ⬆️

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 19:05:35 UTC

2 files · 1 AI reviewer · no blockers · readiness 86/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wires the previously-orphaned resolveDispositionReason into the notification path in agent-action-executor.ts, replacing the plain action.reason with an enriched gate-verdict summary that falls back to the original reason when no verdict is on record. The change is minimal, targeted, and directly closes #6636, and the new test exercises both branches (enriched and fallback) at the actual call site using a real DB insert rather than a mock, which is good evidence of the real path being tested.

Nits — 4 non-blocking
  • The inline comment block at agent-action-executor.ts:566-569 is fairly verbose for a one-line wire-up; consider trimming to a single line referencing the issue and behavior.
  • Confirm resolveDispositionReason's failure path (DB read error) is exercised somewhere, since the PR description claims a fallback on read failure but the new test only covers 'no verdict recorded', not an actual read failure.
  • Consider a short comment or test asserting resolveDispositionReason correctly awaits/handles a thrown DB error to fully cover the 'read fails' fallback claim in the description.
  • The two `await notifyActionToDiscord(...).catch()` / `notifyActionToSlack(...).catch()` calls now both depend on the resolved summary — worth confirming both paths are covered by the existing test suite (Slack notification isn't asserted in the new test).

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6636
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High 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: 145 registered-repo PR(s), 93 merged, 8 issue(s).
Contributor context ✅ Confirmed Gittensor contributor nickmopen; Gittensor profile; 145 PR(s), 8 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The PR imports and calls resolveDispositionReason at the notify site in agent-action-executor.ts, replacing the plain action.reason with the enriched summary while preserving fallback behavior, matching the issue's exact ask. It also adds a regression test covering both the enriched and fallback branches, satisfying the stated test coverage requirement.

Review context
  • Author: nickmopen
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 145 PR(s), 8 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 8ac47e3 into JSONbored:main Jul 16, 2026
15 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

resolveDispositionReason is fully implemented and tested but never called from production code

1 participant