feat(decisions): add the decision platform core, not yet wired

[fanhongy]

Refs #810. This is the second of three PRs for the decision platform's first slice. It adds the platform, but nothing calls it yet, so no launch changes. The third PR wires it into assign, handoff and run steps.

What does run on merge: at startup the server builds the decision engine and sweeps unfinished decision rows, the migration adds the decision_records table, the existing cleanup purges decision records past retention, and the CLI and ops MCP controls are available. With both points off, none of this changes a launch.

#810 asks CAO to choose a model size for a delegation when the caller doesn't name one. This PR builds the parts that choose, record and control that choice. They live in CAO core, with no vendor names. The decider that answers is a plugin.

What

• Two decision points:

  • model.route asks small, medium or large;
  • effort.route asks low, medium or high.

  Both also accept unsure. Each point is off, shadow or on, and both are off by default.

  • off asks nothing and writes nothing.
  • shadow records what the decider would pick, without waiting for it or changing the launch.
  • on waits up to on_timeout_ms (default 1000). It uses an answer whose confidence is at least the threshold (default 0.70). Otherwise it falls back to the profile default, then the policy default, then the provider's own default.
  • An explicit caller value always wins, and the decider is not asked.

• Deciders:

  • Packages register under the new cao.deciders entry-point group.
  • A decider is loaded only when an active point needs it.
  • The built-in fixed_table looks up a tier by profile, then by role.
  • Nothing in this PR makes an external call.

• Policy:

  • PolicyBounds caps a decider's answer at a ceiling and refuses an explicit value above it.
  • The top-level model_tiers setting maps each provider's tiers to model IDs.
  • A required tier with no mapping refuses the launch, naming model_tiers.<provider>.<tier>. Every provider listed in allowed_providers is checked, including one with no model_tiers entry.
  • Each refusal records a fixed reason, taken from the policy error that raised it. The wiring PR names that set as POLICY_REJECTION_REASONS and tests that it is closed.

• Records:

  • A new decision_records table holds one row per point per launch in shadow or on. init_db's existing create_all adds it, with its indexes, to new and existing databases.
  • Rows never hold the task message, description, purpose, prompts or working directory.
  • The message appears only as a keyed HMAC. The key is decision-hash.key, mode 0600, and can be rotated. A key file with group or other access is restricted when it is read; if that fails, it isn't used, and a warning names it.
  • Records are kept for 90 days by default, separately from terminal records. The existing cleanup removes older ones.
  • A startup sweep marks unfinished decision work as interrupted.

• Shadow runner: bounded at 4 concurrent and 64 pending tasks. Overflow is recorded as shadow_dropped.

• Telemetry:

  • One cao.decision span per record, plus cao.decision.requests and cao.decision.latency metrics.
  • No message, hash or probabilities are exported.
  • Metric labels never contain terminal IDs.

• Operator controls:

  • cao decisions, with status, set, tier, table, exclude, tune, list and purge;
  • cao-server --decision <point>=<state>;
  • the CAO_DECISION_* environment variables;
  • six ops MCP tools.

  All of them are local and in-process, with no HTTP routes. The agent-facing MCP server gets none of them, and boundary tests enforce that.

  • tune refuses values outside its ranges (on timeout 50–5000 ms, threshold 0–1, retention 1–36500 days), and --decision refuses an unknown point or state.
  • --decision sets the matching CAO_DECISION_* variable. The server captures these overrides at startup, so set <point> off can't stop an overridden point until a restart, and there is no all-points off yet. A later [Feat] Decision points: opt-in, auditable judgments in orchestration, starting with Auto model routing #810 PR adds cao decisions off, which beats the overrides on a running server.

• The launch seam: prepare_launch and bind_launch in decisions/engine.py, plus launch_note/render_note, which format the result note the wiring PR adds. No handler calls any of them in this PR.

• Also:

  • script_step_of is extracted in the script runner, so workflow-step detection has one implementation.
  • The TUI catalog lists the eight new commands as hidden.
  • docs/decisions.md documents the platform, with a link from the README.
  • decisions/targets.py: profile_source reports a name in feat(ephemeral): create and store ephemeral agents, not yet launchable #881's reserved ephemeral namespace as an ephemeral target, by name alone, as feat(ephemeral): create and store ephemeral agents, not yet launchable #881's contract test requires.
  • CHANGELOG: an entry under Unreleased / Added.

Tests

• test/decisions/ (new) covers:
  • the engine and the policy;
  • settings precedence, accepted ranges and clamping;
  • the registry and the fixed table;
  • the store, the migration, the sweep and retention;
  • hashing and key rotation, including a two-process race;
  • the shadow runner and telemetry, where each record emits once;
  • the operator controls.
• test/fixtures/decision_conformance.py holds reusable behaviour cases. This PR runs them against the engine directly. The wiring PR runs the same cases through the delegation handlers.
• Boundary:
  • test/fixtures/decision_boundary.py scans the agent-facing MCP surface for decision vocabulary, including with a plugin installed.
  • An AST guard rejects every form of decision import there.
  • test_source_tokens_are_neutral keeps integration-specific names out of src/.
  • On Python 3.10, typing.get_type_hints wraps a parameter that defaults to None in one extra Optional, so workflow_resume's decisions input renders as a nested anyOf. The boundary test unwraps only that exact shape, and only when the result equals the pinned base. Any other shape fails the unchanged comparison.

Local results at b2f319dc:

• full suite on CPython 3.12: 13,454 passed, 235 more than main. The one failure, test_kimi_code_compat.py::TestA41TrustBound::test_socket_is_skipped, also fails on main locally because of a long AF_UNIX temp path, so CI decides that one;
• full suite on CPython 3.10: 13,453 passed, with the same single failure. Compared with 3.12, it has one extra skip: a test/services/test_secret_gate.py check that needs Unicode 15.0 tables, and 3.10 ships 13.0.0;
• CPython 3.11: test/decisions/, test/mcp_server/ and test/api/, 1,917 passed;
• coverage: 92.67%;
• mypy: the same errors as main, none new;
• black, isort, the diff check, markdown links and both agent-plugin checks: clean;
• Rust: fmt, clippy and all tests pass;
• gitleaks 8.30.1 with the repo config, over this PR's commits: no leaks.

Review fixes, now f69a66ae after the rebase onto fd5113ea (first pushed as 9ca0f653):

• test/decisions/: 270 passed on CPython 3.10 and 3.12, and again with CAO_HOME_DIR unset;
• the CLI, ops MCP, telemetry, cleanup, clients and command-catalog tests pass, apart from one environment-only failure that also fails at b2f319dc;
• black, isort and mypy: clean on the changed files;
• each new test fails with its fix reverted;
• the wiring PR's commit still applies cleanly on top, and its decision, MCP and API tests pass there.

Rebase and contract fix at cba5d575:

• the rebase was clean, and the tree before the new commit equals CI's merge of 9ca0f653 with fd5113ea;
• test/decisions/, test/services/test_ephemeral_service.py and the agent-profile tests: 479 passed on CPython 3.12, including test_future_profile_source_contract, which fails with the old stub;
• black, isort and mypy: clean on the changed files.

Second review round at ee4d97e3 (four commits on cba5d575):

• test/decisions/ and test/services/test_ephemeral_service.py: 405 passed on CPython 3.10 and 3.12, and again with CAO_HOME_DIR unset;
• the clients, cleanup, ops MCP, CLI, API, MCP server, utils, telemetry and command-catalog tests: 4,026 passed on CPython 3.12;
• black, isort, mypy and the markdown link check: clean;
• each new test fails with its fix reverted, and an independent review of the first three commits found no P1 or P2 issues;
• the wiring PR's commit applies on top with the same single conflict it already has with main (utils/orchestration.py, from feat(ephemeral): create and store ephemeral agents, not yet launchable #881); its validate change is now identical on both sides.

What comes next

The last PR, opened after this one merges, wires the seam into assign, handoff, elastic assign and run steps. Both points stay off by default.

🤖 Generated with Claude Code

[gutosantos82]

PR Review: #863 — feat(decisions): add the decision platform core, not yet wired

Summary

Adds the vendor-neutral launch-decision platform (two points model.route/effort.route, off/shadow/on, pluggable cao.deciders, PolicyBounds, model_tiers, an additive decision_records table with HMAC-only message correlation, shadow runner, telemetry, and operator-only controls). The seam (prepare_launch/bind_launch) is confirmed uncalled by any handler, both points default off, and the agent-facing MCP boundary is enforced by an executable test. The new test suite (235 tests) passes in a clean environment and black/isort/mypy are clean on the changed files. Recommendation: merge-ready after small fixes; nothing found is blocking, but the Important items below are cheap to address in this PR and one is a real (latent) logic error.

Important (should fix)

• [correctness] src/cli_agent_orchestrator/decisions/engine.py prepare_launch — excluded is derived solely from model.route's exclude_profiles, yet it gates every eligible point: eligible = tuple(point for point, value in fields.items() if value == "auto" and not excluded and ...). A profile pinned out of model routing silently loses effort.route decisions too. Latent today (only model.route is populated in fields) but wrong as soon as effort routing is wired. Fix: apply per-point, e.g. not (point == "model.route" and excluded).
• [consistency][verifier] PR body ↔ code — the body says refusal reasons are "a closed set, POLICY_REJECTION_REASONS". No such symbol exists anywhere in src/, test/ or docs/ (verified by grep and dir(policy)). The de-facto set is scattered class-level reason attributes in policy.py (above_ceiling, default_unmapped, explicit_unmapped, model_override_not_allowed, policy_invalid, provider_not_allowed); the only named frozenset is FALLBACK_REASONS (engine.py:48). Either add the constant (and a test asserting closure) or fix the description.
• [consistency] docs/decisions.md / decisions/settings.py:216 / api/main.py:1332 — the doc presents cao-server --decision flags and CAO_DECISION_* env vars as independent precedence layers, but apply_flags implements the flag by writing the env var, and the production loader (lambda: load_settings(environment=decision_environment)) never passes flags=. So load_settings's flags parameter is dead in production (tests only) and the documented layering model does not match the mechanism. Outcome is the same today, but either pass flags= through or describe the single mechanism honestly.
• [correctness] decisions/store.py shadow completion vs bind() — if a shadow decider finishes before bind_launch is called, update_decision emits the span/metric with launch_status="pending" and leaves the row in _shadow_rows; the subsequent bind() then discards it and deliberately does not re-emit. Fast shadow decisions therefore export a wrong terminal launch_status. Observability-only, but it defeats the "one accurate span per record" claim.
• [correctness][verifier] clients/database.py:3086 _migrate_decision_records — redundant with Base.metadata.create_all (DecisionRecordModel is a Base subclass, so the table and indexes are already created), and unlike every sibling migrator it re-raises, so a failure would abort init_db() and server start. Drop it or make it log-and-continue like _migrate_add_handoff_results.
• [security] decider plugin trust boundary under-documented — engine.decision_request() passes the redacted task message, profile description and purpose into decider.decide(), and cao.deciders entry points are arbitrary code executed at load() before shape validation runs. "Content-free" holds for the DB rows (verified: no message/description/purpose/prompt/cwd columns) but not for the decider channel. Not a defect — inherent to entry-point plugins — but docs/decisions.md should say that an installed decider sees redacted task content and is trusted like any pip dependency.
• [tests] test/fixtures/decision_boundary.py AST guard — assert_no_decision_imports walks only ast.Import/ast.ImportFrom; a lazy importlib.import_module("...decisions...") or __import__ inside a tool handler passes untouched, and test_edges::test_ast_guard_catches_all_decision_import_forms only covers the four static forms. The runtime sys.modules check catches import-time leakage but not function-level lazy imports. Add detection (the repo already models this in services/script_lint.py) or explicitly document the guard's scope.
• [conversation] PR body framing — "nothing calls it yet, so no launch changes" understates what is live on merge: api/main.py:1316-1337 unconditionally constructs DecisionStore/DeciderRegistry/ShadowRunner/DecisionEngine at startup, runs decision_store.sweep() against the DB on every boot, and the migration adds a table. Low blast radius with points off, but the body should say so.

Nits (optional)

• [correctness] decisions/store.py purge(rotate=True) — the DELETE commits inside the session but rotate_key() unlinks the key outside the transaction; a record inserted in that window is hashed with the old key and survives --all --rotate-key with a permanently unverifiable hash. Tiny window, same-user tool; document that rotation is race-free only with no launches in flight.
• [correctness] decisions/engine.py prepare_launch — the pre-loop for point, fb in fallbacks.items(): if isinstance(fb, MissingTier): reject(...) always raises, so the later in-loop MissingTier check on the same object is unreachable.
• [correctness] decisions/store.py sweep() — per-row commit() runs synchronously inside lifespan (not offloaded like cleanup_old_data); fine for a few crash leftovers, a large backlog blocks boot.
• [correctness] services/script_runner.py:446,520,796 — script_step_of validates with run_registry.get(run_id) and each caller re-fetches with run_registry[run_id]; no await between them so it is safe, but returning the record from script_step_of would remove the double lookup. (Verified behaviour-identical to the old inline guard over 35 inputs.)
• [verifier] test/decisions/test_telemetry.py:9-32 — the export fixture yields zero spans on any host with OTEL_SDK_DISABLED=true in the environment (6 failures here until unset). Add monkeypatch.delenv("OTEL_SDK_DISABLED", raising=False).
• [verifier] cli/commands/decisions.py — cao decisions set model.route on --decider nosuch_decider returns {"success": true}; the error surfaces only at launch as a decider_unavailable fallback (cached until restart). Consider validating against the registry, or document the tolerance as docs/decisions.md:74 does for exclude_profiles.
• [security] decisions/hashing.py — _read_key only checks len == 32 and accepts a pre-existing world-readable key file; mkdir(mode=0o700, exist_ok=True) leaves a pre-existing looser DB_DIR mode alone, and _restrict_db_file_permissions does not cover decision-hash.key. Suggest an explicit chmod on both.
• [consistency] cli/commands/decisions.py, services/model_tiers.py:11 — option vocabularies (off/shadow/on, small/medium/large) are hardcoded as Choice(...) and TIERS rather than derived from PointState/POINTS; silent-drift risk.
• [consistency] decisions/engine.py:493 — launch_note/render_note is a third uncalled public export of the seam; the body names only prepare_launch/bind_launch.
• [consistency] decisions/targets.py:7 — profile_source is a stub that always returns INSTALLED, so the whole EPHEMERAL branch of prepare_launch is unreachable in this PR (tests pass target_kind=EPHEMERAL explicitly). Fine while deferred; worth a comment.
• [tests] — test_key_two_process_publish_race and test_boundary_with_installed_plugin spawn subprocesses but are not marked slow, so -m "not slow" won't deselect them. Also _honors (engine.py:78) real provider dispatch has no coverage (every test injects honors_model=lambda), and the production lifespan assembly in api/main.py is untested — both should be entry criteria for the wiring PR.
• [conventions] tui/src/catalog.rs — the eight CommandId::Decisions* variants are alphabetical in DISPLAY_ORDER but placed above the "Top-level leaves" comment in the enum in a different order (Status, Set, Tier, Table, Exclude, Tune, List, Purge). Cosmetic; tests guard correctness.
• [conventions] services/model_tiers.py — lives in services/ though only the decisions package uses it; confirm the "shared reader" placement is intentional.
• [consistency] docs/decisions.md allowed_providers — the author-acknowledged gap is confirmed: PolicyBounds.validate() skips any tableless provider regardless of allowed_providers. Unreachable today (only the identity policy ships). Consider hedging the doc sentence until the wiring PR lands the fix.

Tests

Coverage is unusually thorough. Every PR-body claim maps to an assertion-rich test: additive/idempotent migration (3 indexes asserted), startup sweep → interrupted, 90-day retention via cleanup_old_data, key rotation under a real two-process os.link barrier plus thread race, shadow overflow → shadow_dropped, telemetry once-per-record using the real OTel SDK with in-memory exporters and an allowlist test excluding hash/terminal-id/probabilities, and a 7-shape parametrised test that the extracted script_step_of agrees with all three recorders. Conventions (@pytest.mark.asyncio, tmp_path, env cleanup) are followed. Gaps are all at the not-yet-wired seam: the production lifespan assembly, real _honors provider dispatch, and the AST guard's blindness to dynamic imports. The conformance adapter's launched_model assertions are partly tautological (the adapter supplies the fallback itself), but the record-level assertions carry the weight.

Verification

Baseline (verifier, isolated worktree, pip install -e ., OTEL_SDK_DISABLED unset): test/decisions 235 passed, 0 failed. Related existing tests (test_script_runner, test_cleanup_service, test_database, test/ops_mcp_server, run-step/replay tests) 521 passed, 1 failed — test_cleanup_service.py::test_cleanup_old_data_retains_grok_row_when_provider_cleanup_is_deferred fails identically on merge-base 95a0975c → pre-existing. black/isort clean on all 33 changed .py files; mypy "no issues" on the 18 new/changed source files.

• ✓ VERIFIED cao decisions status runs; both points off, on_timeout_ms 1000, threshold 0.7, retention 90, shadow 4/64; all 8 subcommands present; invalid state/point rejected by Click (exit 2).
• ✓ VERIFIED HMAC key created 0600 (parent 0700) via O_CREAT|O_EXCL, published by os.link, no temp leftovers; purge(rotate=True) unlinks and the key regenerates lazily.
• ✓ VERIFIED PolicyBounds.check_explicit raises PolicyViolation(reason="above_ceiling") for large above ceiling medium; cap returns ("medium", True); unmapped explicit → explicit_unmapped. ✗ REFUTED that these reasons live in a POLICY_REJECTION_REASONS constant — no such symbol exists.
• ✓ VERIFIED agent-facing MCP server imports nothing from decisions (grep + import probe after list_tools(): 30 tools, zero cli_agent_orchestrator.decisions* modules loaded; git diff --stat on mcp_server/ is empty).
• ✓ VERIFIED additive migration: fresh init_db() creates decision_records (30 columns, 3 indexes, DB mode 600); idempotent on second call; recreates after DROP TABLE with the other 23 tables intact.
• ✓ VERIFIED script_step_of identical to the old inline logic over 35 inputs (None/{}/MappingProxyType, run_id × step_id shapes, record subclass) — zero mismatches.
• ✓ VERIFIED startup with both points off creates only the .db — no key file, no settings.json, fixed_table not imported. Engine ON with unknown decider → fallback/decider_unavailable; ON with empty fixed_table → fallback/no_answer; record carries message_hash + message_bytes, literal task text absent.
• ✓ VERIFIED all six ops MCP tools register and reject invalid input with structured {"success": false, ...}.
• ⁇ NOT VERIFIED TUI crate compile (host rustc 1.85.1 < required 1.88). Static cross-check: live Click tree yields 103 leaf commands, 8 under decisions, matching COMMAND_COUNT = 103 and the eight new CommandId variants.

Verdict

Approve with nits — the platform is inert by default, the security and boundary claims hold under executable verification, and tests are strong; please fix the latent exclude_profiles→effort.route gating, add or remove the POLICY_REJECTION_REASONS reference, and reconcile the --decision/env precedence doc with the implementation.

[fanhongy]

Update: CI fixes (force-pushed 3f2782e1 → b2f319dc)

Two checks failed on the previous head: Unit Tests (3.10) and gitleaks. Both came from this PR's test code. The fixes are test-only; nothing under src/ changed, and both commit messages are unchanged.

1. Unit Tests (3.10): test/decisions/test_operator.py::test_boundary_with_installed_plugin

• Cause. On Python 3.10 and earlier, typing.get_type_hints wraps a parameter that defaults to None in an extra Optional; 3.11 dropped that. So on 3.10, workflow_resume's decisions input renders as a nested anyOf, while test/fixtures/decision_boundary_base.json pins the flat shape that 3.11 and later produce.
• Fix. The test unwraps only that exact wrapper: outer keys exactly anyOf and default, members exactly [inner, {"type": "null"}], and inner keys exactly anyOf and description. It unwraps only when the result equals the pinned base. Any other shape reaches the unchanged equality check and fails. The base JSON is byte-identical, and on 3.11 and 3.12 the branch is never entered.
• Mutation-checked on 3.10 and 3.12. Each of these still fails: a changed description, extra inner or outer keys, an extra or non-null anyOf member, swapped members, and a description change made in src/. With the normaliser removed, 3.10 fails and 3.12 passes.

2. gitleaks: a generic-api-key false positive at test/fixtures/decision_conformance.py:189

• The rule matched key", emit=self.spans.append in a one-line DecisionStore(...) call. That call is now one argument per line, with a trailing comma so black keeps it that way. There's no allowlist entry, and no .gitleaks.toml or .gitleaksignore change.
• The fix is folded into the S1b commit itself, not added on top. CI's gitleaks scans every commit in the PR range, so the original line would otherwise still be scanned.

3. CHANGELOG

• main added an entry at the top of ### Added (ci: add fork-aware CodeQL analysis (#857) #858), in the same place as this PR's entry, so they conflicted. The text of the entry is unchanged. It now sits directly below the earlier [Feat] Decision points: opt-in, auditable judgments in orchestration, starting with Auto model routing #810 entry, and the PR merges cleanly.

git diff 3f2782e1 b2f319dc touches only test/decisions/test_operator.py (+20/−2), test/fixtures/decision_conformance.py (+3/−1) and the move in CHANGELOG.md. The local results in the description now include 3.10 and 3.11. All 41 CI checks pass.

🤖 Generated with Claude Code

[fanhongy]

Independent automated review commissioned by the PR author and performed by a separate CAO review agent. These findings are the agent's assessment, not the author's manual self-review.

Review of #863 at b2f319dc814dcdbadac16e0a240841bc3010d76c

Summary

This PR adds the decision platform core: points, policy, settings, deciders, the record store, shadow execution, telemetry and operator controls. As the PR says, nothing in src/ calls prepare_launch or bind_launch yet, so worker launches don't change. I focused on the parts that are live after merge:

• cao decisions;
• the six ops MCP tools;
• cao-server --decision;
• the decision_records migration;
• the startup sweep and the retention purge in cleanup_old_data;
• the script_step_of refactor;
• the TUI catalog.

The agent-facing MCP server gains nothing. Before this PR, the ops MCP process already imported clients.database, so the new tools add no import-time side effect. The script_runner refactor behaves the same as before.

There are no P1 findings. I found one P2, in retention handling, and one P3, in test maintainability.

Findings

P2: cao decisions tune --retention-days 0 (or a negative value) is accepted and becomes a one-day purge at startup

• Where:

  • src/cli_agent_orchestrator/cli/commands/decisions.py:73: --retention-days is an unbounded int.
  • src/cli_agent_orchestrator/decisions/settings.py:198-213: tune saves the value without validating it.
  • src/cli_agent_orchestrator/decisions/settings.py:106: load_settings clamps it to 1–36500 without a warning.
  • src/cli_agent_orchestrator/services/cleanup_service.py:107-116: at every server start, records with created_at < now - retention_days are deleted.

• Scenario: An operator who wants to keep decision records indefinitely runs cao decisions tune --retention-days 0. CAO's existing retention settings follow that convention: services/workflow_retention.py:117-127 and :260-270 (from the PR #526 review) say that 0 disables the bound, because reading 0 as "cut off now" "wiped every run on boot". Here, the command exits 0 and saves 0. load_settings() then returns retention_days == 1, and the next cao-server start deletes every decision record older than one day. docs/decisions.md documents the clamps for on_timeout_ms and the threshold, but not for retention.

• Evidence (isolated HOME/CAO_HOME_DIR; the probe calls the real CLI command, then cleanup_old_data()):

  tune -5 exit 0 | saved: -5 | effective: 1
  tune 0 exit 0 | saved: 0 | effective: 1
  rows before cleanup: 3        # records aged 2, 30 and 80 days
  rows after startup cleanup: 0
  

• Impact: Once the wiring PR writes records, a single command meant to keep the audit trail deletes all but the last day of it at the next restart, with no error. The value is saved now, so it carries over to that release.

• Fix: Pick one of these:

  • Reject out-of-range values where they enter: type=click.IntRange(1, 36500) on --retention-days, plus a ValueError in settings.tune for direct callers.
  • Support 0 as "keep forever" (skip the purge in cleanup_service), to match workflow_retention.

  In both cases, log a warning when a saved value is clamped, document the retention range in docs/decisions.md, and add a test for tune --retention-days 0 / -1.

P3: The agent-boundary scan rejects common words in every agent tool and pins the full descriptions of unrelated tools

• Where:

  • test/fixtures/decision_boundary.py:10-19: VOCABULARY includes points, tiers and decisions.
  • test/fixtures/decision_boundary.py:22-46: the scan.
  • test/fixtures/decision_boundary_base.json.
  • These are used by test/decisions/test_operator.py:71-94 and :176-212 against the live agent MCP tool list.

• Scenario: A later, unrelated PR may:

  • add an agent MCP tool whose text says "data points" or "pricing tiers"; or
  • edit one word of the memory_store or workflow_resume description.

  Either change fails test/decisions, although no decision surface changed. When I ran assert_agent_boundary on a tool described as "Plot data points for the run.", it raised AssertionError: ('x', 'points'). Changing memory_store's "Store a" to "Store one" also raised AssertionError.

• Fix: Limit the vocabulary to identifiers specific to decisions: model.route, effort.route, model_tiers, exclude_profiles, cao.deciders and decisions_* tool names. Check tool names and input property names instead of description text, and keep the AST import guard as the main control. The verbatim base pins for memory_store and workflow_resume can then go.

Notes (not findings)

• PR description: The body says refusal reasons are "a closed set, POLICY_REJECTION_REASONS". git grep POLICY_REJECTION_REASONS finds no match in the tree. The reasons exist only as reason class attributes in decisions/policy.py, so either correct the description or add and test the constant.
• Latent behaviours to settle before the wiring PR. I haven't counted these as findings, because no production code reaches them in this PR:
  • on points are awaited one after another (decisions/engine.py:280-305). Two on points can therefore block a launch for about 2 × on_timeout_ms, while the docs say "waits up to the configured timeout".
  • prepare_launch lets profile-loader ValueError/RuntimeError (engine.py:124-131) and the unknown-provider ValueError from _honors (engine.py:78-79) escape. These are not DecisionInputError.
  • The allowed_providers validation gap at policy.py:115, which the PR acknowledges.
  • --decision checks the point but not the state (settings.py:216-221). --decision model.route=ON starts the server with the point off, and the only signal is a warning in the log.
  • The MissingTier branch at engine.py:298-302 is unreachable, because engine.py:205-210 already rejects every MissingTier.

Validation

• Read metadata.json, diff.patch, commits.txt and context.md. The checkout HEAD is b2f319dc814dcdbadac16e0a240841bc3010d76c, and I reviewed the diff against merge-base 95a0975c (40 files, +5448/−43).
• pytest on CPython 3.12 (--no-cov -p no:cacheprovider): 494 passed. Suites: test/decisions, test/services/test_cleanup_service.py, test/services/test_script_runner.py, test/ops_mcp_server, test/test_command_catalog_matches_click.py, test/test_http_only_boundary.py and test/clients/test_database.py.
• pytest on CPython 3.10, test/decisions plus test/services/test_cleanup_service.py: 247 passed.
• mypy on the new or changed modules (decisions/, model_tiers.py, decision_tools.py, cli/commands/decisions.py, cleanup_service.py): no issues in 18 files. black --check on the new packages and fixtures: clean.
• Probes, run with an isolated HOME/CAO_HOME_DIR:
  • the retention probe above;
  • the boundary-scan probe above;
  • a base-vs-head comparison of ops MCP import side effects (clients.database was already imported on base).
• CI on the head is all green, per the PR metadata. These conclusions don't depend on it.
• The checkout was not modified (git status is clean).

[haofeif]

Review at b2f319dc (merge-base 95a0975c): P3 only, so COMMENT.

CI is 41/41 green, and the branch merges cleanly with current main fd5113ea, which includes #881 (no settings, ephemeral-policy or boundary-vocabulary conflicts). Nothing calls prepare_launch/bind_launch yet. What goes live on merge is the startup block, the retention purge, the CLI and ops-MCP controls, and the new table. I reviewed those and the engine seam.

P3 (inline)

