Skip to content
Merged
9 changes: 6 additions & 3 deletions .agents/skills/development-workflow/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#
# SPDX-License-Identifier: MIT
name: development-workflow
description: Deliver a PyAthena change through a dedicated worktree, Draft PR, two distinct self-reviews, independent review, and current CI before Ready. Use when implementing or updating a PR, not for a bounded review-only request.
description: Deliver a PyAthena change through a dedicated worktree, Draft PR, two distinct self-reviews, independent review, and current CI once marked Ready. Use when implementing or updating a PR, not for a bounded review-only request.
---

# PyAthena PR delivery
Expand All @@ -28,8 +28,11 @@ Keep unrelated worktrees and changes intact.
4. Complete [self-review](../self-review/SKILL.md), then [self-review-round-two](../self-review-round-two/SKILL.md), fixing verified findings and validating affected behavior.
5. Collect [independent-review](../independent-review/SKILL.md).
Repairs pass through both self-review perspectives and an independent follow-up before completion.
6. Check the current PR with `gh pr view` and `gh pr checks` before `gh pr ready`.
Confirm the published head matches the reviewed head, all applicable checks have completed successfully, and the PR has no merge conflict.
6. Draft PRs run only the offline checks; `gh pr ready` starts the AWS jobs.
Before it, confirm the published head matches the reviewed head, the offline checks have completed successfully, and the PR has no merge conflict.
After it, check the current PR with `gh pr view` and `gh pr checks`, and confirm all applicable checks have completed successfully.
If an AWS job fails, return the PR to Draft with `gh pr ready --undo` until the failure is resolved.
To obtain AWS results while the PR is still Draft, dispatch the Test workflow on its branch.
Pending, cancelled, missing expected checks, and `UNKNOWN` mergeability do not establish readiness.
Explain intentionally skipped jobs from workflow conditions; a passing rerun of one failed job does not make the remaining failures pass.
Keep the PR Draft while required review or validation remains incomplete, unless the user explicitly changes that requirement.
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/test-suite.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ on:
description: Maximum number of Python versions tested concurrently
type: number
default: 5
skip-spark:
description: Skip the Spark tests of the PyAthena suite
type: boolean
default: false
skip-sqla:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review, both rounds, of the skip-sqla change requested by the maintainer (2c3588ba2ceacd15737a4f60af4a508ee66c246e..87aacabba94436acb6d30a39a2aae044ae5ac879): CLEAN

  • Round one (behavior):
    • PYTEST_ADDOPTS is a folded scalar that joins the two conditional ignore groups. With both inputs false it is whitespace only, which pytest treats as no options (checked with PYTEST_ADDOPTS=' ': 1,889 tests collected).
    • With both inputs true, 1,297 tests are collected and none come from tests/pyathena/(aio/)spark/ or tests/pyathena/(aio/)sqlalchemy/. Neither directory has a conftest.py, so ignoring them removes no shared fixtures.
    • skip-sqla reuses the sqla output, which now also matches tests/pyathena/(aio/)sqlalchemy/: editing those tests runs them, and also runs the compliance suites. This keeps a single SQLAlchemy trigger, as for Spark.
    • Only the test (pyathena) job receives skip-sqla; the compliance jobs keep their job-level if. actionlint and just scripts pass.
  • Round two (claims and operations):
    • docs/testing.md now names the "SQLAlchemy tests" column and lists both parts and all trigger paths. It states that the PyAthena suite runs without the skipped directories. just docs lint passes.
    • Operator effect: a ready PR that touches neither area now runs 1,297 of the PyAthena suite's 1,889 tests. Dialect regressions caused by core modules (cursor, formatter, converter) are then caught by the weekly or pre-release run, the same trade-off already accepted for Spark and the compliance suites.
    • The PR body was updated to describe both inputs and the observed Ready/Draft runs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc58-a3ed-7892-ad0b-91b3546dc52a), static review of 2c3588b..87aacab: CLEAN.

The reviewer checked:

  • the folded scalar and boolean expressions (the ignore groups are space-separated, and empty groups leave only harmless whitespace);
  • PYTEST_ADDOPTS propagation through the tox passenv into the pytest command;
  • the filter regexes;
  • that no test outside the ignored directories imports fixtures or setup from them;
  • that docs/testing.md matches the workflow.

No builds or tests were run. Independent review is complete for head 87aacabba94436acb6d30a39a2aae044ae5ac879.

description: Skip the SQLAlchemy tests of the PyAthena suite
type: boolean
default: false

jobs:
run:
Expand All @@ -26,6 +34,9 @@ jobs:

env:
TEST_TYPE: ${{ inputs.test-type }}
PYTEST_ADDOPTS: >-
${{ inputs.skip-spark && '--ignore=tests/pyathena/spark --ignore=tests/pyathena/aio/spark' || '' }}
${{ inputs.skip-sqla && '--ignore=tests/pyathena/sqlalchemy --ignore=tests/pyathena/aio/sqlalchemy' || '' }}
AWS_DEFAULT_REGION: us-west-2
# Keep SDK retries bounded; PyAthena also retries wrapped catalog errors.
AWS_RETRY_MODE: standard
Expand Down
88 changes: 83 additions & 5 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,21 @@ name: Test

on:
pull_request:
# ready_for_review starts the AWS jobs for a pull request leaving Draft;
# converted_to_draft starts a run without them, which cancels an
# in-progress run through the concurrency group. paths-ignore applies to
# both, so a pull request that now changes only docs starts neither.
types: [opened, synchronize, reopened, ready_for_review, converted_to_draft]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc2e-d2c6-7920-a11f-1a93c81fb383): FINDINGS. This was a static review: the reviewer was told not to edit, run tests, or access GitHub, and did none of these.

  • Snapshot: detached worktree at head 44fab0164369491f4171cb6f55f7509a8f40fb65, base 2a107adf1f68713e7b7da2e334cc98ca543b7e7d, literal diff supplied. No PR number, PR description, or prior findings were included in the prompt.
  • Covered: the diff, the changed workflow/process files, justfile, the tox config, and the SQLAlchemy/Spark callers relevant to the path filters.
  • Finding 1 (test.yaml:13 at the reviewed head): the trigger list omitted converted_to_draft. Returning a Ready PR to Draft while its AWS jobs ran started no new run, so the concurrency group did not cancel them. That contradicted the documented Draft behavior and the skill's gh pr ready --undo step. Accepted and repaired in 7787388: converted_to_draft now starts a run in which changes and the AWS jobs are skipped, and that run cancels the in-progress one. docs/testing.md states this.
  • Finding 2 (path filters, line 88 at this head): shared modules such as pyathena/util.py trigger neither the compliance suites nor the Spark tests, although those callers import them. Rejected as a deliberate trade-off, not a defect. Reduce the AWS cost of integration tests without reducing real-service coverage #834's agreed direction limits the compliance-suite trigger to the dialect, its tests, and dependencies, and relies on the weekly run and the pre-release dispatch (both run every suite) to catch regressions from other modules. docs/testing.md lists exactly which paths trigger each suite.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Repair self-review, both perspectives, for 7787388 (range 44fab0164369491f4171cb6f55f7509a8f40fb65..7787388be47f2fe656c628568567c513f7585edc): CLEAN

  • Round one (behavior): a converted_to_draft event has draft == true, so changes and the three AWS jobs are skipped and only lint runs. The run shares the refs/pull/N/merge concurrency group, so cancel-in-progress cancels an in-progress AWS run. paths-ignore still applies, so docs-only PRs start no run, as before. actionlint passes.
  • Round two (claims and operations): the new docs/testing.md sentence (converting back to Draft cancels the AWS jobs) and the workflow comment match this behavior. The extra cost is one offline lint job per conversion to Draft. The skill's gh pr ready --undo step now also stops any AWS jobs still running.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc33-0cbe-7793-9652-9b5bd11a373f), static review of 44fab01..7787388: FINDINGS

  • Confirmed that the repair fixes finding 1: converted_to_draft starts a run in which changes and the AWS jobs are skipped, and the shared concurrency group cancels the in-flight run.
  • New finding: paths-ignore also applies to converted_to_draft. If a Ready PR reverts its code while AWS jobs run, it is left changing only Markdown, and neither the revert nor the conversion to Draft starts a run. The unconditional claim that converting to Draft cancels the jobs in docs/testing.md was therefore too broad. Accepted; wording repaired in 1ec4635 (7787388..1ec4635): the docs sentence is now scoped to the pull requests the workflow runs for, and the workflow comment notes that paths-ignore applies. Behavior is unchanged. For a docs-only PR, the in-flight jobs finish normally and the docs-only state needs no AWS coverage.
  • Repair self-review, both perspectives: CLEAN. actionlint and just docs lint pass. The sentence now follows the section's scope (docs/testing.md:151). No other prose claims unconditional cancellation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up 2 (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc35-0bd0-7522-82f0-064c7f046913), static review of 7787388..1ec4635: FINDINGS

  • Confirmed that the paths-ignore wording repair resolves the previous finding.
  • New finding: docs/testing.md:163 ("these pull requests") still included external forks from line 152. For a fork PR, marking it Ready starts no AWS jobs, because changes requires the head repository to match. Accepted; wording repaired in 46d3c52 (1ec4635..46d3c52): the table row and the sentence now say "a pull request from a branch of this repository". Behavior is unchanged.
  • Repair self-review, both perspectives: CLEAN. just docs lint passes. The fork policy paragraph that follows (line 170) is unchanged and consistent with the new wording.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up 3 (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc37-16c6-74e1-b7cd-f63b1170cee1), static review of 1ec4635..46d3c52 plus a full pass over the GitHub Actions section: FINDINGS

  • Confirmed that the fork-scoping repair matches the repository check in test.yaml.
  • Finding: rewording for forks dropped the docs-only scope, so the Draft-cancellation claim at docs/testing.md:163 was unconditional again, which regresses follow-up 1. Accepted; repaired in 2c3588b (46d3c52..2c3588b): the sentence now covers a pull request from a branch of this repository that still changes files other than docs/ and Markdown. Behavior is unchanged.
  • Repair self-review, both perspectives: CLEAN. just docs lint passes. The sentence now states both conditions that paths-ignore and the changes job impose.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up 4 (relayed): Codex, via the Claude Code codex:codex-rescue agent (Codex thread 01a0dc39-9c42-7210-b055-5a49c602d347), static review of 46d3c52..2c3588b plus a final pass over the GitHub Actions section of docs/testing.md against test.yaml and test-suite.yaml: CLEAN. The reviewer confirmed that the cancellation sentence is now limited to same-repository PRs that still change files outside docs/ and Markdown, and that the remaining claims in the section match the workflow definitions. Independent review is complete for head 2c3588ba2ceacd15737a4f60af4a508ee66c246e.

paths-ignore:
- 'docs/**'
- '**.md'
# The scheduled run executes every suite, including the ones that pull
# requests only run when related files change.
schedule:
- cron: '0 0 * * 0'
# Allows refreshing the README status badge on demand: the badge reflects
# the latest run on the default branch, which is otherwise only the weekly
# scheduled run and stays red for up to a week after a transient failure.
# Runs every suite on the selected branch: before a release, on demand for
# a pull request, and to refresh the README status badge after a transient
# failure on the default branch.
workflow_dispatch:

permissions:
Expand All @@ -29,20 +36,91 @@ concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

# The three suites create their own schemas and tables, so they run in
# parallel; each is still a separate job for "Re-run failed jobs".
jobs:
# Offline checks run for every event, including Draft and fork pull requests.
lint:
runs-on: ubuntu-latest
permissions:
contents: read
steps:
- name: Checkout
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
persist-credentials: false
- uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0
with:
python-version: '3.12'
enable-cache: true
- uses: taiki-e/install-action@7a79fe8c3a13344501c80d99cae481c1c9085912 # v2.81.10
with:
tool: just
- run: just lint

# Selects the AWS suites. Draft and external-fork pull requests run none.
# A ready pull request always runs the PyAthena suite; it runs the
# SQLAlchemy tests (the compliance suites and the PyAthena suite's
# SQLAlchemy tests) and the Spark tests only when their code, tests,
# dependencies, or this workflow change.
changes:
if: >-

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round one (implementation behavior): FINDINGS, repaired

  • Perspective: behavior and failure paths of the workflow conditions, framework contracts (GitHub Actions needs/if on reusable-workflow jobs, pull_request activity types), and simplicity.
  • Base 2a107adf1f68713e7b7da2e334cc98ca543b7e7d, head 99948ec7fd9ea8524c2b997fe9700562c27aa5a0 (initial pass), repair checked at 44fab0164369491f4171cb6f55f7509a8f40fb65.
  • Covered: .github/workflows/test.yaml, .github/workflows/test-suite.yaml, the tox passenv in pyproject.toml, docs/testing.md, AGENTS.md, .agents/skills/development-workflow/SKILL.md.
  • Checked:
    • A Draft or fork PR skips changes, so the three needs: changes jobs are skipped. Observed on the Draft run: lint passed, changes/test/test-sqla/test-sqla-async skipped.
    • ready_for_review has draft == false. Schedule and dispatch take the non-PR branch, which sets both outputs to true.
    • A failing gh api call fails the step under bash -e. The dependent jobs are then skipped and the run is red, not silently green.
    • An empty PYTEST_ADDOPTS is a no-op. With the ignores set, pytest --collect-only tests/pyathena collects no Spark tests.
    • No branch protection requires these checks, so skipped jobs do not block merges.
  • Finding: docs/testing.md:151 said lint runs on every pull request. paths-ignore means docs/Markdown-only PRs do not run the Test workflow at all. Repaired in 44fab01 (this line and the lint job comment at line 37).
  • Limit: the skip path for a non-dialect ready PR can only be observed after merge.

