Repository navigation
fix: sync the vendored review bridge to the upstream that now carries it - #106
Merged
Merged
Conversation
The vendoring check has failed on every push since v0.7.0. `825da1a` added the review lane's session-cost harvest to the vendored copy of `tui_review_bridge.py` — 210 lines of it — without the change ever reaching the repository the file is vendored from, so `sync-bridge.sh` overwrote them and the diff that follows it never came back empty. ADR-0009 says this bridge is not owned here, and the check exists to hold that. Nothing was wrong with the check: the change was in the wrong repository. It now lives upstream, in review-switch#4, tested there at the seam that repository tests it at — the writer belongs to whichever consumer configured `--machine-log`, so what upstream pins is the argv the lane hands over, while the assertions here stay on the machine-log lines this repository's own writer produces. Upstream CI ran it on 3.11 through 3.14. So the pin moves to that merge commit, and the vendored copy is what `sync-bridge.sh` fetches from it. The 210 lines are byte-identical either way: the whole of this diff is the pin, in the script that holds it and in the comment header the script writes. The check has been red for four releases without stopping one, because it runs only on the 3.11 job and fails in four seconds while the three other jobs go green — worth knowing before trusting the tick beside a future release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite kept its own copy of the pinned sha, so moving the pin turned a green suite red at a test that had found nothing: the script and the test disagreed about a constant only one of them owns. A pin is a fact about the script, and a second place to edit on every upgrade is a place that goes stale — silently, because a test asserting a sha nobody fetches proves nothing about the fetch. The four constants are now read out of `sync-bridge.sh`, which leaves the tests saying what they were always for: the sha the script fetches and the sha it writes into the vendored header are the same one, whatever it is. A script that stops declaring one fails the read rather than skipping the assertion. Verified by breaking it on purpose — a script whose header names a different sha than the one it fetches turns the suite red, so the assertion still bites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
main's CI has failed on every push since v0.7.0, at one step:825da1a(in v0.7.0) added the review lane's session-cost harvest — 210 lines — straight into the vendored copy oftui_review_bridge.py. The change never reached the repository the file is vendored from, sosync-bridge.shoverwrote those lines on every run and the diff behind it never came back empty.ADR-0009 says this bridge is vendored, not owned here, and this check is what holds that. Nothing was wrong with the check. The change was in the wrong repository.
It went unnoticed for four releases because the step runs only on the 3.11 job and fails in four seconds, while 3.12, 3.13 and 3.14 run the full suite and go green beside it.
The fix
The 210 lines now live upstream: review-switch#4, merged as
a5fdabaafter its CI passed on 3.11–3.14.Upstream got the tests too, written at the seam that repository tests at: the machine-log writer belongs to whichever consumer configured
--machine-log, so what upstream pins is the argv the lane hands over, while the assertions here stay on the machine-log lines this repository's own writer produces. That difference between the two suites is deliberate and is not what CI compares.So this PR moves the pin to that merge commit and takes the vendored copy from it. The 210 lines are byte-identical either way — the whole of the source diff is the pin, in the script that holds it and in the comment header the script writes.
Also here
tests/test_sync_bridge.pykept its own copy of the pinned sha, so moving the pin turned the suite red at a test that had found nothing. The four constants are now read out ofsync-bridge.sh, which leaves the tests saying what they were always for: the sha the script fetches and the sha it writes into the header are the same one, whatever it is. Verified by breaking it on purpose — a script whose header names a different sha than the one it fetches still turns the suite red.Verification
bash scripts/sync-bridge.sh && git diff --exit-code -- skills/crew/assets/review/scripts/tui_review_bridge.py— clean, which is the CI step itselfpython3 scripts/validate_plugin_tree.py— plugin tree OKpython3 -m unittest discover -s tests— full suite, run below🤖 Generated with Claude Code