Dev#35
Conversation
…improvements Refactor/headless performance improvements
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 Walkthrough✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
update available banner
Chore/documentation updates 0
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/SECURITY.md (1)
671-675:⚠️ Potential issue | 🟡 MinorOrphaned content at end of file.
Line 672 starts with
4. **Directional Peer Policies**right after the closing "Remember: Security is a process…" paragraph, without any parent section heading. This numbered item appears to be a leftover fragment — it should either be relocated into the relevant section (e.g., under Headless Mode Setup or Network Security) or removed if it's redundant with the peer policies mention on line 22.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 3-11: The 1.1.1 changelog entry under header "[1.1.1] —
2026-02-04" is incomplete; update CHANGELOG.md by expanding the 1.1.1 section
into appropriate subsections (Added/Changed/Fixed) and list all PR changes:
relay probe optimizations, Docker Compose improvements, new docs (CONFIG.md,
AUTH_MATRIX.md, PEER_POLICIES.md), headless-mode session optimizations,
WebSocket events endpoint, and the update-check banner backed by /api/update;
keep the existing version/notes lines and ensure each bullet is concise and
placed under the correct subsection (e.g., Added for new docs and endpoints,
Changed for optimizations/improvements).
In `@compose.yml`:
- Line 22: Remove the hardcoded "platform: linux/x86_64" entry in the
docker-compose service (or make it configurable) because it forces x86 on ARM
hosts; update the compose.yml to either delete the platform: linux/x86_64 line
or replace it with a variable (e.g., ${PLATFORM}) and document/define that env
var so ARM deployments (Raspberry Pi / Apple Silicon / Umbrel) can run without
emulation.
- Around line 41-48: The compose.yml currently bind-mounts the host src
directory via the volumes entry "./src:/app/src:rw", which bypasses the image
build and is unsafe for production; either remove that volumes entry entirely or
move it behind a development-only configuration (e.g., add a "profiles: [dev]"
wrapper or put the mount into a docker-compose.override.yml) and update the file
comment to explicitly state that the bind-mount is for local development only;
ensure the persistent mounts (./.env and ./data) remain unchanged and that any
removal preserves reproducible image behavior for the services referencing the
mounted src path.
In `@docs/AUTH_MATRIX.md`:
- Around line 14-15: The table header row containing the "Purpose" column uses
right-alignment markup (`---:`) causing the "Purpose" column to be
right-aligned; update that header cell's alignment marker to left alignment
(change the `---:` for the "Purpose" column to `---`) so the "Purpose" column
aligns with other text columns in the AUTH_MATRIX.md table header row.
In `@env.example`:
- Around line 111-119: Remove the duplicate WebSocket-specific env var blocks
from the RATE LIMITING section and keep the canonical definitions only in the
WEBSOCKET ABUSE PROTECTION (ADVANCED) section; specifically ensure
RATE_LIMIT_WS_UPGRADE_WINDOW, RATE_LIMIT_WS_UPGRADE_MAX,
WS_MAX_CONNECTIONS_PER_IP, WS_MSG_RATE, and WS_MSG_BURST are defined once in the
ADVANCED block, and replace the removed block in the RATE LIMITING section with
a single brief cross-reference line pointing users to the ADVANCED WebSocket
abuse protection section for these settings.
In `@llm/context/NIP46_IMPLEMENTATION.md`:
- Around line 261-263: Update the fenced code block containing the nostrconnect
example so it includes a language tag (e.g., ```text) to satisfy markdownlint;
specifically change the fence from ``` to ```text for the block that shows
"nostrconnect://pubkey?relay=wss://...&perms=sign_event:1,sign_event:4,nip44_encrypt".
- Around line 170-173: The table rows for the requests endpoints contain
unescaped pipe characters inside inline code (notably the `POST /requests`
action list like `action=approve|deny|fail|complete`) which breaks the Markdown
table; find the inline code spans in NIP46_IMPLEMENTATION.md that include '|'
(e.g., the `action=...` value and any other param values containing '|') and
escape each pipe as '\|' inside the backticks (or replace the inline code with a
code span that doesn't include raw '|'), ensuring the table columns render
correctly for the `GET /requests`, `POST /requests`, and `DELETE /requests`
rows.
🧹 Nitpick comments (10)
docs/RELEASE.md (1)
37-40: Hard-coded version in release branch examples will become stale.The branch name
release/prepare-v1.1.1and the PR description are specific to this release. Consider using a placeholder likerelease/prepare-vX.Y.Zso the guide remains useful for future releases without needing edits each time.Proposed change
-git checkout -b release/prepare-v1.1.1 -git push origin release/prepare-v1.1.1 +git checkout -b release/prepare-vX.Y.Z +git push origin release/prepare-vX.Y.Z-Create PR: `release/prepare-v1.1.1` → `master` +Create PR: `release/prepare-vX.Y.Z` → `master`package.json (1)
36-37:typecheckandtscscripts are identical.Both run
tsc --noEmit. If this is intentional for discoverability (e.g.,bun run tscas a shorthand), that's fine — but you could also alias one to the other to keep a single source of truth:- "typecheck": "tsc --noEmit", - "tsc": "tsc --noEmit" + "typecheck": "tsc --noEmit", + "tsc": "bun run typecheck"src/routes/auth.ts (1)
788-799: Timer skip is consistent with the session-disable path, but consider guardingstopAuthCleanupdefensively.The condition
!HEADLESS || !process.env.API_KEYcorrectly mirrors thevalidateSessionSecretfast-path.stopAuthCleanupalready null-checks the timer handles, so shutdown is safe when timers were never started.One minor note: the condition
process.env.API_KEYhere reads the env var directly at module-load time, whileAUTH_CONFIG.API_KEY(line 240) applies additional gating (HEADLESS ? process.env.API_KEY : undefined). Since both checks are already guarded byHEADLESS, they're equivalent here—but usingAUTH_CONFIG.API_KEYwould be more consistent and avoid a subtle divergence if the gating logic ever changes.♻️ Use AUTH_CONFIG.API_KEY for consistency
-// Skip timers in headless mode with API key - sessions are disabled anyway (perf optimization 2.3) -if (!HEADLESS || !process.env.API_KEY) { +// Skip timers in headless mode with API key - sessions are disabled anyway (perf optimization 2.3) +if (!HEADLESS || !AUTH_CONFIG.API_KEY) {Similarly in
validateSessionSecret:- if (HEADLESS && process.env.API_KEY) { + if (HEADLESS && AUTH_CONFIG.API_KEY) {Note:
AUTH_CONFIGis defined aftervalidateSessionSecretis called (line 245), so this refactor would require reordering—moving the API_KEY resolution beforevalidateSessionSecret(). If that's too disruptive, the current approach is fine.src/routes/update.ts (3)
40-42: Numeric env vars are not clamped or NaN-guarded, unlike other config values in the codebase.Other timeout config in the codebase (e.g.,
FROSTR_SIGN_TIMEOUT,CONNECTIVITY_PING_TIMEOUT_MSper docs/CONFIG.md) clamp values to safe ranges. Here, a non-numericUPDATE_CHECK_TIMEOUT_MSwould produceNaN, andsetTimeout(..., NaN)fires immediately — causing every GitHub fetch to abort instantly.Consider adding simple guards:
Proposed fix — clamp to safe ranges
-const UPDATE_CHECK_TIMEOUT_MS = parseInt(process.env['UPDATE_CHECK_TIMEOUT_MS'] ?? '5000', 10); -const UPDATE_CHECK_TTL_MS = parseInt(process.env['UPDATE_CHECK_TTL_MS'] ?? '21600000', 10); // 6 hours -const UPDATE_CHECK_FAILURE_TTL_MS = parseInt(process.env['UPDATE_CHECK_FAILURE_TTL_MS'] ?? '900000', 10); // 15 minutes +const UPDATE_CHECK_TIMEOUT_MS = Math.min(Math.max(parseInt(process.env['UPDATE_CHECK_TIMEOUT_MS'] ?? '5000', 10) || 5000, 1000), 30000); +const UPDATE_CHECK_TTL_MS = Math.min(Math.max(parseInt(process.env['UPDATE_CHECK_TTL_MS'] ?? '21600000', 10) || 21600000, 60000), 86400000); +const UPDATE_CHECK_FAILURE_TTL_MS = Math.min(Math.max(parseInt(process.env['UPDATE_CHECK_FAILURE_TTL_MS'] ?? '900000', 10) || 900000, 60000), 3600000);
54-58:parseBooleanduplicates pattern fromsrc/const.ts.The boolean parsing logic here mirrors the IIFE pattern used for
HEADLESS,SKIP_ADMIN_SECRET_VALIDATION, etc. insrc/const.ts. Consider extracting a sharedparseEnvBooleanutility to reduce duplication, especially since theconst.tsversion also handles'yes'.
237-247: No deduplication of concurrent fetches while cache is stale.If multiple requests arrive simultaneously after cache expiry, each will trigger an independent GitHub API fetch. For a public, unauthenticated endpoint with a 6-hour default TTL this is low-risk, but a simple in-flight promise could prevent redundant calls.
src/server.ts (2)
749-754: Duplicate WS rate-limit config parsing.The env-parsing block for
RATE_LIMIT_WS_UPGRADE_WINDOW/RATE_LIMIT_WS_UPGRADE_MAXis copy-pasted between the/api/eventsand/WebSocket upgrade paths (lines 749-754 vs 856-861). Consider extracting a small helper to avoid divergence.♻️ Extract shared WS upgrade rate-limit config
+// Near the top with other WS config constants +const WS_UPGRADE_WINDOW_MS = (() => { + const raw = process.env.RATE_LIMIT_WS_UPGRADE_WINDOW ?? process.env.RATE_LIMIT_WINDOW ?? '900'; + const parsed = Number.parseInt(raw, 10); + return Math.max(1000, (Number.isFinite(parsed) ? parsed : 900) * 1000); +})(); +const WS_UPGRADE_MAX = (() => { + const raw = process.env.RATE_LIMIT_WS_UPGRADE_MAX ?? '30'; + const parsed = Number.parseInt(raw, 10); + return Math.max(1, Number.isFinite(parsed) ? parsed : 30); +})();Then replace the inline parsing in both upgrade paths with
wsUpWindow: WS_UPGRADE_WINDOW_MSandwsUpMax: WS_UPGRADE_MAX.Also applies to: 856-861
542-555: Startup echoes are restricted to headless mode only — the same functions ARE actively used in database mode.Verified: startup echo broadcasts (lines 549, 552) are gated to
CONST.HEADLESS && !CONST.SKIP_STARTUP_ECHO, yetsendSelfEcho()andbroadcastShareEcho()are also called unconditionally insrc/routes/user.tsandsrc/routes/env.tsafter credential updates in database mode. This pattern suggests intentional design (startup verification in headless mode; update-time verification in both modes). If this distinction is deliberate, an inline comment clarifying the different verification strategies would improve readability.src/const.ts (1)
92-125: Consider extracting a shared boolean env parser.The truthy-value parsing pattern (
true/1/yes) is now repeated 5+ times across this file. A small helper would reduce duplication and ensure consistent behavior:♻️ Proposed helper
+/** Parse a boolean environment variable (true/1/yes → true, everything else → false). */ +function parseBoolEnv(key: string): boolean { + const value = process.env[key]; + if (!value) return false; + const trimmed = value.trim(); + if (trimmed.length === 0) return false; + const normalized = trimmed.toLowerCase(); + return normalized === 'true' || normalized === '1' || normalized === 'yes'; +} + -export const SKIP_RELAY_PROBE = (() => { - const value = process.env['SKIP_RELAY_PROBE']; - if (!value) return false; - const trimmed = value.trim(); - if (trimmed.length === 0) return false; - const normalized = trimmed.toLowerCase(); - return normalized === 'true' || normalized === '1' || normalized === 'yes'; -})(); +export const SKIP_RELAY_PROBE = parseBoolEnv('SKIP_RELAY_PROBE'); +export const DEFER_RELAY_PROBE = parseBoolEnv('DEFER_RELAY_PROBE'); +export const SKIP_STARTUP_ECHO = parseBoolEnv('SKIP_STARTUP_ECHO');The existing
HEADLESSandSKIP_ADMIN_SECRET_VALIDATIONcould also be migrated in a follow-up.src/node/manager.ts (1)
730-734: Return defensive copies for background probe results.
Avoid exposing mutable arrays/objects from the shared cache.♻️ Suggested change
- lastBackgroundProbeResult = { - originalRelays: relays, - filteredRelays: filtered, - timestamp: Date.now() - }; + lastBackgroundProbeResult = { + originalRelays: [...relays], + filteredRelays: [...filtered], + timestamp: Date.now() + };-export function getLastBackgroundProbeResult(): BackgroundProbeResult | null { - return lastBackgroundProbeResult; -} +export function getLastBackgroundProbeResult(): BackgroundProbeResult | null { + if (!lastBackgroundProbeResult) return null; + return { + ...lastBackgroundProbeResult, + originalRelays: [...lastBackgroundProbeResult.originalRelays], + filteredRelays: [...lastBackgroundProbeResult.filteredRelays] + }; +}Also applies to: 766-768
persist UI event logs to sqlite with pagination, lazy-load blobs, and export
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/components/Signer.tsx`:
- Around line 560-589: handleLoadOlder can repeatedly request the same beforeSeq
when no entry IDs parse to numbers (seqs is empty) because minSeq falls back to
oldestSeq; add a defensive guard: after computing seqs, if seqs.length === 0
then call setHasMoreHistory(false) (and skip updating oldestSeq) to break the
loop and avoid re-requesting the same beforeSeq; reference the
variables/functions handleLoadOlder, seqs, minSeq, oldestSeq, setHasMoreHistory,
and setOldestSeq when applying the change.
In `@src/db/ui-event-log.ts`:
- Around line 267-317: Define a concrete row type and a typed params array
instead of using any: add an interface (e.g. UiEventRow) describing the SELECT
result fields used in list (seq, created_at_ms, created_at, data_preview,
data_hash, data_bytes, type, message) with appropriate unions
(number|string|null where values may be strings or numbers), change params from
any[] to Array<string|number|null> (or more specific unions) and annotate rows
as UiEventRow[] (and the stmt.all return) so the mapping logic uses proper
types; update the variables referenced in list (params, rows) and keep the same
call to stmt.all(...params, limit).
- Around line 149-174: The repo contains an orphaned SQL migration that
duplicates the schema created by ensureUiEventLogSchema; either delete the
migration file (the one creating ui_event_log_blobs and ui_event_log_entries) to
avoid confusion, or move its DDL into your central migration mechanism so
runMigrations() will apply it; update
createUiEventLogStore/ensureUiEventLogSchema to no longer conflict with the
migration path (i.e., if you integrate, remove the inline exec() calls in
ensureUiEventLogSchema or mark them as no-ops when migrations are used) and
ensure schema ownership is singular and tracked by runMigrations()/the migration
system.
In `@src/routes/update.ts`:
- Around line 40-42: The parsed env constants UPDATE_CHECK_TIMEOUT_MS,
UPDATE_CHECK_TTL_MS and UPDATE_CHECK_FAILURE_TTL_MS can become NaN for garbage
env values; update their initialization to validate the parsed result and fall
back to the intended numeric defaults when Number.isNaN or !Number.isFinite is
true (e.g., parseInt(..., 10) -> const v = parseInt(..., 10); const
UPDATE_CHECK_TTL_MS = Number.isFinite(v) ? v : 21600000), ensuring the rest of
the cache logic (e.g., isCacheValid) receives a valid number.
- Around line 79-107: The parseVersion function currently uses
withoutPrefix.split('-', 2) which drops any additional hyphenated segments in
prerelease identifiers (e.g., "alpha-1"); change this to split only on the first
hyphen so the prerelease captures the entire trailing string. Replace the
destructuring with logic like: find the first '-' via
withoutPrefix.indexOf('-'), set core = withoutPrefix when no '-' or
withoutPrefix.slice(0, idx) when present, and set prerelease = idx === -1 ?
undefined : withoutPrefix.slice(idx + 1); keep the rest of parseVersion
(validation of core parts, numeric conversion, allowPrerelease check, and
returned shape) unchanged and use prerelease ?? null when setting the returned
object.
🧹 Nitpick comments (14)
src/db/nip46.ts (1)
339-370: Cursor-based pagination implementation looks correct.The composite
(created_at, id)cursor with the(created_at < ? OR (created_at = ? AND id < ?))pattern is the right approach for deterministic keyset pagination over aDESCordering. Parameterized queries properly prevent injection.One minor observation: the
before.createdAtvalue is only checked for being a non-empty string (line 359) but is not validated as a well-formed datetime. A malformed value won't cause injection (it's parameterized), but it will silently return unexpected results. The route layer (reviewed next) also doesn't validate the format — consider adding a lightweight ISO-8601 check in either layer.src/routes/nip46.ts (1)
296-314: Cursor pagination wiring looks good overall.The both-or-neither validation (lines 300–305) and cursor construction are clean. The
nextCursorheuristic (full page → more data) is standard keyset pagination behavior.Note:
beforeCreatedAtis passed through to the DB layer without any format validation. While parameterized queries prevent injection, an arbitrary string like"hello"would silently return zero results rather than a 400 error. A lightweight datetime sanity check (e.g., regex orDate.parse) would give callers better feedback.frontend/components/ui/log-entry.tsx (1)
100-130: Lazy-loading effect is well-structured with proper cleanup.The cancellation flag pattern, hash format validation, and guard conditions (
!isMessageExpanded,hasFetchedFull,fetchError) are all correct. One small note:Line 115 trusts
(payload as any).datawithout verifying the shape. If the endpoint returns{ data: null },resolvedDatawill be set tonullandhasFetchedFulltotrue, which means the preview is also lost. Consider gating onpayload.data != nullbefore overwriting.Proposed guard
.then(payload => { if (cancelled) return; if (payload && typeof payload === 'object' && 'data' in payload) { - setResolvedData((payload as any).data); - setHasFetchedFull(true); + const fetched = (payload as any).data; + if (fetched != null) { + setResolvedData(fetched); + setHasFetchedFull(true); + } } })src/routes/update.ts (2)
54-58:parseBooleanduplicates logic already insrc/const.ts.The same truthy-string parsing pattern exists in
src/const.tsforHEADLESSandSKIP_ADMIN_SECRET_VALIDATION. Consider extracting a shared utility to avoid drift between the two implementations.
238-248: Concurrent requests may trigger parallel GitHub fetches.Multiple simultaneous requests hitting an expired cache will each call
fetchLatestVersion()before either can write back tocachedUpdate. This is a minor inefficiency rather than a correctness bug (last write wins, all return valid data). For a 6-hour TTL endpoint this is unlikely to matter in practice, but if you want to tighten it, you could store the in-flight promise and deduplicate.src/node/manager.ts (2)
650-659: Background probe state: consider exportingBackgroundProbeResult.
getLastBackgroundProbeResult()is exported and returnsBackgroundProbeResult | null, but the interface itself is not exported. Consumers would have to useReturnType<typeof getLastBackgroundProbeResult>to reference this type. Exporting the interface would make it easier to type downstream code.♻️ Export the interface
-interface BackgroundProbeResult { +export interface BackgroundProbeResult { originalRelays: string[]; filteredRelays: string[]; timestamp: number; }
1338-1357: Persistence hook integration looks solid; minor semicolon inconsistency on Line 1350.The
persistcallback is correctly guarded by try/catch and falls back to ephemeral IDs when persistence is unavailable. The ISO timestamp change is a good improvement for consistent cross-timezone display.Line 1350 is missing a semicolon after the assignment (
const seq = opts?.persist?.(logEntry)), while most surrounding statements use them. This won't break anything since ASI handles it, but it's inconsistent with the local style.Style consistency
- const seq = opts?.persist?.(logEntry) + const seq = opts?.persist?.(logEntry);frontend/components/Signer.tsx (1)
521-558: Potential unnecessary re-fetches whenauthHeadersis an unstable reference.The dependency array
[authHeaders, isHeadlessMode]will cause this effect to re-run wheneverauthHeadersis a new object reference (even with identical content). Since the default isauthHeaders = {}, a new object is created on every render if the parent doesn't memoize it.The de-duplication merge logic prevents visible bugs, but the redundant network requests are wasteful. Consider using
authHeadersRef(already established at Line 188) here as well, or memoizingauthHeadersin the parent.src/routes/event-log.ts (1)
5-24:parseSeqandparseBeforeSeqare identical — consider consolidating.Both functions parse a nullable string into a positive integer or
undefinedwith identical logic. This is a minor DRY opportunity.♻️ Consolidate into a single helper
-function parseSeq(value: string | null): number | undefined { - if (!value) return undefined - const n = Number.parseInt(value, 10) - if (!Number.isFinite(n) || n <= 0) return undefined - return n -} - function parseLimit(value: string | null, fallback = 200): number { if (!value) return fallback const n = Number.parseInt(value, 10) if (!Number.isFinite(n)) return fallback return Math.min(Math.max(n, 1), 500) } -function parseBeforeSeq(value: string | null): number | undefined { +function parsePositiveInt(value: string | null): number | undefined { if (!value) return undefined const n = Number.parseInt(value, 10) if (!Number.isFinite(n) || n <= 0) return undefined return n }src/db/ui-event-log.test.ts (1)
6-38: Thorough test covering append, dedup, pagination, and blob retrieval.Good coverage of the core store operations. The test correctly validates blob content-addressing (same data → same hash → single blob row) and cursor-based pagination semantics.
Minor nit:
ensureUiEventLogSchema(mem)on Line 8 is redundant sincecreateUiEventLogStore(mem)on Line 9 calls it internally (seeui-event-log.tsLine 176). Removing it would simplify setup without changing behavior.src/db/ui-event-log.ts (4)
63-78: Broad substring match on'token'may over-redact benign fields.
k.includes('token')on line 76 will redact fields liketoken_type,tokenCount,tokenIndex, etc. The comment says "redact when we're confident," but a substring match is fairly aggressive. Consider using a word-boundary approach or an allowlist for known-safetoken*keys if over-redaction becomes a problem in practice.
176-206: Missing explicit return type oncreateUiEventLogStore.The function relies entirely on type inference for its return type. Per coding guidelines, explicit types should be declared. Consider defining an interface for the store's public API.
Suggested approach
interface UiEventLogStore { append(entry: UiEventLogStreamEntry): { seq: number; dataHash: string | null }; list(opts?: { limit?: number; beforeSeq?: number; types?: string[] }): UiEventLogListResult; getBlob(hash: string): { data: unknown; byteLength: number } | null; exportChunk(opts?: { afterSeq?: number; untilSeq?: number; limit?: number; types?: string[] }): { rows: UiEventLogExportRow[]; nextAfterSeq: number | null }; }Then:
export function createUiEventLogStore(dbConn: Database): UiEventLogStore { ... }As per coding guidelines, "TypeScript strict mode; explicit types, avoid
any".
209-265: Consider wrapping blob + entry inserts in a transaction.
insertBlob.run(line 235/244) andinsertEntry.run(line 253) are not wrapped in a transaction. If the entry insert fails after the blob was stored, you get an orphaned blob. While harmless due to content-addressing, wrapping in a transaction would be cleaner and prevent accumulation of unused blobs over time.
63-78: Consider adding Nostr-specific sensitive keys to the redaction list.Given this is a FROSTR/Nostr project, keys like
nsec,privkey,private_key, orbunker_secretcould carry secret material but won't be caught by the current checks (none contain"secret"or"token"as substrings). Consider adding them explicitly.Suggested addition
if (k === 'derived_key' || k === 'derivedkey') return true + if (k === 'nsec' || k === 'privkey' || k === 'private_key' || k === 'sk') return true if (k.includes('secret') || k.includes('token')) return true
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/AUTH_MATRIX.md`:
- Line 17: The AUTH_MATRIX entry for `/api/update` is contradictory: the table
marks the endpoint as available ("Yes") in headless/managed modes but the note
says update checks are disabled when HEADLESS=true or
SKIP_ADMIN_SECRET_VALIDATION=true/UPDATE_CHECK_DISABLED=true; update the docs so
behavior is explicit—either change the table cell for `/api/update` to indicate
"No (disabled)" for managed/headless deployments or reword the note to state
that the endpoint exists but returns a "disabled" response/status when HEADLESS,
SKIP_ADMIN_SECRET_VALIDATION, or UPDATE_CHECK_DISABLED are set (mention
`/api/update` and the three env vars by name).
In `@frontend/components/Signer.tsx`:
- Around line 1007-1012: handleClearLogs currently resets state including
oldestSeq to null which permanently disables the handleLoadOlder guard
(!oldestSeq) and prevents reloading history; to fix, either stop clearing
oldestSeq in handleClearLogs or after clearing call the initial-history loader
to repopulate oldestSeq (invoke the same code path used by
handleLoadOlder/initial load, e.g., trigger the function that loads the first
page or call handleLoadOlder with a reset flag) and keep using
setLogs/setHasMoreHistory as needed so the "load older" button becomes
functional again; update handleClearLogs and/or add a small wrapper (e.g.,
reloadHistory or resetAndLoadHistory) and reference handleClearLogs,
handleLoadOlder, oldestSeq, setOldestSeq, and setHasMoreHistory.
In `@src/routes/update.ts`:
- Around line 111-121: The compareVersions function uses localeCompare for
prerelease which is not semver-compliant; update compareVersions (and the
handling of ParsedVersion.prerelease) to split prerelease strings by '.' and
compare each identifier per SemVer rules: treat purely-numeric identifiers as
integers and compare numerically, treat non-numeric identifiers by ASCII lexical
order, numeric identifiers have lower precedence than non-numeric, and if all
compared identifiers are equal the shorter identifier list has lower precedence
(i.e., fewer fields < more fields); replace the localeCompare branch with this
dot-separated, numeric-aware comparison.
🧹 Nitpick comments (10)
compose.yml (1)
40-47: The.envbind-mount directory-creation pitfall is documented but not guarded.If a user runs
docker compose upwithout first creating.env, Docker will silently create./.env/as a directory, breaking both theenv_filedirective and in-app reads. The comments warn about this, but users may not read them.Consider adding a lightweight guard to fail fast, for example via an entrypoint wrapper or a Compose-level check:
Option: Makefile / wrapper script guard
up: `@test` -f .env || (echo "ERROR: .env file not found. Run 'cp env.example .env' first." && exit 1) docker compose up -dAlternatively, if you have a custom entrypoint in the Dockerfile, you could validate
/app/.envis a regular file at startup.docs/AUTH_MATRIX.md (1)
38-38: Minor: Consider wording consistency.For consistency with line 10's phrasing ("does not mean"), consider changing "is not" to "does not mean":
-`AUTH_ENABLED=false` is not "open everything". +`AUTH_ENABLED=false` does not mean "open everything".This maintains a consistent documentation voice throughout the file.
src/routes/update.ts (2)
54-58:parseBooleanduplicates the pattern already insrc/const.ts.The IIFE bodies of
HEADLESSandSKIP_ADMIN_SECRET_VALIDATIONinsrc/const.tsimplement the identical trim → lowercase → check logic. Consider extracting a sharedparseBooleanutility (e.g., insrc/const.tsorsrc/routes/utils.ts) and reusing it in both places.
240-250: Concurrent requests may trigger redundant GitHub fetches.When
cachedUpdateis null or stale, multiple simultaneous requests will all pass theisCacheValidcheck and independently callfetchLatestVersion(). This is a thundering-herd scenario that could burn through GitHub's unauthenticated rate limit (60 requests/hour).A simple guard (e.g., storing a pending
Promiseand deduplicating) would prevent redundant fetches:💡 Sketch of a deduplication approach
+let pendingFetch: Promise<void> | null = null; + +async function refreshCache(): Promise<void> { + try { + const result = await fetchLatestVersion(); + cachedUpdate = { fetchedAt: Date.now(), result }; + } catch (error) { + cachedUpdate = { + fetchedAt: Date.now(), + error: error instanceof Error ? error.message : 'Unknown update check error' + }; + } +} if (!cachedUpdate || !isCacheValid(cachedUpdate)) { - try { - const result = await fetchLatestVersion(); - cachedUpdate = { fetchedAt: Date.now(), result }; - } catch (error) { - cachedUpdate = { - fetchedAt: Date.now(), - error: error instanceof Error ? error.message : 'Unknown update check error' - }; - } + if (!pendingFetch) { + pendingFetch = refreshCache().finally(() => { pendingFetch = null; }); + } + await pendingFetch; }frontend/components/Signer.tsx (3)
521-558:authHeadersin the dependency array may cause repeated fetches on every render.If the parent component creates a new
authHeadersobject on each render (common with inline object literals), this effect will re-run and re-fetch the initial history every time. Since you already maintainauthHeadersReffor the WebSocket effect, consider reusing it here:♻️ Suggested fix
useEffect(() => { let cancelled = false; const load = async () => { try { - const res = await fetch('/api/event-log?limit=200', { headers: authHeaders }); + const res = await fetch('/api/event-log?limit=200', { headers: authHeadersRef.current }); ... } }; if (!isHeadlessMode) { void load(); } return () => { cancelled = true; }; - }, [authHeaders, isHeadlessMode]); + }, [isHeadlessMode]);
534-535: Avoidas any— define a response type instead.These casts (repeated at lines 572–573 in
handleLoadOlder) violate the project's TypeScript guidelines. A minimal interface eliminates all four occurrences:♻️ Suggested type
interface EventLogResponse { entries?: unknown[]; nextBeforeSeq?: number; }Then use:
- const entries: unknown = (payload as any)?.entries; - const nextBeforeSeq: unknown = (payload as any)?.nextBeforeSeq; + const { entries, nextBeforeSeq } = payload as EventLogResponse;As per coding guidelines, "Enable TypeScript strict mode, declare explicit types, and avoid any".
595-621:downloadingLogsin the dependency array is unnecessary and causes extra re-renders.The guard on line 596 (
if (downloadingLogs) return) already prevents concurrent calls. Including the state variable in the deps array creates a new callback reference whenever it toggles, which can trigger unnecessary re-renders of child components receiving this callback.♻️ Suggested fix
- }, [authHeaders, downloadingLogs]); + }, [authHeaders]);llm/context/NIP46_IMPLEMENTATION.md (3)
64-65: Clarify the distinction between schema and runtime behavior.The note states "revoked remains in the schema for compatibility, but revoked sessions are deleted." This might confuse readers. Consider rewording to make it clearer that the enum value exists in the schema definition, but when a session is revoked at runtime, the row is deleted rather than marked as revoked.
📝 Suggested clarification
Notes: -- `revoked` remains in the schema for compatibility, but revoked sessions are deleted and excluded from listings. +- The `revoked` status value remains in the schema CHECK constraint for compatibility, but when a session is revoked at runtime, the row is deleted from the database rather than being marked with status='revoked'.
187-187: Consider splitting for clarity.The parenthetical remark makes this sentence dense. Consider splitting into two statements to clarify the state transition flow.
📝 Suggested rewording
-4. Server creates a session with status `pending` (marked `active` on connect or activity) +4. Server creates a session with status `pending`. The session is marked `active` upon first connect or activity.
379-381: Consider adding tuning guidance.Based on learnings, timeout and rate limit parameters like
FROSTR_SIGN_TIMEOUTshould be tuned per environment (development, staging, production). Consider adding a note recommending operators adjust these values based on their specific requirements and load patterns.📝 Suggested addition
| Variable | Default | Description | |----------|---------|-------------| | `FROSTR_SIGN_TIMEOUT` | `30000` | Timeout for signing operations (ms) | | `NIP46_SESSION_RATE_LIMIT_MAX` | `120` | Max session creates per window | | `NIP46_SESSION_RATE_LIMIT_WINDOW` | `3600` | Rate limit window in seconds | + +**Note:** Adjust timeout and rate limit values based on your environment's requirements and expected load patterns.Based on learnings: "Tune FROSTR_SIGN_TIMEOUT, SIGN_TIMEOUT_MS, AUTH_DERIVED_KEY_TTL_MS, and AUTH_DERIVED_KEY_MAX_READS per environment"
…ogs now calls it after resetting state so oldestSeq gets repopulated and the 'load older' button works again
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
llm/context/NIP46_IMPLEMENTATION.md (1)
386-388:⚠️ Potential issue | 🟡 MinorMissing language specifier on fenced code block.
This code block at line 386 lacks a language tag, similar to the issue previously fixed at line 261.
✏️ Suggested fix
-``` +```text wss://relay.primal.net</details> </blockquote></details> <details> <summary>env.example (1)</summary><blockquote> `244-263`: _⚠️ Potential issue_ | _🟡 Minor_ **Example `SESSION_SECRET` values don't match the documented 64-hex-char requirement.** Lines 91–95 document that `SESSION_SECRET` must be 64 hex chars, but the example values here (e.g., `personal-session-secret-32chars`) are neither hex nor 64 characters. Since these are commented-out illustrative examples, this is low priority, but could confuse users who copy-paste them. </blockquote></details> <details> <summary>frontend/components/Signer.tsx (1)</summary><blockquote> `388-403`: _⚠️ Potential issue_ | _🟡 Minor_ **Auth credentials in WebSocket URL query parameters are visible in server logs and browser history.** Passing `apiKey` and `sessionId` as URL query params (lines 392–395) is a common trade-off for WebSocket auth, but these values will appear in server access logs, reverse proxy logs, and potentially browser history. Ensure server-side logging sanitizes query parameters containing auth tokens, or consider using a short-lived ticket/token exchange pattern where the client first obtains a one-time WebSocket ticket via an authenticated HTTP call, then passes only that ticket in the URL. </blockquote></details> </blockquote></details>🤖 Fix all issues with AI agents
In `@llm/implementation/credential-storage-implementation.md`: - Around line 14-21: The Markdown list under "Data Model" has top-level bullets for "Encrypted fields:" and "Plaintext fields:" but their child items are not indented, so they render as siblings; update the block so that the lines describing `group_cred_encrypted`, `share_cred_encrypted`, `relays`, `group_name`, `salt`, and `password_hash` are indented (e.g., two spaces or a tab) under their respective parent labels ("Encrypted fields:" and "Plaintext fields:") so they become nested list items beneath those headings. In `@package.json`: - Line 3: Update the documentation to match the released package version: find the text "version: 1.1.0" in umbrel-implementation.md and change it to "version: 1.1.1" so it matches the package.json "version": "1.1.1" and the OpenAPI specs; ensure there are no other stale occurrences of 1.1.0 in that document and commit the update. In `@src/server.ts`: - Around line 218-223: The persistUiEventLogEntry hook is typed with a loose data?: any and an entry cast to any when calling it; change its type to match UiEventLogStreamEntry from ./db/ui-event-log.js (which uses data: unknown) by updating the declaration of persistUiEventLogEntry to ((entry: UiEventLogStreamEntry) => number | null) | null and adjust createAddServerLog's persist callback signature accordingly, then remove the entry as any cast wherever you call persistUiEventLogEntry (refer to the call site that currently casts entry and to the createAddServerLog initialization) so all types align with UiEventLogStreamEntry and eliminate any usage. In `@tests/routes/relay-probe.spec.ts`: - Around line 1-4: Move the spec file tests/routes/relay-probe.spec.ts into the same directory as the backend route/module it tests (co-locate the test with the implementation) and keep the filename ending in .spec.ts (e.g., relay-probe.spec.ts next to the route file). After moving, update relative imports in the test (such as runRouteScript, pathToFileURL and bun:test imports) to use correct paths from the new location and ensure any test helper import ('./helpers/script-runner') points to the project-level helper using the proper relative path. Run the test suite to confirm the new location and updated imports (and adjust any module resolution config if needed).🧹 Nitpick comments (18)
package.json (1)
36-37:typecheckandtscscripts are identical.Both run
tsc --noEmit. Consider keeping only one (e.g.,typecheck) and removing the duplicate, or havingtscdelegate totypecheckif both names are needed for discoverability.docs/RELEASE.md (1)
37-40: Hard-coded version in release examples will drift.The branch name and PR reference are pinned to
v1.1.1. Consider using a placeholder likev<x.y.z>so the guide doesn't need updating each release, or add a note that these are examples.docs/CONFIG.md (1)
120-124: Tighten the sentence for DB_PATH definitions.“
DB_PATHcan be either:” reads like a fragment. Consider folding the bullets into a full sentence for smoother docs flow.✍️ Suggested wording
-`DB_PATH` can be either: -- A directory (e.g., `/var/lib/igloo/data`), or -- A file path (e.g., `/var/lib/igloo/igloo.db`) +`DB_PATH` can be either a directory (e.g., `/var/lib/igloo/data`) or a file path (e.g., `/var/lib/igloo/igloo.db`).llm/implementation/node-lifecycle-implementation.md (1)
67-77: Vary repeated sentence starts in the monitoring bullets.If you want to smooth readability, rephrase a couple of “Monitoring …” bullets to avoid repetition (e.g., “It also enforces…”, “It evaluates…”).
src/node/manager.ts (3)
1338-1366: Timestamp format inconsistency between WS stream and internal events.The log entry now uses ISO 8601 (
new Date().toISOString()) for the broadcast stream (line 1339), whilenew Date().toLocaleTimeString()is still used for console output (lines 1361, 1364). This is fine and intentional per the comment. However, note that the peer-status-internal broadcast at line 1572 still usesnew Date().toLocaleTimeString()for itstimestampfield, creating a format inconsistency in the WS stream between regular log events (ISO) and peer-status events (locale).Consider aligning the peer-status broadcast to ISO as well for consistent client-side parsing:
broadcastEvent({ type: 'peer-status-internal', message: '', data: { ... }, - timestamp: new Date().toLocaleTimeString(), + timestamp: new Date().toISOString(), id: Math.random().toString(36).substring(2, 11) });
1500-1514: Indentation is inconsistent within the message handler.Lines 1500–1514 are indented with extra spaces relative to the surrounding block. The
if (typeof tag === 'string')block and the tag-skip guard are indented deeper than expected for their nesting level, while subsequent lines (e.g., 1517 onwards) revert to a different indentation. This makes the control flow harder to follow.
1321-1327:data?: anyin the persist callback parameter type.The
datafield in the persist callback's entry parameter is typed asany. While this matches the existingaddServerLogsignature wheredatais untyped, the coding guidelines recommend avoidingany. Since the log data can genuinely be anything (errors, objects, primitives),unknownwould be more type-safe here, though it would require downstream changes. Flagging as optional since it's consistent with the existing pattern.src/routes/update.ts (2)
240-250: Concurrent requests can trigger duplicate GitHub fetches.If multiple requests hit
/api/updatesimultaneously while the cache is invalid, each will independently callfetchLatestVersion(). This isn't a correctness bug but could cause unnecessary GitHub API calls and potential rate-limiting.A simple mitigation is a "pending promise" pattern — store the in-flight promise and let subsequent callers await the same one.
54-58: Extract boolean-from-string parsing into a shared utility function.The
parseBooleanfunction (lines 54–58) duplicates the same pattern repeated acrosssrc/const.tsforHEADLESS,SKIP_ADMIN_SECRET_VALIDATION,SKIP_RELAY_PROBE,DEFER_RELAY_PROBE, andSKIP_STARTUP_ECHO, as well as insrc/node/manager.tsforHEADLESS_MODE. Consolidate this logic into a single exported utility (e.g.,src/utils/parse-boolean.tsor add tosrc/const.ts) to prevent drift and reduce duplication.tests/routes/headless-optimizations.spec.ts (1)
7-15:afterEachcleanup is defensive but not strictly needed for subprocess-based tests.Since
runRouteScriptspawns a separate Bun process, the parent'sprocess.envmutations don't affect test isolation. The cleanup is still reasonable as a safety net if future tests operate in-process.tests/routes/static-imports.spec.ts (1)
1-3: Test file location doesn't follow the co-location guideline.Per project guidelines, tests should be co-located with their subjects (e.g.,
src/routes/static-imports.spec.ts). However, this appears to be an established pattern in the repo with atests/routes/directory, so flagging as a nit only.As per coding guidelines, "Co-locate tests with code as
*.spec.tsor*.test.tsfiles" and "Prefer unit tests co-located with their subjects; name *.test.ts or *.spec.ts".src/const.ts (1)
95-125: Extract a shared helper for boolean env parsing to reduce duplication.The same 6-line IIFE pattern (read → trim → empty-check → lowercase → match
true/1/yes) is repeated forHEADLESS,SKIP_ADMIN_SECRET_VALIDATION,SKIP_RELAY_PROBE,DEFER_RELAY_PROBE, andSKIP_STARTUP_ECHO. A small helper would DRY this up.♻️ Proposed refactor
Add a helper at the top of the file:
function parseBoolEnv(key: string): boolean { const value = process.env[key]; if (!value) return false; const trimmed = value.trim(); if (trimmed.length === 0) return false; const normalized = trimmed.toLowerCase(); return normalized === 'true' || normalized === '1' || normalized === 'yes'; }Then replace each IIFE:
-export const SKIP_RELAY_PROBE = (() => { - const value = process.env['SKIP_RELAY_PROBE']; - if (!value) return false; - const trimmed = value.trim(); - if (trimmed.length === 0) return false; - const normalized = trimmed.toLowerCase(); - return normalized === 'true' || normalized === '1' || normalized === 'yes'; -})(); +export const SKIP_RELAY_PROBE = parseBoolEnv('SKIP_RELAY_PROBE');Same for
DEFER_RELAY_PROBE,SKIP_STARTUP_ECHO,HEADLESS, andSKIP_ADMIN_SECRET_VALIDATION.src/db/ui-event-log.test.ts (1)
37-37: Multipleas anycasts could be replaced with a typed assertion.Since
getBlobreturns{ data: unknown; byteLength: number } | null, you could narrow with a non-null assertion and type thedatafield more precisely, avoidingany.Example for line 37:
const blob = store.getBlob(e1.dataHash!)!; expect(blob.data).toEqual(commonData);As per coding guidelines, "Enable TypeScript strict mode, declare explicit types, and avoid any".
Also applies to: 57-59, 76-78
src/db/ui-event-log.ts (2)
1-11:unknown | nullis redundant —unknownalready includesnull.On lines 8, 22, 38 (and later at 311, 385), the type
unknown | nullis equivalent to justunknown. If the intent is to signal explicit nullability, consider using a concrete type (e.g.,Record<string, unknown> | null) or justunknown.Also applies to: 13-24, 26-39
191-203: Missing explicit return type oncreateUiEventLogStore.The factory function's return type is inferred. Per the coding guidelines ("explicit types, avoid
any"), consider declaring an explicit interface for the store object returned by this function.Suggested approach
+interface UiEventLogStore { + append(entry: UiEventLogStreamEntry): { seq: number; dataHash: string | null }; + list(opts?: { limit?: number; beforeSeq?: number; types?: string[] }): UiEventLogListResult; + getBlob(hash: string): { data: unknown; byteLength: number } | null; + exportChunk(opts?: { afterSeq?: number; untilSeq?: number; limit?: number; types?: string[] }): { rows: UiEventLogExportRow[]; nextAfterSeq: number | null }; +} + -export function createUiEventLogStore(dbConn: Database) { +export function createUiEventLogStore(dbConn: Database): UiEventLogStore {As per coding guidelines, "TypeScript strict mode; explicit types, avoid
any".frontend/components/Signer.tsx (3)
522-550:loadInitialHistoryusesauthHeadersdirectly — stale closure possible if headers change before the fetch resolves.
loadInitialHistorycapturesauthHeadersfrom itsuseCallbackdependency. Since it's also called fromhandleClearLogs(which depends onloadInitialHistory), ifauthHeaderschanges between the clear and the fetch, the request uses the old headers. This is low-risk since auth headers change infrequently, but using the ref (authHeadersRef.current) would be more consistent with the WebSocket approach.
530-533: Use a typed response interface instead ofas anycasts.Lines 532–533 cast the response payload to
anyto access.entriesand.nextBeforeSeq. A simple interface would satisfy the "avoidany" guideline and improve type safety.Suggested approach
+interface EventLogResponse { + entries?: unknown[]; + nextBeforeSeq?: number | null; +} + const payload = await res.json(); - const entries: unknown = (payload as any)?.entries; - const nextBeforeSeq: unknown = (payload as any)?.nextBeforeSeq; + const { entries, nextBeforeSeq } = payload as EventLogResponse;Apply the same pattern to
handleLoadOlderat lines 571–572.As per coding guidelines, "TypeScript strict mode; explicit types, avoid
any".
150-150:checkServerStatuscloses overauthHeadersfrom props but also reads staleisSignerRunningvia closure.
checkServerStatusdepends onisSignerRunning(line 304), so it re-creates on every state change. However, line 277 usesauthHeadersfrom the outer scope (props), not the ref. Since theuseEffectat line 318 re-subscribes on everycheckServerStatuschange, this creates a new interval every timeisSignerRunningtoggles. This is functional but causes unnecessary interval churn. Consider using a ref forisSignerRunninginside the callback to keep the dependency array stable.Also applies to: 274-304
auth/env hardening and reliability updates
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
frontend/components/nip46/QRScanner.tsx (1)
47-56:⚠️ Potential issue | 🟠 MajorPrevent scanner freeze when
onResultthrows.At Line 51, if
onResultRef.current(result.data)throws, Lines 52-56 are skipped, leavingscanningRef.current = trueand blocking future scans.🐛 Suggested hardening
(result: QrScanner.ScanResult) => { if (scanningRef.current) return if (result.data && result.data.startsWith('nostrconnect://')) { scanningRef.current = true - onResultRef.current(result.data) - scannerRef.current?.destroy() - scannerRef.current = null - timeoutRef.current = setTimeout(() => { - scanningRef.current = false - }, 1000) + try { + onResultRef.current(result.data) + } catch (callbackError) { + const e = callbackError instanceof Error ? callbackError : new Error('QR result handler failed') + onErrorRef.current?.(e) + setError(e.message) + } finally { + scannerRef.current?.destroy() + scannerRef.current = null + timeoutRef.current = setTimeout(() => { + scanningRef.current = false + }, 1000) + } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/nip46/QRScanner.tsx` around lines 47 - 56, The QR scan handler can leave scanningRef.current stuck true if onResultRef.current(result.data) throws; update the handler inside the QrScanner.ScanResult callback to call onResultRef.current(result.data) inside a try/finally (awaiting it via Promise.resolve to handle sync or async errors) and perform the cleanup in the finally block: destroy scannerRef.current, set scannerRef.current = null, and set/reset timeoutRef.current to flip scanningRef.current back to false; also clear any previous timeoutRef before assigning a new one. Refer to scanningRef, onResultRef, scannerRef, timeoutRef and the QrScanner.ScanResult callback when making the change.frontend/components/ui/input-with-validation.tsx (1)
5-12:⚠️ Potential issue | 🟡 MinorRemove trailing whitespace on line 55.
Line 55 has trailing whitespace after the semicolon in the export statement. All existing usages of
InputWithValidationcorrectly pass theisRequiredprop, so the required prop declaration is appropriate and not a breaking change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/input-with-validation.tsx` around lines 5 - 12, Remove the trailing whitespace after the semicolon in the export statement for the InputWithValidation component/interface (the export referring to InputWithValidation or InputWithValidationProps) so the file has no trailing spaces; locate the export that references InputWithValidation (or the InputWithValidationProps interface) and delete the extra space character after the semicolon to fix the lint/trailing-whitespace issue.src/routes/peers.ts (1)
794-811:⚠️ Potential issue | 🟡 MinorRemove
as anycasts in ping result checks.This violates the TypeScript strict typing requirement. There are two instances (lines 810 and 876) — use type narrowing instead.
♻️ Suggested typed check
- if ((result as any).ok) { + const pingResult = result as { ok?: unknown }; + if (pingResult.ok === true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/peers.ts` around lines 794 - 811, The ping result checks currently use unsafe "as any" casts; create a proper type guard (e.g., isPingResponse(obj): obj is PingResponse) that verifies result is an object and has an ok boolean property, then replace the two uses of "(result as any).ok" (inside the mapWithConcurrency callback where result comes from context.node.req.ping, and the second occurrence around line 876) with a guarded check like "if (isPingResponse(result) && result.ok)". Add a PingResponse interface (ok: boolean, plus any other expected fields) and reuse the isPingResponse guard wherever ping results are inspected to satisfy TypeScript strict typing.frontend/components/Configure.tsx (1)
119-125:⚠️ Potential issue | 🟡 MinorPrevent stale aborted requests from clearing active loading state.
When a new request to
loadAdvancedSettingsis initiated, it overwritesloadAdvancedSettingsRef.currentwith a new AbortController, causing the previous request to be aborted. However, the aborted request'sfinallyblock (line 173) still executes and clearsisLoadingAdvancedtofalse, even though the new request is still in flight. This creates a race condition where the loading indicator disappears prematurely.The early return in the
catchblock (line 167) prevents error handling but does not prevent thefinallyblock from executing. To fix this, store the controller in a local variable and only clear the loading state if the current ref still matches that controller:Proposed fix
- loadAdvancedSettingsRef.current = new AbortController(); - const signal = loadAdvancedSettingsRef.current.signal; + const controller = new AbortController(); + loadAdvancedSettingsRef.current = controller; + const signal = controller.signal; ... - } finally { - setIsLoadingAdvanced(false); + } finally { + if (loadAdvancedSettingsRef.current === controller) { + setIsLoadingAdvanced(false); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 119 - 125, The race condition occurs in loadAdvancedSettings: when you replace loadAdvancedSettingsRef.current with a new AbortController the previous request's finally block can still clear isLoadingAdvanced even though a newer request is active; fix it by creating a local const controller = new AbortController() and assigning it to loadAdvancedSettingsRef.current, use controller.signal for the fetch, and in the finally block only set isLoadingAdvanced(false) if loadAdvancedSettingsRef.current === controller (i.e., the controller is still the current one); ensure catch still handles AbortError appropriately but does not rely on early returns to prevent finally from running.frontend/components/Onboarding.tsx (1)
95-97:⚠️ Potential issue | 🟠 MajorSkip-admin mode can still be blocked by the admin-secret gate.
skipAdminValidationis set from status, but the later!hasAdminSecretearly return still blocks onboarding in skip-admin deployments.Suggested fix
- if (!hasAdminSecret) { + if (!hasAdminSecret && !skipAdminValidation) { return ( <PageLayout>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Onboarding.tsx` around lines 95 - 97, The onboarding flow sets skipAdminValidation via setSkipAdminValidation in the success branch (skipAdminValidation), but the component still returns early when !hasAdminSecret, preventing skip-admin deployments from proceeding; update the early-return logic that checks hasAdminSecret (or wherever hasAdminSecret is used to block rendering) to also allow continuation when skipAdminValidation is true (i.e., treat skipAdminValidation as bypass of the admin-secret gate), ensuring the UI/render path uses the skipAdminValidation state variable instead of always blocking on hasAdminSecret.src/routes/env.ts (1)
727-729:⚠️ Potential issue | 🟠 MajorDeleting
RELAYSdoes not reconfigure the running node.When only
RELAYSis deleted,.env/process.envare updated, but node restart/cleanup is skipped. That leaves active relay connections stale until another restart event.💡 Proposed fix
- const deletingCredentials = validKeys.some(key => ['GROUP_CRED', 'SHARE_CRED'].includes(key)); + const deletingCredentials = validKeys.some((key) => ['GROUP_CRED', 'SHARE_CRED'].includes(key)); + const deletingRelays = validKeys.includes('RELAYS'); ... - if (deletingCredentials) { + if (deletingCredentials) { try { // Use synchronized cleanup to prevent race conditions await cleanupNodeSynchronized(context); } catch (error) { // Error already logged by executeUnderNodeLock // Continue anyway - the env vars were deleted } + } else if (deletingRelays) { + const canRecreateNode = + typeof env.GROUP_CRED === 'string' && + env.GROUP_CRED.length > 0 && + typeof env.SHARE_CRED === 'string' && + env.SHARE_CRED.length > 0; + if (canRecreateNode) { + try { + await executeUnderNodeLock(async () => { + await createAndConnectServerNode(env, context); + }, context); + } catch (error) { + return Response.json( + { success: false, message: 'Failed to recreate Bifrost node after RELAYS deletion' }, + { status: 500, headers } + ); + } + } }Also applies to: 735-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/env.ts` around lines 727 - 729, The code only sets deletingCredentials from validKeys.includes(['GROUP_CRED','SHARE_CRED']) and therefore skips the restart/cleanup path when RELAYS is removed; update the deletion detection so RELAYS triggers the same reconfiguration flow: modify the check around deletingCredentials (and/or add a deletingRelays boolean) to include 'RELAYS' (e.g., ['GROUP_CRED','SHARE_CRED','RELAYS']) or OR the new flag into the condition that runs the node restart/cleanup logic, and ensure the branch that performs the restart/cleanup (the same flow currently executed for deletingCredentials in the block covering lines ~735-754) is executed when RELAYS is deleted so active relay connections are properly cleaned up and reconfigured.src/nip46/service.ts (1)
211-212:⚠️ Potential issue | 🟠 MajorDon’t swallow startup failures in
ensureStarted().
ensureStarted()logs and resolves even whenstartInternal()fails; callers can continue with a non-running agent.Suggested patch
setActiveUser(userId: number | bigint | null) { @@ if (userId == null) { void this.stop() } else { - void this.ensureStarted() + void this.ensureStarted().catch((error) => { + this.log('error', 'Failed to start NIP-46 service', { error: this.serializeError(error) }) + }) } } @@ this.startingPromise = (async () => { try { await this.startInternal() } catch (error) { this.log('error', 'Failed to start NIP-46 service', { error: this.serializeError(error) }) + throw error } finally { this.startingPromise = null } })()Also applies to: 223-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nip46/service.ts` around lines 211 - 212, The current ensureStarted() implementation catches errors from startInternal(), logs them and still resolves, which lets callers proceed with a non-running agent; change ensureStarted() to let startInternal() failures propagate (or rethrow after logging) so the returned Promise rejects on startup failure, and apply the same change to the similar startup block around the start/startInternal usage (the other block referencing startInternal at lines 223-231) so neither path swallows errors; ensure you reference and modify the ensureStarted and startInternal call sites and update any tests/callers that assumed silent failures.
♻️ Duplicate comments (8)
frontend/components/ui/log-entry.tsx (1)
112-118:⚠️ Potential issue | 🟡 MinorHandle missing
datain successful blob responses as an explicit failure.The success branch still silently no-ops when payload lacks
data; this should set an error state so the UI can communicate failure.Suggested fix
.then(payload => { if (cancelled) return; if (payload && typeof payload === 'object' && 'data' in payload) { setResolvedData((payload as any).data); setHasFetchedFull(true); + } else { + setFetchError('failed_to_load_payload'); } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/log-entry.tsx` around lines 112 - 118, In the promise .then handler, explicitly treat a successful payload that lacks a data property as a failure: after the cancelled check and the existing branch that calls setResolvedData(...) and setHasFetchedFull(true), add the else branch that sets an error state (e.g. call your component's error setter like setBlobFetchError or setFetchError with a descriptive Error or message) and ensure setHasFetchedFull(false) (and clear any partial resolved data if needed) so the UI can show the failure; keep using the same symbols shown (payload, cancelled, setResolvedData, setHasFetchedFull) to locate where to add the error handling.frontend/components/Signer.tsx (1)
535-536:⚠️ Potential issue | 🟠 MajorReplace
payload as anywith typed narrowing in both history loaders.Lines 535-536 and 574-575 bypass strict typing and can hide malformed server payloads at runtime.
🔧 Suggested change
+type EventLogPayload = { + entries?: unknown; + nextBeforeSeq?: unknown; +}; + +const asEventLogPayload = (value: unknown): EventLogPayload => + value && typeof value === 'object' ? (value as EventLogPayload) : {}; + const payload = await res.json(); - const entries: unknown = (payload as any)?.entries; - const nextBeforeSeq: unknown = (payload as any)?.nextBeforeSeq; + const { entries, nextBeforeSeq } = asEventLogPayload(payload);#!/bin/bash rg -nP "\(payload as any\)\?\.(entries|nextBeforeSeq)" frontend/components/Signer.tsx -C2As per coding guidelines, "
**/*.{ts,tsx}: TypeScript strict mode; explicit types, avoidany."Also applies to: 574-575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Signer.tsx` around lines 535 - 536, The code bypasses TypeScript strictness by using "payload as any" for entries and nextBeforeSeq; replace those casts with a narrow type-check: add a type/interface (e.g., HistoryPayload { entries: unknown[]; nextBeforeSeq?: number | string | null }) and a type guard function (e.g., isHistoryPayload(payload): payload is HistoryPayload) that validates Array.isArray(payload.entries) and the optional nextBeforeSeq shape, then in both history loader blocks use the guard to safely extract const entries = payload.entries and const nextBeforeSeq = payload.nextBeforeSeq only when isHistoryPayload(payload) returns true (handle the else branch by returning a safe default or throwing). Ensure you update the two sites that reference entries and nextBeforeSeq (the history loader variables) to use this typed narrowing instead of "payload as any".docs/AUTH_MATRIX.md (1)
17-17:⚠️ Potential issue | 🟡 MinorClarify
/api/updatebehavior in headless/managed modes.This row currently says the endpoint is available (
Yes) while the note says update checks are disabled in those same modes. Please state explicit behavior (e.g., “endpoint exists but returns disabled status/payload”) to remove ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/AUTH_MATRIX.md` at line 17, Update the `/api/update` entry in AUTH_MATRIX.md to remove ambiguity by changing the availability cell from "Yes" to a clear statement like "Yes — endpoint exists but returns a disabled/unavailable response in managed/headless modes" and adjust the note to explicitly state that while the route is present, update checks are disabled when HEADLESS=true, SKIP_ADMIN_SECRET_VALIDATION=true, or UPDATE_CHECK_DISABLED=true (so consumers know the endpoint will respond with a disabled status/payload rather than performing update checks).src/routes/update.ts (3)
129-137:⚠️ Potential issue | 🔴 CriticalFix
HeadersInitmutation typing to satisfy strict TypeScript.Line 136 mutates a
HeadersInitunion with string indexing, which is not type-safe in strict mode.As per coding guidelines, "TypeScript strict mode; explicit types, avoid `any`".Suggested patch
-function buildHeaders(): HeadersInit { - const headers: HeadersInit = { +function buildHeaders(): Record<string, string> { + const headers: Record<string, string> = { 'Accept': 'application/vnd.github+json', 'User-Agent': 'igloo-server' }; const token = process.env['GITHUB_TOKEN']?.trim(); if (token) { headers['Authorization'] = `Bearer ${token}`; } return headers; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/update.ts` around lines 129 - 137, The buildHeaders function mutates a HeadersInit union via string indexing which fails in strict TS; change the implementation to use a concrete mutable type (e.g., create a Record<string,string> or a Headers instance) so you can safely add Authorization. In practice, inside buildHeaders replace the current headers variable with a mutable object type (Record<string,string>) or instantiate new Headers(), set the Accept and User-Agent, then conditionally add the Authorization header using the safe setter (e.g., headers['Authorization'] = ... for Record or headers.set(...) for Headers) and return it as HeadersInit; reference function name buildHeaders and the Authorization assignment site to locate the change.
222-223:⚠️ Potential issue | 🟡 MinorInclude an
Allowheader in the 405 response.The method-not-allowed branch should advertise allowed methods (
GET, OPTIONS).Suggested patch
if (req.method !== 'GET') { - return Response.json({ error: 'Method not allowed' }, { status: 405, headers }); + return Response.json( + { error: 'Method not allowed' }, + { status: 405, headers: { ...headers, Allow: 'GET, OPTIONS' } } + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/update.ts` around lines 222 - 223, The 405 branch in the request handler currently returns Response.json without an Allow header; update the branch that checks if (req.method !== 'GET') to include an 'Allow' header set to 'GET, OPTIONS' when creating the Response (reuse or extend the existing headers object used in this file), so the Response.json call for the Method Not Allowed path advertises allowed methods; ensure the header key is exactly 'Allow' and returned together with status 405.
117-120:⚠️ Potential issue | 🟡 MinorPrerelease comparison is lexicographic, not SemVer precedence-aware.
localeComparecan misorder versions likerc.10vsrc.9, which can flipupdateAvailable.Suggested patch
+function comparePrerelease(a: string, b: string): number { + const aParts = a.split('.'); + const bParts = b.split('.'); + const len = Math.max(aParts.length, bParts.length); + for (let i = 0; i < len; i++) { + const av = aParts[i]; + const bv = bParts[i]; + if (av === undefined) return -1; + if (bv === undefined) return 1; + const aNum = /^\d+$/.test(av); + const bNum = /^\d+$/.test(bv); + if (aNum && bNum) { + const diff = Number(av) - Number(bv); + if (diff !== 0) return diff; + continue; + } + if (aNum && !bNum) return -1; + if (!aNum && bNum) return 1; + if (av !== bv) return av < bv ? -1 : 1; + } + return 0; +} + function compareVersions(a: ParsedVersion, b: ParsedVersion): number { @@ - if (a.prerelease && b.prerelease) return a.prerelease.localeCompare(b.prerelease); + if (a.prerelease && b.prerelease) return comparePrerelease(a.prerelease, b.prerelease);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/update.ts` around lines 117 - 120, The prerelease comparison using a.prerelease.localeCompare(b.prerelease) is lexicographic and misorders identifiers like "rc.10" vs "rc.9"; replace that comparison with a SemVer-aware comparison (e.g., use the semver package to compare full versions or compare prerelease identifier arrays) so numeric identifier segments are compared numerically and alphanumeric segments lexically; specifically, in the block that currently checks a.prerelease and b.prerelease, call semver.compare(a.version, b.version) (or parse semver.prerelease(...) and compare segments numerically then lexically) instead of localeCompare to determine precedence.src/routes/index.ts (1)
222-224:⚠️ Potential issue | 🟡 Minor
/api/updateis public, which exposes version/deployment metadata to unauthenticated callers.Line 223 puts this route in
publicEndpoints, so exact version data is available even when auth is enabled.Suggested patch (strictest option)
const publicEndpoints = [ @@ - '/api/onboarding/setup', - '/api/update' + '/api/onboarding/setup' ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.ts` around lines 222 - 224, The route '/api/update' is currently listed in publicEndpoints, exposing version/deployment metadata to unauthenticated callers; remove '/api/update' from the publicEndpoints array (or move it to a protectedRoutes list) and ensure the update handler (the route/controller that serves '/api/update') enforces authentication/authorization before returning any sensitive metadata, or alternatively create a non-sensitive public summary endpoint and keep detailed version info behind auth; update any middleware that uses publicEndpoints to stop skipping auth for '/api/update'.docs/openapi/openapi.json (1)
164-236:⚠️ Potential issue | 🟡 Minor
/api/eventsstill documents unconditional auth via inherited global security.The operation description says auth is conditional (
AUTH_ENABLED=true), but without an explicit override it still inherits globalsecurity. Add a route-level override (and public marker) to match runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 164 - 236, The /api/events GET operation (operationId "eventsWebSocket") currently inherits global security but is conditionally public at runtime; modify that operation to explicitly override global security by adding an empty security array (e.g. "security": []) and add a public marker (e.g. vendor extension "x-public": true) to the GET operation object so the OpenAPI doc matches runtime behavior when AUTH_ENABLED is false.
🟡 Minor comments (24)
frontend/components/nip46/RelaySettings.tsx-37-43 (1)
37-43:⚠️ Potential issue | 🟡 MinorAdd explicit return type to
handleRemove.The async function at line 37 is missing an explicit return type annotation, violating the TypeScript strict mode guideline.
Suggested change
- const handleRemove = async (relay: string) => { + const handleRemove = async (relay: string): Promise<void> => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/nip46/RelaySettings.tsx` around lines 37 - 43, The function handleRemove is an async handler missing an explicit return type; update its signature to declare the return type (e.g., Promise<void>) so TypeScript strict mode is satisfied, keeping the body the same and referencing the existing onRemove(relay) call and the try/catch block in handleRemove.frontend/components/ui/modal.tsx-47-51 (1)
47-51:⚠️ Potential issue | 🟡 MinorRestore focus to the previously focused element on close.
Line 50 moves focus into the modal, but focus is not restored after close. Keyboard users can lose context.
💡 Suggested fix
useEffect(() => { if (!isOpen) return; + const previousActive = document.activeElement as HTMLElement | null; // Ensure screen readers and keyboard users land on the active dialog. dialogRef.current?.focus(); + return () => { + previousActive?.focus(); + }; }, [isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/modal.tsx` around lines 47 - 51, When opening the modal in the useEffect that currently focuses dialogRef (useEffect(() => { if (!isOpen) return; dialogRef.current?.focus(); }, [isOpen])), capture the currently focused element (e.g., const previousFocus = document.activeElement as HTMLElement | null) and on cleanup/close restore focus to that saved element (previousFocus?.focus()) if it still exists; implement this by turning the effect into one that saves previousFocus when isOpen becomes true and returns a cleanup function that restores previousFocus when the modal closes, using the existing dialogRef and isOpen symbols.frontend/components/Configure.tsx-175-175 (1)
175-175:⚠️ Potential issue | 🟡 MinorStabilize
authHeadersidentity to prevent repeated reload effects.Because dependencies at Line 175 and Line 318 track
authHeaders, the default object literal (authHeaders = {}) can trigger repeated effect reruns when the prop is omitted, causing avoidable refetch/state churn. While the component is currently always instantiated with an explicitauthHeadersprop, using a frozen constant prevents this latent bug.💡 Proposed fix
+const EMPTY_AUTH_HEADERS: Readonly<Record<string, string>> = Object.freeze({}); + -const Configure: React.FC<ConfigureProps> = ({ onKeysetCreated, onCredentialsSaved, onBack, authHeaders = {} }) => { +const Configure: React.FC<ConfigureProps> = ({ onKeysetCreated, onCredentialsSaved, onBack, authHeaders = EMPTY_AUTH_HEADERS }) => {Also applies to: 318-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` at line 175, The component's authHeaders default object causes unstable identity and repeated effect reruns; define a single frozen constant (e.g. DEFAULT_AUTH_HEADERS = Object.freeze({})) and use it as the default value for the Configure component's authHeaders prop (or in defaultProps) so the identity is stable, then ensure all effects that list authHeaders (the effect referencing authHeaders in Configure and the other effect that also depends on authHeaders) use that stabilized DEFAULT_AUTH_HEADERS as their default/initial value.llm/implementation/umbrel-implementation.md-38-39 (1)
38-39:⚠️ Potential issue | 🟡 MinorUpdate Bun base-image version in documentation to match actual Dockerfile.
Line 38 hardcodes
oven/bun:1.1.30, but bothpackages/umbrel/igloo/Dockerfileand the rootDockerfileuseoven/bun:1.3.10. Update the documentation to reflect the current version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llm/implementation/umbrel-implementation.md` around lines 38 - 39, The documentation currently hardcodes the Bun base image tag "oven/bun:1.1.30"; update the mention in umbrel-implementation.md to "oven/bun:1.3.10" so it matches the actual Dockerfiles, and scan the doc for any other references of "oven/bun:1.1.30" (e.g., the multi-stage build and runtime stage notes) and replace them with "oven/bun:1.3.10" to keep the docs and the Dockerfiles consistent.src/routes/env.ts-437-437 (1)
437-437:⚠️ Potential issue | 🟡 MinorFix env parameter typing to eliminate the
as anycast.The root issue is the
env: anyparameter at line 27; this forces downstream workarounds like the cast at line 437 and violates strict typing requirements. Define env asRecord<string, any>or a specific interface instead, then remove the redundantas anycast at line 437.Per coding guidelines:
**/*.{ts,tsx}: TypeScript strict mode; explicit types, avoidany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/env.ts` at line 437, The env parameter is typed as any which forces the cast (env as any). Change the function signature that declares the env parameter (the env parameter at its declaration) to use a concrete type such as Record<string, any> or a dedicated interface describing keys like RELAYS, then remove the redundant cast when assigning relaysVal (const relaysVal = (env as any).RELAYS) and use env.RELAYS directly; update any other usages in the same module to the new type to satisfy strict TypeScript rules.src/routes/onboarding.ts-270-273 (1)
270-273:⚠️ Potential issue | 🟡 MinorAddress inconsistent PASSWORD_REGEX definitions across the codebase.
The regex change from
[^A-Za-z0-9]to[^A-Za-z0-9\s]correctly excludes whitespace from counting as a special character. This is properly documented in the error message at line 348 ("excluding whitespace") and consistently implemented in both the backend (onboarding.ts) and frontend (Onboarding.tsx).However, src/config/crypto.ts still defines the old regex pattern without the
\sexclusion. This unused definition should be updated to match or removed to avoid confusion, especially since theisPasswordValid()function there would enforce different rules if called elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/onboarding.ts` around lines 270 - 273, Update the stale regex in src/config/crypto.ts so it matches the onboarding PASSWORD_REGEX behavior (exclude whitespace from counting as a special char) or remove the unused definition; specifically, ensure the pattern used by the PASSWORD_REGEX in onboarding.ts (/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9\s]).*$/) is mirrored in crypto.ts and that any function there (e.g., isPasswordValid) uses that same pattern to avoid divergent password rules across the codebase.tests/routes/env.db-mode.spec.ts-10-14 (1)
10-14:⚠️ Potential issue | 🟡 MinorAvoid fallback hard-coded key material in test setup.
Line 13 still embeds a literal secret value. Prefer env/fixture-only input so secret handling stays policy-compliant across all TS files.
🔧 Suggested change
const TEST_KEYSET_SECRET = normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) ?? - normalizeOptionalEnv(process.env.TEST_NSEC_HEX) ?? - 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'; + normalizeOptionalEnv(process.env.TEST_NSEC_HEX); + +if (!TEST_KEYSET_SECRET) { + throw new Error('Set TEST_KEYSET_SECRET (or TEST_NSEC_HEX) for env.db-mode.spec.ts'); +}As per coding guidelines, "Never hard-code secrets; load from environment or data/ fixtures."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/env.db-mode.spec.ts` around lines 10 - 14, The test currently falls back to a hard-coded secret in the TEST_KEYSET_SECRET assignment; remove the literal fallback and instead require/tests to source the secret from environment or a test fixture: replace the fallback branch of the TEST_KEYSET_SECRET initialization (the use of normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) ?? normalizeOptionalEnv(process.env.TEST_NSEC_HEX) ?? 'deadbeef...') so that if both normalizeOptionalEnv(process.env.TEST_KEYSET_SECRET) and normalizeOptionalEnv(process.env.TEST_NSEC_HEX) are undefined the test either throws a clear error instructing to set the env var or reads the secret from a fixtures loader (e.g., a test fixture loader for test key material), ensuring no hard-coded secret remains; update any test setup harness that references TEST_KEYSET_SECRET accordingly.tests/routes/env.db-mode.spec.ts-117-123 (1)
117-123:⚠️ Potential issue | 🟡 Minor
process.exit(0)undermines the new cleanupfinallyblock.Line 117 exits the process immediately after logging, so the cleanup in Lines 118-123 is not guaranteed to run as intended. Let the script return naturally.
🔧 Suggested change
- console.log('@@RESULT@@' + JSON.stringify({ status, hasStamp })); - process.exit(0); + console.log('@@RESULT@@' + JSON.stringify({ status, hasStamp })); + return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/env.db-mode.spec.ts` around lines 117 - 123, The test calls process.exit(0) which aborts execution before the surrounding finally block can run; remove the process.exit(0) call (or replace it with a normal return) so the finally section that restores cwd via process.chdir(originalCwd) and removes the temp dir with fs.rmSync(tmp, { recursive: true, force: true }) always executes; update the code paths in the same try/catch/finally block (the section around process.exit(0), process.chdir(originalCwd), and fs.rmSync) to return or let the function complete normally instead of exiting the process.tests/routes/static-imports.spec.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorCo-locate this backend spec with the module under test.
This new route/backend spec is still under
tests/routesinstead of next to its subject.As per coding guidelines:
**/*.{test,spec}.ts: Co-locate backend tests with code and name them feature.test.ts or feature.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/static-imports.spec.ts` around lines 1 - 4, This spec file is in tests/routes but must be co-located with the module it tests; move and rename the file next to the module under test using the naming convention feature.test.ts or feature.spec.ts, update any relative imports (e.g., runRouteScript and other imports like afterEach/describe/expect/test and pathToFileURL) to the new location so they resolve correctly, and ensure any test runner configuration still picks up the new filename pattern.src/routes/sign.ts-251-255 (1)
251-255:⚠️ Potential issue | 🟡 MinorHandle uppercase
0Xprefix when normalizing signatures.Current normalization only strips lowercase
0x, so0X...gets rejected even when the signature is otherwise valid.🛠️ Proposed fix
- if (typeof signatureHex === 'string' && signatureHex.startsWith('0x')) { + if (typeof signatureHex === 'string' && /^0x/i.test(signatureHex)) { signatureHex = signatureHex.slice(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/sign.ts` around lines 251 - 255, The normalization only removes a lowercase '0x' prefix so signatures beginning with '0X' are rejected; update the logic around the signatureHex variable (the startsWith/regex check and subsequent validation using /^[0-9a-fA-F]{128}$/) to strip either '0x' or '0X' (e.g., check startsWith('0x') || startsWith('0X') or normalize the prefix to lowercase before slicing) before running the hex-length regex, ensuring signatureHex remains a string for the /^[0-9a-fA-F]{128}$/ test.tests/routes/headless-optimizations.spec.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorPlace this backend spec alongside its subject module.
The new spec is not co-located with the code it validates.
As per coding guidelines:
**/*.{test,spec}.ts: Prefer unit tests co-located with their subjects; name *.test.ts or *.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/headless-optimizations.spec.ts` around lines 1 - 4, This spec file (tests/routes/headless-optimizations.spec.ts) should be co-located with the module it tests: move the file from tests/routes/ to the same directory as the subject module (rename or place as headless-optimizations.spec.ts alongside that module), update any relative imports (e.g., runRouteScript and pathToFileURL) to the new location, and ensure the test filename matches the project glob pattern (*.test.ts or *.spec.ts) so the runner picks it up; keep the existing test contents and imports (afterEach, describe, expect, test, runRouteScript) but fix import paths after relocation.tests/routes/static-imports.spec.ts-332-349 (1)
332-349:⚠️ Potential issue | 🟡 MinorTest intent doesn’t match implementation (
server.tsis never imported).The test name and comments claim
server.tsimport coverage, but the script only importssrc/routes/auth.tsandsrc/utils/rate-limiter.ts.♻️ Proposed fix
- test('server.ts can import auth statically without circular dependency', () => { + test('auth.ts and rate-limiter.ts import statically without circular dependency', () => { ... - // This tests that the server module can be imported - // without circular dependency issues after adding static imports + // This tests that auth/rate-limiter static imports do not + // introduce circular dependency issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/static-imports.spec.ts` around lines 332 - 349, The test name says it verifies importing server.ts but the script never imports it; update the test body for "server.ts can import auth statically without circular dependency" to actually import the server module (e.g., await import(root + 'src/server.ts')) after setting process.env, so the test triggers any circular-import errors from Server initialization; keep or also await the existing imports (src/routes/auth.ts and src/utils/rate-limiter.ts) if needed, and ensure the awaits are inside the try/catch so importError captures failures.scripts/release.sh-95-95 (1)
95-95:⚠️ Potential issue | 🟡 MinorQuote
VERSION_TYPEin thenpm versioninvocation.Line 95 is unquoted, so accidental whitespace/extra tokens in input can alter command behavior.
Suggested patch
- NEW_VERSION=$(npm version $VERSION_TYPE --no-git-tag-version) + NEW_VERSION=$(npm version "$VERSION_TYPE" --no-git-tag-version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.sh` at line 95, The npm version invocation uses an unquoted shell variable and should quote VERSION_TYPE to avoid word-splitting or globbing; update the command that assigns NEW_VERSION (the line invoking npm version with VERSION_TYPE) to pass "$VERSION_TYPE" (preserving the variable name NEW_VERSION and the npm version call) so the shell treats the entire variable value as a single argument.src/routes/admin.ts-139-144 (1)
139-144:⚠️ Potential issue | 🟡 MinorClamp fallback
Retry-Afterto positive seconds.If
RATE_LIMIT_WINDOWis0/negative, the fallback header can become invalid.Suggested patch
- const validatedWindowSeconds = Number.isFinite(windowSecondsRaw) ? windowSecondsRaw : 900; + const validatedWindowSeconds = + Number.isFinite(windowSecondsRaw) && windowSecondsRaw > 0 ? windowSecondsRaw : 900;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/admin.ts` around lines 139 - 144, The fallback Retry-After can be zero or negative if RATE_LIMIT_WINDOW is 0/negative; update the logic around windowSecondsRaw/validatedWindowSeconds/fallbackRetryAfterSeconds to clamp the fallback to at least 1 second (e.g., compute fallbackRetryAfterSeconds from Math.max(1, Math.ceil(validatedWindowSeconds))). Keep the existing retryAfterSeconds branch that uses rate.resetAt unchanged but ensure fallbackRetryAfterSeconds is always a positive integer string.src/nip46/service.ts-494-495 (1)
494-495:⚠️ Potential issue | 🟡 MinorGuard duplicate lookup when request id is missing.
String(req.id)turns absent ids into"undefined"/"null", which can cause incorrect duplicate suppression.Suggested patch
- const existing = getPendingNip46RequestByClientId(this.activeUserId, pubkey, String(req.id)) + if (req.id == null) { + await this.sendSocketResponse(req, pubkey, { error: 'Invalid request id' }) + return + } + const requestId = String(req.id) + const existing = getPendingNip46RequestByClientId(this.activeUserId, pubkey, requestId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nip46/service.ts` around lines 494 - 495, The duplicate-request check currently calls getPendingNip46RequestByClientId(this.activeUserId, pubkey, String(req.id)), which converts missing req.id into the string "undefined"/"null" and can incorrectly suppress new requests; update the logic in the handler around this.activeUserId/pubkey/req.id to first guard for a present, valid req.id (e.g., if req.id == null or === undefined) and only call getPendingNip46RequestByClientId when req.id exists, otherwise skip the duplicate lookup so absent IDs do not match an accidental "undefined"/"null" key.src/routes/admin.ts-465-469 (1)
465-469:⚠️ Potential issue | 🟡 MinorMethod-not-allowed responses should include
Allow.The new known-path 405 branch returns status code correctly, but omits allowed methods metadata.
Suggested patch
+const ALLOWED_METHODS_BY_ADMIN_PATH: Record<string, string> = { + '/api/admin/whoami': 'GET, OPTIONS', + '/api/admin/users': 'GET, OPTIONS', + '/api/admin/users/delete': 'POST, OPTIONS', + '/api/admin/api-keys': 'GET, POST, OPTIONS', + '/api/admin/api-keys/revoke': 'POST, OPTIONS', + '/api/admin/status': 'GET, OPTIONS', +}; @@ const isKnownPath = KNOWN_ADMIN_PATHS.has(url.pathname); return Response.json( { error: isKnownPath ? 'Method not allowed' : 'Not found' }, - { status: isKnownPath ? 405 : 404, headers } + { + status: isKnownPath ? 405 : 404, + headers: isKnownPath + ? { ...headers, Allow: ALLOWED_METHODS_BY_ADMIN_PATH[url.pathname] ?? 'GET, POST, OPTIONS' } + : headers + } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/admin.ts` around lines 465 - 469, The 405 branch omits the required Allow header; update the response construction where isKnownPath is computed (KNOWN_ADMIN_PATHS.has(url.pathname)) and the subsequent Response.json call to add an "Allow" header listing permitted methods for that path when isKnownPath is true (e.g., derive allowed methods for the matched admin route and include headers['Allow'] or merge into the headers object passed to Response.json) so that Method Not Allowed responses include the Allow metadata.src/routes/auth.ts-967-980 (1)
967-980:⚠️ Potential issue | 🟡 MinorUse limiter reset time for
Retry-Afterin login 429s.Line 975 currently returns the full configured window, not the actual remaining cooldown from the limiter.
⏱️ Suggested fix
const rate = await checkRateLimit(req); if (!rate.allowed) { + const retryAfter = typeof rate.resetAt === 'number' && Number.isFinite(rate.resetAt) + ? Math.max(0, Math.ceil((rate.resetAt - Date.now()) / 1000)) + : Math.ceil(AUTH_CONFIG.RATE_LIMIT_WINDOW / 1000); return Response.json( { error: 'Too many login attempts. Please try again later.' }, { status: 429, headers: { ...baseHeaders, - 'Retry-After': Math.ceil(AUTH_CONFIG.RATE_LIMIT_WINDOW / 1000).toString(), + 'Retry-After': retryAfter.toString(), 'Set-Cookie': `session=; HttpOnly; Path=/; ${process.env.NODE_ENV === 'production' ? 'Secure; ' : ''}SameSite=Strict; Max-Age=0` } } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/auth.ts` around lines 967 - 980, The current 429 handler uses AUTH_CONFIG.RATE_LIMIT_WINDOW instead of the limiter's actual remaining cooldown; update the return to compute Retry-After from the limiter result returned by checkRateLimit (e.g., use rate.reset or rate.resetAt timestamp or a rate.remainingMs value if provided) by calculating Math.ceil(Math.max(0, (rate.reset - Date.now()) / 1000)) (or equivalent using resetAt/remainingMs), fall back to Math.ceil(AUTH_CONFIG.RATE_LIMIT_WINDOW / 1000) if the limiter result lacks that field, and use that computed value for the 'Retry-After' header while preserving baseHeaders and the session cookie logic.src/db/nip46.ts-431-433 (1)
431-433:⚠️ Potential issue | 🟡 MinorAvoid
any[]for SQL params in strict TypeScript.Line 432 uses an overly permissive
any[]type where an explicit union would be more accurate and type-safe.🧩 Suggested fix
- const params: any[] = [userId] + const params: Array<string | number | bigint> = [userId]Per coding guidelines (
**/*.{ts,tsx}), TypeScript strict mode requires explicit types and avoidingany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 431 - 433, Replace the permissive params: any[] with a precise union type for SQL parameters to satisfy strict TypeScript: change params to something like params: (string | number | boolean | null | Uint8Array)[] and update any downstream uses to match, keeping the initial assignment params = [userId] (where userId should be typed to one of those union members) and leave clauses: string[] unchanged; ensure functions that consume params (e.g., the SQL execution call) accept the new union type.src/server.ts-399-403 (1)
399-403:⚠️ Potential issue | 🟡 MinorRemove
anyusage in the prune timer block.Lines 400 and 402 use
anywhich violates the TypeScript strict mode requirement for explicit types.♻️ Suggested typing fix
runPrune() const interval = setInterval(runPrune, 6 * 60 * 60 * 1000) - ;(interval as any).unref?.() + const timer = interval as ReturnType<typeof setInterval> & { unref?: () => void } + timer.unref?.() } - } catch (e: any) { - console.error('⚠️ Failed to enable UI event log persistence:', e?.message || e) + } catch (e: unknown) { + const message = e instanceof Error ? e.message : String(e) + console.error('⚠️ Failed to enable UI event log persistence:', message) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 399 - 403, Replace the use of `any` when storing the interval and calling unref by giving `interval` a concrete timer type: declare `interval` as ReturnType<typeof setInterval> (or NodeJS.Timeout) when assigning setInterval in the prune timer block, then guard the unref call with a runtime check and cast to NodeJS.Timeout (e.g., if ('unref' in interval) (interval as NodeJS.Timeout).unref()). Update references to `interval` and keep the existing `runPrune` usage unchanged.docs/openapi/openapi.json-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorHealth-check cadence in
info.descriptionis stale.The description says checks run every 30 seconds, but runtime connectivity checks are every 60 seconds. This should be aligned to avoid operator confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` at line 5, Update the OpenAPI info.description to reflect the correct health-check cadence: locate the "info.description" string in openapi.json and change the "Health Checks: Automated health checks every 30 seconds" line to state "every 60 seconds" so the documentation matches the runtime connectivity checks.src/node/manager.ts-744-745 (1)
744-745:⚠️ Potential issue | 🟡 MinorClear cached probe result when canceling/superseding probes.
cancelBackgroundProbe()invalidates generations but keepslastBackgroundProbeResult, so callers can read stale probe data after cleanup or node recreation.🔧 Suggested fix
async function runBackgroundRelayProbe( relays: string[], kind: number = 20004, addServerLog?: ReturnType<typeof createAddServerLog> ): Promise<void> { if (relays.length === 0) { return; } const myGeneration = ++backgroundProbeGeneration; + lastBackgroundProbeResult = null; try { @@ export function cancelBackgroundProbe(): void { backgroundProbeGeneration++; + lastBackgroundProbeResult = null; }Also applies to: 806-808, 2450-2453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/manager.ts` around lines 744 - 745, When invalidating/superseding background probes you must also clear the cached probe result to avoid exposing stale data: in the places where you bump backgroundProbeGeneration and capture myGeneration (the line with "const myGeneration = ++backgroundProbeGeneration" and other similar spots) and inside cancelBackgroundProbe(), set lastBackgroundProbeResult = undefined (or null) immediately after incrementing/invalidating the generation so any in-flight or cancelled probes cannot be read from the stale cache; apply the same change to the other generation-invalidation sites that touch backgroundProbeGeneration or cancelBackgroundProbe().docs/openapi/openapi.yaml-2769-2772 (1)
2769-2772:⚠️ Potential issue | 🟡 MinorAdd explicit hex constraints for cryptographic key fields.
These fields are documented as fixed-size hex keys but currently accept arbitrary strings.
🧩 Suggested constraints
Nip46SessionInput: type: object description: Client-supplied fields when creating or updating a NIP‑46 session properties: pubkey: type: string + pattern: '^[0-9a-fA-F]{64}$' description: Client public key (hex encoded) @@ Nip46TransportInput: type: object properties: transport_sk: type: string + pattern: '^[0-9a-fA-F]{64}$' description: 32-byte transport secret key encoded as hexAlso applies to: 2905-2908
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.yaml` around lines 2769 - 2772, The OpenAPI schema for cryptographic key fields (e.g., the pubkey property) currently allows arbitrary strings—update the pubkey schema to enforce hex encoding and exact length by adding a pattern such as "^[0-9a-fA-F]{<HEX_LEN>}$" and corresponding minLength/maxLength set to <HEX_LEN> (replace <HEX_LEN> with the documented hex length), and similarly add the same pattern/length constraints to the other key fields noted (the block around lines 2905-2908); ensure the pattern, minLength and maxLength are present on each key property so the spec only accepts fixed-size hex keys.docs/openapi/openapi.yaml-1526-1531 (1)
1526-1531:⚠️ Potential issue | 🟡 MinorRequire both fields when
nextCursoris present.
nextCursorcurrently permits{}because the object branch has norequiredkeys. This weakens paging behavior for generated clients.📌 Suggested schema fix
nextCursor: oneOf: - type: object + required: [createdAt, id] properties: createdAt: { type: string } id: { type: string } - type: 'null'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.yaml` around lines 1526 - 1531, The oneOf branch for nextCursor allows an empty object because the object schema for the cursor has no required properties; update the object branch (the one under oneOf) to require both createdAt and id by adding a required array referencing "createdAt" and "id" so that when nextCursor is present it must include both fields (the symbols to edit are the oneOf entry for nextCursor and its object properties createdAt and id).frontend/types/nostr-connect.d.ts-2-6 (1)
2-6:⚠️ Potential issue | 🟡 MinorReplace loose
unknowntypes with precise interfaces matching the actual@cmdcode/nostr-connectAPI.The current types force downstream
as anycasts (seesrc/nip46/service.ts), violating strict mode. Per package documentation,InviteEncoder.decode()returns anInviteobject;SignerAgentconstructor accepts(signer: SignerDeviceAPI, options?: SignerAgentOptions); andSimpleSigneraccepts(seckey?: string | Uint8Array). Define or import these interfaces to eliminate the need for unsafe type casts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/types/nostr-connect.d.ts` around lines 2 - 6, Replace the loose unknowns in the exported types by importing or declaring the precise interfaces from the `@cmdcode/nostr-connect` API: change InviteEncoder.decode to return Invite (import or declare the Invite interface), type SignerAgent as new (signer: SignerDeviceAPI, options?: SignerAgentOptions) => unknown (or better, the concrete instance type) and type SimpleSigner as new (seckey?: string | Uint8Array) => unknown; ensure you import or declare SignerDeviceAPI and SignerAgentOptions (or the actual concrete return types) and update the exported constants InviteEncoder, SignerAgent, and SimpleSigner to use those interfaces so downstream code no longer needs as any casts.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
static/index.html (1)
2-13:⚠️ Potential issue | 🟠 MajorDo not modify generated assets under
static/directly.This file is under
static/**, so theclass="dark"change and the formatting-only change should be made in the source template/build step, then regenerated rather than committed here.As per coding guidelines,
static/**: "Never edit generated assets under static/ by hand" and "Do not commit generated assets under static/ to source control as changes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/index.html` around lines 2 - 13, The change to the generated asset (the HTML <html lang="en" class="dark"> and formatting in the static index) must not be edited directly; revert this file to the generated baseline and instead update the source template/build configuration that produces the <html ...> element (the template that controls the html tag and its class) so the class change and formatting are applied there, then run the build/regeneration step to produce a new static/index.html and commit only the source/template changes and regenerated artifact per the guideline.src/class/relay.ts (1)
233-252:⚠️ Potential issue | 🟠 MajorPrevent duplicate EVENT emissions when filters overlap.
Starting at Line 235, replay iterates each filter independently and sends on every match. If one cached event matches multiple filters, the client can receive duplicate
EVENTframes for the same subscription/event.Suggested fix (dedupe replayed events per subscription)
_onreq ( sub_id : string, filters : EventFilter[] ) : void { @@ - // For each filter: + const sentEventIds = new Set<string>() + // For each filter: for (const filter of filters) { // Set the limit count, if any. let limit_count = filter.limit + if (limit_count === 0) continue // For each event in the cache: for (const event of this.relay.cache) { - // If there is no limit, or we are above the limit: - if (limit_count === undefined || limit_count > 0) { - // If the event matches the current filter: - if (Nostr.match_filter(event, filter)) { - // Send the event to the client. - this.send(['EVENT', sub_id, event]) - this.log.client(`event matched in cache: ${event.id}`) - this.log.client(`event matched subscription: ${sub_id}`) - // Decrement only when we actually sent a matching event. - if (limit_count !== undefined) { - limit_count -= 1 - if (limit_count === 0) break - } - } - } + if (limit_count !== undefined && limit_count <= 0) break + // If the event matches the current filter: + if (Nostr.match_filter(event, filter) && !sentEventIds.has(event.id)) { + sentEventIds.add(event.id) + // Send the event to the client. + this.send(['EVENT', sub_id, event]) + this.log.client(`event matched in cache: ${event.id}`) + this.log.client(`event matched subscription: ${sub_id}`) + // Decrement only when we actually sent a matching event. + if (limit_count !== undefined) { + limit_count -= 1 + } + } } }Please also mirror this in live dispatch by short-circuiting after first match per subscription in
_onevent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/class/relay.ts` around lines 233 - 252, Replay currently iterates filters independently and can send the same cached event multiple times for sub_id; modify the replay logic in the function that calls this.addSub and iterates this.relay.cache to maintain a Set (e.g., sentEventIds) keyed by sub_id and before sending check if event.id is already in the Set, only send and decrement limit when you add it to the Set; mirror the same de-duplication in the live path inside _onevent by short-circuiting per subscription (or tracking sent IDs for that subscription) so the first Nostr.match_filter hit sends the EVENT and prevents subsequent sends of the same event for the same subscription.tests/routes/status-env.test.ts (1)
61-89:⚠️ Potential issue | 🟡 MinorThis no longer proves DB-mode unavailability.
With
AUTH_ENABLEDturned on at Line 66 and no auth on the request, the401can come entirely from the auth guard. That means this test will still pass even if/api/envbecomes reachable in database mode once authenticated. Please either authenticate the request and assert the DB-mode response, or rename the test to match the auth-only behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/status-env.test.ts` around lines 61 - 89, The test currently sets AUTH_ENABLED='true' which causes the 401 to possibly be from the auth guard rather than DB-mode; update the test that calls handleEnvRoute so it deterministically asserts DB-mode behavior: either disable auth (set process.env.AUTH_ENABLED = 'false') so the request reaches handleEnvRoute's DB-mode logic and assert the DB-specific 401/response, or keep AUTH_ENABLED='true' but add valid authentication (headers or whatever your auth flow expects in runRouteScript) and then assert the DB-mode error; reference handleEnvRoute, runRouteScript and the test name ("env route unavailable in database mode") when making the change.llm/workflows/RELEASE_PROCESS.md (1)
182-201:⚠️ Potential issue | 🟠 MajorKeep the release docs aligned with the repository’s actual default/release branch.
These changes now instruct maintainers to open, merge, tag from, and roll back
main, but this PR is still targetingmaster. Following the documented commands as written will point release work at the wrong branch.Also applies to: 328-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llm/workflows/RELEASE_PROCESS.md` around lines 182 - 201, The docs inconsistently reference the default release branch (mentions main in some places but the repo uses master), which will misdirect release commands; update the "Create a Pull Request" GH CLI example and all occurrences of "main" and "master" (e.g., the gh pr create line with --base main, the branch name "release/prepare-v<VERSION>", the PR merge instruction "When the PR is merged to main", and the later block around lines ~328-350) to use a single canonical placeholder like <DEFAULT_BRANCH> (or replace with the repository's actual default branch name) and ensure the CLI/example commands and prose consistently reference that placeholder so all commands target the correct branch.src/db/database.ts (1)
1153-1169:⚠️ Potential issue | 🔴 CriticalAdd migration to create role column for legacy databases and add defensive runtime check.
The
rolecolumn exists in the initialCREATE TABLE usersschema (line 115) for fresh installs, but:
No migration exists: The code comment at line 134 states "Legacy databases will be handled by dedicated migrations," but no such migration has been implemented. Legacy/upgraded databases will lack the
rolecolumn entirely.No defensive check: Unlike
peer_policies(which hasensurePeerPoliciesColumn()at line 137), there is no runtime fallback. Queries indeleteUserSafelywill fail withno such column: roleon upgraded installs.Risk:
deleteUserSafely(lines 1153–1169) and the admin count query (line 1169) will fail when executed against upgraded databases, blocking user deletion operations.Add a migration (
20250922_XXXX_add_role_column.sql) that creates and backfills therolecolumn for legacy installs, or add a defensive check analogous toensurePeerPoliciesColumn()before accessingrolein queries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 1153 - 1169, The deleteUserSafely flow fails on upgraded/legacy DBs because the users.role column may be missing; add a migration (e.g., 20250922_XXXX_add_role_column.sql) that ALTERs the users table to add a role column and backfills values, and/or add a defensive runtime check similar to ensurePeerPoliciesColumn() and call it before deleteUserSafely runs (so queries referencing role won’t throw "no such column: role"); update deleteUserSafely to rely on the ensured column or handle absence by treating missing role as non-admin in the admin-count logic (refer to function names: deleteUserSafely and ensurePeerPoliciesColumn and the users.role column).src/routes/env.ts (1)
728-754:⚠️ Potential issue | 🟠 MajorDeleting
RELAYSnever reapplies the running node configuration.This path now removes
process.env.RELAYS, but it only tears down/recreates the node when credentials are deleted. APOST /api/env/deletefor["RELAYS"]persists the new config while the live node keeps using the old relay set until some later restart.Suggested fix
- const deletingCredentials = validKeys.some(key => ['GROUP_CRED', 'SHARE_CRED'].includes(key)); + const deletingCredentials = validKeys.some(key => ['GROUP_CRED', 'SHARE_CRED'].includes(key)); + const deletingRelays = validKeys.includes('RELAYS'); @@ - if (deletingCredentials) { + if (deletingCredentials) { try { // Use synchronized cleanup to prevent race conditions await cleanupNodeSynchronized(context); } catch (error) { // Error already logged by executeUnderNodeLock // Continue anyway - the env vars were deleted } + } else if (deletingRelays) { + try { + await executeUnderNodeLock(async () => { + await createAndConnectServerNode(env, context); + }, context); + } catch (error) { + return Response.json({ success: false, message: 'Failed to reconfigure relays' }, { status: 500, headers }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/env.ts` around lines 728 - 754, The live node isn't reconfigured when RELAYS is deleted because deletingCredentials only checks GROUP_CRED/SHARE_CRED; update the logic so removing RELAYS also triggers the node cleanup: either include 'RELAYS' in the deletingCredentials determination or add a relaysChanged boolean and include it in the same conditional that calls await cleanupNodeSynchronized(context); also ensure process.env.RELAYS is deleted (the deletion loop in the validKeys handling or the try block that clears process.env values) happens before calling cleanupNodeSynchronized so the synchronized cleanup sees the updated environment (refer to deletingCredentials, validKeys loop, process.env.RELAYS, writeEnvFile, and cleanupNodeSynchronized).src/routes/nip46.ts (1)
93-121:⚠️ Potential issue | 🟡 MinorReject malformed policy patches instead of dropping them.
If
policy.methodsorpolicy.kindscontains bad keys/non-boolean values, this returnsnullandPOST /requestscan still succeed without applying the requested policy update. Reuse the stricterparseBooleanPolicyMap()path here so malformed patches fail closed with 400.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip46.ts` around lines 93 - 121, Update parsePolicyPatch to reuse the stricter parseBooleanPolicyMap for both the methods and kinds branches: when (value as any).methods or (value as any).kinds is present, call parseBooleanPolicyMap(...) to parse/validate it; if parseBooleanPolicyMap indicates the input is malformed (i.e. returns an error/null/invalid marker), have parsePolicyPatch fail closed by returning null so the request becomes a 400, and only set patch.methods/patch.kinds when parseBooleanPolicyMap returns a valid non-empty map. Keep references to parsePolicyPatch, parseBooleanPolicyMap and the properties methods/kinds so the change is localized.
♻️ Duplicate comments (19)
frontend/components/Configure.tsx (1)
625-633:⚠️ Potential issue | 🟠 MajorReject malformed headless relay input instead of persisting the default relay set.
This still silently falls back to the default relay list when
advancedSettings.RELAYSis present but invalid, and then persists that fallback in the headless save path. That can overwrite an intended relay configuration without any user confirmation.💡 Safer relay resolution
const resolveRelaysToSave = (): string[] => { + const sanitizedExisting = Array.isArray(existingRelays) + ? existingRelays.map((relay) => relay.trim()).filter((relay) => relay.length > 0) + : []; + if (!isHeadlessMode) { - if (Array.isArray(existingRelays) && existingRelays.length > 0) { - return existingRelays.map((relay) => relay.trim()).filter((relay) => relay.length > 0); - } + if (sanitizedExisting.length > 0) return sanitizedExisting; return ["wss://relay.primal.net"]; } if (typeof advancedSettings.RELAYS === 'string' && advancedSettings.RELAYS.trim().length > 0) { const parsedRelays = parseRelayList(advancedSettings.RELAYS); - if (parsedRelays) return parsedRelays; + if (!parsedRelays) { + throw new Error('RELAYS must be a non-empty JSON array of relay URLs.'); + } + return parsedRelays; } - if (Array.isArray(existingRelays) && existingRelays.length > 0) { - return existingRelays.map((relay) => relay.trim()).filter((relay) => relay.length > 0); - } + if (sanitizedExisting.length > 0) return sanitizedExisting; return ["wss://relay.primal.net"]; };Also applies to: 638-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 625 - 633, The current resolver silently falls back to defaults when advancedSettings.RELAYS is present but malformed; change the logic so that if typeof advancedSettings.RELAYS === 'string' and parseRelayList(advancedSettings.RELAYS) returns an invalid result, the function returns a clear failure value (e.g., null/undefined) or throws so the headless save path can detect and reject the input instead of persisting the default list; keep using parseRelayList for valid strings and only fall back to existingRelays/default when advancedSettings.RELAYS is completely absent, and apply the same fix to the similar block handling lines 638-650..github/workflows/ci.yml (1)
5-7:⚠️ Potential issue | 🔴 CriticalRestore CI coverage for
master.This PR targets
master, but Lines 5-7 only trigger onmainanddev. Pushes and pull requests againstmasterwill skip the workflow entirely.Suggested fix
on: push: - branches: [ main, dev ] + branches: [ main, master, dev ] pull_request: - branches: [ main, dev ] + branches: [ main, master, dev ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 5 - 7, The workflow triggers currently list only the branch arrays "branches: [ main, dev ]" for both push and "pull_request" so pushes and PRs against "master" are skipped; update both "branches" arrays under the push trigger and the "pull_request" trigger to include "master" (e.g., change to "branches: [ main, dev, master ]") so the CI runs for master as well.src/config/crypto.ts (1)
39-40:⚠️ Potential issue | 🟠 MajorExclude whitespace from the shared password regex.
Lines 39-40 still allow spaces/tabs to satisfy the “special character” requirement. That diverges from the onboarding/frontend rule, so a password can pass here and fail later in the auth flow.
Suggested fix
- PASSWORD_REGEX: /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9]).+$/, + PASSWORD_REGEX: /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9\s]).+$/,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/crypto.ts` around lines 39 - 40, The PASSWORD_REGEX currently allows whitespace to count as the "special character" and permits spaces overall; update the PASSWORD_REGEX constant so the special-character lookahead excludes whitespace and the whole pattern forbids any whitespace. Replace /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9]).+$/ with a pattern like ^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[^A-Za-z0-9\s])\S+$ in the PASSWORD_REGEX definition in src/config/crypto.ts so passwords cannot contain whitespace and whitespace cannot satisfy the special-character requirement.llm/implementation/umbrel-implementation.md (1)
72-76:⚠️ Potential issue | 🟡 MinorAdd the source-repo version bump to this checklist.
The checklist updates store-side metadata, but it still never reminds the releaser to bump
package.json(or this repo’s canonical version source) before publishing. That leaves the app version and the Umbrel store version free to drift again.Suggested fix
## Update Checklist for Future Releases -1. Build and push the new Umbrel image (`:umbrel-<version>` and `:umbrel-latest`). -2. Bump the digest in `igloo-server/docker-compose.yml` (keep the `:umbrel-dev` tag; only the `@sha256:...` digest changes). -3. Revise `igloo-server/umbrel-app.yml` version and release notes. -4. Refresh gallery assets if the UI has changed. +1. Bump this repo's application version (`package.json` or equivalent) and commit it. +2. Build and push the new Umbrel image (`:umbrel-<version>` and `:umbrel-latest`). +3. Bump the digest in `igloo-server/docker-compose.yml` (keep the `:umbrel-dev` tag; only the `@sha256:...` digest changes). +4. Revise `igloo-server/umbrel-app.yml` version and release notes. +5. Refresh gallery assets if the UI has changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llm/implementation/umbrel-implementation.md` around lines 72 - 76, Update the "Update Checklist for Future Releases" in llm/implementation/umbrel-implementation.md to include an explicit step to bump the repository's canonical version (e.g., update package.json version or whatever file holds the canonical version) before publishing; reference the package.json (or the repo's canonical version file) and instruct maintainers to update that version and commit the change alongside the Umbrel image/digest and release notes to prevent version drift.frontend/components/ui/log-entry.tsx (1)
112-118:⚠️ Potential issue | 🟡 MinorTreat a 200 blob response without
dataas a fetch failure.The promise still silently no-ops when the payload object lacks
data. The spinner clears,fetchErrorstaysnull, and the user is left with a preview/blank panel with no retry hint.Suggested fix
.then(payload => { if (cancelled) return; if (payload && typeof payload === 'object' && 'data' in payload) { setResolvedData((payload as any).data); setHasFetchedFull(true); + } else { + setFetchError('failed_to_load_payload'); } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/log-entry.tsx` around lines 112 - 118, The then-handler currently silently no-ops when a 200 response object lacks a data field; update the handler so that if payload is an object but does not contain 'data' you treat it as a fetch failure by calling the existing error state setter (e.g. setFetchError(new Error('Missing data in response'))), ensure any loading state is cleared (e.g. setHasFetchedFull(false) or the same logic you use for failures), and avoid leaving stale preview content (clear setResolvedData or setResolvedData(null) as appropriate). Locate this logic around the promise .then(...) where setResolvedData and setHasFetchedFull are called and add the error branch for payload objects without 'data'..github/workflows/release.yml (2)
14-14:⚠️ Potential issue | 🟠 MajorThis gate is still hard-coded to
main.The repository state in this PR is still
master-based, so a manual dispatch from the active release branch never enters the release job. Please switch this togithub.event.repository.default_branch(or allow both branch names) before shipping.Suggested fix
- if: github.ref == 'refs/heads/main' && github.event_name == 'workflow_dispatch' + if: github.ref == format('refs/heads/{0}', github.event.repository.default_branch) && github.event_name == 'workflow_dispatch'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 14, The CI gate currently hard-codes the branch check in the workflow condition (the expression using "github.ref == 'refs/heads/main'") which prevents runs on repositories where the default branch is "master"; update the conditional in the release job to reference github.event.repository.default_branch (e.g., compare github.ref to format('refs/heads/{0}', github.event.repository.default_branch)) or modify the expression to accept both 'refs/heads/main' and 'refs/heads/master' so manual dispatches on the repository's active branch trigger the release job.
119-123:⚠️ Potential issue | 🟠 MajorThe changelog insertion condition still misses dated headings.
The script creates headings as
## [x.y.z] - YYYY-MM-DD, but this block only inserts when the line equals## [x.y.z]. On a normal run the generated change list never gets inserted.Suggested fix
- $0 == target && !inserted { + index($0, target) == 1 && !inserted {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 119 - 123, The awk condition checks exact equality ($0 == target) so it misses headings with dates like "## [x.y.z] - YYYY-MM-DD"; update the insertion condition in the awk script that uses target and inserted (the block reading insert_file/temp_changelog) to match a heading prefix instead of exact equality — e.g., replace "$0 == target && !inserted" with a prefix or regex test such as "index($0, target) == 1 && !inserted" or "$0 ~ (\"^\" target) && !inserted" so lines like "## [${VERSION}] - YYYY-MM-DD" will be detected and the changelog inserted.src/routes/peers.ts (1)
859-933:⚠️ Potential issue | 🟠 MajorReturn 400 for invalid single-peer ping targets.
normalizePubkey(target)still sits inside the generictry, so malformed client input falls through to the catch path and comes back as HTTP 200 withsuccess: false. That should stay a bad-request response, like the per-policy route already does.Suggested fix
async function handlePingSinglePeer(target: string, context: RouteContext, headers: Record<string, string>): Promise<Response> { + const normalizedPubkey = safeNormalizePubkey(target); + if (!normalizedPubkey) { + return Response.json( + { error: 'Peer pubkey must be a 64-character hex string', pubkey: target, success: false }, + { status: 400, headers } + ); + } + try { // Ping specific peer - const normalizedPubkey = normalizePubkey(target); const startTime = Date.now();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/peers.ts` around lines 859 - 933, The function handlePingSinglePeer currently calls normalizePubkey(target) inside the try block so malformed input is handled as a runtime error and returns HTTP 200; move the pubkey validation out of the try: call normalizePubkey (or safeNormalizePubkey to validate) before the try and if the target is invalid return an immediate 400 Response.json with the same headers shape as other responses; then use the validated normalizedPubkey variable inside the try (and when updating context.peerStatuses and broadcasting) so input validation produces a 400 Bad Request instead of falling through to the catch path.frontend/components/ui/peer-list.tsx (1)
377-417:⚠️ Potential issue | 🟡 MinorType the peer-update event payload explicitly.
CustomEventwithout a detail generic still leavesevent.detailuntyped here, so this normalized update path loses type checking right where it now parses latency and dates.As per coding guidelines "TypeScript strict mode; explicit types, avoid `any`".Suggested fix
+interface PeerUpdateDetail { + pubkey: string; + status: { + online?: unknown; + latency?: unknown; + lastSeen?: unknown; + lastPingAttempt?: unknown; + }; +} + -const handlePeerUpdate = useCallback((event: CustomEvent) => { +const handlePeerUpdate = useCallback((event: CustomEvent<PeerUpdateDetail>) => { const { pubkey, status } = event.detail;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/peer-list.tsx` around lines 377 - 417, The handler uses an untyped CustomEvent so event.detail is implicitly any; explicitly type the event payload (e.g., CustomEvent<PeerUpdatePayload>) and/or reuse an existing type (e.g., PeerStatusUpdate or PeerStatus) so the detail shape (pubkey, status with latency, lastSeen, lastPingAttempt, online) is enforced; update the handlePeerUpdate signature to accept CustomEvent<YourPayloadType> and add/adjust the payload interface definition to match the parsed fields used in handlePeerUpdate (pubkey: string, status: { latency?: number | string | null; lastSeen?: string | null; lastPingAttempt?: string | null; online?: boolean }) so Number(status.latency) and parseDate calls are type-checked.llm/context/ENVIRONMENT_VARIABLES.md (1)
143-155:⚠️ Potential issue | 🟡 Minor
UI_EVENT_LOG_INCLUDE_PINGSis still missing from the env reference.This document now covers most runtime knobs, but operators still can't discover the toggle that controls whether ping events are recorded in the UI event log. If this is meant to be the definitive reference, that variable should be listed with its default and source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llm/context/ENVIRONMENT_VARIABLES.md` around lines 143 - 155, Add a missing table row for the environment variable UI_EVENT_LOG_INCLUDE_PINGS to ENVIRONMENT_VARIABLES.md: create an entry with Variable = `UI_EVENT_LOG_INCLUDE_PINGS`, Purpose = "Include ping events in UI event log", Both Modes Usage = "Identical behavior", Default = the actual default value pulled from the source code, Notes = "Controls whether ping events are recorded in the UI event log", and Source = the exact source file where the variable is defined/used (look up the definition/usage in the repo and reference that file). Ensure the row follows the same column order and style as the other entries in the Relay Probing & Startup Performance table.frontend/components/Signer.tsx (1)
540-545:⚠️ Potential issue | 🟠 MajorCap the merged history pages too.
The realtime append path enforces
MAX_EVENT_LOG_IN_MEMORY, but the merges at Line 544 and Line 582 return the fullmergedarray. Repeated “Load older” actions can therefore grow memory without bound and undo the cap added at Line 49. Apply the same trim after both merges.Also applies to: 579-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Signer.tsx` around lines 540 - 545, The merged results returned from setLogs (inside the realtime append and the "Load older" merge that combines chronological and prev) can exceed MAX_EVENT_LOG_IN_MEMORY; after you build merged (e.g., const merged = [...chronological.filter(...), ...prev]) trim it to the cap before returning (slice or similar) so the final array respects MAX_EVENT_LOG_IN_MEMORY. Do this in both places where merged is returned in the setLogs callbacks (referencing setLogs, chronological, prev, merged and MAX_EVENT_LOG_IN_MEMORY).src/server.ts (1)
658-662:⚠️ Potential issue | 🟠 MajorDefault query-string WebSocket auth to off.
Lines 660-662 still enable
?apiKey=/?sessionId=auth whenALLOW_QUERY_CREDENTIALSis unset. Query credentials leak too easily via browser history, reverse-proxy logs, and metrics, so this should require an explicit opt-in instead of being the compatibility default.🔐 Suggested hardening
const parsedAllowQueryCredentials = (process.env.ALLOW_QUERY_CREDENTIALS ?? '').trim().toLowerCase(); -// Backward-compatible default: allow legacy ?apiKey= / ?sessionId= WS auth unless explicitly disabled. -const WS_ALLOW_QUERY_CREDENTIALS = parsedAllowQueryCredentials === '' - ? true - : !['0', 'false', 'no', 'off'].includes(parsedAllowQueryCredentials); +// Secure default: only enable query-string credentials when explicitly opted in. +const WS_ALLOW_QUERY_CREDENTIALS = ['1', 'true', 'yes', 'on'].includes(parsedAllowQueryCredentials);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 658 - 662, The current logic treats an unset ALLOW_QUERY_CREDENTIALS as opt-in by setting WS_ALLOW_QUERY_CREDENTIALS true when parsedAllowQueryCredentials === ''; change it so the default is opt-out: make WS_ALLOW_QUERY_CREDENTIALS false when parsedAllowQueryCredentials is empty and only true when the env is explicitly set to an affirmative value; update the expression that sets WS_ALLOW_QUERY_CREDENTIALS (which references parsedAllowQueryCredentials and process.env.ALLOW_QUERY_CREDENTIALS) to return true only for explicit positives (e.g., '1','true','yes','on') and false otherwise.docs/AUTH_MATRIX.md (1)
17-17:⚠️ Potential issue | 🟡 MinorClarify
/api/updatebehavior in headless/managed modes.This row still reads as contradictory: the matrix says
/api/updateexists in headless mode, but the note says update checks are disabled whenHEADLESS=trueorSKIP_ADMIN_SECRET_VALIDATION=true. Please spell out whether the route remains mounted and returns a"disabled"response, or whether it is effectively unavailable in those modes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/AUTH_MATRIX.md` at line 17, Clarify the `/api/update` entry in AUTH_MATRIX.md to remove the contradiction: explicitly state whether the `/api/update` route remains mounted or is unmounted in headless/managed modes and what response it returns; update the table row and the explanatory note to say either "route remains mounted and returns { status: 'disabled' } (or HTTP 404/410 if unmounted)"—whichever is correct for the implementation—so consumers know if the endpoint exists and what response to expect, and reference the `/api/update` route name and the existing note text when making the change.tests/routes/event-log.db-mode.spec.ts (2)
26-26:⚠️ Potential issue | 🟡 MinorReplace
{} as anywith a typed route-context stub.
anyremoves the strict-mode guardrail here, so these tests will keep passing even ifhandleEventLogRoutestarts depending on additional context fields. Use the route context type, or a minimal typed stub that matches the handler signature.#!/bin/bash rg -n -C3 'handleEventLogRoute\s*\(' src/routes/event-log.ts rg -n -C3 'type .*Context|interface .*Context' srcAs per coding guidelines:
**/*.{ts,tsx}: Enable TypeScript strict mode, declare explicit types, and avoid any.Also applies to: 90-90, 141-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/event-log.db-mode.spec.ts` at line 26, Replace the untyped stub "const context = {} as any;" with a properly typed route-context stub that matches the handler signature used by handleEventLogRoute: locate the route context type or interface (search for types/interfaces named RouteContext, RequestContext, or similar in src, or inspect the handleEventLogRoute parameter type) and construct a minimal typed object providing only the fields the handler reads (e.g., user, params, logger, db) so the test fails if new context fields are required; update the tests at the referenced lines (26, 90, 141) to use that typed stub instead of any.
1-2:⚠️ Potential issue | 🟡 MinorCo-locate this spec with
src/routes/event-log.ts.Route specs are supposed to live beside the route they cover; keeping this under
tests/routesis still out of step with the repo convention.
As per coding guidelines:**/*.{test,spec}.ts: Co-locate backend tests with code and name them feature.test.ts or feature.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/event-log.db-mode.spec.ts` around lines 1 - 2, Move the spec file tests/routes/event-log.db-mode.spec.ts to live beside the implementation src/routes/event-log.ts (e.g., src/routes/event-log.spec.ts or event-log.db-mode.spec.ts) so the test is co-located with the route; after moving, update any imports inside the test (such as runRouteScript and PROJECT_ROOT) to correct relative paths from the new location and confirm the test file name follows the repo convention (feature.test.ts or feature.spec.ts).docs/RELEASE.md (1)
40-43:⚠️ Potential issue | 🟠 MajorKeep the release guide on
masteruntil the repo actually switches.PR
#35is still targetingmasteras of February 4, 2026, so these instructions would send release PRs, hotfixes, and the dev sync step to the wrong branch.Also applies to: 53-53, 69-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/RELEASE.md` around lines 40 - 43, The release guide currently instructs creating release PRs against `main` but the repo still uses `master` (PR `#35` targets `master`), so update all occurrences that instruct or show creating PRs, hotfixes, and the dev-sync step to target `master` until the repository actually switches default branches; specifically change the example "Create PR: `release/prepare-v1.1.1` → `main`" and the related sections referenced (lines around the shown diff and the other occurrences at 53 and 69-73) to use `master`, and add a short note that these will be updated once the repo officially switches to `main`.docs/openapi/openapi.json (2)
2463-2527:⚠️ Potential issue | 🟠 Major
openapi.jsonstill exposestransport_skin NIP-46 transport responses.Line 2475 and Line 2512 still model the response with secret-key naming, and Line 4263 keeps
Nip46Transportastransport_sk. This leaves the JSON spec contract stale and can break generated clients.🔧 Align JSON transport schemas with the current public contract
@@ "/api/nip46/transport" -> "get" -> "responses" -> "200" -> "content" -> "application/json" -> "schema" - "$ref": "#/components/schemas/Nip46Transport" + "$ref": "#/components/schemas/Nip46TransportPublic" @@ "/api/nip46/transport" -> "put" -> "requestBody" -> "content" -> "application/json" -> "schema" - "$ref": "#/components/schemas/Nip46Transport" + "$ref": "#/components/schemas/Nip46TransportInput" @@ "/api/nip46/transport" -> "put" -> "responses" -> "200" -> "content" -> "application/json" -> "schema" -> "properties" - "transport_sk": { + "transport_pubkey": { "type": "string" } @@ components.schemas + "Nip46TransportInput": { + "type": "object", + "properties": { + "transport_sk": { + "type": "string", + "description": "32-byte transport secret key encoded as hex" + } + }, + "required": ["transport_sk"] + }, + "Nip46TransportPublic": { + "type": "object", + "properties": { + "transport_pubkey": { + "type": "string", + "description": "Transport public key derived from the stored NIP-46 secret" + } + }, + "required": ["transport_pubkey"] + }, - "Nip46Transport": { - "type": "object", - "properties": { - "transport_sk": { - "type": "string", - "description": "32-byte transport secret key encoded as hex" - } - }, - "required": ["transport_sk"] - }Also applies to: 4260-4270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 2463 - 2527, The OpenAPI spec still exposes the secret-key field name transport_sk in the NIP‑46 endpoints and schema; update all occurrences so the public contract matches the current API: change response/request schemas for operationId getNip46Transport and updateNip46Transport and the components schema Nip46Transport to remove or rename transport_sk to the public field (e.g., transport_key) or otherwise match the non-secret public property the API returns, and ensure the 200 response object under updateNip46Transport likewise uses the corrected property name (and update any example/refs at lines referenced by Nip46Transport to keep the spec consistent).
164-235:⚠️ Potential issue | 🟡 Minor
/api/eventsstill documents conditional auth but is modeled as always authenticated.Line 168 says auth is conditional on
AUTH_ENABLED, but this operation has no localsecurityoverride, so it inherits global security and is documented as mandatory-auth for all environments.📘 Suggested OpenAPI adjustment
"/api/events": { "get": { + "security": [], + "x-public": true, "operationId": "eventsWebSocket",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 164 - 235, The OpenAPI entry for the GET operation with operationId "eventsWebSocket" currently inherits global security (making auth mandatory); add a local security override to the operation (e.g., add "security": [] directly under the GET object for "/api/events" / operationId "eventsWebSocket") so it does not inherit global required auth, and keep the existing description text explaining AUTH_ENABLED behavior.docs/openapi/openapi.yaml (1)
121-125:⚠️ Potential issue | 🟠 MajorRemove token-bearing subprotocol and query-param auth guidance from
/api/events.Line 121 and Line 125 still document sending credentials via
Sec-WebSocket-Protocolor URL query params. That conflicts with the security guidance at Line 2217/2233 and increases secret leakage risk.🔐 Suggested hardening
- - For non-browser clients, credentials may be provided via `Sec-WebSocket-Protocol` hints (first offered protocol is echoed back): - - `apikey.<TOKEN>` or `api-key.<TOKEN>` maps to `X-API-Key: <TOKEN>` - - `bearer.<TOKEN>` maps to `Authorization: Bearer <TOKEN>` - - `session.<ID>` maps to `X-Session-ID: <ID>` - - Legacy query params `apiKey` and `sessionId` are supported for compatibility but are discouraged. + - For non-browser clients, use standard authentication mechanisms (`Authorization`, `X-API-Key`, `X-Session-ID`, or secure session cookies). + - Do not place credentials in URL query parameters or token-bearing WebSocket subprotocol values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.yaml` around lines 121 - 125, Update the /api/events OpenAPI documentation to remove guidance that describes sending credentials via Sec-WebSocket-Protocol subprotocols (the "apikey.<TOKEN>", "api-key.<TOKEN>", "bearer.<TOKEN>", "session.<ID>" examples) and remove the note about legacy query params ("apiKey" and "sessionId"); edit the /api/events description block in openapi.yaml so it no longer instructs clients to provide secrets via Sec-WebSocket-Protocol or URL query parameters and instead leaves auth to the centralized security definitions referenced elsewhere in the spec.
🧹 Nitpick comments (13)
frontend/components/nip46/QRScanner.tsx (1)
31-89: Consider guarding against unmount during async initialization.If the component unmounts while
initializeScanner()is executing (between scanner creation at line 45 and ref assignment at line 65), the cleanup function won't destroy the scanner sincescannerRef.currentis still null at that point. This could leave the scanner running and the camera active.♻️ Proposed fix using a mounted flag
useEffect(() => { let scanner: QrScanner | null = null + let mounted = true const initializeScanner = async () => { if (!videoRef.current) { setError('Video element not found') return } try { if (!navigator.mediaDevices || !navigator.mediaDevices.getUserMedia) { throw new Error('Camera access is not supported in this browser') } scanner = new QrScanner( videoRef.current, (result: QrScanner.ScanResult) => { if (scanningRef.current) return if (result.data && result.data.startsWith('nostrconnect://')) { scanningRef.current = true onResultRef.current(result.data) scannerRef.current?.destroy() scannerRef.current = null timeoutRef.current = setTimeout(() => { scanningRef.current = false }, 1000) } }, { returnDetailedScanResult: true, highlightScanRegion: true, highlightCodeOutline: true } ) await scanner.start() + if (!mounted) { + scanner.destroy() + return + } setError(null) setHasPermission(true) scannerRef.current = scanner } catch (err) { const e = err instanceof Error ? err : new Error('Failed to initialize scanner') + if (!mounted) return setError(e.message) setHasPermission(false) if (onErrorRef.current) { onErrorRef.current(e); } else { console.error('Failed to start QR scanner:', e); } scanner?.destroy() scannerRef.current = null } } initializeScanner() return () => { + mounted = false if (timeoutRef.current) { clearTimeout(timeoutRef.current) } scannerRef.current?.stop() scannerRef.current?.destroy() scannerRef.current = null } }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/nip46/QRScanner.tsx` around lines 31 - 89, The initializeScanner async flow can create a local scanner before scannerRef.current is set, so if the component unmounts during initialization the cleanup won't destroy it; fix by introducing a mounted/aborted flag inside the useEffect (e.g., let mounted = true / const isMountedRef) and check it after awaiting operations and before assigning scannerRef.current or calling setState; also in the cleanup set mounted = false and explicitly destroy/stop the local scanner variable (the `scanner` declared in initializeScanner) if it exists to ensure the camera is released; update references in initializeScanner, scannerRef, timeoutRef, and the cleanup closure accordingly.src/routes/user.ts (1)
241-241: Consider adding explicit types for request body parsing.The coding guidelines specify to "avoid
any". While runtime validation is performed, consider defining an interface or inline type for the expected body shape to improve type safety and IDE support.interface CredentialsUpdateBody { group_cred?: string | null; share_cred?: string | null; relays?: string[] | null; group_name?: string | null; }As per coding guidelines, TypeScript strict mode requires explicit types and avoiding
any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/user.ts` at line 241, The variable "body" is currently typed as any (let body: any;) which violates the guideline to avoid any; define an explicit interface (e.g., CredentialsUpdateBody with group_cred, share_cred, relays, group_name) and replace the any with that type wherever the request body is parsed (use: let body: CredentialsUpdateBody or the appropriate handler parameter type), update any downstream usages to respect the typed properties, and if runtime parsing/validation is used keep that but narrow types by casting only after validation (or use a typed validation result) so the code no longer declares body as any.scripts/patch-zod-compat.mjs (1)
45-48: Make the schema rewrite less brittle to upstream changes.This still only patches one exact import spelling. A quote-style or whitespace change in
@cmdcode/nostr-connectwill silently leave the oldzodimport in place. A regex replacement plus an assertion that at least one file was rewritten would make this patch much safer across dependency updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/patch-zod-compat.mjs` around lines 45 - 48, The current replacement logic uses current.includes("import { z } from 'zod';") and replaceAll which is brittle; update the patch to use a regex that matches different quote styles and whitespace (e.g. a pattern matching import\s*\{\s*z\s*\}\s*from\s*['"]zod['"];), perform a replace with the target value (the existing target variable), write the updated content via writeFileSync(fullPath, updated) only when a change occurred, and add an assertion or counter (e.g. track filesModified) to ensure at least one file was rewritten and throw or exit non-zero if none were changed so the script fails loudly when the upstream import style differs; keep references to current.includes, replaceAll, writeFileSync, fullPath and target while replacing replaceAll usage with a regex-based replace and adding the post-run assertion.frontend/components/Recover.tsx (2)
418-441: Consider validating nsec format before storing.The validation checks for a non-empty string but doesn't verify the nsec format (e.g., should start with
nsec1). If the server returns malformed data, users would see it as a valid recovery.💡 Optional: Add format validation
- if (typeof result.nsec !== 'string' || result.nsec.length === 0) { + if (typeof result.nsec !== 'string' || result.nsec.length === 0 || !result.nsec.startsWith('nsec1')) { throw new Error('Recovery completed without an nsec payload'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Recover.tsx` around lines 418 - 441, The code accepts any non-empty string for result.nsec; validate the nsec format before calling setRecoveredNsec and updating state in the success branch of the Recover component: check result.nsec matches the expected prefix/format (e.g., startsWith('nsec1') or use a regex) and if it fails, throw an error or setResult to a failure message instead of storing it; update the logic around setRecoveredNsec and setResult where result.nsec is used so only properly formatted nsec values are accepted and malformed responses are handled gracefully.
458-467: Consider resetting copy status after a timeout.The
copyStatusremains as'copied'or'error'indefinitely after the clipboard operation. For better UX, reset to'idle'after a few seconds.♻️ Proposed fix: Auto-reset copy status
const handleCopyRecoveredNsec = async () => { if (!recoveredNsec) return; try { await navigator.clipboard.writeText(recoveredNsec); setCopyStatus('copied'); + setTimeout(() => setCopyStatus('idle'), 2000); } catch (error) { console.error('Failed to copy recovered NSEC:', error); setCopyStatus('error'); + setTimeout(() => setCopyStatus('idle'), 3000); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Recover.tsx` around lines 458 - 467, The handleCopyRecoveredNsec function sets copyStatus to 'copied' or 'error' but never resets it; update handleCopyRecoveredNsec to start a short timer (e.g., 2–3 seconds) after setting copyStatus that calls setCopyStatus('idle') to revert the UI, and ensure you clear any existing timer before starting a new one to avoid leaks—use a ref or component-level variable to store the timeout id and clearTimeout on subsequent calls and on unmount (cleanup in useEffect) so repeated copies or component teardown don't leave dangling timers.src/class/relay.test.ts (1)
85-104: Add an overlap-filter regression test for duplicate EVENT delivery.This multi-filter test is solid, but it doesn’t cover the case where one event matches both filters. Please add an assertion that only one
EVENTis emitted for the same event/subscription when filters overlap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/class/relay.test.ts` around lines 85 - 104, Add an assertion that overlapping filters don't cause duplicate EVENT delivery: after creating the subscription via relay.handler()/handler.message (the existing test uses NostrRelay, handler, ws and socket), construct an event that matches both filters (e.g., same kind and authorHex), send it into the relay via handler.message (JSON.stringify(['EVENT', event])) and then use decodeSent(socket) to assert there is exactly one ['EVENT', event] sent for 'sub-multi' (or check that the count of messages equal to that event/subscription pair is 1). Ensure you reference the existing symbols: NostrRelay, handler.message, relay.subs, sub.filters, socket and decodeSent.frontend/components/ui/collapsible.tsx (1)
69-69: Note:max-h-screenmay affect animation smoothness for tall content.The change from a fixed
max-h-[500px]tomax-h-screenprovides flexibility but may cause jarring transitions for content significantly shorter than viewport height. If consistent animation timing is important, consider using a CSS variable or JavaScript-based height calculation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/collapsible.tsx` at line 69, The change to use "max-h-screen" for the animated container (the class toggled by isExpanded in the Collapsible component) can cause jarring transitions for shorter content; revert to a predictable max-height by either switching to a CSS variable (e.g., replace "max-h-screen" with a utility that uses a CSS var like max-h-[var(--collapsible-max-h)] and set --collapsible-max-h in CSS or parent) or compute and apply an inline maxHeight in the Collapsible component (measure content in a ref in useEffect and set style.maxHeight when isExpanded) so animations remain smooth and consistent; update the class toggle and/or add the measurement logic around the element referenced by the isExpanded toggle to implement this.frontend/components/ui/log-entry.tsx (1)
12-16: Replace the newanypayload types withunknownor a small union.The lazy-load path adds more untyped surface (
dataPreviewandresolvedData), which weakens the guards below and works against strict-mode catches.As per coding guidelines, "TypeScript strict mode; explicit types, avoid any".
Also applies to: 79-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ui/log-entry.tsx` around lines 12 - 16, The property types using `any` should be replaced with safer types: change `dataPreview?: any` (and similar `any` uses around `resolvedData`) to `unknown` or a narrow union of expected shapes (e.g., string | Record<string, unknown> | Array<unknown>) and update the type guards that consume `dataPreview` and `resolvedData` to perform runtime checks (typeof, Array.isArray, and property checks) before treating them as specific types; ensure components/functions referencing `dataPreview` or `resolvedData` (e.g., rendering previews or calling into lazy-load logic) narrow the `unknown` via user-defined type predicates or inline checks so the code compiles under TS strict mode.src/db/database.ts (1)
391-404: Keepidx_sessions_last_accessusable during cleanup.Wrapping
last_accessinstrftime()prevents SQLite from using the index created on that column, so session cleanup degrades to a full scan as the table grows. Compare against a preformatted timestamp string instead of transforming the indexed column in the predicate.Suggested fix
const nowSeconds = Math.floor(Date.now() / 1000); const ttlSeconds = Math.max(1, Math.floor(ttlMs / 1000)); const cutoff = nowSeconds - ttlSeconds; + const cutoffIso = new Date(cutoff * 1000).toISOString().slice(0, 19).replace('T', ' '); @@ const rows = db .prepare( - `SELECT id FROM sessions WHERE CAST(strftime('%s', last_access) AS INTEGER) <= ?` + `SELECT id FROM sessions WHERE last_access <= ?` ) - .all(cutoff) as { id: string }[]; + .all(cutoffIso) as { id: string }[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 391 - 404, The SELECT uses CAST(strftime('%s', last_access) AS INTEGER) which prevents SQLite from using the idx_sessions_last_access index; change the predicate to compare the indexed last_access column directly to a preformatted timestamp string. Compute cutoffISO from cutoff (e.g., new Date(cutoff * 1000).toISOString()) and replace the query in the sessions cleanup block (the prepare/SELECT id FROM sessions ... statement) to "WHERE last_access <= ?" and pass cutoffISO as the bound, keeping the same batching logic (CHUNK, BEGIN IMMEDIATE, transaction) around the deletion loop.tests/routes/nip46.spec.ts (1)
34-76: Please add new NIP-46 route coverage next tosrc/routes/nip46.ts.This case is useful, but adding more route tests under
tests/routes/keeps drifting from the repo’s co-location rule. Prefer moving this spec tosrc/routes/nip46.spec.ts(or.test.ts) before growing it further.As per coding guidelines:
**/*.{test,spec}.ts: Co-locate backend tests with code and name them feature.test.ts or feature.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/nip46.spec.ts` around lines 34 - 76, Move the new NIP-46 route spec so it is co-located with the route implementation: create nip46.spec.ts (or nip46.test.ts) next to the nip46.ts implementation and remove the copy under tests/routes; update imports inside the spec that reference the project root or absolute paths so they import relative to the route module (preserve the use of handleNip46Route and any database setup/teardown code), and ensure runRouteScript or script bootstrap logic still resolves in the new location and tests still call handleNip46Route with the same context/auth parameters.src/routes/sign.ts (1)
6-15: Consider replacingany[]with a stricter type for tags.The
tagsfield usesany[]which conflicts with TypeScript strict mode guidelines. Since Nostr event tags are arrays of strings, consider usingstring[][]for better type safety.♻️ Proposed fix
type SignRequestBody = { message?: string; // 32-byte hex event id event?: Partial<EventTemplate> & { kind: number; created_at: number; content: string; - tags: any[]; + tags: string[][]; pubkey?: string; // Optional pubkey for event hashing }; };As per coding guidelines: "TypeScript strict mode; explicit types, avoid
any".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/sign.ts` around lines 6 - 15, The SignRequestBody type uses tags: any[] which breaks strict TypeScript rules; change the tags type to a nested string array (string[][]) to reflect Nostr event tags being arrays of strings and improve type safety—update the SignRequestBody definition (and any referenced Partial<EventTemplate> shape if it also declares tags) to use tags: string[][] and adjust any callers or tests that construct events to satisfy the new type.src/routes/admin.ts (1)
291-291:lastUsedIphardcoded tonullin API response.This appears intentional to avoid exposing client IP addresses in the admin listing. If this is a deliberate privacy decision, consider adding a brief comment to document the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/admin.ts` at line 291, The API response currently sets lastUsedIp: null (in the admin listing) to avoid exposing client IPs; add a one-line comment next to the lastUsedIp assignment (the property named lastUsedIp in src/routes/admin.ts) documenting this privacy decision and the intentional omission, or if this was accidental, replace the null with the correct source (e.g., pull from the session or user record) and log a brief note about consent/visibility; keep the comment concise and reference lastUsedIp so future reviewers understand the rationale.src/db/nip46.ts (1)
431-452: Keep the cursor params strongly typed.
paramsonly needsnumber | bigint | string, so widening it toany[]weakens this pagination path for no benefit.Based on learnings, "Applies to **/*.{ts,tsx} : TypeScript strict mode; explicit types, avoid `any`".♻️ Suggested typing
- const params: any[] = [userId] + const params: Array<number | bigint | string> = [userId]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 431 - 452, Change the untyped params array to a strongly typed array (e.g. (number|bigint|string)[]) instead of any[] in src/db/nip46.ts so the pagination/cursor path stays type-safe; update the declaration of params and ensure all pushes (params.push(...statuses), params.push(before.createdAt, before.createdAt, before.id)) and the final call stmt.all(...params, limit) are consistent with that type (casting inputs only if necessary) in the function that builds the WHERE clause and returns Nip46RequestRecord[].
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/components/Configure.tsx (2)
322-340:⚠️ Potential issue | 🟠 MajorStale closure:
isDirtyis captured at effect setup, not at focus time.The dirty check is computed when the effect runs, but
handleFocuscaptures that stale value. If the user edits the form after the effect setup, the focus handler will still use the oldisDirtyvalue.🐛 Proposed fix: compute dirty state inside the callback
useEffect(() => { if (!showAdvanced) return; - const isDirty = JSON.stringify(advancedSettings) !== JSON.stringify(originalAdvancedSettings); - const reloadAdvancedSettings = () => { - if (isDirty) return; - void loadAdvancedSettings(isHeadlessMode); - }; - const handleFocus = () => { reloadAdvancedSettings() }; - reloadAdvancedSettings(); + const reloadIfClean = () => { + const isDirty = JSON.stringify(advancedSettings) !== JSON.stringify(originalAdvancedSettings); + if (isDirty) return; + void loadAdvancedSettings(isHeadlessMode); + }; + const handleFocus = () => { reloadIfClean() }; + reloadIfClean(); window.addEventListener('focus', handleFocus); return () => { window.removeEventListener('focus', handleFocus);However, this still won't work because
advancedSettingsandoriginalAdvancedSettingswould be stale in the closure. A cleaner approach is to use refs to track the latest values:+ const advancedSettingsRef = useRef(advancedSettings); + const originalAdvancedSettingsRef = useRef(originalAdvancedSettings); + useEffect(() => { advancedSettingsRef.current = advancedSettings; }, [advancedSettings]); + useEffect(() => { originalAdvancedSettingsRef.current = originalAdvancedSettings; }, [originalAdvancedSettings]); + useEffect(() => { if (!showAdvanced) return; - const isDirty = JSON.stringify(advancedSettings) !== JSON.stringify(originalAdvancedSettings); - const reloadAdvancedSettings = () => { - if (isDirty) return; + const reloadIfClean = () => { + const isDirty = JSON.stringify(advancedSettingsRef.current) !== JSON.stringify(originalAdvancedSettingsRef.current); + if (isDirty) return; void loadAdvancedSettings(isHeadlessMode); }; - const handleFocus = () => { reloadAdvancedSettings() }; - reloadAdvancedSettings(); + const handleFocus = () => { reloadIfClean() }; + reloadIfClean(); window.addEventListener('focus', handleFocus); ... - }, [showAdvanced, isHeadlessMode, loadAdvancedSettings, advancedSettings, originalAdvancedSettings]); + }, [showAdvanced, isHeadlessMode, loadAdvancedSettings]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` around lines 322 - 340, The effect captures a stale isDirty value so handleFocus may use outdated state; fix by computing the dirty check at focus time (or by reading the latest values from refs) instead of using the isDirty variable captured at effect setup: update handleFocus/reloadAdvancedSettings to compute JSON.stringify(advancedSettingsRef.current) !== JSON.stringify(originalAdvancedSettingsRef.current) on invocation (maintain refs advancedSettingsRef and originalAdvancedSettingsRef that are kept in sync with advancedSettings and originalAdvancedSettings), and keep using loadAdvancedSettings and loadAdvancedSettingsRef for the actual reload logic.
306-306:⚠️ Potential issue | 🟡 MinorAvoid
anytype per coding guidelines.The mock array uses
any[]. Even for placeholder code, prefer explicit typing.💡 Proposed fix
- const shares: any[] = []; // Empty array for now, can be populated for testing + const shares: Array<{ name: string }> = []; // Empty array for now, can be populated for testingAs per coding guidelines: "Enable TypeScript strict mode, declare explicit types, and avoid any"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Configure.tsx` at line 306, The mock shares array uses the any[] type; replace it with an explicit, strict type and avoid any. Define or import the appropriate interface (e.g., Share or SharePlaceholder) and declare shares as Share[] (or Array<Share>), initializing it as an empty array (const shares: Share[] = []); if no real shape exists yet, create a minimal local interface (e.g., interface Share { id: string; name?: string } ) and use that instead of any to satisfy strict TypeScript rules in Configure.tsx.
♻️ Duplicate comments (6)
.github/workflows/ci.yml (1)
5-7:⚠️ Potential issue | 🟠 MajorCI triggers still exclude the active target branch (
master).This PR targets
master, but the workflow listens only onmain/dev. Pushes and PRs targetingmasterwill skip CI.🔧 Suggested fix
on: push: - branches: [ main, dev ] + branches: [ main, master, dev ] pull_request: - branches: [ main, dev ] + branches: [ main, master, dev ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 5 - 7, The workflow's on.branches lists only "main" and "dev", so CI won't run for pushes or PRs targeting "master"; update both branch arrays under the triggers "branches: [ main, dev ]" and "pull_request: branches: [ main, dev ]" to include "master" (e.g., branches: [ main, dev, master ]) so the CI runs for pushes and PRs against master as well..github/workflows/release.yml (2)
14-14:⚠️ Potential issue | 🟠 MajorRelease job can be skipped entirely if the active release branch is still
master.This gate only runs on
refs/heads/main. Manual releases frommasterwill skip the release and dependent docker jobs.🔧 Suggested fix to support both branches
- if: github.ref == 'refs/heads/main' && github.event_name == 'workflow_dispatch' + if: (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') && github.event_name == 'workflow_dispatch'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 14, The release job condition currently only matches "if: github.ref == 'refs/heads/main' && github.event_name == 'workflow_dispatch'", which causes manual releases from a 'master' branch to be skipped; update that conditional to allow both branches (e.g., check github.ref equals 'refs/heads/main' OR 'refs/heads/master' or otherwise match both refs) so the release job (and its dependent docker jobs) runs for manual dispatches from either branch.
119-126:⚠️ Potential issue | 🟠 MajorChangelog insertion target does not match dated headings.
The heading is created at line 95 as
## [version] - date, but the awk condition at line 121 uses exact equality$0 == targetagainst## [${VERSION}](without date). The insertion block will never be triggered.🔧 Suggested fix using prefix match
# Insert changes into changelog after the current version heading awk -v target="## [${VERSION}]" -v insert_file="$temp_changelog" ' { print } - $0 == target && !inserted { + index($0, target) == 1 && !inserted { while ((getline line < insert_file) > 0) print line close(insert_file) inserted = 1 } ' CHANGELOG.md > CHANGELOG.md.tmp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 119 - 126, The awk insertion never fires because the script compares the whole line to "## [${VERSION}]" (variable target) but the actual heading includes a date; update the awk condition in the block that builds CHANGELOG.md.tmp so it detects a heading prefix rather than exact equality — e.g., replace the equality check `$0 == target` with a starts-with or regex test (using index($0, target) == 1 or $0 ~ ("^" target)) while keeping the inserted flag and using insert_file (temp_changelog) and output file (CHANGELOG.md.tmp) unchanged.src/db/nip46.ts (1)
705-712:⚠️ Potential issue | 🟠 MajorVerify that session creation still emits a
createdevent.This limiter now counts only
nip46_session_events.event_type = 'created', but the creation flow visible in this file still logsupsert. If no route/service writes an immutablecreatedrecord on successful session creation, the rate limit becomes a permanent zero.Expected result: at least one session-creation path writes
logSessionEvent(..., 'created', ...)or insertsevent_type = 'created'intonip46_session_events.#!/bin/bash set -euo pipefail ast-grep --pattern "logSessionEvent($$$, 'created', $$$)" ast-grep --pattern 'logSessionEvent($$$, "created", $$$)' rg -n -C3 "nip46_session_events|event_type = 'created'|event_type = \"created\"" src tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 705 - 712, The rate limiter query in nip46_session_events filters only event_type = 'created' but the session creation path currently emits 'upsert' events (no immutable 'created' rows), so either ensure the creation flow writes a 'created' event or update the limiter to count the actual event used; locate the session creation/upsert flow (search for logSessionEvent or any upsertSession/createSession routine in this file) and either add a logSessionEvent(..., 'created', ...) call when a new session is successfully created or change the COUNT query on nip46_session_events to include event_type = 'upsert' (or both 'created' and 'upsert') so the limiter reflects real events.scripts/patch-zod-compat.mjs (2)
27-35:⚠️ Potential issue | 🟠 MajorValidate the actual Zod targets, not just the package directory.
Lines 32-35 assume
./v3/external.jsand./v3/external.cjsexist, but the guard on Lines 27-29 only checksnode_modules/zod. If the installed Zod layout changes, this still writes broken entrypoints and postinstall stays green until build/runtime. At minimum, fail fast on those hardcoded targets too.Suggested change
-if (!existsSync(ZOD_DIR)) { - console.error('[patch-zod-compat] zod directory not found') +if ( + !existsSync(ZOD_DIR) || + !existsSync(join(ZOD_DIR, 'v3', 'external.js')) || + !existsSync(join(ZOD_DIR, 'v3', 'external.cjs')) +) { + console.error('[patch-zod-compat] zod compatibility targets not found') process.exit(1) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/patch-zod-compat.mjs` around lines 27 - 35, The script currently only checks existence of ZOD_DIR before writing entry files but must also validate the specific target files it will overwrite; add existence checks for the concrete targets (e.g., the paths you will create via join(ZOD_DIR, 'index.js'), join(ZOD_DIR, 'index.cjs'), and join(ZOD_DIR, 'index.d.cts') or the upstream files it expects like './v3/external.js' and './v3/external.cjs') and abort with a clear error if any are missing instead of proceeding to call ensureFile; update the guard around ZOD_DIR to also verify those target files exist via existsSync and call process.exit(1) with an error message before calling ensureFile(…, esmStub/cjsStub/cjsDeclarationStub).
42-55:⚠️ Potential issue | 🟠 MajorFail only when schemas are neither patched nor already compatible.
Lines 43-50 only rewrite one exact import spelling. If
@cmdcode/nostr-connectchanges that string or ships an already-fixed variant, this block becomes a successful no-op with no signal either way. Track whether each file was already compatible or patched, and exit non-zero only when neither state is true.Suggested change
} else { try { - const schemaFiles = readdirSync(NOSTR_SCHEMA_DIR).filter(name => name.endsWith('.js')) - for (const file of schemaFiles) { - const fullPath = join(NOSTR_SCHEMA_DIR, file) - const current = readFileSync(fullPath, 'utf8') - const target = "import { zod as z } from '@vbyte/micro-lib/schema';" - if (current.includes("import { z } from 'zod';")) { - const updated = current.replaceAll("import { z } from 'zod';", target) - writeFileSync(fullPath, updated) + const schemaFiles = readdirSync(NOSTR_SCHEMA_DIR).filter(name => name.endsWith('.js')) + const target = "import { zod as z } from '@vbyte/micro-lib/schema';" + let compatibleFiles = 0 + + for (const file of schemaFiles) { + const fullPath = join(NOSTR_SCHEMA_DIR, file) + const current = readFileSync(fullPath, 'utf8') + + if (current.includes(target)) { + compatibleFiles += 1 + continue + } + + if (current.includes("import { z } from 'zod';")) { + const updated = current.replaceAll("import { z } from 'zod';", target) + writeFileSync(fullPath, updated) + compatibleFiles += 1 + } + } + + if (compatibleFiles === 0) { + console.error('[patch-zod-compat] No compatible nostr-connect schema imports found') + process.exit(1) } - } } catch (error) { console.error('[patch-zod-compat] Failed to patch nostr-connect schemas:', error) process.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/patch-zod-compat.mjs` around lines 42 - 55, The patch should detect per-file whether it was already compatible or got patched and only exit non-zero when a file is neither; inside the loop over schemaFiles (variables: schemaFiles, fullPath, current, target) check if current.includes(target) and mark as "compatible", else if current.includes("import { z } from 'zod';") perform the replace/write (as you already do) and mark as "patched", otherwise record an "unknown" file; after the loop, if any unknown files exist log an error and process.exit(1), otherwise proceed normally.
🧹 Nitpick comments (5)
src/nip46/index.ts (1)
13-19: KeepareInitOptionsEquivalentfrom drifting fromInitOptions.This works for the current three fields, but any future
InitOptionsaddition has to be remembered here too. Missing one would incorrectly reuse the existing singleton with stale config. Consider centralizing the compared fields so the type and the equivalence check stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nip46/index.ts` around lines 13 - 19, The equivalence check in areInitOptionsEquivalent will drift from the InitOptions type; centralize the compared fields by declaring a single source of truth (e.g., an exported constant array of keys typed as (keyof InitOptions)[]) and use that array in areInitOptionsEquivalent to iterate and compare each property, so future additions to InitOptions only require updating that one array (reference InitOptions and areInitOptionsEquivalent by name to locate the change)..github/workflows/ci.yml (1)
87-113: Robust retry logic for security audit.The 3-attempt retry with network-error detection is a good improvement for CI reliability. One minor observation: the
last_attemptvariable is set inside the loop but referenced after it exits.🔧 Minor fix for variable initialization
audit_log="$(mktemp)" last_exit=0 + last_attempt=0 for attempt in 1 2 3; do + last_attempt=$attempt if bun audit >"$audit_log" 2>&1; then cat "$audit_log" rm -f "$audit_log" exit 0 else audit_exit=$? last_exit=$audit_exit - last_attempt=$attempt if [ "$attempt" -lt 3 ]; then echo "bun audit failed (attempt $attempt), retrying..." sleep 5 fi fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 87 - 113, Initialize last_attempt before the retry loop so it is always defined when referenced after the loop; set last_attempt=0 (or a sensible default) alongside last_exit and audit_log initialization, keeping the retry loop (for attempt in 1 2 3) and subsequent logic that reads last_attempt, audit_output, and the network/registry grep checks unchanged.src/node/manager.ts (3)
1449-1454: Missing semicolon.Line 1450 is missing a semicolon after the function call. While JavaScript's ASI will handle this, explicit semicolons improve consistency.
♻️ Add semicolon
- const seq = opts?.persist?.(logEntry) + const seq = opts?.persist?.(logEntry);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/manager.ts` around lines 1449 - 1454, The try block that calls opts?.persist?.(logEntry) assigns the result to seq but is missing a trailing semicolon; update the call in the try block (where seq is declared and used to set logEntry.seq and logEntry.id) by adding a semicolon after the expression to maintain consistent statement termination for the seq assignment in manager.ts (look for the try block referencing opts?.persist?.(logEntry), seq, and logEntry).
733-737: Redundant else branch.Both branches assign the same value to
client._relays. Theifcheck is unnecessary.♻️ Suggested simplification
- if (Array.isArray(client._relays)) { - client._relays = [...filteredRelays]; - } else { - client._relays = [...filteredRelays]; - } + client._relays = [...filteredRelays];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/manager.ts` around lines 733 - 737, The if/else block redundantly assigns the same value to client._relays in both branches; remove the conditional and replace both branches with a single assignment: set client._relays = [...filteredRelays] (referencing client._relays and filteredRelays) so the code is simplified while preserving behavior.
690-693: Order-sensitive relay comparison may cause unnecessary updates.
sameRelayListcompares arrays element-by-element, so['a', 'b']and['b', 'a']are considered different. If relay order isn't semantically significant, this could cause unnecessary reconciliation or skip valid updates when order differs.Consider sorting both arrays before comparison if order doesn't matter:
♻️ Suggested fix for order-independent comparison
function sameRelayList(a: string[], b: string[]): boolean { if (a.length !== b.length) return false; - return a.every((relay, index) => relay === b[index]); + const sortedA = [...a].sort(); + const sortedB = [...b].sort(); + return sortedA.every((relay, index) => relay === sortedB[index]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/manager.ts` around lines 690 - 693, sameRelayList currently does an order-sensitive element-by-element comparison which flags ['a','b'] and ['b','a'] as different; update the sameRelayList function to perform an order-independent comparison (e.g., sort both arrays then compare lengths and elements, or compare as Sets) so relay lists with same members but different order are treated as equal; ensure you still return false for differing lengths and keep the function name sameRelayList for easy locating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/db/database.ts`:
- Around line 30-31: DB_DIR/DB_FILE initialization allows a bare relative
filename (e.g., "main" or "main.db") to produce path.dirname(envPath) === '.'
which can cause ensureDataDirSecure() to chmod the repository root; detect the
bare-file case (when isEnvPathFile is true but path.dirname(envPath) === '.' or
envPath contains no path separators) and handle it by either treating DB_DIR as
the defaultDbDir or setting a flag to skip directory hardening, then build
DB_FILE from that safe DB_DIR (or skip calling ensureDataDirSecure for the
bare-file case). Update the logic around DB_DIR, DB_FILE, and the call to
ensureDataDirSecure so ensureDataDirSecure is not invoked on '.'; reference
variables/functions: envPath, isEnvPathFile, DB_DIR, DB_FILE,
ensureDataDirSecure.
In `@src/db/migrations/20260306_0010_harden_nip46_relays_and_requests.sql`:
- Around line 30-31: The index idx_nip46_requests_dedupe on table nip46_requests
is non-unique so it doesn't prevent concurrent duplicate pending requests;
change it to a UNIQUE PARTIAL INDEX covering (user_id, session_pubkey,
client_request_id) with a WHERE clause restricting to pending rows (e.g. WHERE
status = 'pending') so only one pending row can exist, and update the nip46
bootstrap schema to create the identical unique partial index (same name and
predicate) so the runtime schema mirrors the migration.
In `@src/db/nip46.ts`:
- Around line 382-389: The insert currently calls normalizeClientRequestId with
null when params.payload is a string, causing client_request_id to be NULL;
update the logic around normalizeClientRequestId and payload handling in the
nip46 request creation so string payloads are parsed (JSON.parse) to extract the
id before calling normalizeClientRequestId, falling back to a reject/validation
error if parsing fails; ensure the same parsed object or original string is used
as the stored payload value in the INSERT into nip46_requests so
client_request_id reflects the request content for both object and string
payloads.
In `@src/nip46/index.ts`:
- Around line 23-29: The reinit path fires service.stop() but immediately
constructs a new Nip46Service, allowing old and new instances to overlap; change
this to await the stop before creating the new singleton: replace the
fire-and-forget void service.stop().catch(...) with an awaited stop call (try {
await service.stop() } catch (error) { console.warn(..., error) }), ensure the
enclosing function is async if it isn’t, and only assign service = new
Nip46Service(opts) and update serviceInitOptions after the awaited stop
completes so the old instance is fully torn down before publishing the new one;
keep the existing logging behavior on stop failure.
In `@tests/routes/database-path.spec.ts`:
- Around line 12-30: The temp directory created by fs.mkdtempSync (tmpDir) is
never removed; update the child script so that after calling
database.default.exec and awaiting database.closeDatabase() you remove tmpDir
(and its contents) — e.g., in the outer finally block call a synchronous or
awaited recursive removal on tmpDir (using fs.rmSync or fs.promises.rm with {
recursive: true, force: true }) before process.exit(0); ensure removal runs even
if closeDatabase throws by placing it in the outer finally and reference
tmpDir/dbPath and database.closeDatabase so you clean up the igloo-db-path-*
folder on every run.
In `@tests/routes/headless-optimizations.spec.ts`:
- Around line 7-15: The afterEach hook that deletes process.env keys (the
afterEach block removing NODE_ENV, HEADLESS, AUTH_ENABLED, API_KEY,
RATE_LIMIT_ENABLED, SESSION_SECRET, SKIP_STARTUP_ECHO) must be removed because
the tests mutate env only in the spawned child process; delete that hook so the
parent test runner environment isn't clobbered, or if you need a cleanup
placeholder replace it with a no-op that does not modify process.env in the
parent process. Ensure you edit the afterEach definition to either be removed
entirely or changed to an empty function so only the child process environment
is affected.
---
Outside diff comments:
In `@frontend/components/Configure.tsx`:
- Around line 322-340: The effect captures a stale isDirty value so handleFocus
may use outdated state; fix by computing the dirty check at focus time (or by
reading the latest values from refs) instead of using the isDirty variable
captured at effect setup: update handleFocus/reloadAdvancedSettings to compute
JSON.stringify(advancedSettingsRef.current) !==
JSON.stringify(originalAdvancedSettingsRef.current) on invocation (maintain refs
advancedSettingsRef and originalAdvancedSettingsRef that are kept in sync with
advancedSettings and originalAdvancedSettings), and keep using
loadAdvancedSettings and loadAdvancedSettingsRef for the actual reload logic.
- Line 306: The mock shares array uses the any[] type; replace it with an
explicit, strict type and avoid any. Define or import the appropriate interface
(e.g., Share or SharePlaceholder) and declare shares as Share[] (or
Array<Share>), initializing it as an empty array (const shares: Share[] = []);
if no real shape exists yet, create a minimal local interface (e.g., interface
Share { id: string; name?: string } ) and use that instead of any to satisfy
strict TypeScript rules in Configure.tsx.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 5-7: The workflow's on.branches lists only "main" and "dev", so CI
won't run for pushes or PRs targeting "master"; update both branch arrays under
the triggers "branches: [ main, dev ]" and "pull_request: branches: [ main, dev
]" to include "master" (e.g., branches: [ main, dev, master ]) so the CI runs
for pushes and PRs against master as well.
In @.github/workflows/release.yml:
- Line 14: The release job condition currently only matches "if: github.ref ==
'refs/heads/main' && github.event_name == 'workflow_dispatch'", which causes
manual releases from a 'master' branch to be skipped; update that conditional to
allow both branches (e.g., check github.ref equals 'refs/heads/main' OR
'refs/heads/master' or otherwise match both refs) so the release job (and its
dependent docker jobs) runs for manual dispatches from either branch.
- Around line 119-126: The awk insertion never fires because the script compares
the whole line to "## [${VERSION}]" (variable target) but the actual heading
includes a date; update the awk condition in the block that builds
CHANGELOG.md.tmp so it detects a heading prefix rather than exact equality —
e.g., replace the equality check `$0 == target` with a starts-with or regex test
(using index($0, target) == 1 or $0 ~ ("^" target)) while keeping the inserted
flag and using insert_file (temp_changelog) and output file (CHANGELOG.md.tmp)
unchanged.
In `@scripts/patch-zod-compat.mjs`:
- Around line 27-35: The script currently only checks existence of ZOD_DIR
before writing entry files but must also validate the specific target files it
will overwrite; add existence checks for the concrete targets (e.g., the paths
you will create via join(ZOD_DIR, 'index.js'), join(ZOD_DIR, 'index.cjs'), and
join(ZOD_DIR, 'index.d.cts') or the upstream files it expects like
'./v3/external.js' and './v3/external.cjs') and abort with a clear error if any
are missing instead of proceeding to call ensureFile; update the guard around
ZOD_DIR to also verify those target files exist via existsSync and call
process.exit(1) with an error message before calling ensureFile(…,
esmStub/cjsStub/cjsDeclarationStub).
- Around line 42-55: The patch should detect per-file whether it was already
compatible or got patched and only exit non-zero when a file is neither; inside
the loop over schemaFiles (variables: schemaFiles, fullPath, current, target)
check if current.includes(target) and mark as "compatible", else if
current.includes("import { z } from 'zod';") perform the replace/write (as you
already do) and mark as "patched", otherwise record an "unknown" file; after the
loop, if any unknown files exist log an error and process.exit(1), otherwise
proceed normally.
In `@src/db/nip46.ts`:
- Around line 705-712: The rate limiter query in nip46_session_events filters
only event_type = 'created' but the session creation path currently emits
'upsert' events (no immutable 'created' rows), so either ensure the creation
flow writes a 'created' event or update the limiter to count the actual event
used; locate the session creation/upsert flow (search for logSessionEvent or any
upsertSession/createSession routine in this file) and either add a
logSessionEvent(..., 'created', ...) call when a new session is successfully
created or change the COUNT query on nip46_session_events to include event_type
= 'upsert' (or both 'created' and 'upsert') so the limiter reflects real events.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 87-113: Initialize last_attempt before the retry loop so it is
always defined when referenced after the loop; set last_attempt=0 (or a sensible
default) alongside last_exit and audit_log initialization, keeping the retry
loop (for attempt in 1 2 3) and subsequent logic that reads last_attempt,
audit_output, and the network/registry grep checks unchanged.
In `@src/nip46/index.ts`:
- Around line 13-19: The equivalence check in areInitOptionsEquivalent will
drift from the InitOptions type; centralize the compared fields by declaring a
single source of truth (e.g., an exported constant array of keys typed as (keyof
InitOptions)[]) and use that array in areInitOptionsEquivalent to iterate and
compare each property, so future additions to InitOptions only require updating
that one array (reference InitOptions and areInitOptionsEquivalent by name to
locate the change).
In `@src/node/manager.ts`:
- Around line 1449-1454: The try block that calls opts?.persist?.(logEntry)
assigns the result to seq but is missing a trailing semicolon; update the call
in the try block (where seq is declared and used to set logEntry.seq and
logEntry.id) by adding a semicolon after the expression to maintain consistent
statement termination for the seq assignment in manager.ts (look for the try
block referencing opts?.persist?.(logEntry), seq, and logEntry).
- Around line 733-737: The if/else block redundantly assigns the same value to
client._relays in both branches; remove the conditional and replace both
branches with a single assignment: set client._relays = [...filteredRelays]
(referencing client._relays and filteredRelays) so the code is simplified while
preserving behavior.
- Around line 690-693: sameRelayList currently does an order-sensitive
element-by-element comparison which flags ['a','b'] and ['b','a'] as different;
update the sameRelayList function to perform an order-independent comparison
(e.g., sort both arrays then compare lengths and elements, or compare as Sets)
so relay lists with same members but different order are treated as equal;
ensure you still return false for differing lengths and keep the function name
sameRelayList for easy locating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27eb5ca7-5041-4a1f-89b1-b1c905fdd791
📒 Files selected for processing (23)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdfrontend/components/Configure.tsxfrontend/components/ui/button.tsxscripts/patch-zod-compat.mjssrc/db/database.tssrc/db/migrations/20260306_0010_harden_nip46_relays_and_requests.sqlsrc/db/nip46.tssrc/nip46/index.tssrc/node/manager.tssrc/routes/nip04.tssrc/routes/nip44.tssrc/routes/peers.tssrc/routes/utils.tssrc/utils/rate-limiter.tstests/routes/database-path.spec.tstests/routes/headless-optimizations.spec.tstests/routes/helpers/script-runner.spec.tstests/routes/helpers/script-runner.tstests/routes/nip46.spec.tstests/routes/rate-limiter.spec.tstests/routes/user-peers.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/routes/nip44.ts
- src/utils/rate-limiter.ts
- src/routes/utils.ts
- frontend/components/ui/button.tsx
- tests/routes/nip46.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/server.ts (1)
658-662:⚠️ Potential issue | 🟠 MajorDisable query-string WebSocket credentials by default.
Line 660 still enables
?apiKey=/?sessionId=auth whenALLOW_QUERY_CREDENTIALSis unset. That keeps credential material flowing through URLs, access logs, browser history, and intermediary caches unless operators explicitly turn it off. Default this tofalseand only opt in on an explicit allow value.🔐 Suggested hardening
const parsedAllowQueryCredentials = (process.env.ALLOW_QUERY_CREDENTIALS ?? '').trim().toLowerCase(); -// Backward-compatible default: allow legacy ?apiKey= / ?sessionId= WS auth unless explicitly disabled. -const WS_ALLOW_QUERY_CREDENTIALS = parsedAllowQueryCredentials === '' - ? true - : !['0', 'false', 'no', 'off'].includes(parsedAllowQueryCredentials); +const WS_ALLOW_QUERY_CREDENTIALS = ['1', 'true', 'yes', 'on'].includes(parsedAllowQueryCredentials);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 658 - 662, The current logic sets WS_ALLOW_QUERY_CREDENTIALS to true when process.env.ALLOW_QUERY_CREDENTIALS is unset, which keeps ?apiKey / ?sessionId auth enabled by default; change the default to false so query-string credentials are disabled unless explicitly allowed. Update the computation around parsedAllowQueryCredentials and WS_ALLOW_QUERY_CREDENTIALS so that an empty ALLOW_QUERY_CREDENTIALS yields false (not true), and only specific explicit truthy values (e.g., '1','true','yes','on') set WS_ALLOW_QUERY_CREDENTIALS to true; reference the existing symbols parsedAllowQueryCredentials, WS_ALLOW_QUERY_CREDENTIALS and process.env.ALLOW_QUERY_CREDENTIALS when making this change.
🧹 Nitpick comments (1)
src/db/nip46.ts (1)
417-423: Avoid the newanyescapes in this path.
payloadObject: Record<string, any>andconst params: any[]punch holes in the strict parsing/query code you just added.Record<string, unknown>plus a typed params tuple keeps this path type-safe without losing flexibility. As per coding guidelines, "TypeScript strict mode; explicit types, avoid any".Also applies to: 505-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 417 - 423, Change the loose any types to strict unknowns: replace the declaration payloadObject: Record<string, any> with payloadObject: Record<string, unknown> and update the params typing (where params is currently any[]) to a properly typed tuple or union that matches expected parameter shapes instead of any[]; ensure downstream uses of payloadObject and params do appropriate type-narrowing (e.g., typeof checks and Array.isArray) before accessing properties so the code compiles in strict mode and preserves the existing runtime behavior in the functions referencing params.payload and the later occurrence around lines 505-506.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/db/nip46.ts`:
- Around line 780-789: countUserSessionsInWindow() relies on the 'created' event
written to nip46_session_events but the POST /sessions handler calls
upsertSession() and writes that event afterwards while ignoring errors, allowing
quota bypass; fix by making the ledger write atomic with session creation: wrap
upsertSession() and the INSERT into nip46_session_events (the 'created' event)
in a single database transaction or change the POST /sessions handler to abort
and return an error if the ledger INSERT fails, ensuring the write to
nip46_session_events succeeds whenever upsertSession() succeeds.
In `@src/nip46/index.ts`:
- Around line 21-33: The initNip46Service function can race during
reinitialization because callers see the old service while one caller is
awaiting service.stop(); fix by serializing reinitialization: introduce a
module-level init promise or mutex (e.g., initPromise or initLock) and in
initNip46Service check and await that promise when present before proceeding,
set the promise immediately when starting the stop/create sequence, perform
await service.stop(), construct new Nip46Service(opts), assign service and
serviceInitOptions, then resolve/clear the init promise; reference symbols:
initNip46Service, service, serviceInitOptions, Nip46Service,
areInitOptionsEquivalent.
In `@src/server.ts`:
- Around line 225-232: The initNip46Service call is started before the database
schema/migrations finish, causing a race with initializeDatabase(); move the
initNip46Service invocation (the block using CONST.HEADLESS, addServerLog,
broadcastEvent, getNode and calling initNip46Service) so it runs only after
await initializeDatabase() has completed successfully (i.e., relocate both
occurrences to the code path immediately after initializeDatabase() resolves and
only start the service then, rather than fire-and-forget before DB init).
In `@tests/routes/headless-optimizations.spec.ts`:
- Around line 151-174: The test never triggers the startup path that calls
initNip46Service, so change the test to import the actual bootstrap/startup
module that runs the server initialization (the module that contains the if
(!HEADLESS) gating logic in server.ts) instead of directly importing
src/nip46/index.ts; set process.env.HEADLESS appropriately before that import,
call getNip46Service() (from the nip46 module) after the bootstrap import, and
assert null when HEADLESS='true' and non-null when HEADLESS='false' to ensure
the HEADLESS gate is exercised and initNip46Service() is invoked (or skipped) as
intended.
In `@tests/routes/nip46.spec.ts`:
- Around line 250-253: The test currently only checks that an index named
'idx_nip46_requests_pending_dedupe' exists and is unique; update the assertion
to verify the index is a partial (predicate) index for pending deduplication by
asserting the PRAGMA index_list entry has partial === 1, or alternatively read
sqlite_master.sql for the index definition and assert it contains "WHERE status
= 'pending' AND client_request_id IS NOT NULL"; change the hasPendingDedupe
check to inspect the index object returned by database.default.prepare("PRAGMA
index_list(nip46_requests)").all() for partial === 1 (or validate
sqlite_master.sql.sql) so the test confirms the index predicate, not just its
name or uniqueness.
---
Duplicate comments:
In `@src/server.ts`:
- Around line 658-662: The current logic sets WS_ALLOW_QUERY_CREDENTIALS to true
when process.env.ALLOW_QUERY_CREDENTIALS is unset, which keeps ?apiKey /
?sessionId auth enabled by default; change the default to false so query-string
credentials are disabled unless explicitly allowed. Update the computation
around parsedAllowQueryCredentials and WS_ALLOW_QUERY_CREDENTIALS so that an
empty ALLOW_QUERY_CREDENTIALS yields false (not true), and only specific
explicit truthy values (e.g., '1','true','yes','on') set
WS_ALLOW_QUERY_CREDENTIALS to true; reference the existing symbols
parsedAllowQueryCredentials, WS_ALLOW_QUERY_CREDENTIALS and
process.env.ALLOW_QUERY_CREDENTIALS when making this change.
---
Nitpick comments:
In `@src/db/nip46.ts`:
- Around line 417-423: Change the loose any types to strict unknowns: replace
the declaration payloadObject: Record<string, any> with payloadObject:
Record<string, unknown> and update the params typing (where params is currently
any[]) to a properly typed tuple or union that matches expected parameter shapes
instead of any[]; ensure downstream uses of payloadObject and params do
appropriate type-narrowing (e.g., typeof checks and Array.isArray) before
accessing properties so the code compiles in strict mode and preserves the
existing runtime behavior in the functions referencing params.payload and the
later occurrence around lines 505-506.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 663f0cc1-98d0-4bcc-96b5-b354e7b0e7c6
📒 Files selected for processing (8)
src/db/database.tssrc/db/migrations/20260306_0010_harden_nip46_relays_and_requests.sqlsrc/db/nip46.tssrc/nip46/index.tssrc/server.tstests/routes/database-path.spec.tstests/routes/headless-optimizations.spec.tstests/routes/nip46.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
docs/openapi/openapi.json (1)
2463-2527:⚠️ Potential issue | 🟠 MajorPublish
transport_pubkeyin the bundled JSON spec.
docs/openapi/openapi.yamlnow models/api/nip46/transportas returning a public key, but this JSON bundle still exposestransport_skon the GET and PUT responses. That republishes the wrong contract and will mislead generated clients.Also applies to: 4260-4270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 2463 - 2527, The JSON OpenAPI bundle exposes the private field "transport_sk" in the GET (operationId getNip46Transport) and PUT (operationId updateNip46Transport) responses but the YAML model returns the public key; update the response schemas for both operations to publish "transport_pubkey" (string) instead of "transport_sk" so the bundled spec matches docs/openapi/openapi.yaml and generated clients get the correct contract.docs/openapi/openapi.yaml (1)
119-125:⚠️ Potential issue | 🟠 MajorRemove the credential-in-URL / echoed-subprotocol guidance from
/api/events.This still tells clients to send auth material in
Sec-WebSocket-Protocolvalues and legacy query params. That conflicts with the later “never include ... in URL query parameters” guidance and makes credential leakage through logs/intermediaries much more likely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.yaml` around lines 119 - 125, Update the Authentication section for the /api/events endpoint to remove any instructions that tell clients to place credentials in the URL query params or in echoed Sec-WebSocket-Protocol subprotocol hints (the bullet points mapping `apikey.<TOKEN>`, `bearer.<TOKEN>`, `session.<ID>` and the legacy `apiKey`/`sessionId` note). Instead, state that /api/events uses the same auth mechanisms as normal HTTP requests (API key header, Authorization: Bearer, Basic, or session cookie/header) and explicitly forbid sending secrets in URL query strings or in Sec-WebSocket-Protocol values to avoid credential leakage; edit the "Authentication" block in openapi.yaml for /api/events accordingly.src/routes/nip46.ts (1)
17-27:⚠️ Potential issue | 🟠 MajorThe size guard still buffers the whole request body.
Line 25 still calls
await req.text()before the limit is enforced, so requests without a trustworthyContent-Lengthcan still force full buffering in memory. The limit needs to be enforced while reading, or the route should reject bodies whose size cannot be bounded up front.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/nip46.ts` around lines 17 - 27, parseJsonWithLimit currently calls await req.text() which buffers the entire body before enforcing maxBytes; change it to enforce the limit while streaming by reading from the request body reader (use req.body / req.body.getReader()) and accumulate chunks, tracking byte length and returning null (or throwing) as soon as accumulated bytes exceed maxBytes; alternatively, if you prefer to reject unbounded bodies, make parseJsonWithLimit return null immediately when content-length header is missing or not a safe number. Update the parseJsonWithLimit function and references to req.text() accordingly so the size check happens during streaming rather than after full buffering.src/db/nip46.ts (1)
156-198:⚠️ Potential issue | 🟠 MajorDon't continue after a failed
nip46_requestsschema repair.The new
client_request_idcolumn/index setup is required bycreateNip46Request()andgetPendingNip46RequestByClientId(), but any failure in this block is swallowed by the catch below. That letsinitializeNip46DB()resolve with a partially upgraded schema and pushes the failure into request handling. Please fail initialization when thenip46_requestsrepair path cannot complete.Also applies to: 199-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/nip46.ts` around lines 156 - 198, The nip46_requests migration block currently swallows errors and allows initializeNip46DB() to succeed with a partially-upgraded schema; change the flow so any exception during the nip46_requests repair (the code that adds client_request_id, updates values, deletes duplicates, drops/creates indexes using db.exec and hasTableColumn/hasIndex checks) is not absorbed by the outer catch: either remove the local try/catch around this branch or rethrow the caught error (throw err) so initializeNip46DB() fails; apply the same behavior to the adjacent repair block referenced (lines ~199-202) so any failure aborts initialization instead of letting the app continue with an inconsistent schema.
🧹 Nitpick comments (5)
src/node/manager.ts (1)
2190-2239: Consider extracting SimplePool patching to a helper.The inline patching and restoration of
SimplePool.prototype.subscribeManyworks correctly but is deeply nested. Extracting this to a dedicated helper (e.g.,withNormalizedSubscribeMany) would improve readability and testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/manager.ts` around lines 2190 - 2239, The block that patches and restores SimplePool.prototype.subscribeMany inside withSimplePoolSubscribeManyLock is too nested; extract it into a helper (suggested name: withNormalizedSubscribeMany) that accepts a callback and performs the try/patch/restore logic around SimplePool.prototype.subscribeMany (creating normalizedSubscribeMany, capturing originalSubscribeMany and restoreSubscribeMany, and logging patchError via addServerLog), then call that helper to run createConnectedNode; update usages to call withSimplePoolSubscribeManyLock(() => withNormalizedSubscribeMany(() => createConnectedNode(...))) or equivalent to keep the existing locking and error-handling semantics while improving readability and testability.src/routes/update.ts (1)
55-59: Consider reusing the centralized boolean parser fromsrc/const.ts.The
parseBooleanhelper duplicates logic already present insrc/const.ts(used forHEADLESS,SKIP_ADMIN_SECRET_VALIDATION). Extracting and reusing that pattern would reduce duplication and ensure consistent parsing across the codebase.+import { parseEnvBoolean } from '../const.js'; + // Remove local parseBoolean, use imported helper instead -function parseBoolean(value?: string): boolean { - if (!value) return false; - const trimmed = value.trim().toLowerCase(); - return trimmed === 'true' || trimmed === '1' || trimmed === 'yes'; -}Note: This requires exporting a helper from
const.tsor creating a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/update.ts` around lines 55 - 59, The local parseBoolean in update.ts duplicates the boolean parsing logic already used elsewhere; replace it by importing and using the centralized boolean parser from src/const.ts (or move the parser into a shared utility and export it from const.ts) instead of defining parseBoolean in update.ts; update any references inside the update handler to call the exported helper (e.g., boolean parser name from const.ts) and remove the local parseBoolean function to avoid duplication and ensure consistent parsing across HEADLESS/SKIP_ADMIN_SECRET_VALIDATION and routes/update logic.tests/routes/update.spec.ts (2)
5-6: Derive the expected version instead of pinning1.2.0.This assertion will need a manual edit on every release even though the route reads
package.jsondynamically and the release tooling only syncs package/OpenAPI metadata.♻️ Suggested tweak
import { describe, expect, test } from 'bun:test'; import { pathToFileURL } from 'url'; import { runRouteScript } from './helpers/script-runner'; const PROJECT_ROOT = pathToFileURL(process.cwd() + '/').href; +const { version: PACKAGE_VERSION } = + JSON.parse(Bun.file('package.json').textSync()) as { version: string }; describe('/api/update route', () => { @@ - expect(result.currentVersion).toBe('1.2.0'); + expect(result.currentVersion).toBe(PACKAGE_VERSION);Also applies to: 28-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/update.spec.ts` around lines 5 - 6, The test currently pins the expected version (e.g., "1.2.0") which requires manual updates; change the test in tests/routes/update.spec.ts to derive the expected version from the package.json that the route reads instead of hardcoding it: read/require the repository package.json (using PROJECT_ROOT to locate it) to get packageJson.version and use that value in the assertions that currently compare against the pinned string (the assertions around lines referencing 28-31), so the test uses the live package version for expectedVersion.
1-56: Please co-locate this route spec withsrc/routes/update.ts.Keeping the test beside the route makes version/update behavior easier to discover with the code it validates.
As per coding guidelines, "Co-locate backend tests with code and name them feature.test.ts or feature.spec.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/update.spec.ts` around lines 1 - 56, Move the test file tests/routes/update.spec.ts next to the implementation src/routes/update.ts (e.g., src/routes/update.spec.ts or src/routes/update.test.ts) so the route and its spec are co-located; rename the file to follow the project's convention (feature.spec.ts or feature.test.ts) and update any import paths or test runner helpers (runRouteScript usage) that assume its former location to use the new relative path alongside handleUpdateRoute in src/routes/update.ts.tests/routes/headless-optimizations.spec.ts (1)
1-265: Please split/co-locate these runtime specs with the modules they exercise.This file mixes
src/const.ts,src/routes/auth.ts,src/nip46/index.ts, andsrc/server.tsbehavior undertests/routes/, which makes ownership and failures harder to track.Based on learnings, "Prefer unit tests co-located with their subjects; name *.test.ts or *.spec.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/headless-optimizations.spec.ts` around lines 1 - 265, The test file groups runtime specs for multiple modules making ownership unclear; split each describe block into co-located unit/integration tests next to the module they exercise: move SKIP_STARTUP_ECHO tests to a new const.test.ts beside src/const.ts, move session/auth tests (referencing AUTH_CONFIG and stopAuthCleanup) to tests next to src/routes/auth.ts, and move NIP-46 tests (using initNip46Service and getNip46Service and server bootstrap behavior) to tests next to src/nip46/index.ts (and a short bootstrap.integration.test.ts near src/server.ts if needed); rename files to *.test.ts or *.spec.ts, adjust imports to use relative paths and retain runRouteScript setup or equivalent per-module helpers so each test file only references its target module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 3-7: The workflow currently triggers on every push to master (the
on: push branches: - master block) which allows the tag-creation and
release-action steps to overwrite an existing release; change the trigger to
only run when package.json (or other versioned files) change (e.g., on: push
paths: [package.json] for master) OR add an early job step that checks diff of
package.json between HEAD and previous commit (using actions/checkout + git diff
HEAD^..HEAD -- package.json) and exits the job if there is no change; ensure
this check runs before the existing tag creation step and before the release
action so builds and releases are skipped for commits that don’t change the
version.
- Around line 132-139: The Docker job's job-level permissions only specify
"packages: write", which prevents actions/checkout@v4 from fetching the tag at
ref: refs/tags/${{ needs.release.outputs.new_version }}; update the job's
permissions block (the permissions: entry) to include contents: read alongside
packages: write so actions/checkout@v4 can read the repository contents and
checkout the tag.
In `@scripts/release.sh`:
- Around line 31-35: The release script currently runs only the unit tests via
"bun run test:unit", which lets route/OpenAPI regressions slip; update
scripts/release.sh to invoke the full test suite by running "bun test" (or add
"bun test" as an additional step after "bun run test:unit") so the script gates
release prep on the complete test suite, and ensure the command is executed
normally so nonzero exit codes cause the script to fail.
In `@src/server.ts`:
- Around line 839-847: The code in the WebSocket upgrade handling that sets
selectedSubprotocol from proto/offered (variables: proto, offered,
selectedSubprotocol, authReq) must not blindly echo credential-bearing tokens
like "apikey.", "bearer.", or "session." back in the Sec-WebSocket-Protocol
handshake; instead filter offered before choosing a subprotocol by first
removing any token that matches those credential prefixes and then selecting
only from a safe allowlist (or validated pattern) of legitimate subprotocol
names; apply the same filtering logic to the relay upgrade branch that also
reads proto and sets selectedSubprotocol so credential hints are never echoed in
ws.protocol, proxy or access logs.
In `@tests/routes/headless-optimizations.spec.ts`:
- Around line 184-190: The test creates the bootstrap promise (const bootstrap =
import(root + 'src/server.ts')) but never awaits it, so startup errors can occur
after the sleep; change the test to await the bootstrap promise (or replace the
sleep with a readiness poll) before importing getNip46Service and continuing,
ensuring any thrown error surfaces and fails the test; also move this test file
to be co-located with the code under test (e.g., into src/ as server.test.ts or
follow the co-location pattern used by src/routes/utils.test.ts) so it runs
alongside the modules it exercises.
---
Duplicate comments:
In `@docs/openapi/openapi.json`:
- Around line 2463-2527: The JSON OpenAPI bundle exposes the private field
"transport_sk" in the GET (operationId getNip46Transport) and PUT (operationId
updateNip46Transport) responses but the YAML model returns the public key;
update the response schemas for both operations to publish "transport_pubkey"
(string) instead of "transport_sk" so the bundled spec matches
docs/openapi/openapi.yaml and generated clients get the correct contract.
In `@docs/openapi/openapi.yaml`:
- Around line 119-125: Update the Authentication section for the /api/events
endpoint to remove any instructions that tell clients to place credentials in
the URL query params or in echoed Sec-WebSocket-Protocol subprotocol hints (the
bullet points mapping `apikey.<TOKEN>`, `bearer.<TOKEN>`, `session.<ID>` and the
legacy `apiKey`/`sessionId` note). Instead, state that /api/events uses the same
auth mechanisms as normal HTTP requests (API key header, Authorization: Bearer,
Basic, or session cookie/header) and explicitly forbid sending secrets in URL
query strings or in Sec-WebSocket-Protocol values to avoid credential leakage;
edit the "Authentication" block in openapi.yaml for /api/events accordingly.
In `@src/db/nip46.ts`:
- Around line 156-198: The nip46_requests migration block currently swallows
errors and allows initializeNip46DB() to succeed with a partially-upgraded
schema; change the flow so any exception during the nip46_requests repair (the
code that adds client_request_id, updates values, deletes duplicates,
drops/creates indexes using db.exec and hasTableColumn/hasIndex checks) is not
absorbed by the outer catch: either remove the local try/catch around this
branch or rethrow the caught error (throw err) so initializeNip46DB() fails;
apply the same behavior to the adjacent repair block referenced (lines ~199-202)
so any failure aborts initialization instead of letting the app continue with an
inconsistent schema.
In `@src/routes/nip46.ts`:
- Around line 17-27: parseJsonWithLimit currently calls await req.text() which
buffers the entire body before enforcing maxBytes; change it to enforce the
limit while streaming by reading from the request body reader (use req.body /
req.body.getReader()) and accumulate chunks, tracking byte length and returning
null (or throwing) as soon as accumulated bytes exceed maxBytes; alternatively,
if you prefer to reject unbounded bodies, make parseJsonWithLimit return null
immediately when content-length header is missing or not a safe number. Update
the parseJsonWithLimit function and references to req.text() accordingly so the
size check happens during streaming rather than after full buffering.
---
Nitpick comments:
In `@src/node/manager.ts`:
- Around line 2190-2239: The block that patches and restores
SimplePool.prototype.subscribeMany inside withSimplePoolSubscribeManyLock is too
nested; extract it into a helper (suggested name: withNormalizedSubscribeMany)
that accepts a callback and performs the try/patch/restore logic around
SimplePool.prototype.subscribeMany (creating normalizedSubscribeMany, capturing
originalSubscribeMany and restoreSubscribeMany, and logging patchError via
addServerLog), then call that helper to run createConnectedNode; update usages
to call withSimplePoolSubscribeManyLock(() => withNormalizedSubscribeMany(() =>
createConnectedNode(...))) or equivalent to keep the existing locking and
error-handling semantics while improving readability and testability.
In `@src/routes/update.ts`:
- Around line 55-59: The local parseBoolean in update.ts duplicates the boolean
parsing logic already used elsewhere; replace it by importing and using the
centralized boolean parser from src/const.ts (or move the parser into a shared
utility and export it from const.ts) instead of defining parseBoolean in
update.ts; update any references inside the update handler to call the exported
helper (e.g., boolean parser name from const.ts) and remove the local
parseBoolean function to avoid duplication and ensure consistent parsing across
HEADLESS/SKIP_ADMIN_SECRET_VALIDATION and routes/update logic.
In `@tests/routes/headless-optimizations.spec.ts`:
- Around line 1-265: The test file groups runtime specs for multiple modules
making ownership unclear; split each describe block into co-located
unit/integration tests next to the module they exercise: move SKIP_STARTUP_ECHO
tests to a new const.test.ts beside src/const.ts, move session/auth tests
(referencing AUTH_CONFIG and stopAuthCleanup) to tests next to
src/routes/auth.ts, and move NIP-46 tests (using initNip46Service and
getNip46Service and server bootstrap behavior) to tests next to
src/nip46/index.ts (and a short bootstrap.integration.test.ts near src/server.ts
if needed); rename files to *.test.ts or *.spec.ts, adjust imports to use
relative paths and retain runRouteScript setup or equivalent per-module helpers
so each test file only references its target module.
In `@tests/routes/update.spec.ts`:
- Around line 5-6: The test currently pins the expected version (e.g., "1.2.0")
which requires manual updates; change the test in tests/routes/update.spec.ts to
derive the expected version from the package.json that the route reads instead
of hardcoding it: read/require the repository package.json (using PROJECT_ROOT
to locate it) to get packageJson.version and use that value in the assertions
that currently compare against the pinned string (the assertions around lines
referencing 28-31), so the test uses the live package version for
expectedVersion.
- Around line 1-56: Move the test file tests/routes/update.spec.ts next to the
implementation src/routes/update.ts (e.g., src/routes/update.spec.ts or
src/routes/update.test.ts) so the route and its spec are co-located; rename the
file to follow the project's convention (feature.spec.ts or feature.test.ts) and
update any import paths or test runner helpers (runRouteScript usage) that
assume its former location to use the new relative path alongside
handleUpdateRoute in src/routes/update.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bd35ff6-8a1f-4419-8252-5b0b0b38aabd
📒 Files selected for processing (22)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/umbrel-dev.ymldocs/CONFIG.mddocs/DEPLOY.mddocs/RELEASE.mddocs/openapi/openapi.jsondocs/openapi/openapi.yamlenv.examplepackage.jsonscripts/release.shscripts/sync-version.mjssrc/db/nip46.tssrc/nip46/index.tssrc/node/manager.tssrc/routes/auth.tssrc/routes/nip46.tssrc/routes/update.tssrc/server.tstests/routes/headless-optimizations.spec.tstests/routes/nip46.spec.tstests/routes/update.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/routes/nip46.spec.ts
- docs/DEPLOY.md
- package.json
| on: | ||
| push: | ||
| branches: [ master ] | ||
| branches: | ||
| - master | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files ".github/workflows/*.yml"Repository: FROSTR-ORG/igloo-server
Length of output: 154
🏁 Script executed:
cat -n ".github/workflows/release.yml"Repository: FROSTR-ORG/igloo-server
Length of output: 7399
Don't trigger release workflow on every push to master without versioning changes.
Every push to master will execute the release job. When package.json hasn't changed, the version is reused from the previous release. At lines 89–90, tag creation is skipped if the tag already exists, but the job continues executing: it builds artifacts, and at lines 96–127 the release action creates or updates a release for that existing tag. This means multiple commits can publish under the same version tag—each overwriting the previous release's artifacts—making it impossible to track which commit corresponds to a released version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 3 - 7, The workflow currently
triggers on every push to master (the on: push branches: - master block) which
allows the tag-creation and release-action steps to overwrite an existing
release; change the trigger to only run when package.json (or other versioned
files) change (e.g., on: push paths: [package.json] for master) OR add an early
job step that checks diff of package.json between HEAD and previous commit
(using actions/checkout + git diff HEAD^..HEAD -- package.json) and exits the
job if there is no change; ensure this check runs before the existing tag
creation step and before the release action so builds and releases are skipped
for commits that don’t change the version.
| permissions: | ||
| packages: write | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: master | ||
| ref: refs/tags/${{ needs.release.outputs.new_version }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/release.yml | head -160 | tail -50Repository: FROSTR-ORG/igloo-server
Length of output: 1977
🏁 Script executed:
# Get the full context of the docker job
sed -n '100,150p' .github/workflows/release.ymlRepository: FROSTR-ORG/igloo-server
Length of output: 1682
🏁 Script executed:
# Check if there are any other permission patterns in the file
grep -n "permissions:" .github/workflows/release.yml -A 3Repository: FROSTR-ORG/igloo-server
Length of output: 227
🌐 Web query:
actions/checkout v4 permissions contents:read required github actions
💡 Result:
actions/checkout@v4 needs the workflow job’s GITHUB_TOKEN to be able to read repository contents. If you’ve locked down token permissions (for least privilege), you must grant at least:
permissions:
contents: readOtherwise the checkout step can fail because any permission you don’t list is set to none. (github.com)
Notes:
contents: readis sufficient for “clone/build/test” workflows. (github.com)- If you plan to push commits/tags, you’ll need
contents: write(and possibly additional scopes depending on what you do). (github.blog)
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout
- 3: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
🏁 Script executed:
# Check if there are any comments or documentation in the workflow file itself
grep -i "permission\|token\|contents\|read" .github/workflows/release.yml | head -20Repository: FROSTR-ORG/igloo-server
Length of output: 438
Restore repository read permission in the Docker job.
Job-level permissions replace the defaults. With only packages: write here, actions/checkout@v4 cannot fetch the tag reference refs/tags/${{ needs.release.outputs.new_version }}. Add contents: read to enable the checkout step.
🔧 Minimal fix
docker:
runs-on: ubuntu-latest
needs: release
permissions:
+ contents: read
packages: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 132 - 139, The Docker job's
job-level permissions only specify "packages: write", which prevents
actions/checkout@v4 from fetching the tag at ref: refs/tags/${{
needs.release.outputs.new_version }}; update the job's permissions block (the
permissions: entry) to include contents: read alongside packages: write so
actions/checkout@v4 can read the repository contents and checkout the tag.
| echo "🧪 Running type checks and backend tests..." | ||
| bun install --frozen-lockfile | ||
| bun run typecheck | ||
| bun run test:unit | ||
|
|
There was a problem hiding this comment.
Gate release prep on the full bun test suite.
The PR verification steps use bun test, but this script only runs bun run test:unit. That means a release branch can be cut while route/OpenAPI regressions are still red.
🧪 Suggested change
-echo "🧪 Running type checks and backend tests..."
+echo "🧪 Running type checks and full test suite..."
bun install --frozen-lockfile
bun run typecheck
-bun run test:unit
+bun test📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "🧪 Running type checks and backend tests..." | |
| bun install --frozen-lockfile | |
| bun run typecheck | |
| bun run test:unit | |
| echo "🧪 Running type checks and full test suite..." | |
| bun install --frozen-lockfile | |
| bun run typecheck | |
| bun test | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/release.sh` around lines 31 - 35, The release script currently runs
only the unit tests via "bun run test:unit", which lets route/OpenAPI
regressions slip; update scripts/release.sh to invoke the full test suite by
running "bun test" (or add "bun test" as an additional step after "bun run
test:unit") so the script gates release prep on the complete test suite, and
ensure the command is executed normally so nonzero exit codes cause the script
to fail.
| // Parse optional credentials from Sec-WebSocket-Protocol (for non-browser clients) | ||
| const proto = req.headers.get('sec-websocket-protocol'); | ||
| if (proto) { | ||
| const headers = new Headers(authReq.headers); | ||
| const offered = proto.split(',').map(p => p.trim()).filter(Boolean); | ||
| // Choose the first offered value to echo back (required by RFC6455) | ||
| if (offered.length > 0) { | ||
| selectedSubprotocol = offered[0]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the specific line ranges mentioned in the review
echo "=== Lines 839-847 ===" && \
sed -n '839,847p' src/server.ts && \
echo && \
echo "=== Lines 929-937 ===" && \
sed -n '929,937p' src/server.tsRepository: FROSTR-ORG/igloo-server
Length of output: 1210
🏁 Script executed:
# Get broader context around both line ranges to understand the code flow
echo "=== Context around lines 839-847 (WS auth path) ===" && \
sed -n '830,855p' src/server.ts && \
echo && \
echo "=== Context around lines 929-937 (relay upgrade path) ===" && \
sed -n '920,950p' src/server.tsRepository: FROSTR-ORG/igloo-server
Length of output: 3112
🏁 Script executed:
# Search for any other instances of subprotocol handling or Sec-WebSocket-Protocol
rg -n "sec-websocket-protocol|subprotocol|selectedSubprotocol" src/server.ts -i -A 2 -B 2Repository: FROSTR-ORG/igloo-server
Length of output: 1852
Don't echo credential-bearing subprotocol values back in the handshake response.
Both WebSocket upgrade paths blindly echo the first offered Sec-WebSocket-Protocol token back to the client. If a caller sends apikey.<token>, bearer.<token>, or session.<id> as an auth hint, that credential gets exposed in the handshake response, visible in ws.protocol, proxy logs, and access logs.
The event-stream path (lines 839–847) extracts credentials from these hints for authentication, then immediately echoes them back unfiltered. The relay path (lines 929–937) echoes them without even parsing. RFC 6455 requires echoing a negotiated subprotocol, but does not require echoing unvetted tokens from the offer.
Filter out credential-bearing tokens before echoing; only echo legitimate, allowlisted subprotocol names:
Safer negotiation pattern
- if (offered.length > 0) {
- selectedSubprotocol = offered[0];
- }
+ const negotiableProtocols = offered.filter((token) => {
+ const lower = token.toLowerCase();
+ return !(
+ lower.startsWith('apikey.') ||
+ lower.startsWith('api-key.') ||
+ lower.startsWith('bearer.') ||
+ lower.startsWith('session.')
+ );
+ });
+ if (negotiableProtocols.length > 0) {
+ selectedSubprotocol = negotiableProtocols[0];
+ }Apply the same filtering to the relay upgrade path (lines 929–937).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` around lines 839 - 847, The code in the WebSocket upgrade
handling that sets selectedSubprotocol from proto/offered (variables: proto,
offered, selectedSubprotocol, authReq) must not blindly echo credential-bearing
tokens like "apikey.", "bearer.", or "session." back in the
Sec-WebSocket-Protocol handshake; instead filter offered before choosing a
subprotocol by first removing any token that matches those credential prefixes
and then selecting only from a safe allowlist (or validated pattern) of
legitimate subprotocol names; apply the same filtering logic to the relay
upgrade branch that also reads proto and sets selectedSubprotocol so credential
hints are never echoed in ws.protocol, proxy or access logs.
| const bootstrap = import(root + 'src/server.ts'); | ||
| bootstrap.catch((error) => { | ||
| console.error(error instanceof Error ? error.message : String(error)); | ||
| process.exit(1); | ||
| }); | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| const { getNip46Service } = await import(root + 'src/nip46/index.ts'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file exists and get basic info
file_path="tests/routes/headless-optimizations.spec.ts"
if [ -f "$file_path" ]; then
echo "File found. Line count:"
wc -l "$file_path"
else
echo "File not found at $file_path"
# Try to find it
find . -name "headless-optimizations.spec.ts" -type f 2>/dev/null
fiRepository: FROSTR-ORG/igloo-server
Length of output: 138
🏁 Script executed:
# Read the specific lines mentioned: 184-190 and 213-219
echo "=== Lines 184-190 ==="
sed -n '184,190p' tests/routes/headless-optimizations.spec.ts
echo -e "\n=== Lines 213-219 ==="
sed -n '213,219p' tests/routes/headless-optimizations.spec.tsRepository: FROSTR-ORG/igloo-server
Length of output: 833
🏁 Script executed:
# Get more context around those lines to understand the test structure
echo "=== Lines 175-225 (full test context) ==="
sed -n '175,225p' tests/routes/headless-optimizations.spec.tsRepository: FROSTR-ORG/igloo-server
Length of output: 2058
🏁 Script executed:
# Check the file location and see what modules it's testing
echo "=== File location and imports ==="
head -30 tests/routes/headless-optimizations.spec.tsRepository: FROSTR-ORG/igloo-server
Length of output: 1097
🏁 Script executed:
# Check if there's a co-located test file near the modules being tested
echo "=== Checking for co-located tests ==="
find src -name "*.test.ts" -o -name "*.spec.ts" 2>/dev/null | head -20Repository: FROSTR-ORG/igloo-server
Length of output: 181
🏁 Script executed:
# Check the directory structure
echo "=== Directory structure ==="
tree -L 3 src/ 2>/dev/null || find src -type d | head -20
tree -L 3 tests/ 2>/dev/null || find tests -type d | head -20Repository: FROSTR-ORG/igloo-server
Length of output: 2596
Await the bootstrap import; tests can pass after late startup failures.
Both test cases at lines 184–190 and 213–219 create const bootstrap = import(...) but never await it before proceeding. The .catch() handler suppresses errors without blocking; the subsequent await new Promise(..., 1000) only sleeps—it does not await the bootstrap. If the server fails to initialize within that second window, the test can still report success. Await the bootstrap promise before continuing, or poll for readiness to confirm startup completed successfully.
Additionally, this test file should be co-located with the modules it exercises. Move it to src/ alongside the code under test (e.g., src/server.test.ts or restructure to align with the pattern used for other co-located tests like src/routes/utils.test.ts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routes/headless-optimizations.spec.ts` around lines 184 - 190, The test
creates the bootstrap promise (const bootstrap = import(root + 'src/server.ts'))
but never awaits it, so startup errors can occur after the sleep; change the
test to await the bootstrap promise (or replace the sleep with a readiness poll)
before importing getNip46Service and continuing, ensuring any thrown error
surfaces and fails the test; also move this test file to be co-located with the
code under test (e.g., into src/ as server.test.ts or follow the co-location
pattern used by src/routes/utils.test.ts) so it runs alongside the modules it
exercises.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
39-40: Redundant scripts:typecheckandtscare identical.Both scripts execute
tsc --noEmit. Consider removing one or differentiating their purposes (e.g.,tscfor full build,typecheckfor validation).♻️ Suggested fix
- "typecheck": "tsc --noEmit", - "tsc": "tsc --noEmit" + "typecheck": "tsc --noEmit"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 39 - 40, The package.json contains duplicate npm scripts "typecheck" and "tsc" that both run "tsc --noEmit"; remove or differentiate them to avoid redundancy: either delete one of the scripts (keep only "typecheck" or only "tsc") or change "tsc" to perform a full compile/build (e.g., "tsc" -> run the compiler without --noEmit or an actual build step) while keeping "typecheck" as "tsc --noEmit" so their purposes are distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Around line 39-40: The package.json contains duplicate npm scripts "typecheck"
and "tsc" that both run "tsc --noEmit"; remove or differentiate them to avoid
redundancy: either delete one of the scripts (keep only "typecheck" or only
"tsc") or change "tsc" to perform a full compile/build (e.g., "tsc" -> run the
compiler without --noEmit or an actual build step) while keeping "typecheck" as
"tsc --noEmit" so their purposes are distinct.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… milestone Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…api/nip44/* Threshold ECDH on Bifrost returns the raw shared X coordinate, which the previous /api/nip44/encrypt and /api/nip44/decrypt handlers used directly as the NIP-44 conversation key. That broke interoperability with every standards-compliant NIP-44 peer. Adds a shared deriveNip44ConversationKey helper that applies the spec HKDF-extract step (SHA-256, salt='nip44-v2') to the raw shared X before invoking nostr-tools' nip44.encrypt/decrypt. The HTTP route now produces ciphertext decryptable by external NIP-44 peers and accepts externally generated standards-compliant ciphertext, while preserving auth, body-size, rate-limit, method, node-availability, and ECDH-timeout semantics. Released OpenAPI for /api/nip44/* now documents the real POST-only/auth contract surface (401, 405, 413, 500 plus the existing 200/400/429/ 503/504), tightens the peer_pubkey pattern to x-only or 02/03-prefixed compressed forms, and decouples the NIP-04 schema so no NIP-04 behavior is broadened. Adds an interop-first test suite (tests/routes/nip44.spec.ts) that uses nostr-tools as an independent oracle to prove standards compliance, covers the negative legacy/raw-secret/non-v2/malformed cases, and exercises the contract semantics required by the released API. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Test suite: 140 pass / 0 fail - Typecheck, build, docs:validate all green - Review: http-nip44-standards-interop passes - Applied library updates: defense-in-depth auth behavior, secret scanner heuristic workaround Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
All 8 NIP-44 API assertions passed via Bun contract tests: - VAL-NIP44-API-001: encrypt produces standards-compliant ciphertext - VAL-NIP44-API-002: decrypt accepts externally generated ciphertext - VAL-NIP44-API-003: decrypt rejects malformed/non-v2/legacy ciphertext - VAL-NIP44-API-004: auth gating preserved (401 for unauthenticated) - VAL-NIP44-API-005: encrypt/decrypt preserve supported peer key shapes - VAL-NIP44-API-006: HTTP method and operational failure semantics correct - VAL-NIP44-API-007: OpenAPI peer_pubkey schema matches runtime - VAL-NIP44-API-008: OpenAPI documents real auth and response contract Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…44_* RPC Threshold ECDH on Bifrost returns the raw shared X coordinate, which the previous NIP-46 `nip44_encrypt` and `nip44_decrypt` handlers in src/nip46/service.ts used directly as the NIP-44 conversation key. That mirrored the same disclosure already fixed for /api/nip44/* and broke interop with every standards-compliant NIP-44 peer on the NIP-46 RPC surface as well. The fix reuses the shared deriveNip44ConversationKey helper (NIP-44 v2 HKDF-extract with salt 'nip44-v2') in src/routes/crypto-utils.ts, so the HTTP /api/nip44/* and NIP-46 nip44_* surfaces now derive the exact same spec conversation key from the same signer/peer ECDH relationship and cannot drift again. NIP-04 paths are untouched. No legacy raw-secret fallback is introduced. Adds relay-backed-style interop coverage: - tests/routes/nip46.spec.ts gains a `NIP-46 nip44 RPC standards-compliant interop` block covering oracle parity, encrypt/decrypt happy paths against nostr-tools as the standards oracle, negative cases (raw-shared-secret control, malformed, non-v2) on an allowed session, mixed x-only/02/03 peer shapes, missing/empty param errors, request-id preservation on success and on failure, and the contract-required method-specific policy matrix (encrypt-only, decrypt-only, both granted) proving that ungranted nip44_* requests stay pending while granted ones auto-approve via shouldAutoApprove + processApprovedRequest. - tests/routes/protected.api.spec.ts gains cross-surface assertions proving that ciphertext produced by /api/nip44/encrypt decrypts via NIP-46 nip44_decrypt and vice versa on a shared signer instance, with mixed x-only and compressed (02-prefixed) peer key forms, plus an external-peer standards ciphertext that decrypts on both surfaces. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Validators pass: 155 tests, typecheck, build, docs:validate - Review subagent verdict: pass for nip46-nip44-rpc-standards-interop - Applied library updates: architecture.md NIP-44 SSoT note, new testing.md Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…estone Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 participant
Summary
This PR prepares the
v1.2.0release across the API, frontend, runtime, and docs.Highlights
Verification
bun run buildbun testbun run docs:validateSummary by CodeRabbit
New Features
Improvements
Documentation