diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 63eba144..a83e6f25 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -20,64 +20,63 @@ on: - windows-latest - all -jobs: - test-single: - name: Integration Tests (${{ matrix.os }}) - if: inputs.os != 'all' - runs-on: ${{ inputs.os }} - env: - GROQ_API_KEY: ${{ secrets.GROQ_API_KEY }} +# Least privilege by default; no job in this workflow writes to the repository. +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +env: + # actions/checkout runs `git init` before any repository config exists, which + # prints a hint about the default branch name on every job. Setting it via + # GIT_CONFIG_* silences the hint without an extra step. + GIT_CONFIG_COUNT: '1' + GIT_CONFIG_KEY_0: init.defaultBranch + GIT_CONFIG_VALUE_0: main +jobs: + # Turn the `os` input into a matrix so the test job exists exactly once. + # Previously the same job body was duplicated for the single-OS and the + # all-OS case, and both were named "Integration Tests (${{ matrix.os }})" - + # which rendered as "Integration Tests ()" for the job without a matrix. + matrix: + name: Resolve OS Matrix + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + os: ${{ steps.resolve.outputs.os }} steps: - - name: Checkout repository - uses: actions/checkout@v6 - with: - submodules: false - - - name: Setup Bun - uses: oven-sh/setup-bun@v2 - with: - bun-version: latest - - - name: Install dependencies - run: bun install - working-directory: js - - - name: Run integration tests + - name: Resolve OS list + id: resolve + env: + OS_INPUT: ${{ inputs.os }} run: | - if [ -n "${{ inputs.test_pattern }}" ]; then - bun test "${{ inputs.test_pattern }}" + if [ "$OS_INPUT" = "all" ]; then + echo 'os=["ubuntu-latest","macos-latest","windows-latest"]' >> "$GITHUB_OUTPUT" else - bun test ./tests/integration/*.js + printf 'os=["%s"]\n' "$OS_INPUT" >> "$GITHUB_OUTPUT" fi - working-directory: js - - name: Test MCP CLI commands - run: | - # Test help command - bun run src/index.js mcp --help - - # Test mcp add command for Playwright - bun run src/index.js mcp add playwright npx @playwright/mcp@latest - - # Verify configuration was created - cat ~/.config/opencode/opencode.json || echo "Config not found" - - # Test mcp list command - bun run src/index.js mcp list - working-directory: js - - test-all: + test: name: Integration Tests (${{ matrix.os }}) - if: inputs.os == 'all' + needs: [matrix] runs-on: ${{ matrix.os }} + timeout-minutes: 30 strategy: fail-fast: false matrix: - os: [ubuntu-latest, macos-latest, windows-latest] + os: ${{ fromJSON(needs.matrix.outputs.os) }} env: GROQ_API_KEY: ${{ secrets.GROQ_API_KEY }} + defaults: + run: + # windows-latest defaults to pwsh, where the POSIX test syntax below + # is a syntax error. + shell: bash + steps: - name: Checkout repository uses: actions/checkout@v6 @@ -94,11 +93,16 @@ jobs: working-directory: js - name: Run integration tests + # TEST_PATTERN is passed through env instead of being interpolated into + # the script: a `${{ inputs.* }}` expression is substituted before the + # shell runs, so a value with shell metacharacters would be executed. + env: + TEST_PATTERN: ${{ inputs.test_pattern }} run: | - if [ -n "${{ inputs.test_pattern }}" ]; then - bun test "${{ inputs.test_pattern }}" + if [ -n "$TEST_PATTERN" ]; then + bun test --timeout 30000 "$TEST_PATTERN" else - bun test ./tests/integration/*.js + bun test --timeout 30000 ./tests/integration/*.js fi working-directory: js @@ -110,8 +114,10 @@ jobs: # Test mcp add command for Playwright bun run src/index.js mcp add playwright npx @playwright/mcp@latest - # Verify configuration was created - cat ~/.config/opencode/opencode.json || echo "Config not found" + # Verify the configuration was actually created. `|| echo "not found"` + # used to swallow the failure here, so a broken `mcp add` still + # reported a green step. + cat ~/.config/opencode/opencode.json # Test mcp list command bun run src/index.js mcp list diff --git a/.github/workflows/js.yml b/.github/workflows/js.yml index 44729cb9..8f7eefb9 100644 --- a/.github/workflows/js.yml +++ b/.github/workflows/js.yml @@ -37,15 +37,30 @@ on: required: false type: string +# Least privilege by default; jobs that need to write escalate individually. +permissions: + contents: read + concurrency: group: js-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # A release runs on push to main. Cancelling it mid-publish would leave a + # version bumped and tagged but not published, so main runs always finish. + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + +env: + # actions/checkout runs `git init` before any repository config exists, which + # prints a hint about the default branch name on every job. Setting it via + # GIT_CONFIG_* silences the hint without an extra step. + GIT_CONFIG_COUNT: '1' + GIT_CONFIG_KEY_0: init.defaultBranch + GIT_CONFIG_VALUE_0: main jobs: # Changeset check - only runs on PRs changeset-check: name: Check for Changesets runs-on: ubuntu-latest + timeout-minutes: 10 if: github.event_name == 'pull_request' steps: - uses: actions/checkout@v6 @@ -62,9 +77,13 @@ jobs: working-directory: js - name: Check for changesets + # HEAD_REF is a branch name chosen by the PR author. Interpolating it + # into the script would let a branch name execute shell code. + env: + HEAD_REF: ${{ github.head_ref }} run: | # Skip changeset check for automated version PRs - if [[ "${{ github.head_ref }}" == "changeset-release/"* ]]; then + if [[ "$HEAD_REF" == "changeset-release/"* ]]; then echo "Skipping changeset check for automated release PR" exit 0 fi @@ -76,10 +95,23 @@ jobs: lint: name: Lint and Format Check runs-on: ubuntu-latest + timeout-minutes: 10 needs: [changeset-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') }} steps: - uses: actions/checkout@v6 + with: + # Needed to merge the base branch below. + fetch-depth: 0 + + # refs/pull/N/merge is built when the pull request is synchronized, so + # commits landing on the base branch afterwards are not checked. Merging + # them here makes the checks below validate the real merge result. + - name: Simulate fresh merge with base branch (PR only) + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: bash scripts/simulate-fresh-merge.sh - name: Setup Node.js uses: actions/setup-node@v6 @@ -90,6 +122,9 @@ jobs: run: npm install --legacy-peer-deps working-directory: js + - name: Scan for committed secrets + run: npx --yes -p secretlint -p @secretlint/secretlint-rule-preset-recommend secretlint "**/*" + - name: Run ESLint run: npm run lint working-directory: js @@ -108,8 +143,9 @@ jobs: test: name: Unit Tests (Bun on ${{ matrix.os }}) runs-on: ${{ matrix.os }} + timeout-minutes: 20 needs: [changeset-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') }} strategy: fail-fast: false matrix: @@ -133,8 +169,11 @@ jobs: run: bun install working-directory: js + # Use the package script so CI and local runs execute the same suite. + # A hardcoded four-file list silently skipped 48 of the 52 unit test + # files, making CI green while regressions went unnoticed (issue #287). - name: Run unit tests - run: bun test ./tests/json-standard-unit.js ./tests/process-name.js ./tests/cli.ts ./tests/cli_options.ts + run: bun run test working-directory: js - name: Commit cached API responses @@ -166,8 +205,9 @@ jobs: verbose-integration: name: Verbose HTTP Logging Test runs-on: ubuntu-latest + timeout-minutes: 15 needs: [changeset-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') }} steps: - uses: actions/checkout@v6 with: @@ -191,8 +231,9 @@ jobs: package-install: name: Clean Package Install runs-on: ubuntu-latest + timeout-minutes: 15 needs: [changeset-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changeset-check.result == 'success') }} steps: - uses: actions/checkout@v6 @@ -227,8 +268,9 @@ jobs: needs: [lint, test, verbose-integration, package-install] # Use always() to ensure this job runs even if changeset-check was skipped # This is needed because lint/test jobs have a transitive dependency on changeset-check - if: always() && github.ref == 'refs/heads/main' && github.event_name == 'push' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.verbose-integration.result == 'success' && needs.package-install.result == 'success' + if: ${{ !cancelled() && github.ref == 'refs/heads/main' && github.event_name == 'push' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.verbose-integration.result == 'success' && needs.package-install.result == 'success' }} runs-on: ubuntu-latest + timeout-minutes: 30 concurrency: group: release-main cancel-in-progress: false @@ -293,6 +335,7 @@ jobs: name: Instant Release if: github.event_name == 'workflow_dispatch' && github.event.inputs.release_mode == 'instant' runs-on: ubuntu-latest + timeout-minutes: 30 concurrency: group: release-main cancel-in-progress: false @@ -321,7 +364,10 @@ jobs: - name: Version packages and commit to main id: version - run: node scripts/version-and-commit.mjs --mode instant --bump-type "${{ github.event.inputs.bump_type }}" --description "${{ github.event.inputs.description }}" + env: + BUMP_TYPE: ${{ github.event.inputs.bump_type }} + DESCRIPTION: ${{ github.event.inputs.description }} + run: node scripts/version-and-commit.mjs --mode instant - name: Publish to npm # Run if version was committed OR if a previous attempt already committed (for re-runs) @@ -346,6 +392,7 @@ jobs: name: Create Changeset PR if: github.event_name == 'workflow_dispatch' && github.event.inputs.release_mode == 'changeset-pr' runs-on: ubuntu-latest + timeout-minutes: 10 permissions: contents: write pull-requests: write @@ -364,7 +411,10 @@ jobs: working-directory: js - name: Create changeset file - run: node scripts/create-manual-changeset.mjs --bump-type "${{ github.event.inputs.bump_type }}" --description "${{ github.event.inputs.description }}" + env: + BUMP_TYPE: ${{ github.event.inputs.bump_type }} + DESCRIPTION: ${{ github.event.inputs.description }} + run: node scripts/create-manual-changeset.mjs - name: Format changeset with Prettier run: | @@ -374,7 +424,7 @@ jobs: echo "Formatted changeset files" - name: Create Pull Request - uses: peter-evans/create-pull-request@v7 + uses: peter-evans/create-pull-request@v8 with: token: ${{ secrets.GITHUB_TOKEN }} commit-message: 'chore: add changeset for manual ${{ github.event.inputs.bump_type }} release' diff --git a/.github/workflows/model-tests.yml b/.github/workflows/model-tests.yml index 23aaf851..180c4da2 100644 --- a/.github/workflows/model-tests.yml +++ b/.github/workflows/model-tests.yml @@ -65,9 +65,27 @@ on: - without_tools - both +# Least privilege by default; this workflow only reads the repository. +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +env: + # actions/checkout runs `git init` before any repository config exists, which + # prints a hint about the default branch name on every job. Setting it via + # GIT_CONFIG_* silences the hint without an extra step. + GIT_CONFIG_COUNT: '1' + GIT_CONFIG_KEY_0: init.defaultBranch + GIT_CONFIG_VALUE_0: main + jobs: test-model: + name: Test Model runs-on: ubuntu-latest + timeout-minutes: 30 env: GROQ_API_KEY: ${{ secrets.GROQ_API_KEY }} @@ -88,10 +106,14 @@ jobs: - name: Determine model to test id: model + # custom_model is a free-form string. Interpolating `${{ inputs.* }}` + # into the script substitutes it before the shell runs, so a value + # containing shell metacharacters would be executed as code. Passing it + # through env keeps it as data. + env: + CUSTOM_MODEL: ${{ inputs.custom_model }} + MODEL_FROM_DROPDOWN: ${{ inputs.model }} run: | - CUSTOM_MODEL="${{ inputs.custom_model }}" - MODEL_FROM_DROPDOWN="${{ inputs.model }}" - if [ -n "$CUSTOM_MODEL" ]; then MODEL_ID="$CUSTOM_MODEL" else @@ -99,24 +121,39 @@ jobs: fi # Extract provider and model name from provider/model format - PROVIDER=$(echo "$MODEL_ID" | cut -d'/' -f1) - MODEL_NAME=$(echo "$MODEL_ID" | cut -d'/' -f2-) + PROVIDER=${MODEL_ID%%/*} + MODEL_NAME=${MODEL_ID#*/} + + if [ "$PROVIDER" = "$MODEL_ID" ] || [ -z "$MODEL_NAME" ]; then + echo "::error::Model must be in provider/model format, got: $MODEL_ID" + exit 1 + fi - echo "model_id=$MODEL_ID" >> $GITHUB_OUTPUT - echo "model_name=$MODEL_NAME" >> $GITHUB_OUTPUT - echo "provider=$PROVIDER" >> $GITHUB_OUTPUT + { + echo "model_id=$MODEL_ID" + echo "model_name=$MODEL_NAME" + echo "provider=$PROVIDER" + } >> "$GITHUB_OUTPUT" echo "Testing model: $MODEL_ID (provider: $PROVIDER, model: $MODEL_NAME)" - name: Fetch model capabilities id: capabilities + env: + PROVIDER: ${{ steps.model.outputs.provider }} + MODEL_NAME: ${{ steps.model.outputs.model_name }} run: | - OUTPUT=$(node scripts/get-model-info.mjs "${{ steps.model.outputs.provider }}" "${{ steps.model.outputs.model_name }}") + OUTPUT=$(node scripts/get-model-info.mjs "$PROVIDER" "$MODEL_NAME") echo "$OUTPUT" - echo "$OUTPUT" >> $GITHUB_OUTPUT + # Only well-formed key=value lines may reach $GITHUB_OUTPUT. Any other + # line makes the runner fail the step with "Invalid format", and a + # line containing `=` in an unexpected place could inject an output. + echo "$OUTPUT" | grep -E '^[A-Za-z_][A-Za-z0-9_]*=[^[:cntrl:]]*$' >> "$GITHUB_OUTPUT" || true - name: Check API key availability + env: + PROVIDER: ${{ steps.model.outputs.provider }} run: | - if [ "${{ steps.model.outputs.provider }}" = "groq" ]; then + if [ "$PROVIDER" = "groq" ]; then if [ -z "$GROQ_API_KEY" ]; then echo "::warning::GROQ_API_KEY is not set. Groq tests may fail." else @@ -126,20 +163,33 @@ jobs: - name: Test without tools (simple response) if: inputs.test_type == 'without_tools' || inputs.test_type == 'both' - run: node scripts/test-model-simple.mjs "${{ steps.model.outputs.model_id }}" + env: + MODEL_ID: ${{ steps.model.outputs.model_id }} + run: node scripts/test-model-simple.mjs "$MODEL_ID" - name: Test with tools (tool calling) if: inputs.test_type == 'with_tools' || inputs.test_type == 'both' - run: node scripts/test-model-tools.mjs "${{ steps.model.outputs.model_id }}" + env: + MODEL_ID: ${{ steps.model.outputs.model_id }} + run: node scripts/test-model-tools.mjs "$MODEL_ID" - name: Summary + # The summary is most useful when a test failed, so it must not be + # skipped by the failure of a previous step. + if: ${{ !cancelled() }} + env: + MODEL_ID: ${{ steps.model.outputs.model_id }} + PROVIDER: ${{ steps.model.outputs.provider }} + TEST_TYPE: ${{ inputs.test_type }} run: | - echo "## Model Test Summary" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "- **Model**: ${{ steps.model.outputs.model_id }}" >> $GITHUB_STEP_SUMMARY - echo "- **Provider**: ${{ steps.model.outputs.provider }}" >> $GITHUB_STEP_SUMMARY - echo "- **Test Type**: ${{ inputs.test_type }}" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY + { + echo "## Model Test Summary" + echo "" + echo "- **Model**: $MODEL_ID" + echo "- **Provider**: $PROVIDER" + echo "- **Test Type**: $TEST_TYPE" + echo "" + } >> "$GITHUB_STEP_SUMMARY" if [ -f test-output-simple.log ]; then echo "### Simple Test Output" >> $GITHUB_STEP_SUMMARY diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index ad529ae2..c53fc111 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -29,12 +29,24 @@ on: required: false type: string +# Least privilege by default; jobs that need to write escalate individually. +permissions: + contents: read + concurrency: group: rust-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # A release runs on push to main. Cancelling it mid-publish would leave a + # version bumped and tagged but not published, so main runs always finish. + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} env: CARGO_TERM_COLOR: always + # actions/checkout runs `git init` before any repository config exists, which + # prints a hint about the default branch name on every job. Setting it via + # GIT_CONFIG_* silences the hint without an extra step. + GIT_CONFIG_COUNT: '1' + GIT_CONFIG_KEY_0: init.defaultBranch + GIT_CONFIG_VALUE_0: main # Note: -Dwarnings is disabled while Rust implementation is WIP # Enable later with: RUSTFLAGS: -Dwarnings @@ -43,6 +55,7 @@ jobs: changelog-check: name: Changelog Fragment Check runs-on: ubuntu-latest + timeout-minutes: 10 if: github.event_name == 'pull_request' steps: - uses: actions/checkout@v6 @@ -77,10 +90,23 @@ jobs: lint: name: Lint and Format Check runs-on: ubuntu-latest + timeout-minutes: 15 needs: [changelog-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changelog-check.result == 'success' || needs.changelog-check.result == 'skipped') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changelog-check.result == 'success' || needs.changelog-check.result == 'skipped') }} steps: - uses: actions/checkout@v6 + with: + # Needed to merge the base branch below. + fetch-depth: 0 + + # refs/pull/N/merge is built when the pull request is synchronized, so + # commits landing on the base branch afterwards are not checked. Merging + # them here makes the checks below validate the real merge result. + - name: Simulate fresh merge with base branch (PR only) + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: bash scripts/simulate-fresh-merge.sh - name: Setup Rust uses: dtolnay/rust-toolchain@stable @@ -93,7 +119,7 @@ jobs: node-version: '24.x' - name: Cache cargo registry - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: | ~/.cargo/registry @@ -118,8 +144,9 @@ jobs: test: name: Test (${{ matrix.os }}) runs-on: ${{ matrix.os }} + timeout-minutes: 30 needs: [changelog-check] - if: always() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changelog-check.result == 'success' || needs.changelog-check.result == 'skipped') + if: ${{ !cancelled() && (github.event_name == 'push' || github.event_name == 'workflow_dispatch' || needs.changelog-check.result == 'success' || needs.changelog-check.result == 'skipped') }} strategy: fail-fast: false matrix: @@ -132,7 +159,7 @@ jobs: uses: dtolnay/rust-toolchain@stable - name: Cache cargo registry - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: | ~/.cargo/registry @@ -151,8 +178,9 @@ jobs: build: name: Build Package runs-on: ubuntu-latest + timeout-minutes: 20 needs: [lint, test] - if: always() && needs.lint.result == 'success' && needs.test.result == 'success' + if: ${{ !cancelled() && needs.lint.result == 'success' && needs.test.result == 'success' }} steps: - uses: actions/checkout@v6 @@ -160,7 +188,7 @@ jobs: uses: dtolnay/rust-toolchain@stable - name: Cache cargo registry - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: | ~/.cargo/registry @@ -184,8 +212,9 @@ jobs: needs: [lint, test, build] # Use always() to ensure this job runs even when changelog-check was skipped (on push events) # Without always(), GitHub Actions skips jobs when any transitive dependency was skipped - if: always() && github.event_name == 'push' && github.ref == 'refs/heads/main' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.build.result == 'success' + if: ${{ !cancelled() && github.event_name == 'push' && github.ref == 'refs/heads/main' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.build.result == 'success' }} runs-on: ubuntu-latest + timeout-minutes: 30 concurrency: group: release-main cancel-in-progress: false @@ -210,12 +239,20 @@ jobs: git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" + # Read crate name/version from the [package] section only. A plain + # `grep -Po '(?<=^name = ")'` also matches every [lib]/[[bin]]/[[test]] + # section, which produced a malformed crates.io URL and failed this job + # with curl exit code 3 (issue #287). + - name: Read crate metadata + id: crate + run: node scripts/rust-package-info.mjs + - name: Recover missing GitHub release for current version id: recover env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - CURRENT_VERSION=$(grep -Po '(?<=^version = ")[^"]*' rust/Cargo.toml) + CURRENT_VERSION="${{ steps.crate.outputs.version }}" TAG_EXISTS=false RELEASE_EXISTS=false @@ -253,11 +290,17 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - CURRENT_VERSION=$(grep -Po '(?<=^version = ")[^"]*' rust/Cargo.toml) - - # Check if version is published on crates.io (source of truth for publish status) - CRATE_NAME=$(grep -Po '(?<=^name = ")[^"]*' rust/Cargo.toml) - HTTP_STATUS=$(curl -s -o /dev/null -w "%{http_code}" "https://crates.io/api/v1/crates/$CRATE_NAME/$CURRENT_VERSION") + CURRENT_VERSION="${{ steps.crate.outputs.version }}" + CRATE_NAME="${{ steps.crate.outputs.name }}" + + # Check if version is published on crates.io (source of truth for publish status). + # A network hiccup must not fail the job, so curl retries and its exit + # status is tolerated; an unreachable registry simply reads as "not published". + HTTP_STATUS=$(curl -sS -o /dev/null -w "%{http_code}" \ + --retry 3 --retry-delay 2 --retry-all-errors --max-time 30 \ + -A "link-assistant-agent-ci" \ + "https://crates.io/api/v1/crates/$CRATE_NAME/$CURRENT_VERSION" || echo "000") + echo "crates.io HTTP status: $HTTP_STATUS" CRATES_PUBLISHED=false if [ "$HTTP_STATUS" = "200" ]; then CRATES_PUBLISHED=true @@ -302,12 +345,11 @@ jobs: node scripts/rust-version-and-commit.mjs \ --bump-type "${{ steps.bump_type.outputs.bump_type }}" + # Re-read after the bump step so the released version is the bumped one. - name: Get current version id: current_version if: steps.check.outputs.should_release == 'true' - run: | - CURRENT_VERSION=$(grep -Po '(?<=^version = ")[^"]*' rust/Cargo.toml) - echo "version=$CURRENT_VERSION" >> $GITHUB_OUTPUT + run: node scripts/rust-package-info.mjs - name: Build release if: steps.check.outputs.should_release == 'true' @@ -342,8 +384,9 @@ jobs: name: Manual Release needs: [lint, test, build] # Use always() to ensure this job runs even when changelog-check was skipped - if: always() && github.event_name == 'workflow_dispatch' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.build.result == 'success' + if: ${{ !cancelled() && github.event_name == 'workflow_dispatch' && needs.lint.result == 'success' && needs.test.result == 'success' && needs.build.result == 'success' }} runs-on: ubuntu-latest + timeout-minutes: 30 concurrency: group: release-main cancel-in-progress: false @@ -381,10 +424,10 @@ jobs: - name: Version and commit id: version - run: | - node scripts/rust-version-and-commit.mjs \ - --bump-type "${{ github.event.inputs.bump_type }}" \ - --description "${{ github.event.inputs.description }}" + env: + BUMP_TYPE: ${{ github.event.inputs.bump_type }} + DESCRIPTION: ${{ github.event.inputs.description }} + run: node scripts/rust-version-and-commit.mjs - name: Build release if: steps.version.outputs.version_committed == 'true' || steps.version.outputs.already_released == 'true' diff --git a/.gitignore b/.gitignore index 8a6648e3..9f4f7983 100644 --- a/.gitignore +++ b/.gitignore @@ -144,3 +144,4 @@ tmp/ # API cache data data/api-cache/ js/data/api-cache/ +ci-logs/ diff --git a/.gitkeep b/.gitkeep index 2cfca54c..d33988c7 100644 --- a/.gitkeep +++ b/.gitkeep @@ -1 +1,2 @@ -# .gitkeep file auto-generated at 2026-07-30T18:32:11.164Z for PR creation at branch issue-285-531ca23a5284 for issue https://github.com/link-assistant/agent/issues/285 \ No newline at end of file +# .gitkeep file auto-generated at 2026-07-30T18:32:11.164Z for PR creation at branch issue-285-531ca23a5284 for issue https://github.com/link-assistant/agent/issues/285 +# Updated: 2026-07-31T18:24:47.217Z \ No newline at end of file diff --git a/.husky/pre-commit b/.husky/pre-commit index 28dc8b35..acb26ed2 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -1 +1,12 @@ -cd js && npx lint-staged +#!/bin/sh +# Lint and format staged files under js/. +cd js && npx lint-staged || exit 1 +cd .. + +# lint-staged is configured (and can only resolve node_modules) inside js/, so +# the shared CI helper scripts in scripts/ are checked separately here. Without +# this the hook passed while `npm run check` in CI failed on the same commit. +if git diff --cached --name-only --diff-filter=ACM | grep -q '^scripts/.*\.mjs$'; then + npm --prefix js run --silent lint:scripts || exit 1 + ./js/node_modules/.bin/prettier --check scripts || exit 1 +fi diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 00000000..16eed96d --- /dev/null +++ b/.prettierignore @@ -0,0 +1,7 @@ +node_modules +coverage +dist +*.min.js +package-lock.json +bun.lock +.eslintcache diff --git a/js/.prettierrc b/.prettierrc similarity index 100% rename from js/.prettierrc rename to .prettierrc diff --git a/.secretlintrc.json b/.secretlintrc.json new file mode 100644 index 00000000..7a1a5df3 --- /dev/null +++ b/.secretlintrc.json @@ -0,0 +1,7 @@ +{ + "rules": [ + { + "id": "@secretlint/secretlint-rule-preset-recommend" + } + ] +} diff --git a/experiments/unref-sleep.test.js b/experiments/unref-sleep.test.js new file mode 100644 index 00000000..9ec840b7 --- /dev/null +++ b/experiments/unref-sleep.test.js @@ -0,0 +1,3 @@ +import { test, expect } from 'bun:test'; +function sleep(ms){return new Promise(r=>{const t=setTimeout(r,ms); if(t.unref) t.unref();});} +test('unref sleep resolves', async () => { const s=Date.now(); await sleep(2000); expect(Date.now()-s).toBeGreaterThan(1500); }); diff --git a/js/.changeset/ci-cd-audit-287.md b/js/.changeset/ci-cd-audit-287.md new file mode 100644 index 00000000..c52edf7f --- /dev/null +++ b/js/.changeset/ci-cd-audit-287.md @@ -0,0 +1,18 @@ +--- +'@link-assistant/agent': patch +--- + +Fix false positives, false negatives, warnings and errors in CI/CD (#287): + +- Release gating: the verbose HTTP logging integration test no longer asserts a provider-side HTTP 200, so a rate limit (429) or provider outage no longer marks the repository broken and blocks a release. +- npm publish verification no longer races the registry, and `js/package.json` packaging metadata is corrected. +- Test-coverage reporting counted only 4 of 52 test files. +- Lint and format checks now cover the shared `scripts/` helpers, and the pre-commit hook checks the same files CI does. +- All workflows declare a least-privilege top-level `permissions:` block, a `concurrency:` group, and per-job `timeout-minutes`. +- Job conditions use `!cancelled()` instead of `always()`, so cancelling a run stops dependent jobs. +- `workflow_dispatch` inputs and `github.head_ref` are passed through `env:` instead of being interpolated into shell scripts. +- `bun test` runs with a per-test timeout so a hung test reports before the job timeout. +- The rate limit wait timer is no longer unref'd, so a retry cannot be dropped while the wait is the only pending work. +- Third-party actions are pinned to the major versions used by the pipeline templates (`actions/cache@v5`, `peter-evans/create-pull-request@v8`), and a policy test rejects floating refs. + +Adds `js/tests/workflow-policy.js` and `js/tests/verbose-http-log.js` to keep these regressions from returning. diff --git a/js/LICENSE b/js/LICENSE new file mode 100644 index 00000000..fdddb29a --- /dev/null +++ b/js/LICENSE @@ -0,0 +1,24 @@ +This is free and unencumbered software released into the public domain. + +Anyone is free to copy, modify, publish, use, compile, sell, or +distribute this software, either in source code form or as a compiled +binary, for any purpose, commercial or non-commercial, and by any +means. + +In jurisdictions that recognize copyright laws, the author or authors +of this software dedicate any and all copyright interest in the +software to the public domain. We make this dedication for the benefit +of the public at large and to the detriment of our heirs and +successors. We intend this dedication to be an overt act of +relinquishment in perpetuity of all present and future rights to this +software under copyright law. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. +IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR +OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +OTHER DEALINGS IN THE SOFTWARE. + +For more information, please refer to diff --git a/js/package.json b/js/package.json index 249d44ce..6f2fdf9f 100644 --- a/js/package.json +++ b/js/package.json @@ -5,16 +5,17 @@ "main": "src/index.js", "type": "module", "bin": { - "agent": "./src/index.js" + "agent": "src/index.js" }, "scripts": { "dev": "bun run src/index.js", - "test": "bun test ./tests/*.js ./tests/*.ts", + "test": "bun test --timeout 30000 ./tests/*.js ./tests/*.ts", "test:integration": "bun test ./tests/integration/basic.js", - "lint": "eslint .", - "lint:fix": "eslint . --fix", - "format": "prettier --write .", - "format:check": "prettier --check .", + "lint": "eslint . && npm run lint:scripts", + "lint:scripts": "cd ../scripts && eslint . --config ./eslint.config.mjs", + "lint:fix": "eslint . --fix && cd ../scripts && eslint . --config ./eslint.config.mjs --fix", + "format": "prettier --write . ../scripts", + "format:check": "prettier --check . ../scripts", "check:file-size": "node ../scripts/check-file-size.mjs", "check": "npm run lint && npm run format:check && npm run check:file-size", "prepare": "cd .. && husky || true", @@ -28,7 +29,7 @@ }, "repository": { "type": "git", - "url": "https://github.com/link-assistant/agent" + "url": "git+https://github.com/link-assistant/agent.git" }, "keywords": [ "ai", @@ -46,10 +47,7 @@ "src/", "package.json", "README.md", - "../MODELS.md", - "../TOOLS.md", - "../EXAMPLES.md", - "../LICENSE" + "LICENSE" ], "dependencies": { "@actions/core": "^1.11.1", diff --git a/js/src/provider/retry-fetch.ts b/js/src/provider/retry-fetch.ts index f87ae0e3..9d4b473b 100644 --- a/js/src/provider/retry-fetch.ts +++ b/js/src/provider/retry-fetch.ts @@ -183,9 +183,16 @@ export namespace RetryFetch { return; } + // The timer is deliberately NOT unref'd. Every caller awaits this + // promise, so an unref'd timer is the only pending work while a retry + // wait is in flight: the runtime is then free to stop waiting on it, + // which drops the retry instead of performing it. On Bun for Windows + // this showed up as `tests/retry-fetch.ts > retries on 429 and succeeds + // on second attempt` never resuming, burning the whole job timeout. + // See https://github.com/link-assistant/agent/issues/287. + // The abort path below clears the timer, so a cancelled wait still lets + // the process exit immediately (the concern behind #213). const timeout = setTimeout(resolve, ms); - // Prevent sleep timer from keeping event loop alive (#213) - if (timeout.unref) timeout.unref(); if (signal) { const abortHandler = () => { diff --git a/js/tests/ci-scripts.js b/js/tests/ci-scripts.js new file mode 100644 index 00000000..21837126 --- /dev/null +++ b/js/tests/ci-scripts.js @@ -0,0 +1,434 @@ +/** + * Regression tests for the CI/CD helper scripts in ../../scripts. + * + * These cover the two production CI failures fixed in issue #287: + * 1. Rust "Auto Release" exited with code 3 because the crate name was read + * with a plain grep that also matched [lib]/[[bin]]/[[test]] sections. + * 2. The npm publish job reported "Failed to publish after 3 attempts" even + * though attempt 1 had published successfully — verification ran before + * the registry had propagated, and the miss triggered a republish that + * then hit EPUBLISHCONFLICT. + */ + +import { describe, expect, test } from 'bun:test'; + +import { + formatNpmPackageVersion, + parsePackageInfo, +} from '../../scripts/package-info.mjs'; +import { + buildPackageMetadataUrl, + encodePackageName, + isPackageVersionPublished, + normalizeRegistryUrl, +} from '../../scripts/npm-registry.mjs'; +import { + classifyCargoPublish, + isNonRetryableCargoFailure, +} from '../../scripts/cargo-publish-result.mjs'; +import { + buildCrateVersionUrl, + isCrateVersionPublished, +} from '../../scripts/crates-registry.mjs'; +import { isNonRetryableFailure } from '../../scripts/publish-failure-classifier.mjs'; +import { + isAlreadyPublishedError, + publishWithRetry, + waitForVersionOnRegistry, +} from '../../scripts/publish-retry.mjs'; +import { + parseCrateInfo, + readPackageKey, + setPackageVersion, +} from '../../scripts/rust-package-info.mjs'; + +const noSleep = () => Promise.resolve(); +const noLog = () => {}; + +// A Cargo.toml shaped like rust/Cargo.toml: many sections carry `name = "..."`. +const CARGO_TOML = `[package] +name = "link-assistant-agent" +version = "0.9.2" +edition = "2021" + +[lib] +name = "agent_lib" +path = "src/lib.rs" + +[[bin]] +name = "agent" +path = "src/main.rs" + +[[test]] +name = "cli_options" +path = "tests/cli_options.rs" + +[[test]] +name = "version" +path = "tests/version.rs" +`; + +describe('rust-package-info', () => { + test('reads name and version from the [package] section only', () => { + expect(parseCrateInfo(CARGO_TOML)).toEqual({ + name: 'link-assistant-agent', + version: '0.9.2', + }); + }); + + test('does not leak names from [lib]/[[bin]]/[[test]] sections', () => { + const name = readPackageKey(CARGO_TOML, 'name'); + expect(name).toBe('link-assistant-agent'); + expect(name).not.toContain('\n'); + expect(name).not.toContain('agent_lib'); + }); + + test('ignores keys defined before any section header', () => { + expect( + readPackageKey('name = "stray"\n[package]\nname = "real"\n', 'name') + ).toBe('real'); + }); + + test('bumps only the [package] version', () => { + const withDependency = `[package] +name = "crate" +version = "0.9.2" + +[dependencies.serde] +version = "1.0.0" +`; + + const bumped = setPackageVersion(withDependency, '0.9.3'); + + expect(readPackageKey(bumped, 'version')).toBe('0.9.3'); + expect(bumped).toContain('version = "1.0.0"'); + }); + + test('refuses to rewrite a Cargo.toml without a package version', () => { + expect(() => + setPackageVersion('[dependencies]\nversion = "1.0.0"\n', '2.0.0') + ).toThrow(/\[package\]/); + }); + + test('throws when the package name or version is missing', () => { + expect(() => parseCrateInfo('[package]\nversion = "1.0.0"\n')).toThrow( + /name/ + ); + expect(() => parseCrateInfo('[package]\nname = "x"\n')).toThrow(/version/); + }); +}); + +describe('package-info', () => { + test('parses name and version', () => { + expect(parsePackageInfo('{"name":"@scope/pkg","version":"1.2.3"}')).toEqual( + { + name: '@scope/pkg', + version: '1.2.3', + } + ); + }); + + test('reports the file path on invalid JSON', () => { + expect(() => parsePackageInfo('{', 'js/package.json')).toThrow( + /js\/package.json/ + ); + }); + + test('formats a package@version specifier', () => { + expect(formatNpmPackageVersion('@scope/pkg', '1.2.3')).toBe( + '@scope/pkg@1.2.3' + ); + }); +}); + +describe('npm-registry', () => { + test('percent-encodes the slash in scoped package names', () => { + expect(encodePackageName('@link-assistant/agent')).toBe( + '@link-assistant%2Fagent' + ); + expect(encodePackageName('agent')).toBe('agent'); + }); + + test('strips trailing slashes from the registry URL', () => { + expect(normalizeRegistryUrl('https://registry.npmjs.org/')).toBe( + 'https://registry.npmjs.org' + ); + }); + + test('builds the metadata URL', () => { + expect( + buildPackageMetadataUrl( + '@link-assistant/agent', + 'https://registry.npmjs.org/' + ) + ).toBe('https://registry.npmjs.org/@link-assistant%2Fagent'); + }); + + test('returns true only when the exact version exists', async () => { + const fetchFn = async () => ({ + ok: true, + status: 200, + json: async () => ({ versions: { '1.0.0': {} } }), + }); + + expect(await isPackageVersionPublished('pkg', '1.0.0', { fetchFn })).toBe( + true + ); + expect(await isPackageVersionPublished('pkg', '1.0.1', { fetchFn })).toBe( + false + ); + }); + + test('treats a 404 as not published', async () => { + const fetchFn = async () => ({ ok: false, status: 404 }); + expect(await isPackageVersionPublished('pkg', '1.0.0', { fetchFn })).toBe( + false + ); + }); +}); + +describe('publish failure classification', () => { + test('recognises already-published conflicts', () => { + expect(isAlreadyPublishedError('npm error code EPUBLISHCONFLICT')).toBe( + true + ); + expect( + isAlreadyPublishedError( + 'You cannot publish over the previously published versions: 0.25.4.' + ) + ).toBe(true); + expect(isAlreadyPublishedError('npm error 401 Unauthorized')).toBe(false); + }); + + test('marks auth/registry errors as non-retryable', () => { + expect(isNonRetryableFailure('npm error code ENEEDAUTH')).toBe(true); + expect(isNonRetryableFailure('socket hang up')).toBe(false); + }); +}); + +describe('waitForVersionOnRegistry', () => { + test('polls until the version appears', async () => { + let calls = 0; + const verify = async () => ++calls >= 3; + + expect( + await waitForVersionOnRegistry({ verify, sleepFn: noSleep, log: noLog }) + ).toBe(true); + expect(calls).toBe(3); + }); + + test('gives up after the configured number of attempts', async () => { + let calls = 0; + const verify = async () => { + calls++; + return false; + }; + + expect( + await waitForVersionOnRegistry({ + verify, + attempts: 4, + sleepFn: noSleep, + log: noLog, + }) + ).toBe(false); + expect(calls).toBe(4); + }); +}); + +describe('publishWithRetry', () => { + test('does not republish when verification lags behind a successful publish', async () => { + let publishes = 0; + let verifies = 0; + + const { success } = await publishWithRetry({ + publish: async () => { + publishes++; + return { success: true, output: 'packages published successfully' }; + }, + // First verification misses (registry propagation lag), second succeeds. + verify: async () => ++verifies >= 2, + sleepFn: noSleep, + log: noLog, + }); + + expect(success).toBe(true); + expect(publishes).toBe(1); + }); + + test('treats an already-published conflict as a cue to verify, not to fail', async () => { + let publishes = 0; + + const { success } = await publishWithRetry({ + publish: async () => { + publishes++; + return { + success: false, + error: new Error('EPUBLISHCONFLICT'), + output: 'You cannot publish over the previously published versions', + }; + }, + verify: async () => true, + sleepFn: noSleep, + log: noLog, + }); + + expect(success).toBe(true); + expect(publishes).toBe(1); + }); + + test('retries a genuine publish failure and succeeds', async () => { + let publishes = 0; + + const { success } = await publishWithRetry({ + publish: async () => { + publishes++; + if (publishes < 2) { + return { + success: false, + error: new Error('socket hang up'), + output: '', + }; + } + return { success: true, output: '' }; + }, + verify: async () => true, + maxRetries: 3, + sleepFn: noSleep, + log: noLog, + }); + + expect(success).toBe(true); + expect(publishes).toBe(2); + }); + + test('fails fast on a non-retryable error', async () => { + let publishes = 0; + + const { success, error } = await publishWithRetry({ + publish: async () => { + publishes++; + const failure = new Error('npm error code ENEEDAUTH'); + failure.nonRetryable = true; + return { + success: false, + error: failure, + output: 'npm error code ENEEDAUTH', + }; + }, + verify: async () => false, + maxRetries: 3, + sleepFn: noSleep, + log: noLog, + }); + + expect(success).toBe(false); + expect(publishes).toBe(1); + expect(error.nonRetryable).toBe(true); + }); + + test('reports a terminal verification failure without republishing', async () => { + let publishes = 0; + + const { success, error } = await publishWithRetry({ + publish: async () => { + publishes++; + return { success: true, output: '' }; + }, + verify: async () => false, + maxRetries: 3, + sleepFn: noSleep, + log: noLog, + }); + + expect(success).toBe(false); + expect(publishes).toBe(1); + expect(error.verificationFailed).toBe(true); + expect(error.nonRetryable).toBe(true); + }); +}); + +describe('cargo publish classification', () => { + test('a zero exit code is success even when the log mentions errors', () => { + // `cargo publish --verbose` prints dependency diagnostics that contain + // "error: " and "error[E...]"; scanning for them used to turn successful + // publishes into retried failures. + const result = classifyCargoPublish({ + code: 0, + stdout: 'Compiling deps\nerror[E0382]: quoted in a doc example\n', + stderr: 'error: this string appears in a test name\n', + }); + + expect(result.success).toBe(true); + expect(result.error).toBeNull(); + }); + + test('an already-uploaded conflict is not a failure to retry', () => { + const result = classifyCargoPublish({ + code: 101, + stderr: 'error: crate version is already uploaded', + }); + + expect(result.success).toBe(false); + expect(isAlreadyPublishedError(result.output)).toBe(true); + expect(result.error.nonRetryable).toBeUndefined(); + }); + + test('marks auth failures as non-retryable', () => { + const result = classifyCargoPublish({ + code: 101, + stderr: 'error: failed to publish: 403 Forbidden', + }); + + expect(result.success).toBe(false); + expect(result.error.nonRetryable).toBe(true); + expect(isNonRetryableCargoFailure(result.output)).toBe(true); + }); + + test('retries an ordinary non-zero exit', () => { + const result = classifyCargoPublish({ + code: 101, + stderr: 'error: failed to get a 200 OK response, got 502', + }); + + expect(result.success).toBe(false); + expect(result.error.nonRetryable).toBeUndefined(); + }); +}); + +describe('crates-registry', () => { + test('builds the version metadata URL', () => { + expect(buildCrateVersionUrl('link-assistant-agent', '0.9.2')).toBe( + 'https://crates.io/api/v1/crates/link-assistant-agent/0.9.2' + ); + }); + + test('returns true only for an exact version match', async () => { + const fetchFn = async () => ({ + ok: true, + status: 200, + json: async () => ({ version: { num: '0.9.2' } }), + }); + + expect(await isCrateVersionPublished('crate', '0.9.2', { fetchFn })).toBe( + true + ); + expect(await isCrateVersionPublished('crate', '0.9.3', { fetchFn })).toBe( + false + ); + }); + + test('treats a 404 as not published', async () => { + const fetchFn = async () => ({ ok: false, status: 404 }); + expect(await isCrateVersionPublished('crate', '0.9.2', { fetchFn })).toBe( + false + ); + }); + + test('surfaces other registry errors instead of reporting "not published"', async () => { + const fetchFn = async () => ({ ok: false, status: 503 }); + await expect( + isCrateVersionPublished('crate', '0.9.2', { fetchFn }) + ).rejects.toThrow(/503/); + }); +}); diff --git a/js/tests/integration/verbose-hi.js b/js/tests/integration/verbose-hi.js index f7ab2a89..6234cf54 100644 --- a/js/tests/integration/verbose-hi.js +++ b/js/tests/integration/verbose-hi.js @@ -2,6 +2,8 @@ import { test, expect, setDefaultTimeout } from 'bun:test'; // @ts-ignore import { sh } from 'command-stream'; +import { inspectVerboseHttpLog } from '../lib/verbose-http-log.js'; + // Increase default timeout to 120 seconds — real API calls may take longer setDefaultTimeout(120000); @@ -14,7 +16,14 @@ setDefaultTimeout(120000); * This test uses a real API with free-tier limits. It is the ONLY real-API test * intended for CI/CD execution. Other integration tests are manual (workflow_dispatch). * + * What is asserted is the logging contract only. The HTTP status the provider + * returned is reported but not asserted: this test gates the release in js.yml, + * and asserting `status == 200` turned a provider rate limit into a red build. + * The parsing rules live in tests/lib/verbose-http-log.js and are unit tested + * in tests/verbose-http-log.js. + * * @see https://github.com/link-assistant/agent/issues/221 + * @see https://github.com/link-assistant/agent/issues/287 */ test('Agent-cli --verbose mode logs HTTP requests and responses for "hi"', async () => { @@ -34,95 +43,33 @@ test('Agent-cli --verbose mode logs HTTP requests and responses for "hi"', async console.log('\n=== Verbose test: stdout length:', stdout.length); console.log('=== Verbose test: stderr length:', stderr.length); - // --- 1. Verify the agent produced output (non-empty stdout) --- + // The agent must have produced output at all. expect(stdout.length).toBeGreaterThan(0); - // --- 2. Verify HTTP request logs are present --- - // The verbose wrapper logs "HTTP request" with method, URL, headers, body - const hasHttpRequest = - combined.includes('"message": "HTTP request"') || - combined.includes('"message":"HTTP request"'); - expect(hasHttpRequest).toBe(true); - - // --- 3. Verify HTTP response logs are present --- - const hasHttpResponse = - combined.includes('"message": "HTTP response"') || - combined.includes('"message":"HTTP response"'); - expect(hasHttpResponse).toBe(true); - - // --- 4. Verify verbose HTTP logging active diagnostic breadcrumb --- - const hasVerboseActive = - combined.includes('verbose HTTP logging active') || - combined.includes('[verbose] HTTP logging active'); - expect(hasVerboseActive).toBe(true); - - // --- 5. Verify request details are logged (URL, method) --- - // Should contain an API endpoint URL (https://...) - expect(combined.includes('https://')).toBe(true); - - // Should contain HTTP method (POST for LLM API calls) - expect( - combined.includes('"method": "POST"') || - combined.includes('"method":"POST"') - ).toBe(true); + const { checks, failures, status } = inspectVerboseHttpLog(combined); - // --- 6. Verify response status is logged --- - expect( - combined.includes('"status": 200') || combined.includes('"status":200') - ).toBe(true); - - // --- 7. Verify response body or stream is logged --- - const hasResponseBody = - combined.includes('"message": "HTTP response body"') || - combined.includes('"message":"HTTP response body"') || - combined.includes('"message": "HTTP response body (stream)"') || - combined.includes('"message":"HTTP response body (stream)"'); - expect(hasResponseBody).toBe(true); - - // --- 8. Verify headers are logged (with sensitive values masked) --- - expect(combined.includes('"headers"')).toBe(true); - - // Sensitive headers should NOT contain full API keys - // (They should be masked like "sk-a...5678" or "[REDACTED]") - const apiKeyPatterns = [ - /["']?(?:x-api-key|authorization|api-key)["']?\s*:\s*["'][a-zA-Z0-9_-]{20,}["']/i, - ]; - for (const pattern of apiKeyPatterns) { - const match = combined.match(pattern); - if (match) { - const value = match[0]; - const isMasked = value.includes('...') || value.includes('[REDACTED]'); - expect(isMasked).toBe(true); - } + for (const [name, passed] of Object.entries(checks)) { + console.log(` - ${name}: ${passed ? '✓' : '✗'}`); } + console.log(` - provider HTTP status: ${status ?? 'none logged'}`); - // --- 9. Verify request body is logged (should contain bodyPreview) --- - expect(combined.includes('"bodyPreview"')).toBe(true); + if (status !== null && status !== 200) { + // Informational: the provider is having a bad day, the logging still works. + console.log( + `⚠ Provider responded with ${status}; verbose logging is still verified.` + ); + } - // --- 10. Verify duration is logged --- - expect(combined.includes('"durationMs"')).toBe(true); + expect(failures).toEqual([]); - // --- 11. Check if the AI responded (non-blocking) --- - // The agent should produce step_start/step_finish/text events when the model works. - // However, the default model may be temporarily unavailable or produce API errors, - // so this check is informational only — the test's purpose is verifying HTTP logging. + // Informational: the model may be temporarily unavailable, which does not + // affect what this test verifies. const hasStepStart = combined.includes('"type": "step_start"') || combined.includes('"type":"step_start"') || combined.includes('"type": "step-start"') || combined.includes('"type":"step-start"'); - - console.log('\n✅ Verbose HTTP logging verification passed'); - console.log(' - HTTP request logged: ✓'); - console.log(' - HTTP response logged: ✓'); - console.log(' - Verbose diagnostic breadcrumb: ✓'); - console.log(' - Request URL and method: ✓'); - console.log(' - Response status code: ✓'); - console.log(' - Response body/stream: ✓'); - console.log(' - Headers (sanitized): ✓'); - console.log(' - Body preview: ✓'); - console.log(' - Duration timing: ✓'); console.log( - ` - Agent step events: ${hasStepStart ? '✓' : '⚠ (model may be temporarily unavailable)'}` + ` - agent step events: ${hasStepStart ? '✓' : '⚠ (model may be temporarily unavailable)'}` ); }); diff --git a/js/tests/lib/verbose-http-log.js b/js/tests/lib/verbose-http-log.js new file mode 100644 index 00000000..57644b6a --- /dev/null +++ b/js/tests/lib/verbose-http-log.js @@ -0,0 +1,95 @@ +/** + * Assertions over the verbose HTTP log produced by `agent --verbose`. + * + * Extracted from tests/integration/verbose-hi.js so the parsing rules can be + * unit tested against recorded logs without calling a real API. + * + * The point of the integration test is that verbose mode logs the HTTP + * exchange. Whether the upstream provider answered 200, 429 or 503 is a + * property of the provider, not of this repository, so the status code is + * reported but never asserted to be 200 - doing so turned a provider rate + * limit into a red CI run that blocked the release. + * See https://github.com/link-assistant/agent/issues/287. + */ + +const REQUEST_MARKERS = [ + '"message": "HTTP request"', + '"message":"HTTP request"', +]; +const RESPONSE_MARKERS = [ + '"message": "HTTP response"', + '"message":"HTTP response"', +]; +const VERBOSE_MARKERS = [ + 'verbose HTTP logging active', + '[verbose] HTTP logging active', +]; +const BODY_MARKERS = [ + '"message": "HTTP response body"', + '"message":"HTTP response body"', + '"message": "HTTP response body (stream)"', + '"message":"HTTP response body (stream)"', +]; +const METHOD_MARKERS = ['"method": "POST"', '"method":"POST"']; + +const API_KEY_PATTERN = + /["']?(?:x-api-key|authorization|api-key)["']?\s*:\s*["'][a-zA-Z0-9_-]{20,}["']/i; + +function includesAny(log, markers) { + return markers.some((marker) => log.includes(marker)); +} + +/** + * Read the HTTP status code out of the verbose log. + * @param {string} log + * @returns {number|null} The status, or null when none was logged + */ +export function readLoggedStatus(log) { + const match = String(log).match(/"status"\s*:\s*(\d{3})/); + return match ? Number(match[1]) : null; +} + +/** + * Check whether a sensitive header value made it into the log unmasked. + * @param {string} log + * @returns {boolean} + */ +export function hasUnmaskedApiKey(log) { + const match = String(log).match(API_KEY_PATTERN); + if (!match) { + return false; + } + const value = match[0]; + return !value.includes('...') && !value.includes('[REDACTED]'); +} + +/** + * Evaluate the verbose logging contract over a captured log. + * @param {string} log - Combined stdout and stderr of the agent run + * @returns {{checks: Record, failures: string[], status: number|null}} + */ +export function inspectVerboseHttpLog(log) { + const text = String(log ?? ''); + + const checks = { + 'HTTP request logged': includesAny(text, REQUEST_MARKERS), + 'HTTP response logged': includesAny(text, RESPONSE_MARKERS), + 'verbose diagnostic breadcrumb': includesAny(text, VERBOSE_MARKERS), + 'request URL logged': text.includes('https://'), + 'request method logged': includesAny(text, METHOD_MARKERS), + 'response status logged': readLoggedStatus(text) !== null, + 'response body or stream logged': includesAny(text, BODY_MARKERS), + 'headers logged': text.includes('"headers"'), + 'sensitive headers masked': !hasUnmaskedApiKey(text), + 'request body preview logged': text.includes('"bodyPreview"'), + 'request duration logged': text.includes('"durationMs"'), + }; + + return { + checks, + failures: Object.entries(checks) + .filter(([, passed]) => !passed) + .map(([name]) => name), + status: readLoggedStatus(text), + }; +} diff --git a/js/tests/retry-fetch-wait.js b/js/tests/retry-fetch-wait.js new file mode 100644 index 00000000..ec5f3ffe --- /dev/null +++ b/js/tests/retry-fetch-wait.js @@ -0,0 +1,60 @@ +import { test, expect } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Regression test for the retry wait timer. + * + * `sleep()` in src/provider/retry-fetch.ts used to unref its timer. While a + * rate limit wait is in flight that timer is the only pending work, so the + * runtime was free to stop waiting on it: the retry was dropped instead of + * performed. On Bun for Windows the process stopped making progress entirely + * and the unit test job burned its whole 20 minute timeout. + * + * This runs the wait in a fresh process — the failure mode only exists when + * nothing else keeps the event loop alive, which is never true inside the + * shared test runner. Bun on Linux happens to keep servicing an unref'd timer, + * so this test only turns red on a runtime that does not; it is here to catch + * that difference on every platform the matrix covers. + * + * @see https://github.com/link-assistant/agent/issues/287 + */ + +const jsRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); + +const program = ` +import { RetryFetch } from './src/provider/retry-fetch'; +import { config } from './src/config/config'; + +config.retryTimeout = 3600; +config.minRetryInterval = 0; + +let calls = 0; +const baseFetch = () => { + calls++; + return Promise.resolve( + calls === 1 + ? new Response('rate limited', { status: 429 }) + : new Response('ok', { status: 200 }) + ); +}; + +const response = await RetryFetch.create({ baseFetch })('https://example.com'); +console.log(JSON.stringify({ calls, status: response.status })); +`; + +test('a rate limit wait is not dropped when nothing else keeps the loop alive', () => { + const result = spawnSync('bun', ['-e', program], { + cwd: jsRoot, + encoding: 'utf8', + timeout: 60_000, + }); + + expect(result.error).toBeUndefined(); + expect([result.status, result.stderr]).toEqual([0, result.stderr]); + expect(JSON.parse(result.stdout.trim().split('\n').at(-1))).toEqual({ + calls: 2, + status: 200, + }); +}, 60_000); diff --git a/js/tests/simulate-fresh-merge.js b/js/tests/simulate-fresh-merge.js new file mode 100644 index 00000000..660b0daa --- /dev/null +++ b/js/tests/simulate-fresh-merge.js @@ -0,0 +1,120 @@ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Tests for scripts/simulate-fresh-merge.sh. + * + * The false negative it prevents: GitHub builds `refs/pull/N/merge` when the + * pull request is synchronized, so commits landing on main afterwards are not + * part of what CI checked. A pull request could be green while the merged + * result was broken. See https://github.com/link-assistant/agent/issues/287. + */ + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); +const script = join(repoRoot, 'scripts', 'simulate-fresh-merge.sh'); + +let workspace; + +function git(cwd, ...args) { + return execFileSync('git', args, { cwd, encoding: 'utf8' }); +} + +function commit(cwd, file, contents, message) { + writeFileSync(join(cwd, file), contents); + git(cwd, 'add', file); + git(cwd, 'commit', '-m', message); +} + +function runScript(cwd) { + return execFileSync('bash', [script], { + cwd, + encoding: 'utf8', + env: { ...process.env, BASE_REF: 'main' }, + }); +} + +beforeEach(() => { + workspace = mkdtempSync(join(tmpdir(), 'fresh-merge-')); + + // `origin` is a real repository on disk so `git fetch origin main` works. + const origin = join(workspace, 'origin'); + execFileSync('git', ['init', '-q', '-b', 'main', origin]); + git(origin, 'config', 'user.email', 'test@example.com'); + git(origin, 'config', 'user.name', 'Test'); + commit(origin, 'base.txt', 'one\n', 'base'); + + const clone = join(workspace, 'clone'); + execFileSync('git', ['clone', '-q', origin, clone]); + git(clone, 'config', 'user.email', 'test@example.com'); + git(clone, 'config', 'user.name', 'Test'); + git(clone, 'checkout', '-q', '-b', 'feature'); +}); + +afterEach(() => { + rmSync(workspace, { recursive: true, force: true }); +}); + +describe('simulate-fresh-merge.sh', () => { + test('does nothing when the checkout already contains the base branch', () => { + const clone = join(workspace, 'clone'); + + const output = runScript(clone); + + expect(output).toContain('no merge needed'); + expect(git(clone, 'log', '--oneline')).not.toContain('Merge'); + }); + + test('merges commits that landed on the base branch after the checkout', () => { + const origin = join(workspace, 'origin'); + const clone = join(workspace, 'clone'); + commit(clone, 'feature.txt', 'feature\n', 'feature work'); + commit(origin, 'later.txt', 'later\n', 'later base commit'); + + const output = runScript(clone); + + expect(output).toContain('Fresh merge simulation succeeded'); + // The check that runs after this script now sees both changes. + expect(git(clone, 'ls-files')).toContain('later.txt'); + expect(git(clone, 'ls-files')).toContain('feature.txt'); + }); + + test('fails with an actionable error on a merge conflict', () => { + const origin = join(workspace, 'origin'); + const clone = join(workspace, 'clone'); + commit(clone, 'base.txt', 'feature version\n', 'feature edit'); + commit(origin, 'base.txt', 'base version\n', 'base edit'); + + let error; + try { + runScript(clone); + } catch (caught) { + error = caught; + } + + expect(error).toBeDefined(); + expect(error.status).toBe(1); + expect(`${error.stdout}${error.stderr}`).toContain( + '::error::Merge conflict with main' + ); + }); + + test('refuses to run without BASE_REF', () => { + let error; + try { + execFileSync('bash', [script], { + cwd: join(workspace, 'clone'), + encoding: 'utf8', + env: { ...process.env, BASE_REF: '' }, + }); + } catch (caught) { + error = caught; + } + + expect(error).toBeDefined(); + expect(`${error.stdout}${error.stderr}`).toContain('BASE_REF is not set'); + }); +}); diff --git a/js/tests/storage-migration.ts b/js/tests/storage-migration.ts index 1fdf74ba..95bf8325 100644 --- a/js/tests/storage-migration.ts +++ b/js/tests/storage-migration.ts @@ -91,8 +91,11 @@ describe('storage migration path safety', () => { test('path.resolve does not introduce null bytes', () => { const dir = '/workspace/.local/share/link-assistant-agent/storage'; const project = path.resolve(dir, '../project'); + // Compared against path.resolve of the expected path rather than a literal: + // on Windows a rooted POSIX path resolves against the current drive and + // uses backslashes, so the literal only held on POSIX runners (#287). expect(project).toBe( - '/workspace/.local/share/link-assistant-agent/project' + path.resolve('/workspace/.local/share/link-assistant-agent/project') ); expect(project.includes('\0')).toBe(false); }); diff --git a/js/tests/verbose-http-log.js b/js/tests/verbose-http-log.js new file mode 100644 index 00000000..d774fe3c --- /dev/null +++ b/js/tests/verbose-http-log.js @@ -0,0 +1,86 @@ +import { describe, test, expect } from 'bun:test'; + +import { + hasUnmaskedApiKey, + inspectVerboseHttpLog, + readLoggedStatus, +} from './lib/verbose-http-log.js'; + +/** + * Unit tests for the verbose HTTP log contract used by + * tests/integration/verbose-hi.js, which is a release gate in js.yml. + * + * The regression they lock in: the integration test asserted `"status": 200`, + * so a provider-side rate limit (429) or outage (503) reported this repository + * as broken and blocked the release, even though verbose logging worked + * perfectly. See https://github.com/link-assistant/agent/issues/287. + */ + +function buildLog(status) { + return [ + '[verbose] HTTP logging active', + JSON.stringify({ + message: 'HTTP request', + method: 'POST', + url: 'https://api.example.com/v1/chat', + headers: { authorization: 'sk-a...5678' }, + bodyPreview: '{"messages":[{"role":"user","content":"hi"}]}', + }), + JSON.stringify({ message: 'HTTP response', status, durationMs: 412 }), + JSON.stringify({ message: 'HTTP response body (stream)', status }), + ].join('\n'); +} + +describe('readLoggedStatus', () => { + test('reads the status from spaced and compact JSON', () => { + expect(readLoggedStatus('"status": 200')).toBe(200); + expect(readLoggedStatus('"status":429')).toBe(429); + }); + + test('returns null when no status was logged', () => { + expect(readLoggedStatus('nothing here')).toBe(null); + }); +}); + +describe('hasUnmaskedApiKey', () => { + test('accepts masked and redacted values', () => { + expect(hasUnmaskedApiKey('"authorization": "sk-a...5678"')).toBe(false); + expect(hasUnmaskedApiKey('"x-api-key": "[REDACTED]"')).toBe(false); + }); + + test('flags a full key', () => { + expect( + hasUnmaskedApiKey('"authorization": "sk-abcdefghijklmnopqrstuvwxyz"') + ).toBe(true); + }); + + test('accepts a log without any sensitive header', () => { + expect(hasUnmaskedApiKey('"content-type": "application/json"')).toBe(false); + }); +}); + +describe('inspectVerboseHttpLog', () => { + test('passes every check for a successful exchange', () => { + const result = inspectVerboseHttpLog(buildLog(200)); + + expect(result.failures).toEqual([]); + expect(result.status).toBe(200); + }); + + test('still passes when the provider rate limits the request', () => { + // The regression this file exists for: a 429 must not fail the gate. + const result = inspectVerboseHttpLog(buildLog(429)); + + expect(result.failures).toEqual([]); + expect(result.status).toBe(429); + }); + + test('reports which parts of the logging contract are missing', () => { + const result = inspectVerboseHttpLog('completely silent run'); + + expect(result.status).toBe(null); + expect(result.failures).toContain('HTTP request logged'); + expect(result.failures).toContain('HTTP response logged'); + expect(result.checks['sensitive headers masked']).toBe(true); + }); +}); diff --git a/js/tests/workflow-policy.js b/js/tests/workflow-policy.js new file mode 100644 index 00000000..2fffd709 --- /dev/null +++ b/js/tests/workflow-policy.js @@ -0,0 +1,319 @@ +import { describe, test, expect } from 'bun:test'; +import { readdirSync, readFileSync } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Policy checks for the GitHub Actions workflows. + * + * These are executable versions of the rules in + * https://github.com/link-assistant/hive-mind/blob/main/docs/CI-CD-BEST-PRACTICES.md + * and docs/BEST-PRACTICES.md of the pipeline templates + * (link-foundation/js-ai-driven-development-pipeline-template, which enforces + * the same rules in tests/workflow-permissions.test.js and + * tests/ci-timeouts.test.js). + * + * Each rule below failed on at least one workflow of this repository when the + * test was written; see https://github.com/link-assistant/agent/issues/287. + */ + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); +const workflowDir = join(repoRoot, '.github', 'workflows'); + +const workflowFiles = readdirSync(workflowDir) + .filter((file) => /\.ya?ml$/.test(file)) + .sort(); + +function readWorkflow(file) { + return readFileSync(join(workflowDir, file), 'utf8').replaceAll('\r\n', '\n'); +} + +const workflows = workflowFiles.map((file) => ({ + file, + body: readWorkflow(file), +})); + +/** + * List the top-level job ids of a workflow. + * @param {string} body + * @returns {string[]} + */ +export function listJobs(body) { + const jobsStart = body.indexOf('\njobs:\n'); + if (jobsStart === -1) { + return []; + } + return Array.from( + body.slice(jobsStart).matchAll(/^ {2}([a-zA-Z0-9_-]+):\s*$/gm), + (match) => match[1] + ); +} + +/** + * Extract the YAML block belonging to a single job. + * @param {string} body + * @param {string} jobId + * @returns {string} + */ +export function getJobBlock(body, jobId) { + const lines = body.split('\n'); + const start = lines.indexOf(` ${jobId}:`); + if (start === -1) { + return ''; + } + const end = lines.findIndex( + (line, index) => index > start && /^ {2}[a-zA-Z0-9_-]+:\s*$/.test(line) + ); + return lines.slice(start, end === -1 ? lines.length : end).join('\n'); +} + +/** + * Read the `permissions:` block declared at the workflow level. + * Without one, jobs inherit the repository default, which is often + * read/write-all for the whole GITHUB_TOKEN. + * @param {string} body + * @returns {string|undefined} + */ +export function getTopLevelPermissions(body) { + const lines = body.split('\n'); + const start = lines.indexOf('permissions:'); + if (start === -1) { + return undefined; + } + const end = lines.findIndex( + (line, index) => index > start && line !== '' && !line.startsWith(' ') + ); + return lines + .slice(start + 1, end === -1 ? lines.length : end) + .filter((line) => line.trim() !== '') + .join('\n'); +} + +/** + * Collect every `run:` script body of a workflow, both the inline and the + * block form. + * @param {string} body + * @returns {string[]} + */ +export function listRunScripts(body) { + const lines = body.split('\n'); + const scripts = []; + + for (let index = 0; index < lines.length; index++) { + const match = + lines[index].match(/^(\s*)- ?run: ?(.*)$/) || + lines[index].match(/^(\s*)run: ?(.*)$/); + if (!match) { + continue; + } + + const indent = match[1].length; + const inline = match[2].trim(); + + if ( + inline !== '|' && + inline !== '>' && + inline !== '|-' && + inline !== '>-' + ) { + scripts.push(inline); + continue; + } + + const blockLines = []; + for (let next = index + 1; next < lines.length; next++) { + const line = lines[next]; + if (line.trim() !== '' && line.search(/\S/) <= indent) { + break; + } + blockLines.push(line); + } + scripts.push(blockLines.join('\n')); + } + + return scripts; +} + +describe('workflow token permissions', () => { + test('every workflow declares a top-level permissions block', () => { + const missing = workflows + .filter(({ body }) => getTopLevelPermissions(body) === undefined) + .map(({ file }) => file); + + expect(missing).toEqual([]); + }); + + test('the default is read-only repository contents', () => { + for (const { file, body } of workflows) { + expect([file, getTopLevelPermissions(body)]).toEqual([ + file, + ' contents: read', + ]); + } + }); +}); + +describe('CI timeout policy', () => { + test('every job declares timeout-minutes', () => { + const missing = []; + + for (const { file, body } of workflows) { + for (const jobId of listJobs(body)) { + if ( + !/^ {4}timeout-minutes:\s*\d+\s*$/m.test(getJobBlock(body, jobId)) + ) { + missing.push(`${file}:${jobId}`); + } + } + } + + expect(missing).toEqual([]); + }); + + test('every workflow defines at least one job', () => { + for (const { file, body } of workflows) { + expect([file, listJobs(body).length > 0]).toEqual([file, true]); + } + }); +}); + +describe('cancellation propagation', () => { + // always() still evaluates to true for a cancelled run, so dependent jobs + // keep running after a cancel. !cancelled() stops the chain. + // See hive-mind issue #1278. + test('job conditions use !cancelled() instead of always()', () => { + const offenders = []; + + for (const { file, body } of workflows) { + for (const jobId of listJobs(body)) { + const block = getJobBlock(body, jobId); + const condition = block.match(/^ {4}if:(.*)$/m)?.[1] ?? ''; + if (condition.includes('always()')) { + offenders.push(`${file}:${jobId}`); + } + } + } + + expect(offenders).toEqual([]); + }); +}); + +describe('workflow_dispatch input handling', () => { + // A `${{ inputs.x }}` expression is substituted into the shell script before + // it runs, so a value containing shell metacharacters is executed as code. + // Passing the value through env: keeps it as data. + test('run scripts never interpolate workflow inputs directly', () => { + const offenders = []; + + for (const { file, body } of workflows) { + for (const script of listRunScripts(body)) { + const match = script.match( + /\$\{\{\s*(?:inputs\.|github\.event\.inputs\.|github\.head_ref)[^}]*\}\}/ + ); + if (match) { + offenders.push(`${file}: ${match[0]}`); + } + } + } + + expect(offenders).toEqual([]); + }); +}); + +describe('per-test timeouts', () => { + // Without a per-test cap a hung test burns the whole job timeout before + // reporting anything. + test('the bun unit test script sets a global test timeout', () => { + const packageJson = JSON.parse( + readFileSync(join(repoRoot, 'js', 'package.json'), 'utf8') + ); + + expect(packageJson.scripts.test).toContain('--timeout '); + }); +}); + +describe('merge result validation', () => { + // GitHub builds refs/pull/N/merge when the pull request is synchronized, so + // commits pushed to the base branch afterwards are not part of what CI + // checked: a pull request can be green while its merge result is broken. + test('the lint job of each pipeline simulates a fresh merge', () => { + const missing = ['js.yml', 'rust.yml'].filter((file) => { + const body = workflows.find((workflow) => workflow.file === file)?.body; + return !getJobBlock(body ?? '', 'lint').includes( + 'scripts/simulate-fresh-merge.sh' + ); + }); + + expect(missing).toEqual([]); + }); +}); + +describe('secrets detection', () => { + test('the js pipeline scans the repository for committed secrets', () => { + const body = workflows.find((workflow) => workflow.file === 'js.yml').body; + + expect(getJobBlock(body, 'lint')).toContain('secretlint'); + }); +}); + +describe('checkout hygiene', () => { + // actions/checkout runs `git init` before any config exists, so every job + // printed "hint: Using 'master' as the name for the initial branch" into the + // log. Observed in run 30657021842. + test('every workflow silences the git default-branch hint', () => { + const missing = workflows + .filter( + ({ body }) => !body.includes('GIT_CONFIG_KEY_0: init.defaultBranch') + ) + .map(({ file }) => file); + + expect(missing).toEqual([]); + }); +}); + +describe('action pinning', () => { + // A floating ref (@main, @master, a branch name) means a third party can + // change what runs in this repository's CI without a commit here. + test('every external action is pinned to a version tag or a commit', () => { + const offenders = []; + + for (const { file, body } of workflows) { + for (const [, ref] of body.matchAll(/^\s*uses: (\S+)\s*$/gm)) { + if (ref.startsWith('./')) { + continue; // A local action lives in this repository. + } + const version = ref.split('@')[1]; + if ( + version === undefined || + /^(main|master|latest|develop)$/.test(version) + ) { + offenders.push(`${file}: ${ref}`); + } + } + } + + expect(offenders).toEqual([]); + }); +}); + +describe('concurrency control', () => { + test('every workflow declares a concurrency group', () => { + const missing = workflows + .filter(({ body }) => !/^concurrency:\s*$/m.test(body)) + .map(({ file }) => file); + + expect(missing).toEqual([]); + }); + + // A push to main starts a release. `cancel-in-progress: true` at the + // workflow level lets the next push cancel it mid-publish, leaving a version + // bumped and tagged but never published. Only non-main runs may be cancelled. + test('workflows triggered by push to main never cancel main runs', () => { + const offenders = workflows + .filter(({ body }) => /^ {6}- main\s*$/m.test(body)) + .filter(({ body }) => /^ {2}cancel-in-progress: true\s*$/m.test(body)) + .map(({ file }) => file); + + expect(offenders).toEqual([]); + }); +}); diff --git a/rust/changelog.d/20260731_ci_cd_audit_287.md b/rust/changelog.d/20260731_ci_cd_audit_287.md new file mode 100644 index 00000000..2cab8623 --- /dev/null +++ b/rust/changelog.d/20260731_ci_cd_audit_287.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Fixed + +- Fixed the Rust release workflow crate lookup, which exited with code 3 when the crate name grep found no match, and removed the false-positive classification of `cargo publish` failures. +- Hardened the Rust version bump, tag and push steps, and added least-privilege permissions, concurrency groups and per-job timeouts to `rust.yml`. diff --git a/scripts/cargo-publish-result.mjs b/scripts/cargo-publish-result.mjs new file mode 100644 index 00000000..3227637d --- /dev/null +++ b/scripts/cargo-publish-result.mjs @@ -0,0 +1,70 @@ +/** + * Classify the outcome of a `cargo publish` invocation. + * + * Why this is not a substring scan over the whole output: the previous + * implementation searched `cargo publish --verbose` output for the substrings + * `'error: '` and `'error[E'` *before* looking at the exit code, and did so even + * when cargo had exited 0. Verbose cargo output routinely contains those + * substrings (dependency build diagnostics, test names, doc examples), so a + * successful publish could be classified as a failure and retried — the exact + * false-positive class this repository's release job kept hitting. + * + * Unlike `changeset publish`, `cargo publish` reports failure through its exit + * code reliably, so the exit code is the primary signal and text matching is + * only used to recognise the "already uploaded" case and to mark + * authentication errors as non-retryable. + */ + +import { isAlreadyPublishedError } from './publish-retry.mjs'; + +/** + * Failures that will never be fixed by retrying the same command. + */ +const NON_RETRYABLE_PATTERNS = [ + '401 unauthorized', + '403 forbidden', + 'no token found', + 'the remote server responded with an error: unauthorized', + 'is not an owner of crate', + 'crate name has already been taken', +]; + +/** + * @param {string} output + * @returns {boolean} + */ +export function isNonRetryableCargoFailure(output) { + const lowerOutput = String(output || '').toLowerCase(); + return NON_RETRYABLE_PATTERNS.some((pattern) => + lowerOutput.includes(pattern) + ); +} + +/** + * Turn a raw cargo publish result into the shape publishWithRetry expects. + * @param {object} result + * @param {number} result.code - Process exit code + * @param {string} [result.stdout] + * @param {string} [result.stderr] + * @returns {{success: boolean, error: Error|null, output: string}} + */ +export function classifyCargoPublish({ code, stdout = '', stderr = '' }) { + const output = `${stdout}\n${stderr}`; + + if (code === 0) { + return { success: true, error: null, output }; + } + + // A non-zero exit caused by the version already being on the registry is not + // a failure: the caller only has to verify. + if (isAlreadyPublishedError(output)) { + const error = new Error('Crate version is already uploaded'); + return { success: false, error, output }; + } + + const error = new Error(`cargo publish exited with code ${code}`); + if (isNonRetryableCargoFailure(output)) { + error.nonRetryable = true; + } + return { success: false, error, output }; +} diff --git a/scripts/crates-registry.mjs b/scripts/crates-registry.mjs new file mode 100644 index 00000000..cfad62d8 --- /dev/null +++ b/scripts/crates-registry.mjs @@ -0,0 +1,72 @@ +/** + * Minimal crates.io registry client. + * + * Mirrors scripts/npm-registry.mjs so both release pipelines determine + * "is this exact version published?" the same way: a direct metadata request, + * with 404 meaning "not published" and network errors surfacing to the caller + * so they can be treated as "unknown" rather than "absent". + */ + +export const DEFAULT_CRATES_API_URL = 'https://crates.io/api/v1/crates'; + +// crates.io rejects requests without a descriptive User-Agent. +export const DEFAULT_USER_AGENT = 'link-assistant-agent-ci'; + +/** + * Strip trailing slashes from a registry base URL. + * @param {string} registryUrl + * @returns {string} + */ +export function normalizeCratesApiUrl(registryUrl = DEFAULT_CRATES_API_URL) { + return String(registryUrl).replace(/\/+$/, ''); +} + +/** + * Build the metadata URL for one crate version. + * @param {string} crateName + * @param {string} version + * @param {string} [registryUrl] + * @returns {string} + */ +export function buildCrateVersionUrl( + crateName, + version, + registryUrl = DEFAULT_CRATES_API_URL +) { + return `${normalizeCratesApiUrl(registryUrl)}/${encodeURIComponent( + crateName + )}/${encodeURIComponent(version)}`; +} + +/** + * Check whether an exact crate version is published on crates.io. + * @param {string} crateName + * @param {string} version + * @param {object} [options] + * @param {Function} [options.fetchFn] + * @param {string} [options.registryUrl] + * @returns {Promise} + */ +export async function isCrateVersionPublished( + crateName, + version, + { fetchFn = globalThis.fetch, registryUrl = DEFAULT_CRATES_API_URL } = {} +) { + const response = await fetchFn( + buildCrateVersionUrl(crateName, version, registryUrl), + { headers: { 'user-agent': DEFAULT_USER_AGENT } } + ); + + if (response.status === 404) { + return false; + } + + if (!response.ok) { + throw new Error( + `crates.io responded with ${response.status} for ${crateName}@${version}` + ); + } + + const metadata = await response.json(); + return metadata?.version?.num === version; +} diff --git a/scripts/eslint.config.mjs b/scripts/eslint.config.mjs new file mode 100644 index 00000000..7c89009a --- /dev/null +++ b/scripts/eslint.config.mjs @@ -0,0 +1 @@ +export { default } from '../js/eslint.config.js'; diff --git a/scripts/generate-rust-integration-tests.mjs b/scripts/generate-rust-integration-tests.mjs index 2dfcfc48..d76d8462 100644 --- a/scripts/generate-rust-integration-tests.mjs +++ b/scripts/generate-rust-integration-tests.mjs @@ -3,7 +3,7 @@ // so both languages have parallel test files with the same base names. // This script is idempotent: it skips files that already exist. -import { readdirSync, writeFileSync, existsSync, statSync } from 'node:fs'; +import { readdirSync, writeFileSync, existsSync } from 'node:fs'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -15,7 +15,7 @@ function jsToRustName(jsName) { // bash.tools.js -> integration_bash_tools.rs // plaintext.input.js -> integration_plaintext_input.rs const stem = jsName.replace(/\.js$/, ''); - const rustStem = stem.replace(/[.\-]/g, '_'); + const rustStem = stem.replace(/[.-]/g, '_'); return `integration_${rustStem}.rs`; } @@ -36,7 +36,7 @@ for (const jsFile of jsFiles) { } const stem = jsFile.replace(/\.js$/, ''); - const featureName = stem.replace(/\.tools$/, '').replace(/[.\-]/g, ' '); + const featureName = stem.replace(/\.tools$/, '').replace(/[.-]/g, ' '); const content = `//! Rust counterpart of \`js/tests/integration/${jsFile}\`. //! //! The JS suite covers the ${featureName} integration path against the @@ -75,4 +75,6 @@ fn agent_help_runs_cleanly() { console.log(`Created ${rustFile} for ${jsFile}`); } -console.log(`\nDone: ${created} created, ${skipped} skipped (already existed).`); +console.log( + `\nDone: ${created} created, ${skipped} skipped (already existed).` +); diff --git a/scripts/instant-version-bump.mjs b/scripts/instant-version-bump.mjs index 7673338e..fe47621a 100644 --- a/scripts/instant-version-bump.mjs +++ b/scripts/instant-version-bump.mjs @@ -48,7 +48,8 @@ const config = makeConfig({ .option('js-root', { type: 'string', default: getenv('JS_ROOT', ''), - describe: 'JavaScript package root directory (auto-detected if not specified)', + describe: + 'JavaScript package root directory (auto-detected if not specified)', }), }); diff --git a/scripts/js-paths.mjs b/scripts/js-paths.mjs index 810d56bd..802c4dcd 100644 --- a/scripts/js-paths.mjs +++ b/scripts/js-paths.mjs @@ -38,7 +38,9 @@ export function getJsRoot(options = {}) { // If explicitly configured, use that if (explicitRoot !== undefined) { if (verbose) { - console.log(`Using explicitly configured JavaScript root: ${explicitRoot}`); + console.log( + `Using explicitly configured JavaScript root: ${explicitRoot}` + ); } return explicitRoot; } @@ -69,13 +71,13 @@ export function getJsRoot(options = {}) { // No package.json found throw new Error( 'Could not find package.json in expected locations.\n' + - 'Searched in:\n' + - ' - ./package.json (single-language repository)\n' + - ' - ./js/package.json (multi-language repository)\n\n' + - 'To fix this, either:\n' + - ' 1. Run the script from the repository root\n' + - ' 2. Explicitly configure the JavaScript root using --js-root option\n' + - ' 3. Set the JS_ROOT environment variable' + 'Searched in:\n' + + ' - ./package.json (single-language repository)\n' + + ' - ./js/package.json (multi-language repository)\n\n' + + 'To fix this, either:\n' + + ' 1. Run the script from the repository root\n' + + ' 2. Explicitly configure the JavaScript root using --js-root option\n' + + ' 3. Set the JS_ROOT environment variable' ); } @@ -96,7 +98,9 @@ export function getPackageJsonPath(options = {}) { */ export function getPackageLockPath(options = {}) { const jsRoot = getJsRoot(options); - return jsRoot === '.' ? './package-lock.json' : join(jsRoot, 'package-lock.json'); + return jsRoot === '.' + ? './package-lock.json' + : join(jsRoot, 'package-lock.json'); } /** diff --git a/scripts/npm-registry.mjs b/scripts/npm-registry.mjs new file mode 100644 index 00000000..8e642cce --- /dev/null +++ b/scripts/npm-registry.mjs @@ -0,0 +1,97 @@ +export const DEFAULT_NPM_REGISTRY_URL = 'https://registry.npmjs.org'; + +function getNpmRegistryFromEnv() { + try { + return process.env.NPM_CONFIG_REGISTRY || ''; + } catch { + return ''; + } +} + +/** + * Normalize an npm registry URL so package metadata paths can be appended. + * @param {string} registryUrl + * @returns {string} + */ +export function normalizeRegistryUrl( + registryUrl = getNpmRegistryFromEnv() || DEFAULT_NPM_REGISTRY_URL +) { + return String(registryUrl || DEFAULT_NPM_REGISTRY_URL).replace(/\/+$/, ''); +} + +/** + * Encode a package name for npm registry metadata URLs. + * @param {string} packageName + * @returns {string} + */ +export function encodePackageName(packageName) { + if (typeof packageName !== 'string' || packageName.trim() === '') { + throw new Error('Package name is required'); + } + + if (packageName.startsWith('@')) { + const [scope, name] = packageName.split('/'); + if (!scope || !name) { + throw new Error(`Invalid scoped package name: ${packageName}`); + } + return `${scope}%2F${encodeURIComponent(name)}`; + } + + return encodeURIComponent(packageName); +} + +/** + * Build the npm registry package metadata URL. + * @param {string} packageName + * @param {string} registryUrl + * @returns {string} + */ +export function buildPackageMetadataUrl( + packageName, + registryUrl = getNpmRegistryFromEnv() || DEFAULT_NPM_REGISTRY_URL +) { + return `${normalizeRegistryUrl(registryUrl)}/${encodePackageName(packageName)}`; +} + +/** + * Check whether a package version exists in npm registry metadata. + * HTTP 404 means the package has not been published yet and is not an error. + * @param {string} packageName + * @param {string} version + * @param {object} options + * @param {Function} [options.fetchFn] + * @param {string} [options.registryUrl] + * @returns {Promise} + */ +export async function isPackageVersionPublished( + packageName, + version, + { + fetchFn = fetch, + registryUrl = getNpmRegistryFromEnv() || DEFAULT_NPM_REGISTRY_URL, + } = {} +) { + if (typeof version !== 'string' || version.trim() === '') { + throw new Error('Package version is required'); + } + + const metadataUrl = buildPackageMetadataUrl(packageName, registryUrl); + const response = await fetchFn(metadataUrl, { + headers: { + accept: 'application/json', + }, + }); + + if (response.status === 404) { + return false; + } + + if (!response.ok) { + throw new Error( + `Failed to fetch npm package metadata for ${packageName}: ${response.status} ${response.statusText}` + ); + } + + const metadata = await response.json(); + return Object.hasOwn(metadata?.versions || {}, version); +} diff --git a/scripts/package-info.mjs b/scripts/package-info.mjs new file mode 100644 index 00000000..3c431f89 --- /dev/null +++ b/scripts/package-info.mjs @@ -0,0 +1,60 @@ +import { readFileSync } from 'node:fs'; + +import { getPackageJsonPath } from './js-paths.mjs'; + +/** + * Parse package metadata from a package.json file body. + * @param {string} packageJsonContent + * @param {string} packageJsonPath + * @returns {{name: string, version: string}} + */ +export function parsePackageInfo( + packageJsonContent, + packageJsonPath = 'package.json' +) { + let packageJson; + try { + packageJson = JSON.parse(packageJsonContent); + } catch (error) { + throw new Error(`Could not parse ${packageJsonPath}: ${error.message}`); + } + + if (typeof packageJson.name !== 'string' || packageJson.name.trim() === '') { + throw new Error(`Package name is missing in ${packageJsonPath}`); + } + + if ( + typeof packageJson.version !== 'string' || + packageJson.version.trim() === '' + ) { + throw new Error(`Package version is missing in ${packageJsonPath}`); + } + + return { + name: packageJson.name, + version: packageJson.version, + }; +} + +/** + * Read package name and version from the detected JavaScript package root. + * @param {Object} options - Configuration options (passed to getPackageJsonPath) + * @returns {{name: string, version: string}} + */ +export function readPackageInfo(options = {}) { + const packageJsonPath = getPackageJsonPath(options); + return parsePackageInfo( + readFileSync(packageJsonPath, 'utf8'), + packageJsonPath + ); +} + +/** + * Format an npm package@version specifier. + * @param {string} packageName + * @param {string} version + * @returns {string} + */ +export function formatNpmPackageVersion(packageName, version) { + return `${packageName}@${version}`; +} diff --git a/scripts/publish-failure-classifier.mjs b/scripts/publish-failure-classifier.mjs new file mode 100644 index 00000000..cbd818a7 --- /dev/null +++ b/scripts/publish-failure-classifier.mjs @@ -0,0 +1,77 @@ +/** + * Classify npm publish failures and build actionable guidance. + * + * Some publish failures are permanent: retrying a 404/401/403 (or any auth / + * registry-configuration error) produces the same error every time and only + * delays a clear, actionable message. The most common case is the FIRST publish + * of a brand-new package via npm OIDC trusted publishing, which returns E404 + * because npm cannot bootstrap a new package with trusted publishing alone — a + * trusted publisher can only be configured for a package that already exists. + * + * Addresses issue: + * - link-foundation/js-ai-driven-development-pipeline-template#77 + */ + +// Failures caused by authentication / registry configuration. Retrying these is +// pointless and only hides the real cause behind a generic +// "Failed to publish after N attempts" message. +export const NON_RETRYABLE_PATTERNS = [ + 'npm error 404', + 'npm error 401', + 'npm error 403', + 'e404', + 'e401', + 'e403', + 'access token expired', + 'eneedauth', + 'you must be logged in', + 'unable to authenticate', +]; + +/** + * Determine whether a detected failure is caused by authentication / registry + * configuration (and therefore should not be retried). + * @param {string} output - Combined stdout and stderr (and/or error message) + * @returns {boolean} + */ +export function isNonRetryableFailure(output) { + const lowerOutput = String(output || '').toLowerCase(); + return NON_RETRYABLE_PATTERNS.some((pattern) => + lowerOutput.includes(pattern) + ); +} + +/** + * Build an actionable, human-readable explanation for an authentication / + * registry-configuration publish failure (most commonly an E404 on the very + * first publish of a brand-new package via OIDC trusted publishing). + * @param {string} packageName - The package that failed to publish + * @returns {string} + */ +export function buildAuthFailureGuidance(packageName) { + return [ + '', + '=== NPM PUBLISH AUTHENTICATION / REGISTRY FAILURE ===', + '', + `Failed to publish ${packageName}. This is an authentication or registry`, + 'configuration error, not a transient one, so it was not retried.', + '', + 'Most common cause: the FIRST publish of a brand-new package via npm OIDC', + 'trusted publishing returns "E404 Not Found - PUT". npm cannot bootstrap a', + 'new package with trusted publishing alone, because a trusted publisher can', + 'only be configured for a package that already exists on the registry.', + '', + 'SOLUTION (choose one):', + ' 1. Bootstrap the first release with a classic automation token:', + ' - Create a granular/automation token on npmjs.com with publish access.', + ' - Add it as the repository secret NPM_TOKEN.', + ' - The release workflow passes it as NODE_AUTH_TOKEN automatically, so', + ' the next run will publish the initial version.', + ' 2. After the package exists, configure OIDC trusted publishing on', + ' npmjs.com (Package settings -> Trusted publishing) so future releases', + ' need no token at all. The NPM_TOKEN secret then becomes optional.', + '', + 'See: https://docs.npmjs.com/trusted-publishers', + '', + ].join('\n'); +} diff --git a/scripts/publish-retry.mjs b/scripts/publish-retry.mjs new file mode 100644 index 00000000..284115bd --- /dev/null +++ b/scripts/publish-retry.mjs @@ -0,0 +1,212 @@ +/** + * Publish orchestration helpers that keep the two failure domains separate: + * + * - the publish command itself failing (retryable: run `changeset publish` again) + * - post-publish verification missing because the npm registry has not + * propagated yet (NOT retryable by republishing: the only correct response is + * to look again) + * + * A single verification check a couple of seconds after a successful publish + * samples a race: when it misses, republishing fails with "cannot publish over + * the previously published versions" and a successful release is reported as a + * failure. + */ + +export const DEFAULT_VERIFY_ATTEMPTS = 7; +export const DEFAULT_VERIFY_INITIAL_DELAY = 2000; +export const DEFAULT_VERIFY_MAX_DELAY = 30000; + +/** + * Default sleep implementation. + * @param {number} ms + * @returns {Promise} + */ +export function sleep(ms) { + return new Promise((resolve) => globalThis.setTimeout(resolve, ms)); +} + +/** + * Patterns that mean "this exact version is already on the registry". + * Such an error is a cue to verify, not to fail. + */ +const ALREADY_PUBLISHED_PATTERNS = [ + // npm / changesets + 'epublishconflict', + 'cannot publish over the previously published version', + 'cannot publish over previously published version', + 'you cannot publish over the previously published versions', + 'already published', + // cargo / crates.io + 'already exists on crates.io index', + 'crate already uploaded', + 'already exists on the registry', + 'crate version is already uploaded', +]; + +/** + * Check whether publish output indicates the version is already published. + * @param {string} output + * @param {string[]} [extraPatterns] - Additional lowercase substrings to match + * @returns {boolean} + */ +export function isAlreadyPublishedError(output, extraPatterns = []) { + const lowerOutput = String(output || '').toLowerCase(); + return [...ALREADY_PUBLISHED_PATTERNS, ...extraPatterns].some((pattern) => + lowerOutput.includes(pattern) + ); +} + +/** + * Poll the registry until the version becomes visible, using exponential + * backoff. Returns true as soon as the version is found. + * @param {object} options + * @param {Function} options.verify - async () => boolean + * @param {number} [options.attempts] + * @param {number} [options.initialDelay] + * @param {number} [options.maxDelay] + * @param {Function} [options.sleepFn] + * @param {Function} [options.log] + * @returns {Promise} + */ +export async function waitForVersionOnRegistry({ + verify, + attempts = DEFAULT_VERIFY_ATTEMPTS, + initialDelay = DEFAULT_VERIFY_INITIAL_DELAY, + maxDelay = DEFAULT_VERIFY_MAX_DELAY, + sleepFn = sleep, + log = () => {}, +}) { + let delay = initialDelay; + for (let attempt = 1; attempt <= attempts; attempt++) { + await sleepFn(delay); + let found = false; + try { + found = await verify(); + } catch (error) { + // A transient registry/network error is indistinguishable from a miss + // here, so polling continues and the release is not failed at this point. + log(`Verification attempt ${attempt} errored: ${error.message}`); + } + if (found) { + log(`Verification succeeded on attempt ${attempt}`); + return true; + } + log( + `Verification attempt ${attempt} of ${attempts}: version not visible yet` + ); + delay = Math.min(delay * 2, maxDelay); + } + return false; +} + +/** + * Decide whether a publish invocation should move on to verification. + * @param {object} outcome + * @param {boolean} outcome.success + * @param {Error} [outcome.error] + * @param {string} [outcome.output] + * @param {Function} outcome.log + * @returns {boolean} + */ +function shouldVerify({ success, error, output, log }) { + if (success) { + return true; + } + if (!isAlreadyPublishedError(output || error?.message || '')) { + return false; + } + log('Publish reported the version is already published, verifying registry.'); + return true; +} + +/** + * Build the result of the verification stage. A verification miss is terminal: + * the publish path must not be re-entered, because the package may already be + * live and republishing would fail with a conflict. + * @param {boolean} verified + * @returns {{success: boolean, error: Error|null}} + */ +function verificationOutcome(verified, registryLabel) { + if (verified) { + return { success: true, error: null }; + } + const error = new Error( + `Package not found on ${registryLabel} after publish; verification polling exhausted` + ); + error.nonRetryable = true; + error.verificationFailed = true; + return { success: false, error }; +} + +/** + * Run the publish command with retries, then verify with bounded polling. + * + * The publish command is retried only when the publish itself failed. Once a + * publish reports success (or reports an "already published" conflict), the + * flow moves to verification and never re-enters the publish path. + * + * Verification is still required: a publish that falsely claims success still + * fails the release. + * + * @param {object} options + * @param {Function} options.publish - async () => ({ success, error, output }) + * @param {Function} options.verify - async () => boolean + * @param {number} [options.maxRetries] + * @param {number} [options.retryDelay] + * @param {Function} [options.sleepFn] + * @param {Function} [options.log] + * @param {object} [options.verifyOptions] + * @returns {Promise<{success: boolean, error: Error|null, publishAttempts: number}>} + */ +export async function publishWithRetry({ + publish, + verify, + maxRetries = 3, + retryDelay = 10000, + sleepFn = sleep, + log = () => {}, + verifyOptions = {}, + registryLabel = 'npm', +}) { + let publishAttempts = 0; + let lastError = null; + + for (let attempt = 1; attempt <= maxRetries; attempt++) { + log(`Publish attempt ${attempt} of ${maxRetries}...`); + publishAttempts++; + const { success, error, output } = await publish(); + + if (shouldVerify({ success, error, output, log })) { + const verified = await waitForVersionOnRegistry({ + verify, + sleepFn, + log, + ...verifyOptions, + }); + return { + ...verificationOutcome(verified, registryLabel), + publishAttempts, + }; + } + + lastError = error; + + if (error?.nonRetryable) { + return { success: false, error, publishAttempts }; + } + + if (attempt < maxRetries) { + log( + `Publish failed: ${error?.message}, waiting ${retryDelay / 1000}s before retry...` + ); + await sleepFn(retryDelay); + } + } + + return { + success: false, + error: + lastError || new Error(`Failed to publish after ${maxRetries} attempts`), + publishAttempts, + }; +} diff --git a/scripts/publish-to-crates.mjs b/scripts/publish-to-crates.mjs index e7a51287..a78e6289 100644 --- a/scripts/publish-to-crates.mjs +++ b/scripts/publish-to-crates.mjs @@ -1,15 +1,18 @@ #!/usr/bin/env node /** - * Publish Rust crate to crates.io with verification + * Publish the Rust crate to crates.io with verification. * - * Usage: node scripts/publish-to-crates.mjs [--should-pull] + * Usage: node scripts/publish-to-crates.mjs [--should-pull] [--rust-root ] * - * Features: - * - Checks if version is already published before attempting - * - Publishes with retry logic - * - Verifies the crate actually appeared on crates.io after publishing - * - Outputs `published=true` and `published_version=X.Y.Z` for GitHub Actions + * Behaviour: + * - Reads crate name/version from the [package] section of Cargo.toml only + * (see scripts/rust-package-info.mjs for why a plain regex is not enough). + * - Skips publishing when the version is already on crates.io. + * - Retries only genuine publish failures. A successful publish, or an + * "already uploaded" conflict, moves straight to bounded verification + * polling and never re-enters the publish path. + * - Outputs `published=true` and `published_version=X.Y.Z` for GitHub Actions. * * Required environment variables (at least one must be set): * - CARGO_REGISTRY_TOKEN: API token for crates.io @@ -19,20 +22,24 @@ * - GITHUB_OUTPUT: GitHub Actions output file path */ -import { readFileSync, appendFileSync } from 'fs'; import { execSync } from 'child_process'; +import { appendFileSync } from 'fs'; -import { - getRustRoot, - getCargoTomlPath, - needsCd, - parseRustRootConfig, -} from './rust-paths.mjs'; +import { classifyCargoPublish } from './cargo-publish-result.mjs'; +import { isCrateVersionPublished } from './crates-registry.mjs'; +import { publishWithRetry, sleep } from './publish-retry.mjs'; +import { readCrateInfo } from './rust-package-info.mjs'; +import { getRustRoot, needsCd, parseRustRootConfig } from './rust-paths.mjs'; const MAX_RETRIES = 3; const RETRY_DELAY = 10000; // 10 seconds -const VERIFY_DELAY = 20000; // 20 seconds for crates.io propagation -const VERIFY_RETRIES = 5; // Number of verification attempts +// crates.io index propagation is slower than npm's, so verification starts +// later and is given more attempts. +const VERIFY_OPTIONS = { + attempts: 8, + initialDelay: 10000, + maxDelay: 30000, +}; const args = process.argv.slice(2); const getArg = (name, defaultValue) => { @@ -50,12 +57,6 @@ const rustRoot = getRustRoot({ verbose: true, }); -const CARGO_TOML = getCargoTomlPath({ rustRoot }); - -function sleep(ms) { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - function setOutput(key, value) { const outputFile = process.env.GITHUB_OUTPUT; if (outputFile) { @@ -64,139 +65,57 @@ function setOutput(key, value) { console.log(`Output: ${key}=${value}`); } -function exec(command, options = {}) { - const { capture = false, allowFailure = false } = options; - try { - const result = execSync(command, { - encoding: 'utf-8', - stdio: capture ? 'pipe' : 'inherit', - }); - return { code: 0, stdout: result || '', stderr: '' }; - } catch (error) { - if (allowFailure) { - return { - code: error.status || 1, - stdout: error.stdout || '', - stderr: error.stderr || '', - }; - } - throw error; - } -} - -function getPackageName() { - const cargoToml = readFileSync(CARGO_TOML, 'utf-8'); - const match = cargoToml.match(/^name\s*=\s*"([^"]+)"/m); - if (!match) { - throw new Error(`Could not parse package name from ${CARGO_TOML}`); - } - return match[1]; -} - -function getCurrentVersion() { - const cargoToml = readFileSync(CARGO_TOML, 'utf-8'); - const match = cargoToml.match(/^version\s*=\s*"([^"]+)"/m); - if (!match) { - throw new Error(`Could not parse version from ${CARGO_TOML}`); - } - return match[1]; -} - /** - * Check if a specific version of a crate is published on crates.io + * Run a command, returning its exit code and captured output instead of + * throwing, so the classifier decides what the outcome means. */ -async function checkCratesIo(packageName, version) { +function runCommand(command) { try { - const response = await fetch( - `https://crates.io/api/v1/crates/${packageName}/${version}` - ); - if (response.ok) { - const data = await response.json(); - return data.version && data.version.num === version; - } - return false; - } catch { - return false; + const stdout = execSync(command, { encoding: 'utf-8', stdio: 'pipe' }); + return { code: 0, stdout: stdout || '', stderr: '' }; + } catch (error) { + return { + code: error.status || 1, + stdout: error.stdout || '', + stderr: error.stderr || '', + }; } } /** - * Check if the crate exists at all on crates.io (any version) + * Look the version up on crates.io, treating a transient registry error as + * "unknown" rather than "not published". */ -async function checkCrateExists(packageName) { +async function verifyPublished(crateName, version) { try { - const response = await fetch( - `https://crates.io/api/v1/crates/${packageName}` - ); - if (response.ok) { - const data = await response.json(); - return { - exists: true, - owners: data.crate?.owners || [], - versions: (data.versions || []).map((v) => v.num), - }; - } - return { exists: false }; - } catch { - return { exists: false }; - } -} - -const ALREADY_EXISTS_PATTERNS = [ - 'already exists on crates.io index', - 'crate already uploaded', - 'already exists on the registry', -]; - -const FAILURE_PATTERNS = [ - 'error[E', - 'error: ', - '403 Forbidden', - '401 Unauthorized', - 'the remote server responded with an error', -]; - -function detectAlreadyExists(output) { - for (const pattern of ALREADY_EXISTS_PATTERNS) { - if (output.includes(pattern)) { - return true; - } + return await isCrateVersionPublished(crateName, version); + } catch (error) { + console.log(`crates.io lookup failed: ${error.message}`); + return false; } - return false; } -function detectPublishFailure(output) { - if (detectAlreadyExists(output)) { - return null; +function resolveCargoToken() { + if (!process.env.CARGO_REGISTRY_TOKEN && process.env.CARGO_TOKEN) { + console.log('CARGO_REGISTRY_TOKEN not set, using CARGO_TOKEN as fallback'); + process.env.CARGO_REGISTRY_TOKEN = process.env.CARGO_TOKEN; } - for (const pattern of FAILURE_PATTERNS) { - if (output.includes(pattern)) { - return pattern; - } - } - return null; + return Boolean(process.env.CARGO_REGISTRY_TOKEN); } async function main() { try { if (shouldPull) { console.log('Pulling latest changes...'); - exec('git pull origin main'); + execSync('git pull origin main', { stdio: 'inherit' }); } - const packageName = getPackageName(); - const currentVersion = getCurrentVersion(); - console.log( - `Publishing ${packageName}@${currentVersion} to crates.io...` - ); - - // Check if this version is already published - console.log( - `Checking if ${packageName}@${currentVersion} is already on crates.io...` - ); - const alreadyPublished = await checkCratesIo(packageName, currentVersion); + const { name: crateName, version: currentVersion } = readCrateInfo({ + rustRoot, + }); + console.log(`Publishing ${crateName}@${currentVersion} to crates.io...`); - if (alreadyPublished) { + if (await verifyPublished(crateName, currentVersion)) { console.log( `Version ${currentVersion} is already published on crates.io` ); @@ -206,149 +125,62 @@ async function main() { return; } - // Check if crate exists at all (to detect name conflicts early) - const crateInfo = await checkCrateExists(packageName); - if (crateInfo.exists) { - console.log( - `Crate ${packageName} exists on crates.io with versions: ${crateInfo.versions.join(', ')}` - ); - } else { - console.log( - `Crate ${packageName} does not exist on crates.io yet (first publish)` - ); - } - - // Resolve CARGO_REGISTRY_TOKEN with CARGO_TOKEN as fallback - if (!process.env.CARGO_REGISTRY_TOKEN && process.env.CARGO_TOKEN) { - console.log( - 'CARGO_REGISTRY_TOKEN not set, using CARGO_TOKEN as fallback' - ); - process.env.CARGO_REGISTRY_TOKEN = process.env.CARGO_TOKEN; - } - - if (!process.env.CARGO_REGISTRY_TOKEN) { + if (!resolveCargoToken()) { console.error( 'Error: Neither CARGO_REGISTRY_TOKEN nor CARGO_TOKEN environment variable is set' ); + setOutput('published', 'false'); process.exit(1); } - // Publish with retry logic const cargoPublishCmd = needsCd({ rustRoot }) - ? `cd ${rustRoot} && cargo publish --verbose --allow-dirty` - : 'cargo publish --verbose --allow-dirty'; - - for (let i = 1; i <= MAX_RETRIES; i++) { - console.log(`\nPublish attempt ${i} of ${MAX_RETRIES}...`); - - // Before each attempt, check crates.io API to see if a prior attempt succeeded - // (crates.io propagation can cause verification to fail even when publish succeeded) - if (i > 1) { - console.log('Checking crates.io API before retry...'); - const nowPublished = await checkCratesIo(packageName, currentVersion); - if (nowPublished) { - console.log( - `${packageName}@${currentVersion} is now confirmed on crates.io (prior attempt succeeded)` - ); - setOutput('published', 'true'); - setOutput('published_version', currentVersion); - setOutput('already_published', 'true'); - return; + ? `cd ${rustRoot} && cargo publish --allow-dirty` + : 'cargo publish --allow-dirty'; + + // Set when cargo itself accepted the upload; used to distinguish + // "the upload never happened" from "the index has not caught up yet". + let uploadAccepted = false; + + const { success, error } = await publishWithRetry({ + publish: () => { + const result = runCommand(cargoPublishCmd); + const classified = classifyCargoPublish(result); + uploadAccepted = uploadAccepted || classified.success; + if (classified.output.trim()) { + console.log('cargo publish output:'); + console.log(classified.output); } - } - - const result = exec(cargoPublishCmd, { - capture: true, - allowFailure: true, - }); - - const combinedOutput = `${result.stdout}\n${result.stderr}`; - - if (combinedOutput.trim()) { - console.log('cargo publish output:'); - console.log(combinedOutput); - } - - // "already exists" means the crate was published (possibly by a previous attempt) - if (detectAlreadyExists(combinedOutput)) { - console.log( - `Crate ${packageName}@${currentVersion} already exists on crates.io (treating as success)` - ); - setOutput('published', 'true'); - setOutput('published_version', currentVersion); - setOutput('already_published', 'true'); - return; - } - - // Check for real failure patterns in output - const failurePattern = detectPublishFailure(combinedOutput); - if (failurePattern) { - console.error(`Detected publish failure: "${failurePattern}"`); - if (i < MAX_RETRIES) { - console.log( - `Publish failed, waiting ${RETRY_DELAY / 1000}s before retry...` - ); - await sleep(RETRY_DELAY); - } - continue; - } - - // Check exit code for unexpected failures - if (result.code !== 0) { - console.error(`cargo publish exited with code ${result.code}`); - if (i < MAX_RETRIES) { - console.log( - `Publish failed, waiting ${RETRY_DELAY / 1000}s before retry...` - ); - await sleep(RETRY_DELAY); - } - continue; - } - - // Verify the crate is actually on crates.io (with retries for propagation delay) - let verified = false; - for (let v = 1; v <= VERIFY_RETRIES; v++) { - console.log( - `Waiting ${VERIFY_DELAY / 1000}s for crates.io propagation (verification ${v}/${VERIFY_RETRIES})...` - ); - await sleep(VERIFY_DELAY); - - console.log('Verifying crate was published to crates.io...'); - const isPublished = await checkCratesIo(packageName, currentVersion); - - if (isPublished) { - verified = true; - break; - } - - if (v < VERIFY_RETRIES) { - console.log( - `Not found yet, retrying verification...` - ); - } - } + return classified; + }, + verify: () => verifyPublished(crateName, currentVersion), + maxRetries: MAX_RETRIES, + retryDelay: RETRY_DELAY, + sleepFn: sleep, + log: (message) => console.log(message), + verifyOptions: VERIFY_OPTIONS, + registryLabel: 'crates.io', + }); - if (verified) { - setOutput('published', 'true'); - setOutput('published_version', currentVersion); - console.log( - `\u2705 Published ${packageName}@${currentVersion} to crates.io` - ); - return; - } + if (success) { + setOutput('published', 'true'); + setOutput('published_version', currentVersion); + console.log(`✅ Published ${crateName}@${currentVersion} to crates.io`); + return; + } + // cargo accepted the upload but the crates.io index has not exposed it + // within the polling window. Failing here would mark a completed release + // as failed, so the accepted upload is reported as the outcome instead. + if (error?.verificationFailed && uploadAccepted) { console.warn( - `Verification could not confirm ${packageName}@${currentVersion} on crates.io after ${VERIFY_RETRIES} attempts` - ); - console.warn( - 'This may be a crates.io propagation delay. Treating as successful since cargo publish exited with code 0.' + `cargo publish succeeded but ${crateName}@${currentVersion} is not visible on crates.io yet; treating as published.` ); setOutput('published', 'true'); setOutput('published_version', currentVersion); return; } - console.error(`\u274c Failed to publish after ${MAX_RETRIES} attempts`); + console.error(`❌ Publish failed: ${error.message}`); setOutput('published', 'false'); process.exit(1); } catch (error) { diff --git a/scripts/publish-to-npm.mjs b/scripts/publish-to-npm.mjs index eb3c66eb..b132cdff 100644 --- a/scripts/publish-to-npm.mjs +++ b/scripts/publish-to-npm.mjs @@ -2,10 +2,15 @@ /** * Publish to npm using OIDC trusted publishing - * Usage: node scripts/publish-to-npm.mjs [--should-pull] + * Usage: node scripts/publish-to-npm.mjs [--should-pull] [--js-root ] * should_pull: Optional flag to pull latest changes before publishing (for release job) * - * IMPORTANT: Update the PACKAGE_NAME constant below to match your package.json + * Configuration: + * - CLI: --js-root to explicitly set JavaScript root + * - Environment: JS_ROOT= + * + * The package name is read from package.json, so there is nothing to keep in + * sync by hand. * * Uses link-foundation libraries: * - use-m: Dynamic package loading without package.json dependencies @@ -13,17 +18,20 @@ * - lino-arguments: Unified configuration from CLI args, env vars, and .lenv files */ -import { readFileSync, appendFileSync } from 'fs'; +import { appendFileSync } from 'fs'; +import { getJsRoot, needsCd, parseJsRootConfig } from './js-paths.mjs'; +import { isPackageVersionPublished } from './npm-registry.mjs'; +import { readPackageInfo } from './package-info.mjs'; import { - getJsRoot, - getPackageJsonPath, - needsCd, - parseJsRootConfig, -} from './js-paths.mjs'; - -// Package name from package.json -const PACKAGE_NAME = '@link-assistant/agent'; + buildAuthFailureGuidance, + isNonRetryableFailure, +} from './publish-failure-classifier.mjs'; +import { + isAlreadyPublishedError, + publishWithRetry, + sleep, +} from './publish-retry.mjs'; // Load use-m dynamically const { use } = eval( @@ -46,7 +54,8 @@ const config = makeConfig({ .option('js-root', { type: 'string', default: getenv('JS_ROOT', ''), - describe: 'JavaScript package root directory (auto-detected if not specified)', + describe: + 'JavaScript package root directory (auto-detected if not specified)', }), }); @@ -55,10 +64,16 @@ const { shouldPull, jsRoot: jsRootArg } = config; // Get JavaScript package root (auto-detect or use explicit config) const jsRootConfig = jsRootArg || parseJsRootConfig(); const jsRoot = getJsRoot({ jsRoot: jsRootConfig, verbose: true }); + const MAX_RETRIES = 3; const RETRY_DELAY = 10000; // 10 seconds -// Patterns that indicate publish failure in changeset output +// Store the original working directory to restore after cd commands +// IMPORTANT: command-stream's cd is a virtual command that calls process.chdir() +const originalCwd = process.cwd(); + +// Patterns that indicate publish failure in changeset output. +// Guards against a publish command that reports success without publishing. const FAILURE_PATTERNS = [ 'packages failed to publish', 'error occurred while publishing', @@ -70,21 +85,13 @@ const FAILURE_PATTERNS = [ 'ENEEDAUTH', ]; -/** - * Sleep for specified milliseconds - * @param {number} ms - */ -function sleep(ms) { - return new Promise((resolve) => globalThis.setTimeout(resolve, ms)); -} - /** * Check if the output contains any failure patterns * @param {string} output - Combined stdout and stderr * @returns {string|null} - The matched failure pattern or null if no failure detected */ function detectPublishFailure(output) { - const lowerOutput = output.toLowerCase(); + const lowerOutput = String(output || '').toLowerCase(); for (const pattern of FAILURE_PATTERNS) { if (lowerOutput.includes(pattern.toLowerCase())) { return pattern; @@ -99,11 +106,8 @@ function detectPublishFailure(output) { * @param {string} version * @returns {Promise} */ -async function verifyPublished(packageName, version) { - const result = await $`npm view "${packageName}@${version}" version`.run({ - capture: true, - }); - return result.code === 0 && result.stdout.trim().includes(version); +function verifyPublished(packageName, version) { + return isPackageVersionPublished(packageName, version); } /** @@ -118,133 +122,174 @@ function setOutput(key, value) { } } -async function main() { - // Store the original working directory to restore after cd commands - // IMPORTANT: command-stream's cd is a virtual command that calls process.chdir() - const originalCwd = process.cwd(); +/** + * Run changeset:publish command with output capture + * @param {Function} shell + * @param {string} packageRoot + * @param {string} restoreCwd + * @returns {Promise<{result: object|null, error: Error|null}>} + */ +async function runChangesetPublish(shell, packageRoot, restoreCwd) { + try { + // Run changeset:publish from the js directory where package.json with this script exists + // IMPORTANT: Use .run({ capture: true }) to capture output for failure detection + // IMPORTANT: cd is a virtual command that calls process.chdir(), so we restore after + if (needsCd({ jsRoot: packageRoot })) { + const result = + await shell`cd ${packageRoot} && npm run changeset:publish`.run({ + capture: true, + }); + process.chdir(restoreCwd); + return { result, error: null }; + } + const result = await shell`npm run changeset:publish`.run({ + capture: true, + }); + return { result, error: null }; + } catch (error) { + // Restore cwd on error before retry + if (needsCd({ jsRoot: packageRoot })) { + process.chdir(restoreCwd); + } + return { result: null, error }; + } +} +/** + * Analyze publish result for failures using multi-layer detection + * @param {object|null} publishResult - The result from runChangesetPublish + * @param {Error|null} commandError - Error thrown by the command + * @returns {Error|null} - Error if failure detected, null otherwise + */ +function analyzePublishResult(publishResult, commandError) { + if (commandError) { + return commandError; + } + + const combinedOutput = publishResult + ? `${publishResult.stdout || ''}\n${publishResult.stderr || ''}` + : ''; + + // Log the output for debugging + if (combinedOutput.trim()) { + console.log('Changeset output:', combinedOutput); + } + + // Check for failure patterns in output (most reliable for changeset) + const failurePattern = detectPublishFailure(combinedOutput); + if (failurePattern) { + console.error(`Detected publish failure: "${failurePattern}"`); + return new Error(`Publish failed: detected "${failurePattern}" in output`); + } + + // Check exit code (if available and non-zero) + if (publishResult && publishResult.code !== 0) { + console.error(`Changeset exited with code ${publishResult.code}`); + return new Error(`Publish failed with exit code ${publishResult.code}`); + } + + return null; +} + +/** + * Run a single publish command invocation (no verification). + * Verification is a separate failure domain handled by publishWithRetry. + * @param {Function} shell + * @param {string} packageRoot + * @param {string} restoreCwd + * @returns {Promise<{success: boolean, error: Error|null, output: string}>} + */ +async function runPublishCommand(shell, packageRoot, restoreCwd) { + const { result, error } = await runChangesetPublish( + shell, + packageRoot, + restoreCwd + ); + const analysisError = analyzePublishResult(result, error); + const output = [ + analysisError?.message || '', + result?.stdout || '', + result?.stderr || '', + ].join('\n'); + + if (analysisError) { + // Mark authentication / registry-configuration failures as non-retryable so + // the retry loop can fail fast with actionable guidance without burning + // through MAX_RETRIES. + if (!isAlreadyPublishedError(output) && isNonRetryableFailure(output)) { + analysisError.nonRetryable = true; + } + return { success: false, error: analysisError, output }; + } + + return { success: true, error: null, output }; +} + +async function main() { try { if (shouldPull) { // Pull the latest changes we just pushed await $`git pull origin main`; } - // Get current version - const packageJsonPath = getPackageJsonPath({ jsRoot }); - const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); - const currentVersion = packageJson.version; + // Get current package name and version + const { name: packageName, version: currentVersion } = readPackageInfo({ + jsRoot, + }); + console.log(`Package to publish: ${packageName}`); console.log(`Current version to publish: ${currentVersion}`); // Check if this version is already published on npm console.log( `Checking if version ${currentVersion} is already published...` ); - const checkResult = - await $`npm view "${PACKAGE_NAME}@${currentVersion}" version`.run({ - capture: true, - }); - - // command-stream returns { code: 0 } on success, { code: 1 } on failure (e.g., E404) - // Exit code 0 means version exists, non-zero means version not found - if (checkResult.code === 0) { + const alreadyPublished = await isPackageVersionPublished( + packageName, + currentVersion + ); + + if (alreadyPublished) { console.log(`Version ${currentVersion} is already published to npm`); setOutput('published', 'true'); setOutput('published_version', currentVersion); setOutput('already_published', 'true'); return; - } else { - // Version not found on npm (E404), proceed with publish - console.log( - `Version ${currentVersion} not found on npm, proceeding with publish...` - ); } - // Publish to npm using OIDC trusted publishing with retry logic - for (let i = 1; i <= MAX_RETRIES; i++) { - console.log(`Publish attempt ${i} of ${MAX_RETRIES}...`); - let publishResult; - let lastError = null; - - try { - // Run changeset:publish from the js directory where package.json with this script exists - // IMPORTANT: Use .run({ capture: true }) to capture output for failure detection - // IMPORTANT: cd is a virtual command that calls process.chdir(), so we restore after - if (needsCd({ jsRoot })) { - publishResult = await $`cd ${jsRoot} && npm run changeset:publish`.run({ capture: true }); - process.chdir(originalCwd); - } else { - publishResult = await $`npm run changeset:publish`.run({ capture: true }); - } - } catch (error) { - // Restore cwd on error before retry - if (needsCd({ jsRoot })) { - process.chdir(originalCwd); - } - lastError = error; - } - - // Check for failures in multiple ways: - // 1. Check if command threw an exception - // 2. Check exit code (changeset may not return non-zero, but check anyway) - // 3. Check output for failure patterns (most reliable for changeset) - // 4. Verify package is actually on npm (ultimate verification) - - const combinedOutput = publishResult - ? `${publishResult.stdout || ''}\n${publishResult.stderr || ''}` - : ''; - - // Log the output for debugging - if (combinedOutput.trim()) { - console.log('Changeset output:', combinedOutput); - } - - // Check for failure patterns in output - const failurePattern = detectPublishFailure(combinedOutput); - if (failurePattern) { - console.error(`Detected publish failure: "${failurePattern}"`); - lastError = new Error(`Publish failed: detected "${failurePattern}" in output`); - } - - // Check exit code (if available and non-zero) - if (publishResult && publishResult.code !== 0) { - console.error(`Changeset exited with code ${publishResult.code}`); - lastError = lastError || new Error(`Publish failed with exit code ${publishResult.code}`); - } - - // If no errors detected so far, verify the package is actually on npm - if (!lastError) { - console.log('Verifying package was published to npm...'); - // Wait a moment for npm registry to propagate - await sleep(2000); - const isPublished = await verifyPublished(PACKAGE_NAME, currentVersion); - - if (isPublished) { - setOutput('published', 'true'); - setOutput('published_version', currentVersion); - console.log( - `\u2705 Published ${PACKAGE_NAME}@${currentVersion} to npm` - ); - return; - } else { - console.error('Verification failed: package not found on npm after publish'); - lastError = new Error('Package not found on npm after publish attempt'); - } - } - - // If we have an error, either retry or fail - if (lastError) { - if (i < MAX_RETRIES) { - console.log( - `Publish failed, waiting ${RETRY_DELAY / 1000}s before retry...` - ); - await sleep(RETRY_DELAY); - } - } + // Version not found on npm, proceed with publish + console.log( + `Version ${currentVersion} not found on npm, proceeding with publish...` + ); + + // The publish command is retried only when the publish itself failed. + // A verification miss is registry propagation lag, not a publish failure, + // so it is handled by bounded polling and never triggers a republish. + const { success, error } = await publishWithRetry({ + publish: () => runPublishCommand($, jsRoot, originalCwd), + verify: () => verifyPublished(packageName, currentVersion), + maxRetries: MAX_RETRIES, + retryDelay: RETRY_DELAY, + sleepFn: sleep, + log: (message) => console.log(message), + }); + + if (success) { + setOutput('published', 'true'); + setOutput('published_version', currentVersion); + console.log(`✅ Published ${packageName}@${currentVersion} to npm`); + return; } - console.error(`\u274C Failed to publish after ${MAX_RETRIES} attempts`); + console.error(`❌ Publish failed: ${error.message}`); + // Authentication / registry-configuration errors will not be fixed by + // retrying, so print actionable guidance for the operator. + if (error?.nonRetryable && !error?.verificationFailed) { + console.error(buildAuthFailureGuidance(packageName)); + } process.exit(1); } catch (error) { + // Restore cwd on error + process.chdir(originalCwd); console.error('Error:', error.message); process.exit(1); } diff --git a/scripts/rust-collect-changelog.mjs b/scripts/rust-collect-changelog.mjs index dd6712c7..20fb7303 100644 --- a/scripts/rust-collect-changelog.mjs +++ b/scripts/rust-collect-changelog.mjs @@ -32,7 +32,10 @@ const getArg = (name, defaultValue) => { // Get Rust package root (auto-detect or use explicit config) const rustRootConfig = getArg('rust-root', '') || parseRustRootConfig(); -const rustRoot = getRustRoot({ rustRoot: rustRootConfig || undefined, verbose: true }); +const rustRoot = getRustRoot({ + rustRoot: rustRootConfig || undefined, + verbose: true, +}); // Get paths based on detected/configured rust root const CARGO_TOML = getCargoTomlPath({ rustRoot }); diff --git a/scripts/rust-get-bump-type.mjs b/scripts/rust-get-bump-type.mjs index 0a608a91..3d23a393 100644 --- a/scripts/rust-get-bump-type.mjs +++ b/scripts/rust-get-bump-type.mjs @@ -37,7 +37,10 @@ const defaultBump = getArg('default', process.env.DEFAULT_BUMP || 'patch'); // Get Rust package root (auto-detect or use explicit config) const rustRootConfig = getArg('rust-root', '') || parseRustRootConfig(); -const rustRoot = getRustRoot({ rustRoot: rustRootConfig || undefined, verbose: true }); +const rustRoot = getRustRoot({ + rustRoot: rustRootConfig || undefined, + verbose: true, +}); // Get paths based on detected/configured rust root const CHANGELOG_DIR = getChangelogDir({ rustRoot }); diff --git a/scripts/rust-package-info.mjs b/scripts/rust-package-info.mjs new file mode 100644 index 00000000..183b6555 --- /dev/null +++ b/scripts/rust-package-info.mjs @@ -0,0 +1,156 @@ +#!/usr/bin/env node + +/** + * Read the crate name and version from the `[package]` section of Cargo.toml. + * + * Why this exists: the workflow used to shell out to + * + * CRATE_NAME=$(grep -Po '(?<=^name = ")[^"]*' rust/Cargo.toml) + * + * which matches EVERY `name = "..."` at the start of a line — including the one + * in `[lib]`, in `[[bin]]` and in each of the ~40 `[[test]]` sections. The + * variable then held a newline-separated list, the crates.io URL built from it + * was malformed, `curl` exited with code 3, and `set -e` failed the whole + * "Auto Release" job. See https://github.com/link-assistant/agent/issues/287. + * + * Parsing is scoped to the `[package]` section so extra `[[bin]]`/`[[test]]` + * sections can never influence the result. + * + * Usage: + * import { readCrateInfo } from './rust-package-info.mjs'; + * + * CLI (writes name/version to $GITHUB_OUTPUT and stdout): + * node scripts/rust-package-info.mjs + */ + +import { appendFileSync, readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { getCargoTomlPath } from './rust-paths.mjs'; + +/** + * Read a string key from the `[package]` section of a Cargo.toml body. + * @param {string} cargoTomlContent + * @param {string} key + * @returns {string|null} The value, or null when the key is absent + */ +export function readPackageKey(cargoTomlContent, key) { + let inPackageSection = false; + + for (const rawLine of String(cargoTomlContent || '').split(/\r?\n/)) { + const line = rawLine.trim(); + + if (line.startsWith('[')) { + // `[package]` is a table; `[[package]]` or any other header ends it. + inPackageSection = line === '[package]'; + continue; + } + + if (!inPackageSection) { + continue; + } + + const match = line.match(/^([A-Za-z0-9_-]+)\s*=\s*"([^"]*)"/); + if (match && match[1] === key) { + return match[2]; + } + } + + return null; +} + +/** + * Rewrite the `version` key of the `[package]` section, leaving every other + * section untouched. A bare `/^version\s*=\s*"[^"]+"/m` replacement would edit + * whichever `version = "…"` line comes first in the file. + * @param {string} cargoTomlContent + * @param {string} newVersion + * @returns {string} The updated Cargo.toml body + */ +export function setPackageVersion(cargoTomlContent, newVersion) { + let inPackageSection = false; + let replaced = false; + + const lines = String(cargoTomlContent) + .split('\n') + .map((rawLine) => { + const line = rawLine.trim(); + + if (line.startsWith('[')) { + inPackageSection = line === '[package]'; + return rawLine; + } + + if (!inPackageSection || replaced) { + return rawLine; + } + + const match = rawLine.match(/^(\s*version\s*=\s*")[^"]*(".*)$/); + if (!match) { + return rawLine; + } + + replaced = true; + return `${match[1]}${newVersion}${match[2]}`; + }); + + if (!replaced) { + throw new Error('No version key found in the [package] section'); + } + + return lines.join('\n'); +} + +/** + * Parse crate name and version from a Cargo.toml body. + * @param {string} cargoTomlContent + * @param {string} cargoTomlPath - Only used for error messages + * @returns {{name: string, version: string}} + */ +export function parseCrateInfo(cargoTomlContent, cargoTomlPath = 'Cargo.toml') { + const name = readPackageKey(cargoTomlContent, 'name'); + if (!name) { + throw new Error(`Crate name is missing in [package] of ${cargoTomlPath}`); + } + + const version = readPackageKey(cargoTomlContent, 'version'); + if (!version) { + throw new Error( + `Crate version is missing in [package] of ${cargoTomlPath}` + ); + } + + return { name, version }; +} + +/** + * Read crate name and version from the detected Rust package root. + * @param {Object} options - Configuration options (passed to getCargoTomlPath) + * @returns {{name: string, version: string}} + */ +export function readCrateInfo(options = {}) { + const cargoTomlPath = getCargoTomlPath(options); + return parseCrateInfo(readFileSync(cargoTomlPath, 'utf8'), cargoTomlPath); +} + +function setOutput(key, value) { + const outputFile = process.env.GITHUB_OUTPUT; + if (outputFile) { + appendFileSync(outputFile, `${key}=${value}\n`); + } + console.log(`Output: ${key}=${value}`); +} + +function isCliEntryPoint() { + return ( + process.argv?.[1] && + fileURLToPath(import.meta.url) === path.resolve(process.argv[1]) + ); +} + +if (isCliEntryPoint()) { + const { name, version } = readCrateInfo(); + setOutput('name', name); + setOutput('version', version); +} diff --git a/scripts/rust-paths.mjs b/scripts/rust-paths.mjs index 4f4636ab..658d0f7f 100644 --- a/scripts/rust-paths.mjs +++ b/scripts/rust-paths.mjs @@ -69,13 +69,13 @@ export function getRustRoot(options = {}) { // No Cargo.toml found throw new Error( 'Could not find Cargo.toml in expected locations.\n' + - 'Searched in:\n' + - ' - ./Cargo.toml (single-language repository)\n' + - ' - ./rust/Cargo.toml (multi-language repository)\n\n' + - 'To fix this, either:\n' + - ' 1. Run the script from the repository root\n' + - ' 2. Explicitly configure the Rust root using --rust-root option\n' + - ' 3. Set the RUST_ROOT environment variable' + 'Searched in:\n' + + ' - ./Cargo.toml (single-language repository)\n' + + ' - ./rust/Cargo.toml (multi-language repository)\n\n' + + 'To fix this, either:\n' + + ' 1. Run the script from the repository root\n' + + ' 2. Explicitly configure the Rust root using --rust-root option\n' + + ' 3. Set the RUST_ROOT environment variable' ); } diff --git a/scripts/rust-version-and-commit.mjs b/scripts/rust-version-and-commit.mjs index c995cb07..f968230e 100644 --- a/scripts/rust-version-and-commit.mjs +++ b/scripts/rust-version-and-commit.mjs @@ -16,8 +16,9 @@ import { unlinkSync, } from 'fs'; import { join } from 'path'; -import { execSync } from 'child_process'; +import { execFileSync, execSync } from 'child_process'; +import { readPackageKey, setPackageVersion } from './rust-package-info.mjs'; import { getRustRoot, getCargoTomlPath, @@ -40,7 +41,10 @@ const description = getArg('description', process.env.DESCRIPTION || ''); // Get Rust package root (auto-detect or use explicit config) const rustRootConfig = getArg('rust-root', '') || parseRustRootConfig(); -const rustRoot = getRustRoot({ rustRoot: rustRootConfig || undefined, verbose: true }); +const rustRoot = getRustRoot({ + rustRoot: rustRootConfig || undefined, + verbose: true, +}); if (!bumpType || !['major', 'minor', 'patch'].includes(bumpType)) { console.error( @@ -91,10 +95,15 @@ function exec(command) { */ function getCurrentVersion() { const cargoToml = readFileSync(CARGO_TOML, 'utf-8'); - const match = cargoToml.match(/^version\s*=\s*"(\d+)\.(\d+)\.(\d+)"/m); + // Scoped to the [package] table: a bare /^version = "…"/m also matches + // sections such as [dependencies.*], which would bump the wrong value. + const packageVersion = readPackageKey(cargoToml, 'version'); + const match = packageVersion?.match(/^(\d+)\.(\d+)\.(\d+)$/); if (!match) { - console.error('Error: Could not parse version from Cargo.toml'); + console.error( + 'Error: Could not parse a semver version from [package] in Cargo.toml' + ); process.exit(1); } @@ -131,12 +140,8 @@ function calculateNewVersion(current, bumpType) { * @param {string} newVersion */ function updateCargoToml(newVersion) { - let cargoToml = readFileSync(CARGO_TOML, 'utf-8'); - cargoToml = cargoToml.replace( - /^(version\s*=\s*")[^"]+(")/m, - `$1${newVersion}$2` - ); - writeFileSync(CARGO_TOML, cargoToml, 'utf-8'); + const cargoToml = readFileSync(CARGO_TOML, 'utf-8'); + writeFileSync(CARGO_TOML, setPackageVersion(cargoToml, newVersion), 'utf-8'); console.log(`Updated Cargo.toml to version ${newVersion}`); } @@ -248,7 +253,7 @@ ${newEntry} const MAX_PUSH_RETRIES = 3; -async function main() { +function main() { try { // Configure git exec('git config user.name "github-actions[bot]"'); @@ -258,7 +263,11 @@ async function main() { // Pull latest changes from remote before starting console.log('Fetching latest changes from origin/main...'); - exec('git fetch origin main'); + // Tags must be fetched too: checkTagExists() below decides whether this + // version was already released, and without the remote tags it answers + // "no" for a tag that exists upstream. The release then proceeds and + // `git push --tags` is rejected, failing the job. + exec('git fetch origin main --tags'); const localHead = exec('git rev-parse HEAD'); const remoteHead = exec('git rev-parse origin/main'); @@ -320,14 +329,18 @@ async function main() { const commitMsg = description ? `chore(rust): release v${newVersion}\n\n${description}` : `chore(rust): release v${newVersion}`; - exec(`git commit -m "${commitMsg.replace(/"/g, '\\"')}"`); + // execFileSync (no shell) so a description containing quotes, backticks or + // $VAR cannot be interpreted by the shell or break the command. + execFileSync('git', ['commit', '-m', commitMsg], { stdio: 'pipe' }); console.log(`Committed version ${newVersion}`); // Create tag const tagMsg = description ? `Rust Release v${newVersion}\n\n${description}` : `Rust Release v${newVersion}`; - exec(`git tag -a rust-v${newVersion} -m "${tagMsg.replace(/"/g, '\\"')}"`); + execFileSync('git', ['tag', '-a', `rust-v${newVersion}`, '-m', tagMsg], { + stdio: 'pipe', + }); console.log(`Created tag rust-v${newVersion}`); // Push with retry: if another CI job (e.g. JS release) pushed to main @@ -335,8 +348,11 @@ async function main() { for (let attempt = 1; attempt <= MAX_PUSH_RETRIES; attempt++) { try { exec('git push'); - exec('git push --tags'); - console.log('Pushed changes and tags'); + // Push only the tag created above. `git push --tags` also pushes every + // unrelated local tag, so one stale or conflicting tag would fail the + // release even though this one is fine. + exec(`git push origin refs/tags/rust-v${newVersion}`); + console.log('Pushed changes and tag'); break; } catch (pushError) { if (attempt < MAX_PUSH_RETRIES) { diff --git a/scripts/simulate-fresh-merge.sh b/scripts/simulate-fresh-merge.sh new file mode 100755 index 00000000..544b7759 --- /dev/null +++ b/scripts/simulate-fresh-merge.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# simulate-fresh-merge.sh +# +# Merge the latest base branch into the checked-out pull request before CI runs +# its checks, so the checks validate the state that will actually land on the +# base branch rather than a stale merge preview. +# +# GitHub builds the `refs/pull/N/merge` ref when the pull request is opened or +# synchronized. Commits pushed to the base branch afterwards are not in it, so +# a pull request can be green while its merge result is broken. +# +# Usage: +# BASE_REF=main bash scripts/simulate-fresh-merge.sh +# +# Exit code 0 = merge succeeded or was unnecessary, non-zero = merge conflict. +# +# See https://github.com/link-assistant/agent/issues/287 and principle 7 of +# https://github.com/link-assistant/hive-mind/blob/main/docs/CI-CD-BEST-PRACTICES.md + +set -euo pipefail + +if [ -z "${BASE_REF:-}" ]; then + echo "::error::BASE_REF is not set; cannot simulate a merge." + exit 1 +fi + +git config user.email "github-actions[bot]@users.noreply.github.com" +git config user.name "github-actions[bot]" + +git fetch origin "$BASE_REF" + +BEHIND_COUNT=$(git rev-list --count "HEAD..origin/$BASE_REF") + +if [ "$BEHIND_COUNT" -eq 0 ]; then + echo "Checkout already contains every commit of $BASE_REF; no merge needed." + exit 0 +fi + +echo "$BASE_REF has $BEHIND_COUNT commit(s) that are not in this checkout." +echo "Merging them so the checks below run on the real merge result." + +if git merge "origin/$BASE_REF" --no-edit; then + echo "Fresh merge simulation succeeded." +else + echo "::error::Merge conflict with $BASE_REF. Update the branch before merging." + exit 1 +fi diff --git a/scripts/version-and-commit.mjs b/scripts/version-and-commit.mjs index 28d21dce..d849d14c 100644 --- a/scripts/version-and-commit.mjs +++ b/scripts/version-and-commit.mjs @@ -54,7 +54,8 @@ const config = makeConfig({ .option('js-root', { type: 'string', default: getenv('JS_ROOT', ''), - describe: 'JavaScript package root directory (auto-detected if not specified)', + describe: + 'JavaScript package root directory (auto-detected if not specified)', }), });