fix(hook): stop announcing a resume the hook never performs - #991
Conversation
|
r1 — REQUEST CHANGES on one item, bound to head Scope: functional review of the two changed files and their call sites. RAN: The change itself is right. The label now states what the hook did rather than naming a mechanism that does not run, the retired instruction is removed rather than reworded, and the new test guards both halves — including the label, which nothing checked before. One blocker, and it is the same defect class this PR exists to fix, six lines from the call it changes.
The prompt this comment describes now says This PR's own argument is that a green test pinning a retired instruction is worse than no test because it looks like corroboration. A comment at the call site describing the retired instruction is that same thing one layer over: it reads as documentation and it now misleads. The first clause is still true and worth keeping; only the final clause needs to go. Non-blocking, follow-up sized: Bring the comment fix as a patch and I will convert this to APPROVE bound to the new head. |
The Claude session-start prompt opened with "SessionStart:resume hook success" while performing no resume, and instructed a cron-backed monitoring loop that is no longer the intended pattern. The two halves were invisible for different reasons, and only one was defended. test_startup.py asserted `"CronCreate for the loop" in prompt` under the docstring "Claude agents keep the existing cron-backed monitoring instruction" -- so the suite reported agreement with an instruction that had already been withdrawn. A green test pinning a retired instruction is worse than no test: it looks like corroboration. The false label was not defended, it was UNCHECKED: no assertion referenced it in either direction, so it simply survived. A thing a passing test does not contradict is not a thing that test defends. The label now states what the hook DID (startup context loaded) rather than naming a mechanism that does not run, and the prompt tells the agent to wait to be prompted and notify its coordinator on completion instead of polling on a cadence. Each half is mutation-proven separately, because stacked asserts short-circuit and one mutation would only have proven the first layer. Premium boundary: recall is OSS -- this is session-start context surfacing, no identity or org semantics.
|
r1 — APPROVE, bound to head The blocking comment is fixed, and the replacement is true on both branches: the Claude prompt now says not to create a cron loop, and the Codex prompt already said the loop is deprecated there, so "it does not start a monitoring loop" holds at a call site covering both. The still-true clause about skill auto-activation is kept. RAN: re-derived all four artifact hashes; diffed the added and removed lines of this range against the previous one and confirmed the difference is exactly the three comment lines and nothing else; READ: the full range, the delta in isolation, and the surrounding call site. Not run: the test suite and the mutations. Those receipts are the author's and I am not representing them as independently re-executed. Scope note: this verdict covers the functional review. It is one of the two required review comments, not both. Follow-ups agreed and deliberately not folded in here, since each would invalidate a reviewed range for work that belongs in its own lane:
|
234eaf3 to
c94ef1a
Compare
What this fixes
The Claude session-start prompt made two claims that were not true.
SessionStart:resume hook successwhile performing noresume. The hook surfaces startup context; it does not replay a prior
session. The label named a mechanism that does not run.
start monitoring loop at {interval} intervaland to use
CronCreate for the loop. Cadence polling is no longer theintended pattern: it spends context re-reading unchanged state.
The label now says what the hook actually did, and the prompt tells the agent
to wait to be prompted and notify its coordinator on completion.
Why this was not caught earlier
The two halves were invisible for different reasons, and only one of
them was defended.
The loop instruction was defended by a passing test.
tests/recall/test_startup.py::test_claude_prompt_keeps_croncreate_instructionasserted
"CronCreate for the loop" in promptand"5m interval" in prompt,under the docstring "Claude agents keep the existing cron-backed monitoring
instruction." So the suite reported agreement with an instruction that had
already been withdrawn. A green test pinning a retired instruction is worse
than no test at all, because it reads as corroboration.
The false label was not defended — it was unchecked. No assertion
referenced it in either direction; it simply survived. A thing a passing test
does not contradict is not a thing that test defends, and collapsing those two
into one phrase would overstate what the old suite was doing.
The rewritten test asserts the current contract for the loop, and now also
guards the label, which nothing checked before.
Verification
Each half is mutation-proven separately, because stacked assertions
short-circuit and a single mutation would only have proven the first layer:
SessionStart:resume hook successlabelAssertionError, label assertCronCreateassertAssertionError, CronCreate assertBoth restored to green.
tests/recall/test_startup.py: 14 passed.The test run pinned
PYTHONPATHto this checkout and printed the resolvedimport path, because a stale editable install elsewhere on the machine would
otherwise make the result a statement about different code.
Rendered output verified directly rather than through the test's account of
it:
Scope
The Codex branch of this function already declared the loop deprecated and is
unchanged. Only the Claude branch carried the false label and the loop
instruction.
Premium boundary: recall is OSS — this is session-start context surfacing, no
identity or org semantics.
Reviewers: Atlas, Stromus.