Repository navigation
Conversation
`vhir approve` (interactive review) loaded findings.json and timeline.json once, held those lists across the whole review — per-item prompts plus an optional $EDITOR session with a 3600s timeout — and then wrote them back wholesale. save_findings/save_timeline are whole-file replaces, so any finding, timeline event or edit the MCP server staged while the examiner was reviewing was silently deleted from the case, with no warning and no audit-log record of the deletion. The fix reloads both files after the dispositions are collected and rebuilds the lists from disk, substituting the in-memory object only for items the examiner actually approved or rejected. That preserves the examiner's edits and notes on the items they dispositioned (a status-only overlay would lose those) while everything else — including items staged during the review — comes back from disk. The two check_case_file_integrity calls guard the reload, since load_findings returns [] on a corrupt file and an unguarded reload would then truncate the case. reject.py already does exactly this reload, with the same comment, before applying its dispositions; this brings approve.py's interactive path in line with it. Proved by two new tests in TestApproveInteractive: one stages a finding from the prompt callback and asserts it survives the approval, the second does the same on the [n]ote path and additionally asserts the examiner's note is still on the approved finding — a status-only reload would fail it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a-before-interactive-approve
Rescoped. 98147af (_keep_landed) now keeps records *staged* during the approval wait, which was the original target of this branch. Its approach differs on purpose — it writes back an item deleted during the wait, and warns rather than truncating on a corrupt re-read — and this branch's full reload contradicted both (three of 98147af's tests failed against it). The reload and its two tests are dropped. What _keep_landed still misses: it only appends ids it doesn't know. A record that already existed but was *changed* on disk during the wait is written back from the stale copy read before it. Concretely, if another examiner approves F-2 from a second terminal while this one is reviewing and skips F-2, F-2 goes back to DRAFT — while the other examiner's signed record stays in approvals.jsonl, so finding and ledger disagree. Red in all three modes (specific ids, interactive, review) on main. Snapshot each list as first read. At write time, take the on-disk copy of any record this run left byte-identical but that changed on disk. A record this run did change — approved, rejected, noted, edited, coupled — keeps the in-memory version, as before. Deleted-during-wait and corrupt-file behaviour are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The finding kept from disk is someone else's decision. Assert it is not written into this run's verification ledger, so keeping it grants nothing the concurrent writer didn't already have. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
elhoim
changed the title
This branch has not been deployed
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.
BLUF
98147af. Upstream's_keep_landednow keeps records staged during the approval wait — the original target of this PR. It also deliberately writes back an item deleted during the wait and warns on a corrupt re-read; this PR's full reload contradicted both (3 of upstream's tests failed against it). The reload is dropped._keep_landedonly appends ids it hasn't seen. A record that already existed but was changed on disk during the wait is overwritten from the stale copy read before it.approvals.jsonl, so the finding and the ledger disagree. Red in all three modes (specific ids, interactive, review) onmain.Priority: MEDIUM (was HIGH: the common case — items staged during the wait — is now fixed upstream; what remains needs two concurrent writers)
The remaining defect
All three approve modes read
findings.json/timeline.json/iocs.jsonbefore the wait (password prompt, per-item prompts,$EDITOR) and write the in-memory lists back after it.98147afappends records with new ids from disk; records with known ids are written from memory regardless of what happened to them on disk meanwhile.mainwrites98147af)98147af, by design)The fix
approve.py:_snapshot()records each list by id, as canonical JSON, right after it is read._keep_landed(..., read=snapshot)then swaps in the disk copy only where both hold:Deleted-during-wait and corrupt-file behaviour are untouched;
_keep_landedwithoutread=behaves exactly as before.Testing
test_a_concurrent_approval_of_an_untouched_finding_is_kept[ids|interactive|review]intests/test_approve_reject.py, written against upstream's ownwait_case/_runharness. Red onmainin all three modes; green here. Each also asserts the kept finding is not in this run's signed set.All of upstream's approve tests pass, including the three that the previous version of this PR broke.
CI status
Brought up to date with
main@b2324ac(761 tests). The checks may showaction_required— GitHub gates runs from first-time fork contributors until a maintainer approves them — so reproduced locally with the same steps asci.yml, including its unpinned latestruff:pytest tests/— Python 3.10pytest tests/— Python 3.11pytest tests/— Python 3.12ruff check .ruff format --check .History
Opened against
954a623as a full reload of both files after the review. Brought up to date by mergingmain(no force-push); the rescoped fix is a separate commit on top.🤖 Generated with Claude Code