feat: idiot-proof dual-repo workflow with git hooks#304
Conversation
- Add forge-core commit blocker hook (prevents direct commits) - Version all dev-core hooks in scripts/hooks/ - Update Makefile to install all hooks including blocker - Rewrite AGENTS.md Amendment #11 with single-repo experience - Rewrite DUAL_REPO_WORKFLOW.md with correct workflow - All member crates use version.workspace = true - Update to Rust Edition 2024, nightly-2025-05-18
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 significantly refines the dual-repository development workflow by introducing a suite of Git hooks designed to streamline and "idiot-proof" the process. The core objective is to create a seamless "single-repo experience" where developers primarily interact with 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 introduces a well-thought-out set of git hooks to automate and simplify the dual-repository development workflow, which is a great improvement for developer experience. The automation for staging and committing changes to forge-core from the main repository, along with the safety checks to prevent incorrect pushes and direct commits, will surely prevent common errors. The accompanying documentation updates are clear and comprehensive. The transition to workspace-managed versions in Cargo.toml and the update of the check-versions.sh script are also solid maintainability improvements. I have a few suggestions to further improve the robustness and readability of the new scripts and Makefile logic.
| fi | ||
|
|
||
| # Commit with the same message | ||
| if git commit -m "$MSG" 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors from git commit, such as a locked index file or merge conflicts. If an error other than "nothing to commit" occurs, the script will misleadingly report "nothing to commit". It's better to let Git's error messages be visible to the user for easier debugging.
| if git commit -m "$MSG" 2>/dev/null; then | |
| if git commit -m "$MSG"; then |
| @for hook in pre-commit prepare-commit-msg pre-push; do \ | ||
| if [ -f scripts/hooks/$$hook ]; then \ | ||
| cp scripts/hooks/$$hook .git/hooks/$$hook; \ | ||
| chmod +x .git/hooks/$$hook; \ | ||
| fi; \ | ||
| done |
There was a problem hiding this comment.
The current loop for installing git hooks fails silently if a hook script is missing. This could lead to the automated workflow not being set up correctly without the developer noticing. It would be more robust to add a check and exit with an error if a script is not found.
@for hook in pre-commit prepare-commit-msg pre-push; do \
if [ ! -f scripts/hooks/$$hook ]; then \
echo -e "$(FONT_RED)ERROR: Missing hook script scripts/hooks/$$hook. Cannot set up dual-repo workflow.$(FONT_RESET)"; \
exit 1; \
fi; \
cp scripts/hooks/$$hook .git/hooks/$$hook; \
chmod +x .git/hooks/$$hook; \
done
| use std::process::Command; | ||
|
|
||
| use db::models::project::Project; | ||
|
|
There was a problem hiding this comment.
For better code organization and adherence to Rust conventions, use statements should be placed at the top of the module rather than inside functions. This improves readability and makes dependencies clearer. This also applies to the post_project_pull function.
| # Check if forge-core has any changes (staged or unstaged) | ||
| cd "$FORGE_CORE_DIR" | ||
|
|
||
| HAS_STAGED=false | ||
| HAS_UNSTAGED=false | ||
|
|
||
| # Check for staged changes | ||
| if ! git diff --cached --quiet 2>/dev/null; then | ||
| HAS_STAGED=true | ||
| fi | ||
|
|
||
| # Check for unstaged changes | ||
| if ! git diff --quiet 2>/dev/null; then | ||
| HAS_UNSTAGED=true | ||
| fi | ||
|
|
||
| # Check for untracked files | ||
| UNTRACKED=$(git ls-files --others --exclude-standard 2>/dev/null | head -1) | ||
| if [ -n "$UNTRACKED" ]; then | ||
| HAS_UNSTAGED=true | ||
| fi | ||
|
|
||
| cd .. | ||
|
|
||
| if [ "$HAS_STAGED" = "true" ] || [ "$HAS_UNSTAGED" = "true" ]; then | ||
| # Stage all forge-core changes | ||
| cd "$FORGE_CORE_DIR" | ||
| git add -A | ||
| cd .. | ||
|
|
||
| # Mark that we need to sync (commit happens in prepare-commit-msg) | ||
| touch "$SYNC_MARKER" | ||
|
|
||
| echo -e "${CYAN}📦 forge-core changes staged for sync${NC}" | ||
| echo -e " Changes will be committed with the same message" | ||
| fi |
There was a problem hiding this comment.
The logic to check for staged, unstaged, and untracked files can be simplified into a single, more concise check using git status --porcelain. This command's output is empty if and only if the working directory is clean, which covers all cases you're checking for.
| # Check if forge-core has any changes (staged or unstaged) | |
| cd "$FORGE_CORE_DIR" | |
| HAS_STAGED=false | |
| HAS_UNSTAGED=false | |
| # Check for staged changes | |
| if ! git diff --cached --quiet 2>/dev/null; then | |
| HAS_STAGED=true | |
| fi | |
| # Check for unstaged changes | |
| if ! git diff --quiet 2>/dev/null; then | |
| HAS_UNSTAGED=true | |
| fi | |
| # Check for untracked files | |
| UNTRACKED=$(git ls-files --others --exclude-standard 2>/dev/null | head -1) | |
| if [ -n "$UNTRACKED" ]; then | |
| HAS_UNSTAGED=true | |
| fi | |
| cd .. | |
| if [ "$HAS_STAGED" = "true" ] || [ "$HAS_UNSTAGED" = "true" ]; then | |
| # Stage all forge-core changes | |
| cd "$FORGE_CORE_DIR" | |
| git add -A | |
| cd .. | |
| # Mark that we need to sync (commit happens in prepare-commit-msg) | |
| touch "$SYNC_MARKER" | |
| echo -e "${CYAN}📦 forge-core changes staged for sync${NC}" | |
| echo -e " Changes will be committed with the same message" | |
| fi | |
| # Check if forge-core has any changes (staged, unstaged, or untracked) | |
| if [ -n "$(cd "$FORGE_CORE_DIR" && git status --porcelain)" ]; then | |
| # Stage all forge-core changes | |
| (cd "$FORGE_CORE_DIR" && git add -A) | |
| # Mark that we need to sync (commit happens in prepare-commit-msg) | |
| touch "$SYNC_MARKER" | |
| echo -e "${CYAN}📦 forge-core changes staged for sync${NC}" | |
| echo -e " Changes will be committed with the same message" | |
| fi |
- Add forge-core-pre-push hook to block direct pushes - Add `make push-both` command for pushing both repos together - Add `make pr-both` command for creating PRs in both repos (with RC label) - Update dev-core to install both blocker hooks in forge-core - Update AGENTS.md Amendment #11 with new workflow - Update DUAL_REPO_WORKFLOW.md with simplified commands
…elain Replace separate checks for staged, unstaged, and untracked files with a single `git status --porcelain` command. Same behavior, fewer lines.
Adds pre-checkout cleanup step to prevent permission issues on self-hosted runners where stale files from previous runs can cause checkout failures.
The e2e workflow was missing the dtolnay/rust-toolchain step, causing 'cargo: command not found' errors on the self-hosted runner.
Fix clippy warnings about uninlined format args in: - forge-app/src/router.rs (test functions) - forge-app/src/services/mod.rs (test function) - forge-app/examples/test_profile_names.rs - forge-app/examples/test_genie_discovery.rs
1 participant
Summary
Test plan
make dev-core BRANCH=testenables local developmentmake dev-core-offallows push