Conversation
…e:crypto in browser bundle
…and prettier formatting
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete CLI init wizard, robust schema sync/state and status commands, expanded push/pull validation and conflict handling, new Projects API and project/environment management, schema-hash utilities and persisted schema state, optimistic concurrency for content writes, and multiple supporting CLI/server utilities and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as mdcms CLI
participant Server
participant DB as Database
participant Browser
User->>CLI: mdcms init
CLI->>Server: GET /api/v1/healthz
Server-->>CLI: 200
CLI->>Browser: Open OAuth login URL
Browser->>Server: POST /api/v1/auth/cli/login/start (scopes)
Server->>DB: Insert challenge
Server-->>Browser: OAuth redirect / code
Browser->>Server: POST /api/v1/auth/cli/login/exchange
Server->>DB: Create API key
Server-->>CLI: { apiKey }
CLI->>Server: GET /api/v1/projects
Server->>DB: Query projects
Server-->>CLI: [projects]
CLI->>CLI: scanContentFiles / inferSchema / detectLocale / generateConfig
CLI->>Server: PUT /api/v1/schema (schema payload + x-mdcms-schema-hash)
Server->>DB: validate & persist schema sync
Server-->>CLI: { schemaHash }
CLI->>CLI: write .mdcms/schema/<proj>.<env>.json
sequenceDiagram
participant User
participant CLI as mdcms CLI
participant Store as Local Store
participant Server
User->>CLI: mdcms push --validate
CLI->>Store: readSchemaState()
Store-->>CLI: { schemaHash }
CLI->>CLI: build push plan (changed / new / deleted)
CLI->>CLI: validate frontmatter against schema
alt validation fails
CLI->>User: print validation errors
else validation passes
CLI->>Server: PUT /api/v1/content/:id (with schemaHash, draftRevision)
alt stale revision / schema mismatch / conflict
Server-->>CLI: 409 { code }
CLI->>User: record [FAILED] and continue
else success
Server-->>CLI: 200
CLI->>Store: flush manifest update
end
CLI->>Server: POST /api/v1/content (for new)
Server-->>CLI: 201 { id }
CLI->>Store: add manifest entry
CLI->>Server: DELETE /api/v1/content/:id (for deletions)
Server-->>CLI: 204
CLI->>Store: remove manifest entry
CLI->>User: print summary [UPDATED/CREATED/DELETED]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Apr 8, 2026
Docstrings generation was requested by @woywro. * #34 (comment) The following files were modified: * `apps/cli/src/lib/config.ts` * `apps/cli/src/lib/discover.ts` * `apps/cli/src/lib/framework.ts` * `apps/cli/src/lib/init.ts` * `apps/cli/src/lib/init/detect-locale.ts` * `apps/cli/src/lib/init/generate-config.ts` * `apps/cli/src/lib/init/git-cleanup.ts` * `apps/cli/src/lib/init/infer-schema.ts` * `apps/cli/src/lib/init/prompt.ts` * `apps/cli/src/lib/init/scan.ts` * `apps/cli/src/lib/login.ts` * `apps/cli/src/lib/pull.ts` * `apps/cli/src/lib/push.ts` * `apps/cli/src/lib/schema-state.ts` * `apps/cli/src/lib/schema-sync.ts` * `apps/cli/src/lib/status.ts` * `apps/cli/src/lib/validate.ts` * `apps/server/src/lib/auth.ts` * `apps/server/src/lib/content-api/database-store.ts` * `apps/server/src/lib/content-api/in-memory-store.ts` * `apps/server/src/lib/content-api/parsing.ts` * `apps/server/src/lib/content-api/responses.ts` * `apps/server/src/lib/content-api/routes.ts` * `apps/server/src/lib/demo-seed.ts` * `apps/server/src/lib/projects-api.ts` * `apps/server/src/lib/rbac.ts` * `apps/server/src/lib/runtime-with-modules.ts` * `packages/shared/src/lib/contracts/schema-hash.ts` * `packages/shared/src/lib/contracts/schema.ts`
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/lib/framework.ts (1)
288-339:⚠️ Potential issue | 🟡 MinorFix type mismatch:
serverUrlcan be undefined whenrequiresTargetis falseThe
resolveExecutionContextfunction return type declaresserverUrl: string(non-optional), but the implementation only validates and enforces a value wheninput.requiresTargetis true. This allowsserverUrlto beundefinedfor commands likelogin(whererequiresTarget: false), even though the type system promises it will always be a string. The login command usescontext.serverUrldirectly in URL interpolation without null checks, which would fail at runtime if undefined.Either validate
serverUrlregardless ofrequiresTarget, or update the return type toserverUrl: string | undefinedand handle undefined values in callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/framework.ts` around lines 288 - 339, The function resolveExecutionContext currently promises serverUrl: string but only enforces presence when input.requiresTarget is true; change the function's return type to make serverUrl optional (serverUrl: string | undefined) and update all callers (e.g., commands like login that use context.serverUrl) to handle the undefined case explicitly (guard before interpolation or provide a fallback) so runtime errors are avoided; keep the existing validation branch for requiresTarget as-is and ensure TypeScript types and any related interfaces referencing resolveExecutionContext are updated to reflect the optional serverUrl.
🧹 Nitpick comments (13)
apps/cli/src/lib/config.ts (1)
67-81: Good error handling, consider preserving the original error cause.The error extraction logic correctly handles various error shapes. For improved debuggability, consider preserving the original error as a
causeproperty in theRuntimeErrordetails.💡 Optional: Preserve original error for debugging
throw new RuntimeError({ code: "CONFIG_LOAD_FAILED", message: `Failed to load config from "${configPath}": ${message}`, statusCode: 400, - details: { configPath }, + details: { configPath, cause: error }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/config.ts` around lines 67 - 81, The catch block that builds and throws the RuntimeError (throw new RuntimeError({...})) should preserve the original error as a cause: capture the caught error (currently referred to as error / message) and include it in the RuntimeError construction using the cause property (e.g., cause: error) or include it inside details (e.g., details: { configPath, cause: error }) so callers can inspect the original exception; update the RuntimeError invocation in the catch block that references configPath and message to attach the original error as the cause.apps/cli/src/lib/init/git-cleanup.ts (1)
64-98: Function is markedasyncbut uses synchronous operations.
detectTrackedFilesis declaredasyncbut only uses synchronousexecSync/execFileSynccalls. This is misleading and adds unnecessary promise wrapping overhead.♻️ Remove unnecessary async
-export async function detectTrackedFiles( +export function detectTrackedFiles( cwd: string, managedDirectories: string[], -): Promise<string[]> { +): string[] {🤖 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.ts` around lines 64 - 98, The function detectTrackedFiles is declared async but only uses synchronous execSync/execFileSync calls; change its signature to a synchronous function (remove the async keyword and change the return type from Promise<string[]> to string[]) and keep the current synchronous logic (execSync/execFileSync) inside; also update any callers of detectTrackedFiles to stop awaiting it (handle the returned string[] directly) so there’s no needless Promise wrapping.apps/server/src/lib/content-api.test.ts (1)
1971-1971: Remove unusednoopCsrfvariable.The
noopCsrffunction is defined in all three CMS-151 tests but never used. It appears to be leftover boilerplate.🧹 Suggested cleanup
test("CMS-151: stale draftRevision is rejected with 409 STALE_DRAFT_REVISION", async () => { const handler = createHandler(); - const noopCsrf = (headers: Record<string, string> = {}) => headers; const created = await createContentDocument(handler, noopCsrf, scopeHeaders, { + const created = await createContentDocument(handler, (h) => h, scopeHeaders, {Apply similar changes to the other two tests.
Also applies to: 2025-2025, 2063-2063
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/content-api.test.ts` at line 1971, Remove the unused noopCsrf helper from the CMS-151 tests: delete the const noopCsrf = (headers: Record<string, string> = {}) => headers; declarations wherever they appear in the CMS-151 test blocks in content-api.test.ts (there are three occurrences), leaving the rest of the tests unchanged.packages/shared/src/lib/contracts/schema-hash.ts (1)
19-21: Consider sorting object keys for guaranteed deterministic hashing.
JSON.stringifydoes not guarantee consistent key ordering for objects across all JavaScript engines. While theresolvedSchemais explicitly sorted inserializeResolvedEnvironmentSchema, therawConfigSnapshotobject (especially nestedlocales.aliases) may have non-deterministic key ordering.For reliable cross-environment hash consistency, consider using a sorted JSON serializer:
♻️ Suggested approach
+function stableStringify(value: unknown): string { + return JSON.stringify(value, (_, v) => + v && typeof v === "object" && !Array.isArray(v) + ? Object.fromEntries(Object.entries(v).sort(([a], [b]) => a.localeCompare(b))) + : v + ); +} + export function buildSchemaSyncPayload( config: ParsedMdcmsConfig, environment: string, ): SchemaRegistrySyncPayload { const rawConfigSnapshot = toRawConfigSnapshot(config); const resolvedSchema = serializeResolvedEnvironmentSchema( config, environment, ); const schemaHash = createHash("sha256") - .update(JSON.stringify({ environment, rawConfigSnapshot, resolvedSchema })) + .update(stableStringify({ environment, rawConfigSnapshot, resolvedSchema })) .digest("hex"); return { rawConfigSnapshot, resolvedSchema, schemaHash }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/lib/contracts/schema-hash.ts` around lines 19 - 21, The hash generation in schemaHash uses JSON.stringify on { environment, rawConfigSnapshot, resolvedSchema } which can produce non-deterministic key order; create a deterministic serialization step (e.g., deep-sort object keys or use a stable stringify) and apply it to rawConfigSnapshot and the full payload before calling createHash in schemaHash; ensure serializeResolvedEnvironmentSchema's sorted output is used or re-sorted as part of this deterministic serializer so the resulting digest("hex") is stable across JS engines.apps/cli/src/lib/login.ts (1)
312-322: Verify the hardcoded scopes list is complete and consider externalizing.The scopes array is hardcoded directly in the login flow. While this works, consider:
- Is this the complete set of scopes the CLI needs?
- Should this be a constant or configurable for different CLI use cases?
The current scopes appear comprehensive for typical CLI operations (projects, schema, content read/write/delete).
🔧 Optional: Extract scopes to a named constant
+const CLI_LOGIN_SCOPES = [ + "projects:read", + "projects:write", + "schema:read", + "schema:write", + "content:read", + "content:read:draft", + "content:write", + "content:write:draft", + "content:delete", +] as const; + // In the login flow: body: JSON.stringify({ redirectUri: listener.redirectUri, state, - scopes: [ - "projects:read", - "projects:write", - "schema:read", - "schema:write", - "content:read", - "content:read:draft", - "content:write", - "content:write:draft", - "content:delete", - ], + scopes: CLI_LOGIN_SCOPES, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/login.ts` around lines 312 - 322, The hardcoded scopes array in the login flow should be validated and extracted: confirm the list (projects:read/write, schema:read/write, content:read, content:read:draft, content:write, content:write:draft, content:delete) covers all CLI operations, then move that array into a named constant (e.g., DEFAULT_OAUTH_SCOPES) and expose it as configurable (environment variable, config file, or CLI flag) so callers can override for different use cases; update the code that currently constructs the OAuth request (the scopes array in login.ts) to reference the constant and respect any provided override.apps/cli/src/lib/schema-state.test.ts (1)
3-4: Consider consolidating path imports.The
dirnameandjoinfunctions are imported fromnode:pathon separate lines. This works but could be cleaner.🔧 Suggested consolidation
-import { dirname } from "node:path"; -import { join } from "node:path"; +import { dirname, join } from "node:path";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/schema-state.test.ts` around lines 3 - 4, Consolidate the two separate imports from "node:path" into a single import that includes both dirname and join; locate the import statements that currently import dirname and join separately and replace them with one line importing { dirname, join } from "node:path" so the code is cleaner and avoids duplicate module imports.packages/shared/src/lib/contracts/schema.test.ts (1)
484-494: Consider if this type-usability test adds value.This test only verifies that
SchemaStateFilecan be used as a type annotation and that property access works. TypeScript compilation already validates this. Unless there's a specific concern about the type export, this test may be redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/lib/contracts/schema.test.ts` around lines 484 - 494, The test "SchemaStateFile type is usable as a value type" is redundant because TypeScript already enforces type annotations at compile time; remove this test block (the test referencing SchemaStateFile) or replace it with a meaningful runtime check (e.g., validate an actual runtime parser/validator function for the expected shape) if you intended to assert runtime behavior—locate the test by the name or the SchemaStateFile symbol and either delete the entire test function or swap it for a runtime validation that calls your shape validator instead.apps/cli/src/lib/schema-state.ts (1)
33-37: Consider validating the parsed JSON structure.The
JSON.parse(raw) as SchemaStateFilecast trusts the file content without validation. If the file is malformed but valid JSON (e.g.,{}), it could lead to runtime errors when accessing expected properties downstream.A Zod schema or simple property check could catch corruption early:
💡 Optional validation example
try { - return JSON.parse(raw) as SchemaStateFile; + const parsed = JSON.parse(raw); + if (typeof parsed?.schemaHash !== "string") { + return undefined; + } + return parsed as SchemaStateFile; } 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 - 37, Replace the unchecked cast "JSON.parse(raw) as SchemaStateFile" with runtime validation: parse raw, validate the resulting object against a SchemaStateFile shape (either with a Zod schema like schemaStateFileSchema or explicit property/type checks for required fields) and only return the object when validation passes; if validation fails, return undefined (and optionally log the parse/validation error). Ensure you add/consume a validator symbol (e.g., schemaStateFileSchema or validateSchemaStateFile) next to the existing JSON.parse(raw) call so downstream code guaranteed receives a correctly shaped SchemaStateFile.apps/cli/src/lib/init/generate-config.ts (1)
36-55: Field names and type names should be validated or escaped.
renderTypeinterpolatestype.name,type.directory, and fieldnamedirectly. While these are typically alphanumeric, malformed input could break the generated config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/lib/init/generate-config.ts` around lines 36 - 55, renderType currently interpolates type.name, type.directory and unquoted field identifiers directly into the generated config which can produce invalid output for malformed names; update renderType to validate or escape these values: sanitize or validate InferredType.name (and fallback or throw on invalid chars) and escape type.directory using a safe string-escaping helper (e.g., use a function that JSON-encodes or escapes quotes/backslashes) and ensure field keys are rendered safely by quoting them (or validating they match identifier regex) before emitting (refer to renderType, InferredType, and renderFieldValue where field names are iterated and emitted).apps/cli/src/lib/init.test.ts (2)
205-205: Unused variablestderrOutput- consider removing or asserting.The
stderrOutputvariable is declared but never used. Consider adding an assertion likeassert.ok(stderrOutput.includes("503"))to verify the error message, or remove the variable.🤖 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` at line 205, The test declares an unused variable stderrOutput; either remove the declaration or add an assertion to use it (e.g., assert that stderrOutput contains the expected error code/message). Locate the variable stderrOutput in init.test.ts and either delete the let stderrOutput = "" line or replace it by asserting its contents (for example using assert.ok or expect) where the test captures stderr, ensuring the test actually verifies the error output.
129-129: Unused variablestdoutOutput- consider removing or asserting.The
stdoutOutputvariable captures stdout but is never used in assertions. Either remove it or add assertions to verify expected output messages.Suggested fix
- let stdoutOutput = ""; - const command = createInitCommand({ prompter, fetcher, skipAuth: true, }); const exitCode = await runMdcmsCli(["init"], { cwd, commands: [command], - stdout: { - write: (chunk: string) => { - stdoutOutput += chunk; - }, - }, + stdout: { write: () => undefined }, stderr: { write: () => undefined }, fetcher, });🤖 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` at line 129, The test declares a unused variable stdoutOutput in init.test.ts; either remove its declaration and any associated stdout capture setup, or add assertions that validate expected CLI output (e.g., assert stdoutOutput contains the expected strings) so the variable is used; locate the stdoutOutput variable and update the surrounding test body (the test function and any mocking of process.stdout or capture helpers) accordingly.apps/cli/src/lib/init/scan.ts (2)
92-98: Silent failure on parse errors may hide issues.While treating malformed frontmatter as empty is reasonable for a scan function, consider logging a debug-level warning so users can identify files with parsing issues during init.
Optional enhancement
let frontmatter: Record<string, unknown> = {}; try { const content = await readFile(fullPath, "utf-8"); const parsed = parseMarkdownDocument(content); frontmatter = parsed.frontmatter; - } catch { + } catch (err) { // Malformed frontmatter — treat as empty + // Could optionally log: console.debug(`Skipped malformed frontmatter in ${relPath}`); }🤖 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 92 - 98, The try-catch around readFile/parseMarkdownDocument currently swallows parse errors silently; modify the catch in the scan routine to log a debug-level message containing the file identifier (use fullPath) and the caught error before continuing and treating frontmatter as empty. Locate the block that calls readFile(fullPath, "utf-8") and parseMarkdownDocument(...) and add a processLogger.debug or logger.debug (matching the existing logger in scope) inside the catch to record `fullPath` and `error` while keeping the existing behavior of setting frontmatter to empty.
61-111: Sequential file reads may slow down large content directories.The scanner reads files sequentially. For repositories with many content files, consider batching reads with
Promise.all(with concurrency limits) for better performance.🤖 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 61 - 111, The walkDirectory function currently reads each markdown file sequentially; change it to batch file reads per directory by collecting file-entry tasks and using a concurrency-limited Promise.all to parse files in parallel (e.g., use p-limit or a small worker pool). Keep directory recursion (walkDirectory) synchronous per-directory traversal but for each directory build an array of async handlers that call readFile(fullPath, "utf-8"), parseMarkdownDocument(content) and detectLocaleHint(relPath, frontmatter), then await the limited Promise.all and push results into results; ensure error handling for malformed frontmatter remains (catch per-task) and preserve creation of frontmatterKeys and format logic.
🤖 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/package.json`:
- Around line 40-42: The package lists "zod" under devDependencies but the CLI
runtime (see apps/cli/src/lib/init.ts which calls z.string(), z.number(), etc.)
loads zod at runtime, so move "zod": "^4.3.6" from devDependencies into
dependencies in package.json (remove it from devDependencies and add it under
dependencies with the same version), then update the lockfile / reinstall so the
dependency is included for published installs.
In `@apps/cli/src/lib/discover.ts`:
- Around line 55-61: The relative path comparison fails on Windows because
relative(input.cwd, absolutePath) yields backslashes; update the loop that
computes relativePath (used with trackedPaths.has and untracked.push) to
normalize separators to forward slashes before checking/adding (same approach as
scan.ts: import sep from "node:path" and replace all occurrences of sep with "/"
on the relativePath), ensuring trackedPaths.has(relativePath) and later
consumers (e.g., push.ts splitting logic) receive POSIX-style paths.
In `@apps/cli/src/lib/init.ts`:
- Line 494: The call to await prompter.confirm("Confirm inferred types?")
ignores the user's answer; capture its boolean result into a variable and, if
false, abort the init flow (return/throw) or loop back to let the user revise
types before any side effects; specifically, update the code around
prompter.confirm in init.ts to: const confirmed = await prompter.confirm(...);
if (!confirmed) return (or re-prompt) so that subsequent operations that write
mdcms.config.ts or call syncSchema()/importContent() do not run.
- Around line 638-649: When setting locale for an imported file in the block
that currently checks file.localeHint, change the resolution to consult the
user-provided alias map before falling back to the raw value: look up
localeConfig?.aliases[file.localeHint.rawValue] and use that mapped value if
present, otherwise call normalizeLocale(file.localeHint.rawValue) and then fall
back to file.localeHint.rawValue; update the code that assigns locale (the
variable named locale in this block) to use this alias-first logic so files like
post.english.md honor detectLocaleConfig() mappings.
In `@apps/cli/src/lib/init/detect-locale.ts`:
- Around line 100-108: The code only sets type.localized when typeLocales.size
>= 2; change the check to >= 1 so any type whose filenames encode a locale (even
a single locale like *.en.md or en/) is marked localized. Update the condition
involving typeLocales, type.localized and hasMultiLocaleType in detect-locale.ts
(the block that currently uses "if (typeLocales.size >= 2)") to use ">= 1" so
those types are treated as localized and later have their locale suffix/folder
stripped.
In `@apps/cli/src/lib/init/generate-config.ts`:
- Around line 84-86: The generated locale lines currently interpolate
lc.defaultLocale and lc.supported without escaping, so inject malicious
characters or break the output; update the code in generate-config.ts where
lines.push is called for the default and supported entries to emit properly
escaped JSON strings (e.g., use JSON.stringify or an equivalent string-escaping
helper) for lc.defaultLocale and for the array of lc.supported values (serialize
the whole array with JSON.stringify instead of manual map/join) so both default
and supported locale values are safely escaped in the generated config.
- Around line 73-78: The template construction in generate-config.ts uses direct
interpolation (lines.push calls referencing input.project, input.environment,
input.serverUrl, and input.contentDirectories.map(...)) which can break if
values contain quotes; update those pushes to escape values safely by passing
each user value through JSON.stringify (including each element inside
input.contentDirectories) before inserting into the generated TypeScript so the
produced strings are properly quoted and escaped.
In `@apps/cli/src/lib/init/git-cleanup.ts`:
- Around line 100-131: Remove the unnecessary async/Promise typing from
untrackFiles and detectTrackedFiles since they only use execFileSync/execSync
and return synchronously: drop the async keyword and change the return types
from Promise<string[]> (or Promise<...>) to plain string[] (or the appropriate
non-promise type) and return the value directly. Update all call sites that
currently await these functions to call them synchronously (remove await) —
specifically the two production places that call untrackFiles/detectTrackedFiles
and the corresponding test that awaits them. Keep the existing parsing logic (no
regex changes) and ensure exported signatures and tests reflect the non-async
return types.
In `@apps/cli/src/lib/init/infer-schema.test.ts`:
- Around line 20-38: The inferSchema implementation currently marks all inferred
fields optional; update the logic in inferSchema where field metadata is
constructed (use the tracked count for each field) so that optional is set to
count < fileCount rather than always true; locate the block that builds each
field entry (references: inferSchema, the local count variable, and the
fileCount property) and change the optional assignment to evaluate whether the
field appears in every file.
In `@apps/cli/src/lib/pull.ts`:
- Around line 407-413: The code deletes the old file (rm(join(input.context.cwd,
change.previousPath))) before writing the replacement, so if
writeContentFile(...) fails the local copy is lost; change the sequence for
move/rename cases (where change.status === "Moved/Renamed" || "Moved/Renamed
(locally modified)" and change.previousPath exists) to first create/write the
new content (call writeContentFile for change.path in the target cwd) and only
after successful write remove the previousPath; apply the same ordering fix to
the other similar block that removes previousPath around writeContentFile
(ensure removal is skipped on write failure).
In `@apps/cli/src/lib/push.ts`:
- Around line 46-52: The deletion flow is sending unconditional DELETEs and
ignores the draftRevision/publishedVersion carried in DeletionCandidate, so make
deletes conflict-aware by passing the revision constraints through to the server
and failing if they don't match: update deleteDocument(...) (and the analogous
logic at the other locations flagged) to accept a DeletionCandidate (or include
draftRevision/publishedVersion) and send them with the delete request (e.g., as
If-Match/If-Unmodified-Since headers or as body/query params per API) so the
server can reject stale deletions; on conflict, surface a clear error and skip
removing the local file. Ensure all call sites that build DeletionCandidate use
the updated signature.
- Around line 876-899: The catch block around resolveCreatePayload(...) is
catching all errors and always emitting a "no content type maps to directory"
warning; change it to only treat the specific "no type mapping" error as a
warning and rethrow or surface other errors (e.g. malformed localized filenames
/ INVALID_LOCAL_DOCUMENT). Specifically, inside the try/catch that calls
resolveCreatePayload (used to build newCandidates), inspect the thrown error
(error.code, error.name or a sentinel from resolveCreatePayload) and: if it
matches the type-mapping-not-found case, emit the current warning using
file.path and dir; otherwise rethrow the error (or write an appropriate error
message) so malformed filenames and INVALID_LOCAL_DOCUMENT failures are not
silently swallowed.
In `@apps/cli/src/lib/validate.ts`:
- Around line 123-128: The date branch currently accepts any string; update the
"date" case in validate.ts to actually validate date content by allowing Date
instances or non-empty strings that parse to valid dates (e.g., use
Date.parse/new Date and check !isNaN(date.valueOf())). Reject empty strings and
strings that don't produce a valid date, returning an error using the existing
message format with path and typeof value. Ensure you reference the same "case
'date'", value and path variables so behavior and return shape remain unchanged.
In `@apps/server/src/bin/demo-seed-api-key.ts`:
- Around line 19-27: DEMO_KEY_SCOPES currently includes mutating scopes
(content:write, content:delete, schema:write); change seeding to require
explicit opt-in or fail in non-local environments by: make the default
DEMO_KEY_SCOPES read-only (only content:read, content:read:draft, schema:read),
add an explicit boolean guard (e.g., DEMO_SEED_ALLOW_MUTATING or check NODE_ENV
=== "development"/LOCAL flag) before appending mutating scopes to
DEMO_KEY_SCOPES, and if not local and the opt-in flag is missing, throw or exit
to hard-fail; update the code paths that read DEMO_KEY_SCOPES to use this gated
value.
In `@apps/server/src/lib/auth.ts`:
- Around line 3455-3485: The bug is that an empty metadata.contextAllowlist is
treated like "no restrictions" allowing unrestricted keys; change the logic so
empty arrays are treated as "no allowed contexts" (deny), not skip checks:
update the guard around metadata.contextAllowlist and apiKeyAllowsTarget so that
apiKeyAllowsTarget([]) returns false and the code path that currently skips RBAC
when contextAllowlist.length === 0 instead enforces checks; also ensure
assertSessionCanIssueApiKeyScopes continues to perform RBAC evaluation when an
allowlist is empty (i.e., do not short-circuit on empty lists) and add a clear
error when attempting to mint an API key with an empty allowlist for
project/environment-bound keys.
- Around line 3807-3825: The issue is that environment: "*" is stored but
apiKeyAllowsTarget() does exact matches, so project-scoped CLI keys created in
this block (see createApiKeyForUser invocation and normalizeRequestedCliScopes
call) won't match real environments; fix by making the created key use
undefined/null for unspecified environment (instead of the literal "*") or
update apiKeyAllowsTarget() to treat the stored "*" as a wildcard; in practice
prefer changing this creation site to set environment: undefined when
challenge.environment is not provided (and adjust any DB schema handling) or
alternatively implement wildcard logic in apiKeyAllowsTarget() to accept "*" as
matching any target environment.
In `@apps/server/src/lib/content-api/routes.ts`:
- Around line 161-218: The loop currently returns from inside for (const doc of
response.data), so only the first document is processed; fix by changing the
control flow to: 1) iterate over response.data and call
stripUnknownFrontmatterFields(doc.frontmatter, schemaCache.get(doc.type)) for
every doc (use the existing stripUnknownFrontmatterFields and schemaCache logic
inside the loop but do not return), 2) move the options.resolveUsers block out
of the per-document loop so uniqueUserIds = [...new Set(response.data.flatMap(d
=> [d.createdBy, d.updatedBy]))] and await options.resolveUsers(...) runs once
and assigns to response.users, 3) after processing all docs, check if
resolvePaths.length === 0 and return response if so, otherwise call
prepareResolvePlan({ scope, store: options.store, documentType: resolvedType!,
paths: resolvePaths }) once and then replace response.data with
Promise.all(response.data.map(document => applyResolvePlan({ authorize:
options.authorize, request, requiredScope, scope, store: options.store, draft,
document, plan: resolvePlan }))); ensure no early returns inside the
per-document loop and use the existing function names
stripUnknownFrontmatterFields, prepareResolvePlan, and applyResolvePlan.
In `@apps/server/src/lib/projects-api.ts`:
- Around line 222-252: Add a DB-level unique constraint on (projectId, name) for
the environments table and change the create flow to handle insert conflicts
atomically: keep or remove the preflight select (optional) but update the
insertion of environments to either use an upsert/ON CONFLICT handler or catch
the unique-violation error from the DB and rethrow a RuntimeError with code
"INVALID_INPUT" and the same message/details used for duplicates (use the same
message template: Environment with name "${name}" already exists for project
"${normalizedSlug}" and include details: { project: normalizedSlug, name }).
Reference symbols: environments (table), the preflight select that builds
existing, the insert call that returns created, and the RuntimeError with code
"INVALID_INPUT".
In `@docs/adrs/ADR-006-schema-hash-pinning-for-write-clients.md`:
- Line 5: The ADR's metadata field "last_updated" is stale; update the
"last_updated" YAML field in ADR-006-schema-hash-pinning-for-write-clients.md to
the actual revision/acceptance date (e.g., 2026-04-08) so the document reflects
the PR edit date for traceability.
In `@docs/specs/SPEC-008-cli-and-sdk.md`:
- Line 117: Update the description at the earlier occurrence that says
credentials are "keyed by `serverUrl + project + environment`" so it matches the
later detailed behavior: change it to state the credential store is keyed by
`serverUrl` only (remove `+ project + environment`), and clarify that login is
not scoped to project/environment; ensure any references to scoping in the
wizard or "credential store" text use the `serverUrl`-only key consistently
throughout the document.
---
Outside diff comments:
In `@apps/cli/src/lib/framework.ts`:
- Around line 288-339: The function resolveExecutionContext currently promises
serverUrl: string but only enforces presence when input.requiresTarget is true;
change the function's return type to make serverUrl optional (serverUrl: string
| undefined) and update all callers (e.g., commands like login that use
context.serverUrl) to handle the undefined case explicitly (guard before
interpolation or provide a fallback) so runtime errors are avoided; keep the
existing validation branch for requiresTarget as-is and ensure TypeScript types
and any related interfaces referencing resolveExecutionContext are updated to
reflect the optional serverUrl.
---
Nitpick comments:
In `@apps/cli/src/lib/config.ts`:
- Around line 67-81: The catch block that builds and throws the RuntimeError
(throw new RuntimeError({...})) should preserve the original error as a cause:
capture the caught error (currently referred to as error / message) and include
it in the RuntimeError construction using the cause property (e.g., cause:
error) or include it inside details (e.g., details: { configPath, cause: error
}) so callers can inspect the original exception; update the RuntimeError
invocation in the catch block that references configPath and message to attach
the original error as the cause.
In `@apps/cli/src/lib/init.test.ts`:
- Line 205: The test declares an unused variable stderrOutput; either remove the
declaration or add an assertion to use it (e.g., assert that stderrOutput
contains the expected error code/message). Locate the variable stderrOutput in
init.test.ts and either delete the let stderrOutput = "" line or replace it by
asserting its contents (for example using assert.ok or expect) where the test
captures stderr, ensuring the test actually verifies the error output.
- Line 129: The test declares a unused variable stdoutOutput in init.test.ts;
either remove its declaration and any associated stdout capture setup, or add
assertions that validate expected CLI output (e.g., assert stdoutOutput contains
the expected strings) so the variable is used; locate the stdoutOutput variable
and update the surrounding test body (the test function and any mocking of
process.stdout or capture helpers) accordingly.
In `@apps/cli/src/lib/init/generate-config.ts`:
- Around line 36-55: renderType currently interpolates type.name, type.directory
and unquoted field identifiers directly into the generated config which can
produce invalid output for malformed names; update renderType to validate or
escape these values: sanitize or validate InferredType.name (and fallback or
throw on invalid chars) and escape type.directory using a safe string-escaping
helper (e.g., use a function that JSON-encodes or escapes quotes/backslashes)
and ensure field keys are rendered safely by quoting them (or validating they
match identifier regex) before emitting (refer to renderType, InferredType, and
renderFieldValue where field names are iterated and emitted).
In `@apps/cli/src/lib/init/git-cleanup.ts`:
- Around line 64-98: The function detectTrackedFiles is declared async but only
uses synchronous execSync/execFileSync calls; change its signature to a
synchronous function (remove the async keyword and change the return type from
Promise<string[]> to string[]) and keep the current synchronous logic
(execSync/execFileSync) inside; also update any callers of detectTrackedFiles to
stop awaiting it (handle the returned string[] directly) so there’s no needless
Promise wrapping.
In `@apps/cli/src/lib/init/scan.ts`:
- Around line 92-98: The try-catch around readFile/parseMarkdownDocument
currently swallows parse errors silently; modify the catch in the scan routine
to log a debug-level message containing the file identifier (use fullPath) and
the caught error before continuing and treating frontmatter as empty. Locate the
block that calls readFile(fullPath, "utf-8") and parseMarkdownDocument(...) and
add a processLogger.debug or logger.debug (matching the existing logger in
scope) inside the catch to record `fullPath` and `error` while keeping the
existing behavior of setting frontmatter to empty.
- Around line 61-111: The walkDirectory function currently reads each markdown
file sequentially; change it to batch file reads per directory by collecting
file-entry tasks and using a concurrency-limited Promise.all to parse files in
parallel (e.g., use p-limit or a small worker pool). Keep directory recursion
(walkDirectory) synchronous per-directory traversal but for each directory build
an array of async handlers that call readFile(fullPath, "utf-8"),
parseMarkdownDocument(content) and detectLocaleHint(relPath, frontmatter), then
await the limited Promise.all and push results into results; ensure error
handling for malformed frontmatter remains (catch per-task) and preserve
creation of frontmatterKeys and format logic.
In `@apps/cli/src/lib/login.ts`:
- Around line 312-322: The hardcoded scopes array in the login flow should be
validated and extracted: confirm the list (projects:read/write,
schema:read/write, content:read, content:read:draft, content:write,
content:write:draft, content:delete) covers all CLI operations, then move that
array into a named constant (e.g., DEFAULT_OAUTH_SCOPES) and expose it as
configurable (environment variable, config file, or CLI flag) so callers can
override for different use cases; update the code that currently constructs the
OAuth request (the scopes array in login.ts) to reference the constant and
respect any provided override.
In `@apps/cli/src/lib/schema-state.test.ts`:
- Around line 3-4: Consolidate the two separate imports from "node:path" into a
single import that includes both dirname and join; locate the import statements
that currently import dirname and join separately and replace them with one line
importing { dirname, join } from "node:path" so the code is cleaner and avoids
duplicate module imports.
In `@apps/cli/src/lib/schema-state.ts`:
- Around line 33-37: Replace the unchecked cast "JSON.parse(raw) as
SchemaStateFile" with runtime validation: parse raw, validate the resulting
object against a SchemaStateFile shape (either with a Zod schema like
schemaStateFileSchema or explicit property/type checks for required fields) and
only return the object when validation passes; if validation fails, return
undefined (and optionally log the parse/validation error). Ensure you
add/consume a validator symbol (e.g., schemaStateFileSchema or
validateSchemaStateFile) next to the existing JSON.parse(raw) call so downstream
code guaranteed receives a correctly shaped SchemaStateFile.
In `@apps/server/src/lib/content-api.test.ts`:
- Line 1971: Remove the unused noopCsrf helper from the CMS-151 tests: delete
the const noopCsrf = (headers: Record<string, string> = {}) => headers;
declarations wherever they appear in the CMS-151 test blocks in
content-api.test.ts (there are three occurrences), leaving the rest of the tests
unchanged.
In `@packages/shared/src/lib/contracts/schema-hash.ts`:
- Around line 19-21: The hash generation in schemaHash uses JSON.stringify on {
environment, rawConfigSnapshot, resolvedSchema } which can produce
non-deterministic key order; create a deterministic serialization step (e.g.,
deep-sort object keys or use a stable stringify) and apply it to
rawConfigSnapshot and the full payload before calling createHash in schemaHash;
ensure serializeResolvedEnvironmentSchema's sorted output is used or re-sorted
as part of this deterministic serializer so the resulting digest("hex") is
stable across JS engines.
In `@packages/shared/src/lib/contracts/schema.test.ts`:
- Around line 484-494: The test "SchemaStateFile type is usable as a value type"
is redundant because TypeScript already enforces type annotations at compile
time; remove this test block (the test referencing SchemaStateFile) or replace
it with a meaningful runtime check (e.g., validate an actual runtime
parser/validator function for the expected shape) if you intended to assert
runtime behavior—locate the test by the name or the SchemaStateFile symbol and
either delete the entire test function or swap it for a runtime validation that
calls your shape validator instead.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 493bcf6f-7981-4e6c-b5d4-65209b33fa0f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
apps/cli/package.jsonapps/cli/src/lib/config.tsapps/cli/src/lib/discover.test.tsapps/cli/src/lib/discover.tsapps/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/pull.test.tsapps/cli/src/lib/pull.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/parsing.tsapps/server/src/lib/content-api/responses.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/rbac.tsapps/server/src/lib/runtime-with-modules.tsblazebot/memory/CMS-181.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
Collaborator
Author
|
@CodeRabbit pause |
✅ Actions performedReviews paused. |
…d nullable migration
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
mdcms initwizard (project/environment creation, schema inference, optional import, git integration)mdcms schema syncwith persisted local schema state and hashmdcms statusshowing content and schema driftpush/pull(new/delete handling, validation, smarter plans) and draft-revision conflict detectionBug Fixes
Documentation