Skip to content

[WRONG BRANCH] fix(integrations): fail closed when OMP block removal produces invalid YAML - #273

Closed
luvs01 wants to merge 1 commit into
mainfrom
codex/fix-omp-disable-throwing-on-valid-yaml
Closed

[WRONG BRANCH] fix(integrations): fail closed when OMP block removal produces invalid YAML#273
luvs01 wants to merge 1 commit into
mainfrom
codex/fix-omp-disable-throwing-on-valid-yaml

Conversation

@luvs01

@luvs01 luvs01 commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Removing the providers.opencodex block could leave a valid-but-dependent alias in the remaining YAML that Bun.YAML.parse fails on, propagating a parse exception as a 500 instead of refusing safely.

Description

  • Wrap the post-removal YAML parse in patchOmpYamlSource with a try/catch and return null on parse failure so the caller treats it as an unsafe refusal rather than throwing. (src/integrations/omp-yaml-source.ts).
  • Add a regression test that constructs a config where an external alias references an anchor inside the managed block and asserts that disableIntegration returns an unsafe refusal and preserves the original bytes. (tests/integrations-writer.test.ts).
  • The change preserves the existing fail-closed behavior of semanticallyMatches and does not alter successful removal paths.

Testing

  • Ran the focused writer test: bun test tests/integrations-writer.test.ts and the file's 28 tests passed.
  • Typecheck: bun x tsc --noEmit succeeded.
  • Privacy scan: bun run privacy:scan reported no issues.
  • Attempted a full test run (bun run test); unrelated timeouts/failures occurred elsewhere in the large suite (external OAuth live-update, retained-root serialization, and combo-management tests) and are not caused by this change, so the focused tests and typecheck are used to verify the fix.

Codex Task

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of invalid YAML when removing an integration, preventing parsing errors from surfacing unexpectedly.
    • Prevented disabling a managed YAML configuration when it is referenced by an external alias.
  • Tests

    • Added coverage confirming unsafe removal is rejected and the configuration file remains unchanged.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d5c17ac5-0135-4377-b723-95440c9b7021

📥 Commits

Reviewing files that changed from the base of the PR and between 1193075 and 565b5b5.

📒 Files selected for processing (2)
  • src/integrations/omp-yaml-source.ts
  • tests/integrations-writer.test.ts

📝 Walkthrough

Walkthrough

The OMP YAML provider-removal path now handles post-removal parse failures. A test verifies that disabling OMP returns unsafe and preserves the configuration when an external alias references the managed provider block.

Changes

OMP provider removal

Layer / File(s) Summary
Safe removal validation
src/integrations/omp-yaml-source.ts, tests/integrations-writer.test.ts
The removal path catches YAML parsing errors and returns null. The integration test verifies that an external alias causes disable to return unsafe and leaves the edited configuration unchanged.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 565b5

This localized change safely refuses invalid YAML during integration removal and preserves the original configuration; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: dbc-hbin, lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the fail-closed behavior for invalid OMP YAML removal, although the branch marker adds minor noise.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-omp-disable-throwing-on-valid-yaml

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

@github-actions

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

⏳ DRAFT

  • wrong target branch (main); retarget to dev.

What to do

  • Retarget this PR to dev — all contributions go to dev.

Its title has been prefixed with [WRONG BRANCH].
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.

@github-actions github-actions Bot changed the title fix(integrations): fail closed when OMP block removal produces invalid YAML [WRONG BRANCH] fix(integrations): fail closed when OMP block removal produces invalid YAML Aug 13, 2026
@github-actions
github-actions Bot marked this pull request as draft August 13, 2026 01:29

luvs01 commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@luvs01 luvs01 closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant