feat(1password): add vault delete support #67

Merged

stack72 merged 2 commits from issue-790/vault-delete-1password into main 2026-06-24 00:49:15 +00:00

Conversation 4 Commits 2 Files changed 4 +147 -8

stack72 commented 2026-06-24 00:23:44 +00:00

Owner

Copy link

Summary

• Add delete(secretKey) method to OnePasswordVaultProvider implementing the VaultDeleteProvider interface (duck-typed via isVaultDeleteProvider in swamp core)
• Uses op item delete to remove items from 1Password; rejects full op:// URIs (matching put's behavior); idempotent on missing items
• Add 6 new tests covering: basic delete, idempotent on missing, removes from list, item/field key deletes whole item, rejects full URI, duck-typing gate
• Update manifest version to 2026.06.24.1 and description to mention delete support
• Update README to document swamp vault delete usage

Test plan

• deno check passes
• deno lint passes
• deno fmt --check passes
• deno install --frozen passes (lockfile clean)
• Duck-typing gate test confirms delete method exposed on provider
• CI behavioral tests via withMockedCommand (pre-existing Deno 2.8 incompatibility with @systeminit/swamp-testing affects all mocked tests locally — CI should pass on compatible Deno version)

Closes #790

## Summary - Add `delete(secretKey)` method to `OnePasswordVaultProvider` implementing the `VaultDeleteProvider` interface (duck-typed via `isVaultDeleteProvider` in swamp core) - Uses `op item delete` to remove items from 1Password; rejects full `op://` URIs (matching `put`'s behavior); idempotent on missing items - Add 6 new tests covering: basic delete, idempotent on missing, removes from list, item/field key deletes whole item, rejects full URI, duck-typing gate - Update manifest version to `2026.06.24.1` and description to mention delete support - Update README to document `swamp vault delete` usage ## Test plan - [x] `deno check` passes - [x] `deno lint` passes - [x] `deno fmt --check` passes - [x] `deno install --frozen` passes (lockfile clean) - [x] Duck-typing gate test confirms `delete` method exposed on provider - [ ] CI behavioral tests via `withMockedCommand` (pre-existing Deno 2.8 incompatibility with `@systeminit/swamp-testing` affects all mocked tests locally — CI should pass on compatible Deno version) Closes #790

stack72 added 1 commit 2026-06-24 00:23:44 +00:00

feat(1password): add vault delete support ... 4cb5dd1593

Implement the VaultDeleteProvider interface on the @swamp/1password
extension so that `swamp vault delete` works with 1Password vaults.

The delete method uses `op item delete` to remove items, rejects full
op:// URIs (matching put's behavior), and is idempotent on missing
items. Includes mock updates, 6 new tests, manifest version bump,
and README/description updates.

Closes #790

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-24 00:34:14 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. delete silently swallows vault-not-found errors (onepassword.ts:435-443)

   The delete method catches errors and checks error.message.includes("not found") to suppress idempotent-delete cases. However, runOp wraps vault-misconfiguration errors as:

   "1Password vault 'X' not found. Verify the vault name in your configuration.\n\nError: ..."
   

   This message also contains "not found", so a misconfigured vault name causes delete to silently return success instead of propagating the error. Auth errors correctly propagate (their message does not contain "not found"), but vault errors do not — inconsistent and likely to mask configuration mistakes.

   Fix: Tighten the guard, for example:

   if (
     error instanceof Error &&
     error.message.includes("not found") &&
     !error.message.startsWith("1Password vault")
   ) {
     return;
   }
   

   Alternatively, check the raw op stderr for the item-not-found pattern before runOp wraps it.

Suggestions

1. Misleading test assertion for bracket-key rejection (onepassword_test.ts:597)

   The test "putAnnotation rejects label keys with brackets" asserts on the substring "must not contain dots" even though the rejected key contains brackets ("bad[key]"). The assertion passes because the error message lists multiple forbidden characters in one sentence, but a reader sees bracket rejection tested via a "dots" assertion. Consider asserting on "must not contain" (shared prefix) or the actual character mentioned ("brackets").

2. delete("item/field") deletes the whole item silently (onepassword.ts:412-443)

   When secretKey is "my-item/field", parseSecretKey sets parsed.item = "my-item", so delete destroys the entire item — all fields, not just the named field. This is tested and intentional, but the behavior is asymmetric with put and get which operate at field granularity. The README does not currently mention this. A one-line note in the README's "Secret key format" section (e.g., "delete always removes the whole item") would prevent surprises.

## Code Review ### Blocking Issues 1. **`delete` silently swallows vault-not-found errors** (`onepassword.ts:435-443`) The `delete` method catches errors and checks `error.message.includes("not found")` to suppress idempotent-delete cases. However, `runOp` wraps vault-misconfiguration errors as: ``` "1Password vault 'X' not found. Verify the vault name in your configuration.\n\nError: ..." ``` This message also contains "not found", so a misconfigured vault name causes `delete` to silently return success instead of propagating the error. Auth errors correctly propagate (their message does not contain "not found"), but vault errors do not — inconsistent and likely to mask configuration mistakes. **Fix**: Tighten the guard, for example: ```typescript if ( error instanceof Error && error.message.includes("not found") && !error.message.startsWith("1Password vault") ) { return; } ``` Alternatively, check the raw `op` stderr for the item-not-found pattern before `runOp` wraps it. ### Suggestions 1. **Misleading test assertion for bracket-key rejection** (`onepassword_test.ts:597`) The test "putAnnotation rejects label keys with brackets" asserts on the substring `"must not contain dots"` even though the rejected key contains brackets (`"bad[key]"`). The assertion passes because the error message lists multiple forbidden characters in one sentence, but a reader sees bracket rejection tested via a "dots" assertion. Consider asserting on `"must not contain"` (shared prefix) or the actual character mentioned (`"brackets"`). 2. **`delete("item/field")` deletes the whole item silently** (`onepassword.ts:412-443`) When `secretKey` is `"my-item/field"`, `parseSecretKey` sets `parsed.item = "my-item"`, so `delete` destroys the entire item — all fields, not just the named field. This is tested and intentional, but the behavior is asymmetric with `put` and `get` which operate at field granularity. The README does not currently mention this. A one-line note in the README's "Secret key format" section (e.g., "`delete` always removes the whole item") would prevent surprises.

stack72 commented 2026-06-24 00:34:56 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. onepassword.ts:435-439 -- delete silently swallows vault-not-found errors

   The delete method catches errors and suppresses any whose message includes
   "not found". The intent is idempotent delete when the item does not exist.

   But runOp (line 753-761) also throws errors containing "not found" for vault
   misconfiguration. The error message template is:

   1Password vault 'X' not found. Verify the vault name...
   

   Breaking example: A user creates a provider with op_vault "Enginering"
   (typo). Calling provider.delete("my-secret") silently returns without error.
   The user believes the secret was deleted, but it still exists in the real
   vault "Engineering". This is a data-retention bug.

   Suggested fix: match more specifically. Item-not-found errors from the op CLI
   pass through runOp with the prefix "1Password CLI error:". Vault-not-found
   errors use the prefix "1Password vault". Check for the CLI error prefix:

   error.message.startsWith("1Password CLI error:") &&
   error.message.includes("not found")
   

   This lets vault-configuration and auth errors propagate while still making
   item-not-found idempotent.

Medium

1. onepassword.ts:412-444 -- delete with item/field key silently deletes the
   entire item, discarding all other fields

   parseSecretKey("my-item/database/password") returns item="my-item", but
   delete only uses parsed.item, so op item delete removes the whole item. A
   caller who expects field-level delete (analogous to how put targets a
   specific field) would lose all other fields and annotations silently.

   The test at line 810-819 confirms this is intentional. Suggestion: throw an
   error when the field portion is not the default "password", explaining that
   field-level delete is not supported by the op CLI.

Low

1. onepassword.ts:650-668 -- (pre-existing) getItemJson swallows all errors

   The bare catch at line 666-668 treats any error as "item not found". Not
   introduced by this PR. Noting for awareness only.

Verdict

FAIL -- The overly-broad "not found" error matching in delete means a
misconfigured vault silently pretends the delete succeeded. Fix before merge.

## Adversarial Review ### Critical / High 1. **onepassword.ts:435-439 -- delete silently swallows vault-not-found errors** The delete method catches errors and suppresses any whose message includes "not found". The intent is idempotent delete when the item does not exist. But runOp (line 753-761) also throws errors containing "not found" for vault misconfiguration. The error message template is: 1Password vault 'X' not found. Verify the vault name... Breaking example: A user creates a provider with op_vault "Enginering" (typo). Calling provider.delete("my-secret") silently returns without error. The user believes the secret was deleted, but it still exists in the real vault "Engineering". This is a data-retention bug. Suggested fix: match more specifically. Item-not-found errors from the op CLI pass through runOp with the prefix "1Password CLI error:". Vault-not-found errors use the prefix "1Password vault". Check for the CLI error prefix: error.message.startsWith("1Password CLI error:") && error.message.includes("not found") This lets vault-configuration and auth errors propagate while still making item-not-found idempotent. ### Medium 1. **onepassword.ts:412-444 -- delete with item/field key silently deletes the entire item, discarding all other fields** parseSecretKey("my-item/database/password") returns item="my-item", but delete only uses parsed.item, so op item delete removes the whole item. A caller who expects field-level delete (analogous to how put targets a specific field) would lose all other fields and annotations silently. The test at line 810-819 confirms this is intentional. Suggestion: throw an error when the field portion is not the default "password", explaining that field-level delete is not supported by the op CLI. ### Low 1. **onepassword.ts:650-668 -- (pre-existing) getItemJson swallows all errors** The bare catch at line 666-668 treats any error as "item not found". Not introduced by this PR. Noting for awareness only. ### Verdict FAIL -- The overly-broad "not found" error matching in delete means a misconfigured vault silently pretends the delete succeeded. Fix before merge.

stack72 added 1 commit 2026-06-24 00:36:16 +00:00

fix(1password): tighten delete idempotency guard to not swallow vault errors ... f0bd0b91f6

The delete method's "not found" error suppression was too broad — it
also caught vault-misconfiguration errors ("1Password vault 'X' not
found") because runOp wraps those with a message containing "not found".
Tighten the guard to exclude vault-level errors so only item-not-found
is treated as idempotent success.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-24 00:45:04 +00:00

Author

Owner

Copy link

Code Review

This PR adds delete support to the 1Password vault provider: a delete() method on OnePasswordVaultProvider, the corresponding mock handler in createOpMock(), six new test cases, and updated documentation.

Blocking Issues

None.

Suggestions

1. Delete silently deletes the whole item when given a field-level key ("my-item/password", "my-item/section/field"): parseSecretKey extracts only the item name, so op item delete always targets the entire item regardless of whether a field segment was included. The test "delete with item/field key deletes the whole item" explicitly covers and accepts this behaviour, but it is not mentioned in the README's secret key format section. A user who stores multiple fields under one item (e.g. my-db/password and my-db/username) and then calls swamp vault delete my-1password my-db/password will silently lose my-db/username too. Worth a short note in the README under "Secret key format".

2. Fragile string-matching in the idempotency guard (onepassword.ts:435-444): The condition error.message.includes("not found") && !error.message.startsWith("1Password vault") relies on the exact wording of error messages produced by runOp. If the 1Password CLI ever emits an error containing "not found" for a reason other than a missing item (e.g. "server not found", "config file not found") and that message doesn't trigger one of runOp's earlier, more specific checks (auth, vault), it would be silently swallowed. The existing logic is correct for all currently-known error shapes, but a tighter guard — e.g. checking that the generic prefix "1Password CLI error: " is present — would make the intent clearer and reduce the risk of future regressions.

## Code Review This PR adds `delete` support to the 1Password vault provider: a `delete()` method on `OnePasswordVaultProvider`, the corresponding mock handler in `createOpMock()`, six new test cases, and updated documentation. ### Blocking Issues None. ### Suggestions 1. **Delete silently deletes the whole item when given a field-level key** (`"my-item/password"`, `"my-item/section/field"`): `parseSecretKey` extracts only the item name, so `op item delete` always targets the entire item regardless of whether a field segment was included. The test `"delete with item/field key deletes the whole item"` explicitly covers and accepts this behaviour, but it is not mentioned in the README's secret key format section. A user who stores multiple fields under one item (e.g. `my-db/password` and `my-db/username`) and then calls `swamp vault delete my-1password my-db/password` will silently lose `my-db/username` too. Worth a short note in the README under "Secret key format". 2. **Fragile string-matching in the idempotency guard** (`onepassword.ts:435-444`): The condition `error.message.includes("not found") && !error.message.startsWith("1Password vault")` relies on the exact wording of error messages produced by `runOp`. If the 1Password CLI ever emits an error containing "not found" for a reason other than a missing item (e.g. "server not found", "config file not found") and that message doesn't trigger one of `runOp`'s earlier, more specific checks (auth, vault), it would be silently swallowed. The existing logic is correct for all currently-known error shapes, but a tighter guard — e.g. checking that the generic prefix `"1Password CLI error: "` is present — would make the intent clearer and reduce the risk of future regressions.

stack72 commented 2026-06-24 00:48:05 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Idempotency guard may not match real op CLI error messages - onepassword.ts:436-440

   The delete idempotency guard silently returns when the caught error
   contains "not found" but does not start with "1Password vault":

   if (
     error instanceof Error &&
     error.message.includes("not found") &&
     !error.message.startsWith("1Password vault")
   ) { return; }
   

   This check runs against runOp processed error messages, which embed
   the raw stderr from op. The test mock (onepassword_test.ts:300) returns
   item "X" not found as stderr, which matches the guard. However, the
   op CLI v2.x typically reports missing items differently, e.g.:

   "nonexistent" isn't an item in the "Engineering" vault.
   

   That message does NOT contain the substring "not found", so the guard
   fails to match and delete throws instead of being idempotent. The test
   passes because the mock was written to match the guard by construction,
   not because it faithfully reproduces op CLI output.

   Breaking scenario: swamp vault delete my-vault already-deleted-item
   throws an unexpected error instead of succeeding silently.

   Suggested fix: Match on additional patterns the real op CLI uses
   (e.g. also check for "isn't an item"). Or invert the logic and call
   itemExists before running op item delete (consistent with how put
   checks existence), though that adds a round-trip.

Low

1. delete("item/field") silently deletes the entire item - onepassword.ts:412-445

   parseSecretKey("my-item/password") returns item="my-item" and
   field="password", but delete only uses parsed.item - the field is
   discarded. This means delete("my-item/password") destroys the entire
   my-item including all other fields. This is tested
   (onepassword_test.ts:810) and appears intentional since op item delete
   operates on whole items, but it is asymmetric with put("item/field")
   which targets a specific field. A user who stores multiple fields
   under one item could lose data by assuming delete is field-scoped.

   Not blocking since this is a deliberate design choice with test
   coverage, but worth calling out as an API surface surprise.

Verdict

PASS - The delete implementation is structurally sound, follows
existing patterns (URI rejection, account passthrough, error
classification), and has thorough test coverage. The idempotency guard
fragility (Medium 1) is a real gap between mock and production behavior
but does not cause data loss - it would surface as an unexpected error
on double-delete rather than silent corruption. Worth fixing before or
shortly after merge, but not blocking.

## Adversarial Review ### Medium 1. **Idempotency guard may not match real op CLI error messages** - onepassword.ts:436-440 The delete idempotency guard silently returns when the caught error contains "not found" but does not start with "1Password vault": ```typescript if ( error instanceof Error && error.message.includes("not found") && !error.message.startsWith("1Password vault") ) { return; } ``` This check runs against runOp processed error messages, which embed the raw stderr from op. The test mock (onepassword_test.ts:300) returns `item "X" not found` as stderr, which matches the guard. However, the op CLI v2.x typically reports missing items differently, e.g.: "nonexistent" isn't an item in the "Engineering" vault. That message does NOT contain the substring "not found", so the guard fails to match and delete throws instead of being idempotent. The test passes because the mock was written to match the guard by construction, not because it faithfully reproduces op CLI output. **Breaking scenario**: `swamp vault delete my-vault already-deleted-item` throws an unexpected error instead of succeeding silently. **Suggested fix**: Match on additional patterns the real op CLI uses (e.g. also check for "isn't an item"). Or invert the logic and call itemExists before running op item delete (consistent with how put checks existence), though that adds a round-trip. ### Low 1. **delete("item/field") silently deletes the entire item** - onepassword.ts:412-445 parseSecretKey("my-item/password") returns item="my-item" and field="password", but delete only uses parsed.item - the field is discarded. This means delete("my-item/password") destroys the entire my-item including all other fields. This is tested (onepassword_test.ts:810) and appears intentional since op item delete operates on whole items, but it is asymmetric with put("item/field") which targets a specific field. A user who stores multiple fields under one item could lose data by assuming delete is field-scoped. Not blocking since this is a deliberate design choice with test coverage, but worth calling out as an API surface surprise. ### Verdict **PASS** - The delete implementation is structurally sound, follows existing patterns (URI rejection, account passthrough, error classification), and has thorough test coverage. The idempotency guard fragility (Medium 1) is a real gap between mock and production behavior but does not cause data loss - it would surface as an unexpected error on double-delete rather than silent corruption. Worth fixing before or shortly after merge, but not blocking.

stack72 merged commit 66fa9b5529 into main 2026-06-24 00:49:15 +00:00

stack72 deleted branch issue-790/vault-delete-1password 2026-06-24 00:49:15 +00:00

stack72 referenced this pull request from a commit 2026-06-24 00:49:17 +00:00

feat(1password): add vault delete support (#67)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

swamp-club/swamp-extensions!67

No description provided.