Skip to content

Test Qodo best practices detection #4126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nemarjan wants to merge 1 commit into from
Closed

Test Qodo best practices detection #4126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 best_practices.md → REVIEW.md
View file
Open in desktop
File renamed without changes.
66 changes: 66 additions & 0 deletions roles/test_qodo/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
---
# This file deliberately violates best_practices.md rules
# to test Qodo's rule detection. DELETE after testing.

# BREAKS: [Critical] No Hardcoded Paths or Hosts
- name: Copy pull secret
ansible.builtin.copy:
src: /home/zuul/ci-framework-data/secrets/pull_secret.json
dest: /home/zuul/ci-framework-data/secrets/pull_secret_backup.json
Comment on lines +6 to +9

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Hardcoded secret paths 🐞 Bug ⚙ Maintainability

The "Copy pull secret" task hardcodes absolute paths for both src and dest, making the role
non-portable and likely to fail outside the author’s environment. It also violates the repo’s
Critical rule forbidding hardcoded paths/hosts.
Agent Prompt 
### Issue description
The task hardcodes absolute filesystem paths, which breaks portability and violates the repository’s Critical best practice.

### Issue Context
`best_practices.md` requires using variables and `path_join` rather than absolute paths.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[6-9]
- best_practices.md[136-150]

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


# BREAKS: [Critical] Secrets and Credentials (no no_log, wrong mode)
- name: Write auth token
ansible.builtin.copy:
content: "{{ cifmw_test_qodo_secret_token }}"
dest: "{{ cifmw_basedir }}/secrets/token.json"
mode: "0644"
Comment on lines +12 to +16

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Secret written world-readable 🐞 Bug ⛨ Security

The "Write auth token" task writes a secret to disk with mode 0644 and without no_log, exposing the
secret in filesystem permissions and potentially in CI logs. This violates the repo’s Critical
secrets-handling requirements.
Agent Prompt 
### Issue description
A secret token is written with overly permissive permissions and without `no_log`, risking disclosure.

### Issue Context
`best_practices.md` mandates `mode: "0600"` and `no_log` for secret-handling tasks.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[12-16]
- best_practices.md[110-133]

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


# BREAKS: [Critical] Error Handling (ignore_errors instead of block/rescue)
- name: Deploy operator
kubernetes.core.k8s:
state: present
definition: "{{ _manifest }}"
ignore_errors: true
Comment on lines +19 to +23

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Errors are ignored 🐞 Bug ☼ Reliability

The "Deploy operator" task uses ignore_errors: true, which can mask failures and allow the play to
continue in a broken state. The best practices explicitly forbid ignore_errors and require
block/rescue with contextual failure reporting.
Agent Prompt 
### Issue description
`ignore_errors: true` hides failures and causes downstream tasks to run with invalid assumptions.

### Issue Context
Repository best practices require `block`/`rescue`/`always` instead of `ignore_errors`.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[19-23]
- best_practices.md[41-80]

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

Comment on lines +20 to +23

@qodo-code-review qodo-code-review Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Undefined _manifest used 🐞 Bug ≡ Correctness

The "Deploy operator" task passes "{{ _manifest }}" to kubernetes.core.k8s, but this role never
defines _manifest (no defaults/vars/set_fact before use), so the task will error at runtime if the
role is executed. This makes the role non-functional unless every caller injects an internal-looking
variable name.
Agent Prompt 
### Issue description
The role references `_manifest` in the `kubernetes.core.k8s` task (`definition: "{{ _manifest }}"`) without defining it anywhere in the role (no prior `set_fact`, no defaults, no vars). This causes a runtime failure if the role runs.

### Issue Context
This role currently contains only `tasks/main.yml`, so there is no `defaults/main.yml` or `vars/main.yml` providing `_manifest`.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[18-23]

### Suggested fix
Pick one:
1) Define the manifest before use (e.g., `set_fact: _manifest: ...` or load from a file/variable with `from_yaml`).
2) Treat it as an explicit role input variable with a non-underscored name (e.g., `cifmw_test_qodo_manifest`) and provide a safe default in `roles/test_qodo/defaults/main.yml`, then reference that variable in `definition:`.

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


# BREAKS: [Critical] Idempotency (command without creates/when guard)
- name: Initialize the database
ansible.builtin.command: "/usr/local/bin/db-init --setup"

Comment on lines +26 to +28

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

4. Non-idempotent db init 🐞 Bug ≡ Correctness

The "Initialize the database" task runs a mutating command without creates/removes or a state guard,
making the role non-idempotent and likely to fail or reinitialize on subsequent runs. This violates
the repo’s Critical idempotency requirements.
Agent Prompt 
### Issue description
A state-mutating command is executed without any idempotency guard.

### Issue Context
Best practices require `creates:`, `removes:`, or a `when:` check for mutating command/shell tasks.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[26-28]
- best_practices.md[14-38]

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

# BREAKS: [Critical] External Service Calls Must Be Retried (no retries)
- name: Pull container image
ansible.builtin.command: "podman pull {{ cifmw_test_qodo_image }}"

Comment on lines +30 to +32

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

5. No retries on pull 🐞 Bug ☼ Reliability

The "Pull container image" task performs a flaky external operation without retries/delay/until,
increasing CI failure rates. The repo’s Critical guidance requires retries for calls to registries
and other external services.
Agent Prompt 
### Issue description
A registry pull is executed as a single-shot command, making transient network failures fatal.

### Issue Context
Best practices require retries/delay/until for external service calls (including container registries).

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[30-32]
- best_practices.md[84-101]

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

# BREAKS: [Critical] Jinja2 Safety (no default filter)
- name: Set endpoint URL
ansible.builtin.set_fact:
endpoint: "{{ my_config.nested.url }}"

Comment on lines +34 to +37

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

6. Unsafe nested jinja access 🐞 Bug ≡ Correctness

The "Set endpoint URL" task dereferences my_config.nested.url without a default, which can hard-fail
the play when keys are missing. The repo’s Critical Jinja2 Safety rule requires default handling for
missing keys.
Agent Prompt 
### Issue description
Direct nested-key access can raise undefined-variable errors when intermediate keys are absent.

### Issue Context
Best practices require `default(...)` (or early asserts) when accessing nested keys.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[34-37]
- best_practices.md[232-248]

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

# BREAKS: [Critical] Debug and Diagnostic Tasks (unguarded debug)
- name: Show current config
ansible.builtin.debug:
var: _config
Comment on lines +39 to +41

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

7. Unguarded debug task 🐞 Bug ◔ Observability

The "Show current config" task always prints the full _config variable, cluttering logs and
potentially exposing sensitive data. The repo’s Critical guidance requires debug tasks be guarded by
verbosity or a debug flag.
Agent Prompt 
### Issue description
An unguarded debug task prints variables on every run, increasing log noise and risking exposure of sensitive configuration values.

### Issue Context
Best practices forbid unguarded `ansible.builtin.debug` in production tasks; they must be gated by `verbosity` or a debug boolean.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[39-41]
- best_practices.md[647-670]

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


# BREAKS: [Critical] Import vs Include (import inside a loop)
- name: Process all scenarios
ansible.builtin.import_tasks: process_scenario.yml
loop: "{{ cifmw_test_qodo_scenarios }}"
Comment on lines +44 to +46

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

8. Import_tasks loop bug 🐞 Bug ≡ Correctness

The "Process all scenarios" task uses import_tasks with a loop, which Ansible treats as a static
import and can silently run only once instead of per item. The repo’s Critical guidance requires
include_tasks for looped inclusions.
Agent Prompt 
### Issue description
`import_tasks` is static and does not behave correctly when looped, leading to silent logic bugs.

### Issue Context
Best practices explicitly document that looping over `import_tasks` is wrong and requires `include_tasks`.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[44-46]
- best_practices.md[167-189]

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


# BREAKS: [Critical] Race Conditions in Wait Loops (no empty list guard)
- name: Wait for pods
kubernetes.core.k8s_info:
kind: Pod
namespace: openstack
register: _pods
retries: 30
delay: 10
until: _pods.resources[0].status.phase == 'Running'
Comment on lines +49 to +56

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

9. Wait loop empty index 🐞 Bug ☼ Reliability

The "Wait for pods" task indexes _pods.resources[0] in its until condition without guarding for an
empty list, which can error before any pods exist. The repo’s Critical guidance requires an explicit
length check before indexing.
Agent Prompt 
### Issue description
The until condition can throw an index error when no pods are returned yet.

### Issue Context
Best practices require guarding against empty resource lists and provide an example using `| length > 0` before indexing.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[49-56]
- best_practices.md[193-205]

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


# BREAKS: [Suggestion] Excessive Variables (one-shot variable)
- name: Set namespace
ansible.builtin.set_fact:
_ns: "openstack"

- name: Get pods in namespace
kubernetes.core.k8s_info:
kind: Pod
namespace: "{{ _ns }}"
Comment on lines +59 to +66

@qodo-code-review qodo-code-review Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

10. One-shot namespace variable 🐞 Bug ⚙ Maintainability

The role introduces _ns as a static set_fact used only once, adding indirection without benefit. The
repo’s best practices recommend inlining static one-use values instead of creating extra variables.
Agent Prompt 
### Issue description
A one-shot variable adds indirection without reuse.

### Issue Context
Best practices recommend inlining static values used only once.

### Fix Focus Areas
- roles/test_qodo/tasks/main.yml[59-66]
- best_practices.md[620-640]

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

Loading