MCP + CLI: Eden Treaty migration, admin tools, ACL#2433
Merged
Conversation
Replace the raw HTTP wrapper with two typed Eden Treaty clients (user, admin)
so MCP tools become thin path-syntax wrappers over the API and inherit its
type graph end-to-end. The user wanted to thicken the API and keep edge apps
lean; this is the MCP half of that.
User tools: packs, catalog, trips, weather, knowledge, trail-conditions,
trails (existing, rewritten) plus auth, user, feed, packTemplates, seasons,
wildlife, alltrails, upload, guides, ai (new).
Admin tools: stats, users-list, packs-list, catalog-list (CRUD), trails
search/get/geometry, trail-conditions, platform analytics, catalog analytics,
ETL ops. Scoped — invisible until admin_login mints a JWT or
X-PackRat-Admin-Token is supplied.
Feature flags: registerFlaggedTool(flag, ...) keeps a tool hidden until the
flag appears in MCP_FEATURE_FLAGS env or setFeatureFlag toggles it on at
runtime. Backed by MCP SDK's enable/disable + tools/list_changed.
ACL error layer: call() maps 401/403/404/409/422/429 from Treaty's
{ data, error, status } into actionable MCP tool errors, including
admin-aware messages when requiresAdmin: true.
Obsolete __tests__ removed — they targeted the raw client surface and will
be rewritten against the Treaty mock in a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an API-talking surface to the CLI alongside the existing DuckDB
analytics commands. Two cached Treaty clients (user + admin) share a base URL
from ~/.packrat/config.json (env override: PACKRAT_API_URL). Token refresh on
401 is handled by createApiClient; the AuthHooks persist new tokens back to
config atomically (tmp + rename, 0600 perms).
User commands: auth (login/logout/register/refresh/whoami), packs
(list/get/create/delete/items/gap-analysis), trips (list/get/create/delete),
catalog (search/semantic/get/categories), trails (search/get/geometry),
weather (search/forecast — two-step collapsed), feed (list/post/like/comment),
templates (list/get/create/delete), seasons, user (profile/update), ai
(rag/web/sql/schema).
Admin command stub: admin/login mints the short-lived JWT via Basic auth,
admin/logout clears it. Full admin command tree lands in the next commit.
runApi() converts Treaty's { data, error, status } into clean stdout + a
non-zero exit, with ACL-aware messages for 401/403 (admin-vs-user) and
hints to run the right login command.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ytics, ETL) Mirrors the API's admin surface. Every command goes through requireAdmin() which checks the stored JWT (and its visible expiry) before hitting the admin Treaty client. Confirmations gate destructive ops; --json escape hatch for piping into jq. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Group printError/formatError/settle args into an options object so they fit biome's useMaxParams rule, and let biome reorganize imports and collapse a few short multi-line forms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- typeof checks → @packrat/guards (isObject, isString) - raw regex in packTemplates → explicit snake→camel map (no regex) - raw process.env in cli config → nodeEnv.PACKRAT_API_URL (new env entry) - 30 unsafe casts → centralised asRecord/asRecordArray/pickArray helpers in packages/cli/src/api/format.ts; the unavoidable cast is parked once, annotated safe-cast. - MCP feed comment input: parent_comment_id z.string() → z.number().int() so the input schema matches the API's body schema. After this, the pre-push gate (typeof, regex, env, circular, dup-deps, dup-guards, unauth-routes, sorted package.json, casts:strict) is green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the CLI-local format helpers (asRecord/asRecordArray/pickArray) into
@packrat/guards and rename as part of the broader as*→to* convention so
narrowing is one import path for the whole monorepo:
to* (strict) : toString, toNumber, toBoolean, toDate
— return T | undefined when the value matches
to* (coercive) : toArray, toRecord, toRecordArray, toStringRecord
— always return T with an empty default
Legacy as* names are kept as @deprecated aliases so existing callers
continue compiling. pickArray is gone — callers inline it as
toRecordArray(toRecord(data).key), which reads better.
CLI commands now import everything narrowing-related from @packrat/guards
(no more local format.ts).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
isNotFound was using radash's isObject() which returns true only for
plain `{}` objects — Node fs errors are Error instances and slipped past
the check, so the ENOENT was rethrown instead of falling back to an empty
config. Switch to `error instanceof Error` + ErrnoException code check.
Smoke-tested:
packrat packs list → "Not signed in. Run packrat auth login..."
packrat admin stats → "No admin token. Run packrat admin login..."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughAdds admin login and boolean query parsing, new catalog compare and pack weight-breakdown endpoints, weather by-name lookup, schema default removals, extensive CLI commands (auth, admin, packs, trips, catalog, trails, weather, feed, templates, seasons, user, ai), and MCP refactor to typed clients with unified call/error handling. ChangesUnified feature and tooling update
Sequence Diagram(s)sequenceDiagram
rect rgba(66, 135, 245, 0.5)
participant CLI
end
rect rgba(40, 167, 69, 0.5)
participant AdminAPI
end
rect rgba(255, 193, 7, 0.5)
participant Auth
end
CLI->>AdminAPI: POST /api/admin/login { username, password }
AdminAPI->>Auth: checkAdminCredentials()
Auth-->>AdminAPI: valid/invalid
AdminAPI-->>CLI: 200 { token, expiresIn } or 401/429
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
the
dependencies
Contributor
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
Contributor
Coverage Report for API Unit Tests Coverage (./packages/api)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Drop the redundant `.optional().default(N)` pattern from every query
schema where the handler already applies its own default. Treaty types
default-with-optional as required-with-default, which forced every typed
client to pass `period: 'month', range: 12, sort: undefined, limit, offset`
even on calls that should be one-liners. Affected:
catalog: page, limit, vector-search limit/offset, categories limit
guides: list/search page, limit
ai: rag-search limit
admin: analytics platform period/range, analytics catalog limit (x4),
analytics ETL job-failures limit, trails search/conditions
limit/offset, packs-list/trail-conditions limit
feed: posts/comments page, limit
packs: includePublic
trail-conditions: list limit
Body-schema defaults left alone — they're a different cost/benefit and
don't churn every caller the way query params do.
Same commit: clean up `includeDeleted` and gap-analysis `duration`:
- admin/packs-list `includeDeleted` was declared in schema but unread by
the handler. Wire it up (admins can now toggle soft-deleted packs)
and switch to z.coerce.boolean().
- admin/users-list `includeDeleted` was dead code (Better Auth doesn't
support user soft-delete). Drop from the schema entirely.
- admin/trails conditions `includeDeleted` keeps its behavior but switches
to z.coerce.boolean() so typed clients send `true`/`false` instead of
the string literal `'true'`.
- GapAnalysisRequestSchema.duration: z.string() → z.coerce.number().int()
so callers can send the natural number form.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the api schema relaxation: edge apps stop carrying
default-with-default workarounds.
cli/catalog search: drop `sort: undefined` placeholder
cli/admin/analytics: drop forced `period: 'month' ?? args.period` and
`range: 12 ?? args.range`; pass args through, server defaults
cli/admin/packs: pass includeDeleted boolean directly (not '1'/'0')
cli/admin/trails: same — boolean not string literal
cli/packs/gap-analysis: duration is a number now; parse args.duration
mcp/catalog search_gear_catalog: switch from `'sort[field]'`/`'sort[order]'`
bracket notation to `sort: { field, order }` object — the API schema
expects the JSON-preprocessed object form. Previous form was a no-op.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the location query to the first match and returns its 10-day forecast, replacing the two-step (search → forecast) dance every weather caller had to do. MCP `get_weather` and CLI `weather forecast` collapse to a single Treaty call. Saves a round-trip and ~15 lines per consumer (no more "first match not found" / Array.isArray narrowing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Server-computed weight breakdown that returns total / base / worn /
consumable grams plus a per-category aggregation sorted heaviest first.
Removes the 30-line client-side walk MCP's analyze_pack_weight was doing
locally — it's now a single Treaty call.
- New `computePackBreakdown(pack)` in utils/compute-pack.ts
- PackWeightBreakdownSchema + PackCategoryBreakdownSchema in
schemas/packs.ts
- Route uses the same db.query.packs.findFirst + items pattern as the
GET /:packId detail route
- MCP `analyze_pack_weight` collapses to a one-line passthrough
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Accepts { ids: number[] } (2–10) and returns each item's comparison row
plus the lightestId / cheapestId / highestRatedId leaders. Resolved with
a single SQL inArray() query — no per-item round-trips.
MCP `compare_gear_items` collapses to a one-line passthrough: drops ~50
lines of N-parallel-GET + client-side sort logic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…alog
Lean catalog-linked pack item creation. Caller only supplies:
{ catalogItemId, quantity?, notes?, consumable?, worn?, category? }
Server resolves the catalog row, copies name/description/weight/weightUnit/
image/embedding/(default) category, mints the pack item ID, and inserts.
Falls back to the catalog's first category when no override is supplied.
MCP gets a corresponding `add_pack_item_from_catalog` tool that's literally
just the catalog ID + per-pack fields — no more duplicating weight,
weightUnit, name, category at every call site.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every create endpoint now accepts an optional id; the server mints one when absent. Offline-first stores (mobile, Legend State) keep supplying their own client-side IDs so sync-conflict resolution still works — only lean callers (MCP, CLI, web) drop the redundant minting. New shared helper: packages/api/src/utils/ids.ts → `mintId(prefix)`. Format unchanged: `<prefix>_<12-hex>`, e.g. `p_a1b2c3d4e5f6`. Affected endpoints: packs POST / id → optional, server mints p_ packs/items POST /:packId/items id → optional, server mints i_ packs/weight POST /:packId/weight-history id → optional, server mints w_ trips POST / id → optional, server mints t_ pack-templates POST / id → optional, server mints pt_ pack-templates POST /:templateId/items id → optional, server mints pti_ trail-conditions POST / id → optional, server mints tcr_ Also consolidated the duplicate `STRIP_HYPHENS` regex + inline UUID-slice patterns in packTemplates and packs/items handlers — they all go through mintId() now. MCP & CLI stop minting client-side for these create paths (server does it). shortId helpers stay exported in CLI/MCP for the rare offline-first callers that might still want them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a JSON-body credential exchange that mirrors /admin/token's behavior
but doesn't require an HTTP Basic header. The Basic-auth /token route is
kept for the admin SPA's existing CF-Access-cookie + Basic flow; the new
/login is for typed clients (MCP, CLI, anyone using Eden Treaty) so they
no longer have to bypass Treaty just to set Authorization: Basic.
- Same rate-limit + CF Access guard
- Same { token, expiresIn } response shape
- Same credential check, extracted to shared checkAdminCredentials()
- onBeforeHandle exempts both /token and /login from the Bearer guard
MCP `admin_login` and CLI `packrat admin login` now go through Treaty:
no more raw `fetch` with Authorization: Basic, no more bespoke response
parsers — the existing runApi/call wrappers handle errors via the typed
response schema.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
Two bugs caught by `tsc --noEmit` on the merged branch: 1. catalog_items table has `ratingValue` but not `ratingCount` — the new POST /catalog/compare route (T7) was selecting a non-existent column. Drop ratingCount from both the SQL select and CatalogCompareRowSchema. 2. T3 changed gap-analysis `duration` from string→number on the API. Mobile's `GapAnalysisRequest` interface in `apps/expo/features/packs/hooks/ usePackGapAnalysis.ts` still typed it `string` so the Treaty `.post()` call site mismatched. Align with the new API type. Both were silent at runtime in dev (Drizzle would have errored on the unknown column lazily; mobile callers happened to never pass duration). CI's typecheck would have caught them before deploy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
T2 changed admin packs-list / trails/conditions `includeDeleted` from a
string ('true'/undefined) to a boolean, and dropped includeDeleted entirely
from admin users-list (dead code — Better Auth doesn't support user soft-
delete). Three callers in apps/admin/lib/api.ts still passed the string
form and one still set it on users-list — caught by CI typecheck on #2433.
- getUsers(): drop includeDeleted from the request; keep the param on
the public signature for compat but stop forwarding it.
- getPacks(): pass the boolean through unchanged.
- getTrailConditions(): same.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 9b71b69 | Commit Preview URL Branch Preview URL |
May 17 2026, 05:58 AM |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the MCP server and CLI toward Eden Treaty API clients, adds many MCP/CLI API command surfaces, and thickens the API with server-side helper endpoints and optional server-minted IDs.
Changes:
- Replaces MCP legacy API client usage with Treaty clients and adds many new MCP tools/resources.
- Adds CLI auth/admin/user/domain command trees backed by shared API client/config helpers.
- Adds API endpoints/schemas for weather-by-name, pack weight breakdown, catalog compare, from-catalog pack items, optional IDs, and admin login.
Reviewed changes
Copilot reviewed 94 out of 95 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mcp/src/types.ts | Expands MCP agent context/client/admin/feature-flag types. |
| packages/mcp/src/tools/wildlife.ts | Adds wildlife identification MCP tool. |
| packages/mcp/src/tools/weather.ts | Migrates weather tools to Treaty and adds weather search/forecast tools. |
| packages/mcp/src/tools/user.ts | Adds user profile MCP tools. |
| packages/mcp/src/tools/upload.ts | Adds presigned upload MCP tool. |
| packages/mcp/src/tools/trips.ts | Migrates trip MCP tools to Treaty. |
| packages/mcp/src/tools/trails.ts | Migrates trail MCP tools to Treaty and moves AllTrails elsewhere. |
| packages/mcp/src/tools/trail-conditions.ts | Migrates/expands trail condition MCP tools. |
| packages/mcp/src/tools/seasons.ts | Adds season suggestions MCP tool. |
| packages/mcp/src/tools/packTemplates.ts | Adds pack template MCP tools. |
| packages/mcp/src/tools/knowledge.ts | Migrates knowledge tools and adds URL extraction. |
| packages/mcp/src/tools/guides.ts | Adds guide browsing/search MCP tools. |
| packages/mcp/src/tools/feed.ts | Adds feed post/comment MCP tools. |
| packages/mcp/src/tools/catalog.ts | Migrates/expands catalog MCP tools. |
| packages/mcp/src/tools/auth.ts | Adds MCP whoami/admin login/logout tools. |
| packages/mcp/src/tools/alltrails.ts | Adds AllTrails preview MCP tool. |
| packages/mcp/src/tools/ai.ts | Adds AI/web/SQL/schema MCP tools. |
| packages/mcp/src/resources.ts | Migrates MCP resources to Treaty result handling. |
| packages/mcp/src/client.ts | Replaces MCP client re-export with Treaty client factory/result helpers. |
| packages/mcp/src/tests/tools/weather.test.ts | Removes weather tool tests. |
| packages/mcp/src/tests/tools/trips.test.ts | Removes trip tool tests. |
| packages/mcp/src/tests/tools/trail-conditions.test.ts | Removes trail condition tool tests. |
| packages/mcp/src/tests/tools/knowledge.test.ts | Removes knowledge tool tests. |
| packages/mcp/src/tests/tools/catalog.test.ts | Removes catalog tool tests. |
| packages/mcp/src/tests/helpers.ts | Removes MCP test helpers. |
| packages/mcp/src/tests/client.test.ts | Removes MCP client tests. |
| packages/guards/src/narrow.ts | Renames/extends guard helpers under to* aliases. |
| packages/env/src/node.ts | Adds PACKRAT_API_URL to node env parsing. |
| packages/cli/tsconfig.json | Adds CLI type/path mappings. |
| packages/cli/src/index.ts | Registers new API-backed CLI command groups. |
| packages/cli/src/commands/weather/index.ts | Adds weather CLI commands. |
| packages/cli/src/commands/user/index.ts | Adds user profile CLI commands. |
| packages/cli/src/commands/trips/index.ts | Adds trips CLI commands. |
| packages/cli/src/commands/trails/index.ts | Adds trails CLI commands. |
| packages/cli/src/commands/templates/index.ts | Adds pack template CLI commands. |
| packages/cli/src/commands/seasons/index.ts | Adds seasons CLI command. |
| packages/cli/src/commands/packs/list.ts | Adds packs list CLI command. |
| packages/cli/src/commands/packs/items.ts | Adds pack items CLI command. |
| packages/cli/src/commands/packs/index.ts | Registers pack CLI subcommands. |
| packages/cli/src/commands/packs/get.ts | Adds pack get CLI command. |
| packages/cli/src/commands/packs/gap-analysis.ts | Adds pack gap analysis CLI command. |
| packages/cli/src/commands/packs/delete.ts | Adds pack delete CLI command. |
| packages/cli/src/commands/packs/create.ts | Adds pack create CLI command. |
| packages/cli/src/commands/feed/index.ts | Adds feed CLI commands. |
| packages/cli/src/commands/catalog/index.ts | Adds catalog CLI commands. |
| packages/cli/src/commands/auth/whoami.ts | Adds auth session inspection CLI command. |
| packages/cli/src/commands/auth/register.ts | Adds auth registration CLI command. |
| packages/cli/src/commands/auth/refresh.ts | Adds auth refresh CLI command. |
| packages/cli/src/commands/auth/logout.ts | Adds auth logout CLI command. |
| packages/cli/src/commands/auth/login.ts | Adds auth login CLI command. |
| packages/cli/src/commands/auth/index.ts | Registers auth CLI subcommands. |
| packages/cli/src/commands/ai/index.ts | Adds AI CLI commands. |
| packages/cli/src/commands/admin/users.ts | Adds admin user CLI commands. |
| packages/cli/src/commands/admin/trails.ts | Adds admin trail/report CLI commands. |
| packages/cli/src/commands/admin/stats.ts | Adds admin stats CLI command. |
| packages/cli/src/commands/admin/packs.ts | Adds admin pack CLI commands. |
| packages/cli/src/commands/admin/logout.ts | Adds admin logout CLI command. |
| packages/cli/src/commands/admin/login.ts | Adds admin login CLI command. |
| packages/cli/src/commands/admin/index.ts | Registers admin CLI subcommands. |
| packages/cli/src/commands/admin/etl.ts | Adds admin ETL CLI commands. |
| packages/cli/src/commands/admin/catalog.ts | Adds admin catalog CLI commands. |
| packages/cli/src/commands/admin/analytics.ts | Adds admin analytics CLI commands. |
| packages/cli/src/api/run.ts | Adds CLI Treaty response/auth/error helpers. |
| packages/cli/src/api/ids.ts | Adds CLI ID/timestamp helpers. |
| packages/cli/src/api/config.ts | Adds CLI config/token store. |
| packages/cli/src/api/client.ts | Adds CLI Treaty client factories. |
| packages/cli/package.json | Adds API client dependency. |
| packages/api/src/utils/ids.ts | Adds server-side ID minting helper. |
| packages/api/src/utils/compute-pack.ts | Adds pack weight breakdown computation. |
| packages/api/src/schemas/weather.ts | Adds weather-by-name query schema. |
| packages/api/src/schemas/packTemplates.ts | Makes template IDs optional on create schemas. |
| packages/api/src/schemas/packs.ts | Adds weight breakdown schema and numeric duration coercion. |
| packages/api/src/schemas/guides.ts | Moves guide query defaults to handlers. |
| packages/api/src/schemas/catalog.ts | Moves catalog defaults to handlers and adds compare schemas. |
| packages/api/src/schemas/ai.ts | Moves RAG limit default to service/handler. |
| packages/api/src/routes/weather.ts | Adds /weather/by-name. |
| packages/api/src/routes/trips/index.ts | Allows server-minted trip IDs. |
| packages/api/src/routes/trailConditions/reports.ts | Allows server-minted report IDs and adjusts query defaults. |
| packages/api/src/routes/packTemplates/index.ts | Uses server ID minting for templates/items/generated templates. |
| packages/api/src/routes/packs/index.ts | Adds optional IDs, weight breakdown, and from-catalog item endpoint. |
| packages/api/src/routes/guides/index.ts | Applies guide query defaults in handlers. |
| packages/api/src/routes/feed/index.ts | Applies feed pagination defaults in handlers. |
| packages/api/src/routes/catalog/index.ts | Adds catalog compare and handler-side defaults. |
| packages/api/src/routes/admin/trails.ts | Adjusts admin trail report query parsing/defaults. |
| packages/api/src/routes/admin/index.ts | Adds admin body login and updates admin list query handling. |
| packages/api/src/routes/admin/analytics/platform.ts | Moves platform analytics defaults to handlers. |
| packages/api/src/routes/admin/analytics/catalog.ts | Moves catalog analytics defaults to handlers. |
| bun.lock | Updates lockfile for CLI dependency/version catalog changes. |
| apps/expo/features/packs/hooks/usePackGapAnalysis.ts | Updates gap analysis duration type. |
| apps/admin/lib/api.ts | Updates admin API query params for includeDeleted changes. |
Comments suppressed due to low confidence (7)
packages/cli/src/commands/admin/packs.ts:40
- These columns do not match the
packs-listresponse shape: the API returnsuserEmailand does not includeuserIdordeletedinAdminPackItemSchema. Even after unwrapping the paginated response, the table will show blanks for the owner/deleted fields unless the CLI uses fields the endpoint actually returns or the API schema is extended.
packages/mcp/src/tests/client.test.ts:1 - This removes the MCP client/helper tests without adding coverage for the new Treaty-based
call()error mapping and client auth behavior. Since the PR changes the core response/error handling path used by every MCP tool, keep or replace these tests so 401/403/404/422 handling and successful result formatting remain covered.
packages/mcp/src/tests/tools/catalog.test.ts:1 - The catalog tool tests are removed while the implementation now uses different Treaty endpoints, including the new
/catalog/comparepath. Please replace this coverage so route mapping, snake_case-to-query/body translation, and error handling are still verified after the migration.
packages/mcp/src/tests/tools/weather.test.ts:1 - The weather tool tests are deleted, but the tool was migrated to new Treaty calls and a new
/weather/by-nameflow. Add replacement coverage for the new endpoint mapping and error responses so regressions in the single-call forecast flow are caught.
packages/mcp/src/tests/tools/trips.test.ts:1 - The trip tool tests are removed while create/update/delete were migrated from the legacy client to Treaty and changed request shapes. Please replace this coverage so the snake_case inputs, timestamp fields, and PUT/DELETE route mappings remain verified.
packages/mcp/src/tests/tools/trail-conditions.test.ts:1 - Trail-condition tool coverage is being deleted even though this PR adds update/delete/list-my-report behavior and changes create payload mapping. Add replacement tests for those Treaty calls and default/null field handling to avoid regressions.
packages/mcp/src/tests/tools/knowledge.test.ts:1 - The knowledge/AI tool tests are removed while the old tools were split across knowledge and ai modules with new Treaty paths. Replacement tests should cover the new
rag-search, web search, SQL, schema, and URL extraction mappings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+277
to
+282
| const pack = await db.query.packs.findFirst({ | ||
| where: eq(packs.id, params.packId), | ||
| with: { items: { where: eq(packItems.deleted, false) } }, | ||
| }); | ||
| if (!pack) return status(404, { error: 'Pack not found' }); | ||
| return computePackBreakdown(pack); |
Comment on lines
+158
to
+160
| if (items.length === 0) { | ||
| return status(404, { error: 'No catalog items matched the supplied IDs' }); | ||
| } |
Comment on lines
+50
to
+62
| if (cached) return cached; | ||
| try { | ||
| const raw = await readFile(CONFIG_PATH, 'utf8'); | ||
| const parsed = CliConfigSchema.safeParse(JSON.parse(raw)); | ||
| cached = parsed.success ? parsed.data : emptyConfig; | ||
| } catch (e) { | ||
| if (isNotFound(e)) cached = { ...emptyConfig }; | ||
| else throw e; | ||
| } | ||
| // PACKRAT_API_URL env override always wins. Useful for local dev (e.g. | ||
| // pointing the CLI at `http://localhost:8787`). | ||
| const envOverride = nodeEnv.PACKRAT_API_URL?.trim(); | ||
| if (envOverride) cached.baseUrl = envOverride; |
Comment on lines
+26
to
+27
| const password = | ||
| args.password ?? (await consola.prompt('Password', { type: 'text', cancel: 'reject' })); |
Comment on lines
+27
to
+28
| const password = | ||
| args.password ?? (await consola.prompt('Password', { type: 'text', cancel: 'reject' })); |
Comment on lines
+68
to
+76
| printTable( | ||
| toRecordArray(data).map((r) => ({ | ||
| id: r.id, | ||
| trailName: r.trailName, | ||
| condition: r.overallCondition, | ||
| userId: r.userId, | ||
| deleted: r.deleted, | ||
| })), | ||
| { title: 'Trail reports (admin)' }, |
Comment on lines
+224
to
+230
| async ({ content_url, is_app_template }) => | ||
| call( | ||
| agent.api.user['pack-templates']['generate-from-online-content'].post({ | ||
| contentUrl: content_url, | ||
| isAppTemplate: is_app_template, | ||
| }), | ||
| { action: 'generate pack template from URL', requiresAdmin: true }, |
| offset: z.coerce.number().int().min(0).optional(), | ||
| q: z.string().optional(), | ||
| includeDeleted: z.string().optional(), | ||
| includeDeleted: z.coerce.boolean().optional(), |
Comment on lines
+312
to
+315
| q: z.string().optional(), | ||
| limit: z.coerce.number().int().min(1).max(100).optional().default(50), | ||
| offset: z.coerce.number().int().min(0).optional().default(0), | ||
| includeDeleted: z.string().optional(), | ||
| limit: z.coerce.number().int().min(1).max(100).optional(), | ||
| offset: z.coerce.number().int().min(0).optional(), | ||
| includeDeleted: z.coerce.boolean().optional(), |
Comment on lines
+44
to
+47
| const result = await promise; | ||
| if (result.error || result.data == null) { | ||
| printError({ status: result.status, body: errorValue(result.error), opts }); | ||
| process.exit(1); |
Contributor
Deploying packrat-guides with
|
| Latest commit: |
9b71b69
|
| Status: | 🚫 Build failed. |
Contributor
Deploying packrat-landing with
|
| Latest commit: |
9b71b69
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://70679d1e.packrat-landing.pages.dev |
| Branch Preview URL: | https://feat-client-uuid-split.packrat-landing.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/routes/trips/index.ts (1)
76-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap create ID conflicts to 409 instead of generic 500.
With client-provided IDs (offline/retry flows), unique-key collisions are expected edge cases. Returning 500 hides a conflict that callers can handle.
Proposed fix
} catch (error) { + if ( + typeof error === 'object' && + error !== null && + 'code' in error && + (error as { code?: string }).code === '23505' + ) { + return status(409, { error: 'Trip ID already exists' }); + } console.error('Error creating trip:', error); return status(500, { error: 'Failed to create trip' }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/routes/trips/index.ts` around lines 76 - 108, The catch block around the insert in the trips creation path currently maps all DB errors to 500; detect unique-key/ID conflict errors from the DB when inserting via db.insert(trips) (the block that creates newTrip) and return status(409, { error: 'Trip ID already exists' }) instead of a 500. Implement this by inspecting the thrown error in the catch (e.g., error.code === '23505' for Postgres, or checking error.message for 'duplicate'/'unique constraint' if your driver exposes different codes) and branching: if it's a unique-constraint/duplicate-id error return 409, otherwise log and return the existing 500 response.packages/guards/src/narrow.ts (1)
93-101: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
toStringRecorddoesn't exclude arrays liketoRecorddoes.
toRecordguards withArray.isArray(value)buttoStringRecordonly checkstypeof value !== 'object'. Arrays are objects, sotoStringRecord(['a', 'b'])returns{ '0': 'a', '1': 'b' }instead of{}.Proposed fix for consistency
export const toStringRecord = (value: unknown): Record<string, string> => { - if (value === null || typeof value !== 'object') return {}; + if (value === null || typeof value !== 'object' || Array.isArray(value)) return {}; const out: Record<string, string> = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/guards/src/narrow.ts` around lines 93 - 101, toStringRecord currently treats arrays as objects and returns numeric-keyed entries for arrays; mirror toRecord's behavior by excluding arrays—update the guard in toStringRecord to also check Array.isArray(value) and return {} when true, keeping the existing loop and type assertion (value as Record<string, unknown>) unchanged so only true object maps of string values are emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/routes/catalog/index.ts`:
- Around line 141-187: The compare/fetch/ranking business logic in the route
handler should be moved into CatalogService: create a method on CatalogService
(e.g., compareItems or getRankedItems) that accepts ids (or uniqueIds) and
performs the DB select (using createDb()/catalogItems), verifies missing ids,
and returns { items, lightestId, cheapestId, highestRatedId } using the existing
rank logic (extract and reuse the rank function there). Update the route handler
to call CatalogService.compareItems and map its result to the HTTP response;
ensure the service throws or returns a clear error for missing ids so the route
can translate it to a 404 status.
- Around line 143-165: After deduping ids into uniqueIds (the Array.from(new
Set(ids)) result) enforce the "2–10 items" rule by checking uniqueIds.length and
returning a 400 (or the same validation error shape) if uniqueIds.length < 2 (or
>10) before querying the DB; update the logic around uniqueIds / the
missing-items block in the catalog route (the code referencing uniqueIds, items,
foundIds) so requests like [1,1] are rejected as invalid after dedupe.
In `@packages/api/src/routes/packs/index.ts`:
- Around line 734-751: The insert is failing because required hydrated fields
from catalog (e.g., catalog.name used for packItems.name) can be null; before
calling db.insert(packItems) in this route, validate that catalog.name (and any
other non-nullable columns you map such as weight if the schema forbids null)
are present and of the expected type, and if not return a 4xx (e.g., status(400,
{ error: 'Missing required catalog field: name' })) instead of attempting the
insert; locate the lookup using catalog = await
db.query.catalogItems.findFirst(...) and the insert via
db.insert(packItems).values({ id, packId, catalogItemId: catalog.id, name:
catalog.name, ... }) and add the guard/validation and early response there.
- Around line 724-791: The route handler is doing hydration, defaults,
persistence mapping and response shaping; extract this into a service function
(e.g., createPackItemFromCatalog or AddPackItemFromCatalogService) that accepts
(db, user, packId, body), performs pack and catalog lookups, builds the
NewPackItem payload (apply defaults for
quantity/weight/weightUnit/category/consumable/worn/image/notes), inserts the
pack item, updates packs.updatedAt, and returns the fully shaped response object
(with ISO timestamps, boolean defaults, embedding omitted, templateItemId
normalized); replace the current inline logic in the route with a thin call to
that service and return its status/result.
In `@packages/api/src/routes/trailConditions/reports.ts`:
- Line 18: The optional id string currently allows empty/whitespace values;
change the schema for the `id` field(s) from z.string().optional() to a trimmed,
non-empty optional string like
z.string().trim().min(1).optional().describe('Client-generated report ID; server
mints when absent') so blank/whitespace values are rejected (apply the same
change to the other `id` occurrence referenced at line ~107).
In `@packages/api/src/routes/trips/index.ts`:
- Around line 19-21: The schema currently uses id: z.string().optional() which
allows empty/whitespace and lets data.id ?? mintId('t') treat invalid ids as
present; update the validation to reject empty/whitespace by replacing
z.string().optional() with a trimmed nonempty string validator (e.g.,
z.string().trim().min(1).optional() or z.string().nonempty().transform(...)) so
only meaningful client IDs are accepted, and ensure any codepaths that use
data.id (the mintId('t') fallback in the create/update handlers) rely on that
validated field; apply the same change to the other schema instance referenced
around lines 74-80.
In `@packages/api/src/routes/weather.ts`:
- Around line 183-185: Remove the redundant runtime check for `q` in the route
handler: the `if (!q || q.length < 2) { return status(400, { error: 'Query
parameter q (≥ 2 chars) is required' }); }` block in the weather route can be
deleted because `WeatherByNameQuerySchema` (z.string().min(2)) enforces this
before the handler runs; keep the rest of the handler logic and rely on Elysia's
schema validation for input enforcement (references: `WeatherByNameQuerySchema`,
the `q` parameter, and the `status` call).
In `@packages/api/src/schemas/catalog.ts`:
- Around line 353-363: The weightUnit field in CatalogCompareRowSchema is too
permissive (string|null); update CatalogCompareRowSchema to use the existing
WEIGHT_UNITS enum contract instead (e.g., replace weightUnit:
z.string().nullable() with weightUnit: z.enum(WEIGHT_UNITS).nullable() or the
project's equivalent enum wrapper) so the schema matches other catalog schemas
and preserves correct typing; ensure WEIGHT_UNITS is imported/available in the
file and adjust any nullable handling to match the rest of the catalog schemas.
In `@packages/api/src/schemas/packTemplates.ts`:
- Line 58: The schema field id: z.string().optional() allows empty strings,
which bypasses minting because callers use data.id ?? mintId(...); change the
schema to disallow empty strings (e.g., replace with id:
z.string().min(1).optional() or id: z.string().optional().refine(s => s !== "",
{ message: "id cannot be blank" })) so blank ids are rejected and mintId(...)
will run when no id is provided.
In `@packages/api/src/utils/compute-pack.ts`:
- Around line 99-106: The top-level itemCount currently uses pack.items.length
which counts rows, causing inconsistency with byCategory (which uses
quantities); update the code that builds the returned object (the itemCount
field in the compute-pack return) to sum each item's quantity (e.g., iterate
pack.items and add item.quantity or default 1 when missing) so itemCount is
quantity-aware and matches byCategory; reference pack.items and the itemCount
field in the compute-pack.ts return object when making this change.
In `@packages/cli/src/commands/admin/catalog.ts`:
- Around line 61-68: When building the PATCH body in the admin/catalog command,
validate parsed numeric inputs and prevent empty updates: ensure
Number.parseFloat(args.weight) and Number.parseFloat(args.price) are checked for
NaN and if invalid return an error (or exit) with a helpful message; only set
body.weight/body.price when parsing succeeds; after assembling body (the object
used in admin.catalog({ id: args.id }).patch(body) / runApi), reject the
operation if body has no own properties (no fields were supplied) and inform the
user that at least one update flag is required.
In `@packages/cli/src/commands/admin/trails.ts`:
- Around line 13-14: The CLI currently defines limit/offset as strings and
allows invalid values to become NaN downstream; update the options parsing in
the admin/trails command to parse and validate --limit and --offset as integers
(non-negative, limit > 0) before making any requests and throw a clear CLI error
when validation fails. Locate the option definitions for limit/offset and the
command handler in packages/cli/src/commands/admin/trails.ts (the places around
the shown defs and the similar blocks at the other occurrences referenced) and
convert the values to integers (e.g., parseInt), check
isFinite/non-negative/integer constraints, and call yargs'
throw/console.error/exit with a descriptive message if invalid so the CLI fails
fast. Ensure the same validation is applied to the other occurrences (the lines
referenced at 24-25, 45-46, 57-58).
In `@packages/cli/src/commands/admin/users.ts`:
- Around line 12-13: The CLI currently accepts --limit and --offset as strings
and can pass malformed values (NaN) to the API; update the users command to
parse and validate these flags as non-negative integers before building the
request: use parseInt on the option values for 'limit' and 'offset' (the option
keys shown as limit and offset) and if the result is NaN or negative, print a
user-friendly error and exit (or fall back to the defaults), applying the same
guard where the flags are read/used (the second occurrence noted at lines
23-24).
In `@packages/cli/src/commands/ai/index.ts`:
- Around line 15-17: Validate the --limit value before invoking
client.ai['rag-search'].get: check args.limit (parsed as
Number.parseInt(args.limit, 10)) is a finite integer > 0 and within any sensible
max; if invalid, print a clear CLI error and exit instead of sending
NaN/0/negative to the API. Update the code path that calls
client.ai['rag-search'].get (and the similar call around the other block that
parses args.limit) to perform this validation and early-fail with a
user-friendly message.
In `@packages/cli/src/commands/auth/login.ts`:
- Around line 35-46: The sign-in fetch call can throw network exceptions before
you check response.ok; wrap the fetch to `${baseUrl}/api/auth/sign-in/email` in
a try/catch around the await fetch(...) (the code that assigns to response) and
in the catch log a controlled error with consola.error including the caught
error.message (or the error object) and then call process.exit(1); keep the
existing response.ok handling intact so HTTP errors still produce the same
consola.error/chalk.dim body behavior.
In `@packages/cli/src/commands/auth/logout.ts`:
- Around line 23-29: The fetch call to `${baseUrl}/api/auth/sign-out` currently
only catches thrown exceptions but treats non-2xx HTTP responses as success;
modify the code around the fetch (the POST to /api/auth/sign-out that uses
`baseUrl` and `config.accessToken`) to capture the response (`const res = await
fetch(...)`), check `if (!res.ok)` and then log a warning with the status and
response body (await `res.text()` or `res.json()` safely) so non-2xx server
sign-outs are surfaced (keep the catch block to handle network errors as well).
In `@packages/cli/src/commands/auth/register.ts`:
- Around line 26-35: The register command currently sends the API request even
if prompts returned empty strings; update the validation in the register flow to
check email, name, and password (the variables email, name, password populated
via consola.prompt / promptPassword) before building the fetch call to
getBaseUrl()/fetch; if any required field is empty or only whitespace, print or
throw a clear local error (e.g., using consola.error or throw new Error) and
exit early so the request is not sent with invalid payload. Ensure the
validation runs after collecting prompt results but before the fetch invocation.
In `@packages/cli/src/commands/auth/whoami.ts`:
- Around line 20-21: The displayed identity fields currently prefer persisted
config values (config.userId, config.userEmail) over the live profile; change
the selection so the profile-derived values (user.id, user.email) are used first
and config values are fallbacks. Update the object building in whoami.ts to set
userId to user.id ?? config.userId ?? '—' and email to user.email ??
config.userEmail ?? '—', ensuring you reference the existing symbols (userId,
config.userId, user.email, config.userEmail) when making the swap.
In `@packages/cli/src/commands/catalog/index.ts`:
- Around line 101-103: The item summary mapping uses the wrong key for review
count: replace references to r.ratingCount with r.reviewCount in the mapping
that builds the summary (see the object properties rating: r.ratingValue,
reviewCount: r.ratingCount, productUrl: r.productUrl) so the reviewCount field
reads r.reviewCount instead; update any other occurrences in the same function
or related mapping where r.ratingCount is used to ensure the catalog uses the
correct reviewCount field.
- Around line 65-70: The table mapping uses the wrong field for similarity: in
the map inside toRecordArray(toRecord(data).items) replace uses of it.score with
it.similarity (or use a fallback like it.similarity ?? it.score) so the "score"
column shows the vector-search similarity; update the mapping object where id,
name, brand, score are set to read similarity from each item.
In `@packages/cli/src/commands/packs/gap-analysis.ts`:
- Around line 18-35: The duration value from args.duration is parsed with
Number.parseInt inside the request body which can produce NaN or accept partial
strings (e.g., "3abc"); validate and sanitize args.duration before building the
request in run: parse args.duration to an integer, verify
Number.isInteger(parsed) and parsed > 0 (or enforce your business min/max), and
if invalid throw/exit with a clear error message to the user rather than sending
null/NaN in the body; update the logic around run and the call to
client.packs(... )['gap-analysis'].post to use the validated integer (or bail
early) and reference args.duration in the validation step.
In `@packages/cli/src/commands/trails/index.ts`:
- Around line 24-32: The query construction is accepting malformed numbers and
allows only one coordinate; validate parsed values before building the query and
return a clear CLI error if invalid. Specifically, check
Number.parseFloat(args.lat/args.lon/radius) and
Number.parseInt(args.limit/args.offset,10) for NaN and reject with an error
message; require that if either args.lat or args.lon is provided both must be
present (and both numeric); enforce sane defaults for limit/offset if omitted.
Implement these checks right before creating the query object that uses args.q,
args.lat, args.lon, args.radius, args.limit, args.offset and surface errors to
the CLI (throw or call the CLI error helper) so malformed input never reaches
the API call.
In `@packages/cli/src/commands/trips/index.ts`:
- Around line 82-87: The current parsing of args.lat/args.lon (stored in lat,
lon) silently drops an invalid or partial location by setting location to null;
instead validate inputs and fail fast: if one of args.lat or args.lon is
provided without the other, if parseFloat yields NaN, or if lat/ lon are outside
valid ranges (latitude not in [-90,90] or longitude not in [-180,180]), throw or
exit with a clear error message indicating which coord is missing/invalid; only
construct the location object ({ latitude: lat, longitude: lon, name:
args['location-name'] }) when both parsed numbers are valid and in range.
In `@packages/cli/src/commands/user/index.ts`:
- Around line 38-44: The current update flow builds a body object and may call
runApi(client.user.profile.put(body)) with an empty {}; add a guard after
constructing body (check Object.keys(body).length === 0) to short-circuit and
return an immediate user-facing error message (e.g., write error to stderr or
console.error and exit with non-zero status) instead of calling
client.user.profile.put; update the code around the body variable and the runApi
invocation to perform this check and return early when no fields are provided.
In `@packages/mcp/src/index.ts`:
- Around line 163-175: The fetch override reads X-PackRat-Admin-Token and
updates this.state.adminToken but never triggers syncAdminToolVisibility(), so
admin UI remains disabled when token is provided via header; modify the fetch
method (in packages/mcp/src/index.ts) to call this.syncAdminToolVisibility()
after updating state (i.e., when nextAdmin !== this.state.adminToken or nextAuth
changed and you call this.setState(...)) or invoke this.setAdminToken(nextAdmin)
instead of only setState so the visibility logic in
setAdminToken()/syncAdminToolVisibility() runs and admin tools become visible
when the header supplies the token.
In `@packages/mcp/src/tools/admin.ts`:
- Around line 71-75: The query is sending include_deleted as 1/0 which the API
expects as a boolean; update the calls that build the GET query (e.g., the call
to agent.api.admin.admin['packs-list'].get that currently sets query: { ...,
includeDeleted: include_deleted ? 1 : 0 }) to pass a true/false boolean
(includeDeleted: Boolean(include_deleted) or include_deleted === true) instead
of 1/0, and make the same change for the other similar call referenced around
the second occurrence so both places send includeDeleted as a boolean.
- Line 50: The inputSchema for the delete-audit endpoint currently uses
z.string().min(1) which allows whitespace-only reasons; update the schema so the
`reason` is trimmed before validating length (e.g. use z.string().transform(s =>
s.trim()).min(1) or a refine that checks trimmed length) so only non-empty,
non-whitespace reasons pass; modify the `inputSchema` object in admin.ts (the
entry where `inputSchema: { user_id: z.string(), reason: z.string().min(1) }` is
defined) to apply the trim-and-validate transformation or refine on `reason`.
In `@packages/mcp/src/tools/ai.ts`:
- Around line 27-34: The inputSchema currently allows arbitrary SQL in
inputSchema.query before calling
call(agent.api.user.ai['execute-sql'].post(...)); add a schema-level guard on
inputSchema.query to enforce read-only SELECTs by validating the string starts
with a SELECT (case-insensitive), contains no semicolons or disallowed keywords
(INSERT, UPDATE, DELETE, DROP, ALTER, TRUNCATE, CREATE) and fails validation
otherwise; implement this as a zod refinement on inputSchema.query (or a custom
validator) so non-read statements are rejected before calling execute-sql.
In `@packages/mcp/src/tools/alltrails.ts`:
- Around line 10-11: The inputSchema currently allows any URL but should
restrict to AllTrails hosts; update the inputSchema (the z.string().url()
validation in packages/mcp/src/tools/alltrails.ts) to refine the URL host to
only AllTrails domains (e.g., alltrails.com and other regional domains like
alltrails.ca and any subdomains). Implement this by parsing the URL (new
URL(url).hostname) inside a z.string().url().refine(...) (or equivalent
z.preprocess+refine) and return a clear message such as "URL must be an
AllTrails link" on failure so that the AllTrails-only contract described in the
tool description is enforced.
In `@packages/mcp/src/tools/feed.ts`:
- Around line 101-109: The schema defines parent_comment_id as
z.number().int().optional() but the API call uses string comment IDs elsewhere
(e.g., comment_id), causing a type mismatch; update the input schema in the
async handler to use z.string().optional() for parent_comment_id and ensure the
call to agent.api.user.feed(...).comments.post passes parentCommentId as that
string (and adjust any related validation/consumers expecting numeric IDs to
accept strings) so reply linking uses a consistent string ID type (refer to
parent_comment_id and comment_id).
In `@packages/mcp/src/tools/packTemplates.ts`:
- Around line 170-181: The update_pack_template_item inputSchema currently omits
the image field so image cannot be changed after creation; modify the
update_pack_template_item inputSchema to include the same optional image
property used on create (allowing the same type/validation as create, e.g.,
optional string or URL), and also add that image property to the other update
inputSchema block referenced in the comment (the schema around lines 183-191) so
both update paths accept image updates; ensure you use the same field name
"image" and validation rules as the create inputSchema for consistency.
- Around line 218-221: The content_url field in the inputSchema currently
accepts any URL but must be restricted to supported hostnames (TikTok/YouTube)
to match the tool contract; update the inputSchema's content_url validator (the
symbol "content_url" inside "inputSchema" in packTemplates.ts) to replace or
chain the z.string().url() with a hostname whitelist check (e.g., using URL
parsing and zod .refine/.superRefine) that only allows hostnames like
youtube.com, www.youtube.com, youtu.be, tiktok.com, and www.tiktok.com; return a
clear validation message on failure so upstream handlers can reject unsupported
domains before fetching.
In `@packages/mcp/src/tools/upload.ts`:
- Around line 11-18: The inputSchema currently allows any non-empty string for
content_type but the docstring restricts uploads to jpeg/png/webp; update the
validation in inputSchema to only accept the allowed MIME types (e.g.
image/jpeg, image/png, image/webp) instead of z.string().min(1). Locate the
content_type entry inside inputSchema and replace the loose string check with a
strict enum/union of literals so invalid content types are rejected alongside
the existing file_name and size validations.
In `@packages/mcp/src/tools/user.ts`:
- Around line 29-34: Replace the loose Record<string, unknown> with a concrete
interface (e.g., UpdateUserProfileRequest) that declares optional typed
properties firstName?: string, lastName?: string, email?: string, avatarUrl?:
string, then type the body variable as that interface and keep the same mapping
from incoming snake_case params to camelCase fields before calling
call(agent.api.user.user.profile.put(body), { action: 'update profile' }); this
will restore compile-time checking and IDE autocomplete for the request payload.
---
Outside diff comments:
In `@packages/api/src/routes/trips/index.ts`:
- Around line 76-108: The catch block around the insert in the trips creation
path currently maps all DB errors to 500; detect unique-key/ID conflict errors
from the DB when inserting via db.insert(trips) (the block that creates newTrip)
and return status(409, { error: 'Trip ID already exists' }) instead of a 500.
Implement this by inspecting the thrown error in the catch (e.g., error.code ===
'23505' for Postgres, or checking error.message for 'duplicate'/'unique
constraint' if your driver exposes different codes) and branching: if it's a
unique-constraint/duplicate-id error return 409, otherwise log and return the
existing 500 response.
In `@packages/guards/src/narrow.ts`:
- Around line 93-101: toStringRecord currently treats arrays as objects and
returns numeric-keyed entries for arrays; mirror toRecord's behavior by
excluding arrays—update the guard in toStringRecord to also check
Array.isArray(value) and return {} when true, keeping the existing loop and type
assertion (value as Record<string, unknown>) unchanged so only true object maps
of string values are emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97913088-7769-4521-b2a8-93e910edd240
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (96)
apps/admin/lib/api.tsapps/expo/features/packs/hooks/usePackGapAnalysis.tspackages/api/src/routes/admin/analytics/catalog.tspackages/api/src/routes/admin/analytics/platform.tspackages/api/src/routes/admin/index.tspackages/api/src/routes/admin/trails.tspackages/api/src/routes/catalog/index.tspackages/api/src/routes/feed/index.tspackages/api/src/routes/guides/index.tspackages/api/src/routes/packTemplates/index.tspackages/api/src/routes/packs/index.tspackages/api/src/routes/trailConditions/reports.tspackages/api/src/routes/trips/index.tspackages/api/src/routes/weather.tspackages/api/src/schemas/ai.tspackages/api/src/schemas/catalog.tspackages/api/src/schemas/guides.tspackages/api/src/schemas/packTemplates.tspackages/api/src/schemas/packs.tspackages/api/src/schemas/weather.tspackages/api/src/utils/compute-pack.tspackages/api/src/utils/ids.tspackages/cli/package.jsonpackages/cli/src/api/client.tspackages/cli/src/api/config.tspackages/cli/src/api/ids.tspackages/cli/src/api/prompt.tspackages/cli/src/api/run.tspackages/cli/src/commands/admin/analytics.tspackages/cli/src/commands/admin/catalog.tspackages/cli/src/commands/admin/etl.tspackages/cli/src/commands/admin/index.tspackages/cli/src/commands/admin/login.tspackages/cli/src/commands/admin/logout.tspackages/cli/src/commands/admin/packs.tspackages/cli/src/commands/admin/stats.tspackages/cli/src/commands/admin/trails.tspackages/cli/src/commands/admin/users.tspackages/cli/src/commands/ai/index.tspackages/cli/src/commands/auth/index.tspackages/cli/src/commands/auth/login.tspackages/cli/src/commands/auth/logout.tspackages/cli/src/commands/auth/refresh.tspackages/cli/src/commands/auth/register.tspackages/cli/src/commands/auth/whoami.tspackages/cli/src/commands/catalog/index.tspackages/cli/src/commands/feed/index.tspackages/cli/src/commands/packs/create.tspackages/cli/src/commands/packs/delete.tspackages/cli/src/commands/packs/gap-analysis.tspackages/cli/src/commands/packs/get.tspackages/cli/src/commands/packs/index.tspackages/cli/src/commands/packs/items.tspackages/cli/src/commands/packs/list.tspackages/cli/src/commands/seasons/index.tspackages/cli/src/commands/templates/index.tspackages/cli/src/commands/trails/index.tspackages/cli/src/commands/trips/index.tspackages/cli/src/commands/user/index.tspackages/cli/src/commands/weather/index.tspackages/cli/src/index.tspackages/cli/tsconfig.jsonpackages/env/src/node.tspackages/guards/src/narrow.tspackages/guards/src/parse.tspackages/mcp/src/__tests__/auth.test.tspackages/mcp/src/__tests__/client.test.tspackages/mcp/src/__tests__/helpers.tspackages/mcp/src/__tests__/tools/catalog.test.tspackages/mcp/src/__tests__/tools/knowledge.test.tspackages/mcp/src/__tests__/tools/packs.test.tspackages/mcp/src/__tests__/tools/trail-conditions.test.tspackages/mcp/src/__tests__/tools/trips.test.tspackages/mcp/src/__tests__/tools/weather.test.tspackages/mcp/src/client.tspackages/mcp/src/index.tspackages/mcp/src/resources.tspackages/mcp/src/tools/admin.tspackages/mcp/src/tools/ai.tspackages/mcp/src/tools/alltrails.tspackages/mcp/src/tools/auth.tspackages/mcp/src/tools/catalog.tspackages/mcp/src/tools/feed.tspackages/mcp/src/tools/guides.tspackages/mcp/src/tools/knowledge.tspackages/mcp/src/tools/packTemplates.tspackages/mcp/src/tools/packs.tspackages/mcp/src/tools/seasons.tspackages/mcp/src/tools/trail-conditions.tspackages/mcp/src/tools/trails.tspackages/mcp/src/tools/trips.tspackages/mcp/src/tools/upload.tspackages/mcp/src/tools/user.tspackages/mcp/src/tools/weather.tspackages/mcp/src/tools/wildlife.tspackages/mcp/src/types.ts
💤 Files with no reviewable changes (9)
- packages/mcp/src/tests/helpers.ts
- packages/mcp/src/tests/tools/trail-conditions.test.ts
- packages/mcp/src/tests/tools/catalog.test.ts
- packages/mcp/src/tests/auth.test.ts
- packages/mcp/src/tests/tools/weather.test.ts
- packages/mcp/src/tests/tools/knowledge.test.ts
- packages/mcp/src/tests/client.test.ts
- packages/mcp/src/tests/tools/packs.test.ts
- packages/mcp/src/tests/tools/trips.test.ts
Comment on lines
+141
to
+187
| const db = createDb(); | ||
| const { ids } = body; | ||
| const uniqueIds = Array.from(new Set(ids)); | ||
| const items = await db | ||
| .select({ | ||
| id: catalogItems.id, | ||
| name: catalogItems.name, | ||
| brand: catalogItems.brand, | ||
| weight: catalogItems.weight, | ||
| weightUnit: catalogItems.weightUnit, | ||
| price: catalogItems.price, | ||
| ratingValue: catalogItems.ratingValue, | ||
| productUrl: catalogItems.productUrl, | ||
| categories: catalogItems.categories, | ||
| }) | ||
| .from(catalogItems) | ||
| .where(inArray(catalogItems.id, uniqueIds)); | ||
|
|
||
| const foundIds = new Set(items.map((it) => it.id)); | ||
| const missing = uniqueIds.filter((id) => !foundIds.has(id)); | ||
| if (missing.length > 0) { | ||
| return status(404, { | ||
| error: `Catalog item(s) not found: ${missing.join(', ')}`, | ||
| }); | ||
| } | ||
|
|
||
| const rank = <K extends keyof (typeof items)[number]>( | ||
| key: K, | ||
| order: 'asc' | 'desc', | ||
| ): number | null => { | ||
| const ranked = [...items] | ||
| .filter((it) => it[key] != null) | ||
| .sort((a, b) => { | ||
| const av = Number(a[key]); | ||
| const bv = Number(b[key]); | ||
| return order === 'asc' ? av - bv : bv - av; | ||
| }); | ||
| return ranked[0]?.id ?? null; | ||
| }; | ||
|
|
||
| return { | ||
| items, | ||
| lightestId: rank('weight', 'asc'), | ||
| cheapestId: rank('price', 'asc'), | ||
| highestRatedId: rank('ratingValue', 'desc'), | ||
| }; | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move compare query/ranking logic into CatalogService.
The new compare flow contains business logic (fetch + ranking) directly in the route handler, which makes it harder to test/reuse and drifts from the API layering convention.
As per coding guidelines "Business logic belongs in src/services/, not in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/catalog/index.ts` around lines 141 - 187, The
compare/fetch/ranking business logic in the route handler should be moved into
CatalogService: create a method on CatalogService (e.g., compareItems or
getRankedItems) that accepts ids (or uniqueIds) and performs the DB select
(using createDb()/catalogItems), verifies missing ids, and returns { items,
lightestId, cheapestId, highestRatedId } using the existing rank logic (extract
and reuse the rank function there). Update the route handler to call
CatalogService.compareItems and map its result to the HTTP response; ensure the
service throws or returns a clear error for missing ids so the route can
translate it to a 404 status.
Comment on lines
+724
to
+791
| async ({ params, body, user }) => { | ||
| const db = createDb(); | ||
| const packId = params.packId; | ||
|
|
||
| const pack = await db.query.packs.findFirst({ | ||
| where: and(eq(packs.id, packId), eq(packs.userId, user.userId)), | ||
| columns: { id: true }, | ||
| }); | ||
| if (!pack) return status(404, { error: 'Pack not found' }); | ||
|
|
||
| const catalog = await db.query.catalogItems.findFirst({ | ||
| where: eq(catalogItems.id, body.catalogItemId), | ||
| }); | ||
| if (!catalog) { | ||
| return status(404, { error: `Catalog item ${body.catalogItemId} not found` }); | ||
| } | ||
|
|
||
| const id = mintId('i'); | ||
| const now = new Date(); | ||
| const [newItem] = await db | ||
| .insert(packItems) | ||
| .values({ | ||
| id, | ||
| packId, | ||
| catalogItemId: catalog.id, | ||
| name: catalog.name, | ||
| description: catalog.description ?? null, | ||
| weight: catalog.weight ?? 0, | ||
| weightUnit: catalog.weightUnit ?? 'g', | ||
| quantity: body.quantity ?? 1, | ||
| category: body.category ?? catalog.categories?.[0] ?? 'Uncategorized', | ||
| consumable: body.consumable ?? false, | ||
| worn: body.worn ?? false, | ||
| image: catalog.images?.[0] ?? null, | ||
| notes: body.notes ?? null, | ||
| userId: user.userId, | ||
| embedding: catalog.embedding, | ||
| localCreatedAt: now, | ||
| localUpdatedAt: now, | ||
| } as NewPackItem) // safe-cast: object literal matches NewPackItem; embedding field uses the narrower type | ||
| .returning(); | ||
|
|
||
| await db.update(packs).set({ updatedAt: new Date() }).where(eq(packs.id, packId)); | ||
|
|
||
| if (!newItem) return status(400, { error: 'Failed to create item' }); | ||
|
|
||
| return status(201, { | ||
| ...newItem, | ||
| consumable: newItem.consumable ?? false, | ||
| worn: newItem.worn ?? false, | ||
| deleted: newItem.deleted ?? false, | ||
| createdAt: newItem.createdAt.toISOString(), | ||
| updatedAt: newItem.updatedAt.toISOString(), | ||
| embedding: undefined, | ||
| templateItemId: newItem.templateItemId ?? null, | ||
| }); | ||
| }, | ||
| { | ||
| params: z.object({ packId: z.string() }), | ||
| body: AddPackItemFromCatalogSchema, | ||
| isAuthenticated: true, | ||
| detail: { | ||
| tags: ['Pack Items'], | ||
| summary: 'Add a catalog item to a pack', | ||
| security: [{ bearerAuth: [] }], | ||
| }, | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract from-catalog creation workflow into a service.
This handler now owns hydration, defaults, persistence mapping, and response shaping. Move it into packages/api/src/services/ and keep the route thin to reduce duplication and drift with other item-creation paths.
As per coding guidelines, "Validation schemas live in src/schemas/. Business logic belongs in src/services/, not in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/packs/index.ts` around lines 724 - 791, The route
handler is doing hydration, defaults, persistence mapping and response shaping;
extract this into a service function (e.g., createPackItemFromCatalog or
AddPackItemFromCatalogService) that accepts (db, user, packId, body), performs
pack and catalog lookups, builds the NewPackItem payload (apply defaults for
quantity/weight/weightUnit/category/consumable/worn/image/notes), inserts the
pack item, updates packs.updatedAt, and returns the fully shaped response object
(with ISO timestamps, boolean defaults, embedding omitted, templateItemId
normalized); replace the current inline logic in the route with a thin call to
that service and return its status/result.
Comment on lines
+101
to
+109
| parent_comment_id: z.number().int().optional(), | ||
| }, | ||
| }, | ||
| async ({ post_id, content, parent_comment_id }) => | ||
| call( | ||
| agent.api.user.feed({ postId: post_id }).comments.post({ | ||
| content, | ||
| parentCommentId: parent_comment_id, | ||
| }), |
Contributor
There was a problem hiding this comment.
Use a consistent comment ID type for reply linking.
Line 101 defines parent_comment_id as number, but Line 118 treats comment_id as string. This type
mismatch can break reply creation when IDs are string-based.
Proposed fix
inputSchema: {
post_id: z.string(),
content: z.string().min(1),
- parent_comment_id: z.number().int().optional(),
+ parent_comment_id: z.string().optional(),
},Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcp/src/tools/feed.ts` around lines 101 - 109, The schema defines
parent_comment_id as z.number().int().optional() but the API call uses string
comment IDs elsewhere (e.g., comment_id), causing a type mismatch; update the
input schema in the async handler to use z.string().optional() for
parent_comment_id and ensure the call to agent.api.user.feed(...).comments.post
passes parentCommentId as that string (and adjust any related
validation/consumers expecting numeric IDs to accept strings) so reply linking
uses a consistent string ID type (refer to parent_comment_id and comment_id).
Comment on lines
+218
to
+221
| 'Generate a pack template from a TikTok or YouTube link. The server gates this on `user.role === "ADMIN"` on the OAuth-authenticated user — the admin_login JWT does NOT authorize this call. Your signed-in PackRat account must be an admin.', | ||
| inputSchema: { | ||
| content_url: z.string().url(), | ||
| is_app_template: z.boolean().default(false), |
Contributor
There was a problem hiding this comment.
Restrict content_url to supported domains.
Line 218 says TikTok/YouTube only, but Line 220 accepts any URL. Add hostname validation to align with
the tool contract and reduce risky/unnecessary upstream fetches.
Proposed fix
inputSchema: {
- content_url: z.string().url(),
+ content_url: z
+ .string()
+ .url()
+ .refine((raw) => {
+ const host = new URL(raw).hostname.replace(/^www\./, '');
+ return (
+ host === 'tiktok.com' ||
+ host.endsWith('.tiktok.com') ||
+ host === 'youtube.com' ||
+ host.endsWith('.youtube.com') ||
+ host === 'youtu.be'
+ );
+ }, 'Only TikTok or YouTube URLs are supported'),
is_app_template: z.boolean().default(false),
},📝 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.
Suggested change
| 'Generate a pack template from a TikTok or YouTube link. The server gates this on `user.role === "ADMIN"` on the OAuth-authenticated user — the admin_login JWT does NOT authorize this call. Your signed-in PackRat account must be an admin.', | |
| inputSchema: { | |
| content_url: z.string().url(), | |
| is_app_template: z.boolean().default(false), | |
| 'Generate a pack template from a TikTok or YouTube link. The server gates this on `user.role === "ADMIN"` on the OAuth-authenticated user — the admin_login JWT does NOT authorize this call. Your signed-in PackRat account must be an admin.', | |
| inputSchema: { | |
| content_url: z | |
| .string() | |
| .url() | |
| .refine((raw) => { | |
| const host = new URL(raw).hostname.replace(/^www\./, ''); | |
| return ( | |
| host === 'tiktok.com' || | |
| host.endsWith('.tiktok.com') || | |
| host === 'youtube.com' || | |
| host.endsWith('.youtube.com') || | |
| host === 'youtu.be' | |
| ); | |
| }, 'Only TikTok or YouTube URLs are supported'), | |
| is_app_template: z.boolean().default(false), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcp/src/tools/packTemplates.ts` around lines 218 - 221, The
content_url field in the inputSchema currently accepts any URL but must be
restricted to supported hostnames (TikTok/YouTube) to match the tool contract;
update the inputSchema's content_url validator (the symbol "content_url" inside
"inputSchema" in packTemplates.ts) to replace or chain the z.string().url() with
a hostname whitelist check (e.g., using URL parsing and zod
.refine/.superRefine) that only allows hostnames like youtube.com,
www.youtube.com, youtu.be, tiktok.com, and www.tiktok.com; return a clear
validation message on failure so upstream handlers can reject unsupported
domains before fetching.
Comment on lines
+11
to
+18
| inputSchema: { | ||
| file_name: z.string().min(1), | ||
| content_type: z.string().min(1), | ||
| size: z | ||
| .number() | ||
| .int() | ||
| .min(1) | ||
| .max(10 * 1024 * 1024), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider validating content_type to match documented image types.
Description says "jpeg/png/webp" but schema accepts any non-empty string. Server likely validates, but adding client-side validation catches bad input earlier:
Suggested improvement
inputSchema: {
file_name: z.string().min(1),
- content_type: z.string().min(1),
+ content_type: z.enum(['image/jpeg', 'image/png', 'image/webp']),
size: z
.number()📝 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.
Suggested change
| inputSchema: { | |
| file_name: z.string().min(1), | |
| content_type: z.string().min(1), | |
| size: z | |
| .number() | |
| .int() | |
| .min(1) | |
| .max(10 * 1024 * 1024), | |
| inputSchema: { | |
| file_name: z.string().min(1), | |
| content_type: z.enum(['image/jpeg', 'image/png', 'image/webp']), | |
| size: z | |
| .number() | |
| .int() | |
| .min(1) | |
| .max(10 * 1024 * 1024), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcp/src/tools/upload.ts` around lines 11 - 18, The inputSchema
currently allows any non-empty string for content_type but the docstring
restricts uploads to jpeg/png/webp; update the validation in inputSchema to only
accept the allowed MIME types (e.g. image/jpeg, image/png, image/webp) instead
of z.string().min(1). Locate the content_type entry inside inputSchema and
replace the loose string check with a strict enum/union of literals so invalid
content types are rejected alongside the existing file_name and size
validations.
Comment on lines
+29
to
+34
| const body: Record<string, unknown> = {}; | ||
| if (first_name !== undefined) body.firstName = first_name; | ||
| if (last_name !== undefined) body.lastName = last_name; | ||
| if (email !== undefined) body.email = email; | ||
| if (avatar_url !== undefined) body.avatarUrl = avatar_url; | ||
| return call(agent.api.user.user.profile.put(body), { action: 'update profile' }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Strengthen type safety with an explicit interface.
Using Record<string, unknown> works but loses compile-time type checking. An explicit typed interface matching the API schema would catch field name typos and provide better IDE support.
🔒 Proposed type-safe alternative
- async ({ first_name, last_name, email, avatar_url }) => {
- const body: Record<string, unknown> = {};
- if (first_name !== undefined) body.firstName = first_name;
- if (last_name !== undefined) body.lastName = last_name;
- if (email !== undefined) body.email = email;
- if (avatar_url !== undefined) body.avatarUrl = avatar_url;
+ async ({ first_name, last_name, email, avatar_url }) => {
+ const body: {
+ firstName?: string;
+ lastName?: string;
+ email?: string;
+ avatarUrl?: string;
+ } = {};
+ if (first_name !== undefined) body.firstName = first_name;
+ if (last_name !== undefined) body.lastName = last_name;
+ if (email !== undefined) body.email = email;
+ if (avatar_url !== undefined) body.avatarUrl = avatar_url;
return call(agent.api.user.user.profile.put(body), { action: 'update profile' });
},📝 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.
Suggested change
| const body: Record<string, unknown> = {}; | |
| if (first_name !== undefined) body.firstName = first_name; | |
| if (last_name !== undefined) body.lastName = last_name; | |
| if (email !== undefined) body.email = email; | |
| if (avatar_url !== undefined) body.avatarUrl = avatar_url; | |
| return call(agent.api.user.user.profile.put(body), { action: 'update profile' }); | |
| const body: { | |
| firstName?: string; | |
| lastName?: string; | |
| email?: string; | |
| avatarUrl?: string; | |
| } = {}; | |
| if (first_name !== undefined) body.firstName = first_name; | |
| if (last_name !== undefined) body.lastName = last_name; | |
| if (email !== undefined) body.email = email; | |
| if (avatar_url !== undefined) body.avatarUrl = avatar_url; | |
| return call(agent.api.user.user.profile.put(body), { action: 'update profile' }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcp/src/tools/user.ts` around lines 29 - 34, Replace the loose
Record<string, unknown> with a concrete interface (e.g.,
UpdateUserProfileRequest) that declares optional typed properties firstName?:
string, lastName?: string, email?: string, avatarUrl?: string, then type the
body variable as that interface and keep the same mapping from incoming
snake_case params to camelCase fields before calling
call(agent.api.user.user.profile.put(body), { action: 'update profile' }); this
will restore compile-time checking and IDE autocomplete for the request payload.
Worked through the 35 inline findings. Skipped the pure refactor
suggestions (move logic into services) and the cosmetic nits; this commit
covers the real bugs and validation-tightening that change behavior.
Server (api):
- catalog/compare: dedupe-then-floor — [1,1] passed `.min(2)` but
collapsed to one unique row; now rejected with a clear 400.
- packs/items/from-catalog: catalog row may have `name === null`,
which would surface as Postgres 23502. Validate up front and return
422 with the catalog ID.
- weather/by-name: remove unreachable `q.length < 2` check (schema
already enforces it).
- compute-pack `itemCount`: top-level was row count, byCategory was
quantity-aware. Make top-level quantity-aware so a pack with `qty:3`
of a single row reports 3.
- trips/CreateTripRequestSchema.id, trail-conditions report `id`:
`z.string().optional()` accepted ''/whitespace; tighten to
`.trim().min(1).optional()` so bad values fail validation instead of
slipping past `?? mintId(…)`.
CLI:
- catalog semantic: was rendering `it.score`; the API field is
`similarity`.
- catalog get: was rendering `r.ratingCount`; catalog has `reviewCount`
(a distinct column).
- packs gap-analysis: validate `--duration` parses to a positive
integer before posting (otherwise NaN → API 422).
MCP:
- admin packs/trails list: was passing `includeDeleted: 1 | 0`; the
API now uses the strict `queryBoolean()` parser so the typed-client
contract is boolean.
- index.ts: when an admin JWT arrives via the `X-PackRat-Admin-Token`
header (rather than the `admin_login` tool), `syncAdminToolVisibility`
wasn't called, so admin tools stayed hidden. Mirror the setAdminToken
flow here.
- update_pack_template_item input schema was missing `image`, so image
changes were impossible post-create. Add it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
CodeRabbit response — addressed in Worked through the 35 inline comments and applied fixes for the real bugs + validation-tightening, skipping pure refactor suggestions (service-layer extractions) and stylistic nits. Real bugs fixed:
Validation tightening:
Deliberately not done in this PR (out of scope for review-response, would belong in follow-ups):
🤖 Generated with Claude Code |
andrew-bierman
pushed a commit
that referenced
this pull request
May 17, 2026
Eden Treaty types `.optional().default(N)` as required-with-default, forcing typed clients to pass these fields. Same UX problem T1 fixed for query schemas in PR #2433; same treatment now for body schemas. schemas/imageDetection: matchLimit (handler uses service default = 3) schemas/packs: isPublic (handler now `?? false`), consumable / worn on CreatePackItemRequestSchema (handler already used `data.foo || false`) schemas/packTemplates: quantity, consumable, worn on CreatePackTemplateItemRequestSchema (handler now applies `?? false` for consumable/worn; quantity already had `|| 1`); isAppTemplate on GenerateFromOnlineContentRequestSchema (handler already uses `isAppTemplate ?? true`) routes/trailConditions/reports: hazards, waterCrossings, photos — handler already uses `?? []` / `?? 0` `localCreatedAt` / `localUpdatedAt` defaults stay required (offline-first sync semantics). Follow-up to PR #2433. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andrew-bierman
pushed a commit
that referenced
this pull request
May 17, 2026
Architectural design doc proposing a clean split between server-owned `id` (real PK) and client-owned `clientUuid` (idempotency key for offline-first sync). Walks the codebase to ground the design in current schema, route handlers, and Legend State stores. Covers schema target, migration path (recommends hybrid 2-phase rollout), sync plugin rewire, API surface, FK contract during offline window, rollback story, open questions, and a 6-PR implementation plan. Design only — no code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged url.includes('fonts.googleapis.com') as incomplete URL
substring sanitization. Parse URL and check hostname exact-match instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CF Pages' default bun didn't interpolate $PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN in bunfig.toml scopes, causing a 403 on @packrat-ai/nativewindui during the preinstall. Pinning bun to a version that supports env-var interpolation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli/src/commands/packs/gap-analysis.ts (1)
29-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject partial numeric strings for
--duration.Line 29 uses
Number.parseInt, so values like"3abc"pass as3. That bypasses the intended input validation.Proposed fix
- const duration = Number.parseInt(args.duration, 10); + const duration = Number(args.duration.trim()); if (!Number.isInteger(duration) || duration < 1) { consola.error(`Invalid --duration "${args.duration}" — must be a positive integer (days).`); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/packs/gap-analysis.ts` around lines 29 - 31, The current parsing of --duration uses Number.parseInt on args.duration which accepts partial numeric strings like "3abc"; update the validation to reject non-pure-numeric input by first validating args.duration with a strict numeric check (e.g., /^\d+$/) before converting, then parse to an integer (assign to the duration variable) and keep the existing Number.isInteger(duration) && duration >= 1 check; target the duration variable and args.duration usage in the packs/gap-analysis.ts command handler to implement this fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/routes/weather.ts`:
- Around line 186-199: Add explicit timeouts to the two WeatherAPI fetches by
passing an AbortSignal created with AbortSignal.timeout(8000) in each fetch
options; update the fetch calls that produce searchResponse and forecastResponse
(the fetches using WEATHER_API_BASE_URL, WEATHER_API_KEY and q / first.id) to
include { signal: AbortSignal.timeout(8000) } so slow/upstream hangs are
bounded—keep existing error handling which will catch AbortError.
In `@packages/cli/src/commands/catalog/index.ts`:
- Around line 19-20: The parsed numeric CLI args (Number.parseInt(args.limit,
10) and Number.parseInt(args.page, 10), and the other parses at lines ~55-57)
can be NaN for invalid input; add validation after each parse (e.g., const limit
= Number.parseInt(...); if (Number.isNaN(limit) || limit <= 0) { throw new
Error(`Invalid --limit value: ${args.limit}`); } and similarly validate page) so
the command fails fast with a clear message rather than sending NaN to the API;
update the same validation for the other parsed params referenced in the diff.
---
Duplicate comments:
In `@packages/cli/src/commands/packs/gap-analysis.ts`:
- Around line 29-31: The current parsing of --duration uses Number.parseInt on
args.duration which accepts partial numeric strings like "3abc"; update the
validation to reject non-pure-numeric input by first validating args.duration
with a strict numeric check (e.g., /^\d+$/) before converting, then parse to an
integer (assign to the duration variable) and keep the existing
Number.isInteger(duration) && duration >= 1 check; target the duration variable
and args.duration usage in the packs/gap-analysis.ts command handler to
implement this fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1fe4cf3-e8da-4cab-ad4a-f538324bd7c3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (16)
.bun-versionapps/guides/scripts/generate-og-images.tsapps/landing/lib/og-image.tsxapps/landing/scripts/generate-og-images.tspackage.jsonpackages/api/src/routes/catalog/index.tspackages/api/src/routes/packs/index.tspackages/api/src/routes/trailConditions/reports.tspackages/api/src/routes/trips/index.tspackages/api/src/routes/weather.tspackages/api/src/utils/compute-pack.tspackages/cli/src/commands/catalog/index.tspackages/cli/src/commands/packs/gap-analysis.tspackages/mcp/src/index.tspackages/mcp/src/tools/admin.tspackages/mcp/src/tools/packTemplates.ts
Comment on lines
+186
to
+199
| const searchResponse = await fetch( | ||
| `${WEATHER_API_BASE_URL}/search.json?key=${WEATHER_API_KEY}&q=${encodeURIComponent(q)}`, | ||
| ); | ||
| if (!searchResponse.ok) throw new Error(`API error: ${searchResponse.status}`); | ||
| const matches = (await searchResponse.json()) as WeatherAPISearchResponse; // safe-cast: WeatherAPI.com response shape matches this type | ||
| const first = Array.isArray(matches) ? matches[0] : null; | ||
| if (!first) { | ||
| return status(404, { error: `No weather location matched "${q}"` }); | ||
| } | ||
| const forecastResponse = await fetch( | ||
| `${WEATHER_API_BASE_URL}/forecast.json?key=${WEATHER_API_KEY}&q=${encodeURIComponent(`id:${first.id}`)}&days=10&aqi=yes&alerts=yes`, | ||
| ); | ||
| if (!forecastResponse.ok) throw new Error(`API error: ${forecastResponse.status}`); | ||
| const data = (await forecastResponse.json()) as WeatherAPIForecastResponse; // safe-cast: WeatherAPI.com response shape matches this type |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cloudflare Workers, what is the recommended way to enforce timeouts for fetch requests (e.g., AbortController vs AbortSignal.timeout), and are there any runtime caveats?
💡 Result:
In Cloudflare Workers, the recommended way to enforce timeouts for fetch requests is to use the standard AbortSignal.timeout(ms) API [1][2][3]. This approach is concise, standards-compliant, and natively supported by the Workers runtime [2]. Recommended Implementation: You can pass the signal directly into the fetch options: async function fetchWithTimeout(url, timeout = 5000) { return await fetch(url, { signal: AbortSignal.timeout(timeout) }); } Comparison: - AbortSignal.timeout(ms): This is the preferred, modern method. It automatically handles the creation of the controller and the timer, reducing boilerplate code and potential for memory leaks [4]. - AbortController: While still functional, this requires manual management of a setTimeout to trigger the abort method and subsequent cleanup with clearTimeout [5]. This is more verbose and prone to errors if cleanup is missed [4]. Runtime Caveats: - Exception Handling: When a request times out, it throws an AbortError (or a TimeoutError depending on the environment/implementation) [5][4]. You must wrap your fetch call in a try/catch block to handle this gracefully [5][4]. - Unhandled Exceptions: In some versions of the Workers runtime, calling abortController.abort(reason) with an explicit reason can trigger an unhandled exception notice in the logs that may not be catchable by user code [6]. Using AbortSignal.timeout(ms) is generally safer, though you should still be aware of how your specific runtime version handles these rejections [6]. - Resource Cleanup: Aborting a request stops the fetch operation and frees up resources, but it does not necessarily stop the server from processing the request if it has already received it [1]. Always ensure your application logic accounts for the fact that the remote server may have already performed side effects [1]. - Single-Use Signals: AbortSignals are single-use [2]. If you need to combine multiple signals (e.g., a timeout and a user-cancellation signal), use AbortSignal.any([signal1, signal2]) to compose them correctly [4].
Citations:
- 1: https://community.cloudflare.com/t/timeout-with-fetch/25249
- 2: https://blog.cloudflare.com/standards-compliant-workers-api/
- 3: https://community.cloudflare.com/t/2021-10-14-workers-runtime-release-notes/315833
- 4: https://dev.to/solvodevnotes/abortcontroller-the-cancellation-bugs-most-javascript-devs-ship-34kn
- 5: https://app.studyraid.com/en/read/14352/488213/managing-timeouts-and-error-handling-for-external-calls
- 6: 🐛 Bug Report — Runtime APIs — AbortSignal.timeout throws un-catchable async DOMException cloudflare/workerd#1020
Add explicit timeouts for upstream WeatherAPI calls.
Both fetch calls lack timeouts, allowing slow or unresponsive upstreams to hang request handling and cascade latency. Use AbortSignal.timeout() on each fetch to enforce a deadline:
const searchResponse = await fetch(url, { signal: AbortSignal.timeout(8000) });This is the standard, supported approach in Cloudflare Workers. Timeout errors will be caught by existing error handling (ensure parent route handlers catch AbortError).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/weather.ts` around lines 186 - 199, Add explicit
timeouts to the two WeatherAPI fetches by passing an AbortSignal created with
AbortSignal.timeout(8000) in each fetch options; update the fetch calls that
produce searchResponse and forecastResponse (the fetches using
WEATHER_API_BASE_URL, WEATHER_API_KEY and q / first.id) to include { signal:
AbortSignal.timeout(8000) } so slow/upstream hangs are bounded—keep existing
error handling which will catch AbortError.
Comment on lines
+19
to
+20
| const limit = Number.parseInt(args.limit, 10); | ||
| const page = Number.parseInt(args.page, 10); |
Contributor
There was a problem hiding this comment.
Validate parsed numeric args before invoking API.
Number.parseInt(...) can yield NaN (for example, --limit abc), and that value is forwarded into query params. Add a guard/fallback so invalid CLI input fails fast with a clear message.
Suggested fix
- const limit = Number.parseInt(args.limit, 10);
- const page = Number.parseInt(args.page, 10);
+ const limit = Number.parseInt(args.limit, 10);
+ const page = Number.parseInt(args.page, 10);
+ if (!Number.isFinite(limit) || limit <= 0 || !Number.isFinite(page) || page <= 0) {
+ throw new Error('`limit` and `page` must be positive integers');
+ }
...
- const limit = Number.parseInt(args.limit, 10);
+ const limit = Number.parseInt(args.limit, 10);
+ if (!Number.isFinite(limit) || limit <= 0) {
+ throw new Error('`limit` must be a positive integer');
+ }Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/catalog/index.ts` around lines 19 - 20, The parsed
numeric CLI args (Number.parseInt(args.limit, 10) and Number.parseInt(args.page,
10), and the other parses at lines ~55-57) can be NaN for invalid input; add
validation after each parse (e.g., const limit = Number.parseInt(...); if
(Number.isNaN(limit) || limit <= 0) { throw new Error(`Invalid --limit value:
${args.limit}`); } and similarly validate page) so the command fails fast with a
clear message rather than sending NaN to the API; update the same validation for
the other parsed params referenced in the diff.
Collaborator
Author
|
Heads-up: this PR went CONFLICTING after #2414 (type-system unify) merged to development. Conflicts in:
These all sit at the intersection of #2414's drizzle→zod inferred-types refactor and this PR's Treaty-typed schemas. Rather than blind-resolve and risk regressing the schema patterns you've established for the MCP/CLI, leaving this for you (or your agent in the worktree) to merge cleanly. The other foundation PRs (#2422 single-param refactor, #2366 web MVP, #2304 Lighthouse) are also currently waiting on CF Pages flake to clear before merge. CI was otherwise green pre-conflict. |
…factor) Resolves conflicts from #2414 (extract @packrat/db + @packrat/schemas): - Route files: keep dev's @packrat/schemas imports, retain mintId + queryBoolean - packages/schemas/src/catalog.ts: keep my T6 CatalogCompare* schemas + dev's CatalogETLSchema - packages/schemas/src/packs.ts: apply T9 (id optional with trim/min(1)) to CreatePackBody + AddPackItemBody - packages/schemas/src/trips.ts: apply T9 to CreateTripBodySchema - packages/schemas/src/trailConditions.ts: apply T9 + T-thickening (drop .default for hazards/waterCrossings/photos) - packs/index.ts: keep AddPackItemFromCatalogSchema (T8) local, drop redundant CreatePack/AddPackItem local schemas (now in @packrat/schemas) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-push lint enforces all Zod schemas live in packages/schemas/src/. T8's inline schema moves to packs.ts as AddPackItemFromCatalogBodySchema. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-split T9 (optional id + server mintId) and T8 (POST /packs/:id/items/from-catalog) proved hacky on a single id column with dual ownership. Per design doc shipped in #2435 (docs/design/client-uuid-split.md), the cleaner pattern is two columns: server-owned `id` (always minted) + client-owned `clientUuid` (idempotency key with UNIQUE(user_id, client_uuid)). Reverting T8/T9 here so #2433 ships only T1-T6 + T10 (lean endpoints + API thickening). The optional-id work is preserved on feat/client-uuid-split for the proper migration. Removed: - packages/api/src/utils/ids.ts (mintId helper) - packages/api/src/routes/packs/index.ts: /:packId/items/from-catalog endpoint, mintId fallbacks - packages/api/src/routes/trips/index.ts: mintId fallback - packages/api/src/routes/trailConditions/reports.ts: mintId fallback, 23505-only-if-data.id branch - packages/mcp/src/tools/packs.ts: add_pack_item_from_catalog tool - packages/schemas/src/packs.ts: AddPackItemFromCatalogBodySchema - packages/schemas/{trips,packs,trailConditions}.ts: id back to z.string() (required) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andrew-bierman
added a commit
that referenced
this pull request
May 17, 2026
) * 🚸 api: drop redundant .optional().default() from POST body schemas Eden Treaty types `.optional().default(N)` as required-with-default, forcing typed clients to pass these fields. Same UX problem T1 fixed for query schemas in PR #2433; same treatment now for body schemas. schemas/imageDetection: matchLimit (handler uses service default = 3) schemas/packs: isPublic (handler now `?? false`), consumable / worn on CreatePackItemRequestSchema (handler already used `data.foo || false`) schemas/packTemplates: quantity, consumable, worn on CreatePackTemplateItemRequestSchema (handler now applies `?? false` for consumable/worn; quantity already had `|| 1`); isAppTemplate on GenerateFromOnlineContentRequestSchema (handler already uses `isAppTemplate ?? true`) routes/trailConditions/reports: hazards, waterCrossings, photos — handler already uses `?? []` / `?? 0` `localCreatedAt` / `localUpdatedAt` defaults stay required (offline-first sync semantics). Follow-up to PR #2433. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ⬆️ chore: pin bun 1.3.14 via .bun-version + packageManager + engines CF Pages' default bun didn't interpolate $PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN in bunfig.toml scopes, causing a 403 on @packrat-ai/nativewindui during the preinstall. Pinning bun to a version that supports env-var interpolation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Andrew Bierman <andrewbierman@andrews-mini.tailb2708f.ts.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- schemas/packTemplates.ts: revert pack template + item id to required (T9 leftover)
- schemas/admin.ts: drop .optional().default() on AnalyticsPeriodSchema (T1 thickening — keeps Treaty types optional, handler defaults stay)
- api/routes/packs/index.ts: weight-history body uses shared CreatePackWeightHistoryBodySchema (id required, no inline schema lint violation)
- api/routes/admin/trails.ts: includeDeleted via queryBoolean() so CLI/admin SPA can pass boolean; also drop limit/offset defaults
- cli/api/ids.ts: switch shortId from truncated UUIDv4 hex to Bun.randomUUIDv7 — time-ordered, full 128-bit, better B-tree locality
- cli/commands/{packs/create,trips/index,templates/index}.ts: mint client-side id via shortId() so create calls satisfy the required-id schemas
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/routes/catalog/index.ts`:
- Around line 173-191: The current rank helper and its use for lightestId
incorrectly compares raw weight values across differing units; modify the logic
so that before ranking by weight you normalize each item's weight to a common
unit (e.g., grams) using the item's weightUnit, converting values (kg→g, oz→g,
lb→g, etc.), filter out null/NaN weights, and then select the id of the min
normalized weight; you can either add a normalizeWeight(item) helper and use it
inside rank when key === 'weight' or compute lightestId with a dedicated flow
that maps items to {id, normalizedWeight} and picks the smallest id, ensuring
you still return null when no valid weights exist.
In `@packages/mcp/src/tools/packs.ts`:
- Around line 64-74: The request body sent to agent.api.user.packs.post from the
packs creation code is missing the required id field (CreatePackBodySchema
expects id, localCreatedAt, localUpdatedAt), so generate a UUID (e.g., via
crypto.randomUUID()) before calling nowIso() and include that id in the object
passed to call(agent.api.user.packs.post(...)); ensure the id property is added
alongside name, description, category, isPublic/is_public, tags, localCreatedAt,
and localUpdatedAt so validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37891332-18ac-47b4-ad55-956f1eebca9e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (22)
apps/admin/lib/api.tspackages/api/src/routes/admin/analytics/catalog.tspackages/api/src/routes/admin/index.tspackages/api/src/routes/admin/trails.tspackages/api/src/routes/catalog/index.tspackages/api/src/routes/feed/index.tspackages/api/src/routes/guides/index.tspackages/api/src/routes/packs/index.tspackages/api/src/routes/trailConditions/reports.tspackages/api/src/routes/weather.tspackages/api/src/utils/compute-pack.tspackages/cli/src/api/ids.tspackages/cli/src/commands/packs/create.tspackages/cli/src/commands/templates/index.tspackages/cli/src/commands/trips/index.tspackages/mcp/src/tools/packs.tspackages/schemas/src/admin.tspackages/schemas/src/ai.tspackages/schemas/src/catalog.tspackages/schemas/src/guides.tspackages/schemas/src/packs.tspackages/schemas/src/weather.ts
Comment on lines
+173
to
+191
| const rank = <K extends keyof (typeof items)[number]>( | ||
| key: K, | ||
| order: 'asc' | 'desc', | ||
| ): number | null => { | ||
| const ranked = [...items] | ||
| .filter((it) => it[key] != null) | ||
| .sort((a, b) => { | ||
| const av = Number(a[key]); | ||
| const bv = Number(b[key]); | ||
| return order === 'asc' ? av - bv : bv - av; | ||
| }); | ||
| return ranked[0]?.id ?? null; | ||
| }; | ||
|
|
||
| return { | ||
| items, | ||
| lightestId: rank('weight', 'asc'), | ||
| cheapestId: rank('price', 'asc'), | ||
| highestRatedId: rank('ratingValue', 'desc'), |
Contributor
There was a problem hiding this comment.
Normalize weight units before computing lightestId.
Line 189 ranks by raw weight values, which is incorrect when compared items use different
weightUnit values (e.g., kg vs g vs oz). This can return the wrong “lightest” item.
Proposed fix
- const rank = <K extends keyof (typeof items)[number]>(
- key: K,
- order: 'asc' | 'desc',
- ): number | null => {
- const ranked = [...items]
- .filter((it) => it[key] != null)
- .sort((a, b) => {
- const av = Number(a[key]);
- const bv = Number(b[key]);
- return order === 'asc' ? av - bv : bv - av;
- });
- return ranked[0]?.id ?? null;
- };
+ const toGrams = (weight: unknown, unit: unknown): number | null => {
+ const w = Number(weight);
+ if (!Number.isFinite(w)) return null;
+ switch (unit) {
+ case 'kg':
+ return w * 1000;
+ case 'lb':
+ return w * 453.59237;
+ case 'oz':
+ return w * 28.349523125;
+ case 'g':
+ default:
+ return w;
+ }
+ };
+
+ const rankNumeric = (
+ getValue: (item: (typeof items)[number]) => number | null,
+ order: 'asc' | 'desc',
+ ): number | null => {
+ const ranked = [...items]
+ .map((item) => ({ id: item.id, value: getValue(item) }))
+ .filter((x): x is { id: number; value: number } => x.value != null)
+ .sort((a, b) => (order === 'asc' ? a.value - b.value : b.value - a.value));
+ return ranked[0]?.id ?? null;
+ };
return {
items,
- lightestId: rank('weight', 'asc'),
- cheapestId: rank('price', 'asc'),
- highestRatedId: rank('ratingValue', 'desc'),
+ lightestId: rankNumeric((it) => toGrams(it.weight, it.weightUnit), 'asc'),
+ cheapestId: rankNumeric((it) => {
+ const n = Number(it.price);
+ return Number.isFinite(n) ? n : null;
+ }, 'asc'),
+ highestRatedId: rankNumeric((it) => {
+ const n = Number(it.ratingValue);
+ return Number.isFinite(n) ? n : null;
+ }, 'desc'),
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/catalog/index.ts` around lines 173 - 191, The current
rank helper and its use for lightestId incorrectly compares raw weight values
across differing units; modify the logic so that before ranking by weight you
normalize each item's weight to a common unit (e.g., grams) using the item's
weightUnit, converting values (kg→g, oz→g, lb→g, etc.), filter out null/NaN
weights, and then select the id of the min normalized weight; you can either add
a normalizeWeight(item) helper and use it inside rank when key === 'weight' or
compute lightestId with a dedicated flow that maps items to {id,
normalizedWeight} and picks the smallest id, ensuring you still return null when
no valid weights exist.
Comment on lines
+64
to
+74
| const now = nowIso(); | ||
| return call( | ||
| agent.api.user.packs.post({ | ||
| name, | ||
| description, | ||
| category, | ||
| isPublic: is_public, | ||
| tags, | ||
| localCreatedAt: now, | ||
| localUpdatedAt: now, | ||
| }); | ||
| return ok(data); | ||
| } catch (e) { | ||
| return err(e); | ||
| } | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check create-pack schema fields (especially id):"
rg -n --type=ts -C4 'Create.*Pack|id:|localCreatedAt|localUpdatedAt' packages/schemas/src/packs.ts
echo
echo "Check POST /packs route body schema wiring:"
rg -n --type=ts -C5 "\\.post\\(" packages/api/src/routes/packs/index.ts
rg -n --type=ts -C5 'Create.*Pack' packages/api/src/routes/packs/index.ts
echo
echo "Compare CLI and MCP create payloads:"
rg -n --type=ts -C5 'client\\.packs\\.post\\(' packages/cli/src/commands/packs/create.ts
rg -n --type=ts -C5 'agent\\.api\\.user\\.packs\\.post\\(' packages/mcp/src/tools/packs.tsRepository: PackRat-AI/PackRat
Length of output: 6212
Add missing id to MCP create_pack request body.
The CreatePackBodySchema requires id, localCreatedAt, and localUpdatedAt, but the MCP code only sends the latter two. MCP packs creation will fail validation without the id field. Generate and include a UUID:
const id = crypto.randomUUID();
const now = nowIso();
return call(
agent.api.user.packs.post({
id,
name,
description,
category,
isPublic: is_public,
tags,
localCreatedAt: now,
localUpdatedAt: now,
}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcp/src/tools/packs.ts` around lines 64 - 74, The request body sent
to agent.api.user.packs.post from the packs creation code is missing the
required id field (CreatePackBodySchema expects id, localCreatedAt,
localUpdatedAt), so generate a UUID (e.g., via crypto.randomUUID()) before
calling nowIso() and include that id in the object passed to
call(agent.api.user.packs.post(...)); ensure the id property is added alongside
name, description, category, isPublic/is_public, tags, localCreatedAt, and
localUpdatedAt so validation succeeds.
Runtime-portable: same helper works in Node, Workers, browser. Useful if this ever moves out of @packrat/cli (e.g., shared with @packrat/mcp on Cloudflare Workers, which doesn't have Bun's globals). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andrew-bierman
added a commit
that referenced
this pull request
May 17, 2026
… MCP rewrite) #2433 restructured packages/mcp/. Took dev's new MCP wholesale; applied no-owned-max-params refactor on top where dev introduced new multi-param owned functions. Refactors applied to satisfy the single-param rule: - mcp client `call(promise, opts)` → `call({ promise, ...opts })` across all 18 tool files (102 sites) - cli `runApi(promise, opts)` → `runApi({ promise, ...opts })` across 24 command files (60 sites) - cli `printTable(rows, opts)` → `printTable({ rows, options })` across 10 sites - cli `printSummary(data, title)` → `printSummary({ data, title })` across 6 sites - mcp `setFeatureFlag(flag, enabled)` → `setFeatureFlag({ flag, enabled })` - mcp `registerFlaggedTool(flag, ...args)` → `registerFlaggedTool({ flag, args })` - mcp `asContent(uri, body)` → `asContent({ uri, body })` in resources.ts - api `rank(key, order)` → `rank({ key, order })` in catalog compare route - api `checkAdminCredentials(username, password)` → object param - api `compute-pack` new `computePackBreakdown` aligned with single-param `normalize`/`parseWeightUnit` Non-MCP conflict resolutions: - apps/landing/components/sections/landing-hero.tsx — kept HEAD (PR's scrollToSection extraction) - apps/landing/components/site-footer.tsx — kept HEAD (same) - packages/api/src/routes/admin/index.ts — took dev's new /login route and queryBoolean usage, then refactored to single-param for timingSafeEqual / verifyCFAccessRequest / assertAllDefined / checkAdminCredentials - packages/api/src/utils/compute-pack.ts — kept HEAD's single-param computePacksWeights; added dev's computePackBreakdown rewritten to use single-param normalize/parseWeightUnit - packages/guards/src/narrow.ts — deduped nullToUndefined/safeIndexOf (both sides added them); kept HEAD's single-param safeIndexOf shape Two build scripts added to the no-owned-max-params allowlist because they override globalThis.fetch and must match the runtime's (input, init) signature: - apps/landing/scripts/generate-og-images.ts - apps/guides/scripts/generate-og-images.ts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andrew-bierman
added a commit
that referenced
this pull request
May 20, 2026
Substantial rebase covering 225 dev commits — #2414 type unification, #2422 single-param refactor, #2433 MCP+CLI Eden Treaty rewrite, #2439 OG meta validation, #2441/#2442 OG URL fix, plus many smaller. Conflicts resolved: - apps/expo/features/packs/utils/uploadImage.ts: kept HEAD's userId cache, used dev's object-arg getPresignedUrl call (matches function signature). - apps/expo/features/trips/hooks/useDeleteTrip.ts: kept HEAD's async + optimistic-delete comment, used dev's object-arg obs() call (matches current obs signature in apps/expo/lib/store.ts). Post-merge cleanup of dev-introduced single-param violations: - apps/expo/lib/utils/__tests__/getRelativeTime.test.ts: rewrote 3 test call sites to object args matching the refactored getRelativeTime. - packages/api/src/utils/__tests__/embeddingHelper.test.ts: rewrote 7 test call sites to object args matching the refactored getEmbeddingText; updated Parameters<> type indexes. - packages/overpass/src/client.test.ts: converted makeResponse to single object param and updated all 11 call sites. - scripts/lint/no-owned-max-params.ts: added apps/trails/scripts/generate-og-images.ts to EXCLUDED_FILES (same globalThis.fetch shim pattern as the existing landing/guides entries). Verification: bun install ok; bun check-types 0 errors; biome check 0 errors (2 unrelated warnings); no-owned-max-params 0 violations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The MCP server and CLI are now thin Eden Treaty wrappers around the PackRat API — the API stays the source of truth, edge apps stay lean. The branch landed in two phases:
Phase 1 (commits
bc5baa5–2d70fab) — migration: built the Treaty clients, ported every existing MCP tool and added 30+ new ones, added the CLI's API command tree, consolidated narrowing helpers in@packrat/guardsunderto*naming.Phase 2 (commits
1cb8d84–31a0a69) — API thickening: cleaned up Treaty-hostile schema patterns, added five lean endpoints that collapse multi-step / client-side-compute flows into single Treaty calls, madeidoptional on create endpoints (offline-first preserved), and added a body-credential admin login so MCP/CLI no longer bypass Treaty for admin auth.What landed
Phase 1 — MCP + CLI on Eden Treaty
api.user,api.admin). 60+ tools — packs, pack-templates, catalog, trips, weather, trail-conditions, trails, feed, season-suggestions, wildlife, alltrails, upload, guides, ai, plus admin (stats / users / packs / catalog / trails / analytics / ETL). Admin tools hidden untiladmin_loginmints a JWT;registerFlaggedTool(flag, …)does the same forMCP_FEATURE_FLAGStoggles. ACL-awarecall()maps 401/403/404/409/422/429 to friendly tool errors.auth/admin/packs/trips/catalog/trails/weather/feed/templates/seasons/user/aicommand trees alongside DuckDB analytics. Token store in~/.packrat/config.json(0600, atomic write); base URL override viaPACKRAT_API_URL. 401 → refresh → retry viacreateApiClient's AuthHooks.@packrat/guards:toString/toNumber/toBoolean/toDate(strict narrow) +toArray/toRecord/toRecordArray/toStringRecord(coercive). Legacyas*kept as@deprecatedaliases.Phase 2 — API thickening (10 proposals, all in)
1cb8d84Schemas T1+T2+T3 — Dropped redundant.optional().default(N)from every query schema where handlers already applied defaults (catalog, guides, ai, admin analytics platform/catalog, admin trails, admin packs/users lists, feed, packsincludePublic, trail-conditions). Treaty now types these as truly optional. Wire upincludeDeletedon admin packs-list (was schema-declared but unread), drop deadincludeDeletedfrom admin users-list (Better Auth doesn't support user soft-delete), switch remainingincludeDeletedtoz.coerce.boolean(), coercedurationin gap-analysis to number.5d8ab30Edge cleanup T4 — CLI/MCP drop the workarounds the schema changes obsoleted.ffc929eT5GET /weather/by-name?q=…— Combined search + forecast in one call.4e79c18T6GET /packs/:packId/weight-breakdown— Server-computed total/base/worn/consumable grams + per-category aggregation.2fcc4c4T7POST /catalog/compare—{ ids: number[] }→ comparison rows + lightestId/cheapestId/highestRatedId leaders, single SQL query.089af14T8POST /packs/:packId/items/from-catalog— Server hydrates name/weight/category/image/embedding from the catalog row; caller supplies catalogItemId + per-pack metadata.df0cfacT9 Server-side ID minting — Every create endpoint accepts an optionalid; server mints withmintId(prefix)when absent. Offline-first stores (mobile, Legend State) keep supplying client-side IDs for sync. (Tradeoff noted below.)31a0a69T10POST /admin/login— Body-credential variant of/admin/token. MCP/CLI no longer bypass Treaty for admin auth; both go through the typed Treaty client.On T9 (offline-first + ID ownership)
T9 ships the pragmatic interim: one
idfield, client- or server-supplied. Mobile/Legend State stores keep working unchanged. The cleaner long-term design splits ownership — server-ownedid(real PK, auto-generated) + client-ownedclientUuid(idempotency + sync key). That's a bigger lift (schema migration, sync-plugin rewiring across mobile/web) and deserves its own brainstorm + plan. Tracked as a follow-up.Bugs caught while migrating
update_tripwas PATCHing but the API expects PUT (silent 404).get_weatherindexedsearchResults.id/.results[0].id; the API returns a flat array → should be[0].id. (Now obsolete —get_weathercalls the new combined endpoint.)get_season_suggestionsposted{ destination }; API requires{ location, date }.parent_comment_id: string; API body isz.number().int().'sort[field]'/'sort[order]'bracket notation; the catalog schema preprocessessortfrom a JSON object. The bracket form was silently ignored.includeDeletedin the schema but the handler never read it. Now wired up correctly on packs-list (admins can see soft-deleted packs); dropped from users-list where Better Auth doesn't support user soft-delete.Test plan
bun check:casts:strict— no unsafe castspackages/clitypecheckspackrat --help,packrat <group> --help, unauthedpackrat packs list→ friendly auth hint, unauthedpackrat admin stats→ admin hintpackages/apitypecheck — running locally hung; needs CItools/listFollow-ups (out of scope for this PR)
clientUuidsplit — proper server-ownedid+ client-ownedclientUuid(the cleaner T9). Schema migration + Legend State plugin rewire..optional().default(N)cleanup for POST bodies (packs.consumable/worn, packTemplates.quantity, etc.). T1 handled query schemas; bodies have a slightly different cost/benefit and deserve their own pass.shortIdhelpers in CLI/MCP — exported but no internal callers now. Safe to remove if nothing external uses them.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes