Trigger createDependentPR workflow when PR is labeled "I2PR: Update PR" #1401
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)
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
| job.id, | ||
| "Job: Create dependent PR (dispatching to web workflow runner)" | ||
| ) | ||
| // In a future iteration, this worker can invoke a shared usecase. |
There was a problem hiding this comment.
No, we need to actually implement this.
I think previously for other workflows, like autoResolveIssue, we've had to migrate their implementations from the /lib folder into the /shared folder somehow.
Once, I just copied the entire implementation into the /shared folder, with similar folder structure, and vowed to refactor in a future date. We can do something similar here, if needed.
The worker must be able to run the createDependentPR job.
| break | ||
| } | ||
| case "labeled": { | ||
| const installationId = String(parsedPayload.installation?.id ?? "") |
There was a problem hiding this comment.
I think based on the PullRequestPayloadSchema, the installation ID should exist (it is NOT optional). So if it didn't exist in the payload, then our schema.safeParse will already catch that error.
If the rest of our subscribers to the PullRequestPayloadSchema is also converting it to a String, then might as well just make the whole schema installataionId into a string, instead of number.
| return new Response("No installation ID found", { status: 400 }) | ||
| } | ||
| const labelName: string | undefined = | ||
| parsedPayload.label?.name?.trim() |
There was a problem hiding this comment.
I think the whole structure of this file feels a bit jumbled and it's hard to read.
Because we're mixing switch statements, and in between each switch statement, we're doing additional parsing.
It would be nice if we could move all the parsing to 1 section, including the converting and trimming and extracting data, maybe to the top of the file somehow, before we start entering the switch statements.
That way we can just collapse the nested switch statements into a more cohesive tree, without having to see all the parsing functionality interspersed within the tree.
| parsedPayload.label?.name?.trim() | ||
| switch (labelName?.toLowerCase()) { | ||
| case "i2pr: update pr": { | ||
| await handlePullRequestLabelCreateDependentPR({ |
There was a problem hiding this comment.
It would be great if we could have a few different tests built out in the /tests folder somehow.
I'm not sure how to best structure tests, but my initial thinking is we'd need:
- a test to see if a webhook event gets captured by our API here. So if we could send a mock webhook event, and see if our webhook catches it, including this new implementation for the 'labeled' action.
- A test to see if our webhook can correctly create a job for our job queue. I'm guessing there would also be a mock for queueing job events.
- A test to see if our worker would pick up the job correctly and launch the workflow. I guess the workflow itself could also have a mock (so we don't actually launch any workflows).
How are tests often structured? I'm still new to testing.
## 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)
| "^@workers/(.*)$": "<rootDir>/apps/workers/src/$1", | ||
|
|
||
| // NEW: allow direct `shared/...` imports in tests | ||
| "^shared/(.*)$": "<rootDir>/shared/src/$1", |
There was a problem hiding this comment.
This, along with the setupEnv.test.ts, seems unrelated to this PR. I would wish we could carve out these changes in a separate PR from main, and merge that carved out PR first, before reviewing this PR.
…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)
## 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 -->
|
The successor is #1413 , so closing this PR. |
Summary
Details
Why
This enables triggering our createDependentPR workflow via a GitHub PR label, aligning with the request to kick off the dependent PR process when maintainers apply a label.
Notes
How to test
Closes #1399