Skip to content

Conversation

@seemewalkin
Copy link
Contributor

@seemewalkin seemewalkin commented Nov 24, 2025

Summary

Implements infrastructure for v2 branching strategy to support:

  • v1.x maintenance branch: Long-term support for v1 releases
  • RC (Release Candidate) versions: Pre-release testing workflow
  • Channel-based releases: Separate release channels (stable, beta, v1 maintenance)

Key Changes:

  • New classify-version.mjs script for semantic version parsing
  • RC version support in bump-versions.mjs
  • New CI workflow for v1.x branch testing
  • Enhanced release workflows with channel validation
  • Docker build support for v1 and beta release types
  • Release channel schema updated to include '1' for v1 maintenance

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@seemewalkin seemewalkin changed the title feat(ci): Add v2 branching strategy infrastructure ci: Add v2 branching strategy infrastructure Nov 24, 2025
@seemewalkin
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Nov 24, 2025

Claude finished @seemewalkin's task —— View job


PR Review: v2 Branching Strategy Infrastructure

Review Checklist:

  • Analyze new classify-version.mjs script
  • Review bump-versions.mjs changes for RC support
  • Check new CI workflow for v1.x branch
  • Examine enhanced release workflows
  • Review Docker build changes
  • Check schema updates for release channels
  • Verify adherence to project conventions
  • Provide comprehensive feedback

🔍 Code Review Findings

Overall Assessment: This PR successfully implements the v2 branching strategy infrastructure. The code is well-structured and follows good practices. Here are my detailed findings:

Strengths

1. New classify-version.mjs Script (classify-version.mjs:1-103)

  • Clean, well-structured semantic version classifier
  • Proper error handling for invalid versions and 'v' prefix rejection
  • Good separation of concerns with modular class design
  • Comprehensive prerelease type detection (rc, beta, dev, exp)
  • Dual output modes: GitHub Actions and JSON for local testing
  • Uses semver library correctly for robust parsing

2. Enhanced bump-versions.mjs (bump-versions.mjs:25-37, bump-versions.mjs:79-83)

  • Added proper RC version generation logic
  • Follows semantic versioning conventions correctly
  • Good integration with existing experimental version logic

3. v1.x CI Workflow (ci-v1.x.yml:1-52)

  • Well-structured workflow for v1.x branch testing
  • Appropriate node version matrix (20.x, 22.x, 24.3.x)
  • Proper failure notifications via Slack
  • Consistent with existing CI patterns

4. Release Channel Validation (release-push-to-channel.yml:42-66)

  • Excellent channel-version compatibility validation
  • Clear error messages with actionable hints
  • Proper invariants:
    • Channel '1' only for v1.x releases
    • No prereleases in 'stable' or '1' channels

⚠️ Issues & Suggestions

1. Minor Inconsistency in Docker Tag Generation
Location: release-push-to-channel.yml:97, release-push-to-channel.yml:137, release-push-to-channel.yml:189

MINOR="${{ needs.validate-inputs.outputs.major }}.${{ needs.validate-inputs.outputs.minor }}"

Issue: Uses major twice instead of major.minor. Should be:

MINOR="${{ needs.validate-inputs.outputs.major }}.${{ needs.validate-inputs.outputs.minor }}"

2. Potential Security Concern
Location: release-push-to-channel.yml:210

- continue-on-error: true
  run: curl -u docsWorkflows:${{ secrets.N8N_WEBHOOK_DOCS_PASSWORD }} --request GET

Issue: Using basic auth with username in plaintext. Consider using bearer token authentication or ensure the webhook endpoint properly validates the credentials.

3. Documentation Gap
The PR adds significant infrastructure but lacks inline documentation for complex validation logic. Consider adding comments to explain the channel compatibility rules.

Technical Implementation Quality

Docker Configuration (docker-config.mjs:55-64)

  • Proper release type mapping for v2 branching strategy
  • Comprehensive comment explaining release types
  • Good handling of the '1' channel for v1 maintenance

