Repository navigation
fix(*): keep archiving, fix the usage counts, and start conversations in smart mode - #585
Conversation
… fetched Opening the dialog reloads the config snapshot, and two pages do not read from it: the archive list and the usage totals are fetched by the pages themselves and kept on the store. Nothing dropped them, so a dialog opened once held its first answers for the life of the page -- a conversation archived from the rail in between never reached the archive page, and the usage numbers stayed at whatever they were when the dialog first went up. Both pages already load lazily off these two fields, so clearing them on open is the whole change. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…went Three ways the flag did not hold. session.archive persisted by saving the whole session, which rewrites the metadata record from that process's copy of it. A page and a terminal over one home are two managers over one file: the other one had loaded the conversation before the archive, so its copy carried no archived key, and its next save of anything -- a title, a model -- spoke for a flag it had never seen and the conversation came back. It now appends the one key, the way the auto-archive pass already did, so a writer only speaks for what it touched. append_metadata_patch reports whether the patch reached a file, which is what lets the verb answer pending truthfully. The rail read that pending as a success. For a conversation with no transcript yet nothing had reached the disk, yet the row left the rail and the reader met it again on the next load. The archive page asked session.list for fewer channels than the rail lists, so an archived scheduled run was invisible there and unreachable here: no restore, no delete. It now asks for the rail's own set. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
… total settings.usage read its range two ways. Calls, cost and the per-model table came from the daily telemetry files, dated per day. The tool table was topped up by reading session transcripts, and a transcript counted as in-range when its file mtime was -- so a conversation touched recently handed the range every tool call it had ever made, while its model calls, dated per day, stayed outside it. A page reporting thousands of tool calls beside no model calls at all reads as broken, and it was. Telemetry has carried a row per tool call for a while, so the transcript fallback only ever covered conversations older than that. Dropping it leaves one source and one reading of the range; transcripts are still read for session titles, with mtime as a floor rather than a window, because a file last written before the range cannot name a session the range saw. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
ask stopped the agent on every mutation a conversation made, and a prompt answered by reflex is not a gate. Smart is not the weaker setting it sounds like: builtin denials and user deny rules hold in every mode, the reviewer speaks only for the ask tier, and a reviewer that cannot run leaves the call at the same prompt ask would have shown. The value is decided by PermissionsConfig.mode. The other three say it again where they cannot read it: config.get reports the default and config.unset restores it, the web chip paints before the config arrives, and the TUI /perm line falls back when the engine answers nothing. The boot golden moves with the chip, whose shield has one path fewer in this tier. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: archive state can still be erased by a stale writer, and ACP editor sessions lose required ask-mode prompts.
I reviewed the complete target diff plus the affected archive/save callers, usage telemetry path, permission-mode consumers, commit history, and the Runtime/Web/TUI architecture and naming constraints. I also checked that tests were not weakened to manufacture green results and considered backward compatibility.
Verification:
uv run pytest tests/test_rpc_session.py tests/test_rpc_config.py tests/test_config_live.py -x- 267 passed.PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_rpc_settings.py -x- 94 passed.- Web focused tests - 40 passed; web type-check passed.
- TUI permission tests - 7 passed; TUI type-check passed.
- The dedicated ACP default-mode test fails locally, matching the failing GitHub unit shard.
- A direct two-manager reproduction shows
archived: trueimmediately after the patch, then noarchivedkey after the stale manager saves.
No other blocking or nonblocking findings survived refutation.
The hosting deliberately writes no permissions block and takes trunk's default, and that default moved. The note it left behind argued against opening the tier product-wide because it would take an editor user's prompt away for good; the smart tier does not do that, since the reviewer escalates whatever it will not vouch for and the person is asked then. Pinning ask here instead would ship two product defaults and leave that person unable to tell which one they are in. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: archive persistence must still survive a later save from a manager that loaded the session before it was archived.
Reviewed the delta from c0139daf1ac4, the full target diff, the affected permission-gate and session-manager callers, commit history, backward-compatibility implications, and the Runtime/Web/TUI architecture and repository rules. The changed ACP test was checked against the actual smart-mode waterfall rather than accepted merely because it is green: reviewer escalation and reviewer failure still reach the human responder, so that earlier finding has been withdrawn and its thread resolved.
The archive path is unchanged. The two-manager reproduction still reports archived: true after the patch and no archived value after the stale manager's next save(), so the original archive thread remains open. No new findings survived refutation.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_agents_code_launcher.py::test_the_acp_render_leaves_the_trunk_tier_alone tests/test_permissions_gate.py tests/test_rpc_session.py tests/test_rpc_settings.py tests/test_rpc_config.py tests/test_config_live.py -x passed all 582 tests. GitHub's previously failing unit shard is now green; one unrelated unit shard was still running when checked.
…g it The hosting wrote no permissions block and took trunk's default, which was the ask tier, and the note it left said that was on purpose: a person in an editor should still be asked. Trunk's default has moved to smart, where a reviewer speaks for that tier and lets most of it through. That is a product decision about raven's own surfaces; this is not one of them. The person here is watching an agent work on their checkout, and the prompt is how they see each write before it lands. setdefault, so an explicit permissions block in a custom config still wins, and raven dispatching a sub-agent is unaffected either way because raven/acp_client/permissions.py answers every prompt itself. The four other products carry no permissions block of their own and were written as things raven dispatches rather than things a person drives, so they follow trunk. Naming that here because it is a decision, not an oversight. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Both premises check out. One is fixed here; the other is real, is not introduced by this branch, and is being closed in the follow-up rather than here. ACP editor sessions losing the ask prompt -- taken, fixed in edeba53. You are right and I was wrong about it. I had argued that smart still asks, so the hosting could follow trunk. That misses who is on the other end: a person in an editor is watching an agent work on their checkout, and the prompt is how they see each write before it lands. A reviewer that lets most of the tier through takes that away for everything it vouches for. The other four products carry no permissions block of their own and were written as things raven dispatches rather than things a person drives, so they follow trunk. That is a decision, not an oversight, and it is named in the commit so the next person can reverse it in one place. Your note said the dedicated ACP test failed locally and matched the red unit shard. That was against c0139da; 8eea084 had already fixed the shard, and edeba53 is the one that answers the finding itself. Archive state erased by a stale writer -- premise correct, and this branch narrows it rather than closing it. Your two-manager reproduction is the same one I used to find the bug, and it still reproduces, because That is why the claim in the description was too wide, and I have narrowed it: the description now says what remains open and what closes it. Closing it properly needs If you would rather this branch not ship until the whole class is closed, say so and I will hold it behind the follow-up instead. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the delta from 8eea084d232e, the full target diff, render_config callers and history, the permission and session persistence paths, backward compatibility, the changed tests, and the Runtime/Web/TUI architecture and repository rules. The ACP fix now pins ask only for the attended editor hosting, leaves the unattended one-shot full behavior intact, and preserves explicit custom permission modes through setdefault. No new finding survived refutation.
Named follow-up: a stale manager can still erase archive state through a later whole-metadata save(). The maintainer has explicitly deferred the larger persistence change, and the PR description now documents the reachable limitation and why explicit key-removal semantics are a prerequisite. That thread has been closed as nonblocking rather than left as a contradictory open objection.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_agents_code_launcher.py tests/test_permissions_gate.py tests/test_rpc_session.py tests/test_rpc_settings.py tests/test_rpc_config.py tests/test_config_live.py -x passed all 651 tests. The completed frontend, lint, repository, trajectory, Windows, and wheel checks visible on this head were also green when checked.
… with Two changed branches had no case on them, and the diff-coverage gate was right to say so. A refusal from the filesystem is the one path where the archive verb answers pending for a session that does have a transcript, and a client reading that as a success takes the row off its list; the title lookup is the half of the transcript scan that survived this branch, and nothing was asserting which sessions it answers for. The transcript helper gained a channel, because a title is only looked up for a session the telemetry named and the two fixtures were on channels that could never meet. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the test-only delta from edeba531928a, the full target diff, the exercised session and usage callers, history, backward compatibility, test strength, and the Runtime/Web/TUI architecture and repository rules. The new cases cover previously unasserted failure and title-filtering branches without weakening an existing expectation. No new finding survived refutation or meets the fourth-round blocker bar.
Named follow-up remains the already-documented whole-metadata stale-writer archive limitation; the maintainer has scoped that work out of this PR, and both prior review threads remain resolved.
Verification: PYTHONPATH=plugins-dist/everos-memory uv run pytest tests/test_agents_code_launcher.py tests/test_permissions_gate.py tests/test_rpc_session.py tests/test_rpc_settings.py tests/test_rpc_config.py tests/test_config_live.py -x passed all 653 tests. git diff --check passed. Completed CI checks were green when checked; the four unit shards were still running.
## Summary The follow-up #585 named. Archiving a conversation from one client and then saying anything in another put it straight back, and #585 only narrowed that: it stopped the archive verb from erasing what other writers had added, not other writers from erasing the archive. The cause is one line of behaviour. Saving rewrites the whole metadata record from one copy of it, so it speaks for every key that copy happens to hold. A page and a terminal over one home are two managers over one file: whichever saves second wins the whole record, and the flag written after that copy was loaded is gone. Six places did it -- five verbs that each meant one key, and the turn path, which saves on every message and is the one a person actually hits. Two changes, in this order because the second depends on the first. **Every metadata write names the key it means.** Pin, rename, the per-conversation model and permission mode, and the ACP usage owner append the one key they mean, the way the auto-archive pass and (since #585) the archive verb already did. Unpinning writes `pinned: False` and a hand-typed title writes `title_auto: False`, because an appended key can set a key and not remove one. Every reader of both asks whether the flag is true, so the two spellings read alike. **A save keeps the keys it never touched.** The record it writes is now this copy's metadata over the record on disk, read and written inside one write transaction so nothing lands between the two. The re-read is skipped while the file is byte for byte what this copy last read or wrote -- which is every save while one writer has the conversation, so the scan is paid for only when somebody else has actually written to it. That merge cannot tell a key this copy dropped from one it never had, which is why the removers had to be converted first. The last one was the output-limit marker in the turn path; it writes `None`, and its reader asks for an int. The rule now holds across the file and is stated on `_metadata_to_write`: a remover that clears by omission will find its key handed back. ## Type - [x] Fix ## Verification ``` uv run --all-extras pytest -q # 24148 passed, 111 skipped, 4 failed ``` The four are this machine's, not the branch's: checked out the base revision with these changes removed and they fail identically there, then restored and compared the files byte for byte. Two are the vendored tool-face pair, which this checkout fails because its virtualenv carries `web_search` and CI's does not (both are green on CI); one is a token-budget case; one needs a cairo library this machine has not got. ``` uv run ruff check raven tests # All checks passed uv run ruff format --check raven tests # already formatted ``` Every new case was checked against the base revision with the fix removed, by restoring the file from HEAD rather than stashing, and the files were compared byte for byte afterwards: | Case | On base | |---|---| | `test_a_save_keeps_a_key_another_writer_added` | fails: the conversation is un-archived by the other writer's next save | | `test_a_save_does_not_resurrect_a_key_this_copy_cleared` | fails | | `test_a_save_re_reads_only_when_the_file_moved` | fails: it re-reads every time | | `test_pinning_and_renaming_keep_a_key_another_writer_added` | fails: the foreign key is gone after either verb | | `test_a_conversation_mode_keeps_a_key_another_writer_added` | fails | The first of those is the two-manager reproduction from the review on #585, and it is the one that says this is closed. Three more cases cover branches the diff-coverage gate found bare: a filesystem that refuses the append, on both pin and rename, and a transcript that has lost its metadata record. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk `save` is the hottest write path in the product, and it gains a `stat` per call plus a full read of the transcript when that `stat` shows the file moved under it. The stamp is what keeps the read off the ordinary path: one writer saving its own conversation never pays it. The read is bounded by the transcript's length and happens once per foreign write, not once per turn. The merge is only safe while no writer clears a key by leaving it out. All three that did have been converted, and the rule is stated where the merge is, but it is a convention a future writer can break quietly. A test pins the behaviour from the clearing side rather than only the keeping side. Rolling back is the two commits; nothing is written to disk that an older build cannot read, since the added values -- `pinned: false`, `title_auto: false`, `output_limit_turn_at: null` -- are all falsy where an older build expected the key to be missing. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
## Summary Restoring a conversation from the archive page put it back everywhere except the screen. `loadSessions` replaced the rail's rows and never drew them, and the restore reaches the rail only through that function, so the row came back in the server's listing and on disk while the rail kept showing the list from before. Reloading the page was what appeared to restore it. Every other writer in this feature already draws after a replace -- `leave.ts` does it on both of its transitions, the session registry does it after its reconcile -- so the fix is the missing call in the one path that did not, rather than a draw at the restore call site. At boot the rail is still held, where a draw sets the skeleton and `releaseRail` paints the rows, so the other caller is unaffected. Found while driving the merged archive flow end to end on a real host, after #585, #589 and #596. It is the same shape as #596: server state correct, screen stale until a reload. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification On a real host (`raven web` on an isolated `RAVEN_HOME` built from a real config, driven through the browser), reading the archived flag from the last metadata record of the session transcript rather than from the page: | Step | Before | After | | --- | --- | --- | | Archive a conversation from the rail | leaves the rail, `archived: true` on disk | same | | Settings, Archive page | it is listed | same | | Restart the gateway | still archived, does not come back | same | | Restore it from the Archive page | `archived: false` on disk, **rail still does not show it** | rail shows it at once | | Reload the page | rail shows it | rail shows it | Commands: - `npm test --prefix ui-web` -- 189 files, 2470 tests, all pass - `npm run lint --prefix ui-web` -- 0 errors (4 pre-existing warnings in CronPage/SubagentsPage, untouched here) - `npm run type-check --prefix ui-web` -- clean - `npm run --prefix ui-web build && python3 ui-web/build.py` -- boot-snapshot OK (235 nodes match golden) - `npx commitlint --from github/refactor/ui_web_architecture --to HEAD` -- clean The new case, `a plain re-read draws the rows it just replaced`, was run against the unfixed module first and fails there on the assertion it is named for. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk One added call in a function with two callers; the other one runs while the rail is held, where the draw is a no-op beyond the skeleton it already sets. No server change, no stored state change. Rollback is reverting one commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… in smart mode (#585) ## Summary Three reports from acceptance testing, one branch. All three were reproduced on a real host before anything was changed, and the fixes were walked through the same host afterwards. **Archiving did not hold, and the archive page did not show it.** Three separate causes: - `session.archive` persisted by saving the whole session, which rewrites the metadata record from that process's copy of it. A page and a terminal over one home are two managers over one file: the other one had loaded the conversation before the archive, so its copy carried no `archived` key, and its next save of anything spoke for a flag it had never seen. The verb now appends the one key, the way the auto-archive pass already did. `append_metadata_patch` reports whether the patch reached a file, which is what lets the verb answer `pending` truthfully. - The rail read that `pending` as a success, so a conversation with no transcript yet left the rail although nothing had reached the disk. - The archive page asked `session.list` for fewer channels than the rail lists, so an archived scheduled run was invisible there and unreachable here. **The usage page reported no data.** It read its range two ways. Calls, cost and the per-model table came from the daily telemetry files, dated per day; the tool table was topped up by reading session transcripts, and a transcript counted as in-range when its file mtime was. A conversation touched recently therefore handed the range every tool call it had ever made while its model calls, dated per day, stayed outside it -- thousands of tool calls beside no model calls at all, which reads as a page that failed to load. Telemetry has carried a row per tool call for a while, so the transcript fallback only ever covered older conversations; dropping it leaves one source and one reading of the range. **A conversation now starts in the smart permission mode.** `ask` stopped the agent on every mutation, and a prompt answered by reflex is not a gate. Smart is not the weaker setting it sounds like: builtin denials and user deny rules hold in every mode, the reviewer speaks only for the ask tier, and a reviewer that cannot run leaves the call at the same prompt `ask` would have shown. **Shared by the first two:** opening the settings dialog reloads the config snapshot, and neither the archive list nor the usage totals come from it. Nothing dropped them, so a dialog opened once held its first answers for the life of the page. Both are now cleared on open, which their own lazy loads already key off. **What this does not close.** `save` rewrites the whole metadata record from one process's copy of it, and this branch changes who can do that rather than whether it can be done. Archiving no longer erases what another writer added; five other verbs still can (pin, rename, the per-conversation model and permission mode, the ACP usage owner), and so does the turn path, which saves on every message and is the one a user actually hits. So a conversation archived from one client and then typed into from another still comes back. Closing that needs `save` to stop speaking for keys it never touched, and removal is currently expressed by omission -- unpin, `title_auto` and `output_limit_turn_at` each delete a key by not writing it, so a merge on save would resurrect them. They have to become explicit false values first. That work is a round of its own rather than a fourth topic on this branch. The ACP hosting pins the ask tier rather than inheriting the new default: the person there is in an editor watching an agent work on their checkout, and the prompt is how they see each write before it lands. The four other products carry no permissions block and were written as things raven dispatches rather than things a person drives, so they follow trunk. ## Type - [x] Fix - [x] Feature ## Verification Backend, after the rebase: ``` uv run pytest tests/test_rpc_session.py tests/test_rpc_settings.py \ tests/test_rpc_config.py tests/test_config_live.py \ tests/test_rpc_contract_shapes.py tests/test_session_manager.py -q # 498 passed uv run ruff check raven tests # All checks passed uv run ruff format --check raven tests # 1636 files already formatted make check-source-language # pass make check-large-files # pass ``` Frontends, after the rebase: ``` cd ui-web && npx vitest run # 189 files, 2592 passed cd ui-web && npx tsc --noEmit # clean cd ui-tui && npx vitest run # 144 files, 2074 passed cd ui-tui && npm run type-check # clean ``` Every new or changed case was checked against the base revision with the fix removed, by restoring the file from HEAD rather than stashing, and the files were compared byte for byte afterwards: | Case | On base | |---|---| | `test_archiving_keeps_a_key_another_writer_added` | fails: the foreign `pinned` key is gone after the archive | | `test_usage_counts_a_tool_call_on_the_day_it_was_recorded` | fails: a transcript touched today adds two tool calls the range never saw | | `opening again clears what the pages fetched for themselves` | fails: both fields survive the reopen | | `says it failed when the call answers that nothing was persisted` | fails: `pending` is read as a success | | `archived lists the archived sessions of every channel the rail shows` | fails: the call carries no channels | Real host, `raven serve` on a temporary `RAVEN_HOME` with real provider credentials, driven through the built page: | Check | Before | After | |---|---|---| | Archive a conversation from the rail, then reopen Settings > Archive | the list still shows the previous answer | the conversation is at the top of the list | | Telemetry rows on disk vs the usage page's call count, dialog reopened without a page reload | 13 on disk, 12 on the page | 13 and 13 | | Tool table against the `tool_call` rows for the range | inflated by whole transcripts | `exec` 4, the number of rows | | A fresh page with no `permissions` key in the config and no stored pick | chip reads "ask" | chip reads "smart" | | `mkdir -p /tmp/...` through `exec` on that host | approval dialog, directory not created | reviewed and allowed, directory created | The last row is the pair that makes the check able to fail: the same command was run with `permissions.mode` set to `ask`, and it stopped at the prompt. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk The permission default is the user-visible change: an install that has never written `permissions.mode` moves from being asked about every mutation to having the reviewer speak for the ask tier. Denials and deny rules are unaffected, and a reviewer that cannot run still leaves the call at the prompt. Anyone who wants the old behaviour writes `permissions.mode: "ask"`, and rolling the default back is the one literal in `PermissionsConfig.mode` plus the three places that repeat it where they cannot read it. Usage numbers will change for anyone whose home has conversations older than the day telemetry started carrying tool rows: their tool counts drop to what the range actually saw. That is the point of the change, but it will look like a loss. `append_metadata_patch` gained a return value. The existing caller ignores it, so nothing else moves. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
## Summary The follow-up #585 named. Archiving a conversation from one client and then saying anything in another put it straight back, and #585 only narrowed that: it stopped the archive verb from erasing what other writers had added, not other writers from erasing the archive. The cause is one line of behaviour. Saving rewrites the whole metadata record from one copy of it, so it speaks for every key that copy happens to hold. A page and a terminal over one home are two managers over one file: whichever saves second wins the whole record, and the flag written after that copy was loaded is gone. Six places did it -- five verbs that each meant one key, and the turn path, which saves on every message and is the one a person actually hits. Two changes, in this order because the second depends on the first. **Every metadata write names the key it means.** Pin, rename, the per-conversation model and permission mode, and the ACP usage owner append the one key they mean, the way the auto-archive pass and (since #585) the archive verb already did. Unpinning writes `pinned: False` and a hand-typed title writes `title_auto: False`, because an appended key can set a key and not remove one. Every reader of both asks whether the flag is true, so the two spellings read alike. **A save keeps the keys it never touched.** The record it writes is now this copy's metadata over the record on disk, read and written inside one write transaction so nothing lands between the two. The re-read is skipped while the file is byte for byte what this copy last read or wrote -- which is every save while one writer has the conversation, so the scan is paid for only when somebody else has actually written to it. That merge cannot tell a key this copy dropped from one it never had, which is why the removers had to be converted first. The last one was the output-limit marker in the turn path; it writes `None`, and its reader asks for an int. The rule now holds across the file and is stated on `_metadata_to_write`: a remover that clears by omission will find its key handed back. ## Type - [x] Fix ## Verification ``` uv run --all-extras pytest -q # 24148 passed, 111 skipped, 4 failed ``` The four are this machine's, not the branch's: checked out the base revision with these changes removed and they fail identically there, then restored and compared the files byte for byte. Two are the vendored tool-face pair, which this checkout fails because its virtualenv carries `web_search` and CI's does not (both are green on CI); one is a token-budget case; one needs a cairo library this machine has not got. ``` uv run ruff check raven tests # All checks passed uv run ruff format --check raven tests # already formatted ``` Every new case was checked against the base revision with the fix removed, by restoring the file from HEAD rather than stashing, and the files were compared byte for byte afterwards: | Case | On base | |---|---| | `test_a_save_keeps_a_key_another_writer_added` | fails: the conversation is un-archived by the other writer's next save | | `test_a_save_does_not_resurrect_a_key_this_copy_cleared` | fails | | `test_a_save_re_reads_only_when_the_file_moved` | fails: it re-reads every time | | `test_pinning_and_renaming_keep_a_key_another_writer_added` | fails: the foreign key is gone after either verb | | `test_a_conversation_mode_keeps_a_key_another_writer_added` | fails | The first of those is the two-manager reproduction from the review on #585, and it is the one that says this is closed. Three more cases cover branches the diff-coverage gate found bare: a filesystem that refuses the append, on both pin and rename, and a transcript that has lost its metadata record. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk `save` is the hottest write path in the product, and it gains a `stat` per call plus a full read of the transcript when that `stat` shows the file moved under it. The stamp is what keeps the read off the ordinary path: one writer saving its own conversation never pays it. The read is bounded by the transcript's length and happens once per foreign write, not once per turn. The merge is only safe while no writer clears a key by leaving it out. All three that did have been converted, and the rule is stated where the merge is, but it is a convention a future writer can break quietly. A test pins the behaviour from the clearing side rather than only the keeping side. Rolling back is the two commits; nothing is written to disk that an older build cannot read, since the added values -- `pinned: false`, `title_auto: false`, `output_limit_turn_at: null` -- are all falsy where an older build expected the key to be missing. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
## Summary Restoring a conversation from the archive page put it back everywhere except the screen. `loadSessions` replaced the rail's rows and never drew them, and the restore reaches the rail only through that function, so the row came back in the server's listing and on disk while the rail kept showing the list from before. Reloading the page was what appeared to restore it. Every other writer in this feature already draws after a replace -- `leave.ts` does it on both of its transitions, the session registry does it after its reconcile -- so the fix is the missing call in the one path that did not, rather than a draw at the restore call site. At boot the rail is still held, where a draw sets the skeleton and `releaseRail` paints the rows, so the other caller is unaffected. Found while driving the merged archive flow end to end on a real host, after #585, #589 and #596. It is the same shape as #596: server state correct, screen stale until a reload. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification On a real host (`raven web` on an isolated `RAVEN_HOME` built from a real config, driven through the browser), reading the archived flag from the last metadata record of the session transcript rather than from the page: | Step | Before | After | | --- | --- | --- | | Archive a conversation from the rail | leaves the rail, `archived: true` on disk | same | | Settings, Archive page | it is listed | same | | Restart the gateway | still archived, does not come back | same | | Restore it from the Archive page | `archived: false` on disk, **rail still does not show it** | rail shows it at once | | Reload the page | rail shows it | rail shows it | Commands: - `npm test --prefix ui-web` -- 189 files, 2470 tests, all pass - `npm run lint --prefix ui-web` -- 0 errors (4 pre-existing warnings in CronPage/SubagentsPage, untouched here) - `npm run type-check --prefix ui-web` -- clean - `npm run --prefix ui-web build && python3 ui-web/build.py` -- boot-snapshot OK (235 nodes match golden) - `npx commitlint --from github/refactor/ui_web_architecture --to HEAD` -- clean The new case, `a plain re-read draws the rows it just replaced`, was run against the unfixed module first and fails there on the assertion it is named for. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk One added call in a function with two callers; the other one runs while the rail is held, where the draw is a no-op beyond the skeleton it already sets. No server change, no stored state change. Rollback is reverting one commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Three reports from acceptance testing, one branch. All three were reproduced on a
real host before anything was changed, and the fixes were walked through the same
host afterwards.
Archiving did not hold, and the archive page did not show it. Three separate
causes:
session.archivepersisted by saving the whole session, which rewrites themetadata record from that process's copy of it. A page and a terminal over one
home are two managers over one file: the other one had loaded the conversation
before the archive, so its copy carried no
archivedkey, and its next save ofanything spoke for a flag it had never seen. The verb now appends the one key,
the way the auto-archive pass already did.
append_metadata_patchreportswhether the patch reached a file, which is what lets the verb answer
pendingtruthfully.
pendingas a success, so a conversation with no transcriptyet left the rail although nothing had reached the disk.
session.listfor fewer channels than the rail lists, soan archived scheduled run was invisible there and unreachable here.
The usage page reported no data. It read its range two ways. Calls, cost and
the per-model table came from the daily telemetry files, dated per day; the tool
table was topped up by reading session transcripts, and a transcript counted as
in-range when its file mtime was. A conversation touched recently therefore handed
the range every tool call it had ever made while its model calls, dated per day,
stayed outside it -- thousands of tool calls beside no model calls at all, which
reads as a page that failed to load. Telemetry has carried a row per tool call for
a while, so the transcript fallback only ever covered older conversations; dropping
it leaves one source and one reading of the range.
A conversation now starts in the smart permission mode.
askstopped the agenton every mutation, and a prompt answered by reflex is not a gate. Smart is not the
weaker setting it sounds like: builtin denials and user deny rules hold in every
mode, the reviewer speaks only for the ask tier, and a reviewer that cannot run
leaves the call at the same prompt
askwould have shown.Shared by the first two: opening the settings dialog reloads the config
snapshot, and neither the archive list nor the usage totals come from it. Nothing
dropped them, so a dialog opened once held its first answers for the life of the
page. Both are now cleared on open, which their own lazy loads already key off.
What this does not close.
saverewrites the whole metadata record from oneprocess's copy of it, and this branch changes who can do that rather than whether
it can be done. Archiving no longer erases what another writer added; five other
verbs still can (pin, rename, the per-conversation model and permission mode, the
ACP usage owner), and so does the turn path, which saves on every message and is
the one a user actually hits. So a conversation archived from one client and then
typed into from another still comes back.
Closing that needs
saveto stop speaking for keys it never touched, and removalis currently expressed by omission -- unpin,
title_autoandoutput_limit_turn_ateach delete a key by not writing it, so a merge on save would resurrect them. They
have to become explicit false values first. That work is a round of its own rather
than a fourth topic on this branch.
The ACP hosting pins the ask tier rather than inheriting the new default: the
person there is in an editor watching an agent work on their checkout, and the
prompt is how they see each write before it lands. The four other products carry no
permissions block and were written as things raven dispatches rather than things a
person drives, so they follow trunk.
Type
Verification
Backend, after the rebase:
Frontends, after the rebase:
Every new or changed case was checked against the base revision with the fix
removed, by restoring the file from HEAD rather than stashing, and the files were
compared byte for byte afterwards:
test_archiving_keeps_a_key_another_writer_addedpinnedkey is gone after the archivetest_usage_counts_a_tool_call_on_the_day_it_was_recordedopening again clears what the pages fetched for themselvessays it failed when the call answers that nothing was persistedpendingis read as a successarchived lists the archived sessions of every channel the rail showsReal host,
raven serveon a temporaryRAVEN_HOMEwith real provider credentials,driven through the built page:
tool_callrows for the rangeexec4, the number of rowspermissionskey in the config and no stored pickmkdir -p /tmp/...throughexecon that hostThe last row is the pair that makes the check able to fail: the same command was
run with
permissions.modeset toask, and it stopped at the prompt.Risk
The permission default is the user-visible change: an install that has never
written
permissions.modemoves from being asked about every mutation to havingthe reviewer speak for the ask tier. Denials and deny rules are unaffected, and a
reviewer that cannot run still leaves the call at the prompt. Anyone who wants the
old behaviour writes
permissions.mode: "ask", and rolling the default back is theone literal in
PermissionsConfig.modeplus the three places that repeat it wherethey cannot read it.
Usage numbers will change for anyone whose home has conversations older than the
day telemetry started carrying tool rows: their tool counts drop to what the range
actually saw. That is the point of the change, but it will look like a loss.
append_metadata_patchgained a return value. The existing caller ignores it, sonothing else moves.
Related Issues
N/A