[TimeToBuildBob]

Problem

ActivityWatch/activitywatch#1537 reports settings POSTs failing with PermissionError on Windows. The exact file permission/sharing cause still needs reporter diagnosis; this PR does not claim to fix it.

While reproducing the persistence path, an interrupted JSON write also corrupted the prior settings file and left memory advanced to an unsaved value. Failed settings writes currently surface as generic HTTP 500s.

Changes

• Serialize into a same-directory temporary file, flush/fsync and close it before atomic replacement. Clean up failed temporary writes; preserve existing mode and refuse an explicitly unwritable file.
• Commit the in-memory snapshot only after successful persistence. Serialize concurrent snapshot/write/commit operations under one instance lock so independent setting updates are not lost.
• Resolve managed-config symlinks before choosing the temporary directory/replacement target, preserving the link itself.
• Warn when an existing settings file is unwritable. Return HTTP 500 with an actionable message, configuration path and errno for settings OSErrors; never log the value being saved.
• Add the settings regressions to the existing Makefile test selection.

Verification

Test-first reproduction of partial serialization: old file became a JSON fragment and memory became the unsaved locale. Delegated initial tests: 9 failed / 5 characterization passes before implementation. Parent added and observed failing concurrent-update and symlink regressions, then repaired both.

• 139 selected server tests passed (the existing Makefile list plus settings tests; no external listener required).
• 16 settings regressions passed after formatting.
• make typecheck: 31 source files clean.
• Black/Ruff and diff whitespace checks clean on owned files.
• Read-only review found the symlink regression, now covered and fixed.

Limits

Linux offline tests simulate Windows-style replacement denial; native Windows behavior still needs CI/reporter verification. New settings files use the tempfile's private 0600 mode; existing modes are retained. Atomic replacement changes inode identity, so hard links/ownership across different service users are not preserved. No cross-process locking or parent-directory fsync/power-loss durability guarantee is added. The absolute local config path is intentionally included in the authenticated/loopback API diagnostic, as it already is in the local error logs. Read-only files remain read-only: no automatic chmod, deletion or ACL changes.

Related: ActivityWatch/activitywatch#1537. Keep that report open pending diagnosis; this hardening is not evidence that the reporter's permission issue is resolved.

[greptile-apps]

Confidence Score: 4/5

[Medium impact] The changes reach settings persistence and its POST workflow.

Fix the concurrency test’s timeout import before merging; it currently fails the Python 3.9 CI jobs.

Findings

1.  Expected timeout fails CI ▶

Summary

The PR preserves the previous settings file and memory when a save fails.

• Settings updates reach memory only after the file is safely saved.
• Settings save failures show callers where the error happened.

TimeToBuildBob explicitly deferred native Windows confirmation, cross-process locking, parent-directory fsync, and preservation of hard links or ownership across service users. TimeToBuildBob also stated that this PR does not resolve the original Windows permission report. These acknowledged limits are not findings.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Settings POST] --> B[Acquire lock and copy saved state]
    B --> C[Write, flush, and close temporary file]
    C --> D[Replace saved file]
    D --> E[Commit memory and release lock]
    C -->|Failure| F[Remove temporary file and keep previous state]
    D -->|Failure| F
    F --> G[Return HTTP 500 for an OSError]

Loading

_{Reviews (1) · Last reviewed commit: "fix(settings): preserve saved state on p..." · Reviewed by Greptile}

[TimeToBuildBob]

🤖 AI code review

Settings.save() and Settings.set() now serialize writes through an instance lock and persist through a temporary file followed by os.replace(). Settings.set() commits its new dictionary after persistence, while _write() resolves symlinks and copies existing file modes. The REST endpoint adds OSError diagnostics, and the Makefile includes the new settings tests.

Needs a look — P2 only

Confidence 4/5

1 finding · ⚠️ 1 P2 · 🔒 1 security

⚠️ P2 medium · 🔒 security — aw_server/settings.py:69

Creating the temporary file requires write permission on path.parent, whereas the previous open(self.config_file, "w") required only a writable existing file. For example, an existing settings file owned by the server user with mode 0600 inside a directory with mode 0555 can still be loaded and previously could be updated. NamedTemporaryFile now raises PermissionError before serialization, so every settings POST fails despite the settings file being writable. This also affects symlink targets in managed directories.

_{How this was verified: Checked aw_server/settings.py:59-84: the temporary file is created in the resolved target's parent directory, and the permission check at line 61 checks only the existing file.}

Files changed (4) — the diff as I read it

• Makefile — Adds tests/test_settings.py to the test target's pytest selection.
• aw_server/rest.py — Catches OSError from settings POSTs and returns HTTP 500 with a message, configuration path, and errno.
• aw_server/settings.py — Adds locked temporary-file persistence, symlink resolution, mode copying, startup warnings, and dictionary replacement after successful writes.
• tests/test_settings.py — Adds settings persistence tests covering failures, deletion, snapshots, permissions, REST responses, concurrent writes, and symlink preservation.

_{Reviewed 058d886212cf · panel: codex:gpt-6.1-sol · subscription engine · 63s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

Outage coverage: 1-pass, not merge evidence. Subscription review supplied findings while full consensus capacity was unavailable. The consensus merge gate remains unsatisfied.

The optional OpenRouter verifier was unavailable. Findings retain their original severity.

[TimeToBuildBob]

Convergence adjudication — maintainer decision

Recommendation: retain fail-closed atomic persistence and merge only if the maintainer accepts the writable-target-directory requirement below. No auto-merge or further review round requested.

• Fixed: no new code fixes in this session. Greptile's P1 timeout finding was already fixed by 058d886212cf85eeecee6935537a796bbe8ae93f: the concurrency test imports TimeoutError from concurrent.futures. Its inline thread is already answered and resolved; Greptile's 4/5 summary still refers to the older b827ab15 head.
• Remaining: our reviewer fingerprint 651db0e759a2 is a genuine P2 compatibility trade-off, not a security bypass or false positive. A writable existing settings file in a non-writable target directory can no longer be updated. I recommend accepting that restriction because both same-directory temporary creation and replacement require directory write permission; an in-place fallback would restore partial-write corruption. Do not chmod the file/directory automatically. Updated the existing inline reply (no duplicate reply), recorded accepted-tradeoff, and resolved only our bot thread. This records my recommendation, not maintainer acceptance or a code fix.
• Verified against: current head 058d886212cf85eeecee6935537a796bbe8ae93f, also covered by the newest AI review marker (058d886212cf, 2026-10-23). No commit postdates that review. Read the complete diff and review threads; all five threads are resolved, with no unanswered human comment.
• CI: all eight checks pass on this exact head: Linux/macOS/Windows tests, Python 3.9 typecheck, format, lint, CodeQL and Python analysis. No merge conflicts. This session used existing CI evidence; it did not rerun tests. The AI review was outage single-pass coverage, not a satisfied consensus merge gate.
• Domain risk: native Windows sharing/ACL behavior still warrants maintainer/reporter verification; managed symlink targets need writable parent directories. Failed to save categories after import, adding, editing or remove activitywatch#1537 remains open for the original permission diagnosis. Cross-process locking, parent-directory fsync and cross-user ownership/hard-link preservation remain outside this PR.
• Convergence: detector verdict converged means zero unresolved Greptile P1/P2 threads. Its separate round_convergence.stable_rounds is 0 (required 2, status new_blocking from the sole historical review); this is not evidence of two stable review rounds. No Greptile trigger was sent, and no perfect-score chase is needed. Stop here for maintainer judgment.