github-actions[bot] commented on code in PR #4360:
URL: https://github.com/apache/iggy/pull/4360#discussion_r4161247367
##########
scripts/ci/skills.sh:
##########
@@ -47,33 +46,93 @@ fi
echo "🔍 Validating ${#files[@]} skill file(s)..."
# Frontmatter check (SKILL.md only).
-for f in "$SKILLS_DIR"/*/SKILL.md; do
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
awk '
+ # END runs after every exit, so it must not report a second error.
+ function die(msg) { print " ❌ '"$f"': " msg; failed=1; exit 1 }
+ function check_desc() {
+ if (desc == "") die("empty description")
Review Comment:
nit: The emptiness test passes a quoted empty value such as `description:
""`, so the non-empty rule in the file header is not enforced. Strip one pair
of surrounding quotes before the compare.
##########
scripts/ci/skills.sh:
##########
@@ -47,33 +46,93 @@ fi
echo "🔍 Validating ${#files[@]} skill file(s)..."
# Frontmatter check (SKILL.md only).
-for f in "$SKILLS_DIR"/*/SKILL.md; do
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
awk '
+ # END runs after every exit, so it must not report a second error.
+ function die(msg) { print " ❌ '"$f"': " msg; failed=1; exit 1 }
+ function check_desc() {
+ if (desc == "") die("empty description")
+ if (length(desc) > 1024) die("description > 1024 chars")
+ }
BEGIN { state=0 }
- NR==1 && $0!="---" { print " ❌ '"$f"': missing YAML frontmatter on line
1"; exit 1 }
+ NR==1 && $0!="---" { die("missing YAML frontmatter on line 1") }
NR==1 { state=1; next }
+ # A block scalar (description: |) continues on the indented and blank
lines below it.
+ state==1 && in_desc && (/^[[:space:]]/ || $0 == "") {
+ line=$0; sub(/^[[:space:]]+/, "", line)
+ desc = desc (desc == "" ? "" : " ") line
+ next
+ }
+ state==1 && in_desc { in_desc=0; check_desc() }
state==1 && $0=="---" { state=2; exit 0 }
state==1 && /^name:[[:space:]]/ {
name=$0; sub(/^name:[[:space:]]*/, "", name);
- if (name == "") { print " ❌ '"$f"': empty name"; exit 1 }
- if (length(name) > 64) { print " ❌ '"$f"': name > 64 chars"; exit 1 }
- if (name !~ /^[a-z0-9][a-z0-9-]*$/) { print " ❌ '"$f"': name not
kebab-case: " name; exit 1 }
+ if (name == "") die("empty name")
+ if (length(name) > 64) die("name > 64 chars")
+ if (name !~ /^[a-z0-9][a-z0-9-]*$/) die("name not kebab-case: " name)
has_name=1
}
state==1 && /^description:[[:space:]]/ {
desc=$0; sub(/^description:[[:space:]]*/, "", desc);
- if (desc == "") { print " ❌ '"$f"': empty description"; exit 1 }
- if (length(desc) > 1024) { print " ❌ '"$f"': description > 1024 chars";
exit 1 }
has_desc=1
+ if (desc ~ /^[|>][-+]?$/) { desc=""; in_desc=1; next }
Review Comment:
nit: The pattern misses `description: |2` and a trailing comment, so the
length check measures the marker text and accepts an over-long description.
Extend the pattern to accept the indentation and chomping indicators in either
order, and a trailing comment.
##########
scripts/ci/skills.sh:
##########
@@ -47,33 +46,93 @@ fi
echo "🔍 Validating ${#files[@]} skill file(s)..."
# Frontmatter check (SKILL.md only).
-for f in "$SKILLS_DIR"/*/SKILL.md; do
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
awk '
+ # END runs after every exit, so it must not report a second error.
+ function die(msg) { print " ❌ '"$f"': " msg; failed=1; exit 1 }
+ function check_desc() {
+ if (desc == "") die("empty description")
+ if (length(desc) > 1024) die("description > 1024 chars")
+ }
BEGIN { state=0 }
- NR==1 && $0!="---" { print " ❌ '"$f"': missing YAML frontmatter on line
1"; exit 1 }
+ NR==1 && $0!="---" { die("missing YAML frontmatter on line 1") }
NR==1 { state=1; next }
+ # A block scalar (description: |) continues on the indented and blank
lines below it.
+ state==1 && in_desc && (/^[[:space:]]/ || $0 == "") {
+ line=$0; sub(/^[[:space:]]+/, "", line)
+ desc = desc (desc == "" ? "" : " ") line
+ next
+ }
+ state==1 && in_desc { in_desc=0; check_desc() }
state==1 && $0=="---" { state=2; exit 0 }
state==1 && /^name:[[:space:]]/ {
name=$0; sub(/^name:[[:space:]]*/, "", name);
- if (name == "") { print " ❌ '"$f"': empty name"; exit 1 }
- if (length(name) > 64) { print " ❌ '"$f"': name > 64 chars"; exit 1 }
- if (name !~ /^[a-z0-9][a-z0-9-]*$/) { print " ❌ '"$f"': name not
kebab-case: " name; exit 1 }
+ if (name == "") die("empty name")
+ if (length(name) > 64) die("name > 64 chars")
+ if (name !~ /^[a-z0-9][a-z0-9-]*$/) die("name not kebab-case: " name)
has_name=1
}
state==1 && /^description:[[:space:]]/ {
desc=$0; sub(/^description:[[:space:]]*/, "", desc);
- if (desc == "") { print " ❌ '"$f"': empty description"; exit 1 }
- if (length(desc) > 1024) { print " ❌ '"$f"': description > 1024 chars";
exit 1 }
has_desc=1
+ if (desc ~ /^[|>][-+]?$/) { desc=""; in_desc=1; next }
+ check_desc()
}
END {
- if (state != 2) { print " ❌ '"$f"': frontmatter not closed"; exit 1 }
- if (!has_name) { print " ❌ '"$f"': missing name field"; exit 1 }
- if (!has_desc) { print " ❌ '"$f"': missing description field"; exit 1 }
+ if (failed) exit 1
+ if (state != 2) die("frontmatter not closed")
+ if (!has_name) die("missing name field")
+ if (!has_desc) die("missing description field")
}
' "$f" || fail=1
done
+# Codex finds skills through links under .agents/skills, and both clients must
+# agree on whether a skill is user-only.
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
+ skill_dir=${f%/SKILL.md}
+ codex_skill="$CODEX_SKILLS_DIR/${skill_dir##*/}"
+ # Only a relative target resolves in every clone.
+ if [ "$(readlink "$codex_skill")" != "../../$skill_dir" ]; then
+ echo " ❌ $codex_skill: must be a symlink to ../../$skill_dir (ln -s
../../$skill_dir $codex_skill)"
Review Comment:
nit: `ln -s` leaves an existing destination in place, so this hint does not
fix a wrong target. Print `ln -sfn ../../$skill_dir $codex_skill` instead.
##########
scripts/ci/skills.sh:
##########
@@ -47,33 +46,93 @@ fi
echo "🔍 Validating ${#files[@]} skill file(s)..."
# Frontmatter check (SKILL.md only).
-for f in "$SKILLS_DIR"/*/SKILL.md; do
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
awk '
+ # END runs after every exit, so it must not report a second error.
+ function die(msg) { print " ❌ '"$f"': " msg; failed=1; exit 1 }
+ function check_desc() {
+ if (desc == "") die("empty description")
+ if (length(desc) > 1024) die("description > 1024 chars")
+ }
BEGIN { state=0 }
- NR==1 && $0!="---" { print " ❌ '"$f"': missing YAML frontmatter on line
1"; exit 1 }
+ NR==1 && $0!="---" { die("missing YAML frontmatter on line 1") }
NR==1 { state=1; next }
+ # A block scalar (description: |) continues on the indented and blank
lines below it.
+ state==1 && in_desc && (/^[[:space:]]/ || $0 == "") {
+ line=$0; sub(/^[[:space:]]+/, "", line)
+ desc = desc (desc == "" ? "" : " ") line
+ next
+ }
+ state==1 && in_desc { in_desc=0; check_desc() }
state==1 && $0=="---" { state=2; exit 0 }
state==1 && /^name:[[:space:]]/ {
name=$0; sub(/^name:[[:space:]]*/, "", name);
- if (name == "") { print " ❌ '"$f"': empty name"; exit 1 }
- if (length(name) > 64) { print " ❌ '"$f"': name > 64 chars"; exit 1 }
- if (name !~ /^[a-z0-9][a-z0-9-]*$/) { print " ❌ '"$f"': name not
kebab-case: " name; exit 1 }
+ if (name == "") die("empty name")
+ if (length(name) > 64) die("name > 64 chars")
+ if (name !~ /^[a-z0-9][a-z0-9-]*$/) die("name not kebab-case: " name)
has_name=1
}
state==1 && /^description:[[:space:]]/ {
desc=$0; sub(/^description:[[:space:]]*/, "", desc);
- if (desc == "") { print " ❌ '"$f"': empty description"; exit 1 }
- if (length(desc) > 1024) { print " ❌ '"$f"': description > 1024 chars";
exit 1 }
has_desc=1
+ if (desc ~ /^[|>][-+]?$/) { desc=""; in_desc=1; next }
+ check_desc()
}
END {
- if (state != 2) { print " ❌ '"$f"': frontmatter not closed"; exit 1 }
- if (!has_name) { print " ❌ '"$f"': missing name field"; exit 1 }
- if (!has_desc) { print " ❌ '"$f"': missing description field"; exit 1 }
+ if (failed) exit 1
+ if (state != 2) die("frontmatter not closed")
+ if (!has_name) die("missing name field")
+ if (!has_desc) die("missing description field")
}
' "$f" || fail=1
done
+# Codex finds skills through links under .agents/skills, and both clients must
+# agree on whether a skill is user-only.
+for f in "${files[@]}"; do
+ [[ "$f" == */SKILL.md ]] || continue
+ skill_dir=${f%/SKILL.md}
+ codex_skill="$CODEX_SKILLS_DIR/${skill_dir##*/}"
+ # Only a relative target resolves in every clone.
+ if [ "$(readlink "$codex_skill")" != "../../$skill_dir" ]; then
+ echo " ❌ $codex_skill: must be a symlink to ../../$skill_dir (ln -s
../../$skill_dir $codex_skill)"
+ fail=1
+ fi
+
+ claude_flag=$(awk '
+ /^---$/ { if (++section == 2) exit; next }
+ section == 1 && sub(/^disable-model-invocation:[[:space:]]*/, "") {
sub(/[[:space:]]+$/, ""); print; exit }
+ ' "$f")
+ codex_flag=""
+ policy="$skill_dir/agents/openai.yaml"
+ if [ -f "$policy" ]; then
+ codex_flag=$(awk '
+ /^policy:/ { in_policy=1; next }
+ /^[^[:space:]#]/ { in_policy=0 }
+ in_policy && sub(/^[[:space:]]+allow_implicit_invocation:[[:space:]]*/,
"") { sub(/[[:space:]]+$/, ""); print; exit }
+ ' "$policy")
+ fi
+ case "$claude_flag/$codex_flag" in
+ true/false | / | /true | false/ | false/true) ;;
+ true/ | true/true | /false | false/false)
+ echo " ❌ $skill_dir: set disable-model-invocation: true and
policy.allow_implicit_invocation: false together, or neither"
Review Comment:
nit: The message names the key but not the file that holds it. Print
`$policy` so the reader knows which file to edit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
