Git Stunts Prep #2
Open
Conversation
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
Significantly improves the "getting started" experience by adding automated setup scripts, comprehensive documentation, and a dedicated test suite for the setup process. Key changes: - Scripts: Added `setup.sh`, `quickstart.sh`, and `demo.sh` to automate environment bootstrapping and demonstration. - Docs: Added `ADR.md` (Architecture Decision Record), `TESTING_GUIDE.md`, `GETTING_STARTED.md`, and `QUICK_REFERENCE.md`. - CI: Added GitHub Actions workflow (`test-setup.yml`) and BATS test suite (`test/setup.bats`) to verify the setup scripts. - Docker: Updated `docker-compose.yml` and `Dockerfile` to use the parent directory build context, allowing access to the `@git-stunts` sibling dependency. - Config: Added Obsidian vault configuration files.
Signed-off-by: James Ross <james@flyingrobots.dev>
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request introduces a central CmsService abstraction and migrates CLI and server logic to use it, removes several legacy git/asset/secrets utility modules, adds Docker/docker-compose and CI/test infra, many user-facing docs, and interactive setup/demo scripts. Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Cms as CmsService
participant Plumbing as GitPlumbing/@git-stunts/plumbing
participant CAS as `@git-stunts/cas`
participant Vault as `@git-stunts/vault`
CLI->>Cms: saveSnapshot({slug,title,body})
Cms->>Vault: resolve/signingKey? (optional)
Cms->>Plumbing: rev-parse parentSha / create node (empty-tree commit with message)
Plumbing-->>Cms: newSha
Cms->>Plumbing: update-ref refs/_blog/dev/<slug> -> newSha
Plumbing-->>Cms: OK
Cms-->>CLI: { sha: newSha, ref: refs/_blog/dev/<slug> }
CLI->>Cms: publishArticle({slug, sha?})
Cms->>Plumbing: rev-parse publishedRef
Cms->>Plumbing: update-ref --force? (fast-forward enforcement)
Plumbing-->>Cms: prevSha / newPublishedSha
Cms-->>CLI: { ref: refs/_blog/published/<slug>, sha: newPublishedSha }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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: 3171922616
ℹ️ 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".
Comment on lines
78
to
+80
| if (req.method === 'GET' && pathname === '/api/cms/list') { | ||
| const kind = query.kind || 'draft'; | ||
| return send(res, 200, listRefs(kind, { cwd: CWD, refPrefix: REF_PREFIX })); | ||
| const kind = query.kind || 'articles'; | ||
| return send(res, 200, cms.listArticles({ kind })); |
There was a problem hiding this comment.
In src/server/index.js, send immediately JSON.stringifys its payload, but cms.listArticles is async and returns a Promise, so the response body becomes {} and any rejection escapes the surrounding try/catch. The same pattern appears on /api/cms/show, /api/cms/snapshot, and /api/cms/publish, which means clients will receive empty responses (and potential unhandled rejections) whenever those endpoints are used. Await the CmsService methods (and make the req.on('end') callbacks async) before calling send.
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: 80
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
40-69: Consolidate repetitive safety warnings.The same safety message appears twice:
- Lines 40–46:
⚠️ SAFETY WARNING (Docker + low-level Git plumbing risk)- Lines 57–69:
⚠️ SAFETY WARNING (Docker isolation, identical concern)The repetition weakens the message rather than strengthens it. Consider:
- Move the warning to a single, prominent location (e.g., top of Development section), or
- Differentiate the two: one for Quick Start users (brief), one for developers (detailed).
📝 Suggested consolidation approach
Replace lines 40–46 with a brief pointer:
> ⚠️ **SAFETY WARNING:** This tool manipulates Git at a low level. Always use Docker (see [TESTING_GUIDE.md](./TESTING_GUIDE.md)).Keep the detailed explanation at lines 57–69 for developers, or move it to a dedicated Safety section.
Dockerfile (1)
1-45: Add container security and optimization best practices.Static analysis identified three improvements:
- Missing HEALTHCHECK (CKV_DOCKER_2): Add health checks for production stages
- Running as root (CKV_DOCKER_3): Create non-root user for security
- Consecutive RUN statements (DL3059): Consolidate git config commands
While not blocking for dev/test images, these are production-readiness gaps.
🔒 Proposed security and optimization fixes
# Test stage FROM base AS test ENV NODE_ENV=test COPY --from=deps /git-stunts /git-stunts COPY --from=deps /app/node_modules ./node_modules COPY git-cms . -RUN git config --global user.email "bot@git-cms.local" -RUN git config --global user.name "Git CMS Bot" -RUN git config --global init.defaultBranch main +RUN git config --global user.email "bot@git-cms.local" && \ + git config --global user.name "Git CMS Bot" && \ + git config --global init.defaultBranch main + +# Create non-root user +RUN addgroup --system --gid 1001 nodejs && \ + adduser --system --uid 1001 gitcms +USER gitcms + +HEALTHCHECK --interval=30s --timeout=3s \ + CMD node -e "process.exit(0)" || exit 1 + CMD ["npm", "run", "test:local"]bin/git-cms.js (1)
56-59: Error handling omits stack traces.Logging only
err.messageprovides clean output for end users but sacrifices debuggability. Consider logging the full stack trace whenDEBUGorVERBOSEenvironment variables are set, or always include it but write to stderr for proper stream separation.🔍 Enhanced error reporting
} catch (err) { - console.error(`Error: ${err.message}`); + console.error(`Error: ${err.message}`); + if (process.env.DEBUG || process.env.VERBOSE) { + console.error(err.stack); + } process.exit(1); }
🤖 Fix all issues with AI agents
In @.github/workflows/test-setup.yml:
- Around line 22-27: The workflow fails because the test step requires a sibling
repo named git-stunts in the build context; add a step before the "Run BATS
tests" / npm run test:setup step that checks out or clones the git-stunts
repository into a directory named git-stunts (e.g., using actions/checkout@v3
with repository set to the git-stunts repo and path set to git-stunts, or
running git clone into ./git-stunts) so Dockerfile line 21 can find /git-stunts
during the build.
- Line 24: Replace deprecated action versions: update the workflow steps that
reference uses: actions/checkout@v3 and uses: actions/upload-artifact@v3 to
their v4 counterparts (actions/checkout@v4 and actions/upload-artifact@v4) so
the job runs on the supported Node 18 runner; locate the two occurrences in the
.github/workflows/test-setup.yml and change the version tags accordingly,
leaving the rest of each step configuration intact.
In @.gitignore:
- Line 9: The PR message incorrectly says "removes Obsidian directory" while the
change only adds ".obsidian/" to .gitignore (a defensive, untracking-prevention
entry); update the commit/PR title and description to state that you're adding
".obsidian/" to .gitignore as a preventive measure, or if you actually removed a
tracked ".obsidian" directory then instead remove it from the index with git rm
--cached -r .obsidian and commit that removal along with an accurate message;
reference the ".gitignore" change and the ".obsidian/" entry when updating the
text so it correctly reflects whether this is preventive or an actual removal.
In @bin/git-cms.js:
- Line 36: The console message in the conditional that checks items.length uses
"No articles found." with a trailing period which is inconsistent with other
user-facing messages; update the string in the if (items.length === 0) branch to
remove the terminal punctuation (e.g., "No articles found") so it matches the
punctuation style used elsewhere where messages omit a final period.
- Line 8: Extract the magic default 'refs/_blog/dev' into a module-level
constant (e.g., DEFAULT_CMS_REF_PREFIX) and replace the inline literal in the
refPrefix initialization; update the declaration that currently sets const
refPrefix = process.env.CMS_REF_PREFIX || 'refs/_blog/dev' to use the constant
so the default is defined once and easily discoverable.
- Line 62: The script calls the async main() without handling rejection; wrap
the invocation so any rejection is caught and handled: either use top-level
await (if using Node.js version that supports it and project is ES module) or
replace the bare main() call with an explicit invocation that attaches a .catch
handler (or an immediately-invoked async wrapper) which logs the error (e.g.,
via console.error or an existing logger) and exits with a non-zero status;
ensure you reference the main() function so import/argument-destructuring or
other sync errors become caught and produce a clear error log and
process.exit(1).
- Around line 47-51: startServer()'s return value is discarded; capture it
(e.g., const server = startServer()) and attach an 'error' event handler to the
returned http.Server instance that logs the error (including err.message) and
calls process.exit(1); update the 'serve' case where startServer() is invoked to
store the server and add server.on('error', ...) so port conflicts and listen
errors are surfaced and handled.
In @docker-compose.yml:
- Around line 13-14: CI fails because the Docker build expects a sibling repo
"git-stunts" in the build context (see docker-compose.yml context: .. and
Dockerfile's COPY git-stunts /git-stunts at line 21) but
.github/workflows/ci.yml only checks out git-cms; update the CI workflow to
fetch the sibling repo before building: add a second actions/checkout step with
repository set to the git-stunts repo and path "git-stunts" (or otherwise
clone/git fetch it into the parent directory so COPY git-stunts finds it), or
alternatively change the build context/Dockerfile to not rely on a sibling repo;
ensure the workflow runs this checkout step prior to the docker build.
In @Dockerfile:
- Line 21: The Dockerfile's COPY git-stunts line fails in CI because the sibling
repo isn't in the build context; fix by either (A) changing CI workflows (ci.yml
and test-setup.yml) to fetch the sibling repo before building (e.g., checkout
parent and git clone or enable submodules) so docker compose run --rm test has
../git-stunts available, or (B) remove the COPY git-stunts from the Dockerfile
and replace package.json file:../git-stunts/* dependencies with a published
@git-stunts npm package (or versioned dependency) so the build no longer
requires a local sibling; apply the matching change to Dockerfile (remove COPY
git-stunts) and package.json and update CI accordingly.
In @docs/ADR.md:
- Line 363: Duplicate heading "Level 2: Lego Block Responsibilities" appears
twice; rename the second occurrence to reflect that it describes APIs (e.g.,
"Level 2: Lego Block APIs" or "Level 2: Lego Block Interfaces") and update any
internal references or links that point to that heading so they match the new
title; search for the exact heading text "Level 2: Lego Block Responsibilities"
to locate both instances and change the second one and its references
accordingly.
- Around line 1016-1021: The ADR currently cites a drifting file-specific line
number; update the reference so it does not include a fragile file:line location
(e.g., remove "src/lib/CmsService.js:95") and instead reference the method name
used in the diff (updateRef) and/or a stable permalink to the repository version
of the file; ensure the Implementation note reads something like
“Implementation: src/lib/CmsService.js — method updateRef” or includes a
permalink to the exact commit/PR rather than a line number.
- Around line 1008-1015: The doc is inconsistent: it mentions `git update-ref
--stdin` but shows a non-stdin invocation; fix by making the example match the
described mode. Either change the prose to describe the normal CLI form and
remove “--stdin”, keeping the shown command `git update-ref
refs/_blog/articles/hello-world <newSHA> <expectedOldSHA>`, or replace the code
block with the stdin form (e.g. echo "update-ref refs/_blog/articles/hello-world
<newSHA> <expectedOldSHA>" | git update-ref --stdin) so the example and the
description both reference `--stdin`. Ensure the surrounding sentence and the
exit-code statement remain accurate.
- Around line 71-75: The ADR asserts Git content-addressing facts but embeds
SHA-1-specific assumptions; update the "TC-1: Git's Content Addressability
Model" section and every statement that references the empty-tree hash value
4b825dc642cb6eb9a060e54bf8d69288fbee4904 (and the assertions at the locations
referencing SHA-1) to include explicit caveats such as "in SHA-1 object-format
mode" or "using Git's default SHA-1 object format," and add a short subsection
near the SHA-256 migration mention that explains how behavior and literal hashes
change under SHA-256 (including that the empty-tree hash and any hard-coded
SHA-1 values are invalid in SHA-256 mode and must be recalculated or made
configurable).
- Around line 151-160: The server currently calls server.listen(PORT) in
src/server/index.js which binds to 0.0.0.0 by default and the app exposes
unauthenticated write endpoints POST /api/cms/snapshot and POST /api/cms/publish
plus a wildcard CORS header (Access-Control-Allow-Origin: *); fix by either
binding the listener to 127.0.0.1 (call server.listen(PORT, '127.0.0.1') and
update the console.log text) or, preferably, implement token-based auth
middleware applied to the POST /api/cms/snapshot and POST /api/cms/publish
handlers (validate an Authorization header or token param) and replace the
wildcard CORS with a restricted origin list in the CORS middleware/config so
only allowed origins can access write endpoints.
- Around line 365-376: The ADR currently hard-codes versions for modules
(@git-stunts/plumbing, @git-stunts/trailer-codec, @git-stunts/empty-graph,
@git-stunts/cas, @git-stunts/vault) which contradicts the stated independent
versioning; fix by removing the pinned "vX.Y.Z" tokens from the Module list in
the ADR and replace them with either "versioned independently" or a single line
pointing to the canonical source (e.g., package.json or the packages/*
manifests) and add a short TODO to implement automation (CI validation or
monorepo lock) that ensures any documented version stays synchronized; update
the Module 1..5 entries (class and API blocks) to omit literal versions and add
a one-line pointer to the authoritative version source or to the planned CI
check.
In @docs/GETTING_STARTED.md:
- Line 60: The docs contain bare URLs (e.g., "http://localhost:4638") which
violate lint and hinder future rewrites; replace each bare URL occurrence (lines
referenced: the shown line and also lines 442 and 466-467) with either a
Markdown link format [http://localhost:4638](http://localhost:4638) or wrap the
URL in angle brackets <http://localhost:4638> so the linter accepts it and the
link is preserved for rewrites.
- Around line 40-52: Replace the bold pseudo-headings like "Option A: Watch the
Demo (Recommended First Time)" and "Option B: Start the Server" with real
Markdown headings (e.g., "## Option A: Watch the Demo (Recommended First Time)")
and ensure a single blank line before and after each fenced code block (```bash
... ```) to satisfy markdownlint; apply the same fixes for the other occurrences
noted (sections spanning lines 69-75 and 93-113) so all pseudo-headings become
proper headings and all fences have consistent surrounding spacing.
- Around line 373-379: Update the "Permission denied" when running Docker
section to distinguish platforms: state that on macOS/Windows you should ensure
Docker Desktop is running, and on Linux you may need to add your user to the
docker group (the example command `sudo usermod -aG docker $USER`) but note that
this is distro-dependent and may require a different group name, logging out/in
or reboot, or alternatively using sudo to run docker commands; also mention
checking that the Docker daemon is running and that group-based socket access is
configured correctly.
- Around line 282-291: The docs currently hard-code the empty-tree OID
`4b825dc642cb6eb9a060e54bf8d69288fbee4904` (SHA-1) without noting it only
applies to SHA-1 repos; update the GETTING_STARTED text to either (a) explicitly
state that `4b825dc…` is the SHA-1 empty-tree OID and only valid for SHA-1
object-format repos, or (b) replace the hard-coded OID with instructions to
derive the repo’s empty-tree OID (e.g., run git hash-object -t tree /dev/null)
and show that command as the recommended approach so SHA-256 repos get the
correct value.
In @GETTING_STARTED.md:
- Line 27: Update the repository clone URL in GETTING_STARTED.md: replace the
incorrect org `clduab11` with the current PR org `flyingrobots` (i.e., change
the git clone command using the string `git clone
https://github.com/clduab11/git-cms.git` to `git clone
https://github.com/flyingrobots/git-cms.git`) and, if the handoff to Git Stunts
is final, change it instead to the target org `git-stunts` (i.e., `git clone
https://github.com/git-stunts/git-cms.git`), and make the same replacement for
the other occurrence referenced around line 41.
- Line 22: Fix markdownlint violations in GETTING_STARTED.md: add a blank line
immediately after the headings (e.g., "### Option A: Direct Installation" and
the other headings flagged at lines 22, 39, 86, 109) to satisfy MD022; ensure
fenced code blocks have a blank line before and after them at the blocks flagged
(lines 25, 54, 58, 70, 72) to satisfy MD031; and replace the bare URL on line 74
with either an angle-bracket-wrapped URL (<https://...>) or a proper markdown
link to satisfy MD034. Make these edits in the referenced sections so spacing is
consistent across the document.
In @package.json:
- Around line 26-32: Documentation leads users to run "npm install" before the
repo's sibling git-stunts packages exist, causing ENOENT failures; update
GETTING_STARTED.md to instruct running "npm run setup" (which runs setup.sh and
clones git-stunts) before "npm install", or alternatively add a postinstall
script in package.json to invoke setup.sh automatically (e.g., use the
"postinstall" npm lifecycle to run setup.sh), or publish the @git-stunts
packages to npm to remove the local file: dependencies; reference package.json
dependencies block, GETTING_STARTED.md, and setup.sh when making the change so
users run/setup the git-stunts repos before installing.
In @QUICK_REFERENCE.md:
- Around line 183-200: The Troubleshooting section starting at the "## 🐛
Troubleshooting" heading has missing blank lines around headings and fenced code
blocks; fix QUICK_REFERENCE.md by ensuring there is a blank line before and
after the "## 🐛 Troubleshooting" heading and before/after each fenced code
block (the bash block containing `npm run setup # Clones git-stunts
automatically`, the yaml block showing the ports mapping, and the final
instruction line about Docker daemon), so each heading and each
```bash```/```yaml```/``` blocks are separated by an empty line from surrounding
text.
- Around line 118-130: The markdown code block under the "## 📄 Commit Message
Format" section lacks a language specifier (MD040); update the opening fence
from ``` to ```text so the commit message example (starting with "# My Article
Title" and containing fields like Status/Author/Tags/Slug/UpdatedAt) is
annotated for syntax highlighting, leaving the block content and closing fence
unchanged.
- Around line 104-114: The architecture diagram code block in QUICK_REFERENCE.md
is missing a language specifier; update the opening fence from ``` to ```text
for the block containing the "git-cms └─ CmsService..." ASCII diagram so it
renders consistently (ensure the closing ``` remains) and matches other code
blocks in the document.
- Around line 20-28: The table row for the `npm run dev` command contains a bare
URL; wrap the bare URL in Markdown link syntax to satisfy MD034 by replacing the
plain "http://localhost:4638" text with a Markdown link (e.g., use the URL as
both link text and target like [http://localhost:4638](http://localhost:4638) or
provide a short label such as [Local server](http://localhost:4638)), updating
the table entry so the `npm run dev` Purpose cell contains the properly
formatted link.
- Around line 56-66: Add required blank lines and language tags to satisfy
MD022/MD031/MD040: ensure there is a blank line before the "## 🎯 The Core
Concept" heading and before each "### Traditional CMS" and "### Git CMS"
heading, insert a blank line immediately before each fenced code block, and
change the fences from ``` to ```text (or another appropriate language) for the
two code blocks containing "Article → Database Row → SQL Query" and "Article →
Commit Message → git log" so they validate as fenced code with a language
specifier.
- Around line 1-100: The doc has incorrect ref patterns and CLI syntax; update
QUICK_REFERENCE.md to use the actual namespace-aware ref pattern
"{refPrefix}/articles/{slug}" and "{refPrefix}/published/{slug}" (and state that
refPrefix defaults to refs/_blog/dev configurable via CMS_REF_PREFIX), correct
the CLI examples to show that "node bin/git-cms.js list" has no --kind flag (and
note that filtering by kind is done via the HTTP API GET
/api/cms/list?kind=published), change the chunks ref example to use
"{refPrefix}/chunks/{slug}@current" (not a hardcoded refs/_blog/chunks/...), add
the undocumented npm run serve / node bin/git-cms.js serve entry, and clarify
that the draft command (node bin/git-cms.js draft <slug> "<Title>") reads
article content from stdin rather than as a positional argument so users must
pipe or echo content into it.
In @README.md:
- Around line 27-35: The README recommends running npm scripts that may not
exist; update the README to make the demo and quickstart entry points
discoverable by either adding a fallback suggestion to run npm run to list
available scripts or by briefly describing what npm scripts "demo" and
"quickstart" do; specifically mention the script names "demo" and "quickstart"
and provide one-line descriptions for each (e.g., what the demo shows and what
quickstart performs) or instruct users to run npm run if those scripts are
missing so they can see available options.
In @REPO_WALKTHROUGH.md:
- Around line 5-61: The markdown has lint violations (missing blank lines around
headings MD022 and inconsistent list indentation MD007) and also contains
outdated content that needs rewrite; update REPO_WALKTHROUGH.md by inserting a
blank line before and after each top-level heading (e.g., "1. Core Philosophy:
The \"Empty Tree\" Database", "2. Refspace Organization", etc.) and normalize
list indentation to use consistent two-space or single-tab indentation across
nested bullets (fix the lists under the Evidence/NOTE blocks), then perform a
content refresh: revise any stale references, links, or notes (like the NOTE
entries about missing docs) so the file accurately reflects current code
(referencing symbols like EMPTY_TREE, refFor, writeSnapshot, parse.js,
chunks.js, encryptBuffer, resolveSecret, and the server/index.js GIT_CMS_REPO
mention) while preserving the documented structure.
- Around line 1-61: The walkthrough references removed/renamed modules
(src/lib/git.js, src/lib/parse.js, src/lib/chunks.js, src/lib/secrets.js) and
stale GitHub URLs; update REPO_WALKTHROUGH.md to remove or replace those module
references with the new CmsService class and the @git-stunts packages (mention
CmsService and any public methods used instead of
writeSnapshot/parse/encryptBuffer/resolveSecret), correct all repository links
to the canonical flyingrobots/git-cms, and adjust descriptions (e.g.,
commit/asset/secret handling) to map to CmsService APIs and @git-stunts
implementations; optionally run a quick filesystem check script to detect any
remaining references to the legacy filenames and include its results in the PR
description.
In @scripts/demo.sh:
- Line 16: The script uses plain read invocations like read -p "Press Enter to
start..." which triggers ShellCheck SC2162 and can mangle backslashes; update
each interactive read call in scripts/demo.sh (e.g., the occurrences that prompt
with -p) to use read -r -p so backslashes are read literally (apply the same
change to the other read occurrences noted in the comment).
- Around line 177-182: The wording in the demo text conflates commit object IDs
with cryptographic signatures; update the echo line that currently reads "Is
cryptographically signed (SHA-1 hash)" to either "Is identified by a
cryptographic hash (SHA-1)" or make it conditional on actual signing config
(e.g., reference commit.gpgSign), so the script accurately states that commits
are identified by a SHA-1 hash rather than being GPG/SSH-signed.
- Around line 34-37: Replace the current probe that runs the bare "docker
compose" command in the conditional with a check that invokes the "docker
compose version" subcommand (silencing output as before) so the conditional
actually fails when Compose V2 is absent; update the if-branch to print the same
error message and exit non‑zero when that version check fails.
In @scripts/quickstart.sh:
- Line 73: The read invocation currently accepts user input with read -p "Choose
[1-6]: " choice which allows backslashes to be treated as escape sequences;
change it to use read -r -p "Choose [1-6]: " choice so backslashes are not
mangled and input is handled defensively, updating the read command that sets
the variable choice.
- Around line 75-130: Cases 1 and 4 run blocking foreground commands (the docker
compose up app and docker compose logs -f app) but do not terminate the script
after they're interrupted, so execution falls through and later prints the "🎉
Done!" message; update the case blocks for choice 1 and choice 4 to explicitly
exit the script (e.g., exit 0) immediately after the blocking docker commands
(after docker compose up app and after docker compose logs -f app) so the script
does not continue to the final success message on Ctrl+C or normal completion.
In @scripts/README.md:
- Around line 43-46: Update the README bullet that currently reads "Fast-forward
only updates (no force pushes)" to use a hyphenated form: change it to
"Fast-forward-only updates (no force pushes)"; ensure the exact bullet under the
"Creates a local 'Stargate' gateway repository..." section is replaced so the
documented Git rule reads as a single hyphenated phrase.
- Around line 18-22: The README has markdownlint issues: ensure there is a blank
line before and after each fenced code block (e.g., the Usage: ```bash
./scripts/quickstart.sh ``` block) and replace bare URLs with proper Markdown
links or angle-bracketed URLs (apply the same fixes for the other occurrences
noted around lines 31-35 and 48-56). Update the Usage block and the other
sections so each fenced code block is preceded and followed by a blank line and
change any bare URL text to either [label](https://example.com) or
<https://example.com>.
- Around line 5-6: The README claim "All scripts are tested!" is incorrect;
update the sentence to accurately reflect test coverage by either removing the
blanket claim or listing the tested scripts — for example change it to "Only
scripts/setup.sh is tested (see test/setup.bats)" or enumerate the tested
scripts if more are added; reference the existing test file test/setup.bats and
the script names scripts/setup.sh, scripts/bootstrap-stargate.sh,
scripts/demo.sh, and scripts/quickstart.sh when making the edit so consumers
know which scripts lack coverage.
In @scripts/setup.sh:
- Around line 99-101: The current block that prints directory structure uses the
pipeline "ls -ld ../git-cms ../git-stunts | awk '{print $9}'" which is
unreliable; replace that pipeline by directly printing the known paths instead
of parsing ls output—update the snippet around the echo "Directory structure:"
line and remove the ls ... | awk command, using a direct printf/echo invocation
to output "../git-cms" and "../git-stunts" (preserving any desired formatting)
so you print the literal paths reliably.
- Around line 83-95: The script assumes the cloned repo will be named "git-cms"
and does a hardcoded cd git-cms; instead, set or compute a CLONE_DIR and use it
when cloning and changing directories: choose a variable like CLONE_DIR (or
derive it from the repo URL by taking basename and stripping .git), call git
clone <URL> "$CLONE_DIR", check the clone result, and then cd "$CLONE_DIR"
(replace the hardcoded cd git-cms and the git clone call accordingly) so the
script works regardless of the actual target directory name.
- Around line 28-31: The current check uses `command -v docker compose` which
fails for the Docker CLI plugin; update the startup check in the if-block that
currently tests `docker compose` to instead run `docker compose version` (or
`docker compose --version`) and test its exit status, capturing stderr if
needed, so the script correctly detects the Compose plugin and prints the
existing error message and exits when the command fails; modify the if condition
and keep the echo "❌ Docker Compose not found. Please install Docker Desktop."
and `exit 1` behavior in the same block.
In @src/lib/CmsService.js:
- Around line 89-93: The current check process.env.CMS_SIGN === '1' used when
calling this.graph.createNode (sign: process.env.CMS_SIGN === '1') is fragile
and will miss values like "true" or "yes"; replace it with a robust boolean
parser (e.g., a small helper like parseEnvBool or normalize string-to-bool) that
treats "1", "true", "yes" (case-insensitive) as true and all other values as
false, and use that parsed boolean for the sign option passed to
this.graph.createNode.
- Line 136: The ref string is hardcoded as `refs/_blog/chunks/${slug}@current`
which ignores the configured this.refPrefix; change the construction of `ref`
(the `ref` variable used when uploading chunks) to use `this.refPrefix` (e.g.
`${this.refPrefix}/chunks/${slug}@current`), guarding for an empty or
trailing-slash in this.refPrefix so you don't double or miss slashes; update any
code that references `ref` in the upload/chunk methods to use the new variable.
- Around line 120-123: The call to this.vault.resolveSecret (assigning
encryptionKeyRaw using envKey 'CHUNK_ENC_KEY' and vaultTarget
`git-cms-${ENV}-enc-key`) can throw and is currently unhandled; wrap the
resolveSecret call in a try/catch around the block that sets encryptionKeyRaw
(or explicitly document propagation) so Vault errors produce a clear, contextual
error message or are rethrown with added context; in the catch log or throw a
new Error that includes the envKey/vaultTarget and the original error to aid
debugging.
- Line 86: The code silently overwrites caller-provided reserved keys because
finalTrailers spreads the user-provided trailers before forcing status and
updatedAt; update the function to either (A) add JSDoc above the function that
explains that status and updatedAt are system-managed and will be overwritten,
or (B) add explicit input validation right before creating finalTrailers (check
the trailers object for keys 'status' and 'updatedAt' and either strip them with
a clear comment or throw/return a validation error) so callers cannot silently
override those fields; reference the trailers input and the finalTrailers
construction when implementing the change.
In @src/server/index.js:
- Around line 104-114: The POST handler calls cms.publishArticle({ slug, sha })
but does not await the Promise, so send(res, 200, result) will send a Promise
object; change the request end callback to be async (or otherwise await the
call) and await cms.publishArticle(...) before calling send; also wrap the await
in try/catch to return an appropriate error (e.g., send 500 with error) if
publishArticle throws, and keep the slug validation and response flow intact.
- Line 130: The code builds assetUrl using result.manifest.chunks[0].digest
which assumes chunks[0] exists; add a defensive check on result.manifest and
result.manifest.chunks length and on chunks[0].digest before using it (e.g., if
chunks missing or digest absent, log or throw a controlled error or use a safe
fallback path), update the location where assetUrl is assigned (the assetUrl
variable construction) to use the validated/determined digest or handle the
missing-case flow consistently so it cannot throw when the CAS returns an empty
chunks array.
- Around line 91-101: The POST handler is calling cms.saveSnapshot() without
awaiting it, causing a Promise to be sent to the client; change the
req.on('end', ...) callback to an async function and await cms.saveSnapshot({
slug, title, body: content, trailers }) before calling send(res, 200, result),
and wrap the parse/save/send steps in try/catch to send a 4xx/5xx error on JSON
parse or save failures rather than leaking a Promise.
- Around line 78-81: The GET '/api/cms/list' branch is returning a Promise
because cms.listArticles is async; change the call to await the result (e.g.,
const articles = await cms.listArticles({ kind }); return send(res, 200,
articles);) and ensure the surrounding request handler function is declared
async so await can be used.
- Around line 84-88: The handler for the GET '/api/cms/show' is returning a
Promise because cms.readArticle({ slug, kind }) is not awaited; make the
enclosing request handler async (so you can use await), change the return to
await cms.readArticle({ slug, kind: kind || 'articles' }) and then send the
awaited result (e.g., const article = await cms.readArticle(...); return
send(res, 200, article)); also wrap the await in a try/catch and use send(res,
500, { error: 'failed to read article', details: err.message }) on errors to
avoid leaking a raw Promise to the client.
- Around line 29-39: The current path-traversal check using
filePath.startsWith(PUBLIC_DIR) is insufficient against symlinks; update the
validation to resolve symlinks and canonicalize both PUBLIC_DIR and the
requested path using fs.realpathSync (or fs.realpath) inside a try/catch, then
verify the resolved request path starts with the resolved PUBLIC_DIR before
proceeding; reference variables/file names: PUBLIC_DIR, filePath, req.url, and
use realpathSync/realpath to obtain safeResolvedPublicDir and
safeResolvedFilePath and perform the startsWith check on those.
In @test/chunks.test.js:
- Around line 43-63: The test "handles encrypted uploads" sets
process.env.CHUNK_ENC_KEY but only deletes it at the end, risking leak if
assertions fail; update the test so process.env.CHUNK_ENC_KEY is always cleared
(either wrap the body of the it block in a try/finally that deletes
process.env.CHUNK_ENC_KEY in the finally, or add an afterEach that clears
process.env.CHUNK_ENC_KEY for the file), referencing the same test and the
cms.uploadAsset invocation to ensure the cleanup runs regardless of assertion
failures.
- Line 18: CmsService.uploadAsset currently hardcodes the chunk ref to
"refs/_blog/chunks/${slug}@current" and ignores the configured refPrefix; update
uploadAsset to build the ref using the instance config (e.g., use this.refPrefix
+ '/_blog/chunks/${slug}@current' or equivalent) so it honors the refPrefix
property, and then update the test in test/chunks.test.js to expect git
rev-parse against the configured refPrefix (e.g., "git rev-parse
<refPrefix>/_blog/chunks/test-image@current") instead of the hardcoded
"refs/_blog/..." value.
In @test/Dockerfile.bats:
- Line 5: Pin the apk package versions instead of installing unpinned packages;
update the Dockerfile line that runs RUN apk add --no-cache bash git to specify
exact package versions (e.g., bash=<version> git=<version>) matching the Alpine
release used for this image, or reference the exact package APKBUILD/archived
package names from the Alpine repo; check the Alpine package repository for the
correct stable versions for your base image and replace the unpinned names so
installs are deterministic.
- Around line 1-15: The Dockerfile copies scripts/setup.sh into the container
but scripts/setup.sh assumes a sibling ../git-stunts that isn’t present in the
build context, causing runtime failures when test/setup.bats runs; fix by either
(A) modifying test/run-setup-tests.sh to build the image with context .. and
update Dockerfile COPY paths accordingly so ../git-stunts is available, or (B)
adjust Dockerfile.bats to COPY the actual git-stunts content into the image
(e.g., COPY path/to/git-stunts /code/git-stunts) and update scripts/setup.sh
references to /code/git-stunts, or (C) refactor scripts/setup.sh to detect
Docker environments and fall back to cloning the repository from its remote (or
skip git-stunts checks) when ../git-stunts is absent; pick one approach and
update the referenced files (Dockerfile.bats, test/run-setup-tests.sh, and
scripts/setup.sh) consistently so test/setup.bats can run without missing
../git-stunts.
- Line 2: The Dockerfile currently uses an unpinned base image ("FROM
bats/bats:latest"), which harms reproducibility; update that FROM line to pin to
a specific released tag or immutable digest (e.g., bats/bats:<stable-version> or
bats/bats@sha256:<digest>) so CI uses a known, immutable image and only changes
when you intentionally update the tag/digest, then commit the updated Dockerfile
and optionally add a short comment noting the chosen tag and rationale.
In @test/README.md:
- Around line 15-18: The Markdown violates MD031 by not having a blank line
before the fenced code block; add a single blank line immediately above the line
that begins the fenced block (the line with "```bash") so the code block
containing "npm run test:setup" is separated from the preceding paragraph/title;
ensure there is a blank line after the preceding text and keep the closing "```"
on its own line.
- Around line 46-51: The README.md violates MD031 by not having a blank line
before the fenced code block; edit the section containing the bash block that
shows "npm test" / "npm run test:local" and insert a single blank line
immediately before the triple-backtick fence so the code block is surrounded by
blank lines.
- Around line 66-69: Add a blank line before the fenced code block under the
"**Run:**" heading so the code block is separated by an empty line from the
preceding paragraph; update the README section containing the "npm run test:e2e"
code fence to include a blank line above the ```bash block to satisfy MD031.
- Around line 104-120: Fix the MD029 ordered-list numbering in the "Adding
Integration Tests" section by making all ordered list items use the consistent
"1." style; specifically change the list item that reads "Run tests: `npm test`"
(under the "Adding Integration Tests" heading and following the Vitest code
block) from "3." to "1." so the three items use the uniform 1/1/1 numbering.
- Around line 89-103: In the "### Adding BATS Tests" section update the ordered
list to use consistent "1." prefix style (MD029): change the final list item
currently shown as "3. Run tests: `npm run test:setup`" to "1. Run tests: `npm
run test:setup`" and ensure the preceding numbered lines (the two other steps
and the fenced code block example starting with @test) also use the same "1."
prefix rather than mixed numbering so the entire list reads 1., 1., 1.
- Around line 230-234: The README contains bare URLs in the list items for "BATS
Documentation", "Vitest Documentation", and "Playwright Documentation"; replace
each bare URL with Markdown link syntax so the text becomes a link (e.g.,
convert the plain URLs following those labels into [BATS
Documentation](https://...), [Vitest Documentation](https://...), and
[Playwright Documentation](https://...) respectively) to satisfy MD034.
In @test/run-setup-tests.sh:
- Line 10: The docker build in run-setup-tests.sh uses the current directory
(the `docker build -f test/Dockerfile.bats -t git-cms-setup-tests .` line) which
conflicts with the project’s parent-directory build pattern used by
`docker-compose.yml`; either change the build context from `.` to `..` in that
docker build invocation to match `docker-compose.yml` and then update
`test/Dockerfile.bats` COPY paths to account for the parent context, or keep `.`
but add an explicit comment near that docker build line explaining why this test
image must be built in isolation; ensure CI and any shared scripts use the same
context choice.
In @test/server.test.js:
- Around line 47-55: The POST in the "creates a snapshot via POST" test sends
JSON without a Content-Type header; update the fetch call to include headers: {
'Content-Type': 'application/json' } so the server correctly parses the JSON
body when hitting `${baseUrl}/api/cms/snapshot`; ensure the fetch options for
the test include this header alongside method and body.
- Around line 13-31: The test mutates global environment variables
(process.env.GIT_CMS_REPO and process.env.PORT) in the beforeAll hook causing
leaks; update the test to save the originals (e.g., const _origRepo =
process.env.GIT_CMS_REPO, const _origPort = process.env.PORT) before setting
them, and restore them in afterAll (or finally) after shutting down server;
locate the beforeAll/afterAll and startServer usage in server.test.js and ensure
the cleanup runs even on failures so other tests aren't affected.
In @test/setup.bats:
- Around line 43-52: The heredoc currently expands the shell variable $test_dir
inside the generated mock, which is fragile; change the heredoc to a quoted form
to prevent expansion (use <<'EOF') and update the mock to reference the exported
TEST_DIR variable inside the script (replace "$test_dir" with "$TEST_DIR") so
the generated "$TEST_DIR/mocks/git" script uses the correct, safely exported
variable at runtime; leave the cat target (cat > "$TEST_DIR/mocks/git") and
chmod +x unchanged.
- Line 7: The test setup hardcodes ORIGINAL_DIR="/code", which breaks when tests
aren’t run inside that container path; update test/setup.bats to compute
ORIGINAL_DIR dynamically instead of hardcoding: detect the repo root (e.g.,
prefer git rev-parse --show-toplevel if available, then fallback to the current
working directory or a resolved path), allow an existing environment override
(use ORIGINAL_DIR if already set), and ensure any consumers (npm test/setup
scripts) still work with this fallback so tests locate the source files reliably
across container, CI, and local runs.
In @TESTING_GUIDE.md:
- Around line 7-17: The markdown code block in TESTING_GUIDE.md is missing a
blank line before the fenced block and the language spec; add an empty line
immediately above the opening ``` and change the opening fence to include the
language identifier (e.g., ```bash) so the block begins with ```bash and is
preceded by a blank line to satisfy MD031 and MD040.
- Around line 29-33: The fenced code block in TESTING_GUIDE.md is missing a
language specifier (MD040); update the opening fence from ``` to ```bash so the
block is declared as bash, ensuring proper syntax highlighting for the tree
snippet in the documentation.
- Around line 66-70: The markdown has a missing blank line before the fenced
code block after the "Verify Docker is working:" heading, triggering MD031; add
a single empty line between the preceding paragraph ("Verify Docker is
working:") and the code fence that contains the docker commands (the block
starting with ```bash and containing "docker --version" and "docker compose
version") so the code block is properly separated.
- Around line 153-156: The markdown has a missing blank line before the fenced
code block (MD031) for the "docker compose down -v" snippet; add a single blank
line above the line starting with ```bash so the fence is separated from the
preceding text, ensure the fence uses ```bash then the command docker compose
down -v and a closing ``` on its own line to produce a valid fenced code block.
- Around line 289-303: In the "Solution:" section of TESTING_GUIDE.md ensure
there is a blank line immediately before the fenced code block and fix the fence
so it uses a proper opening and closing triple-backtick with the language
specifier (```bash ... ```); remove any stray characters around the closing
fence so the block is a valid fenced code block and satisfies MD031/MD040.
- Around line 53-58: The Markdown snippet in TESTING_GUIDE.md violates
MD031/MD040: add a blank line immediately before the code block and include the
language specifier after the opening backticks (use "bash") for the directory
listing code block (the block starting with "~/git/"). Ensure there is exactly
one blank line above the ```bash line and replace the opening ``` with ```bash
so the linter recognizes the block language.
- Around line 315-321: Add a blank line before the fenced code block under the
"Solution:" paragraph to satisfy MD031; locate the "Solution:" paragraph and
ensure there is an empty line between its text and the ```bash fence that starts
the code block (the ```bash / sudo systemctl start docker / ``` block).
- Around line 305-312: The fenced YAML code block under the "Port 4638 already
in use" section is mis-indented and missing the proper language spec placement
causing MD031/MD040 warnings; fix it by using a top-level fenced block with the
language immediately after the backticks (```yaml) and remove the extra leading
space before the block and the inner lines so the snippet starts with "ports:"
and then the mapping line '- "5000:4638"' (i.e., ensure the code fence and the
lines inside are not indented and that the language tag is on the opening
backticks).
- Around line 140-142: The bare URL on the line "Open your browser to:
http://localhost:4638" violates MD034; update TESTING_GUIDE.md to wrap the URL
in Markdown link syntax (e.g., [http://localhost:4638](http://localhost:4638) or
a descriptive label like [Local UI](http://localhost:4638)) so it is a proper
link and satisfies the MD034 rule.
- Around line 185-191: Add a blank line after the preceding Markdown heading and
before the fenced code block so the heading and code block are separated (fixes
MD022/MD031); ensure the code block uses proper fenced backticks with the
opening ``` on its own line above the commands and the closing ``` on its own
line after the last command (the block containing "cd git-cms", "npm test",
"./test/run-docker.sh", "docker compose run --rm test").
Comment on lines
+71
to
+75
| #### TC-1: Git's Content Addressability Model | ||
| Git uses SHA-1 hashing for object addressing. While SHA-1 has known collision vulnerabilities, Git is transitioning to SHA-256. The system assumes SHA-1 is "good enough" for content addressing (not for security-critical signing). | ||
|
|
||
| **Mitigation:** Use GPG signing (`CMS_SIGN=1`) for cryptographic non-repudiation. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and check the file size
fd -t f "ADR.md" -x wc -l {}Repository: flyingrobots/git-cms
Length of output: 82
🏁 Script executed:
# Read the specific lines mentioned in the review comment
cat -n docs/ADR.md | sed -n '65,80p'Repository: flyingrobots/git-cms
Length of output: 877
🏁 Script executed:
# Check the second flagged location
cat -n docs/ADR.md | sed -n '220,230p'Repository: flyingrobots/git-cms
Length of output: 845
🏁 Script executed:
# Check the third flagged location
cat -n docs/ADR.md | sed -n '1425,1445p'Repository: flyingrobots/git-cms
Length of output: 775
🏁 Script executed:
# Get broader context to understand overall document structure
wc -l docs/ADR.md
# Search for references to SHA-256, SHA-1, and object-format
rg -n "SHA-256|SHA-1|object.format|object-format" docs/ADR.mdRepository: flyingrobots/git-cms
Length of output: 1436
Add object-format caveats throughout—hard-coded SHA-1 values assume SHA-1 mode and will break under SHA-256.
The empty-tree hash on line 224 (4b825dc642cb6eb9a060e54bf8d69288fbee4904) is SHA-1-specific and will produce wrong results if Git uses SHA-256 object format. Statements at lines 72, 209, and 1433 similarly read as universal truths but implicitly assume SHA-1. Each needs explicit caveat like "in SHA-1 mode" or "using default object format" to clarify these are not mode-agnostic. Line 1439 mentions SHA-256 migration as future-state, but lacks guidance on how system behavior changes when actually deployed with SHA-256.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
71-71: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In @docs/ADR.md around lines 71 - 75, The ADR asserts Git content-addressing
facts but embeds SHA-1-specific assumptions; update the "TC-1: Git's Content
Addressability Model" section and every statement that references the empty-tree
hash value 4b825dc642cb6eb9a060e54bf8d69288fbee4904 (and the assertions at the
locations referencing SHA-1) to include explicit caveats such as "in SHA-1
object-format mode" or "using Git's default SHA-1 object format," and add a
short subsection near the SHA-256 migration mention that explains how behavior
and literal hashes change under SHA-256 (including that the empty-tree hash and
any hard-coded SHA-1 values are invalid in SHA-256 mode and must be recalculated
or made configurable).
| style CMS fill:#e1f5ff | ||
| ``` | ||
|
|
||
| ### Level 2: Lego Block Responsibilities |
There was a problem hiding this comment.
Duplicate heading detected (“Level 2: Lego Block Responsibilities” appears twice).
Rename one; the second section is describing APIs, not responsibilities.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
363-363: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
In @docs/ADR.md at line 363, Duplicate heading "Level 2: Lego Block
Responsibilities" appears twice; rename the second occurrence to reflect that it
describes APIs (e.g., "Level 2: Lego Block APIs" or "Level 2: Lego Block
Interfaces") and update any internal references or links that point to that
heading so they match the new title; search for the exact heading text "Level 2:
Lego Block Responsibilities" to locate both instances and change the second one
and its references accordingly.
Comment on lines
+1008
to
+1015
| **Solution:** Use `git update-ref --stdin` with expected old value: | ||
|
|
||
| ```bash | ||
| git update-ref refs/_blog/articles/hello-world <newSHA> <expectedOldSHA> | ||
| ``` | ||
|
|
||
| If `expectedOldSHA` doesn't match, Git returns exit code 1. | ||
|
|
There was a problem hiding this comment.
You describe git update-ref --stdin then show a non-stdin invocation. Pick one.
Right now the “Solution” is internally inconsistent.
🤖 Prompt for AI Agents
In @docs/ADR.md around lines 1008 - 1015, The doc is inconsistent: it mentions
`git update-ref --stdin` but shows a non-stdin invocation; fix by making the
example match the described mode. Either change the prose to describe the normal
CLI form and remove “--stdin”, keeping the shown command `git update-ref
refs/_blog/articles/hello-world <newSHA> <expectedOldSHA>`, or replace the code
block with the stdin form (e.g. echo "update-ref refs/_blog/articles/hello-world
<newSHA> <expectedOldSHA>" | git update-ref --stdin) so the example and the
description both reference `--stdin`. Ensure the surrounding sentence and the
exit-code statement remain accurate.
Comment on lines
+1016
to
+1021
| **Implementation:** `src/lib/CmsService.js:95` | ||
|
|
||
| ```javascript | ||
| await this.plumbing.updateRef({ ref, newSha, oldSha: parentSha }); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Do not cite doc-sensitive line numbers like src/lib/CmsService.js:95.
This is guaranteed to drift; reference the method name and/or a permalink.
🤖 Prompt for AI Agents
In @docs/ADR.md around lines 1016 - 1021, The ADR currently cites a drifting
file-specific line number; update the reference so it does not include a fragile
file:line location (e.g., remove "src/lib/CmsService.js:95") and instead
reference the method name used in the diff (updateRef) and/or a stable permalink
to the repository version of the file; ensure the Implementation note reads
something like “Implementation: src/lib/CmsService.js — method updateRef” or
includes a permalink to the exact commit/PR rather than a line number.
Comment on lines
+153
to
+156
| **To clean up:** | ||
| ```bash | ||
| docker compose down -v # Removes container and volumes | ||
| ``` |
There was a problem hiding this comment.
Add blank line before code block (MD031).
**To clean up:**
+
```bash
docker compose down -v # Removes container and volumes
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
154-154: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @TESTING_GUIDE.md around lines 153 - 156, The markdown has a missing blank
line before the fenced code block (MD031) for the "docker compose down -v"
snippet; add a single blank line above the line starting with bash so the fence is separated from the preceding text, ensure the fence uses bash then
the command docker compose down -v and a closing ``` on its own line to produce
a valid fenced code block.
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+185
to
+191
| cd git-cms | ||
| npm test | ||
| # OR | ||
| ./test/run-docker.sh | ||
| # OR | ||
| docker compose run --rm test | ||
| ``` |
There was a problem hiding this comment.
Fix Markdown heading spacing and code block formatting (MD022, MD031).
-### Myth: "I'll accidentally run commands in my project repo"
+
+### Myth: "I'll accidentally run commands in my project repo"
**Reality:** The CLI checks what directory you're in. If you're in a repo with important files, you'll notice. Plus, git-cms operates in the `refs/_blog/*` namespace, separate from your normal branches.
### Actual Risk: Running Tests Outside Docker
**IF** you run `npm run test:local` (bypassing Docker), tests WILL create temporary repos in your `/tmp` directory. While these are deleted after, there's a non-zero risk if tests fail mid-execution.
**Solution:** Always use `npm test` which automatically uses Docker.
+
---🤖 Prompt for AI Agents
In @TESTING_GUIDE.md around lines 185 - 191, Add a blank line after the
preceding Markdown heading and before the fenced code block so the heading and
code block are separated (fixes MD022/MD031); ensure the code block uses proper
fenced backticks with the opening ``` on its own line above the commands and the
closing ``` on its own line after the last command (the block containing "cd
git-cms", "npm test", "./test/run-docker.sh", "docker compose run --rm test").
Comment on lines
+289
to
+303
|
|
||
| **Solution:** | ||
| ```bash | ||
| # Verify structure | ||
| ls -l .. | ||
| # Should show both git-cms and git-stunts | ||
|
|
||
| # If git-stunts is missing | ||
| cd .. | ||
| git clone https://github.com/flyingrobots/git-stunts.git | ||
| cd git-cms | ||
|
|
||
| # Rebuild Docker images | ||
| docker compose build | ||
| ``` |
There was a problem hiding this comment.
Add blank line before and fix code block specification (MD031, MD040).
**Solution:**
+
-```
+```bash
# Verify structure
ls -l ..
# Should show both git-cms and git-stunts
# If git-stunts is missing
cd ..
git clone https://github.com/flyingrobots/git-stunts.git
cd git-cms
# Rebuild Docker images
docker compose build
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
291-291: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In @TESTING_GUIDE.md around lines 289 - 303, In the "Solution:" section of
TESTING_GUIDE.md ensure there is a blank line immediately before the fenced code
block and fix the fence so it uses a proper opening and closing triple-backtick
with the language specifier (```bash ... ```); remove any stray characters
around the closing fence so the block is a valid fenced code block and satisfies
MD031/MD040.
Comment on lines
+305
to
+312
| ### "Port 4638 already in use" | ||
|
|
||
| **Solution:** Either stop the process using that port, or change the port in `docker-compose.yml`: | ||
|
|
||
| ```yaml | ||
| ports: | ||
| - "5000:4638" # Maps localhost:5000 → container:4638 | ||
| ``` |
There was a problem hiding this comment.
Fix code block spacing and specification (MD031, MD040).
**Solution:** Either stop the process using that port, or change the port in `docker-compose.yml`:
+```yaml
-```yaml
ports:
- "5000:4638" # Maps localhost:5000 → container:4638
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @TESTING_GUIDE.md around lines 305 - 312, The fenced YAML code block under
the "Port 4638 already in use" section is mis-indented and missing the proper
language spec placement causing MD031/MD040 warnings; fix it by using a
top-level fenced block with the language immediately after the backticks
(```yaml) and remove the extra leading space before the block and the inner
lines so the snippet starts with "ports:" and then the mapping line '-
"5000:4638"' (i.e., ensure the code fence and the lines inside are not indented
and that the language tag is on the opening backticks).
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+315
to
+321
| **Solution:** Start Docker Desktop (macOS/Windows) or start the Docker service (Linux): | ||
| ```bash | ||
| # Linux | ||
| sudo systemctl start docker | ||
| ``` |
There was a problem hiding this comment.
Add blank line before code block (MD031).
**Solution:** Start Docker Desktop (macOS/Windows) or start the Docker service (Linux):
+```bash
-```bash
# Linux
sudo systemctl start docker
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @TESTING_GUIDE.md around lines 315 - 321, Add a blank line before the fenced
code block under the "Solution:" paragraph to satisfy MD031; locate the
"Solution:" paragraph and ensure there is an empty line between its text and the
bash fence that starts the code block (the bash / sudo systemctl start
docker / ``` block).
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+22
to
+27
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Run BATS tests | ||
| run: npm run test:setup |
There was a problem hiding this comment.
CRITICAL: Workflow missing git-stunts dependency setup.
This workflow runs npm run test:setup which invokes Docker builds requiring the git-stunts sibling directory (see Dockerfile line 21). The workflow only checks out the current repository, causing the pipeline failure:
'/git-stunts' not found in build context
Required fix: Clone git-stunts before running tests.
🔧 Add missing setup step
steps:
- name: Checkout code
uses: actions/checkout@v4
+
+ - name: Clone git-stunts dependency
+ run: |
+ cd ..
+ git clone https://github.com/flyingrobots/git-stunts.git
- name: Run BATS tests
run: npm run test:setupAlternatively, use a multi-repository checkout action or Git submodules (see Dockerfile review for options).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Run BATS tests | |
| run: npm run test:setup | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Clone git-stunts dependency | |
| run: | | |
| cd .. | |
| git clone https://github.com/flyingrobots/git-stunts.git | |
| - name: Run BATS tests | |
| run: npm run test:setup |
🧰 Tools
🪛 actionlint (1.7.10)
24-24: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/test-setup.yml around lines 22 - 27, The workflow fails
because the test step requires a sibling repo named git-stunts in the build
context; add a step before the "Run BATS tests" / npm run test:setup step that
checks out or clones the git-stunts repository into a directory named git-stunts
(e.g., using actions/checkout@v3 with repository set to the git-stunts repo and
path set to git-stunts, or running git clone into ./git-stunts) so Dockerfile
line 21 can find /git-stunts during the build.
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Upgrade deprecated GitHub Actions to v4.
Both actions/checkout@v3 (line 24) and actions/upload-artifact@v3 (line 31) use the deprecated Node 16 runner. GitHub will eventually remove support.
⬆️ Upgrade to v4
- name: Checkout code
- uses: actions/checkout@v3
+ uses: actions/checkout@v4 - name: Upload test results
if: always()
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: bats-test-results
path: test-results/🧰 Tools
🪛 actionlint (1.7.10)
24-24: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/test-setup.yml at line 24, Replace deprecated action
versions: update the workflow steps that reference uses: actions/checkout@v3 and
uses: actions/upload-artifact@v3 to their v4 counterparts (actions/checkout@v4
and actions/upload-artifact@v4) so the job runs on the supported Node 18 runner;
locate the two occurrences in the .github/workflows/test-setup.yml and change
the version tags accordingly, leaving the rest of each step configuration
intact.
| const [,, cmd, ...args] = process.argv; | ||
| const cwd = process.cwd(); | ||
| const refPrefix = process.env.CMS_REF_PREFIX; | ||
| const refPrefix = process.env.CMS_REF_PREFIX || 'refs/_blog/dev'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the hardcoded default to a named constant.
The magic string 'refs/_blog/dev' is a configuration value that warrants extraction to a module-level constant for maintainability and discoverability. If this default ever needs to change, it should be trivial to locate.
♻️ Suggested refactor
+const DEFAULT_REF_PREFIX = 'refs/_blog/dev';
+
async function main() {
const [,, cmd, ...args] = process.argv;
const cwd = process.cwd();
- const refPrefix = process.env.CMS_REF_PREFIX || 'refs/_blog/dev';
+ const refPrefix = process.env.CMS_REF_PREFIX || DEFAULT_REF_PREFIX;🤖 Prompt for AI Agents
In @bin/git-cms.js at line 8, Extract the magic default 'refs/_blog/dev' into a
module-level constant (e.g., DEFAULT_CMS_REF_PREFIX) and replace the inline
literal in the refPrefix initialization; update the declaration that currently
sets const refPrefix = process.env.CMS_REF_PREFIX || 'refs/_blog/dev' to use the
constant so the default is defined once and easily discoverable.
| case 'list': { | ||
| const items = listRefs('draft', { cwd, refPrefix }); | ||
| const items = await cms.listArticles(); | ||
| if (items.length === 0) console.log("No articles found."); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trivial punctuation inconsistency.
The message "No articles found." includes terminal punctuation, while error messages elsewhere omit it. For pedantic consistency across user-facing output, either standardize on punctuation or omit it uniformly.
🤖 Prompt for AI Agents
In @bin/git-cms.js at line 36, The console message in the conditional that
checks items.length uses "No articles found." with a trailing period which is
inconsistent with other user-facing messages; update the string in the if
(items.length === 0) branch to remove the terminal punctuation (e.g., "No
articles found") so it matches the punctuation style used elsewhere where
messages omit a final period.
Comment on lines
47
to
51
| case 'serve': { | ||
| const { startServer } = await import('../src/server/index.js'); | ||
| startServer(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the startServer function
find . -name "index.js" -path "*/server/*" | head -20Repository: flyingrobots/git-cms
Length of output: 85
🏁 Script executed:
# Check the server module to understand startServer signature
cat -n src/server/index.js | head -100Repository: flyingrobots/git-cms
Length of output: 4196
🏁 Script executed:
# Search for startServer definition and usage
rg -n "startServer" --type=js -B2 -A5Repository: flyingrobots/git-cms
Length of output: 1748
🏁 Script executed:
# Check for similar dynamic import patterns in the codebase
rg -n "import\(" --type=js -B2 -A2 | head -50Repository: flyingrobots/git-cms
Length of output: 293
🏁 Script executed:
# Get the complete startServer function
sed -n '153,170p' src/server/index.jsRepository: flyingrobots/git-cms
Length of output: 444
🏁 Script executed:
# Check entire bin/git-cms.js structure to understand error handling patterns
cat -n bin/git-cms.jsRepository: flyingrobots/git-cms
Length of output: 2401
Capture the server return value and attach error handling.
The startServer() function returns an http.Server object (see src/server/index.js:161), but line 49 discards it entirely. This prevents error handling on the server. The test suite (test/server.test.js:22-26) shows the correct pattern: capture the return value and listen for events.
Add error handling:
case 'serve': {
const { startServer } = await import('../src/server/index.js');
const server = startServer();
server.on('error', (err) => {
console.error(`Server error: ${err.message}`);
process.exit(1);
});
break;
}Without capturing the server, port conflicts and other listen errors will be silently swallowed, making the serve command unreliable.
🤖 Prompt for AI Agents
In @bin/git-cms.js around lines 47 - 51, startServer()'s return value is
discarded; capture it (e.g., const server = startServer()) and attach an 'error'
event handler to the returned http.Server instance that logs the error
(including err.message) and calls process.exit(1); update the 'serve' case where
startServer() is invoked to store the server and add server.on('error', ...) so
port conflicts and listen errors are surfaced and handled.
| echo "" | ||
|
|
||
| # Build the test image | ||
| docker build -f test/Dockerfile.bats -t git-cms-setup-tests . |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent build context with project Docker strategy.
This builds from current directory (.), while docker-compose.yml builds from parent (..) to access git-stunts. This architectural inconsistency will bite you when:
- Shared build scripts or dependencies are needed
- CI needs to run both builds with consistent context
- Someone tries to understand "the right way" to build in this project
Either align this to use parent context, or explicitly document why this test build is special and should remain isolated.
♻️ Proposed fix to align with parent context pattern
-docker build -f test/Dockerfile.bats -t git-cms-setup-tests .
+docker build -f git-cms/test/Dockerfile.bats -t git-cms-setup-tests ..This assumes you're maintaining the parent-directory build pattern. Update test/Dockerfile.bats COPY paths accordingly:
-COPY test/setup.bats /code/test/setup.bats
-COPY scripts/setup.sh /code/scripts/setup.sh
-COPY package.json /code/package.json
+COPY git-cms/test/setup.bats /code/test/setup.bats
+COPY git-cms/scripts/setup.sh /code/scripts/setup.sh
+COPY git-cms/package.json /code/package.json🤖 Prompt for AI Agents
In @test/run-setup-tests.sh at line 10, The docker build in run-setup-tests.sh
uses the current directory (the `docker build -f test/Dockerfile.bats -t
git-cms-setup-tests .` line) which conflicts with the project’s parent-directory
build pattern used by `docker-compose.yml`; either change the build context from
`.` to `..` in that docker build invocation to match `docker-compose.yml` and
then update `test/Dockerfile.bats` COPY paths to account for the parent context,
or keep `.` but add an explicit comment near that docker build line explaining
why this test image must be built in isolation; ensure CI and any shared scripts
use the same context choice.
Comment on lines
+13
to
+31
| 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 }); | ||
|
|
||
| process.env.GIT_CMS_REPO = cwd; | ||
| process.env.PORT = '0'; | ||
|
|
||
| server = startServer(); | ||
|
|
||
| await new Promise((resolve) => { | ||
| if (server.listening) resolve(); | ||
| else server.once('listening', resolve); | ||
| }); | ||
|
|
||
| const port = server.address().port; | ||
| baseUrl = `http://localhost:${port}`; | ||
| }); |
There was a problem hiding this comment.
Test pollutes process.env and will cause flakiness.
Lines 19-20 mutate global environment variables without restoration. Vitest runs tests in parallel by default, so these mutations will leak into other test files and cause non-deterministic failures. Even in serial mode, subsequent tests may be affected.
🧪 Proposed fix to isolate environment
describe('Server API (Integration)', () => {
let cwd;
let server;
let baseUrl;
+ let originalRepo;
+ let originalPort;
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 });
+ originalRepo = process.env.GIT_CMS_REPO;
+ originalPort = process.env.PORT;
process.env.GIT_CMS_REPO = cwd;
process.env.PORT = '0';
server = startServer();
await new Promise((resolve) => {
if (server.listening) resolve();
else server.once('listening', resolve);
});
const port = server.address().port;
baseUrl = `http://localhost:${port}`;
});
afterAll(async () => {
if (server) {
await new Promise(resolve => server.close(resolve));
}
+ if (originalRepo !== undefined) process.env.GIT_CMS_REPO = originalRepo;
+ else delete process.env.GIT_CMS_REPO;
+ if (originalPort !== undefined) process.env.PORT = originalPort;
+ else delete process.env.PORT;
rmSync(cwd, { recursive: true, force: true });
});🤖 Prompt for AI Agents
In @test/server.test.js around lines 13 - 31, The test mutates global
environment variables (process.env.GIT_CMS_REPO and process.env.PORT) in the
beforeAll hook causing leaks; update the test to save the originals (e.g., const
_origRepo = process.env.GIT_CMS_REPO, const _origPort = process.env.PORT) before
setting them, and restore them in afterAll (or finally) after shutting down
server; locate the beforeAll/afterAll and startServer usage in server.test.js
and ensure the cleanup runs even on failures so other tests aren't affected.
Comment on lines
+47
to
+55
| it('creates a snapshot via POST', async () => { | ||
| const res = await fetch(`${baseUrl}/api/cms/snapshot`, { | ||
| method: 'POST', | ||
| body: JSON.stringify({ | ||
| slug: 'api-test', | ||
| title: 'API Title', | ||
| body: 'API Body' | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Missing Content-Type header in POST request.
Line 49-54: You're sending JSON without telling the server it's JSON. The fetch API does not automatically set Content-Type: application/json when you stringify the body. The server may reject this request or fail to parse the body, causing the test to pass by accident if the server has lenient fallback behavior.
🔧 Proposed fix
it('creates a snapshot via POST', async () => {
const res = await fetch(`${baseUrl}/api/cms/snapshot`, {
method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
body: JSON.stringify({
slug: 'api-test',
title: 'API Title',
body: 'API Body'
})
});🤖 Prompt for AI Agents
In @test/server.test.js around lines 47 - 55, The POST in the "creates a
snapshot via POST" test sends JSON without a Content-Type header; update the
fetch call to include headers: { 'Content-Type': 'application/json' } so the
server correctly parses the JSON body when hitting
`${baseUrl}/api/cms/snapshot`; ensure the fetch options for the test include
this header alongside method and body.
| setup() { | ||
| # Create a temporary test directory | ||
| export TEST_DIR="$(mktemp -d)" | ||
| export ORIGINAL_DIR="/code" |
There was a problem hiding this comment.
Hardcoded path assumption may break outside container.
ORIGINAL_DIR="/code" assumes tests run in a Docker container at that specific path. If someone runs npm run test:setup locally or CI uses a different mount point, these tests will fail to find the source files.
🐛 Make path detection dynamic
setup() {
# Create a temporary test directory
export TEST_DIR="$(mktemp -d)"
- export ORIGINAL_DIR="/code"
+ # Detect the repo root dynamically (BATS sets BATS_TEST_DIRNAME to test/ dir)
+ export ORIGINAL_DIR="$(cd "$(dirname "$BATS_TEST_DIRNAME")" && pwd)"Or if BATS is always run from repo root via npm script:
- export ORIGINAL_DIR="/code"
+ export ORIGINAL_DIR="${BATS_CWD:-$(pwd)}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export ORIGINAL_DIR="/code" | |
| setup() { | |
| # Create a temporary test directory | |
| export TEST_DIR="$(mktemp -d)" | |
| # Detect the repo root dynamically (BATS sets BATS_TEST_DIRNAME to test/ dir) | |
| export ORIGINAL_DIR="$(cd "$(dirname "$BATS_TEST_DIRNAME")" && pwd)" |
🤖 Prompt for AI Agents
In @test/setup.bats at line 7, The test setup hardcodes ORIGINAL_DIR="/code",
which breaks when tests aren’t run inside that container path; update
test/setup.bats to compute ORIGINAL_DIR dynamically instead of hardcoding:
detect the repo root (e.g., prefer git rev-parse --show-toplevel if available,
then fallback to the current working directory or a resolved path), allow an
existing environment override (use ORIGINAL_DIR if already set), and ensure any
consumers (npm test/setup scripts) still work with this fallback so tests locate
the source files reliably across container, CI, and local runs.
Comment on lines
+43
to
+52
| cat > "$TEST_DIR/mocks/git" <<EOF | ||
| #!/bin/bash | ||
| if [[ "\$1" == "clone" ]]; then | ||
| mkdir -p "$test_dir/git-stunts" | ||
| echo "Cloning into 'git-stunts'..." | ||
| exit 0 | ||
| fi | ||
| exit 0 | ||
| EOF | ||
| chmod +x "$TEST_DIR/mocks/git" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unquoted variable expansion in heredoc is fragile.
Line 46 uses "$test_dir" inside the heredoc, which depends on shell escaping behavior. If $test_dir contains spaces or special characters, the generated mock will break.
♻️ Use single-quoted heredoc with explicit substitution
mock_git_clone_success() {
local test_dir="$TEST_DIR"
- cat > "$TEST_DIR/mocks/git" <<EOF
+ cat > "$TEST_DIR/mocks/git" <<'EOF'
#!/bin/bash
if [[ "$1" == "clone" ]]; then
- mkdir -p "$test_dir/git-stunts"
+ mkdir -p "$TEST_DIR/git-stunts"
echo "Cloning into 'git-stunts'..."
exit 0
fi
exit 0
EOF
chmod +x "$TEST_DIR/mocks/git"
}Actually, since TEST_DIR is exported in setup(), the mock script can reference it directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cat > "$TEST_DIR/mocks/git" <<EOF | |
| #!/bin/bash | |
| if [[ "\$1" == "clone" ]]; then | |
| mkdir -p "$test_dir/git-stunts" | |
| echo "Cloning into 'git-stunts'..." | |
| exit 0 | |
| fi | |
| exit 0 | |
| EOF | |
| chmod +x "$TEST_DIR/mocks/git" | |
| cat > "$TEST_DIR/mocks/git" <<'EOF' | |
| #!/bin/bash | |
| if [[ "$1" == "clone" ]]; then | |
| mkdir -p "$TEST_DIR/git-stunts" | |
| echo "Cloning into 'git-stunts'..." | |
| exit 0 | |
| fi | |
| exit 0 | |
| EOF | |
| chmod +x "$TEST_DIR/mocks/git" |
🤖 Prompt for AI Agents
In @test/setup.bats around lines 43 - 52, The heredoc currently expands the
shell variable $test_dir inside the generated mock, which is fragile; change the
heredoc to a quoted form to prevent expansion (use <<'EOF') and update the mock
to reference the exported TEST_DIR variable inside the script (replace
"$test_dir" with "$TEST_DIR") so the generated "$TEST_DIR/mocks/git" script uses
the correct, safely exported variable at runtime; leave the cat target (cat >
"$TEST_DIR/mocks/git") and chmod +x unchanged.
- Introduce `docs/adr-tex-2/`: A comprehensive Architecture Decision Record implemented in LaTeX.
- Document core architectural principles: "Zero-Database," "Empty Tree" commits, and the `@git-stunts/*` module decomposition.
- Add TikZ figures for System Context, Component Decomposition, Runtime Sequences, and Deployment Topologies.
- Add `Makefile` for PDF compilation and update `.gitignore` for LaTeX build artifacts.
- Update `test/setup.bats`: Adjust assertions to match specific prompt text ("Would you like me to clone it now") and enable debug logging for the setup script test.
There was a problem hiding this comment.
Actionable comments posted: 51
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 11-15: Remove the trailing space from the '*.aux ' entry (make it
'*.aux') and expand the LaTeX artifact coverage by adding common build outputs
alongside the existing '*.log', '*.out', and '*.toc': include '*.pdf',
'*.synctex.gz', '*.fls', '*.fdb_latexmk', '*.bbl', '*.blg', '*.idx', '*.ind',
'*.ilg', '*.lof', '*.lot', '*.nav', '*.snm', '*.vrb', '*.dvi', and '*.ps' so the
.gitignore fully ignores typical LaTeX artifacts.
In @docs/adr-tex-2/figures/context.tex:
- Line 12: The file ends with "\end{tikzpicture}" but is missing a trailing
newline; add a single newline character at the end of
docs/adr-tex-2/figures/context.tex so the file ends with "\end{tikzpicture}"
followed by a newline (ensure the editor saves with a final newline/EOF marker).
In @docs/adr-tex-2/figures/decomposition.tex:
- Around line 16-20: The LaTeX TikZ lines contain multiple semicolon-separated
\draw commands per line (e.g., commands involving nodes CLI, CMS, HTTP, Codec,
Graph, CAS, Vault, Plumbing), which hurts readability and editing; split each
\draw invocation onto its own line so each edge like "\draw [line] (CLI) --
(CMS);" "\draw [line] (HTTP) -- (CMS);" "\draw [line] (CMS) -- (Codec);" "\draw
[line] (CMS) -- (Graph);" "\draw [line] (CMS) -- (CAS);" "\draw [line] (CMS) --
(Vault);" "\draw [line] (CMS) -- (Plumbing);" "\draw [line] (Graph) --
(Plumbing);" and "\draw [line] (CAS) -- (Plumbing);" becomes a single-line
statement to improve maintainability and ease future edits.
In @docs/adr-tex-2/figures/draft-sequence.tex:
- Line 15: The arrow from (CMS)+(0,-4) to (CMS)+(0.8,-4.2) for createNode() is
too short and looks like a stub; replace it with a proper self-call loop (right,
down, left) anchored on the CMS lifeline (e.g., draw a horizontal segment right,
a vertical down, then back left with the arrow and activation box) to express an
internal self-invocation of Service createNode(), or if createNode() delegates
externally, route the arrow to the PL (or empty-graph) lifeline instead so the
arrow terminates at PL; update the draw command referencing CMS and createNode()
accordingly to implement one of these two clear alternatives.
In @docs/adr-tex-2/figures/responsibilities.tex:
- Around line 1-2: The tikzpicture declares a dead parameter "node distance=1cm"
which is never used because all nodes are placed explicitly (e.g., CMS node and
others use explicit anchors like "right=3cm of CMS.north" and "below=0.5cm of
..."); remove the redundant "node distance=1cm" from the tikzpicture options to
avoid misleading clutter and keep the picture options minimal (look for the
tikzpicture line containing "node distance=1cm" and remove that token while
leaving other options and the CMS node definition intact).
In @docs/adr-tex-2/main.tex:
- Around line 23-30: The hyperref package is being configured twice:
\usepackage[hidelinks]{hyperref} already disables link styling and then
\hypersetup{colorlinks=false,...} repeats that; remove the redundancy by
choosing one configuration method—either keep \usepackage[hidelinks]{hyperref}
and delete the colorlinks=false entry from \hypersetup, or remove the hidelinks
option and only set colorlinks=false (and other PDF metadata) in
\hypersetup—update the lines referencing \usepackage[hidelinks]{hyperref} and
the \hypersetup block accordingly.
- Around line 82-84: Replace the hardcoded "2026-01-11" date with a reusable
macro: either use \today directly or reference a meta macro like \docdate (and
likewise replace the version literal with \docversion); define \docdate and
\docversion in a central meta.tex and ensure that meta.tex is \input or \include
from the document preamble so the main.tex header uses those centralized values.
In @docs/adr-tex-2/Makefile:
- Line 3: The Makefile rule for the target main.pdf omits meta.tex from its
prerequisites, so changes to the file included by main.tex via \input{meta}
won't trigger a rebuild; update the dependency list for the main.pdf target (the
line defining "main.pdf: main.tex $(wildcard sections/*.tex) $(wildcard
figures/*.tex)") to include meta.tex (or a suitable wildcard/pattern that covers
it) so that editing meta.tex causes make to rebuild main.pdf.
- Line 1: The Makefile lacks .PHONY declarations which can cause silent failures
if files named like targets appear; add a .PHONY line listing the phony targets
(at minimum "all" and "clean") near the top of the Makefile so Make treats those
targets as always out-of-date (e.g., add a rule ".PHONY: all clean" above the
"all: main.pdf" target); update any other non-file targets used in this file to
the .PHONY list as well.
- Around line 7-8: The Makefile's clean target only removes a few local
extensions and misses many LaTeX auxiliaries and files generated in
subdirectories; update the clean target (named "clean") to remove additional
extensions (.fls, .fdb_latexmk, .synctex.gz, .nav, .snm, .bbl, .blg) and
recursively delete auxiliaries from subfolders (e.g., sections/, figures/) or
replace the current clean recipe with a call to latexmk -C to perform a thorough
project-wide cleanup instead.
- Around line 4-5: The Makefile currently runs two unconditional pdflatex passes
("pdflatex -interaction=nonstopmode main.tex") so the second run still executes
if the first fails; change the rule to short-circuit on error by chaining the
commands with && (e.g., run the second pdflatex only if the first succeeds) or
replace both calls with a single robust tool like latexmk (e.g., use a target
such as main.pdf that invokes "latexmk -pdf -interaction=nonstopmode main.tex")
so multiple passes and dependencies are handled automatically.
In @docs/adr-tex-2/meta.tex:
- Line 4: Replace the hardcoded date in the \date{...} macro so it isn’t
manually maintained: use LaTeX’s \today (i.e. \date{Version 1.0.0 -- Last
Updated: \today}) or, if you prefer Git-sourced dates, inject the output of git
log -1 --format=%cs at build time into the \date value; also change the double
hyphen to an em-dash by using --- or \textemdash for correct typography (e.g.,
Version 1.0.0 \textemdash Last Updated: \today).
- Line 2: The title uses explicit bolding which is redundant and may conflict
with class formatting; remove the \textbf{...} wrapper from inside \title{...}
so the title line becomes \title{Architecture Decision Record: Git CMS\\ \large
Database-Free Content Management via Git Plumbing}, leaving styling to the
document class and avoiding nested/overridden weight issues.
In @docs/adr-tex-2/sections/01-introduction.tex:
- Around line 11-34: Add LaTeX labels for each Fundamental Requirement
immediately after their \subsubsection headings so they can be referenced
elsewhere; for FR-1 through FR-5 insert \label{fr:zero-database},
\label{fr:crypto-verify}, \label{fr:fast-forward-publish},
\label{fr:client-encryption}, and \label{fr:infinite-recovery} respectively
right after the corresponding \subsubsection{FR-1: ...} through
\subsubsection{FR-5: ...} lines to enable use of \ref{} instead of hardcoded
numbers.
- Around line 16-19: Update the FR-2: Cryptographic Verifiability text to
acknowledge SHA-1 is deprecated and recommend migration to SHA-256;
specifically, replace or augment the phrase "SHA-1 hashing (with optional GPG
signing...)" with language that notes SHA-1 collision weaknesses and states the
system should use SHA-256 where supported (e.g., reference Git's SHA-256 object
format via git init --object-format=sha256) and fall back to SHA-1 only
temporarily for interoperability, while requiring GPG signing for
non-repudiation until full SHA-256 tooling adoption.
In @docs/adr-tex-2/sections/02-constraints.tex:
- Line 42: The text references "git-stargate" without a link; update the
\textbf{Mitigation:} line that mentions "git-stargate (a companion project)" to
either add a footnote with the repository/URL (e.g., replace the parenthetical
with \footnote{https://...} or equivalent LaTeX footnote markup) so readers can
find the project, or remove the "(a companion project)" parenthetical entirely
if the repo is not public. Ensure the visible phrase "git-stargate" remains
unchanged so it’s easy to locate in the document.
- Line 47: The file ending with the line "\textbf{Mitigation:} Use \texttt{git
gc} aggressively. Consider ref pruning for old drafts." is missing a trailing
newline; open the corresponding file and add a single newline character at EOF
so the file ends with a proper POSIX-compliant newline.
- Around line 34-35: The mitigation text for RC-2 is incorrect: replace the
current statement about using Node's crypto with a compliance-focused mitigation
that addresses export controls (e.g., consult legal/export-control counsel,
specify permitted jurisdictions, add distribution controls or licensing checks,
or provide a non-crypto build/config option), or if export restrictions truly do
not apply remove RC-2 entirely; update the RC-2 mitigation to reference the
module name (@git-stunts/vault) and explicitly state the chosen
legal/operational action so the document no longer conflates availability with
legal compliance.
In @docs/adr-tex-2/sections/03-context.tex:
- Line 49: The Authentication line in 03-context.tex currently states "None"
without a security callout; update the \item \textbf{Authentication:} entry to
include a brief cross-reference (for example add an italicized note like "See
Section 11 (Risks) for threat model implications.") and ensure the Risks section
(11-risks.tex) has a matching entry that discusses this assumption and its
threat-model/mitigation details so readers scanning for security implications
can find the full analysis.
In @docs/adr-tex-2/sections/04-solution.tex:
- Line 57: Update the rationale sentence under \textbf{Rationale}: clarify
"Backward parsing" by adding a parenthetical and brief explanation; change
"Backward parsing is efficient." to something like "Backward parsing (from the
message end) is efficient, allowing O(1) access to the trailer block without
scanning the message body." Ensure this replacement appears in the same
rationale sentence mentioning Trailers.
- Around line 15-23: Update the P-3 and P-4 paragraphs to acknowledge SHA-1
limitations: explicitly state that content addressability currently uses SHA-1
(as noted in P-3) but that SHA-1 is vulnerable to collision attacks and
therefore the “mathematically verifiable” claim in P-4 needs caveats; mention
Git’s hardening measures for commit objects and the ongoing SHA-256 migration
(object-format) as mitigations, and/or state intent to migrate to SHA-256 or
re-evaluate the CMS_SIGN trust assumptions to avoid overclaiming integrity
guarantees.
- Line 25: The subsection title uses incorrect LaTeX quoting \"Empty Tree\";
replace the escaped ASCII quotes with proper TeX opening and closing quotes by
changing \"Empty Tree\" to ``Empty Tree'' in the \subsection{Solution Approach:
The "Empty Tree" Stunt} line (look for the \subsection declaration containing
Empty Tree) so the title renders typographic quotation marks in LaTeX.
- Line 43: Replace the incorrect C-style escaped quotes \"current\" with proper
LaTeX quotation marks; change it to ``current'' (or use \enquote{current} if
csquotes is enabled) in the sentence "Each draft save creates a new commit. The
\"current\" article..." so the document uses correct TeX quotes.
- Around line 59-62: The ADR's claim "the gateway never sees plaintext" is
incorrect because the current implementation in src/server/index.js uses
Buffer.from(data, 'base64') to decode incoming data and fs.writeFileSync to
write plaintext to disk before calling cms.uploadAsset(), meaning plaintext
exists on the server; fix this by either (A) updating the D-3 text in
04-solution.tex to state the correct threat model (server-side
decryption/intermediate plaintext storage and that encryption occurs in the
storage layer, not client-side) and remove the inaccurate "gateway never sees
plaintext" sentence, or (B) re-architect the flow so the gateway never persists
plaintext by performing encryption client-side or streaming the decoded data
directly into an encrypt-then-write pipeline (remove the intermediate
fs.writeFileSync usage and ensure cms.uploadAsset() receives encrypted data),
then update the ADR to reflect the implemented model; reference Buffer.from,
fs.writeFileSync, cms.uploadAsset, and the D-3 paragraph when making the change.
In @docs/adr-tex-2/sections/07-deployment.tex:
- Line 58: The file is missing a trailing newline at EOF; open
docs/adr-tex-2/sections/07-deployment.tex and add a single newline character
after the final \end{figure} (ensure the file ends with a newline), and then add
or enable a repository pre-commit hook or editor setting (e.g.,
core.whitespace/newline-at-eof or an editorconfig rule) to automatically enforce
trailing newlines for all files going forward.
- Line 46: The LaTeX node (Repo) currently uses {\.git/objects} which invokes
the \. accent and breaks rendering; change the node content to use a literal dot
string, e.g. replace {\.git/objects} with {\texttt{.git/objects}} (or
{\verb|.git/objects|}) so the path renders correctly.
- Line 26: The TikZ node declaration for CMS_B uses the deprecated positioning
syntax "below of=CMS_A"; update it to the modern form "below=of CMS_A" and
ensure consistency across the diagram (e.g., other nodes already using "below=of
Node"). Also confirm the positioning library is loaded and keep or adjust node
distance (node distance=2.5cm) to work with the modern syntax so all nodes like
CMS_B and CMS_A use the same "=<of>" style.
- Line 10: The label "Local Repo\\(\.git/objects)" uses the LaTeX diacritic
command `\.` and thus renders incorrectly; replace the parenthesized path with a
literal monospaced representation such as `\texttt{.git/objects}` (or
`\verb|.git/objects|`) in the node definition for (Repo) so the path prints as
".git/objects" instead of attempting to apply a dot accent.
In @docs/adr-tex-2/sections/08-crosscutting.tex:
- Around line 44-52: The diagram incorrectly sequences (Input) → (Enc) → (Blob)
→ (Manifest); change the flow so (Enc) produces both (Blob) and (Manifest) in
parallel by replacing the edge from (Blob) → (Manifest) with an edge from (Enc)
→ (Manifest) and make (Manifest) a standalone node (not fit=(Blob)) so it is
visually a sibling output of (Blob); ensure node names referenced are (Input),
(Enc), (Blob), (Manifest) and keep the existing AuthorBox/GatewayBox placements
but update any fit=() references that currently attach the manifest to the blob.
- Line 9: The inline style mutation using "dashed" on the existing \node [block,
dashed] (Empty) should be replaced with a named style: add a new TikZ style
(e.g., blockdashed) in main.tex alongside block and blockshaded that composes
base/rectangle/rounded corners/fill/dashed, then update the node declaration to
use \node [blockdashed] (Empty) {Empty Tree\\4b825dc...}; so the dashed
sentiment is reusable and not applied ad-hoc.
In @docs/adr-tex-2/sections/09-decisions.tex:
- Around line 3-47: The ADR documents (ADR-001..ADR-004) are missing the
"Consequences" section; for each \subsection*{ADR-00X} add a brief
"Consequences" paragraph after the Decision (before Status) describing
trade-offs and limitations introduced by the decision (e.g., for ADR-001 note
commit message size limits and inability to use blobs; for ADR-002 note
parsing/format constraints and potential trailer key collisions; for ADR-003
note inability to rewrite published history and impact on rollbacks; for ADR-004
note key management/rotation complexity and recovery implications). Ensure each
new "Consequences" paragraph references the ADR identifier (ADR-001, ADR-002,
ADR-003, ADR-004) so it’s easy to find and follow the same formatting style as
Context/Decision sections.
- Line 26: Replace the misleading bullet "Faster to parse from the end of the
message." with a clarified statement that parsing trailers from the end requires
reading the entire commit message into memory and therefore is not inherently
faster; either remove the performance claim or rephrase to explain the trade-off
(e.g., "Parsing from the end can simplify trailer detection but requires the
full message to be read first, so it is not inherently faster").
In @docs/adr-tex-2/sections/10-quality.tex:
- Line 1: Add LaTeX labels to enable reliable cross-referencing: insert
\label{sec:quality} immediately after \section{Quality Requirements}, and add
descriptive labels to each relevant subsection such as \label{qs:tamper} for
"QS-1: Tamper Detection" and \label{qs:confidentiality} for the confidentiality
subsection (and a label for the Quality Tree e.g. \label{fig:quality-tree} if
it’s a figure) so other ADRs can reference these sections by name instead of by
page number.
- Around line 3-10: The "Quality Tree" subsection in 10-quality.tex conflicts
with the Quality Goals table in 01-introduction.tex because the former lists
four attributes (Security, Simplicity, Auditability, Performance) while the
latter includes a fifth priority, Portability; reconcile them by choosing a
single source of truth and updating the other: either add a fifth item to the
\subsection{Quality Tree} list—e.g., add "\item \textbf{Portability:} Platform
independence and small footprint" to match 01-introduction.tex—or remove/adjust
the Portability row in the Quality Goals table in 01-introduction.tex so both
files list the same four priorities; ensure wording and priority order match
exactly between the two sections.
- Around line 17-18: The wording overstates detection guarantees; update the two
lines that claim "immediately detected" and "100% detection" to a conditional
phrasing that ties detection to client verification. Replace the sentence with
something like: "The Merkle DAG checksum mismatch is detected by clients that
verify commit ancestry upon pull." and change the metric to a qualified
statement such as "Metric: detection rate among clients that perform
SHA/ancestry verification" (or similar wording) so the doc no longer claims
unconditional or universal detection.
In @docs/adr-tex-2/sections/11-risks.tex:
- Line 21: The file ends without a trailing newline which breaks POSIX line
semantics; open docs/adr-tex-2/sections/11-risks.tex, move to the end after the
final token (\end{table}) and add a single newline character so the file
terminates with a newline (ensure the last line is blank-terminated), then save
the file.
- Around line 10-21: The table environment is missing a caption and label for
cross-referencing; add a descriptive \caption{...} (e.g., "Risks and priorities
for ADR-TEX-2") and a unique \label{tab:risks-adr-tex-2} inside the same table
environment (typically immediately after \centering or right after \caption) so
the table can be referenced elsewhere with \ref{tab:risks-adr-tex-2} and remains
accessible.
- Line 7: The text recommends using `git gc --aggressive` for regular
maintenance but that flag is overly costly; change the wording in the sentence
that currently mentions "Regular \texttt{git gc --aggressive} and ref pruning
strategies are needed" to instead recommend regular `git gc` (without
`--aggressive`) combined with `git repack -adf` for routine maintenance and note
that `--aggressive` should be reserved for one‑time cleanups on very large
repos; keep the note about ref pruning strategies.
In @docs/adr-tex-2/sections/12-glossary.tex:
- Line 11: The glossary entry for the "Trailer" item is inaccurate by claiming
"RFC 822 metadata"; update the "Trailer" item text (the \item[Trailer] entry) to
a precise definition such as "Key-value pairs at the end of a commit message,
inspired by RFC 822 header format" (or similar wording) to reflect that Git
trailers are RFC 822–inspired but not RFC 822 compliant.
- Line 6: The glossary entry for "CAS" is ambiguous; update it to be explicit
for this ADR by either (a) replacing the ambiguous line with a single definitive
meaning "CAS — Content-Addressable Store" (since this ADR uses the Git
object-model sense), or (b) if both senses are truly needed, add two distinct
entries like "CAS (addressing) — Content-Addressable Store" and "CAS
(concurrency) — Compare-and-Swap", ensuring the ADR text and references use the
chosen labels consistently (refer to the CAS glossary item and any occurrences
of CAS in the ADR to adjust usage).
- Line 8: The glossary entry for "Empty Tree" currently uses a truncated OID;
update the \item[Empty Tree] line to include the canonical SHA-1 value instead
of an ellipsis (either the full 40-character SHA-1
4b825dc642cb6eb9a060e54bf8d69288fbee4904 or at minimum the first 12 characters)
so the ADR contains a searchable, canonical identifier for the empty tree.
In @docs/adr-tex-2/sections/C-related.tex:
- Line 3: In the documentation list item for "git-stargate" update the typo by
inserting a space so the phrase reads "enforcing FF-only" instead of
"enforcingFF-only" — locate the string "git-stargate: Git gateway for
enforcingFF-only and signing." and change it to "git-stargate: Git gateway for
enforcing FF-only and signing."
- Line 10: The item listing "RFC 822 (Internet Message Format)" is outdated;
replace that entry with "RFC 5322 (Internet Message Format)" and, if applicable,
update any surrounding text or citation commands in C-related.tex that reference
RFC 822 to instead reference RFC 5322 (the current Internet Message Format
standard) so the trailer-codec docs point to the modern spec.
In @docs/adr-tex-2/sections/D-references.tex:
- Line 7: The file ends with the token "\end{enumerate}" but lacks a trailing
newline; fix by adding a single newline character after "\end{enumerate}" in
docs/adr-tex-2/sections/D-references.tex so the file ends with a newline.
- Line 5: Update the reference item that currently reads "RFC 822 (Internet
Message Format)" to cite the current standard RFC 5322 instead (e.g., "RFC 5322
(Internet Message Format): \\url{https://tools.ietf.org/html/rfc5322}") or
include both with a supersession note (e.g., "RFC 822 (superseded by RFC 2822
and RFC 5322); see RFC 5322 for current standard"), making the change in the
LaTeX \item entry that mentions "RFC 822 (Internet Message Format)".
In @test/setup.bats:
- Around line 1-3: Add additional Bats tests in test/setup.bats to cover the
suggested edge cases: write one test that creates a regular file named
"git-stunts" (not a directory) and asserts setup.sh fails or handles it
correctly; add parameterized tests that run setup.sh with various case variants
of affirmative input ("Y", "yes", "YES") and verify behavior matches lower-case
"y"/"yes"; add a test that removes or renames package.json inside the git-cms
fixture and asserts setup.sh exits with the expected error or recovery path; and
add a test that simulates non-interactive vs interactive TTY behavior (use Bats
helpers or run with/without a pseudo-tty) to verify setup.sh’s TTY-detection
branch. Ensure each test names the scenario clearly and reuses existing fixtures
and the setup.sh invocation to keep consistency.
- Around line 21-23: The teardown function uses an unguarded rm -rf on TEST_DIR
which can run dangerously if TEST_DIR is empty or unset; update teardown to
first validate TEST_DIR is set and non-empty and refuse to run if it equals / or
. or /root (or otherwise clearly unsafe), and only then run rm -rf on that path
(also optionally check it exists and is under an expected base like /tmp or $PWD
to be extra safe); locate the teardown() function and add these guards around
the rm -rf invocation before removing "$TEST_DIR".
- Around line 4-19: The setup() helper uses a hardcoded ORIGINAL_DIR="/code" and
lacks error handling for mktemp and cp; change ORIGINAL_DIR to be
configurable/defaulting to the current working dir (e.g., respect ORIGINAL_DIR
env or fallback to $(pwd)) and add defensive checks after mktemp (TEST_DIR) and
each cp to fail fast with clear error messages; ensure setup() validates
TEST_DIR is non-empty and writable, that mktemp -d succeeded, and that copies of
scripts/package.json succeeded before continuing and exporting PATH.
- Around line 114-130: The test "setup offers to clone git-stunts if not found"
contains leftover debug echo statements (echo "Status: $status" >&3, echo
"Output:" >&3, echo "$output" >&3) that should be removed; delete those three
debug echoes from the test function so it only runs the setup, asserts the exit
status and matches the expected output lines without printing to fd3.
- Around line 75-82: The test replaces PATH entirely which is unsafe; instead
create an empty temporary mock bin directory and set PATH to that plus essential
system dirs so docker cannot be found while other utilities still work: in the
@test "setup checks for docker command" block, create a temp dir (e.g.,
mockbin=$(mktemp -d)), export PATH="$mockbin:/usr/bin:/bin" (or include any
other minimal dirs your environment needs), run bash scripts/setup.sh, and then
rm -rf "$mockbin" in teardown; this ensures docker is absent without nuking the
whole PATH.
- Around line 40-53: The heredoc in mock_git_clone_success is unquoted (<<EOF)
while mock_docker_working and mock_git_clone_fail use quoted heredocs (<<'EOF'),
creating inconsistent expansion behavior; either convert mock_git_clone_success
to a quoted heredoc (<<'EOF') and reference the TEST_DIR variable literally
inside the heredoc (e.g., use "$TEST_DIR" or avoid relying on local test_dir
expansion) so you no longer need to escape \$1, or keep the current unquoted
form but add a clear comment above mock_git_clone_success explaining why
unquoted expansion is intentional; update the function mock_git_clone_success
accordingly to match the style used by mock_docker_working and
mock_git_clone_fail or document the exception.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/adr-tex-2/main.pdfis excluded by!**/*.pdf
📒 Files selected for processing (25)
.gitignoredocs/adr-tex-2/Makefiledocs/adr-tex-2/figures/context.texdocs/adr-tex-2/figures/decomposition.texdocs/adr-tex-2/figures/draft-sequence.texdocs/adr-tex-2/figures/responsibilities.texdocs/adr-tex-2/main.texdocs/adr-tex-2/meta.texdocs/adr-tex-2/sections/01-introduction.texdocs/adr-tex-2/sections/02-constraints.texdocs/adr-tex-2/sections/03-context.texdocs/adr-tex-2/sections/04-solution.texdocs/adr-tex-2/sections/05-building-blocks.texdocs/adr-tex-2/sections/06-runtime.texdocs/adr-tex-2/sections/07-deployment.texdocs/adr-tex-2/sections/08-crosscutting.texdocs/adr-tex-2/sections/09-decisions.texdocs/adr-tex-2/sections/10-quality.texdocs/adr-tex-2/sections/11-risks.texdocs/adr-tex-2/sections/12-glossary.texdocs/adr-tex-2/sections/A-commands.texdocs/adr-tex-2/sections/B-structure.texdocs/adr-tex-2/sections/C-related.texdocs/adr-tex-2/sections/D-references.textest/setup.bats
🧰 Additional context used
🪛 checkmake (0.2.2)
docs/adr-tex-2/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 1-1: Target "all" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (17)
.gitignore (1)
9-10: Remove trailing whitespace from line 12;.obsidian/gitignore entry is fine.Your grand accusation about "schizophrenic" commit messages contradicting each other? Completely fabricated. No commits in this repository mention removing or adding Obsidian configurations. The
.obsidian/entry is a clean, uncontradicted addition to gitignore—your invented contradiction doesn't exist.What DOES exist: egregious trailing whitespace on line 12 (
*.auxhas a space after it). This is what you should actually care about.As an optional enhancement, the LaTeX patterns are incomplete. Consider adding:
.synctex.gz,.fls,.fdb_latexmk,.bbl,.blg,.idx,.ind,.ilg,.lof,.lotfor comprehensive artifact coverage.Likely an incorrect or invalid review comment.
test/setup.bats (3)
25-38: Acceptable mock implementation.The heredoc uses
<<'EOF'correctly to prevent variable expansion, and the logic handles bothdocker infoand default version output. No issues here.
55-66: Solid mock with authentic exit code.Using exit code 128 mirrors real
git clonefailure behavior. Good attention to detail.
132-166: Tests cover critical paths adequately.The clone success/failure scenarios and the next-steps output verification are solid. Line 141's directory existence check correctly validates the side effect of the mocked git clone.
docs/adr-tex-2/figures/context.tex (1)
1-12: Remove unfounded claim about undefined styles—they're properly defined in the main.tex preamble.The
blockandlinestyles used here ARE defined (docs/adr-tex-2/main.tex:63-65), so the diagram will compile without issue. This is standard LaTeX practice: the preamble defines reusable styles, included files use them.However, the file is missing a newline at EOF—add one after
\end{tikzpicture}.Likely an incorrect or invalid review comment.
docs/adr-tex-2/sections/01-introduction.tex (1)
55-64: Non-Goals section is refreshingly honest.Explicit exclusions (high-velocity writes, complex queries, large-scale collaboration, real-time updates) prevent scope creep and set correct expectations. This is good ADR hygiene.
docs/adr-tex-2/sections/09-decisions.tex (1)
31-47: ADR-003 and ADR-004 rationales are solid.Fast-forward-only publishing to preserve commit SHA integrity and client-side AES-256-GCM encryption are well-justified. These align with the security and auditability quality goals.
docs/adr-tex-2/sections/05-building-blocks.tex (1)
1-21: LGTM — Building Block View structure is sound.The section follows arc42 conventions correctly. Figure inclusions reference the appropriate TikZ diagrams added in this PR. The
[H]placement specifier and\resizeboxscaling are appropriate for architectural diagrams.docs/adr-tex-2/sections/B-structure.tex (1)
1-17: LGTM — Directory structure documentation is accurate.The listing correctly reflects the CmsService-centric architecture introduced in this PR. ASCII tree notation is conventional and readable.
docs/adr-tex-2/sections/A-commands.tex (1)
1-21: LGTM — CLI command examples are clear and consistent.Examples demonstrate the four primary operations (draft, publish, list, upload) with minimal, reproducible invocations. Aligns with the CmsService API surface documented elsewhere.
docs/adr-tex-2/sections/06-runtime.tex (1)
17-17: The 256KB chunk size claim cannot be verified in this repository — it lives in an external dependency (@git-stunts/cas) with no exposed constant or documentation link here.The chunking logic is entirely delegated to the
@git-stunts/caspackage (seesrc/lib/CmsService.jsline 127). No chunk size constant exists insrc/, making this documentation claim unverifiable from the local codebase. Either:
- Link to the source package documentation that defines this constant
- Add a code comment pinning this to the exact dependency version and constant name
- Or this drifts silently the next time the package updates
Documentation drift on security-critical parameters (chunk size + encryption) is non-negotiable.
docs/adr-tex-2/main.tex (1)
1-113: Structure is sound.The modular organization with separate section files, proper use of
\appendix, and consistent style definitions is well-executed. The package selection is appropriate for technical documentation.docs/adr-tex-2/sections/08-crosscutting.tex (1)
1-56: Merkle DAG visualization is correct.The arrows correctly represent Git's "child points to parent" semantics, and the ref-to-commit dashed lines appropriately distinguish mutable references from immutable content-addressed objects. Solid work.
docs/adr-tex-2/figures/decomposition.tex (1)
1-21: Diagram accurately represents the layered architecture.The Application Layer / Lego Blocks decomposition is clear, the dependency arrows flow correctly downward, and the component labeling with source paths provides useful traceability. No structural issues.
docs/adr-tex-2/sections/03-context.tex (1)
1-93: Scope boundaries are well-defined and appropriately constrained.The explicit In Scope / Out of Scope delineation prevents scope creep and sets correct expectations. External interface documentation is thorough. The system context diagram correctly captures the essential data flows.
docs/adr-tex-2/sections/04-solution.tex (2)
31-31: Empty tree hash is correct—well done.The SHA-1
4b825dc642cb6eb9a060e54bf8d69288fbee4904is indeed the canonical empty tree object. This is a clever exploitation of Git internals. The documentation accurately describes the technique.
1-62: Overall: Solid ADR section with clear architectural rationale.The structure is disciplined—principles are enumerated (P-1 to P-4), decisions follow a consistent format (Alternative/Decision/Rationale), and the "Empty Tree" technique is well-documented. This is the kind of architectural documentation that actually gets read.
Minor fixes needed for LaTeX typography and one clarification for "backward parsing." The SHA-1 caveat is worth addressing to avoid overpromising cryptographic guarantees.
Comment on lines
+11
to
+15
| # LaTeX artifacts | ||
| *.aux | ||
| *.log | ||
| *.out | ||
| *.toc |
There was a problem hiding this comment.
Eradicate trailing whitespace and complete your LaTeX artifact coverage.
Two infractions detected:
-
Trailing whitespace on line 12 (
*.auxhas a space after it). Absolutely unacceptable. Your .gitignore should be pristine. -
Woefully incomplete LaTeX artifact list. You've listed four patterns but ignored the dozens of other build artifacts LaTeX compilers generate:
*.pdf(compiled output)*.synctex.gz(SyncTeX correlation data)*.fls,*.fdb_latexmk(latexmk tracking)*.bbl,*.blg(BibTeX bibliography)*.idx,*.ind,*.ilg(makeindex)*.lof,*.lot(list of figures/tables)*.nav,*.snm,*.vrb(Beamer presentation)*.dvi,*.ps(legacy formats)
Are you generating LaTeX docs or not? If yes, do it properly.
🔥 Proposed fix for trailing whitespace and comprehensive LaTeX ignores
# LaTeX artifacts
-*.aux
+*.aux
*.log
*.out
*.toc
+*.pdf
+*.synctex.gz
+*.fls
+*.fdb_latexmk
+*.bbl
+*.blg
+*.idx
+*.ind
+*.ilg
+*.lof
+*.lot
+*.nav
+*.snm
+*.vrb
+*.dvi
+*.ps📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # LaTeX artifacts | |
| *.aux | |
| *.log | |
| *.out | |
| *.toc | |
| # LaTeX artifacts | |
| *.aux | |
| *.log | |
| *.out | |
| *.toc | |
| *.synctex.gz | |
| *.fls | |
| *.fdb_latexmk | |
| *.bbl | |
| *.blg | |
| *.idx | |
| *.ind | |
| *.ilg | |
| *.lof | |
| *.lot | |
| *.nav | |
| *.snm | |
| *.vrb | |
| *.dvi | |
| *.ps |
🤖 Prompt for AI Agents
In @.gitignore around lines 11 - 15, Remove the trailing space from the '*.aux '
entry (make it '*.aux') and expand the LaTeX artifact coverage by adding common
build outputs alongside the existing '*.log', '*.out', and '*.toc': include
'*.pdf', '*.synctex.gz', '*.fls', '*.fdb_latexmk', '*.bbl', '*.blg', '*.idx',
'*.ind', '*.ilg', '*.lof', '*.lot', '*.nav', '*.snm', '*.vrb', '*.dvi', and
'*.ps' so the .gitignore fully ignores typical LaTeX artifacts.
| \draw [line] (GitCMS) -- node [scale=0.8] {git push} (Stargate); | ||
| \draw [line] (GitCMS) -- node [scale=0.8] {read/write} (LocalRepo); | ||
| \draw [line] (Stargate) -- node [scale=0.8] {mirror} (PublicMirror); | ||
| \end{tikzpicture} No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file.
Fourth file, fourth offense. Trailing newline missing.
🤖 Prompt for AI Agents
In @docs/adr-tex-2/figures/context.tex at line 12, The file ends with
"\end{tikzpicture}" but is missing a trailing newline; add a single newline
character at the end of docs/adr-tex-2/figures/context.tex so the file ends with
"\end{tikzpicture}" followed by a newline (ensure the editor saves with a final
newline/EOF marker).
| \draw [->] ($(CLI)+(0,-1.5)$) -- node [scale=0.7] {saveSnapshot()} ($(CMS)+(0,-1.5)$); | ||
| \draw [->] ($(CMS)+(0,-2.2)$) -- node [scale=0.7] {revParse(ref)} ($(PL)+(0,-2.2)$); | ||
| \draw [<-] ($(CMS)+(0,-3)$) -- node [scale=0.7] {null} ($(PL)+(0,-3)$); | ||
| \draw [->] ($(CMS)+(0,-4)$) -- node [scale=0.7] {createNode()} ($(CMS)+(0.8,-4.2)$); |
There was a problem hiding this comment.
Broken self-call arrow — this draws a nearly invisible stub.
Line 15 draws from (CMS)+(0,-4) to (CMS)+(0.8,-4.2) — a delta of merely (0.8, -0.2). This produces a tiny, illegible arrow that fails to convey self-invocation semantics.
In UML sequence diagrams, a self-call should use a loop-back arrow (right, down, left) or delegate to another lifeline. If createNode() is internal to Service, use a proper self-activation loop. If it delegates to Plumbing or empty-graph, route the arrow to PL.
🔧 Proposed fix — proper self-loop
- \draw [->] ($(CMS)+(0,-4)$) -- node [scale=0.7] {createNode()} ($(CMS)+(0.8,-4.2)$);
+ % Self-call loop for createNode()
+ \draw [->] ($(CMS)+(0.1,-4)$) -- ++(1,0) -- ++(0,-0.5) -- ++(-1,0) node [midway, above, scale=0.7] {createNode()};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| \draw [->] ($(CMS)+(0,-4)$) -- node [scale=0.7] {createNode()} ($(CMS)+(0.8,-4.2)$); | |
| % Self-call loop for createNode() | |
| \draw [->] ($(CMS)+(0.1,-4)$) -- ++(1,0) -- ++(0,-0.5) -- ++(-1,0) node [midway, above, scale=0.7] {createNode()}; |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/figures/draft-sequence.tex at line 15, The arrow from
(CMS)+(0,-4) to (CMS)+(0.8,-4.2) for createNode() is too short and looks like a
stub; replace it with a proper self-call loop (right, down, left) anchored on
the CMS lifeline (e.g., draw a horizontal segment right, a vertical down, then
back left with the arrow and activation box) to express an internal
self-invocation of Service createNode(), or if createNode() delegates
externally, route the arrow to the PL (or empty-graph) lifeline instead so the
arrow terminates at PL; update the draw command referencing CMS and createNode()
accordingly to implement one of these two clear alternatives.
Comment on lines
+1
to
+2
| \begin{tikzpicture}[node distance=1cm, auto, font=\small] | ||
| \node [blockshaded, text width=2.5cm, minimum height=8cm] (CMS) {\textbf{CmsService}}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cd docs/adr-tex-2 && find . -name "*.tex" | head -20Repository: flyingrobots/git-cms
Length of output: 587
🏁 Script executed:
cat -n docs/adr-tex-2/figures/responsibilities.texRepository: flyingrobots/git-cms
Length of output: 1321
🏁 Script executed:
# Search for TikZ style definitions in the ADR directory
rg -n "blockshaded|dashed-line|tikzset" docs/adr-tex-2/Repository: flyingrobots/git-cms
Length of output: 938
🏁 Script executed:
# Also check main.tex or any preamble files
find docs/adr-tex-2 -name "*.tex" -type f -exec grep -l "blockshaded\|tikzset" {} \;Repository: flyingrobots/git-cms
Length of output: 172
🏁 Script executed:
# Get the full tikzset definition from main.tex
sed -n '61,70p' docs/adr-tex-2/main.texRepository: flyingrobots/git-cms
Length of output: 432
🏁 Script executed:
# Search for 'block' and 'line' style definitions
rg -A 1 "block/.style|line/.style" docs/adr-tex-2/main.texRepository: flyingrobots/git-cms
Length of output: 315
Remove the dead node distance=1cm parameter—it's completely bypassed.
Line 1 declares node distance=1cm, but every node placement from line 3 onwards explicitly overrides it with right=3cm of CMS.north, below=0.5cm of, etc. The CMS node (line 2) uses no positioning at all. This parameter is inert and serves only as misleading clutter.
All style definitions (blockshaded, block, line, dashed-line) are correctly defined in main.tex lines 61–67. No issues there.
🧹 Cleanup
-\begin{tikzpicture}[node distance=1cm, auto, font=\small]
+\begin{tikzpicture}[auto, font=\small]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| \begin{tikzpicture}[node distance=1cm, auto, font=\small] | |
| \node [blockshaded, text width=2.5cm, minimum height=8cm] (CMS) {\textbf{CmsService}}; | |
| \begin{tikzpicture}[auto, font=\small] | |
| \node [blockshaded, text width=2.5cm, minimum height=8cm] (CMS) {\textbf{CmsService}}; |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/figures/responsibilities.tex around lines 1 - 2, The
tikzpicture declares a dead parameter "node distance=1cm" which is never used
because all nodes are placed explicitly (e.g., CMS node and others use explicit
anchors like "right=3cm of CMS.north" and "below=0.5cm of ..."); remove the
redundant "node distance=1cm" from the tikzpicture options to avoid misleading
clutter and keep the picture options minimal (look for the tikzpicture line
containing "node distance=1cm" and remove that token while leaving other options
and the CMS node definition intact).
| @@ -0,0 +1,8 @@ | |||
| all: main.pdf | |||
There was a problem hiding this comment.
Missing .PHONY declarations will cause silent build failures.
When someone inevitably creates a file named all or clean in this directory (or Make's timestamp logic misfires), your entire build pipeline collapses. Declare your phony targets explicitly.
🔧 Proposed fix
+.PHONY: all clean
+
all: main.pdf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| all: main.pdf | |
| .PHONY: all clean | |
| all: main.pdf |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 1-1: Target "all" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In @docs/adr-tex-2/Makefile at line 1, The Makefile lacks .PHONY declarations
which can cause silent failures if files named like targets appear; add a .PHONY
line listing the phony targets (at minimum "all" and "clean") near the top of
the Makefile so Make treats those targets as always out-of-date (e.g., add a
rule ".PHONY: all clean" above the "all: main.pdf" target); update any other
non-file targets used in this file to the .PHONY list as well.
Comment on lines
+4
to
+19
| setup() { | ||
| # Create a temporary test directory | ||
| export TEST_DIR="$(mktemp -d)" | ||
| export ORIGINAL_DIR="/code" | ||
|
|
||
| # Copy setup script to test directory | ||
| mkdir -p "$TEST_DIR/git-cms/scripts" | ||
| cp "$ORIGINAL_DIR/scripts/setup.sh" "$TEST_DIR/git-cms/scripts/setup.sh" | ||
| cp "$ORIGINAL_DIR/package.json" "$TEST_DIR/git-cms/package.json" | ||
|
|
||
| # Create mocks directory | ||
| export PATH="$TEST_DIR/mocks:$PATH" | ||
| mkdir -p "$TEST_DIR/mocks" | ||
|
|
||
| cd "$TEST_DIR/git-cms" | ||
| } |
There was a problem hiding this comment.
Hardcoded /code path is brittle and undocumented.
Line 7 assumes tests run in a Docker container with code mounted at /code. This will explode if anyone attempts to run BATS locally. Either:
- Document this requirement prominently in the file header.
- Make it configurable:
ORIGINAL_DIR="${ORIGINAL_DIR:-$(pwd)}"or derive it dynamically.
Additionally, no error handling exists for mktemp or cp failures. If mktemp -d fails, TEST_DIR is empty and subsequent operations become undefined behavior nightmares.
Proposed defensive fix
setup() {
# Create a temporary test directory
export TEST_DIR="$(mktemp -d)"
- export ORIGINAL_DIR="/code"
+ export ORIGINAL_DIR="${ORIGINAL_DIR:-/code}"
+
+ # Fail fast if temp dir creation failed
+ [[ -d "$TEST_DIR" ]] || { echo "Failed to create temp directory" >&2; return 1; }
# Copy setup script to test directory
mkdir -p "$TEST_DIR/git-cms/scripts"
- cp "$ORIGINAL_DIR/scripts/setup.sh" "$TEST_DIR/git-cms/scripts/setup.sh"
- cp "$ORIGINAL_DIR/package.json" "$TEST_DIR/git-cms/package.json"
+ cp "$ORIGINAL_DIR/scripts/setup.sh" "$TEST_DIR/git-cms/scripts/setup.sh" || return 1
+ cp "$ORIGINAL_DIR/package.json" "$TEST_DIR/git-cms/package.json" || return 1🤖 Prompt for AI Agents
In @test/setup.bats around lines 4 - 19, The setup() helper uses a hardcoded
ORIGINAL_DIR="/code" and lacks error handling for mktemp and cp; change
ORIGINAL_DIR to be configurable/defaulting to the current working dir (e.g.,
respect ORIGINAL_DIR env or fallback to $(pwd)) and add defensive checks after
mktemp (TEST_DIR) and each cp to fail fast with clear error messages; ensure
setup() validates TEST_DIR is non-empty and writable, that mktemp -d succeeded,
and that copies of scripts/package.json succeeded before continuing and
exporting PATH.
Comment on lines
+21
to
+23
| teardown() { | ||
| rm -rf "$TEST_DIR" | ||
| } |
There was a problem hiding this comment.
Unguarded rm -rf is playing with fire.
If TEST_DIR is unset or empty due to a setup failure, this becomes rm -rf "" which on some systems can behave unexpectedly. Add a guard.
Defensive fix
teardown() {
- rm -rf "$TEST_DIR"
+ [[ -n "$TEST_DIR" && -d "$TEST_DIR" ]] && rm -rf "$TEST_DIR"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| teardown() { | |
| rm -rf "$TEST_DIR" | |
| } | |
| teardown() { | |
| [[ -n "$TEST_DIR" && -d "$TEST_DIR" ]] && rm -rf "$TEST_DIR" | |
| } |
🤖 Prompt for AI Agents
In @test/setup.bats around lines 21 - 23, The teardown function uses an
unguarded rm -rf on TEST_DIR which can run dangerously if TEST_DIR is empty or
unset; update teardown to first validate TEST_DIR is set and non-empty and
refuse to run if it equals / or . or /root (or otherwise clearly unsafe), and
only then run rm -rf on that path (also optionally check it exists and is under
an expected base like /tmp or $PWD to be extra safe); locate the teardown()
function and add these guards around the rm -rf invocation before removing
"$TEST_DIR".
Comment on lines
+40
to
+53
| # Helper: Create mock git that succeeds at cloning | ||
| mock_git_clone_success() { | ||
| local test_dir="$TEST_DIR" | ||
| cat > "$TEST_DIR/mocks/git" <<EOF | ||
| #!/bin/bash | ||
| if [[ "\$1" == "clone" ]]; then | ||
| mkdir -p "$test_dir/git-stunts" | ||
| echo "Cloning into 'git-stunts'..." | ||
| exit 0 | ||
| fi | ||
| exit 0 | ||
| EOF | ||
| chmod +x "$TEST_DIR/mocks/git" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Heredoc quoting inconsistency creates cognitive overhead.
This function uses <<EOF (unquoted, variables expand) while mock_docker_working and mock_git_clone_fail use <<'EOF' (quoted, no expansion). The escaping of \$1 on line 45 hints at the complexity.
This works, but the inconsistency forces future maintainers to pause and reason about shell expansion rules. Consider documenting why this differs, or restructure to use consistent quoting:
Alternative using consistent quoting
mock_git_clone_success() {
- local test_dir="$TEST_DIR"
- cat > "$TEST_DIR/mocks/git" <<EOF
+ cat > "$TEST_DIR/mocks/git" <<'OUTER_EOF'
#!/bin/bash
-if [[ "\$1" == "clone" ]]; then
- mkdir -p "$test_dir/git-stunts"
+if [[ "$1" == "clone" ]]; then
+ # Create sibling to git-cms; script cds to git-cms, so ../git-stunts
+ mkdir -p "$(dirname "$(pwd)")/git-stunts"
echo "Cloning into 'git-stunts'..."
exit 0
fi
exit 0
-EOF
+OUTER_EOF
chmod +x "$TEST_DIR/mocks/git"
}Or, keep the current approach but add a comment explaining the deliberate use of unquoted heredoc.
🤖 Prompt for AI Agents
In @test/setup.bats around lines 40 - 53, The heredoc in mock_git_clone_success
is unquoted (<<EOF) while mock_docker_working and mock_git_clone_fail use quoted
heredocs (<<'EOF'), creating inconsistent expansion behavior; either convert
mock_git_clone_success to a quoted heredoc (<<'EOF') and reference the TEST_DIR
variable literally inside the heredoc (e.g., use "$TEST_DIR" or avoid relying on
local test_dir expansion) so you no longer need to escape \$1, or keep the
current unquoted form but add a clear comment above mock_git_clone_success
explaining why unquoted expansion is intentional; update the function
mock_git_clone_success accordingly to match the style used by
mock_docker_working and mock_git_clone_fail or document the exception.
Comment on lines
+75
to
+82
| @test "setup checks for docker command" { | ||
| # Remove docker from PATH | ||
| export PATH="/usr/bin:/bin" | ||
|
|
||
| run bash scripts/setup.sh | ||
| [ "$status" -eq 1 ] | ||
| [[ "$output" =~ "Docker not found" ]] | ||
| } |
There was a problem hiding this comment.
Complete PATH replacement is reckless.
Line 77 nukes the entire PATH to simulate missing docker. This could break the shell itself or other commands the setup script depends on (e.g., command -v, which, utilities). It works by accident because /bin/bash exists.
A safer approach: explicitly ensure no docker exists in the search path without destroying everything.
Safer approach
@test "setup checks for docker command" {
- # Remove docker from PATH
- export PATH="/usr/bin:/bin"
+ # Remove mocks from PATH to ensure no mock docker; rely on real PATH lacking docker
+ # Or create a stub that doesn't exist
+ rm -f "$TEST_DIR/mocks/docker" 2>/dev/null || true
+ # Ensure no real docker by temporarily hiding it
+ export PATH="$TEST_DIR/mocks" # Only mocks dir, which has no docker
run bash scripts/setup.sh
[ "$status" -eq 1 ]
[[ "$output" =~ "Docker not found" ]]
}Or create an empty mocks directory and set PATH to only that (plus essential dirs for bash).
🤖 Prompt for AI Agents
In @test/setup.bats around lines 75 - 82, The test replaces PATH entirely which
is unsafe; instead create an empty temporary mock bin directory and set PATH to
that plus essential system dirs so docker cannot be found while other utilities
still work: in the @test "setup checks for docker command" block, create a temp
dir (e.g., mockbin=$(mktemp -d)), export PATH="$mockbin:/usr/bin:/bin" (or
include any other minimal dirs your environment needs), run bash
scripts/setup.sh, and then rm -rf "$mockbin" in teardown; this ensures docker is
absent without nuking the whole PATH.
Comment on lines
+114
to
+130
| @test "setup offers to clone git-stunts if not found" { | ||
| mock_docker_working | ||
|
|
||
| # Don't create git-stunts | ||
| # Simulate user declining (send 'n' to stdin) | ||
| run bash scripts/setup.sh <<< "n" | ||
|
|
||
| # Debug: print actual output | ||
| echo "Status: $status" >&3 | ||
| echo "Output:" >&3 | ||
| echo "$output" >&3 | ||
|
|
||
| [ "$status" -eq 1 ] | ||
| [[ "$output" =~ "git-stunts not found" ]] | ||
| [[ "$output" =~ "Would you like me to clone it now" ]] | ||
| [[ "$output" =~ "Setup cancelled" ]] | ||
| } |
There was a problem hiding this comment.
Debug cruft committed to the repository.
Lines 121-124 are debug echo statements that will pollute test output on every run. These were clearly left behind from debugging a failing test. Remove them.
Clean it up
# Simulate user declining (send 'n' to stdin)
run bash scripts/setup.sh <<< "n"
- # Debug: print actual output
- echo "Status: $status" >&3
- echo "Output:" >&3
- echo "$output" >&3
-
[ "$status" -eq 1 ]
[[ "$output" =~ "git-stunts not found" ]]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @test "setup offers to clone git-stunts if not found" { | |
| mock_docker_working | |
| # Don't create git-stunts | |
| # Simulate user declining (send 'n' to stdin) | |
| run bash scripts/setup.sh <<< "n" | |
| # Debug: print actual output | |
| echo "Status: $status" >&3 | |
| echo "Output:" >&3 | |
| echo "$output" >&3 | |
| [ "$status" -eq 1 ] | |
| [[ "$output" =~ "git-stunts not found" ]] | |
| [[ "$output" =~ "Would you like me to clone it now" ]] | |
| [[ "$output" =~ "Setup cancelled" ]] | |
| } | |
| @test "setup offers to clone git-stunts if not found" { | |
| mock_docker_working | |
| # Don't create git-stunts | |
| # Simulate user declining (send 'n' to stdin) | |
| run bash scripts/setup.sh <<< "n" | |
| [ "$status" -eq 1 ] | |
| [[ "$output" =~ "git-stunts not found" ]] | |
| [[ "$output" =~ "Would you like me to clone it now" ]] | |
| [[ "$output" =~ "Setup cancelled" ]] | |
| } |
🤖 Prompt for AI Agents
In @test/setup.bats around lines 114 - 130, The test "setup offers to clone
git-stunts if not found" contains leftover debug echo statements (echo "Status:
$status" >&3, echo "Output:" >&3, echo "$output" >&3) that should be removed;
delete those three debug echoes from the test function so it only runs the
setup, asserts the exit status and matches the expected output lines without
printing to fd3.
Comment on lines
+16
to
+20
| \draw [line] (CLI) -- (CMS); \draw [line] (HTTP) -- (CMS); | ||
| \draw [line] (CMS) -- (Codec); \draw [line] (CMS) -- (Graph); | ||
| \draw [line] (CMS) -- (CAS); \draw [line] (CMS) -- (Vault); | ||
| \draw [line] (CMS) -- (Plumbing); \draw [line] (Graph) -- (Plumbing); | ||
| \draw [line] (CAS) -- (Plumbing); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Multiple \draw commands per line — sacrificing readability for density.
You've crammed multiple \draw commands onto single lines. This compiles fine, but when someone needs to adjust arrow routing or debug a visual issue, they're parsing semicolon-delimited statements instead of scanning lines. Break them out.
♻️ One draw per line
- \draw [line] (CLI) -- (CMS); \draw [line] (HTTP) -- (CMS);
- \draw [line] (CMS) -- (Codec); \draw [line] (CMS) -- (Graph);
- \draw [line] (CMS) -- (CAS); \draw [line] (CMS) -- (Vault);
- \draw [line] (CMS) -- (Plumbing); \draw [line] (Graph) -- (Plumbing);
- \draw [line] (CAS) -- (Plumbing);
+ \draw [line] (CLI) -- (CMS);
+ \draw [line] (HTTP) -- (CMS);
+ \draw [line] (CMS) -- (Codec);
+ \draw [line] (CMS) -- (Graph);
+ \draw [line] (CMS) -- (CAS);
+ \draw [line] (CMS) -- (Vault);
+ \draw [line] (CMS) -- (Plumbing);
+ \draw [line] (Graph) -- (Plumbing);
+ \draw [line] (CAS) -- (Plumbing);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| \draw [line] (CLI) -- (CMS); \draw [line] (HTTP) -- (CMS); | |
| \draw [line] (CMS) -- (Codec); \draw [line] (CMS) -- (Graph); | |
| \draw [line] (CMS) -- (CAS); \draw [line] (CMS) -- (Vault); | |
| \draw [line] (CMS) -- (Plumbing); \draw [line] (Graph) -- (Plumbing); | |
| \draw [line] (CAS) -- (Plumbing); | |
| \draw [line] (CLI) -- (CMS); | |
| \draw [line] (HTTP) -- (CMS); | |
| \draw [line] (CMS) -- (Codec); | |
| \draw [line] (CMS) -- (Graph); | |
| \draw [line] (CMS) -- (CAS); | |
| \draw [line] (CMS) -- (Vault); | |
| \draw [line] (CMS) -- (Plumbing); | |
| \draw [line] (Graph) -- (Plumbing); | |
| \draw [line] (CAS) -- (Plumbing); |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/figures/decomposition.tex around lines 16 - 20, The LaTeX
TikZ lines contain multiple semicolon-separated \draw commands per line (e.g.,
commands involving nodes CLI, CMS, HTTP, Codec, Graph, CAS, Vault, Plumbing),
which hurts readability and editing; split each \draw invocation onto its own
line so each edge like "\draw [line] (CLI) -- (CMS);" "\draw [line] (HTTP) --
(CMS);" "\draw [line] (CMS) -- (Codec);" "\draw [line] (CMS) -- (Graph);" "\draw
[line] (CMS) -- (CAS);" "\draw [line] (CMS) -- (Vault);" "\draw [line] (CMS) --
(Plumbing);" "\draw [line] (Graph) -- (Plumbing);" and "\draw [line] (CAS) --
(Plumbing);" becomes a single-line statement to improve maintainability and ease
future edits.
Comment on lines
+23
to
+30
| \usepackage[hidelinks]{hyperref} | ||
|
|
||
| % --- Typography & Layout --- | ||
| \hypersetup{ | ||
| colorlinks=false, | ||
| pdftitle={Architecture Decision Record: Git CMS}, | ||
| pdfauthor={James Ross} | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant hyperref configuration — you're saying the same thing twice.
Line 23 loads hyperref with [hidelinks], which already disables link coloring/boxing. Then lines 26-30 set colorlinks=false again. Pick one approach. This kind of redundancy breeds confusion when someone tries to enable colored links later and wonders why it's not working.
♻️ Proposed consolidation
-\usepackage[hidelinks]{hyperref}
+\usepackage{hyperref}
% --- Typography & Layout ---
\hypersetup{
- colorlinks=false,
+ hidelinks,
pdftitle={Architecture Decision Record: Git CMS},
pdfauthor={James Ross}
}🤖 Prompt for AI Agents
In @docs/adr-tex-2/main.tex around lines 23 - 30, The hyperref package is being
configured twice: \usepackage[hidelinks]{hyperref} already disables link styling
and then \hypersetup{colorlinks=false,...} repeats that; remove the redundancy
by choosing one configuration method—either keep
\usepackage[hidelinks]{hyperref} and delete the colorlinks=false entry from
\hypersetup, or remove the hidelinks option and only set colorlinks=false (and
other PDF metadata) in \hypersetup—update the lines referencing
\usepackage[hidelinks]{hyperref} and the \hypersetup block accordingly.
Comment on lines
+82
to
+84
| \textbf{Author:} James Ross \\ | ||
| \textbf{Version:} 1.0.0 \\ | ||
| \textbf{Date:} 2026-01-11 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded date is a maintenance liability.
2026-01-11 is frozen in time. Every revision requires a manual edit that someone will inevitably forget. Use \today or externalize the date to meta.tex where it belongs with other document metadata.
♻️ Proposed fix
\textbf{Author:} James Ross \\
- \textbf{Version:} 1.0.0 \\
- \textbf{Date:} 2026-01-11
+ \textbf{Version:} \docversion \\
+ \textbf{Date:} \docdateThen define \docversion and \docdate in meta.tex for centralized management, or use \today if automatic dating is acceptable.
🤖 Prompt for AI Agents
In @docs/adr-tex-2/main.tex around lines 82 - 84, Replace the hardcoded
"2026-01-11" date with a reusable macro: either use \today directly or reference
a meta macro like \docdate (and likewise replace the version literal with
\docversion); define \docdate and \docversion in a central meta.tex and ensure
that meta.tex is \input or \include from the document preamble so the main.tex
header uses those centralized values.
Comment on lines
+4
to
+5
| pdflatex -interaction=nonstopmode main.tex | ||
| pdflatex -interaction=nonstopmode main.tex |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No error short-circuiting — second pdflatex runs even when first fails.
If the first compilation explodes, you'll still blindly execute the second pass, wasting time and burying the actual error under a cascade of secondary failures. Consider using && or Make's default error behavior properly.
♻️ Proposed fix with proper chaining
main.pdf: main.tex meta.tex $(wildcard sections/*.tex) $(wildcard figures/*.tex)
- pdflatex -interaction=nonstopmode main.tex
- pdflatex -interaction=nonstopmode main.tex
+ pdflatex -interaction=nonstopmode main.tex && \
+ pdflatex -interaction=nonstopmode main.texAlternatively, consider latexmk which handles dependency tracking and multiple passes automatically:
main.pdf: main.tex meta.tex $(wildcard sections/*.tex) $(wildcard figures/*.tex)
latexmk -pdf -interaction=nonstopmode main.tex📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pdflatex -interaction=nonstopmode main.tex | |
| pdflatex -interaction=nonstopmode main.tex | |
| main.pdf: main.tex meta.tex $(wildcard sections/*.tex) $(wildcard figures/*.tex) | |
| pdflatex -interaction=nonstopmode main.tex && \ | |
| pdflatex -interaction=nonstopmode main.tex |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/Makefile around lines 4 - 5, The Makefile currently runs two
unconditional pdflatex passes ("pdflatex -interaction=nonstopmode main.tex") so
the second run still executes if the first fails; change the rule to
short-circuit on error by chaining the commands with && (e.g., run the second
pdflatex only if the first succeeds) or replace both calls with a single robust
tool like latexmk (e.g., use a target such as main.pdf that invokes "latexmk
-pdf -interaction=nonstopmode main.tex") so multiple passes and dependencies are
handled automatically.
Comment on lines
+7
to
+8
| clean: | ||
| rm -f *.aux *.log *.out *.toc *.pdf |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
clean target is dangerously incomplete.
You're only nuking *.aux *.log *.out *.toc *.pdf in the current directory. LaTeX vomits auxiliary files everywhere. You're missing .fls, .fdb_latexmk, .synctex.gz, .nav, .snm, .bbl, .blg, and critically — any auxiliary files that \inputed files from sections/ or figures/ might generate.
♻️ Proposed comprehensive clean
clean:
- rm -f *.aux *.log *.out *.toc *.pdf
+ rm -f *.aux *.log *.out *.toc *.pdf *.fls *.fdb_latexmk *.synctex.gz
+ rm -f sections/*.aux figures/*.auxOr use latexmk -C for thorough cleanup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| clean: | |
| rm -f *.aux *.log *.out *.toc *.pdf | |
| clean: | |
| rm -f *.aux *.log *.out *.toc *.pdf *.fls *.fdb_latexmk *.synctex.gz | |
| rm -f sections/*.aux figures/*.aux |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/Makefile around lines 7 - 8, The Makefile's clean target only
removes a few local extensions and misses many LaTeX auxiliaries and files
generated in subdirectories; update the clean target (named "clean") to remove
additional extensions (.fls, .fdb_latexmk, .synctex.gz, .nav, .snm, .bbl, .blg)
and recursively delete auxiliaries from subfolders (e.g., sections/, figures/)
or replace the current clean recipe with a call to latexmk -C to perform a
thorough project-wide cleanup instead.
|
|
||
| \textbf{Benefit:} Audit trails are mathematically verifiable, not just trust-based. | ||
|
|
||
| \subsection{Solution Approach: The \"Empty Tree\" Stunt} |
There was a problem hiding this comment.
Incorrect LaTeX quotation marks.
\"Empty Tree\" produces escaped ASCII quotes. LaTeX requires backticks for opening and apostrophes for closing to render proper typographic quotes.
📝 Proposed fix
-\subsection{Solution Approach: The \"Empty Tree\" Stunt}
+\subsection{Solution Approach: The ``Empty Tree'' Stunt}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| \subsection{Solution Approach: The \"Empty Tree\" Stunt} | |
| \subsection{Solution Approach: The ``Empty Tree'' Stunt} |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/sections/04-solution.tex at line 25, The subsection title
uses incorrect LaTeX quoting \"Empty Tree\"; replace the escaped ASCII quotes
with proper TeX opening and closing quotes by changing \"Empty Tree\" to ``Empty
Tree'' in the \subsection{Solution Approach: The "Empty Tree" Stunt} line (look
for the \subsection declaration containing Empty Tree) so the title renders
typographic quotation marks in LaTeX.
| \textbf{Result:} The repository's working directory remains clean. All content lives in \texttt{.git/objects/} and \texttt{.git/refs/}. | ||
|
|
||
| \paragraph{Architectural Pattern: Event Sourcing} | ||
| Each draft save creates a new commit. The \"current\" article is the ref's tip, but the full history is a linked list of commits. |
There was a problem hiding this comment.
Same LaTeX quotation issue.
📝 Proposed fix
-Each draft save creates a new commit. The \"current\" article is the ref's tip, but the full history is a linked list of commits.
+Each draft save creates a new commit. The ``current'' article is the ref's tip, but the full history is a linked list of commits.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Each draft save creates a new commit. The \"current\" article is the ref's tip, but the full history is a linked list of commits. | |
| Each draft save creates a new commit. The ``current'' article is the ref's tip, but the full history is a linked list of commits. |
🤖 Prompt for AI Agents
In @docs/adr-tex-2/sections/04-solution.tex at line 43, Replace the incorrect
C-style escaped quotes \"current\" with proper LaTeX quotation marks; change it
to ``current'' (or use \enquote{current} if csquotes is enabled) in the sentence
"Each draft save creates a new commit. The \"current\" article..." so the
document uses correct TeX quotes.
| \paragraph{D-2: Why Trailers, Not JSON?} | ||
| \textbf{Alternative:} Store \texttt{\{\"title\": \"Hello\", ...\}} as the message. \\ | ||
| \textbf{Decision:} Use RFC 822 trailers. \\ | ||
| \textbf{Rationale:} Trailers are Git-native, human-readable, and diff-friendly. Backward parsing is efficient. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify "Backward parsing."
"Backward parsing is efficient" is insider jargon. Readers unfamiliar with Git's interpret-trailers implementation may not understand that trailers are parsed from the message's end, allowing O(1) access to the trailer block without scanning the entire body.
Consider a parenthetical: "Backward parsing (from message end) is efficient."
🤖 Prompt for AI Agents
In @docs/adr-tex-2/sections/04-solution.tex at line 57, Update the rationale
sentence under \textbf{Rationale}: clarify "Backward parsing" by adding a
parenthetical and brief explanation; change "Backward parsing is efficient." to
something like "Backward parsing (from the message end) is efficient, allowing
O(1) access to the trailer block without scanning the message body." Ensure this
replacement appears in the same rationale sentence mentioning Trailers.
Comment on lines
+59
to
+62
| \paragraph{D-3: Why Encrypt Assets, Not Repos?} | ||
| \textbf{Alternative:} Use \texttt{git-crypt} for the whole repo. \\ | ||
| \textbf{Decision:} Encrypt individual assets client-side. \\ | ||
| \textbf{Rationale:} Granular control; the gateway never sees plaintext. No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for encryption-related code in the codebase
echo "=== Searching for encryption, cipher, and crypto patterns ==="
rg -n 'encrypt|cipher|crypto' --type=js --type=ts -C 2
echo ""
echo "=== Searching for CmsService class ==="
rg -n 'class CmsService' --type=js --type=ts -A 20
echo ""
echo "=== Searching for asset handling and transmission ==="
rg -n 'asset|gateway|transmit|send|post' --type=js --type=ts -C 1 | head -50Repository: flyingrobots/git-cms
Length of output: 6097
🏁 Script executed:
#!/bin/bash
# List repository structure to understand the codebase layout
echo "=== Repository structure ==="
git ls-files | grep -E '\.(js|ts|tsx|jsx)$' | head -30
echo ""
echo "=== Looking for service files ==="
fd -e js -e ts -e tsx 'Service|service' 2>/dev/null | head -20Repository: flyingrobots/git-cms
Length of output: 1585
🏁 Script executed:
#!/bin/bash
# Find and examine ContentAddressableStore implementation
echo "=== Searching for ContentAddressableStore ==="
rg -n 'class ContentAddressableStore' --type=js --type=ts -A 30
echo ""
echo "=== Looking for storeFile implementation ==="
rg -n 'storeFile' --type=js --type=ts -B 2 -A 15Repository: flyingrobots/git-cms
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Check for encryption happening before git operations
echo "=== Searching for encryption implementation details ==="
rg -n 'encrypt|cipher|crypt' --type=js -B 3 -A 10
echo ""
echo "=== Check where ContentAddressableStore is imported/defined ==="
fd -e js -e ts | xargs grep -l 'ContentAddressableStore' | head -10Repository: flyingrobots/git-cms
Length of output: 3829
🏁 Script executed:
#!/bin/bash
# Check imports at top of CmsService.js
echo "=== Top of CmsService.js (imports) ==="
head -20 src/lib/CmsService.js
echo ""
echo "=== Search for ContentAddressableStore definition more broadly ==="
rg -n 'ContentAddressableStore' --type=js
echo ""
echo "=== Search for storeFile with class context ==="
rg -n 'storeFile\s*\(' --type=js -B 5 | head -50Repository: flyingrobots/git-cms
Length of output: 1413
🏁 Script executed:
#!/bin/bash
# Find all files in src/lib directory
echo "=== Files in src/lib ==="
ls -la src/lib/
echo ""
echo "=== Look for CAS or store-related files ==="
fd -e js | grep -i 'cas\|store\|address'Repository: flyingrobots/git-cms
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Examine the asset upload endpoint in full detail
echo "=== Asset upload endpoint (lines 115-140) ==="
sed -n '115,140p' src/server/index.js
echo ""
echo "=== Check if data is ever logged or exposed ==="
rg -n 'console.log|console.error|logger' src/server/index.js
echo ""
echo "=== Full context around asset upload ==="
sed -n '100,145p' src/server/index.jsRepository: flyingrobots/git-cms
Length of output: 3330
🏁 Script executed:
#!/bin/bash
# Verify the actual flow: does plaintext ever exist before encryption?
echo "=== Check uploadAsset signature and behavior ==="
sed -n '115,145p' src/lib/CmsService.jsRepository: flyingrobots/git-cms
Length of output: 1021
The "gateway never sees plaintext" claim is inaccurate and requires immediate correction.
The implementation directly contradicts this security invariant. In src/server/index.js (lines 122-124), the gateway:
- Receives base64-encoded data in the JSON request body
- Decodes it to plaintext with
Buffer.from(data, 'base64') - Writes plaintext to the server filesystem with
fs.writeFileSync(filePath, ...) - Only then passes the plaintext file to
cms.uploadAsset()for encryption
Plaintext exists on the server disk before encryption occurs. The actual architecture is: client sends base64 → server decodes and writes plaintext temporarily → server encrypts in storage layer → git persists encrypted. This is server-side encryption with a plaintext intermediary, not client-side encryption.
Either correct the decision rationale to reflect the actual threat model (encryption in the git storage layer, not client-side), or re-architect to encrypt before server-side persistence.
🤖 Prompt for AI Agents
In @docs/adr-tex-2/sections/04-solution.tex around lines 59 - 62, The ADR's
claim "the gateway never sees plaintext" is incorrect because the current
implementation in src/server/index.js uses Buffer.from(data, 'base64') to decode
incoming data and fs.writeFileSync to write plaintext to disk before calling
cms.uploadAsset(), meaning plaintext exists on the server; fix this by either
(A) updating the D-3 text in 04-solution.tex to state the correct threat model
(server-side decryption/intermediate plaintext storage and that encryption
occurs in the storage layer, not client-side) and remove the inaccurate "gateway
never sees plaintext" sentence, or (B) re-architect the flow so the gateway
never persists plaintext by performing encryption client-side or streaming the
decoded data directly into an encrypt-then-write pipeline (remove the
intermediate fs.writeFileSync usage and ensure cms.uploadAsset() receives
encrypted data), then update the ADR to reflect the implemented model; reference
Buffer.from, fs.writeFileSync, cms.uploadAsset, and the D-3 paragraph when
making the change.
| \centering | ||
| \resizebox{\textwidth}{!}{ | ||
| \begin{tikzpicture}[node distance=1.5cm, auto] | ||
| \node [block, dashed] (Empty) {Empty Tree\\4b825dc...}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ad-hoc style override — define a proper style instead.
You're jamming dashed onto block inline. If this "dashed block" pattern represents something semantically meaningful (it does — the empty tree is a special sentinel), define a named style in main.tex alongside block and blockshaded. Inline style mutations are a code smell even in LaTeX.
♻️ Define in main.tex
blockdashed/.style={base, rectangle, rounded corners=2pt, fill=white, dashed},Then use:
- \node [block, dashed] (Empty) {Empty Tree\\4b825dc...};
+ \node [blockdashed] (Empty) {Empty Tree\\4b825dc...};🤖 Prompt for AI Agents
In @docs/adr-tex-2/sections/08-crosscutting.tex at line 9, The inline style
mutation using "dashed" on the existing \node [block, dashed] (Empty) should be
replaced with a named style: add a new TikZ style (e.g., blockdashed) in
main.tex alongside block and blockshaded that composes base/rectangle/rounded
corners/fill/dashed, then update the node declaration to use \node [blockdashed]
(Empty) {Empty Tree\\4b825dc...}; so the dashed sentiment is reusable and not
applied ad-hoc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prep to hand this repo off to the Git Stunts org.