From d36b1e492eabe840a9063d23c06c5cfea3d3a467 Mon Sep 17 00:00:00 2001 From: Chad Scribner Date: Thu, 23 Apr 2026 09:36:33 +0200 Subject: [PATCH 1/2] docs: add architectural guidelines from PR #74 and #75 reviews Codify recurring review feedback into CONTRIBUTING.md so contributors can self-evaluate before submitting: naming accuracy, hooks vs plugins for mandatory checks, lifecycle placement, reuse, and test coverage for validation logic. Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 797b2743..77ebb38e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -64,6 +64,10 @@ chore(submodule): update two-node-toolbox (abc1234 -> def5678, 5 commits) - No hardcoded credentials — use environment variables - Self-documenting code over comments - First-pass code review is automated by CodeRabbit on all PRs to `main` +- Name accuracy: component names (tools, plugins, hooks, scripts) must describe what the component actually does — not what it aspires to or tangentially relates to +- Reuse before creating: check [ai-helpers](https://github.com/openshift-eng/ai-helpers) and existing repo components before writing new implementations +- Test validation logic: regex patterns, parsing rules, and validation functions require tests with positive and negative cases +- Avoid redundant checks: don't repeatedly validate things that rarely change (e.g., config file existence) — one-time setup belongs in documentation, not runtime checks ## Adding a Tool @@ -91,6 +95,28 @@ Plugin components serve different roles. Choosing the right one matters. **Use a hook** when behavior should trigger automatically on an event (session start, before/after tool use). +### Hooks: Repo-Level vs Plugin + +Not all hooks belong in plugins. The delivery mechanism determines who gets the protection. + +| Mechanism | Location | Scope | Use when | +|-----------|----------|-------|----------| +| **Repo-level hook** | `.claude/settings.json` | All contributors automatically | Check is mandatory/protective (destructive command blocking, secret detection) | +| **Plugin hook** | `plugins//hooks/` | Only users who install the plugin | Check is optional/workflow-specific | + +**Rule of thumb**: if skipping the check would be a security or data-loss risk, it belongs in `.claude/hooks` at the repo level — not in an optional plugin. + +### Lifecycle Placement + +Place each check at the right stage: + +| Stage | Example | Mechanism | +|-------|---------|-----------| +| **One-time setup** | CodeRabbit config, tool installation | Documentation only — verify once, not every session | +| **Session start** | Submodule staleness, undocumented tools | Repo-level hook (`SessionStart`) | +| **Pre-tool / post-tool** | Block destructive git commands, detect secrets in file writes | Repo-level hook (`PreToolUse` / `PostToolUse`) | +| **Pre-commit** | Secret scanning, lint checks | Git pre-commit hook or repo-level `PreToolUse` on Bash matcher | + ### CLI Tools vs MCP Servers When adding external tool integration, decide whether to use a traditional CLI tool (invoked via Bash) or an MCP server. @@ -190,3 +216,9 @@ Plugins extend Claude Code capabilities for the team. For plugin contribution de - CodeRabbit provides automated review on PRs to `main` - `two-node-toolbox/` is excluded from CodeRabbit review (external submodule) - Reviewers check: code quality, security, documentation, and test coverage +- Reviewers also verify architectural fit: + - Component name matches actual behavior + - Mandatory checks use repo-level hooks, not optional plugins + - No duplication of existing components (check ai-helpers and this repo) + - Validation logic (regex, parsers) has test coverage + - Check is placed at the correct lifecycle stage From 56742fbb6c003facdb68ef9b3fbce2a5d5feafed Mon Sep 17 00:00:00 2001 From: Chad Scribner Date: Thu, 23 Apr 2026 09:44:28 +0200 Subject: [PATCH 2/2] docs: remove ai-helpers references from CONTRIBUTING.md Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 77ebb38e..87a2c544 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,7 +65,6 @@ chore(submodule): update two-node-toolbox (abc1234 -> def5678, 5 commits) - Self-documenting code over comments - First-pass code review is automated by CodeRabbit on all PRs to `main` - Name accuracy: component names (tools, plugins, hooks, scripts) must describe what the component actually does — not what it aspires to or tangentially relates to -- Reuse before creating: check [ai-helpers](https://github.com/openshift-eng/ai-helpers) and existing repo components before writing new implementations - Test validation logic: regex patterns, parsing rules, and validation functions require tests with positive and negative cases - Avoid redundant checks: don't repeatedly validate things that rarely change (e.g., config file existence) — one-time setup belongs in documentation, not runtime checks @@ -219,6 +218,6 @@ Plugins extend Claude Code capabilities for the team. For plugin contribution de - Reviewers also verify architectural fit: - Component name matches actual behavior - Mandatory checks use repo-level hooks, not optional plugins - - No duplication of existing components (check ai-helpers and this repo) + - No duplication of existing components in this repo - Validation logic (regex, parsers) has test coverage - Check is placed at the correct lifecycle stage