refactor(advisory): give addPullRequestFindings named signals instead of a 12-argument tail - #10214
Conversation
… of a 12-argument tail addPullRequestFindings exists twice, in the two deliberately-divergent advisory twins, and both took the same POSITIONAL tail. Every new signal had to be threaded in identical ORDER through two files -- a shape where transposing two same-typed arguments compiles cleanly and silently changes a verdict. #10205 added the twelfth argument and paid that tax in both signatures and both call sites; the next one would too. Both copies now take one named object. Each declares its own local type rather than sharing one: keeping these files free of a common import is precisely what the divergence exists for (#4518, keep-divergent recorded for #4881), so the engine still never reaches into the host's graph. Pure refactor, and the evidence is that NOTHING ELSE MOVED: no finding added or removed, no message text touched, no test edited. The diff contains zero changed lines matching findings.push or a finding code literal. The existing advisory suites on both sides, test:engine-parity and engine-parity:drift-check all pass unmodified, which is the whole regression proof -- if an assertion had needed editing, that would have meant behaviour changed. Closes #10210
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-31 13:15:07 UTC
Review summary Nits — 6 non-blocking
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. 🟩 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. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | c48137c | Commit Preview URL Branch Preview URL |
Jul 31 2026, 12:49 PM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10214 +/- ##
==========================================
- Coverage 92.22% 92.21% -0.01%
==========================================
Files 937 937
Lines 114334 114347 +13
Branches 27645 27646 +1
==========================================
+ Hits 105440 105449 +9
Misses 7588 7588
- Partials 1306 1310 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Summary
addPullRequestFindingsexists twice — insrc/rules/advisory.tsand its deliberately-divergent engine twinpackages/loopover-engine/src/advisory/gate-advisory.ts— and both took the same positional tail. Every new signal had to be threaded in identical order through two files.That is a shape where transposing two same-typed arguments compiles cleanly and silently changes a verdict. #10205 added the twelfth argument and paid the tax in both signatures and both call sites; the next signal would too.
Both copies now take one named object. Each declares its own local type rather than sharing one — keeping these two files free of a common import is precisely what the divergence exists for (#4518, keep-divergent recorded for #4881), so
@loopover/enginestill never reaches into the host's graph.Selected as #10170's first target for a specific reason, not arbitrarily: it is the only entry on that list that must be edited in two places every time, so the fix removes a recurring class of drift rather than tidying one signature. Context and the corrected measurements are in this comment on #10170 — the audit's original ranking counted commas inside JSDoc, which inflated the numbers and mis-ranked the targets.
Why you can trust it is behaviour-neutral
Nothing else moved. No finding added or removed, no message text touched, no test edited. The diff contains zero changed lines matching
findings.pushor a finding-code literal — only the signature, the destructure, and the single call site in each file.The existing advisory suites on both sides,
test:engine-parity, andengine-parity:drift-checkall pass unmodified. That is the whole regression proof: if an assertion had needed editing, it would have meant behaviour changed, and the right response would have been to re-examine the refactor rather than adjust the test.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123).Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragenpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateFull
npx vitest run: 1358 files, 26503 tests, 0 failures.@loopover/engine: 945 pass.test:engine-parity: 15 pass.engine-parity:drift-check: 7 twin pairs agree at engine 3.20.0. Also green:dead-source-files,dead-exports,import-specifiers,coverage-boltons,typecheck-coverage,docs:drift,cf-typegen.If any required check was skipped, explain why:
actionlint,test:workers,build:mcp,test:mcp-pack,ui:*. This PR touches exactly two TypeScript files undersrc/rules/andpackages/loopover-engine/src/advisory/— no workflow, worker-pool, MCP, OpenAPI, or UI file. CI runs them all.npm auditreports pre-existing advisories onmain; this PR changes no dependency (nopackage.json/ lockfile diff).Safety
UI Evidencesection below.No contributor-visible string changes at all — the findings this function emits are byte-identical.
UI Evidence
Not applicable — no UI, frontend, docs, or extension files are touched.
Notes
The first three arguments (
repo,pr,findings) stay positional: they are the genuine subject of the call, andfindingsis the accumulator being appended to. Only the resolved-signal tail moved into the object.Closes #10210