Skip to content

Remove canonical account directories for uppercase registered IDs - #25126

Merged
nwparker merged 1 commit into
mainfrom
nwparker/managed-profile-canonical-removal
Oct 4, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/managed-profile-canonical-removal

Conversation

@nwparker

@nwparker nwparker commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​78 $\color{#cf222e}{\Huge{\mathbf{−}}}$​31 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​47
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1

ELI5

Removing an account could hide it from Orca while leaving its credentials on disk when its registered UUID used uppercase letters. Removal now uses the same lowercase directory name as account launch.

What Changed

The profile directory, temporary deletion directory and metadata rollback file use the canonical lowercase UUID. Registered IDs keep their original spelling, and a caller using a different spelling is still refused. Existing recovery tests cover uppercase registrations, deferred cleanup and failed rollback.

Why

This extends the existing directory policy to removal and preserves the current recovery mechanism. No separate account lookup or deletion implementation is needed.

Linked Issue

Addresses the account-removal finding on #24662. Credit to the reviewer for identifying the case-sensitive filesystem mismatch.

Visual Proof

N/A — this changes credential-directory removal, with no interface changes. Before/after evidence uses the installed OpenCode CLI and filesystem state.

Testing

  • Existing recovery tests reproduce three failures before the fix; all 98 focused tests across five files pass afterward.
  • Actual installed OpenCode 2.0.16 on a private case-sensitive APFS filesystem: before, removal hides metadata while auth list still reads the orphaned dummy credential; after, removal deletes that directory and the unrelated profile still works.
  • Original dummy database/configuration and unrelated credential values remain unchanged. All recorded test/CLI processes and ports closed; the owned image was detached.
  • Node typecheck, full changed-file lint/format, six changed-code gates, SAFETY and the official configured anti scan pass. CI covers the full build and other platforms.

Actual reproduction ran on macOS. Filesystem operations remain on the execution host and use existing path joins and removal callbacks for Windows, Linux, SSH and folder workspaces. No wire fields or Git commands change.

Review

Only the existing profile-removal implementation and its recovery test change. The fix preserves UUID validation, directory ownership checks, strict registered identity, metadata rollback, symlink refusal and deferred cleanup.

Agent skill upstream boundary

  • Not applicable; no upstream skill content copied or translated.

Notes

Existing historical orphan discovery is outside this focused repair. Initial test setup failures and database housekeeping differences are retained separately from the successful reproduction.

Checklist

  • This PR is small and focused.
  • User-facing before/after, mechanism and rationale explained.
  • Visual proof is N/A with a reason.
  • Self-reviewed for correctness, security and performance; independent source and actual-evidence review passed.
  • Cross-platform, SSH, folder-workspace and compatibility impact considered.
  • Focused tests and code-quality checks pass; CI covers the full build.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Canonical removal paths — ManagedDataAccountProfileRemoval.remove() now derives the profile directory, .pending-delete quarantine directory, and accounts.json.<id>.rollback name from accountId.toLowerCase(), so an uppercase registered spelling no longer resolves a non-existent directory and orphan the credentials.
  • Strict identity kept — the registered-id lookup remains case-sensitive with the case-insensitive guard throwing Managed account not found. for a differently-spelled caller, so behavior is unchanged apart from the on-disk path.
  • Recovery tests — added a deferred-cleanup case for uppercase registrations and parameterized the quarantine/rollback-failure test over lower/uppercase spellings; verified they fail without the fix (the lowercase pending directory is never quarantined) and pass with it.

The change is correct and well-scoped. Directories are always created lowercase (add() via randomUUID(), requireAccount() at service.ts:247), so removal now matches launch and startup recovery (retry() already lowercases the registered set at profile-removal.ts:173). z.uuid() accepts uppercase, so the case reaches the path logic rather than bailing at validation. I ran the focused suite locally: 25/25 pass.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 56be677a-969e-4436-acaa-71bd7126cc00
📥 Commits

Reviewing files that changed from the base of the PR and between 5df67ef and 46bb9a3.

📒 Files selected for processing (2)
  • src/main/managed-data-accounts/profile-removal-recovery.test.ts
  • src/main/managed-data-accounts/profile-removal.ts

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


📝 Walkthrough

Walkthrough

Profile removal now lowercases the UUID when building the account, pending-deletion, and rollback-backup paths. The metadata path is unchanged. Tests cover uppercase UUID removal, cleanup after restart, and recovery from rename failures for lowercase and uppercase UUIDs.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 46bb9

No actionable issue remains in the supplied review evidence; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 46bb9

The change aligns removal with the directory used to launch an account, while preserving account-ID validation and storage-confinement checks. No new attack path was established. Older interrupted-removal files and duplicate stored identities remain recovery edge cases.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed operation targets one canonical UUID directory within the selected provider's local account namespace, plus its quarantine and metadata recovery files. The inspected change does not add callers, privileges, cross-service dependencies or deployment authority.

Security Findings and Attack Paths

  • inferred — No introduced or worsened attack path was established in the inspected flow. Caller-supplied IDs remain UUID-validated and subject to the existing registration or recovery-state checks before reaching filesystem rename and deletion. Lowercasing changes addressing, not the authority required to remove a registered account.

Trust Boundaries and Controls

  • observed — The account service supplies the storage root, current persisted registration state, ownership assertion and publication callbacks. The changed method does not accept a caller-selected filesystem path; it derives paths from the provider namespace and validated UUID.

Resilience and Maintainability Implications

  • inferred — Legacy recovery remains casing-dependent: an uppercase rollback filename paired with a lowercase credential directory can cause restart recovery to find no uppercase source and discard the backup without quarantining the lowercase directory. This behavior is present at both base and head; the PR prevents that naming mismatch for new removals but does not remediate existing artifacts.
  • inferred — The persisted schema permits case-variant duplicate UUIDs, which share a launch directory. Removing one registration can leave another alias registered without credentials. The lowercase alias could already delete that directory at base, and normal creation generates UUIDs, so broader attacker exposure is not established. Production occurrence is unknown.

Hardening Proposals

  • proposed — Consider explicit, ownership-checked recovery for legacy uppercase rollback filenames paired with canonical lowercase directories, preserving recovery evidence until directory disposition is confirmed.
  • proposed — Consider enforcing case-insensitive UUID uniqueness when accepting persisted state, with an explicit recovery policy for existing duplicates that preserves registered spelling.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: account removal uses canonical lowercase directories for uppercase registered IDs.
Description check ✅ Passed The description explains the problem, change, rationale, testing, visual-proof status, and compatibility considerations. It also links the related finding and completes the relevant checklist items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · 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.

@nwparker
nwparker merged commit 2b1acd0 into main Oct 4, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant