Conversation
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Review: approve with nits
Verified the gate empirically in a scratch worktree at the PR head (71264e87):
- The gate has teeth. Run against
main's root.mdtree it exits 1 (AGENTS.md,CLAUDE.md,FAQ_POC.md,ProjectGovernance.mdwarned); run against this PR's tree it exits 0 (All matched files use Prettier code style!). - Correct binary.
docs/node_modules/.bin/prettieris the lockfile-pinned 3.8.1 (docs/package-lock.json), notnpx— which in this repo resolves a different version and produces false format failures. The step also sits afternpm ciin the same job, so the binary exists, andworking-directory: .resolving to the workspace root matches the in-file precedent of the "Retrieve Node.js version" step. - Glob coverage is complete. Copying all 7 regular root
.mdfiles plus a realCLAUDE.md -> AGENTS.mdsymlink into a scratch dir and appending a violation to each regular file warns on exactly 7 files. SoREADME.md,CONTRIBUTING.md,CHANGELOG.mdandTESTING.mdare covered by the glob and were already compliant; nothing that should be covered is missed.CLAUDE.mdis correctly skipped (prettier does not follow the symlink), soAGENTS.mdisn't checked twice — and the dedicated symlink-guard job runs first anyway. - No ignore-file interference. No root
.prettierignore;docs/.prettierignoreisn't on the cwd path for this invocation;.gitignorehas no.mdentries. - Content changes are formatting-only.
proseWrap: "preserve"means no reflow:AGENTS.mdis table realignment plus the new checklist entry,ProjectGovernance.mdis*→-bullet markers,FAQ_POC.mdis trailing-whitespace and double-space collapse. No semantic drift.
Nothing blocking. Four non-blocking notes inline, plus two things outside the diff:
*.mdis non-recursive, which matches the PR title, but it leaves 19 tracked Markdown files ungated (.github/pull_request_template.md, four crateREADME.mds, 14 files under.claude/). Fine to defer — worth an issue if the intent is eventually "all Markdown is formatted".- Is
format-docsa required status check? I couldn't read branch protection. If it isn't required, this gate is advisory only.
Both inline suggestions were applied in isolation and as a batch against a scratch worktree and re-verified green (exit 0, no drift), then reverted.
Document the --write fix in the CI checklist, run the root Markdown check after the VitePress dead-link build so both failures surface in one CI round, and fix a couple of long-standing typos in FAQ_POC.md touched by the same line. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
….com/IABTechLab/trusted-server into chore/check-root-markdown-formatting
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed 1851e98101e68c17d900506c2ad0ac145dd6dbb7 against 6cae7f5da8911c746cf873581885f90c3820dd96. No actionable issues found.
The new root-Markdown check passed in the exact PR workflow. A separate negative test with the lockfile-pinned Prettier 3.8.1 rejected deliberately malformed copies of all seven regular root .md files, while CLAUDE.md remained a symlink to AGENTS.md. All reported CI checks pass, and the active main ruleset requires format-docs.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds a format-docs step that runs Prettier over root-level Markdown, closing the gap where only docs/ was covered, and normalizes existing drift in three root files. The gate itself works — I ran the exact command against the PR head, injected drift, confirmed it fails, ran the documented --write fix, and confirmed it passes again. All 20 CI checks pass.
One blocking finding: the step is ordered after the VitePress build, which defeats the stated purpose of the commit that introduced that ordering.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them. The remaining findings are body-level because they concern the PR description or span the repository rather than a single line.
Blocking
🔧 wrench
- Root Markdown check is skipped whenever the VitePress build fails — see inline at
.github/workflows/format.yml:155
Non-blocking
🤔 thinking
- Documented local command exits 127 without a prior
npm ci— see inline atAGENTS.md:348
Cross-cutting / body-level findings
-
⛏
FAQ_POC.mdis described as "Prettier drift only" but carries manual content edits — I reformatted the merge-base copy with this PR's own Prettier config and diffed it against the PR head.ProjectGovernance.mdis byte-identical to pure Prettier output, andAGENTS.mddiffers only by the intentional CI-gate item 8.FAQ_POC.mdline 5 has two edits Prettier would never make:punisher→publisher, andcurrent form.Its a proof→current form. It is a proof. Both are correct fixes and commit5c7d4f390mentions them, but the PR description's table still reads "Reformatted (Prettier drift only)" for this file, which will lead a reviewer to skip the one line that actually changed meaning. Worth a description edit rather than a code change. -
🌱 The gate covers 8 of 27 non-
docs/Markdown files, and 4 uncovered ones are already drifted —"*.md"is correctly quoted, so Prettier (not the shell) expands it and the scope is root depth-1 only. That is a reasonable scope for closing #1093. Running the same config over the rest shows these would fail if included today:crates/trusted-server-core/README.md,crates/trusted-server-integration-tests/README.md,.claude/agents/pr-reviewer.md,.claude/skills/deploying-trusted-server-to-fastly/SKILL.md. Flagging only so the remaining gap stays a deliberate follow-up rather than an assumed win.
Verified as safe
Three things worth recording, since they are the non-obvious risks in a change that globs *.md at the repository root:
- The
CLAUDE.mdsymlink is not at risk.CLAUDE.mdis a120000symlink toAGENTS.mdand is matched by the glob. Prettier deduplicates it: with drift injected,--list-differentreported onlyAGENTS.md, and after--writethe symlink was stillCLAUDE.md -> AGENTS.md. Thecheck-claude-md-symlinkguard job cannot be tripped by this gate. - No
.prettierignoregap.docs/.prettierignorelists onlynode_modulesand.vitepress/*; at root depth-1 there is nothing it would need to exclude. - No double-checking of
docs/. The existingnpm run formatruns withcwd=docs, so its scope and the new root glob do not overlap.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (actions): PASS
- CLAUDE.md symlink guard: PASS
- cargo test (cross-adapter parity): PASS
- Analyze (rust): PASS
- cargo test (axum native): PASS
- Analyze (javascript-typescript): PASS
- format-typescript: PASS (required)
- cargo fmt: PASS (required)
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test: PASS (required)
- vitest: PASS
- prepare integration artifacts: PASS
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Summary
format-docsCI job to check root-level Markdown (AGENTS.md,README.md,FAQ_POC.md, etc.) with Prettier, closing the gap where onlydocs/was coveredAGENTS.md,FAQ_POC.md,ProjectGovernance.md) — whitespace/list-marker only, no content changesChanges
.github/workflows/format.ymlformat-docsjob that runsdocs/node_modules/.bin/prettier --config docs/.prettierrc --check "*.md"from the repo root, reusing the already-installed binary and the existingdocs/.prettierrcconfigAGENTS.mdFAQ_POC.mdProjectGovernance.mdCloses #1093
Test plan
cd docs && npm run formatdocs/node_modules/.bin/prettier --config docs/.prettierrc --check "*.md"(run from repo root)format-docsandcheck-claude-md-symlinkGitHub Actions jobs locally viaact— both pass, including the new stepCLAUDE.mdstill resolves as a symlink toAGENTS.md(git ls-treemode120000) after formattingAGENTS.mdcargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)