State files are written by several helpers, and the atomic write has no stated contract #767

New issue

Open

opened 2026-10-08 06:45:50 +00:00 by claude · 1 comment

claude commented

2026-10-08 06:45:50 +00:00

Member

Found during IW-707 (atomic card writes through core/adapters/AtomicFile.scala). All writes of state files should go through one crash-atomic helper, and its contract should be stated and tested.

dashboard/jvm/src/infrastructure/StateRepository.scala has its own temp-and-rename write of state.json; it could share one atomic-write helper with core/adapters/AtomicFile.scala.
ServerConfigRepository.scala:45, ProcessManager.scala:19 and ConfigRepository.scala:17 in core/adapters write with os.write.over, which is not crash-atomic; route them through AtomicFile.write.
ReviewStateMove.copyOut (.partial file and move) and FakeCommandEnv.failWritesPartway model a partial write that the live FileSystem.write no longer produces; remove the knob, the .partial step and its test together.
The FileSystem.write port doc does not state that a write resets the file mode and ownership and needs a writable directory, and no test pins that a symlink at the target is replaced, not followed — core/commands/CommandEnv.scala:107-111, core/test/AtomicFileTest.scala (IW-707 phase 1 review).
ReviewStateAdapter.writeFile and LiveFileSystem.write spell the same Failed to write $path: $e wrapper around AtomicFile.write, and AtomicFile.write gives Left(null) for an exception with no message — core/adapters/ReviewStateAdapter.scala:122, core/commands/LiveCommandEnv.scala:94, core/adapters/AtomicFile.scala:19.

Found during IW-707 (atomic card writes through `core/adapters/AtomicFile.scala`). All writes of state files should go through one crash-atomic helper, and its contract should be stated and tested. - [ ] `dashboard/jvm/src/infrastructure/StateRepository.scala` has its own temp-and-rename write of `state.json`; it could share one atomic-write helper with `core/adapters/AtomicFile.scala`. - [ ] `ServerConfigRepository.scala:45`, `ProcessManager.scala:19` and `ConfigRepository.scala:17` in `core/adapters` write with `os.write.over`, which is not crash-atomic; route them through `AtomicFile.write`. - [ ] `ReviewStateMove.copyOut` (`.partial` file and move) and `FakeCommandEnv.failWritesPartway` model a partial write that the live `FileSystem.write` no longer produces; remove the knob, the `.partial` step and its test together. - [ ] The `FileSystem.write` port doc does not state that a write resets the file mode and ownership and needs a writable directory, and no test pins that a symlink at the target is replaced, not followed — `core/commands/CommandEnv.scala:107-111`, `core/test/AtomicFileTest.scala` (IW-707 phase 1 review). - [ ] `ReviewStateAdapter.writeFile` and `LiveFileSystem.write` spell the same `Failed to write $path: $e` wrapper around `AtomicFile.write`, and `AtomicFile.write` gives `Left(null)` for an exception with no message — `core/adapters/ReviewStateAdapter.scala:122`, `core/commands/LiveCommandEnv.scala:94`, `core/adapters/AtomicFile.scala:19`.

mph referenced this issue from a commit

2026-10-08 06:47:42 +00:00

chore(IW-707): file follow-ups as #767 and on #762

claude referenced this issue

2026-10-08 06:47:56 +00:00

IW-707: Small fixes: core #768

claude commented

2026-10-11 06:11:48 +00:00

Author

Member

Intake: needs-input

1. May the IW-767 work delete the `.partial` step in the card move, its fake-disk knob and its test?

When a tracked review-state card moves out of the tree, the code writes the copy to a .partial file and then moves it into place. Since IW-707, every live write is already all-or-nothing, so this step adds nothing. A test proves it with a fake disk that writes only half a file, and the real disk can no longer do that. Items 1, 2, 4 and 5 of this issue need no decision; only item 3 deletes working code, and that needs your approval.

// A write that fails part way leaves a partial file; at the out-of-tree path it would read as a card

https://code.iterative.works/iterative-works/iw-cli/src/branch/main/core/commands/ReviewStateMove.scala#L39-L53
https://code.iterative.works/iterative-works/iw-cli/src/branch/main/core/test/ReviewStateMoveHarnessTest.scala#L109-L121

a) Delete all three together. One write path and one fewer fake knob. The move then relies only on the documented guarantee of the write port.
b) Keep them and drop item 3 from the issue. No code is deleted. A redundant step and a test of a failure the real disk cannot produce stay.

Recommended: a) — the step guards against a failure that the port contract now excludes, and the issue already plans to pin that contract with tests.

Answer in a comment on this issue, per number, for example "1: a", or in your own words; intake reads it on its next iteration.

Intake: needs-input ### 1. May the IW-767 work delete the `.partial` step in the card move, its fake-disk knob and its test? When a tracked review-state card moves out of the tree, the code writes the copy to a `.partial` file and then moves it into place. Since IW-707, every live write is already all-or-nothing, so this step adds nothing. A test proves it with a fake disk that writes only half a file, and the real disk can no longer do that. Items 1, 2, 4 and 5 of this issue need no decision; only item 3 deletes working code, and that needs your approval. > // A write that fails part way leaves a partial file; at the out-of-tree path it would read as a card https://code.iterative.works/iterative-works/iw-cli/src/branch/main/core/commands/ReviewStateMove.scala#L39-L53 https://code.iterative.works/iterative-works/iw-cli/src/branch/main/core/test/ReviewStateMoveHarnessTest.scala#L109-L121 - **a) Delete all three together.** One write path and one fewer fake knob. The move then relies only on the documented guarantee of the write port. - **b) Keep them and drop item 3 from the issue.** No code is deleted. A redundant step and a test of a failure the real disk cannot produce stay. **Recommended: a)** — the step guards against a failure that the port contract now excludes, and the issue already plans to pin that contract with tests. Answer in a comment on this issue, per number, for example "1: a", or in your own words; intake reads it on its next iteration.

claude added the

needs-input

label

2026-10-11 06:11:48 +00:00