Skip to content

test(review): close e2e gaps for review.tone and review.exclude_paths - #4145

Merged
JSONbored merged 1 commit into
mainfrom
test/review-controls-e2e-gaps-1681
Jul 8, 2026
Merged

test(review): close e2e gaps for review.tone and review.exclude_paths#4145
JSONbored merged 1 commit into
mainfrom
test/review-controls-e2e-gaps-1681

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Closes #1681

Summary

  • Closes the two confirmed test gaps from the review-controls audit (feat(review): test review controls end to end #1681): review.tone and
    review.exclude_paths/review.path_filters were previously only proven at the pure-function level
    (focus-manifest.test.ts's composeManifestReviewInstructions/filterReviewFilesForAi unit tests) or
    with empty-array fingerprint assertions in queue.test.ts — nothing drove the full
    webhook -> processJob -> runGittensoryAiReview pipeline to prove the resolved config actually
    reaches the AI reviewer.
  • Added threads review.tone from .gittensory.yml into the AI reviewer's system prompt (#2044): sets
    review.tone in a repo .gittensory.yml fixture (mirroring the changed_files_summary/effort_score
    tests' exact fixture pattern) and asserts the composed
    Review tone (maintainer voice brief — complements review.profile): ... section is present in the
    actual env.AI.run system message, not just in a pure-function return value.
  • Added genuinely removes a review.exclude_paths match from the AI reviewer's diff in advisory mode (#review-exclude-paths): sets a non-empty review.exclude_paths glob (every existing e2e
    assertion of this field only ever passes empty arrays) with two changed files, one matching the glob,
    and asserts the excluded file's path and patch content are genuinely absent from the AI reviewer's
    user prompt while the other file's diff is present. This is deliberately run in advisory mode:
    runAiReviewForAdvisory (src/queue/processors.ts) only calls filterReviewFilesForAi in advisory
    mode — block mode intentionally reviews the full diff so a filtered path can never be used to dodge an
    AI consensus blocker — so advisory mode is the only branch that actually exercises the filter.
  • Sanity-checked both new tests are non-vacuous: temporarily changed the fixture's glob to a
    non-matching pattern locally and confirmed the exclude-paths test then fails (the excluded filename
    legitimately appears in the prompt), before reverting to the real assertion.
  • No src/**/packages/** changes — this is a test/**-only PR (not Codecov-patch-measured), scoped
    narrowly to feat(review): test review controls end to end #1681's two named test gaps. No fixes for the docs-only fields below are included here —
    see the filed follow-ups.

Review-controls wiring matrix (#1681 deliverable)

Every review.* field parsed from .gittensory.yml, its resolution path, and whether it actually
reaches a live code path (not just parsing/validation). "e2e" = covered by a full
processJob/webhook-level test; "unit" = covered only at the pure-function level before this PR.

Field Status Resolved by Consumed at
review.tone Wired (e2e added this PR) resolveReviewPromptOverrides (src/signals/focus-manifest.ts:250) + composeManifestReviewInstructions (:236) src/queue/processors.ts:9378-9388 (reviewInstructions) -> buildSystemPrompt's repoInstructions append (src/services/ai-review.ts:879-891)
review.profile Wired (unit: ai-review.test.ts) resolveReviewPromptOverrides buildSystemPrompt's REVIEW_PROFILE_SUFFIX (src/services/ai-review.ts:830-837, 869-872)
review.security_focus Wired (unit: ai-review.test.ts) resolveReviewPromptOverrides buildSystemPrompt's SECURITY_FOCUS_SUFFIX (src/services/ai-review.ts:843-844, 873)
review.instructions Wired resolveReviewPromptOverrides folded into reviewInstructions alongside tone (src/queue/processors.ts:9378-9388)
review.path_instructions Wired resolveReviewPromptOverrides resolveReviewPathInstructions (src/queue/processors.ts:7371-7374, 9459-9462) -> buildSystemPrompt's pathGuidance
review.exclude_paths Wired, advisory-only (e2e added this PR) resolveReviewPromptOverrides filterReviewFilesForAi (src/signals/focus-manifest.ts:379-385, 408-414), applied only when aiReviewMode !== "block" (src/queue/processors.ts:7201-7207)
review.path_filters Wired, advisory-only (e2e added this PR) resolveReviewPromptOverrides applyReviewPathFilters (src/signals/focus-manifest.ts:389-405), same gate as exclude_paths above
review.inline_comments Wired resolveReviewPromptOverrides shouldRequestInlineFindings (src/review/inline-comments.ts:43), src/queue/processors.ts:9358-9362
review.suggestions Wired resolveReviewPromptOverrides shouldRenderSuggestions (src/review/inline-comments.ts:60), ANDed with inline comments
review.finding_categories Wired resolveReviewPromptOverrides shouldRenderFindingCategories (src/review/inline-comments.ts:70), ANDed with inline comments
review.changed_files_summary Wired (e2e: existing queue.test.ts test) resolveReviewPromptOverrides src/queue/processors.ts:9142, 10515-10522
review.effort_score Wired (e2e: existing queue.test.ts test) resolveReviewPromptOverrides src/queue/processors.ts:9143, 10530
review.max_findings Wired (e2e: existing queue.test.ts test #2049) resolveReviewPromptOverrides src/queue/processors.ts:10554
review.comment_verbosity Wired resolveReviewPromptOverrides src/queue/processors.ts:10555
review.min_finding_severity Wired resolveReviewPromptOverrides src/queue/processors.ts:9144, 10616
review.impact_map Wired resolveReviewPromptOverrides shouldComputeImpactMap (src/review/impact-map-wire.ts:26), src/queue/processors.ts:7257, 9414
review.culture_profile Wired resolveReviewPromptOverrides ANDed with GITTENSORY_REVIEW_CULTURE_PROFILE, src/queue/processors.ts:7278-7280, 9410
review.ai_model Wired (e2e: existing queue.test.ts test) resolveReviewSelfHostAiModel (src/signals/focus-manifest.ts:306) correlation fields, src/queue/processors.ts:7357-7364
review.memory Wired resolveReviewMemoryManifestToggle (src/signals/focus-manifest.ts:281) shouldApplyReviewMemory, src/queue/processors.ts:9150, 11043
review.pre_merge_checks Wired resolveReviewPreMergeChecks (src/signals/focus-manifest.ts:288) src/queue/processors.ts:6573, 9023
review.auto_review Wired resolveReviewAutoReviewConfig (src/signals/focus-manifest.ts:298) src/queue/processors.ts:8293
review.enrichment (analyzer toggles) Wired resolveRepoEnrichmentToggles (src/signals/focus-manifest.ts:326) src/queue/processors.ts:7315
review.visual Wired resolveReviewVisualConfig (src/signals/focus-manifest.ts:314) src/queue/processors.ts:6973

Docs-only / phantom fields (parsed, never consumed) — follow-ups filed, not fixed in this PR:

Field Status Evidence
review.labeling_rules Docs-only Parsed + validated (packages/gittensory-engine/src/focus-manifest.ts:2002, 2251), but grep -rn "LabelingRule" src/ outside the type re-export in focus-manifest.ts returns nothing — zero real consumer anywhere. Follow-up: #4146
review.auto_merge_summary Docs-only The field itself has zero consumer in src/. Its render primitive buildAutoMergeSummaryCollapsible (src/review/unified-comment.ts:229, #2051) has its own pure-function unit test but is never called from unified-comment-bridge.ts or processors.ts. Follow-up: #4147
review.linked_issue_satisfaction Docs-only (distinct from gate.linkedIssueSatisfaction) src/types.ts:761-769 explicitly documents linkedIssueSatisfactionGateMode (the real, DB-backed field .gittensory.yml gate.linkedIssueSatisfaction overrides) as distinct from this review.* manifest field. Every real consumer in src/queue/processors.ts (e.g. :6638, 7949, 8978) reads settings.linkedIssueSatisfactionGateMode, never manifest.review.linkedIssueSatisfaction — confirmed still true after the recent #4069 merge, which deepened the gate.* field's wiring but never touched this one. Setting review.linked_issue_satisfaction in .gittensory.yml today does nothing. Follow-up: #4149

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes — it only adds two tests to test/unit/queue.test.ts.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (see "Closes feat(review): test review controls end to end #1681" at the top of this description).

Validation

  • git diff --check
  • npm run actionlint (no workflow files changed)
  • npm run typecheck
  • npm run test:coverage locally, unsharded — full suite green: 582 files / 11899 tests passed, 0 failed, 0 uncovered patch lines (this PR touches only test/**, which Codecov does not measure).
  • npm run test:workers (no test/workers/** files touched by this change)
  • npm run build:mcp (no MCP package changes)
  • npm run test:mcp-pack (no MCP package changes)
  • npm run ui:openapi:check (no API/schema changes)
  • npm run ui:lint (no apps/gittensory-ui/** changes)
  • npm run ui:typecheck (no apps/gittensory-ui/** changes)
  • npm run ui:build (no apps/gittensory-ui/** changes)
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New behavior (the two new e2e tests) exercises both the "config set, matching" and (via a temporary local sanity check, since reverted) "config absent/non-matching" branches — see Summary.

If any required check was skipped, explain why:

  • This PR only adds tests to test/unit/queue.test.ts; it does not touch .github/workflows/**, test/workers/**, any MCP package, any API/schema/OpenAPI surface, or apps/gittensory-ui/**, so the corresponding checks above are not applicable. npm run typecheck and the full unsharded npm run test:coverage both ran clean against the actual change.

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. (N/A — no API/OpenAPI/MCP changes.)
  • 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. (N/A — no UI changes; see below.)
  • Public docs/changelogs are updated where needed. (N/A — no docs/changelog changes; CHANGELOG.md is not touched.)

UI Evidence

N/A — this PR only adds backend test coverage (test/unit/queue.test.ts); there is no visible UI, frontend, or docs change.

Notes

…#1681)

review.tone and review.exclude_paths/path_filters were only exercised at
the pure-function level (focus-manifest.test.ts) or with empty-array
fingerprint assertions in queue.test.ts. Add two full webhook -> processJob
e2e tests: one confirms the composed tone section actually reaches
env.AI.run's system prompt, the other confirms a non-empty exclude_paths
glob genuinely removes a matching file's path and patch content from the
AI reviewer's diff in advisory mode (block mode intentionally reviews the
full diff, so the test exercises the branch where filtering actually
applies).
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (acddda9) to head (c029453).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4145   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files         385      385           
  Lines       36212    36212           
  Branches    13266    13266           
=======================================
  Hits        33937    33937           
  Misses       1618     1618           
  Partials      657      657           
🚀 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 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-08 08:03:03 UTC

1 file · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds two e2e regression tests to test/unit/queue.test.ts that drive the full webhook → processJob → runGittensoryAiReview pipeline to confirm review.tone reaches env.AI.run's system message and review.exclude_paths genuinely removes a matching file's path/patch from the AI reviewer's user prompt in advisory mode. Both tests are additive-only (no src/ changes), mirror the existing fixture/mock conventions already used elsewhere in the file (changed_files_summary/effort_score tests), and each asserts a specific string is present or absent in the captured AI.run message content rather than relying on a pure-function unit assertion, which is exactly what the linked issue #1681 asked for. The PR description documents a sanity check (flipping the glob to a non-matching pattern to confirm the exclude-paths test actually fails), which is good practice for proving the test is non-vacuous.

Nits — 6 non-blocking
  • The external scan flags 'generic_secret_assignment' at queue.test.ts:18286 and :18369 — these look like false positives on `GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem()` test fixture setup rather than real secrets, but worth a quick manual confirmation since the diff wasn't fully visible to double-check.
  • In the review.exclude_paths test, `aiReviewAllAuthors: true` is used to bypass the miner-confirmation gate — confirm this field is still the documented, currently-supported opt-in in src/queue/processors.ts (not deprecated) since it's asserted rather than shown in the diff.
  • Both new tests duplicate a fairly large amount of webhook/fetch-mock boilerplate already present in this file; consider whether a shared helper for the common fetch-mock routes (access_tokens, pulls/7, check-runs, status, issues) would reduce repetition, though this is purely a style/DRY concern given the existing file already has this pattern repeated many times.
  • Consider adding a one-line assertion in the review.tone test that the tone section only appears once (not duplicated) if composeManifestReviewInstructions concatenates multiple config sections, to guard against future double-injection regressions.
  • For the exclude_paths test, an additional assertion that the excluded file's patch line count didn't leak into any diff-size/token-budget calculation would strengthen the 'genuinely absent' claim beyond string containment.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1681
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 43 merged, 496 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 52 PR(s), 496 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Partially addressed
The PR adds solid e2e tests for exactly two options (review.tone and review.exclude_paths), but the issue's scope is a full audit and test pass covering inline comments, profile selection, path instructions, pre-merge checks, a wiring matrix, follow-up issues for unwired options, and self-host enablement docs — none of which appear in this diff. It's real, verifiable progress on a narrow slice of

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: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 52 PR(s), 496 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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 manual-review Gittensor contributor context gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. and removed gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 8, 2026
@JSONbored
JSONbored merged commit 107f815 into main Jul 8, 2026
10 checks passed
@JSONbored
JSONbored deleted the test/review-controls-e2e-gaps-1681 branch July 8, 2026 08:03
@loopover-orb loopover-orb Bot added gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. and removed gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. labels Jul 8, 2026
@JSONbored JSONbored added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. and removed gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

feat(review): test review controls end to end

1 participant