feat: add POST /api/notify with dual-keyed rate limiting (v1.1)#1
Open
AndreaDiazCorreia wants to merge 46 commits intomainfrom
Open
feat: add POST /api/notify with dual-keyed rate limiting (v1.1)#1AndreaDiazCorreia wants to merge 46 commits intomainfrom
AndreaDiazCorreia wants to merge 46 commits intomainfrom
Conversation
…c<dyn>]>
- Add src/push/dispatcher.rs with PushDispatcher, DispatchOutcome,
DispatchError. The dispatcher owns Arc<[Arc<dyn PushService>]> + a
parallel Arc<[&'static str]> for backend names, and exposes a single
async dispatch(&RegisteredToken) method that replicates the listener's
prior iteration protocol byte-for-byte (skip non-matching, attempt first
matching, break on first Ok). No log lines inside the dispatcher (D-07).
- Tighten PushService::send_to_token return type to
Box<dyn std::error::Error + Send + Sync> (D-09); cascade through FcmPush
and UnifiedPushService impls and the two blanket Arc<...> impls. Drop
the .map_err Box<dyn Error> workaround at fcm.rs that bridged the
mismatched bound.
- Delete the unused send_silent_push trait method and both concrete impls
(D-10). The method was never called from production code.
- Wire dispatcher into main.rs: build Vec<(Arc<dyn PushService>, &'static
str)> tagged with backend names ("fcm" / "unifiedpush"), wrap in
Arc::new(PushDispatcher::new(...)), drop the tokio::sync::Mutex import,
and pass dispatcher.clone() into NostrListener::new. AppState is
unchanged (D-16; Phase 2 owns that seam).
- Replace the inline iteration loop in listener.rs (previously
push_services.lock().await + for service in services.iter()) with a
single dispatcher.dispatch(®istered_token).await + match on
DispatchOutcome / DispatchError. Log shape is preserved verbatim per
D-08: same info!("Push sent successfully for event {}", event.id) on
success, same error!("Failed to send push: {}", err) per failed
backend.
- Add an explicit anti-CRIT-1 block comment above Filter::new() in
listener.rs (D-11), citing NIP-59 ephemeral keys, admin DMs being sent
directly user-to-user, and PROJECT.md OOS-19 / PITFALLS CRIT-1. This is
the highest-leverage location to prevent a future contributor from
"fixing" the dormant MOSTRO_PUBKEY field by applying it as a filter.
- MOSTRO_PUBKEY config field and its startup validation are explicitly
preserved per D-12. AppState, src/api/, src/store/, src/config.rs,
src/crypto/, src/utils/, Cargo.toml, deploy-fly.sh, and fly.toml are
untouched.
No observable behaviour change: the existing Mostro daemon -> silent push
flow runs through the same trait surface with the same log shape; reads
on the services list become lock-free.
Phase: 01-pushdispatcher-refactor-no-behaviour-change
Plan: 01-01
Requirements satisfied: DISPATCH-01, DISPATCH-02
- Add 01-01-SUMMARY.md documenting the refactor (files modified, decisions D-01..D-16 applied, plan-level must_haves and 11 must-not clauses verified, manual smoke test procedure pending operator). - Mark Phase 1 complete in ROADMAP and STATE (1/1 plans done). - Mark DISPATCH-01 and DISPATCH-02 requirements complete in REQUIREMENTS.md. - Update STATE.md Session Continuity to point at the operator's manual smoke test and `/gsd-plan-phase 2` as the next action. Implementation commit: a43aa49.
Construct a single reqwest::Client at startup with connect_timeout=2s, timeout=5s, pool_idle_timeout=90s. Pass Arc<reqwest::Client> to FcmPush and UnifiedPushService instead of letting each construct its own Client::new(). This bounds outbound calls to FCM and UnifiedPush so a hung remote endpoint cannot saturate tokio worker threads under sustained load. Resolves CONCERNS.md "reqwest::Client::new() per service" item and prepares for Phase 2's spawn-detached /api/notify dispatch path. Per phase 02 decisions D-07 + D-08. No behavioural change to either dispatch path; existing Mostro daemon -> silent push flow byte-identical.
Add 02-01-SUMMARY.md, update STATE.md (Phase 02 Plan 01 complete, metrics + decisions recorded), update ROADMAP.md progress table (Phase 1 1/1 Complete, Phase 2 1/3 Executing).
Add a sender-triggered POST /api/notify endpoint that accepts
{ trade_pubkey } and dispatches a silent push to the registered device
via PushDispatcher::dispatch_silent. Bundle the privacy hardening this
endpoint requires: salted-BLAKE3 log_pubkey() helper, server-side
UUIDv4 X-Request-Id middleware scoped to /notify only, separate FCM
silent payload builder (apns-priority 5, apns-push-type background),
bounded tokio::spawn dispatch with a 50-permit Semaphore, and the
deploy-fly.sh RUST_LOG=info flip required to keep the privacy posture
consistent in production.
Endpoint contract is always-202 on parse-valid input regardless of
registration status (anti-CRIT-2 enumeration oracle); 400 only on JSON
parse / pubkey-validation failure. Mobile-team coordination on the
contract deviation from CHAT_NOTIFICATIONS_PLAN.md Phase 4 is handled
out of band per phase 02 D-04.
PushDispatcher gains dispatch_silent that mirrors dispatch but routes
through the new send_silent_to_token trait method. UnifiedPush relies
on the default delegation; FcmPush overrides with the silent payload.
Existing dispatch (called by src/nostr/listener.rs) is byte-identical;
src/nostr/listener.rs is not touched.
Closes NOTIFY-01, NOTIFY-02, NOTIFY-03, NOTIFY-04, PRIV-01, PRIV-02,
PRIV-03 from phase 02. VERIFY-03 ships in plan 02-03 as a runbook.
Existing /api/register, /api/unregister, /api/health, /api/info,
/api/status request and response shapes are byte-identical (COMPAT-1).
New deps: blake3 = "1", uuid = "1" with v4 feature (both pre-approved
per phase 02 D-16 and D-21). actix-web bumped from "4.4" to "4.9"
floor for explicit middleware::from_fn availability (D-20).
Add 02-02-SUMMARY.md documenting the atomic commit d01dc97 that shipped the always-202 POST /api/notify endpoint with the full privacy-hardening bundle (D-05/D-09/D-10/D-11/D-12/D-13/D-14/D-15/ D-16/D-20/D-21/D-22). Update STATE.md, ROADMAP.md, and REQUIREMENTS.md to reflect Plan 02-02 completion: 7 requirements closed (NOTIFY-01..04 + PRIV-01..03), Phase 02 progress at 2/3 plans. Manual smoke on Fly.io staging is pending operator action.
Add docs/verification/dispute-chat.md — a Spanish operator runbook that documents how to verify end-to-end that admin DMs (sent directly user-to-user, NOT routed through the Mostro daemon) reach registered devices as silent pushes via the existing Nostr-listener path. The runbook reinforces the anti-CRIT-1 anti-fix at the operator level: no .authors(mostro_pubkey) filter must ever be added to src/nostr/listener.rs Filter::new() chain. Includes the bash grep one-liner an operator runs after every deploy to confirm the anti-fix has not been re-introduced. This is the sole deliverable for VERIFY-03 in phase 02. No source code changes; no test code; manual runbook only per phase 02 D-18. Phase 3 will introduce the in-process integration test harness for /api/notify (VERIFY-01) but the dispute-chat path stays manual — Apple/FCM delivery decisions cannot be unit-tested.
SC #5 literal satisfaction: route the shared notify_log_salt through TokenStore and NostrListener so register/unregister/store-lifecycle and Nostr event-recipient/match log lines emit 8-char keyed-BLAKE3 correlators instead of 16-char hex pubkey prefixes. This supersedes the Phase 2 D-14 narrowing (which preserved the legacy prefix logs for operator grep continuity). With the same salt shared across modules, a single trade_pubkey produces the same pk=<8hex> correlator everywhere — preserving log correlation without leaking recognisable pubkey identifiers under RUST_LOG=info. Sites migrated: - src/api/routes.rs:95,141,156 (register/unregister handlers) - src/store/mod.rs:60,72,78 (TokenStore.register/unregister) - src/nostr/listener.rs:110,116,137 (Event recipient + MATCH + miss) TokenStore::new and NostrListener::new now take Arc<[u8; 32]> log_salt. Salt construction moved earlier in main.rs so all consumers share it.
Capture the two-plan breakdown for phase 03 (dual-keyed rate limiting and verification harness) along with the patterns map and sync STATE.md, ROADMAP.md, and config.json before execution starts.
…lpers - Add PerPubkeyLimiter and PerIpLimiter type aliases over governor::DefaultKeyedRateLimiter - Add PUBKEY_BURST (10) and IP_BURST (30) compile-time constants (D-01, D-02) - Add rate_limited_response helper for byte-identical 429 body (D-13, D-14) - Add extract_client_ip with Fly-Client-IP -> rightmost-XFF -> peer_addr precedence (D-10) - Add per_ip_rate_limit_mw middleware with fail-closed 500 on missing IP (D-11) - Add check_soft_cap helper for unit-testable LIMIT-06 warn-emission path - Add start_rate_limit_cleanup_task mirroring store::start_cleanup_task (D-15) - Re-export module via src/api/mod.rs
- Add NotifyRateLimitConfig struct with four fields (per_pubkey_per_min, per_ip_per_min, cleanup_interval_secs, pubkey_limiter_soft_cap) - Append notify_rate_limit field to Config (LIMIT-04: existing fields untouched) - Load four env vars in Config::from_env with info! log when defaults are used (D-03) - Reject zero values for the two rate fields with descriptive error (D-04) - Co-locate rejects_zero unit tests with Mutex serialization to prevent env races
- Add per_pubkey_limiter field to AppState (D-09); existing fields untouched - Wrap /api/notify with per_ip_rate_limit_mw inside request_id_mw (D-19, D-21) - Insert per-pubkey check in notify_token between log_pk emission and semaphore acquire; uses shared rate_limited_response for byte-identical 429 (D-12, D-13) - Construct PerPubkeyLimiter and PerIpLimiter in main.rs with config-driven quotas and compile-time burst constants (D-01, D-02, D-09, D-20) - Spawn start_rate_limit_cleanup_task after limiter construction (D-15) - Register per_ip_limiter via app_data on HttpServer closure (D-20)
- Add 03-01-SUMMARY.md with decisions D-01..D-29, deviations, test results - Update STATE.md: progress 83%, metrics, decisions, session continuity - Mark LIMIT-01..LIMIT-06 complete in REQUIREMENTS.md
…p factory - StubPushService implements PushService, records calls in Arc<Mutex<Vec<(String, Platform)>>> - make_app_state() builds AppState + PerIpLimiter from a stub, no actix service - make_test_components() + build_test_actix_app() factory pair avoids annotating the opaque actix_http::Request type without adding actix-http as a direct dep - make_test_app! macro wraps init_service call for single-line test setup - seed_hex_pubkey(n) generates 64-hex pubkeys for per-IP pubkey rotation tests - Module gated #[cfg(test)] in api/mod.rs; absent from release builds
…4 regression - notify_registered_pubkey_dispatches: 202 + stub records 1 call (D-24 #1) - notify_unregistered_pubkey_no_dispatch: 202 + zero stub calls, anti-CRIT-2 (D-24 #2) - notify_malformed_body_returns_400: 3 sub-cases (short, non-hex, missing field) (D-24 #3) - notify_x_request_id_always_uuidv4_and_overwrites_client_value: header present on 202 and 400, client-supplied value overwritten (D-25 NOTIFY-04)
…etain_recent, and rightmost-XFF - per_ip_burst_exhaustion_returns_429: rotates pubkeys (burst 30 per-IP), iter >= IP_BURST (D-24 #5) - per_pubkey_burst_exhaustion_returns_429: rotates IPs, iter <= PUBKEY_BURST (D-24 #4) - rate_limited_429_body_byte_identical_per_ip_vs_per_pubkey: non-tautological both halves, body locked - retain_recent_reduces_len_with_fake_clock: FakeRelativeClock + advance(120s), no tokio::time (D-27) - check_soft_cap_fires_for_real_limiter_above_cap: real PerPubkeyLimiter, above-cap + boundary - rightmost_xff_used_when_fly_client_ip_missing: anti-CRIT-4, XFF rightmost is the rate-limit key - Fixed: use actix_web::test aliased as atest to avoid shadowing #[test] attribute - Fixed: RateLimiter type annotation includes NoOpMiddleware<<FakeRelativeClock as Clock>::Instant>
…PLOY-3 regression
- register_success_body_is_byte_identical: RegisterResponse field order locked (D-24 #6)
- register_malformed_pubkey_body_is_byte_identical: 400 error body locked
- unregister_not_found_body_is_byte_identical: not-found body locked
- unregister_success_body_is_byte_identical: success body locked
- health_body_is_byte_identical: {"status":"ok"} locked
- info_body_is_byte_identical: version + encryption_enabled + note locked
- status_body_shape_is_byte_identical: running + version + tokens(0,0,0) locked
- health_endpoint_not_rate_limited_1000_burst: 1000x 200 anti-DEPLOY-3 structural lock
- other_endpoints_not_rate_limited_under_burst: register/info/status bypass per-IP mw (LIMIT-03)
…01+VERIFY-02 closed - 03-02-SUMMARY.md: 4 tasks, 30 tests passing, 3 auto-fixed deviations documented - STATE.md: progress 100%, metrics and decisions recorded, session updated - REQUIREMENTS.md: VERIFY-01 and VERIFY-02 marked complete
…est-id WR-01: actix-web wraps middlewares in reverse-registration order, so the last `.wrap()` call is outermost. The previous order put per_ip_rate_limit_mw outermost, meaning per-IP 429 short-circuited before request_id_mw could run and the response missed x-request-id while per-pubkey 429 carried it. That header drift is exactly the anti-RL-2 oracle the byte-identical 429 contract exists to prevent. Swap the two .wrap() calls so request_id_mw is now outermost and runs on every response, including per-IP 429. Add a non-tautological regression test x_request_id_present_on_both_429_paths that captures one 429 from each path and asserts both carry a UUIDv4 x-request-id.
All 6 success criteria verified PASS: - Per-IP 429 with fixed Fly-Client-IP, no XFF bypass - Per-pubkey 429 with byte-identical body across both paths (anti-RL-2) - Other endpoints unaffected (1000-burst /api/health passes) - Quotas configurable; legacy RATE_LIMIT_PER_MINUTE untouched - Soft-cap warn + retain_recent cleanup wired - 31/31 in-process tests green
17/17 requirements satisfied at source level, 4/4 E2E flows wired, 0 critical gaps. Status is tech_debt rather than passed because: - 1 Phase 01 + 7 Phase 02 operator UAT scenarios remain (by design, cannot be asserted from in-process tests) - 3 pre-existing source warnings (WR-02/WR-03/IN-02) flagged by code review but predate this milestone - Nyquist validation not run on any of the 3 phases
Archive ROADMAP, REQUIREMENTS, and MILESTONE-AUDIT into .planning/milestones/v1.1-*.md, move all phase artefacts under .planning/milestones/v1.1-phases/, evolve PROJECT.md with v1.1 outcomes, collapse ROADMAP.md to milestone-grouped index, and seed MILESTONES.md with the shipped-version entry. Audit status at close: tech_debt — 17/17 requirements satisfied, 0 gaps, 3 deferred items acknowledged in STATE.md (1 UAT gap + 2 verification gaps, all human_needed by design).
WalkthroughThe pull request introduces comprehensive post-implementation documentation for the Mostro Push Server v1.1 "Chat Notifications" milestone shipped on 2026-04-26. It captures milestone state, requirements traceability, architectural decisions, three phased execution plans with verification reports, research artifacts, and project conventions in markdown format across the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a38cdb4d18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+56
to
+60
| if let Some(v) = req.headers().get("Fly-Client-IP") { | ||
| if let Ok(s) = v.to_str() { | ||
| if let Ok(ip) = IpAddr::from_str(s.trim()) { | ||
| return Some(ip); | ||
| } |
There was a problem hiding this comment.
This IP extraction path trusts Fly-Client-IP unconditionally before any proxy-derived value. In deployments that are not directly behind Fly edge (or any proxy that forwards client headers unchanged), an attacker can send a different Fly-Client-IP on each /api/notify request and bypass the per-IP limiter by forcing a new bucket every time. Only trust this header when the immediate peer is a trusted Fly proxy, otherwise fall back to trusted forwarding metadata (X-Forwarded-For/peer address).
Useful? React with 👍 / 👎.
Comment on lines
+137
to
+140
| // D-15 + LIMIT-05/06: cleanup task mirrors store::start_cleanup_task. | ||
| api::rate_limit::start_rate_limit_cleanup_task( | ||
| per_pubkey_limiter.clone(), | ||
| Duration::from_secs(config.notify_rate_limit.cleanup_interval_secs), |
There was a problem hiding this comment.
The new cleanup task is started only for per_pubkey_limiter, while per_ip_limiter is also a keyed governor limiter and never gets retain_recent() compaction. On long-running instances with many distinct client IPs, stale per-IP keys will accumulate and grow memory usage over time. Add a periodic cleanup path for the per-IP limiter as well (or a shared cleaner for both keyed limiters).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-DISCUSSION-LOG.md (1)
18-235:⚠️ Potential issue | 🟠 MajorConvert Spanish content to English to meet repo documentation policy.
Large parts of this file (tables/notes) are in Spanish, which breaks the repo’s documentation language requirement and makes future maintenance harder for non-Spanish contributors.
Based on learnings: Code, comments, commit messages, branch names, and documentation in English.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-DISCUSSION-LOG.md around lines 18 - 235, Translate all Spanish content in this document into clear, idiomatic English while preserving table structure, headings (e.g., "Per-IP rate sustained + burst", "Behavior when env vars NOTIFY_RATE_PER_*_PER_MIN are absent or malformed at startup", "Wiring per-pubkey limiter + cleanup", "Claude's Discretion"), selected choices ("User's choice:", "Notes:", D-01..D-28 locks) and bullets exactly as-is; keep numeric values, option labels, and code/config identifiers unchanged (e.g., NOTIFY_RATE_PER_PUBKEY_PER_MIN, RATE_LIMIT_PER_MINUTE, start_rate_limit_cleanup_task(), PerPubkeyLimiter, Arc<DefaultKeyedRateLimiter<String>), update only the natural-language Spanish strings to English, and ensure any inline Spanish words in table cells, notes, or deferred lists are converted to English while preserving formatting and markdown syntax.
🧹 Nitpick comments (5)
.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md (2)
132-139: Add language specifier to grep guards code block.The shell commands should be marked with
shellorbashfor clarity.📝 Suggested improvement
## Grep Guards (all pass) -``` +```shell ! grep -rn "actix-governor|actix_governor" . # PASS (D-05 license invariant) ! grep -n "\.split(',').next()" src/api/rate_limit.rs # PASS (CRIT-4 leftmost-XFF anti-fix) grep -c "\.wrap(" src/api/routes.rs # → 2 (request_id_mw + per_ip_rate_limit_mw)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md around lines 132 - 139, Update the fenced code block that contains the grep commands so it includes a shell language specifier (e.g., change the opening ``` to ```shell) and keep the closing ```; locate the block with the lines matching patterns like actix-governor|actix_governor, "\.split(',').next()", and "\.wrap(" (these unique strings identify the grep snippet) and ensure the opening fence is annotated (shell or bash) for clarity.
124-126: Add language specifier to startup log code block.The startup log sample should have a language identifier for proper syntax highlighting.
📝 Suggested improvement
## Startup Log (sample with defaults) -``` +```log INFO mostro_push_backend > Rate limiters initialized (per-pubkey 30/min burst 10; per-IP 120/min burst 30; cleanup 60s; soft cap 100000)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md
around lines 124 - 126, Add a language specifier to the fenced code block that
contains the startup log sample so it renders with syntax highlighting; locate
the fenced block that wraps the line beginning "INFO mostro_push_backend > Rate
limiters initialized..." and change the opening fence fromtolog (i.e.,
add "log" as the language identifier).</details> </blockquote></details> <details> <summary>.planning/STATE.md (1)</summary><blockquote> `42-46`: **Add language specifier to fenced code block.** The progress bar visualization would benefit from a language specifier (e.g., `text` or `plaintext`) to meet markdown best practices. <details> <summary>📝 Suggested improvement</summary> ```diff **Progress:** [██████████] 100% -``` +```text [████████████████████] 100% Phase 1: PushDispatcher refactor (1/1 plans) [████████████████████] 100% Phase 2: /api/notify endpoint with privacy hardening (3/3 plans) [░░░░░░░░░░░░░░░░░░░░] 0% Phase 3: Dual-keyed rate limiting and verification harness ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.planning/STATE.md around lines 42 - 46, Add a language specifier (e.g.,
"text" or "plaintext") to the fenced code block that contains the progress bar
lines (the block showing "Phase 1: PushDispatcher refactor", "Phase 2:
/api/notify endpoint with privacy hardening", and "Phase 3: Dual-keyed rate
limiting and verification harness") so the markdown follows best practices;
update the opening fence fromtotext (or ```plaintext) for that block in
STATE.md.</details> </blockquote></details> <details> <summary>.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-02-PLAN.md (1)</summary><blockquote> `82-83`: **Unificar el criterio de frontera per-IP (off-by-one documental).** Aquí se documenta `>= IP_BURST + 1 (=31)`, pero en otras secciones del mismo plan y tests se valida `iter >= IP_BURST` con índice 0-based. Conviene dejar un único criterio para evitar ambigüedad en la verificación. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-02-PLAN.md around lines 82 - 83, Unificar la frontera per-IP usando índice 0-based: pick a single criterion (recommended: use IP_BURST as the zero-based first-failing iteration) and update the documentation and all tests that assert the first 429 index to use that same rule; specifically change the textual note that currently says ">= IP_BURST + 1 (=31)" to ">= IP_BURST" (or convert all tests to 1-based consistently), and adjust assertions that compare iteration indices to reference the IP_BURST constant (symbol: IP_BURST) rather than IP_BURST + 1 so per-IP tests assert iter >= IP_BURST and the per-pubkey tests remain unchanged (also ensure rotation logic still uses format!("{:0>64x}", i) for trade_pubkey where applicable). ``` </details> </blockquote></details> <details> <summary>.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md (1)</summary><blockquote> `1127-1141`: **Missing language specifier on fenced code blocks.** The verification section contains fenced code blocks without language specifiers (lines 1127-1141 and 1187-1220). While functional, adding language hints improves readability and enables syntax highlighting. <details> <summary>💡 Suggested improvement</summary> Add language specifiers to the code blocks: ```diff -``` +```text Cargo.lock (auto-regenerated) Cargo.toml ``` And for the commit message block: ```diff -``` +```text feat(api): add POST /api/notify endpoint with privacy hardening ``` </details> Based on static analysis hints. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md
around lines 1127 - 1141, The fenced code blocks in the verification section and
the commit message block lack language specifiers; update the twoblocks that wrap the file list and the commit message to use a language hint (e.g.,text) so they becometext ...; locate the fenced blocks around the
Cargo.lock/Cargo.toml file list and the commit message "feat(api): add POST
/api/notify endpoint with privacy hardening" and add the language specifier to
each opening fence.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In
@.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-01-PLAN.md:
- Line 992: The table row for T-01-02 is malformed (9 columns) and violates the
document's 5-column table schema causing MD056 warnings; edit the line
containing "T-01-02 | Information Disclosure |PushDispatcher::dispatchlogs |
mitigate | D-07 forbids..." to conform to the 5-column structure used elsewhere
(ensure exactly 5 pipe-separated cells: ID, Threat, Artifact, Mitigation,
Notes/Reference), collapse any stray pipes within the Notes/Reference cell
(escape or rephrase inline backticks likePushDispatcher::dispatch,
device_token,trade_pubkey) so they do not create extra columns, and run the
markdown linter to verify the MD056 warning is resolved.In
@.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-CONTEXT.md:
- Around line 16-20: The heading "Dispatch Component Surface" currently uses ###
and is causing a skipped-heading-level MD001 lint; change its Markdown level so
it follows the surrounding heading progression (e.g., adjust "### Dispatch
Component Surface" to the appropriate level such as "## Dispatch Component
Surface" or "####" to match the file's structure), and ensure the subsection
bullet "D-01: New modulesrc/push/dispatcher.rsre-exported via
src/push/mod.rs" remains under that corrected heading.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md:
- Line 311: The table row for T-02-05 is malformed (it contains too many
pipe-separated columns) because it includes extra explanatory text (handler
signature and grep check) that expands the row to 8 columns instead of the 5
defined by the header; fix it by editing that row so it has exactly 5 cells:
either split the extra info into one or more new rows, or move the handler
signature(state: web::Data<AppState>, req: web::Json<NotifyRequest>) -> impl Responderand the grep evidence (! grep -nE '(req\.peer_addr|connection_info|forwarded|HttpRequest)' src/api/notify.rs)
into the "Mitigation Plan" cell (or shorten them to a single concise note), and
ensure pipe separators match the header column count exactly so the markdown
table renders correctly.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-SUMMARY.md:
- Line 296: The summary line declares "a Spanish-language operator runbook" for
docs/verification/dispute-chat.md which violates the repository convention to
keep documentation in English; update the text to state an English-language
operator runbook and ensure the referenced runbook content
(docs/verification/dispute-chat.md) is written or translated into English so
code comments, commit messages, and docs remain consistent with the repository
language policy.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-03-PLAN.md:
- Around line 79-83: Update the stale .planning/phases/... references in this
file to use the milestone directory layout .planning/milestones/v1.1-phases/...
so links and automation won’t break; search for any occurrences of
".planning/phases/02-post-api-notify-endpoint-with-privacy-hardening/02-" (and
the other similar phase paths referenced near the same block) and replace them
with
".planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-"
ensuring the filenames (e.g., 02-CONTEXT.md, 02-RESEARCH.md, 02-PATTERNS.md,
01-01-SUMMARY.md) remain unchanged; also update the additional occurrences noted
(the ones corresponding to the same pattern mentioned around lines 441-447) so
all links point to the milestone layout.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-PATTERNS.md:
- Around line 52-55: Update the pattern map to reflect the current implemented
privacy/logging architecture: remove the claim that log_pubkey() is
"notify-only" and that legacy pubkey-prefix logs are intentionally unmigrated,
and instead document that pubkey/logging migration has been completed and that
logging must never capture sharedKey, peer-to-peer relationships, or sender
identity (the Privacy hard requirement). Edit the sections referencing
log_pubkey() and the legacy prefix-log policy to describe the verified migration
state, the allowed minimal hashing/aggregation patterns for observability, and
explicitly state that the server must not persist or log sharedKey, sender
identity, or P2P relationship metadata; keep references to the verification
artifacts and update the guidance so it matches the implemented behavior and
verification results.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-REVIEW.md:
- Around line 50-55: The fenced code block that shows the two
warning: unused import: \reqwest::Client`` lines lacks a language tag; update the opening
fence fromtotext (or ```bash if preferred) in the markdown where that
block appears so markdownlint MD040 is satisfied and the code block is properly
annotated.- Around line 37-41: Translate the entire milestone report text into English to
match repo conventions, preserving technical terms and references (e.g., "POST
/api/notify", "notify.rs", "routes.rs", BLAKE3,try_acquire_owned(),
Arc<Semaphore>(50), apns headers,deploy-fly.sh, Cargo.toml notes) and keep
the existing findings, warnings, and line references intact; ensure the
translated content maintains the same meaning, tone, and structure (including
the privacy contract and listed non-critical findings) and update any in-file
metadata or comment language markers so the file is fully English and consistent
with repo style.In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-VERIFICATION.md:
- Around line 3-24: The frontmatter and body contain conflicting verification
states: reconcile the metadata fields (verified, re_verified, status, score)
with the narrative so there is a single authoritative status; update either the
frontmatter or the body so both show the same result (e.g., change the body’s
“5/6 with structural gap” text to match the frontmatter’s “6/6” or vice versa),
remove or clarify the duplicated “re_verified” vs “initial verification”
language, and ensure the gaps_resolved list and the narrative (mentions of “6/6
resolved” and “5/6 with structural gap”) reflect the same final state. Also
apply the same fix to the repeated sections indicated around the later text
blocks so all occurrences are consistent.In
@.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-PLAN.md:
- Around line 963-971: The summary output path is wrong: replace the incorrect
target string
".planning/phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md"
with the correct milestone-scoped path
".planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md"
so the phase summary is written under the milestone tree; update the single line
in the document that emits that path (search for the literal
".planning/phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md")
to the new string and run a quick grep to ensure no other occurrences remain.In @.planning/milestones/v1.1-REQUIREMENTS.md:
- Around line 30-37: The requirements checklist shows NOTIFY-01..NOTIFY-04 and
PRIV-01..PRIV-03 marked complete ([x]) while the traceability table lists these
same Phase 2 items as "Pending"; reconcile by deciding the true final status and
updating both places to match (either remove the [x] for those checklist items
or change the traceability entries from "Pending" to "Complete"), ensure you
reference the requirement IDs (NOTIFY-01..NOTIFY-04, PRIV-01..PRIV-03) and the
traceability table entries so both representations are identical, and then
search the document for any other inconsistent STATUS words and make them
consistent as well.- Around line 1-8: The header status "Status: SHIPPED" in the "# Requirements
Archive: v1.1 Chat Notifications" document contradicts the traceability table
showing Phase 2 and Phase 3 as "Pending" and the AI summary calling this
"post-implementation documentation"; reconcile them by either changing "Status:
SHIPPED" to "IN PROGRESS" or "PARTIALLY COMPLETE" and updating the AI summary to
reflect incomplete phases, or mark all Phase 2/Phase 3 entries in the
traceability table as "Complete" and update the AI summary to a true
post-implementation tone; ensure the header line "Archived: 2026-04-26" is
adjusted/removed if the milestone is not fully shipped and update any "Pending"
statuses in the traceability table to match the chosen overall status.In @.planning/milestones/v1.1-ROADMAP.md:
- Around line 17-20: The roadmap checklist still marks "Phase 3: Dual-keyed rate
limiting and verification harness" as incomplete even though the Phase 3
verification artifact shows it passed; update the roadmap checklist and any
progress table entries for Phase 3 (the "Dual-keyed rate limiting and
verification harness" item and its checkbox) to reflect completion, and adjust
the related lines referenced in the comment (the Phase 3 checklist entry and
corresponding progress table rows) so the milestone state is "done" or
"completed" and consistent with the verification artifact.In @.planning/research/PITFALLS.md:
- Around line 499-501: The document .planning/research/PITFALLS.md currently
contains hard-coded absolute paths (e.g.
/home/andrea/Documents/oss/mostrop2p/mostro-push-server/src/nostr/listener.rs,
/home/andrea/.../.planning/PROJECT.md, etc.); update those entries to use
repo-relative references like src/nostr/listener.rs, .planning/PROJECT.md,
src/api/routes.rs, src/store/mod.rs, src/push/fcm.rs, src/main.rs,
deploy-fly.sh, fly.toml (and any other absolute paths found) so the docs are
portable and do not leak local environment details.In @.planning/research/SUMMARY.md:
- Around line 27-29: The research summary currently lists actix-governor (and
its KeyExtractor usage and Fly-Client-IP guidance) as a recommended dependency,
but Phase 3 (D-05) rejects actix-governor due to GPL-3.0 incompatibility; update
the .planning/research/SUMMARY.md to mark lines that mention actix-governor (and
any other references) as superseded by Phase 3 and replace the recommendation
with the approved approach: using a hand-rolled from_fn middleware over
governor::DefaultKeyedRateLimiter; explicitly note the GPL-3.0 license
conflict and reference the Phase 3 decision so future implementers use the
from_fn + DefaultKeyedRateLimiter approach instead of actix-governor.In
@CLAUDE.md:
- Around line 38-66: Update the stale runtime/dependency statements: change the
actix-web version string from "4.4" to "4.9" in the CLAUDE.md dependency list,
remove or correct the sentence claiming governor is "not yet wired" and instead
state that governor is wired with dual-key rate limiting (referencing the rate
limit middleware and main wiring), and replace the claim "no integration test
suite present" with a note that integration tests exist (they use
#[actix_web::test] in the API test modules). Ensure the wording references the
updated actix-web version string, the governor/rate-limit middleware, and the
presence of integration tests using #[actix_web::test] so readers see the
accurate shipped v1.1 state.
Outside diff comments:
In
@.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-DISCUSSION-LOG.md:
- Around line 18-235: Translate all Spanish content in this document into clear,
idiomatic English while preserving table structure, headings (e.g., "Per-IP rate
sustained + burst", "Behavior when env vars NOTIFY_RATE_PER_*_PER_MIN are absent
or malformed at startup", "Wiring per-pubkey limiter + cleanup", "Claude's
Discretion"), selected choices ("User's choice:", "Notes:", D-01..D-28 locks)
and bullets exactly as-is; keep numeric values, option labels, and code/config
identifiers unchanged (e.g., NOTIFY_RATE_PER_PUBKEY_PER_MIN,
RATE_LIMIT_PER_MINUTE, start_rate_limit_cleanup_task(), PerPubkeyLimiter,
Arc<DefaultKeyedRateLimiter), update only the natural-language Spanish
strings to English, and ensure any inline Spanish words in table cells, notes,
or deferred lists are converted to English while preserving formatting and
markdown syntax.
Nitpick comments:
In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md:
- Around line 1127-1141: The fenced code blocks in the verification section and
the commit message block lack language specifiers; update the twoblocks that wrap the file list and the commit message to use a language hint (e.g.,text) so they becometext ...; locate the fenced blocks around the
Cargo.lock/Cargo.toml file list and the commit message "feat(api): add POST
/api/notify endpoint with privacy hardening" and add the language specifier to
each opening fence.In
@.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md:
- Around line 132-139: Update the fenced code block that contains the grep
commands so it includes a shell language specifier (e.g., change the openingtoshell) and keep the closing ```; locate the block with the lines matching
patterns like actix-governor|actix_governor, ".split(',').next()", and
".wrap(" (these unique strings identify the grep snippet) and ensure the
opening fence is annotated (shell or bash) for clarity.- Around line 124-126: Add a language specifier to the fenced code block that
contains the startup log sample so it renders with syntax highlighting; locate
the fenced block that wraps the line beginning "INFO mostro_push_backend > Rate
limiters initialized..." and change the opening fence fromtolog (i.e.,
add "log" as the language identifier).In
@.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-02-PLAN.md:
- Around line 82-83: Unificar la frontera per-IP usando índice 0-based: pick a
single criterion (recommended: use IP_BURST as the zero-based first-failing
iteration) and update the documentation and all tests that assert the first 429
index to use that same rule; specifically change the textual note that currently
says ">= IP_BURST + 1 (=31)" to ">= IP_BURST" (or convert all tests to 1-based
consistently), and adjust assertions that compare iteration indices to reference
the IP_BURST constant (symbol: IP_BURST) rather than IP_BURST + 1 so per-IP
tests assert iter >= IP_BURST and the per-pubkey tests remain unchanged (also
ensure rotation logic still uses format!("{:0>64x}", i) for trade_pubkey where
applicable).In @.planning/STATE.md:
- Around line 42-46: Add a language specifier (e.g., "text" or "plaintext") to
the fenced code block that contains the progress bar lines (the block showing
"Phase 1: PushDispatcher refactor", "Phase 2: /api/notify endpoint with privacy
hardening", and "Phase 3: Dual-keyed rate limiting and verification harness") so
the markdown follows best practices; update the opening fence fromtotext (or ```plaintext) for that block in STATE.md.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `8e67efff-4941-4f64-a72d-3fddc0d05086` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e35586997974ad762e861e3884fa82d27dc53144 and a38cdb4d18814dbb7edcf22ccad32e9837049432. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `Cargo.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (62)</summary> * `.gitignore` * `.planning/MILESTONES.md` * `.planning/PROJECT.md` * `.planning/RETROSPECTIVE.md` * `.planning/ROADMAP.md` * `.planning/STATE.md` * `.planning/config.json` * `.planning/milestones/v1.1-MILESTONE-AUDIT.md` * `.planning/milestones/v1.1-REQUIREMENTS.md` * `.planning/milestones/v1.1-ROADMAP.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-01-PLAN.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-01-SUMMARY.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-CONTEXT.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-DISCUSSION-LOG.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-PATTERNS.md` * `.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-VERIFICATION.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-01-PLAN.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-01-SUMMARY.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-SUMMARY.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-03-PLAN.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-03-SUMMARY.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-CONTEXT.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-DISCUSSION-LOG.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-HUMAN-UAT.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-PATTERNS.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-RESEARCH.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-REVIEW.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-VALIDATION.md` * `.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-VERIFICATION.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-PLAN.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-SUMMARY.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-02-PLAN.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-02-SUMMARY.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-CONTEXT.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-DISCUSSION-LOG.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-PATTERNS.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-REVIEW.md` * `.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-VERIFICATION.md` * `.planning/research/ARCHITECTURE.md` * `.planning/research/FEATURES.md` * `.planning/research/PITFALLS.md` * `.planning/research/SUMMARY.md` * `CLAUDE.md` * `Cargo.toml` * `deploy-fly.sh` * `docs/verification/dispute-chat.md` * `src/api/mod.rs` * `src/api/notify.rs` * `src/api/rate_limit.rs` * `src/api/routes.rs` * `src/api/test_support.rs` * `src/config.rs` * `src/main.rs` * `src/nostr/listener.rs` * `src/push/dispatcher.rs` * `src/push/fcm.rs` * `src/push/mod.rs` * `src/push/unifiedpush.rs` * `src/store/mod.rs` * `src/utils/log_pubkey.rs` * `src/utils/mod.rs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| | Threat ID | Category | Component | Disposition | Mitigation Plan | | ||
| |-----------|----------|-----------|-------------|-----------------| | ||
| | T-01-01 | Tampering | `PushDispatcher::services` post-construction | accept | The slice `Arc<[Arc<dyn PushService>]>` is immutable after `main.rs` wiring. There is no API on `PushDispatcher` to mutate it at runtime, eliminating an entire class of "service swap" attacks. No further mitigation needed. | | ||
| | T-01-02 | Information Disclosure | `PushDispatcher::dispatch` logs | mitigate | D-07 forbids any log lines inside `dispatch`; the dispatcher cannot leak `device_token`, `trade_pubkey`, or backend identifier even by accident. Verified via Task 1 acceptance grep `! grep -qE "info!|warn!|error!|debug!|trace!" src/push/dispatcher.rs`. | |
There was a problem hiding this comment.
Table format issue in threat register.
Similar to the Phase 2 plan, line 992 has a malformed table with 9 columns instead of 5. The content appears to exceed the defined column structure.
Based on static analysis hints (MD056).
🧰 Tools
🪛 LanguageTool
[style] ~992-~992: ‘by accident’ might be wordy. Consider a shorter alternative.
Context: ...ade_pubkey, or backend identifier even by accident. Verified via Task 1 acceptance grep !...
(EN_WORDINESS_PREMIUM_BY_ACCIDENT)
🪛 markdownlint-cli2 (0.22.1)
[warning] 992-992: Table column count
Expected: 5; Actual: 9; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-01-PLAN.md
at line 992, The table row for T-01-02 is malformed (9 columns) and violates the
document's 5-column table schema causing MD056 warnings; edit the line
containing "T-01-02 | Information Disclosure | `PushDispatcher::dispatch` logs |
mitigate | D-07 forbids..." to conform to the 5-column structure used elsewhere
(ensure exactly 5 pipe-separated cells: ID, Threat, Artifact, Mitigation,
Notes/Reference), collapse any stray pipes within the Notes/Reference cell
(escape or rephrase inline backticks like `PushDispatcher::dispatch`,
`device_token`, `trade_pubkey`) so they do not create extra columns, and run the
markdown linter to verify the MD056 warning is resolved.
Comment on lines
+16
to
+20
| ## Implementation Decisions | ||
|
|
||
| ### Dispatch Component Surface | ||
|
|
||
| - **D-01:** New module `src/push/dispatcher.rs` re-exported via `src/push/mod.rs`. Owns the services list and the iteration protocol. |
There was a problem hiding this comment.
Fix heading level progression to satisfy markdownlint.
### Dispatch Component Surface is being interpreted as a skipped heading level in this context. Please normalize heading levels (or adjust surrounding structure) so MD001 does not fail linting in CI.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/01-pushdispatcher-refactor-no-behaviour-change/01-CONTEXT.md
around lines 16 - 20, The heading "Dispatch Component Surface" currently uses
### and is causing a skipped-heading-level MD001 lint; change its Markdown level
so it follows the surrounding heading progression (e.g., adjust "### Dispatch
Component Surface" to the appropriate level such as "## Dispatch Component
Surface" or "####" to match the file's structure), and ensure the subsection
bullet "D-01: New module `src/push/dispatcher.rs` re-exported via
`src/push/mod.rs`" remains under that corrected heading.
| | T-02-02 | Information Disclosure (timing oracle) | dispatch latency vs registration status | mitigate | D-02: dispatch happens in `tokio::spawn` AFTER the response is constructed; handler p99 target under 50 ms. The spawn closure performs the `TokenStore::get` lookup, NOT the handler body — eliminates the variable-time path. Verified by acceptance criterion that the `tokio::spawn(async move { ... })` block contains the `token_store.get(&pubkey).await` call AND the response is returned in the next statement. | | ||
| | T-02-03 | Information Disclosure (FCM-state oracle) | response status on FCM error | mitigate | D-01 + D-02: FCM errors are caught inside the spawn closure and logged via `warn!` ONLY; no propagation to the response (which already returned 202). Verified by acceptance criterion that no `?`, `return`, or `Err(...)` inside the spawn closure short-circuits to the handler return. | | ||
| | T-02-04 | Information Disclosure (pubkey leak in logs) | log_pubkey() coverage in /api/notify paths | mitigate | D-14: `log_pubkey(&state.notify_log_salt, &req.trade_pubkey)` is the only pubkey identifier in `info!`/`warn!` lines from `notify.rs` and the spawn closure. D-15: `deploy-fly.sh` flips `RUST_LOG="info"` so `debug!` lines that log token prefixes elsewhere (`fcm.rs:283`, `unifiedpush.rs:176`) stop emitting. Verified by `! grep -nE 'trade_pubkey\[' src/api/notify.rs` and `grep RUST_LOG deploy-fly.sh` returning `RUST_LOG="info"`. | | ||
| | T-02-05 | Information Disclosure (source-IP correlation) | handler accessing peer_addr / connection_info | mitigate | D-12 + PRIV-03: handler signature is `(state: web::Data<AppState>, req: web::Json<NotifyRequest>) -> impl Responder`. NO `HttpRequest` parameter. Verified by `! grep -nE '(req\.peer_addr|connection_info|forwarded|HttpRequest)' src/api/notify.rs`. | |
There was a problem hiding this comment.
Table format issue in threat model.
Line 311 has a malformed table row with 8 columns instead of the expected 5 as defined in the header (lines 305-306). The row for threat T-02-01 appears to have excessive content that should likely be restructured.
📋 Proposed fix
Review the content in the T-02-01 row and either:
- Split into multiple rows if there are multiple mitigations
- Move excess content into the "Mitigation Plan" column
- Ensure the row has exactly 5 columns matching the header
Based on static analysis hints.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 311-311: Table column count
Expected: 5; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-PLAN.md
at line 311, The table row for T-02-05 is malformed (it contains too many
pipe-separated columns) because it includes extra explanatory text (handler
signature and grep check) that expands the row to 8 columns instead of the 5
defined by the header; fix it by editing that row so it has exactly 5 cells:
either split the extra info into one or more new rows, or move the handler
signature `(state: web::Data<AppState>, req: web::Json<NotifyRequest>) -> impl
Responder` and the grep evidence (`! grep -nE
'(req\.peer_addr|connection_info|forwarded|HttpRequest)' src/api/notify.rs`)
into the "Mitigation Plan" cell (or shorten them to a single concise note), and
ensure pipe separators match the header column count exactly so the markdown
table renders correctly.
|
|
||
| ## Hand-off to Plan 03 (VERIFY-03 dispute-chat runbook) | ||
|
|
||
| The endpoint exists and is operator-verifiable on Fly.io staging. Plan 03 produces a Spanish-language operator runbook at `docs/verification/dispute-chat.md` walking through: |
There was a problem hiding this comment.
Alinear el idioma del runbook con la convención del repositorio.
En Line 296 se declara explícitamente un runbook en español. Eso choca con la regla de documentación en inglés para el repositorio.
Based on learnings: Code, comments, commit messages, branch names, and documentation in English.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-02-SUMMARY.md
at line 296, The summary line declares "a Spanish-language operator runbook" for
docs/verification/dispute-chat.md which violates the repository convention to
keep documentation in English; update the text to state an English-language
operator runbook and ensure the referenced runbook content
(docs/verification/dispute-chat.md) is written or translated into English so
code comments, commit messages, and docs remain consistent with the repository
language policy.
Comment on lines
+79
to
+83
| @.planning/phases/02-post-api-notify-endpoint-with-privacy-hardening/02-CONTEXT.md | ||
| @.planning/phases/02-post-api-notify-endpoint-with-privacy-hardening/02-RESEARCH.md | ||
| @.planning/phases/02-post-api-notify-endpoint-with-privacy-hardening/02-PATTERNS.md | ||
| @.planning/phases/01-pushdispatcher-refactor-no-behaviour-change/01-01-SUMMARY.md | ||
| @src/nostr/listener.rs |
There was a problem hiding this comment.
Update stale planning paths to the milestone directory layout.
This plan references .planning/phases/... while the milestone artifacts here live under .planning/milestones/v1.1-phases/.... Please align these paths to avoid broken links and failed handoff automation.
Also applies to: 441-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-03-PLAN.md
around lines 79 - 83, Update the stale .planning/phases/... references in this
file to use the milestone directory layout .planning/milestones/v1.1-phases/...
so links and automation won’t break; search for any occurrences of
".planning/phases/02-post-api-notify-endpoint-with-privacy-hardening/02-" (and
the other similar phase paths referenced near the same block) and replace them
with
".planning/milestones/v1.1-phases/02-post-api-notify-endpoint-with-privacy-hardening/02-"
ensuring the filenames (e.g., 02-CONTEXT.md, 02-RESEARCH.md, 02-PATTERNS.md,
01-01-SUMMARY.md) remain unchanged; also update the additional occurrences noted
(the ones corresponding to the same pattern mentioned around lines 441-447) so
all links point to the milestone layout.
Comment on lines
+30
to
+37
| - [x] **NOTIFY-01 | ||
| **: Server exposes a new `POST /api/notify` endpoint that accepts a `{ "trade_pubkey": "<64-hex>" }` body, validates the pubkey shape, and dispatches a silent push to the device registered for that pubkey via `PushDispatcher`. The response contract (status codes / body shape) is finalized in `/gsd-plan-phase` against open decision OPEN-1 — see "Open Design Decisions" below. | ||
| - [x] **NOTIFY-02 | ||
| **: The endpoint matches the wire contract that Mostro Mobile will call from `mobile/docs/plans/CHAT_NOTIFICATIONS_PLAN.md` Phase 4. Any contract change negotiated under OPEN-1 is coordinated with the mobile team before implementation. | ||
| - [x] **NOTIFY-03 | ||
| **: The existing `POST /api/register`, `POST /api/unregister`, `GET /api/health`, `GET /api/info`, and `GET /api/status` endpoints' request and response shapes remain byte-identical to the pre-milestone version (no incidental refactor of `RegisterResponse` / `RegisterTokenRequest` / `UnregisterTokenRequest`). | ||
| - [x] **NOTIFY-04 | ||
| **: An `X-Request-Id` middleware generates a server-side UUIDv4 per request and exposes it on the response. The middleware ignores any inbound `X-Request-Id` from the client (privacy-safe correlation only). |
There was a problem hiding this comment.
Checkbox state conflicts with traceability.
All requirements (NOTIFY-01 through NOTIFY-04, PRIV-01 through PRIV-03) are marked complete with [x], but the traceability table at lines 145-152 shows these same Phase 2 requirements as "Pending."
Verify which status is correct and ensure consistency between the requirements checkboxes and the traceability table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/milestones/v1.1-REQUIREMENTS.md around lines 30 - 37, The
requirements checklist shows NOTIFY-01..NOTIFY-04 and PRIV-01..PRIV-03 marked
complete ([x]) while the traceability table lists these same Phase 2 items as
"Pending"; reconcile by deciding the true final status and updating both places
to match (either remove the [x] for those checklist items or change the
traceability entries from "Pending" to "Complete"), ensure you reference the
requirement IDs (NOTIFY-01..NOTIFY-04, PRIV-01..PRIV-03) and the traceability
table entries so both representations are identical, and then search the
document for any other inconsistent STATUS words and make them consistent as
well.
Comment on lines
+17
to
+20
| - [ ] **Phase 1: PushDispatcher refactor (no behaviour change)** — Extract the inline dispatch loop from the Nostr listener into a reusable `PushDispatcher` and drop the `Mutex<Vec<Box<dyn PushService>>>` in favour of `Arc<[Arc<dyn PushService>]>`. | ||
| - [ ] **Phase 2: `POST /api/notify` endpoint with privacy hardening** — Ship the sender-triggered notify handler, request-id middleware, hash-based pubkey logging, and the deploy-side `RUST_LOG=info` change required to keep the endpoint privacy-safe in production. | ||
| - [ ] **Phase 3: Dual-keyed rate limiting and verification harness** — Add per-IP `actix-governor` middleware (Fly-Client-IP keyed) scoped strictly to `/api/notify`, plus an in-handler per-`trade_pubkey` keyed limiter, with the integration-test suite that exercises the wiring end-to-end. | ||
|
|
There was a problem hiding this comment.
Synchronize roadmap progress with Phase 3 verification state.
This roadmap still marks Phase 3 as planned/incomplete, which conflicts with the Phase 3 verification artifact showing pass status. Please update checklist and progress table to reflect the current milestone state.
Also applies to: 121-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/milestones/v1.1-ROADMAP.md around lines 17 - 20, The roadmap
checklist still marks "Phase 3: Dual-keyed rate limiting and verification
harness" as incomplete even though the Phase 3 verification artifact shows it
passed; update the roadmap checklist and any progress table entries for Phase 3
(the "Dual-keyed rate limiting and verification harness" item and its checkbox)
to reflect completion, and adjust the related lines referenced in the comment
(the Phase 3 checklist entry and corresponding progress table rows) so the
milestone state is "done" or "completed" and consistent with the verification
artifact.
Comment on lines
+499
to
+501
| - Codebase (HIGH): `/home/andrea/Documents/oss/mostrop2p/mostro-push-server/src/nostr/listener.rs`, `src/api/routes.rs`, `src/store/mod.rs`, `src/push/fcm.rs`, `src/main.rs`, `deploy-fly.sh`, `fly.toml`. | ||
| - Project constraints (HIGH): `/home/andrea/Documents/oss/mostrop2p/mostro-push-server/.planning/PROJECT.md`. | ||
| - Pre-existing analysis (HIGH): `/home/andrea/Documents/oss/mostrop2p/mostro-push-server/.planning/codebase/CONCERNS.md`. |
There was a problem hiding this comment.
Replace absolute local paths with repo-relative references.
Using /home/andrea/... in committed docs makes sources non-portable and leaks local environment details. Prefer paths like .planning/PROJECT.md and src/....
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/research/PITFALLS.md around lines 499 - 501, The document
.planning/research/PITFALLS.md currently contains hard-coded absolute paths
(e.g.
/home/andrea/Documents/oss/mostrop2p/mostro-push-server/src/nostr/listener.rs,
/home/andrea/.../.planning/PROJECT.md, etc.); update those entries to use
repo-relative references like src/nostr/listener.rs, .planning/PROJECT.md,
src/api/routes.rs, src/store/mod.rs, src/push/fcm.rs, src/main.rs,
deploy-fly.sh, fly.toml (and any other absolute paths found) so the docs are
portable and do not leak local environment details.
Comment on lines
+27
to
+29
| **New dependency (requires explicit user approval per global CLAUDE.md):** | ||
| - `actix-governor` — Actix-web middleware adapter on top of the already-declared `governor` crate. Provides the `KeyExtractor` trait used to source the per-IP rate-limit key from `Fly-Client-IP`. Version pin (`0.5` vs `0.6`) is **NOT** decided here — there were `KeyExtractor` API differences across those minors and the exact signature of `extract`, `SimpleKeyExtractionError`, and `Governor::new` must be verified against current crate docs in `/gsd-plan-phase` before writing implementation code. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifica discrepancia entre research y estado implementado.
rg -n "actix-governor|actix_governor" \
.planning/research/SUMMARY.md \
.planning/milestones/v1.1-phases/03-dual-keyed-rate-limiting-and-verification-harness/03-01-PLAN.md \
Cargo.toml \
src/api/rate_limit.rs \
src/api/routes.rsRepository: MostroP2P/mostro-push-server
Length of output: 6966
Update the research document to reflect the Phase 3 decision against actix-governor.
Lines 27–29 recommend actix-governor as a new dependency, but Phase 3 PLAN explicitly rejects it (D-05): every published version is GPL-3.0, incompatible with the project's MIT license. The implementation uses a hand-rolled from_fn middleware over governor::DefaultKeyedRateLimiter<IpAddr> instead. Update the research summary or mark lines 27–29 (and other actix-governor references in the document) as superseded by Phase 3 Plan to avoid future implementers treating this as authoritative guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/research/SUMMARY.md around lines 27 - 29, The research summary
currently lists actix-governor (and its KeyExtractor usage and Fly-Client-IP
guidance) as a recommended dependency, but Phase 3 (D-05) rejects actix-governor
due to GPL-3.0 incompatibility; update the .planning/research/SUMMARY.md to mark
lines that mention actix-governor (and any other references) as superseded by
Phase 3 and replace the recommendation with the approved approach: using a
hand-rolled from_fn middleware over governor::DefaultKeyedRateLimiter<IpAddr>;
explicitly note the GPL-3.0 license conflict and reference the Phase 3 decision
so future implementers use the from_fn + DefaultKeyedRateLimiter approach
instead of actix-governor.
Comment on lines
+38
to
+66
| - `actix-web = "4.4"` - HTTP server and routing (`src/main.rs`, `src/api/routes.rs`) | ||
| - `actix-rt = "2.9"` - Actix async runtime | ||
| - `tokio = "1.35"` - Async runtime, channels, sync primitives, background tasks | ||
| - `tokio-tungstenite = "0.21"` - WebSocket client library (declared in `Cargo.toml:12`; Nostr relay traffic actually flows via `nostr-sdk`) | ||
| - `nostr-sdk = "0.27"` - Nostr protocol client used in `src/nostr/listener.rs` | ||
| - `mockito = "1.2"` - HTTP mocking for unit tests, declared in `[dev-dependencies]` (`Cargo.toml:55-56`) | ||
| - Built-in `cargo test` runner (no integration test suite present in repo) | ||
| - `cargo build --release` - Production build (used in `Dockerfile:7` and README workflow) | ||
| - `cargo run` - Development runner | ||
| - `cargo clippy` - Linting (referenced in `README.md`) | ||
| - `cargo fmt` - Formatting (referenced in `README.md`) | ||
| ## Key Dependencies | ||
| - `nostr-sdk = "0.27"` - Subscribes to Nostr relays, parses Gift Wrap (kind 1059) events (`src/nostr/listener.rs:2`) | ||
| - `actix-web = "4.4"` - REST API surface for token registration (`src/api/routes.rs:1`) | ||
| - `reqwest = "0.11"` (with `json` feature) - HTTPS client for FCM v1 API and UnifiedPush endpoint POSTs (`src/push/fcm.rs:3`, `src/push/unifiedpush.rs:4`) | ||
| - `jsonwebtoken = "9"` - Signs RS256 JWTs for Firebase OAuth2 token exchange (`src/push/fcm.rs:8`) | ||
| - `serde = "1.0"` (`derive`) and `serde_json = "1.0"` - Configuration, request bodies, FCM payloads, persisted endpoint store | ||
| - `tokio` (`full`) - Concurrency primitives (`Mutex`, `RwLock`), background tasks, time intervals | ||
| - `chrono = "0.4"` (`serde`) - Timestamps for token TTL and registration metadata (`src/store/mod.rs:1`) | ||
| - `secp256k1 = "0.28"` (`rand-std` feature) - Public/secret key handling (`src/crypto/mod.rs:8`) | ||
| - `chacha20poly1305 = "0.10"` - AEAD cipher for token decryption | ||
| - `hkdf = "0.12"` and `sha2 = "0.10"` - Key derivation | ||
| - `rand = "0.8"`, `hex = "0.4"`, `base64 = "0.21"` - Encoding helpers | ||
| - The `crypto` module is gated with `#[allow(dead_code)]` in `src/main.rs:13-15` | ||
| - `config = "0.14"` - Configuration loader (declared in `Cargo.toml:30`; runtime config currently uses `std::env` directly in `src/config.rs`) | ||
| - `dotenv = "0.15"` - Loads `.env` at startup (`src/main.rs:26`) | ||
| - `env_logger = "0.11"` and `log = "0.4"` - Logging facade and stdout backend | ||
| - `governor = "0.6"` - Rate-limiting primitive (declared, not yet wired into request handlers) | ||
| - `futures = "0.3"` and `async-trait = "0.1"` - Async trait support (`src/push/mod.rs:1`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify documentation claims against current repo state (read-only)
set -euo pipefail
echo "== actix-web and governor versions =="
rg -n 'actix-web|governor' Cargo.toml Cargo.lock
echo
echo "== governor wiring in src =="
rg -n --type=rust 'RateLimiter::keyed|DefaultKeyedRateLimiter|per_ip_rate_limit_mw|per_pubkey_limiter|retain_recent'
echo
echo "== integration test presence in src/api =="
rg -n --type=rust '#\[actix_web::test\]|#\[cfg\(test\)\]|test::init_service' src/api src || trueRepository: MostroP2P/mostro-push-server
Length of output: 9253
Update stale stack/runtime statements in CLAUDE.md to reflect shipped v1.1 state.
Lines 38-66 contain outdated claims:
actix-web = "4.4"→ should be4.9- "governor... not yet wired" → FALSE; governor is fully wired (dual-key rate limiting in
src/main.rs:113–126, middleware insrc/api/rate_limit.rs:87, routes wrapper atsrc/api/routes.rs:65) - "no integration test suite" → FALSE; integration tests present at
src/api/routes.rs:203+,src/api/rate_limit.rs:224+,src/api/notify.rs:162+with#[actix_web::test]infrastructure
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 38 - 66, Update the stale runtime/dependency
statements: change the actix-web version string from "4.4" to "4.9" in the
CLAUDE.md dependency list, remove or correct the sentence claiming governor is
"not yet wired" and instead state that governor is wired with dual-key rate
limiting (referencing the rate limit middleware and main wiring), and replace
the claim "no integration test suite present" with a note that integration tests
exist (they use #[actix_web::test] in the API test modules). Ensure the wording
references the updated actix-web version string, the governor/rate-limit
middleware, and the presence of integration tests using #[actix_web::test] so
readers see the accurate shipped v1.1 state.
|
I found one blocking issue.
That means the local variable is dead code, and more importantly the effective default used by the app is not the one described right above it. Why this matters:
Please fix this by reading the env var once and reusing that value for |
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
POST /api/notify { trade_pubkey }(always-202) so Mostro Mobile clients can wake their peer for P2P chat without the server ever learning sharedKeys or sender identity. Existing/api/register|unregister|health|info|statusremain byte-identical.PushDispatcheroverArc<[Arc<dyn PushService>]>(no moreMutexon the dispatch path); Nostr listener flow unchanged. Adds salted-BLAKE3log_pubkey()correlator, server-side UUIDv4X-Request-Idmiddleware, boundedtokio::spawnviaArc<Semaphore>(50), separate FCM silent payload, and flipsRUST_LOG=infoindeploy-fly.sh./api/notifyonly: per-IPfrom_fnmiddleware (Fly-Client-IP > rightmost-XFF > peer_addr) + per-pubkey in-handler check ongovernor 0.6. Both 429 paths return a byte-identical body withRetry-Afterandx-request-id(anti-RL-2 oracle). Quotas configurable viaNOTIFY_RATE_PER_PUBKEY_PER_MIN/NOTIFY_RATE_PER_IP_PER_MIN.New deps:
blake3,uuid(v4).actix-governorwas rejected (GPL-3.0 incompatible with project MIT) — baregovernorwas already declared.Test plan
cargo build --releasesucceedscargo test— 31 in-process integration tests greencargo clippy -- -D warningscleancargo fmt --checkcleantrade_pubkey, publish akind 1059event from asecondary Nostr client (NOT the Mostro daemon) targeting that
pubkey — silent push reaches the Android device
curl -X POST .../api/notify -d '{"trade_pubkey":"..."}'returns202and device wakes up/api/healthreturns 1000/1000 (anti-DEPLOY-3)docs/verification/dispute-chat.mdfor the admin-DMpath (anti-CRIT-1 grep one-liner included)
RUST_LOG=infoand confirm no log line contains a hexpubkey prefix or a registered FCM token (PRIV-03)
Summary by CodeRabbit
New Features
/api/notifyendpoint for sender-triggered push delivery.Improvements