Cap the length of generated slugs

openspec/changes/cap-generated-slug-length/proposal.md

@@ -0,0 +1,72 @@
+# Cap the length of generated slugs
+
+## Context
+
+`slugify` (`pull.go:485`) turns a title into kebab-case with no length limit at
+all — it emits one hyphen-joined token per word for the entire input. Every
+generated change name is therefore as long as whatever it was derived from.
+
+Observed in real use: pulling ExopenGitHub/portal#4331, whose title is
+
+> Multi-entity integrations: "Create all" has no selection UI, and company-name
+> display is inconsistent across pages
+
+produced the directory
+
+```
+openspec/changes/multi-entity-integrations-create-all-has-no-selection-ui-and-company-name-display-is-inconsistent-across-pages
+```
+
+98 characters, which then has to be typed for every `-change` flag, shows up in
+every path in every log line, and had to be renamed by hand immediately. Compare
+against the slugs this repo actually uses — `add-planning-scan`,
+`add-worktree-support`, `add-agent-help-command` — all short, verb-led, and
+hand-picked. The generated ones do not resemble them.
+
+Three call sites are affected, in increasing severity:
+
+- `pull.go:93` — issue title. Long titles are common; a descriptive issue title
+  is good practice and is punished here.
+- `epic.go:83` — epic title, prefixed `epic:`. Same problem.
+- `spinoff.go:92` — **task text**. This is the worst case: task lines are full
+  sentences, often with trailing detail, so spinoff slugs are longer still.
+
+There is no test for `slugify` anywhere in the repo.
+
+## Why not just pass `-change`
+
+`pull` already accepts `-change`, and `slugFromMarker` can read a slug out of
+the body, so a workaround exists. But the default path is what agents and
+newcomers hit first, it produces something nobody would choose by hand, and
+`spinoff` has no equivalent escape hatch for its per-task slugs.
+
+## Proposed change
+
+Cap generated slugs at a sane length, truncating on a word boundary rather than
+mid-word, and never leaving a trailing hyphen.
+
+Suggested shape, to be confirmed by whoever picks this up:
+
+- A single `maxSlugLen` constant (proposal: 48 characters — long enough for
+  `add-agent-help-command`-style names with room to spare, short enough to type).
+- Truncate at the last hyphen at or before the cap, so words stay whole. If the
+  first word alone exceeds the cap, hard-truncate it rather than emit nothing.
+- Apply inside `slugify` so all three call sites benefit, rather than at each
+  call site.
+- `epic.go` prefixes `epic:` before the slug — decide whether the cap covers the
+  prefix or only the slug portion, and be explicit about it.
+
+## Collision risk
+
+Truncation can make two distinct titles collide. Worth deciding deliberately:
+
+- `pull` already handles an existing change directory somehow — check what it
+  does today and whether truncation makes that path more likely, rather than
+  assuming it is fine.
+- A numeric suffix on collision (`-2`) is the obvious fallback, but only if the
+  existing behaviour does not already cover it.
+
+## Out of scope
+
+Changing the slugs of changes that already exist. This only affects newly
+generated ones.

openspec/changes/cap-generated-slug-length/tasks.md

@@ -0,0 +1,71 @@
+# Tasks
+
+## Decide first
+
+- [x] Pick `maxSlugLen`. Proposal: 48. Sanity-checked against this repo's own
+      slugs (`add-agent-help-command` is 22; the longest existing ones sit
+      well under 40) — 48 confirmed.
+- [x] Decide whether `epic.go`'s `epic:` prefix counts toward the cap.
+      Decision: no — the cap applies inside `slugify` to the generated slug
+      only; `epic.go` prefixes `epic:` afterward, so the prefix is fixed
+      overhead on top of an already-capped, readable slug (documented as a
+      comment on `maxSlugLen`).
+- [x] Check what `pull` does today when the target change directory already
+      exists, and decide whether truncation needs a collision suffix or the
+      existing behaviour already covers it.
+      Finding: `pull.go` had **no existing collision guard at all** —
+      `os.MkdirAll` + `os.WriteFile` silently overwrote `proposal.md`/
+      `tasks.md` in place whenever the derived slug matched an existing
+      directory, regardless of whether it was the same issue being re-pulled
+      or a different one that happened to collide. This was a pre-existing
+      gap independent of slug length: two different issues with literally
+      identical titles already collided, untruncated. Truncation raises the
+      odds (demonstrated by `TestSlugifyDistinctLongTitlesCanCollideOnTruncation`)
+      but doesn't create a new class of bug on its own — it just makes an
+      existing one more likely, so it's fixed here rather than deferred.
+      **Fixed** in `pull.go`: a directory that already exists is only ever
+      overwritten when it's an intentional re-pull of the *same* issue (its
+      ref cache names that issue), or an explicit `-change` into a directory
+      with no ref cache at all (a hand-authored, never-synced local change —
+      the human named that exact directory; this is the normal
+      spec-first-then-pull flow, see `TestPullRecordsTaskBase`). Every other
+      case — a ref cache naming a *different* issue, or an auto-derived slug
+      colliding with an untracked hand-authored directory — gets a numeric
+      suffix (`-2`, `-3`, ...) via `nextAvailableSlug`, or an explicit error
+      for an explicit `-change` conflicting with a different tracked issue
+      (pass a different `-change`, or use `specsync adopt`). `spinoff.go`'s
+      own pre-existing error-on-collision is unaffected. `epic.go`'s slug is
+      not used as a directory path, so no collision surface there.
+
+## Implement
+
+- [x] Add the cap inside `slugify` (`pull.go:485`) so all three call sites get
+      it: `pull.go:93`, `epic.go:83`, `spinoff.go:92`.
+- [x] Truncate at the last hyphen at or before the cap so words stay whole.
+- [x] Never emit a trailing or leading hyphen after truncation.
+- [x] Hard-truncate when the first word alone exceeds the cap, rather than
+      returning an empty slug — `pull` errors out on an empty slug, so this
+      would turn a long single-word title into a failure.
+
+## Test
+
+`slugify` currently has no tests at all.
+
+- [x] Table test: normal short title unchanged; long title truncated on a word
+      boundary; title whose first word exceeds the cap; title of only
+      punctuation (already returns empty — lock that in); leading/trailing
+      punctuation; consecutive punctuation runs.
+- [x] Regression case from the report: the 103-character portal#4331 title
+      should produce something short and readable.
+- [x] Collision-risk case: two distinct long titles that truncate to the same
+      stem (`TestSlugifyDistinctLongTitlesCanCollideOnTruncation`) — documents
+      that `slugify` itself doesn't dedupe (it's a pure string function); the
+      directory-level collision this enables is what `pull_collision_test.go`
+      now covers directly (same-issue re-pull overwrites in place;
+      different-issue collision on a generated slug gets suffixed;
+      explicit `-change` collision with a different tracked issue errors;
+      explicit `-change` collision with an untracked hand-authored directory
+      still overwrites; an auto-derived slug colliding with an untracked
+      hand-authored directory gets suffixed; `-dry-run` previews the
+      suffixed slug without writing; `nextAvailableSlug` skips multiple
+      taken suffixes).

pull.go

@@ -96,6 +96,37 @@ func Pull(ctx context.Context, opts PullOptions) (PullResult, error) {
 		return PullResult{}, fmt.Errorf("could not derive a change name from issue %s; pass -change", opts.IssueID)
 	}
 
+	// A slug collision (two issues landing on the same directory — a
+	// generated slug and, more rarely, an explicit -change) must not
+	// silently overwrite an unrelated change's proposal.md/tasks.md.
+	//
+	// Two cases are left alone (existing behavior): re-pulling the *same*
+	// issue into its own existing directory (the intentional refresh path),
+	// and an explicit -change into a directory with no ref cache at all
+	// (a hand-authored, never-synced local change — the human named this
+	// exact directory, and binding an issue's content into it is the normal
+	// spec-first-then-pull flow; see TestPullRecordsTaskBase).
+	//
+	// A genuine conflict — the directory's ref cache names a *different*
+	// issue — refuses an explicit -change (pass a different one, or
+	// `specsync adopt`) and suffixes an auto-derived one instead of
+	// clobbering the other change. An auto-derived slug colliding with an
+	// untracked hand-authored directory is also suffixed, since nothing
+	// asked for that specific name.
+	if candidateDir := filepath.Join(opts.OpenSpecDir, "changes", slug); dirExists(candidateDir) {
+		existing, _ := LoadRefs(candidateDir)
+		conflictsWithDifferentIssue := len(existing) > 0 && !refsContainIssue(existing, issueID)
+		collidesWithUntrackedDir := len(existing) == 0 && opts.Slug == ""
+		if conflictsWithDifferentIssue {
+			if opts.Slug != "" {
+				return PullResult{}, fmt.Errorf("pull: change %q already exists and is not linked to issue %s; pass a different -change or use `specsync adopt`", slug, issueID)
+			}
+			slug = nextAvailableSlug(opts.OpenSpecDir, slug)
+		} else if collidesWithUntrackedDir {
+			slug = nextAvailableSlug(opts.OpenSpecDir, slug)
+		}
+	}
+
 	proposal, tasks, relatedURLs, origAsk, design, disc := splitBody(item.Body, item.Title)
 	// The body may carry only a linked stub for an overflowed design.md;
 	// recover the real content from the comment when supported.
@@ -480,8 +511,17 @@ func slugFromMarker(body string) string {
 	return strings.TrimSpace(rest[:j])
 }
 
+// maxSlugLen caps a generated slug's length. Long enough for
+// add-agent-help-command-style names with room to spare, short enough to
+// type as a -change flag and to keep in every log line and path. Applies to
+// the slug itself, not to a prefix a caller adds afterward (e.g. epic.go's
+// "epic:" marker prefix) — the prefix is fixed overhead, not part of the
+// generated name.
+const maxSlugLen = 48
+
 // slugify turns a title into a kebab-case slug: lowercase, with each run of
-// non-alphanumeric characters collapsed to a single hyphen and trimmed.
+// non-alphanumeric characters collapsed to a single hyphen and trimmed, then
+// capped at maxSlugLen.
 func slugify(s string) string {
 	var b strings.Builder
 	pendingHyphen := false
@@ -497,5 +537,59 @@ func slugify(s string) string {
 			pendingHyphen = true
 		}
 	}
-	return b.String()
+	return capSlug(b.String(), maxSlugLen)
+}
+
+// capSlug truncates s to at most max characters, preferring a word boundary
+// (the last hyphen at or before the cut) so truncation never splits a word.
+// When the first word alone exceeds max, it hard-truncates instead of
+// returning "" — an empty slug is treated as failure by every caller.
+func capSlug(s string, max int) string {
+	if len(s) <= max {
+		return s
+	}
+	cut := s[:max]
+	if i := strings.LastIndexByte(cut, '-'); i > 0 {
+		cut = cut[:i]
+	}
+	return strings.TrimRight(cut, "-")
+}
+
+// dirExists reports whether path exists (any type), for the pull collision
+// guard below.
+func dirExists(path string) bool {
+	_, err := os.Stat(path)
+	return err == nil
+}
+
+// refsContainIssue reports whether any cached ref in refs points at issueID —
+// i.e. the existing directory tracks the very issue being pulled, making this
+// an intentional refresh rather than a collision.
+func refsContainIssue(refs map[string]Ref, issueID string) bool {
+	for _, r := range refs {
+		if r.ID == issueID {
+			return true
+		}
+	}
+	return false
+}
+
+// nextAvailableSlug finds the first "<base>-2", "<base>-3", ... whose change
+// directory doesn't exist yet, trimming base to keep the result within
+// maxSlugLen when the numeric suffix would otherwise push it over.
+func nextAvailableSlug(openspecDir, base string) string {
+	for i := 2; i < 1000; i++ {
+		suffix := fmt.Sprintf("-%d", i)
+		candidate := base + suffix
+		if len(candidate) > maxSlugLen {
+			candidate = capSlug(base, maxSlugLen-len(suffix)) + suffix
+		}
+		if !dirExists(filepath.Join(openspecDir, "changes", candidate)) {
+			return candidate
+		}
+	}
+	// Exhausted a generous range of suffixes — fall back to the last one
+	// tried rather than looping forever; pull will simply overwrite it,
+	// same as today's pre-existing behavior.
+	return fmt.Sprintf("%s-999", base)
 }