fix: address PR #65 review feedback (env vars, tree diagram, flag consistency)#66
Conversation
- CLAUDE.md: add separator comment between agent and infrastructure dirs in tree diagram (fixes misleading nesting visual) - Makefile: fix env var in smoke-bob-agent-engine-dev from legacy BOB_AGENT_ENGINE_NAME_DEV to correct AGENT_ENGINE_BOB_DEV; remove redundant --agent bob flag (bob is already the default) - run_agent_engine_dev_smoke.py: fix env var guidance from AGENT_ENGINE_*_ID_DEV to AGENT_ENGINE_*_DEV (matches what agents/config/agent_engine.py actually reads) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary by QodoFix environment variable names and Makefile configuration
WalkthroughsDescription• Fixed environment variable names in smoke test and Makefile - Changed AGENT_ENGINE_*_ID_DEV to AGENT_ENGINE_*_DEV (matches actual config) - Updated BOB_AGENT_ENGINE_NAME_DEV to AGENT_ENGINE_BOB_DEV • Removed redundant --agent bob flag from Makefile target • Clarified tree diagram structure with separator comment Diagramflowchart LR
A["Environment Variable<br/>Inconsistencies"] --> B["Update Smoke Test<br/>Documentation"]
A --> C["Fix Makefile<br/>References"]
A --> D["Correct Runtime<br/>Guidance Messages"]
C --> E["Remove Redundant<br/>Flag"]
B --> F["Aligned Config<br/>Names"]
D --> F
E --> F
G["Tree Diagram<br/>Ambiguity"] --> H["Add Separator<br/>Comment"]
H --> I["Improved<br/>Clarity"]
File Changes1. scripts/run_agent_engine_dev_smoke.py
|
Summary of ChangesHello @jeremylongshore, 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 several aspects of the codebase and documentation based on automated review feedback. The changes primarily focus on improving consistency in environment variable naming, clarifying guidance for development smoke tests, enhancing the readability of a documentation tree diagram, and removing redundant command-line flags from a Makefile target. These adjustments aim to streamline development workflows and reduce potential confusion. Highlights
Changelog
Activity
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
|
📝 WalkthroughWalkthroughThis PR coordinates documentation and configuration updates: expanding the CLAUDE.md directory structure documentation with infrastructure details, renaming environment variables for consistency across the build system, and simplifying smoke test script invocation. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
241-252:⚠️ Potential issue | 🟠 MajorMove new documentation content into
000-docs/per R6.This new directory layout section is documentation content inside
CLAUDE.md, which violates the single-docs-folder rule. Please relocate this section into000-docs/(NNN-CC-ABCD or 000-CC-ABCD naming) and replace it here with a short pointer link.As per coding guidelines, “**/*.md: All documentation must be placed in 000-docs/ folder with NNN-CC-ABCD or 000-CC-ABCD naming format - Hard Mode Rule R6.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 241 - 252, The section in CLAUDE.md that lists the infrastructure/utility directories is documentation and must be moved into the docs folder: create a new Markdown file under 000-docs/ using the required naming pattern (e.g., 000-CC-ABCD or NNN-CC-ABCD), paste the directory-layout block there, and in CLAUDE.md replace that block with a short pointer link to the new file; ensure filenames and the new link text follow the repo's doc naming convention and that the moved content preserves formatting and headings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CLAUDE.md`:
- Around line 241-252: The section in CLAUDE.md that lists the
infrastructure/utility directories is documentation and must be moved into the
docs folder: create a new Markdown file under 000-docs/ using the required
naming pattern (e.g., 000-CC-ABCD or NNN-CC-ABCD), paste the directory-layout
block there, and in CLAUDE.md replace that block with a short pointer link to
the new file; ensure filenames and the new link text follow the repo's doc
naming convention and that the moved content preserves formatting and headings.
|
| Filename | Overview |
|---|---|
| CLAUDE.md | Added separator comment to clarify tree diagram structure between agent and infrastructure directories |
| Makefile | Updated env var name and removed redundant flag, but introduces breaking inconsistency with rest of codebase |
| scripts/run_agent_engine_dev_smoke.py | Fixed env var name in docstring and error message, but inconsistent with actual variable definitions and tests |
Last reviewed commit: b066728
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @echo "$(BLUE)🚦 Running Bob Agent Engine dev smoke test...$(NC)" | ||
| @echo "$(YELLOW)ℹ️ Requires BOB_AGENT_ENGINE_NAME_DEV to be set after dev deployment$(NC)" | ||
| @$(PYTHON) scripts/run_agent_engine_dev_smoke.py --agent bob | ||
| @echo "$(YELLOW)ℹ️ Requires AGENT_ENGINE_BOB_DEV to be set after dev deployment$(NC)" |
There was a problem hiding this comment.
This env var name doesn't match the variable definitions in agents/config/inventory.py:218 which defines AGENT_ENGINE_BOB_ID_DEV (with _ID_). The runtime code in agents/config/agent_engine.py:146 constructs names without _ID_, but all tests use AGENT_ENGINE_BOB_ID_DEV (see tests/unit/test_agent_engine_client.py:49). This change will break existing configurations and tests.
| Requirements: | ||
| - DEPLOYMENT_ENV=dev | ||
| - AGENT_ENGINE_BOB_ID_DEV set (or another agent configured) | ||
| - AGENT_ENGINE_BOB_DEV set (or another agent configured) |
There was a problem hiding this comment.
This corrected env var name conflicts with the actual variable definition in agents/config/inventory.py:218 which specifies AGENT_ENGINE_BOB_ID_DEV (with _ID_). Need to resolve the inconsistency between what agent_engine.py:146 constructs and what inventory.py defines.
| print(" To configure:") | ||
| print( | ||
| f" export AGENT_ENGINE_{agent_role.replace('-', '_').upper()}_ID_DEV=your-engine-id" | ||
| f" export AGENT_ENGINE_{agent_role.replace('-', '_').upper()}_DEV=your-engine-id" |
There was a problem hiding this comment.
Same issue - this format string generates AGENT_ENGINE_BOB_DEV but inventory.py defines AGENT_ENGINE_BOB_ID_DEV. The core issue is that agent_engine.py:146 constructs variable names without _ID_ while all variable definitions in inventory.py include _ID_. Either the construction logic or all the definitions need to be updated consistently.
Code Review by Qodo
1. Env var contract mismatch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| print(" To configure:") | ||
| print( | ||
| f" export AGENT_ENGINE_{agent_role.replace('-', '_').upper()}_ID_DEV=your-engine-id" | ||
| f" export AGENT_ENGINE_{agent_role.replace('-', '_').upper()}_DEV=your-engine-id" |
There was a problem hiding this comment.
1. Env var contract mismatch 🐞 Bug ✓ Correctness
Smoke guidance now tells users to set AGENT_ENGINE_{AGENT}_DEV, but the repo’s config
inventory/validators still model Agent Engine vars as AGENT_ENGINE_{AGENT}_ID_DEV, so readiness
checks can fail (or mislead) even when following the updated smoke instructions.
Agent Prompt
### Issue description
The smoke script and runtime config loader use `AGENT_ENGINE_{AGENT}_{ENV}` (e.g., `AGENT_ENGINE_BOB_DEV`), but the config inventory (used by `scripts/check_config_all.py` and ARV-style validation) still defines engine vars as `AGENT_ENGINE_{AGENT}_ID_{ENV}`. This makes validation tooling inconsistent with the runtime contract.
### Issue Context
- Runtime reads `AGENT_ENGINE_{AGENT}_{ENV}`.
- Inventory/validators still model `AGENT_ENGINE_*_ID_*`.
- The PR updated user-facing smoke guidance to the non-`_ID` variant, increasing the chance users hit this mismatch.
### Fix Focus Areas
- agents/config/agent_engine.py[116-158]
- agents/config/inventory.py[208-270]
- scripts/check_config_all.py[180-220]
- agents/arv/spec.py[168-178]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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
The pull request successfully addresses feedback from PR #65 by synchronizing environment variable names with the implementation in agents/config/agent_engine.py, improving the clarity of the directory tree in CLAUDE.md, and removing redundant flags in the Makefile. The changes are consistent across the affected files and correctly reflect the project's configuration logic. No major issues were identified in the modified code.
1 participant
Summary
Fixes 3 issues flagged by automated reviewers on PR #65:
smoke-bob-agent-engine-devreferenced legacy env varBOB_AGENT_ENGINE_NAME_DEV— updated toAGENT_ENGINE_BOB_DEV(whatagents/config/agent_engine.pyactually reads)AGENT_ENGINE_*_ID_DEVin guidance but config expectsAGENT_ENGINE_*_DEV(no_ID) — fixed docstring and runtime message│between agent and infrastructure directories — added separator comment for clarity--agent bobflag from Makefile target (bob is already the default)Test plan
make smoke-bob-agent-engine-devshows correct env var namepython3 scripts/run_agent_engine_dev_smoke.pyprints correct env var in guidance🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores