fix(engine): re-trim slugifyAttemptId after truncation to avoid a git-invalid trailing separator - #7563
Conversation
…-invalid trailing separator slugifyAttemptId trimmed leading/trailing `-`/`.` before slicing to MAX_SLUG_LENGTH, so a slug longer than 64 chars whose 64th character is a `.` (a valid interior char that survives the character-class replace) was truncated to a value ending in `.`. That slug feeds the worktree branch name, and git rejects a ref ending in `.` (`git help check-ref-format`) with `fatal: invalid reference` -- reproducible today from the default attempt id built in attempt-cli.ts for a long owner/repo pair with a dot at the boundary. Re-trim the trailing separators after truncation. Only the trailing edge needs it: the pre-truncation trim already removed any leading separators and slice never introduces a new leading one, so the truncated slug keeps its non-separator first char and never empties. Closes JSONbored#7528
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7563 +/- ##
==========================================
- Coverage 88.48% 88.47% -0.01%
==========================================
Files 720 720
Lines 75748 75748
Branches 22558 22558
==========================================
- Hits 67023 67019 -4
Misses 7679 7679
- Partials 1046 1050 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Caution 🛑 LoopOver review result - reject/close recommendedReview updated: 2026-07-20 22:25:54 UTC
Review summary Nits — 4 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentFlagged checks (non-blocking)
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
LoopOver is closing this pull request on the maintainer's behalf (Linked issue overlaps another open PR; duplicate of open PR #7551). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed. |
What
slugifyAttemptId(packages/loopover-engine/src/miner/worktree-plan.ts) trimmed leading/trailing-/.before slicing toMAX_SLUG_LENGTH, not after..is in the allowed character class, so it survives the sanitizing replace. When the pre-truncation slug is longer than 64 chars and its 64th character is a literal., the truncated slug ends in.— a case the trim step never saw because it ran on the untruncated string.That slug becomes the worktree branch name (
loopover/attempt/<slug>), andaddWorktreerunsgit worktree add -b <branchName> …. Git's ref-name rules reject a ref ending in.(git help check-ref-format), so this fails withfatal: invalid reference. It reproduces from the default attempt idattempt-cli.tsbuilds (${repoFullName.replace("/","_")}-${issueNumber}-${nowMs}) when a long owner/repo pair (repo names may contain dots, e.g.socket.io) puts a.at the 64-char boundary — no unusual input required.Fix
Re-trim the trailing separators after truncation:
Only the trailing edge needs re-trimming: the pre-truncation trim already removed any leading separators, and
slice(0, n)never introduces a new leading one, so the truncated slug always keeps its non-separator first char and can never empty (so no extra empty-check branch is added).Tests
Two
REGRESSION (#7528)cases intest/unit/worktree-plan.test.ts, each constructing an attempt id whose slug's 64th char is a separator (.and-) before truncation, asserting the final branch name is the 63-char prefix and ends in neither.nor-. Both are proven to fail against the pre-fix code (received…a./…a-).Validation
npx vitest run test/unit/worktree-plan.test.ts— 10/10 pass.@loopover/enginebuild +npm run typecheck— clean (exit 0).git diff --checkclean.Closes #7528