[Epic] MCP Observability Lifecycle and Tooling Capabilities Expansion

[propr-dev]

MCP Observability Lifecycle and Tooling Capabilities Expansion

This Epic PR aggregates all changes for the plan: MCP Observability Lifecycle and Tooling Capabilities Expansion

Issues in this Epic

• MCP: introduce a structured error envelope (code, message, stage, retryable) with secret redaction for every tool call #2593: MCP: introduce a structured error envelope (code, message, stage, retryable) with secret redaction for every tool call
• MCP: give operation receipts a readable lifecycle (accepted, running, completed, failed, cancelled) with timestamps, artifacts and list_operations #2594: MCP: give operation receipts a readable lifecycle (accepted, running, completed, failed, cancelled) with timestamps, artifacts and list_operations
• MCP: track PR command receipts to completion and report ultrafix cycle progress and final outcome #2595: MCP: track PR command receipts to completion and report ultrafix cycle progress and final outcome
• MCP publish_plan: record per-issue failure stage, release the draft on clean failure, and support resuming a partial publication #2596: MCP publish_plan: record per-issue failure stage, release the draft on clean failure, and support resuming a partial publication
• MCP task submissions: expose submission progress through to task state and pull request, add list_task_submissions and a submission resource #2597: MCP task submissions: expose submission progress through to task state and pull request, add list_task_submissions and a submission resource
• MCP: add get_work_overview — running or recent tasks joined with their pull requests, review decision and checks in one call #2598: MCP: add get_work_overview — running or recent tasks joined with their pull requests, review decision and checks in one call
• MCP PR commands: make expectedHead optional on append-only commands and report the resolved head in the receipt #2599: MCP PR commands: make expectedHead optional on append-only commands and report the resolved head in the receipt
• MCP merge_pull_request and PR state checks: fail with the specific precondition and the pull request's actual state #2600: MCP merge_pull_request and PR state checks: fail with the specific precondition and the pull request's actual state
• MCP documentation tools: list_docs, get_doc (bounded chunks) and search_docs over the docs site #2601: MCP documentation tools: list_docs, get_doc (bounded chunks) and search_docs over the docs site
• Bundle the docs into the app image with a build manifest so the MCP docs tools always match the shipped version #2602: Bundle the docs into the app image with a build manifest so the MCP docs tools always match the shipped version
• Extract preview media loading from previewMediaRoutes into a reusable, authorization-preserving service #2603: Extract preview media loading from previewMediaRoutes into a reusable, authorization-preserving service
• MCP visual previews: list_visual_previews and get_visual_preview returning bounded, downscaled image content #2604: MCP visual previews: list_visual_previews and get_visual_preview returning bounded, downscaled image content
• Plan revision history: record what caused each revision (generation, refinement, manual edit, rename, restore) and expose it from get_plan #2605: Plan revision history: record what caused each revision (generation, refinement, manual edit, rename, restore) and expose it from get_plan
• Validate plan refinement output before saving: merge edit-style output into the full plan or reject it with a structured error #2606: Validate plan refinement output before saving: merge edit-style output into the full plan or reject it with a structured error
• MCP configuration coverage: expose trigger access settings (user allowlist, blocklist and exempt/allowed bots) and audit which settings are MCP-reachable #2607: MCP configuration coverage: expose trigger access settings (user allowlist, blocklist and exempt/allowed bots) and audit which settings are MCP-reachable
• Settings location catalog: find_setting tool and a generated "Where each setting lives" docs page #2608: Settings location catalog: find_setting tool and a generated "Where each setting lives" docs page
• Document the observable MCP surface and add an end-to-end regression covering receipts, errors, overview, docs and previews #2609: Document the observable MCP surface and add an end-to-end regression covering receipts, errors, overview, docs and previews

Auto-close

When this PR is merged, the following issues will be automatically closed:

Fixes #2593
Fixes #2594
Fixes #2595
Fixes #2596
Fixes #2597
Fixes #2598
Fixes #2599
Fixes #2600
Fixes #2601
Fixes #2602
Fixes #2603
Fixes #2604
Fixes #2605
Fixes #2606
Fixes #2607
Fixes #2608
Fixes #2609

---

Created automatically by ProPR

[integry]

/fix

[propr-dev]

✅ Applied the requested follow-up changes in commit be451f3

Addressed 1 AI review comment (IDs: 5913277337)

Addressed finding F6

Summary of Changes

F6 is fixed: a plan left executing with an active publication after an API container restart can now be resumed. The publish-plan tests, the related MCP suites, the API typecheck and lint on the changed files all pass.

What changed

• Claim lease. The publishing attempt now writes a renewal timestamp into the active publication before each issue is created or adopted. Each issue POST is aborted one minute after that renewal.
• Bounded takeover. publish_plan with resume: true takes over an active claim that has gone unrenewed for two minutes. It needs no /proc access, so it works after a restart into a new PID namespace. The existing marker lookup still runs before every POST, so an issue the dead attempt created is adopted, not duplicated.
• Early resume. A resume inside the lease still fails with PRECONDITION_FAILED. The message now says the prior attempt may still be running, and details.claimLapsesAt gives the earliest takeover time.
• Docs. docs/mcp.md describes the lapse and takeover.

I chose the renewal variant over the "claim age plus Octokit timeout" variant from the finding. A slow but live 20-task publication can legitimately outlast a fixed age, and a takeover then could duplicate its in-flight issue.

Behaviour to know about

• Each renewal advances the plan's mcp_revision by one, because the existing draft trigger bumps it on every write. A revision read mid-publication can therefore go stale, and the caller re-reads the plan before resuming.
• A takeover is safe only against an owner running this code. An owner that renews between a resume's read and its claim wins, and the resume fails without contacting GitHub.
• One race remains and cannot be closed: GitHub may still create an issue from a request aborted at the deadline. The one-minute grace before takeover and the marker lookup cover it.

Regressions added in packages/api/test/mcpPublishPlan.test.ts; the first three fail against the previous tool code.

• A foreign-namespace claim with a result-less unknown receipt is refused inside its lease, then taken over once lapsed, adopting the marked issue.
• An owner renewal that lands during a takeover's evidence gathering defeats the takeover.
• The owner stores a renewal before each POST, and a hung POST is aborted at the lease deadline and leaves a resumable partial publication.
• Boundary checks for the lapse calculation.

Independent discovery, not fixed
The MCP GitHub client is built with request: { timeout: 10_000 } in packages/api/mcp/policy.ts. The installed @octokit/request 10 ignores that option and honours only an abort signal, so other MCP GitHub calls have no timeout. The finding's premise that no POST can outlive the Octokit timeout does not hold, which is why the issue POST now carries its own deadline.

No visual preview was produced, since the change is not visible in the UI.

---

🤖 Implementation Details

• Model: Claude Fable 5.1 (high)
• Requested By: @integry
• Time: 16m 37s
• Tokens: 4,825,436 (4,770,745 in / 54,691 out)
• Cost: $6.09
• Subscription usage: Fable +1%

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5913591843✓

[integry]

/review fable astra

[propr-dev]

✅ AI Code Review Complete requested by @integry

Posted 2 reviews:

• fable
• astra

View Task Details

[propr-dev]

🔍 AI Code Review — fable

Overall Evaluation

This epic PR delivers the "observable MCP surface": a structured error envelope with redaction, a durable operation lifecycle (lifecycle, timestamps, artifacts, progress, list_operations), PR-command pickup tracking and ultrafix cycle/outcome reconstruction, resumable publish_plan with a leased claim and marker-based issue adoption, submission progress, get_work_overview, optional expectedHead on append-only PR commands, specific merge preconditions, bundled/versioned docs tools, visual preview tools, plan-revision provenance, refinement output normalization, trigger-access configuration and a settings catalog. The implementation is careful about compare-and-set writes and stale-poll interleavings, and the test coverage for those races is unusually thorough. All current-head checks pass.

✅ Publication claim is fenced end to end — packages/api/mcp/toolsPlanning.ts renews the claim before every issue POST, aborts the POST at the lease deadline (withinPublicationLease), and recordIssue inserts into plan_issues only through a single INSERT … SELECT … WHERE status='executing' AND mcp_revision=? AND context_config=? statement, so a lost claim cannot append rows or release the draft; mcpPublishPlan.test.ts drives takeover, renewal races, process death and lease expiry against that logic.

✅ Stale-poll protection is real, not aspirational — operationTracking.trackExecution guards its receipt write with whereNotIn('state', terminal) (plus started_at/taskId guards for pickup timeouts) and adopts the winning durable row when it loses, and McpOperations.finish/recordProgress use lifecycle-gated CASE updates; mcpCommandProgress.test.ts and mcpOperations.test.ts pause a poll mid-GitHub-read and prove terminal evidence survives.

✅ Secret redaction is applied at every durable boundary — errorEnvelope.ts redacts messages, details and causes, toToolErrorResult re-redacts on the wire, and tracker writes go through redactDetails; tests assert MCP/GitHub tokens never reach mcp_operations or the response.

The PR needs one small correction to the new provenance feature before merge; everything else is suggestion-level.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F7: 🔴 Refinement writes plan_cause: 'refinement' even when the plan was not changed

• Required behavior: Issue Plan revision history: record what caused each revision (generation, refinement, manual edit, rename, restore) and expose it from get_plan #2605 — each revision's cause must record what actually created the live plan (generation, refinement, manual edit, rename, restore); the PR's own invariant (guarded by planProvenanceUpdates.test.ts) is that an unchanged plan keeps its existing cause.

• Evidence: packages/api/routes/plannerHelpers/refineBackground.ts:162 (persistActiveRefinement(... { plan_json, plan_cause: 'refinement', ... })) and packages/api/routes/plannerHelpers/handlers/generationHandlers.ts:118 (plan_cause: 'refinement')

  Static trace:

  1. A draft has a generated plan (plan_cause = 'generation', set by persistGenerationCompletion).
  2. The user asks a question during refinement; refinePlan returns action: 'answered' (or 'clarify'). Both handlers set normalized = { ok: true, plan: currentPlan, merged: false } — the plan content is unchanged, as the new tests answered in background preserves incomplete current tasks assert.
  3. Both handlers nevertheless persist plan_cause: 'refinement' unconditionally.
  4. get_plan.revisionHistory.currentCause, list_plan_revisions.currentCause and the UI (describeRevisionCause) now report the generated plan as "Refined"; on the next manual edit the trigger snapshots it with cause = OLD.plan_cause = 'refinement', so the history permanently shows a "Refined" version whose content came from generation.

  The HTTP and MCP update paths in this same PR avoid exactly this with CASE WHEN plan_json = ? THEN plan_cause ELSE 'manual_edit' END; the refinement paths have no equivalent guard, and neither plannerBackgroundLifecycle.test.ts nor planProvenanceUpdates.test.ts asserts plan_cause after an answered/clarify result. Unverified sibling: packages/core/src/services/taskPlanningService.ts:65 also writes plan_cause: 'generation' on every persistGenerationCompletion update; if that helper is also used for a failed regeneration that leaves plan_json untouched, the same mislabel applies there. Proposed regression: run runBackgroundRefinement with refine returning { action: 'answered' } on a draft whose plan_cause is 'generation' and assert plan_cause is still 'generation'.

• Minimum fix: In both refinement persistence sites, only write plan_cause: 'refinement' when result.action === 'modified' (or reuse the CASE WHEN plan_json = ? THEN plan_cause ELSE 'refinement' END expression already used by plannerRoutes.ts), and add the regression above; apply the same guard to persistGenerationCompletion if it is reachable without a plan_json change.

Suggestions

These are optional follow-ups and are not sent to /fix.

S16: 🟢 reconcileTerminalLifecycles scans every terminal receipt on each list_operations

tools.ts list_operations calls operations.reconcileTerminalLifecycles(principal) without an id, which loads and re-derives artifacts/failure/redaction for every terminal mcp_operations row of the owner/grant on every listing. That is correct but grows linearly with history and will make list_operations progressively slower for active operators; bounding the sweep (e.g. accepted_at >= now - sinceMinutes, or only rows whose lifecycle is still non-terminal or finished_at IS NULL) would keep the recovery behavior without the full scan.

S17: 🟢 Audit remaining consumers of the boolean ultrafixCycle flag

buildUltrafixHistoryMeta now stores ultrafixCycle as a cycle number instead of true; taskHistoryRoutes.findLatestMetadata was updated, but other readers of task-history metadata (activity/recent-activity summaries, goal/task detail, the UI) were not in the diff and may still compare === true. A grep-and-fix pass would prevent silent loss of the "ultrafix cycle" marker in views that were not covered by the updated tests.

S18: 🟢 Rename events consume the 50-row revision cap

The new task_drafts_name_history trigger inserts one row per name change and the plan-history trigger's DELETE keeps only the 50 newest rows of either kind. If the planner UI autosaves the title frequently, consecutive rename rows can evict real plan snapshots. Coalescing consecutive rename rows (like the edit-burst coalescing already applied to plan edits) or excluding rename rows from the cap would protect plan history.

S19: 🟢 Clarify runtime precedence for the both allowlist source

toolsTriggerAccess.ts and configRevision.effectiveGithubUserWhitelist treat the persisted github_user_whitelist as authoritative when both it and GITHUB_USER_WHITELIST exist, and the tool's notes claim an unsuffixed entry authorizes the matching [bot] login. packages/shared/src/userWhitelist.ts still reads only the environment variable and documents the opposite bot rule. Unless the settings→environment sync guarantees the persisted value wins everywhere (the test sets process.env by hand before calling filterCommentByAuthor), an MCP write could succeed while enforcement keeps using the environment list. Worth verifying and documenting in one place.

S20: 🟢 Drafts stuck in executing from pre-PR partial publications remain unrecoverable

publish_plan with resume: true requires a publication object in context_config; drafts that got stuck before this PR (the original incident) have none, so they still fail with PRECONDITION_FAILED and update_plan still refuses them. A one-time recovery path (treat an executing draft with no publication state and no active receipt as resumable by marker lookup, or allow releasing it back to review) would close the original complaint fully.

S21: 🟢 get_visual_preview depends on a raw user access token

toolsPreviews.ts throws GITHUB_CREDENTIAL_REQUIRED when principal.user.accessToken is absent, which may be the normal case for Connect-delegated principals that only carry an Octokit client. Either documenting this in docs/mcp.md/coverage or resolving the media token the same way previewMediaRoutes does via resolveGitHubMetadataToken would make the tool usable on those grants.

S22: 🟢 The ultrafix continuation catch path can now throw

ultrafixJobHelpers.ts and prCommentReviewJob.ts moved a patchUltrafixContinuationMeta call inside the catch that previously only logged. If that write rejects (e.g. state-manager error), the exception escapes the catch and can fail a job whose action already completed. Wrapping the terminal-metadata write in its own try/catch keeps the original best-effort semantics.

S23: 🟢 Whole-result redactDetails in tracker writes can mangle legitimate strings

trackExecution/trackTaskSubmission/trackCancellation now pass the entire tool result through redactDetails, which drops any key matching token|secret|… and replaces strings starting with / by their basename. recoveryReceipt in operations.ts deliberately limits this to diagnostic keys for that reason; aligning the tracker writes with that narrower sanitization would avoid surprises for tools whose results carry legitimate leading-slash paths or token-named settings.

S24: 🟢 Verify the plan-revision down migration preserves unrelated triggers

20261002000000_add_plan_revision_causes.js down uses knex dropColumn on task_drafts, which on SQLite rebuilds the table; triggers attached to the old table (notably the mcp_revision bump trigger from the MCP migration) may not be recreated. The test only asserts the plan-history trigger is restored. Using raw ALTER TABLE … DROP COLUMN or re-creating the other triggers would make rollback safe.

Score

The PR is large, well-designed and unusually well-tested, with all current-head checks green; the only required fix is a small, localized provenance mislabel in the new refinement persistence paths.

