feat: add Docker validation step and pre-pull Edgescan image in workflow #151
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideIntegrates Edgescan security scanning into both GitHub Actions and Azure DevOps by validating Docker and pre-pulling the Edgescan image, adding a reusable Azure DevOps EdgeScan stage with cached Docker image usage, wiring it into the main pipeline, introducing a local npm script to run scans via Docker, tightening CI env var checks for Edgescan credentials, and configuring pnpm audit to ignore a specific advisory. Sequence diagram for Azure DevOps EdgeScan stage with Docker image cachingsequenceDiagram
participant AzurePipeline
participant CacheTask as CacheTask_Cache2
participant BashPullOrRestore as Bash_PullOrRestore
participant Docker as DockerEngine
participant DockerHub as DockerHub_edgescan_image
participant BashRunScan as Bash_RunScan
participant EdgeScan as EdgescanAPI
AzurePipeline->>CacheTask: Execute Cache@2
CacheTask-->>AzurePipeline: Set DOCKER_CACHE_HIT and restore cache dir
AzurePipeline->>BashPullOrRestore: Run Pull or Restore EdgeScan Image script
alt Cache hit and tarball exists
BashPullOrRestore->>Docker: docker load -i edgescan.tar
else Cache miss or missing tarball
BashPullOrRestore->>DockerHub: docker pull edgescan/cicd-integration:latest
DockerHub-->>BashPullOrRestore: Image layers
BashPullOrRestore->>Docker: docker save edgescan/cicd-integration:latest -o edgescan.tar
end
AzurePipeline->>BashRunScan: Run EdgeScan Scan script
BashRunScan->>BashRunScan: Validate ES_API_TOKEN and esAssetId parameters
BashRunScan->>Docker: docker run edgescan/cicd-integration:latest
Docker->>EdgeScan: Call EdgeScan API with api-token and asset-id
EdgeScan-->>Docker: Scan results and status
Docker-->>BashRunScan: Container exit code
BashRunScan-->>AzurePipeline: Job success or failure
Flow diagram for monorepo EdgeScan Azure DevOps stageflowchart TD
A[Start EdgeScan stage] --> B[Cache EdgeScan Docker image<br/>Cache@2]
B --> C{DOCKER_CACHE_HIT is true<br/>and edgescan.tar exists?}
C -- Yes --> D[Load image from cache<br/>docker load -i edgescan.tar]
C -- No --> E[Pull image from Docker Hub<br/>docker pull edgescan/cicd-integration:latest]
E --> F[Save image to cache<br/>docker save -o edgescan.tar]
D --> G[Validate ES_API_TOKEN<br/>env var]
F --> G
G --> H{ES_API_TOKEN set?}
H -- No --> X[Fail: ES_API_TOKEN must be set]
H -- Yes --> I[Validate esAssetId<br/>parameter]
I --> J{esAssetId set?}
J -- No --> Y[Fail: ES_ASSET_ID_DEV must be set]
J -- Yes --> K[Run EdgeScan scan via Docker<br/>--start-scan --max-risk-threshold 3 --wait --color]
K --> Z[End EdgeScan stage]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
… EdgeScan CI/CD integration stage
Member
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
monorepo-edgescan-stage.ymltemplate defines anesAssetIdparameter but the template call inazure-pipelines.ymldoes not pass it, so the stage will run with an empty asset ID; consider wiring this parameter through explicitly. - In
build-pipeline/core/monorepo-edgescan-stage.ymlyou validate onlyES_API_TOKENbefore running Docker, but not the asset ID; adding a similar check for the resolvedesAssetIdvalue would make failures clearer and more consistent with the GitHub Actions workflow. - Both in the GitHub Actions step and in the
edgescan:runscript, theedgescan/cicd-integrationimage is pulled without a tag; pinning to a specific version tag (or at leastlatestexplicitly) would make the behavior more predictable and easier to debug if the upstream image changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `monorepo-edgescan-stage.yml` template defines an `esAssetId` parameter but the template call in `azure-pipelines.yml` does not pass it, so the stage will run with an empty asset ID; consider wiring this parameter through explicitly.
- In `build-pipeline/core/monorepo-edgescan-stage.yml` you validate only `ES_API_TOKEN` before running Docker, but not the asset ID; adding a similar check for the resolved `esAssetId` value would make failures clearer and more consistent with the GitHub Actions workflow.
- Both in the GitHub Actions step and in the `edgescan:run` script, the `edgescan/cicd-integration` image is pulled without a tag; pinning to a specific version tag (or at least `latest` explicitly) would make the behavior more predictable and easier to debug if the upstream image changes.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:28-37` </location>
<code_context>
+ displayName: 'Run EdgeScan Scan'
+ inputs:
+ targetType: 'inline'
+ script: |
+ if [ -z "$(ES_API_TOKEN)" ]; then
+ echo "Error: ES_API_TOKEN is not set in the variable group."
+ exit 1
+ fi
+
+ docker pull edgescan/cicd-integration
+
+ echo "Triggering EdgeScan for Asset ID: ${{parameters.esAssetId}}"
+
+ docker run --rm \
+ -e ES_API_TOKEN=$(ES_API_TOKEN) \
+ -e ES_ASSET_ID=${{parameters.esAssetId}} \
</code_context>
<issue_to_address>
**suggestion:** Add basic shell safety options in the EdgeScan Bash script.
Consider adding strict bash options at the top of the script block:
```bash
set -euo pipefail
if [ -z "$(ES_API_TOKEN)" ]; then
echo "Error: ES_API_TOKEN is not set in the variable group."
exit 1
fi
```
This will fail fast on command errors and unset variables, which is valuable for this pipeline step.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR integrates Edgescan security scanning into the CellixJS project's CI/CD pipelines by adding Docker-based validation, pre-pulling the Edgescan container image, and configuring required secrets and environment variables across GitHub Actions and Azure DevOps workflows.
- Adds Edgescan CI/CD integration with Docker-based security scanning
- Updates security exception configurations in .snyk with new vulnerability ignores and extended expiration dates
- Enforces Edgescan-related secrets (ES_API_TOKEN, ES_ASSET_ID) as required environment variables in workflows
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds edgescan:run npm script for Docker-based security scanning and configures pnpm audit to ignore GHSA-6rw7-vpxm-498p |
| build-pipeline/core/monorepo-edgescan-stage.yml | Introduces new Azure Pipeline stage template for Edgescan security scanning with Docker integration |
| azure-pipelines.yml | Adds edgescan-credential-cellixjs variable group and integrates EdgeScan_DEV stage after DEV deployment |
| .snyk | Extends expiration for sirv vulnerability exception and adds new ignore rule for qs vulnerability (SNYK-JS-QS-14724253) |
| .github/workflows/copilot-setup-steps.yml | Adds Docker validation step, pre-pulls Edgescan image, and enforces ES_API_TOKEN and ES_ASSET_ID as required secrets |
…SET_ID and fail the pipeline on error
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
monorepo-edgescan-stage.yml, the Bash variable expansions use$(ES_ASSET_ID)and$(ES_API_TOKEN)instead of$ES_ASSET_ID/$ES_API_TOKEN, which will break underset -euo pipefailand should be corrected to standard shell variable syntax. - The EdgeScan stage currently mixes the
esAssetIdparameter with theES_ASSET_IDenvironment variable; consider standardizing on one source of truth (e.g., only the parameter or only the variable group) to avoid confusion and misconfiguration. - The error message in the EdgeScan Bash step prints
Error: Error:; you can simplify this to a single prefix to keep logs cleaner and easier to scan.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `monorepo-edgescan-stage.yml`, the Bash variable expansions use `$(ES_ASSET_ID)` and `$(ES_API_TOKEN)` instead of `$ES_ASSET_ID` / `$ES_API_TOKEN`, which will break under `set -euo pipefail` and should be corrected to standard shell variable syntax.
- The EdgeScan stage currently mixes the `esAssetId` parameter with the `ES_ASSET_ID` environment variable; consider standardizing on one source of truth (e.g., only the parameter or only the variable group) to avoid confusion and misconfiguration.
- The error message in the EdgeScan Bash step prints `Error: Error:`; you can simplify this to a single prefix to keep logs cleaner and easier to scan.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:31-32` </location>
<code_context>
+ script: |
+ set -euo pipefail
+
+ if [ -z "${ES_API_TOKEN}" ] || [ -z "$(ES_ASSET_ID)" ]; then
+ echo "Error: Error: ES_API_TOKEN and ES_ASSET_ID must be set in the variable group."
+ exit 1
+ fi
</code_context>
<issue_to_address>
**issue (bug_risk):** The ES_ASSET_ID check is using command substitution instead of reading the variable, and will not behave as intended under `set -euo pipefail`.
In this condition, `"$(ES_ASSET_ID)"` executes the value as a command instead of checking the variable, which can fail under `set -euo pipefail`. Make it consistent with the `ES_API_TOKEN` check, e.g.
```bash
if [ -z "${ES_API_TOKEN}" ] || [ -z "${ES_ASSET_ID}" ]; then
echo "Error: ES_API_TOKEN and ES_ASSET_ID must be set in the variable group."
exit 1
fi
```
This also removes the duplicated "Error: Error:" prefix.
</issue_to_address>
### Comment 2
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:40-41` </location>
<code_context>
+
+ echo "Triggering EdgeScan for Asset ID: ${{parameters.esAssetId}}"
+
+ docker run --rm \
+ -e ES_API_TOKEN=$(ES_API_TOKEN) \
+ -e ES_ASSET_ID=${{parameters.esAssetId}} \
+ -e MAX_RISK_THRESHOLD=3 \
</code_context>
<issue_to_address>
**issue (bug_risk):** Environment variables are passed to `docker run` using command substitution, which will attempt to execute commands instead of expanding the variables.
`$(ES_API_TOKEN)` invokes a command named by the value of `ES_API_TOKEN` instead of expanding the variable, and with `set -euo pipefail` this will almost certainly fail the job. The same issue applies to `ES_ASSET_ID` above.
For `docker run`, either rely on the existing `env:` block:
```bash
docker run --rm \
-e ES_API_TOKEN \
-e ES_ASSET_ID=${{ parameters.esAssetId }} \
-e MAX_RISK_THRESHOLD=3 \
edgescan/cicd-integration:latest --wait --color
```
or use normal variable expansion:
```bash
-e ES_API_TOKEN="${ES_API_TOKEN}" \
-e ES_ASSET_ID="${{ parameters.esAssetId }}" \
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ASSET_ID validation; fix syntax errors
…run command uses tty for better output
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
monorepo-edgescan-stage.ymltemplate declares a requiredesAssetIdparameter but theazure-pipelines.ymltemplate inclusion ofmonorepo-edgescan-stage.ymldoes not pass a value, which will cause template validation/runtime issues; wire in the appropriate variable (e.g. a DEV asset ID) when invoking the template. - In the GitHub Actions 'Validate Docker and Pre-Pull Edgescan Image' step, consider making the Docker validation output more explicit (e.g.
docker infowith an explanatory message and graceful failure) so that CI failures due to missing Docker are easier to diagnose than just a failingdocker ps.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `monorepo-edgescan-stage.yml` template declares a required `esAssetId` parameter but the `azure-pipelines.yml` template inclusion of `monorepo-edgescan-stage.yml` does not pass a value, which will cause template validation/runtime issues; wire in the appropriate variable (e.g. a DEV asset ID) when invoking the template.
- In the GitHub Actions 'Validate Docker and Pre-Pull Edgescan Image' step, consider making the Docker validation output more explicit (e.g. `docker info` with an explanatory message and graceful failure) so that CI failures due to missing Docker are easier to diagnose than just a failing `docker ps`.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:31-39` </location>
<code_context>
# Deploy API package with production dependencies
- task: Bash@3
displayName: 'Artifact: Prepare API'
- condition: and(succeeded(), eq(variables['BuildJob.HAS_BACKEND_CHANGES'], 'true'), ne(variables['Build.Reason'], 'PullRequest'))
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `set -u` with `DOCKER_CACHE_HIT` can cause an unbound variable error when the cache is missed.
Because the script uses `set -euo pipefail`, referencing `DOCKER_CACHE_HIT` directly will cause an "unbound variable" exit when the cache is missed and `Cache@2` hasn’t set `cacheHitVar`.
Please guard this by giving the variable a default, e.g.:
```bash
if [ "${DOCKER_CACHE_HIT:-}" = "true" ] && [ -f "$(Pipeline.Workspace)/docker-cache/edgescan.tar" ]; then
```
or set a default once after `set -euo pipefail`:
```bash
DOCKER_CACHE_HIT="${DOCKER_CACHE_HIT:-false}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…eparation and deployment stages
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
monorepo-edgescan-stage.ymltemplate defines anesAssetIdparameter but theEdgeScan_DEVstage inazure-pipelines.ymldoesn’t pass a value for it, which will cause template validation/runtime issues—either provide the parameter in the call or give it a sensible default. - In the EdgeScan stage Bash script, the error message for a missing asset ID refers to
ES_ASSET_ID_DEVcoming from a variable group even though you’re actually using theesAssetIdtemplate parameter, which could confuse future maintainers; consider aligning the error message and naming with the actual input source.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `monorepo-edgescan-stage.yml` template defines an `esAssetId` parameter but the `EdgeScan_DEV` stage in `azure-pipelines.yml` doesn’t pass a value for it, which will cause template validation/runtime issues—either provide the parameter in the call or give it a sensible default.
- In the EdgeScan stage Bash script, the error message for a missing asset ID refers to `ES_ASSET_ID_DEV` coming from a variable group even though you’re actually using the `esAssetId` template parameter, which could confuse future maintainers; consider aligning the error message and naming with the actual input source.
## Individual Comments
### Comment 1
<location> `package.json:51` </location>
<code_context>
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
- "prepare": "husky"
+ "prepare": "husky",
+ "edgescan:run": "docker run --tty --rm edgescan/cicd-integration:latest --api-token $ES_API_TOKEN --asset-id $ES_ASSET_ID --start-scan --max-risk-threshold 3 --wait --color"
},
"devDependencies": {
</code_context>
<issue_to_address>
**suggestion:** The `edgescan:run` script relies on POSIX-style env var expansion, which may be brittle on Windows.
This npm script assumes a POSIX shell for `$ES_API_TOKEN` / `$ES_ASSET_ID` expansion, so it will fail on default Windows shells like `cmd.exe`. If cross‑platform use is required, consider `cross-env` (e.g. `"cross-env ES_API_TOKEN=... ES_ASSET_ID=... docker run ..."`) or another shell-agnostic approach for passing these variables.
Suggested implementation:
```
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
"prepare": "husky",
"edgescan:run": "docker run --tty --rm --env ES_API_TOKEN --env ES_ASSET_ID edgescan/cicd-integration:latest --start-scan --max-risk-threshold 3 --wait --color"
},
```
This change assumes that `edgescan/cicd-integration:latest` can read `ES_API_TOKEN` and `ES_ASSET_ID` from its environment instead of requiring `--api-token` / `--asset-id` CLI flags. If it strictly requires the flags, you will need to:
1. Confirm if the container provides an alternative way (e.g. reading env vars) or
2. Introduce a small wrapper script inside the container (or use a cross-platform runner like `cross-env-shell`) that maps env vars to CLI flags in a shell-agnostic way.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
monorepo-edgescan-stage.ymltemplate defines a requiredesAssetIdparameter butazure-pipelines.ymlcalls the template without supplying it, which will cause a template validation/runtime error; wire the appropriate asset id variable into that parameter. - The
edgescan:runnpm script relies on$ES_API_TOKENand$ES_ASSET_IDshell expansion, which will not work as written on Windows runners; consider using a cross-platform approach (e.g.cross-envorenvprefix) if this script is intended to be usable outside Linux.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `monorepo-edgescan-stage.yml` template defines a required `esAssetId` parameter but `azure-pipelines.yml` calls the template without supplying it, which will cause a template validation/runtime error; wire the appropriate asset id variable into that parameter.
- The `edgescan:run` npm script relies on `$ES_API_TOKEN` and `$ES_ASSET_ID` shell expansion, which will not work as written on Windows runners; consider using a cross-platform approach (e.g. `cross-env` or `env` prefix) if this script is intended to be usable outside Linux.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
monorepo-edgescan-stage.ymltemplate defines a requiredesAssetIdparameter, but theazure-pipelines.ymltemplate usage does not passesAssetId, so the stage will currently run with an empty asset id and fail the validation check—wire the correct variable through the template parameters. - The
edgescan:runnpm script relies on$ES_API_TOKENand$ES_ASSET_IDshell expansion, which will not work on Windows shells; consider using a cross‑platform approach (e.g.,docker run --env ES_API_TOKEN --env ES_ASSET_ID ...) so the script works consistently across environments. - In
monorepo-edgescan-stage.yml, the error messageES_ASSET_ID_DEV must be set in the variable groupis hard-coded while the check is against the genericesAssetIdparameter; aligning the message with the parameter name or making it environment-agnostic would reduce confusion for future reuse of this template.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `monorepo-edgescan-stage.yml` template defines a required `esAssetId` parameter, but the `azure-pipelines.yml` template usage does not pass `esAssetId`, so the stage will currently run with an empty asset id and fail the validation check—wire the correct variable through the template parameters.
- The `edgescan:run` npm script relies on `$ES_API_TOKEN` and `$ES_ASSET_ID` shell expansion, which will not work on Windows shells; consider using a cross‑platform approach (e.g., `docker run --env ES_API_TOKEN --env ES_ASSET_ID ...`) so the script works consistently across environments.
- In `monorepo-edgescan-stage.yml`, the error message `ES_ASSET_ID_DEV must be set in the variable group` is hard-coded while the check is against the generic `esAssetId` parameter; aligning the message with the parameter name or making it environment-agnostic would reduce confusion for future reuse of this template.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:42` </location>
<code_context>
+ set -euo pipefail
+ mkdir -p $(Pipeline.Workspace)/docker-cache
+
+ if [ "$DOCKER_CACHE_HIT" == "true" ] && [ -f "$(Pipeline.Workspace)/docker-cache/edgescan.tar" ]; then
+ echo "Restoring EdgeScan image from cache..."
+ docker load -i $(Pipeline.Workspace)/docker-cache/edgescan.tar
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `set -u` with `DOCKER_CACHE_HIT` can cause a shell error when the cache is not hit.
Because `-u` treats unset variables as errors, a cache miss (where `Cache@2` doesn’t define `DOCKER_CACHE_HIT`) will cause `[ "$DOCKER_CACHE_HIT" == "true" ]` to abort the script. Consider guarding it with a default, e.g. `if [ "${DOCKER_CACHE_HIT:-}" = "true" ] && ...`, or disabling `-u` for this section.
</issue_to_address>
### Comment 2
<location> `package.json:51` </location>
<code_context>
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
- "prepare": "husky"
+ "prepare": "husky",
+ "edgescan:run": "docker run --tty --rm edgescan/cicd-integration:latest --api-token $ES_API_TOKEN --asset-id $ES_ASSET_ID --start-scan --max-risk-threshold 3 --wait --color"
},
"devDependencies": {
</code_context>
<issue_to_address>
**suggestion:** Environment variable expansion in the `edgescan:run` script is not portable across platforms.
This works in POSIX shells but not in Windows `cmd.exe`, which is what `npm` uses by default, so Docker would receive the literal `$ES_API_TOKEN`/`$ES_ASSET_ID`. If you need this script to be cross‑platform, consider using `cross-env` (e.g. `cross-env ES_API_TOKEN=... ES_ASSET_ID=... docker run ...`) or clearly document that it’s only supported in POSIX environments.
Suggested implementation:
```
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
"prepare": "husky",
"edgescan:run": "cross-env-shell \"docker run --tty --rm edgescan/cicd-integration:latest --api-token $ES_API_TOKEN --asset-id $ES_ASSET_ID --start-scan --max-risk-threshold 3 --wait --color\""
},
"devDependencies": {
"@amiceli/vitest-cucumber": "^5.1.2",
"cross-env-shell": "^5.0.5",
```
If you prefer not to add a new dev dependency, an alternative is to keep the original `docker run ...` script and explicitly document in your README or contributing guide that `edgescan:run` is only supported in POSIX environments (e.g. via a short section explaining that `$ES_API_TOKEN` / `$ES_ASSET_ID` expansion requires a POSIX shell).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
monorepo-edgescan-stage.ymltemplate requires anesAssetIdparameter, but the usage inazure-pipelines.ymldoesn’t pass it; this will fail template validation and should be wired to the appropriate variable (e.g.$(ES_ASSET_ID_DEV)or similar). - In the EdgeScan stage script, the error message references
ES_ASSET_ID_DEVwhile the parameter is namedesAssetId; consider aligning the message and naming so it’s clear which variable/parameter is actually required.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `monorepo-edgescan-stage.yml` template requires an `esAssetId` parameter, but the usage in `azure-pipelines.yml` doesn’t pass it; this will fail template validation and should be wired to the appropriate variable (e.g. `$(ES_ASSET_ID_DEV)` or similar).
- In the EdgeScan stage script, the error message references `ES_ASSET_ID_DEV` while the parameter is named `esAssetId`; consider aligning the message and naming so it’s clear which variable/parameter is actually required.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:64-67` </location>
<code_context>
+ exit 1
+ fi
+
+ if [ -z "${{ parameters.esAssetId }}" ]; then
+ echo "Error: ES_ASSET_ID_DEV must be set in the variable group."
+ exit 1
+ fi
</code_context>
<issue_to_address>
**suggestion:** Error message mentions `ES_ASSET_ID_DEV` and variable group, but the check is actually on the `esAssetId` template parameter.
Since the logic validates `parameters.esAssetId`, the message should either mention that parameter explicitly (e.g. `esAssetId parameter is not set`) or interpolate the actual variable name being used. This will avoid confusion when the parameter is sourced differently across environments and make failed runs easier to diagnose.
```suggestion
if [ -z "${{ parameters.esAssetId }}" ]; then
echo "Error: esAssetId template parameter is not set."
exit 1
fi
```
</issue_to_address>
### Comment 2
<location> `package.json:51` </location>
<code_context>
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
- "prepare": "husky"
+ "prepare": "husky",
+ "edgescan:run": "docker run --tty --rm edgescan/cicd-integration:latest --api-token $ES_API_TOKEN --asset-id $ES_ASSET_ID --start-scan --max-risk-threshold 3 --wait --color"
},
"devDependencies": {
</code_context>
<issue_to_address>
**suggestion:** Using `$ES_API_TOKEN` and `$ES_ASSET_ID` directly in the npm script can be fragile on non-POSIX environments.
Because npm/pnpm scripts may be run on Windows, `$VAR` expansion in the command can break under non-POSIX shells. If this script needs to be cross-platform, either note that it’s Unix-only or move the logic into a small Bash (or similar) wrapper script checked into the repo to control env handling.
Suggested implementation:
```
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
"prepare": "husky",
"edgescan:run": "node ./scripts/edgescan-run.js"
},
```
Create a new file at `scripts/edgescan-run.js` (and ensure it is committed and executable where needed) with logic similar to:
```js
#!/usr/bin/env node
const { spawn } = require('child_process');
const { ES_API_TOKEN, ES_ASSET_ID } = process.env;
if (!ES_API_TOKEN || !ES_ASSET_ID) {
console.error('Missing ES_API_TOKEN or ES_ASSET_ID environment variables.');
process.exit(1);
}
const args = [
'--tty',
'--rm',
'edgescan/cicd-integration:latest',
'--api-token',
ES_API_TOKEN,
'--asset-id',
ES_ASSET_ID,
'--start-scan',
'--max-risk-threshold',
'3',
'--wait',
'--color',
];
const child = spawn('docker', ['run', ...args], { stdio: 'inherit' });
child.on('exit', (code) => {
process.exit(code ?? 1);
});
```
This wrapper:
1. Reads `ES_API_TOKEN` and `ES_ASSET_ID` from the environment via `process.env`.
2. Invokes `docker run` without relying on shell-level `$VAR` expansion, making the npm script cross-platform.
3. For stricter environments, you may also want to add a short note in the README indicating that Docker must be available on the PATH for this script to work.
</issue_to_address>
### Comment 3
<location> `.github/workflows/copilot-setup-steps.yml:80-84` </location>
<code_context>
distribution: 'temurin'
java-version: '17'
+ - name: Validate Docker and Pre-Pull Edgescan Image
+ run: |
+ docker --version
+ docker ps
+ docker pull edgescan/cicd-integration:latest # Ensure Edgescan image is available in this job and verify Docker access
+
- name: Verify required environment variables
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Pulling the `latest` Edgescan image in CI may introduce non-determinism and harder-to-debug failures.
Because this workflow uses a floating `latest` tag, its behavior can change independently of any code changes, making failures harder to reproduce and debug. Please pin the image to a specific version or digest (e.g. `edgescan/cicd-integration:<version>` or `@sha256:...`) and update it intentionally when upgrading.
Suggested implementation:
```
- name: Validate Docker and Pre-Pull Edgescan Image
run: |
docker --version
docker ps
# NOTE: Use a pinned Edgescan image version/digest to keep CI runs deterministic.
# Update this tag (or replace with an image digest) intentionally when upgrading.
docker pull edgescan/cicd-integration:1.0.0 # Ensure Edgescan image is available in this job and verify Docker access
```
You should replace `1.0.0` with the actual version tag or image digest you currently use/approve for this CI workflow, e.g. `edgescan/cicd-integration:2.3.4` or `edgescan/cicd-integration@sha256:<digest>`. If this image/tag is referenced elsewhere in the repo (e.g. in scripts or other workflows), consider centralizing it via a workflow `env:` variable to avoid divergence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai dismiss |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
monorepo-edgescan-stage.ymltemplate declares a requiredesAssetIdparameter but the usage inazure-pipelines.ymldoesn’t supply it, so the pipeline will likely fail at template expansion time; either provideesAssetIdin the template call or give it a sensible default. - In the EdgeScan stage, the error message
'ES_ASSET_ID_DEV must be set in the variable group.'is misleading because the value comes from theesAssetIdparameter, not a variable group—consider updating the message to reference the parameter/asset ID source accurately. - The
edgescan:runnpm script relies on$ES_API_TOKENand$ES_ASSET_ID, which are POSIX-specific environment variable syntax; if you expect this script to be run on Windows as well, consider using a cross-platform approach (e.g.cross-env) or documenting the OS assumption.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `monorepo-edgescan-stage.yml` template declares a required `esAssetId` parameter but the usage in `azure-pipelines.yml` doesn’t supply it, so the pipeline will likely fail at template expansion time; either provide `esAssetId` in the template call or give it a sensible default.
- In the EdgeScan stage, the error message `'ES_ASSET_ID_DEV must be set in the variable group.'` is misleading because the value comes from the `esAssetId` parameter, not a variable group—consider updating the message to reference the parameter/asset ID source accurately.
- The `edgescan:run` npm script relies on `$ES_API_TOKEN` and `$ES_ASSET_ID`, which are POSIX-specific environment variable syntax; if you expect this script to be run on Windows as well, consider using a cross-platform approach (e.g. `cross-env`) or documenting the OS assumption.
## Individual Comments
### Comment 1
<location> `build-pipeline/core/monorepo-edgescan-stage.yml:64-67` </location>
<code_context>
+ exit 1
+ fi
+
+ if [ -z "${{ parameters.esAssetId }}" ]; then
+ echo "Error: ES_ASSET_ID_DEV must be set in the variable group."
+ exit 1
+ fi
</code_context>
<issue_to_address>
**suggestion:** The error message suggests a variable-group secret, but the value actually comes from a template parameter.
This checks `parameters.esAssetId`, but the error references `ES_ASSET_ID_DEV` and a variable group. Consider updating the message to reference the parameter (or the actual pipeline variable passed into `esAssetId`) so it clearly indicates what’s misconfigured.
```suggestion
if [ -z "${{ parameters.esAssetId }}" ]; then
echo "Error: esAssetId template parameter must be provided (check the pipeline variable passed into 'esAssetId')."
exit 1
fi
```
</issue_to_address>
### Comment 2
<location> `package.json:51` </location>
<code_context>
"analyze": "pnpm -r exec -- pnpm dlx @e18e/cli analyze",
- "prepare": "husky"
+ "prepare": "husky",
+ "edgescan:run": "docker run --tty --rm edgescan/cicd-integration:latest --api-token $ES_API_TOKEN --asset-id $ES_ASSET_ID --start-scan --max-risk-threshold 3 --wait --color"
},
"devDependencies": {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Environment variables in the `edgescan:run` script should be quoted to avoid issues with special characters or spaces.
If `ES_API_TOKEN` or `ES_ASSET_ID` contain shell-special characters or spaces, this command can fail. Please quote them in the script: `--api-token "$ES_API_TOKEN" --asset-id "$ES_ASSET_ID"`.
```suggestion
"edgescan:run": "docker run --tty --rm edgescan/cicd-integration:latest --api-token \"$ES_API_TOKEN\" --asset-id \"$ES_ASSET_ID\" --start-scan --max-risk-threshold 3 --wait --color"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate Edgescan security scanning into GitHub and Azure pipelines and ensure required Docker/image and credentials are available.
New Features:
Enhancements:
Build:
CI: