Repository navigation
feat(agents): issue-267-feat-work-context-resume-investigator-fr - #408
mctl-agents[bot] merged 9 commits into
Conversation
Add a client-side mirror of the mctl-api WorkItem contract (orchestrator/work_context/), correlate sibling executions of one work item via a new WorkContextRef block on ContextSnapshot, thread optional work-context flags through the investigator CLI and investigate(), and add an idempotent resume signal plus a work_context query on DevLoopWorkflow so a task can be picked up from a different surface or actor without inheriting the previous actor's approval. Everything is gated behind WORK_CONTEXT_ROLLOUT_MODE, defaulting to off: the work-item store is never contacted and the command stream for every existing Temporal history is byte-identical unless an operator moves the rollout mode. work_context_params() exists as a pure, unit-tested helper with no call site into investigate_params, per the proposal's own sanctioned alternative — merging it from inside DevLoopWorkflow.run would require reading the rollout mode directly in workflow code, which this codebase reads only from activities. Re-cuts tests/fixtures/context/investigator-snapshot.json: adding work_context to ContextSnapshot's hashed payload (unconditionally, like step) moves every content_hash, and the module has no production producer yet, so the only affected artifact is this golden fixture. Adds docs/adr/011-work-item-resume-contract.md, extending ADR 009 sec. 1's field/owner table and reaffirming its sec. 5 boundary. docs/agent-inventory.yaml and agents/_manifests/issue-investigator/ agent.yaml are unchanged: the new CLI flags are additive and do not change the investigator's declared driver, entrypoint, promptSources or runtimeContextInputs. Proposal: platform-gitops/agents-state/mctl-agents/proposals/issue-267-feat-work-context-resume-investigator-fr/
|
Claude finished @mctl-agents[bot]'s task in 5m 44s —— View job Review complete — changes requestedReviewed
P2 findings
P3 findings
What looks goodThe four-stage rollout ladder, its default- I was not able to run the test suite in this environment (Bash approval for |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit Finding 1:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| self._work_item_id = self._work_item_id or work_item_id | ||
| self._seen_execution_ids.add(execution_id) | ||
| surface_transition = bool(self._executions) and ( |
There was a problem hiding this comment.
P2 — surface_transition is inert on every workflow production actually starts, so the PR's headline guarantee ("without inheriting the previous actor's approval") never fires.
surface_transition is gated on bool(self._executions). _executions is only non-empty when IssueRef.work_item_id was supplied, and the only production starter — orchestrator/temporal/start.py:67 — constructs IssueRef(issue_url=issue_url) with no work_item_id. So for every real execution:
- loop starts,
_executions == [],_work_item_id == "" - alice approves on github →
_approved = True resume({"work_item_id": "wi-1", "execution_id": "e2", "surface": "telegram", "actor_kind": "human", "actor_id": "mallory"})bool(self._executions)isFalse→surface_transition = False→_approvedstaysTrue
mallory on telegram inherits alice's github approval, and the new gate at dev_loop.py:1246 is a no-op. Every test in TestWorkContextResume passes work_item_id="wi-1" at start, so none of them covers the shape production emits.
The bool(self._executions) guard looks intended to avoid calling the very first resume a "transition", but that case is exactly the one that must fail closed — there is no recorded surface/actor to compare against, so the prior approval cannot be shown to belong to the resuming actor. Suggest treating "no recorded current surface/actor" as a transition, and adding a test that starts IssueRef(issue_url=...) with no work_item_id.
There was a problem hiding this comment.
Fixed in 6858c93: the bool(self._executions) gate is gone — with no recorded baseline, any declared surface/actor counts as a transition (fail closed). Proven by test_prod_shaped_start_resume_transition_fires_and_flip_waits, which starts the loop exactly as orchestrator/temporal/start.py does (no work_item_id) and asserts the transition fires and approval is not inherited.
| # every history that never calls `resume` — `_approved` is already | ||
| # True, so `wait_condition` resolves immediately and schedules | ||
| # nothing, leaving old histories' command stream untouched. | ||
| if workflow.patched("work-context-resume"): |
There was a problem hiding this comment.
P2 — this second gate is placed too late: a resume that lands in the first gap still produces a durable gitops approval flip attributed to "unknown".
run reads _approver at dev_loop.py:1177, inside the atomic_approve branch:
approve_result = await _run_cwft(
"mctl-agents-approve",
{"service": target_repo, "slug": slug, "approver": self._approver or "unknown"},That call site sits between the original wait_condition (:1102) and this new one (:1246). resume sets _approved = False and _approver = None (:1053-1054). So if a surface/actor-changing resume arrives while the loop is awaiting find_proposal_slug — a real activity await, exactly the gap this comment says the resume can land in — the loop proceeds straight into mctl-agents-approve and commits proposed -> accepted into agents-state with approver: "unknown", i.e. a durable approval record written after approval was revoked and attributed to nobody. The comment at :1170-1176 is explicit that this flip's whole point is that the .status.yaml approval block and the gitops commit message carry the approver.
The new gate then correctly blocks the implement CWFT, so the outcome is "accepted proposal, unknown approver, loop parked" rather than an unapproved implement — bad but not the worst case. Still, either the re-check belongs before the approve flip (or _approver should be snapshotted at :1102 rather than cleared out from under the flip), and there is no test for a resume arriving in that window.
There was a problem hiding this comment.
Fixed in 6858c93: the same patched wait_condition now also runs immediately before the atomic approve CWFT, so a resume landing in the slug-lookup gap re-arms approval before the flip — the gated-slug test drives a resume into exactly that gap and asserts the flip carries the re-approver (bob), never "unknown".
| return | ||
|
|
||
| surface_raw = payload.get("surface") | ||
| surface = SurfaceRef(kind=surface_raw) if isinstance(surface_raw, str) and surface_raw else SurfaceRef() |
There was a problem hiding this comment.
P2 — a resume payload that simply omits surface and actor_kind fails open: it is accepted, recorded as a new execution, and never clears approval.
surface defaults to SurfaceRef() and actor to ActorRef() when the keys are missing or are not non-empty strings. surface_transition at :1035-1038 then requires bool(surface.kind) / bool(actor.kind), so both disjuncts are False for resume({"work_item_id": "wi-1", "execution_id": "e2"}) — no surface, no actor_kind, therefore surface_transition = False and _approved is left untouched.
The execution is still appended to _executions, and _current_surface/_current_actor keep the previous actor's values, so work_context also reports the new execution as belonging to the old actor. An unidentified resumer is precisely the case that cannot be shown to be the approving actor, so it should count as a transition rather than as "nothing changed". test_a_mismatched_work_item_id_is_rejected is the only test sending a surface-less payload, and it asserts the rejection path, not this one.
Related: the closed vocabularies are not applied on this path. SURFACE_KINDS/ACTOR_KINDS are enforced for the CLI flags (run_issue_investigator.py:2377-2380), but any arbitrary string is accepted as surface.kind here.
There was a problem hiding this comment.
Fixed in 6858c93: a payload missing surface or actor_kind is now rejected and recorded with reason="surface-or-actor-missing" (no execution recorded, approval untouched because nothing was accepted); covered by test_resume_without_surface_or_actor_is_rejected_not_inherited.
| resume_from_execution_id: str | None = None, | ||
| surface: str | None = None, | ||
| actor_kind: str | None = None, | ||
| actor_id: str | None = None, |
There was a problem hiding this comment.
P2 — five of the six new parameters are accepted, documented as "#267's work-context seam", and then never read.
Only work_item_id is used (:1720-1759). execution_id, resume_from_execution_id, surface, actor_kind and actor_id have no reference anywhere in the function body; grepping the file only hits the signature, the argparse block, and the call-through at :2455-2463.
Consequences:
_work_context_from_argsvalidates--surface/--actor-kindagainst the closed vocabularies and rejects--resume-from-execution-idwithout--work-item-id, and then every accepted value is silently discarded. An operator passing--surface telegram --actor-id carolgets exit 0 and zero recorded provenance.- No
WorkContextRefis ever constructed by a producer. This PR adds the wholeWorkContextRefblock toContextSnapshotplus itsseal()/_content_payload()wiring, and the investigator is the one place that seals a snapshot (_assemble_context->context_assembly.assemble->seal()), but it never passeswork_context=. Soseal(work_context=...)has no call site outside tests, and none of the new snapshot fields can ever be populated in production.
Either thread them through to the sealed snapshot, or drop the unused parameters and their flags until the producer exists. As written they read as delivered functionality that does nothing.
There was a problem hiding this comment.
Fixed in 6858c93: all six flags now reach seal() — investigate() folds the resolved WorkItem + caller provenance into a WorkContextRef via _work_context_ref (unit-tested) and threads it through _assemble_context → context_assembly.assemble → seal(work_context=). Behaviour at mode=off is unchanged by design (the documented rollout gate).
| "kind": KIND, | ||
| "execution": execution.to_dict(), | ||
| "step": step.to_dict() if step is not None else None, | ||
| "work_context": work_context.to_dict() if work_context is not None else None, |
There was a problem hiding this comment.
P2 — the stated justification for moving every content_hash is out of date, and API_VERSION is not bumped.
The commit message says: "the module has no production producer yet, so the only affected artifact is this golden fixture", citing the docstring at context_snapshot.py:19-21. That docstring is stale as of this branch's parent (#394, issue-265). There is a production producer today:
orchestrator/context_assembly.py:709callsseal()orchestrator/run_issue_investigator.py:1575-1620(_assemble_context) drives it inshadow/onmoderun_issue_investigator.py:1930callswrite_status_yaml(staging, issue, snapshot=context.snapshot), which persistssnapshot_idandcontent_hashinto the gitops.status.yaml(:1077-1082)
So this change means: every snapshot_id already committed to agents-state becomes unreproducible from a re-seal of the same inputs, and two images both declaring api_version: context.mctl.ai/v1alpha1 compute different content_hash values for byte-identical documents. ADR 009 sec. 2 defines content_hash as the hash over every field at a given api_version; silently changing the hashed payload without a version bump breaks that.
Adding work_context to the payload only when it is not None would keep every existing document's hash stable while still distinguishing two executions that carry one (which is what test_two_executions_differing_only_in_execution_id_seal_to_different_snapshot_ids actually asserts). If the unconditional form is deliberate, API_VERSION needs to move and the stale docstring at :19-21 should be corrected in the same change.
There was a problem hiding this comment.
Fixed in 6858c93 the other way around: work_context now participates in content_hash only when present, so persisted #394-producer snapshots keep their identity at an unchanged API_VERSION, and the golden fixture's original hash is restored. Rationale in _content_payload's docstring; test_null_work_context_leaves_the_hash_unchanged_and_present_changes_it covers both directions incl. the ADR-011 execution_id distinction.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 5 P2, 4 P3.
P2 summary:
surface_transitionis gated onbool(self._executions), which is always empty for workflows started bytemporal/start.py:67(nowork_item_id). The headline guarantee — a resume from a different surface/actor does not inherit approval — never fires in production, and no test covers that shape.- A resume payload that omits
surface/actor_kindfails open: accepted, recorded, approval untouched. The signal path also does not applySURFACE_KINDS/ACTOR_KINDS. - The new approval re-check sits after the
mctl-agents-approveCWFT, so a resume landing in the first gap still commits a durable gitopsproposed -> acceptedflip withapprover: "unknown". investigate()accepts six work-context parameters and reads one; the other five, and their validated CLI flags, are discarded. No producer ever passeswork_context=toseal(), so the newWorkContextRefblock cannot be populated._content_payloadaddswork_contextunconditionally, moving everycontent_hash/snapshot_idat an unchangedAPI_VERSION. The justification ("no production producer yet") is stale:context_assembly.assembleseals andwrite_status_yamlpersistssnapshot_id/content_hashinto gitops.
The rollout gating, the work_context package structure, and the contract/rollout unit tests all look solid; the gaps are in the wiring and the fail-open edges.
…e flip, and seal work_context Review pass on #408 (claude 5×P2, agy corroborating two): - surface_transition no longer gated on the seeded-execution list, which is empty on every production start: with no recorded baseline any declared surface/actor counts as a transition, so the first cross-surface resume clears approval instead of inheriting it. - a resume omitting surface/actor_kind is rejected and recorded (surface-or-actor-missing) rather than accepted without ever clearing approval; rejections dedupe on (execution_id, reason). - the approval re-check now also runs immediately before the atomic approve flip, so a resume landing in the slug-lookup gap can no longer commit a gitops approval attributed to "unknown". - the six work-context flags now reach seal(): investigate() folds the resolved WorkItem and caller provenance into a WorkContextRef (_work_context_ref) threaded through _assemble_context and context_assembly.assemble into the sealed snapshot. - work_context participates in content_hash only when present, so already-persisted snapshots (the #394 shadow producer writes snapshot_ids into .status.yaml) keep their identity at an unchanged API_VERSION; the golden fixture's original hash is restored. - P3s: answer_from guards a non-dict 404 payload; WorkContextRef id fields and prior_execution_ids are length-bounded; test-file imports moved to the top. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
…-feat-work-context-resume-investigator-fr # Conflicts: # orchestrator/run_issue_investigator.py # orchestrator/temporal/workflows/dev_loop.py
…-slug test main's #410 stale-issue gate added a get_issue_state activity to the run path; the hand-built activity list in the new prod-shaped resume test needs its open-issue default fake like every other hand-built list here. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
|
Claude finished @mashkovd's task in 5m 15s —— View job Review complete — changes requestedReviewed
Previous round: all five P2s fixed
The four P3s (404 type guard, unbounded New P2 findings
P3 findings
What looks goodThe conditional I could not run the test suite in this environment (Bash approval for |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit Finding 1 (P1)
Finding 2 (P2)
if execution.surface.kind and execution.surface.kind not in SURFACE_KINDS:
return WORK_ITEM_UNKNOWN
if execution.actor.kind and execution.actor.kind not in ACTOR_KINDS:
return WORK_ITEM_UNKNOWNFinding 3 (P2)
Finding 4 (P3)
|
| # implement CWFT covers the later gaps (implementer resolve, | ||
| # the flip itself). | ||
| if workflow.patched("work-context-resume"): | ||
| await workflow.wait_condition(lambda: self._approved) |
There was a problem hiding this comment.
P2 — this gate (and its twin at :1642) re-opens the unbounded, escape-hatch-free park that mctl-agents#420 removed.
The original approval park two hundred lines up is deliberately wait_condition(lambda: self._approved or self._abandoned), wrapped in a poll loop bounded by APPROVAL_WAIT_DEADLINE that also re-reads the issue's state on every boundary. Its own comment is explicit about why:
mctl-agents#420: an unbounded wait_condition here never released on its own -- not on the source issue closing, not on an
abandonsignal, not ever, ifapprovenever arrived
Both new gates are wait_condition(lambda: self._approved): no _abandoned, no timeout, no issue-closed poll.
Failure scenario:
- loop parks at the original gate, alice approves,
_approved = True - loop enters
find_proposal_slug resume({"work_item_id": "wi-1", "execution_id": "e2", "surface": "telegram", "actor_kind": "human", "actor_id": "bob"})— surface transition, so_approved = False- bob changes his mind, or is the wrong person, or never comes back
- the loop blocks here forever
abandon cannot release it, so the only recovery is Temporal terminate — which, per abandon's own docstring, skips _watch_pr's finally and therefore leaks the lifecycle-ownership row. test_prod_shaped_start_resume_transition_fires_and_flip_waits parks on exactly this gate and only ever escapes it with a second approve; no test sends abandon while parked here.
Suggest lambda: self._approved or self._abandoned on both gates, an if self._abandoned: return DevLoopResult(...) immediately after each, and reusing the APPROVAL_WAIT_DEADLINE bound (or at minimum some finite one) so a resumed-but-never-re-approved loop expires the way the first park does.
There was a problem hiding this comment.
Fixed in 84ceb4d — both re-approval gates now go through _await_reapproval, which waits on self._approved or self._abandoned with timeout=APPROVAL_WAIT_DEADLINE; abandon ends the loop with ended="abandoned: …" and timeout with "re-approval wait expired". Covered by test_abandon_releases_the_reapproval_park.
| if item.state not in WORK_ITEM_STATES: | ||
| return WORK_ITEM_UNKNOWN | ||
| for execution in item.executions: | ||
| if execution.surface.kind not in SURFACE_KINDS: |
There was a problem hiding this comment.
P2 — this check defeats the tolerance ExecutionRef.from_payload deliberately provides, and at enforce/only it blocks the run.
ExecutionRef.from_payload (:155-166) treats an absent surface/actor block as legitimate, with an explicit comment saying so:
ABSENT is simply an execution predating this field (or one from a surface that never reported one).
It parses those into the defaults SurfaceRef() / ActorRef(), whose kind is "". "" is not in SURFACE_KINDS or ACTOR_KINDS, so work_item_verdict_for returns WORK_ITEM_UNKNOWN for every such work item.
Failure scenario — mctl-api returns a perfectly valid record whose executions predate the surface/actor fields:
{"work_item_id": "wi-1", "state": "in-progress",
"executions": [{"execution_id": "e1", "sequence": 1}]}WorkItem.from_payload accepts it, work_item_verdict_for answers WORK_ITEM_UNKNOWN. In run_issue_investigator.investigate() (:1836) the answer.verdict == WORK_ITEM_FOUND branch is skipped, so at observe no provenance is ever recorded and no work_context block is sealed; at enforce/only with the default WORK_CONTEXT_REQUIRED=true, blocks_on_unknown() is true and the investigation is refused outright with "work item 'wi-1' could not be resolved: " — and answer.reason is "" on this path, so the operator gets a rejection with no stated cause.
Note also that this is the one place the workflow's own executions land: dev_loop.run seeds execution #1 with no surface/actor at all (dev_loop.py:1367-1375), so the shape this rejects is the shape this repo itself produces.
Suggest matching ContextSnapshot.validate's pattern — only check a non-empty value:
for execution in item.executions:
if execution.surface.kind and execution.surface.kind not in SURFACE_KINDS:
return WORK_ITEM_UNKNOWN
if execution.actor.kind and execution.actor.kind not in ACTOR_KINDS:
return WORK_ITEM_UNKNOWNNo test covers this: test_every_documented_state_classifies_as_found uses executions=[], and every investigate() test builds WorkItemAnswer(verdict=WORK_ITEM_FOUND, ...) by hand rather than going through answer_from. Worth a case asserting that a WorkItem whose executions carry no surface/actor still classifies WORK_ITEM_FOUND.
There was a problem hiding this comment.
Fixed in 84ceb4d — work_item_verdict_for now only rejects a NON-empty out-of-vocabulary kind; executions predating the surface/actor fields classify FOUND (test_kindless_executions_still_classify_found). The resume signal keeps the strict fail-closed check for missing provenance.
|
|
||
| answer = WorkItemClient().get(work_item_id) | ||
| if answer.verdict == WORK_ITEM_FOUND and answer.item is not None: | ||
| canonical = reconstruct_canonical_state(answer.item, proposal_dir, ()) |
There was a problem hiding this comment.
P2 — prior_digests is hard-coded to () here, so the sealed work_context block always under-reports the work item's sibling executions.
reconstruct_canonical_state derives prior_execution_ids only from its prior_digests argument (work_context/contract.py:429-433); it never reads item.executions. Passing () means canonical.prior_execution_ids is unconditionally empty in production.
_work_context_ref then computes both correlation fields from that empty tuple (:1740-1745):
prior_ids = tuple(canonical.prior_execution_ids) # always ()
if resume_from_execution_id and resume_from_execution_id not in prior_ids:
prior_ids = prior_ids + (resume_from_execution_id,)
...
execution_sequence=len(prior_ids) + 1,
prior_execution_ids=prior_ids,Failure scenario: wi-1 has four recorded executions and mctl-api returns all four in item.executions. This run is the fifth, invoked with --work-item-id wi-1 --execution-id e5 and no --resume-from-execution-id. The sealed snapshot carries prior_execution_ids: [] and execution_sequence: 1 — it claims to be the first execution of a work item that has run four times. With --resume-from-execution-id e4 it claims execution_sequence: 2. Those values are content-hashed and land in .status.yaml via write_status_yaml, so the wrong correlation is durable.
item.executions is right there on the resolved WorkItem and already carries execution_id and sequence; deriving prior_execution_ids (and the sequence) from it — merging prior_digests in as an additional source — would make the block say what it claims to say.
test_work_context_ref_folds_flags_and_canonical_state doesn't catch this because it constructs CanonicalState(prior_execution_ids=("e1",)) directly rather than going through reconstruct_canonical_state.
There was a problem hiding this comment.
Fixed in 84ceb4d — reconstruct_canonical_state derives priors from the store's own item.executions (ordered by sequence) merged with prior_digests, and _work_context_ref excludes the current execution from its own priors. Covered by test_reconstruct_priors_come_from_item_executions_and_merge_digests.
| accepted=not body_empty, | ||
| ) | ||
| verdict = work_item_verdict_for(item) | ||
| reason = "" if verdict != WORK_ITEM_UNKNOWN else f"unrecognised work item state {item.state!r}" |
There was a problem hiding this comment.
P3 — this reason attributes every WORK_ITEM_UNKNOWN to item.state, but work_item_verdict_for has two other ways to return it.
If item.state is "open" and an execution carries surface.kind == "carrier-pigeon", the answer is reason="unrecognised work item state 'open'" — a message naming a value that is in fact valid. That string is what investigate() prints on the blocks_on_unknown() path (run_issue_investigator.py:1863), so an operator debugging a blocked run is pointed at the wrong field.
Either have work_item_verdict_for return (verdict, reason), or check item.state not in WORK_ITEM_STATES here before claiming the state is the cause.
There was a problem hiding this comment.
Fixed in 84ceb4d — new work_item_unknown_reason(item) names the actual offending state/field, and answer_from uses it for the UNKNOWN reason string (test_unknown_reason_names_the_actual_cause).
| # parser (stdlib only) and does not need one — reconstruction only ever | ||
| # reads the single `status:` scalar, never anything nested, so a plain | ||
| # regex is the whole parser this needs and adds no dependency. | ||
| _STATUS_LINE_RE = re.compile(r'^status:\s*"?([A-Za-z0-9_-]+)"?\s*$', re.MULTILINE) |
There was a problem hiding this comment.
P3 — the regex only tolerates double quotes, so a single-quoted scalar reads as no status at all.
status: 'accepted' does not match ("? covers only ", and ' is outside [A-Za-z0-9_-]), so _read_prior_status returns "" and CanonicalState.prior_status silently claims the proposal has no status. YAML emitters pick the quoting style, and .status.yaml is also hand-edited, so both spellings are reachable.
_STATUS_LINE_RE = re.compile(r"""^status:\s*['"]?([A-Za-z0-9_-]+)['"]?\s*$""", re.MULTILINE)There was a problem hiding this comment.
Fixed in 84ceb4d — _STATUS_LINE_RE now accepts single or double quotes around the status scalar (test_status_scalar_reads_single_and_double_quotes).
| help="Canonical WorkItem id to resume against (mctlhq/mctl-agents#267)", | ||
| ) | ||
| ap.add_argument( | ||
| "--execution-id", default=None, |
There was a problem hiding this comment.
P3 — the help text promises a derivation that does not exist, and omitting the flag silently disables the whole feature.
"derived deterministically when omitted" describes contract.execution_id_for, which is implemented and unit-tested but has no call site in this module. What actually happens when --execution-id is omitted is investigate()'s if execution_id: guard (:1848) being false, so work_context_ref stays None and no work_context block is sealed — at observe and above alike, with no warning.
So --work-item-id wi-1 --surface telegram --actor-id carol (no --execution-id) contacts the store, logs the canonical state, and seals a snapshot with no provenance at all — which looks like the feature working. Either call execution_id_for(work_item_id, sequence, attempt) to make the help true, or say plainly that work_context is only sealed when --execution-id is given.
There was a problem hiding this comment.
Fixed in 84ceb4d — an omitted --execution-id is now genuinely derived via execution_id_for(work_item_id, len(priors)+1, "cli") in the WORK_ITEM_FOUND branch, and the help text describes exactly that.
| assert first.surface_transition is False | ||
|
|
||
|
|
||
| def test_provenance_is_recorded_at_mode_observe(tmp_path, monkeypatch, capsys): |
There was a problem hiding this comment.
P3 — every investigate() work-context test passes dry_run=True, so nothing exercises the seal-side wiring end to end.
dry_run=True returns before _assemble_context is ever reached, which means the one path this round added — work_context_ref → _assemble_context(work_context=...) → assemble_investigator_context → assemble → seal(work_context=...) — has no test crossing it. _work_context_ref is covered in isolation and seal(work_context=...) is covered in test_context_snapshot.py, but the join between them is not, and that join is exactly what the previous round's "no producer ever passes work_context=" finding was about.
A dry_run=False case with the SDK/clone steps faked, asserting the sealed snapshot's work_context.work_item_id and execution_id, would pin it.
There was a problem hiding this comment.
Fixed in 84ceb4d — test_seal_side_wiring_carries_work_context_end_to_end drives investigate() through the real assemble_investigator_context entry and asserts the SEALED snapshot carries the derived work_context (execution_id_for("wi-1", 2, "cli"), sequence 2).
| return | ||
|
|
||
| surface_raw = payload.get("surface") | ||
| surface = SurfaceRef(kind=surface_raw) if isinstance(surface_raw, str) and surface_raw else SurfaceRef() |
There was a problem hiding this comment.
P3 — the signal path still accepts any string as surface.kind / actor_kind, while the CLI enforces the closed vocabularies.
_work_context_from_args rejects --surface carrier-pigeon (run_issue_investigator.py:2500-2503), but resume({"surface": "carrier-pigeon", "actor_kind": "alien", ...}) is accepted here and appended to _executions. This fails closed (it counts as a transition and clears approval), so it is not a security hole — but it puts out-of-vocabulary values into work_context's query response and, if those executions are ever mirrored back into a WorkItem, work_item_verdict_for will read the whole item as WORK_ITEM_UNKNOWN.
SURFACE_KINDS/ACTOR_KINDS are already imported into this module's imports_passed_through block; rejecting with reason="surface-or-actor-unrecognised" next to the existing surface-or-actor-missing branch would make the two entry points agree.
There was a problem hiding this comment.
Fixed in 84ceb4d — the resume signal now enforces the same closed SURFACE_KINDS/ACTOR_KINDS vocabularies and rejects with surface-or-actor-unrecognised (extended test_resume_without_surface_or_actor_is_rejected_not_inherited).
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 3 P2, 5 P3.
All five P2s from the eabb5f6 round are genuinely fixed: surface_transition no longer gates on bool(self._executions) and is covered by a production-shaped start; a resume omitting surface/actor_kind is now rejected and recorded; the approval re-check is duplicated before the mctl-agents-approve flip; the investigator threads all six work-context parameters into a sealed WorkContextRef; and _content_payload adds work_context only when present, so existing content_hash values are untouched. The bounded-length rule, the answer_from 404 type guard, the _reject_resume dedupe and the E402 imports are all addressed too.
New P2s in this round:
dev_loop.py:1567/:1642— the two newwait_condition(lambda: self._approved)gates observe neither_abandonednor any deadline, re-opening the unbounded park mctl-agents#420 removed. A resume that clears approval and is never re-approved wedges the loop with no escape butterminate, which skips the ownership release.work_context/contract.py:248—work_item_verdict_forrejects the absentsurface/actorblocks thatExecutionRef.from_payloadexplicitly tolerates (and thatdev_loop.run's own seeded execution produces), so atenforce/onlywith the defaultWORK_CONTEXT_REQUIRED=truea valid work item blocks the run — with an emptyreason.run_issue_investigator.py:1838—reconstruct_canonical_state(..., ())hard-codes emptyprior_digestsanditem.executionsis never read, so the sealed block reportsexecution_sequence: 1andprior_execution_ids: []for a work item that has already run N times. Those values are content-hashed and persisted to.status.yaml.
P3s: the WORK_ITEM_UNKNOWN reason mis-attributes every cause to item.state; _STATUS_LINE_RE misses single-quoted scalars; --execution-id's help promises a derivation that does not exist and omitting it silently disables the block; no investigate() test crosses into _assemble_context, so the new producer-to-seal join is untested; and the resume signal still skips the closed vocabularies the CLI enforces.
I could not run the test suite in this environment (Bash approval for uv run pytest was declined), so all findings are from static reading of the pinned diff.
…dicts, real prior-execution correlation Round-2 review pass (claude 3×P2 + 5×P3, agy P1/P2 corroborating): - both re-approval gates now observe `_abandoned` and expire at APPROVAL_WAIT_DEADLINE (`_await_reapproval`), instead of re-opening the escape-hatch-free park #420 removed; an abandon while parked ends the loop gracefully, tested. - `work_item_verdict_for` only rejects NON-EMPTY out-of-vocabulary surface/actor kinds — an absent block (an execution predating the fields, or dev_loop's own seed) no longer turns the whole item UNKNOWN and no longer blocks investigation at enforce/only. - `reconstruct_canonical_state` derives prior_execution_ids from the store's own `item.executions` (ordered by sequence) merged with prior_digests, so a fifth execution no longer seals as execution #1; `_work_context_ref` excludes the current execution from its own priors. - an omitted --execution-id now really is derived deterministically (execution_id_for, attempt salt "cli"), as the help text promised. - UNKNOWN reasons name the actual offending field (work_item_unknown_reason); `.status.yaml` status scalars read single quotes too; the resume signal enforces the same closed vocabularies as the CLI (surface-or-actor-unrecognised). - seal-side wiring pinned end to end: investigate() with dry_run=False drives the real assembly entry and the sealed snapshot carries the derived work_context (plus an assemble()-level threading test). - lint: RUF005 tuple unpacking, I001 import sort. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
|
Claude finished @mashkovd's task in 6m 39s —— View job Review complete — changes requestedReviewed
Previous round: all three P2s and all five P3s fixed
P3s too: New P2 findings
P3 findings
What looks goodThe conditional I could not run the test suite in this environment (Bash approval for |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit FindingsFinding 1 (P3)
Finding 2 (P3)
|
| raise ContextSnapshotError( | ||
| f"work_context.{field_name} exceeds {MAX_WORK_CONTEXT_ID_LENGTH} characters" | ||
| ) | ||
| if len(wc.prior_execution_ids) > MAX_PRIOR_EXECUTION_IDS: |
There was a problem hiding this comment.
P3 — this ceiling is enforced but never respected by the only producer, so a store answer with 65 executions fails the investigation rather than being clamped.
seal() calls validate() (:1137), so this raises ContextSnapshotError out of seal → assemble → _assemble_context, which re-raises at ISSUE_INVESTIGATOR_CONTEXT_MODE=on (run_issue_investigator.py:1683). _work_context_ref passes canonical.prior_execution_ids straight through, and reconstruct_canonical_state now takes them from item.executions — an mctl-api-supplied, unbounded list.
So a legitimately long-lived work item (65+ recorded executions) turns a valid store response into a hard investigation failure at on, and a silent warn:-and-skip at shadow — the same class of "strict schema refuses a shape the tolerant mirror accepts" the work_item_verdict_for fix in this commit just removed.
The ceiling itself is right (ADR 009 sec. 7); the producer should stay inside it. Keeping the most recent MAX_PRIOR_EXECUTION_IDS entries in _work_context_ref — they are already sorted by sequence — makes the bound a clamp rather than a trap. Worth a test with 65 executions either way, since nothing exercises the boundary from the producer side today.
There was a problem hiding this comment.
Fixed in b82be1d — _work_context_ref computes the sequence from the full prior list, then clamps prior_execution_ids to the newest MAX_PRIOR_EXECUTION_IDS entries, so a 65-execution work item seals instead of raising out of validate() at mode on. Covered by test_prior_ids_clamp_to_the_ceiling_but_the_sequence_counts_all (sequence 66, 64 kept, oldest dropped).
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.
All three P2s from the a58c85f round are fixed, and verified in the diff: _await_reapproval now observes _abandoned and expires at APPROVAL_WAIT_DEADLINE (with test_abandon_releases_the_reapproval_park driving an abandon while parked); work_item_verdict_for only rejects non-empty out-of-vocabulary kinds, with test_kindless_executions_still_classify_found; and reconstruct_canonical_state derives priors from item.executions ordered by sequence, merged with prior_digests, pinned by test_reconstruct_priors_come_from_item_executions_and_merge_digests. The five P3s are addressed too — work_item_unknown_reason names the offending field, _STATUS_LINE_RE takes single quotes, --execution-id really is derived, the resume signal enforces SURFACE_KINDS/ACTOR_KINDS, and test_seal_side_wiring_carries_work_context_end_to_end finally crosses the producer-to-seal() join with dry_run=False.
New P2s in this round:
-
dev_loop.py:1152— the work-context state added here is the only instance state not carried acrosscontinue_as_new. Both hop sites dropIssueRef.work_item_id, andMergeWatchResumehas no field for_work_item_id/_executions/_seen_execution_ids/_current_surface/_current_actor/_resume_rejections, despite that dataclass existing precisely to stop such state resetting. After a merge-watch hop thework_contextquery answers empty, thework-item-mismatchguard is vacuous (a resume can rebind the loop to a foreign work item), andresume's duplicate-execution_ididempotency no longer holds. No test crosses a hop. -
run_issue_investigator.py:1841—investigate()never compares the resolvedWorkItem'sissue_url/service/slugwith the issue it resolved from--issue-url. A--work-item-idnaming a different task is accepted: atenforceits terminal state vetoes an unrelated investigation, and otherwise its identity andprior_execution_idsare sealed into this proposal's snapshot and persisted to.status.yaml.DevLoopWorkflow.resumemakes exactly this check (work-item-mismatch); the CLI fails open.
P3s: _work_context_ref's surface_transition compares against origin_surface and ignores the actor, contradicting ExecutionRef.surface_transition's documented meaning in the same PR; the derived --execution-id encodes a sequence one lower than the sealed execution_sequence when --resume-from-execution-id names an unrecorded execution; and MAX_PRIOR_EXECUTION_IDS is enforced in validate() but never clamped by the producer, so a 65-execution work item hard-fails the seal at context mode on.
The rest holds up well: the conditional work_context in _content_payload, the bounded-length ceilings, to_log_dict omitting actor_id, the parent/child equality rule, the rollout ladder, and the workflow.patched("work-context-resume") placement.
I could not run the test suite in this environment (Bash approval for uv run pytest was declined), so all findings are from static reading of the pinned diff.
…heck the work item's issue Round-3 review pass (claude 2×P2 + 3×P3 on 84ceb4d): - MergeWatchResume now carries the work-context binding (work_item_id, executions, seen ids, current surface/actor, rejections) and both hop sites fold it in via _carry_work_context; _resume_merge_watch rehydrates it and re-applies anything gap signals recorded on top of the carried baseline. IssueRef keeps work_item_id across the hop. Without this a hop emptied the work_context query, made the work-item-mismatch guard vacuous and forgot the duplicate-execution_id no-op. - the CLI makes the same identity cross-check the resume signal makes: a --work-item-id whose issue_url disagrees with --issue-url warns (and drops the work context) at observe, refuses at enforce/only — a foreign work item can no longer veto a run or seal its identity into another issue's snapshot. - `surface_transition` in the sealed block now answers ExecutionRef's documented question: changed surface OR actor relative to the LAST recorded execution (falling back to origin), not "differs from origin surface". - the derived --execution-id and the sealed execution_sequence are computed from the same prior list (_prior_execution_ids), so an unrecorded --resume-from-execution-id can no longer skew them. - prior_execution_ids clamps to the newest MAX_PRIOR_EXECUTION_IDS entries (sequence keeps counting all), so a 65-execution work item seals instead of failing validate() at context mode `on`. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
|
Claude finished @mashkovd's task in 5m 32s —— View job Review complete — changes requestedReviewed
Previous round: both P2s and all three P3s fixed
New P2 findingA gap-delivered P3 findings
What looks good
I could not run the test suite in this environment (Bash approval for |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit Finding 1 (P2)
Finding 2 (P2)
Finding 3 (P2)
Finding 4 (P3)
Finding 5 (P3)
Finding 6 (P3)
|
| self._current_surface = resume.current_surface | ||
| self._current_actor = resume.current_actor | ||
| self._resume_rejections = list(resume.resume_rejections) | ||
| for execution in gap_executions: |
There was a problem hiding this comment.
P2 — a resume for a different work item delivered in the continue_as_new gap is re-applied onto the carried binding here, so the work-item-mismatch guard this commit exists to make non-vacuous is still bypassable.
This block deliberately re-applies gap signals, and the comment twenty lines up states this repo's position on the ordering explicitly:
so an
abandonsignal delivered in the continue_as_new gap (applied beforeinitialize_workflowper temporalio's job ordering) is never clobbered by this rehydration
So the gap window is live, and in it resume runs against __init__'s empty state:
- run N is bound to
wi-1, hops resume({"work_item_id": "wi-999", "execution_id": "e7", "surface": "telegram", "actor_kind": "human", "actor_id": "mallory"})lands in the gap.self._work_item_idis"", soif self._work_item_id and work_item_id != self._work_item_idis vacuous — the signal is accepted,e7is appended,_current_surface/_current_actorbecome telegram/mallory- this method then restores
self._work_item_id = "wi-1"— but the loop below re-appendse7unconditionally, becauseExecutionRefcarries nowork_item_idand nothing here re-checks the binding
Result: wi-999's execution is recorded as an execution of wi-1, becomes the surface/actor baseline every later surface_transition is computed against, and is carried forward by _carry_work_context into every subsequent hop and every work_context query answer for the rest of the watch. That is the same "a resume can rebind the loop to a foreign work item" defect the previous round raised, narrowed to the gap rather than closed.
The binding the gap signal wrote is still readable at this point — capture it before the rehydration and gate the re-apply on it:
gap_work_item_id = self._work_item_id
...
for execution in gap_executions:
if resume.work_item_id and gap_work_item_id and gap_work_item_id != resume.work_item_id:
self._reject_resume(execution.execution_id, gap_work_item_id, "work-item-mismatch")
continueNo test covers a gap-delivered resume: test_carried_work_context_is_rehydrated_on_a_resumed_run exercises the rehydration with gap_executions empty, and test_work_context_survives_a_merge_watch_hop sends no resume at all.
There was a problem hiding this comment.
Fixed in 1459dc4 — the gap merge (now _rehydrate_work_context) re-checks the one guard the empty binding made vacuous: when the gap binding disagrees with the carried work_item_id, every gap execution is rejected with work-item-mismatch (recorded via _reject_resume, visible on the query) instead of being grafted; its surface/actor never become the baseline and its pending window is not left open. Unit-tested on a bare instance: test_foreign_gap_resume_is_rejected_not_grafted sends the exact wi-999/e7/mallory gap signal from your scenario, plus test_same_item_gap_resume_is_reapplied_against_the_carried_baseline for the legitimate case.
| key=lambda e: e.sequence, | ||
| default=None, | ||
| ) | ||
| baseline_surface = last.surface.kind if last else item.origin.kind |
There was a problem hiding this comment.
P3 — the baseline this compares against can be this execution itself, and a kindless last execution does not fall back to origin as the commit message says it does.
Two gaps, both in the common shape this repo produces:
-
Self as baseline.
_prior_execution_idsfilterspid != execution_idprecisely because "the store may already have recorded THIS execution"; themax(...)here does not. So--work-item-id wi-1 --execution-id e2 --surface telegram --actor-kind human --actor-id bob, where the store already carriese1(github/alice)ande2(telegram/bob), pickslast = e2— itself — and sealssurface_transition: false. Relative to the execution before it, that is exactly the github→telegram, alice→bob transition the field is defined to report. Addingand e.execution_id != execution_idto the generator fixes it and matches the prior-list rule one function up. -
Kindless baseline never falls back.
baseline_surface = last.surface.kind if last else item.origin.kind— whenlastexists but carries no surface (the absenceExecutionRef.from_payloaddeliberately tolerates, and exactly whatdev_loop.run's seeded execution feat(mctl-docs): fallback to GitHub API when sibling clones absent #1 records),baseline_surfaceis"", sosurface_changedisFalsefor every surface. A work item that originated on github, whose only recorded execution is the kindless dev-loop seed, sealssurface_transition: falsefor a telegram CLI run. The commit message says "falling back to origin", but the fallback only fires when the execution list is empty.
Worth noting the two entry points now disagree on the unprovable case: dev_loop.resume treats an empty baseline as a transition (fail closed, and its docstring argues that explicitly), while this treats it as no transition. Both values are content-hashed into .status.yaml, so a trace view joining the query to the sealed block gets two answers for the same execution. test_surface_transition_baselines_on_the_last_execution_not_origin uses execution_id="e3", which is not in item.executions, so neither case is covered.
There was a problem hiding this comment.
Fixed in 1459dc4 — the baseline now excludes this execution itself (same rule as _prior_execution_ids) and skips kindless executions, taking the newest PRIOR execution with a known kind and falling back to origin. Both of your shapes are pinned in test_surface_transition_baseline_excludes_self_and_kindless_executions: store-recorded e2 as self → transition True; kindless dev_loop-seed last → falls through to e1's github.
| # a --work-item-id about a DIFFERENT issue must not veto this | ||
| # run or seal its identity into this issue's snapshot. Warn | ||
| # at `observe`, refuse where the store's answer has teeth. | ||
| if canonical.issue_url and canonical.issue_url != issue.ref.url: |
There was a problem hiding this comment.
P3 — the new cross-check is an exact string compare against the raw --issue-url, so two spellings of the same issue refuse the run at enforce.
IssueRef.url is url.strip() verbatim (run_issue_investigator.py:264) and _ISSUE_URL_RE (:197-199) accepts http:// and an optional trailing slash:
r"^https?://github\.com/([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+)/issues/(\d+)/?$"So --issue-url https://github.com/mctlhq/mctl-telegram/issues/103/ --work-item-id wi-1 against a store record whose issue_url is https://github.com/mctlhq/mctl-telegram/issues/103 is refused at enforce/only with
work item wi-1 is about mctlhq/mctl-telegram#103, not mctlhq/mctl-telegram#103 — work-item mismatch
— two visually identical URLs and a legitimate investigation blocked. At observe the same pair silently drops the work context, so the block the feature exists to seal is never written and nothing says why beyond that message.
The parse is already available: comparing try_parse_issue_url(canonical.issue_url)'s (owner, repo, number) with issue.ref's (and falling back to the string compare only when the store's value does not parse) keeps the guard's teeth while accepting the same issue written two ways.
There was a problem hiding this comment.
Fixed in 1459dc4 — the cross-check now compares _canonical_issue_key on both sides (owner/repo#number via _ISSUE_URL_RE, case-folded; unparseable falls back to the stripped string and never silently equals a parseable one). Trailing slash, http://, and case differences no longer refuse a run at enforce — test_canonical_issue_key_compares_identities_not_spellings.
| seen_execution_ids=tuple(sorted(self._seen_execution_ids)), | ||
| current_surface=self._current_surface, | ||
| current_actor=self._current_actor, | ||
| resume_rejections=tuple(self._resume_rejections), |
There was a problem hiding this comment.
P3 — _resume_pending is the one piece of the resume state this does not carry, so the "one resume already pending" invariant silently drops at every hop.
Five fields go across; _resume_pending does not, and _resume_merge_watch does not rehydrate it either. It is only ever cleared by approve (:1217), and the continued run force-sets self._approved = True (:1796) without touching it, so the two halves disagree in both directions:
- a resume accepted just before the hop left
_resume_pending = True; after the hop it isFalse, and a differentexecution_idthat would have been rejectedresume-already-pendingis now accepted — the documented idempotency property the rest of this commit went to some length to preserve - a resume accepted in the gap leaves
_resume_pending = Trueon a run where nothing ever callsapprove(the watch has no approval gate), so every later resume is rejectedresume-already-pendingfor the rest of the watch
Either carry it alongside the others, or say in the comment above that the pending window deliberately ends at the hop boundary — right now the omission reads as an oversight next to the five fields that are carried.
There was a problem hiding this comment.
Fixed in 1459dc4 — resume_pending is carried in MergeWatchResume/_carry_work_context and rehydrated (not OR-ed: the loop re-derives the window for gap executions it actually accepts, so a rejected foreign gap resume cannot hold it open). Round-trip pinned in test_carry_work_context_round_trips_through_rehydration.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.
Both P2s from the 84ceb4d round are fixed and verified in the diff: MergeWatchResume now carries the work-context binding, _carry_work_context folds it in at both hop sites, IssueRef.work_item_id crosses the boundary, and _resume_merge_watch rehydrates it before the first await (covered by test_work_context_survives_a_merge_watch_hop and test_carried_work_context_is_rehydrated_on_a_resumed_run); and the CLI now makes the work-item/issue identity cross-check, warning at observe and refusing at enforce. The three P3s are addressed too — one _prior_execution_ids feeds both the derived id and the sealed sequence, surface_transition baselines on an execution rather than the origin, and prior_execution_ids clamps to MAX_PRIOR_EXECUTION_IDS instead of hard-failing validate().
New P2:
- dev_loop.py:1844 — a
resumenaming a DIFFERENT work item, delivered in the continue_as_new gap, is accepted against the empty binding (the mismatch guard is vacuous there) and then re-applied onto the carried binding by this loop, which never re-checkswork_item_id. The foreign execution becomes an execution of the carried work item, becomes the surface/actor baseline, and is carried into every later hop. The module's own comment at :1820 says gap signals are applied beforeinitialize_workflow, so the window is live. No test sends a resume into the gap.
P3s: _work_context_ref's surface_transition baseline can be the current execution itself (item.executions is not filtered by execution_id the way _prior_execution_ids is) and a kindless last execution never falls back to origin; the issue cross-check compares raw URL strings while _ISSUE_URL_RE accepts http:// and a trailing slash, so an equivalent URL refuses the run at enforce; and _resume_pending is the one resume field not carried across the hop, so the already-pending invariant drops at the boundary.
The rest holds: the conditional work_context in _content_payload, the bounded-length ceilings and the new clamp, to_log_dict omitting actor_id, the parent/child equality rule, the tolerant-mirror vs strict-schema split, the rollout ladder, _await_reapproval, and the workflow.patched("work-context-resume") placement.
I could not run the test suite in this environment (Bash approval for uv run pytest was declined), so all findings are from static reading of the pinned diff.
…-feat-work-context-resume-investigator-fr # Conflicts: # orchestrator/run_issue_investigator.py # tests/test_run_issue_investigator.py
…oad blocks, compare issue identities Round-4 review pass (mypy, claude 1×P2 + 3×P3 on b82be1d, agy FAIL:P2 F1-F5): - a `resume` naming a foreign work item delivered in the continue_as_new gap (where the mismatch guard runs against the empty binding) is now rejected with `work-item-mismatch` during rehydration instead of being grafted onto the carried work item; the merge lives in `_rehydrate_work_context`, unit-tested on a bare instance. `_resume_pending` is carried across hops and re-derived for accepted gap executions only. - `SurfaceRef`/`ActorRef.from_payload` treat a kindless block (`{"kind": ""}`, `{}`) as legitimately absent — a record serialized from a default ref, like dev_loop's seed — instead of aborting the whole WorkItem parse into UNKNOWN; a non-string kind stays malformed. `work_item_verdict_for` also classifies an out-of-vocabulary `origin.kind` (which validate() would reject at the seal) with a named reason. - the work-item/issue cross-check compares canonical issue identities (`owner/repo#number`, case-folded) instead of raw strings, so http:// or a trailing slash no longer refuses a legitimate run at enforce. - `surface_transition`'s baseline excludes this execution itself and skips kindless executions, falling back through the newest known kind to origin. - removed `WorkItemClient.list_executions`: `GET .../executions` returns a listing, not a WorkItem envelope, so `answer_from` was the wrong reader — and nothing consumes the route; a typed parser comes with the first real consumer. - mypy: `canonical` is declared `CanonicalState | None` via a non-optional `resolved` intermediate (the observe-mode mismatch path sets it to None). - merged origin/main (#393 execution-identity); conflicts in run_issue_investigator.py imports and test layout resolved keep-both-sides. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
|
Claude finished @mashkovd's task in 5m 15s —— View job Review complete — changes requestedReviewed
Previous round: the P2 and all three P3s fixed
The agy round is answered too: kindless New P2 findingThe gap window closed for P3 findings
What looks goodThe conditional I could not run the test suite in this environment (Bash approval for |
|
Response to agy's round on b82be1d (VERDICT FAIL:P2) — addressed in 1459dc4:
Also in 1459dc4: the mypy lint failure on b82be1d fixed, and origin/main (#393 execution-identity) merged in. |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit Findings[P3] orchestrator/temporal/workflows/dev_loop.py:~1269Anchor:
[P3] orchestrator/work_context/contract.py:~254Anchor:
[P3] orchestrator/work_context/client.py:~130Anchor:
|
| gap_is_foreign = bool( | ||
| gap_work_item_id and self._work_item_id and gap_work_item_id != self._work_item_id | ||
| ) | ||
| for execution in gap_executions: |
There was a problem hiding this comment.
P2 — the gap window this commit closed for work-item-mismatch is still open for resume-already-pending: a resume delivered in the continue_as_new gap is grafted on even when the carried record says one resume is already pending.
_resume_pending is now carried (MergeWatchResume.resume_pending) and restored at :1799, and the comment above that line says the loop below "re-derives the pending window for the gap executions it actually ACCEPTS". But the loop never reads it: gap_is_foreign is checked, _seen_execution_ids is checked, and then the execution is appended unconditionally. The guard resume() applies at :1338 —
if self._resume_pending:
self._reject_resume(execution_id, work_item_id, "resume-already-pending")
return— has no counterpart here, for exactly the reason the mismatch guard had none: in the gap self._resume_pending is __init__'s False, so the signal is accepted there.
Failure scenario, on a merge watch that hops routinely over its 14-day deadline:
- run N:
resume({"work_item_id": "wi-1", "execution_id": "e2", "surface": "telegram", "actor_kind": "human", "actor_id": "bob"})is accepted and transitions, so_resume_pending = True - nothing calls
approve— the merge watch has no approval gate, andapproveis the only closer of this window (:1221), which is precisely why the field had to be carried - run N hops;
_carry_work_contextcarriesresume_pending=True resume({"work_item_id": "wi-1", "execution_id": "e3", "surface": "web", "actor_kind": "human", "actor_id": "mallory"})lands in the gap and is accepted against the empty state_rehydrate_work_contextrestoresresume_pending=Trueand then appendse3anyway
e3 becomes execution N+1, becomes _current_surface/_current_actor, and is carried into every later hop and every work_context answer — while the identical signal delivered a second earlier or later would have been recorded as ("e3", "resume-already-pending"). The documented "a DIFFERENT execution_id arriving while one accepted resume is still awaiting fresh approval is rejected" invariant therefore holds everywhere except at each hop boundary, which is the one place a long watch reliably provides.
The fix is the shape the mismatch check just took — check before the append, against the restored value:
for execution in gap_executions:
if gap_is_foreign:
self._reject_resume(execution.execution_id, gap_work_item_id, "work-item-mismatch")
continue
if execution.execution_id in self._seen_execution_ids:
continue
if self._resume_pending:
self._reject_resume(execution.execution_id, self._work_item_id, "resume-already-pending")
continue
...TestWorkContextGapMerge covers the foreign case and the accepted case, and test_carry_work_context_round_trips_through_rehydration carries resume_pending=True with no gap executions — so nothing exercises the two together.
Related, one line up the call chain: for a legitimate transitioning gap resume, _resume_merge_watch has already run self._approved = True / self._approver = resume.approver (:1862-1863) before rehydration, silently undoing the approval clearing that resume performed. That is harmless today only because a continued run reaches no approval gate — worth a sentence in the comment saying so, since the field is otherwise treated as load-bearing.
There was a problem hiding this comment.
Fixed in 63c5747 — the gap loop now rejects resume-already-pending against the CARRIED resume_pending before appending, exactly the shape the mismatch check took (foreign → mismatch → seen → pending → accept). Pinned by test_gap_resume_is_rejected_while_a_carried_resume_is_pending, which carries resume_pending=True WITH a gap execution — the combination you noted nothing exercised. Also added the sentence on why _resume_merge_watch's _approved = True upstream is deliberately harmless (a continued run reaches no approval gate) to _rehydrate_work_context's docstring.
| # WorkItem) is the wrong reader for it — and nothing in this repo | ||
| # consumes the route. The executions this repo needs arrive embedded in | ||
| # `get`'s WorkItem. Add a typed listing parser with the first real | ||
| # consumer instead of guessing mctl-api#227's response shape here. |
There was a problem hiding this comment.
P3 — record_execution still reads its response with answer_from, which is the exact mismatch this commit cites for deleting list_executions four lines up.
The new comment says GET .../executions "returns an execution LISTING, not a WorkItem envelope, so answer_from (which parses a WorkItem) is the wrong reader for it". POST /api/v1/work-items/{id}/executions is the same route and, by the same reasoning, returns the created execution record — not a WorkItem. So a perfectly successful write answers:
WorkItemAnswer(verdict=WORK_ITEM_UNKNOWN, reason="no work item record in a 201 response", accepted=True)A caller reading verdict (the field every other method's contract is expressed in) sees the write as failed; only accepted tells the truth, and nothing documents that this one method's answer must be read through a different field.
test_record_execution_posts_and_reads_the_response hides this by returning WORK_ITEM_PAYLOAD — a full WorkItem — from the POST handler and asserting WORK_ITEM_FOUND, i.e. it pins a response shape the commit message argues the route does not produce.
Since nothing in the repo consumes record_execution either, the consistent move is the one just made for the read: drop it (or have it return a bare "accepted / not accepted" result) and add a typed parser with the first real consumer, rather than leaving a writer whose success verdict is UNKNOWN.
There was a problem hiding this comment.
Fixed in 63c5747 — record_execution removed for exactly the reason you gave (a successful POST would answer verdict UNKNOWN through answer_from, readable only via accepted), along with its route entry and the test that pinned the WorkItem-shaped response the route does not produce. Comment in client.py says both re-arrive with a typed parser and the first real consumer.
| origin_surface=item.origin.kind, | ||
| current_surface=surface or "", | ||
| actor_kind=actor_kind or "", | ||
| actor_id=actor_id or "", |
There was a problem hiding this comment.
P3 — actor_id is passed straight through to a field validate() rejects above 256 characters, so an over-long --actor-id hard-fails the seal instead of being refused (or clamped) where it enters.
ContextSnapshot.validate enforces MAX_WORK_CONTEXT_ID_LENGTH on work_item_id, work_item_revision, execution_id, actor_id and resumed_from_snapshot_id. Of those, work_item_revision comes from the store and actor_id is operator/surface-supplied free text — neither is bounded on the way in. seal() calls validate(), so a 300-character --actor-id (or a store revision string over the ceiling) raises ContextSnapshotError out of seal → assemble → _assemble_context, which re-raises at ISSUE_INVESTIGATOR_CONTEXT_MODE=on and turns a valid investigation into a crash — and a silent warn:-and-skip at shadow.
This is the same "strict schema traps the producer" shape as the MAX_PRIOR_EXECUTION_IDS finding last round, which was fixed by clamping here. The cheap symmetric fix is either a length check next to the --surface/--actor-kind vocabulary checks in _work_context_from_args (so the CLI says why), or a truncation here with the same comment the prior-ids clamp carries. Nothing exercises the boundary from the producer side today.
There was a problem hiding this comment.
Fixed in 63c5747 — the id-shaped flags (--work-item-id, --execution-id, --resume-from-execution-id, --actor-id) are length-checked in _work_context_from_args next to the vocabulary checks (test_id_shaped_flags_are_length_bounded_at_the_cli, incl. the at-ceiling accept), and the store-supplied work_item_revision is clamped in _work_context_ref with the same rationale as the prior-ids clamp (test_store_supplied_revision_is_clamped_not_a_seal_trap).
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.
The P2 from the b82be1d round is fixed and verified: _rehydrate_work_context captures the gap binding before restoring the carried one and rejects a foreign gap resume with work-item-mismatch instead of grafting it, covered by TestWorkContextGapMerge. The three P3s are addressed too — the surface_transition baseline now excludes this execution and skips kindless ones, _canonical_issue_key compares owner/repo#number case-folded rather than raw URLs, and _resume_pending is carried in MergeWatchResume. The agy round is answered as well: kindless surface/actor blocks parse as absent instead of aborting the WorkItem, an out-of-vocabulary origin.kind classifies UNKNOWN with a named reason, and list_executions is gone rather than left reading a listing with a WorkItem parser.
New P2:
dev_loop.py:1809— the gap window this commit closed forwork-item-mismatchis still open forresume-already-pending._resume_pendingis carried and restored at:1799, but the re-apply loop never reads it, and in the gap it is__init__'sFalse, so the signal is accepted there. Since onlyapprovecloses the window and a merge watch never approves, a pending resume plus one gap-delivered resume with a newexecution_idrecords an execution that the same signal delivered outside the gap would have rejected — and it then becomes the surface/actor baseline carried into every later hop.test_carry_work_context_round_trips_through_rehydrationcarriesresume_pending=Truewith no gap executions, so nothing exercises the two together.
P3s: record_execution still reads a POST to .../executions with answer_from, the exact mismatch cited for deleting list_executions, so a successful write answers WORK_ITEM_UNKNOWN with accepted=True (and its test pins a WorkItem response the route does not return); and actor_id/work_item_revision reach a field validate() rejects above MAX_WORK_CONTEXT_ID_LENGTH without any producer-side bound, the same trap the prior-ids clamp removed.
The rest holds: the conditional work_context in _content_payload and the restored fixture hash, the bounded-length ceilings and the prior-ids clamp, to_log_dict omitting actor_id, the parent/child equality rule, the tolerant-mirror vs strict-schema split with duplicated vocabularies, the rollout ladder's default-off behaviour, _await_reapproval, _carry_work_context as a single fold point, and the workflow.patched("work-context-resume") placement.
I could not run the test suite in this environment (Bash approval for uv run pytest was declined), so all findings are from static reading of the pinned diff.
…dow, bound the id-shaped inputs Round-5 review pass (claude 1×P2 + 2×P3 on 1459dc4, agy PASS): - the gap merge re-checks `resume-already-pending` against the CARRIED `resume_pending` before appending a gap execution — the second guard the empty gap state made vacuous, closed the same way as work-item-mismatch last round; plus a docstring sentence on why the upstream `_approved = True` is deliberately harmless on a continued run. - removed `WorkItemClient.record_execution` for the same reason `list_executions` went: the POST's response is the created execution record, not a WorkItem envelope, so `answer_from` would answer a successful write with verdict UNKNOWN — and nothing consumes the route yet. - the id-shaped CLI flags (--work-item-id/--execution-id/ --resume-from-execution-id/--actor-id) are length-checked in `_work_context_from_args` with a reason, and the store-supplied `work_item_revision` is clamped in `_work_context_ref`, so an over-long value cannot fail validate() inside seal() at context mode `on`. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
|
Claude finished @mashkovd's task in 4m 45s —— View job Review complete — approvedReviewed
Previous round: the P2 and both P3s fixed
The docstring addition at P3 findings
What looks goodThe conditional I could not run the test suite or ruff in this environment (Bash approval for |
Antigravity (agy) reviewModel: gemini-3.8-flash-medium · commit Findings1. Missing test coverage for
|
| # still seals instead of failing validate() in seal(). | ||
| execution_sequence = len(prior_ids) + 1 | ||
| if len(prior_ids) > MAX_PRIOR_EXECUTION_IDS: | ||
| prior_ids = prior_ids[-MAX_PRIOR_EXECUTION_IDS:] |
There was a problem hiding this comment.
P3 — this commit bounds the id-shaped inputs, but the one store-supplied id-shaped value that reaches a validate()-bounded field is still unbounded: the entries of prior_execution_ids.
validate() checks both dimensions (context_snapshot.py:955-964):
if len(wc.prior_execution_ids) > MAX_PRIOR_EXECUTION_IDS: # count — clamped below
raise ...
for prior in wc.prior_execution_ids:
if len(prior) > MAX_WORK_CONTEXT_ID_LENGTH: # per-entry — clamped nowhere
raise ...The producer clamps the count here, and as of this commit truncates item.revision — but prior_ids comes from canonical.prior_execution_ids, which reconstruct_canonical_state fills from item.executions[].execution_id, i.e. straight off an mctl-api response that this package's own docstring insists it only mirrors, tolerantly. One recorded execution id over 256 characters raises ContextSnapshotError out of seal → assemble → _assemble_context, which re-raises at ISSUE_INVESTIGATOR_CONTEXT_MODE=on (turning a valid store answer into a failed investigation) and warns-and-skips at shadow — the same "strict schema traps the producer" shape the count clamp and the work_item_revision truncation on the next lines were both added to remove.
work_item_id=canonical.work_item_id sits in the same position: _work_context_from_args bounds the --work-item-id flag, but the value sealed here is the one the store echoed back, not the one the operator typed.
A per-entry truncation next to the existing clamp finishes the rule this commit started; nothing exercises that boundary from the producer side today, so a case with an over-long recorded execution id would pin it.
| silently-followed redirect to somewhere else.""" | ||
| answer = _client(monkeypatch, _http_error(302, {})).get("wi-1") | ||
| assert answer.verdict == WORK_ITEM_UNKNOWN | ||
|
|
There was a problem hiding this comment.
P3 — removing test_record_execution_posts_and_reads_the_response left the file ending in two blank lines. Cosmetic; I could not run ruff in this environment to confirm whether this image flags it (W is selected in pyproject.toml, and W391 is the rule for trailing blank lines), but the stray lines are worth dropping either way — the rest of the deletion is clean, and the now-unused imports were correctly pruned with it.
There was a problem hiding this comment.
No P1/P2 findings (2 P3). Good to merge.
The P2 from the 1459dc4 round is fixed and verified: _rehydrate_work_context now checks self._resume_pending before re-applying a gap execution, so a gap-delivered resume is rejected against the restored window exactly as resume() would have rejected it outside the gap — pinned by test_gap_resume_is_rejected_while_a_carried_resume_is_pending, which drives a carried resume_pending=True together with a gap resume for the first time. Both P3s are addressed too: record_execution is gone rather than left answering a successful write with verdict UNKNOWN (and its misleading WorkItem-shaped test went with it), and the id-shaped CLI flags are refused at _work_context_from_args with work_item_revision clamped at the producer.
Remaining P3s (non-blocking):
prior_execution_idsentries andwork_item_idare store-supplied and still unbounded at the producer, whilevalidate()rejects both aboveMAX_WORK_CONTEXT_ID_LENGTH— the last instance of the trap the count clamp and the revision truncation removed (run_issue_investigator.py:1841).tests/test_work_context_client.pyends in two stray blank lines after the deletion.
The rest holds the ADR 009/011 line: the conditional work_context in _content_payload with the restored fixture hash, the tolerant-mirror vs strict-schema split (duplicated vocabularies, no import edge), to_log_dict omitting actor_id, the parent/child equality rule, the rollout ladder defaulting to off, _await_reapproval as one releasable bounded park for both gates, _carry_work_context/_rehydrate_work_context as a single fold/unfold pair with seen_execution_ids carried sorted, and workflow.patched("work-context-resume") placed where neither gate schedules a command.
I could not run the test suite or ruff in this environment (Bash approval for uv run was declined), so all findings are from static reading of the pinned diff.
…, register the fake in #408 fixtures agy P2 findings on c9b2d4b, both confirmed against the code: - The WAITING_FOR_INPUT park never observed `_abandoned`: the operator escape hatch (mctl-agents#420) slept through its own signal for up to the request TTL, and a racing answer would have resumed the pipeline anyway. The wait predicate now releases on abandon, the abandon is checked before any delivered answer is consumed (abandon wins the race), and the loop returns a new "abandoned" HumanInputOutcome that run() turns into the standard `ended="abandoned: <reason>"` result. Regression test: abandon during WAITING_FOR_INPUT unblocks immediately and records the abandonment (agy's P3). - The clarification-timeout and failed-continuation-investigate returns left `DevLoopResult.ended` empty, reading as a successful full pipeline downstream; both now record why the loop ended. Also: the two #408 TestWorkContextResume tests whose slug fakes return a real slug now register the no-request find_human_input_request fake — the same fixture gap already fixed for the admission tests, and the whole cause of their WorkflowFailureError on the merged head. No product interaction between #400 and #408 was broken. Claude-Session: https://claude.ai/code/session_01GzwRwpmW1hisJTCTWS3ntK
Implements accepted proposal
mctl-agents/issue-267-feat-work-context-resume-investigator-fr.Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-agents/proposals/issue-267-feat-work-context-resume-investigator-fr/
Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.
Closes #267