Skip to content

ci(.github/workflows): list every changed doc page in docs-preview comment with viewed checkboxes#27166

Draft
nickvigilante wants to merge 1 commit into
vigilante/docs-476-normalize-docs-code-fence-languages-de-risk-shikifumadocsfrom
vigilante/docs-541-docs-preview-pr-comment-list-all-changed-pages-with-per-page
Draft

ci(.github/workflows): list every changed doc page in docs-preview comment with viewed checkboxes#27166
nickvigilante wants to merge 1 commit into
vigilante/docs-476-normalize-docs-code-fence-languages-de-risk-shikifumadocsfrom
vigilante/docs-541-docs-preview-pr-comment-list-all-changed-pages-with-per-page

Conversation

@nickvigilante

Copy link
Copy Markdown
Contributor

Stacked on #27161 for testing convenience (this PR's actual changes don't depend on that one's content; only .github/workflows/** is touched here). Re-target to main once #27161 merges, or independently before then if easier to review standalone.

Problem

The docs-preview comment bot links to exactly one changed page under docs/. See #27161 (comment), where the PR touched a few dozen pages but the comment only surfaced one preview link, with no way to track which of the other pages had actually been reviewed.

Changes

  • List a preview link for every added/modified Markdown file under docs/ in the PR, not just the first.
  • Filter that list to files that resolve to a route in docs/manifest.json (fetched via the contents API at the PR head sha). Anything else (docs/.style/** contributor tooling, or a page not wired into navigation) is dropped so the comment never links to a 404.
  • Render each page as a Markdown checklist item a reviewer can check off in the GitHub UI as they review.
  • Round-trip checked state across pushes: a page's checkbox stays checked as long as its blob sha hasn't changed since the comment was last updated, and resets to unchecked the moment new content lands on that page (a checked box means "I've reviewed the current revision," not some earlier one).
  • Extended test-docs-preview-mapper.sh with regression tests for the manifest-path normalization, checkbox-line parsing, and checked-state-carryover logic, following the existing pattern of replicating workflow shell logic for unit testing without running the full workflow.

Linear: DOCS-541

How this was tested

GitHub Actions can't easily be run locally, so I extracted the run: script from the YAML and exercised it against a fake gh CLI backed by JSON fixtures, covering:

  1. First run: several changed pages, one under docs/.style/, one not in the manifest, one image, one removed file. Only the two manifest-resolvable pages show up, both unchecked.
  2. Second run: a page with an unchanged sha stays checked; a page whose sha changed resets to unchecked even though it was previously checked; a brand-new page starts unchecked.
  3. No eligible Markdown files on a push: stale comment gets deleted.
  4. Markdown files present but none resolve to a manifest route: stale comment gets deleted.
  5. Replayed docs: normalize code-fence languages for Shiki compatibility #27161's actual diff (60 changed docs files) against the real docs/manifest.json: 58 of 59 candidate pages resolved to a route; the one exclusion (docs/admin/templates/extending-templates/external-auth.md) is genuinely absent from the manifest, confirming the filter does its job.

make pre-commit-light (shellcheck, actionlint, markdownlint, shfmt) and bash .github/workflows/test-docs-preview-mapper.sh both pass.


This PR description and the underlying changes were prepared with Coder Agents assistance.

…ed checkboxes

The docs-preview PR comment only ever linked to the first changed page
under docs/, so reviewers on multi-page PRs had no way to see the rest
of the changed pages or track which ones they'd actually reviewed.

- List every added/modified Markdown file under docs/ (not just the
  first), filtered to files that resolve to a route in
  docs/manifest.json. Anything else (docs/.style/** tooling, or a page
  not wired into navigation) is dropped so the comment never links to
  a 404.
- Render each page as a Markdown checklist item a reviewer can check
  off in the GitHub UI.
- Round-trip checked state across pushes: a checkbox stays checked as
  long as the page's blob sha hasn't changed since the comment was
  last updated, and resets to unchecked the moment new content lands
  on that page.
- Extend test-docs-preview-mapper.sh with regression tests for the
  manifest-path normalization, checkbox-line parsing, and checked-
  state carryover logic, following the existing pattern of replicating
  workflow shell logic for unit testing.

Manually exercised end to end against a fake gh CLI (four scenarios:
first run, carryover + reset on a second push, no eligible files, and
files present but none in the manifest) plus a replay of PR #27161's
real diff and manifest.json.

DOCS-541
@linear-code

linear-code Bot commented Jul 10, 2026

Copy link
Copy Markdown

DOCS-541

@nickvigilante

Copy link
Copy Markdown
Contributor Author

/coder-agents-review

@coder-agents-review

coder-agents-review Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Chat: Review posted | View chat
Requested: 2026-07-10 18:29 UTC by @nickvigilante
Spend: $15.15 / $100.00

Review history
  • R1 (2026-07-10): 10 reviewers, 2 Nit, 2 Note, 3 P3, 1 P4, COMMENT. Review

deep-review v0.9.0 | Round 1 | 3aa3e1b..e493fa4

Last posted: Round 1, 8 findings (3 P3, 1 P4, 2 Nit, 2 Note), COMMENT. Review

Finding inventory

Findings

# Sev Status Location Summary Round Reviewer Posted
CRF-1 P3 Dropped by orchestrator (merged into CRF-4) docs-preview.yaml:174 Unguarded gh api body fetch R1 Netero No
CRF-2 Nit Open test-docs-preview-mapper.sh:18 Test map_doc_path parameter name first_doc drifted from workflow's doc_path R1 Netero Yes
CRF-3 Note Open docs-preview.yaml:111 cleanup_stale_and_exit re-lists comments instead of reusing existing_id R1 Netero Yes
CRF-4 P3 Open docs-preview.yaml:174 Body fetch unguarded and placed too early; widens race window for checkbox overwrites and fails the workflow on transient API errors R1 Netero P3, Mafuuu P3, Hisoka P3, Meruem P3 Yes
CRF-5 P3 Open docs-preview.yaml:233 State-marker round-trip (encode, embed, grep-extract, decode) and jq carryover logic have no automated regression test R1 Hisoka P3, Bisky P3, Meruem Note Yes
CRF-6 P3 Open docs-preview.yaml:73 Comment verbosity pattern: 19/36 new/modified comments are 30-60% longer than necessary R1 Gon P2 (downgraded by orchestrator) Yes
CRF-7 P4 Open docs-preview.yaml:275 Checklist loop spawns 2 jq processes per eligible file; single-jq alternative eliminates per-row overhead R1 Hisoka P4 Yes
CRF-8 Nit Open (body) Commit/PR subject is 83 chars; project limit is 72 R1 Leorio Nit Yes
CRF-9 Note Open test-docs-preview-mapper.sh:1 Test script not wired into any CI workflow R1 Hisoka Note Yes
CRF-10 Note Open docs-preview.yaml:203 `[.. objects select(has("path"))]collects everypath` value in manifest regardless of context R1

Contested and acknowledged

(none)

Round log

Round 1

Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Komugi, Chopper, Meruem. Netero first pass. 3 P3, 1 P4, 2 Nit, 3 Note. 1 dropped (merged). Reviewed against 3aa3e1b..e493fa4.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer Focus
Bisky tests
Chopper ops/errors
Churn-guard change verification
Ging language modernization
Gon naming
Hisoka edge cases
Killua perf
Kite change integrity
Knov contracts
Knuckle SQL
Komugi flake/determinism
Kurapika security
Law decomposition
Leorio docs
Luffy product
Mafu-san process
Mafuuu contracts
Melody dispatch/pairing
Meruem structural
Nami frontend
Netero mechanical checks
Pariston premise testing
Pen-botter product gaps
Razor verification
Robin duplication
Ryosuke Go arch
Takumi concurrency
Zoro shape

🤖 Managed by Coder Agents.

@coder-agents-review coder-agents-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-executed CI enhancement. The problem is correctly understood (single-page link is useless on multi-page docs PRs), the solution is proportional (checklist with state round-trip), and the scope is disciplined (only the two stated files touched). Error handling is thoughtfully differentiated: lenient on cleanup (cosmetic miss), strict on upsert (duplicate prevention). The contents: read repetition at job level is correctly explained. All extractable logic is tested with branch coverage.

3 P3, 1 P4, 2 Nit, 3 Note. The P3s are about narrowing a race window, adding a missing round-trip test, and trimming comment verbosity. Nothing blocking.

Commit/PR subject is 83 characters; project convention limits to 72. Shorter form that preserves the intent: ci(.github/workflows): list all changed doc pages with review checkboxes (71 chars). See CRF-8 inline.

"Delete the // false from the jq. Tests still green. Remove the != null guard. Tests still green. Replace the entire jq carryover block with false as $checked. Tests still green. The test proves the shell function works; it does not exercise the jq it claims to replicate." -- Bisky


.github/workflows/test-docs-preview-mapper.sh:18

Nit [CRF-2] The workflow renamed this parameter from first_doc to doc_path (line 134 of docs-preview.yaml) since the function is now called per-file, not for a single "first" doc. The test replica still uses first_doc. Line 11 says "Keep these in sync with the corresponding shell in docs-preview.yaml." (Netero)

🤖

.github/workflows/test-docs-preview-mapper.sh:1

Note [CRF-9] This test script is not referenced by any CI workflow, Makefile target, or scripts/ entry. It runs only when a developer remembers to invoke it by hand. The header says "Keep these in sync" but sync-by-convention with no automated gate is exactly the coupling that drifts when the next editor isn't the original author. This follows the pre-existing pattern (the test file existed before this PR and wasn't in CI), so it's not a regression, but the more logic the test covers the more valuable CI gating becomes. (Hisoka)

🤖

🤖 This review was automatically generated with Coder Agents.

existing_id=$(list_docs_preview_comments | head -n 1) || true
existing_body=""
if [ -n "$existing_id" ]; then
existing_body=$(gh api "repos/${REPO}/issues/comments/${existing_id}" --jq '.body')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 [CRF-4] Two issues with the early body fetch.

1. Error propagation. This gh api call runs under set -euo pipefail with no || true guard. A transient 404 or rate-limit error kills the script before it reaches either the lenient cleanup path or the PATCH-fallback upsert path.

2. Race window. The body is fetched here but not consumed until line 232 (state recovery). The interval between fetch and eventual PATCH (~5-30s depending on PR size, covering the changed-files API call, manifest fetch, and intersection) is a window where a reviewer's checkbox toggle gets silently overwritten. The comment at line 168-170 says the early fetch lets "both the cleanup path and the upsert path reuse it," but cleanup_stale_and_exit never reads existing_body; it only needs existing_id.

Fix: move the body fetch to just before line 232 (after eligible_tsv is confirmed non-empty) and add || true. This cuts the race window roughly in half and prevents transient API errors from killing the workflow. (Netero P3, Mafuuu P3, Hisoka P3, Meruem P3)

🤖

old_state_json="{}"
old_checked_json="{}"
if [ -n "$existing_body" ]; then
old_state_b64=$(printf '%s\n' "$existing_body" | grep -oE "${STATE_PREFIX}[A-Za-z0-9+/=]+" | sed "s/^${STATE_PREFIX}//") || true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 [CRF-5] The state-marker round-trip (base64 encode, embed in HTML comment, grep-extract, base64 decode) and the jq carryover decision logic have no automated regression test.

The test file covers parse_checkbox_line, decide_checked, map_doc_path, and normalize_manifest_path as standalone shell functions. But the jq expressions on lines 220-221 (TSV-to-JSON), 239-242 (checkbox-state parsing), and 249-261 (carryover with old_state/old_checked merge) are different code in a different language than the shell reimplementations that test them. decide_checked in the test takes pre-resolved scalars; the production jq does map lookups with null coalescing (// false, // null). You can break the jq null coalescing and the tests still pass.

The failure mode is silent: every push resets all checkboxes to unchecked, which looks identical to "all files changed." A reviewer would never suspect the mechanism is broken.

Consider a round_trip_state test that pipes fixture JSON through the actual jq expressions: encode a sample {"docs/a.md": "sha1"}, wrap in the HTML comment format, extract with the same grep+sed, decode, and compare. (Hisoka P3, Bisky P3, Meruem)

🤖

run: |
set -euo pipefail

# Marker embedded in the comment body so we can find this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 [CRF-6] This 12-line comment block delivers ~3 lines of content. The same pattern recurs in 19 of 36 new/modified comments across both files, adding ~40 lines of padding. The comments are accurate, not misleading, but consistently pad past the "substantive and concise" bar.

Two examples with trimmed alternatives:

Here (lines 73-84):

# DOCS_PREVIEW_MARKER identifies this workflow's comments. STATE_PREFIX
# carries per-page blob shas for change detection. Keep in sync with
# test-docs-preview-mapper.sh.

Lines 98-109 (cleanup_stale_and_exit):

# Deletes the existing preview comment and exits 0. Errors are
# cosmetic; the next push retries. Upsert uses strict propagation
# to prevent duplicates.

The content in each case is correct. The fix is compression, not rewriting. (Gon)

🤖

checklist=""
while IFS= read -r row; do
[ -z "$row" ] && continue
filename=$(printf '%s' "$row" | jq -r '.filename')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4 [CRF-7] The checklist loop spawns 2 jq processes per eligible file (.filename and .checked). For a 60-page docs PR, that's 120 process spawns at ~50ms each, roughly 6 seconds of CI overhead.

Replacing lines 275-276 with a single jq -r '[.filename, (.checked | tostring)] | @tsv' in the process substitution on line 294, then parsing with IFS=$'\t' read -r filename checked_str, eliminates the per-row overhead. (Hisoka)

🤖

# against the PR-files filenames above.
manifest_content=$(gh api "repos/${REPO}/contents/docs/manifest.json?ref=${HEAD_SHA}" --jq '.content' | base64 -d)
allowed_paths=$(printf '%s' "$manifest_content" \
| jq -r '[.. | objects | select(has("path")) | .path] | .[]' \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note [CRF-10] [.. | objects | select(has("path"))] collects every "path" value in the manifest tree regardless of depth or context. Today all extracted values are page paths, and non-page objects with a "path" key would almost certainly fail to match any docs/*.md filename in the intersection step, so the practical risk is nil. The query's intent (page paths only) is worth noting because it isn't expressed in its selector. (Mafuuu)

🤖

# uses strict propagation by contrast, because silent failure
# there would create duplicate comments.
cleanup_stale_and_exit() {
stale_comment_ids=$(list_docs_preview_comments) || {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note [CRF-3] cleanup_stale_and_exit calls list_docs_preview_comments() again instead of reusing the already-fetched existing_id from line 171. Costs one extra API call per cleanup invocation. (Netero)

🤖

# This workflow posts a docs preview link as a PR comment whenever a
# pull request that touches docs/ is opened or updated. The preview
# is served by coder.com's branch-preview feature at /docs/@<branch>.
# This workflow posts a docs preview comment listing every navigable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [CRF-8] Commit/PR subject is 83 characters; project convention limits to 72. Shorter form: ci(.github/workflows): list all changed doc pages with review checkboxes (71 chars). (Leorio)

🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant