ci: add Metro/Hermes bundle job - #52
Merged
Merged
Conversation
tsc and `node --test` both run under Node's module resolver. Metro and Hermes do not. Three releases have shipped where type-checking was green and the bundle was broken on device — v1.13.0 (raw parser), v1.13.9 (stocks classifier) and v1.13.16 (Bequests picker showed only SOL because an enrichment import resolved to undefined under Hermes). No existing job can see that class of failure. This job produces the actual Hermes bytecode the APK embeds, then asserts it: - exactly one .hbc is emitted; - it carries the Hermes bytecode magic, so a silent fallback to plain JS fails rather than passing as a successful export; - the program id from release.manifest.json appears exactly once, proving app source is in the graph and that no module was duplicated by being reached through two different specifiers. All four failure paths were verified to trip before committing. Not an APK build: no Gradle, no native code, no signing, no artifact published. The authoritative release path is unchanged. No .env is provided in CI, deliberately. EXPO_PUBLIC_* values do not inline into the bundle — the .hbc content hash is byte-identical with and without .env, which is why rpcConfig derives them at runtime.
Contributor
📝 WalkthroughWalkthroughThe CI workflow adds an ChangesAndroid app bundle CI
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
99-99: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDo not persist checkout credentials in this job.
This job runs dependency-provided install and export scripts. It has no shown Git operation that requires persisted credentials. Set
persist-credentials: falseonactions/checkout.Proposed change
- - uses: actions/checkout@v7 + - uses: actions/checkout@v7 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 99, Update the actions/checkout step in the CI job to set persist-credentials to false, ensuring checkout credentials are not retained while leaving the existing checkout behavior 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 @.github/workflows/ci.yml:
- Line 132: Update the hits calculation in the CI workflow to count each
fixed-string occurrence of program_id rather than matching lines: use grep’s
only-matching and fixed-string options, then pipe the results to wc -l while
preserving the existing no-match handling.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 99: Update the actions/checkout step in the CI job to set
persist-credentials to false, ensuring checkout credentials are not retained
while leaving the existing checkout behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # reached by two different specifiers and duplicated into the graph — which for a | ||
| # stateful module means two divergent instances at runtime. | ||
| program_id=$(node -p "require('../release.manifest.json').programId") | ||
| hits=$(grep -a -c -- "$program_id" "$bundle" || true) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
program_id='GXCu5964mvgAJDWmcMriZpzU3vDVqPzjYCM1sxCnsoEb'
bundle="$(mktemp)"
trap 'rm -f "$bundle"' EXIT
printf '%s%s' "$program_id" "$program_id" > "$bundle"
echo "grep -c result (incorrect occurrence count):"
grep -a -c -- "$program_id" "$bundle" || true
echo "grep -oF | wc -l result (correct occurrence count):"
(grep -a -oF -- "$program_id" "$bundle" || true) | wc -lRepository: Romulus-Sol/DMV
Length of output: 254
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- workflow context ---'
sed -n '112,145p' .github/workflows/ci.yml
printf '%s\n' '--- all hits references ---'
rg -n -C 3 'hits=|program_id|bundle' .github/workflows/ci.ymlRepository: Romulus-Sol/DMV
Length of output: 5265
Count program ID occurrences, not matching lines.
grep -c counts matching lines. If one line contains two program ID values, duplicate module content passes the check. Count fixed-string matches with grep -a -oF and wc -l.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 132, Update the hits calculation in the CI
workflow to count each fixed-string occurrence of program_id rather than
matching lines: use grep’s only-matching and fixed-string options, then pipe the
results to wc -l while preserving the existing no-match handling.
The previous assertion claimed the bundle proved no module was duplicated in the graph. It proved neither half of that. First, `grep -c` counts matching LINES, not matches. In binary bytecode two occurrences separated by no newline return 1. The negative test that was supposed to catch this only passed because `echo` inserted a newline between the two copies — it validated the assumption instead of challenging it. Second, and more fundamentally, Hermes interns identical string literals into a shared string table. A duplicated module still contributes one interned string, so no occurrence count taken from bytecode can evidence module-instance uniqueness. Fixing the counting would have produced a correct number supporting an unsupportable conclusion. Now a presence check: app source reached the graph, so this is not an RN-runtime-only artifact. That is what the bytecode can actually attest. The job's real value is unchanged and was never in the count: it produces the Hermes bytecode the APK embeds, so a resolution or Hermes-compile regression fails in CI rather than on a phone. Module-instance uniqueness needs the source-map module list. Left for a separate check rather than approximated here.
Romulus-Sol
added a commit
that referenced
this pull request
Aug 1, 2026
STATUS.md asserted that the Phase 4 bundle proved agentFundingPolicy is instantiated once rather than duplicated by its two import forms, and that marker strings appear exactly once. Both rested on counting string occurrences in the Hermes bytecode, which cannot evidence either claim: grep -c counts matching lines rather than matches, and Hermes interns identical string literals into a shared string table, so a duplicated module still contributes one interned string. Re-measured against the artifact that can actually answer it — the source-map module list. 1,528 modules, 116 app source, zero duplicated module paths. agentFundingPolicy resolves to a single path, so the two import forms do dedupe. The conclusion was right; the original evidence for it was not. The bundle line now claims only what bytecode can attest: the graph bundles and Hermes-compiles, and Metro resolves the explicit .ts specifier the Node test runner requires. The method note is kept deliberately, so the next person does not repeat the same inference. Same correction applied to the CI job in PR #52 (7fb82a3). Docs only.
Romulus-Sol
added a commit
that referenced
this pull request
Aug 3, 2026
Brings in the Metro/Hermes bundle CI job (#52, squashed as 39cc3ef) so PR #51 is actually gated by it — pull_request runs use the workflow from the PR's own branch, so the job could not appear on #51 until devnet was merged in here. No application code, test plan or Phase 4 history is altered by this merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
tscandnode --testboth run under Node's module resolver. Metro and Hermes do not.This repo has shipped three releases where type-checking was green and the bundle was broken on device:
undefinedunder HermesNo job in the existing pipeline can observe that class of bug. Every one of those reached a phone.
What it does
Produces the actual Hermes bytecode the APK embeds, then asserts:
.hbcis emitted;c61fbc03c103191f) — so Metro silently emitting plain JS fails rather than passing as a "successful export";release.manifest.jsonis present in the bytecode, so this is not an RN-runtime-only artifact built from a graph that dropped our code.Each failure path was verified to trip, with a distinct message, before committing.
Correction made during review (
7fb82a3)The first version of assertion 3 counted occurrences and claimed the bundle proved no module was duplicated in the graph. That was wrong twice over, and the correction is worth recording:
grep -ccounts matching lines, not matches. In binary bytecode, two occurrences with no newline between them return1. The negative test meant to catch this only passed becauseechoinserted a newline — it validated the assumption rather than challenging it.Assertion 3 is now a presence check — what the bytecode can actually attest. The job's value was never in the count.
Module-instance uniqueness needs the source-map module list, which is a separate check and deliberately not approximated here.
What it is not
Not an APK build. No Gradle, no native code, no signing, no artifact published, not a release path. The authoritative release path remains
release-tools/build-android-release.sh. Runtime ~90s.Note on
.envDeliberately absent in CI, and the job needs no secrets.
EXPO_PUBLIC_*values do not inline into the bundle — verified: the.hbccontent hash is byte-identical with and without.env, which is exactly whysrc/utils/rpcConfig.tsderives them at runtime.Follow-up
To make this gate the Phase 4 PR (#51),
devnetmust be merged intophase4-transactional-heartbeatafter this lands —pull_requestruns use the workflow from the PR's own branch.