Skip to content

Self-dispatch shape is a block; mined rules become hook checks - #61

Open
asubbarao wants to merge 18 commits into
mainfrom
review/selfdispatch-shape
Open

asubbarao wants to merge 18 commits into
mainfrom
review/selfdispatch-shape

Conversation

@asubbarao

@asubbarao asubbarao commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

Written by Opus 5.5 (Claude Code) for Alok.

What changed

  • Dispatch shape blocks (sql_dispatch_shape). A query that calls selfdispatch must collect it with array_agg/list(selfdispatch(...)) and read the results back with CROSS JOIN UNNEST. Each row's statement may be a concat or a template; the check does not look at templates. A new sql_dispatch_direct_post check blocks hand-written quackapi_post(':9495/sql', …) calls.

  • 16 new hook checks, all mined from Alok's corrections. They cover:

    • shellfs pipes: tail/head, timeout, sed/awk/tr/cut, sleep, nested duckdb -c, curl to dev, and true / : exit hacks;
    • readers: the _auto readers and read_text over a pipe;
    • SQL forms: replace(replace()), list_transform(range()), range as a dispatch source, row_to_json passed to Tera, first_value/last_value, QUALIFY, and hard-coded /Users/ paths.

    Each check was tested with a counter-example that fires; a clean read_csv('cmd |') does not.

  • RULES.md adds rules 28–49, mined from all 5,361 of Alok's SQL and dispatch prompts (Claude and Codex, 09-14 to 10-18). Rebased on main: RULES.md: settle the writeup's contradictions by date #77's text wins on rules 2, 5, 12, 17 and 18, and rule 28 defers to rule 5.

  • duckstack-review gains a 10-point self-dispatch checklist: rows, concat or template, macro, UNNEST, receipts as columns, status, unparsed bodies, timeouts, narrow statements, and order.

  • network.tera (new): one request per row to another host, never dev's own :9495; it unnests quackapi_post, so status, reason and body come back as columns.

Found while testing: when a generated statement fails, dev returns a non-200 with the error in the body, and receipt.error stays NULL. On 10-19 binder, parser, cast and missing-file errors all came back as 422 (a 500 was seen 10-18). A failure check therefore tests receipt.status IS DISTINCT FROM 200; every example derives that flag.

File length (gate warning sql_file_too_long):

  • hooks/hook_checks.sql is one data relation, one row per check. Its single concern is the list of checks, so adding a check adds a row; this PR adds 16.
  • hooks/toolcall_facts.sql is the hook's parse of a tool call into facts. This PR adds one fact column, shellfs_pipes (each shellfs command parsed with sitting_duck's bash grammar).

Splitting either file would scatter one relation across files.

