ci(.github/workflows): list every changed doc page in docs-preview comment with viewed checkboxes#27166
Conversation
…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
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 8 findings (3 P3, 1 P4, 2 Nit, 2 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel: 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-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
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
// falsefrom the jq. Tests still green. Remove the!= nullguard. Tests still green. Replace the entire jq carryover block withfalse 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') |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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] | .[]' \ |
There was a problem hiding this comment.
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) || { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
🤖
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 tomainonce #27161 merges, or independently before then if easier to review standalone.Problem
The
docs-previewcomment bot links to exactly one changed page underdocs/. 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
docs/in the PR, not just the first.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.test-docs-preview-mapper.shwith 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 fakeghCLI backed by JSON fixtures, covering:docs/.style/, one not in the manifest, one image, one removed file. Only the two manifest-resolvable pages show up, both unchecked.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) andbash .github/workflows/test-docs-preview-mapper.shboth pass.This PR description and the underlying changes were prepared with Coder Agents assistance.