Add ats-build skill for Claude harness - #13451
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Introduces an ats-build Claude skill and a Bash pre-hook to enforce using it for build/test/format workflows, while removing build instructions from .claude/CLAUDE.md to reduce context and conflicts.
Changes:
- Added
.claude/skills/ats-build/SKILL.mdwith standardized build/test/format commands and branch-derived build directory guidance. - Added a
PreToolUseBash hook to block directcmake/ctest/autest/format invocations unless explicitly acknowledged. - Simplified
.claude/CLAUDE.mdto direct users to the skill.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .claude/skills/ats-build/SKILL.md | New skill documentation and command reference for ATS build/test/format flows |
| .claude/settings.json | Registers the PreToolUse Bash hook for build-guard enforcement |
| .claude/hooks/ats-build-guard.sh | Implements the guard that denies direct build/test/format tool usage |
| .claude/CLAUDE.md | Removes detailed build instructions and points to the ats-build skill |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (14)
.claude/skills/ats-build/SKILL.md:3
- Project name appears misspelled/cased as 'Trafficserver'. Consider using the canonical name (e.g., 'Traffic Server' or 'Apache Traffic Server') for consistency and searchability.
description: Use this skill for all actions related to building Trafficserver, running tests, or formatting code.
.claude/skills/ats-build/SKILL.md:23
- These examples mix CMake presets with an explicit
-B <build_dir>. Presets typically definebinaryDir, and combining--presetwith-Bcan be confusing and may override preset behavior depending on CMake version. Prefer either (a) using presets without-Band documenting the preset-chosen build dir, or (b) providing non-preset commands that explicitly use<build_dir>.
cmake --preset dev -B <build_dir> -DENABLE_AUTEST=ON
.claude/skills/ats-build/SKILL.md:29
- These examples mix CMake presets with an explicit
-B <build_dir>. Presets typically definebinaryDir, and combining--presetwith-Bcan be confusing and may override preset behavior depending on CMake version. Prefer either (a) using presets without-Band documenting the preset-chosen build dir, or (b) providing non-preset commands that explicitly use<build_dir>.
cmake --preset ci-fedora-quiche -B <build_dir> -DENABLE_AUTEST=ON
.claude/skills/ats-build/SKILL.md:35
- These examples mix CMake presets with an explicit
-B <build_dir>. Presets typically definebinaryDir, and combining--presetwith-Bcan be confusing and may override preset behavior depending on CMake version. Prefer either (a) using presets without-Band documenting the preset-chosen build dir, or (b) providing non-preset commands that explicitly use<build_dir>.
cmake --preset release -B <build_dir>
.claude/skills/ats-build/SKILL.md:17
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
ref=$(git symbolic-ref --short -q HEAD || git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)
BUILD_DIR="build-${ref##*/}"
.claude/skills/ats-build/SKILL.md:23
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake --preset dev -B <build_dir> -DENABLE_AUTEST=ON
.claude/skills/ats-build/SKILL.md:41
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake --build <build_dir> --target <executable_or_library_target>
.claude/skills/ats-build/SKILL.md:47
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
ctest --test-dir <build_dir> [-R <filter>]
.claude/skills/ats-build/SKILL.md:53
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake --build <build_dir> --target format
.claude/skills/ats-build/SKILL.md:61
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
grep "^AUTEST_OPTIONS" <build_dir>/CMakeCache.txt
.claude/skills/ats-build/SKILL.md:67
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake --build <build_dir> --target autest
.claude/skills/ats-build/SKILL.md:73
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake -B <build_dir> -DAUTEST_OPTIONS="-f <autest_name>"
.claude/skills/ats-build/SKILL.md:81
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
grep "^CMAKE_INSTALL_PREFIX" <build_dir>/CMakeCache.txt
.claude/skills/ats-build/SKILL.md:87
- The skill defines
BUILD_DIR=...but the rest of the instructions use a separate<build_dir>placeholder. This makes the 'Determine build directory name' step harder to follow and easier to apply inconsistently. Consider standardizing on one convention (e.g., use$BUILD_DIRthroughout the examples, or remove theBUILD_DIRsnippet and keep<build_dir>everywhere).
cmake --install <build_dir>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.claude/skills/ats-build/SKILL.md:10
- The numbered list under “Build Process” skips step 2 (it goes 1, 3, 4), which reads like an editing error and is confusing for users following the sequence.
1. No build directory yet? Configure one.
3. Which target?
4. Execute.
.claude/CLAUDE.md:24
- This instruction assumes the
ats-buildskill is already available/loaded, but doesn’t indicate where its reference lives. Adding an explicit pointer makes the guidance actionable even when the harness doesn’t auto-load skills.
Use the ats-build skill for building, testing, and formatting, unless instructed otherwise.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.claude/skills/ats-build/SKILL.md:58
- The AuTest example placeholder is ambiguous; AuTest expects the test name without the
.test.pysuffix (as documented elsewhere in the repo). Clarifying this avoids users passing the wrong value.
cmake -B <build_dir> -DAUTEST_OPTIONS="-f <autest_name>"
.claude/skills/ats-build/SKILL.md:12
- The nested bullets under the ordered list item aren’t indented enough to reliably render as a sub-list in GitHub-flavored Markdown, which can break the intended step structure. Also, wording can be made explicit about selecting a preset.
This issue also appears on line 58 of the same file.
1. Which `cmake --list-presets`?
* Need HTTP3? `ci-fedora-quiche`
* Not sure? Ask.
2. No build directory yet? Configure one.
3. Which target?
.claude/CLAUDE.md:24
- This instruction now depends on a separate skill, but doesn’t tell readers where to find it. Adding an explicit path reference keeps CLAUDE.md usable even when skills aren’t auto-loaded.
Use the ats-build skill for building, testing, and formatting, unless instructed otherwise.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.claude/skills/ats-build/SKILL.md:20
cmake --presetusually implies the build directory via the preset; including an optional-B <build_dir>here can be confusing. Consider either showing the preset form alone, or adding a separate non-preset example that explicitly sets-S/-B.
cmake --preset <name> [-B <build_dir>] -DENABLE_AUTEST=ON
.claude/skills/ats-build/SKILL.md:58
- This configure example relies on being run from the source root (implicit source dir) and doesn’t enable autests, so
AUTEST_OPTIONSmay have no effect. Make the source directory explicit and include-DENABLE_AUTEST=ONto ensure the option is applied.
cmake -B <build_dir> -DAUTEST_OPTIONS="-f <autest_name>"
.claude/skills/ats-build/SKILL.md:10
- The first step reads like an incomplete question and may be unclear to follow. Consider phrasing it as selecting a preset from
cmake --list-presets, and use the standard “HTTP/3” spelling for consistency.
This issue also appears in the following locations of the same file:
- line 20
- line 58
1. Which `cmake --list-presets`?
* Need HTTP3? `ci-fedora-quiche`
* Not sure? Ask.
This is to clean up the CLAUDE.md file and hopefully save context. Most users will have their own custom build instructions, so the skill will not have to be loaded.