Schema Updates (generic.config.ts:5)

  • Correctly added '1' to release channel enum
  • Maintains backwards compatibility

Workflow Integration (docker-build-push.yml:23-25)

  • Properly includes new '1' release type in documentation
  • Consistent with existing patterns

🛡️ Security & Best Practices

Version Classification Security

  • Properly rejects 'v' prefixed versions to prevent confusion
  • Uses established semver library instead of custom parsing
  • Good input validation and error handling

CI/CD Security

  • Appropriate use of secrets and GitHub tokens
  • No hardcoded credentials in workflows
  • Proper timeout settings to prevent runaway jobs

📝 Minor Recommendations

  1. Add JSDoc comments to classify-version.mjs methods for better maintainability
  2. Consider adding unit tests for the version classification logic
  3. Add workflow validation to ensure v1.x branch exists before deploying CI workflow

🎯 Conclusion

This PR successfully implements the v2 branching strategy infrastructure with:

  • ✅ Solid architecture and code quality
  • ✅ Proper error handling and validation
  • ✅ Good separation of concerns
  • ✅ Comprehensive workflow coverage

The implementation is production-ready with only minor issues that don't block merge. The channel validation logic is particularly well-designed and will prevent common deployment mistakes.

Recommendation: APPROVE

@n8n-assistant n8n-assistant bot added the n8n team Authored by the n8n team label Nov 24, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

5 issues found across 10 files

Prompt for AI agents (all 5 issues)

Understand the root cause of the following 5 issues and fix them.


<file name=".github/scripts/classify-version.mjs">

<violation number="1" location=".github/scripts/classify-version.mjs:76">
The CLI entrypoint check hardcodes ``file://${process.argv[1]}``, so the script never runs on Windows or paths with spaces because the path string doesn’t equal the percent-encoded file URL; convert both sides via `pathToFileURL`/`fileURLToPath` so direct execution is detected cross‑platform.</violation>
</file>

<file name=".github/workflows/release-push-to-channel.yml">

<violation number="1" location=".github/workflows/release-push-to-channel.yml:39">
Install the `.github/scripts` dependencies (or vendor `semver`) before invoking `classify-version.mjs`; as written the new validation step fails immediately because `semver` isn’t available on the runner, blocking every release channel push.</violation>
</file>

<file name="packages/@n8n/config/src/configs/generic.config.ts">

<violation number="1" location="packages/@n8n/config/src/configs/generic.config.ts:5">
releaseChannel schema now accepts an unsupported `&#39;1&#39;` value that the rest of the stack cannot interpret, so misconfigurations slip past validation and break release-channel checks.</violation>
</file>

<file name=".github/workflows/ci-v1.x.yml">

<violation number="1" location=".github/workflows/ci-v1.x.yml:42">
Without a job-level `if: failure()` (or similar) this notification job is skipped whenever one of its required jobs fails, so Slack never receives the failure alert.</violation>

<violation number="2" location=".github/workflows/ci-v1.x.yml:46">
`if: failure()` only becomes true when a previous step in the same job fails; since this is the first step, the condition is never met and Slack is never invoked. Let the step run unconditionally or evaluate the upstream job results instead.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

import { Config, Env } from '../decorators';

const releaseChannelSchema = z.enum(['stable', 'beta', 'nightly', 'dev']);
const releaseChannelSchema = z.enum(['stable', 'beta', 'nightly', 'dev', '1']);
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Nov 24, 2025

Choose a reason for hiding this comment

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

releaseChannel schema now accepts an unsupported '1' value that the rest of the stack cannot interpret, so misconfigurations slip past validation and break release-channel checks.

Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/generic.config.ts at line 5:

<comment>releaseChannel schema now accepts an unsupported `&#39;1&#39;` value that the rest of the stack cannot interpret, so misconfigurations slip past validation and break release-channel checks.</comment>

<file context>
@@ -2,7 +2,7 @@ import { z } from &#39;zod&#39;;
 import { Config, Env } from &#39;../decorators&#39;;
 
-const releaseChannelSchema = z.enum([&#39;stable&#39;, &#39;beta&#39;, &#39;nightly&#39;, &#39;dev&#39;]);
+const releaseChannelSchema = z.enum([&#39;stable&#39;, &#39;beta&#39;, &#39;nightly&#39;, &#39;dev&#39;, &#39;1&#39;]);
 type ReleaseChannel = z.infer&lt;typeof releaseChannelSchema&gt;;
 
</file context>
Suggested change
const releaseChannelSchema = z.enum(['stable', 'beta', 'nightly', 'dev', '1']);
const releaseChannelSchema = z.enum(['stable', 'beta', 'nightly', 'dev']);
Fix with Cubic

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@bundlemon
Copy link

bundlemon bot commented Nov 24, 2025

BundleMon

Unchanged files (2)
Status Path Size Limits
WASM Dependencies
tree-sitter-bash.wasm
181.26KB -
WASM Dependencies
tree-sitter.wasm
74.47KB -

No change in files bundle size

Groups updated (2)
Status Path Size Limits
**/*.js
11.38MB (+94.32KB +0.82%) -
**/*.css
231.53KB (+10.76KB +4.87%) -

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@currents-bot
Copy link

currents-bot bot commented Nov 24, 2025

E2E Tests: n8n tests passed after 9m 5.2s

🟢 588 · 🔴 0 · ⚪️ 12 · 🟣 1

View Run Details

Run Details

  • Project: n8n

  • Groups: 2

  • Framework: Playwright

  • Run Status: Passed

  • Commit: 25f1d41

  • Spec files: 96

  • Overall tests: 600

  • Duration: 9m 5.2s

  • Parallelization: 9

Groups

GroupId Results Spec Files Progress
ui 🟢 539 · 🔴 0 · ⚪️ 12 · 🟣 1 90 / 90
ui:isolated 🟢 49 · 🔴 0 · ⚪️ 0 6 / 6


This message was posted automatically by currents.dev | Integration Settings

@seemewalkin
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Nov 24, 2025

Claude finished @seemewalkin's task —— View job


PR Status Analysis

Todo List:

  • Analyze current PR state and remaining issues
  • Review cubic-dev-ai feedback status
  • Provide recommendations for next steps

📋 Current Status Summary

This PR implements v2 branching strategy infrastructure and has received comprehensive testing and review. Here's the current state:

Resolved Issues

Most of the issues identified by cubic-dev-ai have been addressed:

  • CLI entrypoint cross-platform compatibility - Fixed in commit d1d86c6
  • Slack notification workflow logic - Fixed in commit d1d86c6
  • Missing semver dependency - Fixed in commit 16d7103

⚠️ Outstanding Issue

One critical issue remains unaddressed: The releaseChannel schema validation

Location: packages/@n8n/config/src/configs/generic.config.ts:5

Issue: The schema now accepts '1' as a valid release channel:

const releaseChannelSchema = z.enum(['stable', 'beta', 'nightly', 'dev', '1']);

Problem: While the CI workflows properly handle the '1' channel for v1 maintenance releases, this value bypasses the application's runtime validation in other parts of the codebase that expect only the standard channels.

🔍 Analysis

After reviewing the codebase, I found that:

  1. CI workflows correctly validate the '1' channel (lines 54-59 in release-push-to-channel.yml)
  2. Docker configuration supports the '1' release type (docker-config.mjs:55-64)
  3. However, the config schema change may allow misconfigurations to pass validation elsewhere in the application

💡 Recommendation

The issue appears to be legitimate. The '1' channel is specifically designed for CI/CD workflows but shouldn't be a runtime configuration option for the application itself.

Would you like me to:

  1. Fix the schema issue by reverting the generic config change and handling the '1' channel only in CI contexts?
  2. Leave as-is if this was intentional for your v2 branching strategy?
  3. Investigate further to understand the intended behavior?

📊 Overall Assessment

The PR is very close to being ready - just this one schema validation concern remains. All tests are passing, and the implementation is solid.


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

Labels

n8n team Authored by the n8n team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants