-
Notifications
You must be signed in to change notification settings - Fork 107
chore(ci): integration test schedule #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
christian-bromann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of integration test provide a lot of valuable validations. Let's make an effort first migrating those to unit tests IMHO.
|
🤖 Devin AI is starting automated code review... |
📋 WalkthroughThis PR adds a scheduled integration-tests GitHub Actions workflow and removes the integration job from the main CI workflow. ◆ Sequence DiagramsequenceDiagram
autonumber
ScheduledWorkflow->>CI Runner: cron trigger (integration-tests.yml)
CI Runner->>Repo: checkout
CI Runner->>Build: install deps & build
Build->>TestRunner: run integration tests
TestRunner->>CI Runner: upload artifacts / test results
CI Runner->>Repo: annotate / report status (optional)
📊 Changes
|
| name: Integration Tests | ||
|
|
||
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overly-permissive workflow permission: actions: write
The workflow requests actions: write at the workflow-level permissions. actions: write grants the workflow the ability to create or modify workflow runs and is broader than typically required for a scheduled integration job. Scheduled workflows that run with repository secrets should use least privilege to reduce risk.
🔧 Suggested fix
# Reduce permissions to the minimum required. If you only need to read contents and run actions, prefer read-only.
permissions:
contents: read
actions: read
# Or remove the `actions` key entirely if not required by any step.If a specific step requires higher privileges, escalate permissions only for that job and document why.
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In `.github/workflows/integration-tests.yml` at line ~6, change workflow-level permissions to least-privilege. If any steps truly require `actions: write`, add a comment explaining why and scope that permission to the smallest job possible.
</details>
| steps: | ||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| - name: Install pnpm | ||
| uses: pnpm/action-setup@41ff72655975bd51cab0327fa583b6e92b6d3061 # v4.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Robustness: archive creation suppresses errors and may produce an empty/corrupt artifact
The tar invocation suppresses stderr and falls back, hiding errors. If none of the referenced paths exist, or tar fails, the command will either produce an empty archive or the fallback may also fail — but errors are hidden. This can cause actions/upload-artifact to upload a missing or invalid artifact.
🔧 Suggested fix
- name: Create build archive
run: |
set -euo pipefail
FILES=()
for p in libs/*/dist libs/*/.turbo .turbo; do
if [ -e "$p" ]; then
FILES+=("$p")
fi
done
if [ ${#FILES[@]} -eq 0 ]; then
echo "No build artifacts found" >&2
exit 1
fi
tar -czf build-artifacts.tar.gz "${FILES[@]}"This avoids hiding errors and ensures a valid artifact is produced. Also remove redundant paths (e.g., .turbo listed twice).
🤖 Prompt for AI Agents
In .github/workflows/integration-tests.yml at line ~22 replace the single-line tar command with a robust multi-line script that checks for artifact presence and exits non-zero if none are found.
|
|
||
| on: | ||
| schedule: | ||
| # Run every Monday at 14:00 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential mismatch between pinned action SHA and input support (setup-node)
The workflow pins actions/setup-node to a specific commit SHA but uses the node-version-file input. Not all historical commit SHAs accept this input; if the pinned commit doesn't support it, the step may ignore the input or fail unexpectedly.
🔧 Suggested fix
Prefer using a released tag that supports the input, e.g. actions/setup-node@v4, or explicitly specify node-version:
- uses: actions/setup-node@v4
with:
node-version-file: .nvmrc
cache: pnpm
# or
- uses: actions/setup-node@v4
with:
node-version: '24'
cache: pnpmPinning to a stable release tag reduces the risk of using a commit SHA that lacks expected features.
🤖 Prompt for AI Agents
In .github/workflows/integration-tests.yml at line ~11, replace the commit SHA pin for actions/setup-node with a maintained release tag (e.g., v4) or change the input to node-version if you prefer explicit version pinning.
| steps: | ||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| - name: Install pnpm | ||
| uses: pnpm/action-setup@41ff72655975bd51cab0327fa583b6e92b6d3061 # v4.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent / suppressed archive creation failures (tar)
The tar command in the workflow suppresses stderr and provides a fallback, which can hide failures and result in empty or corrupted artifacts being uploaded. This is a critical reliability issue for the integration workflow — if artifacts are missing or invalid, downstream test steps will fail unpredictably or run against stale data.
🔧 Suggested fix
Replace the single-line tar invocation with a robust script that: (1) uses strict shell flags, (2) checks which paths exist before archiving, and (3) exits non-zero with a clear error if nothing to archive. Example:
- name: Create build archive
run: |
set -euo pipefail
trap 'echo "ERROR: archive creation failed at line $LINENO" >&2' ERR
FILES=()
for p in libs/*/dist libs/*/.turbo .turbo; do
[ -e "$p" ] && FILES+=("$p")
done
if [ ${#FILES[@]} -eq 0 ]; then
echo "No build artifacts found" >&2
exit 1
fi
tar -czf build-artifacts.tar.gz "${FILES[@]}"This ensures failures are visible and the artifact is valid before upload.
🤖 Prompt for AI Agents
In .github/workflows/integration-tests.yml at line ~22, replace the existing tar command with the robust multi-line script above and remove any stderr suppressions (e.g., 2>/dev/null or || fallbacks).
everywheredennise-oss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Review Findings
🔴 Critical Issues
- Silent / suppressed artifact creation failures in
.github/workflows/integration-tests.yml(around line 22)
- Problem: The tar invocation hides stderr and falls back (
2>/dev/null || ...), which can mask failures and produce empty or corrupted artifacts. If an invalid/missing artifact is uploaded, downstream test steps may run against stale/missing data or fail unpredictably. - Suggested fix: Replace the single-line tar with a robust multi-line script that uses
set -euo pipefail, checks for existing artifact paths, and exits non-zero with a clear error if nothing to archive. See inline suggestion for exact snippet.
🟠 Major Issues
- Overly-permissive workflow permission
actions: writein.github/workflows/integration-tests.yml(around line 6)
- Problem: Granting
actions: writeat workflow-level is broader than typically required and increases risk for scheduled workflows that run with repository secrets. - Suggested fix: Limit permissions to least-privilege (e.g.,
actions: reador removeactionspermission entirely) and scope any necessary escalation to specific jobs only.
- Pinned
actions/setup-nodecommit SHA may not support thenode-version-fileinput (around line 11)
- Problem: Pinning to a commit SHA that predates support for
node-version-filecan cause the step to ignore the input or fail silently. - Suggested fix: Use a maintained release tag that supports the desired input (e.g.,
actions/setup-node@v4) or explicitly providenode-version: '24'.
📈 Summary
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 2 |
| 🔵 Minor | 0 |
Verdict: REQUEST_CHANGES - The tar/artifact handling issue is critical and must be fixed; the workflow permissions and setup-node pinning should be addressed before merging.
christian-bromann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's verify how much our pipeline actually cost before moving forward. Running these e2e tests is very valuable to guarantee we don't regress.
so we dont go bankrupt