feat(hook): session-end Stop hook prompts to lock session corrections (#582) - #590
Merged
Merged
Conversation
…582) Adds a Stop hook entry point that enumerates correction-class beliefs created in the current session and either emits a stderr listing with pre-filled aelf lock commands, or auto-locks them when AELF_AUTOLOCK_CORRECTIONS=1 is set. Candidate filter: session_id == current AND lock_level != LOCK_USER AND (type == BELIEF_CORRECTION OR origin in {agent_inferred, agent_remembered}). The candidate-walk is one list_belief_ids() + one get_belief() per id; small enough for typical session-end stores. A focused SQL query is a follow-up when stores grow. Hook contract: never blocks, never raises. Empty payload, missing session_id, store errors all return 0 silently. main_stop() entry point added. pyproject.toml wiring + setup.py install/uninstall + aelf doctor checks land in follow-up commits.
Mirrors the install_session_start_hook pattern: idempotent settings.json mutation under hooks.Stop, basename + exact-match uninstall semantics, atomic _atomic_write commit. Stop event coexists with the transcript-ingest Stop entry as a separate entry under the same key. resolve_stop_hook_command picks the absolute path with the same project=venv-first / user=PATH-first precedence as the other hook script resolvers. CLI wiring of these helpers into 'aelf setup' / 'aelf uninstall' lands in the next commit.
Default-on Stop hook installation, mirrors the SessionStart pattern. --no-stop-hook flag opts out on either subcommand. Behaviour: aelf setup # installs Stop hook (default ON) aelf setup --no-stop-hook aelf unsetup # removes it (default ON) aelf unsetup --no-stop-hook The Stop hook coexists with the existing transcript-ingest Stop entry under the same hooks.Stop event key in settings.json. Set AELF_AUTOLOCK_CORRECTIONS=1 in the env to make the hook auto-lock session corrections instead of emitting the lock-prompt.
Extends the v2.1 auto-capture nag (#557) to flag installs missing aelf-stop-hook from every scanned settings.json. Catches the upgrade-from-pre-#582 case where the user's settings.json was written by an aelf setup that ran before the Stop hook landed. Updates the parity guardrail test in test_doctor.py to include STOP_HOOK_SCRIPT_NAME in the expected set.
41 new tests across two files:
- tests/test_hook_stop_lock_prompt.py — predicate + collection +
formatter + autolock-env + autolock-mutates + integration
(empty/malformed payload, missing session_id, no candidates,
prompt-mode listing, AUTOLOCK env-mode locking).
- tests/test_setup_stop_hook.py — install + uninstall idempotency,
coexistence with the transcript-ingest Stop entry under the same
Stop event key, basename-uninstall scoped to aelf-stop-hook only.
Also updates the existing doctor + cli-setup tests to expect the
fourth default-on basename in the auto-capture nag set, and to pass
--no-stop-hook in the opt-out integration scenario.
Full suite: 3220 passed, 53 skipped.
Rewrites three docstring lines added in 0f5f2c0 to use 'harness' / 'the Stop-hook contract' instead of naming the host platform. Matches the surrounding existing code's phrasing and clears the pre-push discretion grep on this branch.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideImplements a new default-on session-end Stop hook ( Sequence diagram for the new session-end Stop hook lock promptsequenceDiagram
actor User
participant Harness
participant StopHook as aelf_stop_hook
participant Store as MemoryStore
User->>Harness: Complete assistant turn
Harness->>StopHook: Invoke with Stop JSON payload on stdin
StopHook->>StopHook: _autolock_enabled(env)
StopHook->>StopHook: _extract_session_id(raw)
alt session_id missing or payload invalid
StopHook-->>Harness: Return 0 (no output)
else session_id present
StopHook->>StopHook: _open_store()
StopHook->>Store: list_belief_ids()
loop For each belief id
StopHook->>Store: get_belief(bid)
StopHook->>StopHook: _belief_is_lock_candidate(b, session_id)
end
StopHook->>StopHook: _collect_lock_candidates(...)
alt No candidates
StopHook-->>Harness: Return 0 (no output)
else Candidates found
alt AELF_AUTOLOCK_CORRECTIONS truthy
StopHook->>Store: update_belief() with LOCK_USER and origin user_stated
StopHook->>Harness: stderr "aelfrice: auto-locked ..."
StopHook-->>Harness: Return 0
else Prompt mode
StopHook->>StopHook: _format_stop_prompt(candidates)
StopHook->>Harness: Write <aelfrice-session-end> block to stderr
StopHook-->>Harness: Return 0
end
end
Store-->>StopHook: close()
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
author-noether
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
stop(), you parse the JSON payload intopayloadbut then call_extract_session_id(raw)instead of reading from the parsed dict; consider usingpayload.get("session_id")to avoid double work and reduce reliance on the string-level extractor. - The broad
except Exceptioninside_autolock_candidateswill silently swallow programming errors (e.g., schema mismatches) along with expected store failures; consider catching narrower exceptions or at least logging a full traceback to aid debugging while still honoring the fail-soft contract.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `stop()`, you parse the JSON payload into `payload` but then call `_extract_session_id(raw)` instead of reading from the parsed dict; consider using `payload.get("session_id")` to avoid double work and reduce reliance on the string-level extractor.
- The broad `except Exception` inside `_autolock_candidates` will silently swallow programming errors (e.g., schema mismatches) along with expected store failures; consider catching narrower exceptions or at least logging a full traceback to aid debugging while still honoring the fail-soft contract.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="1582-1591" />
<code_context>
+ serr = stderr if stderr is not None else sys.stderr
+ if not _IMPORTS_OK:
+ return 0
+ try:
+ raw = sin.read()
+ if not raw or not raw.strip():
+ return 0
+ try:
+ payload = json.loads(raw)
+ except json.JSONDecodeError:
+ return 0
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid parsing the Stop payload JSON if the parsed object is never used
`payload = json.loads(raw)` is never used; the rest of the function works directly with `raw` and `_extract_session_id(raw)`. This adds unnecessary overhead on a hot path and could cause behavior drift if `_extract_session_id` later expects different input. Either remove the parse or change the code to use `payload` (for example, by having `_extract_session_id` take the parsed dict).
</issue_to_address>
### Comment 2
<location path="tests/test_setup_stop_hook.py" line_range="107-104" />
<code_context>
+ assert result.removed == 0
+
+
+def test_uninstall_stop_hook_missing_file(tmp_path: Path) -> None:
+ p = tmp_path / "nope.json"
+ result = uninstall_stop_hook(p, command=_STOP_CMD)
+ assert result.removed == 0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Mirror the validation tests from session-start hooks for `uninstall_stop_hook` argument error cases.
These tests only exercise the happy path. Please also add cases covering `uninstall_stop_hook`’s argument validation (both `command` and `command_basename` missing, both provided, and empty-string inputs), mirroring the existing `uninstall_session_start_hook` validation tests so behavior stays consistent and guarded against regressions.
Suggested implementation:
```python
import json
from pathlib import Path
import pytest
```
```python
from aelfrice.setup import (
install_stop_hook,
uninstall_stop_hook,
install_transcript_ingest_hooks,
```
```python
def test_uninstall_stop_hook_no_match(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
install_stop_hook(p, command=_STOP_CMD)
result = uninstall_stop_hook(p, command="/different/binary")
assert result.removed == 0
def test_uninstall_stop_hook_missing_file(tmp_path: Path) -> None:
p = tmp_path / "nope.json"
result = uninstall_stop_hook(p, command=_STOP_CMD)
assert result.removed == 0
def test_uninstall_stop_hook_requires_command_or_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p)
def test_uninstall_stop_hook_rejects_both_command_and_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(
p,
command=_STOP_CMD,
command_basename="stop-binary",
)
def test_uninstall_stop_hook_rejects_empty_command(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p, command="")
def test_uninstall_stop_hook_rejects_empty_command_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p, command_basename="")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1582
to
+1591
| try: | ||
| b.lock_level = LOCK_USER | ||
| b.locked_at = now | ||
| b.demotion_pressure = 0 | ||
| b.origin = ORIGIN_USER_STATED | ||
| store.update_belief(b) | ||
| locked += 1 | ||
| print( | ||
| f"aelfrice: auto-locked {b.id} ({b.type}, origin→user_stated)", | ||
| file=stderr, |
There was a problem hiding this comment.
suggestion (performance): Avoid parsing the Stop payload JSON if the parsed object is never used
payload = json.loads(raw) is never used; the rest of the function works directly with raw and _extract_session_id(raw). This adds unnecessary overhead on a hot path and could cause behavior drift if _extract_session_id later expects different input. Either remove the parse or change the code to use payload (for example, by having _extract_session_id take the parsed dict).
| p = tmp_path / "settings.json" | ||
| install_stop_hook(p, command=_STOP_CMD) | ||
| result = uninstall_stop_hook(p, command="/different/binary") | ||
| assert result.removed == 0 |
There was a problem hiding this comment.
suggestion (testing): Mirror the validation tests from session-start hooks for uninstall_stop_hook argument error cases.
These tests only exercise the happy path. Please also add cases covering uninstall_stop_hook’s argument validation (both command and command_basename missing, both provided, and empty-string inputs), mirroring the existing uninstall_session_start_hook validation tests so behavior stays consistent and guarded against regressions.
Suggested implementation:
import json
from pathlib import Path
import pytestfrom aelfrice.setup import (
install_stop_hook,
uninstall_stop_hook,
install_transcript_ingest_hooks,def test_uninstall_stop_hook_no_match(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
install_stop_hook(p, command=_STOP_CMD)
result = uninstall_stop_hook(p, command="/different/binary")
assert result.removed == 0
def test_uninstall_stop_hook_missing_file(tmp_path: Path) -> None:
p = tmp_path / "nope.json"
result = uninstall_stop_hook(p, command=_STOP_CMD)
assert result.removed == 0
def test_uninstall_stop_hook_requires_command_or_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p)
def test_uninstall_stop_hook_rejects_both_command_and_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(
p,
command=_STOP_CMD,
command_basename="stop-binary",
)
def test_uninstall_stop_hook_rejects_empty_command(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p, command="")
def test_uninstall_stop_hook_rejects_empty_command_basename(tmp_path: Path) -> None:
p = tmp_path / "settings.json"
with pytest.raises(ValueError):
uninstall_stop_hook(p, command_basename="")
Owner
Author
|
[claim:review:Maxwell:2026-05-10T08:11:31Z] |
Owner
Author
|
[release:review:Maxwell:2026-05-10T08:12:44Z] |
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.
Closes #582.
What lands
A new
Stophook (aelf-stop-hook, default-on) that fires once per assistant-turn end, walks the store for unlocked correction-class beliefs created in the currentsession_id, and either emits a stderr listing with pre-filledaelf lockcommands (default) or auto-locks them whenAELF_AUTOLOCK_CORRECTIONS=1is set in the environment.Spec deviation — read this first
The issue's body assumed a
aelf correctCLI command and afeedback_history.kind=correctmarker that don't exist onmain— neither was ever shipped. I confirmed: the onlyaelfsubcommands related to belief mutation arelock,confirm,delete,promote,unlock. Thefeedback_historyschema has nokindcolumn.So the issue's two proposed signals — (a)
feedback_events with kind=correctand (b)beliefs modified via aelf correct CLI— are unimplementable as written.This PR ships the implementable v0 (operator selected; recorded in #582 reply): query unlocked beliefs in the current session whose
type=BELIEF_CORRECTIONORorigin in {agent_inferred, agent_remembered}. That's the closest signal the schema actually exposes for "correction-class belief touched in this session." It does not capture beliefs whose existing content was modified — only newly-created ones from the session. A follow-up to shipaelf correct+feedback_history.kindwould let this hook also surface modifications.Atomic commits
7 SSH-signed commits, in dependency order:
0f5f2c0feat(hook): stop() handler emits lock-prompt for session corrections (#582)— pure logic inhook.py:stop()+_belief_is_lock_candidate+_collect_lock_candidates+_format_stop_prompt+_autolock_enabled+_autolock_candidates+main_stop().27f9507build: register aelf-stop-hook console script (#582)— pyproject entry point.39a8ec5feat(setup): install/uninstall_stop_hook helpers (#582)— settings.json wiring underhooks.Stop, mirrorsinstall_session_start_hookexactly.4fbfcf6feat(cli): wire stop hook into 'aelf setup' / 'aelf unsetup' (#582)— default-on installation,--no-stop-hookopt-out flag on both subcommands.d61be0afeat(doctor): nag when default-on Stop hook is missing (#582)— extends the v2.1 Claude not using aelf to remember checkpoints, only knows to use it to remember user preferences. #557 auto-capture nag to the fourth basename. Updates the doctor parity guardrail test.098faebtest(hook): coverage for stop hook + setup install + doctor nag (#582)— 41 new tests across two files; updates 2 existing tests for the new fourth basename.302f7a0docs(changelog): unreleased entry for #582 stop hook + lock prompt.d7ca88dstyle(hook): use 'harness' phrasing in #582 docstrings (#582)— tiny phrasing fix to clear the pre-push discretion grep.Acceptance
aelf setup(default-on, idempotent).aelf lock --statement '...'commands per candidate.AELF_AUTOLOCK_CORRECTIONS=1env var bypasses prompt, auto-locks candidates, logs each auto-lock to stderr.aelf doctorchecks for the hook's installation (extends the existing Claude not using aelf to remember checkpoints, only knows to use it to remember user preferences. #557 nag).hooks.Stopevent key — both are separate entries; basename-uninstall of one does not displace the other (testtest_uninstall_stop_hook_basename_does_not_remove_transcript_ingest).Verification
uv run pytest -x -q→ 3220 passed, 53 skipped. (3179 baseline + 41 new + 2 updated existing.)style(hook)rephrasing was the only addition; that was the discretion-trip I rephrased away).Out of scope (deferred — explicit)
aelf correctCLI +feedback_history.kindcolumn, neither of which exists on main. Should be filed as a follow-up issue if/when that infrastructure ships.list_belief_ids() + get_belief(). For typical session-end stores (<1k beliefs) this is sub-100ms. A focused SQLSELECT * FROM beliefs WHERE session_id=? AND lock_level != ? AND (type=? OR origin IN (...))is a future optimisation when stores grow.Summary by Sourcery
Add a default-on Stop hook that surfaces session-end correction-class beliefs for locking and wire it into setup, doctor, and CLI tooling.
New Features:
Enhancements:
Documentation:
Tests: