[NO-TICKET] Add hooks/skills foundation - #6147
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 32cdad8 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81b5f9ae34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "Edit|Write", |
There was a problem hiding this comment.
The hook only runs for Edit and Write, so RBS changes made through Bash bypass enforcement entirely. This includes the skill's own generation command in .claude/skills/write-rbs/SKILL.md:52-54, whose shell redirection creates an .rbs file, as well as sed -i, cp, or cat > sig/...; in those cases the skill need never be loaded despite the stated guarantee. Guard relevant Bash writes as well or otherwise prevent shell-based modifications from bypassing the check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Scoping the guard to Edit/Write and deliberately not Bash.
The guard exists to load the write-rbs skill before Claude hand-authors a signature. Shell tools (sed, cp, mv, tee) transform or copy existing bytes rather than author new types, so there is nothing for the skill to guide. Reliably detecting a write to a guarded path inside an arbitrary command line (redirects, heredocs, sed -i, interpolated paths) is not feasible, and matching Bash would fire the hook on every shell command. Scope is now documented in .claude/hooks/README.md.
| test: | ||
| @for t in $(HOOKS_DIR)*.test.rb; do echo "ruby $$t"; ruby "$$t" || exit 1; done |
There was a problem hiding this comment.
These tests are reachable only through this local Make target. A repository-wide search of the Rakefile and the checked GitHub/GitLab configurations finds no invocation of .claude/hooks, require-skill, or this Makefile, so normal CI can accept regressions in the denial logic while the new test suite remains unexecuted. Expose it through the authoritative Rake test structure and include it in an existing validation path.
AGENTS.md reference: AGENTS.md:L61-L66
Useful? React with 👍 / 👎.
| # Build and test the hooks in this directory | ||
| # | ||
| # make bootstrap Spinel if needed, compile every hook, run tests | ||
| # make test run every hook's tests (unit + smoke) under CRuby |
There was a problem hiding this comment.
This introduces a new build, bootstrap, compilation, and verification workflow for repository scripts without updating AGENTS.md or the specialized personas. Agents entering through the repository guidance therefore will not discover the required native re-verification after hook changes, contrary to the repository rule requiring agent guidance to evolve with scripts.
AGENTS.md reference: AGENTS.md:L127-L127
Useful? React with 👍 / 👎.
| ## Core rules | ||
|
|
||
| - MUST leave a blank line between every signature | ||
| - NEVER add a leading `::` to Ruby core classes — write `Hash`, not `::Hash` |
There was a problem hiding this comment.
The blanket prohibition on leading :: produces incorrect constant resolution whenever a Datadog namespace defines a core-named constant. For example, sig/datadog/core/logger.rbs:3 intentionally declares class Logger < ::Logger; following this rule changes it to the recursive Logger < Logger, which RBS rejects as a recursive ancestor. Preserve absolute qualification when it is needed to distinguish the top-level core or standard-library constant.
Useful? React with 👍 / 👎.
| - NEVER split one file across both an inline `# :` signature and a `sig/` `.rbs` — | ||
| pick one per file |
There was a problem hiding this comment.
The repository deliberately splits several profiling classes across inline Ruby signatures and standalone .rbs declarations for C-only methods: Steepfile:11-25 explicitly preserves this arrangement, and files such as sig/datadog/profiling/http_transport.rbs contain the native half. Applying this NEVER rule would force an agent to delete either checked inline types or declarations for methods that cannot live in the Ruby file, so document and preserve the native-extension exception.
Useful? React with 👍 / 👎.
| ```bash | ||
| bundle exec steep check sig/datadog/<path>.rbs |
There was a problem hiding this comment.
Steep's positional check filter identifies source files to type-check, as also documented in docs/StaticTypingGuide.md:13-16; passing the path under sig/ does not target the corresponding Ruby implementation. During iteration this command can therefore appear clean without checking whether lib/datadog/<path>.rb conforms to the edited signature. Use the mirrored Ruby source path for the targeted check.
Useful? React with 👍 / 👎.
|
|
||
| ```rbs | ||
| # Good | ||
| class Point < Struct[[Integer, Integer]] |
There was a problem hiding this comment.
Struct has a single Elem parameter used directly as the return type of #[] and the yielded type of #each; it does not interpret a tuple as per-member positions. Consequently this declaration models point[0] and every yielded value as [Integer, Integer], and to_a as Array[[Integer, Integer]], even though the runtime values are individual integers. Use Struct[Integer] here, or a union of the member value types for heterogeneous structs.
Useful? React with 👍 / 👎.
* Unify the language * Adjust syntax
ce6d945 to
bc2bfef
Compare
| # Hooks | ||
|
|
||
| Claude Code hooks for this repo. A hook is a small script Claude Code runs | ||
| around tool calls — here, one Ruby file per hook. |
There was a problem hiding this comment.
| around tool calls — here, one Ruby file per hook. | |
| around tool calls. |
There was a problem hiding this comment.
I don't agree, this is specifically against bulldozer extractions, rule is simple - 1 hook = 1 script
|
|
||
| ```ruby | ||
| # runs as a hook or a compiled binary, but not when required by the test | ||
| Runner.new(ARGV).run($stdin.read) unless $PROGRAM_NAME.end_with?(".test.rb") |
There was a problem hiding this comment.
This is general knowledge and does not need to be in the file.
| ``` | ||
|
|
||
| Each test file carries two suites. The **unit** suite exercises the hook's logic | ||
| in process. The **smoke** suite runs the hook end to end as a subprocess against |
There was a problem hiding this comment.
Smoke test should be in a separate file.
There was a problem hiding this comment.
Nope, is there any argument for it?
|
|
||
| ## Shims | ||
|
|
||
| `settings.json` never points at a Ruby file directly. It points at a shim in |
There was a problem hiding this comment.
it's a general knowledge of Claude
|
|
||
| `settings.json` never points at a Ruby file directly. It points at a shim in | ||
| `shims/`, which runs the compiled binary when one exists and falls back to plain | ||
| Ruby otherwise. That keeps the wiring stable whether or not a native build is |
There was a problem hiding this comment.
What is a native build and why is it present or not present?
There was a problem hiding this comment.
by default hooks shipped with no dependencies and they are ruby scripts, but for performance reasons you might want to compile them for a speedup. Rest is a general approach for it shim is either picking compiled or plain ruby
| everyone builds their own, and plain Ruby remains the portable fallback. | ||
|
|
||
| Spinel only supports a subset of Ruby, so mind the gaps. `Hash#dig`, for one, | ||
| is unsupported — reach for `Hash#fetch` instead. Because the binary is |
There was a problem hiding this comment.
Surely it is impractical to duplicate the diff between ruby and spinel into this readme file. My suggestion is to delete all ai slop related to it.
There was a problem hiding this comment.
This is just an example of disparity and reasoning for the smoke test. I can remove it for sure, or rephrase it, but I think the reasons for smoke tests presence are makes sense to mention
2 participants
What does this PR do?
Adds a new project scoped skill
/write-rbswith an enforcement via hooks system that will ensure that skill is loaded when Claude tries to create/modify the Ruby RBS filesMotivation:
This should add consistency and reduce review cycles as all the common mistakes are documented via LLM instructions. The output should be considerably better.
Change log entry
No. Internal change
Additional Notes:
Q: Why** hook and not a
rules/folder?A: Because rules doesn't invoke on created from scratch files, only on read/edit and because of that LLM could decide not to load skill when creating a file
Q: How is the hook better than
rules/?A: With hook we are scanning entire session for skill being loaded, covering create/edit operations and it will reject editing if it wasn't done, LLM will be required to load the skill otherwise it will be denied of editing/creating RBS.
Q: Why is it ruby?
A: This is a near zero-dependency setup, the only dependency that might be handy is Spinel, to compile hook and boost execution time (which includes VM boot) from 100-200ms to 4ms.
Q: Is it testable?
A: Yes, ruby is tested via TestUnit and compiled binary passes smoke tests where ruby script output is compared to binary output for the same input.
How to test the change?
/skillsto see the list, then look for project scope and find/write-rbs