Skip to content

Fix shell interpolation in task reports - #3596

Merged
robnester-rh merged 1 commit into
conforma:mainfrom
robnester-rh:EC-2048
Oct 5, 2026
Merged

robnester-rh merged 1 commit into
conforma:mainfrom
robnester-rh:EC-2048

Conversation

@robnester-rh

Copy link
Copy Markdown
Contributor

Pass HOMEDIR through the environment before formatting report output so parameter values are not inserted into shell command text in the canonical task manifests.

This keeps the source tasks aligned with the corresponding catalog fix.

@robnester-rh
robnester-rh requested a review from a team as a code owner October 2, 2026 14:33
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 84545ad3-ce93-44c2-b9d4-cad35a39e24f

📥 Commits

Reviewing files that changed from the base of the PR and between 286f457 and e84631d.

📒 Files selected for processing (2)
  • tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
  • tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

Both verification tasks now set HOMEDIR from a task parameter and use the quoted environment variable to locate report-json.json. JSON formatting and 8,000-character line wrapping remain unchanged.

Changes

Report JSON path handling

Layer / File(s) Summary
Update report path in verification tasks
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml, tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
Both report-json steps set HOMEDIR from the task parameter and use the quoted variable in the JSON input path. Formatting and line wrapping remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: cuipinghuo, simonbaird

Merge Risk: ⚪ Minimal · up to e8463

The report steps continue to read the JSON files from the paths where they are written. No actionable merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing shell interpolation in task reports.
Description check ✅ Passed The description explains what changed and why. It does not include the template headings or a Tickets section, but it provides the essential change context and is mostly complete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@qodo-for-conforma

Copy link
Copy Markdown

PR Summary by Qodo

Fix shell interpolation in canonical task reports

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Pass HOMEDIR through the step environment in both canonical report tasks.
• Quote the report path so parameter values cannot become shell command text.
Diagram

graph TD
  P["HOMEDIR parameter"] --> E["Step environment"] --> J["jq report reader"] --> A["awk line wrapper"] --> L["Task logs"]
  F["JSON report file"] --> J
Loading
High-Level Assessment

Passing HOMEDIR through the step environment is a direct fix for avoiding parameter substitution into shell command text. Shell positional arguments could also separate the value from the script, but add complexity without a clear benefit here.

Files changed (2) +8 / -2

Bug fix (2) +8 / -2
verify-conforma-konflux-ta.yamlQuote the Konflux report path from HOMEDIR +4/-1

Quote the Konflux report path from HOMEDIR

• The report-json step now receives HOMEDIR as an environment variable and uses its quoted value when reading the JSON report. This keeps the parameter value out of the shell command text.

tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml

verify-enterprise-contract.yamlQuote the enterprise contract report path from HOMEDIR +4/-1

Quote the enterprise contract report path from HOMEDIR

• The report-json step now receives HOMEDIR as an environment variable and uses its quoted value when reading the JSON report. This applies the same shell-interpolation fix to the other canonical task.

tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml

@qodo-for-conforma

qodo-for-conforma Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Catalog reports still expand user paths ⊘ Outdated
Description
The two canonical tasks now pass HOMEDIR through the environment, but the corresponding task
copies in conforma/tekton-catalog still interpolate $(params.HOMEDIR) directly into the shell
command. Catalog consumers therefore continue running the old report formatter until the automated
catalog synchronization is merged.
Code

tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[R478-480]

+      env:
+        - name: HOMEDIR
+          value: "$(params.HOMEDIR)"
Relevance

●● Moderate

Catalog synchronization is cross-repository and separately generated; history lacks decisive
precedent for requiring it here.

PR-#3118

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR changes both source tasks to expose HOMEDIR as an environment variable and reference
${HOMEDIR}, while the pinned catalog copies retain the direct $(params.HOMEDIR) shell
interpolation. The catalog synchronization script confirms these task definitions are copied from
the CLI repository through a generated update rather than changing automatically in the same
repository.

cli -> tekton-catalog
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[478-488]
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml[574-584]
External repo: conforma/tekton-catalog, tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml [478-490]
External repo: conforma/tekton-catalog, tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml [574-586]
External repo: conforma/tekton-catalog, hack/sync-ec-cli-tasks.sh [56-76]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The source task definitions now avoid inserting `HOMEDIR` directly into shell command text, but the pinned copies in `conforma/tekton-catalog` still use `$(params.HOMEDIR)` in the report-formatting command. Catalog consumers continue to run the unsafe command until the synchronized task update is published.

## Fix Focus Areas
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[478-480]
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml[574-576]
- /cross_repos/tekton-catalog/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[478-490]
- /cross_repos/tekton-catalog/tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml[574-586]

## Recommended Fix
Run or trigger the tekton-catalog synchronization for both task definitions, verify that it adds the `HOMEDIR` environment variable and changes the command to use `${HOMEDIR}`, then merge the generated catalog update before relying on the fix in catalog-published tasks.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 38 rules
✅ Cross-repo context — repo relationships
  Explored: repo: conforma/infra-deployments-ci (sha: 0fff65f3) — View relationship
  Explored: repo: konflux-ci/konflux-ci (sha: 215770f8) — View relationship
  Explored: repo: konflux-ci/build-definitions (sha: e67f273d) — View relationship
  Explored: repo: konflux-ci/tekton-integration-catalog (sha: 24ed4b2b) — View relationship
  Explored: repo: conforma/e2e-tests (sha: 1cd58e76) — View relationship
  Explored: repo: conforma/tekton-catalog (sha: 8df20d1b) — View relationship
  Explored: repo: redhat-appstudio/infra-deployments (sha: 562b33c4) — View relationship
Review mode: 🚀 Fast: This is a small, localized shell-quoting fix duplicated across two equivalent task manifests, with contained behavioral impact and no high-risk concerns.

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Pass `HOMEDIR` through the environment before formatting report output so parameter values are not inserted into shell command text in either canonical task.

Co-Authored-By: Codex <codex@openai.com>

Ref: EC-2048
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Ended 2:35 PM UTC

Commit: 00145c8 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (ensuring provider "vertex-ai": provider create "vertex-ai" failed: exit status 1 (output: Error: × code: 'Client specified an invalid argument', message: "provider │ credentials are not declared by pr…) · Started 2:37 PM UTC · Completed 2:37 PM UTC

Commit: e84631d · View workflow run →

Effort: high

@codecov

codecov Bot commented Oct 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
acceptance 54.51% <ø> (ø)
generative 12.25% <ø> (ø)
integration 23.56% <ø> (ø)
unit 72.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@robnester-rh
robnester-rh merged commit d34d649 into conforma:main Oct 5, 2026
45 of 47 checks passed
@fullsend-ai-retro

Copy link
Copy Markdown

🤖 Finished Retro · ❌ Failure (ensuring provider "github-artifacts": provider create "github-artifacts" failed: exit status 1 (output: Error: × code: 'Client specified an invalid argument', message: "provider │ credentials are not…) · Started 9:24 PM UTC · Completed 9:24 PM UTC

Commit: e84631d · View workflow run →

Effort: high

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants