Skip to content

fix(release-tools): make verify:changelog version-bump gate fire for nested plugins#169

Open
orioltf wants to merge 5 commits into
developfrom
archon/task-fix-issue-163
Open

fix(release-tools): make verify:changelog version-bump gate fire for nested plugins#169
orioltf wants to merge 5 commits into
developfrom
archon/task-fix-issue-163

Conversation

@orioltf
Copy link
Copy Markdown
Member

@orioltf orioltf commented May 29, 2026

Summary

  • Root cause: git diff --name-only emits repo-root-relative paths (e.g. apps/claude-code/unic-pr-review/commands/review-pr.md) while GUARDED patterns expected plugin-relative paths (commands/review-pr.md). The prefix mismatch meant the version-bump gate was silently skipped for every nested plugin.
  • Fix: add --relative to the git diff invocation + strip the plugin prefix via git rev-parse --show-prefix as a belt-and-suspenders fallback (needed for the test shim and cross-platform safety).
  • Extract: new pure module scripts/lib/changelog-gate.mjs owns GUARDED, isBumpRequired, and evaluateBumpGate with no I/O, no process.exit — mirrors base-branch-resolver.mjs. verify-changelog.mjs stays the thin I/O wrapper.

Layer-1 structural checks and CI diff-base resolution are unchanged.

Files changed

File Change
scripts/lib/changelog-gate.mjs NEW — pure gate module
scripts/lib/changelog-gate.test.mjs NEW — 16 unit tests for pure functions
scripts/verify-changelog.mjs UPDATED — imports gate, adds --relative + prefix-strip, uses evaluateBumpGate
scripts/verify-changelog.test.mjs UPDATED — 3 E2E regression tests with root-relative paths via fake-git shim
package.json UPDATED — adds changelog-gate.test.mjs to test command

Test plan

  • pnpm --filter @unic/release-tools test — 46/46 pass (16 unit + 30 integration/regression)
  • pnpm ci:check — Biome + Prettier clean
  • Regression tests explicitly reproduce the original bug (root-relative paths → exit 1) and the symmetric fix (bumped + bullet → exit 0)
  • Existing Layer-1 structural check tests unchanged and passing

Closes #163

🤖 Generated with Claude Code

…nested plugins

Closes #163.

The version-bump gate in verify-changelog.mjs was silently skipping every
plugin nested under apps/claude-code/. git diff --name-only emits
repo-root-relative paths (e.g. apps/claude-code/unic-pr-review/commands/…)
while the GUARDED patterns expected plugin-relative paths (commands/…).
The prefix mismatch meant triggered was always false for nested plugins.

Two coordinated changes:

1. Fix: add --relative to the git diff invocation so real git strips the
   plugin prefix. Also resolve the prefix via git rev-parse --show-prefix
   and strip it in code as a belt-and-suspenders fallback (required for the
   test shim, which ignores --relative, and for cross-platform safety).

2. Extract: pull the Layer-2 decision into a new pure, injectable module
   scripts/lib/changelog-gate.mjs exposing GUARDED, isBumpRequired, and
   evaluateBumpGate. The module performs no process.exit, no console writes,
   no filesystem or git access — it takes data and returns a verdict,
   mirroring base-branch-resolver.mjs. verify-changelog.mjs stays the thin
   wrapper that owns I/O and exit codes.

Layer-1 structural checks and CI diff-base resolution are unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@orioltf
Copy link
Copy Markdown
Member Author

orioltf commented May 29, 2026

🔍 Comprehensive PR Review

PR: #169 — fix(release-tools): make verify:changelog version-bump gate fire for nested plugins
Reviewed by: 2 specialized agents (code-review, error-handling)
Date: 2026-05-29


Summary

The fix correctly addresses the root cause: git diff --name-only emits repo-root-relative paths that never matched plugin-relative GUARDED patterns. The solution — --relative flag on git diff plus a pluginPrefix fallback for test shims — is robust and well-tested. Gate logic extracted into changelog-gate.mjs (pure, side-effect-free) is the right architectural move.

Verdict: APPROVE

Severity Count
🔴 CRITICAL 0
🟠 HIGH 0
🟡 MEDIUM 1 (pre-existing)
🟢 LOW 6 (mostly pre-existing)

🟡 Medium Issues (Pre-existing — Needs Decision)

Silent base plugin.json parse failure can bypass version-unchanged check

📍 packages/release-tools/scripts/verify-changelog.mjs:130-138

When git show <base>:.claude-plugin/plugin.json exits 0 but returns malformed JSON, the catch {} silently sets baseVersion = ''. In evaluateBumpGate, headVersion === '' is always false, so the version-unchanged guard is skipped. Requires corrupt git output to trigger — extremely unlikely in practice.

Options: Fix now (bind catch (e) + console.error warning) | Create issue | Skip

View one-line fix
try {
    baseVersion = JSON.parse(basePluginRaw.stdout).version
} catch (e) {
    console.error(`verify:changelog: warn — could not parse base plugin.json: ${e.message}`)
    // intentional fallback: treat base as new plugin with no prior version
}

🟢 Low Issues

View 6 low-priority suggestions
Issue Location Agent Suggestion
Shebang in library module scripts/lib/changelog-gate.mjs:1 code-review Remove #!/usr/bin/env node — library files in lib/ don't have shebangs (see platform.mjs)
isBumpRequired called twice verify-changelog.mjs:112 + changelog-gate.mjs:33 code-review Leave as-is — early-exit preserves "don't read files unnecessarily" invariant
No test for baseVersion = '' (new plugin) changelog-gate.test.mjs code-review Add one test case for newly-introduced plugin scenario
Broad catch blocks discard error cause verify-changelog.mjs:34,123,144 error-handling Pre-existing; bind catch (e) and append e.message to fail() messages
showPrefix silent fallback verify-changelog.mjs:87-88 error-handling Intentional by design — no fix needed
Unreachable process.exit(1) without comment verify-changelog.mjs:146 error-handling Add // unreachable — satisfies TS to match line 125

✅ What's Good

  • Clean extraction: changelog-gate.mjs is a pure, side-effect-free module — exactly the right seam for unit tests. This is what made the bug possible before (gate logic tangled with I/O).
  • Dual-path robustness: --relative for real git + pluginPrefix stripping for test shims — each documented with an inline comment.
  • Strong regression tests: runVerifyE2E exercises the exact failure mode from the issue (shim emits repo-root-relative paths) with three distinct cases.
  • Windows path handling consistent: replace(/\\/g, '/') applied everywhere paths are compared.
  • Result pattern: evaluateBumpGate returns GateVerdict instead of throwing — clean design.
  • Full CLAUDE.md compliance: tabs, single quotes, no semicolons, // @ts-check, Node.js APIs only.

📋 Suggested Follow-up Issues

Issue Title Priority
"release-tools: bind error in verify-changelog catch blocks for better CI diagnostics" P3
"release-tools: add new-plugin (baseVersion='') test case to changelog-gate.test.mjs" P3

Reviewed by Archon comprehensive-pr-review workflow
Artifacts: .archon/workspaces/unic/unic-agents-plugins/artifacts/runs/02e1b6ef215b343685a3a2722598d6a0/review/

Fixed:
- Remove misleading shebang from changelog-gate.mjs (library, not CLI entry point)
- Bind error in all catch blocks to include e.message in fail() output for better CI diagnostics
- Emit console.error warning when base plugin.json parse fails instead of silently swallowing
- Add unreachable comment on process.exit(1) after Layer 2 CHANGELOG catch to match Layer 1 style

Tests added:
- evaluateBumpGate: 'treats empty baseVersion as newly introduced plugin (version check passes)'

Skipped:
- Double isBumpRequired call (intentional design — early-exit preserves don't-read-files-unnecessarily invariant)
- showPrefix silent fallback (intentional by design — --relative is primary mechanism)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@orioltf
Copy link
Copy Markdown
Member Author

orioltf commented May 29, 2026

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-fix-issue-163 (commit 31a392c)
Philosophy: Fix everything unless clearly a new concern


Fixes Applied (6 total)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 0
🟡 MEDIUM 1
🟢 LOW 5
View all fixes
  • Shebang removed from library module (scripts/lib/changelog-gate.mjs:1) — removed #!/usr/bin/env node; library files in lib/ don't carry shebangs (matches platform.mjs)
  • Broad catch binds error (Layer 1 CHANGELOG) (verify-changelog.mjs:34) — catch (e) + e.message in fail() output
  • Broad catch binds error (HEAD plugin.json) (verify-changelog.mjs:123) — catch (e) + e.message in fail() output
  • Silent base plugin.json parse failure (verify-changelog.mjs:133–137) — catch (e) + console.error warning instead of silent swallow (MEDIUM)
  • Broad catch binds error (Layer 2 CHANGELOG) (verify-changelog.mjs:144) — catch (e) + e.message in fail() output
  • Unreachable exit comment (verify-changelog.mjs:146) — added // unreachable — satisfies TS to match style at line 125

Tests Added

  • changelog-gate.test.mjstreats empty baseVersion as newly introduced plugin (version check passes) (pins the baseVersion = '' new-plugin edge case)

Skipped (2)

Finding Reason
isBumpRequired called twice on hot path Intentional design: early-exit preserves "don't read files unnecessarily" invariant; evaluateBumpGate stays self-contained. Reviewer recommended leaving as-is.
showPrefix failure silently disables fallback Intentional by design. --relative is primary; showPrefix strip is a test-shim fallback. Reviewer explicitly marked "no fix required".

Suggested Follow-up Issues

(none)


Validation

✅ Lint (0 errors) | ✅ Tests (47 passed, 0 failed)


Self-fix by Archon · aggressive mode · fixes pushed to archon/task-fix-issue-163

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes verify:changelog’s Layer-2 version-bump gate so it correctly triggers for plugins nested under paths like apps/claude-code/<plugin>/, where git diff --name-only previously returned repo-root-relative paths that didn’t match the gate’s plugin-relative guarded patterns.

Changes:

  • Normalize changed-file paths for nested plugins by using git diff --name-only --relative plus a git rev-parse --show-prefix-based prefix-strip fallback.
  • Extract Layer-2 bump-gate logic into a new pure module (scripts/lib/changelog-gate.mjs) and add focused unit tests.
  • Add end-to-end regression tests reproducing the original root-relative path bug and validating the fix.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/release-tools/scripts/verify-changelog.mjs Uses the extracted gate, adds --relative, and normalizes/strips prefixes so guarded-path detection works for nested plugins.
packages/release-tools/scripts/verify-changelog.test.mjs Adds E2E regression coverage for nested-plugin root-relative diff paths (issue #163).
packages/release-tools/scripts/lib/changelog-gate.mjs New pure module owning GUARDED, isBumpRequired, and evaluateBumpGate (no I/O / no process.exit).
packages/release-tools/scripts/lib/changelog-gate.test.mjs New unit tests covering guarded matching and bump-gate verdict logic.
packages/release-tools/package.json Updates test script to include the new unit test file.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/release-tools/scripts/lib/changelog-gate.mjs Outdated
orioltf and others added 2 commits May 29, 2026 11:10
…message

When a plugin is newly introduced the base commit lacks plugin.json, so
baseVersion is empty and the success message rendered "version  → <head>"
with an awkward double space. Substitute a "(new)" sentinel so the log
line stays meaningful. Strengthens the empty-baseVersion unit test to pin
the rendered message.

Addresses Copilot review comment on PR #169.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address findings from the multi-agent PR review:

- Warn when `git rev-parse --show-prefix` fails so a genuine git failure
  (which would also disable the prefix-strip fallback) is traceable in CI
  logs instead of silently relying on --relative. (silent-failure-hunter)
- Guard against a plugin.json missing/blank `version` field — fail() with
  an actionable message instead of crashing later with a raw TypeError
  stack trace in evaluateBumpGate. (silent-failure-hunter)
- Tighten the --relative / prefix-strip comments: --relative emits paths
  relative to cwd (the plugin dir), and the test shim ignores --relative
  entirely rather than "may not" strip. (comment-analyzer)
- Add Windows-backslash E2E regression test exercising the backslash
  normalisation the CI Windows matrix can't reach (shim emits POSIX
  separators regardless of host). (pr-test-analyzer)
- Add unit coverage for a guarded file mixed among unguarded files and an
  all-unguarded list. (pr-test-analyzer)
- Use a template literal for the show-prefix shim output. (code-reviewer)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[release-tools] Make verify:changelog version-bump gate fire for nested plugins

2 participants