ci: automate and harden doc-gardening (model pin + per-PR advisory) - #913
Conversation
In this commit, we make the unsupervised nightly doc sweep trustworthy. It was the one Claude workflow with no --model, so it silently rode the action's default (Sonnet), and would drift again the moment the action bumps that default. The @claude and code-review workflows are already pinned to Opus, so the writer running unattended was, backwards, the least capable and least pinned of the three. We pin it to opus-4-8. We also fold in what our manual sweep learned the hard way. The prompt now mirrors every CLAUDE.md to its AGENTS.md as an explicit gate (the root pair is the one most often missed, and doc-check fails on a byte diff), scrubs leaked tool/formatting artifacts (stray </content> and </invoke> tags) before committing, and forbids `go build`/`go test` so a compiled `accounting` binary never gets swept into `git add -A`.
In this commit, we add a per-PR companion to the nightly sweep. The nightly is batch: drift lands up to 24h late, and each sweep opens its own PR to babysit. Here we catch drift in lockstep with the change that caused it. When a PR touches Go code, we run the doc-gardening skill scoped to just the packages whose .go files changed (and only those that already have a CLAUDE.md, so we never create new docs here, that stays the nightly's job). If a scoped doc has gone stale, we post the proposed CLAUDE.md + AGENTS.md fix back as a single sticky suggested-diff comment, replacing any prior one via --edit-last so the thread never piles up. The workflow never commits, pushes, or opens a PR. It's advisory by design: the comment is the whole signal, and the check exits 0 so it can't block merge. There's a marked one-line toggle to flip it to a non-required soft-fail later if the comment alone proves too easy to ignore. We skip fork PRs (no secrets) and the nightly's own automation PRs.
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f1100e3c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git diff --name-only "$BASE_SHA".."$HEAD_SHA" -- '*.go' \ | ||
| | xargs -r -n1 dirname | sort -u |
There was a problem hiding this comment.
For PRs whose branch is behind main, this two-dot range compares the current base tip directly to the head, so packages changed only on main show up in SCOPE as deletions/changes from the PR. GitHub documents PR diffs as three-dot/merge-base comparisons focused on what the PR introduces, and step 2 repeats the same two-dot range for auditing, so the advisory can spend an Opus run and post unrelated doc fixes until the author rebases. Use $BASE_SHA...$HEAD_SHA or an explicit git merge-base for both scans.
Useful? React with 👍 / 👎.
| 4. **Compute the proposal.** Run `git diff -- '**/CLAUDE.md' | ||
| '**/AGENTS.md' '**/*.md'`. If the diff is empty, print |
There was a problem hiding this comment.
When a PR changes a root-level Go file such as lnd_boarding_wallet.go, the scope command above yields . and the advisor may update the root CLAUDE.md/AGENTS.md, but these pathspecs do not match root-level Markdown files (git ls-files -- '**/CLAUDE.md' omits CLAUDE.md). The resulting diff can be empty even after root docs were changed, so the workflow prints the no-op message and never posts the needed advisory.
Useful? React with 👍 / 👎.
In this commit, we decouple the nightly's failure canary from the bot token it exists to watch. Both notify steps authenticated with BOT_GITHUB_TOKEN, so when that PAT hit its 90-day expiry the checkout step failed with a 401 and the canary failed with the very same 401, leaving two mornings of silent red runs and no tracking issue. The notify steps now use the workflow's own GITHUB_TOKEN, which GitHub mints fresh per run and which cannot expire out from under us. The workflow grants issues: write to make that possible; contents stays read-only. The bot token remains the identity for everything the sweep actually publishes (branch push, PR open) -- only the alarm wiring moves off it.
|
@Roasbeef, remember to re-request review from reviewers when ready |
In this commit, we move both doc-gardening workflows -- the nightly sweep and the per-PR advisory -- from claude-opus-4-8 to claude-opus-5, now that Opus 5 is the strongest model available. The nightly writes docs unattended and the advisory reasons about code-to-doc drift, so both are exactly the kind of judgment-heavy, low-oversight job that wants the most capable model. The pin stays explicit rather than riding the action default, which is the property the original pin commit was protecting.
In this commit, we close the widest prompt-injection escape in the
doc-gardening tool allowlist. awk, sed, and find were granted to Claude
but used by neither prompt, and each is a subprocess spawner that the
per-command allowlist cannot contain: awk 'BEGIN{system(...)}', GNU
sed's e command, and find -exec all launch arbitrary children, which
makes the curl/wget/gh-api entries on the disallow list reachable
anyway and the bot token in the step env exfiltrable. An injected
instruction riding in on a PR diff, go doc output, or the skill file is
the threat we care about here, not a hostile operator.
Removing the three we never call shrinks that surface to zero cost. The
tools the prompts actually use (cp for the CLAUDE.md->AGENTS.md mirror,
sort/xargs for the changed-package pipeline) stay for now; tightening
those needs a matching prompt rewrite and is tracked as follow-up.
86a18e8 to
72504cf
Compare
Second-pass hardening after an adversarial review of the doc-gardening automation. None of these change the sweep's behavior on a normal run; each shrinks what a prompt injection arriving via a PR diff, go doc output, or the skill file could do with the tools the model is granted. - Neutralize git hooks before the model runs, via a pre-step that sets core.hooksPath to /dev/null. The model holds native Write plus Bash(git commit:*), so without this an injection could drop a .git/hooks/pre-commit and have the sweep's own commit execute it with the bot token in env. git config is not in the allowlist, and git -c core.hooksPath=... commit does not match the Bash(git commit:*) prefix, so the model cannot re-enable hooks from inside the action. - Scope the stage step to git add -- '*.md' rather than git add -A, so a stray binary or any secret-bearing file dropped into the worktree can never be committed and pushed onto the PR branch. - Pin the three actions (claude-code-action, checkout, setup-go) to commit SHAs so a moved tag or upstream compromise cannot inject code into a step that holds the bot and OAuth tokens. - Correct the show_full_output rationale: these repos are private, not public, and the real residual is the base64 credential checkout persists into .git/config. Direct-push-to-default needs no change here: the branch ruleset already blocks it (the bot has write, not bypass) and every auto-PR requires one human approval to merge. Token-into-comment exfil stays bounded by keeping BOT_GITHUB_TOKEN a fine-grained, repo-scoped PAT.
|
@claude review this |
|
Claude finished @Roasbeef's task in 3m 10s —— View job Doc-gardening automation reviewI read both workflow files end-to-end and empirically checked the two Codex findings against this repo's actual git pathspec behavior. Both are real. A few additional items below.
Confirmed bugs1. The proposal is computed with So when a PR touches a root-level 2.
Additional findings3. The prompt writes a hidden marker "so the comment is idempotent," but 4. PR description says
5. Both prompts describe scrubbing "the changed docs" but the command is 6.
7. Lower-confidence: Same What's goodThe security posture is genuinely careful and well-documented:
|
Three correctness bugs surfaced by the Codex and Claude review passes,
all verified against this repo before fixing.
The advisory computed its scope with a two-dot range,
"$BASE_SHA".."$HEAD_SHA". base.sha is the base branch tip at event time,
not the branch point, so a PR that has fallen behind sees every file
changed on the base branch since it forked. Measured on this very PR:
two-dot reports 1147 changed files where the PR changed 2. That burns an
Opus run and posts doc fixes for packages the PR never touched. Both
scans now use the three-dot merge-base form.
The proposal diff used '**/CLAUDE.md' '**/AGENTS.md' '**/*.md'. Git's *
already spans /, so a leading **/ requires at least one slash and never
matches a repo-root file:
git ls-files -- '**/CLAUDE.md' -> root CLAUDE.md absent
git ls-files -- '*.md' -> root CLAUDE.md present
A root-level .go change puts . in SCOPE, the advisor edits the root
CLAUDE.md/AGENTS.md pair, and then the empty diff makes the run print
NO-OP and drop the advisory -- silently failing on exactly the pair the
nightly calls the one most often missed. A single '*.md' matches every
depth including root, and subsumes the other two patterns.
The nightly's artifact scrub told the model to run grep, but grep was
never added to the nightly allowlist when that instruction landed (the
advisory workflow has it). Headless, the scrub is denied and leaked tags
reach the commit. Add Bash(grep:*) there.
Two robustness items while here: scope the scrub to --include='*.md' so
a legitimate <parameter in Go source is never treated as an artifact to
delete, and forbid reading .git/config or env into the advisory comment.
checkout persists the bot token into .git/config and a comment is not
log-masked, so the comment body is the one egress path that could carry
a secret off the runner.
2 participants
In this PR, we make the doc-gardening automation more autonomous, and
more trustworthy while we're at it. Two changes, one per commit.
First, we harden the nightly sweep. It was the only Claude workflow with
no
--model, so it silently rode the action's default (Sonnet) andwould drift again the moment the action bumps that default. The
@claudeand code-review workflows are already pinned to Opus, so the writer
running unattended was, backwards, the least capable and least pinned of
the three. We pin it to
opus-4-8, and fold in what the recent manualsweep learned the hard way: the prompt now mirrors every
CLAUDE.mdtoits
AGENTS.mdas an explicit gate (the root pair is the one most oftenmissed, and
doc-checkfails on a byte diff), scrubs leaked toolartifacts (stray
</content>/</invoke>tags) before committing, andforbids
go build/go testso a compiled binary never gets swept intogit add -A.Second, we add a per-PR advisory that catches drift in lockstep with the
change that caused it, instead of waiting up to 24h for the nightly.
When a PR touches Go code, we run the skill scoped to just the packages
whose
.gofiles changed (only ones that already have aCLAUDE.md, sowe never create new docs here, that stays the nightly's job) and post the
proposed fix back as a single sticky suggested-diff comment via
--edit-last. It never commits, pushes, or opens a PR.The advisory is advisory by design: the comment is the whole signal, and
the check exits 0 so it can't block merge. There's a marked one-line
toggle to flip it to a non-required soft-fail later if the comment alone
proves too easy to ignore. We deliberately skipped a hard "changed pkg
must update its doc" gate, since most code changes (bug fixes, refactors,
test-only) don't warrant a doc edit and the escape valve (touch the file
to bypass) just trains people to make meaningless edits.
One cost knob worth flagging: the advisory burns an Opus run on every
Go-touching PR. If that gets noisy, easy dials are a cheaper model for
the scoped diff, or a
paths-ignorefor**/*_test.go.The sibling client-side PR is lightninglabs/darepo-client (same two
commits, adjusted for
mainand no submodule).