Skip to content

fix(skills): treat internal-scope feat commits as patch in /release#642

Merged
carlos-alm merged 3 commits intomainfrom
fix/release-skill-internal-scopes
Mar 27, 2026
Merged

fix(skills): treat internal-scope feat commits as patch in /release#642
carlos-alm merged 3 commits intomainfrom
fix/release-skill-internal-scopes

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • The /release skill's auto-detect logic treated any feat: commit as a minor bump
  • Internal dev tooling scopes (titan, skills, hooks, ci, bench, dogfood, release, housekeep, review, deps-audit, test-health, architect) are not user-facing features
  • Add an explicit exclusion list so feat(titan): etc. count as patch, not minor

This caused PR #639 to incorrectly bump to v3.5.0 instead of v3.4.1 (corrected in #641).

Test plan

  • Run /release with no version arg on a branch containing feat(titan): commits — should detect patch, not minor

The release skill's auto-detect logic treated any feat: commit as a
minor bump. Internal dev tooling scopes (titan, skills, hooks, ci,
bench, dogfood, etc.) are not user-facing features and should not
trigger a minor version bump. Add an explicit exclusion list so these
scopes count as patch.
@claude
Copy link
Copy Markdown

claude bot commented Mar 26, 2026

Claude finished @carlos-alm's task —— View job


Reviewing PR #642

Tasks:

  • Read the changed file to understand the implementation
  • Phase 1: Root Cause Analysis
  • Phase 2: Configuration Change Detection
  • Phase 3: Test Integrity
  • Phase 4: Code Quality
  • Phase 5: Backlog Compliance Assessment
  • Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: The /release skill incorrectly treated ALL feat: commits as minor version bumps, regardless of whether they were user-facing features or internal tooling. This caused PR #639 to bump to v3.5.0 instead of v3.4.1.

Why This Approach: Adding an explicit exclusion list for internal development scopes is the right approach. It preserves the semantic meaning of conventional commits while recognizing that not all "features" are customer-facing. The approach is:

  • Precise: Clearly defines what constitutes internal vs. user-facing scope
  • Maintainable: Uses an explicit allowlist that can evolve
  • Conservative: Defaults to minor for unknown scopes (safer than defaulting to patch)

Risk Assessment: ✅ Low Risk - This is a documentation/process change to an internal skill, not production code.

Configuration Change Detection

No configuration relaxation - This change actually makes the version detection logic MORE strict and precise, not more permissive.

Test Integrity

No test changes - This is purely documentation for an internal Claude skill.

Code Quality

The changes are well-structured:

  • Clear documentation: Explicit list with good examples (feat(titan) vs feat(cfg))
  • Comprehensive scope list: Covers all internal tooling scopes I can identify in the codebase
  • Good examples: Shows the distinction clearly
  • Consistent formatting: Matches existing documentation style

Minor suggestion: The scope list could be alphabetized for easier maintenance, but current order is acceptable.

Backlog Compliance

  • Zero-dep: ✓ - Pure documentation change, no new dependencies
  • Foundation-aligned: ✓ - Improves release process accuracy without violating any principles
  • Problem-fit: 4/5 - Directly improves the accuracy of release automation, which supports the foundation tooling
  • Breaking: No - This is additive logic that makes version detection more accurate
  • Tier: 1

Critical Concerns

None. This is a straightforward, well-reasoned fix to an internal process.

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: This addresses a real problem (incorrect version bumps) with a clean, well-documented solution. The change is low-risk, well-explained, and fixes the specific issue that caused v3.5.0 → v3.4.1 correction. The scope list is comprehensive and the documentation is clear.

Minor improvement: Consider alphabetizing the scope list for easier maintenance in future updates.


@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR fixes the /release skill's version auto-detect logic by introducing an explicit exclusion list of internal-developer scopes (architect, bench, ci, claude, deps-audit, dogfood, hooks, housekeep, release, review, skills, test-health, titan). Commits like feat(titan): that previously triggered an unintended minor bump now correctly resolve to patch.\n\n- The table row wording was clarified (addressing a prior review) to unambiguously state that bare feat: is always minor, while feat(scope): is filtered by the internal list.\n- claude scope is intentionally included as an internal tooling scope (also addressed in prior review).\n- A concrete example output for the new patch-from-internal-feat case is added at line 66.\n- The only remaining nit: the minor bump example output template (line 64) still says found \feat:` commits, which would be inaccurate if the triggering commits were feat(cfg):` style — a cosmetic inaccuracy in generated output that doesn't affect bump logic.

Confidence Score: 5/5

Safe to merge — the fix is correct, targeted, and prior review concerns are fully resolved.

Single-file documentation/instruction change with no code execution risk. The bump logic is sound: bare feat: → minor, feat(internal-scope): → patch, feat(user-facing-scope): → minor. Edge cases (breaking-change takes priority, mixed batches behave correctly) are all handled. Both prior review threads have been addressed. The only remaining item is a P2 style nit on the minor-bump example output string.

No files require special attention.

Important Files Changed

Filename Overview
.claude/skills/release/SKILL.md Adds internal-scope exclusion list to the version bump auto-detect logic, correctly routing feat(titan): etc. to patch instead of minor; previous ambiguity and missing claude scope concerns have been resolved.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Scan commits since last tag] --> B{Any BREAKING CHANGE\nor ! suffix?}
    B -- Yes --> MAJOR[Bump: MAJOR]
    B -- No --> C{Any feat commit?}
    C -- No --> PATCH[Bump: PATCH]
    C -- Yes --> D{feat with scope?}
    D -- No bare feat: --> MINOR[Bump: MINOR]
    D -- Yes feat scope: --> E{Scope in internal list?\narchitect, bench, ci, claude,\ndeps-audit, dogfood, hooks,\nhousekeep, release, review,\nskills, test-health, titan}
    E -- Yes internal scope --> PATCH
    E -- No user-facing scope --> MINOR
Loading

Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/release-ski..." | Re-trigger Greptile

|-----------|------|
| Any commit has a `BREAKING CHANGE:` or `BREAKING-CHANGE:` footer, **or** uses the `!` suffix (e.g. `feat!:`, `fix!:`, `refactor!:`) | **major** |
| Any commit uses `feat:` or `feat(scope):` | **minor** |
| Any commit uses `feat:` or `feat(scope):` **with a user-facing scope** | **minor** |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Qualifier ambiguity for bare feat: commits

The updated table cell reads: Any commit uses \feat:` or `feat(scope):` with a user-facing scope. The trailing qualifier can be read as applying to both alternatives — implying that even a bare feat:commit (no scope) needs a "user-facing scope" to trigger minor. The intent is clearly that barefeat:is always minor; onlyfeat(scope):` is filtered by the exclusion list.

A small reword prevents the AI from mis-reading this:

Suggested change
| Any commit uses `feat:` or `feat(scope):` **with a user-facing scope** | **minor** |
| Any `feat:` commit **or** any `feat(scope):` where the scope is **not** in the internal list below | **minor** |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Reworded the table row to: "Any feat: commit or any feat(scope): where the scope is not in the internal list below". This removes the ambiguity — bare feat: is clearly always minor.


**Internal scopes — treat as patch, not minor:** The following scopes represent internal developer tooling and infrastructure, not user-facing features. A `feat:` commit with one of these scopes counts as a **patch**, not a minor bump:

`titan`, `skills`, `hooks`, `claude`, `ci`, `bench`, `dogfood`, `release`, `housekeep`, `review`, `deps-audit`, `test-health`, `architect`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 claude scope missing from PR description

The added scope list includes claude (between hooks and ci), but the PR description's summary omits it. If claude is intentionally internal, the PR description should be corrected for traceability; if it was accidentally added, it should be removed from the skill.

Suggested change
`titan`, `skills`, `hooks`, `claude`, `ci`, `bench`, `dogfood`, `release`, `housekeep`, `review`, `deps-audit`, `test-health`, `architect`
`titan`, `skills`, `hooks`, `ci`, `bench`, `dogfood`, `release`, `housekeep`, `review`, `deps-audit`, `test-health`, `architect`

(Remove claude if it wasn't meant to be included, or leave as-is and update the PR description.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The \ scope is intentional — commits like \ are internal tooling, not user-facing features. I've kept it in the list and alphabetized all scopes for easier maintenance (addressing Claude bot's suggestion as well).

@carlos-alm
Copy link
Copy Markdown
Contributor Author

Addressed the outside-diff comment about missing patch-from-internal-feat example (lines 63-64). Added a second example:

Detected patch bump (all feat commits use internal scopes: titan, skills). Version: 3.4.0 → 3.4.1

Also addressed Claude bot's suggestion to alphabetize the scope list. All three Greptile P2 items + Claude's minor suggestion are now resolved in commit 4d9c3ac.

@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 3e5ad45 into main Mar 27, 2026
13 checks passed
@carlos-alm carlos-alm deleted the fix/release-skill-internal-scopes branch March 27, 2026 01:59
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2026
@carlos-alm
Copy link
Copy Markdown
Contributor Author

Addressed the remaining nit from Greptile's re-review: the minor-bump example on line 64 now reads \ instead of , which correctly covers both bare \ and scoped \ triggers. Fixed in 2f58d2d.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant