feat: support variables in context instructions#75
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors variable substitution logic into a dedicated module and extends it to support environment variables in both skill descriptions and project instructions, updating the documentation accordingly. The review feedback highlights a critical Python 3 syntax error in system_prompt.py where multiple exceptions are caught using commas instead of parentheses. Additionally, it points out that capturing os.environ at import time in config.py before load_dotenv runs prevents .env variables from being substituted and makes testing difficult, suggesting dynamic evaluation instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for i, path in enumerate(files, 1): | ||
| try: | ||
| content = path.read_text().strip() | ||
| content = substitute_variables(path.read_text().strip(), variables) |
There was a problem hiding this comment.
The except PermissionError, OSError: block on the next line (line 65) contains a syntax error in Python 3.
In Python 3, the comma syntax for catching multiple exceptions was removed. It must be parenthesized as except (PermissionError, OSError):.
However, since PermissionError is a subclass of OSError in Python 3, you can simplify this to just:
except OSError:This will catch both PermissionError and other OSError exceptions cleanly and is fully compatible with Python 3.
Note: The same syntax error also exists at line 15 of this file (except PermissionError, OSError:), which should also be updated to except OSError:.
| PROJECT_SKILLS_DIR = WORKDIR / ".agents" / "skills" | ||
| SKILLS_DIRS = [HOME_SKILLS_DIR, PROJECT_SKILLS_DIR] | ||
|
|
||
| SHELL_ENVIRONMENT = dict(os.environ) |
There was a problem hiding this comment.
Capturing os.environ at import time before load_dotenv is called has two significant drawbacks:
- Excludes
.envvariables: Any environment variables loaded from the.envfile viaload_dotenvwill not be available for substitution inAGENTS.mdorSKILL.md. While the PR description mentions this is intentional ("dotenv-only variables remaining literal"), this behavior is highly counter-intuitive to users who expect.envto act as an extension of their shell environment. - Fragile and hard to test: Capturing environment variables at import time makes it difficult to dynamically update or mock environment variables during testing or runtime.
Recommendation:
Consider removing SHELL_ENVIRONMENT entirely and dynamically reading os.environ inside get_variables() in src/mini_agent/agent/variables.py:
def get_variables() -> dict[str, str]:
variables = dict(os.environ)
variables["MODEL_NAME"] = config.get_model()
return variablesIf you must exclude sensitive variables (like API keys), you can explicitly filter them out by key name rather than excluding all .env variables.
There was a problem hiding this comment.
1 issue found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: Codex <noreply@openai.com>
f409697 to
18291aa
Compare
Summary
AGENTS.mdusing the same built-in variables as skills$VARIABLEand${VARIABLE}support for skills and context filesVerification
make checkmake type-checkSKILL.mdandAGENTS.mdexpansion, confirming onlyMODEL_NAMEexpands while shell, dotenv-only, and missing variables remain literal