[NIT] The review job dropped its dependency on test (changed from needs: test to needs: [review-gate]). This means the heavy review can now run even if the Go build/tests are failing. The previous design required passing tests before reviews ran. This may be intentional but is worth confirming.
[MINOR] Shell variables from GitHub expressions are interpolated directly without quoting: REPO=${{ github.repository }}. If the repository name contained spaces or special characters this would break. Prefer REPO='${{ github.repository }}' or export with quotes for robustness.
[MINOR] The allow_review output is set but never consumed — review.if only checks reason, not allow_review. The output could be removed, or the review.if condition could be simplified to just needs.review-gate.outputs.reason == 'self-review' (which it already is). The unused output is harmless but misleading.
[MINOR] The TTL is calculated using PR_JSON.updated_at, which changes on every push/comment. A PR with a new comment 5 minutes ago will never pass the TTL gate regardless of how old the PR is. If the intent is 'PR was opened and nobody reviewed it for 45 minutes', created_at would be more appropriate. If the intent is 'last activity was 45 minutes ago', the current behavior is correct but differs from the PR description which says 'TTL keeps the gate green'.
[MINOR] The test job now installs jq but never uses it. jq is only needed in the review-gate and review jobs. This adds ~5s to every test run for no reason.
[MINOR] date -u -d "$UPDATED_AT" +%s uses GNU date syntax. This is fine on ubuntu-24.04, but the -d flag for parsing a date string is a GNU extension. Worth a comment so future maintainers know this is intentionally Linux-only.
Sonnet Review
[NIT] import sys, yaml, re — sys and re are imported but never used in the script. Harmless, but slightly misleading.
[MINOR] The jq regex (?im)^###\s+Doc consistency\b is passed through multiple layers of shell quoting and JSON encoding before reaching jq. The \b word-boundary assertion and the multiline mode should work in jq's ONIG engine, but this is non-obvious and fragile. A simpler test("(?m)^### Doc consistency") without the \b would be more readable and equally correct for this use case.
Sonnet Review
[NIT] import sys, yaml, re — sys and re are imported but re is unused (the regex is done via jq, not Python) and sys is unused too. This is harmless but slightly messy.
[NIT] The REPO=${{ github.repository }} and API=... lines use unquoted expression substitution without ${} shell quoting. If the repository name ever contains spaces (unlikely but possible), this would break. Prefer REPO="${{ github.repository }}" style consistently.
[MINOR] The review job if: condition uses string comparison needs.review-gate.outputs.reason == 'self-review' combined with && against event_name checks. In Gitea Actions, if review-gate is skipped (e.g. for push events), needs.review-gate.outputs.reason will be empty and the comparison will be false — meaning review is silently skipped for push-to-main without any indication. This is probably intentional but worth a comment.
[MINOR] The AGE_MIN variable is computed and ALLOW=true/REASON=ttl is set when TTL is exceeded, but allow_review=true with reason=ttl does NOT trigger the review job (the review job gates on reason == 'self-review'). This is correct per the stated intent, but the allow_review output is never actually consumed by any job — only reason is checked. The allow_review output is dead output and may cause future confusion. Consider removing it or documenting its lack of use.
[NIT] The comment on the Lstat call says 'ModeSymlink can never be set' after EvalSymlinks, but the old defense-in-depth fi.Mode()&os.ModeSymlink != 0 check was removed. The comment is accurate and the removal is correct (the check was genuinely unreachable), but removing it does reduce defense-in-depth. Given the comment's explicit acknowledgment this is a deliberate and documented choice — no issue.
[NIT] The pre-open os.Lstat(resolvedDocmap) → os.Open(resolvedDocmap) → f.Stat() → os.SameFile pattern for TOCTOU detection is a genuine improvement, but os.SameFile compares inode/device, not content. On Linux, an attacker who can atomically replace the file with another hardlink pointing to a different inode between Lstat and Open could still bypass this (though this is an extremely narrow and unlikely window). This is a known limitation of the pattern, not a bug introduced here, and the comment already calls it 'defense-in-depth'. No action required — just noting it for awareness.
[NIT] The new TestValidateDocmapPath_InRepoSymlinkAllowed test checks if resolved == symlinkPath to verify that the returned path differs from the symlink. On some systems (e.g., macOS where /tmp is a symlink to /private/tmp), t.TempDir() already returns an EvalSymlinks-resolved path, but symlinkPath is constructed via filepath.Join(dir, ...) which may not yet be resolved. The comparison is reliable here because validateDocmapPath calls EvalSymlinks internally, so resolved will always differ from symlinkPath (a symlink entry). The assertion is correct but a comment clarifying why they must differ would help future readers.