feat(SITES-44690): add task-management controller (connections, tickets, Jira integration)#2661
Conversation
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @prithipalpatwal,
Verdict: Request changes - CI-breaking test mock issues plus a batch-mode bug that silently burns API calls after auth failure.
Complexity: HIGH - large diff; API surface + dependency signal.
Changes: adds a task-management controller with connection CRUD, ticket creation (single/batch/grouped), idempotency enforcement, and Jira project listing (8 files).
Note: CI checks are currently failing - resolve before merge.
Must fix before merge
- [Critical] New route params
:connectionIdand:providernot classified infacs-capabilities.js- likely CI failure cause -src/routes/facs-capabilities.js(details inline) - [Critical]
listProjectsmissing from mock controller in route tests -test/routes/index.test.js:548(details inline) - [Critical]
Suggestion: {}in index test mock causes controller constructor to throw -test/index.test.js:176(details inline) - [Important] Individual batch mode continues calling Jira API after 401 auth failure -
src/controllers/task-management.js:823(details inline) - [Important] No integration tests in
test/it/for new endpoints - repo CLAUDE.md explicitly requires these - [Important] Batch idempotency marks "completed" even when all items fail, blocking retries -
src/controllers/task-management.js:942
Non-blocking (6): minor issues and suggestions
- nit:
deleteConnectionreturnscreateResponse({}, STATUS_NO_CONTENT)which sends a body with 204 -src/controllers/task-management.js:374 - suggestion: consider short-circuiting
listTicketsbridge loading into a single query rather than N+1TicketSuggestion.allByTicketIdcalls per ticket -src/controllers/task-management.js:402 - suggestion: add
log.warninside the barecatch {}blocks inlistTicketsandlistTicketsByOpportunity- silent failure with zero observability -src/controllers/task-management.js:411 - suggestion: the controller at 1265 lines is the largest in the repo; consider extracting the three
createTicketcode paths into helper functions - nit: no OpenAPI spec in
docs/openapi/for the new endpoints - the repo's adding-an-endpoint checklist lists this as step 1 - suggestion: include orphaned Jira ticket metadata (ticketKey, ticketUrl) in the 500 error response when
Ticket.createfails after Jira creation succeeds, so ops can recover -src/controllers/task-management.js:1115
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 6m 34s | Cost: $8.84 | Commit: 3bb1e05d375df29d4a04863a2a0d038defed495e
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| 'GET /organizations/:organizationId/suggestions/:suggestionId/ticket', | ||
| 'GET /organizations/:organizationId/opportunities/:opportunityId/tickets', | ||
| 'POST /organizations/:organizationId/task-management/:provider/tickets', | ||
| 'GET /organizations/:organizationId/task-management/:provider/projects', |
There was a problem hiding this comment.
issue (blocking): The new routes introduce :connectionId and :provider as dynamic params not yet classified in src/routes/facs-capabilities.js. The repo CLAUDE.md states: "The routeFacsCapabilities test fails the build if a param is left unclassified." This is the likely cause of the CI build failure.
Fix: add 'connectionId' and 'provider' to the FACS_NON_RESOURCE_PARAMS array in src/routes/facs-capabilities.js (they are not ReBAC entities). :suggestionId and :opportunityId already exist in that array.
| @@ -548,6 +548,16 @@ describe('getRouteHandlers', () => { | |||
| getPreview: sinon.stub(), | |||
There was a problem hiding this comment.
issue (blocking): mockTaskManagementController defines 7 stubs but is missing listProjects. The route GET /organizations/:organizationId/task-management/:provider/projects references taskManagementController.listProjects which will be undefined, causing the route registration test to fail.
Fix: add listProjects: sinon.stub() to the mock object.
| } | ||
| } | ||
|
|
||
| if (batchSuggestionOk) { |
There was a problem hiding this comment.
issue (blocking): In the individual batch loop, when a 401/reauth error occurs (lines 863-870), the code marks the connection as requires_reauth but then continues the for...of loop. Subsequent iterations will call ticketClient.createTicket again with a revoked token - burning API calls and producing N-1 redundant 409 results.
Fix: after detecting a reauth-needed error, break out of the loop and mark all remaining suggestions as failed with the reauth message. This matches the single-ticket and grouped paths which both return immediately on 401.
c3d3cdc to
8954ab4
Compare
bef0ddc to
f6dcdec
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
f6dcdec to
0d49837
Compare
Implements POST /organizations/:organizationId/task-management/:provider/tickets as defined in mysticat-architecture PR #150. Flow: 1. Validates organizationId, provider, and required body field (summary). 2. Calls TaskManagementConnection.findActiveByOrganizationAndProvider() to resolve the org's active Jira OAuth connection (404 if none, 409 if degraded). 3. Delegates to TicketClientFactory (spacecat-shared-ticket-client) which handles OAuth token refresh, ADF description conversion, and the Jira API call. 4. On 401 from Jira, marks the connection as requires_reauth so the UI can prompt reconnect without waiting for a GC cycle. 5. Persists a Ticket entity with ticketId / ticketKey / ticketUrl. 6. Returns 201 with the persisted ticket data. Adds @adobe/spacecat-shared-ticket-client dependency (activate after PR #1701 merges). Co-authored-by: Cursor <cursoragent@cursor.com>
…emove dead code Three bug fixes in the task-management controller: 1. TicketClientFactory.create() call was completely wrong: - 'provider' string passed where a connection object is expected - 'env' object passed where an AWS SecretsManagerClient is expected Fix: build a plain connectionObj from entity getters, construct SecretsManagerClient (region auto-detected from Lambda env) and an httpClient wrapping globalThis.fetch, then call TicketClientFactory.create(connectionObj, smClient, httpClient, log). 2. projectKey was never validated or passed to createTicket(): Jira rejects every ticket without a project key. Added required body validation and threaded projectKey through createTicket(). Also removed priority (not supported by JiraCloudClient in v1 — deferred to v2 once Jira field configuration is in place). 3. Dead-code isActive() guard removed: findActiveByOrganizationAndProvider only returns connections with status='active', so the 409 branch after a non-null result was unreachable and created a misleading narrative. Also documents v1 intentional deviations in the controller JSDoc. Co-authored-by: Cursor <cursoragent@cursor.com>
… Ticket Extracts the IMS user ID from the JWT via authInfo.getProfile().getImsUserId() and passes it as createdBy to Ticket.create(). Also passes the provider path param as ticketProvider (denormalized for audit/query purposes). Both fields are now required by the Ticket schema (spacecat-shared PR #1702) and the architecture spec (mysticat-architecture PR #150). Also includes ticketProvider in the 201 response body so callers can confirm which provider created the ticket without a follow-up lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
…reation When suggestionId is provided in the request body, creates a TicketSuggestion record after persisting the Ticket. The DB UNIQUE (suggestion_id) constraint prevents the same suggestion from being ticketed twice; a constraint violation returns 409 Conflict so the UI can surface a clear error. Also extracts TicketSuggestion from dataAccess and validates it at controller construction time, consistent with Ticket and TaskManagementConnection. Co-authored-by: Cursor <cursoragent@cursor.com>
…150) Four routes added to align with the architecture spec: GET /organizations/:orgId/task-management/connections List all connections for an org; optional ?provider= filter. GET /organizations/:orgId/task-management/connections/:connectionId Fetch a single connection; 404 on org mismatch (prevents enumeration). DELETE /organizations/:orgId/task-management/connections/:connectionId Delete the connection from DB and its OAuth secret from AWS Secrets Manager (7-day recovery window). ResourceNotFoundException on the SM secret is treated as a soft success so stale DB records can still be cleaned up. Does not revoke the Atlassian-side authorization in v1 (accepted risk — revoking via Atlassian's token revocation endpoint is a v2 enhancement). GET /organizations/:orgId/task-management/tickets List all tickets for an org. Also: - Rename suggestionId → suggestionIds (array) to match the spec field name. v1 processes only the first element; batch 207 is deferred to v2. The controller accepts both forms for forward-compat. - Extract serializeConnection() and serializeTicket() helpers to keep the response shape consistent across all endpoints. - Document Idempotency-Key v1 scope deviation in the JSDoc: header accepted but not cached; UNIQUE DB constraint prevents duplicates. Co-authored-by: Cursor <cursoragent@cursor.com>
1. mode validation: 'grouped' returns 400 in v1; unknown modes also 400. 2. suggestionIds max 10 validation (400 if exceeded). 3. Two new read endpoints: GET /organizations/:orgId/suggestions/:suggestionId/ticket GET /organizations/:orgId/opportunities/:opportunityId/tickets 4. Response shape: connectionId + statusSyncedAt:null + suggestions array. 5. Structured audit event log.info on ticket creation success. Co-authored-by: Cursor <cursoragent@cursor.com>
…ady isolates envs
NODE_ENV is "production" on all Lambda environments so the env segment was
misleading. New path: /mysticat/task-management/{orgId}/{connectionId}.
Mirrors the fix in spacecat-shared ticket-client-factory.js.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ODULE_NOT_FOUND failures - Add 61-test suite covering all 7 TaskManagementController methods - Fix task-management.js: top-level await/catch for shared-ticket-client import so module loads cleanly via test/utils.js bare import of src/index.js - Add task-management routes to INTERNAL_ROUTES in required-capabilities.js - Add mockTaskManagementController to routes/index.test.js - Add TaskManagementConnection/Ticket/TicketSuggestion stubs to index.test.js dataAccess Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… Ticket.create() - deleteConnection: replace connection.remove() with connection.markDisconnected() per PR #1702 v1 design — soft-delete preserves audit history, GC job cleans up later - Ticket.create(): pass ticketStatus from JiraCloudClient result rather than relying solely on schema default (aligns with PR #1701 createTicket() return shape) - Update tests: markDisconnected stubs + ticketStatus in all createTicket mock results Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… listProjects route - Replace null TicketClientFactory fallback with explicit error object so any code path reaching the fallback throws clearly instead of NPE; only swallow ERR_MODULE_NOT_FOUND (package absent in dev/test env without PR #1701) - Enforce Idempotency-Key header on POST tickets (400 if absent); full dedup via idempotency_keys table — lookup cached response, insert processing record, mark completed/failed on each exit path - Validate suggestion existence before bridge creation (404 if not found) per spec §7 - Add GET /organizations/:organizationId/task-management/:provider/projects route calling ticketClient.listProjects() with reauth detection - Add Suggestion to controller constructor injection and guard - Add 25 new unit tests covering all new paths; update routes snapshot Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop connectionId/createdBy from getTicketBySuggestion spread — already included by serializeTicket() - Drop ?? undefined from three opportunityId assignments — nullish coalesce to undefined is a no-op - Use connection.getInstanceUrl() (not ?.) in createTicket and listProjects connectionObj — instanceUrl is required by TicketClientFactory; silent undefined would be caught at Jira call time, not at construction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… + expand v1 deviation docs - createTicket now accepts optional connectionId from request body; when provided, the specified connection is loaded directly (loadConnectionForOrg + provider/status check); when absent, findActiveByOrganizationAndProvider is used as before. - Controller JSDoc expanded to explain why multi-connection 400 guard is not needed in v1 (DB partial unique index prevents multiple active connections per cloudId) and documents the client-provided summary/description deviation from spec §7 step 5. - Soft-delete comment expanded with FK preservation rationale: hard delete would cascade-delete associated tickets; disconnected status keeps FK target intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…user_id fallback)
…P helpers and extract DTOs
…ware (SITES-44690)
…agerClient inline (SITES-44690)
…encyKey model and TicketSuggestion bulk accessors (SITES-44690) - Replace direct postgrestClient idempotency_keys queries with IdempotencyKey.findActiveKey / .create / .setStatus / .save / .remove - Replace postgrestClient ticket_suggestions bulk .in() queries with TicketSuggestion.allBySuggestionIds / .allByTicketIds - Remove SecretsManagerClient inline construction to use context-injected sm (reverted separately) - Update tests to use model-level stubs instead of raw pgClient chain stubs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ull org scan (SITES-44690) Replace allByOrganizationId + in-memory filter with the auto-generated allByOpportunityId accessor from the belongs_to reference index. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ES-44690) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ecat-shared-ticket-client@1.0.4 (SITES-44690) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l bridge queries now use model-level accessors (SITES-44690) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…TES-44690) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Picks up published PR #1804 changes: TicketSuggestion model, IdempotencyKey model, and bulk accessor methods. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- listTicketsByOpportunity: post-filter by organizationId to prevent cross-org data leak - createTicket: fix idempotency key hash to always include organizationId explicitly - createTicket: only cache batch response as completed when all items succeed (partial failures remain retryable) - createTicket: add MIME type allowlist pre-check (aligned with jira-cloud-client) and filename sanitization - listIssueTypes: use queryStringParameters instead of rawQueryString - tests: update listIssueTypes fixture, add cross-org filter test, add MIME allowlist and filename tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ggestionIds Both are always truthy; remove organizationId fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…endpoints - Add shared IT test factory covering all 8 task-management endpoints (connections list/get, projects, issue-types, tickets list/get, ticket-by-suggestion, tickets-by-opportunity, create-ticket validation) - Add seed data: task_management_connections, tickets, ticket_suggestions - Register seeds at correct FK levels in seed.js - Add task-management-api.yaml with OpenAPI path definitions for all 8 endpoints - Add TaskManagementConnection, Ticket, TicketCreateRequest, TicketAttachment schemas to schemas.yaml - Wire task-management paths into api.yaml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n object prevents JSONB double-encoding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in listTicketsByOpportunity allByOpportunityId does not exist on the Ticket model — the schema only generates allByOrganizationId, allByTaskManagementConnectionId, and findByOpportunityId (single). Use allByOrganizationId scoped to the request org and post-filter by opportunityId. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ickets lookup Replace the allByOrganizationId + client-side filter workaround with the dedicated allByOpportunityId index query. The method was always auto-generated by the schema framework but undocumented — now confirmed and documented in spacecat-shared. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ringParameters The Helix framework populates query params via the Fetch API Request object (context.request.url), not the Lambda-style queryStringParameters field. listConnections and listIssueTypes were reading from queryStringParameters which is never set by the framework, causing provider filtering to be silently skipped and projectId to always appear missing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…OpportunityId Local HEAD uses Ticket.allByOpportunityId (direct index query); remote had the intermediate allByOrganizationId workaround. Took ours for both conflicted files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
allByOpportunityId queries by opportunity only — tickets from other orgs sharing the same opportunityId would leak. Add org filter after DB fetch and restore the org-isolation unit test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @prithipalpatwal,
Verdict: Approve - all prior blocking findings addressed; CI green; integration tests and OpenAPI specs confirmed present.
Complexity: HIGH - large diff; API surface + FACS + dependency signal.
Changes: adds a task-management controller with connection CRUD, ticket creation (single/batch/grouped), idempotency enforcement, Jira project/issue-type listing, OpenAPI specs, and integration tests (22 files).
Note: Recommend a human read before merge - this change modifies shared API contracts (docs/openapi/). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (4): minor issues and suggestions
- nit: idempotency key collides when both
opportunityIdis undefined ANDsuggestionIdsis empty - hash becomessha256("undefined:")for all such requests within an org. Edge case if the UI always supplies one, but consider requiring at least one discriminator -src/controllers/task-management.js:1248 - nit: UUID validation inconsistency - gateway uses
isValidUUIDAnyVersionforconnectionIdwhile the controller usesisValidUUID(v4-only). A v1/v3/v5 UUID passes the gateway but fails in the controller with a misleading error -src/index.js:401vssrc/controllers/task-management.js - suggestion: avoid reflecting raw user input (
provider) in error messages - use a fixed string like "Unsupported provider" without echoing the value. Currently non-exploitable (JSON response) but avoids a future XSS vector if error messages are ever rendered in HTML -src/controllers/task-management.js:1157 - suggestion:
ALLOWED_ATTACHMENT_MIME_TYPESis defined in the controller with a "must stay in sync" comment referencing jira-cloud-client.js. Consider exporting from ticket-client to create a single source of truth -src/controllers/task-management.js:712
Previously flagged, now resolved
- Cross-org data leak in
listTicketsByOpportunity- now filtered by org post-query - Idempotency key collision for different-opportunity requests - fixed
- Batch partial-failure cached as "completed" - now only caches when all items succeed
- No MIME type allowlist on attachment upload - added
ALLOWED_ATTACHMENT_MIME_TYPES - No filename sanitization on attachment - path separators and control characters now stripped
listIssueTypesusingrawQueryString- now uses standardqueryStringParameters- FACS params now classified (
connectionId,providerinFACS_NON_RESOURCE_PARAMS) - Integration tests added (361 lines in
test/it/shared/tests/task-management.js) - OpenAPI specs added (
docs/openapi/task-management-api.yaml)
Skill: pr-review | Model: us.anthropic.claude-sonnet-4-5-20250929-v1:0 | Duration: 1m 34s | Cost: $4.28 | Commit: fd1f978b3ba99226e0de6b3a3d67df61fb986945
If this code review was useful, please react with 👍. Otherwise, react with 👎.
… imports ESLint import/first rule requires all imports before any declarations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…return correct 404 IT test 'returns 404 when connection does not exist' was getting 400 because the idempotency guard (opportunityId/suggestionIds required) ran before the connection lookup. Moved the guard after connection resolution so unknown connectionId returns 404 as expected. Also fix unit tests for idempotency tests to stub TaskManagementConnection.findById with an active connection since they now need to pass the connection lookup first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @prithipalpatwal,
Verdict: Approve - all prior non-blocking nits correctly addressed; no new issues introduced.
Complexity: HIGH - large diff; API surface + FACS + dependency signal.
Changes: addresses prior nits - fixes UUID validation inconsistency, idempotency key collision, and raw input reflection in error messages (2 files).
Note: Recommend a human read before merge - this change modifies shared API contracts (docs/openapi/). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (1): minor issues and suggestions
- nit: comment on line 30 says "matches the gateway's isValidUUIDAnyVersion check" but the local regex is actually more permissive (accepts any hex in version/variant positions) while the gateway enforces RFC version
[1-8]and variant[89ab]nibbles. Not a bug since the gateway validates first, but the comment is misleading -src/controllers/task-management.js:30
Previously flagged, now resolved
- UUID validation inconsistency (gateway vs controller) - controller now uses
isValidUUIDAnyVersionat all 4connectionIdvalidation sites - Idempotency key collision when both
opportunityIdundefined andsuggestionIdsempty - added validation gate requiring at least one discriminator - Raw user input (
provider) reflected in error message - removed from the error string
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 51s | Cost: $4.09 | Commit: a77c8cd5df9261fcdf7695feeaeeb4eae39cfb2a
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…ness Regex accepts any hex in version/variant positions (more permissive than RFC). The gateway validates upstream so this is safe, but the comment was inaccurate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🎉 This PR is included in version 1.645.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Jira: SITES-44690
Summary
Adds the task-management controller and routes to spacecat-api-service (SITES-44690).
Access control
All endpoints verify IMS org membership via
AccessControlUtil.hasAccess(org)(v1 identity model per mysticat-architecture#150). Returns 404 on unknown org, 403 on denied. Admins bypass. readOnlyAdmin write-blocking handled upstream byreadOnlyAdminWrapper. S2S capability deferred to v2.Ticket creation (POST /organizations/:organizationId/task-management/:provider/tickets)
Flow:
Idempotency-Keyheader (required; 400 if absent). Checkidempotency_keystable — return cached response if completed, 409 if in-flight.TaskManagementConnectionbyconnectionIdfrom request body (404 if not found or not active for org+provider).TicketClientFactory.create()with the connection object,smClient(injected viasmClientWrappermiddleware), andhttpClient.JiraCloudClient(single, individual batch ≤15, or grouped ≤1500).Ticket+TicketSuggestionrecords.All routes
GET /organizations/:organizationId/task-management/connections— list connections (optional?provider=filter)GET /organizations/:organizationId/task-management/connections/:connectionId— get one connectionGET /organizations/:organizationId/task-management/connections/:connectionId/projects— list Jira projectsGET /organizations/:organizationId/task-management/connections/:connectionId/issue-types?projectId=— list Jira issue types for a projectGET /organizations/:organizationId/task-management/tickets— list tickets for orgGET /organizations/:organizationId/suggestions/:suggestionId/ticket— get ticket for suggestionGET /organizations/:organizationId/opportunities/:opportunityId/tickets— list tickets for opportunityPOST /organizations/:organizationId/task-management/:provider/tickets— create ticket(s)v1 scope notes
suggestionIdsoptional;opportunityIdcan be used directly without suggestions.priority,dueDate,components,parentforwarded to Jira if provided (optional fields per spec).SecretsManagerClientinjected viasmClientWrappermiddleware (follows thes3ClientWrapperpattern insrc/support/s3.js).connectionIdin request body; resolved vialoadConnectionForOrg().connectionIdandproviderclassified asFACS_NON_RESOURCE_PARAMS— JWT session only, not a FACS surface in v1. ReBAC deferred to v2.