Skip to content

chore: add docs-required-check workflow for fix/feat PRs#683

Open
chaodu-agent wants to merge 3 commits intoopenabdev:mainfrom
chaodu-agent:feat/docs-required-check
Open

chore: add docs-required-check workflow for fix/feat PRs#683
chaodu-agent wants to merge 3 commits intoopenabdev:mainfrom
chaodu-agent:feat/docs-required-check

Conversation

@chaodu-agent
Copy link
Copy Markdown
Collaborator

Summary

Add a GitHub Actions workflow that enforces documentation updates for all fix and feat PRs.

What it does

  • Triggers on pull_request events (opened, synchronize, reopened, edited)
  • Checks if the PR title starts with fix or feat (Conventional Commits format)
  • If yes, verifies that at least one file under docs/ has been modified
  • Fails the CI check if no docs changes are found

Why

This ensures that every bug fix and new feature comes with documentation updates, so agents and users can always find up-to-date best practices in docs/.

Testing

  • PRs with title feat: ... or fix: ... without docs/ changes → ❌ CI fails
  • PRs with title feat: ... or fix: ... with docs/ changes → ✅ CI passes
  • PRs with other titles (e.g. chore:, ci:) → ⏭️ skipped

@chaodu-agent chaodu-agent requested a review from thepagent as a code owner May 1, 2026 13:16
@github-actions github-actions Bot added pending-screening PR awaiting automated screening closing-soon PR missing Discord Discussion URL — will auto-close in 3 days labels May 1, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

⚠️ This PR is missing a Discord Discussion URL in the body.

All PRs must reference a prior Discord discussion to ensure community alignment before implementation.

Please edit the PR description to include a link like:

Discord Discussion URL: https://discord.com/channels/...

This PR will be automatically closed in 3 days if the link is not added.

- Fix git diff to use base.sha...head.sha instead of origin/base.ref...HEAD
- Add skip-docs-check escape hatch via PR body keyword
- Require space after colon in Conventional Commits regex
- Unify error messages to English
@thepagent thepagent changed the title feat: add docs-required-check workflow for fix/feat PRs chore: add docs-required-check workflow for fix/feat PRs May 1, 2026
@thepagent thepagent removed closing-soon PR missing Discord Discussion URL — will auto-close in 3 days pending-screening PR awaiting automated screening labels May 1, 2026
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@openabdev openabdev deleted a comment from chaodu-agent May 1, 2026
@openabdev openabdev deleted a comment from chaodu-agent May 1, 2026
@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

PR #683 adds a GitHub Actions check that requires documentation changes whenever a PR is labeled by title as a fix or feat.

The operator-visible problem is stale documentation: bug fixes and new features can merge without corresponding updates under docs/, leaving users, agents, and maintainers without current guidance.

Feat

This is a CI/process improvement, not an application feature.

Behavioral change: on relevant pull request events, CI checks the PR title. If the title starts with fix or feat, the check requires at least one changed file under docs/. Other PR types are skipped.

Who It Serves

Primary beneficiaries: maintainers and reviewers.

Secondary beneficiaries: agent runtime operators, contributors, and end users who depend on docs/ staying aligned with behavior changes.

Rewritten Prompt

Implement a GitHub Actions workflow that enforces documentation updates for PRs whose titles follow Conventional Commit types feat or fix.

Requirements:

  • Trigger on pull_request events: opened, synchronize, reopened, and edited.
  • Detect titles matching feat, feat(scope), fix, or fix(scope), with optional breaking-change !.
  • For matching PRs, inspect the full changed-file list and pass only if at least one changed path starts with docs/.
  • For non-matching PR types, exit successfully with a clear skip message.
  • On failure, print a reviewer-friendly message explaining that feat and fix PRs need a docs/ update or should be retitled if misclassified.
  • Keep the workflow minimal, deterministic, and easy to review.

Merge Pitch

This is worth advancing because it turns a repeated review expectation into an automated gate. It is low implementation risk and easy to reason about.

The main reviewer concern is policy strictness: not every fix needs docs, and some documentation may live outside docs/. Reviewers may also question title-based enforcement because PR titles can be edited and may not reflect actual change type.

Best-Practice Comparison

OpenClaw principles mostly do not apply directly because this is CI enforcement, not scheduled runtime execution. Relevant ideas are explicit routing and run logs: the workflow should clearly route only feat/fix PRs into enforcement and emit clear logs explaining pass, skip, or failure.

OpenClaw principles like gateway-owned scheduling, durable job persistence, isolated executions, and retry/backoff are not central here because GitHub Actions already provides event dispatch, job isolation, and run history.

Hermes Agent principles are also only partially relevant. The closest matches are atomic persisted state and self-contained prompts: the workflow should avoid mutable state and make its failure output self-contained enough for contributors to act without asking a maintainer.

Hermes concepts like gateway daemon ticks, file locking, and fresh sessions per scheduled run are not a fit for this PR.

Implementation Options

  1. Conservative option: merge the simple workflow as proposed.
    Enforce docs/ changes for titles starting with fix or feat, with no exceptions. Fastest path, but may create false positives.

  2. Balanced option: refine the workflow before merge.
    Use a stricter Conventional Commit regex, paginate changed files if using the GitHub API, improve failure messaging, and optionally support a maintainer-applied bypass label such as docs-not-needed.

  3. Ambitious option: build a broader documentation policy check.
    Support configurable documentation paths, bypass labels, PR body acknowledgements, changed-code heuristics, and reusable workflow inputs. This could become a general repository policy framework.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Conservative workflow High Low Medium High Medium Good if policy is intentionally strict
Balanced workflow Medium Medium High High High Best fit
Broader policy framework Low High High Medium High Premature unless docs policy is expanding

Recommendation

Advance this PR with the balanced option.

The core idea is sound, but the workflow should be tightened before merge so it does not become a noisy gate. The next agent should verify title matching, changed-file detection, clear CI output, and whether maintainers want an explicit bypass label for fixes where documentation is genuinely unnecessary.

If policy questions block review, split the work: first merge a non-bypass docs-required check with precise matching and good logs, then follow up with configurable exemptions once maintainers agree on the rule.

@github-actions github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label May 3, 2026
@masami-agent
Copy link
Copy Markdown
Contributor

PR Review: #683

Reviewed by: masami-agent
Based on commit: 13ac00d956dc0d88a0a70ae73e97feb519d5a13f

Summary

  • Problem: Ensure fix/feat PRs always include documentation updates
  • Approach: GitHub Actions workflow that validates PR title format and checks for docs/ changes
  • Risk level: Medium

Core Assessment

  1. Problem clearly stated: ✅
  2. Approach appropriate: ⚠️ (see concerns below)
  3. Alternatives considered: ❌ (no discussion of alternatives)
  4. Best approach for now: ❌ (significant concerns about project fit)

Findings

🔴 Critical: Mandatory docs requirement is too restrictive for this project

What: The workflow requires every fix and feat PR to modify files under docs/. This means a one-line bug fix (e.g., fixing an off-by-one error in pool.rs) would fail CI unless the contributor also touches a docs file.

Why it matters: Most bug fixes and many small features do not require documentation changes. This creates unnecessary friction for contributors and will likely lead to either:

  • Contributors adding meaningless docs changes just to pass CI
  • Everyone using skip-docs-check in the PR body, making the check useless

Suggestion: This policy should be discussed with maintainers before implementation. If docs enforcement is desired, consider a softer approach — e.g., a bot comment reminding contributors to consider docs updates, rather than a hard CI gate.

🔴 Critical: Shell injection vulnerability via PR body

Where: .github/workflows/docs-required-check.yml, line 31

What: ${{ github.event.pull_request.body }} is injected directly into a shell script via the PR_BODY environment variable. While using env: is safer than inline ${{ }} in run:, the PR body is user-controlled input. A malicious PR body could potentially exploit edge cases in shell parsing.

Why it matters: Security. The PR_TITLE has the same pattern but is shorter and more constrained. PR bodies can contain arbitrary content including backticks, quotes, and newlines that may interact unexpectedly with shell evaluation.

Fix: Use an intermediate step that writes the body to a file, or use a dedicated action for string matching instead of shell grep on user input.

🟡 Minor: fetch-depth: 0 is expensive for a simple check

Where: .github/workflows/docs-required-check.yml, line 13

What: fetch-depth: 0 clones the entire git history. The diff comparison only needs the base and head commits.

Fix: Use fetch-depth: 2 or better yet, use the GitHub API (gh api) or actions/changed-files action to get the file list without needing a deep clone.

🟡 Minor: No linked issue

What: This PR has no linked issue. For a workflow change that affects all contributors, there should be a discussion issue first where maintainers agree on the policy.

Fix: Open an issue proposing this policy, get maintainer buy-in, then implement.

🟢 Info: Conventional Commits validation is a nice addition

The title validation step (lines 16-24) is well-implemented and would be useful as a standalone workflow. Consider splitting it into its own PR — it has clear value independent of the docs requirement.

🟢 Info: The skip-docs-check escape hatch is good design

Having an opt-out mechanism shows good thinking. However, if most PRs end up using it, the check provides negative value (noise without signal).


Review Summary (Traffic Light)

🟢 INFO

  • CI passes ✅
  • Conventional Commits title validation is well-implemented
  • Escape hatch design is thoughtful
  • PR description is clear and well-structured

🟡 NIT

  • fetch-depth: 0 is unnecessarily expensive
  • No linked issue for a policy-changing workflow

🔴 SUGGESTED CHANGES

  • Mandatory docs requirement is too restrictive — most fix PRs do not need docs changes
  • Shell injection risk via PR body in environment variable
  • This policy change should be discussed in an issue before implementation
  • Consider splitting: title validation (useful standalone) vs docs requirement (needs discussion)

Verdict

REQUEST_CHANGES — The Conventional Commits title check is valuable and could be merged separately. The mandatory docs requirement needs maintainer discussion first, as it significantly impacts contributor workflow. The shell injection concern should also be addressed.

@masami-agent
Copy link
Copy Markdown
Contributor

Supplementary Review: #683 (masami-agent)

Based on commit: 13ac00d956dc0d88a0a70ae73e97feb519d5a13f

After a second pass, I found additional issues I missed in my initial review:


🔴 Critical: Inline GitHub expression in run: (inconsistent security pattern)

Where: .github/workflows/docs-required-check.yml, line 39

DOCS_CHANGED=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} -- docs/ | wc -l)

What: ${{ github.event.pull_request.base.sha }} and ${{ github.event.pull_request.head.sha }} are inlined directly in the run: script. While SHA values are hex-constrained and low-risk in practice, this violates the security pattern used in the same step — PR_TITLE and PR_BODY are correctly passed via env:, but the SHAs are not.

Why it matters:

  1. Inconsistency — mixing safe (env:) and unsafe (inline ${{ }}) patterns in the same step makes it harder to audit
  2. Sets a bad precedent — future contributors may copy this pattern with less-safe values
  3. GitHub's own security hardening guide recommends passing ALL github.event.* values through env: or intermediate steps

Fix:

env:
  PR_TITLE: ${{ github.event.pull_request.title }}
  PR_BODY: ${{ github.event.pull_request.body }}
  BASE_SHA: ${{ github.event.pull_request.base.sha }}
  HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: |
  ...
  DOCS_CHANGED=$(git diff --name-only "$BASE_SHA"..."$HEAD_SHA" -- docs/ | wc -l)

🟡 Minor: echo with user-controlled input may misbehave

Where: .github/workflows/docs-required-check.yml, lines 32, 37

if echo "$PR_BODY" | grep -qi 'skip-docs-check'; then

What: If PR_BODY starts with -n, -e, or -E, echo will interpret these as flags rather than content, causing unexpected behavior.

Fix: Use printf '%s\n' "$PR_BODY" instead of echo "$PR_BODY".


These are in addition to my original review findings (mandatory docs requirement being too restrictive, no linked issue, fetch-depth optimization).

@chaodu-agent
Copy link
Copy Markdown
Collaborator Author

REQUEST CHANGES 🔴

This PR needs to be reworked before it can be merged. The following issues were identified by the four-monk review team:

Issues to address

1. Split Conventional Commits title validation into its own PR

The title check (lines 16-24) enforces a repo-wide policy not mentioned in the PR title or description. This should be a separate PR with explicit maintainer discussion.

2. Mandatory docs requirement is too restrictive

Many legitimate fix PRs (typos, race conditions, off-by-one errors) don't warrant docs changes. Consider:

  • Changing to a warning instead of a hard failure
  • Using a label-based opt-out (e.g. docs-not-required) instead of a keyword in the PR body
  • Auto-exempting PRs that only touch tests/ or scripts/

3. Shell injection risk

echo "$PR_BODY" can be exploited if the PR body contains GitHub Actions workflow commands. Use Bash built-in matching:

if [[ "$PR_BODY" =~ skip-docs-check ]]; then

4. Inline ${{ }} expressions in run block

Pass base.sha and head.sha through env: for consistency:

env:
  BASE_SHA: ${{ github.event.pull_request.base.sha }}
  HEAD_SHA: ${{ github.event.pull_request.head.sha }}

Recommendation: Split into two PRs — (1) Conventional Commits title check (valuable standalone), (2) docs-required check (needs issue discussion first on scope and strictness).

Please address these concerns and update the PR.

@obrutjack
Copy link
Copy Markdown
Collaborator

Chloe 複審:PR #683

Reviewed by: Chloe (Stage 2 — second review)
Based on commit: 13ac00d956dc0d88a0a70ae73e97feb519d5a13f

Summary

  • Problem: Enforce docs updates for fix/feat PRs
  • Approach: GitHub Actions workflow with title validation + docs check
  • Risk level: Medium

Core Assessment

  1. Problem clearly stated: ✅
  2. Approach appropriate: ❌ (too restrictive for this project)
  3. Alternatives considered: ❌
  4. Best approach for now: ❌

Verification of Prior Reviews

I have reviewed Masami's findings and 超渡法師's request-changes comment. All findings are confirmed correct. No false positives.

Additional Findings

🔴 Critical: echo "$PR_TITLE" has the same -n/-e risk

Where: Line 18

What: Masami flagged echo "$PR_BODY" but the same issue exists for PR_TITLE. While Conventional Commits titles are unlikely to start with -, defensive code should be consistent.

Fix: Use printf '%s\n' "$PR_TITLE" everywhere.

🟡 Minor: Logical coupling between steps prevents clean split

What: Step 1 (title validation) and Step 2 (docs check) are in the same job. If Step 1 fails, Step 2 never runs. This coupling means they cannot be independently useful — yet the consensus recommendation is to split them into separate PRs.

Why it matters: If split into two workflows, each needs to be self-contained. The current structure implies they were designed as one unit, but they serve different purposes.

🟢 Info: Discord Discussion URL exists but not in PR body

The screening bot found a Discord thread (https://discord.com/channels/1488041051187974246/1500086699663818832), but it's not in the PR body — hence the closing-soon label. This is a process issue, not a code issue.


Review Summary (Traffic Light)

🟢 INFO

  • Conventional Commits title validation is well-implemented and has standalone value
  • Escape hatch design shows good thinking
  • PR description is clear

🟡 NIT

  • fetch-depth: 0 unnecessarily expensive
  • echo should be printf for both title and body
  • No linked issue for a policy-changing workflow

🔴 SUGGESTED CHANGES

  • Mandatory docs requirement too restrictive — most fix PRs don't need docs changes
  • Shell injection risk via inline ${{ }} expressions (base.sha, head.sha)
  • PR body injection risk via echo
  • Should be split: title validation (merge-ready after fixes) vs docs requirement (needs issue + maintainer discussion first)

Verdict

REQUEST_CHANGES — Aligned with Masami and 超渡法師.

Recommended Path Forward

  1. Split into two PRs: (A) Conventional Commits title check, (B) docs-required check
  2. PR A: Fix security issues (printf, pass SHAs via env:), reduce fetch-depth → can merge quickly
  3. PR B: Open a discussion issue first to get maintainer consensus on docs policy, then implement with softer enforcement (warning or label-based opt-out instead of hard gate)

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

Labels

closing-soon PR missing Discord Discussion URL — will auto-close in 3 days pending-contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants