feat(update): refresh Claude and Codex integrations together - #2112
Conversation
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 60d62d915e7bc65dc80b635149e9d3216fdf50a3 |
| Request ID | req_1786678152_1754 |
| Review record | 899f4179-b20d-46e9-91b5-336e0ac9562a |
What Improved
- Adds
--runtime allwith scoped Claude and Codex plugin refresh commands. - Preserves the configured execution backend by using the non-config-mutating
setup refreshpath. - Uses the receipt-proven Ouroboros console for artifact refresh and documents host restart behavior consistently across both skill copies and CLI documentation.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh the Claude plugin | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Partially met — the command runs, but partial artifact failures are reported to the updater as success |
| Preserve the configured execution backend rather than selecting either host | Met |
| Document Claude plugin reload and required Codex session restart behavior | Met |
Prior Findings Status
No prior ouroboros-agent review rounds were present. Human and inline review-comment artifacts were empty.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/update.py:507 | BLOCKING | The new all-runtime path cannot detect partial setup refresh failures. _refresh_all_runtime_artifacts treats exit status zero as complete success, but refresh_artifacts deliberately catches failures such as Codex artifact installation OSErrors, prints a warning, and still returns zero (src/ouroboros/cli/commands/setup.py:5001). The existing symlink-refusal test confirms this contract: refresh warns yet exits successfully. Consequently, ouroboros update --runtime all can print both “Refreshed installed runtime artifacts” and the final green “Updated” result while requested artifacts remain stale. Propagate a partial-failure status from setup refresh—while still attempting the remaining artifacts—so the updater reports a partial update and exits nonzero, and add an end-to-end updater test for this path. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | tests/unit/cli/test_update.py:1131 | Medium | High | Add the host-availability matrix for --runtime all: Claude-only, Codex-only, both installed, and neither installed. Current flow tests mock both hosts as present, leaving the intentional None/skip branches and resulting guidance unverified. |
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/update.py:939 | User messaging | The Claude/Codex restart sentence is unconditional for --runtime all, so it mentions Codex restart and Claude reload even when one or both CLIs were absent and skipped. Tailor this guidance to refreshed_hosts. |
| 2 | docs/cli-reference.md:876 | Documentation | “Refreshes the selected host plugin integration” also describes single-runtime updates, but the Codex marketplace refresh occurs only for --runtime all; explicit --runtime codex continues through setup without _refresh_codex_plugin. Clarify the mode-specific behavior. |
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_update.py— 184 passed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/commands/test_setup_refresh.py— 10 passed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/skills/test_skill_artifacts.py -k 'not test_path_shaping_poison_really_breaks_an_unsanitized_python'— 50 passed, 5 deselected.- Installed Codex CLI help confirms
plugin marketplace upgradeis a valid command and accepts a marketplace name. - No full-suite execution was performed during this review.
Design Notes
The orchestration is appropriately centralized in the receipt-bound native updater, and setup refresh is the correct configuration-preserving primitive. Its best-effort success contract, however, is incompatible with using its process status as proof that every requested artifact refresh succeeded.
Design / Roadmap Gate
Package upgrade and post-upgrade version verification remain bound to the proven installation identity. The new path does not rewrite runtime configuration or MCP state and uses scoped marketplace operations. Compatibility for missing host CLIs is graceful, but the required artifact-refresh step lacks an observable partial-failure state, breaking the updater’s result and exit-status contract.
Directional Notes
Review focused on updater ownership, preserved dependency/install identity, exact host executable selection, backend non-reselection, marketplace scope, documentation parity, and failure propagation. Maintainer memory was used only to direct inspection; the blocker is independently demonstrated by the current updater and setup-refresh contracts.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_update.py— 184 passed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/commands/test_setup_refresh.py— 10 passed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/skills/test_skill_artifacts.py -k 'not test_path_shaping_poison_really_breaks_an_unsanitized_python'— 50 passed, 5 deselected.- Installed Codex CLI help confirms
plugin marketplace upgradeis a valid command and accepts a marketplace name. - No full-suite execution was performed during this review.
Merge Recommendation
Request changes until partial setup refresh failures propagate to ouroboros update --runtime all and are covered by an updater-level regression test. The remaining implementation direction is sound.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 60d62d9
request_id: req_1786678152_1754
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Addressed the remaining requested change in 4df0fa0. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 4df0fa0721fed2686a10f89e80363f5d255ef962 |
| Request ID | req_1786808936_1857 |
| Review record | e15536de-adb5-4098-9cf8-d9c9a6753639 |
What Improved
- Adds
--runtime allto refresh Claude and Codex integrations without reselecting the configured execution backend. - Keeps package upgrade and post-upgrade verification bound to the detected installation identity and environment-local console script.
- Scopes the Codex operation to the Ouroboros marketplace and synchronizes the CLI reference and mirrored update skills.
- Propagates a nonzero
setup refreshsubprocess result into the updater’s partial-update exit status.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex host integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin and previously installed runtime artifacts | Partially met — successful paths run, but several artifact failures are silently reported as success |
| Preserve the configured execution backend rather than selecting a host backend | Met |
| Continue refreshing remaining integrations after a per-runtime artifact failure and exit nonzero for incomplete refreshes | Partially met — implemented for caught Codex/Hermes failures, not for false-returning or internally swallowed failures |
| Document Claude reload versus Codex session restart behavior | Met |
Prior Findings Status
The prior concern is partially addressed: caught Codex/Hermes exceptions can now produce a nonzero setup refresh status, and the updater converts a nonzero artifact-refresh subprocess result into a partial-update result. The same failure-observability standard remains unmet for other supported artifact helpers, as independently demonstrated from the current source.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:5027 | BLOCKING | setup refresh still reports success for several artifact installation failures, so update --runtime all can exit 0 and claim the installation was fully updated while artifacts remain stale. The OpenCode, Pi, and GJC bridge installers return False on write/source failures, but those return values are not added to failed; _install_runtime_instruction_artifact() catches OSError and returns no status, while its OpenCode/Gemini/Kiro/Copilot/GJC callers unconditionally append the runtime to refreshed; and _install_hermes_artifacts() swallows FileNotFoundError before the new outer handler can observe it. A focused current-source probe produced exit code 0 plus “Refreshed runtime artifacts: gemini” after injecting a Gemini OSError, and exit code 0 after an existing OpenCode bridge refresh returned False. Make every helper expose success/failure and record each failed runtime while continuing with the remaining artifacts, then add regressions for at least one false-return bridge and one swallowed instruction-write failure through both setup refresh and update --runtime all. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_update.py tests/unit/cli/commands/test_setup_refresh.py: 195 passed.- Skill artifact suite: 50 passed, 5 deselected.
- Ruff check passed and all four changed Python files were already formatted.
- Current tests cover Codex symlink refusal and aggregate subprocess failure, but not false-returning bridge installers or instruction installers that internally swallow
OSError.
Design Notes
The all orchestration is appropriately centralized in the native updater and delegates presence-gated artifact work to setup refresh. The remaining flaw is that the delegated command lacks one consistent result contract across its heterogeneous installer helpers.
Design / Roadmap Gate
The package-upgrade identity and backend-preservation boundaries are intact, and setup refresh does not rewrite runtime configuration or MCP registrations. However, update --runtime all treats the child command’s exit status as the authoritative completion signal, while multiple child operations suppress or return failures without affecting that status. This breaks the cross-command failure contract and can leave persistent installed artifacts stale behind a successful update result.
Directional Notes
Review focus included installation ownership/profile preservation, exact executable selection, backend non-reselection, scoped marketplace mutation, documentation parity, and observable partial failure. Maintainer memory was advisory only; the blocker is established by current helper contracts and focused runtime probes.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_update.py tests/unit/cli/commands/test_setup_refresh.py: 195 passed.- Skill artifact suite: 50 passed, 5 deselected.
- Ruff check passed and all four changed Python files were already formatted.
- Current tests cover Codex symlink refusal and aggregate subprocess failure, but not false-returning bridge installers or instruction installers that internally swallow
OSError.
Merge Recommendation
Request changes until every attempted artifact refresh contributes a truthful success/failure result, failures remain isolated so later runtimes are attempted, and focused tests prove those failures reach the updater’s nonzero partial-update result.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 4df0fa0
request_id: req_1786808936_1857
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Pushed Validation: 26 focused refresh/update tests passed; Ruff/diff checks passed. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 6a00d4c59c8f12b640b458514851d940f3de1605 |
| Request ID | req_1786811805_1868 |
| Review record | 83bc4dc7-0963-426c-bc7f-d00a5c4a538e |
What Improved
- Adds
--runtime allto refresh Claude and Codex host integrations together without changing the configured execution backend. - Scopes Codex marketplace refresh to the
ouroborosmarketplace and reuses the proven installation identity forsetup refresh. - Propagates partial artifact-refresh failures to the updater’s exit status while continuing with remaining runtimes.
- Adds focused regressions for Codex symlink refusal, installer false returns, instruction write errors, and end-to-end partial update reporting.
- Keeps the update skill documents and CLI reference synchronized with the new runtime and restart contract.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations together |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh installed runtime artifacts | Met |
| Preserve the configured execution backend rather than selecting Claude or Codex as the new backend | Met |
| Document that Claude can reload plugins while active Codex sessions require restart | Met |
| Ensure refresh failures are isolated, reported, and propagated as a nonzero update result | Partially met — the setup refresh path is covered, but the shared Hermes helper now silently weakens normal setup --runtime hermes failure handling |
Closes #2110 / references #2109 and openai/codex#31383 |
N/A — issue bodies are not included in the supplied review artifacts |
Prior Findings Status
Prior concerns about setup refresh swallowing per-runtime failures were addressed: current refresh helpers accumulate failures, continue with later runtimes, and return nonzero; the updater converts that status into a partial-update failure. Those prior blockers are withdrawn. The current blocker is a separate regression introduced by broadening the shared Hermes helper’s exception handling without updating its normal setup caller.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:2884 | BLOCKING | Broadening _install_hermes_artifacts() from catching only FileNotFoundError to catching every OSError makes permission, symlink-refusal, and other artifact-write failures return False, but the normal _setup_hermes() caller at src/ouroboros/cli/commands/setup.py:3022 still discards that result and returns True. A direct probe with _install_hermes_artifacts() returning False produced Configured Hermes runtime and setup_result=True, so ouroboros setup --runtime hermes can now report successful activation while its required managed skills were not installed. Either preserve propagation for the normal setup path or make that caller handle the result explicitly, with coverage for a real OSError failure. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:5071 |
P2 | High | When all attempted artifact refreshes fail, the command prints No installed runtime artifacts found to refresh. before reporting the incomplete runtimes. Avoid that contradictory message and retain the actual installer exception/details, especially for Hermes failures currently rendered as Could not locate packaged skills for all OSError causes. |
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/setup.py:5071 | Error messaging | Failure-only refreshes should not emit the success-path “No installed runtime artifacts found” message; report only the incomplete runtime list and actionable failure details. |
Test Coverage Notes
- Reviewed
tests/unit/cli/test_update.pyandtests/unit/cli/commands/test_setup_refresh.py. - Executed the focused update and refresh suites:
197 passed. - Executed skill artifact tests:
50 passed, 5 deselected. ruff check,ruff format --check, andmypypassed for the changed implementation and tests.- No existing test covers normal Hermes setup when
_install_hermes_artifacts()returnsFalse; a direct focused probe reproduced the silent-success behavior.
Design Notes
The new all path has a sound separation between package installation identity, host-plugin refresh, and installed-artifact refresh. Failure aggregation is correctly implemented at the refresh boundary, but the changed helper contract is not safe for all existing callers.
Design / Roadmap Gate
--runtime all preserves the configured backend and uses the proven console script for setup refresh; the new tests cover the primary update and artifact-refresh failure paths. However, _install_hermes_artifacts() is shared by both refresh and normal setup. Its new broad OSError catch changes the normal activation contract, and the caller ignores the new boolean result after persistent runtime configuration has already been committed. This leaves a failed Hermes artifact installation represented as successful setup state and must be corrected before merge.
Directional Notes
Review focus followed the prior rounds’ standard for truthful partial-update status, exact executable ownership, marketplace scope, backend preservation, and documentation parity. Current source evidence confirms the prior refresh-propagation concern is fixed. Maintainer memory also directed inspection toward persistent setup state and cross-runtime failure boundaries, but the blocker above is independently demonstrated by the current source and runtime probe.
Test Coverage
- Reviewed
tests/unit/cli/test_update.pyandtests/unit/cli/commands/test_setup_refresh.py. - Executed the focused update and refresh suites:
197 passed. - Executed skill artifact tests:
50 passed, 5 deselected. ruff check,ruff format --check, andmypypassed for the changed implementation and tests.- No existing test covers normal Hermes setup when
_install_hermes_artifacts()returnsFalse; a direct focused probe reproduced the silent-success behavior.
Merge Recommendation
Request changes. Fix the Hermes normal-setup caller/helper contract so artifact installation failures cannot be reported as successful activation, then add a focused regression test. The new --runtime all behavior and its targeted test coverage otherwise appear directionally correct.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 6a00d4c
request_id: req_1786811805_1868
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | f5a01fd203d904d78f2ad928f482d677a9bcde70 |
| Request ID | req_1786813810_1878-retry-1-retry-2 |
| Review record | 200d740b-a61d-45a7-ae8e-e8126d6271bc |
What Improved
- Adds
--runtime allto refresh Claude Code and Codex integrations together while preserving the configured execution backend. - Uses the receipt-owned Ouroboros console for artifact refresh and limits Codex mutation to the Ouroboros marketplace.
- Makes
setup refreshaccumulate per-runtime failures, continue refreshing independent integrations, and return a truthful nonzero status. - Keeps both update skill copies synchronized and documents the different Claude and Codex restart contracts.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin and previously installed runtime artifacts | Met |
| Preserve the configured execution backend rather than selecting Claude or Codex | Met |
| Report incomplete artifact refreshes as a nonzero partial update while continuing other runtimes | Met |
| Document Claude plugin reload versus active Codex session restart behavior | Met |
| Keep failed setup activation from leaving an incomplete runtime selected | Not met |
Prior Findings Status
The earlier setup refresh failure-propagation concerns are withdrawn: current HEAD accumulates failures, continues independent refreshes, exits nonzero, and reaches the updater’s partial-update result. The prior Hermes concern is modified rather than repeated unchanged: successful completion is no longer reported, but the newly required artifact failure still occurs after durable activation state is committed and is not rolled back.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:3003 | BLOCKING | Hermes artifact installation is now a required activation gate, but _setup_hermes() calls _commit_runtime_activation() before that gate. If _install_hermes_artifacts() fails at line 3019, setup returns failure while ~/.ouroboros/config.yaml already selects runtime_backend: hermes and ~/.hermes/config.yaml already contains the MCP registration. A focused runtime probe confirmed both writes remain after the false return. This leaves a failed setup represented in durable runtime state as an active but incomplete Hermes integration, so later commands can route to Hermes despite the nonzero setup result. Install the required skills before committing activation, or extend the transaction to restore both persistent files when artifact installation fails; add a regression asserting the previous backend and Hermes host configuration remain unchanged. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/update.py:860 | Medium | High | Add an updater-level regression where one host plugin refresh fails and verify the other host plus setup refresh are still attempted before the command reports a partial update. The implementation currently continues correctly, but this failure-isolation contract is central to --runtime all. |
Non-blocking Suggestions
None.
Test Coverage Notes
tests/unit/cli/test_update.py: 185 passed.tests/unit/cli/commands/test_setup_refresh.pyplus the changed Hermes failure test: 14 passed.tests/unit/skills/test_skill_artifacts.pywith the documented poison-control exclusion: 50 passed, 5 deselected.- Ruff passed for all changed Python files.
- A broader Hermes/refresh-filtered setup run produced 28 passes and two unrelated Codex MCP tests that fail because this review environment lacks the MCP extra; the changed targeted tests pass.
- Direct failure-state probing confirmed that a Hermes artifact failure returns
Falsebut leaves both the Hermes backend selection and MCP registration persisted.
Design Notes
The combined updater is appropriately scoped around the proven installation identity and delegates artifact-only work to a configuration-preserving setup command. The remaining issue is transaction ordering in the shared Hermes setup path, not the --runtime all orchestration itself.
Design / Roadmap Gate
The update path preserves package ownership, configured backend state, custom host executable selection, and independent failure reporting. setup refresh does not rewrite configuration or MCP registrations. However, the changed Hermes normal-setup boundary defines skill installation as required only after committing runtime selection and host registration, so a failed activation is not replay-safe and leaves partial global state. That durable-state contract must be corrected before merge.
Directional Notes
Review focus followed the established requirements for exact executable ownership, backend preservation, scoped marketplace mutation, truthful cross-runtime failure propagation, documentation parity, and fail-closed persistent setup writes. Maintainer memory was used only to direct inspection; the blocker is independently demonstrated by current source and a focused runtime probe.
Test Coverage
tests/unit/cli/test_update.py: 185 passed.tests/unit/cli/commands/test_setup_refresh.pyplus the changed Hermes failure test: 14 passed.tests/unit/skills/test_skill_artifacts.pywith the documented poison-control exclusion: 50 passed, 5 deselected.- Ruff passed for all changed Python files.
- A broader Hermes/refresh-filtered setup run produced 28 passes and two unrelated Codex MCP tests that fail because this review environment lacks the MCP extra; the changed targeted tests pass.
- Direct failure-state probing confirmed that a Hermes artifact failure returns
Falsebut leaves both the Hermes backend selection and MCP registration persisted.
Merge Recommendation
Request changes until Hermes required-artifact failure either occurs before persistent activation or rolls back the committed runtime and host configuration, with focused failure-path coverage. The new --runtime all flow and artifact-refresh failure propagation otherwise appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: f5a01fd
request_id: req_1786813810_1878-retry-1-retry-2
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact head f5a01fd and emit a fresh exact-head final_review.v1 verdict. |
|
@ouroboros-agent please re-review exact head 71d4fbf and emit a fresh exact-head final_review.v1 verdict. Hermes required artifacts now install before any backend or host registration commit; the failure regression proves both prior files remain byte-for-byte unchanged. Focused Hermes/update tests pass (26 tests), with Ruff and format green. |
|
@ouroboros-agent please re-review exact head d0d045b and emit a fresh exact-head final_review.v1 verdict. The latest main is merged; Hermes required-artifact ordering and failure-path rollback regressions remain the review focus. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | d0d045b0abf485107cb7bb51bc1f3a2dfc4f098f |
| Request ID | req_1786818873_1898 |
| Review record | fd5f061f-8470-42d5-903f-4b2ac7f275bb |
What Improved
- Adds
--runtime allwith scoped Claude and Codex plugin refreshes while preserving the configured execution backend. - Propagates per-runtime
setup refreshfailures into the updater’s partial-update result and nonzero exit status. - Keeps the two update skill documents synchronized and documents the Claude reload versus Codex restart contract.
- Defers Hermes runtime/config registration until required skills install successfully, addressing the prior activation-order concern.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin integration | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Met |
| Preserve the configured execution backend | Met |
| Document Claude reload and Codex restart behavior | Met |
| Refresh installed artifacts without corrupting the previous working generation when refresh fails | Not met |
Prior Findings Status
The prior backend-preservation, exact executable selection, scoped marketplace mutation, and truthful partial-failure propagation concerns are resolved in the current snapshot. The previous Hermes activation concern is improved because config and host registration are now deferred until skill installation succeeds. It is modified rather than fully withdrawn: fresh current-source evidence shows that a mid-install artifact failure can still destroy the previous Hermes skill generation even though setup reports failure.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:2882 | BLOCKING | Hermes refresh is still destructive on a caught installation failure. _install_hermes_artifacts() now converts every OSError from install_hermes_skills(..., prune=True) into False, but the installer removes each live destination before calling shutil.copytree (src/ouroboros/hermes/artifacts.py:142). A disk-full or permission error during that copy therefore deletes the previously working skill generation; setup refresh continues and eventually exits nonzero, while update --runtime all leaves Hermes partially corrupted. A focused runtime probe against this snapshot produced result=False with both the old skill file and its target directory gone. Stage the replacement and atomically swap it, or snapshot and restore the prior generation on failure, and add a mid-copy failure regression proving existing artifacts remain byte-identical. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py -qpassed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes'passed: 5 passed, 400 deselected.- Focused Ruff checks passed for all changed Python files.
- An additional failure-injection probe forced
shutil.copytreeto raise after removal of an existing Hermes skill and confirmed that the command returned failure after deleting the prior generation.
Design Notes
The updater orchestration and backend-preservation design are sound, and failure aggregation is substantially improved. The remaining issue is below that orchestration layer: Hermes artifact replacement does not provide the transactional semantics now assumed by the new boolean failure contract.
Design / Roadmap Gate
--runtime all preserves runtime configuration, uses the receipt-proven Ouroboros console, refreshes both host integrations independently, and converts child refresh failures into a partial-update exit. However, the installed-artifact boundary is not replay-safe: Hermes replacement can destroy the prior generation before the new one is committed, and neither _install_hermes_artifacts nor refresh_artifacts restores it after reporting failure. This violates the persistence and failure-recovery contract for an infrastructure updater.
Directional Notes
Review focused on persistent setup writes, transactional artifact replacement, backend preservation, exact host executable ownership, scoped marketplace updates, and documentation parity. Maintainer memory was used only to select these inspection paths; the blocker is independently demonstrated by current source and a focused runtime probe.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py -qpassed.SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes'passed: 5 passed, 400 deselected.- Focused Ruff checks passed for all changed Python files.
- An additional failure-injection probe forced
shutil.copytreeto raise after removal of an existing Hermes skill and confirmed that the command returned failure after deleting the prior generation.
Merge Recommendation
Request changes until Hermes skill replacement preserves or restores the previous generation on every reported failure, with a focused mid-copy failure regression. The remaining --runtime all implementation and documentation appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: d0d045b
request_id: req_1786818873_1898
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Addressed the exact-head Hermes durability blocker at 0ce5380. Refresh now builds a complete sibling staging generation, preserves unmanaged files, atomically swaps only after all copies succeed, and restores the prior generation if the final swap fails. Added a true mid-copy failure regression proving the live skill remains byte-identical. Hermes/setup-refresh/update coverage: 208 passed; Ruff passes. Please review the current HEAD. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 0ce5380e90e7101fff58667462a643c058c4049c |
| Request ID | req_1786819764_1905 |
| Review record | 558a0097-5071-4cb0-a57e-1e1f0de0629b |
What Improved
- Adds
--runtime allwith scoped Claude and Codex plugin refreshes while preserving the configured execution backend. - Propagates per-runtime artifact failures through
setup refreshto the updater’s partial-update exit status. - Moves required Hermes artifacts before runtime activation and stages refresh copies so ordinary mid-copy
OSErrorfailures preserve the live generation. - Documents the distinct Claude reload and Codex restart contracts.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
Refresh only the Ouroboros Codex marketplace, refresh the Claude plugin, and run ouroboros setup refresh |
Met |
| Preserve the configured execution backend rather than selecting either host | Met |
| Document Claude plugin reload and active Codex session restart behavior | Met |
Prior Findings Status
The earlier cross-command failure propagation and pre-activation Hermes ordering concerns are resolved in the current snapshot. The latest Hermes replacement concern is partially addressed: staging preserves the live generation during copy failures and restores it when the second rename raises OSError. The durability blocker remains in modified form because interruption between renames leaves the live generation absent and a subsequent retry deletes its backup.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:165 | BLOCKING | The two-rename Hermes commit is not replay-safe. After target_dir is renamed to .ouroboros.old at line 168, process termination before the staging rename leaves no live ouroboros directory. On retry, _prepare_hermes_install_root(target_dir) creates an empty target, then line 166 unconditionally deletes .ouroboros.old, destroying the only working generation before committing the replacement. A focused interruption probe reproduced exactly that state: the live target was absent and the old generation existed only in the backup, which the next invocation removed. Recover an interrupted backup before creating/replacing the target, or use a durable transaction scheme that makes retries restore or preserve it. Add an interruption/retry regression covering the window between the two renames. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran the changed updater, setup-refresh, and Hermes artifact suites: 208 passed.
- Ran Hermes setup-focused coverage: 5 passed, 400 deselected.
- Ran Ruff against all changed Python source and tests: passed.
- Confirmed the installed Codex CLI supports the scoped
plugin marketplace upgrade ouroboroscommand. - Existing coverage proves ordinary mid-copy
OSErrorpreservation, symlink refusal, failure propagation, backend preservation, and updater partial failure, but does not cover process interruption and replay between the directory renames.
Design Notes
The host-refresh orchestration is appropriately scoped and preserves package and backend identity. Hermes replacement now has a sensible sibling-staging design, but its commit protocol still lacks durable recovery for interruption between the backup and activation renames.
Design / Roadmap Gate
The updater preserves the receipt-proven package console, does not rewrite runtime configuration for --runtime all, refreshes hosts independently, and surfaces child refresh failures. The remaining affected-boundary failure is replay safety: Hermes refresh has a multi-step persistent commit whose backup is not recognized as recoverable state on the next invocation. An interrupted refresh can therefore remove the active skill generation and then discard its backup during retry.
Directional Notes
Review focused on truthful partial-update status, exact host executable selection, backend preservation, scoped marketplace mutation, and transactional persistent artifact replacement. Maintainer memory was used only to select these inspection paths; the blocker is independently established by the current source and focused runtime injection.
Test Coverage
- Ran the changed updater, setup-refresh, and Hermes artifact suites: 208 passed.
- Ran Hermes setup-focused coverage: 5 passed, 400 deselected.
- Ran Ruff against all changed Python source and tests: passed.
- Confirmed the installed Codex CLI supports the scoped
plugin marketplace upgrade ouroboroscommand. - Existing coverage proves ordinary mid-copy
OSErrorpreservation, symlink refusal, failure propagation, backend preservation, and updater partial failure, but does not cover process interruption and replay between the directory renames.
Merge Recommendation
Request changes until Hermes artifact replacement recovers or preserves .ouroboros.old after interruption between the two renames, with a focused interruption/retry regression. The rest of the --runtime all implementation and its failure propagation are ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 0ce5380
request_id: req_1786819764_1905
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | cf4e8803c2ebfa88445a4b2aebc7c5a199a28a92 |
| Request ID | req_1786820758_1908 |
| Review record | 20449043-5620-46f0-a76e-90ec3e9b14e3 |
What Improved
- Adds
--runtime allto refresh Claude Code, Codex, and previously installed runtime artifacts without changing the configured execution backend. - Uses installation-receipt ownership and configured host executable paths rather than unrelated binaries from
PATH. - Propagates individual artifact-refresh failures into a nonzero partial-update result while continuing other runtime refreshes.
- Stages Hermes skill copies before replacing the live generation and preserves the live directory on ordinary copy failures.
- Documents the Claude reload and Codex restart requirements.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin independently | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Partially met — invoked and failures propagate, but Hermes retry can discard the interrupted prior generation |
| Preserve the configured execution backend | Met |
| Document Claude plugin reload and active Codex session restart behavior | Met |
| Linked issue #2110 details beyond the PR body | N/A — issue contents were not included in the review artifacts |
Prior Findings Status
The prior replay-safety concern is maintained with fresh current-snapshot evidence. Ordinary OSError rollback is implemented, but interruption between the renames still leaves a backup state that the next invocation does not recover.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:167 | BLOCKING | The Hermes replacement is still not replay-safe after interruption between the two os.replace calls. If the process stops after moving target_dir to .ouroboros.old at line 169 but before installing the staging directory, the next invocation calls _prepare_hermes_install_root(target_dir), which creates a new empty target. It then copies that empty target into staging and unconditionally deletes the recoverable .ouroboros.old generation at line 167. A focused KeyboardInterrupt injection followed by retry reproduced loss of an unmanaged operator-notes.txt file that the normal refresh contract preserves. Recover .ouroboros.old before creating/staging a new target, or otherwise treat it as durable interrupted-transaction state, and add an interruption-then-retry regression. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the changed updater, setup-refresh, Hermes activation, and artifact-installation tests.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.pypassed: 208 tests.- Targeted Hermes setup selection passed: 7 tests, 398 deselected.
- A direct interruption/retry probe demonstrated that
.ouroboros.oldsurvives the interruption but is deleted on retry, losing the preserved unmanaged file. - Existing mid-copy coverage only fails before the live-to-backup rename and therefore does not cover the interrupted durable state.
Design Notes
The host-refresh orchestration is appropriately separated from backend selection and uses the proven package console. The remaining flaw is in the durable Hermes swap protocol: .ouroboros.old is treated as disposable rather than recoverable transaction state.
Design / Roadmap Gate
The update boundary preserves runtime configuration and independently attempts Claude, Codex, and installed-artifact refreshes. Normal helper failures become observable partial updates. However, artifact replacement spans durable renames and must survive process interruption and retry without discarding the previous generation or unmanaged files. The current backup lifecycle violates that recovery contract.
Directional Notes
Review focused on installation ownership, exact host executable selection, backend preservation, scoped marketplace mutation, truthful partial-failure reporting, and replay-safe persistent artifact replacement. Maintainer memory was advisory only; the blocker is independently established by current source and failure injection.
Test Coverage
- Reviewed the changed updater, setup-refresh, Hermes activation, and artifact-installation tests.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.pypassed: 208 tests.- Targeted Hermes setup selection passed: 7 tests, 398 deselected.
- A direct interruption/retry probe demonstrated that
.ouroboros.oldsurvives the interruption but is deleted on retry, losing the preserved unmanaged file. - Existing mid-copy coverage only fails before the live-to-backup rename and therefore does not cover the interrupted durable state.
Merge Recommendation
Request changes until Hermes refresh recognizes and safely recovers an interrupted .ouroboros.old generation, with a focused interruption-then-retry regression. The remaining --runtime all behavior and targeted test coverage are ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: cf4e880
request_id: req_1786820758_1908
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Added interruption recovery at the latest HEAD: an existing |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 4a95cd69238a8fb4026c155be93255d937979fef |
| Request ID | req_1786821503_1912 |
| Review record | 9ef67b35-dff5-4709-9069-f23378ecadc0 |
What Improved
- Adds
--runtime allto refresh Claude, Codex, and installed runtime artifacts while preserving the configured execution backend. - Propagates per-runtime artifact failures through
setup refreshto the updater’s partial-update exit status. - Stages Hermes skill replacements and restores the previous live generation when the final rename fails or a retry follows an interrupted swap.
- Documents the distinct Claude reload and Codex restart requirements.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
Upgrade only the Ouroboros Codex marketplace, refresh the Claude plugin, and run setup refresh for installed artifacts |
Met |
| Preserve the configured execution backend instead of selecting a host as the backend | Met |
| Document Claude plugin reload and active Codex session restart behavior | Met |
| Report incomplete artifact refreshes as partial update failures while continuing other runtimes | Met |
| Preserve the prior Hermes generation, unmanaged files, and recover safely after interrupted replacement | Partially met — interrupted managed swaps recover, but the fixed backup pathname can delete or adopt unrelated operator-owned content |
| Linked issue #2110 details beyond the PR body | N/A — issue content was not included in the review artifacts |
Prior Findings Status
The prior interruption/retry concern is addressed: when execution stops after moving the live generation to the backup pathname, the next invocation restores it before staging. The previous concern is therefore modified rather than repeated. Request changes remains necessary because current HEAD introduces an independently verified ownership/identity failure at that backup boundary.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:167 | BLOCKING | The fixed .ouroboros.old pathname is deleted and recovered solely by name, without proving that Ouroboros created or owns its contents. On an ordinary refresh with a live ouroboros target, _remove_target_path(backup_dir) recursively deletes any pre-existing operator-owned sibling at that pathname. A direct probe placed operator-note.txt there and confirmed that the refresh deleted it. Conversely, line 126 promotes any object at this pathname when the target is absent, so unrelated content can be mistaken for a recoverable generation. Use an ownership/identity marker or a durable transaction record and only recover/delete a backup whose exact generation is verified; add regressions proving foreign sibling content is preserved in both target-present and target-absent states. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- The 12 directly changed updater, setup-refresh, Hermes setup, swap-failure, and interruption-retry regressions passed.
- A broader filtered run produced 228 passes and 2 failures in unchanged Codex MCP launcher tests because this environment could not discover a launchable MCP command.
- Ruff check, Ruff format verification, and MyPy for
src/ouroboros/cli/commands/update.pypassed. - A direct hostile-state probe demonstrated deletion of operator-owned content at the fixed
.ouroboros.oldsibling.
Design Notes
The cross-runtime updater orchestration and failure propagation are appropriately separated from backend configuration. The Hermes generation swap is directionally sound, but its recovery identity is pathname-based rather than ownership-based, making the persistence transaction unsafe around operator-controlled filesystem state.
Design / Roadmap Gate
The updater preserves installation identity, preflights host executable configuration, independently attempts Claude, Codex, and artifact refreshes, and does not rewrite runtime selection under --runtime all. Hermes replacement now handles copy failure, final-swap failure, and interruption/retry. However, recovery and cleanup do not carry durable ownership or exact-generation identity: a conventional sibling pathname is treated as an Ouroboros backup regardless of provenance. That creates a direct operator-data-loss boundary and fails the persistence safety gate.
Directional Notes
Review focused on exact executable ownership, backend preservation, truthful partial-update status, and replay-safe persistent artifact publication. Maintainer memory was used only to select the backup-ownership inspection path; the blocker is independently established by current source and a direct filesystem probe.
Test Coverage
- The 12 directly changed updater, setup-refresh, Hermes setup, swap-failure, and interruption-retry regressions passed.
- A broader filtered run produced 228 passes and 2 failures in unchanged Codex MCP launcher tests because this environment could not discover a launchable MCP command.
- Ruff check, Ruff format verification, and MyPy for
src/ouroboros/cli/commands/update.pypassed. - A direct hostile-state probe demonstrated deletion of operator-owned content at the fixed
.ouroboros.oldsibling.
Merge Recommendation
Request changes until Hermes backup recovery and cleanup verify artifact ownership and exact identity, with tests preserving foreign .ouroboros.old content. The remaining --runtime all behavior, failure propagation, documentation, and interrupted-swap recovery appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 4a95cd6
request_id: req_1786821503_1912
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Replaced the fixed backup identity with UUID-scoped backups carrying an Ouroboros ownership marker. Recovery scans only marker-verified managed generations; foreign |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 7dd5e73c280fe6baeefce39d657d61b76dd55b4c |
| Request ID | req_1786822439_1915 |
| Review record | 4773827a-bc6e-4681-a669-23ad8e65e8fb |
What Improved
- Adds
--runtime allto refresh Claude and Codex integrations without reselecting the configured execution backend. - Scopes Codex updates to the Ouroboros marketplace and uses the receipt-proven Ouroboros console for artifact refresh.
- Propagates per-runtime artifact failures through
setup refreshto the updater’s partial-update exit status. - Stages Hermes skill generations before publication, preserves unmanaged files, and adds failure, interruption, symlink, and foreign-backup coverage.
- Documents the distinct Claude reload and Codex restart contracts.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Partially met — invocation and failure propagation work, but Hermes recovery can restore a stale generation |
| Preserve the configured execution backend rather than selecting a host | Met |
| Document that Claude can reload plugins while active Codex sessions must restart | Met |
| Linked issue #2110 requirements beyond the PR-body summary | N/A — linked issue contents were not included in the review artifacts |
Prior Findings Status
The prior persistence concern is modified but maintained. UUID-scoped marker-verified backups address the previously reported fixed-path foreign-backup collision, so that specific case is withdrawn. Current source independently shows that backup identity still lacks deterministic generation ordering when more than one managed backup survives, leaving the replay-safety blocker unresolved in a narrower form.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:136 | BLOCKING | Recovery does not identify the most recent backup generation. A successfully published refresh can leave its old marker-valid backup behind when cleanup at line 188 fails. If a later refresh is interrupted after moving the newer live generation into another backup, sorted(managed_backups)[-1] selects between random UUID names rather than publication order or an exact generation identity. A focused filesystem probe reproduced recovery of the stale backup and loss of newer operator.txt contents while the newer backup remained stranded. Persist an ordered/exact generation identity, maintain at most one recoverable backup transaction, or fail safely when multiple valid backups are present; add a cleanup-failure followed by interrupted-swap regression proving the newest operator-owned state is restored. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 211 passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes': 5 passed, 400 deselected. - Ran focused Ruff checks across all changed Python files: passed.
- A direct cleanup-failure plus later interruption probe reproduced stale Hermes generation recovery and loss of newer operator state; the committed tests do not cover multiple marker-valid backups.
Design Notes
The updater orchestration is appropriately scoped and preserves installation and backend identity. The remaining defect is inside the Hermes transactional publication protocol: random backup names are treated as recoverable generations without an authoritative ordering or single-transaction invariant.
Design / Roadmap Gate
The host-refresh contract, marketplace scope, subprocess failure propagation, configuration non-mutation, and restart messaging satisfy the affected updater boundary. Hermes publication handles ordinary copy failure, swap failure, and a single interrupted backup, but it is not replay-safe once cleanup failure leaves an older managed generation alongside a newer interrupted generation. Recovery can then publish stale operator-owned state, so the persistence and data-preservation gate remains unmet.
Directional Notes
Review focus followed the established requirements for exact executable ownership, backend preservation, truthful partial-update reporting, and replay-safe persistent artifact replacement. Maintainer memory was used only to prioritize transaction ownership and recovery inspection; the blocker is independently established by current source and a focused runtime probe.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 211 passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes': 5 passed, 400 deselected. - Ran focused Ruff checks across all changed Python files: passed.
- A direct cleanup-failure plus later interruption probe reproduced stale Hermes generation recovery and loss of newer operator state; the committed tests do not cover multiple marker-valid backups.
Merge Recommendation
Request changes until Hermes recovery deterministically restores the newest exact generation—or safely refuses ambiguous recovery—and a regression covers cleanup failure followed by interruption and retry. The remaining --runtime all implementation and focused coverage are ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 7dd5e73
request_id: req_1786822439_1915
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Addressed the exact-head recovery blocker at 7604cf9. A live target now causes stale marker-valid backups from a prior post-publish cleanup failure to be removed before any new backup is created; if the target is absent and multiple managed backups exist, recovery fails safely instead of sorting random UUIDs. Added a cleanup-failure → interrupted-new-swap → retry regression proving the newest operator state is restored and no backup remains. 616 tests passed (1 skipped), MyPy passed, and Ruff/format passed. @ouroboros-agent please re-review exact head 7604cf9. |
|
Re-review requested for exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 48e829b15517048e5662b98c5b1666dffec80f3d |
| Request ID | req_1786867968_2060 |
| Review record | 2b1b915f-9ef4-4e52-99bd-dbabbfd5590a |
What Improved
- Adds
--runtime allto refresh Claude Code, Codex, and previously installed runtime artifacts without changing the configured execution backend. - Propagates per-runtime artifact refresh failures into a nonzero partial-update result while continuing independent refresh operations.
- Makes Hermes activation and rollback substantially more failure-atomic, with recovery records, ownership validation, symlink refusal, and focused interruption coverage.
- Documents the host-specific restart contract and scopes the Codex operation to the Ouroboros marketplace.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after upgrade |
Met |
Upgrade only the Ouroboros Codex marketplace, refresh the Claude plugin, and run ouroboros setup refresh for installed artifacts |
Met |
| Preserve the configured execution backend rather than selecting either refreshed host | Met |
| Continue independent artifact refreshes and truthfully report incomplete runtimes | Met |
| Preserve operator-owned Hermes artifacts across activation failure, interruption, recovery, and concurrent mutation | Partially met |
| Document that Claude can reload plugins while active Codex sessions must restart | Met |
| Linked issue #2110 details beyond the requirements stated in the PR body | N/A |
Prior Findings Status
The earlier blockers concerning truthful refresh status, backend preservation, activation ordering, interruption records, and pre-retirement ownership validation are withdrawn based on the current snapshot and focused tests. The operator-data-preservation concern remains, narrowed to a freshly reproduced per-entry fingerprint-to-unlink race in the current recursive cleanup implementation.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:581 | BLOCKING | _remove_exact_path() checks a file’s pathname fingerprint and then calls unlink() in a separate operation. A concurrent process can replace or modify that path after the check but before the unlink, causing the new operator-owned data to be deleted. _finish_owned_cleanup() uses this helper to recursively destroy a renamed prior Hermes generation, which includes preserved unmanaged files. An independent probe replaced the validated file immediately before Path.unlink; the current implementation deleted the concurrent replacement (concurrent_replacement_survived=False). The root-level insertion test only mutates before recursive validation and does not cover this per-entry check-to-unlink window. Cleanup must avoid destructive pathname deletion unless ownership is still guaranteed at the mutation itself, or preserve the retirement generation when portable atomic ownership-and-unlink semantics are unavailable. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Independently ran
tests/unit/hermes/test_artifacts.py: 50 passed. - Independently ran
tests/unit/cli/commands/test_setup_refresh.pyandtests/unit/cli/test_update.py: 200 passed. - Independently ran
tests/unit/cli/test_setup.py -k hermes: 21 passed, 391 deselected. - Ran a focused destructive-boundary probe demonstrating that a concurrent file replacement between
_tree_fingerprint()andPath.unlink()is deleted. - The repository’s full-suite result from the PR body was not independently rerun.
Design Notes
The host-update orchestration is appropriately scoped and preserves installation and backend identity. The remaining issue is isolated to the shared Hermes durable-publication layer: ownership is validated thoroughly before cleanup, but the final recursive deletion is not atomic with that validation.
Design / Roadmap Gate
Package ownership, exact host executable reuse, marketplace scope, backend non-reselection, sibling refresh continuation, failure propagation, restart messaging, activation ordering, and interruption recovery pass the affected-boundary gate. Persistence safety does not: a validated file can be replaced or changed before the subsequent pathname unlink, allowing cleanup to discard operator data that was never part of the validated generation.
Directional Notes
Review focused on transactional setup effects, replay-safe durable state, truthful cross-runtime failure reporting, and ownership validation at the final destructive filesystem boundary. Maintainer memory was used only to direct inspection; the blocker is independently established by current source and the focused runtime probe.
Test Coverage
- Independently ran
tests/unit/hermes/test_artifacts.py: 50 passed. - Independently ran
tests/unit/cli/commands/test_setup_refresh.pyandtests/unit/cli/test_update.py: 200 passed. - Independently ran
tests/unit/cli/test_setup.py -k hermes: 21 passed, 391 deselected. - Ran a focused destructive-boundary probe demonstrating that a concurrent file replacement between
_tree_fingerprint()andPath.unlink()is deleted. - The repository’s full-suite result from the PR body was not independently rerun.
Merge Recommendation
Request changes until Hermes retirement cleanup closes or avoids the per-entry check-to-unlink race and adds an exact-boundary regression proving a concurrent replacement survives. The --runtime all orchestration itself is otherwise merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 48e829b
request_id: req_1786867968_2060
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | bea90d72f77b821440e0fdf5d16a885b78f685ff |
| Request ID | req_1786868643_2063 |
| Review record | 2abfd0c1-07eb-41ad-91ea-833ff1226a5a |
What Improved
- Adds
--runtime allto refresh Claude and Codex integrations without reselecting the configured execution backend. - Propagates per-runtime artifact failures through
setup refreshto the updater’s partial-update exit status. - Improves Hermes activation ordering, rollback, symlink refusal, atomic publication, and interruption recovery.
- Documents the distinct Claude reload and Codex restart requirements.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Partially met — refresh executes and propagates failures, but each successful Hermes refresh after the first retains another complete prior generation indefinitely |
| Preserve the configured execution backend rather than selecting a host as the new backend | Met |
| Document that Claude may reload plugins while active Codex sessions must restart | Met |
| Continue attempting remaining integrations and return nonzero when any refresh is incomplete | Met |
Prior Findings Status
The latest prior concern about deleting operator content through a check-to-unlink race is addressed: retirement now uses an atomic directory rename and performs no recursive deletion. That concern is therefore withdrawn in its prior form. The broader previously applied requirement for bounded, ownership-safe cleanup remains unmet because the replacement strategy now retains every complete prior generation indefinitely.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:220 | BLOCKING | Every successful Hermes replacement after the first permanently renames the complete previous generation to a UUID-scoped .retired.* directory, then removes the cleanup marker and intent without ever deleting or reclaiming that directory. Consequently, ordinary repeated setup, setup refresh, or update --runtime all calls accumulate an unbounded number of full skill-tree copies. Because unmanaged operator files are copied into each staged generation, files later edited or removed from the live tree also remain durably retained in these hidden snapshots. This is explicitly demonstrated by test_repeated_refreshes_preserve_prior_generations, which expects two retirement directories after two swaps, and by a current-snapshot runtime probe where each additional refresh added another retirement. Replace this permanent-retirement strategy with bounded, ownership-safe reclamation or another publication design that avoids both the destructive pathname race and indefinite retention of prior operator data. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py -q; all selected tests passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes'; 12 passed and 400 were deselected. - Ran a focused repeated-install probe against the current implementation; the second and third successful Hermes installs left one and two permanent
.retired.*generations respectively. - Existing tests thoroughly cover failure propagation, rollback, symlink handling, interruption windows, and ownership races, but currently encode indefinite prior-generation retention as expected behavior rather than bounding it.
Design Notes
The updater orchestration is appropriately thin and preserves installation identity, host executable selection, marketplace scope, and backend configuration. The Hermes state machine is substantially safer under interruption and concurrent mutation, but its terminal state is not operationally bounded because successful transactions permanently retain full historical generations.
Design / Roadmap Gate
Host executable reuse, package ownership, scoped marketplace mutation, backend non-reselection, sibling refresh continuation, failure propagation, restart messaging, activation ordering, rollback, and interruption recovery pass the affected-boundary gate. Durable artifact lifecycle does not: normal successful operation has no terminal reclamation path, so persistent state grows without bound and retains superseded operator-owned content.
Directional Notes
Review focus followed the established requirements for truthful cross-runtime status, transactional setup effects, replay-safe durable state, and ownership validation at destructive filesystem boundaries. Maintainer memory was used only to prioritize persistence and recovery inspection; the blocker is independently proven by current source, current tests, and a focused runtime probe.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py -q; all selected tests passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes'; 12 passed and 400 were deselected. - Ran a focused repeated-install probe against the current implementation; the second and third successful Hermes installs left one and two permanent
.retired.*generations respectively. - Existing tests thoroughly cover failure propagation, rollback, symlink handling, interruption windows, and ownership races, but currently encode indefinite prior-generation retention as expected behavior rather than bounding it.
Merge Recommendation
Request changes until Hermes publication has a bounded, ownership-safe lifecycle for superseded generations without reintroducing the concurrent replacement deletion race. The --runtime all orchestration and failure propagation otherwise appear merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: bea90d7
request_id: req_1786868643_2063
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | ec3a0817f52c62a9ba70c5c19f9be758f0ecc28a |
| Request ID | req_1786870314_2070-retry-1-retry-2 |
| Review record | 7edbf49c-e328-4756-9bd0-292155a69ea5 |
What Improved
- Adds
--runtime allto refresh Claude and Codex integrations together while preserving the configured execution backend. - Scopes Claude and Codex marketplace operations to the Ouroboros integration and refreshes installed artifacts through the proven console script.
- Improves setup-refresh failure propagation, continuation across independent runtimes, Hermes staging, rollback, ownership markers, interruption recovery, and bounded retired-generation retention.
- Adds substantial focused coverage for cross-runtime refresh, Hermes publication, rollback, symlink refusal, malformed records, and interruption paths.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh both Claude Code and Codex integrations |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh the Claude plugin | Met |
Run ouroboros setup refresh for installed runtime artifacts |
Met |
| Preserve the configured execution backend rather than selecting a host as the backend | Met |
| Continue independent runtime refreshes and report partial failures with a nonzero result | Met |
| Document the Claude reload/restart and active Codex-session restart contract | Met |
| Preserve operator-owned Hermes artifacts and make publication/recovery crash-safe | Partially met — publication and recovery are substantially hardened, but retired-generation reclamation can delete concurrent operator content and can leave replay-blocking partial state |
Prior Findings Status
The earlier unbounded-retention concern was addressed directionally by adding retired-generation reclamation. Prior publication, rollback, ownership-marker, symlink, interruption, and failure-propagation concerns appear maintained or addressed in the current source. However, the new reclamation implementation introduces a modified ownership/destructive-boundary blocker: cleanup now has a bounded lifecycle, but its final recursive deletion is not safe against concurrent operator changes or interruption.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:297 | BLOCKING | _reclaim_retired_generations() validates a retired generation and atomically renames it to .reclaim, but then calls recursive _remove_target_path(reclamation) without another ownership/fingerprint check at the destructive boundary. A concurrent operator file added after the validation is therefore removed by the recursive cleanup; a focused probe injected operator-concurrent.txt immediately before this call and confirmed it was deleted while the new generation published successfully. An interruption during that recursive deletion can also leave a partially deleted .reclaim tree; the next refresh revalidates its digest and fails with Refusing foreign Hermes swap backup, blocking future refreshes until manual repair. Keep reclamation bounded, but make cleanup itself replay-safe and ownership-safe at the final deletion boundary, with regressions for both concurrent additions/replacements and mid-cleanup interruption. |
Follow-up Findings
None.
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the changed Hermes, setup-refresh, setup, and update tests.
- Ran the focused selection covering Hermes, setup refresh,
--runtime all, and related setup paths:102 passed, 561 deselected. - Ran Ruff on all changed source and test files: all checks passed.
- Existing tests cover mutation after the private reclamation rename, but not mutation immediately before recursive reclamation deletion or partial cleanup replay; the focused runtime probe reproduced both current failure modes.
Design Notes
The cross-runtime updater has a coherent design: package identity, exact host executables, marketplace scope, backend preservation, and artifact-refresh failure status are separated cleanly. Hermes now uses sibling staging and durable ownership metadata, but its reclamation phase is not yet transactionally safe at the final recursive deletion boundary.
Design / Roadmap Gate
The updater passes the affected contract gate for installation identity, exact executable reuse, scoped host mutation, configured-backend preservation, sibling refresh continuation, failure propagation, and restart messaging. Hermes staging, marker authentication, rollback, and normal interruption recovery also pass the reviewed paths. The persistence gate remains open because reclamation performs a recursive destructive operation after ownership validation, and an interrupted cleanup has no safe completion path when the retained tree no longer matches its recorded fingerprint.
Directional Notes
Review focus followed the PR’s cross-runtime status and backend-preservation requirements, plus the established maintainer standard that persistent setup artifacts must preserve operator state across retries, interruption, and concurrent mutation. The memory artifacts were used only to select these inspection points; the blocker above is independently demonstrated by current source and a focused runtime probe.
Test Coverage
- Reviewed the changed Hermes, setup-refresh, setup, and update tests.
- Ran the focused selection covering Hermes, setup refresh,
--runtime all, and related setup paths:102 passed, 561 deselected. - Ran Ruff on all changed source and test files: all checks passed.
- Existing tests cover mutation after the private reclamation rename, but not mutation immediately before recursive reclamation deletion or partial cleanup replay; the focused runtime probe reproduced both current failure modes.
Merge Recommendation
Request changes until Hermes retired-generation reclamation is safe at the actual deletion boundary and replayable after partial cleanup. The --runtime all orchestration and its failure reporting otherwise appear merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: ec3a081
request_id: req_1786870314_2070-retry-1-retry-2
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 803b8f49e5e2494a19de327cc95dbd8fa6bbed35 |
| Request ID | req_1786871678_2076 |
| Review record | bd971f51-50c0-4ea9-b4d8-c5b69c95bee2 |
What Improved
- Adds
--runtime allorchestration for Claude, Codex, and previously installed runtime artifacts while preserving the configured execution backend. - Uses installation-bound console and host executable identities, scopes Codex mutation to the Ouroboros marketplace, and reports partial refresh failures through a nonzero update result.
- Improves Hermes setup ordering, rollback, ownership authentication, interruption recovery, and bounded retired-generation reclamation.
- Documents the Claude reload and Codex restart contract.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace, refresh the Claude plugin, and refresh installed artifacts | Met |
| Preserve the configured execution backend rather than selecting a host as the backend | Met |
| Document that Claude may reload plugins while active Codex sessions must restart | Met |
| Propagate incomplete installed-artifact refreshes as partial update failures | Met |
| Refresh installed artifacts without deleting concurrent operator-owned state | Not met |
| Close linked issue #2110 | N/A — the issue body was not included in the supplied review artifacts |
Prior Findings Status
Earlier concerns about truthful cross-runtime failure propagation, Hermes activation ordering, rollback, ownership markers, and interruption recovery are withdrawn because current source and focused tests show those paths have been addressed. The durable-reclamation concern is maintained in narrower form: the manifest makes partial deletion replayable, but current source still performs a non-atomic per-entry check followed by pathname deletion, preserving the operator-data-loss race.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:252 | BLOCKING | _finish_reclamation() still has a destructive check-to-unlink race. It fingerprints each pathname with _reclamation_entry(path) and then separately calls path.unlink() or path.rmdir(). A concurrent operator can replace the entry after the check but before deletion, causing the replacement—which was never part of the authenticated retired generation—to be deleted. A focused runtime probe replaced victim.txt immediately after its second _reclamation_entry() check; _finish_reclamation() then completed successfully and removed the replacement and reclamation directory (victim_exists=False). The initial whole-tree comparison and manifest do not close this final pathname race. Reclamation must avoid deleting through a pathname after validation, or use an ownership-safe primitive that binds validation and removal; add an exact-boundary regression proving a replacement made between the final check and deletion survives. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 253 passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes or hermes_artifact': 12 passed, 400 deselected. - Existing tests cover refresh orchestration, failure propagation, setup rollback, interruption replay, symlink refusal, and reclamation additions, but not replacement between the final per-entry fingerprint and pathname deletion.
- A focused runtime probe independently reproduced deletion of a concurrent replacement at that exact boundary.
Design Notes
The cross-host updater orchestration is appropriately separated from backend configuration and uses explicit installation and executable identities. Hermes publication now has substantial transactional and replay machinery, but its reclamation phase still crosses an unauthenticated destructive pathname boundary.
Design / Roadmap Gate
Package ownership, exact executable reuse, marketplace scope, configured-backend preservation, sibling refresh continuation, failure reporting, restart messaging, Hermes activation ordering, rollback, and normal interruption replay pass the affected-boundary gate. Persistence safety does not: authenticated manifest entries are rechecked before deletion, but validation and pathname removal remain separate operations, so a concurrent replacement can be silently discarded.
Directional Notes
Review focus followed the established requirements for backend non-reselection, scoped host mutation, truthful partial status, transactional setup effects, replay-safe durable state, and operator-data preservation. Maintainer memory was used only to prioritize the final reclamation boundary; the blocker is independently established by current source and the focused runtime probe.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 253 passed. - Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/cli/test_setup.py -k 'setup_hermes or hermes_artifact': 12 passed, 400 deselected. - Existing tests cover refresh orchestration, failure propagation, setup rollback, interruption replay, symlink refusal, and reclamation additions, but not replacement between the final per-entry fingerprint and pathname deletion.
- A focused runtime probe independently reproduced deletion of a concurrent replacement at that exact boundary.
Merge Recommendation
Request changes until Hermes reclamation eliminates the final check-to-unlink race and includes an exact-boundary regression proving concurrent replacements survive. The --runtime all orchestration and failure-reporting contract otherwise appear merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 803b8f4
request_id: req_1786871678_2076
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 27f5f468b4920140f84a7d486a50501d66aa7da5 |
| Request ID | req_1786873025_2081 |
| Review record | 35bd8086-2a0c-4614-86e1-cc2fc0b4edf7 |
What Improved
- Adds
--runtime allwith scoped Claude and Codex plugin refreshes, installed-artifact refresh, backend preservation, truthful partial-failure reporting, and clear restart guidance. - Hermes publication now has substantially stronger staging, ownership validation, bounded retirement reclamation, interruption recovery, symlink refusal, and focused regression coverage.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace, refresh the Claude plugin, and refresh installed artifacts | Met |
| Preserve the configured execution backend rather than selecting a host as the new backend | Met |
| Document Claude reload and Codex restart behavior | Met |
| Keep setup and refresh failures observable as an incomplete update | Met |
Prior Findings Status
Prior concerns around --runtime all failure propagation, Hermes activation ordering, ownership markers, interruption replay, final destructive-boundary validation, and bounded retired-generation reclamation are withdrawn based on the current source and passing focused tests. A new blocker remains at the caller’s publication-receipt boundary: rollback can misclassify a concurrent operator edit as setup-owned.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:3030 | BLOCKING | The Hermes rollback receipt is captured after _install_hermes_artifacts() returns rather than at the atomic publication boundary. An operator edit made after atomic_swap_generation() commits but before this snapshot is therefore included in hermes_skill_published; if subsequent host registration fails, restore_hermes() considers that edited tree the setup-owned expected generation and replaces it with the prior snapshot, deleting the operator edit. A focused runtime probe reproduced this by adding operator-after-publish.txt before the installer returned, forcing registration failure, and observing the file removed. The installer must return or durably expose the exact generation identity captured at commit, so rollback only replaces the generation actually published by setup. Add a regression covering this publication-to-receipt race. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the new update, setup-refresh, Hermes setup, rollback, ownership, reclamation, and interruption tests.
tests/unit/hermes/test_artifacts.py: 54 passed.tests/unit/cli/commands/test_setup_refresh.pyplustests/unit/cli/test_update.py: 200 passed.- Hermes-selected
tests/unit/cli/test_setup.py: 21 passed, 391 deselected. - Ruff passed for all changed Python files and tests; MyPy passed for the five affected source modules.
- An additional focused runtime probe reproduced the blocking post-publication rollback race.
Design Notes
The cross-runtime updater design is appropriately scoped and preserves installation and backend identity. The Hermes durable publication state machine is substantially improved, but its setup caller still lacks an exact atomic-publication receipt for safe rollback.
Design / Roadmap Gate
Package ownership, exact executable reuse, marketplace scope, backend non-reselection, sibling refresh continuation, failure reporting, restart messaging, staging, marker authentication, interruption replay, and bounded retirement reclamation pass the affected-boundary gate. Transactional rollback does not: the caller derives ownership from a post-return filesystem snapshot rather than the exact generation committed by the publisher, leaving a narrow but destructive concurrent-edit window.
Directional Notes
Review focus followed the established requirements for scoped host mutation, truthful partial status, transactional setup effects, replay-safe durable state, bounded reclamation, and preservation of operator-owned changes. Maintainer memory was advisory only; the blocker is independently established by current source and a focused runtime probe.
Test Coverage
- Reviewed the new update, setup-refresh, Hermes setup, rollback, ownership, reclamation, and interruption tests.
tests/unit/hermes/test_artifacts.py: 54 passed.tests/unit/cli/commands/test_setup_refresh.pyplustests/unit/cli/test_update.py: 200 passed.- Hermes-selected
tests/unit/cli/test_setup.py: 21 passed, 391 deselected. - Ruff passed for all changed Python files and tests; MyPy passed for the five affected source modules.
- An additional focused runtime probe reproduced the blocking post-publication rollback race.
Merge Recommendation
Request changes until Hermes installation returns an exact publication identity that rollback can validate, with a regression proving edits made immediately after publication are preserved when later activation fails. The --runtime all orchestration and durable reclamation work otherwise appear merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 27f5f46
request_id: req_1786873025_2081
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 19d7657e6035c3ce5d0e083d6032292ad1257c71 |
| Request ID | req_1786873956_2085 |
| Review record | 182bbade-1227-429a-a51d-4754808410cb |
What Improved
- Adds
--runtime allto refresh Claude and Codex integrations together while preserving the configured Ouroboros backend. - Scopes Codex marketplace mutation to
ouroboros, refreshes installed runtime artifacts throughsetup refresh, and propagates partial failures. - Adds transactional Hermes staging, ownership markers, rollback receipts, interruption recovery, and extensive failure-path coverage.
- Documents host-session restart behavior for Claude and Codex.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex host integrations after a package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh the Claude plugin | Met |
| Refresh all previously installed runtime artifacts without changing the configured execution backend | Partially met — the normal path preserves backend state, but crash-retry failures can permanently prevent subsequent artifact refresh |
| Propagate per-runtime refresh failures as a nonzero partial-update result | Met |
| Document Claude reload/restart behavior and the requirement to restart active Codex sessions | Met |
Prior Findings Status
Round 38’s rollback-ownership concern is maintained as resolved: _install_hermes_artifacts() now requests a HermesPublicationReceipt, and _setup_hermes() validates that exact published generation before rollback; the added publication-edit regression covers that boundary. Earlier updater failure-propagation, backend-preservation, scoped marketplace, and activation-ordering concerns remain resolved. The two current blockers are newly verified recovery failures in later Hermes reclamation transitions.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:390 | BLOCKING | The generation -> retirement rename is not replay-safe when os.replace() commits but raises afterward. The cleanup container remains present while the retirement directory also exists; on retry, _recover_swap_intents() reaches the cleanup.exists() branch and rejects the state as “ambiguous Hermes cleanup generations” instead of completing or repairing it. A focused probe reproduced: first refresh raises after the committed rename, and the next refresh permanently fails with OSError. Add committed-rename interruption handling and a regression that retries successfully. |
| 2 | src/ouroboros/hermes/artifacts.py:473 | BLOCKING | The retirement -> reclamation recovery window loses its manifest if os.replace(path, reclamation) commits and then raises. The exception handler unconditionally removes the manifest, leaving the .reclaim generation without the durable record required by _finish_reclamation(). A subsequent refresh finds the reclamation directory but fails with FileNotFoundError reading *.reclaim.manifest.json, so future artifact refreshes remain blocked. Preserve the manifest when the destination rename committed, or reconstruct/validate it during recovery, and add a commit-then-raise retry regression. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None. |
Test Coverage Notes
- Reviewed the changed Hermes artifact, setup-refresh, setup, and update tests.
254focused tests passed acrosstests/unit/hermes/test_artifacts.py,tests/unit/cli/commands/test_setup_refresh.py, andtests/unit/cli/test_update.py.- Hermes setup selection passed
13tests. - Existing tests cover interruption before or during several renames, but do not cover the two committed-rename-then-raise windows above; direct probes reproduced both retry failures.
Design Notes
The updater orchestration is well-factored and preserves installation identity, host executable selection, backend configuration, and truthful partial status. The Hermes artifact publisher has a strong ownership and staging model, but its multi-step reclamation state machine still contains non-replayable committed-rename windows.
Design / Roadmap Gate
The --runtime all contract, scoped host mutations, backend non-reselection, setup-refresh isolation, failure reporting, and documentation fit the stated design. Hermes staging and ownership validation also pass the ordinary success and several interruption paths. The persistence/replay gate remains open because two later atomic rename transitions can leave durable intent state that the next refresh rejects or cannot interpret, preventing recovery and leaving installed artifacts stale.
Directional Notes
Review focus followed the PR body’s backend-preservation and cross-runtime refresh requirements, prior review standards for truthful failure propagation, and the maintainer posture that persistent setup artifacts must preserve operator state and remain replay-safe after interruption. The memory artifacts were used only to select these inspection points; both blockers are independently reproduced from the current source snapshot.
Test Coverage
- Reviewed the changed Hermes artifact, setup-refresh, setup, and update tests.
254focused tests passed acrosstests/unit/hermes/test_artifacts.py,tests/unit/cli/commands/test_setup_refresh.py, andtests/unit/cli/test_update.py.- Hermes setup selection passed
13tests. - Existing tests cover interruption before or during several renames, but do not cover the two committed-rename-then-raise windows above; direct probes reproduced both retry failures.
Merge Recommendation
Request changes. Fix both committed-rename recovery windows and add exact retry regressions before merge. The broader updater behavior and existing focused coverage are otherwise directionally strong.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 19d7657
request_id: req_1786873956_2085
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Fixed both committed-rename recovery windows:
Added exact retry regressions for both windows. Verification: Hermes artifacts 56 passed; Hermes/setup-refresh/update focused selection 283 passed; Ruff, format, MyPy, module-size, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | abbc508c1298d05816af78743e1d319e23fbb924 |
| Request ID | req_1786875225_2090 |
| Review record | ba41c785-c678-418a-9bcd-d8d778fc36da |
What Improved
- Adds
--runtime allorchestration for Claude and Codex while preserving the configured execution backend. - Uses scoped Claude/Codex marketplace operations, the proven installation console, truthful partial-failure reporting, and documented restart behavior.
- Substantially improves Hermes publication with sibling staging, ownership receipts, rollback checks, bounded retirement, and interruption recovery.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh the Claude plugin | Met |
Run ouroboros setup refresh for previously installed runtime artifacts |
Partially met — orchestration and failure propagation work, but Hermes artifact publication still has failure-state and operator-data hazards |
| Preserve the configured execution backend rather than selecting a host as the backend | Met |
| Continue remaining integration refreshes and return nonzero for partial failures | Met |
| Document Claude reload and active Codex restart behavior | Met |
| Linked issue #2110 details beyond the PR body | N/A — separate issue text was not included in the review artifacts |
Prior Findings Status
The prior round’s two committed-rename recovery concerns are addressed in the current source and corresponding retry regressions, so those specific blockers are withdrawn. Applying the same replay-safety and operator-state-preservation standard to the current snapshot identifies the fresh-publication and reclamation-manifest blockers above.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:808 | BLOCKING | A fresh Hermes publication is not recoverable when os.replace(replacement, target) commits and then raises. With no prior target, backup does not exist, so the exception branch deletes the intent even though target now contains the published generation. install_hermes_skills() then reports failure, _setup_hermes() does not activate or roll back, but the persistent global skill tree remains installed. A focused probe reproduced OSError with the new target present and no intent or marker. Preserve enough publication identity to detect and remove or recover an exact committed fresh generation, and add an end-to-end failed-first-activation regression. |
| 2 | src/ouroboros/hermes/artifacts.py:208 | BLOCKING | Reclamation publishes its manifest through _publish_swap_intent(), whose os.replace(staging, manifest) unconditionally overwrites an existing destination. The manifest pathname is predictable from the retained retirement directory, and unlike the reclamation directory itself it is never checked for ownership or occupancy. A focused probe created an operator-owned <retirement>.reclaim.manifest.json; the next swap silently deleted it and completed. Refuse or authenticate an existing manifest before replacement, while separately recognizing legitimate interrupted reclamation state, and add a hostile-sibling regression. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:308 | Medium | High | If reclamation-directory rmdir() commits and then raises, the sibling manifest remains permanently because later scans ignore the manifest-only state. A focused retry reproduced the orphan. Extend the durable protocol so this final committed transition can be recognized and safely completed without deleting an unauthenticated sibling file. |
Non-blocking Suggestions
| None. | N/A | N/A | N/A |
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 256 passed. - Ran focused Hermes setup/rollback tests from
tests/unit/cli/test_setup.py: 13 passed, 400 deselected. - Ran focused runtime probes reproducing the committed fresh-publication leak, operator manifest overwrite, and orphaned final reclamation manifest.
- The PR’s targeted tests are extensive, but they do not cover the two blocking boundaries above.
Design Notes
The cross-runtime updater design is directionally sound and keeps package identity, host integration, and backend selection appropriately separated. The Hermes durability layer is carefully structured but still lacks complete ownership and replay handling at two publication metadata boundaries.
Design / Roadmap Gate
Installation identity, exact executable reuse, scoped marketplace mutation, backend non-reselection, sibling refresh continuation, partial-status reporting, restart messaging, existing-generation staging, rollback receipts, and the previously blocked committed rename transitions pass the affected-boundary gate. Persistence safety remains open because a failed first publication can leave unactivated global artifacts, and reclamation can overwrite an unauthenticated operator-owned manifest sibling.
Directional Notes
Review focus followed the PR body’s backend-preservation and combined-refresh requirements, prior review standards for truthful failure propagation, and the advisory maintainer memory emphasizing transactional persistent setup writes, exact artifact ownership, and replay-safe recovery. Both blockers are independently demonstrated by the current source and focused probes.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.py: 256 passed. - Ran focused Hermes setup/rollback tests from
tests/unit/cli/test_setup.py: 13 passed, 400 deselected. - Ran focused runtime probes reproducing the committed fresh-publication leak, operator manifest overwrite, and orphaned final reclamation manifest.
- The PR’s targeted tests are extensive, but they do not cover the two blocking boundaries above.
Merge Recommendation
Request changes until fresh-publication commit-then-error recovery leaves no unactivated artifact, reclamation manifest publication preserves foreign siblings, and focused regressions cover both exact boundaries. The broader --runtime all orchestration is otherwise merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: abbc508
request_id: req_1786875225_2090
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed both blockers and the adjacent final-transition follow-up:
Verification: Hermes artifacts/setup-refresh/update 259 passed; Hermes setup selection 23 passed; Ruff, format, MyPy, module-size, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | b0dc241fe639de549a5abb78011ed3f24262b5eb |
| Request ID | req_1786876189_2094 |
| Review record | a6e95525-c378-488a-a8a1-c8eaaaa47661 |
What Improved
- Adds
--runtime allto refresh Claude Code, Codex, and previously installed runtime artifacts while preserving the configured execution backend. - Propagates partial artifact-refresh failures to a nonzero updater result and continues refreshing independent integrations.
- Strengthens Hermes publication with staged generations, authenticated recovery records, rollback receipts, symlink refusal, and interruption-recovery coverage.
- Documents the Claude reload and Codex session-restart contract.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after upgrade |
Met |
Upgrade only the Ouroboros Codex marketplace, refresh the Claude plugin, and run ouroboros setup refresh |
Met |
| Preserve the configured execution backend rather than selecting a host backend | Met |
| Document Claude plugin reload and required Codex session restart behavior | Met |
| Preserve operator-owned persistent artifacts throughout refresh, rollback, recovery, and cleanup | Not met |
Prior Findings Status
The prior round’s specific fresh-publication commit-then-error rollback and occupied-manifest publication concerns are addressed and withdrawn. The established transactional ownership standard remains applicable: current HEAD still has adjacent final-mutation races during fresh publication and reclamation-manifest deletion.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/hermes/artifacts.py:840 | BLOCKING | Fresh publication does not preserve the initial “target absent” generation at the final mutation boundary. checked_fingerprint may be None, target.exists() is checked only at line 799, and the later os.replace(replacement, target) can overwrite a concurrently created empty operator directory. A focused probe creating the target immediately before this call showed publication succeeding, the concurrent directory being replaced, and the intent being removed. Use a no-replace publication/reservation or equivalent ownership protocol, and add a regression where the target appears between the absence check and final rename. |
| 2 | src/ouroboros/hermes/artifacts.py:508 | BLOCKING | Orphan reclamation-manifest cleanup validates the manifest and then unlinks the pathname without preserving the validated object’s identity; _finish_reclamation() has the same pattern at line 323. A concurrent operator replacement between read and unlink is therefore deleted. A focused probe replaced a valid orphan manifest immediately before manifest.unlink() and confirmed the operator file disappeared. Atomically detach the manifest to a private path, validate that detached object, and delete only that exact generation, with regressions for both orphan recovery and normal reclamation completion. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Focused Hermes/setup-refresh/updater/setup selection: 117 passed, 556 deselected.
- Full changed-module run: 650 passed, 1 skipped, 22 failed; failures were unrelated Codex MCP launch-command detection tests in this tarball environment.
- Independently reproduced both blocking races with focused runtime probes.
- Existing tests cover foreign manifest occupation before publication and committed reclamation interruptions, but not replacement at manifest deletion or concurrent target creation at fresh publication.
Design Notes
The combined updater orchestration is well-scoped and keeps backend selection separate from host refresh. The Hermes transaction model is substantially improved, but publication and reclamation still need exact identity protection at their final destructive filesystem operations.
Design / Roadmap Gate
Installation identity, exact host executable reuse, marketplace scope, sibling refresh continuation, truthful partial status, restart messaging, staged Hermes copies, rollback receipts, and documented recovery paths pass the affected-boundary gate. Persistence safety remains open because fresh publication can replace a concurrently created target and manifest reclamation can unlink a concurrently replaced sibling.
Directional Notes
Review focus followed the PR body’s combined-refresh and backend-preservation requirements, plus the established maintainer standard that durable setup artifacts must be replay-safe and preserve concurrent operator state. Maintainer memory was advisory only; both blockers are independently established from current source and runtime probes.
Test Coverage
- Focused Hermes/setup-refresh/updater/setup selection: 117 passed, 556 deselected.
- Full changed-module run: 650 passed, 1 skipped, 22 failed; failures were unrelated Codex MCP launch-command detection tests in this tarball environment.
- Independently reproduced both blocking races with focused runtime probes.
- Existing tests cover foreign manifest occupation before publication and committed reclamation interruptions, but not replacement at manifest deletion or concurrent target creation at fresh publication.
Merge Recommendation
Request changes until fresh publication cannot replace a concurrently created target and reclamation cleanup deletes only the exact authenticated manifest generation. Add exact-boundary regressions for both races; the broader --runtime all behavior otherwise appears merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: b0dc241
request_id: req_1786876189_2094
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed both final-mutation races:
Verification: Hermes artifact suite 62 passed; Ruff, format, MyPy, module-size, and diff checks pass. The broader CLI bundle had one unrelated existing Codex snapshot-mock signature failure; all Hermes-targeted tests passed. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 4daa2c2bf0da6fd9b661d4f8abbe35712e9b5e73 |
| Request ID | req_1786878369_2102 |
| Review record | 52d9cf31-d0db-4d80-8942-aa1f9f97ea81 |
What Improved
- Adds
--runtime allto refresh Claude Code, Codex, and previously installed runtime artifacts while preserving the configured execution backend. - Scopes Codex mutation to the Ouroboros marketplace and reports partial refresh failures through a nonzero updater result.
- Adds extensive Hermes publication, ownership, concurrent-mutation, and interruption-recovery coverage.
- Documents host restart behavior for Claude and Codex.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations |
Met |
| Upgrade only the Ouroboros Codex marketplace and refresh the Claude plugin | Met |
Refresh previously installed runtime artifacts through ouroboros setup refresh |
Partially met — normal and reported failure paths work, but Hermes post-publication snapshot failure leaves an uncommitted artifact |
| Preserve the configured execution backend instead of selecting a host backend | Met |
| Document Claude reload and Codex restart behavior | Met |
| Keep affected setup and rollback verification green | Not met — the Codex generation-snapshot regression fails because of the changed helper invocation |
Prior Findings Status
The prior concerns around truthful partial-refresh reporting, backend preservation, fresh-target no-replace publication, and authenticated reclamation appear addressed in the current source and focused tests. The durability concern is maintained in modified form: the remaining blocker is the newly exposed post-publication snapshot failure window. The contributor’s claim that the Codex mock-signature failure is pre-existing is withdrawn based on current-diff evidence; the changed recursive call directly triggers it.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/cli/commands/setup.py:3039 | BLOCKING | The new post-publication _snapshot_path() call is outside any error handling or rollback scope. If snapshotting raises after _install_hermes_artifacts() successfully publishes the skill generation—for example because a file disappears or becomes unreadable—_setup_hermes() propagates the OSError, never attempts host/config activation, and leaves the newly published global Hermes artifact installed despite activation remaining uncommitted. A focused probe reproduced exactly this state: the exception escaped, the target remained present, the original config remained unchanged, and host registration was never called. Catch this failure and safely roll back using the publication receipt/current-generation validation, with a regression covering post-publication snapshot failure. |
Follow-up Findings
src/ouroboros/cli/commands/setup.py:2340[warning] Propagatingfollow_linksthrough recursive calls changed the existing_snapshot_pathpatch contract without updating its regression test.TestCodexSetup::test_install_codex_artifacts_restores_skill_when_generation_snapshot_failsnow deterministically fails withTypeError: ... unexpected keyword argument 'follow_links'attests/unit/cli/test_setup.py:3092. This is introduced by the current diff, not a pre-existing Python 3.14 failure. Update the test seam or preserve compatible recursive invocation so the affected rollback suite is green.
| # | File:Line | Priority | Confidence | Suggestion |
|---|-----------|----------|------------|------------|
|---|-----------|----------|------------|------------|
| None. | N/A | N/A | N/A | None. |
Non-blocking Suggestions
| None. | N/A | N/A | None. |
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --python 3.14 pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.pypassed: 262 tests.tests/unit/cli/test_setup.py -k setup_hermespassed: 14 tests.- The combined affected-file run completed with 653 passed, 1 skipped, and 22 failed. Most failures depended on unavailable MCP-launcher detection in this tarball environment, but the isolated Codex snapshot regression above fails deterministically because of the changed recursive keyword argument.
- A direct runtime probe confirmed that a post-publication Hermes snapshot error escapes and leaves the published artifact behind without committing configuration or host registration.
Design Notes
The combined updater orchestration and scoped host refresh design fit the stated goal. The Hermes publication protocol has strong ownership and replay protections, but the setup caller must treat obtaining rollback state as part of the publication transaction rather than allowing an exception between publication and rollback registration.
Design / Roadmap Gate
The updater’s package identity, host selection, backend non-reselection, sibling refresh continuation, failure reporting, and restart contracts pass. Hermes ordinary publication, no-replace behavior, ownership validation, and interruption reclamation are well covered. The activation boundary remains unsafe when rollback-state snapshotting fails after publication, and the affected shared snapshot helper currently leaves a deterministic rollback test red.
Directional Notes
Review focused on backend preservation, exact host executable reuse, scoped marketplace mutation, truthful partial status, and replay-safe persistent artifact publication. Maintainer memory was advisory only; both blockers are independently established by current source, targeted pytest execution, and a focused runtime probe.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run --python 3.14 pytest -q tests/unit/hermes/test_artifacts.py tests/unit/cli/commands/test_setup_refresh.py tests/unit/cli/test_update.pypassed: 262 tests.tests/unit/cli/test_setup.py -k setup_hermespassed: 14 tests.- The combined affected-file run completed with 653 passed, 1 skipped, and 22 failed. Most failures depended on unavailable MCP-launcher detection in this tarball environment, but the isolated Codex snapshot regression above fails deterministically because of the changed recursive keyword argument.
- A direct runtime probe confirmed that a post-publication Hermes snapshot error escapes and leaves the published artifact behind without committing configuration or host registration.
Merge Recommendation
Request changes until post-publication Hermes snapshot failures are converted into a safe, current-generation-validated rollback and the affected Codex snapshot regression passes. The broader --runtime all implementation otherwise appears directionally merge-ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 4daa2c2
request_id: req_1786878369_2102
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed the remaining activation window and snapshot regression:
Added a post-publication snapshot failure regression proving prior skills/config remain and host registration is not called. Verification: 16 focused setup/Hermes rollback tests and the reviewer-specified Hermes/setup-refresh/update bundle (262 tests) pass; Ruff, format, MyPy, module-size, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|---|---|
| PR | #2112 |
| HEAD checked | 1c20ad0caf97acbf11e3045d69cda65c4f6a5018 |
| Request ID | req_1786879658_2107 |
| Review record | c0f76611-c7ac-42ce-a009-08f43376d3ce |
What Improved
- Adds
--runtime allto refresh Claude Code, Codex, and previously installed runtime artifacts while preserving the configured execution backend. - Propagates per-runtime artifact failures into a truthful partial-update exit status while continuing independent refreshes.
- Makes Hermes publication and rollback receipt-bound, failure-atomic, symlink-safe, ownership-validated, and replayable across interrupted rename and cleanup transitions.
- Documents the Claude reload and Codex restart requirements.
Issue Requirements
| Requirement | Status |
|---|---|
Add --runtime all to refresh Claude Code and Codex integrations after package upgrade |
Met |
| Upgrade only the Ouroboros Codex marketplace | Met |
| Refresh the Claude plugin | Met |
Refresh previously installed runtime artifacts through ouroboros setup refresh |
Met |
| Preserve the configured execution backend rather than selecting a host as the backend | Met |
| Continue independent refresh operations and report incomplete integrations as failure | Met |
| Document Claude plugin reload and active Codex session restart behavior | Met |
| Close #2110 and remain directionally compatible with referenced #2109 / Codex restart context | Met |
Prior Findings Status
The prior blocking concerns are withdrawn. Current source independently shows receipt-bound rollback after Hermes publication failure, and both the previously failing shared Codex snapshot regression and the Hermes post-publication rollback regression pass. Earlier failure-propagation, ownership, race, and replay concerns are also covered by the current state machine and focused tests.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. | N/A | N/A | N/A | None. |
Non-blocking Suggestions
| None. | N/A | N/A | None. |
Test Coverage Notes
- Reviewed the changed updater, setup-refresh, setup transaction, Hermes publication/recovery, documentation, and associated tests.
tests/unit/cli/test_update.pyplustests/unit/cli/commands/test_setup_refresh.py: 200 passed.- Hermes-focused
tests/unit/hermes/test_artifacts.pyandtests/unit/cli/test_setup.py: 86 passed, 391 deselected. - Prior critical regressions for Codex generation-snapshot failure and Hermes post-publication rollback: 2 passed.
- Ruff passed for all changed Python files and tests; MyPy passed for the four changed/new runtime modules.
Design Notes
The implementation keeps package identity verification, host plugin refresh, installed-artifact refresh, and runtime configuration as distinct boundaries. The shared Hermes generation machinery is complex but now uses explicit intents, authenticated ownership, atomic publication, receipt-based rollback, and fail-closed recovery.
Design / Roadmap Gate
The affected boundary passes: --runtime all does not write runtime configuration or MCP registrations, host absence is handled without substituting another backend, child refresh failure reaches the updater’s nonzero result, and Hermes activation publishes required artifacts before committing host/config state. Rollback and recovery authenticate the exact generation at destructive transitions, preserve concurrent changes, reject symlinked or foreign state, and retain durable intent where retry is required.
Directional Notes
Review focused on backend non-reselection, exact executable reuse, scoped marketplace mutation, truthful partial status, transactional setup ordering, preservation of operator-owned files, and replay-safe Hermes publication. Maintainer memory was used only to guide inspection; the approval is based on the current source and executed tests.
Test Coverage
- Reviewed the changed updater, setup-refresh, setup transaction, Hermes publication/recovery, documentation, and associated tests.
tests/unit/cli/test_update.pyplustests/unit/cli/commands/test_setup_refresh.py: 200 passed.- Hermes-focused
tests/unit/hermes/test_artifacts.pyandtests/unit/cli/test_setup.py: 86 passed, 391 deselected. - Prior critical regressions for Codex generation-snapshot failure and Hermes post-publication rollback: 2 passed.
- Ruff passed for all changed Python files and tests; MyPy passed for the four changed/new runtime modules.
Merge Recommendation
Approve. No current-HEAD blocking defect remains, and focused validation covers the updater orchestration, partial-failure contract, prior shared-snapshot regression, and high-risk Hermes publication, rollback, ownership, and recovery paths.
Review-Metadata:
verdict: APPROVE
head_sha: 1c20ad0
request_id: req_1786879658_2107
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2 participants
Summary
--runtime allto refresh both Claude Code and Codex host integrations after a package upgrade.ouroboros setup refreshfor installed artifacts.Test plan
uv run --python 3.14 --no-sync pytest -q tests/unit/cli/test_update.py— 184 passeduv run --python 3.14 --no-sync pytest -q tests/unit/skills/test_skill_artifacts.py -k "not test_path_shaping_poison_really_breaks_an_unsanitized_python"— 50 passed, 5 deselecteduv run --python 3.14 --no-sync ruff check src/ tests/uv run --python 3.14 --no-sync ruff format --check src/ tests/uv run --python 3.14 --no-sync mypy src/ouroboros/cli/commands/update.pyuv runsanitizesPYTHONEXECUTABLEand__PYVENV_LAUNCHER__Related issues
Closes #2110
Refs #2109
Refs openai/codex#31383