Skip to content

Commit 6c8c67d

Browse files
igerberclaude
andcommitted
Address PR R0 P3s: bootstrap placeholder + hardening test coverage
P3 #1 (Maintainability): The bootstrap-skip branch — when BASE_SHA does not yet contain `tools/notebook_md_extract.py` and the PR touches tutorials — previously only logged "skipped" to stdout without emitting anything to the compiled prompt. The reviewer would see no prose section AND no indication why. Restructured the prose-extraction block so CHANGED_NB is computed unconditionally, and added an `elif [ -n "$CHANGED_NB" ]` branch that emits a `<notebook-prose untrusted="true">` placeholder explaining the one-shot bootstrap state and listing the changed tutorial files. This PR itself is the bootstrap case but doesn't touch tutorials, so the path is currently exercised only structurally; the first tutorial-touching PR after merge stops triggering this branch entirely. P3 #2 (Tech Debt): Extended `TestWorkflowPromptHardening` with two parametrized-style tests mirroring the existing `<pr-title>`/`<pr-body>` coverage: one asserts the `<notebook-prose untrusted="true">` wrapper + closing tag are present in the workflow YAML; the other asserts the sanitizer escapes `</notebook-prose>` to `&lt;/notebook-prose&gt;`. A future workflow edit that drops the wrapper or sanitizer for the new tag now fails the test suite. All 16 tests in TestWorkflowPromptHardening + test_notebook_md_extract pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f3ca8c7 commit 6c8c67d

2 files changed

Lines changed: 50 additions & 2 deletions

File tree

.github/workflows/ai_pr_review.yml

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,9 +260,9 @@ jobs:
260260
# the prompt within input budget. Fail-soft per notebook: a
261261
# malformed one degrades to a placeholder line rather than killing
262262
# the AI review job.
263+
CHANGED_NB=$(git --no-pager diff --name-only "$BASE_SHA" "$HEAD_SHA" \
264+
-- 'docs/tutorials/*.ipynb' 2>/dev/null || true)
263265
if [ -f /tmp/notebook_md_extract.py ]; then
264-
CHANGED_NB=$(git --no-pager diff --name-only "$BASE_SHA" "$HEAD_SHA" \
265-
-- 'docs/tutorials/*.ipynb' 2>/dev/null || true)
266266
if [ -n "$CHANGED_NB" ]; then
267267
: > /tmp/notebook-prose.md
268268
while IFS= read -r nb; do
@@ -295,6 +295,27 @@ jobs:
295295
echo "</notebook-prose>"
296296
} >> "$PROMPT"
297297
fi
298+
elif [ -n "$CHANGED_NB" ]; then
299+
# Bootstrap-skip path: the trusted extractor does not yet exist
300+
# on BASE_SHA (one-shot for the PR that introduces it), but the
301+
# PR touches tutorial notebooks. Emit an in-prompt placeholder
302+
# so the reviewer knows prose extraction was intentionally
303+
# skipped (vs silently absent).
304+
{
305+
echo ""
306+
echo "<notebook-prose untrusted=\"true\">"
307+
echo "Tutorial notebook prose extraction was SKIPPED for this run:"
308+
echo "the notebook extractor (tools/notebook_md_extract.py) does"
309+
echo "not yet exist on BASE_SHA. This is the one-shot bootstrap"
310+
echo "state for the PR that introduces the extractor;"
311+
echo "subsequent tutorial-touching PRs after that PR merges will"
312+
echo "see full prose."
313+
echo ""
314+
echo "Changed tutorial files (raw .ipynb JSON is excluded from"
315+
echo "the unified diff above; review the diff directly if needed):"
316+
printf '%s\n' "$CHANGED_NB"
317+
echo "</notebook-prose>"
318+
} >> "$PROMPT"
298319
fi
299320
300321
- name: Run Codex

tests/test_openai_review.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,6 +1754,33 @@ def test_workflow_sanitizes_pr_body_closing_tag(self):
17541754
assert "&lt;/pr-body&gt;" in text
17551755
assert "&lt;/previous-ai-review-output&gt;" in text
17561756

1757+
def test_workflow_wraps_notebook_prose_with_untrusted_attr(self):
1758+
"""Tutorial notebook prose extracted from changed .ipynb files is
1759+
PR-controlled and must be wrapped in <notebook-prose untrusted="true">
1760+
— same pattern as <pr-title>/<pr-body>/<previous-ai-review-output>."""
1761+
assert _SCRIPT_PATH is not None
1762+
repo_root = _SCRIPT_PATH.parent.parent.parent
1763+
wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
1764+
if not wf.exists():
1765+
pytest.skip("workflow not found")
1766+
text = wf.read_text()
1767+
# Shell uses backslash-escaped quotes inside the YAML literal block.
1768+
assert r'<notebook-prose untrusted=\"true\">' in text
1769+
assert "</notebook-prose>" in text
1770+
1771+
def test_workflow_sanitizes_notebook_prose_closing_tag(self):
1772+
"""Notebook content is PR-controlled — adversarial markdown
1773+
containing literal </notebook-prose> must be escaped so the
1774+
wrapper cannot be closed early. Mirrors the pr-body /
1775+
previous-ai-review-output sanitization."""
1776+
assert _SCRIPT_PATH is not None
1777+
repo_root = _SCRIPT_PATH.parent.parent.parent
1778+
wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
1779+
if not wf.exists():
1780+
pytest.skip("workflow not found")
1781+
text = wf.read_text()
1782+
assert "&lt;/notebook-prose&gt;" in text
1783+
17571784

17581785
class TestWorkflowCommentPosting:
17591786
"""The workflow has TWO rerun-detection gates that must agree:

0 commit comments

Comments
 (0)