Skip to content

fix(engine): slugifyAttemptId's trim-then-truncate order can produce a git-invalid worktree branch name ending in "." #7528

Description

@JSONbored

Context

const MAX_SLUG_LENGTH = 64;
function slugifyAttemptId(attemptId: string): string {
  const slug = attemptId
    .trim()
    .toLowerCase()
    .replace(/[^a-z0-9._-]+/g, "-")
    .replace(/^[-.]+|[-.]+$/g, "");
  if (!slug) throw new Error("invalid_attempt_id");
  return slug.slice(0, MAX_SLUG_LENGTH);
}

The leading/trailing -/. trim (.replace(/^[-.]+|[-.]+$/g, "")) runs before the
slice(0, MAX_SLUG_LENGTH) truncation, not after. . is in the allowed character class, so it survives
the character-class replace untouched. If the pre-truncation slug is longer than 64 characters and its
64th character happens to be a literal ., the truncated slug ends in . — a case the trim step never
gets a chance to catch, because it already ran on the untruncated string.

That slug feeds directly into the git ref name:

// planWorktree, same file
branchName: `${WORKTREE_BRANCH_PREFIX}${slug}`,   // WORKTREE_BRANCH_PREFIX = "loopover/attempt/"

and addWorktree runs git worktree add -b <branchName> <path> <baseBranch>. Git's own ref-name rules
(git help check-ref-format) reject a ref ending in . — so this fails with fatal: invalid reference
for a caller supplying a long-enough attempt id with a . landing exactly at the 64-char boundary.
GitHub repo names can legitimately contain dots (e.g. socket.io), and attempt-cli.ts:297 builds the
default attempt id as `${repoFullName.replace("/","_")}-${issueNumber}-${nowMs}` — a sufficiently
long owner/repo pair with a dot positioned at the truncation boundary reproduces this today with no
unusual input required. test/unit/worktree-plan.test.ts has no coverage of the truncation path at all,
so this edge case is both unhandled and untested.

Requirements

  • Reorder slugifyAttemptId so the leading/trailing -/. trim runs after truncation, not only
    before: truncate to MAX_SLUG_LENGTH first, then re-apply .replace(/^[-.]+|[-.]+$/g, "") to the
    truncated result (a second trim pass after truncation, in addition to — not instead of — the existing
    pre-truncation trim, since truncating first without any pre-trim could otherwise slice into the middle
    of a run of separator characters and produce a different, also-invalid edge case).
  • If the post-truncation trim empties the string (a pathological all-separator tail), fail the same way
    the current empty-slug case does (throw new Error("invalid_attempt_id")), not silently.

Deliverables

  • slugifyAttemptId never returns a slug ending in - or ., regardless of where in the original attemptId those characters fall relative to the 64-char boundary.
  • A regression test constructing an attempt id whose slug's 64th character is . before truncation, asserting the final slug has no trailing ./-.

Test Coverage Requirements

99%+ Codecov patch gate, branch-counted, on src/**. The new post-truncation trim branch must be
exercised by both a case where it fires (trailing ./- after truncation) and a case where it's a no-op
(truncation lands cleanly on an allowed non-separator character), plus the existing empty-slug-after-trim
throw path re-verified still fires correctly with the reordered logic.

Expected Outcome

planWorktree/addWorktree can never construct a git worktree add -b <branch> call with a
ref-format-invalid trailing-. branch name, regardless of the attempt id's length or where its
truncation boundary falls.

Links & Resources

  • packages/loopover-miner/lib/attempt-cli.ts:297 — the real default-attempt-id construction that can trigger this with an ordinary long owner/repo name.
  • test/unit/worktree-plan.test.ts — where the new regression test belongs.

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