fix(setup): atomic write for interactive config file#71
Conversation
Closes #70. Cascades the atomic_write_text helper (added in PR #69) into cli/setup.py:178-183, the last remaining user-visible non-atomic write site in the project. Previously the interactive setup flow persisted the user-edited config via path.write_text(...), exposing the same torn-write window the runtime state files had: a Ctrl+C, OOM, or power loss between truncate and final write could leave a zero-byte or half-written <instance_id>.yaml that the next mcp-synology check/serve invocation would fail to parse. Drops the now-redundant _CONFIG_DIR.mkdir(parents=True, exist_ok=True) since atomic_write_text already creates parent dirs. Adds TestSetupAtomicConfigWrite::test_setup_writes_config_atomically_with_no_tmp_sibling which runs the full interactive setup flow against a not-pre-created _CONFIG_DIR, then asserts the dir was auto-created (proves the helper ran), no .tmp sibling lingers after a successful write, and the resulting YAML starts with the generated header and contains the entered host. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
QA Round 1 — One substantive finding
PR cleanly cascades #69's atomic_write_text helper into cli/setup.py, the last user-visible non-atomic write site. Issue #70's stated scope is fully covered (helper swap, redundant _CONFIG_DIR.mkdir removed, regression test asserts parent-dir auto-creation + no .tmp lingers). Repo-wide grep -rn write_text src/ confirms the PR body's claim that no production call sites remain outside core/fs.py's own helper internals.
The new test does what the issue's test plan asked for — and a bit more, asserting the resulting YAML has the expected header and contains the entered host. Good.
Verification
| Check | Result |
|---|---|
uv run pytest |
528 passed (+1), 94 deselected, 96.18% coverage |
uv run pytest tests/core/test_cli_setup.py::TestSetupAtomicConfigWrite -v |
1 passed |
uv run ruff check src/ tests/ scripts/ |
clean |
uv run ruff format --check src/ tests/ scripts/ |
72 files already formatted |
uv run mypy src/ scripts/ |
clean (30 files, strict-mode) |
grep -rn write_text src/ |
only the helper definition + tmp-write inside the helper; 3 production callers all route through atomic_write_text |
Required CI on 19caced1 |
13/13 green |
PR-body manual tests 1–6 all checked.
Findings
| ID | Finding |
|---|---|
| F1 (substantive) | Doc drift in src/mcp_synology/core/fs.py:3-5. The module docstring still reads Currently houses atomic_write_text, used by core/state.pyandcli/version.py to persist runtime state without a torn-write window. After this PR cli/setup.py is also a caller — the inventory is now wrong. Fix: either add `cli/setup.py` to the list, or generalize wording (e.g., used by all sites that persist runtime state and user config without a torn-write window). I prefer the latter — once a third site exists, the per-site inventory is liable to drift again on the next cascade. Per the project's doc-drift-is-substantive rule, blocking. Repo-wide grep for the same drift pattern (grep -rn "core/state.py.*cli/version.py" src/ docs/ README.md) confirms this is the only place — no cross-file fix-up needed. |
Verdict
QA Failed Round 1 on F1. The fix is a one-line docstring edit in the file you just modified; round 2 should be quick.
|
Applying QA Failed as the final act of round 1. F1 is a one-line docstring edit in |
Addresses QA F1 on PR #71. The module docstring listed `core/state.py` and `cli/version.py` as the only callers, which became wrong the moment this PR added `cli/setup.py` as a third caller. Generalizing to "every site that persists runtime state or user config" so the next cascade doesn't reintroduce the same drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 2 — F1 addressedGeneralized the ```python Before"""Filesystem utilities used across Currently houses After"""Filesystem utilities used across Currently houses Per the QA preference: drops the per-site inventory entirely so the next cascade (e.g., a fourth caller someday) won't redrift it. Confirmed via Verification
Ready for round 2. |
cmeans
left a comment
There was a problem hiding this comment.
QA Round 2 — PASS
F1 closed by 5bb8b04 with the generalized-wording option I recommended. New docstring:
"""Filesystem utilities used across `core/`.
Currently houses `atomic_write_text`, used by every site that persists
runtime state or user config without a torn-write window.
"""Future cascades (more callers added to the helper) won't reintroduce the same drift class. Good shape.
Verification (re-run on 5bb8b04)
| Check | Result |
|---|---|
uv run pytest |
528 passed, 94 deselected, 96.18% coverage |
uv run ruff check src/ tests/ scripts/ |
clean |
uv run ruff format --check src/ tests/ scripts/ |
72 files already formatted |
uv run mypy src/ scripts/ |
clean (30 files, strict-mode) |
Required CI on 5bb8b04 |
13/13 green |
Verdict
Ready for QA Signoff. F1 closed; #70 scope still satisfied; no new regressions introduced. Final maintainer call.
|
Applying Ready for QA Signoff as the final act of round 2. F1 closed ( |
## Summary Patch release rolling up the post-0.5.0 fixes. Five PRs land in this release: - **#69** — Atomic state-file writes (closes #36). New `core/fs.py::atomic_write_text` helper; `save_state` and `_save_global_state` route through it. Eliminates torn-write windows on `state.yaml` / `global.yaml`. - **#71** — Atomic write for `cli/setup.py` interactive config (closes #70). Last user-visible non-atomic write site cascaded onto the same helper. - **#72** — README per-installer and per-OS download-breakdown badges. Mirrors mcp-clipboard's layout; all 9 new badges link to `cmeans/pypi-winnow-downloads` (the dogfooded service). - **#73** — Recycle-bin status probed per share (closes #37). Lazy `ensure_recycle_status` helper; self-correct on observation; clear-on-reauth via new `AuthManager.add_on_reauth_callback` API. Fixes incorrect "Recycle bin is enabled" messaging on shares with `#recycle` disabled. - **#77** — Per-path serial for multi-path `getinfo` and `delete` (closes #68). **Critical: fixes a v0.5.0 silent-no-op regression on `delete_files` with multi-path arrays** — caller would see `[+] Deleted N item(s)` listing every path, but none were actually deleted on real DSM 7.x. Same root cause on `get_file_info` (single synthetic record with comma-joined `path`). Both tools now issue one DSM call per input path. ## Files touched (per CLAUDE.md release procedure) - `pyproject.toml`: 0.5.0 → 0.5.1 - `server.json`: synced via `scripts/sync-server-json.py` (top-level + `packages[0].version`) - `uv.lock`: refreshed via `uv lock` - `CHANGELOG.md`: `## Unreleased` renamed to `## 0.5.1 (2026-05-01)`; fresh empty `## Unreleased` added above for the next cycle ## QA ### Manual tests 1. - [x] `python scripts/sync-server-json.py --check` — reports "in sync (0.5.1)". 2. - [x] `uv run pytest` — 550 passed, 96.13% coverage. 3. - [x] `uv run ruff check src/ tests/ scripts/` — clean. 4. - [x] `uv run ruff format --check src/ tests/ scripts/` — clean. 5. - [x] `uv run mypy src/ scripts/` — clean. 6. - [x] CHANGELOG diff is exactly: `## Unreleased` → `## 0.5.1 (2026-05-01)`, plus a fresh empty `## Unreleased` heading inserted above. No content was moved or rewritten — every entry under `## 0.5.1` already lived under `## Unreleased` on `main`. 7. - [x] After merge: tag-push triggers `publish.yml`. The `awk` extractor on `## <version>( |\()` matches `## 0.5.1 (2026-05-01)` correctly and the new empty `## Unreleased` is harmlessly walked past. ### Verification I already ran | Check | Result | |---|---| | `python scripts/sync-server-json.py --check` | in sync (0.5.1) | | `uv run pytest` | 550 passed, 100 deselected, 96.13% coverage | | `uv run ruff check src/ tests/ scripts/` | clean | | `uv run ruff format --check src/ tests/ scripts/` | 72 files already formatted | | `uv run mypy src/ scripts/` | clean (30 files, strict-mode) | | `git diff --stat HEAD~1` | 4 files: CHANGELOG.md, pyproject.toml, server.json, uv.lock | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #70. Cascades the
atomic_write_texthelper (introduced in #69) intocli/setup.py:178-183, the last remaining user-visible non-atomic write site in the project. The interactive setup flow previously persisted the user-edited config viapath.write_text(...), exposing the same torn-write window the runtime state files had: a Ctrl+C, OOM, or power loss between truncate and final write would leave a zero-byte or half-written<instance_id>.yamlthat the nextmcp-synology check/serveinvocation would fail to parse.Practical risk is much lower than the runtime-state case (single small-payload write inside an interactive workflow), but the helper was already there ready to use and closing this site means there are no remaining
path.write_textcalls in the project that persist user/runtime data.Drops the now-redundant
_CONFIG_DIR.mkdir(parents=True, exist_ok=True)sinceatomic_write_textalready creates parent dirs.Diff at a glance
```python
Before
_CONFIG_DIR.mkdir(parents=True, exist_ok=True)
raw_yaml = yaml.dump(config_dict, default_flow_style=False, sort_keys=False)
header = "# Generated by mcp-synology setup\n"
config_path.write_text(header + raw_yaml, encoding="utf-8")
After
from mcp_synology.core.fs import atomic_write_text
raw_yaml = yaml.dump(config_dict, default_flow_style=False, sort_keys=False)
header = "# Generated by mcp-synology setup\n"
atomic_write_text(config_path, header + raw_yaml)
```
The function-local import follows the convention QA round 1 on #69 explicitly endorsed for
cli/version.py:_save_global_state(consistent with the file's existing local-import pattern; setup.py also locally-importspydantic.ValidationErrorandmcp_synology.core.config.format_validation_error).QA
Manual tests
uv run pytest tests/core/test_cli_setup.py::TestSetupAtomicConfigWrite -v— should pass.uv run pytest— full suite green at>=95%total coverage.uv run ruff check src/ tests/ scripts/— clean.uv run ruff format --check src/ tests/ scripts/— clean.uv run mypy src/ scripts/— strict-mode clean.cli/setup.pyno longer calls_CONFIG_DIR.mkdirorconfig_path.write_text. The only remainingwrite_textcalls in the codebase should be in test files. Quick grep:grep -rn "write_text" src/should return only the newatomic_write_textdefinition incore/fs.pyand no production call sites.Verification I already ran
uv run pytestTestSetupAtomicConfigWrite::test_setup_writes_config_atomically_with_no_tmp_sibling.tmplingering, header + host contentuv run ruff check src/ tests/ scripts/uv run ruff format --check src/ tests/ scripts/uv run mypy src/ scripts/Heads-up
Issue #70 itself was unfortunately filed under
cmeans(the maintainer) rather thancmeans-claude-dev[bot]due to a stale-token mistake during PR #69 round 1 — see the## Heads-updiscussion on PR #69 round 2 and the awareness proposal5b8594c8for the durable enforcement design that's awaiting maintainer review. The PR itself (this one) is bot-authored as expected.🤖 Generated with Claude Code