feat: content model, state machine, admin UI, and test infrastructure#3
feat: content model, state machine, admin UI, and test infrastructure#3flyingrobots wants to merge 9 commits intomainfrom
Conversation
Rewrite public/index.html as a full CMS dashboard with sidebar tabs
(Drafts/Live), split markdown editor with live preview, metadata editing,
drag-drop uploads, toast notifications, keyboard shortcuts, and autosave.
Fix three bugs: fetchList now sends kind=articles instead of kind=draft,
save() sends {slug, title, body, trailers} instead of {slug, message},
and all POST requests include Content-Type: application/json header.
Fix CmsService compatibility with npm-published @git-stunts packages:
update imports for plumbing ShellRunner and trailer-codec, replace
empty-graph dependency with direct git plumbing calls, add missing
await on async server handlers, and add SVG MIME type to static server.
Add DI seam to CmsService so tests can inject an InMemoryGraphAdapter instead of forking real git subprocesses. Unit tests now run in ~11ms vs hundreds of ms. Real-git smoke tests moved to test/git-e2e.test.js, excluded from default runs.
Adds markdown preview (split/edit/preview modes), autosave, toast notifications, skeleton loading, drag-and-drop uploads, metadata editor, keyboard shortcuts, and dark mode token system.
- Add inline descriptions for demo/quickstart scripts in README - Correct "all scripts are tested" claim in scripts/README (only setup.sh has BATS coverage) - Convert bare file paths to proper markdown links
Summary by CodeRabbit
WalkthroughAdds a content state machine and policy module, DI-backed graph mode in CmsService, unpublish/revert lifecycle operations (service, server endpoints, CLI), expanded tests including real-git E2E and in-memory state-machine tests, and replaces the admin UI with a client-side SPA editor. Also adds package/script and docs updates; new exports include state policy utilities and CmsValidationError; CmsService constructor accepts Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Cms as CmsService
participant Backend as Graph/Git Backend
Note over Client,Server: Unpublish flow
Client->>Server: POST /api/cms/unpublish { slug }
Server->>Cms: unpublishArticle({ slug })
activate Cms
Cms->>Cms: _resolveArticleState(slug)
Cms->>Cms: validateTransition(PUBLISHED, UNPUBLISHED)
Cms->>Backend: delete published ref
Cms->>Backend: update draft ref via _updateRef
deactivate Cms
Cms-->>Server: { slug, sha, state }
Server-->>Client: 200 { result }
Note over Client,Server: Revert flow
Client->>Server: POST /api/cms/revert { slug }
Server->>Cms: revertArticle({ slug })
activate Cms
Cms->>Cms: _resolveArticleState(slug)
Cms->>Backend: read parent SHA/content
Cms->>Backend: create new draft (REVERTED) via _updateRef
deactivate Cms
Cms-->>Server: { slug, sha, state }
Server-->>Client: 200 { result }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 924184313a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/lib/CmsService.js
Outdated
| validateTransition(effectiveState, STATES.UNPUBLISHED); | ||
|
|
||
| // Delete the published ref | ||
| await this.graph.deleteRef(pubRef); |
There was a problem hiding this comment.
unpublishArticle deletes the live published ref before building and committing the replacement draft snapshot. If showNode, commitNode, or _updateRef fails afterward (for example due to a transient git/storage error), the API returns an error even though the article has already been removed from published refs, causing partial state corruption and unintended content takedown.
Useful? React with 👍 / 👎.
| const { effectiveState, draftSha } = await this._resolveArticleState(canonicalSlug); | ||
| validateTransition(effectiveState, STATES.REVERTED); | ||
|
|
||
| const info = await this.graph.getNodeInfo(draftSha); |
There was a problem hiding this comment.
revertArticle calls getNodeInfo(draftSha) without first validating that draftSha is present. For slugs without an articles/<slug> ref (e.g., never saved, or published via explicit sha only), draftSha is null, so this path can throw a backend error and return 500 instead of a structured validation response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 924184313a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/lib/CmsService.js
Outdated
| await this.graph.deleteRef(pubRef); | ||
|
|
||
| // Read current draft content and re-commit with status: unpublished | ||
| const message = await this.graph.showNode(draftSha); |
There was a problem hiding this comment.
Deleting pubRef before creating/updating the unpublished draft makes unpublishArticle non-recoverable on write failures: if commitNode or the later draft ref update throws (for example a signing failure when CMS_SIGN=1), the live ref is already removed and the article now resolves as draft, so retrying unpublish is blocked by invalid_state_transition and requires manual git repair. Commit and repoint the draft first (or add rollback) so a failed unpublish cannot silently drop live content.
Useful? React with 👍 / 👎.
public/index.html
Outdated
| clearTimeout(previewTimer); | ||
| previewTimer = setTimeout(() => { | ||
| const body = document.getElementById('bodyInput').value; | ||
| document.getElementById('previewPane').innerHTML = marked.parse(body || ''); |
There was a problem hiding this comment.
updatePreview writes marked.parse(body) directly to innerHTML without sanitization, so a markdown payload containing raw HTML (for example an element with an event handler) can execute JavaScript when an editor loads or previews that article. This creates a stored XSS path in the admin UI where repository content can run code in an operator’s browser.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/CmsService.js (3)
177-182:⚠️ Potential issue | 🔴 CriticalInconsistent trailer key casing:
updatedAthere vsupdatedatinunpublishArticle/revertArticle.Line 181 writes the trailer key as
updatedAt(camelCase). Lines 239 and 277 write it asupdatedat(all lowercase). These are distinct JS object keys, so any downstream consumer checkingtrailers.updatedAtwill silently getundefinedfor articles touched by unpublish or revert, and vice versa. Pick one form and use it everywhere.🐛 Proposed fix — normalize to `updatedat` everywhere (matching lowercase trailer convention)
const finalTrailers = { ...safeTrailers, contentid: identity.contentId, status: 'draft', - updatedAt: new Date().toISOString(), + updatedat: new Date().toISOString(), };
178-181: 🛠️ Refactor suggestion | 🟠 MajorUses string literal
'draft'instead ofSTATES.DRAFTconstant.You import
STATESon line 14 and use it religiously everywhere else (STATES.UNPUBLISHED,STATES.REVERTED), yet line 180 hard-codes'draft'. One typo and this silently breaks the state machine.♻️ Proposed fix
const finalTrailers = { ...safeTrailers, contentid: identity.contentId, - status: 'draft', + status: STATES.DRAFT, updatedat: new Date().toISOString(), };
198-217: 🧹 Nitpick | 🔵 Trivial
publishArticleresolves state after readingtargetSha, creating a TOCTOU window.Lines 203–204 read
targetShafrom the draft ref. Lines 207–208 then resolve the article state and validate the transition. Between these two operations, another concurrent request could mutate the draft ref. In production mode_updateRefuses CAS to mitigate this, but the validation itself could pass on stale state. Consider resolving state first (or atomically with the read).src/server/index.js (1)
128-202: 🛠️ Refactor suggestion | 🟠 MajorFive POST handlers with identical body-accumulation boilerplate — extract a helper.
snapshot,publish,unpublish,revert, anduploadall repeat the samelet body = ''; req.on('data', ...); req.on('end', async () => { try { ... } catch { ... } })skeleton. This PR adds two more copies. Extract areadJsonBody(req)helper that returns aPromise<object>and handles the accumulation + parse + size limit in one place.♻️ Example helper
const MAX_BODY_BYTES = 1_048_576; // 1 MiB function readJsonBody(req) { return new Promise((resolve, reject) => { let body = ''; let size = 0; req.on('data', (chunk) => { size += chunk.length; if (size > MAX_BODY_BYTES) { req.destroy(); return reject(Object.assign(new Error('Request body too large'), { statusCode: 413 })); } body += chunk; }); req.on('end', () => { try { resolve(JSON.parse(body || '{}')); } catch (e) { reject(Object.assign(new Error('Invalid JSON'), { statusCode: 400 })); } }); req.on('error', reject); }); }This also addresses the pre-existing unbounded body accumulation across all POST endpoints — a trivially exploitable DoS vector.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 1-33: The changelog has an [Unreleased] link anchor but no link
definition; add a Keep-a-Changelog style reference link for [Unreleased] at the
bottom of CHANGELOG.md (e.g., `[Unreleased]: <compare-URL>`), ensuring the
anchor `[Unreleased]` in the header resolves (use your repo compare URL such as
main...HEAD or the appropriate base branch).
In `@public/index.html`:
- Around line 904-914: The global keydown listener unconditionally calls
UI.deselect on Escape and should ignore keyboard events originating from form
controls or editable elements; update the listener registered with
document.addEventListener to early-return when e.target (or
document.activeElement) is an input, textarea, select, or an element with
contentEditable=true (and also when e.defaultPrevented) so UI.deselect only runs
for non-editing contexts, or alternatively attach the handler to the main app
container instead of document to scope it; reference the existing listener,
UI.deselect, state.currentSlug, and the 'slugInput' element when making the
change.
- Around line 858-867: markDirty() currently clears and resets
state.autosaveTimer to call save() after 3s, but if save() takes longer than 3s
the next autosave fires while a previous save is in-flight and is silently
dropped by the state.saving guard, risking lost edits; update save() to, in its
finally block, check state.dirty and re-arm state.autosaveTimer (using the same
timer duration) when a save completed while still dirty (or alternatively
increase the debounce to 5–10s by changing the timeout value used in
markDirty()), and ensure you use the same symbols state.autosaveTimer,
state.saving, state.dirty, markDirty(), and save() when making the change.
- Line 7: The external CDN scripts (e.g., the marked script
"https://cdn.jsdelivr.net/npm/marked@15/marked.min.js" and the Open Props unpkg
scripts referenced on lines 9–11) are included without Subresource Integrity;
update each external <script> and <link> tag to include a correct
integrity="..." SRI hash and crossorigin="anonymous" attribute, or replace those
CDN references by vendoring the files locally and serving them from your repo;
ensure the integrity values match the exact asset contents (regenerate hashes if
you change the files) and add crossorigin="anonymous" alongside each integrity
attribute.
- Around line 924-942: The drop handler is registered on document causing
uploads when files are dropped anywhere; scope it to the actual element to match
the visual feedback. Replace document.addEventListener('drop', ...) with
dropZone?.addEventListener('drop', ...) (guarding for null), keep the
e.preventDefault() and dropZone.classList.remove('dragover'), read files from
e.dataTransfer and call UI.handleUpload(f) as before, and remove the global
document drop listener so only drops on the element referenced by dropZone
trigger uploads; adjust the dragover/dragleave handlers if needed to also be
attached to dropZone rather than document for consistent UX.
- Around line 588-597: The code injects unescaped server data into innerHTML: in
the items.forEach loop where li.innerHTML = `<span
class="slug">${item.slug}</span>`, replace this unsafe insertion by setting a
safe DOM/text value (e.g., use li.textContent or create a span element and set
its textContent or run item.slug through the existing escAttr helper) so the
slug is escaped before rendering; update the block that creates li (in the same
forEach that also sets li.onclick = () => this.loadArticle(item.slug)) to create
a span element, set span.textContent = item.slug (or escAttr(item.slug)), append
the span to li, and remove direct use of innerHTML.
- Around line 748-762: The renderBadges method is injecting untrusted trailer
values (statusVal and updated) via row.innerHTML causing an XSS risk; replace
the HTML string interpolation with DOM construction: clear row, for statusVal
create a span element, set span.className to "badge" plus the computed cls and
set span.textContent = statusVal, append it to row; for updated create a span
with className "badge badge-info" and set its textContent to "Updated: " +
(relTime(updated) || updated), then append—do not use innerHTML or string
concatenation with trailer values in renderBadges.
- Around line 821-838: The current reader.onload handler assumes e.target.result
contains a comma-separated data URL and does b64 =
e.target.result.split(',')[1]; guard this by validating e?.target?.result is a
string and that result.includes(',') and split(',')[1] is truthy before calling
api.upload; if the base64 segment is missing, show a friendly toast error (or
reject) instead of sending "undefined", and avoid proceeding to insert the image
or call updatePreview; update references: reader.onload, e.target.result, b64,
and api.upload.
- Around line 790-797: In removeTrailerRow, the code attempts to remove the
remove-button by positional index after already deleting elements by data-idx,
which uses a stale positional index; delete the statement that calls
grid.querySelectorAll('.remove-btn')[idx]?.remove() and rely on re-collecting
state via this.collectTrailers() and this.renderTrailers() to update the DOM
correctly, ensuring no other code depends on that positional removal.
- Around line 548-554: The updatePreview function currently assigns untrusted
output from marked.parse(...) directly to previewPane.innerHTML, causing stored
XSS; sanitize the generated HTML before assigning it by importing/including
DOMPurify (or configuring marked's sanitizer) and replace the assignment with
something like DOMPurify.sanitize(marked.parse(body || '')) so
previewPane.innerHTML receives only sanitized HTML; locate the updatePreview
function and the innerHTML assignment and wrap the marked.parse result with
DOMPurify.sanitize (or equivalent configured sanitizer).
In `@src/lib/CmsService.js`:
- Around line 89-106: _resolveArticleState re-canonicalizes an already-canonical
slug (calling canonicalizeSlug on every hot-path call); remove that redundant
normalization and use the incoming slug directly. Edit _resolveArticleState to
stop calling canonicalizeSlug (delete the canonicalizeSlug(canonicalSlug) call
and use the slug parameter when computing draftRef/pubRef via _refFor), leaving
the rest (graph.readRef, graph.showNode, codec.decode, resolveEffectiveState)
unchanged; ensure callers continue to pass a canonicalized slug (or add a short
comment if you decide to keep the defensive call instead).
- Around line 66-72: The _updateRef implementation drops oldSha when calling
this.graph.updateRef, which removes compare-and-swap safety present in
this.repo.updateRef; update _updateRef to preserve CAS semantics by calling
this.graph.updateRef with the oldSha when the graph adapter supports it (e.g.,
pass { ref, newSha, oldSha } or call a CAS-specific method on the adapter), and
if the InMemoryGraphAdapter does not support CAS, either throw an explicit error
or add a clear comment and a defensive runtime check (e.g., detect absence of
CAS API on this.graph and log/throw) so tests do not silently hide
concurrent-write issues—refer to the _updateRef method and the
InMemoryGraphAdapter implementation when making the change.
- Around line 120-148: The DI path in listArticles causes N+1 latency by
awaiting this.graph.readRef(ref) in a loop; fix it by replacing the sequential
reads with a parallel approach: after obtaining refs from
this.graph.listRefs(ns) map them to promises (e.g., this.graph.readRef(ref) for
each) and await Promise.all to collect SHAs, or if the graph adapter exposes a
batch API (e.g., readRefs or readMultiple) call that instead; then build results
using refs and the corresponding SHAs and return the array (update symbols:
listArticles, this.graph.listRefs, this.graph.readRef, results, ns).
- Around line 33-60: In DI mode the constructor leaves this.cas and this.vault
null causing uploadAsset to NPE; fix by either requiring and wiring CAS/Vault
stubs from the caller or adding explicit guards: update the constructor({ cwd,
refPrefix, graph, cas, vault }) to set this.cas = cas || null and this.vault =
vault || null (or pull CAS/Vault adapters from the provided
GraphPersistencePort/InMemoryGraphAdapter when available), and/or modify
uploadAsset to check for this.cas and this.vault at the top and throw a clear
Error like "CmsService.uploadAsset requires cas and vault to be provided in DI
mode" instead of letting a TypeError surface; reference the constructor,
uploadAsset, cas, vault, and GraphPersistencePort/InMemoryGraphAdapter when
making the change.
- Around line 222-250: unpublishArticle (and similarly revertArticle) must guard
against draftSha being null before calling this.graph.showNode or
this.graph.getNodeInfo; update unpublishArticle and revertArticle to check the
resolved draftSha (from _resolveArticleState) and if null throw a clear,
descriptive error (e.g., "Missing draft SHA for <slug> — cannot
unpublish/revert") or return a controlled failure instead of calling
showNode/getNodeInfo, so you avoid opaque runtime exceptions and ensure
_updateRef/commit steps only run when draftSha is present.
In `@src/lib/ContentStatePolicy.js`:
- Around line 42-47: resolveEffectiveState currently falls through to
STATES.DRAFT for any unknown draftStatus; change it to validate draftStatus
(allowed: STATES.DRAFT, STATES.UNPUBLISHED, STATES.REVERTED, or null/undefined)
and if pubSha is truthy still return STATES.PUBLISHED, but if draftStatus is
present and not one of the allowed values throw a CmsValidationError containing
the invalid value and a short message; update the function resolveEffectiveState
to perform this check before returning DRAFT so garbage inputs like "banana"
cause a clear validation error rather than silently becoming DRAFT.
- Around line 28-33: The TRANSITIONS constant is currently Object.freeze'd but
still contains mutable Set instances (TRANSITIONS), allowing callers to mutate
state like TRANSITIONS[STATES.DRAFT].add(...); fix by replacing the mutable Sets
with truly immutable collections or defensive wrappers: either replace each new
Set(...) with a frozen array (e.g., [STATES.DRAFT, ...]) and update
validateTransition (or any consumer using TRANSITIONS) to use .includes()
instead of .has(), or create an immutable wrapper around Set (e.g.,
makeImmutableSet that returns a Proxy which throws on mutation or return an
object exposing only a .has/read method) and use that for TRANSITIONS so runtime
mutation attempts fail; ensure you reference TRANSITIONS and validateTransition
(or any code that calls TRANSITIONS[...].has) and update those call sites
accordingly.
In `@test/git-e2e.test.js`:
- Around line 43-51: The test 'propagates underlying git errors while listing'
monkey-patches cms.plumbing.execute but restores it only after the assertion,
which can leak the patch if the assertion throws; wrap the monkey-patch and
assertion in a try/finally so cms.plumbing.execute is always restored, i.e.,
assign originalExecute, set cms.plumbing.execute to the throwing stub, then run
the await expect(cms.listArticles()).rejects... inside the try and restore
cms.plumbing.execute = originalExecute in finally to guarantee cleanup.
In `@test/git.test.js`:
- Around line 175-188: The test currently calls cms.revertArticle twice;
consolidate into a single invocation that captures the thrown error and asserts
all expectations (message, instance, and code) once. Replace the first await
expect(...).rejects.toThrow(...) and the subsequent duplicate try/catch with one
try/catch that awaits cms.revertArticle({ slug: 'sm-no-parent' }), catches the
error, asserts the error message contains "no parent commit exists" (or use a
regex), asserts the error is an instance of CmsValidationError, and asserts
err.code === 'revert_no_parent'. This keeps cms.revertArticle,
CmsValidationError and the 'revert_no_parent' code checks intact while removing
the redundant second call.
- Around line 190-227: Replace the manual try/catch + expect.fail pattern in the
three tests ("cannot unpublish a draft", "cannot revert a published article",
"cannot publish a reverted article") with Vitest's promise rejection matchers:
call the async CMS method inside expect(...).rejects and assert the error type
and code (e.g., toBeInstanceOf(CmsValidationError) and
toHaveProperty('code','invalid_state_transition')), targeting the
cms.unpublishArticle, cms.revertArticle, and cms.publishArticle calls
respectively so the tests are shorter and use the built-in rejects matcher for
failure assertions.
In `@test/server.test.js`:
- Around line 167-211: Both the "unpublishes an article via POST
/api/cms/unpublish" and "reverts an article via POST /api/cms/revert" tests
perform setup fetches to /api/cms/snapshot and /api/cms/publish without
asserting their results; add assertions immediately after each setup fetch
(e.g., check res.ok or expect(res.status).toBe(200)) so silent failures in the
setup (snapshot/publish) surface clearly; update the two tests' initial fetch
calls to capture the response and assert success before proceeding to the final
unpublish/revert request.
In `@vitest.config.js`:
- Line 15: Update the exclude pattern in the vitest config's exclude array:
replace the literal 'test/git-e2e*' with a future-proof glob such as
'test/git-e2e**' so it will exclude both files like test/git-e2e.test.js and any
potential test/git-e2e/ directory contents; modify the value inside the exclude
array where the symbol exclude is defined.
- Around line 8-13: The vitest alias hardcodes the internal path to
InMemoryGraphAdapter which breaks tests and is unmaintainable; open package.json
of `@git-stunts/git-warp` and check its "exports" or main entry to see if
InMemoryGraphAdapter is publicly exported, then remove the long resolve alias in
vitest.config.js and import the adapter via the package's public export (e.g.,
import from '@git-stunts/git-warp/InMemoryGraphAdapter' or the documented
symbol) or, if it isn't exported, revert the alias and instead create an
upstream issue/PR requesting a public export for InMemoryGraphAdapter and update
test/git.test.js to rely only on public exports.
public/index.html
Outdated
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Git CMS Admin</title> | ||
| <script src="https://cdn.jsdelivr.net/npm/marked@15/marked.min.js"></script> |
There was a problem hiding this comment.
CDN scripts loaded without Subresource Integrity (SRI) hashes.
Line 7 loads marked from jsdelivr, and lines 9-11 load Open Props from unpkg — all without integrity attributes. If either CDN is compromised (or serves a poisoned cache), arbitrary code runs in your admin UI with full access to the CMS API. This is an admin panel that can publish content; the blast radius is total.
Add integrity and crossorigin="anonymous" attributes to every external resource, or vendor these dependencies locally.
Also applies to: 9-11
🤖 Prompt for AI Agents
In `@public/index.html` at line 7, The external CDN scripts (e.g., the marked
script "https://cdn.jsdelivr.net/npm/marked@15/marked.min.js" and the Open Props
unpkg scripts referenced on lines 9–11) are included without Subresource
Integrity; update each external <script> and <link> tag to include a correct
integrity="..." SRI hash and crossorigin="anonymous" attribute, or replace those
CDN references by vendoring the files locally and serving them from your repo;
ensure the integrity values match the exact asset contents (regenerate hashes if
you change the files) and add crossorigin="anonymous" alongside each integrity
attribute.
| resolve: { | ||
| alias: { | ||
| '@git-stunts/git-warp/InMemoryGraphAdapter': | ||
| resolve(__dirname, 'node_modules/@git-stunts/git-warp/src/infrastructure/adapters/InMemoryGraphAdapter.js'), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the `@git-stunts/git-warp` package exists and examine its package.json
if [ -d "node_modules/@git-stunts/git-warp" ]; then
echo "=== Package found, checking exports field ==="
cat node_modules/@git-stunts/git-warp/package.json | jq '.exports // "no exports field defined"'
echo ""
echo "=== Full package.json (main, types, exports) ==="
cat node_modules/@git-stunts/git-warp/package.json | jq '{main, types, exports, version}'
else
echo "Package `@git-stunts/git-warp` NOT found in node_modules"
fiRepository: flyingrobots/git-cms
Length of output: 118
🏁 Script executed:
# Check the actual structure of the package to see if the path exists
if [ -d "node_modules/@git-stunts/git-warp" ]; then
echo "=== Directory structure of `@git-stunts/git-warp` ==="
find node_modules/@git-stunts/git-warp -type f -name "*.js" | grep -i "memorygraph\|InMemory" | head -20
echo ""
echo "=== Check if src/infrastructure/adapters/InMemoryGraphAdapter.js exists ==="
ls -la node_modules/@git-stunts/git-warp/src/infrastructure/adapters/InMemoryGraphAdapter.js 2>&1 || echo "File not found"
fiRepository: flyingrobots/git-cms
Length of output: 46
🏁 Script executed:
# Check the main vitest.config.js and see how the alias is used
echo "=== vitest.config.js content ==="
cat vitest.config.jsRepository: flyingrobots/git-cms
Length of output: 628
🏁 Script executed:
# Search for usage of InMemoryGraphAdapter in the codebase
echo "=== Searching for InMemoryGraphAdapter usage ==="
rg "InMemoryGraphAdapter" --type js --type tsRepository: flyingrobots/git-cms
Length of output: 643
Hardcoded internal module path is unmaintainable — and the package isn't even installed.
The alias reaches into node_modules/@git-stunts/git-warp/src/infrastructure/adapters/InMemoryGraphAdapter.js, but the package itself is not installed in your current environment. This already causes the import in test/git.test.js to fail at runtime. Beyond that immediate breakage, hardcoding paths to package internals is a maintenance disaster:
- Any restructuring of
@git-stunts/git-warp(moving files, renaming directories) breaks your tests silently — no compile error, just a runtime module-not-found that's hell to debug. - You're depending on implementation details with zero guarantees of stability.
- This pattern does not scale.
Fix this by using public exports:
- Check if
@git-stunts/git-warpalready exportsInMemoryGraphAdaptervia itsexportsfield inpackage.json. - If yes, use that:
'@git-stunts/git-warp/InMemoryGraphAdapter'→ resolves through the package's own export map (no alias needed, or a minimal one). - If no, open an issue/PR upstream requesting a public export for this adapter instead of reaching into
src/.
Do not ship code that depends on internal node_modules paths. Period.
🤖 Prompt for AI Agents
In `@vitest.config.js` around lines 8 - 13, The vitest alias hardcodes the
internal path to InMemoryGraphAdapter which breaks tests and is unmaintainable;
open package.json of `@git-stunts/git-warp` and check its "exports" or main entry
to see if InMemoryGraphAdapter is publicly exported, then remove the long
resolve alias in vitest.config.js and import the adapter via the package's
public export (e.g., import from '@git-stunts/git-warp/InMemoryGraphAdapter' or
the documented symbol) or, if it isn't exported, revert the alias and instead
create an upstream issue/PR requesting a public export for InMemoryGraphAdapter
and update test/git.test.js to rely only on public exports.
Rename alias to #test/InMemoryGraphAdapter to signal it's a test-only shim. Add TODO noting the upstream gap (not in git-warp's exports map as of v10.8.0). Simplify URL resolution.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/git.test.js`:
- Around line 92-99: Add explicit negative test cases in the "State Machine"
suite to cover the remaining invalid transitions: reverted → unpublish, reverted
→ revert, unpublished → unpublish, and unpublished → revert. For each case, use
the existing CmsService instance (cms) initialized in beforeEach, drive the
state into the starting state (e.g., call cms.revert() or cms.unpublish() as
needed) and then attempt the invalid transition by calling the appropriate
method (cms.unpublish() or cms.revert()), asserting that the call fails
(throw/rejects or returns an error/status) consistent with other
invalid-transition tests; name each spec clearly (e.g., "does not allow reverted
-> unpublish") so the behavior is locked down.
- Around line 10-13: The test suites duplicate the same beforeEach that
constructs new InMemoryGraphAdapter() and cms = new CmsService({ refPrefix,
graph });; refactor by moving that setup into a shared parent describe or a
small factory helper: create a top-level describe wrapping the "CmsService
(Integration)" and "State Machine" describes and place the beforeEach there
(using InMemoryGraphAdapter and CmsService symbols), or extract a helper
function (e.g., makeCms()) that returns { graph, cms } and call it from both
suites to remove the duplicated beforeEach code.
- Around line 193-199: The tests use expect.fail() which Vitest doesn't provide;
update the failing guards in the try/catch blocks that call cms.unpublishArticle
(and the other similar test cases in this file) to either throw a plain
Error('should have thrown') or import Vitest's assert and call
assert.fail('should have thrown'); if you choose assert, add "assert" to the
existing import from 'vitest'; replace every occurrence of expect.fail('should
have thrown') in these tests with the chosen alternative so the test fails
explicitly when no error is thrown.
| describe('State Machine', () => { | ||
| let cms; | ||
| const refPrefix = 'refs/cms'; | ||
|
|
||
| beforeEach(() => { | ||
| const graph = new InMemoryGraphAdapter(); | ||
| cms = new CmsService({ refPrefix, graph }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding coverage for remaining invalid transitions.
The suite covers the core happy paths and three invalid transitions well. A few edge cases remain untested:
reverted → unpublish(presumably invalid)reverted → revert(double revert)unpublished → unpublish(double unpublish)unpublished → revert
These may be implicitly prevented by the state machine, but explicit negative tests would lock down the contract.
🤖 Prompt for AI Agents
In `@test/git.test.js` around lines 92 - 99, Add explicit negative test cases in
the "State Machine" suite to cover the remaining invalid transitions: reverted →
unpublish, reverted → revert, unpublished → unpublish, and unpublished → revert.
For each case, use the existing CmsService instance (cms) initialized in
beforeEach, drive the state into the starting state (e.g., call cms.revert() or
cms.unpublish() as needed) and then attempt the invalid transition by calling
the appropriate method (cms.unpublish() or cms.revert()), asserting that the
call fails (throw/rejects or returns an error/status) consistent with other
invalid-transition tests; name each spec clearly (e.g., "does not allow reverted
-> unpublish") so the behavior is locked down.
…nup) Fixes 20 actionable items from CodeRabbit/Codex review: DOMPurify for stored XSS, unpublish atomicity reorder, null guards, SRI hashes, unknown-status throw, Promise.all for N+1, and test improvements.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 34: Update the changelog entry so the user-facing phrase uses the proper
noun capitalization: change "markdown preview" to "Markdown preview" in the line
that reads "- **(P1) Stored XSS via markdown preview:** Sanitize
`marked.parse()` output with DOMPurify" (referencing the string containing
`marked.parse()` and `DOMPurify` to locate the exact sentence).
- Around line 5-9: Add a blank line before each second-level heading in
CHANGELOG.md that currently lacks surrounding whitespace: specifically insert an
empty line immediately above the "### Added", "### Changed", and "### Fixed"
headings so they comply with markdownlint MD022; update the three occurrences of
the "### Added"/"### Changed"/"### Fixed" headings in the Unreleased section to
be preceded by a blank line.
In `@public/index.html`:
- Around line 13-16: The stylesheet imports in the <style> block use `@import` of
Open Props from unpkg (the three `@import` lines) which cannot use SRI and poses a
supply-chain risk; replace these remote `@import` calls by vendoring the Open
Props files into the repo (download the referenced files: open-props,
normalize.min.css, buttons.min.css), serve them from your static assets, and
update the <style> block to reference the local CSS files (or link them via
local <link rel="stylesheet">) so the imports no longer rely on unpkg and SRI
isn’t required.
- Around line 906-908: The escAttr function currently only escapes &, ", and <;
update escAttr to also replace single quote characters by mapping ' to ' to
prevent injection into single-quoted attributes—modify the escAttr
implementation (the escAttr(s) function) to include .replace(/'/g, ''') in
the replacement chain so all four characters (&, ", <, ') are escaped.
- Around line 7-12: The marked script src uses a floating major tag
(`marked@15`) which will break SRI on any v15 patch update; update the script
tag to pin the exact version that matches the provided integrity hash (e.g.,
replace `marked@15` with the exact release such as `marked@15.0.12`) in the src
URL and keep the existing integrity attribute (or regenerate the integrity hash
to match the chosen exact version), ensuring the `marked` import used by your
markdown preview (calls like marked.parse) continues to load correctly.
In `@src/lib/CmsService.js`:
- Around line 89-105: The method _resolveArticleState currently treats missing
refs as a draft because draftStatus defaults to STATES.DRAFT; add an existence
guard at the start of _resolveArticleState to check both draftSha and pubSha and
throw a CmsValidationError when both are falsy (include code 'article_not_found'
and field 'slug') so callers querying a nonexistent slug get a clear error
instead of state 'draft'; keep the rest of the function (reading message,
decoding, and calling resolveEffectiveState) unchanged and only proceed when at
least one of draftSha or pubSha exists.
- Around line 237-241: The trailers object in unpublishArticle and revertArticle
uses the wrong key casing 'updatedat' causing UI badges to miss it; update both
occurrences in the codec.encode calls inside unpublishArticle and revertArticle
to use 'updatedAt' (matching saveSnapshot and renderBadges expectations), and
scan for any other places writing trailers to ensure they use camelCase
'updatedAt' consistently.
| ## [Unreleased] — git-stunts branch | ||
|
|
||
| ### Added | ||
| - **Content Identity Policy (M1.1):** Canonical slug validation with NFKC normalization, reserved word rejection, and `CmsValidationError` contract (`ContentIdentityPolicy.js`) | ||
| - **State Machine (M1.2):** Explicit draft/published/unpublished/reverted states with enforced transition rules (`ContentStatePolicy.js`) |
There was a problem hiding this comment.
Markdownlint MD022: headings need surrounding blank lines.
Lines 7, 22, and 29 (### Added, ### Changed, ### Fixed) each lack a blank line before them. This will trigger MD022 in CI if markdownlint is enforced.
Proposed fix (showing line 5–9; repeat for 21–22 and 28–29)
## [Unreleased] — git-stunts branch
### Added
+
- **Content Identity Policy (M1.1):** Canonical slug validation with NFKC normalization, reserved word rejection, and `CmsValidationError` contract (`ContentIdentityPolicy.js`)📝 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.
| ## [Unreleased] — git-stunts branch | |
| ### Added | |
| - **Content Identity Policy (M1.1):** Canonical slug validation with NFKC normalization, reserved word rejection, and `CmsValidationError` contract (`ContentIdentityPolicy.js`) | |
| - **State Machine (M1.2):** Explicit draft/published/unpublished/reverted states with enforced transition rules (`ContentStatePolicy.js`) | |
| ## [Unreleased] — git-stunts branch | |
| ### Added | |
| - **Content Identity Policy (M1.1):** Canonical slug validation with NFKC normalization, reserved word rejection, and `CmsValidationError` contract (`ContentIdentityPolicy.js`) | |
| - **State Machine (M1.2):** Explicit draft/published/unpublished/reverted states with enforced transition rules (`ContentStatePolicy.js`) |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 5 - 9, Add a blank line before each second-level
heading in CHANGELOG.md that currently lacks surrounding whitespace:
specifically insert an empty line immediately above the "### Added", "###
Changed", and "### Fixed" headings so they comply with markdownlint MD022;
update the three occurrences of the "### Added"/"### Changed"/"### Fixed"
headings in the Unreleased section to be preceded by a blank line.
| - Slug canonicalization enforced at all API ingress points | ||
| - Admin UI API calls aligned with server contract (query params, response shapes) | ||
| - Server integration test environment stabilized for CI | ||
| - **(P1) Stored XSS via markdown preview:** Sanitize `marked.parse()` output with DOMPurify |
There was a problem hiding this comment.
"markdown" is a proper noun — capitalize it.
markdown preview → Markdown preview. Flagged by LanguageTool; tiny but this is a user-facing changelog.
-- **(P1) Stored XSS via markdown preview:** Sanitize `marked.parse()` output with DOMPurify
+- **(P1) Stored XSS via Markdown preview:** Sanitize `marked.parse()` output with DOMPurify📝 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.
| - **(P1) Stored XSS via markdown preview:** Sanitize `marked.parse()` output with DOMPurify | |
| - **(P1) Stored XSS via Markdown preview:** Sanitize `marked.parse()` output with DOMPurify |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...abilized for CI - (P1) Stored XSS via markdown preview: Sanitize marked.parse() ou...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 34, Update the changelog entry so the user-facing
phrase uses the proper noun capitalization: change "markdown preview" to
"Markdown preview" in the line that reads "- **(P1) Stored XSS via markdown
preview:** Sanitize `marked.parse()` output with DOMPurify" (referencing the
string containing `marked.parse()` and `DOMPurify` to locate the exact
sentence).
| <script src="https://cdn.jsdelivr.net/npm/marked@15/marked.min.js" | ||
| integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+" | ||
| crossorigin="anonymous"></script> | ||
| <script src="https://cdn.jsdelivr.net/npm/dompurify@3.2.4/dist/purify.min.js" | ||
| integrity="sha384-eEu5CTj3qGvu9PdJuS+YlkNi7d2XxQROAFYOr59zgObtlcux1ae1Il3u7jvdCSWu" | ||
| crossorigin="anonymous"></script> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version marked.js npm package 2025
💡 Result:
The latest marked (marked.js) npm package version released in 2025 was v17.0.1, published November 20, 2025. [1] (Also shown as the latest version on jsDelivr’s npm view.) [2]
🌐 Web query:
marked.js v15 latest version npm
💡 Result:
The latest marked.js v15 release on npm is 15.0.12. [1]
npm i marked@15.0.12🌐 Web query:
marked 15.0.12 SRI hash sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+
💡 Result:
That string is in Subresource Integrity (SRI) format: an integrity attribute value using SHA-384 (sha384-…) to pin a specific CDN file (e.g., marked.min.js) so the browser will block it if the contents don’t match. Marked 15.0.12 is a real published version on cdnjs. [1] cdnjs also documents that SRI hashes may be provided as sri values in API responses for assets. [2]
Example usage:
<script
src="https://cdnjs.cloudflare.com/ajax/libs/marked/15.0.12/marked.min.js"
integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+"
crossorigin="anonymous"></script>To verify/generate the SHA-384 SRI for a downloaded file:
openssl dgst -sha384 -binary marked.min.js | openssl base64 -A
# then prefix with: sha384-Sources: [1] [2]
🏁 Script executed:
cat public/index.html | head -20Repository: flyingrobots/git-cms
Length of output: 899
marked@15 is a floating major-version tag—SRI hash will shatter on the next v15 patch release.
The URL https://cdn.jsdelivr.net/npm/marked@15/marked.min.js resolves to whatever the latest v15.x.y is on the CDN. The SRI hash you've provided (sha384-948ahk4...) matches 15.0.12 exactly. The moment 15.0.13 lands, the CDN serves new minified bytes, the integrity check fails, the browser rejects the script, and marked becomes undefined. Your markdown preview feature dies silently—no error, no fallback, just a null reference crash when the code calls marked.parse().
DOMPurify is correctly pinned to @3.2.4—an exact version. Do the same for marked.
Fix
- <script src="https://cdn.jsdelivr.net/npm/marked@15/marked.min.js"
- integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+"
+ <script src="https://cdn.jsdelivr.net/npm/marked@15.0.12/marked.min.js"
+ integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+"
crossorigin="anonymous"></script>📝 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.
| <script src="https://cdn.jsdelivr.net/npm/marked@15/marked.min.js" | |
| integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+" | |
| crossorigin="anonymous"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/dompurify@3.2.4/dist/purify.min.js" | |
| integrity="sha384-eEu5CTj3qGvu9PdJuS+YlkNi7d2XxQROAFYOr59zgObtlcux1ae1Il3u7jvdCSWu" | |
| crossorigin="anonymous"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/marked@15.0.12/marked.min.js" | |
| integrity="sha384-948ahk4ZmxYVYOc+rxN1H2gM1EJ2Duhp7uHtZ4WSLkV4Vtx5MUqnV+l7u9B+jFv+" | |
| crossorigin="anonymous"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/dompurify@3.2.4/dist/purify.min.js" | |
| integrity="sha384-eEu5CTj3qGvu9PdJuS+YlkNi7d2XxQROAFYOr59zgObtlcux1ae1Il3u7jvdCSWu" | |
| crossorigin="anonymous"></script> |
🤖 Prompt for AI Agents
In `@public/index.html` around lines 7 - 12, The marked script src uses a floating
major tag (`marked@15`) which will break SRI on any v15 patch update; update the
script tag to pin the exact version that matches the provided integrity hash
(e.g., replace `marked@15` with the exact release such as `marked@15.0.12`) in
the src URL and keep the existing integrity attribute (or regenerate the
integrity hash to match the chosen exact version), ensuring the `marked` import
used by your markdown preview (calls like marked.parse) continues to load
correctly.
Freeze TRANSITIONS Sets to prevent mutation, add manual CAS check in DI-mode _updateRef, widen vitest exclude glob, add negative transition tests (reverted→unpublish/revert, unpublished→revert), assert server test setup calls, fix CHANGELOG markdown lint and add [Unreleased] link.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/server.test.js (1)
27-32: 🧹 Nitpick | 🔵 TrivialThe env var save/restore boilerplate is screaming for extraction.
Six env vars × 2 operations (save + restore with undefined check) = 36 lines of pure ceremony. This is a maintenance hazard — add a seventh env var and you'll inevitably forget one branch.
♻️ Proposed helper
+ const ENV_KEYS = [ + 'GIT_CMS_REPO', 'PORT', + 'GIT_AUTHOR_NAME', 'GIT_AUTHOR_EMAIL', + 'GIT_COMMITTER_NAME', 'GIT_COMMITTER_EMAIL', + ]; + let savedEnv; beforeAll(async () => { cwd = mkdtempSync(path.join(os.tmpdir(), 'git-cms-server-api-test-')); execFileSync('git', ['init'], { cwd }); execFileSync('git', ['config', 'user.name', 'Test'], { cwd }); execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd }); - previousRepoEnv = process.env.GIT_CMS_REPO; - previousPortEnv = process.env.PORT; - previousAuthorNameEnv = process.env.GIT_AUTHOR_NAME; - previousAuthorEmailEnv = process.env.GIT_AUTHOR_EMAIL; - previousCommitterNameEnv = process.env.GIT_COMMITTER_NAME; - previousCommitterEmailEnv = process.env.GIT_COMMITTER_EMAIL; + savedEnv = Object.fromEntries(ENV_KEYS.map(k => [k, process.env[k]])); process.env.GIT_CMS_REPO = cwd; // ... remaining assignments unchanged }); afterAll(async () => { // ... server/symlink cleanup unchanged ... - if (previousRepoEnv === undefined) { delete process.env.GIT_CMS_REPO; } else { ... } - // ... repeat ×6 ... + for (const [k, v] of Object.entries(savedEnv)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } rmSync(cwd, { recursive: true, force: true }); });Also applies to: 72-101
1 participant
Summary
CmsValidationErrorcontractgraphfor zero-subprocess unit tests (~11ms vs hundreds of ms)@git-stunts/alfredanddocker-guarddeps, dependency integrity checks, ROADMAP, CHANGELOG, formal LaTeX ADR, onboarding scriptsTest plan
npm run test:local— 30 tests pass (3 suites: git, server, chunks)npm run test:git-e2e— 2 real-git smoke tests pass