Skip to content

engine(results): buildResultsPayload builds the customer-facing PR link from unvalidated input #9611

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

packages/loopover-engine/src/results-payload.ts's buildResultsPayload composes the customer-facing
results payload (ResultsPayload, whose prLink is documented as the "Canonical PR URL"). It builds
that URL by raw interpolation:

const prLink = hasPr ? `https://github.com/${result.repoFullName}/pull/${result.prNumber}` : null;

Neither interpolated value is validated:

  • repoFullName is only z.string().min(1) at both entry points — the OpenAPI route schema
    (BuildResultsPayloadRequestSchema in src/openapi/schemas.ts) and the MCP tool schema
    (BuildResultsPayloadInput in packages/loopover-contract/src/tools/agent.ts:85-94). So
    POST /v1/loop/results-payload with repoFullName: "acme/widgets/../../evil" yields
    prLink: "https://github.com/acme/widgets/../../evil/pull/1", which every browser resolves to
    https://github.com/evil/pull/1. The same unvalidated value is also interpolated into summary,
    which is documented as "One readable, public-safe sentence a customer can act on".
  • prNumber is z.number().int().nullable().optional() — integer, but not positive. hasPr is
    result.prNumber !== null && result.prNumber !== undefined, so prNumber: 0 and prNumber: -3 both
    take the has-PR branch and produce .../pull/0 and .../pull/-3, plus a summary reading
    "Opened PR #-3 in ...".
  • additions/deletions are folded with f.additions ?? 0 and summed with no non-negative
    normalization, so caller-supplied negatives flow straight into totals and into the rendered
    (+N / -N) string.

The asymmetry is inside this one function: result.title is scrubbed —
const safeTitle = redactSecrets(result.title); — with a comment stating the scrub exists "so the
documented 'public-safe' contract actually holds"
. The two values that become a clickable link get no
treatment at all. The same package already has the exact validators needed:
parsePullRequestTargetKey (packages/loopover-engine/src/parse-pull-request-target-key.ts) rejects a
non-integer or non-positive pull number, and the repo-segment guard lives at
packages/loopover-engine/src/governor-ledger.ts:43-47.

ResultsPayload is consumed by buildCustomerLoopView
(packages/loopover-engine/src/customer-loop-view.ts), i.e. it renders on the customer dashboard.

Requirements

  • buildResultsPayload MUST treat repoFullName as valid only when it splits on / into exactly two
    segments that each satisfy the shared isValidRepoSegment guard. When it does not, prLink MUST be
    null and the summary's repo reference MUST render the literal string unknown repository instead
    of the raw value. The payload MUST still be returned (this function never throws).
  • hasPr MUST additionally require Number.isInteger(result.prNumber) && result.prNumber > 0. A
    prNumber of 0, a negative, or a non-integer MUST take the no-PR branch: prLink === null and the
    existing "No pull request was opened for ..." summary phrasing.
  • A valid repoFullName with an invalid prNumber MUST still render the repo name in the summary; an
    invalid repoFullName with a valid prNumber MUST still produce prLink === null.
  • additions and deletions MUST each be normalized to a non-negative integer before they enter
    diffPreview and totals, using the same Number.isFinite(v) ? Math.max(0, Math.floor(v)) : 0 rule
    the sibling Rent-a-Loop modules already use.
  • totals.files, diffPreview ordering, the MAX_DIFF_PREVIEW_FILES cap, the status default, and
    the redactSecrets(title) scrub MUST all be unchanged.

⚠️ Required pattern: reuse the isValidRepoSegment guard from
packages/loopover-engine/src/governor-ledger.ts:43-47 (or the shared module if it has already been
extracted), the positive-integer check from
packages/loopover-engine/src/parse-pull-request-target-key.ts, and the exact
finiteNonNegativeInt body from packages/loopover-engine/src/loop-consumption.ts:56-58. What does
NOT satisfy this issue: throwing on invalid input (the function's contract is pure in/out, never
throws); URL-encoding repoFullName instead of rejecting it; adding a new exported validation
helper as a parallel surface; or tightening the zod schemas in src/openapi/schemas.ts /
packages/loopover-contract instead of fixing the composer (the composer is the shared core used
by both the miner and ORB and must be correct on its own).

Deliverables

  • buildResultsPayload({ repoFullName: "acme/widgets/../../evil", prNumber: 1, title: "t" }) in
    packages/loopover-engine/src/results-payload.ts returns prLink === null and a summary
    containing unknown repository and not containing ../, asserted by a new named regression test
    in test/unit/results-payload.test.ts.
  • buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 0, ... }) and the same with
    prNumber: -3 each return prLink === null and the "No pull request was opened for acme/widgets"
    summary, asserted by new test cases.
  • buildResultsPayload({ ..., changedFiles: [{ path: "a", additions: -5, deletions: 2.7 }] })
    returns diffPreview[0] deep-equal to { path: "a", additions: 0, deletions: 2 } and
    totals equal to { files: 1, additions: 0, deletions: 2 }, asserted by a new test case.
  • buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 42, ... }) still returns
    prLink === "https://github.com/acme/widgets/pull/42", asserted by an existing or new test case.
  • No change to MAX_DIFF_PREVIEW_FILES, to the diff-preview slice, or to the redactSecrets call.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
fixing the prNumber guard and leaving repoFullName interpolated raw, or normalizing the counts
without touching the link — does not resolve this issue.

Test Coverage Requirements

packages/loopover-engine/src/**/*.ts is inside coverage.include in vitest.config.ts and
carries its own engine Codecov flag; the 99%+ branch-counted codecov/patch gate applies here in
full. Both arms of every new conditional need a test: valid vs invalid repoFullName, valid vs
invalid prNumber, and the four-way cross (valid repo + invalid PR, invalid repo + valid PR). The
finiteNonNegativeInt helper needs a finite, a non-finite, a negative, and a fractional input. The
traversal case and the prNumber: 0 case are the required named regression tests for this fix.

Expected Outcome

A malformed or traversal-shaped repoFullName, or a non-positive prNumber, can no longer produce a
customer-facing prLink pointing at an unrelated GitHub path, and negative/fractional file counts can
no longer reach the rendered spend/diff totals.

Links & Resources

  • packages/loopover-engine/src/results-payload.ts (the whole file)
  • packages/loopover-engine/src/parse-pull-request-target-key.ts (positive-integer precedent)
  • packages/loopover-engine/src/loop-consumption.ts:54-58 (finiteNonNegativeInt precedent)
  • packages/loopover-engine/src/customer-loop-view.ts (consumer)
  • src/api/routes.ts (POST /v1/loop/results-payload), src/openapi/schemas.ts
    (BuildResultsPayloadRequestSchema), packages/loopover-contract/src/tools/agent.ts:85-94

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