Skip to content

test: isolate integration assets from inherited omp environment - #4351

Merged
ogulcancelik merged 2 commits into
herdrdev:masterfrom
akbash-bot:akbash/4346-omp-test-environment
Sep 18, 2026
Merged

ogulcancelik merged 2 commits into
herdrdev:masterfrom
akbash-bot:akbash/4346-omp-test-environment

Conversation

@akbash-bot

@akbash-bot akbash-bot commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Issue

Running the integration asset tests from an OMP shell fails four Oh My Pi cases. The same tests pass when OMPCODE is unset.

Problem

Test setup inherited OMPCODE=1 from the calling shell. The integration correctly treated the test instance as a nested session and registered no handlers.

How did we fix it?

Ordinary test setup now clears OMPCODE before importing the integration. The nested-session test still sets it explicitly, and teardown restores the original value. Runtime behavior is unchanged.

Verification

The four failures reproduce before the change. After rebasing, all 39 tests pass with the variable set or unset, including direct Bun execution. Formatting and clippy pass. Both review bots completed without code findings.

Local just check stopped on Rust session-save and socket-discovery tests; Can approved focused validation. macOS and Windows CI pass. Ubuntu CI fails in the unrelated Rust test federated_client_starts_without_local_and_survives_its_restart before reaching these TypeScript tests. Full validation is not green.

refs #4346

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • ai-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e9faf226-156c-488f-b6d2-7c86a47345cc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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: a38e0d6e-ef16-43d9-81d0-4da55c749181

📥 Commits

Reviewing files that changed from the base of the PR and between 68dd6ed and ff9bbbe.

📒 Files selected for processing (1)
  • src/integration/assets/herdr-agent-state.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

configureIntegrationEnvironment now deletes the inherited OMPCODE environment variable before setting test integration variables. Nested OMP-session tests can set OMPCODE explicitly afterward.

Suggested reviewers: ogulcancelik

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to ff9bb

The change is limited to test environment isolation, with no identified merge-blocking risk.

🚥 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 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely describes the main change: isolating integration tests from inherited OMP environment state.
Description check ✅ Passed The description accurately explains the inherited OMPCODE issue, the test setup change, and the validation results. It is directly related to the changeset.

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.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

@kangal-bot I will review the changes.

✅ 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.

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The test-only change appears safe to merge and preserves both ordinary and nested OMP test semantics.

Summary

This PR isolates integration asset tests from an inherited OMP shell environment by clearing OMPCODE during ordinary test setup. The explicit nested-session case continues to opt in after setup, while existing teardown restores the caller's original environment.

  • Prevents ordinary OMP asset tests from being misclassified as nested sessions.
  • Preserves explicit nested-session coverage and production runtime behavior.

Reviews (1) · Last reviewed commit: "test: isolate integration assets from in..."

@akbash-bot

Copy link
Copy Markdown
Collaborator Author

Regarding the docstring-coverage warning in #4351 (comment): this patch adds no function or API. The inline comment explains why the existing test setup clears OMPCODE; adding function docstrings would not improve coverage of the reported failure, so I am leaving this focused change as-is.

@ogulcancelik
ogulcancelik merged commit 3f2a6e7 into herdrdev:master Sep 18, 2026
6 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.

3 participants