diff --git a/CHANGELOG.md b/CHANGELOG.md index 916dec7..4517d71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,19 @@ All notable changes to `codesema` (the npm package in `packages/cli`) are documented here. Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [SemVer](https://semver.org). +## [0.19.0] - 2026-08-28 + +### Added + +- **The ticket state machine is now shared law.** `@codesema/contract` ships `ticket-state.ts`: the full table of legal `tickets.status` transitions (`TICKET_TRANSITIONS`), `isLegalTicketTransition(from, to)`, the derived `TICKET_TERMINAL_STATUSES`, and `targetTicketStatus(type, verdict)`. The runner refuses to report a transition the table forbids from the last status the hub answered with (tracked on the task record as `hub_ticket_status`); the hub validates the same table on its side. +- **A state requires its proof.** `ArmTransition` is now a discriminated union: `mr_opened` requires `mr_url`, `merged` requires `merge_sha`, enforced at compile time, in `sanitizeArmTransition`, and in the JSON schema. After a landed merge the runner reads the merge commit back from the forge (`gh pr list --json number,mergeCommit` / glab's `merge_commit_sha`) and reports it; when the commit cannot be read, the report is skipped and journaled as `merged_sha_unknown` (the hub's forge webhook reconciles). + +### Fixed + +- **No more phantom `mr_opened`.** A ship whose `gh pr create` produced no URL used to be reported as `mr_opened` anyway; the hub then drafted a ticket on a merge request that did not exist. The runner now reports the failure it actually is, with the way out in the message. +- **Merge settings are re-read at merge time.** `mergeStrategy`/`mergePolicy`/`deleteBranchAfterMerge`/`allowMergeWithoutChecks` were frozen at boot; a strategy set through the settings API was ignored until restart. The manager now re-reads them through a live getter (the `getChecksConfig` pattern). +- **Auto-merge without a strategy is refused before the forge.** With no `mergeStrategy` configured, the auto-merge used to call `gh pr merge` blind and fail as a generic forge error. It is now refused up front with reason `merge_strategy_unconfigured` and the way out (set a strategy, retry); the manual-merge path keeps D13 untouched. + ## [0.18.5] - 2026-08-28 ### Fixed diff --git a/docs/recovery.md b/docs/recovery.md new file mode 100644 index 0000000..8aedfb7 --- /dev/null +++ b/docs/recovery.md @@ -0,0 +1,101 @@ +# Recovery doctrine + +How codesema detects, absorbs, and escalates failure. This is the reference the state +machine, the healthchecks, and every retry in the codebase must be written against. + +The founding rule, in one sentence: **everything mechanizable is mechanized; a human +(or an agent) is only reached once the machine has provably exhausted its bounded +options; and every step of that ladder is bounded, tested, and validated.** + +## The ladder + +Every failure walks the same ladder, from the cheapest rung to the most expensive. +A rung is only reached when the one below it is exhausted. + +| Rung | Name | What it is | Example | +| ---- | --------------------------- | -------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | +| 0 | Prevention by construction | An illegal state cannot be represented: contract types, DB CHECK constraints, config validated at install time | `mr_opened` requires a merge-request URL; a merge strategy is asked for before the first merge can ever run | +| 1 | Mechanical detection | Probes, invariants, healthchecks, checks. Silence is never read as success | The egress proxy is probed right after start; a caged turn is watched by the semantic watchdog | +| 2 | Bounded mechanical recovery | Retry, replay, reconcile, re-claim — each with a name, a persisted counter, a ceiling, and a journal event | A vanished agent session is dropped and the turn replayed exactly once | +| 3 | Human escalation | Only after rung 2 is exhausted: the report carries the proof (the run's dying words) AND the way out | "merge refused: no strategy configured — set one in settings, then reply to replay the cycle" | +| 4 | Fix agent | An agent run to repair, opted into by explicit policy, itself fully caged and bounded (turns, budget, timeout) | A post-failure fix turn, behind the same gates as any other turn | + +Rung 4 is not an exception to the doctrine: the fix agent is one more consumer of the +same state machine, and its output goes through the same mechanical gates (criteria, +checks, review) as any human-triggered turn. + +## Rules + +1. **Recovery is a first-class transition.** Never an opportunistic try/catch. Each + mechanical recovery has: + - a **name** (`session_replayed`, `proxy_recreated`, `reconciled`, `reclaimed`); + - a **persisted counter** — it survives a daemon restart, otherwise the bound lies; + - a **journal event** in the task's `events.jsonl` (and, when task-scoped, the hub + outbox); + - a **test** exercising both the recovery and the exhaustion of its bound. +2. **Every transition is bounded.** A retry has a max count, a wait has a timeout, a + loop has a ceiling. Unbounded convergence is a bug by definition. +3. **The transition table is shared.** The legal state transitions live in + `@codesema/contract`, and both sides (runner and hub) refuse a transition that is + not in the table. A refused transition is an explicit error, never a silently + accepted phantom state. +4. **A state requires its proof.** A transition that claims an external fact carries + the evidence: `mr_opened` carries the MR URL, `done` carries the merge SHA. The + contract makes the field mandatory; a DB CHECK enforces it at rest. +5. **Silence is never success.** Every ephemeral process leaves its last words + somewhere readable: agent stdout+stderr tails travel inside exit errors; cage and + proxy containers log through a driver that survives `--rm` (journald when + available); a probe follows every detached start. +6. **Escalation carries the way out.** A rung-3 report names what was tried (which + recoveries, how many times), shows the evidence, and states the single action that + unblocks ("reply to replay", "configure X", "re-run install"). A dead-end message + ("task is waiting") is a doctrine violation. +7. **Client sovereignty bounds observability.** On the client's machine everything is + local and pull-based: `events.jsonl`, journald, `codesema doctor`. The ONLY thing + that leaves the machine is the business channel the client already consented to + (heartbeat, outbox) — richer messages on that channel, never a separate telemetry + agent, never a third-party SDK in the CLI. + +## Healthchecks + +Healthchecks are rung 1 running continuously. Each one declares its probe, its +cadence, and the rung-2 action its failure triggers — bounded like everything else. + +| Component | Probe | Cadence | On failure (bounded) | +| --------------------- | ------------------------------------------------------------------------------------------------------------------ | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Egress proxy (squid) | Docker `HEALTHCHECK` (config check / CONNECT against the allowlist) + `.State.Health` read before every caged turn | container-native + per turn | recreate, max 2; then rung 3: "isolation degraded", never a turn against a dead proxy | +| Runner daemon | systemd `WatchdogSec=` + `sd_notify` on each tick; `/api/status` as the HTTP probe | per tick | systemd restarts a frozen daemon, bounded by `StartLimitBurst`; the boot resume path re-adopts persisted tasks | +| Runner self-diagnosis | doctor-light: agent binary runs, forge CLI authed, container runtime usable, git identity present, disk space | periodic (minutes) | fix what is mechanical (nothing today, candidates later); otherwise report `degraded` + reasons INSIDE the existing heartbeat — the hub shows it in `runner list` and Settings | +| Caged turn | the semantic watchdog (frames, tool budgets, inactivity) — already the turn's healthcheck | continuous during a turn | kill escalation, then the turn's own failure path | +| Hub | container healthchecks (already in place) + a business probe: last runner heartbeat age, ticket queue progressing | periodic | alert on the hub side; the hub never reaches into a runner | + +A healthcheck is itself code under the doctrine: bounded (timeout, interval), tested, +and its failure produces a **named transition**, never just a log line. + +## The incident matrix + +Every incident from the first real production cycle (2026-08-28), read through the +ladder. "Missing rung" is what would have caught it earliest; "mechanization" is the +bounded answer (✅ shipped, ⬜ to build). + +| Incident | Missing rung | Mechanization | Bound | +| --------------------------------------------------------------------------------------------------------- | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------ | +| squid died at boot, silently, on every host | 1 | ✅ post-start liveness probe with crash capture (0.18.3) · ⬜ container `HEALTHCHECK` + journald logging | probe once + capture once | +| caged turn failed with a bare "exit code 1" | 1/3 | ✅ stdout last result frame + stderr tail inside exit errors (0.18.3, 0.18.5) | tails capped (400 chars / 8 KiB) | +| `claude --resume` target vanished (recycled home volume) | 2 | ✅ drop the dead session, replay the turn once with rebuilt context (0.18.5) · ⬜ keep the home volume until a terminal state | replay ×1 | +| no git identity on a fresh server: every commit failed | 0/2 | ✅ identity shipped end-to-end encrypted by autoconfig (0.18.4) · ✅ inline codesema signature as last resort | fallback is deterministic, no retry needed | +| heartbeats rejected ("not claimed") for 20 minutes, no self-healing | 2 | ⬜ reconciliation in the daemon tick: after N consecutive rejections, re-read hub truth and converge (re-claim, adopt, or close the local task) | N = 3 rejections | +| hub believed `mr_opened` while `gh pr create` had failed; the drafter then built a ticket on that phantom | 0 | ⬜ `mr_opened` requires `mr_url` (contract + DB CHECK); `done` requires the merge SHA | by construction | +| ship refused on `waiting_for_you`, refusal visible only as a raw gh error, then the decision disappeared | 3 | ⬜ every refusal is an event, journaled and sent up the outbox, rendered on the ticket with its way out | one event per refusal | +| merge ran with no strategy; gh refuses non-interactively | 0/3 | ⬜ ask the strategy at install/config time, prefill from the repo's allowed merge methods; until set, refuse the auto-merge with a rung-3 message instead of attempting | no blind retry | +| settings written through the API were ignored until a restart | 0 | ⬜ runner-loop settings are re-read per action (like `getChecksConfig`), or the API answers "restart required" | by construction | +| three bugs invisible to 3k+ unit tests (node `--env-file`, wrapped pg errors, gh non-interactive) | 1 (in CI) | ⬜ real-binaries CI suite: packed tarball installed and smoked under node AND bun; real postgres/squid/gh; flag-drift check against the real CLIs' `--help` | CI-only, time-boxed | + +## What this doctrine forbids + +- An unbounded retry loop, anywhere. +- A recovery that only exists in a catch block, without a name, a counter, or a test. +- A status reported without its proof. +- A refusal or failure whose only trace is a local log line. +- A telemetry SDK inside the client-side CLI. +- Reaching a human while a bounded mechanical option remains untried. diff --git a/package.json b/package.json index 584db4e..73e0dc4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "codesema-tools", - "version": "0.18.5", + "version": "0.19.0", "private": true, "type": "module", "workspaces": [ diff --git a/packages/cli/package.json b/packages/cli/package.json index 128a485..0f18632 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "codesema", - "version": "0.18.5", + "version": "0.19.0", "description": "Local merge request review, step by step. Your AI agent reviews, codesema displays.", "license": "MIT", "author": "Hasan TASKIN", diff --git a/packages/cli/src/hub-client.test.ts b/packages/cli/src/hub-client.test.ts index 4de2f7d..d8a15a9 100644 --- a/packages/cli/src/hub-client.test.ts +++ b/packages/cli/src/hub-client.test.ts @@ -341,15 +341,21 @@ describe('heartbeat / transition / pushEvents', () => { await transition( creds, 't1', - { type: 'merged', idempotency_key: 'k1', at: '2026-01-01T00:00:00.000Z' }, + { + type: 'merged', + merge_sha: 'a1b2c3d4e5', + idempotency_key: 'k1', + at: '2026-01-01T00:00:00.000Z', + }, fetchStub(200, {}, calls), ) - const body = JSON.parse(String(calls[0]?.init.body)) as { - type: string - idempotency_key: string - at: string - } - expect(body).toEqual({ type: 'merged', idempotency_key: 'k1', at: '2026-01-01T00:00:00.000Z' }) + const body = JSON.parse(String(calls[0]?.init.body)) as Record + expect(body).toEqual({ + type: 'merged', + merge_sha: 'a1b2c3d4e5', + idempotency_key: 'k1', + at: '2026-01-01T00:00:00.000Z', + }) }) test('pushEvents sends run/ticket ids and the event batch', async () => { diff --git a/packages/cli/src/task-hub.test.ts b/packages/cli/src/task-hub.test.ts index 1ab029d..261f9d1 100644 --- a/packages/cli/src/task-hub.test.ts +++ b/packages/cli/src/task-hub.test.ts @@ -4,14 +4,16 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import { loadGlobalConfig, saveGlobalConfig } from './config.js' -import type { ArmOrder, TaskEvent, TaskRecord, TaskTurn } from './contract.js' +import type { ArmOrder, ArmTicket, TaskEvent, TaskRecord, TaskTurn } from './contract.js' import { flushHubOutbox, heartbeatHubTicket, queueHubEvent, reportHubTransition, resetPendingHubEventBatches, + type ArmTransitionDraft, } from './task-hub.js' +import { loadTask, saveTask } from './tasks-store.js' type Call = { url: string; init: RequestInit } @@ -106,6 +108,25 @@ function withoutHubTicket(record: TaskRecord): TaskRecord { return rest } +function fakeArmTicket(status: ArmTicket['status']): ArmTicket { + return { + id: 'tkt-1', + repo_remote_url: 'https://github.com/o/r.git', + title: 't', + body: 'b', + status, + depends_on: null, + executed_by: 'cli:ws1', + lease_expires_at: null, + issue: null, + branch: 'codesema/task-t', + mr_iid: null, + mr_url: null, + created_at: '2026-01-01T00:00:00.000Z', + updated_at: '2026-01-01T00:00:00.000Z', + } +} + describe('task-hub', () => { const previousConfigDir = process.env.CODESEMA_CONFIG_DIR let configDir: string @@ -184,13 +205,23 @@ describe('task-hub', () => { test('a task with no hub_ticket is a no-op', async () => { const calls: Call[] = [] const record = withoutHubTicket(fakeRecord()) - await reportHubTransition(cwd, record, { type: 'mr_opened' }, fetchStub(200, {}, calls)) + await reportHubTransition( + cwd, + record, + { type: 'mr_opened', mr_url: 'https://hub.example/mr/1' }, + fetchStub(200, {}, calls), + ) expect(calls.length).toBe(0) }) test('a network failure queues the report in the outbox', async () => { const record = fakeRecord() - await reportHubTransition(cwd, record, { type: 'merged' }, fetchOffline()) + await reportHubTransition( + cwd, + record, + { type: 'merged', merge_sha: 'a1b2c3d4e5' }, + fetchOffline(), + ) const lines = outboxLines(cwd) expect(lines.length).toBe(1) const entry = lines[0] as { kind: string; ticket_id: string; transition: { type: string } } @@ -205,7 +236,7 @@ describe('task-hub', () => { await reportHubTransition( cwd, record, - { type: 'merged' }, + { type: 'merged', merge_sha: 'a1b2c3d4e5' }, fetchStub(503, { error: 'down' }, calls), ) expect(outboxLines(cwd).length).toBe(1) @@ -223,12 +254,110 @@ describe('task-hub', () => { expect(calls.length).toBe(1) expect(outboxLines(cwd)).toEqual([]) }) + + test('a report without its proof is refused before any network call', async () => { + const calls: Call[] = [] + // The compiler already forbids this shape; the cast proves the RUNTIME + // gate holds for a record that reached here past it (a replayed file, + // an older caller). + await reportHubTransition( + cwd, + fakeRecord(), + { type: 'mr_opened' } as unknown as ArmTransitionDraft, + fetchStub(200, {}, calls), + ) + await reportHubTransition( + cwd, + fakeRecord(), + { type: 'merged' } as unknown as ArmTransitionDraft, + fetchStub(200, {}, calls), + ) + expect(calls).toEqual([]) + expect(outboxLines(cwd)).toEqual([]) + }) + + test('an out-of-table transition is refused before any network call', async () => { + const calls: Call[] = [] + const record = fakeRecord({ hub_ticket_status: 'published' }) + await reportHubTransition( + cwd, + record, + { type: 'mr_opened', mr_url: 'https://hub.example/mr/1' }, + fetchStub(200, {}, calls), + ) + expect(calls).toEqual([]) + expect(outboxLines(cwd)).toEqual([]) + }) + + test('an unknown local hub status lets the report through: the hub revalidates', async () => { + const calls: Call[] = [] + await reportHubTransition( + cwd, + fakeRecord(), + { type: 'mr_opened', mr_url: 'https://hub.example/mr/1' }, + fetchStub(200, {}, calls), + ) + expect(calls.length).toBe(1) + }) + + test('a legal transition from the last known status is posted', async () => { + const calls: Call[] = [] + const record = fakeRecord({ hub_ticket_status: 'in_progress' }) + await reportHubTransition( + cwd, + record, + { type: 'mr_opened', mr_url: 'https://hub.example/mr/1' }, + fetchStub(200, {}, calls), + ) + expect(calls.length).toBe(1) + }) + + test('a review verdict short of approve is not a transition: never table-checked', async () => { + const calls: Call[] = [] + const record = fakeRecord({ hub_ticket_status: 'published' }) + await reportHubTransition( + cwd, + record, + { type: 'review_result', verdict: 'request_changes' }, + fetchStub(200, {}, calls), + ) + expect(calls.length).toBe(1) + }) + + test('the ticket status the hub answers with is remembered on the record', async () => { + const record = fakeRecord({ hub_ticket_status: 'mr_opened' }) + saveTask(cwd, record) + await reportHubTransition( + cwd, + record, + { type: 'review_result', verdict: 'approve' }, + fetchStub(200, { ticket: fakeArmTicket('ready_to_merge') }, []), + ) + expect(loadTask(cwd, record.id)?.hub_ticket_status).toBe('ready_to_merge') + }) + + test('an answer without a readable ticket leaves the last known status alone', async () => { + const record = fakeRecord({ hub_ticket_status: 'mr_opened' }) + saveTask(cwd, record) + await reportHubTransition( + cwd, + record, + { type: 'review_result', verdict: 'approve' }, + fetchStub(200, {}, []), + ) + expect(loadTask(cwd, record.id)?.hub_ticket_status).toBe('mr_opened') + }) }) describe('flushHubOutbox', () => { test('replays a queued transition and empties the outbox on success', async () => { const record = fakeRecord() - await reportHubTransition(cwd, record, { type: 'merged' }, fetchOffline()) + await reportHubTransition( + cwd, + record, + { type: 'merged', merge_sha: 'a1b2c3d4e5' }, + fetchOffline(), + ) expect(outboxLines(cwd).length).toBe(1) const calls: Call[] = [] @@ -240,7 +369,12 @@ describe('task-hub', () => { test('a 409 on replay drops the entry rather than keeping it queued', async () => { const record = fakeRecord() - await reportHubTransition(cwd, record, { type: 'merged' }, fetchOffline()) + await reportHubTransition( + cwd, + record, + { type: 'merged', merge_sha: 'a1b2c3d4e5' }, + fetchOffline(), + ) expect(outboxLines(cwd).length).toBe(1) const calls: Call[] = [] @@ -251,7 +385,12 @@ describe('task-hub', () => { test('still offline: the entry is kept, not lost', async () => { const record = fakeRecord() - await reportHubTransition(cwd, record, { type: 'merged' }, fetchOffline()) + await reportHubTransition( + cwd, + record, + { type: 'merged', merge_sha: 'a1b2c3d4e5' }, + fetchOffline(), + ) expect(outboxLines(cwd).length).toBe(1) await flushHubOutbox(cwd, fetchOffline('still offline')) diff --git a/packages/cli/src/task-hub.ts b/packages/cli/src/task-hub.ts index 9147f8e..92c7df1 100644 --- a/packages/cli/src/task-hub.ts +++ b/packages/cli/src/task-hub.ts @@ -24,7 +24,11 @@ import { ensureWorkDir } from './config.js' import { ARM_LABEL_MAX, cutCodePoints, + isLegalTicketTransition, sanitizeArmOrder, + sanitizeArmTicket, + sanitizeArmTransition, + targetTicketStatus, type ArmEvent, type ArmOrder, type ArmTransition, @@ -178,6 +182,40 @@ async function postToHub( : { kind: 'client_error', status: res.status, detail } } +type DistributiveOmit = T extends unknown ? Omit : never + +/** + * What a caller hands `reportHubTransition`: an `ArmTransition` minus the + * two fields it computes itself. Distributive on purpose: a plain `Omit` + * over the union would flatten the per-type proof requirements away, and the + * compiler is the first gate the doctrine leans on (`mr_opened` demands + * `mr_url`, `merged` demands `merge_sha`, at every call site). + */ +export type ArmTransitionDraft = DistributiveOmit + +/** + * Remembers the ticket status the hub itself just answered with, so the next + * report can be checked against the shared transition table. Load, mutate + * and save have no await between them, so no concurrent record write can + * interleave. Best-effort by design: an unreadable body or a vanished record + * leaves the last known status in place, and the table guard treats absence + * as "unknown, let the hub decide". + */ +async function rememberHubTicketStatus(cwd: string, taskId: string, body: unknown): Promise { + // Dynamic on purpose: tasks-store.ts statically imports this module + // (queueHubEvent), so a static import back would be a module cycle. + const { loadTask, saveTask } = await import('./tasks-store.js') + const ticket = sanitizeArmTicket((body as { ticket?: unknown } | undefined)?.ticket) + if (!ticket) { + return + } + const current = loadTask(cwd, taskId) + if (!current?.hub_ticket || current.hub_ticket_status === ticket.status) { + return + } + saveTask(cwd, { ...current, hub_ticket_status: ticket.status }) +} + /** * Reports one fact about a hub ticket's execution back to the hub: * `mr_opened` on ship, `review_result` on a settled review verdict, `merged` @@ -197,23 +235,51 @@ async function postToHub( * Never throws. Offline, or a 5xx: appended to `.codesema/hub-outbox.jsonl` * for `flushHubOutbox` to replay later. A 4xx: logged once and abandoned, * never retried. + * + * Two refusals happen HERE, before anything is posted (recovery doctrine, + * rule 3: both sides refuse an out-of-table transition): a report without + * its proof (`sanitizeArmTransition` returns null), and a report whose + * claimed status is not a legal move from the last status the hub itself + * answered with (`hub_ticket_status`, remembered below on every successful + * round trip; unknown on legacy records, which then pass: the hub + * revalidates every report anyway). */ export async function reportHubTransition( cwd: string, record: TaskRecord, - transition: Omit, + transition: ArmTransitionDraft, fetchImpl: typeof fetch = fetch, ): Promise { const ticketId = record.hub_ticket?.id if (!ticketId) { return } - const full: ArmTransition = { + const label = `transition '${transition.type}' for task ${record.id}` + // The same sanitizer the hub-facing schema mirrors: a proof-less claim + // (mr_opened without mr_url, merged without merge_sha) comes back null and + // is never posted. Refusing here is the whole point: a phantom state on + // the hub is worse than a missing report (the forge webhook reconciles). + const full = sanitizeArmTransition({ ...transition, idempotency_key: `${record.id}:${transition.type}:${record.turns.length}`, at: new Date().toISOString(), + }) + if (!full) { + logHubFailure( + label, + 'refused before posting: a state requires its proof and this report carries none', + ) + return + } + const claimed = targetTicketStatus(full.type, full.verdict) + const lastKnown = record.hub_ticket_status + if (claimed && lastKnown && !isLegalTicketTransition(lastKnown, claimed)) { + logHubFailure( + label, + `refused before posting: ${lastKnown} → ${claimed} is not in the shared transition table`, + ) + return } - const label = `transition '${transition.type}' for task ${record.id}` const creds = loadSyncCredentials() if (!creds) { logHubFailure(label, 'no sync credentials configured') @@ -227,6 +293,7 @@ export async function reportHubTransition( fetchImpl, ) if (outcome.kind === 'ok') { + await rememberHubTicketStatus(cwd, record.id, outcome.body) return } if (outcome.kind === 'client_error') { diff --git a/packages/cli/src/task-merge.test.ts b/packages/cli/src/task-merge.test.ts index 3164d5b..2148c99 100644 --- a/packages/cli/src/task-merge.test.ts +++ b/packages/cli/src/task-merge.test.ts @@ -698,7 +698,7 @@ describe("the merge gate's git reads are bounded (MAJEUR 2)", () => { ` cwd: ${JSON.stringify(cwd)},`, ` runnerAutoMerge: true,`, ` task: ${JSON.stringify(greenTask())},`, - ` settings: ${JSON.stringify(settings({ policy: 'auto' }))},`, + ` settings: ${JSON.stringify(settings({ policy: 'auto', strategy: 'merge' }))},`, // Injected: this test is about the ONE git read left on this path. ` inputs: ${JSON.stringify(greenInputs())},`, ` execForge: () => Promise.resolve({ kind: 'ok', stdout: 'https://forge/mr/1' }),`, @@ -786,7 +786,8 @@ describe('mergeTask under mergePolicy: human (the default)', () => { }) describe('mergeTask under mergePolicy: auto', () => { - const auto = (over: Partial = {}) => settings({ policy: 'auto', ...over }) + const auto = (over: Partial = {}) => + settings({ policy: 'auto', strategy: 'merge', ...over }) test('four green conditions call gh pr merge with the expected argv', async () => { const repo = makeRepoWithOrigin('git@github.com:o/r.git') @@ -801,7 +802,7 @@ describe('mergeTask under mergePolicy: auto', () => { }) expect(outcome.kind).toBe('merged') expect(forge.calls).toEqual([ - { cli: 'gh', args: ['pr', 'merge', 'codesema/task-add-rate-limiting'] }, + { cli: 'gh', args: ['pr', 'merge', 'codesema/task-add-rate-limiting', '--merge'] }, ]) expect(outcome.events.at(-1)?.data).toMatchObject({ name: 'merged', cli: 'gh' }) }) @@ -825,20 +826,25 @@ describe('mergeTask under mergePolicy: auto', () => { ]) }) - test('no mergeStrategy means NO strategy option: the convention is the repo’s', async () => { + test('no mergeStrategy refuses the auto-merge BEFORE any forge call, with the way out', async () => { const repo = makeRepoWithOrigin('git@github.com:o/r.git') const forge = recordingForge() - await mergeTask({ + const outcome = await mergeTask({ runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: auto(), + settings: settings({ policy: 'auto' }), inputs: greenInputs(), execForge: forge.exec, }) - for (const flag of ['--merge', '--squash', '--rebase']) { - expect(forge.calls[0]?.args).not.toContain(flag) - } + expect(outcome.kind).toBe('refused') + expect(outcome.kind === 'refused' && outcome.reason.code).toBe('merge_strategy_unconfigured') + expect(outcome.kind === 'refused' && outcome.reason.detail).toContain('mergeStrategy') + expect(forge.calls).toEqual([]) + expect(outcome.events.at(-1)?.data).toMatchObject({ + name: 'refused', + message: expect.stringContaining('mergeStrategy'), + }) }) test('an explicit strategy reaches the argv, per CLI', async () => { @@ -908,6 +914,61 @@ describe('mergeTask under mergePolicy: auto', () => { expect(deleted.calls[0]?.args).toContain('--delete-branch') }) + test('a ticketed merge reads the merge commit back: the proof travels on the journal', async () => { + const answers: ShipCliOutcome[] = [ + { kind: 'ok', stdout: 'Merged pull request https://github.com/o/r/pull/7' }, + { kind: 'ok', stdout: JSON.stringify([{ number: 7, mergeCommit: { oid: 'A1B2C3D4E5F6' } }]) }, + ] + const calls: { cli: string; args: string[] }[] = [] + const outcome = await mergeTask({ + runnerAutoMerge: true, + cwd: makeRepoWithOrigin('git@github.com:o/r.git'), + task: greenTask({ hub_ticket: { id: 'tkt-1', title: 'x' } }), + settings: auto(), + inputs: greenInputs(), + execForge: (cli, args) => { + calls.push({ cli, args }) + return Promise.resolve(answers[calls.length - 1] as ShipCliOutcome) + }, + }) + expect(outcome.kind).toBe('merged') + expect(calls[1]?.args).toContain('number,mergeCommit') + const merged = outcome.events.find((event) => event.data.name === 'merged') + expect(merged?.data.sha).toBe('a1b2c3d4e5f6') + expect(outcome.events.some((event) => event.data.name === 'merged_sha_unknown')).toBe(false) + }) + + test('a landed merge whose commit cannot be read says merged_sha_unknown out loud', async () => { + const forge = recordingForge({ kind: 'ok', stdout: 'Merged pull request #7' }) + const outcome = await mergeTask({ + runnerAutoMerge: true, + cwd: makeRepoWithOrigin('git@github.com:o/r.git'), + task: greenTask({ hub_ticket: { id: 'tkt-1', title: 'x' } }), + settings: auto(), + inputs: greenInputs(), + execForge: forge.exec, + }) + expect(outcome.kind).toBe('merged') + const unknown = outcome.events.find((event) => event.data.name === 'merged_sha_unknown') + expect(unknown?.data.message).toContain('webhook') + const merged = outcome.events.find((event) => event.data.name === 'merged') + expect(merged && 'sha' in merged.data).toBe(false) + }) + + test('a task with no hub_ticket never pays the proof read', async () => { + const forge = recordingForge({ kind: 'ok', stdout: 'Merged pull request #7' }) + const outcome = await mergeTask({ + runnerAutoMerge: true, + cwd: makeRepoWithOrigin('git@github.com:o/r.git'), + task: greenTask(), + settings: auto(), + inputs: greenInputs(), + execForge: forge.exec, + }) + expect(outcome.kind).toBe('merged') + expect(forge.calls).toHaveLength(1) + }) + test('a missing condition emits NO merge command at all', async () => { for (const inputs of [ { review: makeReview('request_changes', [], metVerdicts()) }, @@ -1044,12 +1105,14 @@ describe('arm/runner integration: runnerAutoMerge overrides mergePolicy for a ti cwd: repo, runnerAutoMerge: true, task: ticketedGreenTask(), - settings: settings({ policy: 'human' }), + settings: settings({ policy: 'human', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) expect(outcome.kind).toBe('merged') - expect(forge.calls.length).toBe(1) + // Two calls since the proof read: the merge itself, then the bounded + // merged-list read that fetches the merge commit for the hub report. + expect(forge.calls.length).toBe(2) }) test('runnerAutoMerge: false holds a ticketed task, like any human-policy task', async () => { @@ -1108,7 +1171,7 @@ describe('arm/runner integration: runnerAutoMerge overrides mergePolicy for a ti cwd: repo, runnerAutoMerge: false, task: ticketedGreenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) @@ -1185,7 +1248,7 @@ describe('what the merge never does', () => { runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) @@ -1208,7 +1271,7 @@ describe('what the merge never does', () => { runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) @@ -1228,7 +1291,7 @@ describe('what the merge never does', () => { runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) @@ -1268,7 +1331,7 @@ describe('D20 idempotence: a branch the forge already merged is never merged twi runnerAutoMerge: true, cwd: repo, task, - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge, }) @@ -1288,7 +1351,7 @@ describe('D20 idempotence: a branch the forge already merged is never merged twi '--limit', '1', '--json', - 'number', + 'number,mergeCommit', ]) const mergedEvent = outcome.events.find((event) => event.data.name === 'merged') expect(mergedEvent?.data.already_merged).toBe(true) @@ -1304,7 +1367,7 @@ describe('D20 idempotence: a branch the forge already merged is never merged twi runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge: forge.exec, }) @@ -1330,7 +1393,7 @@ describe('D20 idempotence: a branch the forge already merged is never merged twi runnerAutoMerge: true, cwd: repo, task: greenTask(), - settings: settings({ policy: 'auto' }), + settings: settings({ policy: 'auto', strategy: 'merge' }), inputs: greenInputs(), execForge, }) @@ -1375,7 +1438,7 @@ describe('the merge really runs a forge CLI when nothing is injected', () => { ` cwd: ${JSON.stringify(repo)},`, ` runnerAutoMerge: true,`, ` task: ${JSON.stringify(greenTask())},`, - ` settings: ${JSON.stringify(settings({ policy: 'auto' }))},`, + ` settings: ${JSON.stringify(settings({ policy: 'auto', strategy: 'merge' }))},`, ` inputs: ${JSON.stringify(greenInputs())},`, `})`, `process.stdout.write(`, @@ -1406,6 +1469,7 @@ describe('the merge really runs a forge CLI when nothing is injected', () => { 'pr', 'merge', 'codesema/task-add-rate-limiting', + '--merge', ]) }, 30_000) }) diff --git a/packages/cli/src/task-merge.ts b/packages/cli/src/task-merge.ts index 172e1c7..ce7ad87 100644 --- a/packages/cli/src/task-merge.ts +++ b/packages/cli/src/task-merge.ts @@ -33,6 +33,7 @@ import { DEFAULT_MERGE_SETTINGS, type MergeSettings, type MergeStrategy } from './config.js' import { isTerminalReason, + sanitizeArmSha, type ChecksUnavailableDetail, type CriteriaMissingDetail, type ReasonCode, @@ -636,15 +637,28 @@ export function isMergeConflictError(message: string): boolean { * either way, so the ordinary forge failure this guards falls through and * surfaces exactly as it always has. */ -async function branchAlreadyMerged( +async function fetchMergedProof( cli: 'gh' | 'glab', cwd: string, branch: string, execForge: ShipForgeExecFn, -): Promise { +): Promise<{ merged: boolean; sha: string | null }> { + // gh's `mergeCommit` is `{oid}`; GitLab answers `merge_commit_sha`, or + // `squash_commit_sha` when the MR landed as a squash (the other is null + // then). One list call answers both "is it merged" and "as what commit". const args = cli === 'gh' - ? ['pr', 'list', `--head=${branch}`, '--state', 'merged', '--limit', '1', '--json', 'number'] + ? [ + 'pr', + 'list', + `--head=${branch}`, + '--state', + 'merged', + '--limit', + '1', + '--json', + 'number,mergeCommit', + ] : [ 'mr', 'list', @@ -657,14 +671,60 @@ async function branchAlreadyMerged( ] const outcome = await execForge(cli, args, cwd) if (outcome.kind !== 'ok') { - return false + return { merged: false, sha: null } } try { const data: unknown = JSON.parse(outcome.stdout) - return Array.isArray(data) && data.length > 0 + if (!Array.isArray(data) || data.length === 0) { + return { merged: false, sha: null } + } + const entry = data[0] as { + mergeCommit?: { oid?: unknown } | null + merge_commit_sha?: unknown + squash_commit_sha?: unknown + } + const candidate = + cli === 'gh' ? entry.mergeCommit?.oid : (entry.merge_commit_sha ?? entry.squash_commit_sha) + return { merged: true, sha: sanitizeArmSha(candidate) ?? null } } catch { - return false + return { merged: false, sha: null } + } +} + +/** + * Reports `merged` to the hub WITH its proof, or says out loud why it will + * not: a landed merge whose commit the forge did not answer with is journaled + * as `merged_sha_unknown` and never posted: `merged` without `merge_sha` is + * exactly the phantom-state shape the contract refuses, and the hub's own + * forge webhook (which carries the sha) reconciles the ticket instead. + */ +function reportMergedWithProof( + opts: MergeTaskOptions, + cli: 'gh' | 'glab', + sha: string | null, + events: AppendTaskEventInput[], +): void { + if (!opts.task.hub_ticket) { + return } + if (!sha) { + events.push({ + type: MERGE_EVENT, + data: { + name: 'merged_sha_unknown', + cli, + branch: opts.task.branch, + message: + 'merge landed but the forge did not answer with the merge commit; hub report skipped, the forge webhook reconciles the ticket', + }, + }) + return + } + void reportHubTransition(opts.cwd, opts.task, { + type: 'merged', + branch: opts.task.branch, + merge_sha: sha, + }) } // --- outcome --------------------------------------------------------------- @@ -836,6 +896,34 @@ export async function mergeTask(opts: MergeTaskOptions): Promise { }) return { kind: 'refused', reason, readiness, events } } + if (!settings.strategy) { + // Refused BEFORE any forge CLI runs (recovery doctrine, rung 0/3): a + // strategy nobody configured is never defaulted on their behalf (D13), + // and a blind non-interactive `gh pr merge` on a multi-method repo fails + // with gh's own flag demand anyway. One named refusal that carries the + // way out beats a raw forge error read as `forge_unreachable`. + const reason = taskReason( + 'merge_strategy_unconfigured', + 'auto-merge refused: no mergeStrategy configured. Set one (codesema config, or the runner settings API), then retry the merge', + ) + events.push({ + type: MERGE_EVENT, + data: { + name: 'refused', + policy: settings.policy, + terminal: isTerminalReason(reason.code), + ...(reason.detail ? { message: reason.detail } : {}), + }, + reason_code: reason.code, + }) + if (opts.task.hub_ticket) { + void reportHubTransition(opts.cwd, opts.task, { + type: 'failed', + error_message: reason.detail ?? 'auto-merge refused: no mergeStrategy configured', + }) + } + return { kind: 'refused', reason, readiness, events } + } const execForge = opts.execForge ?? ((cli, args, cwd) => execCli(cli, args, cwd)) let note: string | null = null @@ -873,9 +961,15 @@ export async function mergeTask(opts: MergeTaskOptions): Promise { // branch, and the forge's own refusal (already merged, the PR/MR no // longer open) reads exactly like any other error — never a conflict, // so it never took the branch above. Asked here, not before the call: - // see branchAlreadyMerged's own header for why the cost is paid only + // see fetchMergedProof's own header for why the cost is paid only // once a fresh attempt has already failed. - if (await branchAlreadyMerged(candidate.cli, opts.cwd, opts.task.branch, execForge)) { + const priorProof = await fetchMergedProof( + candidate.cli, + opts.cwd, + opts.task.branch, + execForge, + ) + if (priorProof.merged) { events.push({ type: MERGE_EVENT, data: { @@ -883,14 +977,10 @@ export async function mergeTask(opts: MergeTaskOptions): Promise { cli: candidate.cli, branch: opts.task.branch, already_merged: true, + ...(priorProof.sha ? { sha: priorProof.sha } : {}), }, }) - if (opts.task.hub_ticket) { - void reportHubTransition(opts.cwd, opts.task, { - type: 'merged', - branch: opts.task.branch, - }) - } + reportMergedWithProof(opts, candidate.cli, priorProof.sha, events) return { kind: 'merged', cli: candidate.cli, url: null, readiness, events } } // Keep trying (a dual-remote setup may have the other CLI working) but @@ -899,6 +989,14 @@ export async function mergeTask(opts: MergeTaskOptions): Promise { continue } const url = extractMrUrl(outcome.stdout) + // Neither `gh pr merge` nor `glab mr merge` hands the merge commit back + // on its own output, and `merged` without its sha is a claim the + // contract refuses, so the proof is read back from the forge in one + // bounded follow-up call (the same list read the idempotence guard uses). + const proof = + opts.task.hub_ticket === undefined + ? { merged: true, sha: null } + : await fetchMergedProof(candidate.cli, opts.cwd, opts.task.branch, execForge) events.push({ type: MERGE_EVENT, data: { @@ -908,15 +1006,10 @@ export async function mergeTask(opts: MergeTaskOptions): Promise { strategy: settings.strategy ?? 'forge default', deleted_branch: settings.deleteBranch, ...(url ? { url } : {}), + ...(proof.sha ? { sha: proof.sha } : {}), }, }) - if (opts.task.hub_ticket) { - // `merge_sha` is omitted: neither `gh pr merge` nor `glab mr merge` - // hands one back on this path (only the MR/PR url, when the forge - // gives one). The hub reads a `merged` transition with no sha as - // "landed, sha unknown" rather than a claim about a commit nobody read. - void reportHubTransition(opts.cwd, opts.task, { type: 'merged', branch: opts.task.branch }) - } + reportMergedWithProof(opts, candidate.cli, proof.sha, events) return { kind: 'merged', cli: candidate.cli, url, readiness, events } } diff --git a/packages/cli/src/task-review.ts b/packages/cli/src/task-review.ts index 8e15b4c..34afec6 100644 --- a/packages/cli/src/task-review.ts +++ b/packages/cli/src/task-review.ts @@ -12,7 +12,6 @@ import { ensureWorkDir, type ReviewMode } from './config.js' import { sanitizeRecord, - type ArmTransition, type Finding, type ReviewRecord, type TaskChecks, @@ -48,7 +47,7 @@ import { unmetCriteriaFixChapter, type CriteriaOutcome, } from './task-criteria-gate.js' -import { reportHubTransition } from './task-hub.js' +import { reportHubTransition, type ArmTransitionDraft } from './task-hub.js' import { REVIEW_CUT_DETAIL, taskCriteria, @@ -343,7 +342,7 @@ export function hubSettleTransition(opts: { reviewOutcome?: ReviewRecord reason?: TaskReason costTicks?: number -}): Omit { +}): ArmTransitionDraft { if (opts.status === 'review_ko' && !opts.reviewOutcome) { return { type: 'failed', diff --git a/packages/cli/src/task-server.ts b/packages/cli/src/task-server.ts index 26a9c89..6d17a31 100644 --- a/packages/cli/src/task-server.ts +++ b/packages/cli/src/task-server.ts @@ -646,6 +646,15 @@ export type CreateTaskManagerOptions = { * `codesema review`) honestly offers, and it never merges anything. */ mergeSettings?: MergeSettings + /** + * Same four settings, RE-READ at the moment a merge decision is made + * (same live-getter pattern as `getChecksConfig`): a `mergeStrategy` set + * through the settings API used to be ignored until the next restart, + * because the boot-time `mergeSettings` value was the only one ever + * consulted. When present it wins over `mergeSettings`, which stays as the + * static fallback tests and plain servers hand in. + */ + getMergeSettings?: () => MergeSettings /** * T3.6: merge keys found on the global config file, present but unusable. * Passed through so the degradation is named on the TASK's journal too, not @@ -1881,7 +1890,8 @@ export function createTaskManager(opts: CreateTaskManagerOptions): TaskManager { if ( effectiveMergePolicyIsAuto( record, - opts.mergeSettings ?? DEFAULT_MERGE_SETTINGS, + (opts.getMergeSettings ? opts.getMergeSettings() : opts.mergeSettings) ?? + DEFAULT_MERGE_SETTINGS, resolveRunnerAutoMerge(loadGlobalConfig()), ) ) { @@ -1903,11 +1913,27 @@ export function createTaskManager(opts: CreateTaskManagerOptions): TaskManager { // never instead of it" discipline as the cycle label right above. // Never awaited: a hub round trip must not hold up the ship's own // answer, exactly like the label. - void reportHubTransition(cwd, record, { - type: 'mr_opened', - ...(outcome.mrUrl ? { mr_url: outcome.mrUrl } : {}), - branch: record.branch, - }) + // + // `mr_opened` ONLY when the forge answered with the MR's URL: a push + // whose `gh pr create` failed used to be reported as `mr_opened` + // anyway, and the hub then built on a merge request that did not exist + // (the 2026-08-28 phantom-ticket incident). A state requires its + // proof; without one this reports the failure it actually is. + if (outcome.mrUrl) { + void reportHubTransition(cwd, record, { + type: 'mr_opened', + mr_url: outcome.mrUrl, + branch: record.branch, + }) + } else { + void reportHubTransition(cwd, record, { + type: 'failed', + error_message: + outcome.note ?? + 'branch pushed but no merge request URL came back from the forge: open the MR by hand or retry the ship', + branch: record.branch, + }) + } // T1.9: nothing was ever created for a 'policy' task, so nothing is // attempted for one either — same gate as the runner's abandon path. if (record.isolation === 'container') { @@ -2113,7 +2139,9 @@ export function createTaskManager(opts: CreateTaskManagerOptions): TaskManager { } ctx.merging.add(id) try { - const settings = opts.mergeSettings ?? DEFAULT_MERGE_SETTINGS + const settings = + (opts.getMergeSettings ? opts.getMergeSettings() : opts.mergeSettings) ?? + DEFAULT_MERGE_SETTINGS const run = opts.mergeTaskFn ?? mergeTask let outcome: MergeOutcome try { diff --git a/packages/cli/src/workspace-lifecycle.test.ts b/packages/cli/src/workspace-lifecycle.test.ts index 86f2d72..2de2ef1 100644 --- a/packages/cli/src/workspace-lifecycle.test.ts +++ b/packages/cli/src/workspace-lifecycle.test.ts @@ -1427,6 +1427,18 @@ describe('the unusable merge keys reach the manager too (T3.6, M50)', () => { expect('degradedMergeKeys' in opts).toBe(false) expect(opts.mergeSettings?.policy).toBe('auto') }) + + test('getMergeSettings re-reads the global file: a strategy set after boot is seen', () => { + withGlobalJson({ mergePolicy: 'auto' }) + const opts = workspaceTaskManagerOptions(loadGlobalConfig(), new AbortController(), boot()) + expect(opts.mergeSettings?.strategy).toBeUndefined() + expect(opts.getMergeSettings?.().strategy).toBeUndefined() + // The exact shape of the settings-API bug this getter fixes: mergeStrategy + // written AFTER boot used to stay invisible until the next restart. + withGlobalJson({ mergePolicy: 'auto', mergeStrategy: 'squash' }) + expect(opts.mergeSettings?.strategy).toBeUndefined() + expect(opts.getMergeSettings?.().strategy).toBe('squash') + }) }) // MAJEUR A2 (T1.4 review round 6). `applyRetention()` reads ONE keep count and diff --git a/packages/cli/src/workspace.ts b/packages/cli/src/workspace.ts index 3ec33b3..c2af5f9 100644 --- a/packages/cli/src/workspace.ts +++ b/packages/cli/src/workspace.ts @@ -288,6 +288,11 @@ export function workspaceTaskManagerOptions( ? { taskRetention: config.taskRetentionCount } : {}), mergeSettings: resolveMergeSettings(config), + // Re-read from the global file at the moment a merge decision is made + // (the getChecksConfig pattern): a strategy set through the settings API + // used to stay invisible until the next restart because only the boot + // value above was ever consulted. The boot value stays as the fallback. + getMergeSettings: () => resolveMergeSettings(loadGlobalConfig()), // Named on the task's journal too, not only on the boot line: a user who // typed `mergePolicy: "Auto"` scrolled past the terminal long ago by the // time a task reaches its merge step. diff --git a/packages/contract/package.json b/packages/contract/package.json index 936eceb..b71c3c7 100644 --- a/packages/contract/package.json +++ b/packages/contract/package.json @@ -1,6 +1,6 @@ { "name": "@codesema/contract", - "version": "0.9.0", + "version": "0.10.0", "description": "Shared review contract (types + sanitizers) between the codesema CLI and codesema.com.", "license": "MIT", "author": "Hasan TASKIN", diff --git a/packages/contract/src/arm.test.ts b/packages/contract/src/arm.test.ts index a7de151..9e5b070 100644 --- a/packages/contract/src/arm.test.ts +++ b/packages/contract/src/arm.test.ts @@ -81,8 +81,18 @@ const minimalTransition: ArmTransition = { type: 'mr_opened', idempotency_key: 'tick-1:mr_opened:1', at: '2026-08-14T10:10:00.000Z', + mr_url: 'https://github.com/getCodesema/codesema-cli/pull/7', } +// A state requires its proof: what each type must carry (beyond +// minimalTransition's own fields) for the sanitizer to keep it. +const PROOF_BY_TYPE = { + mr_opened: {}, + review_result: {}, + merged: { merge_sha: 'a1b2c3d4e5' }, + failed: {}, +} as const + const fullTransition: ArmTransition = { type: 'review_result', idempotency_key: 'tick-1:review_result:1', @@ -390,10 +400,24 @@ describe('sanitizeArmTransition', () => { } }) - test('all valid types are kept', () => { + test('all valid types are kept, each carrying its own proof', () => { const types = ['mr_opened', 'review_result', 'merged', 'failed'] as const for (const type of types) { - expect(sanitizeArmTransition({ ...minimalTransition, type })?.type).toBe(type) + expect( + sanitizeArmTransition({ ...minimalTransition, type, ...PROOF_BY_TYPE[type] })?.type, + ).toBe(type) + } + }) + + test('mr_opened without a usable mr_url is refused whole, never degraded', () => { + for (const mr_url of [undefined, '', ' ', 'not a url', 'ftp://example.com/1', 42, null]) { + expect(sanitizeArmTransition({ ...minimalTransition, mr_url })).toBeNull() + } + }) + + test('merged without a usable merge_sha is refused whole, never degraded', () => { + for (const merge_sha of [undefined, '', 'NOT-HEX', 42, null]) { + expect(sanitizeArmTransition({ ...minimalTransition, type: 'merged', merge_sha })).toBeNull() } }) @@ -456,9 +480,9 @@ describe('sanitizeArmTransition', () => { expect(r?.merge_sha).toBe('a1b2c3d') }) - test('mr_url must be an http(s) URL or the field is omitted', () => { + test('mr_url must be an http(s) URL: omitted on a type that needs no proof of it', () => { for (const mr_url of ['not a url', 'ftp://example.com/1']) { - const r = sanitizeArmTransition({ ...minimalTransition, mr_url }) + const r = sanitizeArmTransition({ ...minimalTransition, type: 'failed', mr_url }) expect(r && 'mr_url' in r).toBe(false) } }) @@ -996,8 +1020,11 @@ describe('cross test: sanitizeArmTransition output validates against armTransiti }) test('hostile input, once sanitized, still validates', () => { + // `failed` deliberately: it requires no proof field, so every hostile + // field below degrades to absence and the record survives to be checked + // against the schema (a hostile `merged` is refused whole instead). const hostile = sanitizeArmTransition({ - type: 'merged', + type: 'failed', idempotency_key: 'k'.repeat(500), at: 'x'.repeat(500), mr_iid: 42, @@ -1015,9 +1042,11 @@ describe('cross test: sanitizeArmTransition output validates against armTransiti test('every valid type produces a validating transition', () => { const types = ['mr_opened', 'review_result', 'merged', 'failed'] as const for (const type of types) { - expect(transitionSchemaErrors(sanitizeArmTransition({ ...minimalTransition, type }))).toEqual( - [], - ) + expect( + transitionSchemaErrors( + sanitizeArmTransition({ ...minimalTransition, type, ...PROOF_BY_TYPE[type] }), + ), + ).toEqual([]) } }) }) @@ -1221,9 +1250,11 @@ describe('cross-repo: sanitizeArmTransition output validates against the hub sch test('every valid transition type produces a hub-schema-valid transition', () => { const types = ['mr_opened', 'review_result', 'merged', 'failed'] as const for (const type of types) { - expect(validateTransitionBody(sanitizeArmTransition({ ...minimalTransition, type }))).toBe( - true, - ) + expect( + validateTransitionBody( + sanitizeArmTransition({ ...minimalTransition, type, ...PROOF_BY_TYPE[type] }), + ), + ).toBe(true) } }) }) diff --git a/packages/contract/src/arm.ts b/packages/contract/src/arm.ts index 55b5d91..52f2abe 100644 --- a/packages/contract/src/arm.ts +++ b/packages/contract/src/arm.ts @@ -99,18 +99,22 @@ export type ArmTransitionType = 'mr_opened' | 'review_result' | 'merged' | 'fail /** * One fact the arm reports back to the hub about a ticket it executed. * - * `idempotency_key` is MANDATORY, unlike every other field below: the + * `idempotency_key` is MANDATORY, unlike most other fields: the * hub's report endpoint uses it to tell a retried report from a second, * real transition apart. A transition this sanitizer cannot name one for is * not a degraded transition, it is unsafe to apply, so `sanitizeArmTransition` * refuses the whole record rather than keeping the rest of it. + * + * A state requires its proof (recovery doctrine, rung 0): `mr_opened` is a + * claim that a merge request exists, so it REQUIRES `mr_url`; `merged` is a + * claim that a commit landed, so it REQUIRES `merge_sha`. The discriminated + * union makes a proof-less claim unrepresentable at compile time, and + * `sanitizeArmTransition` refuses one at runtime. */ export type ArmTransition = { - type: ArmTransitionType idempotency_key: string at: string mr_iid?: string - mr_url?: string branch?: string /** * Same literal union as `Verdict` (index.ts), restated rather than @@ -121,10 +125,14 @@ export type ArmTransition = { */ verdict?: 'approve' | 'request_changes' | 'comment' findings_total?: number - merge_sha?: string error_message?: string cost_ticks?: number -} +} & ( + | { type: 'mr_opened'; mr_url: string; merge_sha?: string } + | { type: 'review_result'; mr_url?: string; merge_sha?: string } + | { type: 'merged'; merge_sha: string; mr_url?: string } + | { type: 'failed'; mr_url?: string; merge_sha?: string } +) /** One line of the arm's own execution journal for a ticket run, reported to the hub. */ export type ArmEvent = { @@ -189,7 +197,7 @@ export const ARM_RUN_ID_MAX = 64 export const ARM_EVENT_TYPE_MAX = 100 export const ARM_LABEL_MAX = 500 -const ARM_TICKET_STATUSES: ReadonlySet = new Set([ +export const ARM_TICKET_STATUSES: ReadonlySet = new Set([ 'proposed', 'rejected', 'published', @@ -274,7 +282,7 @@ function isHttpUrl(value: string): boolean { } } -function sanitizeArmSha(raw: unknown): string | undefined { +export function sanitizeArmSha(raw: unknown): string | undefined { if (typeof raw !== 'string') { return undefined } @@ -369,10 +377,13 @@ export function sanitizeArmTicket(raw: unknown): ArmTicket | null { /** * Revalidates an `ArmTransition` before it is sent to, or read back from, - * the hub's report endpoint. Two fields gate the whole record: `type` + * the hub's report endpoint. Two fields gate every record: `type` * (same never-fabricate rule as `ArmTicket.status`) and `idempotency_key`, - * mandatory per this type's own doc comment. Every other field is optional - * and degrades to absence, never to an invented placeholder. + * mandatory per this type's own doc comment. Two more gate their own type, + * because a state requires its proof: an `mr_opened` without a usable + * `mr_url` and a `merged` without a `merge_sha` are refused whole, never + * degraded to a proof-less claim. Every other field is optional and + * degrades to absence, never to an invented placeholder. */ export function sanitizeArmTransition(raw: unknown): ArmTransition | null { if (!raw || typeof raw !== 'object') { @@ -387,26 +398,42 @@ export function sanitizeArmTransition(raw: unknown): ArmTransition | null { return null } const mrIid = str(r.mr_iid, ARM_MR_IID_MAX) - const mrUrl = str(r.mr_url, ARM_MR_URL_MAX) + const rawMrUrl = str(r.mr_url, ARM_MR_URL_MAX) + const mrUrl = rawMrUrl && isHttpUrl(rawMrUrl) ? rawMrUrl : null const branch = str(r.branch, ARM_BRANCH_MAX) const verdict = ARM_VERDICTS.has(r.verdict as ArmVerdict) ? (r.verdict as ArmVerdict) : null const findingsTotal = optionalNonNegativeInt(r.findings_total) const mergeSha = sanitizeArmSha(r.merge_sha) const errorMessage = str(r.error_message, ARM_ERROR_MESSAGE_MAX) const costTicks = optionalNonNegativeInt(r.cost_ticks) - return { - type, + const base = { idempotency_key, at: isoOrNow(r.at), ...(mrIid ? { mr_iid: mrIid } : {}), - ...(mrUrl && isHttpUrl(mrUrl) ? { mr_url: mrUrl } : {}), ...(branch ? { branch } : {}), ...(verdict ? { verdict } : {}), ...(findingsTotal !== null ? { findings_total: findingsTotal } : {}), - ...(mergeSha ? { merge_sha: mergeSha } : {}), ...(errorMessage ? { error_message: errorMessage } : {}), ...(costTicks !== null ? { cost_ticks: costTicks } : {}), } + if (type === 'mr_opened') { + if (!mrUrl) { + return null + } + return { ...base, type, mr_url: mrUrl, ...(mergeSha ? { merge_sha: mergeSha } : {}) } + } + if (type === 'merged') { + if (!mergeSha) { + return null + } + return { ...base, type, merge_sha: mergeSha, ...(mrUrl ? { mr_url: mrUrl } : {}) } + } + return { + ...base, + type, + ...(mrUrl ? { mr_url: mrUrl } : {}), + ...(mergeSha ? { merge_sha: mergeSha } : {}), + } } /** @@ -633,10 +660,11 @@ export const armTicketSchema = { /** * JSON Schema (draft 2020-12) for an `ArmTransition`, same pattern and same - * forward/backward guarantee as `armTicketSchema` above. Every field beyond - * `type`/`idempotency_key`/`at` is optional here exactly as it is on the - * type: `sanitizeArmTransition` omits rather than blanks an unusable one, so - * none of them is in `required`. + * forward/backward guarantee as `armTicketSchema` above. Fields beyond + * `type`/`idempotency_key`/`at` are optional here exactly as they are on the + * type, except the per-type proof fields (`mr_url` on `mr_opened`, + * `merge_sha` on `merged`), required through the `allOf` conditionals below, + * mirroring the discriminated union and `sanitizeArmTransition`'s refusals. */ export const armTransitionSchema = { $schema: 'https://json-schema.org/draft/2020-12/schema', @@ -658,6 +686,18 @@ export const armTransitionSchema = { error_message: { type: 'string', maxLength: ARM_ERROR_MESSAGE_MAX, pattern: NON_BLANK }, cost_ticks: { type: 'integer', minimum: 0, maximum: 9_007_199_254_740_991 }, }, + allOf: [ + { + if: { properties: { type: { const: 'mr_opened' } }, required: ['type'] }, + // oxlint-disable-next-line no-thenable -- JSON Schema conditional keyword, not a thenable + then: { required: ['mr_url'] }, + }, + { + if: { properties: { type: { const: 'merged' } }, required: ['type'] }, + // oxlint-disable-next-line no-thenable -- JSON Schema conditional keyword, not a thenable + then: { required: ['merge_sha'] }, + }, + ], } as const /** diff --git a/packages/contract/src/index.ts b/packages/contract/src/index.ts index 11a3f62..ba8e0a9 100644 --- a/packages/contract/src/index.ts +++ b/packages/contract/src/index.ts @@ -19,6 +19,7 @@ export * from './reasons.js' export * from './recap.js' export * from './runner.js' export * from './tasks.js' +export * from './ticket-state.js' export * from './ticket.js' export type NarrativeConfidence = 'high' | 'medium' | 'low' diff --git a/packages/contract/src/reasons.test.ts b/packages/contract/src/reasons.test.ts index f68d2d2..ad711cd 100644 --- a/packages/contract/src/reasons.test.ts +++ b/packages/contract/src/reasons.test.ts @@ -50,6 +50,7 @@ const EXPECTED_CODES = [ 'branch_diverged', 'checks_unavailable', 'criteria_missing', + 'merge_strategy_unconfigured', 'agent_error', 'inactivity_timeout', 'interrupted_by_user', @@ -73,6 +74,9 @@ const EXPECTED_TERMINAL: Record<(typeof EXPECTED_CODES)[number], boolean> = { // both ask for is a person, so both are terminal. checks_unavailable: true, criteria_missing: true, + // Waiting configures no merge strategy either: the way out is one setting, + // then a retried merge. + merge_strategy_unconfigured: true, } describe('REASON_CODES', () => { @@ -108,20 +112,20 @@ describe('REASON_CODES', () => { } }) - test('today the table is D2 plus T3.6: twelve codes, no more, no less', () => { + test('today the table is D2 plus T3.6 plus the merge-strategy gate: thirteen codes', () => { // The snapshot of the CURRENT roster. Only a deliberate extension touches // this line — never a rename, which the two lock tests above catch first. expect(REASON_CODES.map((entry) => entry.code)).toEqual([...EXPECTED_CODES]) - expect(REASON_CODES).toHaveLength(12) + expect(REASON_CODES).toHaveLength(13) }) test('each code is classified the way this contract documents it', () => { for (const entry of REASON_CODES) { expect(entry.terminal).toBe(EXPECTED_TERMINAL[entry.code]) } - // Seven terminal since T3.6 (D2's five plus `checks_unavailable` and - // `criteria_missing`); the retryable half is untouched at five. - expect(REASON_CODES.filter((entry) => entry.terminal)).toHaveLength(7) + // Eight terminal (D2's five plus `checks_unavailable`, `criteria_missing` + // and `merge_strategy_unconfigured`); the retryable half is untouched at five. + expect(REASON_CODES.filter((entry) => entry.terminal)).toHaveLength(8) expect(REASON_CODES.filter((entry) => !entry.terminal)).toHaveLength(5) }) }) diff --git a/packages/contract/src/reasons.ts b/packages/contract/src/reasons.ts index 2b71dc5..03ee17a 100644 --- a/packages/contract/src/reasons.ts +++ b/packages/contract/src/reasons.ts @@ -34,7 +34,8 @@ export type ReasonCodeEntry = { /** * The ten codes of decision D2, plus the two the automatic merge gate needed - * and could not honestly borrow (T3.6, decisions DP1 and DP2). Order is + * and could not honestly borrow (T3.6, decisions DP1 and DP2), plus the + * merge-strategy gate's own refusal code (recovery doctrine, lot 1). Order is * documentation, not semantics: the terminal ones first, then the retryable * ones. * @@ -116,6 +117,16 @@ export const REASON_CODES = [ code: 'criteria_missing', terminal: true, }, + { + // The automatic merge was refused BEFORE any forge CLI ran because no + // mergeStrategy is configured (recovery doctrine: a consent nobody gave + // is not defaulted, and a blind non-interactive `gh pr merge` on a + // multi-method repo fails with gh's own flag demand). Terminal: waiting + // configures no strategy. The way out is one setting (mergeStrategy via + // `codesema config` or the runner settings API), then a retried merge. + code: 'merge_strategy_unconfigured', + terminal: true, + }, // --- Retryable: the run or its environment has to change ------------------- { // The agent CLI itself failed: crashed, hit its provider's rate limit, diff --git a/packages/contract/src/tasks.test.ts b/packages/contract/src/tasks.test.ts index 3cf4f15..a0d6626 100644 --- a/packages/contract/src/tasks.test.ts +++ b/packages/contract/src/tasks.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test' import { acceptanceCriterionId, + ARM_TICKET_STATUSES, isActiveTaskStatus, isTaskId, isTaskStatus, @@ -966,6 +967,45 @@ describe('sanitizeTaskRecord — hub ticket binding', () => { expect(r?.hub_ticket?.title.length).toBe(TASK_TITLE_MAX) }) + test('hub_ticket_status: a known status round-trips beside its hub_ticket', () => { + const withStatus = { + ...validRecord, + hub_ticket: validHubTicket, + hub_ticket_status: 'mr_opened', + } + expect(sanitizeTaskRecord(structuredClone(withStatus))).toEqual(withStatus as TaskRecord) + }) + + test('hub_ticket_status: every status arm.ts knows round-trips, so the local list cannot lag', () => { + // The runtime set in tasks.ts is spelled locally (importing arm.ts's own + // would cycle); this loop is the lock that keeps the two identical. + for (const status of ARM_TICKET_STATUSES) { + const r = sanitizeTaskRecord({ + ...validRecord, + hub_ticket: validHubTicket, + hub_ticket_status: status, + }) + expect(r?.hub_ticket_status).toBe(status) + } + }) + + test('hub_ticket_status: an unknown value is dropped alone, the ticket itself survives', () => { + for (const junk of ['not-a-status', '', 42, null, {}]) { + const r = sanitizeTaskRecord({ + ...validRecord, + hub_ticket: validHubTicket, + hub_ticket_status: junk, + }) + expect(r?.hub_ticket).toEqual(validHubTicket) + expect(r && 'hub_ticket_status' in r).toBe(false) + } + }) + + test('hub_ticket_status: dropped without a hub_ticket to belong to', () => { + const r = sanitizeTaskRecord({ ...validRecord, hub_ticket_status: 'mr_opened' }) + expect(r && 'hub_ticket_status' in r).toBe(false) + }) + test('hub_ticket: title degrades to an empty string rather than dropping the field', () => { const r = sanitizeTaskRecord({ ...validRecord, diff --git a/packages/contract/src/tasks.ts b/packages/contract/src/tasks.ts index f50fde3..06a9d85 100644 --- a/packages/contract/src/tasks.ts +++ b/packages/contract/src/tasks.ts @@ -3,6 +3,11 @@ // whitelist and truncate, never throw. Everything read back from disk goes // through here before being trusted. +// Type-only, erased at compile time: arm.ts imports this module's runtime +// constants, so a runtime import back into arm.js would cycle. The runtime +// counterpart is the locally spelled TASK_HUB_TICKET_STATUSES below, locked +// to arm.ts's own set by a cross-module test. +import type { ArmTicketStatus } from './arm.js' import { sanitizeReasonCode, sanitizeTaskReason, @@ -607,6 +612,17 @@ export type TaskRecord = { title: string url?: string } + /** + * The hub ticket's last status as the hub itself reported it (claim and + * transition responses both return the ticket), so the runner can refuse + * to report a transition the shared table forbids from that status. + * MUTABLE, unlike `hub_ticket`: it tracks the hub's answer over time. + * OPTIONAL, and absence is the honest default: a record predating this + * field, or one whose hub round trips all failed, knows no hub status; + * the runner-side table guard lets an unknown status pass rather than + * inventing one, and the hub revalidates every report anyway. + */ + hub_ticket_status?: ArmTicketStatus /** * Which half of ship/merge this task is currently inside, when it is * (D20). Written at the start of that step and cleared at its end, success @@ -1020,6 +1036,23 @@ function sanitizeTaskTurn(raw: unknown): TaskTurn | null { * `url` is kept only when it is an http(s) URL, same rule `isHttpUrl` applies * everywhere else in this module. */ +// Restates ArmTicketStatus's members rather than importing arm.ts's own +// ARM_TICKET_STATUSES: that import would be a runtime cycle (arm.ts already +// imports this module's constants). The Set typing refuses +// a stray member; the cross-module equality test in tasks.test.ts catches a +// missing one. +const TASK_HUB_TICKET_STATUSES: ReadonlySet = new Set([ + 'proposed', + 'rejected', + 'published', + 'in_progress', + 'mr_opened', + 'ready_to_merge', + 'done', + 'failed', + 'already_implemented', +]) + function sanitizeHubTicket(raw: unknown): { id: string; title: string; url?: string } | null { if (!raw || typeof raw !== 'object') { return null @@ -1157,6 +1190,12 @@ export function sanitizeTaskRecord(raw: unknown): TaskRecord | null { ...(issue ? { issue } : {}), ...(issueSnapshot ? { issue_snapshot: issueSnapshot } : {}), ...(hubTicket ? { hub_ticket: hubTicket } : {}), + // Unknown or unusable → absent, never fabricated (same rule as + // ArmTicket.status): the runner-side table guard treats absence as + // "status unknown, let the hub decide". + ...(hubTicket && TASK_HUB_TICKET_STATUSES.has(r.hub_ticket_status as ArmTicketStatus) + ? { hub_ticket_status: r.hub_ticket_status as ArmTicketStatus } + : {}), // Optional and whitelisted, same doctrine as `checks_status`: absence is // "not currently shipping or merging", which is also what an unknown or // stale token degrades to rather than being trusted as a step in progress. diff --git a/packages/contract/src/ticket-state.test.ts b/packages/contract/src/ticket-state.test.ts new file mode 100644 index 0000000..499e89d --- /dev/null +++ b/packages/contract/src/ticket-state.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, test } from 'bun:test' +import { ARM_TICKET_STATUSES, type ArmTicketStatus } from './arm.js' +import { + isLegalTicketTransition, + targetTicketStatus, + TICKET_TERMINAL_STATUSES, + TICKET_TRANSITIONS, +} from './ticket-state.js' + +// The same table, spelled a second time BY HAND: a typo in ticket-state.ts +// has to disagree with this list to be caught, which a test derived from the +// table itself could never do. +const EXPECTED_TRANSITIONS: ReadonlyArray<[ArmTicketStatus, ArmTicketStatus]> = [ + ['proposed', 'published'], + ['proposed', 'rejected'], + ['published', 'in_progress'], + ['in_progress', 'published'], + ['in_progress', 'mr_opened'], + ['in_progress', 'already_implemented'], + ['in_progress', 'failed'], + ['in_progress', 'done'], + ['mr_opened', 'mr_opened'], + ['mr_opened', 'ready_to_merge'], + ['mr_opened', 'failed'], + ['mr_opened', 'done'], + ['ready_to_merge', 'mr_opened'], + ['ready_to_merge', 'done'], + ['ready_to_merge', 'failed'], + ['failed', 'in_progress'], + ['failed', 'published'], + ['failed', 'mr_opened'], + ['failed', 'failed'], + ['done', 'mr_opened'], +] + +const key = (from: string, to: string) => `${from}→${to}` + +describe('TICKET_TRANSITIONS', () => { + test('matches the hand-spelled table exactly, no extra and no missing pair', () => { + const actual = new Set(TICKET_TRANSITIONS.map((t) => key(t.from, t.to))) + const expected = new Set(EXPECTED_TRANSITIONS.map(([from, to]) => key(from, to))) + expect(actual).toEqual(expected) + expect(TICKET_TRANSITIONS).toHaveLength(EXPECTED_TRANSITIONS.length) + }) + + for (const [from, to] of EXPECTED_TRANSITIONS) { + test(`${from} → ${to} is legal`, () => { + expect(isLegalTicketTransition(from, to)).toBe(true) + }) + } + + test('every status in the table is a known ticket status', () => { + for (const { from, to } of TICKET_TRANSITIONS) { + expect(ARM_TICKET_STATUSES.has(from)).toBe(true) + expect(ARM_TICKET_STATUSES.has(to)).toBe(true) + } + }) + + test('every non-terminal status has at least one way out', () => { + for (const status of ARM_TICKET_STATUSES) { + if (TICKET_TERMINAL_STATUSES.has(status)) { + continue + } + expect(TICKET_TRANSITIONS.some((t) => t.from === status)).toBe(true) + } + }) + + test('every status is reachable: it appears as a to, or is the creation status', () => { + for (const status of ARM_TICKET_STATUSES) { + if (status === 'proposed') { + continue + } + expect(TICKET_TRANSITIONS.some((t) => t.to === status)).toBe(true) + } + }) +}) + +describe('isLegalTicketTransition', () => { + test.each([ + ['published', 'done'], + ['published', 'mr_opened'], + ['proposed', 'in_progress'], + ['done', 'published'], + ['done', 'done'], + ['failed', 'ready_to_merge'], + ['in_progress', 'ready_to_merge'], + ['ready_to_merge', 'ready_to_merge'], + ] as Array<[ArmTicketStatus, ArmTicketStatus]>)('%s → %s is refused', (from, to) => { + expect(isLegalTicketTransition(from, to)).toBe(false) + }) + + test('a terminal status has no way out at all', () => { + for (const from of TICKET_TERMINAL_STATUSES) { + for (const to of ARM_TICKET_STATUSES) { + expect(isLegalTicketTransition(from, to)).toBe(false) + } + } + }) +}) + +describe('TICKET_TERMINAL_STATUSES', () => { + test('derives to exactly rejected and already_implemented', () => { + expect([...TICKET_TERMINAL_STATUSES].toSorted()).toEqual(['already_implemented', 'rejected']) + }) +}) + +describe('targetTicketStatus', () => { + test('mr_opened claims mr_opened', () => { + expect(targetTicketStatus('mr_opened')).toBe('mr_opened') + }) + + test('merged claims done', () => { + expect(targetTicketStatus('merged')).toBe('done') + }) + + test('failed claims failed', () => { + expect(targetTicketStatus('failed')).toBe('failed') + }) + + test('an approving review claims ready_to_merge', () => { + expect(targetTicketStatus('review_result', 'approve')).toBe('ready_to_merge') + }) + + test('a non-approving review is not a status transition', () => { + expect(targetTicketStatus('review_result', 'request_changes')).toBeNull() + expect(targetTicketStatus('review_result', 'comment')).toBeNull() + expect(targetTicketStatus('review_result')).toBeNull() + }) +}) diff --git a/packages/contract/src/ticket-state.ts b/packages/contract/src/ticket-state.ts new file mode 100644 index 0000000..f8083a2 --- /dev/null +++ b/packages/contract/src/ticket-state.ts @@ -0,0 +1,94 @@ +// The shared ticket state machine (recovery doctrine, rule 3): the legal +// transitions live HERE, and both sides refuse one that is not in the table — +// the hub before writing `tickets.status`, the runner before reporting a +// transition. A refused transition is an explicit error, never a silently +// accepted phantom state. + +import { ARM_TICKET_STATUSES, type ArmTicketStatus, type ArmTransitionType } from './arm.js' + +export type TicketTransition = { from: ArmTicketStatus; to: ArmTicketStatus } + +/** + * Every legal `tickets.status` transition, transcribed from the hub's real + * write sites (each row names its writers). Creation (∅ → `proposed`) is an + * insert, not a transition, so it is not listed. `proposed → rejected` has no + * writer yet: it is product law (a human rejecting a proposal), declared so + * the day the dashboard ships it the table does not refuse it. + * + * The `mr_opened → mr_opened` self-loop is a real fact, not a replay: the + * arm re-ships after a `request_changes` verdict and re-reports `mr_opened` + * under a new idempotency key. No other self-loop is legal — a + * `review_result` that does not approve changes NO status and therefore is + * not a transition at all (see `targetTicketStatus`). + */ +export const TICKET_TRANSITIONS: readonly TicketTransition[] = [ + { from: 'proposed', to: 'published' }, + { from: 'proposed', to: 'rejected' }, + { from: 'published', to: 'in_progress' }, + { from: 'in_progress', to: 'published' }, + { from: 'in_progress', to: 'mr_opened' }, + { from: 'in_progress', to: 'already_implemented' }, + { from: 'in_progress', to: 'failed' }, + { from: 'in_progress', to: 'done' }, + { from: 'mr_opened', to: 'mr_opened' }, + { from: 'mr_opened', to: 'ready_to_merge' }, + { from: 'mr_opened', to: 'failed' }, + { from: 'mr_opened', to: 'done' }, + { from: 'ready_to_merge', to: 'mr_opened' }, + { from: 'ready_to_merge', to: 'done' }, + { from: 'ready_to_merge', to: 'failed' }, + { from: 'failed', to: 'in_progress' }, + { from: 'failed', to: 'published' }, + // A human replying on the runner machine resumes a failed task WITHOUT a + // hub republish: the next ship re-reports mr_opened, and a run that fails + // again re-reports failed with the fresher error. Refusing either would + // strand the hub on a stale failed while the merge request moves on. + { from: 'failed', to: 'mr_opened' }, + { from: 'failed', to: 'failed' }, + { from: 'done', to: 'mr_opened' }, +] + +const TRANSITIONS_BY_FROM: ReadonlyMap> = (() => { + const byFrom = new Map>() + for (const { from, to } of TICKET_TRANSITIONS) { + const set = byFrom.get(from) ?? new Set() + set.add(to) + byFrom.set(from, set) + } + return byFrom +})() + +export function isLegalTicketTransition(from: ArmTicketStatus, to: ArmTicketStatus): boolean { + return TRANSITIONS_BY_FROM.get(from)?.has(to) ?? false +} + +/** + * Derived, never hand-written: a status is terminal exactly when the table + * gives it no way out. Hand-listing them beside the table would be a second + * spelling of the same fact, free to drift. + */ +export const TICKET_TERMINAL_STATUSES: ReadonlySet = new Set( + [...ARM_TICKET_STATUSES].filter((status) => !TRANSITIONS_BY_FROM.has(status)), +) + +/** + * The status an `ArmTransition` type claims to move the ticket to, or `null` + * when the report is not a status transition at all: a `review_result` whose + * verdict is not `approve` is journaled by the hub without touching + * `tickets.status`, so there is nothing to validate against the table. + */ +export function targetTicketStatus( + type: ArmTransitionType, + verdict?: 'approve' | 'request_changes' | 'comment', +): ArmTicketStatus | null { + if (type === 'mr_opened') { + return 'mr_opened' + } + if (type === 'merged') { + return 'done' + } + if (type === 'failed') { + return 'failed' + } + return verdict === 'approve' ? 'ready_to_merge' : null +}