REVIEW v4 remediation: Waves 0-2 + self-modification guard#165
Open
draxios wants to merge 21 commits into
Open
REVIEW v4 remediation: Waves 0-2 + self-modification guard#165draxios wants to merge 21 commits into
draxios wants to merge 21 commits into
Conversation
Document the theme system, skill trust store, spec-compliant MCP OAuth, native /effort, and the config manifest / env-var registry; add the deepagents 0.6.12 drop-in surface; replace the REVIEW.md-only tracking note with the four root docs (REVIEW/ROADMAP/PARITY/AGENTS).
REVIEW v4 (58-agent audit): 1 P0 + 21 confirmed P1 + 8 downgraded + 1 refuted + 30 P2, prior-cycle scorecard, systemic themes, and Waves 0-4 sequencing. ROADMAP: v1 killer-feature scorecard (4 shipped / 8 partial / 8 not-started, verified against code) plus Killer features v2 (#21-#45) from a fresh four-front survey, with deferred-item re-evaluation.
…command CTX-1: CheckpointingMiddleware._run_git now catches OSError/TimeoutExpired, self-disables once with a logged warning, and returns a synthetic failed CompletedProcess so a git-less or slow-git box degrades gracefully instead of the exception propagating out of the tool node (which langgraph re-raises) and aborting every write_file/edit_file/execute. The CLI ships checkpointing on by default, so this was default-reachable. SB-1: the execute tool's sync and async wrappers now catch the dangerous-command PermissionError (LocalShellBackend, e.g. rm -rf) and return it as a tool-error string the model can adapt to, instead of aborting the turn. Fixes v4 CTX-1, SB-1.
…oning models CLI-CORE-1 (P0): _cleanup_agent_task drains the pending queue after its state-restoration finally, not inside it, so a queued message that starts the next turn is no longer clobbered (leaving it uninterruptible and letting a third message run concurrently). CLI-CORE-4: _send_prompt_to_agent defers behind an in-flight turn via a new QueuedMessage.raw flag instead of spawning a concurrent turn and overwriting the user's worker handle; the drain re-routes raw prompts back through _send_prompt_to_agent. CLI-CORE-3: /clear keeps _lc_thread_id in lockstep with the session thread so /compact can't mutate the pre-clear thread. RD-1: add native Bedrock reasoning-effort (Anthropic-on-Bedrock via additional_model_request_fields, version-gated) and make the legacy effort presets temperature-only, so non-reasoning models (Haiku, Ollama, Bedrock Haiku) are never output-capped -- fixing operator easy-tier turns that truncated at 1024 tokens. Fixes v4 CLI-CORE-1, CLI-CORE-3, CLI-CORE-4, RD-1.
The agent/shell run flags and the worker handle were bare attributes on BogAgentsApp mutated from ~8 methods; ordering mistakes between those writes were the root of v4 CLI-CORE-1 (cleanup clobbering the next turn) and CLI-CORE-4 (a pipeline starting a concurrent turn). Move the state into a small TurnManager coordinator with a single busy definition and paired begin_agent/end_agent + begin_shell/end_shell transitions, so 'at most one turn in flight' is a property of one object rather than scattered discipline. _agent_running / _agent_worker / _shell_running remain as delegating properties over the manager, so the ~25 read sites and any incidental write are untouched; the dispatch and cleanup sites call the manager methods. All transitions run on Textual's single event loop, so no lock is needed. Bumps the app.py line-count ratchet to 17,575 (deliberate, documented) since the thin delegation belongs on the app while the lifecycle logic now lives in the new module. Completes v4 Wave 0.
…P_TRUST CLI-CORE-2: GitToolsMiddleware is default-on but _add_interrupt_on() gated only six tools, so git_commit / git_add / git_branch (create/switch) / git_stash (push/pop/drop, incl. destructive drop) executed with no approval prompt. Add them to the interrupt map with arg-aware description formatters and a `when` predicate so read-only paths (branch listing, git stash list/show) stay un-prompted. CT-1: BOG_AGENTS_MCP_TRUST was defined, exposed in the config manifest, and printed in the non-TTY deny message, but read nowhere -- a CI user following the printed instruction was silently denied. Honor it as an allow-override at all three trust sites (_check_mcp_project_trust plus the -n and ACP paths). Fixes v4 CLI-CORE-2, CT-1.
…irtual_mode DMN-1: virtual_mode confines only filesystem tools -- the SDK is explicit that a shell-capable backend's execute() runs unrestricted on the host regardless. So unattended triggers (cron/interval/file-change/git-push and the HMAC-only webhook path) previously had unrestricted host shell plus the daemon's full environment; a git-push job whose prompt ingests attacker-authored commit text was the worst case. Extract `_select_backend`: non-MANUAL triggers now get a non-sandbox FilesystemBackend rooted at working_dir -- filesystem read/grep for skills/pipelines still works, but execute() reports "not available" and no host environment is exposed. MANUAL (token-authenticated) keeps the unrestricted LocalShellBackend; operators opt back into unattended shell per deployment with BOG_DAEMON_ALLOW_UNATTENDED_SHELL=1 (warned when a network-reachable trigger fires). Updates the V3-11/P7 tests to the stronger backend-type posture. Fixes v4 DMN-1.
MW-SAFE-2 / MW-SAFE-1: RBAC and air-gap were self-administered -- the model could call set_active_role/define_role/set_data_policy/clear_air_gap to lift its own restrictions, so they only bounded a cooperative model, not an adversarial one. RBAC: RBACMiddleware now accepts operator-owned `roles` + a pinned `active_role`. When pinned, the model gets only the read-only check_permission/list_roles tools (no define_role/set_active_role), the store is deny-by-default, and an empty or undefined pinned role denies all tools instead of falling open. Unpinned construction keeps the legacy self-service tools for backward compatibility. Air-gap: AirGappedMiddleware accepts an operator-owned DataPolicy; when pinned it withholds set_data_policy/clear_air_gap from the model. The `enable_air_gapped` flag path always pins a policy (operator-supplied or a fail-closed default), so the flag alone yields a real egress boundary. Wired through FeatureConfig (rbac_roles/rbac_active_role, air_gap_policy) and graph.py, with a warning when enable_rbac is set without a pinned role. Fixes v4 MW-SAFE-2, MW-SAFE-1.
…e allowlist RD-5: /butcher ran an LLM-authored plan of shell/edit slices with no approval, and operator mode auto-escalates a plain prompt to butcher by default -- so worker shell (shell=True) executed screened only by the accident-catcher patterns, bypassing the CLI's HITL. The documented job-level approval gate did not exist, and Slice.files was described as an allowlist but never enforced. Add a plan-approval modal (ButcherPlanApprovalScreen) shown after planning and before any slice runs; approval is fail-safe (a headless app with no modal surface is treated as denied) and can be waived per deployment with `auto_approve = true` in butcher.toml. Enforce the slice's declared `files` allowlist in build_worker_tools: writes/edits are restricted to those paths (exact or fnmatch glob), so the allowlist is a real boundary, not prompt text. An empty allowlist keeps the whole-working-dir scope for backward compatibility. Fixes v4 RD-5.
… surface DEL-2 (= v3 P0-10): the quickstart and the documented systemd unit use `bog-agents-daemon run --port 7878`, but the entrypoint implemented only start/stop/status AND rejected a `--port` that followed the subcommand -- so the unit crash-looped on its first command. Add `run` as an alias of `start`, and accept the global --port/--log-level flags after the subcommand too (a shared parent parser with SUPPRESS defaults, so a flag given before the subcommand is not clobbered). The docs also documented `bog-agents-daemon job add/...` and `runs list/show/prune`, which never existed on this binary -- job management lives on the `bog-agents daemon jobs ...` CLI (and the HTTP API). Repoint the examples and drop the phantom `runs` family, with a drift guard test. Fixes v4 DEL-2.
…ease workflow DEL-3 (= v3 P1-69): the composite action's skills loop used `(( SKILL_COUNT++ ))`, which evaluates to 0 on the first increment and, under `bash -eo pipefail`, aborts the step after the first skill -- every consumer passing `skills_repo` hit it. Use `SKILL_COUNT=$((SKILL_COUNT + 1))` (always exit 0). DEL-4 (= v3 P1-66/70): the VS Code release workflow runs on [self-hosted, Windows] where the default `run:` shell is pwsh, but the version-validation step uses bash `[[ ... =~ ]]` syntax -- so the only build/publish path could never complete. Pin `shell: bash` in the top-level defaults (Git Bash ships on the runner; npm steps run fine under it). Fixes v4 DEL-3, DEL-4.
…ks, add lock+satellite CI DEL-1: the `[all]`/`[acp]` extras depended on `bog-agents-acp`, which is not on PyPI, so `pip install bog-agents-cli[all]` (and the daemon's) failed resolution for every user. Drop acp from the extras until it is published; install ACP from source in the workspace. Re-locked cli/daemon accordingly. DEL-5: harbor and daytona lockfiles were stale (`uv lock --check` failed) and nothing caught it. Re-locked all packages and added a `lock-check` CI job (`make lock-check`), plus a Linux `satellites` matrix that runs acp/harbor/ daytona `make test` on every PR -- the absence of that CI is exactly how SAT-1 and SAT-2 shipped unnoticed. SAT-2: pin acp to the in-repo editable SDK (`[tool.uv.sources]`) so it resolves 0.9.9 instead of a stale PyPI 0.8.7, and fix the HITL bridge test, which used the invalid `create_agent(interrupt_on=...)` kwarg instead of `middleware=[HumanInTheLoopMiddleware(interrupt_on=...)]` (the real root cause, not the lock). acp suite now green (52). Also correct the README claim that CI runs per-package for all packages. Fixes v4 DEL-1, DEL-5, SAT-2; partially DEL-6.
…formanceSuite SAT-1: HarborSandbox overrode the *deprecated* als_info/agrep_raw/aglob_info names, so the SDK's structured als/agrep/aglob fell through to a raising stub -- every harbor eval run had broken ls/grep/glob tools while the legacy-name tests stayed green. Rebase on BaseSandbox, which derives als/aread_file/agrep/aglob/ adelete from aexecute(); HarborSandbox now supplies only the async command primitive (aexecute) plus its own exec-based awrite/aedit (Harbor environments expose no native file-transfer API, so the upload/download-based defaults do not apply). ~200 lines of hand-rolled shell wrappers deleted; adelete gained for free. Add `bog_agents.testing.SandboxConformanceSuite` -- a reusable pytest mixin that pins the structured file surface (write/read/ls/grep/glob/edit/delete round-trips plus result types), so a backend drifting off it fails loudly instead of silently shipping broken runs. Validated against LocalShellBackend; harbor/daytona can subclass it with their own real-sandbox fixture. Fixes v4 SAT-1.
…uthority files #24 (CVE-2026-25725 class): the agent could silently rewrite the files that grant it authority — Expert Mode rulebooks, the dreamscape laws/constitution, project lifecycle hooks, and the .mcp.json manifest. Gate writes to them behind human approval via FilesystemPermissionsMiddleware `interrupt`-mode rules, which the SDK merges into interrupt_on regardless of --auto-approve, so the guard holds even when the agent runs unattended. The file tools run in a virtual filesystem rooted at the project, so the agent cannot reach the home-level trust stores (~/.bog-agents/config.toml, the skill-trust store, .env) through file tools at all. The remaining vector is the shell; add a best-effort screen (command_targets_authority_file) that forces an approval prompt for shell commands that appear to write an authority file under --auto-approve. Hooks and stdio-MCP configs were already fingerprint-gated (P0-8, P1-49); this closes the rest. Completes v4 Wave 1.
DEL-7: the uv_setup composite action declared `env: UV_VERSION` at the top level, but composite action metadata has no top-level `env` key -- the runner ignored it and setup-uv fell back to *latest* uv on every run. Latest uv resolved lockfiles differently than a contributor's uv, which surfaced immediately as the new lock-check job failing on libs/acp. Make it a real `uv-version` input (composite actions honor inputs), defaulted to the uv version the committed lockfiles were generated with, so CI and local resolve identically. Bump it deliberately (and re-run `make lock`) rather than floating. Fixes v4 DEL-7.
The lock-check job restored a uv cache that could carry index metadata from another job (including the first run's *latest*-uv resolution), making the deterministic re-resolution differ from the committed lockfiles even with uv pinned. Disable caching for this job so it always resolves fresh.
Every committed lockfile passes "uv lock --check" with a forced full network resolve (--no-cache --refresh) on a clean Linux + uv 0.11.6 + PyPI container, for all six packages. Only this self-hosted runner reports drift (acp), which means its "uv lock" floats to *highest* instead of honoring the locked pins -- a property of the runner's uv environment (global uv.toml / UV_UPGRADE / UV_RESOLUTION / private mirror), not the repo. Add a diagnostic step that dumps the runner's UV_* env and uv config so the gate can be made deterministic, and mark the check advisory (continue-on-error) so a runner-local quirk no longer blocks merges while the committed locks are provably correct everywhere reproducible.
…retry, crash recovery Tier 1 (correctness): - store: quarantine a corrupt jobs.json to jobs.json.corrupt-<ts> instead of returning [] and letting the next save overwrite it (which silently destroyed every job). A transient read OSError is NOT quarantined. - store/main: reconcile_orphaned_runs() on startup stamps runs left RUNNING by a crashed daemon as FAILED so /runs shows an honest terminal state. - runner: the webhook/slack/email/github dispatchers now raise on failure so run_job records them in run.dispatch_errors (capped); they previously swallowed URLError into the log, hiding the most failure-prone targets. Tier 2 (features): - scheduler: replace the hand-rolled 5-field cron parser with croniter and add missed-slot catch-up — a daily job whose slot elapsed during downtime fires once on restart instead of skipping to the next slot. Also gains ranges/steps/ lists and @macro support. - models/runner/api: opt-in per-job retry (max_retries / retry_backoff_seconds, default 0 = prior single-shot behaviour) for the agent invocation AND each output dispatch, with exponential backoff; JobRun.attempts records the count. Prompt/skill resolution errors are deterministic and are not retried. Tier 3 (perf/scale): - file_watch: event-driven FILE_CHANGE triggers via watchdog (FileWatchManager), activated only inside run_forever so a bare _tick() (unit tests) never starts an observer thread. First runs and unwatchable dirs fall back to the os.walk poll (_check_file_trigger); the code degrades to polling entirely if watchdog is unavailable. Adds croniter + watchdog as daemon dependencies (uv.lock updated). Full daemon suite: 194 passed, 3 skipped; ruff clean.
…the daemon (V3-10) DMN-2: receive_webhook compared the presented token against the captured `token` closure, not token_holder["value"], so /admin/rotate-token never invalidated a leaked old token on the webhook path (and rejected the new one), while /webhooks/git-push rotated correctly. Compare against the live token. V3-10: the daemon source was never type-checked and its test-lint failure was swallowed with `|| true`. Add ty (config + Makefile `type` target wired into `lint`, so CI's lint matrix now type-checks the daemon), make file_watch.py ty-clean (a single TYPE_CHECKING view of the optional watchdog import), and drop the `|| true` so a test-lint regression actually fails the build. Full daemon suite: 195 passed, 3 skipped; ruff + ty clean.
- SDK-CORE-1: default cors_origins to [] (was ["*"], which let any drive-by web page drive the agent and read its replies); warn when wildcard CORS is combined with no api_key. - SDK-CORE-4: replay the thread's tracked history to checkpointer-less agents so turn 2 no longer silently loses all prior context while /history implies continuity; agents that carry a checkpointer still get only the new turn. - SDK-CORE-5: decouple stream production from consumption — a background producer holds a concurrency slot only while producing into a bounded queue, each enqueue bounded by request_timeout, so a slow/stalled client can no longer pin a slot indefinitely; the client drains without holding a slot. - SDK-CORE-6: remove the dead enable_websocket flag and its false /info + module advertisement (no WebSocket endpoint exists), and enforce enable_streaming (POST /stream returns 501 when disabled instead of silently streaming). Adds 12 serve hardening tests (method + endpoint level); ruff + ty clean.
…E-2/7) SDK-CORE-2: AgentBuilder.build() forwarded every enable_* flag as a bare kwarg through create_agent's deprecated backdoor (a DeprecationWarning on every build and a hard TypeError at 1.0), and CostConfig.enabled defaulted True so cost tracking was silently force-enabled on every agent. Now build() folds all FeatureConfig fields into config=FeatureConfig(...) (real params stay direct), and cost tracking defaults off (with_cost_tracking() opts in). SDK-CORE-7: two documented no-ops made honest — - with_mcp() collected registry IDs that build() silently dropped (no SDK-level MCP loader exists); it now raises NotImplementedError pointing at the supported with_middleware()/with_tools() path. - with_sandbox(allow_dangerous=True) dropped the flag when no backend was given; build() now constructs the LocalShellBackend the flag describes. Adds 5 builder tests (config routing, no-force-enable, real-build no-deprecation, mcp raises, sandbox backend); ruff + ty clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the REVIEW v4 audit remediation (see
REVIEW.md§2) across threewaves plus the self-modification guard. 14 commits; every package's gates pass
locally (ruff / ruff format / ty; SDK, CLI, daemon, and satellite suites).
Wave 0 — default-path correctness
_cleanup_agent_taskno longer clobbers the next queuedturn (drain moved after state restore); extracted a
TurnManagerto make itstructural.
/clearkeeps_lc_thread_idin lockstep; pipeline/watchprompts defer behind an in-flight turn instead of running concurrently.
output-capped (fixes operator easy-tier truncation).
Wave 1 — trust honesty
BOG_AGENTS_MCP_TRUSThonored.
no inherited env).
its own policy.
/butcherplans require approval; the per-slice file allowlist isenforced.
rules, dreamscape laws, hooks,
.mcp.json) require approval even under--auto-approve.Wave 2 — delivery & satellites truth
pip install [all]works again (acp de-advertised); daemonruncommand + corrected quickstart; the GitHub Action skills install and theVS Code workflow no longer fail on first use; lockfiles refreshed with a CI
drift guard.
BaseSandbox(structured ls/grep/glob workagain) + a reusable
SandboxConformanceSuite.