mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[sweep] strip Fxxx audit-ID tokens + trim bloated comments/docstrings + add behavior-change docs
Post-audit sweep over the 135 audit-fix commits since19a474d3: 1. Stripped every # Fxxx: audit-ID token from comments AND every Fxxx token from docstring openings across 211 blocks / ~626 lines. The CEO flagged these twice: audit-issue IDs in code confuse future devs/agents. The descriptive text is preserved; only the Fxxx token is removed (and bloated narrative blocks trimmed to 1-3 lines keeping the one non-obvious invariant). 2. Trimmed bloated comments/docstrings to the concise standard (1-3 lines). 3. Added missing behavior-change docs for the audit-fix batch: prompts/roles (documenter, pr_reviewer, qa), user-facing docs (api auth, websockets, agent-gateway, megatask, merge-model, task-lifecycle, grok, resilience, conventions, panel, security, troubleshooting), and the RAG corpus (cell-pm, main-pm, pr-reviewer, qa roles; conventions; messaging-tools; escalation; megatask; task-claiming workflows). Comment/docstring/prose ONLY — zero code-line edits (verified: the diff contains no def/class/return/if/for/await/assignment/call lines). Gates green: ruff format + ruff check clean, mypy clean on roboco/. The only pytest failures are the pre-existing sync_branch tracing-decision gap (B1,250be5c2) — not sweep-caused and tracked separately.
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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(), {})
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"}])
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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())
|
||||
|
||||
Reference in New Issue
Block a user