fix(devtools): preserve fail-closed testmon provenance - #3900
Conversation
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis change introduces typed, checkout-bound testmon state validation, transactional seed bootstrap and recovery, structured verification authorization, and durable merge-intent reconciliation. It also adds detached-worktree terminal verification, expanded tests, and related issue acceptance criteria. ChangesTestmon state and bootstrap
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 655dc5255b
ℹ️ 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".
| @@ -2649,6 +2718,19 @@ def main(argv: list[str] | None = None) -> int: | |||
| _warn_low_memory() # check again right before the heavy step | |||
| rc, elapsed, metadata = _run(label, cmd, run=verify_run) | |||
| if rc == 0 and label in {"pytest testmon", "pytest testmon (broad)"}: | |||
There was a problem hiding this comment.
When an ordinary affected run exposes a test failure, pytest-testmon still updates the SQLite execution row from passing to failed—the new integration test demonstrates that a failing testmon run produces a usable graph in tests/integration/devtools/test_testmon_seed_recovery.py:32-42—but this branch refreshes the stamped database fingerprint only when rc == 0. The next default verification therefore rejects the now-stale stamp and requires an expensive full --seed-testmon run instead of rerunning the failed affected test after it is fixed. Record refreshed selection-only red provenance after a completed failing affected run as well.
AGENTS.md reference: AGENTS.md:L323-L327
Useful? React with 👍 / 👎.
| expected_nodeids=expected, | ||
| database=database, | ||
| pytest_step=pytest_step, | ||
| use_database_fallback=False, |
There was a problem hiding this comment.
On environments where a pytest.mark.skipif condition is true, pytest emits a skipped setup report, which devtools/pytest_progress_plugin.py:202-218 records normally. The outcome classifier only recognizes skipped call reports, and disabling the database fallback here therefore labels every setup-skipped node as missing; unsuccessful_nodeids then makes an otherwise successful full seed incomplete and exits 5. This prevents fresh checkouts missing optional tools such as Node from ever establishing the seed required by the default verifier, so skipped reports must be accepted regardless of whether they occur during setup or call.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| is None | ||
| ): | ||
| return False | ||
| _atomic_write_json(local_seed_attempt, rebound_attempt) |
There was a problem hiding this comment.
If the main checkout is killed after _finalize_testmon_seed_attempt atomically writes a status: reusable red attempt but before it writes seed.json, the new fallback accepts that attempt and copies it here without a local seed stamp. The linked-worktree checkout guard, however, only recognizes unmarked attempts whose status is running or incomplete (devtools/checkout_guard.py:280), so the immediately following assert_polylogue_matches_checkout rejects the supposedly successful bootstrap before _testmon_preflight can validate it. Either emit the rebound typed stamp or teach the guard to validate reusable attempt receipts.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| or not isinstance(attempt.get("expected_digest"), str) | ||
| or attempt.get("expected_digest") != hashlib.sha256("\n".join(sorted(expected)).encode()).hexdigest() |
There was a problem hiding this comment.
After the first interrupted seed, _prepare_testmon_seed_attempt recovers the node ledger and writes it into the new running receipt with expected_count, but it does not write expected_digest. If that resumed run is itself interrupted before finalization, the next invocation reaches this new validation, rejects the otherwise intact ledger because the digest is absent, and starts another full --testmon-noselect run rather than resuming. Include the digest whenever the prepared receipt carries recovered node IDs.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| and validate_stamp( | ||
| marker_path, | ||
| state_path / "testmondata", | ||
| checkout_root=repo_root, | ||
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | ||
| ) |
There was a problem hiding this comment.
In every linked worktree containing a seed, this makes the checkout guard run full validate_stamp on every guarded entry point. That call hashes the entire roughly 28 MB testmon database, runs PRAGMA integrity_check, fetches every execution, fingerprint, and dependency-edge row, and scans them in Python; because the guard is wired into devtools, pytest, and the product CLI, even lightweight commands now pay a full graph-validation cost before doing useful work. Keep the guard to cheap checkout-binding checks and reserve the exhaustive SQLite validation for verify/bootstrap preflight.
AGENTS.md reference: AGENTS.md:L609-L616
Useful? React with 👍 / 👎.
| if isinstance(environment, dict): | ||
| environment["checkout_root"] = str(checkout_root.resolve()) | ||
| environment["verify_state_origin"] = str(checkout_root.resolve()) | ||
| _atomic_write_json(destination / "run.json", payload_dict) |
There was a problem hiding this comment.
On a fresh linked worktree, rebinding the source receipt here creates .cache/verify/runs/<run_id>/run.json but does not create the .cache/verify/current-run.json ownership marker required by _cache_artifact. verify.main calls maybe_bootstrap_testmon_seed before assert_polylogue_matches_checkout and before constructing its own VerifyRun, so the guard immediately classifies this newly created directory as an unverified inherited verify cache and exits 125; consequently even a valid green main seed cannot actually bootstrap into a fresh lane. Either bind a marker the guard recognizes or exempt the specifically validated rebound receipt layout.
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: b10e784bf5
ℹ️ 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".
| return False | ||
| if argv[:2] != ["devtools", "verify"]: | ||
| return False | ||
| return len(argv) == 2 or any(option in argv[2:] for option in ("--all", "--full", "--lab", "--seed-testmon")) |
There was a problem hiding this comment.
When the recorded command is devtools verify --json, devtools verify --skip-slow, or any other affected-test invocation with an unlisted option, this returns false even though the command can exit 0 while emitting release_baseline_allowed: false. cmd_check then skips the new permission check and allows a selection-only red testmon attempt to satisfy the merge boundary; classify all non-quick/non-commit devtools verify forms rather than enumerating only four options.
AGENTS.md reference: AGENTS.md:L401-L409
Useful? React with 👍 / 👎.
| _warn_low_memory() # check again right before the heavy step | ||
| rc, elapsed, metadata = _run(label, cmd, run=verify_run) | ||
| if rc == 0 and label in {"pytest testmon", "pytest testmon (broad)"}: | ||
| raw_stamp = _read_json_artifact(TESTMON_SEED_STAMP) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier failed-run case: when verification starts from a reusable red seed-attempt.json, a successful affected run changes testmondata (for example, changing the formerly failing node to passed), but this branch only looks for seed.json, which is deliberately absent for red attempts. The attempt's recorded database fingerprint is therefore stale, so the next default verification rejects the otherwise complete graph and requires a full seed; refresh and persist the selection-only attempt as well.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| if ( | ||
| local_testmon_data.is_file() | ||
| and local_seed_stamp.is_file() |
There was a problem hiding this comment.
In a linked worktree bootstrapped from a red main-checkout attempt, the local state intentionally contains testmondata plus seed-attempt.json and no seed.json. This local-state check can therefore never succeed, so every subsequent devtools verify invocation repeats the online database copy, full fingerprint, integrity check, and graph scan from main even while the local attempt remains valid; validate the local attempt before deciding to bootstrap again.
Useful? React with 👍 / 👎.
| expected_nodeids=expected, | ||
| database=database, | ||
| pytest_step=pytest_step, | ||
| use_database_fallback=False, |
There was a problem hiding this comment.
When an interrupted seed resumes and pytest selects only the outstanding subset—as the resumed-selection handling below explicitly permits—the new events file has no reports for nodes that completed in the original attempt. Disabling fallback here classifies every such prior node as missing, so unsuccessful_nodeids prevents the resumed run from ever producing a green or reusable graph even after all missing edges are repaired; carry forward terminal outcomes proven by the prior attempt while still refusing unproven database-only rows.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Line 2328 in 7d1909d
Bind resumed seeds to the committed tree
When an interrupted seed is resumed after switching or rebasing to another clean revision, this contract still matches because it omits git_head and _worktree_fingerprint() hashes only status/diffs relative to the current HEAD, making every clean revision share the same fingerprint. The resume then retains the old revision's expected_nodeids, allowing finalization to issue a reusable or green stamp whose promised corpus omits tests introduced by the new revision; compare a committed-tree identity, while still permitting different commits with identical trees.
AGENTS.md reference: AGENTS.md:L338-L341
Lines 2597 to 2602 in 7d1909d
Persist inherited selection counts after resume
When a resumed seed executes only the outstanding subset, this persisted selection retains that subset's selected_count, even though expected_nodeids is the inherited full ledger; only the transient attempt_candidate above overrides the count. The resulting complete attempt fails stamp_from_attempt because its count differs from len(expected), so a crash between writing this attempt and seed.json, or a subsequently missing stamp, cannot recover or bootstrap the completed graph and instead forces another full seed.
ℹ️ 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".
| ) -> dict[str, Any]: | ||
| prior = _read_testmon_seed_attempt() if resume else None | ||
| expected = _testmon_seed_expected_nodeids(prior) if prior is not None else [] | ||
| prior_outcomes = prior.get("node_outcomes") if isinstance(prior, Mapping) else None |
There was a problem hiding this comment.
Fresh evidence beyond the previously flagged one-resume case: if a resumed seed is interrupted after _prepare_testmon_seed_attempt writes its new receipt, the carried terminal results now exist only under prior_node_outcomes; the next resume reads only node_outcomes here and replaces that ledger with an empty list. Nodes completed before the first interruption but not selected by the eventual resume are consequently classified as missing, so recovery cannot finalize without another full seed.
Useful? React with 👍 / 👎.
| affected = _read_json_artifact(TESTMON_AFFECTED_STAMP) | ||
| if isinstance(affected, dict) and affected.get("identity") == identity: | ||
| selected_count = affected.get("selected_count") if isinstance(affected, dict) else None | ||
| if ( | ||
| isinstance(affected, dict) | ||
| and affected.get("protocol_version") == 1 | ||
| and affected.get("status") == "complete" |
There was a problem hiding this comment.
After a successful --seed-testmon on a PR with executable changes, the dependency database is current, so the next default verify normally selects zero tests; this function now consults only the affected-run receipt, which seed runs never write. The zero-selection guard therefore exits 5 despite the just-completed full seed, and it cannot create the required affected receipt because those are recorded only when selected_count > 0; once the seed is bound to the committed tree, recognize that validated seed as exact coverage.
AGENTS.md reference: AGENTS.md:L325-L327
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: 2eb71bb05d
ℹ️ 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".
| f"recorded merge-train terminal verify: {command!r} exit={result.returncode} " | ||
| f"release_baseline_allowed={release_allowed!r} accepted={accepted} ({duration_s}s)" | ||
| ) | ||
| if not accepted and result.returncode == 0: |
There was a problem hiding this comment.
When the verification command exits 0 but emits no valid release_baseline_allowed: true receipt, this branch records accepted: false yet cmd_record_full_verify still returns the command's zero exit code. Consequently, workspace merge --with-verify also reports success even though train-status remains incomplete, so automation can continue without the promised terminal full-suite ledger step; return a nonzero status whenever accepted is false.
AGENTS.md reference: AGENTS.md:L409-L415
Useful? React with 👍 / 👎.
| if refreshed is None or refreshed.graph != rebound.graph: | ||
| return False |
There was a problem hiding this comment.
When the main checkout completes an affected run between source validation and the SQLite online backup, the copied database can legitimately have a different failure graph, causing this check to return false after local_testmon_data has already been installed. maybe_bootstrap_testmon_seed then continues to the checkout guard, which rejects the markerless partial cache, so a supported fresh linked-worktree bootstrap fails because of a transient concurrent update; remove the copied state or retry against the refreshed source before returning.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| checkout_root=ROOT, | ||
| protocol_version=TESTMON_SEED_PROTOCOL_VERSION, | ||
| ) | ||
| return stamp.release_baseline_allowed if stamp is not None else False |
There was a problem hiding this comment.
If a green seed run is interrupted after writing seed-attempt.json at line 2624 but before publishing seed.json at line 2626, this fallback reconstructs the green stamp from the attempt and returns release_baseline_allowed=true. The next affected verification therefore treats an incompletely published attempt as release-baseline authority even though the marker is absent; attempts should remain selection-only, with true release permission requiring the successfully published seed marker.
Useful? React with 👍 / 👎.
|
@codex review |
|
Exact-head hold for
The persisted |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06aa4888f
ℹ️ 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 ( | ||
| verification_scope == VerificationScope.RELEASE_BASELINE.value | ||
| and receipt.get("release_baseline_allowed") is not True | ||
| ): |
There was a problem hiding this comment.
When workspace merge records a custom command such as devtools verify --all --skip-slow, verification emits verification_scope: narrow-terminal, exits 0, and deliberately sets release_baseline_allowed: false; this condition checks only release-baseline, so cmd_check still returns success and the YAML-independent merge path proceeds without the required typed authorization. Apply the permission and terminal_authorization checks to narrow-terminal receipts as well.
AGENTS.md reference: AGENTS.md:L400-L409
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: ff8e51f83f
ℹ️ 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".
| add = subprocess.run( | ||
| ["git", "worktree", "add", "--detach", str(worktree), target_sha], |
There was a problem hiding this comment.
When workspace merge --with-verify runs from the standard activated checkout environment, this raw git worktree add creates no worktree-local .venv, so the subsequent devtools verify --all inherits the coordinator checkout's interpreter. The checkout guard identifies that interpreter as belonging to another checkout and exits 125 before verification, meaning the command reports failure only after the PR has already merged and cannot record the promised terminal ledger step. Provision an isolated environment for this detached worktree before invoking verification.
AGENTS.md reference: AGENTS.md:L367-L371
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 361e4985d8
ℹ️ 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 not isinstance(release_allowed, bool): | ||
| verdict.ok = False | ||
| verdict.reasons.append("verification receipt lacks typed release_baseline_allowed permission") | ||
| if verification_scope == VerificationScope.RELEASE_BASELINE.value and release_allowed is not True: |
There was a problem hiding this comment.
When verification starts from a reusable red seed attempt, a successful default devtools verify emits verification_scope: affected with release_baseline_allowed: false; this condition rejects that permission only for release-baseline, so merge-gate check still authorizes the merge. Fresh evidence beyond the earlier comment is the newly added test_check_accepts_affected_receipt_without_release_baseline_permission, which explicitly locks in this bypass. Require typed release permission for affected receipts as well so selection-only red provenance cannot cross the merge boundary.
AGENTS.md reference: AGENTS.md:L400-L409
Useful? React with 👍 / 👎.
| "accepted", | ||
| "merge_sequence", | ||
| "verification_scope", | ||
| "release_baseline_allowed", | ||
| ): | ||
| if key not in receipt: | ||
| raise LedgerStateError(f"merge-train terminal receipt is missing {key!r}") |
There was a problem hiding this comment.
On any checkout that previously ran record-full-verify, the persisted last_full_verify contains only command, exit_code, duration_s, and at; requiring all these new fields makes _read_ledger reject the entire pre-upgrade ledger. Consequently both the required merge wrapper and train-status become unusable after upgrading, while deleting the cache would lose pending-merge history and can incorrectly report a clean train. Treat the legacy receipt as untrusted while retaining its merge entries, or migrate it explicitly.
AGENTS.md reference: AGENTS.md:L410-L415
Useful? React with 👍 / 👎.
| try: | ||
| with _ledger_lock(): | ||
| ledger = _read_ledger_unlocked() | ||
| ledger["last_full_verify"] = { |
There was a problem hiding this comment.
When one merge train has a valid accepted terminal receipt and a later train's terminal verification fails or lacks authority, this assignment overwrites the sole successful receipt with accepted: false. _pending_prs_since_last_full_verify then falls back to timestamp zero and reports every historical merge as pending, rather than only merges since the last successful verification. Keep the last accepted receipt separately from the latest failed attempt.
AGENTS.md reference: AGENTS.md:L410-L415
Useful? React with 👍 / 👎.
| if verification_scope not in {scope.value for scope in VerificationScope}: | ||
| verdict.ok = False | ||
| verdict.reasons.append( | ||
| "verification receipt lacks a valid typed verification_scope; command text cannot grant authority" | ||
| ) | ||
| release_allowed = receipt.get("release_baseline_allowed") | ||
| if not isinstance(release_allowed, bool): | ||
| verdict.ok = False | ||
| verdict.reasons.append("verification receipt lacks typed release_baseline_allowed permission") |
There was a problem hiding this comment.
When the documented devtools workspace merge <PR> --command "devtools test tests/unit/foo.py" form is used, cmd_record receives ordinary test output, so both typed fields are None; these unconditional checks then make every such receipt fail even when the focused command exits successfully. Either wrap supported non-verify commands in a typed receipt or stop advertising the custom focused-command route.
AGENTS.md reference: AGENTS.md:L400-L409
Useful? React with 👍 / 👎.
| if isinstance(local_attempt, Mapping) and attempt_is_checkout_bound( | ||
| local_attempt, | ||
| checkout_root=local_root, | ||
| protocol_version=protocol_version, | ||
| ): |
There was a problem hiding this comment.
When a linked worktree's local testmondata is truncated, changed by an interrupted affected run, or otherwise no longer matches its reusable attempt, this uses attempt_is_checkout_bound, which explicitly does not inspect the SQLite graph or compare the recorded database fingerprint, and therefore skips a valid bootstrap from main. The later verify preflight performs the exhaustive validation and rejects the stale local state, leaving the lane unable to use the automatic main-checkout bootstrap; validate the local attempt with stamp_from_attempt here and fall through to main when it fails.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| if info.get("state") != "MERGED" or not isinstance(merge_commit, dict) or not merge_commit.get("oid"): | ||
| raise LedgerStateError(f"unresolved durable merge intent for PR #{intent['pr']}") | ||
| _complete_merge_intent(int(intent["pr"]), str(intent["head_sha"])) |
There was a problem hiding this comment.
If an intent is recorded for head A and gh pr merge --match-head-commit A fails because the PR advanced to head B, the intent remains; the local gh pr merge --help confirms that this flag requires the head to match. If B is later merged, this reconciliation checks only that the PR is now MERGED with some merge commit, then records A as though it crossed the merge boundary, creating false exact-head audit evidence and a phantom pending entry. Reconciliation must prove the merged PR used the intent's recorded head before completing it.
AGENTS.md reference: AGENTS.md:L400-L409
Useful? React with 👍 / 👎.
361e498 to
6d85102
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d85102726
ℹ️ 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".
| return 1 | ||
| _snapshot_ledger, verification_started_at, merge_sequence = snapshot | ||
| if execution_root is not None: | ||
| argv = ["direnv", "exec", str(execution_root), *argv] |
There was a problem hiding this comment.
Fresh evidence beyond the earlier raw-interpreter case: the replacement now invokes direnv exec in a newly randomized detached worktree, but nothing runs direnv allow for that path. Because this repository's .envrc must be authorized before use flake provisions the checkout-local .venv, record-full-verify exits before verification, and merge --with-verify does so only after the PR has already merged. Provision and authorize the detached worktree before executing the terminal command.
AGENTS.md reference: AGENTS.md:L367-L371
Useful? React with 👍 / 👎.
| identity = prepared.get("identity") | ||
| narrow_terminal = isinstance(identity, Mapping) and identity.get("skip_slow") is True | ||
| terminal_authorized = _testmon_seed_terminal_authorized(prepared) | ||
| release_eligible = green_complete and (not narrow_terminal or terminal_authorized) |
There was a problem hiding this comment.
With the documented fresh-checkout command devtools verify --seed-testmon --skip-slow, narrow_terminal is true while terminal_authorized is false, so this makes release_eligible false even when green_complete is true. The finalizer consequently records a complete dependency graph as reusable, and main converts the successful pytest result to exit 5 because the status is not complete. This makes the repository's standard seed command report failure after doing all the work; either let a complete graph succeed as selection-only state or include the required authorization in the documented command.
AGENTS.md reference: AGENTS.md:L338-L341
Useful? React with 👍 / 👎.
| print(f"merged PR #{pr} @ {head_sha[:8]}: {clean_title!r}") | ||
| _append_merge_entry(pr, head_sha, clean_title) | ||
| try: | ||
| _complete_merge_intent(pr, head_sha) |
There was a problem hiding this comment.
On a protected branch using GitHub's merge queue, a zero exit from gh pr merge does not prove that the PR merged: the local gh pr merge --help states that it may enable auto-merge or add the PR to the queue. Completing the durable intent here immediately creates a merge ledger entry before the commit reaches the default branch; a terminal verification run during that interval can verify the old branch and clear the phantom entry, after which the eventual merge is absent from the ledger. Keep the intent pending until gh pr view proves this exact head was merged.
AGENTS.md reference: AGENTS.md:L400-L409
Useful? React with 👍 / 👎.
|
@codex review Please review exact pushed head |
|
@codex review Please review exact pushed head |
There was a problem hiding this comment.
💡 Codex Review
Line 1697 in ffa2d17
Record the capped worker count in workload receipts
When a seed runs on a host where the adaptive policy chooses more than four workers, the command is correctly capped to -n 4, but this records the uncapped runtime_policy.workers value (for example, 12) as the workload envelope's concurrency. The resulting seed resource receipt therefore contradicts the executed command and cannot provide accurate worker/resource provenance; derive concurrency from the final pytest command or apply the seed cap to the recorded policy as well.
ℹ️ 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".
|
@circleci run |
|
Reopening immediately to refresh the stale Circle quick-gate after synchronizing the structured carrier; no source rollback or draft state. |
|
@circleci run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/devtools/test_verify.py (1)
1026-1148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting this five-stage sequential test.
test_seed_completion_requires_full_failure_free_databaseruns five_finalize_testmon_seed_attemptcalls against one sharedTESTMON_DATA, mutating the database andevents.jsonlbetween stages. Each stage depends on the mutations of the prior stage.A regression in the first stage cascades. The failure output points at the first assertion, not at the stage that actually broke. Five distinct behaviors are pinned: green completion, authorized narrow-terminal completion, red reuse, stale-database rejection, and orphan-edge rejection.
The stages share an expensive SQLite fixture, which is the reason for the coupling. A module-scoped fixture that yields a fresh copy per stage would keep the setup cost low and localize failures.
🤖 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 1026 - 1148, Split test_seed_completion_requires_full_failure_free_database into five focused tests covering green completion, authorized narrow-terminal completion, red reuse, stale-database rejection, and orphan-edge rejection. Keep each test’s setup and database/events.jsonl mutations isolated by using a module-scoped fixture that yields a fresh copy of TESTMON_DATA for each case, while preserving the existing assertions and _finalize_testmon_seed_attempt calls.devtools/merge_boundary.py (2)
779-779: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
--jsonexplicit in the terminal verify command.Line 700 parses the child's entire stdout with
json.loads. The default--verify-commandat Line 779 is"devtools verify --all", with no--jsonflag.
devtools/verify.pydecides its output format implicitly:use_json = args.json if args.json is not None else not sys.stdout.isatty(). Becausesubprocess.run(..., capture_output=True)gives the child a pipe,isatty()is False and JSON is emitted. The contract works today only through that inference.If the terminal verify is ever launched through a pty-allocating wrapper — a CI runner that allocates a tty,
script, or an interactive agent shell —isatty()becomes True.devtools verifythen writes the human summary to stderr and nothing to stdout.json.loads("")raises,structuredbecomesNone,verified_headbecomesNone, andacceptedisFalse.The failure direction is safe, but the outcome is a false negative: a fully green release-baseline run is recorded as unaccepted, and the operator sees "POST-MERGE BROAD VERIFY DID NOT GRANT typed release-baseline authority" with no indication that the real cause was output formatting.
devtools/merge_gate.pyhas the same implicit dependency in_release_baseline_permission,_verification_scope, and_terminal_authorization, all of which parseresult.stdout.🔒 Proposed fix
- merge_p.add_argument("--verify-command", default="devtools verify --all", help="Command for --with-verify") + merge_p.add_argument( + "--verify-command", + default="devtools verify --all --json", + help="Command for --with-verify; must emit the structured JSON receipt on stdout", + )record_p = sub.add_parser( "record-full-verify", help="Run and record the merge-train's terminal full-suite verify now" ) - record_p.add_argument("--command", default="devtools verify --all") + record_p.add_argument("--command", default="devtools verify --all --json")Distinguishing an unparseable receipt from a genuinely refused one also makes the diagnostic honest:
try: structured = json.loads(result.stdout) except (TypeError, json.JSONDecodeError): structured = None + if structured is None and result.returncode == 0: + print( + f"terminal verify exited 0 but emitted no structured receipt on stdout; " + f"ensure {command!r} includes --json", + file=sys.stderr, + )Also applies to: 696-709
🤖 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/merge_boundary.py` at line 779, Update the default verify command in merge_p to explicitly include --json, ensuring subprocess output remains parseable regardless of TTY allocation. Also update the terminal verification command construction and related parsing flows in _release_baseline_permission, _verification_scope, and _terminal_authorization to request JSON explicitly, and distinguish unparseable output from a valid refusal when reporting authorization results.
569-593: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed
gh pr mergeorphans the intent permanently and wedgestrain-status.
cmd_mergerecords a durable intent at Line 570, then runsgh pr merge. If the merge fails, Line 591 returnsmerge_result.returncodeand never removes the intent. Only_complete_merge_intent, reached at Line 597 after a successful merge, clears it.
_reconcile_merge_intentsthen raises on that stale intent. Line 353 checks the PR state; an unmerged PR is notMERGED, so Line 354 raisesLedgerStateError(f"unresolved durable merge intent for PR #{intent['pr']}").cmd_train_statuscatches it at Line 631 and returns 1.The result: one transient
gh pr mergefailure — a network blip, a branch-protection rejection, a head-commit race — permanently blockstrain-status. The reconciler raises on the first unresolved intent, so it never reaches later intents either. No command in this module clears an intent, so recovery requires hand-editing the ledger JSON.The intent-before-merge ordering is correct and necessary; it is what makes external-merge recovery work. The gap is the missing rollback on the known-failed path.
🔒 Proposed rollback on a failed merge
Add an intent-abandon helper and call it when
gh pr mergefails:+def _abandon_merge_intent(pr: int, head_sha: str) -> None: + """Drop a durable intent whose merge attempt provably did not happen.""" + with _ledger_lock(): + ledger = _read_ledger_unlocked() + remaining = [ + intent + for intent in ledger["merge_intents"] + if not (intent.get("pr") == pr and intent.get("head_sha") == head_sha) + ] + if len(remaining) != len(ledger["merge_intents"]): + ledger["merge_intents"] = remaining + _write_ledger_unlocked(ledger)if merge_result.returncode != 0: print(f"gh pr merge failed: {merge_result.stderr.strip()[:500]}", file=sys.stderr) + try: + _abandon_merge_intent(pr, head_sha) + except LedgerStateError as exc: + print( + f"WARNING: merge failed and the durable intent for PR #{pr} could not be cleared: {exc}", + file=sys.stderr, + ) return merge_result.returncodeOnly abandon on a definite non-merge. If the intent cannot be cleared, keep the latch and say so — that preserves the fail-closed guarantee for the ambiguous case.
A regression test belongs alongside
test_external_merge_before_completion_is_reconciled_from_durable_intent: fail the merge withmerge_exit=3, then assertcmd_train_status(as_json=False) == 0and thatmerge_intentsis empty.Also applies to: 344-360
🤖 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/merge_boundary.py` around lines 569 - 593, Update cmd_merge to roll back the durable intent when gh pr merge returns a definite nonzero failure: add an intent-abandon helper alongside _record_merge_intent/_complete_merge_intent, invoke it on the failed merge path, and return the merge exit code only after attempting cleanup. If abandoning fails, retain the intent and report that cleanup failed to preserve fail-closed behavior. Add a regression test alongside test_external_merge_before_completion_is_reconciled_from_durable_intent asserting a failed merge leaves merge_intents empty and cmd_train_status(as_json=False) returns 0.tests/unit/devtools/test_merge_boundary.py (1)
596-601: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
frozen_clockfor the timestamp-dependent merge tests.
test_train_status_blocks_when_pr_merged_after_last_full_verifyreadsmerged_atfrom_append_merge_entry, andtest_record_full_verify_clears_pending_prscompares it with theverification_started_atproduced in the same test flow. Add@pytest.mark.frozen_clock_modules("devtools.merge_boundary")and request thefrozen_clockfixture in these tests so the ordering depends on an explicit fixture value rather than host clock jumps.🤖 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_merge_boundary.py` around lines 596 - 601, Add the frozen_clock_modules marker for devtools.merge_boundary and include the frozen_clock fixture in test_train_status_blocks_when_pr_merged_after_last_full_verify and test_record_full_verify_clears_pending_prs. Update these timestamp-dependent tests to use the explicit frozen clock for merge and verification times, preserving their existing assertions and behavior.Source: Coding guidelines
🤖 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 @.beads/issues.jsonl:
- Line 3: Add an acceptance criterion to issue polylogue-ox2iz requiring
synthetic fixtures only, prohibiting reads from /realm/data/... and uploads of
real session corpora to cloud lanes. Require the canary receipt to record the
fixture-set identity and provenance while preserving the existing sampled-corpus
requirements.
- Line 89: Add an explicit acceptance criterion requiring disposition of
polylogue/maintenance/raw_authority_reset.py: delete the module if obsolete, or
route it exclusively through the typed maintenance coordinator. Ensure any
retained recovery path uses the same authorization, daemon writer-ownership,
durable-receipt, and restart-recovery validation as the existing maintenance
flow.
- Line 89: Correct the lease metadata for issue polylogue-fbkr so it is
internally consistent: either mark the issue unclaimed by updating status,
assignee, and notes together, or renew lease_expires_at and heartbeat_at while
retaining the assignment and in_progress status. Do not leave stale August 5
lease timestamps with the current in_progress assignment.
In `@devtools/checkout_guard.py`:
- Around line 365-373: Update the invalid_testmon_seed EnvironmentArtifact
detail in seed_marker_is_checkout_bound to describe only stale or malformed
marker data and checkout-binding mismatch; remove the claim that the SQLite
graph is incomplete. Leave the remediation and later _testmon_preflight graph
validation unchanged.
- Around line 287-298: Consolidate the duplicate status handling around
attempt_is_checkout_bound by using one condition that accepts both "complete"
and "reusable" statuses, then perform the existing call once with the same
arguments. Preserve the default reusable_only behavior and leave other statuses
on their existing path.
In `@devtools/merge_boundary.py`:
- Around line 686-695: In the verification execution flow around the
subprocess.run call, stop using verification_started_at/started to calculate
duration_s because it includes fetch and worktree setup; capture a separate
timestamp immediately before subprocess.run and compute duration_s from that
timestamp after the command completes. Keep verification_started_at unchanged
for at and pending-window calculations, and preserve the current unbounded
subprocess execution.
- Around line 221-223: Update the LedgerStateError raised by
_read_ledger_unlocked when _LEDGER_PENDING_PATH exists to include the pending
file path and the concrete recovery action, matching the actionable command
guidance used by cmd_merge. Keep the fail-closed behavior unchanged.
- Around line 83-85: The merge-train ledger paths must be rooted at the
repository rather than the process CWD. Update the `_LEDGER_PATH` initialization
or its accessor to use `devtools.repo_root() /
".cache/verify/merge-gate/merge-train-ledger.json"`, while preserving
`_LEDGER_PENDING_PATH` and `_LEDGER_LOCK_PATH` derivation and keeping any
CWD-dependent tests workable.
- Around line 496-516: Update the add-failure branch in the terminal
verification flow to stop calling _remove_detached_worktree for the unregistered
worktree; use git worktree prune if cleanup is required, then return 1 while
emitting only the accurate materialization failure message.
In `@devtools/merge_gate.py`:
- Around line 521-535: Rename the comprehension variable in the
verification_scope validation within the merge-gating function so it does not
reuse the outer scope symbol holding the PrScopeResult. Use a distinct name such
as item while preserving the existing VerificationScope value set and all
authority checks unchanged.
- Around line 222-256: Consolidate receipt decoding in devtools/merge_gate.py by
adding a shared _structured_receipt parser that performs the JSON and dictionary
validation once, then have _release_baseline_permission, _verification_scope,
and _terminal_authorization read and validate fields from that parsed payload.
Update cmd_record to reuse one parsed receipt, and update merge_boundary.py to
use merge_gate._structured_receipt for git_head instead of parsing stdout
separately. Preserve None for invalid or missing data so authorization remains
fail-closed.
In `@devtools/testmon_bootstrap.py`:
- Line 424: Compute a shared boolean for the attempt-receipt/selection-only
branch in the bootstrap flow, combining decision.main_seed_attempt is not None
with decision.selection_only. Reuse that boolean for both the condition near the
attempt-receipt handling and the seed.json publication condition near
staged_stamp, so both branches stay aligned when main_seed_attempt is absent.
- Line 92: Centralize the testmon protocol version in devtools/testmon_state.py
and import that constant everywhere. Update
devtools/testmon_bootstrap.py#L92-L92 to use it for the dataclass
protocol_version default, tests/unit/devtools/test_testmon_bootstrap.py#L53-L53
to derive PROTOCOL_VERSION from it, and
tests/integration/devtools/test_testmon_seed_recovery.py#L44-L44 to use it for
the attempt protocol_version value.
- Around line 536-539: Update maybe_bootstrap_testmon_seed to accept a
seed-attempt relative-path override alongside testmon_data_relpath and
seed_stamp_relpath, then use that parameter for both local_seed_attempt and
main_seed_attempt instead of hardcoding TESTMON_SEED_ATTEMPT_RELPATH. Preserve
the default production path when no override is provided.
- Around line 567-571: Update the failure message immediately before the
decision.main_seed_attempt block to state that pytest-testmon bootstrap was
refused and left no destination state. Remove the stale wording that claims the
seed was bootstrapped or copied before provenance recording failed; preserve the
surrounding decision and return behavior.
In `@devtools/testmon_state.py`:
- Around line 828-842: Update stamp_from_attempt’s TestmonSeedStamp construction
to pass the existing recorded_data value instead of calling
file_fingerprint(data_path) again. Preserve the recorded fingerprint while
removing the unguarded database read and maintaining the function’s
None-on-failure behavior.
- Around line 532-533: Wrap the sqlite3.connect call in the try block with
contextlib.closing so the connection is explicitly closed after use, while
preserving the existing transaction context-manager behavior. Import closing as
needed and follow the established pattern used by _atomic_copy_sqlite_db.
In `@devtools/verify.py`:
- Around line 2023-2031: Update _git_committed_tree to pass a bounded timeout to
subprocess.run and catch OSError and subprocess.TimeoutExpired, returning None
when git is unavailable or the probe times out. Preserve the existing successful
stdout parsing and nullable behavior.
- Around line 2292-2312: Update _testmon_release_baseline_permission to return
bool rather than bool | None, and revise its docstring to remove the unreachable
None case. Preserve the existing boolean returns for stamp validation, missing
attempts, and derived stamp results.
- Around line 2315-2329: Unify checkout identity with ROOT.resolve() instead of
Path.cwd() in _safe_testmon_artifact_dir() and the related
stamp_from_attempt(..., checkout_root=...) validation paths, including the
existing code at the referenced later location. Continue using Path.cwd() only
when intentionally resolving relative run-artifact paths, and ensure all
artifact containment and receipt checks use the binding’s ROOT checkout.
In `@tests/integration/devtools/test_testmon_seed_recovery.py`:
- Around line 30-40: Update the subprocess setup in the testmon seed-recovery
test to use an isolated environment that removes inherited PYTEST_ADDOPTS while
preserving the required TESTMON_DATAFILE and other necessary variables. Add the
established 10-second timeout to subprocess.run, and improve the return-code
assertion to include the captured child output so hangs, collection failures,
and unexpected failures are reported at this call site.
In `@tests/unit/devtools/test_merge_boundary.py`:
- Around line 788-822: Strengthen
test_concurrent_ledger_writer_cannot_lose_merge_entry by coordinating on
_ledger_lock rather than setting started before _append_merge_entry acquires it:
hold the lock in the main test thread while the mocked subprocess runs, signal
that the writer is attempting the append, assert it remains blocked, then
release the lock before cmd_record_full_verify reaches its own lock acquisition.
Join the writer and retain the existing ledger and status assertions.
In `@tests/unit/devtools/test_merge_gate.py`:
- Around line 111-143: Remove the command parametrization from
test_check_accepts_affected_receipt_without_release_baseline_permission and
test_check_blocks_full_receipt_without_release_baseline_permission, since each
test overwrites the typed receipt fields and produces identical outcomes
regardless of command. Replace the parameterized command argument with a single
representative command while preserving the existing assertions and receipt
setup.
- Around line 146-166: Update _fake_run to return Callable[..., MagicMock],
matching the equivalent helper in test_merge_boundary.py, so callers receive the
correct callable type. Remove the cast around _fake_run in
test_record_consumes_structured_verify_release_permission and delete the
now-unused typing.cast import, keeping Callable imported.
In `@tests/unit/devtools/test_testmon_bootstrap.py`:
- Around line 719-720: Rename the test containing the assertions on
local_data.exists() and local_stamp.exists() to reflect that no copied state is
published when the stamp becomes invalid, following the naming pattern of its
sibling tests.
- Around line 636-637: Update the assertions for
local_payload["binding"]["checkout_root"] and source_checkout_root to compare
against resolved tmp_path-derived paths, matching TestmonSeedStamp.rebound and
the sibling assertions. Preserve the existing suffixes and expected binding
values while applying .resolve() before converting them to strings.
In `@tests/unit/devtools/test_testmon_state.py`:
- Around line 201-208: Update the stale-stamp test around _attempt and
stamp_from_attempt to construct a green stamp with release_baseline_allowed
enabled, following the setup used by
test_attempt_and_green_stamp_artifacts_fail_closed_when_malformed. Assert
validate_stamp succeeds before mutating data, then append the stale bytes and
assert validation returns None, ensuring the fingerprint comparison path is
exercised.
In `@tests/unit/devtools/test_verify.py`:
- Around line 534-537: Update
test_two_interrupted_resumes_flatten_all_carried_outcomes and
test_resumed_seed_does_not_reuse_an_unexecuted_database_row to accept the
monkeypatch fixture, remove their manual pytest.MonkeyPatch construction and
try/finally undo cleanup, and de-indent the affected test bodies while
preserving their behavior.
- Around line 96-135: Update tests/unit/devtools/test_verify.py lines 96-135:
make _write_real_testmon_state accept a root parameter, use it for artifact
paths and _TestmonBinding.checkout_root, and update each caller to pass tmp_path
while patching verify.ROOT. Update lines 2110-2138 in
test_verify_main_types_skip_slow_terminal_authority to call
monkeypatch.chdir(tmp_path) before verification so generated run artifacts
remain temporary; the integration test pattern in
tests/integration/devtools/test_testmon_seed_recovery.py requires no change.
- Around line 438-449: Remove the tautological `"stale" in message` assertion
from test_testmon_preflight_rejects_stale_database_fingerprint and retain only
the non-None rejection assertion plus the empty-stderr check. Apply the same
change to the analogous assertions at the other referenced tests, removing
message-content checks while preserving assertions that preflight rejects and
remains silent.
- Around line 2110-2138: Add the pytest monkeypatch fixture to
test_verify_main_types_skip_slow_terminal_authority and call
monkeypatch.chdir(tmp_path) before invoking main, ensuring VerifyRun writes its
run directory and current-run.json under the temporary path instead of the
repository root. Preserve the existing parameterized assertions and patches.
---
Outside diff comments:
In `@devtools/merge_boundary.py`:
- Line 779: Update the default verify command in merge_p to explicitly include
--json, ensuring subprocess output remains parseable regardless of TTY
allocation. Also update the terminal verification command construction and
related parsing flows in _release_baseline_permission, _verification_scope, and
_terminal_authorization to request JSON explicitly, and distinguish unparseable
output from a valid refusal when reporting authorization results.
- Around line 569-593: Update cmd_merge to roll back the durable intent when gh
pr merge returns a definite nonzero failure: add an intent-abandon helper
alongside _record_merge_intent/_complete_merge_intent, invoke it on the failed
merge path, and return the merge exit code only after attempting cleanup. If
abandoning fails, retain the intent and report that cleanup failed to preserve
fail-closed behavior. Add a regression test alongside
test_external_merge_before_completion_is_reconciled_from_durable_intent
asserting a failed merge leaves merge_intents empty and
cmd_train_status(as_json=False) returns 0.
In `@tests/unit/devtools/test_merge_boundary.py`:
- Around line 596-601: Add the frozen_clock_modules marker for
devtools.merge_boundary and include the frozen_clock fixture in
test_train_status_blocks_when_pr_merged_after_last_full_verify and
test_record_full_verify_clears_pending_prs. Update these timestamp-dependent
tests to use the explicit frozen clock for merge and verification times,
preserving their existing assertions and behavior.
In `@tests/unit/devtools/test_verify.py`:
- Around line 1026-1148: Split
test_seed_completion_requires_full_failure_free_database into five focused tests
covering green completion, authorized narrow-terminal completion, red reuse,
stale-database rejection, and orphan-edge rejection. Keep each test’s setup and
database/events.jsonl mutations isolated by using a module-scoped fixture that
yields a fresh copy of TESTMON_DATA for each case, while preserving the existing
assertions and _finalize_testmon_seed_attempt calls.
🪄 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: 70faf8b9-6e28-4b7b-b1be-32c78e4126de
📒 Files selected for processing (14)
.beads/issues.jsonldevtools/checkout_guard.pydevtools/merge_boundary.pydevtools/merge_gate.pydevtools/testmon_bootstrap.pydevtools/testmon_state.pydevtools/verify.pytests/integration/devtools/test_testmon_seed_recovery.pytests/unit/devtools/test_checkout_guard.pytests/unit/devtools/test_merge_boundary.pytests/unit/devtools/test_merge_gate.pytests/unit/devtools/test_testmon_bootstrap.pytests/unit/devtools/test_testmon_state.pytests/unit/devtools/test_verify.py
| @@ -1,3 +1,7 @@ | |||
| {"_type":"issue","id":"polylogue-d96ta","title":"testmon: run and publish a bounded fresh seed receipt","description":"Execute a fresh seed-testmon run after the concurrency cap, retain the complete selection denominator and resource receipt, and decide whether the result is a complete red baseline, release-eligible baseline, or typed incomplete/resource-timeout outcome.","acceptance_criteria":"1. The fresh seed runs on a clean selected code tree and records the complete expected node universe, selection digest, harness and dependency identity, and actual worker/resource evidence. 2. The run reaches a typed terminal outcome or records an explicit timeout/incomplete receipt; no partial selection can be promoted. 3. The resulting receipt is independently replayable and accepted only by the typed testmon promotion gate. 4. The exact command, resource envelope, result, and residual failure attribution are published for the release ledger.","status":"open","priority":0,"issue_type":"task","owner":"ezo.dev@gmail.com","created_at":"2026-08-10T00:40:19Z","created_by":"Sinity","updated_at":"2026-08-10T00:40:19Z","dependencies":[{"issue_id":"polylogue-d96ta","depends_on_id":"polylogue-817er","type":"discovered-from","created_at":"2026-08-10T00:40:29Z","created_by":"Sinity","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} | |||
| {"_type":"issue","id":"polylogue-817er","title":"testmon: complete bounded fresh seed and release-baseline admission","description":"Finish the testmon seed-harness residual after bounding seed-only concurrency. Prove a fresh seed can complete within the declared resource envelope, retain a complete denominator and provenance, and distinguish a complete red baseline from release eligibility.","acceptance_criteria":"1. A fresh-worktree seed selects the complete expected node universe with a declared denominator and does not silently shrink selection. 2. Seed-testmon uses the bounded worker policy and records actual process/resource/timeout evidence; ordinary adaptive test lanes remain unchanged. 3. A completed seed receipt is bound to exact code tree, dependency/testmon graph, harness version, selection digest, and outcome; incomplete or stale receipts cannot promote. 4. Red baseline, green release baseline, incomplete run, and resource-timeout outcomes are distinct typed states. 5. Focused harness mutation tests and devtools verify --quick pass; a fresh seed attempt is run before closure and its exact result is recorded.","status":"open","priority":0,"issue_type":"task","owner":"ezo.dev@gmail.com","created_at":"2026-08-10T00:39:28Z","created_by":"Sinity","updated_at":"2026-08-10T00:39:28Z","dependencies":[{"issue_id":"polylogue-817er","depends_on_id":"polylogue-mq4vx","type":"discovered-from","created_at":"2026-08-10T00:39:37Z","created_by":"Sinity","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} | |||
| {"_type":"issue","id":"polylogue-ox2iz","title":"reindex: execute and classify the canary changelog report","description":"Execute the repaired daemon-owned canary differ route against the selected deployed package and inactive generation, classify every observed row difference against the declared schema/parser deltas, and attach the reviewed report to the candidate-build phase.","acceptance_criteria":"1. A fresh canary run uses a frozen source snapshot, selected deployed package SHA, inactive no-promote generation, and exact comparator version. 2. The sampled corpus includes each origin, zoo/pathology fixtures, and a declared denominator; selection cannot be shrunk after observation. 3. Every sessions/messages/blocks/session_links/derived difference is classified as expected with a cited delta or unexpected with a named Bead, with zero unclassified rows. 4. The report includes receipts, digests, command identity, and reviewer disposition and is consumed by candidate-build preflight. 5. A red mutation removing a diff or changing the authority binding fails the report gate; no pointer promotion occurs.","status":"open","priority":0,"issue_type":"task","owner":"ezo.dev@gmail.com","created_at":"2026-08-10T00:37:28Z","created_by":"Sinity","updated_at":"2026-08-10T00:37:28Z","dependencies":[{"issue_id":"polylogue-ox2iz","depends_on_id":"polylogue-0x7nh","type":"discovered-from","created_at":"2026-08-10T00:37:38Z","created_by":"Sinity","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Declare that the canary corpus is synthetic-only.
The acceptance criteria require a “sampled corpus” but do not prohibit real session data, /realm/data/... access, or cloud upload. Add an explicit criterion for synthetic fixtures only and record the fixture-set identity in the receipt.
Proposed acceptance criterion
6. The canary uses synthetic fixtures only. It never reads `/realm/data/...` or uploads real user session corpora to a cloud lane. The receipt records fixture-set identity and provenance.
As per coding guidelines, never upload real user session corpora in cloud lanes; use synthetic fixtures only and never access /realm/data/....
🤖 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 @.beads/issues.jsonl at line 3, Add an acceptance criterion to issue
polylogue-ox2iz requiring synthetic fixtures only, prohibiting reads from
/realm/data/... and uploads of real session corpora to cloud lanes. Require the
canary receipt to record the fixture-set identity and provenance while
preserving the existing sampled-corpus requirements.
Source: Coding guidelines
| if payload.get("status") == "complete": | ||
| return attempt_is_checkout_bound( | ||
| payload, | ||
| checkout_root=checkout_root, | ||
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | ||
| ) | ||
| if payload.get("status") == "reusable": | ||
| return attempt_is_checkout_bound( | ||
| payload, | ||
| checkout_root=checkout_root, | ||
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Collapse the two identical status branches.
The complete and reusable branches call attempt_is_checkout_bound with identical arguments. attempt_is_checkout_bound already restricts accepted statuses to {"reusable", "complete"} when reusable_only is the default. One branch expresses the same contract.
♻️ Proposed consolidation
- if payload.get("status") == "complete":
- return attempt_is_checkout_bound(
- payload,
- checkout_root=checkout_root,
- protocol_version=_TESTMON_SEED_PROTOCOL_VERSION,
- )
- if payload.get("status") == "reusable":
+ if payload.get("status") in {"complete", "reusable"}:
return attempt_is_checkout_bound(
payload,
checkout_root=checkout_root,
protocol_version=_TESTMON_SEED_PROTOCOL_VERSION,
)📝 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.
| if payload.get("status") == "complete": | |
| return attempt_is_checkout_bound( | |
| payload, | |
| checkout_root=checkout_root, | |
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | |
| ) | |
| if payload.get("status") == "reusable": | |
| return attempt_is_checkout_bound( | |
| payload, | |
| checkout_root=checkout_root, | |
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | |
| ) | |
| if payload.get("status") in {"complete", "reusable"}: | |
| return attempt_is_checkout_bound( | |
| payload, | |
| checkout_root=checkout_root, | |
| protocol_version=_TESTMON_SEED_PROTOCOL_VERSION, | |
| ) |
🤖 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/checkout_guard.py` around lines 287 - 298, Consolidate the duplicate
status handling around attempt_is_checkout_bound by using one condition that
accepts both "complete" and "reusable" statuses, then perform the existing call
once with the same arguments. Preserve the default reusable_only behavior and
leave other statuses on their existing path.
| _LEDGER_PATH = Path(".cache/verify/merge-gate/merge-train-ledger.json") | ||
| _LEDGER_PENDING_PATH = _LEDGER_PATH.with_name(f"{_LEDGER_PATH.name}.pending") | ||
| _LEDGER_LOCK_PATH = _LEDGER_PATH.with_name(f"{_LEDGER_PATH.name}.lock") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the merge-boundary ledger path is resolved and how tests depend on CWD.
set -euo pipefail
echo "=== ledger path definitions and uses ==="
rg -n -C3 '_LEDGER_PATH|_LEDGER_PENDING_PATH|_LEDGER_LOCK_PATH' devtools/merge_boundary.py
echo "=== repo_root helper ==="
rg -n -C5 'def repo_root' devtools/__init__.py
echo "=== tests relying on chdir for ledger placement ==="
rg -n -c 'monkeypatch.chdir' tests/unit/devtools/test_merge_boundary.py
echo "=== how merge_boundary is invoked from the command catalog ==="
rg -n -C3 'merge_boundary|workspace merge' devtools/command_catalog.py devtools/cli*.py 2>/dev/null || trueRepository: Sinity/polylogue
Length of output: 7664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== devtools/__init__.py ==="
cat -n devtools/__init__.py
echo "=== devtools/merge_boundary.py imports and relevant functions ==="
sed -n '1,160p' devtools/merge_boundary.py | cat -n
sed -n '190,240p' devtools/merge_boundary.py | cat -n
sed -n '360,430p' devtools/merge_boundary.py | cat -n
sed -n '620,680p' devtools/merge_boundary.py | cat -n
echo "=== invoke/workspace merge command definitions ==="
rg -n -C4 'devtools\.merge_boundary|merge_boundary|train-status|workspace merge|workspace.*merge' devtools tests/unit/devtools/test_merge_boundary.py | sed -n '1,240p'
echo "=== deterministic cwd probe for Path resolution and _read_ledger_default semantics ==="
python3 - <<'PY'
from pathlib import Path
import textwrap, importlib.util, sys
# Show Path behavior at different cwd values without importing repo code.
ledger = Path(".cache/verify/merge-gate/merge-train-ledger.json")
for base in [Path("/tmp/probe"), Path("/repo/root")]:
print({str(base / ledger): str(base / ".cache/verify/merge-gate/merge-train-ledger.json")})
# Read ledger function source to confirm default value and tests around it.
spec = importlib.util.spec_from_file_location("merge_boundary", "devtools/merge_boundary.py")
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
print("return_default_on_missing=", module._read_ledger_unlocked.__doc__ or "no-doc")
print("ledger_paths:", module._LEDGER_PATH, module._LEDGER_PENDING_PATH, module._LEDGER_LOCK_PATH)
PYRepository: Sinity/polylogue
Length of output: 36323
Anchor the merge-train ledger to the repository root.
_LEDGER_PATH is Path(".cache/verify/merge-gate/merge-train-ledger.json"), so cmd_train_status reads the ledger relative to process CWD. Running from a subdirectory hits the missing-path default, _pending_prs_since_last_full_verify(), returns [], and the gate reports train OK even when the repository ledger contains merged PRs after the last full-suite verify. The lock and pending paths derive from this path, so anchor it once, e.g. with devtools.repo_root() / ".cache/..." inside an accessor to keep CWD-dependent tests working.
🤖 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/merge_boundary.py` around lines 83 - 85, The merge-train ledger
paths must be rooted at the repository rather than the process CWD. Update the
`_LEDGER_PATH` initialization or its accessor to use `devtools.repo_root() /
".cache/verify/merge-gate/merge-train-ledger.json"`, while preserving
`_LEDGER_PENDING_PATH` and `_LEDGER_LOCK_PATH` derivation and keeping any
CWD-dependent tests workable.
| def _write_real_testmon_state(nodeids: tuple[str, ...] = ("tests/test_a.py::test_one",)) -> Path: | ||
| TESTMON_DATA.parent.mkdir(parents=True, exist_ok=True) | ||
| with sqlite3.connect(TESTMON_DATA) as conn: | ||
| conn.execute("CREATE TABLE environment (id INTEGER PRIMARY KEY, environment_name TEXT)") | ||
| conn.execute("CREATE TABLE file_fp (id INTEGER PRIMARY KEY, filename TEXT, fsha TEXT)") | ||
| conn.execute("CREATE TABLE test_execution (id INTEGER PRIMARY KEY, test_name TEXT, failed INTEGER)") | ||
| conn.execute("CREATE TABLE test_execution_file_fp (test_execution_id INTEGER, fingerprint_id INTEGER)") | ||
| for index, nodeid in enumerate(nodeids, start=1): | ||
| conn.execute("INSERT INTO file_fp(id, filename, fsha) VALUES (?, ?, ?)", (index, nodeid, f"sha-{index}")) | ||
| conn.execute("INSERT INTO test_execution(id, test_name, failed) VALUES (?, ?, 0)", (index, nodeid)) | ||
| conn.execute("INSERT INTO test_execution_file_fp VALUES (?, ?)", (index, index)) | ||
| stamp = _TestmonSeedStamp( | ||
| TESTMON_SEED_PROTOCOL_VERSION, | ||
| CollectionStatus.COMPLETE, | ||
| nodeids, | ||
| 0, | ||
| BaselineStatus.GREEN, | ||
| True, | ||
| 0, | ||
| GraphInspection(GraphStatus.COMPLETE, len(nodeids), len(nodeids), (), 0, 0, None, ()), | ||
| _TestmonIdentity("current-head", "covered", "python", True, False, None, "narrow-terminal"), | ||
| _TestmonBinding(BindingMode.EXACT, str(ROOT.resolve())), | ||
| file_fingerprint(TESTMON_DATA), | ||
| "seed", | ||
| ".cache/verify/runs/seed", | ||
| ) | ||
| TESTMON_SEED_STAMP.parent.mkdir(parents=True, exist_ok=True) | ||
| artifact_dir = ROOT / ".cache" / "verify" / "runs" / "seed" | ||
| artifact_dir.mkdir(parents=True, exist_ok=True) | ||
| (artifact_dir / "run.json").write_text( | ||
| json.dumps( | ||
| { | ||
| "run_id": "seed", | ||
| "checkout_root": str(ROOT.resolve()), | ||
| "artifact_dir": ".cache/verify/runs/seed", | ||
| } | ||
| ) | ||
| ) | ||
| TESTMON_SEED_STAMP.write_text(json.dumps(stamp.as_dict())) | ||
| return TESTMON_DATA |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Unit tests write verification state into the real repository checkout. Both sites resolve paths against the real ROOT imported from devtools.verify instead of tmp_path, so artifacts escape the temporary directory, persist after the run, and are shared across parallel xdist workers.
tests/unit/devtools/test_verify.py#L96-L135:_write_real_testmon_statebuildsartifact_dir = ROOT / ".cache" / "verify" / "runs" / "seed"and binds the stamp tostr(ROOT.resolve()). The write is load-bearing —TestmonSeedStamp.from_mappingcalls_is_bound_run_artifactagainstbinding.checkout_root, so the receipt must exist underROOTforvalidate_stampto accept it. Take the root as a parameter, bind the stamp to it, and have each caller passtmp_pathwhile patchingverify.ROOT.tests/unit/devtools/test_verify.py#L2110-L2138:test_verify_main_types_skip_slow_terminal_authoritynever callsmonkeypatch.chdir, so_anchor_verification_paths()sets the CWD toROOTandVerifyRun.__init__creates.cache/verify/runs/<run_id>/run.jsonplus.cache/verify/current-run.jsonin the working tree. Addmonkeypatch.chdir(tmp_path);_anchor_verification_paths()returns early outsideROOT, so the run stays contained.
tests/integration/devtools/test_testmon_seed_recovery.py already demonstrates the correct pattern with monkeypatch.setattr(verify, "ROOT", lane).
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 125-131: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"run_id": "seed",
"checkout_root": str(ROOT.resolve()),
"artifact_dir": ".cache/verify/runs/seed",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 133-133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(stamp.as_dict())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
📍 Affects 1 file
tests/unit/devtools/test_verify.py#L96-L135(this comment)tests/unit/devtools/test_verify.py#L2110-L2138
🤖 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 96 - 135, Update
tests/unit/devtools/test_verify.py lines 96-135: make _write_real_testmon_state
accept a root parameter, use it for artifact paths and
_TestmonBinding.checkout_root, and update each caller to pass tmp_path while
patching verify.ROOT. Update lines 2110-2138 in
test_verify_main_types_skip_slow_terminal_authority to call
monkeypatch.chdir(tmp_path) before verification so generated run artifacts
remain temporary; the integration test pattern in
tests/integration/devtools/test_testmon_seed_recovery.py requires no change.
| def test_testmon_preflight_rejects_stale_database_fingerprint( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] | ||
| ) -> None: | ||
| monkeypatch.chdir(tmp_path) | ||
| TESTMON_DATA.parent.mkdir(parents=True) | ||
| TESTMON_DATA.write_text("seeded") | ||
| seed_stamp = tmp_path / ".cache" / "testmon" / "seed.json" | ||
| seed_stamp.parent.mkdir(parents=True, exist_ok=True) | ||
| seed_stamp.write_text( | ||
| json.dumps( | ||
| { | ||
| "protocol_version": TESTMON_SEED_PROTOCOL_VERSION, | ||
| "status": "complete", | ||
| "git_head": "old-head", | ||
| "testmon_data": hashlib.sha256(b"seeded").hexdigest(), | ||
| } | ||
| ) | ||
| ) | ||
| monkeypatch.setattr("devtools.verify._git_head", lambda: "current-head") | ||
| _write_real_testmon_state() | ||
| TESTMON_DATA.write_bytes(TESTMON_DATA.read_bytes() + b"stale") | ||
|
|
||
| message = _testmon_preflight(seed_testmon=False, full_pytest=False, quick=False, commit=False) | ||
|
|
||
| assert message is None | ||
| assert "different git head" in capsys.readouterr().err | ||
| assert message is not None | ||
| assert "stale" in message | ||
| assert capsys.readouterr().err == "" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
These assertions are tautological against the single rejection message.
_testmon_preflight returns one fixed string for every rejection: "unreadable, stale, malformed, or not graph-complete". The assertion "stale" in message therefore passes for any cause, including a malformed marker or an incomplete graph. The same applies to "stale" in message or "malformed" in message at Line 472 and Line 495.
The tests still prove that rejection occurred and that stderr stays silent, which is their main value. But test_testmon_preflight_rejects_stale_database_fingerprint cannot fail if the fingerprint check is removed, provided some other check still rejects. Asserting message is not None states the same guarantee honestly.
If distinguishing the causes matters, _testmon_preflight would need to emit a cause-specific message. That is a production change and out of scope here.
🤖 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 438 - 449, Remove the
tautological `"stale" in message` assertion from
test_testmon_preflight_rejects_stale_database_fingerprint and retain only the
non-None rejection assertion plus the empty-stderr check. Apply the same change
to the analogous assertions at the other referenced tests, removing
message-content checks while preserving assertions that preflight rejects and
remains silent.
| def test_two_interrupted_resumes_flatten_all_carried_outcomes(tmp_path: Path) -> None: | ||
| monkeypatch = pytest.MonkeyPatch() | ||
| monkeypatch.chdir(tmp_path) | ||
| try: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use the monkeypatch fixture instead of constructing pytest.MonkeyPatch() manually.
test_two_interrupted_resumes_flatten_all_carried_outcomes and test_resumed_seed_does_not_reuse_an_unexecuted_database_row build pytest.MonkeyPatch() by hand and wrap the body in try / finally: monkeypatch.undo(). Every other test in this module takes the monkeypatch fixture, which performs the same teardown automatically and removes the indentation level.
The manual form is correct. It is only inconsistent with the surrounding file.
♻️ Proposed simplification
-def test_two_interrupted_resumes_flatten_all_carried_outcomes(tmp_path: Path) -> None:
- monkeypatch = pytest.MonkeyPatch()
- monkeypatch.chdir(tmp_path)
- try:
- expected = ["tests/test_a.py::test_one", "tests/test_b.py::test_two"]
+def test_two_interrupted_resumes_flatten_all_carried_outcomes(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ monkeypatch.chdir(tmp_path)
+ expected = ["tests/test_a.py::test_one", "tests/test_b.py::test_two"]Then de-indent the body and drop the finally: monkeypatch.undo() block.
As per coding guidelines for tests/**/*.py, I also checked the clock rule: no added test reads datetime.now or time.time directly, and no assertion depends on a timestamp value, so frozen_clock is not required here.
Also applies to: 666-669
🤖 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 534 - 537, Update
test_two_interrupted_resumes_flatten_all_carried_outcomes and
test_resumed_seed_does_not_reuse_an_unexecuted_database_row to accept the
monkeypatch fixture, remove their manual pytest.MonkeyPatch construction and
try/finally undo cleanup, and de-indent the affected test bodies while
preserving their behavior.
Source: Coding guidelines
| @pytest.mark.parametrize( | ||
| ("argv", "expected_scope", "expected_permission"), | ||
| [ | ||
| (["--all", "--skip-slow"], "narrow-terminal", False), | ||
| (["--all", "--skip-slow", "--terminal-authorization", "narrow-terminal"], "narrow-terminal", True), | ||
| ], | ||
| ) | ||
| def test_verify_main_types_skip_slow_terminal_authority( | ||
| capsys: pytest.CaptureFixture[str], argv: list[str], expected_scope: str, expected_permission: bool | ||
| ) -> None: | ||
| def fake_run(label: str, command: list[str], **kwargs: object) -> tuple[int, float, dict[str, object]]: | ||
| del label, command, kwargs | ||
| return 0, 0.01, {} | ||
|
|
||
| with ( | ||
| patch("devtools.verify._run", side_effect=fake_run), | ||
| patch("devtools.verify.build_verify_steps", return_value=[("pytest full", ["pytest"])]), | ||
| patch("devtools.verify._git_head", return_value="head"), | ||
| patch("devtools.verify._save_history"), | ||
| patch("devtools.verify._stamp_head"), | ||
| patch("devtools.verify._notify"), | ||
| ): | ||
| assert main([*argv, "--json"]) == 0 | ||
|
|
||
| payload = json.loads(capsys.readouterr().out) | ||
| assert payload["verification_scope"] == expected_scope | ||
| assert payload["release_baseline_allowed"] is expected_permission | ||
| assert payload["terminal_authorization"] == ("narrow-terminal" if expected_permission else None) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test writes a run directory into the real repository.
The test does not chdir to a temporary directory. main() calls _anchor_verification_paths(), which sets the CWD to ROOT. VerifyRun.__init__ then runs self.run_dir.mkdir(parents=True, exist_ok=True) and self.write() against self.root = Path.cwd(), which is now the real repository root.
Each parameterized case therefore creates .cache/verify/runs/<run_id>/run.json in the working tree and overwrites .cache/verify/current-run.json. _save_history and _stamp_head are patched, but VerifyRun is not.
Overwriting current-run.json is the part that reaches beyond this test. devtools/verify_runs.py guards that file with _current_owner_is_other_live_run, so a concurrent real verify run is protected, but a stale marker is still left behind.
Add monkeypatch.chdir(tmp_path) so VerifyRun writes inside the temporary directory. Note that _anchor_verification_paths() returns early when the CWD is outside ROOT, so the anchoring will not pull the run back into the repository.
🧪 Proposed isolation fix
def test_verify_main_types_skip_slow_terminal_authority(
- capsys: pytest.CaptureFixture[str], argv: list[str], expected_scope: str, expected_permission: bool
+ capsys: pytest.CaptureFixture[str],
+ monkeypatch: pytest.MonkeyPatch,
+ tmp_path: Path,
+ argv: list[str],
+ expected_scope: str,
+ expected_permission: bool,
) -> None:
+ monkeypatch.chdir(tmp_path)
+
def fake_run(label: str, command: list[str], **kwargs: object) -> tuple[int, float, dict[str, object]]:📝 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.
| @pytest.mark.parametrize( | |
| ("argv", "expected_scope", "expected_permission"), | |
| [ | |
| (["--all", "--skip-slow"], "narrow-terminal", False), | |
| (["--all", "--skip-slow", "--terminal-authorization", "narrow-terminal"], "narrow-terminal", True), | |
| ], | |
| ) | |
| def test_verify_main_types_skip_slow_terminal_authority( | |
| capsys: pytest.CaptureFixture[str], argv: list[str], expected_scope: str, expected_permission: bool | |
| ) -> None: | |
| def fake_run(label: str, command: list[str], **kwargs: object) -> tuple[int, float, dict[str, object]]: | |
| del label, command, kwargs | |
| return 0, 0.01, {} | |
| with ( | |
| patch("devtools.verify._run", side_effect=fake_run), | |
| patch("devtools.verify.build_verify_steps", return_value=[("pytest full", ["pytest"])]), | |
| patch("devtools.verify._git_head", return_value="head"), | |
| patch("devtools.verify._save_history"), | |
| patch("devtools.verify._stamp_head"), | |
| patch("devtools.verify._notify"), | |
| ): | |
| assert main([*argv, "--json"]) == 0 | |
| payload = json.loads(capsys.readouterr().out) | |
| assert payload["verification_scope"] == expected_scope | |
| assert payload["release_baseline_allowed"] is expected_permission | |
| assert payload["terminal_authorization"] == ("narrow-terminal" if expected_permission else None) | |
| `@pytest.mark.parametrize`( | |
| ("argv", "expected_scope", "expected_permission"), | |
| [ | |
| (["--all", "--skip-slow"], "narrow-terminal", False), | |
| (["--all", "--skip-slow", "--terminal-authorization", "narrow-terminal"], "narrow-terminal", True), | |
| ], | |
| ) | |
| def test_verify_main_types_skip_slow_terminal_authority( | |
| capsys: pytest.CaptureFixture[str], | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path: Path, | |
| argv: list[str], | |
| expected_scope: str, | |
| expected_permission: bool, | |
| ) -> None: | |
| monkeypatch.chdir(tmp_path) | |
| def fake_run(label: str, command: list[str], **kwargs: object) -> tuple[int, float, dict[str, object]]: | |
| del label, command, kwargs | |
| return 0, 0.01, {} | |
| with ( | |
| patch("devtools.verify._run", side_effect=fake_run), | |
| patch("devtools.verify.build_verify_steps", return_value=[("pytest full", ["pytest"])]), | |
| patch("devtools.verify._git_head", return_value="head"), | |
| patch("devtools.verify._save_history"), | |
| patch("devtools.verify._stamp_head"), | |
| patch("devtools.verify._notify"), | |
| ): | |
| assert main([*argv, "--json"]) == 0 | |
| payload = json.loads(capsys.readouterr().out) | |
| assert payload["verification_scope"] == expected_scope | |
| assert payload["release_baseline_allowed"] is expected_permission | |
| assert payload["terminal_authorization"] == ("narrow-terminal" if expected_permission 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 `@tests/unit/devtools/test_verify.py` around lines 2110 - 2138, Add the pytest
monkeypatch fixture to test_verify_main_types_skip_slow_terminal_authority and
call monkeypatch.chdir(tmp_path) before invoking main, ensuring VerifyRun writes
its run directory and current-run.json under the temporary path instead of the
repository root. Preserve the existing parameterized assertions and patches.
Problem: a markerless complete attempt could recreate release authority, and repeated interrupted resumes could discard outcomes carried by an earlier resume.\n\nWhat changed: markerless attempt consumers now force selection-only state, and resume preparation flattens prior and current outcome ledgers before writing the next attempt. Real bootstrap, state-parser, and two-interruption regressions cover both boundaries.\n\nCompatibility/migration: published seed markers retain their existing typed release contract; only unmarked attempt recovery is downgraded.
Problem: detached terminal verification inherited the feature checkout interpreter, train-ledger corruption could fail open, concurrent merges could be overwritten or covered, and manual recording accepted stale branch output.\n\nWhat changed: terminal commands run through the detached checkout's direnv environment, ledger reads fail closed, writes use a durable pending latch and fsync-backed replacement, verification records its start sequence and merged-master SHA, and manual recording fetches and binds the current default branch. Production-route regressions cover the guard, corruption, injected write failure, concurrent merge, and stale CLI route.\n\nCompatibility/migration: existing ledger entries without sequence fields remain readable and are conservatively compared by timestamp; future terminal receipts require an exact fetched target.
Problem: terminal verification and external merge bookkeeping still had snapshot races, unlocked ledger writers, an untracked post-merge crash window, and inconsistent markerless seed handling. What changed: serialize ledger transactions, persist and reconcile pre-merge intents, snapshot ledger state before fetching the default branch, validate all status fields, accept typed markerless selection attempts through the checkout guard, and fail explicitly on detached worktree cleanup errors. Production-route regressions cover each authority transition and race. Compatibility/migration: legacy merge entries without merge_sequence remain readable. Existing terminal receipts must contain the typed status fields consumed by train-status; malformed or partial receipts now refuse clean status.
Problem: seed-testmon discarded its declared four-worker bound and could start the full adaptive pool. The recorded resume reached 85 processes, 9.7 GiB PSS, 4.5 GiB swap PSS, and the 2,700-second containment timeout. What changed: retain adaptive sizing for ordinary pytest lanes, but cap only seed-testmon at four workers. The focused harness now proves a 12-worker adaptive result is bounded to four on the production command path.
Problem: seed-testmon caps the executed pytest command at four workers, while workload receipts recorded the uncapped adaptive policy. This made resource evidence disagree with the process that actually ran. What changed: derive workload receipt concurrency from the final pytest command. A mutation test proves an uncapped twelve-worker policy records four for a capped seed command.
Problem: read-only graph inspection left SQLite connections open, while stamp creation re-read a database after it had already proved its fingerprint. What changed: close inspection connections deterministically and retain the validated fingerprint in the resulting stamp. A focused mutation test proves stamp construction makes one fingerprint read.
Bootstrap now checks local markerless attempts against their SQLite graph and fingerprint, and it carries the caller's attempt relpath through both roots. Publication uses one selection-only condition so it cannot discard a staged marker under a divergent future decision.
Assert the non-null decision field in the selection-only publication branch so the static type contract matches the branch predicate.
8959bfc to
5bcbb41
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bcbb41c7e
ℹ️ 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".
| "environment": { | ||
| key: os.environ.get(key) for key in ("PYTEST_ADDOPTS", "PYTEST_DISABLE_PLUGIN_AUTOLOAD", "PYTEST_PLUGINS") | ||
| }, |
There was a problem hiding this comment.
Fresh evidence beyond the dependency-version issue is that this harness fingerprint allowlist still omits HYPOTHESIS_PROFILE and POLYLOGUE_CI, even though tests/conftest.py uses them to select profiles ranging from 5/10 to 100 examples. A seed created under the weaker profile can therefore bootstrap into a lane requesting the stronger profile, pass runtime validation, and grant release permission after a zero-selection run without exercising the additional property examples. Include these test-behavior environment inputs in the harness identity. The documented cloud lane explicitly sets HYPOTHESIS_PROFILE=ci, so this cross-environment path is supported rather than hypothetical.
AGENTS.md reference: AGENTS.md:L562-L567
Useful? React with 👍 / 👎.
|
@circleci run |
1 similar comment
|
@circleci run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d95500457
ℹ️ 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".
| ) | ||
| else: | ||
| verification_scope = VerificationScope.AFFECTED | ||
| release_baseline_allowed = _testmon_release_baseline_permission() |
There was a problem hiding this comment.
When affected mode is invoked as devtools verify --skip-slow (including through the merge wrapper's custom --command), pytest explicitly deselects slow tests, but this branch copies the green seed's release permission unchanged and emits verification_scope: affected with release_baseline_allowed: true. A change whose affected coverage is slow-only can therefore produce an authoritative zero-exit receipt without running those tests; even requiring affected receipts to carry true permission would not close this path. Make skip-slow affected runs non-releasable unless an equivalent typed narrow-terminal authorization is supported and supplied.
AGENTS.md reference: AGENTS.md:L400-L409
Useful? React with 👍 / 👎.
| _warn_low_memory() # check again right before the heavy step | ||
| rc, elapsed, metadata = _run(label, cmd, run=verify_run) | ||
| if rc == 0 and label in {"pytest testmon", "pytest testmon (broad)"}: | ||
| raw_stamp = _read_json_artifact(TESTMON_SEED_STAMP) |
There was a problem hiding this comment.
When an ordinary affected run finds a failing test, pytest-testmon updates testmondata with the new failed execution, but this success-only branch does not refresh or replace the green seed receipt. _refresh_testmon_selection_attempt also ignores the surviving complete attempt because it still has release permission, so the next default devtools verify rejects the changed database fingerprint during preflight and requires a full seed instead of rerunning the affected failure. Downgrade a completed failed affected run to validated selection-only state when its graph remains complete.
AGENTS.md reference: AGENTS.md:L325-L327
Useful? React with 👍 / 👎.
1 participant
Summary
Harden testmon provenance and merge-train terminal verification at the current head.
Problem
Reusable testmon data could outlive the dependency environment that produced it. Merge-train verification also rejected explicitly authorized narrow terminal runs, latched valid pending ledger writes after interruption, and could snapshot before reconciling a completed merge intent. The previous exact-head terminal receipt exited 124 with
release_baseline_allowed: falseafter the harness stopped the full pytest run for temporary-storage pressure.Solution
Verification
devtools test tests/unit/devtools/test_merge_boundary.py tests/unit/devtools/test_testmon_state.py tests/unit/devtools/test_testmon_bootstrap.py tests/unit/devtools/test_verify.py -k 'not test_lab_verify_runs_every_registered_lab_policy_command':174 passed, 1 deselected in 18.93s.devtools verify --quick: exit 0 atbe4b28c9.verify --lab; this repair does not alter those registrations or the test assertion.External hold
This PR is not merge-ready until a fresh exact-head, successful typed terminal receipt and the required CI checks are available.
Bead disposition matrix
polylogue-mq4vxpolylogue-817erbe4b28c9, focused harness coverage, quick gatepolylogue-d96ta