Skip to content

docs: require direct unit tests for new helper functions - #1842

Open
dheerajodha wants to merge 1 commit into
conforma:mainfrom
dheerajodha:EC-2098
Open

dheerajodha wants to merge 1 commit into
conforma:mainfrom
dheerajodha:EC-2098

Conversation

@dheerajodha

Copy link
Copy Markdown
Contributor

What:

Document that new public helper functions in policy/lib/ must have direct unit tests, not just indirect coverage through consuming rules. This reflects the existing convention enforced during code review but was not previously documented in AGENTS.md or .claude/rules/policy-rules.md.

Why:

This helps us to ensure accurate test coverage of our helper functions.

Tickets:

Resolves: EC-2098

Document that new public helper functions in policy/lib/ must have direct
unit tests, not just indirect coverage through consuming rules. This
reflects the existing convention enforced during code review but was not
previously documented in AGENTS.md or .claude/rules/policy-rules.md.

Resolves: EC-2098

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dheerajodha
dheerajodha requested a review from a team as a code owner September 8, 2026 12:22
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 5554ad9a-d627-4ad7-b316-432758cb968e

📥 Commits

Reviewing files that changed from the base of the PR and between 9534876 and 0e8b194.

📒 Files selected for processing (2)
  • .claude/rules/policy-rules.md
  • AGENTS.md

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The change updates policy and contributor guidance to require direct unit tests for new public helper functions, including branch, edge-case, and caller-independent contract coverage.

Changes

Testing guidance

Layer / File(s) Summary
Direct test requirements
.claude/rules/policy-rules.md, AGENTS.md
The guidance requires direct tests for shared-library functions and new public helpers. It also specifies branch, edge-case, and caller-independent contract coverage.

Priority: ⬇️ Low — Defer this documentation-only change because it adds contributor guidance for direct tests without altering exported behavior or runtime product functionality.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to 0e8b1

This updates guidance so new public policy helpers receive direct unit tests. It does not change product behavior and has no remaining merge-blocking risk.

Suggested reviewers: joejstuart

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely states the main documentation change: requiring direct unit tests for new helper functions.
Description check ✅ Passed The description includes all required sections. It explains what changed, why it changed, and links the related ticket EC-2098.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:23 PM UTC · Completed 12:32 PM UTC

Commit: cd01138 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-7 · Effort: high · Cost: $4.39

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unit-tests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 8, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Small, docs-only change (2 files, 8 lines added) touching 2 protected governance paths (.claude/ and AGENTS.md) with no code, dependency, or CI surface. Author is an established contributor; modest tier-2 multi-author contention on AGENTS.md pushes the score above the minimum but overall risk is low-to-moderate.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] .claude/rules/policy-rules.md, AGENTS.md — Both changed files are under paths in REVIEW_PROTECTED_PATHS (.claude/ and AGENTS.md). The PR provides sufficient context (linked Jira ticket EC-2098 and a clear rationale explaining that the change codifies an existing review-time convention), but protected-path changes always require explicit human approval regardless of context.
    Remediation: Human reviewer must confirm the governance/documentation update is intended before merge. No code change required.

Low

  • [missing-doc] .claude/skills/pr-checklist/SKILL.md — The PR checklist skill lists "100% test coverage on all changed .rego files" as a Before-Submitting item, but does not mention the newly-documented direct-unit-test requirement for helper functions in policy/lib/. Since the intent of this PR is to make the convention discoverable, the checklist is a natural peer location — but the change is deliberately scoped to AGENTS.md and .claude/rules/policy-rules.md, so this is a follow-up suggestion rather than a blocker.
    Remediation (follow-up): Consider adding a checklist item such as - [ ] New public helper functions in policy/lib/ have direct unit tests in the corresponding _test.rego file (not just indirect coverage through callers).

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 8, 2026

@simonbaird simonbaird left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I guess.

Ps, I though we were supposed to be instruct Fullsend to make these PRs automatically. Not sure exactly how, but I think you type /fs-fix in the upstream issue, or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment risk/moderate PR risk: moderate size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants