Skip to content

refactor(queue): extract manifest-policy gate from maybePublishPrPublicSurface - #4728

Merged
JSONbored merged 1 commit into
mainfrom
refactor/processors-publish-surface-4607-3
Jul 10, 2026
Merged

refactor(queue): extract manifest-policy gate from maybePublishPrPublicSurface#4728
JSONbored merged 1 commit into
mainfrom
refactor/processors-publish-surface-4607-3

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Part of Break up processors.ts mega-functions #4607 (the processors.ts mega-function breakup). This PR does one slice of the
    maybePublishPrPublicSurface portion of that issue — the largest and riskiest of the three functions,
    explicitly called out as needing "its own careful multi-part approach given the size" (2,600+ lines, 17%
    of the file, up to 10 levels of nesting). runAgentMaintenancePlanAndExecute (refactor(queue): extract plan-input builder from runAgentMaintenancePlanAndExecute #4688) and
    processGitHubWebhook (refactor(queue): split processGitHubWebhook into per-event handlers #4695) already shipped as the first two parts of this issue.
  • After reading the entire function start to finish, I picked the focus-manifest policy gate block —
    one of the issue's own named extraction targets ("manifest-policy evaluation") — as the safest first
    slice, over the other candidates (dry-run-chokepoint / unified-comment feature resolution / AI-vision BYOK
    gate are already single-call sites into existing helpers with nothing left to extract; the unified-comment
    render block and the review-memory suppression block are both more deeply nested, inside two stacked
    conditionals after the function's own try/catch). The manifest-policy block instead sits at the same
    shallow, top-level sequential position in the gate-evaluation try block as its already-extracted
    siblings maybeAddSecretLeakFinding / maybeAddLockfileTamperFinding (same file), has zero early
    returns, and every one of its own locals is provably block-scoped — the surrounding code even has its own
    comment confirming e2eTestGenAvailable is "block-scoped to the manifestPolicyGateMode branch above...
    and out of scope here," independently re-resolving it a few hundred lines later.
  • Extracted the whole if (settings.manifestPolicyGateMode !== "off") { ... } block (findings computation
    and the E2E test-generation auto-trigger it gates, both tightly coupled to the same
    policyFindings/e2eTestGenAvailable values) into a new, named maybeApplyManifestPolicyGate helper,
    colocated directly above maybePublishPrPublicSurface, called from the exact spot the inline block used
    to sit.
  • Pure code motion — zero behavior change. Kept the original if (cond) { ... } shape rather than
    inverting it into an early-return guard clause, specifically so a mechanical byte-diff could verify
    fidelity without also having to reason about a control-flow rewrite. Wrote a small Node script
    (not committed — a one-off local check, like the sibling PRs used) that: cuts the original inline block
    from origin/main by exact line range, cuts the new function's body by exact line range, dedents both,
    and reverse-transforms the new text (args.XX, then collapses the X: X object-shorthand the
    rename necessarily expanded back to bare X, e.g. repoFullName: args.repoFullNamerepoFullName) —
    confirmed byte-identical to the original. Every comment is preserved verbatim and in the same order.
  • Net effect: maybePublishPrPublicSurface shrinks by ~77 lines (89 inline → a 12-line call), with the
    logic itself now independently named and one step closer to independently testable.

Coverage follow-up in the same commit (the documented #4607 hard lesson)

Per PR #4695's own retro (a relocated-but-previously-untested line still counts as "new" against Codecov's
diff-based patch gate — line coverage alone also isn't enough, since Codecov measures branches too), I
cross-referenced coverage/coverage-final.json's statementMap/s and branchMap/b (not just the
terminal summary table) against this diff's exact added-line ranges after the raw extraction, before
declaring it done. Found 3 gaps (1 statement + 2 branches, resolving to the same 2 underlying lines). For
each, I confirmed NEW-vs-PRE-EXISTING empirically — not by assumption — by running the identical
vitest --coverage command against an unmodified origin/main checkout (a throwaway git worktree) and
diffing hit-count arrays at the original line numbers:

  • gateFiles ?? [] (branch): hits=[20,0] on origin/main at line 10088, hits=[20,0] post-extraction —
    identical, confirmed pre-existing. It's also provably unreachable on the only real call path: the
    sole caller only invokes this function when manifestPolicyGateMode !== "off", and that exact same
    condition is what makes the caller's own gateFiles local resolve via getReviewFiles() (never null)
    immediately beforehand — the | null on the type exists only for TypeScript soundness on the caller's
    let-then-conditionally-assigned local, not because of a reachable runtime null. Marked
    /* v8 ignore next -- see the comment above */, matching this exact file's own established convention
    (e.g. the neighboring typeLabelsEnabled ?? true).
  • if (!policyCodes.has(finding.code)) continue; (statement + branch): hits=[0,15] on origin/main at
    line 10126, identical post-extraction — also pre-existing, but genuinely reachable and testable:
    buildFocusManifestGuidance can produce finding codes outside the three enforceable ones (e.g.
    manifest_off_focus, manifest_preferred_path, manifest_missing_preferred_label), which this filter is
    specifically there to drop before they reach the advisory. Added one real test
    (test/unit/queue.test.ts) that configures wantedPaths so an out-of-focus changed file produces a
    manifest_off_focus finding alongside a manifest_missing_tests finding from the same pass, and asserts
    the published gate output contains the enforceable one's text but not the filtered one's — exercising
    both sides of the branch with a real functional assertion, not just a coverage-padding call.

Re-ran the full cross-reference after both fixes: zero gaps remaining, at both line and branch level,
within this diff's added-line ranges.

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.

Note on the issue link: this is Part of #4607, not Closes/Fixes — the issue explicitly covers the rest
of maybePublishPrPublicSurface (this is one slice of one of three parts) as further sequential PRs. See
the issue body: "this is expected to land as multiple sequential PRs."

Validation

  • git diff --check
  • npm run actionlint — not run; no .github/workflows/** files touched.
  • npm run typecheck — clean: before the coverage follow-up, after it, and again immediately after
    rebasing onto fresh origin/main.
  • npm run test:coverage — not run as the literal full-suite command; a scoped coverage run (below)
    proved the diff itself is fully exercised at both line and branch level, matching the precedent set
    by the two sibling Break up processors.ts mega-functions #4607 extraction PRs.
  • npm run test:workers — not run; no test/workers/**-relevant code touched.
  • npm run build:mcp / npm run test:mcp-pack — not run; no MCP package changes.
  • npm run ui:openapi:check / npm run ui:lint / npm run ui:typecheck / npm run ui:build — not
    run; no apps/gittensory-ui/** or API/schema changes.
  • npm audit --audit-level=moderate — not run; no dependency changes.
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — the extraction itself has no new behavior; one real regression-style test was added for the pre-existing (relocated) manifest_off_focus filter branch (see Coverage follow-up above).

If any required check was skipped, explain why:

  • This is a single-file-plus-its-test-file, behavior-preserving refactor of src/queue/processors.ts with
    no UI, MCP, workers, schema, OpenAPI, workflow, or dependency surface touched, so those gates are left to
    CI rather than duplicated locally (most path-filter out for this diff anyway). The checks that matter for
    this change were run directly, in full, in the foreground, both before and after rebasing:
    • npm run typecheck: clean every time.
    • npx vitest run test/unit/queue.test.ts: 810/810 tests passed (809 pre-existing, unmodified, plus
      1 new) — the primary suite exercising maybePublishPrPublicSurface end-to-end via webhook processing,
      auto-action convergence, and the manifest-policy-gate-specific scenarios this PR's function now serves.
    • Coverage: npx vitest run test/unit/queue.test.ts --coverage --coverage.include='src/queue/processors.ts' --coverage.reporter=json,
      parsed coverage/coverage-final.json directly (statementMap/s and branchMap/b, not the mixed
      terminal summary) and cross-referenced against the diff's exact added-line ranges from
      git diff origin/main --unified=0. Zero uncovered statements, zero partially-covered branches, within
      those ranges — full detail above.
    • Rebase: git fetch origin && git rebase origin/main immediately before pushing completed with no
      conflicts
      (none of the 6 commits that had landed on main since this branch was created touched
      src/queue/processors.ts). Re-ran typecheck, the full queue.test.ts suite, and the coverage
      cross-reference again after rebasing — identical clean results.

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/cookie/CORS/session code touched (pure structural refactor).
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no API/OpenAPI/MCP surface touched.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI changes.
  • Visible UI changes include a UI Evidence section below... — N/A, no visible/UI changes (backend-only refactor).
  • Public docs/changelogs are updated where needed... — N/A, no doc-affecting behavior change; CHANGELOG.md intentionally not touched.

UI Evidence

Not applicable — this PR has no visible/UI/frontend/docs surface; it is a backend-only, behavior-preserving
extraction inside src/queue/processors.ts.

Notes

  • This is a maintainer/owner PR (issue Break up processors.ts mega-functions #4607 is labeled maintainer-only).
  • Deliberately scoped to ONE block, per the issue's own instruction that maybePublishPrPublicSurface
    "needs its own careful multi-part approach given the size" — did not attempt the unified-comment render
    block, the review-memory suppression block, the AI-review cache/dispatch closure, or any of the other
    large sections in the same pass, to keep this PR's own risk surface reviewable and its byte-diff
    verification tractable.
  • Follow-ups (separate PRs, per the issue): the remaining named blocks in maybePublishPrPublicSurface
    (unified-comment rendering, review-memory/publish-suppression, the AI-review dispatch closure, and
    whatever else the next read of the function surfaces) — each is a substantially larger and more
    deeply-nested/mutable-state-coupled unit than this one, and will need their own individually-scoped PRs
    and their own byte-diff + coverage verification passes.

…licSurface (#4607)

Pulls the focus-manifest policy evaluation and E2E test-generation
auto-trigger block out of maybePublishPrPublicSurface into a named
maybeApplyManifestPolicyGate helper, matching the extraction pattern
from parts 1 and 2 of the same effort (#4607). Pure code motion,
verified byte-identical against the original inline block via a
mechanical dedent+rename+diff script.

Also resolves the two coverage gaps the relocation surfaced against
this diff (both pre-existing on main, confirmed by comparing coverage
runs before and after): a provably-unreachable gateFiles fallback
gets a v8-ignore matching this file's own convention, and the
manifest-finding filter's skip branch gets a real regression test.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.22%. Comparing base (bfc34cd) to head (2a8f9a5).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4728   +/-   ##
=======================================
  Coverage   94.21%   94.22%           
=======================================
  Files         439      439           
  Lines       38704    38704           
  Branches    14101    14100    -1     
=======================================
+ Hits        36466    36468    +2     
  Misses       1576     1576           
+ Partials      662      660    -2     
Files with missing lines Coverage Δ
src/queue/processors.ts 95.76% <100.00%> (+0.05%) ⬆️
🚀 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 10, 2026
@loopover-orb

loopover-orb Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-10 21:26:01 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a clean, well-documented pure code-motion refactor that extracts the focus-manifest policy gate block (findings computation + E2E test-gen auto-trigger) from the 2,600+ line maybePublishPrPublicSurface into a new maybeApplyManifestPolicyGate helper, called at the same sequential position inside the original try block. Comparing the extracted body against the removed original line-by-line, every condition, comment, and variable reference is preserved verbatim (locals renamed to args.* only), the guard `manifestPolicyGateMode !== "off"` moved inside the new function correctly replaces the removed inline check, and mutation of `advisory.findings` still propagates by reference. The PR adds one genuine integration-style test (via processJob, not a fabricated payload) covering that manifest_off_focus findings are filtered while manifest_missing_tests is published, and is explicitly scoped to issue #4607 (a maintainer-tracked decomposition effort with two prior sibling PRs already merged).

Nits — 5 non-blocking
  • src/queue/processors.ts: the new maybeApplyManifestPolicyGate takes a 9-field args bag; consider whether a narrower interface improves readability given this is meant to be a template for further processors.ts extractions.
  • test/unit/queue.test.ts: the new test only exercises manifestPolicyGateMode:"block" with a non-enforceable+enforceable finding mix — confirm the `manifestPolicyGateMode: "off"` no-op branch of the extracted function still has coverage from pre-existing tests, since this diff doesn't add one.
  • src/queue/processors.ts:~9219: the `/* v8 ignore next */` on `args.gateFiles ?? []` asserts the fallback is unreachable on the webhook path; worth a one-line pointer to the pre-existing test/assertion that established this so a future caller change doesn't silently reintroduce a reachable null.
  • Consider linking directly to the specific Break up processors.ts mega-functions #4607 checklist item/comment that names "manifest-policy evaluation" as an extraction target, for reviewer convenience.
  • If more slices of maybePublishPrPublicSurface follow the same args-bag pattern, factor a shared context type once 2-3 helpers exist to avoid duplicated field lists.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 285 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 285 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 is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 285 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.
  • 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 the manual-review Gittensor contributor context label Jul 10, 2026
@JSONbored
JSONbored merged commit 12147aa into main Jul 10, 2026
11 checks passed
@JSONbored
JSONbored deleted the refactor/processors-publish-surface-4607-3 branch July 10, 2026 21:26
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant