Repository navigation
docs: describe the original OMSI 2 mode and its settings table - #550
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6PhrRqXsvNREC8Wn4vpJ8
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf28f1f52
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let doc = std::fs::read_to_string(path).unwrap(); | ||
| let a = doc.find("<!-- catalog:start -->\n").unwrap() + "<!-- catalog:start -->\n".len(); |
There was a problem hiding this comment.
Normalize checkout line endings before checking the table
On Windows checkouts using core.autocrlf=true, the Markdown file contains CRLF line endings, so this LF-only marker search returns None and unwrap() panics even though the documented table matches the catalog. The windows-workspace job in .github/workflows/depot-ci.yml runs the full workspace tests, making this a failure on that checkout configuration. Normalize CRLF to LF before locating the markers and comparing the table; adjusting only the marker search would still leave the equality assertion failing.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6PhrRqXsvNREC8Wn4vpJ8
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED (one small factual fix in the docs; everything else is fine)
Reviewed head bbf28f1f52353a743d660af9fcfb967c838a2287, based on current main (f6a59fe). Posted as a COMMENT review because the PR is opened from the same account.
Must fix
docs/ORIGINAL_ENGINE.md, "Where the code compiles": the page says its tests "run on Linux CI" and that "the Linux jobs run the module's tests". In.github/workflows/depot-ci.ymlthe only job that runsomsi-launcher-coretests iswindows-workspace(cargo test --workspace). Thequalityjob on Linux runs only-p omsi-apptests plus clippy over the workspace. Please say what is true: the module's tests run inwindows-workspaceand on a developer's machine on any desktop OS, and the Linux job only compiles and lints it. Alternatively add a Linuxcargo test -p omsi-launcher-corestep, but that is a separate change.
Checked and fine
- English only. It describes observable behavior, requirements and Depot's own files. No OMSI 2 reference, disassembled or decompiled material, no internal addresses, symbols or layouts, and no mention of upstream or forked projects. The
options.cfgkeys in the last column are the player's own config-file names, which the catalog already has on main. - Platform policy matches main.
available()inomsi-app/src/launcher/original.rsgives(RELEASED && cfg!(windows)) || DEPOT_ORIGINAL_ENGINE=1, withRELEASED = falseand OL-31. Thehttpmodule iscfg(not(target_os = "android")),ureqis a non-Android dependency, and there is a reparse-point check ininstall.rs. Paths (omsi2/settings.json,modules.json,install-record.json,launch/<id>.json,staging) and thelauncher.jsonkeys (enabled,mode,last_mode) matchconfig.rsandinstall.rs. - No implicit expansion of the hardware matrix. The page says Windows 10+ x64 only, hidden, off by default, and no macOS or Android CI, and it does not promise support.
- The generated table has 46 rows, which equals the number of catalog entries. The marker test is sound. I could not run
cargo testhere (the sandbox lacks wayland system libraries forwayland-sys), so the row-for-row equality was not executed by me. Thewindows-workspacecheck on this head must show the test passing before merge. - The commit message and CHANGELOG have no forbidden mentions. The CHANGELOG entry is under Added and says Refs #459.
Process notes (not blocking)
- The PR body checklist item "
cargo test --workspaceandcargo build --releasepass" is unchecked. No check runs were reported yet on this head, so merge only after CI is green on the exact head (7/7). - The attribution line in the body lacks the project thread link. Cosmetic.
No release implied.
Generated by Claude Code
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6PhrRqXsvNREC8Wn4vpJ8
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6PhrRqXsvNREC8Wn4vpJ8
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: APPROVED (conditional on CI green on this exact head and on #549 merging first)
Reviewed head e2bcf881e0256e48d6dbf745716c4d0180776898. Posted as a COMMENT review because the PR is opened from the same account.
Previous finding resolved. The CI paragraph now says that only windows-workspace runs the module's tests and that the Linux jobs compile it (omsi-app, clippy) without running them, which matches depot-ci.yml. The visibility bullet is now exact: Windows once the rollout flag is on, and DEPOT_ORIGINAL_ENGINE=1 on any system, which matches available().
New change checked. The test now normalizes \r\n before comparing the table, which is correct for a Windows checkout and does not weaken the check (the table is still compared byte for byte after normalization). Commit messages are neutral, with no forbidden mentions. The text is still English, behavior and requirements only, with no proprietary material, addresses or hardware-matrix expansion.
Conditions
- This doc describes behavior that #549 introduces (record-based ownership, first download URLs limited to the official repository's releases, all-or-nothing removal), so #549 should merge first. After that, #550 needs a main re-merge if it conflicts, and CI must be green on the final head.
- The
windows-workspacerun on this head must showthe_documented_table_is_the_catalogpassing. I could not execute it in my sandbox (missing wayland system libraries), so I only verified that the table has 46 rows, equal to the catalog's entry count.
No release implied.
Generated by Claude Code
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: APPROVED (conditional on CI green on this exact head)
Reviewed head b8cb99f76ea7cee361abfb0cccd91d3ba73cda05. Posted as a COMMENT review because the PR is opened from the same account.
- The only new commit over
e2bcf88is a merge ofmain(second parent is main, which now contains #549).docs/ORIGINAL_ENGINE.mdandcatalog.rsare byte-identical to the approved head, and the PR's own diff against main is still the same three files (CHANGELOG,catalog.rs, the doc). - The doc matches the #549 code now on main:
REPO = "OmsiModsBR/OmsiLaunch",install-record.jsonand thestagingfolder under~/.depot/omsi2, link and reparse-point checks, and ownership from Depot's record. The catalog still has the same 46 entries as the 46 table rows. - Still English, behavior and requirements only, no proprietary material, no addresses, no hardware-matrix expansion.
Condition: windows-workspace must be green on this head, including the_documented_table_is_the_catalog. I cannot run it in my sandbox (missing wayland system libraries).
No release implied.
Generated by Claude Code
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6PhrRqXsvNREC8Wn4vpJ8
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: APPROVED (conditional on CI green on this exact head)
Reviewed head 6e6a7e6ec7d836838bd9076720acb5039eaeec19. Posted as a COMMENT review because the PR is opened from the same account.
- The only new commit over
b8cb99fis a merge ofmain(it brings in #548, #545 and #534).docs/ORIGINAL_ENGINE.mdandcatalog.rsare byte-identical to the approved head. - The PR's own diff against main is still the same three files, +191 lines. In
CHANGELOG.mdthe diff against main is exactly the one line this PR adds, so the conflict resolution kept both sides. - The earlier checks still hold: English, behavior and requirements only, no proprietary material or addresses, no hardware-matrix expansion, and the doc matches the code on main.
Condition: windows-workspace must be green on this head, including the_documented_table_is_the_catalog. I cannot run it in my sandbox.
No release implied.
Generated by Claude Code
Requested by Leo · project thread
What changes for the player?
Nothing. This is developer documentation for the hidden original OMSI 2 mode.
How to test
Read
docs/ORIGINAL_ENGINE.md.cargo test -p omsi-launcher-core the_documented_tablechecks that the settings table in the page is the catalog's.What does this change, and why?
Adds
docs/ORIGINAL_ENGINE.md: what the mode is, the platform policy (the module compiles on every desktop platform, the interface is Windows only and hidden), how install and removal are made safe, the generated table of OMSI 2 settings, and the files Depot writes.catalog.rsgetsdocs_table()and a test that fails when the page and the catalog differ. Refs #459 (documents the platform policy of #462 and the table of #473).Checklist
cargo test --workspaceandcargo build --releasepassGenerated by Claude Code