Add PR title conventional commit versioning for pr-label

[ben-vaughan-nttd]

Summary

• Add --use-pr-title / AAV_USE_PR_TITLE to derive semver bump intent from conventional commit PR titles (parsed via github.com/leodido/go-conventionalcommits).
• Add --allow-branch-name-fallback / AAV_ALLOW_BRANCH_NAME_FALLBACK to fall back to branch-prefix mapping when the title is invalid, with loud logging and Azure Pipelines ##vso[task.logissue type=warning;] output.
• Introduce structured exit codes (semantic validation failures exit 3) and document PR-title mode in README and copilot instructions.

Test plan

• go test ./...
• Pre-commit hooks (golangci-lint, go vet, go test mod, gosec)
• Run aav pr-label --use-pr-title against a test PR with a conventional commit title
• Run aav pr-label --use-pr-title --allow-branch-name-fallback with an invalid title and confirm pipeline warning output

Made with Cursor

[aarti-joshi-nttd]

PR Review: Add PR title conventional commit versioning for pr-label (#34)

Repo: launchbynttdata/launch-ado-automatic-versioner
Author: ben-vaughan-nttd
Branch: feat/pr-title-conventional-commit-versioning → main

---

What This PR Does Well

• Clean separation of concerns: prtitle resolver, pipeline warning helper, and exitcodes are all new, independently testable packages with solid unit test coverage (valid/invalid titles, breaking-change footer, scoped titles, fallback behavior).
• resolveBumpIntent/resolveFromBranch refactor in prlabel.Service keeps the existing branch-mapping path intact while cleanly layering the new PR-title path on top — good backward compatibility.
• Documentation (README, copilot-instructions, CHANGELOG) is updated in the same PR and is thorough, including a new exit-code table and a commented pipeline snippet.

---

Fixes Required

Blocking

No blocking issues found.

Non-Blocking

#	File & Line	Issue
1	internal/cli/root.go:521-540 (mapPRLabelError)	Exit-code classification relies on strings.Contains matching against formatted error text ("getting pull request title", "listing pr labels", "adding pr label") instead of sentinel errors. This is brittle — any future change to the wrapping message text in service.go will silently reclassify an ADO API failure (exit 2) as a generic config error (exit 1) without any compiler/test signal. Consider exporting typed/sentinel errors from prlabel (or wrapping with exitcodes.WrapADOAPI at the call sites in service.go directly) instead of matching on message text.
2	internal/cli/root.go:490-494	The emitted ##vso[task.logissue type=warning;] message appends ": PR title %q is not a conventional commit" whenever result.PRTitle is non-empty — which is effectively always true in the fallback path (the title is fetched before parsing). This means the documented exact string Semver calculation fallback to branch name (README.md, copilot-instructions.md) is rarely what's actually emitted. Either update the docs to show the real format, or keep the emitted line matching the documented constant and log the extra detail via zap.Warn only (which already happens).
3	internal/cli/root.go (newPRLabelCommand)	--allow-branch-name-fallback without --use-pr-title is silently accepted and has no effect (the flag is only consulted inside the cfg.UsePRTitle branch of resolveBumpIntent). Since the docs state it only applies "with --use-pr-title", consider validating this combination and returning a config error instead of silently ignoring it.
4	internal/services/prlabel/service.go:974-980	fmt.Errorf("%w: %v", prtitle.ErrInvalidConventionalCommit, err) double-wraps the sentinel, since err returned from prtitles.Resolve already wraps ErrInvalidConventionalCommit. This produces a redundant message (e.g. "invalid conventional commit: invalid conventional commit: ..."). Consider just returning err directly since errors.Is checks in mapPRLabelError will still succeed.
5	internal/services/prlabel/service.go:982-989	The branchErr != nil fallback path is effectively unreachable: Apply already guarantees branch is non-empty whenever UsePRTitle && AllowBranchNameFallback, and that's the only error resolveFromBranch can return. Either add a code comment noting this is defensive/unreachable, or simplify given the invariant is enforced earlier.
6	internal/cli/root.go:435-441	if allowBranchFallbackFlag != nil is dead code — the flag is unconditionally bound via bindBoolFlag before RunE executes, so it's never nil. Minor readability cleanup.

---

Overall Recommendation: comment_only

[rakesh-gorige-nttd]

PR Review (additional feedback)

Recommendation: Approve with minor comments — no new blockers beyond what's already been raised.

---

Additional non-blocking items

1. exitcodes package is untested

internal/cli/exitcodes/exitcodes.go introduces the exit-code contract (CodeFor, SemanticError, WrapADOAPI, WrapConfig) but has no _test.go. A small table-driven test would lock in the behavior pipelines will rely on, especially:

• NewSemanticError → exit 3
• WrapADOAPI → exit 2
• unwrapped generic errors → exit 1 (current default)

Without this, a future refactor of mapPRLabelError could change exit semantics with no direct test signal.

---

2. Exit code 3 is never exercised at the CLI boundary

Service tests assert errors.Is(err, prtitle.ErrInvalidConventionalCommit), and main.go calls exitcodes.CodeFor(err), but nothing verifies the full path:

invalid title → mapPRLabelError → SemanticError → os.Exit(3)

Same gap for the fallback path writing ##vso[task.logissue type=warning;]… to stdout. The PR test plan still has those pipeline checks unchecked. Even a lightweight test using cobra's Execute() with a fake ADO client + captured stdout/stderr would close the loop.

---

3. Structured exit codes are effectively pr-label-only today

CodeFor is wired globally in main.go, but only pr-label maps errors into semantic/ADO/config types. ADO failures from infer-bump or create-tag still fall through to exit 1.

That may be intentional for this PR, but worth a one-line note in CHANGELOG/README ("structured exit codes currently apply to pr-label") so consumers don't assume exit 2/3 across all subcommands yet.

---

4. Document PR-title vs PR-description scope

GetPullRequestTitle only reads the ADO title field. The TestResolveBreakingChangeFooter case uses a multiline title with a BREAKING CHANGE: footer — that's unlikely in real ADO usage where titles are single-line.

Worth documenting explicitly:

• Breaking via ! in the header (feat!: …) is the practical ADO pattern.
• Footer-based breaking changes only apply if they appear in the title, not the PR description/body.

That sets correct expectations for teams adopting --use-pr-title.

---

5. Custom / non-enumerated commit types are rejected

bumpFromCommit returns an error for types outside the allowlist (unknown: something fails in tests). The README table lists supported types but doesn't say other conventional-looking types are rejected rather than defaulting to patch.

Calling that out avoids surprise when someone uses e.g. infra: or deps: and gets exit 3 instead of patch.

---

6. Invalid-title failure path could log the attempted title

When --use-pr-title is on and fallback is off, the CLI returns a semantic error but doesn't log the PR title that failed parsing (unlike the fallback path, which logs prTitle in zap). Adding the title to the error log on exit 3 would make pipeline debugging much easier without changing exit semantics.

---

7. GetPullRequestTitle SDK wrapper has no direct unit test

The new ADO method is covered indirectly via fakeClient in service tests, but sdk_client.go parsing/empty-title handling isn't unit-tested. Not blocking given the thin wrapper, but a test against a mocked SDK response (or a small table test for validation branches) would match the test density elsewhere in internal/ado/.

---

What I'm not re-raising

@aarti-joshi-nttd already covered: sentinel vs string-matched errors, pipeline warning text vs docs, silent --allow-branch-name-fallback without --use-pr-title, double-wrapped sentinel, defensive/unreachable branchErr, and the nil flag guard.

---

Overall

Architecture and backward compatibility look good; domain/service tests are solid; CI is green. The items above are mostly test coverage, docs precision, and operability — good follow-ups before or right after merge.

[aarti-joshi-nttd]

No blocking issues found. Detailed review comments posted separately.

[ben-vaughan-nttd]

Review feedback addressed (09d85cd)

Thanks @aarti-joshi-nttd and @rakesh-gorige-nttd — all non-blocking items from both reviews are addressed in this commit.

aarti-joshi-nttd

1. Exit-code classification — Replaced strings.Contains matching with a prlabel.ErrADOAPI sentinel. ADO failures are wrapped at the service call sites; mapPRLabelError now uses errors.Is.
2. Pipeline warning text — ##vso[task.logissue type=warning;] now emits the exact documented constant (Semver calculation fallback to branch name). Invalid-title detail stays in the Zap warning only.
3. Flag combo validation — --allow-branch-name-fallback without --use-pr-title now returns a config error (exit 1).
4. Double-wrapped sentinel — Invalid-title path returns the parse error directly (no re-wrap of ErrInvalidConventionalCommit).
5. Unreachable branchErr path — Fallback now calls branches.Resolve directly; Apply already requires a non-empty branch when fallback is enabled.
6. Dead nil check — Removed the always-true allowBranchFallbackFlag != nil guard.

rakesh-gorige-nttd

7. exitcodes tests — Added table-driven coverage for exit 1 / 2 / 3 (and nil-safe wrap helpers).
8. CLI boundary — Added tests for mapPRLabelError → CodeFor (including exit 3 with PR title in the message) and for the exact ##vso[...] warning line.
9. Exit codes scope — Documented that structured exit codes currently apply to pr-label only (README + CHANGELOG).
10. Title vs description — Documented that only the PR title is parsed; ! is the practical ADO breaking-change marker; footer-style BREAKING CHANGE only counts if present in the title text.
11. Unknown types rejected — Documented that types outside the allowlist (e.g. infra:, deps:) fail with exit 3 rather than defaulting to patch.
12. Failed title on exit 3 — CLI now logs prTitle on invalid conventional commit, and the semantic error message includes invalid format in "<title>".
13. GetPullRequestTitle validation — Extracted pullRequestTitleFromGitPR and added unit tests for nil/empty/blank title handling.

Happy to tweak further if anything still looks off.

[rabindra-yadav-nttd]

Approving

The latest update addresses the previous review feedback:

• Rejects --allow-branch-name-fallback unless --use-pr-title is enabled.
• Adds tests for semantic, Azure DevOps API, and configuration exit codes.
• Adds coverage for the exact Azure Pipelines fallback warning.
• Replaces brittle error-message matching with the typed ErrADOAPI error.
• Clarifies that structured exit codes currently apply to pr-label.
• Documents the recommended ! syntax for breaking changes in Azure DevOps PR titles.

I also verified that:

• go test -race ./... passes.
• All GitHub checks pass, including builds across supported platforms, linting, security, vulnerability scanning, and tests.
• git diff --check reports no formatting issues.

The implementation now has appropriate validation, error classification, documentation, and test coverage. I found no remaining blocking issues.

Approved.