Skip to content

fix(test): repair main — #9813 changed three contracts and updated only some callers - #9831

Closed
JSONbored wants to merge 1 commit into
mainfrom
fix/main-red
Closed

fix(test): repair main — #9813 changed three contracts and updated only some callers#9831
JSONbored wants to merge 1 commit into
mainfrom
fix/main-red

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

main is red

#9813 changed three things and updated only some of their callers:

change missed caller
ignoredCheckRuns added as an 8th arg to fetchLiveCiAggregate* agent-approval-queue.test.ts ×3, queue.test.tstoHaveBeenCalledWith pinned the 7-arg shape
|ign: fragment added to the durable CI-cache key queue.test.ts asserted |adv: only
ignoredCheckRuns documented in .loopover.yml.example config/examples/loopover.full.yml must match it from the WHERE IT LIVES marker onward

Six failures, on main, independent of any open PR — confirmed by checking out origin/main clean and reproducing.

My error, and the specific process failure

I verified #9813 by running the touched test files, not the full suite. All three broken files assert on contracts that PR changed while living elsewhere in the tree, so none of them ran before merge. Targeted runs cannot catch a contract change's blast radius — that is exactly what the full suite is for.

The fixes

  • Add the 8th argument to the four call-shape assertions, each commented with what it is.
  • Update the cache-key assertion to \|adv:\|ign:, and rewrite the comment to explain why both fingerprints are folded in (a config change to either must invalidate the cached aggregate rather than serve a stale verdict) — the old comment named only the advisory half.
  • Port the ignoredCheckRuns block into config/examples/loopover.full.yml so the two examples are byte-identical from the marker onward, which is what the test actually requires.

No production code changes — these are test/doc contracts catching up with a shipped change.

Verification

Full unit suite: 1314 files, 25,209 tests passed, 3 skipped, zero failures. tsc clean.

One thing worth recording: mcp-cli-completion-spec and mcp-cli-response-validation fail on a bare vitest run unless npm run build:mcp has been run first — they exercise the built CLI. CI builds it as part of test:ci; a local full-suite run needs it explicitly, or you get three failures that look real and are not.

…ly some callers

#9813 added `ignoredCheckRuns` as an 8th argument to fetchLiveCiAggregate*, added
an `|ign:` fragment to the durable CI-cache key, and documented the field in
.loopover.yml.example. Three assertions were never updated with it, so main has
been red since it merged:

  - agent-approval-queue.test.ts x3: toHaveBeenCalledWith pinned the 7-arg shape
  - queue.test.ts: same arity pin, plus the cache key asserted as "|adv:" only
  - config-templates.test.ts: config/examples/loopover.full.yml must match
    .loopover.yml.example from the WHERE IT LIVES marker onward, and only the
    root example got the new block

My error, and specifically a verification error: I ran the touched test files
rather than the full suite, so three files that assert on contracts I changed
were never executed before merge. The full suite is green here
(25,209 passed) -- note mcp-cli-* additionally require `npm run build:mcp`
first, which CI does and a bare `vitest run` does not.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 12:24:41 UTC

3 files · 1 AI reviewer · 2 blockers · CI green · dirty

⏸️ Suggested Action - Manual Review

Review summary
This PR fixes three broken test/doc contracts left over from #9813, which added an 8th `ignoredCheckRuns` arg to `fetchLiveCiAggregate*` and a `|ign:` fragment to the durable CI-cache key without updating all callers/fixtures. The changes are mechanical: adding the 8th `undefined`/`null` argument to four `toHaveBeenCalledWith` assertions, updating the cache-key assertion to include `|ign:`, and porting the `ignoredCheckRuns` documentation block into `loopover.full.yml` so it matches `.loopover.yml.example`. The diff is narrow, well-targeted at repairing red-main test drift, matches its stated intent exactly, and the PR description documents verification (full suite passing, tsc clean) consistent with a pure test/doc fix with no production code changes visible in the diff.

Nits — 3 non-blocking
  • This PR description doesn't reference an open issue per the repo's issue-scope enforcement convention for contributor PRs — worth confirming this qualifies as maintainer-authorized review-stack repair work rather than requiring a linked issue.
  • config/examples/loopover.full.yml:293-311 duplicates the exact wording of the existing advisoryCheckRuns comment block near-verbatim; consider a brief cross-reference instead of re-deriving the full spoof-resistance rationale if the two ever need to change together.
  • Verify config/.loopover.yml.example was already updated by feat(gate): gate.ignoredCheckRuns — exclude a third-party check from CI resolution entirely #9813 or a prior CI job, since this PR only touches config/examples/loopover.full.yml and the description implies the two must match from the marker onward.

Concerns raised — review before merging

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example `Closes #123`) before opening the PR.

Decision drivers

  • ❌ Code review — 2 blockers (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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: 14 registered-repo PR(s), 13 merged, 357 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 357 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 357 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 4 steps 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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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.

Decision record
  • action: hold · clause: missing_linked_issue
  • config: c46c30f91c2353c9b35eae16934d9614124e64030e103c76acfbd73c67f25b23 · pack: oss-anti-slop · ci: passed
  • record: 2dd9973eb1dc531cbea32776e3793421d959eef263e8b1f64d9eb627bc60bbef (schema v5, head 0ce032d)

🟩 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

@JSONbored JSONbored self-assigned this Jul 29, 2026
@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Superseded by #9829, which landed the same three fixes first (the 8th-arg assertions, the |adv:|ign: cache key, and the config/examples/loopover.full.yml sync).

Verified against origin/main at 9d7f567 before closing: full unit suite 1314 files, 25,209 tests passed, zero failures. Nothing from this branch is still needed.

Worth carrying forward from the post-mortem regardless of which PR fixed it: the root cause was verifying #9813 with targeted test-file runs instead of the full suite, so three files asserting on the contracts it changed never ran before merge. Also note mcp-cli-* needs npm run build:mcp before a bare vitest run or it reports three failures that aren't real.

@JSONbored JSONbored closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant