review: unwind the retired unifiedComment feature key - #6134
Merged
Conversation
The unified-comment bridge became the sole PR-comment renderer in #6128, which left the unifiedComment ConvergedFeatureKey and its LOOPOVER_REVIEW_UNIFIED_COMMENT env flag declared but functionally inert. Since the drift-check gate requires every declared LOOPOVER_REVIEW_* flag to be documented, and there was no working precedent for a retired-but-still-required flag, this fully removes the key/flag from the resolver, config-as-code examples, self-hosting docs, and env declarations (regenerating worker-configuration.d.ts via cf-typegen) rather than leaving stale references. Also fixes the "signal table" description in docs.how-reviews-work.tsx, left over from the earlier decision-drivers redesign (#6083), and a timing-flaky GraphQL single-flight test unrelated to this change but caught by the same full-suite run (crypto.subtle.digest is backed by a real OS thread pool, so two concurrent callers with identical inputs can complete in either order under load; stubbed with a deterministic pure-microtask digest for the one test that exercises the race).
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
Bundle ReportChanges will decrease total bundle size by 662 bytes (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-15 12:22:38 UTC
⏸️ Suggested Action - Manual Review Review summary Nits — 4 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agent
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6134 +/- ##
==========================================
- Coverage 95.32% 95.32% -0.01%
==========================================
Files 597 597
Lines 47133 47132 -1
Branches 15026 15025 -1
==========================================
- Hits 44930 44929 -1
Misses 1477 1477
Partials 726 726
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
unifiedCommentConvergedFeatureKey(and itsLOOPOVER_REVIEW_UNIFIED_COMMENTenv flag) — the flag became functionally inert in review: remove the legacy comment renderer (#6103) #6128 once the unified-comment bridge became the sole PR-comment renderer, but was left declared "for compatibility." This surfaced a real conflict:test/unit/check-docs-drift-script.test.ts's regression guard requires everyLOOPOVER_REVIEW_*flag declared insrc/env.d.tsto be documented somewhere, and there's no existing precedent for "declared but doc-exempt" within theLOOPOVER_REVIEW_*namespace. Rather than fake-document a no-op flag, this removes it end to end:CONVERGED_FEATURE_KEYS, the resolver'sFEATURE_GLOBAL_FLAGmap,isUnifiedReviewCommentEnabled(now fully dead),env.d.ts,wrangler.jsonc,.env.example/.env.selfhost.example, and regeneratesworker-configuration.d.tsviacf-typegen..loopover.yml.example+ its twinconfig/examples/loopover.full.yml) and five self-hosting docs pages (docs.self-hosting-configuration,docs.privacy-security,docs.github-app,docs.tuning,docs.how-reviews-work) to drop every mention.docs.how-reviews-work.tsxleft over from the earlier decision-drivers redesign (review: collapse signal table into decision drivers + advisory fold #6083) — the page still described the retired flat signal-table layout instead of the current always-visible "Decision drivers" + collapsed "Context & advisory signals" shape.CONVERGENCE_RUNBOOK.md's now-inaccurate claim that the flag still drives the comment path.feature-activation.test.ts,focus-manifest.test.ts,visual-wire.test.ts,queue-3/4/5.test.ts,linked-issue-satisfaction-run.test.ts,reputation-wiring.test.ts,unified-comment-bridge.test.ts) to drop the retired key/flag from fixtures and assertions.test/unit/github-graphql-cache.test.ts("single-flights concurrent cold misses for the same query").graphqlCacheKeyawaits two realcrypto.subtle.digestcalls before checking the single-flight map; since WebCrypto digests are backed by a real OS thread pool, two concurrent callers with identical inputs can complete their digest awaits in either order under load — reproduced standalone outside vitest at an ~11% failure rate over 500 trials with zero induced CPU contention. Stubbed a deterministic pure-microtask digest for this one test only (restored viamockRestore()), which resolved the race deterministically (0/500 in the same repro) without touching the production single-flight logic or changing what the test proves.Test plan
npx tsc --noEmitcleannpm run test:coverage(unsharded): 884 files / 17039 tests passed, 0 failuresnpm audit --audit-level=moderate: 0 vulnerabilitiesnpm run test:ci(full local gate incl. lint, drift checks, engine parity, MCP/miner packs, UI build): green (verified via direct exit code, not a piped/tee'd one)origin/main, no conflicts