feat(cli): always-on --engine-state PATH + safe identity-break recovery (ADR-0220 PR B) (#772)
* feat(cli): always-on --engine-state PATH + safe identity-break recovery (ADR-0220 PR B) Operator ergonomics only — no identity-hashing, strict_identity_continuity, or manifest-schema change (those are gated on ADR-0220 ratification, PR C). - core always-on --engine-state PATH: surface the existing per-life state-root concept. run_daemon already accepts engine_state_path; cmd_always_on now threads the flag through (default None -> $CORE_ENGINE_STATE_DIR / in-repo dir). - _always_on_identity_break_message: replace the terse IdentityContinuityError text with revision-aware recovery guidance (reads written_at_revision so 'git checkout <rev>' is copy-pasteable; offers --engine-state and an in-place clear-runtime-files path). Safe by construction: never suggests mv/rm of the engine_state dir, which under the default IS the tracked Python package. - Suppress ADR-0157's revision-mismatch warning while the formatter reads the manifest (message builder, not a load path). - Tests: flag default/parse, threading to run_daemon, message is safe + revision-aware + placeholder-on-no-manifest. * fix(cli): fall back to <engine_state_dir> placeholder when state dir unresolvable Gemini review nit on #772: if EngineStateStore construction fails AND no --engine-state was given, state_dir stayed None and the recovery message printed a bare 'None' as the dir to clear. Fall back to '<engine_state_dir>'. Adds a test that forces the store-resolution exception path, and guards the no-manifest test against 'None' leaking into the message.
This commit is contained in:
Shay
GitHub
parent
44194e3ef3
commit
b3a1366980
2 changed files with 168 additions and 2 deletions
74
core/cli.py
74
core/cli.py
|
|
@ -329,6 +329,61 @@ def cmd_chat(args: argparse.Namespace) -> int:
|
|||
_ALWAYS_ON_ALIVE_EVERY = 60
|
||||
|
||||
|
||||
def _always_on_identity_break_message(
|
||||
engine_state_path: Path | None, exc: Exception
|
||||
) -> str:
|
||||
"""Operator recovery guidance for an always-on identity-continuity refusal.
|
||||
|
||||
Safe by construction (ADR-0220): it never tells the operator to ``mv``/``rm``
|
||||
the engine-state directory — under the default that directory IS the tracked
|
||||
``engine_state`` Python package, so moving it breaks ``import engine_state``
|
||||
across the codebase (including this very command). It steers to: re-run the
|
||||
originating revision, a separate ``--engine-state`` dir, or clearing ONLY the
|
||||
runtime files. The checkpoint revision is read from the manifest so option 1
|
||||
is copy-pasteable (placeholder when no readable manifest is present)."""
|
||||
import warnings as _warnings
|
||||
|
||||
from engine_state import EngineStateStore
|
||||
|
||||
rev = "<checkpoint_revision>"
|
||||
state_dir: Path | None = engine_state_path
|
||||
try:
|
||||
store = EngineStateStore(engine_state_path)
|
||||
state_dir = store.path
|
||||
with _warnings.catch_warnings():
|
||||
# Reading the manifest re-triggers ADR-0157's revision-mismatch
|
||||
# RuntimeWarning; this is a message formatter, not a load path, so
|
||||
# don't re-warn the operator as a side effect of building the string.
|
||||
_warnings.simplefilter("ignore")
|
||||
manifest = store.load_manifest() or {}
|
||||
rev = str(manifest.get("written_at_revision") or rev)
|
||||
except Exception:
|
||||
# Best-effort: this is a display helper on an error path. If the manifest
|
||||
# can't be read we degrade to the placeholder rev/dir — never let
|
||||
# message-building mask the refusal that brought us here.
|
||||
pass
|
||||
return "\n".join(
|
||||
[
|
||||
"this engine state belongs to a different life (substrate identity "
|
||||
f"or build revision changed): {exc}",
|
||||
"",
|
||||
"Recovery options:",
|
||||
" 1. Resume the old life — run the build that wrote it:",
|
||||
f" git checkout {rev}",
|
||||
" core always-on",
|
||||
" 2. Start a fresh persisted life under a separate state dir "
|
||||
"(leaves the old one untouched):",
|
||||
" core always-on --engine-state ./engine_state_<name>",
|
||||
" 3. Start fresh in place — clear ONLY the runtime files in",
|
||||
f" {state_dir or '<engine_state_dir>'}",
|
||||
" (manifest.json, the 'current' pointer + gen-*/ dirs, "
|
||||
"recognizers.jsonl, discovery_candidates.jsonl,",
|
||||
" session_state.json, proposals.jsonl) — never the directory "
|
||||
"itself (it holds the engine_state package).",
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
def cmd_always_on(args: argparse.Namespace) -> int:
|
||||
"""Run the continuous-life heartbeat daemon (T-experience) until SIGINT/SIGTERM.
|
||||
|
||||
|
|
@ -383,6 +438,7 @@ def cmd_always_on(args: argparse.Namespace) -> int:
|
|||
try:
|
||||
result = run_daemon(
|
||||
config=daemon_config,
|
||||
engine_state_path=getattr(args, "engine_state", None),
|
||||
interval=interval,
|
||||
max_beats=max_beats,
|
||||
no_load_state=bool(getattr(args, "no_load_state", False)),
|
||||
|
|
@ -392,8 +448,9 @@ def cmd_always_on(args: argparse.Namespace) -> int:
|
|||
_die(f"always-on already running for this engine state: {exc}", code=2)
|
||||
except IdentityContinuityError as exc:
|
||||
_die(
|
||||
"this engine state belongs to a different life (substrate identity "
|
||||
f"changed): {exc}",
|
||||
_always_on_identity_break_message(
|
||||
getattr(args, "engine_state", None), exc
|
||||
),
|
||||
code=2,
|
||||
)
|
||||
except IncompatibleEngineStateError as exc:
|
||||
|
|
@ -4265,6 +4322,19 @@ def build_parser() -> argparse.ArgumentParser:
|
|||
"default: load + persist + resume the SAME life across restarts"
|
||||
),
|
||||
)
|
||||
always_on.add_argument(
|
||||
"--engine-state",
|
||||
type=Path,
|
||||
default=None,
|
||||
metavar="PATH",
|
||||
dest="engine_state",
|
||||
help=(
|
||||
"engine-state directory (the per-life state root) for this life; "
|
||||
"default: $CORE_ENGINE_STATE_DIR or the in-repo engine_state/ dir. "
|
||||
"Point at a dedicated path to run a separate persisted life without "
|
||||
"touching another (and without moving the engine_state package dir)."
|
||||
),
|
||||
)
|
||||
always_on.add_argument(
|
||||
"--quiet",
|
||||
action="store_true",
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ import subprocess
|
|||
import sys
|
||||
import threading
|
||||
import time
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
|
@ -26,6 +27,11 @@ from chat.always_on_daemon import (
|
|||
continuous_life_config,
|
||||
run_daemon,
|
||||
)
|
||||
from core.cli import (
|
||||
_always_on_identity_break_message,
|
||||
build_parser,
|
||||
cmd_always_on,
|
||||
)
|
||||
from core.config import RuntimeConfig
|
||||
from workbench.lived_life import lived_life_from_payload
|
||||
|
||||
|
|
@ -210,3 +216,93 @@ def test_interruptible_sleep_without_stop_is_a_plain_sleep() -> None:
|
|||
start = time.monotonic()
|
||||
_sleep_until_stop(0.05, None)
|
||||
assert time.monotonic() - start >= 0.04 # actually waited (no stop predicate)
|
||||
|
||||
|
||||
# --- PR B (ADR-0220): operator ergonomics — per-life state dir + safe recovery ---
|
||||
|
||||
|
||||
def test_always_on_engine_state_flag_defaults_to_none() -> None:
|
||||
# Default: fall back to $CORE_ENGINE_STATE_DIR / the in-repo dir (engine_state_path=None).
|
||||
args = build_parser().parse_args(["always-on"])
|
||||
assert args.engine_state is None
|
||||
|
||||
|
||||
def test_always_on_engine_state_flag_parses_to_path() -> None:
|
||||
args = build_parser().parse_args(["always-on", "--engine-state", "/tmp/es_x"])
|
||||
assert args.engine_state == Path("/tmp/es_x")
|
||||
|
||||
|
||||
def test_always_on_threads_engine_state_to_run_daemon(tmp_path, monkeypatch) -> None:
|
||||
# The wiring obligation: --engine-state must reach run_daemon(engine_state_path=...),
|
||||
# not be silently dropped. Capture the kwargs and short-circuit before the heartbeat.
|
||||
import chat.always_on_daemon as daemon_mod
|
||||
|
||||
class _Stop(Exception):
|
||||
pass
|
||||
|
||||
captured: dict = {}
|
||||
|
||||
def _fake_run_daemon(**kwargs):
|
||||
captured.update(kwargs)
|
||||
raise _Stop() # stop before cmd_always_on's (un-mockable) summary epilogue
|
||||
|
||||
monkeypatch.setattr(daemon_mod, "run_daemon", _fake_run_daemon)
|
||||
target = tmp_path / "branch_life"
|
||||
args = build_parser().parse_args(
|
||||
["always-on", "--engine-state", str(target), "--max-beats", "0"]
|
||||
)
|
||||
with pytest.raises(_Stop):
|
||||
cmd_always_on(args)
|
||||
assert captured["engine_state_path"] == target
|
||||
|
||||
|
||||
def test_identity_break_message_is_safe_and_revision_aware(tmp_path) -> None:
|
||||
# The humane recovery message: revision-aware option 1, the --engine-state escape
|
||||
# hatch, the underlying refusal preserved — and NEVER the mv/rm footgun (which would
|
||||
# destroy the engine_state Python package under the default dir; ADR-0220).
|
||||
state = tmp_path / "engine_state"
|
||||
state.mkdir()
|
||||
(state / "manifest.json").write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"schema_version": 2,
|
||||
"turn_count": 7,
|
||||
"written_at_revision": "deadbeefcafe",
|
||||
"engine_identity": "a" * 64,
|
||||
}
|
||||
),
|
||||
encoding="utf-8",
|
||||
)
|
||||
msg = _always_on_identity_break_message(state, RuntimeError("boom"))
|
||||
assert "git checkout deadbeefcafe" in msg # copy-pasteable originating revision
|
||||
assert "--engine-state" in msg # the safe per-life escape hatch
|
||||
assert "boom" in msg # the original refusal is preserved
|
||||
assert "mv engine_state" not in msg # the footgun is never suggested
|
||||
assert "rm -rf engine_state" not in msg
|
||||
|
||||
|
||||
def test_identity_break_message_without_manifest_uses_placeholder(tmp_path) -> None:
|
||||
# No readable manifest -> placeholder revision, still structured, still safe,
|
||||
# and never degrades the path line to a bare "None".
|
||||
msg = _always_on_identity_break_message(tmp_path / "empty", RuntimeError("x"))
|
||||
assert "<checkpoint_revision>" in msg
|
||||
assert "mv engine_state" not in msg
|
||||
assert "rm -rf engine_state" not in msg
|
||||
assert "None" not in msg
|
||||
|
||||
|
||||
def test_identity_break_message_handles_unresolvable_state_dir(monkeypatch) -> None:
|
||||
# Edge: no --engine-state given AND store resolution fails -> the path line must
|
||||
# fall back to a placeholder, never print a bare "None" (Gemini review nit).
|
||||
import engine_state as es_mod
|
||||
|
||||
def _boom(*_a, **_k):
|
||||
raise RuntimeError("no store")
|
||||
|
||||
# The helper does `from engine_state import EngineStateStore` at call time, so
|
||||
# patching the module attribute reaches it.
|
||||
monkeypatch.setattr(es_mod, "EngineStateStore", _boom)
|
||||
msg = _always_on_identity_break_message(None, RuntimeError("x"))
|
||||
assert "<engine_state_dir>" in msg # path fell back, not None
|
||||
assert "<checkpoint_revision>" in msg # rev fell back too
|
||||
assert "None" not in msg
|
||||
|
|
|
|||
Loading…
Reference in a new issue