feat(desktop): add an existing owned agent identity locally - #7502
feat(desktop): add an existing owned agent identity locally#7502loganj wants to merge 2 commits into
Conversation
Signed-off-by: Logan Johnson <loganj@squareup.com>
Add existing agent admitted an existing record on pubkey/persona/community and then reported a healthy duplicate or repaired a missing key without ever checking the saved NIP-OA attestation. A legacy record with a missing, foreign, or invalid saved owner link could pass Add while launch preparation later refuses it as unowned. Under the existing commit locks, commit_verified now validates the saved record's attestation for the exact current owner and agent via the same runtime_configurations::verify_owner predicate launch preparation applies, before both the healthy-duplicate return and the key-repair write. Absent, foreign, and invalid links are refused with a redacted error and zero writes; no silent re-attestation or owner migration is introduced. Regression coverage at the real commit seam crosses healthy and missing saved keys with absent, foreign, and structurally-valid-but-unverifiable attestations, asserting refusal preserves raw store bytes and key state; positive valid-duplicate and explicit-repair controls are retained. Signed-off-by: Logan Johnson <loganj@squareup.com>
Fix for the review blocker: saved owner link is now verified before duplicate or repair — commit
|
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 8b4f9d6d3fa017690f77fd4fe2f67f54d443419a..a3a0745d0c03bbb2f2b6db0f6fdcfe95867f3a30 (exact live head confirmed immediately before submission)
Risk: critical — this adds a privileged Tauri identity-import command, persists a supplied private key, and transfers an agent's relay access policy onto a new local host.
Behavior/contracts traced: renderer disclosure and retry states → direct Tauri IPC registration/request → signed profile/policy verification → owner/community locks and local atomic persistence → duplicate/repair behavior → ordinary Start and respond-to environment projection. I also reviewed the new saved-owner-attestation delta against VISION.md, TESTING.md, and the feature-local agent contracts.
Blocking findings
1. Imported shared access can silently expose the destination computer to an existing audience
desktop/src-tauri/src/commands/existing_agent.rs:83-85,100-108 accepts an owner-signed anyone or allowlist policy and persists it unchanged. The directly registered command has no destination-host decision in its request (desktop/src-tauri/src/commands/existing_agent.rs:8-16; desktop/src-tauri/src/lib.rs:722-724). Meanwhile, desktop/src/features/agents/ui/AddExistingAgent.tsx:70-75 discloses local key storage and no autostart but does not preview the inherited audience or warn that starting the agent grants that audience access to this computer's files, accounts, and connected tools, despite the local warning contract in desktop/src/features/agents/AGENTS.md:138-167.
This is not renderer-only. A temporary signed-policy probe at this exact head passed both anyone and a nonempty allowlist through the production verified_record seam unchanged. Ordinary Start later projects the persisted policy into the child process (desktop/src-tauri/src/managed_agents/runtime.rs:751-761; desktop/src-tauri/src/managed_agents/access_policy.rs:60-68,74-112) without a destination-host acknowledgement. A user moving a valid identity from another host can therefore click ordinary Start and unknowingly grant its pre-existing audience access to this machine.
Author action: enforce the destination-host safety decision at the native boundary. The smallest robust remedy is to reject or clamp imported policy to owner-only with an empty allowlist, then let the existing warned Edit flow widen access. Add production-seam native regressions proving owner-only succeeds while signed anyone and allowlist cannot be committed. A renderer-supplied boolean alone is forgeable over direct IPC and is not sufficient.
Verification owner: author for native enforcement and regression; reviewer for direct-IPC and Start-path revalidation.
2. The claimed “healthy duplicate = zero writes” regression is non-falsifiable
The feature contract explicitly requires healthy duplicates not to write (desktop/src/features/agents/AGENTS.md:486-497), and production currently returns before persistence at desktop/src-tauri/src/commands/existing_agent.rs:201. Removing that return and falling through to persistence still leaves explicit_file_commit_repair_and_ordinary_save_f6 green. Its byte-equality assertion (desktop/src-tauri/src/commands/existing_agent/tests.rs:130-132 at the original test location; corresponding positive duplicate checks remain byte-based at the current head) cannot distinguish zero writes from an atomic rewrite of identical bytes.
The behavioral consequence is real: without the fence, a healthy duplicate enters the secret-file write/error path and can fail merely because the store is temporarily unwritable, violating the promised no-op behavior. This fails the repository's falsifiability rule in TESTING.md:25-31.
Author action: add an observable no-write witness at the production persistence seam—for example, make writes fail after reads and require a healthy duplicate still succeeds—and mutation-prove that removing the early return fails the regression.
Verification owner: author for test repair; reviewer for mutation recheck.
Reviewed new-head delta
The added saved-owner-link validation is sound in the bounded paths reviewed. commit_verified reloads and cryptographically verifies the saved attestation against the exact saved agent and current owner before duplicate success or repair, while holding identity_mutation and managed_agents_store_lock after a scope recheck (desktop/src-tauri/src/commands/existing_agent.rs:159-203; desktop/src-tauri/src/managed_agents/runtime_configurations.rs:401-418). Its failure is fixed/redacted and precedes persistence. The new matrix covers absent, foreign, and signature-invalid attestations across healthy/missing key states with unchanged-byte/key assertions (desktop/src-tauri/src/commands/existing_agent/tests.rs:184-306). No additional author action is required for that delta.
Validation at exact head a3a0745d0c03bbb2f2b6db0f6fdcfe95867f3a30
Passed:
TAURI_CONFIG='{"bundle":{"externalBin":[],"resources":[]}}' cargo test \
--manifest-path desktop/src-tauri/Cargo.toml --no-default-features \
commands::existing_agent::tests
# 5 passed
TAURI_CONFIG='{"bundle":{"externalBin":[],"resources":[]}}' cargo test \
--manifest-path desktop/src-tauri/Cargo.toml --no-default-features \
saved_owner_link_gates_duplicate_and_repair
# 1 passed
TAURI_CONFIG='{"bundle":{"externalBin":[],"resources":[]}}' cargo test \
--manifest-path desktop/src-tauri/Cargo.toml --no-default-features \
explicit_file_commit_repair_and_ordinary_save_f6
# 1 passed
git diff --check 8b4f9d6d3fa017690f77fd4fe2f67f54d443419a..a3a0745d0c03bbb2f2b6db0f6fdcfe95867f3a30Mutation evidence:
- signed
anyoneandallowlistadmission probe: passed unchanged policy at the production verifier; probe removed; - healthy-duplicate early-return removal: focused commit regression still passed; mutation restored;
- UI secret-clearing mutation was killed by the mounted assertion.
A full no-default-feature Tauri run reported 3236 passed, 1 failed, 11 ignored; the failure was the isolation-sensitive remote-credential fixture inline_revocation_during_ordinary_stop_survives_same_host_start (“fixture child did not confirm…”). No changed import call path was found in that failure, so it is reported as a confidence/gate issue rather than attributed to this PR. New-head CI run 34283625389 was still active at submission; superseded-head CI was excluded.
Manual/native evidence: not performed. No live secret, production identity, or ambient keychain state was exposed to the PR. Source, production-seam tests, and mutation probes establish the two defects without requiring native UI observation.
Residual risk: exact-artifact macOS/Linux UI behavior, narrow viewport, and full live import/start were not exercised. Current CI remains externally owned and must be evaluated at this exact head, but it is separate from the concrete author-actionable defects above.
Summary
Add a reachable native Desktop Agents → Add existing agent action for a user-supplied identity. Preserve the exact public key and existing owner-signed persona linkage; no minting, persona cloning, relay secret transfer, snapshot changes, or automatic start.
Related issue
Owner-approved request in Buzz channel f45d3304-dcf0-44e8-a46d-bcd63b235fbc; work discussion
84fc8babb95ff0bd02e2933f493da98380234cb9d0301fc7c51597b2a164cd48.Closest related PR: #7456 (frozen runtime-configuration validation baseline). This is a separate additive PR based on its unchanged branch at 8b4f9d6; it does not modify that branch or PR. No independent duplicate implementation found on this task branch (predecessor was design-only).
Review scope: 9 files, 693 added lines (including native persistence/scope regressions and mounted dialog tests). This is a concrete +93-line exception to the approximately 600-line review target, preserving a usable end-to-end operation instead of splitting out an unusable credential foundation.
Testing
Exact implementation HEAD:
d8b6dc3c30d8532edbdd06cff352f51c9a4a4b30; worktree clean after commit. Repo Hermit toolchain used.TAURI_CONFIG='{"bundle":{"externalBin":[],"resources":[]}}' cargo test --manifest-path desktop/src-tauri/Cargo.toml --no-default-features— 3,236 library tests + 10 integration tests passed; 11 ignored. The nondefault lane intentionally executes credential fixtures excluded by defaultsystem-keyring.pnpm test— 6,528 passed, including mounted native-command dialog submission, exact identity/scope arguments, secret clearing, safe failure and deliberate retry with no Start call.cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --all-targets -- -D warnings(same test-only TAURI_CONFIG) — passed.pnpm typecheck,pnpm lint, nested Tauricargo fmt --check,just file-size-check,git diff --check— passed. Frontend lint reports pre-existing warnings/infos, no errors.remote_credentials.rsfixture warnings; task warnings were corrected. Default strict Clippy is clean.Not claimed: live Mac/Linux integration, screenshots, provider/model selection, two-Desktop successful launch, or full
just ci(includes prohibited builds/unrelated platform activity). Live acceptance resources, credentials and app builds were explicitly off-limits for this assignment. No Blox/UI automation or live secret reads were performed. Screenshots are deliberately omitted for the same restriction.Manual acceptance after independent auth/persistence review: open Agents → Add existing agent on the destination, enter the exact existing agent public/private key under its owner/community, verify the same persona and stopped local record; retry a healthy duplicate, then explicit missing-key repair; separately approve provider/model and ordinary Start before any live switching acceptance.
Remaining gates
Independent ownership/persistence review (especially deliberate local-file secret storage), CI, and later authorized native integration. No merge/release/Canary is requested.