fix(cli): make quality gate pass#96
Closed
iipanda wants to merge 3 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR is mostly documentation and formatting edits across the repo; the primary functional change defers reading/validating local schema state in the CLI push flow until immediately before content writes (so dry-run/no-write paths can proceed without local state). All other edits are presentational (MDX/Markdown/CSS/JSX/test formatting). Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Preflight
participant State
participant Interactive
participant Writer
User->>CLI: run `mdcms push` [--dry-run] [--sync-schema]
CLI->>Preflight: runSchemaPreflight()
Preflight-->>CLI: preflight result
CLI->>State: readSchemaState() %% initial read retained
State-->>CLI: initial schema state or missing
Note over CLI,State: Final schema-state check happens immediately before writes
CLI->>State: readSchemaState() %% final attempt before writes
alt schema state present
State-->>CLI: schema state loaded
else schema state missing
alt interactive or --sync-schema
CLI->>Interactive: prompt / performSchemaSync()
Interactive-->>CLI: synced schema state
CLI->>State: readSchemaState() %% re-read after sync
State-->>CLI: schema state loaded or still missing
else
CLI-->>CLI: throw SCHEMA_STATE_MISSING
end
end
alt --dry-run
CLI->>Writer: applyPush(dryRun=true, schemaHash)
Writer-->>CLI: dry-run summary
else normal push
CLI->>Writer: applyPush(dryRun=false, schemaHash)
Writer-->>CLI: push complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/docs/api-reference/media-and-webhooks.mdx (1)
364-375:⚠️ Potential issue | 🟠 MajorFix webhook signature verification to decode hex and guard length before
timingSafeEqual.Line 397 currently compares UTF-8 string interpretations of hex values, which breaks HMAC verification and can throw
RangeErrorwhen buffer lengths differ. Bothsignature(from the header) andexpected(from.digest("hex")) are hex-encoded strings that must be decoded and validated before passing totimingSafeEqual.Proposed fix
// Constant-time comparison - return timingSafeEqual(Buffer.from(signature), Buffer.from(expected)); + // Constant-time comparison (hex-decoded, equal length required) + let providedBuf: Buffer; + let expectedBuf: Buffer; + try { + providedBuf = Buffer.from(signature, "hex"); + expectedBuf = Buffer.from(expected, "hex"); + } catch { + return false; + } + + if (providedBuf.length !== expectedBuf.length) { + return false; + } + + return timingSafeEqual(providedBuf, expectedBuf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/api-reference/media-and-webhooks.mdx` around lines 364 - 375, In verifyWebhookSignature, the header-derived signature and the expected digest are currently compared as UTF-8 strings which breaks HMAC verification and can throw RangeError; fix by parsing the hex string values (the header part -> signature and the computed expected from .digest("hex")) into binary Buffers using Buffer.from(..., "hex"), validate that both buffers are defined and have equal length, and only then call crypto.timingSafeEqual(expectedBuf, signatureBuf); if decoding fails or lengths differ return false. Ensure you reference the parts object, the signature variable extracted from it, the expected digest from the HMAC, and the timingSafeEqual call in verifyWebhookSignature when making the change.
🧹 Nitpick comments (1)
apps/docs/api-reference/errors.mdx (1)
35-35: Line 35: sentence fragment in table description.
May be empty {}reads as a fragment in the table row. Consider making it a full sentence for consistency with the rest of the doc prose.Suggested wording
-| `details` | object | Additional context specific to the error type. May be empty `{}`. | +| `details` | object | Additional context specific to the error type. This field may be empty (`{}`). |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/api-reference/errors.mdx` at line 35, Replace the sentence fragment in the table row for `details` (the line containing "| `details` | object | Additional context specific to the error type. May be empty `{}`. |") with a full sentence for consistency—for example: "Additional context specific to the error type. It may be empty ({})." Update that table cell so the description reads as two full sentences or a single grammatically complete sentence.
🤖 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/push.ts`:
- Around line 1647-1652: Remove the stale fallback to initialSchemaState when
reading the schema state: replace the nullish-coalescing use of
initialSchemaState so that schemaState is assigned directly from
readSchemaState(...) (i.e., let schemaState = await readSchemaState({...})), and
ensure any downstream code that used schemaState handles the undefined case
explicitly instead of silently resurrecting the old hash; this change affects
the variable schemaState and the call to readSchemaState.
In `@apps/docs/guide/cli/commands.mdx`:
- Around line 181-184: Update the Warning block text that currently states "Push
requires a valid schema hash..." to clarify that the schema-hash requirement
only applies to real write operations: change the message to say push will fail
without a previously synced local schema hash when performing actual writes, but
that dry-run/no-write invocations (e.g., using --dry-run) do not require local
schema state and can proceed; locate and edit the Warning block containing that
sentence in apps/docs/guide/cli/commands.mdx to reflect this scoped behavior.
In `@apps/docs/guide/concepts.mdx`:
- Line 138: Edit the table row describing `path` so the fragment "Can be renamed
without losing history." becomes a complete sentence by adding an explicit
subject (e.g., change it to "It can be renamed without losing history.") in the
cell that defines `path` to improve clarity.
---
Outside diff comments:
In `@apps/docs/api-reference/media-and-webhooks.mdx`:
- Around line 364-375: In verifyWebhookSignature, the header-derived signature
and the expected digest are currently compared as UTF-8 strings which breaks
HMAC verification and can throw RangeError; fix by parsing the hex string values
(the header part -> signature and the computed expected from .digest("hex"))
into binary Buffers using Buffer.from(..., "hex"), validate that both buffers
are defined and have equal length, and only then call
crypto.timingSafeEqual(expectedBuf, signatureBuf); if decoding fails or lengths
differ return false. Ensure you reference the parts object, the signature
variable extracted from it, the expected digest from the HMAC, and the
timingSafeEqual call in verifyWebhookSignature when making the change.
---
Nitpick comments:
In `@apps/docs/api-reference/errors.mdx`:
- Line 35: Replace the sentence fragment in the table row for `details` (the
line containing "| `details` | object | Additional context specific to the
error type. May be empty `{}`. |") with a full sentence for consistency—for
example: "Additional context specific to the error type. It may be empty ({})."
Update that table cell so the description reads as two full sentences or a
single grammatically complete sentence.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b165aef-d6f2-474d-8483-a15ee48a0175
📒 Files selected for processing (56)
AGENTS.mdapps/cli/scripts/build.tsapps/cli/src/lib/push.test.tsapps/cli/src/lib/push.tsapps/docs/agent-instructions.mdxapps/docs/api-reference/authentication.mdxapps/docs/api-reference/content.mdxapps/docs/api-reference/errors.mdxapps/docs/api-reference/media-and-webhooks.mdxapps/docs/api-reference/overview.mdxapps/docs/api-reference/schema.mdxapps/docs/api-reference/sdk.mdxapps/docs/architecture/data-model.mdxapps/docs/architecture/migrations.mdxapps/docs/architecture/module-system.mdxapps/docs/architecture/overview.mdxapps/docs/architecture/request-flow.mdxapps/docs/development/contributing.mdxapps/docs/development/packages.mdxapps/docs/development/setup.mdxapps/docs/development/testing.mdxapps/docs/docs.jsonapps/docs/guide/cli/ci-cd.mdxapps/docs/guide/cli/commands.mdxapps/docs/guide/cli/configuration.mdxapps/docs/guide/cli/installation.mdxapps/docs/guide/concepts.mdxapps/docs/guide/quickstart.mdxapps/docs/guide/schema/defining-types.mdxapps/docs/guide/schema/mdx-components.mdxapps/docs/guide/schema/references.mdxapps/docs/guide/self-hosting.mdxapps/docs/guide/studio/content-editor.mdxapps/docs/guide/studio/dashboard.mdxapps/docs/guide/studio/localization.mdxapps/docs/guide/studio/publishing.mdxapps/docs/guide/studio/settings.mdxapps/docs/index.mdxapps/server/src/lib/auth.test.tsapps/studio-example/README.mdapps/studio-review/README.mddocs/specs/SPEC-013-mintlify-docs-site.mdpackages/modules/README.mdpackages/sdk/README.mdpackages/studio/README.mdpackages/studio/src/lib/runtime-ui/app/admin/page.tsxpackages/studio/src/lib/runtime-ui/app/admin/schema-page.tsxpackages/studio/src/lib/runtime-ui/app/admin/session-context.tsxpackages/studio/src/lib/runtime-ui/app/admin/settings-page.tsxpackages/studio/src/lib/runtime-ui/app/admin/users-page.tsxpackages/studio/src/lib/runtime-ui/components/layout/page-header.tsxpackages/studio/src/lib/runtime-ui/components/ui/badge.tsxpackages/studio/src/lib/runtime-ui/components/ui/card.tsxpackages/studio/src/lib/runtime-ui/styles.csspackages/studio/src/lib/studio-component.tsxpackages/studio/src/lib/studio.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/push.ts`:
- Around line 1660-1662: The confirmation prompt text is misleading: change the
call to context.confirm that currently asks "Sync schema from server before
pushing content?" to accurately reflect the operation performed by
performSchemaSync(...) (which pushes the local schema to the server and updates
local state). Update the prompt wording to something like "Push local schema to
server before pushing content?" or "Push local schema to server and update local
state before pushing content?" wherever context.confirm is used in this branch
so it matches performSchemaSync's behavior.
🪄 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 Plus
Run ID: 0d8ea4b2-c36f-4a3a-a9a1-b517749c2dd2
📒 Files selected for processing (3)
apps/cli/src/lib/push.tsapps/docs/guide/cli/commands.mdxapps/docs/guide/concepts.mdx
✅ Files skipped from review due to trivial changes (1)
- apps/docs/guide/concepts.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/guide/cli/commands.mdx
Comment on lines
+1660
to
+1662
| const accepted = await context.confirm( | ||
| "Sync schema from server before pushing content?", | ||
| ); |
There was a problem hiding this comment.
Prompt wording reverses the sync direction.
This branch calls performSchemaSync(...), which pushes the local schema to the server and then updates local state. Asking users to “Sync schema from server” suggests the opposite operation and can lead to an incorrect confirmation.
✏️ Suggested wording fix
const accepted = await context.confirm(
- "Sync schema from server before pushing content?",
+ "Sync local schema to server before pushing content?",
);Also applies to: 1669-1689
🤖 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 1660 - 1662, The confirmation prompt
text is misleading: change the call to context.confirm that currently asks "Sync
schema from server before pushing content?" to accurately reflect the operation
performed by performSchemaSync(...) (which pushes the local schema to the server
and updates local state). Update the prompt wording to something like "Push
local schema to server before pushing content?" or "Push local schema to server
and update local state before pushing content?" wherever context.confirm is used
in this branch so it matches performSchemaSync's behavior.
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
format:check.SCHEMA_STATE_MISSINGfailure before real push writes if schema sync does not leave readable local state.Verification
bun run format:checkinitially failed on 58 files.bun nx run cli:buildpasses.bun run qualitypasses.git diff --checkpasses.Local ci:required note
bun run ci:requiredreaches unit tests and fails inserver:teston existing server-side cases outside this branch scope:CLI login authorize applies password backoff without authorizing the challengeCLI login authorize GET with expired challenge returns styled HTML error pageCLI login authorize GET with invalid challenge ID returns styled HTML error pageCLI login authorize POST with invalid credentials returns HTML with error messagedemo:seed completes successfully for a fresh staging scopeSummary by CodeRabbit
New Features
Tests
Documentation
Style