fix(ledger): derive transition status from destination, not target - #28
Merged
Conversation
…estination, not target
annotateArtifact read payload["target"] as the new status on transitioned events, but fest emits the artifact kind or action there ("festival", "reset", "blocked") and the real destination status in payload["to"]. After ingest, every transitioned festival rendered status "festival" and every reset task rendered "reset".
Derive the status from "to" (path.Base normalizes dungeon aliases) and stop reading "target" as a status. The existing normalization test was false-green because its fixtures used status-shaped "target" values the producer never emits; those are corrected to real payloads, and a table-driven regression covers festival/dungeon-alias/reset/blocked using the exact fest producer shapes. Follow-up to #27 (CA0002 / D008).
obey-agent
reviewed
Jul 15, 2026
| // Prefer the trailing path segment as a coarse status when no | ||
| // explicit target/status is present. | ||
| n.Status = path.Base(to) | ||
| if subtype == "transitioned" { |
Member
There was a problem hiding this comment.
Correct contract: fest transitions put destination status in to and artifact kind/action in target (festival, reset, blocked). Reading target as status made every festival show as status "festival". Preferring path.Base(to) for transitioned matches producer shapes and dungeon aliases like dungeon/completed → completed.
obey-agent
reviewed
Jul 15, 2026
obey-agent
left a comment
Member
There was a problem hiding this comment.
Verdict: Approve
Overview
Small, precise ledger ingest fix: transition status is now taken from payload to (with path.Base for dungeon aliases), not from target, which names the artifact kind/action. Tests cover festival status change, dungeon alias, task reset, and blocked using producer-shaped payloads.
Key Findings
- No blocking issues. Prior behavior was a real graph corruption bug for fest transitions.
- Workitem normalization fixtures were corrected to realistic from/to/target values as part of the same contract.
What's Done Well
- Clear comment documenting the fest producer contract
- Table-driven regression that would have caught the original mistake
- Minimal blast radius (kinds.go + tests)
Staff Standard
Yes. Ship it.
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.
Problem
Follow-up to #27 (CA0002 / D008), which merged with a ledger-ingestion contract bug.
The ledger's typed
ledgerkit.Eventenvelope is shared by both producer (fest) and consumer (camp-graph) at a pinned version, so it can't drift. The break is in the untypedpayloadmap:annotateArtifactreadpayload["target"]as the artifact's new status ontransitionedevents —But
festputs the artifact kind or action intarget, and the real destination status into:fest/internal/commands/status/atomic.go(festival status change)from,to: <status>,target: "festival"tofest/internal/progress/manager.go(task reset)to: "pending",target: "reset"pendingfest/internal/progress/manager.go(task blocked)from,to: "blocked",target: "blocked"blockedAfter ingest, every transitioned festival rendered
Status="festival"and every reset task renderedStatus="reset". Task-blocked was correct only by coincidence (to == target == "blocked"). Reproduced with the exactatomic.gopayload: anactive → completedfestival transition producedStatus="festival"instead of"completed".The package's own tests didn't catch it:
TestIngest_WorkitemNormalizationused status-shapedtargetvalues ("active"/"completed") the real producer never emits, so it passed for the wrong reason (false green).Change
annotateArtifactnow derives the status of atransitionedevent fromto(path.Basenormalizes dungeon aliases likedungeon/completed), and never readstargetas a status. This is faithful to the original author's own intent — they already usedpath.Base(to)as the status fallback; the fix removes the wrongtargetoverride and makestoauthoritative so the latest transition wins.The
status-payload handling (used bycreated/completedevents) is unchanged.Tests
TestIngest_WorkitemNormalizationfixtures to real producer payloads (status into, kind intarget); the assertion is unchanged and now passes for the right reason.TestIngest_TransitionStatusFromDestinationNotTarget, a table-driven regression using the exactfestproducer shapes: festival status change, festival→dungeon alias, task reset, task blocked. Each asserts the status comes fromto, nottarget.Verification
go build ./...cleango test ./...— all packages passgo vet ./.../just lintcleangofmtcleanNote / possible follow-up
Festival
createdevents emitstatus="created"(via thestatusfield, nottarget), so a just-created, never-transitioned festival recordsStatus="created"rather than its initial directory status. That is a separate, milder question about thestatus-field convention and is intentionally out of scope here; flagging it for a decision.