Skip to content

Conversation

@hntrl
Copy link
Member

@hntrl hntrl commented Feb 6, 2026

so we dont go bankrupt

Copy link
Member

@christian-bromann christian-bromann left a 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.

@everywheredennise-oss
Copy link

🤖 Devin AI is starting automated code review...

@everywheredennise-oss
Copy link

📋 Walkthrough

This PR adds a scheduled integration-tests GitHub Actions workflow and removes the integration job from the main CI workflow.

◆ Sequence Diagram

sequenceDiagram
  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)
Loading

📊 Changes

File(s) Summary
.github/workflows/ci.yml Modified: removed/disabled inline integration test job to keep main CI faster
.github/workflows/integration-tests.yml Added: new scheduled workflow (cron + workflow_dispatch) that builds and runs integration tests

name: Integration Tests

permissions:
contents: read

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Major

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: pnpm

Pinning 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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).

Copy link

@everywheredennise-oss everywheredennise-oss left a 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

  1. 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

  1. Overly-permissive workflow permission actions: write in .github/workflows/integration-tests.yml (around line 6)
  • Problem: Granting actions: write at 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: read or remove actions permission entirely) and scope any necessary escalation to specific jobs only.
  1. Pinned actions/setup-node commit SHA may not support the node-version-file input (around line 11)
  • Problem: Pinning to a commit SHA that predates support for node-version-file can 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 provide node-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.

Copy link
Member

@christian-bromann christian-bromann left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants