Adding skills for general best practices#71
Conversation
|
WalkthroughA new documentation file SKILL.md is added, detailing the Key Package Lifecycle for Marmot applications. It defines the public KeyPackageManager API, establishes core operational rules, outlines application workflows from bootstrap through maintenance, provides example code sketches, and describes error handling behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SKILL.md`:
- Around line 10-19: The table for KeyPackageManager (client.keyPackages) is
missing the list() and count() methods referenced later; update the API overview
table to include entries for list() — which returns an array of stored key
package metadata (or iterable) — and count() — which returns the number of
stored key packages, matching the behavior used in the workflow example; add
brief one-line descriptions for both methods alongside the existing
create/track/rotate/etc. entries so the table and examples are consistent.
- Line 28: The wording "best-effort" is inaccurate because rotate() and purge()
currently await network.publish() and let failures propagate; update rotate()
and purge() in key-package-manager to demote relay-delete failures by catching
errors from network.publish() (or not awaiting it) and logging the failure
instead of rethrowing, ensuring the functions still complete successfully for UX
flows while preserving telemetry of the publish error.
- Line 104: Update the SKILL.md sentence to clarify that purge() will remove
local material when there are no tracked published events, but rotate() will not
always do so: rotate() throws KeyPackageRotatePreconditionError unless
options.relays is supplied or relays can be recovered from prior publish
records, so explicitly mention this precondition and the two ways to satisfy it
(providing options.relays or recovering relays from previous publish records).
- Around line 63-78: The maintainKeyPackages helper should pass the relay
fallback into the rotation call to avoid KeyPackageRotatePreconditionError for
entries without published events; update the call to
client.keyPackages.rotate(entry.keyPackageRef) inside maintainKeyPackages to
include the relays array (e.g., client.keyPackages.rotate(entry.keyPackageRef, {
relays })) so the rotate invocation receives the relay fallback.
| All operations live on `client.keyPackages` (`KeyPackageManager`): | ||
|
|
||
| | Method | Effect | | ||
| |---|---| | ||
| | `create(options)` | Generate, store locally, sign and publish kind-443 to relays | | ||
| | `track(event)` | Record an observed kind-443 event; returns `false` if not applicable | | ||
| | `markUsed(ref)` | Flag a package as consumed without deleting it | | ||
| | `rotate(ref, options?)` | Publish kind-5 deletion for old events, create+publish replacement, remove old local material | | ||
| | `remove(ref)` | Local store deletion only — no relay cleanup | | ||
| | `purge(refs)` | Publish kind-5 deletion for all known event IDs, remove local material (bulk-friendly) | |
There was a problem hiding this comment.
Document list() and count() in the API overview.
The workflow example on Line 67 and Line 75 uses client.keyPackages.list() and client.keyPackages.count(), but this table omits both methods. That makes the documented surface inconsistent within the same guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` around lines 10 - 19, The table for KeyPackageManager
(client.keyPackages) is missing the list() and count() methods referenced later;
update the API overview table to include entries for list() — which returns an
array of stored key package metadata (or iterable) — and count() — which returns
the number of stored key packages, matching the behavior used in the workflow
example; add brief one-line descriptions for both methods alongside the existing
create/track/rotate/etc. entries so the table and examples are consistent.
| 3. **Track continuously** — feed all incoming kind-443 events to `track()` so publish records stay accurate across devices. This is required for `rotate()` and `purge()` to know which relay event IDs to delete. | ||
| 4. **Prefer `rotate()` over `remove()` for published packages** — rotation cleans relays and replenishes supply atomically. | ||
| 5. **`used` is rotation debt** — packages marked used should be rotated in the background, not immediately during the join flow. | ||
| 6. **Relay deletion is best-effort** — kind-5 events are advisory; do not block UX on confirmation. |
There was a problem hiding this comment.
best-effort overstates the current behavior.
Line 28 suggests delete publication should not affect UX, but both rotate() and purge() currently await network.publish() and let publish failures reject the call (src/client/key-package-manager.ts:246-294 and src/client/key-package-manager.ts:327-365). Please either soften this wording or change the implementation to actually demote relay-delete failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` at line 28, The wording "best-effort" is inaccurate because
rotate() and purge() currently await network.publish() and let failures
propagate; update rotate() and purge() in key-package-manager to demote
relay-delete failures by catching errors from network.publish() (or not awaiting
it) and logging the failure instead of rethrowing, ensuring the functions still
complete successfully for UX flows while preserving telemetry of the publish
error.
| async function maintainKeyPackages( | ||
| client: MarmotClient, | ||
| relays: string[], | ||
| ): Promise<void> { | ||
| const entries = await client.keyPackages.list(); | ||
|
|
||
| for (const entry of entries) { | ||
| if (entry.used) { | ||
| await client.keyPackages.rotate(entry.keyPackageRef); | ||
| } | ||
| } | ||
|
|
||
| if ((await client.keyPackages.count()) < 2) { | ||
| await client.keyPackages.create({ relays, isLastResort: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pass relay fallback into rotate() in this example.
Line 71 can throw KeyPackageRotatePreconditionError for entries that have no tracked published events. Since this helper already receives relays, the safer pattern is to pass them through during rotation.
Suggested doc fix
for (const entry of entries) {
if (entry.used) {
- await client.keyPackages.rotate(entry.keyPackageRef);
+ await client.keyPackages.rotate(entry.keyPackageRef, { relays });
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function maintainKeyPackages( | |
| client: MarmotClient, | |
| relays: string[], | |
| ): Promise<void> { | |
| const entries = await client.keyPackages.list(); | |
| for (const entry of entries) { | |
| if (entry.used) { | |
| await client.keyPackages.rotate(entry.keyPackageRef); | |
| } | |
| } | |
| if ((await client.keyPackages.count()) < 2) { | |
| await client.keyPackages.create({ relays, isLastResort: true }); | |
| } | |
| } | |
| async function maintainKeyPackages( | |
| client: MarmotClient, | |
| relays: string[], | |
| ): Promise<void> { | |
| const entries = await client.keyPackages.list(); | |
| for (const entry of entries) { | |
| if (entry.used) { | |
| await client.keyPackages.rotate(entry.keyPackageRef, { relays }); | |
| } | |
| } | |
| if ((await client.keyPackages.count()) < 2) { | |
| await client.keyPackages.create({ relays, isLastResort: true }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` around lines 63 - 78, The maintainKeyPackages helper should pass
the relay fallback into the rotation call to avoid
KeyPackageRotatePreconditionError for entries without published events; update
the call to client.keyPackages.rotate(entry.keyPackageRef) inside
maintainKeyPackages to include the relays array (e.g.,
client.keyPackages.rotate(entry.keyPackageRef, { relays })) so the rotate
invocation receives the relay fallback.
| | `KeyPackageNotFoundError` | `rotate()` | Refresh local state; fall back to `create()` | | ||
| | `KeyPackageRotatePreconditionError` | `rotate()` | Pass `options.relays` explicitly | | ||
|
|
||
| If a package has no tracked `published` events, `rotate()`/`purge()` still removes local material but skips the kind-5 deletion step. |
There was a problem hiding this comment.
The rotate() part of this statement is incorrect.
If there are no tracked published events, purge() still removes local material, but rotate() does not always do that. rotate() throws KeyPackageRotatePreconditionError unless options.relays is provided or relays can be recovered from prior publish records (src/client/key-package-manager.ts:246-294). This caveat should be called out explicitly here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` at line 104, Update the SKILL.md sentence to clarify that purge()
will remove local material when there are no tracked published events, but
rotate() will not always do so: rotate() throws
KeyPackageRotatePreconditionError unless options.relays is supplied or relays
can be recovered from prior publish records, so explicitly mention this
precondition and the two ways to satisfy it (providing options.relays or
recovering relays from previous publish records).
|
Hey, @hzrd149, could you please take a look? It is my proposal for a skill for AI |
|
Hey @sergey3bv thanks for this contribution. The skill mostly good, but I think will be better to hold the merge of this as there are some changes coming that will make this skill inaccurate as they materialize |
|
Hey, @erskingardner, @hzrd149, @gzuuus, if you are still going to incorporate AI development, could you please create issues for that? I will be happy to work on them. |
2 participants
Gave it a try to implement skill for #61. I will be happy to hear your suggestions
Summary by CodeRabbit