Skip to content

orb(content-lane): applySurfaceGate discards the generic gate's warnings on the blocker-free hold path #10011

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

applySurfaceGate merges the surface-lane verdict onto the generic gate evaluation. Three of its four merge
paths deliberately UNION the two sides' warnings, because GateCheckEvaluation.warnings is what the public
PR comment renders — the bridge feeds it straight into the reviewer note
(buildDualReviewNotes({ …, warnings: args.gate.warnings, … }), src/review/unified-comment-bridge.ts:874).

The union paths (src/review/content-lane-wire.ts:148-168):

  if (opts?.aiJudgmentBlockersMode !== "gate" && isAiJudgmentOnlyFailure(generic) && surface.conclusion === "success") {
    return { ...surface, warnings: [...generic.warnings, ...surface.warnings] };
  }
  if (isDuplicateOnlyFailure(generic) && surface.conclusion === "success") {
    
      warnings: [...generic.blockers, ...generic.warnings, ...surface.warnings],
    };
  }
  return {
    
    blockers: [...generic.blockers, ...surface.blockers],
    warnings: [...generic.warnings, ...surface.warnings],
  };

The fourth path does not (src/review/content-lane-wire.ts:139-143):

  if (generic.blockers.length === 0 && generic.conclusion === "success") return surface; // generic was clean → surface stands
  if (generic.blockers.length === 0) {
    if (surface.conclusion === "success") return generic;
    return surface;
  }

generic.blockers.length === 0 with a non-success conclusion is exactly the generic gate's HOLD shape —
evaluateGateCheckCore returns conclusion: "neutral", blockers: [], and puts the whole reason set in
warnings for every one of its hold paths: the app-state hold (src/rules/advisory.ts:796-805), the
inconclusive-review hold (:831-840), the incomplete-secret-scan hold (:849-859), the size/guardrail hold
(:863-877) and the duplicate-only hold (:898-911). In all of those, warnings is the ONLY carrier of the
finding.

So when the surface lane returns a non-success verdict (manual ⇒ neutral, or close ⇒ failure —
surfaceVerdictToGate, src/review/content-lane-wire.ts:82-96) against a generic gate that was holding, the
bare return surface throws away every one of those findings plus every ordinary advisory warning. The posted
comment then shows only the registry-surface reason, and the contributor never learns that their PR also
tripped the size hold, the guardrail hold, or an incomplete secret scan — signals the sibling branches go out
of their way to preserve "so the concern stays visible in the public comment"
(src/review/content-lane-wire.ts:122-124).

Requirements

  • The generic.blockers.length === 0 / surface.conclusion !== "success" path must return the surface
    evaluation with warnings: [...generic.warnings, ...surface.warnings], matching the three sibling paths.
  • Everything else about that path must be unchanged: the returned conclusion, title, summary, enabled
    and blockers still come from surface. This issue is strictly about not dropping warnings.
  • The generic.blockers.length === 0 && generic.conclusion === "success" early return (:139) must NOT
    change — a clean generic gate has no hold findings to preserve and its advisory warnings are already
    handled by the surface-stands contract documented on that line.
  • The surface.conclusion === "success" sub-branch (:141, return generic) must NOT change — it already
    preserves the generic hold in full.
  • applySurfaceGate must stay PURE (no mutation of either argument, no I/O), as its doc comment states.
  • No duplicate entries: the union must not re-add a finding already present in surface.warnings; use the
    same plain concatenation the sibling branches use (they do not de-duplicate either, and changing that is
    out of scope).

⚠️ Required pattern: mirror the AI-judgment override branch immediately below it
(src/review/content-lane-wire.ts:148-150) — { ...surface, warnings: [...generic.warnings, ...surface.warnings] }.
What does NOT satisfy this issue: changing the returned conclusion on this path (that is the surface
lane's authority, not this fix); adding a de-duplication helper used by only this branch (a second,
divergent merge mechanism); mutating surface.warnings in place; a test-only PR asserting the current
dropping behaviour.

Deliverables

  • src/review/content-lane-wire.ts: the generic.blockers.length === 0 branch returns
    { ...surface, warnings: [...generic.warnings, ...surface.warnings] } when surface.conclusion !== "success".
  • A test in test/unit/content-lane-wire.test.ts: applySurfaceGate with generic = { enabled: true, conclusion: "neutral", title: "…", summary: "…", blockers: [], warnings: [oversizedPrFinding] }
    and surface = surfaceVerdictToGate({ verdict: "manual", summary: "…" }).evaluation returns
    conclusion: "neutral" and warnings containing BOTH oversizedPrFinding and the
    surface_lane_manual finding.
  • A test in test/unit/content-lane-wire.test.ts for the close side: the same generic against
    surfaceVerdictToGate({ verdict: "close", summary: "…" }).evaluation returns conclusion: "failure",
    blockers containing only the surface_lane_reject finding, and warnings containing
    oversizedPrFinding.
  • A test in test/unit/content-lane-wire.test.ts pinning that the two unchanged sub-cases still behave
    as before: a success generic with warnings still returns surface verbatim, and a holding generic
    against a success surface still returns generic verbatim.
  • A regression test at test/unit/content-lane-wire.test.ts named for this bug (e.g.
    "preserves the generic gate's hold warnings when a non-success surface verdict overrides it").

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
fixing the manual case but not the close case, or landing the fix without the two pinning tests for the
unchanged sub-cases — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/review/content-lane-wire.ts is
measured and gated. The change adds a surface.conclusion === "success" decision on a path that previously
returned unconditionally — both arms need a test (surface success ⇒ return generic; surface non-success ⇒
the unioned surface). The generic.blockers.length === 0 && generic.conclusion === "success" guard above it
also needs both arms exercised so the new code is reached at all.

Expected Outcome

A registry-submission PR that is simultaneously oversized, guardrail-held, or held on an incomplete secret
scan AND rejected/held by the surface lane keeps every one of those reasons in the posted comment, instead of
showing only the registry reason and silently dropping the rest.

Links & Resources

  • src/review/content-lane-wire.ts:130-169applySurfaceGate and the four merge paths
  • src/review/content-lane-wire.ts:82-96surfaceVerdictToGate (manual ⇒ neutral, close ⇒ failure)
  • src/rules/advisory.ts:796-911 — every generic hold shape: blockers: [] with the reason in warnings
  • src/review/unified-comment-bridge.ts:874 — the comment renderer's consumption of gate.warnings
  • test/unit/content-lane-wire.test.ts — the existing test file for this module

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions