Date: 2026-09-28. Three reviewers (restore/snapshot logic, signals and lifecycle, M1 spec conformance), two skeptics per finding. 18 findings, all confirmed; the skeptics rated every one low except one missing test (medium/low). No data loss was found.
Raw results: ~/smartconfig-work/review-r3-results.json on the VM.
| Id | Change | Findings it closes |
|---|---|---|
| R4-1 | sc: rework signal handling. The first SIGINT/SIGTERM/SIGHUP/SIGQUIT asks the store to stop at the next safe point (no new work, no more COMMIT retries, never between a rename and its report); sc then exits 1 through its normal error path, so defers remove temp files and the message tells the truth (“interrupted (file not changed)” or “file restored but not recorded; previous content saved as ID”). A second signal forces the old sweep-and-exit. Signals inherited as ignored stay ignored. SIGQUIT no longer dumps a stack trace. |
logic 1, lifecycle 1-6 |
| R4-2 | tests for spec clauses that had none: setuid/setgid/sticky restore and the printed mode, the existed/did-not-exist guards and the retry limit in Restore, the id formula, -q when unchanged, “no previous file existed”, log column order and local time |
logic 2, spec 1, spec 4 |
| R4-3 | fsutil: “replaced while being read” is a retryable error; Snapshot and Restore retry it like a stamp change | logic 4 |
| R4-4 | MILESTONES.md: list every deliberate deviation and known limit that the m1 tag certifies (restore needs a readable current file under 8 MB; retries can add pre-restore rows; stamp granularity on coarse filesystems; logrotate and temp names; …) | spec 2, spec 3, logic 3, 5, 6 |
sc log prints intents and paths raw; a newline or tab in an intent breaks the table (spec 5). Later: escape control characters.sc help <unknown> exits 0 with usage text (spec 6). Later.| Lens | Finding | Reviewer | Skeptics |
|---|---|---|---|
| logic 1 | Ctrl-C or SIGTERM after the rename says only ‘interrupted’, but the file has already been replaced and no restore row is recorded | medium | low, low |
| logic 2 | No test covers the file being created or deleted between savePreRestore and commitRestore, or the retry limit in Restore; removing the data-loss guard passes every test | medium | medium, low |
| logic 3 | Restore now fails after 3 attempts (and adds a pre-restore row for each) when the file’s metadata changes during the pre-restore commit, even if the content is identical; round 2 succeeded | low | low, low |
| logic 4 | ‘was replaced while being read, try again’ is not retried by the new retry loops; a restore that runs next to another sc restore can now fail with it | low | low, low |
| logic 5 | The stamp check misses an in-place rewrite of the same size within the timestamp granularity (vfat, ext4 with 128-byte inodes); the overwritten content is never recorded and the older version is reported as ‘previous state’ | low | low, low |
| logic 6 | Each errChanged retry leaves a committed pre-restore row that no restore follows and that cannot be told apart in sc log | low | low, low |
| lifecycle 1 | An interrupt after the rename exits mid-COMMIT: file restored, no restore row, and the message only says ‘interrupted’ | medium | low, low |
| lifecycle 2 | RemovePending is a one-time sweep while the main goroutine keeps running, so temps created during or after it leak into the config directory | medium | low, low |
| lifecycle 3 | The signal goroutine and the main goroutine both finish, giving contradictory output and exit codes | low | low, low |
| lifecycle 4 | SIGHUP and SIGQUIT bypass the cleanup: an SSH drop leaves the temp copy in /etc, and Ctrl-\ prints a 218-line stack trace | low | low, low |
| lifecycle 5 | signal.Notify re-enables SIGINT that sc inherited as ignored, so background runs from scripts are now interruptible (regression) | low | low, low |
| lifecycle 6 | The signal test covers only the safe point, so none of findings 1-3 would be caught | low | low, low |
| spec 1 | No test covers setuid/setgid/sticky mode handling; mutants that silently drop these bits pass the whole suite | medium | low, low |
| spec 2 | Restore refuses to run when the current file is over 8 MB (or is a directory or FIFO): undocumented, and the error gives no way forward | low | low, low |
| spec 3 | MILESTONES.md M1 notes omit several deliberate deviations and known limits that the m1 tag would certify | low | low, low |
| spec 4 | Several spec output clauses have no test: id formula, -q on unchanged, ‘no previous file existed’ message, log column order and local time | low | low, low |
| spec 5 | sc log prints intents and paths raw: a newline or tab breaks the table and can show a fake row | low | low, low |
| spec 6 | Some errors lack operation context, and sc help <unknown> exits 0 with multi-line usage |
low | low, low |
make build (CGO_ENABLED=0 exported by the Makefile) gives ELF 64-bit LSB executable ... statically linked, and ldd says not a dynamic executable. go vet ./... and gofmt -l . print nothing. go test -count=1 ./... passes as user and as root. A plain CGO_ENABLED=1 go build is dynamically linked, but README.md:2…initialised <abs dir> (a relative SC_HOME is resolved). SC_HOME comes out 0700, objects/ 0700 and changes.db 0644 inside the 0700 dir, with umask 0002. A pre-existing 0755 SC_HOME is chmodded to 0700. Running init twice gives exit 0 both times. When SC_HOME is a regular file, init fails with one line.