fix: resolve upgrade CI failure from dotagents sync target collision - #1374
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
Mesa DescriptionTL;DRResolved upgrade CI failures by preventing a What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the dotagents submodule and modifies the Makefile to skip the sync target from the included submodule Makefile. A review comment points out that including the submodule Makefile early may override the default goal, suggesting an explicit .DEFAULT_GOAL setting.
| SHELL := bash | ||
|
|
||
| # Include dotagents from submodule but keep the local help target authoritative. | ||
| # Include dotagents from submodule but keep the local targets authoritative. |
There was a problem hiding this comment.
Including dotagents/Makefile before any targets are defined in the parent Makefile will cause the first target defined in the included file to become the default goal for this Makefile. This likely overrides the intended default target defined later on line 131. To ensure the local help target remains the default, consider explicitly setting .DEFAULT_GOAL.
# Include dotagents from submodule but keep the local targets authoritative.
.DEFAULT_GOAL := help
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Resolves CI failures caused by GNU Make merging targets from the dotagents/Makefile include, leading to a sync target collision and incorrect path resolution in the parent repo.
Changes:
- Pins the
dotagentssubmodule to a revision that fixes path concatenation for.ruler. - Prevents the
dotagentssynctarget/recipe from leaking into the parent Makefile by settingDOTAGENTS_SKIP_SYNC := 1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dotagents | Updates the submodule SHA to pick up upstream fixes. |
| Makefile | Adds a guard variable to skip dotagents sync when including its Makefile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Include dotagents from submodule but keep the local help target authoritative. | ||
| # Include dotagents from submodule but keep the local targets authoritative. | ||
| DOTAGENTS_SKIP_HELP := 1 | ||
| DOTAGENTS_SKIP_SYNC := 1 |
There was a problem hiding this comment.
DOTAGENTS_SKIP_SYNC := 1 unconditionally forces the behavior for all consumers, which makes it harder to override locally (e.g., if someone intentionally wants dotagents' sync behavior). Consider using a default assignment (?=) so callers/CI can explicitly opt back in by setting DOTAGENTS_SKIP_SYNC=0 (or leaving it unset when appropriate).
| DOTAGENTS_SKIP_SYNC := 1 | |
| DOTAGENTS_SKIP_SYNC ?= 1 |
| SHELL := bash | ||
|
|
||
| # Include dotagents from submodule but keep the local help target authoritative. | ||
| # Include dotagents from submodule but keep the local targets authoritative. |
There was a problem hiding this comment.
The comment is now broader (“local targets authoritative”), but the key nuance is specifically about avoiding GNU Make target/recipe merging for sync (and potentially other targets) when including dotagents/Makefile. Please expand this comment to briefly document the collision/merge behavior and why DOTAGENTS_SKIP_SYNC is set here, so future maintainers don’t remove it as “unused”.
| # Include dotagents from submodule but keep the local targets authoritative. | |
| # Include dotagents from the submodule, but keep local targets authoritative. | |
| # In GNU Make, if both this file and dotagents/Makefile define the same target | |
| # (for example `sync`), prerequisites may be merged and recipes can be | |
| # overridden/combined in surprising ways. `DOTAGENTS_SKIP_SYNC` is therefore | |
| # intentionally set so the included makefile does not define its own `sync` | |
| # target and accidentally change the behavior of the local one. Keep this even | |
| # if it looks unused. `DOTAGENTS_SKIP_HELP` similarly avoids help-target noise. |
Summary
DOTAGENTS_SKIP_SYNC := 1in parent Makefile to prevent dotagentssyncrecipe from leaking when included via-include dotagents/Makefile/separator inruler-apply-global(dotagents.ruler->dotagents/.ruler)Root cause: The parent Makefile includes dotagents/Makefile, which caused GNU Make to merge the
synctarget prerequisites and recipe. The dotagentssyncrecipe (callingruler-apply-global,commands-sync, etc.) would run at the parent level where paths didn't resolve correctly. Additionally,$(abspath ...)strips trailing slashes, so.rulerwas concatenated directly to the directory name.Closes #1372
Summary by cubic
Prevents the
synctarget fromdotagentsrunning in the parent Makefile, which caused path errors and broke upgrade CI (fixes #1372). Also updatesdotagentsto fix a missing/inruler-apply-global.Makefile: setDOTAGENTS_SKIP_SYNC := 1to stopdotagents/Makefilefrom merging and running itssyncrecipe in the parent.dotagentssubmodule to usedotagents/.ruler(instead ofdotagents.ruler) inruler-apply-global.Written for commit d98cf94. Summary will update on new commits.