Support usage of environment variables inside robot documentation#45
Support usage of environment variables inside robot documentation#45JokeWaumans merged 6 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe changes introduce runtime environment variable substitution into test documentation. A test robot file is updated with a runtime variable token, the parser is enhanced to replace such tokens with corresponding environment values via regex pattern matching, and tox test environments are configured with the necessary variables. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mlx/robot2rst/robot2rst.py`:
- Around line 58-68: The current render_template call unpacks all environment
variables into the top-level template namespace (env_vars via **env_vars), which
risks keyword collisions and exposing secrets; change render_template(rst_file,
parser=parser, suite=..., prefix=..., relationship_config=...,
gen_matrix=gen_matrix, **kwargs, env=env_vars) so the template receives a single
namespaced dict (e.g., env) instead of splatting env_vars, and update templates
to read values as env['MY_VAR'] or env.get('MY_VAR') to avoid collisions and
limit exposure.
mlx/robot2rst/robot2rst.py
Outdated
| env_vars = dict(os.environ) | ||
| return render_template( | ||
| rst_file, | ||
| parser=parser, | ||
| suite=rst_file.stem, | ||
| prefix=prefix, | ||
| relationship_config=relationship_config, | ||
| gen_matrix=gen_matrix, | ||
| **env_vars, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
Namespace collision risk and unnecessary exposure of all environment variables.
Two concerns with unpacking **env_vars directly into the template keyword arguments:
-
TypeErrorat runtime if any env-var name collides with an existing keyword argument. If an environment variable happens to be namedparser,suite,prefix,gen_matrix,test_type,only,coverages, etc., Python will raiseTypeError: got multiple values for keyword argument '…'. While most standard env vars are uppercase, this is not guaranteed—CI systems and users can define arbitrary names. -
All environment variables (including secrets) become available to the Mako template. Env vars frequently carry credentials (
AWS_SECRET_ACCESS_KEY,DATABASE_URL, tokens, etc.). Mako templates can execute arbitrary Python, so any template content—including user-supplied.robotdocumentation—could exfiltrate secrets into the generated.rstoutput.
Both problems are solved by passing env vars as a single namespaced dict instead of splatting them into the top-level namespace. Templates would then use ${env['MY_VAR']} instead of ${MY_VAR}.
Proposed fix
- env_vars = dict(os.environ)
return render_template(
rst_file,
parser=parser,
suite=rst_file.stem,
prefix=prefix,
relationship_config=relationship_config,
gen_matrix=gen_matrix,
- **env_vars,
+ env=dict(os.environ),
**kwargs,
)Templates would then reference variables as ${env['MY_VAR']} (or ${env.get('MY_VAR', 'default')}).
🤖 Prompt for AI Agents
In `@mlx/robot2rst/robot2rst.py` around lines 58 - 68, The current render_template
call unpacks all environment variables into the top-level template namespace
(env_vars via **env_vars), which risks keyword collisions and exposing secrets;
change render_template(rst_file, parser=parser, suite=..., prefix=...,
relationship_config=..., gen_matrix=gen_matrix, **kwargs, env=env_vars) so the
template receives a single namespaced dict (e.g., env) instead of splatting
env_vars, and update templates to read values as env['MY_VAR'] or
env.get('MY_VAR') to avoid collisions and limit exposure.
There was a problem hiding this comment.
This seems valid- but you opted not to take this.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Mako template to see if it references environment variables
cat mlx/robot2rst/robot2rst.mako
Length of output: 2958
@Letme, you're right to push back on this. After examining the template file, the environment variables aren't even used by the template — which makes this change both risky and unnecessary.
Key findings:
-
The template never references environment variables. The
robot2rst.makotemplate only usessuite,test_type,parser,prefix,relationship_config,gen_matrix, andcoverage. There's no code path that accesses env vars like${MY_VAR}or${env_vars['MY_VAR']}. -
Environment variable substitution already happens earlier in the pipeline. Looking at the PR changes,
robot_parser.pyalready handles%{VAR}substitution in test documentation strings usingos.environbefore the template is ever rendered. The template'sgenerate_body()function only processesparser.variables, not environment variables. -
This means exposing all env vars to the template serves no purpose — except creating the exact security and collision risks CodeRabbit flagged.
Recommendation:
Remove the env_vars code entirely:
- env_vars = dict(os.environ)
return render_template(
rst_file,
parser=parser,
suite=rst_file.stem,
prefix=prefix,
relationship_config=relationship_config,
gen_matrix=gen_matrix,
- **env_vars,
**kwargs,
)The PR's environment variable feature already works correctly via the parser layer. Adding them to the template context is dead code that creates security exposure.
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
</no_relevant_code_snippets>
…correct env variable in robot parser
…No module named 'pkg_resources'
Summary by CodeRabbit
%{VAR}syntax pattern, which will be replaced with corresponding values at runtime.