docs(skills): document fork re-attach behavior and add fork-before-risky-changes workflow#229
Conversation
…ct, and fork-before-risky-changes workflow
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| hotdata databases set sales # source to protect | ||
| hotdata databases fork --expires-at 24h # deep copy; becomes the active database | ||
| hotdata databases load --catalog sales --table orders --file ./risky.parquet # hits the fork | ||
| ``` | ||
|
|
||
| The fork answers to the same catalog alias as its source, so experimental SQL runs unchanged. Attached connections are re-attached to the fork; indexes are not carried over. When done, keep the fork (`databases set` back to the source) or `databases delete` it. Only DuckLake-backed databases can be forked — see `fork` in the main skill for details. |
There was a problem hiding this comment.
databases set resolves by database id, not a catalog alias. The Set subcommand's arg is documented as "Database id" (src/commands/databases.rs:123-126), and set → get_database performs a direct GET /v1/databases/{id} with no catalog/name resolution (src/commands/databases.rs:1467-1493, 410-412). Every other doc example uses hotdata databases set <id> — this new workflow is the only place that passes a catalog alias (set sales).
Two concrete problems:
-
Opening line —
hotdata databases set salesuses the catalog aliassales, butsetexpects an id, so this likely errors withno database with id 'sales'. -
Cleanup line 145 — "keep the fork (
databases setback to the source)" is unreachable as written. After the fork, the source and the fork both answer to catalogsales(the fork inherits the source's catalog —src/commands/databases.rs:570-572), so there is no unambiguous alias for the source. Even client-side catalog resolution errors here (resolve_database→ "multiple databases have catalog 'sales' — use the database id instead",src/commands/databases.rs:457-466). The agent has no source id to fall back to because the whole workflow deliberately used the alias.
Suggested fix: capture the source id up front (e.g. from databases <id_or_name> / databases list) and use ids for set, or make the workflow reference the source by id explicitly in the cleanup step. This keeps the guidance executable for an agent following it verbatim.
There was a problem hiding this comment.
Fixed in 0392d57 — confirmed against the source that set is id-only (set -> get_database, direct GET with no catalog/name fallback) while fork/delete/inspect resolve via resolve_database. The workflow now captures the source id up front (databases list), uses it for databases set, and calls out explicitly that after the fork the shared catalog alias makes the id the only unambiguous handle on the source. Also corrected the pre-existing set <id_or_name> usage in SKILL.md to set <id> since it's the same defect class.
There was a problem hiding this comment.
Review
Blocking Issues
- skills/hotdata/references/WORKFLOWS.md:140,145 — The new "fork before risky changes" workflow drives
databases setwith a catalog alias (set sales), butsetresolves by database id (directGET /v1/databases/{id}, no catalog/name resolution). The openingdatabases set saleslikely errors, and the cleanup "databases setback to the source" is unreachable because after the fork both the source and the fork share catalogsales, leaving no unambiguous alias for the source. See the inline comment for specific code references and a suggested fix.
Action Required
Reference the source database by id in the workflow (capture it up front and use it for databases set), rather than by the sales catalog alias, so the first and cleanup steps are executable for an agent following the skill verbatim.
Note: the rest of the diff checks out — the fork re-attach / active-database / same-catalog / no-indexes claims match the CLI, and the databases load --catalog sales -> "hits the fork" comment is correct (the active-db preference in tables_load disambiguates the shared catalog).
…ent set as id-only
| hotdata databases load --catalog sales --table orders --file ./risky.parquet # hits the fork | ||
| ``` | ||
|
|
||
| **Capture the source database id up front.** After the fork, both databases answer to the same catalog alias (here `sales`), so the id is the only unambiguous way to refer back to the source. The shared alias means experimental SQL runs unchanged against the fork. Attached connections are re-attached to the fork; indexes are not carried over. When done, keep the fork (`databases set <source_id>` to switch back to the source) or `databases delete` it. Only DuckLake-backed databases can be forked — see `fork` in the main skill for details. |
There was a problem hiding this comment.
super nit: (not blocking) The databases delete cleanup path has the same catalog-ambiguity problem this workflow was rewritten to avoid. After the fork, both source and fork answer to catalog sales, so databases delete sales hits resolve_database and errors with "multiple databases have catalog 'sales' — use the database id instead" (src/commands/databases.rs:457-466). Deleting the fork therefore needs the fork id, but the workflow only tells the agent to capture the source id. The fork id is available (the fork command prints it / it's the new active db), so consider noting that databases delete also needs the fork's dbid..., e.g. "or databases delete <fork_id> it (the id fork printed)."
There was a problem hiding this comment.
Good catch — addressed in the latest commit: the fork step now says to note the fork id from the command output, and the cleanup line reads databases delete <fork_id>. The prose now says to capture both ids (source from databases list, fork from the fork output).
There was a problem hiding this comment.
Prior blocking issue resolved: both databases set usages now take an id (verified against Set { id } at src/commands/databases.rs:123-126 and the id-only get_database path). The load --catalog sales line correctly resolves to the fork via the active-db tie-break in tables_load (src/commands/databases.rs:1635-1653). One non-blocking super nit left inline.
There was a problem hiding this comment.
Cycle 2: prior blocking issue (databases set catalog alias, #3591900061) is resolved — the workflow now captures the source id up front and uses ids for set. Verified the new fork-behavior claims against source (connection re-attach at databases.rs:70-71, fork sets active db at databases.rs:1408, and the load example correctly targets the fork via the active-db catalog-match path at databases.rs:1634-1653). LGTM.
There was a problem hiding this comment.
Cycle 1 blocking issues (catalog-alias ambiguity in set/delete) and the cycle-1 fork-id nit are all resolved in the diff. New claims verified against source: the "re-attached connection catalogs / indexes not carried over" wording matches src/commands/databases.rs:70-71, and set/fork/inspect selection is now consistently id-based. LGTM.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Follow-up to the v0.18.0 fork feature: aligns the bundled agent skill docs with observed CLI behavior (verified live against prod with hotdata 0.18.0).
All other documented fork behaviors (default `-fork` name, expiry carry-over, active-db default source, DuckLake-only error for parquet-backed databases) were tested and matched the existing docs, so no other changes.