Score: 6/10

---

🤖 Review Details

• Model: Claude Fable 5.1
• Time: 11m 50s
• Tokens: 484,778 (428,912 in / 55,866 out)
• Cost: $8.15

View Task

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}

[propr-dev]

🔍 AI Code Review — astra

Overall Evaluation

This PR substantially expands MCP observability through durable lifecycles, structured errors, publication recovery, documentation, and visual previews. It needs changes before merge because terminal receipt responses can contradict their persisted lifecycle, and refinement answers can corrupt plan provenance.

✅ Shared preview authorization — HTTP and MCP media retrieval reuse the same association checks, trusted-host redirects, content validation, and size limits.

✅ Explicit uncertainty — Error classification preserves upstream causes while distinguishing uncertain mutation outcomes from definitive refusals.

✅ Targeted regression coverage — Added tests exercise concurrent receipt polling, publication-related recovery helpers, secret redaction, and both MCP protocol eras.

The supplied status reports 28 passed checks and no blocking failures or pending checks. This review is static; no commands were run. Seven explicitly omitted files, including two substantial test files, limit verification.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F8: 🔴 Terminal receipt responses can change outcome

• Required behavior: Issue MCP: give operation receipts a readable lifecycle (accepted, running, completed, failed, cancelled) with timestamps, artifacts and list_operations #2594 requires durable terminal lifecycle observations that do not regress; a receipt’s reported outcome must agree with its persisted lifecycle.

• Evidence: packages/api/mcp/tools.ts, get_operation and updateReceiptState; packages/api/mcp/operations.ts, finish.

  1. A generate_plan operation returns an accepted receipt. Generation finishes with the draft in review.
  2. Polling calls updateReceiptState, which reports completed. syncLifecycle persists lifecycle = completed, but finish does not change the operation’s underlying state, which remains accepted.
  3. The user subsequently generates the same draft again, and that generation fails, leaving the draft in failed.
  4. Polling the original receipt initially projects its durable completed lifecycle, then updateReceiptState sees row.state === 'accepted' and the draft’s new failure and overwrites receipt.state with failed.
  5. The terminal lifecycle guard correctly refuses to overwrite completed, but get_operation copies only the durable lifecycle back into the response. The caller receives state: failed alongside lifecycle.state: completed.

  The persisted terminal guard therefore protects the database lifecycle but not the public receipt outcome. No concurrent execution is necessary. static trace: followed the accepted-state predicate, lifecycle-only persistence, and final response assembly in the supplied diff.

• Minimum fix: Preserve the durable terminal outcome when refreshing an already-terminal receipt. Prevent draft observations from overriding it, and make the returned top-level state agree with the authoritative lifecycle.

F9: 🔴 Non-modifying answers overwrite plan provenance

• Required behavior: Issue Plan revision history: record what caused each revision (generation, refinement, manual edit, rename, restore) and expose it from get_plan #2605 requires revision causes to describe how each saved plan version was created. An answer or clarification that preserves the plan must preserve its existing cause.

• Evidence: packages/api/routes/plannerHelpers/refineBackground.ts, runBackgroundRefinement; packages/api/routes/plannerHelpers/handlers/generationHandlers.ts, createRefineHandler.

  1. A generated plan has plan_cause = 'generation'.
  2. A refinement request returns action: 'answered' or action: 'clarify'. Both changed handlers explicitly retain currentPlan.
  3. Both handlers nevertheless persist plan_cause: 'refinement' unconditionally.
  4. get_plan now reports that the unchanged plan was created by refinement. When a subsequent manual edit replaces it, the history trigger snapshots that plan with the incorrect refinement cause.

  Preserving the plan array does not preserve its provenance. The added answer/clarification tests assert plan contents and successful metadata, but do not assert that plan_cause remains unchanged. static trace: followed both persistence paths through the new trigger’s use of OLD.plan_cause.

• Minimum fix: Preserve plan_cause for answered and clarify in both handlers; write refinement provenance only when saving an actual refined plan.

Suggestions

These are optional follow-ups and are not sent to /fix.

S25: 🟢 Bound lifecycle recovery scans

list_operations calls reconcileTerminalLifecycles before applying its time window or pagination. That reconciliation loads every terminal receipt for the owner and grant and inspects its result. Consider bounding or incrementally processing recovery as receipt history grows. This is an optional scalability improvement; the supplied diff does not demonstrate a current operational failure.

Score

The implementation has substantial focused coverage and passing authoritative checks, but two verified inconsistencies remain in the new observability contract. Both admit localized corrections.

Score: 6/10

---

🤖 Review Details

• Model: GPT-6 Astra
• Time: 1m 38s
• Tokens: 197,493 (194,543 in / 2,950 out)
• Cost: $2.09
• Review scope: Partial — 7 files were omitted by the review context budget.

View Task

Files omitted from review diff

7 files were omitted from the review diff. Large, binary, generated, and lockfile changes are deprioritized.

Did not fit the review context budget

• packages/api/test/mcpPublishPlan.test.ts
• packages/api/test/mcpOperations.test.ts
• docs/mcp-operator-surface.md
• docs/mcp-coverage.md
• docs/mcp.md
• docs/docs/operations/settings-locations.md
• package-lock.json

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}

[integry]

/fix

[propr-dev]

✅ Applied the requested follow-up changes in commit a7b325c

Addressed 2 AI review comments (IDs: 5914687285, 5914718573)

Addressed findings F7, F8, F9

Summary of Changes

F7, F8 and F9 are fixed, with regressions for each. The API typecheck, lint on the changed files, the MCP suite and the planner suites pass, apart from one intermittent publish-plan lease test. That test failed in two of five runs; it uses mocked timers and never reaches the changed code.

What changed

• F7 and F9, plan provenance: both refinement save paths now share one helper in planRevisions.ts. An answered or clarify result keeps the existing plan_cause. A modified result writes refinement only when the saved plan differs from the stored one.
• F8, terminal receipts: get_operation no longer lets a later reading of the draft or goal replace a terminal lifecycle outcome. The guard covers generate_plan, refine_plan and create_goal receipts. The response's top-level state is also aligned to the durable lifecycle after the final re-read, which closes the window where a concurrent poll settles the receipt first.

Decisions worth knowing

• Answers still save the caller's plan. The editor sends its in-memory copy, which carries client-side task ids, so the stored JSON can change on an answer. I kept that write and preserved the cause, because the content still came from the original source.
• persistGenerationCompletion is unchanged. Its only caller always writes a freshly generated plan, so the mislabel is not reachable there.
• The state alignment is limited to receipts whose stored state stays accepted. Those record their outcome only in the lifecycle. Applying it to every receipt broke an existing test, described below.

Regressions added

• Provenance: answered and clarify keep generation in both handlers, including when the caller's copy with ids is saved. A later manual edit snapshots the old plan as generation, not refinement. modified records refinement, and an identical modified result keeps the prior cause.
• Receipts: a completed generation receipt stays completed after a later failed generation, and the reverse. A failed goal receipt stays failed after the goal is resumed and completes. A stale poll interleaved with a settling poll reports the durable outcome.

I confirmed the new tests fail when the fixes are removed.

Independent discoveries, not changed

• Retried task submissions disagree with their lifecycle. A failed create_task receipt whose submission is retried returns state: queued from get_operation while its lifecycle stays failed. An existing test that predates this PR asserts the queued result, so resolving it needs a product decision.
• Generation receipts are not matched to a run. From reading the code, a generate_plan receipt that was never polled would adopt the outcome of a later generation of the same draft. I did not reproduce this.
• Editor id normalisation may relabel plans. From reading the code, after a refinement the editor re-saves the plan with generated ids if tasks lack them, which the update route would record as a manual edit. I did not reproduce this.

No visual preview was produced, since the change is backend only.

---

🤖 Implementation Details

• Model: Claude Fable 5.1 (high)
• Requested By: @integry
• Time: 17m 44s
• Tokens: 7,262,462 (7,203,590 in / 58,872 out)
• Cost: $7.50
• Subscription usage: Fable +1%

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5914948627✓

[propr-dev]

/review fable

[propr-dev]

ℹ️ Analyzed the follow-up request by @propr-dev[bot]

Analysis Summary

Review of PR #2612 (branch 2593-epic-mcp-observability-cf6 against main, 107 files). I ran an eight-angle review pass, then verified each candidate against the code. The MCP operations, merge precondition and error envelope suites pass on this branch. No blocking defect. Two findings are worth fixing before merge, the rest are low severity.

Correctness

1. Merge read-phase failures are reported as an uncertain mutation. packages/api/mcp/toolsPullRequests.ts:358 calls the check-context pager before the merge PUT. That pager throws plain Error values when GitHub omits pageInfo, the commit id or the cursor, and the GraphQL read itself can return a 5xx. All of these reach the catch in packages/api/mcp/operations.ts:160, which classifies with side effects possible, so the receipt is stored as state unknown with OUTCOME_UNKNOWN and the message telling the caller to inspect the target before acting again. Nothing was mutated. A replay with the same key returns that unknown receipt. This is the exact symptom the epic set out to remove. Every mutation tool's read phase has the same exposure, but merge is the one where callers act on it. The publish tool already tracks whether the request was sent; a similar "no side effect yet" signal from the tool to the runner, or throwing a sub-500 McpError for the malformed-response cases, would let these fail as ordinary retryable errors.

2. In-flight receipts from before the upgrade never resolve. The old runner inserted rows with state running and a null result. The migration at packages/core/src/db/migrations/20261001000000_add_mcp_operation_lifecycle.js maps that to lifecycle accepted but leaves state running. Both invocationInterrupted at packages/api/mcp/operations.ts:86 and the interrupted-marking query at line 258 only consider state accepted, and the old updated_at staleness check is gone. Any operation that was mid-flight during a restart into the new version reports running with a retry hint forever. Mapping state = 'running' AND result IS NULL to accepted in the migration, or accepting both states in those two predicates, closes it.

3. A retryable PUBLISH_FAILED cannot be retried with the revision the caller holds. The release path at packages/api/mcp/toolsPlanning.ts:260 restores status and context but leaves the revision advanced by the claim and any renewals. A GitHub 429 or a busy database on the first issue yields retryable: true, yet a retry with the same expectedRevision fails with STALE_REVISION. Including the current revision in the error details is enough. The existing test avoids this by re-reading the draft.

Efficiency

4. Every list, get and replay reconciles the owner's whole terminal history. packages/api/mcp/tools.ts:368 calls the reconciler with no id. It loads, parses and re-canonicalises every completed, failed or cancelled row for the owner and grant, and issues a select per cancel_operation row, before the paged query runs. Cost grows with lifetime receipt count and ignores sinceMinutes. Bounding the reconcile to the requested window, or to rows whose lifecycle or finished_at still needs repair, keeps the call constant.

5. Terminal create_task receipts are rewritten on every poll. packages/api/mcp/toolsTaskSubmissions.ts:163 re-reads the submission and task and writes result and updated_at unconditionally. Skip the write when the refreshed projection equals the stored one. This also stops updated_at drifting, which the reconciler uses as finished_at evidence for rows that missed lifecycle sync.

Design and cleanup

6. packages/api/mcp/toolExecution.ts:27 now skips target authorisation whenever the target argument is absent. It serves the one-of schema in list_visual_previews, but it silently applies to any tool that declares a target with an optional argument. An explicit opt-in on the target declaration keeps the default fail-closed.

7. packages/api/mcp/pullRequestPreconditions.ts:214 rebuilds a snapshot from a snapshot and repeats the open-state check just to reach the draft guard. An inline draft check does the same.

8. packages/api/routes/plannerHelpers/handlers/generationHandlers.ts:101 and packages/api/routes/plannerHelpers/refineBackground.ts:132 re-run normalizeRefinedPlan on a plan that core refinePlan already normalised and summarised. The second pass always sees a complete plan, so the merged and operations fallbacks are dead. Using the result's merged and operations fields removes two copies of the block.

9. packages/api/mcp/toolsPlanning.ts:179 marks the prior attempt interrupted before computing whether it stopped, but that query requires a non-null result, which the marking never sets. The call cannot change the resume decision. Either drop it or make the intent explicit.

No code changes were necessary based on the current state of the branch.

---

🤖 Analysis Details

• Model: Claude Fable 5.1 (high)
• Time: 12m 27s
• Tokens: 2,539,066 (2,538,070 in / 996 out)
• Cost: $4.81
• Subscription usage: Weekly +1%, Fable +1%

View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5916805479✓

[integry]

Fix the review findings

[propr-dev]

✅ Applied the requested follow-up changes in commit 496e588

Summary of Changes

All nine findings from the latest review are fixed, each with a regression test. The API typecheck, lint on the changed files, and every MCP and planner suite I ran pass. The connect integration suite is skipped by design without a live routing repository.

What changed

Correctness

• Merge read-phase failures fail as ordinary errors. A new helper in the error envelope module tags any error thrown by a mutation's read phase as raised before side effects, and classifyError then keeps the ordinary code instead of OUTCOME_UNKNOWN. The shared pull-request read and the merge tool's GraphQL read plus check-context paging are wrapped. Malformed pagination now throws a stable GITHUB_RESPONSE_INVALID code, marked retryable, instead of a plain error.
• Pre-upgrade in-flight receipts settle. The lifecycle migration now maps the old runner's running rows without a result to accepted. The interruption predicates in the operations runner also accept running, so a database that already ran the migration is covered too.
• Retryable PUBLISH_FAILED reports the current revision. The release path reads the draft's revision after releasing the claim and includes it as details.currentRevision, so a retry does not hit STALE_REVISION.

Efficiency

• list_operations bounds its recovery sweep to the requested sinceMinutes window. Older receipts are still repaired on their own read or replay.
• Terminal receipts are no longer rewritten on every poll. The resolved create_task branch compares the refreshed projection canonically and skips the write when nothing changed. While testing this I found that artifact recording also rewrote updated_at every poll, so that write now only runs when the patch changes something.

Design and cleanup

• Target authorisation fails closed by default. A target must declare optional: true to skip the check on a missing argument. Only list_visual_previews opts in.
• The draft merge guard is inlined through a small shared helper instead of rebuilding a snapshot from a snapshot.
• Refinement normalisation lives in one place. A new helper in the planner helpers resolves what a refinement saves. A result that already reports merged came from core and is saved as is; anything else is normalised there. Both handlers now call it.
• The interrupted-marking call in publish_plan now runs after the stopped-attempt decision with a comment stating it only reclassifies the prior receipt for pollers. It was kept because an existing test asserts that observable effect.

Docs

The error table in docs/mcp.md lists the new GitHub code and the currentRevision detail, and the uncertainty section states that read-phase failures keep their ordinary code.

Verification

I confirmed the merge regression fails with the tagging removed. The line-count lint warning my change introduced in the planning tools file was trimmed back under the limit. No visual preview was produced because the change is backend only.

---

🤖 Implementation Details

• Model: Claude Fable 5.1 (high)
• Requested By: @integry
• Time: 28m 3s
• Tokens: 9,268,868 (9,206,437 in / 62,431 out)
• Cost: $7.93
• Subscription usage: Weekly +1%, Fable +2%

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5917055063✓

[integry]

/review fable

[propr-dev]

✅ AI Code Review Complete requested by @integry

Posted 1 review:

• fable

View Task Details

[propr-dev]

🔍 AI Code Review — fable

Overall Evaluation

This epic PR adds the "observable MCP surface": a structured, secret-redacted error envelope for every tool call, a persisted operation lifecycle (accepted/running/completed/failed/cancelled/unknown with timestamps, artifacts, progress) plus list_operations, durable ultrafix cycle progress and outcomes, a claim/lease/marker-based recovery protocol for publish_plan, submission progress and list_task_submissions, get_work_overview, optional expectedHead on append-only PR commands, specific merge preconditions, bundled/versioned docs tools, visual preview tools on top of an extracted preview-fetch service, plan revision causes, refinement-output validation/merging, trigger-access configuration tools, and a settings catalog with a generated docs page. The changed behaviour is backed by a very large regression suite (lifecycle races, stale polls, takeover races, process death, both SDK eras), and all current-head checks pass.

I traced the main changed paths (operation runner and recovery, tracker/lifecycle synchronization, publish claim/lease/fail paths, merge precondition ordering, refinement normalization, trigger-access revision handshake, docs indexing/chunking, preview rendering) and did not find a defect I can drive from a reachable trigger to an incorrect observable outcome. Caveat: several unchanged helpers the new code depends on (patchUltrafixContinuationMeta persisting to task_history, listScope never returning null for grant-wide listing, runTool authorizing args.repository for target-less tools such as get_visual_preview) are not in the supplied context; I relied on the passing suites and existing conventions for those.

✅ Publication recovery is fenced end to end — packages/api/mcp/toolsPlanning.ts guards every write (renewal, plan_issues insert via a single insert … select … where claim holds, partial/final updates) with the current mcp_revision + context_config token, aborts each issue POST at the lease deadline, and only permits takeover on persisted callback result, verified process death, or lapsed lease; mcpPublishPlan.test.ts exercises the owner-renews-during-takeover race and real child-process death.

✅ Read-phase failures no longer masquerade as uncertain outcomes — beforeSideEffects in errorEnvelope.ts tags errors raised before any external write so classifyError keeps their specific, often retryable, code, while genuinely post-write failures still persist OUTCOME_UNKNOWN with a sanitized cause; covered by mcpMergePreconditions.test.ts for GraphQL/REST read failures.

✅ Lifecycle writes are monotonic under concurrent polls — McpOperations.finish/markStarted/markUnknown/recordProgress are all guarded by lifecycle/state predicates, the pickup-timeout write is additionally fenced on started_at/continuation.taskId/artifacts.taskId, and a losing writer adopts the durable row instead of overwriting it (operationTracking.ts trackExecution, tests in mcpCommandProgress.test.ts and mcpOperations.test.ts).

The PR is merge-ready within scope; the items below are optional follow-ups.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S26: 🟢 Wire the new publication and provenance tests into a CI entry point

package.json adds many new files to test:mcp but not packages/api/test/mcpPublishPlan.test.ts, planProvenanceUpdates.test.ts, previewMediaFetch.test.ts, packages/core/test/refinementOutput.test.ts, test/buildDocsManifest.test.mjs, or test/ultrafixContinuationMeta.test.ts. On this PR the full test-suite shards were skipped, so the most intricate new logic (claim/lease/takeover) may only have run locally. Adding at least mcpPublishPlan.test.ts to test:mcp (or confirming the Validate job globs it) would make the evidence reproducible in CI. Optional because the code paths are otherwise well covered locally per the author's notes.

S27: 🟢 Reconcile ultrafixCycle typing and pickup-timeout docs

buildUltrafixHistoryMeta now writes ultrafixCycle as a number; taskHistoryRoutes.ts and the UI's truthiness check were adjusted, but propr-ui/src/components/TaskDetails/types.ts still types it as boolean and has no ultrafixOutcome. Also docs/mcp-coverage.md still says "Missing intake becomes unknown after two minutes" while PICKUP_DEADLINE_MS is now ten minutes for PR command receipts. A quick audit for other ultrafixCycle === true consumers (e.g. activity digests) and a doc touch-up would keep contracts consistent. Not blocking since runtime behaviour is correct where verified.

S28: 🟢 Narrow path-stripping redaction to path-like fields

redactDetailValue in packages/api/mcp/errorEnvelope.ts reduces any string that starts with / to its basename and strips absolute paths inside prose. It is now applied to whole tracked tool results (trackExecution, trackTaskSubmission) and to lifecycle progress/targetState. That is fine for the values seen today, but a task reason, submission error, or plan task title beginning with / would be silently mangled. Limiting basename reduction to keys like path/file (or documenting the behaviour) would reduce surprise; optional because no current field is demonstrably affected.

S29: 🟢 Avoid the duplicate isUltrafixAutomaticWorkCurrent call

In src/jobs/ultrafixLoopContinuation.ts the state-lost branch calls isUltrafixAutomaticWorkCurrent once for reason and again for outcome, which is redundant and could disagree if the epoch changes between the two Redis reads. Computing it once and deriving both fields is trivial cleanup.

S30: 🟢 Confirm HTTP preview route ordering is acceptable

The extracted previewMediaRoutes.ts now resolves the GitHub token before the enabled-repository check (previously a disabled repository returned 404 without touching credentials) and checks PreviewMediaError before handleGitHubRepositoryAccessError. Issue #2603 asked for no behaviour change; the visible responses are the same in the common cases, but a user without a GitHub credential hitting a disabled repository may now see the credential error instead of 404. Worth a glance at previewMediaRoutes.test.ts coverage; not a blocker.

S31: 🟢 Re-check other pull() callers for a dropped head assertion

pull() in toolsPullRequests.ts no longer enforces expectedHead; every mutation in the diff adds an explicit assertPullRequestHead. get_pull_request_revert_preview (not in the diff) also uses pull(); if its schema accepts expectedHead (the coverage doc describes "exact commit, comment and head"), the preview would silently stop enforcing it. It is a read tool, so the impact would be small, but confirming is cheap.

