diff --git a/agents/prompts/roles/documenter.md b/agents/prompts/roles/documenter.md index 807ce3d0..d801ea49 100644 --- a/agents/prompts/roles/documenter.md +++ b/agents/prompts/roles/documenter.md @@ -22,6 +22,7 @@ You do NOT re-implement the developer's work. You do NOT review or critique the | `commit(message)` | Commits doc changes on the task branch (auto-prefixed `[task-id]`). | Task in `in_progress`; on the task branch. | | `pr_update(task_id, title?, body?, reviewers?)` | Update the PR's title, body, or reviewer list (e.g. to add a doc-relevant summary). At least one field must be set. **Do NOT bash-shim `gh pr edit`** — use this verb. | Task has `pr_number`; you are claimant on the doc task. | | `i_documented(task_id, notes, files)` | Marks docs complete; transitions toward `awaiting_pm_review`. | At least one doc file in `files`; `notes` >= 20 chars. | +| `i_am_blocked(task_id, reason, blocker_type?, what_needed?)` | Record a blocker, escalate to your PM, idle. `blocker_type` ∈ `external`/`internal`/`question`/`dependency`; `what_needed` is a one-sentence concrete unblock request. Use when doc work is genuinely wedged (not a tracing gap — fix those and retry). | Task is yours and active. | | `unclaim(task_id)` | Release this claim back to pending. Use sparingly — your work-in-progress branch survives but the task is unassigned. | Task assigned to you and in claimed/in_progress. | | `resume(task_id)` | Resume a paused task. Transitions paused → in_progress. | Task assigned to you and in paused state. | | `note(text, scope?)` | Journal entry. | None. | @@ -96,4 +97,4 @@ Errors include `error`, `message`, `remediate`, `missing`. Read `remediate` — ### Circuit breaker -When the gateway returns `error: circuit_open`, do NOT retry the verb immediately. The breaker tracks repeated rejections of the same verb (same kind, e.g. `tracing_gap` or `incomplete_input`) within 60 seconds. Read the `remediate` field — it names what was missing across the last N rejections. Fix that one piece (write the missing journal entry, fill the missing field), then retry the verb ONCE. If the breaker fires again, you don't have an `i_am_blocked` verb — `unclaim(task_id)` to release the claim back to pending and `dm(recipient='', text=...)` with the rejection details so the PM knows it's a real wedge, not a transient error. +When the gateway returns `error: circuit_open`, do NOT retry the verb immediately. The breaker tracks repeated rejections of the same verb (same kind, e.g. `tracing_gap` or `incomplete_input`) within 60 seconds. Read the `remediate` field — it names what was missing across the last N rejections. Fix that one piece (write the missing journal entry, fill the missing field), then retry the verb ONCE. If the breaker fires again, `i_am_blocked(task_id, reason='')` to escalate the wedge to your PM (or `unclaim(task_id)` if you'd rather release the claim back to pending) and `dm(recipient='', text=...)` with the rejection details so the PM knows it's a real wedge, not a transient error. diff --git a/agents/prompts/roles/pr_reviewer.md b/agents/prompts/roles/pr_reviewer.md index da8e352a..c2e7079c 100644 --- a/agents/prompts/roles/pr_reviewer.md +++ b/agents/prompts/roles/pr_reviewer.md @@ -23,6 +23,9 @@ The PR is from an outside contributor: its code is **untrusted**. Until a human | `give_me_work()` | Returns an external-PR review task or `idle`. | None. | | `claim_pr_review(task_id)` | Claims the review task and starts it. `pending → claimed → in_progress`. Returns the PR diff inline. | Task is an `external_pr` review task in `pending`. | | `post_pr_review(task_id, body, findings=[...])` | Posts ONE complete change-request and finishes the review. `in_progress → completed`. `body` = a one-paragraph summary; `findings` = the structured list (see step 6) — the GitHub comment is generated from them in the RoboCo format. | Task claimed by you; findings cover every relevant criterion. | +| `claim_gate_review(task_id)` | **In-path gate:** claim an *assembled* cell→root / root→master PR in `awaiting_pr_review` (does NOT transition it — mirrors QA's `claim_review`). Returns the assembled diff + the parent task's acceptance criteria inline. | Task in `awaiting_pr_review`; not already actively claimed by a different reviewer. | +| `pr_pass(task_id, notes)` | **In-path gate:** pass the assembled-PR review; transitions `awaiting_pr_review → awaiting_pm_review` so the PM merges. | Task claimed by you via `claim_gate_review`; `notes` >= 20 chars. | +| `pr_fail(task_id, issues)` | **In-path gate:** fail the assembled-PR review with concrete issues; transitions `awaiting_pr_review → needs_revision`, routed back to the owning dev/cell PM like a QA fail. | Task claimed by you via `claim_gate_review`; each issue references file/line/expected/actual. | | `note(text, scope?)` | Journal entry. Record your reasoning. | None. | | `evidence(task_id)` | Re-fetch the PR diff if you need more detail. | None. | | `roboco_git_diff` / `roboco_git_log` / `roboco_git_status` / `roboco_git_branches` | Read-only git inspection. | None. | @@ -46,6 +49,19 @@ The PR is from an outside contributor: its code is **untrusted**. Until a human - ❌ Being lax on the architectural standard. Be mega-strict: on an in-path gate review, a `block`-level convention violation (a definition in the wrong module per `.roboco/conventions.yml`, a model in a router, a lint/type suppression) is an automatic `pr_fail` — the gate already refuses `pr_pass`, and an introduced or expanded `waiver` must be justified in the diff or rejected. Hold placement and house-style to the same bar as correctness. - ❌ Letting a non-modular assembled change through. The standard also enforces **modularity** (`modular_cohesion`, `thin_routes`, `thin_components`, `god_class`): a file must own one architectural concern (no model in a router, no schema in a component), a route handler must delegate to a service rather than run its own DB access in the route body, a React component must stay presentational with data fetching in a hook, and a class past the method-count threshold must be decomposed. A `block`-level modularity finding refuses `pr_pass` exactly the way it refuses the developer's `i_am_done` — these surface in QA's `claim_review` evidence as `convention_findings`, carry the offending `file:line` + a fix hint, and clear only via a `waiver` committed in the branch. +## In-path gate review (the second surface) + +You have a second, distinct surface: the **in-path PR-review gate**. After a Cell PM's `submit_up` (cell→root PR) or Main PM's `submit_root` (root→master PR), the assembled PR enters `awaiting_pr_review` and the orchestrator dispatches you to gate it before the PM merges. This is internal delivery work, not an external contributor PR — use `claim_gate_review` / `pr_pass` / `pr_fail`, NOT `claim_pr_review` / `post_pr_review` (those are for `external_pr` tasks only). + +1. `give_me_work()` → a task in `awaiting_pr_review`. +2. `claim_gate_review(task_id)` → read the assembled diff + the parent task's acceptance criteria inline. +3. Review the assembled diff against the parent objective + full acceptance criteria + the cross-cell contract, with the same adversarial bar as an external PR (a block-level convention violation — a misplaced definition, a lint/type suppression — is an automatic `pr_fail`; the gate already refuses `pr_pass`). +4. `pr_pass(task_id, notes='<>=20 chars')` to send it on to `awaiting_pm_review` for the PM merge, or `pr_fail(task_id, issues=[...])` to route it back to `needs_revision` (the owning dev/cell PM re-claims and revises — for a Main-PM branch-bearing root, `pr_fail`'s `remediate` tells the Main PM to re-delegate the fixes to the owning cell PM(s) and wait for re-assembly, NOT to re-submit the unchanged root). + +**On a blocked `pr_pass`:** if the toolchain or conventions validator cannot run in your workspace (interpreter mismatch, validator hang), the gate refuses `pr_pass` and its `remediate` points at `pr_fail(issues=['toolchain: ...'])` — your reject lever, since you have no `i_am_blocked` verb. Do NOT chase `i_am_blocked`; send the PR back with `pr_fail` so the dev rebuilds the environment. + +**Single-claimant:** a gate task already actively claimed by a different reviewer returns `invalid_state` ("it may already be claimed; `give_me_work` for the next") — call `give_me_work()` for the next review. A re-claim by the same reviewer is idempotent. + ## When the gateway returns an error Errors include `error`, `message`, `remediate`, `missing`. Read `remediate` — it names the literal next call. Fix that one piece and retry the same verb. diff --git a/agents/prompts/roles/qa.md b/agents/prompts/roles/qa.md index 71589b41..f7470923 100644 --- a/agents/prompts/roles/qa.md +++ b/agents/prompts/roles/qa.md @@ -19,7 +19,8 @@ A pass without evidence is a betrayal of your role: the entire downstream chain | `give_me_work()` | Returns a task in `awaiting_qa` for your team or `idle`. | None. | | `claim_review(task_id)` | Claims the QA task; returns PR data inline. | Task in `awaiting_qa`; you are not the original developer. | | `pass(task_id, notes, ac_verdicts)` | Accepts the work; transitions to `awaiting_documentation`. `ac_verdicts` is one verification entry per acceptance criterion — the gateway **rejects a pass that doesn't cover every criterion**. | Task claimed by you; `notes` >= 80 chars; one `ac_verdicts` entry per criterion; journal `learning` entry recorded. | -| `fail(task_id, issues)` | Rejects with concrete actionable issues; transitions to `needs_revision`. | Task claimed by you; each issue references criterion/file/line. | +| `fail(task_id, issues)` | Rejects with concrete actionable issues; transitions to `needs_revision`, **routed back to the original dev (never the pool)** so they re-claim and revise. | Task claimed by you; each issue references criterion/file/line. | +| `i_am_blocked(task_id, reason, blocker_type?, what_needed?)` | Record a blocker, escalate to your PM, idle. `blocker_type` ∈ `external`/`internal`/`question`/`dependency`; `what_needed` is a one-sentence concrete unblock request. Use when a review is genuinely wedged (not a tracing gap — fix those and retry). | Task is yours and active. | | `unclaim(task_id)` | Release this claim back to pending. Use sparingly — your work-in-progress branch survives but the task is unassigned. | Task assigned to you and in claimed/in_progress. | | `resume(task_id)` | Resume a paused task. Transitions paused → in_progress. | Task assigned to you and in paused state. | | `note(text, scope?)` | Journal entry. Required: `scope='learning'` before `pass`/`fail`. | None. | @@ -101,4 +102,4 @@ Errors include `error`, `message`, `remediate`, `missing`. Read `remediate` — ### Circuit breaker -When the gateway returns `error: circuit_open`, do NOT retry the verb immediately. The breaker tracks repeated rejections of the same verb (same kind, e.g. `tracing_gap` or `incomplete_input`) within 60 seconds. Read the `remediate` field — it names what was missing across the last N rejections. Fix that one piece (write the missing journal entry, fill the missing field), then retry the verb ONCE. If the breaker fires again, you don't have an `i_am_blocked` verb — `unclaim(task_id)` to release the claim back to pending and `dm(recipient='', text=...)` with the rejection details so the PM knows it's a real wedge, not a transient error. +When the gateway returns `error: circuit_open`, do NOT retry the verb immediately. The breaker tracks repeated rejections of the same verb (same kind, e.g. `tracing_gap` or `incomplete_input`) within 60 seconds. Read the `remediate` field — it names what was missing across the last N rejections. Fix that one piece (write the missing journal entry, fill the missing field), then retry the verb ONCE. If the breaker fires again, `i_am_blocked(task_id, reason='')` to escalate the wedge to your PM (or `unclaim(task_id)` if you'd rather release the claim back to pending) and `dm(recipient='', text=...)` with the rejection details so the PM knows it's a real wedge, not a transient error. diff --git a/docs/api/auth.md b/docs/api/auth.md index 28f9c873..ec1d490e 100644 --- a/docs/api/auth.md +++ b/docs/api/auth.md @@ -39,14 +39,16 @@ Set `ROBOCO_AGENT_AUTH_REQUIRED=true` to require a signed token on every REST re !!! tip "How the panel authenticates as the CEO" The control panel acts as the CEO agent. In secure mode, nginx injects the panel's CEO `X-Agent-Token` so your browser session is authenticated without you handling the secret — you just use the panel as normal. -## The WebSocket caveat +## The WebSocket + live-chat streams -Token enforcement is **REST-only**. The [WebSocket streams](./websockets.md) do not check the HMAC token: +Secure mode extends beyond REST. When `ROBOCO_AGENT_AUTH_REQUIRED=true`: -- The per-resource sockets (`/ws/channels|agents|sessions|notifications/{id}`) validate their `agent_id`/`viewer_id` query param against the database and channel access, but not a token. -- `/ws/system` is fully unauthenticated. +- The **per-resource WebSocket streams** (`/ws/channels|agents|sessions|notifications/{id}`) require the **CEO panel token** — the same signed `X-Agent-Token` nginx injects for the panel. An agent on the Docker network can no longer subscribe to another agent's notifications with no auth. They still validate `agent_id`/`viewer_id` against the DB and channel access on top. +- The **`/api/v1/do/*` content routes** require a valid per-agent HMAC token bound to `X-Agent-ID` (the do router serves every role, so the gate is token-only, not role-specific). +- The **live-chat bridges** (`/prompter/live/*`, `/secretary/live/*`) — the prompter/secretary intake chats — require the CEO panel token on their start/stream/status/messages/stop endpoints. They were the last panel-facing API surface that ran unauthenticated. +- **`/ws/system`** stays operator-only and read-only by design (it carries system telemetry and accepts nothing from the client); it is not token-gated. -The streams are read-only and carry no control surface or secrets, so this isn't a privilege-escalation path the way the REST headers are — but it does mean the orchestrator port should stay trusted-network-only until WebSocket auth lands, even when you've enabled secure-mode REST. +A presented-but-forged token is rejected even in dev (header-trust) mode, so you can roll out tokens before flipping the switch without breaking anything. The container→relay internal callback is left ungated by design (internal Docker network, opaque session id). ## What to do diff --git a/docs/api/websockets.md b/docs/api/websockets.md index 775b0477..7b35b5d3 100644 --- a/docs/api/websockets.md +++ b/docs/api/websockets.md @@ -8,16 +8,16 @@ There are four per-resource streams plus one operator-wide stream: | Endpoint | Stream | Auth | |----------|--------|------| -| `/ws/channels/{channel_id}` | Live messages in a team channel | `agent_id` query param, validated against the DB + channel access | -| `/ws/agents/{agent_id}` | An agent's output and lifecycle events | `viewer_id`/`agent_id` query param, validated against the DB | -| `/ws/sessions/{session_id}` | Messages in a communication session | `agent_id` query param, validated | -| `/ws/notifications/{agent_id}` | An agent's notifications | `agent_id` query param, validated | -| `/ws/system` | Operator/system-wide stream — no per-agent keying | **Unauthenticated, read-only** | +| `/ws/channels/{channel_id}` | Live messages in a team channel | `agent_id` query param, validated against the DB + channel access; **CEO panel token required in secure mode** | +| `/ws/agents/{agent_id}` | An agent's output and lifecycle events | `viewer_id`/`agent_id` query param, validated against the DB; **CEO panel token required in secure mode** | +| `/ws/sessions/{session_id}` | Messages in a communication session | `agent_id` query param, validated; **CEO panel token required in secure mode** | +| `/ws/notifications/{agent_id}` | An agent's notifications | `agent_id` query param, validated; **CEO panel token required in secure mode** | +| `/ws/system` | Operator/system-wide stream — no per-agent keying | **Unauthenticated, read-only** (operator-only by design; not token-gated) | All sockets support a `ping`/`pong` keepalive: send `{"type": "ping"}` and you'll get a `pong` back. -!!! warning "WebSocket auth is not the REST auth" - The per-resource sockets validate their `agent_id`/`viewer_id` query param against the database (and channel access via the permissions layer), but they do **not** enforce the HMAC `X-Agent-Token` that secure-mode REST requires — token enforcement is REST-only. `/ws/system` is intentionally fully unauthenticated. None of the streams carry a control surface or secrets, so they're read-only by design, but the orchestrator port should be treated as trusted-network-only until WebSocket auth lands. See [Authentication](./auth.md) and [Security](../troubleshooting/security.md). +!!! info "Secure mode now covers the per-agent streams" + When `ROBOCO_AGENT_AUTH_REQUIRED=true`, the four per-resource sockets require the **CEO panel token** (the signed `X-Agent-Token` nginx injects for the panel) on top of their `agent_id`/`viewer_id` DB validation — an agent on the Docker network can no longer subscribe to another agent's stream unauthenticated. `/ws/system` is intentionally left operator-only and read-only. A forged token is rejected even in dev mode. See [Authentication](./auth.md). ## How events reach the sockets diff --git a/docs/company/agent-gateway.md b/docs/company/agent-gateway.md index 5dd9ecda..fd06e32c 100644 --- a/docs/company/agent-gateway.md +++ b/docs/company/agent-gateway.md @@ -17,8 +17,8 @@ Two more read-only servers give agents a read-only view of git (`status`, `log`, At spawn, every agent is handed a **manifest** listing exactly the verbs its role may call — and nothing else. The manifest is built from a server-side role configuration and mounted read-only into the container. The result is that the lifecycle's role rules aren't just policy, they're *unreachable code* for the wrong role: -- A **developer** can `give_me_work`, open a PR, and mark itself done — but there is no merge verb in its manifest. -- **QA** can claim a review and pass or fail it — but it has no `commit`. +- A **developer** can `give_me_work`, open a PR, mark itself done, and `sync_branch` (rebase its branch onto its base through the gate) — but there is no merge verb in its manifest. +- **QA** can claim a review and pass or fail it — but it has no `commit`. QA and Documenters also get `i_am_blocked` as their escape hatch when they're stuck. - A **PR reviewer** can pass or fail an assembled PR and post its review on the PR — but it never gets agent chat verbs. - The **Auditor** is restricted to leaving a private note and reading evidence; it cannot `say` or `dm`. It observes; it does not participate. @@ -38,6 +38,9 @@ That `next` / `remediate` contract is why agents move through the lifecycle reli A few more protections run by construction, the same way on every backend (Claude or Grok): - **Claim-locking** serializes work, so two agents can't grab the same task or race a merge. +- **Content posts require an active claim.** `commit`, `note`, `say`, `dm`, and `evidence` on a specific task are refused unless the agent holds that task's active claim — an agent can't write to a task it hasn't locked. +- **Human-only roles are never spawned.** The CEO, the Intake (prompter), and the Secretary are human-driven, so `spawn_agent` structurally refuses them — a notification addressed to the CEO can never launch a CEO container that acts as the human. Intake and Secretary run through their own dedicated, guarded chat paths instead. +- **Notifications can't target human-only roles.** `notify` rejects the CEO/prompter/secretary as recipients — there is no agent acknowledgement path for them, so a notification to them is a no-op rather than a stuck ack. - **The token never enters the container.** Your GitHub PAT is injected only for the moment of a git operation, orchestrator-side, and scrubbed from every clone — see [Register a project](../get-started/first-project.md#what-happens-under-the-hood). - **A prompt-injection guard** screens task prompts, and a bash guard blocks credential-exfiltration and identity-forgery patterns. - **Rate limits and overloads park, they don't crash-loop.** If a provider returns a 429 or a persistent overload, RoboCo *queues* that agent's work and probes for recovery instead of burning tokens retrying. You'll see an amber banner; the work resumes automatically when the provider does. diff --git a/docs/company/megatask.md b/docs/company/megatask.md index d220f1c8..e318b125 100644 --- a/docs/company/megatask.md +++ b/docs/company/megatask.md @@ -25,6 +25,8 @@ For each task it proposes, the agent declares a small **collision surface**: whi The waves are just ordinary task dependencies, so the same dependency-gate that already paces the rest of the company runs them: a wave starts only once the previous wave's tasks have reached a terminal state — normally each one's pull request is merged (a cancelled task releases the next wave too). +The same collision-aware sequencing follows the work **down the chain**, not just at the top level. When a cell PM delegates a root-subtask into developer tasks, the dev-task collision surfaces flow through the same DAG — file-overlap serializes, migration-adders chain, shared-surface edits wait their turn — and cell tasks themselves wave-chain off their sibling root-subtasks. So a batch that spans a shared codebase stays ordered all the way to the leaves, not only at the umbrella. The task hierarchy is capped at four layers (umbrella → root → cell → dev) to fit this MegaTask shape. + ## What gets created When you confirm, RoboCo creates one **umbrella** task that groups the batch, and one **root-subtask** per piece of work: diff --git a/docs/company/merge-model.md b/docs/company/merge-model.md index 1e6d5842..fa3b039d 100644 --- a/docs/company/merge-model.md +++ b/docs/company/merge-model.md @@ -40,6 +40,15 @@ graph BT Each of those assembled pull requests passes through the [in-path PR-review gate](task-lifecycle.md#the-in-path-pr-review-gate) before its PM merges it. +## Submit gates that keep the chain clean + +Two gate-level checks stop a stale branch from sneaking through: + +- **Behind-base gate on `i_am_done`.** If a sibling's PR merged into the parent branch while the developer worked, the dev's branch is now behind its base and the assembled PR won't merge cleanly. The gate refuses `i_am_done` in that state and steers the developer to `sync_branch` — the gate-level rebase verb that rebases the branch onto its base (raw shell git is denied to agents, so the rebase goes through the gate, traced and evidenced). Conflicts abort with no force-push and point the dev at resolve-by-hand. The gate fails open on a flaky fetch so a transient git error can't strand a task at the submit gate. +- **Unchanged-PR gate on `submit_root`.** When a Main-PM root PR is `pr_fail`'d and re-submitted byte-identical, the loop would repeat forever. The gate refuses the re-submit when the assembled root PR's head SHA is unchanged since the last `pr_fail` (no new cell work → identical diff); a different SHA means the branch advanced and the submit proceeds. Every ambiguous case fails open. + +PR operations are also **scoped per project** — `open_pr`, `pr_target`, `close_pull_request`, and `merge_pr` all require the project and resolve the PR number within it, so two tasks in different repos that happen to share a PR number can never collide and merge the wrong repository's PR. + ## Only the CEO merges to master The final pull request — root → master — is the one place the company stops and hands the decision back to you. It lands in your **CEO Approval Queue** and waits. diff --git a/docs/company/task-lifecycle.md b/docs/company/task-lifecycle.md index ee0f255d..c62d95b0 100644 --- a/docs/company/task-lifecycle.md +++ b/docs/company/task-lifecycle.md @@ -54,6 +54,8 @@ stateDiagram-v2 Rejection isn't a dead end — it's a loop. When **QA fails** a task, or a **PR reviewer rejects** an assembled pull request, the task drops back to `needs_revision`, the developer reworks it, and it re-enters the flow. The same is true when *you* request changes from the CEO Approval Queue. Nothing is lost; the task carries its history, branch, and pull request with it the whole way around. +A failed developer task is routed back to **the developer who worked it** (resolved from the work session), not the pool — so the revision lands with whoever has the context, rather than being re-claimed cold by a cell PM. Only a task no developer ever touched falls back to the pool. + ## The in-path PR-review gate Most leaf developer tasks are reviewed by QA and never need a separate PR review. But when work is **assembled and pushed up the chain as a pull request**, it stops for a dedicated review before any PM merges it: @@ -81,6 +83,7 @@ Transitions aren't suggestions; they're enforced. A handful of the rules: - **`pr_pass` / `pr_fail`** are PR-reviewer-only. - **Merging** (`awaiting_pm_review → completed`) is PM-only; **escalating to the CEO** and the final **approve / request-changes / cancel** are CEO-only. - **Cancelling** is PM-only. +- **A Main-PM coordination root can never be `task_type=code`.** The Main PM coordinates; it doesn't write code itself, so the combination is rejected at creation — a structural guard, not a hint. How those role boundaries are enforced — and why a developer literally cannot call the merge verb — is the subject of [How agents are sandboxed](agent-gateway.md). diff --git a/docs/models/grok.md b/docs/models/grok.md index 8559396b..a03d9a24 100644 --- a/docs/models/grok.md +++ b/docs/models/grok.md @@ -28,7 +28,7 @@ The Grok access token has a fixed ~6-hour, server-set lifetime, and the CLI can' - As a backstop, each agent's entrypoint runs `python -m roboco.llm.providers.grok_auth --check` and **refuses to start** on a missing or expired token instead of hanging. !!! warning "The orchestrator's `~/.grok` mount must be writable" - The orchestrator rewrites `auth.json` when it refreshes the token, so the orchestrator's own mount of `~/.grok` must be **read-write**. (The per-agent mount stays read-only — agents only read the credential.) If the orchestrator can't write it, the token will expire and Grok agents will fail their start-up `--check`. + The orchestrator rewrites `auth.json` when it refreshes the token, so the orchestrator's own mount of `~/.grok` must be **read-write**. (The per-agent mount stays read-only — agents only read the credential.) If the orchestrator can't write it, the token will expire and Grok agents will fail their start-up `--check`. If the host `auth.json` is missing entirely at spawn time, the orchestrator logs a loud warning — a missing credential is the most common Grok misconfiguration, so it's surfaced early rather than as a fleet of failed starts. ## Per-fleet tuning diff --git a/docs/models/resilience.md b/docs/models/resilience.md index 37ab3c65..0e979ae0 100644 --- a/docs/models/resilience.md +++ b/docs/models/resilience.md @@ -38,6 +38,9 @@ The crucial property: **work is queued, never dropped.** Parked tasks wait; the !!! tip "Parked is not stuck" If a run goes quiet, check the banner before assuming something broke. A parked provider with a counting-down timer is RoboCo waiting out a rate limit on purpose. The work is held and will resume — there's nothing for you to do. +!!! info "Escape hatch for a probe that never recovers" + Park-and-probe assumes the provider comes back. If a provider's probe fails persistently (the secret was rotated, the endpoint moved), an escape hatch releases the parked work back to the pool instead of holding it forever — so a permanently-dead provider doesn't strand its tasks. Grok auth-missing (exit 78) is parked the same way rather than crash-retried straight back into the same missing-credential failure. + ## Disk housekeeping: dangling-image prune Every agent-image rebuild leaves the previous build behind as a dangling (``) Docker image. Left alone they pile up and eat disk. The orchestrator's background sweep prunes them on a throttle (~6h): it removes **only** dangling images — a tagged image, or one still backing a running container, is never touched. It is gated by `ROBOCO_IMAGE_PRUNE_ENABLED`, which is **on by default**. This isn't a feature flag you opt into; it's an always-on safety net you can disable if you'd rather manage image cleanup yourself. diff --git a/docs/optional/conventions.md b/docs/optional/conventions.md index 0b434da7..185be886 100644 --- a/docs/optional/conventions.md +++ b/docs/optional/conventions.md @@ -14,7 +14,7 @@ The rules live in a per-project `.roboco/conventions.yml` with four curated part |------|-----------| | **Module map** | Path prefixes mapped to a human purpose and the definition *kinds* forbidden there (`model`, `route`, `helper`, `business_logic`, `component`). "`routers/` is for HTTP routes — no models, no helpers." | | **Rules** | A toggleable rule set. Each rule fires at `warn` (advisory, never blocks) or `block` (refuses the gate). | -| **Custom rules** | Project-specific regex rules — a pattern, a message, and a level, optionally scoped to languages. | +| **Custom rules** | Project-specific regex rules — a pattern, a message, and a level, optionally scoped to languages. TypeScript-scoped custom rules apply to both `.ts` and `.tsx` files. | | **Waivers** | Accountable per-`(path, rule)` escape hatches with a written reason — the sanctioned way to relieve a false positive, reviewed in the PR. | ### Placement, hygiene, and modularity checks @@ -34,7 +34,7 @@ The validator runs four check families over each changed file: | `god_class` | A class grows past 15 methods (single-responsibility smell) | `warn` | !!! info "Precision over recall" - Every check fires only on a confident, structural signal, and abstains when it is uncertain — so a `block`-level gate is never tripped by a guess. If the validator genuinely *cannot* run on a diff (a parse or grammar error), it is **fail-loud**: it exits non-zero and the gate blocks rather than passing silently. + Every check fires only on a confident, structural signal, and abstains when it is uncertain — so a `block`-level gate is never tripped by a guess. If the validator genuinely *cannot* run on a diff (a parse or grammar error), it is **fail-loud**: it exits non-zero and the gate blocks rather than passing silently. The validator is also **time-bounded** — a hung run (a tree-sitter deadlock, an enormous repo) is killed after 120s and treated as `could_not_run`, so a stuck subprocess can't hang the `i_am_done` / `pr_pass` gate forever or orphan a process on restart. And if the *effective map itself* can't be resolved (a conventions-service error), the gate **fails closed** rather than silently disabling the standard for that task. ## The effective map: defaults, present, absent, or partial diff --git a/docs/panel/tasks-and-kanban.md b/docs/panel/tasks-and-kanban.md index 37b942c4..c0a16b0b 100644 --- a/docs/panel/tasks-and-kanban.md +++ b/docs/panel/tasks-and-kanban.md @@ -52,7 +52,7 @@ A swim-lane board of the delivery pipeline, switched with the `?view=` query par | **PR Review** | `pr-review` | assembled PRs at the in-path review gate | | **PM** | `pm` | tasks awaiting PM review and merge | -Each board is a read-at-a-glance view of where work sits in the [lifecycle](../company/task-lifecycle.md). Switching tabs updates the URL, so a specific board is shareable. +Each board is a read-at-a-glance view of where work sits in the [lifecycle](../company/task-lifecycle.md). Switching tabs updates the URL, so a specific board is shareable. A drag that would skip a lifecycle precondition (moving a task past a gate it hasn't passed) opens a confirmation dialog first, so an accidental drop can't silently bypass the flow. ## Next diff --git a/docs/rag/roles/cell-pm.md b/docs/rag/roles/cell-pm.md index 6a0d5462..dc635d21 100644 --- a/docs/rag/roles/cell-pm.md +++ b/docs/rag/roles/cell-pm.md @@ -150,12 +150,28 @@ notify(target="be-dev-1", text="Please prioritise task X by EOD.", When every subtask of your cell-scoped parent is terminal (each leaf PR merged into your cell branch via `complete`), call `submit_up(task_id, notes)`. This opens the **cell→root PR** and moves the parent into the in-path PR-review gate (`awaiting_pr_review`), where your cell's **PR reviewer** reviews the assembled diff: - `pr_pass` → the parent moves to `awaiting_pm_review`; you then `complete(task_id, notes)` to merge the cell→root PR into the root branch. -- `pr_fail` → the parent returns to `needs_revision` (owned by you) with the reviewer's issues; fix, then re-`submit_up`. +- `pr_fail` → the parent returns to `needs_revision` (owned by you) with the reviewer's issues; fix, then re-`submit_up`. The reviewer's verdict + issues are carried in your task handoff, so you are not blind on the rework. + +Re-`submit_up` is refused if the assembled PR is **unchanged** since the last `pr_fail` (no new commits on it) — it stops a re-submit-the-same-PR loop. Fix the issues and commit before re-submitting. You merge your own cell→root PR — the Main PM does **not** merge your cell branch. The Main PM owns the **root** task: once every cell's parent is terminal, it runs the same gate one level up (`submit_root` → main reviewer → escalate to CEO) and only the CEO merges to `master`. You never open or merge a master PR yourself. `submit_up` is for finished work entering the merge gate; `escalate_up` (below) is for *help* you need while work is still in flight. +### Sequencing dev-task collisions + +When you `delegate` a dev subtask you may pass the collision surface so the sequencing DAG orders siblings that touch the same files: + +```python +delegate(parent_task_id=..., ..., + intends_to_touch=["roboco/api/routes/*.py"], # file globs + adds_migration=False, # adds a DB migration + touches_shared=True, # edits a shared module + depends_on=[""]) # explicit ordering +``` + +Siblings whose `intends_to_touch` globs overlap are serialized (more-important first); migration-adders chain serially; a shared-surface edit runs after each non-shared task it overlaps. Omit these and only the weak assignee-keyed spawn barrier orders your dev tasks (the 2026-06-27 out-of-order break). + ## Escalating to Main PM Use `escalate_up(task_id, reason)` when: diff --git a/docs/rag/roles/main-pm.md b/docs/rag/roles/main-pm.md index 9d2f55e7..322dd4b8 100644 --- a/docs/rag/roles/main-pm.md +++ b/docs/rag/roles/main-pm.md @@ -111,7 +111,7 @@ master ← feature/main_pm/{root} ← feature/{cell}/{root}/{cell-pm} ← ``` - A cell PM's `complete` merges a leaf PR into its cell branch; after the cell gate, its `complete` merges the cell→root PR into your root branch. You do not merge cell branches. -- Once every cell's parent is terminal, **`submit_root(root_task_id, notes)`** opens the root→master PR and enters the in-path gate (`awaiting_pr_review`). The **main PR reviewer** checks the assembled root diff: `pr_pass` → `awaiting_pm_review`; `pr_fail` → `needs_revision` (owned by you, fix + re-`submit_root`). +- Once every cell's parent is terminal, **`submit_root(root_task_id, notes)`** opens the root→master PR and enters the in-path gate (`awaiting_pr_review`). The **main PR reviewer** checks the assembled root diff: `pr_pass` → `awaiting_pm_review`; `pr_fail` → `needs_revision` (owned by you, fix + re-`submit_root`). The reviewer's verdict + issues are carried in your task handoff, and re-`submit_root` is refused if the root PR is **unchanged** since the last `pr_fail` — fix and commit before re-submitting. - After `pr_pass`, `complete(root_task_id, notes)` escalates the root to the CEO (`awaiting_ceo_approval`) — it does **not** merge. A branchless coordination root (product fan-out, no repo) skips the gate and `complete` escalates directly. - The CEO approves and merges the root→master PR from the panel. Only the CEO ever merges to `master`. diff --git a/docs/rag/roles/pr-reviewer.md b/docs/rag/roles/pr-reviewer.md index 031c9624..9a126991 100644 --- a/docs/rag/roles/pr-reviewer.md +++ b/docs/rag/roles/pr-reviewer.md @@ -21,7 +21,9 @@ The `pr_reviewer` role also runs the **in-path gate** on the org's OWN assembled ### Gate enforcement -When the architectural-conventions standard is enabled, `pr_pass` is refused on any block-level convention finding, the same way the developer's `i_am_done` is. When toolchain matching is enabled, `pr_pass` is likewise refused on a "broken" toolchain status. Your verdict note is a mandatory structured field (`pr_reviewer_notes`) written at `pr_pass` / `pr_fail`; it is persisted structured with a derived text mirror. +When the architectural-conventions standard is enabled, `pr_pass` is refused on any block-level convention finding, the same way the developer's `i_am_done` is — the remediation hint points you at the offending `file:line` + the `pr_fail` verb (not `i_am_blocked`). When toolchain matching is enabled, `pr_pass` is likewise refused on a "broken" toolchain status. Your verdict note is a mandatory structured field (`pr_reviewer_notes`) written at `pr_pass` / `pr_fail`; it is persisted structured with a derived text mirror. + +You cannot `pr_pass` / `pr_fail` an assembled PR you authored (self-review guard, same shape as QA's). A `claim_gate_review` on your own work returns `not_authorized`. ## What You CAN Do diff --git a/docs/rag/roles/qa.md b/docs/rag/roles/qa.md index 4653d46d..66475f15 100644 --- a/docs/rag/roles/qa.md +++ b/docs/rag/roles/qa.md @@ -22,6 +22,7 @@ - Read-only inspect git via `roboco_git_status / _log / _diff / _branch_list` - Search the knowledge base via `roboco_ask_mentor` / `roboco_kb_search` - Note evidence via `note(text=..., scope="...")` and `evidence(...)` +- Block your own review on an external dependency via `i_am_blocked(task_id, reason="...")` (Cell PM unblocks) ## What You CANNOT Do @@ -41,6 +42,8 @@ claim_review(task_id) → claim for review pass(task_id, notes) → moves to awaiting_documentation fail(task_id, issues=[...]) → moves to needs_revision; the dev's original assignee gets it back +i_am_blocked(task_id, reason=...) → external blocker (broken env, can't + reproduce); Cell PM unblocks unclaim(task_id) / resume(task_id) / i_am_idle() ``` @@ -48,7 +51,7 @@ unclaim(task_id) / resume(task_id) / i_am_idle() | MCP server | Verbs you can call | |-----------------------|--------------------| -| `roboco-flow` | `give_me_work`, `claim_review`, `pass`, `fail`, `unclaim`, `resume`, `i_am_idle` | +| `roboco-flow` | `give_me_work`, `claim_review`, `pass`, `fail`, `i_am_blocked`, `unclaim`, `resume`, `i_am_idle` | | `roboco-do` | `note`, `say`, `dm`, `evidence` (no `commit`, no `notify`) | | `roboco-git-readonly` | `roboco_git_status`, `roboco_git_log`, `roboco_git_diff`, `roboco_git_branch_list` | | `roboco-optimal` | `roboco_ask_mentor`, `roboco_kb_search` | @@ -118,4 +121,4 @@ dm(recipient="be-pm", task_id="...") ``` -If the situation is unresolvable from the QA side (e.g. test environment broken, can't reproduce), `fail(task_id, issues)` with the full context is the right move; the Cell PM will pick it up from `needs_revision`. +For an external blocker (test environment broken, can't reproduce, missing infra), use `i_am_blocked(task_id, reason="...")` — your Cell PM is notified and `unblock`s you. If the work itself is wrong, `fail(task_id, issues)` with the full context is the right move; the Cell PM picks it up from `needs_revision`. diff --git a/docs/rag/standards/conventions.md b/docs/rag/standards/conventions.md index 808414e1..fe5f3d71 100644 --- a/docs/rag/standards/conventions.md +++ b/docs/rag/standards/conventions.md @@ -54,7 +54,7 @@ A single Python CLI classifies every changed definition with tree-sitter (Python python -m roboco.conventions check --root --files ... ``` -It favours precision over recall — it abstains when it cannot classify a definition, so a `block` gate can never strand a task on a guess — and it fails loud: a validator that cannot run exits non-zero so the gate blocks rather than silently passing. +It favours precision over recall — it abstains when it cannot classify a definition, so a `block` gate can never strand a task on a guess — and it fails loud: a validator that cannot run exits non-zero so the gate blocks rather than silently passing. A **hung** validator is reaped after a timeout and the gate blocks the same way, and a conventions **resolution error** (effective-map build failure) fails closed — the gate is never silently disabled by an upstream error. ## Modularity diff --git a/docs/rag/tools/messaging-tools.md b/docs/rag/tools/messaging-tools.md index f74197d9..a2d52349 100644 --- a/docs/rag/tools/messaging-tools.md +++ b/docs/rag/tools/messaging-tools.md @@ -18,6 +18,8 @@ Don't invent channel slugs. Call `channels()` first if unsure: channels() # -> {"writable": [...], "readable": [...]} ``` +**Active-claim required (explicit `task_id`):** when you pass an explicit `task_id`, `say` / `dm` / `note` check that you are the task's **active claimant** — not just `assigned_to`, which goes stale across a reap/handoff. A reaped or reassigned agent can no longer post to a former task; if you see `not_authorized` on a content post, re-`claim` the task first (or drop the explicit `task_id` for a general channel post). + Valid slugs: cell channels (`backend-cell`, `frontend-cell`, `uxui-cell`); cross-cell (`dev-all`, `qa-all`, `pm-all`, `doc-all`); management (`main-pm-board`, `board-private`); broadcast (`announcements`, `all-hands`). ## Direct message (A2A) — `dm` @@ -40,6 +42,8 @@ notify(target="be-dev-1", text="Task ready for you", priority="normal", task_id= `priority` is `normal | high | urgent`. `task_id` auto-injects from the active task when omitted. +`notify` rejects **human-only recipients** (`prompter`, `secretary`) — they have no agent ack path, so an ack-required alert to them would sit unacked forever. The CEO is allowed (acks via the panel). + ## Receiving notifications Every role with an inbox gets these (so `i_am_idle()` doesn't soft-block on unread items): diff --git a/docs/rag/workflows/escalation.md b/docs/rag/workflows/escalation.md index 618867ed..480dfd70 100644 --- a/docs/rag/workflows/escalation.md +++ b/docs/rag/workflows/escalation.md @@ -29,6 +29,8 @@ escalate_up( Auto-routes to your escalation target (you cannot choose it). +`escalate_up` is refused on a **terminal** task (`completed` / `cancelled`) — it returns `invalid_state` rather than resurrecting a finished task. Escalate live work only. + ## When to Escalate | Situation | Escalate To | diff --git a/docs/rag/workflows/megatask.md b/docs/rag/workflows/megatask.md index 31f03aad..ab014ca8 100644 --- a/docs/rag/workflows/megatask.md +++ b/docs/rag/workflows/megatask.md @@ -14,7 +14,7 @@ Hierarchy: Umbrella (Main PM) → Root-subtasks (Main PM) → Cell tasks (cell P - The umbrella does **no git**. It is exempt from the branch gate (it reaches `in_progress` with no branch) and you must **not** call `submit_root` on it — it assembles no PR. Each root-subtask opens and is reviewed on its own PR. - The umbrella **completes** only when every root-subtask is terminal; then it escalates to the CEO (PR requirement waived). - The root-subtasks are sequenced: a wave's tasks dispatch only once the previous wave's tasks reach a terminal state (ordinary dependency-gating). You do not reorder them — the analyzer set the order at create time. -- On the Board route the root-subtasks are held in `backlog` until the CEO approves the umbrella, then released to `pending`. On the Approve & Start route they start immediately. +- On the Board route the root-subtasks are held in `backlog` until the CEO approves the umbrella, then released to `pending`. On the Approve & Start route they start immediately. On Board-route activation a `code`-typed root-subtask is **retyped to `planning`** — a Main PM never owns a `code` task (the `main_pm + code` combo is the 2026-06-27 meltdown trigger). ## For the Main PM diff --git a/docs/rag/workflows/task-claiming.md b/docs/rag/workflows/task-claiming.md index a3645e36..aa011590 100644 --- a/docs/rag/workflows/task-claiming.md +++ b/docs/rag/workflows/task-claiming.md @@ -41,7 +41,7 @@ The claim verb both claims and starts the task — there is no separate `start` ## Claiming Rules -- **One at a time (workers only)**: Developers, QA, and documenters can't hold multiple in-progress tasks at once. **PM coordinators are exempt** — a Main / Cell PM plans and delegates many roots in parallel, so it may hold several at once; only a real upstream **sequence dependency** (an unfinished task it depends on) holds one of its roots back. +- **One at a time (workers only)**: Developers, QA, and documenters can't hold multiple in-progress tasks at once. A **blocked** task still counts as active — a blocked dev cannot `claim` a second task; unblock or `unclaim` first. **PM coordinators are exempt** — a Main / Cell PM plans and delegates many roots in parallel, so it may hold several at once; only a real upstream **sequence dependency** (an unfinished task it depends on) holds one of its roots back. - **Self-review prevention**: QA cannot `claim_review` tasks they developed - **Self-documentation prevention**: Documenter cannot claim tasks they developed - **Branch requirement**: Branch auto-created on `i_will_work_on` diff --git a/docs/troubleshooting/security.md b/docs/troubleshooting/security.md index 8a4ca5eb..0ba6d27a 100644 --- a/docs/troubleshooting/security.md +++ b/docs/troubleshooting/security.md @@ -45,9 +45,16 @@ Project GitHub tokens are **encrypted the moment you save them** (with `ROBOCO_E This is the guarantee that makes it safe to hand RoboCo a private repo: **your GitHub PAT is never present inside an agent container.** The orchestrator decrypts the token only at the moment of a git operation, injects it for that operation, and immediately after cloning **scrubs the token out of the clone's git config** — then verifies no token byte survives anywhere under `.git/`, destroying the workspace if one did. A compromised or misbehaving agent has nothing to exfiltrate, because the credential was never on its disk. The clone scrub is described in [Register a project](../get-started/first-project.md#what-happens-under-the-hood), and the broader sandboxing model in [the gateway](../company/agent-gateway.md). -## WebSocket auth caveat +## WebSocket + live-chat auth -The per-resource WebSocket streams are keyed to a resource, but the operator stream is not authenticated. **`/ws/system` carries no per-agent keying and no token** even when `ROBOCO_AGENT_AUTH_REQUIRED=true` — secure mode does not extend to it. It is **read-only** (it carries system events like rate-limit lifecycle and usage snapshots; it accepts nothing from the client), so the exposure is limited to a reader seeing system telemetry. It is, however, one more reason the system must sit on a trusted network: anyone who can open that socket can watch the operator stream. +Secure mode (`ROBOCO_AGENT_AUTH_REQUIRED=true`) extends to the live streams and the content routes, not just REST: + +- The **per-resource WebSocket streams** (`/ws/channels|agents|sessions|notifications/{id}`) require the **CEO panel token** — the signed `X-Agent-Token` nginx injects for the panel — on top of their `agent_id`/`viewer_id` DB validation. An agent on the Docker network can no longer subscribe to another agent's notifications with no auth. +- The **`/api/v1/do/*` content routes** require a per-agent HMAC token bound to `X-Agent-ID`. +- The **live-chat bridges** (`/prompter/live/*`, `/secretary/live/*`) require the CEO panel token — they were the last panel-facing surface that ran unauthenticated. +- **`/ws/system`** is the one exception: it stays operator-only and read-only by design (system telemetry, no client input), and is not token-gated. It is one more reason the system must sit on a trusted network: anyone who can open that socket can watch the operator stream. + +A presented-but-forged token is rejected even in header-trust (dev) mode, so rolling out tokens before flipping the switch breaks nothing. ## Next diff --git a/roboco/api/routes/a2a.py b/roboco/api/routes/a2a.py index ae5ab111..97168be4 100644 --- a/roboco/api/routes/a2a.py +++ b/roboco/api/routes/a2a.py @@ -297,11 +297,10 @@ async def subscribe_to_task( Opens a persistent connection that streams task state changes until the task reaches a terminal state or client disconnects. - F024: each poll opens a SHORT-LIVED session via ``get_session_factory`` - and closes it before the next ``asyncio.sleep`` — never holding one - asyncpg connection across the full SSE lifetime (up to 1 hour / 720 - polls), which previously exhausted the pool one connection per connected - client. The route takes no ``db: DbSession`` for the same reason. + Each poll opens a SHORT-LIVED session via ``get_session_factory`` and + closes it before the next ``asyncio.sleep`` — never holding one asyncpg + connection across the full SSE lifetime (up to 1 hour / 720 polls). The + route takes no ``db: DbSession`` for the same reason. """ session_factory = get_session_factory() @@ -324,9 +323,9 @@ async def subscribe_to_task( if await request.is_disconnected(): break - # F024: refresh task state from a per-poll session that is - # released before the sleep below — never held across the poll - # interval, so the asyncpg pool is free between queries. + # Refresh task state from a per-poll session released before the + # sleep — never held across the poll interval, so the asyncpg pool + # is free between queries. async with session_factory() as session: task = await A2AService(session).get_task(task_id) if task is None: diff --git a/roboco/api/routes/tasks.py b/roboco/api/routes/tasks.py index cea1fe6e..37b8042c 100644 --- a/roboco/api/routes/tasks.py +++ b/roboco/api/routes/tasks.py @@ -2024,11 +2024,10 @@ async def escalate_task( raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Task not found" ) - # F043: a terminal task (completed / cancelled) must not be resurrected to - # BLOCKED via escalation. Refuse BEFORE sending the escalation notification - # so a finished/cancelled task isn't yanked back into the workflow (and the - # PM isn't pinged about a task that's already done). The single write - # primitive apply_escalation guards this too — defense in depth. + # A terminal task (completed / cancelled) must not be resurrected to BLOCKED + # via escalation — refuse BEFORE sending the notification so a finished task + # isn't yanked back into the workflow. apply_escalation guards this too + # (defense in depth). if task.status in (TaskStatus.COMPLETED, TaskStatus.CANCELLED): raise HTTPException( status_code=status.HTTP_409_CONFLICT, diff --git a/roboco/api/routes/v1/_role_dep.py b/roboco/api/routes/v1/_role_dep.py index faa013ea..c9f7bc8f 100644 --- a/roboco/api/routes/v1/_role_dep.py +++ b/roboco/api/routes/v1/_role_dep.py @@ -62,7 +62,7 @@ require_pr_reviewer = _require_roles(frozenset({Role.PR_REVIEWER})) def _require_authenticated_agent() -> params.Depends: - """Token-only guard for the content-tool (do) router (F003/F014). + """Token-only guard for the content-tool (do) router. The do router serves every role — content tools are role-uniform, with per-role removal handled in the spawn manifest — so, unlike the flow diff --git a/roboco/api/routes/v1/flow_doc.py b/roboco/api/routes/v1/flow_doc.py index 7e723ba8..ad2eb622 100644 --- a/roboco/api/routes/v1/flow_doc.py +++ b/roboco/api/routes/v1/flow_doc.py @@ -104,9 +104,8 @@ async def i_am_blocked( x_agent_id: _AgentIdHeader, choreographer: _ChoreographerDep, ) -> dict: - """F015: the documenter manifest registers ``i_am_blocked`` — surface the - route so a blocked documenter's escape hatch returns an envelope instead of - a 404.""" + """Surface the ``i_am_blocked`` route so a blocked documenter's escape + hatch returns an envelope instead of a 404.""" env = await choreographer.i_am_blocked( x_agent_id, body.task_id, diff --git a/roboco/api/routes/v1/flow_qa.py b/roboco/api/routes/v1/flow_qa.py index b4e00341..5f1264d7 100644 --- a/roboco/api/routes/v1/flow_qa.py +++ b/roboco/api/routes/v1/flow_qa.py @@ -116,8 +116,8 @@ async def i_am_blocked( x_agent_id: _AgentIdHeader, choreographer: _ChoreographerDep, ) -> dict: - """F015: the QA manifest registers ``i_am_blocked`` — surface the route so a - blocked QA agent's escape hatch returns an envelope instead of a 404.""" + """Surface the ``i_am_blocked`` route so a blocked QA agent's escape hatch + returns an envelope instead of a 404.""" env = await choreographer.i_am_blocked( x_agent_id, body.task_id, diff --git a/roboco/api/websocket.py b/roboco/api/websocket.py index 36fea98f..8e5641c8 100644 --- a/roboco/api/websocket.py +++ b/roboco/api/websocket.py @@ -29,25 +29,21 @@ from roboco.services.repositories import resolve_agent_uuid router = APIRouter() log = structlog.get_logger() -# F066: server-side idle timeout for WS receive loops. A half-open socket -# (dead agent container, silent client) blocks ``receive_text()`` forever; -# wrapping it in ``asyncio.wait_for`` reaps the socket after this many -# seconds of silence. No env-var/config precedent exists in ``config.py`` -# for WS tuning, so this is a module constant — callers/tests patch it. +# Server-side idle timeout for WS receive loops. A half-open socket (dead +# agent container, silent client) blocks ``receive_text()`` forever; +# ``asyncio.wait_for`` reaps the socket after this many seconds of silence. IDLE_TIMEOUT_SECONDS: float = 90.0 -# F064: per-connection send queue + send timeout. Each registered connection -# owns a bounded ``asyncio.Queue`` drained by a sender task, so a slow client -# can't back-pressure the fan-out: broadcast enqueues (non-blocking) and -# returns immediately. When the queue is full the message is dropped + logged -# (the client is lagging, not the whole fan-out). ``send_text`` itself is -# wrapped in ``wait_for`` so a stuck transport doesn't wedge the sender. +# Per-connection send queue + send timeout. Each registered connection owns a +# bounded ``asyncio.Queue`` drained by a sender task, so a slow client can't +# back-pressure the fan-out: broadcast enqueues (non-blocking) and returns +# immediately; a full queue drops + logs (client lagging, not the fan-out). MAX_SEND_QUEUE: int = 256 SEND_TIMEOUT_SECONDS: float = 10.0 class _ClientConnection: - """F064: per-connection send queue + sender task. + """Per-connection send queue + sender task. Holds the bounded outbound queue drained by ``sender``; broadcast enqueues here instead of awaiting ``send_text`` directly, so one slow client cannot @@ -63,20 +59,13 @@ class _ClientConnection: async def _require_panel_token(websocket: WebSocket) -> bool: - """F004: bind a per-agent WS upgrade to the panel/CEO HMAC token. + """Bind a per-agent WS upgrade to the panel/CEO HMAC token. - The /ws/* streams are operator-only — the control panel is the sole WS - client (agents use MCP verbs, not WS), and nginx injects the CEO panel - token as ``X-Agent-Token`` on /ws/ upgrades. Without verifying it the - per-agent endpoints (channels/agents/sessions/notifications) accepted a - bare ``agent_id`` query param with no auth, so in strict mode - (``ROBOCO_AGENT_AUTH_REQUIRED=true``) an agent on the Docker network - could hit e.g. ``/ws/notifications/{id}`` directly and subscribe to - another agent's notifications. This gate requires + verifies the token - against the CEO identity in strict mode, and rejects a presented-but- - forged token even in dev mode — the same contract as the HTTP - ``_check_agent_auth_token`` role gates. Returns True to proceed, False - to close with a policy violation (caller closes the socket). + /ws/* streams are operator-only (the panel is the sole WS client; agents + use MCP verbs). nginx injects the CEO panel token as ``X-Agent-Token``. + In strict mode (``ROBOCO_AGENT_AUTH_REQUIRED=true``) the token is required + + verified against the CEO identity; a presented-but-forged token is + rejected even in dev mode. Returns True to proceed, False to close. """ token = websocket.headers.get("x-agent-token") if _auth_required() and not token: @@ -119,15 +108,15 @@ class ConnectionManager: # websocket -> agent_id (for tracking who is connected) self.connection_agents: dict[WebSocket, UUID] = {} - # F064: websocket -> per-connection send queue + sender task. Every - # connect_* registers here; disconnect cancels + removes. Broadcast - # enqueues into these queues instead of awaiting send_text directly so - # one slow client can't block the fan-out. + # websocket -> per-connection send queue + sender task. Every connect_* + # registers here; disconnect cancels + removes. Broadcast enqueues into + # these queues instead of awaiting send_text directly so one slow client + # can't block the fan-out. self.connection_senders: dict[WebSocket, _ClientConnection] = {} - # F064: fire-and-forget fallback send tasks for unregistered sockets - # (legacy path). Held only to satisfy ruff RUF006 + to allow clean - # shutdown; each task removes itself on completion. + # Fire-and-forget fallback send tasks for unregistered sockets (legacy + # path). Held to satisfy ruff RUF006 + allow clean shutdown; each task + # removes itself on completion. self._pending_sends: set[asyncio.Task[None]] = set() def _register_sender(self, websocket: WebSocket) -> _ClientConnection: @@ -246,21 +235,19 @@ class ConnectionManager: # Remove from tracking self.connection_agents.pop(websocket, None) - # F064: cancel + drop the per-connection sender task so a slow/stale - # client's queue doesn't leak after the socket is removed. + # Cancel + drop the per-connection sender task so a slow/stale client's + # queue doesn't leak after the socket is removed. conn = self.connection_senders.pop(websocket, None) if conn is not None and conn.sender is not None: conn.sender.cancel() def _enqueue_or_send(self, websocket: WebSocket, data: str) -> None: - """F064: fan out one message to one connection without blocking. + """Fan out one message to one connection without blocking. - Registered connections (created via ``connect_*``) get the message - enqueued into their bounded send queue — non-blocking, drop + warn on - overflow. An unregistered socket (legacy path: present in a - subscription set but not in ``connection_senders``) falls back to a - timeout-bounded ``send_text`` scheduled on the loop, so the broadcast - still never blocks on a single slow client. + Registered connections get the message enqueued into their bounded send + queue (non-blocking, drop + warn on overflow). An unregistered socket + falls back to a timeout-bounded ``send_text`` scheduled on the loop, so + the broadcast never blocks on a single slow client. """ conn = self.connection_senders.get(websocket) if conn is not None: @@ -380,7 +367,7 @@ async def channel_stream( Clients receive real-time messages for the channel. """ - # F004: verify the panel/CEO token before any subject lookup. + # Verify the panel/CEO token before any subject lookup. if not await _require_panel_token(websocket): await websocket.close(code=status.WS_1008_POLICY_VIOLATION) return @@ -428,10 +415,9 @@ async def channel_stream( # exit path (anyio closed-resource, CancelledError, transport errors). pass except TimeoutError: - # F066: idle timeout — the client has been silent for - # IDLE_TIMEOUT_SECONDS (likely a half-open socket from a dead - # container). Log and fall through to the finally so the socket is - # removed from every subscription set. + # Idle timeout — the client has been silent for IDLE_TIMEOUT_SECONDS + # (likely a half-open socket from a dead container). Fall through to + # the finally so the socket is removed from every subscription set. log.warning( "WebSocket idle timeout — disconnecting", timeout=IDLE_TIMEOUT_SECONDS ) @@ -449,7 +435,7 @@ async def agent_stream( Clients receive real-time LLM output from the agent. """ - # F004: verify the panel/CEO token before any subject lookup. + # Verify the panel/CEO token before any subject lookup. if not await _require_panel_token(websocket): await websocket.close(code=status.WS_1008_POLICY_VIOLATION) return @@ -494,10 +480,9 @@ async def agent_stream( # exit path (anyio closed-resource, CancelledError, transport errors). pass except TimeoutError: - # F066: idle timeout — the client has been silent for - # IDLE_TIMEOUT_SECONDS (likely a half-open socket from a dead - # container). Log and fall through to the finally so the socket is - # removed from every subscription set. + # Idle timeout — the client has been silent for IDLE_TIMEOUT_SECONDS + # (likely a half-open socket from a dead container). Fall through to + # the finally so the socket is removed from every subscription set. log.warning( "WebSocket idle timeout — disconnecting", timeout=IDLE_TIMEOUT_SECONDS ) @@ -515,7 +500,7 @@ async def session_stream( Clients receive real-time messages for a specific session. """ - # F004: verify the panel/CEO token before any subject lookup. + # Verify the panel/CEO token before any subject lookup. if not await _require_panel_token(websocket): await websocket.close(code=status.WS_1008_POLICY_VIOLATION) return @@ -558,10 +543,9 @@ async def session_stream( # exit path (anyio closed-resource, CancelledError, transport errors). pass except TimeoutError: - # F066: idle timeout — the client has been silent for - # IDLE_TIMEOUT_SECONDS (likely a half-open socket from a dead - # container). Log and fall through to the finally so the socket is - # removed from every subscription set. + # Idle timeout — the client has been silent for IDLE_TIMEOUT_SECONDS + # (likely a half-open socket from a dead container). Fall through to + # the finally so the socket is removed from every subscription set. log.warning( "WebSocket idle timeout — disconnecting", timeout=IDLE_TIMEOUT_SECONDS ) @@ -579,7 +563,7 @@ async def notification_stream( Agents receive real-time notifications via this stream. """ - # F004: verify the panel/CEO token before any subject lookup. + # Verify the panel/CEO token before any subject lookup. if not await _require_panel_token(websocket): await websocket.close(code=status.WS_1008_POLICY_VIOLATION) return @@ -611,10 +595,9 @@ async def notification_stream( # exit path (anyio closed-resource, CancelledError, transport errors). pass except TimeoutError: - # F066: idle timeout — the client has been silent for - # IDLE_TIMEOUT_SECONDS (likely a half-open socket from a dead - # container). Log and fall through to the finally so the socket is - # removed from every subscription set. + # Idle timeout — the client has been silent for IDLE_TIMEOUT_SECONDS + # (likely a half-open socket from a dead container). Fall through to + # the finally so the socket is removed from every subscription set. log.warning( "WebSocket idle timeout — disconnecting", timeout=IDLE_TIMEOUT_SECONDS ) @@ -649,10 +632,9 @@ async def system_stream(websocket: WebSocket) -> None: # exit path (anyio closed-resource, CancelledError, transport errors). pass except TimeoutError: - # F066: idle timeout — the client has been silent for - # IDLE_TIMEOUT_SECONDS (likely a half-open socket from a dead - # container). Log and fall through to the finally so the socket is - # removed from every subscription set. + # Idle timeout — the client has been silent for IDLE_TIMEOUT_SECONDS + # (likely a half-open socket from a dead container). Fall through to + # the finally so the socket is removed from every subscription set. log.warning( "WebSocket idle timeout — disconnecting", timeout=IDLE_TIMEOUT_SECONDS ) diff --git a/roboco/llm/providers/grok.py b/roboco/llm/providers/grok.py index db203654..7844a4e9 100644 --- a/roboco/llm/providers/grok.py +++ b/roboco/llm/providers/grok.py @@ -56,15 +56,11 @@ GROK_AUTH_HOST_PATH = os.environ.get("ROBOCO_HOST_GROK_DIR", str(Path.home() / " # In-container paths. _MCP_CONFIG_IN_CONTAINER = "/app/mcp-config.json" -# F005: the host ~/.grok DIRECTORY is mounted read-only here (NOT the single -# auth.json file). A single-file bind mount pins the inode, so the -# orchestrator's atomic auth.json refresh (tmp+rename within the dir) never -# reached a running container — a long-lived grok container hung at the login -# prompt when the original ~6h token expired. A directory mount sees the -# rename, so the refreshed token propagates to running containers. The -# entrypoint symlinks ~/.grok/auth.json -> this RO mount so grok (and the -# --check backstop) read the live credential while grok's own writable state -# (config.toml, sessions/) still lands in the image's ~/.grok. +# The host ~/.grok DIRECTORY (not a single auth.json file) is mounted RO here: +# a single-file bind mount pins the inode so the orchestrator's atomic +# tmp+rename refresh never reaches a running container. The entrypoint +# symlinks ~/.grok/auth.json -> this RO mount; grok's writable state lives in +# the image's ~/.grok. _GROK_AUTH_DIR_IN_CONTAINER = "/home/agent/.grok-auth-ro" # Per-agent data dir (the host side is reused from the shared assembly): the # entrypoint writes the captured token usage here so the orchestrator reads it @@ -166,18 +162,16 @@ class GrokCliProvider(AgentProvider): def _append_grok_auth_mount(cmd: list[str]) -> None: """Mount the host's SuperGrok ``~/.grok`` directory (read-only). - F005: the mount is the DIRECTORY, not the single ``auth.json`` file. - A single-file bind mount pins the inode, so when the orchestrator + The mount is the DIRECTORY, not the single ``auth.json`` file: a + single-file bind mount pins the inode, so when the orchestrator atomically refreshes the token (``tmp.replace`` = rename within the host ``~/.grok``), a running container kept reading the stale inode and hung at grok's login prompt once the original ~6h token expired. A directory bind mount sees the rename, so the refreshed ``auth.json`` propagates to running containers. The entrypoint symlinks - ``~/.grok/auth.json`` at this RO directory mount, so grok (and the - ``--check`` backstop) read the live credential while grok's own - writable state (``config.toml``, ``sessions/``) still lands in the - image's ``~/.grok``. Read-only so concurrent containers can't corrupt - the shared subscription credential. + ``~/.grok/auth.json`` at this RO directory mount; grok's own writable + state (``config.toml``, ``sessions/``) lands in the image's ``~/.grok``. + Read-only so concurrent containers can't corrupt the shared credential. """ auth_dir = Path(GROK_AUTH_HOST_PATH) if (auth_dir / "auth.json").exists(): diff --git a/roboco/mcp/do_server.py b/roboco/mcp/do_server.py index 29ee20c7..d0711527 100644 --- a/roboco/mcp/do_server.py +++ b/roboco/mcp/do_server.py @@ -38,7 +38,7 @@ _SDK_TIMEOUT = 2.0 # FastAPI's default missing-route status. Every /api/v1/do/* route returns # 200 with an Envelope (including not_found rejections), so a 404 from the # orchestrator is always a manifest-registered tool whose HTTP route is -# missing — F069 synthesizes an invalid_state Envelope for it. +# missing — synthesize an invalid_state Envelope for it. _MISSING_ROUTE_STATUS = 404 # Envelope error kinds that count toward the per-verb circuit breaker. @@ -54,8 +54,8 @@ _CIRCUIT_REJECTION_KINDS: frozenset[str] = frozenset( # Dict-shaped `error.code` values (from FastAPI's exception handlers — # `roboco_exception_handler` / `http_exception_handler` / `generic_exception_handler`) -# mapped to the counted breaker kind they are semantically equivalent to. F068: -# a 422 / 500 / 4xx-exception storm is retry-storm-worthy but the response body +# mapped to the counted breaker kind they are semantically equivalent to. A +# 422 / 500 / 4xx-exception storm is retry-storm-worthy but the response body # carries `error` as a DICT (not a string kind), so the breaker's string-only # check skipped it — unbounded retries. We classify by `error.code` so the SDK # actually records the attempt. Kinds not in `_CIRCUIT_REJECTION_KINDS` are @@ -88,7 +88,7 @@ def _classify_rejection(payload: dict[str, Any]) -> str | None: The breaker only counts rejections whose kind is in ``_CIRCUIT_REJECTION_KINDS`` (the SDK's authoritative catalog). Three reachable rejection shapes must all map to a counted kind so a storm of - any of them trips the breaker (F068): + any of them trips the breaker: 1. Envelope rejection: ``error`` is a STRING kind. Forward it if in the counted set (existing behaviour). Uncounted string kinds (e.g. @@ -156,18 +156,14 @@ def _post(path: str, body: dict[str, Any]) -> dict[str, Any]: headers=_build_headers(), json=body, ) - # F069: a 404 here means a manifest-registered content tool has no - # matching route on the orchestrator (every /api/v1/do/* route - # returns 200 with an Envelope — including not_found rejections — so - # a 404 status with FastAPI's default body (``{"detail": "Not - # Found"}``, no ``error`` field) is always a missing route, never a - # legit Envelope). That body is a non-envelope payload the breaker - # can't classify, so a storm of these bypassed the circuit breaker → - # unbounded retries on a tool that can never succeed. Synthesize an - # ``invalid_state`` Envelope rejection so the breaker counts it (via - # ``_classify_rejection``) and the agent gets a remediation hint - # instead of a raw ``detail`` body. A 404 that DOES carry a real - # Envelope (an ``error`` field) is surfaced as-is. Mirrors + # A 404 here means a manifest-registered content tool has no matching + # route on the orchestrator: every /api/v1/do/* route returns 200 with + # an Envelope (including not_found rejections), so FastAPI's default + # 404 body (no ``error`` field) is always a missing route, never a legit + # Envelope. Synthesize an ``invalid_state`` Envelope so the breaker + # counts it (via ``_classify_rejection``) and the agent gets a + # remediation hint instead of a raw ``detail`` body. A 404 carrying a + # real Envelope (``error`` field) is surfaced as-is. Mirrors # flow_server._post. if response.status_code == _MISSING_ROUTE_STATUS: try: @@ -244,11 +240,11 @@ def _record_and_check_circuit( # Gateway envelopes use a string `error` (kind); RobocoError-derived # exceptions surface a dict-shaped error via FastAPI's middleware, and # 422 validation failures carry a `detail` list with no `error` field - # at all. F068: classify all three rejection shapes so a storm of 500s - # or 422s counts toward the breaker (previously bypassed → unbounded - # retries). The dict-shape defence against `TypeError: unhashable type: - # 'dict'` lives in `_classify_rejection` (isinstance checks, never a - # `dict in frozenset` membership test). + # at all. Classify all three rejection shapes so a storm of 500s or 422s + # counts toward the breaker (previously bypassed → unbounded retries). The + # dict-shape defence against `TypeError: unhashable type: 'dict'` lives in + # `_classify_rejection` (isinstance checks, never a `dict in frozenset` + # membership test). rejection_kind = _classify_rejection(payload) if rejection_kind is None: return payload diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index 943e3f02..48942d88 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -56,7 +56,7 @@ _SDK_TIMEOUT = 2.0 # FastAPI's default missing-route status. Every gateway route returns 200 # with an Envelope (including not_found rejections), so a 404 from the # orchestrator is always a manifest-registered verb whose HTTP route is -# missing — F069 synthesizes an invalid_state Envelope for it. +# missing — synthesize an invalid_state Envelope for it. _MISSING_ROUTE_STATUS = 404 # Envelope error kinds that count toward the per-verb circuit breaker. @@ -70,8 +70,8 @@ _CIRCUIT_REJECTION_KINDS: frozenset[str] = frozenset( # Dict-shaped `error.code` values (from FastAPI's exception handlers — # `roboco_exception_handler` / `http_exception_handler` / `generic_exception_handler`) -# mapped to the counted breaker kind they are semantically equivalent to. F068: -# a 422 / 500 / 4xx-exception storm is retry-storm-worthy but the response body +# mapped to the counted breaker kind they are semantically equivalent to. A +# 422 / 500 / 4xx-exception storm is retry-storm-worthy but the response body # carries `error` as a DICT (not a string kind), so the breaker's string-only # check skipped it — unbounded retries. We classify by `error.code` so the SDK # actually records the attempt. Kinds not in `_CIRCUIT_REJECTION_KINDS` are @@ -104,7 +104,7 @@ def _classify_rejection(payload: dict[str, Any]) -> str | None: The breaker only counts rejections whose kind is in ``_CIRCUIT_REJECTION_KINDS`` (the SDK's authoritative catalog). Three reachable rejection shapes must all map to a counted kind so a storm of - any of them trips the breaker (F068): + any of them trips the breaker: 1. Envelope rejection: ``error`` is a STRING kind. Forward it if in the counted set (existing behaviour). Uncounted string kinds (e.g. @@ -176,19 +176,15 @@ def _post(path: str, body: dict[str, Any]) -> dict[str, Any]: headers=_build_headers(), json=body, ) - # F069: a 404 here means a manifest-registered verb has no matching - # route on the orchestrator (every gateway route returns 200 with an - # Envelope — including not_found rejections — so a 404 status with - # FastAPI's default body (``{"detail": "Not Found"}``, no ``error`` - # field) is always a missing route, never a legit Envelope). That - # body is a non-envelope payload the breaker can't classify, so a - # storm of these bypassed the circuit breaker → unbounded retries on - # a verb that can never succeed. Synthesize an ``invalid_state`` - # Envelope rejection so the breaker counts it (via - # ``_classify_rejection``) and the agent gets a remediation hint - # instead of a raw ``detail`` body. A 404 that DOES carry a real - # Envelope (an ``error`` field — e.g. a proxy re-status a 200 - # rejection to 404) is surfaced as-is. + # A 404 here means a manifest-registered verb has no matching route on + # the orchestrator: every gateway route returns 200 with an Envelope + # (including not_found rejections), so FastAPI's default 404 body (no + # ``error`` field) is always a missing route, never a legit Envelope. + # Synthesize an ``invalid_state`` Envelope so the breaker counts it + # (via ``_classify_rejection``) and the agent gets a remediation hint + # instead of a raw ``detail`` body. A 404 carrying a real Envelope (an + # ``error`` field — e.g. a proxy re-status a 200 rejection to 404) is + # surfaced as-is. if response.status_code == _MISSING_ROUTE_STATUS: try: body_404 = response.json() @@ -270,11 +266,11 @@ def _record_and_check_circuit( # Gateway envelopes use a string `error` (kind); RobocoError-derived # exceptions surface a dict-shaped error via FastAPI's middleware, and # 422 validation failures carry a `detail` list with no `error` field - # at all. F068: classify all three rejection shapes so a storm of 500s - # or 422s counts toward the breaker (previously bypassed → unbounded - # retries). The dict-shape defence against `TypeError: unhashable type: - # 'dict'` lives in `_classify_rejection` (isinstance checks, never a - # `dict in frozenset` membership test). + # at all. Classify all three rejection shapes so a storm of 500s or 422s + # counts toward the breaker (previously bypassed → unbounded retries). The + # dict-shape defence against `TypeError: unhashable type: 'dict'` lives in + # `_classify_rejection` (isinstance checks, never a `dict in frozenset` + # membership test). rejection_kind = _classify_rejection(payload) if rejection_kind is None: return payload diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 0401744a..5221b427 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -304,16 +304,13 @@ _GROK_INTERACTIVE_DOCKERFILES = { # the retry window (unknown-provider time-expiry fallback in _probe_target). _GROK_RATE_LIMIT_EXIT_CODE = 75 _GROK_RATE_LIMIT_RETRY_AFTER_S = 60.0 -# F097: grok has no real recovery probe (the grok CLI's xAI endpoint is closed -# and the SuperGrok OIDC access token is not a valid bearer for the metered -# api.x.ai, so a probe would either no-op or strand grok parked forever). So -# the probe loop clears a grok park optimistically on a timer, a cleared park -# dispatches a fresh grok agent that immediately hits the still-active xAI 429, -# exits 75, and re-parks — a flat ~90s crash-retry cycle for the whole xAI -# window. Back the re-park retry_after off exponentially within one episode so -# the churn dampens (60 -> 120 -> 240 -> ... capped) instead of spinning flat. -# Cap bounds the cycle; the episode gap (> the max cycle) resets the count once -# the rate limit has actually lifted (no re-park for the gap => fresh episode). +# Grok has no real recovery probe (the SuperGrok OIDC token is not a valid +# bearer for the metered api.x.ai, so a probe would no-op or strand grok +# parked). The probe loop clears a grok park on a timer; the fresh agent hits +# the still-active xAI 429, exits 75, and re-parks. Back the re-park retry_after +# off exponentially within one episode so the churn dampens (60 -> 120 -> 240 +# -> ... capped) instead of spinning flat. The episode gap resets the count +# once the rate limit has actually lifted. _GROK_REPARK_BACKOFF_CAP = 4 # max 2**4 = 16x base (~16min cycle) _GROK_REPARK_EPISODE_GAP_S = 1500.0 # 25min — > the capped ~16min cycle # A one-shot Grok container exits with this code (EX_CONFIG) when the @@ -925,14 +922,10 @@ class AgentOrchestrator: # kill-switch parity (the grok CLI exposes no live usage hook). 0 disables. # See _enforce_grok_cost_budget. self._grok_max_cost_usd: float = settings.grok_max_cost_usd - # F097: grok re-park backoff state. Grok has no real recovery probe, so - # the probe loop clears a grok park optimistically on a timer; a cleared - # park respawns a grok agent that hits the still-active xAI 429 and - # re-parks. Track the re-park count within one episode so the retry_after - # can back off exponentially (dampening the ~90s crash-retry churn), and - # the last park time so a gap (the rate limit actually lifted) resets - # the count for the next episode. Per-provider state would be cleaner, - # but grok is a single provider key, so a scalar suffices. + # Grok re-park backoff state. Track the re-park count within one episode + # so retry_after can back off exponentially (dampening the ~90s + # crash-retry churn), and the last park time so a gap (the rate limit + # actually lifted) resets the count for the next episode. self._grok_last_park_at: datetime | None = None self._grok_repark_count: int = 0 @@ -4848,11 +4841,9 @@ class AgentOrchestrator: error=str(exc), ) continue - # F040: finalize the spawn session BEFORE popping the instance so - # the captured usage/cost is recorded in the DB/dashboard. - # _finalize_spawn_session reads self._instances[agent_id] for the - # model + usage_session_id; popping first would lose them and leave - # the session row open (ended_at IS NULL) — the burn invisible. + # Finalize the spawn session BEFORE popping the instance so the + # captured usage/cost is recorded; popping first would lose the + # model + usage_session_id and leave the session row open. with contextlib.suppress(Exception): await self._finalize_spawn_session(agent_id, exit_reason="cost_cap") self._instances.pop(agent_id, None) @@ -6834,11 +6825,10 @@ Start by: error=str(e), ) - # F045 orphan fallback: resume agents parked for a provider the + # Orphan fallback: resume agents parked for a provider the # tracker-listed loop above did not cover (activate failed silently or - # Redis was down at park time). Empty state => probe immediately; on - # success ``_on_probe_success`` clears the tracker (self-healing) and - # resumes the parked agents. + # Redis was down at park time). On probe success ``_on_probe_success`` + # clears the tracker (self-healing) and resumes the parked agents. orphan_providers: set[str] = set() for record in self._waiting_records.values(): if record.waiting_for != "rate_limit_lifted": @@ -6984,11 +6974,10 @@ Start by: if provider_type not in (None, ModelProvider.ANTHROPIC.value): return None tail = await self._tail_container_logs(f"roboco-agent-{agent_id}") - # F036: the SDK server writes model-API errors to /tmp/sdk-server.log, - # not stdout, so the overload marker (529/500/503) may appear only in - # the durable Claude transcript — the same rationale already applied to - # the session-limit detector. Without the transcript an overload is - # missed and the agent crash-respawns straight back into it. + # The SDK server writes model-API errors to /tmp/sdk-server.log, not + # stdout, so the overload marker may appear only in the durable Claude + # transcript; without it an overload is missed and the agent + # crash-respawns straight back into it. transcript_tail = self._transcript_tail_text(agent_id) lowered = (tail + "\n" + transcript_tail).lower() if any(marker in lowered for marker in _ANTHROPIC_OVERLOAD_MARKERS): @@ -7058,16 +7047,13 @@ Start by: kind=kind, error=str(exc), ) - # F035: register a WaitingRecord so the probe-resume loop can revive - # this agent when the provider recovers. ``_on_probe_success`` reads - # ``_waiting_records`` filtered by ``waiting_for == "rate_limit_lifted"`` - # + ``context.provider``; without a record here it resumes nobody and - # recovery falls to the 600s stale-claim reaper instead of the - # probe-success path the parking design relies on. Persisted (mirrors - # ``mark_waiting_long``) so a restart still resolves the wait. We do NOT - # call ``mark_waiting_long`` itself — the container is already dead (it - # exited), so there is nothing to stop, and parking keeps OFFLINE (not - # WAITING_LONG) so the reaper's live-skip / health loop ignore it. + # Register a WaitingRecord so the probe-resume loop can revive this + # agent when the provider recovers; without it recovery falls to the + # 600s stale-claim reaper instead of the probe-success path the parking + # design relies on. Persisted so a restart still resolves the wait. + # We do NOT call ``mark_waiting_long`` — the container is already dead, + # and parking keeps OFFLINE so the reaper's live-skip / health loop + # ignore it. task_id = str(instance.current_task_id) if instance.current_task_id else None record = WaitingRecord( agent_id=agent_id, @@ -8429,13 +8415,11 @@ Start now: evidence(task_id="{task_id}") continue if not is_running: continue - # F033: capture the real container id. _check_health skips - # ``container_id is None`` instances, so a re-adopted instance - # without the id would be invisible to the health loop — when the - # container later exits the stopped-container handler never runs - # and the task strands under a phantom ACTIVE instance. Best-effort: - # a probe failure degrades to the prior None (still re-adopted as - # ACTIVE; the reaper's Docker-liveness fallback covers it). + # Capture the real container id: ``_check_health`` skips instances + # with ``container_id is None``, so a re-adopted instance without + # the id would be invisible to the health loop and strand the task + # under a phantom ACTIVE instance. Best-effort — a probe failure + # degrades to None (reaper's Docker-liveness fallback covers it). container_id: str | None = None try: container_id = await self._resolve_container_id(f"roboco-agent-{slug}") @@ -8643,11 +8627,11 @@ Start now: evidence(task_id="{task_id}") and not await self._maybe_recover_broken_gateway(t) ): continue - # F035: a provider-parked agent (session-limit / overload / - # grok-429) is OFFLINE with a dead container and a - # ``rate_limit_lifted`` WaitingRecord. The probe-resume loop - # owns its recovery — do NOT reap the claim, or probe-success - # would later respawn the agent on a task it no longer owns. + # A provider-parked agent (session-limit / overload / grok-429) + # is OFFLINE with a dead container and a ``rate_limit_lifted`` + # WaitingRecord. The probe-resume loop owns its recovery — do + # NOT reap the claim, or probe-success would respawn the agent + # on a task it no longer owns. if self._assignee_is_provider_parked(t): continue task_id = require_uuid(t.id) @@ -9225,12 +9209,11 @@ Start now: evidence(task_id="{task_id}") # are acted on by the release routes + executor, never dispatched. if task.get("source") == RELEASE_MANAGER_SOURCE: continue - # F059: a self-heal fix task is HELD for the CEO's Approve-&-Start + # A self-heal fix task is HELD for the CEO's Approve-&-Start # (confirmed_by_human=False at origination). It must NOT dispatch - # autonomously — the loop only OPENS it; the CEO's approve_and_start - # flips confirmed_by_human True, after which it flows through the - # assigned-PM path below like any other PM task. (The fix still ships - # through dev -> QA -> PR review -> the CEO's merge.) + # autonomously — the CEO's approve_and_start flips + # confirmed_by_human True, after which it flows through the + # assigned-PM path below like any other PM task. if task.get("source") == SELF_HEAL_SOURCE and not task.get( "confirmed_by_human" ): @@ -9616,9 +9599,9 @@ Never `commit`, never write code, never run `git`. PMs coordinate. # Release proposals are CEO-gated artifacts, never dev work. if task.get("source") == RELEASE_MANAGER_SOURCE: continue - # F059: a self-heal fix task held for the CEO's Approve-&-Start is - # not dev work yet — it must not route to its assigned_to as a dev - # before the CEO approves it. + # A self-heal fix task held for the CEO's Approve-&-Start is not dev + # work yet — it must not route to its assigned_to as a dev before + # the CEO approves it. if task.get("source") == SELF_HEAL_SOURCE and not task.get( "confirmed_by_human" ): diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index b5e526ca..26d1816e 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1919,7 +1919,7 @@ class Choreographer: ``reviewer=True`` for the pr_pass gate: a PR reviewer has no ``i_am_blocked`` verb, so the remediation points at ``pr_fail`` (their reject lever, sending the PR back to needs_revision for the dev to fix - the environment) instead of a verb they cannot call (F044). + the environment) instead of a verb they cannot call. """ from roboco.config import settings as _settings @@ -2019,7 +2019,7 @@ class Choreographer: ``file:line`` + fix hint. ``warn`` findings never block. Inert when the flag is off. This is the pr_pass (reviewer) path — the remediation is reviewer-aware (``pr_fail``, not ``i_am_blocked`` which a reviewer - lacks) via ``_conventions_rejection(..., reviewer=True)`` (F044). + lacks) via ``_conventions_rejection(..., reviewer=True)``. """ from roboco.config import settings as _settings @@ -2036,7 +2036,7 @@ class Choreographer: ``reviewer=True`` for the pr_pass gate: a reviewer has no ``i_am_blocked`` verb, so the could_not_run remediation points at - ``pr_fail`` instead (F044). + ``pr_fail`` instead. """ if result.get("could_not_run"): if reviewer: @@ -2067,15 +2067,10 @@ class Choreographer: f"- {f.get('file')}:{f.get('line')} — {f.get('fix_hint')}" for f in blocks ) if reviewer: - # F047: the pr_pass gate runs this on the REVIEWER, who does not own - # the assembled cell→root / root→master branch and has no commit - # verb on it. The dev-path remediation ("add a waiver in your - # branch") is unreachable by the reviewer and would strand the gate - # on every false positive with no self-recovery. The reviewer's only - # lever is pr_fail — bounce the PR back to needs_revision carrying - # the findings as issues so the dev fixes the violation or commits - # the waiver (the dev CAN commit to the branch). Waiver authorship is - # framed as the dev's action, not the reviewer's. + # The pr_pass gate runs on the REVIEWER, who has no commit verb on + # the assembled branch; the only lever is pr_fail — bounce the PR + # back to needs_revision with the findings as issues so the dev + # fixes the violation or commits a waiver. remediate = ( "the assembled PR carries block-level architectural-convention" " violations. call pr_fail(issues=[, ...])" @@ -2913,14 +2908,9 @@ class Choreographer: task_id=task_id, verb="i_am_blocked", ) - # F017: ``block`` is the LAST composed action, so a ``None`` return - # (TaskService.escalate resolved no escalation target — missing task, - # agent, escalation-target slug, or target agent row) flows out of - # ``run_intent`` as the verb's result. Without this guard the caller - # re-binds ``t`` to ``None`` and dereferences ``t.status`` building the - # success envelope → AttributeError → HTTP 500, and the agent - # respawn-loops with no actionable rejection. Surface invalid_state - # instead, pointing the agent at a direct CEO escalation or a retry. + # ``block`` is the LAST composed action, so a ``None`` return (escalate + # resolved no target) re-binds ``t`` to ``None``; guard the deref with + # an invalid_state so the agent gets a retryable rejection, not a 500. if updated is None: return t, await self._emit_rejection( Envelope.invalid_state( @@ -3014,16 +3004,9 @@ class Choreographer: # provider is "unknown" (orchestrator not wired or not tracking the # agent) to avoid polluting the tracker with meaningless keys. # - # F045: an activate() failure is logged loudly, NOT bare-suppressed. - # The probe-resume loop is tracker-driven — it iterates - # ``list_rate_limited_providers()`` — so a silent activate failure here - # leaves every parked agent in ``_waiting_records`` with a provider the - # tracker never learned about, and no probe ever runs to resume them - # (the stranded-fleet blind spot). The orchestrator's - # ``_sweep_rate_limit_probes`` has an in-memory ``_waiting_records`` - # fallback that resumes them when the provider recovers even without - # tracker state; this error log makes the condition visible to - # operators either way. + # An activate() failure is logged loudly, NOT bare-suppressed: the + # probe-resume loop is tracker-driven, so a silent failure strands + # every parked agent waiting on a provider the tracker never learned. if provider != "unknown": try: from roboco.services.gateway.rate_limit_tracker import ( @@ -3968,10 +3951,9 @@ class Choreographer: reality. Checkpoint failure is swallowed; it must never block the pause. """ in_progress = await self.task.list_in_progress_for_agent(agent_id) - # F018: the lookup now also returns blocked tasks (so the claim guard - # sees them). i_am_idle only auto-pauses genuinely in_progress tasks — - # a blocked task is waiting on an external dep, not on the agent, so it - # stays blocked (and isn't reported as paused for the agent to resume). + # The lookup also returns blocked tasks (so the claim guard sees them); + # i_am_idle only auto-pauses genuinely in_progress ones — a blocked task + # waits on an external dep, not the agent, so it stays blocked. from roboco.models.base import TaskStatus paused_ids: list[str] = [] @@ -5381,10 +5363,9 @@ class Choreographer: # + subtasks-terminal + branch-present. None of these are modelled by # the spec yet — keep them in the verb body. guard = await self._submit_up_guard(pm_agent_id, task_id, t, notes) - # F007: the cell-level unchanged-PR loop-stopper. Only consult it once - # the state guard above has passed (ownership/tracing/branch all OK) — - # mirroring submit_root, a prior preflight reject short-circuits before - # the head-sha comparison runs. + # Cell-level unchanged-PR loop-stopper. Consulted only after the state + # guard above passes; a prior preflight reject short-circuits before the + # head-sha comparison runs (mirroring submit_root). if guard is None: guard = await self._submit_up_unchanged_pr_guard(t, briefing) if guard is not None: @@ -6173,7 +6154,7 @@ class Choreographer: async def _current_pr_head_sha(self, t: Any) -> str | None: """Best-effort current head SHA of the task's assembled PR (fail-open). - The lookup both unchanged-PR gates (submit_root F016 + submit_up F007) + The lookup both unchanged-PR gates (submit_root + submit_up) compare against — ``pr_fail`` stamps the head SHA for cell AND root gate tasks alike (the capture is gate-verb-level, not root-level), so one resolver serves both. Returns ``None`` on every ambiguous case (no @@ -6201,18 +6182,12 @@ class Choreographer: async def _submit_up_unchanged_pr_guard( self, t: Any, briefing: dict[str, Any] ) -> Envelope | None: - """F007 — the cell-level analogue of ``_submit_root_unchanged_pr_guard``. + """Cell-level analogue of ``_submit_root_unchanged_pr_guard``. - The root loop-stopper was root-only; a weak cell PM could re-submit the - unchanged cell→root PR after a ``pr_fail`` and loop - ``awaiting_pr_review`` → ``pr_fail`` forever. ``pr_fail`` stamps the - assembled PR's head SHA into ``notes_structured.pr_review.head_sha`` - for cell gate tasks too (the capture is gate-verb-level), so the same - structural refusal applies: if the cell PR's current head SHA equals - the SHA the last ``pr_fail`` recorded, no new dev work landed on the - cell branch ⇒ the diff is byte-identical ⇒ refuse. Every ambiguous case - FAILS OPEN (shared ``_current_pr_head_sha``) — only the exact-unchanged - case is hard-blocked; the rest fall through to the cell reviewer. + Refuses re-submit when the cell PR's current head SHA equals the SHA + ``pr_fail`` recorded in ``notes_structured.pr_review.head_sha`` (no new + dev work landed ⇒ byte-identical diff). Ambiguous cases FAIL OPEN via + ``_current_pr_head_sha``; only the exact-unchanged case is hard-blocked. """ pr_review = (getattr(t, "notes_structured", None) or {}).get("pr_review") or {} if pr_review.get("verdict") != "failed": @@ -6334,16 +6309,10 @@ class Choreographer: task_id=task_id, verb="submit_root", ) - # F016: submit_for_review returns None when the root->master PR was - # already opened (the task raced out of in_progress, or a prior call - # already transitioned it to awaiting_pr_review). The create_root_pr - # pre-side-effect already ran, so the PR exists, but the transition - # did not happen — dereferencing t.status here 500'd. Surface an - # actionable invalid_state so the PM re-fetches and reconciles (if the - # task is already awaiting_pr_review the PR is open — wait for the - # reviewer; otherwise re-delegate the fixes and retry) instead of a - # crash. The None-guard + success envelope share a finalize helper so - # submit_root's own return count stays under the branch-limit. + # submit_for_review returns None when the root->master PR was already + # opened (race out of in_progress / prior call). The PR exists but the + # transition did not — guard the None deref with an invalid_state so + # the PM re-fetches and reconciles instead of crashing. return await self._submit_root_finalize( main_pm_agent_id, task_id, t, role_str, briefing ) @@ -6358,7 +6327,7 @@ class Choreographer: ) -> Envelope: """Build the submit_root result envelope after the verb runner returns. - ``None`` (F016) → invalid_state rejection (the root->master PR was + ``None`` → invalid_state rejection (the root->master PR was already opened / the task raced out of in_progress); otherwise the success envelope keyed off the post-transition status. """ @@ -6627,17 +6596,14 @@ class Choreographer: verb="complete", ): return soup - # F001: a MegaTask umbrella is branchless by design and never goes - # through submit_root / pr_pass, so it sits in in_progress with no - # branch/PR. The ``complete`` action's source_statuses= - # {AWAITING_PM_REVIEW} spec gate would reject it before - # main_pm_complete's branchless-aware guard can run. Skip the spec - # gate for an in_progress batch umbrella and fall through to - # main_pm_complete, which walks in_progress -> awaiting_pm_review -> - # awaiting_ceo_approval (the CEO merges the root PR; no agent touches - # master). Role membership is preserved (role_str == "main_pm"); - # main_pm_complete's own guard re-checks assignment, subtasks- - # terminal, and the journal:decision gate. + # A MegaTask umbrella is branchless by design and never goes through + # submit_root / pr_pass, so it sits in in_progress with no branch/PR. + # The ``complete`` action's source_statuses={AWAITING_PM_REVIEW} spec + # gate would reject it before main_pm_complete's branchless-aware guard + # can run; skip the spec gate for an in_progress batch umbrella and fall + # through to main_pm_complete (CEO merges the root PR; no agent touches + # master). Role membership is preserved; main_pm_complete re-checks + # assignment, subtasks-terminal, and the journal:decision gate. umbrella_in_progress = ( role_str == "main_pm" and str(t.status) == "in_progress" diff --git a/roboco/services/gateway/choreographer/pr_gate.py b/roboco/services/gateway/choreographer/pr_gate.py index 863f539b..acdf4b12 100644 --- a/roboco/services/gateway/choreographer/pr_gate.py +++ b/roboco/services/gateway/choreographer/pr_gate.py @@ -282,16 +282,10 @@ class PRGateMixin(_Base): task_id=task_id, verb=verb, ) - # F046: a concurrent transition (cancel, or a racing reviewer) between - # the precondition gate and the runner's final composed action makes - # the source-status check fail mid-flight and run_intent returns None - # (the verb runner's documented contract for a last-action source-status - # failure). Without this guard the dereferences below (t.assigned_to, - # t.status, _post_gate_review_to_pr(t, ...)) crash the gate with a 500 - # AttributeError. Surface a clean invalid_state rejection so the - # reviewer re-fetches with evidence(task_id) and re-issues — the - # already-authored verdict note is harmless (the task is no longer in - # the gate state) and no PR post / a2a runs against a None task. + # A concurrent transition (cancel, racing reviewer) between the + # precondition gate and the runner's final action makes run_intent + # return None; guard the dereferences below with a clean rejection so + # the reviewer re-fetches and re-issues. if t is None: return await self._emit_rejection( Envelope.invalid_state( diff --git a/roboco/services/gateway/claim_guards.py b/roboco/services/gateway/claim_guards.py index 1e5fd968..7712fd79 100644 --- a/roboco/services/gateway/claim_guards.py +++ b/roboco/services/gateway/claim_guards.py @@ -24,13 +24,9 @@ from roboco.services.gateway.envelope import Envelope if TYPE_CHECKING: from uuid import UUID -# Statuses that count as "still actively worked" — pre-gateway -# _helpers.py:check_blocking_tasks 134-152. -# -# F018: ``blocked`` is included — a blocked task is still owned by the dev and -# ``unblock_with_restore`` resumes it to ``in_progress``. Excluding it let a dev -# claim a second task while blocked, then end up with TWO ``in_progress`` -# tasks once the first was unblocked, violating the one-active-task invariant. +# Statuses that count as "still actively worked". +# ``blocked`` is included: it is still owned by the dev and resumes to +# ``in_progress``; excluding it let a dev hold two in_progress tasks at once. _ACTIVE_BLOCKING_STATUSES: frozenset[str] = frozenset( {"claimed", "in_progress", "verifying", "blocked"} ) diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index 59e52c5d..deabf095 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -1208,10 +1208,9 @@ class ContentActions: # A dependency block is a "wait silently" situation — never a CEO signal. # An agent must not page the CEO to relax or escalate a task that is # simply waiting on an unfinished upstream; that wait clears on its own. - # F048: also reject human-only recipients (prompter/secretary) — they - # have no agent ack path, so an ack-required signal would sit permanently - # unacked and suppress later same-purpose notifications via the dedup - # query. The CEO acks via the panel and stays an allowed recipient. + # Also reject human-only recipients (prompter/secretary) — they have no + # agent ack path, so an ack-required signal would sit permanently unacked + # and suppress later same-purpose notifications via the dedup query. if reject := await self._reject_disallowed_recipient(target, task_id): return reject await self.notifications.send_ack_notification( diff --git a/roboco/services/gateway/evidence_builder.py b/roboco/services/gateway/evidence_builder.py index 87b236d9..a6693997 100644 --- a/roboco/services/gateway/evidence_builder.py +++ b/roboco/services/gateway/evidence_builder.py @@ -106,14 +106,10 @@ def build_task_handoff( # Upstream dependencies that completed and were cleared — present only on a # just-unblocked task, so the revived dependent knows what it can build on. completed_deps = _typed(getattr(task, "completed_dependency_ids", None), list, []) - # F008 — the persisted in-path PR-review gate verdict + concrete issues. - # ``pr_fail`` authors ``notes_structured.pr_review`` (verdict / summary / - # issues / head_sha) on every fail, but the a2a steer to the owning PM is - # fire-and-forget — a PM respawned into ``needs_revision`` later read none - # of it (build_task_handoff never looked at notes_structured), saw a generic - # "needs revision" with zero change-requests, and re-submitted the same PR - # (the 2026-06-27 infinite pr_fail loop on 9980d0a0 / PR #138). Surfacing it - # here puts the concrete issues in every PM briefing for the task. + # The persisted in-path PR-review gate verdict + concrete issues. + # ``pr_fail`` writes ``notes_structured.pr_review``; surfacing it here puts + # the concrete issues in every PM briefing so a respawned PM doesn't + # re-submit the same PR blind. pr_review = _extract_pr_review(getattr(task, "notes_structured", None)) has_prior = bool( commits diff --git a/roboco/services/git.py b/roboco/services/git.py index 29880ce0..a192bee6 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -277,9 +277,8 @@ class GitService(BaseService): try: result = await loop.run_in_executor(_GIT_EXECUTOR, _run) except subprocess.TimeoutExpired as e: - # F019: the timed-out git process was SIGKILL'd mid-mutation and may - # have orphaned .git/*.lock files; clear them so the workspace isn't - # wedged for the next op (incl. the next fresh-claim reset --hard). + # Timed-out git process was SIGKILL'd mid-mutation and may orphan + # .git/*.lock files; clear them so the workspace isn't wedged. await loop.run_in_executor( _GIT_EXECUTOR, _remove_stale_git_locks, workspace ) diff --git a/roboco/services/messaging.py b/roboco/services/messaging.py index 97c84248..32c03c24 100644 --- a/roboco/services/messaging.py +++ b/roboco/services/messaging.py @@ -1495,10 +1495,9 @@ class MessagingService(BaseService): subject=f"You were mentioned in #{channel_slug}", body=message.content[:500], # Truncate for notification related_task_id=message.task_id, - # F009: MENTION is informational (ACK_REQUIRED_BY_TYPE -> False). - # The column default True made every @mention require an ack, - # inflating the recipient's unacked set and soft-blocking - # i_am_idle into respawn churn. + # MENTION is informational (ACK_REQUIRED_BY_TYPE -> False); the + # column default True would inflate unacked sets and soft-block + # i_am_idle. requires_ack=False, ) self.session.add(notification) diff --git a/roboco/services/notification.py b/roboco/services/notification.py index a55ee2cc..3af82709 100644 --- a/roboco/services/notification.py +++ b/roboco/services/notification.py @@ -493,15 +493,9 @@ class NotificationService: # already acked all go through. Body text is NOT compared, so # rewording cannot defeat the guard. # - # F010: the dedup only applies to ACTION-REQUIRED types - # (ACK_REQUIRED_BY_TYPE -> True). Informational types - # (KNOWLEDGE_SHARE / MENTION / A2A_REQUEST / BROADCAST / the - # pickup-proves-receipt triad) carry distinct content per send — a - # new learning, a new mention — and acking them is voluntary, so a - # recipient who never acks would let the dedup permanently suppress - # every subsequent same-sender broadcast (silent learning-broadcast - # data loss). The anti-loop rationale only holds for ack-required - # signals; informational ones are not deduped. + # Dedup only applies to ACTION-REQUIRED types; informational types + # carry distinct content per send and acking is voluntary, so + # deduping them would silently drop broadcasts. related = params.related_task_id is_ack_required = ACK_REQUIRED_BY_TYPE.get(params.notification_type, True) if is_ack_required: @@ -535,14 +529,9 @@ class NotificationService: subject=params.subject, body=params.body, related_task_id=params.related_task_id, - # F009: requires_ack follows ACK_REQUIRED_BY_TYPE (the spec's - # action-required vs informational split), not the column's True - # default. Without this every notification — including - # informational REVIEW_REQUEST / DOCUMENTATION_REQUEST / - # A2A_REQUEST / MENTION / KNOWLEDGE_SHARE — became requires_ack, - # inflating recipients' unacked sets and soft-blocking - # i_am_idle into respawn churn. Default to True for an unmapped - # type (preserve the safe action-required bias). + # requires_ack follows ACK_REQUIRED_BY_TYPE (action-required vs + # informational), not the column's True default; default True + # for an unmapped type preserves the safe action-required bias. requires_ack=ACK_REQUIRED_BY_TYPE.get(params.notification_type, True), ) db.add(notification) diff --git a/roboco/services/release_executor.py b/roboco/services/release_executor.py index 700cc07a..4f0ad674 100644 --- a/roboco/services/release_executor.py +++ b/roboco/services/release_executor.py @@ -255,9 +255,8 @@ class _GitReleaseOps: "commit", "-S", "-m", f"chore(release): {version}" ) if commit_rc != 0: - # F012: a failed commit (gpgsign/pre-commit reject/no-op bump) must - # abort BEFORE rev-parse+push — otherwise the pre-bump base gets - # pushed and tagged as the new version. + # A failed commit (gpgsign/pre-commit reject/no-op bump) must abort + # before push — otherwise the pre-bump base gets tagged as the release. logger.error("release commit failed", error=commit_out.strip()[:300]) raise RuntimeError(f"release commit failed: {commit_out.strip()[:200]}") _, out = await self._git("rev-parse", "HEAD") diff --git a/roboco/services/release_proposal.py b/roboco/services/release_proposal.py index 8f9750ee..ca6c76d8 100644 --- a/roboco/services/release_proposal.py +++ b/roboco/services/release_proposal.py @@ -33,10 +33,8 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -# F013: a Redis mutex guarding the ~40min release execute against concurrent -# approves (CEO double-click / panel retry). TTL is a backstop above the CI -# poll ceiling (ReleaseExecutor ~40min) so a crashed process can't hold the -# release hostage forever; the lock is released explicitly on completion. +# Redis mutex guarding the ~40min release execute against concurrent +# approves; TTL backstops a crash, lock is released on completion. _RELEASE_LOCK_PREFIX = "roboco:release_proposal:" _RELEASE_LOCK_TTL_SECONDS = 3000 # 50 min > 40 min CI ceiling diff --git a/roboco/services/release_readiness.py b/roboco/services/release_readiness.py index ab09b285..26140e3e 100644 --- a/roboco/services/release_readiness.py +++ b/roboco/services/release_readiness.py @@ -438,12 +438,9 @@ def _canonical_bump_files(root: Path, version: str) -> list[str]: return sorted( line.strip() for line in files_raw.splitlines() if line.strip() ) - # F058: the FIRST release has no prior ``chore(release):`` commit, so the - # historical derivation returns ``[]`` and the executor would publish a tag - # with no files bumped (a no-op release masquerading as X.Y.Z). Fall back to - # the version-reference scan — the files currently embedding the version are - # exactly the set a first release must bump, and the set the first release - # commit then records as canonical for every subsequent release. Read-only. + # First release has no prior ``chore(release):`` commit, so derivation + # returns [] — fall back to the version-reference scan: files embedding the + # version are exactly the set a first release must bump. Read-only. return _tracked_files_with_version(root, version) diff --git a/roboco/services/task.py b/roboco/services/task.py index 7b717383..d61bbcab 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -2316,19 +2316,13 @@ class TaskService(BaseService): task.last_heartbeat_at = original_heartbeat task.active_claimant_id = original_claimant_id await self.session.flush() - # F060: emit the reversal audit row so the journey doesn't - # diverge from real state. The forward ``task.claimed`` row - # (emitted above via ``_validate_and_set_status``) was already - # committed by the audit service on its OWN connection — this + # emit the reversal audit row so the journey doesn't diverge + # from real state. The forward ``task.claimed`` audit row was + # already committed on the audit service's own connection; this # rollback's flush reverts the task row but NOT that audit row. - # Without a matching reversal row the journey's last event stays - # ``task.claimed`` while the task is back to its pre-claim - # status, corrupting every downstream metric reconstructed from - # ``task.`` events (cycle time, bottlenecks). Emitted - # only when the forward transition was made (the original status - # was claimable) and attributed to the claimant via the explicit - # ``audit_agent_id`` (``claimed_by`` was just rolled back to - # ``None``). + # Without a matching reversal row, downstream metrics + # (cycle time, bottlenecks) reconstructed from ``task.`` + # events would be corrupted. if original_status in self._CLAIMABLE_STATUSES: self._emit_status_transition_audit( task, @@ -5248,12 +5242,12 @@ class TaskService(BaseService): already = task.assigned_to == main_pm.id task.assigned_to = cast("Any", main_pm.id) - # F059: this is the CEO's start gate — approving the task confirms it for - # dispatch. A self-heal fix task is opened held (confirmed_by_human=False) - # so the orchestrator + give_me_work keep it out of dispatch until now; - # flipping it True lifts that hold. Idempotent for board/intake tasks, - # which are already confirmed at creation. (The release-manager proposal - # is not routed through approve_and_start — it has its own CEO routes.) + # this is the CEO's start gate — approving the task confirms it for + # dispatch. A self-heal fix task is opened held + # (confirmed_by_human=False) so dispatch skips it until now; flipping + # it True lifts that hold. Idempotent for board/intake tasks (already + # confirmed at creation). The release-manager proposal is not routed + # here — it has its own CEO routes. task.confirmed_by_human = cast("Any", True) # The board-reviewed coordination task now belongs to Main PM, who will # delegate it to the cells. Leaving team="board" is misleading once it's @@ -5317,14 +5311,12 @@ class TaskService(BaseService): if child.batch_id is not None and child.status == TaskStatus.BACKLOG: child.status = TaskStatus.PENDING child.team = cast("Any", Team.MAIN_PM) - # F002: a board-routed root-subtask is created in BACKLOG with - # team=board and task_type=code (intake only coerces main_pm-team - # drafts, so a board-routed code root reaches activation still - # code-typed). Now that team is flipped to MAIN_PM, leaving - # task_type=code would re-introduce the 2026-06-27 main_pm+code - # meltdown. Retype code->planning, mirroring approve_and_start's - # own retype above — the activated child is a planning-typed - # coordination root the Main PM delegates to the cells. + # a board-routed root-subtask is created in BACKLOG with + # team=board and task_type=code. Now that team is flipped to + # MAIN_PM, leaving task_type=code would re-introduce the + # main_pm+code meltdown — retype code->planning so the activated + # child is a planning-typed coordination root the Main PM + # delegates to the cells. if main_pm_cannot_own_code(team=child.team, task_type=child.task_type): self.log.info( "activate_batch_root_subtasks retyped main-pm code " diff --git a/roboco/services/workspace.py b/roboco/services/workspace.py index 9175c55a..1f09fc3c 100644 --- a/roboco/services/workspace.py +++ b/roboco/services/workspace.py @@ -1049,14 +1049,11 @@ class WorkspaceService: workspace=str(workspace), ) except subprocess.CalledProcessError as e: - # F063: a failure anywhere in clone/configure/leakcheck/own leaves - # a half-configured workspace on disk. If _configure_git raised - # before its `remote set-url` scrub, .git/config still carries the - # tokenized auth URL (the project PAT); _assert_no_pat_leak never - # ran, and the next ensure_workspace's health short-circuit would - # skip straight past the leak — mounting the agent on a workspace - # whose .git/config lets it read+exfiltrate the PAT. Destroy the - # workspace so the next ensure_workspace re-clones from scratch. + # a failure anywhere in clone/configure/leakcheck/own leaves a + # half-configured workspace whose .git/config may still carry the + # tokenized auth URL (the project PAT). Destroy it so the next + # ensure_workspace re-clones from scratch — fail-closed against + # PAT exfiltration. shutil.rmtree(workspace, ignore_errors=True) raise WorkspaceError( f"Failed to clone repository: {e.stderr or e.stdout}" diff --git a/tests/foundation/test_identity.py b/tests/foundation/test_identity.py index 06159910..3af42c66 100644 --- a/tests/foundation/test_identity.py +++ b/tests/foundation/test_identity.py @@ -232,7 +232,7 @@ def test_team_for_slug() -> None: def test_role_for_slug_or_none_unknown_returns_none() -> None: - """F019/F031: a safe variant for defensive skip-guards — an unknown/stale + """A safe variant for defensive skip-guards — an unknown/stale slug returns None instead of raising KeyError, so a stale assignee or notification-target slug can't crash the whole dispatcher tick.""" assert identity.role_for_slug_or_none("nonexistent-slug") is None diff --git a/tests/foundation/test_lifecycle_spec.py b/tests/foundation/test_lifecycle_spec.py index e1d959c7..2b854cc7 100644 --- a/tests/foundation/test_lifecycle_spec.py +++ b/tests/foundation/test_lifecycle_spec.py @@ -567,7 +567,7 @@ def test_can_invoke_intent_developer_open_pr_no_commits_tracing_gap() -> None: # --------------------------------------------------------------------------- # -# F101: open_pr must enforce the PR-open state gate (parity with the HTTP path) +# open_pr must enforce the PR-open state gate (parity with the HTTP path) # --------------------------------------------------------------------------- # @@ -584,10 +584,8 @@ def _owned_task(**overrides: Any) -> SimpleNamespace: def test_open_pr_rejected_on_claimed_task() -> None: - """F101: ``open_pr`` has ``composes=()`` so the spec gate applied NO - source-status check — a dev could open a PR from ``claimed`` (before - ``in_progress``), skipping the active-dev state the HTTP path's - ``_assert_pr_create_allowed`` enforces. The state gate now rejects it.""" + """``open_pr`` must be rejected from ``claimed`` — only ``in_progress`` may + open a PR (mirrors the HTTP path's ``_assert_pr_create_allowed``).""" actor = uuid4() d = spec.can_invoke_intent( spec.Role.DEVELOPER, @@ -673,14 +671,8 @@ def test_open_pr_state_gate_takes_priority_over_unowned() -> None: def test_escalate_up_rejected_on_completed_task() -> None: - """F043: a PM must not resurrect a COMPLETED task via escalate_up. - - escalate_up has composes=() and historically no source-status guard, so the - spec gate accepted it on a terminal task and apply_escalation set it back to - BLOCKED — bypassing the state machine's terminal-state invariant. The spec - now rejects terminal tasks (completed / cancelled) before the journal:decision - write fires. - """ + """A PM must not resurrect a COMPLETED task via ``escalate_up`` — the spec + gate rejects terminal tasks before the journal:decision write fires.""" d = spec.can_invoke_intent( spec.Role.CELL_PM, "escalate_up", @@ -692,7 +684,7 @@ def test_escalate_up_rejected_on_completed_task() -> None: def test_escalate_up_rejected_on_cancelled_task() -> None: - """F043: cancelled is terminal — escalate_up must not resurrect it either.""" + """Cancelled is terminal — escalate_up must not resurrect it either.""" d = spec.can_invoke_intent( spec.Role.MAIN_PM, "escalate_up", @@ -704,7 +696,7 @@ def test_escalate_up_rejected_on_cancelled_task() -> None: def test_escalate_up_allowed_on_blocked_task() -> None: - """F043: the terminal guard must not over-restrict — BLOCKED is the natural + """The terminal guard must not over-restrict — BLOCKED is the natural escalation source and must still be allowed.""" d = spec.can_invoke_intent( spec.Role.CELL_PM, diff --git a/tests/integration/services/test_dep_update_probe.py b/tests/integration/services/test_dep_update_probe.py index 657435d9..c2c69739 100644 --- a/tests/integration/services/test_dep_update_probe.py +++ b/tests/integration/services/test_dep_update_probe.py @@ -102,13 +102,10 @@ async def test_explicit_dep_update_paths_scope(tmp_path: Path) -> None: async def test_probe_holds_read_clone_lock_across_local_clone( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """F116: the dep-update probe must hold the read-clone lock for the - duration of the local ``git clone --local`` from the read clone, so a - concurrent ``ensure_read_clone`` → ``_sync_read_clone`` (fetch + hard-reset - to origin's default branch) cannot mutate the read clone mid-clone. The - lock is released before the upgrade runs on the independent copy (the - upgrade never touches the read clone, so holding the lock past the clone - would needlessly block conventions reads for the upgrade duration).""" + """The dep-update probe holds the read-clone lock across the local + ``git clone --local`` so a concurrent ``_sync_read_clone`` cannot mutate the + read clone mid-clone; released before the upgrade (which runs on an + independent copy).""" read_clone = _make_read_clone(tmp_path) svc = _svc(read_clone) # Unique slug → a fresh lock not shared with any other test. diff --git a/tests/integration/test_claim_lock_serialization.py b/tests/integration/test_claim_lock_serialization.py index 80d25c2a..72bff1fe 100644 --- a/tests/integration/test_claim_lock_serialization.py +++ b/tests/integration/test_claim_lock_serialization.py @@ -1,19 +1,8 @@ -"""F074 — real-Postgres proof that ``TaskService.acquire_claim_lock`` serializes -concurrent claims by the SAME agent (the one-task-per-agent invariant) while NOT -serializing claims by DIFFERENT agents. - -The choreographer-level ordering + coordinator-exemption is covered by the unit -suite (``test_choreographer_claim_lock.py``); this test pins the DB-level -contract the unit suite mocks out: that ``pg_advisory_xact_lock`` keyed by -``hashtextextended(agent_id)`` actually blocks a second transaction trying to -acquire the same agent's lock until the first commits/rolls back, and that a -different agent's lock is uncontended. Skips when Postgres is unreachable. - -Each ``acquire_claim_lock`` call uses its own fresh session/engine. The -blocking call's session is single-use: ``asyncio.wait_for`` cancelling an -in-flight asyncpg query leaves the SQLAlchemy session mid-connection-checkout -("provisioning a new connection"), so a throwaway session per timed acquire -keeps the rest of the test on clean connections. +"""Real-Postgres proof that ``TaskService.acquire_claim_lock`` serializes +concurrent claims by the SAME agent (one-task-per-agent) while NOT serializing +different agents. Skips when Postgres is unreachable; each ``acquire_claim_lock`` +uses a throwaway session so cancellation mid-checkout can't poison the rest of +the test. """ from __future__ import annotations diff --git a/tests/integration/test_messaging_service.py b/tests/integration/test_messaging_service.py index dcc00a7d..b4de1c4e 100644 --- a/tests/integration/test_messaging_service.py +++ b/tests/integration/test_messaging_service.py @@ -1198,7 +1198,7 @@ async def _seed_messages_same_timestamp( async def test_get_messages_compound_before_cursor_no_skip_on_equal_timestamps( msg_setup: dict, ) -> None: - """Equal-timestamp messages must not be skipped across pages (F106). + """Equal-timestamp messages must not be skipped across pages. With a strict ``timestamp < before`` cursor and ``order_by(timestamp.desc())``, messages sharing the page's last timestamp are cut by ``limit`` on page 1 @@ -1239,7 +1239,7 @@ async def test_get_messages_compound_after_cursor_no_skip_on_equal_timestamps( ) -> None: """Forward pagination (``after``) with the compound ``(timestamp, id)`` cursor tie-breaks on id so newer-direction pagination across equal - timestamps skips nothing either (F106). + timestamps skips nothing either. With a strict ``timestamp > after`` cursor, every row sharing the cursor's timestamp is EXCLUDED — so forward-paginating from a middle message would diff --git a/tests/integration/test_notification_delivery_phantom.py b/tests/integration/test_notification_delivery_phantom.py index e1cc6353..5ba73a18 100644 --- a/tests/integration/test_notification_delivery_phantom.py +++ b/tests/integration/test_notification_delivery_phantom.py @@ -1,17 +1,8 @@ -"""F107 — Redis bus publish must be deferred until the DB commit lands. +"""Redis bus publish must be deferred until the DB commit lands so a rollback +drops the event (no phantom notification for a row that never became durable). -`NotificationDeliveryService.deliver` historically published -``NOTIFICATION_SENT`` to the Redis event bus *before* the caller committed -the notification row. A commit failure (DB hiccup, constraint, asyncpg error) -rolled the row back but left the bus event behind — connected WebSocket -clients received a push for an id that no longer existed (a phantom -notification). The fix defers the bus publish to the session's -``after_commit`` so a rollback drops it; the row is durable by the time the -event fires. - -These tests need a real ``AsyncSession`` (the deferral uses SQLAlchemy -session commit/rollback events) plus a recording bus stand-in, so they are -integration tests against the migrated Postgres test DB. +Integration tests against the migrated Postgres DB: the deferral uses +SQLAlchemy ``after_commit`` events and a recording bus stand-in. """ from __future__ import annotations @@ -129,12 +120,8 @@ async def _seed_agents_and_notification( async def test_deliver_does_not_publish_before_commit( db_session: AsyncSession, monkeypatch: pytest.MonkeyPatch ) -> None: - """The bus event must NOT fire until the session commits (F107). - - Currently RED: ``deliver`` publishes immediately, so the bus is non-empty - before any commit — the phantom window. With the deferred-publish fix, - ``deliver`` only schedules; the event fires on commit. - """ + """The bus event must NOT fire until the session commits — ``deliver`` + only schedules; the event fires on commit.""" bus = _RecordingBus() monkeypatch.setattr( "roboco.services.notification_delivery.get_event_bus", lambda: bus @@ -152,7 +139,7 @@ async def test_deliver_does_not_publish_before_commit( async def test_deliver_publishes_after_commit( db_session: AsyncSession, monkeypatch: pytest.MonkeyPatch ) -> None: - """Commit drains the deferred publish — one event per recipient (F107).""" + """Commit drains the deferred publish — one event per recipient.""" bus = _RecordingBus() monkeypatch.setattr( "roboco.services.notification_delivery.get_event_bus", lambda: bus @@ -179,7 +166,7 @@ async def test_deliver_rollback_drops_phantom( db_session: AsyncSession, monkeypatch: pytest.MonkeyPatch ) -> None: """A rollback instead of commit drops the pending publish — no phantom - event for a row that never became durable (F107).""" + event for a row that never became durable.""" bus = _RecordingBus() monkeypatch.setattr( "roboco.services.notification_delivery.get_event_bus", lambda: bus diff --git a/tests/integration/test_task_service_basics.py b/tests/integration/test_task_service_basics.py index 65500095..08481fe8 100644 --- a/tests/integration/test_task_service_basics.py +++ b/tests/integration/test_task_service_basics.py @@ -1467,10 +1467,9 @@ async def _gate_task(task_setup: dict, db_session: AsyncSession) -> Any: async def test_pr_gate_claim_rejects_second_reviewer_race( task_setup: dict, db_session: AsyncSession ) -> None: - """F114: a second PR-reviewer race-claiming a gate task already claimed by a - reviewer must be refused (last-write-wins would otherwise overwrite the - first reviewer's claim and the first reviewer's pr_pass/pr_fail would - actor-mismatch).""" + """A second PR-reviewer race-claiming a gate task already claimed by another + reviewer is refused, so the first reviewer's claim and subsequent + pr_pass/pr_fail actor-checks are not overwritten.""" svc = task_setup["svc"] reviewer1 = _reviewer("R1") reviewer2 = _reviewer("R2") @@ -1497,10 +1496,9 @@ async def test_pr_gate_claim_rejects_second_reviewer_race( async def test_pr_gate_claim_allows_first_reviewer_when_pm_owns_root( task_setup: dict, db_session: AsyncSession ) -> None: - """F114 regression guard: the gate task is owned by the PM at entry - (submit_for_review does not clear ownership), so the FIRST reviewer must - still be allowed to claim — the guard only rejects a competing REVIEWER - claim, not the PM owner.""" + """The first reviewer can still claim a gate task owned by the PM at entry + (submit_for_review does not clear ownership); the guard only rejects a + competing REVIEWER claim, not the PM owner.""" svc = task_setup["svc"] pm = _pm("PM") reviewer = _reviewer("R") @@ -1526,8 +1524,8 @@ async def test_pr_gate_claim_allows_first_reviewer_when_pm_owns_root( async def test_pr_gate_claim_idempotent_for_same_reviewer( task_setup: dict, db_session: AsyncSession ) -> None: - """F114: a reviewer re-claiming its OWN gate claim is idempotent (allowed), - not rejected — the guard only refuses a DIFFERENT reviewer.""" + """A reviewer re-claiming its own gate claim is idempotent (allowed); the + guard only refuses a different reviewer.""" svc = task_setup["svc"] reviewer = _reviewer("R") db_session.add(reviewer) diff --git a/tests/integration/test_task_service_lifecycle_misc.py b/tests/integration/test_task_service_lifecycle_misc.py index b9d427f7..615ba934 100644 --- a/tests/integration/test_task_service_lifecycle_misc.py +++ b/tests/integration/test_task_service_lifecycle_misc.py @@ -453,14 +453,10 @@ async def test_create_work_session_no_project_returns_none( async def test_create_work_session_delegates_to_service_create( task_setup: dict, db_session: AsyncSession, monkeypatch: pytest.MonkeyPatch ) -> None: - """F113: the claim path must create the WorkSession through the validated - ``WorkSessionService.create`` (the single source of truth), not construct a - ``WorkSessionTable`` directly. Two divergent creation sites had drifted and - bypassed the service-layer validation (existing-active check, supersede - invariant, project/task existence). Routing through ``create`` collapses - them to one validated path. The derived target_branch (parent branch for - subtasks, project default for roots) is passed in via ``WorkSessionCreate``. - """ + """The claim path must create the WorkSession via ``WorkSessionService.create`` + (single source of truth) rather than constructing a ``WorkSessionTable`` + directly, so service-layer validation (existing-active check, supersede + invariant) is not bypassed.""" svc = task_setup["svc"] task = await svc.create(_req(task_setup)) task.branch_name = "feature/backend/delegate" diff --git a/tests/integration/test_task_service_transitions.py b/tests/integration/test_task_service_transitions.py index 7cc0ed17..5aa7b57f 100644 --- a/tests/integration/test_task_service_transitions.py +++ b/tests/integration/test_task_service_transitions.py @@ -1657,11 +1657,9 @@ async def test_submit_for_pm_review_advances_with_notes( async def test_submit_for_pm_review_waives_branch_pr_for_batch_umbrella( task_setup: dict, db_session: AsyncSession ) -> None: - """F001: a MegaTask umbrella is branchless by design (no branch/PR) yet - must walk in_progress -> awaiting_pm_review so main_pm_complete can - escalate it to the CEO. submit_for_pm_review must waive the branch+PR - requirement for a batch umbrella, or umbrella completion deadlocks in - in_progress forever (the Main PM loops on `complete` -> invalid_state).""" + """A MegaTask umbrella is branchless by design yet must walk + in_progress -> awaiting_pm_review; submit_for_pm_review waives the + branch+PR requirement for a batch umbrella so completion does not deadlock.""" svc = task_setup["svc"] task = await svc.create(_req(task_setup)) task.status = TaskStatus.IN_PROGRESS @@ -1682,13 +1680,10 @@ async def test_submit_for_pm_review_waives_branch_pr_for_batch_umbrella( async def test_activate_batch_root_subtasks_retypes_code_to_planning( task_setup: dict, db_session: AsyncSession ) -> None: - """F002: a board-routed MegaTask root-subtask is created in BACKLOG with - team=board and task_type=code (intake only coerces main_pm-team drafts, so a - board-routed code root-subtask reaches activation still code-typed). When - the CEO approves the umbrella, _activate_batch_root_subtasks flips the held - child to team=main_pm — but if it leaves task_type=code the combo - re-introduces the 2026-06-27 main_pm+code meltdown. The activation must - retype code->planning, mirroring approve_and_start's own retype.""" + """A board-routed MegaTask root-subtask is created in BACKLOG with + task_type=code; _activate_batch_root_subtasks must retype it code->planning + when flipping team to main_pm, mirroring approve_and_start, or the + main_pm+code combo recurs.""" svc = task_setup["svc"] # approve_and_start resolves the main-pm agent by slug — seed it. main_pm = AgentTable( diff --git a/tests/unit/api/routes/v1/test_do_auth.py b/tests/unit/api/routes/v1/test_do_auth.py index fd66546c..df397a92 100644 --- a/tests/unit/api/routes/v1/test_do_auth.py +++ b/tests/unit/api/routes/v1/test_do_auth.py @@ -1,4 +1,4 @@ -"""F003/F014: /api/v1/do/* must enforce the same HMAC agent-token gate as +"""/api/v1/do/* must enforce the same HMAC agent-token gate as the /api/v1/flow/* routers. The do router serves every role (content tools are role-uniform), so it has diff --git a/tests/unit/api/routes/v1/test_flow_doc.py b/tests/unit/api/routes/v1/test_flow_doc.py index 5a10741e..c67d48c8 100644 --- a/tests/unit/api/routes/v1/test_flow_doc.py +++ b/tests/unit/api/routes/v1/test_flow_doc.py @@ -194,10 +194,8 @@ async def test_resume_dispatches() -> None: @pytest.mark.asyncio async def test_i_am_blocked_dispatches_to_choreographer() -> None: - """F015: POST /api/v1/flow/documenter/i_am_blocked must exist (the - documenter manifest registers i_am_blocked) and return an envelope, not 404 - with a non-envelope body. Without this route a blocked documenter's escape - hatch 404s.""" + """POST /api/v1/flow/documenter/i_am_blocked returns an envelope (the + documenter manifest registers i_am_blocked) rather than a raw 404.""" mock_chore = MagicMock() mock_chore.i_am_blocked = AsyncMock( return_value=_make_envelope(status="blocked", task_id=_TASK_ID) diff --git a/tests/unit/api/routes/v1/test_flow_main_pm.py b/tests/unit/api/routes/v1/test_flow_main_pm.py index f6beef65..c667a312 100644 --- a/tests/unit/api/routes/v1/test_flow_main_pm.py +++ b/tests/unit/api/routes/v1/test_flow_main_pm.py @@ -366,13 +366,9 @@ async def test_resume_dispatches() -> None: @pytest.mark.asyncio async def test_triage_route_exists_and_dispatches() -> None: - """F067: POST /api/v1/flow/main_pm/triage must exist and wire to - choreographer.triage. The main_pm manifest (from lifecycle.intents_for_role) - advertises `triage` alongside `triage_all`, so a main_pm agent calling - `triage` must hit a real route — not a raw 404 that bypasses the circuit - breaker. Mirrors flow_cell_pm's /triage route (the choreographer.triage - impl is team-scoped and works for any PM role). - """ + """POST /api/v1/flow/main_pm/triage wires to choreographer.triage (the + main_pm manifest advertises `triage` alongside `triage_all`); the + team-scoped choreographer.triage impl works for any PM role.""" mock_chore = MagicMock() mock_chore.triage = AsyncMock(return_value=_make_envelope(status="idle")) client = TestClient(_build_app(mock_chore)) diff --git a/tests/unit/api/routes/v1/test_flow_qa.py b/tests/unit/api/routes/v1/test_flow_qa.py index 8e66260c..2a826209 100644 --- a/tests/unit/api/routes/v1/test_flow_qa.py +++ b/tests/unit/api/routes/v1/test_flow_qa.py @@ -223,9 +223,8 @@ async def test_i_am_idle_dispatches_agent_id() -> None: @pytest.mark.asyncio async def test_i_am_blocked_dispatches_to_choreographer() -> None: - """F015: POST /api/v1/flow/qa/i_am_blocked must exist (the QA manifest - registers i_am_blocked) and return an envelope, not 404 with a non-envelope - body. Without this route a blocked QA agent's escape hatch 404s.""" + """POST /api/v1/flow/qa/i_am_blocked returns an envelope (the QA manifest + registers i_am_blocked) rather than a raw 404.""" mock_chore = MagicMock() mock_chore.i_am_blocked = AsyncMock( return_value=_make_envelope(status="blocked", task_id=_TASK_ID) diff --git a/tests/unit/api/schemas/v1/test_note_request_no_null.py b/tests/unit/api/schemas/v1/test_note_request_no_null.py index 50f9c88a..b0af1598 100644 --- a/tests/unit/api/schemas/v1/test_note_request_no_null.py +++ b/tests/unit/api/schemas/v1/test_note_request_no_null.py @@ -117,11 +117,8 @@ def test_note_request_coerces_string_next_steps_to_list() -> None: def test_note_request_coerces_string_where_to_look_to_list() -> None: - """F118: a single string for where_to_look is wrapped into a one-element - list. It is a list-typed handoff field like consequences/next_steps and - must tolerate a lone scalar — without this a well-intentioned - ``where_to_look="src/api/"`` 422'd at the route (no remediation envelope) - and the agent's retry loop tripped the do-server circuit breaker.""" + """A single string for where_to_look is wrapped into a one-element list, + mirroring consequences/next_steps, so a lone scalar does not 422 the route.""" req = NoteRequest.model_validate( {"text": "x", "scope": "handoff", "where_to_look": "src/api/auth.py"} ) diff --git a/tests/unit/api/test_a2a_message_auth.py b/tests/unit/api/test_a2a_message_auth.py index 2298e0ef..45078c6b 100644 --- a/tests/unit/api/test_a2a_message_auth.py +++ b/tests/unit/api/test_a2a_message_auth.py @@ -1,14 +1,6 @@ -"""F023: POST /api/a2a/message/send and /message/stream must enforce the same -HMAC agent-token gate as the /api/v1/do/* router (F003). - -Both routes previously took only ``request: SendMessageRequest, db: DbSession`` -— no auth dependency. The sender was self-declared in the request body -(``metadata.from_agent``), so any caller could impersonate any agent and -inject A2A notifications that the orchestrator dispatcher picks up to spawn -target agents. The fix reuses F003's ``require_any_authenticated_agent`` -(token-only, DB-free, no role assertion — the a2a router serves every role). -In dev (header-trust) mode a missing token is a no-op; a presented-but-forged -token is still rejected, exactly as the do router does. +"""POST /api/a2a/message/send and /message/stream enforce the same HMAC +agent-token gate as the /api/v1/do/* router (``require_any_authenticated_agent``, +token-only, DB-free, no role assertion — the a2a router serves every role). """ from __future__ import annotations diff --git a/tests/unit/api/test_a2a_subscribe.py b/tests/unit/api/test_a2a_subscribe.py index 4f3b8728..7f3b1a4b 100644 --- a/tests/unit/api/test_a2a_subscribe.py +++ b/tests/unit/api/test_a2a_subscribe.py @@ -1,12 +1,8 @@ -"""F024: the SSE ``subscribe_to_task`` endpoint must (a) be authenticated -like the rest of the a2a message surface (F023) and (b) acquire a SHORT-LIVED -DB session per poll iteration instead of holding the request-scoped -``db: DbSession`` for the full SSE lifetime (up to 1 hour / 720 polls), which -exhausted the asyncpg pool one connection per connected client. - -The fix mirrors F003's ``require_any_authenticated_agent`` for auth and uses -``get_session_factory()`` inside the generator so each poll opens, queries, -and closes its own session — no connection is held across ``asyncio.sleep``. +"""SSE ``subscribe_to_task`` is authenticated like the rest of the a2a +message surface and opens a SHORT-LIVED DB session per poll iteration +(via ``get_session_factory()``) instead of holding the request-scoped +``db: DbSession`` for the full SSE lifetime, which exhausted the asyncpg +pool one connection per connected client. """ from __future__ import annotations @@ -112,9 +108,9 @@ async def test_subscribe_accepts_valid_token_then_404s_unknown_task( def test_subscribe_route_does_not_hold_request_scoped_db() -> None: - """F024: the route must NOT depend on ``get_db`` — the request-scoped - session would be held for the full SSE lifetime (up to 1 hour). Each - poll must open its own short-lived session via ``get_session_factory``. + """The route must NOT depend on ``get_db`` — the request-scoped session + would be held for the full SSE lifetime (up to 1 hour). Each poll opens + its own short-lived session via ``get_session_factory``. """ subscribe_route = cast( "APIRoute", @@ -146,13 +142,10 @@ def test_subscribe_route_does_not_hold_request_scoped_db() -> None: async def test_subscribe_opens_a_short_lived_session_per_poll( a2a_client: AsyncClient, monkeypatch: pytest.MonkeyPatch ) -> None: - """F024: each poll iteration opens its own session and closes it before - the next ``asyncio.sleep`` — never holding one connection across the full - SSE lifetime. We patch ``get_session_factory`` to count session opens, - patch ``A2AService.get_task`` to return a non-terminal task, patch - ``asyncio.sleep`` to no-op, and make ``request.is_disconnected`` return - True after a few polls to terminate the stream quickly. The count of - session opens must exceed 1 (one per poll, not one for the lifetime).""" + """Each poll iteration opens its own session and closes it before the next + ``asyncio.sleep`` — never holding one connection across the full SSE + lifetime. Asserts more than one session open (one per poll, not one for + the lifetime).""" monkeypatch.setenv("ROBOCO_AGENT_AUTH_SECRET", _SECRET) monkeypatch.setenv("ROBOCO_AGENT_AUTH_REQUIRED", "true") diff --git a/tests/unit/api/test_app.py b/tests/unit/api/test_app.py index c1d7784d..e608f550 100644 --- a/tests/unit/api/test_app.py +++ b/tests/unit/api/test_app.py @@ -147,12 +147,10 @@ async def test_lifespan_startup_and_shutdown_happy_path() -> None: @pytest.mark.asyncio async def test_lifespan_stops_orchestrator_before_closing_db_and_optimal() -> None: - """F117: orchestrator.stop() must run BEFORE close_optimal_service / close_db - on shutdown. stop() drains fire-and-forget DB writes (respawn_tracker - upserts, audit-log rows) and stop_agent finalizes work sessions / agent - state — all needing the DB still open. Closing the DB first (the old order, - where only bootstrap's finally called stop() after lifespan had already - closed the DB) silently dropped those final writes.""" + """orchestrator.stop() runs BEFORE close_optimal_service / close_db on + shutdown — stop() drains fire-and-forget DB writes (respawn_tracker + upserts, audit-log rows) and finalizes work sessions, all needing the + DB still open.""" order: list[str] = [] def _record(label: str) -> AsyncMock: diff --git a/tests/unit/api/test_dashboard_auditor_auth.py b/tests/unit/api/test_dashboard_auditor_auth.py index a96a30eb..641c15f0 100644 --- a/tests/unit/api/test_dashboard_auditor_auth.py +++ b/tests/unit/api/test_dashboard_auditor_auth.py @@ -1,12 +1,7 @@ -"""F025: dashboard auditor flag/report mutating routes must be gated to the -Auditor or CEO. - -``create_auditor_flag`` / ``resolve_auditor_flag`` / ``create_auditor_report`` -/ ``send_auditor_report`` previously took only ``db: DbSession`` — no -``CurrentAgentContext``, no role check — so any unauthenticated caller could -create/resolve flags and mark reports as sent to the CEO. The fix mirrors -``roboco/api/routes/playbooks.py::_require_curator``: a ``CurrentAgentContext`` -dependency plus a coarse role gate that admits only ``AUDITOR`` and ``CEO``. +"""Dashboard auditor flag/report mutating routes (``create_auditor_flag``, +``resolve_auditor_flag``, ``create_auditor_report``, ``send_auditor_report``) +are gated to AUDITOR or CEO via a ``CurrentAgentContext`` dependency plus a +coarse role gate, mirroring ``roboco/api/routes/playbooks.py::_require_curator``. """ from __future__ import annotations diff --git a/tests/unit/api/test_middleware.py b/tests/unit/api/test_middleware.py index 8a304238..a516d638 100644 --- a/tests/unit/api/test_middleware.py +++ b/tests/unit/api/test_middleware.py @@ -279,7 +279,7 @@ def test_request_validation_handler_returns_422_with_details() -> None: # --------------------------------------------------------------------------- -# F022: secret scrubbing in the 422 log line +# secret scrubbing in the 422 log line # --------------------------------------------------------------------------- @@ -296,10 +296,10 @@ class _SecretBody(BaseModel): def test_request_validation_handler_scrubs_secrets_from_log() -> None: - """F022: a 422 on a secret-bearing request must not dump the plaintext - secret into the log line — only the redacted placeholder. The 422 - response body is unchanged (the client sent those values; the server - only redacts its own log).""" + """A 422 on a secret-bearing request must not dump the plaintext secret + into the log line — only the redacted placeholder. The 422 response body + is unchanged (the client sent those values; the server only redacts its + own log).""" app = FastAPI() setup_middleware(app) @@ -354,8 +354,8 @@ def test_request_validation_handler_scrubs_secrets_from_log() -> None: def test_request_validation_handler_log_preserves_non_secret_fields() -> None: - """F022: non-secret fields in the body are still logged in full — only - the known credential-looking field names are redacted.""" + """Non-secret fields in the body are still logged in full — only the + known credential-looking field names are redacted.""" app = FastAPI() setup_middleware(app) diff --git a/tests/unit/api/test_orchestrator_auth.py b/tests/unit/api/test_orchestrator_auth.py index e93628ec..6fb81f91 100644 --- a/tests/unit/api/test_orchestrator_auth.py +++ b/tests/unit/api/test_orchestrator_auth.py @@ -1,14 +1,8 @@ -"""F026: orchestrator control routes (/api/orchestrator/*) must be gated to -the CEO/operator identity. - -``spawn_agent`` / ``stop_agent`` / ``resolve_wait`` / ``mark_waiting`` previously -took no auth dependency at all — any client that could reach the API could -spawn, stop, mark-waiting, or resolve-wait any agent. The fix mirrors the -F004 panel-token guard (DB-free): bind the presented ``X-Agent-ID`` to a -verified HMAC token and assert the role is CEO. In dev (header-trust) mode a -missing token is a no-op (the panel/operator flow keeps working), but a -presented-but-forged token is still rejected — same contract as the v1 flow -role guards and the do router (F003). +"""Orchestrator control routes (/api/orchestrator/*) are gated to the +CEO/operator identity: the presented ``X-Agent-ID`` is bound to a verified +HMAC token (DB-free panel-token guard) and the role asserted as CEO. In dev +(header-trust) mode a missing token is a no-op; a presented-but-forged token +is still rejected. """ from __future__ import annotations diff --git a/tests/unit/api/test_websocket_auth.py b/tests/unit/api/test_websocket_auth.py index 428dcea9..55c106eb 100644 --- a/tests/unit/api/test_websocket_auth.py +++ b/tests/unit/api/test_websocket_auth.py @@ -1,13 +1,8 @@ -"""F004: WebSocket streams must enforce the HMAC panel/CEO token gate when -ROBOCO_AGENT_AUTH_REQUIRED=true. - -The /ws/* streams are operator-only (the panel is the sole WS client; agents -use MCP verbs, not WS). nginx injects the CEO panel token as X-Agent-Token on -/ws/ upgrades, but the endpoints never read or verified it — so in strict mode -an agent on the Docker network could hit /ws/notifications/{id} directly and -subscribe to another agent's notifications with no auth. The fix binds each -per-agent WS upgrade to the CEO token: require + verify it in strict mode, and -reject a forged token even in dev mode (same contract as the HTTP role gates). +"""WebSocket streams (/ws/*, operator-only — the panel is the sole WS client) +enforce the HMAC panel/CEO token gate when ROBOCO_AGENT_AUTH_REQUIRED=true: +each per-agent WS upgrade requires + verifies the CEO token in strict mode +and rejects a forged token even in dev mode (same contract as the HTTP role +gates). """ from __future__ import annotations diff --git a/tests/unit/api/test_websocket_handler_cleanup.py b/tests/unit/api/test_websocket_handler_cleanup.py index 74bf2f59..5e2e5229 100644 --- a/tests/unit/api/test_websocket_handler_cleanup.py +++ b/tests/unit/api/test_websocket_handler_cleanup.py @@ -1,21 +1,11 @@ -"""F065: WS route handlers must disconnect on ANY exit path, not just +"""WS route handlers must disconnect on ANY exit path, not just WebSocketDisconnect. -The old handlers were ``try: ... while True: receive_text() ... except -WebSocketDisconnect: manager.disconnect(websocket)`` with NO ``finally``. -If ``receive_text()`` raised anything else (anyio closed-resource during -shutdown, ``asyncio.CancelledError``, transport errors), the exception -propagated WITHOUT calling ``manager.disconnect(websocket)``, so the dead -socket stayed in the subscription set + ``connection_agents`` forever and -was still fanned out to on every broadcast. - -The fix adds ``finally: manager.disconnect(websocket)`` to every handler. -``disconnect`` is idempotent (``set.discard`` / ``dict.pop`` with default), -so the clean-disconnect path (still caught by ``except WebSocketDisconnect`` -for clarity) and the new finally both calling it is safe. - -These tests use mock sockets (no real app/Redis) and an isolated -``ConnectionManager`` patched in for the module-global ``manager``. +Each handler adds ``finally: manager.disconnect(websocket)``; ``disconnect`` +is idempotent (``set.discard`` / ``dict.pop`` with default), so the +clean-disconnect path and the finally both calling it is safe. Tests use +mock sockets (no real app/Redis) and an isolated ``ConnectionManager`` +patched in for the module-global ``manager``. """ from __future__ import annotations diff --git a/tests/unit/api/test_websocket_idle_timeout.py b/tests/unit/api/test_websocket_idle_timeout.py index 3c96a443..d1af9cd0 100644 --- a/tests/unit/api/test_websocket_idle_timeout.py +++ b/tests/unit/api/test_websocket_idle_timeout.py @@ -1,19 +1,11 @@ -"""F066: server-side idle timeout reaps half-open WS sockets. +"""Server-side idle timeout reaps half-open WS sockets. -The keepalive was client-driven (respond to ``"ping"`` with ``"pong"``); the -server never sent its own ping and never timed out a silent client. If an -agent container died leaving the TCP socket half-open, ``receive_text()`` -blocked forever and ``disconnect`` was never called. - -The fix wraps ``receive_text()`` in -``asyncio.wait_for(..., timeout=IDLE_TIMEOUT_SECONDS)`` per handler; on -``asyncio.TimeoutError`` the finally (from F065) disconnects the idle -socket. ``IDLE_TIMEOUT_SECONDS`` is a named module constant (ruff PLR2004). - -Deterministic: the slow-socket test patches ``IDLE_TIMEOUT_SECONDS`` to a -tiny value (0.05s) and uses a ``receive_text`` that returns a never-resolved -``Future`` — so the test asserts a prompt disconnect in well under a second, -never relying on real wall-clock timing of the default timeout. +Each handler wraps ``receive_text()`` in +``asyncio.wait_for(..., timeout=IDLE_TIMEOUT_SECONDS)``; on timeout the +handler's ``finally`` disconnects the idle socket. ``IDLE_TIMEOUT_SECONDS`` +is a named module constant (ruff PLR2004). Tests patch it to a tiny value +and use a never-resolved ``Future`` so assertions hold in well under a +second, never relying on real wall-clock timing. """ from __future__ import annotations diff --git a/tests/unit/api/test_websocket_send_queue.py b/tests/unit/api/test_websocket_send_queue.py index 6b575f26..484454c0 100644 --- a/tests/unit/api/test_websocket_send_queue.py +++ b/tests/unit/api/test_websocket_send_queue.py @@ -1,24 +1,13 @@ -"""F064: per-connection send queue + send timeout — one slow WS client must -not back-pressure ALL event delivery to ALL clients. +"""Per-connection send queue + send timeout — one slow WS client must not +back-pressure ALL event delivery to ALL clients. -The old broadcast did ``await asyncio.gather(*[conn.send_text(data) for conn -in connections], return_exceptions=True)`` with no per-connection send queue -and no send timeout. If one client was slow to drain, ``conn.send_text(data)`` -awaited indefinitely on the transport, blocking the gather → the bridge -handler → ``_dispatch_event`` → the whole ``_listen_loop`` for every event -type and recipient. - -The fix gives each registered connection a bounded send queue + a sender -coroutine that drains it, with ``send_text`` behind +Each registered connection gets a bounded send queue + a sender coroutine +that drains it, with ``send_text`` behind ``asyncio.wait_for(..., timeout=SEND_TIMEOUT_SECONDS)``. Broadcasts become -fire-and-enqueue: a slow client's queue fills, then drops/overflows (logged -as a warning) instead of blocking the fan-out. The listen loop is never -blocked on a single client. - -Determinism: every slow-send test uses a ``receive``/``send_text`` that -awaits a never-resolved ``Future`` and patches ``SEND_TIMEOUT_SECONDS`` to a -tiny value, so assertions hold in well under a second and never rely on real -wall-clock timing of the default timeout. +fire-and-enqueue: a slow client's queue fills, then drops/overflows (logged) +instead of blocking the fan-out. Tests patch ``SEND_TIMEOUT_SECONDS`` to a +tiny value and use a never-resolved ``Future`` so assertions hold in well +under a second, never relying on real wall-clock timing. """ from __future__ import annotations @@ -223,21 +212,17 @@ async def test_broadcast_send_timeout_protects_legacy_unregistered_socket() -> N # --------------------------------------------------------------------------- -# Send-side failure proactively reaps the dead socket (F119) +# Send-side failure proactively reaps the dead socket. # --------------------------------------------------------------------------- @pytest.mark.asyncio async def test_sender_triggers_disconnect_on_send_error() -> None: - """F119: when send_text raises (transport closed / dead socket), the sender - task must proactively disconnect the socket from every subscription set — - not just stop sending and wait for the receive loop's idle timeout to reap - it. Without this a send-side-detected dead socket lingers in the sets and - broadcasts keep enqueuing into a queue whose consumer has exited - (queue-overflow log spam, then silent drops) for up to IDLE_TIMEOUT_SECONDS - until the receive loop's idle timeout finally fires. The send path provably - failed, so it should clean up immediately — the receive-side-only reap was - the gap the audit flagged.""" + """When ``send_text`` raises (transport closed / dead socket), the sender + task must proactively disconnect the socket from every subscription set + rather than wait for the receive loop's idle timeout — otherwise a + send-side-detected dead socket lingers and broadcasts keep enqueuing into + a queue whose consumer has exited.""" mgr = ConnectionManager() dead_ws = _make_ws(send_side_effect=ConnectionError("transport closed")) await mgr.connect_system(dead_ws) @@ -257,10 +242,10 @@ async def test_sender_triggers_disconnect_on_send_error() -> None: @pytest.mark.asyncio async def test_sender_keeps_live_socket_on_send_timeout_only() -> None: - """F119: a send TIMEOUT alone (slow client, not a dead socket) must NOT - disconnect the socket — only a hard send Exception (transport closed) does. - A slow-but-live client should keep receiving once it drains; timing it out - is the existing F064 graceful-degradation path, not a reap trigger.""" + """A send TIMEOUT alone (slow client, not a dead socket) must NOT disconnect + the socket — only a hard send Exception (transport closed) does. A + slow-but-live client should keep receiving once it drains; timing it out + is the graceful-degradation path, not a reap trigger.""" mgr = ConnectionManager() hang: asyncio.Future[None] = asyncio.Future() slow_ws = _make_ws(send_side_effect=hang) diff --git a/tests/unit/gateway/test_choreographer_claim_lock.py b/tests/unit/gateway/test_choreographer_claim_lock.py index 47c280c1..afcda9d5 100644 --- a/tests/unit/gateway/test_choreographer_claim_lock.py +++ b/tests/unit/gateway/test_choreographer_claim_lock.py @@ -1,28 +1,14 @@ -"""F074 — the one-task-per-agent invariant had no DB-level enforcement. +"""The one-task-per-agent invariant is enforced at the DB level by a +PostgreSQL transaction-scoped advisory lock keyed by agent_id, acquired in +``_claim_plan_start_gate`` BEFORE the guard reads (non-coordinator roles +only) and held until the request transaction commits, so a second concurrent +claim's guard read sees the first's committed in_progress task and is +rejected. -``_run_claim_guards`` read the agent's other tasks via unlocked SELECTs -(``list_in_progress_for_agent`` / ``list_paused_for_agent``) BEFORE ``claim()`` -took its row lock, and ``claim()``'s ``FOR UPDATE`` locked only the TARGET row -— not the agent-wide invariant. So two concurrent ``i_will_work_on`` calls by -the SAME agent on TWO DIFFERENT pending tasks each locked their own target row -(no contention), each read an empty in_progress set, each passed -``already_active_guard``, and each claim+start succeeded → the agent ended with -two in_progress tasks. The in-process ``asyncio.Lock`` serializes container -spawns per agent but is lost on orchestrator-restart split-brain, so it is not -a DB-level guarantee. - -The fix: a PostgreSQL transaction-scoped advisory lock keyed by agent_id, -acquired in ``_claim_plan_start_gate`` BEFORE the guard reads (for non- -coordinator roles only). Held until the request transaction commits, it spans -the guard read + the savepoint + the claim write, so the second concurrent -claim's guard read sees the first's committed in_progress task and is rejected. - -CRITICAL logical-regression guard: the advisory lock is acquired ONLY for non- -coordinator roles. The PM coordinator concurrency feature (CLAUDE.md) lets a -cell_pm / main_pm plan + delegate many roots in parallel — acquiring a per- -agent advisory lock for a coordinator would serialize those claims and -REGRESS that feature. So coordinators are exempt (matching the existing -``_COORDINATOR_ROLES`` guard exemption for ``already_active`` / ``paused``). +CRITICAL regression guard: the lock is acquired ONLY for non-coordinator +roles — acquiring it for a coordinator would serialize a cell_pm / main_pm's +parallel root planning and regress coordinator concurrency (matches the +``_COORDINATOR_ROLES`` guard exemption). """ from __future__ import annotations @@ -281,22 +267,17 @@ async def test_coordinator_claim_does_not_acquire_lock() -> None: # --------------------------------------------------------------------------- -# F124: the unmet_dependency guard reads dependency state via an unlocked -# SELECT, then fires release_dependency_blocked_claim (a state mutation: -# claimed/in_progress -> pending, clears branch_name, abandons WorkSession) -# as a side-effect BEFORE returning the rejection. If an upstream dependency -# completes (transitions to completed/cancelled) in the microseconds between -# the read and the release, the task is NEEDLESSLY released — its branch -# cleared + WorkSession abandoned + assignee bounced, only to be re-dispatched -# + re-claimed when the dependency-completion re-dispatch fires. Dependencies -# are monotonic (unmet -> met, terminal: completed/cancelled never reopen), so -# a fresh re-read that now finds them met stays met: safe to proceed without -# releasing. The fix re-checks unmet_dependency_ids immediately before the -# release and skips it (returning None — proceed) when the upstream just -# completed. The "still unmet" path is byte-for-byte the prior behavior. +# the unmet_dependency guard reads dependency state via an unlocked SELECT, +# then fires release_dependency_blocked_claim (claimed/in_progress -> pending, +# clears branch_name, abandons WorkSession) BEFORE returning the rejection. If +# the upstream completes in the microseconds between the read and the release, +# the task is NEEDLESSLY released. Dependencies are monotonic (unmet -> met, +# terminal never reopen), so a fresh re-read that now finds them met stays met: +# safe to proceed without releasing. The fix re-checks unmet_dependency_ids +# immediately before the release and skips it when the upstream just completed. # --------------------------------------------------------------------------- -# Initial dependency read + the re-check before release (F124). +# Initial dependency read + the re-check before release. _DEP_READ_INITIAL_PLUS_RECHECK = 2 @@ -320,12 +301,9 @@ def _dep_task_svc(agent_id: object, task_id: object, dep_id: object) -> AsyncMoc @pytest.mark.asyncio async def test_dependency_guard_skips_release_when_upstream_just_completed() -> None: - """F124: the first dependency read sees the upstream still unmet, but by the - re-check (a few microseconds later) it has completed. The guard must NOT - release the task — the dependency is now met, so the task can proceed. - Releasing would needlessly clear its branch + abandon its WorkSession only - to be re-dispatched + re-claimed when the dependency-completion re-dispatch - fires. Returns None (proceed), no release.""" + """The first dependency read sees the upstream still unmet, but the re-check + sees it completed; the guard must NOT release the task (the dependency is + now met). Returns None (proceed), no release.""" agent_id = uuid4() task_id = uuid4() dep_id = uuid4() @@ -353,10 +331,10 @@ async def test_dependency_guard_skips_release_when_upstream_just_completed() -> @pytest.mark.asyncio async def test_dependency_guard_releases_when_still_unmet_no_regression() -> None: - """F124 no-regression: both the first read AND the re-check see the upstream - still unmet. The guard releases the task to pending (stopping respawn churn - into a blocked task) and returns the rejection — byte-for-byte the prior - behavior. The re-check must not weaken the genuine-blocked release path.""" + """No-regression: both the first read AND the re-check see the upstream + still unmet; the guard releases the task to pending and returns the + rejection, so the re-check must not weaken the genuine-blocked release + path.""" agent_id = uuid4() task_id = uuid4() dep_id = uuid4() diff --git a/tests/unit/gateway/test_choreographer_completion_guards.py b/tests/unit/gateway/test_choreographer_completion_guards.py index 3e3eb36c..c54310cd 100644 --- a/tests/unit/gateway/test_choreographer_completion_guards.py +++ b/tests/unit/gateway/test_choreographer_completion_guards.py @@ -363,15 +363,14 @@ async def test_main_pm_complete_handles_escalate_returning_none() -> None: @pytest.mark.asyncio async def test_complete_escalates_batch_umbrella_from_in_progress() -> None: - """F001: a MegaTask umbrella is branchless by design and sits in - in_progress with no branch/PR. The ``complete`` verb's spec gate - (``complete`` action source_statuses={AWAITING_PM_REVIEW}) must NOT - reject it — the Main PM routes through main_pm_complete, which walks - in_progress -> awaiting_pm_review -> awaiting_ceo_approval. Calling the - ``complete`` ENTRY point (not main_pm_complete directly) must succeed - and escalate to the CEO. This exercises the real spec gate - (can_invoke_intent is pure) — the prior test mocked submit_pm_review and - called main_pm_complete directly, bypassing the gate (false green).""" + """A MegaTask umbrella is branchless by design and sits in ``in_progress`` + with no branch/PR; the ``complete`` verb's spec gate + (``source_statuses={AWAITING_PM_REVIEW}``) must NOT reject it — the Main + PM routes through ``main_pm_complete``, walking in_progress -> + awaiting_pm_review -> awaiting_ceo_approval. Calling the ``complete`` + ENTRY point (not ``main_pm_complete`` directly) must succeed and escalate + to the CEO; this exercises the real spec gate (``can_invoke_intent`` is + pure).""" pm_id = uuid4() umbrella_id = uuid4() batch_id = uuid4() diff --git a/tests/unit/gateway/test_claim_guards_blocked.py b/tests/unit/gateway/test_claim_guards_blocked.py index f5581ff3..b986173c 100644 --- a/tests/unit/gateway/test_claim_guards_blocked.py +++ b/tests/unit/gateway/test_claim_guards_blocked.py @@ -1,12 +1,6 @@ -"""F018 — ``already_active_guard`` must treat a ``blocked`` task as active. - -``_ACTIVE_BLOCKING_STATUSES`` excluded ``blocked``, so a developer with a -blocked task could claim a second task (the guard passed). When the blocked -task was later unblocked via ``unblock_with_restore`` it resumed to -``in_progress`` — leaving the dev silently holding TWO ``in_progress`` tasks, -violating the one-active-task-per-dev invariant the guard exists to enforce. -A blocked task is still owned and will resume to active, so it must block a -new claim. +"""``already_active_guard`` must treat a ``blocked`` task as active. A blocked +task is still owned and will resume to ``in_progress`` on unblock, so it must +block a new claim (preserves the one-active-task-per-dev invariant). """ from __future__ import annotations @@ -25,7 +19,7 @@ def _task(*, status: str) -> MagicMock: def test_already_active_guard_blocks_when_agent_has_blocked_task() -> None: - """A blocked task the dev still owns must block a new claim (F018).""" + """A blocked task the dev still owns must block a new claim.""" target_id = uuid4() blocked = _task(status="blocked") env = already_active_guard([blocked], target_id) diff --git a/tests/unit/gateway/test_conventions_gate_pr_pass.py b/tests/unit/gateway/test_conventions_gate_pr_pass.py index fc4d195f..327b121a 100644 --- a/tests/unit/gateway/test_conventions_gate_pr_pass.py +++ b/tests/unit/gateway/test_conventions_gate_pr_pass.py @@ -82,9 +82,9 @@ async def test_pr_pass_guard_blocks_when_validator_cannot_run( async def test_pr_pass_guard_could_not_run_remediation_uses_pr_fail( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F044: _conventions_guard is the pr_pass (reviewer) path. A reviewer has no + # _conventions_guard is the pr_pass (reviewer) path. A reviewer has no # i_am_blocked verb, so the could_not_run remediation must point at pr_fail - # (the reviewer's reject lever) — not tell them to call a verb they lack. + # (the reviewer's reject lever), not a verb they lack. monkeypatch.setattr(settings, "conventions_enabled", True) c = _make_choreographer(check_result={"findings": [], "could_not_run": True}) env = await c._conventions_guard(uuid4(), MagicMock(), {}) @@ -98,13 +98,11 @@ async def test_pr_pass_guard_could_not_run_remediation_uses_pr_fail( async def test_pr_pass_guard_block_remediation_uses_pr_fail_not_reviewer_waiver( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F047: on the pr_pass (reviewer) path a block-level finding's remediation - # must point at pr_fail (the reviewer's only lever) and frame the waiver as - # the DEV's action — NOT tell the reviewer to "add a waiver to - # .roboco/conventions.yml in your branch". A pr_reviewer does not own the - # assembled cell→root / root→master branch and has no commit verb on it, so - # the shared dev-path waiver remediation is unreachable and would strand the - # gate on every false positive (no self-recovery). + # on the pr_pass (reviewer) path a block-level finding's remediation must + # point at pr_fail (the reviewer's only lever) and frame the waiver as the + # DEV's action — a pr_reviewer does not own the assembled branch and has no + # commit verb on it, so the dev-path waiver remediation would strand the + # gate on every false positive. monkeypatch.setattr(settings, "conventions_enabled", True) c = _make_choreographer(check_result=_BLOCK_RESULT) env = await c._conventions_guard(uuid4(), MagicMock(), {}) diff --git a/tests/unit/gateway/test_delegate_parent_lock.py b/tests/unit/gateway/test_delegate_parent_lock.py index cf2cd74b..7ba9af5b 100644 --- a/tests/unit/gateway/test_delegate_parent_lock.py +++ b/tests/unit/gateway/test_delegate_parent_lock.py @@ -1,32 +1,14 @@ -"""F125 — the delegate sibling-dedup guard had a read/write TOCTOU. +"""The delegate sibling-dedup guard is serialized by a PostgreSQL +transaction-scoped advisory lock keyed by the parent task id, acquired at the +TOP of the delegate body (before the first ``get_subtasks`` read) and held +through ``create_subtask``'s flush + the outer request commit. Different +parents hash to different keys (seed ``1``, disjoint from the per-agent claim +lock's seed ``0``) so cross-parent delegates are not serialized. -``_delegate_sibling_dedup_guard`` reads the parent's existing subtasks via an -unlocked ``get_subtasks`` SELECT (the dedup read), then the verb body calls -``create_subtask`` (the write) — with no DB serialization between the two. Two -concurrent ``delegate`` calls for the SAME parent (a PM re-delegating while a -stale-heartbeat reaper unclaims + re-dispatches, or two orchestrator ticks -racing) each read an empty/duplicate-free sibling set, each pass the dedup -guard, and each create a subtask → the parent gets the duplicate the guard -exists to prevent (the smoke-run runaway pattern the guard was built for). - -The fix: a PostgreSQL transaction-scoped advisory lock keyed by the parent -task id, acquired at the TOP of the delegate body — before the first -``get_subtasks`` read (the briefing's context read AND the dedup guard's -sibling read) and held through ``create_subtask``'s flush + the outer request -commit. The second concurrent same-parent delegate blocks on the lock until -the first commits; its dedup read then sees the first's committed sibling and -is rejected. Different parents hash to different keys (seed ``1``, disjoint -from the per-agent claim lock's seed ``0``) so cross-parent delegates are not -serialized — the PM coordinator concurrency feature (parallel root planning) -is preserved. - -CRITICAL logical-regression guard: the lock is per-PARENT, not per-agent. A -single cell_pm / main_pm legitimately delegates many subtasks under one parent -in quick succession (a per-dev sequenced queue), and a coordinator PM plans -many roots in parallel. A per-agent lock would serialize all of a PM's -delegates and regress coordinator concurrency; a per-parent lock serializes -only same-parent delegates (the actual dedup invariant is per-parent) and -leaves different parents untouched. +CRITICAL regression guard: the lock is per-PARENT, not per-agent. A per-agent +lock would serialize all of a coordinator PM's delegates and regress +coordinator concurrency; the dedup invariant is per-parent, so only same-parent +delegates serialize. """ from __future__ import annotations diff --git a/tests/unit/gateway/test_evidence_builder.py b/tests/unit/gateway/test_evidence_builder.py index a56dc036..4b5548eb 100644 --- a/tests/unit/gateway/test_evidence_builder.py +++ b/tests/unit/gateway/test_evidence_builder.py @@ -190,10 +190,8 @@ class TestTaskHandoff: class TestPrReviewSurface: - """F008 — the persisted pr_fail verdict + issues must surface in the PM - briefing's task_handoff, not just the fire-and-forget a2a. A PM respawned - into ``needs_revision`` after a pr_fail otherwise sees a generic "needs - revision" with zero concrete change-requests and re-submits the same PR.""" + """The persisted pr_fail verdict + issues must surface in the PM + briefing's task_handoff, not just the fire-and-forget a2a.""" def test_surfaces_pr_fail_verdict_and_issues(self) -> None: t = _task(pr_number=138, commits=[{"sha": "abc", "message": "feat: x"}]) diff --git a/tests/unit/gateway/test_i_am_blocked_no_escalation_target.py b/tests/unit/gateway/test_i_am_blocked_no_escalation_target.py index 5a57d158..92eb499a 100644 --- a/tests/unit/gateway/test_i_am_blocked_no_escalation_target.py +++ b/tests/unit/gateway/test_i_am_blocked_no_escalation_target.py @@ -1,21 +1,7 @@ -"""F017 — ``i_am_blocked`` must surface ``invalid_state`` instead of a 500. - -The bug: ``i_am_blocked`` (any non-``rate_limited`` reason) composes the -single ``(block,)`` atomic action, whose handler calls -``TaskService.escalate``. ``escalate`` returns ``None`` in four cases -(no task, no agent, no resolvable escalation-target slug, no target agent -row) — e.g. a developer whose role has no PM above it in -``get_escalation_target``. Because ``block`` is the LAST composed action, -its ``None`` return flows out of ``run_intent`` as the verb's result. The -choreographer then re-binds ``t`` to that ``None`` and dereferences -``t.status`` building the success envelope → ``'NoneType' object has no -attribute 'status'`` → HTTP 500. The agent gets no actionable rejection -and respawn-loops. - -The fix mirrors F016's ``submit_root`` guard: in -``_run_i_am_blocked_intent``, when the runner returns ``None``, emit an -``invalid_state`` rejection (re-fetch + escalate-to-CEO directly) instead -of letting the caller dereference ``None.status``. +"""``i_am_blocked`` must surface ``invalid_state`` instead of 500 when the +block action returns ``None`` (no escalation target resolvable) — the +choreographer emits a re-fetch + escalate-to-CEO rejection rather than +dereferencing ``None.status``. """ from __future__ import annotations @@ -71,7 +57,7 @@ def _make_task_svc(agent_id: object, task_id: object) -> AsyncMock: team="backend", slug="be-dev-1", ) - # F017: escalate resolves no escalation target for this role → None. + # escalate resolves no escalation target for this role → None. task_svc.escalate.return_value = None return task_svc diff --git a/tests/unit/gateway/test_i_am_blocked_rate_limited.py b/tests/unit/gateway/test_i_am_blocked_rate_limited.py index 0e7b9360..2eaeb457 100644 --- a/tests/unit/gateway/test_i_am_blocked_rate_limited.py +++ b/tests/unit/gateway/test_i_am_blocked_rate_limited.py @@ -508,12 +508,9 @@ class TestRateLimitTrackerActivateOnParking: assert env.status == "in_progress" async def test_activate_failure_is_logged_not_silent(self) -> None: - """F045: an activate() failure must be logged loudly, not bare-suppressed. - - The probe-resume loop is tracker-driven, so a silent activate failure - strands every parked agent in WAITING_LONG with no probe ever running. - A loud error log makes the stranded-fleet condition visible to - operators (and pairs with the orchestrator's in-memory fallback sweep). + """An activate() failure must be logged loudly, not bare-suppressed — + the probe-resume loop is tracker-driven, so a silent failure strands + every parked agent in WAITING_LONG with no probe ever running. """ agent_id = uuid4() task_id = uuid4() diff --git a/tests/unit/gateway/test_notify.py b/tests/unit/gateway/test_notify.py index 1497eee4..d72dc041 100644 --- a/tests/unit/gateway/test_notify.py +++ b/tests/unit/gateway/test_notify.py @@ -234,11 +234,9 @@ async def test_notify_auditor_rejected_with_not_authorized() -> None: @pytest.mark.asyncio async def test_notify_rejects_prompter_recipient() -> None: - """F048: the prompter (intake-1) is a human-only role with no agent ack - path. An ack-required ALERT sent to it sits permanently unacked and — via - the dedup query's ``~acked_by.contains`` — permanently suppresses any - later same-purpose notification to that role. The notify verb must reject - a prompter recipient at the handler, not deliver an un-ackable signal.""" + """The prompter (intake-1) is human-only with no agent ack path, so an + ack-required ALERT to it would sit unacked and dedup-suppress later + same-purpose notifications — notify must reject it at the handler.""" agent_id = uuid4() task_svc = AsyncMock() task_svc.get_active_task_for_agent.return_value = None @@ -264,7 +262,7 @@ async def test_notify_rejects_prompter_recipient() -> None: @pytest.mark.asyncio async def test_notify_rejects_secretary_recipient() -> None: - """F048: the secretary (secretary-1) is human-only with no agent ack path — + """The secretary (secretary-1) is human-only with no agent ack path — same un-ackable-signal + dedup-suppression hazard as the prompter.""" agent_id = uuid4() task_svc = AsyncMock() @@ -292,9 +290,9 @@ async def test_notify_rejects_secretary_recipient() -> None: @pytest.mark.asyncio async def test_notify_allows_ceo_recipient() -> None: - """F048: the CEO is human-only too, but the human acks via the panel, so a - non-dependency-block CEO notification is a valid ack-required target. The - recipient guard must NOT over-exclude the CEO (only prompter/secretary).""" + """The CEO is human-only too, but acks via the panel, so a + non-dependency-block CEO notification is a valid ack-required target — + the guard must NOT over-exclude the CEO (only prompter/secretary).""" agent_id = uuid4() task_svc = AsyncMock() task_svc.get_active_task_for_agent.return_value = None diff --git a/tests/unit/gateway/test_open_pr_milestone_lock.py b/tests/unit/gateway/test_open_pr_milestone_lock.py index 6efa9e55..d19858c5 100644 --- a/tests/unit/gateway/test_open_pr_milestone_lock.py +++ b/tests/unit/gateway/test_open_pr_milestone_lock.py @@ -1,35 +1,20 @@ -"""F127 — open_pr's idempotent re-entry guard was a read-then-act with no DB -serialization, so a CONCURRENT (respawn-race) retry double-emitted the -"opened PR #N" milestone progress entry. +"""open_pr's idempotent re-entry guard reads ``t.pr_number`` from an unlocked +fetch, so two CONCURRENT (respawn-race) retries both pass the guard and both +emit the 70% "opened PR #N" milestone progress entry — double-counting one +PR-open event in the Progress tab + cycle-time metrics (no PR duplication; +GitHub's 422 'already exists' guard holds). -The sequential-retry guard at ``open_pr`` (``if t.pr_number is not None and -t.assigned_to == agent_id: return Envelope.ok(...)``) short-circuits BEFORE -the runner and BEFORE ``_open_pr_success_envelope`` — so a SECOND call AFTER -the first completed does NOT re-emit the 70% milestone. But this guard reads -``t.pr_number`` from an unlocked fetch. Two CONCURRENT ``open_pr`` calls from -the same agent (the alive-but-unresponsive respawn race CLAUDE.md documents) -both fetch ``t`` with ``pr_number=None``, both pass the guard, both run the -runner (``create_pr``'s GitHub 422 'already exists' path ensures only one PR -is created — no double PR), and both then reach -``_open_pr_success_envelope`` → ``_record_milestone_progress`` (the 70% -"opened PR #N" entry). Result: TWO milestone progress entries for one PR — -the Progress tab + audit reconstruction double-count one PR-open event, and -cycle-time/milestone metrics are skewed. No PR duplication (GitHub 422 guard -holds) and no state corruption — purely a double-emission under the narrow -concurrent-retry case. - -The fix: a PostgreSQL transaction-scoped advisory lock keyed by the task id, -acquired at the TOP of ``open_pr`` BEFORE the ``t = await self.task.get(...)`` -fetch (the read the idempotent guard consults) and held through the runner + +The fix: a PostgreSQL transaction-scoped advisory lock keyed by the task id +(seed ``2``, disjoint from the per-agent claim lock seed ``0`` and the +per-parent delegate lock seed ``1``) acquired at the top of ``open_pr`` +BEFORE the ``t = await self.task.get(...)`` fetch and held through ``_record_milestone_progress`` + the outer request commit. The second -concurrent same-task ``open_pr`` blocks on the lock until the first commits; -its fetch then sees the first's committed ``pr_number``, the idempotent guard -fires, and it short-circuits WITHOUT re-emitting the milestone. Per-TASK (not -per-agent): the single-active-task guard means a dev has one task at a time, -so concurrent ``open_pr`` on the SAME task is purely the respawn-race bug case -— no legitimate concurrency is regressed. Seed ``2`` keeps this in a disjoint -key space from the per-agent claim lock (seed ``0``) and the per-parent -delegate lock (seed ``1``). +same-task concurrent ``open_pr`` blocks until the first commits; its fetch +then sees the committed ``pr_number``, the idempotent guard fires, and it +short-circuits without re-emitting. Per-TASK (not per-agent): the +single-active-task guard means a dev has one task at a time, so concurrent +``open_pr`` on the SAME task is purely the respawn-race case — no legitimate +concurrency is regressed. """ from __future__ import annotations diff --git a/tests/unit/gateway/test_playbook_verbs.py b/tests/unit/gateway/test_playbook_verbs.py index d4f5d0d9..3a52a10a 100644 --- a/tests/unit/gateway/test_playbook_verbs.py +++ b/tests/unit/gateway/test_playbook_verbs.py @@ -115,7 +115,7 @@ async def test_approve_playbook_for_auditor(monkeypatch: pytest.MonkeyPatch) -> assert env.error is None assert env.status == "playbook_approved" svc.approve.assert_awaited_once() - # F057: the status commit gates the index — commit then index, never index + # the status commit gates the index — commit then index, never index # before commit (the index write auto-commits on its own connection). actions.task.session.commit.assert_awaited_once() svc.index_approved.assert_awaited_once_with(approved) @@ -138,7 +138,7 @@ async def test_reject_playbook_archives_for_auditor( ) assert env.status == "playbook_archived" svc.reject.assert_awaited_once() - # F057: de-index is the post-commit step (commit gates it). + # de-index is the post-commit step (commit gates it). actions.task.session.commit.assert_awaited_once() svc.unindex_playbook.assert_awaited_once_with(archived) @@ -147,7 +147,7 @@ async def test_reject_playbook_archives_for_auditor( async def test_archive_playbook_retires_approved_for_auditor( monkeypatch: pytest.MonkeyPatch, ) -> None: - """archive_playbook is the distinct APPROVED->archived retire path (F109): + """archive_playbook is the distinct APPROVED->archived retire path: it calls ``svc.archive`` (NOT ``svc.reject``), commits, then de-indexes.""" archived = MagicMock() archived.id = uuid4() @@ -172,7 +172,7 @@ async def test_approve_playbook_invalid_state_envelope( ) -> None: """A status-precondition ConflictError from the service becomes a clean invalid_state envelope (not a 500) — the agent gets a remediate hint to - re-fetch the playbook's current status before re-trying (F109).""" + re-fetch the playbook's current status before re-trying.""" svc = MagicMock() svc.approve = AsyncMock( side_effect=ConflictError("not draft", resource_type="playbook") diff --git a/tests/unit/gateway/test_pr_gate_notifies_pm.py b/tests/unit/gateway/test_pr_gate_notifies_pm.py index ee7ffe84..b5d88e39 100644 --- a/tests/unit/gateway/test_pr_gate_notifies_pm.py +++ b/tests/unit/gateway/test_pr_gate_notifies_pm.py @@ -244,13 +244,10 @@ async def test_pr_fail_a2a_failure_is_swallowed() -> None: @pytest.mark.asyncio async def test_pr_fail_returns_invalid_state_when_runner_returns_none() -> None: - """F046: if a concurrent transition (cancel or a racing reviewer) moved the - task out of ``awaiting_pr_review`` between the precondition gate and the - runner's final composed action, ``run_intent`` returns None (the verb - runner's documented contract for a last-action source-status failure). - ``_gate_decision`` must surface a clean ``invalid_state`` rejection so the - reviewer re-fetches and re-issues — NOT dereference None and crash the - gate with a 500 AttributeError on ``t.assigned_to`` / ``t.status``. + """A concurrent transition (cancel or racing reviewer) moving the task + out of ``awaiting_pr_review`` after the gate makes ``run_intent`` return + None; ``_gate_decision`` must surface ``invalid_state`` rather than + dereference None and 500 on ``t.assigned_to`` / ``t.status``. """ reviewer_id = uuid4() task_id = uuid4() @@ -279,7 +276,7 @@ async def test_pr_fail_returns_invalid_state_when_runner_returns_none() -> None: @pytest.mark.asyncio async def test_pr_pass_returns_invalid_state_when_runner_returns_none() -> None: - """F046: the same None-guard covers pr_pass — a concurrent cancel between + """The same None-guard covers pr_pass — a concurrent cancel between gate and runner must surface invalid_state, not crash on ``str(t.status)``. """ reviewer_id = uuid4() diff --git a/tests/unit/gateway/test_submit_root_unchanged_pr_guard.py b/tests/unit/gateway/test_submit_root_unchanged_pr_guard.py index 3627be01..14f905f4 100644 --- a/tests/unit/gateway/test_submit_root_unchanged_pr_guard.py +++ b/tests/unit/gateway/test_submit_root_unchanged_pr_guard.py @@ -407,17 +407,16 @@ async def test_pr_pass_does_not_capture_head_sha() -> None: # --------------------------------------------------------------------------- -# F016 — submit_root must not 500 when submit_for_review returns None +# submit_root must not 500 when submit_for_review returns None # --------------------------------------------------------------------------- @pytest.mark.asyncio async def test_submit_root_invalid_state_when_submit_for_review_returns_none() -> None: - """F016: submit_for_review returns None when the root->master PR was already - opened (the task raced out of in_progress, or a prior call already - transitioned it). create_root_pr already ran as the pre-side-effect, so the - PR exists, but the transition did not happen. submit_root must surface an - invalid_state envelope, not dereference None.status and 500.""" + """submit_for_review returns None when the root->master PR was already + opened (task raced out of in_progress, or a prior call transitioned it). + submit_root must surface ``invalid_state``, not dereference None.status + and 500.""" c, main_pm_id, root_task_id = _resubmit_root(notes_structured=None) # The transition did not happen (PR already opened / task raced). c.task.submit_for_review.return_value = None diff --git a/tests/unit/gateway/test_submit_up_unchanged_pr_guard.py b/tests/unit/gateway/test_submit_up_unchanged_pr_guard.py index 4f55b9d5..69ae4156 100644 --- a/tests/unit/gateway/test_submit_up_unchanged_pr_guard.py +++ b/tests/unit/gateway/test_submit_up_unchanged_pr_guard.py @@ -1,17 +1,7 @@ -"""F007 — the unchanged-PR re-submit loop-stopper is root-only; ``submit_up`` -(cell→root) had no head_sha guard, so a weak cell PM could re-submit the -unchanged cell PR and loop ``awaiting_pr_review`` → ``pr_fail`` forever -(the cell-level analogue of the 2026-06-27 root loop F016 closes). +"""The unchanged-PR re-submit loop-stopper, applied to ``submit_up`` (cell→root). -``pr_fail`` stamps the assembled PR's head SHA into -``notes_structured.pr_review.head_sha`` for BOTH cell and root gate tasks -(``pr_gate._capture_pr_head_sha`` / ``_record_gate_verdict`` are -gate-verb-level, not root-level). So the same structural refusal applies -to ``submit_up``: if the cell PR's current head SHA equals the SHA the -last ``pr_fail`` recorded, no new dev work landed on the cell branch ⇒ -the diff is byte-identical ⇒ refuse, do not re-open the gate. Every -ambiguous case FAILS OPEN, identical to the root guard (shared -``_current_pr_head_sha``). +Refuses to re-open the gate when the cell PR's head SHA equals the SHA the +last ``pr_fail`` recorded (no new dev work landed); ambiguous cases FAIL OPEN. """ from __future__ import annotations @@ -145,25 +135,17 @@ async def test_submit_up_fail_open_when_no_prior_pr_fail_verdict() -> None: # --------------------------------------------------------------------------- -# F122: when submit_for_review returns None (a concurrent state change moved -# the task out of in_progress AFTER the create_pr pre-side-effect already -# opened the cell→root PR), the invalid_state remediate must TELL the cell PM -# the PR is already open. The old remediate ('must be in_progress with PR -# ready') hid that the PR exists — so the agent could not tell an orphaned PR -# was sitting on GitHub. The orphan is inherent to the correct pre-side-effect -# ordering (submit_for_review's pr_created gate requires create_pr first, see -# lifecycle.py:1338-1343) and is recoverable via create_pr's idempotent re-issue -# — but only if the agent KNOWS the PR is open. Mirrors submit_root's F016 -# remediate (_impl.py:6305-6310). +# submit_for_review returns None when the task raced out of in_progress after +# create_pr already opened the cell→root PR; the remediate must tell the PM the +# PR is open so the orphan is recoverable via create_pr's idempotent re-issue. # --------------------------------------------------------------------------- @pytest.mark.asyncio async def test_submit_up_none_remediate_names_the_already_open_pr() -> None: - """F122: submit_for_review returns None (raced out of in_progress) AFTER - create_pr already opened the cell→root PR. The rejection remediate must - name the open PR and point the PM at re-fetching + reconciling, not the - misleading 'must be in_progress with PR ready' that hides the PR exists.""" + """submit_for_review returns None (raced out of in_progress) AFTER create_pr + already opened the cell→root PR. The remediate must name the open PR and point + the PM at re-fetching + reconciling, not the misleading 'PR ready' hint.""" c, cell_pm_id, cell_task_id = _resubmit_cell(notes_structured=None) # A concurrent transition (stale-heartbeat reaper unclaim, or a racing # i_am_blocked) moved the task out of in_progress between the precondition diff --git a/tests/unit/gateway/test_toolchain_guard.py b/tests/unit/gateway/test_toolchain_guard.py index 7ce4fdb8..5821f135 100644 --- a/tests/unit/gateway/test_toolchain_guard.py +++ b/tests/unit/gateway/test_toolchain_guard.py @@ -99,11 +99,9 @@ async def test_guard_silent_when_no_marker(monkeypatch: pytest.MonkeyPatch) -> N async def test_guard_reviewer_remediation_uses_pr_fail_not_i_am_blocked( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F044: the pr_pass gate runs this guard on the REVIEWER's workspace. A PR - # reviewer has no i_am_blocked verb, so the dev-path remediation ("call - # i_am_blocked(reason='toolchain')") sends them to a verb they cannot call. - # The reviewer's reject lever is pr_fail — the remediation must point there - # so the PR goes back to needs_revision for the dev to fix the environment. + # pr_pass runs this guard on the REVIEWER's workspace; the remediation must + # use pr_fail (not i_am_blocked — a reviewer has no i_am_blocked verb) so the + # PR returns to needs_revision for the dev to fix the environment. monkeypatch.setattr(settings, "toolchain_match_enabled", True) c = _make_choreographer(status="broken") env = await c._toolchain_broken_guard(uuid4(), MagicMock(), reviewer=True) @@ -118,9 +116,8 @@ async def test_guard_reviewer_remediation_uses_pr_fail_not_i_am_blocked( async def test_guard_dev_remediation_still_uses_i_am_blocked( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F044: the dev (i_am_done) path keeps i_am_blocked — a dev DOES have that - # verb, so the original remediation is correct there. The reviewer flag must - # not change the dev-path wording. + # the dev (i_am_done) path keeps i_am_blocked — a dev has that verb, so the + # reviewer flag must not change the dev-path wording. monkeypatch.setattr(settings, "toolchain_match_enabled", True) c = _make_choreographer(status="broken") env = await c._toolchain_broken_guard(uuid4(), MagicMock()) diff --git a/tests/unit/llm/providers/test_grok_auth.py b/tests/unit/llm/providers/test_grok_auth.py index c2e9da53..0633f58e 100644 --- a/tests/unit/llm/providers/test_grok_auth.py +++ b/tests/unit/llm/providers/test_grok_auth.py @@ -112,11 +112,10 @@ def test_refresh_mints_new_token_when_stale(tmp_path: Path) -> None: def test_refresh_omitting_expires_in_still_marks_token_valid(tmp_path: Path) -> None: - """F092: if xAI's refresh response omits ``expires_in``, the access token's - JWT ``exp`` claim is the authoritative expiry — decode it so a fresh token - isn't left with the stale pre-refresh ``expires_at`` (which would make - ``is_valid`` / ``--check`` forever reject it and the refresh loop re-rotate - the single-use refresh token every tick).""" + """If xAI's refresh response omits ``expires_in``, the access token's JWT + ``exp`` claim is the authoritative expiry — decode it so a fresh token isn't + left with the stale pre-refresh ``expires_at`` (which would re-rotate the + single-use refresh token every tick).""" path = tmp_path / "auth.json" _write(path, _bundle(_PAST)) exp_unix = int((datetime.now(UTC) + timedelta(hours=6)).timestamp()) @@ -136,10 +135,9 @@ def test_refresh_omitting_expires_in_still_marks_token_valid(tmp_path: Path) -> def test_refresh_omitting_expires_in_with_unreadable_jwt_defaults_ttl( tmp_path: Path, ) -> None: - """F092 fallback: expires_in missing AND the access token isn't a JWT with a - readable ``exp`` — default to the documented ~6h TTL so a fresh token is - treated as live instead of stale, rather than forever rejected (the warning - is emitted via structlog, visible in the captured stdout).""" + """Fallback when ``expires_in`` is missing AND the access token isn't a JWT + with a readable ``exp``: default to the documented ~6h TTL so a fresh token + is treated as live instead of forever rejected.""" path = tmp_path / "auth.json" _write(path, _bundle(_PAST)) @@ -185,12 +183,10 @@ def test_refresh_failed_when_no_access_token(tmp_path: Path) -> None: def test_refresh_persists_rotated_token_when_atomic_write_fails( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """F006: a rotated refresh_token is single-use — xAI invalidates the old one - the moment it issues the new one. If the atomic write (tmp+replace) fails - after the rotation, the file keeps the now-dead old refresh_token and the - credential is permanently lost on the next refresh. The write must fall back - to a direct write so the rotated refresh_token survives even when the atomic - replace can't.""" + """A rotated refresh_token is single-use — xAI invalidates the old one on + rotation. If the atomic write (tmp+replace) fails after rotation, the file + keeps the now-dead refresh_token and the credential is permanently lost; the + write must fall back to a direct write so the rotated token survives.""" path = tmp_path / "auth.json" _write(path, _bundle(_PAST)) diff --git a/tests/unit/llm/test_providers.py b/tests/unit/llm/test_providers.py index 7a4c2e60..c33576ff 100644 --- a/tests/unit/llm/test_providers.py +++ b/tests/unit/llm/test_providers.py @@ -242,10 +242,9 @@ async def test_grok_spawn_mounts_auth_when_present(_isolate_grok_auth: Path) -> ) as exec_mock: await provider.spawn(_config()) cmd = list(exec_mock.call_args.args) - # F005: mount the host ~/.grok DIRECTORY (ro), not the single auth.json - # file — a single-file bind mount pins the inode, so the orchestrator's - # atomic auth.json refresh (rename) never reaches a running container. - # The entrypoint symlinks ~/.grok/auth.json at this RO dir mount. + # mount the host ~/.grok DIRECTORY (ro), not the single auth.json file — a + # single-file bind mount pins the inode, so the orchestrator's atomic + # auth.json refresh (rename) never reaches a running container. expected = f"{_isolate_grok_auth}:/home/agent/.grok-auth-ro:ro" assert expected in cmd diff --git a/tests/unit/mcp_servers/test_do_server_circuit_breaker.py b/tests/unit/mcp_servers/test_do_server_circuit_breaker.py index d1521ed3..a13b17aa 100644 --- a/tests/unit/mcp_servers/test_do_server_circuit_breaker.py +++ b/tests/unit/mcp_servers/test_do_server_circuit_breaker.py @@ -265,17 +265,10 @@ def test_verb_extracted_from_path(do_module: types.ModuleType) -> None: def test_dict_shaped_error_does_not_crash(do_module: types.ModuleType) -> None: """A RobocoError.to_dict()-shaped response must not TypeError the breaker. - Smoke-7: A2AAccessDeniedError escaped to middleware and was rendered as - {'error': {'code': ..., 'message': ..., 'details': ...}}. The circuit - breaker's `error in frozenset` check then crashed with - `TypeError: unhashable type: 'dict'`. - - F068: a dict-shaped `error` is a retry-storm-worthy rejection (the - orchestrator's exception handlers all surface this shape on 4xx/5xx), - so the breaker must COUNT it — mapped to a counted kind by the - classifier — rather than passing it through silently. The original - dict payload still reaches the agent (the breaker only substitutes - when open). No TypeError may be raised either way. + A dict-shaped `error` is a retry-storm-worthy rejection (the orchestrator's + exception handlers surface this shape on 4xx/5xx), so the breaker must count + it via the classifier rather than passing it through silently. The original + dict payload still reaches the agent (the breaker only substitutes when open). """ factory, captured = _make_client( orchestrator_response={ @@ -311,10 +304,9 @@ def test_dict_shaped_error_does_not_crash(do_module: types.ModuleType) -> None: def test_422_validation_failure_counts_as_incomplete_input( do_module: types.ModuleType, ) -> None: - """F068: a 422 validation-failure body (`{"detail": [...], "body": ...}`, - no `error` field) must count toward the breaker — a storm of 422s is - retry-storm-worthy (the agent keeps re-submitting malformed input). - Mapped to `incomplete_input` (the agent's input was incomplete/invalid). + """A 422 validation-failure body (`{"detail": [...], "body": ...}`, no `error` + field) must count toward the breaker — a storm of 422s is retry-storm-worthy. + Mapped to `incomplete_input`. """ factory, captured = _make_client( orchestrator_response={ @@ -347,9 +339,8 @@ def test_422_validation_failure_counts_as_incomplete_input( def test_dict_shaped_internal_error_counts_as_invalid_state( do_module: types.ModuleType, ) -> None: - """F068: a 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler) - must count toward the breaker as `invalid_state` — a storm of 500s is - retry-storm-worthy and previously bypassed the breaker entirely. + """A 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler) must + count toward the breaker as `invalid_state` — a storm of 500s is retry-storm-worthy. """ factory, captured = _make_client( orchestrator_response={ @@ -379,7 +370,7 @@ def test_dict_shaped_internal_error_counts_as_invalid_state( def test_dict_shaped_invalid_input_counts_as_incomplete_input( do_module: types.ModuleType, ) -> None: - """F068: a dict-shaped INVALID_INPUT (mapped from 422 by http_exception_handler) + """A dict-shaped INVALID_INPUT (mapped from 422 by http_exception_handler) counts as `incomplete_input` — semantically the agent's input was invalid. """ factory, captured = _make_client( @@ -404,19 +395,18 @@ def test_dict_shaped_invalid_input_counts_as_incomplete_input( # --------------------------------------------------------------------------- -# F069 — a manifest-registered content tool whose route is missing must -# return an envelope rejection (not a raw 404 body) so the breaker counts it. +# A manifest-registered content tool whose route is missing must return an +# envelope rejection (not a raw 404 body) so the breaker counts it. # --------------------------------------------------------------------------- def test_missing_route_404_returns_envelope_and_counts( do_module: types.ModuleType, ) -> None: - """F069: a 404 from the orchestrator (manifest-registered tool with no - route) must surface as a proper `invalid_state` Envelope rejection — not - FastAPI's raw ``{"detail": "Not Found"}`` body — and the breaker must - count it. Without this, the agent retries the missing tool forever and - the breaker never trips. Mirrors flow_server's 404 handling. + """A 404 from the orchestrator (manifest-registered tool with no route) must + surface as a proper `invalid_state` Envelope rejection — not FastAPI's raw + ``{"detail": "Not Found"}`` body — and the breaker must count it. Mirrors + flow_server's 404 handling. """ captured: list[tuple[str, dict[str, Any] | None]] = [] diff --git a/tests/unit/mcp_servers/test_flow_server_circuit_breaker.py b/tests/unit/mcp_servers/test_flow_server_circuit_breaker.py index 1143321d..025b997a 100644 --- a/tests/unit/mcp_servers/test_flow_server_circuit_breaker.py +++ b/tests/unit/mcp_servers/test_flow_server_circuit_breaker.py @@ -432,17 +432,16 @@ def test_task_id_none_is_forwarded_as_null(flow_module: types.ModuleType) -> Non # --------------------------------------------------------------------------- -# F068 — non-string-error / no-error-field rejection shapes are counted +# Non-string-error / no-error-field rejection shapes are counted # --------------------------------------------------------------------------- def test_422_validation_failure_counts_as_incomplete_input( flow_module: types.ModuleType, ) -> None: - """F068: a 422 validation-failure body (`{"detail": [...]}`, no `error`) - must count toward the breaker as `incomplete_input` — a storm of 422s is - retry-storm-worthy. Mirrors do_server's classifier (the two servers share - the same breaker logic and must stay in parity). + """A 422 validation-failure body (`{"detail": [...]}`, no `error`) must count + toward the breaker as `incomplete_input` — a storm of 422s is retry-storm-worthy. + Mirrors do_server's classifier (the two servers share the same breaker logic). """ factory, captured = _make_client( orchestrator_response={ @@ -475,8 +474,8 @@ def test_422_validation_failure_counts_as_incomplete_input( def test_dict_shaped_internal_error_counts_as_invalid_state( flow_module: types.ModuleType, ) -> None: - """F068: a 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler) - counts as `invalid_state` — a storm of 500s previously bypassed the breaker. + """A 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler) counts + as `invalid_state` — a storm of 500s is retry-storm-worthy. """ factory, captured = _make_client( orchestrator_response={ @@ -506,9 +505,8 @@ def test_dict_shaped_internal_error_counts_as_invalid_state( def test_dict_shaped_not_found_does_not_count( flow_module: types.ModuleType, ) -> None: - """F068: a dict-shaped NOT_FOUND (404 family) does NOT count — parity with - the string-error contract that a `not_found` rejection isn't counted - (retrying a missing resource won't help until state changes). + """A dict-shaped NOT_FOUND (404 family) does NOT count — parity with the + string-error contract that a `not_found` rejection isn't counted. """ factory, captured = _make_client( orchestrator_response={ @@ -524,8 +522,8 @@ def test_dict_shaped_not_found_does_not_count( # --------------------------------------------------------------------------- -# F069 — a manifest-registered verb whose route is missing must return an -# envelope rejection (not a raw 404 body) so the breaker counts it. +# A manifest-registered verb whose route is missing must return an envelope +# rejection (not a raw 404 body) so the breaker counts it. # --------------------------------------------------------------------------- @@ -570,11 +568,9 @@ def _make_404_client() -> tuple[Any, list[tuple[str, dict[str, Any] | None]]]: def test_missing_route_404_returns_envelope_and_counts( flow_module: types.ModuleType, ) -> None: - """F069: a 404 from the orchestrator (manifest-registered verb with no - route) must surface as a proper `invalid_state` Envelope rejection — not - FastAPI's raw ``{"detail": "Not Found"}`` body — and the breaker must - count it. Without this, the agent retries the missing route forever and - the breaker never trips. + """A 404 from the orchestrator (manifest-registered verb with no route) must + surface as a proper `invalid_state` Envelope rejection — not FastAPI's raw + ``{"detail": "Not Found"}`` body — and the breaker must count it. """ factory, captured = _make_404_client() with patch("httpx.Client", side_effect=factory): diff --git a/tests/unit/runtime/test_blocker_and_claimed_dispatch.py b/tests/unit/runtime/test_blocker_and_claimed_dispatch.py index 153b84f6..dfa4e9f8 100644 --- a/tests/unit/runtime/test_blocker_and_claimed_dispatch.py +++ b/tests/unit/runtime/test_blocker_and_claimed_dispatch.py @@ -196,16 +196,9 @@ def test_in_progress_task_with_no_agent_returns_assignee() -> None: def test_claimed_task_with_unknown_assignee_returns_slug_for_release() -> None: - # F032: a claimed/in_progress task whose assignee is a stale/unknown UUID - # (no seeded agent) must reach the release-to-pending path. The human-only - # guard (role_for_slug_or_none) returns None for an unknown slug, and - # ``None in (CEO, PROMPTER, SECRETARY)`` is False — so it does NOT - # short-circuit, the slug falls through the grace window, and the resolver - # returns the unknown slug. _dispatch_claimed_without_agent then sees - # get_agent_role(slug) == "unknown" and releases the claim to pending for - # a role-matched reclaim. Before F031's role_for_slug_or_none fix the guard - # raised KeyError on the unknown slug, crashing the whole tick before the - # release path could run. + # A claimed/in_progress task whose assignee is a stale/unknown UUID (no + # seeded agent) must reach the release-to-pending path: the human-only guard + # returns None for unknown slugs, so the slug falls through and is released. orch = _orch() unknown_uuid = str(uuid4()) task: dict[str, Any] = { diff --git a/tests/unit/runtime/test_ci_watch_loop.py b/tests/unit/runtime/test_ci_watch_loop.py index 108928b4..45688107 100644 --- a/tests/unit/runtime/test_ci_watch_loop.py +++ b/tests/unit/runtime/test_ci_watch_loop.py @@ -58,11 +58,11 @@ async def test_load_watch_set_filters_enabled_one_per_repo() -> None: @pytest.mark.asyncio async def test_load_watch_set_keeps_distinct_workflows_per_repo() -> None: - """F115: a monorepo's several cell-projects each carrying their OWN - ``ci_watch_workflow`` must ALL be watched — collapsing to the canonical - cell's workflow would miss a red on the other cells' workflows (under-count). - Same repo, DIFFERENT workflows → one entry per (repo, workflow). The engine's - per-git_url fix-task dedup still prevents duplicate fix tasks for the repo.""" + """A monorepo's several cell-projects each carrying their OWN + ``ci_watch_workflow`` must ALL be watched — collapsing to the canonical cell's + workflow would miss a red on the other cells' workflows (under-count). Same + repo, DIFFERENT workflows → one entry per (repo, workflow); per-git_url dedup + still prevents duplicate fix tasks for the repo.""" orch = _orch() be = MagicMock( slug="be", diff --git a/tests/unit/runtime/test_dep_update_loop.py b/tests/unit/runtime/test_dep_update_loop.py index 3e619312..554a5527 100644 --- a/tests/unit/runtime/test_dep_update_loop.py +++ b/tests/unit/runtime/test_dep_update_loop.py @@ -46,12 +46,12 @@ async def test_load_set_filters_command_one_per_repo() -> None: @pytest.mark.asyncio async def test_load_set_keeps_distinct_commands_per_repo() -> None: - """F115: a monorepo's several cell-projects each carrying their OWN - ``dep_update_command`` (different ecosystems → different lockfiles) must - ALL be probed — collapsing to the canonical cell's command would miss the - other cells' lockfile drift (under-count). Same repo, DIFFERENT commands → - one entry per (repo, command). The engine's per-git_url open-task dedup - still prevents duplicate update tasks for the repo.""" + """A monorepo's several cell-projects each carrying their OWN + ``dep_update_command`` (different ecosystems → different lockfiles) must ALL + be probed — collapsing to the canonical cell's command would miss the other + cells' lockfile drift (under-count). Same repo, DIFFERENT commands → one + entry per (repo, command); per-git_url open-task dedup still prevents + duplicate update tasks for the repo.""" orch = _orch() be = MagicMock( slug="be", git_url="https://x/a.git", dep_update_command="uv lock --upgrade" diff --git a/tests/unit/runtime/test_grok_cost_budget.py b/tests/unit/runtime/test_grok_cost_budget.py index 6c89498c..553997ef 100644 --- a/tests/unit/runtime/test_grok_cost_budget.py +++ b/tests/unit/runtime/test_grok_cost_budget.py @@ -60,11 +60,9 @@ async def test_cost_over_cap_kills_and_evicts(monkeypatch: pytest.MonkeyPatch) - async def test_cost_over_cap_finalizes_spawn_session_before_evict( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F040: a cost-cap-killed grok container must finalize its spawn session so - # the captured usage/cost is recorded in the DB/dashboard — otherwise the - # session row stays open (ended_at IS NULL) and the burn is invisible. - # Finalization must run BEFORE the instance is popped: _finalize_spawn_session - # reads self._instances[agent_id] for the model + usage_session_id. + # Cost-cap-killed grok container must finalize its spawn session BEFORE the + # instance is popped: _finalize_spawn_session reads _instances[agent_id] for + # the model + usage_session_id; otherwise the burn stays invisible. orch, _remove_mock = _orch(monkeypatch, cap=5.0, cost=7.5) finalize = AsyncMock() monkeypatch.setattr(orch, "_finalize_spawn_session", finalize) diff --git a/tests/unit/runtime/test_grok_rate_limit.py b/tests/unit/runtime/test_grok_rate_limit.py index e030064a..009f1a0e 100644 --- a/tests/unit/runtime/test_grok_rate_limit.py +++ b/tests/unit/runtime/test_grok_rate_limit.py @@ -116,7 +116,7 @@ async def test_park_grok_rate_limited_activates_and_offlines( # needs the dict + persist stub to exercise that without AttributeError. orch._waiting_records = {} orch._rate_limit_ceo_notified = set() - # F097 backoff state — the constructor (skipped here) initializes these. + # Backoff state — the constructor (skipped here) initializes these. orch._grok_last_park_at = None orch._grok_repark_count = 0 inst = _grok_instance() @@ -159,7 +159,7 @@ async def test_handle_stopped_container_parks_on_grok_429( # --------------------------------------------------------------------------- -# F041: exit 78 (auth missing/expired) parks instead of crash-retrying +# Exit 78 (auth missing/expired) parks instead of crash-retrying # --------------------------------------------------------------------------- @@ -178,11 +178,9 @@ def test_is_grok_auth_exit() -> None: async def test_handle_stopped_container_parks_on_grok_auth_exit( monkeypatch: pytest.MonkeyPatch, ) -> None: - # F041: a grok container whose entrypoint ran `grok_auth --check` and found - # the token missing/expired exits 78 (EX_CONFIG). Crash-retrying 3x burns - # tokens for zero progress (the agent can't start without a valid token); - # park it like the 429 exit-75 path so the probe-resume loop revives the - # task once grok_auth.refresh_if_stale mints a fresh token. + # A grok container whose entrypoint ran `grok_auth --check` and found the + # token missing/expired exits 78 (EX_CONFIG); park it (like the 429 exit-75 + # path) so the probe-resume loop revives the task once a fresh token is minted. orch = AgentOrchestrator.__new__(AgentOrchestrator) inst = _grok_instance() park = AsyncMock() @@ -224,9 +222,9 @@ async def test_park_grok_auth_unavailable_activates_with_auth_missing_kind( # --------------------------------------------------------------------------- # -# F097 — grok has no real probe, so an optimistic clear respawns into a still- -# active xAI 429 every ~90s. Back off the re-park retry_after within one rate- -# limit episode so the churn dampens instead of spinning flat at 60s. +# Grok has no real probe, so an optimistic clear respawns into a still-active +# xAI 429 every ~90s; back off the re-park retry_after within one rate-limit +# episode so the churn dampens instead of spinning flat at 60s. # --------------------------------------------------------------------------- # diff --git a/tests/unit/runtime/test_intake_spawn.py b/tests/unit/runtime/test_intake_spawn.py index 96e1bc26..9f9e4be8 100644 --- a/tests/unit/runtime/test_intake_spawn.py +++ b/tests/unit/runtime/test_intake_spawn.py @@ -37,8 +37,8 @@ def _make_minimal_orchestrator() -> AgentOrchestrator: # (F071); without this the post-docker-run guard would AttributeError on # the constructor-skipped instance. orch._running = True - # F093: concurrent intake starts serialize on this lock; the constructor - # (skipped here) initializes it. + # Concurrent intake starts serialize on this lock; the constructor (skipped + # here) initializes it. orch._intake_spawn_lock = asyncio.Lock() return orch @@ -572,11 +572,9 @@ class TestDeliverWhenReady: # --------------------------------------------------------------------------- -# F071 — non-blocking intake spawn must not orphan a container if shutdown -# arrives between ``docker run`` and the _instances registration. The guarded -# wrapper runs concurrently with stop(); without a post-docker-run shutdown -# check, the just-started container is never recorded in _instances (which -# stop() already iterated) so nothing tears it down — a leaked container. +# Non-blocking intake spawn must not orphan a container if shutdown arrives +# between ``docker run`` and _instances registration: without a post-docker-run +# shutdown check the just-started container is never recorded so leaks. # --------------------------------------------------------------------------- diff --git a/tests/unit/runtime/test_interactive_grok_spawn.py b/tests/unit/runtime/test_interactive_grok_spawn.py index 3189e1d5..55950a36 100644 --- a/tests/unit/runtime/test_interactive_grok_spawn.py +++ b/tests/unit/runtime/test_interactive_grok_spawn.py @@ -82,7 +82,7 @@ def test_intake_grok_mounts_subscription_auth_when_present( cmd = AgentOrchestrator._build_intake_run_cmd( _intake_spec("grok", base_url="https://api.x.ai/v1", token="xai-key") ) - # F005: directory mount (ro), not the single-file inode-pinning mount. + # directory mount (ro), not the single-file inode-pinning mount. assert f"{grok_dir}:/home/agent/.grok-auth-ro:ro" in cmd diff --git a/tests/unit/runtime/test_orchestrator_shutdown_drain.py b/tests/unit/runtime/test_orchestrator_shutdown_drain.py index 2320f996..b376ea56 100644 --- a/tests/unit/runtime/test_orchestrator_shutdown_drain.py +++ b/tests/unit/runtime/test_orchestrator_shutdown_drain.py @@ -1,22 +1,10 @@ -"""F070 — fire-and-forget ``_bg_tasks`` (respawn_tracker upserts, audit-log -writes, intake first-message delivery) were never cancelled or drained on -shutdown. ``Orchestrator.stop()`` cancelled only the named loop tasks and the -agents, then returned, abandoning any in-flight ``_schedule_bg`` work. +"""Drain ``_bg_tasks`` on shutdown so fire-and-forget writes (respawn_tracker +upserts, audit-log writes, intake first-message delivery) are not abandoned. -The data-loss tail: an in-flight ``_persist_respawn_record`` upsert dropped at -shutdown means the last few gate-mutation strikes never reach the DB. The -in-memory counter dies with the process; ``restore_respawn_tracker()`` on the -next start repopulates a stale lower count and the dispatcher re-burns the -full strike threshold (4 spawns) against a still-wedged task — the exact -re-burn the durable tracker exists to stop. Audit-log writes (load-bearing for -the cycle-time / rework metrics) are similarly dropped. - -The fix DRAINs ``_bg_tasks`` with a bounded timeout on shutdown — short DB -writes finish before the process exits (data preserved), while a stuck task -can't hang shutdown (it is cancelled once the drain deadline passes). Cancels -outright would lose the data (the opposite of the goal), so the drain tries to -let work complete first. The ``stop_agent`` loop is also wrapped so one agent's -stop error can't skip the drain (which would still drop the data). +Invariant: ``Orchestrator.stop()`` drains ``_bg_tasks`` with a bounded timeout — +short DB writes finish before the process exits (data preserved), a stuck task +is cancelled once the deadline passes (can't hang shutdown). The ``stop_agent`` +loop is wrapped so one agent's stop error can't skip the drain. """ from __future__ import annotations @@ -152,10 +140,9 @@ async def test_stop_failing_agent_does_not_skip_drain() -> None: @pytest.mark.asyncio async def test_stop_is_idempotent_double_call_is_noop() -> None: - """F117: stop() is idempotent. The lifespan shutdown path now stops the - orchestrator before closing the DB, and bootstrap's finally block re-calls - stop() as a safety net. The second call must be a clean no-op — not a - re-drain, not a re-stop of already-stopped agents — guarded by ``_stopped``.""" + """stop() is idempotent: the lifespan path and bootstrap's finally block both + call it, so the second call must be a clean no-op — not a re-drain or re-stop + of already-stopped agents — guarded by ``_stopped``.""" orch = _make_orchestrator() real_drain = orch._drain_bg_tasks drain_calls = 0 diff --git a/tests/unit/runtime/test_orchestrator_write_hooks.py b/tests/unit/runtime/test_orchestrator_write_hooks.py index 69539793..173153d0 100644 --- a/tests/unit/runtime/test_orchestrator_write_hooks.py +++ b/tests/unit/runtime/test_orchestrator_write_hooks.py @@ -568,10 +568,9 @@ def _stop_agent_patches(orch: AgentOrchestrator) -> Any: async def test_stop_agent_releases_claim_when_release_claim_true() -> None: - """F120: stop_agent(release_claim=True) hands the agent's claimed task back - to the pool immediately. A SIGTERM/budget-kill mid-verb otherwise leaves - the task CLAIMED/IN_PROGRESS with no running agent for up to - stale_claim_reap_seconds (the reaper's heartbeat TTL).""" + """stop_agent(release_claim=True) releases the agent's claimed task to the + pool immediately, so a mid-verb SIGTERM/budget-kill doesn't strand the task + CLAIMED/IN_PROGRESS until the reaper's heartbeat TTL expires.""" orch = _make_orchestrator() instance = _make_instance(_AGENT_ID) instance.current_task_id = str(uuid4()) @@ -631,10 +630,9 @@ async def test_stop_agent_does_not_release_claim_by_default() -> None: async def test_stop_agent_skips_release_for_provider_parked_agent() -> None: - """F120: a provider-parked agent (rate_limit_lifted WaitingRecord) must NOT - have its claim released even when release_claim=True. The probe-resume loop - owns its recovery and the claim must survive so probe-success revives the - SAME agent on the SAME task — reaping would let another agent claim it.""" + """A provider-parked agent (rate_limit_lifted WaitingRecord) must NOT have + its claim released even when release_claim=True — the probe-resume loop + revives the SAME agent on the SAME task, so reaping would lose the claim.""" orch = _make_orchestrator() instance = _make_instance(_AGENT_ID) instance.current_task_id = str(uuid4()) diff --git a/tests/unit/runtime/test_provider_overload_break.py b/tests/unit/runtime/test_provider_overload_break.py index d5f01b0f..53a24b61 100644 --- a/tests/unit/runtime/test_provider_overload_break.py +++ b/tests/unit/runtime/test_provider_overload_break.py @@ -108,11 +108,9 @@ async def test_clean_output_is_not_overload( async def test_detects_overload_marker_in_transcript( orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch ) -> None: - # F036: the SDK server writes model-API errors to /tmp/sdk-server.log, not - # stdout, so the overload marker (529/500/503) may appear only in the durable - # Claude transcript — exactly the rationale already applied to the - # session-limit detector. Without reading the transcript here an overload - # is missed and the agent crash-respawns straight back into it. + # The overload marker may appear only in the durable Claude transcript, not + # stdout; without reading it an overload is missed and the agent + # crash-respawns straight back into it. monkeypatch.setattr(settings, "overload_break_enabled", True) monkeypatch.setattr(orch, "_tail_container_logs", AsyncMock(return_value="")) monkeypatch.setattr( @@ -128,10 +126,9 @@ async def test_detects_overload_marker_in_transcript( async def test_agent_writing_about_error_500_does_not_park( orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch ) -> None: - # F037: an agent that merely writes about an HTTP error code in its own - # notes ("the endpoint returned error 500, retrying") must NOT trip the - # overload detector and park the whole Anthropic fleet. Markers must be - # specific to the API error formatter, not bare "error NNN". + # An agent merely writing about an HTTP error code in its own notes must NOT + # trip the detector and park the whole fleet — markers must be specific to + # the API error formatter, not bare "error NNN". monkeypatch.setattr(settings, "overload_break_enabled", True) agent_note = ( "be-dev-1: the /health endpoint returned error 500 on retry; " @@ -203,11 +200,10 @@ async def test_park_offlines_and_activates_with_kind( async def test_park_registers_waiting_record_so_probe_can_resume( orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch ) -> None: - # F035: the probe-resume loop reads _waiting_records filtered by - # waiting_for == "rate_limit_lifted" + context.provider. Without a record - # here, _parked_agents_for(provider) returns [] and _on_probe_success - # resumes nobody — recovery falls to the 600s stale-claim reaper instead of - # the probe-success path the parking design relies on. + # The probe-resume loop reads _waiting_records filtered by + # waiting_for == "rate_limit_lifted" + context.provider; without a record + # here recovery falls to the 600s stale-claim reaper instead of the + # probe-success path. orch._waiting_records = {} inst = _instance() inst.current_task_id = "task-1" @@ -232,7 +228,7 @@ async def test_park_registers_waiting_record_so_probe_can_resume( async def test_probe_success_respawns_parked_agent( orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch ) -> None: - # F035: once the probe succeeds, the parked agent must be respawned via + # Once the probe succeeds, the parked agent must be respawned via # resolve_wait — not left stranded for the 600s reaper. orch._waiting_records = { "be-dev-1": WaitingRecord( diff --git a/tests/unit/runtime/test_rate_limit_sweep.py b/tests/unit/runtime/test_rate_limit_sweep.py index 82c67e12..39e0007a 100644 --- a/tests/unit/runtime/test_rate_limit_sweep.py +++ b/tests/unit/runtime/test_rate_limit_sweep.py @@ -570,13 +570,11 @@ class TestCEONotificationThreshold: class TestOrphanProviderFallback: - """F045: an activate() failure in the in-verb ``i_am_blocked(rate_limited)`` - path leaves agents parked in ``_waiting_records`` but the provider never - makes it into the tracker — so the tracker-driven loop never probes it and - the parked agents strand in WAITING_LONG forever. The sweep must scan the - in-memory records for any ``rate_limit_lifted`` provider the tracker-listed - set did NOT cover and probe it via the time-expiry fallback so - ``_on_probe_success`` can resume them. + """An activate() failure in the ``i_am_blocked(rate_limited)`` path parks + agents in ``_waiting_records`` without entering the tracker, so the + tracker-driven loop never probes them. The sweep must scan the in-memory + records for any ``rate_limit_lifted`` provider the tracker missed and probe + it via the time-expiry fallback so ``_on_probe_success`` can resume them. """ async def test_orphan_parked_agent_resumed_when_tracker_lacks_provider( diff --git a/tests/unit/runtime/test_readopt_running_agents.py b/tests/unit/runtime/test_readopt_running_agents.py index f8c232f8..065f96dc 100644 --- a/tests/unit/runtime/test_readopt_running_agents.py +++ b/tests/unit/runtime/test_readopt_running_agents.py @@ -82,11 +82,8 @@ async def test_readopt_swallows_probe_errors() -> None: @pytest.mark.asyncio async def test_readopt_records_container_id_so_health_check_can_see_exit() -> None: - # F033: a re-adopted instance registered with container_id=None is skipped by - # _check_health (`if instance.container_id is None: continue`), so when the - # container later exits the stopped-container handler never runs — the task - # is stranded under a phantom ACTIVE instance forever. Re-adopt must capture - # the real container id so the health loop can observe the later exit. + # Re-adopt must capture the real container id; a None container_id is skipped + # by _check_health, stranding the task under a phantom ACTIVE instance. orch = _orch() orch._inspect_container_state = AsyncMock(return_value=(True, 0)) orch._resolve_container_id = AsyncMock(return_value="deadbeef1234") diff --git a/tests/unit/runtime/test_reaper_subprocess_timeout.py b/tests/unit/runtime/test_reaper_subprocess_timeout.py index 8703f5ad..9626465a 100644 --- a/tests/unit/runtime/test_reaper_subprocess_timeout.py +++ b/tests/unit/runtime/test_reaper_subprocess_timeout.py @@ -1,21 +1,10 @@ -"""F072 — reaper Docker subprocess calls (``docker inspect`` / ``docker exec``) -had no deadline: a hung Docker daemon or a stuck container FS would freeze the -single asyncio event loop, because the reaper runs inline before every dispatch -tick and shares that loop with every background sweeper (rate-limit probe, -self-heal, ci-watch, dep-update, release-manager, grok-auth refresh). - -The fix bounds each call with ``asyncio.wait_for``; on expiry ``proc.kill()`` the -child and either raise (``inspect`` / ``resolve_container_id`` — the callers -already apply their own fail-direction) or return ``None`` (the gateway probe — -inconclusive, the caller declines to act, matching its existing probe-failure -contract). The deadlines are generous enough that a legitimate slow docker call -is never wrongly aborted. ``_check_health`` is also hardened so one agent's hung -inspect skips that agent, not the whole sweep — preserving the per-tick -check-all-agents invariant the timeout-then-raise would otherwise break. - -Deterministic: the slow-docker tests patch the timeout constants tiny and use a -never-resolving ``communicate``/``wait`` so a bounded fail-close is asserted in -well under a second, never relying on real wall-clock timing of the defaults. +"""Reaper Docker subprocess calls (``docker inspect`` / ``docker exec``) are +bounded with ``asyncio.wait_for`` so a hung Docker daemon can't freeze the shared +asyncio event loop (the reaper runs before every dispatch tick). On timeout the +child is killed and the call either raises (``inspect`` / +``resolve_container_id``) or returns ``None`` (gateway probe — inconclusive). +``_check_health`` is hardened per-agent so one hung inspect skips that agent, not +the whole sweep, preserving the per-tick check-all-agents invariant. """ from __future__ import annotations diff --git a/tests/unit/runtime/test_resolve_wait_repark.py b/tests/unit/runtime/test_resolve_wait_repark.py index 7131e605..550009b5 100644 --- a/tests/unit/runtime/test_resolve_wait_repark.py +++ b/tests/unit/runtime/test_resolve_wait_repark.py @@ -1,13 +1,8 @@ -"""F098: a re-park during probe-success resume must not orphan the agent. - -``resolve_wait`` deletes the waiting record (in-memory + durable) and then calls -``spawn_agent`` to respawn the parked agent. If the provider re-parks in the -window between the probe-success clear and the spawn (the rate limit lifts then -immediately re-limits, or a second provider limit lands), ``spawn_agent`` bails -with an OFFLINE instance — the F095 parked-provider short-circuit. The old order -deleted the record BEFORE the spawn, so a bail orphaned the agent: no record -means the probe-resume loop can never revive it and the spawn gate bails every -tick. The record must stay until a container actually launches. +"""A re-park during probe-success resume must not orphan the agent. The waiting +record must stay until ``spawn_agent`` actually launches a container — deleting +it before the spawn lets a provider re-park (``spawn_agent`` bails OFFLINE on +the parked-provider short-circuit) leave the agent with no record for the +probe-resume loop to revive. """ from __future__ import annotations diff --git a/tests/unit/runtime/test_respawn_persistence.py b/tests/unit/runtime/test_respawn_persistence.py index 586f485b..df6a871a 100644 --- a/tests/unit/runtime/test_respawn_persistence.py +++ b/tests/unit/runtime/test_respawn_persistence.py @@ -83,13 +83,9 @@ def test_partition_drops_terminal_and_missing_rows() -> None: def test_partition_restamps_last_check_to_now_to_avoid_stale_tracing_gap() -> None: - # F034: a persisted last_check from BEFORE the restart would make the first - # post-restart ``_pm_made_rule_following_retry`` audit lookup - # (``since = record.get("last_check")``) match a PRE-restart tracing_gap - # row, falsely resetting the breaker on the very first post-restart spawn — - # exactly when a fresh strike count should be evaluating current state. - # Restore must re-stamp last_check to the restore time so only post-restart - # tracing gaps can reset the counter. + # Restore must re-stamp last_check to the restore time so a pre-restart + # tracing_gap row can't falsely reset the breaker on the first post-restart + # spawn. tid = uuid4() stale_check = datetime(2026, 6, 20, tzinfo=UTC) rows = [_row(tid, last_check=stale_check)] @@ -380,7 +376,7 @@ async def test_restart_midloop_continues_identically_to_no_restart() -> None: # --------------------------------------------------------------------------- # -# F096 — fire-and-forget persist commit ordering +# fire-and-forget persist commit ordering # --------------------------------------------------------------------------- # diff --git a/tests/unit/runtime/test_secretary_spawn_shutdown.py b/tests/unit/runtime/test_secretary_spawn_shutdown.py index 28ec8477..6e408ef3 100644 --- a/tests/unit/runtime/test_secretary_spawn_shutdown.py +++ b/tests/unit/runtime/test_secretary_spawn_shutdown.py @@ -1,17 +1,7 @@ -"""F071 — the Secretary non-blocking spawn (``start_secretary_session`` → -``_schedule_bg(_spawn_secretary_container_guarded)``) runs ``docker run`` and -only registers the instance in ``_instances`` at the END. If shutdown arrives -between ``docker run`` and the registration line, the container is started but -the orchestrator has no handle to it — ``stop()`` iterates only ``_instances``, -so the container is orphaned (leaked, must be cleaned up with ``docker rm``). -Worse, the F070 drain can let the spawn coroutine COMPLETE the registration -AFTER ``stop()`` already iterated ``_instances``, landing a live container into -a shutting-down registry that nothing tears down. - -The fix: after ``docker run`` returns the container id, re-check ``self._running`` -and, if the orchestrator began shutting down, remove the just-started container -and abort WITHOUT registering. The guarded wrapper closes the relay silently -(shutdown is not a user-facing failure). +"""The Secretary non-blocking spawn registers the instance in ``_instances`` +only at the END of ``docker run``; if shutdown arrives mid-spawn the container +must be removed and the registration aborted, or ``stop()`` (which iterates only +``_instances``) leaks an orphaned container into a shutting-down registry. """ from __future__ import annotations @@ -38,7 +28,7 @@ def _make_orchestrator() -> AgentOrchestrator: orch._instances = {} orch._bg_tasks = set() orch._running = True - # F093: concurrent secretary starts serialize on this lock; the constructor + # Concurrent secretary starts serialize on this lock; the constructor # (skipped here) initializes it. orch._secretary_spawn_lock = asyncio.Lock() return orch @@ -170,12 +160,11 @@ async def test_running_spawn_registers_normally( # --------------------------------------------------------------------------- -# F093 — concurrent Secretary starts must serialize. The Secretary agent id is a -# single fixed id, so two concurrent ``spawn_secretary_session`` calls race on -# the container name (``docker run --name roboco-agent-secretary``) and the -# ``_instances[SECRETARY_AGENT_ID]`` write, orphaning a container + relay. The -# spawn body runs under ``_secretary_spawn_lock`` so the second start only begins -# once the first has fully registered (so the second's reap-prior sees it). +# Concurrent Secretary starts must serialize — the single fixed Secretary agent +# id makes two concurrent ``spawn_secretary_session`` calls race on the container +# name and the ``_instances[SECRETARY_AGENT_ID]`` write. The spawn body runs +# under ``_secretary_spawn_lock`` so the second start only begins once the first +# has fully registered. # --------------------------------------------------------------------------- diff --git a/tests/unit/runtime/test_self_heal_ceo_gate.py b/tests/unit/runtime/test_self_heal_ceo_gate.py index b54c798f..7ca682d3 100644 --- a/tests/unit/runtime/test_self_heal_ceo_gate.py +++ b/tests/unit/runtime/test_self_heal_ceo_gate.py @@ -1,23 +1,7 @@ -"""F059: self-heal fix tasks must WAIT for the CEO's Approve-&-Start. - -The module docstring promises the loop 'only NOTIFIES and, at most, OPENS a -PENDING task ... the task waits for the CEO's Approve-&-Start and terminates at -awaiting_ceo_approval'. The implementation did the opposite: it created the -task ``confirmed_by_human=True`` and the orchestrator dispatched it at once — -a self-heal fix that re-broke CI would trigger another self-heal cycle, open -another auto-dispatched fix, and loop with no CEO gate on dispatch. - -The fix restores the documented gate: -* ``_originate`` opens the task ``confirmed_by_human=False`` (held for the CEO). -* The orchestrator holds a self-heal task out of dispatch until the CEO - approves it (``confirmed_by_human`` flips True via ``approve_and_start``). -* ``give_me_work`` (``list_pending_for_agent``) never offers a held task to an - already-alive agent. -* ``approve_and_start`` is the CEO's start gate — it flips ``confirmed_by_human`` - True so the held task finally dispatches. - -The 'never self-deploys' guarantee (no merge) is unchanged; only the dispatch -gate is restored. +"""Self-heal fix tasks must WAIT for the CEO's Approve-&-Start. ``_originate`` +opens them ``confirmed_by_human=False`` (held); the orchestrator holds them out +of dispatch until ``approve_and_start`` flips it True. The 'never self-deploys' +guarantee (no merge) is unchanged. """ from __future__ import annotations @@ -269,7 +253,7 @@ async def test_list_pending_for_agent_excludes_held_self_heal() -> None: @pytest.mark.asyncio async def test_list_pending_for_agent_still_offers_delegated_subtask() -> None: - """Regression guard (F059): the hold is scoped to self-heal. A delegated + """Regression guard: the hold is scoped to self-heal. A delegated subtask (source != self_heal, confirmed_by_human=False — the default for PM-delegated work, where the delegation IS the authorization to start) must STILL be offered via give_me_work. A universal confirmed_by_human filter diff --git a/tests/unit/runtime/test_stale_claim_reaper.py b/tests/unit/runtime/test_stale_claim_reaper.py index 98549954..3db6276c 100644 --- a/tests/unit/runtime/test_stale_claim_reaper.py +++ b/tests/unit/runtime/test_stale_claim_reaper.py @@ -380,11 +380,10 @@ async def test_reap_releases_on_registry_miss_when_container_gone( async def test_reap_spares_provider_parked_agent_for_probe_resume( monkeypatch: pytest.MonkeyPatch, ) -> None: - """F035: a provider-parked agent (dead container, OFFLINE, with a + """A provider-parked agent (dead container, OFFLINE, with a ``rate_limit_lifted`` WaitingRecord) must NOT be reaped by the stale-claim - reaper. The probe-resume loop owns its recovery and respawns it when the - provider recovers; reaping would release the claim to pending, and then - probe-success would respawn the agent on a task it no longer owns. + reaper — reaping would release the claim to pending and probe-success would + respawn the agent on a task it no longer owns. """ now = datetime.now(UTC) task_id = uuid4() diff --git a/tests/unit/services/optimal_brain/test_learnings_shareable_enforced.py b/tests/unit/services/optimal_brain/test_learnings_shareable_enforced.py index 15bd7251..24d7b68b 100644 --- a/tests/unit/services/optimal_brain/test_learnings_shareable_enforced.py +++ b/tests/unit/services/optimal_brain/test_learnings_shareable_enforced.py @@ -1,26 +1,7 @@ -"""F054: the LEARNINGS index must not leak private (shareable=False) entries -through ANY shared retrieval path. - -A private LEARNING journal entry is recorded into the LEARNINGS index with -``shareable=False`` (journal.py records it for completeness but it is never -meant to surface to other agents). The shared retrieval paths all reach the -plugin's retrieval with no ``include_private`` opt-in: - -- ``OptimalService.search`` (used by the briefing / ``similar_memory``) calls - ``search_with_embedding`` directly with no filters. -- ``search_learnings`` (shareable_only=True, the default) and the - ``get_learnings_by_category`` / ``get_learnings_by_role`` / - ``get_team_learnings`` cross-agent views call ``search`` with a filters dict - that does NOT carry a ``shareable`` key. - -The base ``_citations_to_results`` only filters when a ``shareable`` filter is -present, so a ``shareable=False`` chunk sails through into another agent's -briefing — a private reflection leaked across the cross-agent corpus. - -The fix: the LEARNINGS plugin forces ``shareable=True`` on retrieval unless the -caller explicitly opts into the private view via ``include_private=True`` (the -``search_learnings(shareable_only=False)`` audit/admin path). An empty filters -dict does NOT opt out — shareable is the safe default on every shared path. +"""The LEARNINGS index must not leak private (``shareable=False``) entries +through any shared retrieval path. The plugin forces ``shareable=True`` on +retrieval unless the caller opts into the private view via +``include_private=True``; an empty filters dict does NOT opt out. """ from __future__ import annotations diff --git a/tests/unit/services/optimal_brain/test_playbooks_index.py b/tests/unit/services/optimal_brain/test_playbooks_index.py index f107e243..5850376f 100644 --- a/tests/unit/services/optimal_brain/test_playbooks_index.py +++ b/tests/unit/services/optimal_brain/test_playbooks_index.py @@ -44,8 +44,8 @@ def test_build_source_uri_none_when_missing() -> None: def test_delete_playbook_removes_its_chunks_by_source() -> None: - """F011: deleting a playbook removes its embedded chunks from the vector - store by the playbook's source URI (idempotent — no-op if absent). A + """Deleting a playbook removes its embedded chunks from the vector store + by the playbook's source URI (idempotent — no-op if absent). A rejected/archived playbook must not stay retrievable in the PLAYBOOKS index.""" plugin = PlaybooksIndexPlugin.__new__(PlaybooksIndexPlugin) store = MagicMock() diff --git a/tests/unit/services/optimal_brain/test_replace_chunks_atomic.py b/tests/unit/services/optimal_brain/test_replace_chunks_atomic.py index 3d4a258a..636d1179 100644 --- a/tests/unit/services/optimal_brain/test_replace_chunks_atomic.py +++ b/tests/unit/services/optimal_brain/test_replace_chunks_atomic.py @@ -1,16 +1,7 @@ -"""F108 — ``VectorStore.replace_chunks`` must be a single atomic transaction. - -The replace-on-reingest path used to be ``delete_by_source`` (one pool -connection) followed by ``add_chunks`` (a *second* pool connection). Two -concurrent re-indexes of the same source interleaved across those two -connections and produced duplicate chunk rows; an add failure after a -successful delete also lost the source's index rows. The fix is a single -``replace_chunks(source, chunks)`` that deletes + inserts on ONE connection -inside ONE asyncpg transaction, so the whole replace is atomic. - -These tests mock the asyncpg pool/connection to assert the atomicity -invariant (single acquire, transaction entered, delete + insert on the -same connection) without standing up a pgvector DB. +"""``VectorStore.replace_chunks`` is a single atomic transaction: delete + +insert on ONE connection inside ONE asyncpg transaction, so concurrent +re-indexes can't interleave and an insert failure can't lose the source's +rows. These tests mock the asyncpg pool to assert that invariant. """ from __future__ import annotations diff --git a/tests/unit/services/test_conventions_cache_put.py b/tests/unit/services/test_conventions_cache_put.py index ed3504a9..8ae1dbb1 100644 --- a/tests/unit/services/test_conventions_cache_put.py +++ b/tests/unit/services/test_conventions_cache_put.py @@ -1,4 +1,4 @@ -"""ConventionsService._cache_put isolates a concurrent-duplicate insert (F042). +"""ConventionsService._cache_put isolates a concurrent-duplicate insert. Two task creates for the same project/HEAD can race to populate the conventions cache; the loser's INSERT fails the partial-unique index with @@ -84,10 +84,10 @@ def _mapping() -> ConventionsStandard: @pytest.mark.asyncio async def test_cache_put_tolerates_concurrent_duplicate_without_poisoning() -> None: - # F042: the loser of a concurrent cache-populate race must not crash the - # shared task-create session. The duplicate IntegrityError is contained to - # a savepoint; _cache_put returns cleanly, the session is not poisoned, and - # no full rollback undoes the outer task-create transaction. + # The loser of a concurrent cache-populate race must not crash the shared + # task-create session: the duplicate IntegrityError is contained to a + # savepoint, the session is not poisoned, and no full rollback undoes the + # outer task-create transaction. session = _FakeSession(duplicate=True) svc = ConventionsService(session=cast("Any", session)) diff --git a/tests/unit/services/test_escalation_board_guard.py b/tests/unit/services/test_escalation_board_guard.py index 43825477..c0e03b87 100644 --- a/tests/unit/services/test_escalation_board_guard.py +++ b/tests/unit/services/test_escalation_board_guard.py @@ -430,11 +430,8 @@ async def test_is_board_advisory_agent_classifies_roles() -> None: @pytest.mark.asyncio async def test_apply_escalation_refuses_completed_task() -> None: - # F043: a COMPLETED task is terminal — apply_escalation must not resurrect - # it to BLOCKED. The HTTP escalate route bypasses the spec gate, so the - # single write primitive must refuse terminal tasks itself. Returns False - # so callers (escalate / HTTP route) can surface a clean invalid_state / 409 - # instead of mutating a finished task. + # apply_escalation must refuse terminal tasks: the HTTP route bypasses the + # spec gate, so the primitive guards itself and returns False for a 409. svc = _service() original_assignee = uuid4() task = MagicMock( @@ -466,7 +463,7 @@ async def test_apply_escalation_refuses_completed_task() -> None: @pytest.mark.asyncio async def test_apply_escalation_refuses_cancelled_task() -> None: - # F043: cancelled is terminal too — must not be resurrected via escalation. + # cancelled is terminal too — must not be resurrected via escalation. svc = _service() task = MagicMock( id=uuid4(), @@ -495,8 +492,8 @@ async def test_apply_escalation_refuses_cancelled_task() -> None: @pytest.mark.asyncio async def test_apply_escalation_blocks_non_terminal_task() -> None: - # F043: the terminal guard must not over-restrict — a normal in_progress - # task still escalates (blocked + reassigned) and returns True. + # the terminal guard must not over-restrict: a normal in_progress task + # still escalates (blocked + reassigned) and returns True. svc = _service() target_id = uuid4() task = MagicMock( diff --git a/tests/unit/services/test_git_conventions_pr_dirty.py b/tests/unit/services/test_git_conventions_pr_dirty.py index 46be612f..bfef1f2a 100644 --- a/tests/unit/services/test_git_conventions_pr_dirty.py +++ b/tests/unit/services/test_git_conventions_pr_dirty.py @@ -1,15 +1,6 @@ -"""F051: open_conventions_pr must not operate on a dirty working tree. - -``open_conventions_pr`` cuts its scaffold branch in an agent's clone (or the -project's shared ``workspace_path``). It does ``checkout `` with -``check=False`` and then ``checkout -B ``. On a dirty tree the -``checkout `` either no-ops (already on base) or is refused and silently -swallowed; ``checkout -B `` then carries the agent's uncommitted -work onto the scaffold branch, and the ``commit`` sweeps it into the -project-level conventions commit — the agent's in-progress change is gone -from their working tree and rides a PR they never intended. Refuse a dirty -tree up front (return None, no checkout) so an active workspace is never -touched. +"""``open_conventions_pr`` refuses a dirty working tree up front (returns +None, no checkout) so an active agent workspace is never swept into a +project-level conventions commit. """ from __future__ import annotations diff --git a/tests/unit/services/test_git_lock_cleanup.py b/tests/unit/services/test_git_lock_cleanup.py index 13bf3f5d..cc301a59 100644 --- a/tests/unit/services/test_git_lock_cleanup.py +++ b/tests/unit/services/test_git_lock_cleanup.py @@ -1,14 +1,6 @@ -"""F019 — a git mutation op killed by ``_run_git``'s timeout orphans lock files. - -``subprocess.run(..., timeout=...)`` sends SIGKILL on timeout. A git mutation -(commit / merge --ff-only / rebase / reset --hard / add) killed mid-write -orphaned ``.git/index.lock`` (+ ``HEAD.lock`` / ``refs/**.lock`` / -``packed-refs.lock``), wedging the workspace for every subsequent op — -including the next fresh-claim ``reset --hard`` — with -"Another git process seems to be running in this repository". The fix -best-effort removes stale ``.git/**/*.lock`` files in the timeout branch -before re-raising, since the git process is dead by the time the timeout -fires. +"""A git mutation op killed by ``_run_git``'s timeout best-effort removes +orphaned ``.git/**/*.lock`` files before re-raising — the SIGKILL'd git +process can't clean up itself, and the locks wedge every subsequent op. """ from __future__ import annotations diff --git a/tests/unit/services/test_git_merge_method_fallback.py b/tests/unit/services/test_git_merge_method_fallback.py index 14799064..a56f1de1 100644 --- a/tests/unit/services/test_git_merge_method_fallback.py +++ b/tests/unit/services/test_git_merge_method_fallback.py @@ -167,13 +167,9 @@ async def test_merge_does_not_retry_when_method_allowed( async def test_merge_already_merged_pr_is_idempotent_success( monkeypatch: pytest.MonkeyPatch, ) -> None: - """F049: the CEO ``merge_pull_request`` path must treat an already-merged PR - as idempotent success, not raise GitError — mirroring ``_merge_with_retry`` - (the agent-facing path). A merge PUT on an already-merged PR returns the - same 405 as a genuine "not mergeable" refusal, so without disambiguation a - CEO retry (double-click, or a re-merge after a network blip where the first - PUT actually landed) raises GitError instead of no-opping — surfacing a - spurious failure on the very master-merge path the CEO owns. + """The CEO ``merge_pull_request`` path treats an already-merged PR as + idempotent success (not GitError) — a merge PUT on an already-merged PR + returns the same 405 as a genuine refusal, so they must be disambiguated. """ svc = _git_service() @@ -216,8 +212,8 @@ async def test_merge_already_merged_pr_is_idempotent_success( async def test_merge_raises_when_not_merged_and_refused( monkeypatch: pytest.MonkeyPatch, ) -> None: - """F049: a genuine merge refusal (not mergeable, NOT already-merged) still - raises GitError — the idempotency guard must not mask a real failure.""" + """A genuine merge refusal (not mergeable, NOT already-merged) still raises + GitError — the idempotency guard must not mask a real failure.""" svc = _git_service() monkeypatch.setattr( diff --git a/tests/unit/services/test_git_merge_pr_for_task_pr_match.py b/tests/unit/services/test_git_merge_pr_for_task_pr_match.py index a1333564..00cb68de 100644 --- a/tests/unit/services/test_git_merge_pr_for_task_pr_match.py +++ b/tests/unit/services/test_git_merge_pr_for_task_pr_match.py @@ -1,13 +1,6 @@ -"""F050: merge_pr_for_task must not merge a caller-provided pr_number that -doesn't match the task's recorded PR. - -``GitMergePRRequest.pr_number`` is caller-provided. When a ``task_id`` is -present the service knows the task's *own* recorded PR (``task.pr_number``), -set when the PR was opened. Without a match check a caller (a buggy client, a -stale panel form, an agent that cached an old PR number) can ask the CEO/PM -merge path to merge PR #N for task T whose recorded PR is #M — merging the -wrong PR against the wrong task's work-session and auto-complete. The recorded -PR is the source of truth; the caller's number must agree with it. +"""``merge_pr_for_task`` rejects a caller-provided ``pr_number`` that doesn't +match the task's recorded ``task.pr_number`` — the recorded PR is the source +of truth, so a stale caller number can't merge the wrong PR for a task. """ from __future__ import annotations diff --git a/tests/unit/services/test_git_token_decryption_log.py b/tests/unit/services/test_git_token_decryption_log.py index c4fd2f06..703bf4a7 100644 --- a/tests/unit/services/test_git_token_decryption_log.py +++ b/tests/unit/services/test_git_token_decryption_log.py @@ -1,16 +1,8 @@ -"""F053: _token_for_project must log a Fernet decryption failure with the -project context, not swallow it silently as 'no token'. +"""Log a Fernet decryption failure with the project slug before returning None. -On an encryption-key rotation the stored PAT (encrypted with the old key) can't -be decrypted — ``crypto.decrypt_token`` raises ``EncryptionError``. The -crypto layer logs a generic message, but ``_token_for_project`` catches the -``EncryptionError`` and returns ``None`` with no project context, so every -best-effort workspace git op (push, PR, clone-with-token) silently looks like -'this project has no token' — indistinguishable from a project that genuinely -never set one. The operator can't tell which project is wedged by a key -rotation. Log the failure with the project slug before returning None (the -best-effort skip behavior is preserved — this only makes the cause -diagnosable). +On a key rotation the stored PAT can't be decrypted; ``_token_for_project`` must +not mask that as a silent 'no token' — log the project slug so the cause is +diagnosable (best-effort skip behavior preserved). """ from __future__ import annotations diff --git a/tests/unit/services/test_messaging_channel_race.py b/tests/unit/services/test_messaging_channel_race.py index bd3f1fff..99df4f82 100644 --- a/tests/unit/services/test_messaging_channel_race.py +++ b/tests/unit/services/test_messaging_channel_race.py @@ -1,15 +1,8 @@ -"""F055: ``get_or_create_channel_by_slug`` must recover from a concurrent -auto-create race on the channel slug's UNIQUE constraint instead of crashing -the caller with an ``IntegrityError``. +"""Recover from a concurrent auto-create race on the channel slug's UNIQUE +constraint instead of crashing the caller with an ``IntegrityError``. -Two concurrent callers (e.g. two Main-PM group-create requests hitting the -groups route) both miss the lookup, both auto-create the same seed channel, -and the loser's ``flush`` raises ``IntegrityError`` on ``channels.slug`` -unique. With no handling that propagates as a 500 to whichever caller lost the -race, even though the channel they wanted now exists. The fix: isolate the -insert in a savepoint, and on a unique-conflict ``IntegrityError`` re-fetch -the now-existing channel (the winner's row) and return it. A conflict that -did NOT produce a row on re-fetch is re-raised (don't mask a real failure). +Isolate the insert in a savepoint; on a unique-conflict ``IntegrityError`` +re-fetch the winner's row. A conflict that did NOT produce a row is re-raised. """ from __future__ import annotations diff --git a/tests/unit/services/test_messaging_session_race.py b/tests/unit/services/test_messaging_session_race.py index 7ee55583..8181ddd5 100644 --- a/tests/unit/services/test_messaging_session_race.py +++ b/tests/unit/services/test_messaging_session_race.py @@ -1,18 +1,8 @@ -"""F056: ``create_session`` (and its delegate ``get_or_create_active_session``, -plus the L1868 channel-post adapter that routes through it) must not orphan an -ACTIVE session under concurrent posts. +"""``create_session`` must not orphan an ACTIVE session under concurrent posts. -``create_session`` does a plain check-then-create: read ``group.active_session_id``, -reuse if ACTIVE, else INSERT a new ACTIVE session and point the group at it. -Two concurrent posts can both miss the active session, both INSERT, and the -second ``flush`` overwrites ``group.active_session_id`` — the first session -stays ACTIVE but unreferenced (orphaned) forever. There is no DB uniqueness on -``(group_id, status='active')`` (tables.py:1121-1125 only carries indexes), so -nothing stops the double-insert. - -The fix: lock the group row (``SELECT ... FOR UPDATE``) and re-read -``active_session_id`` under the lock before deciding to create, so concurrent -callers serialize per group and the loser reuses the winner's session. +Lock the group row (``SELECT ... FOR UPDATE``) and re-read +``active_session_id`` under the lock before creating, so concurrent callers +serialize per group and the loser reuses the winner's session. """ from __future__ import annotations diff --git a/tests/unit/services/test_notification.py b/tests/unit/services/test_notification.py index 6a57d9cd..0552cd94 100644 --- a/tests/unit/services/test_notification.py +++ b/tests/unit/services/test_notification.py @@ -325,7 +325,7 @@ async def test_create_notification_skips_when_no_resolvable_recipients( # --------------------------------------------------------------------------- -# F009 — requires_ack must follow ACK_REQUIRED_BY_TYPE, not the True default +# requires_ack must follow ACK_REQUIRED_BY_TYPE, not the True default # --------------------------------------------------------------------------- @@ -333,8 +333,8 @@ async def test_create_notification_skips_when_no_resolvable_recipients( async def test_informational_notification_does_not_require_ack( svc: NotificationService, ) -> None: - """F009: REVIEW_REQUEST / DOCUMENTATION_REQUEST / A2A_REQUEST are - informational (pickup proves receipt) — requires_ack must be False, not the + """REVIEW_REQUEST / DOCUMENTATION_REQUEST / A2A_REQUEST are informational + (pickup proves receipt) — requires_ack must be False, not the NotificationTable True default. A False type forced to True inflates the recipient's unacked set and soft-blocks i_am_idle → respawn churn.""" aid = uuid4() @@ -368,7 +368,7 @@ async def test_informational_notification_does_not_require_ack( async def test_action_required_notification_still_requires_ack( svc: NotificationService, ) -> None: - """F009: BLOCKER_ESCALATION / APPROVAL / ALERT are action-required — + """BLOCKER_ESCALATION / APPROVAL / ALERT are action-required — requires_ack stays True (ACK_REQUIRED_BY_TYPE maps them True).""" aid = uuid4() db = _FakeDb(agent_uuid=aid) @@ -389,8 +389,8 @@ async def test_action_required_notification_still_requires_ack( async def test_create_notification_requires_ack_derives_from_type( svc: NotificationService, ) -> None: - """F009: a raw _create_notification call derives requires_ack from the type - via ACK_REQUIRED_BY_TYPE (KNOWLEDGE_SHARE → False).""" + """A raw _create_notification call derives requires_ack from the type via + ACK_REQUIRED_BY_TYPE (KNOWLEDGE_SHARE → False).""" aid = uuid4() db = _FakeDb(agent_uuid=aid) with _patch_db_context(db): diff --git a/tests/unit/services/test_playbook_index_ordering.py b/tests/unit/services/test_playbook_index_ordering.py index 6d0b1409..465eb3b9 100644 --- a/tests/unit/services/test_playbook_index_ordering.py +++ b/tests/unit/services/test_playbook_index_ordering.py @@ -1,19 +1,9 @@ -"""F057: the PLAYBOOKS RAG index write must not commit independently of — and -BEFORE — the playbook status transaction. +"""The PLAYBOOKS RAG index write must not commit independently of — and BEFORE — +the playbook status transaction. -``approve()`` / ``reject()`` used to call ``_index_approved`` / ``_unindex_playbook`` -inline, AFTER ``flush()`` but BEFORE the caller's ``commit()``. The vector store -writes chunks via its OWN pool connection (vector_store.py:211-237), which -auto-commits immediately and independently of the SQLAlchemy session -transaction. So a status-commit failure (or a crash between the index write and -the commit) left the RAG corpus with an approved/archived playbook whose DB row -was still DRAFT/APPROVED — a divergence agents then surfaced in briefings. - -The fix: ``approve()`` / ``reject()`` flush the status ONLY; the index/unindex -is a separate post-commit step (``index_approved`` / ``unindex_playbook``) the -caller runs AFTER the status transaction commits. Both entry points — the panel -route (playbooks.py) and the Auditor gateway verb (content_actions. -_curate_playbook) — commit-then-index, and skip the index if the commit fails. +``approve()`` / ``reject()`` flush the status only; the index/unindex is a +separate post-commit step the caller runs after the status commits (skipped if +the commit fails), so the RAG corpus never diverges from the DB row. """ from __future__ import annotations @@ -61,7 +51,7 @@ async def test_approve_does_not_index_before_commit( ) -> None: """``approve()`` flushes the status change but must NOT write to the RAG index — that writes through its own auto-committing connection, so it would - durably land before the caller commits the status (the F057 divergence).""" + durably land before the caller commits the status (the divergence).""" monkeypatch.setattr(settings, "org_memory_enabled", True) session = AsyncMock() session.flush = AsyncMock() diff --git a/tests/unit/services/test_playbook_slug_race.py b/tests/unit/services/test_playbook_slug_race.py index 74a1fe7d..54c1d787 100644 --- a/tests/unit/services/test_playbook_slug_race.py +++ b/tests/unit/services/test_playbook_slug_race.py @@ -1,18 +1,10 @@ -"""F110 — ``PlaybookService.draft`` slug TOCTOU must not 500. +"""``PlaybookService.draft`` slug TOCTOU must not 500. -``draft`` pre-checks the slug with ``_get_by_slug`` then INSERTs. Two concurrent -same-title drafts both miss the pre-check (neither sees the other's uncommitted -row), so the loser's flush hits the ``playbooks.slug`` UNIQUE constraint and -raises ``IntegrityError``. The pre-check alone cannot close the race — the DB -constraint is the authoritative guard. The fix wraps the insert in a savepoint -and converts the ``IntegrityError`` into a clean ``ConflictError`` (the same -error the pre-check raises), so the loser gets a 409, not an unhandled 500. - -Playbooks are distinct curated content (unlike shared-infrastructure channels, -where the loser reuses the winner's row): two same-title drafts are two -different procedures that collided on the derived slug, so the loser must be -told to retry with a distinct title — it must NOT silently reuse the winner's -row (that would drop the loser's content). +Two concurrent same-title drafts both miss the pre-check; the loser's flush +hits the ``playbooks.slug`` UNIQUE constraint. The fix wraps the insert in a +savepoint and converts ``IntegrityError`` into a clean ``ConflictError`` (409), +so the loser is told to retry with a distinct title — it must NOT silently +reuse the winner's row (that would drop the loser's content). """ from __future__ import annotations @@ -68,7 +60,7 @@ def _create(title: str = "Retry flaky pg") -> PlaybookCreate: async def test_draft_slug_race_raises_conflict_not_integrity_error() -> None: """Concurrent same-title loser: pre-check misses (None), flush raises IntegrityError on the UNIQUE slug — draft must convert it to a clean - ConflictError, not let it propagate as an unhandled 500 (F110).""" + ConflictError, not let it propagate as an unhandled 500.""" svc, session = _svc(flush_side_effect=_integrity_error()) # Pre-check misses the row (the race window: the other draft is uncommitted). object.__setattr__(svc, "_get_by_slug", AsyncMock(return_value=None)) @@ -84,7 +76,7 @@ async def test_draft_slug_race_raises_conflict_not_integrity_error() -> None: @pytest.mark.asyncio async def test_draft_happy_path_still_inserts() -> None: """No race: pre-check misses, flush succeeds — the savepoint path is used - and the row is added (regression guard for the F110 wrap).""" + and the row is added (regression guard for the savepoint wrap).""" svc, session = _svc() object.__setattr__(svc, "_get_by_slug", AsyncMock(return_value=None)) diff --git a/tests/unit/services/test_release_executor_commit_fail_closed.py b/tests/unit/services/test_release_executor_commit_fail_closed.py index a3162158..44721d43 100644 --- a/tests/unit/services/test_release_executor_commit_fail_closed.py +++ b/tests/unit/services/test_release_executor_commit_fail_closed.py @@ -1,10 +1,8 @@ -"""F012 — ``_GitReleaseOps.commit_and_push`` must be fail-closed on commit. +"""``_GitReleaseOps.commit_and_push`` must be fail-closed on commit. -The release commit step discarded the ``git add`` / ``git commit`` return codes: -on a failed commit (gpgsign unavailable, pre-commit hook rejection, nothing to -commit after a no-op bump) the code still ran ``rev-parse HEAD`` + pushed the -pre-bump base, so ``gh release create`` would tag the *old* tree as the new -version. The fix checks both return codes and raises before any push. +A failed ``git add`` / ``git commit`` (gpgsign unavailable, pre-commit rejection, +no-op bump) must raise before any push — otherwise ``gh release create`` tags +the pre-bump base as the new version. """ from __future__ import annotations diff --git a/tests/unit/services/test_release_executor_subprocess_timeout.py b/tests/unit/services/test_release_executor_subprocess_timeout.py index d3a2c028..a3b11fa7 100644 --- a/tests/unit/services/test_release_executor_subprocess_timeout.py +++ b/tests/unit/services/test_release_executor_subprocess_timeout.py @@ -1,13 +1,6 @@ -"""F078 — ``_GitReleaseOps`` subprocesses (git, ``make quality``, ``gh release -create``, the release-clone ``git clone``) had no deadline: a hung child would -block the CEO-gated release loop indefinitely. - -The fix wraps each ``proc.communicate()`` in ``asyncio.wait_for`` and, on -expiry, ``proc.kill()``s the child and returns a non-zero rc (124) so the -caller fails closed — mirroring the quality-gate ``_run_one`` kill-on-timeout -idiom. The deadlines are generous (a full ``make quality`` run, a network -push/clone can legitimately take minutes) so a healthy release is never -wrongly aborted. +"""``_GitReleaseOps`` subprocesses (git, ``make quality``, ``gh release create``, +the release-clone ``git clone``) are wrapped in ``asyncio.wait_for`` with a +kill-on-timeout fail-close so a hung child cannot block the release loop. These tests hang the subprocess (a never-resolving ``communicate``) and patch the timeout constants tiny so a deterministic fail-close is asserted in well diff --git a/tests/unit/services/test_release_proposal_concurrency.py b/tests/unit/services/test_release_proposal_concurrency.py index d2536e05..d36ea9f8 100644 --- a/tests/unit/services/test_release_proposal_concurrency.py +++ b/tests/unit/services/test_release_proposal_concurrency.py @@ -1,12 +1,6 @@ -"""F013 — concurrent approve races on the shared release clone. - -The approve flow ran the ~40min ``ReleaseExecutor.execute`` with no guard, so -two concurrent CEO ``POST /proposal/approve`` calls (double-click, panel retry) -both found the same held proposal and raced on the shared, ``rm -rf``'d writable -release clone — interleaving ``git add``/``commit``/``push`` and corrupting the -release. The fix acquires a Redis ``SET NX`` mutex keyed by the proposal id -before execute (TTL > the 40min CI ceiling) and releases it on completion; a -second concurrent approve sees the lock held and refuses instead of racing. +"""Concurrent CEO approve races on the shared release clone are serialized by a +Redis ``SET NX`` mutex keyed by the proposal id; a second concurrent approve +sees the lock held and refuses instead of racing on the writable clone. """ from __future__ import annotations diff --git a/tests/unit/services/test_release_readiness_first_release.py b/tests/unit/services/test_release_readiness_first_release.py index 845980c1..490dfed3 100644 --- a/tests/unit/services/test_release_readiness_first_release.py +++ b/tests/unit/services/test_release_readiness_first_release.py @@ -1,17 +1,6 @@ -"""F058: the FIRST release (no prior ``chore(release):`` commit) must still -produce a non-empty version-bump plan. - -``_canonical_bump_files`` derived the bump-target set from the previous -``chore(release):`` commit's touched files. On the first release ever there is -no such commit, so it returned ``[]`` → ``assess`` set -``version_bump_plan=[]`` → ``ReleaseExecutor.apply_version_bumps`` bumped NO -files and published a tag masquerading as X.Y.Z with nothing actually changed. - -The fix: when no prior release commit exists, fall back to the version- -reference scan — the files currently embedding the version string are exactly -the set a first release must bump (and the set a subsequent release's -``chore(release):`` commit would record as canonical). This is read-only -derivation only; the CEO-approval gate and fail-closed executor are untouched. +"""The FIRST release (no prior ``chore(release):`` commit) must still produce a +non-empty version-bump plan: ``_canonical_bump_files`` falls back to the +version-reference scan when no prior release commit exists. """ from __future__ import annotations @@ -60,7 +49,7 @@ def _first_release_repo(tmp_path: Path) -> Path: def test_canonical_bump_files_falls_back_on_first_release(tmp_path: Path) -> None: """No prior ``chore(release):`` commit ⇒ the canonical set is the version- - reference scan, NOT empty (the F058 regression: it returned ``[]``).""" + reference scan, NOT empty.""" root = _first_release_repo(tmp_path) files = _canonical_bump_files(root, "0.1.0") assert files # non-empty diff --git a/tests/unit/services/test_self_heal_originate_db.py b/tests/unit/services/test_self_heal_originate_db.py index 815cbd82..71e20a61 100644 --- a/tests/unit/services/test_self_heal_originate_db.py +++ b/tests/unit/services/test_self_heal_originate_db.py @@ -151,9 +151,8 @@ async def test_originate_creates_pending_main_pm_assigned_task( # Assigned to the Main PM agent up front (not just team=main_pm) so that, once # the CEO approves it, the orchestrator dispatches it straight to that agent. assert task.assigned_to == MAIN_PM_UUID - # F059: held for the CEO's Approve-&-Start — NOT auto-confirmed. The - # orchestrator + give_me_work keep it out of dispatch until the CEO - # approves it (approve_and_start flips this True). + # held for the CEO's Approve-&-Start — NOT auto-confirmed: the orchestrator + # + give_me_work keep it out of dispatch until approve_and_start flips this. assert task.confirmed_by_human is False assert task.team == Team.MAIN_PM assert task.source == "self_heal" diff --git a/tests/unit/services/test_task.py b/tests/unit/services/test_task.py index 1355b77b..db431896 100644 --- a/tests/unit/services/test_task.py +++ b/tests/unit/services/test_task.py @@ -1065,16 +1065,10 @@ async def test_ensure_branch_raises_when_neither_project_nor_product() -> None: @pytest.mark.asyncio async def test_finalize_claim_rollback_emits_reversal_audit() -> None: - """F060: when branch creation fails mid-claim, the rollback must emit a - REVERSAL audit row (CLAIMED -> original) so the audit journey doesn't - diverge from the real (rolled-back) task state. - - The audit service writes on its own connection (fire-and-forget), so the - forward `task.claimed` row committed at the pre-branch flush is NOT undone - by the rollback's flush. Without a matching reversal row, the journey's - last event stays `task.claimed` while the task is back to PENDING — the - audit trail diverges from real state and corrupts every downstream metric - reconstructed from `task.` events (cycle time, bottlenecks). + """When branch creation fails mid-claim, the rollback must emit a REVERSAL + audit row (CLAIMED -> original) so the audit journey matches the real + (rolled-back) task state. The audit service writes on its own connection, so + the forward `task.claimed` row is NOT undone by the rollback's flush. """ session = MagicMock() session.flush = AsyncMock() @@ -1121,19 +1115,11 @@ async def test_finalize_claim_rollback_emits_reversal_audit() -> None: @pytest.mark.asyncio async def test_emit_status_transition_audit_writes_in_session_atomically() -> None: - """F061/F073/F075: the status-transition audit row is written into the - CALLER's session (same transaction as the transition), not fire-and-forget - on a separate connection. - - Fire-and-forget decouples the audit commit from the transition commit: - a transition that rolls back inside a verb savepoint leaves a PHANTOM audit - row (F075), and a swallowed persist failure means a committed transition - can have NO audit row (F073) — silently corrupting the cycle-time / - bottleneck metrics reconstructed from ``task.`` events (F061). - Writing the row in-session makes it commit/roll back atomically with the - transition, closing all three. Asserted at the unit level: the row is - ``session.add``-ed (same txn) with the metric-reconstruction details, and - NO fire-and-forget background task is spawned. + """The status-transition audit row is written into the CALLER's session (same + transaction as the transition), not fire-and-forget on a separate connection, + so it commits/rolls back atomically with the transition and cannot diverge + from real state. Asserted at the unit level: the row is ``session.add``-ed + (same txn) and NO fire-and-forget background task is spawned. """ session = MagicMock() added: list[object] = [] diff --git a/tests/unit/services/test_work_session.py b/tests/unit/services/test_work_session.py index e73f687c..c928f706 100644 --- a/tests/unit/services/test_work_session.py +++ b/tests/unit/services/test_work_session.py @@ -78,7 +78,7 @@ async def test_has_unpushed_commits_false_when_session_missing() -> None: # --------------------------------------------------------------------------- -# F062 — merge_pr must be idempotent + active-guarded like close()/complete() +# merge_pr must be idempotent + active-guarded like close()/complete() # --------------------------------------------------------------------------- @@ -114,9 +114,9 @@ async def test_merge_pr_completes_active_session() -> None: @pytest.mark.asyncio async def test_merge_pr_idempotent_on_already_completed_preserves_audit_trail() -> None: - """F062 (mode 1): a retried merge after a successful-but-unconfirmed GitHub - merge must NOT overwrite the original ``merged_by`` / ``pr_merged_at`` — the - merge audit trail is preserved. Mirrors close()'s idempotency guard.""" + """A retried merge after a successful-but-unconfirmed GitHub merge must NOT + overwrite the original ``merged_by`` / ``pr_merged_at`` — the merge audit + trail is preserved. Mirrors close()'s idempotency guard.""" original_merger = uuid4() original_ts = datetime(2026, 6, 1, 12, 0, tzinfo=UTC) @@ -144,9 +144,9 @@ async def test_merge_pr_idempotent_on_already_completed_preserves_audit_trail() @pytest.mark.asyncio async def test_merge_pr_does_not_resurrect_abandoned_session() -> None: - """F062 (mode 2): merge_pr on an ABANDONED session must NOT flip it to - COMPLETED — that would silently undo the single-active invariant's - abandonment and make discarded work look like a successful merge.""" + """``merge_pr`` on an ABANDONED session must NOT flip it to COMPLETED — that + would silently undo the single-active invariant's abandonment and make + discarded work look like a successful merge.""" ws = MagicMock(status=WorkSessionStatus.ABANDONED, pr_number=42, merged_by=None) svc, session = _merge_service() diff --git a/tests/unit/services/test_workspace_clone_pat_leak_cleanup.py b/tests/unit/services/test_workspace_clone_pat_leak_cleanup.py index 4f5501ac..6746a88d 100644 --- a/tests/unit/services/test_workspace_clone_pat_leak_cleanup.py +++ b/tests/unit/services/test_workspace_clone_pat_leak_cleanup.py @@ -1,21 +1,9 @@ -"""F063 — a failed ``_configure_git`` must not leave the project PAT on disk. +"""A failed ``_configure_git`` must not leave the project PAT on disk. -``_clone_repo`` runs ``_do_clone`` (which writes the tokenized auth URL into -``.git/config``), then ``_configure_git`` (which scrubs it via -``git remote set-url origin ``), then ``_assert_no_pat_leak``. If -``_configure_git`` raises ``CalledProcessError`` BEFORE the scrub completes -(disk error, permission issue, broken git), the PAT stays in ``.git/config`` -and ``_assert_no_pat_leak`` never runs. The except clauses raised -``WorkspaceError`` without removing the workspace, so on the next -``ensure_workspace`` the health short-circuit (a valid ``.git`` with HEAD + -objects) skipped straight past the leak — the agent was then mounted on a -workspace whose ``.git/config`` still carried ``https://TOKEN@github.com/...``, -letting it read and exfiltrate the project PAT. - -The fix: the clone-failure except clauses ``rmtree`` the workspace before -raising, so a half-configured clone is destroyed and the next -``ensure_workspace`` re-clones from scratch instead of short-circuiting past -the leak. +The clone-failure except clauses ``rmtree`` the workspace before raising, so a +half-configured clone (PAT still in ``.git/config``) is destroyed and the next +``ensure_workspace`` re-clones from scratch instead of short-circuiting past the +leak on a valid ``.git`` health check. """ from __future__ import annotations diff --git a/tests/unit/test_notification_dedup.py b/tests/unit/test_notification_dedup.py index 6b96b3f7..fa2a71a8 100644 --- a/tests/unit/test_notification_dedup.py +++ b/tests/unit/test_notification_dedup.py @@ -75,12 +75,11 @@ async def test_create_notification_suppresses_same_purpose_duplicate() -> None: @pytest.mark.asyncio async def test_informational_knowledge_share_not_deduped() -> None: - """F010: KNOWLEDGE_SHARE (informational, requires_ack=False) must NOT be - deduped. Each learning broadcast carries distinct content (a new learning); - a recipient who never acks the prior one (acking is voluntary for - informational types) would permanently suppress every subsequent - knowledge-share from the same sender → silent learning-broadcast data loss. - The dedup's anti-loop rationale only applies to action-required types.""" + """KNOWLEDGE_SHARE (informational, requires_ack=False) must NOT be deduped: + a recipient who never acks the prior one would permanently suppress every + subsequent knowledge-share from the same sender → silent learning-broadcast + data loss. The dedup's anti-loop rationale only applies to action-required + types.""" db = MagicMock() # A same-purpose unacked KNOWLEDGE_SHARE prior exists — but it must NOT # suppress the new one.