Skip to content

engine(miner): worktree-plan's slugifyAttemptId collides for distinct attempt ids #10312

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/miner/worktree-plan.ts's module header (lines 9-11) makes an explicit
no-collision guarantee:

"naming is deterministic and keyed on the attempt id (never a random suffix), so two concurrent
attempts on the same repo can never be handed the same worktree path or branch"

planWorktree derives both worktreePath and branchName solely from slugifyAttemptId (lines
38-51):

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

Lowercasing and collapsing any run of non-[a-z0-9._-] characters to a single - means genuinely
distinct attempt ids can collide on the same slug: "Attempt-ABC" and "attempt-abc" both produce
"attempt-abc"; "retry:42" and "retry!42" both produce "retry-42". Since the slug is the sole
input to both the worktree path and the branch name, two such attempts would be handed the identical
worktree/branch — directly contradicting the module's own guarantee.

attemptId is caller-suppliable, not internally generated: packages/loopover-miner/lib/attempt-cli.ts
line 406 reads options.attemptId ?? ${parsed.repoFullName.replace("/", "_")}-${parsed.issueNumber}-${nowMs}``
— an operator or an automated caller can pass an explicit attemptId, making this reachable from the
real CLI, not just a theoretical input. The would-be collision guard in `worktree-pool.ts`,
`isWorktreeAllocated`, checks the raw `attemptId` string, not the slug, so it does not catch this
class of collision either.

test/unit/worktree-plan.test.ts's "no collision" assertion (lines 24-32) only compares
"attempt-42" vs "attempt-43" — two slugs that were already distinct before slugification — so it
never exercises a case/punctuation-collapsing collision.

Requirements

  • Change planWorktree (or slugifyAttemptId) so that two attempt ids which are distinct strings
    but collapse to the same slug under the current sanitization NEVER produce the same
    worktreePath/branchName. The fix must preserve slugifyAttemptId's existing properties: it is
    still deterministic (same attemptId in → same output every time), the result is still
    filesystem- and git-ref-safe, and the existing truncation/trim-trailing-separator behavior for the
    MAX_SLUG_LENGTH case (fix(engine): slugifyAttemptId's trim-then-truncate order can produce a git-invalid worktree branch name ending in "." #7528) is unchanged for ids that don't collide.
  • A reasonable approach is appending a short deterministic disambiguator derived from the
    original, pre-sanitization attemptId (e.g. a short hash of the raw string) whenever the
    sanitized slug differs from a case-normalized/character-collapsed reduction of the input — but the
    exact mechanism is up to the implementation as long as it satisfies the collision-free requirement
    above and the existing MAX_SLUG_LENGTH cap is still respected after any disambiguator is
    appended.
  • Do not change WorktreeExecFn, addWorktree, removeWorktree, or the retention-policy behavior
    in this file — this issue is scoped to the naming/collision logic only.

Deliverables

  • planWorktree({ repoPath: "/repo", attemptId: "Attempt-ABC" }) and
    planWorktree({ repoPath: "/repo", attemptId: "attempt-abc" }) produce different
    worktreePath and branchName values.
  • planWorktree({ repoPath: "/repo", attemptId: "retry:42" }) and
    planWorktree({ repoPath: "/repo", attemptId: "retry!42" }) produce different worktreePath
    and branchName values.
  • The same attemptId passed twice still produces the identical plan (determinism is
    preserved).
  • The existing MAX_SLUG_LENGTH/trailing-separator-trim behavior (fix(engine): slugifyAttemptId's trim-then-truncate order can produce a git-invalid worktree branch name ending in "." #7528) still holds for ids
    that do not collide with anything.

All four Deliverables are required in this one PR — there is no narrower scope for this issue.

Test Coverage Requirements

packages/loopover-engine/src/** is measured by Codecov via two separate uploads whose hits are
unioned
— root test/** AND packages/loopover-engine/test/**. This module's existing test lives
at root test/unit/worktree-plan.test.ts — add the new collision-regression tests there (add
packages/loopover-engine/test/** coverage too if the fix touches any code path not already
exercised from root). Assert each Deliverable above as an explicit test case. Target 100% branch
coverage of any new disambiguation logic.

Expected Outcome

worktree-plan.ts's documented no-collision guarantee holds for attempt ids that differ only in
case or punctuation, not just for ids that were already distinct after sanitization.

Links & Resources

  • packages/loopover-engine/src/miner/worktree-plan.ts (module header lines 9-11, slugifyAttemptId
    lines 38-51, planWorktree)
  • packages/loopover-miner/lib/attempt-cli.ts (line ~406, the real caller supplying attemptId)
  • test/unit/worktree-plan.test.ts
  • Milestone: Miner Wave 4.6 — AMS Hardening Round 3

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