Skip to content

feat(desktop): add an existing owned agent identity locally - #7502

Open
loganj wants to merge 2 commits into
validation/runtime-config-integration-411ec2f0from
feat/local-existing-agent-7ed69de4
Open

feat(desktop): add an existing owned agent identity locally#7502
loganj wants to merge 2 commits into
validation/runtime-config-integration-411ec2f0from
feat/local-existing-agent-7ed69de4

Conversation

@loganj

@loganj loganj commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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.

  • Re-fetch signed kind:0 ownership and independently verify owner-signed kind:30177 at the selected exact agent coordinate. Require the linked persona to already exist locally.
  • Fence the write with workspace apply → identity mutation → managed-agent store authorities and revalidate owner/community after asynchronous reads.
  • Healthy duplicates are no-ops. Explicit missing/revoked-key repair changes only the stored credential, preserving runtime/configuration state. Ordinary save/Move/restore retain F6: no provisioning authority.
  • Deliberate persistence choice: this explicit operation writes the supplied credential to the existing owner-only local agent file, atomically with the record, rather than doing a keyring write before the store commit. The dialog discloses this. Failure leaves retryable original state without orphan keyring entries. This trade-off requires independent security/persistence review.
  • Password input is local-only, cleared on submission/close, and never echoed in errors. Existing profile avatar is retained; imported runtime defaults inherit the same persona.

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.

  • Full native package: TAURI_CONFIG='{"bundle":{"externalBin":[],"resources":[]}}' cargo test --manifest-path desktop/src-tauri/Cargo.toml --no-default-features3,236 library tests + 10 integration tests passed; 11 ignored. The nondefault lane intentionally executes credential fixtures excluded by default system-keyring.
  • Native regressions bind the actual verifier/commit/storage seams: exact identity/persona/avatar; wrong key/foreign owner/invalid signatures/conflicting OA; healthy duplicate; post-await owner/community changes; explicit missing-key repair preserving running/config state; ordinary save refusal/no restoration; atomic-write preparation failure with unchanged bytes and successful retry; mode 0600; no autostart.
  • Full frontend pnpm test6,528 passed, including mounted native-command dialog submission, exact identity/scope arguments, secret clearing, safe failure and deliberate retry with no Start call.
  • Default-feature cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --all-targets -- -D warnings (same test-only TAURI_CONFIG) — passed.
  • pnpm typecheck, pnpm lint, nested Tauri cargo fmt --check, just file-size-check, git diff --check — passed. Frontend lint reports pre-existing warnings/infos, no errors.
  • Nondefault strict Clippy initially surfaced existing disabled-keyring dead-code and remote_credentials.rs fixture 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.

Signed-off-by: Logan Johnson <loganj@squareup.com>
@loganj
loganj requested a review from a team as a code owner September 8, 2026 21:30
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>
@loganj

loganj commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Fix for the review blocker: saved owner link is now verified before duplicate or repair — commit a3a0745d0

Blocker being closed (from the independent review of d8b6dc3c3): commit_verified admitted an existing record on pubkey/persona/community alone, then returned a healthy duplicate or repaired a missing key without ever checking the saved NIP-OA attestation. A legacy record with an absent, foreign, or invalid saved owner link could pass Add here while launch preparation (managed_agents/runtime_configurations.rs::prepareverify_owner) later refuses it as unowned.

Change (desktop native + docs, 5 files, +168/−24)

  • commands/existing_agent.rs — under the existing identity-mutation/store commit locks and after the persona/community match, the saved record's attestation is verified for the exact current owner+agent via agents::runtime_configurations::verify_owner — the same predicate launch preparation applies — before both the healthy-duplicate return and the key-repair write. Absent/foreign/invalid links are refused with a redacted fixed-string error and zero writes. No silent re-attestation, no owner migration, no new parser, no broadened semantics; kind:0 + owner-signed kind:30177 verification, scope checks and locking are unchanged.
  • managed_agents/runtime_configurations.rsverify_owner promoted to pub(crate) with a doc comment (visibility only; the predicate is unchanged).
  • managed_agents/types.rs — the private_key_nsec storage comment now describes the deliberate inline existing-agent import, not only the keyring-unreachable fallback (the review's flagged inaccuracy).
  • desktop/src/features/agents/AGENTS.md — the existing-agent import contract records the guard.
  • commands/existing_agent/tests.rs — new commit-seam regression saved_owner_link_gates_duplicate_and_repair: production creation commit first, then positive healthy-duplicate and explicit-repair controls, then absent / foreign / invalid saved attestations × healthy / missing saved keys (6 cases), each asserting refusal with the guard error, unchanged raw store bytes, and unchanged key state. The foreign case is a fully valid tag from a different owner; the invalid case is structurally valid JSON embedding the current owner but signed over a different agent's preimage, so only signature verification rejects it. The test is falsifiable: with the guard removed it fails at the commit seam (verified locally). The F6 duplicate/repair/atomic-write/0600 test is retained; its seed is now production-realistic (record + credential + owner attestation in one write).

Local validation (nondefault configuration executes credential fixtures; TAURI_CONFIG test bundle)

  • Full native package cargo test --manifest-path desktop/src-tauri/Cargo.toml --no-default-features: 3,237 library tests passed, 0 failed, 11 ignored (3,236 before + this regression), 10 integration tests passed, doc tests pass.
  • Focused: all 5 commands::existing_agent tests pass.
  • Default-feature all-target cargo clippy ... -- -D warnings: pass.
  • Nondefault all-target clippy: only the pre-existing baseline warnings (disabled-keyring dead code in secret_store.rs, remote_credentials.rs cloned-ref) — none in touched files.
  • Nested Tauri cargo fmt --check, just file-size-check, git diff --check: pass. Frontend untouched — prior 6,528-test / typecheck / lint evidence unchanged.

CI on the new head a3a0745d0 (initial inspection)

DCO, Dead Token Reference Guard, Detect Changed Paths, Semgrep OSS, zizmor: pass. In progress: Desktop Core, 4 Smoke E2E shards, macOS/Windows builds, Desktop E2E relay, Rust Lint, Windows Rust. Unrelated lanes path-skipped. No failing check. Old-head CI results do not carry over to this head.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..a3a0745d0c03bbb2f2b6db0f6fdcfe95867f3a30

Mutation evidence:

  • signed anyone and allowlist admission 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.

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.

2 participants