ci: run viewer/tests in CI (close the viewer-coverage gap) - #403
Merged
Conversation
The viewer is where most recent regressions landed, yet its test suite was not run by CI — three viewer PRs each had to caveat 'viewer tests not in CI, verified locally'. Add a viewer-tests job that runs the full viewer/tests/ suite on push + PR, single-process. Setup mirrors the existing test job (checkout + astral-sh/setup-uv) plus: - actions/setup-node: the JSX behaviour tests transpile the real .jsx through the VENDORED babel-standalone under node's vm. Babel is vendored and the harness only requires node built-ins (fs, vm), so no npm install is needed — just a node binary, which makes those tests RUN instead of skipIf-ing. - a venv with pydantic + pytest: test_portrait_gen's real-subprocess test shells the engine via 'uv run --directory servers/engine --no-project' (a bare interpreter) to prove the null image provider opens no socket. That child resolves the ACTIVE venv, so pydantic (the engine's declared runtime dep) must be present for the engine to import store/imagegen. Installing it here makes the previously-failing test RUN and pass — no source change, no weakened assertion. Single-process (-p no:xdist), no parallel workers — lean for the host. CI-config + env only; no engine/viewer/skill source, no wire contracts.
📝 WalkthroughWalkthroughThe PR adds a new ChangesViewer Tests CI Job
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 23-28: The viewer-tests job currently inherits broad default token
permissions; update the viewer-tests job to declare explicit least-privilege
permissions by adding a permissions block that limits the GITHUB_TOKEN to only
the scopes needed for read-only test execution (for example: set contents: read
and any other minimal read-only scopes required by your test harness) so the job
no longer inherits full default permissions.
- Around line 30-39: The workflow currently uses unpinned tags for actions
(actions/checkout@v4, astral-sh/setup-uv@v5, actions/setup-node@v4) and leaves
checkout credentials persisted; update the viewer-tests job to pin each action
to a specific commit SHA instead of the tag and modify the actions/checkout step
to include persist-credentials: false to avoid leaking tokens to checked-out
workflows. Locate the steps referencing the exact strings "actions/checkout@v4",
"astral-sh/setup-uv@v5", and "actions/setup-node@v4" and replace their uses with
the corresponding full commit SHAs, and add persist-credentials: false under the
checkout step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b352aee1-b669-47fa-8edb-1d222eb6a6d9
📒 Files selected for processing (1)
.github/workflows/ci.yml
Comment on lines
+23
to
+28
| viewer-tests: | ||
| # The viewer is where most recent regressions landed, but its suite was not in | ||
| # CI (three viewer PRs each had to caveat "verified locally"). This job runs the | ||
| # whole viewer/tests/ suite, single-process, on push + PR so viewer regressions | ||
| # are caught in CI like the engine/rules/voice suites. | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Add explicit least-privilege permissions for this job.
This job currently inherits default token permissions, which is broader than needed for read-only test execution.
🔐 Proposed fix
viewer-tests:
+ permissions:
+ contents: read
# The viewer is where most recent regressions landed, but its suite was not in📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| viewer-tests: | |
| # The viewer is where most recent regressions landed, but its suite was not in | |
| # CI (three viewer PRs each had to caveat "verified locally"). This job runs the | |
| # whole viewer/tests/ suite, single-process, on push + PR so viewer regressions | |
| # are caught in CI like the engine/rules/voice suites. | |
| runs-on: ubuntu-latest | |
| viewer-tests: | |
| permissions: | |
| contents: read | |
| # The viewer is where most recent regressions landed, but its suite was not in | |
| # CI (three viewer PRs each had to caveat "verified locally"). This job runs the | |
| # whole viewer/tests/ suite, single-process, on push + PR so viewer regressions | |
| # are caught in CI like the engine/rules/voice suites. | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-57: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 23 - 28, The viewer-tests job
currently inherits broad default token permissions; update the viewer-tests job
to declare explicit least-privilege permissions by adding a permissions block
that limits the GITHUB_TOKEN to only the scopes needed for read-only test
execution (for example: set contents: read and any other minimal read-only
scopes required by your test harness) so the job no longer inherits full default
permissions.
Comment on lines
+30
to
+39
| - uses: actions/checkout@v4 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v5 | ||
| - name: Install Node (for the JSX behaviour harness) | ||
| # A subset of viewer tests transpile the real .jsx through the VENDORED | ||
| # babel-standalone (viewer/openworlds/vendor/) and run it under node's `vm`. | ||
| # Babel is vendored and the harness only requires node built-ins (fs, vm), so | ||
| # NO `npm install` is needed — just a node binary on PATH. Without node these | ||
| # tests `skipIf` themselves; we install it so they actually RUN in CI. | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the referenced section of ci.yml with line numbers
if [ -f .github/workflows/ci.yml ]; then
echo "---- .github/workflows/ci.yml (lines 1-120) ----"
nl -ba .github/workflows/ci.yml | sed -n '1,120p'
else
echo "Missing: .github/workflows/ci.yml"
fi
# 2) Check if persist-credentials is already set anywhere in workflows
echo "---- Searching for persist-credentials in .github/workflows ----"
rg -n "persist-credentials\s*:" .github/workflows || true
# 3) List all action uses across workflows (only 'uses:' lines)
echo "---- Listing all uses: lines across .github/workflows ----"
rg -n "^\s*uses:\s*([^\s]+)" .github/workflows || true
# 4) Narrow to the specific actions mentioned in the comment
echo "---- Searching for actions/checkout, setup-uv, setup-node usage ----"
rg -n "uses:\s*(actions/checkout@|astral-sh/setup-uv@|actions/setup-node@)" .github/workflows || trueRepository: electricsheephq/WorldOS
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f .github/workflows/ci.yml ]; then
echo "---- .github/workflows/ci.yml (first 160 lines with line numbers) ----"
awk 'NR<=160{printf "%d\t%s\n", NR, $0}' .github/workflows/ci.yml
else
echo "Missing: .github/workflows/ci.yml"
fi
echo "---- Searching for persist-credentials in .github/workflows ----"
rg -n "persist-credentials\s*:" .github/workflows || true
echo "---- Listing all uses: lines across .github/workflows (uses only) ----"
rg -n "^\s*uses:\s*" .github/workflows || true
echo "---- Searching for actions/checkout, astral-sh/setup-uv, actions/setup-node usage ----"
rg -n "uses:\s*(actions/checkout@|astral-sh/setup-uv@|actions/setup-node@)" .github/workflows || trueRepository: electricsheephq/WorldOS
Length of output: 4091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show macos-swift.yml with line numbers
if [ -f .github/workflows/macos-swift.yml ]; then
echo "---- .github/workflows/macos-swift.yml (first 120 lines) ----"
awk 'NR<=120{printf "%d\t%s\n", NR, $0}' .github/workflows/macos-swift.yml
fi
# Confirm there is no persist-credentials in that file
if [ -f .github/workflows/macos-swift.yml ]; then
echo "---- persist-credentials occurrences (macos-swift.yml) ----"
rg -n "persist-credentials\s*:" .github/workflows/macos-swift.yml || true
fiRepository: electricsheephq/WorldOS
Length of output: 1243
Harden workflow action supply-chain usage (pin SHAs) and disable checkout credential persistence.
In .github/workflows/ci.yml’s viewer-tests job, actions/checkout@v4, astral-sh/setup-uv@v5, and actions/setup-node@v4 are only pinned to tags, and checkout does not set persist-credentials: false.
🛡️ Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<FULL_LENGTH_COMMIT_SHA>
+ with:
+ persist-credentials: false
- name: Install uv
- uses: astral-sh/setup-uv@v5
+ uses: astral-sh/setup-uv@<FULL_LENGTH_COMMIT_SHA>
- name: Install Node (for the JSX behaviour harness)
- uses: actions/setup-node@v4
+ uses: actions/setup-node@<FULL_LENGTH_COMMIT_SHA>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 30-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 30-30: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 30 - 39, The workflow currently uses
unpinned tags for actions (actions/checkout@v4, astral-sh/setup-uv@v5,
actions/setup-node@v4) and leaves checkout credentials persisted; update the
viewer-tests job to pin each action to a specific commit SHA instead of the tag
and modify the actions/checkout step to include persist-credentials: false to
avoid leaking tokens to checked-out workflows. Locate the steps referencing the
exact strings "actions/checkout@v4", "astral-sh/setup-uv@v5", and
"actions/setup-node@v4" and replace their uses with the corresponding full
commit SHAs, and add persist-credentials: false under the checkout step.
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The viewer is where most recent regressions landed, yet
viewer/tests/was not run by CI. CI ran onlyservers/engine+servers/rules+servers/voicepytest +scripts/license_check.py. Three recent viewer PRs each had to caveat "viewer tests not in CI — verified locally" (e.g. #402), so viewer regressions could slip through. This closes that gap.What this adds
A new
viewer-testsjob in.github/workflows/ci.ymlthat runs the entireviewer/tests/suite on push + PR, single-process. CI-config + env only — no engine/viewer/skill source touched, no wire contracts touched.The setup mirrors the existing
testjob (actions/checkout+astral-sh/setup-uv) and adds exactly what the viewer suite genuinely needs:actions/setup-node@v4— a subset of viewer tests (test_recovery_timing,test_live_narration_stream,test_combat_event_cards) are a real-JSX behaviour harness: they transpile the actual.jsxthrough the vendoredviewer/openworlds/vendor/babel-standalone-7.29.0.min.jsand run it under node'svm. Babel is vendored and the harness onlyrequire()s node built-ins (fs,vm), so nonpm installis needed — just anodebinary. Without node these tests@unittest.skipIfthemselves; installing node makes them actually run in CI.pydantic+pytest— see below.Invocation is single-process:
python -m pytest viewer/tests -q -p no:xdist(no parallel workers — lean by design).How
test_portrait_gen/ deps are handled (the one known local failure)viewer/tests/test_portrait_gen.py::PortraitGenRealSubprocessTests::test_null_default_returns_placeholder_no_networkis the test that fails locally on a bare box withModuleNotFoundError: No module named 'pydantic'. Root cause (verified, not guessed):server._portrait_gen(...), which shells the engine viauv run --directory servers/engine --no-project python -c <snippet>.--no-projectdeliberately gives a bare interpreter (so a wedged gateway can't tie up the viewer thread).import imagegen→import store→from pydantic import ValidationError. With--no-project, pydantic is absent unless it's in the active environment. That's the env gap — not a real defect (the test's purpose is proving the null image provider opens no socket; the engine side already proves the same inservers/engine/tests/test_imagegen.py).Fix (no source change, no weakened assertion): the job creates a venv, exports
VIRTUAL_ENV+ PATH so subsequent steps (and theuv run --no-projectchild) inherit it, and installspydantic— the engine's declared runtime dep (servers/engine/pyproject.toml:pydantic>=2.6). I verified locally thatuv run --no-projectresolves the active venv, so the engine subprocess importsstore/imagegenand the test runs and passes rather than erroring on import. No assertion was relaxed; the test does real work.Green CI evidence (reproduced locally, single-process)
Baseline on
origin/main(bare env):201 ran, 1 failed (test_portrait_gen → ModuleNotFoundError: pydantic), 1 skipped— matches the caveat in the three viewer PRs.With this job's setup (node + active venv w/ pydantic), from repo root:
test_portrait_gen(all 13, incl. the real-subprocess one) runs and passes (pydantic present).test_roster_surface::test_known_canon_pick_resolves_its_ingested_portrait, whichskipTests when there's "no ingested_privateportrait for hartlebury in this checkout."_private/is never committed, so this is correctly skipped in CI too — it's gated on private art, not an env gap, and forcing it to run is out of scope (no public fixture exists).Which viewer tests now run in CI vs. remain uncovered
viewer/tests/suite — all 25 test files, including the Python server-route/read-model tests and the node/babel JSX-behaviour tests andtest_portrait_gen's real-engine-subprocess test.test_roster_surfacecanon-portrait assertion that requires an ingested_privateportrait (private content, never committed).Notes for the reviewer
unittest-style tests under pytest (the suite isunittest.TestCaseclasses); bothpython -m unittest discover -s viewer/testsandpytest viewer/testsagree it's green. I used pytest to match the existingtestjob's convention..venv-vieweris created only on the CI runner and discarded with the job — nothing is committed.Do NOT close on merge — verify on next CI run.
Summary by CodeRabbit