fix(devtools): shard resumable testmon seeding - #3956
Conversation
Problem: the 21k-node seed wrote to one testmon database through xdist workers and hit the bounded pytest timeout before it could publish a complete baseline. What changed: collect the full selected node set first, partition it deterministically, and execute each shard serially with one testmon writer. Persist shard-level node outcomes after every shard and resume only unfinished shards. Release publication now requires a terminal shard ledger as well as complete, failure-free graph coverage.
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesTestmon verification
FTS freshness convergence
Annotation fixture corrections
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ConvergencePipeline
participant InsightMaterialization
participant FTSFreshnessStage
ConvergencePipeline->>InsightMaterialization: materialize insights
ConvergencePipeline->>FTSFreshnessStage: execute fts_freshness stage
FTSFreshnessStage->>ConvergencePipeline: record archive-wide freshness snapshot
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@codex review Please review exact head |
![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
Lines 2809 to 2812 in 136c2ef
Do not use stale database rows as shard outcomes
When a shard is interrupted after collection but before one or more selected nodes start, seed mode may already have successful rows for those nodes because it updates an existing testmon database. This fallback labels those unexecuted nodes as passed; _checkpoint_testmon_seed_shard can consequently persist the shard as complete, and an interrupted outer verify followed by resume skips it and may publish a green seed without those nodes running. The pre-sharding finalizer deliberately disabled this fallback for exactly this case, so shard completion should likewise require current-shard events rather than pre-existing SQLite results.
ℹ️ 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".
| expected = _testmon_seed_expected_nodeids(prepared) if prepared.get("resume") else [] | ||
| if not expected: | ||
| expected = _seed_selection_nodeids(selection or {}) or [] |
There was a problem hiding this comment.
On a resumed seed, a nonempty prior ledger always wins and the collection produced immediately before this call is ignored. If collection changes without changing the resume identity—for example, environment- or external-data-dependent parametrization—the resumed run executes only the old shards and can publish a green seed while omitting nodes present in the current collection. Compare the fresh complete selection with the persisted expected set and invalidate or extend the shard plan when they differ.
Useful? React with 👍 / 👎.
|
@codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136c2ef18c
ℹ️ 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".
| # Collection produces the exact corpus contract before any testmon | ||
| # write. Shards below are generated from this ledger and run one | ||
| # at a time, so pytest-testmon has exactly one SQLite writer. | ||
| pytest_cmd.extend(["-m", base_marker, "--collect-only", "-n", "0"]) |
There was a problem hiding this comment.
The new collection ledger uses only the scale/slow expression, so devtools verify --seed-testmon selects every @pytest.mark.benchmark test and permanently includes those nodes in the shard plan. The repository config's --benchmark-disable disables measurement but does not deselect the benchmark body, while pytest --help confirms that -m is the option that limits tests by marker; therefore expensive benchmark campaigns under tests/benchmarks/ run as part of the correctness/testmon seed and can exhaust the bounded seed runtime. Add not benchmark to this marker expression while preserving the existing slow and scale clauses.
Useful? React with 👍 / 👎.
| outcomes = _seed_node_outcomes_from_events( | ||
| artifact_dir / "events.jsonl" if artifact_dir is not None else Path(".missing-testmon-events"), | ||
| expected_nodeids=nodeids, | ||
| database=database, | ||
| pytest_step=step, |
There was a problem hiding this comment.
When a shard times out or crashes after collection but before emitting node reports, this call leaves use_database_fallback=True, so pre-existing passing rows in testmondata classify every unexecuted node as passed. Because the selection ledger is written during collection, selection_matches is also true and the shard is checkpointed as complete; a resumed run then skips it and can publish a green current-tree seed without executing that shard. Disable database fallback for shard checkpoints, or otherwise require fresh terminal events before assigning COMPLETE.
Useful? React with 👍 / 👎.
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. 👍 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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2617-2642: The expected node IDs must be normalized before shard
validation and digest generation. In devtools/verify.py lines 2617-2642, update
_prepare_testmon_seed_shards to sort expected after fallback selection and hash
the normalized order. In tests/unit/devtools/test_verify.py lines 332-370, use
fixture node IDs in a non-sorted collection order and compute expected_digest
from sorted IDs so the resume round-trip verifies normalization.
In `@tests/unit/devtools/test_pytest_progress_plugin.py`:
- Around line 14-23: The autouse fixture _restore_plugin_state must preserve
supervisor-provided event destinations during pytest execution instead of
deleting them globally. Move environment clearing into the specific direct
plugin-call setup that needs test-owned paths, and retain managed values for the
lifecycle; add a regression test confirming a plugin test writes its node event
to the supervisor ledger.
In `@tests/unit/devtools/test_verify.py`:
- Around line 332-370: Update the resume test fixture to use node IDs whose
collection order differs from sorted order, while preserving the shard
assignment. Compute _atomic_payload["expected_digest"] from sorted(expected),
and update the selection.json and events.jsonl fixtures to reference the ordered
shard node (ordered[0]). Keep the existing completed-shard and resume assertions
unchanged.
🪄 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: 478134a7-9d12-456c-8476-7d883579eebe
📒 Files selected for processing (7)
devtools/checkout_guard.pydevtools/testmon_state.pydevtools/verify.pytests/conftest.pytests/unit/devtools/test_pytest_progress_plugin.pytests/unit/devtools/test_verify.pytests/unit/test_pytest_temp_policy.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 265cf41b09
ℹ️ 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".
| if shard_rc != 0 and exit_code == 0: | ||
| exit_code = shard_rc |
There was a problem hiding this comment.
When a shard hits the 45-minute runtime cap or the stall timeout, this loop records its nonzero exit but continues launching every remaining shard. Since _run applies the timeout independently to each invocation, a 64-shard seed can now consume up to 64 times the configured cap, and repeated resource failures are retried immediately instead of leaving the durable ledger for a later resume. Break after timeout, stall, interruption, or resource-preflight diagnoses while still allowing ordinary test failures to populate the remaining dependency graph.
Useful? React with 👍 / 👎.
| "POLYLOGUE_PYTEST_EVENTS_PATH", | ||
| "POLYLOGUE_PYTEST_SELECTION_PATH", | ||
| "POLYLOGUE_PYTEST_SUMMARY_PATH", | ||
| "POLYLOGUE_PYTEST_CONTAINMENT_PATH", |
There was a problem hiding this comment.
During a managed verify run, preserving POLYLOGUE_PYTEST_CONTAINMENT_PATH makes tests that call _run with a pytest... label—several tests in tests/unit/devtools/test_verify.py do this—start their nested supervisor against the outer shard's receipt. When the nested process marks that shared receipt finished, the outer _run_pytest_with_heartbeat loop reads it as its own supervisor having exited while output pipes remain open and can force-kill the still-running shard after the grace period. Keep the event-ledger variables needed by the progress plugin, but scrub or replace the containment path for code executed inside each test.
Useful? React with 👍 / 👎.
| _MANAGED_VERIFY_ENV = frozenset( | ||
| { | ||
| "POLYLOGUE_VERIFY_RUN_ID", | ||
| "POLYLOGUE_PYTEST_RUN_ID", |
There was a problem hiding this comment.
When a managed focused run and a test-invoked _run both select the same basetemp root, preserving POLYLOGUE_PYTEST_RUN_ID makes the nested run reuse the outer pytest session's basetemp name. The nested _run unconditionally calls cleanup_managed_pytest_basetemp on completion, so tests such as the real _run exercises in tests/unit/devtools/test_verify.py can delete the outer session's basetemp—including active tmp_path fixtures—while the shard is still executing. Scrub this control-plane ID in the per-test fixture so nested runs mint their own identifier; monkeypatch teardown can restore the outer ID before session finish.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e97c329a5e
ℹ️ 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".
| command = [argument for argument in collection_command if argument != "--collect-only"] | ||
| command.extend(["--testmon", "--testmon-noselect", *nodeids]) |
There was a problem hiding this comment.
For any seed containing multiple shards, this copies the collection command's fixed --junitxml=.cache/test-reports/verify-latest.xml and --json-report-file=.cache/verify/last-pytest.json arguments into every shard. _run clears those paths before each pytest invocation, so the final shard erases all earlier shard reports; if an earlier shard fails and a later one passes, evidence_dashboard._pytest_health() subsequently reports an ok result and counts only the last shard despite the seed failing overall. Checked pytest --help, which defines --junitxml=path as creating a report at the specified path. Give shards distinct report paths or merge their reports into the canonical artifacts.
Useful? React with 👍 / 👎.
| for shard in shards: | ||
| if shard.get("status") == SeedShardStatus.COMPLETE.value: | ||
| continue |
There was a problem hiding this comment.
When all tests emit terminal reports but testmon leaves a missing or orphaned dependency edge, _checkpoint_testmon_seed_shard() still marks each shard complete, while finalization correctly leaves the overall attempt incomplete. The next matching invocation is therefore considered resumable but skips every shard here, performs no database repair, and finalizes the same incomplete graph again; subsequent resumes repeat forever. Only skip a completed shard after confirming its recorded graph remains complete, or invalidate the completed statuses when final graph validation fails.
Useful? React with 👍 / 👎.
Problem: the fresh convergence-property seed exposed fixture failures before the intended laws could run. Raw and attachment references were admitted without published bytes, append fixtures required an inapplicable FTS mutation, and readiness fixtures lacked parser-authority receipts. What changed: publish fixture raw and attachment blobs through the archive publisher, record complete parser census evidence for admitted raws, refresh the FTS freshness ledger after both stages, and preserve the raw-replay mutation's intended comparator failure when raw acquisition is bypassed. Compatibility: test infrastructure only; production convergence behavior is unchanged.
Keep cold annotation reads on the default read-only archive handle and reopen a writable handle for retries. Derive delegation target and evidence identities from persisted action rows so the fixture follows the current message identity law.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
devtools/verify.py (1)
2623-2636: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize fresh selection node IDs before persistence.
_testmon_seed_expected_nodeidsvalidatesexpected_digestfromsorted(expected). Line 2636 hashes the unsorted collection order when this is a new seed. A non-lexical collection then fails validation on resume and cannot recover its persisted expected node IDs.
devtools/verify.py#L2623-L2636: sortexpectedafter the selection fallback and before shard validation and digest generation.tests/unit/devtools/test_verify.py#L310-L345: passpreparedto checkpointing, or assert its persisted expected node IDs and digest, so unsorted collection input verifies this contract.🤖 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 2623 - 2636, The fresh selection flow in devtools/verify.py lines 2623-2636 must sort expected after the selection fallback and before validate_seed_shard_ledger and digest generation, ensuring persisted node IDs and expected_digest use canonical lexical order. Update tests/unit/devtools/test_verify.py lines 310-345 to pass prepared to checkpointing or assert the persisted expected node IDs and digest using unsorted input.
🤖 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 `@tests/unit/devtools/test_pytest_progress_plugin.py`:
- Around line 103-105: Strengthen the assertions over the test_report events in
the test by requiring exactly three reports, all for the configured target
nodeid, with setup/call/teardown phases, expected passed outcomes, and run_id
equal to "subprocess-regression". Validate the complete payload identity rather
than only comparing the set of when values.
---
Duplicate comments:
In `@devtools/verify.py`:
- Around line 2623-2636: The fresh selection flow in devtools/verify.py lines
2623-2636 must sort expected after the selection fallback and before
validate_seed_shard_ledger and digest generation, ensuring persisted node IDs
and expected_digest use canonical lexical order. Update
tests/unit/devtools/test_verify.py lines 310-345 to pass prepared to
checkpointing or assert the persisted expected node IDs and digest using
unsorted input.
🪄 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: 4794ac48-eae7-4177-a5ff-594f0cc8eccb
📒 Files selected for processing (7)
devtools/verify.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 (1)
- tests/conftest.py
Problem: serial explicit-node shards replaced pytest-testmon's controller-owned xdist write path, stretching the seed into dozens of process startups. Restore one managed xdist pytest-testmon invocation, retain event-backed per-node outcomes and graph validation, and remove the shard-only receipt protocol.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f60850904
ℹ️ 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".
| "POLYLOGUE_PYTEST_SELECTION_PATH", | ||
| "POLYLOGUE_PYTEST_SUMMARY_PATH", |
There was a problem hiding this comment.
When a seed shard runs one of the load-sensitive supervisor tests on a host with user systemd, preserving this path lets tests/unit/devtools/test_pytest_supervisor.py::_xdist_controller_cmd() inherit the outer shard's selection destination through env = os.environ.copy(). That nested pytest invocation loads the progress plugin and overwrites selection.json with its single temporary node; _checkpoint_testmon_seed_shard() then sees a selection mismatch, marks the otherwise executed shard incomplete, and every resume repeats the same failure. Preserve the event stream if needed, but scrub or replace the selection and summary destinations for pytest processes launched by test code.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| from polylogue.daemon.fts_startup import record_fts_freshness_snapshot_sync | ||
|
|
||
| with sqlite3.connect(archive.root / "index.db") as conn: | ||
| record_fts_freshness_snapshot_sync(conn) |
There was a problem hiding this comment.
When the insights stage materializes new work-event rows after the FTS stage, this direct call repairs the freshness ledger inside the harness even though record_fts_freshness_snapshot_sync() has no production caller and is not one of the DaemonConverger stages. Consequently the derived-readiness property can remain green if the real post-ingest convergence route leaves session_work_events_fts stale or unreadable; wire the refresh into the production stage sequence, or assert the state left by that sequence without repairing it in the test harness.
AGENTS.md reference: AGENTS.md:L168-L172
Useful? React with 👍 / 👎.
| revision=RawRevisionEnvelope( | ||
| logical_source_key=str(make_session_id(session.source_name, session.provider_session_id)), | ||
| kind=RawRevisionKind.FULL, | ||
| source_revision=content_hash, |
There was a problem hiding this comment.
When these convergence fixtures exercise retained-raw replay or revision-governance paths, this records the normalized parsed-session content_hash as RawRevisionEnvelope.source_revision. Production admit_raw_observation() instead records the retained payload digest (blob_hash.hex()), and downstream retention and repair code treats a full revision as that blob identity; the synthetic source.db therefore contains authority evidence that cannot be produced by ingestion and can mask or create false reindex results. Use raw_blob_hash here and retain content_hash only for SessionWritePayload.
AGENTS.md reference: AGENTS.md:L138-L144
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 602474e034
ℹ️ 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".
| _TESTMON_STATE_MARKER = _TESTMON_STATE_DIR / "seed.json" | ||
| _TESTMON_SEED_ATTEMPT = _TESTMON_STATE_DIR / "seed-attempt.json" | ||
| _TESTMON_SEED_PROTOCOL_VERSION = 6 | ||
| _TESTMON_SEED_PROTOCOL_VERSION = 7 |
There was a problem hiding this comment.
In linked worktrees, devtools.verify still writes and bootstraps protocol-6 seed markers (devtools/verify.py:219), but this guard now validates them as protocol 7. Consequently, a freshly bootstrapped or completed seed is classified as invalid_testmon_seed, and subsequent devtools verify/devtools test invocations exit during the checkout guard. Bump the verifier's protocol constant in the same change, or leave this validator at 6.
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.
Because this argument is in the common pytest command, it excludes the entire directory from default, seed, lab, and --all runs. Checked local pytest --help: --ignore=path means “Ignore path during collection.” This also drops unmarked correctness tests such as tests/benchmarks/test_scale_tiers.py::test_scale_small_*, whose module explicitly declares them part of the default verify gate, so marker-based scale coverage disappears and --all is no longer the documented full non-integration diagnostic. Exclude actual benchmark campaigns without ignoring the whole directory.
AGENTS.md reference: AGENTS.md:L342-L343
Useful? React with 👍 / 👎.
| if source_conn.execute("SELECT 1 FROM raw_sessions WHERE raw_id = ?", (raw_id,)).fetchone() is not None: | ||
| record_current_parser_source_census(source_conn, raw_id, parser_sessions=[session]) |
There was a problem hiding this comment.
When this fixture is used for retained-raw replay or revision-governance checks, this records a current-parser receipt from the already-constructed in-memory session rather than from the bytes just published. _raw_payload() serializes a normalized ParsedSession.model_dump(), not Codex wire records; Codex lowering consumes its messages list and loses the top-level provider_session_id, while census_parse_worker does not use the stored native-id override for this FULL revision, so replay falls back to a filename-derived ID such as 000-revision-chain. The receipt therefore certifies parser output that the retained bytes cannot reproduce, masking replay failures; publish real Codex-shaped bytes or parse the published payload before recording the census.
AGENTS.md reference: AGENTS.md:L153-L156
Useful? React with 👍 / 👎.
| "POLYLOGUE_PYTEST_SELECTION_PATH", | ||
| "POLYLOGUE_PYTEST_SUMMARY_PATH", | ||
| ): | ||
| monkeypatch.delenv(name, raising=False) |
There was a problem hiding this comment.
During a managed devtools verify --seed-testmon run, this autouse fixture deletes the outer progress plugin's event destinations for the entire setup and call phases of every test in this module. The outer plugin reads these variables dynamically from pytest_runtest_logreport, so it records the start and eventual teardown after monkeypatch restoration but no call report; seed finalization disables database fallback and classifies each affected node as missing, making the shard/attempt permanently incomplete on every resume. Isolate destinations only around direct helper calls or in the nested subprocess environment rather than scrubbing them for the lifetime of the outer test.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf960b0705
ℹ️ 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".
| "POLYLOGUE_VERIFY_RUN_ID", | ||
| "POLYLOGUE_PYTEST_EVENTS_DIR", | ||
| "POLYLOGUE_PYTEST_EVENTS_PATH", |
There was a problem hiding this comment.
When a test launches a nested pytest process during a managed seed, preserving this event directory and the outer POLYLOGUE_VERIFY_RUN_ID makes the nested process append reports that are indistinguishable from the outer run's evidence. _seed_node_outcomes_from_events() accepts every report by node ID without checking PID or execution provenance, so if the nested process passes another expected node and the outer run terminates before executing that node itself, finalization can treat it as freshly passed. Preserve the outer plugin environment for the parent process, but give subprocess pytest runs a separate event destination/run ID or filter their events out.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| make_embed_stage(db_path, defer=embed_defer), | ||
| make_claude_workflow_stage(db_path), | ||
| make_insights_stage(db_path), | ||
| make_fts_freshness_stage(db_path), |
There was a problem hiding this comment.
On a large archive, registering this always-applicable stage in the default production sequence makes every live converge_batch() call execute fts_invariant_snapshot_sync(). That snapshot performs exact counts and missing/excess/identity joins across the complete blocks and FTS surfaces, even when the batch contains one small append or the preceding insight stage had no work, turning incremental ingest into repeated O(archive-size) scans. Record freshness incrementally from the changed subjects or defer the exact archive-wide snapshot to a bounded quiet/global-maintenance pass.
AGENTS.md reference: AGENTS.md:L168-L179
Useful? React with 👍 / 👎.
| with open_connection(archive_db) as conn: | ||
| _record_convergence_fts_freshness_sync(conn) | ||
| return True |
There was a problem hiding this comment.
When an exact freshness query raises sqlite3.Error—for example because an FTS shadow table is unreadable—record_fts_freshness_snapshot_sync() logs and returns without updating the ledger, after which this function still returns True. The converger therefore marks fts_freshness done and records no retryable debt, leaving either a stale/empty ledger or an obsolete ready row until an unrelated future pass happens to refresh it. Return a success signal from the snapshot helper or re-raise the failure so this stage remains retryable.
AGENTS.md reference: AGENTS.md:L168-L176
Useful? React with 👍 / 👎.
| kind=RawRevisionKind.FULL, | ||
| source_revision=raw_blob_hash, | ||
| acquisition_generation=index, |
There was a problem hiding this comment.
When the append-prefix property passes append_only=True, the raw row is given source_index=-1 but this code still asserts every observation as an independent FULL revision with a corpus-position generation. Production raw admission instead derives the generation from the previously acquired head and records a byte-prefix successor as APPEND with predecessor/baseline offsets, or quarantines an ambiguous payload; rotated fixture order therefore creates authority evidence that production cannot emit and can keep retained-replay/append tests green when the real revision chain is broken. Route these bytes through the production admission classifier or construct the envelope from the actual predecessor relation.
AGENTS.md reference: AGENTS.md:L114-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@polylogue/daemon/convergence_stages.py`:
- Line 401: Update the convergence stage around
record_fts_freshness_snapshot_sync so the recorder returns an explicit success
status, returning False when snapshot generation catches sqlite3.Error and
succeeds otherwise. In DaemonConverger’s stage handling, propagate that result
and return False instead of reporting convergence success when freshness
snapshot recording fails.
- Around line 754-757: Remove the direct connection_profile import from the
daemon convergence stage and replace the
open_connection/_record_convergence_fts_freshness_sync flow with an existing
operations, insights, or api boundary for the archive freshness update. Keep the
same archive operation behavior while ensuring daemon surface code no longer
imports storage internals.
- Around line 756-757: Update the convergence freshness callback around
DaemonConverger to stop importing or calling open_connection directly. Route the
_record_convergence_fts_freshness_sync write through an existing allowed
daemon-boundary interface in insights, operations, or api, while preserving the
current freshness-recording behavior.
In `@tests/conftest.py`:
- Around line 648-650: Update _restore_plugin_state in tests/conftest.py to
clear POLYLOGUE_PYTEST_SELECTION_PATH and POLYLOGUE_PYTEST_SUMMARY_PATH only for
direct helper calls, preserving supervisor-managed destinations during report
hooks; retain the subprocess regression behavior. In
tests/unit/devtools/test_pytest_progress_plugin.py lines 20-27, adjust the
affected test expectations or setup to reflect the preserved destinations.
In `@tests/unit/devtools/test_pytest_progress_plugin.py`:
- Around line 106-115: Extend the test around subprocess.run to create sentinel
contents in both selection and summary destination files before launching the
child process. After the child exits, read both files and assert their contents
remain unchanged, alongside the existing reports assertions.
🪄 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: 83012f64-fbd4-4192-a972-b9fda44ce7a0
📒 Files selected for processing (6)
devtools/verify.pypolylogue/daemon/convergence_stages.pytests/conftest.pytests/infra/convergence_harness.pytests/unit/devtools/test_pytest_progress_plugin.pytests/unit/devtools/test_verify.py
| with open_connection(archive_db) as conn: | ||
| _record_convergence_fts_freshness_sync(conn) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline polylogue/daemon/convergence.py --items all --match 'DaemonConverger|converge'
rg -nP --type=py -C 4 '\.(?:execute|execute_many|execute_sessions)\s*\(' polylogue/daemon
rg -nP --type=py -C 4 'ProcessPoolExecutor|ThreadPoolExecutor|multiprocessing|Process\(|converge_sessions' polylogue/daemonRepository: Sinity/polylogue
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- DaemonConverger implementation ---'
sed -n '150,590p' polylogue/daemon/convergence.py
printf '%s\n' '--- DaemonConverger call sites ---'
rg -n -P --type=py -C 6 'DaemonConverger\s*\(|\.converge_(?:file|batch|sessions|all)\s*\(' polylogue
printf '%s\n' '--- Worker and process boundaries ---'
rg -n -P --type=py -C 8 'ProcessPoolExecutor|ThreadPoolExecutor|multiprocessing|multiprocessing\.|concurrent\.futures|worker|worker_process|Process\s*\(' polylogue/daemon polylogue | head -n 500
printf '%s\n' '--- Freshness stage definition and callback wiring ---'
rg -n -P -C 10 'make_fts_freshness_stage|_record_convergence_fts_freshness_sync|record_fts_freshness_snapshot_sync|open_connection\s*\(' polylogue/daemon/convergence_stages.pyRepository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Freshness stage wiring ---'
rg -n -P -C 14 'make_fts_freshness_stage|_record_convergence_fts_freshness_sync|record_fts_freshness_snapshot_sync|open_connection\s*\(' polylogue/daemon/convergence_stages.py
printf '%s\n' '--- Convergence construction and execution ---'
sed -n '1720,1780p' polylogue/daemon/cli.py
sed -n '2500,2605p' polylogue/daemon/cli.py
rg -n -P -C 12 'converger\s*=|_converger|Converger|converge_file|converge_batch|converge_sessions' polylogue/sources/live/batch.py polylogue/daemon/cli.py
printf '%s\n' '--- Live batch worker boundary ---'
rg -n -P -C 12 'process_pool_executor|ProcessPoolExecutor|executor\.(map|submit)|worker|_run_batch|_validate_record_sync' polylogue/sources/live/batch.py polylogue/pipeline/services polylogue/daemon/cli.py | head -n 500Repository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
root = Path("polylogue")
target_methods = {"converge_file", "converge_batch", "converge_sessions", "converge_all"}
process_names = {
"ProcessPoolExecutor",
"process_pool_executor",
"multiprocessing",
"Process",
}
for path in sorted(root.rglob("*.py")):
try:
tree = ast.parse(path.read_text(), filename=str(path))
except (OSError, SyntaxError):
continue
for node in ast.walk(tree):
if isinstance(node, ast.Call):
func = node.func
if isinstance(func, ast.Attribute) and func.attr in target_methods:
print(f"CONVERGENCE_CALL {path}:{node.lineno}: .{func.attr}()")
if isinstance(func, ast.Name) and func.id in process_names:
print(f"PROCESS_CALL {path}:{node.lineno}: {func.id}()")
if isinstance(func, ast.Attribute) and func.attr in process_names:
print(f"PROCESS_CALL {path}:{node.lineno}: .{func.attr}()")
for node in ast.walk(tree):
if isinstance(node, ast.ImportFrom) and node.module:
imported = {alias.name for alias in node.names}
if imported & process_names:
print(f"PROCESS_IMPORT {path}:{node.lineno}: {node.module}: {sorted(imported & process_names)}")
elif isinstance(node, ast.Import):
imported = {alias.name.split(".")[0] for alias in node.names}
if imported & process_names:
print(f"PROCESS_IMPORT {path}:{node.lineno}: {sorted(imported & process_names)}")
PYRepository: Sinity/polylogue
Length of output: 1646
Route the freshness write through an allowed daemon boundary.
DaemonConverger has no process-pool convergence call sites, so this callback does not need a main-process queue. However, record() directly imports polylogue.storage.sqlite.connection_profile.open_connection, which violates the daemon boundary. Route the write through insights, operations, or api.
🤖 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 `@polylogue/daemon/convergence_stages.py` around lines 756 - 757, Update the
convergence freshness callback around DaemonConverger to stop importing or
calling open_connection directly. Route the
_record_convergence_fts_freshness_sync write through an existing allowed
daemon-boundary interface in insights, operations, or api, while preserving the
current freshness-recording behavior.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a55b7f5503
ℹ️ 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".
| if ( | ||
| os.environ.get("PYTEST_CURRENT_TEST") | ||
| and os.environ.get("POLYLOGUE_VERIFY_RUN_ID") | ||
| and not os.environ.get("POLYLOGUE_PYTEST_NESTED_PRIVATE") |
There was a problem hiding this comment.
When pytest is launched by managed devtools verify, pytest sets PYTEST_CURRENT_TEST before autouse fixture setup, so this predicate is true in the outer test process itself rather than only in nested subprocesses. It deletes the run/event destinations before setup and call reports, and the progress plugin reads those variables dynamically, causing seed finalization to classify current nodes as missing and refuse to publish the testmon seed. Fresh evidence after the earlier nested-ledger finding is that isolation has moved into this root autouse fixture with a predicate that also matches every outer test; isolate the subprocess environment at launch instead.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| if admission.arm.value not in {"baseline", "append", "supersede"}: | ||
| raise AssertionError(f"raw fixture admission was not executable: {admission!r}") |
There was a problem hiding this comment.
When the rich convergence corpus processes its two revisions of the same native session, _raw_payload() serializes each revision as a complete JSON document while the composer changes the title, metadata, and existing message text. The second payload is therefore neither equal to nor a byte-prefix extension of the first, so admit_raw_observation() returns REFUSED_AMBIGUOUS and this assertion aborts build_converged_archive() before the append-prefix property can run. Fresh evidence after the prior raw-admission comment is that the harness now invokes the production classifier, but its synthetic revision bytes still cannot satisfy that classifier; construct genuinely growing bytes or model these as independent full observations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31e08362ba
ℹ️ 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".
| from polylogue.storage.blob_publication import ArchiveBlobPublisher, consume_blob_publication_receipt | ||
| from polylogue.storage.sqlite.archive_tiers.bootstrap import initialize_archive_tier | ||
| from polylogue.storage.sqlite.archive_tiers.source_write import write_source_raw_session | ||
| from polylogue.storage.sqlite.archive_tiers.raw_admission import PriorRawHead, admit_raw_observation |
There was a problem hiding this comment.
When test_convergence_property_raw_replay_mutation_red_twin runs, its monkeypatch.setattr(convergence_harness, "write_source_raw_session", ...) now raises AttributeError because this change removes that imported module attribute and routes ingestion through admit_raw_observation. The affected/full verification therefore fails before exercising the mutation assertion; retarget the mutation at the new admission path or preserve an injectable raw-writer seam.
AGENTS.md reference: AGENTS.md:L342-L343
Useful? React with 👍 / 👎.
| selected = _validate_session_indexes(pathology, session_indexes) | ||
| source_paths: list[Path] = [] | ||
| session_ids: list[str] = [] | ||
| prior_heads: dict[str, PriorRawHead] = {} |
There was a problem hiding this comment.
When a logical session's revisions are split across calls to ingest_convergence_pathology—as test_convergence_property_append_prefix_matches_full explicitly does for its prefix and delta—this per-call empty map forgets the head already stored in source.db. The delta is consequently admitted with prior_head=None as another asserted generation-0 baseline rather than an append/supersede revision, so its durable authority facts diverge from the one-call archive and the property no longer models resumable production ingestion. Resolve the prior head from the existing source tier or carry it between calls.
AGENTS.md reference: AGENTS.md:L342-L343
Useful? React with 👍 / 👎.
## 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_finished` plus 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 - record complete per-node seed outcomes and preserve resumable shard state; - require call-phase evidence or a corroborating testmon result before accepting a passing teardown; - keep missing, failed, worker-crash, timeout, and interrupted outcomes distinct; - isolate unit-test testmon caches so checkout-integrity tests observe real state; - preserve the managed event ledger across the host environment scrub. ## 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 at `eda8751e3`; - current-head Codex review was addressed with a regression test for teardown-only evidence. This PR is self-contained on top of the already-merged testmon sharding work in #3956. <!-- polylogue-pr-scope:v2 { "assigned_beads": [], "dispositions": [], "mutated_beads": [], "scope_digest": "79a7984a9ec80a157c96dc8d28561159c642c15de3793837eff751fa7eac34a1", "scope_kind": "self_contained", "version": 2 } --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added durable, resumable test verification through deterministic shards. * Verification now records per-shard progress and outcomes, allowing interrupted runs to continue safely. * Added validation to ensure complete and consistent test coverage before finalizing results. * **Bug Fixes** * Improved handling of teardown outcomes so incomplete tests are not incorrectly marked as successful. * Benchmark tests are excluded from standard verification collection. * Improved freshness and readiness checks for search-related data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
Make the pytest-testmon seed resumable and bounded by deterministic serial shards.
Problem
The previous seed ran 21,079 non-integration tests in one xdist invocation, timed out after 45 minutes, and produced no eligible seed. Raising the timeout or weakening release checks would hide failures and preserve an unusable harness.
Solution
Collect the complete correctness-test node set before testmon writes, partition it into deterministic 256-node shards, run one shard at a time with one SQLite writer, checkpoint shard/node outcomes, resume unfinished shards, and require a terminal complete failure-free graph before publishing
seed.json. Protocol version 7 invalidates old incomplete receipts. Benchmark campaigns remain explicit and are excluded from the correctness/testmon seed.Verification
devtools test tests/unit/devtools/test_verify.py tests/unit/devtools/test_testmon_state.py tests/unit/devtools/test_checkout_guard.py— 185 passed before the marker correction; 185 passed after itdevtools verify --quick— all 25 steps passeda50b4b548; no release seed is claimed yet.This is a self-contained test-harness correction; no Beads state was changed.
Summary by CodeRabbit
Improvements
Bug Fixes