fix(config): align AGENT_ENGINE env var convention to _ID_ pattern#67
fix(config): align AGENT_ENGINE env var convention to _ID_ pattern#67jeremylongshore merged 2 commits intomainfrom
Conversation
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 addresses a critical inconsistency in how 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
|
Review Summary by QodoAlign Agent Engine env var convention to ID pattern across codebase
WalkthroughsDescription• Align runtime env var construction to canonical AGENT_ENGINE_{AGENT}_ID_{ENV} pattern
• Fix contract mismatch between documented setup and actual runtime expectations
• Update all docstrings, examples, and guidance across config, scripts, and Makefile
• Add clarifying separator comment in CLAUDE.md directory tree diagram
Diagramflowchart LR
A["Runtime Code<br/>agent_engine.py:146"] -->|"Add _ID_ to<br/>env var construction"| B["AGENT_ENGINE_{AGENT}_ID_{ENV}"]
C["Docstrings &<br/>Examples"] -->|"Update to match<br/>canonical pattern"| B
D["Makefile &<br/>Scripts"] -->|"Fix guidance &<br/>echo messages"| B
B -->|"Matches documented<br/>setup in inventory.py"| E["Consistent Contract"]
File Changes1. agents/config/agent_engine.py
|
📝 WalkthroughWalkthroughUpdated environment variable naming convention from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
![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 correctly fixes a bug where an incorrect environment variable name was being constructed, leading to 'agent not configured' errors. The fix aligns the runtime environment variable construction with the canonical AGENT_ENGINE_{AGENT}_ID_{ENV} pattern. The changes are applied consistently across the core logic, documentation, examples, and helper scripts. My review includes one suggestion to improve the maintainability of the Makefile by making a build target more explicit.
| @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_ID_DEV to be set after dev deployment$(NC)" | ||
| @$(PYTHON) scripts/run_agent_engine_dev_smoke.py |
There was a problem hiding this comment.
While the run_agent_engine_dev_smoke.py script defaults to agent bob, it's more explicit and robust to specify the agent directly in this make target. The smoke-bob-agent-engine-dev target is specifically for 'bob', and relying on the script's default creates a tight coupling. If the script's default ever changes, this target would silently start testing a different agent. Explicitly passing --agent bob makes the Makefile's intent clear and independent of the script's internal defaults.
@$(PYTHON) scripts/run_agent_engine_dev_smoke.py --agent bob
|
| Filename | Overview |
|---|---|
| CLAUDE.md | Added inline comment separator for infrastructure/utility directories - cosmetic change |
| Makefile | Updated smoke test target to reference correct env var AGENT_ENGINE_BOB_ID_DEV and removed redundant --agent bob parameter |
| agents/config/agent_engine.py | Fixed runtime env var construction to match canonical _ID_ pattern - updated line 146 and all documentation/examples |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User sets env var] --> B{Which pattern?}
B -->|Before fix| C[AGENT_ENGINE_BOB_DEV]
B -->|After fix| D[AGENT_ENGINE_BOB_ID_DEV]
C --> E[Runtime constructs:<br/>AGENT_ENGINE_BOB_DEV]
D --> F[Runtime constructs:<br/>AGENT_ENGINE_BOB_ID_DEV]
E --> G{Match?}
F --> H{Match?}
G -->|NO| I[❌ Variable not found<br/>Agent not configured error]
H -->|YES| J[✅ Variable found<br/>Agent configured successfully]
K[Documentation/Tests/inventory.py] --> L[Canonical pattern:<br/>AGENT_ENGINE_*_ID_*]
L --> D
style I fill:#ffcccc
style J fill:#ccffcc
style C fill:#ffffcc
style D fill:#ccffcc
Last reviewed commit: 6123ee1
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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)
219-253:⚠️ Potential issue | 🟠 MajorMove the new directory layout documentation into 000-docs/.
This added layout detail is documentation content in CLAUDE.md, which is outside the required docs folder. Please relocate this block to a properly named file under 000-docs/ and keep CLAUDE.md as a pointer. As per coding guidelines, "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 219 - 253, The "Agent Directory Layout" block in CLAUDE.md is documentation and must be moved into a new file under 000-docs using the required naming format (NNN-CC-ABCD or 000-CC-ABCD); create a new doc file with that name containing the directory layout content (the agents/ tree and its description) and replace the original block in CLAUDE.md with a short pointer line that references the new 000-docs file; ensure the new doc preserves the exact layout text and that CLAUDE.md only contains the pointer.
🤖 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 219-253: The "Agent Directory Layout" block in CLAUDE.md is
documentation and must be moved into a new file under 000-docs using the
required naming format (NNN-CC-ABCD or 000-CC-ABCD); create a new doc file with
that name containing the directory layout content (the agents/ tree and its
description) and replace the original block in CLAUDE.md with a short pointer
line that references the new 000-docs file; ensure the new doc preserves the
exact layout text and that CLAUDE.md only contains the pointer.
Code Review by Qodo
1. Docs use wrong env var
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @echo "$(YELLOW)ℹ️ Requires AGENT_ENGINE_BOB_ID_DEV to be set after dev deployment$(NC)" | ||
| @$(PYTHON) scripts/run_agent_engine_dev_smoke.py |
There was a problem hiding this comment.
1. Docs use wrong env var 🐞 Bug ✓ Correctness
Docs/CHANGELOG still instruct setting BOB_AGENT_ENGINE_NAME_DEV (full resource name), but runtime + Makefile now require AGENT_ENGINE_BOB_ID_DEV (ID only). Users following docs will be “not configured” or may paste a full resource name into an *_ID_* var and produce invalid resource paths.
Agent Prompt
### Issue description
Docs and CHANGELOG still instruct `BOB_AGENT_ENGINE_NAME_DEV` (full resource name) while the runtime/Makefile now requires `AGENT_ENGINE_BOB_ID_DEV` (ID only). This will cause users following docs to fail configuration.
### Issue Context
- Runtime reads `AGENT_ENGINE_{AGENT}_ID_{ENV}`.
- Some docs still describe the old env var and a full resource-name value.
### Fix Focus Areas
- Makefile[433-437]
- scripts/run_agent_engine_dev_smoke.py[16-26]
- CHANGELOG.md[629-639]
- 000-docs/000-DR-STND-inline-source-deployment-for-vertex-agent-engine.md[508-546]
- 000-docs/130-AA-REPT-phase-5-first-dev-deploy-and-smoke-test.md[83-91]
- agents/config/agent_engine.py[166-193]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @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_ID_DEV to be set after dev deployment$(NC)" | ||
| @$(PYTHON) scripts/run_agent_engine_dev_smoke.py |
There was a problem hiding this comment.
2. Docs cite wrong smoke script 🐞 Bug ⛯ Reliability
Multiple docs/CHANGELOG reference scripts/smoke_test_bob_agent_engine_dev.py, but the Makefile target runs scripts/run_agent_engine_dev_smoke.py. This will break the documented smoke workflow and create confusion about which script is canonical.
Agent Prompt
### Issue description
Docs/CHANGELOG point to `scripts/smoke_test_bob_agent_engine_dev.py` while the Makefile runs `scripts/run_agent_engine_dev_smoke.py`. The documented smoke workflow is inconsistent.
### Issue Context
Users will follow docs for smoke testing and expect the Makefile target to run the same script.
### Fix Focus Areas
- Makefile[433-437]
- CHANGELOG.md[629-639]
- 000-docs/000-DR-STND-inline-source-deployment-for-vertex-agent-engine.md[508-546]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The runtime code in agent_engine.py:146 constructed env var names as
AGENT_ENGINE_{AGENT}_{ENV} (e.g. AGENT_ENGINE_BOB_DEV), but the
canonical convention used by inventory.py, tests, and all docs is
AGENT_ENGINE_{AGENT}_ID_{ENV} (e.g. AGENT_ENGINE_BOB_ID_DEV).
This caused a contract mismatch where following the documented setup
would result in "agent not configured" errors at runtime.
Fixed by adding _ID_ to the runtime construction and aligning all
docstrings, examples, and guidance messages across:
- agents/config/agent_engine.py (runtime + docstrings)
- scripts/run_agent_engine_dev_smoke.py (guidance output)
- Makefile (env var echo message)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeremylongshore
force-pushed
the
fix/pr65-review-feedback
branch
from
6a18ee6 to
a526e94
Compare
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)
agents/config/agent_engine.py (1)
325-343:⚠️ Potential issue | 🟡 MinorDocstring still references the old env var pattern.
Line 331 says
AGENT_ENGINE_*_{ENV}but should sayAGENT_ENGINE_*_ID_{ENV}to match the updated convention. This inconsistency could confuse developers reading the docs.📝 Proposed fix
""" List all agents configured in a specific environment. - Scans environment variables for AGENT_ENGINE_*_{ENV} patterns. + Scans environment variables for AGENT_ENGINE_*_ID_{ENV} patterns. Args: env: Environment (if None, uses current environment)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/config/agent_engine.py` around lines 325 - 343, The docstring for list_configured_agents incorrectly describes the env var pattern as AGENT_ENGINE_*_{ENV}; update that description to the new convention AGENT_ENGINE_*_ID_{ENV} so the docs match the implemented parsing logic in list_configured_agents; edit the docstring within the function list_configured_agents to replace the old pattern text with AGENT_ENGINE_*_ID_{ENV} (and adjust any example or explanatory text if it references the old pattern).
🤖 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 `@agents/config/agent_engine.py`:
- Around line 325-343: The docstring for list_configured_agents incorrectly
describes the env var pattern as AGENT_ENGINE_*_{ENV}; update that description
to the new convention AGENT_ENGINE_*_ID_{ENV} so the docs match the implemented
parsing logic in list_configured_agents; edit the docstring within the function
list_configured_agents to replace the old pattern text with
AGENT_ENGINE_*_ID_{ENV} (and adjust any example or explanatory text if it
references the old pattern).
1 participant
Summary
Fixes the env var contract mismatch flagged by Greptile and Qodo on PR #66.
Root cause:
agents/config/agent_engine.py:146constructed env var names asAGENT_ENGINE_{AGENT}_{ENV}(e.g.AGENT_ENGINE_BOB_DEV), but the canonical convention used everywhere else wasAGENT_ENGINE_{AGENT}_ID_{ENV}(e.g.AGENT_ENGINE_BOB_ID_DEV).Impact: Users following documented setup (inventory.py, docs, test examples) would set
AGENT_ENGINE_BOB_ID_DEVbut the runtime looked forAGENT_ENGINE_BOB_DEV— resulting in "agent not configured" errors.Fix: Added
_ID_to the runtime env var construction to match the canonical convention. Updated all docstrings, examples, Makefile echo, and smoke script guidance to be consistent.Files changed:
agents/config/agent_engine.py— runtime construction + docstrings/examplesMakefile— env var in echo messagescripts/run_agent_engine_dev_smoke.py— guidance output + docstringTest plan
test_agent_engine_client.py:49already usesAGENT_ENGINE_BOB_ID_DEV)grep -r "AGENT_ENGINE_BOB_DEV" agents/ scripts/ Makefilereturns zero matches (only_ID_DEV)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes