Review fixes: server/, ci-timing and video_frames return rows, no hand-built JSON or files

[asubbarao]

Fixes from running duckstack-review over main 2e26f01. Each rewrite ran old vs new on the same inputs; EXCEPT ALL both ways was 0 for every one.

File	Fix	Compared
server/profile.sql	tera loops/ifs/set moved into SQL; templates only substitute; printf rounding -> DECIMAL casts	6 pages, 0/0
server/macros.sql	quote_literal: dollar quotes instead of chr(39)	5 inputs, identical
server/host.sql	terminal_sessions unnests receipt rows instead of json_extract_string(..., '$[*].pid')	live tabs 5 vs 5
server/replica/pull.sql	latest attempt per table with arg_max, not QUALIFY row_number() = 1	5 synthetic rows
skills/ci-timing/SKILL.md	PR files come back in the receipts; no COPY to raw/prfiles-*.json and read back	192 vs 192
skills/tera/references/video_frames.tera + .sql	rows returned, no Parquet; scenes built from pass-one receipts	12 scenes / 255 frames, 0/0

Left, with reasons: printf('%,d') thousands grouping in profile.sql; fs.dirs/fs.filelist and replica pull_log tables that boot needs; maximum_depth := 1 in agent.sql (STRUCT vs MAP merge); xml_extract_elements in ext_catalog.sql. Known bug not changed: quote_literal does not escape a quote inside the value (O'Brien) — changing an approved macro's output needs Alok.

🤖 Generated with Claude Code

[asubbarao]

duckstack/rules: failure

• block sql_pattern_functions__tooling in server/host.sql: Parse with string functions, parsers and extension functions first; a short pattern is allowed only on web pages and log lines, on an explicit petition.
• block sql_pattern_functions in server/host.sql: Parse with string functions, parsers and extension functions first; a short pattern is allowed only on web pages and log lines, on an explicit petition.
• block sql_create_macro in server/macros.sql: Propose the macro to Alok first (name, parameters, body, what it replaces) and create it only on his yes; otherwise spell the SQL out.
• block sql_create_macro__dispatch in server/macros.sql: Propose the macro to Alok first (name, parameters, body, what it replaces) and create it only on his yes; otherwise spell the SQL out.
• block sql_create_macro__dev in server/macros.sql: Propose the macro to Alok first (name, parameters, body, what it replaces) and create it only on his yes; otherwise spell the SQL out.
• block sql_create_macro__tooling in server/macros.sql: Propose the macro to Alok first (name, parameters, body, what it replaces) and create it only on his yes; otherwise spell the SQL out.
• warn sql_dispatch_shape in server/host.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.
• warn sql_vague_cte_names in server/host.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• warn sql_file_too_long in server/host.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 server/profile.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_row_number_pick in skills/ci-timing/SKILL.md#5.sql: No picking rows with QUALIFY row_number() ... <= n: arg_max / max_by, or a join on keys (10-17).

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

[asubbarao]

duckstack/rules: success

• warn sql_file_too_long in server/host.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 server/host.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.
• warn sql_vague_cte_names in server/host.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• warn sql_file_too_long in server/profile.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_row_number_pick in skills/ci-timing/SKILL.md#5.sql: No picking rows with QUALIFY row_number() ... <= n: arg_max / max_by, or a join on keys (10-17).

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

[asubbarao]

duckstack/review: failure

• skills/watch-video/SKILL.md:36 RULES 25 (delete wrong advice); RULES 26: This edited paragraph still says every bash flag is a template context value (ffmpeg_input_flags, ffmpeg_filters, ffmpeg_output_flags), and the knob table still lists engine. The video_frames.tera after this PR declares only video, out_dir, fps, start, end, width, quality and sql_tag, and has no ffmpeg branch, so an agent following the skill would pass context the template never reads.
  Rewrite: Two passes: the whole video at fps, then each busy scene re-rendered from the same template with start, end and the busy fps (one rendered statement per scene, built from pass one's receipts and run by array_agg(selfdispatch(sql))). The template's context is exactly video, out_dir, fps, start, end, width, quality, sql_tag; a different capture is a different row, never a different program. — and delete the engine row of the table and the closing 'ffmpeg branch … unverified' rule, since the template no longer has that branch.
• skills/tera/references/video_frames.sql:31 RULES 7 (declare everything up front in the first CTE); check 4 (intent CTE): busy types the video path, out_dir, width, quality and sql_tag a second time, and the busy fps (10), the busy threshold (16 bits) and the scene threshold (10 bits, lines 28 and 48) appear partway through the query. Editing the first CTE alone therefore leaves pass two pointed at the old video, which the header comment ('Edit the literals') does not warn about.
  Rewrite: WITH video AS (
  SELECT '/abs/path/help.mov' AS video, '/abs/scratch/help_frames' AS out_dir, 2 AS fps, 1400 AS width, 0 AS start, -1 AS "end",
  0.8 AS quality, 'frames_cmd' AS sql_tag, 10 AS scene_bits, 16 AS busy_bits, 10 AS busy_fps
  ), pass_one_rendered AS (
  SELECT v., tera_render('video_frames.tera', v::JSON, autoescape := false, template_path := '…/references/.tera') AS sql FROM video AS v
  ), pass_one_dispatched AS ( -- the declared values ride along with the receipts; no singleton cross join
  SELECT * EXCLUDE (sql), array_agg(selfdispatch(sql)) AS receipts FROM pass_one_rendered GROUP BY ALL
  ), pass_one AS (
  SELECT d.* EXCLUDE (receipts, fps, start, "end"), (frame ->> 'second')::DOUBLE AS second, frame ->> 'path' AS path, (frame ->> 'phash')::BIGINT AS phash
  FROM pass_one_dispatched AS d CROSS JOIN UNNEST(receipts) AS t(receipt) CROSS JOIN UNNEST(receipt.body::JSON[]) AS f(frame)
  ), …
  sum(CASE WHEN bits_changed IS NULL THEN 1 WHEN bits_changed >= scene_bits THEN 1 ELSE 0 END) OVER (ORDER BY second) AS scene …
  ), busy AS (
  SELECT video, out_dir, busy_fps AS fps, width, min(second) AS start, max(second) + 0.5 AS "end", quality, sql_tag
  FROM pass_one_scenes GROUP BY video, out_dir, busy_fps, width, quality, sql_tag, busy_bits, scene
  HAVING max(bits_changed) >= busy_bits ORDER BY max(bits_changed) DESC LIMIT 20
  )

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

[asubbarao]

duckstack/rules: success

• warn sql_dispatch_shape in server/host.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.
• warn sql_file_too_long in server/host.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 server/host.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• warn sql_file_too_long in server/profile.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_row_number_pick in skills/ci-timing/SKILL.md#5.sql: No picking rows with QUALIFY row_number() ... <= n: arg_max / max_by, or a join on keys (10-17).

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

[asubbarao]

duckstack/review: failure

• skills/ci-timing/SKILL.md:95 check 5 (raw data kept): The PR-files stage no longer keeps GitHub's raw response, which contradicts the skill's own 'raw/ first, never overwritten' rule and the jobs stage just above it, which tees into raw/ and still returns its rows in the receipt.
  Rewrite: FROM read_json($cmd$gh api --paginate --slurp "repos/{{ repo }}/pulls/{{ pr }}/files?per_page=100" | tee raw/prfiles-{{ run_id }}-{{ pr }}.json |$cmd$)
• skills/watch-video/SKILL.md:49 RULES 26: This advice was not run: the query has no scenes relation (its last SELECT is FROM numbered), and a WHERE clause cannot use the aggregate aliases ends and starts.
  Rewrite: Add HAVING max(second) - min(second) >= 3 to the query's last SELECT … FROM numbered GROUP BY scene, ordered by starts.
• skills/tera/references/video_frames.sql:18 RULES 1 / check 1: Both passes call tera_render inside array_agg(selfdispatch(...)) instead of building the statements in one CTE and dispatching them in the next, and the template call and its path are written out twice (lines 18 and 34).
  Rewrite: ), pass_one_rendered AS (
  SELECT , tera_render('video_frames.tera', v::JSON, autoescape := false,
  template_path := '/Users/aloksubbarao/duckdb-skills/skills/tera/references/.tera') AS sql
  FROM video AS v
  ), pass_one AS (
  SELECT * EXCLUDE (sql), array_agg(selfdispatch(sql)) AS receipts FROM pass_one_rendered GROUP BY ALL
  -- same pair (busiest_rendered, pass_two) for pass two
• skills/tera/references/video_frames.sql:29 RULES 7: The + 0.5 hard-codes one frame interval at pass one's 2 fps, so changing fps in the first CTE quietly makes the busy-scene spans wrong.
  Rewrite: SELECT video, out_dir, busy_fps AS fps, width, min(second) AS start, max(second) + 1.0 / pass_one_scenes.fps AS "end", quality, sql_tag,
  scene_bits, max(bits) AS scene_bits_changed
  FROM pass_one_scenes GROUP BY video, out_dir, pass_one_scenes.fps, busy_fps, width, quality, sql_tag, scene_bits, busy_bits, scene
• skills/tera/references/video_frames.sql:36 RULES 7: The busy-scene count (20) is a literal in the middle of the query rather than a value in the first CTE, even though watch-video lists it as a knob. The stated reason, 'LIMIT takes no column', doesn't hold, because a list slice does take a column.
  Rewrite: -- video CTE: add 20 AS busy_scenes; busy: carry busy_scenes in its SELECT and GROUP BY
  ), busiest AS ( -- the busy_scenes scenes that moved most
  SELECT unnest(array_agg(b ORDER BY scene_bits_changed DESC)[1:busy_scenes], recursive := true)
  FROM busy AS b GROUP BY busy_scenes
  ), pass_two AS (
  SELECT *, array_agg(selfdispatch(sql)) AS receipts FROM busiest_rendered GROUP BY ALL
• server/profile.sql:125 RULES 17, RULES 9: The page context is a hand-built json_object with nine keys, and the latency cast (lines 125 and 132) and the CPU sum cast (lines 125 and 133) are each written twice, when a named row could be the context.
  Rewrite: ), page AS (
  SELECT $sql AS sql, sum(seconds)::DECIMAL(18, 4)::VARCHAR AS cpu,
  (profile ->> 'latency')::DOUBLE::DECIMAL(18, 4)::VARCHAR AS latency,
  CASE WHEN saved_status = '200' THEN tera_render(...) ELSE tera_render(...) END AS saved,
  arg_max(shown, seconds) AS top, len(flatten(list(members))) AS operators,
  list(tera_render('…', shown::JSON) ORDER BY seconds DESC) AS phases,
  list_transform(list_sort(flatten(list(members))), m -> m.html) AS nodes,
  list_transform([{t: 'Latency', v: latency, unit: ' s'}, {t: 'Operator CPU', v: cpu, unit: ' s'}, …],
  c -> tera_render('
  …
  ', c::JSON)) AS cards
  FROM shown_phase GROUP BY profile, path, saved_status
  )
  SELECT tera_render($t$…$t$, page::JSON) AS html FROM page;
• server/host.sql:60 RULES 14: The new unnest aliases t(r) and p(tty_row) don't describe what they hold, and t is reused two lines later as the alias for terminal_tabs.
  Rewrite: FROM dispatched CROSS JOIN UNNEST(receipts) AS tab(tab_receipt)
  CROSS JOIN UNNEST(try_cast(tab_receipt.receipt.body AS JSON[])) AS holder(tty_row)
  -- and SELECT tab_receipt.window_id, tab_receipt.tab_index, (tty_row ->> 'pid')::BIGINT AS pid
• server/host.sql:235 check 8 / RULES 13, RULES 9: To avoid writing OR, the exemption test adds the lengths of two list_filter results and checks the sum is above 0, and that arithmetic hides a simple 'any of these predicates' test.
  Rewrite: AND NOT (group_spawned_by = 'orphan'
  AND list_bool_or([starts_with(group_root_args, '/System/'), starts_with(group_root_args, '/usr/'),
  starts_with(group_root_args, '/Library/'), starts_with(group_root_args, '/opt/homebrew/Cellar/podman/'),
  contains(group_root_args, '.app/Contents/')]))

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

[asubbarao]

duckstack/rules: success

• warn sql_vague_cte_names in server/host.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• warn sql_file_too_long in server/host.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 server/host.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.
• warn sql_row_number_pick in skills/ci-timing/SKILL.md#5.sql: No picking rows with QUALIFY row_number() ... <= n: arg_max / max_by, or a join on keys (10-17).

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

[asubbarao]

duckstack/review: failure

• (none supplied):0 review input missing: a clean verdict needs evidence from the code: The prompt held only the skill, RULES.md and the manifesto. No PR diff, changed .sql file or ```sql block came with it, and this headless run has no tool to read one. With no code to check, a clean verdict would approve code nobody read, and auto-merge would act on it.
  Rewrite: Have queue.sql include the PR's added lines in the review prompt, e.g. the output of gh pr diff <n> read through shellfs as `read_text($cmd$gh pr diff --repo asubbarao/duckstack |$cmd$)`. Then rerun duckstack/review on the current head.

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

[asubbarao]

duckstack/review: failure

• skills/watch-video/SKILL.md:36 RULES 25 (delete wrong advice): The paragraph that continues from this line still names ffmpeg_input_flags, ffmpeg_filters and ffmpeg_output_flags as template context. The rewritten video_frames.tera has no ffmpeg branch and takes no such values.
  Rewrite: …built from pass one's receipts and run by array_agg(selfdispatch(sql))). The template's context is the row: video, out_dir, fps, start, end, width, quality, sql_tag; a different slice or rate is a different row, never a different program. (Also drop the engine/ffmpeg table row and Rules bullet.)
• skills/watch-video/SKILL.md:49 RULES 26 (tested before offered): This advice cannot run. The query's last SELECT is FROM numbered, not scenes, and a filter on aggregates needs HAVING, not WHERE.
  Rewrite: Add HAVING max(second) - min(second) >= 3 to the query's last SELECT … FROM numbered GROUP BY scene, and order it ORDER BY starts.
• skills/tera/references/video_frames.sql:47 RULES 10 (lossless, array_agg not count): len(array_agg(frame_path)) builds each scene's frame list and then throws it away, so the result keeps only a count.
  Rewrite: array_agg(frame_path ORDER BY second) AS frame_paths, len(frame_paths) AS frames, max(bits) AS most_bits_changed, arg_max(frame_path, second) AS settled_frame
• skills/tera/references/video_frames.sql:40 Check 4 (readable relations) / RULES 14: The second UNION ALL branch has unnamed columns and lines up with the first only by position. It also repeats the JSON parsing from lines 22–23.
  Rewrite: SELECT second, frame_path, phash, scene_bits FROM pass_one_frames
  UNION ALL BY NAME
  SELECT (frame ->> 'second')::DOUBLE AS second, frame ->> 'path' AS frame_path, (frame ->> 'phash')::BIGINT AS phash, scene_bits
  FROM pass_two
  CROSS JOIN UNNEST(receipts) AS t(receipt)
  CROSS JOIN UNNEST(receipt.body::JSON[]) AS f(frame)
• skills/tera/references/video_frames.sql:26 Check 9 (bloat): The scene-numbering expression is written out twice (lines 26 and 43), and its CASE has two branches that both return 1.
  Rewrite: SELECT *, sum((coalesce(bits, scene_bits) >= scene_bits)::INT) OVER (ORDER BY second) AS scene
• skills/tera/references/video_frames.sql:29 RULES 7 (declare everything up front): max(second) + 0.5 hard-codes one pass-one frame step for fps = 2. Change fps in the first CTE and the busy span is wrong.
  Rewrite: SELECT video, out_dir, busy_fps AS fps, width, min(second) AS start, max(second) + 1.0 / base_fps AS "end", quality, sql_tag,
  scene_bits, max(bits) AS scene_bits_changed
  FROM (SELECT * EXCLUDE (fps), fps AS base_fps FROM pass_one_scenes)
  GROUP BY video, out_dir, busy_fps, base_fps, width, quality, sql_tag, scene_bits, busy_bits, scene
• skills/tera/references/video_frames.sql:36 RULES 7 (declare everything in the first CTE): The busy-scene count of 20 is a literal buried in pass_two. The stated reason, 'LIMIT takes no column', does not hold: a DuckDB LIMIT accepts a scalar subquery, and the house ban on scalar subqueries covers only SELECT lists.
  Rewrite: In video, add 20 AS busy_scenes. Then: FROM (FROM busy ORDER BY scene_bits_changed DESC LIMIT (SELECT busy_scenes FROM video)) AS b
• skills/tera/references/video_frames.sql:34 Check 1 / RULES 1 (one array of self-posts): GROUP BY ALL over each busy row produces 20 one-element arrays instead of one array of posts, and the render is again hidden inside the dispatch. The only column used downstream is scene_bits.
  Rewrite: pass_two_rendered AS (
  SELECT scene_bits, tera_render('video_frames.tera', b::JSON, autoescape := false,
  template_path := '/Users/aloksubbarao/duckdb-skills/skills/tera/references/*.tera') AS sql
  FROM (FROM busy ORDER BY scene_bits_changed DESC LIMIT (SELECT busy_scenes FROM video)) AS b
  ), pass_two AS (
  SELECT scene_bits, array_agg(sql) AS sqls, array_agg(selfdispatch(sql)) AS receipts FROM pass_two_rendered GROUP BY scene_bits
  )
• skills/tera/references/video_frames.sql:18 Check 1 / manifesto §4 (visible dispatch): array_agg(selfdispatch(tera_render(…))) nests the render inside the dispatch, so the generated SQL is never a column beside its receipt. GROUP BY ALL over every config column is used only to carry the row along.
  Rewrite: pass_one_rendered AS (
  SELECT , tera_render('video_frames.tera', v::JSON, autoescape := false,
  template_path := '/Users/aloksubbarao/duckdb-skills/skills/tera/references/.tera') AS sql
  FROM video AS v
  ), pass_one AS (
  SELECT * EXCLUDE (sql), sql, array_agg(selfdispatch(sql)) AS receipts FROM pass_one_rendered GROUP BY ALL
  )
• skills/ci-timing/SKILL.md:94 RULES 10 (lossless), check 7: Stage 3 keeps only file.filename and the final SELECT (line 102) pulls out four named keys. That drops status, additions, deletions and changes, which the old raw/prfiles copies kept. A key that goes missing breaks the projection.
  Rewrite: SELECT {{ run_id }} AS run_id, '{{ event }}' AS event, {{ pr }} AS pr, file.*
  FROM read_json($cmd$gh api --paginate --slurp "repos/{{ repo }}/pulls/{{ pr }}/files?per_page=100" |$cmd$)
  CROSS JOIN UNNEST(json) AS page(file)
  …
  SELECT (file ->> 'run_id')::BIGINT AS run_id, (file ->> 'pr')::BIGINT AS pr, file ->> 'filename' AS filename, file AS pr_file
  FROM files_dispatched
  CROSS JOIN UNNEST(receipts) AS r(receipt)
  CROSS JOIN UNNEST(receipt.body::JSON[]) AS f(file);
• server/host.sql:223 RULES 8, 13 (code as data, no chains): The codex exclusions are a hand-written chain of three AND NOT starts_with(g.args, …) clauses (lines 223–225). The reap expression at the end of the same file already keeps its prefixes as a list.
  Rewrite: WHEN contains(p.args || ' ', '/codex ')
  AND len(list_filter(['/Applications/ChatGPT.app/', '/Users/aloksubbarao/.codex/', '/Users/aloksubbarao/.local/bin/'],
  k -> starts_with(g.args, k))) = 0 THEN 'codex'

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

[asubbarao]

duckstack/rules: failure

• block sql_new_table_in_file in server/ext_catalog.sql: Propose the new TABLE (name, columns, why a view or a direct extension call cannot do it) and create it only on Alok's yes; derive state as views.
• existing sql_file_too_long in server/fs.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_vague_cte_names in server/fs.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• existing sql_new_table_in_file in server/fs.sql: Propose the new TABLE (name, columns, why a view or a direct extension call cannot do it) and create it only on Alok's yes; derive state as views.
• existing sql_dispatch_shape in server/host.sql: Self-dispatch has one shape: source rows, one generated statement per row, array_agg(selfdispatch(sql)), then CROSS JOIN UNNEST of the receipts.
• existing sql_file_too_long in server/host.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_vague_cte_names in server/host.sql: Name every CTE for what it holds (content_first_100, self_dispatch), not t / x / sd / tmp.
• existing sql_row_number_pick in skills/ci-timing/SKILL.md#5.sql: No picking rows with QUALIFY row_number() ... <= n: arg_max / max_by, or a join on keys (10-17).

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