Skip to content

fix(cli): name the reviewed recovery for Aside profiles that read off (stale) - #6825

Merged
lidge-jun merged 10 commits into
devfrom
codex/n5-6757-aside-recovery
Oct 9, 2026
Merged

lidge-jun merged 10 commits into
devfrom
codex/n5-6757-aside-recovery

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Problem (Aside integration profiles silently go off (stale), breaking sidecar sync (no auto-recovery, client sync no-ops) #6757): ocx integration client status --client aside can show profiles as off (stale). ocx integration client sync --client aside then printed "No eligible Aside profiles to synchronize. Check integration status and profile sync preferences." and gave no next step. Sync refreshes only profiles whose sync preference is on, which is correct and unchanged.
  • Sync: an empty result now explains that rule, names ocx integration client status --client aside, and gives the preview-then-enable commands with a <N> placeholder. The enable step says to run it "if the preview permits the change and you accept it", because a refused preview can still exit 0.
  • Status: the list and --profile N views print those commands with the real ID for each profile that is off beside a stale managed block. Conflict, unsafe, current and enabled profiles get no hint.
  • Config load: a malformed asideProfileSync (for example, one non-boolean entry) still falls back to { allProfiles: false }, which turns every profile off. The config load now warns about this once and does not echo the value. This is the one silent path we found that turns all profiles off.
  • Unchanged: nothing is re-enabled and no request is added. The attested sync exchange, the management API, and all --json output stay the same. The sync response does not carry profile IDs on purpose: reading profile state runs integration maintenance (snapshot pruning), and an empty sync currently writes nothing.
  • Cause: still unproven. No automatic stale, fingerprint, attestation or restart path turns an explicit true into false. The candidates are the schema fallback above, a lost legacy-derived default (no policy, and the root ownership record no longer matching), an explicit disable or Undo, or a different OPENCODEX_HOME. Telling them apart needs the affected runtime's asideProfileSync and ownership records.
  • Lane plan unit devlog/_plan/261009_n5_small_sweep/ (N5 sweep) is included.

Closes #6757

Verification

With a temporary HOME, on this head:

  • bun test tests/cli/cli-aside-sync.test.ts tests/config/config-load-degrade.test.ts: 64 pass, 0 fail.
  • bun test tests/cli/cli-headless-parity.test.ts -t Aside: 15 pass, 0 fail.
  • bun test tests/ci-workflows/file-size-ratchet.test.ts tests/test-layout.test.ts: 11 pass, 0 fail.
  • bun run typecheck, bun run structure:check, bun run privacy:scan: all passed.
  • Red check: with the src/ changes reverted, the four new behavior tests fail: the sync guidance, the status list, the single-profile status, and the config warning.
  • The full suite, docs build, and the Windows and macOS shards were not run locally (sweep lane, minimal local scope). Exact-head hosted CI covers them.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. docs-site integrations guide, structure/cli-management.md, structure/config.md, structure/clients/integrations.md.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No auth or transport change; the warning echoes no config value. Independent review recorded before merge.

Summary by CodeRabbit

  • New Features
    • Aside status and sync commands now provide recovery guidance for profiles marked off (stale), including commands to preview and conditionally re-enable each affected profile.
    • Invalid Aside sync settings now trigger a warning and leave profiles disabled. Profiles are never re-enabled automatically.
  • Documentation
    • Updated integration and configuration guidance to explain recovery steps and the behavior of invalid settings. JSON status output remains unchanged.

… (stale)

Sync refreshes only profiles whose preference is on, so an "off (stale)"
profile left "ocx integration client sync --client aside" printing a bare
no-op. The empty result now names the status command and the preview-then-
enable commands, and status (list and --profile) prints them with the real
ID for each off profile beside a stale block. Nothing is re-enabled and no
request is added; --json output is unchanged.

A malformed asideProfileSync still falls back to all profiles off, but the
config load now says so once without echoing the value.

Closes #6757
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 9, 2026 11:30
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T11:33:37.102015Z bcb9ad3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e4eb63ab-74a4-4315-abd7-c6c21f540220

📥 Commits

Reviewing files that changed from the base of the PR and between 37e9294 and e6251d5.


📒 Files selected for processing (17)
  • devlog/_plan/261009_n5_small_sweep/000_plan.md
  • devlog/_plan/261009_n5_small_sweep/010_pr6734_thread_links.md
  • devlog/_plan/261009_n5_small_sweep/011_pr6734_review.md
  • devlog/_plan/261009_n5_small_sweep/020_pr6813_account_selection_carry.md
  • devlog/_plan/261009_n5_small_sweep/030_issue6757_aside_recovery.md
  • devlog/_plan/261009_n5_small_sweep/040_closeout.md
  • docs-site/src/content/docs/guides/integrations.md
  • src/cli/aside-profile-recovery.ts
  • src/cli/integration-aside-sync.ts
  • src/cli/integrations.ts
  • src/config/load-degrade.ts
  • structure/cli-management.md
  • structure/clients/integrations.md
  • structure/config.md
  • tests/cli/cli-aside-sync.test.ts
  • tests/cli/cli-headless-parity.test.ts
  • tests/config/config-load-degrade.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.



📝 Walkthrough

Walkthrough

The change records N5 review and closeout procedures. It adds CLI guidance for recovering disabled stale Aside profiles and warns when a persisted asideProfileSync policy is invalid. The CLI does not re-enable profiles automatically.

Changes

N5 workflow and Aside recovery

Layer / File(s) Summary
Sweep plans, review records, and closeout
devlog/_plan/261009_n5_small_sweep/*
The plans and records specify review, verification, merge, and closeout procedures for the tracked PRs and issue.
Aside CLI recovery guidance
src/cli/aside-profile-recovery.ts, src/cli/integration-aside-sync.ts, src/cli/integrations.ts, tests/cli/cli-aside-sync.test.ts, tests/cli/cli-headless-parity.test.ts, docs-site/src/content/docs/guides/integrations.md, structure/cli-management.md
An empty sync result directs the operator to status. Status output adds preview and enable commands only for disabled stale profiles with distinct, valid IDs. Tests check filtering, requests, and unchanged JSON output.
Invalid Aside sync policy warning
src/config/load-degrade.ts, tests/config/config-load-degrade.test.ts, structure/config.md, structure/clients/integrations.md
Config loading warns when a present policy is invalid and retains the all-profiles-off fallback. Tests check warning count, content, and valid or omitted policies.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature · Severity of issue fixed: Medium


Merge Risk: ⚪ Minimal · up to e6251

The change provides manual recovery instructions and a malformed-configuration warning without automatically enabling profiles. No concrete merge-blocking issue is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check Warning The functional and documentation changes support issue #6757. However, the PR also adds unrelated lane-plan and closeout material under devlog/_plan/261009_n5_small_sweep/. The reported files `010_p… Remove the unrelated #6734, #6813, and lane-administration plan entries from this pull request, or move them to a separate planning pull request. Keep the #6757 recovery plan and implementation documentation here.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title identifies the main change: CLI recovery guidance for Aside profiles reported as "off (stale)". The wording is awkward, but it remains specific and related to the primary change.
Linked Issues check Passed Issue #6757 requires either automatic re-application or clear recovery guidance. The PR implements the guidance path. src/cli/aside-profile-recovery.ts:10-15 tells users to run `ocx integration clie…
Docstring Coverage Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 7 files. (10 skipped: 10…

Full details: Out of Scope Changes check

Explanation

The functional and documentation changes support issue #6757. However, the PR also adds unrelated lane-plan and closeout material under devlog/_plan/261009_n5_small_sweep/. The reported files 010_pr6734_thread_links.md, 011_pr6734_review.md, and 020_pr6813_account_selection_carry.md plan or record work for PRs #6734 and #6813, not Aside recovery. 000_plan.md and 040_closeout.md add lane administration for those other work items. These changes have no coding connection to issue #6757.



  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 9, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration record (MAINTAINERS.md, dev exception), on the maintainer's instruction to merge this sweep lane's PRs.

  • Exact head e6251d5ab6434e38af9b596fce7ef2692b217751: every code check passed (Cross-platform CI including test 1/4 through 4/4, gates, keyring, npm-global ubuntu and windows, docker smoke, storage policy, structure gate, docs site build, react-doctor, hygiene, label). Skipped jobs did not apply to these paths.
  • enforce-target for this head was queued from 11:50 UTC for over an hour. A later run was cancelled by the repository's concurrency group. The dev ruleset has no required status checks, and every code check is green, so I'm treating that check as non-blocking under the lane's merge gate.
  • Union: this head plus fix(codex): commit explicit account selections atomically (carry #6813) #6824 merged onto dev c1a360993f without conflicts. On that union: 636 focused tests passed (account selection, auth API, config edits, pool rotation, Aside sync, config load, layout guards, file-size ratchet), the Aside status tests passed (15), typecheck was clean, and structure:check passed. structure/config.md stays at 600 lines.
  • Independent gpt-6.1-sol review: PASS. It also covered the security boundary: no new request, no auth, attestation or capability change, no preference mutation, and no value echo. Its two minor test gaps were folded in e6251d5 and passed re-check. In a red check, the four new behavior tests fail with src/ reverted.
  • No review threads or change requests are outstanding.

@lidge-jun
lidge-jun merged commit d5d6f9b into dev Oct 9, 2026
32 of 33 checks passed
@lidge-jun
lidge-jun deleted the codex/n5-6757-aside-recovery branch October 9, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant