chore: improve unstract maintenance path by lphuc2250gma · Pull Request #2023 · Zipstack/unstract

lphuc2250gma · 2026-06-09T02:01:58Z

Summary:

Add or tighten focused edge-case tests or type assertions in tests/rig/cli.py, tests/conftest.py related to Python typing, tests, CLI ergonomics, observability; avoid docs-only changes and broad refactors.
Keep the change narrow so it is straightforward to review.

Notes:

I kept this scoped to the relevant implementation and tests.

CLAassistant · 2026-06-09T02:02:03Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

coderabbitai · 2026-06-09T02:02:11Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d7953a84-9019-4a52-9b0e-e70a171b3a30

📥 Commits

Reviewing files that changed from the base of the PR and between 2877bd8 and 9015501.

📒 Files selected for processing (2)

tests/rig/cli.py
tests/rig/tests/test_cli.py

Summary by CodeRabbit

Bug Fixes
- Fixed handling of the --timeout 0 command-line argument to correctly override the group's default timeout setting instead of being replaced by it.

Walkthrough

The PR fixes a bug where cmd_run incorrectly treated --timeout 0 as a falsy value and fell back to group defaults. The fix replaces the args.timeout or group.timeout_seconds logic with an explicit None check, so zero is honored as a valid override. A new test validates the fix.

Changes

Timeout Zero Override Fix

Layer / File(s)	Summary
Explicit None check for timeout argument `tests/rig/cli.py`, `tests/rig/tests/test_cli.py`	`cmd_run` now uses explicit `None` check (`group.timeout_seconds if args.timeout is None else args.timeout`) when passing timeout to `_execute_group`, respecting zero as a valid override. New test `test_timeout_zero_is_respected` validates `--timeout 0` is honored by capturing the timeout kwarg and verifying it equals `0`.

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description provides high-level objectives but lacks specific details required by the template, such as What, Why, How, impact analysis, and testing notes.	Fill in the required template sections (What, Why, How, impact analysis, testing notes) with concrete details about the timeout fix and test addition.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title is vague and generic, using non-descriptive terms like 'chore' and 'maintenance path' that don't clearly convey the specific changes being made.	Replace with a specific, descriptive title that highlights the main change, e.g., 'fix: handle zero timeout value correctly in CLI group execution' or 'test: add test for zero timeout handling'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

sonarqubecloud · 2026-06-09T02:02:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

greptile-apps · 2026-06-09T02:04:32Z

Greptile Summary

This PR fixes a bug where --timeout 0 was silently overridden by the group's default timeout because the previous args.timeout or group.timeout_seconds expression treats 0 as falsy. The fix replaces it with an explicit is None sentinel check, and adds a focused regression test that monkeypatches _execute_group to confirm the value is forwarded unchanged.

tests/rig/cli.py: Single-line change replacing args.timeout or group.timeout_seconds with group.timeout_seconds if args.timeout is None else args.timeout, matching argparse's None-when-absent contract for type=int arguments.
tests/rig/tests/test_cli.py: Adds test_timeout_zero_is_respected — patches the manifest, registry, and _execute_group; asserts captured["timeout"] == 0 after a cmd_run invocation with --timeout 0.

Confidence Score: 5/5

Safe to merge — the change is a one-line sentinel fix with no side effects on the normal path and a new test that directly exercises the previously broken case.

The fix is minimal and surgical: it corrects one expression, the argparse default=None contract is well-established, and the new test reproduces the exact failure mode (zero being swallowed by the or) and asserts the repaired behavior end-to-end through cmd_run. No other code paths are touched.

No files require special attention.

Important Files Changed

Filename	Overview
tests/rig/cli.py	Replaces falsy `or`-based timeout fallback with an explicit `is None` check, correctly allowing `--timeout 0` to propagate to `_execute_group`.
tests/rig/tests/test_cli.py	Adds `test_timeout_zero_is_respected` which monkeypatches `_execute_group` to verify `--timeout 0` is forwarded as `0` and not silently overridden by the group default.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["CLI: parse_args()"] --> B{"args.timeout is None?"}
    B -- "Yes (--timeout not supplied)" --> C["use group.timeout_seconds"]
    B -- "No (--timeout 0 or N)" --> D["use args.timeout (0 or N)"]
    C --> E["_execute_group(..., timeout=group.timeout_seconds)"]
    D --> E
    E --> F["_spawn(cmd, timeout=T)\nsubprocess.run(..., timeout=T+30)"]
    E --> G["pytest --timeout=T"]

_{Reviews (1): Last reviewed commit: "chore: improve unstract maintenance path" | Re-trigger Greptile}

chore: improve unstract maintenance path

9015501

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve unstract maintenance path#2023

chore: improve unstract maintenance path#2023
lphuc2250gma wants to merge 1 commit into
Zipstack:mainfrom
lphuc2250gma:maint/20260609015205

lphuc2250gma commented Jun 9, 2026

Uh oh!

CLAassistant commented Jun 9, 2026

Uh oh!

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

❌ Failed checks (2 warnings, 1 inconclusive)

Uh oh!

sonarqubecloud Bot commented Jun 9, 2026

Uh oh!

greptile-apps Bot commented Jun 9, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lphuc2250gma commented Jun 9, 2026

Uh oh!

CLAassistant commented Jun 9, 2026

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

❌ Failed checks (2 warnings, 1 inconclusive)

Uh oh!

sonarqubecloud Bot commented Jun 9, 2026

Quality Gate passed

Uh oh!

greptile-apps Bot commented Jun 9, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading