Follow-up: safer container exec, push validation, and test fixes for createDependentPR #1413
Conversation
…Add handler for PR label "I2PR: Update PR" to enqueue a createDependentPR job\n- Extend pull_request webhook payload schema to include label, sender and number\n- Wire new handler into webhook route\n- Define CreateDependentPR job schema in shared package\n- Handle createDependentPR job in worker (stub for now)
…follow-ups\n\n- Worker: add createDependentPR orchestrator and wire it into handler; remove stub and execute shared workflow\n- Shared: migrate DependentPRAgent and createDependentPR workflow to shared so workers can run it\n- Webhook: simplify PR labeled handling by removing redundant installationId missing check (schema guarantees it)\n- Tests: add unit tests for PR label handler enqueuing job and worker routing to orchestrator\n\nKeeps changes scoped to requested feature; no broader refactors.
## Summary - add a Jest test covering the GitHub webhook issues label path for auto-resolve - mock webhook handlers and sign a sample payload with a fixed secret to satisfy signature validation ## Testing - pnpm test __tests__/api/webhook/github.route.test.ts ------ [Codex Task](https://chatgpt.com/codex/tasks/task_b_693b868dca34832a873724c553f9df48)
…sts-for-handleissuelabelautoresolve
## Summary - add a node suite test for the GitHub issue label auto-resolve handler covering success and error paths - mock the job enqueue service and require the Redis URL environment variable in tests - map shared package imports in Jest to support the new test ## Testing - pnpm test:node -- --runTestsByPath __tests__/lib/webhook/label.autoResolveIssue.handler.test.ts ------ [Codex Task](https://chatgpt.com/codex/tasks/task_b_693b8694741c832abea909a54c707e50)
…st-test-for-autoresolveissue-handler
…abel-followup-b1d670eb' into codex/add-jest-test-for-autoresolveissue-handler
…erWithDockerode: accept argv arrays to avoid shell interpolation; keep string support for backward compatibility\n- createDependentPRWorkflow: switch to argv-based git commands; validate push result and fail workflow on push errors; wrap container cleanup in try/catch to avoid masking original errors\n- Orchestrator: pass numeric installationId to Octokit auth\n- Tests: fix handler.autoResolveIssue test types (import Job, use role in messages); add missing REDIS_URL validation test for PR label handler\n- Jest config: remove duplicate moduleNameMapper entry for shared/*
youngchingjui
added
the
AI generated
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements a webhook-based trigger for creating dependent PRs. When a GitHub PR receives the "i2pr: update pr" label, the webhook enqueues a job that orchestrates the dependent PR creation workflow through worker processes, coordinating authentication, repository management, and an AI agent to generate changes. Changes
Sequence Diagram(s)sequenceDiagram
actor GitHub
participant Route as Webhook Route
participant Handler as Label Handler
participant Queue as Job Queue
participant Worker as Worker Process
participant Orchestrator as Orchestrator
participant Neo4j as Neo4j
participant Git as Git/Container
participant Workflow as createDependentPRWorkflow
participant Agent as DependentPRAgent
GitHub->>Route: POST /api/webhook/github<br/>(PR labeled: "i2pr: update pr")
Route->>Route: Verify signature
Route->>Handler: dispatch payload + installationId
Handler->>Handler: Validate REDIS_URL<br/>Extract fields
Handler->>Queue: addJob(createDependentPR,<br/>payload data)
Queue-->>Handler: job enqueued
Handler-->>Route: return {status: "noop"}
Route-->>GitHub: 200 OK
rect rgba(100, 150, 200, 0.2)
Note over Worker,Agent: Async Job Processing
Queue->>Worker: poll createDependentPR job
Worker->>Orchestrator: invoke(jobId, jobData)
Orchestrator->>Neo4j: initialize settings reader
Orchestrator->>Git: load GitHub App key<br/>init Octokit
Orchestrator->>Git: create installation token
Orchestrator->>Git: setup container checkout
Orchestrator->>Workflow: invoke with adapters
Workflow->>Git: fetch PR details, diffs,<br/>comments, reviews
Workflow->>Git: checkout PR head<br/>to dependent branch
Workflow->>Agent: initialize with context
Workflow->>Agent: run agent
Agent->>Agent: apply changes via tools<br/>(files, branches, PR)
Agent-->>Workflow: completion
Workflow->>Git: push dependent branch
Workflow-->>Orchestrator: branch name
Orchestrator->>Worker: return branch info
Worker->>Queue: publish completion status
end
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
__tests__/lib/workers/createDependentPR.worker.test.ts (1)
15-33: Consider strengthening test assertions.The test validates the basic routing but could be more robust:
- Line 18: Using
anytype weakens type safety. Consider using the actualJobtype from BullMQ or a proper test fixture.- Line 31: The test verifies
createDependentPRwas called but doesn't validate the arguments. Consider adding:expect(createDependentPR).toHaveBeenCalledWith("job-123", { repoFullName: "owner/repo", pullNumber: 55, githubLogin: "octocat", githubInstallationId: "123", })- Lines 7-12: The Job entity mock that preserves the actual implementation doesn't appear to serve a purpose. Consider removing it unless there's a specific reason to keep it.
__tests__/apps/workers/handler.autoResolveIssue.test.ts (1)
27-31: Consider using proper message types instead ofas any.The explicit
as conston the role is good for type safety, but theas anycast on line 31 weakens type checking. If the autoResolveIssue orchestrator returns a specific message type (e.g.,ChatCompletionMessageParam[]), consider importing and using that type instead of casting toany.__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts (1)
25-28: Add afterAll to restore original environment.The
beforeEachresetsprocess.envbefore each test, but if tests in this file modify the environment and fail mid-execution, subsequent test suites could inherit the modified state. Consider adding anafterAllhook to ensure cleanup.beforeEach(() => { jest.restoreAllMocks() process.env = { ...originalEnv } }) + + afterAll(() => { + process.env = originalEnv + })__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts (2)
4-6: Consider adding beforeEach to reset mock state.The module-level mock with
mockResolvedValuepersists across tests. IfaddJobis called in one test and assertions in another test check call counts, this could cause flaky tests. Consider adding abeforeEachto clear mock state.jest.mock("shared/services/job", () => ({ addJob: jest.fn().mockResolvedValue("job-id-123"), })) describe("handlePullRequestLabelCreateDependentPR", () => { const installationId = "123456" + + beforeEach(() => { + jest.clearAllMocks() + })
43-52: Consider asserting the specific error message.The test uses generic
toThrow()without verifying the error message. For better test specificity, assert the expected message.await expect( handlePullRequestLabelCreateDependentPR({ payload: makePayload({ repository: { name: "", owner: { login: "" } } }), installationId, }) - ).rejects.toThrow() + ).rejects.toThrow("Missing required fields for createDependentPR job")__tests__/api/webhook/github.route.test.ts (1)
25-30: Consider grouping mocks before imports for clarity.While Jest hoists
jest.mock()calls, placing thehandleIssueLabelAutoResolvemock after its import (line 26 before line 28) can be confusing. Consider moving this mock to join the other mocks at lines 7-24 for consistency.jest.mock("@/lib/webhook/github/handlers/repository/edited.revalidateRepoCache.handler", () => ({ handleRepositoryEditedRevalidate: jest.fn(), })) +jest.mock("@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler", () => ({ + handleIssueLabelAutoResolve: jest.fn(), +})) import { POST } from "@/app/api/webhook/github/route" import { handleIssueLabelAutoResolve } from "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler" - -jest.mock("@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler", () => ({ - handleIssueLabelAutoResolve: jest.fn(), -}))shared/src/usecases/workflows/createDependentPR.ts (2)
144-156: Approve with note on credential handling.The session token embedded in the git remote URL is standard practice for GitHub App authentication. The token is short-lived and scoped to the installation.
Consider adding
--quietflag to suppress any potential credential echoing in logs, though git typically handles this securely:command: [ "git", "remote", "set-url", + "--quiet", "origin", `https://x-access-token:${sessionToken}@github.com/${repoFullName}.git`, ],
222-238: Consider using consistent timestamp formatting.Using
toLocaleString()(lines 225-226, 234-235) produces locale-dependent output that varies based on server configuration. For reproducibility and clarity, consider ISO format:- `Comment ${i + 1} by ${c.user?.login || "unknown"} at ${new Date( - c.created_at || new Date().toISOString() - ).toLocaleString()}\n${c.body}` + `Comment ${i + 1} by ${c.user?.login || "unknown"} at ${ + c.created_at || new Date().toISOString() + }\n${c.body}`However, this is a minor concern since the timestamps are for agent context only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
__tests__/api/webhook/github.route.test.ts(1 hunks)__tests__/apps/workers/handler.autoResolveIssue.test.ts(1 hunks)__tests__/config/jest.config.base.ts(1 hunks)__tests__/config/jest.config.node.ts(1 hunks)__tests__/lib/utils/setupEnv.test.ts(4 hunks)__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts(1 hunks)__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts(1 hunks)__tests__/lib/workers/createDependentPR.worker.test.ts(1 hunks)app/api/webhook/github/route.ts(3 hunks)apps/workers/workflow-workers/src/handler.ts(3 hunks)apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts(1 hunks)lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts(1 hunks)lib/webhook/github/types.ts(2 hunks)shared/src/entities/events/Job.ts(1 hunks)shared/src/lib/agents/DependentPRAgent.ts(1 hunks)shared/src/lib/docker.ts(4 hunks)shared/src/usecases/workflows/createDependentPR.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
__tests__/config/jest.config.node.tsshared/src/entities/events/Job.tslib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.tsshared/src/lib/agents/DependentPRAgent.tslib/webhook/github/types.tsapp/api/webhook/github/route.tsshared/src/lib/docker.ts__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts__tests__/apps/workers/handler.autoResolveIssue.test.tsshared/src/usecases/workflows/createDependentPR.tsapps/workers/workflow-workers/src/orchestrators/createDependentPR.ts__tests__/lib/workers/createDependentPR.worker.test.ts__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts__tests__/api/webhook/github.route.test.tsapps/workers/workflow-workers/src/handler.ts__tests__/config/jest.config.base.ts__tests__/lib/utils/setupEnv.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
📚 Learning: 2025-09-12T01:44:51.070Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: app/api/webhook/github/route.ts:85-90
Timestamp: 2025-09-12T01:44:51.070Z
Learning: In the webhook handlers at app/api/webhook/github/route.ts, runWithInstallationId is intentionally not awaited to allow fire-and-forget background processing of long-running workflows, avoiding webhook timeouts. The team plans to migrate these to proper queues/workers in a separate issue later.
Applied to files:
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.tsapp/api/webhook/github/route.ts__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts__tests__/api/webhook/github.route.test.ts
📚 Learning: 2025-09-10T07:15:30.472Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1231
File: lib/agents/DependentPRAgent.ts:61-61
Timestamp: 2025-09-10T07:15:30.472Z
Learning: In lib/agents/DependentPRAgent.ts, the constructor uses `super({ model: "gpt-5", ...base })` intentionally to allow base.model to override "gpt-5" if needed. GPT-5 should be treated as the default model, not enforced, to maintain flexibility for future use cases.
Applied to files:
shared/src/lib/agents/DependentPRAgent.ts
📚 Learning: 2025-09-11T09:11:14.319Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1214
File: lib/webhook.ts:184-189
Timestamp: 2025-09-11T09:11:14.319Z
Learning: The autoResolveIssue workflow signature is autoResolveIssue(params, ports) where params contains {issue, repository, jobId} and ports contains {auth: AuthReaderPort, settings: SettingsReaderPort, eventBus?: EventBusPort}.
Applied to files:
__tests__/apps/workers/handler.autoResolveIssue.test.ts__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts
📚 Learning: 2025-09-11T09:11:14.319Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1214
File: lib/webhook.ts:184-189
Timestamp: 2025-09-11T09:11:14.319Z
Learning: The autoResolveIssue workflow in the codebase follows clean architecture principles with a two-argument signature: autoResolveIssue(params, ports), where ports contains auth and settings adapters.
Applied to files:
__tests__/apps/workers/handler.autoResolveIssue.test.ts__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts
📚 Learning: 2025-09-04T14:57:04.612Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/code-structure.mdc:0-0
Timestamp: 2025-09-04T14:57:04.612Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mts,mjs} : Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Applied to files:
__tests__/config/jest.config.base.ts
🧬 Code graph analysis (8)
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (3)
lib/webhook/github/types.ts (1)
PullRequestPayload(51-51)shared/src/entities/Queue.ts (1)
WORKFLOW_JOBS_QUEUE(3-3)shared/src/services/job.ts (1)
addJob(10-19)
app/api/webhook/github/route.ts (1)
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
handlePullRequestLabelCreateDependentPR(11-51)
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts (3)
lib/webhook/github/types.ts (1)
PullRequestPayload(51-51)lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
handlePullRequestLabelCreateDependentPR(11-51)shared/src/services/job.ts (1)
addJob(10-19)
apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (6)
apps/workers/workflow-workers/src/helper.ts (2)
publishJobStatus(48-54)getEnvVar(43-46)lib/neo4j/index.ts (1)
neo4jDs(27-33)shared/src/adapters/neo4j/dataSource.ts (1)
createNeo4jDataSource(11-29)shared/src/adapters/neo4j/repositories/SettingsReaderAdapter.ts (1)
makeSettingsReaderAdapter(28-54)shared/src/adapters/ioredis/EventBusAdapter.ts (1)
EventBusAdapter(10-32)shared/src/auth.ts (1)
setAccessToken(11-13)
__tests__/lib/workers/createDependentPR.worker.test.ts (2)
apps/workers/workflow-workers/src/handler.ts (1)
handler(24-86)apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (1)
createDependentPR(38-103)
__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts (3)
lib/webhook/github/types.ts (1)
IssuesPayload(33-33)lib/webhook/github/handlers/issue/label.autoResolveIssue.handler.ts (1)
handleIssueLabelAutoResolve(11-47)shared/src/entities/Queue.ts (1)
WORKFLOW_JOBS_QUEUE(3-3)
__tests__/api/webhook/github.route.test.ts (1)
lib/webhook/github/handlers/issue/label.autoResolveIssue.handler.ts (1)
handleIssueLabelAutoResolve(11-47)
apps/workers/workflow-workers/src/handler.ts (2)
apps/workers/workflow-workers/src/helper.ts (1)
publishJobStatus(48-54)apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (1)
createDependentPR(38-103)
🔇 Additional comments (29)
__tests__/config/jest.config.node.ts (1)
13-17: LGTM! Test pattern extension looks good.The addition of the
appstest pattern aligns with the new worker handler tests introduced in this PR and follows the existing naming conventions.shared/src/entities/events/Job.ts (1)
37-55: LGTM! Schema definition follows established patterns.The
CreateDependentPRJobSchemais well-structured and consistent with existing job schemas in the file. The integration into the discriminated union is correct.__tests__/lib/utils/setupEnv.test.ts (2)
7-26: LGTM! Type definitions improve mock accuracy.The explicit typing for
ExecCallback,ExecException, andExecOptionsmakes the test mock more type-safe and aligns properly with Node'schild_processmodule types.
36-87: LGTM! Mock implementation correctly handles both callback signatures.The mock properly handles both
exec(cmd, callback)andexec(cmd, options, callback)signatures through the union type and type guard. The{} as ChildProcesscast is acceptable for test purposes since the actual ChildProcess instance behavior isn't exercised.__tests__/apps/workers/handler.autoResolveIssue.test.ts (1)
26-58: LGTM! Test comprehensively validates the autoResolveIssue flow.The test properly verifies:
- Orchestrator invocation with correct parameters
- Status publishing at key workflow stages
- Correct transformation of messages to return value
The test structure is solid and provides good coverage of the handler's behavior.
__tests__/config/jest.config.base.ts (1)
11-16: LGTM! Path aliases refined for shared module access.The changes introduce:
- A new
shared/*alias (line 12) alongside the existing@shared/*(line 11) — supporting both import styles- A composite pattern (lines 14-15) for specific subdirectories within
shared/src- A generic
@/*fallback (line 16)This approach provides flexible import paths while maintaining specific routing for shared modules. The dual aliases (
@sharedandshared) appear intentional to support different import conventions across the codebase.lib/webhook/github/types.ts (1)
35-50: LGTM! Payload schema appropriately extended for PR label events.The additions support the new "labeled" PR webhook flow:
- Line 37: Top-level
numberfor PR number in label events- Line 38:
labelobject for the applied label- Line 39:
senderfor the user who triggered the event- Line 43: Nested
numberinpull_requestfor other event typesAll fields are properly optional, maintaining backward compatibility with existing PR event handlers.
apps/workers/workflow-workers/src/handler.ts (1)
70-75: LGTM! Handler integration follows established patterns.The
createDependentPRcase is well-integrated:
- Consistent with existing job handling patterns
- Appropriate status messages at start and completion
- Returns the created branch name for tracking
- Error handling covered by the outer try-catch block
__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts (3)
30-56: LGTM!The test properly verifies the job enqueue behavior with correct queue name, job payload structure, and Redis URL passthrough.
58-67: LGTM!Correctly tests the REDIS_URL validation path.
69-86: LGTM!Good coverage for the missing required fields validation path. The type assertion is appropriate here to simulate malformed webhook payloads.
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts (2)
24-41: LGTM!The test correctly verifies the job enqueue with proper payload structure including
repoFullName,pullNumber,githubLogin, andgithubInstallationId.
54-66: LGTM with minor note.The test correctly validates the REDIS_URL check. The conditional restoration on line 65 is acceptable, though using a
beforeEach/afterEachpattern for environment management would be more consistent with the other test file.shared/src/lib/docker.ts (2)
129-136: LGTM! Good security improvement.The updated documentation clearly explains the two execution modes: shell-wrapped for string commands (backward compatibility) and direct argv for array commands (avoiding shell interpolation). This enables safer execution of user-controlled inputs.
179-188: LGTM!The implementation correctly distinguishes between array and string commands. When an array is provided, it's passed directly to Docker exec, preventing shell interpolation attacks on user-controlled inputs like branch names or refs.
app/api/webhook/github/route.ts (1)
171-189: LGTM!The new
labeledcase follows the established pattern from theissuesevent handler. The case-insensitive label matching and installationId extraction are consistent with other handlers. Awaiting the handler is appropriate since it only enqueues a job (fast operation), unlike the fire-and-forget pattern noted in learnings for long-running workflows.lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
1-51: LGTM!The handler follows the established pattern from
handleIssueLabelAutoResolve. Key strengths:
- Proper environment validation for REDIS_URL
- Defensive extraction of
pullNumberfrom multiple payload locations- Clear error message listing all missing fields
- Correct job data structure matching the orchestrator's
CreateDependentPRJobDatatypeshared/src/lib/agents/DependentPRAgent.ts (3)
86-100: LGTM!Good defensive programming with try/catch around remote tool attachment. The warning log provides visibility into failures without breaking the agent initialization. The schema validation ensures repository metadata is well-formed before use.
61-61: Noted: GPT-5 default model is intentional.Per learnings, the
super({ model: "gpt-5", ...base })pattern allowsbase.modelto override the default when needed, maintaining flexibility.
68-70: The fire-and-forgetsetDeveloperPrompt()pattern is safe in this codebase. Although the method is async and not awaited, the agent is not used immediately after construction—there is substantial processing (data formatting, context building) between instantiation and theawait agent.runWithFunctions()call, providing ample time for the prompt to be added to the queue. This same pattern is consistently used across other agents (ThinkerAgent, PlanAndCodeAgent, MergeConflictResolverAgent), indicating it is an intentional design choice.Likely an incorrect or invalid review comment.
apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (3)
14-29: LGTM!The in-file
userRepoimplementation is minimal but sufficient for the settings lookup. The Cypher query is properly parameterized to prevent injection attacks.
67-84: LGTM!The GitHub App authentication flow is correctly implemented:
Number(githubInstallationId)satisfies@octokit/auth-app's type requirements (PR objective)- Token validation is thorough before use
- Global
setAccessTokenusage is noted as addressed separately in #1409 per PR objectives
38-103: Error handling is already implemented at the handler level. The handler function wraps the orchestrator calls in a try-catch block that publishes failure status viapublishJobStatus(job.id,Failed: ${msg})before re-throwing the error. No changes needed.Likely an incorrect or invalid review comment.
__tests__/api/webhook/github.route.test.ts (1)
45-80: LGTM! Solid test coverage for webhook routing.The test correctly validates:
- Signature verification with HMAC-SHA256
- Proper routing of labeled issue events to the auto-resolve handler
- Correct argument passing (installationId as string, payload fields)
The mock request setup and assertions are well-structured.
shared/src/usecases/workflows/createDependentPR.ts (5)
36-46: LGTM!Clean interface definitions with appropriate optional markers.
58-65: LGTM!Proper validation of API key result with both success check and value presence before proceeding.
92-108: LGTM!Good use of
Promise.allfor parallel fetching of independent PR artifacts.
320-330: LGTM!Good error handling pattern: cleanup failures are logged as warnings without masking the original error, as noted in the PR objectives.
36-46: No changes needed. The interfaces do not need to be exported. The orchestrator that calls this workflow function constructs the parameter objects inline without importing the type definitions, and no other external consumers reference these types. The function signature itself adequately documents the expected parameter shapes.Likely an incorrect or invalid review comment.
apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts
Outdated
Show resolved
Hide resolved
| import { getEnvVar, publishJobStatus } from "../helper" | ||
|
|
||
| // Minimal user repository implementation for SettingsReaderAdapter | ||
| const userRepo = { |
There was a problem hiding this comment.
Wait, this is crazy that this is here. Why isn't this embedded directly within makeSettingsReaderAdapter?
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
shared/src/usecases/workflows/createDependentPR.ts (1)
304-312: Fix silent skip when sessionToken is missing.If
sessionTokenis falsy, the branch push is silently skipped, but the workflow still reports success and returns the branch name. This could mislead callers and cause downstream failures when attempting to create a PR from a non-existent remote branch.Apply this diff to fail explicitly when the token is missing:
if (sessionToken) { const { exitCode } = await execInContainerWithDockerode({ name: containerName, command: ["git", "push", "-u", "origin", dependentBranch], }) if (exitCode !== 0) { throw new Error(`Failed to push branch ${dependentBranch}`) } + } else { + throw new Error( + `Cannot push branch ${dependentBranch}: no session token available` + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/usecases/workflows/createDependentPR.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
shared/src/usecases/workflows/createDependentPR.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/git-management.mdc:0-0
Timestamp: 2025-09-04T14:58:54.915Z
Learning: Can create and update pull requests through GitHub API when requested
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
📚 Learning: 2025-09-04T14:57:28.594Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/container-use.mdc:0-0
Timestamp: 2025-09-04T14:57:28.594Z
Learning: When providing the "checkout command for human", ensure it switches to a branch that tracks the container-use remote and note that it will not pull changes.
Applied to files:
shared/src/usecases/workflows/createDependentPR.ts
🧬 Code graph analysis (1)
shared/src/usecases/workflows/createDependentPR.ts (6)
shared/src/ports/repositories/settings.reader.ts (1)
SettingsReaderPort(9-19)shared/src/ports/events/eventBus.ts (1)
EventBusPort(8-13)shared/src/ports/events/publisher.ts (1)
createWorkflowEventPublisher(8-141)shared/src/lib/docker.ts (1)
execInContainerWithDockerode(138-226)shared/src/lib/agents/DependentPRAgent.ts (1)
DependentPRAgent(49-102)shared/src/lib/langfuse.ts (1)
langfuse(3-6)
🔇 Additional comments (2)
shared/src/usecases/workflows/createDependentPR.ts (2)
1-46: LGTM!The imports and interface definitions are clean and well-structured.
182-189: LGTM! Exit code check properly implemented.The
git pullexit code is now correctly validated, and the workflow throws an error on failure. This addresses the previous review concern about divergent branches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
shared/src/usecases/workflows/createDependentPR.ts (3)
163-166: Add exit code check for git fetch.The
git fetchcommand could fail (network issues, invalid ref, etc.), but the exit code isn't checked. This could leave the workflow with stale remote refs or proceed without the target branch.Apply this diff to check the fetch result:
- await execInContainerWithDockerode({ + const { exitCode: fetchExit } = await execInContainerWithDockerode({ name: containerName, command: ["git", "fetch", "origin", headRef], }) + if (fetchExit !== 0) { + throw new Error(`Failed to fetch branch ${headRef} from origin`) + }
173-176: Add exit code check for git checkout -b.The
git checkout -bcommand could fail (permissions, disk space, etc.), but the exit code isn't checked. Continuing without the correct branch would corrupt the workflow state.Apply this diff:
- await execInContainerWithDockerode({ + const { exitCode: checkoutExit } = await execInContainerWithDockerode({ name: containerName, command: ["git", "checkout", "-b", headRef, `origin/${headRef}`], }) + if (checkoutExit !== 0) { + throw new Error(`Failed to create and checkout branch ${headRef}`) + }
178-181: Add exit code check for git checkout.The
git checkoutcommand could fail, leaving the workflow on the wrong branch. This would cause subsequent commits and pushes to target an incorrect branch.Apply this diff:
- await execInContainerWithDockerode({ + const { exitCode: checkoutExit } = await execInContainerWithDockerode({ name: containerName, command: ["git", "checkout", "-q", headRef], }) + if (checkoutExit !== 0) { + throw new Error(`Failed to checkout branch ${headRef}`) + }
🧹 Nitpick comments (1)
shared/src/usecases/workflows/createDependentPR.ts (1)
226-258: Simplify fallback date pattern.The pattern
new Date(timestamp || new Date().toISOString())is redundant. When the timestamp is falsy,new Date().toISOString()creates a string that's immediately passed back tonew Date().Consider simplifying to:
-`Comment ${i + 1} by ${c.user?.login || "unknown"} at ${new Date( - c.created_at || new Date().toISOString() -).toLocaleString()}\n${c.body}` +`Comment ${i + 1} by ${c.user?.login || "unknown"} at ${ + c.created_at ? new Date(c.created_at).toLocaleString() : new Date().toLocaleString() +}\n${c.body}`Apply the same pattern to reviews (lines 238-240) and review threads (lines 247-249).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/usecases/workflows/createDependentPR.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
shared/src/usecases/workflows/createDependentPR.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/git-management.mdc:0-0
Timestamp: 2025-09-04T14:58:54.915Z
Learning: Can create and update pull requests through GitHub API when requested
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
📚 Learning: 2025-09-11T09:11:14.319Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1214
File: lib/webhook.ts:184-189
Timestamp: 2025-09-11T09:11:14.319Z
Learning: The autoResolveIssue workflow signature is autoResolveIssue(params, ports) where params contains {issue, repository, jobId} and ports contains {auth: AuthReaderPort, settings: SettingsReaderPort, eventBus?: EventBusPort}.
Applied to files:
shared/src/usecases/workflows/createDependentPR.ts
📚 Learning: 2025-09-04T14:57:28.594Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/container-use.mdc:0-0
Timestamp: 2025-09-04T14:57:28.594Z
Learning: When providing the "checkout command for human", ensure it switches to a branch that tracks the container-use remote and note that it will not pull changes.
Applied to files:
shared/src/usecases/workflows/createDependentPR.ts
📚 Learning: 2025-09-04T14:57:28.594Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/container-use.mdc:0-0
Timestamp: 2025-09-04T14:57:28.594Z
Learning: Always inform the user how to view their changes using a 'git checkout' command that does not include container-use/remote.
Applied to files:
shared/src/usecases/workflows/createDependentPR.ts
🔇 Additional comments (4)
shared/src/usecases/workflows/createDependentPR.ts (4)
1-46: LGTM: Clean imports and type definitions.The import organization is logical, and the type definitions for
CreateDependentPRParamsandPortsare clear and well-structured.
48-125: LGTM: Well-structured initialization with efficient parallel operations.The initialization logic is clean and follows good practices:
- Proper dependency injection via ports pattern
- Early validation of API key with clear error messages
- Efficient parallel fetching of PR artifacts using
Promise.all- Proper workflow event emission throughout
299-317: LGTM: Proper push validation and error handling.The push logic correctly:
- Checks the exit code and throws on failure
- Emits a clear warning when sessionToken is unavailable
- Prevents silent failures
324-343: LGTM: Robust error handling and safe cleanup.The error handling is well-structured:
- Proper workflow state transitions on error
- Cleanup in
finallyblock ensures execution regardless of outcome- Cleanup errors are caught and logged without masking the original error
This implementation follows the PR objective for "cleanup safety."
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 19
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Attach prompt with reasoning usage enabled by Responses API | ||
| this.setDeveloperPrompt(DEVELOPER_PROMPT).catch((error) => { | ||
| console.error("Error initializing DependentPRAgent system prompt:", error) | ||
| }) |
There was a problem hiding this comment.
Race condition in developer prompt initialization
The DependentPRAgent constructor calls setDeveloperPrompt() without awaiting it. This async method internally awaits trackInput() (a Neo4j operation) before pushing to inputQueue. When the workflow later calls await agent.addInput() for the user message, both operations race to complete their Neo4j calls. Whichever finishes first pushes to the queue first. If the user message wins the race, it appears before the developer prompt in inputQueue, causing the LLM to receive messages in the wrong order and potentially behave incorrectly.
Additional Locations (1)
## Summary
Implement webhook handling for PR labels to trigger the createDependentPR workflow when PRs are labeled with "I2PR: Update PR".
## Features Implemented
### 🔗 Webhook Handler Foundation
- **New Handler**: `handlePullRequestLabelCreateDependentPR` enqueues createDependentPR jobs
- **Enhanced Schema**: Extended `PullRequestPayloadSchema` with `label`, `number`, and `sender` fields
- **Route Integration**: Added PR labeled event routing in webhook endpoint
### 🎯 How It Works
1. **GitHub Event**: PR gets labeled with "I2PR: Update PR"
2. **Webhook Processing**: Validates payload and extracts job data
3. **Job Queuing**: Enqueues `createDependentPR` job with:
- `repoFullName` (e.g., "owner/repo")
- `pullNumber` (PR number)
- `githubLogin` (who applied the label)
- `githubInstallationId` (for GitHub App auth)
4. **Worker Processing**: (Next PR) Worker will create dependent branch/PR
### 🧪 Test Coverage
- [x] Handler validates required fields and environment
- [x] Handler enqueues jobs with correct payload structure
- [x] Webhook route correctly routes labeled events
- [x] Integration test covers full webhook → handler flow
## Technical Details
### New Files
```
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts
__tests__/api/webhook/github.route.test.ts
```
### Modified Files
```
app/api/webhook/github/route.ts - Added labeled event routing
lib/webhook/github/types.ts - Extended PullRequestPayloadSchema
```
### Example Usage
When a PR is labeled "I2PR: Update PR", this triggers:
```typescript
// Enqueues Redis job:
{
name: "createDependentPR",
data: {
repoFullName: "owner/repo",
pullNumber: 42,
githubLogin: "developer",
githubInstallationId: "12345"
}
}
```
## Context & Sequence
This PR is part of breaking down the large PR #1413 into manageable pieces:
1. ✅ **PR #1437**: Test infrastructure fixes
2. ✅ **PR #1438**: Container security enhancements
3. ✅ **This PR**: Webhook handler foundation
4. 🔄 **Next**: Worker infrastructure (Redis/Neo4j setup)
5. 🔄 **Final**: CreateDependentPR core workflow
## Testing
```bash
pnpm test __tests__/lib/webhook/handlers/pullRequest/
pnpm test __tests__/api/webhook/github.route.test.ts
```
Addresses the webhook handling requirements from **Issue #1399**.
🤖 Generated with [Claude Code](https://claude.ai/code)
---
Update: Adjust handler to no-op per review feedback
- Addressed RLC-001 and RLC-002: The worker does not yet recognize a `createDependentPR` job name. To keep this PR scoped to webhook routing and avoid enqueueing unknown jobs, the handler now performs a no-op.
- Changes:
- `handlePullRequestLabelCreateDependentPR` no longer enqueues a job. It validates required fields, logs a clear message indicating the webhook was received, and returns a small `{ status: "noop", ... }` result.
- Updated unit tests to reflect the no-op behavior (removed Redis/job enqueue expectations; assert logging/return value instead).
- Route tests remain the same (they mock the handler and only assert routing/invocation).
Rationale
- This keeps the boundary clean and avoids enqueueing jobs that would fail in the worker until the job schema/handler support is added in a follow-up PR.
Verification
- Jest tests for the updated handler and webhook route pass locally:
- `__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts`
- `__tests__/api/webhook/github.route.test.ts`
Next steps
- Follow-up PR to register the `createDependentPR` job type in the worker and wire up the actual enqueue logic.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* Pull requests can now be automatically processed when labeled with "i2pr: update pr"
* Enhanced webhook system to recognize and route pull request label events with proper signature verification
* **Tests**
* Added comprehensive unit tests for webhook routing and signature verification
* Added tests for pull request label event handling, including validation of required payload fields
<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shared/src/lib/agents/DependentPRAgent.ts (1)
67-70: Race condition:setDeveloperPrompt()is not awaited.The async
setDeveloperPrompt()method is called without await, which can cause a race condition where subsequentaddInput()calls may complete before the developer prompt is pushed to the input queue, resulting in incorrect message ordering for the LLM.Consider making initialization synchronous or ensuring the prompt is set before any user input is added.
🧹 Nitpick comments (6)
__tests__/api/webhook/github.route.test.ts (4)
7-43: Consider grouping all mocks before imports.The mock for
handleIssueLabelAutoResolve(lines 38-43) appears after importing the function (line 35). While Jest hoisting makes this work correctly, grouping alljest.mock()calls before any imports improves readability and follows common convention.🔎 Suggested reorg
Move lines 38-43 to appear before line 34:
+jest.mock( + "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler", + () => ({ + handleIssueLabelAutoResolve: jest.fn(), + }) +) jest.mock( "@/lib/webhook/github/handlers/repository/edited.revalidateRepoCache.handler", () => ({ handleRepositoryEditedRevalidate: jest.fn(), }) ) import { POST } from "@/app/api/webhook/github/route" import { handleIssueLabelAutoResolve } from "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler" import { handlePullRequestLabelCreateDependentPR } from "@/lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler" -jest.mock( - "@/lib/webhook/github/handlers/issue/label.autoResolveIssue.handler", - () => ({ - handleIssueLabelAutoResolve: jest.fn(), - }) -)
58-96: Strengthen routing verification by asserting other handlers weren't called.The test verifies that
handleIssueLabelAutoResolveis called, but doesn't confirm that other mocked handlers (e.g.,handlePullRequestLabelCreateDependentPR,handleIssueLabelResolve) remained uncalled. Adding these assertions would catch routing bugs where the wrong handler is invoked or multiple handlers fire unexpectedly.🔎 Example assertions to add
After line 95, add:
expect(handlePullRequestLabelCreateDependentPR).not.toHaveBeenCalled() expect(handleIssueLabelResolve).not.toHaveBeenCalled() // Add for other mocked handlers as neededNote: You'll need to import the mocked handlers or access them via
jest.mocked().
98-136: Strengthen routing verification by asserting other handlers weren't called.Similar to the first test, verifying that
handleIssueLabelAutoResolveand other mocked handlers weren't invoked would strengthen the test's ability to detect routing logic errors.🔎 Example assertions to add
After line 135, add:
expect(handleIssueLabelAutoResolve).not.toHaveBeenCalled() expect(handleIssueLabelResolve).not.toHaveBeenCalled() // Add for other mocked handlers as needed
45-136: Consider adding negative test cases for comprehensive coverage.The current tests effectively verify happy-path routing logic. To further improve robustness, consider adding test cases for:
- Invalid webhook signatures
- Missing required fields (e.g., installation ID, repository info)
- Unexpected event types
- Malformed payloads
These cases would help ensure the route handles errors gracefully and provides clear failure signals.
__tests__/tsconfig.json (1)
17-18: Consider consolidating duplicate path aliases.Both
@/shared/*and@shared/*map to the same location. While this may support a gradual migration, maintaining both aliases long-term can lead to inconsistent import styles across the codebase.If both are needed temporarily for migration, consider documenting which alias is preferred and planning to deprecate one in a future PR.
shared/tsconfig.json (1)
11-12: Duplicate alias pattern.Similar to the test configuration, both
@shared/*and@/shared/*resolve to the same location. This duplication appears across multiple tsconfig files in this PR.Consider standardizing on a single alias pattern repository-wide for consistency and easier maintenance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
__tests__/api/webhook/github.route.test.ts__tests__/apps/workers/handler.autoResolveIssue.test.ts__tests__/config/jest.config.base.ts__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts__tests__/lib/workers/createDependentPR.worker.test.ts__tests__/tsconfig.jsonapps/workers/workflow-workers/src/handler.tslib/fetch/github/issues.tslib/github/issues.tslib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.tslib/webhook/github/types.tsshared/src/lib/agents/DependentPRAgent.tsshared/src/lib/github/content.tsshared/src/lib/github/index.tsshared/src/lib/github/pullRequests.tsshared/src/lib/tools/CreatePRTool.tsshared/src/services/job.tsshared/src/services/queue.tsshared/src/usecases/workflows/createDependentPR.tsshared/tsconfig.jsontsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/lib/workers/createDependentPR.worker.test.ts
- lib/webhook/github/types.ts
- tests/apps/workers/handler.autoResolveIssue.test.ts
- shared/src/usecases/workflows/createDependentPR.ts
- tests/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts
- lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
shared/src/services/queue.ts__tests__/api/webhook/github.route.test.tslib/github/issues.tsshared/src/lib/github/pullRequests.tsshared/src/lib/tools/CreatePRTool.tsapps/workers/workflow-workers/src/handler.tsshared/src/services/job.ts__tests__/config/jest.config.base.tsshared/src/lib/github/index.tsshared/src/lib/github/content.tslib/fetch/github/issues.tsshared/src/lib/agents/DependentPRAgent.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: app/api/webhook/github/route.ts:85-90
Timestamp: 2025-09-12T01:44:51.070Z
Learning: In the webhook handlers at app/api/webhook/github/route.ts, runWithInstallationId is intentionally not awaited to allow fire-and-forget background processing of long-running workflows, avoiding webhook timeouts. The team plans to migrate these to proper queues/workers in a separate issue later.
📚 Learning: 2025-09-04T14:57:04.612Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/code-structure.mdc:0-0
Timestamp: 2025-09-04T14:57:04.612Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mts,mjs} : Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Applied to files:
tsconfig.jsonshared/tsconfig.json__tests__/tsconfig.jsonshared/src/lib/tools/CreatePRTool.ts__tests__/config/jest.config.base.tsshared/src/lib/github/index.ts
📚 Learning: 2025-09-12T01:44:51.070Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: app/api/webhook/github/route.ts:85-90
Timestamp: 2025-09-12T01:44:51.070Z
Learning: In the webhook handlers at app/api/webhook/github/route.ts, runWithInstallationId is intentionally not awaited to allow fire-and-forget background processing of long-running workflows, avoiding webhook timeouts. The team plans to migrate these to proper queues/workers in a separate issue later.
Applied to files:
__tests__/api/webhook/github.route.test.ts
📚 Learning: 2025-09-04T14:58:54.915Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/git-management.mdc:0-0
Timestamp: 2025-09-04T14:58:54.915Z
Learning: Can create and update pull requests through GitHub API when requested
Applied to files:
shared/src/lib/tools/CreatePRTool.ts
📚 Learning: 2025-09-10T07:15:30.472Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1231
File: lib/agents/DependentPRAgent.ts:61-61
Timestamp: 2025-09-10T07:15:30.472Z
Learning: In lib/agents/DependentPRAgent.ts, the constructor uses `super({ model: "gpt-5", ...base })` intentionally to allow base.model to override "gpt-5" if needed. GPT-5 should be treated as the default model, not enforced, to maintain flexibility for future use cases.
Applied to files:
shared/src/lib/agents/DependentPRAgent.ts
🧬 Code graph analysis (3)
__tests__/api/webhook/github.route.test.ts (2)
lib/webhook/github/handlers/issue/label.autoResolveIssue.handler.ts (1)
handleIssueLabelAutoResolve(11-47)lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
handlePullRequestLabelCreateDependentPR(10-62)
apps/workers/workflow-workers/src/handler.ts (2)
apps/workers/workflow-workers/src/helper.ts (1)
publishJobStatus(49-55)apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (1)
createDependentPR(39-108)
shared/src/lib/agents/DependentPRAgent.ts (2)
shared/src/lib/tools/CreatePRTool.ts (1)
createCreatePRTool(74-85)lib/agents/DependentPRAgent.ts (1)
DependentPRAgent(58-121)
🔇 Additional comments (17)
lib/github/issues.ts (1)
136-136: LGTM: Cleaner token extraction with nullish coalescing.The change from a questionable cast to the nullish coalescing operator (
?? null) is a clear improvement. It explicitly normalizesundefinedtonull, yielding a predictablestring | nulltype, and is more idiomatic TypeScript.lib/fetch/github/issues.ts (1)
23-23: LGTM: Consistent token handling improvement.This change mirrors the improvement in
lib/github/issues.ts, replacing the redundant cast with nullish coalescing for clearer, more consistent token extraction across the codebase.shared/src/lib/agents/DependentPRAgent.ts (5)
1-13: LGTM!Imports are organized and follow the guideline to import directly from original module paths rather than barrel files.
15-35: LGTM!The developer prompt is well-structured with clear objectives, operating principles, and required end state that align with the dependent PR workflow use case.
37-47: LGTM!Interface is well-typed with helpful JSDoc comments explaining the purpose of each optional parameter.
72-100: LGTM!Tool attachment logic is well-structured:
- Core workspace tools are always attached
- Container exec tool is conditionally added based on environment type
- Remote tools are properly guarded with try-catch and credential checks
- Clear separation between tools requiring sessionToken (SyncBranchTool) and those using app-level auth (CreatePRTool)
102-104: LGTM!Providing both named and default exports allows flexibility in how consumers import the class.
apps/workers/workflow-workers/src/handler.ts (2)
23-29: LGTM! Generic job interface decouples handler from BullMQ.The new
JobInput<D>interface and generic handler signature provide a clean abstraction that decouples the handler from the BullMQ-specificJobtype, improving testability and flexibility.
75-80: LGTM! New job case follows established patterns and is properly typed.The
createDependentPRcase is well-integrated:
JobEventSchemainshared/src/entities/events/Job.tsincludesCreateDependentPRJobSchemawith the correct discriminator literal"createDependentPR"CreateDependentPRJobDatatype properly defines all required fields:repoFullName,pullNumber,githubLogin, andgithubInstallationId, matching the schema definition- Follows the same status publishing pattern as existing cases
- Correctly invokes the orchestrator with job ID and data
- Returns the created branch name as expected
shared/src/lib/tools/CreatePRTool.ts (1)
7-9: LGTM! Import path migration to shared aliases.The import paths have been updated to use the shared
@/shared/lib/...aliases, aligning with the PR's goal of migrating to shared path conventions. All imports resolve correctly and follow best practices by importing from specific module files rather than barrel files to maintain clear dependency graphs. The logic remains unchanged.shared/src/lib/github/pullRequests.ts (1)
1-1: LGTM! Import path migration to shared aliases.The import paths have been successfully migrated to the shared
@/shared/lib/...and@/shared/utils/...aliases. All imported modules and their exports exist at the aliased locations. No runtime behavior changes.tsconfig.json (1)
45-45: Good practice: excluding the shared package source.Adding
shared/**to the exclude list prevents TypeScript from processing the shared package files directly when they're also accessible via path aliases. This avoids duplicate declarations and potential compilation issues.shared/src/services/job.ts (1)
3-5: Import migrations are consistent.All three imports have been updated to use the
@/shared/*alias pattern. TheaddJobfunction signature and logic remain unchanged.shared/src/services/queue.ts (1)
3-4: Import path updates are correct and resolve properly.Both target modules (
shared/src/adapters/ioredis/client.tsandshared/src/entities/Queue.ts) exist and contain actual implementations rather than re-exports, conforming to the barrel file guidelines. The imports are valid and consistent with the@/shared/*alias pattern.__tests__/config/jest.config.base.ts (2)
21-26: Transform configuration is appropriate.The
transformandtransformIgnorePatternsconfiguration correctly handles TypeScript file processing and ES module transformation for the three packages that require it:shared(workspace dependency),@octokit/*(direct dependencies), anduniversal-user-agent(transitive dependency of @octokit packages).
6-19: Granular module mappings improve clarity.The refactored
moduleNameMapperadds explicit, granular mappings for specific import aliases before the catch-all@/(.*)$pattern. This order is correct—more specific patterns must precede the catch-all to match correctly.Verify that all test files resolve imports correctly with the new configuration by running the test suite.
shared/src/lib/github/index.ts (1)
10-11: Import paths are correctly configured.The updated imports from
@/shared/authand@/shared/lib/types/githubare properly resolved byshared/tsconfig.jsonand the target modules exist. No old import patterns remain in the codebase.
| octokit.rest.repos.getBranch({ | ||
| owner, | ||
| repo, | ||
| branch, | ||
| }) |
There was a problem hiding this comment.
Critical: Missing await breaks branch existence check.
The API call to octokit.rest.repos.getBranch returns a Promise but is not awaited. This causes the function to immediately return true (line 186) without waiting for the API call to complete, and errors won't be caught by the catch block. The function will always return true regardless of whether the branch exists.
🔎 Proposed fix
- octokit.rest.repos.getBranch({
+ await octokit.rest.repos.getBranch({
owner,
repo,
branch,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| octokit.rest.repos.getBranch({ | |
| owner, | |
| repo, | |
| branch, | |
| }) | |
| await octokit.rest.repos.getBranch({ | |
| owner, | |
| repo, | |
| branch, | |
| }) |
🤖 Prompt for AI Agents
In shared/src/lib/github/content.ts around lines 180 to 184, the call to
octokit.rest.repos.getBranch is not awaited so the function returns true
immediately and exceptions aren't caught; update the code to await the API call
(await octokit.rest.repos.getBranch(...)) and only return true after the awaited
call completes successfully, allowing the existing try/catch to catch errors and
return false on failure.
…p-b1d670eb-followup-926bdc7d
## Summary
Fix critical command injection vulnerabilities in container execution by adding support for safe array commands and updating vulnerable code patterns.
## Security Impact
**CRITICAL**: The previous implementation allowed command injection through user-controlled input like branch names, potentially leading to:
- Arbitrary code execution in containers (running as root)
- Data exfiltration from Neo4j, Redis, or file system
- Container escape or privilege escalation
## What Changed
### 🔒 Enhanced Container Execution Security
- **execInContainerWithDockerode** now accepts both strings and arrays
- Array commands bypass shell interpolation completely (injection-proof)
- Added comprehensive security documentation with examples
### 🛠️ Fixed Critical Vulnerability in Branch Tool
**Before (vulnerable)**:
```typescript
await exec(`git checkout ${branch}`) // If branch = "main; rm -rf /" → RCE!
```
**After (secure)**:
```typescript
await execInContainerWithDockerode({
name: container,
command: ["git", "checkout", branch] // Safe: no shell interpolation
})
```
### 📚 Security Guidelines Added
- Clear documentation of safe vs unsafe patterns
- Examples showing injection attack vectors
- Migration path for existing code
## Test Plan
- [x] All existing docker tests pass
- [x] New tests verify array command functionality
- [x] Branch tool updated to use secure pattern
- [x] Backwards compatibility maintained for existing string commands
## Migration Recommendation
This PR provides the **foundation for security**. Follow-up PRs should:
1. **Audit all execInContainerWithDockerode calls** for user input
2. **Migrate high-risk patterns** (git operations, file paths, etc.) to array form
3. **Add input validation** for branch names and other user-controlled data
## Context
This addresses the command injection vulnerability identified in CodeRabbit reviews of PR #1403/#1413, where `headRef` from GitHub PR data was being interpolated into shell commands.
🤖 Generated with [Claude Code](https://claude.ai/code)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> Strengthens container command execution to prevent injection and updates tooling to use safe patterns.
>
> - `execInContainerWithDockerode` now accepts `string | string[]`; array form executes directly, strings run via `sh -c` for backward compatibility. Added security documentation and examples in JSDoc.
> - Updated `shared/src/lib/tools/Branch.ts` to use array-form git commands for `branch --list`, `checkout -b`, and `checkout -q` inside containers.
> - New tests in `__tests__/lib/docker.test.ts` validate string vs array behavior, proper `Cmd` formation, and error handling for missing/empty container names.
>
> <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8072b2c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Enhanced security of container command execution to prevent shell injection vulnerabilities.
* Improved Git operations reliability in containers through safer command handling.
* **Tests**
* Added comprehensive test coverage for container command execution.
<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
…p-b1d670eb-followup-926bdc7d
…p-b1d670eb-followup-926bdc7d
🚧 DRAFT - This PR has been split into smaller, manageable PRs
This large PR (1212 additions, 41 deletions) has been broken down into smaller, focused PRs for easier review and safer merging.
🔄 Split Status
✅ Completed Smaller PRs
PR Improve test infrastructure and Jest configuration #1437 - Test Infrastructure & Configuration ✅ MERGED
PR Add container security enhancements to prevent command injection #1438 - Container Security Enhancements ⏳ Ready for Review
execInContainerWithDockerodePR Add webhook handler for PR label 'I2PR: Update PR' #1440 - Webhook Handler Foundation ⏳ Ready for Review
🔄 Remaining PRs (To Be Created)
Worker Infrastructure - Coming Next
CreateDependentPR Core Workflow - Final
🔒 Security Issue Created
Issue #1439 - Complete Migration from String to Array Commands
📋 Instructions for Undrafting
This PR should only be undrafted and merged when:
PR Improve test infrastructure and Jest configuration #1437 is merged(DONE)Process:
🎯 Why This Approach?
Original Problem: 1212 line PR touching multiple systems - too large to review safely
Benefits of Split Approach:
🔗 Related Links
Original Description (For Reference)
Click to view original PR description
Summary
This follow-up addresses specific review feedback on PR #1403 without changing overall intent.
What I changed
Safer container exec (injection mitigation):
Push validation and cleanup safety:
Octokit installationId type fix:
Tests and config:
Notable decisions
Verification
Request
Please review the targeted changes above. Once #1409 merges, we can follow up if any auth scoping updates are still needed here.
Closes #1399
🤖 This PR description was updated to reflect the split approach. Original functionality will be preserved across the smaller PRs.
Summary by CodeRabbit
New Features
Infrastructure
Chores
✏️ Tip: You can customize this high-level summary in your review settings.