feat(session-flow): add save_point.py fill so the handoff fills every slot in one call - #4153
kyle-sexton wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 3m 53s —— View job CI code review (
|
|
Claude finished @kyle-sexton's task in 1m 37s —— View job Security review (
|
|
Last security-reviewed head: |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa7799b65d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`fill <file> --slots <json>` replaces every `<!-- FILL: ... -->` slot in a shape-2 handoff skeleton from one JSON object in one write, so filling a skeleton no longer costs one Edit per slot and an interrupt mid-batch cannot leave a partly filled file. Substitution only, with the one exception a closing handoff needs: a `next` value of exactly `Next: none (closed)` rewrites the bare `Next:` line above the slot and deletes the slot line, which is the only shape `validate` accepts as closed, and refuses when that line is not exactly `Next:`. An optional slot left out of the JSON has its line deleted. A required slot absent, a key naming no slot, a slot name occurring twice, and a value carrying the literal `<!-- FILL` are each refused by name, and the target is byte-identical on every refusal path. The read and write go through bytes and `splitlines(keepends=True)` rather than `_read_lines`, so a CRLF target keeps its line endings and a multi-line value takes the replaced line's own terminator per line. Values are encoded to UTF-8 before any write, so a lone surrogate refuses by key rather than truncating the file. The test helper `fill()` becomes `model_fill()`, since a subcommand named `fill` now shares the suite with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
….36.0 The procedure is now `new`, one slots JSON, `fill`, `validate`, `emit`, and the Edit tool is only the repair path after a failed `validate`. `reference/structure.md` rewrites step 6 to assign `SLOTS` beside the handoff and issue the `fill` call, teaches the JSON rather than only naming it (the slot set is branch-dependent, so the names come out of the skeleton `new` just wrote; a multi-line value is one string with escaped newlines; an optional slot left out has its line deleted; a closing handoff passes the `next` value `Next: none (closed)` exactly), adds the `fill` exit paragraph beside `new`'s, and states that the Python-absent fallback is unchanged. `reference/save-point.md` counts four subcommands rather than three, gains the `fill` bullet with its exits, drops the `new` bullet's claim that the model deletes the optional slots, and routes the closing form through the `next` value instead of describing a hand-written line. `skills/handoff/SKILL.md` and `README.md` follow, and the checklist box that read "only FILL slots edited" becomes the one `fill` call with its refusal discipline. `skills/handoff/evals/evals.json` case 16 grades the one-call fill, and case 17's premise is reachable again: a leftover FILL slot cannot survive `fill`, so the refusal it exercises is a required slot omitted from the JSON, fixed at the JSON and re-run before the `Next:` failure and the UNVALIDATED banner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…f file Nine binding findings from a second review of the two landed commits: - The final write goes to a `tempfile.mkstemp` file in the target's own directory and is `os.replace`d into place, unlinking the temp on any OSError and exiting 2. An interrupted write can no longer truncate a handoff, and `find-handoff` globs only `*.md`, so a stray temp is never read as one. - `fill` exits 2 on a file whose frontmatter is not `type: handoff`, through the same `parse_doc` check `validate` and `emit` use, applied before the slots file is read. - The unreadable-target test fed a directory, which only re-ran the missing-target branch; it now feeds invalid UTF-8 so the decode branch runs. - The non-string and non-encodable checks run over every key before the `FILL_MARK` check runs over any, so a payload carrying both defects exits 2 whatever order its keys arrive in. - The final encode's try/except was unreachable (every value was already encode-checked and the source decoded cleanly) and is gone. - A CRLF variant of the closing-handoff test asserts the rewritten `Next: none (closed)` line ends in CRLF with no `\r\r` and no bare `\n`. - The round-trip test's docstring names `len(matches) > len(carrying)` as the line that actually pins non-greediness. - The exit-2 enumerations in the module docstring, `reference/save-point.md` and `reference/structure.md` gain the non-UTF-8-encodable value and the non-handoff file, and both docs state the temp-file write. `evals.json` case 17 names Edit as the repair path after a post-fill `validate` failure, since no slot survives `fill`. `reference/structure.md:15` is reflowed. - The `## [0.36.0]` entry gains one clause each for the atomic write and the handoff guard. No version bump. Suite: 106 passed, 1 skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`cmd_fill` guarded only `type: handoff`, so a skeleton written by a newer engine was accepted: a `handoff_shape: 3` file carrying a familiar `<!-- FILL: ... -->` slot was rewritten with exit 0, while `validate` exits 3 on the same file and says to read it and not rewrite it. `fill` now runs `validate_doc`'s shape ladder before parsing any slot, in the same order and for the same reason: `_UnparsableShape` first so no later comparison meets a non-integer, then shape 1 or an absent key, then a shape below 1, then a shape newer than 2. A newer shape exits 3 with `validate`'s verdict; every other shape that is not 2 exits 2 beside the `type: handoff` guard. Both refusals land before the write, so the target stays byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5c7837a to
7437e7c
Compare
1 participant
Closes #4156
Summary
save_point.py newwrites a shape-2 handoff skeleton with 19 to 22<!-- FILL: ... -->slots, and the write procedure told the model to fill each one with the Edit tool. That made the slot count the write count: one message carrying twenty Edits against one file, and an interrupt mid-batch left a partly filled skeleton that took more turns to find and repair. The fill step had no script surface, so Edit was the only tool that fit a slot.Fix
save_point.py fill <file> --slots <json>: one JSON object keyed by slot name, every value a string, applied to every slot in one atomic write (temp file beside the target, then rename). A slot line's own terminator is kept, so a CRLF skeleton stays CRLF and a multi-line value takes the replaced line's ending per line. An inline prefix on a slot's line (**Amended:**,**Next action serves it by:**, thedid: ... · left: ...pair on one line) is preserved, because only the slot span is replaced.optional:) left out of the JSON has its line deleted. A required slot absent, a key naming no slot, a slot name occurring twice, a value carrying the literal slot marker, or a file with no slot exits 1 naming it; a missing or unreadable target, a non-handoff file, a target whosehandoff_shapeis not 2 (shape 1, no key, below 1, or non-integer), or a slots file that is missing, unreadable, not a JSON object, or holds a non-string or non-UTF-8-encodable value exits 2; a shape newer than 2 exits 3 withvalidate's read-it-do-not-rewrite-it verdict, so version skew cannot corrupt a future-format handoff. Every refusal leaves the target byte-identical.fillprints nothing on success.nextvalue of exactlyNext: none (closed)rewrites the bareNext:line above the slot and deletes the slot line, the only shapevalidateaccepts as closed; it refuses when that line is not exactlyNext:.reference/structure.mdstep 6,reference/save-point.md,skills/handoff/SKILL.md, andREADME.mdnow describenew, one Write of<same stem>.slots.jsonbeside the handoff,fill,validate,emit; the Edit tool is the repair path after a failedvalidate, never the fill step. The handoff evals case for a leftover slot is rewritten to the reachable failure (a required slot omitted from the JSON). Session-flow 0.36.0,### Added.Verification
scripts/tests/test_save_point.pythroughsave_point.test.sh: red first (31 failures oninvalid choice: 'fill'), then green; 106 passed and 1 skipped after the review-fix commit, 112 passed and 1 skipped after the shape-guard commit (six shape cases red first against fa7799b). Cases cover every acceptance criterion: full fill thenvalidateexit 0, inline-prefix and two-slots-on-one-line preservation, multi-line values, optional-slot deletion, required-slot and unknown-key refusal, duplicate slot name, marker-in-value, malformed and non-object JSON, non-string and lone-surrogate values, missing and invalid-UTF-8 target, non-handoff file, every non-2 shape, CRLF preservation with\r\ninside a value, the closing rewrite (LF and CRLF, with and withoutgoal-rearm, refusal when the line above is notNext:), and no temp file left behind.new(22 slots), one JSON,fillexit 0,validatePASS,emitclean, noFILLtext left, thedid · leftline intact.check-changed-skills.shreports handoff PASS;check-changelog-parity.sh --check-bump,--check-preserved,--check-orderpass; markdownlint 0 issues on the touched docs.fill; stdin input is out of scope.Related
handoff-inbox/in-progress/20260911-052500-session-flow-handoff-one-edit-per-fill-slot.md(local queue).hop_chain.pycarries a sibling slot parse for its own skeletons;fillcopies the shape rather than importing, per the house copy-not-import rule stated insave_point.py.🤖 Generated with Claude Code