Skip to content

Verify runpath does not contain a file - #14370

Merged
tbwiik merged 1 commit into
equinor:mainfrom
tbwiik:faulty-runpath-error-msg
Sep 9, 2026
Merged

Verify runpath does not contain a file#14370
tbwiik merged 1 commit into
equinor:mainfrom
tbwiik:faulty-runpath-error-msg

Conversation

@tbwiik

@tbwiik tbwiik commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Issue
Resolves #14265

Approach
If the user provide a runpath, where parts of the runpath contain an existing file,
this is not a valid directory and a config error is thrown.

gui
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When screenshots are changed: Review screenshot-PR in ert-testdata,
    merge screenshot-PR in ert-testdata before merging this PR.
  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@tbwiik tbwiik added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Sep 4, 2026
@tbwiik

tbwiik commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Should this be backported?

@codecov-commenter

codecov-commenter commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.96%. Comparing base (03e5ba6) to head (95733a6).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14370      +/-   ##
==========================================
+ Coverage   90.81%   90.96%   +0.15%     
==========================================
  Files         497      495       -2     
  Lines       36063    36112      +49     
==========================================
+ Hits        32751    32851     +100     
+ Misses       3312     3261      -51     
Flag Coverage Δ
cli-tests ?
fuzz 43.76% <88.23%> (-0.10%) ⬇️
gui-tests 58.52% <88.23%> (+0.38%) ⬆️
performance-and-unit-tests 80.95% <100.00%> (-0.10%) ⬇️
test 45.45% <88.23%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/ert/config/model_config.py 100.00% <100.00%> (ø)

... and 50 files with indirect coverage changes

Copilot AI 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.

🟡 Changes recommended

The validation mishandles legacy placeholders and can both miss invalid prefixes and reject unrelated paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds early RUNPATH validation to prevent files from being used as directory prefixes.

Changes:

  • Validates static RUNPATH components.
  • Adds tests for file and directory prefixes.
File summaries
File Description
src/ert/config/model_config.py Adds RUNPATH prefix validation.
tests/ert/unit_tests/config/test_model_config.py Tests static prefix validation.
Review details

Suppressed comments (1)

src/ert/config/model_config.py:142

  • This comprehension does not stop at the first dynamic component and only the combined path is checked. Thus base/<IENS>/later checks the unrelated base/later, while base/file/child/<IENS> misses that file is an existing regular file because the combined path does not exist. Walk each prefix until the first token and reject any existing non-directory prefix.
        # stops when entering first dynamic part
        static_parts = [part for part in parts if not _TOKEN_REGEX.search(part)]
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/ert/config/model_config.py Outdated
Comment thread tests/ert/unit_tests/config/test_model_config.py Outdated
@codspeed-hq

codspeed-hq Bot commented Sep 4, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing tbwiik:faulty-runpath-error-msg (95733a6) with main (074eb57)

Open in CodSpeed

@tbwiik
tbwiik requested a balanced review from Copilot September 4, 2026 11:15
@tbwiik
tbwiik force-pushed the faulty-runpath-error-msg branch from e267345 to 1de8b48 Compare September 4, 2026 11:15

Copilot AI 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.

🟡 Changes recommended

The validation can reject valid dynamic paths and miss files in earlier static components.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/ert/config/model_config.py Outdated
Comment thread src/ert/config/model_config.py
@berland

berland commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

(maybe already fixed, but ensure the string "PosixPath" is not part the user experience)

@tbwiik
tbwiik force-pushed the faulty-runpath-error-msg branch from 1de8b48 to b2ee759 Compare September 4, 2026 12:19
@tbwiik
tbwiik requested a balanced review from Copilot September 4, 2026 12:23

Copilot AI 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.

🟡 Changes recommended

The first-token stopping behavior needs targeted regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tests/ert/unit_tests/config/test_model_config.py Outdated
@tbwiik
tbwiik force-pushed the faulty-runpath-error-msg branch from b2ee759 to b3cf6f6 Compare September 4, 2026 12:40
@tbwiik
tbwiik requested a review from berland September 7, 2026 11:14
Comment thread src/ert/config/model_config.py
@tbwiik
tbwiik removed the request for review from berland September 7, 2026 13:00
Raise an error if the static parts of the runpath
provided by user is not a valid directory.
@tbwiik
tbwiik force-pushed the faulty-runpath-error-msg branch from b3cf6f6 to 95733a6 Compare September 8, 2026 06:47
@tbwiik
tbwiik requested a review from xjules September 8, 2026 09:58

@xjules xjules 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.

👍

@tbwiik
tbwiik merged commit d2123d5 into equinor:main Sep 9, 2026
39 checks passed
@tbwiik
tbwiik deleted the faulty-runpath-error-msg branch September 9, 2026 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes:bug-fix Automatically categorise as bug fix in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An erroneous RUNPATH can give a very long stacktrace

5 participants