Build and deploy skysim from the repository, world included - #9
Conversation
skysim was the only service in the account with no build pipeline. Its image was
built by hand, which is why the one running on dev and prod was a day stale and
missing the two fixes the overwatch coordinator depends on — the deployed
simulator counted other aircraft as buildings and refused every relief launch,
and nobody could tell from the outside.
Two things here differ from the other services, both deliberate:
* x86_64, not ARM. Both skysim task definitions are X86_64, and an image built
for the wrong architecture fails at task start rather than at build time.
* The cooked world is fetched from S3 and baked into the image. The plain
Dockerfile ships no tiles on purpose — a new city should not mean a new
image, and locally the world is mounted — but the ECS services have no
volume mounts, so without this skysim starts flat. A flat world is not a
harmless default here: every route sweep answers "no building data", which
the coordinator reads as unverified and refuses to fly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds an ECR-specific Docker image that embeds optional simulator world tiles and a CodeBuild pipeline that builds, tags, publishes, and deploys the image to ECS. ChangesECR deployment pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CodeBuild
participant AmazonS3
participant Docker
participant AmazonECR
participant AmazonECS
CodeBuild->>Docker: Start daemon and wait for readiness
CodeBuild->>AmazonECR: Authenticate with get-login-password
CodeBuild->>AmazonS3: Download optional world archive
CodeBuild->>Docker: Build skysim and Dockerfile.ecr images
Docker->>Docker: Copy world-tiles into /opt/skysim/tiles/
CodeBuild->>AmazonECR: Push latest and commit SHA tags
CodeBuild->>AmazonECS: Force new service deployment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
buildspec.yml (2)
38-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPull the cache image before using
--cache-from.The newly started CodeBuild Docker daemon does not have
$IMAGE_NAME:latestlocally, and--cache-fromdoes not pull remote images. This makes the intended cache ineffective and causes cold builds. Pull the image after ECR login or use a registry-backed BuildKit cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildspec.yml` at line 38, Update the Docker build step to pull $IMAGE_NAME:latest from ECR after authentication and before invoking docker build with --cache-from, preserving the existing image tag and cache usage.
27-30: 🗄️ Data Integrity & Integration | 🔵 TrivialPin the S3 world artifact for reproducible image tags.
The image is tagged by source commit at Line [40], but the S3 object is mutable. Rebuilding the same commit can therefore produce different images under the same commit tag. Use an immutable/versioned S3 object or record its version ID/checksum in the image metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildspec.yml` around lines 27 - 30, Update the WORLD_S3_URI artifact retrieval in the buildspec to use an immutable, versioned S3 object, or require and record its S3 version ID or checksum in the built image metadata. Ensure rebuilding the same source commit always fetches and embeds the identical world artifact while preserving the existing extraction flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@buildspec.yml`:
- Line 45: Add an `aws ecs wait services-stable` command immediately after the
`aws ecs update-service` command, using the existing ECS_CLUSTER and ECS_SERVICE
variables, so the build waits for healthy replacement tasks and fails if the
deployment does not stabilize.
- Around line 25-34: Update the no-world build flow around WORLD_S3_URI and the
entrypoint’s tile argument handling so an unset WORLD_S3_URI does not export
world-tiles as an enabled tiles directory. Prefer skipping --tiles for this
variant, or ensure the generated directory contains a valid empty index.json
before startup; preserve normal tile extraction and --tiles behavior when
WORLD_S3_URI is set.
In `@Dockerfile.ecr`:
- Around line 12-14: Update the Dockerfile.ecr image setup to create or select a
non-root runtime user, add a USER instruction for that user, and ensure the
simulator binary and /opt/skysim/tiles remain readable by it.
---
Nitpick comments:
In `@buildspec.yml`:
- Line 38: Update the Docker build step to pull $IMAGE_NAME:latest from ECR
after authentication and before invoking docker build with --cache-from,
preserving the existing image tag and cache usage.
- Around line 27-30: Update the WORLD_S3_URI artifact retrieval in the buildspec
to use an immutable, versioned S3 object, or require and record its S3 version
ID or checksum in the built image metadata. Ensure rebuilding the same source
commit always fetches and embeds the identical world artifact while preserving
the existing extraction flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 286f293c-d37c-4e9c-9844-ad3b6b04d4da
📒 Files selected for processing (2)
Dockerfile.ecrbuildspec.yml
| - mkdir -p world-tiles | ||
| - | | ||
| if [ -n "$WORLD_S3_URI" ]; then | ||
| echo "Fetching cooked world from $WORLD_S3_URI" | ||
| aws s3 cp "$WORLD_S3_URI" /tmp/world.tgz | ||
| tar -xzf /tmp/world.tgz -C world-tiles | ||
| echo "world: $(ls world-tiles | wc -l) file(s)" | ||
| else | ||
| echo "WORLD_S3_URI is unset — building with no buildings" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '^(buildspec\.yml|Dockerfile\.ecr|src/main\.cpp|.*TileStreamer.*|.*tile.*stream.*|.*ecs.*|.*task.*json)$' || true
printf '\n== buildspec.yml ==\n'
nl -ba buildspec.yml | sed -n '1,120p'
printf '\n== Dockerfile.ecr ==\n'
nl -ba Dockerfile.ecr | sed -n '1,220p'
printf '\n== search for index.json / tiles references ==\n'
rg -n --hidden --glob '!**/.git/**' 'index\.json|world-tiles|--tiles|TileStreamer|tile_dir|tiles missing|zero-loaded|no-world|WORLD_S3_URI' .Repository: ID-Robots/skysim
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba buildspec.yml | sed -n '20,60p'Repository: ID-Robots/skysim
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
ls -1Repository: ID-Robots/skysim
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== buildspec.yml ==\n'
sed -n '1,120p' buildspec.yml
printf '\n== Dockerfile.ecr ==\n'
sed -n '1,220p' Dockerfile.ecr
printf '\n== src/main.cpp ==\n'
sed -n '1,240p' src/main.cpp
printf '\n== src/terrain/tile_streamer.h ==\n'
sed -n '1,220p' src/terrain/tile_streamer.h
printf '\n== src/terrain/tile_streamer.cpp ==\n'
sed -n '1,260p' src/terrain/tile_streamer.cppRepository: ID-Robots/skysim
Length of output: 19958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== main tile startup ==\n'
grep -n -A80 -B40 'TileStreamer\|--tiles\|tiles' src/main.cpp
printf '\n== all --tiles references ==\n'
rg -n --hidden --glob '!**/.git/**' -- '--tiles|TileStreamer|index\.json|world-tiles|/opt/skysim/tiles' .Repository: ID-Robots/skysim
Length of output: 37963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' docker-entrypoint.sh
printf '\n== relevant docs ==\n'
sed -n '35,70p' docs/SKYHUB_INTEGRATION.md
printf '\n== README tiles section ==\n'
sed -n '115,145p' README.mdRepository: ID-Robots/skysim
Length of output: 5223
Keep the no-world image from exporting --tiles.
world-tiles/ is copied into /opt/skysim/tiles/, and the entrypoint adds --tiles whenever that directory exists. With WORLD_S3_URI unset, TileStreamer aborts on missing index.json, so the no-world build still fails at startup. Either skip --tiles for this variant or generate a valid empty index.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@buildspec.yml` around lines 25 - 34, Update the no-world build flow around
WORLD_S3_URI and the entrypoint’s tile argument handling so an unset
WORLD_S3_URI does not export world-tiles as an enabled tiles directory. Prefer
skipping --tiles for this variant, or ensure the generated directory contains a
valid empty index.json before startup; preserve normal tile extraction and
--tiles behavior when WORLD_S3_URI is set.
| commands: | ||
| # Both tags, so a deployment can be traced back to the commit that produced it. | ||
| - docker push -a $IMAGE_NAME | ||
| - aws ecs update-service --service $ECS_SERVICE --force-new-deployment --cluster $ECS_CLUSTER --region $AWS_DEFAULT_REGION |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l buildspec.yml
sed -n '1,120p' buildspec.yml | cat -nRepository: ID-Robots/skysim
Length of output: 2598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ecs wait|services-stable|update-service" .Repository: ID-Robots/skysim
Length of output: 302
Wait for the ECS deployment to stabilize. update-service only starts the deployment; add aws ecs wait services-stable --cluster $ECS_CLUSTER --services $ECS_SERVICE so the build fails if replacement tasks never become healthy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@buildspec.yml` at line 45, Add an `aws ecs wait services-stable` command
immediately after the `aws ecs update-service` command, using the existing
ECS_CLUSTER and ECS_SERVICE variables, so the build waits for healthy
replacement tasks and fails if the deployment does not stabilize.
| FROM skyhub-skysim:build | ||
|
|
||
| COPY world-tiles/ /opt/skysim/tiles/ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Run the ECS workload as a non-root user.
This image inherits the base image’s default user and contains no USER instruction, so the simulator runs as root. Create/select a non-root user in the base or runtime image, set USER, and verify that the binary and /opt/skysim/tiles remain readable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile.ecr` around lines 12 - 14, Update the Dockerfile.ecr image setup
to create or select a non-root runtime user, add a USER instruction for that
user, and ensure the simulator binary and /opt/skysim/tiles remain readable by
it.
Source: Linters/SAST tools
The endpoint went in with the battery model and was never exercised: the fake tick
thread in test_api did not even handle BatteryResetCommand, so both arms of its
result check were dead code as far as the suite was concerned. That is the call
the gateway makes after every landing — without it skysim keeps discharging the
old pack and the vehicle is retired after one sortie — so it is worth a test on
its own merits.
Two cases: a live vehicle answers {"ok":true}, and one that is not there answers
404 rather than silently reporting a pack it never fitted.
This also takes branch coverage from 69.9% to 71.3%, back over the 70% gate. The
gate was failing on main, not on anything this branch changed; padding the
threshold would have hidden an endpoint with no test behind a number.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
skysim was the only service in the account with no build pipeline. Its image was built by hand, which is why the one running on dev and prod was a day stale and missing the two fixes the overwatch coordinator depends on — the deployed simulator counted other aircraft as buildings and refused every relief launch, and nothing about that was visible from the outside.
This adds
buildspec.ymlandDockerfile.ecr, so the image is built the same way every other service here is. The matching CodeBuild projects (skyhub-dev-skysim-build-and-deploy,skyhub-prod-skysim-build-and-deploy) are created alongside.Two deliberate differences from the other services
x86_64, not ARM. Both skysim task definitions are
X86_64, while every other build in this account uses the aarch64 CodeBuild image. An image built for the wrong architecture fails at task start rather than at build time, which is a slow and confusing way to find out.The cooked world is fetched from S3 and baked in. The plain
Dockerfileships no tiles on purpose — a new city should not mean a new image, and locally the world is mounted — but the ECS services have no volume mounts. Without this skysim starts flat, and a flat world is not a harmless default: every route sweep answerschecked: false, which the overwatch coordinator reads as "unverified" and refuses to fly. The world lives ats3://skyhub-{env}-assets/skysim-world/plovdiv.tgz;WORLD_S3_URIis unset-safe, producing a working simulator with no buildings rather than a failed build.Images are tagged
latestand the resolved commit, so a running task can be traced back to what produced it.🤖 Generated with Claude Code
Summary by CodeRabbit
latestand commit-specific tags.