fix(testmon): preserve terminal teardown outcomes - #3958
Conversation
📝 WalkthroughWalkthroughThe change adds durable testmon seed shards with deterministic planning, ledger validation, serial execution, checkpoint/resume support, outcome aggregation, and protocol-v7 completion checks. It also updates convergence fixtures, pytest isolation, and freshness snapshot return behavior. ChangesTestmon shard seeding
Test infrastructure and API cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Verify
participant Pytest
participant ShardLedger
participant Checkpoint
Verify->>Pytest: Run collect-only seed selection
Verify->>ShardLedger: Create and validate shard plan
Verify->>Pytest: Execute incomplete shard serially
Pytest-->>Verify: Return node outcomes and metadata
Verify->>Checkpoint: Save shard results and ledger status
Checkpoint-->>Verify: Provide resumable seed state
Verify->>ShardLedger: Validate terminal ledger
ShardLedger-->>Verify: Allow seed finalization
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcaa7ceaf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif nodeid in finished and any( | ||
| report.get("when") == "teardown" and report.get("outcome") == "passed" for report in node_reports | ||
| ): |
There was a problem hiding this comment.
When the durable event stream drops a call report—the exact scenario handled here—a failed test can still emit a passing teardown report and test_finished, because that teardown outcome only describes fixture cleanup. This branch therefore classifies the node as passed even if the testmon database records it as failed, since it runs before the database fallback; if the database row is also missing, _checkpoint_testmon_seed_shard marks the shard complete and subsequent seed resumes skip it permanently. Keep teardown-only nodes nonterminal unless a prior outcome or the database corroborates the call result.
Useful? React with 👍 / 👎.
4b0ee53 to
7eecafe
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
7eecafe to
02b2d0d
Compare
|
@codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eda8751e33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| checkout_cache = Path(__file__).resolve().parents[3] / ".cache" / "testmon" | ||
| if checkout_cache.exists(): | ||
| shutil.move(str(checkout_cache), str(tmp_path / "checkout-testmon-generated")) |
There was a problem hiding this comment.
When this module is run in a checkout that already has a valid seed, the autouse fixture executes after the first test and moves the entire checkout-level .cache/testmon directory into that test's temporary directory, even when the test did not create the cache; nothing restores it afterward. This discards the main checkout's reusable seed, forcing a costly reseed and preventing linked worktrees from bootstrapping from it. Quarantine and restore pre-existing state, and remove only cache artifacts created by this test module.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| # them are correctness-shaped and lack the benchmark marker, so a | ||
| # marker expression alone cannot keep performance probes out of | ||
| # the correctness/testmon corpus. | ||
| "--ignore=tests/benchmarks", |
There was a problem hiding this comment.
This ignore applies to the seed, affected-test, and --all commands, so it removes the entire benchmarks tree rather than only tests marked benchmark. That tree deliberately contains correctness gates such as the unmarked tests in test_rebuild_cost_model.py and the scale_small tests in test_scale_tiers.py, whose module contract says they belong to default verification. These tests consequently disappear from the testmon graph and even the documented full non-integration run, allowing regressions in those paths to pass both normal and terminal merge-train verification; retain the tree and rely on the existing marker expression to exclude actual performance probes.
AGENTS.md reference: AGENTS.md:L342-L343
Useful? React with 👍 / 👎.
| consume_blob_publication_receipt(source_conn, attachment_receipt, attachment_hash_bytes) | ||
| if raw_blob_size != len(payload): | ||
| raise AssertionError(f"published raw payload size drifted for {source_path}") | ||
| raw_id = write_source_raw_session( |
There was a problem hiding this comment.
When a convergence property reaches rebuild_retained_raw_index, this low-level writer has created each raw row with its default revision_kind='unknown' and revision_authority='quarantined', without the census/logical-key evidence produced by admit_raw_observation; it also records the blob reference without publishing the payload bytes into the archive blob store. rebuild_index_from_source_sync validates frozen source authority before replay and rejects these rows as quarantined or undecided, and the retained payload would be unavailable even if that check were bypassed. Restore the admission plus ArchiveBlobPublisher route so the convergence rebuild properties remain executable.
Useful? React with 👍 / 👎.
| assert cold.canonical_provenance_bytes() == original.canonical_provenance_bytes() | ||
|
|
||
| with ArchiveStore.open_existing(archive_root, read_only=False) as reopened: | ||
| replay = reopened.save_annotation_batch(exact_retry) |
There was a problem hiding this comment.
ArchiveStore.open_existing() defaults to read_only=True, so after removing the separate read_only=False context this call reaches _open_user_write_connection() and raises ReadOnlyArchiveError. The test therefore fails before checking either idempotent retry behavior or incompatible provenance; retain the writable reopen or explicitly pass read_only=False before invoking save_annotation_batch.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/devtools/test_pytest_progress_plugin.py (1)
34-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the plugin globals before relocating the cache directory.
Line 35-37 runs
shutil.movebefore lines 38-43 restore the plugin module globals. If the move raises — a permission error, a cross-device link error, or an existing destination — the globals stay mutated and every later test in the session observes leaked_SELECTED_COUNT,_DESELECTED_COUNT, and slowest-report state.Restore the globals first, or wrap the move so it cannot skip restoration.
🐛 Proposed fix
yield - checkout_cache = Path(__file__).resolve().parents[3] / ".cache" / "testmon" - if checkout_cache.exists(): - shutil.move(str(checkout_cache), str(tmp_path / "checkout-testmon-generated")) pytest_progress_plugin._SELECTED_COUNT = selected_count pytest_progress_plugin._DESELECTED_COUNT = deselected_count pytest_progress_plugin._DESELECTED_NODEIDS_SAMPLE[:] = deselected_nodeids pytest_progress_plugin._SLOWEST_REPORTS[:] = slowest_reports pytest_progress_plugin._COLLECTION_STARTED_AT = collection_started_at pytest_progress_plugin._COLLECTION_DURATION_S = collection_duration_s + checkout_cache = Path(__file__).resolve().parents[3] / ".cache" / "testmon" + if checkout_cache.exists(): + shutil.move(str(checkout_cache), str(tmp_path / "checkout-testmon-generated"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/devtools/test_pytest_progress_plugin.py` around lines 34 - 43, Update the fixture cleanup around pytest_progress_plugin global restoration so _SELECTED_COUNT, _DESELECTED_COUNT, _DESELECTED_NODEIDS_SAMPLE, _SLOWEST_REPORTS, _COLLECTION_STARTED_AT, and _COLLECTION_DURATION_S are restored before calling shutil.move on checkout_cache, or guarantee restoration with a finally block if the move fails.devtools/verify.py (1)
1980-2000: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegister the unmarked benchmark tests
tests/benchmarks/test_schema_linear_scaling.pyandtests/benchmarks/test_fts_trigger_amPLIFICATION.pyare excluded from all correctness lanes by design, but neither file is a registereddevtools bench campaigntarget. Add@pytest.mark.benchmarkand register both files, or move their correctness tests out oftests/benchmarks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devtools/verify.py` around lines 1980 - 2000, Register the unmarked benchmark files tests/benchmarks/test_schema_linear_scaling.py and tests/benchmarks/test_fts_trigger_amPLIFICATION.py as devtools bench campaign targets, and add the pytest benchmark marker to their tests. Keep them excluded from correctness lanes through the existing benchmark filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/verify.py`:
- Around line 2698-2714: Update the shard status handling around
`selection_matches` and the `shard.update` payload to persist an explicit
selection-mismatch diagnostic when pytest’s selected nodeids differ from
`nodeids`. Record the expected and actual node sets (or an equivalent structured
mismatch field) in the shard receipt while preserving the existing
`COMPLETE`/`INCOMPLETE` determination and outcome tracking.
- Line 2636: Update the expected_digest expression in the relevant verification
flow to hash "\n".join(sorted(expected)) instead of the unsorted expected
sequence. Keep the existing None result when expected is empty, ensuring this
digest uses the same canonical ordering as the other digest producers and
consumers.
- Around line 2645-2652: Update _seed_shard_command so each shard uses unique
JUnit and JSON report paths instead of inheriting the shared collection report
arguments. Derive per-shard artifact paths under the run directory using the
shard identity, while preserving the existing testmon and explicit nodeid
arguments so aggregate seed reporting can read every shard’s results.
- Around line 2812-2824: The teardown handling in devtools/verify.py (lines
2812-2824) must only treat a recorded passed result as terminal when
use_database_fallback is enabled; keep recorded failures ungated so they still
block release. Add a test in tests/unit/devtools/test_verify.py (lines 461-489)
using a passed database node outcome with use_database_fallback=False and assert
the result remains missing.
- Around line 3416-3441: Update the seed-shard execution around
_seed_shard_command so each shard invokes pytest with only its shard nodeids as
collection targets, rather than retaining root arguments that trigger
full-corpus collection. Preserve the existing marker expression and shard
execution behavior, then measure and compare full --seed-testmon wall-clock time
against the previous single-process seed to validate the improvement.
- Around line 2617-2631: Update _prepare_testmon_seed_shards and
_testmon_seed_can_resume to compare the freshly collected node-ID set with the
resumed seed’s expected node-ID set. When they differ, reject resume or
initialize a new seed plan from the fresh collection instead of reusing prior
shards, ensuring newly collected tests are seeded.
In `@tests/infra/convergence_harness.py`:
- Around line 237-246: Update the seeding flow around write_source_raw_session
to publish payload through ArchiveBlobPublisher before creating the raw session
reference. Pass the resulting publication receipt into write_source_raw_session
and consume that receipt within the source transaction, ensuring the blob
content exists before the raw_payload reference is recorded.
In `@tests/unit/devtools/test_pytest_progress_plugin.py`:
- Around line 80-94: Redirect all testmon and verification artifacts into
temporary test directories instead of the checkout. In
tests/unit/devtools/test_pytest_progress_plugin.py lines 80-94, set
TESTMON_DATAFILE in the child env to a tmp_path location and remove checkout
cache creation; at lines 34-43, remove the checkout-cache shutil.move while
retaining plugin-global restoration. In tests/unit/devtools/test_verify.py lines
126-140, remove _quarantine_checkout_testmon; in lines 325-337, 354, 422, and
493, call _isolate_verify_artifacts(tmp_path, monkeypatch) in each shard test so
TESTMON_DATA and TESTMON_SEED_ATTEMPT resolve under tmp_path.
- Around line 112-115: Update the test around the child-process environment
setup and event assertions to cover the configured
POLYLOGUE_PYTEST_SELECTION_PATH and POLYLOGUE_PYTEST_SUMMARY_PATH outputs by
asserting both files exist after the run; alternatively remove those environment
variables if the test is intentionally limited to event phases.
In `@tests/unit/devtools/test_verify.py`:
- Around line 1240-1269: Remove the duplicate
test_seed_node_outcomes_keep_unconfirmed_teardown_incomplete test. Add its
reason assertion to the existing
test_seed_outcome_does_not_infer_call_success_from_teardown, preserving the
shared missing outcome and teardown event coverage in that single test.
- Around line 306-311: Replace the tautological indexed assertion in the test
around the pytest command checks with a direct membership assertion for
"--ignore=tests/benchmarks", matching the existing "--collect-only" and
"--testmon" assertions; leave the separate "-n" value validation unchanged.
- Around line 116-117: Update the _quarantine_checkout_testmon fixture
annotation from object to Iterator[None], importing Iterator from
collections.abc if it is not already available, so the yield-based session
fixture has a generator-compatible return type.
---
Outside diff comments:
In `@devtools/verify.py`:
- Around line 1980-2000: Register the unmarked benchmark files
tests/benchmarks/test_schema_linear_scaling.py and
tests/benchmarks/test_fts_trigger_amPLIFICATION.py as devtools bench campaign
targets, and add the pytest benchmark marker to their tests. Keep them excluded
from correctness lanes through the existing benchmark filtering.
In `@tests/unit/devtools/test_pytest_progress_plugin.py`:
- Around line 34-43: Update the fixture cleanup around pytest_progress_plugin
global restoration so _SELECTED_COUNT, _DESELECTED_COUNT,
_DESELECTED_NODEIDS_SAMPLE, _SLOWEST_REPORTS, _COLLECTION_STARTED_AT, and
_COLLECTION_DURATION_S are restored before calling shutil.move on
checkout_cache, or guarantee restoration with a finally block if the move fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8221a540-4074-427f-b34b-4985bbf3e7e5
📒 Files selected for processing (9)
devtools/testmon_state.pydevtools/verify.pypolylogue/daemon/fts_startup.pytests/conftest.pytests/infra/convergence_harness.pytests/unit/annotations/test_durable_storage.pytests/unit/annotations/test_importer.pytests/unit/devtools/test_pytest_progress_plugin.pytests/unit/devtools/test_verify.py
💤 Files with no reviewable changes (2)
- tests/unit/annotations/test_durable_storage.py
- tests/conftest.py
| def _prepare_testmon_seed_shards( | ||
| prepared: Mapping[str, Any], | ||
| *, | ||
| selection: Mapping[str, Any] | None, | ||
| ) -> dict[str, Any]: | ||
| """Persist the full planned corpus before the first testmon DB mutation.""" | ||
| expected = sorted(_testmon_seed_expected_nodeids(prepared)) if prepared.get("resume") else [] | ||
| if not expected: | ||
| expected = _seed_selection_nodeids(selection or {}) or [] | ||
| prior_shards = validate_seed_shard_ledger(prepared.get("shards"), expected_nodeids=expected) | ||
| shards = ( | ||
| prior_shards | ||
| if prior_shards is not None | ||
| else (seed_shard_plan(expected, shard_size=TESTMON_SEED_SHARD_SIZE) if expected else []) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the resume contract to see whether it constrains the collected corpus.
set -euo pipefail
ast-grep run --pattern $'def _testmon_seed_resume_contract($$$) {
$$$
}' --lang python devtools/verify.py || rg -nP -A25 'def _testmon_seed_resume_contract' devtools/verify.py
rg -nP -B3 -A12 'def _testmon_seed_can_resume' devtools/verify.pyRepository: Sinity/polylogue
Length of output: 2254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2520,2665p' devtools/verify.py
sed -n '2880,3050p' devtools/verify.py
sed -n '3360,3470p' devtools/verify.py
rg -n -P -B5 -A15 '_prepare_testmon_seed_shards|_testmon_seed_can_resume|_testmon_seed_resume_contract|_seed_selection_nodeids|_testmon_seed_expected_nodeids|selection' devtools/verify.pyRepository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -P -B12 -A24 'prepared_seed_attempt|_prepare_testmon_seed_attempt|seed-testmon collect|_testmon_seed_can_resume|_testmon_seed_resume_contract|git_tree.*worktree_fingerprint|worktree_fingerprint.*git_tree' devtools/verify.py
rg -n -P -B8 -A20 'resume.*seed|seed.*resume|expected_nodeids|selected_nodeids' tests devtools 2>/dev/null | head -n 320Repository: Sinity/polylogue
Length of output: 37756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2400,2488p' devtools/verify.py
sed -n '1880,2010p' devtools/verify.py
sed -n '130,180p' devtools/pytest_progress_plugin.py
sed -n '1,130p' tests/integration/devtools/test_testmon_seed_recovery.py
rg -n -P -B5 -A18 'def _testmon_seed_identity|def _worktree_fingerprint|dependency_environment|pytest_harness|git_tree' devtools/verify.pyRepository: Sinity/polylogue
Length of output: 24260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from hashlib import sha256
def seed_selection_nodeids(selection):
nodeids = selection.get("selected_nodeids")
count = selection.get("selected_count")
omitted = selection.get("selected_nodeids_omitted")
if (
not isinstance(nodeids, list)
or not nodeids
or any(not isinstance(nodeid, str) or not nodeid for nodeid in nodeids)
or len(set(nodeids)) != len(nodeids)
or not isinstance(count, int)
or count != len(nodeids)
or omitted != 0
):
return None
return sorted(nodeids)
def prepare_expected(prepared, selection):
expected = (
sorted(prepared["expected_nodeids"])
if prepared.get("resume")
else []
)
if not expected:
expected = seed_selection_nodeids(selection or {}) or []
return expected
prior = ["tests/test_a.py::test_old"]
fresh = {
"selected_nodeids": ["tests/test_a.py::test_old", "tests/test_b.py::test_new"],
"selected_count": 2,
"selected_nodeids_omitted": 0,
}
result = prepare_expected(
{"resume": True, "expected_nodeids": prior},
fresh,
)
assert result == prior
assert set(result) != set(fresh["selected_nodeids"])
print({
"prior_expected": prior,
"fresh_selected": fresh["selected_nodeids"],
"resumed_expected": result,
"fresh_selection_ignored": True,
})
PYRepository: Sinity/polylogue
Length of output: 366
Reject resume when the collected node IDs differ. On resume, _prepare_testmon_seed_shards always uses the prior expected_nodeids and ignores the fresh collection. _testmon_seed_can_resume does not compare collection contents. New tests can therefore remain unseeded. Compare both node-ID sets and start a new seed or fail on mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devtools/verify.py` around lines 2617 - 2631, Update
_prepare_testmon_seed_shards and _testmon_seed_can_resume to compare the freshly
collected node-ID set with the resumed seed’s expected node-ID set. When they
differ, reject resume or initialize a new seed plan from the fresh collection
instead of reusing prior shards, ensuring newly collected tests are seeded.
| **dict(prepared), | ||
| "expected_nodeids": expected, | ||
| "expected_count": len(expected), | ||
| "expected_digest": hashlib.sha256("\n".join(expected).encode()).hexdigest() if expected else None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use sorted(expected) for the digest to match every other digest site.
Line 2636 hashes "\n".join(expected). Every other producer and consumer hashes "\n".join(sorted(expected)): line 2582, lines 3034 and 3077, _testmon_seed_expected_nodeids at line 2498, and stamp_from_attempt in devtools/testmon_state.py at line 993.
The values agree today only because expected is already sorted at lines 2623 and 2625. If either source stops sorting, the digest silently mismatches and _testmon_seed_expected_nodeids discards the whole ledger, which kills resume. Make the expression canonical here.
🐛 Proposed fix
- "expected_digest": hashlib.sha256("\n".join(expected).encode()).hexdigest() if expected else None,
+ "expected_digest": hashlib.sha256("\n".join(sorted(expected)).encode()).hexdigest() if expected else None,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "expected_digest": hashlib.sha256("\n".join(expected).encode()).hexdigest() if expected else None, | |
| "expected_digest": hashlib.sha256("\n".join(sorted(expected)).encode()).hexdigest() if expected else None, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devtools/verify.py` at line 2636, Update the expected_digest expression in
the relevant verification flow to hash "\n".join(sorted(expected)) instead of
the unsorted expected sequence. Keep the existing None result when expected is
empty, ensuring this digest uses the same canonical ordering as the other digest
producers and consumers.
| def _seed_shard_command(collection_command: Sequence[str], shard: Mapping[str, Any]) -> list[str]: | ||
| """Build a serial, explicit-node pytest-testmon invocation for one shard.""" | ||
| nodeids = shard.get("nodeids") | ||
| if not isinstance(nodeids, list) or not nodeids: | ||
| raise ValueError("testmon seed shard is missing nodeids") | ||
| command = [argument for argument in collection_command if argument != "--collect-only"] | ||
| command.extend(["--testmon", "--testmon-noselect", *nodeids]) | ||
| return command |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Each shard overwrites the same JSON and JUnit report files.
_seed_shard_command copies the collection command and removes only --collect-only. The command still contains --junitxml={PYTEST_JUNIT_REPORT_DIR}/verify-latest.xml and --json-report-file={PYTEST_REPORT_PATH} from lines 1986-1989.
Every shard therefore writes the same two files. After a sharded seed only the last shard's report remains. _read_pytest_report and _compare_against_last then describe one shard instead of the whole seed, so test counts and duration comparisons are wrong for the seed tier.
Redirect both report paths per shard, or drop them from the shard command and rely on the per-shard artifacts under the run directory.
🐛 Proposed fix: per-shard report paths
-def _seed_shard_command(collection_command: Sequence[str], shard: Mapping[str, Any]) -> list[str]:
+def _seed_shard_command(collection_command: Sequence[str], shard: Mapping[str, Any]) -> list[str]:
"""Build a serial, explicit-node pytest-testmon invocation for one shard."""
nodeids = shard.get("nodeids")
if not isinstance(nodeids, list) or not nodeids:
raise ValueError("testmon seed shard is missing nodeids")
- command = [argument for argument in collection_command if argument != "--collect-only"]
+ index = shard.get("index")
+
+ def _shard_report_arg(argument: str) -> str:
+ if argument.startswith("--junitxml="):
+ return f"--junitxml={PYTEST_JUNIT_REPORT_DIR}/verify-latest-shard-{index}.xml"
+ if argument.startswith("--json-report-file="):
+ return f"--json-report-file={PYTEST_REPORT_DIR / f'last-pytest-shard-{index}.json'}"
+ return argument
+
+ command = [_shard_report_arg(argument) for argument in collection_command if argument != "--collect-only"]
command.extend(["--testmon", "--testmon-noselect", *nodeids])
return command🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devtools/verify.py` around lines 2645 - 2652, Update _seed_shard_command so
each shard uses unique JUnit and JSON report paths instead of inheriting the
shared collection report arguments. Derive per-shard artifact paths under the
run directory using the shard identity, while preserving the existing testmon
and explicit nodeid arguments so aggregate seed reporting can read every shard’s
results.
| terminal = all(item.get("outcome") in {"passed", "failed", "error", "skipped"} for item in outcomes) | ||
| selection_matches = selected == nodeids | ||
| shard.update( | ||
| { | ||
| "status": SeedShardStatus.COMPLETE.value | ||
| if selection_matches and terminal | ||
| else SeedShardStatus.INCOMPLETE.value, | ||
| "started_at": shard.get("started_at") or datetime.now(timezone.utc).isoformat(), | ||
| "finished_at": datetime.now(timezone.utc).isoformat(), | ||
| "exit_code": step.get("exit"), | ||
| "artifact_dir": step.get("artifact_dir"), | ||
| "selection": dict(selection) if isinstance(selection, Mapping) else None, | ||
| "database": database, | ||
| "node_outcomes": outcomes, | ||
| "pytest_step": dict(step), | ||
| } | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A shard that pytest partially deselects can never reach complete.
Line 2699 requires the shard's own selection.json node set to equal the shard node set exactly. Line 2702 marks the shard complete only when that holds and every node has a terminal outcome.
If pytest deselects any node inside a shard — a collection-time skip, a plugin-level deselect, or a nodeid that no longer resolves — selection_matches is False forever. The shard stays incomplete, seed_shard_ledger_is_terminal stays False, stamp_from_attempt returns None, and every later --seed-testmon run re-executes the same shard with the same result. The receipt records status: "incomplete" without naming the mismatch, so the loop is not diagnosable from the ledger.
Record the mismatch explicitly so an operator can see why the shard cannot complete.
🐛 Proposed fix: persist the selection mismatch
terminal = all(item.get("outcome") in {"passed", "failed", "error", "skipped"} for item in outcomes)
selection_matches = selected == nodeids
shard.update(
{
"status": SeedShardStatus.COMPLETE.value
if selection_matches and terminal
else SeedShardStatus.INCOMPLETE.value,
+ "selection_matches": selection_matches,
+ "unselected_nodeids": sorted(set(nodeids) - set(selected or [])),
"started_at": shard.get("started_at") or datetime.now(timezone.utc).isoformat(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| terminal = all(item.get("outcome") in {"passed", "failed", "error", "skipped"} for item in outcomes) | |
| selection_matches = selected == nodeids | |
| shard.update( | |
| { | |
| "status": SeedShardStatus.COMPLETE.value | |
| if selection_matches and terminal | |
| else SeedShardStatus.INCOMPLETE.value, | |
| "started_at": shard.get("started_at") or datetime.now(timezone.utc).isoformat(), | |
| "finished_at": datetime.now(timezone.utc).isoformat(), | |
| "exit_code": step.get("exit"), | |
| "artifact_dir": step.get("artifact_dir"), | |
| "selection": dict(selection) if isinstance(selection, Mapping) else None, | |
| "database": database, | |
| "node_outcomes": outcomes, | |
| "pytest_step": dict(step), | |
| } | |
| ) | |
| terminal = all(item.get("outcome") in {"passed", "failed", "error", "skipped"} for item in outcomes) | |
| selection_matches = selected == nodeids | |
| shard.update( | |
| { | |
| "status": SeedShardStatus.COMPLETE.value | |
| if selection_matches and terminal | |
| else SeedShardStatus.INCOMPLETE.value, | |
| "selection_matches": selection_matches, | |
| "unselected_nodeids": sorted(set(nodeids) - set(selected or [])), | |
| "started_at": shard.get("started_at") or datetime.now(timezone.utc).isoformat(), | |
| "finished_at": datetime.now(timezone.utc).isoformat(), | |
| "exit_code": step.get("exit"), | |
| "artifact_dir": step.get("artifact_dir"), | |
| "selection": dict(selection) if isinstance(selection, Mapping) else None, | |
| "database": database, | |
| "node_outcomes": outcomes, | |
| "pytest_step": dict(step), | |
| } | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devtools/verify.py` around lines 2698 - 2714, Update the shard status
handling around `selection_matches` and the `shard.update` payload to persist an
explicit selection-mismatch diagnostic when pytest’s selected nodeids differ
from `nodeids`. Record the expected and actual node sets (or an equivalent
structured mismatch field) in the shard receipt while preserving the existing
`COMPLETE`/`INCOMPLETE` determination and outcome tracking.
| elif nodeid in finished and any( | ||
| report.get("when") == "teardown" and report.get("outcome") == "passed" for report in node_reports | ||
| ): | ||
| # Teardown describes fixture cleanup, not the test body. It may | ||
| # corroborate a terminal testmon row, but it cannot replace a | ||
| # missing call report: a failed call can still end with a passing | ||
| # teardown, and an unrecorded call must remain resumable. | ||
| if recorded.get(nodeid) == "passed": | ||
| outcome, reason = "passed", "passing teardown corroborated by testmon success" | ||
| elif recorded.get(nodeid) == "failed": | ||
| outcome, reason = "failed", "passing teardown contradicted by testmon failure" | ||
| else: | ||
| outcome, reason = "missing", "passing teardown without call report or testmon result" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The teardown arm treats a testmon database row as terminal evidence, and no test pins that arm under the shard flow's flag. The shared root cause is that the new branch reads recorded without checking use_database_fallback, so the shard path — which passes use_database_fallback=False at lines 2696 and 2969 specifically to distrust the database — can still classify a node passed from a row the current shard did not produce.
devtools/verify.py#L2812-L2824: guard thepassedarm withuse_database_fallback, and leave thefailedarm ungated so a recorded failure always blocks release.tests/unit/devtools/test_verify.py#L461-L489: add a case withdatabase={"node_outcomes": {nodeid: "passed"}}anduse_database_fallback=False, asserting the outcome staysmissing.
📍 Affects 2 files
devtools/verify.py#L2812-L2824(this comment)tests/unit/devtools/test_verify.py#L461-L489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devtools/verify.py` around lines 2812 - 2824, The teardown handling in
devtools/verify.py (lines 2812-2824) must only treat a recorded passed result as
terminal when use_database_fallback is enabled; keep recorded failures ungated
so they still block release. Add a test in tests/unit/devtools/test_verify.py
(lines 461-489) using a passed database node outcome with
use_database_fallback=False and assert the result remains missing.
| checkout_root = Path(__file__).resolve().parents[3] | ||
| # The real testmon plugin receives no TESTMON_DATAFILE here by design: | ||
| # this regression test models a child process after the host scrub. Give | ||
| # its default relative path a parent directory without permitting the | ||
| # resulting cache to leak into later tests. | ||
| (checkout_root / ".cache" / "testmon").mkdir(parents=True, exist_ok=True) | ||
| env = os.environ.copy() | ||
| env.update( | ||
| { | ||
| "POLYLOGUE_PYTEST_EVENTS_DIR": str(events_dir), | ||
| "POLYLOGUE_PYTEST_SELECTION_PATH": str(tmp_path / "selection.json"), | ||
| "POLYLOGUE_PYTEST_SUMMARY_PATH": str(tmp_path / "summary.json"), | ||
| "POLYLOGUE_VERIFY_RUN_ID": "subprocess-regression", | ||
| # This child deliberately owns a private destination so the test | ||
| # can verify the nested-process isolation contract without making | ||
| # its reports part of the outer seed ledger. | ||
| "POLYLOGUE_PYTEST_NESTED_PRIVATE": "1", | ||
| } | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unit tests write testmon state into the real checkout, then relocate it during teardown. The shared root cause is that these tests let production code resolve testmon paths against ROOT instead of redirecting those paths to tmp_path. Each site then compensates with a shutil.move of the developer's .cache/testmon, so an interrupted session can leave the checkout without its seed database.
tests/unit/devtools/test_pytest_progress_plugin.py#L80-L94: setTESTMON_DATAFILEto a path undertmp_pathin the childenvand delete themkdirofcheckout_root / ".cache" / "testmon".tests/unit/devtools/test_pytest_progress_plugin.py#L34-L43: delete theshutil.moveof the checkout cache once the child writes intotmp_path, and keep only the plugin-global restoration.tests/unit/devtools/test_verify.py#L126-L140: delete the_quarantine_checkout_testmonsession fixture once no test writes checkout-level state.tests/unit/devtools/test_verify.py#L325-L337: call_isolate_verify_artifacts(tmp_path, monkeypatch)in each new shard test soTESTMON_DATAandTESTMON_SEED_ATTEMPTresolve undertmp_path; apply the same change at lines 354, 422, and 493.
📍 Affects 2 files
tests/unit/devtools/test_pytest_progress_plugin.py#L80-L94(this comment)tests/unit/devtools/test_pytest_progress_plugin.py#L34-L43tests/unit/devtools/test_verify.py#L126-L140tests/unit/devtools/test_verify.py#L325-L337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/devtools/test_pytest_progress_plugin.py` around lines 80 - 94,
Redirect all testmon and verification artifacts into temporary test directories
instead of the checkout. In tests/unit/devtools/test_pytest_progress_plugin.py
lines 80-94, set TESTMON_DATAFILE in the child env to a tmp_path location and
remove checkout cache creation; at lines 34-43, remove the checkout-cache
shutil.move while retaining plugin-global restoration. In
tests/unit/devtools/test_verify.py lines 126-140, remove
_quarantine_checkout_testmon; in lines 325-337, 354, 422, and 493, call
_isolate_verify_artifacts(tmp_path, monkeypatch) in each shard test so
TESTMON_DATA and TESTMON_SEED_ATTEMPT resolve under tmp_path.
| assert result.returncode == 0, result.stdout + result.stderr | ||
| assert selection_path.read_text(encoding="utf-8") == "selection-sentinel\n" | ||
| assert summary_path.read_text(encoding="utf-8") == "summary-sentinel\n" | ||
| events = [json.loads(line) for path in events_dir.glob("*.jsonl") for line in path.read_text().splitlines()] | ||
| reports = [event for event in events if event.get("event") == "test_report"] | ||
| assert len(reports) == 3 | ||
| assert {(event["nodeid"], event["when"], event["outcome"], event["run_id"]) for event in reports} == { | ||
| ( | ||
| "tests/unit/core/test_identity_law.py::test_session_id_is_origin_native_id", | ||
| phase, | ||
| "passed", | ||
| "subprocess-regression", | ||
| ) | ||
| for phase in ("setup", "call", "teardown") | ||
| } | ||
| assert {event["when"] for event in reports} == {"setup", "call", "teardown"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The selection and summary paths are configured but never asserted.
Lines 90-91 set POLYLOGUE_PYTEST_SELECTION_PATH and POLYLOGUE_PYTEST_SUMMARY_PATH for the child. The assertions at lines 112-115 check only the return code and the event report phases.
Either assert that both files exist after the run, or remove the two variables so the setup states what the test verifies.
♻️ Proposed addition
assert result.returncode == 0, result.stdout + result.stderr
+ assert (tmp_path / "selection.json").is_file()
+ assert (tmp_path / "summary.json").is_file()
events = [json.loads(line) for path in events_dir.glob("*.jsonl") for line in path.read_text().splitlines()]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert result.returncode == 0, result.stdout + result.stderr | |
| assert selection_path.read_text(encoding="utf-8") == "selection-sentinel\n" | |
| assert summary_path.read_text(encoding="utf-8") == "summary-sentinel\n" | |
| events = [json.loads(line) for path in events_dir.glob("*.jsonl") for line in path.read_text().splitlines()] | |
| reports = [event for event in events if event.get("event") == "test_report"] | |
| assert len(reports) == 3 | |
| assert {(event["nodeid"], event["when"], event["outcome"], event["run_id"]) for event in reports} == { | |
| ( | |
| "tests/unit/core/test_identity_law.py::test_session_id_is_origin_native_id", | |
| phase, | |
| "passed", | |
| "subprocess-regression", | |
| ) | |
| for phase in ("setup", "call", "teardown") | |
| } | |
| assert {event["when"] for event in reports} == {"setup", "call", "teardown"} | |
| assert result.returncode == 0, result.stdout + result.stderr | |
| assert (tmp_path / "selection.json").is_file() | |
| assert (tmp_path / "summary.json").is_file() | |
| events = [json.loads(line) for path in events_dir.glob("*.jsonl") for line in path.read_text().splitlines()] | |
| reports = [event for event in events if event.get("event") == "test_report"] | |
| assert {event["when"] for event in reports} == {"setup", "call", "teardown"} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/devtools/test_pytest_progress_plugin.py` around lines 112 - 115,
Update the test around the child-process environment setup and event assertions
to cover the configured POLYLOGUE_PYTEST_SELECTION_PATH and
POLYLOGUE_PYTEST_SUMMARY_PATH outputs by asserting both files exist after the
run; alternatively remove those environment variables if the test is
intentionally limited to event phases.
| @pytest.fixture(scope="session", autouse=True) | ||
| def _quarantine_checkout_testmon(tmp_path_factory: pytest.TempPathFactory) -> object: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the Iterator import and check how other generator fixtures are annotated.
set -euo pipefail
rg -nP '^(from|import)\s' tests/unit/devtools/test_verify.py | head -40
rg -nP -B2 -A2 'def .*\) -> Iterator\[None\]' tests/unit/devtools/ | head -40
rg -nP 'warn_return_any|disallow_untyped_defs|strict\s*=' pyproject.tomlRepository: Sinity/polylogue
Length of output: 1346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fixture and imports ---'
sed -n '1,145p' tests/unit/devtools/test_verify.py
printf '%s\n' '--- mypy configuration ---'
sed -n '205,238p' pyproject.toml
printf '%s\n' '--- verify-step references ---'
rg -n -A8 -B8 'build_verify_steps|mypy' devtools tests/unit/devtools pyproject.tomlRepository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mypy availability and generator diagnostic ---'
if command -v mypy >/dev/null 2>&1; then
mypy --version
cat <<'PY' | mypy --show-error-codes --no-error-summary /dev/stdin
from collections.abc import Iterator
def as_object() -> object:
yield None
def as_iterator() -> Iterator[None]:
yield None
PY
else
printf '%s\n' 'mypy is not available'
fi
printf '%s\n' '--- sibling fixture imports and implementation ---'
sed -n '1,28p' tests/unit/devtools/test_pytest_progress_plugin.py
sed -n '108,145p' tests/unit/devtools/test_verify.pyRepository: Sinity/polylogue
Length of output: 2767
🌐 Web query:
mypy error generator function return type should be Generator or one of its supertypes object annotation
💡 Result:
This mypy error occurs because a generator function is a function that returns a generator object, and mypy expects the return type annotation to reflect this specific type or its supertypes [1][2]. To resolve this error, you should use the Generator type from collections.abc (or typing in older versions) [3][4][5]. The Generator type is a generic that takes three type arguments: Generator[YieldType, SendType, ReturnType] [1][3][5]. For a simple generator that only yields values, you can set the SendType and ReturnType to None [1][6]: from collections.abc import Generator def my_generator -> Generator[int, None, None]: yield 1 yield 2 If you do not need to support send(), throw(), or close() methods explicitly in your type interface, you may also use Iterator[YieldType] or Iterable[YieldType] as these are valid supertypes that mypy accepts [1][3][5]. However, Generator is the most precise type for functions containing a yield statement [1]. Key guidelines for choosing the correct annotation: - Use Generator[YieldType, None, None] (or simply Generator[YieldType]) if your function only yields values and does not utilize send() or return a value [1][5]. - Use the full Generator[YieldType, SendType, ReturnType] signature if your generator receives values via send() or returns a final value using the return statement [1][5]. - Use Iterator[YieldType] if you want a more general type that does not expose generator-specific methods like send() [1][6]. If you receive this error even when using Generator, ensure you are not accidentally returning an incompatible type, such as a different iterator or a generator expression that mypy is failing to infer correctly [7]. In such cases, explicitly annotating the return type as Generator[...] will typically resolve the conflict [7][8].
Citations:
- 1: https://mypy.readthedocs.io/en/stable/kinds_of_types.html
- 2: https://www.linuxjournal.com/content/pythons-mypy-callables-and-generators
- 3: https://stackoverflow.com/questions/38419654/proper-type-annotation-of-python-functions-with-yield
- 4: https://stackoverflow.com/questions/43658999/what-is-the-return-type-hint-of-a-generator-function
- 5: https://docs.python.org/3/library/typing.html
- 6: https://stackoverflow.com/questions/68047395/how-to-determine-the-yield-send-return-values-for-any-generator-in-python
- 7: https://stackoverflow.com/questions/50273130/mypy-error-when-returning-a-generator-expression
- 8: Incorrect "incompatible type" error for context manager typed with AbstractContextManager python/mypy#16260
Annotate the generator fixture as Iterator[None].
_quarantine_checkout_testmon contains yield. Strict mypy requires a generator-compatible return type. Add from collections.abc import Iterator if needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/devtools/test_verify.py` around lines 116 - 117, Update the
_quarantine_checkout_testmon fixture annotation from object to Iterator[None],
importing Iterator from collections.abc if it is not already available, so the
yield-based session fixture has a generator-compatible return type.
| assert label == "pytest seed-testmon collect" | ||
| assert "--collect-only" in command | ||
| assert command[command.index("--ignore=tests/benchmarks")] == "--ignore=tests/benchmarks" | ||
| assert "--testmon" not in command | ||
| assert "-n" in command | ||
| assert command[command.index("-n") + 1] == "8" | ||
| assert command[command.index("-n") + 1] == "0" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 308 is a tautology; assert membership directly.
command[command.index(x)] == x holds for every present x and raises ValueError instead of an assertion failure when x is absent. Use in, which matches the surrounding style at lines 307 and 309 and produces a readable failure.
💚 Proposed fix
assert label == "pytest seed-testmon collect"
assert "--collect-only" in command
- assert command[command.index("--ignore=tests/benchmarks")] == "--ignore=tests/benchmarks"
+ assert "--ignore=tests/benchmarks" in command
assert "--testmon" not in command📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert label == "pytest seed-testmon collect" | |
| assert "--collect-only" in command | |
| assert command[command.index("--ignore=tests/benchmarks")] == "--ignore=tests/benchmarks" | |
| assert "--testmon" not in command | |
| assert "-n" in command | |
| assert command[command.index("-n") + 1] == "8" | |
| assert command[command.index("-n") + 1] == "0" | |
| assert label == "pytest seed-testmon collect" | |
| assert "--collect-only" in command | |
| assert "--ignore=tests/benchmarks" in command | |
| assert "--testmon" not in command | |
| assert "-n" in command | |
| assert command[command.index("-n") + 1] == "0" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/devtools/test_verify.py` around lines 306 - 311, Replace the
tautological indexed assertion in the test around the pytest command checks with
a direct membership assertion for "--ignore=tests/benchmarks", matching the
existing "--collect-only" and "--testmon" assertions; leave the separate "-n"
value validation unchanged.
| def test_seed_node_outcomes_keep_unconfirmed_teardown_incomplete(tmp_path: Path) -> None: | ||
| """A terminal teardown does not prove that the missing call phase passed.""" | ||
| events = tmp_path / "events.jsonl" | ||
| events.write_text( | ||
| "\n".join( | ||
| [ | ||
| json.dumps({"event": "test_started", "nodeid": "tests/test_a.py::test_finished"}), | ||
| json.dumps( | ||
| { | ||
| "event": "test_report", | ||
| "nodeid": "tests/test_a.py::test_finished", | ||
| "when": "teardown", | ||
| "outcome": "passed", | ||
| } | ||
| ), | ||
| json.dumps({"event": "test_finished", "nodeid": "tests/test_a.py::test_finished"}), | ||
| ] | ||
| ) | ||
| + "\n" | ||
| ) | ||
|
|
||
| outcomes = _seed_node_outcomes_from_events( | ||
| events, | ||
| expected_nodeids=["tests/test_a.py::test_finished"], | ||
| database={"node_outcomes": {"tests/test_a.py::test_finished": "missing"}}, | ||
| pytest_step={"diagnosis": "pytest_failed"}, | ||
| ) | ||
|
|
||
| assert outcomes[0]["outcome"] == "missing" | ||
| assert outcomes[0]["reason"] == "passing teardown without call report or testmon result" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This test duplicates the first case of test_seed_outcome_does_not_infer_call_success_from_teardown.
Lines 1242-1268 build the same event shape as lines 465-481 — test_started, a passing teardown report, test_finished — and assert the same missing outcome. The only new assertion is the reason string.
Fold the reason assertion into the existing test at lines 461-489 and delete this one, so one test owns the teardown arm.
🧰 Tools
🪛 ast-grep (0.45.1)
[info] 1245-1245: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"event": "test_started", "nodeid": "tests/test_a.py::test_finished"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1246-1253: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"event": "test_report",
"nodeid": "tests/test_a.py::test_finished",
"when": "teardown",
"outcome": "passed",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1254-1254: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"event": "test_finished", "nodeid": "tests/test_a.py::test_finished"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/devtools/test_verify.py` around lines 1240 - 1269, Remove the
duplicate test_seed_node_outcomes_keep_unconfirmed_teardown_incomplete test. Add
its reason assertion to the existing
test_seed_outcome_does_not_infer_call_success_from_teardown, preserving the
shared missing outcome and teardown event coverage in that single test.
1 participant
Summary
Record complete testmon seed outcomes without treating fixture teardown as proof that the test body passed.
Problem
Interrupted or worker-disrupted shards can retain
test_finishedplus a passing teardown report without a call-phase report. A failed test can therefore be incorrectly terminalized and skipped on resume; an unrecorded call can be treated as passed.Solution
Verification
.venv/bin/python -m devtools test tests/unit/devtools/test_verify.py tests/unit/devtools/test_testmon_state.py tests/unit/devtools/test_checkout_guard.py tests/unit/devtools/test_pytest_progress_plugin.py— 196 passed;.venv/bin/python -m devtools verify --quick— 25/25 steps passed ateda8751e3;This PR is self-contained on top of the already-merged testmon sharding work in #3956.
Summary by CodeRabbit
New Features
Bug Fixes