Skip to content

fix: require OBS SDK and validate Windows plugin package - #379

Merged
azumag merged 11 commits into
mainfrom
fix/378-windows-obs-module
Oct 15, 2026
Merged

azumag merged 11 commits into
mainfrom
fix/378-windows-obs-module

Conversation

@azumag

@azumag azumag commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Problem and change

Refs #378. The reported nightly-20260813-142324 Windows ZIP (tag commit 12a3ccea) contained a standalone x64 DLL at obs-stabilizer/obs-stabilizer-opencv.dll: its export directory was empty, so OBS could not recognize it as a module.

Plugin builds now fail unless the OBS SDK is present; only an explicit BUILD_STANDALONE=ON enables the developer/test build. Windows CI builds the official OBS 30.0.0 source archive pinned by SHA-256 and applies the tracked, idempotent OBS Windows CMake target fix. Linux CI installs libobs-dev. Windows setup, configure, and build steps check $LASTEXITCODE. The preset persistence workflow explicitly selects standalone mode.

The Windows gate verifies the exact build/RelWithDebInfo/obs-stabilizer-opencv.dll and the same file extracted from the ZIP. Both must be x64, import obs.dll, and export the six exact OBS module entrypoints: obs_module_load, obs_module_set_pointer, obs_module_ver, obs_module_name, obs_module_description, and obs_module_unload. The parser accepts the actual dumpbin = @ILT+… annotation while matching the exact name column, and rejects similar symbol names.

The Windows package now pins the observed 22-DLL payload in scripts/windows-package-dlls.txt. It checks the build output against that exact set after excluding only the named test/host DLLs, verifies the staged and extracted sets, normalizes either slash style in ZIP entry paths and compares the complete path set, and compares SHA-256 for every packaged DLL against its exact build output. The uploaded windows-package-dlls-report.txt lists each filename and hash. Regression tests reject missing, unexpected, duplicate, and case-changed names. Failed plugin checks remove stale reports and write Status: PASS only after validation succeeds.

In Build OBS Stabilizer / Build on Windows, Configure CMake runs the Test missing OBS SDK contract substep before configure; the job then runs Test Windows ABI gate, Test Windows package DLL allowlist, and Verify and package Windows plugin. CMake also registers obs_dependency_contract with CTest.

The plugin targets the documented OBS 30.0 API minimum. OBS 32.2.x accepts an older module version, but that does not prove imported-symbol or runtime compatibility; see the official 32.2.2 loader version check, 30.0.0 API version, and 32.2.2 API version. The package filename obs-stabilizer-opencv.dll is the loader identifier; the OBS display/filter name is Video Stabilizer.

Evidence and validation

  • The exact old ZIP had SHA-256 b44d34d27ce7cf9f7fb09bd07c3a44b005c6a3f2e41f3eec50d48070e836ed93; the exact DLL member was 802,816 bytes with SHA-256 eafe8a8caa1c7d6858cf865498b468edb7429935aa5437a7c1abdc239c19e071. llvm-objdump -p reported coff-x86-64 and an empty export directory.
  • Local macOS plugin build and CTest: 3/3 passed (stabilizer_unit_tests, obs_dependency_contract, macos_packaging_scripts; 97.39 seconds). cmake -P tests/test_obs_dependency.cmake, PowerShell allowlist fixtures, PowerShell AST parsing, and git diff --check passed.
  • Fresh CI on head d4e10406962113e83d35be4290bde64d26e13cc0 passed all nine workflows: Build OBS Stabilizer, Performance Tests, Quality Assurance, Feature Implementation Flow, macOS Plugin Package, Integration Tests, Memory Sanitizer, Thread Sanitizer, and Preset Persistence. In the Windows job, unit tests, ABI gate, separator/path fixtures, exact package-set verification, and upload passed. The exact build DLL build/RelWithDebInfo/obs-stabilizer-opencv.dll and ZIP-extracted build/windows-package-check/obs-stabilizer/obs-plugins/64bit/obs-stabilizer-opencv.dll both have SHA-256 DEAC33856376E07B47D7D4E31BC956BC3C09C6AB9B8C94BDFC8D628E155E323B; ZIP SHA-256 is 18DC79823EAB74E4A7903FFA7AFBDB2DEBAD6E57DB54C42A2A24EBC91A68526A. Artifact obs-stabilizer-windows-621 (ID 11998702342, digest sha256:e8b074fdc9ee0bdd3a6183513df42ee8c439f934dc6cd90dcc4eaa859db88c46) contains the ZIP and validation reports. Earlier run 38784461639 confirmed the actual ZIP entry paths use backslashes; d4e1040 normalizes separators before exact full-path comparison. Fixtures cover forward, backward, and mixed separators, duplicate normalized paths, and unexpected paths.

Acceptance still open

A real OBS 32.2.x load, filter behavior, and verification of a release/nightly artifact remain outstanding. Static ABI checks do not establish runtime loading. Keep #378 open until the exact distributed DLL is verified in OBS. This PR does not install or restart OBS, operate a stream, merge, or publish a release.

@github-actions

Copy link
Copy Markdown

⚠️ Performance Regression Detected

Performance tests failed on windows. Please review the uploaded artifacts for details.

This may indicate a performance degradation in recent changes.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 16:02:31 UTC 2026

Results by Platform

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.68,11.45,12.79,0.19,901496832,457166848,true,true,""

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 109980c

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 736b08b

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

Independent review — record-level

Verdict: conditional approval. 4 blockers before merge, 3 of them CI/evidence-related.

First, a scope disclaimer so this is not mistaken for more than it is: I have no tool access to the diff, the check runs, or the release artifacts. Everything below is a review of the PR record (body, comments, linked issue state). I have not read a single line of the changed code. A real code-level review still needs to happen — see "What I could not review".

Verified from the record

  • The PR says Refs #378, not Closes #378, and the body explicitly states Can't work with OBS 32.2.1 (64-bit, windows) #378 must stay open until real-load evidence exists. Auto-close on merge is therefore not intended. ⚠️ GitHub also honours closing keywords in commit messages — please confirm none of the commits on this branch contain Closes #378 / Fixes #378.
  • The pre-fix evidence is properly falsifiable: ZIP SHA-256 b44d34d2…, exact member obs-stabilizer\obs-stabilizer-opencv.dll (802816 bytes), DLL SHA-256 eafe8a8c…, and llvm-objdump -p showing Entry 0 0000000000000000 00000000 Export Directory. An empty export directory is exactly consistent with the standalone fallback. Recording hashes and the command makes this checkable later — good.
  • The changed behaviour matches the confirmed root cause: OBS SDK is now required, and only an explicit BUILD_STANDALONE=ON can produce a standalone build. That closes the "silently ships a non-plugin" defect, which was the actual bug.
  • The export gate now covers all six module exports, matching the corrected list.

Blockers

B1 — CI state is unresolved, and one signal contradicts the others. The first bot comment reports “Performance Regression Detected — Performance tests failed on windows”, while the following comment’s performance table contains only a ubuntu row that passes, and later comments assert “All automated checks passed”. Those three cannot all be true. Quoting “7 checks succeeded, 7 running” is not a green state. Nothing merges until the Windows performance job’s actual result is resolved and the full check set is green.

B2 — justify OBS SDK 30.0.0 against a 32.2.x target. The reported users run OBS 32.2.1 / 32.2.2, but the SDK is pinned to 30.0.0. Please state the ABI rationale explicitly: what OBS does with a module whose obs_module_ver() reports an older version, and why building against a ~2-year-old API cannot miss symbols the plugin needs. If the reason is “the 32.x SDK was harder to fetch in CI”, say so — that is a legitimate trade-off, but it is a different statement from “this is the right API to build against”, and it changes how much the runtime test matters.

B3 — a SHA-256-pinned dependency that is then patched locally. The Windows CI downloads OBS 30.0.0 and applies a backported 30.1.0 OBS::w32-pthreads fix to it. That means the tree actually compiled is not the artefact the hash pins. That is acceptable if it is explicit: the patch should live as a versioned file in the repo with its own hash, the pin should be described as “archive SHA-256 + in-repo patch”, and tests/test_obs_dependency.cmake (idempotent single insertion) must be a required check. “Byte-for-byte identical to upstream 30.1.0” is a claim about the patch text — worth asserting mechanically rather than by inspection.

B4 — confirm the naming direction. The body says installation/testing docs “use that filename and layout”. My earlier acceptance criterion asked for the shipped name and the documented name to agree, but not which one wins. Please state the direction: if the target stays obs-stabilizer-opencv.dll and the docs were changed to match, confirm that, and confirm obs_module_name() returns the same user-visible name (historically this module has been called test-stabilizer, per #152).

Acceptance criteria correction (mine, not yours)

My earlier wording said the artifact must contain “exactly one plugin DLL”. That is unachievable and wrong: the package must ship the vcpkg/OpenCV runtime DLLs, which OBS will scan and skip — the reporter’s log shows exactly that pattern for every plugin that ships dependencies. Corrected criterion: exactly one OBS module DLL (with obs_module_* exports); auxiliary runtime DLLs are expected and fine.

Non-blocking observations

  • Building OBS from source in CI is a notable cost and supply-chain surface. If a prebuilt dependency bundle (obs-deps-style) could supply libobs headers + import lib for the same API version, it would be simpler and faster. Not required for this PR — just note why source-build was chosen.
  • Export Directory being all-zero is a useful signal to keep in the packaging gate: asserting “export directory present and contains ≥6 obs_module_*” is stronger than asserting a symbol list alone.

What I could not review

I did not, and cannot from here, review: the actual diff, the new PowerShell/CMake logic, tests/test_obs_dependency.cmake, the three CI scripts, the packaging script, or the check results. For a genuine diff review I need either file access to this repo in my session, or the relevant diff hunks pasted here. Until then, treat my sign-off as covering the record and the acceptance criteria, not the implementation.

Status from my side: #378 stays open, per the agreed rule. Ready to promote this to full approval once B1–B4 are closed and someone has actually read the diff.

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 38b937e

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 16:58:05 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,16.82,9.55,31.64,5.26,909574144,462962688,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.42,11.21,12.10,0.13,901476352,457148416,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,14.17,13.60,21.36,1.03,886800384,444989440,true,true,""

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

Independent review — implementation level (head 7b43eec9)

The pasted diff moves this from a record review to a real one. Verdict: core fix is sound; 4 merge gates remain, 2 of which I cannot close from what was pasted.

The core defect is genuinely fixed

The key behaviour change is in cmake/OBSDependencies.cmake:

  • option(BUILD_STANDALONE ... OFF) — standalone is now opt-in, not a fallback.
  • elseif(APPLE OR (WIN32 AND OBS_SOURCE_DIR)) … else() ends in message(FATAL_ERROR "OBS SDK required for plugin builds …").
  • On Windows, the SDK branch additionally FATAL_ERRORs if the import library is missing.

So the exact chain that shipped the broken nightly — OBS not found → silently build standalone → publish — is now impossible: the "not found" path terminates the configure. That was the actual bug, and it is closed.

Defence in depth is also right: even if someone configured a release with -DBUILD_STANDALONE=ON, scripts/package-windows.ps1 runs the export verification before packaging, so the wrong artefact cannot leave the pipeline. Verification both before packaging and on the DLL re-extracted from the ZIP is the correct shape — it catches the "correct DLL, wrong thing in the archive" failure, which is exactly what happened here.

Other things I'd like to keep:

  • cmake/FetchWindowsOBS.cmake: EXPECTED_HASH SHA256=c23dd463… and TLS_VERIFY ON, plus a post-build assertion that obs.lib exists. Fail-closed at every step.
  • cmake/PatchOBSWindowsSDK.cmake: early return() if OBS::w32-pthreads is already created (idempotent), a FATAL_ERROR if the anchor is absent (so an upstream layout change breaks loudly instead of silently not patching), and bracket-quoted insertion so ${CMAKE_SOURCE_DIR} is expanded by OBS at configure time, not by us at patch time. That last detail is the kind of thing that is easy to get wrong.
  • release.yml: replacing the old copy build\RelWithDebInfo\*.dll package\obs-stabilizer\ with the packaging script fixes a second, pre-existing layout bug — the old archive had no obs-plugins/64bit level at all.
  • libobs-dev added to the Linux dependency step; $LASTEXITCODE checks in the new steps; BUILD_STANDALONE=ON removed from preset-persistence.yml's preset-test build.

B2 — withdrawn, with the reasoning recorded

I asked why the SDK is pinned to OBS 30.0.0 when affected users run 32.2.x. The diff answers it: the same 30.0.0 URL and the same SHA256=c23dd463… were already in the old CMakeLists.txt on the macOS path, with the comment “Build against the documented minimum so the module ABI remains loadable by OBS 30.x as well as newer OBS releases”. This change extends an existing release baseline to Windows rather than introducing a new one, and OBS permits a module built against an older API on a newer host. I withdraw B2. Please still state the obs_module_ver()/API-version semantics in the PR description for the record, since that is the sentence a future maintainer will need.

Merge gates

G1 — CI is not green and one signal still contradicts the others. Windows build and benchmark are outstanding. The earlier “Performance Regression Detected — Performance tests failed on windows” comment is still unexplained while other bot comments assert “All automated checks passed”. One of those is wrong; resolve it explicitly.

G2 — the SDK contract test appears to have been removed from CI. The diff removes it from two places at once:

  • configure-cmake/action.yml: the step - name: Test missing OBS SDK contract / run: cmake -P tests/test_obs_dependency.cmake
  • CMakeLists.txt: add_test(NAME obs_dependency_contract COMMAND ${CMAKE_COMMAND} -P …/tests/test_obs_dependency.cmake)

If nothing else runs it, the fail-closed guarantee — the one property that makes this whole fix meaningful — is no longer continuously verified. Where is tests/test_obs_dependency.cmake executed now? If the answer is "it isn't", this is a blocker: the test that proves missing SDK ⇒ hard failure must be a required check, or nothing stops a future edit from reintroducing the fallback.

G3 — the highest-risk files were not in the paste. I cannot sign off on the verification logic without them:

  • scripts/verify-windows-plugin.ps1 (referenced by package-windows.ps1, absent from the diff)
  • the remainder of scripts/package-windows.ps1 (truncated at foreach ($directory in @($stage, $check)) {)
  • the new Windows regression that "compiles a load-only DLL and proves the gate rejects its exact path for missing obs_module_set_pointer" — this is the test that justifies the six-symbol list, so it matters most
  • the docs/naming diff (see B4 below)

The package-windows.ps1 logic that matters is exactly the part that was cut off: how the DLL is located, how the ZIP is inspected, and whether a missing/renamed artefact fails or silently passes.

G4 — patch provenance and caching. The archive hash pins the download, but the tree that is actually compiled is archive + in-place patch, so the pin no longer describes the input. Please make the result verifiable: hash the patched os-windows.cmake (or assert the inserted block equals the upstream 30.1.0 text) rather than relying on inspection for the "byte-for-byte identical" claim. Related: because extraction is skipped when the source directory already exists and the patch returns early when already applied, a future edit to PatchOBSWindowsSDK.cmake will not take effect against a cached/extracted tree. Either key the cache on the patch hash or force re-extraction when the patch changes.

Questions (non-blocking, please answer in the PR)

  1. preset-persistence.yml lost -DBUILD_STANDALONE=ON. With the new defaults that job now builds against the OBS SDK — does that runner install libobs-dev, and was changing what that job covers intended? It looked like standalone preset coverage.
  2. Same for the Linux release path: confirm every Linux job that configures CMake goes through the step that installs libobs-dev, otherwise the new FATAL_ERROR will fire.
  3. build-project/action.yml is ambiguous in the paste — I read the if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } line as possibly being removed. Please confirm the exit-code check is retained on the Windows build step.
  4. B4 still open: state the directory of travel — the target stays obs-stabilizer-opencv.dll and the docs were changed to match, or the reverse — and confirm obs_module_name() returns that same user-visible name (historically test-stabilizer, per [ARCHITECTURAL DEBT] Multiple plugin entry points create confusion and maintenance burden #152).

Acceptance criteria

Unchanged from my previous comment, including the earlier correction: exactly one OBS module DLL — auxiliary OpenCV runtime DLLs are expected in the package and are fine.

Once G1–G4 are closed I'll raise this to approval. #378 stays open until the real OBS 32.2.x load is confirmed by someone who actually has OBS — neither of us does, so that step needs the reporters.

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: ff0fcc3

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 17:20:48 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,12.54,9.01,22.92,2.82,909672448,463052800,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.39,11.21,15.54,0.44,901484544,457158656,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,17.17,16.67,24.11,0.82,886804480,444989440,true,true,""

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 17:22:50 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,14.81,9.66,26.98,4.21,909508608,462921728,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,6.32,6.09,7.58,0.21,901509120,457236480,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,17.66,17.05,27.63,1.55,885252096,444219392,true,true,""

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 17:32:18 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.83,8.63,26.01,3.21,909590528,462970880,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.81,11.62,12.19,0.09,901435392,457105408,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,17.52,16.83,24.77,1.47,884989952,444092416,true,true,""

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: ca17a60

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 072ac7e

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 17:44:20 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,14.38,9.45,28.53,4.13,909574144,462970880,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,9.28,8.97,12.30,0.38,901472256,457199616,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,16.67,15.07,22.59,1.34,884961280,444073984,true,true,""

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 17:48:22 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,15.38,9.12,23.26,2.93,909459456,462921728,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,6.09,6.01,6.81,0.13,901509120,457238528,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,10.06,9.08,13.44,0.79,885256192,444213248,true,true,""

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

G3 review — scripts/ + tests/test_windows_plugin_gate.ps1 (head 181c99a5)

G3: substantially resolved. The verification logic is well built. Six findings below, none of which alone blocks merge, plus a recurrence of the G2 problem.

What is right (and worth keeping)

  • Exact-path design is enforced, not documented. verify-windows-plugin.ps1 takes a mandatory -PluginPath, asserts the filename, and never searches by name. This is the property that makes the gate meaningful, and test_windows_plugin_gate.ps1 proves it: it builds a deliberate load-only DLL with the correct filename while a complete plugin sits in build/RelWithDebInfo, and requires rejection. That is a real regression test for the actual failure mode, not a smoke test.
  • Case-sensitive symbol matching. -cmatch / -cnotcontains plus the obs_module_load_helper fixture case is what stops the gate from being satisfied by a substring. A case-insensitive or -contains implementation would have passed the incomplete DLL. Good.
  • The archive is verified as shipped. hash(build DLL) == hash(extracted DLL), exactly one obs-stabilizer*.dll in the ZIP, then the extracted DLL is re-verified. Correct shape — the earlier real bug was "correct DLL, wrong thing in the archive".
  • ILT handling with a table-driven regression, including the = @ILT+… annotation and a genuinely-missing case.
  • $ErrorActionPreference = 'Stop' everywhere, throw on missing MSVC/dumpbin, finally cleanup in the fixture test. Fail-closed throughout.

Findings

F1 (medium) — the evidence file is written before validation. In verify-windows-plugin.ps1, Set-Content $ReportPath runs before the x64, export and import checks. A failing verification therefore still leaves a windows-plugin-build.txt that looks like evidence. Write the report after the checks pass, or make its first line an explicit PASS/FAIL status.

F2 (medium) — the parser only tolerates the = … form of trailing text. The regex accepts a trailing annotation only in the shape \s+=\s+.*. If another dumpbin version prints a different annotation, the row will not match at all and the export silently becomes missing — safe (fail-closed) but a maintenance trap that will look like a genuine failure. Loosen the tail: ^\s+\d+\s+\d+\s+[0-9a-fA-F]+\s+(?<name>\S+)(?:\s+.*)?\s*$ captures the name column and tolerates any annotation.

F3 (medium, supply chain) — the package ships by denylist. Get-ChildItem … -Filter '*.dll' | Where-Object { $_.Name -notmatch '^(gtest.*|obs|w32-pthreads)\.dll$' } copies everything that is not explicitly excluded, so any new artefact in $Configuration ships automatically. Prefer an allowlist (the plugin plus explicitly named runtime dependencies), or at minimum assert the exact set of shipped DLL names.

F4 (low) — the filename check uses -cne, so OBS-STABILIZER-OPENCV.DLL would be rejected on case-insensitive Windows. Deterministic and safe; noting it so nobody is surprised.

F5 (low) — Remove-Item -Recurse -Force runs on $stage / $check under $build. Fine as written, but assert both resolve inside $build before the recursive delete so a future default-value change cannot widen the blast radius.

F6 (info) — the verifier now requires MSVC + dumpbin, so it cannot run on macOS. That is the right trade for a Windows artefact, but it means the pre-fix check (llvm-objdump on macOS) and the gate are different tools; worth one sentence in the docs.

G2 — recurrence, and now I think it is a pattern

I still have no answer on where tests/test_obs_dependency.cmake executes. The earlier paste of build.yml also read as if the step - name: Test Windows ABI gate / run: ./tests/test_windows_plugin_gate.ps1 was replaced by ./scripts/package-windows.ps1 rather than run alongside it.

If that reading is right, then both new regression tests — the SDK contract test and the plugin-gate test — exist in the repo and are not executed by CI. That would make every guarantee discussed in this PR unenforced: the fail-closed SDK behaviour and the six-symbol export check would both be free to silently regress, which is precisely the failure class this PR exists to eliminate.

Please answer concretely: list the CI steps, by workflow and step name, that run tests/test_obs_dependency.cmake and tests/test_windows_plugin_gate.ps1 on a pull request. If a step is missing, adding it is a merge gate, not a follow-up.

G4 — unchanged

Patch provenance and cache invalidation are still unaddressed: hash the patched os-windows.cmake (or assert the inserted block equals upstream 30.1.0 text), and make a patch edit force re-extraction rather than being skipped by the already-extracted / already-patched early returns.

Status

G3 is close to done — F1–F3 are worth fixing before merge, F4–F6 are notes. G1, G2 and G4 remain open. #378 stays open. I have still not issued an approval for this PR.

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 8fd36b0

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

G2 withdrawn / G4 needs the code / one new item to diagnose

G2 — 取り下げます。私の誤りでした

Test Windows ABI gate は実行されています。最も強い根拠は失敗そのものです——実行されていないステップは失敗しません。共通 configure-cmake 内の SDK 契約テストも同様です。G2 はクローズします。

誤りの原因も書いておきます。先に貼られた build.yml の差分を、私は文字どおりに読みすぎました。同じ貼り付けの build-project/action.yml については「+/- のマーカーが信用できない」と自分で指摘しておきながら、build.yml にはその注意を適用しませんでした。ゲートとして立てたのは私の判断ミスです。

G4 — claim だけでは閉じられません

「archive+patch の fingerprint 変更時に source/build を再作成する」はまさに求めていた答えです。ただし検証にはコードが必要です。現行の cmake/FetchWindowsOBS.cmake と、キャッシュ関連のワークフロー ステップを貼ってください。私は次の4点を見ます:

  1. fingerprint が archive のハッシュとパッチスクリプトの内容の両方を対象にしているか(できれば bootstrap 自身も)
  2. 不一致時に source と build の両方を作り直すか(再 configure だけで済ませていないか)
  3. 既存の早期 return(if(NOT EXISTS "${root}/obs-studio-30.0.0/CMakeLists.txt") と patch_obs_windows_sdk の if(contents MATCHES …) return)が無効化を打ち消さないか。fingerprint 検査を早期 return より前に置くか、ディレクトリを消す必要があります
  4. 「パッチが変わったらキャッシュが無効化される」ことの回帰があるか(正常系だけでは不十分)

新規 — ABI ゲートの失敗を先に診断してください

ゲートが落ちたのはゲートが仕事をしたという意味ですが、CI は赤です(G1 は未解決のまま)。ジョブログを直接読もうとして失敗しました(GitHub のログ本文は取得ページに含まれません)。Test Windows ABI gate ステップの失敗箇所を貼ってください。 次のどれかで意味が大きく変わります:

  • fixture の DLL が「受理された」 → ゲートが fail-open → ブロッカー(起こりうる中で最悪の結果)
  • fixture のビルドに失敗(cmake/MSVC/ツールチェーン) → ゲートの欠陥ではなく環境問題
  • パーサが正常な行を拒否 → F2 で指摘した末尾正規表現の問題が実害として出た

どれなのか、出力つきで示してください。

F1 / F3

了解しました。妥当な処置です。F1(検証前にレポートを書く)と F3(デナイリスト梱包)は、失敗ログ対応と同じ変更に載せてください。後続タスクに分離しないでほしい。

Status

G2 クローズ。G1 未解決、G4 未検証、加えて ABI ゲートの失敗。 承認は出しません。#378 は open のままです。

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 6540a5e

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 18:00:54 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,10.18,8.59,17.44,1.49,909656064,463020032,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.86,11.32,24.09,1.67,901378048,457105408,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,10.47,9.51,15.96,0.83,884948992,444063744,true,true,""

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 18:05:36 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,13.59,9.08,27.20,4.06,909541376,462954496,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,10.38,8.35,15.95,1.63,901591040,457275392,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,9.51,8.91,14.10,1.01,884989952,444096512,true,true,""

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: daa92a5

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 18:24:40 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,14.80,9.08,85.26,8.28,909524992,462921728,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,9.21,9.05,10.92,0.20,901615616,457273344,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,17.22,16.85,20.14,0.54,884953088,444061696,true,true,""

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: e8a4f93

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@github-actions

Copy link
Copy Markdown

Performance Test Results

Run on: Thu Oct 15 18:33:11 UTC 2026

Results by Platform

macos

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,11.57,8.71,23.10,3.04,909574144,462987264,true,true,""

ubuntu

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,12.14,11.92,14.85,0.36,901402624,457097216,true,true,""

windows

Quick test results:

scenario_name,resolution_width,resolution_height,frame_rate,avg_processing_time_ms,min_processing_time_ms,max_processing_time_ms,std_deviation_ms,peak_memory_bytes,avg_memory_bytes,passed,meets_realtime_requirement,failure_reason
Resolution 1080p,1920,1080,30,17.35,16.64,25.60,1.13,885223424,444186624,true,true,""

@github-actions

Copy link
Copy Markdown

Feature Implementation Report

Implementation Details

  • Feature Name: Feature Implementation
  • Feature Type: general
  • Branch: 379/merge
  • Commit: 22f3be6

Pre-Commit Checks

  • ✅ Code style checks passed
  • ✅ TODO/FIXME comments reviewed
  • ✅ Hardcoded values reviewed
  • ✅ Console output checked

Quality Checks

  • ✅ Static analysis passed
  • ✅ Unit tests passed
  • ✅ Code coverage checked

Security Checks

  • ✅ Security implications reviewed
  • ✅ Deprecated functions checked

Recommendations

  • Review breaking changes if API modified
  • Consider adding integration tests for new features
  • Update documentation for any user-facing changes

Next Steps

  1. Review the implementation report above
  2. Address any warnings or issues
  3. Run manual testing on target platform
  4. Merge to main after approval

@github-actions

Copy link
Copy Markdown

✅ Feature Implementation Flow Complete

All automated checks passed. The PR is ready for review.

Please review the implementation report and checklist above.

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

Review — F1/F3 accepted in principle; F2 unanswered; G4 still blocked

F1 — accepted

「検証成功後だけ PASS レポートを保存」+「失敗時に古い PASS が残らないことの回帰」は、私が求めた以上です(私は書き込み位置の移動だけを指摘しました)。クローズ。

F3 — accepted, and stronger than what I asked

build / stage / ZIP / 展開後の4箇所で集合と各 DLL ハッシュを照合するのは、私が提案したアローリストより強い形です。クローズ。

ただし固定 allowlist は保守上の結合を新たに作ります(vcpkg / OpenCV の更新で DLL が増減・改名されるとパッケージングが落ちる)。方向としては正しい(fail-closed)ので blocker にはしませんが、2点確認したい:

  1. 不一致時のエラーメッセージは問題の DLL 名を出しますか? 出さないと、依存更新のたびに「どれが余分/欠落か」を人手で探すことになります
  2. 比較は順序非依存の集合として、かつ Windows なので大文字小文字を無視して行っていますか? また 22 件にはプラグイン本体が含まれますか(含まないなら 22+1 の関係を明記)

🚧 F2 — 未回答です

パーサの末尾は依然 (?:\s+=\s+.*)? のままでしょうか。= … 形式の注記しか許容しないため、別の dumpbin が別表記を出した行は「一致しない=その export は欠落」と判定されます。危険な方向ではありません(fail-closed なので誤って通すことはない)が、**「書式変更でリリースが止まり、しかも本物の export 欠落と見分けがつかない」**という診断性の問題です。私は (?:\s+.*)? への緩和を提案しました。

  • 変更したなら、その旨
  • 意図的に = のままにするなら、dumpbin が出し得る表記を確認した根拠(どのバージョンで何形式か)

どちらかを示してください。放置は避けたい——「一致しない」と「本当に無い」が同じ結果になるのは、このゲートで最も避けたい曖昧さです。

🚧 G4 — コードがまだ提出されていません

「archive+patch の fingerprint 変更時に source/build を再作成」は答えとして正しいのですが、私は claim では閉じられません。現行の cmake/FetchWindowsOBS.cmake を貼ってください。確認するのは4点です:

  1. fingerprint が archive のハッシュとパッチスクリプトの内容の両方を対象にしているか
  2. 不一致時に source と build の両方を作り直すか
  3. 既存の早期 return(if(NOT EXISTS …CMakeLists.txt) と patch_obs_windows_sdk の if(contents MATCHES …) return)が無効化を打ち消さないか
  4. 「パッチ変更 ⇒ キャッシュ無効化」の回帰があるか

🚧 ABI ゲート失敗の原因 — 一行でいいので教えてください

以前このゲートが落ちました。原因が次のどれだったかで意味が変わります:

  • fixture の DLL が受理された → fail-open(最も重い。潰しておきたい)
  • fixture のビルド失敗 → 環境問題(問題なし)
  • パーサが正常な行を拒否 → F2 が実害化

差分の提出依頼(レビュー品質のため)

F1 は scripts/verify-windows-plugin.ps1、F3 は scripts/package-windows.ps1 の変更ですね。この2ファイルを貼ってください。 現状の私の審査は「説明を読んだ」段階で、コードは読めていません。この2つで差分レビューは完了できます。

G1

「14チェック成功・Windows最終検証待ち」はまだ final green ではありません。F1〜F3 を含む HEADでの run URL とチェック一覧をください。

判定:未承認のまま。 残るのは F2 の回答、G4 のコード、上記2ファイル、原因の一行、そして final run URL です。

@azumag

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

判定:承認(条件付き)— マージ可

G1 / G2 / G3 / G4 の状況

ゲート 状態
G1 CI ✅ d4e10406 / run 38785933113、15チェック成功・mergeable/clean(私はチェック状態を読めないため、run URL を記録として受け取ります)
G2 テストが CI で走るか ✅ クローズ(私の誤読でした)
G3 検証ロジック ✅ F1・F3 クローズ、F2 は下記の理由で受け入れ
G4 パッチ来歴 ✅ 機構は確認。残りはフォローアップ2件

F1 — クローズ

Test-Path で先にレポートを削除し、全チェック通過後にのみ Status: PASS 付きで書く。古い PASS が残る経路も塞がっています。私の指摘以上です。

F3 — クローズ

アローリストを windows-package-dlls.txt に外部化し、build 出力 / stage / ZIP エントリ / 展開後の4箇所で集合一致を要求、さらに全 DLL の SHA256 を build と出荷物で照合。除外は完全一致4件に限定し、そのうえで集合一致を課すので、新しいビルド生成物が広いパターンに隠れて出荷される経路は塞がっています。ZIP の区切り文字正規化も適切。順序非依存・case-sensitive という回答も要件どおりです。

F2 — 「厳格なまま」で受け入れます

私の懸念は「一致しない行」と「本当に欠落」が区別できないことでした。パースを実CIで観測した2形式に限定しつつ、例外に実際の obs_module_* 行を出すことで、書式ドリフトは失敗出力から即座に判別できます。誤って通す方向ではないので、この設計を承認します。

1点だけ:「未知形式は意図的に拒否する」旨をコメントか docs に残してください。 次に触る人が「緩めれば直る」と誤って判断するのを防ぎます。

G4 — 機構は確認しました

コードを読み、私の4点は満たされていると判断します:

  1. fingerprint = archive_hash + patch_sha256 ✅
  2. 不一致時に file(REMOVE_RECURSE "${obs_source_dir}" "${obs_build_dir}" "${stamp_path}") で source と build の両方を削除 ✅
  3. 無効化判定が抽出・パッチ適用より前に走るため、既存の早期 return() が無効化を打ち消さない ✅
  4. patched os-windows.cmake のハッシュをログ出力 ✅(「byte-for-byte 同一」の主張が機械的に記録可能になった)

フォローアップ2件 — マージは止めませんが、issue 化してください:

  • F-1:patched ファイルのハッシュをスタンプに保存し、再利用時に照合する。現状はログ出力のみなので、ツリーが部分的・手で編集された場合を検出できません。スタンプに source_fingerprint と patched_cmake_sha256 の両方を書き、再利用時に一致を assert する
  • F-2:invalidate_obs_windows_sdk_cache の単体テスト(同一 fingerprint で保持/変更で source・build・スタンプを削除/スタンプ欠落で削除)。FetchWindowsOBS.cmake は NOT WIN32 で FATAL_ERROR になるため、この関数を直接呼ぶテストを tests/test_obs_dependency.cmake に追加する形になります

未読の2ファイル

scripts/ObsModuleExports.psm1(F2 の AllowEmptyString 対応)と scripts/WindowsPackageFiles.psm1(集合比較)は貼られていません。この承認は「説明+CI」に基づくもので、この2ファイルはコード未読です。完全な差分レビューにしたいなら貼ってください。ただしマージを止める理由にはしません。

ABI ゲート失敗の説明を受け入れます

必須 string[] 引数が dumpbin の空行を拒否した、という説明は妥当です(fail-open ではないことを確認できました)。空行 fixture の追加も適切です。

❌ #378 は閉じないでください

この PR をマージしても #378 は完了ではありません。OBS 32.2.x 実機でのロード確認が未実施です。マージ後:

  1. ゲート通過の nightly を公開
  2. 報告者(vtnil / TrashJager)に実機確認を依頼
  3. 結果が出たら Can't work with OBS 32.2.1 (64-bit, windows) #378 を close

依頼文は私が #378 に起草します。マージ後に声をかけてください。

azumag commented Oct 15, 2026

Copy link
Copy Markdown
Owner Author

Integration verification for d4e1040: all 15 checks passed, including Windows ABI fixture, exact 22-DLL allowlist and ZIP round-trip validation (run 38785933113). Independent code review found no remaining mandatory defect; DSH review 6199651560 accepts G1–G4.

Parser policy: unknown dumpbin row formats are intentionally rejected (fail closed). Only ordinary name-column rows and observed = ... annotations are accepted. Failure output includes the actual obs_module_* rows for diagnosis; do not replace exact symbol checks with substring matching.

SDK cache tests already cover matching/changed fingerprints; cache-tree tampering detection and broader fingerprint-composition regression remain optional hardening, not evidence of current runtime acceptance. Issue #378 must remain open until a validated release is loaded successfully in OBS 32.2.x. No live OBS acceptance or release publication is claimed.

@azumag
azumag merged commit 98e1ec7 into main Oct 15, 2026
15 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