Skip to content

feat(source-control): count the security lane's docs-only skip as review evidence for all-docs pull requests - #8791

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/docs-only-security-evidence
Oct 21, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
feat/docs-only-security-evidence

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner decision A7 (2026-10-21, automation-lanes interview): a security-lane skip=docs-only counts as review evidence for docs-only paths.

Hold: a human merges this. It changes merge-gate evidence; poteto reviews first, then the Security & Cost pass, then the owner merges.

Summary

The babysit merge gate (babysit_review_evidence.py) unbinds the security lane on skip=docs-only (raise Unbound("skip-not-no-change")), so a documentation-only pull request never merges unattended: the security review lane skips it on purpose, and the gate reads that as no review.

Fix

  • bind() computes once whether every changed path (both rename sides) is documentation by the security lane's own rule: its docs-only-paths default at the pinned ci-workflows v0.48.0 (docs/**/*.md, **/README.md, **/CHANGELOG.md) minus the agent-instruction paths the lane never counts as documentation. Stricter than the lane, any reference/ directory, docs/adr/, docs/conventions/ and docs/standards/ never count, in any case (Security & Cost). A path holding \r, \n, U+2028 or U+2029 is never docs (the lane's JavaScript . stops at them). The globs and the instruction pattern are a byte-level port of the lane's globToRegExp and INSTRUCTIONS, fixed in code like LANES.
  • _resolve() binds the security lane's skip=docs-only at the head as a chain of its own only when that flag holds and every record the lane left on the pull request is a docs-only skip (the lane never reviewed it), so an earlier review's findings are never dropped; and the lane's reviewed-head marker is not older than every listed run (a deleted reviewing run would otherwise leave its marker for the skip to edit). Every other skip, and the code review lane's docs-only skip, still unbinds with skip-not-no-change.
  • The lane applies its rule to the files since its last review, so its skip alone says nothing about the whole pull request; the gate re-checks the whole diff.
  • Open security threads still count. A docs-only skip never reports whole_review_clears, because a whole re-review of an all-docs pull request skips again (avoids a feat(pr-pipeline): request a whole review when a lane PR is held only by findings an incremental review cannot clear #8580 request loop).
  • Fails safe both ways: a wider lane list than the gate's is refused here; a narrower one never emits the skip.

Verification

  • python3 -m unittest tests.test_babysit_review_evidence: 136 tests OK at f143a26, including the new SecurityDocsOnlyTests (binds on all-docs, unbinds on one code path, on non-matching docs-looking paths, on instruction paths such as skills/**/README.md and AGENTS.md, on a rename from code, on an empty diff; an unresolved or agent-resolved security thread still counts; glob grammar).
  • Mutation checks: removing the whole_review_clears guard, the never-reviewed check, the line-break check, or switching fullmatch to search each fails a test.
  • poteto reviewed 2b171fe; findings 1-3 fixed in ca56dc4 (three-push case, line breaks, anchoring tests), OK at ca56dc4.
  • Security & Cost held ca56dc4 on three findings, fixed in f143a26 (instruction trees, stale marker, case); each fix fails a test when removed.
  • scripts/run-ruff.sh check and format --check: clean.
  • Producer behaviour read at the pinned SHA 96b50d45 (pr-review-security.yml): the skip record is ... skip=docs-only, and the reviewed-head marker is written on a skip too.

Related

  • Code-review lane unchanged (its reusable's docs-only-paths default is empty), so docs-only PRs still need a real code review.
  • Not in scope: a code PR whose last push is docs-only gets skip=docs-only at head with no through, so the security lane stays unbound there.
  • Live proof after merge: the next docs-only lane PR merging through pr-merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DZUxjJX2Sbu4ASZUPcwC3M

…iew evidence for all-docs pull requests

The merge gate unbound the security lane on skip=docs-only, so a
documentation-only pull request never merged unattended (owner decision A7).
The skip now binds the security lane, with no findings of its own, when
every changed path matches the lane's v0.48.0 docs-only default and none is
an agent instruction path. The code review lane is unchanged; open security
threads still count, and the skip never reports that a whole review clears.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZUxjJX2Sbu4ASZUPcwC3M
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 21, 2026 03:29
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-21T03:30:49.233780Z 2b171fe Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed f143a261c063fcad41d2db8a7a7ee07b8c25c0e0: 0 finding(s), 0 as inline comments.

Security review: incremental since ca56dc4b (head f143a261)

No security findings. I reviewed .claude-lane/incremental.diff for babysit_review_evidence.py and its test file, plus the surrounding code. I did not run the tests.

The change only makes the docs-only skip stricter. It does not make binding easier:

  • Path rule (INSTRUCTIONS_RE): reference/ directories and docs/{adr,conventions,standards}/ no longer count as documentation, and the match is now case-insensitive. Fewer paths pass as docs-only, so fewer pull requests get a skipped security review accepted as their review. The only caller is is_docs (line 227), where a match means "not docs", so nothing widens.
  • Marker-age check in _bind_lane: a docs-only skip now stays unbound (marker-predates-runs) when the marker is older than every listed lane run. That closes the case where a deleted run that did review the pull request wrote the marker. lane_runs cannot be empty at that point, because _resolve has already bound a run at the head. A missing created_at becomes "", which only makes the check less likely to trip, never more likely to bind.
  • _require_marker return value: it now returns the marker's creation time. The "newest marker names the head" logic is unchanged.
  • Tests: the changes add cases for the new unbound paths. The existing docs-only positive test now uses docs/guide/setup.md instead of docs/adr/, consistent with the new rule.

No instruction-surface deletion applies: no guardrail text is removed, and the code-level control is tightened.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed f143a261c063fcad41d2db8a7a7ee07b8c25c0e0: 0 finding(s), 0 as inline comments.

Incremental review: ca56dc4 → f143a26

No findings. I read the incremental diff and the surrounding code in babysit_review_evidence.py. I did not run the test suite: the sandbox blocked the command. The PR's "136 tests OK" is author-claimed and unverified by me.

Changes reviewed

  • INSTRUCTIONS_RE: now covers reference/ and docs/(adr|conventions|standards)/, case-insensitively. The extra |^docs/... alternative sits at the top level of the pattern, so it is not bound inside the (?:^|/) group. is_docs applies it with search, so it behaves as intended.
  • _require_marker: returns the marker's created_at. _bind_lane unbinds with marker-predates-runs when a docs-only-skip chain has a marker older than every listed run. Both sides are the same ISO-8601 Z string format. A missing timestamp becomes "", which unbinds, so it fails safe. A real run is created before the marker its job writes, so marker >= min(run created_at) holds for legitimate cases, and the test helpers use equal timestamps. chain[0] is always set once _resolve returns.
  • Tests: the new cases cover the reference/, docs/adr, docs/conventions and docs/standards paths, the case variants, and the stale-marker unbind. The non-skip control case, where marker age doesn't matter, is also covered.

Observations, not blocking

  • Two new test paths pass for a different reason than their comment says. Docs/ADR/x.md and agents.md never match the case-sensitive docs globs, so they are rejected whether or not IGNORECASE is set. docs/Agents/x.md does exercise the case-insensitivity.
  • The marker-predates-runs check only catches a deleted run that is older than every listed run, such as one removed by retention. A deleted reviewing run that falls between listed runs would not be caught. The PR's stated scope doesn't claim more than that.

kyle-sexton and others added 2 commits October 20, 2026 23:40
…ane never reviewed the pull request

poteto's review of #8791: a docs-only skip at the head replaced the whole
chain, so an earlier security review's summary findings dropped out of the
count. The skip now binds only when every record the lane left on the pull
request is a docs-only skip. is_docs also refuses \r, \n, U+2028 and
U+2029, which the lane's JavaScript `.` never matches, and tests pin the
^...$ anchoring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZUxjJX2Sbu4ASZUPcwC3M
… the docs-only bind

Security & Cost's pass on #8791: is_docs now also refuses any reference/
directory (skills read it as instructions, such as the GitHub provider
README the babysit skills follow), docs/adr/, docs/conventions/ and
docs/standards/, in any case. A docs-only skip also unbinds when the lane's
reviewed-head marker is older than every listed run: the lane edits one
marker in place, so such a marker came from a run no longer listed that may
have reviewed the pull request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZUxjJX2Sbu4ASZUPcwC3M
@kyle-sexton
kyle-sexton enabled auto-merge October 21, 2026 04:07
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit a22e385 Oct 21, 2026
52 of 53 checks passed
@kyle-sexton
kyle-sexton deleted the feat/docs-only-security-evidence branch October 21, 2026 04:34
@kyle-sexton

Copy link
Copy Markdown
Member Author

Security & Cost put this PR on HOLD at f143a26 after auto-merge was armed, and it merged before the hold was relayed. The gaps they found are in #8827 (fix or revert; the owner decides). Security & Cost recommends reverting: as built, this bound 0 of the last 100 lane PRs.

github-merge-queue Bot pushed a commit that referenced this pull request Oct 21, 2026
…kip as review evidence (#8841)

Closes #8827

## Summary

Reverts #8791. Security & Cost found after it merged that the security
lane's docs-only skip could still count as review evidence when the
reviewing run was deleted, and for `docs/plugin-philosophy.md`,
`docs/upstream/**` and `docs/architecture/**`, which instruction files
cite. As built it bound 0 of the last 100 lane pull requests, so the
revert removes the bypass and loses nothing in practice. Owner decision
on #8827, 2026-10-21.

## Fix

- `git revert` of the #8791 merge commit: `babysit_review_evidence.py`
and its tests return to their state before #8791.
- #8791's changelog fragment was never released, so the revert deletes
it and adds a `bump: none` fragment for source-control.
- A docs-only evidence rule returns only if a real lane needs it, with a
recorded-attempt window check and an explicit docs allowlist, passed
again by Security & Cost.

## Verification

- `scripts/check-affected.sh`: 12 passed, 1 skipped (the changed Python
tests and the changelog fragment checks included).
- Left out: nothing beyond CI's own suites, which run on this PR.

## Related

- #8791: the change reverted.
- #8834: the related finding that pr-merge reads merge settings at a
PR's stale base.


🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant