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)

🤖

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)

🤖

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)

🤖

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)

🤖

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)

🤖

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)

🤖

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)

🤖