Skip to content

feat: support user-defined custom commands in PR welcome message#1133

Closed
rnetser wants to merge 34 commits into
mainfrom
feature/issue-1132-custom-commands-welcome
Closed

feat: support user-defined custom commands in PR welcome message#1133
rnetser wants to merge 34 commits into
mainfrom
feature/issue-1132-custom-commands-welcome

Conversation

@rnetser

@rnetser rnetser commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add a custom-commands configuration option (global and per-repo) that renders user-defined commands in a new Custom Commands section of the PR welcome message. These are documentation-only — the server displays them but does not process them, allowing external bots/tools to handle them independently.

Changes

File Change
webhook_server/config/schema.yaml Added custom-commands schema with name (pattern-constrained, ^[a-zA-Z0-9_-]+$) and description fields. DRY via $defs.custom-command-item referenced from both global and per-repo. minItems: 1 at global; per-repo allows empty list to disable. minLength: 1 on both name and description. maxItems: 50 on both arrays.
webhook_server/libs/github_api.py Added _validate_custom_commands() load-time validator (matching _validate_custom_check_runs pattern). Uses isinstance type check, fullmatch() for safe-name regex, built-in command collision check, and log prefix handling for empty prefix.
webhook_server/libs/handlers/pull_request_handler.py Added _prepare_custom_commands_welcome_section property and _escape_markdown() static method. Renders each command as * \/{name}` - {sanitized}. Markdown special chars and @` mentions escaped in descriptions.
webhook_server/utils/constants.py Added BUILTIN_COMMAND_NAMES frozenset using existing COMMAND_*_STR constants for collision check against built-in commands.
AGENTS.md Documented custom-commands feature: three resolution layers (repo-local, per-repo in config.yaml, global), override semantics (no list merge), handler details.
examples/config.yaml Added custom-commands example entries.
pyproject.toml Added jsonschema>=4.0.0 to tests dependency group.

Tests added

Test file Tests added Coverage
webhook_server/tests/test_config_schema.py 8 schema validation tests (valid global/per-repo, rejects missing name, invalid pattern, extra properties, empty name, empty description) Schema enforcement
webhook_server/tests/test_prepare_retest_welcome_comment.py 6 welcome rendering tests + 11 _validate_custom_commands tests (including builtin collision) Welcome section + load-time validation
webhook_server/tests/test_config.py 9 tests (Config edge cases: file-not-found race condition, permission error, generic exception, YAML error, label validation, repository local data) Config robustness
webhook_server/tests/test_push_handler.py 7 tests (PyPI title truncation, ctx step tracking, fail_step with traceback assertions) Push handler coverage
webhook_server/tests/test_ai_cli.py 2 tests (sidecar available/unavailable with assert_awaited_once_with) AI CLI async correctness
webhook_server/tests/test_pull_request_review_handler.py 1 test (ctx step tracking) Review handler coverage

Testing

  • All 1730 tests pass
  • 90.13% code coverage (above 90% threshold)
  • Pre-commit checks pass

Closes #1132

@myakove-bot

Copy link
Copy Markdown
Collaborator

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: Disabled for this repository
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message
  • /security-override - Set security check runs to pass (maintainers only)
  • /security-override cancel - Re-run security checks

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest python-module-install - Test Python package installation
  • /retest pre-commit - Run pre-commit hooks and checks
  • /retest conventional-title - Validate commit message format
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3
  • /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

  • /rebase - Rebase this PR branch onto its base branch

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 1 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove
  • rnetser

Reviewers:

  • myakove
  • rnetser
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
  • Test Oracle: Triggers: approved (claude/claude-opus-4-6[1m]); /test-oracle can be used anytime
Security Checks
  • Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
  • Committer Identity Check: Verifies last committer matches PR author
  • Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Support user-defined custom commands in PR welcome message
✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

Description

• Add custom-commands config to document repo-specific slash commands in PR welcome comment.
• Render a new “Custom Commands” section in the welcome message when configured.
• Extend schema validation, examples, and tests to cover formatting and placement.
Diagram

