Skip to content

fix(github): app.ts/comments.ts repoFullName parsing is missing the segment-count/whitespace guard every sibling GitHub-write module has #8311

Description

@JSONbored

Context

Several src/github/*.ts modules validate an incoming repoFullName: string before splitting it into owner/repo and issuing a GitHub API call. src/github/pr-actions.ts (its splitRepo helper, #6613), src/github/assignees.ts, src/github/labels.ts, and src/github/issues.ts/src/github/milestones.ts (their own copies of parseRepoFullName) all use the identical guard:

// e.g. src/github/labels.ts:9-19
function parseRepoFullName(repoFullName: string): { owner: string; repo: string } {
  const parts = repoFullName.split("/");
  const owner = parts[0];
  const repo = parts[1];
  // Reject any whitespace ... so a padded slug can never reach a GitHub call — a valid owner/repo name never
  // contains spaces.
  if (parts.length !== 2 || !owner || !repo || /\s/.test(repoFullName)) {
    throw new Error(`Invalid repository full name: ${repoFullName}`);
  }
  return { owner, repo };
}

This guards two failure modes explicitly named in these files' own comments: (1) "owner/repo/extra" — a naive two-variable destructure silently drops the extra segment and issues a call against owner/repo instead of erroring on the caller's malformed input; (2) "owner/ repo" / " owner/repo" — padded segments that pass a bare truthiness check and get encodeURIComponent-ed straight into a GitHub API URL.

src/github/app.ts — the file with the heaviest GitHub-write traffic in this codebase (installation tokens, check-run creation, workflow-run cancellation) — never adopted either guard. Three call sites there do a bare two-variable destructure with only a truthiness check:

  • src/github/app.ts:446 (getRepositoryCollaboratorPermission): const [owner, name] = repoFullName.split("/"); if (!owner || !name || !login) return null;
  • src/github/app.ts:620 (cancelInFlightWorkflowRunsForHeadSha): const [owner, repo] = repoFullName.split("/"); if (!owner || !repo) return { kind: "error", ... };
  • src/github/app.ts:911 (createOrUpdateNamedCheckRun, the function backing every createOrUpdate*GateCheckRun/createOrUpdateCheckRun call — i.e. the gate-check-posting path): const [owner, repo] = repoFullName.split("/"); if (!owner || !repo) throw new Error(...);

src/github/comments.ts:84-90 (createOrUpdateIssueCommentWithMarker, backing createOrUpdatePrIntelligenceComment/createOrUpdateVisualFollowupComment/createOrUpdateAgentCommandComment) is a partial case: it has the segment-count guard (with a comment explicitly citing "owner/repo/extra" as the reason) but is missing the /\s/ whitespace check that pr-actions.ts/assignees.ts/labels.ts added under #6613.

test/unit/github-app.test.ts only exercises the "no slash at all" case ("invalid" → throws) at each of the three app.ts call sites — there is no test for a 3-segment or whitespace-padded value anywhere in that file, unlike the sibling guard's own tests in github-pr-actions.test.ts (via pr-actions.ts's splitRepo).

Requirements

  • app.ts's three call sites (getRepositoryCollaboratorPermission, cancelInFlightWorkflowRunsForHeadSha, createOrUpdateNamedCheckRun) must reject a repoFullName that is not exactly two non-empty, whitespace-free segments — the identical guard already used by pr-actions.ts/assignees.ts/labels.ts/issues.ts/milestones.ts.
  • comments.ts:84-90's existing guard must additionally reject whitespace (/\s/.test(repoFullName)), bringing it in line with the #6613 fix already applied to pr-actions.ts/assignees.ts/labels.ts.
  • Each call site's existing failure contract must be preserved exactly (do not change return-null vs. return-error-object vs. throw semantics per call site — only tighten the validation each one already performs):
    • getRepositoryCollaboratorPermission returns null.
    • cancelInFlightWorkflowRunsForHeadSha returns { kind: "error", warning: ... }.
    • createOrUpdateNamedCheckRun throws Error("Invalid repository full name: ...").
    • createOrUpdateIssueCommentWithMarker throws Error("Invalid repository full name: ...") (already does; just add the whitespace condition to its existing check).
  • ⚠️ Required pattern: follow this repo's established convention of a small, per-module local copy of the parse/validate helper (see assignees.ts/labels.ts/issues.ts/milestones.ts, each of which duplicates this ~10-line check rather than importing a shared one — issues.ts:5-15's own comment: "each GitHub-write module keeps its own copy rather than importing a shared one, matching the existing house convention for this tiny pure check"). Do not introduce a new shared cross-file helper in client.ts or elsewhere for this fix — add the guard logic directly at each of the four affected call sites (or as a small local helper within app.ts reused by its own three call sites, since they're in the same file), matching the pattern already proven out five times elsewhere in this same directory.

Deliverables

  • src/github/app.ts: harden all three call sites listed above with the segment-count + whitespace guard (a local helper within app.ts used by all three is fine; a new file/shared cross-module export is not — see the required-pattern note above).
  • src/github/comments.ts:90: add the /\s/.test(repoFullName) condition to the existing segment-count check.
  • Regression tests in test/unit/github-app.test.ts (three cases, one per call site) and test/unit/github-comments.test.ts (one case), each asserting that both "owner/repo/extra" and "owner/ repo" are rejected the same way the existing "invalid" (no-slash) case already is — mirroring the existing coverage pattern in test/unit/github-pr-actions.test.ts/github-assignees.test.ts/github-labels.test.ts for the sibling guard.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on every changed line — both the newly-rejected malformed-input branch and the existing valid-input branch at each of the four call sites must be exercised by a real test, not just inferred from an existing "no slash" case.

Expected Outcome

src/github/app.ts and src/github/comments.ts reject the same malformed repoFullName shapes (extra path segments, embedded whitespace) that every sibling GitHub-write module in src/github/ already rejects, closing the one remaining inconsistency in this defense-in-depth boundary — with no behavior change for any well-formed owner/repo value.

Links & Resources

  • src/github/pr-actions.ts:23-32 (splitRepo, the #6613 whitespace fix)
  • src/github/assignees.ts:7-19, src/github/labels.ts:9-19, src/github/issues.ts:5-15, src/github/milestones.ts:5-15 (the five existing copies of this exact guard)
  • src/github/comments.ts:84-90 (the partial case — has segment-count, missing whitespace)
  • src/github/app.ts:440-457, :613-622, :895-912 (the three ungated call sites)
  • test/unit/github-app.test.ts:2289-2305 (the existing, incomplete coverage)

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