feat: add nostr skill — protocol operations for AI agents#73
feat: add nostr skill — protocol operations for AI agents#73whoabuddy merged 3 commits intoaibtcdev:mainfrom
Conversation
Adds nostr skill with 7 subcommands: - post: Post kind:1 notes to relays (requires wallet) - read-feed: Read recent notes from relays - search-tags: Search by hashtag using NIP-12 #t filter - get-profile: Get kind:0 profile metadata - set-profile: Set kind:0 profile metadata (requires wallet) - get-pubkey: Derive Nostr pubkey via NIP-06 from BIP84 path - relay-list: List configured relay URLs Key derivation uses BIP84 m/84'/0'/0'/0/0 — same secp256k1 keypair as the BTC wallet. Uses nostr-tools + ws packages.
arc0btc
left a comment
There was a problem hiding this comment.
Good addition — Nostr is a natural fit for agent-to-agent communication and public presence. The code is clean and well-structured. A few things worth addressing before merge:
NIP-06 Path Mislabeling (medium)
The PR calls this "NIP-06 Key Derivation" in the code comments, SKILL.md, and AGENT.md, but uses BIP84 path m/84'/0'/0'/0/0 — the BTC SegWit derivation path. NIP-06 specifies m/44'/1237'/0'/0/0 (coin_type 1237).
Using the BTC path is a design choice (shared identity between BTC and Nostr), and the PR description acknowledges it. But the labeling is inaccurate and will cause confusion:
- A user expecting NIP-06 compliance will get a different npub than they'd get from Alby, Damus, or any other NIP-06 wallet with the same seed
- The skill's
get-pubkeyoutput will say"derivationPath": "m/84'/0'/0'/0/0"which tells you it's the BTC path, not NIP-06 — that's honest - Just rename "NIP-06 Key Derivation" to "Key Derivation (BTC-shared)" in comments and docs to avoid the false NIP-06 claim
The security implication of sharing the keypair (Nostr key compromise = BTC key compromise) is real but acknowledged. If this is the intended design, the docs just need to reflect it accurately.
set-profile Silently Wipes Unspecified Fields (minor)
const content: Record<string, string> = {};
if (opts.name) content.name = opts.name;
// ...kind:0 events are replaceable — publishing a new one replaces all previous fields. Calling set-profile --name "foo" with no other flags will delete the agent's about, picture, lud16, etc. The typical UX is to fetch the existing profile first and merge, then publish. Not a blocker for first release, but worth a follow-up or a note in the docs.
publishToRelays Timeout Race (minor)
await Promise.race([
pool.publish([relay], event),
new Promise((_, reject) => setTimeout(() => reject(new Error("timeout")), WS_TIMEOUT_MS)),
]);In nostr-tools v2+, SimplePool.publish returns Promise<string>[] (array), not a single Promise<string>. Promise.race receiving an array as its first element treats it as a non-thenable value and resolves immediately with it — the timeout never fires and the actual publish is not awaited. This may work in practice if the underlying connections complete anyway, but the timeout protection is ineffective. Suggest spreading: Promise.race([...pool.publish([relay], event), timeoutPromise]) or using Promise.allSettled on the publish results directly.
Everything else ✅
- NIP-12
#tfilter choice over NIP-50 is correct and well-documented - WebSocket polyfill via
globalThis.WebSocketis the right pattern for Bun/Node resolveHexPubkeyhandles both hex and npub inputs cleanly- Relay connection cleanup (
pool.close) is consistent - skills.json placement is alphabetically correct
- SKILL.md frontmatter is valid and consistent with the repo's skill schema
- AGENT.md covers prerequisites and workflow clearly
- Rename 'NIP-06' references to 'BTC-shared key derivation' — we use m/84'/0'/0'/0/0 (BIP84), not m/44'/1237'/0'/0/0 (NIP-06). This is intentional (shared BTC+Nostr identity) but labeling was inaccurate. - Fix publishToRelays timeout race — pool.publish() returns Promise[] in nostr-tools v2+, so Promise.race received an array (non-thenable) and resolved immediately. Now spreads the array properly. - Fix set-profile field wipe — kind:0 is replaceable, so publishing with only --name would delete about/picture/etc. Now fetches existing profile and merges before publishing. Co-reviewed-by: arc0btc
|
Thanks @arc0btc — all three addressed in 3961efc:
|
|
All three fixes in 3961efc look clean:
No further concerns — my approval stands. Ready for merge whenever @cocoa007 or @whoabuddy are ready. |
JackBinswitch-btc
left a comment
There was a problem hiding this comment.
Review: feat: add nostr skill
Clean implementation. The CLI structure, timeout handling, and profile merge logic are solid. A few observations:
Key derivation design choice
BIP-84 path (m/84'/0'/0'/0/0) instead of NIP-06 (m/44'/1237'/0'/0/0) is well-documented and makes sense for shared BTC/Nostr identity. Worth noting in SKILL.md that standard Nostr clients importing the same mnemonic via NIP-06 will derive a different key -- agents should be aware their npub is not NIP-06-recoverable.
Minor items
-
SKILL.md mentions mnemonic temp files ("Always clean up mnemonic temp files after signing operations") but the code uses
getWalletManager().getActiveAccount()directly -- no temp files created. This warning is stale and should be removed to avoid confusion. -
(globalThis as any).WebSocket = WebSocket-- global mutation. If SimplePool accepts a WebSocket option in its constructor, passing it explicitly would be safer. If not, this is fine as-is. -
No read-side signature verification -- events from relays are trusted as-is. Acceptable for a v1 skill but worth a
// TODO: verify event signaturescomment for future hardening.
What's correct
finalizeEventfromnostr-tools/pureis the right API for signing- NIP-12
#tfilter for tag search (not NIP-50) is pragmatic - set-profile fetches existing profile before merge -- prevents field wipes
- 10s WebSocket timeout is reasonable
- Error handling with try/catch in every command
Looks good overall. The stale mnemonic-temp-file warning in SKILL.md is the only thing I'd fix before merge.
JackBinswitch-btc
left a comment
There was a problem hiding this comment.
Reviewed the full diff. Clean implementation overall -- well-structured CLI, good docs, smart profile merge in set-profile. A few notes:
1. Key derivation path mismatch with MCP server
This skill uses BIP84 (m/84'/0'/0'/0/0) while the MCP server's nostr_sign_event tool uses NIP-06 (m/44'/1237'/0'/0/0) -- see aibtcdev/aibtc-mcp-server#247. Both are valid choices, but agents using both will end up with two different Nostr identities from the same wallet. Worth documenting this clearly in SKILL.md so agents know which pubkey to use where.
2. Stale mnemonic temp file reference in docs
SKILL.md mentions "Always delete mnemonic from temp files" and "/tmp/.mnemonic_tmp", but the code uses walletManager.getActiveAccount().privateKey directly -- no temp file involved. The security note is misleading and could be removed.
3. Post rate limit is advisory only
"Max 2 posts per day" is in the docs but not enforced in code. Not a blocker, but a counter/check would prevent agents from accidentally spamming relays.
None of these are blockers. Solid contribution.
Derivation path alignment questionNice work on this skill — the implementation is solid. Before merging, I want to get your thinking on the derivation path choice since you've been driving the Nostr/NIP discussion and this is the right time to nail it down. Current stateThis PR uses Questions
The goal is to pick one approach across both skills so agents don't end up with two different Nostr identities depending on which skill they call. Your input on the right default would be valuable since you started this discussion. |
|
the proper fix is to align the derivation path with nip06, not re-derive a segwit address |
nip06 by default makes sense to me, I like the what agents really need to understand (and maybe part of a different skill) are the different addresses it has. we tell it segwit and taproot for L1, stacks address L2 c32check (w/ Larry's proposal that'd be the 5757' BTC address?), and this would add nostr address L1 for 1237', and so on. |
|
Well I didn't intend for my last round of updates to merge this and end the discussion - @arc0btc can you reopen the points about derivation paths in an issue and tag the relevant parties? |
Add Nostr Skill
Adds a complete Nostr protocol skill for AI agents with 7 subcommands:
#tfilter (NOT NIP-50)Key Details
m/84'/0'/0'/0/0gives secp256k1 privkey → x-only pubkey for Nostr. Same keypair as BTC wallet.#tfilter (NIP-12), notsearch(NIP-50) — most relays don't support NIP-50wss://relay.damus.io,wss://nos.lol(relay.nostr.band often unreachable from sandboxed environments)nostr-tools+wsnpm packagesFiles Added
nostr/SKILL.md— Skill documentation with YAML frontmatternostr/AGENT.md— Agent delegation guidenostr/nostr.ts— TypeScript CLI with all subcommandsskills.json— Updated with nostr entry