From a0f56ff66c9b8b4083852008849e0252a9d59f1b Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:25:35 -0700 Subject: [PATCH 1/7] F11: HOLDING link targets live document (?op=N), not dissolved ops tab holding_readout.exec_href built /executions/{id}?tab=operations&op=N; the operations tab is dissolved and the document route ignores tab. Emit /executions/{id}?op={order} and have execdoc.js read ?op= on load, calling the existing jumpToStep (which expands the collapsed OP card). Co-Authored-By: Claude Fable 5 --- src/opal/core/holds.py | 4 ++-- src/opal/web/static/js/execdoc.js | 8 ++++++++ tests/unit/test_execution_holds.py | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/opal/core/holds.py b/src/opal/core/holds.py index 51ed863..19a55d9 100644 --- a/src/opal/core/holds.py +++ b/src/opal/core/holds.py @@ -170,8 +170,8 @@ def holding_readout(db: Session, issue: Issue) -> list[dict]: wo_label = instance.work_order_number or f"WO #{instance.id}" def exec_href(op_order: int | None = None) -> str: - base = f"/executions/{instance.id}?tab=operations" - return f"{base}&op={op_order}" if op_order is not None else base + base = f"/executions/{instance.id}" + return f"{base}?op={op_order}" if op_order is not None else base anchor = None anchor_id = issue.containment_step_id or issue.raised_step_id diff --git a/src/opal/web/static/js/execdoc.js b/src/opal/web/static/js/execdoc.js index 69bf435..e4e5421 100644 --- a/src/opal/web/static/js/execdoc.js +++ b/src/opal/web/static/js/execdoc.js @@ -1221,6 +1221,14 @@ postFocus(initial); } } + + // A HOLDING link (holds.py exec_href) lands here with ?op=N — jump to + // that step, expanding its collapsed OP card. jumpToStep is idempotent. + const opParam = new URLSearchParams(window.location.search).get('op'); + if (opParam !== null && opParam !== '') { + const opOrder = parseInt(opParam, 10); + if (!Number.isNaN(opOrder)) jumpToStep(opOrder); + } } document.addEventListener('DOMContentLoaded', () => { diff --git a/tests/unit/test_execution_holds.py b/tests/unit/test_execution_holds.py index 51a1ff5..7d4e9b0 100644 --- a/tests/unit/test_execution_holds.py +++ b/tests/unit/test_execution_holds.py @@ -447,6 +447,25 @@ def test_strict_sequence_gates_sub_step_complete(client): assert complete2.json()["status"] == "completed" +# ============ 11a. Holding readout links to the live document (F11) ============ + + +def test_holding_readout_links_to_document_op_param(client): + """The HOLDING link targets the live document shape (/executions/{id}?op=N), + not the dissolved operations tab.""" + instance_id = _create_instance(client) + resp = _raise_nc(client, instance_id, 1, containment="step") + issue_id = resp.json()["id"] + + holding = client.get(f"/api/issues/{issue_id}/holding") + assert holding.status_code == 200, holding.text + targets = holding.json() + assert targets, "an undispositioned step-contained NC holds something" + for target in targets: + assert "tab=operations" not in target["href"] + assert target["href"].startswith(f"/executions/{instance_id}?op=") + + # ============ 11. Required-data enforcement ============ From 431e0bd3414054501aa028dcd5ea8108057dd0d9 Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:28:04 -0700 Subject: [PATCH 2/7] F2: WO close-out authoritative on the issue set, not per-row hold buckets check_instance_completion inferred close-out from hold_state.wo_blocked alone, and OP auto-complete consulted only blockers_for_complete. Two reachable states closed a WO COMPLETED over an open, undispositioned, containment-bearing NC: a 'resolve by' boundary bound to an OP/terminal row (start_blocked, never consulted) and an op/step hold set onto an already-terminal row nothing revisits. Fix: the final close-out gate now blocks on blocking_issues_for_instance (any non-advisory undispositioned open issue on the WO), and the OP auto-complete gate folds blockers_for_start alongside blockers_for_complete. Dispositioned/advisory issues stay excluded, so legitimate completion is never deadlocked. Co-Authored-By: Claude Fable 5 --- src/opal/core/execution_flow.py | 23 ++++-- tests/unit/test_execution_holds.py | 123 +++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/src/opal/core/execution_flow.py b/src/opal/core/execution_flow.py index c2b9118..239cab9 100644 --- a/src/opal/core/execution_flow.py +++ b/src/opal/core/execution_flow.py @@ -26,7 +26,7 @@ from sqlalchemy.dialects.sqlite import insert as sqlite_insert from sqlalchemy.orm import Session -from opal.core.holds import get_hold_state +from opal.core.holds import blocking_issues_for_instance, get_hold_state from opal.db.models.attachment import Attachment from opal.db.models.execution import ( InstanceStatus, @@ -490,6 +490,12 @@ def check_instance_completion(db: Session, instance: ProcedureInstance) -> None: all_steps = db.query(StepExecution).filter(StepExecution.instance_id == instance.id).all() hold_state = get_hold_state(db, instance.id) + def _row_held(se: StepExecution) -> bool: + # A bound "resolve by" hold (start_blocked) gates a row's COMPLETE just + # as a raised/op hold does — fold both so an OP never auto-completes, + # and the WO never closes, over an open hold anchored on a terminal row. + return bool(hold_state.blockers_for_complete(se) or hold_state.blockers_for_start(se)) + for step_exec in all_steps: if step_exec.level == 0: children = [s for s in all_steps if s.parent_step_order == step_exec.step_number] @@ -501,9 +507,7 @@ def check_instance_completion(db: Session, instance: ProcedureInstance) -> None: in (StepStatus.COMPLETED.value, StepStatus.SKIPPED.value) for c in children ) - scope_held = bool(hold_state.blockers_for_complete(step_exec)) or any( - hold_state.blockers_for_complete(c) for c in children - ) + scope_held = _row_held(step_exec) or any(_row_held(c) for c in children) if all_children_done and not scope_held: step_exec.status = StepStatus.COMPLETED step_exec.completed_at = datetime.now(UTC) @@ -534,9 +538,14 @@ def _to_aware(dt: datetime) -> datetime: ): return - # WO containment holds close-out: every step may be terminal while an - # undispositioned wo-scoped issue keeps the work order open. - if hold_state.wo_blocked: + # Close-out is authoritative on the issue set, not on per-row hold buckets: + # ANY non-advisory, containment-bearing, undispositioned issue open on this + # work order keeps it open — a wo-scoped hold, an op/step hold whose anchor + # already went terminal, or a "resolve by" boundary bound to a terminal or + # OP row. Inferring close-out from wo_blocked alone let those slip through + # (a WO closing COMPLETED over an open NC). Dispositioned/advisory issues + # are excluded here, so legitimate completion is never deadlocked. + if blocking_issues_for_instance(db, instance.id): return instance.status = InstanceStatus.COMPLETED diff --git a/tests/unit/test_execution_holds.py b/tests/unit/test_execution_holds.py index 7d4e9b0..1d47138 100644 --- a/tests/unit/test_execution_holds.py +++ b/tests/unit/test_execution_holds.py @@ -447,6 +447,129 @@ def test_strict_sequence_gates_sub_step_complete(client): assert complete2.json()["status"] == "completed" +def _sub_step_instance(client): + """OP 1 (sub-steps 1.1, 1.2) + flat step 2; orders OP=1, 1.1=2, 1.2=3, + step2=4. Returns instance_id.""" + proc_id = client.post("/api/procedures", json={"name": "Sub-step WO"}).json()["id"] + op = client.post(f"/api/procedures/{proc_id}/steps", json={"title": "OP 1"}).json() + client.post( + f"/api/procedures/{proc_id}/steps", + json={"title": "Sub 1.1", "parent_step_id": op["id"]}, + ) + client.post( + f"/api/procedures/{proc_id}/steps", + json={"title": "Sub 1.2", "parent_step_id": op["id"]}, + ) + client.post(f"/api/procedures/{proc_id}/steps", json={"title": "Step 2"}) + client.post(f"/api/procedures/{proc_id}/publish") + resp = client.post("/api/procedure-instances", json={"procedure_id": proc_id}) + assert resp.status_code == 201, resp.text + return resp.json()["id"] + + +def _complete(client, instance_id, order, **body): + return client.post( + f"/api/procedure-instances/{instance_id}/steps/{order}/complete", json=body + ) + + +def _inst_status(client, instance_id): + return client.get(f"/api/procedure-instances/{instance_id}").json()["status"] + + +# ============ F2: close-out authoritative on the issue set ============ + + +def test_wo_stays_open_under_boundary_bound_to_terminal_step(client): + """F2(a): a 'resolve by' boundary bound to an already-terminal step lands + in start_blocked, which the old close-out gate never consulted — the WO + closed COMPLETED over the open NC. Now the issue set is authoritative.""" + instance_id = _create_instance(client) # flat 3 steps + + assert _complete(client, instance_id, 1).status_code == 200 + assert _complete(client, instance_id, 2).status_code == 200 + + # Raise an NC on step 3, boundary bound to the already-terminal step 1. + nc = _raise_nc( + client, instance_id, 3, containment="step", containment_step_number=1 + ) + assert nc.status_code == 201, nc.text + issue_id = nc.json()["id"] + + # Step 3 isn't the boundary, so it completes; every step is now terminal. + assert _complete(client, instance_id, 3).status_code == 200 + + # The open NC must keep the work order out of COMPLETED. + assert _inst_status(client, instance_id) != "completed" + + # Signing the disposition releases it and the WO closes. + _disposition(client, issue_id) + assert _inst_status(client, instance_id) == "completed" + + +def test_wo_stays_open_under_op_hold_on_terminal_op(client): + """F2(b): an op-containment NC set onto an OP that already auto-completed + lands only in op_complete_blocked on a terminal row nothing revisits. The + close-out gate must still see the open issue.""" + instance_id = _sub_step_instance(client) + + # Finish OP 1's sub-steps -> OP 1 auto-completes. + assert _complete(client, instance_id, 2).status_code == 200 + assert _complete(client, instance_id, 3).status_code == 200 + + # Now raise an advisory issue on a (terminal) sub-step and widen to op. + issue = client.post( + "/api/issues", + json={ + "title": "Late finding on OP 1", + "issue_type": "non_conformance", + "containment": "advisory", + "procedure_instance_id": instance_id, + }, + ).json() + inst = client.get(f"/api/procedure-instances/{instance_id}").json() + se_by_num = {s["step_number"]: s["id"] for s in inst["step_executions"]} + widen = client.post( + f"/api/issues/{issue['id']}/containment", + json={"containment": "op", "containment_step_id": se_by_num[2]}, + ) + assert widen.status_code == 200, widen.text + + # Finish the last flat step; the op-NC must keep the WO open. + assert _complete(client, instance_id, 4).status_code == 200 + assert _inst_status(client, instance_id) != "completed" + + _disposition(client, issue["id"]) + assert _inst_status(client, instance_id) == "completed" + + +def test_op_does_not_auto_complete_over_bound_hold_on_op_row(client): + """F2: a boundary bound to the OP row (start_blocked on the OP) must block + the OP's auto-complete — folding blockers_for_start into the gate.""" + instance_id = _sub_step_instance(client) + + # NC raised on sub 1.1 (order 2), boundary bound to the OP row (order 1). + nc = _raise_nc( + client, instance_id, 2, containment="step", containment_step_number=1 + ) + assert nc.status_code == 201, nc.text + issue_id = nc.json()["id"] + + # Sub-steps complete (neither is the boundary) but OP 1 must stay open. + assert _complete(client, instance_id, 2).status_code == 200 + assert _complete(client, instance_id, 3).status_code == 200 + + inst = client.get(f"/api/procedure-instances/{instance_id}").json() + op_status = next(s["status"] for s in inst["step_executions"] if s["step_number"] == 1) + assert op_status not in ("completed", "signed_off", "skipped") + assert _inst_status(client, instance_id) != "completed" + + # Disposition releases the OP; finishing the last step closes the WO. + _disposition(client, issue_id) + assert _complete(client, instance_id, 4).status_code == 200 + assert _inst_status(client, instance_id) == "completed" + + # ============ 11a. Holding readout links to the live document (F11) ============ From 0bf572181df548146a9d6c352c35cb3c00240f59 Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:31:25 -0700 Subject: [PATCH 3/7] F3: reconcile a containment issue's work order against its step anchors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core/holds filters holds on Issue.procedure_instance_id, but the create and bind seams never validated it. A non-advisory issue could carry a procedure_instance_id that named a different WO than its anchor step (blocks the wrong WO), or an MCP bind could point an issue at a foreign execution's step (reports success, blocks nothing). - issues.create / MCP _raise_issue: for non-advisory containment, derive procedure_instance_id from the step anchor when unset and reject an anchor (or a second anchor) that names a different WO. A bare draft (no anchor, no WO) still allowed — the WO is linked later. - MCP _bind_issue_hold: adopt the target WO when the issue names none, reject when it names a different one. Co-Authored-By: Claude Fable 5 --- src/opal/api/routes/issues.py | 45 ++++++++++++++++++++- src/opal/mcp/server.py | 40 ++++++++++++++++++- tests/unit/test_execution_holds.py | 49 +++++++++++++++++++++++ tests/unit/test_mcp_execution.py | 63 ++++++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 2 deletions(-) diff --git a/src/opal/api/routes/issues.py b/src/opal/api/routes/issues.py index adbe8b7..e4dbd28 100644 --- a/src/opal/api/routes/issues.py +++ b/src/opal/api/routes/issues.py @@ -16,6 +16,7 @@ from opal.core.designators import generate_issue_number from opal.core.events import emit_issue_dispositioned from opal.core.holds import holding_readout +from opal.db.models.execution import StepExecution from opal.db.models.issue import ( CONTAINMENT_RANK, Containment, @@ -36,6 +37,39 @@ def _get_enum_val(obj: object, attr: str) -> str: return val.value if hasattr(val, "value") else val +def _resolve_containment_instance( + db, + containment: Containment, + containment_step_id: int | None, + raised_step_id: int | None, + procedure_instance_id: int | None, +) -> int | None: + """Reconcile a non-advisory issue's work order against its step anchors. + + Derives procedure_instance_id from a step anchor when unset, and rejects a + procedure_instance_id (or a second anchor) that names a different work order + (F3). Advisory issues pass through untouched — they hold nothing. + """ + if containment == Containment.ADVISORY: + return procedure_instance_id + resolved = procedure_instance_id + for step_id in (containment_step_id, raised_step_id): + if step_id is None: + continue + step = db.query(StepExecution).filter(StepExecution.id == step_id).first() + if step is None: + raise HTTPException(status_code=400, detail=f"Step {step_id} not found") + if resolved is None: + resolved = step.instance_id + elif step.instance_id != resolved: + raise HTTPException( + status_code=400, + detail=f"Step {step_id} belongs to a different work order " + "than procedure_instance_id", + ) + return resolved + + # ============ Schemas ============ @@ -342,6 +376,15 @@ def create_issue( status_code=400, detail=f"Invalid containment: {data.containment}" ) from err + # A non-advisory containment holds a specific work order via the anchor's + # procedure_instance_id (core/holds filters on it). Derive it from any step + # anchor and reject anchors that name a different work order — otherwise the + # hold reports success but blocks nothing, or blocks the wrong WO (F3). A + # bare draft (no anchor, no WO) is still allowed; the WO is linked later. + procedure_instance_id = _resolve_containment_instance( + db, containment, data.containment_step_id, data.raised_step_id, data.procedure_instance_id + ) + issue = Issue( issue_number=generate_issue_number(db), title=data.title, @@ -359,7 +402,7 @@ def create_issue( expected_benefit=data.expected_benefit, part_id=data.part_id, procedure_id=data.procedure_id, - procedure_instance_id=data.procedure_instance_id, + procedure_instance_id=procedure_instance_id, raised_step_id=data.raised_step_id, raised_by_id=user_id, assigned_to_id=data.assigned_to_id, diff --git a/src/opal/mcp/server.py b/src/opal/mcp/server.py index 605a967..9401201 100644 --- a/src/opal/mcp/server.py +++ b/src/opal/mcp/server.py @@ -2908,6 +2908,29 @@ async def _raise_issue(db, args: dict) -> list[TextContent]: priority = args.get("priority", "medium") containment = args.get("containment", "advisory") + # A non-advisory containment holds a specific work order (core/holds filters + # on procedure_instance_id). Derive it from any step anchor, reject anchors + # naming a different WO — otherwise the hold blocks nothing or the wrong WO + # (F3). A bare draft (no anchor, no WO) is still allowed. + procedure_instance_id = args.get("procedure_instance_id") + if containment != Containment.ADVISORY.value: + for step_id in (args.get("containment_step_id"), args.get("raised_step_id")): + if step_id is None: + continue + step = db.query(StepExecution).filter(StepExecution.id == step_id).first() + if step is None: + return json_response({"success": False, "error": f"Step {step_id} not found"}) + if procedure_instance_id is None: + procedure_instance_id = step.instance_id + elif step.instance_id != procedure_instance_id: + return json_response( + { + "success": False, + "error": f"Step {step_id} belongs to a different work order " + "than procedure_instance_id", + } + ) + issue = Issue( issue_number=generate_issue_number(db), title=args["title"], @@ -2919,7 +2942,7 @@ async def _raise_issue(db, args: dict) -> list[TextContent]: containment_step_id=args.get("containment_step_id"), should_be=args.get("should_be"), actual=args.get("actual"), - procedure_instance_id=args.get("procedure_instance_id"), + procedure_instance_id=procedure_instance_id, raised_step_id=args.get("raised_step_id"), raised_by_id=args.get("user_id"), part_id=args.get("part_id"), @@ -6280,6 +6303,21 @@ async def _bind_issue_hold(db, args: dict) -> list[TextContent]: if not issue: return json_response({"error": f"Issue {args['issue_id']} not found"}) + # A hold that names one work order cannot be bound into another's step — the + # containment filter is per-instance, so a cross-WO bind would report + # success and block nothing (F3). Adopt the target WO when the issue names + # none yet; reject when it names a different one. + if issue.procedure_instance_id is None: + issue.procedure_instance_id = instance.id + db.flush() + elif issue.procedure_instance_id != instance.id: + return json_response( + { + "error": f"{issue.issue_number} belongs to work order " + f"{issue.procedure_instance_id}, not this execution ({instance.id})" + } + ) + containment = issue.containment.value if hasattr(issue.containment, "value") else issue.containment if containment == Containment.STEP.value and issue.containment_step_id == step_exec.id: label = step_exec.step_number_str or str(step_exec.step_number) diff --git a/tests/unit/test_execution_holds.py b/tests/unit/test_execution_holds.py index 1d47138..b3a1f8d 100644 --- a/tests/unit/test_execution_holds.py +++ b/tests/unit/test_execution_holds.py @@ -570,6 +570,55 @@ def test_op_does_not_auto_complete_over_bound_hold_on_op_row(client): assert _inst_status(client, instance_id) == "completed" +# ============ F3: containment issue reconciled against its work order ============ + + +def test_create_issue_rejects_anchor_in_different_wo(client): + """F3: a step-containment issue whose raised step belongs to WO A cannot + claim procedure_instance_id = WO B.""" + instance_a = _create_instance(client) + instance_b = _create_instance(client) + inst_a = client.get(f"/api/procedure-instances/{instance_a}").json() + step_a = inst_a["step_executions"][0]["id"] + + resp = client.post( + "/api/issues", + json={ + "title": "Mismatch", + "issue_type": "non_conformance", + "containment": "step", + "raised_step_id": step_a, + "procedure_instance_id": instance_b, + }, + ) + assert resp.status_code == 400 + assert "different work order" in resp.json()["detail"] + + +def test_create_issue_derives_wo_from_anchor(client): + """F3: omitting procedure_instance_id derives it from the anchor step so the + hold binds to the right WO (rather than blocking nothing).""" + instance_id = _create_instance(client) + inst = client.get(f"/api/procedure-instances/{instance_id}").json() + step = inst["step_executions"][0] + + resp = client.post( + "/api/issues", + json={ + "title": "Derive", + "issue_type": "non_conformance", + "containment": "step", + "raised_step_id": step["id"], + }, + ) + assert resp.status_code == 201, resp.text + assert resp.json()["procedure_instance_id"] == instance_id + + # The derived link means the hold actually holds the anchor step's COMPLETE. + blocked = _complete(client, instance_id, step["step_number"]) + assert blocked.status_code == 400 + + # ============ 11a. Holding readout links to the live document (F11) ============ diff --git a/tests/unit/test_mcp_execution.py b/tests/unit/test_mcp_execution.py index 3359d29..b9f5bfd 100644 --- a/tests/unit/test_mcp_execution.py +++ b/tests/unit/test_mcp_execution.py @@ -461,6 +461,69 @@ def test_bind_issue_hold_lifted_by_disposition(client, db_session, test_user, ad assert f"completed by {test_user.name}" in data["message"] +def test_bind_issue_hold_rejects_foreign_work_order(client, db_session, test_user): + """F3: an issue that names work order A cannot be bound into B's step — + the cross-WO bind would report success and block nothing.""" + instance_a, _ = _create_instance(client) + instance_b, _ = _create_instance(client) + + issue_data = _call( + server._raise_issue, + db_session, + { + "title": "Cross-WO NC", + "issue_type": "non_conformance", + "containment": "step", + "procedure_instance_id": instance_a, + }, + ) + issue_id = issue_data["issue"]["id"] + + data = _call( + server._bind_issue_hold, + db_session, + { + "execution_id": instance_b, + "step_number": 1, + "issue_id": issue_id, + "user_id": test_user.id, + }, + ) + assert "error" in data + assert str(instance_a) in data["error"] + + +def test_bind_issue_hold_adopts_work_order_when_unset(client, db_session, test_user): + """F3: binding a WO-less draft adopts the target work order so the hold + actually blocks the bound step.""" + instance_id, _ = _create_instance(client) + + issue_data = _call( + server._raise_issue, + db_session, + {"title": "Draft NC", "issue_type": "non_conformance", "containment": "step"}, + ) + issue_id = issue_data["issue"]["id"] + assert db_session.get(Issue, issue_id).procedure_instance_id is None + + data = _call( + server._bind_issue_hold, + db_session, + { + "execution_id": instance_id, + "step_number": 2, + "issue_id": issue_id, + "user_id": test_user.id, + }, + ) + assert data["success"] is True + assert db_session.get(Issue, issue_id).procedure_instance_id == instance_id + + state = _call(server._get_execution_state, db_session, {"execution_id": instance_id}) + step2 = next(s for s in state["steps"] if s["order"] == 2) + assert len(step2["holds"]) == 1 + + def test_bind_issue_hold_unknown_issue(client, db_session): instance_id, _ = _create_instance(client) From 78422d4429957e7f8b40ed68825428dd9b84a163 Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:33:07 -0700 Subject: [PATCH 4/7] F8: containment boundary is clearable + WO-validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_containment (API + MCP) applied containment_step_id only when non-null, so a mis-bound 'resolve by' boundary was immutable, and it never checked the posted step belonged to the issue's work order. Decision (flagged for confirmation): use explicit-presence semantics via model_fields_set / key-in-args (a documented explicit-null clear), rather than the disposition_type empty-string sentinel — containment_step_id is an int field, and an explicit JSON null is the natural clear. An omitted field leaves the boundary untouched; an explicit null clears it. A non-null boundary must belong to the issue's WO (seeds the WO on an unlinked issue, rejected when it names a different one). Co-Authored-By: Claude Fable 5 --- src/opal/api/routes/issues.py | 21 +++++++++++-- src/opal/mcp/server.py | 24 ++++++++++++-- tests/unit/test_execution_holds.py | 50 ++++++++++++++++++++++++++++++ tests/unit/test_mcp_execution.py | 36 +++++++++++++++++++++ 4 files changed, 127 insertions(+), 4 deletions(-) diff --git a/src/opal/api/routes/issues.py b/src/opal/api/routes/issues.py index e4dbd28..5b934e0 100644 --- a/src/opal/api/routes/issues.py +++ b/src/opal/api/routes/issues.py @@ -657,8 +657,25 @@ def set_containment( old_values = get_model_dict(issue) issue.containment = new_containment - if data.containment_step_id is not None: - issue.containment_step_id = data.containment_step_id + # Boundary re-binding uses explicit presence, not None-skip: an omitted + # containment_step_id leaves the boundary untouched, while an explicit null + # clears it (a mis-bound boundary was previously immutable — F8). A non-null + # boundary must belong to this issue's work order; it seeds the WO on a + # still-unlinked issue and is rejected when it names a different one. + if "containment_step_id" in data.model_fields_set: + new_step_id = data.containment_step_id + if new_step_id is not None: + step = db.query(StepExecution).filter(StepExecution.id == new_step_id).first() + if step is None: + raise HTTPException(status_code=404, detail=f"Step {new_step_id} not found") + if issue.procedure_instance_id is None: + issue.procedure_instance_id = step.instance_id + elif step.instance_id != issue.procedure_instance_id: + raise HTTPException( + status_code=400, + detail=f"Step {new_step_id} belongs to a different work order than this issue", + ) + issue.containment_step_id = new_step_id log_update(db, issue, old_values, user_id) if narrowing and (data.note or "").strip(): diff --git a/src/opal/mcp/server.py b/src/opal/mcp/server.py index 9401201..f98920d 100644 --- a/src/opal/mcp/server.py +++ b/src/opal/mcp/server.py @@ -3046,8 +3046,28 @@ async def _set_containment(db, args: dict) -> list[TextContent]: user_id = args.get("user_id") old_values = get_model_dict(issue) issue.containment = new_containment - if args.get("containment_step_id") is not None: - issue.containment_step_id = args["containment_step_id"] + # Explicit presence, not None-skip: an omitted containment_step_id leaves + # the boundary untouched; an explicit null clears it (F8). A non-null + # boundary must belong to this issue's work order. + if "containment_step_id" in args: + new_step_id = args["containment_step_id"] + if new_step_id is not None: + step = db.query(StepExecution).filter(StepExecution.id == new_step_id).first() + if step is None: + return json_response( + {"success": False, "error": f"Step {new_step_id} not found"} + ) + if issue.procedure_instance_id is None: + issue.procedure_instance_id = step.instance_id + elif step.instance_id != issue.procedure_instance_id: + return json_response( + { + "success": False, + "error": f"Step {new_step_id} belongs to a different work order " + "than this issue", + } + ) + issue.containment_step_id = new_step_id log_update(db, issue, old_values, user_id) if narrowing and note: diff --git a/tests/unit/test_execution_holds.py b/tests/unit/test_execution_holds.py index b3a1f8d..bb1c393 100644 --- a/tests/unit/test_execution_holds.py +++ b/tests/unit/test_execution_holds.py @@ -570,6 +570,56 @@ def test_op_does_not_auto_complete_over_bound_hold_on_op_row(client): assert _inst_status(client, instance_id) == "completed" +# ============ F8: boundary clear semantics + WO membership ============ + + +def test_set_containment_clears_boundary_with_explicit_null(client): + """F8: an explicit null clears a mis-bound boundary (previously immutable, + since the endpoint applied containment_step_id only when non-null).""" + instance_id = _create_instance(client) + inst = client.get(f"/api/procedure-instances/{instance_id}").json() + se_by_num = {s["step_number"]: s["id"] for s in inst["step_executions"]} + + # Raise on step 1, bind the boundary to step 3. + resp = _raise_nc(client, instance_id, 1, containment="step") + issue_id = resp.json()["id"] + client.post( + f"/api/issues/{issue_id}/containment", + json={"containment": "step", "containment_step_id": se_by_num[3]}, + ) + assert _complete(client, instance_id, 3).status_code == 400 + + # Clear the boundary — the hold falls back to the raised step (step 1). + clear = client.post( + f"/api/issues/{issue_id}/containment", + json={"containment": "step", "containment_step_id": None}, + ) + assert clear.status_code == 200, clear.text + assert clear.json()["containment_step_id"] is None + + # Step 3 is free again; step 1 is now the held anchor. + assert _complete(client, instance_id, 3).status_code == 200 + assert _complete(client, instance_id, 1).status_code == 400 + + +def test_set_containment_rejects_foreign_step(client): + """F8: the posted boundary must belong to the issue's work order.""" + instance_a = _create_instance(client) + instance_b = _create_instance(client) + inst_b = client.get(f"/api/procedure-instances/{instance_b}").json() + foreign_step = inst_b["step_executions"][0]["id"] + + resp = _raise_nc(client, instance_a, 1, containment="step") + issue_id = resp.json()["id"] + + bad = client.post( + f"/api/issues/{issue_id}/containment", + json={"containment": "step", "containment_step_id": foreign_step}, + ) + assert bad.status_code == 400 + assert "different work order" in bad.json()["detail"] + + # ============ F3: containment issue reconciled against its work order ============ diff --git a/tests/unit/test_mcp_execution.py b/tests/unit/test_mcp_execution.py index b9f5bfd..01dfed8 100644 --- a/tests/unit/test_mcp_execution.py +++ b/tests/unit/test_mcp_execution.py @@ -461,6 +461,42 @@ def test_bind_issue_hold_lifted_by_disposition(client, db_session, test_user, ad assert f"completed by {test_user.name}" in data["message"] +def test_set_containment_rejects_foreign_step(client, db_session, test_user): + """F8 (MCP mirror): a boundary step from another WO is rejected.""" + instance_a, _ = _create_instance(client) + instance_b, _ = _create_instance(client) + foreign_step = ( + db_session.query(server.StepExecution) + .filter(server.StepExecution.instance_id == instance_b) + .first() + ) + + issue_data = _call( + server._raise_issue, + db_session, + { + "title": "NC on A", + "issue_type": "non_conformance", + "containment": "step", + "procedure_instance_id": instance_a, + }, + ) + issue_id = issue_data["issue"]["id"] + + data = _call( + server._set_containment, + db_session, + { + "issue_id": issue_id, + "containment": "step", + "containment_step_id": foreign_step.id, + "user_id": test_user.id, + }, + ) + assert data["success"] is False + assert "different work order" in data["error"] + + def test_bind_issue_hold_rejects_foreign_work_order(client, db_session, test_user): """F3: an issue that names work order A cannot be bound into B's step — the cross-WO bind would report success and block nothing.""" From e22801e4fde982e386a9728b8d9800107ce6a7b6 Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:35:37 -0700 Subject: [PATCH 5/7] F5: SKIP respects the open-redline-rework gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit skip_step checked only containment/scope holds, so after a disposition was signed COMPLETE still refused ('Waiting on redline op') while SKIP marked the host step terminal SKIPPED — stranding authorized rework, reversing the invariant the old code asserted by name. Decision (flagged for confirmation): the safe reading is that an open redline-rework op blocks SKIP too. Added core.skip_blockers = held scope + the redline-class blockers only, deliberately splitting out strict_sequence and OP-dependency ordering (skipping legitimately need not wait on predecessors). skip_step now gates on it. Co-Authored-By: Claude Fable 5 --- src/opal/api/routes/execution.py | 16 ++++----- src/opal/core/execution_flow.py | 21 +++++++++-- tests/unit/test_execution.py | 61 ++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/opal/api/routes/execution.py b/src/opal/api/routes/execution.py index d1ba23b..3f20141 100644 --- a/src/opal/api/routes/execution.py +++ b/src/opal/api/routes/execution.py @@ -30,8 +30,8 @@ clear_focus, complete_step_flow, focus_step, - held_scope_blockers, mark_instance_in_work, + skip_blockers, touch_user_presence, ) from opal.core.genealogy import record_assembly_genealogy @@ -820,14 +820,12 @@ async def skip_step( if step_status in (StepStatus.COMPLETED.value, StepStatus.SIGNED_OFF.value): raise HTTPException(status_code=400, detail="Cannot skip completed step") - # Containment gate: SKIP is a terminal commitment, same scope check as - # COMPLETE — skipping held work would sweep the hold. - skip_blockers = get_hold_state(db, instance_id).blockers_for_complete(step_exec) - if skip_blockers: - raise HTTPException(status_code=400, detail=_hold_blocker_detail("skip", skip_blockers)) - - # Skip is a commitment moment too — held work cannot be skipped around. - holds = held_scope_blockers(db, instance, step_exec) + # SKIP is a terminal commitment: it inherits the held scope (a hold cannot + # be skipped around) and any open redline-rework op on the gate — skipping + # the host step must not strand authorized rework (F5). It does not inherit + # strict_sequence/dependency ordering: skipping legitimately need not wait + # on predecessors. + holds = skip_blockers(db, instance, step_exec) if holds: raise HTTPException( status_code=400, diff --git a/src/opal/core/execution_flow.py b/src/opal/core/execution_flow.py index 239cab9..01a1906 100644 --- a/src/opal/core/execution_flow.py +++ b/src/opal/core/execution_flow.py @@ -244,13 +244,30 @@ def complete_blockers( step_exec: StepExecution, ) -> list[Blocker]: """Everything holding this step's (or OP's) COMPLETE: the held scope - plus the structural sequence gates. SKIP checks only the held scope — - sequencing gates COMPLETE alone.""" + plus the structural sequence gates.""" return held_scope_blockers(db, instance, step_exec) + sequence_blockers( db, instance, step_exec ) +def skip_blockers( + db: Session, + instance: ProcedureInstance, + step_exec: StepExecution, +) -> list[Blocker]: + """Everything holding this step's SKIP: the held scope plus any open + redline-rework op on the gate. + + SKIP is a terminal commitment, so it inherits the held scope (a hold + cannot be skipped around) and the redline gate — skipping the host step + must not strand an authorized rework op (F5). It deliberately omits + strict_sequence and OP-dependency ordering: skipping legitimately does + not require predecessors to be done.""" + scope = held_scope_blockers(db, instance, step_exec) + redlines = [b for b in sequence_blockers(db, instance, step_exec) if b.kind == "redline"] + return scope + redlines + + # ============ Presence (focus) ============ diff --git a/tests/unit/test_execution.py b/tests/unit/test_execution.py index 10fbfd0..6b5458e 100644 --- a/tests/unit/test_execution.py +++ b/tests/unit/test_execution.py @@ -926,6 +926,67 @@ def test_redline_requires_nc(client): assert bad.status_code == 400 +def test_skip_blocked_by_open_redline_after_disposition(client): + """F5: after the disposition is signed the NC hold releases, but an open + redline-rework op still gates the host step. SKIP must refuse just as + COMPLETE does — skipping the host step cannot strand authorized rework.""" + instance_id, issue_id, _ = _create_redline_setup(client) + op_resp = client.post( + f"/api/procedure-instances/{instance_id}/ad-hoc-ops", + json={"issue_id": issue_id, "title": "Rework", "steps": [{"title": "Do rework"}]}, + ).json() + sub_step_number = op_resp["sub_steps"][0]["step_number"] + + _sign(client, issue_id, "rework", "rework per redline") + + # COMPLETE refuses on the open redline... + resp = client.post(f"/api/procedure-instances/{instance_id}/steps/1/complete", json={}) + assert resp.status_code == 400 + assert "redline" in resp.json()["detail"].lower() + + # ...and so must SKIP (F5): the redline gate applies to both. + skip = client.post(f"/api/procedure-instances/{instance_id}/steps/1/skip", json={}) + assert skip.status_code == 400 + assert "redline" in skip.json()["detail"].lower() + + # Once the redline sub-step is executed, the gate clears and SKIP works. + client.post( + f"/api/procedure-instances/{instance_id}/steps/{sub_step_number}/complete", json={} + ) + skip = client.post(f"/api/procedure-instances/{instance_id}/steps/1/skip", json={}) + assert skip.status_code == 200, skip.text + assert skip.json()["status"] == "skipped" + + +def test_skip_not_blocked_by_strict_sequence_predecessor(client): + """F5 (scope boundary): SKIP deliberately omits strict_sequence predecessor + ordering — a later sub-step may be skipped even with its predecessors still + pending. Only the held scope and the redline class gate SKIP.""" + proc_id = client.post("/api/procedures", json={"name": "Strict skip"}).json()["id"] + op = client.post( + f"/api/procedures/{proc_id}/steps", + json={"title": "Strict OP", "strict_sequence": True}, + ).json() + client.post( + f"/api/procedures/{proc_id}/steps", json={"title": "Sub 1", "parent_step_id": op["id"]} + ) + client.post( + f"/api/procedures/{proc_id}/steps", json={"title": "Sub 2", "parent_step_id": op["id"]} + ) + client.post(f"/api/procedures/{proc_id}/publish") + instance_id = client.post( + "/api/procedure-instances", json={"procedure_id": proc_id} + ).json()["id"] + # Orders: OP=1, 1.1=2, 1.2=3. COMPLETE on 1.2 is sequence-gated... + blocked = client.post(f"/api/procedure-instances/{instance_id}/steps/3/complete", json={}) + assert blocked.status_code == 400 + assert "Waiting on" in blocked.json()["detail"] + # ...but SKIP on 1.2 is allowed with 1.1 still pending. + skip = client.post(f"/api/procedure-instances/{instance_id}/steps/3/skip", json={}) + assert skip.status_code == 200, skip.text + assert skip.json()["status"] == "skipped" + + # ============ Containment scope across the op hierarchy ============ From cd02a46ca15bb7e3754a9573f5f91050a70aa74b Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:38:31 -0700 Subject: [PATCH 6/7] F4: single server-computed gate drives COMPLETE/SKIP controls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _step_row.html and _dockbar.html derived 'held' from step_holding_ncs (raised/containment) only, ignoring bound 'resolve by' holds (start_blocked) and sequence/redline gates — both of which the server refuses. A held or sequence-gated row rendered an active COMPLETE the server then 400'd. Fix at altitude: the route computes one authoritative answer per step execution — complete_gate_by_se (core.complete_blockers: held scope + sequence) and skip_gate_by_se (core.skip_blockers: held scope + redline). A new ex.complete_control macro renders the button or the absent-control blocker line from that single field; both templates use it, and SKIP hides on skip_gate. Templates no longer re-derive gating. Removes the dead step_complete_blockers/step_start_blockers context keys. Co-Authored-By: Claude Fable 5 --- src/opal/web/routes.py | 44 ++++++++++++------- .../web/templates/executions/_dockbar.html | 8 +--- .../templates/executions/_exec_macros.html | 13 ++++++ .../web/templates/executions/_step_row.html | 11 ++--- tests/unit/test_issue_hold_rendering.py | 32 ++++++++++++++ 5 files changed, 79 insertions(+), 29 deletions(-) diff --git a/src/opal/web/routes.py b/src/opal/web/routes.py index 765b8da..6a12208 100644 --- a/src/opal/web/routes.py +++ b/src/opal/web/routes.py @@ -28,7 +28,7 @@ validate_password_strength, verify_payload, ) -from opal.core.holds import get_hold_state, holding_readout, scope_label +from opal.core.holds import holding_readout, scope_label from opal.db.models import ( InventoryRecord, Kit, @@ -2305,20 +2305,24 @@ def _se_status(se): linked_issues.sort(key=lambda i: (not i.is_blocking, disp_rank.get(i.disp_state, 4), -i.id)) context["linked_issues"] = linked_issues - # Containment-derived hold state: which controls are absent, and which - # issues replace them. Keyed by step_execution_id. - hold_state = get_hold_state(db, instance.id) - step_complete_blockers: dict[int, list[Issue]] = {} - step_start_blockers: dict[int, list[Issue]] = {} + # One authoritative answer per step execution for the COMPLETE and SKIP + # controls: the full fold the server enforces (held scope + sequence for + # COMPLETE; held scope + redline for SKIP), so a control never renders + # active where the server would 400 it (F4/F5). Templates render the + # controls from these — they do not re-derive gating. Keyed by + # step_execution_id; a present, non-empty list means the control is absent + # and replaced by a blocker line. + complete_gate_by_se: dict[int, list[exec_flow.Blocker]] = {} + skip_gate_by_se: dict[int, list[exec_flow.Blocker]] = {} for se in instance.step_executions: - complete_blockers = hold_state.blockers_for_complete(se) - if complete_blockers: - step_complete_blockers[se.id] = complete_blockers - start_blockers = hold_state.blockers_for_start(se) - if start_blockers: - step_start_blockers[se.id] = start_blockers - context["step_complete_blockers"] = step_complete_blockers - context["step_start_blockers"] = step_start_blockers + cg = exec_flow.complete_blockers(db, instance, se) + if cg: + complete_gate_by_se[se.id] = cg + sg = exec_flow.skip_blockers(db, instance, se) + if sg: + skip_gate_by_se[se.id] = sg + context["complete_gate_by_se"] = complete_gate_by_se + context["skip_gate_by_se"] = skip_gate_by_se # Per-op aggregate of open NCs (op-level + any of its sub-steps). Used to # decide when to show the "+ ADD REDLINE OP" button and to populate the @@ -2585,8 +2589,16 @@ def _set_bar_step(context: dict, step_order: int | None) -> None: op_data["step"]["order"], [] ), "step_kit": vs.get("step_kit") or [], - "raised_holds": ( - context.get("step_holding_ncs", {}).get(row["execution"].id, []) + # Single per-row gate answer (F4): the COMPLETE/SKIP controls render + # from these, never re-derived in the template. Non-empty => control + # absent, blocker line shown. + "complete_blockers": ( + context.get("complete_gate_by_se", {}).get(row["execution"].id, []) + if row.get("execution") is not None + else [] + ), + "skip_blockers": ( + context.get("skip_gate_by_se", {}).get(row["execution"].id, []) if row.get("execution") is not None else [] ), diff --git a/src/opal/web/templates/executions/_dockbar.html b/src/opal/web/templates/executions/_dockbar.html index 2661403..155479b 100644 --- a/src/opal/web/templates/executions/_dockbar.html +++ b/src/opal/web/templates/executions/_dockbar.html @@ -8,7 +8,7 @@ {% set actionable = inst_status in ['cut', 'in_work'] %} {% set bs = bar_step.status %} {% set workable = actionable and bs in ['pending', 'in_progress'] %} - {% set menu_skip = workable and not bar_step.raised_holds %} + {% set menu_skip = workable and not bar_step.skip_blockers %} {% set menu_consume = workable and bar_step.step_kit and not bar_step.has_children %} {% set menu_redline = bar_step.op_open_ncs and not bar_step.op_is_ad_hoc %}
@@ -61,11 +61,7 @@ {% if actionable and bs == 'awaiting_signoff' %} {% elif workable and not bar_step.has_children %} - {% if bar_step.raised_holds %} - COMPLETE — held by {% for nc in bar_step.raised_holds %}{{ nc.issue_number }}{% if not loop.last %}, {% endif %}{% endfor %} - {% else %} - - {% endif %} + {{ ex.complete_control(bar_step.order, bar_step.complete_blockers) }} {% endif %} {% endif %}
diff --git a/src/opal/web/templates/executions/_exec_macros.html b/src/opal/web/templates/executions/_exec_macros.html index 9bef204..6a88a93 100644 --- a/src/opal/web/templates/executions/_exec_macros.html +++ b/src/opal/web/templates/executions/_exec_macros.html @@ -31,6 +31,19 @@ {% endif %} {% endmacro %} +{# The COMPLETE control, from the single server-computed gate (F4): the button + when nothing holds the step, else the absent-control blocker line naming the + reason. `blockers` is a list of core.execution_flow.Blocker — issue-backed + ones link, structural ones (sequence/redline) render their message. Never + re-derive gating here. #} +{% macro complete_control(order, blockers) %} +{%- if blockers %} +COMPLETE — {% for b in blockers %}{% if b.issue_number %}held by {{ b.issue_number }}{% else %}{{ b.message }}{% endif %}{% if not loop.last %}; {% endif %}{% endfor %} +{%- else %} + +{%- endif %} +{% endmacro %} + {# Row indicators: counts as words, absent when zero (empty-state rule). #} {% macro indicators(step, attach_counts, vs) %} {% set exec = step.execution %} diff --git a/src/opal/web/templates/executions/_step_row.html b/src/opal/web/templates/executions/_step_row.html index d621c41..d1e289d 100644 --- a/src/opal/web/templates/executions/_step_row.html +++ b/src/opal/web/templates/executions/_step_row.html @@ -85,7 +85,8 @@ {% set body_schema = step.required_data_schema or vs.get('required_data_schema') %} {% set workable = inst_status in ['cut', 'in_work'] and step.status in ['pending', 'in_progress'] %} - {% set step_raised_holds = step_holding_ncs.get(exec.id, []) if exec else [] %} + {% set complete_gate = complete_gate_by_se.get(exec.id, []) if exec else [] %} + {% set skip_gate = skip_gate_by_se.get(exec.id, []) if exec else [] %} {% if workable and exec %} {# The expanded step is a control surface: fields, notes, actions. #}
@@ -100,17 +101,13 @@
- {% if not step_raised_holds %} + {% if not skip_gate %} {% endif %} {% if step_kit %} {% endif %} - {% if step_raised_holds %} - COMPLETE — held by {% for nc in step_raised_holds %}{{ nc.issue_number }}{% if not loop.last %}, {% endif %}{% endfor %} - {% else %} - - {% endif %} + {{ ex.complete_control(step.order, complete_gate) }}
{% elif inst_status in ['cut', 'in_work'] and step.status == 'awaiting_signoff' %} diff --git a/tests/unit/test_issue_hold_rendering.py b/tests/unit/test_issue_hold_rendering.py index ac8c8dd..2b253f2 100644 --- a/tests/unit/test_issue_hold_rendering.py +++ b/tests/unit/test_issue_hold_rendering.py @@ -53,6 +53,38 @@ def test_held_step_renders_blocker_line_not_complete_button(web_client): assert "alert(`NC logged" not in page.text +def test_bound_hold_row_renders_no_active_complete(web_client): + """F4: a 'resolve by' boundary (start_blocked) — which the server refuses to + COMPLETE — must not render an active COMPLETE button; the control is absent, + replaced by the blocker line. Previously the row derived 'held' from raised + NCs only and showed a button the server then 400'd.""" + proc_id = web_client.post("/api/procedures", json={"name": "Bound gate"}).json()["id"] + for title in ("S1", "S2", "S3"): + web_client.post(f"/api/procedures/{proc_id}/steps", json={"title": title}) + web_client.post(f"/api/procedures/{proc_id}/publish") + instance_id = web_client.post( + "/api/procedure-instances", json={"procedure_id": proc_id} + ).json()["id"] + inst = web_client.get(f"/api/procedure-instances/{instance_id}").json() + se_by_num = {s["step_number"]: s["id"] for s in inst["step_executions"]} + + # Raise on step 1, bind the boundary to step 3 (start_blocked on step 3). + nc = web_client.post( + f"/api/procedure-instances/{instance_id}/steps/1/nc", json={"title": "Bound NC"} + ).json() + web_client.post( + f"/api/issues/{nc['id']}/containment", + json={"containment": "step", "containment_step_id": se_by_num[3]}, + ) + + page = web_client.get(f"/executions/{instance_id}") + assert page.status_code == 200 + # Step 3's COMPLETE control is absent — the server would 400 the bound hold. + assert "completeStep(3, this)" not in page.text + # The blocker line names the issue instead. + assert nc["issue_number"] in page.text + + def test_signed_disposition_restores_controls(web_client): """(§10.3) Signing releases the containment; the controls return.""" instance_id, by_label = _build_instance_with_sub_steps(web_client) From e76dae5b329ff328a52f54e9f32eccae1fb8ac04 Mon Sep 17 00:00:00 2001 From: CST-100 <42722690+CST-100@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:41:11 -0700 Subject: [PATCH 7/7] cleanup: one home for enum-unwrap, terminal set, and assignable users Per 'one fact, one home', consolidate duplication adjacent to these fixes: - execution_flow._status_value was a copy of holds._val; alias the canonical core/holds helper instead. - routes.py inlined the terminal-status set twice; reuse execution_flow.TERMINAL_STEP_STATUSES. - drop the dead context['users'] query (execution templates read the identical active_users). The dead step_complete_blockers/step_start_blockers keys were already removed with F4. Co-Authored-By: Claude Fable 5 --- src/opal/core/execution_flow.py | 10 ++-------- src/opal/mcp/server.py | 4 +--- src/opal/web/routes.py | 34 +++++++++------------------------ 3 files changed, 12 insertions(+), 36 deletions(-) diff --git a/src/opal/core/execution_flow.py b/src/opal/core/execution_flow.py index 01a1906..27a46d8 100644 --- a/src/opal/core/execution_flow.py +++ b/src/opal/core/execution_flow.py @@ -26,6 +26,7 @@ from sqlalchemy.dialects.sqlite import insert as sqlite_insert from sqlalchemy.orm import Session +from opal.core.holds import _val as _status_value # enum-unwrap, one home in core/holds from opal.core.holds import blocking_issues_for_instance, get_hold_state from opal.db.models.attachment import Attachment from opal.db.models.execution import ( @@ -61,10 +62,6 @@ def __init__(self, message: str): self.message = message -def _status_value(obj: Any) -> str: - return obj.value if hasattr(obj, "value") else obj - - def user_initials(name: str | None) -> str: if not name: return "?" @@ -245,9 +242,7 @@ def complete_blockers( ) -> list[Blocker]: """Everything holding this step's (or OP's) COMPLETE: the held scope plus the structural sequence gates.""" - return held_scope_blockers(db, instance, step_exec) + sequence_blockers( - db, instance, step_exec - ) + return held_scope_blockers(db, instance, step_exec) + sequence_blockers(db, instance, step_exec) def skip_blockers( @@ -481,7 +476,6 @@ def complete_step_flow( old_instance_status = _status_value(instance.status) check_instance_completion(db, instance) - instance_completed = ( old_instance_status != InstanceStatus.COMPLETED.value and _status_value(instance.status) == InstanceStatus.COMPLETED.value diff --git a/src/opal/mcp/server.py b/src/opal/mcp/server.py index f98920d..ceccd5d 100644 --- a/src/opal/mcp/server.py +++ b/src/opal/mcp/server.py @@ -3054,9 +3054,7 @@ async def _set_containment(db, args: dict) -> list[TextContent]: if new_step_id is not None: step = db.query(StepExecution).filter(StepExecution.id == new_step_id).first() if step is None: - return json_response( - {"success": False, "error": f"Step {new_step_id} not found"} - ) + return json_response({"success": False, "error": f"Step {new_step_id} not found"}) if issue.procedure_instance_id is None: issue.procedure_instance_id = step.instance_id elif step.instance_id != issue.procedure_instance_id: diff --git a/src/opal/web/routes.py b/src/opal/web/routes.py index 6a12208..8c9baae 100644 --- a/src/opal/web/routes.py +++ b/src/opal/web/routes.py @@ -2028,9 +2028,7 @@ def _execution_detail_context( same facts.""" version = db.query(ProcedureVersion).filter(ProcedureVersion.id == instance.version_id).first() - context = get_base_context( - request, db, f"{instance.work_order_number or 'Execution'} - OPAL" - ) + context = get_base_context(request, db, f"{instance.work_order_number or 'Execution'} - OPAL") context["instance"] = instance context["version"] = version context["statuses"] = [s.value for s in InstanceStatus] @@ -2350,16 +2348,8 @@ def _se_status(se): open_ncs_by_op_order[op_step["order"]] = bucket context["op_open_ncs_by_order"] = open_ncs_by_op_order - # Capture pre-fill: assignable users for the anomaly modal. - from opal.db.models.user import User as _User - - context["users"] = ( - db.query(_User).filter(_User.is_active == True).order_by(_User.name).all() # noqa: E712 - ) - # Gating lookup: top-level ops whose prerequisite ops haven't reached # a terminal status yet. Keyed by op.order → list of blocking step_number_str. - terminal_step_statuses = {"completed", "signed_off", "skipped"} exec_by_order = {se.step_number: se for se in instance.step_executions} gated_ops_by_order: dict[int, list[str]] = {} version_steps_for_gating = version.content.get("steps", []) if version else [] @@ -2377,7 +2367,7 @@ def _se_status(se): prereq_status = ( prereq.status.value if hasattr(prereq.status, "value") else prereq.status ) - if prereq_status not in terminal_step_statuses: + if prereq_status not in exec_flow.TERMINAL_STEP_STATUSES: blockers.append(prereq.step_number_str or str(dep_order)) if blockers: gated_ops_by_order[vs["order"]] = blockers @@ -2455,9 +2445,7 @@ def _se_status(se): context["cursors_by_se"] = cursors_by_se current_user = context.get("current_user") - my_cursor = next( - (c for c in cursors if current_user and c.user_id == current_user.id), None - ) + my_cursor = next((c for c in cursors if current_user and c.user_id == current_user.id), None) context["my_cursor_order"] = ( my_cursor.step_execution.step_number if my_cursor is not None and my_cursor.step_execution is not None @@ -2489,7 +2477,7 @@ def _se_status(se): # strict_sequence display gating: sub-step N waits on its prior siblings. # Display-only — the claim API gate in core/execution_flow is authoritative. - terminal = {"completed", "signed_off", "skipped"} + terminal = exec_flow.TERMINAL_STEP_STATUSES seq_blockers_by_order: dict[int, str] = {} for op_data in ops + contingency_ops: op_vs = context["version_steps_map"].get(op_data["step"]["order"]) or {} @@ -2551,16 +2539,12 @@ def _set_bar_step(context: dict, step_order: int | None) -> None: if step_order is not None: target = next(((od, r) for od, r in rows if r["order"] == step_order), None) if target is None and context.get("my_cursor_order") is not None: - target = next( - ((od, r) for od, r in rows if r["order"] == context["my_cursor_order"]), None - ) + target = next(((od, r) for od, r in rows if r["order"] == context["my_cursor_order"]), None) if target is None: - leaf_rows = [ - (od, r) for od, r in rows if not od["sub_steps"] or r is not od["step"] - ] - target = next( - ((od, r) for od, r in leaf_rows if r["status"] in _BAR_ACTIONABLE), None - ) or (leaf_rows[0] if leaf_rows else None) + leaf_rows = [(od, r) for od, r in rows if not od["sub_steps"] or r is not od["step"]] + target = next(((od, r) for od, r in leaf_rows if r["status"] in _BAR_ACTIONABLE), None) or ( + leaf_rows[0] if leaf_rows else None + ) if target is None: context["bar_step"] = None