feat: add agenix secret add/delete support - #656
feat: add agenix secret add/delete support#656Scott McMaster (scottmcmaster) wants to merge 3 commits into
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 85fd686
|
📋 PR Overview
🔬 Coverage
|
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Always start on SOPS | Consistent entry point | One extra click for agenix users | Change one useState default |
| Start on detected backend | Correct default for most users | Slightly more logic at startup | Small; discovery data is already available |
3. The submit button stays enabled when no recipients are registered for the selected backend, showing only a warning message.
The form already disables the submit button when the agenix configuration files are not discoverable. Extending that same guard to cover an empty recipient list is one additional condition. Without that guard, a user can fill out the form, read the warning about missing recipients, and still submit. The backend then rejects the operation with an error after the encryption attempt starts. The user loses the form state and must fill it out again after fixing recipients. The form already knows the list is empty, so it can block submission at that point instead of at the backend.
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Warn only, keep button enabled | Flexibility if the warning is a false positive | Confusing error after form submission; lost form state | Add one condition to invalid |
| Disable button when list is empty | Clear signal before submission; no lost state | None; the backend rejects anyway | Already resolved if changed |
4. New agenix secrets are always placed at `secrets/{name}.age` in the repository root, with no option to choose a different directory.
Most agenix repositories follow the flat secrets/ convention, so the default is correct for the common case. A user who organizes secrets by host or by namespace must move the file after nixmac creates it and update the rule path by hand. That manual step is not explained in the UI. The risk is low today and grows as more varied repository layouts are onboarded.
5. The runtime path shown in the UI is always `/run/agenix/{name}`, even if the user set a custom path via `age.secrets.{name}.path` in their Nix configuration.
Most agenix configurations use the default path, so the displayed value is correct for the common case. The path is informational, so the system is never broken by showing the wrong value. The risk grows when a user who has set a custom path copies the displayed value into application configuration. Reading the evaluated path from the vault at display time would eliminate the discrepancy without a user migration cost. That requires an additional Nix evaluation step not included in this PR.
6. Age encryption runs with no progress feedback and no timeout while `nix shell nixpkgs#age` downloads or executes.
On a warm Nix store the command completes quickly and the absence of feedback is acceptable. On a cold store the binary fetch can take more than ten seconds. A user who has never run the command before may interpret the silence as a hang and dismiss the sheet. A label such as "Fetching age..." or a bounded timeout would reduce that uncertainty. This matches the SOPS flow, which has the same limitation.
7. Three agenix-specific discovery fields sit alongside shared fields in the vault model that both backends share.
For two backends the flat layout is readable and the data contract is clear. A third backend would force more top-level fields to accumulate alongside the shared ones. A per-backend metadata map keyed by backend name would scale better, but the refactor can wait until a third backend appears. No user-visible problem exists today.
8. When nixmac finds more than one file declaring agenix secrets, the conflict error does not name the conflicting files.
A user who hits this error must search the repository manually to identify the conflicting files. Including the candidate paths in the error message would make the error immediately actionable. The system already has the paths when it produces the error. This is a one-line improvement with no architectural cost.
9. Both backends always appear in the add-form toggle, even in repositories that are configured for only one backend.
A user with a SOPS-only repository sees an agenix tab that shows a configuration error when selected. That is informative rather than misleading, but it introduces a concept the user does not need. A user with an agenix-only repository sees a SOPS tab that would succeed, which could create unintended cross-backend secrets. Showing only the configured backend would reduce cognitive load. Today with two backends the overhead is small.
10. Deleting an agenix secret removes the encrypted file, the recipient rule, and the Nix declaration in one committed operation, with a build check before the commit.
The three artifacts are tightly coupled. A dangling rule points at a missing file, and a dangling declaration references a non-existent secret. Removing all three together and verifying the build before committing is the correct approach. If the build check fails, the system restores exactly the files the operation touched and cannot disturb unrelated user work.
11. After adding an agenix secret, nixmac verifies that every recipient used for encryption also appears in the evaluated rule before committing.
This check closes the gap between the encryption step and the Nix configuration. Without it, a typo in a recipient key would encrypt the secret for the wrong set of identities and the build would still pass. The check confirms that the rule the system wrote matches the recipients it used. The extra Nix evaluation adds a few seconds but the safety gain is clear.
12. The encrypted directory path in the add-secret preview is always computed as the path from the declaration module to `secrets/` at the repository root.
This path drives the preview diff line that shows how the file attribute will be set. The computation is consistent with the backend, which also hardcodes secrets/ as the encrypted directory. A repository that stores encrypted files under a different directory name would see an incorrect preview and a backend error at submit time. The limitation is bounded by the same assumption as the hardcoded output path decision and carries the same low risk today.
Since the last review
- Still open: UI always shows /run/agenix/{name} as the runtime path regardless of any custom path set in Nix configuration (
format!("/run/agenix/{secret_id}")in add_age_secret andruntimePath = backend === "agenix" ? "/run/agenix/${slug}"in add-secret-view.tsx remain hardcoded in the current diff) - Still open: Submit button stays enabled when no recipients are registered, sending the user to a backend error after form submission (The
invalidcondition in add-secret-view.tsx does not includecommittedRecipients.length === 0; a warning paragraph renders but the button remains active) - Still open: Backend toggle always starts on SOPS regardless of which backend the repository actually uses (
useState<SecretBackend>("sops")in add-secret-view.tsx; no smart-default logic was added despite agenix discovery fields now being present in the vault) - Still open: New agenix secrets are always written to secrets/{name}.age with no configurable output directory (
encrypted_rel = format!("secrets/{secret_id}.age")is hardcoded in add_age_secret; the frontend also hardcodessecrets/${slug}.agein buildAddRequest) - Still open: Age encryption runs through nix shell nixpkgs#age with no progress indicator and no timeout (
age_encrypt_commandbuilds the samenix shell nixpkgs#ageinvocation with no timeout wrapper or progress event emitted to the frontend) - Still open: Three agenix-specific metadata fields sit at the top level of SecretsVault alongside shared fields (
agenixRulesFile,agenixDeclarationFile, andagenixEncryptedDirectoryFromDeclarationare added directly to theSecretsVaultstruct in secrets_management.rs and in the generated type bindings) - Still open: Multi-file conflict error does not name the conflicting files, leaving the user to search manually (
find_secret_declaration_filestill emits "Found multiple Nix modules containing {declaration_marker}; expected one {backend_name} declaration module" with no file paths included) - Still open: Team decision on whether agenix-only repositories should get a smart backend default, or whether always-SOPS is accepted long-term (No PR comment or code change addresses this question; the default remains "sops")
- Still open: Plan to read the evaluated age.secrets.{name}.path at display time rather than hardcoding the runtime path (No PR comment or code change addresses this question; the runtime path remains hardcoded in both the backend result and the frontend preview)
- Still open: Plan for a progress indicator or cancellation path for the nix shell nixpkgs#age fetch in a follow-up (No PR comment or code change addresses this question; no progress events or timeout were added)
Open questions
-
Has the team accepted the always-SOPS default as permanent behavior, or is there a near-term plan to auto-detect the repository backend and pre-select it?
-
Is there a plan to read the evaluated
age.secrets.{name}.pathvalue when displaying the runtime path, or is the hardcoded/run/agenix/{name}accepted as the long-term display? -
Is a progress indicator or bounded timeout for the
nix shell nixpkgs#agefetch on the roadmap for a follow-up?
Recommendation
Ship with changes
The core feature is well-implemented: the delete path correctly skips decryption, the add path verifies recipients and build state before committing, and the rollback is bounded to the files the operation owns. The one change required before shipping is adding the empty-recipients guard to the submit-button condition in the add form. Every other concern is a carry-forward trade-off that was already accepted in the prior review, and none of the new decisions introduce user-visible regressions.
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The agenix diff preview hardcodes ../../secrets/${slug}.age as the file path, which is only correct when the declaration module lives at exactly modules/darwin/agenix-secrets.nix (two directorie...
apps/native/src/components/widget/secrets/add-secret-view.tsx:24
1 finding(s) posted as inline comments.
There was a problem hiding this comment.
Pull request overview
This PR implements agenix secret add and delete support in nixmac, bringing the agenix backend to feature parity with the existing SOPS flow. Both flows encrypt/remove a per-secret .age file, edit the classic agenix rules file and the age.secrets declaration module, verify with a darwin-rebuild dry build, and commit — with bounded rollback on failure. The frontend gains a backend toggle (sops-nix / agenix) in the add-secret form and backend-aware copy across the apply and delete UIs. The change also includes DRY refactoring: shared helpers ensure_clean_repo, restore_repo_files_on_failure, and verify_dry_build_for_secret_edit; extracted sops_config_path / discover_agenix_rules_path / match_agenix_secret_entries in recipients.rs; and a new remove_attrpath_in_file plus quote-aware split_attrpath_for_match in nix_file_editor.rs.
Changes:
- Implement
add_age_secret/delete_age_secretwith agenix rules + declaration editing, age encryption over stdin, dry-build verification, and rollback. - Refactor shared secret-edit safety helpers and agenix rule discovery/matching; add quote-aware attrpath splitting so keys like
"api.token.age"resolve correctly. - Frontend: backend selector in the add-secret form, backend-aware recipient filtering/empty-state, and agenix-aware copy in the apply sheet and delete dialog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/native/src-tauri/src/secrets/secrets_management.rs | Core agenix add/delete logic, shared clean-repo/verify/rollback helpers, agenix rule/declaration writers and tests. |
| apps/native/src-tauri/src/secrets/recipients.rs | Extracts sops_config_path, discover_agenix_rules_path, and match_agenix_secret_entries; adjusts debug logging. |
| apps/native/src-tauri/src/evolve/nix_file_editor.rs | Adds quote-aware split_attrpath_for_match and filesystem remove_attrpath_in_file, with tests. |
| apps/native/src/components/widget/secrets/add-secret-view.tsx | Backend toggle, backend-specific paths/copy, registration-based recipient filtering, empty-state message. |
| apps/native/src/components/widget/secrets/secret-detail-view.tsx | canDelete allows agenix regardless of local decrypt capability; backend-aware delete dialog copy. |
| apps/native/src/components/widget/secrets/apply-sheet.tsx | Derives backend-specific encryption label ("age"/"SOPS"/fallback) for review and applying states. |
| apps/native/src/components/widget/secrets/types.ts | Adds optional backend field to ApplyRequest. |
| apps/native/src/components/widget/secrets/secrets-management.tsx | Generalizes the add-secret failure message to be backend-agnostic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f5d297c to
b76a140
Compare
f4ee943 to
6593ba3
Compare
b76a140 to
5b2306b
Compare
6593ba3 to
11665de
Compare
5b2306b to
f117d3d
Compare
11665de to
0ee0c75
Compare
652ca35 to
0ad03b4
Compare
darkmatteragent
left a comment
darkmatteragent
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
0ad03b47165f · 5 findings
Requesting changes: the new agenix flows can verify one tree and commit another. - The declaration-module finder discards its validated canonical path, so with a symlinked standard module the edit lands on the target while commit and rollback touch the symlink. - Two majors: suffix-colliding rule basenames make deletion falsely refuse, and encrypt_age_secret can deadlock on large values. - Two nits: a wrong doc comment and a missed DRY migration.
Findings
Caution
blocker · correctness — find_secret_declaration_file discards the validated canonical path and returns the unvalidated lexical one
apps/native/src-tauri/src/secrets/secrets_management.rs:617
Gutters 615-616 canonicalize and scope-check the standard module via resolve_existing_path_in_dir(base, standard_relative), then discard the result, and gutter 617 returns Ok(standard) — base.join(standard_relative) (gutter 609), the raw lexical path. When the standard module (or an ancestor like a symlinked modules/darwin) is a tracked symlink to another file inside the repo, is_file()
Warning
major · correctness — matching_agenix_rule_key basename fallback string-suffix over-matches and blocks deletion
apps/native/src-tauri/src/secrets/secrets_management.rs:264
The exact branch (gutter 254, evaluated_path.ends_with(Path::new(key))) compares whole path components, so for declarations written via builtins.path — which this patch itself produces (gutter 746, nix_builtins_path_meta_value) — the evaluated secret.file is /nix/store/<hash>-<basename>.age, whose hash-prefixed component never equals the rule key's components, and every such secret falls
Warning
major · correctness — encrypt_age_secret can deadlock on large plaintexts (stdin write before draining stdout)
apps/native/src-tauri/src/secrets/secrets_management.rs:674
encrypt_age_secret pipes stdin, stdout, and stderr (gutters 666-668) and writes the entire plaintext with write_all (gutter 674) before any output is drained (wait_with_output only runs at gutter 676). age encrypts as a stream: once the child has emitted ~64KiB of ciphertext its stdout pipe fills, it stops reading stdin, and the parent blocks in write_all with the stdin pipe still full — a mut
Tip
nit · documentation — remove_agenix_declaration doc says "rules file" but edits the declaration module
apps/native/src-tauri/src/secrets/secrets_management.rs:293
The doc comment at gutter 293 reads "Remove an agenix declaration for a specific secret from the rules file", but the function removes age.secrets."{secret_id}" via remove_attrpath_in_file on the caller-provided relative_file, which delete_age_secret passes as declaration_rel (gutter 147) — the declaration module, not the rules file. Same-family function remove_agenix_rule (gutter 286) is
Tip
nit · maintainability — New find_secret_declaration_file duplicates find_sops_declaration_file instead of replacing it
apps/native/src-tauri/src/secrets/secrets_management.rs:600
The new generic find_secret_declaration_file (gutters 600-654) reproduces the gitignore-filtered walkdir scan, visibility check for the standard module, and single-candidate match of the untouched find_sops_declaration_file (pre-existing, near-verbatim); only the error strings differ. The same applies to the $RULES/secrets.nix/secrets/secrets.nix discovery, now written twice (find_agenix_rules
Caution blocker · correctness — find_secret_declaration_file discards the validated canonical path and returns the unvalidated lexical one Gutters 615-616 canonicalize and scope-check the standard module via Warning major · correctness — matching_agenix_rule_key basename fallback string-suffix over-matches and blocks deletion The exact branch (gutter 254, Warning major · correctness — encrypt_age_secret can deadlock on large plaintexts (stdin write before draining stdout) encrypt_age_secret pipes stdin, stdout, and stderr (gutters 666-668) and writes the entire plaintext with Tip nit · documentation — remove_agenix_declaration doc says "rules file" but edits the declaration module The doc comment at gutter 293 reads "Remove an agenix declaration for a specific secret from the rules file", but the function removes Tip nit · maintainability — New find_secret_declaration_file duplicates find_sops_declaration_file instead of replacing it The new generic find_secret_declaration_file (gutters 600-654) reproduces the gitignore-filtered walkdir scan, visibility check for the standard module, and single-candidate match of the untouched find_sops_declaration_file (pre-existing, near-verbatim); only the error strings differ. The same applies to the $RULES/ reviewed: 0ad03b4 — 2026-09-01T00:03Z verdict: request_changes findings: 5
Run details
|
||||||||||||||||||||||||||||
ac9ad52 to
6d33048
Compare
0ad03b4 to
12ae9b2
Compare
12ae9b2 to
d9c860f
Compare
6d33048 to
9cbf1b4
Compare
There was a problem hiding this comment.
Caution
encrypt_age_secret calls write_all(plaintext) on the child's stdin and only then calls wait_with_output to drain stdout.
apps/native/src-tauri/src/secrets/secrets_management.rs:674
Caution
find_secret_declaration_file calls resolve_existing_path_in_dir(base, standard_relative) for scope validation but discards the returned canonical path, returning Ok(standard) — the raw lexical `...
apps/native/src-tauri/src/secrets/secrets_management.rs:617
Warning
The basename fallback in matching_agenix_rule_key uses secret.file.ends_with(basename) — a string-suffix check, not a path-component check.
apps/native/src-tauri/src/secrets/secrets_management.rs:264
3 finding(s) posted as inline comments.
Resolved findings (1)
The agenix diff preview hardcodes../../secrets/${slug}.ageas thefilepath, which is only correct when the declaration module lives at exactlymodules/darwin/agenix-secrets.nix(two directorie...apps/native/src/components/widget/secrets/add-secret-view.tsx:24
d9c860f to
90b8230
Compare
9cbf1b4 to
03f4a59
Compare
Prelint Review SummaryActive findings (3)Caution
Caution
Warning The basename fallback in
Resolved findings (1)
3 active, 1 resolved |
90b8230 to
5ef9bbe
Compare
|
Prelint reached the review limit for this pull request (5 reviews of each kind). New pushes do not start a review.
|
Summary
Implement the agenix secret add/delete analogous to what we did for SOPS. (Not including screenshots since there's no practical difference in the UI.)
Also includes some related DRY refactoring in secrets_management.rs and recipients.rs.
Test Plan
Some new unit tests where possible/appropriate, plus manual e2e testing with my test config repo.
Docs