Skip to content

docs: add architectural guidelines for hooks, test coverage, and review process#77

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift-eng:mainfrom
brandisher:docs/contributing-architectural-guidelines
Apr 23, 2026
Merged

docs: add architectural guidelines for hooks, test coverage, and review process#77
openshift-merge-bot[bot] merged 2 commits intoopenshift-eng:mainfrom
brandisher:docs/contributing-architectural-guidelines

Conversation

@brandisher
Copy link
Copy Markdown
Contributor

@brandisher brandisher commented Apr 23, 2026

Summary

  • Adds general code standards for naming accuracy, reuse, test coverage of validation logic, and avoiding redundant runtime checks
  • Adds "Hooks: Repo-Level vs Plugin" and "Lifecycle Placement" subsections to clarify when protective checks should be repo-level hooks (mandatory) vs plugin hooks (optional)
  • Adds architectural fit criteria to the Review Process section so contributors can self-evaluate before submitting

Test plan

  • markdownlint passes with no errors
  • No overlap with plugins/docs/CONTRIBUTING.md
  • Review: guidelines are actionable and match existing doc style

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Expanded contribution guidelines: require component names to reflect behavior, mandate positive and negative tests for validation/regex/parsing, and advise documenting rarely-changing guard checks instead of repeating them at runtime.
    • Added guidance distinguishing repo-level vs plugin hooks and a lifecycle-placement matrix for where checks should run (one-time setup, session/tool phases, pre-commit).
    • Enhanced reviewer checklist to include naming, hook-scope, non-duplication, test coverage, and lifecycle placement.

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 <noreply@anthropic.com>
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 82dd927a-e19f-4b41-9ede-17e869b78902

📥 Commits

Reviewing files that changed from the base of the PR and between d36b1e4 and 56742fb.

📒 Files selected for processing (1)
  • CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CONTRIBUTING.md

Walkthrough

Updates CONTRIBUTING.md to add concrete general standards (component naming, validation test expectations, guard-check guidance), introduce repo-level vs plugin hook scope guidance, add a lifecycle-placement table mapping check types to mechanisms/stages, and expand the reviewer checklist with architectural fit criteria.

Changes

Cohort / File(s) Summary
Documentation
CONTRIBUTING.md
Expanded general contribution standards with: component-name must match behavior; validation/regex/parsing require positive and negative tests; avoid repeated runtime guard checks (document instead). Added hook-author guidance distinguishing repo-level vs plugin hooks and a lifecycle-placement matrix for where checks belong. Extended reviewer checklist to include naming, hook-scope, non-duplication, validation test coverage, and lifecycle-stage placement checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and clearly summarizes the main change: adding architectural guidelines for hooks, test coverage expectations, and review process enhancements to CONTRIBUTING.md.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@brandisher brandisher changed the title docs: add architectural guidelines from PR #74 and #75 reviews docs: add architectural guidelines for hooks, test coverage, and review process Apr 23, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@brandisher
Copy link
Copy Markdown
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 23, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 23, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brandisher, vimauro

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot Bot merged commit 094e6c8 into openshift-eng:main Apr 23, 2026
2 checks passed
@fonta-rh
Copy link
Copy Markdown
Contributor

Post-merge feedback: The lifecycle placement table (lines 122-127) documents PreToolUse/PostToolUse (no hooks registered) but omits Stop, which is the one non-SessionStart stage that actually has a hook (scripts/lint-markdown.sh). Consider adding a row:

| **Stop (session end)** | Markdown linting, cleanup tasks | Repo-level hook (`Stop`) |

openshift-merge-bot Bot pushed a commit that referenced this pull request Apr 23, 2026
The lifecycle table documented SessionStart and PreToolUse/PostToolUse
stages but omitted Stop, which has an active repo-level hook
(scripts/lint-markdown.sh) for markdown linting.

Addresses post-merge feedback on PR #77.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants