Skip to content

ci: run tests, lint, bandit, TUI and viewer checks on pull requests - #1309

Open
itzzdev09 wants to merge 3 commits into
usestrix:mainfrom
itzzdev09:ci/pull-request-checks
Open

itzzdev09 wants to merge 3 commits into
usestrix:mainfrom
itzzdev09:ci/pull-request-checks

Conversation

@itzzdev09

@itzzdev09 itzzdev09 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Refs #1260 — the first slice of it. What's in and what's deferred is spelled out below.

Why

The only workflow is Build & Release, which runs on version tags. Nothing runs the documented checks on a pull request, so a change can merge without them ever running in CI.

What this adds

A Checks workflow on pull requests and pushes to main:

job runs mirrors
ruff ruff check . and ruff format --check . make lint, without --fix
bandit bandit -r strix/ -c pyproject.toml make security
pytest the suite on 3.12, 3.13 and 3.14 every version pyproject.toml classifies
tui gofmt -l, go vet, go test -race make tui-lint, make tui-test
viewer npm ci and npm run build make viewer, minus the commit step

Conventions match build-release.yml: every action pinned to a full commit SHA with a version comment, persist-credentials: false on checkout, and read-only permissions. A concurrency group cancels superseded runs on the same branch.

No job changes files, and none is required. That follows the rollout in #1260: land the checks, clear the baseline, then require each check once it's green.

Baseline on main, measured before writing this

I checked what each job reports on current main so the first run holds no surprises:

actionlint 1.7.12 passes on the workflow, and a structural check confirms every uses: is SHA-pinned.

Deliberately deferred

🤖 Generated with Claude Code

The only workflow ran on version tags, so a change could merge without
any of the documented checks ever running in GitHub Actions.

Add a Checks workflow for pull requests and pushes to main:

- ruff check and ruff format --check, using the ruff pinned in uv.lock
- bandit with the project config
- pytest on every supported Python version (3.12, 3.13, 3.14)
- gofmt, go vet and go test -race for the TUI, as in `make tui-lint`
  and `make tui-test`
- a production build of the viewer

No job changes files, and none is marked required: usestrix#1260's rollout is to
land the checks, clear any baseline failure, then require each green
check. Type checking stays in typecheck.yml.

Refs usestrix#1260

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The workflow is safe to merge, but its pytest signal will remain non-actionable until the acknowledged baseline failure is handled, and the type-checking comment should be corrected.

Findings

  1. P2 Known Red Test Baseline
  2. P2 Missing Typecheck Workflow
Fix with agent prompt
### Issue 1
.github/workflows/checks.yml:98
The new matrix runs the full test suite on Python 3.12, 3.13, and 3.14 without excluding or marking `test_resolves_common_bare_model_names`, which the PR identifies as already failing on `main`. Every pull request will therefore receive three failed pytest checks regardless of whether it introduced a regression, making these checks poor regression signals until the known failure is fixed.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
.github/workflows/checks.yml:7
This comment says type checking lives in `typecheck.yml`, but that workflow is not present in the repository and is only proposed in a separate, unmerged change. This incorrectly tells maintainers that pull requests already receive type-check coverage; please describe it as deferred instead.

```suggestion
# Type checking is deferred to a separate workflow.
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • The configured environments and commands align with repository metadata and existing Makefile targets.
  • The pytest job currently has a known red baseline across all three matrix entries.
  • The workflow documentation prematurely refers to a type-check workflow that is not present.

Reviews (1) · Last reviewed commit: "ci: run tests, lint, bandit, TUI and vie..."

run: uv sync --frozen --python ${{ matrix.python-version }}

- name: pytest
run: uv run --python ${{ matrix.python-version }} pytest -q

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.

P2 Known Red Test Baseline

The new matrix runs the full test suite on Python 3.12, 3.13, and 3.14 without excluding or marking test_resolves_common_bare_model_names, which the PR identifies as already failing on main. Every pull request will therefore receive three failed pytest checks regardless of whether it introduced a regression, making these checks poor regression signals until the known failure is fixed.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/checks.yml
Line: 98

Comment:
**Known Red Test Baseline**

The new matrix runs the full test suite on Python 3.12, 3.13, and 3.14 without excluding or marking `test_resolves_common_bare_model_names`, which the PR identifies as already failing on `main`. Every pull request will therefore receive three failed pytest checks regardless of whether it introduced a regression, making these checks poor regression signals until the known failure is fixed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread .github/workflows/checks.yml Outdated
# are required yet: the issue's rollout is to land the checks, clear any
# baseline failure, and only then mark each green check required.
#
# Type checking lives in typecheck.yml, which runs mypy per platform.

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.

P2 Missing Typecheck Workflow

This comment says type checking lives in typecheck.yml, but that workflow is not present in the repository and is only proposed in a separate, unmerged change. This incorrectly tells maintainers that pull requests already receive type-check coverage; please describe it as deferred instead.

Suggested change
# Type checking lives in typecheck.yml, which runs mypy per platform.
# Type checking is deferred to a separate workflow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/checks.yml
Line: 7

Comment:
**Missing Typecheck Workflow**

This comment says type checking lives in `typecheck.yml`, but that workflow is not present in the repository and is only proposed in a separate, unmerged change. This incorrectly tells maintainers that pull requests already receive type-check coverage; please describe it as deferred instead.

```suggestion
# Type checking is deferred to a separate workflow.
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@itzzdev09

Copy link
Copy Markdown
Contributor Author

First CI run is in, and it matches the baseline in the description:

job result
ruff
bandit
tui (gofmt, vet, go test -race)
viewer build
pytest (python 3.12) ❌ 1 failed, 1752 passed
pytest (python 3.13) ❌ 1 failed, 1752 passed
pytest (python 3.14) ❌ 1 failed, 1752 passed

The single pytest failure is the same test on all three versions, and it's the known one:

FAILED tests/test_pricing.py::test_resolves_common_bare_model_names
  AssertionError: assert 'openrouter/x-ai/grok-4.5' == 'xai/grok-4.5'

That's #1288. No other test fails on Linux on any supported Python, so once #1289, #1279 or #1215 lands, all five jobs should be green and ready to mark required.

Two things this run confirmed that I could only argue before:

  • TUI was the one job I couldn't run locally (no Go toolchain), and it passes.
  • 3.14 syncs cleanly from the frozen lockfile and runs the full suite, so the matrix covers every classified version.

itzzdev09 and others added 2 commits September 14, 2026 12:17
… price

LiteLLM's cost map now lists grok-4.5 under xai/, perplexity/xai/ and
openrouter/x-ai/ at identical prices, so alphabetical order resolved the
bare name to openrouter and failed test_resolves_common_bare_model_names.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workflow comment said type checking lives in typecheck.yml, which only exists in the unmerged usestrix#1301, so it implied pull requests already get type-check coverage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@itzzdev09

itzzdev09 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — both are fair points.

Missing typecheck workflow — fixed in 1b8835a. The comment said type checking lives in typecheck.yml, but that only exists in #1301, which isn't merged. It now says type checking isn't covered here and points at #1301.

Known red baseline — correction. I wrote that I was leaving it red, but that was out of date: 76267dc on this branch already fixes the cause of test_resolves_common_bare_model_names (#1288). LiteLLM lists grok-4.5 under xai/, perplexity/xai/ and openrouter/x-ai/ at the same price, and alphabetical order picked OpenRouter; matches are now sorted by path depth first, so the direct provider wins. With it, all eight checks pass, pytest included on 3.12, 3.13 and 3.14. It overlaps with #1289 / #1279 / #1215, so if one of those lands first this commit can be dropped.

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