Review rounds:

  • Rounds 1–3: all findings fixed, or answered in the thread (rule 43's count line dropped; shell.tera still needs the options key).
  • Round 4: shell-pipe checks parse each command with parse_ast_list(…, 'bash') and match the programs it runs. git log --grep tail and echo "sleep" no longer fire; /usr/bin/sed and $(sleep 1) do.
  • Round 5: sql_shellfs_exit_hack now uses the parsed pipe (list_has_any(pipe.commands, ['true', ':'])). On six hook events: before, || true hit, ||true, || /usr/bin/true and || : were missed, and echo "a || true b" was a false positive; after, the four hacks hit and the quoted text and a clean pipe do not. The transcript GOOD example joins open sessions to agent.messages with the filter pushed in: 56 prompt rows, 14.6 KB, where the raw .jsonl read returned 4.5 MB and 10.7 MB bodies and a 507.

Known follow-ups (not in this PR)

  • skills/self-dispatch/references/five_stage_pipeline.sql still uses inline $t$ statements and ::JSON[] over receipt bodies.
  • 26 existing files still use the old inline-template dispatch, among them server/lake.sql, server/ext_catalog.sql, skills/crawl/references/crawl_rows.sql and useful_queries.sql. 3 more use range as a source. Any future PR that edits one of them must convert it.
  • The warn-level checks (/Users paths, read_text pipes) fire on existing code, including canonical.sql's template_path.

🤖 Generated with Claude Code

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao
asubbarao enabled auto-merge October 18, 2026 21:54
@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • skills/self-dispatch/RULES.md:118 RULES.md 25 (no contradictory rules): 'never unnest() in a SELECT list' reads as a blanket ban, but it contradicts the owner's own example in SKILL.md (SELECT unnest(['/tmp', …]) AS path) and the unnest-built hook_checks view. The 10-12/10-13 correction was about receipts.
    Rewrite: 30. One array, one CROSS JOIN UNNEST(receipts) per step; never unnest(receipts) in a SELECT list. A receipt stays on its source row, so nothing joins back afterwards (10-12, 10-13). Make the same scoping change in SKILL.md line 149.
  • skills/self-dispatch/RULES.md:121 RULES.md 25 (delete wrong advice; no contradictory rules): Rule 31 says WITH ORDINALITY only numbers the elements and ordering comes from array_agg(... ORDER BY ...), but rule 2 (and the manifesto §4) still tell readers to use WITH ORDINALITY when order matters, so the file now holds two conflicting rules.
    Rewrite: In the same PR, edit rule 2 to: 'inside one array use array_agg(… ORDER BY …) / array_sort only when order matters; WITH ORDINALITY only numbers the elements; otherwise it is async (09-24, 10-08, 10-14).' Then drop the duplicate sentence from rule 31.
  • hooks/hook_checks.sql:337 RULES.md 17 (a few canonical templates: shell, pipeline, sql, network, loop) / RULES.md 8: The new block allows only shell.tera, sql.tera and bash_loop_fallback_method.tera, so a dispatch through the canonical pipeline or network template gets denied, and the allowed list is a literal typed into the predicate rather than taken from the templates folder.
    Rewrite: Declare the canonical set once, with every shape rule 17 names: list_has_any(f.literals, ['shell.tera', 'pipeline.tera', 'sql.tera', 'network.tera', 'bash_loop_fallback_method.tera']). Better still, join a canonical_templates view (hostfs ls of skills/self-dispatch/templates) and make the same change to the list in SKILL.md line 144 and RULES.md line 106.
  • hooks/hook_checks.sql:383 Check 9 (bloat) / RULES.md 8 (code as data): The same pipe tokenizer, string_split(translate(l, E'|;&\t\n()', ' '), ' ') guarded by ends_with(rtrim(l), '|'), is pasted into seven predicates (lines 383, 387, 391, 395, 399 twice, and 403) instead of being one derived column.
    Rewrite: Add a column to toolcall_facts: sql_shellfs_pipe_words := flatten of the tokenized words of every table-function literal ending in '|'. Then each predicate is one line, e.g. list_has_any(sql_shellfs_pipe_words, ['tail', 'head']), list_has_any(sql_shellfs_pipe_words, ['sed', 'gsed', 'awk', 'gawk', 'tr', 'cut']), list_contains(sql_shellfs_pipe_words, 'sleep').
  • hooks/hook_checks.sql:407 RULES.md 13 (OR chains) / check 8: list_has_any([contains(l, '|| true'), contains(l, '; true')], [true]) is an OR hidden inside a list of booleans, not a set of terms.
    Rewrite: len(list_filter(['|| true', '; true'], hack -> contains(l, hack))) > 0
  • skills/duckstack-review/SKILL.md:212 Check 6 / RULES.md 12 (readable strings, no doubled quotes): The reader options are a SQL fragment with a leading comma and doubled quotes (', format := ''newline_delimited'''), which is the escape-heavy string building that check 6 flags.
    Rewrite: SELECT 'read_json' AS reader, {'format': 'newline_delimited'} AS options, ... and have shell.tera render the struct's key := value pairs; or, at minimum, write $opt$format := 'newline_delimited'$opt$ AS options and let the template add the separator.
  • skills/duckstack-review/SKILL.md:217 RULES.md 38 (sql_hardcoded_user_path): The second 'good' example hard-codes the same /Users/aloksubbarao/ template path.
    Rewrite: template_path := concat_ws('/', getenv('HOME'), '.duck/deploy/skills/self-dispatch/templates/*.tera')
  • skills/duckstack-review/SKILL.md:175 RULES.md 38 (sql_hardcoded_user_path, added by this PR): The new 'good' example hard-codes /Users/aloksubbarao/ in template_path, which breaks the rule this same PR adds.
    Rewrite: template_path := concat_ws('/', getenv('HOME'), '.duck/deploy/skills/self-dispatch/templates/*.tera')

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • skills/self-dispatch/RULES.md:18 RULES 25 (delete wrong advice): Rule 2 now says WITH ORDINALITY only numbers the elements, and that array_agg(… ORDER BY …) sets the order. SQL-MANIFESTO §4 ('Use WITH ORDINALITY when order matters') and the CLAUDE.md/AGENTS.md mirrors still teach the old rule, so the rubric now contradicts itself.
    Rewrite: In this PR, change SQL-MANIFESTO.md §4 to 'Use array_agg(… ORDER BY …) when order inside one array matters; WITH ORDINALITY only numbers the receipts.' Propose the same wording for the CLAUDE.md / AGENTS.md mirrors.
  • skills/self-dispatch/RULES.md:155 RULES 25 (delete wrong advice): The new rule 43 says len(array_agg(…)) used as a readout is still a count. This PR leaves two places that recommend exactly that: SKILL.md 'How to run it' step 2 ('row counts as len(array_agg(…))') and the sql_star_count hook message ('len(list(x)) or a named count column').
    Rewrite: In this PR, change SKILL.md step 2 to 'Run what can be run on dev: EXPLAIN, SUMMARIZE / DESCRIBE, timing'. Change the sql_star_count message to 'No star count: DESCRIBE / SUMMARIZE for shape, array_agg(DISTINCT ...) for values.'
  • skills/duckstack-review/SKILL.md:212 check 6, RULES 12: The reader options are declared as a raw SQL fragment with a leading comma and embedded quotes ($opt$, format := 'newline_delimited'$opt$). That is string plumbing pasted into the statement, not values declared as data.
    Rewrite: Declare the option as a value: 'newline_delimited' AS format. The template then substitutes only that value (, format := '{{ format }}'), so the first CTE holds values rather than SQL fragments.
  • skills/duckstack-review/SKILL.md:210 RULES 29, checklist item 6: The rewritten GOOD session example ends with a bare SELECT receipt.* and derives no status flag. Checklist item 6, added in this same PR, calls that a finding.
    Rewrite: End the example with SELECT receipt.*, receipt.status IS DISTINCT FROM 200 AS failed FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);, the same as the first GOOD example.
  • hooks/hook_checks.sql:381 check 9; gate warning sql_file_too_long: The gate warned that hook_checks.sql and toolcall_facts.sql are over 80 lines and asks the PR to split them or state the reason and line count. The PR adds 16 rows and the description does neither.
    Rewrite: Either state in the PR body that hook_checks.sql is one data relation of N rows, that one row per check is its single concern, and its line count, or move the mined 2026-10-18 SQL rows into their own file (hooks/checks_sql_mined.sql, one view) that hook_checks UNION ALL BY NAMEs.
  • hooks/hook_checks.sql:419 check 9 (bloat): list_has_any(f.args[1:1], [...]) slices out a one-element list just to test the first argument.
    Rewrite: len(list_filter(sql_function_calls, f -> f.name IN ('list_transform', 'list_apply', 'apply') AND f.args[1] IN ('range', 'generate_series'))) > 0
  • hooks/hook_checks.sql:434 check 9 (bloat): The new sql_qualify check is the exact complement of the existing sql_row_number_pick check (sql_has_qualify AND NOT row_number against sql_has_qualify AND row_number). Both warn, so two rows do the work of one.
    Rewrite: Delete sql_qualify. Change sql_row_number_pick to 'predicate_sql': $p$sql_has_qualify$p$ with the message 'No picking rows with QUALIFY (row_number, rank, dense_rank "latest"): arg_max / max_by, or a join on keys (10-08, 10-17).'
  • hooks/hook_checks.sql:337 RULES 8 (code as data): The list of canonical templates is typed by hand in the predicate and again in the message, and once more in SKILL.md and RULES.md. Adding a template means editing four places.
    Rewrite: Declare the template names once, as a fact computed in toolcall_facts: len(list_filter(function_calls, f -> f.name = 'tera_render' AND list_has_any(f.literals, canonical_templates))) > 0 AS uses_canonical_template, where canonical_templates comes from one declared column or the hostfs listing of templates/*.tera. The predicate becomes list_contains(sql_functions, 'selfdispatch') AND NOT (list_contains(sql_functions, 'array_agg') AND sql_has_unnest_cross_join AND sql_uses_canonical_template). The message points to templates/ instead of repeating the names.
  • hooks/toolcall_facts.sql:213 RULES 10 (lossless), check 9: sql_shellfs_pipe_words puts the words of every pipe in every statement of the call into one distinct bag, using three nested flattens. That loses which command each word came from, so the co-occurrence checks built on it can fire across two unrelated commands: duckdb in one pipe plus -c in another (shell.tera's own jq -c), or curl in one plus :9495 in another.
    Rewrite: Keep one word list per command, built where the call is parsed. In node_facts: list(CASE WHEN tfn IS NOT NULL THEN flatten([string_split(translate(l, E'|;&\t\n()', ' '), ' ') FOR l IN literal_args IF ends_with(rtrim(l), '|')]) END) AS shellfs_pipes. In sql_call_facts: flatten(list(shellfs_pipes)) AS sql_shellfs_pipes. Test co-occurrence inside one command: len(list_filter(sql_shellfs_pipes, p -> list_contains(p, 'duckdb') AND list_contains(p, '-c'))) > 0. Single-word checks use list_has_any(flatten(sql_shellfs_pipes), [...]).

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • hooks/toolcall_facts.sql:149 RULES 8 (code as data), RULES 45 (derive input lists from relations, never typed literal lists): The canonical template names are typed in the parser as a literal list (['shell.tera', 'sql.tera', 'bash_loop_fallback_method.tera']), so they repeat what the templates folder already holds. They are also copied into the hook message, SKILL.md and RULES.md, and adding a template means editing the parser.
    Rewrite: Read the names from the templates folder once, as a relation, and check against it:
    CREATE OR REPLACE VIEW canonical_templates AS
    SELECT list(parse_filename(file)) AS names
    FROM glob('~/.duck/deploy/skills/self-dispatch/templates/*.tera');
    -- in node_facts:
    bool_or(fn = 'tera_render' AND list_has_any(literal_args, canonical_templates.names)) AS uses_canonical_template
    -- with canonical_templates joined on keys, or its names passed into the fact through the caller's tc_src, the same way bash_src is
  • skills/duckstack-review/SKILL.md:212 sql_coalesce_empty_default / RULES 7, 10 (never invent data); PR description: The PR notes say the session example "passes no options at all" and that "the options string is gone", but the GOOD example still declares '' AS options. That invents an empty value instead of leaving the column out, and the example the reviewer is told to copy does not match what the author says was tested.
    Rewrite: WITH sessions AS (
    SELECT 'read_json' AS reader, -- read_json detects newline-delimited JSON itself
    [concat_ws('/', getenv('HOME'), '.claude/projects', translate(cwd, '/.', '--'), concat(sessionId, '.jsonl'))] AS sources
    FROM terminal_sessions WHERE sessionId IS NOT NULL
    ), ... -- rerun it on dev and show status-200 rows; if shell.tera fails without an options key, say so in the PR rather than passing ''

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • hooks/toolcall_facts.sql:147 SQL-MANIFESTO §6 / RULES 19 (parse with extension functions, not string surgery): The new shared fact shellfs_pipes tokenizes shell text by hand: translate turns punctuation into spaces, then string_split on ' '. Six new block-level checks depend on it (tail/head, timeout, sed/awk/tr/cut, sleep, nested duckdb, curl to self). Hand tokenizing gets real cases wrong both ways. A quoted argument such as git log --grep tail or echo "sleep" fires a check that shouldn't fire. An absolute-path command (/usr/bin/sed) or one inside $(...) gets past the check. The repo already parses shell with real parsers: sitting_duck, and the parse-bash views used for bash_command_details.
    Rewrite: Parse each pipe literal with sitting_duck's bash grammar and keep the command names, so the checks compare executables rather than loose words:
    shellfs_command_nodes AS (
    SELECT f.call_id, f.sql_source, f.program_index, f.statement_no, n.peek AS pipe_word
    FROM node_facts f
    CROSS JOIN UNNEST(f.shellfs_literals) AS u(pipe_literal)
    CROSS JOIN LATERAL parse_ast(rtrim(rtrim(u.pipe_literal), '|'), 'bash') AS n
    WHERE n.type = 'command_name')
    with shellfs_literals collected as flatten(list_filter(list(CASE WHEN tfn IS NOT NULL THEN [l FOR l IN literal_args IF ends_with(rtrim(l), '|')] END), x -> x IS NOT NULL)). Then shellfs_pipes becomes list(parse_filename(pipe_word)) grouped per statement. If parse_ast only accepts a literal here, self-dispatch it per literal. Once that is in place, switch the existing sql_shellfs_grep check to read this fact too, so only one tokenizer is left.

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • hooks/toolcall_facts.sql:225 same-query aliases (CLAUDE.md minimal SQL): flatten(list(shellfs_pipes)) is computed on line 224 as sql_shellfs_pipes and then computed again inside line 225 rather than reused through the alias.
    Rewrite: list_distinct(flatten([p.commands FOR p IN sql_shellfs_pipes])) AS sql_shellfs_commands,
  • hooks/hook_checks.sql:407 RULES.md 37 / PR's own round-4 claim of one shell parser: sql_shellfs_exit_hack still matches loose substrings ('|| true', '; true') in the raw literal, even though round 4 moved every other shell-pipe check onto the bash-parsed sql_shellfs_pipes. So echo "x || true" | fires, || /usr/bin/true doesn't, and a second ad-hoc way of spotting shellfs pipes (ends_with(rtrim(l), '|')) stays alive.
    Rewrite: 'predicate_sql': $p$list_contains(sql_shellfs_commands, 'true')$p$,
  • hooks/toolcall_facts.sql:149 check 9 (bloat) / check 1 (measure, narrow work): Each shellfs literal is parsed with parse_ast_list(rtrim(rtrim(l), '|'), 'bash') twice, once for commands (line 149) and once for words (line 150), and the hook pays for this on every SQL-carrying tool call.
    Rewrite: flatten(list_filter(list(CASE WHEN tfn IS NOT NULL THEN
    [{'commands': [parse_filename(node.peek) FOR node IN pipe_ast IF node.type = 'command_name'],
    'words': [node.peek FOR node IN pipe_ast IF node.type = 'word']}
    FOR pipe_ast IN [parse_ast_list(rtrim(rtrim(l), '|'), 'bash') FOR l IN literal_args IF ends_with(rtrim(l), '|')]] END), x -> x IS NOT NULL)) AS shellfs_pipes

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.

duckstack review queue, rule gate (hooks/hook_checks.sql)

1 similar comment
@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • hooks/hook_checks.sql:407 RULES.md 37 / PR round 4 (one shell parser); check 6: sql_shellfs_exit_hack still finds pipes with ends_with(rtrim(l), '|') and then substring-matches '|| true' / '; true' in the raw literal. Round 4 moved every other shell check to the bash-grammar fact, but this one was left out: it misses ||true and || :, and it fires on a quoted argument that contains the text.
    Rewrite: 'predicate_sql': $p$len(list_filter(sql_shellfs_pipes, pipe -> list_has_any(pipe.commands, ['true', ':']))) > 0$p$
  • hooks/toolcall_facts.sql:155 check 9 (bloat) / measure: each statement narrow: Each shellfs literal is parsed with parse_ast_list twice, once for command_name nodes and once for word nodes, and rtrim(rtrim(l), '|') is written twice. Every pipe on every hooked tool call pays for two bash parses.
    Rewrite: Parse each pipe once and filter the same node list twice: flatten(list_filter(list(CASE WHEN tfn IS NOT NULL THEN [{'commands': [parse_filename(x.peek) FOR x IN pipe_ast IF x.type = 'command_name'], 'words': [x.peek FOR x IN pipe_ast IF x.type = 'word']} FOR pipe_ast IN [parse_ast_list(rtrim(rtrim(l), '|'), 'bash') FOR l IN literal_args IF ends_with(rtrim(l), '|')]] END), x -> x IS NOT NULL)) AS shellfs_pipes

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

1 similar comment
@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_dispatch_shape in skills/self-dispatch/SKILL.md#1.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_dispatch_shape in skills/self-dispatch/SKILL.md#1.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • skills/self-dispatch/templates/canonical.sql:48 check 9 (readability): The new example is numbered 4 but sits before example 3, and the file header still says 'The three self-dispatch templates', so a reader can't match the examples to the template table.
    Rewrite: Move the network block after example 3 and update the header: '-- The four self-dispatch templates, one example each ... shell | sql | network: a request to another host | bash_loop_fallback_method: a FALLBACK'.
  • hooks/toolcall_facts.sql:219 RULES 9 (same-query aliases, readable): sql_shellfs_commands recomputes flatten(list(shellfs_pipes)), which the line above already aliased as sql_shellfs_pipes. It also shares its line with sql_cte_names, and lines 177 and 235 have the same two-columns-on-one-line merge.
    Rewrite: flatten(list(shellfs_pipes)) AS sql_shellfs_pipes,
    list_distinct(flatten([p.commands FOR p IN sql_shellfs_pipes])) AS sql_shellfs_commands,
    list_distinct(flatten(list(cte_names))) AS sql_cte_names,
  • hooks/toolcall_facts.sql:149 check 1/9 (efficiency, narrow work): Every shellfs literal is parsed with parse_ast_list twice, once for command_name nodes and once for word nodes, inside a CASE-then-list_filter-null construction.
    Rewrite: Parse once per literal in its own named relation and derive both lists from it:
    shell_literals AS (
    SELECT call_id, sql_source, program_index, statement_no,
    parse_ast_list(rtrim(rtrim(l), '|'), 'bash') AS bash_nodes
    FROM sql_nodes CROSS JOIN UNNEST(literal_args) AS u(l)
    WHERE tfn IS NOT NULL AND ends_with(rtrim(l), '|'))
    ... list({'commands': [parse_filename(n.peek) FOR n IN bash_nodes IF n.type = 'command_name'],
    'words': [n.peek FOR n IN bash_nodes IF n.type = 'word']}) AS shellfs_pipes ... GROUP BY ALL
  • hooks/hook_checks.sql:407 check 6 / RULES 40 (loose string matching where a parser exists): sql_shellfs_exit_hack still matches the substrings '|| true' and '; true' in raw literals. Round 4 says every shell-pipe check now reads sitting_duck's bash parse, and this one ignores it, so a quoted argument fires and ||true does not.
    Rewrite: 'predicate_sql': $p$len(list_filter(sql_shellfs_pipes, pipe -> list_has_any(pipe.commands, ['true', ':']))) > 0$p$,
  • skills/self-dispatch/RULES.md:134 RULES 12; hook sql_concat_chain: Rule 34a gives the same three-operand || chain as the canonical statement form, which contradicts rule 12 and trips the concat-chain warning.
    Rewrite: concat (concat('SELECT * FROM read_csv(', quote_literal(path), ')')); a template is for when the holes get many.
  • skills/duckstack-review/SKILL.md:144 RULES 12; hook sql_concat_chain: The checklist names a three-operand || chain as the default statement form. Rule 12 bans || chains, and the house hook's own sql_concat_chain check (more than two operands) warns on this exact example.
    Rewrite: 2. The statement is a concat or a template. A plain concat('SELECT * FROM read_csv(', quote_literal(path), ')') is the default; ...
  • skills/self-dispatch/SKILL.md:15 RULES 19, 18; check 2: The skill's lead example lists folders with ls -la through read_csv(header := false). That parses ls text into unnamed column0 rows, but hostfs already lists folders, and fs.folders is the source this same query reads.
    Rewrite: WITH folders AS (
    SELECT name AS folder_name, path FROM fs.folders WHERE parent = concat(getenv('HOME'), '/.duck')
    ), dispatched AS (
    SELECT folder_name,
    array_agg(selfdispatch(concat('SELECT * FROM ls(', quote_literal(path), ')'))) AS receipts
    FROM folders GROUP BY ALL
    )
    SELECT folder_name, receipt.*, receipt.status IS DISTINCT FROM 200 AS failed
    FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);
  • skills/self-dispatch/templates/network.tera:4 check 9 (bloat); RULES 12: The template wraps a single scalar call in a SELECT * FROM (...) subquery that adds nothing, and it puts {{ url }} raw between single quotes, so any quote in the URL breaks the generated statement.
    Rewrite: SELECT quackapi_post({{ url_literal }}, $body${{ body }}$body$) AS response
    -- with url_literal := quote_literal(url) declared as a column in the first CTE
  • skills/self-dispatch/templates/canonical.sql:51 RULES 32; check 1 (dispatch hidden in a template): The network example labels 'http://127.0.0.1:9495/sql' as 'other_host', but that URL is dev itself. Each row therefore makes a hand-written quackapi_post to dev's /sql, hidden inside network.tera, which also gets past sql_dispatch_direct_post because that check only reads literals in the query.
    Rewrite: WITH requests AS (
    SELECT host_name AS target, concat('http://', host_name, ':9495/sql') AS url,
    json_object('sql', 'SELECT 42 AS answer')::VARCHAR AS body
    FROM remote_duckdb_hosts -- a real relation of other hosts; dev itself is reached only through selfdispatch(sql)
    ), rendered AS ( ...network.tera... ), dispatched AS (
    SELECT target, array_agg(selfdispatch(sql)) AS receipts FROM rendered GROUP BY ALL
    )
    SELECT target, receipt.* FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

1 similar comment
@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • skills/self-dispatch/SKILL.md:38 RULES.md 25 (write down what was verified): This file says a failed statement comes back as '422 or 500'. RULES.md 29 and the duckstack-review checklist record only the verified 500, so the three documents disagree.
    Rewrite: - Check status. A failed statement comes back as 500 with DuckDB's error in the body and receipt.error NULL (verified 2026-10-18). Flag it with receipt.status IS DISTINCT FROM 200 and keep the row.
  • hooks/toolcall_facts.sql:218 Check 9 / RULES.md 9 (readable): Edit leftovers put two column definitions on one line with a run of spaces between them, here and at lines 176 and 234 (sql_shellfs_commands, … sql_cte_names, shellfs_pipes, … cte_names, sql_has_qualify, … sql_lateral_functions). That breaks the file's one-column-per-line layout.
    Rewrite: Put each column on its own line, for example:
    list_distinct(flatten([p.commands FOR p IN flatten(list(shellfs_pipes))])) AS sql_shellfs_commands,
    list_distinct(flatten(list(cte_names))) AS sql_cte_names,
    (and the same at 176 and 234).
  • hooks/hook_checks.sql:337 RULES.md 26 (tested before offered), 27: The PR says sql_dispatch_shape requires a canonical template called by name, using a uses_canonical_template fact built from a glob(...templates/*.tera) in sql_statement_facts. Neither the fact nor the glob is in toolcall_facts.sql, and the predicate only checks array_agg/list plus any unnest cross join somewhere in the call.
    Rewrite: Make the description match the code: say the shape check requires array_agg/list plus a CROSS JOIN UNNEST and does not check templates. Or add the fact and use it: list_contains(sql_functions, 'selfdispatch') AND NOT (list_contains(sql_functions, 'array_agg') AND sql_has_unnest_cross_join), with a separate check on uses_canonical_template once the column exists in sql_statement_facts.
  • hooks/hook_checks.sql:407 Check 6 / PR's own one-shell-parser claim: sql_shellfs_exit_hack still matches the substrings '|| true' and '; true' in the raw literal. Round 4 says every shell-pipe check now uses the parsed bash fact. This check misses ||true and || /usr/bin/true, and it fires on a quoted argument that contains the text.
    Rewrite: 'predicate_sql': $p$len(list_filter(sql_shellfs_pipes, pipe -> list_has_any(pipe.commands, ['true', ':']))) > 0$p$
  • skills/self-dispatch/RULES.md:115 RULES.md 5 and 24; CLAUDE.md (a missing quackapi feature is a change to quackapi): Rule 28 allows ->> per field on receipt.body 'until quackapi returns typed replies'. That relaxes Alok's rule 5, which says results come back as columns with no manual field parsing, to work around a missing quackapi feature.
    Rewrite: 28. Receipts are columns. Read receipt.status, receipt.error, receipt.body as columns. The body is JSON text today (tested 10-18); the fix is typed replies in quackapi (asubbarao/quackapi issue), not field-by-field ->> in callers. Until then, the next stage reads the body with a reader in its own dispatched statement; no ->> ladders, no hand-typed from_json.
  • skills/self-dispatch/RULES.md:135 RULES.md 12 (no || chains): Rule 34a gives a || chain as the canonical concat, which contradicts rule 12 in the same file. The self-dispatch SKILL.md uses concat(...) instead.
    Rewrite: concat (concat('SELECT * FROM read_csv(', quote_literal(path), ')')); a template is for when the holes get many.
  • skills/duckstack-review/SKILL.md:144 RULES.md 12 (no || chains), hook sql_concat_chain: The checklist calls 'SELECT * FROM read_csv(' || quote_literal(path) || ')' the default. That is a three-operand || chain, which rule 12 bans and the hook's sql_concat_chain check warns on.
    Rewrite: 2. The statement is a concat or a template. A plain concat('SELECT * FROM read_csv(', quote_literal(path), ')') is the default; a template from templates/ is called by name once the holes get many.
  • skills/self-dispatch/templates/network.tera:4 RULES.md 32, check 6: The template's comment says another DuckDB's /sql is 'a host like any other'. That invites a call back to dev itself, and nothing in the template or the hook stops a url on 127.0.0.1:9495.
    Rewrite: Change the comment to: {# A request to another host (never dev's own :9495; dev is selfdispatch(sql)). Row: url, body. #}. Also extend sql_dispatch_direct_post so it looks at tera_render('network.tera', …) calls whose context literals contain 127.0.0.1:9495 or localhost:9495.
  • skills/self-dispatch/templates/canonical.sql:52 RULES.md 32 (post through the macro), check 1: The network example points url at dev's own http://127.0.0.1:9495/sql. The rendered statement is then a hand-written quackapi_post to dev's /sql, which rule 32 bans. The literal sits in a CTE column instead of a quackapi_post argument, so sql_dispatch_direct_post never sees it.
    Rewrite: Keep the network example for other hosts only: WITH requests AS (SELECT 'peer' AS target, 'http://<peer-host>:9495/sql' AS url, json_object('sql', 'SELECT 42 AS answer')::VARCHAR AS body). Add a comment that the example needs a reachable peer. A statement meant for dev runs as array_agg(selfdispatch('SELECT 42 AS answer')), the sql.tera example above, and never goes through network.tera.

Opus 5.5, duckstack review queue (duckstack-review skill)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • warn sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • warn sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • warn sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • warn sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • hooks/hook_checks.sql:337 RULES.md 25 / 26 (write down what was verified): The PR description says sql_dispatch_shape requires a canonical template called by name, adds a uses_canonical_template fact, and globs the template names in sql_statement_facts. None of these is in the code: the predicate accepts any array_agg(selfdispatch) plus CROSS JOIN UNNEST, and toolcall_facts adds only shellfs_pipes.
    Rewrite: Correct the PR description to say what the code enforces: 'sql_dispatch_shape requires array_agg/list(selfdispatch(...)) and CROSS JOIN UNNEST; the statement may be a concat or a template.' Remove the uses_canonical_template and glob claims.
  • hooks/hook_checks.sql:407 RULES.md 19 (parsers for code) / manifesto §6: sql_shellfs_exit_hack still matches substrings in the literal (contains(l, '|| true')). It misses ||true and fires on a quoted argument. That contradicts round 4's claim that shell checks now use the sitting_duck bash parse and no loose word matching is left.
    Rewrite: 'predicate_sql': $p$len(list_filter(sql_shellfs_pipes, pipe -> list_contains(pipe.commands, 'true'))) > 0$p$,
  • skills/duckstack-review/SKILL.md:224 RULES.md 19, 34 / checklist 9 / manifesto §9: The new GOOD example reads each session's raw .jsonl transcript in full: the PR's own note says each receipt body is 4–26 MB. This is the broad, unfiltered transcript read that rule 19 (agent.stream / agent_data for transcripts) and rule 34 (push the filter into each generated statement) forbid.
    Rewrite: WITH sessions AS (
    SELECT sessionId AS session_id FROM terminal_sessions WHERE sessionId IS NOT NULL
    ), dispatched AS (
    SELECT session_id, array_agg(selfdispatch(concat('SELECT timestamp, content_bytes, content FROM agent.messages WHERE is_prompt AND timestamp > now() - INTERVAL 1 DAY AND session = ', quote_literal(session_id)))) AS receipts
    FROM sessions GROUP BY ALL
    )
    SELECT session_id, receipt.*, receipt.status IS DISTINCT FROM 200 AS failed FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);
  • skills/self-dispatch/SKILL.md:17 RULES.md 29 / duckstack-review checklist 6: The skill's main example reads status and body but derives no failure flag, although line 38 of the same file says to flag every failure with receipt.status IS DISTINCT FROM 200 and keep the row.
    Rewrite: SELECT folder_name, receipt.status, receipt.body, receipt.status IS DISTINCT FROM 200 AS failed
    FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);
  • skills/self-dispatch/templates/canonical.sql:60 RULES.md 29 / duckstack-review checklist 6 (status checked): The new network example returns receipt.* with no failure flag. That breaks the status rule this PR adds, in the file it calls the verified reference.
    Rewrite: SELECT target, receipt.*, receipt.status IS DISTINCT FROM 200 AS failed FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);
  • skills/self-dispatch/templates/network.tera:4 RULES.md 5 / 28 (receipts are columns, no ladders): The template puts the whole quackapi_post struct into one response column. The verified receipt body is therefore [{"response":{"status":…,"body":"…"}}], and reading the remote status or body takes a ->0->'response'->>'body' ladder, which rule 28 bans.
    Rewrite: SELECT unnest(quackapi_post($url${{ url }}$url$, $body${{ body }}$body$))

Opus 5.5, duckstack review queue (duckstack-review skill)

asubbarao and others added 2 commits October 19, 2026 09:40
Mined all 5,361 of Alok's SQL/dispatch prompts (Claude + Codex, 09-14 to 10-18)
with the agent-stream skill's prompts query. RULES.md gains rules 28-49; 16 of
them are now hook checks (shellfs tail/head, timeout, sed/awk, sleep, nested
duckdb, curl to /sql, exit hacks, _auto readers, nested replace,
list_transform(range), range dispatch sources, row_to_json, first/last_value,
QUALIFY, /Users paths, read_text pipes). The dispatch shape check now blocks:
canonical template by name, selfdispatch macro, array_agg + CROSS JOIN UNNEST.
duckstack-review gets a self-dispatch checklist covering status, unparsed
bodies and timeouts; its examples use the canonical templates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Pipe checks share one fact, sql_shellfs_pipe_words, in toolcall_facts.
- || true check is a list_filter over terms, not a list of booleans.
- unnest ban scoped to receipts; rule 2 owns ordering (WITH ORDINALITY only numbers).
- Rule 17 names the three templates that exist (shell, sql, bash_loop fallback).
- Review examples use getenv('HOME') for template_path and a $opt$ options string.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
asubbarao and others added 16 commits October 19, 2026 09:40
A checked-in .sql run with duckdb -f as its own process (how the hooks and
the review queue's per-PR programs run) is not the nested inline program
the rule bans.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e QUALIFY check

- toolcall_facts keeps one word list per shellfs command (sql_shellfs_pipes), so
  co-occurrence checks (duckdb + -c, curl + :9495) stay inside one command.
- sql_uses_canonical_template is the one place the template names live.
- sql_qualify folded into sql_row_number_pick (any QUALIFY warns).
- list_transform(range) check reads args[1] directly.
- Manifesto and the UNNEST hook message: array_agg ORDER BY orders, ORDINALITY numbers.
- Rule 43 drops the secondhand 'len(array_agg) is a count' line; Alok's own
  10-05/10-08 words recommend len(array_agg).
- Session example: no options string, read_json detects NDJSON; adds failed flag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
toolcall_facts matches each statement's tera_render literals against
glob(~/.duck/deploy/skills/self-dispatch/templates/*.tera), inside
sql_statement_facts so the AST parse is shared (a separate view re-parsed
and ran out of memory). Same peak memory and time as main on the same inputs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tera_render node joins the folder's files by name, so the separate CTE, its DISTINCT, the HOME concat and the
final join are gone. Tested on two events: a dispatch naming shell.tera passes the shape check, an inline template blocks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
toolcall_facts parses each shellfs literal with parse_ast_list(…, 'bash') and
keeps the programs it runs (basenames, $(...) included) and its words. The
pipe checks and sql_shellfs_grep now match executables: 'git log --grep tail'
and 'echo "sleep"' no longer fire; '/usr/bin/sed' and '$(sleep 1)' do. The
template match is one LEFT JOIN glob inside node_facts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…der, split command words

RULES 34a-c: the generated statement carries the row's keys (no parallel arrays
matched by position); one tera_render per statement; command words split by role
and joined with same-query aliases. New block sql_tera_nested: a tera_render inside
another's arguments, found from the AST.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- RULES 34a: array_agg({key cols..., 'receipt': selfdispatch(sql)}), UNNEST, read
  d.key next to d.receipt.* (no parallel arrays).
- RULES 34b: nested tera_render is fine when a statement dispatches further
  (an array sent to another host's :9495); sql_tera_nested block removed.
- network.tera: one POST per row; canonical.sql example 4, tested on dev
  (nested post to :9495 returned answer 42 with its row's target).
- Rule 17 names the four templates; git comes from dev's duck_tails views.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sql_dispatch_positional: a self-dispatch that indexes an array by a column
(array_extract(col, col)) is the parallel-array pairing; the message gives the
row+receipt array_agg form. Tested: the parallel form is caught, the struct form passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The skill's core example aggregated receipts alone, so any agent needing a row's
key afterwards invented parallel arrays. It now aggregates the row with its
receipt (verified on dev: 46 folders, each receipt beside its folder_name), names
the four templates incl. network.tera, allows nesting, and points git at dev's
duck_tails views. The description says when to reach for it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The skill now leads with the pure form Alok gave: concat each row's statement,
SELECT the row's columns with array_agg(selfdispatch(sql)) GROUP BY ALL, then
CROSS JOIN UNNEST, so every receipt sits beside its row (verified on dev, ls -la
per folder, all 200). Templates are for statements with many holes.
sql_dispatch_shape no longer demands a template (it blocked the plain concat);
the template-folder fact is gone. Examples and checklists use the group-by form.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- SKILL.md headline: hostfs ls(quote_literal(path)) per folder, with the rows dev
  returned (and the 500 rows a bad column gives); no shell text parsing.
- network.tera: dollar-quoted url and body, the post itself as the statement.
- canonical example 4 labels dev as dev; says to put another host:9495 in.
- toolcall_facts parses each shell literal once (rtrim once).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eturns typed replies

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ct dispatch shape, rows shown

- RULES 34a and the review checklist teach concat(..., quote_literal(path), ...) instead of a || chain
- canonical.sql example 4 posts to httpbin, not dev's own /sql
- sql_dispatch_shape again requires array_agg over selfdispatch itself; list_transform_range drops f.args[1]
- toolcall_facts: one column per line
- duckstack-review examples show the rows they returned on dev

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed transcript example

- sql_shellfs_exit_hack matches a true or : command in the parsed shellfs pipe instead of
  substrings; catches ||true, /usr/bin/true and ':', no longer fires on quoted text.
- network.tera unnests quackapi_post, so status, reason and body come back as columns.
- canonical.sql and self-dispatch SKILL.md examples derive the failed flag.
- duckstack-review GOOD example joins open sessions to agent.messages with the filter
  pushed in, instead of reading whole raw transcripts.
- Failed statements are recorded as 422 (measured 10-19), not 500.
- RULES.md: main's #77 text wins on rules 2, 17 and 18; rule 28 defers to main's rule 5.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@asubbarao
asubbarao force-pushed the review/selfdispatch-shape branch from ede0abf to 1fd0918 Compare October 19, 2026 17:11
@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/rules: success

  • existing sql_coalesce_empty_default in hooks/check_hits.sql: Never invent data: missing values stay NULL (NULLIF(col, '') for empty strings); COALESCE only expresses source precedence.
  • existing sql_file_too_long in hooks/check_hits.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • existing sql_file_too_long in hooks/hook_checks.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.
  • existing sql_json_extract in hooks/toolcall_facts.sql: Read JSON with read_json and its typed columns (dot access on structs); no json_extract on nested fields.
  • existing sql_vague_cte_names in hooks/toolcall_facts.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
  • existing sql_file_too_long in hooks/toolcall_facts.sql: Keep a job or loader file to 30-80 lines and one concern; split it, or state the reason and line count in the PR.

duckstack review queue, rule gate (hooks/hook_checks.sql)

@asubbarao

Copy link
Copy Markdown
Owner Author

duckstack/review: failure

  • skills/self-dispatch/SKILL.md:14 RULES 7; manifesto 4 (keep the generated sql beside its receipt): The skill's own description says to concat each row's statement into a column, but the reference example builds it inside the selfdispatch() call, so the statement is never declared and can't be shown next to its receipt.
    Rewrite: WITH folders AS (
    SELECT name AS folder_name, concat('SELECT * FROM ls(', quote_literal(path), ')') AS sql
    FROM fs.folders WHERE parent = concat(getenv('HOME'), '/.duck')
    ), dispatched AS (
    SELECT folder_name, sql, array_agg(selfdispatch(sql)) AS receipts FROM folders GROUP BY ALL
    )
    SELECT folder_name, sql, receipt.status, receipt.body, receipt.status IS DISTINCT FROM 200 AS failed
    FROM dispatched CROSS JOIN UNNEST(receipts) AS t(receipt);
  • skills/self-dispatch/SKILL.md:29 RULES 25-26 (write down what was verified): Line 29 says the failed variant of an example verified on 10-19 came back with status 500. Line 39 says failed statements return 422 as of 10-19 and that 500 was only seen on 10-18, so one of the two claims is stale.
    Rewrite: in place of *, every row came back with a non-200 status (422 on 2026-10-19) and DuckDB's error in the body: a failure is a row, not a crash.
  • SQL-MANIFESTO.md:79 RULES 2 and 25 (RULES.md overrides; delete wrong advice instead of keeping contradictory rules): The new text says WITH ORDINALITY only numbers the receipts and that array_agg(… ORDER BY …) is how to order. RULES.md rule 2, which this PR leaves unchanged, still says to use WITH ORDINALITY / array_sort when order matters, so the manifesto, hook message 226 and review checklist item 10 now disagree with the file that overrides them.
    Rewrite: Change RULES.md rule 2 in this PR so all four places match: 'inside one array, order comes from array_agg(… ORDER BY …) only when order matters; WITH ORDINALITY numbers the receipts; otherwise it is async (09-24, 10-08, 10-19).' Keep manifesto lines 79-80 as written.
  • hooks/toolcall_facts.sql:219 Shorter version (columns derived once); RULES 9: sql_shellfs_commands works out flatten(list(shellfs_pipes)) again, even though line 218 already names that value sql_shellfs_pipes in the same SELECT.
    Rewrite: list_distinct(flatten([p.commands FOR p IN sql_shellfs_pipes])) AS sql_shellfs_commands,

Opus 5.5, duckstack review queue (duckstack-review skill)

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.

1 participant