Skip to content

Add branchFormat support for Jira issues#937

Open
kazimzaidi wants to merge 1 commit into
iloom-ai:mainfrom
kazimzaidi:feat/jira-branch-format
Open

Add branchFormat support for Jira issues#937
kazimzaidi wants to merge 1 commit into
iloom-ai:mainfrom
kazimzaidi:feat/jira-branch-format

Conversation

@kazimzaidi

Copy link
Copy Markdown

Summary

  • Add branchFormat configuration option for Jira issue management, enabling custom branch naming templates
  • Introduce TemplateBranchNameStrategy with {ticketId} and {slug} variable substitution
  • Wire branchFormat through the full pipeline: settings schema → JiraIssueTracker → IssueTracker interface → LoomManager → BranchNamingService
  • Expose branchFormat on LinearService for parity (was already in schema but unused)

Motivation

Jira users often have branch naming conventions tied to their ticket IDs (e.g., PRINT-1234-fix-deps-bug). The current hardcoded feat/issue-{number}__{slug} format doesn't support this. Linear already had branchFormat in its schema (though unused) — this completes the feature for both providers.

Configuration

{
  "issueManagement": {
    "provider": "jira",
    "jira": {
      "branchFormat": "{ticketId}-{slug}"
    }
  }
}

il start PRINT-1234 → branch name: print-1234-fix-deps-bug

Test plan

  • All 4995 existing tests pass (zero failures)
  • TypeScript compiles with zero errors
  • Added 14 new tests covering TemplateBranchNameStrategy, slugify, and branchFormat integration
  • Manual test: configure branchFormat in .iloom/settings.json and run il start with a Jira ticket

🤖 Generated with Claude Code

Allow Jira users to configure a custom branch naming template
via issueManagement.jira.branchFormat in settings.json. The
template supports {ticketId} and {slug} variables, producing
branches like "print-1234-fix-deps-bug" instead of the default
"feat/issue-PRINT-1234__fix-deps-bug" format.

Changes:
- Add TemplateBranchNameStrategy and slugify utility
- Add branchFormat to Jira settings schema (both v1 and v2)
- Wire branchFormat through JiraIssueTracker -> IssueTracker
  interface -> LoomManager -> BranchNamingService
- Expose branchFormat on LinearService for parity
- Add comprehensive tests for template strategy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@acreeger acreeger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

First off, thank you @kazimzaidi for taking this on. The overall shape of the change is great: the slugify factoring is clean, the priority ordering in generateBranchName is clear, and a user-configurable template is a feature we definitely want.

Before we merge, I'd like us to address a systemic issue that popped up when I traced how branch names get parsed elsewhere in the app, plus a few smaller items inline. Really appreciate your patience on this one.

🔴 Critical: branchFormat output cannot be reverse-parsed by the rest of the app

When a user configures branchFormat: "{ticketId}-{slug}" and runs il start PRINT-1234, the generated branch is print-1234-fix-deps-bug (no issue- prefix, no __ separator). Unfortunately every loom-lookup path in the CLI currently relies on that prefix being present:

  • GitWorktreeManager.findWorktreeForIssue (src/lib/GitWorktreeManager.ts:427) uses regex (?:^|[/_-])issue[-/]${issueNumber}(?:-|__|$), which will not match these branches.
  • extractIssueNumber (src/utils/git.ts:183) has the same issue- assumption across all four priority patterns.

The downstream impact is wide. Every one of these commands calls findWorktreeForIssue and will report No worktree found for looms that exist: il finish, il cleanup, il dev-server, il shell, il vscode, il open, il summary, il recap, il run.

Suggested fix: have findWorktreeForIssue fall back to a metadata-based scan (match against LoomMetadata.issue_numbers) when the branch regex misses. Metadata is authoritative; branch regex is a bootstrap heuristic. This also protects us from the next custom-format foot-gun.

🟠 Minor item, separate from the above

The PR base is f403faa, which is a bit behind main. A rebase will bring in the new effort / swarmEffort fields from #959 and make the diff cleaner. No functional impact, just a nice-to-have.

🟢 What I liked

  • Clean factoring of slugify into a shared helper
  • Clear priority ordering (explicit strategy, then branchFormat, then default)
  • Solid test coverage for the happy paths
  • Config plumbing from Zod schema through to the tracker follows the existing pattern nicely

Thanks again for the contribution, and sorry for the extra round trip. Happy to pair on the findWorktreeForIssue fallback if that would help.

.replace(/\{slug\}/g, slug)

