refactor(util): make render_diff generic (caller supplies side titles); move to root util
tracker-policy-pr / check-pr (pull_request) Successful in 8s
test / integration-docker (pull_request) Successful in 36s
test / unit (pull_request) Successful in 43s
lint / lint (push) Successful in 56s
test / integration-firecracker (pull_request) Successful in 3m27s
test / coverage (pull_request) Successful in 21s
test / publish-infra (pull_request) Has been skipped
tracker-policy-pr / check-pr (pull_request) Successful in 8s
test / integration-docker (pull_request) Successful in 36s
test / unit (pull_request) Successful in 43s
lint / lint (push) Successful in 56s
test / integration-firecracker (pull_request) Successful in 3m27s
test / coverage (pull_request) Successful in 21s
test / publish-infra (pull_request) Has been skipped
Generalize render_diff so it isn't supervise-flavored: the "(current)" / "(proposed)" side titles are no longer baked in — callers pass `before_title` and `after_label` (required). Move it from orchestrator/supervisor/util.py to the base bot_bottle.util alongside sha256_hex, and delete the now-empty supervisor/util.py. The supervise callsite (Orchestrator.supervise_respond) assigns before_title="current", after_label="proposed"; the facade no longer re-exports render_diff (callers import from bot_bottle.util). Behavior at the supervise callsite is unchanged. Full unit suite green (2243). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -40,8 +40,8 @@ from .supervisor import (
|
|||||||
TOOL_EGRESS_ALLOW,
|
TOOL_EGRESS_ALLOW,
|
||||||
TOOL_EGRESS_BLOCK,
|
TOOL_EGRESS_BLOCK,
|
||||||
Supervisor,
|
Supervisor,
|
||||||
render_diff,
|
|
||||||
)
|
)
|
||||||
|
from ..util import render_diff
|
||||||
|
|
||||||
|
|
||||||
# Operator decision → Response.status. The apply half (egress tools) runs
|
# Operator decision → Response.status. The apply half (egress tools) runs
|
||||||
@@ -350,7 +350,10 @@ class Orchestrator:
|
|||||||
operator_action=status,
|
operator_action=status,
|
||||||
operator_notes=notes,
|
operator_notes=notes,
|
||||||
justification=proposal.justification,
|
justification=proposal.justification,
|
||||||
diff=render_diff(diff_before, diff_after, label=component),
|
diff=render_diff(
|
||||||
|
diff_before, diff_after, label=component,
|
||||||
|
before_title="current", after_label="proposed",
|
||||||
|
),
|
||||||
))
|
))
|
||||||
return True, ""
|
return True, ""
|
||||||
|
|
||||||
|
|||||||
@@ -7,9 +7,10 @@ object — optionally scoped to a `db_path` — makes the orchestrator's supervi
|
|||||||
dependency explicit and injectable in tests.
|
dependency explicit and injectable in tests.
|
||||||
|
|
||||||
For convenience this package also re-exports the neutral vocabulary
|
For convenience this package also re-exports the neutral vocabulary
|
||||||
(`bot_bottle.supervisor`'s types + `SupervisePlan`) and the pure `render_diff`
|
(`bot_bottle.supervisor`'s types + `SupervisePlan`), so orchestrator-side
|
||||||
helper (`util`), so orchestrator-side callers import from one place. The data
|
callers import from one place. (Generic helpers like `render_diff` /
|
||||||
plane must NOT import this package — it imports
|
`sha256_hex` live in `bot_bottle.util`.) The data plane must NOT import this
|
||||||
|
package — it imports
|
||||||
`bot_bottle.supervisor` (neutral) directly and reaches the queue over the
|
`bot_bottle.supervisor` (neutral) directly and reaches the queue over the
|
||||||
control-plane RPC.
|
control-plane RPC.
|
||||||
"""
|
"""
|
||||||
@@ -24,7 +25,6 @@ from ..store.store_manager import StoreManager
|
|||||||
from ..store.queue_store import QueueStore
|
from ..store.queue_store import QueueStore
|
||||||
from ...store.audit_store import AuditStore
|
from ...store.audit_store import AuditStore
|
||||||
from ...supervisor.types import AuditEntry, Proposal, Response
|
from ...supervisor.types import AuditEntry, Proposal, Response
|
||||||
from .util import render_diff # noqa: F401 — re-exported for callers
|
|
||||||
|
|
||||||
|
|
||||||
class Supervisor:
|
class Supervisor:
|
||||||
|
|||||||
@@ -1,26 +0,0 @@
|
|||||||
"""Pure supervise helpers — diff rendering.
|
|
||||||
|
|
||||||
Stateless utility used alongside the `Supervisor` service. Kept as a plain
|
|
||||||
function (not a method) because it carries no state. (Content hashing is a
|
|
||||||
generic helper — `bot_bottle.util.sha256_hex`.)
|
|
||||||
"""
|
|
||||||
|
|
||||||
from __future__ import annotations
|
|
||||||
|
|
||||||
import difflib
|
|
||||||
|
|
||||||
|
|
||||||
def render_diff(before: str, after: str, *, label: str = "config") -> str:
|
|
||||||
"""Unified diff suitable for the audit log + TUI. Empty diff (no changes)
|
|
||||||
renders as the empty string."""
|
|
||||||
diff = difflib.unified_diff(
|
|
||||||
before.splitlines(keepends=True),
|
|
||||||
after.splitlines(keepends=True),
|
|
||||||
fromfile=f"{label} (current)",
|
|
||||||
tofile=f"{label} (proposed)",
|
|
||||||
lineterm="",
|
|
||||||
)
|
|
||||||
parts = list(diff)
|
|
||||||
if not parts:
|
|
||||||
return ""
|
|
||||||
return "".join(p if p.endswith("\n") else p + "\n" for p in parts).rstrip("\n")
|
|
||||||
@@ -5,6 +5,7 @@ level deeper, under their backend package."""
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import difflib
|
||||||
import hashlib
|
import hashlib
|
||||||
import ipaddress
|
import ipaddress
|
||||||
import os
|
import os
|
||||||
@@ -16,6 +17,31 @@ def sha256_hex(content: str) -> str:
|
|||||||
return hashlib.sha256(content.encode("utf-8")).hexdigest()
|
return hashlib.sha256(content.encode("utf-8")).hexdigest()
|
||||||
|
|
||||||
|
|
||||||
|
def render_diff(
|
||||||
|
before: str,
|
||||||
|
after: str,
|
||||||
|
*,
|
||||||
|
label: str = "config",
|
||||||
|
before_title: str,
|
||||||
|
after_label: str,
|
||||||
|
) -> str:
|
||||||
|
"""Unified diff of `before` vs `after`, with the two sides headed
|
||||||
|
`{label} ({before_title})` and `{label} ({after_label})`. Empty diff (no
|
||||||
|
changes) renders as the empty string. The side titles are the caller's —
|
||||||
|
e.g. "current"/"proposed" for a supervise proposal."""
|
||||||
|
diff = difflib.unified_diff(
|
||||||
|
before.splitlines(keepends=True),
|
||||||
|
after.splitlines(keepends=True),
|
||||||
|
fromfile=f"{label} ({before_title})",
|
||||||
|
tofile=f"{label} ({after_label})",
|
||||||
|
lineterm="",
|
||||||
|
)
|
||||||
|
parts = list(diff)
|
||||||
|
if not parts:
|
||||||
|
return ""
|
||||||
|
return "".join(p if p.endswith("\n") else p + "\n" for p in parts).rstrip("\n")
|
||||||
|
|
||||||
|
|
||||||
def is_ip_literal(value: str) -> bool:
|
def is_ip_literal(value: str) -> bool:
|
||||||
try:
|
try:
|
||||||
ipaddress.ip_address(value)
|
ipaddress.ip_address(value)
|
||||||
|
|||||||
@@ -7,7 +7,7 @@ from pathlib import Path
|
|||||||
|
|
||||||
from bot_bottle.orchestrator import supervisor as supervise
|
from bot_bottle.orchestrator import supervisor as supervise
|
||||||
from bot_bottle.paths import host_db_path
|
from bot_bottle.paths import host_db_path
|
||||||
from bot_bottle.util import sha256_hex
|
from bot_bottle.util import render_diff, sha256_hex
|
||||||
from tests.unit import use_bottle_root
|
from tests.unit import use_bottle_root
|
||||||
from bot_bottle.store.audit_store import AuditStore
|
from bot_bottle.store.audit_store import AuditStore
|
||||||
from bot_bottle.orchestrator.store.queue_store import QueueStore
|
from bot_bottle.orchestrator.store.queue_store import QueueStore
|
||||||
@@ -21,7 +21,6 @@ from bot_bottle.orchestrator.supervisor import (
|
|||||||
Supervisor,
|
Supervisor,
|
||||||
TOOL_EGRESS_ALLOW,
|
TOOL_EGRESS_ALLOW,
|
||||||
TOOL_GITLEAKS_ALLOW,
|
TOOL_GITLEAKS_ALLOW,
|
||||||
render_diff,
|
|
||||||
)
|
)
|
||||||
|
|
||||||
_SV = Supervisor()
|
_SV = Supervisor()
|
||||||
@@ -249,10 +248,10 @@ class TestAuditLog(unittest.TestCase):
|
|||||||
|
|
||||||
class TestDiffAndHash(unittest.TestCase):
|
class TestDiffAndHash(unittest.TestCase):
|
||||||
def test_render_diff_returns_empty_when_unchanged(self):
|
def test_render_diff_returns_empty_when_unchanged(self):
|
||||||
self.assertEqual("", render_diff("a\nb\n", "a\nb\n"))
|
self.assertEqual("", render_diff("a\nb\n", "a\nb\n", before_title="current", after_label="proposed"))
|
||||||
|
|
||||||
def test_render_diff_shows_changes(self):
|
def test_render_diff_shows_changes(self):
|
||||||
diff = render_diff("a\nb\nc\n", "a\nB\nc\n", label="routes.yaml")
|
diff = render_diff("a\nb\nc\n", "a\nB\nc\n", label="routes.yaml", before_title="current", after_label="proposed")
|
||||||
self.assertIn("routes.yaml (current)", diff)
|
self.assertIn("routes.yaml (current)", diff)
|
||||||
self.assertIn("routes.yaml (proposed)", diff)
|
self.assertIn("routes.yaml (proposed)", diff)
|
||||||
self.assertIn("-b", diff)
|
self.assertIn("-b", diff)
|
||||||
|
|||||||
Reference in New Issue
Block a user