fix: resolve Codex review findings #1-5 (P1) - #60
Merged
Conversation
Verified each of Codex's adversarial-review findings against source before writing anything (per the review request); all 5 confirmed real, no false positives: 1. Template_Manager::update_template() checks wp_insert_post()'s error but silently discards both wp_set_object_terms() calls that follow it (class-template-manager.php:316, :320). A pre_insert_term failure (or any other wp_set_object_terms() failure) leaves a published wp_template/ wp_template_part row with no wp_theme term -- orphaned: get_block_templates() can never find it again, yet it lingers in the database with whatever content the call went on to write, and the caller is told success:true. 2. Pattern_Manager::create_pattern() checks `'' === $title` on the RAW value before sanitize_text_field() runs (class-pattern-manager.php:747-748, applied only later at :817). A whitespace-only or pure-markup title (e.g. "<script></script>") passes the raw check and collapses to an empty post_title at insert time. 3. create_pattern() always returns a `core/block` synced-reference snippet (class-pattern-manager.php:858), even for sync_status:"unsynced" -- following it would re-link content the caller explicitly asked to keep non-propagating. 4. Tool_Executor::execute_list_block_types() never reads `include_supports` from $input, so ability callers always get stripped block types regardless of what they pass (class-tool-executor.php:150-159). 5. Tool_Executor::execute_list_patterns() never reads `category` and never forwards the REST response's `categories` vocabulary, so ability results diverge from REST/npm-MCP (class-tool-executor.php:191-225). New tests/Abilities/ToolExecutorParityAuditTest.php: a structural audit -- every property a manifest tool declares in its input_schema must appear, verbatim, in that tool's Tool_Executor::execute_<name>() method body. Running it against the unmodified codebase caught findings #4 and #5 exactly, plus a third divergence it correctly surfaced and I investigated separately: update_block's `block_name` is declared but never referenced. That one is NOT a wiring gap -- block_name selects which npm-MCP-server-side enricher (src/enrichers.ts) to run entirely client-side before only the already-enriched attributes/innerHTML are sent over the wire; the REST endpoint never accepts a block_name parameter, so Tool_Executor has nothing to forward it to. Documented as a real, separate, pre-existing gap (the Abilities path gets no automatic computed-field derivation the way npm-MCP callers do) via an explicit, commented exemption rather than silently dropped or expanded into a PHP-side enrichment port. New tests/Abilities/ListToolsAbilityParityTest.php: behavioral proof for findings #4/#5 specifically (the structural audit proves the argument name is *referenced*; these prove it changes the actual response). tests/REST/CreatePatternTest.php gains title-sanitization tests (#2, plus a control case: real text with incidental whitespace must still succeed) and reference/insert_hint tests (#3, plus a control case pinning the synced response shape is unchanged). tests/Templates/TemplateManagerWriteTest.php gains two rollback tests (#1): a pre_insert_term filter forces wp_theme term assignment to fail for a wp_template write, and a second test isolates the wp_template_part_area assignment specifically (by letting wp_theme succeed and only failing the area term) so both checks this issue asks for are independently proven. Confirmed red (full suite, not just the new files): $ composer test Tests: 1425, Assertions: 18456, Failures: 8. 1) CreatePatternTest::test_whitespace_only_title_is_rejected 2) CreatePatternTest::test_markup_only_title_that_sanitizes_to_empty_is_rejected 3) CreatePatternTest::test_unsynced_pattern_does_not_return_synced_reference 4) ListToolsAbilityParityTest::test_list_block_types_ability_forwards_include_supports 5) ListToolsAbilityParityTest::test_list_patterns_ability_forwards_category_filter_and_includes_categories 6) ToolExecutorParityAuditTest::test_every_manifest_input_property_is_referenced_by_its_tool_executor_method 7) TemplateManagerWriteTest::test_update_template_rolls_back_new_override_when_wp_theme_term_assignment_fails 8) TemplateManagerWriteTest::test_update_template_rolls_back_new_override_when_area_term_assignment_fails $ npm test Test Files 55 passed (55) -- no TS-side change needed for red yet; types/tool description updates for finding #3 land in the green commit. Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
Codex review of the merged range 602ae77...b089815 flagged five findings; all five verified genuine against source and fixed here: 1. Template_Manager::update_template() now checks the WP_Error return of both wp_set_object_terms() calls when creating a new override, deleting the freshly-inserted post and returning the error on failure instead of leaving an untraceable orphan post behind. 2. Pattern_Manager::create_pattern() now sanitizes `title` before the emptiness check, so a whitespace- or markup-only title that collapses to '' under sanitize_text_field() is rejected instead of silently creating a nameless pattern. 3. create_pattern()'s response now omits `reference` for an unsynced pattern (core/block is a live, propagating reference — wrong for a one-off) and instead returns `insert_hint` pointing callers at insert_pattern's synced:false path. TS types (CreatePatternResponse) and the create_pattern tool description are updated to match; the synced case is unchanged (backward compatible). 4. Tool_Executor::execute_list_block_types() now forwards `include_supports` on the Abilities/MCP-Adapter path, matching REST and the npm MCP server. 5. Tool_Executor::execute_list_patterns() now forwards `category` and includes `categories` in its response, matching REST and the npm MCP server. Findings 4 and 5 are the same REST-vs-Abilities parity bug class as prior fixes; ToolExecutorParityAuditTest (added in the red commit) statically compares every manifest-declared input property against each tool's execute_*() method body via Reflection, so a newly added argument that isn't forwarded on the Abilities path now fails CI instead of shipping silently. It documents two narrow, deliberate exemptions: tools that forward $input wholesale, and update_block's `block_name` (a client-side-only enrichment selector with no REST equivalent — a real, separate gap, not this bug class). All five findings are pinned by tests (see the red commit); none required a manual-only verification note. Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
WalkthroughThe changes update ChangesPattern response handling
Tool listing parity
Template override rollback
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
wordpress-plugin/gk-block-mcp/includes/class-tool-executor.php (1)
197-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the checks before these ternaries.
Inline
isset()/type checks in ternaries conflict with the plugin conditional-check convention.Proposed cleanup
+ $has_category = isset( $input['category'] ); $params = array( - 'category' => isset( $input['category'] ) ? (string) $input['category'] : null, + 'category' => $has_category ? (string) $input['category'] : null, ); + $has_categories = isset( $data['categories'] ) && is_array( $data['categories'] ); - $categories = isset( $data['categories'] ) && is_array( $data['categories'] ) ? $data['categories'] : array(); + $categories = $has_categories ? $data['categories'] : array();As per coding guidelines, “Assign checks to named variables before
if,while, or ternary conditionals instead of inlining function calls or compound expressions.”Also applies to: 217-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress-plugin/gk-block-mcp/includes/class-tool-executor.php` at line 197, In the tool input construction around the category and corresponding line 217 field, assign each inline isset/type-check condition to a clearly named boolean variable before the ternary expressions. Update the ternaries to use those named checks while preserving the existing cast and null fallback behavior.Source: Coding guidelines
wordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.php (1)
2-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace review history and off-tree references with local behavioral contracts.
wordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.php#L2-L9: describe the parity behavior under test; remove Codex-review history and meta-test commentary.wordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.php#L20-L24: state the Abilities bootstrap precondition locally; remove the cross-test docblock reference.wordpress-plugin/gk-block-mcp/tests/Abilities/ToolExecutorParityAuditTest.php#L2-L9: describe the audit’s current invariant without review-finding history.wordpress-plugin/gk-block-mcp/tests/Abilities/ToolExecutorParityAuditTest.php#L20-L29: document the wholesale-forwarding exemption as a contract; remove verification-process instructions.wordpress-plugin/gk-block-mcp/tests/Abilities/ToolExecutorParityAuditTest.php#L45-L58: retain only the PHP-side exemption contract; removesrc/enrichers.tspointers and future-feature discussion.wordpress-plugin/gk-block-mcp/tests/Abilities/ToolExecutorParityAuditTest.php#L96-L101: state why missing executor methods are skipped without referring to another test.As per coding guidelines, comments must document “current behavior and hard contracts only” and omit “history, journal entries, off-tree specification pointers, and future-architecture speculation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.php` around lines 2 - 9, Replace review history, meta-test commentary, off-tree references, verification instructions, and future-architecture speculation in ListToolsAbilityParityTest.php (lines 2-9 and 20-24) and ToolExecutorParityAuditTest.php (lines 2-9, 20-29, 45-58, and 96-101) with concise comments describing only the local behavioral parity contract, Abilities bootstrap precondition, wholesale-forwarding and PHP-side exemptions, and why missing executor methods are skipped.Source: Coding guidelines
wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json (1)
1122-1122: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify whether the unsynced insertion hint is partial.
The manifest and regression test describe only
pattern_idandsynced:false, butinsert_patternrequirespost_id. Make the hint explicitly partial and require callers to addpost_id, or expose a complete executable parameter set.
wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json#L1122-L1122: clarify the missing destination parameter in the public description.wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L244-L269: align the assertion and documentation with the partial-hint contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json` at line 1122, Clarify the unsynced pattern contract in wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json: describe insert_hint as a partial parameter set and explicitly require callers to add post_id before invoking insert_pattern. In wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php lines 244-269, update the regression assertion and accompanying documentation to verify and describe that post_id is intentionally omitted and must be supplied by the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress-plugin/gk-block-mcp/includes/class-template-manager.php`:
- Around line 319-329: In the template creation flow, assign the results of both
is_wp_error() checks for $theme_term and $area_term to clearly named boolean
variables before their respective if statements, then branch on those variables
while preserving the existing cleanup and return behavior.
- Around line 318-329: Update the rollback paths in the template creation flow
around the wp_theme and wp_template_part_area taxonomy assignments to check the
result of wp_delete_post( $post_id, true ). If cleanup fails, return a dedicated
WP_Error with HTTP status 500; otherwise preserve returning the original
taxonomy/write error. Apply this consistently to every rollback deletion path in
the surrounding logic.
In `@wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php`:
- Around line 168-175: Remove the “Codex review” attribution and review-history
wording from both documentation blocks in
wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L168-L175 and
`#L223-L229`. Retain only the current contracts: sanitize the title before
checking emptiness at the first site, and document the synced/unsynced response
behavior at the second site.
- Around line 211-220: Update
test_title_with_real_text_and_surrounding_whitespace_is_accepted in
CreatePatternTest so the title input contains real markup, such as emphasized
text, alongside surrounding whitespace. Adjust the expected response title
assertion to match the sanitized result, preserving coverage for successful
markup handling rather than whitespace-only input.
In `@wordpress-plugin/gk-block-mcp/tests/Templates/TemplateManagerWriteTest.php`:
- Around line 394-408: Replace the historical “Codex review” comment near the
term-assignment tests with a concise present-tense contract describing that
failed required taxonomy assignment returns WP_Error and deletes the newly
created override. Remove review-tool attribution, defect history, and
explanatory implementation narrative while preserving the test’s behavioral
requirement.
---
Nitpick comments:
In `@wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json`:
- Line 1122: Clarify the unsynced pattern contract in
wordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.json: describe
insert_hint as a partial parameter set and explicitly require callers to add
post_id before invoking insert_pattern. In
wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php lines 244-269,
update the regression assertion and accompanying documentation to verify and
describe that post_id is intentionally omitted and must be supplied by the
caller.
In `@wordpress-plugin/gk-block-mcp/includes/class-tool-executor.php`:
- Line 197: In the tool input construction around the category and corresponding
line 217 field, assign each inline isset/type-check condition to a clearly named
boolean variable before the ternary expressions. Update the ternaries to use
those named checks while preserving the existing cast and null fallback
behavior.
In
`@wordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.php`:
- Around line 2-9: Replace review history, meta-test commentary, off-tree
references, verification instructions, and future-architecture speculation in
ListToolsAbilityParityTest.php (lines 2-9 and 20-24) and
ToolExecutorParityAuditTest.php (lines 2-9, 20-29, 45-58, and 96-101) with
concise comments describing only the local behavioral parity contract, Abilities
bootstrap precondition, wholesale-forwarding and PHP-side exemptions, and why
missing executor methods are skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ace5a0b-0025-439e-aa36-c36fe79e3fde
📒 Files selected for processing (11)
src/tools/patterns.tssrc/types.tswordpress-plugin/gk-block-mcp/assets/mcp-server/index.cjswordpress-plugin/gk-block-mcp/includes/abilities/tools.manifest.jsonwordpress-plugin/gk-block-mcp/includes/class-pattern-manager.phpwordpress-plugin/gk-block-mcp/includes/class-template-manager.phpwordpress-plugin/gk-block-mcp/includes/class-tool-executor.phpwordpress-plugin/gk-block-mcp/tests/Abilities/ListToolsAbilityParityTest.phpwordpress-plugin/gk-block-mcp/tests/Abilities/ToolExecutorParityAuditTest.phpwordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.phpwordpress-plugin/gk-block-mcp/tests/Templates/TemplateManagerWriteTest.php
Comment on lines
+318
to
+329
| $theme_term = wp_set_object_terms( $post_id, get_stylesheet(), 'wp_theme' ); | ||
| if ( is_wp_error( $theme_term ) ) { | ||
| wp_delete_post( $post_id, true ); | ||
| return $theme_term; | ||
| } | ||
|
|
||
| if ( 'wp_template_part' === $type ) { | ||
| $area = ! empty( $template->area ) ? (string) $template->area : WP_TEMPLATE_PART_AREA_UNCATEGORIZED; | ||
| wp_set_object_terms( $post_id, _filter_block_template_part_area( $area ), 'wp_template_part_area' ); | ||
| $area = ! empty( $template->area ) ? (string) $template->area : WP_TEMPLATE_PART_AREA_UNCATEGORIZED; | ||
| $area_term = wp_set_object_terms( $post_id, _filter_block_template_part_area( $area ), 'wp_template_part_area' ); | ||
| if ( is_wp_error( $area_term ) ) { | ||
| wp_delete_post( $post_id, true ); | ||
| return $area_term; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant range.
ast-grep outline wordpress-plugin/gk-block-mcp/includes/class-template-manager.php --view expanded || true
wc -l wordpress-plugin/gk-block-mcp/includes/class-template-manager.php
sed -n '280,360p' wordpress-plugin/gk-block-mcp/includes/class-template-manager.php
# Find surrounding error-handling patterns in the plugin.
rg -n "wp_delete_post\\(|new WP_Error|handle_error\\(|is_wp_error\\(" wordpress-plugin/gk-block-mcp/includes -g '*.php'Repository: GravityKit/block-mcp
Length of output: 20636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect WordPress deletion semantics from the codebase if any wrapper exists.
rg -n "wp_delete_post\\(" wordpress-plugin -g '*.php'
rg -n "cleanup-specific 500|WP_Error|500" wordpress-plugin/gk-block-mcp/includes -g '*.php'Repository: GravityKit/block-mcp
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific section around the reported lines.
sed -n '300,350p' wordpress-plugin/gk-block-mcp/includes/class-template-manager.phpRepository: GravityKit/block-mcp
Length of output: 1822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the matching cleanup pattern in post-manager and the template write test.
sed -n '236,262p' wordpress-plugin/gk-block-mcp/includes/class-post-manager.php
sed -n '390,430p' wordpress-plugin/gk-block-mcp/tests/Templates/TemplateManagerWriteTest.phpRepository: GravityKit/block-mcp
Length of output: 2903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '400,440p' wordpress-plugin/gk-block-mcp/tests/Templates/TemplateManagerWriteTest.php
sed -n '404,422p' wordpress-plugin/gk-block-mcp/includes/class-template-manager.phpRepository: GravityKit/block-mcp
Length of output: 2234
Handle rollback deletion failures. In every rollback path here, check wp_delete_post() and return a dedicated 500 if cleanup fails; otherwise the original taxonomy/write error is returned while the published override can remain orphaned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/includes/class-template-manager.php` around
lines 318 - 329, Update the rollback paths in the template creation flow around
the wp_theme and wp_template_part_area taxonomy assignments to check the result
of wp_delete_post( $post_id, true ). If cleanup fails, return a dedicated
WP_Error with HTTP status 500; otherwise preserve returning the original
taxonomy/write error. Apply this consistently to every rollback deletion path in
the surrounding logic.
Comment on lines
+319
to
+329
| if ( is_wp_error( $theme_term ) ) { | ||
| wp_delete_post( $post_id, true ); | ||
| return $theme_term; | ||
| } | ||
|
|
||
| if ( 'wp_template_part' === $type ) { | ||
| $area = ! empty( $template->area ) ? (string) $template->area : WP_TEMPLATE_PART_AREA_UNCATEGORIZED; | ||
| wp_set_object_terms( $post_id, _filter_block_template_part_area( $area ), 'wp_template_part_area' ); | ||
| $area = ! empty( $template->area ) ? (string) $template->area : WP_TEMPLATE_PART_AREA_UNCATEGORIZED; | ||
| $area_term = wp_set_object_terms( $post_id, _filter_block_template_part_area( $area ), 'wp_template_part_area' ); | ||
| if ( is_wp_error( $area_term ) ) { | ||
| wp_delete_post( $post_id, true ); | ||
| return $area_term; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Name the error predicates before branching.
Extract each is_wp_error() result into a named boolean before its if statement.
As per coding guidelines, “Assign checks to named variables before if, while, or ternary conditionals instead of inlining function calls or compound expressions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/includes/class-template-manager.php` around
lines 319 - 329, In the template creation flow, assign the results of both
is_wp_error() checks for $theme_term and $area_term to clearly named boolean
variables before their respective if statements, then branch on those variables
while preserving the existing cleanup and return behavior.
Source: Coding guidelines
Comment on lines
+168
to
+175
| // ── Codex review: title sanitized before, not after, the empty check ── | ||
|
|
||
| /** | ||
| * A whitespace-only title passes the raw non-empty-string check but | ||
| * `sanitize_text_field()` trims it to '' when building post_title, | ||
| * silently creating a nameless pattern. The emptiness check must run | ||
| * against the sanitized value. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove review-tool history from test documentation.
Both ranges include Codex review attribution. Comments should describe only the current sanitization and response-shape contracts.
wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L168-L175: remove the review-history banner and retain the sanitize-before-empty-check contract.wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L223-L229: remove the review-history banner and retain the synced/unsynced response contract.
As per coding guidelines, comments/docblocks must document current behavior and hard contracts and omit review-tool attributions and historical journals.
📍 Affects 1 file
wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L168-L175(this comment)wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L223-L229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php` around lines
168 - 175, Remove the “Codex review” attribution and review-history wording from
both documentation blocks in
wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php#L168-L175 and
`#L223-L229`. Retain only the current contracts: sanitize the title before
checking emptiness at the first site, and document the synced/unsynced response
behavior at the second site.
Source: Coding guidelines
Comment on lines
+211
to
+220
| public function test_title_with_real_text_and_surrounding_whitespace_is_accepted() { | ||
| $response = $this->create_pattern( | ||
| array( | ||
| 'title' => ' Real Title ', | ||
| 'content' => '<!-- wp:paragraph --><p>hi</p><!-- /wp:paragraph -->', | ||
| ) | ||
| ); | ||
|
|
||
| $this->assertInstanceOf( \WP_REST_Response::class, $response ); | ||
| $this->assertSame( 'Real Title', $response->get_data()['title'] ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the markup-preserving success case.
The docblock claims whitespace/markup coverage, but the input only contains whitespace. Use real text wrapped in markup, such as <em>Real Title</em>, and assert the sanitized result so over-aggressive sanitization remains covered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/tests/REST/CreatePatternTest.php` around lines
211 - 220, Update
test_title_with_real_text_and_surrounding_whitespace_is_accepted in
CreatePatternTest so the title input contains real markup, such as emphasized
text, alongside surrounding whitespace. Adjust the expected response title
assertion to match the sanitized result, preserving coverage for successful
markup handling rather than whitespace-only input.
Comment on lines
+394
to
+408
| // ── Codex review: term-assignment failure must not orphan the override ─ | ||
|
|
||
| /** | ||
| * `wp_insert_post()`'s error is checked, but the two `wp_set_object_terms()` | ||
| * calls that follow it were not — a taxonomy failure (e.g. a | ||
| * `pre_insert_term` filter rejecting the `wp_theme` term) left a | ||
| * published `wp_template`/`wp_template_part` row with no `wp_theme` | ||
| * term, so `get_block_templates()` could never find it again | ||
| * (`get_template()` would keep resolving the theme file), yet the | ||
| * orphaned post — and whatever content the call went on to write into it | ||
| * — stayed in the database with `success: true` reported to the caller. | ||
| * The fix must check both term-assignment calls and roll back | ||
| * (`wp_delete_post()`) the freshly-created override on failure, exactly | ||
| * like the existing legacy-block-rejection rollback. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove review history from the test comments.
“Codex review” and the defect history violate the comment policy. Keep a short present-tense contract, e.g. “A failed required taxonomy assignment returns WP_Error and removes the new override.”
As per coding guidelines, comments must “document current behavior and hard contracts” and omit “review-tool attributions” and “historical journals.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/tests/Templates/TemplateManagerWriteTest.php`
around lines 394 - 408, Replace the historical “Codex review” comment near the
term-assignment tests with a concise present-tense contract describing that
failed required taxonomy assignment returns WP_Error and deletes the newly
created override. Remove review-tool attribution, defect history, and
explanatory implementation narrative while preserving the test’s behavioral
requirement.
Source: Coding guidelines
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Codex's adversarial review of the merged range
602ae77...b089815(BLOCK-29, BLOCK-33, BLOCK-34, BLOCK-35) returned five P1 findings. All five were verified against source before fixing — all five are genuine, no false positives.class-template-manager.php:316update_template()ignoredWP_Errorfromwp_set_object_terms()while creating a new override, leaving an orphaned post on failurewp_theme, andwp_template_part_areafor parts); on failure, delete the freshly-inserted post and return theWP_Errorclass-pattern-manager.php:747create_pattern()validatedtitlebefore sanitizing, so a whitespace-/markup-only title (e.g." ","<script></script>") that sanitizes to''was accepted, silently creating a nameless patternsanitize_text_field()first, then check the sanitized value for emptinessclass-pattern-manager.php:858create_pattern()always returned acore/blockreference, even for an unsynced pattern — butcore/blockis WordPress's live, propagating synced-block reference, wrong for a one-offreferenceis now returned only whensync_status === 'synced'. Forunsynced, the response instead includesinsert_hint: {tool: "insert_pattern", params: {pattern_id, synced: false}}(verifiedBlock_Writer::insert_pattern()already supportssynced:falseagainst anypattern_idindependent of the pattern's own sync-status meta). TSCreatePatternResponse(src/types.ts) and thecreate_patterntool description (src/tools/patterns.ts) updated to match. The synced case is unchanged — backward compatible.class-tool-executor.php:151list_block_typesnever forwardedinclude_supports— silently dropped on the Abilities/MCP-Adapter transport while REST and the npm MCP server honored itinclude_supportsinto the REST-handler paramsclass-tool-executor.php:194list_patternsignoredcategoryand droppedcategoriesfrom the responsecategoryinto the REST-handler params; includecategoriesin the returned arrayFindings 4 and 5 are the same REST-vs-Abilities parity bug class fixed twice already in this range (BLOCK-34, BLOCK-35). Per the review request, the structural audit is now generalized:
ToolExecutorParityAuditTest::test_every_manifest_input_property_is_referenced_by_its_tool_executor_method()reflects over everyTool_Executor::execute_*()method and asserts every manifest-declaredinput_schemaproperty for that tool is referenced in the method body. It documents two narrow, deliberate exemptions rather than silently special-casing them:create_post,create_pattern,upload_media,list_terms,list_posts,get_post_info,edit_block_tree,update_post,yoast_update_seo) — these forward$inputverbatim (minus an explicitunset()) to the REST handler, so "does the property name appear as a literal" is the wrong question for them by construction.update_block'sblock_name— a genuinely new finding surfaced while writing this test, not the same bug class. It's a client-side-only enrichment selector (src/enrichers.ts) with no REST/PHP equivalent; the npm MCP server derives computed fields (e.g. CBP'scodeHTML) on the client before sending only the already-enriched attributes over the wire, soTool_Executorhas nothing to forward it to. This is a real, standing gap (an Abilities-path caller doesn't get automatic enricher-derived fields), but porting enrichment to PHP is a separate feature, not a wiring fix — documented in the test rather than silently exempted or force-fixed here.ListToolsAbilityParityTestadds behavioral coverage on top of the structural check, provinginclude_supports/categoryactually change the returned data (not just that the property name appears in the method body).Verification
All 5 findings are pinned by tests — none required a manual-only verification note. Visible red/green git history (unsquashed):
2cc44cdtest(codex-review): pin all 5 confirmed P1 findings [red]258c143fix(codex-review): resolve all 5 confirmed P1 findings [green]Full local gate, all green:
No Siteminter smoke test in this PR (not required per the assignment — the fixes are fully covered by the tests above, and staging validation is happening separately). No Linear issue exists for this batch (Codex review, not a tracked issue); noting this PR as a comment on BLOCK-31/33/34.
Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
Summary by CodeRabbit
New Features
Bug Fixes
💾 Build file (258c143).