github.event_name != 'pull_request' ||
(!github.event.pull_request.draft &&
github.event.pull_request.head.repo.full_name == github.repository)
runs-on: ubuntu-latest
permissions:
pull-requests: read
outputs:
sqla: ${{ steps.filter.outputs.sqla }}
spark: ${{ steps.filter.outputs.spark }}
steps:
- id: filter
env:
GH_TOKEN: ${{ github.token }}
EVENT_NAME: ${{ github.event_name }}
REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
if [[ "$EVENT_NAME" != "pull_request" ]]; then
echo "sqla=true" >> "$GITHUB_OUTPUT"
echo "spark=true" >> "$GITHUB_OUTPUT"
exit 0
fi
files=$(gh api "repos/$REPO/pulls/$PR_NUMBER/files" --paginate --jq '.[].filename')
printf 'Changed files:\n%s\n' "$files"
shared='^(\.github/workflows/test(-suite)?\.yaml|justfile|pyproject\.toml|uv\.lock)$'
sqla="$shared|^pyathena/(aio/)?sqlalchemy/|^tests/sqlalchemy/|^tests/pyathena/(aio/)?sqlalchemy/|^setup\.cfg$"
spark="$shared|^pyathena/(aio/)?spark/|^tests/pyathena/(aio/)?spark/"
if grep -qE "$sqla" <<< "$files"; then
echo "sqla=true" >> "$GITHUB_OUTPUT"
else
echo "sqla=false" >> "$GITHUB_OUTPUT"
fi
if grep -qE "$spark" <<< "$files"; then
echo "spark=true" >> "$GITHUB_OUTPUT"
else
echo "spark=false" >> "$GITHUB_OUTPUT"
fi

# The three suites create their own schemas and tables, so they run in
# parallel; each is still a separate job for "Re-run failed jobs".
test:
needs: changes
uses: ./.github/workflows/test-suite.yaml
with:
test-type: pyathena
skip-spark: ${{ needs.changes.outputs.spark != 'true' }}
skip-sqla: ${{ needs.changes.outputs.sqla != 'true' }}

test-sqla:
needs: changes
if: needs.changes.outputs.sqla == 'true'
uses: ./.github/workflows/test-suite.yaml
with:
test-type: sqla

