Skip to content

docs(review): document the mixed-files vs manual asymmetry for multi-provider companions - #2587

Merged
JSONbored merged 1 commit into
mainfrom
fix/registry-companion-scope-asymmetry
Jul 2, 2026
Merged

docs(review): document the mixed-files vs manual asymmetry for multi-provider companions#2587
JSONbored merged 1 commit into
mainfrom
fix/registry-companion-scope-asymmetry

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

classifyRegistryPrScope's early mixed-files guard is:

if (entryFiles.length > 1 || (entryFiles.length === 0 && providerFiles.length > 1)) {
  return { scope: "mixed-files", ... };
}

This only hard-closes 2+ provider files as mixed-files when there's no entry file present. When exactly one entry file rides alongside 2+ provider companion files, the PR instead falls through to entry-submission scope, and runSurfaceReview only catches the ambiguous companion shape later as a soft manual routing (companion-file-changes review), not a hard close.

This is a pre-existing asymmetry (it predates the registry surface-lane content-lane feature entirely — the guard and the orchestrator's companion loop are both unchanged by this PR) that was flagged as a real inconsistency during a recent adversarial code review of a separate surface-lane bugfix PR.

Decision: keep the current behavior (manual, not mixed-files/close) — this is a deliberate trade-off, not a bug — and document it clearly + lock it in with regression tests.

Rationale: with zero entry files, there is nothing else in the diff worth preserving if the provider-file shape is ambiguous, so a decisive close is correct. With one entry file present, the entry itself may still be a perfectly legitimate, valid submission — only its companion shape is unclear — so holding it for a human via manual is safer than throwing away potentially-good entry content with an outright close. Making the two shapes symmetric (i.e. also closing the entry+multi-provider case) would be a real behavior change with no clear product-safety win, and this PR does not make that call unilaterally.

What changed

  • classifyRegistryPrScope's doc comment now explicitly states the two mixed-files conditions and calls out the asymmetry by name (DELIBERATE ASYMMETRY), including how to tighten it later (entryFiles.length > 1 || providerFiles.length > 1, dropping the entryFiles.length === 0 qualifier) if a future spec's needs change this trade-off.
  • The guard itself is refactored into two named booleans (tooManyEntryFiles, tooManyProviderFilesWithNoEntry) so the asymmetry is visible in the code, not just the comment.
  • runSurfaceReview's companion-file loop gets a cross-referencing comment.
  • New regression tests in test/unit/content-lane-registry-logic.test.ts and test/unit/content-lane-orchestrator.test.ts lock in both sides: entry + 2 provider files → entry-submission scope → manual; 2+ provider files with no entry → mixed-filesclose.

No runtime behavior changes — this is documentation + naming + regression tests only.

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 follow-up from a recent adversarial code review, not tied to a filed issue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — both touched src/** files (registry-logic.ts, orchestrator.ts) are 100% line- and branch-covered.
  • 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, docs/tests-only change.
  • Public docs/changelogs are updated where needed. — N/A, internal code comments only, no user-facing docs affected.

Notes

  • This PR is based on main and is independent of #2586 (a separate, in-flight registry surface-lane bugfix PR touching the same two files for unrelated reasons). Expect a rebase on whichever of the two merges second.

@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:16:48 UTC

4 files · 1 AI reviewer · no blockers · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
This change documents an existing classifier/orchestrator asymmetry and refactors the mixed-files condition into named booleans without changing behavior. The added registry-logic tests cover both sides of the intended classifier split, and the orchestrator comment matches the post-change control flow where extra provider-looking files fall through to the companion-file manual path. I do not see a reachable break in the visible diff.

Nits — 4 non-blocking
  • nit: src/review/content-lane/registry-logic.ts:722 adds a long policy rationale directly in a function doc comment, which duplicates the test comments and can drift if this behavior changes again.
  • nit: test/unit/content-lane-registry-logic.test.ts:352 should assert the full classifier shape for the entry-plus-multiple-provider case, especially providerCompanionFile being null, since that is what makes the orchestrator's manual fallback reachable.
  • src/review/content-lane/registry-logic.ts:722 could keep the doc comment focused on the invariant and point to the orchestrator manual fallback instead of carrying the full product rationale in multiple places.
  • test/unit/content-lane-registry-logic.test.ts:352 should add expectations for isProvider and providerCompanionFile so the regression locks the exact handoff contract, not just the scope string.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
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:S; 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 ✅ Passing No configured blocker found.
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.
  • 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

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
…provider companions

classifyRegistryPrScope's mixed-files guard hard-closes 2+ provider
files only when there's no entry file present; the same "which
provider is the real companion?" ambiguity alongside a single
well-formed entry file instead falls through to entry-submission
scope, and runSurfaceReview routes it to a manual-review hold rather
than a close.

This asymmetry predates the surface-lane content-lane feature and was
flagged as a real inconsistency during a recent adversarial review, but
it's a deliberate trade-off, not a bug: with zero entry files there is
nothing else in the diff worth preserving, so a decisive close is
correct; with one entry file present, the entry itself may still be a
legitimate submission, so holding for a human is safer than throwing
away potentially-good content with an outright close.

Names the two mixed-files conditions explicitly, documents the
trade-off (and how to tighten it later if the trade-off changes), and
adds regression tests locking in both sides of the behavior.
@JSONbored
JSONbored force-pushed the fix/registry-companion-scope-asymmetry branch from b604b03 to 3aecdd4 Compare July 2, 2026 11:17
@JSONbored

Copy link
Copy Markdown
Owner Author

Rebased onto main now that #2586 merged (both PRs touched classifyRegistryPrScope). Reconciled the conflict, removed a test that became a duplicate of one #2586 already added, and re-ran the full local gate (test:ci, test:coverage — 100% line/branch on both touched files, npm audit) — all green.

@JSONbored
JSONbored merged commit e054ee8 into main Jul 2, 2026
9 checks passed
@JSONbored
JSONbored deleted the fix/registry-companion-scope-asymmetry branch July 2, 2026 11:22
@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 96.00%. Comparing base (335d9cd) to head (3aecdd4).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2587      +/-   ##
==========================================
+ Coverage   95.98%   96.00%   +0.01%     
==========================================
  Files         229      230       +1     
  Lines       25813    25916     +103     
  Branches     9390     9421      +31     
==========================================
+ Hits        24777    24880     +103     
  Misses        425      425              
  Partials      611      611              
Files with missing lines Coverage Δ
src/review/content-lane/orchestrator.ts 100.00% <ø> (ø)
src/review/content-lane/registry-logic.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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