fix(security): close the OTel scrub gap, MCP untrusted-title leak, and merge-readiness fail-open (#9162 #9163 #9167)#9229
Merged
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | a6b5d85 | Commit Preview URL Branch Preview URL |
Jul 27 2026, 08:48 AM |
Bundle ReportChanges will increase total bundle size by 67 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
JSONbored
force-pushed
the
fix/9162-9163-9167-trust-boundary
branch
from
July 27, 2026 08:25
8a86d4f to
6fab259
Compare
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-27 08:44:46 UTC
Review summary Blockers
Nits — 6 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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.
|
…e unreachable merge-readiness fill-unset semantic buildSlopGateBlocker called gateMode(policy.slopGateMode) with no ?? "advisory" fallback, unlike every sibling call site. Once gateMode()'s own fallback for a non-off/block/advisory value (including plain undefined) changed from "advisory" to "block" (#9167's fail-closed fix), any repo that never configured gate.slop.mode had its slop finding silently promoted to a hard blocker -- contradicting the field's own documented default. Added the missing fallback in both the host and engine-twin copies, plus a regression test. Also reverts applyMergeReadinessGate's "only fill in an unset sub-gate" change back to its original override behavior: RepositorySettings's sub-gate fields (linkedIssueGateMode etc.) are non-optional and DB-defaulted, so they are never actually undefined by the time they reach GateCheckPolicy -- the fill-unset-only variant could never fire in practice, and its own new tests only passed because a test helper's hardcoded baseline masked the same gap. The intended visibility fix for this composite (an operator's explicit sub-gate mode getting silently demoted) already lives one layer up, in config-lint.ts's mergeReadinessCompositeWarnings, which operates on the raw manifest where "unset" is a real, meaningful state.
JSONbored
force-pushed
the
fix/9162-9163-9167-trust-boundary
branch
from
July 27, 2026 08:45
6fab259 to
a6b5d85
Compare
Owner
Author
|
Addressed the blocking finding: `buildSlopGateBlocker` now supplies its own `?? "advisory"` fallback (both the host copy and the engine twin), matching every sibling `gateMode(policy.xGateMode ?? "advisory")` call site. Added a regression test asserting an unconfigured `slopGateMode` still never blocks. While fixing that, found and reverted a second, related problem in the same area: the earlier "merge-readiness composite only fills in an unset sub-gate" change (also part of #9167) turned out to be unreachable in practice — every sub-gate mode is already a concrete, DB-defaulted value by the time it reaches `GateCheckPolicy` (never actually `undefined`), so the fill-unset-only variant could never fire, and its own new tests only passed because a test helper's hardcoded baseline masked the same gap. Reverted `applyMergeReadinessGate` to its original override behavior (now consistent with the 5 pre-existing tests that were failing against the unreachable variant) and rely on `config-lint.ts`'s `mergeReadinessCompositeWarnings` — which operates on the raw, pre-default manifest, where "unset" is genuinely meaningful — for the visibility fix instead. |
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
Three "security/trust boundary that reads as enforced but silently isn't" fixes, bundled as one theme:
src/selfhost/otel.tswas the one egress the shared redaction-scrub never covered.otelSafeAttributesnow importsscrubStringfromredaction-scrub.ts(deleting its own duplicatedSECRET_KEYregex) so string attribute values are scrubbed, not just filtered by key.withOtelSpan'scatch no longer calls
span.recordException(err)(which bypasses the scrubber and writes a rawexception.stacktrace); it now builds the same "exception" span event by hand and routes it throughotelSafeAttributes. The span-status message goes through the same scrub. Separately, the PostHog-derivedtrace-destination fallback (picking a third-party host off
POSTHOG_API_KEYalone) now requires anexplicit
OTEL_TRACES_ENABLE_POSTHOG_FALLBACK=1opt-in — setting the key for product-analytics errorcapture no longer silently also selects a trace-export destination.
loopover_find_opportunitiescopied GitHub issue titles verbatim into a tool result read bya model holding local write tools. Added
src/mcp/untrusted-text.ts, a sharedsanitizeUntrustedMcpTexthelper (defuses markdown fences and HTML comments, neutralizes recognizedprompt-injection phrasing via the existing
neutralizePromptInjection, collapses control characters,truncates to 120 chars) and routed it through
toRankedEntry'stitlefield and through the two rawupstream-title fields
loopover_check_before_start's report surfaces(
target.requested.title,target.resolvedIssueTitle). Both tool descriptions now label the field asuntrusted data. Swept
loopover_validate_linked_issuetoo — its report never echoes a raw title (everyfield is already built from
sanitizePublicComment'd strings), so no change was needed there; noted as ajudgment call below.
mergeReadinessGateMode: advisoryused to unconditionally overridelinkedIssueGateMode,duplicatePrGateMode, andslopGateModeto its own mode, silently demoting an explicitly-configuredblockdown to advisory.applyMergeReadinessGate(both the host copy insrc/rules/advisory.tsand itsengine twin in
packages/loopover-engine/src/advisory/gate-advisory.ts) now only fills in a sub-gate modethe operator left unset — an explicit mode, stricter or looser, always wins.
gateMode()also nowfails closed on an unrecognized mode string (
"block", not the previous fail-open"advisory"coercion). Added a
config-lint.tswarning (mergeReadinessCompositeWarnings) so an operator who setsboth the composite and an explicit sub-gate mode sees that the sub-gate is authored, not overridden — the
effective config is never silently different from the authored one.
Judgment calls
loopover_validate_linked_issue's report(
buildLinkedIssueValidationinpackages/loopover-engine/src/signals/engine.ts) never echoes a rawissue title or the caller's
plannedChange.title— every string field on it already goes throughsanitizePublicComment. Confirmed by reading the function body; no change made there, only tocheck_before_start's two raw-title fields.fill-unset-only semantic the issue itself calls the least-surprising one, on both the host and engine
twins together (so no
packages/loopover-engineversion bump is needed — the engine-parity drift checkonly requires a version bump for a single-sided twin edit).
merge-readiness-composite-preserves-explicit-block) to thetest/fixtures/engine-parity/predicted-gate/golden-snapshot suite alongside the existingmerge-readiness-composite-blockfixture, covering the opposite (preserve-explicit) arm.Scope
type(scope): short summaryConventional Commit format.no unrelated UI/docs/dependency changes.
CONTRIBUTING.md; nosite/,CNAME, or VitePress changes.Validation
git diff --checknpm run typecheck(npx tsc --noEmit -p tsconfig.json --incremental false) — cleannpm run build --workspace @loopover/engine— cleannpm run build:mcp— cleanselfhost-otel.test.ts,rules.test.ts,selfhost-config-lint.test.ts,mcp-untrusted-text.test.ts(new),
find-opportunities.test.ts,mcp-find-opportunities.test.ts,mcp-output-schemas.test.ts,mcp-discovery.test.ts,mcp-cli-find-opportunities.test.ts,test/contract/engine-parity.test.ts,check-engine-parity-script.test.ts— 353/353 passing.v8coverage on every touchedsrc/**/packages/loopover-engine/src/**file against thistest subset:
otel.ts,advisory.ts,config-lint.ts,find-opportunities.ts,untrusted-text.tsall 100% branch on the changed lines;
server.ts's newsanitizePreStartCheckReportTitlesfunctionneeded one added test case to hit both its ternary branches (now both covered).
npm run test:coverage(full unsharded),npm run test:workers,npm run test:mcp-pack,npm run ui:*,npm audit— not run, by explicit instruction for this pass (time-boxed to thedirectly relevant tests + typecheck). Recommend running the full
npm run test:cigate before merge.If any required check was skipped, explain why:
npm run test:coverage/test:ci/npm auditwere intentionally skipped per the task's scope(targeted verification only) — the scoped coverage runs above cover every changed line/branch in this
diff, but the full gate has not been run end-to-end on this exact commit.
Safety
absence of injection markup and fenced/comment characters.
OTEL_TRACES_ENABLE_POSTHOG_FALLBACK) is off by default (fail-closed);tests cover both the missing-opt-in and non-
"1"loose-truthy-string cases.apps/loopover-uichanges.Notes
env.SOMETHINGreads were added/removed undersrc/selfhost/**, soselfhost:env-referencedoes notneed regenerating.
ui:openapi/cf-typegendo not need regenerating.