diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 89df50d..b771d7e 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -20,14 +20,18 @@ 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) +and [RFC-0011](rfcs/0011-require-changes-destination.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`. +`APPROVE`, `REJECT` and `REQUIRE_CHANGES` 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` sends a job back to `DRAFT` +([RFC-0011](rfcs/0011-require-changes-destination.md)) and stays distinguishable from a +rejection by its route: a `REJECT` passes through `REJECTED` and leaves it in the job's +history, a `REQUIRE_CHANGES` never enters that state. See +[RFC-0002](rfcs/0002-governance-decision-contract.md) and +`packages/core/devfactory_core/decision.py`. An approval can also carry `expires_at`, and one that has passed it authorises nothing: the engine refuses to move the job into execution, and a job left holding a lapsed diff --git a/conformance/payload_check.py b/conformance/payload_check.py index 8e14bb9..ca5015d 100644 --- a/conformance/payload_check.py +++ b/conformance/payload_check.py @@ -17,8 +17,9 @@ 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. +a record, and a ``REQUIRE_CHANGES`` sends the job back to ``DRAFT`` without its +trail ever passing through ``REJECTED`` — RFC-0011, which is what keeps +*"REQUIRE_CHANGES ไม่ใช่ REJECT"* true of the record and not only of the wording. Usage:: @@ -137,10 +138,10 @@ def build_validator(schemas: dict[str, dict], target_id: str, non_schema_keys: l def run_scenario(): """Drive real jobs through the real engine and return the log plus what happened. - Seven jobs across two tenants, covering every terminal state, the mid-run - approval pause, rejection and resubmission, recovery by supersession, and an - approval that expired where it sat — plus two inbound external events that no - job caused. + Eight jobs across two tenants, covering every terminal state, the mid-run + approval pause, rejection and resubmission, being sent back for changes, + recovery by supersession, and an approval that expired where it sat — plus two + inbound external events that no job caused. The journeys themselves are ``simulation/flows.py``, which issue #7 made the one place they are written down. Two files describing the same lifecycle @@ -158,6 +159,7 @@ def run_scenario(): happy_path, job_factory, rejected_then_resubmitted, + require_changes_then_resubmitted, stalled_awaiting_approval, ) @@ -200,7 +202,15 @@ def run_scenario(): expires_at=datetime.now(timezone.utc) - timedelta(hours=1), ) - jobs = [happy, revised, failed, replacement, cancelled, stalled, expired] + # 7. sent back for changes, revised, resubmitted, approved — RFC-0011. The only + # flow that produces an approval/v1 payload carrying decision=REQUIRE_CHANGES, + # so the third value of a closed vocabulary is validated as something the + # engine really emitted rather than asserted about in the abstract. + sent_back = require_changes_then_resubmitted( + acme, job_id="job-011", authority=reviewer + ) + + jobs = [happy, revised, failed, replacement, cancelled, stalled, expired, sent_back] for job in jobs: log.extend(job.events) @@ -344,10 +354,10 @@ def check_decisions(log, jobs, validator) -> None: ) check_expiry_enforced() - # REQUIRE_CHANGES อยู่ใน vocabulary แต่ยังไม่มี RFC กำหนดว่ามันพา job ไปไหน - # engine ต้องปฏิเสธ ไม่ใช่เดาปลายทาง — ดู states.DECISION_TARGET - from devfactory_core import DecisionType, Job, Principal - from devfactory_core.errors import UnmappedDecision + # REQUIRE_CHANGES มีปลายทางแล้วตาม rfcs/0011 — DRAFT ตรงจากประตู + # เดิมเช็คนี้ยืนยันว่า engine ปฏิเสธค่านี้ ตอนนี้กลับด้านเป็นยืนยันว่ามันทำงานถูก + # ดู states.DECISION_TARGET + from devfactory_core import DecisionType, Job, JobState, Principal probe = Job( job_id="job-007", @@ -356,15 +366,27 @@ def check_decisions(log, jobs, validator) -> None: principal=Principal("human", "alice"), ) probe.submit_for_governance() - try: - probe.decide( - DecisionType.REQUIRE_CHANGES, - authority=Principal("human", "bob"), - reason="needs a test plan", + probe.decide( + DecisionType.REQUIRE_CHANGES, + authority=Principal("human", "bob"), + reason="needs a test plan", + ) + if probe.state is JobState.DRAFT and probe.approval is None: + ok("approval", "REQUIRE_CHANGES พา job กลับไป DRAFT และไม่เหลือ approval ค้างไว้ (rfcs/0011)") + else: + fail( + "approval", + f"REQUIRE_CHANGES พา job ไปจบที่ {probe.state.value} " + f"(approval={probe.approval is not None}) — ต้องเป็น DRAFT และไม่มี approval", ) - fail("approval", "REQUIRE_CHANGES ถูกรับเข้า — ต้องปฏิเสธจนกว่าจะมี RFC กำหนดปลายทาง") - except UnmappedDecision: - ok("approval", "REQUIRE_CHANGES ถูกปฏิเสธ — ยังไม่มี RFC กำหนดปลายทางของมัน") + + # "REQUIRE_CHANGES ไม่ใช่ REJECT" — ปลายทางเดียวกันกับ REJECT แต่คนละเส้นทาง + # เส้นทางอยู่ใน trail จริง ไม่ใช่แค่ใน decision record จึงตรวจที่ history + visited = [h.to_state.value for h in probe.history] + if visited == ["GOVERNANCE_ANALYSIS", "DRAFT"]: + ok("approval", "REQUIRE_CHANGES ข้าม REJECTED — trail แยกออกจากการถูกปฏิเสธ") + else: + fail("approval", f"เส้นทางของ REQUIRE_CHANGES คือ {visited} — คาดว่าต้องข้าม REJECTED") def check_expiry_enforced() -> None: diff --git a/contract-semantics.yaml b/contract-semantics.yaml index 78cb133..79bd5ab 100644 --- a/contract-semantics.yaml +++ b/contract-semantics.yaml @@ -107,6 +107,7 @@ contracts: note: >- packages/core ทำ decision interface ครบตาม RFC-0002 แล้ว (issue #5) vocabulary ยังมีครบ 3 ค่าตามชุดปิด — ห้ามลบค่าใดออกจาก contract + · ตั้งแต่ rfcs/0011 ทั้งสามค่ามีปลายทางแล้ว engine ไม่ปฏิเสธค่าใดอีก enforced_optional_fields: # field ที่ agent-platform เพิ่มเองได้ (platform_may_add_freely ด้านล่าง) # แต่ตัว schema เขียนความหมายไว้แล้ว — repo นี้จึงบังคับตามความหมายนั้น @@ -121,14 +122,28 @@ contracts: optional: >- ยัง optional เหมือนใน schema — approval ที่ไม่มี expires_at คือไม่มีวันหมดอายุ บังคับให้ต้องมีจะทำให้ engine เข้มกว่า contract ที่ conform อยู่ · issue #17 - 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 — สิ่งที่ขาดคือปลายทาง ไม่ใช่ตัวค่า + decision_targets: + # ครบทั้งสามค่าแล้วตั้งแต่ rfcs/0011 — เดิม REQUIRE_CHANGES ไม่มีปลายทาง + # และ engine ปฏิเสธด้วย UnmappedDecision + APPROVE: APPROVED + REJECT: REJECTED + REQUIRE_CHANGES: DRAFT + require_changes: + detail: >- + rfcs/0011 กำหนดให้ REQUIRE_CHANGES พา job กลับไป DRAFT ผ่าน edge ใหม่ + GOVERNANCE_ANALYSIS → DRAFT · ไม่เพิ่ม state และไม่เพิ่มค่าใน vocabulary + · approval ถูกล้างเหมือน REJECT เพราะการถูกสั่งให้แก้ไม่ใช่การได้รับอนุมัติ + how_it_stays_distinct_from_reject: >- + ปลายทางเดียวกันแต่คนละเส้นทาง และเส้นทางอยู่ใน trail จริง: + REJECT เดิน GOVERNANCE_ANALYSIS → REJECTED → DRAFT (state REJECTED ค้างใน + history ตลอดไป) ส่วน REQUIRE_CHANGES เดิน GOVERNANCE_ANALYSIS → DRAFT ตรง ๆ + ข้าม REJECTED · คนที่อ่านย้อนจาก log อย่างเดียวจึงแยกออกได้ ไม่ต้องเชื่อ + decision record อย่างเดียว · invariant "REQUIRE_CHANGES ไม่ใช่ REJECT" + จึงถูกบังคับด้วยรูปร่างของ trail ไม่ใช่ด้วยคำอธิบาย + note_for_consumers: >- + ผลที่ตามมา: "transition นี้เป็นคำตัดสินหรือไม่" ต้องดูที่ edge ไม่ใช่ที่ปลายทาง + — GOVERNANCE_ANALYSIS → DRAFT เป็นคำตัดสิน ส่วน REJECTED → DRAFT ไม่ใช่ + (เป็นแค่ขั้นแก้ไขหลังคำตัดสินที่บันทึกไปแล้ว) · ดู states.DECISION_BY_EDGE platform_may_add_freely: # เพิ่มได้ผ่าน ADR ฝั่ง agent-platform อย่างเดียว ไม่ต้องมี RFC ที่นี่ (Rule 1) @@ -200,6 +215,7 @@ not_derived: - rfcs/0001-job-state-machine.md - rfcs/0007-job-lifecycle-completeness.md # amend 0001 - rfcs/0010-failable-states.md # ระบุว่า state ไหนไป FAILED ได้ + - rfcs/0011-require-changes-destination.md # เพิ่ม edge GOVERNANCE_ANALYSIS → DRAFT note: >- ให้ vocabulary ไว้เพื่อให้ execution/v1 อ้างความหมายของ job state ได้ โดยไม่ต้องคาดเดา — คำเตือน external-authority-pending ใน execution.schema.yaml @@ -224,7 +240,9 @@ not_derived: # ไม่ลง state ที่อยู่ใน terminal เพราะไม่มี edge ออก — ดู terminal ด้านบน progression: DRAFT: [GOVERNANCE_ANALYSIS] - GOVERNANCE_ANALYSIS: [APPROVED, REJECTED] + # rfcs/0011 เพิ่ม DRAFT — ประตูมีสามคำตัดสิน จึงมีทางออกสามทาง ทางละคำตัดสิน + # GOVERNANCE_ANALYSIS → DRAFT คือ REQUIRE_CHANGES · ไม่ผ่าน REJECTED โดยเจตนา + GOVERNANCE_ANALYSIS: [APPROVED, REJECTED, DRAFT] APPROVED: [TASK_PLANNING] REJECTED: [DRAFT] TASK_PLANNING: [IN_PROGRESS] @@ -264,6 +282,8 @@ not_derived: - FAILED · CANCELLED · TIMED_OUT ต้องมี reason metadata - job ที่ยังไม่ผ่าน APPROVED เข้า FAILED ไม่ได้ — recovery ด้วย supersedes_job_id จะไม่มีอะไรให้ supersede (rfcs/0010) - approval ที่เลย expires_at แล้วใช้เดินงานต่อไม่ได้ — engine ปฏิเสธ ไม่ใช่ปล่อยผ่าน · job ที่ค้างอยู่เข้า TIMED_OUT ได้ (rfcs/0007 Amendment 1) + - REQUIRE_CHANGES กับ REJECT จบที่ DRAFT เหมือนกันแต่คนละเส้นทาง — REJECT ผ่าน REJECTED ส่วน REQUIRE_CHANGES ข้าม · แยกกันที่ trail ไม่ใช่แค่ที่ decision record (rfcs/0011) + - คำถามว่า transition นี้เป็นคำตัดสินไหม ต้องอ่านที่ edge ไม่ใช่ที่ปลายทาง — REJECTED → DRAFT ไม่ใช่คำตัดสิน (rfcs/0011) layering: job (ที่นี่) > execution (execution/v1) > step (event/) orchestration_execution_boundary: diff --git a/packages/core/README.md b/packages/core/README.md index 3205689..a6f9c34 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -3,8 +3,9 @@ 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) and decisions from +as amended by [RFC-0007](../../rfcs/0007-job-lifecycle-completeness.md) and +[RFC-0011](../../rfcs/0011-require-changes-destination.md), with the tenant 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. *Approval* is a decision by an @@ -59,11 +60,10 @@ enforced, so the engine rejects rather than repairs. | any post-approval state under an approval past its `expires_at` | `ExpiredApproval` — it has to be granted again | | `FAILED` / `CANCELLED` / `TIMED_OUT` without a reason | `MissingReason` | | `CANCELLED` without a principal | `MissingPrincipal` | -| `APPROVED` / `REJECTED` without an authority and reason | `MissingAuthority` | +| a transition that *is* a decision, without an authority and reason | `MissingAuthority` — including `GOVERNANCE_ANALYSIS → DRAFT`, which is a `REQUIRE_CHANGES` | | 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 | @@ -75,7 +75,7 @@ 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`; -entering `APPROVED` or `REJECTED` also emits `GOVERNANCE_DECISION`; reaching +an edge that *is* a governance decision 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 @@ -98,10 +98,27 @@ job.decide(DecisionType.APPROVE, authority=bob, reason="scope agreed") # == job 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 +job.require_changes(authority=bob, reason="add tests") # → DRAFT, RFC-0011 ``` +All three of RFC-0002's decisions are executable. +[RFC-0011](../../rfcs/0011-require-changes-destination.md) settled the last one: +`REQUIRE_CHANGES` sends a job to `DRAFT` by the `GOVERNANCE_ANALYSIS → DRAFT` edge, +clearing the approval on the way — being told to make changes is not being told to +proceed. + +It stays distinguishable from a `REJECT`, which ends in the same place, by the **route**: +a rejection goes `GOVERNANCE_ANALYSIS → REJECTED → DRAFT` and leaves `REJECTED` standing +in the job's history forever, while a `REQUIRE_CHANGES` goes straight there and never +enters that state. The distinction is in the trail, so a reader who was not present can +make it from the log alone — which is what makes *"REQUIRE_CHANGES ไม่ใช่ REJECT"* a +checked guarantee rather than a stated one. + +One thing to read carefully if you consume the lifecycle: **whether a transition is a +decision is a property of the edge, not the destination.** `GOVERNANCE_ANALYSIS → DRAFT` +is a `REQUIRE_CHANGES`; `REJECTED → DRAFT` is the ordinary revision step and is not a +decision at all. Ask `states.decision_for_edge()`. + ## Tests ```bash diff --git a/packages/core/devfactory_core/decision.py b/packages/core/devfactory_core/decision.py index c5e50ca..d4f83bf 100644 --- a/packages/core/devfactory_core/decision.py +++ b/packages/core/devfactory_core/decision.py @@ -51,9 +51,10 @@ class DecisionType(str, Enum): 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. + three would quietly narrow the contract this repository publishes. All three + are also executable since RFC-0011 gave ``REQUIRE_CHANGES`` a destination — + see ``states.DECISION_TARGET``, which is also where the reason a + ``REQUIRE_CHANGES`` stays distinguishable from a ``REJECT`` is written down. ``str`` mixin so a decision serialises as its own name. """ diff --git a/packages/core/devfactory_core/errors.py b/packages/core/devfactory_core/errors.py index 778a724..ab987f1 100644 --- a/packages/core/devfactory_core/errors.py +++ b/packages/core/devfactory_core/errors.py @@ -119,34 +119,49 @@ def __init__(self, requested: str, *, expired_at: str, now: str) -> None: class MissingAuthority(JobStateMachineError): - """APPROVED and REJECTED are decisions and must name who made them.""" + """A transition that *is* a governance decision must name who made it. - def __init__(self, state: str) -> None: + Which transitions those are is the edge, not the destination state: + ``GOVERNANCE_ANALYSIS -> DRAFT`` is a ``REQUIRE_CHANGES`` and needs an + authority, while ``REJECTED -> DRAFT`` is the revision step after a verdict + already recorded and does not. See ``states.decision_for_edge``. + """ + + def __init__(self, state: str, *, decision: str | None = None) -> None: self.state = state + self.decision = decision + named = f"entering {state} as a {decision}" if decision else state super().__init__( - f"{state} requires an accountable authority and a reason — " - f"an approval nobody signed is not auditable" + f"{named} requires an accountable authority and a reason — " + f"a decision nobody signed is not auditable" ) 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. + """A declared decision type with nowhere in the lifecycle to send a job. + + **Nothing raises this today**, and that is the point of keeping it. Since + RFC-0011 gave ``REQUIRE_CHANGES`` its destination, all three of RFC-0002's + decision types are in ``states.DECISION_TARGET``. The vocabulary is a closed + set, so a fourth value can only arrive through an RFC here — and this is what + that RFC's implementation runs into if it adds the value to + :class:`~devfactory_core.decision.DecisionType` without also declaring the edge + it travels and the entry in ``DECISION_TARGET`` that names it. + + Deleting it would turn that omission into a ``KeyError`` from inside + :meth:`~devfactory_core.job.Job.decide` — an accident rather than a refusal. + Governance code should refuse loudly and say why, and the failure it guards + against is the one governance can least afford: an audit record whose recorded + meaning is not the meaning that was made. """ 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." + f"{decision} is a declared decision type with no entry in " + f"states.DECISION_TARGET, so no RFC in this repository says which state " + f"it moves a job to — the engine will not guess a destination. A new " + f"decision type needs the edge and the destination declared together." ) diff --git a/packages/core/devfactory_core/job.py b/packages/core/devfactory_core/job.py index aeeab1a..37a8866 100644 --- a/packages/core/devfactory_core/job.py +++ b/packages/core/devfactory_core/job.py @@ -1,7 +1,7 @@ """The in-memory job state machine. Canonical spec: ``packages/core/state-machine.md`` — RFC-0001 as amended by -RFC-0007 and RFC-0010, with the tenant model from RFC-0006. +RFC-0007, RFC-0010 and RFC-0011, with the tenant model from RFC-0006. Scope, per issue #2: in memory, no persistence, no policy engine, no API. What this module owns is the lifecycle and the guards on it. @@ -15,6 +15,12 @@ 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. +All three of RFC-0002's decisions are executable since RFC-0011, which settled +where ``REQUIRE_CHANGES`` sends a job: ``DRAFT``, straight from the gate. It +clears the approval exactly as ``REJECT`` does — being told to make changes is not +being told to proceed — and is told apart from a rejection by its *route*, since +it never passes through ``REJECTED``. See ``states.DECISION_TARGET``. + An approval can also say when it stops being one. ``approval/v1`` carries ``expires_at`` and states what it means — *"approval ที่หมดอายุแล้วใช้เดินงานไม่ได้ ต้องขอใหม่"* — so the engine refuses to move a job into execution under a lapsed @@ -49,11 +55,11 @@ from .identity import Principal, validate_id from .states import ( APPROVAL_PAUSABLE, - DECISION_BY_TARGET, DECISION_TARGET, POST_APPROVAL, TERMINAL, JobState, + decision_for_edge, reachable_from, ) @@ -63,11 +69,6 @@ {JobState.FAILED, JobState.CANCELLED, JobState.TIMED_OUT} ) -#: States whose entry is a decision and must name an accountable authority. -#: 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) class TransitionRecord: @@ -259,19 +260,24 @@ def transition( 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) + # Whether this move *is* a governance decision is a property of the edge, + # not of the destination — since RFC-0011, DRAFT is reached both by a + # REQUIRE_CHANGES from the gate and by the ordinary revision step out of + # REJECTED, and only the first is a verdict. + decides = decision_for_edge(self._state, to) + self._check_guards(to, reason=reason, principal=principal, decides=decides) record: Decision | None = None - if to in AUTHORITY_REQUIRED: + if decides is not None: # "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. + # about which method the caller reached for. Entering a decision state + # 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 + decides, authority=principal, reason=reason ) - self._check_decision(record, to) + self._check_decision(record, decides, to) elif decision is not None: raise DecisionStateMismatch(decision.decision.value, to.value) @@ -283,12 +289,16 @@ def transition( elif previous is JobState.AWAITING_APPROVAL: self._awaiting_from = None - if to is JobState.APPROVED: + if decides is DecisionType.APPROVE: 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. + elif decides is not None: + # A verdict that is not an APPROVE leaves the job unauthorised, and it + # goes back to DRAFT for revision — REJECT by way of REJECTED, + # REQUIRE_CHANGES directly. Either way the approval that was never + # granted must not carry over, and an approval granted to an earlier + # revision must not authorise the revised one. RFC-0011 keeps + # REQUIRE_CHANGES on this side of the line deliberately: "งานยังมีชีวิต" + # says the job may come back, not that it may proceed. self._approval = None self._state = to @@ -355,9 +365,9 @@ def decide( 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``. + All three of RFC-0002's decisions are accepted. A decision type that + somehow has no destination is refused with ``UnmappedDecision`` rather + than sent somewhere invented — see that error for when it can arise. ``expires_at`` is ``approval/v1``'s deadline for the decision, and it is the caller's to set: the timeout *policy* — how long an approval is good @@ -409,6 +419,17 @@ def approve( def reject(self, *, authority: Principal, reason: str) -> Decision: return self.decide(DecisionType.REJECT, authority=authority, reason=reason) + def require_changes(self, *, authority: Principal, reason: str) -> Decision: + """Send the job back for changes — RFC-0011. + + The job lands in ``DRAFT`` holding no approval, ready to be revised and + resubmitted. It is not a rejection and its trail does not say it was: the + route skips ``REJECTED`` entirely. + """ + return self.decide( + DecisionType.REQUIRE_CHANGES, authority=authority, reason=reason + ) + def pause_for_approval(self, *, reason: str | None = None) -> Event: return self.transition(JobState.AWAITING_APPROVAL, reason=reason) @@ -473,14 +494,22 @@ def _check_edge(self, to: JobState) -> None: ) def _check_guards( - self, to: JobState, *, reason: str | None, principal: Principal | None + self, + to: JobState, + *, + reason: str | None, + principal: Principal | None, + decides: DecisionType | None, ) -> None: if to in REASON_REQUIRED and not (reason and reason.strip()): raise MissingReason(to.value) if to is JobState.CANCELLED and principal is None: raise MissingPrincipal(to.value) - if to in AUTHORITY_REQUIRED and (principal is None or not (reason and reason.strip())): - raise MissingAuthority(to.value) + # A decision names who made it and why, whichever decision it is. Asking + # the edge rather than a list of destination states means a decision type + # gaining a destination cannot leave that edge ungoverned. + if decides is not None and (principal is None or not (reason and reason.strip())): + raise MissingAuthority(to.value, decision=decides.value) # 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. @@ -500,13 +529,13 @@ def _check_guards( ) def _decision_for( - self, to: JobState, *, authority: Principal | None, reason: str | None + self, decides: DecisionType, *, authority: Principal | None, reason: str | None ) -> Decision: - """Mint the decision that entering ``to`` must have been. + """Mint the decision this edge 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. + Only reachable for an edge ``states.decision_for_edge`` names, 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( @@ -514,7 +543,7 @@ def _decision_for( tenant_id=self._tenant_id, workspace_id=self._workspace_id, subject=Subject("job", self._job_id), - decision=DECISION_BY_TARGET[to], + decision=decides, reason=reason, authority=authority, decided_at=self._clock(), @@ -523,9 +552,18 @@ def _decision_for( ), ) - 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: + def _check_decision( + self, record: Decision, decides: DecisionType, to: JobState + ) -> None: + """Refuse a decision that does not belong to this job or this transition. + + Compared against the decision the *edge* is, not merely against the + destination: since RFC-0011 two decisions can share a destination by way of + different routes, and a REJECT offered for the direct + ``GOVERNANCE_ANALYSIS -> DRAFT`` hop would otherwise record a rejection on + a trail that never entered ``REJECTED``. + """ + if record.decision is not decides: raise DecisionStateMismatch(record.decision.value, to.value) if record.subject != Subject("job", self._job_id): raise WrongDecisionSubject( diff --git a/packages/core/devfactory_core/states.py b/packages/core/devfactory_core/states.py index 06817f6..86d9596 100644 --- a/packages/core/devfactory_core/states.py +++ b/packages/core/devfactory_core/states.py @@ -1,10 +1,10 @@ """Job lifecycle states and the transition table. -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. ``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. +Canonical spec: ``packages/core/state-machine.md`` — RFC-0001 as amended by +RFC-0007 and RFC-0011. This module is the single place the transition table is +expressed in code; nothing 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 @@ -96,7 +96,13 @@ class JobState(str, Enum): # The lifecycle proper, before the cross-cutting exits are folded in. _PROGRESSION: dict[JobState, frozenset[JobState]] = { JobState.DRAFT: frozenset({JobState.GOVERNANCE_ANALYSIS}), - JobState.GOVERNANCE_ANALYSIS: frozenset({JobState.APPROVED, JobState.REJECTED}), + # RFC-0011 added ``DRAFT``: it is where ``REQUIRE_CHANGES`` sends a job. The + # gate has three verdicts and now three ways out, one per verdict. Returning + # here without passing REJECTED is the whole point — the trail has to be able + # to say "sent back for changes" without saying "rejected". + JobState.GOVERNANCE_ANALYSIS: frozenset( + {JobState.APPROVED, JobState.REJECTED, JobState.DRAFT} + ), JobState.APPROVED: frozenset({JobState.TASK_PLANNING}), JobState.REJECTED: frozenset({JobState.DRAFT}), JobState.TASK_PLANNING: frozenset({JobState.IN_PROGRESS}), @@ -135,55 +141,64 @@ def _build() -> dict[JobState, frozenset[JobState]]: TRANSITIONS: dict[JobState, frozenset[JobState]] = _build() +#: The one state a governance decision is made in. RFC-0002: a decision is what +#: moves a job *out of the gate*, so every edge below leaves from here. +DECISION_GATE: JobState = JobState.GOVERNANCE_ANALYSIS + #: Where a governance decision sends a job — RFC-0002 meeting RFC-0001. #: +#: All three of RFC-0002's decisions have a destination. ``REQUIRE_CHANGES`` got +#: its own in RFC-0011: ``DRAFT``, reached straight from the gate. +#: #: 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 +#: ``DECISION_GATE``. 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: +#: ``REJECT`` and ``REQUIRE_CHANGES`` both leave the job revisable in ``DRAFT``, +#: which is what ``approval/v1`` requires of the second — *"REQUIRE_CHANGES ไม่ใช่ +#: REJECT — งานยังมีชีวิตและกลับมายื่นใหม่ได้"*. They stay distinguishable because +#: they take **different routes there**, and the route is in the trail: #: -#: * ``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. +#: * ``REJECT`` — ``GOVERNANCE_ANALYSIS -> REJECTED -> DRAFT``, two transitions, +#: with ``REJECTED`` standing in the job's history forever. +#: * ``REQUIRE_CHANGES`` — ``GOVERNANCE_ANALYSIS -> DRAFT``, one transition, and +#: ``REJECTED`` never appears. #: -#: 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. +#: So a reader with only the ``STATE_TRANSITION`` records can tell the two apart +#: without consulting the decision, and a reader with the decisions can tell them +#: apart without consulting the states. Neither half is load-bearing alone, which +#: is why the invariant survives the shared destination. See RFC-0011. DECISION_TARGET: dict[DecisionType, JobState] = { DecisionType.APPROVE: JobState.APPROVED, DecisionType.REJECT: JobState.REJECTED, + DecisionType.REQUIRE_CHANGES: JobState.DRAFT, } -#: 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() +#: The inverse, keyed by the **edge** rather than by the destination. +#: +#: Entering a decision state 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. +#: +#: It has to be keyed by the edge since RFC-0011, because ``DRAFT`` is now +#: reachable two ways and only one of them is a decision: ``GOVERNANCE_ANALYSIS -> +#: DRAFT`` is a ``REQUIRE_CHANGES``, while ``REJECTED -> DRAFT`` is the revision +#: step that follows a rejection already recorded. Keying this by destination +#: alone would demand a decision record for the second, and would let a replay +#: read a resubmission as a verdict nobody made. +DECISION_BY_EDGE: dict[tuple[JobState, JobState], DecisionType] = { + (DECISION_GATE, target): decision for decision, target in DECISION_TARGET.items() } +def decision_for_edge(from_state: JobState, to_state: JobState) -> DecisionType | None: + """The decision this edge *is*, or ``None`` if the edge is not a decision.""" + return DECISION_BY_EDGE.get((from_state, to_state)) + + 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 85607b9..94daa68 100644 --- a/packages/core/state-machine.md +++ b/packages/core/state-machine.md @@ -1,7 +1,8 @@ # Job State Machine Spec Canonical states per [RFC-0001](../../rfcs/0001-job-state-machine.md) as amended by -[RFC-0007](../../rfcs/0007-job-lifecycle-completeness.md). +[RFC-0007](../../rfcs/0007-job-lifecycle-completeness.md) and +[RFC-0011](../../rfcs/0011-require-changes-destination.md). ## States @@ -26,7 +27,7 @@ Terminal: `COMPLETED` · `FAILED` · `CANCELLED` · `TIMED_OUT` ```text DRAFT → GOVERNANCE_ANALYSIS -GOVERNANCE_ANALYSIS → APPROVED | REJECTED +GOVERNANCE_ANALYSIS → APPROVED | REJECTED | DRAFT APPROVED → TASK_PLANNING REJECTED → DRAFT TASK_PLANNING → IN_PROGRESS @@ -57,19 +58,33 @@ engine records it and emits `GOVERNANCE_DECISION` alongside the `STATE_TRANSITIO 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`** | +| decision | job goes to | route | +| --- | --- | --- | +| `APPROVE` | `APPROVED` | `GOVERNANCE_ANALYSIS → APPROVED` | +| `REJECT` | `REJECTED` | `GOVERNANCE_ANALYSIS → REJECTED → DRAFT` | +| `REQUIRE_CHANGES` | `DRAFT` | `GOVERNANCE_ANALYSIS → DRAFT` | + +### `REQUIRE_CHANGES` is not a `REJECT`, and the trail says so + +Both end in `DRAFT`. They are told apart by the **route**, which is in the audit log as +`STATE_TRANSITION` records: a rejected job stands in `REJECTED` and its history says so +forever, while a job sent back for changes never entered that state. -`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. +That is what keeps `approval/v1`'s invariant — *"REQUIRE_CHANGES ไม่ใช่ REJECT — งานยัง +มีชีวิตและกลับมายื่นใหม่ได้"* — checkable rather than merely stated. A reader holding only +the transitions can tell them apart; so can a reader holding only the decisions; and the +two halves check each other, so a `GOVERNANCE_DECISION` claiming `REJECT` on the direct +`GOVERNANCE_ANALYSIS → DRAFT` hop is refused on replay rather than believed. See +[RFC-0011](../../rfcs/0011-require-changes-destination.md). + +A `REQUIRE_CHANGES` clears the approval in force, exactly as a `REJECT` does — being told +to make changes is not being told to proceed. + +One consequence worth reading twice: **whether a transition is a decision is a property +of the edge, not of the destination.** `GOVERNANCE_ANALYSIS → DRAFT` is a +`REQUIRE_CHANGES` and needs an authority and a reason; `REJECTED → DRAFT` is the revision +step after a verdict already recorded and needs neither. `states.DECISION_BY_EDGE` is what +answers that question, and both the engine and `devfactory_observability.replay` ask it. Guarantees the engine enforces, not just documents: @@ -149,7 +164,10 @@ definitions: 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. +- How many times may a job be sent back for changes? RFC-0011 sets no limit: each round + trip is recorded, so the loop is visible in the trail and boundable by policy, but a + retry counter in the state machine would be a policy value in the lifecycle. + - 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 diff --git a/packages/core/tests/test_decisions.py b/packages/core/tests/test_decisions.py index ed9b3b7..3be1ffc 100644 --- a/packages/core/tests/test_decisions.py +++ b/packages/core/tests/test_decisions.py @@ -27,12 +27,19 @@ ExecutionBeforeApproval, IncompleteDecision, InvalidIdentifier, + InvalidTransition, SelfApproval, UnmappedDecision, WrongDecisionSubject, ) from devfactory_core.identity import ID_PATTERN -from devfactory_core.states import DECISION_BY_TARGET, DECISION_TARGET, TRANSITIONS +from devfactory_core.states import ( + DECISION_BY_EDGE, + DECISION_GATE, + DECISION_TARGET, + TRANSITIONS, + decision_for_edge, +) @pytest.fixture @@ -65,32 +72,116 @@ def test_all_three_decision_types_are_declared(): 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] + declared = TRANSITIONS[DECISION_GATE] assert set(DECISION_TARGET.values()) <= set(declared) - assert DECISION_BY_TARGET == {t: d for d, t in DECISION_TARGET.items()} + assert DECISION_BY_EDGE == { + (DECISION_GATE, t): d for d, t in DECISION_TARGET.items() + } + +def test_every_declared_decision_has_a_destination(): + """RFC-0011 closed the last gap — the vocabulary and the table now agree.""" + assert set(DECISION_TARGET) == set(DecisionType) -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_sends_a_job_back_to_draft(alice, reviewer, clock): + """RFC-0011. Not a rejection, and not a fourteenth state — back to DRAFT.""" + assert DECISION_TARGET[DecisionType.REQUIRE_CHANGES] is JobState.DRAFT -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 + record = job.decide( + DecisionType.REQUIRE_CHANGES, authority=reviewer, reason="add a test plan" + ) + assert job.state is JobState.DRAFT + assert record.decision is DecisionType.REQUIRE_CHANGES + assert job.decisions == (record,) + assert job.history[-1].decision_id == record.decision_id + assert (job.history[-1].from_state, job.history[-1].to_state) == ( + JobState.GOVERNANCE_ANALYSIS, + JobState.DRAFT, + ) + + +def test_require_changes_works_by_its_string_form_too(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + job.decide("REQUIRE_CHANGES", authority=reviewer, reason="needs tests") + assert job.state is JobState.DRAFT + + +def test_require_changes_has_a_named_method_like_the_other_two(alice, reviewer, clock): + job = _at_the_gate(alice, clock) + record = job.require_changes(authority=reviewer, reason="add a test plan") + assert record.decision is DecisionType.REQUIRE_CHANGES + assert job.state is JobState.DRAFT + + +def test_require_changes_leaves_the_job_alive_and_resubmittable(alice, reviewer, clock): + """``approval/v1``: "งานยังมีชีวิตและกลับมายื่นใหม่ได้".""" + job = _at_the_gate(alice, clock) + job.require_changes(authority=reviewer, reason="add a test plan") + job.submit_for_governance(reason="test plan added") + job.approve(authority=reviewer, reason="the changes asked for are in") + assert job.state is JobState.APPROVED + assert job.decisions[-1].supersedes_decision_id == job.decisions[0].decision_id + + +def test_require_changes_clears_the_approval_like_a_reject_does(alice, reviewer, clock): + """Being told to make changes is not being told to proceed — RFC-0011.""" + job = _at_the_gate(alice, clock) + job.approve(authority=reviewer, reason="first pass") + job.transition(JobState.TASK_PLANNING) + job.fail(reason="the approved plan does not work") + + revised = job.supersede(job_id="job-001b") + revised.submit_for_governance(reason="revised plan") + revised.require_changes(authority=reviewer, reason="still needs a test plan") + assert revised.approval is None + revised.submit_for_governance(reason="test plan added") + with pytest.raises(InvalidTransition): + revised.transition(JobState.TASK_PLANNING) -def test_require_changes_is_refused_by_its_string_form_too(alice, reviewer, clock): +def test_require_changes_and_reject_end_in_the_same_place_by_different_routes( + alice, reviewer, clock +): + """RFC-0011's load-bearing claim: the destination is shared, the trail is not. + + This is the whole reason ``REQUIRE_CHANGES -> DRAFT`` does not collapse into + ``REJECT``. Read the two histories back and one of them stood in ``REJECTED``. + """ + sent_back = _at_the_gate(alice, clock) + sent_back.require_changes(authority=reviewer, reason="add a test plan") + + rejected = _at_the_gate(alice, clock, job_id="job-002") + rejected.reject(authority=reviewer, reason="out of scope") + rejected.transition(JobState.DRAFT) + + assert sent_back.state is rejected.state is JobState.DRAFT + assert [h.to_state for h in sent_back.history] == [ + JobState.GOVERNANCE_ANALYSIS, + JobState.DRAFT, + ] + assert [h.to_state for h in rejected.history] == [ + JobState.GOVERNANCE_ANALYSIS, + JobState.REJECTED, + JobState.DRAFT, + ] + assert JobState.REJECTED not in [h.to_state for h in sent_back.history] + + +def test_resubmitting_after_a_rejection_is_not_itself_a_decision(alice, reviewer, clock): + """``REJECTED -> DRAFT`` shares its destination with a decision and is not one. + + If it were read as one it would need an authority, and a replay would demand a + ``GOVERNANCE_DECISION`` for a verdict nobody made. + """ job = _at_the_gate(alice, clock) - with pytest.raises(UnmappedDecision): - job.decide("REQUIRE_CHANGES", authority=reviewer, reason="needs tests") + job.reject(authority=reviewer, reason="out of scope") + before = len(job.decisions) + job.transition(JobState.DRAFT) # no authority, no reason, and that is correct + assert decision_for_edge(JobState.REJECTED, JobState.DRAFT) is None + assert len(job.decisions) == before + assert job.history[-1].decision_id is None def test_a_decision_outside_the_vocabulary_is_not_invented(alice, reviewer, clock): @@ -99,6 +190,28 @@ def test_a_decision_outside_the_vocabulary_is_not_invented(alice, reviewer, cloc job.decide("AUTO_APPROVE", authority=reviewer, reason="looks fine") +def test_a_decision_type_with_no_destination_is_refused_not_guessed( + alice, reviewer, clock, monkeypatch +): + """``UnmappedDecision`` has no caller today, and still has a job. + + A future RFC adding a fourth decision type has to declare its edge and its + destination together. If it declares only the value, this is what it meets — + a refusal that says so, rather than a ``KeyError`` from inside the engine. + """ + from devfactory_core import job as job_module + + job = _at_the_gate(alice, clock) + monkeypatch.setattr(job_module, "DECISION_TARGET", {}) + before = len(job.events) + with pytest.raises(UnmappedDecision) as excinfo: + job.approve(authority=reviewer, reason="fine") + assert "DECISION_TARGET" in str(excinfo.value) + assert job.state is JobState.GOVERNANCE_ANALYSIS + assert job.decisions == () + assert len(job.events) == before + + # ---- the decision record --------------------------------------------------- diff --git a/packages/core/tests/test_guards.py b/packages/core/tests/test_guards.py index 0d30ecb..7b2fe1d 100644 --- a/packages/core/tests/test_guards.py +++ b/packages/core/tests/test_guards.py @@ -106,8 +106,13 @@ def test_cancel_records_who_cancelled(alice, clock): assert job.history[-1].principal is alice -@pytest.mark.parametrize("target", [JobState.APPROVED, JobState.REJECTED], ids=lambda s: s.value) +@pytest.mark.parametrize( + "target", + [JobState.APPROVED, JobState.REJECTED, JobState.DRAFT], + ids=lambda s: s.value, +) def test_decisions_require_an_authority_and_a_reason(target, alice, clock): + """All three verdicts, including RFC-0011's ``GOVERNANCE_ANALYSIS -> DRAFT``.""" job = drive(_fresh(alice, clock), JobState.GOVERNANCE_ANALYSIS, alice) with pytest.raises(MissingAuthority): job.transition(target, reason="because") @@ -116,6 +121,30 @@ def test_decisions_require_an_authority_and_a_reason(target, alice, clock): assert job.state is JobState.GOVERNANCE_ANALYSIS +def test_the_revision_step_after_a_rejection_needs_no_authority(alice, clock): + """Same destination as a ``REQUIRE_CHANGES``, and not a decision. + + The guard keys on the edge for exactly this reason: requiring an authority here + would demand a second verdict for a rejection already recorded. + """ + job = drive(_fresh(alice, clock), JobState.REJECTED, alice) + job.transition(JobState.DRAFT) + assert job.state is JobState.DRAFT + assert job.history[-1].decision_id is None + + +def test_entering_draft_from_the_gate_records_the_decision_it_must_have_been( + alice, clock +): + """The generic call mints the same record ``require_changes()`` would.""" + job = drive(_fresh(alice, clock), JobState.GOVERNANCE_ANALYSIS, alice) + job.transition(JobState.DRAFT, reason="add a test plan", principal=alice) + assert job.state is JobState.DRAFT + assert job.decisions[-1].decision.value == "REQUIRE_CHANGES" + assert job.events[-2].type_value == "GOVERNANCE_DECISION" + assert job.approval is None + + # ---- the direction lock ---------------------------------------------------- diff --git a/packages/core/tests/test_states.py b/packages/core/tests/test_states.py index 40700f8..d030c5f 100644 --- a/packages/core/tests/test_states.py +++ b/packages/core/tests/test_states.py @@ -6,12 +6,15 @@ from devfactory_core.states import ( APPROVAL_PAUSABLE, + DECISION_GATE, + DECISION_TARGET, FAILABLE, POST_APPROVAL, TERMINAL, TIMEOUTABLE, TRANSITIONS, JobState, + decision_for_edge, is_terminal, static_targets, ) @@ -103,6 +106,44 @@ def test_failed_not_reachable_before_execution(state): assert JobState.FAILED not in static_targets(state) +def test_the_gate_has_one_way_out_per_verdict(): + """RFC-0011 added the third. Three decisions, three edges, no fourteenth state.""" + assert set(DECISION_TARGET.values()) == { + JobState.APPROVED, + JobState.REJECTED, + JobState.DRAFT, + } <= set(static_targets(DECISION_GATE)) + assert len(JobState) == 13 + + +def test_the_gate_can_send_a_job_back_for_changes(): + """``REQUIRE_CHANGES`` needs an edge, and this is it — RFC-0011.""" + assert JobState.DRAFT in static_targets(JobState.GOVERNANCE_ANALYSIS) + assert ( + decision_for_edge(JobState.GOVERNANCE_ANALYSIS, JobState.DRAFT) is not None + ) + + +def test_the_two_ways_into_draft_are_not_the_same_kind_of_move(): + """One is a verdict, one is the revision step that follows one. + + Sharing a destination is what makes RFC-0011 workable and is also its only + hazard: anything reading the lifecycle has to key on the edge, or it will read + a resubmission as a decision nobody made. + """ + entrances = { + source for source, targets in TRANSITIONS.items() if JobState.DRAFT in targets + } + assert entrances == {JobState.GOVERNANCE_ANALYSIS, JobState.REJECTED} + assert decision_for_edge(JobState.REJECTED, JobState.DRAFT) is None + + +def test_the_new_edge_does_not_open_a_way_into_execution(): + """RFC-0011 must not weaken the direction lock — DRAFT is the far side of it.""" + assert JobState.DRAFT not in POST_APPROVAL + assert static_targets(JobState.DRAFT) & POST_APPROVAL == frozenset() + + def test_approved_is_the_only_gate_into_execution(): """Direction lock: every path into POST_APPROVAL passes through APPROVED.""" entrances = { diff --git a/packages/core/tests/test_transitions.py b/packages/core/tests/test_transitions.py index 680e3fb..96eb109 100644 --- a/packages/core/tests/test_transitions.py +++ b/packages/core/tests/test_transitions.py @@ -7,7 +7,7 @@ from conftest import drive from devfactory_core import Job, JobState, Principal from devfactory_core.errors import InvalidTransition, JobStateMachineError, TerminalState -from devfactory_core.states import TERMINAL, TRANSITIONS +from devfactory_core.states import TERMINAL, TRANSITIONS, decision_for_edge def _fresh(alice: Principal, clock, name: str = "job-001") -> Job: @@ -16,8 +16,15 @@ def _fresh(alice: Principal, clock, name: str = "job-001") -> Job: ) -def _args_for(target: JobState, authority: Principal) -> dict: - """The guard-required arguments for entering ``target``.""" +def _args_for(target: JobState, authority: Principal, source: JobState | None = None) -> dict: + """The guard-required arguments for entering ``target`` from ``source``. + + ``source`` matters since RFC-0011: ``GOVERNANCE_ANALYSIS -> DRAFT`` is a + ``REQUIRE_CHANGES`` and needs an authority and a reason, while + ``REJECTED -> DRAFT`` is the ordinary revision step and needs neither. + """ + if source is not None and decision_for_edge(source, target) is not None: + return {"reason": "decided", "principal": authority} if target in (JobState.APPROVED, JobState.REJECTED): return {"reason": "decided", "principal": authority} if target is JobState.CANCELLED: @@ -45,7 +52,7 @@ def test_drive_reaches_every_state(state, alice, clock): def test_every_static_edge_is_walkable(source, target, alice, clock): job = drive(_fresh(alice, clock), source, alice) assert job.state is source - job.transition(target, **_args_for(target, alice)) + job.transition(target, **_args_for(target, alice, source)) assert job.state is target diff --git a/packages/observability/devfactory_observability/replay.py b/packages/observability/devfactory_observability/replay.py index 48a73eb..d9cde3e 100644 --- a/packages/observability/devfactory_observability/replay.py +++ b/packages/observability/devfactory_observability/replay.py @@ -46,7 +46,7 @@ from devfactory_core.decision import DecisionType from devfactory_core.events import INTERNAL_SOURCE, Event, EventType from devfactory_core.states import ( - DECISION_BY_TARGET, + DECISION_BY_EDGE, POST_APPROVAL, TERMINAL, JobState, @@ -222,14 +222,20 @@ def replay_job(events: Iterable[Event]) -> ReplayedJob: job_id, from_state.value, to_state.value, event.event_id ) - decision_id = _settle_decision(job_id, to_state, pending, event.event_id) + # Whether this edge is a decision, read off ``states.py`` by edge rather + # than by destination — RFC-0011 gave DRAFT two ways in and only the one + # from the gate is a verdict. + decided = DECISION_BY_EDGE.get((state, to_state)) + decision_id = _settle_decision(job_id, decided, to_state, pending, event.event_id) settled = pending if decision_id is not None else None if decision_id is not None: pending = None - if to_state is JobState.APPROVED: + if decided is DecisionType.APPROVE: approval_decision_id = decision_id approval_expires_at = _expiry_of(settled) - elif to_state is JobState.REJECTED: + elif decided is not None: + # A REJECT or a REQUIRE_CHANGES: read back, neither leaves an approval + # in force, exactly as neither does in the engine. approval_decision_id = None approval_expires_at = None @@ -289,16 +295,22 @@ def replay_job(events: Iterable[Event]) -> ReplayedJob: def _settle_decision( - job_id: str, to_state: JobState, pending: dict[str, Any] | None, event_id: str + job_id: str, + expected: DecisionType | None, + to_state: JobState, + pending: dict[str, Any] | None, + event_id: str, ) -> str | None: """The decision id behind this transition, or None if it needed no decision. - Which states are decisions is ``DECISION_BY_TARGET``, which lives in + Which *edges* are decisions is ``DECISION_BY_EDGE``, which lives in ``devfactory_core.states`` beside the table it has to agree with. Asking it rather than listing the decision states again is what stops this module from - acquiring an opinion of its own about what counts as a decision. + acquiring an opinion of its own about what counts as a decision — and since + RFC-0011 it is what keeps a resubmission out of ``REJECTED`` from being read + back as a verdict, ``REJECTED -> DRAFT`` and ``GOVERNANCE_ANALYSIS -> DRAFT`` + having the same destination and different meanings. """ - expected = DECISION_BY_TARGET.get(to_state) if expected is None: return None if pending is None: diff --git a/packages/observability/tests/test_replay.py b/packages/observability/tests/test_replay.py index e079149..d668b24 100644 --- a/packages/observability/tests/test_replay.py +++ b/packages/observability/tests/test_replay.py @@ -250,12 +250,49 @@ def test_execution_with_no_approve_behind_it_is_refused(make_job, reviewer, monk job = approved(make_job(), reviewer) job.transition(JobState.TASK_PLANNING) - monkeypatch.setattr(replay_module, "DECISION_BY_TARGET", {}) + monkeypatch.setattr(replay_module, "DECISION_BY_EDGE", {}) with pytest.raises(UnauditedExecution) as excinfo: replay_job(job.events) assert excinfo.value.state == "TASK_PLANNING" +def test_a_require_changes_replays_as_a_decision_that_cleared_the_approval( + make_job, reviewer +): + """RFC-0011, read back: the verdict is in the trail and so is its effect.""" + job = approved(make_job(), reviewer) + job.transition(JobState.TASK_PLANNING) + job.fail(reason="the approved plan does not work") + revised = job.supersede(job_id="job-001b") + revised.submit_for_governance(reason="revised plan") + revised.require_changes(authority=reviewer, reason="still needs a test plan") + + seen = replay_job(revised.events) + assert seen.state is JobState.DRAFT + assert seen.decision_ids == (revised.decisions[0].decision_id,) + assert seen.history[-1].decision_id == revised.decisions[0].decision_id + assert seen.approval_decision_id is None + + +def test_the_revision_step_out_of_rejected_is_not_read_as_a_decision( + make_job, reviewer +): + """``REJECTED -> DRAFT`` and ``GOVERNANCE_ANALYSIS -> DRAFT`` share a + destination and are different moves. Reading by destination would demand a + ``GOVERNANCE_DECISION`` here for a verdict nobody made, and refuse a trail that + is entirely correct. + """ + job = make_job() + job.submit_for_governance(reason="ready") + job.reject(authority=reviewer, reason="out of scope") + job.transition(JobState.DRAFT) + + seen = replay_job(job.events) + assert seen.state is JobState.DRAFT + assert len(seen.decision_ids) == 1 + assert seen.history[-1].decision_id is None + + def test_a_completion_the_transitions_do_not_reach_is_refused(make_job): """The one truncation replay can notice — see ``IncompleteSettlement``.""" job = make_job() diff --git a/platform-contract.yaml b/platform-contract.yaml index 2957911..683dc15 100644 --- a/platform-contract.yaml +++ b/platform-contract.yaml @@ -95,8 +95,6 @@ remaining: - เปิด branch protection แล้วตั้ง check ของ workflow test เป็น required เพื่อให้ release gate บล็อกการ merge ได้จริง (ADR-0006 ข้อ 3) - end-to-end simulation (issue #7) - - RFC กำหนดปลายทางของ REQUIRE_CHANGES — vocabulary มีครบ 3 ค่าแล้ว - แต่ engine ปฏิเสธค่านี้อยู่ (UnmappedDecision) เพราะยังไม่มี RFC บอกว่า job ไปไหนต่อ registration: conforming # registered | conforming diff --git a/rfcs/0001-job-state-machine.md b/rfcs/0001-job-state-machine.md index c46716b..9b662a8 100644 --- a/rfcs/0001-job-state-machine.md +++ b/rfcs/0001-job-state-machine.md @@ -10,6 +10,10 @@ Read the two together; RFC-0007 supersedes this document where they differ. **Further amended by [RFC-0010](0010-failable-states.md)** — enumerates which states may reach `FAILED`, which neither this document nor RFC-0007 stated. +**Further amended by [RFC-0011](0011-require-changes-destination.md)** — adds the +`GOVERNANCE_ANALYSIS → DRAFT` edge, which is where RFC-0002's `REQUIRE_CHANGES` sends a +job. No new state; the gate now has one exit per verdict. + ## Context devfactory-core is a governance-first control plane. A deterministic job lifecycle is required to ensure governance, diff --git a/rfcs/0002-governance-decision-contract.md b/rfcs/0002-governance-decision-contract.md index 0bcfb7f..4f5a320 100644 --- a/rfcs/0002-governance-decision-contract.md +++ b/rfcs/0002-governance-decision-contract.md @@ -12,6 +12,11 @@ RFC. Changing what a decision *means* still requires an RFC here first. Tenant scope on decisions is specified in [RFC-0006](0006-tenant-workspace-model.md). +**Completed by [RFC-0011](0011-require-changes-destination.md)** — this RFC names three +decision types and does not say where each sends a job. RFC-0011 settles the last one: +`REQUIRE_CHANGES` returns a job to `DRAFT`, by a route that never passes through +`REJECTED`. The vocabulary is unchanged. + ## Context Governance is the control plane authority in devfactory-core. All execution must be gated by explicit governance decisions. diff --git a/rfcs/0011-require-changes-destination.md b/rfcs/0011-require-changes-destination.md new file mode 100644 index 0000000..2476afa --- /dev/null +++ b/rfcs/0011-require-changes-destination.md @@ -0,0 +1,211 @@ +# RFC-0011: Where `REQUIRE_CHANGES` Sends a Job + +## Status +Draft — proposed 2026-08-19 · pending maintainer approval per `GOVERNANCE.md` + +Amends [RFC-0001](0001-job-state-machine.md) as already amended by +[RFC-0007](0007-job-lifecycle-completeness.md) and [RFC-0010](0010-failable-states.md). +Completes [RFC-0002](0002-governance-decision-contract.md)'s decision vocabulary. + +## Context + +RFC-0002 declares three governance decisions — `APPROVE`, `REJECT`, `REQUIRE_CHANGES` — +and `contract-semantics.yaml` marks the set **closed**, so all three have to exist in +`DecisionType` or the contract this repository publishes would be narrower than the one +it wrote. + +Two of the three have somewhere to send a job. The third does not. `states.py` has +carried a paragraph since PR #13 explaining that `REQUIRE_CHANGES` is deliberately +unmapped and that `Job.decide` raises `UnmappedDecision` for it; `platform-contract.yaml` +lists the missing RFC under `remaining`; `conformance/payload_check.py` has a check whose +job is to confirm the refusal still happens. + +`approval/v1` already states what the decision *means*, as a frozen invariant: + +> REQUIRE_CHANGES ไม่ใช่ REJECT — งานยังมีชีวิตและกลับมายื่นใหม่ได้ + +What it does not say — and what no RFC here said — is **which state the job lands in**, +which is the one thing the engine needs. Refusing was the right call while that was +open: an audit record whose recorded meaning is not the meaning that was made is the one +failure governance cannot absorb. But the refusal is a hole in a closed vocabulary, and +it has been open since issue #5. + +## Problem Statement + +Which state does a job enter when governance decides `REQUIRE_CHANGES`? + +## Decision — `REQUIRE_CHANGES` sends a job to `DRAFT`, by a new +`GOVERNANCE_ANALYSIS → DRAFT` edge + +```text +GOVERNANCE_ANALYSIS → APPROVED | REJECTED | DRAFT +``` + +`DECISION_TARGET` gains `REQUIRE_CHANGES → DRAFT`. The job lands in `DRAFT` holding no +approval, ready to be revised and resubmitted through the gate. + +**Rationale.** + +1. **It is the closest thing to the frozen guarantee.** *"งานยังมีชีวิตและกลับมายื่นใหม่ + ได้"* describes `DRAFT` exactly: it is where a job that has not been decided about sits, + and the only state from which `GOVERNANCE_ANALYSIS` can be entered. Nothing has to be + invented for the job to be alive there. + +2. **It adds no vocabulary.** No new state, no new decision type, no new event type. + That keeps the change inside this repository: a fourteenth state (`CHANGES_REQUESTED`) + would be a `job_state_machine` vocabulary change, which under + [RFC-0009](0009-vocabulary-extension.md) means `agent-platform` needs an ADR of its own + before `execution/v1` can reference the state. A transition between two states both + repositories already know about needs nothing from them. + +3. **`REJECTED → DRAFT` was considered and is ruled out.** Routing `REQUIRE_CHANGES` + through `REJECTED` would record it as a rejection, which contradicts the frozen + invariant directly. It is not a near-miss; it is the thing the invariant forbids. + +## The part that carries this RFC: the two are told apart by **route**, not by record + +`REJECT` and `REQUIRE_CHANGES` now end in the same state. That is the obvious objection +to this decision, and it deserves a direct answer rather than an appeal to the decision +record. + +They are distinguishable because they take **different paths**, and the path is in the +audit trail as `STATE_TRANSITION` events: + +| decision | route | transitions | `REJECTED` in history | +| --- | --- | --- | --- | +| `REJECT` | `GOVERNANCE_ANALYSIS → REJECTED → DRAFT` | two | **yes, permanently** | +| `REQUIRE_CHANGES` | `GOVERNANCE_ANALYSIS → DRAFT` | one | no | + +`REJECTED` is a state a rejected job *stands in*. The trail records entering it, and the +history is append-only, so a job that was rejected says so forever — even after it moves +on to `DRAFT` and is approved on a later pass. A job sent back for changes never entered +that state and its trail never claims it did. + +This matters because it makes the distinction **checkable by a reader who was not +there**, from the log alone: + +- A reader holding only the transitions can tell them apart: one trail passes through + `REJECTED` and one does not. +- A reader holding only the decisions can tell them apart: one says `REJECT` and one says + `REQUIRE_CHANGES`. +- The two halves check each other. A forged `GOVERNANCE_DECISION` claiming `REJECT` on the + direct `GOVERNANCE_ANALYSIS → DRAFT` hop does not match the edge it sits on, and + `devfactory_observability.replay` refuses the trail (`UnauditedDecision`) rather than + believing the record. + +So the guarantee *"REQUIRE_CHANGES ไม่ใช่ REJECT"* is not preserved by convention or by a +field that a reader has to trust. It is preserved by the shape of the trail, which the +engine cannot produce any other way and a replay re-derives independently. That is the +standard the rest of this lifecycle is held to, and this decision meets it. + +`simulation/tests/test_e2e_flow.py` drives both flows and asserts the replayed routes +differ; `simulation/e2e_flow.py` check `[2b]` does the same in the runnable script. + +## Consequence — a `REQUIRE_CHANGES` clears the approval + +Being told to make changes is not being told to proceed. `REQUIRE_CHANGES` therefore +clears `Job.approval` exactly as `REJECT` does, and `replay` clears it on the reading side +for the same reason. An approval granted to an earlier revision must not authorise the +revised one; that rule was already in force for rejections and is not weakened here. + +The direction lock is untouched: a job in `DRAFT` cannot reach any post-approval state +without passing `APPROVED` again, and `POST_APPROVAL` does not gain a member. + +## Consequence — "is this transition a decision?" is a property of the edge + +`DRAFT` is now reachable two ways and only one of them is a verdict: + +- `GOVERNANCE_ANALYSIS → DRAFT` — a `REQUIRE_CHANGES`. Requires an authority and a + reason, mints a `Decision`, emits `GOVERNANCE_DECISION`. +- `REJECTED → DRAFT` — the revision step that follows a rejection **already recorded**. + Requires neither, and mints nothing. + +Anything that answered that question by looking at the destination state — the engine's +`AUTHORITY_REQUIRED`, replay's `DECISION_BY_TARGET` — now has to look at the edge. +`states.DECISION_BY_EDGE` and `states.decision_for_edge()` replace `DECISION_BY_TARGET`, +and both the engine and the replay ask them. This is a real cost of sharing a +destination, and it is recorded here rather than discovered later: reading by destination +would demand a second verdict for a rejection already made, and would let a replay report +a resubmission as a decision nobody took. + +## Non-Goals + +- **Who may issue a `REQUIRE_CHANGES`.** Same authority rules as any other decision; + RFC-0002 owns them and nothing here narrows or widens them. Note that the agent + self-approval invariant is about `APPROVE` only, so — as with `REJECT` — an agent may + ask for changes to its own job. +- **How many times a job may be sent back.** No limit is introduced. A loop between + `DRAFT` and `GOVERNANCE_ANALYSIS` is a policy question, not a lifecycle one; see Risk. +- **What "the changes" are.** The reason string carries them. Structured change requests + would be a payload change and belong to `agent-platform` under RFC-0005 Rule 1. +- **Timeout policy values**, exactly as RFC-0007 and RFC-0010 left them. + +## Architectural Impact + +- **Control Plane** — one edge added to `_PROGRESSION`, one entry to `DECISION_TARGET`, + and the decision-or-not question re-keyed from destination to edge. `Job.require_changes()` + joins `approve()` and `reject()`. `UnmappedDecision` keeps its place as the refusal a + future fourth decision type meets if it declares a value without a destination. +- **Orchestration** — a job may now return to `DRAFT` without having been rejected. + Anything that treated "reached `DRAFT` again" as implying a rejection was already wrong + about `REJECTED → DRAFT` and is now wrong twice; the edge is the thing to read. +- **Execution** — no change. Nothing executes on either side of this edge. +- **Observability** — `GOVERNANCE_DECISION` payloads now really carry + `decision: REQUIRE_CHANGES`, which `conformance/payload_check.py` validates against + `approval/v1` as an emitted payload rather than as an assertion. Replay gains the + ability to distinguish the two routes, which is what makes the guarantee auditable. +- **`agent-platform`** — nothing required. No vocabulary changed, so no ADR and no + re-pin; see the version note below. + +## Risk Assessment + +| risk | severity | mitigation | +| --- | --- | --- | +| A reader treats `REQUIRE_CHANGES` and `REJECT` as the same outcome because the destination is the same | **high** | The routes differ and the difference is in the trail; asserted in `simulation/tests/test_e2e_flow.py` on replayed histories, not just on live jobs. This is the decision's central claim and is tested as one | +| Code elsewhere keys "is this a decision?" on the destination state and mis-reads `REJECTED → DRAFT` | medium | `DECISION_BY_TARGET` is **removed**, not left alongside the new map, so a stale reader fails to import rather than reading the wrong answer. `states.py` remains the only declaration | +| A job ping-pongs between `DRAFT` and `GOVERNANCE_ANALYSIS` forever | medium | Out of scope by design (Non-Goals): each round trip is fully recorded, so the loop is visible in the trail and boundable by policy. Giving the lifecycle a retry counter would put a policy value in the state machine, which RFC-0007 and RFC-0010 both refuse | +| The new edge weakens "execution is forbidden before `APPROVED`" | low | `DRAFT` is on the far side of the gate and `POST_APPROVAL` is unchanged. Asserted directly in `test_states.py::test_the_new_edge_does_not_open_a_way_into_execution` and structurally by `test_approved_is_the_only_gate_into_execution` | +| The forward-path derivation in `simulation/flows.py` picks the wrong branch now that the gate has three exits | low | `DRAFT` joins `_NOT_FORWARD` — a return to the start is not progress. `main_line()` raises rather than choosing if the path ever genuinely forks, and `MAIN_LINE` is still asserted equal to issue #7's flow | +| A `REQUIRE_CHANGES` is used to smuggle a job past governance | low | It clears the approval, so it strictly reduces authority. There is no path from it into execution that does not pass `APPROVED` | + +## `semantics_version` — no bump + +`contract-semantics.yaml` gains the new edge under +`not_derived.job_state_machine.progression`, and its `contracts.approval.implementation_status` +block stops saying `REQUIRE_CHANGES` is unmapped. + +Neither is inside a `frozen:` block. `decision_types` is untouched — the same three values, +still closed. `guarantees` and `invariants` under `contracts.approval` are untouched, and +the invariant this RFC is about (*"REQUIRE_CHANGES ไม่ใช่ REJECT"*) is now **enforced** +rather than merely stated, which is not a change to it. + +So `semantics_version` stays at `1.1`, on the same grounds RFC-0010 used: the `frozen` +scope is untouched, and bumping would turn `agent-platform`'s drift check red for a block +it does not derive from. RFC-0010's Open Question — whether `not_derived` deserves a +signal of its own — remains open and this RFC does not settle it. + +## Migration Plan + +1. Accept this RFC. +2. `states.py` declares the edge and maps the decision; `DECISION_BY_TARGET` becomes + `DECISION_BY_EDGE`. **Included in this change.** +3. `job.py` accepts `REQUIRE_CHANGES`, clears the approval, and gains + `require_changes()`. **Included in this change.** +4. `replay.py` reads decisions by edge. **Included in this change.** +5. `contract-semantics.yaml` publishes the edge; `platform-contract.yaml` drops the item + from `remaining`. **Included in this change.** +6. `conformance/payload_check.py`'s check inverts — from confirming the refusal to + confirming the destination and the route. **Included in this change.** +7. `simulation/flows.py` gains `require_changes_then_resubmitted`, and the simulation + proves replay separates it from the rejection flow. **Included in this change.** +8. `state-machine.md`, `packages/core/README.md`, and `ARCHITECTURE.md` stop describing + the refusal. **Included in this change.** + +## Future Work + +- Whether a job sent back for changes should carry a pointer to the decision that sent it + back, the way `supersedes_job_id` points at a `FAILED` job. Today the link is + reconstructible from the trail; making it a field would be an `approval/v1` change and + therefore `agent-platform`'s. +- Whether repeated `REQUIRE_CHANGES` rounds should be bounded, and by whom. Policy, not + lifecycle. diff --git a/simulation/README.md b/simulation/README.md index 617f272..52b8994 100644 --- a/simulation/README.md +++ b/simulation/README.md @@ -27,13 +27,24 @@ nobody is watching. Exit code is non-zero if any check fails. | --- | --- | --- | | 1 | the full flow, `DRAFT → … → COMPLETED` | `check_full_flow` | | 2 | `REJECTED → DRAFT`, then resubmitted | `check_rejection_flow` | +| 2b | `REQUIRE_CHANGES → DRAFT`, and a replay that separates it from 2 | `check_require_changes_flow` | | 3 | `FAILED` with a reason, from a state `FAILABLE` allows | `check_failure_flow` | | 4 | the governance gate blocks execution without an `APPROVE` | `check_governance_gate` | | 5 | every transition emits an audit event | `check_every_transition_is_audited` | | 6 | the log is complete and replays to the same state | `check_replay` | | 7 | a runnable script or a test suite | this directory — both | -One flow arrived after issue #7 was written: `approval_expired` drives a job whose +Two flows arrived after issue #7 was written. + +`require_changes_then_resubmitted` drives a job the gate sends back for changes +([RFC-0011](../rfcs/0011-require-changes-destination.md)). It is in the run for a +specific reason: it ends in the same state as the rejection flow, and check `[2b]` +replays both to prove they are still distinguishable from the log alone — the +`REQUIRE_CHANGES` trail never passes through `REJECTED` and the rejection's does. +That is the claim RFC-0011 rests on, so it is checked here rather than asserted in +the RFC. + +`approval_expired` drives a job whose `APPROVE` lapsed where it sat, which settles at `TIMED_OUT` rather than at a failure. It is in the run so the trail checks 5 and 6 work on covers the `APPROVED → TIMED_OUT` edge and an approval carrying `expires_at` @@ -50,7 +61,10 @@ is expressed. A simulation that wrote `APPROVED → TASK_PLANNING` into itself i order to walk it would be a second declaration with the first one's authority, so `flows.main_line()` *derives* the path instead: at each state, discard the exits available from nearly everywhere — `CANCELLED`, `TIMED_OUT`, `FAILED`, `REJECTED`, -and the `AWAITING_APPROVAL` pause — and one successor is left. +the `AWAITING_APPROVAL` pause, and `DRAFT`, which since RFC-0011 the gate can send +a job back to — and one successor is left. A return to the start is not progress; +if the path ever genuinely forks, `main_line()` raises rather than picking a branch, +because a fork is a lifecycle change and belongs in an RFC first. The flow issue #7 spells out appears exactly once, as the thing that derivation is compared *against*. If the table and the issue ever disagree, the comparison fails diff --git a/simulation/__init__.py b/simulation/__init__.py index 68ef1ba..dee168f 100644 --- a/simulation/__init__.py +++ b/simulation/__init__.py @@ -19,6 +19,7 @@ main_line, never_approved, rejected_then_resubmitted, + require_changes_then_resubmitted, stalled_awaiting_approval, ) @@ -33,5 +34,6 @@ "main_line", "never_approved", "rejected_then_resubmitted", + "require_changes_then_resubmitted", "stalled_awaiting_approval", ] diff --git a/simulation/e2e_flow.py b/simulation/e2e_flow.py index 38796a2..3934fe4 100644 --- a/simulation/e2e_flow.py +++ b/simulation/e2e_flow.py @@ -10,6 +10,8 @@ [1] the full flow, DRAFT → … → COMPLETED [2] rejection, revision, resubmission + [2b] being sent back for changes — the same destination by a different route, + which is what RFC-0011 rests on and so is checked rather than asserted [3] failure, from every state RFC-0010 allows and from nowhere else [4] the governance gate — no execution without an APPROVE *record* [5] every transition leaves an audit event @@ -51,7 +53,12 @@ InvalidTransition, JobStateMachineError, ) -from devfactory_core.states import FAILABLE, POST_APPROVAL, TERMINAL # noqa: E402 +from devfactory_core.states import ( # noqa: E402 + DECISION_BY_EDGE, + FAILABLE, + POST_APPROVAL, + TERMINAL, +) from devfactory_observability import ( # noqa: E402 BrokenTrail, EventLog, @@ -69,6 +76,7 @@ job_factory, never_approved, rejected_then_resubmitted, + require_changes_then_resubmitted, stalled_awaiting_approval, ) @@ -223,6 +231,79 @@ def check_rejection_flow(new, reviewer) -> Job: return job +# ---- [2b] sent back for changes --------------------------------------------- +# RFC-0011. Not one of issue #7's six items — the flow did not exist when #7 was +# written — but it belongs beside rejection, because the two end in the same place +# and the whole question is whether they can still be told apart afterwards. + + +def check_require_changes_flow(new, reviewer) -> Job: + job = require_changes_then_resubmitted(new, job_id="job-002d", authority=reviewer) + expected = ( + "DRAFT", + "GOVERNANCE_ANALYSIS", + "DRAFT", + "GOVERNANCE_ANALYSIS", + "APPROVED", + "TASK_PLANNING", + ) + check( + "changes", + visited(job) == expected, + f"job-002d เดิน {' → '.join(expected)}", + f"job-002d เดินได้ {visited(job)}", + ) + check( + "changes", + [d.decision.value for d in job.decisions] == ["REQUIRE_CHANGES", "APPROVE"], + "ตีกลับให้แก้แล้วยื่นใหม่ ได้ decision ใบที่สอง ไม่ใช่การแก้ใบเดิม", + f"decision ที่บันทึกคือ {[d.decision.value for d in job.decisions]}", + ) + + # A REQUIRE_CHANGES leaves no approval behind, exactly as a REJECT does not. + sent_back = new("job-002e") + sent_back.submit_for_governance() + sent_back.require_changes(authority=reviewer, reason="needs a test plan") + if sent_back.approval is not None: + fail("changes", "REQUIRE_CHANGES ไม่ได้ล้าง approval — งานที่ยังไม่อนุมัติถือ approval อยู่") + else: + try: + sent_back.transition(JobState.TASK_PLANNING) + fail("changes", "งานที่ถูกตีกลับให้แก้ยังเดินเข้า TASK_PLANNING ได้") + except InvalidTransition: + ok("changes", "REQUIRE_CHANGES ไม่ใช่การอนุมัติ — งานยังทำงานไม่ได้จนกว่าจะได้ APPROVE") + + # 🔑 The claim RFC-0011 rests on: same destination, different trail. Checked by + # replaying both from the log alone — if a reader who was not there can tell + # them apart, the invariant survived the shared destination. + rejected = rejected_then_resubmitted(new, job_id="job-002f", authority=reviewer) + by_replay = { + "require_changes": replay_job(job.events), + "reject": replay_job(rejected.events), + } + routes = { + name: tuple(s.value for s in seen.states_visited) + for name, seen in by_replay.items() + } + check( + "changes", + JobState.REJECTED not in by_replay["require_changes"].states_visited + and JobState.REJECTED in by_replay["reject"].states_visited + and routes["require_changes"] != routes["reject"], + "replay แยกสองเคสออกจากกันได้ — เส้นทางของ REQUIRE_CHANGES ไม่ผ่าน REJECTED " + "ส่วนของ REJECT ผ่าน", + f"replay แยกไม่ออก: {routes}", + ) + check( + "changes", + [d for d in by_replay["require_changes"].decision_ids] + == [d.decision_id for d in job.decisions], + "decision ที่ replay ได้ตรงกับที่ engine บันทึกจริง รวมใบ REQUIRE_CHANGES", + "decision ที่ replay ได้ไม่ตรงกับของจริง", + ) + return job + + # ---- [3] failure ------------------------------------------------------------ @@ -422,16 +503,20 @@ def check_every_transition_is_audited(jobs, log) -> None: f"transition ที่ event หายไปหรือเนื้อหาไม่ตรง: {unlinked}", ) + # Which transitions are decisions is read off states.py by *edge*: since + # RFC-0011 the destination alone no longer says, DRAFT being reachable both by + # a REQUIRE_CHANGES from the gate and by the revision step out of REJECTED. undecided = [ job.job_id for job in jobs - if len([h for h in job.history if h.to_state in (JobState.APPROVED, JobState.REJECTED)]) + if len([h for h in job.history if (h.from_state, h.to_state) in DECISION_BY_EDGE]) != len([e for e in job.events if e.type_value == "GOVERNANCE_DECISION"]) ] check( "audit", not undecided, - "ทุกครั้งที่เข้า APPROVED หรือ REJECTED มี GOVERNANCE_DECISION คู่กับ STATE_TRANSITION", + "ทุก edge ที่เป็นคำตัดสินมี GOVERNANCE_DECISION คู่กับ STATE_TRANSITION " + "และ edge ที่ไม่ใช่คำตัดสินไม่มี", f"job ที่เข้า state ตัดสินใจโดยไม่มี GOVERNANCE_DECISION: {undecided}", ) @@ -523,6 +608,9 @@ def simulate(log: EventLog) -> list[Job]: print("\n[2] flow ปฏิเสธ — REJECTED กลับ DRAFT แล้วยื่นใหม่") rejected = check_rejection_flow(new, reviewer) + print("\n[2b] flow ตีกลับให้แก้ — REQUIRE_CHANGES กลับ DRAFT ตรง ๆ (rfcs/0011)") + sent_back = check_require_changes_flow(new, reviewer) + print("\n[3] flow ล้มเหลว — FAILED จาก state ที่ RFC-0010 อนุญาตเท่านั้น") failures = check_failure_flow(new, reviewer) @@ -542,7 +630,7 @@ def simulate(log: EventLog) -> list[Job]: expires_at=datetime(2026, 8, 19, 8, tzinfo=timezone.utc), ) - jobs = [happy, rejected, *failures, *gated, cancelled, stalled, expired] + jobs = [happy, rejected, sent_back, *failures, *gated, cancelled, stalled, expired] for job in jobs: log.extend(job.events) return jobs diff --git a/simulation/flows.py b/simulation/flows.py index 472b49f..1df4c89 100644 --- a/simulation/flows.py +++ b/simulation/flows.py @@ -44,9 +44,14 @@ def __call__(self, job_id: str) -> Job: ... #: and most offer ``TIMED_OUT`` or ``FAILED`` #: * ``REJECTED`` — a verdict, and the flow that follows it is its own #: * ``AWAITING_APPROVAL`` — a pause inside the path, not a step along it +#: * ``DRAFT`` — since RFC-0011 the gate can send a job back there for changes. +#: That is a return to the start, which is the opposite of progress; the flow it +#: begins is its own, exactly as ``REJECTED``'s is. ``DRAFT`` is never a forward +#: target from anywhere, so excluding it cannot hide a step. _NOT_FORWARD: frozenset[JobState] = (TERMINAL - {JobState.COMPLETED}) | { JobState.REJECTED, JobState.AWAITING_APPROVAL, + JobState.DRAFT, } @@ -148,6 +153,30 @@ def rejected_then_resubmitted( return job +def require_changes_then_resubmitted( + new: JobFactory, *, job_id: str, authority: Principal +) -> Job: + """Sent back for changes, revised, resubmitted, approved — RFC-0011. + + The counterpart to :func:`rejected_then_resubmitted`, and the reason both are + written down: they end in the same place and their trails are not the same + trail. This one goes ``GOVERNANCE_ANALYSIS -> DRAFT`` in a single step and + ``REJECTED`` never appears in its history, which is what keeps *"REQUIRE_CHANGES + ไม่ใช่ REJECT"* true of the record and not only of the wording. + + It carries on into ``TASK_PLANNING`` for the same reason the rejection flow + does: a ``REQUIRE_CHANGES`` clears the approval, so reaching execution proves + the *second* decision restored authority the first one withheld. + """ + job = new(job_id) + job.submit_for_governance(reason="first submission") + job.require_changes(authority=authority, reason="add a test plan and resubmit") + job.submit_for_governance(reason="test plan added") + job.approve(authority=authority, reason="the changes asked for are in") + job.transition(JobState.TASK_PLANNING) + return job + + def failed_at( new: JobFactory, *, diff --git a/simulation/tests/test_e2e_flow.py b/simulation/tests/test_e2e_flow.py index f58aafc..21605df 100644 --- a/simulation/tests/test_e2e_flow.py +++ b/simulation/tests/test_e2e_flow.py @@ -28,7 +28,7 @@ InvalidTransition, JobStateMachineError, ) -from devfactory_core.states import FAILABLE, POST_APPROVAL, TERMINAL +from devfactory_core.states import DECISION_BY_EDGE, FAILABLE, POST_APPROVAL, TERMINAL from devfactory_observability import ( BrokenTrail, EmptyTrail, @@ -52,6 +52,7 @@ main_line, never_approved, rejected_then_resubmitted, + require_changes_then_resubmitted, stalled_awaiting_approval, ) @@ -171,6 +172,115 @@ def test_a_rejection_does_not_leave_an_earlier_approval_behind(new, reviewer): replacement.transition(JobState.TASK_PLANNING) +# ---- [2b] sent back for changes, RFC-0011 ----------------------------------- +# The same destination as a rejection, by a different route. Every test here is +# about whether that route survives being written down and read back, because that +# is the only thing keeping ``REQUIRE_CHANGES ไม่ใช่ REJECT`` true of the record. + + +def test_require_changes_returns_to_draft_without_passing_rejected(new, reviewer): + job = require_changes_then_resubmitted(new, job_id="job-002d", authority=reviewer) + assert visited(job) == ( + "DRAFT", + "GOVERNANCE_ANALYSIS", + "DRAFT", + "GOVERNANCE_ANALYSIS", + "APPROVED", + "TASK_PLANNING", + ) + assert "REJECTED" not in visited(job) + + +def test_the_two_ways_back_to_draft_leave_different_trails(new, reviewer): + """RFC-0011's load-bearing claim, on the two flows side by side.""" + sent_back = require_changes_then_resubmitted( + new, job_id="job-002d", authority=reviewer + ) + rejected = rejected_then_resubmitted(new, job_id="job-002", authority=reviewer) + + assert sent_back.state is rejected.state is JobState.TASK_PLANNING + assert visited(sent_back) != visited(rejected) + assert "REJECTED" in visited(rejected) + assert "REJECTED" not in visited(sent_back) + # One hop to DRAFT rather than two: the gate sent it back itself. + assert len(sent_back.history) == len(rejected.history) - 1 + + +def test_replay_tells_the_two_apart_from_the_log_alone(new, reviewer): + """A reader who was not there has only the trail. It is enough.""" + sent_back = replay_job( + require_changes_then_resubmitted( + new, job_id="job-002d", authority=reviewer + ).events + ) + rejected = replay_job( + rejected_then_resubmitted(new, job_id="job-002", authority=reviewer).events + ) + assert sent_back.state is rejected.state is JobState.TASK_PLANNING + assert JobState.REJECTED not in sent_back.states_visited + assert JobState.REJECTED in rejected.states_visited + assert sent_back.states_visited != rejected.states_visited + + +def test_the_require_changes_decision_survives_the_round_trip(new, reviewer): + job = require_changes_then_resubmitted(new, job_id="job-002d", authority=reviewer) + seen = replay_job(job.events) + assert [d.decision.value for d in job.decisions] == ["REQUIRE_CHANGES", "APPROVE"] + assert list(seen.decision_ids) == [d.decision_id for d in job.decisions] + # The approval in force is the APPROVE, not the REQUIRE_CHANGES that preceded it. + assert seen.approval_decision_id == job.decisions[-1].decision_id + + +def test_a_require_changes_leaves_no_approval_behind(new, reviewer): + job = new("job-002e") + job.submit_for_governance() + job.require_changes(authority=reviewer, reason="needs a test plan") + assert job.state is JobState.DRAFT + assert job.approval is None + job.submit_for_governance(reason="revised") + with pytest.raises(InvalidTransition): + job.transition(JobState.TASK_PLANNING) + + +def test_a_trail_that_calls_a_require_changes_a_rejection_is_refused(new, reviewer): + """Forge the decision record and the edge no longer matches it. + + The two halves check each other: the route says "sent back for changes" and the + record would say "rejected", so the trail contradicts itself and replay says so + rather than believing the record. + """ + job = new("job-002g") + job.submit_for_governance() + job.require_changes(authority=reviewer, reason="needs a test plan") + forged = [ + dataclasses.replace( + e, + metadata={ + **e.metadata, + "approval": {**e.metadata["approval"], "decision": "REJECT"}, + }, + ) + if e.type_value == "GOVERNANCE_DECISION" + else e + for e in job.events + ] + with pytest.raises(UnauditedDecision) as excinfo: + replay_job(forged) + assert excinfo.value.recorded == "REJECT" + + +def test_resubmitting_after_a_rejection_needs_no_decision_of_its_own(new, reviewer): + """``REJECTED -> DRAFT`` shares a destination with a decision and is not one.""" + job = new("job-002h") + job.submit_for_governance() + job.reject(authority=reviewer, reason="out of scope") + job.transition(JobState.DRAFT) + seen = replay_job(job.events) + assert seen.state is JobState.DRAFT + assert len(seen.decision_ids) == 1 + assert [t.decision_id for t in seen.history] == [None, job.decisions[0].decision_id, None] + + # ---- [3] failure ------------------------------------------------------------ @@ -391,7 +501,7 @@ def test_the_direction_lock_is_a_backstop_on_replay_too(new, reviewer, monkeypat job.approve(authority=reviewer, reason="approved") job.transition(JobState.TASK_PLANNING) - monkeypatch.setattr(replay_module, "DECISION_BY_TARGET", {}) + monkeypatch.setattr(replay_module, "DECISION_BY_EDGE", {}) with pytest.raises(UnauditedExecution) as excinfo: replay_job(job.events) assert excinfo.value.state == "TASK_PLANNING" @@ -409,12 +519,14 @@ def run(new, owner, reviewer, log): new, job_id="job-001b", authority=reviewer, pause_in=JobState.DEPLOYABLE ), rejected_then_resubmitted(new, job_id="job-002", authority=reviewer), + require_changes_then_resubmitted(new, job_id="job-002d", authority=reviewer), failed_at(new, job_id="job-003", authority=reviewer, state=JobState.IN_PROGRESS), never_approved(new, job_id="job-004"), cancelled_by_a_person(new, job_id="job-005", owner=owner), stalled_awaiting_approval(new, job_id="job-006", authority=reviewer), ] - jobs.append(jobs[3].supersede(job_id="job-003-next")) + failed = next(j for j in jobs if j.state is JobState.FAILED) + jobs.append(failed.supersede(job_id="job-003-next")) for job in jobs: log.extend(job.events) return jobs, log @@ -438,16 +550,19 @@ def test_every_transition_points_at_an_event_that_is_really_in_the_log(run): assert event.transition.get("reason") == record.reason -def test_every_decision_state_entry_is_accompanied_by_a_decision_event(run): +def test_every_decision_edge_is_accompanied_by_a_decision_event(run): + """Read off ``states.py`` by edge, not by destination. + + Since RFC-0011 the destination alone no longer says whether a transition was a + verdict: ``GOVERNANCE_ANALYSIS -> DRAFT`` is one and ``REJECTED -> DRAFT`` is + not, and this counts both correctly only by asking about the edge. + """ jobs, _ = run for job in jobs: - entries = [ - h - for h in job.history - if h.to_state in (JobState.APPROVED, JobState.REJECTED) - ] + entries = [h for h in job.history if (h.from_state, h.to_state) in DECISION_BY_EDGE] emitted = [e for e in job.events if e.type_value == "GOVERNANCE_DECISION"] assert len(entries) == len(emitted) == len(job.decisions), job.job_id + assert [h.decision_id for h in entries] == [d.decision_id for d in job.decisions] def test_the_whole_run_reaches_one_tenant_partition(run):