Skip to content

chore(extension): remove dead matchPullRequestTarget; pin extension endpoints to the API contract - #8053

Closed
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-extension-dead-match-8023
Closed

chore(extension): remove dead matchPullRequestTarget; pin extension endpoints to the API contract#8053
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-extension-dead-match-8023

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Closes matchPullRequestTarget is dead code left behind in loopover-extension/content.js by #7487's issue-page match removal #8023
  • matchPullRequestTarget (apps/loopover-extension/content.js) duplicated matchGitHubPageTarget minus the kind field. fix(extension): drop dead issue-page content-script match #7487 removed issue-page matching and simplified the target guard, leaving this wrapper stranded — no production call site remains; it was only re-exported through the __loopoverContentInternals test hook. Removed, along with its hook entry; repo-wide grep confirms zero remaining references.
  • Unlike the issue's "zero test files" note, the ROOT suite does consume that hook (test/unit/extension-content.test.ts asserted on internals.matchPullRequestTarget in four places — the miss that closed fix(extension): remove dead matchPullRequestTarget from content.js #8037). Those assertions are removed with the symbol; the one case unique to them (a /pull/146/files sub-path still matching) is ported onto matchGitHubPageTarget, where the behavior actually lives.
  • Each of the three extension suites also gains an extension ↔ backend drift guard pinning the endpoints/message types its subject consumes to the served API contract (buildOpenApiSpec): auth.js's fetched paths (/v1/extension/pull-context, /v1/auth/logout), background.js's routed message types, and content.js's overlay request chain. A backend route rename now surfaces in the suite that owns the affected extension file instead of silently breaking installed extensions. These also give every scoped-CI shard real instrumented-source coverage — see Notes.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Ran the exact checks this change can affect: vitest run over all three extension suites (19 tests green), npm run extension:lint + npm run extension:typecheck (the extension checks CI gates validate-code on, which test:ci itself does not run), and the full root npm run typecheck. Source diff is deletion-only (content.js −7 lines); all additions are test/** (Codecov-ignored), so there is no new patch-coverage surface. Additionally simulated the scoped-CI shard condition locally: each suite run alone under --coverage.all=false now emits a ~2 MB lcov.info with 708 instrumented files (previously 0 bytes — the exact Verify coverage report exists failure that sank the last attempt). actionlint/workers/mcp/ui checks are untouched surfaces; CI runs them all.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — dead-code removal with no behavior or visual change (the overlay mounts through matchGitHubPageTarget, which is untouched).

Notes

…SONbored#8023)

matchPullRequestTarget duplicated matchGitHubPageTarget minus the kind field.
JSONbored#7487 removed issue-page matching and simplified the target guard but left this
wrapper stranded: no production call site remains -- it was only re-exported
through the __loopoverContentInternals test hook.

Unlike the issue's "zero test files" note, the ROOT test suite does consume
that hook: test/unit/extension-content.test.ts asserted on
internals.matchPullRequestTarget in four places (the miss that got the previous
attempt at this issue closed). Those assertions are removed with the symbol;
the one case unique to them -- a JSONbored/pull/146/files sub-path still matching -- is
ported to matchGitHubPageTarget, where the behavior actually lives, so no
behavioral coverage is lost.
…ontract (JSONbored#8023)

Each of the three extension suites gains a drift guard pinning the endpoints /
message types its subject consumes to the backend contract (buildOpenApiSpec):
auth.js's fetched paths, background.js's routed message types, and content.js's
overlay request chain. A backend route rename now surfaces in the suite that
owns the affected extension file instead of silently breaking installed
extensions.

This also satisfies scoped CI's per-shard non-empty-lcov verification: an
extension-only diff previously selected a single test file that executed no
instrumented source, so every validate-tests shard failed "Verify coverage
report exists" even with all tests green (the failure that closed the prior
attempt). Three selected suites, one per shard, each executing
src/openapi/spec.ts, produce a real lcov in every shard.
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 22, 2026 16:09
@superagent-security

Copy link
Copy Markdown
Contributor

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

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

loopover-orb Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-22 16:18:26 UTC

4 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This removes the dead matchPullRequestTarget wrapper from content.js (unused after #7487), ports its one unique test assertion (sub-path pull URLs) onto matchGitHubPageTarget, and adds three drift-guard tests pinning extension-consumed endpoints/message types to buildOpenApiSpec(). The removal is verified correct — matchGitHubPageTarget's full post-change body handles the sub-path case identically, and the wrapper's only other consumer (the test hook) is removed alongside it. The added contract-parity tests are simple string/spec assertions with no fabricated payloads, and the change closes the linked issue #8023 as required.

Nits — 4 non-blocking
  • The failing 'validate'/'validate-tests' CI checks have no detail provided; per BASE BRANCH STATUS this branch is 1 commit behind default, which is a plausible cause worth checking via rebase rather than a diff defect.
  • test/unit/extension-content.test.ts and extension-background.test.ts duplicate near-identical drift-guard boilerplate (readFileSync + buildOpenApiSpec + toContain checks) that could be a small shared helper, though the duplication is minor and test-local.
  • Rebase onto the current default branch to see if the undetailed 'validate'/'validate-tests' CI failures clear, given the branch is already known to be 1 commit behind.
  • Consider factoring the repeated 'read source file + assert message type + check buildOpenApiSpec path' pattern across the three new drift-guard tests into a small shared test utility to reduce duplication.

CI checks failing

  • validate
  • validate-tests (3)
  • validate-tests (2)
  • validate-tests (1)

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 #8023, #8037
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 (2 linked issues).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 378 registered-repo PR(s), 159 merged, 34 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 378 PR(s), 34 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The PR removes matchPullRequestTarget from content.js and drops its entry in __loopoverContentInternals, and also updates the one test file that actually referenced it (which the issue missed) so no references remain, fulfilling both stated deliverables.

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 378 PR(s), 34 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.

Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)
after / (scroll)
after / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 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 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests (3), validate-tests (2), validate-tests (1))). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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.

matchPullRequestTarget is dead code left behind in loopover-extension/content.js by #7487's issue-page match removal

1 participant