refactor(cli): atomic hook-salt publish via temp file + hard_link

[amondnet]

Summary

Replaces first-run machine-salt persistence (O_CREAT|O_EXCL + separate write, bridged by a bounded read-retry loop) with a fully-written temp file linked into place via hard_link.

hard_link is a single atomic syscall that fails EEXIST if the target exists — this keeps the single-winner property AND makes the salt file appear only once fully written.

Changes

• Closes the create→write crash micro-window: a winner that died between the exclusive create and its write left a 0-byte file, forcing a loser onto its own unpersisted salt for one invocation. That state is now structurally unreachable, and the read-retry loop is removed (a loser reads the winner's complete bytes in one shot).
• Removed now-dead read_valid_salt_retrying() and create_secret_file_exclusive(); added publish_secret_atomically() + a per-process TEMP_SEQ atomic (temp names unique per process via PID, per thread via the counter).

Test Plan

• 22/22 honmoon-cli tests pass, including two new tests: first_run_loser_adopts_complete_winner_salt (deterministic loser path, no empty-file window) and machine_salt_concurrent_first_run_converges_to_one_key (16-thread convergence + temp cleanup)
• cargo clippy --all-targets -- -D warnings clean
• cargo fmt --check clean

Related Issues

Closes #28

---

Summary by cubic

Make first-run machine-salt publish in honmoon-cli atomic by writing to a random-name, 0600 temp created with O_CREAT|O_EXCL and linking it into place with hard_link. This removes the empty-file race, blocks temp pre-planting, and makes concurrent hooks converge on one salt (closes #28).

• Bug Fixes

  • Eliminates the create→write crash window; losers adopt the winner’s complete salt in one read, so the retry loop is gone.
  • Cleans up the temp on write failure and only warns on non-NotFound temp-cleanup errors.
  • Logs rare lost-race anomalies separately (short vs unreadable winner) for better diagnostics.

• Refactors

  • Adds publish_secret_atomically() and hex_encode(); reintroduces create_secret_file_exclusive(); removes read_valid_salt_retrying().
  • Uses unpredictable temp names; adds tests for loser fallback (including short-winner case) and 16-thread convergence; documents the machine-salt security model and the residual TOCTOU risk in attacker-writable dirs.

^{Written for commit d2e2ce2. Summary will update on new commits.}

[gemini-code-assist]

Code Review

This pull request replaces the exclusive file creation and retry-read loop for generating machine salt with an atomic publish mechanism using a temporary file and hard links, alongside adding concurrent race tests. Feedback suggests avoiding the temporary file and hard link pattern because it can fail on certain filesystems (like NFS or Docker) and may lead to convergence issues, recommending instead to stick to exclusive file creation directly on the target file with a retry-read mechanism for losers.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/honmoon-cli/src/hook.rs	87.50%	12 Missing ⚠️

📢 Thoughts on this report? Let us know!

[codspeed-hq]

Merging this PR will degrade performance by 24.81%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 15 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	parse_k8s_path[/api/v1/namespaces/prod/secrets/db-password]	5.8 µs	9.1 µs	-36.4%
❌	parse_k8s_path[/apis/apps/v1/namespaces/staging/deployments/api]	6.2 µs	7 µs	-11.1%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing amondnet/hook-salt-atomic-publish (d2e2ce2) with main (84e340c)}

[cubic-dev-ai]

All reported issues were addressed across 3 files

Architecture diagram

sequenceDiagram
    participant Hook as Hook Process
    participant FS as Filesystem
    participant Dir as .honmoon dir
    participant Salt as hook-salt file
    participant Temp as hook-salt.tmp.<nonce>

    Note over Hook,Temp: First-run machine salt publish flow

    Hook->>Hook: Generate 32-byte HMAC salt<br/>(session_salt payload)

    alt No hook-salt exists (first run)

        Hook->>Hook: publish_secret_atomically(dir, path, salt)

        Note over Hook,Temp: Step 1 — Write to exclusive temp<br/>(O_CREAT|O_EXCL, mode 0600)

        Hook->>Dir: Create temp with random 16-byte nonce
        activate Temp
        Hook->>FS: create_secret_file_exclusive(tmp, salt)
        FS-->>Temp: Write salt bytes, close file
        deactivate Temp

        Note over Hook,Temp: Step 2 — Atomic publish via hard_link

        Hook->>FS: hard_link(tmp, path)
        alt Link succeeds (we are the winner)
            FS-->>Salt: Salt now at path (atomic, already complete)
            FS-->>Hook: Ok(())
            Hook->>FS: remove_file(tmp) — cleanup
            Hook-->>Hook: Return our own salt
        else Link fails with AlreadyExists (loser race)
            FS-->>Hook: EEXIST
            Hook->>FS: remove_file(tmp) — cleanup our temp
            Note over Hook,Salt: Single read — no retry loop needed<br/>(winner's salt is already fully written)
            Hook->>FS: read(path)
            alt Read succeeds, >= 16 bytes
                FS-->>Hook: Winner's complete salt
                Hook-->>Hook: Adopt winner's salt
            else Read succeeds but short/corrupt
                FS-->>Hook: Short bytes
                Note over Hook: LOG: anomaly — winner salt short/corrupt
                Hook-->>Hook: Fall back to our own salt
            else Read fails with error
                FS-->>Hook: IO error
                Note over Hook: LOG: unexpected error reading winner salt
                Hook-->>Hook: Fall back to our own salt
            end
        else Link fails with other error
            FS-->>Hook: Permissions/IO error
            Hook-->>Hook: Propagate error upward
        end
    else hook-salt exists (subsequent runs)
        Hook->>FS: read(path)
        alt Read succeeds, >= 16 bytes
            FS-->>Hook: Persisted salt
            Hook-->>Hook: Use persisted salt
        else File short/corrupt or missing
            Note over Hook: LOG: regenerating<br/>(self-heal path)
            Hook->>Hook: Generate new salt
            Hook->>Hook: publish_secret_atomically() — same atomic flow
        end
    end

    Note over Hook: Salt used as HMAC-SHA256 key<br/>for placeholder unforgeability<br/>(session_salt)

Loading

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[greptile-apps]

Greptile Summary

This PR replaces the old first-run machine-salt publish (exclusive create → write → bounded retry loop) with a temp-file + hard_link pattern in publish_secret_atomically. The target file now only ever becomes visible once fully written, which closes the empty-file race window and removes the need for the read-retry loop.

• Atomic publish via hard_link: the temp is exclusively created at an unguessable random name (16 bytes from /dev/urandom), fully written, then linked into place with a single syscall that fails EEXIST if the target already exists — exactly one winner, and every loser reads the complete winner bytes in one shot.
• Previous review findings addressed: temp-file leak on write_all failure is now cleaned up before returning; the misleading "may linger" warning on NotFound is suppressed.
• Three new tests: deterministic loser adoption, short-winner fallback, and 16-thread convergence with temp-cleanup verification.

Confidence Score: 5/5

The change is safe to merge — it tightens the first-run concurrency story without altering any existing interface or data format.

The core logic is mechanically sound: hard_link atomically fails EEXIST when the target exists, the temp is written exclusively before linking so no reader ever sees an empty target, and the loser/fallback paths are exercised by three new tests. Both issues flagged in the previous review round are correctly fixed. The residual TOCTOU in attacker-writable directories is documented and acknowledged as out of the standard threat model.

No files require special attention.

Important Files Changed

Filename	Overview
crates/honmoon-cli/src/hook.rs	Replaces retry-loop atomic publish with temp-file + hard_link; adds publish_secret_atomically, hex_encode, and three new tests covering loser adoption, short-winner fallback, and 16-thread convergence; previous review findings are correctly addressed.
.claude/agent-memory/review-review-security-reviewer/hook-salt-security-model.md	New security-model knowledge document describing the 0600 invariant, the TOCTOU residual risk in attacker-writable directories, and the fail-open fallback; documentation only.
.claude/agent-memory/review-review-security-reviewer/MEMORY.md	Adds a one-line index entry for the new hook-salt-security-model document.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant P1 as Process A (winner)
    participant P2 as Process B (loser)
    participant FS as Filesystem

    P1->>FS: random_bytes(16) → nonce_A
    P2->>FS: random_bytes(16) → nonce_B
    P1->>FS: create_new(hook-salt.tmp.nonce_A, 0600)
    P2->>FS: create_new(hook-salt.tmp.nonce_B, 0600)
    P1->>FS: write_all(salt_A)
    P2->>FS: write_all(salt_B)
    P1->>FS: hard_link(tmp_A → hook-salt)
    FS-->>P1: Ok(()) — winner
    P2->>FS: hard_link(tmp_B → hook-salt)
    FS-->>P2: Err(AlreadyExists) — loser
    P1->>FS: remove_file(tmp_A)
    P2->>FS: remove_file(tmp_B)
    P2->>FS: read(hook-salt)
    FS-->>P2: salt_A (32 bytes, complete)
    Note over P1,P2: Both return salt_A — converged

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant P1 as Process A (winner)
    participant P2 as Process B (loser)
    participant FS as Filesystem

    P1->>FS: random_bytes(16) → nonce_A
    P2->>FS: random_bytes(16) → nonce_B
    P1->>FS: create_new(hook-salt.tmp.nonce_A, 0600)
    P2->>FS: create_new(hook-salt.tmp.nonce_B, 0600)
    P1->>FS: write_all(salt_A)
    P2->>FS: write_all(salt_B)
    P1->>FS: hard_link(tmp_A → hook-salt)
    FS-->>P1: Ok(()) — winner
    P2->>FS: hard_link(tmp_B → hook-salt)
    FS-->>P2: Err(AlreadyExists) — loser
    P1->>FS: remove_file(tmp_A)
    P2->>FS: remove_file(tmp_B)
    P2->>FS: read(hook-salt)
    FS-->>P2: salt_A (32 bytes, complete)
    Note over P1,P2: Both return salt_A — converged

Loading

_{Reviews (3): Last reviewed commit: "chore(agent-memory): scope salt TOCTOU t..." | Re-trigger Greptile}

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud