Add specs - #1
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded a Spec‑Driven Development toolchain: nine speckit command specs, feature scaffolding and plan scripts, template artifacts, a project constitution, agent‑context tooling, and a full Mostro Mobile v2 feature (spec, plan, research, data model, contracts, tasks, quickstart, checklists). No existing source code APIs were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SpeckitAnalyze as Speckit.Analyze
participant PrereqScript as check-prerequisites.sh
participant FS as FileSystem
participant Constitution as ConstitutionFile
participant Analyzer as SemanticAnalyzer
User->>SpeckitAnalyze: run speckit.analyze ($ARGUMENTS)
SpeckitAnalyze->>PrereqScript: invoke --json --require-tasks --include-tasks
PrereqScript-->>SpeckitAnalyze: JSON { FEATURE_DIR, AVAILABLE_DOCS }
SpeckitAnalyze->>FS: read FEATURE_DIR/spec.md, plan.md, tasks.md
SpeckitAnalyze->>Constitution: load .specify/memory/constitution.md
SpeckitAnalyze->>Analyzer: build semantic model & run detection passes
Analyzer-->>SpeckitAnalyze: findings (severity-classified, bounded)
SpeckitAnalyze->>User: present Markdown report + suggested next actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (10)
.specify/scripts/bash/setup-plan.sh (1)
41-50: Consider checking if plan file already exists before overwriting.The script unconditionally copies the template to
$IMPL_PLAN, potentially overwriting an existing plan file with user content. This may be intentional for a "setup" script, but a warning or confirmation might prevent accidental data loss.Proposed safeguard
TEMPLATE=$(resolve_template "plan-template" "$REPO_ROOT") || true if [[ -n "$TEMPLATE" ]] && [[ -f "$TEMPLATE" ]]; then + if [[ -f "$IMPL_PLAN" ]]; then + echo "Warning: $IMPL_PLAN already exists, skipping copy" + else cp "$TEMPLATE" "$IMPL_PLAN" echo "Copied plan template to $IMPL_PLAN" + fi else echo "Warning: Plan template not found" - # Create a basic plan file if template doesn't exist - touch "$IMPL_PLAN" + if [[ ! -f "$IMPL_PLAN" ]]; then + touch "$IMPL_PLAN" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/setup-plan.sh around lines 41 - 50, The current template copy unconditionally overwrites $IMPL_PLAN; modify the setup logic in this script to first check whether the target plan file ($IMPL_PLAN) already exists and, if so, avoid silently overwriting it (options: skip copy and echo a warning, create a timestamped backup, or prompt the user); implement the check around the cp "$TEMPLATE" "$IMPL_PLAN" block (use the existing TEMPLATE variable and resolve_template call to locate the template) and ensure the script logs the chosen action (skipped, backed up, or overwritten) so users won’t lose existing plan content unexpectedly.specs/001-mostro-p2p-client/contracts/types.md (1)
11-97: Consider adding language specifiers to fenced code blocks.Static analysis flags these code blocks as lacking a language identifier. Since these are schema definitions rather than executable code, using
textorplaintextas the identifier would satisfy the linter while accurately representing the content.Example fix for one block
### OrderKind -``` +```text Buy | Sell</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@specs/001-mostro-p2p-client/contracts/types.mdaround lines 11 - 97, Add a
language specifier (e.g., "text" or "plaintext") to each fenced code block in
this file so the linter recognizes them as non-executable schema blocks; update
every block such as the Buy | Sell block and the blocks labeled OrderStatus,
TradeRole, BuyerStep, SellerStep, TradeStep, TradeOutcome, MessageType,
DisputeStatus, DisputeResolution, RelayStatus, ConnectionState,
QueuedMessageStatus, CooperativeCancelState, FileType, DownloadStatus, and
WalletStatus by changing the openingtotext (or ```plaintext) while
keeping the contents unchanged.</details> </blockquote></details> <details> <summary>.claude/commands/speckit.taskstoissues.md (1)</summary><blockquote> `14-30`: **Consider documenting task filtering behavior.** The outline specifies creating issues "for each task in the list" but doesn't clarify whether this includes: - Only unchecked tasks (`- [ ]`) - All tasks regardless of status - Tasks with specific labels/phases This could lead to duplicate issues if run multiple times or after partial completion. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/commands/speckit.taskstoissues.md around lines 14 - 30, The outline is ambiguous about which tasks to create issues for and risks duplicates; update the spec to explicitly define task-filtering behavior (e.g., only unchecked markdown tasks '- [ ]', all tasks, or tasks matching labels/phases) and add deduplication rules (e.g., match by task text or embedded task ID) before the MCP create step so the MCP/GitHub issue creation loop only processes the intended subset; reference the existing artifacts FEATURE_DIR, AVAILABLE_DOCS and the tasks list produced by .specify/scripts/bash/check-prerequisites.sh, and document/configure a flag or parameter used by the Git remote / MCP server step to control filtering and dedupe logic. ``` </details> </blockquote></details> <details> <summary>specs/001-mostro-p2p-client/contracts/identity.md (1)</summary><blockquote> `16-21`: **Add language identifier to fenced code blocks.** Fenced code blocks should specify a language for proper syntax highlighting and linter compliance. For pseudo-code structures, use `text` or `rust` (since the module is in Rust). <details> <summary>📝 Proposed fix</summary> ```diff **Returns**: -``` +```text IdentityCreationResult { public_key: String # Hex-encoded public key mnemonic_words: Vec<String> # 12-word BIP-39 mnemonic (show once, user must back up) } ``` ``` Apply similar changes to the code blocks on lines 111 and 139. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/contracts/identity.md` around lines 16 - 21, The fenced code blocks in this file lack a language identifier; update the blocks that define IdentityCreationResult and the similar structures at the other two locations to include a language tag (e.g., use `text` or `rust`) so linters and syntax highlighters work correctly—specifically add the language identifier to the block containing the IdentityCreationResult structure and to the fenced blocks at the other two occurrences referenced (around the code near lines 111 and 139). ``` </details> </blockquote></details> <details> <summary>.claude/commands/speckit.checklist.md (1)</summary><blockquote> `241-241`: **Add hyphen to compound modifier.** "Rate limiting requirements" should be "rate-limiting requirements" when the compound modifier precedes the noun. <details> <summary>📝 Proposed fix</summary> ```diff -- "Are rate limiting requirements quantified with specific thresholds? [Clarity]" +- "Are rate-limiting requirements quantified with specific thresholds? [Clarity]" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/commands/speckit.checklist.md at line 241, The compound modifier "rate limiting" in the checklist string should be hyphenated; locate the checklist entry containing the exact text "Are rate limiting requirements quantified with specific thresholds? [Clarity]" and change it to "Are rate-limiting requirements quantified with specific thresholds? [Clarity]" so the compound modifier precedes the noun correctly. ``` </details> </blockquote></details> <details> <summary>.specify/CURRENT_FEATURES.md (1)</summary><blockquote> `33-33`: **Use inline code to prevent markdown link interpretation.** The blob structure notation uses square brackets that markdown interprets as a link reference. Wrapping in backticks clarifies this is a format specification. <details> <summary>📝 Proposed fix</summary> ```diff -- Blob structure: [nonce:12][encrypted_data][auth_tag:16] +- Blob structure: `[nonce:12][encrypted_data][auth_tag:16]` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.specify/CURRENT_FEATURES.md at line 33, The blob structure line in .specify/CURRENT_FEATURES.md uses square brackets which Markdown treats as link references; update the "Blob structure: [nonce:12][encrypted_data][auth_tag:16]" line to use inline code formatting (wrap the entire blob specification in backticks, e.g. `[...]`) so the notation is preserved literally and not parsed as links. ``` </details> </blockquote></details> <details> <summary>.specify/scripts/bash/update-agent-context.sh (2)</summary><blockquote> `436-436`: **Remove unused variable `file_ended`.** This variable is declared but never referenced anywhere in the script. <details> <summary>♻️ Proposed fix</summary> ```diff local changes_entries_added=false local existing_changes_count=0 - local file_ended=false ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/update-agent-context.sh at line 436, The variable file_ended is declared as "local file_ended=false" but never used; remove this unused local variable declaration from the script (search for "file_ended" and delete the "local file_ended=false" line), ensuring no other code depends on it and run the script's lint/tests to confirm no references remain. ``` </details> --- `512-512`: **Variable `changes_entries_added` is set but never read.** This variable is assigned on lines 471 and 512 but its value is never used for any decision or output. Consider removing it or using it for reporting. <details> <summary>♻️ Proposed fix (remove if not needed)</summary> ```diff local in_tech_section=false local in_changes_section=false local tech_entries_added=false - local changes_entries_added=false local existing_changes_count=0 ``` And remove the assignments on lines 471 and 512: ```diff in_changes_section=true - changes_entries_added=true continue ``` ```diff echo "$new_change_entry" >> "$temp_file" - changes_entries_added=true fi ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/update-agent-context.sh at line 512, The variable changes_entries_added is assigned but never used; remove its declaration and both assignments to clean up dead code in the script (or, if intended for reporting, actually increment/use it and emit a summary message). Locate the symbol changes_entries_added in the update-agent-context.sh script and either delete all references/assignments to it or implement logic to update and print its value (e.g., increment where entries are added and echo a summary at the end) so the variable is meaningful. ``` </details> </blockquote></details> <details> <summary>specs/001-mostro-p2p-client/spec.md (1)</summary><blockquote> `285-285`: **Add hyphen to compound modifier.** "NWC compatible wallet" should be hyphenated as "NWC-compatible wallet" when the compound modifier precedes the noun. <details> <summary>📝 Proposed fix</summary> ```diff -- **FR-027**: Users MUST be able to connect a Nostr Wallet Connect (NWC) compatible wallet by pasting a NWC URI. +- **FR-027**: Users MUST be able to connect a Nostr Wallet Connect (NWC)-compatible wallet by pasting a NWC URI. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/spec.md` at line 285, Update the wording of requirement FR-027 to hyphenate the compound modifier: change the phrase "NWC compatible wallet" to "NWC-compatible wallet" in the FR-027 line so the compound modifier correctly precedes the noun; ensure the exact requirement identifier "FR-027" and the phrase are updated in the specs/001-mostro-p2p-client/spec.md document. ``` </details> </blockquote></details> <details> <summary>.claude/commands/speckit.implement.md (1)</summary><blockquote> `25-25`: **Add language identifiers to fenced code blocks.** Unlabeled fences trigger markdown lint and reduce readability in tooling that supports syntax highlighting. <details> <summary>Suggested edit</summary> ```diff - ``` + ```text ... - ``` + ```text ... - ``` + ```text ... - ``` + ```text ``` </details> Also applies to: 36-36, 180-180, 191-191 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/commands/speckit.implement.md at line 25, The markdown contains
unlabeled fenced code blocks using plain triple backticks (```); update each
unlabeled fence in .claude/commands/speckit.implement.md (instances around the
current block and also at the locations noted: lines ~36, ~180, ~191) to include
a language identifier (use "text" for these plain examples) so fences becomeindentation/spacing is preserved for the blocks around the examples referenced by the diff markers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/speckit.implement.md:
- Around line 134-136: The current parsing step "Parse tasks.md structure and
extract: - **Task phases**..." hard-codes phase names (Setup, Tests, Core,
Integration, Polish); change it to detect phase headers dynamically instead:
scan tasks.md for heading patterns (e.g., markdown headings like
/^#{1,6}\s*(.+)/) and treat each matched heading text as a phase name in
document order, allow optional configurable overrides (e.g., a provided phase
list or frontmatter key) and preserve their sequence for dependency rules, and
remove any logic that compares against literal strings "Setup", "Tests", etc.;
update the code that builds the phase list (the "Task phases" extraction logic)
to use the dynamic heading extractor and fall back to a sensible default if no
headings are found.
In @.claude/commands/speckit.plan.md:
- Line 70: Step 4 in .claude/commands/speckit.plan.md currently refers to "Phase
2 planning" but the document only defines Phase 0 and Phase 1; update the text
in Step 4 to read "after Phase 1 planning" (replace the "Phase 2" token) so it
matches the defined Phases section. Locate the Step 4 line that contains "after
Phase 2 planning" and change the phrase to "after Phase 1 planning" (no other
structural changes needed).
In @.claude/commands/speckit.specify.md:
- Around line 116-118: The spec text currently conflicts by both mandating
creation of FEATURE_DIR/checklists/requirements.md in Step 6 and also stating
that checklist creation is a separate command; update the command/spec to choose
one policy and make it explicit: either (A) keep Step 6 and require generating
the checklist file at FEATURE_DIR/checklists/requirements.md (remove any
“separate command” phrasing), or (B) remove the Step 6 mandate and reference the
separate checklist command (e.g., “run the checklist-generation command to
create FEATURE_DIR/checklists/requirements.md”); ensure the wording around
"Specification Quality Validation", the Step 6 paragraph, and any references at
lines 246-247 consistently reflect the chosen option so there is no ambiguity
about when and how requirements.md is created.
In @.specify/scripts/bash/create-new-feature.sh:
- Around line 246-259: Validate BRANCH_NUMBER before using it to compute
FEATURE_NUM and BRANCH_NAME: ensure BRANCH_NUMBER is an integer between 1 and
999 (inclusive) and error out with a clear message if not; specifically, before
FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))") add a guard that checks
BRANCH_NUMBER is numeric (matches ^[0-9]+$) and within 1..999, printing a
usage/error and exiting non‑zero on failure so invalid values like "foo" or
"1000" fail fast (this relates to variables BRANCH_NUMBER, FEATURE_NUM and
BRANCH_NAME and will keep downstream matchers in common.sh happy).
- Around line 297-307: The script always creates a new
FEATURE_DIR="$SPECS_DIR/$BRANCH_NAME", which can create duplicate numeric-prefix
dirs and break the resolver; instead detect an existing directory in $SPECS_DIR
that shares the numeric prefix of $BRANCH_NAME (extract the prefix before the
first '-') and, if found, set FEATURE_DIR to that existing directory; only mkdir
-p when no existing dir was found; when populating
SPEC_FILE="$FEATURE_DIR/spec.md" use a non-destructive copy (or skip copying) so
you do not overwrite an existing spec.md (e.g., check [ -f "$SPEC_FILE" ] before
cp or use cp -n), and keep using resolve_template and TEMPLATE logic as-is for
creating a new spec when none exists.
- Around line 70-75: The current trim uses xargs which re-tokenizes
FEATURE_DESCRIPTION and breaks on quotes/apostrophes under set -e; replace the
xargs-based trimming with a safe shell-only trimming approach that does not
reparse words — e.g., use POSIX-safe parameter expansion or a printf | sed
pipeline that reads FEATURE_DESCRIPTION as-is and strips leading/trailing
whitespace — then keep the existing empty-check (the if [ -z
"$FEATURE_DESCRIPTION" ] block) unchanged; locate the FEATURE_DESCRIPTION
assignment and its subsequent validation to make this replacement.
In @.specify/scripts/bash/update-agent-context.sh:
- Line 398: The current line uses "local tech_stack=$(format_technology_stack
"$NEW_LANG" "$NEW_FRAMEWORK")" which masks the exit status of
format_technology_stack; change it to declare the local variable first (local
tech_stack) then call format_technology_stack with "$NEW_LANG" and
"$NEW_FRAMEWORK" and assign its output to tech_stack
(tech_stack=$(format_technology_stack ...)) and then check the function's exit
code ($?) so failures from format_technology_stack are not lost.
- Around line 317-319: The three variables escaped_lang, escaped_framework, and
escaped_branch are declared and assigned in the same statement which masks
command-substitution failures under set -e; declare each variable first with
local (e.g., local escaped_lang) and then assign using the command substitution
(e.g., escaped_lang=$(printf ... | sed ...)) for escaped_lang,
escaped_framework, and escaped_branch so any sed/printf error will propagate
instead of being hidden.
In @.specify/templates/plan-template.md:
- Line 6: Update the self-referential sentence that currently reads "See
.specify/templates/plan-template.md for the execution workflow" to point to the
actual command/workflow documentation for filling this template (the
/speckit.plan command) so users aren’t directed back to the same template;
locate the sentence in .specify/templates/plan-template.md and replace the
reference with a clear pointer to the /speckit.plan command or its workflow
docs.
In `@CLAUDE.md`:
- Around line 18-20: Remove the leaked template token "[ONLY COMMANDS FOR ACTIVE
TECHNOLOGIES]" from the "## Commands" section in CLAUDE.md so the line reads a
valid command list (e.g., "cargo test cargo clippy"); specifically locate the
"## Commands" header and the line containing the token and delete the bracketed
token occurrences to prevent the malformed markdown and undefined reference
label.
- Around line 26-29: The Recent Changes section contains a duplicated entry for
"001-mostro-p2p-client" (the line mentioning Rust stable 1.75+, Dart 3.x /
Flutter 3.x, nostr-sdk 0.44+, mostro-core, flutter_rust_bridge 2.x, Riverpod,
go_router); remove the duplicate so the entry appears only once under the "##
Recent Changes" heading and ensure spacing/formatting remains consistent with
the surrounding list.
In `@specs/001-mostro-p2p-client/contracts/messages.md`:
- Around line 80-87: The fenced block for the FileDownloadResult type lacks a
language identifier and there is a duplicated heading "## Streams"; update the
code fence around FileDownloadResult to include a language (e.g., ```text) and
rename the duplicate "## Streams" heading to a distinct title like "##
Attachment Streams" to avoid anchor conflicts; ensure you update both
occurrences where FileDownloadResult and the duplicate heading appear and keep
the schema name FileDownloadResult unchanged.
- Around line 5-7: The spec contradicts itself about which key encrypts
attachments: the top-level text states P2P chat uses sharedKey (ECDH) and
admin/dispute chat uses tradeKey (BIP-32), but the send_file description says
“key from shared trade key,” creating ambiguity; update the send_file and any
other references (e.g., lines 64-65) to explicitly name the correct key
(sharedKey for peer-to-peer messages, tradeKey for admin/dispute messages),
reference NIP-59 Gift Wrapped for attachment formatting, and ensure all
occurrences consistently use the exact identifiers sharedKey or tradeKey so
implementers know which key (ECDH-derived sharedKey vs BIP-32-derived tradeKey)
to use for attachment encryption.
In `@specs/001-mostro-p2p-client/contracts/nostr.md`:
- Around line 84-90: The fenced code block that shows the MostroSettings struct
(containing mostro_pubkey, expiration_hours, expiration_seconds) is missing a
language identifier; update the opening fence to include a language (e.g.,
change ``` to ```text) so the block becomes a fenced code block with a language
identifier and satisfies MD040.
In `@specs/001-mostro-p2p-client/contracts/orders.md`:
- Around line 182-186: The TradeTimeoutInfo struct currently uses OrderStatus
for its state field which is incorrect for active trade timeout progression;
update the schema so TradeTimeoutInfo { trade_id: String, seconds_remaining:
u32, state: TradeState } (replace OrderStatus with TradeState), and regenerate
any API models/docs or adjust consumers that reference TradeTimeoutInfo to use
the TradeState type instead; ensure the identifier TradeTimeoutInfo and the
field name state are the ones changed.
In `@specs/001-mostro-p2p-client/contracts/reputation.md`:
- Around line 49-54: Add fence language identifiers to the Markdown code blocks
that describe the RatingInfo and RatingReceivedEvent structures so they pass
lint MD040; locate the blocks that list fields like "trade_id", "score",
"is_mine", "created_at" (RatingInfo) and the block with "trade_id", "score",
"from_pubkey" (RatingReceivedEvent) and change their opening triple-backtick to
include a language token (e.g., ```text) for both occurrences (also update the
similar blocks at lines noted in the comment).
In `@specs/001-mostro-p2p-client/data-model.md`:
- Around line 250-252: The spec currently lists encryption_key as stored
plaintext; change the data model so that raw symmetric keys are never persisted:
replace or augment the encryption_key field with an encrypted key blob (e.g.,
encryption_key_encrypted or key_material_blob) plus derivation/metadata fields
(KDF/salt/iter/count, key_id, kms_reference or wrapping_key_id) and keep the
plaintext symmetric key only in-memory at runtime; also clarify encryption_nonce
remains a stored 12-byte nonce and ensure download_status stays
unchanged—document that decryption requires the encrypted key blob + metadata
and that plaintext keys must be ephemeral.
In `@specs/001-mostro-p2p-client/research.md`:
- Around line 110-123: The fenced ASCII state-machine block beginning with
"Pending" and showing states like WaitingBuyerInvoice, WaitingPayment, Active,
FiatSent, SettledHoldInvoice, Success, Dispute, Expired, Canceled, and
CooperativelyCanceled lacks a language tag; add "text" to the opening
triple-backtick fence (i.e., change ``` to ```text) so the code fence is
properly annotated and markdownlint MD040 is satisfied.
In `@specs/001-mostro-p2p-client/spec.md`:
- Around line 20-25: The numbered acceptance scenarios are out of
sequence—specifically the line beginning "Given a buyer with a connected NWC
wallet, When a hold invoice is presented..." is labeled 6 but should be
renumbered so the list reads 1–6 sequentially; update the numeric prefixes so
the current "6." becomes "3.", shift the existing "3.", "4.", "5." to "4.",
"5.", "6." respectively, and verify the full sequence of scenario headers is 1
through 6.
In `@specs/001-mostro-p2p-client/tasks.md`:
- Line 387: The line stating "No test tasks generated" is contradictory with
defined test tasks T098–T103; update the note to accurately reflect that test
tasks exist (e.g., replace the sentence with a brief summary like "Test tasks
T098–T103 defined" or remove the incorrect sentence), and ensure any surrounding
documentation or tooling hints reference the actual tasks T098–T103 so reviewers
and execution tooling aren't misled; locate the phrase "No test tasks generated"
and change it to match the presence of T098–T103.
---
Nitpick comments:
In @.claude/commands/speckit.checklist.md:
- Line 241: The compound modifier "rate limiting" in the checklist string should
be hyphenated; locate the checklist entry containing the exact text "Are rate
limiting requirements quantified with specific thresholds? [Clarity]" and change
it to "Are rate-limiting requirements quantified with specific thresholds?
[Clarity]" so the compound modifier precedes the noun correctly.
In @.claude/commands/speckit.implement.md:
- Line 25: The markdown contains unlabeled fenced code blocks using plain triple
backticks (```); update each unlabeled fence in
.claude/commands/speckit.implement.md (instances around the current block and
also at the locations noted: lines ~36, ~180, ~191) to include a language
identifier (use "text" for these plain examples) so fences become ```text;
ensure the matching closing fences remain unchanged and that indentation/spacing
is preserved for the blocks around the examples referenced by the diff markers.
In @.claude/commands/speckit.taskstoissues.md:
- Around line 14-30: The outline is ambiguous about which tasks to create issues
for and risks duplicates; update the spec to explicitly define task-filtering
behavior (e.g., only unchecked markdown tasks '- [ ]', all tasks, or tasks
matching labels/phases) and add deduplication rules (e.g., match by task text or
embedded task ID) before the MCP create step so the MCP/GitHub issue creation
loop only processes the intended subset; reference the existing artifacts
FEATURE_DIR, AVAILABLE_DOCS and the tasks list produced by
.specify/scripts/bash/check-prerequisites.sh, and document/configure a flag or
parameter used by the Git remote / MCP server step to control filtering and
dedupe logic.
In @.specify/CURRENT_FEATURES.md:
- Line 33: The blob structure line in .specify/CURRENT_FEATURES.md uses square
brackets which Markdown treats as link references; update the "Blob structure:
[nonce:12][encrypted_data][auth_tag:16]" line to use inline code formatting
(wrap the entire blob specification in backticks, e.g. `[...]`) so the notation
is preserved literally and not parsed as links.
In @.specify/scripts/bash/setup-plan.sh:
- Around line 41-50: The current template copy unconditionally overwrites
$IMPL_PLAN; modify the setup logic in this script to first check whether the
target plan file ($IMPL_PLAN) already exists and, if so, avoid silently
overwriting it (options: skip copy and echo a warning, create a timestamped
backup, or prompt the user); implement the check around the cp "$TEMPLATE"
"$IMPL_PLAN" block (use the existing TEMPLATE variable and resolve_template call
to locate the template) and ensure the script logs the chosen action (skipped,
backed up, or overwritten) so users won’t lose existing plan content
unexpectedly.
In @.specify/scripts/bash/update-agent-context.sh:
- Line 436: The variable file_ended is declared as "local file_ended=false" but
never used; remove this unused local variable declaration from the script
(search for "file_ended" and delete the "local file_ended=false" line), ensuring
no other code depends on it and run the script's lint/tests to confirm no
references remain.
- Line 512: The variable changes_entries_added is assigned but never used;
remove its declaration and both assignments to clean up dead code in the script
(or, if intended for reporting, actually increment/use it and emit a summary
message). Locate the symbol changes_entries_added in the update-agent-context.sh
script and either delete all references/assignments to it or implement logic to
update and print its value (e.g., increment where entries are added and echo a
summary at the end) so the variable is meaningful.
In `@specs/001-mostro-p2p-client/contracts/identity.md`:
- Around line 16-21: The fenced code blocks in this file lack a language
identifier; update the blocks that define IdentityCreationResult and the similar
structures at the other two locations to include a language tag (e.g., use
`text` or `rust`) so linters and syntax highlighters work correctly—specifically
add the language identifier to the block containing the IdentityCreationResult
structure and to the fenced blocks at the other two occurrences referenced
(around the code near lines 111 and 139).
In `@specs/001-mostro-p2p-client/contracts/types.md`:
- Around line 11-97: Add a language specifier (e.g., "text" or "plaintext") to
each fenced code block in this file so the linter recognizes them as
non-executable schema blocks; update every block such as the Buy | Sell block
and the blocks labeled OrderStatus, TradeRole, BuyerStep, SellerStep, TradeStep,
TradeOutcome, MessageType, DisputeStatus, DisputeResolution, RelayStatus,
ConnectionState, QueuedMessageStatus, CooperativeCancelState, FileType,
DownloadStatus, and WalletStatus by changing the opening ``` to ```text (or
```plaintext) while keeping the contents unchanged.
In `@specs/001-mostro-p2p-client/spec.md`:
- Line 285: Update the wording of requirement FR-027 to hyphenate the compound
modifier: change the phrase "NWC compatible wallet" to "NWC-compatible wallet"
in the FR-027 line so the compound modifier correctly precedes the noun; ensure
the exact requirement identifier "FR-027" and the phrase are updated in the
specs/001-mostro-p2p-client/spec.md document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edeb6f9e-4821-4502-bc52-91fe82af98bc
📒 Files selected for processing (38)
.claude/commands/speckit.analyze.md.claude/commands/speckit.checklist.md.claude/commands/speckit.clarify.md.claude/commands/speckit.constitution.md.claude/commands/speckit.implement.md.claude/commands/speckit.plan.md.claude/commands/speckit.specify.md.claude/commands/speckit.tasks.md.claude/commands/speckit.taskstoissues.md.specify/CURRENT_FEATURES.md.specify/memory/constitution.md.specify/scripts/bash/check-prerequisites.sh.specify/scripts/bash/common.sh.specify/scripts/bash/create-new-feature.sh.specify/scripts/bash/setup-plan.sh.specify/scripts/bash/update-agent-context.sh.specify/templates/agent-file-template.md.specify/templates/checklist-template.md.specify/templates/constitution-template.md.specify/templates/plan-template.md.specify/templates/spec-template.md.specify/templates/tasks-template.mdCLAUDE.mdspecs/001-mostro-p2p-client/checklists/requirements.mdspecs/001-mostro-p2p-client/contracts/disputes.mdspecs/001-mostro-p2p-client/contracts/identity.mdspecs/001-mostro-p2p-client/contracts/messages.mdspecs/001-mostro-p2p-client/contracts/nostr.mdspecs/001-mostro-p2p-client/contracts/nwc.mdspecs/001-mostro-p2p-client/contracts/orders.mdspecs/001-mostro-p2p-client/contracts/reputation.mdspecs/001-mostro-p2p-client/contracts/types.mdspecs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/plan.mdspecs/001-mostro-p2p-client/quickstart.mdspecs/001-mostro-p2p-client/research.mdspecs/001-mostro-p2p-client/spec.mdspecs/001-mostro-p2p-client/tasks.md
…view - Fill plan.md with technical context, constitution check, and project structure - Fix shell scripts: safe whitespace trim, branch number validation, spec dir dedup, split local+assign for proper error propagation, remove unused vars - Add markdown fence language identifiers across all contract files - Fix spec acceptance scenario numbering (1-6 sequential) - Replace plaintext encryption_key with encrypted key blob in data-model - Fix TradeTimeoutInfo.state type from OrderStatus to TradeStep - Clarify sharedKey vs tradeKey usage for attachment encryption - Rename duplicate Streams heading to Attachment Streams in messages contract - Clean up CLAUDE.md: remove template tokens, deduplicate entries - Add overwrite protection to setup-plan.sh and create-new-feature.sh - Add task filtering and dedup rules to taskstoissues command - Fix contradictory "No test tasks generated" note (T098-T103 exist) - Dynamic phase detection in implement command instead of hardcoded names - Fix Phase 2 reference to Phase 1 in plan command - Resolve checklist creation policy contradiction in specify command
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
specs/001-mostro-p2p-client/data-model.md (2)
284-304: Add language identifier to relationships code block.Same MD040 issue as above.
📝 Suggested fix
## Relationships -``` +```text Identity (1) ──── (*) Trade🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/data-model.md` around lines 284 - 304, The fenced code block showing the entity relationships (lines containing Identity, Trade, Order, Message, Dispute, FileAttachment, Rating, NwcWallet, Relay, Settings, MessageQueue) must include a language identifier to satisfy MD040; update the opening fence from ``` to ```text (or another appropriate language) so the block is ```text Identity (1) ──── (*) Trade ... ``` and save.
58-72: Add language identifier to fenced code block.The state machine diagram code block lacks a language identifier, which triggers markdownlint MD040.
📝 Suggested fix
**State machine**: -``` +```text Pending ├─→ WaitingBuyerInvoice🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/data-model.md` around lines 58 - 72, The fenced code block under the "State machine" section is missing a language identifier which triggers markdownlint MD040; update the opening triple-backtick that currently appears before the state diagram to include a language identifier (e.g., ```text or ```dot) so the block becomes ```text (or another appropriate identifier) and leave the block contents unchanged; look for the "State machine" heading and the triple-backtick-delimited diagram to apply the change..specify/scripts/bash/create-new-feature.sh (1)
144-147: Consider separating declaration and assignment to preserve return values.ShellCheck SC2155: When
localand command substitution are combined, the exit status of the subshell is masked bylocal's success. Ifget_highest_from_branchesorget_highest_from_specsever returns non-zero to signal an error, it would be silently ignored.♻️ Suggested fix
- local highest_branch=$(get_highest_from_branches) + local highest_branch + highest_branch=$(get_highest_from_branches) - local highest_spec=$(get_highest_from_specs "$specs_dir") + local highest_spec + highest_spec=$(get_highest_from_specs "$specs_dir")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/create-new-feature.sh around lines 144 - 147, Declare the variables first and assign the command substitutions in separate statements to avoid masking the commands' exit statuses: change the combined "local highest_branch=$(get_highest_from_branches)" and "local highest_spec=$(get_highest_from_specs "$specs_dir")" into separate declaration and assignment for highest_branch and highest_spec so that failures from get_highest_from_branches and get_highest_from_specs are preserved and can be checked (referencing functions get_highest_from_branches, get_highest_from_specs and variables highest_branch, highest_spec)..claude/commands/speckit.taskstoissues.md (1)
27-29: Consider specifyingghCLI availability check.The command uses
gh issue listfor deduplication (line 28) and impliesghCLI availability, but doesn't specify how to handle cases where the GitHub CLI is not installed or not authenticated. Adding a prerequisite check would prevent confusing failures.📝 Suggested addition after line 23
> [!CAUTION] > ONLY PROCEED TO NEXT STEPS IF THE REMOTE IS A GITHUB URL +1. **Verify GitHub CLI availability**: Run `gh auth status` to confirm the CLI is installed and authenticated. If not authenticated, prompt the user to run `gh auth login` first. + 1. **Filter tasks**: Only process unchecked tasks (`- [ ]`). Skip tasks already marked complete (`- [x]` or `- [X]`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/speckit.taskstoissues.md around lines 27 - 29, Add a prerequisite check for the GitHub CLI before using "gh issue list": verify the "gh" binary is available and authenticated (e.g., attempt "gh auth status" or check PATH), and if not present or unauthenticated, fail fast with a clear message or fall back to an alternative (call the GitHub API or skip deduplication). Update the documentation around the "gh issue list" deduplication step to state this requirement and the expected behavior when "gh" is missing or not logged in so the speckit.taskstoissues workflow doesn't error silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/speckit.specify.md:
- Around line 75-77: The PowerShell example incorrectly references the Bash
script `create-new-feature.sh` and uses Bash-style flags; update the example to
either remove the PowerShell variant or show a correct PowerShell invocation
that runs the Bash script via WSL/Git Bash and preserves the Bash flags (e.g.,
call the script through `wsl`/`bash -c` or explicitly invoke the shell) and use
`--json` and `--short-name` (not `-Json`/`-ShortName`), or replace it with a
true PowerShell-native example; locate the `create-new-feature.sh` example and
change the second line accordingly.
In @.specify/scripts/bash/update-agent-context.sh:
- Line 514: The variable changes_entries_added is assigned but never used;
remove its declaration/assignment to clean up dead code (search for the symbol
changes_entries_added in update-agent-context.sh and delete the line setting it
and any related unused references), or if the original intent was to track added
change entries mirror the pattern used by tech_entries_added by setting and
checking changes_entries_added in the same validation/conditional
blocks—preferably remove the unused variable assignment to keep the script tidy.
---
Nitpick comments:
In @.claude/commands/speckit.taskstoissues.md:
- Around line 27-29: Add a prerequisite check for the GitHub CLI before using
"gh issue list": verify the "gh" binary is available and authenticated (e.g.,
attempt "gh auth status" or check PATH), and if not present or unauthenticated,
fail fast with a clear message or fall back to an alternative (call the GitHub
API or skip deduplication). Update the documentation around the "gh issue list"
deduplication step to state this requirement and the expected behavior when "gh"
is missing or not logged in so the speckit.taskstoissues workflow doesn't error
silently.
In @.specify/scripts/bash/create-new-feature.sh:
- Around line 144-147: Declare the variables first and assign the command
substitutions in separate statements to avoid masking the commands' exit
statuses: change the combined "local
highest_branch=$(get_highest_from_branches)" and "local
highest_spec=$(get_highest_from_specs "$specs_dir")" into separate declaration
and assignment for highest_branch and highest_spec so that failures from
get_highest_from_branches and get_highest_from_specs are preserved and can be
checked (referencing functions get_highest_from_branches, get_highest_from_specs
and variables highest_branch, highest_spec).
In `@specs/001-mostro-p2p-client/data-model.md`:
- Around line 284-304: The fenced code block showing the entity relationships
(lines containing Identity, Trade, Order, Message, Dispute, FileAttachment,
Rating, NwcWallet, Relay, Settings, MessageQueue) must include a language
identifier to satisfy MD040; update the opening fence from ``` to ```text (or
another appropriate language) so the block is ```text Identity (1) ──── (*)
Trade ... ``` and save.
- Around line 58-72: The fenced code block under the "State machine" section is
missing a language identifier which triggers markdownlint MD040; update the
opening triple-backtick that currently appears before the state diagram to
include a language identifier (e.g., ```text or ```dot) so the block becomes
```text (or another appropriate identifier) and leave the block contents
unchanged; look for the "State machine" heading and the
triple-backtick-delimited diagram to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40aa54a6-a943-4400-8213-8dbeb42365a8
📒 Files selected for processing (23)
.claude/commands/speckit.checklist.md.claude/commands/speckit.implement.md.claude/commands/speckit.plan.md.claude/commands/speckit.specify.md.claude/commands/speckit.taskstoissues.md.specify/CURRENT_FEATURES.md.specify/DESIGN_SYSTEM.md.specify/scripts/bash/create-new-feature.sh.specify/scripts/bash/setup-plan.sh.specify/scripts/bash/update-agent-context.sh.specify/templates/plan-template.mdCLAUDE.mdspecs/001-mostro-p2p-client/contracts/identity.mdspecs/001-mostro-p2p-client/contracts/messages.mdspecs/001-mostro-p2p-client/contracts/nostr.mdspecs/001-mostro-p2p-client/contracts/orders.mdspecs/001-mostro-p2p-client/contracts/reputation.mdspecs/001-mostro-p2p-client/contracts/types.mdspecs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/plan.mdspecs/001-mostro-p2p-client/research.mdspecs/001-mostro-p2p-client/spec.mdspecs/001-mostro-p2p-client/tasks.md
✅ Files skipped from review due to trivial changes (14)
- CLAUDE.md
- .specify/DESIGN_SYSTEM.md
- .specify/scripts/bash/setup-plan.sh
- specs/001-mostro-p2p-client/contracts/reputation.md
- .specify/templates/plan-template.md
- specs/001-mostro-p2p-client/contracts/types.md
- specs/001-mostro-p2p-client/plan.md
- specs/001-mostro-p2p-client/contracts/nostr.md
- .claude/commands/speckit.plan.md
- .specify/CURRENT_FEATURES.md
- specs/001-mostro-p2p-client/research.md
- specs/001-mostro-p2p-client/contracts/orders.md
- specs/001-mostro-p2p-client/contracts/identity.md
- specs/001-mostro-p2p-client/contracts/messages.md
Comment on lines
+75
to
+77
| - Bash example: `.specify/scripts/bash/create-new-feature.sh "$ARGUMENTS" --json --short-name "user-auth" "Add user authentication"` | ||
| - PowerShell example: `.specify/scripts/bash/create-new-feature.sh "$ARGUMENTS" -Json -ShortName "user-auth" "Add user authentication"` | ||
|
|
Contributor
There was a problem hiding this comment.
PowerShell example references a Bash script.
Line 76 shows a PowerShell example with -Json and -ShortName flags, but the script is create-new-feature.sh (a Bash script). PowerShell would use different syntax to invoke a shell script, and the flag style shown doesn't match the Bash script's argument parser.
Consider either removing the PowerShell example or clarifying that PowerShell users should invoke the Bash script via WSL/Git Bash with the standard --json flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/speckit.specify.md around lines 75 - 77, The PowerShell
example incorrectly references the Bash script `create-new-feature.sh` and uses
Bash-style flags; update the example to either remove the PowerShell variant or
show a correct PowerShell invocation that runs the Bash script via WSL/Git Bash
and preserves the Bash flags (e.g., call the script through `wsl`/`bash -c` or
explicitly invoke the shell) and use `--json` and `--short-name` (not
`-Json`/`-ShortName`), or replace it with a true PowerShell-native example;
locate the `create-new-feature.sh` example and change the second line
accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Mar 23, 2026
4 tasks
This was referenced Jul 24, 2026
codaMW
pushed a commit
to codaMW/app
that referenced
this pull request
Jul 30, 2026
…form
First phase of docs/cashu/README.md. Pure dependency PR, as specified: no
features, no behaviour change, nothing Cashu-related is reachable at runtime.
0.13.1 -> 0.14.1 compiles with zero source changes on native and wasm; the
suite passes unmodified. What 0.14 adds is the frozen Cashu protocol surface:
Action::{AddCashuEscrow, CashuEscrowLocked, CashuPmSignature},
Payload::{CashuLockProof, CashuSignatures} and five CantDoReason variants.
The phase's other deliverable was to verify the wire form rather than trust
§2's illustrative JSON. Done, and pinned as tests (rust/src/mostro/cashu_wire.rs)
so an upstream rename fails our suite instead of a live trade: Action is
kebab-case, Payload snake_case (so the discriminator is `cashu_lock_proof`),
CashuLockProof's field names are as documented, and fee_token is
skip_serializing_if=Option::is_none — a node charging no fee produces the
pre-0.14 form byte-for-byte. §2's example turned out to be accurate.
One finding changes the plan. The escrow request (Mostro -> seller after a
take) is not merely undocumented, it is absent from mostro-core 0.14.1: the
Cashu fields live on the daemon-internal Order and NOT on SmallOrder, which is
what payloads actually carry, and there is no payload variant for the request.
So C5's seller side cannot be implemented without inventing a wire format.
A test asserts this, so the day upstream adds the carrier the suite goes red
and points at the decision. Risk MostroP2P#1 updated from "not yet published" to
"confirmed absent", with the blast radius scoped: only C5's seller side.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores