feat: add Workday integration plugin - #475
Conversation
|
@Dhirenderchoudhary is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR introduces the
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Factory as executeWorkdayOperation
participant Client as makeWorkdayRequest
participant Http as corsair/http
participant Errors as errorHandlers
participant Webhook as WebhookPipeline
Caller->>Factory: invoke endpoint(ctx, input)
Factory->>Factory: normalizeInputAliases + zod parse input
Factory->>Factory: "resolvePath (encode {ID} placeholders)"
Factory->>Client: path, apiKey, method/body/query/service/version
Client->>Http: request(config, options)
alt success
Http-->>Client: response T
Client-->>Factory: T
Factory->>Factory: zod parse output schema
Factory-->>Caller: typed result
else ApiError 429
Http-->>Client: throw ApiError
Client-->>Factory: rethrow ApiError (status + retryAfter preserved)
Factory-->>Errors: RATE_LIMIT_ERROR.match
Errors-->>Caller: maxRetries 5 + headersRetryAfterMs
end
Note over Webhook: Incoming webhook POST
Webhook->>Webhook: pluginWebhookMatcher (x-workday-signature or x-workday-event header)
Webhook->>Webhook: matchWorkdayTenantWebhook (tenant_external_id)
Webhook->>Webhook: createWorkdayEventMatch (header type or body.type)
Webhook->>Webhook: verifyWorkdayWebhookSignature via verifyHmacSignature
alt signature valid
Webhook-->>Caller: success true
else signature invalid
Webhook-->>Caller: success false statusCode 401
end
Reviews (11): Last reviewed commit: "fix(workday): preserve ApiError retryAft..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ✅ | |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | ✅ | |
| R4 — Demo video / recording | ✅ |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @Dhirenderchoudhary, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Rule Used: Flag Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
|
Remaining findings are being fixed by a bot commit — it will be re-reviewed automatically. |
| pluginWebhookMatcher: (request) => { | ||
| return 'x-workday-signature' in request.headers; | ||
| }, |
There was a problem hiding this comment.
pluginWebhookMatcher accepts any request that carries an x-workday-signature header, but never calls verifyWorkdayWebhookSignature. The HMAC-SHA256 verification logic is correctly implemented and exported, yet it is not invoked anywhere in the webhook routing path. As a result, the plugin accepts any payload delivered with this header regardless of whether it passes cryptographic validation, leaving the webhook endpoint open to spoofed events.
There was a problem hiding this comment.
fixed check
Maintainer review neededAutomated rounds are exhausted. Remaining findings:
|
github-actions
Bot
added
the
needs-maintainer
| WorkdayEndpointOutputs['getJobById'] | ||
| >('v1/job/getJobById/{id}', ctx.key, { | ||
| method: 'GET', | ||
| // Justification: The makeWorkdayRequest client expects a generic unknown record. |
There was a problem hiding this comment.
{id} placeholder never interpolated into URL
The URL 'v1/job/getJobById/{id}' is a plain string — JavaScript does not substitute {id} at runtime. The actual HTTP request goes to https://api.workday.com/v1/job/getJobById/{id} literally, which will return a 404 or an error from Workday. The id value from input is instead appended as a query parameter (via the query spread), so the path parameter is never filled. The URL must be constructed dynamically — for example `v1/job/jobs/${input?.id}` — or the input needs to be used to build the path before calling makeWorkdayRequest.
| const expected = crypto | ||
| .createHmac('sha256', secret) | ||
| .update(bodyString) | ||
| .digest('base64'); | ||
| if (signature !== expected) { | ||
| return { valid: false, error: 'Invalid webhook signature' }; |
There was a problem hiding this comment.
signature !== expected is a regular string comparison, which leaks information about how many leading bytes of a forged signature match the real one via response-time differences. The shared verifyHmacSignature utility already exported from corsair/http uses crypto.timingSafeEqual for a constant-time comparison and also uses hex encoding (rather than base64). An attacker making repeated webhook requests can exploit the timing oracle to recover the HMAC secret character-by-character. Replace the comparison with verifyHmacSignature(bodyString, secret, signature) from corsair/http.
There was a problem hiding this comment.
Fixed check
Replace fabricated v1/{domain}/{op} stubs with staffing/absence/
recruiting/payroll/common/person routes, tenant-hosted OAuth, and
the 13 Composio-aligned triggers.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds a Workday provider package with 85 REST operations, typed request and response validation, OAuth authentication, webhook triggers, cache synchronization, error handling, schemas, tests, and Corsair provider registration. ChangesWorkday integration
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Keep vercel/whatsapp from main with workday registration; regenerate lockfile.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@greptile review |
| try { | ||
| return await request<T>(config, requestOptions); | ||
| } catch (error) { | ||
| if (error instanceof Error) throw new WorkdayAPIError(error.message); | ||
| throw new WorkdayAPIError('Unknown error'); | ||
| } |
There was a problem hiding this comment.
ApiError status and retry metadata
makeWorkdayRequest converts every thrown error into a plain WorkdayAPIError, stripping the original type. The rate-limit handler in error-handlers.ts then checks error instanceof ApiError && error.status === 429 (line 7) and reads error.retryAfter (line 13), but because the error is now a WorkdayAPIError, the instanceof ApiError branch never executes and retryAfterMs is always undefined. Workday's Retry-After header value is permanently lost, so rate-limited callers will always retry without the server-requested delay. The catch block should either rethrow the original ApiError directly or copy status and retryAfter onto WorkdayAPIError.
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
| try { | ||
| return await request<T>(config, requestOptions); | ||
| } catch (error) { | ||
| if (error instanceof Error) throw new WorkdayAPIError(error.message); | ||
| throw new WorkdayAPIError('Unknown error'); | ||
| } |
There was a problem hiding this comment.
makeWorkdayRequest catches the ApiError thrown by request() and converts it to a WorkdayAPIError, discarding status and retryAfter. In error-handlers.ts, RATE_LIMIT_ERROR.match checks error instanceof ApiError && error.status === 429 — but that guard is always false here because the error reaching the handler is a WorkdayAPIError, not an ApiError. The fallback msg.includes('429') may accidentally fire depending on the message text, but even then error.retryAfter is unavailable on WorkdayAPIError, so headersRetryAfterMs is always undefined and Workday's Retry-After header is silently ignored. Either re-throw the original ApiError (or a subclass that preserves status/retryAfter), or extend WorkdayAPIError to carry those fields and update the matcher accordingly.
|
@greptile review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
packages/workday/endpoints/routes.ts (1)
164-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo route entries define the same endpoint with different query sets.
getJobChangeReasonValuesandgetJobChangeReasonsboth targetGET /values/jobChangesGroup/reasonon Staffing v6, but they declare differentqueryParamsand different output schemas (WorkdayCollectionSchemafor both, yet different filter surfaces).getCollectionOfJobsandlistJobscarry an explicit alias comment; these two do not.If the duplication is an intentional Composio alias, add the same explanatory comment and align
queryParams. If it is not intentional, remove one entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/routes.ts` around lines 164 - 206, Resolve the duplicate GET route definitions for /values/jobChangesGroup/reason by either removing one of getJobChangeReasonValues or getJobChangeReasons, or, if they are intentional aliases, adding the established alias comment and making both queryParams lists identical. Preserve a single consistent endpoint contract and output schema.packages/workday/endpoints/cache-sync.ts (4)
113-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe
cacheItemsfallback can cache a collection envelope as an entity.If a collection response uses a list key that is not in
rule.listKeys, the function falls through toreturn [response].cacheEntityIdthen readsidfrom the envelope. Workday collection envelopes carrytotalanddata, so the risk is low today, but a response shaped like{ id, items: [...] }would store the envelope under an entity id.Restrict the single-resource fallback to responses that do not look like collections.
🛡️ Proposed change
for (const key of rule.listKeys ?? []) { const value = response[key]; if (Array.isArray(value)) return value.filter(isRecord); } + // A collection envelope without a recognized list key must not be cached as an entity. + if (typeof response.total === 'number') return []; + return [response];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/cache-sync.ts` around lines 113 - 123, Update cacheItems so the single-resource fallback only returns [response] when the record does not contain collection-shaped fields such as total, data, or array-valued properties; otherwise return an empty list. Preserve the existing array filtering and configured rule.listKeys handling.
186-192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache upserts run one at a time on the request path.
List routes such as
getWorkersCollectionStaffing,listJobPostings, andgetCollectionOfPayrollreturn up to 100 items per call, becauselimitis capped at 100 intypes.ts. This loop awaits one database write per item, so a single API call adds up to 100 serial round trips beforeexecuteWorkdayOperationreturns.Run the upserts concurrently with a bound, or add a batch upsert to the cache client.
♻️ Proposed change
if (!client.upsertByEntityId) return; - for (const item of cacheItems(response, rule)) { - const entityId = cacheEntityId(item, rule); - if (!entityId) continue; - await client.upsertByEntityId(entityId, item); - } + const pending = cacheItems(response, rule) + .map((item) => ({ item, entityId: cacheEntityId(item, rule) })) + .filter((entry): entry is { item: Record<string, unknown>; entityId: string } => + Boolean(entry.entityId), + ); + + const CONCURRENCY = 8; + for (let i = 0; i < pending.length; i += CONCURRENCY) { + await Promise.all( + pending + .slice(i, i + CONCURRENCY) + .map(({ entityId, item }) => client.upsertByEntityId?.(entityId, item)), + ); + } } catch (error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/cache-sync.ts` around lines 186 - 192, Update the cache upsert loop in the request path around client.upsertByEntityId to avoid serial awaits: execute item upserts concurrently with a bounded concurrency mechanism, or use an available batch-upsert API. Preserve entityId filtering and ensure all scheduled upserts complete before the surrounding operation returns.
134-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe delete branch is unreachable, and
cacheDeleteEntityIdsreturns at most one id.No route in
routes.tsuses methodDELETE, and no route usesriskLevel: 'destructive'. So the branch at Lines 176-184 never executes in this PR. AlsocacheDeleteEntityIdsreturns a single-element array despite the plural name, because it returns on the first matching key.Either add the DELETE routes that this branch serves, or rename the helper to
cacheDeleteEntityIdand return one optional id. Keep the code if the DELETE routes arrive in a follow-up; state that intent in a comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/cache-sync.ts` around lines 134 - 148, Resolve the unreachable delete flow by either adding the DELETE/destructive routes it supports or, if those are intentionally deferred, retain the implementation and add a comment documenting that follow-up intent. Also align cacheDeleteEntityIds with its single-value behavior by renaming it to cacheDeleteEntityId and returning one optional ID, updating all call sites accordingly.
159-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
ctx.dbcast hides the cache client contract.
ctx.dbis cast to an inline structural type inside the function. Any change to the generated database client breaks silently at runtime instead of at compile time. Extract a named type and place it next to theCacheEntityunion, then havesyncWorkdayOperationCacheaccept it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/cache-sync.ts` around lines 159 - 173, Extract the inline database client shape from syncWorkdayOperationCache into a named type placed alongside the CacheEntity union, then use that type for the function’s ctx.db contract instead of casting ctx.db inline. Preserve the existing optional upsertByEntityId and deleteByEntityId methods and entity lookup behavior.packages/workday/endpoints/types.ts (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
PaginationQueryShapeis unused, and the query-value union is repeated about fifty times.Every schema re-declares
limit,offset, and thez.union([z.string(), z.number(), z.boolean(), z.array(z.string())])shape inline. Extract shared helpers and spread them. This removes roughly a thousand duplicated lines and keeps the pagination bounds in one place.♻️ Proposed refactor
const PaginationQueryShape = { limit: z.number().int().min(1).max(100).optional(), offset: z.number().int().min(0).optional(), }; + +const FilterValue = z + .union([z.string(), z.number(), z.boolean(), z.array(z.string())]) + .optional(); + +const IdShape = { + ID: z.string().min(1), + id: z.string().min(1).optional(), + workerId: z.string().min(1).optional(), +};Then each entry becomes, for example:
getJobById: z.object({ ...IdShape, ...PaginationQueryShape }).passthrough(), getJobChangeFrequencies: z .object({ effectiveDate: FilterValue, event: FilterValue, ...PaginationQueryShape, }) .passthrough(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/types.ts` around lines 21 - 24, Replace the unused PaginationQueryShape and repeated inline query-value unions in the endpoint schemas with shared FilterValue and PaginationQueryShape helpers. Update each affected z.object definition, including getJobById and getJobChangeFrequencies, to spread these helpers while preserving endpoint-specific fields and passthrough behavior; keep the existing limit and offset bounds centralized in PaginationQueryShape.packages/workday/endpoints/operations.ts (1)
9-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
workdayEndpointsNestedduplicates thegroupfield and can drift fromroutes.ts.Each route already declares
group. This object restates the grouping for all 85 operations by hand. If someone adds a route, the nested surface silently omits it, because no type checks completeness.Derive the grouping from the route table, or add a compile-time completeness check.
♻️ Sketch of a derived grouping
type WorkdayGroup = (typeof workdayRoutes)[number]['group']; export const workdayEndpointsNested = workdayRoutes.reduce( (acc, route) => { const group = (acc[route.group] ??= {}); group[route.name] = workdayOperations[route.name]; return acc; }, {} as Record<WorkdayGroup, Record<string, WorkdayEndpoint>>, );If the named exports must keep precise per-group types, keep the literal object but add an assertion that every route name appears exactly once:
type NestedNames = { [G in keyof typeof workdayEndpointsNested]: keyof (typeof workdayEndpointsNested)[G]; }[keyof typeof workdayEndpointsNested]; // Fails to compile if a route is missing from workdayEndpointsNested. const _completeness: Record<WorkdayRouteName, true> = {} as Record< NestedNames, true >;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/operations.ts` around lines 9 - 180, Update workdayEndpointsNested to derive its groups and operation entries from workdayRoutes, preserving the existing workdayOperations mappings and exported access shape where practical. Ensure every route name is represented automatically; alternatively, retain the literal object but add a compile-time assertion using workdayRoute names that fails when any route is missing or duplicated.packages/workday/endpoints/factory.ts (2)
160-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInput validation failures and connection failures are not logged.
.parseat Lines 164-167,resolveConnectionat Line 170, andresolvePathat Line 171 all run before thetryblock. If any of them throws, thefinallyblock never runs, sologEventFromContextrecords nothing. Every other failure path recordsstatus: 'failed'.Move these calls inside the
tryblock so that failures produce a log event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/factory.ts` around lines 160 - 171, Move the input parsing and connection/path resolution calls currently before the try block into that try block, preserving their existing order and behavior. Update the surrounding flow in the endpoint handler containing WorkdayEndpointInputSchemas, resolveConnection, and resolvePath so exceptions from all three operations reach the existing catch/finally logging path and produce the same failed event as other errors.
53-67: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the positional mapping between path placeholders and
pathParams.
resolvePathmaps the Nth{...}placeholder toroute.pathParams[index]and ignores the placeholder name unless the array is shorter. For all current routes the placeholder names equal thepathParamsentries in the same order, so the behavior is correct today. A future route whosepathParamsorder differs from the placeholder order would resolve the wrong value silently.Prefer name-based lookup, and keep the positional path only as a fallback.
♻️ Proposed refactor
let index = 0; return path.replace(/\{([^}]+)\}/g, (_, placeholder: string) => { - const mappedKey = route.pathParams[index]; index += 1; - const key = mappedKey ?? placeholder; + const key = route.pathParams.includes(placeholder) + ? placeholder + : (route.pathParams[index - 1] ?? placeholder); const value = resolvePathParam(input, key) ?? resolvePathParam(input, placeholder); return encodePathPart(value); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/endpoints/factory.ts` around lines 53 - 67, Update resolvePath so each placeholder first resolves using its own name, while retaining the positional route.pathParams[index] lookup only as a fallback when name-based resolution is unavailable. Preserve encoding and existing behavior for routes whose names and positional entries match.packages/workday/webhooks/triggers.ts (1)
14-34: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the payload with the matching event schema.
WorkdayTriggerEventSchemasinpackages/workday/webhooks/types.tsdefines a schema for every event, but the handler never parses the request. The handler declares the payload asWorkdayWebhookOutputs[K], so consumers receive a typed value that was never checked at runtime. A bridge that posts a malformed body then passes verification and reaches downstream code with the wrong shape.Parse the payload after signature verification and return 400 when parsing fails.
♻️ Proposed refactor to validate the payload
-import { - createWorkdayEventMatch, - verifyWorkdayWebhookSignature, -} from './types'; +import { + createWorkdayEventMatch, + verifyWorkdayWebhookSignature, + WorkdayTriggerEventSchemas, +} from './types';if (!v.valid) { return { success: false, statusCode: 401, error: v.error || 'Signature verification failed', }; } + const parsed = WorkdayTriggerEventSchemas[eventType].safeParse( + request.payload, + ); + if (!parsed.success) { + return { + success: false, + statusCode: 400, + error: `Invalid ${eventType} payload`, + }; + } return { success: true };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/webhooks/triggers.ts` around lines 14 - 34, Update the handler around verifyWorkdayWebhookSignature to parse the verified request with the matching schema from WorkdayTriggerEventSchemas. Perform schema validation only after signature verification, return success false with statusCode 400 when parsing fails, and pass the parsed payload downstream so the WorkdayWebhookOutputs[K] value is runtime-validated.packages/workday/webhooks/types.ts (1)
83-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeader reads assume lowercase keys and scalar values. Both sites look up Workday headers with a lowercase literal key and assume a specific value shape. HTTP header names are case-insensitive, and the Corsair contract types header values as
string | string[] | undefined. Add one shared header helper in the Workday package that lowercases the lookup key and collapses array values, then use it at both sites.
packages/workday/webhooks/types.ts#L83-L87: readx-workday-eventthrough the shared helper instead of a direct lowercase index and atypeof === 'string'check.packages/workday/index.ts#L953-L958: replace the twoinchecks with the shared helper soX-Workday-SignatureandX-Workday-Eventalso match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/webhooks/types.ts` around lines 83 - 87, The Workday webhook header checks need case-insensitive lookup and support for Corsair’s string-or-array header values. Add one shared header helper in the Workday package that lowercases the lookup key and collapses array values, then use it in packages/workday/webhooks/types.ts:83-87 for x-workday-event and packages/workday/index.ts:953-958 for X-Workday-Signature and X-Workday-Event, replacing the direct index and in checks while preserving the existing matching behavior.packages/workday/index.ts (1)
166-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain
workdayEndpointSchemaswithRequiredPluginEndpointSchemas.Import the type from
corsair/coreand usesatisfies RequiredPluginEndpointSchemas<typeof workdayEndpointsNested>so new endpoint paths require schema entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workday/index.ts` around lines 166 - 170, Import RequiredPluginEndpointSchemas from corsair/core and constrain the workdayEndpointSchemas object with satisfies RequiredPluginEndpointSchemas<typeof workdayEndpointsNested>, ensuring every endpoint path has a corresponding schema entry while preserving the existing schema mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/workday/api.test.ts`:
- Around line 242-251: Update packages/workday/api.test.ts lines 242-251 to
exercise the webhook processing path with a known valid x-workday-signature and
assert that invalid and missing signatures are rejected; do not rely only on
trigger.match. Update packages/workday/api.test.ts lines 71-78 to assert
registration of the required worker.updated, worker.created, and job.changed
triggers, while preserving the existing trigger assertions as applicable.
In `@packages/workday/client.ts`:
- Around line 25-47: Update normalizeWorkdayHost to validate scheme-less inputs
as a hostname with an optional port and no path, userinfo, or other URL
components before returning them. When a value contains ://, rethrow any URL
parsing failure instead of falling through to the raw trailing-slash result,
while preserving the HTTPS-only requirement and normalized host return.
In `@packages/workday/endpoints/factory.ts`:
- Around line 191-198: Update the finally block around logEventFromContext so a
logging rejection cannot replace an existing error from makeWorkdayRequest or
output validation. Preserve the original operation error while handling or
suppressing logging failures, and keep successful request behavior unchanged.
- Around line 102-125: Update requestBody to always exclude the pagination keys
limit and offset, regardless of route.queryParams. Also exclude the optional id
and workerId alias fields unconditionally by including their declared aliases in
the filtered key set, while preserving existing path, query, control-key, and
undefined-value filtering.
In `@packages/workday/endpoints/routes.ts`:
- Around line 88-100: Remove the limit and offset entries from
getJobById.queryParams, leaving the single-resource GET route with only its ID
path parameter and existing metadata.
- Around line 1446-1456: Correct the Workday route definitions consumed by
getWorkerInfo and the associated service/version mappings: replace Compensation
v3 /workers/{ID} with the documented service and version, and update the
Recruiting and Staffing entries to documented versions for their configured
paths. Keep workdayRouteByName and getWorkdayRoute unchanged.
In `@packages/workday/endpoints/types.ts`:
- Around line 22-23: Apply z.coerce-based pagination validation to every limit
and offset field in WorkdayEndpointInputSchemas, rather than only updating the
unused PaginationQueryShape. Replace all duplicated declarations or reuse the
shared coercing shape, preserving the existing integer and range constraints; if
supporting Zod versions below 3.20 is required, raise the peer dependency
minimum accordingly.
In `@packages/workday/error-handlers.ts`:
- Around line 6-10: Update the rate-limit matcher in the error handler’s `match`
function to stop treating arbitrary occurrences of “429” in error messages as
rate limits. Preserve the existing ApiError status check and only accept a
delimited status-code token or another exact status representation, so unrelated
IDs, paths, and timestamps do not trigger retries.
In `@packages/workday/index.ts`:
- Around line 914-920: Update the OAuth configuration flow around
workdayOAuthUrls so authType === 'oauth_2' requires an explicit host, just as
tenant is required. Remove the wd2-impl-services1.workday.com fallback and
validate or reject missing host before constructing OAuth URLs, while preserving
the existing tenant-specific URL generation for valid configurations.
In `@packages/workday/package.json`:
- Around line 16-19: Create packages/workday/tsconfig.typecheck.json extending
the existing tsconfig.json and override emitDeclarationOnly to false. Update the
typecheck script in package.json to run tsc --noEmit --project
tsconfig.typecheck.json, while leaving the build configuration and scripts
unchanged.
In `@packages/workday/webhooks/types.ts`:
- Around line 106-113: Update the HMAC verification flow around
verifyHmacSignature to require request.rawBody as a string; if it is absent,
treat verification as failed instead of serializing request.payload. Preserve
signature verification only against the exact raw body received.
- Around line 83-87: Update the webhook event-header matching in the returned
predicate to locate x-workday-event case-insensitively and accept both string
and string[] header values, matching eventType when any supplied value matches.
Apply the same case-insensitive header lookup and array-aware matching in the
related logic near the workday webhook handling entry point, while preserving
body-based matching behavior.
---
Nitpick comments:
In `@packages/workday/endpoints/cache-sync.ts`:
- Around line 113-123: Update cacheItems so the single-resource fallback only
returns [response] when the record does not contain collection-shaped fields
such as total, data, or array-valued properties; otherwise return an empty list.
Preserve the existing array filtering and configured rule.listKeys handling.
- Around line 186-192: Update the cache upsert loop in the request path around
client.upsertByEntityId to avoid serial awaits: execute item upserts
concurrently with a bounded concurrency mechanism, or use an available
batch-upsert API. Preserve entityId filtering and ensure all scheduled upserts
complete before the surrounding operation returns.
- Around line 134-148: Resolve the unreachable delete flow by either adding the
DELETE/destructive routes it supports or, if those are intentionally deferred,
retain the implementation and add a comment documenting that follow-up intent.
Also align cacheDeleteEntityIds with its single-value behavior by renaming it to
cacheDeleteEntityId and returning one optional ID, updating all call sites
accordingly.
- Around line 159-173: Extract the inline database client shape from
syncWorkdayOperationCache into a named type placed alongside the CacheEntity
union, then use that type for the function’s ctx.db contract instead of casting
ctx.db inline. Preserve the existing optional upsertByEntityId and
deleteByEntityId methods and entity lookup behavior.
In `@packages/workday/endpoints/factory.ts`:
- Around line 160-171: Move the input parsing and connection/path resolution
calls currently before the try block into that try block, preserving their
existing order and behavior. Update the surrounding flow in the endpoint handler
containing WorkdayEndpointInputSchemas, resolveConnection, and resolvePath so
exceptions from all three operations reach the existing catch/finally logging
path and produce the same failed event as other errors.
- Around line 53-67: Update resolvePath so each placeholder first resolves using
its own name, while retaining the positional route.pathParams[index] lookup only
as a fallback when name-based resolution is unavailable. Preserve encoding and
existing behavior for routes whose names and positional entries match.
In `@packages/workday/endpoints/operations.ts`:
- Around line 9-180: Update workdayEndpointsNested to derive its groups and
operation entries from workdayRoutes, preserving the existing workdayOperations
mappings and exported access shape where practical. Ensure every route name is
represented automatically; alternatively, retain the literal object but add a
compile-time assertion using workdayRoute names that fails when any route is
missing or duplicated.
In `@packages/workday/endpoints/routes.ts`:
- Around line 164-206: Resolve the duplicate GET route definitions for
/values/jobChangesGroup/reason by either removing one of
getJobChangeReasonValues or getJobChangeReasons, or, if they are intentional
aliases, adding the established alias comment and making both queryParams lists
identical. Preserve a single consistent endpoint contract and output schema.
In `@packages/workday/endpoints/types.ts`:
- Around line 21-24: Replace the unused PaginationQueryShape and repeated inline
query-value unions in the endpoint schemas with shared FilterValue and
PaginationQueryShape helpers. Update each affected z.object definition,
including getJobById and getJobChangeFrequencies, to spread these helpers while
preserving endpoint-specific fields and passthrough behavior; keep the existing
limit and offset bounds centralized in PaginationQueryShape.
In `@packages/workday/index.ts`:
- Around line 166-170: Import RequiredPluginEndpointSchemas from corsair/core
and constrain the workdayEndpointSchemas object with satisfies
RequiredPluginEndpointSchemas<typeof workdayEndpointsNested>, ensuring every
endpoint path has a corresponding schema entry while preserving the existing
schema mappings.
In `@packages/workday/webhooks/triggers.ts`:
- Around line 14-34: Update the handler around verifyWorkdayWebhookSignature to
parse the verified request with the matching schema from
WorkdayTriggerEventSchemas. Perform schema validation only after signature
verification, return success false with statusCode 400 when parsing fails, and
pass the parsed payload downstream so the WorkdayWebhookOutputs[K] value is
runtime-validated.
In `@packages/workday/webhooks/types.ts`:
- Around line 83-87: The Workday webhook header checks need case-insensitive
lookup and support for Corsair’s string-or-array header values. Add one shared
header helper in the Workday package that lowercases the lookup key and
collapses array values, then use it in packages/workday/webhooks/types.ts:83-87
for x-workday-event and packages/workday/index.ts:953-958 for
X-Workday-Signature and X-Workday-Event, replacing the direct index and in
checks while preserving the existing matching behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8718de06-7e97-48b2-940c-b43e9d095d73
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
packages/corsair/core/constants.tspackages/workday/api.test.tspackages/workday/client.tspackages/workday/endpoints/cache-sync.tspackages/workday/endpoints/factory.tspackages/workday/endpoints/index.tspackages/workday/endpoints/operations.tspackages/workday/endpoints/routes.tspackages/workday/endpoints/types.tspackages/workday/error-handlers.tspackages/workday/index.tspackages/workday/jest.config.cjspackages/workday/mocks/corsair-core-mock.tspackages/workday/package.jsonpackages/workday/schema/database.tspackages/workday/schema/index.tspackages/workday/tsconfig.jsonpackages/workday/tsconfig.test.jsonpackages/workday/tsup.config.tspackages/workday/webhooks/index.tspackages/workday/webhooks/oauth-tenant-link.tspackages/workday/webhooks/tenant-matcher.tspackages/workday/webhooks/triggers.tspackages/workday/webhooks/types.ts
3 participants
Description
This PR introduces the official Workday Integration Plugin (
packages/workday) to the Corsair monorepo.Key implementation details:
.catchall(z.unknown())schemas to ensure stable data flow without strict upfront typing (in compliance with rule R7 for zeroanyusage).worker.updatedevent matcher to demonstrate standard Workday webhook lifecycle handling.api.test.ts, restricts modifications solely to the authorized plugin boundaries, and correctly hooks intopackages/corsair/core/constants.ts.Closes #474
Checklist
Before submitting your PR, please verify the following:
pnpm lintand all checks passpnpm typecheckand there are no TypeScript errorspnpm buildand all packages build successfullypnpm testand all tests passScreenshots / Demos (if applicable)
Additional Notes
ts-jestcan occasionally throw local execution errors on.jsfiles due to global ESM/CommonJS merging settings injest.config.cjs, but this does not affect the actual static typing, build step, or core plugin architecture.mainwithout capturing any transient history.Summary by CodeRabbit