refactor(supervise): make Supervisor a service class the orchestrator calls
tracker-policy-pr / check-pr (pull_request) Successful in 12s
test / integration-docker (pull_request) Successful in 19s
lint / lint (push) Failing after 55s
test / unit (pull_request) Successful in 2m8s
test / integration-firecracker (pull_request) Successful in 3m25s
test / coverage (pull_request) Successful in 39s
test / publish-infra (pull_request) Has been skipped
tracker-policy-pr / check-pr (pull_request) Successful in 12s
test / integration-docker (pull_request) Successful in 19s
lint / lint (push) Failing after 55s
test / unit (pull_request) Successful in 2m8s
test / integration-firecracker (pull_request) Successful in 3m25s
test / coverage (pull_request) Successful in 39s
test / publish-infra (pull_request) Has been skipped
Turn the loose queue/audit functions in orchestrator/supervisor/queue.py into
methods on a concrete Supervisor service. The Orchestrator now owns an
injectable `self._supervisor` and calls `self._supervisor.write_proposal(...)`
etc., instead of module-level free functions — the supervise dependency is
explicit and mockable, and a Supervisor can be scoped to a `db_path` (defaults
to the host DB) so tests can point it at a temp database.
- Supervisor (in the package __init__) drops the ABC and gains write_proposal,
read_proposal, list_pending_proposals, list_all_pending_proposals,
write_response, read_response, archive_all_proposals, write_audit_entry,
read_audit_entries, plus the launch-time prepare().
- render_diff / sha256_hex are pure and stateless, so they move to
orchestrator/supervisor/util.py (re-exported from the facade) rather than
becoming methods.
- queue.py is deleted; service.py + the supervise tests call through a
Supervisor instance.
Full unit suite green (2243).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -29,7 +29,7 @@ from bot_bottle.orchestrator.supervisor import (
|
||||
Proposal,
|
||||
TOOL_EGRESS_ALLOW,
|
||||
sha256_hex,
|
||||
write_proposal,
|
||||
Supervisor,
|
||||
)
|
||||
|
||||
|
||||
@@ -421,7 +421,7 @@ class TestDispatchSupervise(unittest.TestCase):
|
||||
p = Proposal.new(
|
||||
bottle_slug=slug, tool=TOOL_EGRESS_ALLOW, proposed_file=proposed,
|
||||
justification="need it", current_file_hash=sha256_hex(proposed))
|
||||
write_proposal(p)
|
||||
Supervisor().write_proposal(p)
|
||||
return p.id
|
||||
|
||||
def test_list_pending(self) -> None:
|
||||
|
||||
@@ -20,9 +20,8 @@ from bot_bottle.orchestrator.supervisor import (
|
||||
Proposal,
|
||||
STATUS_APPROVED,
|
||||
TOOL_EGRESS_ALLOW,
|
||||
read_response,
|
||||
sha256_hex,
|
||||
write_proposal,
|
||||
Supervisor,
|
||||
)
|
||||
|
||||
|
||||
@@ -185,7 +184,7 @@ class TestOrchestratorSupervise(unittest.TestCase):
|
||||
p = Proposal.new(
|
||||
bottle_slug=slug, tool=TOOL_EGRESS_ALLOW, proposed_file=proposed,
|
||||
justification="need it", current_file_hash=sha256_hex(proposed))
|
||||
write_proposal(p)
|
||||
Supervisor().write_proposal(p)
|
||||
return p.id
|
||||
|
||||
def test_pending_lists_queued_proposal(self) -> None:
|
||||
@@ -208,7 +207,7 @@ class TestOrchestratorSupervise(unittest.TestCase):
|
||||
assert rec is not None
|
||||
self.assertEqual(new_routes, rec.policy)
|
||||
# response written -> agent unblocks, proposal no longer pending
|
||||
self.assertEqual(STATUS_APPROVED, read_response("demo", pid).status)
|
||||
self.assertEqual(STATUS_APPROVED, Supervisor().read_response("demo", pid).status)
|
||||
self.assertEqual([], self.orch.supervise_pending())
|
||||
|
||||
def test_pending_carries_human_label(self) -> None:
|
||||
@@ -261,7 +260,7 @@ class TestOrchestratorSupervise(unittest.TestCase):
|
||||
rec = self.store.get(bottle_id)
|
||||
assert rec is not None
|
||||
self.assertEqual("routes:\n - host: existing.com\n", rec.policy)
|
||||
self.assertEqual("rejected", read_response("demo", pid).status)
|
||||
self.assertEqual("rejected", Supervisor().read_response("demo", pid).status)
|
||||
|
||||
def test_unknown_proposal_is_error(self) -> None:
|
||||
ok, err = self.orch.supervise_respond(
|
||||
|
||||
@@ -17,19 +17,15 @@ from bot_bottle.orchestrator.supervisor import (
|
||||
STATUS_APPROVED,
|
||||
STATUS_MODIFIED,
|
||||
STATUS_REJECTED,
|
||||
Supervisor,
|
||||
TOOL_EGRESS_ALLOW,
|
||||
TOOL_GITLEAKS_ALLOW,
|
||||
list_pending_proposals,
|
||||
read_audit_entries,
|
||||
read_proposal,
|
||||
read_response,
|
||||
render_diff,
|
||||
sha256_hex,
|
||||
write_audit_entry,
|
||||
write_proposal,
|
||||
write_response,
|
||||
)
|
||||
|
||||
_SV = Supervisor()
|
||||
|
||||
|
||||
FIXED_TS = datetime(2026, 5, 25, 12, 0, 0, tzinfo=timezone.utc)
|
||||
|
||||
@@ -123,26 +119,26 @@ class TestQueueIO(unittest.TestCase):
|
||||
|
||||
def test_write_and_read_proposal(self):
|
||||
p = _proposal()
|
||||
path = write_proposal(p)
|
||||
path = _SV.write_proposal(p)
|
||||
self.assertTrue(path.exists())
|
||||
self.assertEqual(host_db_path(), path)
|
||||
self.assertEqual(0o600, path.stat().st_mode & 0o777)
|
||||
loaded = read_proposal(self.slug, p.id)
|
||||
loaded = _SV.read_proposal(self.slug, p.id)
|
||||
self.assertEqual(p, loaded)
|
||||
|
||||
def test_list_pending_excludes_responded(self):
|
||||
a = _proposal(justification="first")
|
||||
b = _proposal(justification="second")
|
||||
write_proposal(a)
|
||||
write_proposal(b)
|
||||
write_response(self.slug, Response(
|
||||
_SV.write_proposal(a)
|
||||
_SV.write_proposal(b)
|
||||
_SV.write_response(self.slug, Response(
|
||||
proposal_id=a.id, status=STATUS_APPROVED, notes="",
|
||||
))
|
||||
pending = list_pending_proposals(self.slug)
|
||||
pending = _SV.list_pending_proposals(self.slug)
|
||||
self.assertEqual([b.id], [p.id for p in pending])
|
||||
|
||||
def test_list_pending_returns_empty_for_missing_slug(self):
|
||||
self.assertEqual([], list_pending_proposals("nope"))
|
||||
self.assertEqual([], _SV.list_pending_proposals("nope"))
|
||||
|
||||
def test_list_pending_sorted_by_arrival(self):
|
||||
# Fabricate two with explicit timestamps.
|
||||
@@ -159,15 +155,15 @@ class TestQueueIO(unittest.TestCase):
|
||||
now=datetime(2026, 5, 25, 14, 0, 0, tzinfo=timezone.utc),
|
||||
)
|
||||
# Write in reverse order.
|
||||
write_proposal(b)
|
||||
write_proposal(a)
|
||||
ordered = list_pending_proposals(self.slug)
|
||||
_SV.write_proposal(b)
|
||||
_SV.write_proposal(a)
|
||||
ordered = _SV.list_pending_proposals(self.slug)
|
||||
self.assertEqual([a.id, b.id], [p.id for p in ordered])
|
||||
|
||||
def test_write_and_read_response(self):
|
||||
r = Response(proposal_id="xyz", status=STATUS_REJECTED, notes="no")
|
||||
write_response(self.slug, r)
|
||||
self.assertEqual(r, read_response(self.slug, "xyz"))
|
||||
_SV.write_response(self.slug, r)
|
||||
self.assertEqual(r, _SV.read_response(self.slug, "xyz"))
|
||||
|
||||
|
||||
class TestAuditLog(unittest.TestCase):
|
||||
@@ -193,15 +189,15 @@ class TestAuditLog(unittest.TestCase):
|
||||
justification="agent needed gh-api token",
|
||||
diff="--- before\n+++ after\n",
|
||||
)
|
||||
path = write_audit_entry(e)
|
||||
path = _SV.write_audit_entry(e)
|
||||
self.assertEqual(host_db_path(), path)
|
||||
self.assertEqual(0o600, path.stat().st_mode & 0o777)
|
||||
loaded = read_audit_entries("cred-proxy", "dev")
|
||||
loaded = _SV.read_audit_entries("cred-proxy", "dev")
|
||||
self.assertEqual([e], loaded)
|
||||
|
||||
def test_appends_one_line_per_entry(self):
|
||||
for i in range(3):
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp=f"2026-05-25T12:00:0{i}+00:00",
|
||||
bottle_slug="dev",
|
||||
component="egress",
|
||||
@@ -210,7 +206,7 @@ class TestAuditLog(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
entries = read_audit_entries("egress", "dev")
|
||||
entries = _SV.read_audit_entries("egress", "dev")
|
||||
self.assertEqual(3, len(entries))
|
||||
self.assertEqual(
|
||||
["2026-05-25T12:00:00+00:00", "2026-05-25T12:00:01+00:00",
|
||||
@@ -219,7 +215,7 @@ class TestAuditLog(unittest.TestCase):
|
||||
)
|
||||
|
||||
def test_separate_logs_per_component_slug(self):
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp="t",
|
||||
bottle_slug="dev",
|
||||
component="cred-proxy",
|
||||
@@ -228,7 +224,7 @@ class TestAuditLog(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp="t",
|
||||
bottle_slug="dev",
|
||||
component="egress",
|
||||
@@ -237,7 +233,7 @@ class TestAuditLog(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp="t",
|
||||
bottle_slug="other",
|
||||
component="cred-proxy",
|
||||
@@ -246,12 +242,12 @@ class TestAuditLog(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
self.assertEqual(1, len(read_audit_entries("cred-proxy", "dev")))
|
||||
self.assertEqual(1, len(read_audit_entries("egress", "dev")))
|
||||
self.assertEqual(1, len(read_audit_entries("cred-proxy", "other")))
|
||||
self.assertEqual(1, len(_SV.read_audit_entries("cred-proxy", "dev")))
|
||||
self.assertEqual(1, len(_SV.read_audit_entries("egress", "dev")))
|
||||
self.assertEqual(1, len(_SV.read_audit_entries("cred-proxy", "other")))
|
||||
|
||||
def test_read_audit_entries_missing_log_returns_empty(self):
|
||||
self.assertEqual([], read_audit_entries("cred-proxy", "no-such-bottle"))
|
||||
self.assertEqual([], _SV.read_audit_entries("cred-proxy", "no-such-bottle"))
|
||||
|
||||
|
||||
class TestDiffAndHash(unittest.TestCase):
|
||||
|
||||
@@ -17,14 +17,12 @@ from bot_bottle.orchestrator.supervisor import (
|
||||
AuditEntry,
|
||||
Proposal,
|
||||
STATUS_APPROVED,
|
||||
Supervisor,
|
||||
TOOL_EGRESS_ALLOW,
|
||||
list_pending_proposals,
|
||||
read_audit_entries,
|
||||
read_proposal,
|
||||
read_response,
|
||||
write_audit_entry,
|
||||
)
|
||||
|
||||
_SV = Supervisor()
|
||||
|
||||
|
||||
def _proposal() -> Proposal:
|
||||
return Proposal.new(
|
||||
@@ -47,21 +45,21 @@ class TestReadMalformed(unittest.TestCase):
|
||||
with patch.dict("os.environ", {"HOME": d}):
|
||||
QueueStore("slug").migrate()
|
||||
with self.assertRaises(FileNotFoundError):
|
||||
read_proposal("slug", "p")
|
||||
_SV.read_proposal("slug", "p")
|
||||
|
||||
def test_read_response_missing_row(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as d:
|
||||
with patch.dict("os.environ", {"HOME": d}):
|
||||
QueueStore("slug").migrate()
|
||||
with self.assertRaises(FileNotFoundError):
|
||||
read_response("slug", "p")
|
||||
_SV.read_response("slug", "p")
|
||||
|
||||
def test_list_pending_reads_db_only(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as d:
|
||||
with patch.dict("os.environ", {"HOME": d}):
|
||||
QueueStore("slug").migrate()
|
||||
supervise.write_proposal(_proposal())
|
||||
pending = list_pending_proposals("slug")
|
||||
_SV.write_proposal(_proposal())
|
||||
pending = _SV.list_pending_proposals("slug")
|
||||
self.assertEqual(1, len(pending))
|
||||
self.assertEqual("slug", pending[0].bottle_slug)
|
||||
|
||||
@@ -70,26 +68,26 @@ class TestReadMalformed(unittest.TestCase):
|
||||
with patch.dict("os.environ", {"HOME": d}):
|
||||
QueueStore("slug").migrate()
|
||||
p = _proposal()
|
||||
supervise.write_proposal(p)
|
||||
supervise.write_response("slug", supervise.Response(
|
||||
_SV.write_proposal(p)
|
||||
_SV.write_response("slug", supervise.Response(
|
||||
proposal_id=p.id,
|
||||
status=STATUS_APPROVED,
|
||||
notes="",
|
||||
))
|
||||
self.assertEqual([], list_pending_proposals("slug"))
|
||||
self.assertEqual([], _SV.list_pending_proposals("slug"))
|
||||
|
||||
|
||||
class TestReadAuditEntries(unittest.TestCase):
|
||||
def test_missing_log_returns_empty(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as home, \
|
||||
patch.dict("os.environ", {"HOME": home}):
|
||||
self.assertEqual([], read_audit_entries("egress", "nope"))
|
||||
self.assertEqual([], _SV.read_audit_entries("egress", "nope"))
|
||||
|
||||
def test_reads_entries_from_db(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as home, \
|
||||
patch.dict("os.environ", {"HOME": home}):
|
||||
AuditStore().migrate()
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp="t",
|
||||
bottle_slug="slug",
|
||||
component="egress",
|
||||
@@ -98,7 +96,7 @@ class TestReadAuditEntries(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
write_audit_entry(AuditEntry(
|
||||
_SV.write_audit_entry(AuditEntry(
|
||||
timestamp="t",
|
||||
bottle_slug="other",
|
||||
component="egress",
|
||||
@@ -107,7 +105,7 @@ class TestReadAuditEntries(unittest.TestCase):
|
||||
justification="",
|
||||
diff="",
|
||||
))
|
||||
entries = read_audit_entries("egress", "slug")
|
||||
entries = _SV.read_audit_entries("egress", "slug")
|
||||
self.assertEqual(1, len(entries))
|
||||
self.assertEqual("approve", entries[0].operator_action)
|
||||
|
||||
|
||||
@@ -50,6 +50,11 @@ from bot_bottle.gateway.supervise_server import (
|
||||
validate_proposed_file,
|
||||
)
|
||||
|
||||
# The orchestrator's supervise service, backing the fake resolver's queue reads
|
||||
# and writes (the fake stands in for the RPC; state still goes through the real
|
||||
# queue store).
|
||||
_SV = _sv.Supervisor()
|
||||
|
||||
# Fixed caller identity for the handler tests. The control plane attributes by
|
||||
# (source_ip, identity_token); the fake resolver ignores them and answers for a
|
||||
# fixed bottle, since attribution itself is covered by the orchestrator tests.
|
||||
@@ -89,7 +94,7 @@ class _FakeSuperviseResolver:
|
||||
bottle_slug=self.bottle_id, tool=tool, proposed_file=proposed_file,
|
||||
justification=justification, current_file_hash=_sv.sha256_hex(proposed_file),
|
||||
)
|
||||
_sv.write_proposal(proposal)
|
||||
_SV.write_proposal(proposal)
|
||||
return proposal.id
|
||||
|
||||
def poll_supervise(
|
||||
@@ -101,10 +106,10 @@ class _FakeSuperviseResolver:
|
||||
if self.bottle_id is None or self.poll_none:
|
||||
return None
|
||||
try:
|
||||
response = _sv.read_response(self.bottle_id, proposal_id)
|
||||
response = _SV.read_response(self.bottle_id, proposal_id)
|
||||
except FileNotFoundError:
|
||||
try:
|
||||
_sv.read_proposal(self.bottle_id, proposal_id)
|
||||
_SV.read_proposal(self.bottle_id, proposal_id)
|
||||
except FileNotFoundError:
|
||||
return {"status": _sv.POLL_STATUS_UNKNOWN}
|
||||
return {"status": _sv.POLL_STATUS_PENDING}
|
||||
@@ -377,10 +382,10 @@ class TestHandleToolsCall(unittest.TestCase):
|
||||
matching response — the operator half, out of band."""
|
||||
def runner():
|
||||
for _ in range(200):
|
||||
pending = _sv.list_pending_proposals("dev")
|
||||
pending = _SV.list_pending_proposals("dev")
|
||||
if pending:
|
||||
p = pending[0]
|
||||
_sv.write_response("dev", _sv.Response(
|
||||
_SV.write_response("dev", _sv.Response(
|
||||
proposal_id=p.id, status=status, notes=notes,
|
||||
))
|
||||
return
|
||||
@@ -487,7 +492,7 @@ class TestHandleToolsCall(unittest.TestCase):
|
||||
responder.join()
|
||||
# A decided proposal drops off the operator's pending list (a response
|
||||
# row exists) — poll itself no longer archives (issue #469 review).
|
||||
self.assertEqual([], _sv.list_pending_proposals("dev"))
|
||||
self.assertEqual([], _SV.list_pending_proposals("dev"))
|
||||
|
||||
def test_pending_response_times_out_without_archive(self):
|
||||
result = _tools_call(
|
||||
@@ -505,7 +510,7 @@ class TestHandleToolsCall(unittest.TestCase):
|
||||
text = result["content"][0]["text"] # type: ignore[index]
|
||||
self.assertIn("status: pending", text)
|
||||
self.assertIn("proposal remains queued", text)
|
||||
self.assertEqual(1, len(_sv.list_pending_proposals("dev")))
|
||||
self.assertEqual(1, len(_SV.list_pending_proposals("dev")))
|
||||
|
||||
_ALLOW: dict[str, object] = {
|
||||
"name": _sv.TOOL_EGRESS_ALLOW,
|
||||
@@ -747,7 +752,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
justification="need example.com",
|
||||
current_file_hash=_sv.sha256_hex(self._ROUTES),
|
||||
)
|
||||
_sv.write_proposal(p)
|
||||
_SV.write_proposal(p)
|
||||
return p
|
||||
|
||||
# --- pending response carries the id ---
|
||||
@@ -771,7 +776,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
self.assertFalse(result["isError"]) # type: ignore[index]
|
||||
text = result["content"][0]["text"] # type: ignore[index]
|
||||
self.assertIn("status: pending", text)
|
||||
pending = _sv.list_pending_proposals("dev")
|
||||
pending = _SV.list_pending_proposals("dev")
|
||||
self.assertEqual(1, len(pending)) # still queued, not archived
|
||||
self.assertIn(pending[0].id, text) # agent got the id to poll
|
||||
|
||||
@@ -779,7 +784,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
|
||||
def test_check_returns_approved_idempotently(self):
|
||||
p = self._seed_proposal()
|
||||
_sv.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_APPROVED, notes="ok"))
|
||||
_SV.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_APPROVED, notes="ok"))
|
||||
result = _check(self.resolver, {"arguments": {"proposal_id": p.id}})
|
||||
self.assertFalse(result["isError"])
|
||||
text = result["content"][0]["text"] # type: ignore[index]
|
||||
@@ -792,7 +797,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
|
||||
def test_check_rejected_sets_isError(self):
|
||||
p = self._seed_proposal()
|
||||
_sv.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_REJECTED, notes="no"))
|
||||
_SV.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_REJECTED, notes="no"))
|
||||
result = _check(self.resolver, {"arguments": {"proposal_id": p.id}})
|
||||
self.assertTrue(result["isError"])
|
||||
self.assertIn("status: rejected", result["content"][0]["text"]) # type: ignore[index]
|
||||
@@ -804,7 +809,7 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
text = result["content"][0]["text"] # type: ignore[index]
|
||||
self.assertIn("status: pending", text)
|
||||
self.assertIn(p.id, text)
|
||||
self.assertEqual(1, len(_sv.list_pending_proposals("dev"))) # not archived
|
||||
self.assertEqual(1, len(_SV.list_pending_proposals("dev"))) # not archived
|
||||
|
||||
def test_check_unknown_id_is_error(self):
|
||||
result = _check(self.resolver, {"arguments": {"proposal_id": "no-such-proposal"}})
|
||||
@@ -848,15 +853,15 @@ class TestNonBlockingSupervise(unittest.TestCase):
|
||||
},
|
||||
ServerConfig(response_timeout_seconds=0.05),
|
||||
)
|
||||
pid = _sv.list_pending_proposals("dev")[0].id
|
||||
pid = _SV.list_pending_proposals("dev")[0].id
|
||||
self.assertIn(pid, result["content"][0]["text"]) # type: ignore[index]
|
||||
# 2. operator decides out-of-band
|
||||
_sv.write_response("dev", _sv.Response(proposal_id=pid, status=_sv.STATUS_APPROVED, notes="ok"))
|
||||
_SV.write_response("dev", _sv.Response(proposal_id=pid, status=_sv.STATUS_APPROVED, notes="ok"))
|
||||
# 3. agent resumes by polling — no re-proposing
|
||||
poll = _check(self.resolver, {"arguments": {"proposal_id": pid}})
|
||||
self.assertFalse(poll["isError"])
|
||||
self.assertIn("status: approved", poll["content"][0]["text"]) # type: ignore[index]
|
||||
self.assertEqual([], _sv.list_pending_proposals("dev")) # resolved (response exists)
|
||||
self.assertEqual([], _SV.list_pending_proposals("dev")) # resolved (response exists)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user