Skip to content

refactor(signals): extract gate:-to-settings override mapping into its own function - #3373

Merged
JSONbored merged 1 commit into
mainfrom
refactor/extract-gate-settings-override-mapping
Jul 5, 2026
Merged

refactor(signals): extract gate:-to-settings override mapping into its own function#3373
JSONbored merged 1 commit into
mainfrom
refactor/extract-gate-settings-override-mapping

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • resolveEffectiveSettings (src/signals/focus-manifest.ts) inlined a ~40-line chain of individual if (gate.field !== null) effective.otherFieldName = gate.field; mappings, mixed together with the settings spread, the sparse typeLabels/linkedIssueLabelPropagation/linkedIssueHardRules merges, and the post-merge normalization safety nets (the requireLinkedIssue-implies-block rule, the qualityGateMode: "block" downgrade). Extracted the gate-mapping chain into its own named function, applyGateConfigOverrides(effective, gate), so resolveEffectiveSettings itself now reads as a short, linear sequence: spread settings → apply sparse overrides → apply gate overrides → normalize.
  • Zero behavior change. This is pure code-quality cleanup — no bug, no config-resolution difference. FocusManifestGateConfig's shape is completely untouched, so every existing direct consumer of manifest.gate.* (src/rules/predicted-gate.ts — the engine behind the predict_gate MCP tool contributors run pre-push — and src/mcp/server.ts) needed zero changes. gate: still wins over an overlapping settings: value for the same field (the extracted function still runs after the settings spread), matching the documented precedence in the self-hosting-configuration docs.
  • Why this scope, not a bigger one: the original idea was to eliminate the mapping entirely by having the parser emit settings-shaped keys directly. Investigated that further and found it would require restructuring FocusManifestGateConfig's field names, which are read directly (by the OLD names) in predicted-gate.ts (~15 references) and mcp/server.ts — i.e. it would mean rewriting the gate-prediction engine itself for a purely non-functional cleanup. Not worth that risk for no behavior gain, so this PR does the safe, contained version: same names, same shape, same precedence, just a named extraction.
  • No issue filed — pure internal refactor, no user-facing or config-schema change.

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. (Single file, single extraction.)
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run typecheck (clean)
  • npx vitest run test/unit/focus-manifest.test.ts test/unit/focus-manifest-loader.test.ts test/unit/selftune-readback.test.ts test/unit/repository-settings-enforcement.test.ts (300/300 passing, unchanged)
  • npx vitest run test/unit/predicted-gate.test.ts (44/44 passing, unchanged — confirms zero impact on the untouched gate-prediction consumer)
  • npx vitest run test/unit/queue.test.ts (483/483 passing — full end-to-end review-pipeline regression check, since this function sits on the hot settings-resolution path)
  • npm run test:workers / npm run build:mcp / npm run test:mcp-pack / npm run ui:openapi:check / npm run ui:build — not run individually this PR; no worker/MCP/OpenAPI/UI surface touched (this PR moves existing lines within one src/ file; FocusManifestGateConfig's public shape and every serializer/consumer are unchanged). Ran the full npm run test:ci gate once already this session (PR fix(settings): authorize circuit-breaker hold labels via merge/close autonomy #3363) with no relevant failures.
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — N/A here: no new branches or logic were introduced, only relocated; the existing 255-case focus-manifest.test.ts suite already exercises every line of the extracted function via its existing calls into resolveEffectiveSettings, and continues to pass unchanged.

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.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no API/OpenAPI/MCP surface changed; predicted-gate.ts/mcp/server.ts consumers verified unaffected (see Validation).
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI change.
  • Visible UI changes include a UI Evidence section below. — N/A, no visible UI change.
  • Public docs/changelogs are updated where needed. — N/A, no behavior or schema change to document.

Notes

…s own function

resolveEffectiveSettings inlined a ~40-line chain of field-by-field
gate.* -> RepositorySettings mappings, mixed in with the settings
spread, the sparse typeLabels/linkedIssue*/blacklist merges, and the
post-merge normalization safety nets. Split it into
applyGateConfigOverrides, a single named place a gate:-related field
maps onto its settings counterpart, so resolveEffectiveSettings itself
reads as: spread settings, apply sparse overrides, apply gate
overrides, normalize. No behavior change -- FocusManifestGateConfig's
shape and every existing consumer (predicted-gate.ts, mcp/server.ts)
are untouched, and gate: still wins over an overlapping settings:
value for the same field, matching documented precedence.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.89%. Comparing base (113da08) to head (6560fef).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3373   +/-   ##
=======================================
  Coverage   93.89%   93.89%           
=======================================
  Files         283      283           
  Lines       30573    30573           
  Branches    11138    11138           
=======================================
  Hits        28705    28705           
  Misses       1211     1211           
  Partials      657      657           
Files with missing lines Coverage Δ
src/signals/focus-manifest.ts 98.56% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-05 05:31:53 UTC

1 file · 1 AI reviewer · 1 blocker · readiness 100/100 · CI green · unknown

⏸️ Suggested Action - Manual Review

  • Maintainer test expectations unmet — Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.

Review summary
This PR cleanly extracts the existing `gate:` to `RepositorySettings` mapping into `applyGateConfigOverrides` while preserving the call order inside `resolveEffectiveSettings`, so `settings:` is still spread first and `gate:` still wins before the linked-issue/readiness normalization runs. I do not see a behavioral wiring change in the visible diff: the same null-gated field mappings were moved intact, including the `checkMode` vs `enabled` precedence and the AI-review clamp comment remaining at the projection layer. The only review concern is coverage/process: this touches branch-heavy config-resolution code without any changed characterization test evidence, even though the stated goal is zero behavior change.

Nits — 4 non-blocking
  • nit: `src/signals/focus-manifest.ts:1749` adds a new branch-heavy helper in measured `src/**` with no test-path changes; for a zero-behavior refactor, I would prefer a focused characterization test that exercises `gate.checkMode`, the `gate.enabled` fallback, and one sparse null field to prove the extracted call still runs after the `settings:` spread.
  • Add or update an existing `resolveEffectiveSettings` characterization test around `src/signals/focus-manifest.ts:1863` to assert that `settings.reviewCheckMode` is overridden by `gate.checkMode`, and by `gate.enabled` only when `checkMode` is absent.
  • Consider keeping the helper comment at `src/signals/focus-manifest.ts:1742` shorter and moving the precedence detail into the existing `resolveEffectiveSettings` doc, since the ordering guarantee is enforced by the call site rather than the helper itself.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

Concerns raised — review before merging

  • Maintainer test expectations unmet — Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.
Signal Result Evidence
Code review ❌ 1 blocker 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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 46 merged, 416 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 56 PR(s), 416 issue(s).
Gate result ❌ Blocking Repo-configured hard 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: 56 PR(s), 416 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

@JSONbored
JSONbored merged commit 8334b1b into main Jul 5, 2026
10 checks passed
@JSONbored
JSONbored deleted the refactor/extract-gate-settings-override-mapping branch July 5, 2026 05:34
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