[rubambiza]

Summary

Two doc-drift review follow-ups from the #124 / rossoctl/repoman#12 review threads, folded into one PR under that shared purpose.

#130 — precheck worklist defaults to a persistent path. doc-drift-precheck.sh defaulted its worklist into the per-run temp dir that setup_workspace's EXIT trap deletes, so a bare run (no --worklist) printed worklist: <path> for a file already gone by the time the caller read it — fail-open in the worst direction. It now defaults to ${REPORTS_DIR:-$HOME/reports/doc-drift}/worklist.json (mirroring the scanner), mkdir -ps it, and states the contract in usage. --worklist still overrides.

#128 — behavioural test for the scanner. doc-drift-scanner.sh was the only doc-drift component with no behavioural test (just bash -n + shellcheck). Adds tests/test-doc-drift-scanner.sh driving the real scanner with gh stubbed:

• new findings → gh issue create with [doc-drift] owner/name
• checker failure → prior findings carried into latest.json, issue not closed, exit 0, repos_failed==1
• findings cleared → existing issue closed
• --dry-run → no issue create/edit/close, latest.json not written
• --issue-limit 1 → stops after one repo
• latest.json shape (repos_scanned/corrections/additions/delta) + one history.json row

Registered in both CI lists (shellcheck group + run loop) — the lists are explicit, so an unlisted test never runs.

Testing

• tests/test-doc-drift-precheck.sh (with the new bare-run regression case) and tests/test-doc-drift-scanner.sh both pass; shellcheck --severity=warning clean on all touched files; workflow YAML valid.
• Scope: doc-drift-precheck.sh: bare run prints a worklist path its own EXIT trap deletes #130 fixes the worklist default; doc-drift-scanner.sh has no behavioural test: carry-forward, rollup open/close and ledger are untested #128 is test-only (no production change to the scanner).

Fixes #130
Fixes #128

Assisted-By: Claude Code

[clawgenti]

Clean follow-up pair: the #130 fix correctly defaults the precheck worklist to a persistent path (verified against setup_workspace's EXIT trap, with a real regression test), and the new scanner behavioural test drives the real scanner with stubbed gh across create / carry-forward / close / dry-run / issue-limit / report-shape paths — both tests pass locally and CI is green. Two nits below.

• nit scripts/doc-drift-precheck.sh:27 — --help renders literal backslashes
• nit tests/test-doc-drift-scanner.sh:303 — grep -c ... || echo 0 double-prints on the zero-match path

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

nit: The usage heredoc is quoted (<<'EOF'), so ${...} never expands — the backslashes here are unnecessary and render literally in --help output: \${REPORTS_DIR:-\$HOME/reports/doc-drift}/worklist.json. Dropping the escapes prints the intended plain form.

[rubambiza]

Fixed in cf08866 — dropped the backslash escapes; the single-quoted heredoc now renders the plain ${REPORTS_DIR:-$HOME/reports/doc-drift}/worklist.json in --help.

[clawgenti]

nit: grep -c already prints 0 (with exit status 1) when nothing matches, so the || echo 0 fallback appends a second 0 — on the failure path c5creates becomes two lines and the [ "$c5creates" -le 1 ] check below errors with "integer expression expected" instead of reaching the intended fail_msg. Dropping || echo 0 (or using || true) keeps that path clean.

[rubambiza]

Fixed in cf08866 — switched || echo 0 to || true, so the no-match path yields a single 0 and the -le integer check stays well-formed.

[clawgenti]

Nit-fix commit cf08866 correctly addresses both findings from the earlier review — the --help heredoc now renders ${REPORTS_DIR:-$HOME/...} literally, and the case-5 count uses || true so the zero-match path stays a clean integer — with both test suites passing (verified locally against the PR head) and CI green on both platforms.

All checks pass. Ready for human review.

---

Reviewed by clawgenti using the github-pr-review skill

[jimcadden]

Reviewed cf08866. No blocking bugs or security findings found. The persistent worklist default fixes the deleted-temp-file problem, and its regression passes. The new scanner tests cover issue creation, failure carry-forward, closure, dry-run, issue limiting, and report structure; both explicit CI lists include the test. Two nonblocking test improvements are included inline.

Author: rubambiza (MEMBER, maintainer).
Areas reviewed: shell, tests, CI/YAML.
Agent/IDE config (.claude/.vscode): none.
Commits: 3, all signed off.
CI status: 4/4 checks passing, including Ubuntu and macOS.
Verification: precheck, scanner, and checker test suites passed locally; shell syntax checks and git diff --check passed. Local ShellCheck was unavailable; CI runs it.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[jimcadden]

suggestion: Assert exactly one issue create rather than <= 1. With two drifting fixtures, the current assertion also passes if --issue-limit 1 incorrectly suppresses all issue creation. Assert repos_scanned == 2 as well to guard against limiting the scan instead of issue writes.

[jimcadden]

suggestion: Extend the dry-run guard to reject issue comment and label create, and seed/assert unchanged history.json. The current case protects issue create/edit/close and latest.json, but could miss other remote writes or history changes.