Open
Conversation
Contributor
Author
|
/hold |
Contributor
|
@coderabbitai full review |
1 similar comment
Contributor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This requires a rebasebot to support new flag: --conflict-policy strict Signed-off-by: Michal Pryc <mpryc@redhat.com>
kaovilai
force-pushed
the
rebasebot-fix-silent-commit
branch
from
b355855 to
d40a047
Compare
Contributor
|
openshift-eng/rebasebot#85 has now been opened in-place of a rebased 78 |
Contributor
|
85 closed due to rebased 78 |
…or better handling of malformed unicode characters Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
run-oadp-rebase.sh (1)
326-337:⚠️ Potential issue | 🟡 MinorRemove duplicate
--conflict-policy strictflags from command templates.The
--conflict-policy strictoption is already specified earlier in both command templates (line 327 in local mode, line 421 in container mode), then appended again at lines 336–337 and 430–431. This duplication should be cleaned up.Suggested cleanup
- --github-cloner-key \"$SECRETS_DIR/oadp-rebasebot-cloner-key\" \ - --conflict-policy strict" + --github-cloner-key \"$SECRETS_DIR/oadp-rebasebot-cloner-key\""- --github-cloner-key /secrets/oadp-rebasebot-cloner-key \ - --conflict-policy strict" + --github-cloner-key /secrets/oadp-rebasebot-cloner-key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run-oadp-rebase.sh` around lines 326 - 337, The CMD command template includes a duplicated flag "--conflict-policy strict"; remove the redundant occurrence so the CMD variable (the rebasebot command construction) only contains a single "--conflict-policy strict" option (keep the first instance and delete the trailing duplicate near the end of the CMD string), and apply the same cleanup to the container-mode CMD template to ensure neither CMD for local or container mode repeats the flag.
🧹 Nitpick comments (1)
run-oadp-rebase.sh (1)
506-508: Avoid coupling the PR summary to rebasebot's current log wording.This grep is keyed to free-form
INFOmessages from another project, so a small wording or log-level change will silently empty the summary. A stable marker or machine-readable output would make this much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run-oadp-rebase.sh` around lines 506 - 508, The current extraction using grep on rebase_output_file for free-form INFO lines is brittle; change the producer/consumer to use a stable machine-readable marker (e.g., have rebasebot emit a dedicated tag or JSON line like "REBASEBOT_PR_STATUS:" or a JSON object) and then update the consumer here to parse that marker instead of matching human log text; modify the rebase output generation to include that marker and replace the grep/sed on rebase_output_file -> pr_summary_file with a deterministic extractor that looks for the marker (with a safe fallback to legacy parsing if needed) so pr_summary_file is not tied to informal INFO wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rebasebot-hook-scripts/fix-malformed-filenames-and-commit.sh`:
- Around line 37-40: Before running git mv in the branch that checks if "$file"
!= "$clean_name", add a collision guard that detects if any other tracked path
already maps to the same "$clean_name" (e.g., using git ls-files or similar) or
if "$clean_name" already exists and is not the same as "$file"; if a collision
is detected, print a clear error mentioning both conflicting paths and exit
non-zero instead of attempting git mv to avoid partial renames. Update the block
around the existing variables "file" and "clean_name" to perform this check
prior to mkdir -p and git mv, and ensure the error message is deterministic and
aborts early so the hook does not continue under set -e.
---
Outside diff comments:
In `@run-oadp-rebase.sh`:
- Around line 326-337: The CMD command template includes a duplicated flag
"--conflict-policy strict"; remove the redundant occurrence so the CMD variable
(the rebasebot command construction) only contains a single "--conflict-policy
strict" option (keep the first instance and delete the trailing duplicate near
the end of the CMD string), and apply the same cleanup to the container-mode CMD
template to ensure neither CMD for local or container mode repeats the flag.
---
Nitpick comments:
In `@run-oadp-rebase.sh`:
- Around line 506-508: The current extraction using grep on rebase_output_file
for free-form INFO lines is brittle; change the producer/consumer to use a
stable machine-readable marker (e.g., have rebasebot emit a dedicated tag or
JSON line like "REBASEBOT_PR_STATUS:" or a JSON object) and then update the
consumer here to parse that marker instead of matching human log text; modify
the rebase output generation to include that marker and replace the grep/sed on
rebase_output_file -> pr_summary_file with a deterministic extractor that looks
for the marker (with a safe fallback to legacy parsing if needed) so
pr_summary_file is not tied to informal INFO wording.
🪄 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: c8b18457-1b0c-420b-983f-650c5d329e4c
📒 Files selected for processing (2)
rebasebot-hook-scripts/fix-malformed-filenames-and-commit.shrun-oadp-rebase.sh
Comment on lines
37
to
40
| if [[ "$file" != "$clean_name" ]]; then | ||
| echo "Renaming: $file -> $clean_name" | ||
| mkdir -p "$(dirname "$clean_name")" | ||
| git mv "$file" "$clean_name" |
There was a problem hiding this comment.
Guard against normalized-name collisions before git mv.
If two tracked paths normalize to the same clean_name, Line 40 will fail and abort the hook (set -e), potentially after partial renames. Add an explicit collision check and fail with a clear message.
Suggested patch
+declare -A seen_targets=()
while IFS= read -r -d '' file; do
clean_name="$file"
clean_name="${clean_name//$LRM/}"
clean_name="${clean_name//$RLM/}"
clean_name="${clean_name//$ZWSP/}"
if [[ "$file" != "$clean_name" ]]; then
+ if [[ -n "${seen_targets[$clean_name]:-}" ]]; then
+ echo "Collision after normalization: '$file' -> '$clean_name'" >&2
+ exit 1
+ fi
+ if [[ -e "$clean_name" && "$file" != "$clean_name" ]]; then
+ echo "Target already exists after normalization: '$file' -> '$clean_name'" >&2
+ exit 1
+ fi
+ seen_targets["$clean_name"]=1
echo "Renaming: $file -> $clean_name"
mkdir -p "$(dirname "$clean_name")"
git mv "$file" "$clean_name"
found=1
fi
done < <(git ls-files -z)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rebasebot-hook-scripts/fix-malformed-filenames-and-commit.sh` around lines 37
- 40, Before running git mv in the branch that checks if "$file" !=
"$clean_name", add a collision guard that detects if any other tracked path
already maps to the same "$clean_name" (e.g., using git ls-files or similar) or
if "$clean_name" already exists and is not the same as "$file"; if a collision
is detected, print a clear error mentioning both conflicting paths and exit
non-zero instead of attempting git mv to avoid partial renames. Update the block
around the existing variables "file" and "clean_name" to perform this check
prior to mkdir -p and git mv, and ensure the error message is deterministic and
aborts early so the hook does not continue under set -e.
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.
This requires a rebasebot to support new flag:
--conflict-policy strict
Requires PR: openshift-eng/rebasebot#78
Summary by CodeRabbit
Chores
Bug Fixes
User-facing Reporting