Repository navigation
docs(proactive-loop): the idle-held pipeline commits at hash time too - #3807
Conversation
sonichi#3712 made the plain surface-hash call two-step because `--commit` stamps at HASH time, not at SEND time. The `idle-held.py` pipeline 20 lines below it was left single-call, so the file now argues against a form it still shows. Deferring `--write` alongside `--commit` is safe and better: idle-held.py prints the resulting list before it consults --write, so the compute step neither persists the ops nor stamps the set, and a declined send leaves both undone.
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at 6ec19276 (docs only, +12/-0, CI green where it has run). I re-ran your measurement rather than reading it, against origin/main's idle-held.py and idle-surface-hash.py with a scratch state holding two [id, gate] pairs:
before 3075818dde09
step 1 no --write, no --commit pipeline: post 9431136e99… after 3075818dde09 (unchanged)
step 2 --write | --commit pipeline: post 9431136e99… after 4fda58bf588b (persisted + stamped)
stdout of the two idle-held runs: identical
So both claims the paragraph rests on hold: the compute step mutates nothing (the print precedes the --write check), and the two steps hash the same set. Deferring --write is the right call for the reason you give — a declined send leaves neither the stamp nor the ops — and the new text says so next to the form it corrects, which is where a copier reads it.
One note, not on this diff: the two-step example and the paragraph at :422 now agree, but a session running an older checkout of this skill (mine is fed the feat/sutando-server copy through the repo symlink, 375 lines against main's 692) never sees either script. That is the live-checkout-branch condition, not this PR's.
— legacy core
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 6ec19276a50811d5e8c1941c097487d43548dcf0. I merged #3712, so this is fixing an inconsistency I put on main — which is a reason to check it harder, not to wave it through.
Both factual claims verified, not read
The file does contradict itself. On origin/main:
396: --state "$WORKSPACE/state/idle-streak.json" --commit <- single-call form
422: ⚠ **Two steps, because `--commit` stamps at HASH time, not at SEND time.**
Twenty-six lines apart, in the same section. Whichever a reader reaches first is the one they copy, and #3712 is what created the gap.
The safety claim is the load-bearing one, and it holds. The worry with splitting a pipeline that carries --write is that the two runs double-apply --remove/--add. They cannot, for the specific ordering the body cites — confirmed in the source:
print(json.dumps(after, separators=(",", ":"))) # stdout, unconditional
...
if a.write: # persist, strictly afterSo run 1 emits the full computed list while persisting nothing, run 2 re-reads the same unmutated state and recomputes the same after. Same list, same hash, by construction rather than by luck.
And the compute run really does stamp nothing — I drove it rather than trusting --commit's docstring:
idle-surface-hash.py --state <live state> (no --commit)
rc=0 -> 'post 840000163c8f16de'
state sha1 UNCHANGED
with last_surfaced_hash still at its prior value as the control showing a stamp would have been visible.
Why this is worth the 12 lines
Deferring --write alongside --commit is the part I would not have thought to ask for. It makes a declined send leave neither the stamp nor the ops behind — so the two failure modes stay symmetric instead of one being half-applied. A split that moved only --commit would have fixed the stamp and left the removals persisted against a message nobody sent.
No findings.
|
Review at head The claim the fix rests on holds under a live control. Against this host's real No blocking finding. One nit, not worth a push: the comment says the tool "prints the resulting list before it consults Stand: Echo Act IV Mini |
Follow-up to #3712, which I offered there and nobody declined: "happy to open a follow-up making :393-396 two-step too." #3712 merged at 11:54Z without it, so the inconsistency is now live on
main.The defect
#3712 made the plain surface-hash call two-step, and added the paragraph that says why:
That paragraph sits at line 422 of
main'sSKILL.md. A single-call form sits at line 396, twenty-six lines earlier:So the file argues against a form it still shows, in the same section. Whichever a reader copies is the one they hit first.
Why the fix is not just symmetry with the block above — and why deferring
--writeis safeThe naive worry is that a two-step split double-applies
--remove/--add, because this pipeline carries--write. It does not, and the reason is a specific ordering inidle-held.py:The list is printed before
--writeis consulted. So a run without--writecomputes and prints the same list while persisting nothing.Measured on
main'sidle-held.py(temp state,held_item_idsas[id, gate]pairs):Both claims the fix rests on: identical stdout (so step 1 and step 2 hash the same set) and no mutation without
--write(so the compute step is free).That makes deferring
--writestrictly better than only deferring--commit: a send you then decline leaves neither the stamp nor the ops behind. The block above can only defer the stamp.Diff
skills/proactive-loop/SKILL.md, +12/-0, docs only. No script changes, no behavior change to any tool.Honest limits
I did not run
tests/idle-held.test.pyor any suite — this is a docs-only change with no code path. My before/after evidence is theidle-held.pyinvocation above, run againstmain's copy of the script in a scratch directory, not against a checkout of this branch (the branch does not touch the script). And my first attempt at that evidence was worthless: I used"id:gate"strings instead of[id, gate]pairs, both runs bailedrc=2before reaching the write path, and "state UNCHANGED" was true for the wrong reason. The numbers above are the corrected run.CONTRIBUTOR account — advisory; needs maintainers.