mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Chore/all/logical gaps sweep (#286)
* release-manager: fencing-token mutex + executor/readiness hardening
Closes the release-mutex TTL race (#17, HIGH) and the remaining
release-manager gaps (#88, #89, #201, #202):
- #17: the release mutex is now acquired with a uuid4 fencing token and
released via Lua compare-and-del; a background asyncio heartbeat
compare-and-expires the TTL ~every 60s while the execute owns the lock,
so a live execute no longer expires and a crashed one auto-releases
<=3000s. A second approve after TTL expiry cannot usurp and rm -rf the
in-flight clone — the fenced first-finally keeps its lock.
- #89: a Redis outage during acquire stays fail-closed (the execute never
runs) but now returns a distinct redis_unavailable result + log so the
CEO sees the cause instead of a false already_in_progress.
- #88: commit_and_push RuntimeError is wrapped into a structured
ReleaseResult(commit_failed) instead of a 500.
- #201: first-release fallback still emits untracked version-ref files as
gaps (no longer silenced by the first-release branch).
- #202: _await_proc awaits proc.wait() after kill() so a timeout cannot
leak a zombie.
TDD: tests/unit/services/test_release_proposal_concurrency.py extends
_FakeRedis with eval/get/expire and pins the fencing/heartbeat/usurper
invariants + the redis_unavailable result.
* PM/code-task creation guard + main_pm coverage + issue carve-out
Closes the creation-time role x task_type gap (the user's explicit example)
and the main_pm delegate hole:
- New pure helper `pm_cannot_own_code(role, task_type, is_issue_resolution)`
in foundation/policy/batch.py — single source of truth. Both PM roles
(cell_pm + main_pm) coordinate; a `code` task assigned/claimed by a PM is
a structural mismatch, EXCEPT a PM taking a code task in needs_revision to
resolve review/QA issues directly (the carve-out).
- Creation-time guard: TaskService.create calls the helper (closes the
create-with-cell-PM-assignee hole the team-based check misses).
- Delegate path + spec claim gate consult the same helper.
`_validate_assignee_task_type` / `_task_type_hint_for` now key on the
Role (CELL_PM OR MAIN_PM), not the cell-PM slug set — closes the
delegate-to-main-pm-as-code hole.
- identity.role_for_uuid_or_none is None-tolerant (treats None as "not a
PM" and proceeds) so a malformed/missing assignee cannot crash the guard.
- prompter.create_task_from_draft reuses the guard at draft-create.
TDD: test_batch.py (helper matrix + carve-out), test_main_pm_code_guard.py
(main_pm coverage), test_delegate_assignee_task_type.py (delegate parity),
test_lifecycle_spec.py (claim gate: rejects PM claiming code from pending,
allows from needs_revision + PM claiming planning + dev claiming code).
* task-service: completion hooks + escalation/cancel/audit hardening
Closes the task-service cluster (#21/#98, #99, #100, #101, #103, #216; #102
verified already-covered, #217 verified already-guarded):
- #21/#98: ceo_approve now closes the work session + triggers completion
hooks before worktree removal (no-op when work_session_id is None), so a
CEO-approved task lands the same close-path as PM-completed.
- #99: apply_escalation routes through the transition validator with an
enumerated escalation exemption (_ESCALATABLE_TO_BLOCKED) instead of an
arbitrary source->BLOCKED write; BACKLOG is refused.
- #100: branchless ceo_reject awaiting_ceo_approval->pending gets a real
spec edge (ceo_reject_to_pool ActionSpec + _STATUS_TRANSITIONS entry) so
future admin-override tightening can't wedge the path.
- #101: revision_count bump is documented as the single chokepoint, with
the pre-block RESTORE path undoing it when restoring a snapshotted
needs_revision (same cycle resuming, not a new rejection).
- #103: cancel cascade surfaces non-terminal orphans instead of swallowing
the role violation.
- #216: _remove_task_worktree_on_terminal escalates recurring FS/permission
failure (audit/notify after N) instead of silent-failing forever.
- #102: pinned in test_verb_runner_midverb_invalid_state.py (committed with
the choreographer cluster) — verb-runner savepoints already surface a
concurrent mid-verb state change as INVALID_STATE.
- #217: submit_for_qa claimed_by guard verified intact.
TDD: test_task.py, test_worktree_cleanup_on_complete.py,
test_escalation_board_guard.py (#99), test_task_service_* integration,
test_lifecycle_spec.py.
* choreographer: gate-claim guards + pr-gate hardening + fail-open logging
Closes the choreographer cluster (#5/#222, #29, #30, #82, #188, #189,
#192; #157/#187 verified already-fixed/pinned; #102 pin lives here):
- #5/#222: the unchanged-PR guard's fail-open head_sha lookup now logs
(warning) on a slug-resolver/git-helper error so a regression cannot
silently turn the pr_fail re-submit loop-stopper into a no-op. Stays
fail-open (never wedges the PM).
- #29: pinned (REFUTED-with-pin) — _lane_claim_guard already returns the
error envelope without releasing the claim on a transient lookup error.
- #30: pinned (REFUTED-with-negative-pin) — a non-batch branchless main_pm
root cannot bypass the complete spec gate (is_batch_umbrella requires
batch_id set).
- #82: _post_gate_review_to_pr wraps the slug-resolution call in try/except
(mirrors _capture_pr_head_sha) so a malformed cell_map AttributeError no
longer 500s the reviewer after a committed gate transition.
- #188: _is_hand_formatted_verdict anchors the header regex to line-start,
so a quoted (> ## Summary) or inline (mid-prose) header mention no longer
false-refuses a hand-formatted verdict.
- #189: pr_fail re-captures the PR head SHA after the transition commits and
re-stamps the verdict note only when it advanced (closes the stale-SHA
false-allow loop-hole); no-advance stays a single note write.
- #192: claim_gate_review skips the dev claim guards (already_active/paused/
lane) via a new skip_dev_guards param — a pr_reviewer inspecting an
assembled PR does not start work, so the single-active-task / code-lane
invariants do not apply; the dependency guard is kept, and QA's
claim_review parity is preserved.
- #157/#187: verified in tree — pr_review-only handoff is intentionally
prior-work-worth-resuming; self_review_block wiring (reviewer != dev)
holds on assembled tasks with 4 existing pin tests.
TDD: test_choreographer_*, test_pr_gate_posts_review (#82),
test_pr_review_hand_format_guard (#188), test_submit_root_unchanged_pr_guard
(#189), test_claim_gate_review_guards (#192), test_verb_runner_midverb
_invalid_state (#102 pin).
* playbook curate: guard the gating commit against a poisoned session (#55)
The explicit `session.commit()` that gates the RAG index (commit-before-index
so an uncommitted playbook cannot land in the corpus) raised PendingRollbackError
when a prior mid-verb failure had rolled the caller's session back — 500ing the
whole curation verb instead of returning a clean envelope, and (worse) risking a
fall-through to index an uncommitted playbook. Wrap the commit: on
PendingRollbackError, log + return invalid_state with a re-fetch/retry remediate
and skip the index. The happy path still commits exactly once then indexes.
TDD: test_playbook_verbs.py — poisoned-session returns a clean invalid_state and
does NOT index; clean-session still commits once + indexes (pins no fail-closed
inversion / no double-commit).
* [chore] gateway: atomic activate merge — preserve probe_failures across re-park (#156)
activate() was a blind SET that reset probe_failures to 0, so a probe-failure
increment that just landed (or was in flight) could be wiped by a concurrent
re-park — resetting the give-up / CEO-notify count mid-episode. Route activate
through a server-side Lua merge (roboco:activate_rate_limit) that refreshes the
episode metadata (kind / activated_at / retry_after / affected_agents) while
carrying over the previous probe_failures count. Indivisible w.r.t. the
increment/reset scripts (Redis single-threads an EVAL).
#56 (notify to prompter/secretary refused) verified SAFE — the pin tests
(test_notify_rejects_prompter_recipient / _secretary_recipient /
_allows_ceo_recipient) already cover the only human notify target invariant;
no legitimate send is dropped, no code change.
* [chore] foundation/policy: spec gates + QA retry-key pin (Cluster F)
#50 sync_branch composes=() so the spec gate accepted a terminal/paused/
blocked task and the handler rebased a dead/parked branch — add a
PRECONDITION_SYNC_BRANCH_STATE (claimed/in_progress/verifying/needs_revision
only), rejection_kind=invalid_state. TDD: 28 spec tests.
#148 submit_root's prose asserts 'a Main-PM root is planning-typed, never
code' but only the creation path (main_pm_cannot_own_code) backed it — add
PRECONDITION_ROOT_NOT_CODE on the submit_root IntentSpec (defense in depth),
scoped to submit_root only so the shared submit_for_review action keeps
cell_pm+code submit_up parity. Graceful on Mock/None task_type so the
choreographer Mock-task tests don't crash. TDD: 2 spec tests.
#150 VERB_RETRY_LIMITS is keyed by the MCP-exposed names (pass/fail), not
the IntentSpec-internal pass_review/fail_review — already correct; add a
pin test so a one-sided rename can't silently drop the QA-handoff cap.
#142 main_pm_cannot_own_code/pm_cannot_own_code already normalize casing
(.lower()) — no-op, pin test test_main_pm_cannot_own_code_is_case_insensitive
already in tree.
* [chore] worksession-git: 405 merge-method fallback + non-destructive close (Cluster W)
#108 _merge_with_retry hardcoded 'squash' and raised MergeConflictError on a
405 with no method fallback — wedging the PM on an open, mergeable PR whose
repo merely had the squash button off. Add a 405 fallback to a permitted
method (via _first_allowed_merge_method, exclude='squash'), mirroring the CEO
merge_pull_request path. A 405 with no permitted fallback (or a second 405)
still falls through to the already-merged disambiguation / MergeConflictError.
TDD: 2 new tests (fallback-success, no-permitted-method-raises).
#109 close_pull_request defaulted delete_branch=True, so the choreographer
supersede path deleted a superseded PR's branch while the orchestrator
supersede path explicitly preserved it — the two disagreed, and the
destructive default ran on the 'close the dead PR' path where the branch may
still be referenced / useful for audit. Flip the default to False (opt-in
deletion) and make the choreographer caller explicit (parity with the
orchestrator). TDD: 1 new test (default preserves branch); existing
deletion-when-requested test now passes delete_branch=True explicitly.
Dispositions verified against current code (no silent drops):
- #27 REFUTED/FIXED-UNDEPLOYED: work_session.merge_pr resolves by session_id
(no global pr_number lookup); the real cross-repo collision fix
(project_id scoping on pr_merge/close_pull_request/rebase_pr_for_task/
pr_target) is already in tree + tested (test_pr_merge_scopes_task_lookup_
by_project_id, test_close_pull_request_scopes_task_lookup_by_project_id,
test_git_pr_target_scoping). Verify-only.
- #106 REFUTED: a guard exists (rev-list --count {base_ref}..{branch} == 0)
before reset --hard + base_ref falls back to default_branch; tests lock
the safety (test_create_branch_never_repoints_branch_with_real_work,
test_create_branch_does_not_reset_or_checkout_shared_clone).
- #218 BY-DESIGN: the merge_pr idempotent guard intentionally preserves the
audit trail (docstring + test_merge_pr_idempotent_on_already_completed_
preserves_audit_trail); a COMPLETED session always carries attribution
(COMPLETED only via merge_pr), so the NULL-COMPLETED case is unreachable.
- #104 BY-DESIGN: agents never merge to the repo default branch in RoboCo's
model (root→master is CEO-only); the guard is a correct CEO-only rail,
locked by test_pr_merge_into_default_branch_is_ceo_only.
* [chore] llm: surface disabled-provider downgrade + scrub probe log (#20/#3/#211)
#20/#3 resolve_for_agent silently fell through to the legacy Anthropic path
when a configured provider was disabled — indistinguishable from 'no
assignment', so the operator got no signal that spawns bypassed the
provider. Surface the bypass with a warning (graceful degradation stays the
default — a stalled spawn is worse than a routing miss) and add an opt-in
ROBOCO_ROUTING_STRICT (default-off) that fail-closes instead. Wired into the
panel Feature Flags card. TDD: 3 unit tests (warn-on-disabled, strict-raises,
no-assignment-stays-silent).
#211 probe_ollama_tags logged str(exc) raw on the generic-exception branch —
structured log could carry connection internals / stack traces. Log the
exception class name only. TDD: existing generic-branch test strengthened to
assert the log kwargs don't leak the raw text.
* [chore] support/stream/optimal/playbook/comms hardening (Cluster S)
Logical-gaps sweep, Cluster S (TDD, red→green per item):
#64 notification_delivery.acknowledge published the NOTIFICATION_ACKED bus
event directly (bypassing the outbox) — a rollback left a phantom ACK. Route
it through defer_bus_publish (after_commit), mirroring deliver.
#76 playbook.archive()/reject() stamped the archiver into approved_by/
approved_at, overwriting approval provenance (and fabricating approval for a
rejected draft). Add archived_by/archived_at (migration 053 + table + model)
and write those on archive/reject, leaving approval attribution intact.
#181 vector_store.replace_chunks wiped existing index rows even when every
chunk lacked an embedding (embedder failure). Skip the wipe when chunks is
non-empty but records is empty — preserve good rows for nothing.
#182/#183 optimal.record_learning recomputed a learn-{md5(full_content)}
tracking source that never matched the URI the plugin embedded chunks under
(roboco://learnings/{doc_id}, doc_id=lrn-{hash100}). Use the plugin's
returned doc_id so de-index/lookup-by-source finds the chunk rows.
#96/#97 transcription periodic flush only peeked ready buffers (unbounded
map growth) and ran sync callbacks on the event loop (a slow callback
blocked the flush task). Flush (remove) each ready buffer after notifying,
and offload each callback to a thread.
#212 _TEAM_SCOPED_ROLES was duplicated across communications/agents_config/
seeds. Single-source it in foundation.policy.communications; consumers
reference that object (identity-tested).
#19 stream_bus._dispatch_event re-ran already-succeeded handlers on a
recover_pending replay (duplicate side effects). Add a per-(event.id,
handler) SET-NX idempotency guard: skip on a hit, clear the key on handler
failure so a replay re-runs it, fail-open when redis is unavailable.
Dispositions (no code change): #77 approve() index-write pair asserted
BY-DESIGN; #62/#63 notification DB-dedup verified pinned; #184/#185 REFUTED;
#214 REFUTED; #215 BY-DESIGN.
* [chore] db/migrations: graph-integrity guard + conftest unreachable-DB warning (Cluster D)
Logical-gaps sweep, Cluster D (TDD + real alembic upgrade head verification):
#16/#37 add tests/unit/test_migration_graph_integrity.py — a static guard that
the alembic migration graph has exactly one head, every down_revision resolves,
every revision is reachable from a root, and no revision id is duplicated. The
suite builds its DB via Base.metadata.create_all (not alembic upgrade head), so
a forked head / dangling down_revision / duplicate id would otherwise ship
silently and break a real deploy mid-stream.
Caught a real bug in the process: migration 053's revision id
"053_playbook_archived_attribution" (33 chars) exceeded alembic's
alembic_version.version_num VARCHAR(32) — a fresh `alembic upgrade head` raised
"value too long for type character varying(32)" at the 053 stamp. Renamed to
"053_playbook_archived_attr" (26 chars). Verified end-to-end on a scratch PG:
upgrade head stamps 053, downgrade -1 returns to 052. (The pre-existing
test_every_migration_revision_id_fits_the_alembic_version_column guard is now
green too; it had been red on the 33-char id.)
#90 conftest silently pytest.skip'd every DB test when Postgres was unreachable
— a non-Docker box reported a green run of all-skips. Extract the warning into
_warn_if_pg_unavailable and fire it at import so the operator sees the DB is
down (the per-test skip path is unchanged). Test: warns when unavailable, silent
when reachable (verified under -W error::UserWarning).
Dispositions (verified against real code + a fresh alembic upgrade head, no code
change): #6 REFUTED — sa.Enum(create_type=False) at 001:119/304 does NOT break a
fresh upgrade head (001→052 applied cleanly on a scratch DB); #8 REFUTED — the
upgrade passed 030/031 (RAG chunk tables) without pgvector installed; pgvector is
a runtime concern handled by roboco/db/base.py, not a migration prerequisite;
#40 REFUTED — the `|| echo` mask was already removed and partial-schema drift
reports exit 1 (only by-design unreachable/unmigrated skips remain); #204 REFUTED
— the property walk seed IS pinned (random.Random(20260504), line 97); #205/#206
REFUTED — the smoke-trace fixture IS wired via
test_lifecycle_smoke_replay.py (8 passed); no shell smoke scripts exist in the
tree to wire; #137 BY-DESIGN — pyproject version 0.14.0 is an operational note,
no code gate.
* [chore] panel: admin-override force flag + kanban subtask_count + ws cleanup + ui-store dedupe (Cluster P)
#13: kanban admin-override into a hatch state (completed / awaiting_qa /
awaiting_pm_review) now requires an explicit force=true from the panel and
emits a dedicated task.admin_override audit row server-side; non-hatch
overrides need no force. Backend gate in tasks route + admin_set_status;
panel kanban-board sends force for hatch targets; TaskUpdate carries force.
#198: kanban service threads the real subtask_count (one grouped query) into
dev + priority-swimlane + main-pm-flat boards instead of a hardcoded 0.
#79: useWebSocket cleanup clears messages/lastMessage/state on unmount or
endpoint change so a dep-change (navigating to another stream) can't leak the
prior subscription's stale snapshot as live.
#186: disambiguate the duplicate ui-store modules -- the session/scroll store
in lib/stores renamed to useScrollRestorationStore / scroll-restoration-store
(barrel + 2 consumers updated); the sidebar/theme useUIStore in @/store is now
the sole useUIStore.
#12: verified already in-tree (release-proposal-card surfaces non-404 errors
with retry; getProposal maps only 404->null). #80 by-design (handleTransportError
already resets isSending on a no-payload SSE drop). #81 docs (streamUrl docstring
records that live-intake SSE auth is session-id-based bearer-style).
Backend: ruff+mypy clean, 278 tests green. Panel: lint+typecheck clean, 159 tests.
* orchestrator: park/reaper/readopt/a2a hardening + self-heal/ci-watch dedupe (Cluster O)
Closes the orchestrator-side logical gaps from the sweep:
- #75 a2a human-only drop surfaced: _dispatch_a2a_work logs the skip
("a2a request targets a human-only role; left as a notification for the
human (not spawned)") instead of silently dropping the target — the
CEO/secretary/prompter still see the notification; only the spawn is
suppressed. (orchestrator.py)
- #72 readopt liveness: _readopt_running_agents requires a non-stale live
claim (via _agent_holds_live_claim) and skips a zombie container so a
reaped-but-restart-readopted agent isn't double-counted as active.
- #74 shutdown drain: stop() calls _flush_respawn_tracker so the durable
respawn counter write-throughs aren't lost on a clean stop.
- #71 resolve_wait active-guard + deferred liveness: a rate_limit_lifted
WaitingRecord is only confirmed-live after a _confirm_resume_liveness
probe (deferred deletion _resume_confirm_delay=30.0), and an
already-active agent short-circuits the repark. Scoped to
rate_limit_lifted records (the only ones at risk of a false lift).
- #73 stuck-Claude kill: _maybe_kill_stuck_claude + _claude_stuck_kill_ttl
(config.claude_stuck_kill_seconds) — a live container whose heartbeat is
stale past the grace AND whose gateway probe is broken is killed+evicted,
not protected forever by the reaper's live-skip.
- #230 verified FIXED-UNDEPLOYED: _gateway_broken_past_grace already
requires N consecutive false-broken probes (not one flaky streak); no
change, test added to pin the N-consecutive invariant.
- #43 self-heal per-observation dedupe: a fingerprint collapses repeat
CEO notifications for the same CI regression.
- #44 ci_watch dedupe by (git_url, workflow): a monorepo's multiple
workflows each get their own fix task (was collapsed by git_url alone).
- #49 identity.role_for_slug_or_none None-hardening: a stale/malformed
slug resolves to None and the human-only skip falls through to the safe
"not spawnable" path instead of crashing.
- #193 strategy engine: notify the CEO on a persistent assess failure
instead of failing silently in the background loop.
TDD: test_no_spawn_human_roles (a2a skip surfaced), test_orchestrator_
shutdown_drain (#74), test_provider_overload_break (#71), test_readopt_
running_agents (#72), test_resolve_wait_repark (#71), test_stale_claim_
reaper (#73/#230), test_strategy_engine_loop (#193, new),
test_self_heal_engine (#43), test_ci_watch_engine (#44), test_identity
(#49). All red->green.
* chore: make-quality green — xenon complexity refactors + mypy test fixes + lifecycle regen
No behavior changes. Brings the tree to a fully green `make quality` (the
base branch never passed the xenon B-rank gate on several blocks; the
lifecycle artifacts had drifted from the committed ceo_reject_to_pool edge).
Xenon B-rank refactors (extract a helper; preserve semantics exactly):
- api/routes/tasks.py: _apply_forced_status_override + _StatusOverride
dataclass bundle (update_task override block).
- services/task.py: _enforce_no_pm_code_on_create (create guards) +
_escalation_diverts_to_pool (collapses the two board/advisory +
main_pm+code divert branches into one predicate).
- services/prompter.py: _coerce_pm_code_to_planning (create_task_from_draft).
- services/notification.py: _duplicate_unacked_exists (_create_notification
purpose-based dedup query + ACK_REQUIRED_BY_TYPE gate).
- services/sequencing.py: _same_assignee_lane_edges (the undeclared-surface
same-assignee lane fallback at the tail of dev_task_collision_edges).
- gateway/choreographer/_impl.py: _pm_task_type_error static helper
(_validate_assignee_task_type compound PM guard).
- gateway/choreographer/pr_gate.py: _gate_review_event_verdict +
_gate_review_body static helpers (_post_gate_review_to_pr).
mypy test fixes (no type:ignore — banned; use typing.cast with quoted
strings per TC006):
- test_task_update_completeness: TaskUpdate(acceptance_criteria=None).
- test_bus: cast("Redis", _FakeRedis()); Redis import under TYPE_CHECKING.
- test_pr_merge_concurrency: capture AsyncMocks into locals before asserting.
- test_notification_delivery_phantom: cast("UUID", to_agents[0]).
Lifecycle artifact regen (owed from Cluster T #100 — the
awaiting_ceo_approval -> pending `ceo_reject_to_pool` edge was added to the
spec in 3d633084 without regenerating the derived artifacts the
foundation-check gate diffs against): docs/rag/lifecycle/intent-verbs.md,
docs/rag/lifecycle/status-transitions.md, panel/lib/lifecycle.json.
services/kanban.py: ruff format only (collapses the _load_subtask_counts
signature that drifted unformatted from Cluster P).
* [chore] logical-gaps sweep — Cluster I (intake/product/pitch)
#57/#58 prompter: preserve a top-level product_id with a 1-cell map
(prompter.py create_task_from_draft — top-level target wins over a
redundant 1-cell map instead of dropping product_id); reject — not
silently skip — a malformed project_id in the_work cell entries
(prompter.py _draft_cell_map raises ValidationError).
#59/#159 prompter: create_task_from_draft now operates on a copy
(_copy_draft) so _validate_and_coerce_draft / _clean_list never mutate
the caller's draft dict.
#160 prompter: _resolve_owning_team consults product/board routing
before forcing MAIN_PM on a multi-cell map (product root stays Board,
product+assignee-is-board stays Board).
#83/#84 github_provisioning: create_repo is idempotent by GitHub name
— a 422 "name already exists" (orphaned repo from a rolled-back prior
approval) is fetched and reused instead of erroring; pitch re-approval
now reuses the orphaned repo end-to-end.
#196 kanban: flat main-PM board has a "coordination" column for
non-cell teams (MAIN_PM/Board) instead of dropping their cards.
#197 project update: an explicit null in the PATCH body now clears the
stored field, distinct from an absent field (leave unchanged).
ProjectService.update drops exclude_none so explicit-None applies; the
PATCH route uses ProjectUpdate.model_validate(data.model_dump(
exclude_unset=True)) to preserve the request's unset-tracking (the old
field-by-field construction marked every field set and defeated the
distinction — nulling NOT-NULL git_url).
TDD: prompter 47, github_provisioning+pitch 16, kanban+project 67,
project routes 37 — all green; ruff + mypy clean.
* [chore] logical-gaps sweep — Cluster M (mcp-servers)
#60 flow_server/do_server: the circuit-breaker substitution no longer
erases the fixable rejection — the original envelope (kind/message/
remediate) is nested as inner on a copy of the SDK's circuit_open
envelope (the SDK dict is not mutated in place). The agent still sees
WHY the verb failed, not just that the breaker tripped.
#61 flow_server/do_server: a 404 carrying a *descriptive* detail
(not FastAPI's bare default {"detail":"Not Found"}) is now a
real resource not_found, surfaced as not_found so the agent
re-fetches state — instead of a misleading "server-side wiring gap"
invalid_state. The bare default and unparseable 404s still synthesize
the wiring-gap envelope; a 404 with a real Envelope (error field)
is still surfaced as-is.
#161 flow_server/do_server: dict error.code classification now uses
an exact-code map (authoritative for the codes the handlers emit) with
a substring fallback for unknown codes. Fixes the real regression:
AUTHENTICATION_REQUIRED carries no AUTHORIZED/DENIED/PERMISSION
substring, so the old substring-only rule dropped it to invalid_state
instead of not_authorized — an auth storm attributed as a state storm.
The fallback also adds AUTH so future AUTH-prefixed codes classify.
#162 flow_server/do_server: _register_tools gains a
ROBOCO_ALLOW_FULL_TOOLSET env override (default-off) so a missing
manifest falls back to the full tool set instead of raising — a
dev/test escape hatch. Production fail-loud behaviour is unchanged.
#163 intake_server: propose_batch accepts name as well as
title (intake drafts in the wild have used both), normalizing a
name-only draft onto a copy as title (caller's dict never mutated),
and reports the dropped count + reason in the return instead of
silently vanishing malformed drafts. The empty-batch hint now names
name as an alternative.
TDD: 123 mcp_servers tests green (14 new + 2 updated); ruff + mypy clean.
* [chore] Cluster N — conventions/docs logical-gaps sweep
#33: _create_new_doc/_update_existing_doc now resolve via
_resolve_contained_path (the RAG-returned update path was not containment-
checked — an escaping source could write/overwrite outside the docs dir).
#34: _commit_doc_to_repo returns committed/skipped/failed instead of
swallowing all exceptions; surfaced on DocRef.commit_status, the write
response, and the docs MCP guidance so a failed repo commit is fail-loud.
#35: write_doc only updates the similar doc when its filename matches — a
different filename creates a new file instead of collapsing onto the
similar doc's path (the dedup-overwrite defect codified by the old tests).
#129: a custom rule scoped to a language the validator never reports (a
typo) is surfaced as a warn finding on .roboco/conventions.yml via the
runner's once-per-run validation; #32 (tsx->typescript dialect) stays
BY-DESIGN.
#130: _cache_put only swallows a UNIQUE violation (23505) as a concurrent
duplicate; a non-unique IntegrityError (FK/NOT NULL/check) is log-errored
and re-raised instead of being silently misattributed.
#132: health re-reads the live file status (a cached degraded row hid an
in-place repair at a stale head key); get_map skips cached degraded rows
and stops caching degraded so a repaired file re-derives. #134 BY-DESIGN.
#133: _DB_METHODS gains stream/stream_scalars (SQLAlchemy 2.0 streaming
constructs are data access too — a route calling them is not thin).
#199: regenerate_verb_tables._annot_str strips Annotated[...] metadata
(BeforeValidator) before rendering; regenerated verbs.md + per-role
prompts so the BeforeValidator(func=...) repr (with a memory address) no
longer leaks into agent-facing prompt text.
TDD: 206 conventions/docs tests green (incl. 5 new files / appended
cases); ruff + mypy clean.
* [chore] Cluster 16 — cross-cutting hygiene logical-gaps sweep
Disposition + fix the 10 cross-cutting-hygiene gaps, TDD. make quality green
(ruff, mypy 944 files, pytest, xenon, vulture, foundation-check, enum-parity).
FIX:
- #24 /ws/system now gated by _require_panel_token (matches every sibling
/ws/* stream); rejects a missing token in strict mode and a forged token
even in dev. (roboco/api/websocket.py)
- #25 two drifted _require_ceo implementations (orchestrator router vs release
handler) unified on a single require_ceo_role helper in deps — same 403,
same role set, accepts Role/AgentRole/"ceo". (roboco/api/deps.py,
routes/orchestrator.py, routes/release.py)
- #11 a spawn session for a delivery role (developer/qa/documenter) with no
task_id now logs an unattributed-usage warning via is_unattributed_delivery_spawn.
(roboco/runtime/orchestrator.py)
- #65 pricing returns a structured CostResult(cost_usd, unpriced, is_anthropic)
so an unpriced Anthropic model (real spend we'd undercount) is flagged
instead of silently $0; calculate_cost stays a thin float wrapper.
(roboco/billing/pricing.py, billing/__init__.py)
- #67 blocker-metrics "blocked since" reads the task.blocked audit transition
(indexed on target_id/event_type/timestamp), not updated_at — which
over-counted when a blocked task was touched for a non-blocking reason.
Falls back to updated_at/created_at only with no audit row.
(roboco/services/metrics.py)
- #94 grok refresh_if_stale uses double-checked locking (_refresh_lock +
_recheck_or_refresh) so two concurrent callers don't both POST the
single-use refresh grant and burn the credential. (grok_auth.py)
DOCS (fix the doc, behavior already correct/pinned by tests):
- #66 get_summary docstring corrected — it sums raw agent_spawn_sessions rows
(sub-day precise); daily_usage_rollups/get_today_summary can diverge for
"today" until the sweeper catches up. (roboco/services/usage.py)
- #68 DashboardStorage is a documented in-memory stub; added a test pinning
that auditor flags are lost on storage reset (persisting = a migration +
service refactor, out of scope as a half-implementation).
(tests/integration/test_dashboard_service.py)
BY-DESIGN (no code change, with file:line evidence):
- #28 dashboard reads are open to the authenticated operator (dashboard.py:36
documents this); mutating auditor routes already gate via
_require_auditor_or_ceo. Role-gating reads would break the panel (no
X-Agent-ID on dashboard reads) and CEO-token-gating the router would block
the Auditor (auditor token != CEO token). nginx is the prod boundary.
REFUTED (narrowing would reintroduce a documented hang):
- #93 the ~/.grok directory mount (vs a single auth.json file) is load-bearing
— a single-file bind mount pins the inode so the atomic tmp.replace refresh
doesn't propagate to running containers (they hang at grok's login prompt).
Already documented in grok.py:161 and locked by
test_intake_grok_mounts_subscription_auth_when_present.
Incidental gate-greening (mypy errors a stale .mypy_cache had hidden in
earlier-cluster test files; xenon refactors for the new B-threshold):
- tests/unit/test_regenerate_verb_tables.py: type the dynamic-module loader.
- tests/unit/services/test_prompter.py: annotate the draft dict as dict[str,Any].
- tests/unit/services/test_conventions_cache_put.py: _FakeOrig is a real Exception
(IntegrityError's orig arg requires BaseException).
- metrics._blocked_since_map extracted from get_blocker_metrics (complexity).
- intake_server._normalize_batch_drafts extracted from propose_batch (complexity).
* Docs update
* [bug] spawn: self-heal vanished clone + branch ref before worktree ensure (be-dev-1 fatal loop)
A vanished clone_root (disk loss / /data/workspaces wipe / manual cleanup)
fatal-looped the resume path: _ensure_worktree_before_spawn ran
`git -C <missing>` and released the claim, but the reaper-style release
preserves assigned_to + branch_name so the next dispatch is a RESUME
(create_branch never re-runs to re-clone) and the same missing clone failed
every ~30s.
- workspace.py: ensure_worktree_self_heal re-attaches a present worktree +
symlinks the shared .venv; on a missing local branch ref it fetches from
origin (create_branch pushes at claim time, so pushed work survives) and
re-creates the ref, falling back to -b origin/HEAD only when the branch
was never pushed. _fetch_branch_ref is the token-aware fetch helper.
- orchestrator.py: _ensure_worktree_before_spawn health-checks the clone
and re-clones via ensure_workspace BEFORE the worktree self-heal. Fatal
git-state (WorkspaceError) still releases the claim + aborts; transient
failures abort without releasing (a fresh claim wouldn't help and
re-cloning is destructive).
TDD: 21 new + 61 related worktree/git/cancel/cleanup tests green; ruff +
mypy clean.
---------
Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -123,3 +123,24 @@ async def test_replace_chunks_skips_chunks_without_embeddings() -> None:
|
||||
assert conn.execute.await_count == 1 # DELETE ran
|
||||
_sql, records = conn.executemany.await_args.args
|
||||
assert len(records) == 1 # only the embedded chunk inserted
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_replace_chunks_does_not_wipe_on_all_none_embeddings() -> None:
|
||||
"""#181: chunks were passed (an embedder ran) but NONE produced a usable
|
||||
embedding — that is an embedder failure, not a deliberate clear. Wiping the
|
||||
source's existing rows on a failed embed would lose good index rows for
|
||||
nothing. The replace must no-op (no DELETE) in that case, distinct from the
|
||||
deliberate empty-list clear (which DOES delete)."""
|
||||
store, conn, _pool = _make_store_with_conn()
|
||||
source = "roboco://learnings/lrn-deadbeef"
|
||||
|
||||
chunks = [
|
||||
Chunk(text="failed-embed-1", source=source, metadata={}), # no embedding
|
||||
Chunk(text="failed-embed-2", source=source, metadata={}), # no embedding
|
||||
]
|
||||
await store.replace_chunks(source, chunks)
|
||||
|
||||
# No-op: no connection acquired, no DELETE, no INSERT — existing rows kept.
|
||||
assert conn.execute.await_count == 0
|
||||
assert conn.executemany.await_count == 0
|
||||
|
||||
@@ -9,10 +9,13 @@ crashes. The fix runs the insert in a savepoint and rolls back ONLY the
|
||||
savepoint on conflict, leaving the outer transaction usable (the winner's
|
||||
row satisfies the next ``_cache_get``).
|
||||
|
||||
These tests exercise the contract with a fake session: a duplicate must not
|
||||
raise out of ``_cache_put``, must not call a full ``session.rollback()``
|
||||
(which would undo the outer task-create transaction), and must not poison
|
||||
the session — while the happy path still adds + commits the savepoint.
|
||||
These tests exercise the contract with a fake session: a UNIQUE violation
|
||||
(23505) must not raise out of ``_cache_put``, must not call a full
|
||||
``session.rollback()`` (which would undo the outer task-create transaction),
|
||||
and must not poison the session — while the happy path still adds + commits
|
||||
the savepoint. A NON-unique IntegrityError (FK / NOT NULL / check) is a real
|
||||
bug, not a benign concurrent duplicate, so it must re-raise + log-error
|
||||
instead of being silently misattributed as "concurrent put" (#130).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -26,13 +29,22 @@ from roboco.services.conventions import ConventionsService
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
|
||||
|
||||
class _FakeOrig(Exception):
|
||||
"""A stand-in DBAPI exception carrying a SQLSTATE code."""
|
||||
|
||||
def __init__(self, sqlstate: str) -> None:
|
||||
super().__init__(sqlstate)
|
||||
self.sqlstate = sqlstate
|
||||
|
||||
|
||||
class _FakeNested:
|
||||
"""A stand-in for the savepoint returned by ``session.begin_nested()``.
|
||||
|
||||
On release (``__aexit__``) it flushes the queued insert: a concurrent
|
||||
duplicate raises IntegrityError and the SAVEPOINT is rolled back — the
|
||||
outer session is NOT poisoned (only the savepoint failed). The happy path
|
||||
releases the savepoint cleanly.
|
||||
releases the savepoint cleanly. ``sqlstate`` selects unique (23505) vs a
|
||||
non-unique integrity error (e.g. 23503 FK violation).
|
||||
"""
|
||||
|
||||
def __init__(self, session: _FakeSession) -> None:
|
||||
@@ -42,17 +54,20 @@ class _FakeNested:
|
||||
self._session.savepoint_started += 1
|
||||
|
||||
async def __aexit__(self, exc_type: Any, exc: Any, _tb: Any) -> None:
|
||||
if self._session._duplicate:
|
||||
if self._session._sqlstate is not None:
|
||||
self._session.savepoint_rolled_back = True
|
||||
raise IntegrityError(
|
||||
"INSERT ... conventions_cache", {}, Exception("unique")
|
||||
"INSERT ... conventions_cache",
|
||||
{},
|
||||
_FakeOrig(self._session._sqlstate),
|
||||
)
|
||||
self._session.savepoint_committed = True
|
||||
|
||||
|
||||
class _FakeSession:
|
||||
def __init__(self, *, duplicate: bool) -> None:
|
||||
self._duplicate = duplicate
|
||||
def __init__(self, *, sqlstate: str | None = None) -> None:
|
||||
# None = happy path; "23505" = unique duplicate; "23503" = FK violation.
|
||||
self._sqlstate = sqlstate
|
||||
self.added: list[Any] = []
|
||||
self.poisoned = False
|
||||
self.full_rollback_called = False
|
||||
@@ -65,10 +80,10 @@ class _FakeSession:
|
||||
|
||||
async def flush(self) -> None:
|
||||
# Old-code path: a bare flush on a duplicate poisons the shared session.
|
||||
if self._duplicate:
|
||||
if self._sqlstate is not None:
|
||||
self.poisoned = True
|
||||
raise IntegrityError(
|
||||
"INSERT ... conventions_cache", {}, Exception("unique")
|
||||
"INSERT ... conventions_cache", {}, _FakeOrig(self._sqlstate)
|
||||
)
|
||||
|
||||
def begin_nested(self) -> _FakeNested:
|
||||
@@ -85,10 +100,10 @@ def _mapping() -> ConventionsStandard:
|
||||
@pytest.mark.asyncio
|
||||
async def test_cache_put_tolerates_concurrent_duplicate_without_poisoning() -> None:
|
||||
# The loser of a concurrent cache-populate race must not crash the shared
|
||||
# task-create session: the duplicate IntegrityError is contained to a
|
||||
# task-create session: the UNIQUE 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)
|
||||
session = _FakeSession(sqlstate="23505")
|
||||
svc = ConventionsService(session=cast("Any", session))
|
||||
|
||||
await svc._cache_put(uuid4(), "deadbeef", _mapping(), "ok")
|
||||
@@ -100,7 +115,7 @@ async def test_cache_put_tolerates_concurrent_duplicate_without_poisoning() -> N
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cache_put_happy_path_adds_and_releases_savepoint() -> None:
|
||||
session = _FakeSession(duplicate=False)
|
||||
session = _FakeSession(sqlstate=None)
|
||||
svc = ConventionsService(session=cast("Any", session))
|
||||
|
||||
await svc._cache_put(uuid4(), "deadbeef", _mapping(), "ok")
|
||||
@@ -109,3 +124,20 @@ async def test_cache_put_happy_path_adds_and_releases_savepoint() -> None:
|
||||
assert session.savepoint_committed is True
|
||||
assert not session.poisoned
|
||||
assert not session.full_rollback_called
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cache_put_reraises_non_unique_integrity_error() -> None:
|
||||
# A non-unique integrity error (FK / NOT NULL / check) is a real bug, not a
|
||||
# benign concurrent duplicate — it must surface, not be silently swallowed
|
||||
# as "concurrent put" (#130).
|
||||
session = _FakeSession(sqlstate="23503")
|
||||
svc = ConventionsService(session=cast("Any", session))
|
||||
|
||||
with pytest.raises(IntegrityError):
|
||||
await svc._cache_put(uuid4(), "deadbeef", _mapping(), "ok")
|
||||
|
||||
# The savepoint was rolled back (only the failed insert), never a full
|
||||
# session rollback — the outer task-create transaction stays usable.
|
||||
assert session.savepoint_rolled_back is True
|
||||
assert not session.full_rollback_called
|
||||
|
||||
@@ -490,6 +490,43 @@ async def test_apply_escalation_refuses_cancelled_task() -> None:
|
||||
flush.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_escalation_refuses_backlog_task() -> None:
|
||||
"""#99: escalation sets BLOCKED directly, bypassing the transition
|
||||
validator. A BACKLOG task was never activated — escalating it to BLOCKED is
|
||||
a nonsense transition with no spec edge. The primitive guards itself
|
||||
(HTTP route bypasses the spec gate) and returns False, leaving the task
|
||||
untouched. Active work statuses still escalate (next test)."""
|
||||
svc = _service()
|
||||
original_assignee = uuid4()
|
||||
task = MagicMock(
|
||||
id=uuid4(),
|
||||
parent_task_id=uuid4(),
|
||||
task_type=TaskType.CODE,
|
||||
assigned_to=original_assignee,
|
||||
blocker_raised_by=None,
|
||||
status=TaskStatus.BACKLOG,
|
||||
)
|
||||
flush = AsyncMock()
|
||||
object.__setattr__(svc.session, "flush", flush)
|
||||
_bind(svc, "_is_board_advisory_agent", AsyncMock(return_value=False))
|
||||
_bind(svc, "_is_main_pm_agent", AsyncMock(return_value=False))
|
||||
_bind(svc, "_emit_status_transition_audit", MagicMock())
|
||||
|
||||
applied = await svc.apply_escalation(
|
||||
task=task,
|
||||
target_agent_id=uuid4(),
|
||||
escalator_slug="be-pm",
|
||||
target_slug="main-pm",
|
||||
reason="please review",
|
||||
)
|
||||
|
||||
assert applied is False
|
||||
assert task.status == TaskStatus.BACKLOG # untouched
|
||||
assert task.assigned_to == original_assignee
|
||||
flush.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_escalation_blocks_non_terminal_task() -> None:
|
||||
# the terminal guard must not over-restrict: a normal in_progress task
|
||||
|
||||
@@ -186,7 +186,10 @@ async def test_close_pull_request_patches_state_closed(
|
||||
|
||||
with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
|
||||
await svc.close_pull_request(
|
||||
159, project_id=uuid4(), comment="superseded by #158"
|
||||
159,
|
||||
project_id=uuid4(),
|
||||
comment="superseded by #158",
|
||||
delete_branch=True,
|
||||
)
|
||||
|
||||
assert (
|
||||
@@ -197,6 +200,75 @@ async def test_close_pull_request_patches_state_closed(
|
||||
delete_branch.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_pull_request_does_not_delete_branch_by_default(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#109: close_pull_request defaulted delete_branch=True, so the
|
||||
choreographer supersede path deleted a superseded PR's branch while the
|
||||
orchestrator supersede path explicitly preserved it — the two disagreed,
|
||||
and the destructive default ran on the "close the dead PR" path where the
|
||||
branch may still be referenced / useful for audit. The default is now
|
||||
False (opt-in deletion); a superseded-PR close preserves the branch unless
|
||||
the caller explicitly asks to delete. The PATCH still fires.
|
||||
"""
|
||||
svc = _git_service()
|
||||
task = type("T", (), {"id": "t", "assigned_to": None, "created_by": None})()
|
||||
session = AsyncMock()
|
||||
session.execute = AsyncMock(
|
||||
return_value=type("Res", (), {"scalar_one_or_none": lambda _self: task})()
|
||||
)
|
||||
delete_branch = AsyncMock()
|
||||
monkeypatch.setattr(svc, "session", session, raising=False)
|
||||
monkeypatch.setattr(
|
||||
svc,
|
||||
"_project_for_task",
|
||||
AsyncMock(return_value=type("P", (), {"slug": "proj"})()),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_resolve_workspace_agent_id", MagicMock(return_value=None)
|
||||
)
|
||||
monkeypatch.setattr(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws")))
|
||||
monkeypatch.setattr(
|
||||
svc, "_get_project_token_or_raise", AsyncMock(return_value="tok")
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_parse_github_remote", MagicMock(return_value=("owner", "repo"))
|
||||
)
|
||||
monkeypatch.setattr(svc, "_delete_pr_branch_best_effort", delete_branch)
|
||||
|
||||
class _Resp:
|
||||
is_success = True
|
||||
status_code = 200
|
||||
text = ""
|
||||
|
||||
def json(self) -> dict[str, str]:
|
||||
return {"state": "open"}
|
||||
|
||||
class _Client:
|
||||
async def __aenter__(self) -> _Client:
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *_a: Any) -> None:
|
||||
return None
|
||||
|
||||
async def get(self, _url: str, **_kw: Any) -> _Resp:
|
||||
return _Resp()
|
||||
|
||||
async def post(self, _url: str, **_kw: Any) -> _Resp:
|
||||
return _Resp()
|
||||
|
||||
async def patch(self, _url: str, **_kw: Any) -> _Resp:
|
||||
return _Resp()
|
||||
|
||||
with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
|
||||
# No delete_branch kwarg → default must preserve the branch.
|
||||
await svc.close_pull_request(159, project_id=uuid4(), comment="superseded")
|
||||
|
||||
# The close PATCH still fires (the PR is closed); the branch is NOT deleted.
|
||||
delete_branch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_pull_request_idempotent_when_already_closed(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
|
||||
@@ -89,3 +89,54 @@ async def test_network_error_raises(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = GitHubProvisioningService(token="tok", org="acme", client=_client(handler))
|
||||
with pytest.raises(ProvisioningError):
|
||||
await svc.create_repo("x")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_repo_reuses_already_existing_repo(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#83/#84: a 422 'name already exists' (a repo orphaned by a rolled-back
|
||||
prior approval) is fetched and returned, not errored — so re-approval reuses
|
||||
the GitHub repo instead of colliding."""
|
||||
monkeypatch.setattr(settings, "provisioning_enabled", True)
|
||||
calls: list[str] = []
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
calls.append(request.url.path)
|
||||
if request.url.path == "/orgs/acme/repos":
|
||||
return httpx.Response(
|
||||
422, text='{"message": "name already exists on this account"}'
|
||||
)
|
||||
if request.url.path == "/repos/acme/orphan":
|
||||
return httpx.Response(
|
||||
200,
|
||||
json={
|
||||
"full_name": "acme/orphan",
|
||||
"clone_url": "https://github.com/acme/orphan.git",
|
||||
"html_url": "https://github.com/acme/orphan",
|
||||
},
|
||||
)
|
||||
return httpx.Response(404)
|
||||
|
||||
svc = GitHubProvisioningService(token="tok", org="acme", client=_client(handler))
|
||||
repo = await svc.create_repo("orphan", "desc")
|
||||
assert repo.full_name == "acme/orphan"
|
||||
assert repo.clone_url.endswith("orphan.git")
|
||||
# POST create -> 422, then GET the existing repo.
|
||||
assert calls == ["/orgs/acme/repos", "/repos/acme/orphan"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_repo_other_422_still_raises(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""A 422 that is NOT the 'already exists' shape (e.g. a validation error) is
|
||||
still a hard failure — only the orphaned-repo case is treated idempotently."""
|
||||
monkeypatch.setattr(settings, "provisioning_enabled", True)
|
||||
svc = GitHubProvisioningService(
|
||||
token="tok",
|
||||
org="acme",
|
||||
client=_client(lambda _r: httpx.Response(422, text="name reserved")),
|
||||
)
|
||||
with pytest.raises(ProvisioningError):
|
||||
await svc.create_repo("dup")
|
||||
|
||||
@@ -37,6 +37,7 @@ from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.foundation.identity import AGENTS
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
Complexity,
|
||||
@@ -140,6 +141,79 @@ async def test_create_allows_main_pm_plus_planning() -> None:
|
||||
assert task.task_type == TaskType.PLANNING
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_rejects_cell_pm_assignee_plus_code() -> None:
|
||||
# The team-based backstop above only catches main_pm+code. A code task on a
|
||||
# cell team (backend) assigned TO the cell PM (be-pm) is the same mismatch
|
||||
# — the role-based guard closes it at create time.
|
||||
|
||||
be_pm_uuid = AGENTS["be-pm"].uuid
|
||||
svc = TaskService(
|
||||
MagicMock(add=MagicMock(), flush=AsyncMock(), execute=AsyncMock())
|
||||
)
|
||||
req = TaskCreateRequest(
|
||||
title="rogue cell-pm code task",
|
||||
description="should not persist",
|
||||
acceptance_criteria=["ship it"],
|
||||
team=Team.BACKEND,
|
||||
created_by=uuid4(),
|
||||
assigned_to=be_pm_uuid,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
project_id=uuid4(),
|
||||
)
|
||||
with pytest.raises(ValidationError, match="PM_NO_CODE"):
|
||||
await svc.create(req)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_allows_cell_pm_assignee_plus_planning() -> None:
|
||||
|
||||
be_pm_uuid = AGENTS["be-pm"].uuid
|
||||
svc = TaskService(
|
||||
MagicMock(add=MagicMock(), flush=AsyncMock(), execute=AsyncMock())
|
||||
)
|
||||
req = TaskCreateRequest(
|
||||
title="cell-pm planning task",
|
||||
description="fine",
|
||||
acceptance_criteria=["decompose the slice"],
|
||||
team=Team.BACKEND,
|
||||
created_by=uuid4(),
|
||||
assigned_to=be_pm_uuid,
|
||||
task_type=TaskType.PLANNING,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
project_id=uuid4(),
|
||||
)
|
||||
task = await svc.create(req)
|
||||
assert task.task_type == TaskType.PLANNING
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_allows_dev_assignee_plus_code() -> None:
|
||||
# The role-based guard is PM-scoped — a developer assignee + code is fine.
|
||||
|
||||
dev_uuid = AGENTS["be-dev-1"].uuid
|
||||
svc = TaskService(
|
||||
MagicMock(add=MagicMock(), flush=AsyncMock(), execute=AsyncMock())
|
||||
)
|
||||
req = TaskCreateRequest(
|
||||
title="dev code task",
|
||||
description="fine",
|
||||
acceptance_criteria=["implement it"],
|
||||
team=Team.BACKEND,
|
||||
created_by=uuid4(),
|
||||
assigned_to=dev_uuid,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
project_id=uuid4(),
|
||||
)
|
||||
task = await svc.create(req)
|
||||
assert task.task_type == TaskType.CODE
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Site 2 — approve_and_start retype (board→main-pm handoff)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -14,7 +14,7 @@ from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from typing import cast
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
@@ -25,6 +25,11 @@ from roboco.models.optimal import (
|
||||
IndexType,
|
||||
)
|
||||
from roboco.services.optimal import OptimalService
|
||||
from roboco.services.optimal_brain.indexes.base import IngestResult
|
||||
from roboco.services.optimal_brain.indexes.learnings import (
|
||||
LearningsIndexPlugin,
|
||||
RecordLearningParams,
|
||||
)
|
||||
|
||||
|
||||
class _StubOptimalService(OptimalService):
|
||||
@@ -150,3 +155,57 @@ async def test_record_review_raises_when_file_path_empty() -> None:
|
||||
|
||||
with pytest.raises(ValueError, match="file_path is required"):
|
||||
await svc.record_review(params)
|
||||
|
||||
|
||||
# --- #182/#183: record_learning tracking-row URI must match the chunk URI ---
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_record_learning_tracking_source_matches_chunk_uri() -> None:
|
||||
"""#182/#183: the indexed_documents tracking row's ``source`` must be the
|
||||
SAME URI the learnings plugin embedded the chunks under, so a later
|
||||
de-index/lookup-by-source against the tracking row finds the chunk rows.
|
||||
The plugin returns ``doc_id`` (``lrn-{hash100}``); the tracking source must
|
||||
be ``roboco://learnings/{doc_id}`` — NOT a locally-recomputed
|
||||
``learn-{md5(full_content)}`` that never matches the chunk rows."""
|
||||
captured: dict[str, str] = {}
|
||||
|
||||
class _CapturingOptimalService(_StubOptimalService):
|
||||
async def _track_indexed_document(
|
||||
self,
|
||||
index_type: IndexType,
|
||||
source: str,
|
||||
title: str | None = None,
|
||||
preview: str | None = None,
|
||||
metadata: dict | None = None,
|
||||
) -> None:
|
||||
del index_type, title, preview, metadata
|
||||
captured["source"] = source
|
||||
|
||||
svc = _CapturingOptimalService()
|
||||
# Real plugin instance so the isinstance(plugin, LearningsIndexPlugin) branch
|
||||
# fires; mock only the embedding coroutine to return a known doc_id.
|
||||
plugin = LearningsIndexPlugin()
|
||||
ingest = IngestResult(doc_id="lrn-deadbeefdead", chunk_count=2, success=True)
|
||||
with patch.object(plugin, "record_learning", AsyncMock(return_value=ingest)):
|
||||
svc._plugins = {IndexType.LEARNINGS: plugin}
|
||||
svc._initialized = True
|
||||
|
||||
# Content longer than 100 chars — the old code hashed the FULL content
|
||||
# while the plugin hashes only the first 100, so a mismatched recompute
|
||||
# diverges even on the hash input, not just the prefix.
|
||||
long_content = "x" * 250
|
||||
params = RecordLearningParams(
|
||||
content=long_content,
|
||||
category="error_handling",
|
||||
agent_id=uuid4(),
|
||||
shareable=True,
|
||||
)
|
||||
doc_id = await svc.record_learning(params)
|
||||
|
||||
assert doc_id == "lrn-deadbeefdead"
|
||||
# The tracking source matches the chunk URI the plugin used (prefix lrn-,
|
||||
# the plugin's doc_id) — no locally-recomputed learn-/{full-hash} divergence.
|
||||
assert captured["source"] == f"roboco://learnings/{doc_id}"
|
||||
assert captured["source"].startswith("roboco://learnings/lrn-")
|
||||
assert "learn-" not in captured["source"]
|
||||
|
||||
@@ -21,7 +21,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.exceptions import GitError
|
||||
from roboco.exceptions import GitError, MergeConflictError
|
||||
from roboco.services.git import GitService
|
||||
|
||||
# Module-level constants kept local so the assertions stay readable and
|
||||
@@ -331,3 +331,80 @@ async def test_pr_merge_scopes_task_lookup_by_project_id() -> None:
|
||||
# The scoped project_id value is bound into the WHERE (Postgres renders
|
||||
# UUID literals without hyphens), not just the column name.
|
||||
assert project_id.hex in sql
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #108: _merge_with_retry 405 method-not-allowed fallback.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_HTTP_METHOD_NOT_ALLOWED = 405
|
||||
|
||||
|
||||
def _merge_ctx(pr_number: int = 11) -> GitService._MergeContext:
|
||||
return GitService._MergeContext(
|
||||
owner="acme",
|
||||
repo="repo",
|
||||
pr_number=pr_number,
|
||||
git_token="tok",
|
||||
workspace=Path("/tmp/ws"),
|
||||
target="feature/x",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_merge_with_retry_falls_back_on_405_method_not_allowed() -> None:
|
||||
"""#108: _merge_with_retry hardcoded 'squash' and raised MergeConflictError
|
||||
on a 405 (repo disallows squash) with no method fallback — wedging the PM
|
||||
on an open, mergeable PR whose repo merely has the squash button off. The
|
||||
CEO merge_pull_request path already falls back to a permitted method; the
|
||||
agent path must too. A 405 on squash → retry once with a permitted method.
|
||||
"""
|
||||
svc = GitService(_make_session(MagicMock(), MagicMock))
|
||||
_bind(svc, "log", MagicMock())
|
||||
call_seq = AsyncMock(
|
||||
side_effect=[_fake_response(_HTTP_METHOD_NOT_ALLOWED), _fake_response(200)]
|
||||
)
|
||||
_bind(svc, "_call_merge_api", call_seq)
|
||||
first_method = AsyncMock(return_value="merge")
|
||||
_bind(svc, "_first_allowed_merge_method", first_method)
|
||||
pr_is_merged = AsyncMock(return_value=False)
|
||||
_bind(svc, "_pr_is_merged", pr_is_merged)
|
||||
_bind(svc, "_sync_target_branch", AsyncMock())
|
||||
|
||||
resp = await svc._merge_with_retry(_merge_ctx())
|
||||
|
||||
assert resp.is_success
|
||||
# Two merge attempts: squash (405) then the permitted fallback (200).
|
||||
assert call_seq.await_count == _EXPECTED_MERGE_ATTEMPTS
|
||||
# The fallback method was looked up (excluding the refused squash).
|
||||
first_method.assert_awaited_once()
|
||||
first_call = first_method.await_args
|
||||
assert first_call is not None
|
||||
assert first_call.kwargs["exclude"] == "squash"
|
||||
# An already-merged disambiguation must NOT be consulted — the 405 was
|
||||
# method-not-allowed, resolved by retry, not an already-merged PR.
|
||||
pr_is_merged.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_merge_with_retry_raises_when_no_permitted_fallback_on_405() -> None:
|
||||
"""A 405 with no permitted fallback method (every merge button off) is a
|
||||
real refusal — raise MergeConflictError, don't loop or mask it. The
|
||||
already-merged disambiguation still runs first so a 405-on-already-merged
|
||||
PR stays idempotent success."""
|
||||
svc = GitService(_make_session(MagicMock(), MagicMock))
|
||||
_bind(svc, "log", MagicMock())
|
||||
call_seq = AsyncMock(side_effect=[_fake_response(_HTTP_METHOD_NOT_ALLOWED)])
|
||||
_bind(svc, "_call_merge_api", call_seq)
|
||||
_bind(svc, "_first_allowed_merge_method", AsyncMock(return_value=None))
|
||||
pr_is_merged = AsyncMock(return_value=False)
|
||||
_bind(svc, "_pr_is_merged", pr_is_merged)
|
||||
_bind(svc, "_sync_target_branch", AsyncMock())
|
||||
|
||||
with pytest.raises(MergeConflictError):
|
||||
await svc._merge_with_retry(_merge_ctx())
|
||||
|
||||
# Only the initial squash attempt — no fallback retry when none permitted.
|
||||
assert call_seq.await_count == 1
|
||||
# Already-merged disambiguation ran (the 405 could also mean already-merged).
|
||||
pr_is_merged.assert_awaited_once()
|
||||
|
||||
@@ -32,6 +32,7 @@ from roboco.services.base import ServiceError, ValidationError
|
||||
from roboco.services.prompter import (
|
||||
PrompterService,
|
||||
_cell_teams,
|
||||
_clean_list,
|
||||
_draft_cell_map,
|
||||
compose_description,
|
||||
derive_scale,
|
||||
@@ -434,6 +435,10 @@ async def test_confirm_live_draft_main_pm_route_assigns_main_pm(
|
||||
row = await db_session.get(TaskTable, task_id)
|
||||
assert row.status == TaskStatus.PENDING
|
||||
assert row.assigned_to == UUID(AGENT_UUIDS["main-pm"])
|
||||
# A PM coordinates — a code task handed to the Main PM is coerced to
|
||||
# planning (the PM/code invariant; the draft's team=backend is honored but
|
||||
# the type is retyped so the combo never persists).
|
||||
assert row.task_type == TaskType.PLANNING
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -769,20 +774,31 @@ def test_draft_cell_map_empty_when_no_entry_has_project_id() -> None:
|
||||
assert _draft_cell_map({"the_work": [{"team": "backend"}]}) == []
|
||||
|
||||
|
||||
def test_draft_cell_map_ignores_off_enum_teams_and_bad_uuids() -> None:
|
||||
"""Off-enum team names and malformed project_ids are skipped, not crashed on
|
||||
(the intake agent is an LLM)."""
|
||||
def test_draft_cell_map_skips_off_enum_teams_but_rejects_bad_uuids() -> None:
|
||||
"""Off-enum team names are skipped (the intake agent is an LLM and can emit
|
||||
a non-cell team), and an entry with no project_id is skipped (legacy
|
||||
single-cell). But a present-but-malformed project_id is a hard error —
|
||||
silently dropping it would collapse a 2-cell map to 1-cell and mis-route the
|
||||
draft as a single-project task (#58)."""
|
||||
good = uuid4()
|
||||
draft = {
|
||||
"the_work": [
|
||||
_work("backend", good),
|
||||
{"team": "marketing", "project_id": str(uuid4())}, # not a cell
|
||||
_work("frontend", None), # missing
|
||||
{"team": "ux_ui", "project_id": "not-a-uuid"}, # bad uuid
|
||||
_work("frontend", None), # missing project_id — skipped
|
||||
]
|
||||
}
|
||||
assert _draft_cell_map(draft) == [(Team.BACKEND, good)]
|
||||
|
||||
bad = {
|
||||
"the_work": [
|
||||
_work("backend", good),
|
||||
{"team": "ux_ui", "project_id": "not-a-uuid"}, # malformed — reject
|
||||
]
|
||||
}
|
||||
with pytest.raises(ValidationError, match="Invalid project_id"):
|
||||
_draft_cell_map(bad)
|
||||
|
||||
|
||||
def test_validate_batch_scope_accepts_single_multi_cell_draft() -> None:
|
||||
"""One 2-cell draft already spans ≥2 distinct projects → valid MegaTask."""
|
||||
@@ -899,3 +915,107 @@ async def test_resolve_owning_team_single_cell_still_routes_to_lead_cell() -> No
|
||||
default_lead=Team.BACKEND,
|
||||
)
|
||||
assert team is Team.BACKEND
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_owning_team_product_with_cell_map_stays_board(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#160: a product draft that also carries a ≥2-cell the_work map is still a
|
||||
product root — on the board-review path it stays team=board, not forced to
|
||||
Main PM (which would strand it past the CEO Approve & Start gate)."""
|
||||
service = get_prompter_service()
|
||||
be_proj, fe_proj = uuid4(), uuid4()
|
||||
draft = {"the_work": [_work("backend", be_proj), _work("frontend", fe_proj)]}
|
||||
product_id = uuid4()
|
||||
|
||||
async def _is_board(_agent_id: UUID) -> bool:
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(service, "_assignee_is_board", _is_board)
|
||||
team = await service._resolve_owning_team(
|
||||
draft,
|
||||
resolved_product_id=product_id,
|
||||
resolved_assigned_to=uuid4(),
|
||||
team_override=None,
|
||||
default_lead=Team.BACKEND,
|
||||
)
|
||||
assert team is Team.BOARD
|
||||
|
||||
async def _not_board(_agent_id: UUID) -> bool:
|
||||
return False
|
||||
|
||||
monkeypatch.setattr(service, "_assignee_is_board", _not_board)
|
||||
team = await service._resolve_owning_team(
|
||||
draft,
|
||||
resolved_product_id=product_id,
|
||||
resolved_assigned_to=uuid4(),
|
||||
team_override=None,
|
||||
default_lead=Team.BACKEND,
|
||||
)
|
||||
assert team is Team.MAIN_PM
|
||||
|
||||
|
||||
def test_clean_list_extracts_dict_wrapped_items() -> None:
|
||||
"""#159: _clean_list (via coerce_str_list) extracts text from the Claude
|
||||
SDK's XML-ish dict wrappers (``<item>…</item>`` -> ``{"item": {"$text": …}}``)
|
||||
instead of rendering ``str(dict)``. Pins the behavior so a regression to
|
||||
``str(dict)`` in the rendered description is caught."""
|
||||
out = _clean_list([{"item": {"$text": "build it"}}, "ship it", " ", ""])
|
||||
assert out == ["build it", "ship it"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_task_from_draft_preserves_product_with_one_cell_map(
|
||||
db_session: Any,
|
||||
) -> None:
|
||||
"""#57: a draft carrying a top-level product_id AND a 1-cell the_work map
|
||||
keeps the product — the lone cell map is redundant, not a signal to drop the
|
||||
product and force the cell's project_id."""
|
||||
_project_id, ceo_id = await _seed_project_and_ceo(db_session)
|
||||
product_id = uuid4()
|
||||
db_session.add(
|
||||
ProductTable(
|
||||
id=product_id,
|
||||
name="One-cell product",
|
||||
slug=f"prod-{uuid4().hex[:8]}",
|
||||
description="x",
|
||||
created_by=ceo_id,
|
||||
)
|
||||
)
|
||||
await db_session.flush()
|
||||
service = get_prompter_service(db=db_session)
|
||||
draft = {
|
||||
"title": "Board-led single-cell product",
|
||||
"acceptance_criteria": ["done"],
|
||||
"product_id": str(product_id),
|
||||
"the_work": [_work("backend", uuid4())],
|
||||
}
|
||||
task = await service.create_task_from_draft(draft, ceo_id)
|
||||
assert task.product_id == product_id
|
||||
assert task.project_id is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_task_from_draft_does_not_mutate_caller_draft(
|
||||
db_session: Any,
|
||||
) -> None:
|
||||
"""#59: create_task_from_draft coerces + recomposes on a copy — the caller's
|
||||
draft dict and its the_work unit dicts are left untouched (no in-place
|
||||
rewrite of acceptance_criteria / items)."""
|
||||
project_id, ceo_id = await _seed_project_and_ceo(db_session)
|
||||
service = get_prompter_service(db=db_session)
|
||||
original_items = [" trim me ", "keep"]
|
||||
draft: dict[str, Any] = {
|
||||
"title": "No-mutation check",
|
||||
"acceptance_criteria": ["done"],
|
||||
"project_id": str(project_id),
|
||||
"the_work": [
|
||||
{"team": "backend", "summary": "s", "items": list(original_items)}
|
||||
],
|
||||
}
|
||||
await service.create_task_from_draft(draft, ceo_id)
|
||||
# The caller's the_work unit items were NOT coerced in place...
|
||||
assert draft["the_work"][0]["items"] == original_items
|
||||
# ...and the top-level acceptance_criteria was NOT replaced.
|
||||
assert draft["acceptance_criteria"] == ["done"]
|
||||
|
||||
@@ -47,9 +47,10 @@ def _make_redis_mock(initial_store: dict[str, Any] | None = None) -> AsyncMock:
|
||||
return 1 if store.pop(key, None) is not None else 0
|
||||
|
||||
async def _eval(script: str, _numkeys: int, *keys_and_args: Any) -> Any:
|
||||
# Mirror the tracker's two atomic Lua scripts (see rate_limit_tracker.py)
|
||||
# so the counter update is observable in-process. Single-threaded tests
|
||||
# get the same result production gets from Redis' single-threaded Lua.
|
||||
# Mirror the tracker's atomic Lua scripts (see rate_limit_tracker.py)
|
||||
# so the counter update + activate merge are observable in-process.
|
||||
# Single-threaded tests get the same result production gets from Redis'
|
||||
# single-threaded Lua.
|
||||
key = keys_and_args[0]
|
||||
raw = store.get(key)
|
||||
text = raw.decode() if isinstance(raw, bytes) else (str(raw) if raw else None)
|
||||
@@ -69,6 +70,16 @@ def _make_redis_mock(initial_store: dict[str, Any] | None = None) -> AsyncMock:
|
||||
state["probe_failures"] = 0
|
||||
store[key] = json.dumps(state)
|
||||
return None
|
||||
if "roboco:activate_rate_limit" in script:
|
||||
# Mirror the production merge: carry over the previous probe_failures
|
||||
# (if any) into the fresh episode blob from ARGV[1].
|
||||
fresh = json.loads(keys_and_args[1])
|
||||
if text is not None:
|
||||
prev = json.loads(text)
|
||||
if "probe_failures" in prev and prev["probe_failures"] is not None:
|
||||
fresh["probe_failures"] = prev["probe_failures"]
|
||||
store[key] = json.dumps(fresh)
|
||||
return fresh.get("probe_failures", 0)
|
||||
raise AssertionError(f"unknown eval script: {script[:80]}")
|
||||
|
||||
mock = AsyncMock()
|
||||
|
||||
@@ -75,6 +75,17 @@ def _make_redis_mock(initial_store: dict[str, Any] | None = None) -> AsyncMock:
|
||||
state["probe_failures"] = 0
|
||||
store[key] = json.dumps(state)
|
||||
return None
|
||||
if "roboco:activate_rate_limit" in script:
|
||||
# Mirror the production merge: decode the fresh episode blob from
|
||||
# ARGV[1], carry over the previous probe_failures if a blob exists,
|
||||
# else keep the fresh probe_failures (0). Atomic vs increment/reset.
|
||||
fresh = json.loads(keys_and_args[1])
|
||||
if text is not None:
|
||||
prev = json.loads(text)
|
||||
if "probe_failures" in prev and prev["probe_failures"] is not None:
|
||||
fresh["probe_failures"] = prev["probe_failures"]
|
||||
store[key] = json.dumps(fresh)
|
||||
return fresh.get("probe_failures", 0)
|
||||
raise AssertionError(f"unknown eval script: {script[:80]}")
|
||||
|
||||
mock = AsyncMock()
|
||||
@@ -100,8 +111,8 @@ async def test_increment_uses_atomic_eval_not_separate_get_set() -> None:
|
||||
mock = _make_redis_mock()
|
||||
tracker = _make_tracker(mock)
|
||||
await tracker.activate(retry_after=60.0, affected_agents=["be-dev-1"])
|
||||
# activate issued the only legitimate SET; reset call counts so the increment
|
||||
# path's commands are isolated.
|
||||
# activate routes through its own atomic eval; reset call counts so the
|
||||
# increment path's commands are isolated.
|
||||
mock.set.reset_mock()
|
||||
mock.get.reset_mock()
|
||||
mock.eval.reset_mock()
|
||||
@@ -182,3 +193,100 @@ async def test_reset_zeroes_after_increments() -> None:
|
||||
await tracker.increment_probe_failures()
|
||||
await tracker.reset_probe_failures()
|
||||
assert (await tracker.get_state())["probe_failures"] == 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #156: activate must MERGE probe_failures, not blind-SET the whole blob.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_INITIAL_RETRY_AFTER = 60.0
|
||||
_REPARK_RETRY_AFTER = 300.0
|
||||
_PROBE_FAILURES_BEFORE_REPARK = 8
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_activate_routes_through_atomic_eval() -> None:
|
||||
"""activate must go through ``eval`` (one atomic server-side op), not a
|
||||
separate ``get``+``set`` — a non-atomic activate is exactly the blind write
|
||||
whose SET can land after a racing increment's SET and reset the count to 0.
|
||||
The merge runs server-side so it is indivisible w.r.t. increment/reset."""
|
||||
mock = _make_redis_mock()
|
||||
tracker = _make_tracker(mock)
|
||||
await tracker.activate(
|
||||
retry_after=_INITIAL_RETRY_AFTER, affected_agents=["be-dev-1"]
|
||||
)
|
||||
mock.eval.assert_awaited_once()
|
||||
mock.set.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_activate_fresh_starts_probe_failures_at_zero() -> None:
|
||||
"""With no prior blob there is nothing to merge — activate starts a fresh
|
||||
episode at probe_failures=0 and writes the full episode metadata."""
|
||||
mock = _make_redis_mock()
|
||||
tracker = _make_tracker(mock)
|
||||
await tracker.activate(
|
||||
retry_after=_INITIAL_RETRY_AFTER,
|
||||
affected_agents=["be-dev-1"],
|
||||
kind="rate_limited",
|
||||
)
|
||||
state = await tracker.get_state()
|
||||
assert state["probe_failures"] == 0
|
||||
assert state["rate_limited"] is True
|
||||
assert state["kind"] == "rate_limited"
|
||||
assert state["retry_after"] == _INITIAL_RETRY_AFTER
|
||||
assert state["affected_agents"] == ["be-dev-1"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_activate_preserves_probe_failures_on_repark() -> None:
|
||||
"""#156: a re-park (activate) used to blind-SET a fresh blob with
|
||||
probe_failures=0, so a probe-failure increment that just landed (or was in
|
||||
flight) could be wiped by the concurrent re-park — resetting the give-up /
|
||||
CEO-notify count mid-episode. activate must MERGE: refresh the episode
|
||||
metadata (kind / activated_at / retry_after / affected_agents) while
|
||||
preserving the accumulated probe_failures count."""
|
||||
mock = _make_redis_mock()
|
||||
tracker = _make_tracker(mock)
|
||||
await tracker.activate(
|
||||
retry_after=_INITIAL_RETRY_AFTER,
|
||||
affected_agents=["be-dev-1"],
|
||||
kind="rate_limited",
|
||||
)
|
||||
for _ in range(_PROBE_FAILURES_BEFORE_REPARK):
|
||||
await tracker.increment_probe_failures()
|
||||
assert (await tracker.get_state())[
|
||||
"probe_failures"
|
||||
] == _PROBE_FAILURES_BEFORE_REPARK
|
||||
|
||||
await tracker.activate(
|
||||
retry_after=_REPARK_RETRY_AFTER,
|
||||
affected_agents=["be-dev-1", "fe-dev-1"],
|
||||
kind="overloaded",
|
||||
)
|
||||
|
||||
state = await tracker.get_state()
|
||||
assert state["probe_failures"] == _PROBE_FAILURES_BEFORE_REPARK # preserved, not 0
|
||||
# episode metadata refreshed by the re-park
|
||||
assert state["kind"] == "overloaded"
|
||||
assert state["retry_after"] == _REPARK_RETRY_AFTER
|
||||
assert state["affected_agents"] == ["be-dev-1", "fe-dev-1"]
|
||||
assert state["rate_limited"] is True
|
||||
# a subsequent increment continues from the preserved count
|
||||
assert await tracker.increment_probe_failures() == _PROBE_FAILURES_BEFORE_REPARK + 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_activate_repark_after_reset_keeps_zero() -> None:
|
||||
"""A re-park arriving after the counter was explicitly reset (probe_failures
|
||||
already 0) carries over 0 — merge never manufactures a positive count."""
|
||||
mock = _make_redis_mock()
|
||||
tracker = _make_tracker(mock)
|
||||
await tracker.activate()
|
||||
for _ in range(5):
|
||||
await tracker.increment_probe_failures()
|
||||
await tracker.reset_probe_failures()
|
||||
await tracker.activate(
|
||||
retry_after=_INITIAL_RETRY_AFTER, affected_agents=["be-dev-1"]
|
||||
)
|
||||
assert (await tracker.get_state())["probe_failures"] == 0
|
||||
|
||||
@@ -35,10 +35,20 @@ def _report() -> ReleaseReadinessReport:
|
||||
class _FakeOps:
|
||||
"""Records the call sequence; flags drive gate/CI/already-published outcomes."""
|
||||
|
||||
def __init__(self, *, already: bool = False, gate: bool = True, ci: bool = True):
|
||||
def __init__(
|
||||
self,
|
||||
*,
|
||||
already: bool = False,
|
||||
gate: bool = True,
|
||||
ci: bool = True,
|
||||
commit_raises: str | None = None,
|
||||
publish_raises: str | None = None,
|
||||
):
|
||||
self._already = already
|
||||
self._gate = gate
|
||||
self._ci = ci
|
||||
self._commit_raises = commit_raises
|
||||
self._publish_raises = publish_raises
|
||||
self.calls: list[str] = []
|
||||
self.bumped_plan: list[str] | None = None
|
||||
self.bumped_version: str | None = None
|
||||
@@ -62,6 +72,8 @@ class _FakeOps:
|
||||
|
||||
async def commit_and_push(self, _version: str) -> str:
|
||||
self.calls.append("commit")
|
||||
if self._commit_raises is not None:
|
||||
raise RuntimeError(self._commit_raises)
|
||||
return "deadbeef"
|
||||
|
||||
async def wait_for_ci(self, _commit_sha: str) -> bool:
|
||||
@@ -70,6 +82,8 @@ class _FakeOps:
|
||||
|
||||
async def publish_release(self, version: str, _notes: str) -> str:
|
||||
self.calls.append("publish")
|
||||
if self._publish_raises is not None:
|
||||
raise RuntimeError(self._publish_raises)
|
||||
return f"https://github.com/x/roboco/releases/tag/v{version}"
|
||||
|
||||
|
||||
@@ -129,6 +143,37 @@ async def test_already_published_is_a_noop() -> None:
|
||||
assert "publish" not in ops.calls
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_commit_push_failure_returns_structured_commit_failed() -> None:
|
||||
"""#88: a RuntimeError from commit_and_push (gpgsign/pre-commit/non-ff
|
||||
push) becomes a structured ``commit_failed`` result — not a 500 bubbling
|
||||
out of ``approve``. Fail-closed: publish never runs."""
|
||||
ops = _FakeOps(commit_raises="release push failed: non-fast-forward")
|
||||
result = await ReleaseExecutor(ops).execute(_report())
|
||||
assert result.status == "commit_failed"
|
||||
assert result.commit_sha is None
|
||||
assert result.release_url is None
|
||||
assert "commit_failed" in result.detail or "push failed" in result.detail
|
||||
assert "publish" not in ops.calls
|
||||
assert "ci" not in ops.calls
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_failure_returns_structured_publish_failed() -> None:
|
||||
"""#88: a RuntimeError from ``gh release create`` (auth/quota/network) becomes
|
||||
a structured ``publish_failed`` result. The commit is already pushed and CI
|
||||
is green, so the release is half-landed — the CEO can retry ``gh release
|
||||
create`` for the same version (the executor is idempotent on the commit
|
||||
side). No 500."""
|
||||
ops = _FakeOps(publish_raises="gh release create failed: forbidden")
|
||||
result = await ReleaseExecutor(ops).execute(_report())
|
||||
assert result.status == "publish_failed"
|
||||
assert result.commit_sha == "deadbeef"
|
||||
assert result.release_url is None
|
||||
assert "gh release create failed" in result.detail
|
||||
assert ops.calls.count("publish") == _ONE
|
||||
|
||||
|
||||
def test_release_result_carries_outcome_fields() -> None:
|
||||
result = ReleaseResult(
|
||||
status="published",
|
||||
|
||||
@@ -39,11 +39,13 @@ _MIN_PUBLISH_TIMEOUT = 120 # gh release create
|
||||
class _HangingProc:
|
||||
"""Subprocess whose ``communicate()`` never resolves — a hung git/make/gh.
|
||||
|
||||
Records ``kill()`` so a test can assert the child was reaped, not leaked.
|
||||
Records ``kill()`` and ``wait()`` so a test can assert the child was reaped
|
||||
(kill + wait), not leaked as a zombie.
|
||||
"""
|
||||
|
||||
def __init__(self) -> None:
|
||||
self.killed = False
|
||||
self.waited = False
|
||||
self._never: asyncio.Future[None] = asyncio.Future()
|
||||
|
||||
async def communicate(self) -> tuple[bytes, bytes]:
|
||||
@@ -53,6 +55,12 @@ class _HangingProc:
|
||||
def kill(self) -> None:
|
||||
self.killed = True
|
||||
|
||||
async def wait(self) -> int:
|
||||
# A real killed child's wait() resolves once the OS reaps it; the fake
|
||||
# records the reap and returns the timeout rc.
|
||||
self.waited = True
|
||||
return _TIMEOUT_RC
|
||||
|
||||
|
||||
class _DoneProc:
|
||||
"""Subprocess that completes immediately with a fixed rc + stdout."""
|
||||
@@ -61,6 +69,7 @@ class _DoneProc:
|
||||
self.returncode = returncode
|
||||
self._out = out
|
||||
self.killed = False
|
||||
self.waited = False
|
||||
|
||||
async def communicate(self) -> tuple[bytes, bytes]:
|
||||
return (self._out, b"")
|
||||
@@ -68,6 +77,10 @@ class _DoneProc:
|
||||
def kill(self) -> None:
|
||||
self.killed = True
|
||||
|
||||
async def wait(self) -> int:
|
||||
self.waited = True
|
||||
return self.returncode
|
||||
|
||||
|
||||
def _exec_returning(proc: object) -> object:
|
||||
async def _exec(*_args: object, **_kwargs: object) -> object:
|
||||
@@ -147,6 +160,24 @@ async def test_git_op_times_out_and_kills_proc(monkeypatch: pytest.MonkeyPatch)
|
||||
assert proc.killed
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_git_op_timeout_reaps_the_zombie(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""A timed-out child is ``kill()`` + ``wait()`` — reaped, not left as a
|
||||
zombie that leaks the PID/PGID and pipe FDs over a long release session."""
|
||||
monkeypatch.setattr(
|
||||
"roboco.services.release_executor._GIT_OP_TIMEOUT_SECONDS", 0.05
|
||||
)
|
||||
proc = _HangingProc()
|
||||
monkeypatch.setattr(
|
||||
"roboco.services.release_executor.asyncio.create_subprocess_exec",
|
||||
_exec_returning(proc),
|
||||
)
|
||||
ops = _ops()
|
||||
await asyncio.wait_for(ops._git("rev-parse", "HEAD"), timeout=2.0)
|
||||
assert proc.killed
|
||||
assert proc.waited # kill without wait leaves a zombie
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_gate_times_out_returns_false(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
|
||||
@@ -5,14 +5,19 @@ sees the lock held and refuses instead of racing on the writable clone.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models.base import TaskStatus
|
||||
from roboco.services import release_proposal as rp
|
||||
from roboco.services.release_executor import ReleaseResult
|
||||
from roboco.services.release_proposal import ReleaseProposalService
|
||||
from roboco.services.release_proposal import (
|
||||
_RELEASE_LOCK_PREFIX,
|
||||
ReleaseProposalService,
|
||||
)
|
||||
|
||||
|
||||
def _task(*, source: str = "release_manager") -> MagicMock:
|
||||
@@ -30,26 +35,55 @@ def _session() -> MagicMock:
|
||||
|
||||
|
||||
class _FakeRedis:
|
||||
"""Single-key SET NX / DEL recorder for the release-proposal lock."""
|
||||
"""In-memory single-key store backing the release-proposal lock.
|
||||
|
||||
Models ``SET NX EX``, ``GET``, ``EXPIRE`` and the two Lua scripts the
|
||||
service uses (compare-and-del release, compare-and-expire heartbeat) so the
|
||||
fencing token and heartbeat are observable without a real Redis.
|
||||
"""
|
||||
|
||||
def __init__(self, *, held: bool = False) -> None:
|
||||
self._held = held
|
||||
self._force_held = held
|
||||
self._store: dict[str, str] = {}
|
||||
self.set_calls: list[tuple[str, str, bool, int]] = []
|
||||
self.del_calls: list[str] = []
|
||||
self.eval_calls: list[tuple[str, tuple[Any, ...]]] = []
|
||||
self.expire_calls: list[tuple[str, Any]] = []
|
||||
|
||||
async def set(
|
||||
self, name: str, value: str, *, nx: bool = False, ex: int = 0
|
||||
) -> bool:
|
||||
self.set_calls.append((name, value, nx, ex))
|
||||
if nx and self._held:
|
||||
if nx and (self._force_held or name in self._store):
|
||||
return False
|
||||
self._held = True
|
||||
self._store[name] = value
|
||||
return True
|
||||
|
||||
async def get(self, name: str) -> str | None:
|
||||
return self._store.get(name)
|
||||
|
||||
async def expire(self, name: str, seconds: int) -> bool:
|
||||
if name in self._store:
|
||||
self.expire_calls.append((name, seconds))
|
||||
return True
|
||||
return False
|
||||
|
||||
async def delete(self, name: str) -> int:
|
||||
self.del_calls.append(name)
|
||||
self._held = False
|
||||
return 1
|
||||
return 1 if self._store.pop(name, None) is not None else 0
|
||||
|
||||
async def eval(self, script: str, _numkeys: int, *args: Any) -> int:
|
||||
self.eval_calls.append((script, args))
|
||||
key = args[0]
|
||||
token = args[1]
|
||||
if "expire" in script:
|
||||
if self._store.get(key) == token:
|
||||
self.expire_calls.append((key, args[2]))
|
||||
return 1
|
||||
return 0
|
||||
# compare-and-del release
|
||||
if self._store.get(key) == token:
|
||||
self._store.pop(key, None)
|
||||
return 1
|
||||
return 0
|
||||
|
||||
async def aclose(self) -> None:
|
||||
return None
|
||||
@@ -139,7 +173,10 @@ async def test_approve_acquires_lock_then_runs_executor_and_releases() -> None:
|
||||
assert nx is True
|
||||
assert ex >= _FORTY_MIN_SECONDS # > the 40 min CI ceiling
|
||||
assert name.endswith(str(task.id))
|
||||
assert fake_redis.del_calls == [name]
|
||||
# Released via the fenced compare-and-del (a bare delete would not record
|
||||
# an eval); the lock is no longer held.
|
||||
assert await fake_redis.get(name) is None
|
||||
assert any("del" in s for s, _ in fake_redis.eval_calls)
|
||||
w["executor"].execute.assert_awaited_once()
|
||||
|
||||
|
||||
@@ -205,5 +242,154 @@ async def test_failed_execute_releases_lock_so_ceo_can_retry() -> None:
|
||||
assert result.status == "gate_failed"
|
||||
# Proposal stays open (not COMPLETED) for retry...
|
||||
assert task.status != TaskStatus.COMPLETED.value
|
||||
# ...and the lock is released so the retry can acquire it.
|
||||
assert len(fake_redis.del_calls) == 1
|
||||
# ...and the lock is released via the fenced compare-and-del so the retry
|
||||
# can acquire it.
|
||||
assert len(fake_redis.set_calls) == 1
|
||||
name, _value, _nx, _ex = fake_redis.set_calls[0]
|
||||
assert await fake_redis.get(name) is None
|
||||
assert any("del" in s for s, _ in fake_redis.eval_calls)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fenced_release_does_not_delete_a_usurper_lock() -> None:
|
||||
"""The first execute's late finally must not release a lock a usurper
|
||||
re-acquired after TTL expiry — the fencing token makes release compare-and-del."""
|
||||
task = _task()
|
||||
fake_redis = _FakeRedis()
|
||||
lock_key = f"{_RELEASE_LOCK_PREFIX}{task.id}"
|
||||
svc = ReleaseProposalService(_session())
|
||||
|
||||
with patch(
|
||||
"roboco.services.release_proposal.redis.from_url", return_value=fake_redis
|
||||
):
|
||||
token_a = await svc._acquire_release_lock(lock_key)
|
||||
assert token_a is not None
|
||||
assert await fake_redis.get(lock_key) == token_a
|
||||
|
||||
# TTL expired mid-execute and a second approve re-acquired with its own token.
|
||||
fake_redis._store[lock_key] = "usurper-token"
|
||||
|
||||
# The first execute's finally runs late and tries to release its stale token.
|
||||
await svc._release_release_lock(lock_key, token_a)
|
||||
|
||||
# The usurper's lock survives — a bare DEL would have deleted it.
|
||||
assert await fake_redis.get(lock_key) == "usurper-token"
|
||||
|
||||
# And the positive case: releasing with the owning token does clear it.
|
||||
with patch(
|
||||
"roboco.services.release_proposal.redis.from_url", return_value=fake_redis
|
||||
):
|
||||
await svc._release_release_lock(lock_key, "usurper-token")
|
||||
assert await fake_redis.get(lock_key) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_redis_outage_returns_redis_unavailable_not_already_in_progress() -> None:
|
||||
"""#89: when Redis itself is unreachable, ``approve`` stays fail-closed
|
||||
(execute never runs) but returns a distinct ``redis_unavailable`` result so
|
||||
the CEO sees the real cause — not a misleading ``already_in_progress``
|
||||
(which would imply a concurrent approve to wait out)."""
|
||||
task = _task()
|
||||
published = ReleaseResult(
|
||||
status="published",
|
||||
version="0.13.0",
|
||||
files_changed=[],
|
||||
commit_sha=None,
|
||||
release_url=None,
|
||||
detail="ok",
|
||||
)
|
||||
|
||||
broken_redis = MagicMock()
|
||||
broken_redis.set = AsyncMock(side_effect=ConnectionError("redis down"))
|
||||
broken_redis.aclose = AsyncMock()
|
||||
|
||||
task_svc = MagicMock()
|
||||
task_svc.get = AsyncMock(return_value=task)
|
||||
executor = MagicMock()
|
||||
executor.execute = AsyncMock(return_value=published)
|
||||
markers_mod = MagicMock()
|
||||
markers_mod.get_release_report = MagicMock(return_value=_REPORT)
|
||||
report = MagicMock()
|
||||
report.proposed_version = "0.13.0"
|
||||
report_from_dict_mock = MagicMock(return_value=report)
|
||||
|
||||
svc = ReleaseProposalService(_session())
|
||||
with (
|
||||
patch(
|
||||
"roboco.services.release_proposal.get_task_service", return_value=task_svc
|
||||
),
|
||||
patch(
|
||||
"roboco.services.release_proposal.get_release_executor",
|
||||
AsyncMock(return_value=executor),
|
||||
),
|
||||
patch("roboco.services.release_proposal.markers", markers_mod),
|
||||
patch(
|
||||
"roboco.services.release_proposal.report_from_dict",
|
||||
report_from_dict_mock,
|
||||
),
|
||||
patch(
|
||||
"roboco.services.release_proposal.redis.from_url", return_value=broken_redis
|
||||
),
|
||||
):
|
||||
result = await svc.approve(task.id)
|
||||
|
||||
assert result is not None
|
||||
assert result.status == "redis_unavailable"
|
||||
assert result.release_url is None
|
||||
# Fail-closed: the executor MUST NOT run without the mutex.
|
||||
executor.execute.assert_not_awaited()
|
||||
# The report is built before the lock attempt so the result carries the
|
||||
# proposed version even on an infra failure (the CEO sees which version
|
||||
# the approval was for).
|
||||
report_from_dict_mock.assert_called_once_with(_REPORT)
|
||||
# And the proposal is not marked COMPLETED.
|
||||
assert task.status != TaskStatus.COMPLETED.value
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_refreshes_lock_and_is_cancelled_in_finally(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The heartbeat refreshes the TTL while the execute owns the lock and is
|
||||
cancelled (no leaked task) before the fenced release."""
|
||||
monkeypatch.setattr(rp, "_RELEASE_LOCK_HEARTBEAT_SECONDS", 0.001)
|
||||
|
||||
task = _task()
|
||||
fake_redis = _FakeRedis()
|
||||
published = ReleaseResult(
|
||||
status="published",
|
||||
version="0.13.0",
|
||||
files_changed=["pyproject.toml"],
|
||||
commit_sha="abc",
|
||||
release_url="https://x",
|
||||
detail="ok",
|
||||
)
|
||||
|
||||
async def _slow_execute(_report: Any) -> ReleaseResult:
|
||||
# Yield long enough for ≥1 heartbeat refresh to land.
|
||||
await asyncio.sleep(0.02)
|
||||
return published
|
||||
|
||||
w = _wire(task, _REPORT, published, fake_redis)
|
||||
w["executor"].execute = AsyncMock(side_effect=_slow_execute)
|
||||
svc = ReleaseProposalService(_session())
|
||||
|
||||
with (
|
||||
w["patches"][0],
|
||||
w["patches"][1],
|
||||
w["patches"][2],
|
||||
w["patches"][3],
|
||||
w["patches"][4],
|
||||
):
|
||||
result = await svc.approve(task.id)
|
||||
|
||||
assert result is not None
|
||||
assert result.status == "published"
|
||||
name, _token, _nx, _ex = fake_redis.set_calls[0]
|
||||
# At least one compare-and-expire refreshed the TTL — the fake only records
|
||||
# an expire when the stored value still equals our fencing token, so this
|
||||
# proves the heartbeat extended a lock we still owned.
|
||||
assert len(fake_redis.expire_calls) >= 1
|
||||
assert all(k == name for k, _ttl in fake_redis.expire_calls)
|
||||
# And the lock was released at the end.
|
||||
assert await fake_redis.get(name) is None
|
||||
|
||||
@@ -10,6 +10,7 @@ from __future__ import annotations
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
import redis.asyncio as redis_asyncio
|
||||
from roboco.config import settings as cfg
|
||||
from roboco.services.notification import NotificationService
|
||||
from roboco.services.self_heal_engine import SelfHealEngine
|
||||
@@ -81,7 +82,15 @@ async def test_run_cycle_notifies_ceo_when_enabled(
|
||||
monkeypatch.setattr(cfg, "self_heal_enabled", True)
|
||||
send = AsyncMock()
|
||||
monkeypatch.setattr(NotificationService, "send_ack_notification", send)
|
||||
obs = await _engine([_sample(1.0)]).run_cycle()
|
||||
engine = _engine([_sample(1.0)])
|
||||
# Isolate the notify-fires assertion from Redis/dedupe + the open-task
|
||||
# lookup (run_cycle now dedupes per fingerprint and links a task_id).
|
||||
monkeypatch.setattr(engine, "_already_notified", AsyncMock(return_value=False))
|
||||
monkeypatch.setattr(engine, "_mark_notified", AsyncMock(return_value=None))
|
||||
monkeypatch.setattr(
|
||||
engine, "_open_self_heal_task_ids_by_fp", AsyncMock(return_value={})
|
||||
)
|
||||
obs = await engine.run_cycle()
|
||||
assert len(obs) == 1
|
||||
send.assert_awaited_once()
|
||||
call = send.await_args
|
||||
@@ -96,3 +105,110 @@ async def test_fingerprint_is_stable() -> None:
|
||||
a = (await _engine([_sample(1.0)]).assess())[0].fingerprint
|
||||
b = (await _engine([_sample(1.0)]).assess())[0].fingerprint
|
||||
assert a == b
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #43: per-fingerprint notify dedupe + task_id linking + fail-open
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_cycle_dedupes_repeated_fingerprint_across_cycles(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#43: a regression that stays red across cycles must notify the CEO ONCE
|
||||
per episode, not every cycle. The engine keeps a per-fingerprint "already
|
||||
notified" guard so a persistent red state doesn't spam the CEO each tick."""
|
||||
monkeypatch.setattr(cfg, "self_heal_enabled", True)
|
||||
monkeypatch.setattr(cfg, "self_heal_originate_enabled", False)
|
||||
send = AsyncMock()
|
||||
monkeypatch.setattr(NotificationService, "send_ack_notification", send)
|
||||
|
||||
notified: set[str] = set()
|
||||
|
||||
async def _already(fp: str) -> bool:
|
||||
return fp in notified
|
||||
|
||||
async def _mark(fp: str) -> None:
|
||||
notified.add(fp)
|
||||
|
||||
async def _open_map() -> dict[str, object]:
|
||||
return {}
|
||||
|
||||
engine = _engine([_sample(1.0)])
|
||||
monkeypatch.setattr(engine, "_already_notified", _already)
|
||||
monkeypatch.setattr(engine, "_mark_notified", _mark)
|
||||
monkeypatch.setattr(engine, "_open_self_heal_task_ids_by_fp", _open_map)
|
||||
|
||||
await engine.run_cycle()
|
||||
await engine.run_cycle() # same red state — must NOT re-notify
|
||||
|
||||
send.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_cycle_links_task_id_when_open_task_exists(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#43: the CEO notification carries the open self-heal fix task's id so the
|
||||
panel can route the CEO to the fix (instead of a free-floating alert)."""
|
||||
monkeypatch.setattr(cfg, "self_heal_enabled", True)
|
||||
monkeypatch.setattr(cfg, "self_heal_originate_enabled", False)
|
||||
send = AsyncMock()
|
||||
monkeypatch.setattr(NotificationService, "send_ack_notification", send)
|
||||
|
||||
fp = (await _engine([_sample(1.0)]).assess())[0].fingerprint
|
||||
task_uuid = "11111111-1111-1111-1111-111111111111"
|
||||
|
||||
async def _already(_fp: str) -> bool:
|
||||
return False
|
||||
|
||||
async def _mark(_fp: str) -> None:
|
||||
return None
|
||||
|
||||
async def _open_map() -> dict[str, object]:
|
||||
return {fp: task_uuid}
|
||||
|
||||
engine = _engine([_sample(1.0)])
|
||||
monkeypatch.setattr(engine, "_already_notified", _already)
|
||||
monkeypatch.setattr(engine, "_mark_notified", _mark)
|
||||
monkeypatch.setattr(engine, "_open_self_heal_task_ids_by_fp", _open_map)
|
||||
|
||||
await engine.run_cycle()
|
||||
|
||||
send.assert_awaited_once()
|
||||
call = send.await_args
|
||||
assert call is not None
|
||||
assert call.kwargs.get("task_id") == task_uuid
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_cycle_notifies_when_dedupe_check_fails_open(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""#43: a Redis outage in the dedupe check must FAIL OPEN — better to risk
|
||||
a duplicate CEO ping than to silently swallow a regression alert. The real
|
||||
_already_notified catches the Redis error and returns False (notify)."""
|
||||
monkeypatch.setattr(cfg, "self_heal_enabled", True)
|
||||
monkeypatch.setattr(cfg, "self_heal_originate_enabled", False)
|
||||
send = AsyncMock()
|
||||
monkeypatch.setattr(NotificationService, "send_ack_notification", send)
|
||||
|
||||
def _boom(*_a: object, **_kw: object) -> object:
|
||||
raise RuntimeError("redis down")
|
||||
|
||||
monkeypatch.setattr(redis_asyncio, "from_url", _boom)
|
||||
|
||||
async def _mark(_fp: str) -> None:
|
||||
return None
|
||||
|
||||
async def _open_map() -> dict[str, object]:
|
||||
return {}
|
||||
|
||||
engine = _engine([_sample(1.0)])
|
||||
monkeypatch.setattr(engine, "_mark_notified", _mark)
|
||||
monkeypatch.setattr(engine, "_open_self_heal_task_ids_by_fp", _open_map)
|
||||
|
||||
await engine.run_cycle()
|
||||
|
||||
send.assert_awaited_once()
|
||||
|
||||
@@ -595,6 +595,126 @@ async def test_admin_set_status_non_blocked_is_bare_status_set() -> None:
|
||||
assert task.assigned_to == owner
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_admin_set_status_force_emits_distinct_override_audit_row() -> None:
|
||||
"""#13: a forced override (force=True) emits a distinct ``task.admin_override``
|
||||
audit row marking the bypass past the lifecycle gate — distinguishable from
|
||||
the in-band transition row. Without force, no override row is emitted."""
|
||||
owner = uuid4()
|
||||
task = _build_task(
|
||||
status=TaskStatus.AWAITING_PM_REVIEW,
|
||||
assigned_to=owner,
|
||||
claimed_by=owner,
|
||||
)
|
||||
added: list[object] = []
|
||||
session = MagicMock()
|
||||
session.flush = AsyncMock()
|
||||
session.add.side_effect = added.append
|
||||
svc = TaskService(session)
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
|
||||
await svc.admin_set_status(
|
||||
task.id, TaskStatus.COMPLETED, actor_id=owner, actor_role="ceo", force=True
|
||||
)
|
||||
|
||||
rows = [r for r in added if isinstance(r, AuditLogTable)]
|
||||
override_rows = [r for r in rows if r.event_type == "task.admin_override"]
|
||||
assert len(override_rows) == 1
|
||||
row = override_rows[0]
|
||||
assert row.target_id == task.id
|
||||
assert row.severity == "warning"
|
||||
assert row.details["forced"] is True
|
||||
assert row.details["from_status"] == "awaiting_pm_review"
|
||||
assert row.details["to_status"] == "completed"
|
||||
assert row.agent_id == owner
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_admin_set_status_no_force_emits_no_override_audit_row() -> None:
|
||||
"""#13: without force, only the transition row is emitted — no
|
||||
``task.admin_override`` row (the bypass was not acknowledged)."""
|
||||
owner = uuid4()
|
||||
task = _build_task(
|
||||
status=TaskStatus.BLOCKED,
|
||||
assigned_to=owner,
|
||||
claimed_by=owner,
|
||||
)
|
||||
added: list[object] = []
|
||||
session = MagicMock()
|
||||
session.flush = AsyncMock()
|
||||
session.add.side_effect = added.append
|
||||
svc = TaskService(session)
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
|
||||
await svc.admin_set_status(task.id, TaskStatus.PENDING, actor_id=owner)
|
||||
|
||||
rows = [r for r in added if isinstance(r, AuditLogTable)]
|
||||
assert not any(r.event_type == "task.admin_override" for r in rows)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_block_restore_skips_revision_count_bump() -> None:
|
||||
"""#101 Gap B: restoring a blocked task to its snapshotted needs_revision
|
||||
state is a RESTORE, not a rework bounce — ``revision_count`` must not
|
||||
increment. The rework counter counts rejections INTO needs_revision, and
|
||||
this task was already rejected before it was blocked; unblocking it back to
|
||||
needs_revision is the same rework cycle resuming, not a new one."""
|
||||
REWORK_BOUNCES_BEFORE_BLOCK = 2
|
||||
dev = uuid4()
|
||||
task = _build_task(
|
||||
status=TaskStatus.BLOCKED,
|
||||
assigned_to=dev,
|
||||
claimed_by=dev,
|
||||
branch_name="feature/backend/abc--def--ghi",
|
||||
pre_block_state="needs_revision",
|
||||
pre_block_assignee=dev,
|
||||
revision_count=REWORK_BOUNCES_BEFORE_BLOCK,
|
||||
)
|
||||
svc = TaskService(MagicMock(flush=AsyncMock()))
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
out = await svc.unblock_with_restore(task.id, uuid4(), restore=True)
|
||||
assert out is task
|
||||
assert task.status == TaskStatus.NEEDS_REVISION
|
||||
# A restore must NOT bump the rework counter — only a fresh rejection does.
|
||||
assert task.revision_count == REWORK_BOUNCES_BEFORE_BLOCK
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_activate_batch_root_subtasks_emits_audit_for_activated_child() -> None:
|
||||
"""#101 Gap A: ``_activate_batch_root_subtasks`` sets a held root-subtask
|
||||
BACKLOG→PENDING directly. No status change may bypass the audit log — the
|
||||
transition journey (the metric source of truth) must record the activation,
|
||||
or the child's lifecycle reconstruction silently drops its start point."""
|
||||
batch = uuid4()
|
||||
child = _build_task(
|
||||
status=TaskStatus.BACKLOG,
|
||||
batch_id=batch,
|
||||
team=Team.BOARD,
|
||||
task_type=TaskType.CODE,
|
||||
)
|
||||
umbrella = _build_task(
|
||||
status=TaskStatus.PENDING,
|
||||
batch_id=batch,
|
||||
parent_task_id=None,
|
||||
team=Team.BOARD,
|
||||
task_type=TaskType.PLANNING,
|
||||
)
|
||||
added: list[object] = []
|
||||
session = MagicMock()
|
||||
session.flush = AsyncMock()
|
||||
session.add.side_effect = added.append
|
||||
svc = TaskService(session)
|
||||
_bind(svc, "get_subtasks", AsyncMock(return_value=[child]))
|
||||
|
||||
await svc._activate_batch_root_subtasks(umbrella)
|
||||
|
||||
assert child.status == TaskStatus.PENDING
|
||||
rows = [r for r in added if isinstance(r, AuditLogTable)]
|
||||
assert any(
|
||||
r.event_type == "task.pending" and r.target_id == child.id for r in rows
|
||||
), "batch root-subtask activation must emit a task.pending audit row"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_generates_ac_ids_and_carries_parent_ac_refs() -> None:
|
||||
# Every task gets one stable id per acceptance criterion (1:1), and a
|
||||
|
||||
@@ -0,0 +1,112 @@
|
||||
"""#96/#97: the periodic flush must (a) actually remove a ready buffer after
|
||||
notifying callbacks (``get_ready_buffers`` only peeks — without a flush the
|
||||
buffer map grows unbounded), and (b) offload each sync callback to a thread so
|
||||
a slow callback can't block the flush task / event loop.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models.transcription import TranscriptionConfig
|
||||
from roboco.services.transcription import TranscriptionService
|
||||
|
||||
|
||||
def _svc() -> TranscriptionService:
|
||||
# Tiny intervals so a single flush tick is fast and deterministic.
|
||||
return TranscriptionService(
|
||||
TranscriptionConfig(
|
||||
min_chars_for_extraction=2,
|
||||
idle_threshold_seconds=0.0,
|
||||
flush_interval_seconds=0.01,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
def _ready_buffer(svc: TranscriptionService, content: str) -> tuple[UUID, UUID]:
|
||||
"""Create a buffer for a fresh agent/session, append content, mark it
|
||||
complete (ready for extraction). Returns the (agent_id, session_id)."""
|
||||
agent_id, session_id, channel_id = uuid4(), uuid4(), uuid4()
|
||||
buffer = svc.get_buffer(
|
||||
agent_id=agent_id, session_id=session_id, channel_id=channel_id
|
||||
)
|
||||
buffer.append(content)
|
||||
buffer.is_complete = True
|
||||
return agent_id, session_id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_periodic_flush_removes_ready_buffer() -> None:
|
||||
"""#96: after a flush tick, a ready buffer is popped from ``_buffers``
|
||||
(not re-yielded forever). Without the flush call the map grew unbounded."""
|
||||
svc = _svc()
|
||||
agent_id, session_id = _ready_buffer(svc, "some content")
|
||||
assert agent_id in svc._buffers
|
||||
|
||||
await svc._flush_ready_buffers()
|
||||
|
||||
# The ready buffer was flushed (removed), not left to accumulate.
|
||||
assert agent_id not in svc._buffers or session_id not in svc._buffers[agent_id]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_periodic_flush_invokes_callbacks() -> None:
|
||||
"""#96: callbacks are still notified on the flush tick."""
|
||||
svc = _svc()
|
||||
_ready_buffer(svc, "content for callback")
|
||||
|
||||
seen: list[object] = []
|
||||
svc.register_callback(seen.append)
|
||||
|
||||
await svc._flush_ready_buffers()
|
||||
|
||||
assert len(seen) == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_periodic_flush_offloads_sync_callback_to_thread() -> None:
|
||||
"""#97: a sync callback runs off the event loop (in a worker thread) so a
|
||||
slow callback can't block the flush task. Assert the callback executes in a
|
||||
thread that is NOT the running event loop's main thread."""
|
||||
svc = _svc()
|
||||
_ready_buffer(svc, "content for offload")
|
||||
|
||||
callback_thread: list[object] = []
|
||||
|
||||
def _slow_callback(_b: object) -> None:
|
||||
# Record the thread id; a sleep here would block the loop if not offloaded.
|
||||
callback_thread.append(threading.get_ident())
|
||||
|
||||
svc.register_callback(_slow_callback)
|
||||
|
||||
await svc._flush_ready_buffers()
|
||||
|
||||
assert len(callback_thread) == 1
|
||||
# The callback ran in a worker thread, not on the event loop's main thread.
|
||||
assert callback_thread[0] != threading.get_ident()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_periodic_flush_callback_error_does_not_abort_flush() -> None:
|
||||
"""A failing callback must not abort the flush (the buffer is still removed
|
||||
and a sibling callback still runs)."""
|
||||
svc = _svc()
|
||||
agent_id, session_id = _ready_buffer(svc, "content with bad callback")
|
||||
|
||||
sibling_seen: list[object] = []
|
||||
|
||||
def _bad(_b: object) -> None:
|
||||
raise RuntimeError("boom")
|
||||
|
||||
def _good(b: object) -> None:
|
||||
sibling_seen.append(b)
|
||||
|
||||
svc.register_callback(_bad)
|
||||
svc.register_callback(_good)
|
||||
|
||||
await svc._flush_ready_buffers() # must not raise
|
||||
|
||||
assert len(sibling_seen) == 1
|
||||
assert agent_id not in svc._buffers or session_id not in svc._buffers[agent_id]
|
||||
@@ -11,7 +11,7 @@ import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from typing import TYPE_CHECKING
|
||||
from unittest.mock import patch
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
from roboco.services.workspace import WorkspaceService
|
||||
@@ -240,3 +240,162 @@ async def test_two_concurrent_task_worktrees_independent(clone: Path) -> None:
|
||||
assert _git(wt_b, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/8e460893"
|
||||
# Clone root stays on main — neither task branch moved it.
|
||||
assert _git(clone, "rev-parse", "--abbrev-ref", "HEAD").strip() == "main"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# ensure_worktree_self_heal — spawn-time clone + branch-ref self-heal (F123).
|
||||
# A vanished clone_root fatal-looped the resume path (`git -C <missing>`); the
|
||||
# reaper-style claim release preserves ownership + branch_name, so the next
|
||||
# dispatch is a RESUME (create_branch never re-runs to re-clone). self_heal
|
||||
# recovers the branch ref from origin (create_branch pushes at claim time) so
|
||||
# the pushed work survives, falling back to a fresh branch off origin/HEAD
|
||||
# only when the branch was never pushed.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _ref_exists(repo: Path, ref: str) -> bool:
|
||||
return (
|
||||
subprocess.run(
|
||||
["git", "-C", str(repo), "rev-parse", "--verify", "--quiet", ref],
|
||||
check=False,
|
||||
).returncode
|
||||
== 0
|
||||
)
|
||||
|
||||
|
||||
async def test_self_heal_noop_when_worktree_present(clone: Path) -> None:
|
||||
svc = _service()
|
||||
wt = clone / ".worktrees" / "a3c40fe7"
|
||||
with patch("roboco.services.workspace._ensure_agent_owned"):
|
||||
await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main")
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
WorkspaceService, "_fetch_branch_ref", new_callable=AsyncMock
|
||||
) as fetch,
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
):
|
||||
await svc.ensure_worktree_self_heal(clone, wt, "feature/a3c40fe7", "proj")
|
||||
|
||||
assert fetch.await_count == 0, "present worktree must not trigger a fetch"
|
||||
assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7"
|
||||
|
||||
|
||||
async def test_self_heal_readds_pruned_worktree_from_local_ref(clone: Path) -> None:
|
||||
# Common resume case: clone healthy, worktree pruned, local branch ref
|
||||
# survives -> re-add with NO fetch (no origin round-trip on every spawn).
|
||||
svc = _service()
|
||||
wt = clone / ".worktrees" / "a3c40fe7"
|
||||
with patch("roboco.services.workspace._ensure_agent_owned"):
|
||||
await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main")
|
||||
_git(clone, "worktree", "remove", str(wt), "--force") # prune
|
||||
assert not wt.exists()
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
WorkspaceService, "_fetch_branch_ref", new_callable=AsyncMock
|
||||
) as fetch,
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
):
|
||||
await svc.ensure_worktree_self_heal(clone, wt, "feature/a3c40fe7", "proj")
|
||||
|
||||
assert fetch.await_count == 0, "local ref survives -> no fetch needed"
|
||||
assert wt.exists()
|
||||
assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7"
|
||||
|
||||
|
||||
def _bare_remote_with_branch(tmp_path: Path, branch: str, push_branch: bool) -> Path:
|
||||
"""A bare remote carrying `main`; optionally also `branch` with a commit."""
|
||||
remote = tmp_path / "remote.git"
|
||||
remote.mkdir()
|
||||
_git(remote, "init", "--bare", "-b", "main")
|
||||
src = tmp_path / "src"
|
||||
_init_clone(src)
|
||||
_git(src, "remote", "add", "origin", str(remote))
|
||||
_git(src, "push", "origin", "main")
|
||||
if push_branch:
|
||||
_git(src, "checkout", "-b", branch)
|
||||
(src / "work.txt").write_text("x")
|
||||
_git(src, "add", "work.txt")
|
||||
_git(src, "commit", "-m", "work")
|
||||
_git(src, "push", "origin", branch)
|
||||
return remote
|
||||
|
||||
|
||||
def _recloned_clone(
|
||||
tmp_path: Path, remote: Path, fetch_branch: str | None = None
|
||||
) -> Path:
|
||||
"""A clone with only `main` locally (simulates a fresh re-clone: no task
|
||||
branch ref). origin/HEAD is set so ensure_worktree's -b fallback resolves.
|
||||
When ``fetch_branch`` is given, its remote-tracking ref is pre-seeded here
|
||||
(the real ``_fetch_branch_ref`` is mocked in the test) to model a branch
|
||||
that was pushed at claim time and is recoverable from origin."""
|
||||
clone = tmp_path / "clone"
|
||||
_init_clone(clone)
|
||||
_git(clone, "remote", "add", "origin", str(remote))
|
||||
_git(clone, "fetch", "origin", "main")
|
||||
if fetch_branch is not None:
|
||||
_git(clone, "fetch", "origin", fetch_branch)
|
||||
_git(clone, "remote", "set-head", "origin", "main")
|
||||
return clone
|
||||
|
||||
|
||||
async def test_self_heal_recovers_branch_from_origin(tmp_path: Path) -> None:
|
||||
# THE BUG SCENARIO: clone vanished, re-cloned (only main locally), but the
|
||||
# task branch was pushed at claim time -> recover it from origin so the
|
||||
# pushed work survives (not -b'd over with a divergent branch). The real
|
||||
# _fetch_branch_ref is mocked (a spy); the remote-tracking ref it would
|
||||
# populate is pre-seeded, exercising the ref-recovery + worktree re-add.
|
||||
branch = "feature/8e460893"
|
||||
remote = _bare_remote_with_branch(tmp_path, branch, push_branch=True)
|
||||
clone = _recloned_clone(tmp_path, remote, fetch_branch=branch)
|
||||
assert not _ref_exists(clone, f"refs/heads/{branch}"), "precondition: no local ref"
|
||||
assert _ref_exists(
|
||||
clone, f"refs/remotes/origin/{branch}"
|
||||
), "precondition: pushed branch reachable on origin"
|
||||
|
||||
svc = _service()
|
||||
wt = clone / ".worktrees" / "8e460893"
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
WorkspaceService, "_fetch_branch_ref", new_callable=AsyncMock
|
||||
) as fetch,
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
):
|
||||
await svc.ensure_worktree_self_heal(clone, wt, branch, "proj")
|
||||
|
||||
assert fetch.await_count == 1, "missing local ref must trigger a fetch"
|
||||
assert wt.exists()
|
||||
assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == branch
|
||||
# Recovered commit present — pushed work was NOT lost to a divergent -b.
|
||||
assert (wt / "work.txt").exists(), "pushed commit must survive recovery"
|
||||
|
||||
|
||||
async def test_self_heal_falls_back_to_origin_head_when_branch_not_pushed(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
# Never-pushed branch (push failed at claim time, or first claim never
|
||||
# pushed): origin doesn't have it -> re-create from origin/HEAD rather
|
||||
# than fatal-loop. No pushed work is lost because none existed.
|
||||
branch = "feature/8e460893"
|
||||
remote = _bare_remote_with_branch(tmp_path, branch, push_branch=False)
|
||||
clone = _recloned_clone(tmp_path, remote)
|
||||
assert not _ref_exists(
|
||||
clone, f"refs/remotes/origin/{branch}"
|
||||
), "precondition: not on origin"
|
||||
|
||||
svc = _service()
|
||||
wt = clone / ".worktrees" / "8e460893"
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
WorkspaceService, "_fetch_branch_ref", new_callable=AsyncMock
|
||||
) as fetch,
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
):
|
||||
await svc.ensure_worktree_self_heal(clone, wt, branch, "proj")
|
||||
|
||||
assert fetch.await_count == 1, "missing local ref still attempts a fetch"
|
||||
assert wt.exists(), "fallback -b from origin/HEAD must break the loop"
|
||||
assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == branch
|
||||
|
||||
@@ -12,7 +12,7 @@ failure never blocks completion.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
@@ -139,7 +139,8 @@ async def test_ceo_approve_removes_assignee_worktree_best_effort() -> None:
|
||||
svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api")))
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
_bind(svc, "_validate_and_set_status", MagicMock())
|
||||
_bind(svc, "_extract_completion_learnings", AsyncMock())
|
||||
_bind(svc, "_close_work_session_for_task", AsyncMock())
|
||||
_bind(svc, "_trigger_completion_hooks", AsyncMock())
|
||||
_bind(svc, "_unblock_dependents", AsyncMock())
|
||||
_bind(svc, "_emit_task_event", AsyncMock())
|
||||
remove = AsyncMock()
|
||||
@@ -157,7 +158,8 @@ async def test_ceo_approve_skips_worktree_cleanup_for_branchless_task() -> None:
|
||||
svc, _ = _svc(AsyncMock())
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
_bind(svc, "_validate_and_set_status", MagicMock())
|
||||
_bind(svc, "_extract_completion_learnings", AsyncMock())
|
||||
_bind(svc, "_close_work_session_for_task", AsyncMock())
|
||||
_bind(svc, "_trigger_completion_hooks", AsyncMock())
|
||||
_bind(svc, "_unblock_dependents", AsyncMock())
|
||||
_bind(svc, "_emit_task_event", AsyncMock())
|
||||
remove = AsyncMock()
|
||||
@@ -167,3 +169,134 @@ async def test_ceo_approve_skips_worktree_cleanup_for_branchless_task() -> None:
|
||||
|
||||
assert result is task
|
||||
remove.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ceo_approve_closes_work_session_and_triggers_completion_hooks() -> None:
|
||||
"""#21/#98: ceo_approve must close the work session and run the full
|
||||
completion-hook fan-out (commits + dev_notes indexing), mirroring
|
||||
``complete()`` — not hand-roll a lone learnings task and leave the work
|
||||
session ACTIVE forever. The PR-merged gate already ran above."""
|
||||
task = _build_task(status=TaskStatus.AWAITING_CEO_APPROVAL, work_session_id=None)
|
||||
svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api")))
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
_bind(svc, "_validate_and_set_status", MagicMock())
|
||||
close_ws = AsyncMock()
|
||||
hooks = AsyncMock()
|
||||
_bind(svc, "_close_work_session_for_task", close_ws)
|
||||
_bind(svc, "_trigger_completion_hooks", hooks)
|
||||
_bind(svc, "_unblock_dependents", AsyncMock())
|
||||
_bind(svc, "_emit_task_event", AsyncMock())
|
||||
_bind(svc, "_remove_task_worktree_best_effort", AsyncMock())
|
||||
|
||||
result = await svc.ceo_approve(task.id)
|
||||
|
||||
assert result is task
|
||||
close_ws.assert_awaited_once()
|
||||
# The hooks fire with the CEO actor (None) — same as complete() for a
|
||||
# non-agent completion path.
|
||||
hooks.assert_awaited_once_with(task, None)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #216: recurring FS/permission cleanup failure escalates to the CEO
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _reset_cleanup_streak() -> None:
|
||||
TaskService._worktree_cleanup_fail_streak = 0
|
||||
TaskService._worktree_cleanup_escalated = False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_recurring_worktree_cleanup_failure_escalates_to_ceo() -> None:
|
||||
"""#216: a recurring FS/permission failure (stuck mount, perms) used to
|
||||
leak worktrees indefinitely with only a warning log. After N consecutive
|
||||
OSError failures a CEO alert fires once; further failures are suppressed
|
||||
until a success resets the streak."""
|
||||
_reset_cleanup_streak()
|
||||
n = TaskService._WORKTREE_CLEANUP_ESCALATE_AFTER
|
||||
task = _build_task(status=TaskStatus.COMPLETED)
|
||||
svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api")))
|
||||
_bind(
|
||||
svc,
|
||||
"_remove_task_worktree_best_effort",
|
||||
AsyncMock(side_effect=PermissionError("denied")),
|
||||
)
|
||||
|
||||
with patch(
|
||||
"roboco.services.notification.NotificationService.send_ack_notification",
|
||||
new=AsyncMock(),
|
||||
) as notifier:
|
||||
for _ in range(n - 1):
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
notifier.assert_not_awaited()
|
||||
await svc._remove_task_worktree_on_terminal(task) # Nth failure
|
||||
notifier.assert_awaited_once()
|
||||
call = notifier.await_args
|
||||
assert call is not None
|
||||
assert call.kwargs["to_agent"] == "ceo"
|
||||
assert str(task.id) in str(call.kwargs.get("task_id") or call.args)
|
||||
# Suppressed on the (N+1)th — one escalation per failure streak.
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
notifier.assert_awaited_once()
|
||||
|
||||
_reset_cleanup_streak()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_worktree_cleanup_success_resets_failure_streak() -> None:
|
||||
"""A successful cleanup resets the streak + escalation flag, so a later
|
||||
failure streak must re-accumulate to the full threshold before escalating."""
|
||||
_reset_cleanup_streak()
|
||||
n = TaskService._WORKTREE_CLEANUP_ESCALATE_AFTER
|
||||
task = _build_task(status=TaskStatus.COMPLETED)
|
||||
svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api")))
|
||||
remove = AsyncMock(side_effect=PermissionError("denied"))
|
||||
_bind(svc, "_remove_task_worktree_best_effort", remove)
|
||||
|
||||
with patch(
|
||||
"roboco.services.notification.NotificationService.send_ack_notification",
|
||||
new=AsyncMock(),
|
||||
) as notifier:
|
||||
for _ in range(n - 1):
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
notifier.assert_not_awaited()
|
||||
# A success resets the streak.
|
||||
remove.side_effect = None
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
# Re-introduce failures: needs a full N again, not 1.
|
||||
remove.side_effect = PermissionError("denied")
|
||||
for _ in range(n - 1):
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
notifier.assert_not_awaited()
|
||||
await svc._remove_task_worktree_on_terminal(task) # Nth of new streak
|
||||
notifier.assert_awaited_once()
|
||||
|
||||
_reset_cleanup_streak()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_fs_worktree_cleanup_failure_does_not_escalate() -> None:
|
||||
"""A non-FS failure (git RuntimeError, DB error) is task-specific, not the
|
||||
systemic operator-actionable FS case — it logs but never escalates, and it
|
||||
resets the streak so an interleaved FS streak can't ride on it."""
|
||||
_reset_cleanup_streak()
|
||||
n = TaskService._WORKTREE_CLEANUP_ESCALATE_AFTER
|
||||
task = _build_task(status=TaskStatus.COMPLETED)
|
||||
svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api")))
|
||||
_bind(
|
||||
svc,
|
||||
"_remove_task_worktree_best_effort",
|
||||
AsyncMock(side_effect=RuntimeError("git worktree remove failed")),
|
||||
)
|
||||
|
||||
with patch(
|
||||
"roboco.services.notification.NotificationService.send_ack_notification",
|
||||
new=AsyncMock(),
|
||||
) as notifier:
|
||||
for _ in range(n + 2):
|
||||
await svc._remove_task_worktree_on_terminal(task)
|
||||
notifier.assert_not_awaited()
|
||||
|
||||
_reset_cleanup_streak()
|
||||
|
||||
Reference in New Issue
Block a user