feat: sync-to-forge-core automation + codex API fixes#294
Conversation
- Remove orphaned dev-core-helper.sh (549 lines, never called) - Consolidate mode detection to grep ^\[patch\. in config.toml - Fix push-both and pr targets to use correct detection method - Inline dev-core-check diagnostics (was delegating to deleted script) - Update check-versions.sh to use grep-based detection - Improve dev-help documentation with workflow explanation
Summary of ChangesHello @namastex888, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines and simplifies the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 does a great job of consolidating the dev-core workflow, removing the orphaned dev-core-helper.sh script, and simplifying mode detection. The changes make the developer experience clearer and more robust. My feedback focuses on a few minor areas to further improve the robustness of the new diagnostic scripts and ensure the documentation in dev-help is accurate.
| fi | ||
| @if [ -d "forge-core" ]; then \ | ||
| echo -e "Core dir: $(FONT_GREEN)EXISTS$(FONT_RESET)"; \ | ||
| echo -e "Branch: $$(cd forge-core && git branch --show-current)"; \ |
There was a problem hiding this comment.
The command git branch --show-current will output an error message to stderr if the forge-core directory exists but is not a git repository, or if the HEAD is detached. This can be confusing for the user. It's better to suppress the error and provide a fallback message, similar to how it's done for the 'Version' line, to make the health check script more robust.
echo -e "Branch: $$(cd forge-core && git branch --show-current 2>/dev/null || echo 'detached HEAD or not a git repo')"; \
| @echo " make dev-core-status Detailed dev-core status" | ||
| @echo " make commit-both Commit to both repos" | ||
| @echo " make dev-core-check Health check diagnostics" | ||
| @echo " make commit-both MSG='x' Commit to both repos with same message" |
There was a problem hiding this comment.
The documentation for commit-both suggests it accepts a MSG variable from the command line (MSG='x'). However, the implementation of the commit-both target is interactive and uses read -p to prompt for a commit message. This documentation is misleading. Please update it to reflect the interactive nature of the command.
@echo " make commit-both Commit to both repos with same message (interactive)"
| DEV_CORE_ACTIVE=false | ||
| if grep -q '^\[patch\.' "$REPO_ROOT/.cargo/config.toml" 2>/dev/null; then | ||
| DEV_CORE_ACTIVE=true | ||
| fi | ||
| if [ "$DEV_CORE_ACTIVE" = "false" ] && [ -n "$UNIQUE_TAGS" ]; then |
There was a problem hiding this comment.
This logic to check for dev-core mode can be simplified. Instead of using a flag variable (DEV_CORE_ACTIVE), you can directly use the grep command's exit code in the if condition. This makes the code more concise and easier to read.
| DEV_CORE_ACTIVE=false | |
| if grep -q '^\[patch\.' "$REPO_ROOT/.cargo/config.toml" 2>/dev/null; then | |
| DEV_CORE_ACTIVE=true | |
| fi | |
| if [ "$DEV_CORE_ACTIVE" = "false" ] && [ -n "$UNIQUE_TAGS" ]; then | |
| if ! grep -q '^\[patch\.' "$REPO_ROOT/.cargo/config.toml" 2>/dev/null && [ -n "$UNIQUE_TAGS" ]; then |
- Add sync-to-forge-core.yml workflow that auto-syncs forge-core/ changes when PRs merge to dev - Fix .cargo/config.toml codex patches (must always be active) - Update dev-core-off to not touch crates-io patches - Remove obsolete Makefile targets (commit-both, push-both, pr) - Update dev-help documentation with new automated workflow The new workflow means developers only work in automagik-forge. When PRs merge, forge-core is automatically updated - no manual interaction with forge-core repo needed.
namastex888
changed the title
1 participant
Summary
sync-to-forge-core.ymlworkflow that automatically syncsforge-core/changes when PRs merge to dev.cargo/config.tomlcodex patches (must always be active, not toggled by dev-core)dev-core-offand pre-push hook to only check for forge-core patches, not crates-io patchescommit-both,push-both,pr)dev-helpdocumentation with new automated workflowNew Workflow
Developers now ONLY work in automagik-forge. When PRs merge to dev:
sync-to-forge-core.ymlautomatically pushes forge-core/ changes to forge-core repoTest plan