From 801ee9a5641b6c227d21ce844e2744f25cd3606c Mon Sep 17 00:00:00 2001 From: monthop-gmail Date: Wed, 19 Aug 2026 19:12:38 +0700 Subject: [PATCH] =?UTF-8?q?Governance=20decision=20interface=20=E2=80=94?= =?UTF-8?q?=20decision=20=E0=B9=80=E0=B8=9B=E0=B9=87=E0=B8=99=20record=20?= =?UTF-8?q?=E0=B9=84=E0=B8=A1=E0=B9=88=E0=B9=83=E0=B8=8A=E0=B9=88=20flag?= =?UTF-8?q?=20(#5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Decision` เป็น frozen dataclass ที่บังคับ 4 field ตาม RFC-0002 (decision · reason · decided_at · authority) และ serialize เป็นรูป approval/v1 ของ agent-platform · `Job._approved: bool` กลายเป็น `Job._approval: Decision | None` — ข้อห้าม "execution ก่อน APPROVED" จึงผูกกับ record จริงแทน flag ที่ใครก็ตั้งได้ REQUIRE_CHANGES: vocabulary มีครบ 3 ค่าตามสัญญา แต่ engine ปฏิเสธค่านี้ด้วย UnmappedDecision เพราะไม่มีเอกสารไหนบอกว่า job ไปสถานะไหนต่อ — approval/v1 บอกแค่ว่า "ไม่ใช่ REJECT · งานยังมีชีวิต" และ GOVERNANCE_ANALYSIS มีทางออก แค่ APPROVED/REJECTED · ไม่เดาปลายทาง ไม่เพิ่ม edge ไม่เพิ่ม state ที่ 14 ถามไว้ที่ agent-platform#22 แล้ว รอ RFC ก่อนค่อยเปิดใช้ ทุก APPROVE/REJECT emit GOVERNANCE_DECISION คู่กับ STATE_TRANSITION เสมอ ไม่ว่าจะเข้าผ่าน decide() หรือ transition() ทั่วไป — guarantee ผูกกับ state ไม่ใช่กับ method known gap: approval/v1 บังคับให้ "อ้างใบเดิม" แต่ schema ไม่มี field ให้ ใส่ supersedes_decision_id ไปก่อนและลงทะเบียนใน gaps: → agent-platform#22 pytest 350 passed (เดิม 302) · payload_check 13 passed 0 fail Co-Authored-By: Claude Opus 5 (1M context) --- ARCHITECTURE.md | 8 + conformance/payload_check.py | 84 +++- contract-semantics.yaml | 15 + packages/core/README.md | 58 ++- packages/core/devfactory_core/__init__.py | 21 +- packages/core/devfactory_core/decision.py | 193 +++++++++ packages/core/devfactory_core/errors.py | 93 +++++ packages/core/devfactory_core/events.py | 9 +- packages/core/devfactory_core/job.py | 223 +++++++++- packages/core/devfactory_core/states.py | 55 ++- packages/core/state-machine.md | 48 ++- packages/core/tests/test_decisions.py | 472 ++++++++++++++++++++++ packages/core/tests/test_events.py | 24 +- platform-contract.yaml | 22 +- 14 files changed, 1277 insertions(+), 48 deletions(-) create mode 100644 packages/core/devfactory_core/decision.py create mode 100644 packages/core/tests/test_decisions.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 91f7a99..8bf4f9f 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -21,6 +21,14 @@ Terminal: COMPLETED · FAILED · CANCELLED · TIMED_OUT Full spec: [packages/core/state-machine.md](packages/core/state-machine.md) · [RFC-0001](rfcs/0001-job-state-machine.md) as amended by [RFC-0007](rfcs/0007-job-lifecycle-completeness.md) +## Governance Decisions +`APPROVE` and `REJECT` move a job out of `GOVERNANCE_ANALYSIS`; each is recorded as an +immutable decision and emitted as `GOVERNANCE_DECISION` next to the `STATE_TRANSITION` +it caused, so no job reaches `APPROVED` without a record of who decided it and why. +`REQUIRE_CHANGES` is part of the vocabulary and is refused by the engine until an RFC +says which state it sends a job to. See [RFC-0002](rfcs/0002-governance-decision-contract.md) +and `packages/core/devfactory_core/decision.py`. + ## Multi-Tenancy Tenant → Workspace → Resource. `tenant_id` is required on every job, decision, and event; isolation is enforced at the storage layer, not by query filter. diff --git a/conformance/payload_check.py b/conformance/payload_check.py index cb5b1b2..f144115 100644 --- a/conformance/payload_check.py +++ b/conformance/payload_check.py @@ -15,6 +15,11 @@ append-only, no silent state change, no fabricated identifiers, no reasoning traces in an audit record. +The governance decisions those jobs produced are validated the same way against +``approval/v1``, together with the guarantees RFC-0002 owns: every APPROVE leaves +a record, and ``REQUIRE_CHANGES`` is refused rather than given a destination +nobody has decided on yet. + Usage:: python3 conformance/payload_check.py # fetch schemas, then validate @@ -271,6 +276,74 @@ def check_payloads(log, validator, gaps: list[dict]) -> None: ) +def check_decisions(log, jobs, validator) -> None: + """RFC-0002 — the decisions this engine produced, judged by ``approval/v1``. + + Same rule as the events above: nothing is hand-written to please the schema. + These are the records the real state machine wrote while the scenario ran. + """ + decisions = [ + payload + for tenant in log.tenants() + for payload in log.payloads(tenant) + if payload["event_type"] == "GOVERNANCE_DECISION" + ] + real = 0 + for payload in decisions: + approval = (payload.get("metadata") or {}).get("approval") + if approval is None: + real += 1 + fail( + "approval", + f"GOVERNANCE_DECISION {payload['event_id'][:8]} ไม่มี approval payload", + ) + continue + for error in sorted(validator.iter_errors(approval), key=lambda e: list(e.path)): + real += 1 + where = "/".join(str(part) for part in error.path) or "" + fail( + "approval", + f"{approval.get('decision')} ({str(approval.get('approval_id'))[:8]}): " + f"{error.message} at {where}", + ) + if real == 0: + ok("approval", f"{len(decisions)} decision ผ่าน approval/v1") + + # "ทุก APPROVE ต้อง auditable — ต้องมี event GOVERNANCE_DECISION คู่กันเสมอ" + mismatched = [ + job.job_id + for job in jobs + if len([h for h in job.history if h.to_state.value == "APPROVED"]) + != len([d for d in job.decisions if d.decision.value == "APPROVE"]) + ] + if mismatched: + fail("approval", f"เข้า APPROVED โดยไม่มี decision record: {mismatched}") + else: + ok("approval", "ทุก APPROVE มี decision record และ event คู่กับ STATE_TRANSITION") + + # REQUIRE_CHANGES อยู่ใน vocabulary แต่ยังไม่มี RFC กำหนดว่ามันพา job ไปไหน + # engine ต้องปฏิเสธ ไม่ใช่เดาปลายทาง — ดู states.DECISION_TARGET + from devfactory_core import DecisionType, Job, Principal + from devfactory_core.errors import UnmappedDecision + + probe = Job( + job_id="job-007", + tenant_id="acme", + workspace_id="ws-core", + principal=Principal("human", "alice"), + ) + probe.submit_for_governance() + try: + probe.decide( + DecisionType.REQUIRE_CHANGES, + authority=Principal("human", "bob"), + reason="needs a test plan", + ) + fail("approval", "REQUIRE_CHANGES ถูกรับเข้า — ต้องปฏิเสธจนกว่าจะมี RFC กำหนดปลายทาง") + except UnmappedDecision: + ok("approval", "REQUIRE_CHANGES ถูกปฏิเสธ — ยังไม่มี RFC กำหนดปลายทางของมัน") + + def check_gap_expiry(gaps: list[dict], today: str) -> None: """A waiver with no end date is a permanent exception, which ADR-0006 forbids.""" for gap in gaps or (): @@ -424,13 +497,20 @@ def main() -> int: "https://schemas.agent-platform.internal/event/v1/event.schema.yaml", pinned["non_schema_keys"], ) + approval_validator = build_validator( + schemas, + "https://schemas.agent-platform.internal/approval/v1/approval.schema.yaml", + pinned["non_schema_keys"], + ) log, jobs, external = run_scenario() print(f"\n[1] payload ที่ระบบผลิตจริง — {len(log)} event จาก {len(jobs)} job") check_payloads(log, validator, pinned.get("known_gaps") or []) - print("\n[2] guarantee ที่ JSON Schema ตรวจไม่ได้") + print("\n[2] คำตัดสินที่ระบบผลิตจริง — approval/v1 (RFC-0002)") + check_decisions(log, jobs, approval_validator) + print("\n[3] guarantee ที่ JSON Schema ตรวจไม่ได้") check_guarantees(log, jobs, external) - print("\n[3] ช่องว่างที่รู้ตัว — ต้องมี issue และวันหมดอายุ") + print("\n[4] ช่องว่างที่รู้ตัว — ต้องมี issue และวันหมดอายุ") check_gap_expiry(pinned.get("known_gaps") or [], args.today) fails = [f for f in findings if f[0] == "FAIL"] diff --git a/contract-semantics.yaml b/contract-semantics.yaml index c656a0b..0a71417 100644 --- a/contract-semantics.yaml +++ b/contract-semantics.yaml @@ -101,6 +101,21 @@ contracts: - agent ออก APPROVE ให้งานของตัวเองไม่ได้ — "no agent has total authority" - REQUIRE_CHANGES ไม่ใช่ REJECT — งานยังมีชีวิตและกลับมายื่นใหม่ได้ + # ℹ️ ไม่ใช่ส่วนหนึ่งของ contract — บอกสถานะฝั่ง implementation เฉย ๆ + # อยู่นอก frozen: จึงไม่ขยับ semantics_version และไม่กระทบ drift_check + implementation_status: + note: >- + packages/core ทำ decision interface ครบตาม RFC-0002 แล้ว (issue #5) + vocabulary ยังมีครบ 3 ค่าตามชุดปิด — ห้ามลบค่าใดออกจาก contract + unmapped: + - decision: REQUIRE_CHANGES + detail: >- + engine ปฏิเสธด้วย UnmappedDecision เพราะยังไม่มี RFC ที่ repo นี้บอกว่า + job ที่ถูกตีกลับให้แก้ไปอยู่ state ไหน · REJECTED ผิด invariant + ("REQUIRE_CHANGES ไม่ใช่ REJECT") · DRAFT ต้องมี edge ใหม่ที่ยังไม่มี RFC ประกาศ + · state ที่ 14 คือการเปลี่ยน lifecycle · ดู states.DECISION_TARGET + means: ค่านี้ยังอยู่ใน contract — สิ่งที่ขาดคือปลายทาง ไม่ใช่ตัวค่า + platform_may_add_freely: # เพิ่มได้ผ่าน ADR ฝั่ง agent-platform อย่างเดียว ไม่ต้องมี RFC ที่นี่ (Rule 1) # ปิดประเด็น §4.6 ของ consumer-devfactory-core.md — rfcs/0002 ไม่ได้ "ขาด" field เหล่านี้ diff --git a/packages/core/README.md b/packages/core/README.md index 6cda0b6..759f335 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -1,12 +1,15 @@ # core module -The job state machine — the control plane's lifecycle engine. +The job state machine and the governance decision interface that gates it. Spec: [`state-machine.md`](state-machine.md) — [RFC-0001](../../rfcs/0001-job-state-machine.md) as amended by [RFC-0007](../../rfcs/0007-job-lifecycle-completeness.md), with the tenant -model from [RFC-0006](../../rfcs/0006-tenant-workspace-model.md). +model from [RFC-0006](../../rfcs/0006-tenant-workspace-model.md) and decisions from +[RFC-0002](../../rfcs/0002-governance-decision-contract.md). -In memory only. No persistence, no policy engine, no API — those are issues #5, #6, and #7. +In memory only. No persistence, no policy engine, no API. *Approval* is a decision by an +authority; *policy* — whether approval was needed at all — is `policy/v1` and is not +here. ## Use @@ -14,6 +17,7 @@ In memory only. No persistence, no policy engine, no API — those are issues #5 from devfactory_core import Job, JobState, Principal alice = Principal("human", "alice") +bob = Principal("human", "bob") job = Job( job_id="job-001", tenant_id="default", # RFC-0006: never omitted, even single-tenant @@ -22,14 +26,18 @@ job = Job( ) job.submit_for_governance(reason="ready for review") -job.approve(authority=alice, reason="scope matches milestone v0.1") + +decision = job.approve(authority=bob, reason="scope matches milestone v0.1") +assert job.approval is decision # what execution runs under, not a boolean +assert decision.as_payload()["decision"] == "APPROVE" # approval/v1 wire shape + job.transition(JobState.TASK_PLANNING) job.transition(JobState.IN_PROGRESS) job.pause_for_approval(reason="merge needs sign-off") assert job.state is JobState.AWAITING_APPROVAL assert job.awaiting_from is JobState.IN_PROGRESS -job.resume(reason="approved", principal=alice) +job.resume(reason="approved", principal=bob) job.transition(JobState.VALIDATING) job.transition(JobState.DEPLOYABLE) @@ -54,27 +62,49 @@ enforced, so the engine rejects rather than repairs. | pausing outside `IN_PROGRESS` / `VALIDATING` / `DEPLOYABLE` | `MissingApprovalContext` | | resuming into a state other than `awaiting_from` | `WrongResumeState` | | a malformed identifier | `InvalidIdentifier` — `identity/v1` `Id` form | +| `decide(REQUIRE_CHANGES)` | `UnmappedDecision` — no RFC says where it sends a job | +| a decision missing decision, reason, authority, or timestamp | `IncompleteDecision` | +| an agent approving the job it is the principal for | `SelfApproval` | +| a decision from another tenant or workspace | `CrossTenantDecision` — rejected, never coerced | +| a decision about another job | `WrongDecisionSubject` | +| a decision that does not produce the transition being made | `DecisionStateMismatch` | A refused call leaves the job untouched and writes nothing to the audit trail. ## Events Construction emits `JOB_CREATED`; every accepted transition emits `STATE_TRANSITION`; -reaching `COMPLETED` also emits `JOB_COMPLETED`. There is no way to change state -without going through `transition()`, which is what makes *no silent state change* -hold rather than merely be documented. - -`event_payloads()` renders the trail in `event/v1` wire shape. It is **not** validated +entering `APPROVED` or `REJECTED` also emits `GOVERNANCE_DECISION`; reaching +`COMPLETED` also emits `JOB_COMPLETED`. There is no way to change state without going +through `transition()`, which is what makes *no silent state change* hold rather than +merely be documented — and no way to reach `APPROVED` without a decision record, which +is what makes *every APPROVE is auditable* hold. + +`event_payloads()` renders the trail in `event/v1` wire shape, and +`Decision.as_payload()` renders a decision in `approval/v1` shape. Neither is validated here — owning a copy of the schema would be a parallel schema, which -[RFC-0005](../../rfcs/0005-platform-contract-authority.md) Rule 4 forbids. Validation -against the pinned contract is issue #6, and these payloads are what it will validate. +[RFC-0005](../../rfcs/0005-platform-contract-authority.md) Rule 4 forbids. +`conformance/payload_check.py` validates both against the pinned contracts. + +## Decisions + +```python +from devfactory_core import DecisionType + +job.decide(DecisionType.APPROVE, authority=bob, reason="scope agreed") # == job.approve(...) +job.decisions # every decision made about this job, immutable, in order +job.approval # the APPROVE it executes under, or None + +job.decide(DecisionType.REQUIRE_CHANGES, authority=bob, reason="add tests") +# UnmappedDecision: declared by RFC-0002, but no RFC says which state it sends a job to +``` ## Tests ```bash cd packages/core -python -m pytest # 235 tests -python -m pytest --cov=devfactory_core # coverage gate at 90%, currently 100% +python -m pytest # the full core suite +python -m pytest --cov=devfactory_core # coverage gate at 90% ``` ## Which states may fail diff --git a/packages/core/devfactory_core/__init__.py b/packages/core/devfactory_core/__init__.py index c9ba30f..32d4627 100644 --- a/packages/core/devfactory_core/__init__.py +++ b/packages/core/devfactory_core/__init__.py @@ -1,10 +1,15 @@ """devfactory-core — governance-first control plane. Phase 1: the job state machine, in memory. See ``packages/core/state-machine.md``. +The governance decision interface it is gated by is ``decision.py`` — RFC-0002. """ +from .decision import Decision, DecisionType, Subject, new_decision_id from .errors import ( + CrossTenantDecision, + DecisionStateMismatch, ExecutionBeforeApproval, + IncompleteDecision, InvalidIdentifier, InvalidTransition, JobStateMachineError, @@ -12,22 +17,31 @@ MissingAuthority, MissingPrincipal, MissingReason, + SelfApproval, TerminalState, + UnmappedDecision, + WrongDecisionSubject, WrongResumeState, ) from .events import INTERNAL_SOURCE, Event, EventType from .identity import DEFAULT_TENANT, Principal from .job import Job, TransitionRecord -from .states import TERMINAL, TRANSITIONS, JobState +from .states import DECISION_TARGET, TERMINAL, TRANSITIONS, JobState __all__ = [ + "DECISION_TARGET", "DEFAULT_TENANT", "TERMINAL", "TRANSITIONS", "INTERNAL_SOURCE", + "CrossTenantDecision", + "Decision", + "DecisionStateMismatch", + "DecisionType", "Event", "EventType", "ExecutionBeforeApproval", + "IncompleteDecision", "InvalidIdentifier", "InvalidTransition", "Job", @@ -38,7 +52,12 @@ "MissingPrincipal", "MissingReason", "Principal", + "SelfApproval", + "Subject", "TerminalState", "TransitionRecord", + "UnmappedDecision", + "WrongDecisionSubject", "WrongResumeState", + "new_decision_id", ] diff --git a/packages/core/devfactory_core/decision.py b/packages/core/devfactory_core/decision.py new file mode 100644 index 0000000..eb3f5fc --- /dev/null +++ b/packages/core/devfactory_core/decision.py @@ -0,0 +1,193 @@ +"""Governance decisions — the interface RFC-0002 specifies. + +Canonical spec: ``rfcs/0002-governance-decision-contract.md``. This repository +owns the *semantics* of a decision (the vocabulary and the guarantees); the wire +shape is ``approval/v1`` in ``agent-platform``, which owns field names, types and +structure — RFC-0005 Rule 1. :meth:`Decision.as_payload` therefore renders to +*their* field names rather than inventing ours, and this module deliberately does +not validate: owning a copy of the schema here would be a parallel schema, which +Rule 4 forbids. It builds the payload; the contract judges it. + +The names differ in two places, on purpose: + +=========================== ========================== +here (semantics) ``approval/v1`` (wire) +=========================== ========================== +``Decision.decision_id`` ``approval_id`` +``Decision.decision`` ``decision`` +``Decision.subject`` ``subject`` — ``{type, id}`` +=========================== ========================== + +An approval *is* a decision to us and a record to them; keeping our own name and +mapping it at the boundary is what the authority split looks like in code. + +What a decision may **not** do is move a job somewhere the lifecycle does not +already go. The map from decision to destination lives in :mod:`.states` +(``DECISION_TARGET``) precisely so that it is checked against the transition +table rather than expressed twice. +""" + +from __future__ import annotations + +import uuid +from dataclasses import dataclass +from datetime import datetime +from enum import Enum +from typing import Any, Literal + +from .errors import IncompleteDecision +from .identity import Principal, validate_id + + +class DecisionType(str, Enum): + """RFC-0002's decision vocabulary — a **closed** set. + + ``contract-semantics.yaml`` marks this closed on purpose, unlike + :class:`~devfactory_core.events.EventType`, which RFC-0009 opened. The + asymmetry is deliberate: a new approval outcome (``AUTO_APPROVE``, + ``APPROVE_WITH_CONDITIONS`` with nobody checking the conditions) can let + execution proceed without a human ``APPROVE`` *by adding a value*, which is + the one thing Rule 2 exists to stop. A new event type only adds something to + observe. + + All three values are declared because the set is closed: declaring two of + three would quietly narrow the contract this repository publishes. Declaring + ``REQUIRE_CHANGES`` is not the same as being able to execute it — see + ``states.DECISION_TARGET`` for why the engine refuses it. + + ``str`` mixin so a decision serialises as its own name. + """ + + APPROVE = "APPROVE" + REJECT = "REJECT" + REQUIRE_CHANGES = "REQUIRE_CHANGES" + + +#: ``approval/v1`` ``subject.type`` — what a decision can be *about*. The enum is +#: agent-platform's (Rule 1); it is mirrored here for the same reason +#: :mod:`.identity` mirrors the ``Id`` pattern — to refuse a malformed subject +#: before it reaches an audit record, not to own the vocabulary. +SUBJECT_TYPES: frozenset[str] = frozenset( + {"job", "execution", "tool_call", "artifact", "deployment"} +) + +SubjectTypeName = Literal["job", "execution", "tool_call", "artifact", "deployment"] + + +def new_decision_id() -> str: + """A fresh decision id in the identity/v1 ``Id`` form. + + ``uuid4().hex`` is 32 lowercase hex characters, which satisfies the pattern + without needing to be reshaped. + """ + return uuid.uuid4().hex + + +@dataclass(frozen=True, slots=True) +class Subject: + """What a decision is about — ``approval/v1`` ``subject``. + + A decision that cannot say what it decided about is not auditable, which is + why both halves are required and neither has a default. + """ + + type: SubjectTypeName + id: str + + def __post_init__(self) -> None: + if self.type not in SUBJECT_TYPES: + raise ValueError( + f"subject type must be one of {sorted(SUBJECT_TYPES)} — got {self.type!r}" + ) + validate_id("subject.id", self.id) + + def as_payload(self) -> dict[str, str]: + return {"type": self.type, "id": self.id} + + +@dataclass(frozen=True, slots=True) +class Decision: + """One governance decision. Frozen — RFC-0002 guarantees decisions are immutable. + + "Immutable" is enforced here rather than documented: changing your mind is a + *new* decision that cites the one it replaces, which is what + ``supersedes_decision_id`` carries. + + RFC-0002's four required meanings map to ``decision``, ``reason``, + ``authority`` and ``decided_at``. ``tenant_id`` is required by RFC-0006 and + carries the invariant that a decision lives in the same tenant as the thing + it decides about — checked by the engine, never coerced. + """ + + decision_id: str + tenant_id: str + subject: Subject + decision: DecisionType + reason: str + authority: Principal + decided_at: datetime + workspace_id: str | None = None + + #: The decision this one replaces. + #: + #: ``approval/v1`` states the guarantee — "การเปลี่ยนใจคือ approval ใบใหม่ที่ + #: อ้างใบเดิม" — and the pinned schema (agent-platform @ 7263588) has no field + #: to put the citation in. We fill it on our side anyway: the alternative is + #: dropping a link the guarantee explicitly requires. ``approval/v1`` does not + #: set ``additionalProperties: false``, so the payload still validates today, + #: and if agent-platform names the field differently this becomes a rename + #: rather than a redesign. Recorded as a gap in ``platform-contract.yaml`` so + #: they see it without having to ask. + supersedes_decision_id: str | None = None + + def __post_init__(self) -> None: + validate_id("decision_id", self.decision_id) + validate_id("tenant_id", self.tenant_id) + if self.workspace_id is not None: + validate_id("workspace_id", self.workspace_id) + if self.supersedes_decision_id is not None: + validate_id("supersedes_decision_id", self.supersedes_decision_id) + if not isinstance(self.subject, Subject): + raise IncompleteDecision("subject") + # A plain string is accepted the way ``transition()`` accepts one, and an + # unknown value raises ValueError rather than being kept as-is: the + # vocabulary is closed, so there is no "unrecognised but keep it" case + # here of the kind event/v1 requires for event types. + object.__setattr__(self, "decision", DecisionType(self.decision)) + if not isinstance(self.authority, Principal): + raise IncompleteDecision("authority") + if not isinstance(self.reason, str) or not self.reason.strip(): + raise IncompleteDecision("reason") + if not isinstance(self.decided_at, datetime): + raise IncompleteDecision("decided_at") + + def as_payload(self) -> dict[str, Any]: + """Render to the ``approval/v1`` wire shape. + + Keys are omitted when unset rather than sent as null or an empty string — + RFC-0008's rule against inventing a value to satisfy a field applies to + every payload this repository produces, not only to events. + """ + payload: dict[str, Any] = { + # agent-platform's name for it (RFC-0005 Rule 1) — ours is decision_id. + "approval_id": self.decision_id, + "tenant_id": self.tenant_id, + "subject": self.subject.as_payload(), + "decision": self.decision.value, + "reason": self.reason, + "authority": self.authority.as_payload(), + "decided_at": self.decided_at.isoformat(), + } + if self.workspace_id is not None: + payload["workspace_id"] = self.workspace_id + if self.supersedes_decision_id is not None: + # Our field, not theirs — see the field comment above. + payload["supersedes_decision_id"] = self.supersedes_decision_id + return payload + + def __repr__(self) -> str: + return ( + f"Decision(decision={self.decision.value}, " + f"subject={self.subject.type}:{self.subject.id}, " + f"authority={self.authority.id!r})" + ) diff --git a/packages/core/devfactory_core/errors.py b/packages/core/devfactory_core/errors.py index e5264bc..a9fc5e5 100644 --- a/packages/core/devfactory_core/errors.py +++ b/packages/core/devfactory_core/errors.py @@ -105,6 +105,99 @@ def __init__(self, state: str) -> None: ) +class UnmappedDecision(JobStateMachineError): + """A decision this repository has declared but has not yet said what to do with. + + ``REQUIRE_CHANGES`` is the only one today. RFC-0002 declares all three + decision types and the vocabulary is a closed set, so the type has to exist; + what no RFC here says is **which state a job lands in** when it is returned + for changes. The engine refuses rather than guessing, because a governance + record whose recorded meaning is not the meaning that was made is worse than + a refusal. See ``states.DECISION_TARGET`` for the candidates and why each + needs an RFC first. + """ + + def __init__(self, decision: str) -> None: + self.decision = decision + super().__init__( + f"{decision} is declared by RFC-0002 but no RFC in this repository says " + f"which state it moves a job to — the engine will not guess a destination. " + f"Settling that is an RFC (see states.DECISION_TARGET), not a code change." + ) + + +class IncompleteDecision(JobStateMachineError): + """A decision missing one of the four meanings RFC-0002 requires.""" + + def __init__(self, field: str) -> None: + self.field = field + super().__init__( + f"a decision without {field} is not auditable — RFC-0002 requires the " + f"decision, the reason for it, the authority accountable for it, and when " + f"it was made" + ) + + +class SelfApproval(JobStateMachineError): + """An agent tried to APPROVE the work it is itself accountable for. + + "No agent has total authority" — RFC-0002 rejects agent self-approval by + name and ``approval/v1`` states it as an invariant on ``authority``. + """ + + def __init__(self, authority_id: str, job_id: str) -> None: + self.authority_id = authority_id + self.job_id = job_id + super().__init__( + f"agent {authority_id!r} cannot APPROVE {job_id} — it is the principal " + f"accountable for that job, and no agent has total authority over its own work" + ) + + +class CrossTenantDecision(JobStateMachineError): + """A decision from one tenant tried to decide another tenant's job. + + RFC-0006 and ``approval/v1``: a mismatch is invalid and must be **rejected, + never coerced** — silently rewriting the tenant is how an isolation boundary + stops being one. + """ + + def __init__(self, field: str, decision_value: str, subject_value: str) -> None: + self.field = field + self.decision_value = decision_value + self.subject_value = subject_value + super().__init__( + f"decision {field}={decision_value!r} does not match the job's " + f"{field}={subject_value!r} — a decision must live in the same scope as what " + f"it decides about, and a mismatch is rejected, never coerced" + ) + + +class WrongDecisionSubject(JobStateMachineError): + """A decision about something else was offered as this job's decision.""" + + def __init__(self, subject_type: str, subject_id: str, job_id: str) -> None: + self.subject_type = subject_type + self.subject_id = subject_id + self.job_id = job_id + super().__init__( + f"decision is about {subject_type}:{subject_id}, not job:{job_id} — " + f"an approval granted to one subject cannot authorise another" + ) + + +class DecisionStateMismatch(JobStateMachineError): + """The decision offered does not produce the transition being made.""" + + def __init__(self, decision: str, requested: str) -> None: + self.decision = decision + self.requested = requested + super().__init__( + f"a {decision} decision does not move a job to {requested} — the audit " + f"trail would record a decision that was never made" + ) + + class InvalidIdentifier(JobStateMachineError): """Identifiers must match the identity/v1 Id form.""" diff --git a/packages/core/devfactory_core/events.py b/packages/core/devfactory_core/events.py index e4538df..a54aee0 100644 --- a/packages/core/devfactory_core/events.py +++ b/packages/core/devfactory_core/events.py @@ -36,11 +36,14 @@ class EventType(str, Enum): which is noted below and is not enforced by this enum. Emitted by the job state machine (``packages/core``): - ``JOB_CREATED`` · ``STATE_TRANSITION`` · ``JOB_COMPLETED`` - - Emitted by governance (issue #5): + ``JOB_CREATED`` · ``STATE_TRANSITION`` · ``JOB_COMPLETED`` · ``GOVERNANCE_DECISION`` + ``GOVERNANCE_DECISION`` is emitted alongside the ``STATE_TRANSITION`` it + causes, never instead of it (issue #5, RFC-0002): "every APPROVE is + auditable" is not satisfied by a state change that leaves no record of who + decided it. Its subject is the approval; the job is on ``job_id``. + Emitted by orchestration and execution (issue #7 and later): ``TASK_ASSIGNED`` · ``EXECUTION_STARTED`` · ``EXECUTION_FAILED`` """ diff --git a/packages/core/devfactory_core/job.py b/packages/core/devfactory_core/job.py index f0f143c..0dd09d3 100644 --- a/packages/core/devfactory_core/job.py +++ b/packages/core/devfactory_core/job.py @@ -10,6 +10,10 @@ IN_PROGRESS, AWAITING_APPROVAL, VALIDATING, and DEPLOYABLE only — the states where work exists to fail — and refused before APPROVED, where the honest outcomes are REJECTED, CANCELLED, or TIMED_OUT. See ``states.FAILABLE``. + +Governance decisions are RFC-0002, added by issue #5. A job holds the decision it +is executing under (``approval``) rather than a boolean, because "approved" is +not a fact about a job — it is a record of who decided what, when, and why. """ from __future__ import annotations @@ -18,20 +22,28 @@ from datetime import datetime from typing import Any, Callable +from .decision import Decision, DecisionType, Subject, new_decision_id from .errors import ( + CrossTenantDecision, + DecisionStateMismatch, ExecutionBeforeApproval, InvalidTransition, MissingApprovalContext, MissingAuthority, MissingPrincipal, MissingReason, + SelfApproval, TerminalState, + UnmappedDecision, + WrongDecisionSubject, WrongResumeState, ) from .events import Event, EventType, new_event_id, utc_now from .identity import Principal, validate_id from .states import ( APPROVAL_PAUSABLE, + DECISION_BY_TARGET, + DECISION_TARGET, POST_APPROVAL, TERMINAL, TRANSITIONS, @@ -45,9 +57,9 @@ ) #: States whose entry is a decision and must name an accountable authority. -AUTHORITY_REQUIRED: frozenset[JobState] = frozenset( - {JobState.APPROVED, JobState.REJECTED} -) +#: Derived from ``DECISION_TARGET`` rather than listed again, so a decision type +#: gaining a destination cannot leave its destination state ungoverned. +AUTHORITY_REQUIRED: frozenset[JobState] = frozenset(DECISION_TARGET.values()) @dataclass(frozen=True, slots=True) @@ -60,15 +72,20 @@ class TransitionRecord: reason: str | None = None principal: Principal | None = None event_id: str | None = None + #: The decision that caused this transition, for the two states that have one. + decision_id: str | None = None class Job: """A governed unit of work moving through the lifecycle. Construction emits ``JOB_CREATED``; every accepted transition emits - ``STATE_TRANSITION``; reaching COMPLETED also emits ``JOB_COMPLETED``. There - is no way to change ``state`` without going through :meth:`transition`, which - is what makes "no silent state change" hold rather than merely be documented. + ``STATE_TRANSITION``; entering a decision state also emits + ``GOVERNANCE_DECISION``; reaching COMPLETED also emits ``JOB_COMPLETED``. + There is no way to change ``state`` without going through + :meth:`transition`, which is what makes "no silent state change" hold rather + than merely be documented — and no way to reach ``APPROVED`` without leaving + a decision record, which is what makes "every APPROVE is auditable" hold. """ def __init__( @@ -99,7 +116,10 @@ def __init__( self._state = JobState.DRAFT self._awaiting_from: JobState | None = None - self._approved = False + # RFC-0002: what authorises execution is a decision, not a flag. Holding + # the record means the engine can always answer "on whose authority?" + self._approval: Decision | None = None + self._decisions: list[Decision] = [] self._history: list[TransitionRecord] = [] self._events: list[Event] = [] @@ -145,6 +165,24 @@ def awaiting_from(self) -> JobState | None: def is_terminal(self) -> bool: return self._state in TERMINAL + @property + def approval(self) -> Decision | None: + """The APPROVE this job executes under, or None. + + Cleared by a REJECT: an approval granted to an earlier revision must not + authorise the revised one. + """ + return self._approval + + @property + def decisions(self) -> tuple[Decision, ...]: + """Every governance decision made about this job, in order. + + Immutable records of immutable objects — a decision is never edited, so + changing one's mind appears here as a second decision citing the first. + """ + return tuple(self._decisions) + @property def history(self) -> tuple[TransitionRecord, ...]: return tuple(self._history) @@ -176,17 +214,38 @@ def transition( *, reason: str | None = None, principal: Principal | None = None, + decision: Decision | None = None, ) -> Event: """Move to ``to``, or refuse and leave the job untouched. Returns the ``STATE_TRANSITION`` event so a caller can forward it without reaching back into the history. + + ``decision`` is how :meth:`decide` hands its record to the one method that + may change state. Entering a decision state without one is still allowed + and still produces a record — see :meth:`_decision_for`. """ to = JobState(to) + if decision is not None and not isinstance(decision, Decision): + raise TypeError("decision must be a Decision — 'who decided what' is required") self._check_not_terminal() self._check_edge(to) self._check_guards(to, reason=reason, principal=principal) + record: Decision | None = None + if to in AUTHORITY_REQUIRED: + # "Every APPROVE is auditable" is a guarantee about the state, not + # about which method the caller reached for. Entering APPROVED or + # REJECTED through the generic API therefore mints the same record + # decide() would have: the guards above have already established that + # an authority and a reason are present. + record = decision if decision is not None else self._decision_for( + to, authority=principal, reason=reason + ) + self._check_decision(record, to) + elif decision is not None: + raise DecisionStateMismatch(decision.decision.value, to.value) + previous = self._state # AWAITING_APPROVAL's return address is recorded on entry and cleared on # exit, so a job paused during DEPLOYABLE cannot resume as IN_PROGRESS. @@ -196,15 +255,30 @@ def transition( self._awaiting_from = None if to is JobState.APPROVED: - self._approved = True + self._approval = record elif to is JobState.REJECTED: # A rejected job returns to DRAFT for revision. The approval that was # never granted must not carry over, and an approval granted to an # earlier revision must not authorise the revised one. - self._approved = False + self._approval = None self._state = to + if record is not None: + self._decisions.append(record) + # The decision is emitted before the transition it caused, so the + # trail reads in causal order. Its subject is the approval itself, + # with job_id naming the job it belongs to — event/v1's own example + # of the distinction (EXECUTION_STARTED is about the execution and + # carries the job it sits under). + self._emit( + EventType.GOVERNANCE_DECISION, + actor=record.authority, + subject_type="approval", + subject_id=record.decision_id, + metadata={"approval": record.as_payload()}, + ) + event = self._emit( EventType.STATE_TRANSITION, actor=principal or self._principal, @@ -218,6 +292,7 @@ def transition( reason=reason, principal=principal, event_id=event.event_id, + decision_id=record.decision_id if record is not None else None, ) ) if to is JobState.COMPLETED: @@ -231,11 +306,60 @@ def transition( def submit_for_governance(self, *, reason: str | None = None) -> Event: return self.transition(JobState.GOVERNANCE_ANALYSIS, reason=reason) - def approve(self, *, authority: Principal, reason: str) -> Event: - return self.transition(JobState.APPROVED, reason=reason, principal=authority) + # ---- governance decisions, RFC-0002 ------------------------------------ - def reject(self, *, authority: Principal, reason: str) -> Event: - return self.transition(JobState.REJECTED, reason=reason, principal=authority) + def decide( + self, + decision: DecisionType | str, + *, + authority: Principal, + reason: str, + supersedes_decision_id: str | None = None, + ) -> Decision: + """Record a governance decision and move the job where it sends it. + + Returns the :class:`~devfactory_core.decision.Decision` — the record is + what a caller wants to forward, and the events it produced are on + :attr:`events`. The job moves and the decision is written in one call + because they are one act: a decision that does not move the job is not a + decision, and a move without a decision is what this whole module exists + to prevent. + + Refuses ``REQUIRE_CHANGES`` with ``UnmappedDecision``: RFC-0002 declares + it, no RFC here says where it sends a job, and the engine will not invent + a destination. See ``states.DECISION_TARGET``. + """ + decision = DecisionType(decision) + target = DECISION_TARGET.get(decision) + if target is None: + raise UnmappedDecision(decision.value) + record = Decision( + decision_id=new_decision_id(), + tenant_id=self._tenant_id, + workspace_id=self._workspace_id, + subject=Subject("job", self._job_id), + decision=decision, + reason=reason, + authority=authority, + decided_at=self._clock(), + # "Changing your mind is a new approval that cites the old one" + # (approval/v1). The citation is filled from this job's own history + # rather than left to the caller to remember — it points at a + # decision that really was made, so nothing is being fabricated. + supersedes_decision_id=( + supersedes_decision_id + if supersedes_decision_id is not None + else (self._decisions[-1].decision_id if self._decisions else None) + ), + ) + self.transition(target, reason=reason, principal=authority, decision=record) + return record + + def approve(self, *, authority: Principal, reason: str) -> Decision: + return self.decide(DecisionType.APPROVE, authority=authority, reason=reason) + + def reject(self, *, authority: Principal, reason: str) -> Decision: + return self.decide(DecisionType.REJECT, authority=authority, reason=reason) def pause_for_approval(self, *, reason: str | None = None) -> Event: return self.transition(JobState.AWAITING_APPROVAL, reason=reason) @@ -312,9 +436,70 @@ def _check_guards( # Structural backstop for the direction lock. The table already makes # APPROVED the only way in, so this can only fire if the table is edited # wrongly — which is exactly when it is worth having. - if to in POST_APPROVAL and not self._approved: + if to in POST_APPROVAL and self._approval is None: raise ExecutionBeforeApproval(to.value) + def _decision_for( + self, to: JobState, *, authority: Principal | None, reason: str | None + ) -> Decision: + """Mint the decision that entering ``to`` must have been. + + Only reachable for states in ``AUTHORITY_REQUIRED``, and only after the + guards have established that both an authority and a reason are present — + so nothing here is invented to fill a field. + """ + assert authority is not None and reason is not None # guaranteed by _check_guards + return Decision( + decision_id=new_decision_id(), + tenant_id=self._tenant_id, + workspace_id=self._workspace_id, + subject=Subject("job", self._job_id), + decision=DECISION_BY_TARGET[to], + reason=reason, + authority=authority, + decided_at=self._clock(), + supersedes_decision_id=( + self._decisions[-1].decision_id if self._decisions else None + ), + ) + + def _check_decision(self, record: Decision, to: JobState) -> None: + """Refuse a decision that does not belong to this job or this transition.""" + if DECISION_TARGET.get(record.decision) is not to: + raise DecisionStateMismatch(record.decision.value, to.value) + if record.subject != Subject("job", self._job_id): + raise WrongDecisionSubject( + record.subject.type, record.subject.id, self._job_id + ) + # RFC-0006: a decision belongs to the same scope as what it decides about. + # Reject the mismatch; never quietly rewrite it to match. + if record.tenant_id != self._tenant_id: + raise CrossTenantDecision("tenant_id", record.tenant_id, self._tenant_id) + if record.workspace_id is not None and record.workspace_id != self._workspace_id: + raise CrossTenantDecision( + "workspace_id", record.workspace_id, self._workspace_id + ) + if record.decision is DecisionType.APPROVE and self._is_self_approval( + record.authority + ): + raise SelfApproval(record.authority.id, self._job_id) + + def _is_self_approval(self, authority: Principal) -> bool: + """Whether this APPROVE would be an agent approving its own work. + + Scoped to agents on purpose. ``approval/v1`` states the invariant as + "agent ออก APPROVE ให้งานของตัวเองไม่ได้ — authority.id ต้องไม่ใช่ agent_id + ของงานที่กำลังอนุมัติ", and RFC-0002 rejects "agent self-approval" by name. + A person approving a job they filed is a *different* rule: it may well be + one this repository wants, but adopting it here would make the engine + stricter than the contract it conforms to, and stricter is still + different. That belongs in an RFC, not in an implementation detail. + + The job has no ``agent_id`` of its own, so the principal accountable for + the job — the one that created it — is what ``agent_id`` maps to here. + """ + return authority.type == "agent" and authority.id == self._principal.id + # ---- emit -------------------------------------------------------------- def _transition_payload( @@ -334,14 +519,20 @@ def _emit( actor: Principal | None = None, transition: dict[str, Any] | None = None, metadata: dict[str, Any] | None = None, + subject_type: str = "job", + subject_id: str | None = None, ) -> Event: + # The subject defaults to the job because almost every event this engine + # emits is about the job. GOVERNANCE_DECISION is about the approval and + # says so; job_id stays on every event either way, which is the rule + # RFC-0008 holds us to. event = Event( event_id=new_event_id(), event_type=event_type, tenant_id=self._tenant_id, workspace_id=self._workspace_id, - subject_type="job", - subject_id=self._job_id, + subject_type=subject_type, + subject_id=subject_id or self._job_id, job_id=self._job_id, occurred_at=self._clock(), actor=actor, diff --git a/packages/core/devfactory_core/states.py b/packages/core/devfactory_core/states.py index 6b2f6ef..23d7875 100644 --- a/packages/core/devfactory_core/states.py +++ b/packages/core/devfactory_core/states.py @@ -2,13 +2,17 @@ Canonical spec: ``packages/core/state-machine.md`` — RFC-0001 as amended by RFC-0007. This module is the single place the transition table is expressed in code; nothing -else may hard-code an edge. +else may hard-code an edge. ``DECISION_TARGET`` lives here for the same reason: +where a governance decision (RFC-0002) sends a job has to be checked against the +table, not asserted separately from it. """ from __future__ import annotations from enum import Enum +from .decision import DecisionType + class JobState(str, Enum): """The thirteen job states. @@ -121,6 +125,55 @@ def _build() -> dict[JobState, frozenset[JobState]]: TRANSITIONS: dict[JobState, frozenset[JobState]] = _build() +#: Where a governance decision sends a job — RFC-0002 meeting RFC-0001. +#: +#: Every destination here is an edge ``_PROGRESSION`` already declares out of +#: ``GOVERNANCE_ANALYSIS``. A decision may not invent a transition: if a decision +#: needs a new edge, that edge is a lifecycle change and belongs in an RFC first +#: (``docs/governance/CORE_BOUNDARY.md``). ``test_decisions.py`` asserts this +#: containment so the rule cannot quietly lapse. +#: +#: ``REQUIRE_CHANGES`` is deliberately absent, and its absence is the decision, +#: not an oversight: +#: +#: * RFC-0002 declares it and ``contract-semantics.yaml`` marks the vocabulary a +#: closed set, so :class:`~devfactory_core.decision.DecisionType` must carry all +#: three values. Dropping it would narrow the contract we publish. +#: * ``approval/v1`` says what it *means* — "REQUIRE_CHANGES ไม่ใช่ REJECT — งานยัง +#: มีชีวิตและกลับมายื่นใหม่ได้" — but nothing in this repository says which state +#: the job lands in, and that is the part the engine would need. +#: +#: Each candidate destination needs something that does not exist yet: +#: +#: * ``REJECTED`` — forbidden outright by the invariant above: it is not a REJECT, +#: and recording it as one would make the audit trail say the wrong thing. +#: * ``DRAFT`` — needs a ``GOVERNANCE_ANALYSIS -> DRAFT`` edge that neither +#: RFC-0001 nor RFC-0007 declares, and it would erase the difference from the +#: ``REJECTED -> DRAFT`` path, which is exactly the distinction the invariant +#: asks us to keep. +#: * a fourteenth state (``CHANGES_REQUESTED``) — a new state, which is a +#: lifecycle change and needs an RFC. +#: +#: So :meth:`~devfactory_core.job.Job.decide` raises ``UnmappedDecision`` for it. +#: Guessing would be the one failure mode governance cannot afford: an audit +#: record whose meaning is not the meaning that was decided. When an RFC settles +#: the destination, the fix is one entry in this dict plus whatever edge that RFC +#: declares — deliberately a small change, sitting behind a decision only a human +#: can make. +DECISION_TARGET: dict[DecisionType, JobState] = { + DecisionType.APPROVE: JobState.APPROVED, + DecisionType.REJECT: JobState.REJECTED, +} + +#: The inverse. Entering ``APPROVED`` or ``REJECTED`` through the generic +#: ``transition()`` still has to produce a decision record — "every APPROVE is +#: auditable" is a guarantee about the state, not about which method was called — +#: and this is how that path names the decision it must have been. +DECISION_BY_TARGET: dict[JobState, DecisionType] = { + target: decision for decision, target in DECISION_TARGET.items() +} + + def static_targets(state: JobState) -> frozenset[JobState]: """States reachable from ``state`` without per-job context.""" return TRANSITIONS[state] diff --git a/packages/core/state-machine.md b/packages/core/state-machine.md index 88ff9d5..6124ed4 100644 --- a/packages/core/state-machine.md +++ b/packages/core/state-machine.md @@ -47,6 +47,39 @@ Execution is forbidden before `APPROVED`. before `APPROVED`, where the honest outcomes are `REJECTED`, `CANCELLED`, or `TIMED_OUT` ([RFC-0010](../../rfcs/0010-failable-states.md)). +## Governance decisions + +Per [RFC-0002](../../rfcs/0002-governance-decision-contract.md), rendered on the wire +as `approval/v1`. A decision is what moves a job out of `GOVERNANCE_ANALYSIS`; the +engine records it and emits `GOVERNANCE_DECISION` alongside the `STATE_TRANSITION` it +caused — an approval that leaves no record is not auditable, so there is no path to +`APPROVED` that skips one. + +| decision | job goes to | +| --- | --- | +| `APPROVE` | `APPROVED` | +| `REJECT` | `REJECTED` | +| `REQUIRE_CHANGES` | **refused — `UnmappedDecision`** | + +`REQUIRE_CHANGES` is part of the vocabulary (a closed set: dropping it would narrow +the contract this repository publishes) and has no destination, because no RFC here +says which state a job returned for changes lands in. `REJECTED` is ruled out by the +invariant *"REQUIRE_CHANGES ไม่ใช่ REJECT"*; `DRAFT` needs a `GOVERNANCE_ANALYSIS → +DRAFT` edge nothing declares; a fourteenth state is a lifecycle change. The engine +refuses rather than guessing — see `states.DECISION_TARGET`, and +[the open questions](#open-questions) below. + +Guarantees the engine enforces, not just documents: + +- decisions are immutable — changing one's mind is a second decision citing the first + (`supersedes_decision_id`) +- a decision lives in the same tenant and workspace as the job it decides about; + a mismatch is rejected, never coerced +- an agent may not `APPROVE` a job it is the principal for — *no agent has total + authority* +- execution stays locked until the job holds an `APPROVE` record, not merely the + `APPROVED` state + ## AWAITING_APPROVAL Entered when at least one execution of the job is in `awaiting_approval` @@ -72,6 +105,19 @@ definitions: ## Guarantees - Every transition emits a `STATE_TRANSITION` event — no silent state change. -- `APPROVED` requires an explicit governance decision. +- `APPROVED` requires an explicit governance decision, recorded and emitted. - `FAILED`, `CANCELLED`, and `TIMED_OUT` all require reason metadata. - `CANCELLED` records the cancelling principal. + +## Open questions + +Recorded rather than answered — each needs an RFC, not a code change. + +- Where does `REQUIRE_CHANGES` send a job? Until an RFC says, the engine refuses it. +- May a *person* approve a job they filed? `approval/v1` and RFC-0002 both state the + self-approval invariant about agents only, so the engine refuses agent + self-approval and allows the human case. Widening it would make this engine + stricter than the contract it conforms to. +- `APPROVED` is in neither `FAILABLE` nor `TIMEOUTABLE`, so an approval nobody acts on + has no automatic exit — [RFC-0010](../../rfcs/0010-failable-states.md) records this, + and it is issue #17. diff --git a/packages/core/tests/test_decisions.py b/packages/core/tests/test_decisions.py new file mode 100644 index 0000000..baca06a --- /dev/null +++ b/packages/core/tests/test_decisions.py @@ -0,0 +1,472 @@ +"""Governance decisions — RFC-0002's interface, and what it refuses. + +The decision *is* the governance layer: everything else in this package exists to +make sure a job cannot execute without one. So most of what is asserted here is a +refusal. +""" + +from __future__ import annotations + +import pytest + +from conftest import drive +from devfactory_core import ( + Decision, + DecisionType, + Job, + JobState, + Principal, + Subject, +) +from devfactory_core.decision import new_decision_id +from devfactory_core.errors import ( + CrossTenantDecision, + DecisionStateMismatch, + ExecutionBeforeApproval, + IncompleteDecision, + InvalidIdentifier, + SelfApproval, + UnmappedDecision, + WrongDecisionSubject, +) +from devfactory_core.identity import ID_PATTERN +from devfactory_core.states import DECISION_BY_TARGET, DECISION_TARGET, TRANSITIONS + + +@pytest.fixture +def reviewer() -> Principal: + """Someone other than the job's own principal — the usual case.""" + return Principal("human", "bob", display_name="Bob") + + +def _fresh(alice, clock, **kw) -> Job: + base = dict( + job_id="job-001", tenant_id="acme", workspace_id="ws-core", principal=alice, clock=clock + ) + base.update(kw) + return Job(**base) + + +def _at_the_gate(alice, clock, **kw) -> Job: + job = _fresh(alice, clock, **kw) + job.submit_for_governance() + return job + + +# ---- the vocabulary, RFC-0002 ---------------------------------------------- + + +def test_all_three_decision_types_are_declared(): + """A closed set declared in full — two of three would narrow the contract.""" + assert {d.value for d in DecisionType} == {"APPROVE", "REJECT", "REQUIRE_CHANGES"} + + +def test_a_decision_only_moves_a_job_along_an_edge_that_already_exists(): + """A decision may not invent a transition — that would be a lifecycle change.""" + declared = TRANSITIONS[JobState.GOVERNANCE_ANALYSIS] + assert set(DECISION_TARGET.values()) <= set(declared) + assert DECISION_BY_TARGET == {t: d for d, t in DECISION_TARGET.items()} + + +def test_require_changes_is_declared_but_has_no_destination(): + """Declared because the vocabulary is closed; unmapped because no RFC says where.""" + assert DecisionType.REQUIRE_CHANGES not in DECISION_TARGET + + +def test_require_changes_is_refused_and_says_why(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + before = len(job.events) + with pytest.raises(UnmappedDecision) as excinfo: + job.decide(DecisionType.REQUIRE_CHANGES, authority=reviewer, reason="needs tests") + assert "RFC" in str(excinfo.value) + # A refusal leaves the job exactly as it was — no state, no record, no event. + assert job.state is JobState.GOVERNANCE_ANALYSIS + assert job.decisions == () + assert len(job.events) == before + + +def test_require_changes_is_refused_by_its_string_form_too(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + with pytest.raises(UnmappedDecision): + job.decide("REQUIRE_CHANGES", authority=reviewer, reason="needs tests") + + +def test_a_decision_outside_the_vocabulary_is_not_invented(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + with pytest.raises(ValueError): + job.decide("AUTO_APPROVE", authority=reviewer, reason="looks fine") + + +# ---- the decision record --------------------------------------------------- + + +def test_approve_records_the_decision_and_moves_the_job(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + record = job.approve(authority=reviewer, reason="scope matches milestone v0.1") + + assert isinstance(record, Decision) + assert record.decision is DecisionType.APPROVE + assert record.reason == "scope matches milestone v0.1" + assert record.authority is reviewer + assert record.subject == Subject("job", "job-001") + assert record.tenant_id == "acme" + assert record.workspace_id == "ws-core" + assert job.state is JobState.APPROVED + assert job.approval is record + assert job.decisions == (record,) + + +def test_decide_is_the_general_form_of_approve_and_reject(alice, reviewer, clock): + approved = _at_the_gate(alice, clock) + approved.decide("APPROVE", authority=reviewer, reason="fine") + assert approved.state is JobState.APPROVED + + rejected = _at_the_gate(alice, clock, job_id="job-002") + rejected.decide(DecisionType.REJECT, authority=reviewer, reason="missing risk analysis") + assert rejected.state is JobState.REJECTED + + +def test_a_decision_is_immutable(alice, reviewer, clock): + record = _at_the_gate(alice, clock).approve(authority=reviewer, reason="ok") + with pytest.raises(Exception): + record.reason = "something else" # type: ignore[misc] + + +def test_the_decision_list_is_a_read_only_copy(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + job.approve(authority=reviewer, reason="ok") + assert isinstance(job.decisions, tuple) + with pytest.raises(AttributeError): + job.decisions.append("forged") # type: ignore[attr-defined] + + +def test_changing_your_mind_cites_the_decision_it_replaces(alice, reviewer, clock): + """approval/v1: a decision is never edited — the second one references the first.""" + job = _at_the_gate(alice, clock) + first = job.reject(authority=reviewer, reason="missing risk analysis") + job.transition(JobState.DRAFT) + job.submit_for_governance(reason="risk analysis added") + second = job.approve(authority=reviewer, reason="addressed") + + assert first.supersedes_decision_id is None + assert second.supersedes_decision_id == first.decision_id + assert job.decisions == (first, second) + + +def test_an_explicit_citation_wins_over_the_inferred_one(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + earlier = new_decision_id() + record = job.decide( + DecisionType.APPROVE, + authority=reviewer, + reason="carrying a decision made elsewhere", + supersedes_decision_id=earlier, + ) + assert record.supersedes_decision_id == earlier + + +def test_rejection_leaves_no_approval_behind(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + job.reject(authority=reviewer, reason="out of scope") + assert job.approval is None + assert job.decisions[-1].decision is DecisionType.REJECT + + +def test_the_approval_says_who_authorised_execution(alice, reviewer, clock): + job = drive(_fresh(alice, clock), JobState.IN_PROGRESS, reviewer) + assert job.approval is not None + assert job.approval.authority is reviewer + assert job.history[-1].decision_id is None # the move into IN_PROGRESS is not a decision + + +def test_history_links_the_transition_to_the_decision(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + record = job.approve(authority=reviewer, reason="ok") + assert job.history[-1].to_state is JobState.APPROVED + assert job.history[-1].decision_id == record.decision_id + + +# ---- the four required meanings -------------------------------------------- + + +@pytest.mark.parametrize("blank", ["", " ", "\n"], ids=repr) +def test_a_decision_without_a_reason_is_refused(blank, alice, reviewer, clock): + job = _at_the_gate(alice, clock) + with pytest.raises(IncompleteDecision) as excinfo: + job.approve(authority=reviewer, reason=blank) + assert excinfo.value.field == "reason" + assert job.state is JobState.GOVERNANCE_ANALYSIS + + +def test_a_decision_without_an_authority_is_refused(alice, clock): + job = _at_the_gate(alice, clock) + with pytest.raises(IncompleteDecision) as excinfo: + job.approve(authority=None, reason="signed by nobody") # type: ignore[arg-type] + assert excinfo.value.field == "authority" + + +def test_a_decision_without_a_timestamp_is_refused(alice, reviewer): + with pytest.raises(IncompleteDecision) as excinfo: + Decision( + decision_id=new_decision_id(), + tenant_id="acme", + subject=Subject("job", "job-001"), + decision=DecisionType.APPROVE, + reason="ok", + authority=reviewer, + decided_at="2026-08-19T09:00:00+00:00", # type: ignore[arg-type] + ) + assert excinfo.value.field == "decided_at" + + +def test_a_decision_without_a_subject_is_refused(reviewer, clock): + with pytest.raises(IncompleteDecision) as excinfo: + Decision( + decision_id=new_decision_id(), + tenant_id="acme", + subject="job-001", # type: ignore[arg-type] + decision=DecisionType.APPROVE, + reason="ok", + authority=reviewer, + decided_at=clock(), + ) + assert excinfo.value.field == "subject" + + +def test_the_decision_is_timestamped_from_the_job_clock(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + record = job.approve(authority=reviewer, reason="ok") + assert record.decided_at.tzinfo is not None + assert record.decided_at <= job.events[-1].occurred_at + + +# ---- no agent has total authority ------------------------------------------ + + +def test_an_agent_cannot_approve_its_own_job(planner, clock): + job = _at_the_gate(planner, clock) + before = len(job.events) + with pytest.raises(SelfApproval): + job.approve(authority=planner, reason="my own work looks good to me") + assert job.state is JobState.GOVERNANCE_ANALYSIS + assert job.approval is None + assert len(job.events) == before + + +def test_another_agent_may_approve(planner, clock): + job = _at_the_gate(planner, clock) + job.approve(authority=Principal("agent", "reviewer-bot"), reason="independent review") + assert job.state is JobState.APPROVED + + +def test_an_agent_may_still_reject_its_own_job(planner, clock): + """The invariant is about APPROVE — refusing your own work is not a conflict.""" + job = _at_the_gate(planner, clock) + job.reject(authority=planner, reason="I got the plan wrong") + assert job.state is JobState.REJECTED + + +def test_a_person_approving_their_own_job_is_allowed_today(alice, clock): + """Scope check, not an endorsement. + + ``approval/v1`` and RFC-0002 both state the invariant about *agents*. Making + the engine stricter than the contract it conforms to needs an RFC here first, + so this passes deliberately and is flagged rather than silently tightened. + """ + job = _at_the_gate(alice, clock) + job.approve(authority=alice, reason="my own job, approved by me") + assert job.state is JobState.APPROVED + + +# ---- a decision belongs to what it decides about --------------------------- + + +def _decision(clock, reviewer, **kw) -> Decision: + base = dict( + decision_id=new_decision_id(), + tenant_id="acme", + workspace_id="ws-core", + subject=Subject("job", "job-001"), + decision=DecisionType.APPROVE, + reason="ok", + authority=reviewer, + decided_at=clock(), + ) + base.update(kw) + return Decision(**base) + + +def test_a_decision_from_another_tenant_is_rejected_not_coerced(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + foreign = _decision(clock, reviewer, tenant_id="globex") + with pytest.raises(CrossTenantDecision) as excinfo: + job.transition(JobState.APPROVED, reason="ok", principal=reviewer, decision=foreign) + assert excinfo.value.field == "tenant_id" + assert job.state is JobState.GOVERNANCE_ANALYSIS + assert job.tenant_id == "acme" # not rewritten to match the decision + + +def test_a_decision_from_another_workspace_is_refused(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + foreign = _decision(clock, reviewer, workspace_id="ws-platform") + with pytest.raises(CrossTenantDecision) as excinfo: + job.transition(JobState.APPROVED, reason="ok", principal=reviewer, decision=foreign) + assert excinfo.value.field == "workspace_id" + + +def test_a_decision_about_another_job_cannot_authorise_this_one(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + elsewhere = _decision(clock, reviewer, subject=Subject("job", "job-999")) + with pytest.raises(WrongDecisionSubject): + job.transition(JobState.APPROVED, reason="ok", principal=reviewer, decision=elsewhere) + + +def test_a_decision_that_does_not_produce_this_transition_is_refused(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + rejection = _decision(clock, reviewer, decision=DecisionType.REJECT) + with pytest.raises(DecisionStateMismatch): + job.transition(JobState.APPROVED, reason="ok", principal=reviewer, decision=rejection) + + +def test_a_decision_cannot_ride_along_an_ordinary_transition(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + job.approve(authority=reviewer, reason="ok") + stray = _decision(clock, reviewer) + with pytest.raises(DecisionStateMismatch): + job.transition(JobState.TASK_PLANNING, decision=stray) + + +def test_a_decision_must_be_a_decision(alice, clock): + job = _at_the_gate(alice, clock) + with pytest.raises(TypeError): + job.transition(JobState.APPROVED, reason="ok", principal=alice, decision="APPROVE") + + +# ---- every APPROVE is auditable -------------------------------------------- + + +def test_a_decision_emits_its_own_event_next_to_the_transition(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + before = len(job.events) + record = job.approve(authority=reviewer, reason="ok") + + added = job.events[before:] + assert [e.type_value for e in added] == ["GOVERNANCE_DECISION", "STATE_TRANSITION"] + assert added[0].subject_type == "approval" + assert added[0].subject_id == record.decision_id + assert added[0].job_id == "job-001" # RFC-0008: our events always carry it + assert added[1].transition == {"from": "GOVERNANCE_ANALYSIS", "to": "APPROVED", "reason": "ok"} + + +def test_every_approve_in_a_run_has_a_governance_decision_event(alice, reviewer, clock): + job = drive(_fresh(alice, clock), JobState.COMPLETED, reviewer) + approvals = [d for d in job.decisions if d.decision is DecisionType.APPROVE] + emitted = [ + p for p in job.event_payloads() if p["event_type"] == "GOVERNANCE_DECISION" + ] + assert approvals + assert {d.decision_id for d in approvals} <= {p["subject_id"] for p in emitted} + + +def test_ordinary_transitions_emit_no_decision(alice, reviewer, clock): + job = drive(_fresh(alice, clock), JobState.IN_PROGRESS, reviewer) + decisions = [e for e in job.events if e.type_value == "GOVERNANCE_DECISION"] + assert len(decisions) == 1 # the approval, and nothing else + + +def test_entering_a_decision_state_directly_still_records_a_decision(alice, reviewer, clock): + """The guarantee is about the state, not about which method was called.""" + job = _at_the_gate(alice, clock) + job.transition(JobState.APPROVED, reason="approved out of band", principal=reviewer) + + assert job.approval is not None + assert job.approval.decision is DecisionType.APPROVE + assert job.approval.authority is reviewer + assert job.events[-2].type_value == "GOVERNANCE_DECISION" + + +def test_execution_needs_the_decision_not_just_the_state(alice, clock): + """The direction lock reads the record, so a forged state does not unlock it.""" + job = _at_the_gate(alice, clock) + job._state = JobState.APPROVED # bypass the engine, simulating a bad edit + with pytest.raises(ExecutionBeforeApproval): + job.transition(JobState.TASK_PLANNING) + + +# ---- the approval/v1 wire shape -------------------------------------------- + +REQUIRED = {"approval_id", "tenant_id", "subject", "decision", "reason", "authority", "decided_at"} + + +def test_the_payload_carries_everything_approval_v1_requires(alice, reviewer, clock): + payload = _at_the_gate(alice, clock).approve(authority=reviewer, reason="ok").as_payload() + assert REQUIRED <= set(payload), f"missing {REQUIRED - set(payload)}" + assert payload["decision"] == "APPROVE" + assert payload["subject"] == {"type": "job", "id": "job-001"} + assert payload["authority"]["id"] == "bob" + assert payload["tenant_id"] == "acme" + assert payload["workspace_id"] == "ws-core" + + +def test_the_payload_uses_the_platforms_field_names(alice, reviewer, clock): + """RFC-0005: we own the meaning, agent-platform owns the wire names.""" + record = _at_the_gate(alice, clock).approve(authority=reviewer, reason="ok") + payload = record.as_payload() + assert payload["approval_id"] == record.decision_id + assert "decision_id" not in payload + + +def test_the_event_carries_the_approval_payload(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + record = job.approve(authority=reviewer, reason="ok") + decision_event = [p for p in job.event_payloads() if p["event_type"] == "GOVERNANCE_DECISION"][0] + assert decision_event["metadata"]["approval"] == record.as_payload() + + +def test_unset_keys_are_omitted_rather_than_nulled(alice, reviewer, clock): + """RFC-0008's rule against inventing a value holds for approvals too.""" + payload = _at_the_gate(alice, clock).approve(authority=reviewer, reason="ok").as_payload() + assert "supersedes_decision_id" not in payload + + +def test_the_citation_appears_once_there_is_something_to_cite(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + first = job.reject(authority=reviewer, reason="no") + job.transition(JobState.DRAFT) + job.submit_for_governance() + payload = job.approve(authority=reviewer, reason="yes").as_payload() + assert payload["supersedes_decision_id"] == first.decision_id + + +def test_decision_ids_are_unique_and_well_formed(alice, reviewer, clock): + ids = [new_decision_id() for _ in range(100)] + assert len(set(ids)) == len(ids) + assert all(ID_PATTERN.match(i) for i in ids) + + +def test_identifiers_on_a_decision_are_validated(reviewer, clock): + with pytest.raises(InvalidIdentifier): + _decision(clock, reviewer, tenant_id="ACME") + + +# ---- the subject ----------------------------------------------------------- + + +def test_a_subject_names_a_kind_the_contract_knows(reviewer, clock): + with pytest.raises(ValueError): + Subject("banana", "job-001") # type: ignore[arg-type] + + +def test_a_subject_id_is_an_identity_v1_id(): + with pytest.raises(InvalidIdentifier): + Subject("job", "JOB-001") + + +def test_a_subject_renders_as_type_and_id(): + assert Subject("execution", "exec-1").as_payload() == {"type": "execution", "id": "exec-1"} + + +def test_repr_names_the_decision_and_who_made_it(alice, reviewer, clock): + record = _at_the_gate(alice, clock).approve(authority=reviewer, reason="ok") + text = repr(record) + assert "APPROVE" in text and "job:job-001" in text and "bob" in text diff --git a/packages/core/tests/test_events.py b/packages/core/tests/test_events.py index aa28d6a..dc4fdc7 100644 --- a/packages/core/tests/test_events.py +++ b/packages/core/tests/test_events.py @@ -95,10 +95,16 @@ def test_our_events_always_carry_job_id(alice, clock): def test_subject_is_always_answerable(alice, clock): + """Every event says what it is about — and a decision is about the approval.""" job = drive(_fresh(alice, clock), JobState.COMPLETED, alice) + approvals = {d.decision_id for d in job.decisions} for payload in job.event_payloads(): - assert payload["subject_type"] == "job" - assert payload["subject_id"] == "job-001" + if payload["event_type"] == "GOVERNANCE_DECISION": + assert payload["subject_type"] == "approval" + assert payload["subject_id"] in approvals + else: + assert payload["subject_type"] == "job" + assert payload["subject_id"] == "job-001" def test_tenant_scope_on_every_event(alice, clock): @@ -138,11 +144,12 @@ def test_optional_keys_are_omitted_when_unset(alice, clock): def test_decision_events_name_the_authority(alice, planner, clock): - """Every APPROVE is auditable — the record says who signed it.""" + """Every APPROVE is auditable — both records say who signed it.""" job = _fresh(alice, clock) job.submit_for_governance() - event = job.approve(authority=planner, reason="meets milestone") - assert event.as_payload()["actor"]["id"] == "planner-1" + job.approve(authority=planner, reason="meets milestone") + signed = [p["actor"]["id"] for p in job.event_payloads()[-2:]] + assert signed == ["planner-1", "planner-1"] def test_transition_returns_the_emitted_event(alice, clock): @@ -190,7 +197,7 @@ def test_job_exposes_its_identity_and_settlement(alice, clock): def test_the_seven_canonical_event_types_are_declared(): - """RFC-0003's vocabulary, in one place. The state machine emits three of them.""" + """RFC-0003's vocabulary, in one place. The state machine emits four of them.""" assert {t.value for t in EventType} == { "JOB_CREATED", "STATE_TRANSITION", @@ -202,14 +209,17 @@ def test_the_seven_canonical_event_types_are_declared(): } -def test_the_state_machine_emits_only_its_own_three(alice, clock): +def test_the_state_machine_emits_only_its_own_four(alice, clock): + """The other three belong to orchestration and execution — issue #7 and later.""" job = drive(_fresh(alice, clock), JobState.COMPLETED, alice) emitted = {e.event_type for e in job.events} assert emitted <= { EventType.JOB_CREATED, EventType.STATE_TRANSITION, + EventType.GOVERNANCE_DECISION, EventType.JOB_COMPLETED, } + assert EventType.GOVERNANCE_DECISION in emitted # ---- the wider surface RFC-0008 needs for external events ------------------- diff --git a/platform-contract.yaml b/platform-contract.yaml index f6eabe4..2957911 100644 --- a/platform-contract.yaml +++ b/platform-contract.yaml @@ -25,8 +25,9 @@ conformance: # 1. manifest ไฟล์นี้ # 2. conformance test ใน CI ที่ validate payload จริง # conformance/payload_check.py — รัน scenario ผ่าน job state - # machine และ event log จริง แล้ว validate event 37 ตัว - # กับ event/v1 ที่ pin ไว้ · ไม่มี fixture ที่เขียนขึ้นเพื่อให้ schema ผ่าน + # machine และ event log จริง แล้ว validate event 42 ตัว + # กับ event/v1 ที่ pin ไว้ และ decision 5 ใบกับ approval/v1 + # · ไม่มี fixture ที่เขียนขึ้นเพื่อให้ schema ผ่าน # 3. release gate job `core` และ `conformance` ใน .github/workflows/test.yml # รันทุก PR # @@ -93,7 +94,9 @@ blocking: remaining: - เปิด branch protection แล้วตั้ง check ของ workflow test เป็น required เพื่อให้ release gate บล็อกการ merge ได้จริง (ADR-0006 ข้อ 3) - - governance decision interface (issue #5) · end-to-end simulation (issue #7) + - end-to-end simulation (issue #7) + - RFC กำหนดปลายทางของ REQUIRE_CHANGES — vocabulary มีครบ 3 ค่าแล้ว + แต่ engine ปฏิเสธค่านี้อยู่ (UnmappedDecision) เพราะยังไม่มี RFC บอกว่า job ไปไหนต่อ registration: conforming # registered | conforming @@ -111,3 +114,16 @@ gaps: บันทึกเป็น known_gaps ใน conformance/pinned.yaml แคบไว้ที่ field event_type ของ event ที่ source.kind = external เท่านั้น · หมดอายุ 2026-11-18 severity: low + + - id: approval-supersedes-field-missing + issue: https://github.com/monthop-gmail/agent-platform/issues/22 # เปิดแล้ว 2026-08-19 (พบตอนทำ issue #5) + detail: >- + approval/v1 มี guarantee ว่า "decision เป็น immutable · การเปลี่ยนใจคือ approval + ใบใหม่ที่อ้างใบเดิม" แต่ schema ไม่มี field ให้ใส่การอ้างนั้นเลย — ทำตาม guarantee + แล้วไม่มีที่เก็บผลลัพธ์ + workaround: >- + ฝั่งเราใส่ field ชื่อ supersedes_decision_id ลงใน payload เอง + (Decision.as_payload · packages/core/devfactory_core/decision.py) + approval/v1 ไม่ได้ตั้ง additionalProperties: false จึงยัง validate ผ่าน + ถ้า agent-platform ตั้งชื่อ field นี้เองภายหลัง งานที่เหลือคือ rename ไม่ใช่ออกแบบใหม่ + severity: low