Repository navigation
#27 fix: check out skills-vendor/ in the test job - #30
Merged
Merged
Conversation
tests/test_skills_hook.py dereferences the .claude/hooks/ symlink into skills-vendor/, so it needs a populated submodule, not merely a correctly-shaped link. actions/checkout does not fetch submodules by default, so the link dangled on the runner and the test could not pass in CI at all — red on main since 7bf5988, 443 of 444. `true` rather than `recursive`: neither submodule nests, so recursion only adds a foreach pass. Scoped to the test job — ruff never reads skills-vendor/. Both remotes are public HTTPS, so this needs no token and does not touch the wheelhouse WIF auth. Unrelated to the #22 v1.2 hold in both directions: the suspended refresh is a developer-machine SessionStart hook CI never invokes, and checkout resolves the SHA pinned here rather than upstream tip, so the pin the hold froze stays frozen. The assertion message named only the local remedy, which reads as nonsense on a runner; it now names both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregoryfoster
added a commit
that referenced
this pull request
Aug 14, 2026
Two of these were bugs in the wiring rather than the guards, and both were verified by probe before being written down. **Writers are keyed by alias, not by provider (CR #26).** AsyncGcsDriver takes a bucket in its constructor and never sees another, so a driver *is* a bucket and the key has to be whatever selects one. Keyed by provider, two gcs bindings collapsed onto a single dict entry: a command naming `public` reached the driver holding `co-gcs-internal` and wrote outside the root its own binding declared — validate_destination guards the prefix half of T3, and nothing downstream re-checks the bucket because by then it is baked into the driver. The loser was also dropped with its HTTP session open, since shutdown iterates that dict. Invisible today only because no host provisions two aliases. **Provider failures classify by HTTP status (CR #27).** Everything used to become TransientReplicateError, which loop.py exempts from the delivery ceiling — so a 403 on a misprovisioned bucket retried forever and published *no fact at all*, leaving the issuer waiting on a command that could never succeed. Reachable from the wire, too: object_options is an opaque dict, so a bad storage_class is a 400 any bus writer could use to park a command in the PEL indefinitely. Now 4xx closes (400 is the issuer's — invalid_destination; the rest are the host's — provider_disabled) while 5xx, 408, 429 and statusless failures stay open, which T4 makes safe. Read as a number rather than caught as google.api_core classes: that package is an undeclared transitive, ConditionalWriter is provider-agnostic, and 408 has no named class there at all. ValueError is left to propagate — it is the driver's own seam guard, a defect on our side, and the ceiling should see it. **"The first non-terminal fact" was never emitted (CR #28, #34).** Claimed in five places, including the previous commit message and what I reported. A transient outcome returns RETRY and publishes nothing; build_replicate_reporter still stamps terminal=True on every fact. The decision not to close an INDETERMINATE stands; the description of how it reaches the issuer was wrong. Whether that silence should become a terminal=false fact is a contract change, and is not made here. Also: a driver that cannot be built is skipped and logged rather than fatal, so an expired key file no longer stops the fetch path — load_alias_table promises exactly that one line earlier (#29). The shutdown close loop is guarded per writer; unguarded it ran before client.aclose() and one raising driver leaked the Redis client (#30). A blob swept between locate_blob and the open is blob_expired instead of a ceiling-burning handler_error (#33). The write timeout is a setting rather than the SDK's unchosen 120s (#38). Stale prose in replicate.py, the reporter, ARCHITECTURE.md, AGENTS.md and README corrected (#31, #36); read_blob deleted as dead code (#32); the object_options isinstance hoisted out of its comprehension (#35); _TERMINAL_OUTCOMES inlined (#37). Two found while fixing the above. ty caught that ReplicationCompleteEvent .public_url is a required str while GcsCreateResult types it str | None — a success carrying no URL raised ValidationError inside a publisher that re-raises, so the command retried forever; now refused unclassified before the publish. And build_completion_publisher had no test at all: every write-path test substitutes its own complete seam, so the envelope, the topic and the three echoed correlators were exercised nowhere. 683 tests, coverage 99.83%, ruff clean. Not re-run against the live bucket: the GCS interaction is unchanged apart from timeout_seconds, which a unit test asserts, and a verification run leaves an object objectCreator cannot delete (#38). 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
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.
Fixes #27.
tests/test_skills_hook.py::test_the_hook_the_symlink_points_at_actually_existsdereferences the
.claude/hooks/symlink intoskills-vendor/.actions/checkoutdoes not fetch submodules by default, so the link dangled on the runner and the test
could not pass in CI at all —
mainred since7bf5988, 443 of 444.Change
One key on the
testjob's checkout, plus the comment explaining why it is load-bearing.true, notrecursive— neither submodule carries a.gitmodules, so recursiononly adds a
foreachpass to checkout and post-job cleanup.testjob only — ruff never readsskills-vendor/; fetching two submodules torun a linter is cost for nothing.
.gitmodules. No token, no WIF,no wheelhouse impact.
Also widens the assertion message, which named only
.skills/doctor.sh— developer-machineadvice that reads as nonsense on a runner.
Correcting the issue's evidence table
The third row is misattributed. Run 31117568515
(Merge #26) failed at
Set up jobwithFailed to resolve action download info. Error: Service Unavailable— yesterday's Actions outage. It never reachedcheckout, let alonepytest, and its
lintjob was cancelled at 15m rather than failed. Two runs share thiscause, not three;
afb1f97would have failed the same way, but that was never observed.On the #22 v1.2 hold
Unaffected in both directions. The suspended refresh is a developer-machine SessionStart
hook CI never invokes, and
checkoutresolves the SHA pinned in this superproject ratherthan upstream tip — so the pin the hold froze stays frozen.
Verification
444 passed, 18 deselected(was 443 + 1 failed in CI).testgreen with 444, not 443 — the second assertion isos.access(HOOK, os.X_OK), andthe target is mode
100755in the skills index.lintuntouched and still green.baseline.
Coupling accepted knowingly
The
testjob now depends on two third-party public repos being fetchable at checkout.An upstream force-push that GCs a pinned SHA would fail the job at checkout — an error
that looks nothing like a test failure. Noted in the workflow comment so the next person
reading a checkout failure knows where to look.
🤖 Generated with Claude Code