Repository navigation
chore(room-ops): remove the context/doc verbs and doc_sync; the room vault is gone [Devault] - #5740
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff.
|
dfe5a49 to
867b82f
Compare
liususan091219
left a comment
There was a problem hiding this comment.
Review at 867b82f (not a draft on GitHub, but the body says "do not merge yet"). Verdict: COMMENT - the removal itself is clean; nothing here blocks, and approval waits on the stated holds plus a reviewer approval at this head.
What I checked
-
Completeness. At the head,
git grepfinds no remaining caller, test, doc, skill, manifest, CLAUDE.md or AGENTS.md reference toroom_ops context/room_ops doc,doc.py,doc_sync,doc_get/doc_put/doc_rm,prep_get/prep_put/prep_delete,ROOM_DOC_*,room-live-contextorstate/room-doc-sync. The other "vault" hits in the repo are the secret vault, the workspace-sync vault and_gateway.py's Keychain tier, which are unrelated, as the body says. Two stale comments are left, both inskills/agent-room-ops/events.py:- line 12: "same shape as doc/mention" - there is no doc op any more.
- line 70: "Mirrors doc._call - same envelope, same degrade map." -
doc._callno longer exists, so this cross-reference points at nothing.
Neither changes behaviour. A one-line edit each before merge would finish the sweep.
-
Outside this repo. The desktop-shipped
room-commonsskill (not in sonichi/sutando) still ends its description with "Notroom_ops context, which holds a room's stored Context files." After this lands, that steers agents toward a verb that is gone. It needs a follow-up in whichever repo ships that skill, merged around the same time as this PR. It is not a defect in this PR. -
Data safety. This PR deletes only client code. The room-vault content stays on the server, and its fate belongs to the backend B2 PR. Locally,
doc_sync getwrote base copies under<workspace>/state/room-doc-sync/*.base. They become orphaned but harmless, since they are only merge bases and the remote holds the real content. Nothing on an existing install is deleted. One question for B2: once these verbs are gone, agents have no client path left to read the vault. Is any vault content (room-live-context, room-todo, room-memo, room-live-transcript) being exported or migrated to Commons before the store is dropped? Or is it confirmed disposable? A sentence in the body would settle it. -
Replacement. #5738 (merged) already pointed agents at Room Commons pages and databases. This PR removes the last SKILL.md rows and the vault line, and no agent-facing instruction in the repo still names the removed verbs. At the head,
room_ops.py doc getexits 2 with argparse's invalid-choice error, which is the intended loud failure for any remaining external caller, such as the qingyun-001 PR-shepherd cron named in the body. -
Tests. The deleted tests covered only the removed verbs. Nothing left asserts on removed code, and
python-suite-costs.txtplus the old-collab-literals baseline (now{}) are consistent. Locally at the head, all of these exit 0:skills/agent-room-ops/test_room_ops.py(164 tests OK), everytests/agent-room-ops-*,tests/room-ops-*andtests/room_ops_*,tests/lint-old-collab-literals.test.pyandtests/python-ci-legs-partition.test.py.scripts/lint-old-collab-literals.pyreports 0 occurrences. CI is 33 success and 1 skipped (Windows).license/clais success (1 status context present).scripts/review-checks.shon the diff passes, and the added lines contain no hardcoded paths.
What approval still needs
-
The two holds in the body: B2 merged and the qingyun-001 cron off
room_ops doc. Since GitHub does not show this PR as a draft, consider marking it draft or labelling it until both are done, so the hold is not only in prose. -
At least one approving review at this head. There are none yet.
-
Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
There was a problem hiding this comment.
Changes requested at 867b82f, for one blocker.
Blocker: this PR turns main red once merged.
- #5704 added
tests/hermetic-env-sentinels.test.py:121to main after this PR's CI ran. That line still liststests/agent-room-ops-context-rename.test.py, which this PR deletes. - On the tree merged with current main (
git merge-tree --write-tree origin/main 867b82f7),test_room_ops_gateway_tests_read_no_outer_selectorfails with "no gateway module was imported: the case ran nothing". The same test passes on origin/main. - The two changes merge without a textual conflict. That's why GitHub shows MERGEABLE, and why CI here stays green: it ran on the old base (18c3121, 20 commits behind).
- Fix: rebase, remove or replace that tuple entry, and rerun CI.
Otherwise the removal is complete.
- At head there are no callers of
prep_get/put/delete,doc_sync,doc.pyorroom_ops doc|context. The same grep finds 56 hits at the parent. - Every
_gatewayhelper is still used. The old-collab-literals baseline is{}, andpython-suite-costs.txtmatches the CI partition (1211 suites). room_ops docnow exits 2 with argparse's invalid-choice error.- Room-ops, lint and partition tests pass at head.
- §15 doesn't apply here. This removes a CLI verb and doesn't change a bridge, the delivery loop or startup.
Non-blocking
- Two stale comments:
skills/agent-room-ops/events.py:12("same shape as doc/mention") and:70("Mirrors doc._call"). - The desktop-shipped
room-commonsskill still says "Notroom_ops context". It needs a follow-up in the repo that ships it. - The body says "Draft, do not merge yet", but the PR isn't marked as a draft. Please mark it draft until B2 (ag2space-backend#2858) and the qingyun-001 cron change land.
- The
/observeexample andtests/observe-policy.test.py:28still useartifact.updated, which B2 stops emitting.
After the rebase and that fix, and once the holds are met, this is ready from my side. Codex-core compatibility verification will be requested at the new head before I approve.
Review by Rui's Claude core (agent-assisted).
…vault is gone [Devault] The doc verbs read and wrote the AG2 Space room vault through the broker's prep_get/prep_put/prep_delete ops, which the backend removes. Durable room state lives in Room Commons pages and databases. Delete doc.py, doc_sync.py, the context/doc subcommand, their tests, and the doc_sync literal baseline.
|
Addressed at 1570266:
Holds: qingyun-001 cron is off |
867b82f to
1570266
Compare
|
@cla-assistant check |
Withdrawn at 1570266. The blocker is fixed: the PR is rebased on main, tests/hermetic-env-sentinels.test.py no longer lists the deleted context-rename suite (19/19 pass at this head, including test_room_ops_gateway_tests_read_no_outer_selector), and the stale events.py comments are tidied. My APPROVE waits on Codex-core compatibility verification at this head, and the stated merge holds (B2 #2858, the qingyun-001 cron) still apply.
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 1570266. Verdict: COMMENT. The code is ready from my side and no change is needed; approval waits only on a reviewer approval at this head and the stated B2 hold.
Since my COMMENT at 867b82f
- Both stale comments I named are fixed: skills/agent-room-ops/events.py:12 now says "same shape as mention", and :70 no longer points at doc._call.
- The blocker in john-the-dev's change request is fixed. The PR is rebased, and tests/hermetic-env-sentinels.test.py:121 no longer lists the deleted context-rename suite. I checked this on the tree merged with current main (f4f09d7, two commits ahead of the base, no conflict): test_room_ops_gateway_tests_read_no_outer_selector passes. That is the test John showed failing.
- The range-diff shows the main commit is unchanged by the rebase (867b82f = bea0116). The only new code is 1570266, which changes 3 lines in 2 files.
- The qingyun-001 cron hold is reported done in the author's 06:41Z comment. The room-commons skill line is going to a separate PR, and the /observe artifact.updated example is kept on purpose with a stated reason. I accept both.
Removal sweep, on the merged tree
- git grep finds no remaining caller, doc or test of room_ops context/doc, doc.py, doc_sync, doc_get/put/rm, prep_get/put/delete, room-live-context or room-doc-sync. The only ROOM_DOC_ hits are scripts/lint-old-collab-literals.py:22 and its test fixture (tests/lint-old-collab-literals.test.py:67). Those are the lint's own patterns, and they are correct to keep.
- Coverage of kept behaviour: the deleted 404-legibility suite reached _gateway.degrade_reason_from only through doc._call. That helper is still used by events.py, and its 401/403/404 cases stay pinned directly in skills/agent-room-ops/test_room_ops.py:1657-1671 and tests/room-ops-events-server-reason.test.py. writer_id was doc_sync-only. I found no coverage hole.
- These all exit 0: skills/agent-room-ops/test_room_ops.py, every tests/agent-room-ops-, tests/room-ops- and tests/room_ops_* suite, tests/lint-old-collab-literals.test.py and tests/python-ci-legs-partition.test.py. scripts/lint-old-collab-literals.py reports 0 occurrences. room_ops.py doc get exits 2 with argparse's invalid-choice error. scripts/review-checks.sh on the diff passes.
- Limit of my local run: three other hermetic-env-sentinels cases (core_heartbeat writer) fail on this host. They fail identically on origin/main, so the cause is this host, not this PR.
CI at this head
- 29 success, 2 skipped. The 2 failures come from one test file: tests/skills/process-hygiene/reap-orphans.test.py, "changes between scan and signal" (change='cwd'), in python leg 7/9. The "tsc + tests (clean install)" job only reports that leg. This is the open flake #5733, and this diff does not touch process-hygiene. A rerun of that leg should clear it.
- license/cla is success, with 1 status context present. There are no unresolved review threads.
Merge-readiness: not ready to merge yet. The diff needs no further change. These are still missing:
- An approving review at 1570266. None exists at any head. John withdrew his change request and said his approval waits on Codex-core compatibility verification at this head.
- The B2 backend hold that the body names. I cannot see that repository from here, so I cannot confirm its state.
- Nit, not blocking: the body still says "rebased on main (18c3121)" and lists the cron as an open hold. Updating it, or marking the PR as a draft until B2 lands, would keep the hold visible outside the prose.
Blast radius: this deletes client code only, and on an existing install nothing is deleted beyond orphaned *.base merge copies. An external caller still using room_ops doc gets a loud exit 2, not a silent no-op.
- Sutando (susans-MacBook-Pro)
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at 1570266. No caller of room_ops doc/context, doc_sync, prep_* or the removed functions remains anywhere in the repo, crons included. The room-commons SKILL.md disambiguation row is already gone upstream. The gateway-token Keychain code is untouched, CI is green, and the baseline change is only the doc_sync.py removal.
Optional nit: room_ops doc|context now hits argparse's bare "invalid choice". A short stub that prints "retired; use Room Commons (room-commons skill / room.commons.* Actions)" would help agents following older notes. Not blocking. LGTM.
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at 1570266. Verdict: COMMENT. I found nothing blocking. Approval is pending Codex-core verification at 1570266.
My earlier blocker (CR at 867b82f) is resolved.
tests/hermetic-env-sentinels.test.py:121no longer lists the deletedagent-room-ops-context-rename.test.py.- I merged the head with current main (
git merge-tree --write-tree origin/main 15702667, no conflict; main is 2 commits ahead, f4f09d7, and neither touches room-ops). On that tree,tests/hermetic-env-sentinels.test.pyruns 19 tests, OK. That includestest_room_ops_gateway_tests_read_no_outer_selector, the test that failed before.
The rest of what I raised:
- Both stale
events.pycomments (:12, :70) are fixed. - The B2 hold is met: ag2space-backend#2858 merged at 2026-10-21T18:32:05Z.
- The body no longer says "do not merge yet". The PR is not a draft, so draft status is no longer needed.
- The
room-commonsline and the/observeartifact.updatedexample go to other PRs, for the reasons the author gave. I accept both.
Removal sweep, on the merged tree: git grep for room_ops doc|context, doc_sync, doc.py, doc._call, prep_get/put/delete, room-live-context, room-doc-sync and the three deleted test names returns 0 files. The same pattern returns 11 files on origin/main, so the grep can match.
Tests on the merged tree, all rc=0:
skills/agent-room-ops/test_room_ops.py(164 OK)- every
tests/agent-room-ops-*,tests/room-ops-*andtests/room_ops_*suite (17 files) tests/lint-old-collab-literals.test.pytests/python-ci-legs-partition.test.pytests/hermetic-env-sentinels.test.py(19 OK)
Also:
scripts/lint-old-collab-literals.py: 0 occurrences.room_ops.py doc getandroom_ops.py contextexit 2 with the invalid-choice error.scripts/review-checks.sh --diff <gh pr diff 5740>: PASS.- All 15 required checks pass at this head.
Non-blocking: I agree with @sonichi's nit. A one-line "retired; use Room Commons" message for doc/context would help agents that follow older notes.
Review by Rui's Claude core (agent-assisted).
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at 1570266, merged locally with current main (a1f8f324e, no conflict):
test_room_ops.py: 164 tests OK.tests/hermetic-env-sentinels.test.py: OK.tests/lint-old-collab-literals.test.py: OK.git grepforroom_ops doc|context,doc_syncandprep_getreturns nothing.room_ops.py doc …now exits with argparse's invalid-choice error.
The server side is already gone. With backend #2858 deployed, a context get from current main returns ok:false with "not allowed: the room vault is retired", so these verbs can only fail now. Removing them is correct.
Non-blocking, and agreeing with sonichi's nit: an invalid-choice error doesn't tell an agent working from old notes where the documents went. A stub doc/context subcommand that prints "retired: use Room Commons (room.commons.* Actions)" and exits 2 would tell it.
What
Removes the room-ops verbs that read and wrote the AG2 Space room vault, which is being retired:
skills/agent-room-ops/doc.pyanddoc_sync.pyroom_ops.py: removes thecontextsubcommand and itsdocalias (parser + dispatch)SKILL.md: removes the two retiringdocrows and the last vault line. docs(room-ops): durable room state goes to Room Commons; the room vault is retiring [Devault] #5738 already moved agents to Room Commons.tests/agent-room-ops-{doc-404-legibility,doc-sync-writer-id,context-rename}.test.py,DocTestsandtest_doc_put_missing_file_structured_errorinskills/agent-room-ops/test_room_ops.pyscripts/old-collab-literals.baseline.json:doc_sync.pywas its only entry, so the baseline is now{}(regenerated with--update)tests/python-suite-costs.txt: drops the two deleted test filesKept:
_gateway.pyandtests/room_ops_gateway_vault.test.py. That "vault" is the local Keychain holding the gateway token, not Forgejo.Evidence
room_ops.py capabilitiesandroom_ops.py doc get:Tests at HEAD:
skills/agent-room-ops/test_room_ops.py, alltests/agent-room-ops-*,tests/room-ops-*,tests/room_ops_*,tests/lint-old-collab-literals.test.pyandtests/python-ci-legs-partition.test.pypass.python3 scripts/lint-old-collab-literals.pyreportsok (0 occurrences in 0 files).git diff origin/main | bash scripts/review-checks.shpasses.tests/agent-room-ops-read-redaction.test.pyfails locally the same way on origin/main (detect-secrets is missing on this host). Unrelated.