feat: consolidate docs, backend/session infra, and settings updates
This commit is contained in:
Dwindi Ramadhana
177
docs/architecture/PLUGIN_AUDIT_RETRACE_FIFTH_PASS_2026-05-25.md
Normal file
177
docs/architecture/PLUGIN_AUDIT_RETRACE_FIFTH_PASS_2026-05-25.md
Normal file
@@ -0,0 +1,177 @@
|
||||
# WP Agentic Writer Fifth Retrace Audit
|
||||
|
||||
Status: COMPLETE / RETRACED
|
||||
Completion marker: 2026-05-25
|
||||
Follow-up report: `docs/architecture/PLUGIN_AUDIT_RETRACE_SIXTH_PASS_2026-05-25.md`
|
||||
|
||||
Audit date: 2026-05-25
|
||||
Baseline retraced: `docs/architecture/PLUGIN_AUDIT_RETRACE_FOURTH_PASS_2026-05-25.md`
|
||||
Scope: fifth pass after fourth-retrace implementation, covering REST authorization, conversation context/history, provider metadata, cost attribution, model defaults, and release readiness.
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The fourth-pass implementation closed meaningful gaps:
|
||||
|
||||
- `handle_clear_context()` now checks `edit_post` before clearing post context.
|
||||
- Legacy chat migration now deletes `_wpaw_chat_history` and writes `_wpaw_chat_history_migrated`.
|
||||
- `get_context()` now attempts migrate-on-read when no session exists and legacy post meta is present.
|
||||
- `record_usage()` is now marked deprecated.
|
||||
- PHP syntax validation still passes.
|
||||
- `assets/js/sidebar.js` and `assets/js/settings-v2.js` still pass JavaScript syntax validation.
|
||||
|
||||
The plugin is closer, but not fully clean. The remaining problem is no longer broad absence of checks; it is **ordering and coverage**. Several handlers added permission checks, but some still read post config/meta before checking access. Other AI utility routes accept `postId` for cost attribution or context but still never verify that the current user can edit that post.
|
||||
|
||||
## Verification Performed
|
||||
|
||||
- PHP syntax check across plugin PHP files: passed.
|
||||
- `node -c assets/js/sidebar.js`: passed.
|
||||
- `node -c assets/js/settings-v2.js`: passed.
|
||||
- Static trace of fourth-pass findings against current code.
|
||||
- No live WordPress browser workflow was run in this pass.
|
||||
|
||||
## Fourth-Pass Status Trace
|
||||
|
||||
| Fourth-pass item | Current status | Evidence |
|
||||
|---|---:|---|
|
||||
| Clear-context post auth | Fixed | `handle_clear_context()` checks `check_post_permission()` before clearing at `includes/class-gutenberg-sidebar.php:1240-1263`. |
|
||||
| Legacy chat migration cleanup | Fixed | `migrate_legacy_chat_history()` deletes legacy meta and writes `_wpaw_chat_history_migrated` at `includes/class-context-service.php:310-312`. |
|
||||
| Migrate-on-read behavior | Improved | `get_context()` triggers migration when no session exists and legacy history is present at `includes/class-context-service.php:62-78`. |
|
||||
| Deprecated legacy cost method | Improved | `record_usage()` is marked deprecated at `includes/class-cost-tracker.php:162-188`. |
|
||||
| Permission sweep | Partially fixed | Several routes now check, but some checks still happen after post reads and some post-id routes still lack checks. |
|
||||
| Provider metadata response contract | Still open | Chat exposes provider metadata; non-chat generated responses remain inconsistent. |
|
||||
| Model registry/default unification | Still open | Defaults remain spread across PHP, JS, providers, wrapper, and image manager. |
|
||||
|
||||
## Critical Findings
|
||||
|
||||
### P0: Some Permission Checks Still Happen After Post-Scoped Reads
|
||||
|
||||
The fourth-pass report asked for checks before any post read/write/cost attribution/streaming. Some handlers added checks, but they are still too late:
|
||||
|
||||
- `handle_revise_plan()` calls `resolve_post_config_from_request()` and reads `_wpaw_detected_language` before checking `edit_post` at `includes/class-gutenberg-sidebar.php:2023-2057`.
|
||||
- `handle_block_refine()` calls `resolve_post_config_from_request()` before checking `edit_post` at `includes/class-gutenberg-sidebar.php:4203-4230`.
|
||||
- `handle_refine_from_chat()` calls `resolve_post_config_from_request()` before checking `edit_post` at `includes/class-gutenberg-sidebar.php:4857-4885`.
|
||||
- `handle_generate_meta()` reads post content/title with `get_post()` before checking `edit_post` at `includes/class-gutenberg-sidebar.php:6077-6108`.
|
||||
- `handle_check_clarity()` calls `resolve_post_config_from_request()` before checking `edit_post` at `includes/class-gutenberg-sidebar.php:3809-3839`.
|
||||
|
||||
Impact:
|
||||
|
||||
- A user can still trigger reads of post config, detected language, post title/content, or other post-scoped metadata for posts they cannot edit.
|
||||
- These are not merely theoretical because helper calls like `resolve_post_config_from_request()` fall back to stored post config.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Move the post permission check immediately after extracting `postId`, before any helper call that may read post meta/content.
|
||||
- Use one centralized helper so the ordering is hard to get wrong:
|
||||
- Extract post id.
|
||||
- If `postId > 0`, require `edit_post`.
|
||||
- Only then read config/meta/content or start streaming.
|
||||
|
||||
### P0: Some Post-ID Utility Routes Still Have No Target-Post Check
|
||||
|
||||
Several routes still accept `postId` and use it for context or cost attribution without validating target-post access:
|
||||
|
||||
- `handle_summarize_context()` accepts `postId` and records cost against it without checking `edit_post` at `includes/class-gutenberg-sidebar.php:6258-6330`.
|
||||
- `handle_detect_intent()` accepts `postId` and records cost against it without checking `edit_post` at `includes/class-gutenberg-sidebar.php:6357-6415`.
|
||||
- `handle_refine_multi_pass()` accepts `postId` and records cost against it without checking `edit_post` at `includes/class-gutenberg-sidebar.php:6730-6782`.
|
||||
|
||||
Impact:
|
||||
|
||||
- Cost records can still be attributed to posts the user cannot edit.
|
||||
- These endpoints can be used to pollute another post's cost ledger even if they do not read that post's content directly.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- If an endpoint accepts `postId > 0`, require `edit_post` before using it for cost tracking.
|
||||
- If the endpoint does not need post authority, ignore client-provided `postId` and track cost against `0` or the active session instead.
|
||||
|
||||
## High Priority Findings
|
||||
|
||||
### P1: Provider Metadata Is Still Not Uniform Outside Chat
|
||||
|
||||
Provider selection metadata exists in many code paths, but only chat responses consistently expose it. Non-chat flows still often return content, blocks, variants, keyword suggestions, or SEO results without a consistent provider envelope.
|
||||
|
||||
Examples from the current trace:
|
||||
|
||||
- Plan/revise/execute/refine handlers use `WP_Agentic_Writer_Provider_Manager::get_provider_for_task()`, but response envelopes do not consistently return `provider`, `selected_provider`, `fallback_used`, `warnings`, `model`, and `cost`.
|
||||
- Keyword and image helper classes unwrap provider results but do not expose fallback metadata to the calling UI consistently.
|
||||
|
||||
Impact:
|
||||
|
||||
- Users and support logs still cannot reliably answer "which provider/model served this request?" outside chat.
|
||||
- Fallback behavior is harder to debug in plan, refinement, SEO/GEO, keyword, and image paths.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Add a shared helper that builds provider metadata from `WPAW_Provider_Selection_Result` plus model/cost response data.
|
||||
- Add it to every generated response and streaming completion event.
|
||||
|
||||
### P1: Migrate-On-Read May Not Return the Newly Created Session
|
||||
|
||||
`get_context()` now calls `migrate_legacy_chat_history( $post_id, $session_id )`, but `migrate_legacy_chat_history()` only accepts `$post_id` in its signature and creates a new session when none exists. After that, `get_context()` tries `$manager->get_session( $session_id )` again.
|
||||
|
||||
Evidence:
|
||||
|
||||
- `get_context()` calls migration with two arguments at `includes/class-context-service.php:71-73`.
|
||||
- `migrate_legacy_chat_history()` signature is `migrate_legacy_chat_history( $post_id )` at `includes/class-context-service.php:267`.
|
||||
- When no session exists, migration creates a new session but does not return that new session id at `includes/class-context-service.php:301-307`.
|
||||
|
||||
Impact:
|
||||
|
||||
- Legacy history can be migrated and deleted, but the same read may still return an empty context if the caller's original `$session_id` was not the newly created session.
|
||||
- The data is safer than before, but UX may still appear as "history disappeared" until a separate session lookup/list refresh.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Make `migrate_legacy_chat_history()` return the target/new `session_id`, or accept a requested session id and create/update that session.
|
||||
- In `get_context()`, fetch the returned session id after migration.
|
||||
|
||||
## Medium Priority Findings
|
||||
|
||||
### P2: Legacy `/chat-history` Endpoint Still Exposes Deprecated Storage
|
||||
|
||||
The route now has permission checks and a deprecated response marker, and legacy writes are disabled. Still, `/chat-history/(?P<post_id>\d+)` remains active and reads `_wpaw_chat_history`.
|
||||
|
||||
Impact:
|
||||
|
||||
- A deprecated route can keep old UI/client assumptions alive.
|
||||
- It can confuse the source-of-truth contract if any caller still consumes it.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Return an empty deprecated response after the migration window, or remove the route once the sidebar is fully on conversation sessions.
|
||||
|
||||
### P2: Legacy `record_usage()` Still Defaults Provider to OpenRouter
|
||||
|
||||
The method is deprecated, but it still records provider as `openrouter` for any caller that uses it.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Change the fallback provider to `unknown`, or require callers to migrate to `record_usage_full()` before accepting records from this compatibility path.
|
||||
|
||||
### P2: Model Defaults Remain Fragmented
|
||||
|
||||
Model defaults still live across activation, settings PHP, settings JS, providers, image manager, and WP AI wrapper. This is unchanged from the fourth pass.
|
||||
|
||||
Recommended fix:
|
||||
|
||||
- Add a PHP model registry/default resolver.
|
||||
- Localize resolved defaults to JS.
|
||||
- Treat legacy `execution_model` as a migration alias into canonical `writing_model`.
|
||||
|
||||
### P2: Sidebar WordPress Compatibility Still Needs Browser Verification
|
||||
|
||||
The sidebar still imports `PluginSidebar` from `wp.editPost`. The previous fallback was removed. Syntax is green, but compatibility still needs a browser check on the minimum supported WordPress version.
|
||||
|
||||
## Definition of Done Gates for This Pass
|
||||
|
||||
Before considering this pass complete:
|
||||
|
||||
- Move every post permission check before any post config/meta/content read.
|
||||
- Add target-post checks or ignore `postId` in summarize-context, detect-intent, and refine-multi-pass.
|
||||
- Make legacy migrate-on-read return the migrated session in the same request.
|
||||
- Standardize provider metadata in non-chat generated responses.
|
||||
- Keep PHP and JS syntax checks green.
|
||||
|
||||
## Current Decision
|
||||
|
||||
The fourth-pass implementation is a real improvement and the legacy history storage is much closer to sane. Do not shift fully into new chat/context work yet. First finish the permission-order sweep and the few remaining cost-attribution endpoints; then the remaining issues are mostly consistency and observability rather than core safety.
|
||||
Reference in New Issue
Block a user