Copilot commented on code in PR #13451:
URL: https://github.com/apache/trafficserver/pull/13451#discussion_r3676639474
##########
.claude/skills/ats-build/SKILL.md:
##########
@@ -0,0 +1,104 @@
+---
+name: ats-build
+description: Use this skill for all actions related to building Trafficserver,
running tests, or formatting code.
+---
+
+There are two development modes for ATS: host local, and docker mount. If a
Docker container named `ats-dev` exists, then the source
+code is bind mounted into that container. If that
Review Comment:
The introductory paragraph is incomplete (ends with `If that`). Please
complete or remove the fragment so the skill’s guidance is unambiguous.
##########
.claude/skills/ats-build/SKILL.md:
##########
@@ -0,0 +1,104 @@
+---
+name: ats-build
+description: Use this skill for all actions related to building Trafficserver,
running tests, or formatting code.
+---
+
+There are two development modes for ATS: host local, and docker mount. If a
Docker container named `ats-dev` exists, then the source
+code is bind mounted into that container. If that
+
+## Build Process
+
+1. Determine whether build mode is host local or docker mount.
+2. Determine build directory name, which is based on current branch or tag.
This will be used for all builds on this branch/tag.
+3. If that build directory exists, skip this step, otherwise, initialize the
build.
+4. Determine the correct target to use.
+5. Launch the build.
+
+## Command Reference
+
+The commands in this reference are shown without the `docker exec` prefix. If
in docker mount mode, wrap each of the commands in `docker exec`
+
+### Determining Build Directory Name
+
+```bash
+ref=$(git symbolic-ref --short -q HEAD || git describe --tags)
+BUILD_DIR="build-${ref##*/}"
+```
+
+### Initializing a Development Build
+
+```bash
+ATS_BUILD=1 cmake --preset dev -B <build_dir> -DENABLE_AUTEST=ON"
Review Comment:
There is an extra trailing quote (`\"`) at the end of the cmake command,
which will break copy/paste usage. Remove the trailing quote so the command is
valid.
##########
.claude/skills/ats-build/SKILL.md:
##########
@@ -0,0 +1,104 @@
+---
+name: ats-build
+description: Use this skill for all actions related to building Trafficserver,
running tests, or formatting code.
+---
+
+There are two development modes for ATS: host local, and docker mount. If a
Docker container named `ats-dev` exists, then the source
+code is bind mounted into that container. If that
+
+## Build Process
+
+1. Determine whether build mode is host local or docker mount.
+2. Determine build directory name, which is based on current branch or tag.
This will be used for all builds on this branch/tag.
+3. If that build directory exists, skip this step, otherwise, initialize the
build.
+4. Determine the correct target to use.
+5. Launch the build.
+
+## Command Reference
+
+The commands in this reference are shown without the `docker exec` prefix. If
in docker mount mode, wrap each of the commands in `docker exec`
+
+### Determining Build Directory Name
+
+```bash
+ref=$(git symbolic-ref --short -q HEAD || git describe --tags)
+BUILD_DIR="build-${ref##*/}"
Review Comment:
`git describe --tags` can fail when no reachable tags exist (or in some
detached HEAD states), which would leave `ref` empty and make `BUILD_DIR`
incorrect. Consider using `git describe --tags --always` or adding a final
fallback (e.g., short SHA) to ensure the build directory name is always derived
deterministically.
##########
.claude/hooks/ats-build-guard.sh:
##########
@@ -0,0 +1,75 @@
+#!/usr/bin/env bash
+# PreToolUse/Bash guard: redirect build, test, and format commands to the
+# ats-build skill, which encodes project-specific requirements (ats-dev Docker
+# container detection, branch-derived build directory) that raw cmake
+# invocations skip.
+#
+# Escape hatch: prefix the command with ATS_BUILD=1 to acknowledge the skill's
+# procedure is being followed and run the command anyway.
+
+set -uo pipefail
+
+payload=$(cat)
+command=$(printf '%s' "$payload" | jq -r '.tool_input.command // empty')
Review Comment:
The guard depends on `jq` but does not verify it exists. If `jq` is missing,
the script will fail to extract `.tool_input.command` and will effectively
become a silent no-op (allowing direct `cmake/ctest/...` runs against the
intent of this PR). Add an explicit `command -v jq` check and either (a) deny
with a clear reason, or (b) fail-open with an explicit warning to stderr so the
loss of enforcement is visible.
##########
.claude/skills/ats-build/SKILL.md:
##########
@@ -0,0 +1,104 @@
+---
+name: ats-build
Review Comment:
PR description refers to a new `ads-build` skill, but the implementation
adds `ats-build`. Please align the PR description (or the skill name) to avoid
confusion for consumers.
##########
.claude/settings.json:
##########
@@ -0,0 +1,17 @@
+{
+ "hooks": {
+ "PreToolUse": [
+ {
+ "matcher": "Bash",
+ "hooks": [
+ {
+ "type": "command",
+ "command":
"\"$CLAUDE_PROJECT_DIR/.claude/hooks/ats-build-guard.sh\"",
Review Comment:
The `command` value includes embedded quotes, which will likely cause the
hook runner to attempt to execute a command name that literally contains quotes
(and fail to locate the script). The JSON string should typically be the path
itself (or an explicit `bash -lc ...`), without wrapping it in extra escaped
quotes.
--
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]
