Skip to content

Verification improvements & hardening#325

Merged
khaliqgant merged 15 commits intomainfrom
verification-improvements
Jan 28, 2026
Merged

Verification improvements & hardening#325
khaliqgant merged 15 commits intomainfrom
verification-improvements

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

@khaliqgant khaliqgant commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View issue and 5 additional flags in Devin Review.

Open in Devin Review

Comment thread .github/workflows/verify-publish.yml
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 4 new potential issues.

View issues and 7 additional flags in Devin Review.

Open in Devin Review

Comment on lines 65 to +81
if: inputs.version != 'latest'
run: |
echo "Waiting for npm to propagate version ${{ inputs.version }}..."
FOUND=0
for i in {1..30}; do
if npm view agent-relay@${{ inputs.version }} version 2>/dev/null; then
echo "Package found on npm registry"
FOUND=1
break
fi
echo "Attempt $i: Package not yet available, waiting 10s..."
sleep 10
done
if [ "$FOUND" -eq 0 ]; then
echo "ERROR: Package agent-relay@${{ inputs.version }} not found on npm after 5 minutes"
exit 1
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Wait for npm propagation step skipped when version is empty string

The npm propagation wait step uses if: inputs.version != 'latest' condition, but when the workflow is triggered via pull_request (for testing the workflow itself), inputs.version will be empty/undefined, not 'latest'. This causes the condition to evaluate to true (empty != 'latest'), but then the step tries to use ${{ inputs.version }} which is empty, resulting in an invalid npm view command.

Click to expand

How it happens

In verify-publish.yml, the workflow can be triggered via:

  1. pull_request - no inputs provided
  2. workflow_dispatch - version input with default 'latest'
  3. workflow_call - version input with default 'latest'

When triggered via pull_request, inputs.version is empty/undefined. The condition at line 65 if: inputs.version != 'latest' evaluates to true because empty string != 'latest'.

Then the step runs:

npm view agent-relay@${{ inputs.version }} version

With empty inputs.version, this becomes npm view agent-relay@ version which is invalid.

