fix: close the remaining 5 pre-existing test failures on main#9285
fix: close the remaining 5 pre-existing test failures on main#9285JSONbored wants to merge 1 commit into
Conversation
Follows #9276, which fixed 8 files / 12 cases and left these 4 files / 5 cases still red on main (and therefore red on every open PR). Each diagnosed to its introducing commit; two are real source gaps, two are stale sibling tests a deliberate behavior change never updated. SOURCE fixes: - selfhost/metrics.ts: register two counters emitted since #9243 and #9245 but never added to DEFAULT_METRIC_META, so renderMetrics() emitted them as bare undocumented samples with no HELP/TYPE — exactly what that drift guard exists to catch. Both commits even name the counter in their own body text; a pure registration miss, not a behavior decision. TEST fixes (source verified correct in both cases): - selfhost-pg-retention: the fake Postgres still matched the pre-#9083 ctid semi-join, so every DELETE fell through to rowCount 0. #9083 deliberately moved retention to an index-backed PK range delete with ORDER BY (the fix for prune-retention blowing its timeout and stalling permanently); it updated the SQLite twin but not the Postgres one. Regex now captures whichever key column is in play, keeping BOTH the mapped-PK and ctid-fallback paths exercised. - salvageability: #9085 made an absent blocker confidence degrade to CONFIDENCE_WHEN_UNSTATED (0.5) instead of 1.0, so it is sub-floor against the 0.93 default and the low-confidence hold owns the case. It renamed both sibling assertions in rules.test.ts but missed this third consumption site. Both calls kept — they are the only coverage of the default-floor arm and the nullish-confidence arm respectively. - worker-entry-boundary: false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string "crossed the visual-diff threshold" failed a green tree over a sentence, in a file worker-reachable since #4120 that imports none of these deps. Narrowed to scan module specifiers (reusing the file's own parseImportSpecifiers) — the only way a Node-only dep can actually reach the bundle. Added a discriminating test so it cannot pass vacuously; verified by injecting a real `import sharp` and confirming it fails, naming the file and specifier.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9285 +/- ##
===========================================
+ Coverage 75.38% 88.54% +13.16%
===========================================
Files 275 836 +561
Lines 58023 109515 +51492
Branches 6181 26084 +19903
===========================================
+ Hits 43739 96971 +53232
+ Misses 14014 11562 -2452
- Partials 270 982 +712
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-27 12:10:52 UTC
Review summary Nits — 4 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
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.
|
Summary
Follows #9276, which fixed 8 files / 12 cases and left 4 files / 5 cases still red on main — and therefore red on every open PR, including unrelated ones like #9283. Each traced to its introducing commit. Two are real source gaps; two are stale sibling tests that a deliberate behavior change never updated.
Source fixes
src/selfhost/metrics.ts— registers two counters emitted since #9243 (loopover_orb_relay_multiple_live_enrollments_total) and #9245 (loopover_private_manifest_warnings_total) but never added toDEFAULT_METRIC_META. Without an entry,renderMetrics()emits them as bare undocumented samples with no# HELP/# TYPE— exactly the gap that drift guard exists to catch. Both commits name the new counter in their own body text, so this is a pure registration miss, not a behavior decision.Test fixes (source verified correct in both cases)
selfhost-pg-retention— the fake Postgres still matched the pre-#9083ctidsemi-join shape, so everyDELETEfell through torowCount: 0and nothing was ever counted as deleted. #9083 deliberately moved retention to an index-backed PK range delete withORDER BY— the fix forprune-retentionblowing its 30-minute timeout and stalling retention permanently. It updated the SQLite twin (retention.test.ts) but not the Postgres one. I verified the source is correct before touching the test: all 14 mapped PK columns are real single-column primary keys, theWHEREpredicate is unchanged so the eligible set is identical, andORDER BYonly makes batches deterministically oldest-first. Retention deletes neither too much nor too little. Regex now captures whichever key column is in play, so both the mapped-PK andctid-fallback paths stay exercised.salvageability— #9085 made an absent blocker confidence degrade toCONFIDENCE_WHEN_UNSTATED(0.5) instead of 1.0, so it is sub-floor against the 0.93 gate default and the low-confidence hold owns that case. #9085 renamed both sibling assertions inrules.test.tsbut missed this third consumption site. Both calls kept deliberately — they are the only coverage in that file of theaiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCEdefault arm and theconfidence ?? CONFIDENCE_WHEN_UNSTATEDnullish arm respectively.worker-entry-boundary— a false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string"route(s) crossed the visual-diff threshold"failed a green tree over a sentence — in a file worker-reachable since #4120 that imports none of these deps. A Node-only dep can only reach the Worker bundle by being imported, so the check is now scoped to module specifiers, reusing the file's own existingparseImportSpecifiers. Bending correct user-facing copy to dodge a test regex would have been the wrong repair.Test plan
npx vitest run test/unit/salvageability.test.ts test/unit/selfhost-metrics.test.ts test/unit/selfhost-pg-retention.test.ts test/unit/worker-entry-boundary.test.ts— 69 passedretention.test.ts,selfhost-pg-dialect.test.ts,alerts-metric-name-references.test.ts(builds its allowed set from the same array),rules.test.ts,selfhost-pg.test.ts— 191 passed, 11 skippednpx tsc --noEmit -p tsconfig.json— cleanimport sharp from "sharp"into a worker-reachable file → check failed, naming the exact file and specifier; removed it → green. Also added a permanent test pinning both directions (flags real specifiers, ignores prose).