Skip to content

fix(credentials): write atomically through symlinks - #685

Merged
justrach merged 1 commit into
mainfrom
fix/405-symlink-policy-scratch
Aug 30, 2026
Merged

fix(credentials): write atomically through symlinks#685
justrach merged 1 commit into
mainfrom
fix/405-symlink-policy-scratch

Conversation

@yxlyx

@yxlyx yxlyx commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

What changed

  • Resolve the destination's final symlink chain before staging an atomic replacement, preserving the symlink while replacing its referent.
  • Anchor relative targets to each link's directory without lexically collapsing ..; retain correct absolute, dangling, chained, and Windows drive/UNC-rooted semantics.
  • Preserve existing target permissions, reject symlink cycles, and keep the original whole-file rename guarantee.
  • Add focused coverage for open-inode atomicity, relative and absolute chains, dangling targets, symlinked-parent .. traversal, cycles, permissions, and Windows volume roots.

Why

Problem / failure mode

createFileAtomic(..., replace = true) renames over the named directory entry. When that entry is a symlink such as settings.json -> real.json, it replaces the symlink itself and leaves real.json stale. Credential readers following the link then continue to observe old tokens even though the save reported success.

Reason for this approach

Resolving only the final symlink chain before staging lets the existing atomic writer place its temporary file beside the actual referent and rename there. Readers still see either the complete old referent or the complete new referent, while the user's symlink remains intact. Centralizing this in credential_store.replaceFile applies the policy consistently to interactive login, silent refresh/subagent recovery, key storage, MCP OAuth/cache, fallback settings, and model/router/pricing caches used by the TUI runtime.

Constraints and trade-offs

This is issue #405 item 1 only. It intentionally does not add the separate keys_cli live-platform coverage or directory fsync requested by items 2–3. Like other portable path-based filesystem operations, symlink resolution is a snapshot and is not linearizable against another process concurrently retargeting links; Zig 0.16 exposes no portable atomic follow-final-link-and-rename primitive. The existing crash/process-concurrency whole-file guarantee remains unchanged.

Rejected alternatives

  • A single readLink followed by createFileAtomic (the approach in PR fix: stream foreground bash and close the small follow-up batch #511) misanchors relative targets, misses chains, and does not define dangling, cycle, .., or Windows volume-rooted behavior.
  • Truncating through the symlink would naturally follow it, but gives up the atomic whole-file replacement that protects refresh tokens from crashes and torn concurrent reads.

Verification

  • zig build test: 1,758 passed, 1 skipped (1,759 total)
  • zig build tui-test: 459/459 passed
  • pre-push tier 1: green, including all 17 TUI PTY probes, reachability, invariants, SDK drift, formatting, and the 600-line ceiling
  • zig build -Dtarget=x86_64-linux: passed
  • zig build -Dtarget=x86_64-windows: passed
  • focused #405 filter: 54/54 passed

Addresses #405 item 1 only; items 2–3 remain open.

Atomic rename replaces a destination symlink itself, leaving the credential
referent stale. Resolve final symlink chains before staging so relative,
absolute, dangling, and Windows volume-rooted targets keep write-through
semantics while preserving whole-file replacement, permissions, and cycle
failure behavior.

Co-Authored-By: Codegraff <blackfloofie@codegraff.com>
@justrach
justrach merged commit 72e9a00 into main Aug 30, 2026
6 checks passed
@justrach
justrach deleted the fix/405-symlink-policy-scratch branch August 30, 2026 04:29
cursor Bot pushed a commit that referenced this pull request Aug 30, 2026
Lands cursor/eval-frontier-4ffc on the 281 cut: OpenCode --dir / dsh
harnesses, in-house fixtures, frontier SVGs, list-price plot, hardlink
graff-pinned + detached learn init, scripted-REPL usage footer, and the
#685 atomic credential write already on main. Conflicts in
graff-evals/README.md and harnesses.json kept both Pi-on-SuperGrok and
the new in-house / OpenCode / dsh entries.

Do not tag. Do not merge to main from this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants