Bundle inputs mode for build_bundle.sh + GTFS-RT stop-remap wiring#132
Conversation
No behavior change for single-mode use: same env vars, messages, and artifacts. Adds set -euo pipefail so a failed wget/gtfstidy/zip/java now aborts the build instead of silently continuing, and BUNDLE_DIR (default /bundle) so tests don't need to touch /bundle. Lays the function structure (validate_mode_env, bundle_mode, fetch_url, run_single_mode, main) that later multi-input mode tasks build on.
Runs the real transit-data-federation builder jar inside the built oba image against two 3-stop fixture GTFS feeds in multi-input mode, proving the manifest/sha256/entity-replacement path end-to-end rather than through the stubbed unit harness alone. Also pins the keeper-missing mapping-row failure mode: the replaced stop is dropped from the bundle either way, but with no valid keeper the builder leaves a dangling stopTime pointing at a null stop (StopTimeEntriesFactory ERROR) instead of cleanly remapping it — the concrete harm that justifies obacloud excluding keeper-missing rows before publish. Assertions were adjusted from the plan's draft based on what the real jar actually does: the hypothesized "error replacing entity ... replacement not found" log line does not appear against the 2.7.1 builder, and an anchored `strings | grep '^ID$'` probe against the serialized TransitGraph.obj is flaky because the Java serialization stream has no line terminators. Both were replaced with directly verified, reproducible checks.
|
Warning Review limit reached
Next review available in: 53 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. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors the bundle build scripts to support single-input and multi-input manifest-driven builds with stop consolidation mapping, adds template wiring for stop remapping, and expands unit, integration, CI, and documentation coverage. ChangesMulti-input bundle building
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant FixtureServer as python3 http.server
participant Docker as oba image
participant BuildScript as build_bundle.sh
CI->>FixtureServer: start fixture server
CI->>Docker: run integration test
Docker->>BuildScript: execute bundle build
BuildScript->>FixtureServer: fetch bundle inputs and GTFS zip
BuildScript-->>Docker: build log and artifacts
Docker-->>CI: test result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
bundler/Dockerfile (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--no-install-recommendsand clean the apt cache.Reduces image size and avoids pulling unneeded recommended packages; also keeps the layer clean.
♻️ Proposed fix
-RUN apt-get update && apt-get install -y jq unzip zip +RUN apt-get update && apt-get install -y --no-install-recommends jq unzip zip \ + && rm -rf /var/lib/apt/lists/*🤖 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 `@bundler/Dockerfile` at line 45, The Dockerfile’s apt install step needs to be tightened to avoid unnecessary packages and leftover cache. Update the existing apt-get update && apt-get install -y jq unzip zip command to include --no-install-recommends, and clean up apt cache in the same layer so the image stays smaller; make the change in the Dockerfile build step that installs jq, unzip, and zip.Source: Linters/SAST tools
bundler/build_bundle.sh (1)
258-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the glob so tidied filenames aren't parsed as options.
zip ../gtfs_tidied.zip *lets a filename beginning with-be treated as a flag. Use./*.♻️ Proposed fix
- zip ../gtfs_tidied.zip * + zip ../gtfs_tidied.zip ./*🤖 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 `@bundler/build_bundle.sh` around lines 258 - 263, The zip command in the gtfs-out packaging block is vulnerable to filenames starting with a dash being interpreted as options. Update the gtfs_tidied.zip creation logic in the gtfs-out cd/zip sequence to use a safe glob like ./* instead of * so tidied filenames are passed as paths, not flags, and keep the GTFS_ZIP_FILENAME assignment unchanged.Source: Linters/SAST tools
bin/build_bundle_test.sh (1)
53-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSC2015 pattern (
A && B || C) is a false positive here.
pass/failend in a plain variable assignment (passed=$((...))), which always exits 0, sofailcan't accidentally fire from the&&branch. Functionally fine as-is; only worth a real if/then/else if you want to silence the linter noise across the file.🤖 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 `@bin/build_bundle_test.sh` around lines 53 - 59, The shell test assertions use the A && B || C pattern in the build bundle test script, which triggers SC2015 as a false positive. Keep the current behavior in the test helpers like pass and fail if you want, but to satisfy the linter noise either rewrite the affected assertions into explicit if/then/else blocks or otherwise adjust the surrounding test logic in build_bundle_test.sh so the SC2015 warning is avoided without changing the test semantics.Source: Linters/SAST tools
oba/config/onebusaway-transit-data-federation-webapp-data-sources.xml.hbs (1)
80-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated branching mirrors the existing agencyId/agencyIds block.
The
{{#ifthis.agencyIds.length}}...{{else if this.agencyId}}structure here duplicates the shape of the block at lines 68-78 just to extract/apply the agency id a second time. Not blocking, but if more per-agency wiring is added later this duplication will compound.🤖 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 `@oba/config/onebusaway-transit-data-federation-webapp-data-sources.xml.hbs` around lines 80 - 94, The stopModificationStrategy Handlebars block duplicates the earlier agencyId/agencyIds branching and repeats the same agency-id extraction logic. Refactor the repeated logic in one shared place, either by introducing a reusable partial/helper or by assigning a single normalized agencyId value before rendering ConsolidatedStopsModificationStrategy, so both branches in the template use the same source of truth.bin/bundle_inputs_integration_test.sh (1)
38-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed
sleep 1with a readiness poll.A fixed 1-second sleep for the
http.serverto become ready is a latent flakiness source on loaded/slow CI runners — if the server isn't up yet, the subsequentwgetcalls inside the container will fail the whole test run.♻️ Proposed fix: poll instead of fixed sleep
-sleep 1 +for i in $(seq 1 20); do + curl -sf "http://127.0.0.1:${PORT}/bundle-inputs.json" >/dev/null 2>&1 && break + sleep 0.5 +done🤖 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 `@bin/bundle_inputs_integration_test.sh` around lines 38 - 41, The test setup in the bundle inputs integration script uses a fixed sleep after starting the http.server, which can race on slower CI runners. Replace that sleep with a readiness poll in the same setup block by waiting until the server is actually reachable on $PORT before proceeding, using the existing SERVER_PID/PORT/SERVE flow so the later wget-based checks in the test only run once the server is ready..github/workflows/test.yaml (2)
34-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps.Neither checkout step needs subsequent git push/pull operations, so persisting credentials to disk is unnecessary risk exposure flagged by zizmor's artipacked audit.
🔒 Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: falseApply to both the
bundle_scriptandbundle_inputs_integrationjobs' checkout steps.Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml around lines 34 - 35, The checkout steps in the bundle_script and bundle_inputs_integration jobs should disable credential persistence. Update each actions/checkout@v4 usage to set persist-credentials to false, since these workflows do not need later git push/pull access. Use the checkout step in both jobs as the target location and apply the same setting consistently.Source: Linters/SAST tools
43-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit
permissions:block tobundle_inputs_integration.The job has no
permissions:key and thus inherits the default (broader)GITHUB_TOKENscope, unlike the siblingbundle_scriptjob which correctly setscontents: read.🔒 Proposed fix
bundle_inputs_integration: name: Bundle inputs integration (real builder) runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml around lines 43 - 61, The bundle_inputs_integration job is missing an explicit permissions block, so it inherits a broader GITHUB_TOKEN scope than needed. Update the bundle_inputs_integration workflow job in test.yaml to match the sibling bundle_script job by adding a permissions section with contents set to read, keeping the rest of the steps unchanged.Source: Linters/SAST tools
🤖 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 `@bundler/build_bundle.sh`:
- Around line 150-204: Escape the manifest-derived values before writing them
into bundle-context.xml in generate_bundle_context_xml, since id and
defaultAgencyId are inserted directly into XML attributes. Update the jq reads
for feeds[$i].id and feeds[$i].defaultAgencyId to produce escaped text so
special characters like &, <, and > cannot break the generated XML. Keep the
rest of the XML template logic in build_bundle.sh unchanged and ensure the
generated GtfsBundle entries remain valid for all feed values.
---
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 34-35: The checkout steps in the bundle_script and
bundle_inputs_integration jobs should disable credential persistence. Update
each actions/checkout@v4 usage to set persist-credentials to false, since these
workflows do not need later git push/pull access. Use the checkout step in both
jobs as the target location and apply the same setting consistently.
- Around line 43-61: The bundle_inputs_integration job is missing an explicit
permissions block, so it inherits a broader GITHUB_TOKEN scope than needed.
Update the bundle_inputs_integration workflow job in test.yaml to match the
sibling bundle_script job by adding a permissions section with contents set to
read, keeping the rest of the steps unchanged.
In `@bin/build_bundle_test.sh`:
- Around line 53-59: The shell test assertions use the A && B || C pattern in
the build bundle test script, which triggers SC2015 as a false positive. Keep
the current behavior in the test helpers like pass and fail if you want, but to
satisfy the linter noise either rewrite the affected assertions into explicit
if/then/else blocks or otherwise adjust the surrounding test logic in
build_bundle_test.sh so the SC2015 warning is avoided without changing the test
semantics.
In `@bin/bundle_inputs_integration_test.sh`:
- Around line 38-41: The test setup in the bundle inputs integration script uses
a fixed sleep after starting the http.server, which can race on slower CI
runners. Replace that sleep with a readiness poll in the same setup block by
waiting until the server is actually reachable on $PORT before proceeding, using
the existing SERVER_PID/PORT/SERVE flow so the later wget-based checks in the
test only run once the server is ready.
In `@bundler/build_bundle.sh`:
- Around line 258-263: The zip command in the gtfs-out packaging block is
vulnerable to filenames starting with a dash being interpreted as options.
Update the gtfs_tidied.zip creation logic in the gtfs-out cd/zip sequence to use
a safe glob like ./* instead of * so tidied filenames are passed as paths, not
flags, and keep the GTFS_ZIP_FILENAME assignment unchanged.
In `@bundler/Dockerfile`:
- Line 45: The Dockerfile’s apt install step needs to be tightened to avoid
unnecessary packages and leftover cache. Update the existing apt-get update &&
apt-get install -y jq unzip zip command to include --no-install-recommends, and
clean up apt cache in the same layer so the image stays smaller; make the change
in the Dockerfile build step that installs jq, unzip, and zip.
In `@oba/config/onebusaway-transit-data-federation-webapp-data-sources.xml.hbs`:
- Around line 80-94: The stopModificationStrategy Handlebars block duplicates
the earlier agencyId/agencyIds branching and repeats the same agency-id
extraction logic. Refactor the repeated logic in one shared place, either by
introducing a reusable partial/helper or by assigning a single normalized
agencyId value before rendering ConsolidatedStopsModificationStrategy, so both
branches in the template use the same source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48b29d61-dfea-4707-9bf2-9103de08086e
📒 Files selected for processing (16)
.github/workflows/test.yamlREADME.mdbin/build_bundle_test.shbin/bundle_inputs_integration_test.shbin/testdata/build_bundle/bundle-inputs-no-mapping.jsonbin/testdata/build_bundle/bundle-inputs.jsonbin/testdata/build_bundle/golden-context-no-mapping.xmlbin/testdata/build_bundle/golden-context-with-mapping.xmlbin/testdata/build_bundle/make_fixtures.shbin/testdata/build_bundle/mapping-keeper-missing.txtbin/testdata/build_bundle/mapping-valid.txtbundler/Dockerfilebundler/build_bundle.shoba/build_bundle.shoba/config/onebusaway-transit-data-federation-webapp-data-sources.xml.hbsoba/config/template_renderer/main_test.go
The 'Services with Bundler' job was failing on stops-for-location returning 0 stops. Root cause is environmental, not this PR: the job built from the live Unitrans feed, and the current feed leaves ~42% of its stops out of OBA's runtime geospatial STRtree (near-coincident directional stop pairs). validate.sh picks 'first route's first stop' dynamically, so it fails at random depending on which stop it lands on. The single-mode build path, OBA JAR, and gtfstidy output are all unchanged by this PR (verified), so main would fail the same way today. Make the job deterministic by building from a small committed GTFS fixture (7 well-separated stops, all indexed) served on the compose network via a new docker-compose.ci.yml override, instead of the live feed. Also address CodeRabbit review feedback: - Escape manifest-derived ids/agencyIds before writing bundle-context.xml (new xml_attr_escape helper) + a unit test; both build_bundle.sh copies stay byte-identical. - zip ../gtfs_tidied.zip ./* (glob safety). - bundler/Dockerfile: --no-install-recommends + clean apt lists. - bundle_inputs_integration job: add contents:read permissions; persist-credentials:false on the two new jobs' checkouts. - Integration test: poll for the fixture server instead of a fixed sleep.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
45-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo job-level timeout guard other than the integration step.
bundle_inputs_integrationsets a 20-minutetimeout-minutesonly on the "Run integration test" step; the "Build oba image" step has no timeout and could hang on a stuck buildx cache/network issue, consuming the full default GitHub Actions job timeout (6h).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml around lines 45 - 67, The bundle_inputs_integration job only times out the Run integration test step, so a hung Build oba image step can still run until the full job timeout. Add an explicit timeout guard to the job or to the docker buildx build step in bundle_inputs_integration so the build cannot stall indefinitely, and keep the existing Run integration test timeout in place.bin/testdata/services/make_services_fixture.sh (1)
63-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winZip won't overwrite stale entries on regeneration.
zipupdates an existing archive rather than replacing it; ifservices-gtfs.zipalready exists (e.g. after editing which.txtfiles are generated), stale entries from a prior run can persist alongside new ones. Since the comment header instructs re-running this script to regenerate the fixture, consider removing the old archive first.Proposed fix
-(cd "$WORK" && zip -q "$OUT_DIR/services-gtfs.zip" ./*.txt) +rm -f "$OUT_DIR/services-gtfs.zip" +(cd "$WORK" && zip -q "$OUT_DIR/services-gtfs.zip" ./*.txt)🤖 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 `@bin/testdata/services/make_services_fixture.sh` at line 63, The services-gtfs archive generation step can leave stale entries behind because zip updates an existing file instead of replacing it. In make_services_fixture.sh, remove any existing services-gtfs.zip before the zip command runs, so rerunning the fixture regeneration from the same WORK directory always produces a clean archive. Keep the fix localized to the archive creation block that uses WORK, OUT_DIR, and services-gtfs.zip.
🤖 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.
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 45-67: The bundle_inputs_integration job only times out the Run
integration test step, so a hung Build oba image step can still run until the
full job timeout. Add an explicit timeout guard to the job or to the docker
buildx build step in bundle_inputs_integration so the build cannot stall
indefinitely, and keep the existing Run integration test timeout in place.
In `@bin/testdata/services/make_services_fixture.sh`:
- Line 63: The services-gtfs archive generation step can leave stale entries
behind because zip updates an existing file instead of replacing it. In
make_services_fixture.sh, remove any existing services-gtfs.zip before the zip
command runs, so rerunning the fixture regeneration from the same WORK directory
always produces a clean archive. Keep the fix localized to the archive creation
block that uses WORK, OUT_DIR, and services-gtfs.zip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 403b9a0a-df53-4c0d-ba32-75c6a6ee5c28
⛔ Files ignored due to path filters (1)
bin/testdata/services/services-gtfs.zipis excluded by!**/*.zip
📒 Files selected for processing (8)
.github/workflows/test.yamlbin/build_bundle_test.shbin/bundle_inputs_integration_test.shbin/testdata/services/make_services_fixture.shbundler/Dockerfilebundler/build_bundle.shdocker-compose.ci.ymloba/build_bundle.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- bundler/Dockerfile
- bin/build_bundle_test.sh
- bin/bundle_inputs_integration_test.sh
- bundler/build_bundle.sh
- oba/build_bundle.sh
The bash ${//} version produced '<lt;' instead of '<' on Ubuntu's bash
5.2, where a literal & in the replacement means the matched text. Use sed,
which is version-independent. Verified 49/49 in an ubuntu:24.04 container
(bash 5.2.21, GNU sed 4.9).
1 participant
Summary
Multi-input ("bundle inputs") mode for
build_bundle.sh, standalone stop-consolidation support, and GTFS-RT stop-remap wiring. Companion to OneBusAway/obacloud's stop-consolidation PR (which generates theBUNDLE_INPUTS_URLDockerfiles and publishes the manifest) and OneBusAway/gtfs-merge-service (which uploads the per-feed zips + manifest).Why: the OBA server must build its bundle from per-feed zips with per-zip
defaultAgencyId— a single merged zip stamps every stop with the first agency in agency.txt, collapsing multi-agency regions onto one prefix.What's included
build_bundle.shrefactor (bothoba/andbundler/copies, byte-identical, now CI-enforced): functions +set -euo pipefail, one-lineERROR:diagnostics. Classic single-zip mode is byte-for-byte compatible (env vars, messages, artifacts, gtfstidy) — pinned by the 48-test unit harness.BUNDLE_INPUTS_URL, mutually exclusive withGTFS_URL/GTFS_ZIP_FILENAME/STOP_CONSOLIDATION_URL): downloadsbundle-inputs.json(version 1), sha256-verifies each per-feed zip (hard fail — no partial bundles), generates a Springbundle-context.xml(gtfs-bundlesbean with per-feeddefaultAgencyIdin manifest order;entityReplacementStrategybeans when the manifest carriesstopConsolidationUrl), and runs the real transit-data-federation builder with no gtfstidy (legacy parity). Mapping lands at<bundle>/StopConsolidation.txt(hardcoded runtime filename).STOP_CONSOLIDATION_URLin classic single-zip mode (replacement-beans-only context XML).GtfsRealtimeSourcebean now receives aConsolidatedStopsModificationStrategy(explicitagencyId= feed's first agency;consolidatedStopsServiceautowired) — without this, consolidated keepers silently lose member-agency realtime. Verified two ways: Go renderer tests against the real template, and a live docker-compose federation boot (bean graph loads;"missing Stop Consolidation File; skipping"no-op confirmed; zero BeanCreation errors).jqadded to the bundler image (wget verified already present in the tomcat base); newbundle_scriptCI job (unit harness + byte-identical-copies guard) andbundle_inputs_integrationjob — a real-JAR (2.7.1) end-to-end test on fixture feeds that also pins the keeper-missing failure mode: a mapping line whose keeper doesn't exist leaves the replaced stop GONE from the serializedTransitGraph.obj(byte-level check) with a dangling-stopTime ERROR — the serialized-bundle proof of why obacloud excludes keeper-missing rows at publish.Verification
main → validate → download → XML → javaseam and cross-checked every manifest field name against the Go producer and the Rails consumer: no drift.Follow-ups (non-blocking)
GTFS_ZIP_FILENAME+STOP_CONSOLIDATION_URLcombination test.rm -rf /bundle/inputsafter a successful multi-input build (currently baked into the image layer, consistent with single mode's leftover zips).imageandbundle_inputs_integrationjobs.jq empty, non-emptyid/defaultAgencyId) for uniform ERROR diagnostics on malformed manifests.Summary by CodeRabbit