refactor(executor): make the three protection-bearing context fields REQUIRED (#9541) - #9582
Conversation
…REQUIRED (#9541) Deliverable 3 of #9541, and the structural half of #9482. #9482's behavioural fixes landed: the approval-accept path now threads authorLogin, expectedBaseRef and moderationSettings. But it threads them by CONVENTION -- three explanatory comments at the call site -- while the type still marked all three optional. Nothing stopped the next caller omitting them again, which is precisely what #9482's own deliverable asked for: "prefer a shared, typed decision-pass context constructed by both paths over ad-hoc optional fields -- that structurally prevents the next omission." All three are now required on AgentActionExecutionContext, following #9539's required `decisionNowMs` precedent. `null` remains a valid value; what is no longer expressible is OMITTING the field. The distinction matters because each absence disabled a protection outright rather than degrading it: authorLogin step 8c's cap-lock key degrades to `contributor-cap-lock:<repo>:`, giving zero exclusion AND collapsing every accept-path merge in the repo onto one shared empty-author key; and maybeEscalateModeration early-returns, so enforcement closes recorded no violation and never counted toward the warning/ban threshold. expectedBaseRef #9055's base-retarget guard never runs, so a PR retargeted after staging merges into a base the reviewed diff and CI never targeted. A wrong-merge class. moderationSettings the repo's per-repo overrides silently fall back to global defaults. IssueActionExecutionContext gets the same treatment. #9482 only audited the PR context, but its twin carries two of the same fields with the same hazard -- fixing one type and not the other is exactly the drift this deliverable exists to end. WHAT THE TYPE CHANGE FOUND Four more call sites in processors.ts omitting expectedBaseRef (and two also moderationSettings), none of them audited by #9482. All four are close or update_branch paths where null is genuinely correct -- a close has no base to protect against a retarget -- so the fix is a stated `null` with the reason, not a behaviour change. That they were invisible until the compiler demanded an answer is the argument for the type guarantee over a convention. Behaviour-preserving: the full suite passes 23,940 tests, with test fixtures updated only to supply the fields real callers must now supply. Two tests that deliberately exercised the absent-author degradation move from `undefined` to `null` -- the same case, spelled the way the type now expresses it.
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-28 18:30:25 UTC
Review summary Nits — 5 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9582 +/- ##
==========================================
- Coverage 89.84% 89.03% -0.82%
==========================================
Files 875 875
Lines 110980 110990 +10
Branches 26402 26412 +10
==========================================
- Hits 99706 98815 -891
- Misses 9992 11155 +1163
+ Partials 1282 1020 -262
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Closes #9541 — deliverable 3 of 3, and the structural half of #9482.
Deliverable 1 shipped as #9580; deliverable 2 as #9557. This completes the issue.
The gap this closes
#9482's behavioural fixes already landed: the approval-accept path now threads
authorLogin,expectedBaseRefandmoderationSettings.But it threads them by convention — three explanatory comments at one call site — while the type still marked all three optional. Nothing stopped the next caller omitting them again. That is exactly what #9482's own deliverable asked for:
What changes
All three are now required on
AgentActionExecutionContext, following #9539's requireddecisionNowMsprecedent.nullremains a valid value. What is no longer expressible is omitting the field. That distinction is the whole point, because each absence disabled a protection rather than degrading it:authorLogincontributor-cap-lock:<repo>:— zero exclusion, and every accept-path merge in the repo collapsing onto one shared empty-author key. Separately,maybeEscalateModerationearly-returns, so enforcement closes recorded no violation and never counted toward the warning/ban threshold.expectedBaseRefmoderationSettingsIssueActionExecutionContextgets the same treatment. #9482 audited only the PR context, but its twin carries two of the same fields with the same hazard — fixing one type and not the other is precisely the drift this deliverable exists to end.What the type change found
Four more call sites in
processors.tsomittingexpectedBaseRef(two alsomoderationSettings), none of them audited by #9482.All four turn out to be close or
update_branchpaths wherenullis genuinely correct — a close has no base to protect against a retarget — so the fix is a statednullwith the reason, not a behaviour change.That they stayed invisible until the compiler demanded an answer is the entire argument for a type guarantee over a convention.
Behaviour-preserving
Full suite: 24,016 passed. Test fixtures were updated only to supply the fields real callers must now supply.
Two tests that deliberately exercised the absent-author degradation move from
undefinedtonull— the same case, spelled the way the type now expresses it. They still assert the same behaviour (maybeEscalateModerationskipping, the cap-recheck path), so the degradation stays covered; it just can no longer be reached by accident.Testing
Two type-level invariants pin the guarantee itself: that
nullstays expressible (so "no author / no base / inherit defaults" remains a stated choice rather than a forbidden one), and that a real value is carried through unchanged. Both would failtscif any field reverted to optional.