Skip to content

fix: add workflow-timeouts guard and test coverage for issue #420#655

Open
mvillmow wants to merge 6 commits into
mainfrom
420-auto-impl
Open

fix: add workflow-timeouts guard and test coverage for issue #420#655
mvillmow wants to merge 6 commits into
mainfrom
420-auto-impl

Conversation

@mvillmow

@mvillmow mvillmow commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements issue #420: adds a custom CI guard (scripts/check-workflow-timeouts.sh) that verifies every job in .github/workflows/*.yml declares an explicit timeout-minutes value. The JSON-Schema validator cannot enforce this semantic rule, so this guard fills the gap.

Changes:

  1. Prerequisite fixes for existing workflows that were missing timeouts

    • label-sync.ymlsync-labels job: timeout-minutes: 5
    • tech-debt-discovery.ymldiscover job: timeout-minutes: 10
  2. New guard script (scripts/check-workflow-timeouts.sh)

    • Parses YAML with pyyaml to verify semantic rule
    • Emits GitHub Actions ::error:: annotations for violations
    • Skips reusable-workflow callers (which cannot have timeout-minutes)
    • Uses git ls-files to find workflows (respects .gitignore)
  3. Regression tests (tests/test_workflow_timeouts.py)

    • test_every_job_has_timeout_minutes — sanity check on shipped workflows
    • test_script_flags_missing_timeout — negative case, script rejects bad workflows
    • test_script_accepts_clean_workflow — happy path, script passes valid workflows
  4. CI integration (added job to _required.yml)

    • New workflow-timeouts job wired after lint (follows established pattern)
    • Runs the guard as a required check in the PR/main pipeline
    • 3-minute timeout (ample for YAML scanning)
  5. Dev dependency — Added pyyaml = ">=6.0,<7" to pixi dev environment

Test plan

  • All three new regression tests pass (pytest tests/test_workflow_timeouts.py)
  • No regression in existing unit test suite (pytest -m "not integration" → 481 passed)
  • Script accepts all 5 current workflows (scripts/check-workflow-timeouts.sh → exit 0)
  • Lint & pre-commit checks pass (no style issues)
  • Workflow YAML validates against GitHub Actions schema
  • Negative sanity check: script correctly rejects workflows missing timeout-minutes

Closes #420

Co-Authored-By: Claude Haiku 4.5 noreply@anthropic.com

@mvillmow mvillmow left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, complete, well-tested guard for #420; all 17 workflow jobs comply. One minor bool-as-int edge case in the script.

Comment thread scripts/check-workflow-timeouts.sh Outdated

@mvillmow mvillmow left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Guard script + tests + workflow fixes are correct and address prior bool review; but a generated coverage.xml (851 lines, leaks absolute path) was accidentally committed and must be removed.

Comment thread coverage.xml Outdated

@mvillmow mvillmow left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Guard script + tests + workflow timeout fixes are correct and address prior bool review; but a generated coverage.xml (851 lines, leaks absolute path) was committed by accident and must be removed.

Comment thread coverage.xml Outdated
mvillmow and others added 6 commits June 28, 2026 09:36
Both jobs were missing explicit timeout-minutes declarations, which is a
prerequisite for the new workflow-timeouts guard in issue #420. This commit
adds reasonable timeout values:
- sync-labels: 5 minutes (simple label synchronization)
- discover: 10 minutes (repo-wide tech-debt scan)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
…420)

Adds a custom CI check (scripts/check-workflow-timeouts.sh) that verifies
every job in .github/workflows/*.yml declares an explicit timeout-minutes
value. The JSON-Schema validator cannot enforce this semantic rule, so this
guard fills the gap by parsing YAML and asserting each job has a positive
integer timeout.

Key features:
- Parses all .github/workflows/*.yml files with pyyaml
- Emits ::error:: annotations for missing or invalid timeouts
- Skips reusable-workflow callers (which cannot have timeout-minutes)
- Includes three regression tests (positive and negative cases)
- Added pyyaml to pixi.toml dev dependencies

Regression tests:
- test_every_job_has_timeout_minutes: sanity check on all shipped workflows
- test_script_flags_missing_timeout: script correctly rejects bad workflow
- test_script_accepts_clean_workflow: script accepts valid workflows

Closes #420

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
Adds the new workflow-timeouts job to the required CI checks in _required.yml.
This job runs scripts/check-workflow-timeouts.sh to enforce the semantic rule
that every job in .github/workflows/*.yml must declare an explicit
timeout-minutes value.

The job:
- Runs after lint (needs: lint) to follow the established pattern
- Has a 3-minute timeout (ample for YAML parsing)
- Uses standard GitHub Actions annotations for CI feedback
- Skips reusable-workflow callers automatically

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
Python's bool is a subclass of int, so `timeout-minutes: true` would
pass the isinstance check silently as 1. Add explicit bool rejection
to catch malformed YAML values with a clear error message.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
Unit test coverage missing for _required.yml workflow changes

Closes #420

Implemented-By: claude-sonnet-4-6
Co-Authored-By: Claude Code <noreply@anthropic.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
Signed-off-by: mvillmow <4211002+mvillmow@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit test coverage missing for _required.yml workflow changes

1 participant