From e818394abd4fbfa6a476890ba9f546430e177082 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Tue, 22 Sep 2026 12:58:07 +0530 Subject: [PATCH 1/9] fix: block duplicate promotions --- command-snapshot.json | 1 + messages/devops.promote.md | 8 +++ schemas/devops-pipeline-create.json | 19 ++++++ src/commands/devops/pipeline/create.ts | 6 ++ src/commands/devops/promote.ts | 36 +++++++++- src/commands/devops/work-item/create.ts | 1 + src/utils/createPipeline.ts | 46 +++++++++++++ src/utils/createWorkItem.ts | 45 +++++++------ src/utils/promoteStage.ts | 45 +++++++++++++ test/commands/devops/pipeline/create.test.ts | 7 ++ test/commands/devops/promote.test.ts | 66 +++++++++++++++++++ test/commands/devops/work-item/create.test.ts | 3 + test/utils/createPipeline.test.ts | 52 +++++++++++++++ test/utils/createWorkItem.test.ts | 21 ++++++ test/utils/promoteStage.test.ts | 47 ++++++++++++- 15 files changed, 381 insertions(+), 22 deletions(-) diff --git a/command-snapshot.json b/command-snapshot.json index 89c83d54..85ec70e9 100644 --- a/command-snapshot.json +++ b/command-snapshot.json @@ -118,6 +118,7 @@ "api-version", "deploy-all", "flags-dir", + "force", "json", "skip-validation", "stage-id", diff --git a/messages/devops.promote.md b/messages/devops.promote.md index 277f87da..e243d34b 100644 --- a/messages/devops.promote.md +++ b/messages/devops.promote.md @@ -52,6 +52,14 @@ ID of the source pipeline stage whose approved work items will be promoted. Mutu Skip pre-promote validation. By default, promotion is validated before proceeding to prevent promoting work items without an associated PR. Use this flag to bypass validation. +# flags.force.summary + +Submit the promotion even if another promotion to the same target stage is already in progress. By default, the command blocks to avoid creating duplicate promotions. + +# error.PromotionInFlight + +A promotion to this stage is already in progress (request token%s: %s). Submitting now can create a duplicate promotion. Wait for it to finish (check with "%s devops request status --request-token "), or pass --force to promote anyway. + # error.NoModeFlag Provide either --work-item-id to promote specific work items or --stage-id to promote all approved work items from a stage. diff --git a/schemas/devops-pipeline-create.json b/schemas/devops-pipeline-create.json index 042b87e0..49f53209 100644 --- a/schemas/devops-pipeline-create.json +++ b/schemas/devops-pipeline-create.json @@ -20,6 +20,12 @@ "repository": { "$ref": "#/definitions/RepoInfo" }, + "stages": { + "type": "array", + "items": { + "$ref": "#/definitions/PipelineStageInfo" + } + }, "error": { "type": "string" } @@ -42,6 +48,19 @@ }, "required": ["repoUrl", "repoType", "created"], "additionalProperties": false + }, + "PipelineStageInfo": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "required": ["id"], + "additionalProperties": false } } } diff --git a/src/commands/devops/pipeline/create.ts b/src/commands/devops/pipeline/create.ts index 12aa4e53..8593bca7 100644 --- a/src/commands/devops/pipeline/create.ts +++ b/src/commands/devops/pipeline/create.ts @@ -185,6 +185,12 @@ export default class DevopsPipelineCreate extends SfCommand 0) { this.log(` Projects: ${projectIds.join(', ')}`); } + if (result.stages && result.stages.length > 0) { + this.log(' Stages:'); + for (const stage of result.stages) { + this.log(` ${stage.name ?? ''} (${stage.id})`); + } + } this.log(' Next steps:'); const orgLabel = username ?? ''; const pipelineIdLabel = result.pipelineId ?? ''; diff --git a/src/commands/devops/promote.ts b/src/commands/devops/promote.ts index 97eac967..852a3368 100644 --- a/src/commands/devops/promote.ts +++ b/src/commands/devops/promote.ts @@ -16,7 +16,12 @@ import { Connection, Messages } from '@salesforce/core'; import { SfCommand, Flags } from '@salesforce/sf-plugins-core'; -import { promoteStage, PromoteStageResult } from '../../utils/promoteStage.js'; +import { + promoteStage, + PromoteStageResult, + findInFlightPromotions, + InFlightPromotion, +} from '../../utils/promoteStage.js'; import { resolveProjectIdFromWorkItem } from '../../utils/prepareWorkItem.js'; import { getPipelineIdForProject } from '../../utils/pipelineUtils.js'; import { deployAll, testLevel as testLevelFlag, specificTestsNoChar } from '../../common/flags/promote/promoteFlags.js'; @@ -80,6 +85,10 @@ export default class DevopsPromote extends SfCommand { summary: messages.getMessage('flags.skip-validation.summary'), default: false, }), + force: Flags.boolean({ + summary: messages.getMessage('flags.force.summary'), + default: false, + }), }; private static async assertWorkItemsPromotable(connection: Connection, workItemIds: string[]): Promise { @@ -168,6 +177,10 @@ export default class DevopsPromote extends SfCommand { } } + if (!flags.force) { + await this.assertNoInFlightPromotion(connection, targetStageId); + } + if (!flags['skip-validation']) { await this.runValidation(connection, pipelineId, resolvedWorkItemIds, targetStageId, !workItemIds?.length); } @@ -211,6 +224,27 @@ export default class DevopsPromote extends SfCommand { }; } + /** + * Blocks promotion if another promotion to the same target stage is already in flight, to + * avoid creating a duplicate. Scoped to the target stage (and therefore its pipeline), so + * promotions in other stages or pipelines are unaffected. Best-effort: if the check can't + * run, the promotion proceeds as before. + */ + private async assertNoInFlightPromotion(connection: Connection, targetStageId: string): Promise { + let inFlight: InFlightPromotion[]; + try { + inFlight = await findInFlightPromotions(connection, targetStageId); + } catch { + // Guard failure must not break the existing promote flow. + return; + } + if (inFlight.length > 0) { + const tokens = inFlight.map((p) => p.requestToken).filter(Boolean); + const plural = tokens.length > 1 ? 's' : ''; + this.error(messages.getMessage('error.PromotionInFlight', [plural, tokens.join(', '), this.config.bin])); + } + } + private async runValidation( connection: Connection, pipelineId: string, diff --git a/src/commands/devops/work-item/create.ts b/src/commands/devops/work-item/create.ts index f1029cf9..eadef1bf 100644 --- a/src/commands/devops/work-item/create.ts +++ b/src/commands/devops/work-item/create.ts @@ -71,6 +71,7 @@ export default class DevopsWorkItemCreate extends SfCommand { + try { + const stages = await fetchPipelineStages(connection, pipelineId); + return orderStages(stages).map((s) => ({ id: s.Id, name: s.Name })); + } catch { + return undefined; + } +} + +/** + * Orders stages from the first stage following the NextStageId chain. Falls back to + * the original query order if the chain can't be resolved into a single sequence. + */ +function orderStages(stages: PipelineStageRecord[]): PipelineStageRecord[] { + const firstId = computeFirstStageId(stages); + if (!firstId) return stages; + const ordered: PipelineStageRecord[] = []; + const seen = new Set(); + let currentId: string | undefined = firstId; + while (currentId && !seen.has(currentId)) { + const stage = stages.find((s) => s.Id === currentId); + if (!stage) break; + ordered.push(stage); + seen.add(currentId); + currentId = stage.NextStageId ?? undefined; + } + return ordered.length === stages.length ? ordered : stages; +} diff --git a/src/utils/createWorkItem.ts b/src/utils/createWorkItem.ts index 28ca77a3..bb817bd4 100644 --- a/src/utils/createWorkItem.ts +++ b/src/utils/createWorkItem.ts @@ -51,28 +51,33 @@ export async function createWorkItem(params: CreateWorkItemParams): Promise) ?? {}; return { success: true, - workItemId: (data.id ?? data.Id) as string | undefined, - workItemName: (data.name ?? data.Name) as string | undefined, + workItemId: (data.workItemId ?? data.id ?? data.Id) as string | undefined, + workItemName: (data.workItemName ?? data.name ?? data.Name) as string | undefined, subject: (data.subject ?? data.Subject ?? subject) as string, }; } catch (error: unknown) { - const err = error as Record & { response?: { data?: unknown }; body?: unknown; message?: unknown }; - const data: unknown = (err.response?.data ?? err.body ?? err) as unknown; - const message: unknown = - (typeof data === 'object' && - data !== null && - ((data as Record).message ?? - (data as Record).error ?? - (data as Record).errorDescription)) || - err.message || - 'Unknown error'; - const details = - typeof data === 'object' && data !== null && Array.isArray((data as Record).body) - ? ((data as Record).body as string[]).join('; ') - : undefined; - return { - success: false, - error: details ? `${String(message)}: ${details}` : String(message), - }; + return { success: false, error: parseErrorMessage(error) }; } } + +/** + * Extracts a human-readable error message from a failed work item request, + * unwrapping the various shapes the Connect API and jsforce may return. + */ +function parseErrorMessage(error: unknown): string { + const err = error as Record & { response?: { data?: unknown }; body?: unknown; message?: unknown }; + const data: unknown = (err.response?.data ?? err.body ?? err) as unknown; + const message: unknown = + (typeof data === 'object' && + data !== null && + ((data as Record).message ?? + (data as Record).error ?? + (data as Record).errorDescription)) || + err.message || + 'Unknown error'; + const details = + typeof data === 'object' && data !== null && Array.isArray((data as Record).body) + ? ((data as Record).body as string[]).join('; ') + : undefined; + return details ? `${String(message)}: ${details}` : String(message); +} diff --git a/src/utils/promoteStage.ts b/src/utils/promoteStage.ts index 3e3b0e95..732363a7 100644 --- a/src/utils/promoteStage.ts +++ b/src/utils/promoteStage.ts @@ -15,6 +15,7 @@ */ import { Connection } from '@salesforce/core'; +import { validateSalesforceId } from './soqlUtils.js'; export type PromoteStageParams = { connection: Connection; @@ -81,3 +82,47 @@ export async function promoteStage(params: PromoteStageParams): Promise { + validateSalesforceId(targetStageId, 'target stage'); + const statusList = IN_FLIGHT_PROMOTION_STATUSES.join("', '"); + const result = await connection.query( + `SELECT Id, Status, RequestInfoId, RequestInfo.RequestToken FROM DevopsPipelnStgProm WHERE PipelineStageId = '${targetStageId}' AND Status IN ('${statusList}')` + ); + return (result.records ?? []).map((r) => ({ + id: r.Id, + requestToken: r.RequestInfo?.RequestToken ?? r.RequestInfoId ?? '', + status: r.Status, + })); +} diff --git a/test/commands/devops/pipeline/create.test.ts b/test/commands/devops/pipeline/create.test.ts index db6158de..22580939 100644 --- a/test/commands/devops/pipeline/create.test.ts +++ b/test/commands/devops/pipeline/create.test.ts @@ -72,6 +72,10 @@ describe('devops pipeline create', () => { repoType: 'github', created: false, }, + stages: [ + { id: '0Sx000000000001', name: 'Integration' }, + { id: '0Sx000000000002', name: 'UAT' }, + ], }); await CreateCommand.run([ @@ -86,6 +90,9 @@ describe('devops pipeline create', () => { expect(ctx.stdout).to.contain('Successfully created pipeline: Release Pipeline'); expect(ctx.stdout).to.contain('0XB000000000001'); expect(ctx.stdout).to.contain('https://github.com/myorg/myrepo'); + expect(ctx.stdout).to.contain('Stages:'); + expect(ctx.stdout).to.contain('Integration (0Sx000000000001)'); + expect(ctx.stdout).to.contain('UAT (0Sx000000000002)'); expect(ctx.stdout).to.contain('Next steps'); expect(ctx.stdout).to.contain('sf devops pipeline stage add'); expect(ctx.stdout).to.contain('sf devops pipeline project add'); diff --git a/test/commands/devops/promote.test.ts b/test/commands/devops/promote.test.ts index 6c7e1517..02c6d3b4 100644 --- a/test/commands/devops/promote.test.ts +++ b/test/commands/devops/promote.test.ts @@ -42,6 +42,7 @@ describe('devops promote', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any let PromoteCommand: any; const promoteStageStub = sinon.stub(); + const findInFlightPromotionsStub = sinon.stub(); const resolveProjectIdFromWorkItemStub = sinon.stub(); const getPipelineIdForProjectStub = sinon.stub(); const validatePromotionStub = sinon.stub(); @@ -50,6 +51,7 @@ describe('devops promote', () => { const mod = await esmock('../../../src/commands/devops/promote.js', { '../../../src/utils/promoteStage.js': { promoteStage: promoteStageStub, + findInFlightPromotions: findInFlightPromotionsStub, }, '../../../src/utils/prepareWorkItem.js': { resolveProjectIdFromWorkItem: resolveProjectIdFromWorkItemStub, @@ -67,6 +69,9 @@ describe('devops promote', () => { beforeEach(() => { sandbox = sinon.createSandbox(); promoteStageStub.reset(); + findInFlightPromotionsStub.reset(); + // Default: no promotion in flight, so the guard is a no-op + findInFlightPromotionsStub.resolves([]); resolveProjectIdFromWorkItemStub.reset(); getPipelineIdForProjectStub.reset(); validatePromotionStub.reset(); @@ -502,6 +507,67 @@ describe('devops promote', () => { // ── Validation gate ─────────────────────────────────────────────────────── + describe('in-flight promotion guard', () => { + test + .stdout() + .stderr() + .it('blocks promotion when another promotion is already in flight', async (ctx) => { + resolveProjectIdFromWorkItemStub.resolves({ projectId: 'PROJ001', pipelineStageId: '' }); + getPipelineIdForProjectStub.resolves('PIPE001'); + queryMock = sinon.stub().resolves({ records: [{ Id: '1fkxx0000000001', Status: 'READY_TO_PROMOTE' }] }); + findInFlightPromotionsStub.resolves([ + { id: '0Rq000000000001', requestToken: 'TOKEN-123', status: 'IN_PROGRESS' }, + ]); + + try { + await PromoteCommand.run(['-o', 'testOrg', '-i', '1fkxx0000000001', '-t', '1QVxx0000000003']); + expect.fail('should have thrown'); + } catch (e) { + // expected + } + + expect(ctx.stderr).to.contain('already in progress'); + expect(ctx.stderr).to.contain('TOKEN-123'); + // Neither validation nor the promote submission should run when blocked + expect(validatePromotionStub.called).to.be.false; + expect(promoteStageStub.called).to.be.false; + }); + + test + .stdout() + .stderr() + .it('promotes anyway when --force is passed despite an in-flight promotion', async () => { + resolveProjectIdFromWorkItemStub.resolves({ projectId: 'PROJ001', pipelineStageId: '' }); + getPipelineIdForProjectStub.resolves('PIPE001'); + queryMock = sinon.stub().resolves({ records: [{ Id: '1fkxx0000000001', Status: 'READY_TO_PROMOTE' }] }); + findInFlightPromotionsStub.resolves([ + { id: '0Rq000000000001', requestToken: 'TOKEN-123', status: 'IN_PROGRESS' }, + ]); + promoteStageStub.resolves(mockPromoteResult); + + await PromoteCommand.run(['-o', 'testOrg', '-i', '1fkxx0000000001', '-t', '1QVxx0000000003', '--force']); + + // --force bypasses the guard entirely + expect(findInFlightPromotionsStub.called).to.be.false; + expect(promoteStageStub.calledOnce).to.be.true; + }); + + test + .stdout() + .stderr() + .it('proceeds when the in-flight check itself fails (best-effort guard)', async () => { + resolveProjectIdFromWorkItemStub.resolves({ projectId: 'PROJ001', pipelineStageId: '' }); + getPipelineIdForProjectStub.resolves('PIPE001'); + queryMock = sinon.stub().resolves({ records: [{ Id: '1fkxx0000000001', Status: 'READY_TO_PROMOTE' }] }); + findInFlightPromotionsStub.rejects(new Error('query failed')); + promoteStageStub.resolves(mockPromoteResult); + + await PromoteCommand.run(['-o', 'testOrg', '-i', '1fkxx0000000001', '-t', '1QVxx0000000003']); + + expect(promoteStageStub.calledOnce).to.be.true; + }); + }); + describe('validation gate', () => { test .stdout() diff --git a/test/commands/devops/work-item/create.test.ts b/test/commands/devops/work-item/create.test.ts index 460f50fd..0acdfe68 100644 --- a/test/commands/devops/work-item/create.test.ts +++ b/test/commands/devops/work-item/create.test.ts @@ -62,6 +62,9 @@ describe('devops work-item create', () => { await CreateCommand.run(['--target-org', 'testOrg', '--project-id', '1Qg000000000001', '--subject', 'Fix bug']); expect(ctx.stdout).to.contain('Successfully created work item'); + expect(ctx.stdout).to.contain('ID: WI001'); + expect(ctx.stdout).to.contain('Name: WI-001'); + expect(ctx.stdout).to.contain('Subject: Fix bug'); }); }); diff --git a/test/utils/createPipeline.test.ts b/test/utils/createPipeline.test.ts index 1ad646cb..74b8c09b 100644 --- a/test/utils/createPipeline.test.ts +++ b/test/utils/createPipeline.test.ts @@ -319,6 +319,58 @@ describe('createPipeline utilities', () => { expect(body).to.not.have.property('projectIds'); }); + it('fetches and returns stage ids ordered along the promotion path', async () => { + (connectionStub.request as sinon.SinonStub).resolves({ + id: '1PJ000000000001', + message: '', + status: 'SUCCESS', + }); + (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); + // Returned out of order to verify NextStageId-based ordering + (connectionStub.query as sinon.SinonStub).resolves({ + records: [ + { Id: '0Sx000000000003', Name: 'Production', NextStageId: null }, + { Id: '0Sx000000000001', Name: 'Integration', NextStageId: '0Sx000000000002' }, + { Id: '0Sx000000000002', Name: 'UAT', NextStageId: '0Sx000000000003' }, + ], + }); + + const result = await createPipeline({ + connection: connectionStub as unknown as Connection, + name: 'Release Pipeline', + repo: 'https://github.com/myorg/myrepo', + repoType: 'github', + }); + + expect(result.pipelineId).to.equal('1PJ000000000001'); + expect(result.stages).to.deep.equal([ + { id: '0Sx000000000001', name: 'Integration' }, + { id: '0Sx000000000002', name: 'UAT' }, + { id: '0Sx000000000003', name: 'Production' }, + ]); + }); + + it('succeeds without stages when the stage lookup fails', async () => { + (connectionStub.request as sinon.SinonStub).resolves({ + id: '1PJ000000000009', + message: '', + status: 'SUCCESS', + }); + (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); + (connectionStub.query as sinon.SinonStub).rejects(new Error('QUERY failed')); + + const result = await createPipeline({ + connection: connectionStub as unknown as Connection, + name: 'Release Pipeline', + repo: 'https://github.com/myorg/myrepo', + repoType: 'github', + }); + + expect(result.success).to.be.true; + expect(result.pipelineId).to.equal('1PJ000000000009'); + expect(result.stages).to.be.undefined; + }); + it('propagates API errors', async () => { (connectionStub.request as sinon.SinonStub).rejects(new Error('Bad Request')); (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); diff --git a/test/utils/createWorkItem.test.ts b/test/utils/createWorkItem.test.ts index 2afec9ba..a2a31f52 100644 --- a/test/utils/createWorkItem.test.ts +++ b/test/utils/createWorkItem.test.ts @@ -46,6 +46,27 @@ describe('createWorkItem', () => { expect(result.subject).to.equal('Fix bug'); }); + it('returns success with workItemId and workItemName from response', async () => { + (connectionStub.request as sinon.SinonStub).resolves({ + success: 'true', + workItemId: '1fkxx000000003F', + workItemName: 'WI-000003', + }); + + const result = await createWorkItem({ + connection: connectionStub as unknown as Connection, + projectId: 'PROJ001', + subject: 'new work item', + description: '', + }); + + expect(result.success).to.be.true; + expect(result.workItemId).to.equal('1fkxx000000003F'); + expect(result.workItemName).to.equal('WI-000003'); + // subject is not in the response, so it falls back to the requested subject + expect(result.subject).to.equal('new work item'); + }); + it('returns success with uppercase Id and Name from response', async () => { (connectionStub.request as sinon.SinonStub).resolves({ Id: 'WI002', Name: 'WI-002', Subject: 'My subject' }); diff --git a/test/utils/promoteStage.test.ts b/test/utils/promoteStage.test.ts index 265f6166..6028c6bb 100644 --- a/test/utils/promoteStage.test.ts +++ b/test/utils/promoteStage.test.ts @@ -17,7 +17,7 @@ import { expect } from '@oclif/test'; import sinon from 'sinon'; import { Connection } from '@salesforce/core'; -import { promoteStage } from '../../src/utils/promoteStage.js'; +import { promoteStage, findInFlightPromotions } from '../../src/utils/promoteStage.js'; describe('promoteStage utilities', () => { let connectionStub: sinon.SinonStubbedInstance; @@ -149,4 +149,49 @@ describe('promoteStage utilities', () => { expect(result.message).to.equal(''); expect(result.promotedWorkitemIds).to.deep.equal([]); }); + + describe('findInFlightPromotions', () => { + it('queries stage-scoped in-flight promotions and maps the records', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ + records: [ + { + Id: '0Pr000000000001', + Status: 'IN_PROGRESS', + RequestInfoId: '8Mw001', + RequestInfo: { RequestToken: 'TOKEN-1' }, + }, + { Id: '0Pr000000000002', Status: 'NEW', RequestInfoId: '8Mw002', RequestInfo: { RequestToken: 'TOKEN-2' } }, + ], + }); + + const result = await findInFlightPromotions(connectionStub as unknown as Connection, '1QVxx0000000003'); + + const soql = (connectionStub.query as sinon.SinonStub).firstCall.args[0] as string; + expect(soql).to.contain('FROM DevopsPipelnStgProm'); + expect(soql).to.contain("PipelineStageId = '1QVxx0000000003'"); + expect(soql).to.contain("'NEW', 'IN_PROGRESS', 'FINALIZING'"); + expect(result).to.deep.equal([ + { id: '0Pr000000000001', requestToken: 'TOKEN-1', status: 'IN_PROGRESS' }, + { id: '0Pr000000000002', requestToken: 'TOKEN-2', status: 'NEW' }, + ]); + }); + + it('falls back to the request info id when the token is not populated', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ + records: [{ Id: '0Pr000000000003', Status: 'FINALIZING', RequestInfoId: '8Mw003', RequestInfo: null }], + }); + + const result = await findInFlightPromotions(connectionStub as unknown as Connection, '1QVxx0000000003'); + + expect(result).to.deep.equal([{ id: '0Pr000000000003', requestToken: '8Mw003', status: 'FINALIZING' }]); + }); + + it('returns an empty array when no promotions are in flight', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [] }); + + const result = await findInFlightPromotions(connectionStub as unknown as Connection, '1QVxx0000000003'); + + expect(result).to.deep.equal([]); + }); + }); }); From 4e3d7dd604bf87bdd0633f440de44c75b2ce18fa Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 00:30:03 +0530 Subject: [PATCH 2/9] fix: block orphaning stage environment on re-add, add --force to replace sf devops stage environment add unconditionally created a new DevopsEnvironment and re-pointed DevopsPipelineStage.DevOpsEnvironmentId at it, orphaning the previously attached environment record. Add getStageEnvironment() to detect an existing environment and guard the add: block by default with a clear error, or with --force remove the existing environment (via deleteStageEnvironment) before adding the new one, aborting if that removal fails. Co-Authored-By: Claude Opus 4.8 (1M context) --- messages/devops.stage.environment.add.md | 12 ++ src/commands/devops/stage/environment/add.ts | 51 +++++++- src/utils/addStageEnvironment.ts | 29 +++++ .../devops/stage/environment/add.test.ts | 122 ++++++++++++++++++ test/utils/addStageEnvironment.test.ts | 38 ++++++ 5 files changed, 250 insertions(+), 2 deletions(-) diff --git a/messages/devops.stage.environment.add.md b/messages/devops.stage.environment.add.md index 9130687e..8c3263b5 100644 --- a/messages/devops.stage.environment.add.md +++ b/messages/devops.stage.environment.add.md @@ -26,6 +26,10 @@ Type of the Salesforce org. Valid values: Production, Sandbox. Don't auto-open the browser for OAuth authentication. The redirect URL is printed for manual use. +# flags.force.summary + +Replace the stage's existing environment. By default, the command blocks if the stage already has an environment to avoid orphaning it. With this flag, the existing environment is removed before the new one is added. + # examples - Add a production environment to a stage using its ID: @@ -60,6 +64,14 @@ Pipeline stage "%s" doesn't exist in pipeline "%s". Check the stage ID and try a Failed to create environment for stage: %s +# error.EnvironmentAlreadyExists + +Stage "%s" already has an environment ("%s", %s). Adding another would leave the existing one orphaned. Remove it first with "%s devops stage environment delete", or pass --force to replace it. + +# error.ReplaceEnvironmentFailed + +Failed to remove the existing environment before adding the new one: %s + # error.AuthTimeout Authentication timed out. The environment was created but not yet authenticated. Re-run the command or authenticate manually via the org's DevOps Center setup. diff --git a/src/commands/devops/stage/environment/add.ts b/src/commands/devops/stage/environment/add.ts index 72d437f9..28895a8b 100644 --- a/src/commands/devops/stage/environment/add.ts +++ b/src/commands/devops/stage/environment/add.ts @@ -15,9 +15,15 @@ */ import { execFile } from 'node:child_process'; -import { Messages, Org } from '@salesforce/core'; +import { Connection, Messages, Org } from '@salesforce/core'; import { SfCommand, Flags } from '@salesforce/sf-plugins-core'; -import { addStageEnvironment, AddStageEnvironmentResult, OrgType } from '../../../../utils/addStageEnvironment.js'; +import { + addStageEnvironment, + AddStageEnvironmentResult, + OrgType, + getStageEnvironment, +} from '../../../../utils/addStageEnvironment.js'; +import { deleteStageEnvironment } from '../../../../utils/deleteStageEnvironment.js'; import { fetchPipelineStages } from '../../../../utils/pipelineUtils.js'; import { PipelineStageRecord } from '../../../../utils/types.js'; import { validateSalesforceId } from '../../../../utils/soqlUtils.js'; @@ -98,6 +104,10 @@ export default class DevopsStageEnvironmentAdd extends SfCommand { @@ -134,6 +144,8 @@ export default class DevopsStageEnvironmentAdd extends SfCommand { + const existingEnvironment = await getStageEnvironment(connection, stageId); + if (!existingEnvironment) { + return; + } + if (!force) { + this.error( + messages.getMessage('error.EnvironmentAlreadyExists', [ + stageId, + existingEnvironment.environmentName ?? existingEnvironment.environmentId, + existingEnvironment.environmentId, + this.config.bin, + ]) + ); + } + const deleteResult = await deleteStageEnvironment(connection, existingEnvironment.environmentId); + if (!deleteResult.success) { + this.error(messages.getMessage('error.ReplaceEnvironmentFailed', [deleteResult.error ?? ''])); + } + } + + private logResult( + result: AddStageEnvironmentResult, + stageId: string, + orgType: OrgType, + pipelineId: string + ): AddStageEnvironmentResult { if (result.success) { this.log(messages.getMessage('info.Success')); this.log(` Stage ID: ${stageId}`); diff --git a/src/utils/addStageEnvironment.ts b/src/utils/addStageEnvironment.ts index 807acdf8..3bb546aa 100644 --- a/src/utils/addStageEnvironment.ts +++ b/src/utils/addStageEnvironment.ts @@ -15,9 +15,38 @@ */ import { Connection } from '@salesforce/core'; +import { validateSalesforceId } from './soqlUtils.js'; export type OrgType = 'Production' | 'Sandbox'; +export type ExistingStageEnvironment = { + environmentId: string; + environmentName?: string; +}; + +/** + * Returns the environment currently associated with a pipeline stage, if any. + * + * A stage references its environment via DevopsPipelineStage.DevOpsEnvironmentId. Adding a new + * environment re-points that lookup, so callers must check for an existing environment first to + * avoid orphaning the old DevopsEnvironment record. + */ +export async function getStageEnvironment( + connection: Connection, + stageId: string +): Promise { + validateSalesforceId(stageId, 'stage'); + const result = await connection.query<{ + DevOpsEnvironmentId: string | null; + DevOpsEnvironment: { Name: string } | null; + }>(`SELECT DevOpsEnvironmentId, DevOpsEnvironment.Name FROM DevopsPipelineStage WHERE Id = '${stageId}' LIMIT 1`); + const record = (result.records ?? [])[0]; + if (!record?.DevOpsEnvironmentId) { + return undefined; + } + return { environmentId: record.DevOpsEnvironmentId, environmentName: record.DevOpsEnvironment?.Name }; +} + const ORG_TYPE_API_MAP: Record = { Production: 'PRODUCTION', Sandbox: 'SANDBOX', diff --git a/test/commands/devops/stage/environment/add.test.ts b/test/commands/devops/stage/environment/add.test.ts index 802a20a0..29f1f03a 100644 --- a/test/commands/devops/stage/environment/add.test.ts +++ b/test/commands/devops/stage/environment/add.test.ts @@ -27,6 +27,8 @@ describe('devops stage environment add', () => { const mockConnection = { getApiVersion: () => '65.0', query: queryStub }; const mockOrg = { id: '1', getOrgId: () => '1', getConnection: () => mockConnection, getUsername: () => 'testOrg' }; const addStageEnvironmentStub = sinon.stub(); + const getStageEnvironmentStub = sinon.stub(); + const deleteStageEnvironmentStub = sinon.stub(); const fetchPipelineStagesStub = sinon.stub(); const execFileStub = sinon.stub(); @@ -34,6 +36,10 @@ describe('devops stage environment add', () => { const mod = await esmock('../../../../../src/commands/devops/stage/environment/add.js', { '../../../../../src/utils/addStageEnvironment.js': { addStageEnvironment: addStageEnvironmentStub, + getStageEnvironment: getStageEnvironmentStub, + }, + '../../../../../src/utils/deleteStageEnvironment.js': { + deleteStageEnvironment: deleteStageEnvironmentStub, }, '../../../../../src/utils/pipelineUtils.js': { fetchPipelineStages: fetchPipelineStagesStub, @@ -48,6 +54,10 @@ describe('devops stage environment add', () => { beforeEach(() => { sandbox = sinon.createSandbox(); addStageEnvironmentStub.reset(); + getStageEnvironmentStub.reset(); + getStageEnvironmentStub.resolves(undefined); + deleteStageEnvironmentStub.reset(); + deleteStageEnvironmentStub.resolves({ success: true }); fetchPipelineStagesStub.reset(); execFileStub.reset(); queryStub.reset(); @@ -458,4 +468,116 @@ describe('devops stage environment add', () => { expect(opened).to.not.match(/["\s]/); }); }); + + describe('existing environment guard', () => { + test + .stdout() + .stderr() + .it('blocks when the stage already has an environment and --force is not set', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); + + try { + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + ]); + expect.fail('should have thrown'); + } catch (e) { + // expected + } + + expect(ctx.stderr).to.contain('already has an environment'); + expect(ctx.stderr).to.contain('Existing_Org'); + // The new environment must not be created while one already exists. + expect(addStageEnvironmentStub.called).to.equal(false); + expect(deleteStageEnvironmentStub.called).to.equal(false); + }); + + test + .stdout() + .stderr() + .it('with --force, removes the existing environment then adds the new one', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); + deleteStageEnvironmentStub.resolves({ success: true, environmentId: '0Hi000000000001' }); + addStageEnvironmentStub.resolves({ + success: true, + stageId: '0Xp000000000001', + environmentId: '0Hi000000000002', + environmentName: 'Production_Org', + orgType: 'Production', + pipelineId: '0Xo000000000001', + organizationId: '00D000000000002', + }); + + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + '--force', + ]); + + // The existing environment is deleted before the new one is created. + expect(deleteStageEnvironmentStub.calledOnce).to.equal(true); + expect(deleteStageEnvironmentStub.firstCall.args[1]).to.equal('0Hi000000000001'); + expect(addStageEnvironmentStub.calledOnce).to.equal(true); + expect(deleteStageEnvironmentStub.calledBefore(addStageEnvironmentStub)).to.equal(true); + expect(ctx.stdout).to.contain('Successfully added environment to the stage.'); + expect(ctx.stdout).to.contain('0Hi000000000002'); + }); + + test + .stdout() + .stderr() + .it('with --force, aborts when removing the existing environment fails', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); + deleteStageEnvironmentStub.resolves({ success: false, error: 'delete failed' }); + + try { + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + '--force', + ]); + expect.fail('should have thrown'); + } catch (e) { + // expected + } + + expect(ctx.stderr).to.contain('Failed to remove the existing environment'); + // The new environment must not be created if the old one couldn't be removed. + expect(addStageEnvironmentStub.called).to.equal(false); + }); + }); }); diff --git a/test/utils/addStageEnvironment.test.ts b/test/utils/addStageEnvironment.test.ts index bb24ca64..b9bd5240 100644 --- a/test/utils/addStageEnvironment.test.ts +++ b/test/utils/addStageEnvironment.test.ts @@ -21,6 +21,7 @@ import { addStageEnvironment, createEnvironment, getEnvironment, + getStageEnvironment, validateEnvironment, pollForAuthentication, } from '../../src/utils/addStageEnvironment.js'; @@ -28,12 +29,15 @@ import { describe('addStageEnvironment utilities', () => { let connectionStub: sinon.SinonStubbedInstance; let requestStub: sinon.SinonStub; + let queryStub: sinon.SinonStub; beforeEach(() => { connectionStub = sinon.createStubInstance(Connection); (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); requestStub = sinon.stub(); connectionStub.request = requestStub as unknown as typeof connectionStub.request; + queryStub = sinon.stub(); + connectionStub.query = queryStub as unknown as typeof connectionStub.query; }); afterEach(() => { @@ -113,6 +117,40 @@ describe('addStageEnvironment utilities', () => { }); }); + describe('getStageEnvironment', () => { + it('returns the environment when the stage references one', async () => { + queryStub.resolves({ + records: [{ DevOpsEnvironmentId: '0Hi000000000001', DevOpsEnvironment: { Name: 'Production_Org' } }], + }); + + const result = await getStageEnvironment(connectionStub as unknown as Connection, '0Xp000000000001'); + + expect(result).to.deep.equal({ environmentId: '0Hi000000000001', environmentName: 'Production_Org' }); + expect(queryStub.firstCall.args[0]).to.contain("WHERE Id = '0Xp000000000001'"); + }); + + it('returns undefined when the stage has no environment', async () => { + queryStub.resolves({ records: [{ DevOpsEnvironmentId: null, DevOpsEnvironment: null }] }); + + const result = await getStageEnvironment(connectionStub as unknown as Connection, '0Xp000000000001'); + expect(result).to.be.undefined; + }); + + it('returns undefined when the stage is not found', async () => { + queryStub.resolves({ records: [] }); + + const result = await getStageEnvironment(connectionStub as unknown as Connection, '0Xp000000000001'); + expect(result).to.be.undefined; + }); + + it('returns the environment id when the related name is missing', async () => { + queryStub.resolves({ records: [{ DevOpsEnvironmentId: '0Hi000000000001', DevOpsEnvironment: null }] }); + + const result = await getStageEnvironment(connectionStub as unknown as Connection, '0Xp000000000001'); + expect(result).to.deep.equal({ environmentId: '0Hi000000000001', environmentName: undefined }); + }); + }); + describe('getEnvironment', () => { it('calls GET /connect/devops/environment/{id}', async () => { requestStub.resolves({ From da298e1639f60b6ea76f6117b08b2424b418e7b6 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 00:57:14 +0530 Subject: [PATCH 3/9] fix: enforce work-item status transitions and route description via sObject Guard status changes so a work item can move to IN_PROGRESS only from NEW and to READY_TO_PROMOTE only from IN_REVIEW. Route subject/description updates through the WorkItem sObject (the connect work-item endpoint only accepts status), fixing the "Unrecognized field description" error. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/utils/updateWorkItem.ts | 92 +++++++++++--- test/utils/updateWorkItem.test.ts | 191 ++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 18 deletions(-) create mode 100644 test/utils/updateWorkItem.test.ts diff --git a/src/utils/updateWorkItem.ts b/src/utils/updateWorkItem.ts index d54e27a5..01a4d984 100644 --- a/src/utils/updateWorkItem.ts +++ b/src/utils/updateWorkItem.ts @@ -57,6 +57,46 @@ export function toApiStatus(status: string): string { return apiStatus; } +/** + * Allowed current statuses for each target status a user can set. A work item may only move + * to IN_PROGRESS while it is NEW, and to READY_TO_PROMOTE while it is IN_REVIEW. Target statuses + * absent from this map have no precondition. + */ +export const STATUS_TRANSITION_PRECONDITIONS: Record = { + IN_PROGRESS: ['NEW'], + READY_TO_PROMOTE: ['IN_REVIEW'], +}; + +/** + * Verifies that the work item's current status permits the requested transition. Throws with a + * descriptive error when it does not. Only runs a query when the target status has a precondition, + * so subject/description-only updates are unaffected. + */ +export async function assertStatusTransitionAllowed( + connection: Connection, + workItemId: string, + targetApiStatus: string +): Promise { + const allowedFrom = STATUS_TRANSITION_PRECONDITIONS[targetApiStatus]; + if (!allowedFrom) { + return; + } + validateSalesforceId(workItemId, 'work item'); + const result = await connection.query<{ Status: string }>( + `SELECT Status FROM WorkItem WHERE Id = '${workItemId}' LIMIT 1` + ); + const current = result.records?.[0]?.Status; + if (!current) { + throw new Error(`Work item with ID '${workItemId}' not found.`); + } + if (!allowedFrom.includes(current)) { + const allowed = allowedFrom.join(', '); + throw new Error( + `Cannot change status to ${targetApiStatus}: the work item is currently ${current}, but this transition is only allowed from ${allowed}.` + ); + } +} + export async function resolveWorkItemByName(connection: Connection, workItemName: string): Promise { const result = await connection.query<{ Id: string; DevopsProjectId: string }>( `SELECT Id, DevopsProjectId FROM WorkItem WHERE Name = '${escapeSOQL(workItemName)}' LIMIT 1` @@ -81,32 +121,48 @@ export async function resolveProjectIdForWorkItem(connection: Connection, workIt } /** - * Updates fields on a DevOps Center work item via the Connect API. - * PATCH /services/data/v{version}/connect/devops/projects/{projectId}/workitem/{workItemId} + * Updates a DevOps Center work item. + * + * Subject and description are plain WorkItem sObject fields — the DevOps Center connect work item + * endpoint does not accept them — so they are written via the sObject API. Status changes go + * through the connect endpoint (PATCH /connect/devops/projects/{projectId}/workitem/{workItemId}), + * which is the only field that endpoint accepts, so platform-side status handling runs. */ export async function updateWorkItem(params: UpdateWorkItemParams): Promise { const { connection, workItemId, projectId, status, subject, description } = params; - const path = `/services/data/v${connection.getApiVersion()}/connect/devops/projects/${projectId}/workitem/${workItemId}`; - const payload: Record = {}; - if (status !== undefined) payload.status = toApiStatus(status); - if (subject !== undefined) payload.subject = subject; - if (description !== undefined) payload.description = description; + if (subject !== undefined || description !== undefined) { + const fields: { Id: string; Subject?: string; Description?: string } = { Id: workItemId }; + if (subject !== undefined) fields.Subject = subject; + if (description !== undefined) fields.Description = description; + const saveResult = await connection.sobject('WorkItem').update(fields); + if (!saveResult.success) { + const errorMsg = saveResult.errors + .map((e) => (typeof e === 'string' ? e : e.message)) + .filter(Boolean) + .join('; '); + return { success: false, workItemId, error: errorMsg || 'Failed to update work item fields.' }; + } + } - const response = await connection.request({ - method: 'PATCH', - url: path, - body: JSON.stringify(payload), - headers: { 'Content-Type': 'application/json' }, - }); + let apiStatus: string | undefined; + if (status !== undefined) { + apiStatus = toApiStatus(status); + await assertStatusTransitionAllowed(connection, workItemId, apiStatus); + const path = `/services/data/v${connection.getApiVersion()}/connect/devops/projects/${projectId}/workitem/${workItemId}`; + await connection.request({ + method: 'PATCH', + url: path, + body: JSON.stringify({ status: apiStatus }), + headers: { 'Content-Type': 'application/json' }, + }); + } - const data = (response as Record) ?? {}; return { success: true, workItemId, - status: status !== undefined ? ((data.status ?? data.Status ?? status) as string) : undefined, - subject: subject !== undefined ? ((data.subject ?? data.Subject ?? subject) as string) : undefined, - description: - description !== undefined ? ((data.description ?? data.Description ?? description) as string) : undefined, + status: apiStatus, + subject, + description, }; } diff --git a/test/utils/updateWorkItem.test.ts b/test/utils/updateWorkItem.test.ts new file mode 100644 index 00000000..1765d9a6 --- /dev/null +++ b/test/utils/updateWorkItem.test.ts @@ -0,0 +1,191 @@ +/* + * Copyright 2026, Salesforce, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect } from '@oclif/test'; +import sinon from 'sinon'; +import { Connection } from '@salesforce/core'; +import { assertStatusTransitionAllowed, updateWorkItem } from '../../src/utils/updateWorkItem.js'; + +describe('updateWorkItem status transition guard', () => { + let connectionStub: sinon.SinonStubbedInstance; + const workItemId = '0Wx000000000001'; + const projectId = '1Qg000000000001'; + + beforeEach(() => { + connectionStub = sinon.createStubInstance(Connection); + (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); + }); + + afterEach(() => { + sinon.restore(); + }); + + describe('assertStatusTransitionAllowed', () => { + it('allows IN_PROGRESS when the work item is NEW', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'NEW' }] }); + + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'IN_PROGRESS'); + + const soql = (connectionStub.query as sinon.SinonStub).firstCall.args[0] as string; + expect(soql).to.contain('SELECT Status FROM WorkItem'); + expect(soql).to.contain(`Id = '${workItemId}'`); + }); + + it('blocks IN_PROGRESS when the work item is not NEW', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'IN_PROGRESS' }] }); + + try { + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'IN_PROGRESS'); + expect.fail('should have thrown'); + } catch (e: unknown) { + expect((e as Error).message).to.contain('currently IN_PROGRESS'); + expect((e as Error).message).to.contain('only allowed from NEW'); + } + }); + + it('allows READY_TO_PROMOTE when the work item is IN_REVIEW', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'IN_REVIEW' }] }); + + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'READY_TO_PROMOTE'); + }); + + it('blocks READY_TO_PROMOTE when the work item is not IN_REVIEW', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'NEW' }] }); + + try { + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'READY_TO_PROMOTE'); + expect.fail('should have thrown'); + } catch (e: unknown) { + expect((e as Error).message).to.contain('currently NEW'); + expect((e as Error).message).to.contain('only allowed from IN_REVIEW'); + } + }); + + it('does not query for a target status with no precondition', async () => { + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'CLOSED'); + + expect((connectionStub.query as sinon.SinonStub).called).to.be.false; + }); + + it('throws when the work item is not found', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [] }); + + try { + await assertStatusTransitionAllowed(connectionStub as unknown as Connection, workItemId, 'IN_PROGRESS'); + expect.fail('should have thrown'); + } catch (e: unknown) { + expect((e as Error).message).to.contain('not found'); + } + }); + }); + + describe('updateWorkItem', () => { + it('blocks the PATCH when the transition is invalid', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'IN_PROGRESS' }] }); + + try { + await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + status: 'In Progress', + }); + expect.fail('should have thrown'); + } catch (e: unknown) { + expect((e as Error).message).to.contain('Cannot change status'); + } + + expect((connectionStub.request as sinon.SinonStub).called).to.be.false; + }); + + it('sends the PATCH when the transition is valid', async () => { + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'NEW' }] }); + (connectionStub.request as sinon.SinonStub).resolves({ status: 'IN_PROGRESS' }); + + const result = await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + status: 'In Progress', + }); + + expect(result.success).to.be.true; + const callArgs = (connectionStub.request as sinon.SinonStub).firstCall.args[0]; + expect(callArgs.method).to.equal('PATCH'); + const body = JSON.parse(callArgs.body as string) as Record; + expect(body.status).to.equal('IN_PROGRESS'); + }); + + it('updates subject/description via the sObject API, not the connect endpoint', async () => { + const updateStub = sinon.stub().resolves({ id: workItemId, success: true, errors: [] }); + (connectionStub.sobject as sinon.SinonStub).returns({ update: updateStub }); + + const result = await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + subject: 'New subject', + description: 'New description', + }); + + expect((connectionStub.sobject as sinon.SinonStub).calledWith('WorkItem')).to.be.true; + expect(updateStub.firstCall.args[0]).to.deep.equal({ + Id: workItemId, + Subject: 'New subject', + Description: 'New description', + }); + // No status change → no status query and no connect PATCH. + expect((connectionStub.query as sinon.SinonStub).called).to.be.false; + expect((connectionStub.request as sinon.SinonStub).called).to.be.false; + expect(result).to.deep.include({ success: true, subject: 'New subject', description: 'New description' }); + }); + + it('returns a failure result when the sObject update fails', async () => { + const updateStub = sinon.stub().resolves({ id: workItemId, success: false, errors: [{ message: 'boom' }] }); + (connectionStub.sobject as sinon.SinonStub).returns({ update: updateStub }); + + const result = await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + description: 'New description', + }); + + expect(result.success).to.be.false; + expect(result.error).to.contain('boom'); + }); + + it('updates both fields and status in a single call', async () => { + const updateStub = sinon.stub().resolves({ id: workItemId, success: true, errors: [] }); + (connectionStub.sobject as sinon.SinonStub).returns({ update: updateStub }); + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'NEW' }] }); + (connectionStub.request as sinon.SinonStub).resolves({ success: 'true' }); + + const result = await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + subject: 'New subject', + status: 'In Progress', + }); + + expect(updateStub.calledOnce).to.be.true; + expect((connectionStub.request as sinon.SinonStub).calledOnce).to.be.true; + expect(result.status).to.equal('IN_PROGRESS'); + expect(result.subject).to.equal('New subject'); + }); + }); +}); From 59df2979cd5f208c097a09db1095bde7e783c58f Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 00:57:25 +0530 Subject: [PATCH 4/9] feat: show work item ID in work-item list output Add an ID column to the work-item list table so the record ID is visible alongside the name. The ID was already in the query and JSON output. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/devops/work-item/list.ts | 3 ++- test/commands/devops/work-item/list.test.ts | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/commands/devops/work-item/list.ts b/src/commands/devops/work-item/list.ts index db2b6934..3db3665b 100644 --- a/src/commands/devops/work-item/list.ts +++ b/src/commands/devops/work-item/list.ts @@ -64,6 +64,7 @@ export default class DevopsWorkItemList extends SfCommand ({ + ID: wi.id, Name: wi.name, Subject: wi.subject ?? '', Status: wi.status, @@ -74,7 +75,7 @@ export default class DevopsWorkItemList extends SfCommand { await ListCommand.run(['--target-org', 'testOrg', '--project-id', '1Qg000000000001']); expect(ctx.stdout).to.contain('WI-001'); + // The work item ID must be shown alongside the name. + expect(ctx.stdout).to.contain('WI001'); }); }); From 293ac78f68d37709b341b4eb9a03efdfd1ca1215 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 00:57:44 +0530 Subject: [PATCH 5/9] fix: leave work-item target branch blank on the final pipeline stage The target branch fell back to the first stage whenever the next-stage lookup was empty, so a work item already on the last stage (e.g. a closed item) wrongly showed the first stage's branch. Only fall back to the first stage when the work item has not entered the pipeline; otherwise use its next stage's branch, leaving it blank when there is no next stage. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/utils/workItems.ts | 11 ++++++--- test/utils/workItems.test.ts | 48 ++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/utils/workItems.ts b/src/utils/workItems.ts index 6fea01be..533125bc 100644 --- a/src/utils/workItems.ts +++ b/src/utils/workItems.ts @@ -175,10 +175,13 @@ function mapRawItemToWorkItem( }; if (ctx) { - let targetStageId = resolveTargetStageId(mapped.PipelineStageId, ctx.stages); - if (!targetStageId) { - targetStageId = ctx.firstStageId; - } + // A work item's changes are promoted into the branch of its *next* pipeline stage. Only fall + // back to the first stage when the work item hasn't entered the pipeline yet (no current stage); + // a work item already on the final stage has no next stage, so it has no target branch (e.g. a + // closed work item that reached the last stage), and the branch is left blank. + const targetStageId = mapped.PipelineStageId + ? resolveTargetStageId(mapped.PipelineStageId, ctx.stages) + : ctx.firstStageId; const targetStage = findStageById(ctx.stages, targetStageId); mapped.TargetBranch = getBranchNameFromStage(targetStage); mapped.TargetStageId = targetStageId; diff --git a/test/utils/workItems.test.ts b/test/utils/workItems.test.ts index b31118a4..78695e96 100644 --- a/test/utils/workItems.test.ts +++ b/test/utils/workItems.test.ts @@ -176,4 +176,52 @@ describe('fetchWorkItems', () => { expect(result[0].TargetStageId).to.equal('S2'); expect(result[0].TargetBranch).to.equal('branch-2'); }); + + it('leaves the target branch blank for a work item on the final stage', async () => { + // Work item currently sits on S2, the last stage (NextStageId is null), so it has no next + // stage to promote into — the target branch must not fall back to the first stage. + (connectionStub.query as sinon.SinonStub).resolves({ + records: [{ ...MOCK_RECORD, DevopsPipelineStageId: 'S2' }], + }); + (connectionStub.request as sinon.SinonStub).rejects(new Error('no vcs')); + + const { fetchWorkItems } = await esmock('../../src/utils/workItems.js', { + '../../src/utils/pipelineUtils.js': { + getPipelineIdForProject: sinon.stub().resolves('PIPE001'), + fetchPipelineStages: sinon.stub().resolves([ + { Id: 'S1', NextStageId: 'S2', SourceCodeRepositoryBranch: { Name: 'branch-1' } }, + { Id: 'S2', NextStageId: null, SourceCodeRepositoryBranch: { Name: 'branch-2' } }, + ]), + }, + }); + + const result = await fetchWorkItems(connectionStub as unknown as Connection, 'PROJ001'); + + expect(result[0].TargetStageId).to.be.undefined; + expect(result[0].TargetBranch).to.be.undefined; + }); + + it('targets the first stage for a work item that has not entered the pipeline', async () => { + // No current stage (DevopsPipelineStageId null) — the work item first promotes into the first + // stage (S1, the stage nothing points to), so the target branch is that stage's branch. + (connectionStub.query as sinon.SinonStub).resolves({ + records: [{ ...MOCK_RECORD, DevopsPipelineStageId: null }], + }); + (connectionStub.request as sinon.SinonStub).rejects(new Error('no vcs')); + + const { fetchWorkItems } = await esmock('../../src/utils/workItems.js', { + '../../src/utils/pipelineUtils.js': { + getPipelineIdForProject: sinon.stub().resolves('PIPE001'), + fetchPipelineStages: sinon.stub().resolves([ + { Id: 'S1', NextStageId: 'S2', SourceCodeRepositoryBranch: { Name: 'branch-1' } }, + { Id: 'S2', NextStageId: null, SourceCodeRepositoryBranch: { Name: 'branch-2' } }, + ]), + }, + }); + + const result = await fetchWorkItems(connectionStub as unknown as Connection, 'PROJ001'); + + expect(result[0].TargetStageId).to.equal('S1'); + expect(result[0].TargetBranch).to.equal('branch-1'); + }); }); From ec45390793a545563af2b7d8dd873911b001c5b2 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 01:00:39 +0530 Subject: [PATCH 6/9] chore: update command snapshot for stage environment add --force flag Co-Authored-By: Claude Opus 4.8 (1M context) --- command-snapshot.json | 1 + 1 file changed, 1 insertion(+) diff --git a/command-snapshot.json b/command-snapshot.json index 85ec70e9..fa8d3269 100644 --- a/command-snapshot.json +++ b/command-snapshot.json @@ -206,6 +206,7 @@ "api-version", "environment-name", "flags-dir", + "force", "json", "no-browser", "org-type", From 04cf67fa6b9c38ab634efd22e27673a5aade0c7d Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Wed, 23 Sep 2026 12:50:35 +0530 Subject: [PATCH 7/9] fix: block orphaning stage branch on re-add, add --force to replace sf devops stage branch add created a new SourceCodeRepositoryBranch and re-pointed DevopsPipelineStage.SourceCodeRepositoryBranchId at it, orphaning the previously associated branch record. Add getStageBranch() to detect an existing branch and guard the add: block by default with a clear error, or with --force associate the new branch and then remove the orphaned record (via deleteOrphanedBranch, which only deletes when no other stage references it). Cleanup is best-effort and warns on failure. Update the command snapshot for the new --force flag. Co-Authored-By: Claude Opus 4.8 (1M context) --- command-snapshot.json | 1 + messages/devops.stage.branch.add.md | 18 ++- src/commands/devops/stage/branch/add.ts | 68 +++++++++- src/utils/addStageBranch.ts | 50 ++++++++ test/commands/devops/stage/branch/add.test.ts | 120 ++++++++++++++++++ test/utils/addStageBranch.test.ts | 56 +++++++- 6 files changed, 309 insertions(+), 4 deletions(-) diff --git a/command-snapshot.json b/command-snapshot.json index fa8d3269..8740e6cf 100644 --- a/command-snapshot.json +++ b/command-snapshot.json @@ -182,6 +182,7 @@ "branch-name", "create-vcs-branch", "flags-dir", + "force", "json", "pipeline-id", "stage-id", diff --git a/messages/devops.stage.branch.add.md b/messages/devops.stage.branch.add.md index 7c8c2140..23d44bb2 100644 --- a/messages/devops.stage.branch.add.md +++ b/messages/devops.stage.branch.add.md @@ -5,7 +5,7 @@ Add a source code repository branch to a pipeline stage. # description By default, the branch must exist in the repository. Use --create-vcs-branch to create a branch if it doesn't exist. -Each pipeline stage supports only one branch. Adding a branch replaces any existing branch linked to the pipeline stage. +Each pipeline stage supports only one branch. If the stage already has a branch, the command blocks so the existing branch isn't orphaned; pass --force to replace it. # flags.pipeline-id.summary @@ -23,6 +23,10 @@ Name of the repository branch to assign to the stage. Create the branch in the remote repository if it doesn't already exist. +# flags.force.summary + +Replace the stage's existing branch. By default, the command blocks if the stage already has a branch to avoid orphaning it. With this flag, the new branch is associated and the previous branch record is removed if no other stage references it. + # examples - Add an existing branch to a stage: @@ -44,3 +48,15 @@ You must set up a branch on stage "%s" before configuring stage "%s". Branches m # error.BranchAttachFailed Failed to associate branch with stage: %s + +# error.BranchAlreadyExists + +Stage "%s" already has a branch ("%s"). Adding another would leave the existing one orphaned. Remove it first with "%s devops stage branch delete", or pass --force to replace it. + +# info.ReplacedBranchRemoved + +Removed the stage's previous branch "%s". + +# warn.ReplacedBranchCleanupFailed + +Associated the new branch, but couldn't remove the previous branch record (%s). Remove it manually if it's no longer needed. diff --git a/src/commands/devops/stage/branch/add.ts b/src/commands/devops/stage/branch/add.ts index 5ada2374..5f3f8151 100644 --- a/src/commands/devops/stage/branch/add.ts +++ b/src/commands/devops/stage/branch/add.ts @@ -14,9 +14,15 @@ * limitations under the License. */ -import { Messages, Org } from '@salesforce/core'; +import { Connection, Messages, Org } from '@salesforce/core'; import { SfCommand, Flags } from '@salesforce/sf-plugins-core'; -import { addStageBranch, AddStageBranchResult } from '../../../../utils/addStageBranch.js'; +import { + addStageBranch, + AddStageBranchResult, + ExistingStageBranch, + getStageBranch, + deleteOrphanedBranch, +} from '../../../../utils/addStageBranch.js'; import { fetchPipelineStages } from '../../../../utils/pipelineUtils.js'; import { PipelineStageRecord } from '../../../../utils/types.js'; @@ -24,6 +30,11 @@ Messages.importMessagesDirectoryFromMetaUrl(import.meta.url); const messages = Messages.loadMessages('@salesforce/plugin-devops-center', 'devops.stage.branch.add'); const commonErrorMessages = Messages.loadMessages('@salesforce/plugin-devops-center', 'commonErrors'); +// The platform stores created branch names with a "$$" marker prefix; strip it for display. +function displayBranchName(existingBranch: ExistingStageBranch): string { + return (existingBranch.branchName ?? existingBranch.branchId).replace(/^\$\$/, ''); +} + export default class DevopsStageBranchAdd extends SfCommand { public static readonly summary = messages.getMessage('summary'); public static readonly description = messages.getMessage('description'); @@ -51,6 +62,10 @@ export default class DevopsStageBranchAdd extends SfCommand { @@ -85,6 +100,8 @@ export default class DevopsStageBranchAdd extends SfCommand { + const existingBranch = await getStageBranch(connection, stageId); + if (!existingBranch) { + return undefined; + } + if (!force) { + this.error( + messages.getMessage('error.BranchAlreadyExists', [stageId, displayBranchName(existingBranch), this.config.bin]) + ); + } + return existingBranch; + } + + /** + * Removes the branch record the stage was re-pointed away from, unless another stage still + * references it. Best-effort: a cleanup failure is surfaced as a warning and does not fail the + * command, since the new branch has already been associated successfully. + */ + private async cleanupReplacedBranch(connection: Connection, existingBranch: ExistingStageBranch): Promise { + try { + const deleted = await deleteOrphanedBranch(connection, existingBranch.branchId); + if (deleted) { + this.log(messages.getMessage('info.ReplacedBranchRemoved', [displayBranchName(existingBranch)])); + } + } catch { + this.warn(messages.getMessage('warn.ReplacedBranchCleanupFailed', [existingBranch.branchId])); + } + } + + private logResult(result: AddStageBranchResult, stageId: string, pipelineId: string): AddStageBranchResult { if (result.success) { const action = result.branchCreated ? 'Created branch and associated it' : 'Successfully associated branch'; this.log(`${action} with the stage.`); diff --git a/src/utils/addStageBranch.ts b/src/utils/addStageBranch.ts index e03f505e..71884dfa 100644 --- a/src/utils/addStageBranch.ts +++ b/src/utils/addStageBranch.ts @@ -15,6 +15,7 @@ */ import { Connection } from '@salesforce/core'; +import { validateSalesforceId } from './soqlUtils.js'; export type AddStageBranchParams = { connection: Connection; @@ -24,6 +25,55 @@ export type AddStageBranchParams = { createVcsBranch: boolean; }; +export type ExistingStageBranch = { + branchId: string; + branchName?: string; +}; + +/** + * Returns the branch currently associated with a pipeline stage, if any. + * + * A stage references its branch via DevopsPipelineStage.SourceCodeRepositoryBranchId. Adding a new + * branch re-points that lookup, so callers must check for an existing branch first to avoid + * orphaning the old SourceCodeRepositoryBranch record. + */ +export async function getStageBranch( + connection: Connection, + stageId: string +): Promise { + validateSalesforceId(stageId, 'stage'); + const result = await connection.query<{ + SourceCodeRepositoryBranchId: string | null; + SourceCodeRepositoryBranch: { Name: string } | null; + }>( + `SELECT SourceCodeRepositoryBranchId, SourceCodeRepositoryBranch.Name FROM DevopsPipelineStage WHERE Id = '${stageId}' LIMIT 1` + ); + const record = (result.records ?? [])[0]; + if (!record?.SourceCodeRepositoryBranchId) { + return undefined; + } + return { branchId: record.SourceCodeRepositoryBranchId, branchName: record.SourceCodeRepositoryBranch?.Name }; +} + +/** + * Deletes a SourceCodeRepositoryBranch record only when no pipeline stage still references it. + * Returns true when the record was deleted, false when it is still referenced (left intact). + * + * Used to clean up the branch a stage was re-pointed away from during a --force replace, so the old + * record isn't orphaned in the org. The reference check keeps a branch shared by another stage safe. + */ +export async function deleteOrphanedBranch(connection: Connection, branchId: string): Promise { + validateSalesforceId(branchId, 'branch'); + const refs = await connection.query<{ Id: string }>( + `SELECT Id FROM DevopsPipelineStage WHERE SourceCodeRepositoryBranchId = '${branchId}' LIMIT 1` + ); + if ((refs.records ?? []).length > 0) { + return false; + } + await connection.sobject('SourceCodeRepositoryBranch').delete(branchId); + return true; +} + export type AddStageBranchResult = { success: boolean; stageId: string; diff --git a/test/commands/devops/stage/branch/add.test.ts b/test/commands/devops/stage/branch/add.test.ts index 58ef915a..fb538a38 100644 --- a/test/commands/devops/stage/branch/add.test.ts +++ b/test/commands/devops/stage/branch/add.test.ts @@ -26,12 +26,16 @@ describe('devops stage branch add', () => { const mockConnection = { getApiVersion: () => '65.0' }; const mockOrg = { id: '1', getOrgId: () => '1', getConnection: () => mockConnection, getUsername: () => 'testOrg' }; const addStageBranchStub = sinon.stub(); + const getStageBranchStub = sinon.stub(); + const deleteOrphanedBranchStub = sinon.stub(); const fetchPipelineStagesStub = sinon.stub(); before(async () => { const mod = await esmock('../../../../../src/commands/devops/stage/branch/add.js', { '../../../../../src/utils/addStageBranch.js': { addStageBranch: addStageBranchStub, + getStageBranch: getStageBranchStub, + deleteOrphanedBranch: deleteOrphanedBranchStub, }, '../../../../../src/utils/pipelineUtils.js': { fetchPipelineStages: fetchPipelineStagesStub, @@ -43,6 +47,10 @@ describe('devops stage branch add', () => { beforeEach(() => { sandbox = sinon.createSandbox(); addStageBranchStub.reset(); + getStageBranchStub.reset(); + getStageBranchStub.resolves(undefined); + deleteOrphanedBranchStub.reset(); + deleteOrphanedBranchStub.resolves(true); fetchPipelineStagesStub.reset(); }); @@ -346,6 +354,118 @@ describe('devops stage branch add', () => { }); }); + describe('existing branch guard', () => { + test + .stdout() + .stderr() + .it('blocks when the stage already has a branch and --force is not set', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageBranchStub.resolves({ branchId: '0Xq000000000001', branchName: '$$UAT' }); + + try { + await AddBranchCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--branch-name', + 'UAT123', + ]); + expect.fail('should have thrown'); + } catch (e) { + // expected + } + + expect(ctx.stderr).to.contain('already has a branch'); + // The "$$" marker prefix is stripped for display. + expect(ctx.stderr).to.contain('UAT'); + expect(ctx.stderr).to.not.contain('$$'); + // No new branch is associated while one already exists. + expect(addStageBranchStub.called).to.equal(false); + expect(deleteOrphanedBranchStub.called).to.equal(false); + }); + + test + .stdout() + .stderr() + .it('with --force, associates the new branch then removes the orphaned one', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageBranchStub.resolves({ branchId: '0Xq000000000001', branchName: '$$UAT' }); + deleteOrphanedBranchStub.resolves(true); + addStageBranchStub.resolves({ + success: true, + stageId: '0Xp000000000001', + branchName: 'UAT123', + branchCreated: true, + repoBranchId: '0Xq000000000002', + pipelineId: '0Xo000000000001', + }); + + await AddBranchCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--branch-name', + 'UAT123', + '--create-vcs-branch', + '--force', + ]); + + // The new branch is associated first, then the previous branch record is removed. + expect(addStageBranchStub.calledOnce).to.equal(true); + expect(deleteOrphanedBranchStub.calledOnce).to.equal(true); + expect(deleteOrphanedBranchStub.firstCall.args[1]).to.equal('0Xq000000000001'); + expect(addStageBranchStub.calledBefore(deleteOrphanedBranchStub)).to.equal(true); + expect(ctx.stdout).to.contain('Created branch and associated it with the stage.'); + expect(ctx.stdout).to.contain("Removed the stage's previous branch"); + }); + + test + .stdout() + .stderr() + .it('with --force, warns but succeeds when the orphaned branch cleanup fails', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageBranchStub.resolves({ branchId: '0Xq000000000001', branchName: '$$UAT' }); + deleteOrphanedBranchStub.rejects(new Error('delete not allowed')); + addStageBranchStub.resolves({ + success: true, + stageId: '0Xp000000000001', + branchName: 'UAT123', + branchCreated: true, + repoBranchId: '0Xq000000000002', + pipelineId: '0Xo000000000001', + }); + + await AddBranchCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--branch-name', + 'UAT123', + '--create-vcs-branch', + '--force', + ]); + + // The command still reports success even though cleanup failed. + expect(ctx.stdout).to.contain('Created branch and associated it with the stage.'); + expect(ctx.stderr).to.contain("couldn't remove the previous branch record"); + }); + }); + describe('rethrows other errors', () => { test .stdout() diff --git a/test/utils/addStageBranch.test.ts b/test/utils/addStageBranch.test.ts index 09ed4c12..9c9eb310 100644 --- a/test/utils/addStageBranch.test.ts +++ b/test/utils/addStageBranch.test.ts @@ -17,14 +17,17 @@ import { expect } from '@oclif/test'; import sinon from 'sinon'; import { Connection } from '@salesforce/core'; -import { addStageBranch } from '../../src/utils/addStageBranch.js'; +import { addStageBranch, getStageBranch, deleteOrphanedBranch } from '../../src/utils/addStageBranch.js'; describe('addStageBranch utilities', () => { let connectionStub: sinon.SinonStubbedInstance; + let queryStub: sinon.SinonStub; beforeEach(() => { connectionStub = sinon.createStubInstance(Connection); (connectionStub.getApiVersion as sinon.SinonStub).returns('65.0'); + queryStub = sinon.stub(); + connectionStub.query = queryStub as unknown as typeof connectionStub.query; }); afterEach(() => { @@ -125,4 +128,55 @@ describe('addStageBranch utilities', () => { expect((e as Error).message).to.contain('Network timeout'); } }); + + describe('getStageBranch', () => { + it('returns the branch when the stage references one', async () => { + queryStub.resolves({ + records: [{ SourceCodeRepositoryBranchId: '0Xq000000000001', SourceCodeRepositoryBranch: { Name: '$$UAT' } }], + }); + + const result = await getStageBranch(connectionStub as unknown as Connection, '0Xp000000000001'); + + expect(result).to.deep.equal({ branchId: '0Xq000000000001', branchName: '$$UAT' }); + expect(queryStub.firstCall.args[0]).to.contain("WHERE Id = '0Xp000000000001'"); + }); + + it('returns undefined when the stage has no branch', async () => { + queryStub.resolves({ records: [{ SourceCodeRepositoryBranchId: null, SourceCodeRepositoryBranch: null }] }); + + const result = await getStageBranch(connectionStub as unknown as Connection, '0Xp000000000001'); + expect(result).to.be.undefined; + }); + + it('returns undefined when the stage is not found', async () => { + queryStub.resolves({ records: [] }); + + const result = await getStageBranch(connectionStub as unknown as Connection, '0Xp000000000001'); + expect(result).to.be.undefined; + }); + }); + + describe('deleteOrphanedBranch', () => { + it('deletes the branch and returns true when no stage references it', async () => { + queryStub.resolves({ records: [] }); + const deleteStub = sinon.stub().resolves({ success: true }); + connectionStub.sobject = sinon.stub().returns({ delete: deleteStub }) as unknown as typeof connectionStub.sobject; + + const deleted = await deleteOrphanedBranch(connectionStub as unknown as Connection, '0Xq000000000001'); + + expect(deleted).to.equal(true); + expect(deleteStub.calledOnceWith('0Xq000000000001')).to.equal(true); + }); + + it('leaves the branch intact and returns false when a stage still references it', async () => { + queryStub.resolves({ records: [{ Id: '0Xp000000000002' }] }); + const deleteStub = sinon.stub().resolves({ success: true }); + connectionStub.sobject = sinon.stub().returns({ delete: deleteStub }) as unknown as typeof connectionStub.sobject; + + const deleted = await deleteOrphanedBranch(connectionStub as unknown as Connection, '0Xq000000000001'); + + expect(deleted).to.equal(false); + expect(deleteStub.called).to.equal(false); + }); + }); }); From 69db646b78a0c6da0141f31d79da1ea38d6a3e56 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Thu, 24 Sep 2026 15:47:24 +0530 Subject: [PATCH 8/9] fix: address PR review comments on promote and work-item update - updateWorkItem: run the status transition guard before the sObject write so a combined --subject/--status call cannot persist the subject change when the status transition is rejected (no partial update). - promote: surface a warning instead of silently swallowing the error when the in-flight promotion check fails, so operators know the duplicate-promotion guard was skipped. - promoteStage: add LIMIT 10 to the findInFlightPromotions SOQL query as a safety net. Co-Authored-By: Claude Opus 4.8 (1M context) --- messages/devops.promote.md | 4 ++++ src/commands/devops/promote.ts | 6 ++++-- src/utils/promoteStage.ts | 2 +- src/utils/updateWorkItem.ts | 14 ++++++++++---- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/messages/devops.promote.md b/messages/devops.promote.md index e243d34b..3c1e6db7 100644 --- a/messages/devops.promote.md +++ b/messages/devops.promote.md @@ -60,6 +60,10 @@ Submit the promotion even if another promotion to the same target stage is alrea A promotion to this stage is already in progress (request token%s: %s). Submitting now can create a duplicate promotion. Wait for it to finish (check with "%s devops request status --request-token "), or pass --force to promote anyway. +# warn.InFlightCheckSkipped + +Couldn't check for in-flight promotions to this stage, so a duplicate promotion may not be blocked: %s + # error.NoModeFlag Provide either --work-item-id to promote specific work items or --stage-id to promote all approved work items from a stage. diff --git a/src/commands/devops/promote.ts b/src/commands/devops/promote.ts index 852a3368..b091cf2b 100644 --- a/src/commands/devops/promote.ts +++ b/src/commands/devops/promote.ts @@ -234,8 +234,10 @@ export default class DevopsPromote extends SfCommand { let inFlight: InFlightPromotion[]; try { inFlight = await findInFlightPromotions(connection, targetStageId); - } catch { - // Guard failure must not break the existing promote flow. + } catch (err) { + // Guard failure must not break the existing promote flow, but surface it so operators can see + // the duplicate-promotion check was skipped rather than silently passing. + this.warn(messages.getMessage('warn.InFlightCheckSkipped', [err instanceof Error ? err.message : String(err)])); return; } if (inFlight.length > 0) { diff --git a/src/utils/promoteStage.ts b/src/utils/promoteStage.ts index 732363a7..1623a2c3 100644 --- a/src/utils/promoteStage.ts +++ b/src/utils/promoteStage.ts @@ -118,7 +118,7 @@ export async function findInFlightPromotions( validateSalesforceId(targetStageId, 'target stage'); const statusList = IN_FLIGHT_PROMOTION_STATUSES.join("', '"); const result = await connection.query( - `SELECT Id, Status, RequestInfoId, RequestInfo.RequestToken FROM DevopsPipelnStgProm WHERE PipelineStageId = '${targetStageId}' AND Status IN ('${statusList}')` + `SELECT Id, Status, RequestInfoId, RequestInfo.RequestToken FROM DevopsPipelnStgProm WHERE PipelineStageId = '${targetStageId}' AND Status IN ('${statusList}') LIMIT 10` ); return (result.records ?? []).map((r) => ({ id: r.Id, diff --git a/src/utils/updateWorkItem.ts b/src/utils/updateWorkItem.ts index 01a4d984..eb1130c6 100644 --- a/src/utils/updateWorkItem.ts +++ b/src/utils/updateWorkItem.ts @@ -131,6 +131,15 @@ export async function resolveProjectIdForWorkItem(connection: Connection, workIt export async function updateWorkItem(params: UpdateWorkItemParams): Promise { const { connection, workItemId, projectId, status, subject, description } = params; + // Validate the status transition before writing any field. This guard is read-only, so running it + // first avoids persisting subject/description when the status change would be rejected — otherwise + // a combined --subject/--status call could silently keep the subject change after throwing. + let apiStatus: string | undefined; + if (status !== undefined) { + apiStatus = toApiStatus(status); + await assertStatusTransitionAllowed(connection, workItemId, apiStatus); + } + if (subject !== undefined || description !== undefined) { const fields: { Id: string; Subject?: string; Description?: string } = { Id: workItemId }; if (subject !== undefined) fields.Subject = subject; @@ -145,10 +154,7 @@ export async function updateWorkItem(params: UpdateWorkItemParams): Promise Date: Mon, 5 Oct 2026 11:59:23 +0530 Subject: [PATCH 9/9] fix: address PR review on force-replace and work-item partial update Environment --force replace now follows the add-then-cleanup pattern used by branch --force: the new environment is associated first and the old record is removed only on success, so a failed add no longer leaves the stage with no environment. Cleanup is best-effort (warns instead of failing) and only deletes an environment no other stage references. Work-item update now returns a partial-success result when the status Connect call fails after the subject/description sObject write has already committed, naming the fields that persisted instead of throwing a bare error that hides them. Co-Authored-By: Claude Opus 4.8 (1M context) --- messages/devops.stage.environment.add.md | 10 ++- src/commands/devops/stage/environment/add.ts | 46 +++++++++--- src/utils/addStageEnvironment.ts | 20 ++++++ src/utils/updateWorkItem.ts | 39 ++++++++-- .../devops/stage/environment/add.test.ts | 72 ++++++++++++++----- test/utils/updateWorkItem.test.ts | 27 +++++++ 6 files changed, 179 insertions(+), 35 deletions(-) diff --git a/messages/devops.stage.environment.add.md b/messages/devops.stage.environment.add.md index 8c3263b5..783f9b6d 100644 --- a/messages/devops.stage.environment.add.md +++ b/messages/devops.stage.environment.add.md @@ -28,7 +28,7 @@ Don't auto-open the browser for OAuth authentication. The redirect URL is printe # flags.force.summary -Replace the stage's existing environment. By default, the command blocks if the stage already has an environment to avoid orphaning it. With this flag, the existing environment is removed before the new one is added. +Replace the stage's existing environment. By default, the command blocks if the stage already has an environment to avoid orphaning it. With this flag, the new environment is added first and the previous one is removed only after the new environment is associated successfully. # examples @@ -68,9 +68,13 @@ Failed to create environment for stage: %s Stage "%s" already has an environment ("%s", %s). Adding another would leave the existing one orphaned. Remove it first with "%s devops stage environment delete", or pass --force to replace it. -# error.ReplaceEnvironmentFailed +# info.ReplacedEnvironmentRemoved -Failed to remove the existing environment before adding the new one: %s +Removed the stage's previous environment "%s". + +# warn.ReplacedEnvironmentCleanupFailed + +Associated the new environment, but couldn't remove the previous environment record (%s). Remove it manually if it's no longer needed. # error.AuthTimeout diff --git a/src/commands/devops/stage/environment/add.ts b/src/commands/devops/stage/environment/add.ts index 28895a8b..709a1e7d 100644 --- a/src/commands/devops/stage/environment/add.ts +++ b/src/commands/devops/stage/environment/add.ts @@ -22,8 +22,9 @@ import { AddStageEnvironmentResult, OrgType, getStageEnvironment, + deleteOrphanedEnvironment, + ExistingStageEnvironment, } from '../../../../utils/addStageEnvironment.js'; -import { deleteStageEnvironment } from '../../../../utils/deleteStageEnvironment.js'; import { fetchPipelineStages } from '../../../../utils/pipelineUtils.js'; import { PipelineStageRecord } from '../../../../utils/types.js'; import { validateSalesforceId } from '../../../../utils/soqlUtils.js'; @@ -144,7 +145,7 @@ export default class DevopsStageEnvironmentAdd extends SfCommand { + private async guardExistingEnvironment( + connection: Connection, + stageId: string, + force: boolean + ): Promise { const existingEnvironment = await getStageEnvironment(connection, stageId); if (!existingEnvironment) { - return; + return undefined; } if (!force) { this.error( @@ -201,9 +211,29 @@ export default class DevopsStageEnvironmentAdd extends SfCommand { + try { + const deleted = await deleteOrphanedEnvironment(connection, existingEnvironment.environmentId); + if (deleted) { + this.log( + messages.getMessage('info.ReplacedEnvironmentRemoved', [ + existingEnvironment.environmentName ?? existingEnvironment.environmentId, + ]) + ); + } + } catch { + this.warn(messages.getMessage('warn.ReplacedEnvironmentCleanupFailed', [existingEnvironment.environmentId])); } } diff --git a/src/utils/addStageEnvironment.ts b/src/utils/addStageEnvironment.ts index 3bb546aa..d49906b1 100644 --- a/src/utils/addStageEnvironment.ts +++ b/src/utils/addStageEnvironment.ts @@ -47,6 +47,26 @@ export async function getStageEnvironment( return { environmentId: record.DevOpsEnvironmentId, environmentName: record.DevOpsEnvironment?.Name }; } +/** + * Deletes a DevopsEnvironment record only when no pipeline stage still references it. + * Returns true when the record was deleted, false when it is still referenced (left intact). + * + * Used to clean up the environment a stage was re-pointed away from during a --force replace, so the + * old record isn't orphaned in the org. The reference check keeps an environment shared by another + * stage safe. + */ +export async function deleteOrphanedEnvironment(connection: Connection, environmentId: string): Promise { + validateSalesforceId(environmentId, 'environment'); + const refs = await connection.query<{ Id: string }>( + `SELECT Id FROM DevopsPipelineStage WHERE DevOpsEnvironmentId = '${environmentId}' LIMIT 1` + ); + if ((refs.records ?? []).length > 0) { + return false; + } + await connection.sobject('DevopsEnvironment').delete(environmentId); + return true; +} + const ORG_TYPE_API_MAP: Record = { Production: 'PRODUCTION', Sandbox: 'SANDBOX', diff --git a/src/utils/updateWorkItem.ts b/src/utils/updateWorkItem.ts index eb1130c6..5d613969 100644 --- a/src/utils/updateWorkItem.ts +++ b/src/utils/updateWorkItem.ts @@ -127,6 +127,10 @@ export async function resolveProjectIdForWorkItem(connection: Connection, workIt * endpoint does not accept them — so they are written via the sObject API. Status changes go * through the connect endpoint (PATCH /connect/devops/projects/{projectId}/workitem/{workItemId}), * which is the only field that endpoint accepts, so platform-side status handling runs. + * + * The two writes hit different APIs with no shared transaction. Subject/description are committed + * first; if the status call then fails, this returns `{ success: false }` with an error that names + * the fields that did persist, rather than throwing and leaving the committed changes unreported. */ export async function updateWorkItem(params: UpdateWorkItemParams): Promise { const { connection, workItemId, projectId, status, subject, description } = params; @@ -156,12 +160,35 @@ export async function updateWorkItem(params: UpdateWorkItemParams): Promise 0 + ? ` The ${persisted.join(' and ')} ${persisted.length > 1 ? 'changes were' : 'change was'} already saved.` + : ''; + return { + success: false, + workItemId, + subject, + description, + error: `Failed to update work item status: ${statusError}.${persistedNote}`, + }; + } } return { diff --git a/test/commands/devops/stage/environment/add.test.ts b/test/commands/devops/stage/environment/add.test.ts index 29f1f03a..a307bcdf 100644 --- a/test/commands/devops/stage/environment/add.test.ts +++ b/test/commands/devops/stage/environment/add.test.ts @@ -28,7 +28,7 @@ describe('devops stage environment add', () => { const mockOrg = { id: '1', getOrgId: () => '1', getConnection: () => mockConnection, getUsername: () => 'testOrg' }; const addStageEnvironmentStub = sinon.stub(); const getStageEnvironmentStub = sinon.stub(); - const deleteStageEnvironmentStub = sinon.stub(); + const deleteOrphanedEnvironmentStub = sinon.stub(); const fetchPipelineStagesStub = sinon.stub(); const execFileStub = sinon.stub(); @@ -37,9 +37,7 @@ describe('devops stage environment add', () => { '../../../../../src/utils/addStageEnvironment.js': { addStageEnvironment: addStageEnvironmentStub, getStageEnvironment: getStageEnvironmentStub, - }, - '../../../../../src/utils/deleteStageEnvironment.js': { - deleteStageEnvironment: deleteStageEnvironmentStub, + deleteOrphanedEnvironment: deleteOrphanedEnvironmentStub, }, '../../../../../src/utils/pipelineUtils.js': { fetchPipelineStages: fetchPipelineStagesStub, @@ -56,8 +54,8 @@ describe('devops stage environment add', () => { addStageEnvironmentStub.reset(); getStageEnvironmentStub.reset(); getStageEnvironmentStub.resolves(undefined); - deleteStageEnvironmentStub.reset(); - deleteStageEnvironmentStub.resolves({ success: true }); + deleteOrphanedEnvironmentStub.reset(); + deleteOrphanedEnvironmentStub.resolves(true); fetchPipelineStagesStub.reset(); execFileStub.reset(); queryStub.reset(); @@ -501,18 +499,18 @@ describe('devops stage environment add', () => { expect(ctx.stderr).to.contain('Existing_Org'); // The new environment must not be created while one already exists. expect(addStageEnvironmentStub.called).to.equal(false); - expect(deleteStageEnvironmentStub.called).to.equal(false); + expect(deleteOrphanedEnvironmentStub.called).to.equal(false); }); test .stdout() .stderr() - .it('with --force, removes the existing environment then adds the new one', async (ctx) => { + .it('with --force, adds the new environment first then removes the previous one', async (ctx) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any sandbox.stub(Org, 'create' as any).returns(mockOrg); fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); - deleteStageEnvironmentStub.resolves({ success: true, environmentId: '0Hi000000000001' }); + deleteOrphanedEnvironmentStub.resolves(true); addStageEnvironmentStub.resolves({ success: true, stageId: '0Xp000000000001', @@ -537,24 +535,25 @@ describe('devops stage environment add', () => { '--force', ]); - // The existing environment is deleted before the new one is created. - expect(deleteStageEnvironmentStub.calledOnce).to.equal(true); - expect(deleteStageEnvironmentStub.firstCall.args[1]).to.equal('0Hi000000000001'); + // The new environment is associated first; the old one is removed only afterward. expect(addStageEnvironmentStub.calledOnce).to.equal(true); - expect(deleteStageEnvironmentStub.calledBefore(addStageEnvironmentStub)).to.equal(true); + expect(deleteOrphanedEnvironmentStub.calledOnce).to.equal(true); + expect(deleteOrphanedEnvironmentStub.firstCall.args[1]).to.equal('0Hi000000000001'); + expect(addStageEnvironmentStub.calledBefore(deleteOrphanedEnvironmentStub)).to.equal(true); expect(ctx.stdout).to.contain('Successfully added environment to the stage.'); expect(ctx.stdout).to.contain('0Hi000000000002'); + expect(ctx.stdout).to.contain("Removed the stage's previous environment"); }); test .stdout() .stderr() - .it('with --force, aborts when removing the existing environment fails', async (ctx) => { + .it('with --force, keeps the existing environment when the add fails', async () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any sandbox.stub(Org, 'create' as any).returns(mockOrg); fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); - deleteStageEnvironmentStub.resolves({ success: false, error: 'delete failed' }); + addStageEnvironmentStub.rejects(new Error('Authentication timed out')); try { await AddEnvironmentCommand.run([ @@ -575,9 +574,46 @@ describe('devops stage environment add', () => { // expected } - expect(ctx.stderr).to.contain('Failed to remove the existing environment'); - // The new environment must not be created if the old one couldn't be removed. - expect(addStageEnvironmentStub.called).to.equal(false); + // The add failed, so the existing environment must be left intact — no delete runs. + expect(deleteOrphanedEnvironmentStub.called).to.equal(false); + }); + + test + .stdout() + .stderr() + .it('with --force, warns but still succeeds when removing the previous environment fails', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + getStageEnvironmentStub.resolves({ environmentId: '0Hi000000000001', environmentName: 'Existing_Org' }); + deleteOrphanedEnvironmentStub.rejects(new Error('delete failed')); + addStageEnvironmentStub.resolves({ + success: true, + stageId: '0Xp000000000001', + environmentId: '0Hi000000000002', + environmentName: 'Production_Org', + orgType: 'Production', + pipelineId: '0Xo000000000001', + organizationId: '00D000000000002', + }); + + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + '--force', + ]); + + // Cleanup failure is a warning, not a command failure — the new environment is already set. + expect(ctx.stdout).to.contain('Successfully added environment to the stage.'); + expect(ctx.stderr).to.contain("couldn't remove the previous environment record"); }); }); }); diff --git a/test/utils/updateWorkItem.test.ts b/test/utils/updateWorkItem.test.ts index 1765d9a6..b516d537 100644 --- a/test/utils/updateWorkItem.test.ts +++ b/test/utils/updateWorkItem.test.ts @@ -187,5 +187,32 @@ describe('updateWorkItem status transition guard', () => { expect(result.status).to.equal('IN_PROGRESS'); expect(result.subject).to.equal('New subject'); }); + + it('returns a partial-success result naming saved fields when the status PATCH fails', async () => { + const updateStub = sinon.stub().resolves({ id: workItemId, success: true, errors: [] }); + (connectionStub.sobject as sinon.SinonStub).returns({ update: updateStub }); + (connectionStub.query as sinon.SinonStub).resolves({ records: [{ Status: 'NEW' }] }); + (connectionStub.request as sinon.SinonStub).rejects(new Error('network error')); + + const result = await updateWorkItem({ + connection: connectionStub as unknown as Connection, + workItemId, + projectId, + subject: 'New subject', + description: 'New description', + status: 'In Progress', + }); + + // The sObject write committed before the status PATCH failed. + expect(updateStub.calledOnce).to.be.true; + expect(result.success).to.be.false; + // Status was not applied, so it is not reported as changed. + expect(result.status).to.be.undefined; + // The subject/description that did persist are reported, and the error names them. + expect(result.subject).to.equal('New subject'); + expect(result.description).to.equal('New description'); + expect(result.error).to.contain('network error'); + expect(result.error).to.contain('subject and description'); + }); }); });