Regression coverage for real-use bugs, and fixes for their live siblings - #16
Merged
Merged
Conversation
…IL/ACIL scores When every iteration's trainAccuracy is a whole number (e.g. a run that scores a perfect 1 on every iteration), DuckDB infers the column as BIGINT and MAX() returns a JS BigInt, which Hono's c.json cannot serialize. The SCIL and ACIL history pages then fail with a 500. Cast the aggregate to DOUBLE. Co-Authored-By: Claude <noreply@anthropic.com>
…ole number ROUND() of a BIGINT stays BIGINT. When every total_cost_usd in the data set is a whole number (e.g. early-failing runs that report 0), DuckDB infers BIGINT and queryPerTest / queryTestRunDetails return a JS BigInt, which breaks JSON serialization on the per-test analytics and run detail pages. Cast to DOUBLE. Co-Authored-By: Claude <noreply@anthropic.com>
Route unit tests mock the data layer and fake c.json, so a BigInt or a missing parquet file never reached real JSON serialization and both crashes shipped. Extract the API wiring into createApp(dataDir) so an integration test can send real requests through the real Hono app, backed by parquet built from JSONL with whole-number scores, accuracies, and costs, plus an empty data directory. Co-Authored-By: Claude <noreply@anthropic.com>
createSandbox never checked the exit code of the interactive sbx run, so a failed login or template download still printed "Sandbox is ready" and the next eval run failed far from the real cause. Throw a SandboxError instead. The test stubs for sbx run now report exitCode 0, as a real finished process does. Co-Authored-By: Claude <noreply@anthropic.com>
The CLI catches SandboxError separately from SkillwalkerError, and only the SkillwalkerError path was tested. Run a sandbox command against a fake sbx on PATH that fails, and check the output is one Error line with no help text or stack trace. Confirmed the test fails when the SandboxError catch is removed. Co-Authored-By: Claude <noreply@anthropic.com>
CI never built or ran ./build/skillwalker, and the staging code's unit tests stub the Bun runtime, so nothing could catch the compiled binary failing to load the DuckDB native addon. Add a smoke test that runs update-analytics-data with the compiled CLI and queries /api/test-runs from the compiled web server, and a CI job that runs it after make build. Smoke tests are excluded from the unit and make test runs because they need build output. Confirmed both tests fail when build/duckdb.node is missing. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
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.
Adds regression tests for the bugs hit in real use of skillwalker, and test-drives fixes for three similar bugs still in the code. The review of past sessions found that several earlier fixes shipped with tests whose fakes or sample data avoided the exact condition that broke. These tests use the real collaborators instead.
Fixes
trainAccuracyis a whole number, DuckDB infers a BIGINT column, andMAX()returns a JSBigIntthatc.jsoncan't serialize, so/api/sciland/api/acilreturned 500. It's now cast toDOUBLE(run-status.ts).ROUND()of a BIGINT stays BIGINT, so a data set where everytotal_cost_usdis a whole number (e.g.0) broke/api/analytics/per-testand/api/test-runs/:runId. It's now cast toDOUBLE(analytics.ts).sandbox createsaid "ready" whensbx runfailed. The exit code was never checked. It now throwsSandboxError(lifecycle.ts).New tests
packages/web/src/server/app.integration.test.ts: sends real requests through the real Hono app, against Parquet built from real JSONL, with nothing mocked. Covers whole-number scores, accuracies, and costs, plus an empty data directory. Before the two data fixes, four of these routes returned 500. To make this possible, the API wiring moved fromindex.tsintocreateApp(dataDir)inapp.ts, andindex.tsstill adds the embedded static assets.packages/cli/src/compiled-binary.smoke.test.ts(newtest:smokescript and new CI job): runsmake build, thenupdate-analytics-datawith the compiled CLI, and queries/api/test-runsfrom the compiled web server. Both tests fail whenbuild/duckdb.nodeis missing. Smoke tests are excluded frombun run testandmake testbecause they need build output.command-registration.integration.test.ts: a fake failingsbxonPATHchecks that sandbox errors print as oneError:line. This test fails when theSandboxErrorcatch inpackages/cli/index.tsis removed.analytics.integration.test.tsand a unit test inlifecycle.test.tsfor each fix. Each was observed failing before its fix.Not in this PR
SandboxErrorunderSkillwalkerErroris deferred.SkillwalkerErrorlives inexecution, which depends onsandbox-integration, so it first needs a new home lower in the dependency graph.Checks
libduckdb.so)main; the extra one is the file's existing(globalThis as any).Bun.spawnidiom.🤖 Generated with Claude Code