feat: add pre and post implementation custom steps#81
Closed
kasin-it wants to merge 3 commits into
Closed
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughAdds a GitHub-triggered PR review pipeline: workflow config parsing/validation, PR bundle assembly, complexity and AI review checks with caching and fingerprints, VCS adapter review APIs, findings→annotations/comments mapping, GitHub webhook handler, Temporal dispatch helper, orchestration workflow, extensive tests, and documentation. ChangesPR Review Pipeline v1 — Checks, Workflow, and GitHub Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SETUP.md (1)
357-357:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale internal link reference.
The link to troubleshooting points to
#12-troubleshooting, but Troubleshooting is now section 13 after the PR Review Pipeline section was added.Proposed fix
-If anything stalls, jump to [troubleshooting](`#12-troubleshooting`). +If anything stalls, jump to [troubleshooting](`#13-troubleshooting`).🤖 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 `@SETUP.md` at line 357, Update the stale internal link in SETUP.md: find the text "If anything stalls, jump to [troubleshooting](`#12-troubleshooting`)." and change the anchor to point to the current Troubleshooting section (replace "`#12-troubleshooting`" with the correct anchor, e.g. "`#13-troubleshooting`" or the exact slug of the "Troubleshooting" header). Ensure the link text remains "troubleshooting" and the anchor matches the actual header slug for the Troubleshooting section.
🧹 Nitpick comments (3)
package.json (1)
34-34: 💤 Low valueDependency ordering inconsistency.
The
aipackage at line 34 is placed afterpino, breaking alphabetical order. For consistency and maintainability, consider moving it to line 20 (before@ai-sdk/anthropic) or afterh3at line 30.Proposed reordering
"h3": "^1", + "ai": "^5.0.0", "js-yaml": "^4.1.0", "nitropack": "^2", "pino": "^10.3.1", - "ai": "^5.0.0", "workflow": "latest",🤖 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 `@package.json` at line 34, The dependency "ai": "^5.0.0" is out of alphabetical order in package.json; move the "ai" entry so dependencies are sorted alphabetically (for example place "ai" before "`@ai-sdk/anthropic`" or after "h3") to restore consistent ordering, preserving its version string and trailing commas/JSON syntax while keeping other dependency entries unchanged.docs/GITHUB-APP-SETUP.md (1)
231-237: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block at line 231 should specify a language for proper syntax highlighting and to resolve the markdownlint warning.
Proposed fix
- ``` + ```text https://<your-vercel-domain>/webhooks/vcs/github/pull-request ```🤖 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 `@docs/GITHUB-APP-SETUP.md` around lines 231 - 237, Add a language identifier to the fenced code block containing the URL "https://<your-vercel-domain>/webhooks/vcs/github/pull-request" so markdownlint stops warning; change the opening fence from ``` to a language-specific fence such as ```text (or ```bash if you prefer) to enable proper syntax highlighting and linting for that code block.src/adapters/vcs/github.test.ts (1)
514-543: ⚡ Quick winAdd a regression test for
\ No newline at end of filemarker handling.Current parser tests cover
+++headers, but not the in-hunk backslash metadata marker. Add this case to prevent line-range drift regressions.Suggested test case
describe("parseChangedLineRangesFromPatch", () => { + it("ignores '\\ No newline at end of file' marker for line counting", () => { + const patch = "@@ -1,2 +1,2 @@\n-old\n+new\n\\ No newline at end of file\n+next\n"; + expect(parseChangedLineRangesFromPatch(patch)).toEqual([ + { start: 1, end: 1 }, + { start: 2, end: 2 }, + ]); + });🤖 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 `@src/adapters/vcs/github.test.ts` around lines 514 - 543, Add a regression test to ensure the in-hunk backslash metadata marker ("\ No newline at end of file") is ignored by parseChangedLineRangesFromPatch: create a new it block in the existing describe("parseChangedLineRangesFromPatch") tests that passes a patch containing a hunk with a real "+" addition line followed immediately by the "\ No newline at end of file" marker and assert that parseChangedLineRangesFromPatch returns only the expected range for the real added line (and does not treat the backslash marker as an added line); reference the parseChangedLineRangesFromPatch function name and the test suite in github.test.ts when adding this case.
🤖 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 `@docs/GITHUB-APP-SETUP.md`:
- Line 273: The markdown link "[PR Review Pipeline (v1)](#)" has an empty href;
update that link so it points to the actual section anchor in SETUP.md (e.g.,
replace "#" with "SETUP.md#pr-review-pipeline-v1" or the correct anchor
generated from the "PR Review Pipeline (v1)" header). Locate the link text "[PR
Review Pipeline (v1)](#)" and change the href to the proper relative anchor so
the link navigates to the PR Review Pipeline (v1) section.
In `@src/adapters/vcs/github.ts`:
- Around line 534-542: The logic that increments newLineNum when encountering a
context line mistakenly treats diff metadata like "\ No newline at end of file"
as a real file line; update the diff-parsing block (the one using rangeStart,
rangeEnd, ranges, and newLineNum) to detect and skip metadata lines (e.g., lines
that exactly start with "\ No newline at end of file" or match the typical diff
metadata pattern) so you do not increment newLineNum for those cases; leave
range flushing behavior intact but only increment newLineNum for actual file
lines (context/added/removed lines) rather than metadata markers.
In `@src/lib/check-output.ts`:
- Around line 42-44: sanitizeFences currently only replaces exact triple
backticks which allows runs of 4+ backticks to leave an unneutralized ```
sequence; change the replacement to target any run of 3 or more backticks by
using a /`{3,}/g matcher and neutralize the entire run by inserting zero‑width
joiners between each backtick (e.g., map the matched ticks to
ticks.split('').join('\u200D')); update the sanitizeFences function accordingly
and apply the same updated regex + replacement logic to the other
fence-sanitization call sites in this file that use the /```/g replacement so
all 3+ backtick runs are neutralized.
In `@src/lib/checks/ai-review.ts`:
- Around line 409-415: The whole-PR path in runWholePr is using a hard-coded
checkId value ("ai_review") when calling toFinding, which causes fingerprint
collisions; update that to use the configured check id (the same source used by
runPerFile) — e.g., replace the literal "ai_review" in the toFinding call with
the configured requested_data["check_id"] (or the variable that holds the check
id in this scope) so runWholePr and runPerFile generate consistent fingerprints
for the same configured check.
- Around line 440-445: The prompt currently injects PR-controlled strings raw
(e.g., file.path in buildPerFilePrompt and the changed_files usage around the
other block), so treat those as untrusted too: pass file.path and any
changed_files values through the same sanitizer (wrapUntrusted) before
concatenating into prompts (e.g., replace direct uses of file.path and
changed_files with wrapUntrusted(file.path) and
wrapUntrusted(changed_filesItem)) to ensure filenames/changed file lists cannot
inject instructions outside the protected regions.
In `@src/lib/checks/cache.ts`:
- Around line 27-34: The parser currently finds the first MANIFEST_OPEN and can
pick up injected markers earlier in the text; update parseCacheManifest to
prefer the manifest appended at the end by locating the last occurrence of
MANIFEST_OPEN (use text.lastIndexOf(MANIFEST_OPEN)) and the last occurrence of
MANIFEST_CLOSE (use text.lastIndexOf(MANIFEST_CLOSE)), verify the close index is
after the open index, then compute after = open + MANIFEST_OPEN.length and slice
the body from after..end and trim as before; this ensures the trailing footer
manifest (not an earlier injected marker) is parsed.
In `@src/routes/webhooks/vcs/github/pull-request.post.ts`:
- Around line 70-75: The repo-name equality check is case-sensitive and can
falsely ignore valid webhooks; update the comparison that uses fullName and
expectedFullName to normalize casing (e.g., .toLowerCase() or locale-insensitive
lowercasing) and trim whitespace on both values before comparing, and keep the
existing logger call but log the normalized values (still using fullName and
expectedFullName identifiers to find the code). Ensure you compare
normalizedFullName === normalizedExpectedFullName and return the same ignored
response only when they truly differ.
In `@src/workflows/prompts-step.ts`:
- Around line 101-125: The lexical path check using path.relative +
fs.stat/fs.readFile can be bypassed by symlinks; update the loader (symbols:
repoRoot, abs, rel, MAX_PROMPT_BYTES, fs.stat, fs.readFile) to canonicalize and
validate real paths: call fs.realpath on both repoRoot and the candidate abs and
ensure the real abs starts with the real repoRoot, and use fs.lstat on abs (at
least the final component) to reject if it is a symlink before reading; if lstat
indicates a symlink or the realpath check fails, throw an error similar to the
existing messages.
In `@src/workflows/review.ts`:
- Around line 464-484: The whole-PR branch in src/workflows/review.ts ignores
requested "file_content" even though computeBundleRequest fetched it; update the
whole_pr branch in the else block to honor data.includes("file_content") by
adding the file contents from bundle into the request payload (similar to how
changed_files is handled). Specifically, when data.includes("file_content") set
base["file_content"] to the file content structure produced by
computeBundleRequest (e.g., derived from bundle.files entries or whatever bundle
property holds fetched contents) so whole-PR reviews receive the same file
content input as ai_review mode.
- Around line 789-817: The current createReviewStep call (inside the try that
computes result from findingsToComments) can throw and override the overall
check outcome; separate review publication from check completion by moving the
createReviewStep invocation out of the main result-producing try/catch (or wrap
it in its own try/catch), ensure any errors from createReviewStep are
caught/logged (do not rethrow), and always proceed to persist/complete the Check
Run with the computed result (result, summary, conclusion) even if review
comment publishing fails; references: findingsToComments, checkCfg.comments,
result, createReviewStep, existingComments.
---
Outside diff comments:
In `@SETUP.md`:
- Line 357: Update the stale internal link in SETUP.md: find the text "If
anything stalls, jump to [troubleshooting](`#12-troubleshooting`)." and change the
anchor to point to the current Troubleshooting section (replace
"`#12-troubleshooting`" with the correct anchor, e.g. "`#13-troubleshooting`" or the
exact slug of the "Troubleshooting" header). Ensure the link text remains
"troubleshooting" and the anchor matches the actual header slug for the
Troubleshooting section.
---
Nitpick comments:
In `@docs/GITHUB-APP-SETUP.md`:
- Around line 231-237: Add a language identifier to the fenced code block
containing the URL
"https://<your-vercel-domain>/webhooks/vcs/github/pull-request" so markdownlint
stops warning; change the opening fence from ``` to a language-specific fence
such as ```text (or ```bash if you prefer) to enable proper syntax highlighting
and linting for that code block.
In `@package.json`:
- Line 34: The dependency "ai": "^5.0.0" is out of alphabetical order in
package.json; move the "ai" entry so dependencies are sorted alphabetically (for
example place "ai" before "`@ai-sdk/anthropic`" or after "h3") to restore
consistent ordering, preserving its version string and trailing commas/JSON
syntax while keeping other dependency entries unchanged.
In `@src/adapters/vcs/github.test.ts`:
- Around line 514-543: Add a regression test to ensure the in-hunk backslash
metadata marker ("\ No newline at end of file") is ignored by
parseChangedLineRangesFromPatch: create a new it block in the existing
describe("parseChangedLineRangesFromPatch") tests that passes a patch containing
a hunk with a real "+" addition line followed immediately by the "\ No newline
at end of file" marker and assert that parseChangedLineRangesFromPatch returns
only the expected range for the real added line (and does not treat the
backslash marker as an added line); reference the
parseChangedLineRangesFromPatch function name and the test suite in
github.test.ts when adding this case.
🪄 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: 309faa1f-9ed8-490e-8d0e-406bc9b4a727
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
SETUP.mddocs/GITHUB-APP-SETUP.mddocs/superpowers/plans/2026-05-20-pr-review-extensions-implementation-plan.mddocs/superpowers/specs/2026-05-19-pr-review-extensions-design.mdenv.tspackage.jsonsrc/adapters/vcs/github.test.tssrc/adapters/vcs/github.tssrc/adapters/vcs/gitlab.test.tssrc/adapters/vcs/gitlab.tssrc/adapters/vcs/types.tssrc/lib/check-output.test.tssrc/lib/check-output.tssrc/lib/checks/ai-review.test.tssrc/lib/checks/ai-review.tssrc/lib/checks/cache.test.tssrc/lib/checks/cache.tssrc/lib/checks/complexity.test.tssrc/lib/checks/complexity.tssrc/lib/checks/registry.tssrc/lib/checks/result.test.tssrc/lib/checks/result.tssrc/lib/checks/types.tssrc/lib/dispatch-review.tssrc/lib/dispatch.test.tssrc/lib/pr-context.test.tssrc/lib/pr-context.tssrc/lib/prompts.tssrc/lib/workflow-config.test.tssrc/lib/workflow-config.tssrc/routes/webhooks/vcs/github/pull-request.post.test.tssrc/routes/webhooks/vcs/github/pull-request.post.tssrc/workflows/prompts-step.test.tssrc/workflows/prompts-step.tssrc/workflows/review.test.tssrc/workflows/review.tsworkflow.config.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/checks/cache.ts`:
- Around line 29-33: The manifest parsing uses MANIFEST_OPEN and MANIFEST_CLOSE
but finds the closing marker with lastIndexOf which can match a later `-->`
elsewhere; update the logic in the function in src/lib/checks/cache.ts
(variables: start, after, end and constants MANIFEST_OPEN/MANIFEST_CLOSE) to
locate the closing marker paired with the selected open marker by using a search
for MANIFEST_CLOSE starting at `after` (e.g., indexOf with `after` as the
fromIndex) and then validate `end > start`; adjust any subsequent slicing to use
this `end` so the manifest block is correctly extracted.
In `@src/workflows/prompts-step.ts`:
- Around line 111-132: The tests for loadReviewPrompt need their
node:fs/promises mock updated to include lstat and realpath and adjusted
per-test setup: add lstat and realpath to the vi.mock("node:fs/promises", ...)
return object, reset their jest/vi spies in the loadReviewPrompt beforeEach, and
in the “valid relative path” test stub lstat to return an object whose
isSymbolicLink() returns false and stub realpath to return canonical paths such
that realAbs === realRoot or realAbs startsWith(realRoot + path.sep; use the
same repoRoot value as the test), while the missing/oversize local tests should
either have lstat/realpath throw/reject to exercise the new guards or have them
succeed and let stat/readFile drive the size/missing-file assertions.
🪄 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: 2cb52ef8-8662-47a0-9011-4a86dece5bd4
📒 Files selected for processing (11)
SETUP.mddocs/GITHUB-APP-SETUP.mdpackage.jsonsrc/adapters/vcs/github.test.tssrc/adapters/vcs/github.tssrc/lib/check-output.tssrc/lib/checks/ai-review.tssrc/lib/checks/cache.tssrc/routes/webhooks/vcs/github/pull-request.post.tssrc/workflows/prompts-step.tssrc/workflows/review.ts
Comment on lines
+29
to
+33
| const start = text.lastIndexOf(MANIFEST_OPEN); | ||
| if (start < 0) return null; | ||
| const after = start + MANIFEST_OPEN.length; | ||
| const end = text.lastIndexOf(MANIFEST_CLOSE); | ||
| if (end <= start) return null; |
There was a problem hiding this comment.
Pair the manifest close marker with the selected open marker.
This now assumes the cache block owns the final --> in the whole check output, but src/workflows/review.ts still prefixes the manifest before overflow/unanchored text. Any later --> in a finding or code sample will make parsing fall through to null, so cache reuse silently stops working for that run.
Suggested fix
- const end = text.lastIndexOf(MANIFEST_CLOSE);
- if (end <= start) return null;
+ const end = text.indexOf(MANIFEST_CLOSE, after);
+ if (end < after) return null;🤖 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 `@src/lib/checks/cache.ts` around lines 29 - 33, The manifest parsing uses
MANIFEST_OPEN and MANIFEST_CLOSE but finds the closing marker with lastIndexOf
which can match a later `-->` elsewhere; update the logic in the function in
src/lib/checks/cache.ts (variables: start, after, end and constants
MANIFEST_OPEN/MANIFEST_CLOSE) to locate the closing marker paired with the
selected open marker by using a search for MANIFEST_CLOSE starting at `after`
(e.g., indexOf with `after` as the fromIndex) and then validate `end > start`;
adjust any subsequent slicing to use this `end` so the manifest block is
correctly extracted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/superpowers/plans/2026-05-21-pre-research-wiring.md`:
- Around line 1329-1331: Add a fence language label to the unlabeled code block
containing the text starting "2. **Read pre-research brief (if present)**" so
markdownlint MD040 is satisfied; change the opening triple backticks to include
a language such as text (e.g., ```text) and leave the rest of the block
unchanged, ensuring the `<pre_research_brief>` mention and emphasis are
preserved.
🪄 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: 2dccd68b-408a-4202-879d-7b026451a185
📒 Files selected for processing (3)
docs/superpowers/plans/2026-05-21-pre-research-wiring.mdsrc/lib/checks/ai-review.test.tssrc/workflows/prompts-step.test.ts
Comment on lines
+1329
to
+1331
| ``` | ||
| 2. **Read pre-research brief (if present)** — If the input begins with a \`<pre_research_brief>\` block, treat its contents as additional context. Trust the brief on external/library/API facts but **verify any code-level claims against the actual repo** before depending on them. | ||
| ``` |
There was a problem hiding this comment.
Add a fence language on Line 1329 to satisfy markdownlint MD040.
This code block is currently unlabeled and can trigger docs-lint noise/failures.
Proposed fix
-```
+```text
2. **Read pre-research brief (if present)** — If the input begins with a `<pre_research_brief>` block, treat its contents as additional context. Trust the brief on external/library/API facts but **verify any code-level claims against the actual repo** before depending on them.</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 1329-1329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/superpowers/plans/2026-05-21-pre-research-wiring.md around lines 1329 -
1331, Add a fence language label to the unlabeled code block containing the text
starting "2. Read pre-research brief (if present)" so markdownlint MD040 is
satisfied; change the opening triple backticks to include a language such as
text (e.g., ```text) and leave the rest of the block unchanged, ensuring the
<pre_research_brief> mention and emphasis are preserved.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation