Dev

[alirezarezvani]

Summary

Checklist

• Target branch is dev (not main — PRs to main will be auto-closed)
• Skill has SKILL.md with valid YAML frontmatter (name, description, license)
• Scripts (if any) run with --help without errors
• No hardcoded API keys, tokens, or secrets
• No vendor-locked dependencies without open-source fallback
• Follows existing directory structure (domain/skill-name/SKILL.md)

Type of Change

• New skill
• Improvement to existing skill
• Bug fix
• Documentation
• Infrastructure / CI

Testing

[claude]

Code Review — snowflake-development Skill

PR #424 · +1595 / −13 · State: MERGED

This is a well-structured new skill with solid domain coverage (SQL best practices, Dynamic Tables, Streams/Tasks, Cortex AI, Snowpark, dbt, RBAC). The reference files are thorough. A few issues to address in a follow-up:

---

🔴 Bug — --json flag is unreachable after a subcommand

File: engineering-team/snowflake-development/scripts/snowflake_query_helper.py line 161–163

--json is registered on the top-level parser, not on each subparser. In argparse, parent-level flags must appear before the subcommand name. Anything after the subcommand is parsed by the subparser, so --json placed at the end is silently ignored or raises an "unrecognized arguments" error.

The module docstring example on line 13 demonstrates the broken form:

python snowflake_query_helper.py merge --target t --source s --key id --columns a,b --json

Fix options:

1. Move --json into each subparser (add merge_p.add_argument("--json", ...) etc.), or
2. Move it before the subcommand in the docstring/examples (script.py --json merge ...).

---

🟡 generate_merge hardcodes updated_at

File: scripts/snowflake_query_helper.py lines 35–46

The function unconditionally appends updated_at = CURRENT_TIMESTAMP() to every MERGE statement. If the target table doesn't have an updated_at column the generated SQL will fail at runtime with no warning to the user. Either:

• Add an --updated-at / --no-updated-at flag so users can opt in/out, or
• Add a comment in the generated SQL noting that the column must exist.

---

🟡 Missing license in YAML frontmatter

File: engineering-team/snowflake-development/SKILL.md lines 1–4

The PR checklist explicitly requires name, description, and license in frontmatter. The current frontmatter only has two fields:

---
name: "snowflake-development"
description: "..."
---

Add license: "MIT" (or whichever license applies) to match every other skill in the repo.

---

🟡 STRIP_NULL_VALUE typo

File: engineering-team/snowflake-development/SKILL.md line 54

Use STRIP_NULL_VALUE = TRUE on load.

Snowflake's JSON file format option is STRIP_NULL_VALUES (plural). The singular form STRIP_NULL_VALUE is not a valid Snowflake option and will silently be ignored or cause a parse error.

---

🟡 Cortex AI function table is incomplete in SKILL.md

File: engineering-team/snowflake-development/SKILL.md lines 120–130

The function reference table lists 7 functions but omits AI_SUMMARIZE and AI_TRANSLATE, both of which are present in references/cortex_ai_and_agents.md. Users reading only the SKILL.md will not know these exist. The deprecated mapping section at line 130 also calls out SUMMARIZE → should map to the correct current name.

---

🟡 Overall skill count not updated in CLAUDE.md

File: CLAUDE.md lines 9 and 206

The PR updated the architecture section to say "30 core engineering skills" but did not update the two remaining instances that still read "205 production-ready skills across 9 domains" (should be 206). Also 268 Python automation tools may need updating if the new script is being counted.

---

🟢 PR checklist not filled in

No checklist items were checked ([ ] throughout). This is informational — the skill looks like it would pass all items — but it's worth enforcing the habit so reviewers can trust the checklist.

---

✅ What's done well

• YAML frontmatter present (name + description)
• Three reference files with substantial depth (snowflake_sql_and_pipelines.md, cortex_ai_and_agents.md, troubleshooting.md)
• Script uses only stdlib, follows CLI-first design, supports --json output intent
• Proactive Triggers section is a nice pattern for surfacing issues automatically
• Cross-references section links to related skills correctly
• All index files updated consistently: .codex/skills-index.json, .gemini/skills-index.json, marketplace.json, mkdocs.yml, docs pages
• Symlinks for Codex/Gemini platform integration created correctly

---

Testing Results

Check	Result
Python syntax (py_compile)	✅ Clean (manual review — no syntax errors found)
YAML frontmatter present	⚠️ Missing license field
Script --help accessible	✅ (argparse structure is valid)
--json flag functional	❌ Broken when placed after subcommand
No hardcoded credentials	✅
No paid API dependencies	✅
Index files consistent	✅ (except overall 205 count in CLAUDE.md)