Skip to content

fix(review): stop the surface lane from auto-closing clean registry submissions - #2586

Merged
JSONbored merged 2 commits into
mainfrom
fix/registry-surface-companion-and-duplicate-warning
Jul 2, 2026
Merged

fix(review): stop the surface lane from auto-closing clean registry submissions#2586
JSONbored merged 2 commits into
mainfrom
fix/registry-surface-companion-and-duplicate-warning

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Two bugs in the registry surface-review lane were closing (or routing to manual review) structurally clean registry submissions:

Bug 1 — a genuine debut-provider companion was rejected even though the scope classifier already approved it. classifyRegistryPrScope (src/review/content-lane/registry-logic.ts) already computes that a companion file matching providerFilePattern is a legitimate part of an "entry + its first provider in the same PR" submission — but runSurfaceReview (src/review/content-lane/orchestrator.ts) threw that away and routed any companion file straight to manual review, never invoking the already-validated standalone-provider path for it. RegistryScopeResult now carries a providerCompanionFile field; when present, the orchestrator loads and validates it via the spec's assessProviderEntry and combines that with the entry's own aggregate assessment — merge only when both sides are clean, close if either is invalid. artifactPattern companions (generated build output) remain allowed as-is with no validation attempted. The entry head/base fetch and the companion fetch now run concurrently (Promise.all) instead of three sequential round-trips.

Bug 2 — a warning-severity finding could singlehandedly force a hard close. duplicate_pr_risk (a same-linked-issue overlap finding) is always pushed at severity: "warning", but duplicatePrGateMode: "block" (the deliberate, twice-reaffirmed default — see #488 and #644) escalates it into a hard blocker in the generic gate. applySurfaceGate (src/review/content-lane-wire.ts) then unconditionally unioned that blocker into the final conclusion, even when the surface lane's own deterministic verdict for that PR was a clean merge — a same-linked-issue lead is not proof of a defect in the PR's own content. There's already a purpose-built escape hatch for the legitimate "sequenced, non-competing" case (isDuplicateClusterWinnerByClaim + GITTENSORY_DUPLICATE_WINNER), but it doesn't help every PR in a cluster — only the earliest claimant. A new isDuplicateOnlyFailure check (mirroring the existing isAiJudgmentOnlyFailure carve-out) now downgrades a duplicate_pr_risk-only generic failure to a neutral hold instead of a close when the surface lane merges cleanly, with the held check-run's title/summary naming the actual reason and the finding staying visible in warnings. This is scoped to exactly duplicate_pr_risk via a code allowlist, not every warning-severity finding — missing_linked_issue, self_authored_linked_issue, manifest_linked_issue_required, and manifest_missing_tests are also severity "warning" but are block-mode-escalatable via their own independent maintainer-configured gate, and that explicit opt-in must still close a PR outright. (An earlier draft of this fix used a blanket severity check instead of a code allowlist; an adversarial review caught that it would have silently defeated those other four gates, so it was replaced before this PR was opened.)

Both fixes are fully generic — no repo name, installation ID, or PR number is hardcoded into the engine.

Live evidence (JSONbored/metagraphed, read-only — not touched by this PR)

Operator note — GITTENSORY_DUPLICATE_WINNER

This PR does not flip GITTENSORY_DUPLICATE_WINNER on in the deployed environment. That flag (already fully implemented and already covered by pre-existing tests in test/unit/rules.test.ts and test/unit/queue.test.ts) is a global toggle affecting every installed repo, not something to change from inside a code PR — it needs an explicit deploy-time decision from whoever owns the deployment config. New tests in this PR (test/unit/rules.test.ts, test/unit/content-lane-wire.test.ts) exercise both the flag-on suppression path and the new neutral-hold path so the interaction is verified end to end regardless of when/whether the flag is enabled.

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.
  • No issue is linked — this is a direct maintainer fix against confirmed live evidence (see above), not tied to a filed issue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — the four changed src/** files (registry-logic.ts, orchestrator.ts, content-lane-wire.ts, advisory.ts) are 100% line-covered, and every uncovered branch reported for them is a pre-existing, untouched line (verified individually against git diff), not part of this diff.
  • 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

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. — N/A, no auth/session/CORS changes.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — no OpenAPI/MCP surface touched; ui:openapi:check confirms no drift.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, backend-only change.
  • Visible UI changes include a UI Evidence section below with screenshots. — N/A, backend-only change, no UI evidence needed.
  • Public docs/changelogs are updated where needed. — N/A, no user-facing docs affected.

Notes

  • See the operator note above re: GITTENSORY_DUPLICATE_WINNER.

…ubmissions

Two bugs in the registry surface-review lane were closing structurally
clean registry submissions:

1. classifyRegistryPrScope already approves a genuine debut-provider
   companion file riding alongside an entry submission (isAllowed
   matches providerFilePattern), but runSurfaceReview threw that away
   and routed any companion file straight to manual review. It now
   validates the companion via the spec's assessProviderEntry and
   combines it with the entry's own assessment: merge only when both
   sides are clean, close if either is invalid. artifactPattern
   companions (generated build output) are still allowed as-is with no
   validation attempted. The entry/base/companion fetches run
   concurrently instead of sequentially.

2. A duplicate_pr_risk finding (severity "warning") escalated into a
   hard blocker by duplicatePrGateMode: "block" was able to
   singlehandedly override a clean, deterministic surface-lane merge
   and force the whole PR closed via applySurfaceGate's unconditional
   union. It now downgrades to a neutral hold instead, mirroring the
   existing AI-judgment-only carve-out, and the held check-run's
   title/summary name the actual reason. This is scoped to exactly
   duplicate_pr_risk (not every warning-severity finding), since
   missing_linked_issue / self_authored_linked_issue /
   manifest_linked_issue_required / manifest_missing_tests are also
   warning-severity but block-mode-escalatable via their own
   independent maintainer-configured gate and must still close
   outright when a maintainer opts into that.

Both were confirmed live against JSONbored/metagraphed: PR #2654 (an
entry + debut-provider companion) and PR #2680 (a provider-only
resubmission sharing #2654's linked issue) now resolve to merge and a
held-for-review neutral hold respectively, instead of reject/close.
@dosubot dosubot Bot added the size:L label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 11:01:23 UTC

8 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change cleanly threads provider companion classification into the surface-lane orchestrator and preserves the intended entry/provider validation ordering, including the non-debut manual path. The duplicate-only gate carve-out is narrowly keyed to `duplicate_pr_risk` and keeps the signal visible by demoting it to warnings on a neutral hold. I do not see a reachable correctness break in the provided diff; the main maintainability issue is the amount of policy prose duplicated across code comments and tests.

Nits — 6 non-blocking
  • nit: `src/review/content-lane/orchestrator.ts:99` and `src/review/content-lane-wire.ts:24` carry very long policy comments that restate behavior already covered by tests, making future policy edits harder to keep synchronized.
  • nit: `test/unit/content-lane-orchestrator.test.ts` uses numbered labels like `[test 1]` and `[test 2a]`, which adds noise and can go stale as cases are reordered.
  • nit: `src/review/content-lane-wire.ts:131` joins multiple duplicate-only blocker reasons with a space; a semicolon separator would make the held summary clearer if more than one matching blocker is ever present.
  • In `src/review/content-lane-wire.ts:131`, build `heldReason` with `.join("; ")` so multiple demoted blocker details remain readable.
  • In `test/unit/content-lane-orchestrator.test.ts`, drop the bracketed test numbering and let the assertion text describe the scenario.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
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 (size label size:L; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 553 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 553 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 553 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.99%. Comparing base (be209f5) to head (304d945).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2586   +/-   ##
=======================================
  Coverage   95.98%   95.99%           
=======================================
  Files         229      229           
  Lines       25810    25837   +27     
  Branches     9389     9400   +11     
=======================================
+ Hits        24774    24801   +27     
  Misses        425      425           
  Partials      611      611           
Files with missing lines Coverage Δ
src/review/content-lane-wire.ts 98.50% <100.00%> (+0.06%) ⬆️
src/review/content-lane/orchestrator.ts 100.00% <100.00%> (ø)
src/review/content-lane/registry-logic.ts 100.00% <100.00%> (ø)
src/rules/advisory.ts 97.64% <100.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…merging it

classifyRegistryPrScope identifies a companion by file path alone
(providerFilePattern), which only proves the file is shaped like a
provider submission, not that it's a genuine debut (a brand-new
provider, not an edit to one already registered). runSurfaceReview now
also fetches the companion's base content and only runs it through the
debut-provider merge/close flow when base is absent; a companion that
already exists at base routes to manual instead, since editing an
existing, unrelated provider record alongside an entry submission is a
more sensitive shape that needs a human. The entry and both companion
refs are still fetched in one concurrent round-trip.
@JSONbored

Copy link
Copy Markdown
Owner Author

Pushed a follow-up fix: the companion-provider path was proving a companion is a provider-shaped file (path pattern match) but not that it's actually a debut (a brand-new provider, not an edit to one already registered).

runSurfaceReview now fetches the companion's base content too and only runs the debut-provider merge/close flow when base is absent — the same "null base = brand-new" convention already used for the entry file. If the companion already exists at base (an edit to an existing registry provider riding alongside an unrelated entry), it now routes to manual review instead of being silently validated and merged through the debut flow. All four reads (entry head/base, companion head/base) still resolve in one concurrent round-trip.

npm run test:ci, npm run test:coverage (100% line/branch on all touched files, net of pre-existing unrelated gaps), and npm audit --audit-level=moderate are all green.

@JSONbored
JSONbored merged commit efb87e4 into main Jul 2, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix/registry-surface-companion-and-duplicate-warning branch July 2, 2026 11:05
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.

Development

Successfully merging this pull request may close these issues.

1 participant