Skip to content

fix(sl-toolkit): edits use PATCH, and sql_dialect is snake_case - #94

Merged
cjayyy merged 1 commit into
mainfrom
vn/sl-toolkit-patch-and-sql-dialect
Aug 3, 2026
Merged

fix(sl-toolkit): edits use PATCH, and sql_dialect is snake_case#94
cjayyy merged 1 commit into
mainfrom
vn/sl-toolkit-patch-and-sql-dialect

Conversation

@cjayyy

@cjayyy cjayyy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Two bugs in the semantic-layer skill, both verified against a live metastore
(canary-orion, project 431). Either one alone is enough to make the skill fail or do damage.

Found while porting this skill into Kai's in-platform tooling
(AI-3661) —
rather than working around them downstream, fixing them here.

1. sqlDialect is rejected — /sl-build cannot create a model today

The API requires snake_case sql_dialect. camelCase gives:

422  missing property 'sql_dialect'

The skill's POST envelope, sl-build's model-create call, and the test fixture all used
sqlDialect, so the greenfield wizard fails at its very first write.

Both CI guards were asserting the wrong direction. test_sqldialect_is_camelcase and
test_no_dialect_drift required camelCase and forbade the correct key, and
schemas/semantic-model.json did the same in its not.anyOf. That's why the bug shipped and
stayed green. Inverted them, and the consistency check now matches the quoted payload key
instead of the bare word, so the gotchas section can still name sqlDialect in prose to say
it's rejected.

Also corrected the schema enum — only Snowflake and BigQuery are accepted. Redshift and
Postgres were listed; both return 422 value must be one of 'Snowflake', 'BigQuery'.

2. PATCH exists — the documented DELETE + POST edit path is destructive

The skill stated "The metastore has no PATCH — editing is DELETE old + POST updated" and built
rollback machinery around it. PATCH /api/v1/repository/{type}/{id} works:

before after
uuid 019fc788-d6cb-7582-95d5-46d96f1bad9f same
revision 1 2

Verified on both a semantic-metric and a semantic-glossary object, so it isn't type-specific.

Delete-then-post instead mints a new UUID, resets revision history, breaks anything
referencing the old UUID, and leaves the layer missing an object if the POST fails.

Rewrote the edit section around a new api_patch helper — send only the fields that change. The
rename cascade to constraint metrics[] is preserved and is now also in-place. The rollback block
is deleted: nothing is removed, so there is nothing to roll back.

Testing

All 30 tests pass. I also re-introduced the sqlDialect bug in the fixture and confirmed the
corrected guards now catch it (2 failures) rather than demanding it.

30 passed in 0.06s

Release Notes

Fixes the semantic-layer skill's model-create payload (sql_dialect, not sqlDialect), which
was rejected by the metastore with a 422, and replaces the destructive DELETE + POST edit
recipe with in-place PATCH so object UUIDs and revision history survive an edit.

Two bugs, both verified against a live metastore (canary-orion). Either one is enough to
make the skill fail or do damage.

**1. `sqlDialect` is rejected — `/sl-build` cannot create a model today.**
The API requires snake_case `sql_dialect`; camelCase returns
`422 missing property 'sql_dialect'`. The skill's POST envelope, sl-build's model-create
call, and the test fixture all used `sqlDialect`, so the greenfield wizard fails at its
first write.

Both CI guards were asserting the wrong direction — `test_sqldialect_is_camelcase` and
`test_no_dialect_drift` required camelCase and *forbade* the correct key, and
`schemas/semantic-model.json` did the same in its `not.anyOf`. That is why the bug shipped
and stayed. Assertions inverted, and the consistency check now matches the quoted payload
key rather than the bare word so the gotchas section can still name `sqlDialect` in prose.

Also corrected the schema enum: only `Snowflake` and `BigQuery` are accepted. `Redshift`
and `Postgres` were listed but both return
`422 value must be one of 'Snowflake', 'BigQuery'`.

**2. `PATCH` exists — the documented DELETE + POST edit path is destructive.**
`PATCH /api/v1/repository/{type}/{id}` updates in place, keeps the object's UUID and bumps
its revision (verified 1 -> 2 on both a metric and a glossary term). The skill said "the
metastore has no PATCH" and built rollback machinery around delete-then-post, which mints a
new UUID, resets revision history, breaks anything referencing the old UUID, and can leave
the layer missing an object if the POST fails.

Rewrote the edit section around `api_patch` (new helper) — send only changed fields. The
rename cascade to constraint `metrics[]` is preserved and also now in-place. The rollback
block is gone: nothing is deleted, so there is nothing to roll back.

All 30 tests pass; confirmed the corrected guards fail when the bug is reintroduced.
@cjayyy cjayyy self-assigned this Aug 3, 2026
@cjayyy

cjayyy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

CI note: Tier 0 (static lint + case validation) passes — that's the job covering these changes, and it's the one that was previously asserting the wrong casing.

Tier 1 (skill activation routing) fails for an unrelated infra reason — the repo's Anthropic key is out of credit:

anthropic.BadRequestError: Error code: 400 — 'Your credit balance is too low to
access the Anthropic API. Please go to Plans & Billing to upgrade or purchase credits.'

It errors before evaluating any case, so it's not a signal about this diff. Locally: 30 passed.

@cjayyy
cjayyy requested a review from a team August 3, 2026 13:50
@linear-code

linear-code Bot commented Aug 3, 2026

Copy link
Copy Markdown

AI-3661

@cjayyy
cjayyy marked this pull request as ready for review August 3, 2026 13:50
@cjayyy

cjayyy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Re-checked the Tier 1 failure after the CI notification — it is not caused by this PR, and nothing in the diff can fix it. Evidence:

  1. It fails on the API call, before evaluating any case. The job runs 3s and dies at:

    anthropic.BadRequestError: Error code: 400 — 'Your credit balance is too low to
    access the Anthropic API. Please go to Plans & Billing to upgrade or purchase credits.'
    

    The secret is present (the secret not available line in the log is the script being echoed, not the branch taken), so this isn't a fork-skip — the repo's Anthropic account is out of credit.

  2. Tier 1 routes on skill frontmatter, which this PR does not touch. git diff origin/main..HEAD -- …/SKILL.md contains no name: or description: change.

  3. The deterministic half of Tier 1 passes locallyactivation/test_activation_cases.py + test_parse_answer.py: 80 passed. Those are the offline case-set and answer-parsing checks that don't need an API key.

  4. Tier 0 — the job that actually covers these changes — passes, and it's the one that was previously asserting the wrong sql_dialect casing.

So: needs credit topped up on the repo's Anthropic account to go green. Happy to re-run once that's sorted.

@cjayyy
cjayyy merged commit 58b358b into main Aug 3, 2026
1 of 2 checks passed
@cjayyy
cjayyy deleted the vn/sl-toolkit-patch-and-sql-dialect branch August 3, 2026 14:19
claude Bot pushed a commit that referenced this pull request Aug 3, 2026
Resolve conflict from PR #94 (sl-toolkit: PATCH edits + snake_case sql_dialect).
PR #94 modified 7 files under plugins/sl-toolkit/ which #88 consolidated into
plugins/keboola/. Ported all content changes to the new locations:
- commands/sl-build.md, skills/semantic-layer/SKILL.md,
  tests/{fixtures,schemas}/semantic-model.json,
  tests/test_skill_consistency.py, tests/test_smoke.py
plugins/sl-toolkit/README.md: kept #88's deletion (per-plugin READMEs were
consolidated); its one-line PATCH note is preserved in the ported SKILL.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants