Implement createDependentPR worker job, streamline PR labeled webhook, and add tests #1403
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.
youngchingjui
added
the
AI generated
WalkthroughAdds a webhook path that enqueues a new createDependentPR job when a PR is labeled "i2pr: update pr", a worker job type and orchestrator to run the createDependentPR workflow, plus agent and workflow implementation to create dependent PRs. Changes
Sequence DiagramsequenceDiagram
participant GitHub
participant Webhook as Webhook Route
participant Handler as PR Label Handler
participant Queue as Job Queue
participant Worker as Workflow Worker
participant Orchestrator
participant Workflow as createDependentPRWorkflow
participant Agent as DependentPRAgent
participant GH as GitHub API
GitHub->>Webhook: PR "labeled" event
Webhook->>Webhook: parse payload, extract label & installationId
alt label == "i2pr: update pr"
Webhook->>Handler: invoke handler
Handler->>Handler: validate payload & env
Handler->>Queue: enqueue `createDependentPR` job (repoFullName, pullNumber, githubLogin, githubInstallationId)
end
Queue->>Worker: worker picks up job
Worker->>Orchestrator: createDependentPR(jobId, data)
Orchestrator->>Orchestrator: init services (Neo4j, Redis), load GitHub App key
Orchestrator->>GH: request installation access token
Orchestrator->>Workflow: run createDependentPRWorkflow(params, ports)
Workflow->>Workflow: fetch PR context (diffs, comments, reviews)
Workflow->>Workflow: setup container workspace & git
Workflow->>Agent: instantiate DependentPRAgent with PR metadata
Agent->>Agent: generate changes, create branch, commit
Workflow->>GH: push dependent branch
Workflow->>Orchestrator: return branch name
Orchestrator->>Worker: return branch name
Worker->>Queue: publish completion status
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (9)
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts (1)
11-22: Consider adding a test case for REDIS_URL validation.The makePayload helper is well-structured. However, the tests don't verify that the handler throws when
REDIS_URLis not set. Consider adding a test case to ensure proper environment validation.it("throws if REDIS_URL is not set", async () => { const originalRedisUrl = process.env.REDIS_URL delete process.env.REDIS_URL await expect( handlePullRequestLabelCreateDependentPR({ payload: makePayload(), installationId, }) ).rejects.toThrow("REDIS_URL is not set") process.env.REDIS_URL = originalRedisUrl })__tests__/lib/workers/createDependentPR.worker.test.ts (1)
1-12: Consider removing unnecessary Job entity mock.The mock for
shared/entities/events/Job(lines 7-12) is a passthrough that doesn't modify behavior. Unless this is needed for future test isolation, it can be removed to simplify the test.-jest.mock("shared/entities/events/Job", () => { - const actual = jest.requireActual("shared/entities/events/Job") - return { - ...actual, - } -})shared/src/usecases/workflows/createDependentPR.ts (4)
48-66: Fail fast is good; error reporting is currently split across two event systems.
Line 56 createspub, but most status/error reporting usescreateStatusEvent/createErrorEvent/createWorkflowStateEventinstead. Consider standardizing on either theEventBusPortpublisher (pub.workflow.*) or the Neo4jcreate*Eventcalls to avoid “missing events” depending on runtime wiring.
67-81: Workflow state/events likely double-source-of-truth (Neo4j services vs EventBusPort).
Right now the function takeseventBusbut doesn’t use it for the core state/status updates (onlypub.workflow.erroron missing API key). If workers are expected to stream events via Redis (EventBusAdapter), this can silently break observability.Also applies to: 298-310
92-109:Promise.allmakes the whole workflow fail if any single PR artifact fetch fails.
If partial context is acceptable (e.g., reviews API flakes), switch toPromise.allSettledand continue with best-effort context + a warning status event.
215-249: Use stable timestamps for agent context.
toLocaleString()(Lines 219-221, 228-230, 237-239) makes traces/prompts vary by container locale/timezone. Prefer ISO strings (toISOString()) so runs are reproducible and comparable.apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (3)
13-29: MinimaluserRepolooks fine; ensure the Settings node property name is stable.
Line 26 readssettings.openAIApiKey; if the graph schema ever uses a different key (e.g.,openAiApiKey), this will silently return null and fail later. If there’s a canonical settings schema module, consider reusing it here.
56-66: Consider reusing a Neo4j datasource instead of creating one per job.
Creating a new datasource/driver per job can add overhead and connection churn; if this worker handles many jobs, a shared singleton (or injected ds) will be more stable.
38-45: Publish a failure status on error for better debuggability.
IfcreateDependentPRWorkflowthrows, this function doesn’t publish a terminal job status (only the worker’s outer handler might). Wrapping the call to publishFailed: ...(with redaction) will make webhook-triggered runs much easier to diagnose.Also applies to: 85-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.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/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__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.tsapps/workers/workflow-workers/src/handler.tslib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.tsshared/src/entities/events/Job.tsshared/src/usecases/workflows/createDependentPR.tsshared/src/lib/agents/DependentPRAgent.tsapps/workers/workflow-workers/src/orchestrators/createDependentPR.ts__tests__/lib/workers/createDependentPR.worker.test.tslib/webhook/github/types.tsapp/api/webhook/github/route.ts
🧠 Learnings (5)
📓 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-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__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.tslib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.tsapp/api/webhook/github/route.ts
📚 Learning: 2025-09-22T09:32:04.764Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/types/db/neo4j.ts:89-91
Timestamp: 2025-09-22T09:32:04.764Z
Learning: The new WorkflowStateEventSchema in shared/src/entities/events/WorkflowEvent.ts does not contain a workflowId field, so DB-level schemas should not attempt to omit it when using this schema.
Applied to files:
shared/src/entities/events/Job.ts
📚 Learning: 2025-09-22T09:32:04.764Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/types/db/neo4j.ts:89-91
Timestamp: 2025-09-22T09:32:04.764Z
Learning: The new WorkflowStateEventSchema in shared/src/entities/events/WorkflowEvent.ts is built from BaseFields (id, timestamp) and does not contain a workflowId field, so DB-level schemas should not attempt to omit workflowId when using this schema.
Applied to files:
shared/src/entities/events/Job.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 (6)
__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)
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)
shared/src/usecases/workflows/createDependentPR.ts (5)
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/agents/DependentPRAgent.ts (1)
DependentPRAgent(49-102)shared/src/lib/langfuse.ts (1)
langfuse(3-6)
apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts (5)
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)
__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-102)
app/api/webhook/github/route.ts (1)
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
handlePullRequestLabelCreateDependentPR(11-51)
🔇 Additional comments (20)
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts (3)
4-6: LGTM!The mock setup for the job service is appropriate and returns a resolved promise to simulate successful job enqueuing.
24-39: LGTM!The test correctly verifies that the handler enqueues a job with the expected payload structure and validates all required fields are passed through.
41-48: LGTM!The test appropriately validates error handling when required fields are missing.
apps/workers/workflow-workers/src/handler.ts (2)
20-20: LGTM!The import follows the established pattern for other orchestrators in this file.
70-75: LGTM!The new job case follows the established pattern for other job types in the handler. Status updates are published appropriately, and the branch name is returned consistently with the function signature.
app/api/webhook/github/route.ts (2)
14-14: LGTM!The import is appropriately placed with other handler imports.
171-189: LGTM!The labeled action handling is well-structured and follows the same pattern as the issues labeled handling. The comment correctly notes that
installationis required by the schema, eliminating the need for a null check. The case-insensitive label matching is appropriate for user-facing labels.Note: Based on learnings, this handler is awaited (line 178), which differs from some other handlers that use fire-and-forget to avoid webhook timeouts. Since this handler enqueues a job to Redis (a fast operation), awaiting should not cause timeout issues.
__tests__/lib/workers/createDependentPR.worker.test.ts (1)
15-33: LGTM!The test correctly verifies that the workflow handler routes createDependentPR jobs to the orchestrator and returns the expected branch name. The use of
anytype for the job object (line 18) is acceptable in test code for simplicity.lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (4)
1-10: LGTM!The imports are appropriate, and the docstring clearly describes the handler's purpose.
18-21: LGTM!Proper validation of the REDIS_URL environment variable with a clear error message.
23-32: LGTM!The field extraction and validation logic is robust. Using the fallback
payload.number || payload.pull_request?.numberhandles both payload structures appropriately, and thetypeof pullNumber !== "number"check ensures type safety.
37-50: LGTM!The job is properly enqueued with all required data fields matching the CreateDependentPRJobSchema. The addJob call is appropriately awaited to ensure the job is successfully added to the queue before the webhook responds.
lib/webhook/github/types.ts (1)
37-43: LGTM!The schema extensions appropriately support the new labeled PR workflow. Making these fields optional maintains backward compatibility with existing webhook handlers while enabling the new createDependentPR flow. The dual
numberfield placement (top-level and withinpull_request) matches GitHub's webhook payload structure for different action types.shared/src/entities/events/Job.ts (2)
37-49: LGTM!The CreateDependentPRJobSchema is well-defined with appropriate validation constraints. The use of
.int().positive()forpullNumberensures data integrity.
55-55: LGTM!The new job schema is correctly added to the discriminated union, enabling type-safe routing in the worker handler.
shared/src/lib/agents/DependentPRAgent.ts (5)
15-35: LGTM!The DEVELOPER_PROMPT is comprehensive and provides clear guidance for the agent. It properly sets expectations for understanding PR feedback, making targeted changes, and creating a dependent PR.
37-47: LGTM!The DependentPRAgentParams interface is well-structured with appropriate fields for the dependent PR workflow. All optional fields are clearly marked.
50-70: LGTM!The constructor properly initializes the base agent with "gpt-5" as the default model (which can be overridden by base.model per learnings). The jobId assignment and error handling for the developer prompt setup are appropriate.
73-83: LGTM!Core workspace tools are properly registered. The conditional addition of ContainerExec tool based on environment kind is appropriate.
86-100: LGTM!The remote tool registration is well-structured with defensive error handling. The try-catch block ensures that parsing errors for repository metadata don't crash the agent initialization, and the warning log provides useful debugging information.
| export type CreateDependentPRJobData = { | ||
| repoFullName: string | ||
| pullNumber: number | ||
| githubLogin: string | ||
| githubInstallationId: string | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@octokit/auth-app createAppAuth installationId type number vs string
💡 Result:
Use a number. The official docs and TypeScript types for @octokit/auth-app expect installationId as a number (both the createAppAuth options and auth({ type: "installation", installationId }) use numeric installation IDs). If you have a string, convert it (e.g., Number(installationId)). [1][2]
Sources:
[1] GitHub README for @octokit/auth-app (installationId: number).
[2] npm package docs for @octokit/auth-app (createAppAuth / auth installationId: number).
🏁 Script executed:
# Read the file to see the context around lines 31-36 and 67-75
wc -l apps/workers/workflow-workers/src/orchestrators/createDependentPR.tsRepository: youngchingjui/issue-to-pr
Length of output: 141
🏁 Script executed:
# Search for how githubInstallationId is used throughout the codebase
rg "githubInstallationId" --type ts --type tsx -B 2 -A 2Repository: youngchingjui/issue-to-pr
Length of output: 96
🏁 Script executed:
# Read the file to see the specific lines and usage
cat -n apps/workers/workflow-workers/src/orchestrators/createDependentPR.tsRepository: youngchingjui/issue-to-pr
Length of output: 3716
🏁 Script executed:
# Search for githubInstallationId without the problematic tsx flag
rg "githubInstallationId" -B 2 -A 2Repository: youngchingjui/issue-to-pr
Length of output: 7020
Convert githubInstallationId from string to number before passing to createAppAuth.
@octokit/auth-app's createAppAuth expects installationId as a number, not a string. At line 73, githubInstallationId (a string) is passed directly to the installationId field, which will cause type mismatches and unreliable behavior. Convert the value before use: installationId: Number(githubInstallationId). This same issue exists in autoResolveIssue.ts and should be corrected there as well.
🤖 Prompt for AI Agents
In apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts around
lines 31 to 36, the githubInstallationId is declared as a string but is passed
directly into createAppAuth's installationId (line ~73); convert
githubInstallationId to a number before passing it (e.g., use
Number(githubInstallationId) or parseInt) so installationId is a number to
satisfy @octokit/auth-app, and make the same change in autoResolveIssue.ts where
the same pattern occurs.
| const auth = await octokit.auth({ type: "installation" }) | ||
| if (!auth || typeof auth !== "object" || !("token" in auth) || typeof auth.token !== "string") { | ||
| throw new Error("Failed to get installation token") | ||
| } | ||
| setAccessToken(auth.token) | ||
|
|
There was a problem hiding this comment.
Global setAccessToken is unsafe if the worker processes jobs concurrently.
If two jobs overlap, Job A can overwrite the token used by Job B’s GitHub calls (cross-tenant data access). If concurrency is possible, avoid global auth state and pass tokens explicitly (or scope them via an Octokit instance per job, without globals). If concurrency is not possible, please document/enforce it at the worker queue level.
🤖 Prompt for AI Agents
In apps/workers/workflow-workers/src/orchestrators/createDependentPR.ts around
lines 79 to 84, the code calls setAccessToken(auth.token) which mutates global
auth state and is unsafe for concurrent jobs; instead, stop using the global
setter and scope auth to the job: create or return an Octokit (or client
wrapper) instantiated with auth.token and pass that instance or the token
explicitly into any downstream functions that make GitHub calls; if you
intentionally require single-job concurrency, enforce or document that at the
worker/queue level (and add a runtime assertion/config) rather than relying on a
global token.
There was a problem hiding this comment.
We're cleaning this up with #1409 . So let's merge that first, before we complete this PR.
There was a problem hiding this comment.
@youngchingjui, sounds good! I'll keep an eye out for #1409 to land first. Thanks for tackling the concurrent auth safety concern in that PR. 👍
🧠 Learnings used
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.
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.
| // Authenticate remote for fetch/push | ||
| const [owner, repoName] = repoFullName.split("/") | ||
| const sessionToken = await getInstallationTokenFromRepo({ | ||
| owner, | ||
| repo: repoName, | ||
| }) | ||
|
|
||
| // Check permissions | ||
| const permissions: RepoPermissions | null = | ||
| await checkRepoPermissions(repoFullName) | ||
| if (!permissions?.canPush || !permissions?.canCreatePR) { | ||
| await createStatusEvent({ | ||
| workflowId, | ||
| content: `Warning: Insufficient permissions to push or create PRs (${permissions?.reason || "unknown"}). Will still attempt local changes and report back.`, | ||
| }) | ||
| } | ||
|
|
||
| // Ensure origin remote embeds credentials | ||
| if (sessionToken) { | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git remote set-url origin "https://x-access-token:${sessionToken}@github.com/${repoFullName}.git"`, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Avoid embedding access tokens in git remote URLs (leak risk).
Line 148 sets origin to https://x-access-token:${sessionToken}@...; this commonly leaks via git config, error output, process args, or debug logs. Prefer http.extraHeader / credential helper / askpass, and ensure any command logging redacts secrets.
| // Fetch and checkout the PR head branch | ||
| await createStatusEvent({ | ||
| workflowId, | ||
| content: `Checking out head branch ${headRef}`, | ||
| }) | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git fetch origin ${headRef}`, | ||
| }) | ||
| // Try checkout tracking remote if local doesn't exist | ||
| const { exitCode: chk1 } = await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git rev-parse --verify ${headRef}`, | ||
| }) | ||
| if (chk1 !== 0) { | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git checkout -b ${headRef} origin/${headRef}`, | ||
| }) | ||
| } else { | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git checkout -q ${headRef}`, | ||
| }) | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git pull --ff-only origin ${headRef}`, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential command injection / ref parsing issues via interpolated headRef / dependentBranch.
headRef ultimately comes from GitHub and is interpolated into shell-ish command strings (Lines 159, 164, 169, 174, 178) and also used to build dependentBranch (Line 183). If execInContainerWithDockerode invokes a shell, this is exploitable; even without a shell, odd refs can break the commands. At minimum validate refs against an allowlist (and reject/escape otherwise), or update execInContainerWithDockerode to take argv arrays and never route through a shell.
Also applies to: 182-193
🤖 Prompt for AI Agents
In shared/src/usecases/workflows/createDependentPR.ts around lines 152-180 (and
also address 182-193), user-controlled headRef/dependentBranch are interpolated
into shell command strings which allows command injection or broken git refs;
either validate/sanitize the refs with a strict allowlist regex (reject
characters like ; | & $ ` newlines, disallow path-traversal and suspicious
sequences such as .. or leading/trailing dashes) before using them and reject
the operation on invalid refs, or change execInContainerWithDockerode calls to
accept argv arrays (no shell interpolation) and pass git arguments as separate
elements; ensure the derived dependentBranch is constructed only from validated
components and consider using explicit git argument separators (or the argv
approach) so refs cannot be interpreted as options or shell metacharacters.
There was a problem hiding this comment.
OK, let's use argv commands instead like Cmd: ["git", "checkout", "-q", headRef] if possible with dockerode.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of execInContainerWithDockerode
ast-grep --pattern 'export function execInContainerWithDockerode($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also search with async
ast-grep --pattern 'export async function execInContainerWithDockerode($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Fallback: use rg to find the function definition
rg -nP -A 20 'export.*function execInContainerWithDockerode' --type=ts
Length of output: 1991
🏁 Script executed:
#!/bin/bash
# Get the full implementation of execInContainerWithDockerode from shared/src
rg -nP -A 80 'export async function execInContainerWithDockerode' shared/src/lib/docker.ts | head -100
Length of output: 2372
🏁 Script executed:
#!/bin/bash
# Check how dockerode's exec is being called - look for Cmd usage
rg -nP -B 5 -A 10 'Cmd:' shared/src/lib/docker.ts
Length of output: 941
@youngchingjui Perfect! That's the right approach. The current implementation wraps commands in sh -c at line 180, which enables shell interpolation and the command injection risk.
Here's how to refactor it:
1. Update execInContainerWithDockerode in shared/src/lib/docker.ts
Modify the function to accept either a string or array:
export async function execInContainerWithDockerode({
name,
command,
cwd,
}: {
name: string
command: string | string[] // Accept both string and array
cwd?: string
}): Promise<{ stdout: string; stderr: string; exitCode: number }> {
// ... existing validation code ...
try {
// When array is provided, use it directly (no shell). Otherwise use sh -c for backwards compat.
const exec = await container.exec({
Cmd: Array.isArray(command) ? command : ["sh", "-c", command],
AttachStdout: true,
AttachStderr: true,
WorkingDir: cwd,
User: "root:root",
})
// ... rest of implementation ...2. Update all calls in createDependentPR.ts
Replace the string interpolations with argv arrays:
// Line 159 - fetch
await execInContainerWithDockerode({
name: containerName,
command: ["git", "fetch", "origin", headRef],
})
// Line 164 - rev-parse check
const { exitCode: chk1 } = await execInContainerWithDockerode({
name: containerName,
command: ["git", "rev-parse", "--verify", headRef],
})
// Line 169 - checkout new branch
await execInContainerWithDockerode({
name: containerName,
command: ["git", "checkout", "-b", headRef, `origin/${headRef}`],
})
// Line 174 - checkout existing
await execInContainerWithDockerode({
name: containerName,
command: ["git", "checkout", "-q", headRef],
})
// Line 178 - pull
await execInContainerWithDockerode({
name: containerName,
command: ["git", "pull", "--ff-only", "origin", headRef],
})
// Line 189 - checkout dependent branch
await execInContainerWithDockerode({
name: containerName,
command: ["git", "checkout", "-b", dependentBranch],
})Also update the git remote set-url command around line 146 (though that one uses sessionToken which is trusted, it's still good practice):
await execInContainerWithDockerode({
name: containerName,
command: [
"git",
"remote",
"set-url",
"origin",
`https://x-access-token:${sessionToken}@github.com/${repoFullName}.git`,
],
})This approach completely eliminates shell interpolation for these commands, making them immune to command injection even if headRef or other variables contain malicious characters.
🧠 Learnings used
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.
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}.
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: lib/webhook/handlers/pullRequestHandler.ts:34-41
Timestamp: 2025-09-12T02:32:22.420Z
Learning: When fixing command injection vulnerabilities in Docker operations, replace shell-based commands like `docker ps --filter "label=${userInput}"` with dockerode API calls using the filters object parameter. This completely eliminates injection risks by avoiding shell interpolation of user-controlled data.
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.
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: Only suggest git commands if explicitly requested by the user
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.
| // Best-effort: ensure branch is pushed (idempotent if already pushed) | ||
| await createStatusEvent({ | ||
| workflowId, | ||
| content: `Ensuring branch ${dependentBranch} is pushed`, | ||
| }) | ||
| if (sessionToken) { | ||
| await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: `git push -u origin ${dependentBranch} || true`, | ||
| }) | ||
| } | ||
|
|
||
| await createWorkflowStateEvent({ workflowId, state: "completed" }) | ||
| return { | ||
| agentResult: result, | ||
| branch: dependentBranch, | ||
| } | ||
| } catch (error) { | ||
| await createErrorEvent({ workflowId, content: String(error) }) | ||
| await createWorkflowStateEvent({ | ||
| workflowId, | ||
| state: "error", | ||
| content: String(error), | ||
| }) | ||
| throw error | ||
| } finally { | ||
| if (containerCleanup) await containerCleanup() | ||
| } |
There was a problem hiding this comment.
Don’t mark workflow completed if push fails; also don’t let cleanup mask the real error.
- Line 294 uses
|| true, so push failure is silently ignored but the workflow is still markedcompleted(Line 298). If the branch isn’t pushed, downstream PR creation can’t succeed. - Line 312
await containerCleanup()can throw and override the original error from the try/catch. Wrap cleanup in its own try/catch and emit a warning status.
Suggested adjustment:
- if (sessionToken) {
- await execInContainerWithDockerode({
- name: containerName,
- command: `git push -u origin ${dependentBranch} || true`,
- })
- }
+ 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}`)
+ }
+ }
...
} finally {
- if (containerCleanup) await containerCleanup()
+ if (containerCleanup) {
+ try {
+ await containerCleanup()
+ } catch (e) {
+ await createStatusEvent({
+ workflowId,
+ content: `Warning: container cleanup failed: ${String(e)}`,
+ })
+ }
+ }
}📝 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.
| // Best-effort: ensure branch is pushed (idempotent if already pushed) | |
| await createStatusEvent({ | |
| workflowId, | |
| content: `Ensuring branch ${dependentBranch} is pushed`, | |
| }) | |
| if (sessionToken) { | |
| await execInContainerWithDockerode({ | |
| name: containerName, | |
| command: `git push -u origin ${dependentBranch} || true`, | |
| }) | |
| } | |
| await createWorkflowStateEvent({ workflowId, state: "completed" }) | |
| return { | |
| agentResult: result, | |
| branch: dependentBranch, | |
| } | |
| } catch (error) { | |
| await createErrorEvent({ workflowId, content: String(error) }) | |
| await createWorkflowStateEvent({ | |
| workflowId, | |
| state: "error", | |
| content: String(error), | |
| }) | |
| throw error | |
| } finally { | |
| if (containerCleanup) await containerCleanup() | |
| } | |
| // Best-effort: ensure branch is pushed (idempotent if already pushed) | |
| await createStatusEvent({ | |
| workflowId, | |
| content: `Ensuring branch ${dependentBranch} is pushed`, | |
| }) | |
| 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}`) | |
| } | |
| } | |
| await createWorkflowStateEvent({ workflowId, state: "completed" }) | |
| return { | |
| agentResult: result, | |
| branch: dependentBranch, | |
| } | |
| } catch (error) { | |
| await createErrorEvent({ workflowId, content: String(error) }) | |
| await createWorkflowStateEvent({ | |
| workflowId, | |
| state: "error", | |
| content: String(error), | |
| }) | |
| throw error | |
| } finally { | |
| if (containerCleanup) { | |
| try { | |
| await containerCleanup() | |
| } catch (e) { | |
| await createStatusEvent({ | |
| workflowId, | |
| content: `Warning: container cleanup failed: ${String(e)}`, | |
| }) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In shared/src/usecases/workflows/createDependentPR.ts around lines 286 to 313,
the code currently ignores git push failures with `|| true` and still marks the
workflow "completed", and the final `containerCleanup()` call can throw and mask
an earlier error. Change the push to not swallow failures: run `git push -u
origin ${dependentBranch}` and check its exit status or catch its error; if the
push fails, emit an error status event (createErrorEvent /
createWorkflowStateEvent with state "error") and rethrow so the workflow is not
marked "completed". Also wrap `containerCleanup()` in its own try/catch in the
finally block so any cleanup error is logged/emitted as a warning status event
(createStatusEvent or createErrorEvent with warning content) but does not
override or replace an existing error thrown from the try block; ensure the
original error is rethrown after cleanup handling.
There was a problem hiding this comment.
OK, good points for both. let's implement these suggestions.
There was a problem hiding this comment.
@youngchingjui Sounds good! The two changes will ensure:
-
Push validation — The workflow won't silently proceed to "completed" if the branch push fails. Instead, it will throw an error and mark the workflow state as "error", giving clear feedback when the branch can't be published.
-
Cleanup safety — Wrapping
containerCleanup()in its own try/catch ensures that if cleanup fails (e.g., container already removed), the original error from the workflow is preserved and rethrown, while the cleanup failure is logged as a warning. This prevents masking the root cause of failures.
Let me know once you've implemented these changes, and I can verify the updated logic! 🚀
🧠 Learnings used
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.
## 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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
__tests__/lib/utils/setupEnv.test.ts (1)
36-52: Consider adding a defensive guard for the callback extraction.The callback extraction logic correctly handles both
exec(cmd, callback)andexec(cmd, options, callback)signatures. However, if bothoptionsOrCallbackandmaybeCallbackare falsy, the type assertion on line 45 would assignundefinedtocallback, causing a runtime error.In practice,
setupEnvalways provides three arguments, so this scenario won't occur. Still, a guard would make the mock more robust against future changes.Optional improvement:
- const callback: ExecCallback = - typeof optionsOrCallback === "function" - ? optionsOrCallback - : (maybeCallback as ExecCallback) + const callback: ExecCallback = + typeof optionsOrCallback === "function" + ? optionsOrCallback + : maybeCallback ?? + (() => { + throw new Error("exec mock called without callback") + })__tests__/apps/workers/handler.autoResolveIssue.test.ts (1)
19-56: Consider adding error scenario tests.The current test suite only covers the happy path. Consider adding tests for:
- Orchestrator throwing an error
- Invalid job data failing schema validation
- Edge cases like empty message arrays
These would improve coverage and ensure robust error handling.
Example error test:
it("publishes failure status when orchestrator throws", async () => { const error = new Error("Orchestrator failed") mockAutoResolveIssue.mockRejectedValue(error) const job = { id: "job-123", name: "autoResolveIssue", data: { repoFullName: "owner/repo", issueNumber: 42, branch: "feature-branch", githubLogin: "octocat", githubInstallationId: "install-1", }, } await expect(handler(job as Job)).rejects.toThrow("Orchestrator failed") expect(mockPublishJobStatus).toHaveBeenCalledWith( job.id, expect.stringContaining("Failed:") ) })__tests__/api/webhook/github.route.test.ts (1)
45-80: LGTM!The test properly verifies webhook routing with correct HMAC signature generation and handler invocation. The mock request type assertion on line 67 is necessary for testing.
Optional: Consider expanding test coverage in a future PR.
The current test covers the happy path. Future improvements could include:
- Invalid signature verification
- Different label values
- Missing installation ID
- Error responses
However, this is optional and can be deferred per the project's preference for tight PR scope.
Based on learnings, youngchingjui prefers to manage PR scope tightly and defer improvements to future PRs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__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/label.autoResolveIssue.handler.test.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__/api/webhook/github.route.test.ts__tests__/apps/workers/handler.autoResolveIssue.test.ts__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts__tests__/config/jest.config.node.ts__tests__/lib/utils/setupEnv.test.ts__tests__/config/jest.config.base.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.
📚 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__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 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 (2)
__tests__/api/webhook/github.route.test.ts (1)
lib/webhook/github/handlers/issue/label.autoResolveIssue.handler.ts (1)
handleIssueLabelAutoResolve(11-47)
__tests__/apps/workers/handler.autoResolveIssue.test.ts (2)
apps/workers/workflow-workers/src/helper.ts (1)
publishJobStatus(48-54)apps/workers/workflow-workers/src/handler.ts (1)
handler(24-86)
🪛 Biome (2.1.2)
__tests__/config/jest.config.base.ts
[error] 12-12: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (14)
__tests__/config/jest.config.node.ts (1)
13-17: LGTM!The addition of the apps test path pattern is correct and consistent with existing patterns. This properly enables Jest to discover tests under
__tests__/apps/for the new worker and webhook tests.__tests__/config/jest.config.base.ts (1)
15-17: Mapper ordering looks correct.The multi-namespace alias for shared paths and the catch-all
^@/(.*)$pattern are appropriately ordered—more specific patterns precede the general fallback, ensuring correct resolution.__tests__/lib/utils/setupEnv.test.ts (3)
7-11: LGTM!The type imports are appropriate and necessary for properly typing the mock implementation.
21-26: LGTM!The custom
ExecCallbacktype definition is clear and appropriate for these test mocks. While Node.js provides similar types, this simplified version (usingstringinstead ofstring | Buffer) is well-suited for the test scenarios.
66-88: LGTM with the same optional improvement.The failure test mock correctly simulates an error scenario. The same optional defensive guard for callback extraction mentioned in the success test (lines 36-52) would apply here as well.
The error object construction using
Object.assignto addstdoutandstderrproperties is an appropriate approach for the test mock.__tests__/apps/workers/handler.autoResolveIssue.test.ts (2)
1-10: LGTM!The mock setup is correct and properly hoisted before imports. The paths and mock structures align with the handler's dependencies.
24-56: Well-structured test with good assertions.The test correctly verifies:
- Orchestrator routing with proper arguments
- Status publishing sequence (parsing → job start → completion)
- Result formatting (newline-joined message contents)
The mock data structure aligns with the handler's expectations.
__tests__/lib/webhook/label.autoResolveIssue.handler.test.ts (4)
1-9: LGTM!The imports and mock setup are clean and appropriate. Direct imports avoid barrel files as per coding guidelines.
11-28: LGTM!The test setup demonstrates good practices with a reusable payload builder and proper test isolation via
beforeEachcleanup.
30-56: LGTM!The test comprehensively verifies that the handler enqueues a job with all expected parameters, including queue name, job data fields, and Redis URL.
58-86: LGTM!Both error case tests properly verify validation logic. The type assertion on line 78 is necessary for testing invalid payloads.
__tests__/api/webhook/github.route.test.ts (3)
1-5: LGTM!Appropriate Jest environment configuration and imports for testing the GitHub webhook route.
7-30: LGTM!Comprehensive mocking setup for all webhook handlers. The pattern of mocking before importing ensures handlers are properly stubbed during route execution.
32-43: LGTM!Proper test setup with environment isolation and cleanup.
| import { handler } from "../../../apps/workers/workflow-workers/src/handler" | ||
| import { publishJobStatus } from "../../../apps/workers/workflow-workers/src/helper" | ||
| import { autoResolveIssue } from "../../../apps/workers/workflow-workers/src/orchestrators/autoResolveIssue" | ||
|
|
||
| const mockAutoResolveIssue = jest.mocked(autoResolveIssue) | ||
| const mockPublishJobStatus = jest.mocked(publishJobStatus) |
There was a problem hiding this comment.
Missing import for Job type.
Line 43 uses the Job type without importing it, which will cause a TypeScript compilation error.
Add the missing import at the top of the file:
+import type { Job } from "../../../shared/src/entities/events/Job"
+
import { handler } from "../../../apps/workers/workflow-workers/src/handler"
import { publishJobStatus } from "../../../apps/workers/workflow-workers/src/helper"
import { autoResolveIssue } from "../../../apps/workers/workflow-workers/src/orchestrators/autoResolveIssue"📝 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.
| import { handler } from "../../../apps/workers/workflow-workers/src/handler" | |
| import { publishJobStatus } from "../../../apps/workers/workflow-workers/src/helper" | |
| import { autoResolveIssue } from "../../../apps/workers/workflow-workers/src/orchestrators/autoResolveIssue" | |
| const mockAutoResolveIssue = jest.mocked(autoResolveIssue) | |
| const mockPublishJobStatus = jest.mocked(publishJobStatus) | |
| import type { Job } from "../../../shared/src/entities/events/Job" | |
| import { handler } from "../../../apps/workers/workflow-workers/src/handler" | |
| import { publishJobStatus } from "../../../apps/workers/workflow-workers/src/helper" | |
| import { autoResolveIssue } from "../../../apps/workers/workflow-workers/src/orchestrators/autoResolveIssue" | |
| const mockAutoResolveIssue = jest.mocked(autoResolveIssue) | |
| const mockPublishJobStatus = jest.mocked(publishJobStatus) |
🤖 Prompt for AI Agents
In __tests__/apps/workers/handler.autoResolveIssue.test.ts around lines 12 to
17, the test uses the Job type on line 43 but hasn't imported it; add an import
for the Job type (for example: import { Job } from "bullmq") at the top of the
file alongside the other imports so TypeScript can resolve the type; ensure the
import path matches the project's Job type source if you use a different module
and keep imports grouped/ordered consistently.
There was a problem hiding this comment.
I don't see any Job export from that file, but yes, we seem to be missing the type. Please find the right type or create one so that the typescript can pass in this file.
In addition, we also have an issue with messages:
Argument of type '{ content: string; }[]' is not assignable to parameter of type 'EnhancedMessage[] | Promise<EnhancedMessage[]>'.
Type '{ content: string; }[]' is not assignable to type 'EnhancedMessage[]'.
Type '{ content: string; }' is not assignable to type 'EnhancedMessage'.
Type '{ content: string; }' is not assignable to type 'ChatCompletionFunctionMessageParam & { id?: string | undefined; timestamp?: Date | undefined; }'.
Type '{ content: string; }' is missing the following properties from type 'ChatCompletionFunctionMessageParam': name, rolets(2345)
⌘+click to open in new tab
const messages: {
content: string;
}[]
let's fix that too.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Job type exports in the codebase
rg -n "export (type|interface) Job" --type=ts
# Also check for default Job exports
rg -n "export.*Job.*from.*bullmq" --type=ts
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Look at the full test file to understand the context
cat __tests__/apps/workers/handler.autoResolveIssue.test.ts
Length of output: 1737
🏁 Script executed:
#!/bin/bash
# Find EnhancedMessage type definition
rg -n "type EnhancedMessage|interface EnhancedMessage" --type=ts
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Check what bullmq exports (if it's being used)
rg -n "from ['\"]bullmq['\"]" --type=ts -C2
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Look at the handler file to see what Job type it expects
cat apps/workers/workflow-workers/src/handler.ts | head -50
Length of output: 1729
🏁 Script executed:
#!/bin/bash
# Check what autoResolveIssue orchestrator returns
cat apps/workers/workflow-workers/src/orchestrators/autoResolveIssue.ts | tail -100
Length of output: 2567
🏁 Script executed:
#!/bin/bash
# Look at the EnhancedMessage type definition
cat shared/src/lib/types/chat.ts
Length of output: 328
🏁 Script executed:
#!/bin/bash
# Check if ChatCompletionMessageParam is from openai
rg -n "ChatCompletionMessageParam" --type=ts | head -20
Length of output: 1784
@youngchingjui, I found the issues. The Job type comes from the bullmq library (as used in the handler), and the messages mock needs to match the ChatCompletionMessageParam structure which requires a role field at minimum.
Here's the fix:
import type { Job } from "bullmq"
import { handler } from "../../../apps/workers/workflow-workers/src/handler"
import { publishJobStatus } from "../../../apps/workers/workflow-workers/src/helper"
import { autoResolveIssue } from "../../../apps/workers/workflow-workers/src/orchestrators/autoResolveIssue"
const mockAutoResolveIssue = jest.mocked(autoResolveIssue)
const mockPublishJobStatus = jest.mocked(publishJobStatus)
describe("handler - autoResolveIssue", () => {
beforeEach(() => {
jest.clearAllMocks()
})
it("routes autoResolveIssue jobs and publishes status updates", async () => {
const messages = [
{ role: "assistant" as const, content: "first message" },
{ role: "assistant" as const, content: "second message" },
]
mockAutoResolveIssue.mockResolvedValue(messages)
const job = {
id: "job-123",
name: "autoResolveIssue",
data: {
repoFullName: "owner/repo",
issueNumber: 42,
branch: "feature-branch",
githubLogin: "octocat",
githubInstallationId: "install-1",
},
}
const result = await handler(job as Job)
expect(mockAutoResolveIssue).toHaveBeenCalledWith(job.id, job.data)
expect(result).toBe("first message\nsecond message")
expect(mockPublishJobStatus).toHaveBeenCalledWith(job.id, "Parsing job")
expect(mockPublishJobStatus).toHaveBeenCalledWith(
job.id,
"Job: Auto resolve issue"
)
expect(mockPublishJobStatus).toHaveBeenCalledWith(
job.id,
"Completed: first message\nsecond message"
)
})
})The changes:
- Added
import type { Job } from "bullmq"at the top - Updated the messages mock to include the
rolefield (set to"assistant") which is required byChatCompletionMessageParam
🧠 Learnings used
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}.
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.
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.
## Summary Fix Jest test infrastructure to support testing new worker and shared functionality added in the createDependentPR feature chain. ## What Changed - **Fix Jest moduleNameMapper order and conflicts**: Move `"^@/(.*)$"` to end to prevent it from overriding more specific mappings - **Add missing shared path mappings**: `"^shared/(.*)$"` and clean architecture paths for `adapters|entities|lib|ports|providers|services|ui|usecases|utils` - **Include apps/workers in test patterns**: Enable Jest to run tests for new worker orchestrators - **Improve TypeScript types in setupEnv.test.ts**: Add proper types for `child_process.exec` mocks to prevent type errors ## Why These Changes Were Needed This fixes test infrastructure issues that emerged during the createDependentPR feature development: 1. **Issue #1399 → PR #1401 → PR #1403 → PR #1413**: As new worker orchestrators and shared utilities were added, Jest couldn't resolve imports properly 2. **Missing path mappings**: Tests for new `shared/` modules and worker orchestrators were failing due to unresolved imports 3. **Type safety**: CodeRabbit reviews flagged loose typing in test mocks that needed to be fixed ## Test Plan - [x] Existing tests continue to pass - [x] setupEnv.test.ts runs with improved type safety - [x] Jest can now resolve `shared/` and worker import paths properly - [x] Foundation is set for testing the worker functionality in subsequent PRs This enables the test infrastructure needed for the remaining PRs in the createDependentPR feature chain. 🤖 Generated with [Claude Code](https://claude.ai/code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Improves Jest resolution and test coverage, plus tightens types in a CLI test. > > - Update `__tests__/config/jest.config.base.ts`: > - Add `^shared/(.*)$` and grouped clean-architecture alias `^@/(adapters|entities|ports|providers|services|ui|usecases|utils)(/.*)?$` to map to `shared/src` > - Keep `@shared` and `@workers` mappings; move catch‑all `^@/(.*)$` to the end to avoid shadowing specific aliases > - Update `__tests__/config/jest.config.node.ts`: > - Expand `testMatch` to include `**/__tests__/apps/**/*.ts?(x)` > - Update `__tests__/lib/utils/setupEnv.test.ts`: > - Add strict typings for `child_process.exec` mock (`ChildProcess`, `ExecException`, `ExecOptions`) and a typed callback; adjust mock implementations and casts accordingly > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e42310c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Security Fixes
- **Enhanced execInContainerWithDockerode**: Now supports string arrays to bypass shell interpolation
- **Fixed Branch tool injection vulnerability**: Replaced string interpolation with safe array commands
- **Added comprehensive security documentation**: Clear examples of safe vs unsafe patterns
## Technical Details
- Array commands (`["git", "checkout", branch]`) are passed directly to Docker, preventing injection
- String commands still supported for backwards compatibility but clearly marked as unsafe
- Fixed critical vulnerability where malicious branch names could execute arbitrary commands
## Examples Fixed
```typescript
// BEFORE (vulnerable to injection)
await exec(`git checkout ${userBranch}`) // If userBranch = "main; rm -rf /"
// AFTER (injection-proof)
await execInContainerWithDockerode({
name: container,
command: ["git", "checkout", userBranch]
})
```
This addresses the command injection vulnerability identified in PR #1403 review feedback.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
## 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 -->
|
The successor to this PR is #1413 , so closing this PR. |
Summary
This follow-up addresses review feedback on PR #1401 by implementing the actual worker-side processing for the createDependentPR job, simplifying the webhook handling for PR label events, and adding targeted tests.
What changed
Worker
Shared
Webhook
Tests
Notes on scope
Verification
If anything else would help (e.g., additional webhook route tests or broader refactors to consolidate parsing), I can follow up in a separate PR.
Closes #1399
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.