fix: autocommit CAS + prune tie-break, bwrap --new-session, cost estimates, tool schema contract - #14
Merged
Conversation
…mates, tool schema contract
Four tracker items, each with a regression test verified to fail against the
reintroduced bug.
## autocommit: concurrent writes silently destroyed history
`parent` comes from this process's in-memory `auto_commits`, so two instances
sharing a session id (second pane, resumed session) each built their own commit
chain and both wrote the same shadow ref. The later `update-ref` orphaned the
other's history — exactly the history /undo exists to reach, gone with no error.
`update-ref` now carries an expected-old value, making the write a
compare-and-swap that git enforces atomically under its own ref lock. No
lockfile of ours to leak, and it works across processes and machines. A
conflict returns the new `SnapshotOutcome::Conflict` and is surfaced in the TUI
rather than only logged — the turn is missing from /undo history, so the user
has to know.
Two corrections while building this, both from checking assumptions rather than
trusting them:
- Reading the ref at the start of the call is useless: a competing writer that
landed between turns has already committed, and we would cheerfully CAS
against its value. The meaningful expectation is our own chain head.
- That chain head is `auto_commits.last()`, NOT the undo position. `restore_to`
rewrites the working tree but deliberately leaves the ref alone, so after an
/undo the ref still points at the newest commit. Using the undo position
manufactured a conflict on the first turn after any undo — caught by the
existing redo test.
## autocommit: prune could delete the newer of two sessions
`%(committerdate:unix)` has one-second granularity, so sessions created in
quick succession tie, and ties fell through to git's output order
(refname-alphabetical) — unrelated to recency. Ties now break on refname
descending: session ids are monotonic within a run, so it is a better proxy and
is at minimum deterministic. Documented as a heuristic, not a guarantee.
## bwrap: --new-session (P3)
Without it the sandboxed process shares our controlling terminal and can push
characters back into it with TIOCSTI, which the parent shell then executes as
if typed — an escape straight out of the sandbox. Modern kernels default
`dev.tty.legacy_tiocsti=0`, but that is a host setting we do not control.
## cost: unknown models were priced as fact (P4)
An unrecognised model silently fell back to Sonnet-tier rates, so /cost and any
/budget cap reported a guess as a published number — and an unknown model may be
an order of magnitude cheaper or dearer. The fallback stays (a budget should
still function) but is now flagged: the model is warned about once, per-model
rows are marked `~`, and the summary carries a footnote.
## tools: standing schema contract (P5)
Swept all 43 registered tools for schema/implementation drift. The only static
hit was a false positive — `Grep`'s ripgrep-style flags (`-A`, `-i`, …) connect
through `#[serde(rename)]`, which the scanner did not model.
Rather than leave that as a one-off, the sweep is now a test over the **full**
registry (`all_tools`, 44 tools — `default_tools` is a subset and would have
hidden this): names unique and non-empty, descriptions present, schemas
type=object, every property typed and described, everything in `required`
actually declared.
It immediately found 5 real defects that the static pass missed —
`TaskUpdate.status`, `TaskUpdate.task_id`, `TaskStop.task_id`,
`browse_done.achieved`, `browse_done.summary` all shipped with no description,
leaving the model to guess their meaning. Fixed.
QA: 575 tests pass, 0 failures. Clippy clean under the CI gate. Release builds
at 19.05 MB. Each of the four fixes verified by reintroducing its bug:
dropping the CAS, removing the tie-break, un-flagging the estimate, and
deleting a param description each fail their test.
Co-Authored-By: Arch Linux <noreply@archlinux.org>
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.
Four tracker items (autocommit P2, bwrap P3, cost P4, tool sweep P5), each with a regression test verified to fail against the reintroduced bug.
autocommit: concurrent writes silently destroyed history
parentcomes from this process's in-memoryauto_commits, so two instances sharing a session id each built their own chain and both wrote the same shadow ref. The laterupdate-reforphaned the other's history — the history /undo exists to reach, gone with no error.update-refnow carries an expected-old value, making it a compare-and-swap git enforces atomically under its own ref lock. No lockfile of ours to leak. A conflict returnsSnapshotOutcome::Conflictand is surfaced in the TUI, not just logged — the turn is absent from /undo history, so the user must know.Two corrections found by checking assumptions rather than trusting them:
auto_commits.last(), not the undo position:restore_torewrites the working tree but leaves the ref alone. Using the undo position manufactured a conflict on the first turn after any undo — caught by the existing redo test.autocommit: prune could delete the newer of two sessions
%(committerdate:unix)is one-second granular, so sessions tie and ties fell through to git's refname-alphabetical output order — unrelated to recency. Now broken deterministically on refname descending (session ids are monotonic within a run). Documented as a heuristic, not a guarantee.bwrap
--new-session(P3)Without it the sandboxed process shares our controlling terminal and can inject characters via TIOCSTI, which the parent shell executes as if typed. Kernels default
dev.tty.legacy_tiocsti=0, but that is a host setting we do not control.cost: unknown models priced as fact (P4)
Unrecognised models silently used Sonnet-tier rates, so /cost and /budget reported a guess as a published number. Fallback retained so budgets still function, but now flagged — warned once per model, rows marked
~, footnote in the summary.tools: standing schema contract (P5)
Swept all 43 registered tools. The only static hit was a false positive —
Grep's ripgrep-style flags connect via#[serde(rename)].The sweep is now a test over the full registry (
all_tools, 44 tools —default_toolsis a subset and would have hidden this). It immediately caught 5 real defects the static pass missed:TaskUpdate.status,TaskUpdate.task_id,TaskStop.task_id,browse_done.achieved,browse_done.summaryall shipped with no description, leaving the model to guess. Fixed.QA
575 tests, 0 failures. Clippy clean under the CI gate. Release builds at 19.05 MB.
Each fix verified by reintroducing its bug — dropping the CAS, removing the tie-break, un-flagging the estimate, and deleting a param description each fail their test.