feat: add SOPS secret add/delete with darwin-rebuild verification and commit - #655
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Scott McMaster (scottmcmaster)
changed the title
🎨 Storybook previewUpdated for db16ba5
|
📋 PR Overview
🔬 Coverage
|
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Hidden (current) | Clean surface | No signal of planned features | Low |
| Disabled with tooltip | Intent communicated | Slightly busier surface | Low |
15. When multiple Nix files contain 'sops.secrets', the fallback search fails with an error that does not name the conflicting files.
The error reads 'Found multiple Nix modules containing sops.secrets; expected modules/darwin/sops-secrets.nix'. It tells the user what was expected but not which files caused the conflict. A user with a home-manager options file that also references sops.secrets cannot self-diagnose without searching the repository manually. The backend already collects the candidate list; reporting those paths in the error costs one line.
Since the last review
- Still open: Delete button is active for agenix secrets; backend rejects only after the user confirms the dialog (secret-detail-view.tsx:
disabled={capability === "unavailable" || isDeleting}still leaves the button active for agenix secrets; secrets_management.rs returns the error string only after confirmation.) - Still open: Delete uses a plain confirmation dialog with no indication that a darwin-rebuild check will run (AlertDialogDescription in secret-detail-view.tsx describes the operation but adds no phase label or note about a running build check.)
- Still open: Delete shows only a spinner in the button with no phase label during the darwin-rebuild verification step (secret-detail-view.tsx: the button label transitions to 'Deleting…' with a Loader2 spinner; no status line outside the button reflects the running build phase.)
- Still open: Clean repository prerequisite fires at confirm time, not when the detail view or delete dialog opens (secrets_management.rs:
status.clean_headcheck is insidedelete_sops_secret, which runs after the user confirms the dialog.) - Still open: Delete confirmation dialog names no specific files that will change (AlertDialogDescription: 'This removes the encrypted value and Nix declaration, verifies the configuration, and commits the change.' — no file paths are named.)
- Still open: Secret name is normalized to a lowercase slug with no record of the original name in the configuration (secrets_management.rs:
declare_sops_secretcallsapply_semantic_editwith sopsFile and key attrs only; no Nix comment preserving the original name is added.) - Still open: All SOPS secrets share one fixed encrypted file with no warning for users with a non-standard layout (secrets_management.rs:
MANAGED_SOPS_FILEis hardcoded to 'secrets/secrets.yaml'; no user-facing warning is emitted when this file is absent.) - Still open: SOPS encryption target recipients are not reported in the add result (shared_types/secrets_management.rs:
AddSecretResultfields are secret_id, encrypted_file, declaration_file, runtime_path, and commit_hash — no recipients or matched creation rule field.) - Addressed in code: Recipients section header implies these specific identities will receive access, in tension with the clarifying note (add-secret-view.tsx: checkboxes removed entirely; clarifying note 'Encryption uses the recipients in the matching .sops.yaml creation rule.' added directly below the 'Recipients — who can decrypt' heading.)
- Still open: Edit value and Rotate & re-key are hidden instead of shown in a disabled state (secret-detail-view.tsx:
canEdit = falseandcanRotate = falsestill render these buttons only when the flags are true, so they remain invisible.) - Still open: Multiple sops.secrets files error does not name the conflicting files (secrets_management.rs: error reads 'Found multiple Nix modules containing sops.secrets; expected modules/darwin/sops-secrets.nix' — candidate paths are collected but not included in the message.)
- Still open: No committed milestone for agenix delete support; active button affects real sessions indefinitely (secrets_management.rs returns 'Deleting agenix secrets is not yet implemented'; no date, milestone, or UI indicator is added in this PR.)
- Still open: Vault model may expose only global recipients, not per-file creation rule recipients (add-secret-view.tsx still derives displayed recipients from
vault.recipients.filter((r) => r.inUse); no per-rule filtering is added.) - Still open: Error message for multiple sops.secrets files does not list the conflicting file paths (secrets_management.rs: the multi-candidate error formats only the expected standard path string, not the list of found candidate paths.)
Open questions
-
Does the team have a committed milestone for agenix add and delete support? The 'Agenix support will be added later' note and the backend error for delete both carry no date. If the timeline is open-ended, the active delete button on agenix secrets affects real sessions until that milestone ships.
-
Does the vault model expose per-file .sops.yaml creation rule recipients, or only the global list of all vault identities? If it exposes only the global list, the recipients section in the add form may show identities that SOPS will not use for the secrets/secrets.yaml creation rule.
-
When a user with two Nix modules containing 'sops.secrets' tries to add or delete a secret, does the error message name the conflicting files? The backend collects the candidates but the current message does not list them.
Recommendation
Ship with changes
The core SOPS add and delete flows are end-to-end: they encrypt, declare, verify, commit, and restore on failure. One change blocks shipment: the delete button is active for agenix secrets and the backend rejects the operation only after the user completes a destructive confirmation. Disabling the delete button when the secret backend is agenix (matching the pattern the add form already uses) removes the only disagree verdict. The remaining agree-with-concerns items are real gaps but they do not break the primary SOPS use case.
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
The Delete button is rendered unconditionally for all secrets regardless of secret.decryptionCapability, but delete_sops_secret on the backend calls decrypt_sops_file internally — it must decryp...
apps/native/src/components/widget/secrets/secret-detail-view.tsx:248
1 finding(s) posted as inline comments.
There was a problem hiding this comment.
Pull request overview
This PR wires the previously mocked SOPS "add secret" and "delete secret" flows to real Rust backend implementations, replacing frontend simulations. On the backend it encrypts/decrypts the shared secrets/secrets.yaml via sops, declares/removes the secret in the nix-darwin module through the semantic Nix AST editor, verifies with darwin-rebuild dry-run, and commits only the two managed files. It also extracts reusable path helpers and a multi-file commit helper, and refactors ensure_secret to share them.
Changes:
- New
secrets::secrets_management::add_secret/delete_secret(SOPS only) with clean-repo precondition, dry-build verification, post-edit vault re-verification, and best-effort rollback; exposed via newsecrets.addSecret/secrets.deleteSecretoRPC procedures and sharedAddSecretResult/DeleteSecretResulttypes. - Reusable helpers:
git::commit_files(selective multi-file commit),file_opspath helpers (repo_relative_path,repo_relative_path_string,relative_path_between), andnix_file_editor::remove_attrpath;ensure_secretrefactored onto the shared relative-path helper. - Frontend: un-hides the Add-secret button, backend-owns the add flow with error surfacing in the apply sheet, adds a delete confirmation dialog in the detail view, and simplifies the add form to SOPS-only.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
apps/native/src-tauri/src/secrets/secrets_management.rs |
Core SOPS add/delete: encrypt/decrypt over stdin, YAML edit, module declaration edit, build verify, commit, rollback, plus tests. |
apps/native/src-tauri/src/orpc/secrets.rs |
New addSecret/deleteSecret procedures and shared Git-state refresh after mutation. |
apps/native/src-tauri/src/shared_types/secrets_management.rs |
Adds AddSecretResult/DeleteSecretResult specta types. |
apps/native/src-tauri/src/git/exec.rs / git/mod.rs |
Generalizes commit_file into commit_files for selective multi-file commits; exports it; adds a test. |
apps/native/src-tauri/src/evolve/file_ops.rs |
Adds repo-relative and lexical relative-path helpers with tests. |
apps/native/src-tauri/src/evolve/nix_file_editor.rs |
Adds remove_attrpath for structural attrpath removal with tests. |
apps/native/src-tauri/src/evolve/ensure_secret.rs / evolve/mod.rs |
Reuses shared path helper; exports GitignoreChecker. |
apps/native/src/ipc/orpc-bindings.ts |
Generated TS bindings for the new inputs/results and procedures. |
apps/native/src/components/widget/secrets/*.tsx |
Wires add flow, delete confirmation dialog, apply-sheet error display; SOPS-only add form. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cae401a to
55c957d
Compare
f4ee943 to
6593ba3
Compare
f45afc8 to
6500519
Compare
6500519 to
cb02d05
Compare
6593ba3 to
11665de
Compare
darkmatteragent
left a comment
darkmatteragent
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
ac9ad5228e23 · 5 findings
Implements SOPS secret add/delete end-to-end with verification and scoped commits, but the feature breaks on nested-config layouts and on nixmac's own per-secret files — request changes.
- Git calls receive config-relative paths, so nested configs stage and restore the wrong paths, leaving half-applied edits.
- Delete hardcodes secrets/secrets.yaml while nixmac's own ensure_secret flow creates secrets/.yaml, so those secrets can never be deleted.
- Mutation-triggered vault refresh unmounts the UI mid-operation, losing result/error feedback; full-file re-encryption also re-keys sibling recipients unverified.
Findings
Caution
blocker · correctness — Git paths passed as config-relative but joined against repo workdir — nested configs commit and roll back the wrong paths
apps/native/src-tauri/src/secrets/secrets_management.rs:135
Gutter 135 passes &[MANAGED_SOPS_FILE, &declaration_rel] to commit_files(config_dir, ...) (same at gutters 287-291), and gutters 147-148 / 306-308 pass the same strings to restore_file(config_dir, ...). Both paths are relative to config_dir (gutters 94/243 set base = Path::new(config_dir) for every file_ops call), but commit_files (git/exec.rs:196-226) runs `git2::Repository::discover(
Caution
blocker · correctness — Delete hardcodes secrets/secrets.yaml; nixmac's own ensure_secret secrets (secrets/.yaml) deterministically fail to delete
apps/native/src-tauri/src/secrets/secrets_management.rs:95
Gutters 84-92 load the vault entry but gutter 95 discards secret.file and resolves the constant MANAGED_SOPS_FILE ("secrets/secrets.yaml", gutter 33) for every SOPS row — and secret-detail-view.tsx gutters 105-108 dispatch every vault row's Delete button here. This collides with the app's own producers: execute_ensure_secret creates secrets/{name}.yaml per secret (evolve/ensure_secret.rs:9
Warning
major · correctness — Mutation-triggered vault refresh unmounts the secrets UI mid-flight; add/delete result and error feedback are lost
apps/native/src-tauri/src/orpc/secrets.rs:102
Gutters 76 and 93 call refresh_state_after_mutation before the handler returns, and gutter 102 runs crate::git::query::status_and_cache(config_dir, &ctx.app) synchronously. status_and_cache writes the GitState cell, and git_state::update_status calls secrets_vault::refresh_if_active on every change (git_state.rs:114-115) — after a commit it always has. secrets_vault::refresh immediatel
Warning
major · security — Full-file re-encryption re-keys all sibling secrets to current .sops.yaml rules and is never verified decryptable before commit
apps/native/src-tauri/src/secrets/secrets_management.rs:261
Gutter 258 builds plaintext from updated_sops_plaintext, which decrypts the ENTIRE managed file (every sibling secret), and gutter 261 encrypt_sops_yaml(config_dir, MANAGED_SOPS_FILE, plaintext.as_bytes())? re-encrypts all of it with a fresh sops --encrypt (gutters 406-419 pass no metadata; decrypt_sops_file's --output-type yaml strips the sops: metadata block). Recipients therefore
Important
minor · correctness — remove_attrpath splits on '.' before unquoting, making dot-containing secret ids undeletable
apps/native/src-tauri/src/evolve/nix_file_editor.rs:916
Line 916 splits the raw attrpath on '.' (attrpath.split('.')) before line 917's normalize_attrpath_for_match strips quotes. For sops.secrets."my.token" — a legal nix quoted attr and a secret name nixmac itself permits (evolve/ensure_secret.rs:143 allows '.' in secret names) that the vault lists with id my.token — the target becomes ["sops","secrets","my","token"]. But `collect_attrpath_ass
Caution blocker · correctness — lastVault cache is not keyed to host/config source: a pending add can commit plaintext to the wrong repository Gutters 101-104: Caution blocker · data-loss — remove_attrpath's quote/whitespace normalization collides distinct keys and silently deletes sibling declarations Target segments are normalized at gutters 915-919 ( Warning major · correctness — Review sheet depicts a shared secrets.yaml update, but the backend creates a new per-secret file buildAddRequest still renders the pre-patch shared-file story: gutter 18 Warning major · correctness — Delete only works for nixmac-created per-file secrets; shared-sopsFile declarations fail with a cryptic error Gutter 93 Warning major · correctness — Add cannot merge into grouped/nested sops.secrets declarations — build check always fails and rolls back declare_sops_secret always emits SetAttrs with reviewed: ac9ad52 — 2026-08-31T23:24Z verdict: request_changes findings: 5
Run details
|
||||||||||||||||||||||||||||
ac9ad52 to
6d33048
Compare
0dc059a to
0766016
Compare
darkmatteragent
left a comment
darkmatteragent
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
6d330489eaf4 · 1 finding
End-to-end SOPS secret add/delete with clean-tree guards, darwin-rebuild verification, targeted commits, and fail-closed rollback — well built overall. One blocker: the new delete flow is keyed by secret id alone, but ids are only unique per backend, so a backend collision silently deletes the wrong secret.
Fix by carrying (backend, id) through the selection chain.
Findings
Caution
blocker · data-loss — Delete is keyed by id only; a backend id collision deletes the wrong (SOPS) secret
apps/native/src/components/widget/secrets/secret-detail-view.tsx:106
The new destructive call await client.secrets.deleteSecret({ secretId: secret.id, backend: secret.backend }) (lines 105-108) trusts an entry resolved upstream by id alone. In secrets-management.tsx, selectedSecret = vault.entries.find((s) => s.id === view.secretId) and VaultView's onOpenSecret(secret.id) carry only secret.id, but load_secrets_vault (secrets_management.rs:738-739) concate
6d33048 to
9cbf1b4
Compare
|
Prelint reached the review limit for this pull request (5 reviews of each kind). New pushes do not start a review.
|
9cbf1b4 to
03f4a59
Compare
darkmatteragent
left a comment
darkmatteragent
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
65a2f4562997 · 5 findings
Wires SOPS add/delete end-to-end with verification, scoped commits, and rollback — but two data-loss-class bugs must be fixed first: a stale, un-keyed vault cache that can commit a secret's plaintext to the wrong repo, and an attrpath normalizer that can silently delete sibling secret declarations.
- The route's
lastVaultcache ignores host/config identity, so an add begun for repo A submits against repo B after a settings change. remove_attrpathstrips quotes/whitespace on both sides, so deleting one secret can match and remove a differently-quoted sibling and commit it.- The review sheet also previews a shared secrets.yaml update while the backend writes per-secret files, and both flows fail on common non-nixmac repo layouts.
Findings
Caution
blocker · correctness — lastVault cache is not keyed to host/config source: a pending add can commit plaintext to the wrong repository
apps/native/src/components/widget/secrets/secrets-management.tsx:104
Gutters 101-104: const lastVault = useRef<SecretsVault | null>(null); if (state?.vault) lastVault.current = state.vault; const vault = state?.vault ?? (state?.loading ? lastVault.current : null); — the cache is a plain ref with no source identity, and SecretsVaultState carries none (state/secrets_vault.rs refresh_now writes only activated/loading/error/vault). SecretsManagementRoute is rende
Caution
blocker · data-loss — remove_attrpath's quote/whitespace normalization collides distinct keys and silently deletes sibling declarations
apps/native/src-tauri/src/evolve/nix_file_editor.rs:970
Target segments are normalized at gutters 915-919 (.map(normalize_attrpath_for_match) on attrpath.split('.')) and source keys at gutter 970 (normalize_attrpath_for_match(&attr.syntax().text().to_string())), and normalize_attrpath_for_match strips all whitespace and quotes. Two valid coexisting declarations sops.secrets."foo bar" = …; and sops.secrets.foobar = …; both normalize to `foob
Warning
major · correctness — Review sheet depicts a shared secrets.yaml update, but the backend creates a new per-secret file
apps/native/src/components/widget/secrets/add-secret-view.tsx:18
buildAddRequest still renders the pre-patch shared-file story: gutter 18 { path: "secrets/secrets.yaml", note: "· encrypted update", mark: "~" }, gutter 21 diffFile: "secrets/secrets.yaml", gutter 25 { kind: "added", text: + ${slug}: ENC[AES256_GCM,data:••••••,type:str] }, and the form preview gutter 53 const encryptTarget = secrets/secrets.yaml › ${slug}``. The backend this PR wires u
Warning
major · correctness — Delete only works for nixmac-created per-file secrets; shared-sopsFile declarations fail with a cryptic error
apps/native/src-tauri/src/secrets/secrets_management.rs:93
Gutter 93 let encrypted_file = managed_sops_file(secret_id); hardcodes secrets/{id}.yaml for every delete, and gutter 94 resolve_existing_path_in_dir(base, &encrypted_file) fails when that file doesn't exist. Vault entries come from file = toString secret.sopsFile (load_sops_secrets), so secrets using the standard sops-nix pattern sopsFile = ./secrets.yaml — a shared file with no `secret
Warning
major · correctness — Add cannot merge into grouped/nested sops.secrets declarations — build check always fails and rolls back
apps/native/src-tauri/src/secrets/secrets_management.rs:450
declare_sops_secret always emits SetAttrs with path: format!("sops.secrets.\"{secret_id}\"") (gutter 450). set_attrs only merges when the FULL target path already exists; otherwise it inserts a fresh flat sops.secrets."<id>" = { … }; at top level. For a module whose secrets are grouped — sops.secrets = { existing = { … }; }; (discovered by find_sops_declaration_file because it contains "sops
| subtitle: `New secret · ${slug}`, | ||
| files: [{ path: "secrets/secrets.yaml", note: "· updated", mark: "~" }], | ||
| files: [ | ||
| { path: "secrets/secrets.yaml", note: "· encrypted update", mark: "~" }, |
There was a problem hiding this comment.
[major] Review sheet depicts a shared secrets.yaml update, but the backend creates a new per-secret file
buildAddRequest still renders the pre-patch shared-file story: gutter 18 { path: "secrets/secrets.yaml", note: "· encrypted update", mark: "~" }, gutter 21 diffFile: "secrets/secrets.yaml", gutter 25 { kind: "added", text: + ${slug}: ENC[AES256_GCM,data:••••••,type:str] }, and the form preview gutter 53 const encryptTarget = secrets/secrets.yaml › ${slug}``. The backend this PR wires up does something else: secrets_management.rs gutters 36-38 managed_sops_file returns `secrets/{secret_id}.yaml`, and add_sops_secret encrypts/commits that NEW file (gutter 242). The ApplySheet is the consent step for a config-repo commit; the user approves an "encrypted update to secrets/secrets.yaml" while a brand-new file is created and committed. Fix: build chips/diff/preview from the managed layout — `secrets/${slug}.yaml` with mark "+", and a declaration diff instead of a fabricated ENC line.
| /// `sops.secrets.foo = { ... };`, nested attrsets, and a mixture of both. Empty parent | ||
| /// attrsets are intentionally preserved. An absent attrpath is reported as an error so | ||
| /// callers performing destructive operations do not silently succeed. | ||
| pub(crate) fn remove_attrpath(content: &str, attrpath: &str) -> Result<String> { |
There was a problem hiding this comment.
remove_attrpath splits the attrpath on ., so sops.secrets."my.secret" never matches — and the agent flow allows . in names (ensure_secret.rs:136-153). Delete of a dotted-name secret then always fails, after the encrypted file was already removed, relying on rollback.
| .ok_or_else(|| anyhow!("Secret declaration '{secret_id}' does not exist"))?; | ||
|
|
||
| let base = Path::new(config_dir); | ||
| let encrypted_file = managed_sops_file(secret_id); |
There was a problem hiding this comment.
Delete still targets by convention, not by declaration. The entry found in the vault just above is only existence-checked and discarded; the path is rebuilt as secrets/<id>.yaml. A secret whose sopsFile basename differs from its declaration name, lives outside secrets/, or sits in a shared file fails after the confirm dialog — and if an unrelated secrets/<id>.yaml happens to exist, that file is deleted and committed instead. Reproduced on a fresh template repo with demo = { sopsFile = ../../secrets/demo-renamed.yaml; } plus an unreferenced stray secrets/demo.yaml: delete reports success, and
$ git show --stat HEAD # "secrets: delete demo (sops)"
modules/darwin/sops-secrets.nix | 3 ---
secrets/demo.yaml | 16 ----------------
— the stray is gone, secrets/demo-renamed.yaml is left orphaned in the repo. Would resolving the path from the declaration's actual sopsFile work here?
| variant="ghost" | ||
| size="sm" | ||
| className="text-destructive" | ||
| disabled={capability === "unavailable" || isDeleting} |
There was a problem hiding this comment.
Delete no longer needs decryption (it just removes the file), but this gate still disables it for capability === "unavailable" — exactly the case where delete matters most: a secret nothing local can decrypt anymore. Same gate on the confirm button at line 318. Can it be dropped?
Alex Shabalin (alex-sparus)
left a comment
Alex Shabalin (alex-sparus)
left a comment
There was a problem hiding this comment.
Went over the new round — the refresh gate + last-vault UI, the backend-qualified ids, and the nixmacignore discovery all check out nicely. Three residuals as inline comments. The first is the one I'd still block on: delete resolves the file by naming convention rather than from the declaration, and I could reproduce it deleting and committing an unrelated file while reporting success. The other two (dotted names can't be deleted; the delete button still requires decryption capability it no longer needs) are smaller and fail closed.
df62437 to
d5670a6
Compare
65a2f45 to
db16ba5
Compare
Juanpe Bolívar (arximboldi)
left a comment
Juanpe Bolívar (arximboldi)
left a comment
There was a problem hiding this comment.
I like the direction this is taking!
But sadly, besides the comments inline, after spending a bunch of time trying the feature I haven't managed to get it to fully work. Some of my struggles where due to my own incorrect expectations, but some of it are, I reckon, bugs.
Problems getting it to work
The first set of problems was that I started trying it on a configuration that did not have SOPS set up in the flake at all. I did have a SOPS_AGE_KEY_FILE env variable, that was being correctly shown in "keys and recipients". The "add secret" button was available, so I tried it, filled in the data, and I was greeted with this error:
I went ahead and added the sops.yaml file as instructed.
Then I was greeted with this error:
So I committed separately.
Clearly that was not the only file needed though, since adding the secret would then cause the build to fail, because it was missing the SOPS flake input and the import of the module.
But then it was getting more annoying, because somehow sometimes (I think when the commit failed on the nix build or eval stages), Nixmac would leave the new secret and edited configuration there, and not revert it, which I would have to undo manually before trying again.
At some point I even got a full screen error:
Finally, after I have resolved all the SOPS setup errors, I am still unable to add a secret:
Once again, the changes are left on disk even after I press "Cancel", so if I try again it just shows the "Uncommited changes" error.
The upside of this is that I could verify the changes manually and commit myself, which worked, showing that this error is somehow an implementation issue.
Adding a second secret via the UI failed again with the same cryptic commit SOPS secret error.
So there are a few takeways from this journey:
-
I think
nixmacshould allow you to setup SOPS, so you can do the whole workflow from the UI. There could be a few ways to do this in the UI, which we could discuss elsewhere. Perhaps you had it already planned for a later step, which would make sense to scope down this PR, sorry if this was the case. -
If not, the messages indicating what's missing could be improved, but no need really to spend much time on this if (1) is gonna be addressed.
-
The requirement to have a clean working dir was a bit annoying, specially when the cause was the feature not cleaning up after error. But perhaps things could be improved:
a. Maybe the requirement could be relaxed? The feature could stage/commit only the hunks it edits, so if there are no conflicts in that area, it could still commit and leave the working dir dirty with the other changes.
b. The message could be shown as soon as you press the "add secret" button, or even gray it out and show some kind of message elsewhere that says "manual uncommited changes, editing secrets requires a clean working dir" or alike.
c. The error could tell you what's dirty and have "review and commit" button to bring some kind of popup to solve the issue directly. -
The last error that I got stuck with looks genuinely like a bug. Maybe triggered by the fact that my flake is in a subdir
nix/os? The produced edits where correct and built properly, it just failed to commit the changes.
Usability improvements
Besides this, there are some small usability issues:
-
It seems like auto-correct is enabled in the "Name" and "Value" inputs in the "Add secret" form. This would cause surprising things like auto-capitalize the word when you leave the input, specially suprising on the Value, where you can't see change by default! I'd just disable auto-correct there.
-
Pressing TAB when the Name input is focused moves the focus to the "hidden" button. It should move it to the "Value" input IMHO.
-
Editing reformatted my whole configuration. I'm not sure I vibe with this...
| let base = Path::new(config_dir); | ||
| if !base.join(".sops.yaml").is_file() && !base.join("sops.yaml").is_file() { | ||
| anyhow::bail!("No .sops.yaml or sops.yaml was found in the repository root"); | ||
| } |
There was a problem hiding this comment.
I guess this is where it tried to detect if sops is there.
Writing .sops.yaml by hand isn't enough, two more preconditions follow:
- find_sops_declaration_file (:411) needs a .nix file that already contains sops.secrets, else Could not find a Nix module containing sops.secrets. Nothing creates this one: declare_sops_secret goes through apply_semantic_edit, which requires an existing file (Copilot's point at :530).
- the local age key has to match a creation rule, or encryption fails.
| if !status.clean_head { | ||
| anyhow::bail!( | ||
| "The repository has uncommitted changes. Commit or stash them before adding a secret so nixmac can roll back safely if verification fails." | ||
| ); | ||
| } |
There was a problem hiding this comment.
See general comment for suggestions on how to make this error less frustrating for the user :)
| child | ||
| .stdin | ||
| .take() | ||
| .ok_or_else(|| anyhow!("open sops stdin"))? | ||
| .write_all(plaintext) | ||
| .context("send plaintext to sops")?; | ||
| let output = child.wait_with_output().context("wait for sops encrypt")?; | ||
| if !output.status.success() { | ||
| anyhow::bail!( | ||
| "sops encryption failed: {}", | ||
| String::from_utf8_lossy(&output.stderr).trim() | ||
| ); | ||
| } |
There was a problem hiding this comment.
sops's real error is hidden by a broken pipe.
When sops exits early (no matching creation rule, the common case) the write fails with EPIPE and returns first. The user sees send plaintext to sops: Broken pipe, and the sops encryption failed: … branch below never runs. Ignoring the write error, or reading stderr before propagating, would surface what sops actually said.
| .filter_map(|entry| { | ||
| std::fs::read_to_string(entry.path()) | ||
| .ok() | ||
| .filter(|text| text.contains("sops.secrets")) |
There was a problem hiding this comment.
Module search misses the nested form. This doesn't match sops = { secrets = { … }; };, the form remove_attrpath handles and its own test uses. Add and delete both fail with "Could not find a Nix module containing sops.secrets".
| let encrypted_file = managed_sops_file(secret_id); | ||
| let encrypted_path = resolve_existing_path_in_dir(base, &encrypted_file) | ||
| .with_context(|| format!("resolve {encrypted_file}"))?; | ||
| let declaration_file = find_sops_declaration_file(base)?; |
There was a problem hiding this comment.
The module isn't checked for the secret being deleted.
find_sops_declaration_file returns the standard module whenever it exists, without confirming it declares secret_id. Deleting a secret declared elsewhere fails at remove_attrpath with "attrpath does not exist" — after the encrypted file is already gone. Add has the mirror problem: it writes into a module that may not be where the user's other secrets live.
| if (applyInFlight.current) return; | ||
| setApplyPhase("building"); | ||
| setApplyError(null); | ||
| if (apply?.origin === "add" && pendingSecret) { |
There was a problem hiding this comment.
Not sure this is an actually reachable condition, but in theory add with no pending secret reports success.
When origin === "add" but pendingSecret is null, runApply falls through to the simulation timer, reaches done, and shows "Committed to your config", but nothing was sent to the backend. Could the add branch key off origin alone and error when the payload is missing?
| const runtimePath = (backend === "agenix" ? "/run/agenix/" : "/run/secrets/") + slug; | ||
| const encryptTarget = `secrets/secrets.yaml › ${slug}`; | ||
| const runtimePath = `/run/secrets/${slug}`; | ||
| const invalid = !name.trim() || !value.trim(); |
There was a problem hiding this comment.
Edge case: validation checks the name, not the slug. The request sends slugifySecretName(name). A punctuation-only name slugifies to empty, passes the form, and only fails in the backend's validate_new_secret once the apply sheet is open. Can also be solved by sluggifying punctuation to -.
| /// This is deliberately a no-op when the observable is not managed, which | ||
| /// keeps lower-level config helpers usable in isolated tests and early startup. | ||
| pub fn refresh<R: Runtime + 'static>(app: &AppHandle<R>) { | ||
| let mut gate = REFRESH_GATE.lock().unwrap(); |
There was a problem hiding this comment.
The refresh gate holds its lock across refresh_now. (nit)
The guard stays alive while refresh_now does a synchronous write_sync() and event emit. std::sync::Mutex isn't reentrant, so anything re-entering refresh or begin_mutation from that emit deadlocks. A panic under the lock also poisons it, making the .unwrap()s at :38, :53 and :92 panic from then on.
I haven't given this bit that much though but I don't love raw/global locks in Rust they are normally an smell and I wonder if there is an alternative design where synchronization happens by wrapping the appropriate data.
| // Fall back to the direct invalidation used before this state was | ||
| // synchronized here. | ||
| log::warn!("[{operation}] Failed to refresh Git state: {error}"); | ||
| secrets_vault::refresh(&ctx.app); |
There was a problem hiding this comment.
The documented fallback never runs. (nit)
This is called while _refresh_guard is alive, so refresh just re-sets an already-true refresh_pending. The fallback the comment describes can't happen — the refresh always comes from Drop. Worth dropping the guard first, or dropping the comment.
Summary
Hooks up the original storybook mocks end-to-end for SOP secret add and delete operations. (Age will be forthcoming.)
Includes some fileops and nix editor refactorings and enhancements for reusable/recurring patterns.
Screenshots:
Test Plan
New unit tests as appropriate, plus manual testing e2e.
Docs