**Breaking**: from data["estimated_bytes"] to data["offer"]["estimated_bytes"] in maintain check and maintain semantic commands; semantic plan reports what changed, not what was re-typed; - #250
Open
marcociav-exmergo wants to merge 8 commits into
Conversation
…f unchanged definitions in semantic plans
…in commands to handle pending offers
…s and costless scans
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.
Two envelopes that told the truth about everything except themselves
Closes #109. Closes #136.
Both issues are the same defect wearing different clothes: the envelope reported
something that was not what happened, in the one field a reader uses to decide
whether to trust the result. A third instance turned up in the branch #136
touches, and is fixed here too.
#136: a free answer stopped arriving shaped like a bill
maintain checkandmaintain semanticfinish their free axes on every call.Schema, volume, and the reference and definition half of semantic are metadata
reads: they complete, they settle, and nothing about them is pending. Both
commands returned that finished work inside a
needs_confirmationenvelope,because the axes that scan were priced and unconfirmed.
So the command a maintenance session opens with reported its entire triage, 373
findings in one field report, in a response shaped like a pending charge for work
the caller had not asked for and might never want. Two costs followed. Confirming
things that cost nothing is a habit, and the handshake only earns its keep on the
commands where confirming does cost something. And the shape taught the wrong
read: reaching for
data.findingsinside a refusal is the natural move, and doingit is how the stale-baseline line sitting in
warningsgot missed.The split now turns on whether the caller asked for the work, which is mechanical
rather than a judgement call:
confirmation_requestonly returns a request when--confirmwas absent, and every other refusal (over-ceiling, no-ceiling) raises.needs_confirmationmeans dex is waiting on you for work you requested, andnothing you asked for has run.
status: ok,findings final, estimate under
data.offerwith the same breakdown and--confirm --budgethint a refusal carried.data.offer.axesnames what the estimate would add anddata.axes_runwhatfinished. That pair is load-bearing now that the status no longer implies it:
without it, "grain ran and found nothing" and "grain did not run" look identical.
cost.estimatestays unset, so anoknever carries a number that reads asspend. The spend gate itself did not move: the confirmed re-issue is identical and
no scan runs without it.
explore relationships --verifyandexplore map --verifykeepneeds_confirmation, deliberately. Their checkpoint fires mid-command on analready-confirmed run whose budget ran out, so the caller did ask for the probes
and dex genuinely is blocked. The safety spine's
test_api_verify_checkpoint_keeps_what_it_already_paid_forpasses untouched,which is the evidence the line is drawn in the right place.
Breaking: a host reading
data["estimated_bytes"]on these two commands readsdata["offer"]["estimated_bytes"].The adjacent defect, found while reading that branch
The early-return path built its result without
_baseline_warnings, which thesettled path includes. Every reason the baseline may no longer describe the
warehouse (a cache newer than the snapshot, a snapshot pinned from an
already-stale cache) was therefore dropped from precisely the response most
sessions read, and never from the one they read second.
Confirmed in the field, not just in the code. Against the dogfood project, same
session and same baseline:
maintain semanticreported a 327-hour-old baseline,and
maintain checkreported nothing at all. Both paths now build identicalwarnings, because what bounds the settled answer bounds the free one.
#109:
semantic planreports what changed, not what was re-typedcheck_modecompared names against the project and nothing else, so any namealready present read as
updated. The edit unit is a whole file, so extending ashared
semantic_models.ymlmeans restating every definition in it.Measured on the dogfood project before the change: a payload adding two metrics to
a 27-metric file produced a
+16/-0diff and reported 27 objects as updated.The one place a reviewer confirms blast radius was the place it was hidden.
There is now a third class.
updatedmeans the parsed definition actually differsfrom the project's; a definition restated identically in the file that already
holds it is
unchanged. Two deliberate edges: key order and formatting are notchanges, but list order is (a reordered
dimensions:block is a real diff), andidentical content written to a different file is a move whose diff is real, so it
stays
updated. A plan whose every definition is unchanged warns that it changesnothing.
Same payload after:
defined: 2,updated: 0,unchanged: 25.#109's stronger fix: a per-definition edit unit
The whole-file unit is what generated the noise, and retyping twenty-seven
untouched definitions to add two is also how a stray key gets injected into a
metric by hand, which the field report caught by eye and by the parse gate rather
than by design. So
--definitions-filesits beside--edits-fileonsemantic define|update|plan:{"definitions": [ {"kind": "metric", "content": "name: files_per_session\ntype: ratio\n..."} ]}The name is read from the content, so the two cannot disagree.
pathmay beomitted for a definition the project already declares and defaults to the file
that holds it; an explicit path that would relocate an existing definition is
refused, because writing it to a second file duplicates the name and only
dbt parsewould notice.Text splice, not a YAML round trip.
safe_dumpwould reformat the file andstrip the comments a hand-written semantic layer accumulates, producing a larger
diff than the payload it replaces and destroying the adjudication notes in the
dogfood project's own YAML. So each definition is spliced in place and every other
byte survives. A comment at the item's indent is a section banner and stays put; a
comment indented inside the body belongs to the definition and goes with it.
It fails closed. A layout the scanner cannot span safely (flow-style sequence,
anchors or aliases, multiple documents, tab indentation) is refused with
--edits-filenamed as the way in, and the spliced result is re-parsed andcompared against what was sent, so a splice that reads as valid YAML but says
something else cannot reach the plan store.
Classification is scoped to the definitions named. This one is worth flagging: the
unit lowers to whole-file
PlanEdits, so without the scope a spliced file's othertwenty-five definitions would come back as
unchanged, a quieter version of thenoise the whole change exists to remove. Caught by a test, not by review.
It lowers to the edit unit the engine already stores, so the plan format, the
diffs, the conflict hashing, and
transform applyare untouched. Deleting adefinition remains a whole-file edit; the semantic verbs author and do not delete,
and keeping that rule intact beats the symmetry.
Same change as a definitions payload:
defined: 2,unchanged: 0, one edit, a+16/-0diff, comments untouched.Verification
1965 tests pass. New coverage: the three-way classification including the two
edges, the patch unit and its refusals, the flipped billed-handshake tests, the
ok-with-offer and both-set-raises cases at the envelope layer,maintain semantic's offer branch, and a safety-spine assertion that an unconfirmedtwo-phase command returns
ok, issues only dry runs, and keeps the estimate outof
cost.estimate.Dogfooded against
internal-product-analytics(BigQuery,exmergo-viz, 10semantic models and 27 metrics), before and after on the same baseline:
maintain checkneeds_confirmation, 42 findings, no stale-baseline warningok, 42 findings,offer.axes: ["grain"], 891 MB estimate, stale-baseline warning presentsemantic plan(whole file, +2 metrics)updated: 27defined: 2,unchanged: 25semantic plan(no-op restate)updated: 27unchanged, warns it changes nothingsemantic plan(definitions)defined: 2,unchanged: 0,+16/-0The paid path was then exercised for real, once, at exactly the offered estimate:
maintain check --confirm --budget 891289600returnedokwith all four axes(
grain: 5new findings, 47 total), noofferkey, andspend.bytes_billed: 891289600, matching the estimate to the byte. Thestale-baseline warning is present there too, which is the parity the third fix is
about.
Also verified: DuckDB is unaffected (plain
ok, all four axes, no offer, andparadigm: free_local), the unconfirmed BigQuery path issues dry runs only, andexplore semanticagainst the live dbt Cloud Semantic Layer still returnsok(13 rows for
active_usersbymetric_time__month) with its cost-guard warningintact. That last one is a neighbouring-surface regression check, not evidence for
either fix: neither code path touches the hosted backend.
Plan runs left four unapplied plan files in that repo's
.dex/plans/and touched.dex/drift.json; nothing was applied and no project file was written.Not in this PR
transform plan(the model and schema path) has no classification at all.Whether it should is a separate question, not a regression.
reason: internalrather thanconnectionorprerequisite. Noticed when credentials lapsed mid-session; worth its ownissue, since the reason code is what a host branches on to decide whether a
retry could ever help.