Conversation
There was a problem hiding this comment.
Code Review
This pull request implements full recipe loading, serializing the complete plan state into a recipe file to enable reproducible runs. A key change involves dynamically deriving the discover-phase attribute from test paths. However, a critical security vulnerability has been identified: the implementation of test path recreation in the discover step is susceptible to Path Traversal. The path attribute from the recipe is used to construct filesystem paths for directory creation without adequate sanitization, potentially allowing an attacker to create directories outside the intended workdir using .. sequences. This requires remediation by validating that the resulting paths do not escape the intended base directory. Additionally, there are a couple of suggestions to improve code robustness and clarity in tmt/recipe.py.
3c63f94 to
18b7353
Compare
thrix
left a comment
There was a problem hiding this comment.
The full recipe loading direction is good. Three findings, one requiring a change:
-
Run.environmentsilently ignores CLI--environmentwhen recipe is loaded - The recipe env unconditionally overrides CLI options. New--environmentoverrides on replay are silently lost. This should merge recipe env with CLI env, letting CLI take precedence. -
linkfield type mismatch after unserialization removal - Raw data stored whereLinksobject is expected. Not an active runtime bug but incorrect typing that could break on re-serialization paths. -
Unrelated schema change -
display-guestinreport/display.yamlshould be split out.
|
Besides other comments, the code appears to assume the recipe file is well-formed, do you consider add some validation to the recipe file provided, say, a schema file? |
|
@therazix please, set the "Size" of this PR. |
thrix
left a comment
There was a problem hiding this comment.
Review: Add support for full recipe loading
Good progress on extending recipe loading from discover-only to all plan steps. The environment simplification and removal of discover_phase are clean.
Issues
Blocking:
- CLI environment silently ignored (
tmt/base/core.py): The early return bypasses_environment_from_cli, so--environment FOO=baris silently dropped when using--recipe. See inline comment for suggested fix.
Latent bug:
_RecipeTest.linkunserialization removed (tmt/recipe.py): Works today because all test links are[](falsy short-circuit), but wouldAttributeErroron any recipe with non-empty test links. See inline comment.
Hygiene:
- Unrelated schema change (
tmt/schemas/report/display.yaml):display-guestaddition is not related to recipe loading — should be a separate commit/PR. - PR checklist: All items are unchecked — docs, spec, schema, version, release note still needed.
Verified non-issues
- Path traversal in
discover_from_recipe: Therelative_to()+resolve()+ parent check is sufficient. The gemini-code-assist security warning is overblown. - Removed "non-existent plan" test: Correct — with
tree.children.clear(), the tree IS the recipe, so the old error case no longer applies. - Only saving
_environment_from_fmfin_RecipePlan: Reasonable —_RecipeRun.environmentcaptures the full merged env, and intrinsics should be regenerated per run.
Generated-by: Claude Code
18b7353 to
4201f23
Compare
452c14b to
7165403
Compare
7165403 to
8360382
Compare
8810887 to
27d199a
Compare
|
@therazix would it be possible to provide a solid MR description for the changes, so it is easier to follow the changes, for example I am looking at this diff: And I would like to understand why this was changed, I would expect it would be mentioned that this is one of the improvements made to support full recipe loading ... (or something similar) |
d246817 to
c5f0683
Compare
611ed5d to
6cbd173
Compare
6cbd173 to
3a5b24b
Compare
3a5b24b to
ed6e5da
Compare
|
Eh, it's still far from satisfactory, but that's not @therazix's fault, it's just so complex field, with a tech debt of its own. Hopefully, this wouldn't be the last patch, and together with the transition of |
ed6e5da to
f09cb56
Compare
f09cb56 to
a08cbbf
Compare
Release notes for the following user-facing changes: - Full recipe loading support (#4661) - Artifact plugin lists packages in `artifacts.yaml` (#4703) - Auto-verify artifact packages against `require`/`recommend` (#4726) - Fix beakerlib library cache in `tmt try` (#4759) Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Release notes for the following user-facing changes: - Full recipe loading support (#4661) - Artifact plugin lists packages in `artifacts.yaml` (#4703) - Auto-verify artifact packages against `require`/`recommend` (#4726) - Fix beakerlib library cache in `tmt try` (#4759) Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Release notes for the following user-facing changes: - Full recipe loading support (#4661) - Artifact plugin lists packages in `artifacts.yaml` (#4703) - Auto-verify artifact packages against `require`/`recommend` (#4726) - Fix beakerlib library cache in `tmt try` (#4759) Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
This PR implements a full recipe loading feature. All phases can now be loaded directly from the recipe. Serialization and deserialization were replaced with
to_spec/from_specmethods, and the generated recipe will now contain only non-empty values to reduce its size.Resolves: #4531
Pull Request Checklist