1. The operator kill switch can't override --decision or CAO_DECISION_* on a running server, and there is no all-points off (#810 acceptance) — decisions/settings.py L72.
2. Unguarded OpenTelemetry import on the cao-server startup path — decisions/telemetry.py L5.
3. tune --retention-days 0/negative is saved, then silently clamped to 1 day — cli/commands/decisions.py L73. I agree with the commissioned review on this one. I rate it P3, not P2, only because no decision record can be written until the wiring PR. Please fix it before that PR lands.
4. model.route.exclude_profiles also suppresses effort.route — decisions/engine.py L212. I agree with @gutosantos82 here.

I also agree with these, without separate threads: the PR body's closed set POLICY_REJECTION_REASONS doesn't exist anywhere in the tree, so either add it or fix the body. --decision doesn't validate the state; a typo fails safe to off but is silent at startup.

Checked and fine: _honors (provider_class and honors_model are classmethods), _migrate_decision_records (checkfirst=True, idempotent), sweep() (guards its own errors), and the TUI rows (Hidden, no routes).

[fanhongy]

Thanks @haofeif. I pushed 9ca0f653 on top of b2f319dc; each thread has the details.

• P3 1, the kill switch: documented. A restart is needed while an override is set, and there's no all-points off yet. A later [Feat] Decision points: opt-in, auditable judgments in orchestration, starting with Auto model routing #810 PR adds cao decisions off, which beats the overrides on a running server.
• P3 2, the OpenTelemetry import: guarded, and a no-op without OpenTelemetry.
• P3 3, the ranges: tune now refuses out-of-range values for all three settings, and the docs state the ranges.
• P3 4, exclude_profiles: now applies to model.route only, with a test.
• POLICY_REJECTION_REASONS: the body no longer claims it. The reasons are the fixed reason values of the policy errors, and the wiring PR adds the named set with a closure test.
• --decision: an unknown point or state now stops startup with a usage error. No flag is applied unless all of them are valid.

This also covers @gutosantos82's three asks: the exclude_profiles gating, the POLICY_REJECTION_REASONS reference and the precedence docs. The PR body is updated to match, and it now also says what runs on merge.

🤖 Generated with Claude Code

[fanhongy]

I rebased this branch onto main fd5113ea and force-pushed it. The head is now cba5d575.

• Why: CI on 9ca0f653 failed test/services/test_ephemeral_service.py::test_future_profile_source_contract[routing] and [agreement].

  • feat(ephemeral): create and store ephemeral agents, not yet launchable #881 added that contract test after this PR's last green run. It stays skipped until decisions/targets.py exists, then requires profile_source to classify a reserved ephemeral name as ephemeral, by name alone.
  • The function it needs, routes_to_ephemeral_store, exists only on main, hence the rebase.

• The old and new SHAs:

  • e8d848bb → 44946dc3;
  • b2f319dc → e90cdef8;
  • 9ca0f653 → f69a66ae. These are the review fixes my replies above cite, with the content unchanged.

  The rebase was clean. The tree before the new commit equals CI's merge of 9ca0f653 with fd5113ea.

• The new commit, cba5d575: profile_source returns EPHEMERAL for a name in the reserved namespace, without reading a profile, and INSTALLED otherwise.

• Dependency Security fails on this PR because of CVE-2026-102422 in shell-quote 1.10.0, in docusaurus/package-lock.json. That file comes from main; this PR doesn't touch it.

🤖 Generated with Claude Code

[gutosantos82]

PR Review: #863 — feat(decisions): add the decision platform core, not yet wired

Summary

Re-review at head cba5d575 (previous heads 3f2782e1, b2f319dc). Two things changed since the last head: commit f69a66ae answers maintainer @haofeif's four P3 threads — click.IntRange/FloatRange plus a shared-constant _check_range in settings.tune() (rejects out-of-range, bool, float-for-int and non-finite values), a two-pass validate-then-apply in apply_flags, the exclude_profiles gate scoped to model.route only, and an ImportError-guarded OpenTelemetry import that degrades to a no-op only when opentelemetry* itself is missing — and commit cba5d575 rebases onto main fd5113ea (#881) and makes targets.profile_source classify #881's reserved ephemeral names as EPHEMERAL by name alone, which is exactly what #881's test_future_profile_source_contract requires. Every claimed fix holds on code inspection, the new tests cover both layers (settings and CLI) and the full shadow/on × delegation/workflow exclusion matrix, and the PR body no longer makes the false POLICY_REJECTION_REASONS claim or understates what runs at startup. CI is 41/42 green; the single failure (Dependency Security, CVE-2026-102422 shell-quote in docusaurus/package-lock.json) is unrelated — this PR touches no docusaurus/ path. The platform remains inert by default (both points off, no production caller of prepare_launch/bind_launch), the agent-facing MCP boundary and redaction properties check out, and what remains is cheap hardening and doc work. Merge recommendation: approve with nits.

Important (should fix)

• [correctness] src/cli_agent_orchestrator/clients/database.py:3266-3272 _migrate_decision_records ↩︎ — still catches then re-raises, unlike every sibling migrator (_migrate_add_handoff_results ~860, _migrate_workflow_plan_approval ~1192 both log-and-continue), and it is redundant with Base.metadata.create_all (DecisionRecordModel is a Base subclass). A transient failure here aborts init_db() and server start. Drop it or log-and-continue. Introduced.
• [correctness] decisions/store.py:106-124 update_decision / :162-171 bind() + shadow.py ↩︎ — a shadow decision that completes before bind_launch emits its span with launch_status="pending", and bind() then deliberately skips re-emit for rows still in _shadow_rows, so fast shadow decisions export a permanently wrong terminal launch_status. Observability-only, but it defeats the one-accurate-span-per-record claim. Introduced.
• [security] decisions/hashing.py:41-60 _read_key / clients/database.py:804-824 _restrict_db_file_permissions ↩︎ — key creation is correct (O_CREAT|O_EXCL, 0o600, fsync, link-publish), but _read_key validates only len == 32 and accepts a pre-existing key file regardless of mode, and _restrict_db_file_permissions chmods the sqlite file + -wal/-shm but not decision-hash.key. Mitigated by the 0o700 DB_DIR; still, one explicit os.chmod(key_path, 0o600) closes it. Introduced.
• [security] docs/decisions.md:166-176 decider-package trust boundary ↩︎ (softened) — the doc now says the same user installs deciders and that external deciders must not expose decision access via agent-tool hooks, but still never states that a cao.deciders entry point is arbitrary code executed at entry.load()/cls() (registry.py:26-32) before shape validation (:35-44) and is trusted like any pip dependency. Note the load is lazy — DeciderRegistry() at boot (api/main.py:1343) enumerates entry-point names only, and third-party code runs only when an active on/shadow point first needs that decider — so with points off no plugin code ever runs; one sentence saying both things would close it. Introduced.
• [consistency] docs/decisions.md:74-76 allowed_providers ↩︎ — "Providers with no table are not checked … unless listed in allowed_providers" is still wrong: PolicyBounds.validate() (policy.py) continues past any tableless provider even when listed. The PR body acknowledges this as a known gap deferred to the wiring PR; shipping a knowingly inaccurate sentence is cheap to avoid — hedge it now. Unreachable with the identity policy today. Introduced.
• [consistency] decisions/settings.py:69-71, 85 load_settings(flags=) 🆕 — after the doc rewrite the flags parameter is fully dead in production: the lifespan calls load_settings(environment=…) (api/main.py:1339, 1353), apply_flags writes os.environ (settings.py:238-241), and no documented precedence layer corresponds to it — yet it still outranks environment and is exercised only by test_foundation.py:111. Either drop it or mark it test-only so the next reader does not assume a third precedence layer. Introduced.

Nits (optional)

New this round:

• [tests] test/decisions/test_checkpoint.py:510 test_tune_accepts_range_bounds 🆕 — unlike its sibling at :478, it does not pre-create SETTINGS_FILE before calling tune(); if _load_or_raise ever refuses a missing file this test fails for the wrong reason. Pre-create it, or assert the missing-file behaviour explicitly.
• [tests] test/decisions/test_edges.py:158 test_exclusion_pins_only_model_route 🆕 — covers the INSTALLED target only; no case asserting exclusion is a no-op for an EPHEMERAL target (where agent_profile is not a fact). Minor.
• [correctness] cli/commands/decisions.py:71-73 vs settings.py:202-234 🆕 — nan slips past click.FloatRange (nan comparisons are false) and is caught by _check_range → ClickException (exit 1), while inf is caught by Click (exit 2). Behaviour is correct and tested (test_cli_tune_refuses_non_finite_threshold); only the exit-code asymmetry is worth a comment.
• [consistency] test/fixtures/decision_boundary_base.json + decision_boundary.py:10-39 🆕 — the golden pins verbatim descriptions and the workflow_resume.decisions schema of two unrelated agent tools, and VOCABULARY includes generic words (points, tiers, decisions). A future reword of memory_store/workflow_resume, or any new agent tool mentioning "data points", breaks test/decisions with no decision surface changed. Legitimate fixture, real cross-package coupling — consider scoping the vocabulary or documenting the coupling.
• [consistency] decisions/settings.py:116-118 🆕 — shadow limits still use inline literals (1/1024, 0/100000, 50/60000) while the tunables now use the named *_RANGE constants. Stylistic asymmetry only; the tunables themselves are single-sourced across CLI, tune() and the load-time clamp (good).

Carried over, unchanged:

• [conventions] CODEBASE.md:26-52 ↩︎ — the package map still omits the new top-level src/cli_agent_orchestrator/decisions/ (14 modules) although it lists its peers; the repo's own documentation-maintenance rule puts package ownership here. One table row.
• [conventions] docs/configuration.md:356-417 ↩︎ — the CAO_* env-var index does not mention or link CAO_DECISION_*; its generic "CLI flag > env > settings.json" chain also does not hold for decisions (flag and env are one layer). A pointer plus caveat suffices.
• [consistency] cli/commands/decisions.py:33, 42 ↩︎ (partially improved) — numeric ranges are now derived from shared constants 🆕, but Choice(("off","shadow","on")) and Choice(("small","medium","large")) remain hardcoded rather than derived from PointState / model_tiers.TIERS — while apply_flags (settings.py:238) derives the same state set from PointState a few lines away.
• [consistency] decisions/engine.py:471, 503, 507 ↩︎ — launch_note/render_note are an uncalled third public seam the PR body does not name (it names only prepare_launch/bind_launch).
• [correctness] decisions/store.py sweep() / api/main.py:1342 ↩︎ — per-row commit() runs synchronously inside the async lifespan; a large interrupted-row backlog blocks boot.
• [correctness] decisions/engine.py:204-210 vs ~292 ↩︎ — the pre-loop MissingTier reject always raises, so the in-loop MissingTier re-check is unreachable. Also, fallbacks are computed for all fields, so an off point with an unmapped default tier would still refuse the launch once wired.
• [correctness] decisions/engine.py:118-125 ↩︎ — the profile load catches only FileNotFoundError; a malformed installed profile raising ValueError/RuntimeError escapes prepare_launch as a non-DecisionInputError.
• [correctness] decisions/store.py purge(rotate=True) ↩︎ — DELETE commits inside the session but rotate_key() unlinks outside it; documented as race-free only with no launches in flight.
• [tests] test/decisions/test_telemetry.py:7-33 export fixture ↩︎ — no monkeypatch.delenv("OTEL_SDK_DISABLED", raising=False); with that variable set in the host env the span/metric count assertions fail (fail, not false-pass).
• [tests] test/fixtures/decision_boundary.py assert_no_decision_imports ↩︎ — AST-only (ast.Import/ast.ImportFrom); a lazy importlib.import_module/__import__ passes untouched. Document the scope or extend (cf. services/script_lint.py).
• [conventions] services/model_tiers.py ↩︎ — lives in services/ though only decisions/settings.py:13 consumes it; confirm intent.
• [conventions] tui/src/catalog.rs:239-246 vs :336-343 ↩︎ — CommandId::Decisions* enum order differs from the generated DISPLAY_ORDER; cosmetic, test-guarded.

Tests

