[mashkovd]

Part of mctlhq/.github#57 — step E1. Pairs with the WorkflowTemplate in
mctlhq/mctl-gitops#1249.

Stacked on #378 (D2/D3/D4), which it imports _dev_loop_owns_answer and the
LEGACY_* constants from. Review that one first; GitHub will retarget this to main when
it merges.

This code briefly appeared inside #378 — swept in by a git add -A, under a commit
titled about shadow.py. It is out of there and here, with its own review. The three P2s
found while it was misfiled are all fixed below.

Why

Before the observe soak starts, the store has to hold a row for every entity the old
mechanism already knows about. Otherwise every comparison reads
store-permits-old-forbids — the one dangerous class — and the measurement is swamped
by its own migration.

Why not an mctl-api endpoint

The import rules need .status.yaml from the gitops checkout, _service_mode /
SHEPHERD_SKIP_SERVICES, and the pr_url fallback. None of those exist in mctl-api, and
moving them there would put a second copy of the shepherd's policy in a repository that
cannot see the state it applies to.

The three safety properties

Read first, and any UNKNOWN aborts the whole run with nothing written. An id whose
owner cannot be seen is not one to write an owner over. It aborts the run, not the id: a
store that failed on one id was failing, and the rest of the batch is not more trustworthy
for having answered.

Idempotent on two legs. The read-first pass reports an entity that already has a row as
already-owned and never acquires it; and owner ids are deterministic
(shepherd:{service}, never a pod name), so a re-run that races that pass writes the same
owner instead of colliding with itself.

Ambiguity is reported, not resolved. ADR-010 asks bootstrap to write a conflicted
state; types.py defines four states and says there deliberately is no such thing, because
it would be a derived condition needing something to sweep and write it. An entity no
rule covers is left UNOWNED and listed under ambiguous — which is the old mechanism still
driving it, i.e. the status quo, not a new risk.

Two defects that lived in the discovery layer

Both found by review, and both invisible to every test that built ProposalRefs by hand —
which was all of them. There are now tests that drive main() against a tmp_path state
dir.

1. A dry run rewrote the gitops checkout. _discover_refs writes .status.yaml when it
   finds a PR by branch — flipping in-progress → implemented — before the store is ever
   read. It is called with dry_run=True now, always, including under --apply: this tool
   writes ownership rows, never gitops files, and a "dry run" that edited the checkout would
   contradict the aborted path's own "nothing was written".
2. The pr-steward rung was dead code. It fires iff the service is SKIP, which is
   exactly the set _discover_refs drops. fix_only=True is what lets those services be
   discovered at all; the override affects discovery only, so policy.default_owner_for
   still reads the real service mode and still answers pr-steward.

Also: counts covered planned only, so counts["ambiguous"] was structurally 0 beside a
populated ambiguous list — and total, which the soak's sample target is re-derived from,
excluded those entities entirely.

Smaller things worth a look

• The liveness probe runs in the sweep's own pool under the sweep's own budget. Serially it
  is one HTTP call per proposal between the store read and the decisions made from it,
  and that gap is where a bootstrap goes wrong. A ref the budget did not answer is
  ambiguous, never a decision.
• A DevLoop row carries temporal_workflow_id, as the workflow's own acquire does.
• An empty workflow id can never become a row: dev_loop.py's bool(result.owner_id)
  guards exist because a record owned by nobody withholds the entity from everyone and
  names no one to ask.

Verification

pytest tests/test_lifecycle_bootstrap.py    # 22 tests, incl. 7 through main()
pytest tests/                               # 1300 passed
ruff check orchestrator/ tests/

tests/test_proc.py fails on this machine for an unrelated, pre-existing reason — it fails
identically on main.

Then, once released and the WorkflowTemplate's agent_image is bumped: a dry_run=true
submission whose report an operator reads row by row against
mctl_get_lifecycle_ownership and live Temporal.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC

[claude]

Claude finished @mashkovd's task in 6m 34s —— View job

---

Code review complete

• Read repo conventions (LLMS.md, pyproject.toml lint/type config)
• Read the pinned diff (4b4b691...374f6ab)
• Read surrounding modules (contract.py, client.py, policy.py, rollout.py, run_shepherd._discover_refs / _dev_loop_owns_answer / _service_mode, dev_loop.py)
• Analyze correctness, safety and test coverage
• Post inline findings and verdict

Verdict: changes requested — 2 P2, 5 P3. Findings are posted as inline comments on the diff.

P2

• bootstrap.py:367 — probe_all is given every ref, including entities the store already holds, whose probe answer plan_for discards at rung 1. The 60 s DEV_LOOP_LIVENESS_BUDGET_S is shared wall-clock, so already-decided refs crowd out the ones whose decision actually depends on the answer; those fall through to LEGACY_UNKNOWN → ambiguous → left UNOWNED, which is the store-permits-old-forbids class this tool exists to eliminate. On a re-run — the idempotence path the description sells — the whole budget goes to answers that are thrown away.
• bootstrap.py:436 — --apply writes unconditionally, bypassing rollout.records_writes(), the switch every other writer in the package honours (orchestrator/temporal/activities/lifecycle.py:209). At LIFECYCLE_ROLLOUT_MODE=off the DevLoop rows written here are never heartbeated or released by the workflow they name, because that workflow's own ownership activity is skipped — leaving orphan active rows that block the shepherd once the mode advances. Gate it, or state the required ordering in the docstring and the WorkflowTemplate.

P3

• probe_all:290 — future.result() re-raises through the except FuturesTimeoutError; unlike _filter_dev_loop_owned, probe is a parameter here, so the broad-except guarantee is the caller's.
• Plan.as_dict:87 — omits proposal_ref, policy_ref, temporal_workflow_id, three of the six fields apply_report writes, so the dry-run report is not diffable field-for-field against the store.
• build_report:372 — or OwnershipAnswer(verdict=UNOWNED) is unreachable and defaults the opposite way to the identical expression at line 355 that drives the abort.
• main:439 — a run that classified everything ambiguous exits 0; a one-line summary to stderr would make the Argo log readable.
• owner_for:145 — pr-steward:{owner}/{repo} (from parsed URL text) vs shepherd:{service}; nothing in this repo pins the id format the actual pr-steward will use.
• tests/test_lifecycle_bootstrap.py:184 — no test that main() exits 1 when acquires are refused.

What checks out

The three fixes claimed in the description verify against the source: dry_run=True does suppress update_status_file; fix_only=True with no service_filter does force every service past the SKIP drop while policy.default_owner_for still reads the real mode via _service_mode(service); and counts now spans both lists so total includes the ambiguous entities. Read-first-then-abort-on-any-UNKNOWN, the tri-state probe at rung 2, the empty-workflow-id guard, and the deterministic owner ids are all correct. OWNER_DEVLOOP_WORKFLOW matches dev_loop.OWNER_TYPE, and _devloop_workflow_id derives the same id the probe queries.

Not verified

I could not execute pytest or ruff in this environment — both were blocked by command approval — so the "22 tests / 1300 passed / ruff clean" claims in the description are unconfirmed here. This review is static analysis only.
· branch feat/lifecycle-bootstrap

[claude]

Claude finished @mashkovd's task in 7m 32s —— View job

---

Review complete

• Read repo conventions
• Read the pinned diff (0ba4677...400e054)
• Read surrounding modules (contract.py, client.py, policy.py, run_shepherd._discover_refs / _dev_loop_owns_answer / _parse_pr_url, dev_loop.py)
• Analyze correctness / safety / test coverage
• Post inline findings and verdict

Verdict: changes requested — 2 P2, 4 P3. Neither P2 is a data-safety defect; nothing is written on either path.

What holds

The three stated safety properties check out against the surrounding code, not just the docstrings:

• Read-first abort. The UNKNOWN scan uses (answers.get(id) or OwnershipAnswer()).verdict, and OwnershipAnswer's default verdict is UNKNOWN — so a missing id also aborts. Fails closed in both directions.
• Idempotence, both legs. already-owned is skipped in apply_report, and the owner ids are deterministic. More importantly the ids match the other writer: dev_loop.py:1267-1279 sends the same kind, phase, owner_type and owner_id=info.workflow_id, with proposal_ref in the same {service}/{slug} form (dev_loop.py:2186). A bootstrap-written DevLoop row is the row the workflow's own acquire would have produced, so the workflow re-acquires its own row instead of reading OWNED_BY_OTHER and standing down.
• The two discovery fixes are real. _discover_refs(dry_run=True) is what suppresses the update_status_file call at run_shepherd.py:1073; and fix_only=True with service_filter=None makes force_fix_only true for every service (run_shepherd.py:1029), which is the only way a SKIP service survives the drop at line 1037. policy.default_owner_for calls _service_mode(service) without the override, so the ladder still sees the real mode.
• The ladder's LEGACY_UNKNOWN-before-LEGACY_OWNED ordering is the right call, and verdict_for never returns OWNED_BY_ME here (asking=None), so the already-owned rung is effectively the OWNED_BY_OTHER test.

Findings

	File	What
P2	bootstrap.py:290	future.result() is unguarded: a raising probe aborts the run with no report, and a builtin TimeoutError is misattributed to the budget (they are the same class since 3.11) and silently drops already-completed futures. Both contradict this function's own "an unanswered probe must not become a decision".
P2	bootstrap.py:258	_devloop_workflow_id is a fourth copy of the workflow-id derivation and the only one written into a durable record as owner_id. "The id recorded here is the one the probe just confirmed alive" is coincidental rather than structural, and no test pins it against _dev_loop_owns_answer's copy. Drift hazard, not a present bug.
P3	bootstrap.py:122	The abort path still emits a full counts block computed from a partial scan — including total, the number the soak's sample target is re-derived from.
P3	bootstrap.py:347	Zero discovery exits 0 and looks identical to a successful import — the one outcome that should not.
P3	bootstrap.py:232	Both halves of rung 4's guard are unconditionally true for anything _discover_refs produces, so rung 5 is unreachable in production.
P3	test_lifecycle_bootstrap.py:396	The 0.2s probe budget makes answers.get(0) == LEGACY_FREE a wall-clock race on a loaded runner.

Not run

I did not execute pytest or ruff — both need approval beyond this job's allowlist, so the PR's "22 tests / 1300 passed / ruff clean" claims are unverified here. Every finding above is from static reading of the pinned diff plus the modules it imports.
· feat/lifecycle-bootstrap

[claude]

P2: the liveness probe runs for every ref, including the ones the store already holds — whose probe answer plan_for then discards at rung 1 (already-owned returns before legacy_answer is read).

Two consequences, both on the axis this file is most careful about:

1. On a re-run — the idempotence path the PR sells, where every entity is already-owned — 100% of the 60 s DEV_LOOP_LIVENESS_BUDGET_S is spent on answers that are thrown away.
2. The budget is shared wall-clock, not per-ref. When mctl-api is slow, probes for already-owned entities crowd out the ones whose decision actually depends on the answer; those get no answer, fall to LEGACY_UNKNOWN, and are reported ambiguous — left UNOWNED, which is exactly the store-permits-old-forbids class the tool exists to eliminate. So a degraded probe makes the bootstrap less effective in proportion to how much of its work was already done.

probe_refs can be built from just the undecided entities:

undecided = [e for e in ordered if answers[e].verdict not in (OWNED_BY_OTHER, OWNED_BY_ME)]
legacy = probe_all([by_entity[e][0] for e in undecided], probe)
by_id = dict(zip(undecided, (legacy.get(i, LEGACY_UNKNOWN) for i in range(len(undecided)))))

then index the loop below by by_id.get(entity_id, LEGACY_UNKNOWN) instead of by position. That also removes the positional coupling between ordered and probe_all's integer keys, which is the other thing here that only works because the two lists are built one line apart.

Fix this →

[claude]

P2: --apply writes to the store unconditionally, bypassing rollout.records_writes() — the one switch every other writer in this package honours (orchestrator/temporal/activities/lifecycle.py:209 skips every ownership op below observe, and the comment there says it is gated on records_writes() precisely because every op is a mutation).

Concretely, running this at LIFECYCLE_ROLLOUT_MODE=off — which the PR body implies is the intended window, "before the observe soak starts" — produces orphan rows:

1. bootstrap writes active rows for the devloop-workflow decisions;
2. the live DevLoopWorkflow that those rows name does not heartbeat, progress, or release them, because its own lifecycle_ownership activity is skipped at off;
3. the workflow finishes; the row stays active with a stale last_seen;
4. the mode advances to observe/enforce and the shepherd reads OWNED_BY_OTHER — blocks_others True — for a PR nobody drives, until the dead/stuck bound recovers it.

That is store-forbids-old-permits, the mirror of the class this tool exists to remove, and it is manufactured by the tool itself rather than measured.

Either gate --apply on rollout.records_writes(), or — if running below observe is genuinely intended — state the required ordering in the module docstring and in the WorkflowTemplate, because the rest of the package reads as though no write can happen at off. A one-line print of rollout.mode() in the report would also let an operator see which regime produced it.

Fix this →

[claude]

P3: future.result() re-raises whatever the probe raised, and the only handler around this loop is except FuturesTimeoutError. So a probe that raises takes down build_report -> main() with a traceback instead of producing the report.

In _filter_dev_loop_owned the same shape is safe because the callable is pinned to _dev_loop_owns_answer, whose except clause is broad by construction. Here probe is a parameter, so the guarantee is the caller's rather than this function's — and the docstring promises "a ref the budget did not answer is LEGACY_UNKNOWN", which is true of a timeout but not of a raise. Nothing is written either way (build_report never writes), so this is a legibility cost, not a data one:

try:
    answers[futures[future]] = future.result()
except Exception as exc:  # noqa: BLE001 - an unanswered probe is not a decision
    print(f"warn: dev-loop probe raised: {exc}")

leaves the entry absent, which the caller already reads as LEGACY_UNKNOWN.

[claude]

P3: Plan.as_dict drops proposal_ref, policy_ref and temporal_workflow_id — three of the six fields apply_report actually writes.

The module docstring says the dry-run report is the deliverable and that "an operator reads it against the store and live Temporal before anything is written". They cannot: policy_ref is the provenance the whole policy.py module exists to record, and temporal_workflow_id is the field that makes the "against live Temporal" check possible. For the devloop rows temporal_workflow_id == owner_id, so it is visible by coincidence; for shepherd and pr-steward rows the two refs are invisible until after the write.

Adding them costs three lines and makes the dry run diffable against mctl_get_lifecycle_ownership field for field.

[claude]

P3: this fallback is unreachable, and it defaults the opposite way to the identical expression 20 lines above.

OwnershipAnswer() at line 355 defaults to verdict=UNKNOWN, so a by_entity id missing from answers has already aborted the run. By the time control reaches here, answers[entity_id] is guaranteed present. The or OwnershipAnswer(verdict=UNOWNED) therefore never fires — but it reads as a deliberate "treat a missing read as unowned", which is exactly the rule the abort above exists to forbid. answers[entity_id] says what is actually true and fails loudly if the invariant ever breaks.

[claude]

P3: a run that left entities ambiguous exits 0, so the Argo Workflow goes green.

report.failed is only ever populated under --apply, so a dry run always exits 0 regardless of what it found, and an --apply run that classified every entity as ambiguous also exits 0 while having written nothing. Ambiguous entities are the ones the store does not cover, which is the store-permits-old-forbids condition the module docstring opens by naming as the dangerous class — the outcome most worth surfacing is the one the exit code is silent about.

Not necessarily a non-zero exit (an operator reads the JSON either way), but the summary line is worth printing to stderr next to the report: N planned, M ambiguous, K written, J failed. An Argo log tail then answers "did this work" without parsing 400 lines of JSON.

[claude]

P3: two different id shapes, and this one is derived from parsed URL text rather than from policy.

shepherd:{service} is keyed on the service name; pr-steward:{repo} is keyed on {owner}/{repo} reconstructed by _parse_pr_url from whatever string .status.yaml happened to hold. The docstring calls both DETERMINISTIC, and they are deterministic per-URL — but pr: pointing at the same PR via the API host (https://api.github.com/repos/mctlhq/x/pulls/7) and via the web host both parse to mctlhq/x, while a fork URL would not, so this id is only as stable as the gitops YAML.

More to the point: the actual pr-steward lives in another repository and nothing in this repo pins the id format it will read or write. If it claims rows as, say, pr-steward:{service}, these bootstrap rows name an owner that never matches, and _lost_to_someone_else-style comparisons on the other side see a stranger holding the entity. Worth either deriving it from service for symmetry with the shepherd arm, or recording the agreed format in contract.py next to OWNER_PR_STEWARD so the two repos have one place to disagree in.

[claude]

P3 (test coverage): this asserts report.failed, but not that the failure reaches the exit code.

main() ends with return 1 if report.failed else 0, and the only main()-level exit-code assertion in the file is the aborted one. So the branch where --apply ran, some acquires were refused, and the Argo step must fail is untested — a future refactor that moved apply_report after the print or dropped the failed check would pass the whole suite.

One line on top of the existing _install_client helper covers it:

client = _Client(acquire_answer=OwnershipAnswer(verdict=OWNED_BY_OTHER, reason="409"))
_install_client(monkeypatch, client)
assert bootstrap.main(["--state-dir", str(root), "--apply"]) == 1

[claude]

Has P1/P2 findings, changes requested: 2 P2, 5 P3.

P2-1 (bootstrap.py:367) — the liveness probe runs for every ref, including entities the store already holds, whose answer plan_for discards at rung 1. The 60s budget is shared wall-clock, so already-decided refs crowd out the ones whose decision depends on the probe; those fall to ambiguous and stay UNOWNED, which is the store-permits-old-forbids class this tool exists to remove. On a re-run the entire budget is spent on discarded answers.

P2-2 (bootstrap.py:436) — --apply writes unconditionally, bypassing rollout.records_writes(), the switch every other writer honours (activities/lifecycle.py:209). At LIFECYCLE_ROLLOUT_MODE=off the DevLoop rows this writes are never heartbeated or released by the workflow they name, leaving orphan active rows that block the shepherd once the mode advances. Gate it, or document the required ordering.

P3s: probe exceptions escape except FuturesTimeoutError; Plan.as_dict omits the three provenance fields --apply writes, so the dry run is not diffable against the store; an unreachable or OwnershipAnswer(verdict=UNOWNED) that defaults opposite to the abort check above it; ambiguous-only runs exit 0; two inconsistent owner-id shapes with the pr-steward one derived from parsed URL text; no test that main() exits 1 on refused acquires.

The three fixes claimed in the description all check out against the source: dry_run=True does suppress update_status_file, fix_only=True with no service_filter does force every service past the SKIP drop while policy.default_owner_for still reads the real mode, and counts now spans both lists. Read-first-then-abort, the tri-state probe at rung 2, and the deterministic owner ids are right.

Note: I could not execute the test suite or ruff in this environment (command approval), so the verification claims in the description are unconfirmed here; the review is static.

Reviewed at 374f6ab.

[claude]

Claude finished @mashkovd's task in 6m 48s —— View job

---

Review complete

• Read repo conventions (CLAUDE.md / AGENTS.md)
• Read the pinned diff (3dc9e9f...873be5e, 847 added lines across 2 files)
• Read surrounding modules (contract.py, client.py, policy.py, rollout.py, run_shepherd._discover_refs / _parse_pr_url / _service_mode, temporal/activities/lifecycle.py)
• Analyze correctness / safety / test coverage
• Post inline findings and verdict

---

What holds up

The three safety properties the description claims are real and are backed by code I verified, not just by comments:

• Read-first abort. OwnershipAnswer() really does default to verdict=UNKNOWN (contract.py:246), and client.get_many returns every requested id even on a store outage (client.py:216-224), so the unknown scan at bootstrap.py:351 cannot miss an id by absence. main() returns before apply_report on that path.
• Deterministic owner ids. shepherd:{service} / pr-steward:{repo} carry no pod name or timestamp, so the race between the read-first pass and a concurrent re-run writes the same owner rather than colliding.
• Ambiguity reported, not resolved. Consistent with types.py's four-state vocabulary, and verdict_for (contract.py:403-409) already resolves an unrecognised state toward UNKNOWN — the same direction.

Both discovery-layer defects the description calls out are genuinely fixed: dry_run=True is unconditional, and fix_only=True with service_filter=None does force FIX_ONLY for every service (run_shepherd.py:1027-1031), which is what makes the pr-steward rung reachable while policy.default_owner_for still re-reads the unforced mode.

Findings

I posted 4 new inline comments; 8 from an earlier pass already sit on this same SHA. Totals: 0 P1, 3 P2, 9 P3.

New this pass:

Sev	Location	Finding
P2	bootstrap.py:429	_discover_refs is called with service_filter/slug_filter hardcoded to None, so --apply is always fleet-wide. Combined with apply_report skipping already-owned, a re-run cannot correct a row this run got wrong — the second run reads OWNED_BY_OTHER and reports it already-owned. Nothing in the file releases or rewrites. The two flags already exist as _discover_refs parameters.
P3	bootstrap.py:258	Third copy of the DevLoop workflow-id rule (run_shepherd.py:216, issue_ref.py:37, orphans.py:99). run_shepherd's hardcoded mctlhq is justified in-line by "a wrong owner would only produce a 404, i.e. the fail-open path" — that justification does not transfer here, because this id is written into temporal_workflow_id, the field a dead-owner sweep resolves against Temporal.
P3	bootstrap.py:362	The abort path returns a report whose ambiguous list and counts.total are already populated from the pre-read discovery loop — so a store outage prints a plausible-looking total that excludes every id that read UNKNOWN. as_dict's own comment calls total the number the soak's sample target is re-derived from.
P3	tests/test_lifecycle_bootstrap.py:293	The regression test for defect 1 passes whether or not the path it guards ran: if the ref is never discovered, .status.yaml is untouched for the trivial reason. Non-vacuous today only because in-progress happens to be in SHEPHERD_INPUT_STATUSES — a fact the test neither states nor pins.

Carried over from the earlier pass on this SHA (still open): the probe running for already-owned refs under a shared budget (:367, P2); --apply bypassing rollout.records_writes() (:436, P2); and five P3s on as_dict field coverage, future.result() re-raising past a timeout-only handler, the unreachable or OwnershipAnswer(verdict=UNOWNED), the exit code on an all-ambiguous run, and the two owner-id shapes.

Notes

• Verdict binding. Submitted CHANGES_REQUESTED explicitly against 873be5e. Worth knowing: the earlier review on this PR was recorded against 374f6ab — a commit not in this checkout — so this SHA had no verdict of its own until now.
• Tests not run. pytest and ruff were not permitted in this environment, so the description's "1300 passed / 22 tests" is unverified here. Findings above come from reading the code and the modules it calls, not from a failing run.
  · branch feat/lifecycle-bootstrap

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect import correctness, report integrity, discovery reliability, and ownership correlation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a read-first bootstrap to import existing pull requests into lifecycle ownership storage.

Changes:

• Adds discovery, planning, probing, reporting, and optional ownership writes.
• Adds safety, idempotence, ambiguity, and integration tests.

File summaries

File	Summary	Findings
tests/test_lifecycle_bootstrap.py	Tests bootstrap behavior and safety.	No findings.
orchestrator/lifecycle/bootstrap.py	Implements bootstrap discovery and ownership import.	Critical: filter terminal PRs. Moderate: prevent late probe output from corrupting JSON, address unauthenticated fallback discovery, and carry the PR head SHA in EntityRef.version.

Review details

Suppressed comments (3)

orchestrator/lifecycle/bootstrap.py:318

• _parse_pr_url only extracts the last four path segments; it does not validate the GitHub host or that the route is /pull/<n>//pulls/<n>. Consequently a bad pr: such as https://github.com/mctlhq/mctl-web/issues/42 is accepted here and can create an ownership row for an issue instead of being reported ambiguous. Because this migration writes durable ownership, reject non-PR URLs (and, if required by the state contract, URLs whose repository does not match the proposal) before adding them to by_entity.

    for ref in refs:
        try:
            owner_name, repo_name, number = _parse_pr_url(ref.pr_url or "")

orchestrator/lifecycle/bootstrap.py:429

• The fallback this call relies on for missing pr: fields invokes gh pr list and requires GitHub authentication, but the paired lifecycle-bootstrap WorkflowTemplate does not provide GITHUB_TOKEN (it explicitly removed the OAuth token). Any in-progress proposal whose URL is missing will therefore be silently omitted from the import, leaving migration noise despite this module's advertised fallback. Provide read-capable GitHub credentials to that workflow or make the fallback fail/report explicitly.

    refs = _discover_refs(args.state_dir, dry_run=True, fix_only=True)

orchestrator/lifecycle/bootstrap.py:391

• This constructs the imported entity with an empty version, so the acquire sends no current PR head SHA. The lifecycle contract defines a pull-request EntityRef.version as the head SHA (ADR-010 §3), and the live workflow passes head_sha on its own acquire; imported rows therefore do not carry the last observed head needed for consistent ownership/conflict correlation. Fetch the PR head during discovery and carry it through the plan into this EntityRef.

            EntityRef(kind=KIND_PULL_REQUEST, id=plan.entity_id),
            PHASE_REVIEW_REMEDIATION,
            Owner(type=plan.owner_type, id=plan.owner_id),
            proposal_ref=plan.proposal_ref,
            policy_ref=plan.policy_ref,

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

P2 — future.result() re-raises whatever the probe raised, and the only handler around this loop is except FuturesTimeoutError. Two distinct consequences:

1. A raising probe kills the whole run instead of producing one ambiguous ref. Anything the probe raises that is not TimeoutError propagates out of probe_all -> build_report -> main(), so the operator gets a traceback and no report at all. _dev_loop_owns_answer is defensive but it is not total: re.match on a non-str slug, or anything raised before its own try, escapes. That contradicts this function's own docstring six lines up ("A ref the budget did not answer is LEGACY_UNKNOWN ... An unanswered probe must not become a decision"), and it is precisely the failure mode run_shepherd.py:227 records for the sweep: "aborted the whole sweep instead of failing open for the one proposal."

2. A builtin TimeoutError from the probe is misread as budget exhaustion. Since 3.11, concurrent.futures.TimeoutError is TimeoutError, so such a result lands in the budget handler. The for loop then exits early, every future that had already completed but not yet been iterated is silently dropped to ambiguous, and the warn line blames a budget that never expired.

Both stay inside the stated contract with a per-future guard:

for future in as_completed(futures, timeout=DEV_LOOP_LIVENESS_BUDGET_S):
    try:
        answers[futures[future]] = future.result()
    except Exception as exc:  # noqa: BLE001 - an unanswered probe is ambiguous, not fatal
        print(f"warn: dev-loop probe raised for a proposal: {exc}")

Nothing has been written at this point, so this is not a data-safety defect. Under --apply it is still the difference between an import that reports one ambiguous ref and one that reports nothing. (_filter_dev_loop_owned has the same shape today, but it fails open by design; here the same shape fails loud, and this module's docstring promises otherwise.)

[claude]

P2 — this is a verbatim second copy of run_shepherd.py:215-216:

m = re.match(r"issue-(\d+)-", slug)
...
workflow_id = f"dev-loop-mctlhq-{service}-{m.group(1)}"

The docstring at line 217 states the guarantee that rests on it — "a row written here names the same owner the workflow would have named" — and line 249 says "the same rule run_shepherd's liveness probe applies, so the id recorded here is the one the probe just confirmed alive." That holds only while the two copies agree, and nothing pins them together: test_the_devloop_rung_names_the_workflow_the_probe_confirmed asserts dev-loop-mctlhq-mctl-web-7 against this copy, not against _dev_loop_owns_answer's.

This is exactly the convention policy.py's module docstring sets for this package — "thin wrappers over run_shepherd's existing constants rather than a second copy of them. A second copy would drift, and the whole point of this package is to stop having two answers to one question." The repo already carries three derivations of this id (issue_ref.workflow_id_for, orphans._expected_workflow_id, _dev_loop_owns_answer); this makes four, and it is the only one whose output is written into a durable ownership record as the owner_id. If it drifts, the probe confirms workflow A alive and the store records workflow B as the holder — a row nothing will ever release, recoverable only via the reconciler's liveness bound.

Extracting the expression in run_shepherd (e.g. _dev_loop_workflow_id(service, slug) -> str | None) and calling it from both _dev_loop_owns_answer and here makes the guarantee structural instead of coincidental, and removes the function-level import re at line 251 as a side effect.

Note this is a drift hazard, not a present bug — the two copies are identical today.

[claude]

P3 — on the abort path as_dict() still emits a fully populated counts block, and it is computed from a partial scan.

build_report returns at line 362 before the plan loop runs, so planned is empty and ambiguous holds only the pre-read entries (unparseable pr_url, duplicate entity id). A run over 300 proposals that aborts on the store read therefore prints "counts": {"ambiguous": 2, ..., "total": 2} next to "aborted": "...".

The comment directly above says total is "the measured decision rate the soak's sample target is re-derived from" — which is the one number that must not be quietly wrong. aborted being non-empty and main() returning 1 do make it detectable, but an operator scraping counts.total out of an Argo log has no reason to re-check aborted first. Suppressing counts/planned/ambiguous when aborted is set (or adding a scanned field) would make the partial view unmistakable.

[claude]

P3 — a run that discovered nothing is indistinguishable from a successful import.

by_entity empty returns a report with aborted: "", every count 0, and main() exits 0. That is the same signal a real import gives, and the premise of the whole tool is that "the store has to hold a row for every entity the old mechanism already knows about" — so a zero-row outcome is the one result that should not look like success. _discover_refs raises SystemExit only when --state-dir is not a directory; a valid-but-wrong checkout (empty clone, a gitops repo mounted one level off, every proposal outside SHEPHERD_INPUT_STATUSES) reaches here silently, and the soak then starts against an unpopulated store reading store-permits-old-forbids for everything — precisely the outcome the module docstring says this exists to prevent.

A warn: line on the empty case, or a distinct exit code, costs nothing and makes the misconfiguration visible in the Argo log.

[claude]

P3 — both halves of this guard are unconditionally true for anything main() can produce, so rung 5 ("no rule in the ladder applied", line 239) is unreachable in production.

ProposalRef.proposal_dir is a required Path field with no default (run_shepherd.py:809), so it is never None; and _discover_refs continues on not pr_url (run_shepherd.py:1074), so ref.pr_url is always truthy. Rung 5 is therefore only reachable from hand-built refs, and no test exercises it.

Not a defect — the ladder is still correct, and keeping an explicit final else is defensible as a closed-vocabulary guard. But the docstring presents rung 4's condition as a real discriminator, and it is worth a line saying it is a belt-and-braces restatement of what _discover_refs already guarantees, so a future reader does not assume PR-less refs can arrive here.

[claude]

P3 — a 0.2s budget makes this assertion timing-dependent. assert answers.get(0) == LEGACY_FREE requires the fast probe to be submitted, scheduled on a pool thread, and collected by as_completed inside 200ms. That is normally ample, but on a loaded CI runner (this suite is ~1300 tests) thread-pool startup can lose to it, and the failure would look like a real regression in probe_all rather than a scheduling hiccup.

A threading.Event the fast probe sets, waited on before the budget can matter — or simply a larger budget with the slow probe blocking on release — pins the same property without a wall-clock race. The second assertion (1 not in answers) is sound either way.

[claude]

Has P1/P2 findings, changes requested: 2 P2, 4 P3.

The three safety properties hold as described. Read-first with a whole-run abort on any UNKNOWN is correct and the fallback in the abort scan (OwnershipAnswer() defaults to UNKNOWN) fails closed for a missing id. already-owned plus deterministic owner ids gives real idempotence, and the ids match what the other side writes — dev_loop.py:1267-1279 uses the same kind, phase, owner_type and owner_id, so a bootstrap-written DevLoop row is the row the workflow's own acquire would produce. The dry_run=True / fix_only=True pairing at _discover_refs is right on both counts, and policy.default_owner_for does re-read the unforced mode, so the pr-steward rung is live.

The two P2s:

• probe_all (line 290): future.result() is unguarded, so a raising probe aborts the run with no report, and a builtin TimeoutError is misattributed to the budget and drops already-completed futures. Both contradict the function's own "an unanswered probe must not become a decision".
• _devloop_workflow_id (line 258): a fourth copy of the workflow-id derivation, and the only one written into a durable record as owner_id. The "same id the probe just confirmed alive" guarantee is coincidental rather than structural, and no test pins the two copies equal. Drift hazard, not a present bug.

Neither is a data-safety defect — nothing is written on either path.

P3s: partial counts emitted on the abort path, zero-discovery exiting 0 indistinguishably from success, a dead condition in rung 4, and a 0.2s wall-clock budget in the probe test.

[claude]

P2: --apply has no way to be scoped, so the first production write is the whole fleet at once — and this tool has no path back.

_discover_refs already takes service_filter and slug_filter (orchestrator/run_shepherd.py:992-995); this call hardcodes both to None. The consequences compose badly with the rest of the module's own design:

• apply_report skips already-owned, so a re-run cannot correct a row this run got wrong — the second run reads OWNED_BY_OTHER for the very row the first run mis-wrote and reports it already-owned. Nothing in this file releases or rewrites a row.
• The recovery is therefore manual, per entity, against the store — for however many proposals happened to be open that day.
• The module docstring's mitigation is "an operator reads the report against the store and live Temporal before anything is written". That is a credible check for one service's worth of rows; it is not the same check when the report is every open PR in the org, and as_dict omits policy_ref/temporal_workflow_id from it anyway.

Passing the filters through costs two add_argument lines and turns the rollout into "one service, verify, widen" instead of one irreversible step:

parser.add_argument("--service", default=None)
parser.add_argument("--slug", default=None)
...
refs = _discover_refs(args.state_dir, args.service, args.slug, dry_run=True, fix_only=True)

The force_fix_only interaction is already correct for this: _discover_refs scopes the override to service_filter when one is set, which is exactly the service being imported.

Fix this →

[claude]

P3: this is the third copy of the DevLoop workflow-id rule, and the first one whose drift is durable.

The other two: orchestrator/run_shepherd.py:216 (inside _dev_loop_owns_answer — the probe whose answer this line is reacting to) and orchestrator/temporal/issue_ref.py:37 (dev-loop-{owner}-{repo}-{number}, which start.py actually uses). orchestrator/temporal/activities/orphans.py:99 is a fourth variant, which takes owner as a parameter instead of hardcoding it.

policy.py's module docstring states the rule this breaks: "thin wrappers over run_shepherd's existing constants rather than a second copy of them. A second copy would drift, and the whole point of this package is to stop having two answers to one question."

The asymmetry that makes this copy worse than run_shepherd's is written at run_shepherd.py:211-214: mctlhq is hardcoded there because "a wrong owner would only produce a 404, i.e. the fail-open 'not owned' path". That justification does not transfer. Here the derived id is not probed — it is written into temporal_workflow_id on the row, which is the field a dead-owner sweep resolves against Temporal. A copy that drifts from issue_ref.py produces rows naming a workflow that never existed, and "is this owner still alive" then has no answer for exactly the entities the bootstrap claimed on the DevLoop's behalf.

Exporting the derivation from run_shepherd (or calling issue_ref) keeps the id the probe confirmed and the id recorded as one expression rather than two that happen to agree today.

[claude]

P3: an aborted report still prints a populated ambiguous list and a counts.total derived from it.

The unparseable-URL and duplicate-entity plans are appended to report.ambiguous in the discovery loop above, which runs before the store is read. So the return report here emits a report whose counts.total is not "the entities this run classified" but "the entities this run rejected before it found out the store was down" — a number that excludes every id that read UNKNOWN.

aborted being non-empty makes it detectable, but the field the module says is load-bearing is the one that misleads: as_dict's own comment calls total "the measured decision rate the soak's sample target is re-derived from". A run that aborted on a store outage and printed "total": 3 is the shape most likely to be re-derived from by mistake, because 3 is a plausible number rather than an obvious sentinel.

Either clear the lists on the abort path, or have as_dict suppress counts/planned/ambiguous when aborted is set — "nothing was written" reads better next to nothing classified.

[claude]

P3 (test coverage): this assertion holds whether or not the path it guards ran.

If the proposal is never discovered — a status dropped from SHEPHERD_INPUT_STATUSES, a filter added to _discover_refs, _find_pr_url_by_branch moved behind another name — then _discover_refs returns [], build_report returns an empty report, .status.yaml is untouched for the trivial reason, and this test stays green while guarding nothing.

It is non-vacuous today only because in-progress happens to be in SHEPHERD_INPUT_STATUSES (orchestrator/run_shepherd.py:393) — a fact this file neither states nor pins. Given the PR's own framing (this is the regression test for defect 1, which was invisible to every test that built ProposalRefs by hand), it is worth one line that fails if the ref stops reaching the ladder:

report = _report_of(capsys.readouterr().out)
assert report["counts"]["total"] == 1, "the ref never reached the ladder; the assertion below is vacuous"
assert status.read_text() == before, "the dry run rewrote .status.yaml"

The same reasoning applies to capsys being in the signature but unused here.

[claude]

Has P1/P2 findings, changes requested: 3 P2, 9 P3 against 873be5e (this verdict is bound to that SHA; the earlier CHANGES_REQUESTED on this PR was recorded against 374f6ab).

P2-1 (bootstrap.py:367) - the liveness probe runs for every ref including already-owned ones, whose answer plan_for discards at rung 1; a degraded probe then starves the undecided entities of the shared 60s budget.
P2-2 (bootstrap.py:436) - --apply writes unconditionally, bypassing rollout.records_writes(); at LIFECYCLE_ROLLOUT_MODE=off it manufactures orphan active rows nothing heartbeats or releases.
P2-3 (bootstrap.py:429) - _discover_refs is called with service_filter/slug_filter hardcoded to None, so --apply is fleet-wide and apply_report's already-owned skip means a re-run cannot correct it.

P3: as_dict drops proposal_ref/policy_ref/temporal_workflow_id; future.result() re-raises past the timeout-only handler; unreachable UNOWNED fallback at line 372; exit 0 on an all-ambiguous run; two owner-id shapes; _devloop_workflow_id is a third copy of the workflow-id rule written durably into temporal_workflow_id; an aborted report still prints a populated counts.total; two tests that can pass vacuously.

The three safety properties the PR claims (read-first abort, two-legged idempotence, ambiguity reported not resolved) hold as written and are tested. Tests were not executed in this run.

[claude]

Claude finished @mashkovd's task in 5m 3s —— View job

---

Review complete

• Read repo conventions (LLMS.md, pyproject.toml)
• Read the pinned diff (ce86e1d...416addb)
• Read surrounding modules (shadow.py, contract.py, client.py, policy.py, rollout.py, run_shepherd)
• Analyze correctness / safety / test coverage
• Post inline findings and verdict

Verdict: changes requested — 2 P2, 3 P3 against 416addb.

What the two new commits get right

416addb's diagnosis is correct. DEV_LOOP_LIVENESS_BUDGET_S is 60s because the sweep runs every five minutes and an unanswered ref is free there (run_shepherd.py:142-148 says so in as many words), while cbdaf27 made an unanswered ref undetermined and therefore red. Splitting the constant is the right move, the reason is written into the constant rather than left in a commit message, and the probe test now monkeypatches bootstrap.PROBE_BUDGET_S instead of the sweep's. Plan.undetermined enumerates all five producers correctly, and the ladder table asserts the flag on every row — that table was the one place two rungs reporting AMBIGUOUS and meaning opposite things were indistinguishable.

6928f5d is right on the merits too: with the pr-steward rung gone a SKIP-service entity can only land in rung 4, and shadow.compare_proposal_refs runs inside the sweep, which drops those services — so discovering them padded counts.total with entities that will never produce a comparison.

P2

Location	Finding
bootstrap.py:103	The commit moved the budget off the sweep but not the ceiling, and the docstring it adds names an operator knob that does not exist. PROBE_BUDGET_S is a module constant — this file imports argparse, json, re, sys and no os, no env var, no flag — so probe_all's "sizing the budget is an operator concern before a first --apply" means editing Python and bumping agent_image. And wall-clock is half the bound: max_workers is still the sweep's 8, chosen for the cadence reason the commit disavows. Degraded mctl-api → 8 × 300 / 10 = 240 refs answered, inside the "few hundred proposals" the commit message names as motivating. test_the_probe_budget_is_this_modules_own holds at 61 as well as 300.
bootstrap.py:716 (carried from 6928f5d, already inline)	Dropping fix_only makes what this tool imports depend on run_shepherd.SHEPHERD_SKIP_SERVICES, an import-time constant read from this pod's env, and nothing in the report records what it was. Unset (the default) the change is a no-op with no signal; set to a superset of the shepherd's, a service the sweep does compare is dropped before the ladder — its live-DevLoop entities never probed, never reported, store-permits-old-forbids left in place by a run that exits 0. rollout_mode is echoed into the report for exactly this reason; the skip set is now load-bearing the same way.

P3

• tests:97 — third pass: the ladder table's LEGACY_UNKNOWN comment still explains the row by "falling through to the shepherd rung", deleted in 7ad96cd. It matters more now — the new fifth column is the only thing distinguishing that row from the one above it, and this comment is the only prose explaining why they differ.
• tests:352 — test_a_skipped_service_is_not_discovered drops main()'s return value, which is 1 via the "discovered no proposals at all" branch — a message naming the wrong cause when the proposals were found and dropped as SKIP. Before 6928f5d that branch could only fire on a genuinely empty checkout.
• bootstrap.py:761 (carried) — the transient/permanent split is re-derived from a substring of a human-readable reason, which is what Plan.undetermined was introduced to stop doing. "Needing a fix in .status.yaml" is also not true of the empty-workflow-id refusal (that is a directory rename), and the comment's "a store read that came back empty for one id" cannot reach this line — such a read sets report.aborted and returns above.

What holds

The three safety properties still hold and are tested: read-first with a whole-run abort on any UNKNOWN (OwnershipAnswer defaults to UNKNOWN and client.get_many answers every requested id, so a missing id aborts too), two-legged idempotence, and ambiguity reported rather than resolved. The probe is still keyed by entity id via zip(..., strict=True) and runs only for undecided entities, future.result() is guarded per-future, dry_run=True at _discover_refs is unconditional, and run_shepherd.devloop_workflow_id remains the single definition that _dev_loop_owns_answer calls.

Not run

uv run pytest and uv run ruff check both need approval beyond this job's allowlist, so the description's test and lint claims are unverified here. Every finding comes from reading the pinned diff (ce86e1d...416addb) and the modules it calls.
· branch feat/lifecycle-bootstrap

[claude]

P2: the commit renames the budget's owner but not its ceiling, and the docstring it adds names an operator knob that does not exist.

There is no knob. probe_all's docstring now closes with "sizing the budget is an operator concern before a first --apply on a large fleet" (line 402). PROBE_BUDGET_S is a module constant: this file imports argparse, json, re, sys and no os, there is no env var, and no CLI flag. An operator who reads that line and concludes the budget is theirs to size finds that sizing it means editing Python and bumping agent_image. Every other operator-facing dial in this module is either a flag (--service, --slug) or an attested env var (rollout.mode(), echoed into the report for exactly this reason) — this one is neither, and it is the one the commit message says decides whether a large-fleet run goes green.

The ceiling barely moved for the case the commit names. Wall-clock is only half the bound; the other half is max_workers=min(DEV_LOOP_LIVENESS_WORKERS, len(refs)) on line 409, still the sweep's 8. In the degraded case the commit message is about — a slow mctl-api where calls run to DEV_LOOP_LIVENESS_TIMEOUT_S — the pass answers workers × budget / timeout refs:

8 × 300 / 10 = 240

"A fleet of a few hundred proposals against a slow mctl-api" is the stated motivating scenario, and 240 sits inside it. The budget was the constant borrowed from the sweep, but DEV_LOOP_LIVENESS_WORKERS was borrowed from there too, and for the same reason the commit disavows: run_shepherd.py:142-148 picks 8 to avoid "overlapping ticks piling load onto an already-sick API". There are no overlapping ticks here — this runs once. Leaving the concurrency at the cadence-protecting value while moving the budget off it is half the change, and --apply on such a fleet still writes every row it could and then exits 1.

test_the_probe_budget_is_this_modules_own pins PROBE_BUDGET_S > DEV_LOOP_LIVENESS_BUDGET_S, which holds at 61 as well as at 300, so nothing in the suite is sensitive to the sizing the commit is actually about.

Any of these closes it: raise the worker count with the budget and say why this caller may (once, not per tick); expose --probe-budget so the docstring's "operator concern" is true; or state the throughput ceiling in the comment so the next operator sizes --service scope against it rather than discovering it from a red run.

Fix this →

[claude]

P3 (third pass raising this): 416addb updated every row of this table and left the comment that explains the row it just made load-bearing.

There is no shepherd rung. 7ad96cd deleted it, so LEGACY_UNKNOWN cannot "fall through to the shepherd rung" and cannot "hand it a pull request another machine may be pushing to" — it falls through to rung 4, which is the row directly above and produces the same DECISION_AMBIGUOUS with the same empty owner_type.

That matters more here than in the other places this stale phrase survives, because the new fifth column is the only thing that distinguishes these two rows, and this comment is the only prose on the page explaining why they differ. What it should say is what plan_for's docstring now says correctly: rung 4 measured an absence and is a success; rung 2 obtained no measurement and makes the run red.

Same deleted justification still sits on test_a_skipped_service_is_not_imported_either's premise at line 122 and on the fix_only history in bootstrap.py.

[claude]

P3: this drops main()'s return value, and what it returns is the interesting half of the change.

The assertion below is counts["total"] == 0, which is exactly the condition main() treats as a failure eight lines from the end:

if not report.planned and not report.ambiguous:
    print("lifecycle-bootstrap: discovered no proposals at all", file=sys.stderr)
    return 1

So this run exits 1 — and says the checkout was empty, when in fact the proposal was found, read, and dropped because the service resolved to SKIP. 6928f5d made that path reachable for a reason it is silent about: before it, fix_only=True forced every service past the SKIP drop, so a fleet could only fail this check by genuinely having nothing. Now a bootstrap pod whose SHEPHERD_SKIP_SERVICES covers every service with open proposals — or a --service naming a skipped one — reds with a message pointing the operator at the mount path rather than at the env var.

Asserting the exit code here would pin it either way. If 1 is intended, the stderr line is worth splitting: "discovered no proposals at all" versus "every discovered service is skipped", which is one bool from _discover_refs's own skip notices and sends the operator to the right place.

Fix this →

[claude]

Has P1/P2 findings, changes requested: 1 P2, 2 P3 new against 416addb, plus 1 P2 and 1 P3 carried from the pass on 6928f5d that this commit did not touch. (Verdict bound to 416addb; earlier CHANGES_REQUESTED were recorded against 374f6ab, 873be5e, e4512cc, 5fa881c, c236c11, b39a222, 66282ba and cbdaf27.)

What 416addb gets right: the diagnosis is correct and is the one the previous pass raised. DEV_LOOP_LIVENESS_BUDGET_S is 60s because the sweep runs every five minutes and an unanswered ref is free there (run_shepherd.py:142-148 says so), while cbdaf27 made an unanswered ref undetermined and therefore red. Splitting the constant is the right move, the reason is written into the constant rather than into a commit message, the probe test now monkeypatches bootstrap.PROBE_BUDGET_S instead of the sweep constant, Plan.undetermined enumerates all five producers correctly (rung 2, rung 3 twice, unreadable pr:, duplicate mapping), and the ladder table asserts the flag on every row - which was the one place two rungs reporting AMBIGUOUS and meaning opposite things were indistinguishable.

6928f5d is also the right call on the merits: with the pr-steward rung gone, a SKIP-service entity can only land in rung 4, and shadow.compare_proposal_refs runs inside the sweep, which drops those services - so discovering them padded counts.total with entities that will never produce a comparison.

P2 (bootstrap.py:103) - the commit moved the budget off the sweep but not the ceiling, and the docstring it adds names an operator knob that does not exist. PROBE_BUDGET_S is a module constant: bootstrap.py imports argparse, json, re, sys and no os, there is no env var and no CLI flag, so probe_all:402s "sizing the budget is an operator concern before a first --apply on a large fleet" means editing Python and bumping agent_image. And wall-clock is only half the bound: max_workers is still min(DEV_LOOP_LIVENESS_WORKERS, len(refs)), i.e. the sweeps 8, which was chosen for the same cadence reason the commit disavows. On a degraded mctl-api where calls run to DEV_LOOP_LIVENESS_TIMEOUT_S the pass answers workers * budget / timeout = 8 * 300 / 10 = 240 refs - inside "a few hundred proposals", the scenario the commit message names as motivating. test_the_probe_budget_is_this_modules_own pins only PROBE_BUDGET_S > DEV_LOOP_LIVENESS_BUDGET_S, which holds at 61 too, so nothing is sensitive to the sizing the commit is about. Raise the concurrency with the budget and say why this one-shot caller may, or expose --probe-budget so the docstring is true, or state the throughput ceiling so an operator sizes --service scope against it rather than discovering it from a red run.

P2 (bootstrap.py:716, carried from 6928f5d) - dropping fix_only makes what this tool imports depend on run_shepherd.SHEPHERD_SKIP_SERVICES, an import-time module constant read from THIS pods environment, and nothing in the report records what it was. Unset (the default, and what mctlhq/mctl-gitops#1249 was written against) the change is a no-op in production with no signal that it did not take effect; set to a superset of the shepherds, a service the sweep does compare is dropped before the ladder, so its live-DevLoop entities are never probed, never planned and never reported - store-permits-old-forbids left in place by a run that exits 0. The module already has the pattern for exactly this: rollout_mode is echoed into the report because the gate reads this processs environment and not the workers. The skip set is now load-bearing in the same way and belongs in the report and in KNOWN LIMITS.

P3 (tests:97) - third pass: the ladder tables LEGACY_UNKNOWN comment still explains the row by "falling through to the shepherd rung", deleted in 7ad96cd. It matters more now, because the new fifth column is the only thing distinguishing that row from the one above it and this comment is the only prose explaining why they differ.

P3 (tests:352) - test_a_skipped_service_is_not_discovered drops main()s return value, which is 1 via the "discovered no proposals at all" branch - a message naming the wrong cause when the proposals were found and dropped as SKIP. Before 6928f5d that branch could only fire on a genuinely empty checkout.

P3 (bootstrap.py:761, carried from 6928f5d) - the transient/permanent split is re-derived from a substring of a human-readable reason, which is what Plan.undetermined was introduced to stop doing; the "needing a fix in .status.yaml" label is not true of the empty-workflow-id refusal (that is a directory rename), and the comments "a store read that came back empty for one id" cannot reach this line, since such a read sets report.aborted and returns above.

The three safety properties still hold and are tested: read-first with a whole-run abort on any UNKNOWN (OwnershipAnswer defaults to UNKNOWN and client.get_many answers every requested id, so a missing id aborts too), two-legged idempotence, and ambiguity reported rather than resolved. The probe is still keyed by entity id via zip(..., strict=True) and run only for undecided entities, future.result() is still guarded per-future, dry_run=True at _discover_refs is unconditional, and run_shepherd.devloop_workflow_id remains the single definition that _dev_loop_owns_answer calls.

Tests not run: uv run pytest and uv run ruff check both need approval beyond this jobs allowlist, so the descriptions test and lint claims are unverified here. Every finding comes from reading the pinned diff (ce86e1d...416addb) and the modules it calls.

[claude]

Claude finished @mashkovd's task in 5m 29s —— View job

---

Review complete

• Read repo conventions (LLMS.md, pyproject.toml)
• Read the pinned diff (ce86e1d...f910a80)
• Read surrounding modules (policy.py, rollout.py, client.py, contract.py, run_shepherd._discover_refs / _service_mode / devloop_workflow_id, shadow.py)
• Analyze correctness / safety / test coverage
• Post inline findings and verdict

Verdict: changes requested — 1 P2, 3 P3 against f910a80.

What f910a80 gets right

The reversal is correct, and the premise it corrects was genuinely false. A DevLoopWorkflow is started per issue and never consults SHEPHERD_SKIP_SERVICES, so a skipped service really can have a live DevLoop driving one of its pull requests — and dropping it at discovery left store-permits-old-forbids in place behind a green run. fix_only=True is the right lever: _service_mode returns FIX_ONLY before either service set is read (run_shepherd.py:616), and _discover_refs scopes the override to the targeted service (run_shepherd.py:1051), so --service on a skipped repo still works and no other service's mode is silently overridden. The comment now says what the flag does rather than what it was once for. test_a_skipped_service_is_still_discovered is non-vacuous — its monkeypatched _service_mode returns SKIP unless force_fix_only, so dropping the argument again fails it.

Making retryable a field is right for the stated reason: main() was grepping a substring of a human-readable reason, which is the thing Plan.undetermined was introduced to replace, and the prose was wrong anyway — the empty-workflow-id refusal is fixed by renaming a proposal directory, not by editing .status.yaml. The new wording no longer claims otherwise.

P2

Location

Finding

bootstrap.py:296

Discovery is now independent of this pod's environment; the rows it writes are not. policy.policy_ref_for calls run_shepherd._service_mode(service) with no force_fix_only (policy.py:93), and apply_report writes the result durably as policy_ref. On the default this commit names — the variable unset in the bootstrap pod, since mctlhq/mctl-gitops#1249 was written against a caller that did not read it — a DevLoop row imported for mctl-claude-remote records service-mode:mctl-claude-remote=full, while the shepherd resolves that service to skip. full is the mode meaning "discovers, fixes and merges", recorded against the one repository this repo's own comments call out as another PR lifecycle's. Worse than the discovery version in two ways: skip_services at least makes that one diagnosable after the fact, while policy_ref is durable and apply_report skips already-owned, so a re-run from a correct pod cannot rewrite it — and policy.py's docstring is explicit that the field exists so an operator need not reconstruct the deciding process's environment. Nothing reads policy_ref, so it is an audit field that says something untrue rather than a behavioural break.

P3

• bootstrap.py:759 — discovery_ignored_skip_set is a hardcoded literal, so it can only ever be true, including on the run where it would be false. The regression that matters is caught elsewhere, which is why this is a nit — but the field's docstring promises a second independent signal and there is only one. Relatedly, skip_services justifies itself by a hazard fix_only=True eliminates three lines above; the value is load-bearing for policy_ref and the rung-4 reason, which is not where the comment points.
• tests/test_lifecycle_bootstrap.py:119 — the ladder table pins undetermined on every row and not retryable, the second field main() reads off a plan. Rung 3's two refusals are asserted nowhere for it, so setting retryable=True there would tell an operator to re-run a checkout problem forever with the suite still green.
• bootstrap.py:433 — carried from 416addb and widened here: the budget moved off the sweep's, the concurrency did not. max_workers is still DEV_LOOP_LIVENESS_WORKERS = 8, so a degraded mctl-api caps the pass at 8 × 300 / 10 = 240 refs, and restoring fix_only puts skipped services back in that denominator. probe_all's docstring calls sizing the budget an operator concern; there is no flag and no env var, and test_the_probe_budget_is_this_modules_own holds at 61 as well as 300.

What holds

The three safety properties still hold and are tested: read-first with a whole-run abort on any UNKNOWN (OwnershipAnswer defaults to UNKNOWN and client.get_many answers every requested id, so a missing id aborts too), two-legged idempotence, and ambiguity reported rather than resolved. dry_run=True at _discover_refs is unconditional, the probe is keyed by entity id via zip(..., strict=True) and runs only for undecided entities, future.result() is guarded per-future, and run_shepherd.devloop_workflow_id remains the single definition _dev_loop_owns_answer calls.

Not run

uv run pytest and uv run ruff check both need approval beyond this job's allowlist, so the description's test and lint claims are unverified here. Every finding comes from reading the pinned diff (ce86e1d...f910a80) and the modules it calls.
· branch feat/lifecycle-bootstrap

[github-actions]

Antigravity (agy) review

Model: gemini-3.8-flash-medium · commit 416addb.

No significant issues found.

[claude]

P2: the commit makes discovery independent of this pod's environment, but every row it writes still records that environment durably.

policy.policy_ref_for re-derives the mode without the override (orchestrator/lifecycle/policy.py:93):

return f"service-mode:{service}={run_shepherd._service_mode(service)}"

No force_fix_only, so it reads SHEPHERD_SKIP_SERVICES and SHEPHERD_FIX_ONLY_SERVICES as this pod happens to have them, and apply_report writes the result into the row as policy_ref. Concretely, on the default the commit message itself names — the variable unset in the bootstrap pod, because mctlhq/mctl-gitops#1249 was written against a caller that did not read it — a DevLoop row imported for mctl-claude-remote is written with

policy_ref: service-mode:mctl-claude-remote=full

while the shepherd's own environment resolves that service to skip. full is not a near-miss: it is the mode meaning "the shepherd discovers, fixes and merges", recorded against the one repository this repo's own comments call out as belonging to another PR lifecycle (run_shepherd.py:512-515).

That is the same failure shape as the one just fixed, one layer down, and it is worse in two ways. The skip set is now recorded in the report, so the discovery hazard is at least diagnosable after the fact; policy_ref is a durable field on a durable row, and apply_report skips already-owned, so a re-run in a correctly-configured pod cannot rewrite it. And policy.py's own docstring is explicit that this is the field's whole purpose:

Stored on the ownership record so an operator can see the reason without reconstructing the environment the deciding process ran in — which today is a CWFT env var in another repository.

Nothing in this repository reads policy_ref, so this is not a behavioural break — it is an audit field that says something untrue and cannot be corrected.

Worth noting alongside: Report.skip_services justifies itself by "a skip set wider than the shepherd's would silently drop entities a live DevLoop drives", which is exactly the hazard fix_only=True eliminates three lines above it. The skip set no longer decides what is discovered; it still decides what is recorded on each row, and the rung-4 reason at bootstrap.py:405 (policy.default_owner_for, also unforced). The new recording is pointed at the one path that no longer depends on the variable, and not at the two that still do.

Either state the env-parity requirement in the KNOWN LIMITS block — "policy_ref reflects THIS pod's service-mode vars; run with the shepherd's" — or have the bootstrap record the policy that governs the entity rather than the one that governed the importer. tests/test_lifecycle_bootstrap.py:207 asserts only the prefix, so nothing pins the value either way.

Fix this →

[claude]

P3: this is a literal, not a measurement, so it can only ever be True — including on the run where it would be false.

discovery_ignored_skip_set reads as a fact about what discovery did, and the report's consumer has no way to tell it apart from a computed one. It is set five lines below fix_only=True and derived from nothing; the commit this fixes was exactly a silent removal of that argument, and had the field existed then it would have gone on printing true while discovery honoured the skip set.

The regression that matters is caught — test_a_skipped_service_is_still_discovered fails if fix_only=True goes away — so this is a nit rather than a hole. But the field's own docstring ("and whether discovery was made independent of it") promises a second, independent signal, and there is only one. Deriving it from the argument actually passed, e.g. binding discovery_ignores_skip_set = True next to the _discover_refs call and using it in both places, costs a line and makes the attestation as honest as rollout_mode, which really is read back from the environment it describes.

Related, on the same pair of fields: skip_services is justified by "a skip set wider than the shepherd's would silently drop entities a live DevLoop drives" — the hazard fix_only=True removes three lines earlier. The value is genuinely load-bearing for policy_ref and the rung-4 reason, which still read the unforced mode; the comment points at the one path where it no longer is.

[claude]

P3 (test coverage): retryable is the second field main() reads off a plan, and the ladder table pins the first one only.

416addb added want_undetermined to this parametrisation for exactly the right reason — two rungs report AMBIGUOUS and mean opposite things — and f910a80 adds a second flag with the same shape of risk: rung 2 sets retryable=True, rung 3's two refusals set it False, and the difference is what the operator is told to do next ("re-run" versus "edit the checkout").

Today the distinction is pinned in exactly one place, test_the_summary_separates_transient_from_permanent, which covers rung 2 against an unreadable pr:. Nothing asserts that a rung 3 refusal is non-retryable — neither test_the_whole_repo_must_match_the_service nor test_a_slug_with_no_workflow_id_under_a_live_devloop_is_undetermined looks at the field — so setting retryable=True alongside undetermined=True in either of those branches would tell an operator to re-run a .status.yaml problem forever, and the suite would stay green. A sixth column here is the same one-line fix the fifth was.

[claude]

P3 (carried, and f910a80 widens it): the budget moved off the sweep's; the concurrency did not, and the knob the docstring names does not exist.

max_workers is still DEV_LOOP_LIVENESS_WORKERS — 8, chosen at run_shepherd.py:142-148 to keep overlapping five-minute ticks from piling load onto a sick API, which is the cadence reason PROBE_BUDGET_S's own comment disavows for this caller. Wall-clock is only half the bound; in the degraded case the constant is written for, where calls run to DEV_LOOP_LIVENESS_TIMEOUT_S, the pass answers 8 × 300 / 10 = 240 refs. f910a80 restores fix_only=True, which is right for the reason it gives, and which also puts every skipped service's proposals back into that denominator.

And probe_all's docstring closes with "sizing the budget is an operator concern before a first --apply on a large fleet" (line 426). There is no flag and no env var — this module imports argparse, json, re, sys and no os — so sizing it means editing Python and bumping agent_image. Every other operator-facing input here is either a flag (--service, --slug) or an attested variable echoed into the report (rollout_mode, and now skip_services); this one is neither, and it is the one that decides whether a large-fleet --apply exits 1 after writing every row it could.

test_the_probe_budget_is_this_modules_own pins PROBE_BUDGET_S > DEV_LOOP_LIVENESS_BUDGET_S, which holds at 61 as well as at 300, so nothing in the suite is sensitive to the sizing.

Red-on-unanswered is the right direction and nothing wrong is written, which is why this is P3 — but either raising the worker count for this one-shot caller with the reason written down, or exposing --probe-budget so the docstring's "operator concern" is actionable, would close it.

[claude]

Has P1/P2 findings, changes requested: 1 P2, 3 P3 against f910a80 (verdict bound to that SHA; earlier CHANGES_REQUESTED were recorded against 374f6ab, 873be5e, e4512cc, 5fa881c, c236c11, b39a222, 66282ba, cbdaf27, 6928f5d and 416addb).

What f910a80 gets right. The reversal is correct and the premise it corrects was genuinely false: a DevLoopWorkflow is started per issue and never consults SHEPHERD_SKIP_SERVICES, so a skipped service really can have a live DevLoop driving one of its pull requests, and dropping it at discovery left store-permits-old-forbids in place behind a green run. fix_only=True is the right lever — _service_mode returns FIX_ONLY before either service set is read (run_shepherd.py:616) — and _discover_refs scopes the override to the targeted service, so --service on a skipped repo still works and no other service's mode is silently overridden. The comment now says what the flag does rather than what it was once for, which is what the last three passes asked for. test_a_skipped_service_is_still_discovered is non-vacuous: its monkeypatched _service_mode returns SKIP unless force_fix_only, so dropping the argument again fails it.

Making retryable a field is also right, and right for the stated reason: main() was grepping a substring of a human-readable reason, which is the thing Plan.undetermined was introduced to replace, and the prose was wrong anyway — the empty-workflow-id refusal is fixed by renaming a proposal directory, not by editing .status.yaml. The new wording no longer claims otherwise, and test_the_summary_separates_transient_from_permanent pins the split end-to-end through main().

P2 (bootstrap.py:296) - discovery is now independent of this pod's environment; the rows it writes are not. policy.policy_ref_for calls run_shepherd._service_mode(service) with no force_fix_only (policy.py:93), so policy_ref encodes this pod's SHEPHERD_SKIP_SERVICES / SHEPHERD_FIX_ONLY_SERVICES and apply_report writes it durably. On the default this commit names — the variable unset in the bootstrap pod, since mctlhq/mctl-gitops#1249 was written against a caller that did not read it — a DevLoop row imported for mctl-claude-remote records service-mode:mctl-claude-remote=full while the shepherd resolves it to skip. full is the mode meaning the shepherd discovers, fixes AND merges, recorded against the one repository this repo's own comments call out as another PR lifecycle's. Same failure shape as the one just fixed, one layer down, and worse in two ways: the report's skip_services at least makes the discovery hazard diagnosable, while policy_ref is durable and apply_report skips already-owned, so a re-run from a correctly-configured pod cannot rewrite it — and policy.py's docstring is explicit that the field exists so an operator need not reconstruct the environment the deciding process ran in. Nothing in this repo reads policy_ref, so this is an audit field that says something untrue rather than a behavioural break. The rung-4 reason (bootstrap.py:405, policy.default_owner_for) reads the unforced mode too, though that one is transient prose. Either state the env-parity requirement in KNOWN LIMITS or record the policy that governs the entity rather than the one that governed the importer; tests:207 asserts only the prefix, so nothing pins it either way.

P3 (bootstrap.py:759) - discovery_ignored_skip_set is a hardcoded literal, so it can only ever be true, including on the run where it would be false. The regression that matters is caught by test_a_skipped_service_is_still_discovered, which is why this is a nit, but the field's docstring promises a second independent signal and there is only one. Relatedly, skip_services justifies itself by a hazard fix_only=True eliminates three lines above; the value is load-bearing for policy_ref and the rung-4 reason, which is not where the comment points.

P3 (tests:119) - the ladder table pins undetermined on every row and not retryable, the second field main() now reads off a plan. Rung 3's two refusals are asserted nowhere for it, so setting retryable=True alongside undetermined=True in either branch would tell an operator to re-run a checkout problem forever with the suite still green. A sixth column is the same one-line fix the fifth was.

P3 (bootstrap.py:433, carried from 416addb and widened here) - the budget moved off the sweep's, the concurrency did not: max_workers is still DEV_LOOP_LIVENESS_WORKERS=8, chosen for the cadence reason PROBE_BUDGET_S's comment disavows, so a degraded mctl-api caps the pass at 8*300/10 = 240 refs — and restoring fix_only puts skipped services back in that denominator. probe_all's docstring calls sizing the budget an operator concern; there is no flag and no env var, and test_the_probe_budget_is_this_modules_own holds at 61 as well as 300. Safe direction, nothing wrong written.

The three safety properties still hold and are tested: read-first with a whole-run abort on any UNKNOWN (OwnershipAnswer defaults to UNKNOWN and client.get_many answers every requested id, so a missing id aborts too), two-legged idempotence, and ambiguity reported rather than resolved. dry_run=True at _discover_refs is unconditional, the probe is keyed by entity id via zip(..., strict=True) and runs only for undecided entities, future.result() is guarded per-future, and run_shepherd.devloop_workflow_id remains the single definition _dev_loop_owns_answer calls.

Tests not run: uv run pytest and uv run ruff check both need approval beyond this job's allowlist, so the description's test and lint claims are unverified here. Every finding comes from reading the pinned diff (ce86e1d...f910a80) and the modules it calls.

[github-actions]

Antigravity (agy) review

Model: gemini-3.8-flash-medium · commit f910a80.

No significant issues found.

[mashkovd]

Closing in favour of a split: #384 (plan and report, zero writes) and #385 (the write path, stacked on #384).

Eleven review rounds on one change, and the round on 416addb raised a new P2 rather than a carry-over — policy_ref encoding the importing pod's SHEPHERD_SKIP_SERVICES rather than the policy that decided the row, the same failure shape as the one fixed the round before, one layer down and durable. That is the agreed signal to stop iterating and divide the work along the line the review kept finding.

The line is the one this change never had: everything that decides versus everything that writes.

• feat(lifecycle): plan the in-flight pull request import, and report it #384 — discovery, the DevLoop liveness probe, the import ladder, the ambiguity/undetermined model, the deterministic report, exit codes. No acquire call exists in it. --apply is accepted and refused, so the WorkflowTemplate's contract does not have to change twice.
• feat(lifecycle): write the planned rows under an exact-observe gate #385 — apply_report, the exact-observe rollout gate, idempotence and refusal semantics. Its input is a reviewed production dry run from feat(lifecycle): plan the in-flight pull request import, and report it #384, not a promise that one follows.

Every finding from the last round is closed in #384: the P2 above, plus the rung-4 reason resolving through the same environment, discovery_ignored_skip_set being a second literal that could only say true, PROBE_WORKERS (the budget had moved off the sweep's, the concurrency had not — the ceiling is workers x budget / timeout), and retryable pinned in the ladder table beside undetermined.

Nothing is lost: #385's bootstrap.py is byte-for-byte this branch's f910a80 plus exactly those five fixes. This branch stays on the remote for reference.