S32: 🟢 Investigate the non-blocking package validation failures

Several "Validate unsigned … package" and "Packaged Connect (darwin-arm64)" checks report non-blocking failures on this head. Adding sharp (with platform-specific @img/sharp-* optional binaries) to packages/api is the kind of change that can affect cross-platform packaging. If those checks were already failing on the base, ignore; otherwise this is worth a look before release.

S33: 🟢 Minor efficiency and exposure nits

list_visual_previews calls reader.enabledRepositories twice per request (once inline, once inside listPublishedPreviews). The active publication record stored in task_drafts.context_config includes owner.bootId/pidNamespace/pid, which the HTTP planner routes return to the plan owner; harmless but unnecessary host detail. Both are cosmetic.

S34: 🟢 Map GraphQL response errors to GitHub codes

classifyGithub requires a numeric status, so a real GraphqlResponseError (no status, but errors[].type such as NOT_FOUND/FORBIDDEN/RATE_LIMITED) falls through to INTERNAL_ERROR, and inside a mutation becomes OUTCOME_UNKNOWN without a GitHub-specific cause. Mapping errors[].type would give callers the same stable codes the REST path already produces.

S35: 🟢 Terminal ultrafix tasks without an outcome marker stay "running"

trackUltrafix derives the receipt state from durable ultrafixOutcome metadata (or the legacy Redis loop). A loop task cancelled while still queued (no worker ran handleUserCancellation) or failed through a path that does not add ultrafixTerminalMetadata leaves the receipt running with phase: 'paused'. Recording an outcome from the task-cancellation path, or treating a cancelled/failed latest task with no active loop as terminal, would close that gap. Pre-existing in spirit (Redis loop stayed active before), so optional.

Score

Large, carefully fenced implementation of the objective with extensive race and recovery coverage; no verified regression in the changed behaviour and all current checks pass, with only documentation/test-wiring follow-ups outstanding.

Score: 8/10

---

🤖 Review Details

• Model: Claude Fable 5.1
• Time: 11m 39s
• Tokens: 500,938 (446,429 in / 54,509 out)
• Cost: $8.31

View Task

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}

[integry]

/fix S26 S27 S28 S29 S30

[propr-dev]

✅ Applied the requested follow-up changes in commit 658ae50

Addressed 1 AI review comment (IDs: 5918986542)

Addressed suggestions S26, S27, S28, S29, S30

Summary of Changes

All five selected suggestions are applied, each with a regression where behaviour changed. The API, root worker and UI typechecks, lint on the changed files, and every affected test suite pass. Nothing here is visually perceptible, so no preview files were created.

S26, CI wiring. The three API suites for publish-plan claim/lease/takeover, plan provenance and the preview fetch service now sit in the test:mcp script, together with the root ultrafix continuation metadata test. The core refinement-output test and the docs manifest test went into test:unit, because the change classifier only allows api files in test:mcp. The full-suite discovery already globs all of these on every PR. The CI deduplication and classification tests confirm the new entries are discovered units.

S27, ultrafixCycle contract. The UI history metadata type now accepts a number or the legacy true, and gains the ultrafixOutcome union. The task-info flag stays boolean because the API normalizes it there. The only strict === true uses in the tree are test fixtures, and the activity digest never reads the field, so no other consumer needed changes. The coverage doc now says a PR command receipt goes unknown with a COMMAND_NOT_PICKED_UP failure after ten minutes.

S28, path redaction. Whole-string basename reduction now applies only to path-like keys such as path, logsPath, files, repoRoot or cwd, with array items inheriting their key. Free-text fields such as reason, error or a plan task title keep a leading slash, so a /merge or /fix S26 string survives intact. Multi-segment absolute paths embedded in prose are still stripped, so credential and home-directory leakage is unchanged. A regression covers both classes of key across the tracked-result wrappers.

S29, duplicate epoch read. The state-lost branch now reads the epoch once and derives reason and outcome from the same value. The regression simulates state cleared between load and record, asserts exactly one epoch read after the loss, and checks both pairings.

S30, preview route ordering. The route again checks visual-preview enablement before resolving the GitHub credential, so a user without a credential gets 404 for a disabled repository as before the extraction. The service still performs its own check, but the route hands it the result it already read, so enablement is read once per request. Keeping the preview-specific error ahead of the GitHub access handler is correct and now documented in code: the preview 404s were direct responses before, and the status-based handler would otherwise rewrite them into the repository-access 404. Two regressions cover the no-credential path, the single read, and the distinct 404 bodies.

One independent observation, not acted on: non-comment receipts still use a two-minute pickup window in the tracking code. That is separate from the PR command deadline the doc sentence describes and is outside the selected records.

---

🤖 Implementation Details

• Model: Claude Fable 5.1 (high)
• Requested By: @integry
• Time: 8m 19s
• Tokens: 3,062,977 (3,037,421 in / 25,556 out)
• Cost: $3.37
• Subscription usage: Session +3%

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5919124925✓

[integry]

/merge

[propr-dev]

🔀 Auto-merged main into 2593-epic-mcp-observability-cf6 (clean merge) in commit 3048b40

No conflicts were found — the merge was verified by an AI agent.

---

🤖 Verification Details

• Model: gpt-6-astra
• Time: 1m 38s

View Task Execution

---

System-triggered merge conflict resolution