-
Notifications
You must be signed in to change notification settings - Fork 2
add architect hook installers #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Issue: Add built-in architect hook commands for Claude/Codex/Gemini and require lint in the workflow. Solution: Extend the embedded architect helper with hook subcommands that update configs idempotently. Solution: Update Gemini hook wrapper to prefer architect notify when available. Solution: Document the installer flow in AI integration docs and note lint in CLAUDE.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d53647617d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds hook installer subcommands to the embedded architect shell helper, enabling users to automatically configure Claude, Codex, and Gemini CLI tools for Architect notifications. The installer updates config files only when hooks are missing, preserving the existing notify behavior.
Changes:
- Extended the embedded Python
architectcommand withhook claude|codex|geminisubcommands that intelligently update tool configs - Updated the Gemini hook wrapper to prefer the built-in
architect notifycommand when available - Documented the installer workflow and added
just lintto the required build steps
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shell.zig | Added architect_command_script with hook installer logic and PATH injection for the architect command |
| scripts/architect_hook_gemini.py | Updated to prefer architect command over direct script invocation |
| docs/architecture.md | Documented the architect command availability in shell PATH |
| docs/ai-integration.md | Added hook installer instructions and clarified when to use the built-in command vs. scripts |
| README.md | Updated documentation reference to mention the new architect commands |
| CLAUDE.md | Added just lint to the required build and test steps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue: architect hook gemini wrote literal \n in JSON and architect notify did not light up. Solution: write real newlines in hook installer output and notify payloads. Solution: document restarting terminals after upgrades for refreshed helper scripts.
Issue: Review comments requested narrowing exception handling in embedded installer script. Solution: Catch ImportError for tomllib import, JSONDecodeError for JSON parsing, and TOMLDecodeError for tomllib parsing.
Issue: Users may already have Codex notify configured and need safe updates with backups. Solution: Add notify chaining via wrapper and timestamped backups for settings updates.
Issue: Users asked to replace Codex notify while warning and printing backup names. Solution: Always overwrite notify, emit warning on existing config, and print timestamped backup path.
Issue: Codex notify line was appended under the last table instead of root. Solution: Remove existing notify lines and insert the new notify before the first table header.
Solution: