Add AI skills for rebase rules automation#681
Conversation
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
RomanNikitenko
force-pushed
the
add-validate-rebase-rules-skill
branch
from
60a4fe4 to
9f0c211
Compare
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
1 similar comment
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
RomanNikitenko
force-pushed
the
add-validate-rebase-rules-skill
branch
from
9f0c211 to
2b4e80b
Compare
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
This PR contains changes to files in directories that are typically not intended to be committed:
Please verify these changes are intentional. |
RomanNikitenko
force-pushed
the
add-validate-rebase-rules-skill
branch
4 times, most recently
from
497029b to
78471f2
Compare
RomanNikitenko
changed the title
RomanNikitenko
requested review from
azatsarynnyy,
rgrunber and
vitaliy-guliy
as code owners
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
1 similar comment
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
|
Warning Review limit reached
More reviews will be available in 44 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds documentation for the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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: 4
♻️ Duplicate comments (2)
.claude/skills/validate-rebase-rules/SKILL.md (1)
19-34:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftValidation will succeed against configured version, but rebase.sh won't use it (lines 19-34)
This skill correctly reads
CURRENT_UPSTREAM_VERSIONfromrebase.shand validates rules against it. However, due to the hardcoded version inrebase.shline 582, the validation will pass but the actual rebase will fail or use the wrong version.The critical fix on
rebase.shline 582 must be made before this skill's validations will be meaningful.🤖 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 @.claude/skills/validate-rebase-rules/SKILL.md around lines 19 - 34, The skill reads CURRENT_UPSTREAM_VERSION/PREVIOUS_UPSTREAM_VERSION from rebase.sh but rebase.sh still contains a hardcoded upstream ref that overrides it; edit rebase.sh to remove the hardcoded ref and reference the parsed variable instead (use CURRENT_UPSTREAM_VERSION where the hardcoded upstream-code/<version> is used), ensure any git show/fetch invocations use upstream-code/${CURRENT_UPSTREAM_VERSION}:<path> (and similarly use PREVIOUS_UPSTREAM_VERSION where appropriate) so the validation and actual rebase use the same version..claude/skills/fix-rebase-rules/SKILL.md (1)
28-41:⚠️ Potential issue | 🔴 CriticalFixes will target wrong upstream version if rebase.sh line 582 is not fixed (lines 28-41)
This skill fetches upstream files at the configured
CURRENT_UPSTREAM_VERSION. However, if the critical issue inrebase.shline 582 (hardcoded version) is not fixed, the fixes generated here will be validated against the wrong upstream version during the actual rebase.All fixes created by this skill depend on the
rebase.shfix being applied first.🤖 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 @.claude/skills/fix-rebase-rules/SKILL.md around lines 28 - 41, The skill currently fetches upstream files using CURRENT_UPSTREAM_VERSION and PREVIOUS_UPSTREAM_VERSION extracted from rebase.sh, but if rebase.sh still contains the hardcoded upstream version at line 582 the skill will target the wrong release; first update rebase.sh to remove the hardcoded version at that location so it uses the CURRENT_UPSTREAM_VERSION variable, then ensure this skill uses those two variables (CURRENT_UPSTREAM_VERSION and PREVIOUS_UPSTREAM_VERSION) when constructing git show invocations (the logic referenced in the Step 1 text) so all file fetches are validated against the intended upstream version.
🧹 Nitpick comments (1)
.claude/skills/rebase/SKILL.md (1)
103-103: ⚡ Quick winAdd language specifiers to fenced code blocks.
Lines 103, 365, and 370 contain code blocks without language identifiers. Add
```bashor```yamlas appropriate.Also applies to: 365-365, 370-370
🤖 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 @.claude/skills/rebase/SKILL.md at line 103, Several fenced code blocks in SKILL.md are missing language specifiers (notably the blocks around lines referenced in the review); update those triple-backtick fences to include the correct language identifiers (e.g., change ``` to ```bash or ```yaml as appropriate) so syntax highlighting works for the code samples in .claude/skills/rebase/SKILL.md (ensure you update the fences at the three reported locations).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 @.claude/skills/add-rebase-rules/SKILL.md:
- Around line 59-79: The sed encoding table in SKILL.md is inaccurate:
rebase.sh's escape_litteral() only escapes \$, \[ and \], it does not escape &,
*, " or encode newlines/tabs; update SKILL.md or the script to make them
consistent. Fix options: 1) Update the Sed encoding section in SKILL.md to list
only the characters actually escaped by escape_litteral() (show that & and * are
not escaped and that newlines/tabs must be encoded earlier), and correct the
guidance about escaping `&` in sed replacement strings (advise using `\\&` in by
values); or 2) Modify rebase.sh's escape_litteral() to also escape `&`, `*`, `"`
and perform newline/tab encoding (and ensure tests/usage of apply_changes still
work). Reference rebase.sh and the escape_litteral() function and the sed
encoding table in SKILL.md when making the change.
In @.claude/skills/test-rebase-rules/run-all-tests.sh:
- Around line 215-243: The script currently silently ignores FILTER_FILES that
don't match any entry in MAPPINGS; modify the logic around MAPPINGS and the
for-loop that iterates over "${MAPPINGS[@]}" so that you track which
FILTER_FILES were matched (e.g., maintain a MATCHED_FILTERS set/array when you
set match=true for a given filter) and after the loop check for any FILTER_FILES
that were never matched; if any exist, print a clear error including the unknown
filter paths and exit non-zero (exit 1) so typos or stale paths fail fast.
Ensure the comparison remains the exact filename match used in the IFS='|' read
for file_path and do not change existing test_file invocation or the normal
successful flow when all filters are valid.
In @.claude/skills/test-rebase-rules/test-rebase-handler.sh:
- Around line 23-25: The wrapper currently evals the rebase handlers and then
blindly executes whatever is passed as "$@", which can run arbitrary binaries;
change the script to validate that there is at least one arg, ensure the first
argument is a sourced function by using declare -F "$1", and constrain "$1" to
the expected handler-name regex (e.g., the project's handler prefix pattern)
before dispatching; if the check fails, print an error and exit non‑zero,
otherwise call the handler with "$@" (this change affects the
test-rebase-handler.sh invocation logic around the eval and the subsequent "$@"
dispatch).
In `@rebase.sh`:
- Around line 16-17: The script rebase.sh currently hardcodes the upstream ref
when resolving the commit (uses "upstream-code/release/1.108") which breaks the
contract with PREVIOUS_UPSTREAM_VERSION and CURRENT_UPSTREAM_VERSION; update the
git rev-parse invocation to use the CURRENT_UPSTREAM_VERSION variable (e.g., git
rev-parse "upstream-code/${CURRENT_UPSTREAM_VERSION}") so the script honors the
configured CURRENT_UPSTREAM_VERSION variable and the skills' expectations.
---
Duplicate comments:
In @.claude/skills/fix-rebase-rules/SKILL.md:
- Around line 28-41: The skill currently fetches upstream files using
CURRENT_UPSTREAM_VERSION and PREVIOUS_UPSTREAM_VERSION extracted from rebase.sh,
but if rebase.sh still contains the hardcoded upstream version at line 582 the
skill will target the wrong release; first update rebase.sh to remove the
hardcoded version at that location so it uses the CURRENT_UPSTREAM_VERSION
variable, then ensure this skill uses those two variables
(CURRENT_UPSTREAM_VERSION and PREVIOUS_UPSTREAM_VERSION) when constructing git
show invocations (the logic referenced in the Step 1 text) so all file fetches
are validated against the intended upstream version.
In @.claude/skills/validate-rebase-rules/SKILL.md:
- Around line 19-34: The skill reads
CURRENT_UPSTREAM_VERSION/PREVIOUS_UPSTREAM_VERSION from rebase.sh but rebase.sh
still contains a hardcoded upstream ref that overrides it; edit rebase.sh to
remove the hardcoded ref and reference the parsed variable instead (use
CURRENT_UPSTREAM_VERSION where the hardcoded upstream-code/<version> is used),
ensure any git show/fetch invocations use
upstream-code/${CURRENT_UPSTREAM_VERSION}:<path> (and similarly use
PREVIOUS_UPSTREAM_VERSION where appropriate) so the validation and actual rebase
use the same version.
---
Nitpick comments:
In @.claude/skills/rebase/SKILL.md:
- Line 103: Several fenced code blocks in SKILL.md are missing language
specifiers (notably the blocks around lines referenced in the review); update
those triple-backtick fences to include the correct language identifiers (e.g.,
change ``` to ```bash or ```yaml as appropriate) so syntax highlighting works
for the code samples in .claude/skills/rebase/SKILL.md (ensure you update the
fences at the three reported locations).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfc0dc6b-6ddf-4858-9467-d0c70be91856
📒 Files selected for processing (10)
.claude/skills/add-rebase-rules/SKILL.md.claude/skills/dependency-rebase-rules/SKILL.md.claude/skills/fix-rebase-rules/SKILL.md.claude/skills/rebase/SKILL.md.claude/skills/rebase/rebase-config.yaml.claude/skills/test-rebase-rules/SKILL.md.claude/skills/test-rebase-rules/run-all-tests.sh.claude/skills/test-rebase-rules/test-rebase-handler.sh.claude/skills/validate-rebase-rules/SKILL.mdrebase.sh
| FILTER_FILES=("${@+"$@"}") | ||
|
|
||
| MAPPINGS=() | ||
| while IFS= read -r line; do | ||
| MAPPINGS+=("$line") | ||
| done < <(parse_handlers) | ||
|
|
||
| total=${#MAPPINGS[@]} | ||
| i=1 | ||
| for mapping in "${MAPPINGS[@]}"; do | ||
| IFS='|' read -r file_path handler handler_arg <<< "$mapping" | ||
|
|
||
| if [ ${#FILTER_FILES[@]} -gt 0 ]; then | ||
| match=false | ||
| for filter in "${FILTER_FILES[@]}"; do | ||
| if [[ "$file_path" == "$filter" ]]; then | ||
| match=true | ||
| break | ||
| fi | ||
| done | ||
| if [ "$match" = false ]; then | ||
| continue | ||
| fi | ||
| fi | ||
|
|
||
| echo "[$i/$total] $file_path ($handler)..." | ||
| test_file "$file_path" "$handler" "$handler_arg" | ||
| ((i++)) | ||
| done |
There was a problem hiding this comment.
Unknown filter paths currently produce a false-success no-op.
If a requested file is not present in MAPPINGS, the loop just skips it and the script can finish with 0 passed, 0 warnings, 0 failed, 0 skipped. That hides typos and stale examples instead of failing fast. With the current upstream dispatch in rebase.sh, this matters immediately for paths like code/src/server-main.ts, because the actual mapping is code/src/server-main.js.
Suggested fix
FILTER_FILES=("${@+"$@"}")
MAPPINGS=()
while IFS= read -r line; do
MAPPINGS+=("$line")
done < <(parse_handlers)
+
+if [ ${`#FILTER_FILES`[@]} -gt 0 ]; then
+ known_files="$(printf '%s\n' "${MAPPINGS[@]}" | cut -d'|' -f1)"
+ for filter in "${FILTER_FILES[@]}"; do
+ if ! printf '%s\n' "$known_files" | grep -qxF "$filter"; then
+ echo "ERROR: No handler mapping found for $filter" >&2
+ exit 1
+ fi
+ done
+fi
total=${`#MAPPINGS`[@]}
i=1
for mapping in "${MAPPINGS[@]}"; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FILTER_FILES=("${@+"$@"}") | |
| MAPPINGS=() | |
| while IFS= read -r line; do | |
| MAPPINGS+=("$line") | |
| done < <(parse_handlers) | |
| total=${#MAPPINGS[@]} | |
| i=1 | |
| for mapping in "${MAPPINGS[@]}"; do | |
| IFS='|' read -r file_path handler handler_arg <<< "$mapping" | |
| if [ ${#FILTER_FILES[@]} -gt 0 ]; then | |
| match=false | |
| for filter in "${FILTER_FILES[@]}"; do | |
| if [[ "$file_path" == "$filter" ]]; then | |
| match=true | |
| break | |
| fi | |
| done | |
| if [ "$match" = false ]; then | |
| continue | |
| fi | |
| fi | |
| echo "[$i/$total] $file_path ($handler)..." | |
| test_file "$file_path" "$handler" "$handler_arg" | |
| ((i++)) | |
| done | |
| FILTER_FILES=("${@+"$@"}") | |
| MAPPINGS=() | |
| while IFS= read -r line; do | |
| MAPPINGS+=("$line") | |
| done < <(parse_handlers) | |
| if [ ${`#FILTER_FILES`[@]} -gt 0 ]; then | |
| known_files="$(printf '%s\n' "${MAPPINGS[@]}" | cut -d'|' -f1)" | |
| for filter in "${FILTER_FILES[@]}"; do | |
| if ! printf '%s\n' "$known_files" | grep -qxF "$filter"; then | |
| echo "ERROR: No handler mapping found for $filter" >&2 | |
| exit 1 | |
| fi | |
| done | |
| fi | |
| total=${`#MAPPINGS`[@]} | |
| i=1 | |
| for mapping in "${MAPPINGS[@]}"; do | |
| IFS='|' read -r file_path handler handler_arg <<< "$mapping" | |
| if [ ${`#FILTER_FILES`[@]} -gt 0 ]; then | |
| match=false | |
| for filter in "${FILTER_FILES[@]}"; do | |
| if [[ "$file_path" == "$filter" ]]; then | |
| match=true | |
| break | |
| fi | |
| done | |
| if [ "$match" = false ]; then | |
| continue | |
| fi | |
| fi | |
| echo "[$i/$total] $file_path ($handler)..." | |
| test_file "$file_path" "$handler" "$handler_arg" | |
| ((i++)) | |
| 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 @.claude/skills/test-rebase-rules/run-all-tests.sh around lines 215 - 243,
The script currently silently ignores FILTER_FILES that don't match any entry in
MAPPINGS; modify the logic around MAPPINGS and the for-loop that iterates over
"${MAPPINGS[@]}" so that you track which FILTER_FILES were matched (e.g.,
maintain a MATCHED_FILTERS set/array when you set match=true for a given filter)
and after the loop check for any FILTER_FILES that were never matched; if any
exist, print a clear error including the unknown filter paths and exit non-zero
(exit 1) so typos or stale paths fail fast. Ensure the comparison remains the
exact filename match used in the IFS='|' read for file_path and do not change
existing test_file invocation or the normal successful flow when all filters are
valid.
| eval "$(sed -n '1,/^# perform rebase/p' rebase.sh | grep -v '^set -[eu]$')" | ||
|
|
||
| "$@" |
There was a problem hiding this comment.
Restrict execution to sourced rebase handlers before invoking "$@".
This wrapper is documented as a handler runner, but today it will execute any command line it receives after the eval. In an AI-driven flow, a typo or prompt-influenced argument can run an arbitrary binary on the operator machine instead of failing fast. Validate "$1" with declare -F and restrict it to the expected handler naming pattern before dispatch.
Suggested fix
eval "$(sed -n '1,/^# perform rebase/p' rebase.sh | grep -v '^set -[eu]$')"
-"$@"
+if [ "$#" -lt 1 ] || ! declare -F "$1" >/dev/null 2>&1 || [[ ! "$1" =~ ^apply_ ]]; then
+ echo "Usage: bash test-rebase-handler.sh <rebase-handler-function> [args...]" >&2
+ exit 2
+fi
+
+"$@"🤖 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 @.claude/skills/test-rebase-rules/test-rebase-handler.sh around lines 23 -
25, The wrapper currently evals the rebase handlers and then blindly executes
whatever is passed as "$@", which can run arbitrary binaries; change the script
to validate that there is at least one arg, ensure the first argument is a
sourced function by using declare -F "$1", and constrain "$1" to the expected
handler-name regex (e.g., the project's handler prefix pattern) before
dispatching; if the check fails, print an error and exit non‑zero, otherwise
call the handler with "$@" (this change affects the test-rebase-handler.sh
invocation logic around the eval and the subsequent "$@" dispatch).
| PREVIOUS_UPSTREAM_VERSION="release/1.104" | ||
| CURRENT_UPSTREAM_VERSION="release/1.108" |
There was a problem hiding this comment.
Root cause: rebase.sh hardcodes the upstream version; skills assume it uses variables (critical — blocks PR functionality)
The PR adds PREVIOUS_UPSTREAM_VERSION and CURRENT_UPSTREAM_VERSION to rebase.sh (lines 16–17) to support skills in reading the configured version. However, rebase.sh line 582 still hardcodes git rev-parse upstream-code/release/1.108 instead of using ${CURRENT_UPSTREAM_VERSION}. This creates a critical contract failure:
- Add-rebase-rules, validate-rebase-rules, and fix-rebase-rules all fetch upstream files at the version stored in
CURRENT_UPSTREAM_VERSION(skill docs at.claude/skills/rebase/SKILL.md#L27, etc.) - rebase.sh itself always rebases to
1.108, ignoring what the skills prepared - Result: Skills validate and fix rules against one version, but
rebase.shapplies them to a different version, causing silent mismatches or rule failures
The past review comment already flagged this exact issue. Update line 582 in rebase.sh:
- UPSTREAM_VERSION=$(git rev-parse upstream-code/release/1.108)
+ UPSTREAM_VERSION=$(git rev-parse "upstream-code/${CURRENT_UPSTREAM_VERSION}")This is a blocker; without it, the rebase skill workflow will not function correctly.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 16-16: PREVIOUS_UPSTREAM_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 17-17: CURRENT_UPSTREAM_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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 `@rebase.sh` around lines 16 - 17, The script rebase.sh currently hardcodes the
upstream ref when resolving the commit (uses "upstream-code/release/1.108")
which breaks the contract with PREVIOUS_UPSTREAM_VERSION and
CURRENT_UPSTREAM_VERSION; update the git rev-parse invocation to use the
CURRENT_UPSTREAM_VERSION variable (e.g., git rev-parse
"upstream-code/${CURRENT_UPSTREAM_VERSION}") so the script honors the configured
CURRENT_UPSTREAM_VERSION variable and the skills' expectations.
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
1 similar comment
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
RomanNikitenko
force-pushed
the
add-validate-rebase-rules-skill
branch
from
78471f2 to
83e00d5
Compare
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com> Generated-by: Cursor AI
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com> Assisted-by: Cursor AI
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 @.claude/skills/validate-rebase-rules/SKILL.md:
- Around line 68-80: The validation logic for `.rebase/add/` rules should treat
any upstream key that now exists with a different value as an ERROR instead of a
WARNING, because the `jq` merge in the add-rule path still silently prefers the
add rule. Update the rule-checking flow in the rebase validation code that
inspects leaf keys from add-rule JSON and compares them against
`CURRENT_UPSTREAM_VERSION`, so conflicting values are escalated, while absent
keys remain OK and identical values can still be flagged as redundant.
- Around line 12-17: The remote setup check only verifies that upstream-code
exists, but it can still point to the wrong repository. Update the validation in
the SKILL.md guidance so the upstream-code remote is explicitly compared against
the expected VS Code URL in addition to fetching it, and if it differs, instruct
the user to reset or replace that remote before any later git show checks. Refer
to the upstream-code remote setup block so the fix stays anchored to the
existing remote validation step.
- Around line 50-60: The escape-handling guidance in validate-rebase-rules is
wrong: the notes around the `from`/`by` checks and `jq -r` incorrectly describe
a custom escape convention that `jq` does not perform. Update the wording in the
SKILL.md section that describes matching `from` and `by` so it consistently uses
standard JSON escaping, or change the implementation guidance to explicitly
describe any custom decoder used before comparison. Keep the instructions
aligned with the existing `from`/`by` validation flow and the `jq -r` examples.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d71c7f30-0c34-488b-bb62-0e5adcb83bfd
📒 Files selected for processing (1)
.claude/skills/validate-rebase-rules/SKILL.md
| Ensure the `upstream-code` remote exists and is fetched: | ||
|
|
||
| ```bash | ||
| git remote get-url upstream-code || git remote add upstream-code https://github.com/microsoft/vscode | ||
| git fetch upstream-code | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pin upstream-code to the expected VS Code remote.
This only checks that the remote exists. If it already points somewhere else, every later git show will validate against the wrong upstream.
Suggested fix
-git remote get-url upstream-code || git remote add upstream-code https://github.com/microsoft/vscode
+git remote get-url upstream-code >/dev/null 2>&1 || git remote add upstream-code https://github.com/microsoft/vscode
+git remote get-url upstream-code | grep -qx 'https://github.com/microsoft/vscode'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ensure the `upstream-code` remote exists and is fetched: | |
| ```bash | |
| git remote get-url upstream-code || git remote add upstream-code https://github.com/microsoft/vscode | |
| git fetch upstream-code | |
| ``` | |
| Ensure the `upstream-code` remote exists and is fetched: | |
🧰 Tools
🪛 SkillSpector (2.2.3)
[error] 136: [MP3] Memory Manipulation: Skill manipulates agent memory, state, or stored context. Memory corruption can alter personality, override safety rules, or cause unpredictable behavior.
Remediation: Protect agent memory and state from modification by untrusted content. Use read-only memory for critical instructions and validate all state changes.
(Memory Poisoning (MP3))
[error] 192: [MP3] Memory Manipulation: Skill manipulates agent memory, state, or stored context. Memory corruption can alter personality, override safety rules, or cause unpredictable behavior.
Remediation: Protect agent memory and state from modification by untrusted content. Use read-only memory for critical instructions and validate all state changes.
(Memory Poisoning (MP3))
🤖 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 @.claude/skills/validate-rebase-rules/SKILL.md around lines 12 - 17, The
remote setup check only verifies that upstream-code exists, but it can still
point to the wrong repository. Update the validation in the SKILL.md guidance so
the upstream-code remote is explicitly compared against the expected VS Code URL
in addition to fetching it, and if it differs, instruct the user to reset or
replace that remote before any later git show checks. Refer to the upstream-code
remote setup block so the fix stays anchored to the existing remote validation
step.
| The purpose of `add` rules is to add keys that **do not exist** in upstream. If a key now exists in upstream, the rule may be redundant or causing a silent override. | ||
|
|
||
| **For each file in `.rebase/add/`:** | ||
|
|
||
| 1. Identify the upstream file path (strip `code/` prefix). | ||
| 2. Read the add rule JSON. | ||
| 3. Fetch the upstream file at `CURRENT_UPSTREAM_VERSION`. | ||
| 4. For every leaf key/value in the add rule, check if that key exists in the upstream file: | ||
| - **Key absent in upstream** → OK, the add rule is still needed. | ||
| - **Key exists with the same value** → WARNING: add rule is redundant, upstream already has this value. Consider removing it. | ||
| - **Key exists with a different value** → WARNING: the add rule silently overrides the upstream value. This is a potential conflict. | ||
| - For version-like values (semver): if upstream version ≥ add-rule version → the add rule may be downgrading. Report as WARNING. | ||
| - For non-version values: report the upstream value vs add-rule value for manual review. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escalate conflicting add rules to ERROR.
If upstream now contains the key with a different value, the jq merge still prefers the add rule. That is a silent behavior change, so a WARNING understates the impact.
Suggested fix
- - **Key exists with a different value** → WARNING: the add rule silently overrides the upstream value. This is a potential conflict.
+ - **Key exists with a different value** → ERROR: the add rule silently overrides the upstream value and must be reviewed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The purpose of `add` rules is to add keys that **do not exist** in upstream. If a key now exists in upstream, the rule may be redundant or causing a silent override. | |
| **For each file in `.rebase/add/`:** | |
| 1. Identify the upstream file path (strip `code/` prefix). | |
| 2. Read the add rule JSON. | |
| 3. Fetch the upstream file at `CURRENT_UPSTREAM_VERSION`. | |
| 4. For every leaf key/value in the add rule, check if that key exists in the upstream file: | |
| - **Key absent in upstream** → OK, the add rule is still needed. | |
| - **Key exists with the same value** → WARNING: add rule is redundant, upstream already has this value. Consider removing it. | |
| - **Key exists with a different value** → WARNING: the add rule silently overrides the upstream value. This is a potential conflict. | |
| - For version-like values (semver): if upstream version ≥ add-rule version → the add rule may be downgrading. Report as WARNING. | |
| - For non-version values: report the upstream value vs add-rule value for manual review. | |
| The purpose of `add` rules is to add keys that **do not exist** in upstream. If a key now exists in upstream, the rule may be redundant or causing a silent override. | |
| **For each file in `.rebase/add/`:** | |
| 1. Identify the upstream file path (strip `code/` prefix). | |
| 2. Read the add rule JSON. | |
| 3. Fetch the upstream file at `CURRENT_UPSTREAM_VERSION`. | |
| 4. For every leaf key/value in the add rule, check if that key exists in the upstream file: | |
| - **Key absent in upstream** → OK, the add rule is still needed. | |
| - **Key exists with the same value** → WARNING: add rule is redundant, upstream already has this value. Consider removing it. | |
| - **Key exists with a different value** → ERROR: the add rule silently overrides the upstream value and must be reviewed. | |
| - For version-like values (semver): if upstream version ≥ add-rule version → the add rule may be downgrading. Report as WARNING. | |
| - For non-version values: report the upstream value vs add-rule value for manual review. |
🧰 Tools
🪛 SkillSpector (2.2.3)
[error] 136: [MP3] Memory Manipulation: Skill manipulates agent memory, state, or stored context. Memory corruption can alter personality, override safety rules, or cause unpredictable behavior.
Remediation: Protect agent memory and state from modification by untrusted content. Use read-only memory for critical instructions and validate all state changes.
(Memory Poisoning (MP3))
[error] 192: [MP3] Memory Manipulation: Skill manipulates agent memory, state, or stored context. Memory corruption can alter personality, override safety rules, or cause unpredictable behavior.
Remediation: Protect agent memory and state from modification by untrusted content. Use read-only memory for critical instructions and validate all state changes.
(Memory Poisoning (MP3))
🤖 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 @.claude/skills/validate-rebase-rules/SKILL.md around lines 68 - 80, The
validation logic for `.rebase/add/` rules should treat any upstream key that now
exists with a different value as an ERROR instead of a WARNING, because the `jq`
merge in the add-rule path still silently prefers the add rule. Update the
rule-checking flow in the rebase validation code that inspects leaf keys from
add-rule JSON and compares them against `CURRENT_UPSTREAM_VERSION`, so
conflicting values are escalated, while absent keys remain OK and identical
values can still be flagged as redundant.
83e00d5 to
e2bee76
Compare
|
@rgrunber @azatsarynnyy @sbouchet wdyt? |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-681-amd64 |
rgrunber
left a comment
rgrunber
left a comment
There was a problem hiding this comment.
This seems fine to me.
|
also seems fine. |
|
|
||
| Over time, upstream may adopt the same or newer versions, making some pins redundant or harmful: | ||
|
|
||
| - **npm EOVERRIDE** — when an override version conflicts with a direct dependency version |
There was a problem hiding this comment.
typo
| - **npm EOVERRIDE** — when an override version conflicts with a direct dependency version | |
| - **npm OVERRIDE** — when an override version conflicts with a direct dependency version |
There was a problem hiding this comment.
EOVERRIDE is an npm error code that occurs when there's a conflict between a package version specified as a direct dependency and the same package specified in the overrides section of package.json.
There was a problem hiding this comment.
Concrete example from this project's context:
- Suppose upstream VS Code has devDependencies.tar:
^7.5.9 - Che adds an override overrides.tar:
^7.5.11(to fix a CVE) - npm will fail with
EOVERRIDEbecause^7.5.9(direct dep) !=^7.5.11(override)
4 participants
What does this PR do?
Adds a set of AI skills (for Claude / Cursor) that automate the upstream rebase rules lifecycle:
.rebase/rules against current upstream VS Code; generates a validation report with stalefrom/byvalues, redundant add/override pins, and uncovered Che-specific changesrebase.shhandlers against upstream content and diffs the result against the expected che-code state. Includesrun-all-tests.shandtest-rebase-handler.shscripts.rebase/add/and.rebase/override/(CVE pins, EOVERRIDE conflicts, lock file stability)rebase.shexecution → build verification → PR creationfromvalues, and a completeness verification stepAlso adds
PREVIOUS_UPSTREAM_VERSION/CURRENT_UPSTREAM_VERSIONvariables torebase.sh, used by the skills to fetch correct upstream file content.What issues does this PR fix?
eclipse-che/che#23749
I used these skills for alignments with upstream: #646 and #692
How to test this PR?
validate-rebase-rulesDoes this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderGenerated-by: Cursor AI
Summary by CodeRabbit