graph TD
  A["Repo / Global config"] --> B["schema.yaml"] --> C["config.get_value"] --> D["PullRequestHandler"] --> E["Welcome comment"]
  F["Unit tests"] --> D

  subgraph Legend
    direction LR
    _cfg["Config"] ~~~ _code["Runtime code"] ~~~ _test["Tests"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Generalize to configurable “custom sections”
  • ➕ Avoids adding more one-off sections in the future (custom-commands, custom-links, etc.).
  • ➕ Supports richer formatting (multiple section types) with a single config shape.
  • ➖ More complex schema and rendering logic for a single current use-case.
  • ➖ Harder to keep output consistent with existing curated sections.
2. Allow markdown snippet injection (trusted-only)
  • ➕ Maximum flexibility (tables, links, multi-line docs).
  • ➕ No need to constrain command names with patterns.
  • ➖ Higher risk of malformed markdown and inconsistent formatting.
  • ➖ Would need guardrails to avoid surprising/unreviewed content in comments.

Recommendation: Keep the PR’s current targeted custom-commands approach: it’s simple, validated (patterned name + required description), and explicitly documentation-only, with tests covering formatting and placement. If future feature requests add more custom content types, consider evolving toward a generic “custom sections” model rather than adding multiple bespoke arrays.

Files changed (4) +159 / -1

Enhancement (1) +18 / -0
pull_request_handler.pyRender Custom Commands section in PR welcome comment +18/-0

Render Custom Commands section in PR welcome comment

• Adds '_prepare_custom_commands_welcome_section' that reads 'custom-commands' from config and formats them as markdown bullets. Integrates the new section into '_prepare_welcome_comment()' before Label Management and omits it when not configured.

webhook_server/libs/handlers/pull_request_handler.py

Tests (1) +106 / -1
test_prepare_retest_welcome_comment.pyAdd tests for custom commands welcome section +106/-1

Add tests for custom commands welcome section

• Adds a dedicated test class validating absence when unconfigured, correct formatting for each command, header newline placement, and that the section appears before Label Management. Uses a config.get_value override to inject custom-commands for the handler under test.

webhook_server/tests/test_prepare_retest_welcome_comment.py

Documentation (1) +9 / -0
config.yamlDocument 'custom-commands' usage in example config +9/-0

Document 'custom-commands' usage in example config

• Adds a commented example showing how to define 'custom-commands' with 'name' and 'description', clarifying they are display-only and handled by external tooling.

examples/config.yaml

Other (1) +26 / -0
schema.yamlAdd schema for 'custom-commands' configuration +26/-0

Add schema for 'custom-commands' configuration

• Introduces a new 'custom-commands' array configuration option. Each item requires 'name' (validated by '^[a-zA-Z0-9_-]+$') and 'description', and disallows additional properties.

webhook_server/config/schema.yaml

@qodo-code-review

qodo-code-review Bot commented Jun 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

Grey Divider


Action required

1. Local overrides ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
_prepare_custom_commands_welcome_section calls Config.get_value("custom-commands") without
passing the repository-local config dict, so custom-commands configured in
.github-webhook-server.yaml will not render in the welcome comment. This contradicts the
documented precedence in Config.get_value and how GithubWebhook loads other repo-local settings,
leading to confusing/incorrect behavior for teams relying on repo-local config.
Code

webhook_server/libs/handlers/pull_request_handler.py[R855-857]

+        raw_commands: object = self.github_webhook.config.get_value("custom-commands", [])
+        if not isinstance(raw_commands, list) or not raw_commands:
+            return ""
Relevance

⭐⭐⭐ High

Team previously accepted fixing missing extra_dict so in-repo .github-webhook-server.yaml overrides
apply (security-checks).

PR-#1109

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The handler reads custom-commands directly from Config.get_value without repo-local context, but
Config.get_value only applies repo-local .github-webhook-server.yaml values when passed via
extra_dict. The system does fetch repo-local config during GithubWebhook initialization and uses
extra_dict to populate other settings, so custom-commands should be loaded similarly to be
effective.

webhook_server/libs/handlers/pull_request_handler.py[847-881]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[176-183]
webhook_server/libs/github_api.py[873-946]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PullRequestHandler._prepare_custom_commands_welcome_section` reads `custom-commands` via `self.github_webhook.config.get_value(...)` without the repository-local config context (`.github-webhook-server.yaml`). As a result, repo-local `custom-commands` are ignored and won’t appear in the PR welcome message.

## Issue Context
`Config.get_value` only consults repo-local config when it is provided via `extra_dict`. `GithubWebhook` already fetches `.github-webhook-server.yaml` and uses it (via `extra_dict`) to populate other per-repo settings inside `_repo_data_from_config`, but `custom-commands` is not loaded there.

## Fix Focus Areas
- webhook_server/libs/github_api.py[176-183]
- webhook_server/libs/github_api.py[873-946]
- webhook_server/libs/handlers/pull_request_handler.py[848-881]
- webhook_server/libs/config.py[132-153]

## Suggested fix
1. In `GithubWebhook._repo_data_from_config(...)`, load and type-guard `custom-commands` using `extra_dict=repository_config`, e.g.:
  - `raw = self.config.get_value(value="custom-commands", return_on_none=[], extra_dict=repository_config)`
  - `self.custom_commands = raw if isinstance(raw, list) else []`
  (Optionally validate each entry is a dict with `name`/`description` strings.)
2. Update `PullRequestHandler._prepare_custom_commands_welcome_section` to use `self.github_webhook.custom_commands` rather than calling `config.get_value(...)` directly.
3. Adjust tests to set `process_github_webhook.custom_commands` instead of monkeypatching `config.get_value` (optional but cleaner).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. custom-commands missing global schema ✓ Resolved 📎 Requirement gap ≡ Correctness
Description
custom-commands is only defined under per-repo repositories.* schema properties, so a global
custom-commands configuration will fail schema validation. This violates the requirement to
support both global config and per-repo override for custom-commands.
Code

webhook_server/config/schema.yaml[R719-744]

+        custom-commands:
+          type: array
+          description: |
+            Custom commands to display in the PR welcome message.
+            These are documentation-only - the server renders them in the welcome
+            message but does NOT process them. External bots or tools handle them.
+
+            Examples:
+              - name: deploy-staging
+                description: Deploy this PR to the staging environment
+              - name: run-e2e
+                description: Trigger end-to-end test suite against this PR
+          items:
+            type: object
+            properties:
+              name:
+                type: string
+                pattern: "^[a-zA-Z0-9_-]+$"
+                description: Command name (without the leading slash)
+              description:
+                type: string
+                description: Human-readable description of what the command does
+            required:
+              - name
+              - description
+            additionalProperties: false
Relevance

⭐⭐⭐ High

Team enforces global+repo override schema consistency (e.g., added per-repo support + tests for
options in PR #1112).

PR-#1112
PR-#980

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The checklist requires custom-commands to be supported in both global config and per-repo
overrides. The schema change adds custom-commands only within the repositories per-repo
properties block, with no corresponding root-level definition.

Add custom-commands option to configuration schema
Support both global config and per-repo override for custom-commands
webhook_server/config/schema.yaml[325-351]
webhook_server/config/schema.yaml[719-744]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`custom-commands` is currently only allowed under `repositories.*` in `schema.yaml`, but the feature requires support both globally and as a per-repo override.

## Issue Context
`Config.get_value()` already checks repository scope first and then root scope, so the primary gap is schema validation rejecting a root-level `custom-commands` key.

## Fix Focus Areas
- webhook_server/config/schema.yaml[719-744]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Missing custom-commands schema tests ✓ Resolved 📎 Requirement gap ☼ Reliability
Description
The PR adds a new custom-commands schema entry but does not add automated tests that validate
valid/invalid custom-commands configs against the schema expectations. This increases the risk of
regressions in schema enforcement.
Code

webhook_server/config/schema.yaml[R719-744]

+        custom-commands:
+          type: array
+          description: |
+            Custom commands to display in the PR welcome message.
+            These are documentation-only - the server renders them in the welcome
+            message but does NOT process them. External bots or tools handle them.
+
+            Examples:
+              - name: deploy-staging
+                description: Deploy this PR to the staging environment
+              - name: run-e2e
+                description: Trigger end-to-end test suite against this PR
+          items:
+            type: object
+            properties:
+              name:
+                type: string
+                pattern: "^[a-zA-Z0-9_-]+$"
+                description: Command name (without the leading slash)
+              description:
+                type: string
+                description: Human-readable description of what the command does
+            required:
+              - name
+              - description
+            additionalProperties: false
Relevance

⭐⭐⭐ High

Schema-option additions typically require schema validation tests; reviewers explicitly accepted
adding schema tests in PR #1112.

PR-#1112
PR-#1094

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 181413 requires schema validation tests for the new custom-commands option. The PR updates
schema.yaml to add custom-commands, and adds welcome-message rendering tests, but does not add
schema tests covering valid/invalid custom-commands configurations.

Add schema validation tests for custom-commands
webhook_server/config/schema.yaml[719-744]
webhook_server/tests/test_prepare_retest_welcome_comment.py[353-455]
webhook_server/tests/test_config_schema.py[350-359]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new `custom-commands` config schema was added, but there are no dedicated schema validation tests to ensure valid configurations are accepted and invalid ones are rejected.

## Issue Context
The repo already has schema-focused tests (e.g., asserting per-repo schema keys/types). Add coverage for `custom-commands` consistent with those patterns.

## Fix Focus Areas
- webhook_server/config/schema.yaml[719-744]
- webhook_server/tests/test_config_schema.py[350-359]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (2)
4. ghp_test123 missing allowlist ✓ Resolved 📘 Rule violation ⛨ Security
Description
A token-like literal (ghp_test123) was added to tests without the required `# pragma: allowlist
secret` annotation. This can trigger secret scanners and violates the repository’s rule for
explicitly annotating allowed test tokens.
Code

webhook_server/tests/test_config_schema.py[R1038-1045]

+        config: dict[str, Any] = {
+            "github-tokens": ["ghp_test123"],
+            "custom-commands": [
+                {"name": "deploy-staging", "description": "Deploy to staging"},
+                {"name": "run-e2e", "description": "Run e2e tests"},
+            ],
+        }
+        validate(instance=config, schema=schema)
Relevance

⭐⭐ Medium

Prior suggestion to use # pragma: allowlist secret for token-like test strings had undetermined
outcome (PR #1081).

PR-#1081

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 181401 disallows hardcoded secret-like strings unless they are explicitly annotated
as allowed test tokens using # pragma: allowlist secret. The test config includes ghp_test123
without the required annotation.

Rule 181401: Disallow hardcoded secrets; explicitly annotate allowed test tokens
webhook_server/tests/test_config_schema.py[1038-1045]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A GitHub-token-shaped test string is committed without the required allowlist annotation comment.

## Issue Context
The compliance rule requires any token-like literals used in tests/examples to be annotated on the same line with `# pragma: allowlist secret`.

## Fix Focus Areas
- webhook_server/tests/test_config_schema.py[1038-1045]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Custom commands render crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
_prepare_custom_commands_welcome_section directly indexes cmd['name'] and cmd['description'] from
the configured list; if custom-commands is not a list of dicts or any item is missing those keys,
welcome comment generation raises (TypeError/KeyError) and PR-open tasks won’t be scheduled. This
repo’s Config loader does not validate config.yaml against schema.yaml at runtime, so malformed
values can reach this code path.
Code

webhook_server/libs/handlers/pull_request_handler.py[R853-860]

+        custom_commands: list[dict[str, str]] = self.github_webhook.config.get_value("custom-commands", [])
+        if not custom_commands:
+            return ""
+
+        lines: list[str] = ["\n#### Custom Commands"]
+        for cmd in custom_commands:
+            lines.append(f"* `/{cmd['name']}` - {cmd['description']}")
+
Relevance

⭐⭐ Medium

Mixed history: config hardening accepted (PR #978) but some KeyError-hardening suggestions rejected
(PR #1002).

PR-#978
PR-#1002
PR-#980

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new custom-commands rendering indexes dict keys without validation, and the PR-open path builds
the welcome message before creating/scheduling async tasks; any exception prevents those tasks from
being scheduled. Since Config initialization does not perform schema validation, malformed
configuration structures can reach this new code.

webhook_server/libs/handlers/pull_request_handler.py[846-861]
webhook_server/libs/handlers/pull_request_handler.py[235-266]
webhook_server/libs/config.py[13-26]
PR-#978

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PullRequestHandler._prepare_custom_commands_welcome_section` assumes `custom-commands` is a `list[dict]` with required `name`/`description` keys and indexes them directly. A mis-typed config (e.g., string/object instead of list, or list items missing keys) will raise and prevent `process_pull_request_webhook_data()` from building/scheduling PR-open tasks.

### Issue Context
Runtime config loading does not validate config structure against `config/schema.yaml`, so schema constraints are not sufficient to prevent malformed values from reaching handler code.

### Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[846-861]
- webhook_server/libs/handlers/pull_request_handler.py[235-266]
- webhook_server/libs/config.py[13-26]

### Implementation notes
- Defensive checks:
 - If `custom_commands` is not a `list`, treat it as empty.
 - For each entry, require `isinstance(cmd, dict)` and `isinstance(name, str)`/`isinstance(description, str)` and both non-empty.
 - Use `cmd.get("name")` / `cmd.get("description")` and skip invalid entries (optionally log debug/warning once).
- Ensure the section header is only emitted if at least one valid command remains.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

6. Missing built-in command names ✓ Resolved 🐞 Bug ≡ Correctness
Description
BUILTIN_COMMAND_NAMES is missing several slash commands that the server already processes (e.g.,
reprocess, security-override, add-allowed-user, regenerate-welcome, plus label-commands like
hold/automerge), so _validate_custom_commands() will incorrectly allow those names as “custom
commands”. This undermines the collision-prevention behavior and can mislead users by listing
server-handled commands under the “Custom Commands” section with arbitrary descriptions.
Code

webhook_server/utils/constants.py[R152-167]

+# Built-in command names that cannot be overridden by custom commands
+BUILTIN_COMMAND_NAMES: frozenset[str] = frozenset({
+    "lgtm",
+    "approve",
+    "assign-reviewers",
+    "assign-reviewer",
+    "check-can-merge",
+    "build-and-push-container",
+    "cherry-pick",
+    "cherry-pick-retry",
+    "rebase",
+    "wip",
+    "verified",
+    "retest",
+    "test-oracle",
+})
Relevance

⭐⭐⭐ High

Team regularly updates command constants when adding commands (/reprocess, /rebase, /automerge);
collision list completeness aligns.

PR-#894
PR-#1108
PR-#812

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The server’s built-in command surface includes multiple commands beyond those listed in
BUILTIN_COMMAND_NAMES, and custom-command validation only blocks names present in that set.
Therefore, any omitted built-in command name will not be blocked and can be accepted as a “custom
command”.

webhook_server/utils/constants.py[23-64]
webhook_server/utils/constants.py[152-167]
webhook_server/libs/handlers/issue_comment_handler.py[161-178]
webhook_server/libs/github_api.py[1656-1659]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GithubWebhook._validate_custom_commands()` rejects custom commands when `name.lower()` collides with `BUILTIN_COMMAND_NAMES`, but the current `BUILTIN_COMMAND_NAMES` set is incomplete. As a result, configs like:

```yaml
custom-commands:
 - name: reprocess
   description: ...
```

will incorrectly validate and render, even though `/reprocess` is a server-handled command.

### Issue Context
The server already recognizes additional commands via `IssueCommentHandler.user_commands()` and also treats several labels as commands (e.g. `/hold`, `/automerge`). Those should be treated as “built-in” for collision purposes.

### Fix Focus Areas
- webhook_server/utils/constants.py[152-167]
- webhook_server/libs/github_api.py[1656-1659]
- webhook_server/tests/test_prepare_retest_welcome_comment.py[595-605]

### Suggested fix
1. Update `BUILTIN_COMMAND_NAMES` to include all server-handled slash commands and label-commands, including at least:
  - `reprocess`, `security-override`, `add-allowed-user`, `regenerate-welcome`, `automerge`, `hold`
2. Prefer deriving this set from existing constants (e.g., `COMMAND_*_STR`, `BUILD_AND_PUSH_CONTAINER_STR`, and `USER_LABELS_DICT.keys()`) to prevent drift.
3. Extend the existing builtin-collision test to include at least one of the previously-missing names (e.g., `reprocess` and/or `security-override`) so regressions are caught.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Trailing-newline name bypass ✗ Dismissed 🐞 Bug ⛨ Security
Description
_validate_custom_commands() uses safe_name_pattern.match(name) with a ^...$ pattern, which can
accept a command name like "deploy\n" (because $ may match before a final newline), letting an
invalid name pass validation. That invalid name can then inject newlines into the “Loaded … custom
command(s)” log line and into the welcome comment where name is rendered verbatim.
Code

webhook_server/libs/github_api.py[R1647-1648]

+            if not safe_name_pattern.match(name):
+                self.logger.warning(f"{prefix} Custom command name {name!r} does not match safe pattern, skipping")
Relevance

⭐⭐⭐ High

Team accepted regex tightening to block tabs/newlines in validation (PR #900); similar
regex-anchoring hardening accepted (PR #1073).

PR-#900
PR-#1073

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The validator compiles ^[a-zA-Z0-9_-]+$ and checks it with .match(name), which is vulnerable to
the trailing-newline $ behavior; the schema indicates the intention is to allow only those
characters, and the PR welcome comment renders name directly so any newline that slips through
affects output.

webhook_server/libs/github_api.py[1626-1649]
webhook_server/config/schema.yaml[71-79]
webhook_server/libs/handlers/pull_request_handler.py[875-881]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_validate_custom_commands()` validates `name` with `safe_name_pattern.match(name)` where the pattern is `^[a-zA-Z0-9_-]+$`. In Python regex, `$` can match before a final `\n`, so a value like `"deploy\n"` can incorrectly be treated as valid.

## Issue Context
This validator is a defense-in-depth layer to keep `custom-commands[].name` aligned with the schema’s intent (safe characters only). The name is later used in logs and rendered directly into the PR welcome comment.

## Fix
- Replace `safe_name_pattern.match(name)` with `safe_name_pattern.fullmatch(name)`.
 - Alternatively, change the pattern to use `\Z` instead of `$` (and keep `match()`), but `fullmatch()` is the clearest.
- (Optional hardening) `name = name.strip()` before validation, or explicitly reject any leading/trailing whitespace.

## Fix Focus Areas
- webhook_server/libs/github_api.py[1626-1648]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Length limits not enforced ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
CLAUDE.md claims _validate_custom_commands() enforces max lengths for name (100) and
description (500), but the validator does not check lengths at all. Since config loading does not
apply schema.yaml at runtime, overlong values can still be accepted and rendered/logged,
contradicting the documented/schema constraints.
Code

CLAUDE.md[706]

+**Validation:** `GithubWebhook._validate_custom_commands()` in `webhook_server/libs/github_api.py` — validates at load time (entries must be dicts with non-empty `name` (max 100 chars) matching `^[a-zA-Z0-9_-]+$` and non-empty `description` (max 500 chars); duplicate names are rejected, keeping only the first occurrence). Invalid and duplicate entries are logged and skipped.
Relevance

⭐⭐⭐ High

Team previously accepted fixing contradictory CLAUDE.md guidance (PR #930); likely to correct
inaccurate validation claims.

PR-#930

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The docs claim max-length validation, the schema encodes those max lengths, but the runtime
validator lacks any length checks and config loading does not perform schema validation—so the
docs/schema constraints aren’t enforced at runtime.

CLAUDE.md[698-710]
webhook_server/config/schema.yaml[71-84]
webhook_server/libs/github_api.py[1604-1656]
webhook_server/libs/config.py[66-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CLAUDE.md` documents that `GithubWebhook._validate_custom_commands()` enforces `name`/`description` max lengths, but the current validator only enforces non-empty strings, a name regex, and uniqueness. Because `Config.root_data` simply `yaml.safe_load`s without runtime JSONSchema validation, the documented max-length limits are not actually enforced at runtime.

### Issue Context
- `schema.yaml` defines `custom-command-item.name.maxLength: 100` and `custom-command-item.description.maxLength: 500`.
- The runtime validator `_validate_custom_commands()` does not check `len(name)` / `len(description)`.
- The docs explicitly claim those limits are validated at load time.

### Fix Focus Areas
- CLAUDE.md[704-706]
- webhook_server/config/schema.yaml[71-84]
- webhook_server/libs/github_api.py[1604-1656]
- webhook_server/libs/config.py[66-83]

### Suggested fix
Choose one (preferred: implement runtime enforcement to match schema + docs):
1) Add length checks in `GithubWebhook._validate_custom_commands()`:
  - if `len(name) > 100`: warn and skip
  - if `len(description) > 500`: warn and skip
  (Optionally define constants to keep schema/validator aligned.)
2) Or, if you intentionally only enforce lengths via schema tests, update `CLAUDE.md` to remove the “max 100/500 chars” claim for load-time validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (9)
9. Docs allow empty global list ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
CLAUDE.md states that custom-commands: [] at “any layer” disables lower layers, but the root-level
custom-commands schema requires minItems: 1, so an empty list in config.yaml root is
schema-invalid. This can cause confusion when users try to “disable” global custom commands via an
empty root list and hit schema validation failures.
Code

CLAUDE.md[704]

+**Config:** Three resolution layers (first match wins, no list merge): (1) repo-local `.github-webhook-server.yaml`, (2) `repositories.<repo>.custom-commands` in `config.yaml`, (3) root-level `custom-commands` in `config.yaml`. An empty list (`custom-commands: []`) at any layer disables lower layers.
Relevance

⭐⭐⭐ High

Team fixes doc/schema contradictions (accepted CLAUDE.md correction in PR #930); likely align
empty-list behavior/docs/schema.

PR-#930
PR-#1112

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The documentation says an empty custom-commands list works “at any layer”, but the schema sets
minItems: 1 for the root-level custom-commands, which makes custom-commands: [] invalid in
config.yaml root.

CLAUDE.md[704-704]
webhook_server/config/schema.yaml[272-280]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CLAUDE.md` claims an empty list (`custom-commands: []`) works at any configuration layer, but the schema disallows an empty list at the **root** (global) layer (`minItems: 1`). This is a docs/schema mismatch that may mislead operators.

### Issue Context
- Root-level `custom-commands` in `config.yaml` is constrained by schema (`minItems: 1`).
- Per-repo override explicitly allows an empty list to disable inherited/global commands.

### Fix Focus Areas
- CLAUDE.md[704-704]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Docs omit repo layer ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
CLAUDE.md says custom-commands is configured only in root config.yaml or repo-local
.github-webhook-server.yaml, but the runtime resolution order also supports
repositories.<repo>.custom-commands in config.yaml. This can mislead operators into putting repo
overrides in the wrong place or assuming server-side per-repo overrides aren’t supported.
Code

CLAUDE.md[704]

+**Config:** Global (`config.yaml`) or per-repo (`.github-webhook-server.yaml`). Per-repo overrides global (no list merge); an empty list (`custom-commands: []`) disables global defaults.
Relevance

⭐⭐⭐ High

Team fixes misleading config docs; accepted correcting per-repo override claims and CLAUDE.md
contradictions (PRs #1109, #930).

PR-#1109
PR-#930
PR-#1112

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The docs line claims only global config.yaml or repo-local .github-webhook-server.yaml, but
Config.get_value() explicitly checks extra_dict (repo-local), then repository_data
(repositories.<repo>), then root_data (global). GithubWebhook._repo_data_from_config() loads
custom-commands through get_value(..., extra_dict=repository_config), so
repositories.<repo>.custom-commands is honored when extra_dict has no value.

CLAUDE.md[702-708]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[938-947]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Custom Commands docs imply there are only two configuration locations (global `config.yaml` and repo-local `.github-webhook-server.yaml`), but the code supports three layers (repo-local file, `config.yaml` `repositories.<repo>`, then root `config.yaml`).

### Issue Context
`Config.get_value()` resolves `extra_dict` first, then `repository_data`, then `root_data`, and `GithubWebhook._repo_data_from_config()` loads `custom-commands` using `extra_dict=repository_config`. This means `repositories.<repo>.custom-commands` in `config.yaml` is a valid per-repo override path and should be documented alongside `.github-webhook-server.yaml`.

### Fix Focus Areas
- CLAUDE.md[704-704]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


11. Docs claim config merging ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
CLAUDE.md says per-repo custom-commands “merges with global”, but Config.get_value() returns the
repo-local value directly when present (no list merge), so repo config replaces the global list.
This can lead to incorrect expectations and misconfiguration when users intend to “add” commands on
top of global defaults.
Code

CLAUDE.md[704]

+**Config:** Global (`config.yaml`) or per-repo (`.github-webhook-server.yaml`). Per-repo merges with global; an empty list (`custom-commands: []`) disables global defaults.
Relevance

⭐⭐⭐ High

Team favors docs matching runtime; merge/extend semantics proposal was rejected in PR #1109,
implying override is expected.

PR-#1109
PR-#978

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The documentation claims merge semantics, but Config.get_value() returns extra_dict’s nested
value if it is present (even an empty list), and GithubWebhook._repo_data_from_config() uses that
mechanism to load custom-commands, which results in override/replacement rather than merging.

CLAUDE.md[698-711]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[944-948]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CLAUDE.md` states that per-repo `custom-commands` “merges with global”, but the config resolution path uses `Config.get_value(..., extra_dict=repository_config)` which implements override semantics (repo value wins) and does not merge arrays.

## Issue Context
This is documentation-only, but it affects how users configure multi-repo deployments: they may expect global commands to be automatically included when defining repo-specific commands.

## Fix Focus Areas
- CLAUDE.md[698-711]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


12. Async test misses await ✓ Resolved 🐞 Bug ☼ Reliability
Description
test_call_ai_sidecar_available asserts call_ai_once was called, but not that it was awaited, so
the test can still pass if call_ai() forgets to await the coroutine. This weakens regression
coverage for a common async bug class.
Code

webhook_server/tests/test_ai_cli.py[R62-86]

+            with patch(
+                "webhook_server.libs.ai_cli.call_ai_once",
+                new_callable=AsyncMock,
+                return_value=expected,
+            ) as mock_call:
+                result = await call_ai(
+                    prompt="test",
+                    ai_provider="claude",
+                    ai_model="sonnet",
+                    cwd="/tmp",
+                    timeout_minutes=5,
+                    system_prompt="be helpful",
+                )
+                assert result.success is True
+                assert result.text == "hello"
+                mock_call.assert_called_once_with(
+                    prompt="test",
+                    ai_provider="claude",
+                    ai_model="sonnet",
+                    cwd="/tmp",
+                    ai_call_timeout=5,
+                    system_prompt="be helpful",
+                    tools=None,
+                    custom_tools=None,
+                )
Relevance

⭐⭐⭐ High

Team recently accepted switching AsyncMock assertions to assert_awaited* to catch missing-await
regressions (PR #1117).

PR-#1117

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test currently uses assert_called_once_with on an AsyncMock, which does not verify the awaited
behavior, while the implementation explicitly awaits call_ai_once.

webhook_server/tests/test_ai_cli.py[53-86]
webhook_server/libs/ai_cli.py[10-38]
PR-#1117

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`call_ai_once` is patched with `AsyncMock`, but the test asserts `assert_called_once_with(...)` instead of `assert_awaited_once_with(...)`. This allows the test to pass even if the production code stops awaiting the coroutine.

### Issue Context
`call_ai()` currently does `return await call_ai_once(...)`, so the test should enforce the await behavior.

### Fix Focus Areas
- webhook_server/tests/test_ai_cli.py[62-86]

### Suggested change
Replace:
- `mock_call.assert_called_once_with(...)`

With:
- `mock_call.assert_awaited_once_with(...)`

(Optional) Also assert `check_sidecar_available` was awaited in both tests if you keep a handle to the patched mock.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


13. Empty descriptions validate ✓ Resolved 🐞 Bug ≡ Correctness
Description
custom-command-item.description is only typed as a string in the schema, so description: ""
passes validation, but _prepare_custom_commands_welcome_section skips entries when description
is empty/falsy. This creates a confusing state where schema-valid configuration silently fails to
render in the PR welcome message.
Code

webhook_server/config/schema.yaml[R78-83]

+      description:
+        type: string
+        description: Human-readable description of what the command does
+    required:
+      - name
+      - description
Relevance

⭐⭐⭐ High

Team often tightens validation to reject empty strings/non-empty requirements (accepted in PR #1117;
similar non-empty enforcement in PR #900).

PR-#1117
PR-#900

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The schema defines description as a plain string with no minLength, while the renderer
explicitly rejects empty/falsy description values (... or not description), so schema-valid
empty descriptions will be dropped at runtime.

webhook_server/config/schema.yaml[71-84]
webhook_server/libs/handlers/pull_request_handler.py[855-869]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The schema allows `custom-commands` entries with an empty string `description`, but the welcome-comment renderer skips commands when `description` is empty/falsy. This leads to schema-valid config that won’t render.

## Issue Context
Runtime rendering explicitly requires `description` to be a non-empty string, so schema should reject empty (and ideally whitespace-only) descriptions to match runtime behavior.

## Fix Focus Areas
- webhook_server/config/schema.yaml[71-84]
- webhook_server/libs/handlers/pull_request_handler.py[855-869]

## Suggested fix
- In `custom-command-item.properties.description`, add `minLength: 1` (and optionally a pattern like `.*\S.*` to reject whitespace-only strings).
- Add/extend a schema test to assert `{"name": "deploy", "description": ""}` fails validation.
- (Optional) Consider `strip()`-based emptiness check in the renderer if you want whitespace-only descriptions treated as missing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


14. Silent custom-commands fallback ✓ Resolved 🐞 Bug ◔ Observability
Description
GithubWebhook._repo_data_from_config silently coerces a non-list custom-commands value to [],
which disables the Custom Commands section without any warning. Since runtime config loading does
not validate schema.yaml, this makes production misconfigurations difficult to detect and debug.
Code

webhook_server/libs/github_api.py[R944-949]

+        raw_custom_commands = self.config.get_value(
+            value="custom-commands", return_on_none=[], extra_dict=repository_config
+        )
+        self.custom_commands: list[dict[str, str]] = (
+            raw_custom_commands if isinstance(raw_custom_commands, list) else []
+        )
Relevance

⭐⭐⭐ High

Team usually logs warnings for mis-typed config instead of silent fallback; e.g., trusted-committers
type warning merged.

PR-#1117
PR-#978
PR-#998

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new code assigns custom_commands to an empty list when the configured value is not a list and
does not log; the welcome section is then omitted when the list is empty, masking the
misconfiguration. The codebase already logs this kind of mis-typed config for custom-check-runs,
and the runtime Config class does not perform schema validation (only labels validation), so
schema.yaml tests won’t prevent production misconfigs.

webhook_server/libs/github_api.py[944-949]
webhook_server/libs/handlers/pull_request_handler.py[855-860]
webhook_server/libs/github_api.py[1497-1505]
webhook_server/libs/config.py[13-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`custom-commands` is read from config and coerced to `[]` when it is not a list, but this happens silently (no warning). Because the runtime config loader doesn’t validate `schema.yaml`, a mis-typed YAML value (string/object) can reach this code path and the feature will appear to “not work” with no diagnostic signal.

## Issue Context
There is an existing pattern for configuration hardening in this file: `_validate_custom_check_runs(...)` logs a warning and skips invalid config rather than failing silently.

## Fix Focus Areas
- webhook_server/libs/github_api.py[944-949]

### Suggested implementation approach
- Add a small validator for `custom-commands` similar to `_validate_custom_check_runs`:
 - If `raw_custom_commands` is not a list, log a warning including the actual type and fall back to `[]`.
 - (Optional) Filter list elements to dicts only (or leave that to `PullRequestHandler`, but at least warn once if any element is not a dict).
- Use `getattr(self, "log_prefix", "")` for early-init safe log prefix, as done in `_validate_custom_check_runs`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


15. Cannot clear global commands ✓ Resolved 🐞 Bug ≡ Correctness
Description
The schema enforces minItems: 1 for per-repo custom-commands, so a repository cannot set
custom-commands: [] to override and disable a global custom-commands default. This conflicts
with the config resolution order (repo scope overrides root) and the handler behavior that treats an
empty list as “no section.”
Code

webhook_server/config/schema.yaml[R740-743]

+        custom-commands:
+          type: array
+          minItems: 1
+          description: |
Relevance

⭐⭐⭐ High

Team previously fixed schema blocking supported per-repo overrides; aligns schema with runtime
precedence (PR #1112).

PR-#1112
PR-#1109
PR-#978

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The schema prohibits empty per-repo lists, but the runtime config resolution and welcome rendering
both support empty-list semantics as an explicit override to produce no section; that means the
schema blocks a supported configuration behavior.

webhook_server/config/schema.yaml[740-766]
webhook_server/libs/config.py[132-153]
webhook_server/libs/handlers/pull_request_handler.py[855-857]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Per-repo `custom-commands` is defined with `minItems: 1`, which prevents setting `custom-commands: []` at repository scope. Because config resolution prefers repo-scoped values over root values, an explicit empty list is the natural way to disable a global default for one repo.

Currently:
- `Config.get_value(...)` returns the repository-scoped value if present, even if it's an empty list.
- The welcome-section renderer treats an empty list as "do not render".
- But the schema rejects `[]`, making this override pattern impossible in schema-valid configs.

## Issue Context
This is specifically about *per-repo overrides* when a global default is configured.

## Fix Focus Areas
- webhook_server/config/schema.yaml[740-743]

## Suggested fix
Adjust the per-repo schema for `custom-commands` to allow an explicit empty list override, for example:
- Remove `minItems: 1` at the repository level (keep it at global/root if you still want to require non-empty when globally configured), or
- Set `minItems: 0` at the repository level, or
- Use a `oneOf` that allows either an empty array or a non-empty array of valid objects.

After the schema change, consider adding a small schema test that validates `custom-commands: []` under a repository config when a global default exists (if your schema test harness supports such combined instances).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


16. custom-commands allows empty list ✓ Resolved 📎 Requirement gap ≡ Correctness
Description
The new global custom-commands schema does not enforce a non-empty list, so an explicitly empty
configuration will still validate even though the compliance requirement calls for validating a
non-empty list of command objects. This can weaken schema enforcement and allow misconfiguration to
pass unnoticed.
Code

webhook_server/config/schema.yaml[R256-275]

+  custom-commands:
+    type: array
+    description: |
+      Custom commands to display in the PR welcome message (global default).
+      These are documentation-only - the server renders them in the welcome
+      message but does NOT process them. External bots or tools handle them.
+    items:
+      type: object
+      properties:
+        name:
+          type: string
+          pattern: "^[a-zA-Z0-9_-]+$"
+          description: Command name (without the leading slash)
+        description:
+          type: string
+          description: Human-readable description of what the command does
+      required:
+        - name
+        - description
+      additionalProperties: false
Relevance

⭐⭐ Medium

No historical evidence enforcing minItems in schema arrays; schema feedback often rejected as
docs/examples (PR #961).

PR-#961

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 181406 requires the schema to validate a non-empty list of objects for
custom-commands. In the new global schema definition, type: array is present but there is no
minItems: 1, so custom-commands: [] would validate.

Add custom-commands configuration option to schema (global + per-repo override)
webhook_server/config/schema.yaml[256-275]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Global `custom-commands` schema allows an empty list, but the co...

Comment thread webhook_server/config/schema.yaml Outdated
Comment thread webhook_server/config/schema.yaml Outdated
Comment thread webhook_server/libs/handlers/pull_request_handler.py Outdated
@rnetser

rnetser commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/config/schema.yaml:719 (qodo requirement gap) — custom-commands missing global schema

Addressed: Fixed in 51b41f1 — added custom-commands to root-level schema properties, enabling both global config and per-repo override. Schema validation tests confirm both levels.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — Missing custom-commands schema tests

Addressed: Fixed in 51b41f1 — added TestCustomCommandsSchema class with 6 schema validation tests in test_config_schema.py.

webhook_server/libs/handlers/pull_request_handler.py:853 (qodo bug) — Custom commands render crash

Addressed: Fixed in 51b41f1 — _prepare_custom_commands_welcome_section now validates: raw_commands is a list, each entry is a dict, name/description are non-empty strings. Invalid entries are skipped with warning logs.

@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/config/schema.yaml:719 (qodo requirement gap) — custom-commands missing global schema

Addressed: Fixed in 51b41f1 — added custom-commands to root-level schema properties, enabling both global config and per-repo override. Schema validation tests confirm both levels.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — Missing custom-commands schema tests

Addressed: Fixed in 51b41f1 — added TestCustomCommandsSchema class with 6 schema validation tests in test_config_schema.py.

webhook_server/libs/handlers/pull_request_handler.py:853 (qodo bug) — Custom commands render crash

Addressed: Fixed in 51b41f1 — _prepare_custom_commands_welcome_section now validates: raw_commands is a list, each entry is a dict, name/description are non-empty strings. Invalid entries are skipped with warning logs.

Looks good — the PR now covers the previously open items:

  • finding 1: custom-commands is added to the root schema, so global config is supported.
  • finding 2: schema coverage was added with validation tests for both valid and invalid configs.
  • finding 3: the render path now guards against bad custom-commands data, so the crash is avoided.

So at this point, the custom commands feature appears internally consistent across schema, rendering, and tests.

Comment thread webhook_server/config/schema.yaml Outdated
Comment thread webhook_server/tests/test_config_schema.py
Comment thread webhook_server/tests/test_config_schema.py
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 51b41f1

@rnetser

rnetser commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/tests/test_config_schema.py:1038 (qodo rule violation) — ghp_test123 missing allowlist

Addressed: Fixed in 57db85f — added # pragma: allowlist secret to all ghp_test123 occurrences in test_config_schema.py.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — custom-commands missing global schema

Addressed: Qodo confirmed this is resolved in their reply — custom-commands is now at both root and per-repo schema levels.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — Missing custom-commands schema tests

Addressed: Qodo confirmed this is resolved — 6 schema validation tests added in TestCustomCommandsSchema.

webhook_server/config/schema.yaml:256 (qodo requirement gap) — custom-commands allows empty list

Addressed: Fixed in 57db85f — added minItems: 1 to both global and per-repo custom-commands schema, preventing empty lists.

webhook_server/tests/test_config_schema.py:8 (qodo bug) — Missing jsonschema dependency

Addressed: Fixed in 57db85f — added jsonschema>=4.0.0 to tests dependency group in pyproject.toml.

webhook_server/libs/handlers/pull_request_handler.py:864 (qodo bug) — Custom commands markdown injection

Addressed: Fixed in 57db85f — added runtime re.fullmatch name pattern validation and description newline sanitization.

@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/tests/test_config_schema.py:1038 (qodo rule violation) — ghp_test123 missing allowlist

Addressed: Fixed in 57db85f — added # pragma: allowlist secret to all ghp_test123 occurrences in test_config_schema.py.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — custom-commands missing global schema

Addressed: Qodo confirmed this is resolved in their reply — custom-commands is now at both root and per-repo schema levels.

webhook_server/config/schema.yaml:719 (qodo requirement gap) — Missing custom-commands schema tests

Addressed: Qodo confirmed this is resolved — 6 schema validation tests added in TestCustomCommandsSchema.

webhook_server/config/schema.yaml:256 (qodo requirement gap) — custom-commands allows empty list

Addressed: Fixed in 57db85f — added minItems: 1 to both global and per-repo custom-commands schema, preventing empty lists.

webhook_server/tests/test_config_schema.py:8 (qodo bug) — Missing jsonschema dependency

Addressed: Fixed in 57db85f — added jsonschema>=4.0.0 to tests dependency group in pyproject.toml.

webhook_server/libs/handlers/pull_request_handler.py:864 (qodo bug) — Custom commands markdown injection

Addressed: Fixed in 57db85f — added runtime re.fullmatch name pattern validation and description newline sanitization.

Thanks — those items look addressed in 57db85f.

  • The schema/docs/tests changes for custom-commands are covered.
  • The markdown injection and empty-list cases also appear fixed.
  • jsonschema was added to the test deps as well.

One note: the review index still shows finding 6 as active, so it may just be stale now. If you want, I can help verify that it’s safe to close.

Comment thread webhook_server/config/schema.yaml
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 57db85f

@rnetser

rnetser commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/config/schema.yaml:740 (qodo bug) — Cannot clear global commands

Addressed: Fixed in 588b03f — removed minItems: 1 from per-repo custom-commands schema. Global level keeps minItems: 1, per-repo allows empty list override.

webhook_server/tests/test_config_schema.py:8 (qodo bug) — Missing jsonschema dependency

Addressed: Already fixed in 57db85f — jsonschema>=4.0.0 added to tests dependency group in pyproject.toml.

@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/config/schema.yaml:740 (qodo bug) — Cannot clear global commands

Addressed: Fixed in 588b03f — removed minItems: 1 from per-repo custom-commands schema. Global level keeps minItems: 1, per-repo allows empty list override.

webhook_server/tests/test_config_schema.py:8 (qodo bug) — Missing jsonschema dependency

Addressed: Already fixed in 57db85f — jsonschema>=4.0.0 added to tests dependency group in pyproject.toml.

Thanks — both comments are addressed in the diff:

  • finding 5 is fixed by allowing per-repo custom-commands to be emptied while keeping the global default required.
  • finding 7 is covered by adding jsonschema>=4.0.0 to the tests dependency group in pyproject.toml.

So the remaining review state looks stale rather than unresolved.

Comment thread webhook_server/libs/handlers/pull_request_handler.py Outdated
@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:1647 (qodo bug) — Trailing-newline name bypass

Addressed: Fixed — changed safe_name_pattern.match(name) to safe_name_pattern.fullmatch(name) to close the trailing-newline bypass. Qodo confirmed resolved.

Thanks — finding 6 is resolved.

  • The switch from match() to fullmatch() closes the trailing-newline bypass.
  • No further action needed on that item.

@rnetser

rnetser commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review

Please re-evaluate all remaining sticky findings against the current code.
For each finding, check if the referenced code has been fixed in subsequent commits.
Remove findings that are fully addressed. Keep any findings where the issue is still present in the code.

@qodo-code-review

Copy link
Copy Markdown

code-review

Please re-evaluate all remaining sticky findings against the current code.
For each finding, check if the referenced code has been fixed in subsequent commits.
Remove findings that are fully addressed. Keep any findings where the issue is still present in the code.

I re-checked the only remaining sticky item and removed it:

  • finding 6 — fully addressed in webhook_server/libs/github_api.py.
    • The custom-command name validation now uses a strict safe-pattern fullmatch, so the trailing-newline bypass is no longer present.

No active sticky findings remain.

@rnetser

rnetser commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

/verified

@myakove myakove left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review

Found 4 issue(s) in this PR:

💡 Suggestions (4)

File Line Issue
docs/configuration-reference.md 302 [CRITICAL] Manual edit to docs/ violates AGENTS.md rule
webhook_server/libs/github_api.py 1618 [WARNING] Defensive None guard is dead code
webhook_server/libs/github_api.py 1635 [WARNING] Log prefix leading space when empty
webhook_server/libs/github_api.py 1649 [SUGGESTION] No collision check against built-in command names

Review generated by pi

Comment thread docs/configuration-reference.md Outdated
Comment thread webhook_server/libs/github_api.py
Comment thread webhook_server/libs/github_api.py
Comment thread webhook_server/libs/github_api.py
@myakove

myakove commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Review Finding (Round 7 — Soup Nazi Edition 🍜)

⚠️ PR description STILL claims CLAUDE.md + node:26-slim (unresolved since round 3)

The PR description table still says:

  • CLAUDE.md was modified — actual diff modifies AGENTS.md
  • "updated sidecar Docker base image to node:26-slim" — not in the diff at all

NO SOUP FOR YOU until the description matches the diff!

- Remove manual edits to docs/ (auto-generated by docsfy)
- Replace dead None guard with isinstance(list) type check
- Fix log_prefix leading space when empty
- Add BUILTIN_COMMAND_NAMES collision check for built-in commands
- Add test for builtin command name collision

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
Comment thread webhook_server/utils/constants.py
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 6589c8d

Add reprocess, security-override, add-allowed-user, regenerate-welcome,
and hold to the collision check set.

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@rnetser

rnetser commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/utils/constants.py:152 (qodo bug) — Missing built-in command names

Addressed: Added missing built-in commands to BUILTIN_COMMAND_NAMES: reprocess, security-override, add-allowed-user, regenerate-welcome, hold. The collision check now covers all server-processed commands.

@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/utils/constants.py:152 (qodo bug) — Missing built-in command names

Addressed: Added missing built-in commands to BUILTIN_COMMAND_NAMES: reprocess, security-override, add-allowed-user, regenerate-welcome, hold. The collision check now covers all server-processed commands.

Acknowledged — finding 6 is addressed.

  • BUILTIN_COMMAND_NAMES now includes the missing server-processed commands (reprocess, security-override, add-allowed-user, regenerate-welcome, hold).
  • The custom-command collision check now covers the full built-in set.

Comment thread webhook_server/utils/constants.py Outdated
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 21770be

Replace hardcoded string literals with COMMAND_*_STR and label
constants. Also add COMMAND_AUTOMERGE_STR.

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@rnetser

rnetser commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/utils/constants.py:152 (qodo bug) — Missing built-in command names

Addressed: Replaced all hardcoded string literals in BUILTIN_COMMAND_NAMES with existing COMMAND_*_STR and label constants. Also added COMMAND_AUTOMERGE_STR.

@qodo-code-review

Copy link
Copy Markdown

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/utils/constants.py:152 (qodo bug) — Missing built-in command names

Addressed: Replaced all hardcoded string literals in BUILTIN_COMMAND_NAMES with existing COMMAND_*_STR and label constants. Also added COMMAND_AUTOMERGE_STR.

Yes — this addresses finding 6.

  • BUILTIN_COMMAND_NAMES now uses the existing COMMAND_*_STR and label constants instead of hardcoded literals.
  • Adding COMMAND_AUTOMERGE_STR also closes the missing-name gap.

If you want, I can also sanity-check the remaining active findings for any similar constant duplication.

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit ce0a834

@rnetser

rnetser commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed — PR description updated:

  • CLAUDE.mdAGENTS.md
  • Removed stale node:26-slim reference
  • Added webhook_server/utils/constants.py to changes table
  • Updated test count (1730) and coverage (90.13%)

@rnetser

rnetser commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

/build-and-push-container

@myakove-bot

Copy link
Copy Markdown
Collaborator

New container for ghcr.io/myk-org/github-webhook-server:pr-1133 published

@myakove myakove left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review

Found 2 issue(s) in this PR:

💡 Suggestions (2)

File Line Issue
webhook_server/libs/github_api.py 1622 [WARNING] Log prefix inconsistency in early return
webhook_server/libs/github_api.py 1656 [WARNING] Case-insensitive builtin check vs case-sensitive dupe check

Review generated by pi

Comment thread webhook_server/libs/github_api.py Outdated
Comment thread webhook_server/libs/github_api.py
- Use log_msg_prefix in early isinstance return (was using raw prefix with leading space)
- Make duplicate name detection case-insensitive via name.lower()
- Update test for case-insensitive duplicate detection

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@rnetser

rnetser commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-merge-pro please provide a final comprehensive review of the entire PR. List ALL findings at once — this is the last review pass before merge.

@myakove — requesting your final review as well. Please post all remaining comments in one pass so we can address everything together.

@qodo-code-review

Copy link
Copy Markdown

merge-pro please provide a final comprehensive review of the entire PR. List ALL findings at once — this is the last review pass before merge.

@myakove — requesting your final review as well. Please post all remaining comments in one pass so we can address everything together.

I don’t have any remaining findings to raise on this PR.

Final review

  • 0 active findings remain.
  • The previously reported items are either implemented or dismissed.
  • I don’t see any additional merge-blocking issues from the current diff.

Summary

This PR looks ready to merge from my side.

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit b0245d2

…s-4-6) <noreply@pi.dev>

Signed-off-by: rnetser <rnetser@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 94e819e

@rnetser

rnetser commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Closing — scope changed from custom-commands to welcome-extra-info. New issue/PR to follow.

@myakove-bot

Copy link
Copy Markdown
Collaborator

Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-1133.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: support user-defined custom commands in PR welcome message

3 participants