feat(php-backend-sdlc): publish review findings + conclusion as GitHub PR comments - #9
Merged
Merged
Conversation
…review comments) Core of the publish-review-comments feature (issue #8), built from the BMAD planning chain (research/brief/prd/architecture/epics/readiness under specs/). - lib/common.sh: add wrap-safe num_add (digit-string addition) + lower helper. - scripts/post-review-findings.sh: the shared poster — gated (default-off capabilities.publish_pr_comments, first action), idempotent hidden-marker publish (REST list → marker+author match → PATCH else POST), canonical finding-record ledger, per-lens deduped+severity-ordered render, --conclusion aggregate (counts found / auto-fixed / duration via wrap-safe num_add), in-scope base-repo authorization, secret redaction, full degrade matrix (every failure → note + exit 0). Dual jq/python backend confined to a deterministic JSON→TSV projection so renders are byte-identical; the rest is single-impl bash. shellcheck -x clean. Verified by smoke: gating on/off, jq↔python parity, dedup by (cwe,location,endpoint), severity order, dropped grouping, redaction (password=/AKIA/JWT/url-creds), conclusion math + duration. Skill Publish slots, orchestrator wiring, profile keys, and the test tiers land in follow-up commits on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rator, profile, tests) Completes the publish-review-comments feature (issue #8) on top of the poster script (d96fb04), per the BMAD architecture. Profile keys (FR-8): capabilities.publish_pr_comments (bool, default false) and make.post_review_findings (nullable, plugin-substitutes the script) — added to docs/profile-schema.md (table + # profile-example), emitted by generate-profile.sh, required by validate-profile.sh MAKE_KEYS (valid.yml fixture updated to stay complete). Gated Publish steps (FR-6) in security-audit (§5.7), bmad-fr-nfr-review-gate, and code-review SKILLs — each gates on the capability, resolves the poster via make.post_review_findings (null → ${CLAUDE_PLUGIN_ROOT}/scripts/...), emits its lens ledger, and states the degrade contract; both keys added to each skill's Profile keys consumed. Orchestrator (FR-10): /sdlc-review captures loop start/end and posts the --conclusion once at loop close (single post; no double-post on the finish-pr hand-off). Poster improvements from the test-author critique: edit the OLDEST (min numeric id, wrap-safe) not list-order; minimize surplus duplicate marker'd comments (lazy node_id fetch) for corruption recovery; emit the no-PR skip-note to stderr so it surfaces; zero-pad the conclusion duration (12m 04s); tighten redaction rule #6 so `/`-containing paths aren't over-redacted. Tests (FR-11): tests/post-review-findings.bats — 37 cases (render, jq↔python byte-parity, idempotent create/update, duplicate collapse, dedup, severity order, redaction, gating on/off, every degrade row, base-repo refusal, conclusion math + wrap-safe + duration, install-cache) + fixtures. Full suite 235/235 green; shellcheck -x clean; markdownlint clean; component counts 8/7/22 unchanged; NFR-2 denylist clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds an opt-in, default-off ChangesPublish Review Findings as GitHub PR Comments
Gitignore Update
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as /sdlc-review
participant Skill as Lens SKILL
participant Poster as post-review-findings.sh
participant GH as gh CLI
Orchestrator->>Orchestrator: capture REVIEW_STARTED_AT
loop per review iteration
Orchestrator->>Skill: run lens (security / fr-nfr / code-review)
Skill->>Skill: emit findings to .sdlc/review-ledgers/<lens>.json
Skill->>Poster: invoke with lens arg (gated: publish_pr_comments=true)
Poster->>Poster: validate ledger, dedup, redact secrets
Poster->>GH: list PR comments (author-filtered)
GH-->>Poster: existing comments
alt marker found
Poster->>GH: PATCH existing comment
else no marker
Poster->>GH: POST new comment
end
end
Orchestrator->>Orchestrator: capture REVIEW_ENDED_AT
Orchestrator->>Poster: invoke --conclusion (post-exit, gated)
Poster->>Poster: aggregate counts via num_add, format duration
Poster->>GH: POST/PATCH conclusion comment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
…ression test Real-PR QA on PR #9 caught it: `gh api -X POST/PATCH ... -f body=@-` posts the LITERAL string "@-", because gh only reads stdin for a field with -F/--field (@-/@file expansion); -f/--raw-field takes the value verbatim. The comment body was therefore never sent (the marker'd comment never appeared; junk "@-" comments were created instead). Switch both the POST and PATCH writes to `-F body=@-` so the rendered Markdown is read from stdin. The bats gh-stub logs calls but cannot emulate gh's -f/-F stdin semantics, so it missed this — added a regression assertion to the CREATE test that the write uses `-F body=@-` and never `-f body=@-`. Verified live on PR #9: create → one marker'd comment; re-run → idempotent update (still one); secret redaction confirmed in the posted body; QA comments cleaned up afterward. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/php-backend-sdlc/tests/fixtures/profiles/valid.yml (1)
50-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
publish_pr_commentscapability in the reference fixture.The fixture should include
publish_pr_comments: falsein the capabilities block to match the schema and the auto-emitted profile. Currently it's missing.🔧 Proposed fix
capabilities: structurizr: false observability_emf: false load_testing: true + publish_pr_comments: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/php-backend-sdlc/tests/fixtures/profiles/valid.yml` around lines 50 - 54, Add the missing `publish_pr_comments: false` capability to the capabilities block in the valid.yml fixture file. The capabilities section currently includes structurizr, observability_emf, and load_testing, but is missing the publish_pr_comments field which is required by the schema and expected in auto-emitted profiles. Insert this capability in the appropriate location within the capabilities block to ensure the fixture matches the complete schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/php-backend-sdlc/commands/sdlc-review.md`:
- Around line 191-197: The documentation shows a contract mismatch where the
text describes using the profile-mapped poster target from
`make.post_review_findings`, but the bash command at Line 196 hardcodes the
default script path instead of using a variable. Replace the hardcoded
`"${CLAUDE_PLUGIN_ROOT}/scripts/post-review-findings.sh"` path in the conclusion
invocation with a variable (such as `POST_REVIEW_FINDINGS`) that should be
defined from the profile configuration with a default fallback. Add the variable
definition immediately before this snippet to establish the profile-mapped
target while maintaining the default path as a fallback when no profile override
is provided.
In `@specs/autonomous/2026-06-14-publish-review-comments/architecture.md`:
- Around line 317-334: The current filter logic in the matches condition allows
marker-only matching when POSTING_LOGIN is empty, which creates a security risk
where any comment containing the marker could be updated instead of only the
bot's own comment. Remove the fallback to marker-only matching by modifying the
filter condition: when POSTING_LOGIN is empty (posting identity cannot be
resolved), the filter should not match any comments, effectively skipping the
update path. This prevents updating reviewer comments that happen to quote the
marker when the bot login cannot be determined.
---
Outside diff comments:
In `@plugins/php-backend-sdlc/tests/fixtures/profiles/valid.yml`:
- Around line 50-54: Add the missing `publish_pr_comments: false` capability to
the capabilities block in the valid.yml fixture file. The capabilities section
currently includes structurizr, observability_emf, and load_testing, but is
missing the publish_pr_comments field which is required by the schema and
expected in auto-emitted profiles. Insert this capability in the appropriate
location within the capabilities block to ensure the fixture matches the
complete schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca4af57-f6e4-4987-b7f9-7a1078906464
📒 Files selected for processing (28)
.gitignoreplugins/php-backend-sdlc/commands/sdlc-review.mdplugins/php-backend-sdlc/docs/profile-schema.mdplugins/php-backend-sdlc/scripts/generate-profile.shplugins/php-backend-sdlc/scripts/lib/common.shplugins/php-backend-sdlc/scripts/post-review-findings.shplugins/php-backend-sdlc/scripts/validate-profile.shplugins/php-backend-sdlc/skills/bmad-fr-nfr-review-gate/SKILL.mdplugins/php-backend-sdlc/skills/code-review/SKILL.mdplugins/php-backend-sdlc/skills/security-audit/SKILL.mdplugins/php-backend-sdlc/tests/fixtures/ledgers/20-digit-count.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/dedup-pair.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/dropped-and-open.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/empty.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/full.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/minimal.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/mixed-severity.jsonplugins/php-backend-sdlc/tests/fixtures/ledgers/secret-laden.jsonplugins/php-backend-sdlc/tests/fixtures/profiles/publish-off.ymlplugins/php-backend-sdlc/tests/fixtures/profiles/publish-on.ymlplugins/php-backend-sdlc/tests/fixtures/profiles/valid.ymlplugins/php-backend-sdlc/tests/post-review-findings.batsspecs/autonomous/2026-06-14-publish-review-comments/architecture.mdspecs/autonomous/2026-06-14-publish-review-comments/epics.mdspecs/autonomous/2026-06-14-publish-review-comments/implementation-readiness.mdspecs/autonomous/2026-06-14-publish-review-comments/prd.mdspecs/autonomous/2026-06-14-publish-review-comments/product-brief.mdspecs/autonomous/2026-06-14-publish-review-comments/research.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…poster)
CI shellcheck (older version than local) flagged SC2119 on profile_path with no
arg → call profile_path "$PWD".
Adversarial review of the feature found 2 High + 4 Medium real bugs, all fixed:
- [High] the comment-list `gh --jq` did `gsub` on a possibly-null `.body`, which
errors out, empties the list, and makes the poster CREATE a duplicate every
run; guard with `(.body // "")`.
- [High] finding text was interpolated raw into the Markdown table — a `|` in a
summary/location injected phantom columns; escape `|`→`\|` in cell_or_na.
- [Med] dedup by (cwe,location,endpoint) collapsed distinct cwe-less fr-nfr /
code-review findings at the same location, dropping real findings; key cwe-less
findings by their unique id instead.
- [Med] the high-entropy redaction rule nuked any 32+ alnum run (long class
names / path segments); now require a digit in the run (real tokens have one).
- [Med] the keyword-assignment redaction fired on prose ("token: expired"); now
require an ≥8-char value.
- [Med] --conclusion duration ignored the ledgers' started_at/ended_at fallback
(OQ-7); derive min-start … max-end from the ledgers when no explicit source.
- redact() is now a single python pass on fd 3 (not `python3 -`, which would
read the program from stdin and swallow the piped data); human_duration
rejects an absurd >12-digit value (NFR-4 letter).
4 regression bats added (pipe-escape, distinct-cwe-less-dedup, no-over-redaction,
duration ledger fallback). Poster suite 41/41; shellcheck -x clean; verified
live on PR #9 (create→idempotent update, redaction, cleanup).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit (CHANGES_REQUESTED) + cubic + qlty actionable items: - valid.yml reference fixture: add capabilities.publish_pr_comments: false so it matches the full schema / auto-emitted profile (CodeRabbit Major). - poster author filter: never marker-only match when the posting identity is unresolved — that could edit a human comment quoting the marker; require a resolved login to match, else create + warn (CodeRabbit/cubic, R7). - /sdlc-review conclusion invocation: resolve the poster via make.post_review_findings (null → bundled script) instead of a hardcoded path, matching the per-lens Publish steps; guard $PR (cubic P1/P2 contract mismatch). - qlty: exclude **/tests/fixtures/** from scanning — the redaction-test ledgers hold intentional fake secrets (AWS example key / sample JWT), not real ones (qlty gitleaks/checkov false positives). - architecture.md §4.2: sync the doc with the implementation — null-guarded body in the list jq, resolved-login-required match, -F (not -f) body=@-, and the lazy node_id fetch for the minimize path (cubic P1 doc mismatch). shellcheck -x clean; bats 84/84 (poster + profile suites) green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Addressed review feedback (commit ce9e515)Thanks @coderabbitai @cubic-dev-ai @qltysh — all actionable items resolved:
Also fixed in earlier commits on this branch from an internal adversarial pass + real-PR QA: the |
|
Tip For best results, initiate chat on the files or code changes.
The bonus fixes from the adversarial pass (especially the If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…he conclusion slot
cubic (P1, conf 10): the prior conclusion example called `profile_make_target`,
which does not exist → a Bash `command not found` at runtime (the `${POSTER:-…}`
fallback masked it). Use the actual lib/common.sh helpers the plugin scripts
already source — `profile_path` + `profile_get "$profile" make.post_review_findings ""`
— with the bundled script as the null default; a non-null key maps to a custom
publisher. Matches the per-lens Publish steps' null-substitution.
dmytrocraft
dismissed
coderabbitai[bot]’s stale review
June 14, 2026 13:13
All CodeRabbit threads resolved in ce9e515; cubic + qlty re-reviewed non-blocking; CI green. Dismissing the superseded review.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Publish review findings + conclusion as GitHub PR comments
Closes #8. Built via the BMAD planning chain (
specs/autonomous/2026-06-14-publish-review-comments/) → implementation.Adds an opt-in, default-off capability so the plugin's three review lenses (security-audit, BMAD fr/nfr, code-review) publish their findings as one idempotent PR comment per lens, plus a conclusion comment at loop close (counts found by lens×severity, count auto-fixed root-cause-with-regression-test, run duration).
What's here
scripts/post-review-findings.sh— shared poster: gated (first action), idempotent hidden-marker publish (REST list → marker+author match → edit oldest, else create; minimize surplus duplicates), canonical finding-record ledger, per-lens deduped+severity-ordered render,--conclusionaggregate, in-scope base-repo authorization, secret redaction, full degrade matrix (every failure → note +exit 0). Dual jq/python backend confined to a deterministic JSON→TSV projection so renders are byte-identical.lib/common.sh— wrap-safenum_add+lowerhelpers (no(( ))over counts).--conclusionwiring in/sdlc-review.capabilities.publish_pr_comments(bool, default false) +make.post_review_findings(nullable) — schema + generator + validator.tests/post-review-findings.bats(37 cases) + fixtures; prompt-quality LLM-judge re-judges the edited skills.Verification
shellcheck -xclean; markdownlint clean; component counts 8/7/22 unchanged; NFR-2 denylist clean.🤖 Generated with Claude Code
Summary by cubic
Adds an opt-in feature to publish review findings as GitHub PR comments: one consolidated, idempotent comment per lens (security, FR/NFR, code review) plus a single conclusion at loop end. Improves safety and reliability with correct
gh -Fposting, stricter author matching, safer redaction, Markdown escaping, and duration fallback.Bug Fixes
lib/common.shhelpers (profile_path+profile_get make.post_review_findings, null → bundled script) and guard$PRin/sdlc-review..body, escape|in table cells, and fix cwe-less dedup by uniqueid.started_at/ended_atand rejects absurd (>12-digit) values.**/tests/fixtures/**from quality scanners to prevent false positives.Migration
capabilities.publish_pr_comments: trueto enable.make.post_review_findings; otherwise uses the built-in script.ghcan comment on PRs in the base repository.Written for commit 5a17624. Summary will update on new commits.