Skip to content

ci: fix integration test runs - #3112

Merged
opieter-aws merged 1 commit into
strands-agents:mainfrom
opieter-aws:opieter-aws/pr-2836-skipped-integ-tests
Jul 7, 2026
Merged

opieter-aws merged 1 commit into
strands-agents:mainfrom
opieter-aws:opieter-aws/pr-2836-skipped-integ-tests

Conversation

@opieter-aws

@opieter-aws opieter-aws commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

Integration tests have been silently skipping on every PR since #2940 merged. The Run integration tests job reports skipping even when the PR is authorized and non-draft, so no integration coverage actually runs — for strands-py or strands-ts — in the merge gate.

#2940 added a validate-call-ref job that only runs on the workflow_call path (if: inputs.ref != ''). On pull_request_target and merge_group that job is skipped, and a skipped needs predecessor propagates transitively through the implicit success() gate of every downstream job. #2940 handled this for authorization-check by giving it if: always() && ..., but the test job (check-access-and-checkout / run-integration-tests) has no such guard, so the skip cascaded into it — the job was killed before it ever reached the environment approval gate.

This adds the same guard to the test job in both integration-test workflows:

if: always() && needs.authorization-check.result == 'success'

always() detaches the job from the transitively-skipped validate-call-ref, while requiring authorization-check to have succeeded keeps the security posture unchanged: a fork/cross-repo ref fails validate-call-ref, which makes authorization-check evaluate to skipped (not success), so the test job still does not run. The auth gate and the environment-based manual-approval flow for drafts/non-writers are untouched.

Related Issues

Investigation traced from #2836 (where the skip surfaced). Regression introduced by #2940.

Documentation PR

Not applicable — CI-only change.

Type of Change

Bug fix

Testing

pull_request_target runs the workflow definition from the base branch, so the fix only takes effect once merged; it cannot be exercised from the PR branch itself. Validated statically:

  • Both workflow files parse as valid YAML.

  • Traced the guard against every trigger path — pull_request_target (writer/draft/non-writer), merge_group, workflow_call (valid ref), and workflow_call (rejected fork/cross-repo ref). Only the fork/bad-ref case keeps the job skipped, confirming the trust invariant holds.

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions github-actions Bot added chore Maintenance tasks, dependency updates, CI changes, refactoring with no user-facing impact size/xs area-community Related to community and contributor health labels Jul 7, 2026
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@opieter-aws
opieter-aws requested a review from poshinchen July 7, 2026 16:07
@opieter-aws
opieter-aws marked this pull request as ready for review July 7, 2026 16:07
@opieter-aws
opieter-aws requested a review from a team as a code owner July 7, 2026 16:07
@opieter-aws
opieter-aws enabled auto-merge (squash) July 7, 2026 16:09
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Assessment: Approve

Correct, minimal, security-preserving fix. I traced the always() && needs.authorization-check.result == 'success' guard across every trigger path (pull_request_target writer/draft/non-writer, merge_group, workflow_call valid ref, and workflow_call rejected fork/cross-repo ref) and confirmed it restores lost integration coverage while keeping the trust invariant intact — a rejected ref leaves authorization-check as skipped, so the test job still won't run.

Review Notes
  • Correctness: Guard behaves as described on all paths; env-based approval flow for drafts/non-writers untouched.
  • Scope/completeness: Both integration-test workflows are fixed. Other authorization-check consumers have it as their first job (no validate-call-ref predecessor), so they don't share this transitive-skip bug — nothing else to patch here.
  • Maintainability (non-blocking): The new guard is the only trust-relevant if: in these files without an explanatory comment. Given how subtle the pattern is, a short comment (as noted inline) would protect it from being "simplified" away later.

Nice investigative work tracing this back to the #2940 regression.

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

Labels

area-community Related to community and contributor health chore Maintenance tasks, dependency updates, CI changes, refactoring with no user-facing impact size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants