Conversation
…e:crypto in browser bundle
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an interactive CLI init wizard, schema sync/status/push enhancements with local schema-state pinning and draft concurrency, CLI prompter and scanning/inference utilities, server project/environment APIs, schema-hash utilities in shared packages, and multiple tests and DB migration to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "mdcms init"
participant Server as "API Server"
participant FS as "File System"
User->>CLI: run `mdcms init`
CLI->>User: prompt (server URL, login, project/env)
CLI->>Server: GET /healthz
Server-->>CLI: OK
CLI->>Server: POST /api/v1/auth/cli/login/start
Server-->>CLI: challenge
User->>Server: complete browser auth
Server-->>CLI: token / exchange → API key
CLI->>Server: GET /api/v1/projects
Server-->>CLI: projects list
CLI->>User: prompt select/create project
alt create
CLI->>Server: POST /api/v1/projects
Server-->>CLI: created
end
CLI->>FS: scan content files
FS-->>CLI: discovered files
CLI->>User: prompt directories / locale confirmations
CLI->>Server: PUT /api/v1/schema (schema sync)
Server-->>CLI: { schemaHash, affectedTypes }
CLI->>FS: write .mdcms/schema/<proj>.<env>.json
CLI->>Server: POST /api/v1/content (import docs)
Server-->>CLI: document IDs
CLI->>FS: write manifest
CLI-->>User: finish
sequenceDiagram
participant User
participant CLI as "mdcms push"
participant FS as "File System"
participant Server as "API Server"
participant DB as "Database"
User->>CLI: run `mdcms push`
CLI->>FS: read .mdcms/schema/<proj>.<env>.json
alt missing
CLI-->>User: exit 1 (schema state required)
else present
CLI->>FS: load manifest & content files
CLI->>CLI: validate candidates (optional --validate)
loop per candidate
CLI->>Server: POST/PUT /api/v1/content with x-mdcms-schema-hash and draftRevision
alt success
Server->>DB: apply update/create
DB-->>Server: OK
Server-->>CLI: 200 updated
CLI->>FS: update manifest
else 409 STALE_DRAFT_REVISION
Server-->>CLI: 409 { code: STALE_DRAFT_REVISION }
CLI-->>User: report stale draft (partial success)
else 409 SCHEMA_HASH_MISMATCH
Server-->>CLI: 409 { code: SCHEMA_HASH_MISMATCH }
CLI-->>User: report schema mismatch (partial success)
end
end
CLI-->>User: summary (successes/failures)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/adrs/ADR-004-project-environment-hierarchy.md (1)
5-5:⚠️ Potential issue | 🟡 MinorUpdate
last_updatedto reflect the addendum.The frontmatter shows
last_updated: 2026-03-11, but the addendum is dated 2026-04-01. Consider updating the frontmatter to keep the document metadata accurate.📝 Proposed fix
-last_updated: 2026-03-11 +last_updated: 2026-04-01🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adrs/ADR-004-project-environment-hierarchy.md` at line 5, Update the document frontmatter's last_updated field to match the addendum date by changing last_updated from 2026-03-11 to 2026-04-01; ensure the frontmatter entry named last_updated and the addendum section's date are consistent and, if present, update any other metadata fields that reference the addendum date to avoid mismatch.apps/server/src/lib/content-api/database-store.ts (1)
1037-1049:⚠️ Potential issue | 🟠 MajorGuard updates against concurrent soft-delete in the SQL predicate.
The transactional
UPDATEcan still mutate a document that was soft-deleted after the earlier read, becauseisDeleted = falseis not part of theWHEREclause.💡 Suggested fix
.where( and( eq(documents.projectId, scopeIds.projectId), eq(documents.environmentId, scopeIds.environmentId), eq(documents.documentId, normalizedDocumentId), + eq(documents.isDeleted, false), ...(options?.expectedDraftRevision !== undefined ? [ eq( documents.draftRevision, BigInt(options.expectedDraftRevision), ), ] : []), ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/content-api/database-store.ts` around lines 1037 - 1049, The SQL UPDATE predicate is missing a guard against concurrent soft-deletes: add a condition checking documents.isDeleted is false inside the and(...) predicate that builds the WHERE (the same place using scopeIds.projectId, scopeIds.environmentId, normalizedDocumentId and the optional expectedDraftRevision). Ensure the transactional UPDATE uses this augmented predicate so the update will not affect rows that were soft-deleted after the earlier read.
🧹 Nitpick comments (12)
apps/cli/src/lib/init/prompt.ts (1)
38-38: Consider making cancellation handling injectable for testability.Using
process.exit(0)directly makes it difficult to test cancellation behavior in unit tests. Consider accepting an optionalonCancelcallback or throwing a dedicatedCancellationErrorthat the caller can handle.💡 Alternative approach
+export class PromptCancelledError extends Error { + constructor() { + super("Prompt cancelled by user"); + this.name = "PromptCancelledError"; + } +} + +type PrompterOptions = { + onCancel?: () => never; +}; + +export function createClackPrompter(options?: PrompterOptions): Prompter { + const handleCancel = options?.onCancel ?? (() => process.exit(0)); // ... in each method: - if (clack.isCancel(result)) process.exit(0); + if (clack.isCancel(result)) handleCancel();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/prompt.ts` at line 38, Replace the direct call to process.exit(0) in the cancellation branch (the clack.isCancel(result) check) with injectable behavior: add an optional onCancel callback parameter (e.g., onCancel?: () => void) to the prompt function and invoke it when clack.isCancel(result) is true, or alternatively throw a dedicated CancellationError class (e.g., export class CancellationError extends Error) so callers can handle cancellation; update callers/tests to either pass an onCancel mock or catch the CancellationError instead of relying on process.exit.apps/server/src/bin/demo-seed-api-key.ts (1)
20-25: Prefer least-privilege defaults for the seeded demo key.Line 24 adds
schema:write, which broadens a fixed demo key significantly. Consider making write scope opt-in via env flag and defaulting to read-only scopes.🔐 Suggested hardening
const DEMO_KEY_SCOPES = [ "content:read", "content:read:draft", "schema:read", - "schema:write", + ...(resolveEnv("MDCMS_DEMO_ALLOW_SCHEMA_WRITE", "false") === "true" + ? ["schema:write"] + : []), ] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/bin/demo-seed-api-key.ts` around lines 20 - 25, DEMO_KEY_SCOPES currently includes "schema:write"; change the default to read-only by removing "schema:write" from the DEMO_KEY_SCOPES const and instead conditionally append that scope only when an explicit env flag is set (e.g. DEMO_KEY_ALLOW_WRITE or ENABLE_DEMO_KEY_WRITE === "true"). Update the code that builds/uses DEMO_KEY_SCOPES so it constructs the scopes array at runtime (preserving the readonly type) and only adds "schema:write" when the env flag is enabled; ensure any seeding logic that consumes DEMO_KEY_SCOPES uses the new runtime variable.packages/shared/src/lib/contracts/schema.ts (1)
770-790: Consider canonical ordering intoRawConfigSnapshotto reduce hash churn.If semantically equivalent config values arrive in different insertion orders, schema hashes can drift unnecessarily.
♻️ Suggested canonicalization
export function toRawConfigSnapshot(config: ParsedMdcmsConfig): JsonObject { + const sortedAliases = Object.fromEntries( + Object.entries(config.locales.aliases).sort(([a], [b]) => + a.localeCompare(b), + ), + ); + return { project: config.project, serverUrl: config.serverUrl, ...(config.environment ? { environment: config.environment } : {}), ...(config.contentDirectories.length > 0 - ? { contentDirectories: config.contentDirectories } + ? { contentDirectories: [...config.contentDirectories].sort() } : {}), ...(config.locales.implicit ? {} : { locales: { default: config.locales.default, - supported: config.locales.supported, - ...(Object.keys(config.locales.aliases).length > 0 - ? { aliases: config.locales.aliases } + supported: [...config.locales.supported].sort(), + ...(Object.keys(sortedAliases).length > 0 + ? { aliases: sortedAliases } : {}), }, }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/lib/contracts/schema.ts` around lines 770 - 790, The toRawConfigSnapshot function produces objects whose key/array insertion order can vary and cause schema hash churn; update toRawConfigSnapshot to build the returned JsonObject with a stable canonical ordering: always add keys in the same sequence (project, serverUrl, environment, contentDirectories, locales), sort config.contentDirectories array (e.g., lexicographically) before including it, sort config.locales.supported array, and canonicalize config.locales.aliases by creating a new object with its keys iterated in sorted order (Object.keys(...).sort()) so the resulting snapshot is deterministic even if inputs had different insertion orders.apps/cli/src/lib/init/scan.ts (1)
91-98: Consider distinguishing file system errors from parse errors.The empty catch block silently swallows all errors, including file system errors (permission denied, file busy, etc.), not just malformed frontmatter. While the comment mentions "Malformed frontmatter," file read errors would also be silently ignored.
♻️ Optional: Log or differentiate error types
let frontmatter: Record<string, unknown> = {}; try { const content = await readFile(fullPath, "utf-8"); const parsed = parseMarkdownDocument(content); frontmatter = parsed.frontmatter; - } catch { - // Malformed frontmatter — treat as empty + } catch (error) { + // Malformed frontmatter or read error — treat as empty + // Optionally: log debug info for troubleshooting }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/scan.ts` around lines 91 - 98, The current catch block around readFile(fullPath, "utf-8") and parseMarkdownDocument(...) swallows all errors (filesystem and parse); split the operations and handle them separately: first await readFile(...) in its own try/catch and rethrow or surface filesystem errors (inspect error.code like EACCES/ENOENT) instead of treating them as malformed frontmatter, then call parseMarkdownDocument(...) in a second try/catch that treats parse failures as "malformed frontmatter" and sets frontmatter = {} or logs as needed; reference the variables/functions fullPath, readFile, parseMarkdownDocument, and frontmatter when making these changes.apps/cli/src/lib/init.test.ts (1)
20-40: Mock fetcher uses pattern matching viaincludes- be aware of potential false matches.The pattern matching approach is simple but could match unintended URLs (e.g.,
/healthzwould match/api/v1/healthzand/other/healthz). For these tests this is acceptable, but consider exact matching or path prefix checks for more complex scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init.test.ts` around lines 20 - 40, The mock fetcher createMockFetcher currently matches handlers using url.includes(pattern), which can produce false positives; update the matching logic in createMockFetcher to use stricter matching (for example compare exact URL strings, use startsWith for path prefixes, or parse the input into a URL and compare url.pathname or host+pathname) and ensure handlers keys are interpreted accordingly so tests only match intended endpoints (adjust tests/handler keys if you choose pathname-only matching).apps/cli/src/lib/init/detect-locale.test.ts (1)
87-94: Test validates input mutation - consider documenting this behavior.The test asserts that
detectLocaleConfigmutates the inputtypesarray by settinglocalized: true. While the test correctly validates this behavior, mutating input parameters can be surprising.If this is intentional, consider adding a brief comment in the implementation noting that
typesis modified in-place, or document this in the function's JSDoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/detect-locale.test.ts` around lines 87 - 94, The test demonstrates that detectLocaleConfig(mutates the input types array) by setting types[0].localized = true; make this explicit in the implementation by adding a short in-code comment and/or updating the function JSDoc for detectLocaleConfig to state that it modifies the passed-in types array in-place (mentioning the localized property is set), so callers are aware; reference detectLocaleConfig and the types parameter (and makeType usage) when adding the note.apps/cli/src/lib/schema-state.ts (1)
33-34: Consider validating the parsed JSON structure.The type assertion
as SchemaStateFileassumes the parsed JSON matches the expected shape. If the file contains valid JSON but with an incorrect structure (e.g., missingschemaHash), this could cause runtime errors in calling code that expects those properties to exist.Consider using Zod or a simple property check to validate the structure before returning.
💡 Optional validation approach
+function isSchemaStateFile(value: unknown): value is SchemaStateFile { + return ( + typeof value === "object" && + value !== null && + "schemaHash" in value && + typeof (value as SchemaStateFile).schemaHash === "string" + ); +} + export async function readSchemaState( scope: SchemaStateScope, ): Promise<SchemaStateFile | undefined> { const path = resolveSchemaStatePath(scope); let raw: string; try { raw = await readFile(path, "utf8"); } catch { return undefined; } try { - return JSON.parse(raw) as SchemaStateFile; + const parsed: unknown = JSON.parse(raw); + return isSchemaStateFile(parsed) ? parsed : undefined; } catch { return undefined; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/schema-state.ts` around lines 33 - 34, The JSON parse is currently asserted with "as SchemaStateFile" which can hide malformed structures; after parsing raw with JSON.parse(raw) validate the result against the expected SchemaStateFile shape (e.g., check required properties like schemaHash and their types) or integrate a Zod schema for SchemaStateFile and parse/validate the object; if validation fails, throw a clear error describing the missing/invalid fields instead of returning the asserted object so callers relying on SchemaStateFile get a guaranteed shape.apps/cli/src/lib/schema-sync.ts (1)
26-30: Consider adding a request timeout.The
fetchercall has no timeout configured. If the server is unresponsive, the CLI will hang indefinitely. Consider adding a timeout viaAbortControlleror ensuring thefetcherimplementation has built-in timeout handling.💡 Example timeout implementation
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30_000); + + try { const response = await fetcher(`${serverUrl}/api/v1/schema`, { method: "PUT", headers, body: JSON.stringify(payload), + signal: controller.signal, }); + } finally { + clearTimeout(timeoutId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/schema-sync.ts` around lines 26 - 30, The fetch call to fetcher (sending payload to `${serverUrl}/api/v1/schema`) has no timeout and can hang; update the code in schema-sync.ts to use an AbortController: create a controller before calling fetcher, pass controller.signal into the fetcher options along with method/headers/body, start a setTimeout that calls controller.abort() after a reasonable timeout (e.g. configurable constant), and clear the timeout when the request completes; ensure you handle the abort error (thrown when controller aborts) and convert it into a friendly timeout error for the CLI.apps/cli/src/lib/init/detect-locale.ts (1)
97-100: Side-effect: mutates inputtypesarray.The function modifies
type.localized = truedirectly on the input objects. This side-effect isn't obvious from the function signature and could cause unexpected behavior if callers assume the input is unchanged.Consider returning a new array with the localized flags set, or document this mutation in the function's JSDoc:
♻️ Option A: Return localized type names instead of mutating
export async function detectLocaleConfig( files: DiscoveredFile[], types: InferredType[], prompter?: { select: (message: string, choices: { label: string; value: string }[]) => Promise<string>; }, -): Promise<LocaleConfig | null> { +): Promise<{ config: LocaleConfig; localizedTypes: Set<string> } | null> { const allLocales: string[] = []; const rawToNormalized: Record<string, string> = {}; let hasMultiLocaleType = false; + const localizedTypes = new Set<string>(); for (const type of types) { // ... existing logic ... if (typeLocales.size >= 2) { - type.localized = true; + localizedTypes.add(type.name); hasMultiLocaleType = true; } } // ... rest of function ... - return { defaultLocale, supported, aliases }; + return { config: { defaultLocale, supported, aliases }, localizedTypes }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/detect-locale.ts` around lines 97 - 100, The function in detect-locale.ts currently mutates the input objects by setting type.localized = true; instead, avoid side effects by creating new objects (e.g., shallow-clone each item) before adding the localized flag and return the new array/updated structure; update the logic around the symbol type (the loop that uses the variable named type and the collection types) to push cloned objects with localized: true rather than modifying the original, and ensure the function's return value documents/returns the new array (or update callers to accept the transformed result) so callers are not surprised by mutation.apps/cli/src/lib/push.ts (1)
855-877: Minor: Schema state is read twice.
readSchemaStateis called at Line 820 and again at Line 861. Since the schema state file is unlikely to change between these calls, the second read could be avoided by reusingschemaStatefrom the first read.♻️ Reuse existing schemaState
if (options.validate) { const resolvedSchema = serializeResolvedEnvironmentSchema( context.config as ParsedMdcmsConfig, context.environment, ); - const schemaSyncState = await readSchemaState({ - cwd: context.cwd, - project: context.project, - environment: context.environment, - }); - - if (schemaSyncState) { + // schemaState already read above and guaranteed to exist + { const currentPayload = buildSchemaSyncPayload( context.config as ParsedMdcmsConfig, context.environment, ); - if (schemaSyncState.schemaHash !== currentPayload.schemaHash) { + if (schemaState.schemaHash !== currentPayload.schemaHash) { context.stderr.write( "Warning: Local schema differs from last synced schema. Run `cms schema sync` to update the server.\n", ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/push.ts` around lines 855 - 877, The code redundantly calls readSchemaState a second time; reuse the already-read schemaState (from the earlier read at the top of push flow) instead of calling readSchemaState again and remove the duplicate await; update the local variable usage (replace schemaSyncState with the existing schemaState or vice versa) in the validate block where buildSchemaSyncPayload and schemaHash comparison occur so the logic references the previously loaded schemaState and no extra file read happens.apps/cli/src/lib/validate.ts (1)
112-115: Date validation is lenient — any string passes.The
datekind validation accepts any string value without verifying it's a valid ISO 8601 date. Invalid strings like"not-a-date"will pass validation but likely fail server-side or cause runtime issues.Consider validating the date format:
♻️ Proposed stricter date validation
case "date": - return typeof value === "string" || value instanceof Date - ? [] - : [`Field "${path}" expected kind "date" (ISO string), got ${typeof value}.`]; + if (value instanceof Date) { + return Number.isNaN(value.getTime()) + ? [`Field "${path}" is an invalid Date object.`] + : []; + } + if (typeof value === "string") { + const parsed = Date.parse(value); + return Number.isNaN(parsed) + ? [`Field "${path}" expected a valid ISO date string, got "${value}".`] + : []; + } + return [`Field "${path}" expected kind "date" (ISO string), got ${typeof value}.`];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/validate.ts` around lines 112 - 115, The "date" branch in validate.ts currently treats any string as valid; update the case handling inside the validator (the "date" case) to check that a string parses to a real date or that a Date instance is valid: for strings attempt to parse (e.g., new Date(string) and ensure !isNaN(date.getTime())) and for Date objects ensure !isNaN(value.getTime()); if parsing fails return the existing error message referencing path and the invalid input type/value. Use the "date" case in the validator function to locate and implement this stricter check.apps/server/src/lib/projects-api.ts (1)
243-253: Optional chaining on route registration may hide configuration errors.Using
projectApp.get?.()silently skips route registration if the method doesn't exist. This could lead to missing routes at runtime without any error. Consider asserting the app has the required methods or removing optional chaining.♻️ Proposed fix
export function mountProjectApiRoutes( app: unknown, options: MountProjectApiRoutesOptions, ): void { const projectApp = app as ProjectRouteApp; + + if (!projectApp.get || !projectApp.post) { + throw new Error("App must implement get and post methods for route mounting"); + } - projectApp.get?.("/api/v1/projects", ({ request }: any) => { + projectApp.get("/api/v1/projects", ({ request }: any) => {Apply similar changes to all route registrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/projects-api.ts` around lines 243 - 253, The route registration uses optional chaining on projectApp.get (projectApp.get?.("/api/v1/projects")), which can silently skip registering the route; change this to require the method exists by removing the optional chaining and either call projectApp.get(...) directly or assert/throw if projectApp.get is undefined (e.g., validate ProjectRouteApp supports get before registering). Apply the same change to all other route registrations so missing methods surface as errors instead of silently skipping routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/lib/init/generate-config.ts`:
- Around line 88-93: The generated config currently emits unquoted alias object
keys in the loop over Object.entries(lc.aliases), which breaks when keys contain
non-identifier characters (e.g., "en-us"); update the loop that builds lines
(the block referencing lc.aliases and lines.push) to emit quoted and escaped
keys (for example by wrapping the raw key with double quotes or using a
string-escaping helper/JSON.stringify(raw)) so each alias line becomes
"<escapedRaw>": "<normalized>", ensuring valid JavaScript output.
In `@apps/cli/src/lib/init/git-cleanup.ts`:
- Around line 106-111: The loop in untrackFiles interpolates directory names
directly into a shell command using execSync (same issue as detectTrackedFiles);
replace usage of execSync with a variant that accepts an argv array (e.g.,
execFileSync or spawnSync) and pass "git" as the command and
["rm","-r","--cached", dir] as arguments so dir is not interpreted by a shell,
keep cwd and encoding options, and ensure you properly catch and log errors in
the same try/catch around untrackFiles to preserve current behavior.
- Around line 79-84: The loop over managedDirectories in git-cleanup.ts is
vulnerable because it interpolates dir into a shell string passed to execSync;
replace this with a call to execFileSync (or spawnSync) passing "git" as the
executable and an args array like ["ls-files", dir] plus the cwd/encoding
options to avoid shell interpretation—update the code that currently calls
execSync(`git ls-files "${dir}"`, { cwd, encoding: "utf8" }) to use
execFileSync("git", ["ls-files", dir], { cwd, encoding: "utf8" }) and adjust any
error handling around the output variable accordingly.
In `@apps/cli/src/lib/init/scan.test.ts`:
- Around line 31-35: The test currently relies on traversal order when asserting
elements of the files array; before asserting, sort the files array by the
relativePath property (e.g., files.sort((a,b) =>
a.relativePath.localeCompare(b.relativePath))) so assertions on files[0] and
files[1] are deterministic; update the test in scan.test.ts to sort files by
relativePath and then assert files.length, files[0]!.relativePath,
files[0]!.format, files[1]!.relativePath, and files[1]!.format.
In `@apps/cli/src/lib/schema-state.test.ts`:
- Around line 23-30: The test currently asserts a POSIX-only string literal;
change it to build the expected path using Node's path.join to be
platform-neutral: import path from "path" in the test file and replace the
literal "/home/user/project/.mdcms/schema/marketing.staging.json" with
path.join("/home/user/project", ".mdcms", "schema", "marketing.staging.json") so
resolveSchemaStatePath is compared against a platform-correct path; reference:
resolveSchemaStatePath in this test.
In `@apps/cli/src/lib/status.test.ts`:
- Around line 92-101: The writeLocalFile helper uses string splitting to compute
the directory (absolutePath.substring(...lastIndexOf("/"))), which is not
cross-platform; update writeLocalFile to call path.dirname(absolutePath) (or
import dirname from "path") to compute dir, use that dirname when calling mkdir,
and ensure the path module is imported at the top so the function works on
Windows as well as POSIX.
In `@apps/server/src/lib/projects-api.ts`:
- Around line 199-235: In createEnvironment, handle the DB unique constraint on
(projectId,name) so a duplicate environment name produces a friendly validation
error: either pre-check for an existing environment by querying environments
where projectId === projectRow.id and name === name and throw a RuntimeError
with code "INVALID_INPUT" if found, or wrap the
db.insert(environments).values(...).returning(...) call in a try/catch and on a
unique-constraint error for unique_environment_per_project map it to a
RuntimeError({ code: "INVALID_INPUT", message: `Environment with name "${name}"
already exists for project "${normalizedSlug}".`, statusCode: 400, details: {
project: normalizedSlug, name } }); keep other errors rethrown.
- Around line 74-78: The toIsoString function can throw for null/undefined or
unparseable inputs; update toIsoString to defensively validate the input before
calling toISOString: if value is null/undefined return a safe sentinel (e.g.,
null or empty string) or throw a clear RangeError, otherwise create a Date
instance (new Date(value)) and check isNaN(date.getTime()) to detect unparseable
values and handle them accordingly, then call date.toISOString(); locate and
change the toIsoString function to implement this validation and clear
error/sentinel behavior.
In `@apps/studio-example/content/posts/hello-mdcms.md`:
- Line 10: Remove the stray word "edit" left on line 10 of the markdown post
hello-mdcms.md (it appears to be accidental); either delete that word so the
content reads correctly or replace it with the intended sentence/markup if it
was meant to be there — ensure the file's frontmatter and body remain valid
after the change.
---
Outside diff comments:
In `@apps/server/src/lib/content-api/database-store.ts`:
- Around line 1037-1049: The SQL UPDATE predicate is missing a guard against
concurrent soft-deletes: add a condition checking documents.isDeleted is false
inside the and(...) predicate that builds the WHERE (the same place using
scopeIds.projectId, scopeIds.environmentId, normalizedDocumentId and the
optional expectedDraftRevision). Ensure the transactional UPDATE uses this
augmented predicate so the update will not affect rows that were soft-deleted
after the earlier read.
In `@docs/adrs/ADR-004-project-environment-hierarchy.md`:
- Line 5: Update the document frontmatter's last_updated field to match the
addendum date by changing last_updated from 2026-03-11 to 2026-04-01; ensure the
frontmatter entry named last_updated and the addendum section's date are
consistent and, if present, update any other metadata fields that reference the
addendum date to avoid mismatch.
---
Nitpick comments:
In `@apps/cli/src/lib/init.test.ts`:
- Around line 20-40: The mock fetcher createMockFetcher currently matches
handlers using url.includes(pattern), which can produce false positives; update
the matching logic in createMockFetcher to use stricter matching (for example
compare exact URL strings, use startsWith for path prefixes, or parse the input
into a URL and compare url.pathname or host+pathname) and ensure handlers keys
are interpreted accordingly so tests only match intended endpoints (adjust
tests/handler keys if you choose pathname-only matching).
In `@apps/cli/src/lib/init/detect-locale.test.ts`:
- Around line 87-94: The test demonstrates that detectLocaleConfig(mutates the
input types array) by setting types[0].localized = true; make this explicit in
the implementation by adding a short in-code comment and/or updating the
function JSDoc for detectLocaleConfig to state that it modifies the passed-in
types array in-place (mentioning the localized property is set), so callers are
aware; reference detectLocaleConfig and the types parameter (and makeType usage)
when adding the note.
In `@apps/cli/src/lib/init/detect-locale.ts`:
- Around line 97-100: The function in detect-locale.ts currently mutates the
input objects by setting type.localized = true; instead, avoid side effects by
creating new objects (e.g., shallow-clone each item) before adding the localized
flag and return the new array/updated structure; update the logic around the
symbol type (the loop that uses the variable named type and the collection
types) to push cloned objects with localized: true rather than modifying the
original, and ensure the function's return value documents/returns the new array
(or update callers to accept the transformed result) so callers are not
surprised by mutation.
In `@apps/cli/src/lib/init/prompt.ts`:
- Line 38: Replace the direct call to process.exit(0) in the cancellation branch
(the clack.isCancel(result) check) with injectable behavior: add an optional
onCancel callback parameter (e.g., onCancel?: () => void) to the prompt function
and invoke it when clack.isCancel(result) is true, or alternatively throw a
dedicated CancellationError class (e.g., export class CancellationError extends
Error) so callers can handle cancellation; update callers/tests to either pass
an onCancel mock or catch the CancellationError instead of relying on
process.exit.
In `@apps/cli/src/lib/init/scan.ts`:
- Around line 91-98: The current catch block around readFile(fullPath, "utf-8")
and parseMarkdownDocument(...) swallows all errors (filesystem and parse); split
the operations and handle them separately: first await readFile(...) in its own
try/catch and rethrow or surface filesystem errors (inspect error.code like
EACCES/ENOENT) instead of treating them as malformed frontmatter, then call
parseMarkdownDocument(...) in a second try/catch that treats parse failures as
"malformed frontmatter" and sets frontmatter = {} or logs as needed; reference
the variables/functions fullPath, readFile, parseMarkdownDocument, and
frontmatter when making these changes.
In `@apps/cli/src/lib/push.ts`:
- Around line 855-877: The code redundantly calls readSchemaState a second time;
reuse the already-read schemaState (from the earlier read at the top of push
flow) instead of calling readSchemaState again and remove the duplicate await;
update the local variable usage (replace schemaSyncState with the existing
schemaState or vice versa) in the validate block where buildSchemaSyncPayload
and schemaHash comparison occur so the logic references the previously loaded
schemaState and no extra file read happens.
In `@apps/cli/src/lib/schema-state.ts`:
- Around line 33-34: The JSON parse is currently asserted with "as
SchemaStateFile" which can hide malformed structures; after parsing raw with
JSON.parse(raw) validate the result against the expected SchemaStateFile shape
(e.g., check required properties like schemaHash and their types) or integrate a
Zod schema for SchemaStateFile and parse/validate the object; if validation
fails, throw a clear error describing the missing/invalid fields instead of
returning the asserted object so callers relying on SchemaStateFile get a
guaranteed shape.
In `@apps/cli/src/lib/schema-sync.ts`:
- Around line 26-30: The fetch call to fetcher (sending payload to
`${serverUrl}/api/v1/schema`) has no timeout and can hang; update the code in
schema-sync.ts to use an AbortController: create a controller before calling
fetcher, pass controller.signal into the fetcher options along with
method/headers/body, start a setTimeout that calls controller.abort() after a
reasonable timeout (e.g. configurable constant), and clear the timeout when the
request completes; ensure you handle the abort error (thrown when controller
aborts) and convert it into a friendly timeout error for the CLI.
In `@apps/cli/src/lib/validate.ts`:
- Around line 112-115: The "date" branch in validate.ts currently treats any
string as valid; update the case handling inside the validator (the "date" case)
to check that a string parses to a real date or that a Date instance is valid:
for strings attempt to parse (e.g., new Date(string) and ensure
!isNaN(date.getTime())) and for Date objects ensure !isNaN(value.getTime()); if
parsing fails return the existing error message referencing path and the invalid
input type/value. Use the "date" case in the validator function to locate and
implement this stricter check.
In `@apps/server/src/bin/demo-seed-api-key.ts`:
- Around line 20-25: DEMO_KEY_SCOPES currently includes "schema:write"; change
the default to read-only by removing "schema:write" from the DEMO_KEY_SCOPES
const and instead conditionally append that scope only when an explicit env flag
is set (e.g. DEMO_KEY_ALLOW_WRITE or ENABLE_DEMO_KEY_WRITE === "true"). Update
the code that builds/uses DEMO_KEY_SCOPES so it constructs the scopes array at
runtime (preserving the readonly type) and only adds "schema:write" when the env
flag is enabled; ensure any seeding logic that consumes DEMO_KEY_SCOPES uses the
new runtime variable.
In `@apps/server/src/lib/projects-api.ts`:
- Around line 243-253: The route registration uses optional chaining on
projectApp.get (projectApp.get?.("/api/v1/projects")), which can silently skip
registering the route; change this to require the method exists by removing the
optional chaining and either call projectApp.get(...) directly or assert/throw
if projectApp.get is undefined (e.g., validate ProjectRouteApp supports get
before registering). Apply the same change to all other route registrations so
missing methods surface as errors instead of silently skipping routes.
In `@packages/shared/src/lib/contracts/schema.ts`:
- Around line 770-790: The toRawConfigSnapshot function produces objects whose
key/array insertion order can vary and cause schema hash churn; update
toRawConfigSnapshot to build the returned JsonObject with a stable canonical
ordering: always add keys in the same sequence (project, serverUrl, environment,
contentDirectories, locales), sort config.contentDirectories array (e.g.,
lexicographically) before including it, sort config.locales.supported array, and
canonicalize config.locales.aliases by creating a new object with its keys
iterated in sorted order (Object.keys(...).sort()) so the resulting snapshot is
deterministic even if inputs had different insertion orders.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 107fef98-4790-49dd-902b-a4c1df1bc0ef
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
.gitignoreapps/cli/package.jsonapps/cli/src/lib/framework.test.tsapps/cli/src/lib/framework.tsapps/cli/src/lib/init.test.tsapps/cli/src/lib/init.tsapps/cli/src/lib/init/detect-locale.test.tsapps/cli/src/lib/init/detect-locale.tsapps/cli/src/lib/init/generate-config.test.tsapps/cli/src/lib/init/generate-config.tsapps/cli/src/lib/init/git-cleanup.test.tsapps/cli/src/lib/init/git-cleanup.tsapps/cli/src/lib/init/infer-schema.test.tsapps/cli/src/lib/init/infer-schema.tsapps/cli/src/lib/init/prompt.test.tsapps/cli/src/lib/init/prompt.tsapps/cli/src/lib/init/scan.test.tsapps/cli/src/lib/init/scan.tsapps/cli/src/lib/login.test.tsapps/cli/src/lib/login.tsapps/cli/src/lib/push.test.tsapps/cli/src/lib/push.tsapps/cli/src/lib/schema-state.test.tsapps/cli/src/lib/schema-state.tsapps/cli/src/lib/schema-sync.test.tsapps/cli/src/lib/schema-sync.tsapps/cli/src/lib/status.test.tsapps/cli/src/lib/status.tsapps/cli/src/lib/validate.test.tsapps/cli/src/lib/validate.tsapps/server/drizzle/0010_keen_blockbuster.sqlapps/server/drizzle/meta/0010_snapshot.jsonapps/server/drizzle/meta/_journal.jsonapps/server/src/bin/demo-seed-api-key.tsapps/server/src/lib/auth.tsapps/server/src/lib/content-api.integration.test.tsapps/server/src/lib/content-api.test.tsapps/server/src/lib/content-api/database-store.tsapps/server/src/lib/content-api/in-memory-store.tsapps/server/src/lib/content-api/routes.tsapps/server/src/lib/content-api/types.tsapps/server/src/lib/db/schema.tsapps/server/src/lib/demo-seed.tsapps/server/src/lib/projects-api.tsapps/server/src/lib/runtime-with-modules.tsapps/studio-example/.mdcms/manifests/marketing-site.staging.jsonapps/studio-example/content/pages/about.mdxapps/studio-example/content/posts/hello-mdcms.mdapps/studio-example/content/posts/pull-push-demo.mddocs/adrs/ADR-004-project-environment-hierarchy.mddocs/adrs/ADR-006-schema-hash-pinning-for-write-clients.mddocs/specs/SPEC-004-schema-system-and-sync.mddocs/specs/SPEC-008-cli-and-sdk.mdpackages/shared/package.jsonpackages/shared/src/lib/contracts/schema-hash.tspackages/shared/src/lib/contracts/schema.test.tspackages/shared/src/lib/contracts/schema.ts
Comment on lines
+31
to
+35
| assert.equal(files.length, 2); | ||
| assert.equal(files[0]!.relativePath, "content/blog/post.mdx"); | ||
| assert.equal(files[0]!.format, "mdx"); | ||
| assert.equal(files[1]!.relativePath, "content/index.md"); | ||
| assert.equal(files[1]!.format, "md"); |
There was a problem hiding this comment.
Avoid order-dependent assertions for discovered files.
These positional checks can become flaky if traversal order differs across environments. Sort by relativePath in the test before asserting.
💡 Suggested fix
- assert.equal(files.length, 2);
- assert.equal(files[0]!.relativePath, "content/blog/post.mdx");
- assert.equal(files[0]!.format, "mdx");
- assert.equal(files[1]!.relativePath, "content/index.md");
- assert.equal(files[1]!.format, "md");
+ assert.equal(files.length, 2);
+ const sorted = [...files].sort((a, b) =>
+ a.relativePath.localeCompare(b.relativePath),
+ );
+ assert.equal(sorted[0]!.relativePath, "content/blog/post.mdx");
+ assert.equal(sorted[0]!.format, "mdx");
+ assert.equal(sorted[1]!.relativePath, "content/index.md");
+ assert.equal(sorted[1]!.format, "md");📝 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
| assert.equal(files.length, 2); | |
| assert.equal(files[0]!.relativePath, "content/blog/post.mdx"); | |
| assert.equal(files[0]!.format, "mdx"); | |
| assert.equal(files[1]!.relativePath, "content/index.md"); | |
| assert.equal(files[1]!.format, "md"); | |
| assert.equal(files.length, 2); | |
| const sorted = [...files].sort((a, b) => | |
| a.relativePath.localeCompare(b.relativePath), | |
| ); | |
| assert.equal(sorted[0]!.relativePath, "content/blog/post.mdx"); | |
| assert.equal(sorted[0]!.format, "mdx"); | |
| assert.equal(sorted[1]!.relativePath, "content/index.md"); | |
| assert.equal(sorted[1]!.format, "md"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/lib/init/scan.test.ts` around lines 31 - 35, The test currently
relies on traversal order when asserting elements of the files array; before
asserting, sort the files array by the relativePath property (e.g.,
files.sort((a,b) => a.relativePath.localeCompare(b.relativePath))) so assertions
on files[0] and files[1] are deterministic; update the test in scan.test.ts to
sort files by relativePath and then assert files.length, files[0]!.relativePath,
files[0]!.format, files[1]!.relativePath, and files[1]!.format.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/cli/src/lib/validate.test.ts (1)
175-194: Consider adding edge case tests for arrays.The array validation tests are solid. For additional robustness, you might consider adding tests for:
- Empty array
[](valid case)- Array of objects with nested validation
- Array with all invalid items (to verify all errors are collected)
These can be deferred if not immediately needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/validate.test.ts` around lines 175 - 194, Add three edge-case tests in apps/cli/src/lib/validate.test.ts alongside the existing "validateFrontmatter validates array item types" test that call validateFrontmatter with SchemaRegistryTypeSnapshot schemas that exercise arrays: (1) an empty array case (e.g., tags: [] with kind: "array" and item kind "string") and assert result.errors.length === 0, (2) an array of objects case that defines item as kind: "object"/fields and pass nested invalid/valid objects to assert nested validation errors are reported on the correct path (e.g., tags[0].field), and (3) an all-invalid-items case where every array element is invalid and assert result.errors.length equals the number of invalid items and each error path includes the index (e.g., tags[0], tags[1]); use the same test utilities and assertions style as the existing test that references validateFrontmatter and SchemaRegistryTypeSnapshot.apps/cli/src/lib/init/git-cleanup.test.ts (2)
115-117: Strengthen untrack assertion to validate exact expected file(s).
removed.length > 0can pass even if the wrong files were removed. Assert the expected path(s) explicitly to prevent false positives.Proposed assertion improvement
const removed = await untrackFiles(cwd, ["content"]); - assert.ok(removed.length > 0); + assert.equal(removed.length, 1); + assert.ok(removed.includes("content/post.md"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/git-cleanup.test.ts` around lines 115 - 117, The test currently checks only that removed.length > 0 which can hide wrong removals; update the assertion after calling untrackFiles(cwd, ["content"]) to explicitly verify the returned list equals the expected file path(s) (for example assert.deepStrictEqual(removed.sort(), ["content"].sort()) or assert.deepStrictEqual(removed, ["content"]) as appropriate for the return shape), referencing the untrackFiles call in git-cleanup.test.ts to ensure the exact expected path(s) were untracked.
24-28: UseexecFileSyncwith argument arrays instead ofexecSyncshell strings for git commands.Single-quote arguments are treated as literal characters by Windows
cmd.exebut as string delimiters by POSIX shells (/bin/sh). Using argument arrays withexecFileSyncavoids platform-specific shell parsing and ensures consistent behavior across Windows and Unix systems.Proposed refactor
-import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; ... function initGitRepo(cwd: string): void { - execSync("git init", { cwd, stdio: "ignore" }); - execSync("git config user.email 'test@test.com'", { cwd, stdio: "ignore" }); - execSync("git config user.name 'Test'", { cwd, stdio: "ignore" }); + execFileSync("git", ["init"], { cwd, stdio: "ignore" }); + execFileSync("git", ["config", "user.email", "test@test.com"], { + cwd, + stdio: "ignore", + }); + execFileSync("git", ["config", "user.name", "Test"], { + cwd, + stdio: "ignore", + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/git-cleanup.test.ts` around lines 24 - 28, The initGitRepo helper uses execSync with shell strings which breaks on Windows; replace those calls with execFileSync and pass the git executable plus argument arrays (e.g., ["config", "user.email", "test@test.com"]) and keep the same options object ({ cwd, stdio: "ignore" }) so the commands run without shell parsing; update each invocation inside initGitRepo (git init, git config user.email, git config user.name) to use execFileSync with argv arrays to ensure cross-platform consistency.apps/cli/src/lib/init/detect-locale.ts (1)
58-61: Avoid re-filtering all files for each type.At Line 58–61,
files.filter(...)runs once per type, yielding O(types × files). This can get slow on large content trees. Consider pre-indexing files by top-level directory (or grouping once) and reusing that map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/detect-locale.ts` around lines 58 - 61, The loop currently recomputes files.filter(...) for each element of types (creating typeFiles) which is O(types × files); instead, pre-group files once by their top-level directory and reuse that map inside the loop: build a Map keyed by directory (using the same predicate logic as filesBelongToDirectory) before iterating types, then replace the per-type files.filter(...) with a single map lookup for type.directory (update references to typeFiles accordingly). Keep the filesBelongToDirectory logic when constructing the map so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/lib/init/detect-locale.ts`:
- Around line 70-89: The code currently prompts every time normalizeLocale(raw)
returns falsy; modify the flow to first check a cache map rawToNormalized for an
existing decision before calling prompter.select, returning/using that cached
value if present (treat a cached "__skip__" as continue). After the user selects
in prompter.select, immediately store the decision in rawToNormalized[raw] to
prevent future prompts and alias drift, and when adding to
typeLocales/allLocales ensure you only add unique entries (use existence checks
or Sets) so later iterations cannot overwrite or duplicate earlier mappings;
keep references to normalizeLocale, rawToNormalized, prompter.select,
typeLocales, and allLocales when making these changes.
In `@apps/cli/src/lib/push.test.ts`:
- Around line 703-727: The success branch of the fetcher mock should validate
that the follow-up request is a PUT to the expected document resource instead of
returning 200 for any non-conflict call. In the fetcher function (the mocked
fetch used in these tests), accept the second argument (init), assert
init?.method === "PUT" and verify the URL matches the expected resource path
(e.g., endsWith("/api/v1/content/doc-stale") -> for the stale test ensure the
PUT target is the document resource like "/api/v1/content/doc-stale" or the
intended document path), then return createSuccessResponse(...) only when both
method and URL match; otherwise return a non-matching response (e.g., 404) to
fail the test. Apply the same change to the other mock instance referenced at
lines ~835-859.
- Around line 54-70: The schema-state fixture written by writeSchemaStateFile is
deterministic, so update the tests to assert the exact expected schema hash
string instead of only checking truthiness: compute expectedHash =
"test-schema-hash-" + "a".repeat(48) and assert equality against the
header/field the code sends (where the existing truthy check occurs) and
likewise in the POST fallback fetcher test ensure the forwarded header/body
contains expectedHash (use the same expectedHash variable) rather than merely
checking presence; locate these checks around the current writeSchemaStateFile
usage and the POST fallback fetcher handler and replace truthy assertions with
strict equality checks against expectedHash.
In `@apps/server/src/lib/auth.ts`:
- Line 3714: The code is treating "*" as a literal value instead of a wildcard,
so update the environment checks to treat "*" as a wildcard in both
apiKeyAllowsTarget and authorizeRequest (and the similar check around line
3793). Concretely, change the strict equality checks that compare
requirement.environment or target.environment to the key/environment value so
that if either side is "*" it counts as a match (e.g., replace A === B with a
comparison that returns true when A === B OR A === "*" OR B === "*"). Ensure
this logic is applied to the functions/methods named apiKeyAllowsTarget and
authorizeRequest and to the places that read challenge.environment (which
currently falls back to "*").
- Around line 65-66: The RBAC mapping and checks must be updated so
"projects:read" and "projects:write" are handled instead of falling through: in
toRbacAction() add explicit cases mapping "projects:read" ->
ResourceAction{resource: PROJECT, action: READ} and "projects:write" ->
ResourceAction{resource: PROJECT, action: WRITE} (remove the fall-through that
returns no mapping for these scopes); ensure assertSessionRbacAuthorization() no
longer skips RBAC for session requests when a projects:* scope is present (stop
returning allow-all on that fall-through path and evaluate the mapped
ResourceAction); and update resolveApiKeyRbacAction() to accept/mint the
projects:read and projects:write scopes rather than throwing during key issuance
so API-key creation can produce those scopes.
- Around line 3710-3725: The current flow lets cli:user-level exchanges create
global tokens because authContextAllowlist is set to [] when challenge.project
is missing; update the logic before calling assertSessionCanIssueApiKeyScopes
(and before persisting the allowlist) to ensure the allowlist is never empty for
scope types that require a project: if challenge.project is absent, build
authContextAllowlist from the session's granted project targets (or reject the
request) rather than leaving it as [], and validate that the resulting
authContextAllowlist has at least one entry (mirroring the
contextAllowlist.min(1) invariant); adjust code around
normalizeRequestedCliScopes, authContextAllowlist, and the call to
assertSessionCanIssueApiKeyScopes to enforce this.
- Around line 3434-3464: The current check skips context enforcement when
metadata.contextAllowlist is empty; change authorize() so that routing context
is always enforced when a contextAllowlist is present (i.e., do not treat an
empty array as “allow all”). Specifically, replace the `if
(metadata.contextAllowlist.length > 0)` gate with a presence check (e.g., `if
(metadata.contextAllowlist !== undefined)` or remove the length check) so you
always validate `requirement.project`/`requirement.environment` and call
`apiKeyAllowsTarget(metadata.contextAllowlist, { project: requirement.project,
environment: requirement.environment })`; let an empty array cause the FORBIDDEN
branch to run rather than bypassing checks. Ensure code paths that throw
RuntimeError still include the same messages and details.
In `@apps/server/src/lib/projects-api.ts`:
- Around line 236-241: The mountProjectApiRoutes function currently uses a
single authorize callback (passed via MountProjectApiRoutesOptions) that only
enforces "projects:read", which incorrectly allows write endpoints to be
authorized with read-only scope; update the API to accept two callbacks
(authorizeRead and authorizeWrite) and wire them so GET routes continue to call
authorizeRead while POST /api/v1/projects and POST
/api/v1/projects/:slug/environments call authorizeWrite, and then update the
caller in runtime-with-modules.ts to pass an authorizeWrite that requires
"projects:write" (keeping authorizeRead for "projects:read").
---
Nitpick comments:
In `@apps/cli/src/lib/init/detect-locale.ts`:
- Around line 58-61: The loop currently recomputes files.filter(...) for each
element of types (creating typeFiles) which is O(types × files); instead,
pre-group files once by their top-level directory and reuse that map inside the
loop: build a Map keyed by directory (using the same predicate logic as
filesBelongToDirectory) before iterating types, then replace the per-type
files.filter(...) with a single map lookup for type.directory (update references
to typeFiles accordingly). Keep the filesBelongToDirectory logic when
constructing the map so behavior is unchanged.
In `@apps/cli/src/lib/init/git-cleanup.test.ts`:
- Around line 115-117: The test currently checks only that removed.length > 0
which can hide wrong removals; update the assertion after calling
untrackFiles(cwd, ["content"]) to explicitly verify the returned list equals the
expected file path(s) (for example assert.deepStrictEqual(removed.sort(),
["content"].sort()) or assert.deepStrictEqual(removed, ["content"]) as
appropriate for the return shape), referencing the untrackFiles call in
git-cleanup.test.ts to ensure the exact expected path(s) were untracked.
- Around line 24-28: The initGitRepo helper uses execSync with shell strings
which breaks on Windows; replace those calls with execFileSync and pass the git
executable plus argument arrays (e.g., ["config", "user.email",
"test@test.com"]) and keep the same options object ({ cwd, stdio: "ignore" }) so
the commands run without shell parsing; update each invocation inside
initGitRepo (git init, git config user.email, git config user.name) to use
execFileSync with argv arrays to ensure cross-platform consistency.
In `@apps/cli/src/lib/validate.test.ts`:
- Around line 175-194: Add three edge-case tests in
apps/cli/src/lib/validate.test.ts alongside the existing "validateFrontmatter
validates array item types" test that call validateFrontmatter with
SchemaRegistryTypeSnapshot schemas that exercise arrays: (1) an empty array case
(e.g., tags: [] with kind: "array" and item kind "string") and assert
result.errors.length === 0, (2) an array of objects case that defines item as
kind: "object"/fields and pass nested invalid/valid objects to assert nested
validation errors are reported on the correct path (e.g., tags[0].field), and
(3) an all-invalid-items case where every array element is invalid and assert
result.errors.length equals the number of invalid items and each error path
includes the index (e.g., tags[0], tags[1]); use the same test utilities and
assertions style as the existing test that references validateFrontmatter and
SchemaRegistryTypeSnapshot.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c907d046-a9e0-4341-96b9-d4ede2c2c892
📒 Files selected for processing (24)
apps/cli/src/lib/init.tsapps/cli/src/lib/init/detect-locale.tsapps/cli/src/lib/init/generate-config.test.tsapps/cli/src/lib/init/generate-config.tsapps/cli/src/lib/init/git-cleanup.test.tsapps/cli/src/lib/init/infer-schema.tsapps/cli/src/lib/init/prompt.test.tsapps/cli/src/lib/init/scan.test.tsapps/cli/src/lib/init/scan.tsapps/cli/src/lib/login.test.tsapps/cli/src/lib/push.test.tsapps/cli/src/lib/push.tsapps/cli/src/lib/schema-state.test.tsapps/cli/src/lib/schema-sync.test.tsapps/cli/src/lib/schema-sync.tsapps/cli/src/lib/status.test.tsapps/cli/src/lib/validate.test.tsapps/cli/src/lib/validate.tsapps/server/drizzle/meta/0010_snapshot.jsonapps/server/drizzle/meta/_journal.jsonapps/server/src/lib/auth.tsapps/server/src/lib/content-api/database-store.tsapps/server/src/lib/projects-api.tspackages/shared/src/lib/contracts/schema.ts
✅ Files skipped from review due to trivial changes (11)
- apps/server/drizzle/meta/_journal.json
- apps/cli/src/lib/login.test.ts
- apps/cli/src/lib/init/prompt.test.ts
- apps/cli/src/lib/schema-state.test.ts
- apps/server/src/lib/content-api/database-store.ts
- apps/server/drizzle/meta/0010_snapshot.json
- apps/cli/src/lib/init/scan.test.ts
- apps/cli/src/lib/init/scan.ts
- apps/cli/src/lib/init.ts
- apps/cli/src/lib/push.ts
- apps/cli/src/lib/status.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/cli/src/lib/init/generate-config.test.ts
- apps/cli/src/lib/schema-sync.ts
- apps/cli/src/lib/validate.ts
- apps/cli/src/lib/init/infer-schema.ts
- apps/cli/src/lib/init/generate-config.ts
Comment on lines
+70
to
+89
| const normalized = normalizeLocale(raw); | ||
| if (!normalized) { | ||
| if (!prompter) continue; | ||
|
|
||
| const currentSupported = [...new Set(allLocales)]; | ||
| const choices = [ | ||
| ...currentSupported.map((l) => ({ label: l, value: l })), | ||
| { label: "Skip this file", value: "__skip__" }, | ||
| ]; | ||
| const selection = await prompter.select( | ||
| `Cannot normalize locale "${raw}" from ${file.relativePath}. Map to:`, | ||
| choices, | ||
| ); | ||
|
|
||
| if (selection === "__skip__") continue; | ||
|
|
||
| typeLocales.add(selection); | ||
| allLocales.push(selection); | ||
| rawToNormalized[raw] = selection; | ||
| continue; |
There was a problem hiding this comment.
Cache invalid-locale decisions to prevent repeated prompts and alias drift.
At Line 70–89, the same un-normalizable raw locale can trigger prompter.select repeatedly. This also allows later selections to overwrite earlier alias mappings for the same raw value (rawToNormalized[raw]), making results order-dependent.
💡 Suggested fix
@@
export async function detectLocaleConfig(
@@
): Promise<LocaleConfig | null> {
const allLocales: string[] = [];
const rawToNormalized: Record<string, string> = {};
+ const skippedRawLocales = new Set<string>();
let hasMultiLocaleType = false;
@@
const normalized = normalizeLocale(raw);
if (!normalized) {
+ const existing = rawToNormalized[raw];
+ if (existing) {
+ typeLocales.add(existing);
+ allLocales.push(existing);
+ continue;
+ }
+
+ if (skippedRawLocales.has(raw)) continue;
if (!prompter) continue;
@@
const selection = await prompter.select(
`Cannot normalize locale "${raw}" from ${file.relativePath}. Map to:`,
choices,
);
- if (selection === "__skip__") continue;
+ if (selection === "__skip__") {
+ skippedRawLocales.add(raw);
+ continue;
+ }
typeLocales.add(selection);
allLocales.push(selection);
rawToNormalized[raw] = selection;
continue;
}📝 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 normalized = normalizeLocale(raw); | |
| if (!normalized) { | |
| if (!prompter) continue; | |
| const currentSupported = [...new Set(allLocales)]; | |
| const choices = [ | |
| ...currentSupported.map((l) => ({ label: l, value: l })), | |
| { label: "Skip this file", value: "__skip__" }, | |
| ]; | |
| const selection = await prompter.select( | |
| `Cannot normalize locale "${raw}" from ${file.relativePath}. Map to:`, | |
| choices, | |
| ); | |
| if (selection === "__skip__") continue; | |
| typeLocales.add(selection); | |
| allLocales.push(selection); | |
| rawToNormalized[raw] = selection; | |
| continue; | |
| const normalized = normalizeLocale(raw); | |
| if (!normalized) { | |
| const existing = rawToNormalized[raw]; | |
| if (existing) { | |
| typeLocales.add(existing); | |
| allLocales.push(existing); | |
| continue; | |
| } | |
| if (skippedRawLocales.has(raw)) continue; | |
| if (!prompter) continue; | |
| const currentSupported = [...new Set(allLocales)]; | |
| const choices = [ | |
| ...currentSupported.map((l) => ({ label: l, value: l })), | |
| { label: "Skip this file", value: "__skip__" }, | |
| ]; | |
| const selection = await prompter.select( | |
| `Cannot normalize locale "${raw}" from ${file.relativePath}. Map to:`, | |
| choices, | |
| ); | |
| if (selection === "__skip__") { | |
| skippedRawLocales.add(raw); | |
| continue; | |
| } | |
| typeLocales.add(selection); | |
| allLocales.push(selection); | |
| rawToNormalized[raw] = selection; | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/lib/init/detect-locale.ts` around lines 70 - 89, The code
currently prompts every time normalizeLocale(raw) returns falsy; modify the flow
to first check a cache map rawToNormalized for an existing decision before
calling prompter.select, returning/using that cached value if present (treat a
cached "__skip__" as continue). After the user selects in prompter.select,
immediately store the decision in rawToNormalized[raw] to prevent future prompts
and alias drift, and when adding to typeLocales/allLocales ensure you only add
unique entries (use existence checks or Sets) so later iterations cannot
overwrite or duplicate earlier mappings; keep references to normalizeLocale,
rawToNormalized, prompter.select, typeLocales, and allLocales when making these
changes.
Comment on lines
+54
to
+70
| async function writeSchemaStateFile( | ||
| cwd: string, | ||
| project: string, | ||
| environment: string, | ||
| ): Promise<void> { | ||
| const dir = join(cwd, ".mdcms", "schema"); | ||
| await mkdir(dir, { recursive: true }); | ||
| await writeFile( | ||
| join(dir, `${project}.${environment}.json`), | ||
| JSON.stringify({ | ||
| schemaHash: "test-schema-hash-" + "a".repeat(48), | ||
| syncedAt: "2026-03-31T12:00:00.000Z", | ||
| serverUrl: "http://localhost:4000", | ||
| }), | ||
| "utf8", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Assert the actual schema hash, not just its presence.
writeSchemaStateFile() now creates a deterministic fixture, but Line 119 only checks truthiness and the POST fallback test at Line 191 still doesn't validate what gets forwarded. A regression that reads the wrong schema-state file or sends the wrong value would still pass this suite.
Possible tightening
+const TEST_SCHEMA_HASH = "a".repeat(64);
+
async function writeSchemaStateFile(
cwd: string,
project: string,
environment: string,
): Promise<void> {
@@
join(dir, `${project}.${environment}.json`),
JSON.stringify({
- schemaHash: "test-schema-hash-" + "a".repeat(48),
+ schemaHash: TEST_SCHEMA_HASH,
syncedAt: "2026-03-31T12:00:00.000Z",
serverUrl: "http://localhost:4000",
}),
@@
const headers = new Headers(init?.headers as Record<string, string>);
- assert.ok(
- headers.get("x-mdcms-schema-hash"),
- "x-mdcms-schema-hash header must be present",
- );
+ assert.equal(headers.get("x-mdcms-schema-hash"), TEST_SCHEMA_HASH);Apply the same header equality check in the POST fallback fetcher.
Also applies to: 117-121, 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/lib/push.test.ts` around lines 54 - 70, The schema-state fixture
written by writeSchemaStateFile is deterministic, so update the tests to assert
the exact expected schema hash string instead of only checking truthiness:
compute expectedHash = "test-schema-hash-" + "a".repeat(48) and assert equality
against the header/field the code sends (where the existing truthy check occurs)
and likewise in the POST fallback fetcher test ensure the forwarded header/body
contains expectedHash (use the same expectedHash variable) rather than merely
checking presence; locate these checks around the current writeSchemaStateFile
usage and the POST fallback fetcher handler and replace truthy assertions with
strict equality checks against expectedHash.
Comment on lines
+3434
to
3464
| if (metadata.contextAllowlist.length > 0) { | ||
| if (!requirement.project || !requirement.environment) { | ||
| throw new RuntimeError({ | ||
| code: "FORBIDDEN", | ||
| message: | ||
| "API key authorization requires explicit project/environment routing context.", | ||
| statusCode: 403, | ||
| }); | ||
| } | ||
|
|
||
| if (!isContextAllowed) { | ||
| throw new RuntimeError({ | ||
| code: "FORBIDDEN", | ||
| message: | ||
| "API key is not allowed for the requested project/environment context.", | ||
| statusCode: 403, | ||
| details: { | ||
| const isContextAllowed = apiKeyAllowsTarget( | ||
| metadata.contextAllowlist, | ||
| { | ||
| project: requirement.project, | ||
| environment: requirement.environment, | ||
| }, | ||
| }); | ||
| ); | ||
|
|
||
| if (!isContextAllowed) { | ||
| throw new RuntimeError({ | ||
| code: "FORBIDDEN", | ||
| message: | ||
| "API key is not allowed for the requested project/environment context.", | ||
| statusCode: 403, | ||
| details: { | ||
| project: requirement.project, | ||
| environment: requirement.environment, | ||
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't turn an empty allowlist into "ignore routed target".
apps/server/src/lib/content-api/routes.ts at Lines 98-102 and apps/server/src/lib/schema-api.ts at Lines 774-778 already pass the routed project/environment into authorize(). This branch stops enforcing that context whenever contextAllowlist is empty, so any empty-allowlist key with the right scope can cross project/environment boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/lib/auth.ts` around lines 3434 - 3464, The current check
skips context enforcement when metadata.contextAllowlist is empty; change
authorize() so that routing context is always enforced when a contextAllowlist
is present (i.e., do not treat an empty array as “allow all”). Specifically,
replace the `if (metadata.contextAllowlist.length > 0)` gate with a presence
check (e.g., `if (metadata.contextAllowlist !== undefined)` or remove the length
check) so you always validate `requirement.project`/`requirement.environment`
and call `apiKeyAllowsTarget(metadata.contextAllowlist, { project:
requirement.project, environment: requirement.environment })`; let an empty
array cause the FORBIDDEN branch to run rather than bypassing checks. Ensure
code paths that throw RuntimeError still include the same messages and details.
Comment on lines
+3710
to
3725
| const authContextAllowlist: ApiKeyScopeTuple[] = challenge.project | ||
| ? [ | ||
| { | ||
| project: challenge.project, | ||
| environment: challenge.environment ?? "*", | ||
| }, | ||
| ] | ||
| : []; | ||
|
|
||
| await assertSessionCanIssueApiKeyScopes( | ||
| session, | ||
| normalizeRequestedCliScopes( | ||
| challenge.requestedScopes as ApiKeyOperationScope[], | ||
| ), | ||
| [ | ||
| { | ||
| project: challenge.project, | ||
| environment: challenge.environment, | ||
| }, | ||
| ], | ||
| authContextAllowlist, | ||
| ); |
There was a problem hiding this comment.
cli:user-level keys currently bypass RBAC and mint as global tokens.
When challenge.project is absent, authContextAllowlist becomes []. assertSessionCanIssueApiKeyScopes() only checks permissions inside the for (const target of contextAllowlist) loop on Lines 2913-2938, so supported scopes like content:write, content:publish, or schema:write skip the caller's project grants entirely. The exchange then persists that empty allowlist, and this path also sidesteps the contextAllowlist.min(1) invariant enforced by the regular API-key creation schema on Line 412.
Also applies to: 3786-3803
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/lib/auth.ts` around lines 3710 - 3725, The current flow lets
cli:user-level exchanges create global tokens because authContextAllowlist is
set to [] when challenge.project is missing; update the logic before calling
assertSessionCanIssueApiKeyScopes (and before persisting the allowlist) to
ensure the allowlist is never empty for scope types that require a project: if
challenge.project is absent, build authContextAllowlist from the session's
granted project targets (or reject the request) rather than leaving it as [],
and validate that the resulting authContextAllowlist has at least one entry
(mirroring the contextAllowlist.min(1) invariant); adjust code around
normalizeRequestedCliScopes, authContextAllowlist, and the call to
assertSessionCanIssueApiKeyScopes to enforce this.
| ? [ | ||
| { | ||
| project: challenge.project, | ||
| environment: challenge.environment ?? "*", |
There was a problem hiding this comment.
"*" is not a wildcard with the current matcher.
apiKeyAllowsTarget() on Lines 1777-1780 still does exact equality, and authorizeRequest() on Lines 3435-3440 still requires a concrete requirement.environment. A key labeled cli:<project>/* will not match dev, prod, etc., so project-scoped CLI logins without an explicit environment will produce unusable keys.
Also applies to: 3793-3793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/lib/auth.ts` at line 3714, The code is treating "*" as a
literal value instead of a wildcard, so update the environment checks to treat
"*" as a wildcard in both apiKeyAllowsTarget and authorizeRequest (and the
similar check around line 3793). Concretely, change the strict equality checks
that compare requirement.environment or target.environment to the
key/environment value so that if either side is "*" it counts as a match (e.g.,
replace A === B with a comparison that returns true when A === B OR A === "*" OR
B === "*"). Ensure this logic is applied to the functions/methods named
apiKeyAllowsTarget and authorizeRequest and to the places that read
challenge.environment (which currently falls back to "*").
Collaborator
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation