feat(NEX-23): add copyFlow via ProcessFlow REST API

[sonisoft-cnanda]

Summary

• FlowManager.copyFlow() — Copies an existing flow into a target scoped application via POST /api/now/processflow/flow/{id}/copy
• The copied flow lands in draft/unpublished state, enabling the agent workflow: copy → modify → test → publish
• Reuses ProcessFlowRequest and ProcessFlowApiResponse from NEX-24 — no new HTTP infrastructure needed
• New model types: CopyFlowOptions, FlowCopyResult

API Details

Endpoint: POST /api/now/processflow/flow/{source_sys_id}/copy?sysparm_transaction_scope={target_scope}

Payload: { "name": "<display name>", "scope": "<target_scope_sys_id>" }

Response: result.data = sys_id of the newly created flow copy

Test plan

• 9 new unit tests: validation, payload construction, identifier resolution, error handling
• 3 new integration tests against dev224436 — all passing:
  • Copy "Change - Standard" into "My Awesome App" scope — returns valid sys_id
  • Verify copy exists via getFlowDefinition() — name matches
  • Non-existent source flow — returns graceful failure
• All 525 FlowManager unit tests pass
• TypeScript compiles cleanly

Closes #23

🤖 Generated with Claude Code

[claude]

Overall the implementation is clean and follows established patterns. Good test coverage (unit + integration). A few issues worth addressing before merge.

Bug: dead / misleading error-check condition (FlowManager.ts line 610, also pre-existing at lines 433 and 532):

The condition if (apiResult.errorCode \!== 0 || (apiResult.errorCode == null && apiResult.errorMessage)) has a dead second clause. In JavaScript, null \!== 0 is true (strict inequality), so when errorCode is null the first condition is already true, making the second clause unreachable. If the API returns { errorCode: null, data: "new-sys-id" } for a successful copy, this wrongly returns { success: false }. The intended logic should use apiResult.errorCode \!= null && apiResult.errorCode \!== 0 for the first clause. This same bug exists pre-existing in getFlowDefinition (line 433) and testFlow (line 532); the PR copies that pattern.

Bug (minor): success path has no guard on data (FlowManager.ts line 620):

const newFlowSysId = apiResult.data as string; — if the API returns { errorCode: 0, data: null }, the method returns { success: true, newFlowSysId: null }, which callers will not expect. A runtime check should verify newFlowSysId is a non-empty string before returning success.

Nit: rawResponse missing from the HTTP-error catch block (lines 632-635): The API-error path (lines 612-617) includes rawResponse: apiResult but the outer catch block does not. Minor inconsistency with testFlow and getFlowDefinition patterns.

Nit: hardcoded instance-specific sys_ids in integration tests (FlowManager_IT.test.ts lines ~535-537): The fallback values for SOURCE_FLOW_SYS_ID and TARGET_SCOPE are specific to dev224436. In CI or any other instance these would silently use wrong IDs and produce confusing failures. Consider removing the hardcoded fallbacks or skipping the describe block when the env vars are absent.

What looks good:

• Method signature and JSDoc are clear and accurate
• All three required fields validated with .trim(), consistent with other methods
• _resolveFlowIdentifier correctly reused — no duplicate logic
• New model types (CopyFlowOptions, FlowCopyResult) are well-typed and documented
• Unit tests cover all important paths: validation, resolve failure, API error, HTTP error, and happy path with payload/query verification
• No new HTTP infrastructure needed — good reuse of ProcessFlowRequest

[claude]

This placeholder review was posted in error; please see the next comment for the actual code review.

[claude]

---

Code Review

Summary

Solid implementation that follows the established patterns in FlowManager. The bug fix applied to the existing getFlowDefinition and testFlow error-check conditions is a welcome improvement, and the new copyFlow method is consistent with the surrounding code. A few issues worth addressing:

---

Bug / Correctness

Hardcoded sys_ids in integration tests (medium)

The hardcoded fallback values (e89e3ade... and 4a5a6115...) are specific to dev224436 and will silently produce false failures or false passes in any other environment, including CI:

const SOURCE_FLOW_SYS_ID = process.env.TEST_COPY_SOURCE_FLOW || 'e89e3ade731310108ef62d2b04f6a744';
const TARGET_SCOPE       = process.env.TEST_COPY_TARGET_SCOPE || '4a5a6115402946939ee48e3fe80f60f8';

Consider requiring the env vars and skipping when absent rather than falling back to hardcoded values.

---

Best Practices

Integration tests leave artifacts behind (low)

Each test run creates one or two new flow copies but never cleans them up. Over repeated runs this accumulates junk flows on the target instance. An afterAll cleanup block would prevent this.

rawResponse: null in catch block is inconsistent (low)

The copyFlow catch block explicitly sets rawResponse: null, while the equivalent path in testFlow omits the field entirely. Since rawResponse?: unknown accepts both, pick one convention and apply it uniformly to avoid confusing callers who check if (result.rawResponse).

---

Positive Notes

• The fix to the error predicate is correct and important: the old errorCode !== 0 logic treated errorCode: undefined as an error, causing false failures when the API omits the field.
• The typeof newFlowSysId !== 'string' runtime guard after the as string cast is good defensive coding for a dynamic API response shape.
• targetScope intentionally appears in both the query string and the JSON body per the documented API contract.
• Unit test coverage is thorough: validation, happy path, API error, HTTP error, and null-data cases are all hit.