Repository navigation
Conversation
The skill turns a ticket with an accepted Design, acceptance criteria and Build plan into a reviewed local branch. It writes a sub-session checklist on the ticket and runs build, review and test as fresh subagents. It stops for the engineer on design changes, the review round limit, the budget and non-converging problems. Pi runs the steps in one session. .gitignore re-includes the skill directory, which **/build-*/ would otherwise ignore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on (PROF-16594) Pi can get subagents from an extension such as pi-subagents. This version still runs the parts one after another in Pi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A drive with pi-subagents called its tool to read the extension guide. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A trial-run drive had MCP servers and write tools, which the gh stub does not cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two critical workflow gaps stand out: the pre-push hook installation step (step 0) is never invoked, so stale-review checks can be bypassed on subsequent pushes; and --allowedTools is used instead of --tools, leaving Write and Bash permissions active during test drives and defeating the required isolation.
🤖 Bits Code Review · Commit 009a0c0
| Then run `pre-pr-review` steps 4 to 7 in this session. The S3 reviews are its | ||
| step 3 reviews, its step 4 fixes go through S1 as in step 4 above, and the marker |
There was a problem hiding this comment.
In a clone without the pre-push shim, this workflow never installs it: S1 invokes step 2, S3 invokes step 3, and Close invokes steps 4–7, omitting step 0. Writing the review marker does not install the hook. Consequently, later pushes to an open PR can bypass the stale-review check. Run the idempotent hook-installation step before completing the workflow.
| Then run `pre-pr-review` steps 4 to 7 in this session. The S3 reviews are its | |
| step 3 reviews, its step 4 fixes go through S1 as in step 4 above, and the marker | |
| Then run `pre-pr-review` step 0 and steps 4 to 7 in this session. S3 supplies its | |
| step 3 reviews, its step 4 fixes go through S1 as in step 4 above, and the marker |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 5372adf: the close now runs pre-pr-review step 0 (install-hook) before steps 4 to 7.
| - **S2 Test** runs the `test-drive` acceptance criteria in a fresh clone of the | ||
| candidate commit, never in the worktree. Each drive runs without MCP servers and | ||
| with only the tools that its criterion needs, for example `claude -p` with | ||
| `--strict-mcp-config` and `--allowedTools`. Never skip hooks or bypass |
There was a problem hiding this comment.
--allowedTools adds permission rules; it does not restrict available tools. A drive inheriting this repository’s .claude/settings.json retains its configured Write and Bash permissions, even when only Read is listed. Such a drive can still modify files and execute commands, defeating the required isolation. Use --tools to restrict available tools, alongside permission rules for those retained.
| `--strict-mcp-config` and `--allowedTools`. Never skip hooks or bypass | |
| `--strict-mcp-config`, `--tools` and `--allowedTools`. Never skip hooks or bypass |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 5372adf: drives use --tools to limit the available tools and --allowedTools to permit them, with --strict-mcp-config.
| 4. Run S2 only after a review round that left no Critical or High finding open. A | ||
| failed check goes to a new S1 run like a finding, its fix goes to S3, and then S2 | ||
| runs again. |
There was a problem hiding this comment.
When S2 reports blocked or inconclusive—for example, because the required harness is unavailable—the loop handles only failed checks, and none of the stops handles these unverified states. It consequently permits Close to tick the checklist and declare the branch ready without passing acceptance evidence. Stop for the engineer until the required results are verified.
| 4. Run S2 only after a review round that left no Critical or High finding open. A | |
| failed check goes to a new S1 run like a finding, its fix goes to S3, and then S2 | |
| runs again. | |
| 4. Run S2 only after a review round that left no Critical or High finding open. A | |
| failed check goes to a new S1 run like a finding, its fix goes to S3, and then S2 | |
| runs again. If a required S2 criterion is `blocked` or `inconclusive`, stop for | |
| the engineer; do not proceed to Close without the required passing evidence. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
![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: 009a0c04a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| the kind of change, and the `pre-pr-review` step 2 self-checks. It commits | ||
| locally only when they pass, and it does not push. Its report names the commit |
There was a problem hiding this comment.
When a required local check is legitimately blocked—for example, verify-profiler-change explicitly requires that status when Zing or a required runtime is unavailable—S1 cannot commit, while the loop assumes a candidate commit and the stop list has no first-time blocked-check case. The orchestration therefore stalls instead of reporting the supported blocked state; add a stop/decision path or allow a candidate commit carrying blocked evidence.
AGENTS.md reference: AGENTS.md:L488-L493
Useful? React with 👍 / 👎.
| Then run `pre-pr-review` steps 4 to 7 in this session. The S3 reviews are its | ||
| step 3 reviews, its step 4 fixes go through S1 as in step 4 above, and the marker | ||
| names this session. For its step 7, use the sub-session reports and this | ||
| session's own mistakes. Do not push and do not open a PR: tell the engineer that |
There was a problem hiding this comment.
On normal completion, the preceding instruction requires posting the evidence table before this line forbids pushing, but ticket-writing requires candidate commits and branches to be links and explicitly says an unpushed commit link does not resolve. Because S1 creates only local commits, the ticket cannot record compliant evidence here without violating one of the two instructions; defer the evidence until after a push or define a permitted publication step.
AGENTS.md reference: AGENTS.md:L488-L493
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5372adf: the close names the unpushed commits by hash and adds the links after the engineer pushes, as ticket-writing allows ("push first, or add the links after the push").
CI Test ResultsRun: #43459899527 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-13 16:32:52 UTC |
- Run pre-pr-review step 0 at the close, so the pre-push hook is installed in a fresh clone. - Limit test-drive tools with --tools; --allowedTools only permits. - Stop when a required S1 or S2 check is blocked or inconclusive, and let S1 commit with a blocked check the engineer accepted. - Name unpushed commits by hash and add the links after the push. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…6594) S1 gets the engineer's acceptance of a blocked check in its prompt, and the stop for blocked or inconclusive checks ignores results that are only stale after a new commit. Rewrap two lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
The PR adds a ticket-driven build, review, and test workflow for accepted PROF designs. Tool isolation and hook installation are fixed, but one issue remains: S1 cannot commit after the engineer accepts an inconclusive check because its prompt and commit rule only honor accepted blocked checks.
🤖 Bits Code Review · Commit d5b1b92
| triaged findings or the failed checks, quoted, and any `blocked` check that the | ||
| engineer accepted (step 5). Never give your own view of the | ||
| change or of its defects. Tell the sub-session to read its part on the ticket, to | ||
| write nothing to the ticket, and to return a report. | ||
| - **S1 Build** changes the code. It does not start another orchestration skill, | ||
| such as the cairn `dev-process` pipeline or `sphinx:implement`. It runs the | ||
| `local` acceptance criteria, the checks that `verify-profiler-change` names for | ||
| the kind of change, and the `pre-pr-review` step 2 self-checks. It commits | ||
| locally only when they pass, or when its prompt quotes the engineer's | ||
| acceptance of a check that stays `blocked`. Otherwise it reports without a | ||
| commit. It does not push. Its report names the commit and each check as | ||
| `passed`, `failed`, `blocked` or `inconclusive`. |
There was a problem hiding this comment.
When S1 reports a required check as inconclusive, step 5 allows the engineer to accept the evidence gap. However, the sub-session prompt forwards only accepted blocked checks, and S1’s commit exception likewise covers only blocked results. The implementation therefore remains uncommitted despite the engineer’s acceptance, preventing review and marker creation for that candidate. Honor accepted inconclusive results in both places.
| triaged findings or the failed checks, quoted, and any `blocked` check that the | |
| engineer accepted (step 5). Never give your own view of the | |
| change or of its defects. Tell the sub-session to read its part on the ticket, to | |
| write nothing to the ticket, and to return a report. | |
| - **S1 Build** changes the code. It does not start another orchestration skill, | |
| such as the cairn `dev-process` pipeline or `sphinx:implement`. It runs the | |
| `local` acceptance criteria, the checks that `verify-profiler-change` names for | |
| the kind of change, and the `pre-pr-review` step 2 self-checks. It commits | |
| locally only when they pass, or when its prompt quotes the engineer's | |
| acceptance of a check that stays `blocked`. Otherwise it reports without a | |
| commit. It does not push. Its report names the commit and each check as | |
| `passed`, `failed`, `blocked` or `inconclusive`. | |
| triaged findings or the failed checks, quoted, and any `blocked` or | |
| `inconclusive` check that the engineer accepted (step 5). Never give your own | |
| view of the change or of its defects. Tell the sub-session to read its part on | |
| the ticket, to write nothing to the ticket, and to return a report. | |
| - **S1 Build** changes the code. It does not start another orchestration skill, | |
| such as the cairn `dev-process` pipeline or `sphinx:implement`. It runs the | |
| `local` acceptance criteria, the checks that `verify-profiler-change` names for | |
| the kind of change, and the `pre-pr-review` step 2 self-checks. It commits | |
| locally only when they pass, or when its prompt quotes the engineer's | |
| acceptance of a check that stays `blocked` or `inconclusive`. Otherwise it | |
| reports without a commit. It does not push. Its report names the commit and | |
| each check as `passed`, `failed`, `blocked` or `inconclusive`. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Correct, and known: the last Sphinx fix-range review found the same gap. Roman Kennke accepted it as an open Low, with no further review rounds in this PR, so it is not fixed here. The PR description lists it.
What does this PR do?:
Adds the
build-test-reviewagent skill. It turns a PROF ticket whose Design, acceptance criteria and Build plan are accepted into a reviewed local branch:inconclusiveafter each new commit.pre-pr-reviewround limit, before the review budget is exceeded, and when a problem does not converge. It also stops before any build step when a sign-off is missing, and does not start the design itself.pre-pr-reviewsteps, writes the marker in its own name and stops before push. The engineer opens the PR.A
.gitignoreline re-includes the skill directory, because**/build-*/would ignore it.Motivation:
Today an engineer sets up the build, test and review sessions and the loop between them by hand for each change. The PROF-16588 build ran this flow manually; this skill writes it down.
Additional Notes:
pre-pr-review; the skill links to its anchors, so the two cannot drift.pi-subagentsextension provides one. Pi subagents, cost per sub-session and a regression drive of the whole loop are PROF-16614.ghstub onPATHrefuses writes. Known gap, accepted as Low: a drive can still reach the realghby its absolute path.pre-pr-reviewgate row (Sphinx).pre-pr-reviewallows. Its Low stays open, accepted by Roman Kennke: aninconclusivecheck that the engineer accepts is not passed to S1 the way ablockedone is.How to test the change?:
python3 utils/check-skills.py;bash utils/tests/test_quality_check_scripts.shskills: OK (10 skills);PASS: 231 test groupsbuild-test-review, neithercairn:dev-processnorsphinx:implementsubagenttool (3 runs each)The ticket has the evidence tables with commits and configurations.
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code