Skip to content

fix(env): keep stored ciphertext when a secret value is unchanged - #340

Draft
roodboi wants to merge 7 commits into
nextfrom
wf/f7-stable-ciphertext-b16
Draft

roodboi wants to merge 7 commits into
nextfrom
wf/f7-stable-ciphertext-b16

Conversation

@roodboi

@roodboi roodboi commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Linear: HACK-1152 (WU18-P08, pattern HACK-P08)

Final behavior

  • setProjectEnvValue (the path behind hack env add / hack env set) keeps a secret entry's stored ciphertext when that ciphertext decrypts to exactly the requested value under the current key. Repeating hack env set KEY --secret --value-stdin with the same value leaves the env file byte-identical, reports No changes needed, and leaves git status clean.
  • A changed value, a plain entry becoming secret, or an entry that does not decrypt under the current key (malformed, or sealed with another key) gets new ciphertext with a fresh IV. Returning to an earlier value also gets new ciphertext; old ciphertext is never reused for a different current value.
  • Env layer writes (writeProjectEnvConfigFile, used by set, unset and migrate) commit by rename. writeTextFileAtomicIfChanged in src/lib/fs.ts creates an exclusive sibling (O_CREAT|O_EXCL|O_NOFOLLOW), writes and fsyncs it, then renames it over the target. If the process dies before the rename, the previous complete layer stays in place and still resolves. The helper keeps the file's permission bits, an existing symlink keeps its link (the write lands at the resolved target), and the staging file is removed on any failure the process sees.
  • Rework (verifier finding): a write killed before the rename can leave a .<layer>.<uuid>.tmp staging file that holds the whole layer. The managed .hack/.gitignore block now ignores .hack.env*.tmp, so an orphan from a --local write cannot be committed by git add -A, and an orphan from a tracked-layer write does not leave git status dirty. Existing projects pick up the entry on their next --local write, hack up, hack init or hack doctor --fix. hack doctor's generated-files check also flags a tracked staging file. Orphans are not swept on the next write, because a concurrent writer may still own one. This repository's own .hack/.gitignore block is refreshed.
  • The HACK-P08 card moves from deferred to partial. It now names the hydration command directly: hack env set <key> --local --value-stdin --secret, next to hack env explain <key> --json. To allow that, the argv-env-value banned-recipe scan in tests/helpers/pattern-recipes.ts skips flags and stops at an inline-code backtick. It still flags any form with two operands, including hack env set --local KEY value, a backtick command substitution as the value, and a value-taking flag such as --env dev. The missing contract is narrowed: each command writes one key, and there is no multi-key import.
  • docs/env.md documents the no-op, rotation and rename behavior. It now says the file stays byte-identical once Hack has written it, because a hand-edited layer is normalized on the first repeat. It also lists the rename edge cases: a dangling symlink becomes a regular file, and the directory must be writable even when the file is. The ignore-rules list includes the new entry.

Validation (local, macOS host, head ad65a7ea)

  • tests/project-env-stable-ciphertext.test.ts: 10 pass. New in this rework:
    • SIGKILL before the commit rename, for both the tracked and the --local layer. Each asserts that the old bytes are kept and resolve, that git status --porcelain is empty with the orphan present, and that git add -A stages nothing.
    • A symlinked layer keeps its link, the target gets the new ciphertext, and no staging file is left in either directory.
    • A failed commit rename (directory at the target) leaves no staging file.
  • tests/hack-gitignore.test.ts checks the ignore rule with real git check-ignore for tracked- and local-layer staging names. tests/doctor-generated-files.test.ts covers detection and untracking of a leaked staging file, with a clean status afterwards.
  • tests/pattern-catalogue.test.ts has a new allowed/flagged table for argv-env-value.
  • 21 files, 247 pass, 0 fail:
    • env: project-env-config, project-env-local-inheritance, project-env-metadata, project-env-worktree-key, env-value-stdin, env-value-stdin-cli, env-command-matrix, env-command-modern, env-key-rotation-model, env-key-argument-redaction-cli, private-env-file, env
    • patterns and agents: pattern-catalogue, pattern-examples, agent-instruction-source, skill-references
    • required gates and this PR: integration-ownership, cli-spawn-env-isolation, project-env-stable-ciphertext, hack-gitignore, doctor-generated-files
  • bun run generate:agent-plugins regenerated only plugins/hack/skills/hack-cli/references/patterns.md. No ownership fingerprint or instruction revision change was required.
  • bun run --cwd packages/cli typecheck: clean. bun run --cwd packages/cli check: 0 errors (34 pre-existing warnings, none in touched files). Biome on the 10 changed source/test files: clean. bun run privacy:check: ok. No CLI surface or Rust changes.
  • All secrets in the tests are synthetic strings. CLI and worker children use privateCliEnv. The key file lives in the temp project. Nothing touches Docker, the keychain, the real HOME or client config.

Negative controls (mutations; each reverted, tree verified clean)

  1. Always re-encrypt (the pre-fix behavior): the repeat no-op test and the CLI git-status test fail.
  2. Reuse any ciphertext that decrypts, without comparing the plaintext: the rotation, interrupted-write and CLI tests fail.
  3. Treat an entry that fails to decrypt as unchanged: the wrong-key test fails.
  4. Treat a plain entry equal to the new value as unchanged: the plain-to-secret test fails.
  5. Restore the non-atomic Bun.write path: the SIGKILL-before-commit tests fail.
  6. Drop the permission-bit preservation: the permission test fails.
  7. Drop realpath (verifier M7, previously survived): the symlink test fails.
  8. Drop the staging unlink on failure (verifier M8, previously survived): the staging-cleanup test fails.
  9. Remove .hack.env*.tmp from the managed entries: both kill tests, the check-ignore test and the doctor untrack test fail (4 fail).
  10. Remove the doctor staging pathspec: both doctor inspection tests fail.
  11. Restore the old argv-env-value regex: the rendered-catalogue scan and the new allowed/flagged table fail.

Remaining gaps

  • No multi-key hydration/import command exists. Atomicity is per key and per layer file, not across a scripted multi-key hydration. P08 stays partial (owner WU27/WU14).
  • Orphaned staging files are ignored, not removed. They stay on disk until someone deletes them. If a layer is a symlink to a file outside .hack/, its staging file lands next to that target, where the managed ignore does not apply.
  • A dangling symlink layer is replaced by a regular file, and a read-only directory makes the write fail with EACCES. Both are documented, not changed.
  • Same-plaintext detection decrypts the existing entry with the current key. It is not a constant-time comparison; this is acceptable for a local CLI that already holds the key.
  • Directory fsync after the rename is best effort. Durability across power loss on filesystems that refuse directory fsync is not qualified.
  • The legacy (non-modern) .env and secret-store env set paths are unchanged. The one-time legacy migration (buildMigratedValues) still re-encrypts.
  • Hosted CI on this exact head has not been confirmed. No Linear writes were made.

Rework 2

Base: remote head 52f0ffc9; pushed head 5d58e9b0 (includes the main session's fixture fix). New commit: fix(env): refuse to replace an env layer that cannot be read or written.

Fixed

  • Data loss on an unreadable layer (should-fix). writeTextFileAtomicIfChanged used to treat any read error as "file absent". hack env set on a layer it could not read (mode 000, or root-owned 0600 after sudo hack) then wrote a layer holding only the new key and renamed it over the old one. Now the helper checks the target before it stages anything. Only a missing path, or a symlink whose target is missing, counts as absent. An existing target that cannot be read, is not a regular file, or is not writable (access(W_OK)) is refused. So is one whose owner and group the staging file cannot take. The error is a closed AtomicWriteRefusedError: it gives the path and a fixed reason, never a value, and the file is left as it was. This brings back the guard the old in-place write got from the kernel.
  • Owner change on a replaced layer. A rewrite now keeps the file's owner and group as well as its mode. The staging file is fchowned to the old owner and group before the rename. If it cannot be, the write is refused. As a result, sudo hack env set keeps the user's ownership instead of leaving a root-owned layer.
  • Docs and comments (minor). docs/env.md now covers the refusals and says that rename breaks hard links. The checkTrackedGeneratedFiles docstring now lists the .hack/.hack.env*.tmp pathspec (and .hack/tickets).

Tests (tests/project-env-stable-ciphertext.test.ts, 16 pass)

New tests:

  • A chmod 000 layer holding another key is refused (unreadable), checked through the library and through the CLI (env set --secret --value-stdin exits non-zero). The bytes are kept and no staging file is left.
  • A chmod 0444 layer is refused (not_writable). The inode, mode and bytes are kept.
  • A rewrite keeps a non-default group. The test uses a secondary group of the test user and is a no-op when the user has none.
  • A dangling symlink layer is replaced by a regular file, as documented.
  • A directory at the target is refused (not_regular_file) before anything is staged.
  • The staging-cleanup test no longer uses a directory at the target, because that case is now refused before staging. It now runs a child (tests/fixtures/atomic-write-rename-failure-worker.ts) whose commit rename fails after staging. It asserts that the old bytes are kept and no staging file is left.

The permission refusals are skipped when running as root, because root ignores permission bits.

Mutations (each reverted; src/lib/fs.ts byte-compared with the good copy afterwards):

  1. Unreadable treated as absent (the round-1 behavior): both unreadable tests fail.
  2. access(W_OK) check removed: the read-only test fails.
  3. Ownership preservation removed: the group test fails.
  4. Dangling symlink refused: the dangling-symlink test fails.
  5. Staging unlink on failure removed: the rename-failure test fails.
  6. Regular-file check removed: the directory test fails.

Gates on the new head

  • gitleaks 8.30.1 --log-opts=origin/next..HEAD: no leaks (5 commits).
  • bun run --cwd packages/cli typecheck: clean. check: 0 errors. Biome on the changed files: clean.
  • Full bun run test (local macOS, heavily loaded host, after bun run build:config-compiler): 7203 pass, 126 skip, 35 fail. I reran each failing file serially with --timeout 60000:
    • These now pass, so their failures were load-only timeouts: agent-onboard-command, bun-crash-loop, e2e/run-dependency-cache, init-assistant, mcp-publication-readiness, native-candidate-authored-artifact, native-compose-adoption-job-worktrees, native-compose-routing-command, native-compose-storage-witness, native-env-values (72/72), native-network-project, native-project-branch-cli.
    • These still fail: native-authored-process-incarnation, native-authored-project-recovery, native-compose-adoption-routing-generation, native-compose-proxy-probe, native-compose-retained-storage-command, native-compose-storage-activation-command, native-compose-storage-fence-cost. None of them writes env layers. The full log has no AtomicWriteRefusedError. I ran five of these files with the pre-change src/lib/fs.ts from 52f0ffc9, and they failed the same way. So these are host-environment failures on this machine, not regressions. Hosted test passed on 52f0ffc9.
  • No Rust changes.

Still open

  • The owner_not_preserved refusal path cannot be reached by a non-root test user, so no test covers it. Covering it needs a root CI job or a mocked fchown.
  • The checks run before the rename and are not an atomic fence against a concurrent permission change.
  • The remaining gaps listed above still apply. These include orphaned staging files outside .hack/ for symlinked layers, the lack of multi-key atomicity, and best-effort directory fsync.

🤖 Generated with Claude Code

roodboi and others added 3 commits October 19, 2026 11:50
setProjectEnvValue now keeps an entry's existing ciphertext when it already
decrypts to the requested value under the current key, so repeating
`hack env set --secret --value-stdin` with the same value leaves the tracked
env file byte-identical and git status clean. A different value, a plain
entry becoming secret, or an entry that does not open under the current key
is re-encrypted with a fresh IV.

Env layer writes now commit by rename from an exclusive, fsynced sibling, so
a write interrupted before the commit leaves the previous layer intact and
resolvable. Existing permission bits and symlink targets are preserved.

HACK-P08 moves from deferred to partial: per-key no-op detection and atomic
layer writes exist; a multi-key hydration import remains missing.

Refs HACK-1152

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…vior

A `--local` env write killed before its commit rename left a
`.hack.env*.<uuid>.tmp` staging file that git did not ignore, so a later
`git add -A` could commit machine-local values, and a tracked-layer orphan
left the tree dirty. The managed `.hack/.gitignore` now ignores
`.hack.env*.tmp`, and `hack doctor` flags a tracked staging file.

Tests now cover symlink preservation, staging cleanup after a failed
rename, and clean git status after a killed tracked or local write. The
argv-env-value recipe skips flags, so the P08 card can name
`hack env set <key> --local --value-stdin --secret`. Docs describe the
first-repeat normalization of hand-edited files and the rename edge cases.

Refs HACK-1152

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs HACK-1152

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@blacksmith-sh

This comment has been minimized.

roodboi and others added 4 commits October 19, 2026 12:54
With stable ciphertext, setting an unchanged secret keeps the stored
ciphertext, so the shared cipher fixture returned identical values and the
ciphertext-only change tests no longer produced a change. Encrypt each
fixture value in its own store so every call is a fresh encryption.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rename-committed env write treated any read failure as an absent file,
so `hack env set` on a layer it could not read (mode 000, or root-owned
0600 from an earlier `sudo hack`) built a layer holding only the new key and
renamed it over the old one, dropping every other value. Rename also skips
the write-permission check an in-place write got, so a read-only or foreign
layer was silently replaced with a new owner.

writeTextFileAtomicIfChanged now classifies the target before staging:
only a missing path or a dangling symlink is absent. An existing target
that cannot be read, is not a regular file, is not writable, or whose owner
and group the staging file cannot take is refused with a closed,
value-free AtomicWriteRefusedError, and the file is left as it was. A
rewrite keeps the file's owner and group as well as its mode.

Document the refusals and that rename detaches hard links, and list the
staging pathspec in the doctor check docstring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve plugins/hack/skills/hack-cli/references/patterns.md by
regenerating it (bun run generate:agent-plugins) from the merged
src/agents/pattern-catalogue.ts, carrying this branch's pattern wording
and the changes merged in train b18.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve generated plugin conflicts (plugins/hack/skills/hack-cli/references/patterns.md ) by regenerating
them with bun run generate:agent-plugins from the merged canonical source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@blacksmith-sh

This comment has been minimized.

This branch has not been deployed

No deployments
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.

1 participant