skills: add malloy-model-as-you-go - #1079
Conversation
2b02ca3 to
bbb766f
Compare
jswir
changed the title
kylenesbit
left a comment
kylenesbit
left a comment
There was a problem hiding this comment.
Approving — the skill is strong and the mechanics check out. Three shared-skill governance items to address, the first before merge if possible.
What I verified
- The writable-surface ladder (model file → notebook
extend→ stated assumption + snippet) keyed off tools present, not guessed roles, is the right design, and confirm-before-codify with both numbers attached is the right control — "reporting it afterwards is not confirming it" is exactly the distinction. - Symlink target correct;
skills_bundle.jsonregenerates byte-identically from this tree (57 entries, new skill present); DCO signed; all checks green; bare tool names with the host note; and the explicit departures fromskill:malloy-modelare declared in both skills, which is how to disagree with a sibling skill without creating a contradiction.
Items
-
skill:malloy-dashboardsbreaks the shared-skill reference rule.skills/README.mdon this branch says a shared skill may onlyskill:-reference other shared skills, and classifiesmalloy-dashboardsas Publisher-specific ("dashboards are a Publisher surface"). The new skill references it in "Save the view when a question is worth re-asking" (the PR body lists it among shared references, which contradicts the README). The README's own escape hatch fits: neutral prose — "views wanted side by side belong in a notebook, or in a dashboard surface if the host has one" — keeping onlyskill:malloy-notebooks. One-line fix; would be good before merge. -
malloy-modelis a shared skill edited downstream-first. The README rule is upstream-first (ms2data/agent-skills), then copy, or the next sync conflicts. If the agreed direction for the new skill is publisher-first, fine — but the hunk in the existing sharedmalloy-modelis squarely what the rule exists for. Please mirror it (and the new skill, if it is to be shared) upstream at or before merge, and note it here. The new skill also needs adding to the README's shared-skills list. -
The narrowing doesn't reach where hosts actually route from.
malloy-analyzeis narrowed to "open-ended exploration" in the two routers, but its own front-matterdescriptionstill says "Explore data for insights and build views/dashboards/notebooks… or any analysis task" — and that description is what Claude Code matches for auto-invocation. A first-message data question will keep landing onmalloy-analyzedirectly, bypassing the routers. Either narrow that description too (again upstream-first), or state in the README/PR that description-routing hosts won't see the new behavior until themalloy-analyzeretirement lands.
Notes, not blockers
- Branch is behind
main; needs a branch update before merge (protection is strict; no conflicts). skills-npm.yml's PR job packs and tests but doesn't compare versions against npm — moot here since0.1.9> published0.1.7, so no bump needed.- On the honest flag that the confirm checkpoints and the notebook/chat rungs weren't exercised on a fresh dataset: given they're the heart of the skill, one real run on the middle rung would be worth doing before this ships broadly — not holding the PR for it.
|
Thanks — all three addressed, plus the branch update. 1.
You were right that the PR body contradicted the README; the body was wrong, not the classification. 2. Direction of the I've added 3. The narrowing not reaching description-routing hosts. This was the most useful catch — the routers were doing nothing for auto-invocation.
Notes. Rebased onto current On the unexercised middle rung: agreed, and it stays flagged in the PR body. A real viewer-session run is the check I'd want before this ships broadly, and it's the one claim in the skill I haven't been able to back with evidence. |
5776a46 to
a09914a
Compare
An analysis that lives in a chat transcript is not reproducible. Six weeks on nobody can say what revenue excluded, which of four timestamps was the order date, or whether the last period was complete, so the work gets redone. This skill codifies the analysis into a semantic model as it runs, rather than proposing one from the schema up front. Answer a question, write down what the answer assumed, answer the next one; every field ends up traceable to the question that needed it. The spine is naming the judgment calls the data cannot settle, and confirming one before it is codified - a decision stops being the analyst's the moment it becomes a measure everyone inherits. The codify rubric leads with 'a reader needs this to trust the number' rather than reuse. Shared skill, copied verbatim from ms2data/agent-skills (upstream first, per skills/README.md). No platform-specific tools or answers; all eight skill: references point at other shared skills. Includes the .claude/skills symlink and the regenerated MCP bundle. No packages/skills version bump: main is 0.1.9, npm is 0.1.7, and skills-npm.yml asks whether the declared version is ahead of npm rather than whether this PR changed it. Signed-off-by: James Swirhun <james@credibledata.com>
Step 1 read as though the skill's job was generating questions: a one-line 'if the user brought a question, use it' followed by three paragraphs on the branch where they didn't. The description matched, keying on 'analyzing data that has no model yet' rather than on someone asking something. The skill is what an agent reaches for when a user arrives WITH a data question. Step 1 now says so: restate what was asked, decide what a right answer looks like, and do not widen it into a modelling project or swap it for a more interesting question found on the way. Ambiguity is resolved by taking the most reasonable reading and stating it rather than stalling. Proposing questions survives as a fallback for a genuinely empty start, in one paragraph, marked as not the normal path. Copied verbatim from ms2data/agent-skills; bundle regenerated. Signed-off-by: James Swirhun <james@credibledata.com>
Four changes from running the skill end to end on an unmodelled dataset. A broad ask is an ask. The empty-start fallback told the agent to profile and offer three questions, two paragraphs after "the user gets a real answer on question one" and next to an anti-pattern against opening with a menu. For "analyse the political ads data" those point opposite ways, and the agent had to referee. Say plainly: pick the obvious question, name it, answer it. Working out what is analysable does not need a skill. Confirm binding decisions, do not report them. The rule existed but was scoped to a measure encoding an ANSWER decision, and "report it afterwards" read as compliance. In the run, `spend_estimate` was defined as a bucket midpoint and the user was told after the fact. Worse, `where: regions = 'US'` went onto the source - scoping every future question against it - and was also only mentioned in the write-up. Widen the rule to dimensions, views, joins and source-level filters, and require stopping for an answer. Every number comes out of a query. The run reported a campaign-level rollup (~$192M vs ~$136M) added up by hand from a `limit: 15` table, which drops every committee below the cut and cannot be re-run. There is a query language and an execute tool; nothing needs estimating by eye. Do not encode tool bugs as data properties. POINT said a schema tool "may return no columns - do not debug that". That taught the agent to route around a bug silently. `select: *` is the better first move on its own merits, because sample values catch what a column list hides. An unexplainable schema result is a bug to report, not a fact about the data. Also name the query tool per host, since Publisher and Credible differ and draft-vs-published is easy to confuse; and fix a house-style em-dash that was failing the bundle's own spec on this branch. Signed-off-by: James Swirhun <james@credibledata.com>
…ocs in the loop Three things a session following this skill got wrong, each traceable to the text rather than to the agent. Docs were listed as step 1 of the hand-over checklist, under 'Do not do this during the loop'. Followed correctly, that produces a model with no #(doc) at all - the reasoning reaches nobody, because the hand-over often never comes. Docs now happen in CODIFY, one line while the query is still in front of you, and hand-over is a re-read for drift. Adds the #(doc)-versus-comment rule: #(doc) for what a consumer needs, // for what only a maintainer needs. Saving a view was one line near the end, after a codify table listing only dimensions, measures, joins, and filters, so it read as an afterthought and got skipped. It is now part of the sweep, and dashboards are named alongside views. malloy-model's 'Views: None' said the opposite with no exception, so this scopes that rule to schema-first and gives the reason: a schema-first model is written before anyone asks a question, so its views would be guesses. Access modifiers and one-file-per-table are now explicitly out of scope for analysis-first, in both skills. They solve a schema-first problem, and an analysis-first model has nothing to curate - every field in it was paid for by a question. Signed-off-by: James Swirhun <james@credibledata.com>
The bundle embeds each skill's full body, so the served copy was stale after the previous commit's edits to malloy-reproducible-analysis and malloy-model. Signed-off-by: James Swirhun <james@credibledata.com>
No guidance removed, only restatement. The intro said 'a model is the fix' twice; 'leave behind the ad-hoc' and 'codify thin' were one point split in two; the #(doc) rule ran as two paragraphs with an overlapping middle; and the views section restated the binding-decision rule that the confirm table above it already carries. Two anti-patterns collapsed into their neighbours for the same reason. skills/malloy-reproducible-analysis/SKILL.md 370 -> 344 lines, and the malloy-model addition from the previous commit drops from 17 lines to 11. Signed-off-by: James Swirhun <james@credibledata.com>
The skill re-explained how to answer a question (restate it, discover, construct, verify, present) before getting to what it was for: writing the answer's assumptions down. That duplicated malloy-analysis and malloy-analysis-pitfalls, and hid the new part. Now: answer with malloy-analysis, then CODIFY. What changes is where the assumption lands, and that depends on what the session can write, not on who the user is. Three rungs: a #(doc)'d field in the model when the files are editable; a source extend plus a markdown note in the notebook when only reports can be authored; the assumption stated in the answer plus a Malloy snippet when it is chat only. Pick the highest rung the tools allow and never skip codifying because the model is read-only. Kept from the previous shape: the one-line source bootstrap when no model exists, naming the decisions the answer rests on, confirm before codifying anything binding, the codify rubric, #(doc) versus //, the notes file, saving the view, the hand-over list. 344 -> 289 lines. Routing: malloy-modeling and the malloy index now send a first-message data question to this skill; malloy-analyze keeps open-ended exploration with no intent to keep anything. Bundle regenerated. Signed-off-by: James Swirhun <james@credibledata.com>
Three items from Kyle's review. 1. The save-the-view paragraph referenced skill:malloy-dashboards, which skills/README.md classifies as Publisher-specific, and a shared skill may only skill:-reference other shared skills. Uses the README's own escape hatch instead: neutral prose for the dashboard surface, keeping only skill:malloy-notebooks. 2. malloy-model-as-you-go added to the README's shared-skills list. The README's upstream-first rule is recorded as being reversed by ms2data/service#6177, under which this repo becomes the source of truth and agent-skills vendors from a Publisher tag with a drift check; this skill and the malloy-model hunk were authored here on that basis, and need mirroring by hand until the vendoring lands. 3. Narrowing malloy-analyze in the two routers did not reach the place hosts actually route from. Its own frontmatter description still said "any analysis task", and that is what a description-matching host auto-invokes on, so a first-message data question kept landing there and bypassing the routers. The description now scopes it to open-ended exploration and view-building, and names where a specific question goes instead. Bundle regenerated. Signed-off-by: James Swirhun <james@credibledata.com>
a09914a to
fae1f2d
Compare
malloy-model and malloy-notebooks already mark #(filter) legacy and document given: as the replacement. Four consumer-facing skills were never updated and still present #(filter) as the current mechanism - malloy-define even tells the agent to write new #(filter) annotations into a model it is authoring. Each now points at given: and names the two exceptions malloy-model already documents, required and implicit, so the guidance does not overshoot into cases given: cannot cover yet. Signed-off-by: James Swirhun <james@credibledata.com>
|
Folded #1081 into this PR (its commit, cherry-picked unchanged) and re-signed the history. Why here: #1081 was unreviewed and is 14 lines of the same kind of content this PR carries — pointing consumer skills at @kylenesbit this is new content since your approval, so it wants a fresh look — sorry for the moving target. It's one commit ( Also fixed: DCO went red after the rebase. The original remediation commit named a SHA that the rebase had rewritten, so it no longer covered anything. Signed every commit properly and dropped the remediation commit instead of stacking a second one; the tree is byte-identical, only the history changed. Bundle regenerated (57 entries, 32 skills). |
0.1.9 is now on npm, so publishing the declared version would skip this package: the release stays green while shipping none of the skills content this PR adds. The release guard catches exactly that. Signed-off-by: James Swirhun <james@credibledata.com>
The publish job rewrites the scaffolder's `@malloy-publisher/skills: workspace:*` to `^<skills version>`, so bumping skills to 0.1.10 changes what this package ships even though no skills file enters its tarball. 0.0.8 is already on npm, so without this the scaffolder would be skipped at release and keep depending on ^0.1.9. Signed-off-by: James Swirhun <james@credibledata.com>
Two conflicts, plus one the merge could not see. RELEASE_NOTES.md: the 0.2.0 stamp (#1090) renamed the colocated-persist `[Unreleased]` heading this branch's new section sits above. Kept both -- the new section stays `[Unreleased]`, the stamped heading stays 0.2.0, per the file's own rule that a version-stamped heading is history. skills_bundle.json: regenerated from the merged `skills/` tree rather than merged by hand, per skills/README.md. Carries main's new malloy-model-as-you-go alongside this branch's `from()` corrections; 57 entries, drift specs pass. The one tsc caught: #1079 made `modelPath` a required argument to `filterPublisherOwnedRenderLogs`, deliberately, so a new call site cannot widen the suppression back to every query. The load-time call site this branch adds is exactly such a new call site, and it merged cleanly while failing to compile. It now passes the model's own path. Version bumps for @malloy-publisher/skills and create-malloy-package are no longer this branch's: 0.2.0 landed the same 0.1.10 and 0.0.9 independently. Signed-off-by: James Swirhun <james@credibledata.com>
2 participants
What this does
Adds a skill for the step after a data question is answered: writing down what the answer assumed, in the most durable place the session can write. Today an agent either builds a full model before answering anything, or answers ad hoc and formalizes later if someone remembers to. Both lose the reasoning behind each definition, because nobody wrote it down when the decision was actually made.
malloy-analysisanswers the question. This skill runs right after, while the query is still in front of the agent, and decides what to codify and where.Where the assumption lands depends on what the session can write
The same judgment call ("returns don't count as revenue") lands differently depending on the tools available, and the skill says so explicitly instead of assuming everyone can edit the model:
measure:/dimension:/view:with a#(doc)in the.malloy, after confirming binding decisionssource: orders_q is orders extend { ... }with the#(doc)in a notebook cell, plus a markdown cell stating the assumptionPick the highest rung the tools allow; never skip codifying because the model is read-only.
What reviewers should look closely at
The skill asks the user to confirm a judgment call before it goes into the model, not after. While a decision lives in one query it's the analyst's own call. The moment it becomes a
measure:, it's the definition everyone downstream inherits, and they never see the reasoning. So the skill stops and asks first, with both numbers shown, instead of picking one and mentioning it in the write-up. It only asks when the alternative would change the number, when the call is about to become permanent, or when a different analyst could reasonably pick the other option. Everything else gets a one-line note.Routing
malloy-modelingand themalloyindex now send a first-message data question (and "build a model but not sure what metrics") to this skill.malloy-analyzekeeps open-ended exploration with no intent to keep anything. Retiringmalloy-analyzeand folding its EDA loop intomalloy-analysisis a separate change (proposed in ms2data/service#6177); this PR goes ahead of it.Shared-skill compliance
Shared skill, so it lives here first and gets vendored downstream. No
credible-prefix; no platform-specific tools or answers (noexecute_query_draft, no hosted draft/publish flow, no#(index)/#(agent-hidden)). Allskill:references point at shared skills:malloy-analysis,malloy-analysis-pitfalls,malloy-charts,malloy-dashboards,malloy-notebooks,malloy-model,malloy-lookml-review,malloy-analyze. Tools are named bare, with the host note.Packaging
.claude/skills/malloy-model-as-you-gosymlink so Claude Code discovers it.skills_bundle.jsonregenerated: 57 entries, 32 skills plus 25 reference files.packages/skillsbumped to0.1.10.0.1.9is already published on npm, so leaving the version alone would make a release skip this package and stay green while shipping none of these skills. The release guard catches that.Testing
bun test src/mcp/skillsinpackages/server(18 pass: bundle in sync, no em dashes) andbun run --cwd packages/skills test(15 pass) on this branch.The earlier shape of this skill was run end to end against a real BigQuery dataset: three questions produced a five-field model, re-verified by running the codified measures back through a served package and matching the original numbers to the cent. Not yet exercised on a fresh dataset: the confirm-before-codify checkpoints, and the notebook and chat-only rungs of the ladder. Those were written from the four user situations (local developer, Credible viewer, Credible modeler, chat-only), not from a run. Worth a reviewer's eye.
History
Earlier commits on this branch carry the previous name (
malloy-reproducible-analysis) and shape; the last commit does the rename and the reshape.