Replace Reverse-Sync Version Bumping with Minor-Bump-on-Promote Strategy#517
Conversation
- Remove tests validating old reverse-sync behavior (force-push, protection manipulation, patch-increment-logic, patch-does-not-overflow-minor) - Rename test_integration_version_compute_step_exists → test_integration_read_version_step_exists (new step name) - Add tests for minor version bump on main, integration reset to patch .1, no force-push, no branch protection API calls - Add constant PATCH_BUMP_INTEGRATION_WORKFLOW and new TestPatchBumpIntegrationWorkflow class with 14 tests
Replace the reverse-sync approach (force-push integration to main + patch bump) with a clean two-step minor bump: - Compute new_main = MAJOR.$((MINOR+1)).0 and new_int = MAJOR.$((MINOR+1)).1 - Commit and push new_main to main - Checkout integration, commit and push new_int to integration No force-push, no branch protection manipulation.
New workflow that triggers on every PR merge to integration and bumps the patch version (MAJOR.MINOR.$((PATCH+1))). Replaces the patch-bump responsibility previously embedded in version-bump.yml's reverse-sync logic.
Version management is now exclusively the responsibility of CI workflows: - patch-bump-integration.yml bumps patch on every integration PR merge - version-bump.yml bumps minor on promotion PR merge to main Remove the threshold-based minor bump logic from the skill (Phase 8), renumber Phase 9 (PR Creation) → Phase 8, update sub-section numbers, and remove version_bumped/new_version from the output token block.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
| - name: Compute integration patch version | ||
| id: integration_version | ||
| - name: Read integration current version | ||
| id: int_version |
There was a problem hiding this comment.
[warning] cohesion: Step ID asymmetry: the main-branch step has ID 'version' and owns both new_main and new_int outputs, while the integration-branch step has ID 'int_version' and only owns 'old'. The new_int output (integration's new version) should ideally be computed and owned by the int_version step, not the main-branch version step.
There was a problem hiding this comment.
Investigated — this is intentional. new_int (X.(MINOR+1).1) is derived from main's MINOR, computed in the 'version' step (lines 29-43). The 'int_version' step (lines 91-99) is purposely a reader-only step whose sole output is old — integration's current version for the replacement operation. Computing new_int in 'int_version' would require it to depend on MINOR from 'version', creating a derivation dependency without architectural benefit. The asymmetry is intentional: 'version' is the single source of truth for all MINOR+1-derived values.
| echo "Nothing to commit" | ||
| else | ||
| git commit -m "chore: bump integration version to ${{ steps.integration_version.outputs.new }}" | ||
| git commit -m "chore: bump integration version to ${{ steps.version.outputs.new_int }}" |
There was a problem hiding this comment.
[warning] cohesion: Commit message for integration branch uses ${{ steps.version.outputs.new_int }} — the main-branch step ID 'version' owns the integration version output. This cross-ownership is confusing: integration's new version should be computed in the int_version step and referenced as steps.int_version.outputs.new.
There was a problem hiding this comment.
Investigated — this is intentional. ${{ steps.version.outputs.new_int }} is the correct reference because new_int is owned by the 'version' step (it derives from main's MINOR+1 computation). The integration commit message correctly references the value it is applying. Moving new_int computation to 'int_version' is architecturally unsound — see reply on the line-92 comment for full rationale.
| run_block = step.get("run", "") | ||
| assert "$((PATCH + 1))" in run_block | ||
|
|
||
| def test_patch_increment_does_not_overflow_minor(self): |
There was a problem hiding this comment.
[warning] tests: test_patch_increment_does_not_overflow_minor has no assertion that step is not None before calling step.get('run', ''). If the 'Compute new patch version' step is renamed, the test silently passes on an empty string rather than failing with a meaningful error.
There was a problem hiding this comment.
Investigated — this is a false positive. Line 314 already has assert step is not None before step.get('run', '') is called at line 315. If 'Compute new patch version' is renamed, the assertion at line 314 fails immediately with an AssertionError rather than silently passing on an empty string. No change needed.
| - name: Compute integration patch version | ||
| id: integration_version | ||
| - name: Read integration current version | ||
| id: int_version |
There was a problem hiding this comment.
[warning] cohesion: Step ID asymmetry: the main-branch step has ID 'version' and owns both new_main and new_int outputs, while int_version only owns 'old'. The new_int output (integration's new version) should be computed in the int_version step to keep ownership clear.
There was a problem hiding this comment.
Investigated — this is intentional. new_int (X.(MINOR+1).1) is derived from main's MINOR, computed in the 'version' step (lines 29-43). The 'int_version' step (lines 91-99) is purposely a reader-only step whose sole output is old — integration's current version for the replacement operation. Computing new_int in 'int_version' would require it to depend on MINOR from 'version', creating a derivation dependency without architectural benefit. The asymmetry is intentional: 'version' is the single source of truth for all MINOR+1-derived values.
| echo "Nothing to commit" | ||
| else | ||
| git commit -m "chore: bump integration version to ${{ steps.integration_version.outputs.new }}" | ||
| git commit -m "chore: bump integration version to ${{ steps.version.outputs.new_int }}" |
There was a problem hiding this comment.
[warning] cohesion: Commit message references ${{ steps.version.outputs.new_int }} — the main-branch step ID 'version' owns the integration version output. Integration's new version should be computed and owned by the int_version step (e.g., steps.int_version.outputs.new) for clear cross-branch ownership.
There was a problem hiding this comment.
Investigated — this is intentional. ${{ steps.version.outputs.new_int }} is the correct reference because new_int is owned by the 'version' step (it derives from main's MINOR+1 computation). The integration commit message correctly references the value it is applying. Moving new_int computation to 'int_version' is architecturally unsound — see reply on the line-92 comment for full rationale.
| run_block = step.get("run", "") | ||
| assert "$((PATCH + 1))" in run_block | ||
|
|
||
| def test_patch_increment_does_not_overflow_minor(self): |
There was a problem hiding this comment.
[warning] tests: test_patch_increment_does_not_overflow_minor has no None guard on the step lookup. If 'Compute new patch version' is renamed, step.get('run', '') executes on None and silently passes on an empty string. Add: assert step is not None, 'step not found'.
There was a problem hiding this comment.
Investigated — this is a false positive. Line 314 already has assert step is not None before step.get('run', '') is called at line 315. If 'Compute new patch version' is renamed, the assertion at line 314 fails immediately with an AssertionError rather than silently passing on an empty string. No change needed.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review found 9 blocking issues. See inline comments.
Verdict: changes_requested — 9 warning-level findings across 3 files.
Summary by dimension:
- defense (3): Version string not validated before arithmetic; integration version mismatch risk when branches diverge; missing early-exit guard on int_version read step.
- cohesion (2): Main-branch 'version' step owns both
new_mainandnew_intoutputs — integration's new version should be computed by theint_versionstep for clear ownership. - tests (4): Three tests in TestPatchBumpIntegrationWorkflow use weak raw-text string checks (pyproject.toml, plugin.json, uv lock) instead of structured YAML assertions; test_patch_increment_does_not_overflow_minor missing None guard on step lookup.
…patch-bump-integration.yml
… to use parsed YAML
Summary
Replace the current reverse-sync version bumping strategy (which patches main then force-pushes to integration then patches integration again) with a clean two-trigger system:
patch-bump-integration.yml(new): patches integration's version when any PR merges to integration.version-bump.yml(rewritten): when the promotion PR (integration → main) merges, bumps the minor version on both branches — main getsX.(Y+1).0, integration getsX.(Y+1).1. No force-push. No branch-protection manipulation.Additionally, Phase 8 of the
promote-to-mainskill (the threshold-based minor-bump commit made by the skill) is removed entirely. Version management is now the exclusive responsibility of CI workflows.Requirements
Version Invariants
Patch Automation
Promotion Automation
Cleanup
Architecture Impact
Development Diagram
%%{init: {'flowchart': {'nodeSpacing': 50, 'rankSpacing': 60, 'curve': 'basis'}}}%% flowchart TB %% CLASS DEFINITIONS %% classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff; classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; %% TRIGGERS %% T_INT([PR merged to integration]) T_MAIN([PR merged to main<br/>from integration]) subgraph CI ["CI / CD WORKFLOWS"] direction TB PATCH["★ patch-bump-integration.yml<br/>━━━━━━━━━━<br/>Trigger: PR closed → integration<br/>Guard: merged == true<br/>Action: PATCH+1 on integration"] VB["● version-bump.yml<br/>━━━━━━━━━━<br/>Trigger: PR closed → main<br/>Guard: merged==true, head==integration<br/>Action: MINOR+1 on main+integration"] TESTS["tests.yml<br/>━━━━━━━━━━<br/>task test-all<br/>pytest -n 4"] RELEASE["release.yml<br/>━━━━━━━━━━<br/>stable → git tag<br/>gh release create"] end subgraph BUILD ["BUILD TOOLING"] direction LR TASK["Taskfile.yml<br/>━━━━━━━━━━<br/>test-all, test-check<br/>sync-plugin-version"] UV["uv@0.9.21<br/>━━━━━━━━━━<br/>Package manager<br/>uv lock"] end subgraph QUALITY ["CODE QUALITY GATES"] direction LR RUFF_FMT["ruff format<br/>━━━━━━━━━━<br/>Auto-fix formatter"] RUFF_LINT["ruff check<br/>━━━━━━━━━━<br/>Linter + auto-fix"] MYPY["mypy<br/>━━━━━━━━━━<br/>Type checking src/"] end subgraph TESTING ["TEST INFRASTRUCTURE"] direction TB TEST_WF["● test_release_workflows.py<br/>━━━━━━━━━━<br/>TestVersionBumpWorkflow (18)<br/>TestPatchBumpIntegrationWorkflow (13)<br/>TestReleaseWorkflow (12)"] PYTEST["pytest + xdist<br/>━━━━━━━━━━<br/>-n 4 parallel<br/>asyncio + httpx"] end subgraph VERSION_STATE ["VERSION STATE AFTER CI"] direction LR INT_STATE["integration = X.Y.N<br/>━━━━━━━━━━<br/>patch monotonically ↑"] MAIN_STATE["main = X.(Y+1).0<br/>integration = X.(Y+1).1<br/>━━━━━━━━━━<br/>post-promotion"] end %% FLOW %% T_INT --> PATCH T_MAIN --> VB PATCH --> INT_STATE VB --> MAIN_STATE TASK --> PYTEST PYTEST --> TEST_WF TESTS --> TASK RUFF_FMT --> RUFF_LINT --> MYPY %% CLASS ASSIGNMENTS %% class T_INT,T_MAIN terminal; class PATCH newComponent; class VB,TEST_WF handler; class TESTS,RELEASE,TASK phase; class UV output; class RUFF_FMT,RUFF_LINT,MYPY detector; class PYTEST handler; class INT_STATE,MAIN_STATE stateNode;Color Legend:
patch-bump-integration.ymlworkflowversion-bump.ymlandtest_release_workflows.pyCloses #512
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260326-162457-967149/.autoskillit/temp/make-plan/replace_reverse_sync_version_bumping_with_minor_bump_on_promote_plan_2026-03-26_000000.md🤖 Generated with Claude Code via AutoSkillit