Actual vs Expected

  • Actual: Step runs with empty version, causing npm view to fail or behave unexpectedly
  • Expected: Step should be skipped when version is empty (same as when it's 'latest')

Impact

The verification workflow will fail when triggered via pull_request, which is one of its documented trigger conditions.

Recommendation: Change the condition to also check for empty version: if: inputs.version != 'latest' && inputs.version != '' or use the same pattern as line 52-54 which properly handles empty input.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +321 to +329
run: |
VERSION="${{ inputs.version || 'latest' }}"
if [ "$VERSION" = "latest" ]; then
SPEC="agent-relay"
else
SPEC="agent-relay@${VERSION}"
fi
echo "spec=$SPEC" >> $GITHUB_OUTPUT
echo "Testing: $SPEC"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Inconsistent version handling between verify job and verify-macos job

The verify job and verify-macos job use different logic to determine the package spec, which can lead to inconsistent behavior.

Click to expand

How it happens

In the verify job (lines 51-61), the code explicitly checks for empty input:

VERSION_INPUT="${{ inputs.version }}"
if [ -z "$VERSION_INPUT" ] || [ "$VERSION_INPUT" = "latest" ]; then
    VERSION="latest"
    SPEC="agent-relay"

But in the verify-macos job (lines 321-328), it uses a different pattern:

VERSION="${{ inputs.version || 'latest' }}"
if [ "$VERSION" = "latest" ]; then
    SPEC="agent-relay"

The inputs.version || 'latest' syntax in GitHub Actions will use 'latest' only if inputs.version is falsy (empty/undefined). This is correct, but the inconsistency makes the code harder to maintain and could lead to subtle bugs if one pattern is changed but not the other.

Impact

Low - both approaches work, but inconsistent patterns can lead to maintenance issues.

Recommendation: Use consistent version handling across all jobs. The pattern in the verify job (lines 51-61) is more explicit and defensive.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +333 to +349
if: inputs.version != 'latest'
run: |
echo "Waiting for npm to propagate version ${{ inputs.version }}..."
FOUND=0
for i in {1..30}; do
if npm view agent-relay@${{ inputs.version }} version 2>/dev/null; then
echo "Package found on npm registry"
FOUND=1
break
fi
echo "Attempt $i: Package not yet available, waiting 10s..."
sleep 10
done
if [ "$FOUND" -eq 0 ]; then
echo "ERROR: Package agent-relay@${{ inputs.version }} not found on npm after 5 minutes"
exit 1
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 verify-macos npm propagation wait condition doesn't handle empty version

Similar to BUG-0001, the verify-macos job's npm propagation wait step at line 333 uses if: inputs.version != 'latest' which will incorrectly run when version is empty.

Click to expand

How it happens

When the workflow is triggered via pull_request, inputs.version is empty. The condition inputs.version != 'latest' evaluates to true, causing the wait step to run with an empty version:

npm view agent-relay@${{ inputs.version }} version

This becomes npm view agent-relay@ version which is invalid.

Actual vs Expected

  • Actual: Step runs with empty version string
  • Expected: Step should be skipped when version is empty

Recommendation: Change the condition to: if: inputs.version != 'latest' && inputs.version != ''

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +473 to +489
if: inputs.version != 'latest'
run: |
echo "Waiting for npm to propagate version ${{ inputs.version }}..."
FOUND=0
for i in {1..30}; do
if npm view agent-relay@${{ inputs.version }} version 2>/dev/null; then
echo "Package found on npm registry"
FOUND=1
break
fi
echo "Attempt $i: Package not yet available, waiting 10s..."
sleep 10
done
if [ "$FOUND" -eq 0 ]; then
echo "ERROR: Package agent-relay@${{ inputs.version }} not found on npm after 5 minutes"
exit 1
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 verify-docker npm propagation wait condition doesn't handle empty version

The verify-docker job's npm propagation wait step at line 473 has the same issue as BUG-0001 and BUG-0003.

Click to expand

How it happens

When triggered via pull_request, inputs.version is empty. The condition inputs.version != 'latest' evaluates to true, causing the wait step to run with an empty version.

Impact

The Docker verification will fail when triggered via pull_request.

Recommendation: Change the condition to: if: inputs.version != 'latest' && inputs.version != ''

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 14 additional flags in Devin Review.

Open in Devin Review

Comment on lines +326 to +327
if [ "${{ github.event_name }}" = "pull_request" ]; then
SPEC="./$(ls agent-relay-*.tgz | head -1)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 verify-macos job will fail on PRs due to missing checkout and npm pack steps

The verify-macos job attempts to use a local tarball in PR mode but lacks the necessary setup steps to create it.

Click to expand

Issue

At lines 326-327 in verify-publish.yml, the verify-macos job tries to use a local tarball when github.event_name == 'pull_request':

if [ "${{ github.event_name }}" = "pull_request" ]; then
  SPEC="./$(ls agent-relay-*.tgz | head -1)"

However, unlike the verify job which has both a checkout step (line 42) and an npm pack step (lines 44-46), the verify-macos job is missing both of these prerequisites.

Actual behavior

When a PR is opened that touches .github/workflows/verify-publish.yml or scripts/post-publish-verify/**, the verify-macos job will fail with an error because:

  1. No checkout means no source code to pack
  2. No npm pack step means no tarball is created
  3. The ls agent-relay-*.tgz command will fail or return nothing

Expected behavior

The verify-macos job should either:

  1. Add checkout and npm pack steps like the verify job has, OR
  2. Use the same logic as verify job's Get package spec step which doesn't reference the tarball

Impact

The verify-macos job will always fail when triggered by PRs, preventing proper testing of macOS binary verification on pull requests.

Recommendation: Add checkout and npm pack steps to the verify-macos job before the 'Get package spec' step, similar to how the 'verify' job handles it at lines 41-46.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

khaliqgant and others added 12 commits January 28, 2026 10:23
Addresses Devin review feedback - the verify-macos job was missing the
npm propagation wait step that exists in the verify job, causing potential
race conditions when verifying newly published packages.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The npm propagation wait loops in verify, verify-macos, and verify-docker
jobs were silently continuing even if the package was never found on npm.
After 30 attempts (5 minutes), the workflow would proceed with the tests
which would then fail with confusing errors.

Changes:
- Add FOUND flag to track if package was found
- Exit with error code 1 if package not found after timeout
- Add npm propagation wait to verify-docker job (was missing entirely)
- Add Node.js setup step to verify-docker for npm view command

This ensures CI fails fast with a clear error message instead of
proceeding with tests that will inevitably fail.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@khaliqgant khaliqgant force-pushed the verification-improvements branch from 3b527f5 to 4d2918d Compare January 28, 2026 09:28
@khaliqgant khaliqgant merged commit 847804a into main Jan 28, 2026
28 of 31 checks passed
@khaliqgant khaliqgant deleted the verification-improvements branch January 28, 2026 09:37
khaliqgant added a commit that referenced this pull request Jan 28, 2026
…ovements"

This reverts commit 847804a, reversing
changes made to 1e13a6a.
khaliqgant added a commit that referenced this pull request Jan 28, 2026
khaliqgant added a commit that referenced this pull request Jan 28, 2026
The @agent-relay/utils package had "main" and "default" export fields pointing to
the ES module (dist/index.js) instead of the CommonJS version (dist/cjs/index.js).
This caused require() calls to fail with "ERROR: require() of ES Module ... not supported"
when used from CommonJS contexts like the Docker verification tests.

Changes:
- Set "main" field to "dist/cjs/index.js" (was "dist/index.js")
- Reorder exports conditions: require before import
- Set "default" field to "./dist/cjs/index.js" for all export paths

This matches the fix applied to the root package in PR #325 and resolves the
Docker 18 spawn infrastructure test failure.

🤖 Generated with Claude Code

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant