fix(github-action): handle string "false" for ENABLE_OUTPUT setting

[gvago]

Summary

• Bug: GITHUB_ACTION_CONFIG.ENABLE_OUTPUT can arrive as a string "false" from environment variables or YAML config. Since non-empty strings are truthy in Python, if enable_output: evaluates to True even when the user explicitly set it to "false", causing unwanted GitHub Action output writes.
• Fix: Added explicit string-to-boolean coercion at both the ingestion point (github_action_runner.py) and the consumption point (utils.py), treating "false", "0", "no", and "" as falsy.
• Tests: Added two new test cases verifying that string "false" disables output and string "true" enables it.

Test plan

• Existing 4 tests continue to pass (bool True, bool False, not set, error case)
• New test_github_action_output_disabled_string_false — string "false" does not write to GITHUB_OUTPUT
• New test_github_action_output_enabled_string_true — string "true" correctly writes to GITHUB_OUTPUT
• Manual: set enable_output: "false" in a GitHub Action workflow and verify no output is written

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix string "false" truthiness for ENABLE_OUTPUT setting

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Handle string "false" for ENABLE_OUTPUT setting in GitHub Actions
  - Added explicit string-to-boolean coercion treating "false", "0", "no", and "" as falsy
  - Applied fix at both ingestion point (github_action_runner.py) and consumption point (utils.py)
• Added two new test cases verifying string boolean handling
  - test_github_action_output_disabled_string_false validates "false" disables output
  - test_github_action_output_enabled_string_true validates "true" enables output

Diagram

flowchart LR
  A["Environment Variables<br/>YAML Config"] -->|"String 'false'<br/>arrives as string"| B["github_action_runner.py<br/>Coerce to boolean"]
  B -->|"Boolean value"| C["get_settings()"]
  C -->|"Retrieve setting"| D["utils.py<br/>github_action_output"]
  D -->|"Coerce if string"| E["Check enable_output<br/>value"]
  E -->|"False"| F["Skip output write"]
  E -->|"True"| G["Write to GITHUB_OUTPUT"]

Loading

File Changes

1. pr_agent/algo/utils.py 🐞 Bug fix +4/-1

Add string-to-boolean coercion in output function

• Added explicit string-to-boolean coercion in github_action_output() function
• Treats "false", "0", "no", and empty string as falsy values
• Converts string values to lowercase and strips whitespace before comparison
• Prevents non-empty string "false" from being evaluated as truthy

pr_agent/algo/utils.py

---

2. pr_agent/servers/github_action_runner.py 🐞 Bug fix +2/-0

Add string-to-boolean coercion at setting ingestion

• Added explicit string-to-boolean coercion when reading ENABLE_OUTPUT setting
• Handles "false", "0", "no", and empty string as falsy values
• Converts string values to lowercase and strips whitespace before comparison
• Ensures consistent boolean handling at the ingestion point

pr_agent/servers/github_action_runner.py

---

3. tests/unittest/test_github_action_output.py 🧪 Tests +29/-0

Add tests for string boolean handling

• Added test_github_action_output_disabled_string_false() to verify string "false" disables output
• Added test_github_action_output_enabled_string_true() to verify string "true" enables output
• Tests validate that string boolean values are properly coerced and handled
• Ensures GITHUB_OUTPUT file is created only when output is enabled

tests/unittest/test_github_action_output.py

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)

🐞\ ☼ Reliability (1) ⚙ Maintainability (1)

1. Duplicated bool coercion 🐞 ⚙

Description

ENABLE_OUTPUT string-to-boolean coercion is implemented separately in run_action() and
github_action_output(); if either side changes later, behavior can diverge and reintroduce
inconsistent output enabling/disabling. This is a maintainability risk because the same parsing
rules must be kept in sync in two places.

Code

pr_agent/algo/utils.py[R1259-1262]

+        enable_output = get_settings().get('github_action_config.enable_output', False)
+        if isinstance(enable_output, str):
+            enable_output = enable_output.lower().strip() not in ("false", "0", "no", "")
+        if not enable_output:

Evidence

The PR adds the same string-to-bool coercion expression in both the ingestion point (GitHub Action
runner) and the consumption point (github_action_output), duplicating the parsing rules (same
normalization + same falsy set). This duplication increases the chance of future drift (e.g., adding
a new falsy token in one location only).

pr_agent/algo/utils.py[1259-1262]
pr_agent/servers/github_action_runner.py[63-66]

Agent prompt

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

### Issue description
ENABLE_OUTPUT string-to-bool parsing is duplicated in two places, which can drift over time.

### Issue Context
Both locations currently use the same `lower().strip() not in ("false", "0", "no", "")` logic.

### Fix Focus Areas
- pr_agent/servers/github_action_runner.py[63-66]
- pr_agent/algo/utils.py[1259-1262]

### Suggested change
Extract a small helper (e.g., `coerce_env_bool(value, default=False)` or similar) in a shared module (or in `utils.py` if appropriate), and call it from both locations so the falsy/true rules live in exactly one place.

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

---

2. Brittle output parsing test 🐞 ☼

Description

The new test parses the GitHub output line using split('=') without limiting to the first
delimiter; if the JSON payload ever contains '=' inside a string value, the test will mis-parse and
fail (test-only flake). This reduces test reliability and can cause false negatives unrelated to
ENABLE_OUTPUT behavior.

Code

tests/unittest/test_github_action_output.py[R68-70]

+        actual_key = env_value.split('=')[0]
+        actual_data = json.loads(env_value.split('=')[1])
+

Evidence

github_action_output writes output as f"{key}={json.dumps(...)}" (single line). JSON string values
can legally include '=' characters, so a plain split('=') can produce more than two segments and
break json.loads(...) in the test; using split('=', 1) (or partition('=')) avoids this
brittleness.

tests/unittest/test_github_action_output.py[68-70]
pr_agent/algo/utils.py[1265-1267]

Agent prompt

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

### Issue description
Test parses `${key}=${json}` using `split('=')`, which is brittle if the JSON contains '=' inside string values.

### Issue Context
This is test-only, but can cause flaky failures as test payloads evolve.

### Fix Focus Areas
- tests/unittest/test_github_action_output.py[68-70]

### Suggested change
Replace:
- `actual_key = env_value.split('=')[0]`
- `actual_data = json.loads(env_value.split('=')[1])`

With something like:
- `actual_key, _, json_part = env_value.partition('=')`
- `actual_data = json.loads(json_part)`

(or `split('=', 1)`) to ensure only the first '=' is treated as the delimiter.

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

---

ⓘ The new review experience is currently in Beta. Learn more