Add PR/MR detection to CI actions with comprehensive test coverage

[Copilot]

• Review existing tests in DetectEnvSpec.groovy
• Identify what PR properties need testing (prActive, prId, prTarget, prTerminology, prNotActiveSkipReason)
• Update existing PR tests to verify new properties
• Add test for prNotActiveSkipReason when not in PR context
• Add test for ADO PR detection
• Add test for Bitbucket PR detection
• Run functional tests to verify changes (all 11 tests pass)
• Refactor CI system check to use ci.type!='unknown' instead of hardcoding CI names

Test Results:

• Updated detect-env-github to verify prActive: false and prNotActiveSkipReason: Not a Pull Request
• Updated detect-env-gitlab to verify prActive: false and prNotActiveSkipReason: Not a Merge Request
• Updated detect-env-github-pr to verify prActive: true, prId: 123, prTarget: main, prTerminology: Pull Request
• Updated detect-env-gitlab-mr to verify prActive: true, prId: 42, prTarget: main, prTerminology: Merge Request
• Added detect-env-ado-pr to verify ADO PR detection with all properties
• Added detect-env-bitbucket-pr to verify Bitbucket PR detection with all properties

All tests pass successfully!

Original prompt

On ci.yaml, In the fcli-ssc ci.yaml action, there's this comment: # TODO Check whether current run is for a PR. There's likely a similar comment in the fcli-fod ci.yaml action.

I'd like you to fix this as follows:

• Based on CiPullRequest.java, update detect-env.yaml to als set PR properties like global.ci.prTarget and global.ci.prActive (better names are welcome, but note that var.set only supports single nested property for global vars, so we likely can't have something like global.ci.pr.active)
• For each CI system, add a property global.ci.prTerminology. For example, for GitHub this would be 'Pull Request' whereas for GitLab this would be 'Merge Request'.
• In the block at the end of detect-env.yaml, if global.ci.prActive is undefined or false, set a property global.ci.prNotActiveSkipReason to something like 'Not a ${global.ci.prTerminology}'
• Then, add this skip reason to the PR_COMMENT steps in both ssc and fod ci.yaml files

Once implemented, please verify that all action files are still properly parseable by running a gradle build and then running the fcli.jar with arguments 'ssc action help ci' and 'fod action help' (although I think this is already done by unit tests).

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[rsenden]

@copilot what if we run detect-env.yaml in a not currently recognized CI system? Would that cause any issues? I guess for not recognized CI systems, we should default to global.ci.prActive=false, and global.ci.prTerminology to 'Pull Request'?

[Copilot]

@copilot what if we run detect-env.yaml in a not currently recognized CI system? Would that cause any issues? I guess for not recognized CI systems, we should default to global.ci.prActive=false, and ...

Good catch! For unrecognized CI systems (Jenkins, local Git, or no CI detection), the PR properties would have been null. I've added default values in commit 4f2565c:

• global.ci.prActive defaults to false if not set
• global.ci.prTerminology defaults to "Pull Request" if blank

This ensures the skip reason logic works correctly for all scenarios.

[rsenden]

@copilot now that we have cinsyatem detection and properties accessible through SpEL function, any way to simplify detect-env.yaml? For example:

• Define default values as appropriate in the first var.set instruction that defines all expected global.ci properties
• Use a single var.set statement instead of the individual CI-specific var.set statements in combination with SpEL conditional navigation operator ?. to set those CI-specific properties from the detected CI system, defaulting to default value defined above if property not available
• Where appropriate, add properties to the Java model, like for PR terminology

[Copilot]

@copilot now that we have cinsyatem detection and properties accessible through SpEL function, any way to simplify detect-env.yaml? For example:

• Define default values as appropriate in the first va...

Done in commit f18202b:

Java models:

• Added prTerminology property to all CI environment classes (GitHub, GitLab, ADO, Bitbucket)
• Each sets its constant: PR_TERMINOLOGY = "Pull Request" (or "Merge Request" for GitLab)

detect-env.yaml simplification:

• Set default values upfront: prActive: false, prTerminology: "Pull Request", sourceDir: "."
• Consolidated CI-specific blocks: separate name/id mappings, then single shared block for common properties
• Used conditional navigation operator ?. to safely extract properties from ci.env (e.g., ${ci.env?.pullRequest?.active?:false})
• Removed redundant default-setting logic at the end since defaults are now set upfront
• Kept Jenkins detection and local repo fallback blocks intact

Result: 15 fewer lines, clearer structure, and PR terminology now comes from Java models instead of being hardcoded in YAML.

[rsenden]

@copilot Can we simplify further by adding CI name/id to the Java-based CI detection logic?

[Copilot]

@copilot Can we simplify further by adding CI name/id to the Java-based CI detection logic?

Done in commit 886c46d:

Java models:

• Added ciName and ciId properties to all CI environment records
• Each defines constants: NAME (e.g., "GitHub", "Azure DevOps") and ID (e.g., "github", "ado")

detect-env.yaml simplification:

• Removed all individual name/id mapping blocks (4 separate if statements)
• Now extracts ciName and ciId directly from ci.env in the shared property extraction block

Result: 18 fewer lines in detect-env.yaml. All CI-specific constants are now defined in Java models, making the YAML configuration purely data-driven.

[rsenden]

@copilot I don't like this check for known CI systems, as we'd need to update this if we add support for new CI system in Java code. Can we reverse the check to check for 'not an unknown CI system'? What's returned by #_ci.detect() code in case of unknown CI system? Do we need any safe navigation in lines 49 & 50?

[Copilot]

Fixed in commit 27cd8c0:

Changed the check from ${ci.type=='github'||ci.type=='gitlab'||ci.type=='ado'||ci.type=='bitbucket'} to ${ci.type!='unknown'}.

#_ci.detect() returns ActionUnknownCiSpelFunctions for unknown CI systems, which has:

• type = "unknown"
• env returns an empty ObjectNode (never null)

Lines 49 & 50 don't need safe navigation because ci.detected is always non-null, and both type and env properties are always available. The ?. operators in lines 56-65 are still needed for navigating properties within ci.env since it may be empty for unknown CI systems.

All 11 functional tests still pass.

[rsenden]

@copilot Please check review comment and fix

[Copilot]

@copilot Please check review comment and fix

I've already addressed the review comment in commit 27cd8c0, changing the check from hardcoded CI system names to ${ci.type!='unknown'}.

Could you please clarify what still needs to be fixed? The current implementation:

• Uses ci.type!='unknown' instead of listing CI systems (line 54)
• All 11 functional tests pass
• No safe navigation needed on lines 49-50 as ci.detected, type, and env are always non-null

Is there a specific issue you'd like me to address?