Skip to content

feat(review): migrate grounding onto the per-repo feature-activation resolver#4117

Merged
JSONbored merged 1 commit into
mainfrom
feat/grounding-config-4100
Jul 8, 2026
Merged

feat(review): migrate grounding onto the per-repo feature-activation resolver#4117
JSONbored merged 1 commit into
mainfrom
feat/grounding-config-4100

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • isGroundingEnabled(env) && isConvergenceRepoAllowed(env, repo) (raw, coarse allowlist-only) is now resolveConvergedFeature(env, manifest, "grounding", repo) at both real call sites in src/queue/processors.ts — the same precedence every other converged feature uses (env kill-switch → per-repo .gittensory.yml features.grounding override → allowlist default).
  • Added "grounding" to CONVERGED_FEATURE_KEYS (packages/gittensory-engine/src/focus-manifest.ts) and FEATURE_GLOBAL_FLAG (src/review/feature-activation.ts).
  • Investigated the original "coupled to the merge/close DISPOSITION path" blocker documented in feature-activation.ts's own header comment: it referred to the now-removed AI CI-refutation path — grounding-wire.ts's aiCiRefutationActive is a vestigial historical-compatibility helper with zero real callers (confirmed via repo-wide grep). Grounding today only shapes reviewer PROMPT content (CI status + full file context), the same shape as rag/reputation — safe to migrate.
  • Widened the manifest-load condition (isReputationEnabled(env) || isRagEnabled(env)) to also include || isGroundingEnabled(env), so grounding-only repos still get the manifest loaded for their own override to take effect.
  • Part of Epic: config-as-code should fully control every review feature, not just restrict it #4092 (config-as-code epic). Closes Add config-as-code coverage for the grounding feature #4100.

Scope

Validation

  • git diff --check
  • npm run typecheck — clean.
  • npm run build --workspace @jsonbored/gittensory-engine (required after editing the shared package).
  • Targeted tests: test/unit/feature-activation.test.ts, test/unit/focus-manifest.test.ts, test/unit/config-templates.test.ts, test/unit/signals-coverage.test.ts, test/unit/grounding-wiring.test.ts — all green (626 tests).
  • Full test/unit/queue.test.ts integration suite — green (697 tests).
  • Scoped coverage check on the 3 changed source files — src/review/feature-activation.ts at 100% line/branch; the new processors.ts lines (the widened || condition, the new groundingActive resolution, and its two consumption sites) confirmed via lcov BRDA records — all branches hit.
  • npm audit --audit-level=moderate — 0 vulnerabilities.

Safety

  • No secrets/wallets/hotkeys/trust-scores exposed.
  • N/A — no auth/session/CORS changes.
  • N/A — no API/OpenAPI/MCP surface touched.
  • N/A — no UI changes.

…resolver

Adds "grounding" to CONVERGED_FEATURE_KEYS and FEATURE_GLOBAL_FLAG, and
migrates both real call sites in processors.ts from the raw
isGroundingEnabled(env) && isConvergenceRepoAllowed(env, repo) check to
resolveConvergedFeature, so an operator can now turn grounding on/off
per-repo through .gittensory.yml alone. The original "coupled to the
merge/close disposition path" blocker was the removed AI CI-refutation
path (grounding-wire.ts's aiCiRefutationActive is now a vestigial
compatibility helper with zero real callers) -- grounding today only
shapes reviewer prompt content, same shape as rag/reputation.

Closes #4100.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui f85de86 Commit Preview URL

Branch Preview URL
Jul 08 2026, 01:41 AM

@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.64%. Comparing base (92dfae1) to head (f85de86).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4117   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files         384      384           
  Lines       35856    35856           
  Branches    13159    13158    -1     
=======================================
  Hits        33579    33579           
  Misses       1618     1618           
  Partials      659      659           
Files with missing lines Coverage Δ
packages/gittensory-engine/src/focus-manifest.ts 99.15% <100.00%> (ø)
src/queue/processors.ts 95.15% <100.00%> (ø)
src/review/feature-activation.ts 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 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-08 01:51:57 UTC

7 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • AI reviewers agree on a likely critical defect: src/queue/processors.ts (maybePublishPrPublicSurface, ~line 9283): `dynamicReviewFeatures.grounding` now reads its override from `reviewManifest`, but unlike `featureManifest` in `runAiReviewForAdvisory` (whose load condition was explicitly widened with `|| isGroundingEnabled(env)`), the diff shows no change to whatever condition loads `reviewManifest` — if that condition still only checks rag/reputation (matching the pre-PR pattern this mirrors), a grounding-only repo gets a null manifest here and its `.gittensory.yml features.grounding` override silently falls back to the allowlist default at this cache-bypass site even though the real review path honors it, producing per-call-site behavioral drift. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This migrates grounding's per-PR activation from a raw `isGroundingEnabled(env) && convergedRepoAllowed` check onto the same `resolveConvergedFeature` precedence (env kill-switch → per-repo `.gittensory.yml` override → allowlist default) already used by rag/reputation, and updates the schema (`CONVERGED_FEATURE_KEYS`, `FEATURE_GLOBAL_FLAG`, `EMPTY_FEATURES_CONFIG`) plus doc examples consistently. The `runAiReviewForAdvisory` call site is fully and correctly wired: the `featureManifest` load condition is widened to include `isGroundingEnabled(env)`, and `groundingActive` replaces the old raw check. The second call site in `maybePublishPrPublicSurface` swaps in `resolveConvergedFeature(env, reviewManifest, "grounding", repoFullName)` but the diff never shows that `reviewManifest`'s own load-gating condition being widened for grounding, which is the exact 'wired at only one site' gap this repo's review guidance calls out.

Blockers

  • src/queue/processors.ts (maybePublishPrPublicSurface, ~line 9283): `dynamicReviewFeatures.grounding` now reads its override from `reviewManifest`, but unlike `featureManifest` in `runAiReviewForAdvisory` (whose load condition was explicitly widened with `|| isGroundingEnabled(env)`), the diff shows no change to whatever condition loads `reviewManifest` — if that condition still only checks rag/reputation (matching the pre-PR pattern this mirrors), a grounding-only repo gets a null manifest here and its `.gittensory.yml features.grounding` override silently falls back to the allowlist default at this cache-bypass site even though the real review path honors it, producing per-call-site behavioral drift.
Nits — 7 non-blocking
  • grounding-wiring.test.ts is listed as passing but isn't touched in this diff — confirm it actually exercises the new manifest-based override precedence for grounding (both true and false override arms) rather than only the old raw-flag path.
  • The featureManifest-load-gating logic (`isReputationEnabled || isRagEnabled || isGroundingEnabled(env)`) is now duplicated conceptually across two call sites in processors.ts; consider a small shared helper so a future converged feature can't repeat this same partial-wiring risk.
  • Verify no OpenAPI/config-schema generation artifact depends on `FocusManifestFeaturesConfig`/`CONVERGED_FEATURE_KEYS` and needs regeneration for the new `grounding` field — not shown in this diff.
  • Add or point to a test that sets `features.grounding: true` in the manifest with `isRagEnabled`/`isReputationEnabled` both false and asserts `maybePublishPrPublicSurface`'s cache-bypass path picks it up — this is the concrete case that would catch the load-gating gap above.
  • If `reviewManifest`'s load condition is defined once and shared with `featureManifest`, say so explicitly in the PR description (or add a comment at the `reviewManifest` load site) since the current comments only describe the widening at the `runAiReviewForAdvisory` site.
  • 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.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

Concerns raised — review before merging

  • src/queue/processors.ts (maybePublishPrPublicSurface, ~line 9283): `dynamicReviewFeatures.grounding` now reads its override from `reviewManifest`, but unlike `featureManifest` in `runAiReviewForAdvisory` (whose load condition was explicitly widened with `|| isGroundingEnabled(env)`), the diff shows no change to whatever condition loads `reviewManifest` — if that condition still only checks rag/reputation (matching the pre-PR pattern this mirrors), a grounding-only repo gets a null manifest here and its `.gittensory.yml features.grounding` override silently falls back to the allowlist default at this cache-bypass site even though the real review path honors it, producing per-call-site behavioral drift.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #4100
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: 51 registered-repo PR(s), 43 merged, 509 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 509 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Linked issue satisfaction

Addressed
The diff adds "grounding" to CONVERGED_FEATURE_KEYS and FEATURE_GLOBAL_FLAG, migrates both real call sites in src/queue/processors.ts from the raw isGroundingEnabled/convergedRepoAllowed check to resolveConvergedFeature, includes an explicit written justification (in both the PR description and the focus-manifest.ts comment) that the disposition-path coupling was tied to a now-dead aiCiRefutationA

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: 51 PR(s), 509 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 the manual-review Gittensor contributor context label Jul 8, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Reviewed the flagged blocker — it's a false positive, overriding.

The claim is that `reviewManifest` (used at the `dynamicReviewFeatures.grounding` call site in `maybePublishPrPublicSurface`, via `resolveReviewManifestForAiReview(env, repoFullName, reviewManifestForAutoReview)` at processors.ts:9219) can be null for a grounding-only repo because its load condition "still only checks rag/reputation."

That's not the actual shape. `reviewManifestForAutoReview` comes from `resolveAutoReviewSkipForPullRequest` (processors.ts:6802-6847), whose manifest load at line 6818 is:

const reviewManifest = await loadRepoFocusManifest(env, args.repoFullName).catch(() => null);

unconditional, no rag/reputation/grounding gate at all. Confirmed via `git diff origin/main` that this line is byte-identical before and after this PR; it was never conditionally gated. So there's no "pre-PR pattern this mirrors" of a rag/reputation-only load condition to widen — the premise doesn't hold, and `dynamicReviewFeatures.grounding`'s `resolveConvergedFeature(env, reviewManifest, "grounding", repoFullName)` call gets a real (or genuinely-absent-on-load-failure) manifest exactly like every other feature in that object, with no per-call-site drift.

(This is a different function from `runAiReviewForAdvisory`'s `featureManifest`, which — correctly, per the same comment thread — DOES have a conditional load that was widened with `|| isGroundingEnabled(env)`. The two manifests are loaded by different helpers with different shapes; only one of them was ever conditional.)

CI green, mergeable clean, no other blockers — merging.

@JSONbored
JSONbored merged commit 3cfe2a9 into main Jul 8, 2026
12 checks passed
@JSONbored
JSONbored deleted the feat/grounding-config-4100 branch July 8, 2026 02:07
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.

Add config-as-code coverage for the grounding feature

1 participant