ci: add pre-merge checks (tests, core conformance, built-image smoke) - #96
Merged
Conversation
The repo had no pre-merge checks. notify-ci.yml fires on push to main — after the merge button — and only dispatches to the private CI repo; PR checks were CodeRabbit alone, and branch protection required a review but zero status checks. tests/test_antseed_node.py was consequently red for a month across #63/#68/#69/#70/#71 and every one merged. Three jobs: - tests: pytest against a UTF8 Postgres service (SQL_ASCII makes psycopg return TEXT as bytes, which silently voids settings overrides), submodules checked out so the engine-backed tests are real, node present for the sidecar suite. - core-tests: the Lua policy core's unit + golden conformance vectors. - images: build both images, then smoke the artifact. The image smoke is the one that would have caught #95. A green suite proves the REPO is consistent and says nothing about what COPY put in the image — which is exactly how a control.js requiring ./ids.js shipped without it, died at import, and took every wallet endpoint down with a 502. scripts/check_sidecar_modules.js resolves (never executes) each shipped module's local imports inside the built image; verified to reproduce that failure against the old COPY list.
The router opens a host-store pool at startup, so booting it without Postgres proved only that it can fail to connect. Adds a postgres service to the images job and runs the container with --network host so it can reach it (a bridged container cannot see the runner's localhost). Also drops the '|| true' after docker run: a container that fails to start must fail the job immediately, not fall through to a curl loop that reports the same thing sixty seconds later.
📝 WalkthroughWalkthroughAdded a GitHub Actions workflow for Python, Lua, Docker image, sidecar module, and router health validation. Added a Node.js checker for relative imports in shipped sidecar modules. ChangesCI and image validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Images as Docker images
participant Checker as check_sidecar_modules.js
participant Router as Router container
participant PostgreSQL
CI->>Images: Build main and antseed images
CI->>Checker: Validate sidecar imports
CI->>Router: Start built image
Router->>PostgreSQL: Connect to PostgreSQL
CI->>Router: Poll /healthz
Router-->>CI: Return health status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 101-117: Update the “Router image boots and serves /healthz”
workflow step to start the required sidecar listener with DATABASE_URL before
launching the router, then wait for the sidecar health endpoint before polling
router health. Ensure the sidecar is started from the appropriate
image/configuration and remains available on the expected listener;
alternatively, replace the upstream-dependent /healthz assertion with a
process-level router readiness check if this job is intended to validate startup
only.
In `@scripts/check_sidecar_modules.js`:
- Line 22: Update the LOCAL_IMPORT pattern used by the sidecar import scanner to
match both ./ and ../ relative specifiers, including side-effect import
statements and dynamic import() calls, while preserving existing require() and
from imports.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50d33185-c90e-4976-849b-8abaace6679a
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/check_sidecar_modules.js
Comment on lines
+101
to
+117
| - name: Router image boots and serves /healthz | ||
| run: | | ||
| set -euo pipefail | ||
| docker run -d --name router-ci --network host \ | ||
| -e DATABASE_URL='postgresql://postgres:test@localhost:5432/hoststore_ci' \ | ||
| --entrypoint python unhardcoded:ci \ | ||
| serve.py --config config.live.lua --metrics metrics.live.lua \ | ||
| --default-profile default --host 0.0.0.0 --port 18080 | ||
| for _ in $(seq 1 30); do | ||
| if curl -fsS localhost:18080/healthz >/dev/null 2>&1; then | ||
| echo "router image boots and serves /healthz"; exit 0 | ||
| fi | ||
| sleep 2 | ||
| done | ||
| echo "::error::router image did not become healthy within 60s" | ||
| docker logs router-ci 2>&1 | tail -60 | ||
| exit 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Start the sidecar before polling router health.
auth_proxy.py:1030-1036 forwards /healthz to its upstream and returns HTTP 502 when that request fails. This job only runs unhardcoded-antseed:ci for the one-shot import checker at Lines 91-96. It never starts a sidecar listener. The poll therefore cannot return success even when the router starts correctly.
Start the sidecar with its required DATABASE_URL before starting the router, and wait for the sidecar health endpoint. If this test must validate only router process startup, replace the upstream-dependent /healthz check with a process-level readiness check.
🤖 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 101 - 117, Update the “Router image
boots and serves /healthz” workflow step to start the required sidecar listener
with DATABASE_URL before launching the router, then wait for the sidecar health
endpoint before polling router health. Ensure the sidecar is started from the
appropriate image/configuration and remains available on the expected listener;
alternatively, replace the upstream-dependent /healthz assertion with a
process-level router readiness check if this job is intended to validate startup
only.
| const path = require("path"); | ||
|
|
||
| const DIR = process.argv[2] || "/usr/local/lib/antseed"; | ||
| const LOCAL_IMPORT = /(?:require\(\s*|from\s+)['"](\.\/[^'"]+)['"]/g; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo "== locate file =="
git ls-files | rg '^scripts/check_sidecar_modules\.js$|package\.json$|docker|Dockerfile' || true
echo "== file size and outline =="
wc -l scripts/check_sidecar_modules.js
ast-grep outline scripts/check_sidecar_modules.js || true
echo "== relevant file content =="
cat -n scripts/check_sidecar_modules.js | sed -n '1,180p'
echo "== package scripts/deps hints =="
if [ -f package.json ]; then jq '.scripts, .files, .devDependencies, .dependencies' package.json; fiRepository: genlayerlabs/unhardcoded
Length of output: 1937
🏁 Script executed:
#!/bin/bash
set -eu
echo "== locate file =="
git ls-files | rg '^scripts/check_sidecar_modules\.js$|package\.json$|Dockerfile|docker|\.docker' || true
echo "== file size and outline =="
wc -l scripts/check_sidecar_modules.js
ast-grep outline scripts/check_sidecar_modules.js || true
echo "== relevant file content =="
cat -n scripts/check_sidecar_modules.js | sed -n '1,220p'
echo "== package scripts/files hints =="
if [ -f package.json ]; then jq '.scripts, .files, .devDependencies, .dependencies' package.json; fi
echo "== deterministic regex probe =="
node - <<'JS'
const LOCAL_IMPORT = /(?:require\(\s*|from\s+)['"](\.\/[^'"]+)['"]/g;
const samples = [
'import sidecar from "./sidecar";',
'./sidecar; import "./sidecar";',
'import("./sidecar")',
'import("../sidecar")',
'require("./sidecar")',
'require("../sidecar")',
];
for (const s of samples) {
const copy = s.slice();
const matches = (() => { const m; const out = []; while ((m = LOCAL_IMPORT.exec(copy)) !== null) out.push(m[1]); return out; })();
console.log(JSON.stringify({ input: s, matches }));
}
JSRepository: genlayerlabs/unhardcoded
Length of output: 1937
Match all local import forms.
This scanner only catches ./ specifiers after require( or from, so it misses ../ specifiers, side-effect imports such as import "./sidecar", and dynamic imports such as import("./sidecar"). Include those forms so the check cannot pass while a local import target is missing from the shipped image.
Proposed fix
-const LOCAL_IMPORT = /(?: require\(\s*|from\s+)['"](\.\/[^'"]+)['"]/g;
+const LOCAL_IMPORT = /(?: require\s*\(\s*|import\s*\(\s*|from\s+|import\s*)['"](\.{1,2}\/[^'"]+)['"]/g;🤖 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 `@scripts/check_sidecar_modules.js` at line 22, Update the LOCAL_IMPORT pattern
used by the sidecar import scanner to match both ./ and ../ relative specifiers,
including side-effect import statements and dynamic import() calls, while
preserving existing require() and from imports.
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
2 participants
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.
This repo has no pre-merge checks.
notify-ci.ymltriggerson: push: branches: [main]— after the merge button — and only dispatches to the private CI repo. PR checks are CodeRabbit alone. Branch protection requires one review andrequired_status_checksreturns 404, "not enabled".So the 726-test suite gates nothing. Not theoretical:
tests/test_antseed_node.pysat red for a month across #63, #68, #69, #70 and #71, each PR body noting "1 pre-existing failure", and all five merged — not by overriding a gate, but because there wasn't one.Three jobs
tests— pytest against a UTF8 Postgres service, submodules checked out (the Lua core is a submodule; without it the engine-backed tests import nothing and the suite is quietly meaningless), node present for the sidecar suite.POSTGRES_INITDB_ARGS: --encoding=UTF8is load-bearing: a SQL_ASCII cluster makes psycopg return TEXT asbytes,settings.reload()silently drops every override, and ~26 store-backed tests fail looking like application bugs.core-tests— the policy core's unit + golden conformance vectors.images— builds both images, then smokes the artifact.Why
imagesis the one that mattersA green suite proves the repo is self-consistent and says nothing about what
COPYput in the image. That gap caused a production outage today: #94 addedids.js/queue.jsand madecontrol.jsrequire them,Dockerfile.antseednamed its COPY files individually and wasn't updated, the build succeeded, every test stayed green — and the sidecar's control server died at import in prod (Cannot find module './ids.js'),:8379never bound, and every wallet endpoint 502'd. The only symptom was a Cloudflare error page on the dashboard Deposit button.scripts/check_sidecar_modules.jsresolves — never executes, so a CI run cannot start the control server or touch a wallet — each shipped module's local imports inside the built image. Verified in both directions locally:This generalises past this one bug: missing COPY, wrong entrypoint, a dependency in
requirements-devbut notrequirements— all invisible to any test that runs against source.Ordering. This is expected to be RED until #95 merges, because its
imagesjob correctly catches theids.jsregression still onmain. That failure is the feature demonstrating itself. Merge #95 first.Status of the checks here. GitHub Actions has been in a major outage since 15:22 UTC (incident "Investigating", critical). On the branch these commits came from:
imagespassed twice including the module guard and the healthz boot;testsandcore-testspassed on the prior commit and are unchanged by the commit that followed. The failures since areSet up jobwith no steps, no logs and no annotations — platform, not content.For discussion, not assumed by this PR
Summary by CodeRabbit
Tests
Chores