Make publish.yml github-release job idempotent#13
Conversation
Two related problems addressed together per Option C in #12: 1. Not idempotent ----------------- The old github-release step ran ``gh release create`` unconditionally. If a Release for the tag already existed (e.g. pre-created with hand-written notes before the workflow ran), the step failed with HTTP 422 and the entire workflow was marked failed — even though the PyPI publish job had already succeeded. This is exactly what happened on the v0.5.0 publish (run 24263840766). The job now checks for an existing Release with ``gh release view`` and uses ``gh release edit`` to update it in place instead of failing. Create flow handles the no-existing-release case. Net: the job is a no-op in content terms when the release already exists with the same notes, and a silent content update when it exists with different notes. 2. Low-quality auto-generated notes ----------------------------------- The old step used ``--generate-notes``, which produces a plain commit list. That's significantly worse than hand-written CHANGELOG entries and created the incentive to pre-create releases manually, which in turn triggered problem #1. The new step reads the Release body from CHANGELOG.md directly. A small awk block extracts the section between the current ``## <ver>`` heading (skipped to avoid duplicating the title) and the next ``## `` heading. CHANGELOG.md is already the authoritative release narrative, so there's no duplication and no quality tradeoff. If the CHANGELOG has no matching entry (e.g. an emergency tag without doc prep), the step falls back to ``--generate-notes`` rather than creating an empty release body. A ::warning:: annotation surfaces the fallback in the workflow UI. Verification performed locally on CHANGELOG.md at HEAD: - awk extraction for v0.5.0 returns 34 lines of content starting with ``### Changed``, ending at the next ``## `` header. - Extraction for a nonexistent version produces an empty file, triggering the fallback path. - ``python3 -c 'import yaml; yaml.safe_load(...)'`` confirms the workflow file parses cleanly. The fix cannot be fully end-to-end tested without a tag push, so the next real release (or a manual workflow re-dispatch against an existing tag) will be the first full exercise of both the create and edit paths. The failure mode if something is wrong is loud (workflow red) and the PyPI publish step is independent and already succeeded before this step runs, so there's no risk to the actual package distribution. Closes #12 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Adding QA Active — beginning review. This is a CI workflow PR so verification will focus on: YAML validity, the awk extraction logic against the real CHANGELOG.md, the gh release create/edit idempotency paths, the env var injection-safety pattern, and the empty-file fallback to |
cmeans
left a comment
There was a problem hiding this comment.
QA Review — Round 1
Verdict: Ready for QA Signoff. Clean review — no substantive findings, three minor observations on the regex/convention dependencies that are dormant in practice.
Verification (fresh in this session, commit ca8bed5)
The interesting parts of this PR are testable in isolation, so I exercised them locally rather than waiting for the next real release.
Normal CI sanity (workflow change should not affect source-code CI):
uv run pytest:312 passed, 94 deselected, 7 warnings in 16.66suv run ruff check .: All checks passed!uv run mypy src/: Success: no issues found in 27 source files
YAML validity:
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/publish.yml'))"→ no error
awk extraction against the real CHANGELOG.md (the bit the PR description claims I should be able to verify locally):
For VERSION=0.5.0:
- Output: 34 lines, 3594 bytes
- First line:
### Changed(the## 0.5.0heading itself is correctly excluded by thenextafter the flag is set — confirms title-duplication avoidance works) - Last lines: the
### Fixedblock ending at the## 0.4.1boundary — clean cut, no bleed-through into the previous version's section - Line count and starting heading both match what the PR description claims
For a nonexistent VERSION=9.9.9:
- Output: 0 bytes
[ -s release_notes.md ]evaluates false →use_changelog=falseis set → workflow falls through togh release create/edit ... --generate-notes(or--title onlyon the edit branch)- Fallback path works as designed
Code review
Idempotency logic at lines 99-114 is the right shape: gh release view "$TAG" >/dev/null 2>&1 is the canonical existence check, then branch into edit-vs-create. The four-way matrix (exists × CHANGELOG-or-not) is handled distinctly:
- exists + CHANGELOG →
gh release edit $TAG --title $TAG --notes-file … - exists + no CHANGELOG →
gh release edit $TAG --title $TAG(preserves whatever notes are there; the comment at line 104 calls out that you can't regenerate auto-notes on edit) - new + CHANGELOG →
gh release create $TAG --title $TAG --notes-file … - new + no CHANGELOG →
gh release create $TAG --title $TAG --generate-notes
That's the correct decomposition.
Injection safety is correctly handled via env: blocks at lines 63-64 and 91-94 rather than ${{ … }} interpolation into the shell script. This matches the GitHub docs guidance the PR description cites — standard practice, but worth confirming it's actually applied at every interpolation point. It is.
sed -i '/./,$!d' release_notes.md at line 80 is the standard "strip leading blank lines" idiom. GNU sed (Ubuntu runners), so the bare -i works.
Observations (non-blocking)
1. $VERSION is inserted into the awk regex without escaping. Line 73:
$0 ~ "^## "version"( |\\()"The . characters in 0.5.0 are regex metacharacters (match any single char), so a CHANGELOG line ## 0X5X0 (date) would technically match for VERSION=0.5.0. Dormant in practice — version numbers are always literal dots and CHANGELOG entries are always literal dots, so dots match dots. But a future version scheme with pre-release suffixes (e.g., 0.5.1-rc1) or a typo in the CHANGELOG could surface latent issues. A gsub(/\./, "\\.", version) inside awk, or pre-escaping in shell, would close the loophole. Not a blocker.
2. The regex requires a space or ( after the version. ( |\() in the awk pattern means a future CHANGELOG entry written as just ## 0.6.0 (no date suffix, no trailing space) would not match. This relies on the existing convention ## X.Y.Z (date). Worth a one-line comment in the workflow noting the convention dependency, so a future contributor doesn't accidentally break extraction by changing CHANGELOG style. Not a blocker.
3. Substring-match edge case — verified safe. For VERSION=0.5.1 against a hypothetical ## 0.5.10 (date) line: after matching 0.5.1, the next character would need to be space or (, but ## 0.5.10 has 0 there, so the regex correctly doesn't match. Mentioning this as a positive — the ( |\() anchor specifically prevents this category of bug.
Untestable in this PR cycle (acknowledged)
End-to-end testing of the actual gh release create/edit paths against the real GitHub API requires a real tag push. The PR description acknowledges this. Mitigations are in place:
- The PyPI publish step (the actually-important one for users) is independent and runs first, so a
github-releasefailure on the next release would still leave PyPI in a correct state — same as the v0.5.0 incident. - The shell logic for extraction and fallback is fully exercised by my local tests above, so the only thing the next real release will be exercising for the first time is the
gh releaseAPI surface itself.
Test plan checkboxes (verifiable items only)
- CI green — confirmed: lint, typecheck, test 3.11/3.12/3.13, qa-approved, on-push all SUCCESS
- YAML lint passes —
yaml.safe_loadclean - Next release exercises the create path — out of QA scope, owner-verified post-merge
- Manual workflow re-run against existing v0.5.0 tag exercises the edit path — out of QA scope, optional owner verification
Summary
Targeted CI fix, well-scoped (one file, one job rewrite). The Option C choice — fix idempotency AND switch to CHANGELOG-derived notes — is the right call: it removes both the technical footgun (existing-tag 422) and the incentive to pre-create releases manually that triggered the original failure. Local verification of the awk logic, YAML validity, and the fallback path all pass. No substantive findings.
Applying Ready for QA Signoff as the final act. Awaiting maintainer for QA Approved + merge.
|
QA audit — Round 1 complete (clean) CI workflow PR. Verification focused on testable parts of the change rather than waiting for the next real tag push. Verification (fresh in this session, commit ca8bed5):
No substantive findings. Three minor observations on regex/convention dependencies (unescaped Applying Ready for QA Signoff as the final act. Awaiting maintainer for QA Approved + merge. |
Addresses Round 3 observation on PR #16: every PR should add a CHANGELOG entry, not defer it to release prep. PRs #13, #15, and this PR (#16) all merged or were under review without entries because the prior convention was "CHANGELOG updated only at release time." That convention is now retired. Adds an `## Unreleased` section at the top of CHANGELOG.md with three entries: - `### Fixed` — PR #13 (publish.yml github-release idempotency, closes #12). Captures the awk extraction approach, gh release view → edit fallback, --generate-notes fallback path, and the env-var hardening against shell injection. - `### Changed` — PR #15 (centralize version, closes #11). Captures scripts/sync-server-json.py, the new version-sync CI job, the scripts/ scope extension on lint/typecheck, and the documented release flow. - `### Internal` — PR #16 (Phase 1 of #14). Captures the 81% → 85% delta, the five files brought to 100%, the 336 → 392 test count, the new test classes/files, and the closure-extraction technique. The publish.yml github-release awk extractor matches `## <version>(\\s|\\()` — an `## Unreleased` section without parens or following whitespace is harmless: it just isn't picked as release notes when a release tag is pushed. Going forward every PR adds an entry to `## Unreleased` and the release flow renames it to `## <version> (<date>)` plus opens a fresh empty `## Unreleased` for the next cycle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…E.md Bundles the documentation update into PR #16 per maintainer request. Adds two new sections to CLAUDE.md > Common Tasks: 1. **Adding a CHANGELOG entry on every PR** — spells out the convention established in the previous Round 3 commit. Lists the section heading taxonomy (Added / Changed / Fixed / Internal / Documentation), the PR/issue reference format, and the rationale (PRs #13/#15/#16 were the trigger). 2. **Bumping the version for a release** updated to: - Step 4 now says "rename `## Unreleased` to `## <version> (<date>)` and add a fresh empty `## Unreleased` for the next cycle" instead of "update CHANGELOG.md with the new version section" - Trailing note documents that the publish.yml github-release awk extractor (`## <version>( |\\()`) walks past `## Unreleased` harmlessly during tag-push releases (the section won't be picked as release notes because it lacks the required space-or-paren suffix) Also adds a `### Documentation` entry to the Unreleased section in CHANGELOG.md for this CLAUDE.md change, demonstrating the convention in the same PR that establishes it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Phase 1 of #14: cover CLI and module registration code paths Raises total project coverage from 81% to 85% by closing the easy gaps identified in #14's Phase 1. Five files reach 100%, 56 new test cases. **Files brought to 100%:** - src/mcp_synology/cli/check.py 51% -> 100% (-20 lines) - src/mcp_synology/cli/main.py 56% -> 100% (-31 lines) - src/mcp_synology/cli/logging_.py 78% -> 100% (-4 lines) - src/mcp_synology/modules/system/__init__.py 23% -> 100% (-17 lines) - src/mcp_synology/modules/filestation/__init__.py 70% -> 100% (-40 lines) **TOTAL: 81% (463 missing) -> 85% (351 missing)** **Tests: 336 -> 392 (+56)** The issue's Phase 1 estimate of "~90%" was optimistic — cli/version.py alone (Phase 2 target) accounts for ~3.9% of the remaining gap. Hitting 85% in this phase is the realistic ceiling for what Phase 1 covers. **New tests in tests/core/test_cli.py (additions to existing file):** - TestCheckLogin (8 cases) — exercises cli/check.py:_check_login by injecting AsyncMock-based DsmClient and AuthManager. Covers happy path, SynologyError → exit 1, OSError → exit 1, RuntimeError on non-AppConfig, RuntimeError on missing connection, the verbose-flag branch of the check command, and the load_config validation-error path that exits 1. - TestMainGroupOptions (12 cases) — covers all four top-level option branches in cli/main.py: --check-update (with newer version for uv, pipx, and unknown installer; and the no-update path), --auto-upgrade enable/disable, --revert with flag-value form and explicit version, no-subcommand → help, version-change tracking that records previous, the auto-upgrade-on-non-serve trigger path (both fires-update and no-update branches), and the serve command's create_server + run(transport='stdio') path. - TestCliLogging (5 cases) — covers cli/logging_.py: --verbose forces DEBUG even with SYNOLOGY_LOG_LEVEL=warning, env var sets level when not verbose, default is INFO, _configure_logging adds a FileHandler for the configured log_file, and no FileHandler is added when log_file is None. Each verbose/env-var test snapshots and restores the root logger so basicConfig isn't no-opped by handlers from prior tests. **New file tests/modules/system/test_register.py (7 cases)** — tests modules/system/__init__.py:register() with all-allowed, only-system-info, only-resource-usage, and empty allowed-tools sets. Then invokes both registered tool functions via server._tool_manager._tools[name].fn and verifies the closure walks through manager.get_client() and the patched underlying domain function. **New file tests/modules/filestation/test_register.py (24 cases)** — same pattern for the much larger filestation module: - 6 register() shape tests: all tools, READ-only, WRITE-only, none, custom settings, default_download_dir tilde expansion - 14 tool invocation tests covering each registered tool body (one per read tool, one per write tool). For each, the underlying domain function (e.g. listing.list_shares) is monkeypatched to a sentinel AsyncMock, the tool function is pulled from the FastMCP tool manager and invoked with realistic kwargs, and we assert the closure forwarded the result through manager.with_update_notice - 4 download_file/upload_file path tests: explicit dest_folder, default dir from settings, no-dest error paths for both tools. For both transfer tools the closure-installed progress_callback is invoked to verify it forwards to ctx.report_progress The key technique for filestation is pulling `server._tool_manager._tools[name].fn` after register() and invoking it directly. This walks the closure body lines (`await manager.get_client()` + `manager.with_update_notice(...)`) that the prior `assert server is not None` tests left uncovered. No production code touched — all changes are in tests/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * QA Round 2: apply snapshot/restore pattern to remaining logging tests Addresses the single Round 1 observation on PR #16: the two test_configure_logging_* tests didn't snapshot/restore the root logger state, breaking the symmetry the three sibling _init_early_logging_* tests in the same class established. The previous test_configure_logging_with_log_file added a FileHandler pointing at tmp_path/test.log and only removed it manually at the end — which still left the root logger's level mutated from "debug", AND if the test failed before the cleanup block ran, the FileHandler would leak into subsequent tests with a stale baseFilename pointing into a torn-down tmp directory. Wrap both tests in the same _reset_root_logger / _restore_root_logger pattern as their siblings. The FileHandler.close() call is preserved (now inside the try block) because _restore_root_logger only detaches handlers — it doesn't close them, and leaving an open file handle past tmp_path teardown would leak the OS file descriptor. 392 tests still passing. Lint, format, mypy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * QA Round 3: add CHANGELOG entries for #13, #15, #16 Addresses Round 3 observation on PR #16: every PR should add a CHANGELOG entry, not defer it to release prep. PRs #13, #15, and this PR (#16) all merged or were under review without entries because the prior convention was "CHANGELOG updated only at release time." That convention is now retired. Adds an `## Unreleased` section at the top of CHANGELOG.md with three entries: - `### Fixed` — PR #13 (publish.yml github-release idempotency, closes #12). Captures the awk extraction approach, gh release view → edit fallback, --generate-notes fallback path, and the env-var hardening against shell injection. - `### Changed` — PR #15 (centralize version, closes #11). Captures scripts/sync-server-json.py, the new version-sync CI job, the scripts/ scope extension on lint/typecheck, and the documented release flow. - `### Internal` — PR #16 (Phase 1 of #14). Captures the 81% → 85% delta, the five files brought to 100%, the 336 → 392 test count, the new test classes/files, and the closure-extraction technique. The publish.yml github-release awk extractor matches `## <version>(\\s|\\()` — an `## Unreleased` section without parens or following whitespace is harmless: it just isn't picked as release notes when a release tag is pushed. Going forward every PR adds an entry to `## Unreleased` and the release flow renames it to `## <version> (<date>)` plus opens a fresh empty `## Unreleased` for the next cycle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * QA Round 3 (cont.): document the per-PR CHANGELOG convention in CLAUDE.md Bundles the documentation update into PR #16 per maintainer request. Adds two new sections to CLAUDE.md > Common Tasks: 1. **Adding a CHANGELOG entry on every PR** — spells out the convention established in the previous Round 3 commit. Lists the section heading taxonomy (Added / Changed / Fixed / Internal / Documentation), the PR/issue reference format, and the rationale (PRs #13/#15/#16 were the trigger). 2. **Bumping the version for a release** updated to: - Step 4 now says "rename `## Unreleased` to `## <version> (<date>)` and add a fresh empty `## Unreleased` for the next cycle" instead of "update CHANGELOG.md with the new version section" - Trailing note documents that the publish.yml github-release awk extractor (`## <version>( |\\()`) walks past `## Unreleased` harmlessly during tag-push releases (the section won't be picked as release notes because it lacks the required space-or-paren suffix) Also adds a `### Documentation` entry to the Unreleased section in CHANGELOG.md for this CLAUDE.md change, demonstrating the convention in the same PR that establishes it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * QA Round 4: collapse CHANGELOG categories to strict Keep a Changelog Addresses Round 4 substantive finding on PR #16: the Round 3 commit introduced `### Internal` and `### Documentation` as new CHANGELOG section headers, but the 0.5.0 entry deliberately transitioned to strict Keep a Changelog (Added/Changed/Fixed only). Worse, the Round 3 CLAUDE.md taxonomy section codified the violation as official convention. **CHANGELOG.md `## Unreleased` reorganized:** - PR #16 test coverage entry: `### Internal` -> `### Added` (precedent: 0.5.0 unit test coverage went under Added) - PR #16 CLAUDE.md doc entry: `### Documentation` -> `### Added` (precedent: 0.5.0 docs/error-codes.md went under Added) - Sections reordered Added/Changed/Fixed to match Keep a Changelog ordering and the 0.5.0 entry's own ordering - The CLAUDE.md doc entry's text updated to reference only the three valid categories instead of listing the now-removed Internal and Documentation headings **CLAUDE.md "Adding a CHANGELOG entry on every PR" section updated:** - Removed `### Internal` and `### Documentation` from the taxonomy - Now states explicitly: "strict Keep a Changelog categories — only three section headings are valid" - Added an explicit "do not invent new categories like ### Internal, ### Documentation, or ### Code Quality" line so future contributors (and future me) don't repeat the regression - Notes that 0.5.0 deliberately transitioned away from the older Conventional Commits-style taxonomy - Spells out the precedent: test coverage and doc additions both go under `### Added` No production code touched. Tests, lint, format, mypy all clean. 392 passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes #12 with Option C: the
github-releasejob inpublish.ymlis now idempotent AND reads its Release body fromCHANGELOG.mdinstead of using auto-generated notes.Problem recap
During the v0.5.0 publish (run 24263840766):
build(tests + wheel)publish-pypi(PyPI upload)github-releaseHTTP 422: Release.tag_name already existsThe workflow tried to
gh release createa Release that was already created manually (with hand-written notes), and failed with 422. This marked the whole workflow as failed even though everything important (PyPI) succeeded.The root cause was two-fold:
gh release createfails on existing tags.--generate-notesproduces low-quality output, which incentivized pre-creating releases manually — which triggered Rename package to mcp-synology, add transfer module and icons #1.Fix (Option C from the issue)
Both problems addressed together so neither recurs.
1. Extract release body from
CHANGELOG.mdA small awk block extracts the section between the current version's
## <version>heading and the next##heading:The heading itself is skipped (the Release title already says
v0.5.0, no need to duplicate). If extraction produces an empty file (e.g. emergency tag without CHANGELOG prep), the step falls back to--generate-noteswith a::warning::annotation so the workflow UI makes it visible.2. Idempotent create-or-update
Works whether a Release was pre-created or not. Content flows from
CHANGELOG.mdeither way.Verification
Tested the shell logic locally against the current
CHANGELOG.md:### Changed, ending cleanly at the next##header. First line is### Changed(not the version heading), confirming the title-duplication avoidance works.use_changelog=false, falls back to--generate-notes. No crash.python3 -c 'import yaml; yaml.safe_load(...)'passes.Full end-to-end testing requires a real tag push, so the first real exercise of both the create and edit paths will be the next release. Failure mode is loud (workflow red), and the PyPI publish step is independent and already succeeded before this job runs, so there's zero risk to actual package distribution.
Why environment variables instead of
${{ }}inrun:Both shell blocks use
env:to passTAG/USE_CHANGELOGinstead of interpolating${{ github.ref_name }}directly into the script. GitHub's docs recommend this for scripts because it avoids shell-injection surface if a tag name ever contains shell metacharacters. Standard practice, not specific to this fix.Files changed
.github/workflows/publish.yml—github-releasejob rewritten (51 insertions, 4 deletions)Test plan
ci.yml) should pass unchangedCloses #12
🤖 Generated with Claude Code