Strong this round. Every fix ships with targeted tests: test_exclusion_pins_only_model_route (test_edges.py:158) covers the full shadow/on × delegation/workflow_step matrix and proves model.route yields neither a question nor a record while effort.route is still asked (or scoped out with out_of_scope); the two OTel guard tests (test_telemetry.py:189, 205) genuinely exercise both branches — sys.modules["opentelemetry"] = None for the real ImportError/no-op path, and a planted broken opentelemetry/__init__ to prove a different missing dependency is re-raised, not masked; the range validation is tested at the settings layer (12 params including 0, -5, 36501, nan, inf, True, with the file asserted byte-unchanged) and at the CLI layer (exit 2 with Click's range text, settings file never created, plus the nan→exit-1 case); flag validation is tested both via apply_flags (no env mutated on a bad flag) and end-to-end (cao-server --decision model.route=onn exits 2). test_operator.py:59-60 asserts profile_source(None) == INSTALLED and profile_source("Reviewer-audit_logs-3f9a") == EPHEMERAL, so the EPHEMERAL branch is now reached by test rather than only via explicit target_kind. Hygiene is good (autouse isolated_settings, tmp_path, explicit subprocess env, monkeypatch-reverted sys.modules). Remaining gaps are unchanged and all sit at the not-yet-wired seam (production lifespan assembly, real _honors dispatch) plus the OTEL_SDK_DISABLED fixture nit and the AST-guard blindness to dynamic imports. The earlier suggestion to mark subprocess tests @pytest.mark.slow is withdrawn: the repo registers no slow marker anywhere, so there is no convention to follow.

Verification

Dynamic verification did not return before synthesis. Static evidence at this head: CI 41/42 SUCCESS, the one failure being Dependency Security on shell-quote in docusaurus/package-lock.json (no docusaurus/ path in the 40-file diff — unrelated); the merge-base against upstream main is fd5113ea and the local diff stat matches gh pr diff exactly (40 files, +5726/−43); git diff HEAD~2 HEAD confirms the two new commits touch only decisions/, cli/commands/decisions.py, docs/decisions.md and test/decisions/ (11 files, ~315 lines). The verifier's in-progress probe for the OTel-absent path was observed passing (import ok; OTEL_AVAILABLE = False, emit_record no-op, DecisionStore init OK with no opentelemetry.* modules loaded), but its full report — change-selected test run, the #881 contract test, tune/flag CLI probes, agent-boundary import probe, key-file mode — had not arrived. Prior-head dynamic results (235/235 test/decisions, HMAC key 0600/O_EXCL, idempotent additive migration, agent-MCP import probe clean) apply to the unchanged parts of src/.

Verdict

Approve with nits — the two new commits do exactly what the author claims: the three code fixes for the maintainer's P3 threads are correct and well-tested, the kill-switch behaviour is now accurately documented pending a later all-points-off command, and the rebase onto #881 classifies reserved ephemeral names correctly by name alone. The platform stays inert by default with verified agent-boundary and redaction properties. The remaining Important items are all cheap and worth landing in this PR: make _migrate_decision_records non-fatal like its siblings, chmod the HMAC key file, add the one-sentence plugin-trust note, hedge the allowed_providers sentence, and drop or label the now-dead load_settings(flags=).

[fanhongy]

Thanks @gutosantos82. I pushed four commits on cba5d575: 56a66c80 (fixes), 07c5baad (refactor), cd6e63a2 (docs and tests) and ee4d97e3 (follow-ups from an independent review of those three).

Important

• _migrate_decision_records: removed. You're right that it's redundant: DecisionRecordModel is a Base subclass, and init_db runs create_all before it, which creates the table and its indexes on existing databases too. Its tests now cover create_all. One correction: it wasn't the only migrator that re-raises. _migrate_memory_source_kind, _migrate_memory_scope_null_uniqueness and the three _migrate_vault_* do too. Removing it changes no startup failure mode, because create_all isn't guarded either.
• Shadow span vs bind: documented, with no code change. launch_status isn't a span attribute (see telemetry.FIELDS) or a metric label, so no wrong terminal status is exported. What a shadow decision that finishes before bind does lack is gen_ai.request.model and model_honored. The design defines that attribute as the launched model "when known", and the record still gets both at bind. docs/decisions.md now says so.
• The key file mode: fixed. _read_key restricts a key file with group or other access to 0600 on the open descriptor. If that fails, it raises InsecureKeyError: the key isn't used, no record is written, and a warning names the file. The docs say how to recover. Tests cover a 0644 key, a 0600 key (no chmod) and a key that can't be restricted.
• Plugin trust: documented. A decider package is trusted code, like any other installed dependency. CAO imports and constructs it before checking what it declares, and with every point off no decider code runs.
• allowed_providers: fixed rather than hedged. The one-line validate fix and its two tests moved here from the wiring PR, so the existing sentence is now true. The "Known gap" section is gone from the body.
• load_settings(flags=): removed. The tests pass environment= instead.

Nits, done

• Code:
  • The CLI's state and tier choices come from PointState and TIERS.
  • The shadow limits use named ranges.
  • A comment explains nan (exit 1) versus inf (exit 2).
  • The unreachable in-loop MissingTier refusal is gone.
• Tests:
  • The export fixture clears OTEL_SDK_DISABLED; I reproduced the 5 failures with it set.
  • The import guard also rejects importlib.import_module and __import__ calls with a literal decision module name: absolute or relative, positional or name=, including fromlist entries. Its docstring says what neither check covers: a computed name, or a lazy import inside a function that hasn't run.
  • The real init_db upgrade test now asserts that decision_records and its indexes are created on an old database.
  • The boundary fixture documents its deliberate coupling to unrelated tools.
  • New cases cover an excluded name on an ephemeral target and tune with a pre-created settings file.
• Docs:
  • Key rotation should run with no launches in flight.
  • CODEBASE.md lists decisions/.
  • docs/configuration.md points to the CAO_DECISION_* variables and notes that the flag and the variable are one layer.
  • The PR body names launch_note/render_note.

Nits, not changed

• sweep() on the event loop: the backlog is bounded by the previous process's in-flight work, at most max_pending shadow rows (64 by default) plus unbound launches, and every boot clears it.
• An off point's unmapped default tier refusing the launch: intended. Policy checks don't depend on point state, just as for ephemeral targets, where they run with every point off. An installed launch with every point off returns before any check.
• Profile load catching only FileNotFoundError: this mirrors the create path. create_terminal also catches only FileNotFoundError around load_launch_profile, which for installed names calls the same load_agent_profile. So a malformed profile raises the same error from both, just earlier here. Catching it would ask a decider and write a record for a launch that can't start. I'll add a test pinning this to the wiring PR.
• services/model_tiers.py: deliberate. It reads settings.json the way settings_service does, and explicit tiers in ephemeral specs are its planned second consumer. Today decisions/ is its only consumer.
• The TUI enum order: cosmetic and test-guarded, and DISPLAY_ORDER is generated, so I left it.

Local results are in the updated body. An independent review of the first three commits found no P1 or P2 issues; ee4d97e3 addresses its nits.

🤖 Generated with Claude Code

[gutosantos82]

PR Review: #863 — feat(decisions): add the decision platform core, not yet wired

Summary

Re-review at head ee4d97e3 (previous heads 3f2782e1, b2f319dc, cba5d575). Four commits since the last reviewed head (56a66c80 fixes, 07c5baad refactor, cd6e63a2 docs/tests, ee4d97e3 follow-ups; 17 files, +254/−56) respond to our previous comment. Every claimed fix holds on independent read of the worktree: the redundant re-raising _migrate_decision_records is gone and Base.metadata.create_all is proven (by a real init_db upgrade test) to add decision_records plus its three indexes to an existing database; _read_key now fstats and fchmods the open descriptor to 0600 when any group/other bit is set and raises InsecureKeyError on failure, which DecisionStore.insert catches so no record is written and a warning names the file; PolicyBounds.validate now checks every provider listed in allowed_providers including one with no model_tiers entry, so the documentation sentence that was previously false is now true; the dead load_settings(flags=) layer is removed; CLI choice sets and shadow limits are single-sourced; the unreachable in-loop MissingTier branch is removed; and the plugin-trust, shadow-span-timing and key-handling paragraphs in docs/decisions.md match the code. The platform remains inert by default (both points off, nothing calls prepare_launch/bind_launch), the agent-facing MCP boundary is enforced by a now-wider import guard, CI is 42/42 at this head, and the PR merges cleanly. Recommendation: approve with nits.

Important (should fix)

None introduced this round. All six Important items from our review at cba5d575 are resolved (see Prior feedback).

Nits (optional)

New this round:

• [security] src/cli_agent_orchestrator/decisions/hashing.py:59 🆕 — the read path uses path.open("rb"), which follows symlinks, so the new fchmod would tighten a symlink target. DB_DIR is owner-only 0700 and chmod needs ownership, so this is same-user/self-inflicted rather than an attack surface; os.open(..., O_NOFOLLOW) would be strict belt-and-braces.
• [correctness] decisions/hashing.py KeyCache.digest / _read_key 🆕 — the 0600 restriction runs only on a cache miss, and the cache stamp is (ino, mtime_ns); a later chmod changes only ctime, so a key loosened after it was cached keeps being used without re-restriction until the inode or mtime changes. Defense-in-depth within the documented same-user model.
• [correctness] decisions/hashing.py:57-66 🆕 — _restrict runs before the len(key) != 32 check, so a loose and malformed key is chmod'd before InvalidKeyError. Harmless; mention only.
• [tests] test/decisions/test_checkpoint.py:155 🆕 — InsecureKeyError is tested at the store.insert seam but not through engine.prepare_launch, i.e. no test shows a launch still returns a plan with no record when the key is insecure. The real-world triggers named in the PR body (key owned by another user, symlinked key) are exercised only via a monkeypatched os.fchmod. Fine to add in the wiring PR.
• [consistency] docs/configuration.md:449-454 🆕 — the "Launch decisions" paragraph lists all four CAO_DECISION_* variables and then says --decision <point>=<state> "sets the matching variable", which could read as if the flag covers all four; it sets only the two point-state variables. The <point>=<state> wording keeps it technically correct.
• [consistency] docs/decisions.md Settings 🆕 — accepted ranges and clamp-with-warning are documented for on_timeout_ms, the threshold and retention, but the shadow limits (now MAX_CONCURRENT_RANGE 1–1024, MAX_PENDING_RANGE 0–100000, SHADOW_TIMEOUT_MS_RANGE 50–60000) are clamped the same way and their ranges are not stated.
• [consistency] PR body, Tests section 🆕 — "An AST guard rejects every form of decision import there" overstates slightly; the guard's own docstring (test/fixtures/decision_boundary.py:59-64) correctly scopes out a computed module name and a lazy import inside a function that never runs. The docstring is the accurate statement.
• [conventions] commits 56a66c80, 07c5baad, ee4d97e3 🆕 — subject lines are 75–90 characters (git's soft limit is 72). No commitlint gate in the repo; cosmetic.
• [conventions] test/decisions/ 🆕 — the only test subdirectory without an __init__.py (all 22 siblings ship one). Collection works today; consistency only.
• [security, discussion] decisions/hashing.py:62-64 🆕 — a key found with loose bits is tightened and then used; restricting cannot undo prior exposure. Acceptable under the documented threat model (0700 parent blocks other-user traversal) and the docs already offer "remove it to start a new key" as the cautious recovery. Noting the trade-off, not a defect.

Tests

Strong and targeted. Every source change in the delta ships with a test that would fail on revert: test_loose_key_file_is_restricted_on_read (0644 → 0600), test_key_file_that_cannot_be_restricted_is_not_used (InsecureKeyError, no row, warning names the file), the real init_db upgrade test in test/clients/test_database.py:1990 plus test_create_all_adds_decision_table_to_existing_database (table + 3 indexes on an old database), test_explicit_allowed_provider_requires_a_mapping (the policy.py:115 fix), test_ast_guard_catches_all_decision_import_forms with a paired negative test for benign dynamic imports, and test_exclusion_does_not_apply_to_ephemeral_targets. test_owner_only_key_file_is_not_rechmodded and test_unrestricted_provider_validation_keeps_empty_maps_unchanged are behavioural guards rather than revert sentinels (they pass both before and after the fix), which is appropriate for what they assert. All four test nits from our previous review (pre-created settings file in test_tune_accepts_range_bounds, ephemeral-target exclusion case, OTEL_SDK_DISABLED cleared in the export fixture, dynamic-import coverage in the guard) are resolved. Isolation is correct throughout (HOME/CAO_HOME_DIR/SETTINGS_FILE monkeypatched, all CAO_DECISION_* cleared, tmp_path, caplog). The remaining gaps are the two listed under Nits.

Verification

Dynamic verification did not return before synthesis. Static evidence at this head: CI 42/42 SUCCESS (the Dependency Security check that failed at the previous head on shell-quote in docusaurus/package-lock.json, a file this PR does not touch, now passes); mergeable: MERGEABLE; gh pr diff and the checked-out worktree agree on 43 files, +5924/−43; a grep across src/, test/, docs/ and CHANGELOG.md in the worktree finds no reference to _migrate_decision_records or flags= in decisions/; no open code-scanning or Dependabot alerts on refs/pull/863/head. The author reports 405 passed for test/decisions/ + test/services/test_ephemeral_service.py on CPython 3.10 and 3.12 and 4,026 passed across the adjacent suites on 3.12, with black, isort, mypy and the markdown link check clean.

Verdict

Approve with nits — the four new commits do exactly what the author's reply claims: the redundant migrator is removed in favour of create_all with an upgrade test, the key file is restricted on read with a fail-safe that writes no record, allowed_providers is enforced per provider so the documentation is now accurate, the dead flags layer is gone, and the remaining documentation clarifications (plugin trust, shadow-span timing, key recovery) match the code. The platform stays off by default and uncalled, the agent-facing boundary is enforced by a wider guard, CI is green and the branch merges cleanly. Everything left is cosmetic.

[haofeif]

Re-review at ee4d97e3 (merge-base fd5113ea): APPROVE. No P1 or P2.

CI: 42/42 checks pass at this head.

Before merge: rebase onto main. GitHub reports this PR as conflicting. Since fd5113ea, main has 7 new commits (#891, #892, #898, #899, #902, #903 and a repin). git merge-tree shows one conflict, in src/cli_agent_orchestrator/cli/main.py: #902 replaced the cli.add_command(...) list with the lazy _COMMANDS table. Do not keep the old import and add_command lines. Add one row:

("decisions", _PKG + "decisions", "decisions", "Manage local launch decisions.", False),

_LazyGroup.get_command reads only this table, so it does not find a command that cli.add_command adds. test_table_matches_real_commands and test_every_catalog_row_has_a_click_command fail if the row is missing or its short help is different. The other files that both sides change merge cleanly (api/main.py, clients/database.py, pyproject.toml, CHANGELOG.md, README.md, docs/configuration.md), and the merged Python files parse. A ruleset dismisses approvals on push, so I will review the rebase diff again.

Earlier findings (review 5428700593 at b2f319dc)

1. Kill switch while an override is set: documented in docs/decisions.md (Settings). A restart is necessary, and there is no all-points off yet. I accept this for a PR that is not wired. The #810 criterion (one command turns every point off) stays open for the wiring PR, which adds cao decisions off.
2. Unguarded OpenTelemetry import: fixed. The ImportError guard degrades only for a missing opentelemetry* module, and emit_record is then a no-op.
3. tune ranges: fixed. The CLI (IntRange/FloatRange) and settings.tune() (_check_range) use the same constants.
4. exclude_profiles gating effort.route: fixed. excluded(point) applies only to model.route, for the asked points and for the out_of_scope records.
5. Doc nit ("read fresh for each call"): fixed. The doc names the shadow limits that the server reads once.

I am resolving the four threads.

Checked and fine (delta e90cdef8..ee4d97e3, and the merge with main)

• targets.profile_source uses only agent_profiles.routes_to_ephemeral_store, which #898 did not change. The #881 test_future_profile_source_contract is the same on main.
• #898 removed EphemeralLaunchRefused. This PR does not use it.
• Ephemeral auto still refuses with auto_requires_decision_platform and does not look for this package, so the merge does not change ephemeral behaviour.
• _migrate_decision_records is gone. create_all creates the table and its indexes on an old database, and the new upgrade test covers this.
• _read_key restricts a loose key on the open descriptor, or does not use the key. allowed_providers now checks a listed provider that has no tier table. apply_flags validates every flag before it sets a variable.
• No test module name in test/decisions/ collides with another test module on main.