Physical modularity: extract add-submodules.yml inline logic to shared shell library#42
Merged
wpak-ai merged 3 commits intoJun 30, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInline submodule orchestration is extracted into ChangesShared submodule orchestration refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
tests/test_submodule_ops.bats (2)
101-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the mixed precedence case for exit-code composition.
The suite checks
submodule_fatalandfinalize_rcseparately, but not together. The helper’s contract is that a non-zero finalize code overrides the batch fatal code, so this needs one explicit test.Proposed follow-up
`@test` "combine_batch_and_finalize_rc: finalize rc wins when non-zero" { submodule_fatal=0 run combine_batch_and_finalize_rc 3 [ "$status" -eq 3 ] } + +@test "combine_batch_and_finalize_rc: finalize rc overrides submodule_fatal" { + submodule_fatal=2 + run combine_batch_and_finalize_rc 3 + [ "$status" -eq 3 ] +}🤖 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 `@tests/test_submodule_ops.bats` around lines 101 - 110, Add a new test in combine_batch_and_finalize_rc coverage that exercises the mixed precedence case where both submodule_fatal and finalize_rc are set; use the combine_batch_and_finalize_rc helper to verify that a non-zero finalize_rc takes precedence over the batch fatal exit code. Place it alongside the existing combine_batch_and_finalize_rc tests in tests/test_submodule_ops.bats so the contract is explicitly covered.
72-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the fatal-path return code too.
This test locks in the summary buckets, but not the non-zero status that
process_submodule_listis supposed to emit when any processor returns2. A regression there would still pass this test while changing workflow control flow.Proposed follow-up
`@test` "process_submodule_list: records updates and fatals by exit code" { stub_processor() { case "$1" in ok) return 0 ;; skip) return 1 ;; fail) return 2 ;; esac } process_submodule_list stub_processor ok skip fail [ "${`#UPDATES`[@]}" -eq 1 ] [ "${UPDATES[0]}" = "ok" ] [ "${`#SUBMODULE_FATAL`[@]}" -eq 1 ] [ "${SUBMODULE_FATAL[0]}" = "fail" ] [ "$submodule_fatal" -eq 1 ] } + +@test "process_submodule_list: returns non-zero when any submodule is fatal" { + stub_processor() { + case "$1" in + fail) return 2 ;; + *) return 0 ;; + esac + } + + set +e + process_submodule_list stub_processor ok fail + status=$? + set -e + + [ "$status" -ne 0 ] +}🤖 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 `@tests/test_submodule_ops.bats` around lines 72 - 87, The process_submodule_list test covers the updates and fatal buckets but does not assert the overall non-zero exit status when stub_processor returns 2. Update the existing test case to capture and check the return code from process_submodule_list, alongside the current UPDATES, SUBMODULE_FATAL, and submodule_fatal assertions, so the fatal-path control flow is locked in.
🤖 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/assets/submodule_ops.sh:
- Around line 107-108: The final success path in process_submodule_list is
falling through with a nonzero status because the trailing conditional using
submodule_fatal only runs phase_err on failures and otherwise returns 1. Update
the end of the batch handling in process_submodule_list so it explicitly returns
success when no submodule failures occurred, while still calling phase_err for
the submodule_fatal error case, ensuring callers under set -e do not abort
before finalization.
---
Nitpick comments:
In `@tests/test_submodule_ops.bats`:
- Around line 101-110: Add a new test in combine_batch_and_finalize_rc coverage
that exercises the mixed precedence case where both submodule_fatal and
finalize_rc are set; use the combine_batch_and_finalize_rc helper to verify that
a non-zero finalize_rc takes precedence over the batch fatal exit code. Place it
alongside the existing combine_batch_and_finalize_rc tests in
tests/test_submodule_ops.bats so the contract is explicitly covered.
- Around line 72-87: The process_submodule_list test covers the updates and
fatal buckets but does not assert the overall non-zero exit status when
stub_processor returns 2. Update the existing test case to capture and check the
return code from process_submodule_list, alongside the current UPDATES,
SUBMODULE_FATAL, and submodule_fatal assertions, so the fatal-path control flow
is locked in.
🪄 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: 1c8b0ae7-3c07-453f-bb0b-2c34cd11751e
📒 Files selected for processing (6)
.github/workflows/add-submodules.yml.github/workflows/assets/submodule_ops.sh.github/workflows/start-translation.ymlscripts/lint.shtests/helpers/common.bashtests/test_submodule_ops.bats
henry0816191
approved these changes
Jun 30, 2026
wpak-ai
approved these changes
Jun 30, 2026
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
3 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.
Close #34.
Summary by CodeRabbit
.gitmoduleswhen explicit inputs aren’t provided.