Fix silent submodule failure.#6
Open
whisper67265 wants to merge 4 commits intocppalliance:masterfrom
Open
Conversation
📝 WalkthroughWalkthroughPer-submodule processing and error handling in GitHub Actions were hardened: commits are only made when staged changes exist; submodule init/fetch/clone/doc-path failures now surface (fatal rc=2) and are aggregated via a ChangesSubmodule handling, guarded commits, and error propagation
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Runner
participant Workflow as Workflow script
participant Git as git CLI
participant Remote as origin (remote repo)
Note over Runner,Workflow: Workflow starts (start-translation / sync-translation / add-submodules)
Runner->>Workflow: invoke workflow
Workflow->>Git: git submodule update --init
Git-->>Workflow: success / failure
alt init failure
Workflow->>Runner: print error & return rc 1
else init success
Workflow->>Git: for each submodule: check path exists
alt path missing
Workflow->>Runner: warn and skip
else path present
Workflow->>Git: git fetch origin (no suppression)
Git-->>Workflow: fetch result
Workflow->>Git: git submodule update --remote "$sub_path"
Git-->>Workflow: update result (0 / 1 / 2)
alt rc == 0
Workflow->>Workflow: append submodule to UPDATES
else rc == 2
Workflow->>Workflow: increment submodule_fatal
end
end
end
Workflow->>Git: if staged changes -> git commit && git push
Git-->>Workflow: commit result
Workflow->>Runner: if submodule_fatal > 0 -> exit non-zero with summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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/add-submodules.yml:
- Around line 176-187: The loop currently calls process_one_submodule and
immediately relies on rc but set -e causes the script to exit on non-zero
returns; change the call so the return code is captured without triggering
errexit (e.g., invoke process_one_submodule and on failure assign rc from $? ,
then default rc to 0 if unset) and then use rc to decide whether to append to
UPDATES or increment submodule_fatal; alternatively wrap the
process_one_submodule call in a subshell or temporarily disable errexit around
that call to preserve existing rc/submodule_fatal/UPDATES logic.
In @.github/workflows/start-translation.yml:
- Around line 324-335: The loop currently invokes process_one_submodule under
set -e which causes the script to exit before capturing its status; change the
invocation to capture the exit code without triggering errexit (e.g., run
process_one_submodule "$sub" with an explicit || rc=$? or an if ...; then/else
wrapper so the shell does not exit) and then use that rc to decide behavior: if
rc==0 append to UPDATES, if rc==2 increment submodule_fatal; also update
process_one_submodule to explicitly return distinct codes for fatal vs non-fatal
failures (use return 2 for fatal conditions) so the loop can properly count
submodule_fatal and avoid premature exits.
🪄 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: c172c863-6dc5-41d7-bf48-966564ec5013
📒 Files selected for processing (4)
.github/workflows/add-submodules.yml.github/workflows/assets/lib.sh.github/workflows/start-translation.yml.github/workflows/sync-translation.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/add-submodules.yml:
- Around line 171-185: process_one_submodule currently masks failures because
create_new_repo_and_push does not return non-zero on fatal errors; update
create_new_repo_and_push to check each step (create_repo, git commit, git push,
etc.) and explicitly return 2 on unrecoverable failures (matching the pattern
used around the other calls at the earlier checks in the script such as the
checks at lines similar to 112–114 and 118–119), or propagate errors by enabling
set -o errexit within the function scope and wrapping critical commands with
conditional checks that set rc=2 and return 2 on failure so
process_one_submodule’s caller won’t append failed submodules to UPDATES.
In @.github/workflows/start-translation.yml:
- Around line 319-333: The helpers are masking git failures because functions
like sync_repo_master, update_local_merge_from_master,
ensure_local_branch_in_repo, and process_local_branch run git commands without
checking exit codes and process_local_branch currently always returns success;
update these functions so every git command is followed by an explicit check
(e.g., command || return 2) or otherwise test $? and return a non-zero/fatal
status, ensure process_local_branch returns that propagated error instead of
unconditionally 0, and make process_one_submodule treat a returned 2 as fatal so
the aggregation loop can detect and fail fast.
In @.github/workflows/sync-translation.yml:
- Around line 65-72: The loop that checks each submodule path (reading into
sub_path after git submodule update --init) currently logs a warning and uses
continue when a submodule directory is missing; change this to mark failure and
cause the job to fail instead of silently succeeding: either set a failure flag
(e.g., missing_submodules or reuse updated) when [[ ! -d "$sub_path" ]] is true
and continue to gather all missing entries, then after the while loop check that
flag and exit with a non-zero status (exit 1) if any were missing, or
immediately exit 1 inside the if block; reference the variables/labels sub_path,
updated (or your chosen missing_submodules flag), and the existing warning echo
to locate where to change behavior.
🪄 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: 0fe123b9-f054-4193-8553-47d6fff6d945
📒 Files selected for processing (3)
.github/workflows/add-submodules.yml.github/workflows/start-translation.yml.github/workflows/sync-translation.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/add-submodules.yml (1)
135-135:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing error propagation from
create_new_repo_and_push.When
process_one_submoduleis called as the condition ofif(line 185),set -eis disabled for the entire function body. Ifcreate_new_repo_and_pushreturns 2 here, the error is silently ignored andprocess_one_submodulereturns 0, causing a failed submodule to be marked as successfully updated.🐛 Proposed fix
- create_new_repo_and_push "$MODULE_ORG" "$sub_name" "$sub_clone" "$org_repo_url" "$LIBS_REF" + create_new_repo_and_push "$MODULE_ORG" "$sub_name" "$sub_clone" "$org_repo_url" "$LIBS_REF" || return 2🤖 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/add-submodules.yml at line 135, process_one_submodule currently calls create_new_repo_and_push but ignores its exit status when used in an if-context, so failures (e.g., return 2) get swallowed; update process_one_submodule to propagate errors from create_new_repo_and_push by checking its return code and returning a non-zero status on failure (e.g., capture its exit code after the call and if non-zero, return that code or fail the function), ensuring create_new_repo_and_push's failure is not silently treated as success.
🤖 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.
Outside diff comments:
In @.github/workflows/add-submodules.yml:
- Line 135: process_one_submodule currently calls create_new_repo_and_push but
ignores its exit status when used in an if-context, so failures (e.g., return 2)
get swallowed; update process_one_submodule to propagate errors from
create_new_repo_and_push by checking its return code and returning a non-zero
status on failure (e.g., capture its exit code after the call and if non-zero,
return that code or fail the function), ensuring create_new_repo_and_push's
failure is not silently treated as success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fed114c7-6e99-4a74-9547-dcf818243407
📒 Files selected for processing (2)
.github/workflows/add-submodules.yml.github/workflows/start-translation.yml
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.
issue : #10
Summary by CodeRabbit
Bug Fixes
Chores