test-sqla-async:
needs: changes
if: needs.changes.outputs.sqla == 'true'
uses: ./.github/workflows/test-suite.yaml
with:
test-type: sqla_async
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Preserve existing and upstream copyright notices; do not apply the new original-
- **NEVER** commit directly to `master` — always create a feature branch and PR
- Create PRs as drafts: `gh pr create --draft`
- Use a dedicated worktree for each PR and follow the repository's PR template.
- For PR delivery, follow [development-workflow](.agents/skills/development-workflow/SKILL.md): create the draft, complete two distinct self-reviews, collect an independent review, and check current CI before Ready.
- For PR delivery, follow [development-workflow](.agents/skills/development-workflow/SKILL.md): create the draft, complete two distinct self-reviews, collect an independent review, then mark the PR Ready to start its AWS CI and confirm the result.
- Run [self-review](.agents/skills/self-review/SKILL.md) for behavior and implementation, then [self-review-round-two](.agents/skills/self-review-round-two/SKILL.md) for compatibility, operational consequences, and factual claims.
- Follow [independent-review](.agents/skills/independent-review/SKILL.md) after both rounds; Codex-authored changes use Claude Code `claude-fable-5-1`, profile `max`, effort `high`, never Enterprise.
- Post new review records inline on relevant diff lines using the GitHub review API's `comments` array with an empty review body; record repairs with the review-comment reply API.
Expand Down
22 changes: 22 additions & 0 deletions docs/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,28 @@ Sanitize logs before sharing them.

## GitHub Actions

The Test workflow runs for pull requests that change files other than `docs/` and Markdown.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round two (claims and operational behavior): FINDINGS, repaired

  • Base 2a107adf1f68713e7b7da2e334cc98ca543b7e7d, head 44fab0164369491f4171cb6f55f7509a8f40fb65.
  • Claims checked:
    • The trigger table here matches the changes filter. Verified locally against sample file lists (dialect, compliance tests, Spark, uv.lock, workflow, docs workflow, tests/pyathena/sqlalchemy/).
    • Dispatch runs every suite, so the release procedure (dispatch on master) keeps full coverage.
    • gh pr ready --undo is the command that returns a PR to Draft.
    • The cost figures in the PR body (65% of sampled queries from the compliance suites, about $20 Spark) come from the measurements recorded on Reduce the AWS cost of integration tests without reducing real-service coverage #834.
  • Finding: the PR body said lint previously ran only inside the AWS jobs. The Scripts workflow also runs just lint (via just scripts) for script, workflow, and dependency changes. PR body corrected.
  • Operator view: a Draft PR now makes no AWS calls. A ready non-dialect PR runs 5 jobs instead of 15. Weekly and pre-release runs still cover everything. A regression in a skipped suite surfaces at the next weekly or release run.
  • Deferred: the smaller Python matrix for intermediate pushes (Reduce the AWS cost of integration tests without reducing real-service coverage #834 item 2), pending cost measurement.

It runs the offline checks (`just lint`) on each of them, including Drafts and external forks, and runs the AWS suites as follows:

| Trigger | PyAthena suite | SQLAlchemy tests | Spark tests |
| --- | --- | --- | --- |
| Draft pull request | No | No | No |
| Ready pull request from a branch of this repository | Yes | When related files change | When related files change |
| Weekly schedule and manual dispatch | Yes | Yes | Yes |

The SQLAlchemy tests are the compliance suites and the PyAthena suite's `tests/pyathena/sqlalchemy/` and `tests/pyathena/aio/sqlalchemy/`.
The Spark tests are the PyAthena suite's `tests/pyathena/spark/` and `tests/pyathena/aio/spark/`.
When the SQLAlchemy or Spark tests do not run, the PyAthena suite runs without them.
For the SQLAlchemy tests, the related files are `pyathena/sqlalchemy/`, `pyathena/aio/sqlalchemy/`, `tests/sqlalchemy/`, their PyAthena suite test directories, and `setup.cfg`.
For the Spark tests, they are `pyathena/spark/`, `pyathena/aio/spark/`, and their PyAthena suite test directories.
Changes to `pyproject.toml`, `uv.lock`, `justfile`, or the Test workflows run both.
For a pull request from a branch of this repository that still changes files other than `docs/` and Markdown, marking the Draft ready for review starts the AWS jobs, and converting it back to Draft cancels AWS jobs still running.
To run every suite on a branch, dispatch the workflow:

```bash
gh workflow run test.yaml --ref <branch>
```

Project policy excludes external-fork pull requests from AWS integration CI.
Maintainers do not approve those jobs as a substitute for contributor testing.
Checks without AWS access may still run on a fork pull request.
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ commands =
sqla_async: just test sqla-async
passenv =
TOXENV
PYTEST_ADDOPTS
AWS_*
GITHUB_*
"""
Loading