Skip to content

sync: mirror from thyn-ai/codna@3177a0b861e68aede678cf2c28e8697690cf3855 #35

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
0xamlab wants to merge 1 commit into from
Closed

sync: mirror from thyn-ai/codna@3177a0b861e68aede678cf2c28e8697690cf3855 #35

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
38 changes: 18 additions & 20 deletions codna/testrun.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ def _detect_pytest_suites(repo_dir: str) -> list[str]:
continue
if _has_pytest_config(sub):
suites.append(sub)
continue
if name not in _CONTAINER_DIRS:
continue
try:
Expand Down Expand Up @@ -344,17 +345,19 @@ def discover_failing_tests(repo_dir: str, test_cmd: str | None = None, *, timeou
"budget. Point Codna at the right one with fix.test_command in codna.yaml or "
"--test-cmd \"<your test command>\" (writing JUnit to $CODNA_JUNIT)."
)
# Each suite runs from its own root so its pyproject/pytest.ini applies. Distribute the
# caller's total integer-second budget as evenly as possible without exceeding it.
base_budget, extra_seconds = divmod(timeout_seconds, len(suites))
# Each suite runs from its own root so its pyproject/pytest.ini applies; the overall budget
# is split evenly (deterministic). The 60s floor is safe: _MAX_SUITES caps the count, so
# floor * suites can never exceed 8*60s = 480s even when timeout_seconds is small.
per_suite = max(60, timeout_seconds // len(suites))

@codna-ai codna-ai Bot Oct 31, 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.

🟡 MEDIUM · correctness — Per-suite budget floor can cause total wall time to exceed timeout_seconds

per_suite = max(60, timeout_seconds // len(suites)) — when timeout_seconds < 60 * len(suites), each suite is allocated 60 s and the aggregate budget becomes 60 * len(suites), which can be up to 60 * 8 = 480 s regardless of what timeout_seconds was. A caller that passes timeout_seconds=120 with 4 suites will wait up to 240 s before the last suite times out, violating the contract. The comment's reasoning ('floor * suites can never exceed 8*60s') describes an absolute maximum but not a bound relative to timeout_seconds.

Reply @codna fix and Codna opens a verified, test-green, risk-gated fix PR.

runs = []
for i, suite in enumerate(suites):
suite_junit = junit if len(suites) == 1 else f"{os.path.splitext(junit)[0]}-{i}.xml"
argv = [sys.executable, "-m", "pytest", "-q"]
# A container suite must not recursively collect nested roots that run independently.
argv.extend(f"--ignore={candidate}" for candidate in suites if os.path.dirname(candidate) == suite)
argv.append(f"--junitxml={suite_junit}")
runs.append((suite, argv, suite_junit, base_budget + (1 if i < extra_seconds else 0)))
runs.append((
suite,
[sys.executable, "-m", "pytest", "-q", f"--junitxml={suite_junit}"],
suite_junit,
per_suite,
))
Comment on lines +357 to +360

@codna-ai codna-ai Bot Oct 31, 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.

🔴 HIGH · correctness — Removed --ignore flags cause nested suites to be double-collected

The diff deletes the --ignore arguments that prevented a parent pytest suite from recursively collecting tests in child suite directories. Previously: argv.extend(f"--ignore={candidate}" for candidate in suites if os.path.dirname(candidate) == suite). Without these flags, when suite A is an ancestor of suite B in the file system (e.g., repo/tests and repo/tests/integration), running pytest on suite A will also collect all tests under suite B. Those tests are then run and reported again when suite B is processed. This causes duplicate test IDs in the failing list, inflated failure counts, and incorrect len(failing) in the returned summary.

Suggested change
[sys.executable, "-m", "pytest", "-q", f"--junitxml={suite_junit}"],
suite_junit,
per_suite,
))
Restore the --ignore= arguments:
```python
argv = [sys.executable, "-m", "pytest", "-q"]
argv.extend(f"--ignore={candidate}" for candidate in suites if os.path.dirname(candidate) == suite)
argv.append(f"--junitxml={suite_junit}")
runs.append((suite, argv, suite_junit, per_suite)) 

_Reply `@codna fix` and Codna opens a verified, test-green, risk-gated fix PR._

<!-- codna:review:fp=b1f30f6472f67457 -->
<!-- codna:finding eyJmcCI6ICJiMWYzMGY2NDcyZjY3NDU3IiwgInBhdGgiOiAiY29kbmEvdGVzdHJ1bi5weSIsICJsaW5lIjogMzU3LCAiZW5kX2xpbmUiOiAzNjAsICJzZXZlcml0eSI6ICJoaWdoIiwgImNhdGVnb3J5IjogImNvcnJlY3RuZXNzIiwgInRpdGxlIjogIlJlbW92ZWQgLS1pZ25vcmUgZmxhZ3MgY2F1c2UgbmVzdGVkIHN1aXRlcyB0byBiZSBkb3VibGUtY29sbGVjdGVkIiwgImV4cGxhbmF0aW9uIjogIlRoZSBkaWZmIGRlbGV0ZXMgdGhlIGAtLWlnbm9yZWAgYXJndW1lbnRzIHRoYXQgcHJldmVudGVkIGEgcGFyZW50IHB5dGVzdCBzdWl0ZSBmcm9tIHJlY3Vyc2l2ZWx5IGNvbGxlY3RpbmcgdGVzdHMgaW4gY2hpbGQgc3VpdGUgZGlyZWN0b3JpZXMuIFByZXZpb3VzbHk6IGBhcmd2LmV4dGVuZChmXCItLWlnbm9yZT17Y2FuZGlkYXRlfVwiIGZvciBjYW5kaWRhdGUgaW4gc3VpdGVzIGlmIG9zLnBhdGguZGlybmFtZShjYW5kaWRhdGUpID09IHN1aXRlKWAuIFdpdGhvdXQgdGhlc2UgZmxhZ3MsIHdoZW4gc3VpdGUgQSBpcyBhbiBhbmNlc3RvciBvZiBzdWl0ZSBCIGluIHRoZSBmaWxlIHN5c3RlbSAoZS5nLiwgYHJlcG8vdGVzdHNgIGFuZCBgcmVwby90ZXN0cy9pbnRlZ3JhdGlvbmApLCBydW5uaW5nIHB5dGVzdCBvbiBzdWl0ZSBBIHdpbGwgYWxzbyBjb2xsZWN0IGFsbCB0ZXN0cyB1bmRlciBzdWl0ZSBCLiBUaG9zZSB0ZXN0cyBhcmUgdGhlbiBydW4gYW5kIHJlcG9ydGVkIGFnYWluIHdoZW4gc3VpdGUgQiBpcyBwcm9jZXNzZWQuIFRoaXMgY2F1c2VzIGR1cGxpY2F0ZSB0ZXN0IElEcyBpbiB0aGUgYGZhaWxpbmdgIGxpc3QsIGluZmxhdGVkIGZhaWx1cmUgY291bnRzLCBhbmQgaW5jb3JyZWN0IGBsZW4oZmFpbGluZylgIGluIHRoZSByZXR1cm5lZCBzdW1tYXJ5LiJ9 -->


# Actually ENFORCE network denial (bwrap/unshare on Linux CI) — not just record it — so untrusted
# test code can't exfiltrate. When fail-closed egress is required (privacy.egress) but no kernel
Expand All @@ -368,15 +371,14 @@ def discover_failing_tests(repo_dir: str, test_cmd: str | None = None, *, timeou

failing: list[str] = []
per_suite_failing: list[tuple[str, list[str]]] = []
crashed: list[tuple[str, bool]] = []
crashed: list[str] = []
saw_failure_rc = False
last_tail = ""
for run_dir, argv, run_junit, budget in runs:
run_env = {**env, "CODNA_JUNIT": run_junit}
# pytest exits 1 when tests fail — that is the expected/accepted case here (not an error).
result = Sandbox(
network="deny", timeout_seconds=budget, env=run_env, network_backend=backend
).run(argv, cwd=run_dir, accepted_exit_codes=(0, 1))
result = Sandbox(network="deny", timeout_seconds=budget, env=env, network_backend=backend).run(
argv, cwd=run_dir, accepted_exit_codes=(0, 1)
)
if result.timed_out:
raise RuntimeError(f"test run timed out after {budget}s")

Expand Down Expand Up @@ -413,7 +415,7 @@ def discover_failing_tests(repo_dir: str, test_cmd: str | None = None, *, timeou
failing.extend(suite_failing)
elif result.returncode not in (0,):
saw_failure_rc = True
crashed.append((run_dir, os.path.isfile(run_junit)))
crashed.append(run_dir)
last_tail = (result.stdout or result.stderr or "")[-1500:]

if failing:
Expand All @@ -425,12 +427,8 @@ def discover_failing_tests(repo_dir: str, test_cmd: str | None = None, *, timeou
]
# A suite that crashed without producing JUnit ids is never dropped from the report just
# because a sibling suite produced some — its tail is real failure evidence too.
for suite, has_report in crashed:
message = (
"exited non-zero with JUnit report but no failing test ids"
if has_report else "exited non-zero with no JUnit report"
)
parts.append(f"{os.path.relpath(suite, repo_dir)}: {message}")
for suite in crashed:
parts.append(f"{os.path.relpath(suite, repo_dir)}: exited non-zero with no JUnit report")

@codna-ai codna-ai Bot Oct 31, 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.

🟡 MEDIUM · correctness — Crashed-suite diagnostic always reports 'no JUnit report' even when one exists

The old code stored (run_dir, os.path.isfile(run_junit)) in crashed and produced a different message when a JUnit file was present ("exited non-zero with JUnit report but no failing test ids") versus absent ("exited non-zero with no JUnit report"). The diff simplifies crashed to list[str] and unconditionally emits "exited non-zero with no JUnit report". A suite that exits non-zero but produces a JUnit report without failing test cases (e.g., plugin errors, teardown errors, or the file was written before the crash) is now misdiagnosed. Callers parsing the returned message will take the wrong remediation path.

Reply @codna fix and Codna opens a verified, test-green, risk-gated fix PR.

involved = len(per_suite_failing) + len(crashed)
return (
f"{len(failing)} failing test(s) across {involved} suite(s) — "
Expand Down
Loading
Loading