// Normalize: lowercase, remove trailing hyphens
return branchName.toLowerCase().replace(/-+$/g, '')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] Lowercasing the full template output folds the Jira ticket ID, so PROJ-123 becomes proj-123. That breaks Jira smart-commit integration, Jira/Bitbucket branch-to-issue auto-linking, and any CI parser that keys off the upper-case ticket ID. Since the whole point of opting into a custom template is usually to match external tooling, we're defeating the purpose here.

The {slug} portion is already lowercased inside slugify, so suggest only normalising trailing hyphens and leaving the rest of the template's casing intact:

return branchName.replace(/-+$/g, '')

If we want a safety net for weird casing in user templates, a preserveCase option (default true) is the most flexible path.


const branchName = this.template
.replace(/\{ticketId\}/g, ticketId)
.replace(/\{slug\}/g, slug)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] The template is a free-form string from settings.json and nothing here sanitises git-forbidden characters. A user writing "feature WIP/{ticketId}" or "{ticketId}:{slug}" will compile fine but fail later at git checkout -b with a cryptic error. Consider either stripping/rejecting any of :, ~, ^, ?, *, [, \, spaces, and .. here, or adding a Zod .refine() at schema load time (see SettingsManager comment). Loud failure at config load is friendlier than a broken il start.

.replace(/[^a-z0-9]+/g, '-')
.replace(/^-|-$/g, '')
.substring(0, maxLength)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] substring(0, maxLength) runs after replace(/^-|-$/g, ''), so a truncation that lands on a hyphen can still leave a trailing -. For example slugify('Add-User', 4) yields add-. Reordering to trim hyphens last fixes it:

return title
  .toLowerCase()
  .replace(/[^a-z0-9]+/g, '-')
  .substring(0, maxLength)
  .replace(/^-|-$/g, '')

Worth adding a test that asserts the output never ends in -.

issueNumber,
title,
strategy: nameStrategy.constructor.name,
branchFormat,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Small thing: this debug log now includes title, which per CLAUDE.md must never reach telemetry. Local stderr-only logging is fine, but if anything ever wires this logger into the telemetry pipeline the title would leak. Either drop title from the log, or add a comment noting this is deliberately local-only so future us doesn't accidentally promote it.

branchFormat: z
.string()
.optional()
.describe('Branch naming template for Jira issues. Variables: {ticketId} (e.g., "PROJ-123"), {slug} (slugified title). Example: "{ticketId}-{slug}" → "proj-123-fix-bug"'),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] A user can set branchFormat: "my-branch" or "" today and every issue resolves to the same branch (collisions as soon as they run il start twice). Suggest a .refine() that requires the string to be non-empty and contain at least one of {ticketId} or {slug}:

branchFormat: z
  .string()
  .min(1)
  .refine(
    v => v.includes('{ticketId}') || v.includes('{slug}'),
    { message: 'branchFormat must contain at least one of {ticketId} or {slug}' },
  )
  .optional()
  .describe('...')

Same change applies to the IloomSettingsSchemaNoDefaults copy further down.

Comment thread src/lib/LinearService.ts
// IssueTracker interface implementation
readonly providerName = 'linear'
readonly supportsPullRequests = false // Linear doesn't have pull requests
readonly branchFormat?: string | undefined

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Thanks for wiring branchFormat through on LinearService too! For symmetry, would you mind adding the same .describe(...) to Linear's schema entry that Jira has? Right now Linear's branchFormat field has no hint about {ticketId} / {slug} and users are likely to guess wrong (Handlebars syntax, {{key}}, etc.).

Comment thread src/utils/claude.ts
* Uses case-insensitive matching for issue number (Linear uses uppercase like MARK-1)
*/
function isValidBranchName(name: string, issueNumber: string | number): boolean {
export function isValidBranchName(name: string, issueNumber: string | number): boolean {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Tiny thing: isValidBranchName flipped from private to export in this PR but I don't see a new caller added. Either include the consumer here or revert to keep the module's public surface tight.

@acreeger

Copy link
Copy Markdown
Collaborator

Follow-up on the critical finding: design sketch for the parser fallback

Hey @kazimzaidi, sorry to pile more on top of the review. I want to flesh out the "branchFormat output cannot be reverse-parsed" issue so you have everything you need to act on it, because I realise I flagged it without giving you a concrete path forward. Thank you again for your patience on this one.

Why this has to be solved before shipping

Once a user sets branchFormat and runs il start PRINT-1234, the loom creates fine, but every loom-lookup command downstream fails. Specifically:

  • GitWorktreeManager.findWorktreeForIssue (src/lib/GitWorktreeManager.ts:427) uses (?:^|[/_-])issue[-/]${issueNumber}(?:-|__|$), which only matches the default issue-{id} shape.
  • extractIssueNumber (src/utils/git.ts:183) has the same assumption across all four priority patterns.

Every command that calls findWorktreeForIssue is affected: il finish, il cleanup, il dev-server, il shell, il vscode, il open, il summary, il recap, il run. The user sees "No worktree found" on a loom that exists. That's not really a usable shape for the feature.

Proposed design

Two changes, one union lookup and one new helper.

1. findWorktreeForIssue: regex OR metadata

Keep the regex as the fast path (covers all legacy and default-format branches, needs no I/O). Add a metadata-based fallback that scans LoomMetadata.issueKey and LoomMetadata.issue_numbers. MetadataManager becomes an optional second arg so existing tests and call sites keep compiling without changes; upgrade the real callers.

// src/lib/GitWorktreeManager.ts
async findWorktreeForIssue(
  issueNumber: string | number,
  metadataManager?: MetadataManager,
): Promise<GitWorktree | null> {
  const worktrees = await this.listWorktrees({ porcelain: true })
  const needle = String(issueNumber).toLowerCase()

  // Fast path: legacy / default-format branch names
  const pattern = new RegExp(`(?:^|[/_-])issue[-/]${issueNumber}(?:-|__|$)`, 'i')
  const byRegex = worktrees.find(wt => pattern.test(wt.branch))
  if (byRegex) return byRegex

  // Fallback: authoritative metadata lookup (covers custom branchFormat)
  if (metadataManager) {
    for (const wt of worktrees) {
      const meta = await metadataManager.loadForWorktree(wt.path)
      if (!meta) continue
      const match =
        meta.issueKey?.toLowerCase() === needle ||
        meta.issue_numbers?.some(n => String(n).toLowerCase() === needle)
      if (match) return wt
    }
  }
  return null
}

Wiring: the 9 command files (listed above) already instantiate both a GitWorktreeManager and a MetadataManager. Passing it through is a grep-and-edit.

2. resolveIssueNumber helper next to extractIssueNumber

extractIssueNumber has a narrow contract ("extract from a branch string") and some call sites genuinely have only the string, so don't overload it. Add a companion:

// src/utils/git.ts
export function resolveIssueNumber(
  branchName: string,
  metadata?: Pick<LoomMetadata, 'issueKey' | 'issue_numbers'> | null,
): string | null {
  if (metadata?.issueKey) return metadata.issueKey
  if (metadata?.issue_numbers?.[0]) return metadata.issue_numbers[0]
  return extractIssueNumber(branchName)
}

Migrate these call sites over:

  • loom-formatter.ts determineLoomType and extractIssueNumbers (they currently ignore metadata even when the caller has it).
  • IdentifierParser.parseForPatternDetection where it has a worktree in hand.
  • The metadata?.issueKey ?? metadata?.issue_numbers?.[0] ?? extracted pattern already repeated in commit.ts, summary.ts, and finish.ts collapses into a single call.

What this buys us (behavior table)

Branch Has metadata? Found before Found after
feat/issue-42__desc yes yes (regex) yes (regex)
feat/issue-42__desc no yes (regex) yes (regex)
print-1234-fix-bug custom yes no yes (metadata)
print-1234-fix-bug custom no no no (unrecoverable, acceptable)
my-hand-made-branch yes no yes (metadata)

Legacy behaviour is preserved. The only new ground is "has metadata, does not match regex", which is exactly the case this PR introduces.

Edge cases worth explicit tests

  1. Two worktrees claim the same issue (stale plus current): regex-first plus return-first-hit gives stable ordering. Consider a getLogger().warn for visibility.
  2. Casing: Jira keys are upper-case (PRINT-1234) but the regex already has the i flag and our metadata comparison is lower-cased on both sides.
  3. #42 vs 42: strip the # on the needle once at the top of findWorktreeForIssue.
  4. findWorktreeForPR is unaffected because the _pr_N suffix is added to the path by generateWorktreePath and is independent of branch shape.

Scope estimate

Roughly 50 to 80 LOC plus tests:

  • ~30 LOC in GitWorktreeManager.ts plus the optional second arg
  • ~15 LOC new helper in utils/git.ts
  • ~9 call-site touches to thread metadataManager through
  • 6 to 8 migrations from the three-way fallback to resolveIssueNumber
  • 6 new test cases (the table above plus the two ambiguity cases)

Two paths forward, pick whichever works for you

Option A: bundle into this PR. Scope grows but it ships as a coherent feature and is easier for reviewers to reason about as a unit.

Option B: split into a prereq PR. File a separate PR for just the parser fallback, land that first, then rebase this PR on top. A bit more PR plumbing but lets each change stay small.

I don't have a strong preference between A and B. Happy to review promptly either way. If you hit questions while wiring this up I am around, and again, really appreciate you driving this feature forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants