Tell the routing dispatcher what the workflow dispatcher already hears - #12
Open
futurebuildai wants to merge 2 commits into
Open
futurebuildai wants to merge 2 commits into
futurebuildai wants to merge 2 commits into
Conversation
internal/depot resolves where a run is rooted and, when the branch rung was
possible but declined, writes a sentence for an operator explaining why and
where the route went instead: "branch "Plano Yard" (...003) has no coordinates
in GableLBM — it has never been geocoded; rooted at the branch this run's
orders ship from ("Dallas Yard" (...001)) instead".
workflow.Plan carried that note to the client. routing.Plan did not — it had no
field for it at all; internal/routing/service.go logged it and dropped it. So a
dispatcher on the /plan surface was told their route had been rooted at a yard
their load does not leave from, and a dispatcher calling POST
/api/v1/routing/plan was not. The warning existed and never reached half the
people it was written for. That asymmetry was the defect.
route_plans is a column-backed table with no depot_source/depot_note column, and
adding one is a migration, so the fix is response-only: both values now ride the
routing.Plan struct out of Plan(), which puts them on the CREATE response at the
moment the dispatcher is looking at a fresh route and can still act on them.
Nothing is written to Postgres.
That leaves a real limitation — a later GET of the same plan carries neither —
and it is documented rather than left to be rediscovered. The load-bearing part
is that an empty note on the read path means "not stored", NOT "no fallback
happened", so nothing may read a read-path blank as an all-clear and nothing may
fill it in by re-resolving (which would answer today's question about an old
plan). Plan.DepotSource, Repository.Get and a new CLAUDE.md section all say so,
and each names what a migration would buy: the warning surviving a reload.
The slog line stays. It is now the durable copy of a response-only field, and a
test pins that it carries the response's sentence word for word.
Tests: the create response carries the note and the right source; a clean run
carries an empty note (a note is always a warning, never happy-path noise); and
a plan re-read through a store that persists only what has a column comes back
with neither field, asserted as the documented behaviour. The store fake models
route_plans' actual column list, because the existing fake hands back the very
pointer it was given and so could not show the loss. Existing routing and
workflow tests are untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VS2YVQehtn78L2Cq49TSj3
None changes behaviour; each was a sentence that would mislead the next reader.
The byte-identity claim was too strong. Routing and workflow produce identical
notes for identical INPUT, both coming from internal/depot — but not for the
same 'run': routing accepts a request branch that workflow.IngestRequest has no
field for, so routing can emit a sentence workflow structurally cannot.
A third surface went undocumented. POST /routing/plan/{id}/approve also returns
a Plan and also carries neither field, for the same reason GET does not — it
reads back from Postgres. Naming only POST and GET implied approve was covered.
TestPlanResponseNoteMatchesTheLoggedNote did not pin the log. It compares
resolveDepot's output to the response and never captures slog, so changing the
log line to print a different variable would not fail it. Renamed to
...MatchesTheResolverOutput and the comment now says what actually keeps them
in step: shared locals in Service.Plan, not this test.
And a cross-reference pointed 'below' at a section above 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
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.
internal/depotproduces an operator-facing sentence when a run's origin falls back — "branch'Plano Yard' has no coordinates in GableLBM … rooted at the branch this run's orders ship from
('Dallas Yard') instead".
workflow.Plancarried that to the client.routing.Planlogged it toslogand dropped it, and had no field for it at all.So the warning existed and never reached the API a dispatcher calls.
What changed
Two response-only fields on
routing.Plan, json tags matchingworkflow.Planexactly:Populated in
Service.Planfrom the same four return values the existingslogline consumes —one call to
resolveDepot, two consumers, so the log and the response cannot drift.No migration.
route_plansis column-backed, so these are populated onPOST /routing/planand absent on
GET /routing/plan/{id}and onPOST /routing/plan/{id}/approve, both of whichread back from Postgres.
That asymmetry is deliberate and documented in three places, each naming what a migration would
buy. The load-bearing part: an empty note on the read path means "provenance was never stored",
not "no fallback happened" — indistinguishable there, so nothing downstream may read it as an
all-clear. Re-resolving on read is explicitly prohibited: it would answer today's question about
yesterday's plan, since the branch may have been geocoded since.
Verification
Confirmed by execution, not by reading:
depot_sourceanddepot_noteare present in the POST body.Repository.Getscans a freshPlanandits SELECT list has no depot columns.
and config, 6/6 matching on source and note.
columnBackedStorefake persists exactlySave's INSERT list and returns exactlyGet'sSELECT list — the existing fake returns the pointer it was handed and would have reported a note
Postgres could never produce.
Gates:
gofmt·go vet·go build·go test -race ./... -count=1— green, including on atrial merge with
main@c4b88a0(the licensing seam).One correction to the framing, from the review
The commit originally said the workflow dispatcher was told and the routing one was not. That is
true at the JSON payload level only. Grepping
depot_note|depot_sourceacrossapp/src/returns zero hits — neither
RoutePlannorWorkflowPlandeclares the fields, so no dispatchersees the sentence on either surface today.
This PR closes an API-payload asymmetry. The human-facing warning still reaches nobody, on
either surface, and that gap is pre-existing rather than routing-specific. Surfacing it in the UI is
a separate change and worth doing — the sentence is written for a person and currently read by none.