Repository navigation
SIM-25: one owner for the Unreal-vs-ROS2 patch routing rule - #61
Merged
Merged
Conversation
patches/cosys-airsim/ holds deviations for BOTH halves of Cosys-AirSim, so every consumer had to decide which half a patch belongs to -- and that decision lived in three scripts: build_airsim_wrapper.sh skipped Unreal-side patches, convert_world.sh applied them to a world's injected plugin, and build_blocks.sh carried a copy-paste of the same loop differing only in one remedy sentence. Misrouting is not cosmetic. Miss 0005 and the vehicle falls through a World Partition world forever; miss 0006 and the renderer dies mid-flight. All three copies already carried a `die` for the drift case, which is the tell: the risk was understood and the cause was not addressed. The repo has paid for this three times. collision_witness.py exists because "an absent witness is UNKNOWN" was written twice and the two copies disagreed within one commit, one scoring a run PASS that the other failed. And on 2026-08-17 I described patches/cosys-airsim/0007 as "applied to nothing" three times, having checked one script -- all three globbed that directory, 0007 matched every one, and the next world conversion would have silently shipped a physics gate that kills bring-up. scripts/vendor_patches.sh now owns it: vp_is_unreal_side (the predicate, matching the DIFF HEADER rather than the prose, because 0005 mentions the plugin path four times but only twice in a +++ b/ line), vp_apply_unreal (apply/already/die, with the remedy sentence as a parameter) and vp_list, so no caller re-globs -- a second glob is a second copy of "which patches exist". The non-recursive glob that keeps experimental/ parked now has a test: making it recursive would ship 0004 and 0007. VERIFIED BY BUILDING. Both paths exercised -- `already` when the patches are present, `applied` after reversing them out -- and the resulting libUnrealEditor-AirSim.so is md5 20f5430c1a61, BYTE-IDENTICAL to the pre-refactor build from two days earlier. Same inputs, same artifact. The build caught what review would have had to reason about: $applied became unbound after the extraction, and convert_world.sh used it in two more places than the one I had seen, so the build died after patching under set -u. Counts are published as VP_APPLIED/VP_ALREADY rather than squeezed through a return status, which is an exit code and would be the next person's mistake. Two of the new tests were also wrong, in the same way as one written earlier today: one matched the glob inside a comment, the other rejected "-r " and hit `read -r`. Substring checks passing for the wrong reason. Both now check code, not text. 183 tests, up from 177.
…next door Seven findings. Two matter. build_airsim_wrapper.sh adopted the predicate but KEPT ITS OWN ENUMERATION, globbing the directory itself with PATCHDIR as a second spelling of the path. So this branch's own stated invariant -- no caller re-globs, because a second glob is a second copy of "which patches exist" -- was violated by the branch, and the guard test deliberately checked only the other two callers. It uses vp_list now. And convert_world.sh decided "no build needed" from SOURCE state alone: TIER = A1 && VP_APPLIED -eq 0. Run it once with --no-build (0005 applies), then run again normally -- every patch reverse-applies, so VP_APPLIED is 0, the build is skipped, and the world ships a .so WITHOUT 0005. The vehicle then falls through a World Partition world forever, which is the exact failure that patch exists to prevent. build_blocks.sh had already learned this and carried the earned version with a comment explaining why. That is this ticket's thesis arriving from the other direction: the duplication was not just the routing rule, it was every decision these three scripts share, and the second copy was already wrong. So the rebuild rule is now the second thing with one owner, vp_needs_rebuild, and neither caller keeps its own artifact check. The real apply was unchecked: patch --forward --batch --silent had its exit status ignored, and --dry-run writes nothing, so it passes on a read-only or root-owned tree where the write then fails -- reporting "applied", incrementing the count, and letting a caller compile unpatched source. Survivable inline in three set -euo pipefail scripts; not survivable in a library whose fallbacks exist to support being sourced anywhere. VP_APPLIED/VP_ALREADY were assigned only on the success path, so both early exits left the previous values in place -- a dead contract of the kind two guards on the previous branch turned out to be. Reset on entry. Documentation that had become load-bearing and wrong: experimental/README.md, docs/vendor/cosys-airsim.md and docs/todo.md all cited build_blocks.sh:65 and convert_world.sh:154 as the globs, which is the record of WHY 0007 is parked. Neither globs now, so a maintainer re-verifying "is 0007 really out of reach?" would grep the cited line, find nothing, and reasonably conclude the rationale was stale. All three point at scripts/vendor_patches.sh:47. The same citations in the 2026-08-16 worklog are left alone: worklogs are frozen, and that one correctly records what was true when it was written. Two more tests checked text rather than structure -- a substring scan a fourth copy spelled "^+++ b/Unreal" would pass, and an exact "== 2" that a third legitimate use would break for no reason. Both assert the invariant now. Rebuilt through the reworked owner: both paths exercised, and the artifact is still md5 20f5430c1a61. 186 tests, up from 183.
…itation fix drifted the same way Ten findings. The review exercised vendor_patches.sh directly -- apply, already, drift, read-only tree, missing plugin root, and all four rebuild branches -- and found no correctness bug in the extraction. These are the gaps around it. test_no_caller_re_globs_the_patch_directory checked convert_world.sh and build_blocks.sh only. The previous pass converted build_airsim_wrapper.sh to vp_list precisely BECAUSE it was the caller that re-globbed, and it was the one caller the guard did not cover -- re-introducing its glob passed the whole suite. It is covered now, including the $PATCHDIR spelling. The citation fix drifted the same way it was fixing. Three docs record WHY 0007 is parked; the previous pass replaced three stale references with three references to `scripts/vendor_patches.sh:47` -- a hard-coded line in a 147-line file whose header is 35 lines of prose, so the next edit invalidates all three at once. They name vp_list now, and a test forbids the line-number form. The worklog carries the correction rather than an edit. vp_needs_rebuild treated unknown as fine: `find` on a missing or unreadable Source/ returns the same empty string as "nothing newer than the binary", so it answered "the binary is current". Both callers happen to guard upstream, but the guard belongs to the rule. And its warning opened "every patch is already applied", which is a claim about state it never checked -- true by construction when the text was inline in build_blocks.sh, false once convert_world.sh could reach it with no Unreal-side patches at all. PATCHDIR survived in build_airsim_wrapper.sh as a second spelling of the directory vp_list now owns, used in two die messages -- so if the directory moved, those errors would name a path that was not searched. The owner publishes VP_PATCHDIR. The worklog render summarised "177 -> 183 tests" while its own body said 186, and the index card carried the same stale number. Both now say 187. And the render had NO diagram, while every other worklog render has one -- the house-style figure CSS was present with nothing using it, so it had been dropped rather than omitted deliberately. Added: the three-copies-to-one-owner shape, with convert_world.sh's already-wrong rebuild rule called out, since that is the point. Rebuilt through the reworked owner once more: libUnrealEditor-AirSim.so is still md5 20f5430c1a61. 187 tests, up from 186.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The rule deciding whether a patch under
patches/cosys-airsim/belongs to the Unreal plugin or the ROS 2 wrapper lived in three scripts. All three already carried adiefor the drift case — which is the tell: the risk was understood and the cause was not addressed.Miss
0005and the vehicle falls through a World Partition world forever; miss0006and the renderer dies mid-flight.The repo has paid for this three times
collision_witness.pyexists because "an absent witness is UNKNOWN" was written twice, and the copies disagreed within one commit — one scoring a run PASS that the other failed.0007as "applied to nothing" three times, having checked one script. All three globbed that directory,0007matched every one, and the next world conversion would have silently shipped a physics gate that kills bring-up.One owner
scripts/vendor_patches.sh—vp_is_unreal_side(the predicate, matching the diff header not the prose:0005names the plugin path four times but only twice in a+++ b/line),vp_apply_unreal(apply/already/die, remedy sentence as a parameter — the only real difference between the copies), andvp_listso no caller re-globs.The non-recursive glob is load-bearing and now tested:
experimental/is how a patch is parked, and0004/0007live there because the glob doesn't reach them.Verified by building
Both paths exercised —
alreadywhen present,appliedafter reversing them out. The resultinglibUnrealEditor-AirSim.sois md520f5430c1a61: byte-identical to the pre-refactor build. Same inputs, same artifact.Building caught what review would have had to reason about:
$appliedwent unbound after the extraction, andconvert_world.shused it in two more places than the one I'd seen — the build died after patching underset -u. Counts are nowVP_APPLIED/VP_ALREADY, deliberately not returned as an exit status.Two of my new tests were also wrong in the same way as one written earlier today — one matched a comment, the other hit
read -r. Both now check code, not text.183 tests, up from 177.