feat(runtime): add tool registry and execution policy boundary - #252
feat(runtime): add tool registry and execution policy boundary#252JoTalbot wants to merge 158 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| def append(self, event: ExecutionAuditEvent) -> ExecutionAuditEvent: | ||
| event = event.with_identity() | ||
| if self._contains(event.event_id): |
There was a problem hiding this comment.
🔥 The Roast: Проверка _contains() и последующая запись в файл — не атомарная операция. Два процесса одновременно видят, что события нет, и оба его дописывают. Это как два кассира, которые одновременно проверяют наличие денег в кассе и оба выписывают чек на одну и ту же сумму — в итоге касса пустая, а чеков два.
🩹 The Fix: Используй файловую блокировку (fcntl.flock или filelock) вокруг проверки и записи, или перейди на sqlite3/sled для аппаратно-атомарных операций.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| def _contains(self, event_id: str) -> bool: | ||
| if not self.path.exists(): | ||
| return False | ||
| return any(json.loads(line).get("event_id") == event_id for line in self.path.read_text(encoding="utf-8").splitlines() if line.strip()) |
There was a problem hiding this comment.
🔥 The Roast: json.loads(line) без обработки ошибок — один битый байт в audit-логе и вся проверка падает с JSONDecodeError. Это как проверять список гостей в ресторане и падать в обморок от одной опечатки в бронировании.
🩹 The Fix: Оборачивай json.loads в try/except и пропускай битые строки (или quarantine-их, как в execution_commit.py).
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| result = [] | ||
| for line in self.path.read_text(encoding="utf-8").splitlines(): | ||
| if line.strip(): | ||
| event = ExecutionAuditEvent(**json.loads(line)) |
There was a problem hiding this comment.
🔥 The Roast: Тот же json.loads без страховки, только теперь в методе events(). Один битый байт — и ты не можешь прочитать НИ ОДНО событие из аудита. Это как сжечь всю библиотеку потому что одна книга испачкана.
🩹 The Fix: Обработай json.JSONDecodeError в цикле, пропуская или quarantine-и повреждённые строки.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raw.setdefault("status", "pending") | ||
| raw.setdefault("sequence", expected) | ||
| commit = ExecutionCommit(**raw) | ||
| if commit.sequence != expected or commit.with_integrity().checksum != commit.checksum: |
There was a problem hiding this comment.
🔥 The Roast: expected увеличивается даже для quarantined-строк. Если в середине журнала есть битая запись, все ПОСЛЕДУЮЩИЕ валидные коммиты тоже попадут в карантин. Один испорченный боек — и вся история после него считается бракованной. Это как наложить цензуру на всю книгу из-за одной грязной страницы.
🩹 The Fix: Увеличивай expected только для валидных коммитов. Quarantine не должен сдвигать счётчик последовательности для последующих строк.
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| registry.register("add", add, permissions={"compute"}) | ||
| audit = ExecutionAudit() | ||
| sandbox = ToolSandbox(registry, audit) | ||
| executor = AgentExecutor(sandbox) |
There was a problem hiding this comment.
🔥 The Roast: Передаёшь ToolSandbox в AgentExecutor, а он ждёт ToolExecutor. Это как попытаться завести машину от пульта от телевизора — форма похожая, но функции совсем другие.
🩹 The Fix:
| executor = AgentExecutor(sandbox) | |
| executor = AgentExecutor(ToolExecutor(sandbox)) |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| result = await orchestrator.run("calculate", "task-1", {"permissions": ["compute"]}) | ||
|
|
||
| assert result.status == "completed" | ||
| assert result.result == [5] |
There was a problem hiding this comment.
🔥 The Roast: AgentExecutor.execute возвращает список ToolResult, а ты проверяешь result == [5]. Это как ожидать пиццу, а получить коробку от неё — формально связано, но голод не утоляется.
🩹 The Fix:
| assert result.result == [5] | |
| assert result[0].value == 5 |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| def _read(self): | ||
| if not self.path.exists(): | ||
| return [] | ||
| return [RecoveryQueueItem(**json.loads(line)) for line in self.path.read_text(encoding="utf-8").splitlines() if line.strip()] |
There was a problem hiding this comment.
🔥 The Roast: json.loads(line) без обработки ошибок — один битый байт в recovery-очереди и весь _read() падает. Операторы больше не смогут ни queue, ни resolve ничего. Это как швейцарскому ножу, который ломается при попытке открыть консерву.
🩹 The Fix: Обработай json.JSONDecodeError при чтении каждой строки и пропускай/карантини повреждённые записи.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @router.post("/retry", dependencies=[Depends(guard)]) | ||
| def retry(execution_id: str): | ||
| try: | ||
| return service.resolve(execution_id, "retry") |
There was a problem hiding this comment.
🔥 The Roast: Эндпоинт /retry пытается resolve записи с action="retry", но в очередь попадают только QUARANTINE и MANUAL_REVIEW. Эндпоинт вечно возвращает false с кодом 200 — как автомат по продаже воды в пустыне, который никогда не выдает товар.
🩹 The Fix: Либо убери эндпоинт /retry, либо измени логику так, чтобы он действительно перезапускал executions (например, через RuntimeBootstrap или RecoveryManager).
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| def _read(self): | ||
| try: | ||
| return json.loads(self.path.read_text(encoding="utf-8")) | ||
| except (FileNotFoundError, json.JSONDecodeError): |
There was a problem hiding this comment.
🔥 The Roast: _read() тихо глотает json.JSONDecodeError и возвращает {}. При повреждении файла все лизы исчезают без предупреждения — как если бы файл сгорел, а ты сказал «ой, ничего не было». Повторяет тот же паттерн, что и в execution_store.py, но для критичного к ресурсам компонента.
🩹 The Fix: Обработай json.JSONDecodeError отдельно от FileNotFoundError и подними исключение, чтобы данные не терялись молча:
| except (FileNotFoundError, json.JSONDecodeError): | |
| def _read(self): | |
| try: | |
| return json.loads(self.path.read_text(encoding="utf-8")) | |
| except FileNotFoundError: | |
| return {} | |
| except json.JSONDecodeError as exc: | |
| raise RuntimeError(f"corrupted lease store: {self.path}") from exc |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if self.store: | ||
| return await self.store.load(execution_id) | ||
| return None | ||
| async def recover(self, loop, agent: Any, context: Optional[dict] = None): |
There was a problem hiding this comment.
🔥 The Roast: Метод recover переименовал сигнатуру из recover(execution_id) в recover(loop, agent, context=None), а runtime/self_healing.py:9 всё ещё зовёт его по-старому с одним аргументом. Это как поменять замок на двери и не выдать ключи жильцам — теперь SelfHealing.inspect() падает с TypeError каждый раз, когда статус не ok.
🩹 The Fix: Обновите вызывающий код: self.recovery_manager.recover(loop, agent, context) или добавьте адаптер с совместимой сигнатурой.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raise | ||
|
|
||
| async def execute(self, goal: str, agent: Any, context: Optional[dict] = None): | ||
| if not self.started: |
There was a problem hiding this comment.
🔥 The Roast: execute() проверяет self.started без всякой синхронизации. Два конкурентных вызова могут одновременно увидеть False и оба запустить start(), в результате recover_with_loop выполнится дважды. Это не guard, а «пожалуйста, не делайте так» записочка на двери.
🩹 The Fix: Добавьте asyncio.Lock или threading.Lock вокруг проверки и установки self.started, либо сделайте start() и execute() атомарными.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| self._transition(state, "retrying", attempt=attempt, error=failed.error) | ||
| plan = await self.planner.create_plan(f"{goal} [replan attempt {attempt + 1}]") | ||
| self._transition(state, "running", attempt=attempt + 1, plan=plan, error=None) | ||
| await self._publish(REPLAN_COMPLETED, execution, {"attempt": attempt + 1, "plan": plan}) |
There was a problem hiding this comment.
🔥 The Roast: На последней итерации цикла, когда attempt < max_attempts, создаётся новый план и рассылается REPLAN_COMPLETED, но цикл сразу заканчивается — план никогда не выполняется. Это как объявить премьеру фильма, который так и не снимут.
🩹 The Fix: Не публикуйте REPLAN_COMPLETED сразу после создания плана; опубликуйте его после успешного выполнения, или переместите публикацию в конец успешной итерации.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| async def run(self, goal: str, agent: Any, context: Optional[dict] = None, execution_context: Optional[ExecutionContext] = None): | ||
| context = dict(context or {}) | ||
| execution = execution_context or ExecutionContext(agent_id=str(getattr(agent, "id", None) or agent), goal=goal, metadata=context) |
There was a problem hiding this comment.
🔥 The Roast: str(getattr(agent, "id", None) or agent) — если agent.id равен 0 или "", выражение or agent подставит весь объект вместо id. Фальшивые идентификаторы демобилизовались.
🩹 The Fix: Используйте явную проверку: agent_id = getattr(agent, "id", None); agent_id = agent_id if agent_id is not None else str(agent).
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| registry.register("add", add, permissions={"compute"}) | ||
| audit = ExecutionAudit() | ||
| sandbox = ToolSandbox(registry, audit) | ||
| executor = AgentExecutor(sandbox) |
There was a problem hiding this comment.
🔥 The Roast: Передаёте ToolSandbox в AgentExecutor, который ждёт ToolExecutor. Это как дать шеф-повару кирпич и попросить суфле — формы похожи, но функция совсем другая. При вызове execute() получится TypeError или KeyError.
🩹 The Fix:
| executor = AgentExecutor(sandbox) | |
| executor = AgentExecutor(ToolExecutor(sandbox)) |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @pytest.mark.asyncio | ||
| async def test_shutdown_is_idempotent(): | ||
| manager = ShutdownManager() | ||
| assert await manager.shutdown() == {"status": "stopped"} |
There was a problem hiding this comment.
🔥 The Roast: Та же проблема: await manager.shutdown() == {"status": "stopped"}. None == dict — это как сравнивать ноль с пирогом.
🩹 The Fix:
| assert await manager.shutdown() == {"status": "stopped"} | |
| await manager.shutdown() | |
| assert await manager.shutdown() is None |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| fastapi = pytest.importorskip("fastapi") | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from api.main import operator_validator |
There was a problem hiding this comment.
🔥 The Roast: from api.main import operator_validator — в api/main.py нет переменной operator_validator, только вызов create_app(operator_validator=authenticate). Импорт упадёт с ImportError ещё до запуска теста.
🩹 The Fix: Импортируйте напрямую:
| from api.main import operator_validator | |
| from api.security import authenticate as operator_validator |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| monkeypatch.setenv("AIOS_OPERATOR_TOKEN", "secret") | ||
| class Request: | ||
| headers = {"authorization": "Bearer secret"} | ||
| assert operator_validator(Request()) is True |
There was a problem hiding this comment.
🔥 The Roast: assert operator_validator(Request()) is True — новая authenticate возвращает SecurityContext, а не True. Объект не тождественен True, тест падает.
🩹 The Fix:
| assert operator_validator(Request()) is True | |
| context = operator_validator(Request()) | |
| assert context is not None | |
| assert context.role == OperatorRole.OPERATOR |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| monkeypatch.setenv("AIOS_OPERATOR_TOKEN", "secret") | ||
| class Request: | ||
| headers = {"authorization": "Bearer wrong"} | ||
| assert operator_validator(Request()) is False |
There was a problem hiding this comment.
🔥 The Roast: assert operator_validator(Request()) is False — при неверном токене authenticate возвращает None, а не False. None is False — это ложь, замаскированная под проверку.
🩹 The Fix:
| assert operator_validator(Request()) is False | |
| context = operator_validator(Request()) | |
| assert context is None |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from .security import OperatorRole, require_role | ||
|
|
||
|
|
||
| def build_operator_audit_router(service, authorize_operator=None): |
There was a problem hiding this comment.
🔥 The Roast: build_operator_audit_router определён, но нигде не импортируется и не используется. Новый api/app.py не подключает этот роутер. Мёртвый код, который только путает.
🩹 The Fix: Либо подключите роутер в create_app, либо удалите файл, если он не нужен.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| fastapi = pytest.importorskip("fastapi") | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from api.main import operator_validator |
There was a problem hiding this comment.
🔥 The Roast: Ты импортируешь operator_validator из api.main, но там его нет — api/main.py определяет только app. Это как просить у соседа молоко, которого он никогда не покупал. Тесты не запустятся вообще.
🩹 The Fix:
| from api.main import operator_validator | |
| from api.security import authenticate as operator_validator |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| await self._publish(EXECUTION_COMPLETED, ctx, {"agent_id": agent_id, "result_count": len(results)}) | ||
| return results | ||
| except Exception as exc: | ||
| await self._publish(EXECUTION_FAILED, ctx, {"agent_id": agent_id, "error": str(exc)}) |
There was a problem hiding this comment.
🔥 The Roast: В except Exception блоке ты вызываешь _publish(EXECUTION_FAILED). Если publish упадёт, raise поднимет исключение публикации, и оригинальное исключение exc навсегда потеряется. Мы будемdebug-ить не ту проблему.
🩹 The Fix: Оберни publish в try/except или вынеси публикациюfailure до raise, сохраняя оригинальное исключение.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raise | ||
| except Exception as exc: | ||
| result = ToolResult.failure(call, exc) | ||
| await self._publish(TOOL_FAILED, ctx, {"tool": call.tool, "call_id": call.call_id, "error": str(exc)}) |
There was a problem hiding this comment.
🔥 The Roast: Та же проблема: _publish(TOOL_FAILED) в except блоке. Если publish упадёт, return result на строке 35 никогда не выполнится, и вызывающий получит исключение публикации вместо осмысленного ToolResult.
🩹 The Fix: Оберни publish в try/except, чтобы return result всегда выполнялся.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| results.append(result) | ||
| if self.memory and hasattr(self.memory, "remember"): | ||
| self.memory.remember({"execution_id": ctx.execution_id, "agent_id": agent_id, "tool": tool, "call_id": call.call_id, "ok": result.ok, "result": result.value, "error": result.error}) | ||
| await self._publish(EXECUTION_COMPLETED, ctx, {"agent_id": agent_id, "result_count": len(results)}) |
There was a problem hiding this comment.
🔥 The Roast: _publish(EXECUTION_COMPLETED) стоит в success path. Если publish упадёт, except поймает это, пометит выполнение как failed, и вычисленные results навсегда потеряются. Успех превратился в провал из-за шумного соседа.
🩹 The Fix: Обработай ошибку publish отдельно, чтобы не терять результаты.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| context["execution_id"] = execution.execution_id | ||
| return OrchestrationResult(goal, task_id, "completed", result, context) | ||
| except Exception as exc: | ||
| await self.events.failed(execution, task_id=task_id, error=str(exc)) |
There was a problem hiding this comment.
🔥 The Roast: await self.events.failed(...) в except Exception блоке. Если event bus упадёт, оригинальное исключение снова потеряется. Оркестратор превращается в магию потери ошибок.
🩹 The Fix: Оберни publish в try/except или используй contextlib.suppress/логирование для publish ошибок.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if not hmac.compare_digest(token[7:], expected): | ||
| return None | ||
| try: | ||
| role = OperatorRole(request.headers.get("x-aios-role", OperatorRole.OPERATOR.value)) |
There was a problem hiding this comment.
🔥 The Roast: При отсутствии заголовка x-aios-role роль по умолчанию — OPERATOR. Любой владелец валидного токена автоматически получает полные права оператора. «Принцип наименьших привилегий» ушёл на обед.
🩹 The Fix: Используй OperatorRole.VIEWER как значение по умолчанию: OperatorRole(request.headers.get("x-aios-role", OperatorRole.VIEWER.value)).
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raise HTTPException(status_code=403, detail="operator role required") | ||
| try: | ||
| return service.resolve(payload.execution_id, payload.action, actor=operator.actor, reason=payload.reason, correlation_id=operator.correlation_id) | ||
| except KeyError as exc: |
There was a problem hiding this comment.
🔥 The Roast: except KeyError — слишком узкая ловушка. service.resolve может выбросить что угодно (битый JSON в очереди, ошибки записи аудита), и всё это улетит как 500 вместо осмысленного ответа.
🩹 The Fix: Лови Exception и возвращай 500 для неизвестных ошибок, либо конкретизируй, какие исключения ожидаются от resolve.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| raise HTTPException(status_code=403, detail="operator role required") | ||
| try: | ||
| return service.resolve(payload.execution_id, "retry", actor=operator.actor, reason=payload.reason, correlation_id=operator.correlation_id) | ||
| except KeyError as exc: |
There was a problem hiding this comment.
🔥 The Roast: Та же узкая except KeyError в /retry эндпоинте. Случайный OSError при записи файла превратится в необработанный 500.
🩹 The Fix: Аналогично строке 61 — расширь обработку исключений.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| def test_recovery_auth_receives_request(): | ||
| app = create_app(operator_validator=lambda request: request.headers.get("x-operator") == "1") |
There was a problem hiding this comment.
🔥 The Roast: operator_validator=lambda request: request.headers.get("x-operator") == "1" возвращает True, а не SecurityContext. Recovery-эндпоинты ждут объект с .role, а получают булево значение. Тест ожидает 200, а получает 403.
🩹 The Fix: Верни SecurityContext из валидатора: operator_validator=lambda request: SecurityContext(actor="test", role=OperatorRole.OPERATOR, correlation_id="test").
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| result = await self._execute_with_retry(call, agent_id, permissions, ctx) | ||
| results.append(result) | ||
| if self.memory and hasattr(self.memory, "remember"): | ||
| self.memory.remember({"execution_id": ctx.execution_id, "agent_id": agent_id, "tool": tool, "call_id": call.call_id, "ok": result.ok, "result": result.value, "error": result.error}) |
There was a problem hiding this comment.
🔥 The Roast: self.memory.remember(...) без обёртки в try/except. Один шумный бэкенд памяти — и весь успешный запуск инструмента превращается в провал. Память не должна держать выполнение в заложниках.
🩹 The Fix: Оберни в try/except или логируй ошибку памяти и продолжай.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except asyncio.CancelledError: | ||
| raise | ||
| except Exception as exc: | ||
| retryable = isinstance(exc, (asyncio.TimeoutError, TimeoutError, ConnectionError, OSError)) |
There was a problem hiding this comment.
🔥 The Roast: Вы классифицируете retryable через isinstance(exc, (asyncio.TimeoutError, TimeoutError, ConnectionError, OSError)). Беда в том, что PermissionError и FileNotFoundError — тоже подклассы OSError. А ToolSandbox.execute бросает именно PermissionError("agent identity is required") (runtime/tool_sandbox.py:23). Итог: отказ по правам помечается retryable=True, и AgentExecutor._execute_with_retry будет биться в ту же закрытую дверь self.retries раз — ровно тот антипаттерн, который мы только что вылечили в agent_executor. Плюс TimeoutError и так подкласс OSError, а asyncio.TimeoutError в 3.11+ — его алиас, так что вся кортеж сводится к одному OSError. Это как лечить аллергию на арахис, разрешив есть все орехи оптом.
🩹 The Fix:
| retryable = isinstance(exc, (asyncio.TimeoutError, TimeoutError, ConnectionError, OSError)) | |
| retryable = isinstance(exc, (TimeoutError, ConnectionError)) and not isinstance(exc, (PermissionError, FileNotFoundError)) |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Reviewed PR #252 against the current vNext execution architecture. The Tool Registry / permission boundary direction is correct. Before merge, the main integration risk is wiring: Scheduler's executor path, AgentExecutor/ToolExecutor contracts, canonical persistence, and RuntimeContext recovery should remain one path. Please ensure no second persistence/execution state is introduced and add an integration regression covering Scheduler → AgentExecutor → ToolRegistry/PermissionBoundary → tool result → persistence. |
JoTalbot
left a comment
JoTalbot
left a comment
There was a problem hiding this comment.
PR #252 is architecturally substantial and currently reports mergeable=false. I reviewed the changed-file set and the execution-context propagation in scheduler/AgentExecutor. The remaining blocker should be resolved before merge: reconcile the branch with current main and run the full integration/security suite. In particular verify one canonical execution_id across Orchestrator → Scheduler → AgentExecutor → ToolExecutor, plus one canonical persistence/recovery path. Do not merge while the PR reports merge conflicts.
|
|
||
| def test_scheduler_executor_contract_accepts_execution_context(): | ||
| from kernel.scheduler import Scheduler | ||
| assert hasattr(Scheduler, "execute") |
There was a problem hiding this comment.
🔥 The Roast: Название теста обещает проверить, что Scheduler.execute «принимает execution_context», а проверка сводится к hasattr(Scheduler, "execute"). Это как поручиться за то, что официант понимает заказ на французском, и убедиться только, что он дышит. Если execute потеряет параметр execution_context, этот тест так и будет зелёный, а в продакшне TypeError встретит оркестратора как старый знакомый.
🩹 The Fix: Замените hasattr на проверку сигнатуры метода. Импортируйте inspect и убедитесь, что параметр execution_context присутствует:
import inspect
assert "execution_context" in inspect.signature(Scheduler.execute).parameters📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Architecture
Advances the vNext runtime toward:
Intent -> Planner -> Scheduler -> Agent -> Tool Registry -> Permission Boundary -> Tool -> Memory -> ReflectionThe sandbox is intentionally a policy boundary, not a claim of OS-level isolation. A stronger process/container sandbox can be introduced behind the same contract later.