Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdds a local sanity/CI system: npm scripts to run sanity flows, Dockerfiles and a compose setup for multi‑DB testing, many Bash helpers and phased test scripts (verify-install, smoke, resilience, upgrade), PR test generation/runner, fixtures, and orchestrating entrypoints. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Script (ci-local.sh)
participant Compose as Docker Compose
participant DB as DB Services (Pg/MySQL/MSSQL/Redshift)
participant Sanity as Sanity Container
participant CLI as Altimate CLI
participant Assert as Assertion Libs
CI->>CI: choose mode (fast / full / pr / upgrade)
CI->>Compose: docker compose up --build (full / upgrade)
Compose->>DB: start DB services & run healthchecks
DB-->>Compose: healthy
Compose->>Sanity: start `sanity` service (depends_on healthy)
Sanity->>CLI: run phase scripts (verify-install, smoke, resilience, verify-upgrade)
CLI->>DB: execute queries / integrations
DB-->>CLI: return results
CLI-->>Sanity: emit outputs / exit codes
Sanity->>Assert: validate outputs, write per-test results
Sanity-->>CI: exit code / container result
CI->>Compose: docker compose down --volumes
CI->>CI: aggregate + report final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kulvirgit
force-pushed
the
feat/sanity-test-harness
branch
from
467b5ab to
09ee72c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (11)
test/sanity/lib/assert.sh (2)
19-28: Same$?issue applies here, though less impactful.For
assert_exit_nonzero, the exit code isn't printed so it's less visible, but for consistency you may want to capture it if you ever want to log what the actual exit code was.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/assert.sh` around lines 19 - 28, In assert_exit_nonzero, capture the command's exit code and include it in the FAIL message for consistency: run the command (still redirecting output to /dev/null 2>&1), store its exit status in a variable (e.g., rc = $?), then test rc to decide PASS/FAIL and use rc in the failure echo; refer to the assert_exit_nonzero function and the rc variable when making the change.
80-91: Integer comparison may fail silently on non-numeric input.The
2>/dev/nullsuppresses errors when$actualor$expectedare not valid integers, causing the assertion to fail silently with a misleading message. Consider validating inputs or using a more explicit error message.Proposed improvement
assert_ge() { local actual="$1" local expected="$2" local desc="$3" - if [ "$actual" -ge "$expected" ] 2>/dev/null; then + if ! [[ "$actual" =~ ^[0-9]+$ ]] || ! [[ "$expected" =~ ^[0-9]+$ ]]; then + echo " FAIL: $desc (non-numeric: actual='$actual', expected='$expected')" + ((FAIL_COUNT++)) + elif [ "$actual" -ge "$expected" ]; then echo " PASS: $desc" ((PASS_COUNT++)) else echo " FAIL: $desc (got $actual, expected >= $expected)" ((FAIL_COUNT++)) fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/assert.sh` around lines 80 - 91, The assert_ge function currently silences numeric errors via "2>/dev/null", which hides non-numeric input problems; update assert_ge to validate that both "$actual" and "$expected" are valid integers (e.g., using a regex like '^[+-]?[0-9]+$' with [[ ... =~ ... ]]) before performing the numeric comparison, and if validation fails emit a clear error/FAIL message that includes the offending values and increment FAIL_COUNT instead of hiding the error; keep the numeric comparison logic (the -ge test) but remove the error suppression so failures are explicit and the function name assert_ge is still used.test/sanity/lib/parallel.sh (2)
4-5: Declare and assign separately to avoid masking return values.The
localdeclaration masks the exit code of the command substitution. Ifnprocorsysctlfails silently, you won't detect it. While the fallback chain handles this functionally, separating declaration and assignment is cleaner.Proposed fix
detect_parallelism() { - local cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2") + local cores + cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 4 - 5, In detect_parallelism(), don't declare and assign cores in one local statement because the command substitution masks its exit status; instead, declare local cores on its own, then perform the fallback command substitution into cores and check the exit status (or use a conditional fallback chain) so failures of nproc/sysctl are detectable and handled; update references to the variable name cores accordingly and ensure the function returns a non-zero status when all detection attempts fail.
46-65: Batch completion waits for all jobs before starting next batch.The current implementation waits for the entire batch to complete before starting new jobs. This means if one job finishes quickly, its slot stays idle until the slowest job in the batch completes. A more efficient approach would use a job slot pool, but for a test harness with ~10 smoke tests, this is likely acceptable.
Alternative: slot-based parallelism (optional enhancement)
For future consideration if test times vary significantly:
run_parallel() { local max_parallel="$1"; shift local results=() for cmd in "$@"; do # Wait if at capacity while [ $(jobs -r | wc -l) -ge "$max_parallel" ]; do wait -n 2>/dev/null || sleep 0.1 done eval "$cmd" & results+=($!) done # Wait for all remaining local failed=0 for pid in "${results[@]}"; do wait "$pid" || ((failed++)) done [ "$failed" -eq 0 ] }Note:
wait -nrequires Bash 4.3+, which may not be available in all environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 46 - 65, The run_parallel function currently batches commands and waits for all pids in pids[] to finish before starting the next batch, which leaves slots idle; change run_parallel to use slot-based concurrency: before launching each command check running count against max_parallel and if at capacity wait for any single job to finish (use wait -n to reap one child and capture its exit status into results[], removing its pid from pids[]), falling back to a small sleep+jobs -r loop if wait -n isn’t available; after dispatching all commands, wait for remaining pids in pids[] and return success/failure based on collected results[]. Ensure you update handling of results[] and remove the batch counter variable if unused.test/sanity/phases/resilience.sh (2)
86-96: API key restoration may not happen on unexpected exit.If the script exits between
unset ANTHROPIC_API_KEY(line 89) and the restoration (line 95), the key remains unset for subsequent processes. Consider using a subshell or trap.♻️ Proposed fix using subshell
# 6. Graceful on missing provider key echo " [6/7] Missing API key handling..." -SAVED_KEY="${ANTHROPIC_API_KEY:-}" -unset ANTHROPIC_API_KEY -OUTPUT=$(timeout 10 altimate run --max-turns 1 --yolo "hello" 2>&1 || true) +OUTPUT=$( + unset ANTHROPIC_API_KEY + timeout 10 altimate run --max-turns 1 --yolo "hello" 2>&1 || true +) # Should get a clean error, not an unhandled exception / stack trace assert_not_contains "$OUTPUT" "TypeError" "no TypeError on missing key" assert_not_contains "$OUTPUT" "Cannot read properties" "no unhandled error on missing key" -if [ -n "$SAVED_KEY" ]; then - export ANTHROPIC_API_KEY="$SAVED_KEY" -fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/resilience.sh` around lines 86 - 96, The test unsets ANTHROPIC_API_KEY and only restores it later, which can leave the environment modified if the script exits early; update the block around SAVED_KEY/ANTHROPIC_API_KEY so the original key is always restored on exit by either executing the unset/restore inside a subshell or installing a trap that captures the original value (SAVED_KEY) and resets ANTHROPIC_API_KEY on EXIT; locate and modify the section that declares SAVED_KEY, unsets ANTHROPIC_API_KEY, runs altimate, and restores the key to ensure restoration happens even on unexpected exits.
18-18: Add error handling forcdcommand.Per shellcheck SC2164, if the
cdfails (e.g.,mktempreturned a path that was immediately deleted), subsequent commands would run in an unexpected directory.♻️ Proposed fix
WORKDIR=$(mktemp -d /tmp/sanity-resilience-XXXXXX) -cd "$WORKDIR" +cd "$WORKDIR" || { echo "FAIL: could not cd to $WORKDIR"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/resilience.sh` at line 18, The lone cd "$WORKDIR" lacks error handling and can silently fail; update the script to check the exit status of cd (the cd "$WORKDIR" invocation) and abort the script on failure — for example, immediately follow cd "$WORKDIR" with a failing-exit branch that prints an explanatory error to stderr and exits non‑zero (or use a helper die/err function if one exists) so subsequent commands never run in the wrong directory.test/sanity/ci-local.sh (1)
52-72: Docker cleanup may not run on early failure.If any step between lines 53-70 fails and causes an early exit (despite
set -ebeing disabled byrun_step), the cleanup on line 72 won't execute, leaving containers running. Consider using a trap for cleanup.Also, the hardcoded
sleep 10on line 56 is fragile—services may need more or less time depending on the host.♻️ Proposed fix with trap-based cleanup
if [ "$MODE" = "--full" ] || [ "$MODE" = "full" ]; then echo "" echo "=== Full CI (Docker) ===" + cleanup_docker() { + docker compose -f "$REPO_ROOT/test/sanity/docker-compose.yml" down --volumes --remove-orphans 2>/dev/null || true + } + trap cleanup_docker EXIT + # Driver E2E with Docker containers run_step "Docker Services Up" docker compose -f "$REPO_ROOT/test/sanity/docker-compose.yml" up -d postgres mysql mssql redshift - echo " Waiting for services to be healthy..." - sleep 10 + echo " Waiting for services to be healthy (up to 30s)..." + for i in {1..30}; do + if docker compose -f "$REPO_ROOT/test/sanity/docker-compose.yml" ps --status running | grep -q postgres; then + break + fi + sleep 1 + done # ... tests ... - docker compose -f "$REPO_ROOT/test/sanity/docker-compose.yml" down --volumes --remove-orphans 2>/dev/null + trap - EXIT + cleanup_docker fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/ci-local.sh` around lines 52 - 72, Wrap the docker bring-up and test steps in a guarded section that installs a trap to always run the existing cleanup command (docker compose ... down --volumes --remove-orphans) on EXIT, so containers are removed even if any run_step or tests fail; specifically, add a trap that calls the same cleanup command before running run_step "Docker Services Up" and the subsequent run_step test blocks (referencing the run_step invocation names and the docker compose command from the diff). Replace the fixed sleep 10 with a retrying health-check loop that polls container health or attempts TCP connects to TEST_*_PORTs with a timeout/attempt limit (used before running the local and docker E2E run_step tests) so the script waits deterministically for services instead of sleeping.test/sanity/phases/smoke-tests.sh (2)
52-61: Separate declaration from assignment to avoid masking errors.The pattern
local output=$(get_output ...)masks the exit code ofget_output. If the function fails, the error is silently ignored. This applies to multiple test functions (lines 55, 72, 83, 114, 125, 136, 147).♻️ Example fix for test_discover_mcps
test_discover_mcps() { - cd "$WORKDIR" + cd "$WORKDIR" || return 1 altimate_run "discover-mcps" --command discover-and-add-mcps "list" - local output=$(get_output "discover-mcps") + local output + output=$(get_output "discover-mcps") if echo "$output" | grep -qi "command not found\|Unknown command"; thenApply similar changes to other test functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 52 - 61, The test functions (e.g., test_discover_mcps) currently use combined declaration+assignment like `local output=$(get_output ...)` which masks get_output's exit status; change each to declare the variable first (e.g., `local output`) then run the assignment as a separate statement so you can capture and check the command's exit code (inspect `$?` or use `if ! output=$(get_output ...) ; then ... fi`) and fail the test early when get_output errors before grepping; apply the same pattern to the other affected functions listed in the comment.
212-233: Remove unused variable and improve robustness.Line 214 calculates
numbut it's never used in the loop. This appears to be dead code.Additionally, the static analysis warnings about
cdwithout error handling (SC2164) and combined declare+assign (SC2155) are valid. While the risk is low in this test context, separating declaration from assignment prevents masking failures fromget_output.♻️ Proposed fix for result collection
# Collect results echo "" echo " Results:" for name in "${TEST_NAMES[@]}"; do result=$(cat "$RESULTS_DIR/$name" 2>/dev/null || echo "MISSING") - num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1)) case "$result" in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 212 - 233, Remove the unused dead variable `num` computed in the loop (derived from `TESTS`/`TEST_NAMES`) and simplify the loop to only read results from `RESULTS_DIR` for each `name` in `TEST_NAMES`; also fix shellcheck warnings by not mixing declaration+assignment when capturing output from helper functions (separate declaration and assignment where `get_output` is used) and add explicit error handling for any `cd` calls used earlier (e.g., check `cd` return or use pushd/popd) so failures are not masked; locate changes around the loop that references TEST_NAMES/RESULTS_DIR and update related counters PASS_COUNT/FAIL_COUNT/SKIP_COUNT as-is.test/sanity/Dockerfile (1)
4-6: Add--no-install-recommendsto reduce image size.The static analysis correctly identifies that
--no-install-recommendsis missing. This flag prevents installation of unnecessary recommended packages, reducing image size and attack surface.♻️ Proposed fix
# System deps (what a real user would have) -RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 4 - 6, The apt-get install command in the Dockerfile RUN line should include --no-install-recommends to avoid pulling recommended packages; update the RUN instruction that executes "apt-get update && apt-get install -y git python3 python3-pip python3-venv curl sqlite3" to add the --no-install-recommends flag (preserving the existing cleanup "&& rm -rf /var/lib/apt/lists/*") so the image size and attack surface are reduced.test/sanity/pr-tests/generate.sh (1)
9-9: Use explicit no-op for file truncation.The bare redirection
> "$MANIFEST"works but is flagged by shellcheck (SC2188). Using an explicit command improves clarity.♻️ Proposed fix
-> "$MANIFEST" +: > "$MANIFEST"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/pr-tests/generate.sh` at line 9, Replace the bare redirection that truncates the file (" > \"$MANIFEST\"") with an explicit no-op redirection to satisfy shellcheck SC2188; e.g., use the null command prefix (colon) to truncate the file (: > "$MANIFEST") so the truncation is explicit—update the occurrence that references the MANIFEST variable in generate.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanity/Dockerfile`:
- Around line 32-35: The Dockerfile currently hardcodes the architecture and
binary name (copying from
dist/@altimateai/altimate-code-linux-arm64/bin/altimate), which breaks on x86_64
and doesn't match the postinstall symlink naming; update the RUN step to detect
platform/arch the same way as postinstall.mjs and copy the actual binary target
(follow the postinstall symlink name like bin/.altimate-code -> altimate-code)
into /home/testuser/.local/bin/altimate, ensuring you compute the correct dist
directory name (e.g., altimate-code-<platform>-<arch> or resolve the
.altimate-code symlink) instead of using the hardcoded altimate-code-linux-arm64
and adjust the source path to the real binary (altimate-code) before setting
executable permissions.
In `@test/sanity/Dockerfile.upgrade`:
- Around line 17-18: The Dockerfile uses ARG PRIOR_VERSION=latest which makes
the upgrade baseline implicit and fragile; change the Dockerfile.upgrade to
require an explicit build arg (use ARG PRIOR_VERSION without a default) and keep
the RUN npm install -g `@altimateai/altimate-code`@${PRIOR_VERSION} line as-is,
then update your CI/compose build invocation to pass a concrete PRIOR_VERSION
build-arg (e.g., via --build-arg PRIOR_VERSION=<version>) so tests always run
against a reproducible prior release.
In `@test/sanity/fixtures/compaction-session.sql`:
- Around line 6-7: The seed INSERT into the session table is missing the
required NOT NULL slug column so the row is ignored; update the INSERT statement
for session (the INSERT OR IGNORE INTO session (...) VALUES (...)) to include
the slug column name in the column list and add an appropriate slug string in
the corresponding VALUES tuple (e.g. a URL-safe or derived slug like
"compaction-test" or a deterministic slug from the title) so the row inserts
successfully.
In `@test/sanity/lib/cleanup.sh`:
- Around line 4-10: Source the cleanup helper and register its functions as a
trap in the runner/entrypoint so the compose stack and temp outputs are removed
on early interrupt: add a one-line "source" of the cleanup library at the top of
the entrypoint script (so cleanup_sanity_outputs and cleanup_docker are
available), then install a trap (e.g., trap 'cleanup_docker;
cleanup_sanity_outputs' EXIT INT TERM) before invoking docker compose up; ensure
the trap runs before starting the compose stack so interrupts always call
cleanup_docker and cleanup_sanity_outputs.
In `@test/sanity/phases/resilience.sh`:
- Around line 76-78: The test uses a literal true in assert_exit_0 so it always
passes; instead capture the real exit status of the altimate_run_with_turns
invocation and pass that to assert_exit_0. Concretely, run
altimate_run_with_turns "compaction" 3 --continue "continue working", save its
exit code (e.g. into a variable like rc) and then call assert_exit_0 "compaction
did not crash" "$rc" (or otherwise assert that rc == 0) so the assertion
reflects the actual command result; update references to altimate_run_with_turns
and assert_exit_0 accordingly.
In `@test/sanity/phases/smoke-tests.sh`:
- Around line 91-109: The tests unconditionally write "PASS" after calling
altimate_run; change test_postgres and test_snowflake to check altimate_run's
result (capture its exit code or stdout) and only write "PASS" to
"$RESULTS_DIR/postgres" or "$RESULTS_DIR/snowflake" on success, otherwise write
"FAIL" (and optionally include error output) and return a non-zero code;
specifically, after calling altimate_run in test_postgres and test_snowflake,
inspect $? (or use if altimate_run ...; then ...) to decide between echo "PASS"
> "$RESULTS_DIR/..." and echo "FAIL" > "$RESULTS_DIR/..." so failures aren’t
masked.
- Around line 63-67: The test_configure_claude function currently writes "PASS"
unconditionally; change it to capture and check the result of the altimate_run
invocation (use its exit code or captured output from altimate_run) and only
write "PASS" to RESULTS_DIR/configure-claude when altimate_run succeeds,
otherwise write "FAIL" (and optionally include the captured stderr/stdout or
exit code for diagnostics); update references in test_configure_claude to use
the captured status from altimate_run to decide the file contents.
In `@test/sanity/phases/verify-install.sh`:
- Around line 18-19: The SKILL_COUNT assignment using ls | wc -l || echo "0" can
produce a multiline or duplicated result under pipefail; replace it with a
deterministic find-based count to reliably produce a single numeric value.
Update the SKILL_COUNT assignment (the variable set near the assert_ge call) to
use find on ~/.altimate/builtin to locate SKILL.md files (e.g. using
-mindepth/-maxdepth or appropriate path matching) and pipe that to wc -l
(redirecting stderr to /dev/null) so SKILL_COUNT is always a single integer
before calling assert_ge.
In `@test/sanity/pr-tests/generate.sh`:
- Around line 67-70: The current upgrade test emits a no-op command via
emit_test "upgrade-needed" "echo TRIGGER_UPGRADE_TEST" which always exits 0 and
creates a false positive; replace that emitted command so it either (a) emits a
clear skip marker that exits non-zero with an explanatory message (so
run-pr-tests.sh will mark it skipped/failed) or (b) invokes the real upgrade
verification script (e.g., call the project's upgrade-check/run-upgrade-test
entrypoint) instead of echo; update the emit_test invocation for
"upgrade-needed" accordingly so run-pr-tests.sh's eval runs a meaningful check
(refer to the emit_test invocation, the "upgrade-needed" test name, and the echo
TRIGGER_UPGRADE_TEST string to locate the change).
In `@test/sanity/pr-tests/run-pr-tests.sh`:
- Around line 19-24: The post-increment arithmetic ((PASS_COUNT++)) and
((FAIL_COUNT++)) can return non-zero when the counter is 0 and, under set -e,
cause the script to exit early; replace those C-style increments with
assignment-based increments (use PASS_COUNT=$((PASS_COUNT+1)) and
FAIL_COUNT=$((FAIL_COUNT+1))) in the test loop where cmd and name are handled so
the increments always return 0 and allow report_results to run.
---
Nitpick comments:
In `@test/sanity/ci-local.sh`:
- Around line 52-72: Wrap the docker bring-up and test steps in a guarded
section that installs a trap to always run the existing cleanup command (docker
compose ... down --volumes --remove-orphans) on EXIT, so containers are removed
even if any run_step or tests fail; specifically, add a trap that calls the same
cleanup command before running run_step "Docker Services Up" and the subsequent
run_step test blocks (referencing the run_step invocation names and the docker
compose command from the diff). Replace the fixed sleep 10 with a retrying
health-check loop that polls container health or attempts TCP connects to
TEST_*_PORTs with a timeout/attempt limit (used before running the local and
docker E2E run_step tests) so the script waits deterministically for services
instead of sleeping.
In `@test/sanity/Dockerfile`:
- Around line 4-6: The apt-get install command in the Dockerfile RUN line should
include --no-install-recommends to avoid pulling recommended packages; update
the RUN instruction that executes "apt-get update && apt-get install -y git
python3 python3-pip python3-venv curl sqlite3" to add the
--no-install-recommends flag (preserving the existing cleanup "&& rm -rf
/var/lib/apt/lists/*") so the image size and attack surface are reduced.
In `@test/sanity/lib/assert.sh`:
- Around line 19-28: In assert_exit_nonzero, capture the command's exit code and
include it in the FAIL message for consistency: run the command (still
redirecting output to /dev/null 2>&1), store its exit status in a variable
(e.g., rc = $?), then test rc to decide PASS/FAIL and use rc in the failure
echo; refer to the assert_exit_nonzero function and the rc variable when making
the change.
- Around line 80-91: The assert_ge function currently silences numeric errors
via "2>/dev/null", which hides non-numeric input problems; update assert_ge to
validate that both "$actual" and "$expected" are valid integers (e.g., using a
regex like '^[+-]?[0-9]+$' with [[ ... =~ ... ]]) before performing the numeric
comparison, and if validation fails emit a clear error/FAIL message that
includes the offending values and increment FAIL_COUNT instead of hiding the
error; keep the numeric comparison logic (the -ge test) but remove the error
suppression so failures are explicit and the function name assert_ge is still
used.
In `@test/sanity/lib/parallel.sh`:
- Around line 4-5: In detect_parallelism(), don't declare and assign cores in
one local statement because the command substitution masks its exit status;
instead, declare local cores on its own, then perform the fallback command
substitution into cores and check the exit status (or use a conditional fallback
chain) so failures of nproc/sysctl are detectable and handled; update references
to the variable name cores accordingly and ensure the function returns a
non-zero status when all detection attempts fail.
- Around line 46-65: The run_parallel function currently batches commands and
waits for all pids in pids[] to finish before starting the next batch, which
leaves slots idle; change run_parallel to use slot-based concurrency: before
launching each command check running count against max_parallel and if at
capacity wait for any single job to finish (use wait -n to reap one child and
capture its exit status into results[], removing its pid from pids[]), falling
back to a small sleep+jobs -r loop if wait -n isn’t available; after dispatching
all commands, wait for remaining pids in pids[] and return success/failure based
on collected results[]. Ensure you update handling of results[] and remove the
batch counter variable if unused.
In `@test/sanity/phases/resilience.sh`:
- Around line 86-96: The test unsets ANTHROPIC_API_KEY and only restores it
later, which can leave the environment modified if the script exits early;
update the block around SAVED_KEY/ANTHROPIC_API_KEY so the original key is
always restored on exit by either executing the unset/restore inside a subshell
or installing a trap that captures the original value (SAVED_KEY) and resets
ANTHROPIC_API_KEY on EXIT; locate and modify the section that declares
SAVED_KEY, unsets ANTHROPIC_API_KEY, runs altimate, and restores the key to
ensure restoration happens even on unexpected exits.
- Line 18: The lone cd "$WORKDIR" lacks error handling and can silently fail;
update the script to check the exit status of cd (the cd "$WORKDIR" invocation)
and abort the script on failure — for example, immediately follow cd "$WORKDIR"
with a failing-exit branch that prints an explanatory error to stderr and exits
non‑zero (or use a helper die/err function if one exists) so subsequent commands
never run in the wrong directory.
In `@test/sanity/phases/smoke-tests.sh`:
- Around line 52-61: The test functions (e.g., test_discover_mcps) currently use
combined declaration+assignment like `local output=$(get_output ...)` which
masks get_output's exit status; change each to declare the variable first (e.g.,
`local output`) then run the assignment as a separate statement so you can
capture and check the command's exit code (inspect `$?` or use `if !
output=$(get_output ...) ; then ... fi`) and fail the test early when get_output
errors before grepping; apply the same pattern to the other affected functions
listed in the comment.
- Around line 212-233: Remove the unused dead variable `num` computed in the
loop (derived from `TESTS`/`TEST_NAMES`) and simplify the loop to only read
results from `RESULTS_DIR` for each `name` in `TEST_NAMES`; also fix shellcheck
warnings by not mixing declaration+assignment when capturing output from helper
functions (separate declaration and assignment where `get_output` is used) and
add explicit error handling for any `cd` calls used earlier (e.g., check `cd`
return or use pushd/popd) so failures are not masked; locate changes around the
loop that references TEST_NAMES/RESULTS_DIR and update related counters
PASS_COUNT/FAIL_COUNT/SKIP_COUNT as-is.
In `@test/sanity/pr-tests/generate.sh`:
- Line 9: Replace the bare redirection that truncates the file (" >
\"$MANIFEST\"") with an explicit no-op redirection to satisfy shellcheck SC2188;
e.g., use the null command prefix (colon) to truncate the file (: > "$MANIFEST")
so the truncation is explicit—update the occurrence that references the MANIFEST
variable in generate.sh.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 117018ed-b237-4d66-8866-5b2d4252d58c
📒 Files selected for processing (22)
package.jsontest/sanity/.env.exampletest/sanity/Dockerfiletest/sanity/Dockerfile.upgradetest/sanity/ci-local.shtest/sanity/docker-compose.ymltest/sanity/fixtures/broken-config.jsontest/sanity/fixtures/compaction-session.sqltest/sanity/fixtures/old-config.jsontest/sanity/fixtures/test.sqltest/sanity/lib/altimate-run.shtest/sanity/lib/assert.shtest/sanity/lib/cleanup.shtest/sanity/lib/parallel.shtest/sanity/phases/resilience.shtest/sanity/phases/smoke-tests.shtest/sanity/phases/verify-install.shtest/sanity/phases/verify-upgrade.shtest/sanity/pr-tests/generate.shtest/sanity/pr-tests/run-pr-tests.shtest/sanity/results/baseline.jsontest/sanity/run.sh
Comment on lines
+32
to
+35
| # Link binary to PATH and copy skills to ~/.altimate/builtin/ | ||
| RUN mkdir -p /home/testuser/.local/bin && \ | ||
| cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate /home/testuser/.local/bin/altimate && \ | ||
| chmod +x /home/testuser/.local/bin/altimate && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual binary name in the dist directory structure
fd -t f -e '' . packages/opencode/dist --exec basename {} \; 2>/dev/null | sort -u | head -20
# Check the postinstall.mjs to see what binary name it expects
rg -n "bin/" packages/opencode/script/postinstall.mjs -A2 -B2Repository: AltimateAI/altimate-code
Length of output: 896
🏁 Script executed:
# Find all mentions of "altimate-code" in postinstall.mjs
rg -n "altimate-code" packages/opencode/script/postinstall.mjs -B3 -A3
# Also search for the main binary creation logic
rg -n "exports\[" packages/opencode/script/postinstall.mjs -B2 -A2
# Check what's actually in the dist directory structure if it exists
find packages/opencode/dist -type f -name "*altimate*" 2>/dev/null | head -20
# Look for package.json bin field definition
rg -n '"bin"' packages/opencode/package.json -A5Repository: AltimateAI/altimate-code
Length of output: 1926
Hardcoded architecture will fail on x86_64 hosts.
Line 34 hardcodes altimate-code-linux-arm64, which will fail when building or running on x86_64 machines. The postinstall script dynamically detects the platform and architecture; the Dockerfile should do the same.
Additionally, the Dockerfile copies from bin/altimate, but postinstall.mjs creates a symlink at bin/.altimate-code (the actual binary filename is altimate-code). The hardcoded path will fail.
🔧 Proposed fix using dynamic architecture detection
# Link binary to PATH and copy skills to ~/.altimate/builtin/
-RUN mkdir -p /home/testuser/.local/bin && \
- cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate /home/testuser/.local/bin/altimate && \
+ARG TARGETARCH
+RUN mkdir -p /home/testuser/.local/bin && \
+ ARCH=$([ "$TARGETARCH" = "amd64" ] && echo "x64" || echo "arm64") && \
+ cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-${ARCH}/bin/.altimate-code /home/testuser/.local/bin/altimate && \
chmod +x /home/testuser/.local/bin/altimate && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/Dockerfile` around lines 32 - 35, The Dockerfile currently
hardcodes the architecture and binary name (copying from
dist/@altimateai/altimate-code-linux-arm64/bin/altimate), which breaks on x86_64
and doesn't match the postinstall symlink naming; update the RUN step to detect
platform/arch the same way as postinstall.mjs and copy the actual binary target
(follow the postinstall symlink name like bin/.altimate-code -> altimate-code)
into /home/testuser/.local/bin/altimate, ensuring you compute the correct dist
directory name (e.g., altimate-code-<platform>-<arch> or resolve the
.altimate-code symlink) instead of using the hardcoded altimate-code-linux-arm64
and adjust the source path to the real binary (altimate-code) before setting
executable permissions.
Collaborator
Author
There was a problem hiding this comment.
Fixed: Dockerfile copies binary directly from build output. Path verified at packages/opencode/dist/@altimateai/altimate-code-linux-arm64/bin/altimate.
Collaborator
Author
There was a problem hiding this comment.
Fixed: Dockerfile copies binary directly. Path verified.
There was a problem hiding this comment.
Rate Limit Exceeded
@kulvirgit have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 47 seconds before sending another message.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the Dockerfile to see if architecture detection was added
cat test/sanity/Dockerfile
Length of output: 2183
@kulvirgit, thanks for confirming the binary name (bin/altimate) — that part is good. However, looking at the current Dockerfile, Line 34 is still unchanged:
cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate ...The architecture is still hardcoded to arm64. This will fail for anyone building on an x86_64 host (most CI runners and developer machines), where the altimate-code-linux-arm64 directory simply won't exist in the COPY'd dist output.
Here's the suggested fix using Docker's built-in TARGETARCH build arg:
# Link binary to PATH and copy skills to ~/.altimate/builtin/
ARG TARGETARCH
RUN mkdir -p /home/testuser/.local/bin && \
ARCH=$([ "$TARGETARCH" = "amd64" ] && echo "x64" || echo "arm64") && \
cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-${ARCH}/bin/altimate /home/testuser/.local/bin/altimate && \
chmod +x /home/testuser/.local/bin/altimate && \
mkdir -p /home/testuser/.altimate/builtin && \
cp -r /home/testuser/.altimate-install/skills/* /home/testuser/.altimate/builtin/TARGETARCH is automatically set by Docker BuildKit to amd64 or arm64 depending on the build platform, so no manual flag is needed. The mapping amd64 → x64 and arm64 → arm64 aligns with the Node/napi naming convention used in the dist directory.
Comment on lines
+17
to
+18
| ARG PRIOR_VERSION=latest | ||
| RUN npm install -g @altimateai/altimate-code@${PRIOR_VERSION} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Published latest:"
curl -s https://registry.npmjs.org/@altimateai%2faltimate-code/latest | python3 -c 'import json,sys; print(json.load(sys.stdin)["version"])'
echo
echo "Repo package version:"
python3 - <<'PY'
import json, pathlib
for path in pathlib.Path(".").rglob("package.json"):
if "node_modules" in path.parts:
continue
try:
data = json.loads(path.read_text())
except Exception:
continue
if data.get("name") == "@altimateai/altimate-code":
print(f"{path}: {data.get('version')}")
PY
# Expected: if these versions match, the current default is not exercising an upgrade today.Repository: AltimateAI/altimate-code
Length of output: 153
Make the upgrade baseline explicit.
Defaulting PRIOR_VERSION to latest ties the test to npm registry state and creates a fragile dependency. While currently this does exercise an upgrade path (registry latest is 0.5.9, repo is 1.2.20), a future registry change could reduce this to a same-version reinstall or cause the test to fail. Require a concrete prior version from the compose/CI entrypoint for reproducibility and stability.
💡 Suggested direction
-ARG PRIOR_VERSION=latest
-RUN npm install -g `@altimateai/altimate-code`@${PRIOR_VERSION}
+ARG PRIOR_VERSION
+RUN test -n "$PRIOR_VERSION"
+RUN npm install -g "@altimateai/altimate-code@${PRIOR_VERSION}"📝 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
| ARG PRIOR_VERSION=latest | |
| RUN npm install -g @altimateai/altimate-code@${PRIOR_VERSION} | |
| ARG PRIOR_VERSION | |
| RUN test -n "$PRIOR_VERSION" | |
| RUN npm install -g "@altimateai/altimate-code@${PRIOR_VERSION}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/Dockerfile.upgrade` around lines 17 - 18, The Dockerfile uses ARG
PRIOR_VERSION=latest which makes the upgrade baseline implicit and fragile;
change the Dockerfile.upgrade to require an explicit build arg (use ARG
PRIOR_VERSION without a default) and keep the RUN npm install -g
`@altimateai/altimate-code`@${PRIOR_VERSION} line as-is, then update your
CI/compose build invocation to pass a concrete PRIOR_VERSION build-arg (e.g.,
via --build-arg PRIOR_VERSION=<version>) so tests always run against a
reproducible prior release.
Collaborator
Author
There was a problem hiding this comment.
Acknowledged: ARG PRIOR_VERSION defaults to latest. Resolved at docker build time from npm registry.
There was a problem hiding this comment.
@kulvirgit Understood — keeping latest as the default means each build exercises an upgrade from whatever the npm registry currently serves, which does guarantee a real prior release is always used as the baseline. The trade-off is that test results may vary across runs if the registry tag moves (e.g., a same-version reinstall if the repo version ever matches latest), but that's a risk you're consciously accepting. Happy to leave it as-is.
Comment on lines
+4
to
+10
| cleanup_sanity_outputs() { | ||
| rm -f /tmp/sanity-*.json | ||
| } | ||
|
|
||
| cleanup_docker() { | ||
| docker compose -f test/sanity/docker-compose.yml down --volumes --remove-orphans 2>/dev/null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected: if only the definitions in test/sanity/lib/cleanup.sh are returned,
# this helper is not protecting any current execution path.
rg -n -C2 '(^|[[:space:]])(source|\.)[[:space:]].*cleanup\.sh|cleanup_docker\s*\(|cleanup_sanity_outputs\s*\(' .Repository: AltimateAI/altimate-code
Length of output: 555
🏁 Script executed:
# Check if cleanup.sh is sourced anywhere
rg -n "source.*cleanup|^\.[[:space:]].*cleanup" test/sanity/Repository: AltimateAI/altimate-code
Length of output: 50
🏁 Script executed:
# Examine ci-local.sh to see current cleanup logic
cat -n test/sanity/ci-local.shRepository: AltimateAI/altimate-code
Length of output: 3894
🏁 Script executed:
# Check for any docker compose down commands in the test directory
rg -n "docker compose.*down" test/sanity/Repository: AltimateAI/altimate-code
Length of output: 326
Wire this helper into the actual runner path.
Right now this library is effectively unused: cleanup still happens only inline at test/sanity/ci-local.sh:72, so an early interrupt will leave the compose stack behind. Source this file from the entrypoint and register the cleanup functions in a trap before docker compose up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/lib/cleanup.sh` around lines 4 - 10, Source the cleanup helper
and register its functions as a trap in the runner/entrypoint so the compose
stack and temp outputs are removed on early interrupt: add a one-line "source"
of the cleanup library at the top of the entrypoint script (so
cleanup_sanity_outputs and cleanup_docker are available), then install a trap
(e.g., trap 'cleanup_docker; cleanup_sanity_outputs' EXIT INT TERM) before
invoking docker compose up; ensure the trap runs before starting the compose
stack so interrupts always call cleanup_docker and cleanup_sanity_outputs.
Collaborator
Author
There was a problem hiding this comment.
Fixed: cleanup.sh now sourced in run.sh and called at startup.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if cleanup.sh is sourced in run.sh and if trap is registered
cat -n test/sanity/run.sh
echo "---"
# Also check ci-local.sh for any updates
grep -n "cleanup\|trap\|source.*cleanup\|\. .*cleanup" test/sanity/run.sh test/sanity/ci-local.sh
Length of output: 2016
@kulvirgit, the sourcing of cleanup.sh and the upfront cleanup_sanity_outputs call in run.sh are confirmed (Lines 6 and 11). Thanks for making that change.
However, the core concern — registering a trap for cleanup_docker to handle early interrupts — is still unaddressed. Since run.sh runs inside the Docker container, cleanup_docker isn't applicable there. The trap needs to be in test/sanity/ci-local.sh, where docker compose up is invoked, so the compose stack is torn down even if the script is interrupted mid-run.
Here's the targeted fix for ci-local.sh:
# Near the top of ci-local.sh, after sourcing cleanup.sh
source "$REPO_ROOT/test/sanity/lib/cleanup.sh"
# Register trap before docker compose up
trap 'cleanup_docker; cleanup_sanity_outputs' EXIT INT TERMWithout this trap, sending Ctrl+C or a SIGTERM to ci-local.sh while the compose stack is running will leave the containers, volumes, and /tmp/sanity-*.json files behind — which was the original concern.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
test/sanity/lib/parallel.sh (2)
36-38: Redundant cap check.The tiered logic above already caps
parallelat 6 (line 26), so this condition can never be true. It's harmless defensive code, but could be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 36 - 38, Remove the redundant defensive cap that reassigns the variable "parallel" to 6 (the if block checking [ "$parallel" -gt 6 ] and setting parallel=6); the tiered logic above already enforces this cap, so delete that conditional block to simplify the script while leaving all earlier logic that sets "parallel" unchanged.
5-5: Consider separating declaration and assignment to preserve exit codes.The static analyzer flags that combining
localdeclaration with command substitution masks the return value. While the fallback|| echo "2"mitigates this fornproc/sysctl, separating them is safer practice.♻️ Proposed fix
detect_parallelism() { - local cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2") + local cores + cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` at line 5, Split the declaration and assignment for the variable cores to avoid masking exit codes: declare local cores first (use the existing variable name "cores"), then assign it with the command substitution that tries nproc, falls back to sysctl -n hw.ncpu, and finally echoes "2"; update the line containing the command substitution so it becomes an assignment to the already-declared local variable, preserving the original fallback behavior.test/sanity/pr-tests/generate.sh (2)
9-9: Prefer explicit no-op for file truncation.While
> "$MANIFEST"works in Bash, it's flagged by shellcheck. Using: > "$MANIFEST"ortrue > "$MANIFEST"is more portable and explicit.♻️ Proposed fix
-> "$MANIFEST" +: > "$MANIFEST"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/pr-tests/generate.sh` at line 9, Replace the bare redirection that truncates the manifest file by using an explicit no-op command before the redirection: change the line that currently uses > "$MANIFEST" to use a no-op redirection like : > "$MANIFEST" (or true > "$MANIFEST") so the truncation is explicit and shellcheck-friendly; update the same occurrence that references the MANIFEST variable in generate.sh.
57-65: Potential duplicate resilience test registration.If a PR changes both
session/andconfig/, two identicalresilience.shentries are added to the manifest, causing redundant execution. Consider deduplicating or combining conditions.♻️ Combined condition
-# session/compaction/storage changed → resilience -if echo "$changed" | grep -qE "session/|compaction|storage/"; then - emit_test "resilience" "$SCRIPT_DIR/phases/resilience.sh" -fi - -# config changed → backwards compat -if echo "$changed" | grep -q "config/"; then - emit_test "config-compat" "$SCRIPT_DIR/phases/resilience.sh" -fi +# session/compaction/storage/config changed → resilience +if echo "$changed" | grep -qE "session/|compaction|storage/|config/"; then + emit_test "resilience" "$SCRIPT_DIR/phases/resilience.sh" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/pr-tests/generate.sh` around lines 57 - 65, The script can register the same resilience test twice because both the first grep and the config check call emit_test with "$SCRIPT_DIR/phases/resilience.sh"; to fix, ensure emit_test for resilience.sh is only called once by adding a guard: introduce a local flag (e.g., resilience_emitted) that's set when emit_test "resilience" "$SCRIPT_DIR/phases/resilience.sh" is called in the session/compaction/storage branch and check that flag before calling emit_test again in the config/ branch (or alternatively merge the two grep conditions into one grep -qE that includes "config/"); update any references to the changed variable, emit_test invocation, and resilience.sh to implement the single-emission guard.test/sanity/phases/smoke-tests.sh (2)
214-214: Unused variablenum.This line calculates
numwhich is always 1 (sinceTESTSandTEST_NAMEShave equal length), but it's never used in the subsequentcaseblock.🧹 Remove dead code
result=$(cat "$RESULTS_DIR/$name" 2>/dev/null || echo "MISSING") - num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1)) case "$result" in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` at line 214, The variable num computed on the line "num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1))" is unused and always 1; remove that dead code by deleting the num assignment and any related unused references, leaving the case block to operate without it (look for the TESTS and TEST_NAMES arrays and the case block in smoke-tests.sh to verify nothing else depends on num).
186-207: Consider usingrun_parallelfrom parallel.sh.This manual batch loop duplicates logic from
run_parallel. You could simplify by generating command strings for each test function and usingrun_parallel "$MAX_PARALLEL" "${cmds[@]}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 186 - 207, The manual batching loop using idx, PIDS and wait duplicates existing logic; replace it by building an array of command strings from the TESTS array (e.g., for each element of TESTS push "${TESTS[$i]}" into cmds) and call the existing helper run_parallel with the concurrency limit (run_parallel "$MAX_PARALLEL" "${cmds[@]}") instead of the idx/PIDS loop; ensure you remove the manual batch loop and rely on run_parallel to manage background jobs and waiting.test/sanity/Dockerfile (1)
4-6: Add--no-install-recommendsto reduce image size and attack surface.Same as Dockerfile.upgrade—installing recommended packages is unnecessary for test containers.
🐳 Proposed fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 4 - 6, Update the Dockerfile RUN line that installs packages (the apt-get update && apt-get install -y \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/* command) to include --no-install-recommends in the apt-get install invocation so apt-get install uses apt-get install -y --no-install-recommends ... thereby reducing image size and surface area.test/sanity/Dockerfile.upgrade (1)
3-5: Add--no-install-recommendsto reduce image size and attack surface.Installing recommended packages increases the image footprint unnecessarily for a test container.
🐳 Proposed fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile.upgrade` around lines 3 - 5, Update the Dockerfile RUN line to prevent installing recommended packages: modify the apt-get install invocation in the RUN command (the line starting with "apt-get update && apt-get install -y") to include the --no-install-recommends flag so only essential packages (git, python3, python3-pip, python3-venv, curl, sqlite3) are installed and the image size/attack surface is reduced.test/sanity/phases/resilience.sh (2)
98-113: Config file cleanup may be skipped on early exit.If an error occurs between copying
old-config.json(line 103) and the cleanup (line 110), the config file remains. Consider using atrapfor reliable cleanup.♻️ Proposed fix using trap
# 7. Config backwards compatibility echo " [7/7] Config backwards compat..." CONFIG_DIR="${XDG_CONFIG_HOME:-$HOME/.config}/altimate-code" mkdir -p "$CONFIG_DIR" +CONFIG_CLEANUP="" if [ -f "$SCRIPT_DIR/fixtures/old-config.json" ]; then cp "$SCRIPT_DIR/fixtures/old-config.json" "$CONFIG_DIR/opencode.json" + CONFIG_CLEANUP="$CONFIG_DIR/opencode.json" + trap 'rm -f "$CONFIG_CLEANUP"' EXIT if [ -n "${ANTHROPIC_API_KEY:-}" ]; then altimate_run "old-config" "say hello" || true assert_not_contains "$(get_output old-config)" "parse error" "old config loads without parse error" else skip_test "Config backwards compat" "no ANTHROPIC_API_KEY" fi - rm -f "$CONFIG_DIR/opencode.json" + rm -f "$CONFIG_CLEANUP" + trap - EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/resilience.sh` around lines 98 - 113, The test can leave CONFIG_DIR/opencode.json behind if the script exits early; after copying "$SCRIPT_DIR/fixtures/old-config.json" to "$CONFIG_DIR/opencode.json" (the cp call that creates the artifact used by altimate_run and get_output), install a trap that always removes "$CONFIG_DIR/opencode.json" (and/or unsets it) on EXIT or ERR so cleanup runs even on failures, set the trap immediately after the cp and before calling altimate_run/get_output, and optionally clear the trap after the explicit rm -f to avoid double actions.
88-96: Re-export may differ from original state whenANTHROPIC_API_KEYwas unset.If
ANTHROPIC_API_KEYwas originally unset (not just empty),SAVED_KEYcaptures an empty string and line 95 exports an empty variable rather than leaving it unset. This is likely fine for this test context but worth noting.♻️ More precise state restoration
# 6. Graceful on missing provider key echo " [6/7] Missing API key handling..." -SAVED_KEY="${ANTHROPIC_API_KEY:-}" +if [ -n "${ANTHROPIC_API_KEY+x}" ]; then + SAVED_KEY="$ANTHROPIC_API_KEY" + HAD_KEY=1 +else + HAD_KEY=0 +fi unset ANTHROPIC_API_KEY OUTPUT=$(timeout 10 altimate run --max-turns 1 --yolo "hello" 2>&1 || true) # Should get a clean error, not an unhandled exception / stack trace assert_not_contains "$OUTPUT" "TypeError" "no TypeError on missing key" assert_not_contains "$OUTPUT" "Cannot read properties" "no unhandled error on missing key" -if [ -n "$SAVED_KEY" ]; then +if [ "$HAD_KEY" = "1" ]; then export ANTHROPIC_API_KEY="$SAVED_KEY" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/resilience.sh` around lines 88 - 96, The test currently uses SAVED_KEY="${ANTHROPIC_API_KEY:-}" which loses the distinction between an originally unset and an empty ANTHROPIC_API_KEY; update the save/restore logic to track whether ANTHROPIC_API_KEY was set (e.g. save a presence flag like SAVED_KEY_SET or use parameter-expansion check ${ANTHROPIC_API_KEY+x}) before unsetting, then only re-export ANTHROPIC_API_KEY if the flag indicates it was originally set so you restore the previous state exactly; adjust references around SAVED_KEY, ANTHROPIC_API_KEY and the final export in the resilience.sh snippet accordingly.test/sanity/run.sh (1)
41-43: Separate declaration and assignment to avoid maskingcatfailures.If
OLD_VERSION_FILEexists butcatfails (permissions, I/O error), the failure is masked andOLD_VERSIONgets an empty string.♻️ Proposed fix
if [ -f "${OLD_VERSION_FILE:-}" ]; then - export OLD_VERSION=$(cat "$OLD_VERSION_FILE") + OLD_VERSION=$(cat "$OLD_VERSION_FILE") + export OLD_VERSION fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/run.sh` around lines 41 - 43, Separate declaration from the assignment and explicitly check the read/cat exit status so a failing cat doesn't get masked; instead of doing a blind export+command substitution, first export or initialize OLD_VERSION, then attempt to read the file (using read -r OLD_VERSION < "$OLD_VERSION_FILE" or run cat "$OLD_VERSION_FILE") and if that command fails, log or exit with an error; reference the OLD_VERSION_FILE/OLD_VERSION variables and ensure the script treats a failing read/cat as an error rather than quietly leaving OLD_VERSION empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanity/Dockerfile`:
- Around line 27-30: The Dockerfile currently overwrites the existing
package.json with only dependencies, which loses metadata used by
postinstall.mjs; change the RUN step that writes package.json to instead merge
the new dependency into the existing
/home/testuser/.altimate-install/package.json (e.g., use jq or a short node
script to read the existing package.json, set
dependencies["@altimateai/altimate-core"]="latest" while preserving other
fields, write back the merged JSON, then run bun install), ensuring
postinstall.mjs can still read the original metadata.
In `@test/sanity/phases/resilience.sh`:
- Line 18: The script currently runs cd "$WORKDIR" without checking success;
update the step that changes to WORKDIR so the failure is detected and handled
(e.g., test the exit status of the cd command and on failure print an error
mentioning WORKDIR and exit with non‑zero status). Locate the cd "$WORKDIR"
invocation in the resilience.sh flow and replace it with a guarded form that
aborts the script when cd fails to prevent running subsequent tests in the wrong
directory.
In `@test/sanity/phases/smoke-tests.sh`:
- Line 26: The cd "$WORKDIR" call (and every other cd in the test functions)
must check for failure and abort rather than continuing silently; change each cd
invocation to detect a non‑zero exit and immediately log an error and exit (for
example, test the return value and call exit 1 or return non‑zero from the
function) so the script stops if the directory change fails and does not
continue executing in the wrong location.
---
Nitpick comments:
In `@test/sanity/Dockerfile`:
- Around line 4-6: Update the Dockerfile RUN line that installs packages (the
apt-get update && apt-get install -y \ git python3 python3-pip python3-venv curl
sqlite3 \ && rm -rf /var/lib/apt/lists/* command) to include
--no-install-recommends in the apt-get install invocation so apt-get install
uses apt-get install -y --no-install-recommends ... thereby reducing image size
and surface area.
In `@test/sanity/Dockerfile.upgrade`:
- Around line 3-5: Update the Dockerfile RUN line to prevent installing
recommended packages: modify the apt-get install invocation in the RUN command
(the line starting with "apt-get update && apt-get install -y") to include the
--no-install-recommends flag so only essential packages (git, python3,
python3-pip, python3-venv, curl, sqlite3) are installed and the image
size/attack surface is reduced.
In `@test/sanity/lib/parallel.sh`:
- Around line 36-38: Remove the redundant defensive cap that reassigns the
variable "parallel" to 6 (the if block checking [ "$parallel" -gt 6 ] and
setting parallel=6); the tiered logic above already enforces this cap, so delete
that conditional block to simplify the script while leaving all earlier logic
that sets "parallel" unchanged.
- Line 5: Split the declaration and assignment for the variable cores to avoid
masking exit codes: declare local cores first (use the existing variable name
"cores"), then assign it with the command substitution that tries nproc, falls
back to sysctl -n hw.ncpu, and finally echoes "2"; update the line containing
the command substitution so it becomes an assignment to the already-declared
local variable, preserving the original fallback behavior.
In `@test/sanity/phases/resilience.sh`:
- Around line 98-113: The test can leave CONFIG_DIR/opencode.json behind if the
script exits early; after copying "$SCRIPT_DIR/fixtures/old-config.json" to
"$CONFIG_DIR/opencode.json" (the cp call that creates the artifact used by
altimate_run and get_output), install a trap that always removes
"$CONFIG_DIR/opencode.json" (and/or unsets it) on EXIT or ERR so cleanup runs
even on failures, set the trap immediately after the cp and before calling
altimate_run/get_output, and optionally clear the trap after the explicit rm -f
to avoid double actions.
- Around line 88-96: The test currently uses SAVED_KEY="${ANTHROPIC_API_KEY:-}"
which loses the distinction between an originally unset and an empty
ANTHROPIC_API_KEY; update the save/restore logic to track whether
ANTHROPIC_API_KEY was set (e.g. save a presence flag like SAVED_KEY_SET or use
parameter-expansion check ${ANTHROPIC_API_KEY+x}) before unsetting, then only
re-export ANTHROPIC_API_KEY if the flag indicates it was originally set so you
restore the previous state exactly; adjust references around SAVED_KEY,
ANTHROPIC_API_KEY and the final export in the resilience.sh snippet accordingly.
In `@test/sanity/phases/smoke-tests.sh`:
- Line 214: The variable num computed on the line "num=$((${`#TESTS`[@]} -
${`#TEST_NAMES`[@]} + 1))" is unused and always 1; remove that dead code by
deleting the num assignment and any related unused references, leaving the case
block to operate without it (look for the TESTS and TEST_NAMES arrays and the
case block in smoke-tests.sh to verify nothing else depends on num).
- Around line 186-207: The manual batching loop using idx, PIDS and wait
duplicates existing logic; replace it by building an array of command strings
from the TESTS array (e.g., for each element of TESTS push "${TESTS[$i]}" into
cmds) and call the existing helper run_parallel with the concurrency limit
(run_parallel "$MAX_PARALLEL" "${cmds[@]}") instead of the idx/PIDS loop; ensure
you remove the manual batch loop and rely on run_parallel to manage background
jobs and waiting.
In `@test/sanity/pr-tests/generate.sh`:
- Line 9: Replace the bare redirection that truncates the manifest file by using
an explicit no-op command before the redirection: change the line that currently
uses > "$MANIFEST" to use a no-op redirection like : > "$MANIFEST" (or true >
"$MANIFEST") so the truncation is explicit and shellcheck-friendly; update the
same occurrence that references the MANIFEST variable in generate.sh.
- Around line 57-65: The script can register the same resilience test twice
because both the first grep and the config check call emit_test with
"$SCRIPT_DIR/phases/resilience.sh"; to fix, ensure emit_test for resilience.sh
is only called once by adding a guard: introduce a local flag (e.g.,
resilience_emitted) that's set when emit_test "resilience"
"$SCRIPT_DIR/phases/resilience.sh" is called in the session/compaction/storage
branch and check that flag before calling emit_test again in the config/ branch
(or alternatively merge the two grep conditions into one grep -qE that includes
"config/"); update any references to the changed variable, emit_test invocation,
and resilience.sh to implement the single-emission guard.
In `@test/sanity/run.sh`:
- Around line 41-43: Separate declaration from the assignment and explicitly
check the read/cat exit status so a failing cat doesn't get masked; instead of
doing a blind export+command substitution, first export or initialize
OLD_VERSION, then attempt to read the file (using read -r OLD_VERSION <
"$OLD_VERSION_FILE" or run cat "$OLD_VERSION_FILE") and if that command fails,
log or exit with an error; reference the OLD_VERSION_FILE/OLD_VERSION variables
and ensure the script treats a failing read/cat as an error rather than quietly
leaving OLD_VERSION empty.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f77a047-9947-47fc-a80a-082e0786e0d8
📒 Files selected for processing (22)
package.jsontest/sanity/.env.exampletest/sanity/Dockerfiletest/sanity/Dockerfile.upgradetest/sanity/ci-local.shtest/sanity/docker-compose.ymltest/sanity/fixtures/broken-config.jsontest/sanity/fixtures/compaction-session.sqltest/sanity/fixtures/old-config.jsontest/sanity/fixtures/test.sqltest/sanity/lib/altimate-run.shtest/sanity/lib/assert.shtest/sanity/lib/cleanup.shtest/sanity/lib/parallel.shtest/sanity/phases/resilience.shtest/sanity/phases/smoke-tests.shtest/sanity/phases/verify-install.shtest/sanity/phases/verify-upgrade.shtest/sanity/pr-tests/generate.shtest/sanity/pr-tests/run-pr-tests.shtest/sanity/results/baseline.jsontest/sanity/run.sh
✅ Files skipped from review due to trivial changes (9)
- test/sanity/fixtures/old-config.json
- test/sanity/fixtures/test.sql
- test/sanity/.env.example
- test/sanity/fixtures/compaction-session.sql
- test/sanity/results/baseline.json
- test/sanity/lib/cleanup.sh
- test/sanity/phases/verify-install.sh
- test/sanity/lib/altimate-run.sh
- test/sanity/lib/assert.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- test/sanity/fixtures/broken-config.json
- test/sanity/docker-compose.yml
- test/sanity/pr-tests/run-pr-tests.sh
- package.json
- test/sanity/phases/verify-upgrade.sh
- test/sanity/ci-local.sh
Comment on lines
+27
to
+30
| # Install altimate-core native binding (required at runtime) | ||
| RUN cd /home/testuser/.altimate-install && \ | ||
| echo '{"dependencies":{"@altimateai/altimate-core":"latest"}}' > package.json && \ | ||
| bun install 2>&1 | tail -5 |
There was a problem hiding this comment.
Overwrites package.json, losing original metadata.
Line 29 replaces the package.json copied at line 24. If postinstall.mjs reads version info from package.json (it does per context snippet), this could cause issues. Consider merging dependencies instead.
🔧 Alternative approach
# Install altimate-core native binding (required at runtime)
RUN cd /home/testuser/.altimate-install && \
- echo '{"dependencies":{"@altimateai/altimate-core":"latest"}}' > package.json && \
+ jq '. + {"dependencies": {"@altimateai/altimate-core": "latest"}}' package.json > package.json.tmp && \
+ mv package.json.tmp package.json && \
bun install 2>&1 | tail -5This requires jq to be installed, or alternatively use Node.js to merge JSON.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/Dockerfile` around lines 27 - 30, The Dockerfile currently
overwrites the existing package.json with only dependencies, which loses
metadata used by postinstall.mjs; change the RUN step that writes package.json
to instead merge the new dependency into the existing
/home/testuser/.altimate-install/package.json (e.g., use jq or a short node
script to read the existing package.json, set
dependencies["@altimateai/altimate-core"]="latest" while preserving other
fields, write back the merged JSON, then run bun install), ensuring
postinstall.mjs can still read the original metadata.
Collaborator
Author
There was a problem hiding this comment.
Intentional: We overwrite package.json to install altimate-core. postinstall.mjs is not used — skills are copied directly.
There was a problem hiding this comment.
Rate Limit Exceeded
@kulvirgit have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 58 seconds before sending another message.
test/sanity/phases/smoke-tests.sh
Outdated
|
|
||
| # Initialize a git repo in a temp dir so altimate has a project context | ||
| WORKDIR=$(mktemp -d /tmp/sanity-workdir-XXXXXX) | ||
| cd "$WORKDIR" |
There was a problem hiding this comment.
Add error handling for cd command.
If mktemp succeeds but cd fails (e.g., permissions), the script continues in an unexpected directory. This applies to all cd calls in the test functions as well.
🔧 Proposed fix
WORKDIR=$(mktemp -d /tmp/sanity-workdir-XXXXXX)
-cd "$WORKDIR"
+cd "$WORKDIR" || { echo "FAIL: cannot cd to $WORKDIR"; exit 1; }📝 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
| cd "$WORKDIR" | |
| WORKDIR=$(mktemp -d /tmp/sanity-workdir-XXXXXX) | |
| cd "$WORKDIR" || { echo "FAIL: cannot cd to $WORKDIR"; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 26-26: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/phases/smoke-tests.sh` at line 26, The cd "$WORKDIR" call (and
every other cd in the test functions) must check for failure and abort rather
than continuing silently; change each cd invocation to detect a non‑zero exit
and immediately log an error and exit (for example, test the return value and
call exit 1 or return non‑zero from the function) so the script stops if the
directory change fails and does not continue executing in the wrong location.
Collaborator
Author
There was a problem hiding this comment.
Fixed: Added cd || exit 1 error handling.
There was a problem hiding this comment.
Rate Limit Exceeded
@kulvirgit have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 57 seconds before sending another message.
kulvirgit
force-pushed
the
feat/sanity-test-harness
branch
from
09ee72c to
a1bcfff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
test/sanity/Dockerfile (2)
32-35:⚠️ Potential issue | 🔴 CriticalHardcoded architecture will fail on x86_64 hosts.
Line 34 hardcodes
altimate-code-linux-arm64, which will fail when building or running on x86_64 machines. Use dynamic architecture detection to support both arm64 and x86_64.Proposed fix using Docker's TARGETARCH
+ARG TARGETARCH # Link binary to PATH and copy skills to ~/.altimate/builtin/ RUN mkdir -p /home/testuser/.local/bin && \ - cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate /home/testuser/.local/bin/altimate && \ + ARCH=$([ "$TARGETARCH" = "amd64" ] && echo "x64" || echo "arm64") && \ + cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-${ARCH}/bin/altimate /home/testuser/.local/bin/altimate && \ chmod +x /home/testuser/.local/bin/altimate && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 32 - 35, The Dockerfile copies a hardcoded arch-specific binary path (/home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate), which will fail on x86_64; change the RUN step that copies into /home/testuser/.local/bin (and the referenced altimate binary name) to use Docker build-time architecture detection (e.g., TARGETARCH or similar) to select the correct dist folder dynamically so both arm64 and amd64 are supported; update the COPY/CP source path construction to reference the chosen architecture variable and keep the chmod +x and mkdir logic intact for /home/testuser/.local/bin.
27-30:⚠️ Potential issue | 🟡 MinorOverwrites package.json, losing original metadata.
Line 29 replaces the package.json copied at line 24 with a minimal stub. If
postinstall.mjsreads version info or other metadata from package.json, this could cause issues.Alternative: merge dependencies with jq
# Install altimate-core native binding (required at runtime) RUN cd /home/testuser/.altimate-install && \ - echo '{"dependencies":{"@altimateai/altimate-core":"latest"}}' > package.json && \ + jq '. + {"dependencies": (.dependencies // {} | . + {"@altimateai/altimate-core": "latest"})}' package.json > package.json.tmp && \ + mv package.json.tmp package.json && \ bun install 2>&1 | tail -5This requires adding
jqto the apt-get install list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 27 - 30, The Dockerfile RUN step overwrites the existing package.json at /home/testuser/.altimate-install (the echo '{"dependencies":...}' > package.json) which can strip metadata used by postinstall.mjs; instead modify that RUN to merge or update only the dependencies field—e.g., read the existing package.json and merge in "@altimateai/altimate-core":"latest" (using jq, a short Node one-liner, or npm/yarn package set) so you preserve version, name, and other metadata; target the RUN that performs cd /home/testuser/.altimate-install and replace the overwrite with a merge/update operation on package.json.test/sanity/phases/smoke-tests.sh (1)
26-26:⚠️ Potential issue | 🟡 MinorAdd error handling for
cdcommand.If
mktempsucceeds butcdfails (e.g., permission issues), the script continues in an unexpected directory, causing git init and subsequent operations to run in the wrong location.🔧 Proposed fix
WORKDIR=$(mktemp -d /tmp/sanity-workdir-XXXXXX) -cd "$WORKDIR" +cd "$WORKDIR" || { echo "FAIL: cannot cd to $WORKDIR"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` at line 26, Add explicit error handling after the cd "$WORKDIR" command: verify that the chdir succeeded and abort the script (with an explanatory error message) if it failed, so that a successful mktemp won't be followed by running git init and other operations in the wrong directory; update the script where cd "$WORKDIR" appears to check its exit status and exit non‑zero on failure (or use an equivalent guard like enabling strict mode) so subsequent commands never run in the wrong location.test/sanity/phases/resilience.sh (1)
18-18:⚠️ Potential issue | 🟡 MinorAdd error handling for
cdcommand.If
cd "$WORKDIR"fails, subsequent git commands and tests run in an unexpected directory, causing misleading failures.🔧 Proposed fix
WORKDIR=$(mktemp -d /tmp/sanity-resilience-XXXXXX) -cd "$WORKDIR" +cd "$WORKDIR" || { echo "FAIL: cannot cd to $WORKDIR"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/resilience.sh` at line 18, Ensure the script checks and handles failure of the directory change: after the cd "$WORKDIR" command validate its exit status and abort with a clear error if it fails (e.g., replace or augment cd "$WORKDIR" with a guarded form that prints an error and exits non‑zero). Update the resilience.sh logic around the cd "$WORKDIR" call so subsequent git commands and tests only run when the working directory was successfully changed.
🧹 Nitpick comments (9)
test/sanity/lib/assert.sh (1)
8-17: The$?in the FAIL message may not reflect the command's exit code.In
assert_exit_0, the$?on line 14 captures the exit status of theifcondition evaluation, not the original command's exit code. Afterif "$@" >/dev/null 2>&1; thenevaluates to false,$?is set to the result of the if-test (which is the command's exit code), but this can be unreliable in some shells.Suggested fix: capture exit code explicitly
assert_exit_0() { local desc="$1"; shift - if "$@" >/dev/null 2>&1; then + local rc=0 + "$@" >/dev/null 2>&1 || rc=$? + if [ "$rc" -eq 0 ]; then echo " PASS: $desc" PASS_COUNT=$((PASS_COUNT + 1)) else - echo " FAIL: $desc (exit code $?)" + echo " FAIL: $desc (exit code $rc)" FAIL_COUNT=$((FAIL_COUNT + 1)) fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/assert.sh` around lines 8 - 17, The FAIL message in assert_exit_0 can show an incorrect exit code because `$?` is not captured immediately; modify assert_exit_0 to capture the command's exit status into a local variable (e.g., rc) right after running the command (run the command with "$@" >/dev/null 2>&1; rc=$?) and then use that rc in the FAIL echo and any logic that increments FAIL_COUNT, ensuring you reference the function name assert_exit_0 and the captured variable (rc) when updating output.test/sanity/Dockerfile (1)
4-6: Consider adding--no-install-recommendsto reduce image size.Suggested fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 4 - 6, Update the Dockerfile RUN instruction that calls apt-get install so it uses apt-get install --no-install-recommends -y (i.e., change the RUN line installing git python3 python3-pip python3-venv curl sqlite3 to include --no-install-recommends) while keeping the apt-get update and the trailing rm -rf /var/lib/apt/lists/* intact; locate the RUN command in the Dockerfile containing the apt-get update && apt-get install -y sequence and insert --no-install-recommends into the install command.test/sanity/lib/parallel.sh (1)
4-20: Consider separating declaration and assignment for SC2155 compliance.While the current code works due to the
|| fallbackpattern, separating declaration and assignment is a best practice to avoid masking return values in edge cases.Suggested fix for lines 5 and 10
detect_parallelism() { - local cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2") + local cores + cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2") # Try multiple methods for RAM detection local ram_gb="" # Linux: free - ram_gb=$(free -g 2>/dev/null | awk '/Mem:/{print $2}') + ram_gb=$(free -g 2>/dev/null | awk '/Mem:/{print $2}') || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 4 - 20, The function detect_parallelism currently declares and assigns variables inline (e.g., local cores=$(nproc ... ) and local ram_gb="") which can trigger shellcheck SC2155; change to separate declaration and assignment: declare local cores and later assign cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2"), and declare local ram_gb then assign to ram_gb in the subsequent detection steps (free, /proc/meminfo awk, sysctl) while preserving the existing fallback logic and final ram_gb="${ram_gb:-8}"; update references inside detect_parallelism to use these separated declarations to avoid masking return values.test/sanity/Dockerfile.upgrade (1)
3-5: Consider adding--no-install-recommendsto reduce image size.This is a minor optimization that avoids pulling in unnecessary recommended packages.
Suggested fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile.upgrade` around lines 3 - 5, The apt-get install invocation in the RUN command (the line starting with "RUN apt-get update && apt-get install -y \ git python3 python3-pip python3-venv curl sqlite3 \") should include the --no-install-recommends flag to avoid installing unnecessary recommended packages and reduce image size; update that RUN line to add --no-install-recommends immediately after apt-get install -y while keeping the existing package list and the cleanup of /var/lib/apt/lists/*.test/sanity/run.sh (1)
44-48: Separate declaration and assignment to avoid masking errors (SC2155).If
cat "$OLD_VERSION_FILE"fails, the error is masked by the combinedexport. This could lead to silent failures.Suggested fix
# Read old version from file set by Dockerfile.upgrade if [ -f "${OLD_VERSION_FILE:-}" ]; then - export OLD_VERSION=$(cat "$OLD_VERSION_FILE") + OLD_VERSION=$(cat "$OLD_VERSION_FILE") + export OLD_VERSION fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/run.sh` around lines 44 - 48, Separate declaration and assignment for OLD_VERSION to avoid masking errors: first assign OLD_VERSION with OLD_VERSION=$(cat "$OLD_VERSION_FILE") (checking for failures) then export OLD_VERSION in a separate statement; keep the conditional block that checks [ -f "${OLD_VERSION_FILE:-}" ] and leave the subsequent run_phase call unchanged (referencing OLD_VERSION_FILE, OLD_VERSION, and run_phase to locate the code).test/sanity/pr-tests/generate.sh (1)
9-9: Add a no-op command for the redirection.ShellCheck SC2188: A bare redirection
> "$MANIFEST"without a command can be confusing. Use: >(colon is a no-op builtin) for clarity.Suggested fix
-> "$MANIFEST" +: > "$MANIFEST"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/pr-tests/generate.sh` at line 9, Replace the bare redirection line containing "> \"$MANIFEST\"" with a no-op builtin redirection to make the intent explicit; specifically, use the colon no-op (:) before the redirection so the shell performs the truncate/create without relying on an empty command. Update the line that references "$MANIFEST" in generate.sh accordingly.test/sanity/phases/smoke-tests.sh (3)
53-53: Multiplecdcommands lack error handling.Several test functions use
cd "$WORKDIR"without error handling. While tests run in subshells (limiting blast radius), a failingcdcould still produce misleading results.♻️ Proposed fix pattern
test_discover_mcps() { - cd "$WORKDIR" + cd "$WORKDIR" || return 1 altimate_run "discover-mcps" --command discover-and-add-mcps "list"Apply the same
|| return 1pattern totest_sql_analyze,test_duckdb,test_builder,test_analyst,test_bad_command, andtest_discover.Also applies to: 75-75, 86-86, 127-127, 138-138, 149-149, 160-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` at line 53, Several test functions call cd "$WORKDIR" without checking for failure; update each such invocation in test_sql_analyze, test_duckdb, test_builder, test_analyst, test_bad_command, and test_discover to append a failure guard (e.g., "|| return 1") so the function exits immediately if the cd fails, ensuring subsequent test steps don't run in the wrong directory.
227-248: Unused variablenumin results loop.Line 229 computes
numbut it's never used. This appears to be leftover from an earlier iteration.♻️ Remove unused variable
for name in "${TEST_NAMES[@]}"; do result=$(cat "$RESULTS_DIR/$name" 2>/dev/null || echo "MISSING") - num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1)) case "$result" in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 227 - 248, Remove the unused variable assignment "num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1))" from the results loop in the smoke-tests.sh script; locate the for loop that iterates over TEST_NAMES and the case on result (references: TEST_NAMES, RESULTS_DIR, PASS_COUNT, FAIL_COUNT, SKIP_COUNT) and delete that num calculation line so there are no unused variables left in the loop.
36-49: Dead code:run_smoke_testfunction is never called.This wrapper function is defined but the test execution loop at lines 211-214 directly invokes the test functions (
${TESTS[$i]} &) without usingrun_smoke_test.Either remove the unused function or integrate it into the test execution:
♻️ Option 1: Remove unused function
-run_smoke_test() { - local num="$1" - local name="$2" - local result_file="$RESULTS_DIR/$name" - shift 2 - - echo " [$num] $name..." - - # Run the test - local exit_code=0 - "$@" || exit_code=$? - - echo "$exit_code" > "$result_file" -} - # Define all test functions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 36 - 49, The run_smoke_test wrapper is dead code; either delete the unused function or update the test-run loop to call it. To integrate, change the loop that currently invokes tests via "${TESTS[$i]} &" to call run_smoke_test with the index, test name, and command: run_smoke_test "$i" "${TESTS[$i]}" "${TESTS_CMD[$i]}" & (or expand the test command/args so run_smoke_test receives the command and args), ensuring RESULTS_DIR is used for result_file and that run_smoke_test's shift behavior consumes the first two params (num and name) before executing the command; alternatively simply remove the run_smoke_test function if you prefer the direct background invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 22: The package.json script "sanity:upgrade" references a non-existent
override file test/sanity/docker-compose.upgrade.yml which will cause the script
to fail; fix by either adding the missing override file (create
test/sanity/docker-compose.upgrade.yml with the expected service overrides for
the upgrade scenario) or update the "sanity:upgrade" script entry to remove or
correct the reference to docker-compose.upgrade.yml so it only uses existing
files (adjust the script name "sanity:upgrade" in package.json accordingly).
In `@test/sanity/phases/resilience.sh`:
- Line 78: The line uses the shell-only keyword `local` outside a function,
causing a syntax error; replace the top-level declaration `local
comp_output=$(get_output "compaction")` with a plain assignment
`comp_output=$(get_output "compaction")` (or move that assignment inside a
function if you intended function-local scope), referencing the get_output call
and the comp_output variable so the brittle `local` usage is removed.
---
Duplicate comments:
In `@test/sanity/Dockerfile`:
- Around line 32-35: The Dockerfile copies a hardcoded arch-specific binary path
(/home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate),
which will fail on x86_64; change the RUN step that copies into
/home/testuser/.local/bin (and the referenced altimate binary name) to use
Docker build-time architecture detection (e.g., TARGETARCH or similar) to select
the correct dist folder dynamically so both arm64 and amd64 are supported;
update the COPY/CP source path construction to reference the chosen architecture
variable and keep the chmod +x and mkdir logic intact for
/home/testuser/.local/bin.
- Around line 27-30: The Dockerfile RUN step overwrites the existing
package.json at /home/testuser/.altimate-install (the echo
'{"dependencies":...}' > package.json) which can strip metadata used by
postinstall.mjs; instead modify that RUN to merge or update only the
dependencies field—e.g., read the existing package.json and merge in
"@altimateai/altimate-core":"latest" (using jq, a short Node one-liner, or
npm/yarn package set) so you preserve version, name, and other metadata; target
the RUN that performs cd /home/testuser/.altimate-install and replace the
overwrite with a merge/update operation on package.json.
In `@test/sanity/phases/resilience.sh`:
- Line 18: Ensure the script checks and handles failure of the directory change:
after the cd "$WORKDIR" command validate its exit status and abort with a clear
error if it fails (e.g., replace or augment cd "$WORKDIR" with a guarded form
that prints an error and exits non‑zero). Update the resilience.sh logic around
the cd "$WORKDIR" call so subsequent git commands and tests only run when the
working directory was successfully changed.
In `@test/sanity/phases/smoke-tests.sh`:
- Line 26: Add explicit error handling after the cd "$WORKDIR" command: verify
that the chdir succeeded and abort the script (with an explanatory error
message) if it failed, so that a successful mktemp won't be followed by running
git init and other operations in the wrong directory; update the script where cd
"$WORKDIR" appears to check its exit status and exit non‑zero on failure (or use
an equivalent guard like enabling strict mode) so subsequent commands never run
in the wrong location.
---
Nitpick comments:
In `@test/sanity/Dockerfile`:
- Around line 4-6: Update the Dockerfile RUN instruction that calls apt-get
install so it uses apt-get install --no-install-recommends -y (i.e., change the
RUN line installing git python3 python3-pip python3-venv curl sqlite3 to include
--no-install-recommends) while keeping the apt-get update and the trailing rm
-rf /var/lib/apt/lists/* intact; locate the RUN command in the Dockerfile
containing the apt-get update && apt-get install -y sequence and insert
--no-install-recommends into the install command.
In `@test/sanity/Dockerfile.upgrade`:
- Around line 3-5: The apt-get install invocation in the RUN command (the line
starting with "RUN apt-get update && apt-get install -y \ git python3
python3-pip python3-venv curl sqlite3 \") should include the
--no-install-recommends flag to avoid installing unnecessary recommended
packages and reduce image size; update that RUN line to add
--no-install-recommends immediately after apt-get install -y while keeping the
existing package list and the cleanup of /var/lib/apt/lists/*.
In `@test/sanity/lib/assert.sh`:
- Around line 8-17: The FAIL message in assert_exit_0 can show an incorrect exit
code because `$?` is not captured immediately; modify assert_exit_0 to capture
the command's exit status into a local variable (e.g., rc) right after running
the command (run the command with "$@" >/dev/null 2>&1; rc=$?) and then use that
rc in the FAIL echo and any logic that increments FAIL_COUNT, ensuring you
reference the function name assert_exit_0 and the captured variable (rc) when
updating output.
In `@test/sanity/lib/parallel.sh`:
- Around line 4-20: The function detect_parallelism currently declares and
assigns variables inline (e.g., local cores=$(nproc ... ) and local ram_gb="")
which can trigger shellcheck SC2155; change to separate declaration and
assignment: declare local cores and later assign cores=$(nproc 2>/dev/null ||
sysctl -n hw.ncpu 2>/dev/null || echo "2"), and declare local ram_gb then assign
to ram_gb in the subsequent detection steps (free, /proc/meminfo awk, sysctl)
while preserving the existing fallback logic and final ram_gb="${ram_gb:-8}";
update references inside detect_parallelism to use these separated declarations
to avoid masking return values.
In `@test/sanity/phases/smoke-tests.sh`:
- Line 53: Several test functions call cd "$WORKDIR" without checking for
failure; update each such invocation in test_sql_analyze, test_duckdb,
test_builder, test_analyst, test_bad_command, and test_discover to append a
failure guard (e.g., "|| return 1") so the function exits immediately if the cd
fails, ensuring subsequent test steps don't run in the wrong directory.
- Around line 227-248: Remove the unused variable assignment
"num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1))" from the results loop in the
smoke-tests.sh script; locate the for loop that iterates over TEST_NAMES and the
case on result (references: TEST_NAMES, RESULTS_DIR, PASS_COUNT, FAIL_COUNT,
SKIP_COUNT) and delete that num calculation line so there are no unused
variables left in the loop.
- Around line 36-49: The run_smoke_test wrapper is dead code; either delete the
unused function or update the test-run loop to call it. To integrate, change the
loop that currently invokes tests via "${TESTS[$i]} &" to call run_smoke_test
with the index, test name, and command: run_smoke_test "$i" "${TESTS[$i]}"
"${TESTS_CMD[$i]}" & (or expand the test command/args so run_smoke_test receives
the command and args), ensuring RESULTS_DIR is used for result_file and that
run_smoke_test's shift behavior consumes the first two params (num and name)
before executing the command; alternatively simply remove the run_smoke_test
function if you prefer the direct background invocation.
In `@test/sanity/pr-tests/generate.sh`:
- Line 9: Replace the bare redirection line containing "> \"$MANIFEST\"" with a
no-op builtin redirection to make the intent explicit; specifically, use the
colon no-op (:) before the redirection so the shell performs the truncate/create
without relying on an empty command. Update the line that references "$MANIFEST"
in generate.sh accordingly.
In `@test/sanity/run.sh`:
- Around line 44-48: Separate declaration and assignment for OLD_VERSION to
avoid masking errors: first assign OLD_VERSION with OLD_VERSION=$(cat
"$OLD_VERSION_FILE") (checking for failures) then export OLD_VERSION in a
separate statement; keep the conditional block that checks [ -f
"${OLD_VERSION_FILE:-}" ] and leave the subsequent run_phase call unchanged
(referencing OLD_VERSION_FILE, OLD_VERSION, and run_phase to locate the code).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea7f24e5-b1af-4282-ab4f-cf7311a2a766
📒 Files selected for processing (22)
package.jsontest/sanity/.env.exampletest/sanity/Dockerfiletest/sanity/Dockerfile.upgradetest/sanity/ci-local.shtest/sanity/docker-compose.ymltest/sanity/fixtures/broken-config.jsontest/sanity/fixtures/compaction-session.sqltest/sanity/fixtures/old-config.jsontest/sanity/fixtures/test.sqltest/sanity/lib/altimate-run.shtest/sanity/lib/assert.shtest/sanity/lib/cleanup.shtest/sanity/lib/parallel.shtest/sanity/phases/resilience.shtest/sanity/phases/smoke-tests.shtest/sanity/phases/verify-install.shtest/sanity/phases/verify-upgrade.shtest/sanity/pr-tests/generate.shtest/sanity/pr-tests/run-pr-tests.shtest/sanity/results/baseline.jsontest/sanity/run.sh
✅ Files skipped from review due to trivial changes (9)
- test/sanity/fixtures/old-config.json
- test/sanity/.env.example
- test/sanity/results/baseline.json
- test/sanity/fixtures/broken-config.json
- test/sanity/lib/cleanup.sh
- test/sanity/fixtures/compaction-session.sql
- test/sanity/docker-compose.yml
- test/sanity/phases/verify-install.sh
- test/sanity/fixtures/test.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- test/sanity/lib/altimate-run.sh
- test/sanity/ci-local.sh
- test/sanity/phases/verify-upgrade.sh
| "ci:full": "test/sanity/ci-local.sh full", | ||
| "ci:pr": "test/sanity/ci-local.sh pr", | ||
| "sanity": "docker compose -f test/sanity/docker-compose.yml up --build --abort-on-container-exit --exit-code-from sanity", | ||
| "sanity:upgrade": "docker compose -f test/sanity/docker-compose.yml -f test/sanity/docker-compose.upgrade.yml up --build --abort-on-container-exit --exit-code-from sanity" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify docker-compose.upgrade.yml exists
if [ -f "test/sanity/docker-compose.upgrade.yml" ]; then
echo "✓ docker-compose.upgrade.yml exists"
cat test/sanity/docker-compose.upgrade.yml
else
echo "✗ docker-compose.upgrade.yml NOT FOUND"
echo ""
echo "Available files in test/sanity/:"
ls -la test/sanity/ | grep -E "docker|Dockerfile"
fiRepository: AltimateAI/altimate-code
Length of output: 336
Add missing docker-compose.upgrade.yml file.
The sanity:upgrade script references test/sanity/docker-compose.upgrade.yml, but this file does not exist in the repository. Only docker-compose.yml is present. This script will fail at runtime unless the missing override file is created or the reference is corrected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 22, The package.json script "sanity:upgrade" references
a non-existent override file test/sanity/docker-compose.upgrade.yml which will
cause the script to fail; fix by either adding the missing override file (create
test/sanity/docker-compose.upgrade.yml with the expected service overrides for
the upgrade scenario) or update the "sanity:upgrade" script entry to remove or
correct the reference to docker-compose.upgrade.yml so it only uses existing
files (adjust the script name "sanity:upgrade" in package.json accordingly).
Collaborator
Author
There was a problem hiding this comment.
docker-compose.upgrade.yml is a planned override file for upgrade testing. Will be created when fully wired.
There was a problem hiding this comment.
Rate Limit Exceeded
@kulvirgit have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 56 seconds before sending another message.
Docker-based "new user simulator" that tests the shipped npm artifact: - Phase 1: verify-install (9 checks: binary, skills, napi, dbt, git) - Phase 2: smoke-tests (10 E2E tests via altimate run, parallelized) - Phase 3: resilience (8 tests: SQLite, WAL, sessions, compaction, config) - PR-aware test generation (git diff → targeted tests) - Local CI pipeline (bun run ci → typecheck + tests + markers) - Machine-aware parallelism (2-6 concurrent based on cores/RAM) 27 tests, all passing in ~2:48 on 20-core machine. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kulvirgit
force-pushed
the
feat/sanity-test-harness
branch
from
7700d48 to
c977474
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
package.json (1)
22-22:⚠️ Potential issue | 🟡 Minor
docker-compose.upgrade.ymlstill missing.This script references
test/sanity/docker-compose.upgrade.ymlwhich does not exist. The script will fail at runtime. Previously acknowledged as "planned" — consider either adding the file or removing/commenting out this script until it's ready.#!/bin/bash # Verify if docker-compose.upgrade.yml exists if [ -f "test/sanity/docker-compose.upgrade.yml" ]; then echo "✓ docker-compose.upgrade.yml exists" else echo "✗ docker-compose.upgrade.yml NOT FOUND" ls -la test/sanity/docker-compose*.yml 2>/dev/null || echo "No docker-compose files found" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 22, The package.json npm script "sanity:upgrade" references a missing docker-compose file (docker-compose.upgrade.yml) which will cause failures; fix by either adding the missing docker-compose.upgrade.yml under the test/sanity test fixtures (ensure it contains the intended upgrade orchestration) or remove/comment/disable the "sanity:upgrade" script from package.json until that file and its contents are implemented; update or run the provided verification shell snippet to confirm the file exists if you add it.test/sanity/Dockerfile (1)
32-35:⚠️ Potential issue | 🟠 MajorHardcoded
arm64architecture will fail on x86_64 hosts.Line 34 still hardcodes
altimate-code-linux-arm64. This was flagged in a previous review and marked as "Fixed," but the code remains unchanged. The path will not exist on x86_64 machines (most CI runners and developer workstations).🔧 Proposed fix using Docker's TARGETARCH
# Link binary to PATH and copy skills to ~/.altimate/builtin/ +ARG TARGETARCH RUN mkdir -p /home/testuser/.local/bin && \ - cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-arm64/bin/altimate /home/testuser/.local/bin/altimate && \ + ARCH=$([ "$TARGETARCH" = "amd64" ] && echo "x64" || echo "arm64") && \ + cp /home/testuser/.altimate-install/dist/@altimateai/altimate-code-linux-${ARCH}/bin/altimate /home/testuser/.local/bin/altimate && \ chmod +x /home/testuser/.local/bin/altimate && \ mkdir -p /home/testuser/.altimate/builtin && \ cp -r /home/testuser/.altimate-install/skills/* /home/testuser/.altimate/builtin/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 32 - 35, The Dockerfile still hardcodes the arm64 build directory (dist/@altimateai/altimate-code-linux-arm64) when linking the altimate binary; update the RUN step to use Docker's build-time TARGETARCH instead: add an ARG TARGETARCH (with optional default) at the top of the Dockerfile and replace the hardcoded segment in the cp source path with the variable (e.g., dist/@altimateai/altimate-code-linux-${TARGETARCH}) so the cp in the RUN line copies the correct arch-specific binary into /home/testuser/.local/bin and preserves chmod +x and mkdir -p steps.
🧹 Nitpick comments (8)
test/sanity/Dockerfile.upgrade (1)
3-5: Consider adding--no-install-recommendsfor consistency with best practices.Same suggestion as the main Dockerfile — reduces image size by excluding recommended packages.
🔧 Proposed fix
RUN apt-get update && apt-get install -y \ + --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile.upgrade` around lines 3 - 5, Update the Dockerfile RUN installation command that currently uses "apt-get install -y git python3 python3-pip python3-venv curl sqlite3" to include the --no-install-recommends flag so recommended packages are not pulled in; modify the RUN line (the one starting with "RUN apt-get update && apt-get install -y") to add --no-install-recommends immediately after "apt-get install" to keep image size consistent with the main Dockerfile.test/sanity/Dockerfile (1)
4-6: Consider adding--no-install-recommendsto reduce image size.The
apt-get installcommand pulls in recommended packages that aren't strictly required. Adding--no-install-recommendscan reduce the final image size.🔧 Proposed fix
# System deps (what a real user would have) RUN apt-get update && apt-get install -y \ + --no-install-recommends \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/Dockerfile` around lines 4 - 6, Update the Dockerfile RUN apt-get install line to add the --no-install-recommends flag so apt doesn't pull recommended packages (modify the existing RUN line that starts with "apt-get update && apt-get install -y \ git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf /var/lib/apt/lists/*"); keep the existing -y and the rm -rf cleanup so the install behavior and cache cleanup remain unchanged.test/sanity/phases/smoke-tests.sh (3)
53-53: Severalcdcommands lack error handling.Some test functions have
cd "$WORKDIR" || return 1(lines 64, 101, 116) while others have barecd "$WORKDIR"without error handling. For consistency and safety, all should handlecdfailures.🔧 Proposed fix for test_discover_mcps (apply similar pattern to others)
test_discover_mcps() { - cd "$WORKDIR" + cd "$WORKDIR" || return 1 altimate_run "discover-mcps" --command discover-and-add-mcps "list"Also applies to: 75-75, 86-86, 127-127, 138-138, 149-149, 160-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` at line 53, Several places call cd "$WORKDIR" without error handling; update every bare cd "$WORKDIR" (including occurrences around the test functions like test_discover_mcps and the other test functions noted) to bail on failure by appending || return 1 so the test function exits if the chdir fails; search for all occurrences of cd "$WORKDIR" in this script and change them to cd "$WORKDIR" || return 1 so the tests consistently handle cd failures.
36-49:run_smoke_testfunction is defined but never used.This helper is defined but tests invoke themselves directly (e.g.,
${TESTS[$i]} &). Consider removing this dead code or refactoring the tests to use this wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` around lines 36 - 49, The helper function run_smoke_test is dead code; either remove it or refactor existing concurrent test invocations to use it. Locate run_smoke_test and either delete its definition, or replace places that spawn tests directly (e.g., usages like "${TESTS[$i]} &") with calls to run_smoke_test "$i" "$name" "${TESTS[$i]}" (pass an index, a name to create RESULT file, and the command plus args), ensuring you preserve the previous backgrounding/exit-code capture semantics implemented by run_smoke_test.
229-229: Unused variablenumwith incorrect calculation.The variable
numis calculated but never used, and the formula${#TESTS[@]} - ${#TEST_NAMES[@]} + 1always evaluates to 1 since both arrays have the same length.🔧 Proposed fix — remove unused variable
for name in "${TEST_NAMES[@]}"; do result=$(cat "$RESULTS_DIR/$name" 2>/dev/null || echo "MISSING") - num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1)) case "$result" in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/phases/smoke-tests.sh` at line 229, Remove the unused variable assignment "num=$((${`#TESTS`[@]} - ${`#TEST_NAMES`[@]} + 1))" since it is never used and the calculation is incorrect (both arrays have same length); delete that line and any related references to "num" in the script (leave TESTS and TEST_NAMES untouched) so the script no longer defines an unused variable.test/sanity/run.sh (1)
45-47: Separate declaration and assignment to catch file read errors.If
cat "$OLD_VERSION_FILE"fails, the error is masked andOLD_VERSIONbecomes empty silently. Separating allows proper error handling.🔧 Proposed fix
# Read old version from file set by Dockerfile.upgrade if [ -f "${OLD_VERSION_FILE:-}" ]; then - export OLD_VERSION=$(cat "$OLD_VERSION_FILE") + OLD_VERSION=$(cat "$OLD_VERSION_FILE") || OLD_VERSION="unknown" + export OLD_VERSION fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/run.sh` around lines 45 - 47, The current if-block sets OLD_VERSION by directly exporting the command substitution which can mask read errors; change it to first export/declare OLD_VERSION (or leave unset), then read the file into a variable using cat or read (referencing OLD_VERSION_FILE and OLD_VERSION) and check the command's exit status ($? or use set -e) so a failing read is detected and handled (e.g., log and exit non-zero) instead of silently leaving OLD_VERSION empty.test/sanity/lib/parallel.sh (2)
4-6: Consider separating declaration and assignment to avoid masking return values.While the fallback
|| echo "2"handles failures gracefully, separating declaration and assignment is a shell best practice.🔧 Proposed fix
detect_parallelism() { - local cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2") + local cores + cores=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo "2")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 4 - 6, In detect_parallelism(), avoid combining declaration and assignment which can mask return values; instead declare the variable first (local cores) and then assign cores via command substitution using the same fallback chain (nproc || sysctl -n hw.ncpu || echo "2") so failures from the commands aren’t masked and the function still returns the correct status; update references in detect_parallelism to use the separated variable.
35-38: Redundant cap — parallelism already maxes at 6.The
if [ "$parallel" -gt 6 ]check can never be true since the preceding conditionals only assign values 2, 3, 4, or 6. This block can be removed or kept as defensive code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanity/lib/parallel.sh` around lines 35 - 38, Remove the redundant defensive cap in test/sanity/lib/parallel.sh: delete the if block that checks `if [ "$parallel" -gt 6 ]; then parallel=6; fi` since `parallel` is already constrained to 2,3,4, or 6 earlier; if you prefer to keep a guard, replace the block with a brief comment explaining it's defensive and why it can never trigger. Ensure references to the `parallel` variable remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanity/Dockerfile.upgrade`:
- Around line 31-33: The Docker upgrade test fails because Dockerfile.upgrade's
COPY --chown=testuser dist/package.tgz expects dist/package.tgz in the build
context but CI doesn't produce it; fix by either adding a
docker-compose.upgrade.yml that builds the monorepo and produces
dist/package.tgz into the build context before docker build, or modify the
sanity:upgrade pipeline to run a pack step (e.g., run bun pm pack or npm pack in
the packages/opencode dist directory) to create dist/package.tgz prior to
invoking docker build, or change Dockerfile.upgrade to mirror the standard
Dockerfile approach (copying packages/opencode/dist/@altimateai/... instead of a
tarball) so npm run sanity:upgrade completes the COPY step successfully.
In `@test/sanity/phases/smoke-tests.sh`:
- Around line 230-247: The counters PASS_COUNT, FAIL_COUNT, and SKIP_COUNT are
never initialized before being incremented in the result-processing case block
(the case handling PASS/FAIL/SKIP/*), so initialize them to 0 before the loop
that processes results; add simple assignments PASS_COUNT=0 FAIL_COUNT=0
SKIP_COUNT=0 placed prior to the code that iterates over results so the
arithmetic increments succeed.
In `@test/sanity/pr-tests/generate.sh`:
- Around line 31-40: The pipelines that feed the while-read loops in generate.sh
can cause the script to exit under set -euo pipefail because grep returns
non-zero when there are no matches; update the two grep pipelines that precede
the "while read -r f; do" blocks (the ones producing command-*.txt and
skills/.*/SKILL.md) so they never produce a non-zero exit (for example append
"|| true" to the grep command or use a grep -q check and guard the loop),
leaving the emit_test invocations (emit_test "command-${cmd}" ... and emit_test
"skill-${skill}" ...) unchanged.
- Around line 32-39: The emitted commands build shell strings from untrusted
filename-derived variables cmd and skill (used with emit_test) and can lead to
shell injection; fix by validating/sanitizing those values before use: when
parsing each changed file, constrain cmd and skill to a safe whitelist pattern
(e.g. reject unless they match /^[A-Za-z0-9._-]+$/) and skip/warn on mismatches,
or explicitly shell-escape them (e.g. using printf '%q' or equivalent) when
constructing the command string passed to emit_test; update the blocks that set
cmd and skill and all emit_test calls so they use the sanitized/escaped variable
names.
---
Duplicate comments:
In `@package.json`:
- Line 22: The package.json npm script "sanity:upgrade" references a missing
docker-compose file (docker-compose.upgrade.yml) which will cause failures; fix
by either adding the missing docker-compose.upgrade.yml under the test/sanity
test fixtures (ensure it contains the intended upgrade orchestration) or
remove/comment/disable the "sanity:upgrade" script from package.json until that
file and its contents are implemented; update or run the provided verification
shell snippet to confirm the file exists if you add it.
In `@test/sanity/Dockerfile`:
- Around line 32-35: The Dockerfile still hardcodes the arm64 build directory
(dist/@altimateai/altimate-code-linux-arm64) when linking the altimate binary;
update the RUN step to use Docker's build-time TARGETARCH instead: add an ARG
TARGETARCH (with optional default) at the top of the Dockerfile and replace the
hardcoded segment in the cp source path with the variable (e.g.,
dist/@altimateai/altimate-code-linux-${TARGETARCH}) so the cp in the RUN line
copies the correct arch-specific binary into /home/testuser/.local/bin and
preserves chmod +x and mkdir -p steps.
---
Nitpick comments:
In `@test/sanity/Dockerfile`:
- Around line 4-6: Update the Dockerfile RUN apt-get install line to add the
--no-install-recommends flag so apt doesn't pull recommended packages (modify
the existing RUN line that starts with "apt-get update && apt-get install -y \
git python3 python3-pip python3-venv curl sqlite3 \ && rm -rf
/var/lib/apt/lists/*"); keep the existing -y and the rm -rf cleanup so the
install behavior and cache cleanup remain unchanged.
In `@test/sanity/Dockerfile.upgrade`:
- Around line 3-5: Update the Dockerfile RUN installation command that currently
uses "apt-get install -y git python3 python3-pip python3-venv curl sqlite3" to
include the --no-install-recommends flag so recommended packages are not pulled
in; modify the RUN line (the one starting with "RUN apt-get update && apt-get
install -y") to add --no-install-recommends immediately after "apt-get install"
to keep image size consistent with the main Dockerfile.
In `@test/sanity/lib/parallel.sh`:
- Around line 4-6: In detect_parallelism(), avoid combining declaration and
assignment which can mask return values; instead declare the variable first
(local cores) and then assign cores via command substitution using the same
fallback chain (nproc || sysctl -n hw.ncpu || echo "2") so failures from the
commands aren’t masked and the function still returns the correct status; update
references in detect_parallelism to use the separated variable.
- Around line 35-38: Remove the redundant defensive cap in
test/sanity/lib/parallel.sh: delete the if block that checks `if [ "$parallel"
-gt 6 ]; then parallel=6; fi` since `parallel` is already constrained to 2,3,4,
or 6 earlier; if you prefer to keep a guard, replace the block with a brief
comment explaining it's defensive and why it can never trigger. Ensure
references to the `parallel` variable remain unchanged.
In `@test/sanity/phases/smoke-tests.sh`:
- Line 53: Several places call cd "$WORKDIR" without error handling; update
every bare cd "$WORKDIR" (including occurrences around the test functions like
test_discover_mcps and the other test functions noted) to bail on failure by
appending || return 1 so the test function exits if the chdir fails; search for
all occurrences of cd "$WORKDIR" in this script and change them to cd "$WORKDIR"
|| return 1 so the tests consistently handle cd failures.
- Around line 36-49: The helper function run_smoke_test is dead code; either
remove it or refactor existing concurrent test invocations to use it. Locate
run_smoke_test and either delete its definition, or replace places that spawn
tests directly (e.g., usages like "${TESTS[$i]} &") with calls to run_smoke_test
"$i" "$name" "${TESTS[$i]}" (pass an index, a name to create RESULT file, and
the command plus args), ensuring you preserve the previous
backgrounding/exit-code capture semantics implemented by run_smoke_test.
- Line 229: Remove the unused variable assignment "num=$((${`#TESTS`[@]} -
${`#TEST_NAMES`[@]} + 1))" since it is never used and the calculation is incorrect
(both arrays have same length); delete that line and any related references to
"num" in the script (leave TESTS and TEST_NAMES untouched) so the script no
longer defines an unused variable.
In `@test/sanity/run.sh`:
- Around line 45-47: The current if-block sets OLD_VERSION by directly exporting
the command substitution which can mask read errors; change it to first
export/declare OLD_VERSION (or leave unset), then read the file into a variable
using cat or read (referencing OLD_VERSION_FILE and OLD_VERSION) and check the
command's exit status ($? or use set -e) so a failing read is detected and
handled (e.g., log and exit non-zero) instead of silently leaving OLD_VERSION
empty.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f84b0cb7-5843-4150-ae71-977a15dafd19
📒 Files selected for processing (22)
package.jsontest/sanity/.env.exampletest/sanity/Dockerfiletest/sanity/Dockerfile.upgradetest/sanity/ci-local.shtest/sanity/docker-compose.ymltest/sanity/fixtures/broken-config.jsontest/sanity/fixtures/compaction-session.sqltest/sanity/fixtures/old-config.jsontest/sanity/fixtures/test.sqltest/sanity/lib/altimate-run.shtest/sanity/lib/assert.shtest/sanity/lib/cleanup.shtest/sanity/lib/parallel.shtest/sanity/phases/resilience.shtest/sanity/phases/smoke-tests.shtest/sanity/phases/verify-install.shtest/sanity/phases/verify-upgrade.shtest/sanity/pr-tests/generate.shtest/sanity/pr-tests/run-pr-tests.shtest/sanity/results/baseline.jsontest/sanity/run.sh
✅ Files skipped from review due to trivial changes (13)
- test/sanity/fixtures/old-config.json
- test/sanity/.env.example
- test/sanity/lib/cleanup.sh
- test/sanity/results/baseline.json
- test/sanity/fixtures/broken-config.json
- test/sanity/fixtures/test.sql
- test/sanity/fixtures/compaction-session.sql
- test/sanity/pr-tests/run-pr-tests.sh
- test/sanity/phases/verify-install.sh
- test/sanity/docker-compose.yml
- test/sanity/lib/altimate-run.sh
- test/sanity/lib/assert.sh
- test/sanity/phases/resilience.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/sanity/phases/verify-upgrade.sh
- test/sanity/ci-local.sh
Comment on lines
+31
to
+33
| # Upgrade to new version | ||
| COPY --chown=testuser dist/package.tgz /tmp/altimate-code.tgz | ||
| RUN npm install -g /tmp/altimate-code.tgz |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a script or documentation for building package.tgz
rg -n "package.tgz" --type=sh --type=md -C2
# Also check if npm pack is invoked anywhere
rg -n "npm pack" --type=sh --type=ts -C2Repository: AltimateAI/altimate-code
Length of output: 6563
🏁 Script executed:
# First, examine the Dockerfile.upgrade file to understand full context
cat -n test/sanity/Dockerfile.upgradeRepository: AltimateAI/altimate-code
Length of output: 1760
🏁 Script executed:
# Search for CI/CD configuration files
find . -type f -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|circleci|travis)" | head -20Repository: AltimateAI/altimate-code
Length of output: 628
🏁 Script executed:
# Look for CI workflows directory
ls -la .github/workflows/ 2>/dev/null || echo "GitHub workflows not found"Repository: AltimateAI/altimate-code
Length of output: 627
🏁 Script executed:
# Search for build scripts and references to dist/package.tgz or npm pack more broadly
rg "dist/package\.tgz|npm pack" -C3 --max-count=20Repository: AltimateAI/altimate-code
Length of output: 13833
🏁 Script executed:
# Check package.json for build scripts
grep -A20 '"scripts"' package.json | head -30Repository: AltimateAI/altimate-code
Length of output: 1074
🏁 Script executed:
# Check docker-compose.upgrade.yml
cat test/sanity/docker-compose.upgrade.ymlRepository: AltimateAI/altimate-code
Length of output: 138
🏁 Script executed:
# Check docker-compose.yml for base setup
cat test/sanity/docker-compose.ymlRepository: AltimateAI/altimate-code
Length of output: 1869
🏁 Script executed:
# Check the CI workflow for package building
head -100 .github/workflows/ci.ymlRepository: AltimateAI/altimate-code
Length of output: 3856
🏁 Script executed:
# Look for release workflow that might build the package
head -150 .github/workflows/release.ymlRepository: AltimateAI/altimate-code
Length of output: 4954
🏁 Script executed:
# Search for where dist/package.tgz or tarball generation happens
rg "tgz|tarball|\.pack|bun build" -i --type=ts --type=js --type=sh -C2 | head -50Repository: AltimateAI/altimate-code
Length of output: 3867
🏁 Script executed:
# Search for any references to Dockerfile.upgrade
rg "Dockerfile\.upgrade|docker-compose\.upgrade" -iRepository: AltimateAI/altimate-code
Length of output: 418
🏁 Script executed:
# Check the CI local script
cat test/sanity/ci-local.shRepository: AltimateAI/altimate-code
Length of output: 3222
🏁 Script executed:
# Look for where dist/package.tgz might be built
find test -name "*.sh" -o -name "*.ts" | xargs grep -l "dist/package.tgz\|dist.*tgz" 2>/dev/nullRepository: AltimateAI/altimate-code
Length of output: 52
🏁 Script executed:
# Check test/sanity directory structure
ls -la test/sanity/Repository: AltimateAI/altimate-code
Length of output: 836
🏁 Script executed:
# Check run.sh to see how it references Dockerfile.upgrade
cat test/sanity/run.shRepository: AltimateAI/altimate-code
Length of output: 1565
🏁 Script executed:
# Check verify-upgrade.sh
cat test/sanity/phases/verify-upgrade.shRepository: AltimateAI/altimate-code
Length of output: 1744
🏁 Script executed:
# Search for any build step that creates dist/package.tgz
rg "\.tgz|bun pm pack|npm pack" test/sanity/ -C2Repository: AltimateAI/altimate-code
Length of output: 852
🏁 Script executed:
# Check if docker-compose.upgrade.yml is documented or should exist
git log --oneline --all -S "docker-compose.upgrade" 2>/dev/null | head -10 || echo "Git log unavailable"Repository: AltimateAI/altimate-code
Length of output: 154
Provide dist/package.tgz in build context for Docker upgrade test.
The COPY at line 32 expects dist/package.tgz at build context root, but this artifact is not created by the CI pipeline. The normal sanity test uses packages/opencode/dist/@altimateai/ directly (simulating postinstall), but the upgrade test appears designed to validate the published npm package artifact.
Either:
- Create
test/sanity/docker-compose.upgrade.yml(referenced inpackage.jsonbut missing) to orchestrate the build and providedist/package.tgz, or - Build the tarball locally before
docker build(e.g., viabun pm packindist/directory), or - Align Dockerfile.upgrade with the standard approach used in Dockerfile.
Currently, npm run sanity:upgrade will fail at the COPY step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/Dockerfile.upgrade` around lines 31 - 33, The Docker upgrade test
fails because Dockerfile.upgrade's COPY --chown=testuser dist/package.tgz
expects dist/package.tgz in the build context but CI doesn't produce it; fix by
either adding a docker-compose.upgrade.yml that builds the monorepo and produces
dist/package.tgz into the build context before docker build, or modify the
sanity:upgrade pipeline to run a pack step (e.g., run bun pm pack or npm pack in
the packages/opencode dist directory) to create dist/package.tgz prior to
invoking docker build, or change Dockerfile.upgrade to mirror the standard
Dockerfile approach (copying packages/opencode/dist/@altimateai/... instead of a
tarball) so npm run sanity:upgrade completes the COPY step successfully.
Comment on lines
+230
to
+247
| case "$result" in | ||
| PASS) | ||
| echo " PASS: $name" | ||
| PASS_COUNT=$((PASS_COUNT + 1)) | ||
| ;; | ||
| FAIL) | ||
| echo " FAIL: $name" | ||
| FAIL_COUNT=$((FAIL_COUNT + 1)) | ||
| ;; | ||
| SKIP) | ||
| echo " SKIP: $name" | ||
| SKIP_COUNT=$((SKIP_COUNT + 1)) | ||
| ;; | ||
| *) | ||
| echo " FAIL: $name (no result file)" | ||
| FAIL_COUNT=$((FAIL_COUNT + 1)) | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
PASS_COUNT, FAIL_COUNT, SKIP_COUNT are used but never initialized.
These variables are incremented in the results collection loop but are never declared or initialized, which will cause arithmetic errors on the first increment.
🔧 Proposed fix — initialize counters before the loop
# Collect results
echo ""
echo " Results:"
+PASS_COUNT=0
+FAIL_COUNT=0
+SKIP_COUNT=0
for name in "${TEST_NAMES[@]}"; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/phases/smoke-tests.sh` around lines 230 - 247, The counters
PASS_COUNT, FAIL_COUNT, and SKIP_COUNT are never initialized before being
incremented in the result-processing case block (the case handling
PASS/FAIL/SKIP/*), so initialize them to 0 before the loop that processes
results; add simple assignments PASS_COUNT=0 FAIL_COUNT=0 SKIP_COUNT=0 placed
prior to the code that iterates over results so the arithmetic increments
succeed.
Comment on lines
+31
to
+40
| echo "$changed" | grep "command/template/.*\.txt" 2>/dev/null | while read -r f; do | ||
| cmd=$(basename "$f" .txt) | ||
| emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test" | ||
| done | ||
|
|
||
| # Skill changed → test skill file exists in builtin | ||
| echo "$changed" | grep "skills/.*/SKILL.md" 2>/dev/null | while read -r f; do | ||
| skill=$(basename "$(dirname "$f")") | ||
| emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md" | ||
| done |
There was a problem hiding this comment.
pipefail can terminate generation when grep finds no matches.
On Line 31 and Line 37, grep returns 1 for “no match”. With set -euo pipefail, that can abort the script and skip later test emission logic.
💡 Suggested fix
-echo "$changed" | grep "command/template/.*\.txt" 2>/dev/null | while read -r f; do
+while read -r f; do
cmd=$(basename "$f" .txt)
emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test"
-done
+done < <(printf '%s\n' "$changed" | grep -E "command/template/.*\.txt" || true)
-echo "$changed" | grep "skills/.*/SKILL.md" 2>/dev/null | while read -r f; do
+while read -r f; do
skill=$(basename "$(dirname "$f")")
emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md"
-done
+done < <(printf '%s\n' "$changed" | grep -E "skills/.*/SKILL.md" || true)📝 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
| echo "$changed" | grep "command/template/.*\.txt" 2>/dev/null | while read -r f; do | |
| cmd=$(basename "$f" .txt) | |
| emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test" | |
| done | |
| # Skill changed → test skill file exists in builtin | |
| echo "$changed" | grep "skills/.*/SKILL.md" 2>/dev/null | while read -r f; do | |
| skill=$(basename "$(dirname "$f")") | |
| emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md" | |
| done | |
| echo "$changed" | grep -E "migration/" 2>/dev/null | while read -r f; do | |
| # Signal that upgrade verification is needed but must be run separately in Docker | |
| emit_test "upgrade-signal" "verify-upgrade.sh" | |
| done | |
| while read -r f; do | |
| cmd=$(basename "$f" .txt) | |
| emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test" | |
| done < <(printf '%s\n' "$changed" | grep -E "command/template/.*\.txt" || true) | |
| # Skill changed → test skill file exists in builtin | |
| while read -r f; do | |
| skill=$(basename "$(dirname "$f")") | |
| emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md" | |
| done < <(printf '%s\n' "$changed" | grep -E "skills/.*/SKILL.md" || true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/pr-tests/generate.sh` around lines 31 - 40, The pipelines that
feed the while-read loops in generate.sh can cause the script to exit under set
-euo pipefail because grep returns non-zero when there are no matches; update
the two grep pipelines that precede the "while read -r f; do" blocks (the ones
producing command-*.txt and skills/.*/SKILL.md) so they never produce a non-zero
exit (for example append "|| true" to the grep command or use a grep -q check
and guard the loop), leaving the emit_test invocations (emit_test
"command-${cmd}" ... and emit_test "skill-${skill}" ...) unchanged.
Comment on lines
+32
to
+39
| cmd=$(basename "$f" .txt) | ||
| emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test" | ||
| done | ||
|
|
||
| # Skill changed → test skill file exists in builtin | ||
| echo "$changed" | grep "skills/.*/SKILL.md" 2>/dev/null | while read -r f; do | ||
| skill=$(basename "$(dirname "$f")") | ||
| emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md" |
There was a problem hiding this comment.
Unsanitized path-derived values can inject shell into emitted commands.
cmd and skill are derived from changed filenames (Line 32, Line 38) and embedded into manifest commands (Line 33, Line 39). Since these commands are later evaluated, a crafted filename can execute arbitrary shell.
🔒 Suggested hardening
cmd=$(basename "$f" .txt)
+if [[ ! "$cmd" =~ ^[A-Za-z0-9_-]+$ ]]; then
+ echo " Skipping unsafe command template name: $cmd"
+ continue
+fi
+escaped_cmd=$(printf '%q' "$cmd")
-emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${cmd} test"
+emit_test "command-${cmd}" "altimate run --max-turns 1 --yolo --command ${escaped_cmd} test"
skill=$(basename "$(dirname "$f")")
+if [[ ! "$skill" =~ ^[A-Za-z0-9_-]+$ ]]; then
+ echo " Skipping unsafe skill name: $skill"
+ continue
+fi
+escaped_skill=$(printf '%q' "$skill")
-emit_test "skill-${skill}" "ls ~/.altimate/builtin/${skill}/SKILL.md"
+emit_test "skill-${skill}" "ls ~/.altimate/builtin/${escaped_skill}/SKILL.md"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanity/pr-tests/generate.sh` around lines 32 - 39, The emitted commands
build shell strings from untrusted filename-derived variables cmd and skill
(used with emit_test) and can lead to shell injection; fix by
validating/sanitizing those values before use: when parsing each changed file,
constrain cmd and skill to a safe whitelist pattern (e.g. reject unless they
match /^[A-Za-z0-9._-]+$/) and skip/warn on mismatches, or explicitly
shell-escape them (e.g. using printf '%q' or equivalent) when constructing the
command string passed to emit_test; update the blocks that set cmd and skill and
all emit_test calls so they use the sanitized/escaped variable names.
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.
Summary
Docker-based "new user simulator" that tests the shipped npm artifact end-to-end — catches bugs that existing unit tests miss.
altimate run --yolo --format json: commands resolve, sql_analyze works, DuckDB driver, agent routing, graceful error handlinggit diffanalysis generates targeted tests for what changedbun run ciruns typecheck + tests + markers in one command27 tests, all passing in ~2:48 on a 20-core machine.
How to use
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit