fix(parse): classify every spawn candidate as InvokeSpawn (calque#189 follow-up) - #192
Merged
Merged
Conversation
…pty target (calque#189 follow-up) calque#189's SpawnCallSites candidate expansion made a dict-subscript .spawn() call site visible, but invocationKinds' consider() call for the "spawn" case was still keyed on the empty ic.Target -- the candidate callable's own ir.Function.Invoke never became InvokeSpawn, so ResolveSpawnCallables found zero callables and BuildSpawnManifests produced zero shards. The exact failure #189 was filed to prevent, one layer deeper. Found by writing a synthesized end-to-end test (cmd/calque/spawn_dict_dispatch_e2e_test.go) that wires Parse->ResolveSpawnCallables->SpawnCallSitesReport->BuildSpawnManifests together against the real fixture, instead of only asserting on SpawnCallSites in isolation -- confirmed the test fails without this fix and passes with it. The defect lived in the gap between two already-individually-tested layers. Also filed calque#191 (not fixed here): spawn-run only ever binds a spawned callable's FIRST positional arg regardless of its real arity -- a distinct, pre-existing limitation this investigation surfaced but is out of scope for #189's own target-resolution fix.
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.
Summary
This is a follow-up to #190 (calque#189), prompted by a direct question: "why not synthesize a test that exercises what the AI-Almanac script is doing under controlled and understood conditions?" I hadn't — I'd only tested
SpawnCallSitesin isolation. Writing the actual end-to-end pipeline test surfaced a real second bug..spawn()call site visible (SpawnCallSitesnow emits aSpawnCallSite{Target: "bundle", ...}instead of nothing).invocationKinds'consider()call for the"spawn"case was still keyed on the emptyic.Target, not the candidates — so the candidate callable's ownir.Function.Invokenever becameInvokeSpawn.ResolveSpawnCallables(internal/exec/spawnshard.go) only ever finds a callable whose OWNInvokeisInvokeSpawn, so it returned zero callables, andBuildSpawnManifests'byTargetlookup had a call site with nothing to shard against — zero shards, silently. The exact failure .spawn() call site via a dynamic subscript (dict[key].spawn(...)) resolves to an empty target — spawnrun can't drive it #189 exists to prevent, one layer deeper.invocationKindsnow classifies every candidate asInvokeSpawnwhen a spawn call site'sTargetis empty butCandidatesis populated.Why the original PR missed this
The original fix touched two layers (
SpawnCallSitesfor call-site resolution) but I only unit-tested that layer in isolation, plus one subprocess-level test confirming the JSON wire shape. I never assembled the real pipeline (Parse → ResolveSpawnCallables → SpawnCallSitesReport → BuildSpawnManifests) end-to-end — which is exactly whatspawnRunFromScript(cmd/calque/spawnrun.go) actually does. The defect lived in the gap between two already-individually-tested layers, invisible to either one's own unit tests.What this PR adds
internal/parse/parse.go).TestInvocationKindsClassifiesSpawnCandidatesNotEmptyTarget— pure unit test oninvocationKinds.cmd/calque/spawn_dict_dispatch_e2e_test.go'sTestSpawnDictDispatchEndToEndProducesRealShards— a real end-to-end test wiring the full pipeline together against thespawn_dict_dispatch.pyfixture, asserting a real, non-empty shard comes out the other end. Verified this test actually fails without the fix (revertedparse.golocally, confirmed both assertions fail with the exact "zero callables" symptom, then restored the fix).Also found, filed separately (not fixed here)
calque#191:
spawn-runonly ever binds a spawned callable's FIRST positional arg regardless of its real arity (found via the same end-to-end test —bundle'sjob_id/configargs showed onlyjob_idbound in the built shard's payload). This is a distinct, pre-existing limitation inspawnshard.go's own binding protocol, out of scope for #189's target-resolution fix — calque#187's arity guard doesn't coverspawn-run's separate driver at all.Test plan
go build ./... && go vet ./... && gofmt -l . && go test -count=1 ./...— all passgolangci-lint run ./...— 0 issuesruff check .— all checks passedTestSpawnDictDispatchEndToEndProducesRealShardsfails without this fix (manually revertedparse.go, re-ran, confirmed the exact "0 callables / 0 shards" failure), passes with it