Skip to content

[2594 by GPT-5.6 Sol] MCP: give operation receipts a readable lifecycle (accepted, running, completed, failed, cancelled) with timestamps, artifacts and list_operations - #2613

Merged
integry merged 19 commits into
2593-epic-mcp-observability-cf6from
2594/gpt-5.6-sol-mcp-give-operation-receip-20260929-0054-naw
Sep 29, 2026
Merged

integry merged 19 commits into
2593-epic-mcp-observability-cf6from
2594/gpt-5.6-sol-mcp-give-operation-receip-20260929-0054-naw

Conversation

@propr-dev

@propr-dev propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

AI Implementation Summary

Closes #2594

Branch: 2594/gpt-5.6-sol-mcp-give-operation-receip-20260929-0054-naw
Commits: ✅ Changes committed (8340d1d)


AI Processing Completed

Execution Details:

Summary:
Implemented GitHub issue #2594.

Key changes:

  • Added lifecycle migration with backfill and SQLite-safe rollback.
  • Added persisted lifecycle timestamps, failures, artifacts, and progress.
  • Added monotonic lifecycle transition helpers and tracker synchronization.
  • Extended all operation projections with the lifecycle block.
  • Added scoped, paginated list_operations with filtering and refresh hints.
  • Persisted task/PR/issue/comment artifacts and cancellation outcomes.
  • Added concurrency, pruning durability, migration, failure, and grant-isolation tests.
  • Updated manual test schemas for the new migration.

Validation:

  • API TypeScript check passes.
  • Full MCP suite passes: 114 tests.
  • No visual preview generated because this is an API/database-only change.

Detailed Logs:

  • Conversation: 149 messages
  • Session: 01a0eaa8-e814-72e2-a139-d67c1ed4c8e9

Log files stored at:

  • conversation: /tmp/claude-logs/issue-2594-2026-09-29T01-09-28-521Z-conversation.json
  • output: /tmp/claude-logs/issue-2594-2026-09-29T01-09-28-521Z-output.txt
Latest Conversation Messages

This PR was created automatically by ProPR after processing issue #2594.


💡 Need changes?

Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!

…e lifecycle

Implemented by ProPR AI using gpt-5.6-sol model.

Implementation completed successfully.
@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

/ultrafix
Triggered automatically by the requested execution settings.

@propr-dev propr-dev Bot added the ultrafix label Sep 29, 2026
@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /review

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

CI failed: Validate Changes

Please investigate and fix this CI failure.

Failure evidence

.github:8
Process completed with exit code 1.

.github:360
Process completed with exit code 1.

.github:1
"The ubuntu-latest label will migrate to Ubuntu 26 beginning October 19, 2026. For more information, see actions/runner-images#14748"

@github-actions

Copy link
Copy Markdown

Checks Failed

Validation failed during setup, tests, CLI packaging, linting, or build checks.

View Logs
Build check diagnostics for run 36506556170, attempt 1
Checkout outcome: success

--- GitHub Actions workflow lint ---
docker.io/rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667: Pulling from rhysd/actionlint
589002ba0eae: Pulling fs layer
c09af8888d6a: Pulling fs layer
4ccd7d76ca98: Pulling fs layer
b6b4b7b0e257: Pulling fs layer
c09af8888d6a: Verifying Checksum
c09af8888d6a: Download complete
4ccd7d76ca98: Verifying Checksum
4ccd7d76ca98: Download complete
589002ba0eae: Verifying Checksum
589002ba0eae: Download complete
b6b4b7b0e257: Verifying Checksum
b6b4b7b0e257: Download complete
589002ba0eae: Pull complete
c09af8888d6a: Pull complete
4ccd7d76ca98: Pull complete
b6b4b7b0e257: Pull complete
Digest: sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667
Status: Downloaded newer image for rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667
docker.io/rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667

--- Release shell script lint ---

--- Toolchain ---
v22.23.2
10.9.8

--- Dependency installation ---
npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated gar@1.0.4: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm warn deprecated glob@7.2.3: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me

added 1126 packages, and audited 1135 packages in 19s

322 packages are looking for funding
  run `npm fund` for details

3 moderate severity vulnerabilities

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

--- CLI release package ---
Verifying propr-cli release package...

> propr@0.8.15 cli:pack
> node packages/cli/scripts/build-publish.mjs


> @propr/shared@0.8.15 build
> tsc


> @propr/local-setup@0.8.15 build
> tsc


> @propr/cli@0.8.15 build
> tsc && node scripts/copy-assets.mjs

copy-assets: copied 9 file(s) into the CLI package.

Staged propr-cli@0.8.15 at /home/runner/work/propr/propr/dist-publish/propr-cli
npm notice
npm notice 📦  propr-cli@0.8.15
npm notice Tarball Contents
npm notice 25.7kB README.md
npm notice 36.3kB dist/agentSkill.js
npm notice 754B dist/api/agentRuntime.js
npm notice 7.1kB dist/api/agents.js
npm notice 1.2kB dist/api/agentTank.js
npm notice 10.9kB dist/api/client.js
npm notice 4.7kB dist/api/errors.js
npm notice 5.4kB dist/api/implement.js
npm notice 2.0kB dist/api/index.js
npm notice 1.9kB dist/api/logs.js
npm notice 5.7kB dist/api/plans.js
npm notice 4.2kB dist/api/relay.js
npm notice 10.4kB dist/api/repos.js
npm notice 11.1kB dist/api/settings.js
npm notice 1.6kB dist/api/syntheticPools.js
npm notice 1.8kB dist/api/system.js
npm notice 5.8kB dist/api/tasks.js
npm notice 2.9kB dist/api/todos.js
npm notice 86B dist/api/types.js
npm notice 313B dist/api/visualPreviewAuth.js
npm notice 26.1kB dist/assets/env.example.txt
npm notice 5.7kB dist/auth/githubLogin.js
npm notice 19.5kB dist/commands/agentCommands.js
npm notice 4.2kB dist/commands/agentPoolCommands.js
npm notice 4.3kB dist/commands/agentSkillCommands.js
npm notice 28.7kB dist/commands/agentValidation.js
npm notice 65.2kB dist/commands/checkCommands.js
npm notice 8.9kB dist/commands/configCommands.js
npm notice 14.6kB dist/commands/connectCommand.js
npm notice 2.4kB dist/commands/imageCommands.js
npm notice 7.2kB dist/commands/implementCommands.js
npm notice 1.6kB dist/commands/index.js
npm notice 6.0kB dist/commands/initCommands.js
npm notice 14.1kB dist/commands/initStack.js
npm notice 6.1kB dist/commands/logCommands.js
npm notice 24.9kB dist/commands/planCommands.js
npm notice 8.4kB dist/commands/relayCommands.js
npm notice 30.1kB dist/commands/repoCommands.js
npm notice 7.5kB dist/commands/runtimeCommands.js
npm notice 16.1kB dist/commands/settingCommands.js
npm notice 4.1kB dist/commands/setup/agentHostActions.js
npm notice 123B dist/commands/setup/agents.js
npm notice 4.5kB dist/commands/setup/engine.js
npm notice 51B dist/commands/setup/github.js
npm notice 15.3kB dist/commands/setup/hostActions.js
npm notice 25.3kB dist/commands/setup/sequential.js
npm notice 51B dist/commands/setup/state.js
npm notice 51B dist/commands/setup/types.js
npm notice 10.7kB dist/commands/setupCommand.js
npm notice 4.4kB dist/commands/stackCommands.js
npm notice 1.4kB dist/commands/startCommand.js
npm notice 13.5kB dist/commands/systemCommands.js
npm notice 2.2kB dist/commands/tankCommands.js
npm notice 43.2kB dist/commands/taskCommands.js
npm notice 29.6kB dist/commands/todoCommands.js
npm notice 31.0kB dist/commands/tunnelCommand.js
npm notice 2.7kB dist/commands/uiDocsCommands.js
npm notice 8.3kB dist/completion.js
npm notice 20.0kB dist/config/ConfigManager.js
npm notice 276B dist/config/index.js
npm notice 989B dist/config/rootKey.js
npm notice 345B dist/config/types.js
npm notice 42.9kB dist/connectIdentity.js
npm notice 25.0kB dist/connectRootAuthority.js
npm notice 32.5kB dist/connectWindowsAuthority.js
npm notice 2.2kB dist/desktopDiscovery.js
npm notice 12.4kB dist/desktopLocalSetup.js
npm notice 16.3kB dist/index.js
npm notice 4.5kB dist/native/darwin-authority-broker.c
npm notice 8.9kB dist/native/directory-operations.c
npm notice 50.1kB dist/native/prebuilds/darwin-arm64/connect-authority-broker
npm notice 54.2kB dist/native/prebuilds/darwin-arm64/directory-operations.node
npm notice 17.1kB dist/native/prebuilds/darwin-x64/connect-authority-broker
npm notice 24.3kB dist/native/prebuilds/darwin-x64/directory-operations.node
npm notice 28.9kB dist/native/prebuilds/linux-arm64/directory-operations.node
npm notice 17.4kB dist/native/prebuilds/linux-x64/directory-operations.node
npm notice 1.2kB dist/native/README.md
npm notice 3.1kB dist/orchestrator/format.js
npm notice 13.4kB dist/orchestrator/index.js
npm notice 307B dist/orchestrator/manifest.json
npm notice 117.7kB dist/orchestrator/orchestrator.mjs
npm notice 408B dist/orchestrator/types.js
npm notice 237B dist/skill/propr/agents/openai.yaml
npm notice 6.1kB dist/skill/propr/SKILL.md
npm notice 4.5kB dist/tui/AgentTableApp.js
npm notice 4.5kB dist/tui/app.js
npm notice 11.6kB dist/tui/CheckApp.js
npm notice 5.1kB dist/tui/render.js
npm notice 29.9kB dist/tui/SetupApp.js
npm notice 12.6kB dist/tui/SetupApp.test.js
npm notice 8.2kB dist/tui/StartApp.js
npm notice 3.6kB dist/utils/apiErrorPresentation.js
npm notice 12.0kB dist/utils/directoryDescriptor.js
npm notice 739B dist/utils/dockerPort.js
npm notice 5.0kB dist/utils/envFile.js
npm notice 520B dist/utils/index.js
npm notice 5.7kB dist/utils/io.js
npm notice 1.5kB dist/utils/nativeArtifact.js
npm notice 544B dist/utils/parseState.js
npm notice 417B dist/utils/positiveInteger.js
npm notice 4.5kB dist/utils/privateFilesystem.js
npm notice 4.2kB dist/utils/resolveProject.js
npm notice 7.2kB dist/vendor/local-setup/agents.js
npm notice 62.7kB dist/vendor/local-setup/engine.js
npm notice 5.0kB dist/vendor/local-setup/envFile.js
npm notice 9.1kB dist/vendor/local-setup/github.js
npm notice 188B dist/vendor/local-setup/index.js
npm notice 25.1kB dist/vendor/local-setup/publicInstanceIdentity.js
npm notice 14.6kB dist/vendor/local-setup/state.js
npm notice 2.8kB dist/vendor/local-setup/types.js
npm notice 1.0kB dist/vendor/shared/accountStatusTimestamp.js
npm notice 7.6kB dist/vendor/shared/activityEvents.js
npm notice 3.4kB dist/vendor/shared/agentLogin.js
npm notice 5.1kB dist/vendor/shared/apiOrigin.js
npm notice 8.5kB dist/vendor/shared/connectDiscovery.js
npm notice 254B dist/vendor/shared/demoMode.js
npm notice 4.3kB dist/vendor/shared/desktopPairing.js
npm notice 481B dist/vendor/shared/desktopTokenRevocation.js
npm notice 8.9kB dist/vendor/shared/events.js
npm notice 1.6kB dist/vendor/shared/githubAuthMode.js
npm notice 2.0kB dist/vendor/shared/githubEventIntakeMode.js
npm notice 10.7kB dist/vendor/shared/index.js
npm notice 214B dist/vendor/shared/instanceAuthorization.js
npm notice 54B dist/vendor/shared/instanceCatalog.js
npm notice 4.0kB dist/vendor/shared/intakeModePrerequisites.js
npm notice 2.9kB dist/vendor/shared/labelUtils.js
npm notice 18.1kB dist/vendor/shared/modelDefinitions.js
npm notice 3.5kB dist/vendor/shared/notificationLinks.js
npm notice 51.3kB dist/vendor/shared/notifications.js
npm notice 866B dist/vendor/shared/projectSlug.js
npm notice 3.8kB dist/vendor/shared/proprCompatibility.js
npm notice 13.0kB dist/vendor/shared/proprServiceUrls.js
npm notice 2.2kB dist/vendor/shared/publishedVisualPreviews.js
npm notice 2.3kB dist/vendor/shared/reasoningLevels.js
npm notice 9.4kB dist/vendor/shared/reviewContextBudget.js
npm notice 5.3kB dist/vendor/shared/reviewFeedbackIds.js
npm notice 1.7kB dist/vendor/shared/reviewPrompt.js
npm notice 861B dist/vendor/shared/sessionSecret.js
npm notice 603B dist/vendor/shared/statusKeys.js
npm notice 7.5kB dist/vendor/shared/syntheticAgents.js
npm notice 2.2kB dist/vendor/shared/taskIdentifiers.js
npm notice 647B dist/vendor/shared/taskLifecycle.js
npm notice 3.7kB dist/vendor/shared/usageTips.js
npm notice 503B dist/vendor/shared/usageTypes.js
npm notice 1.1kB dist/vendor/shared/userWhitelist.js
npm notice 758B dist/vendor/shared/validateRelayUrl.js
npm notice 2.2kB dist/vendor/shared/validateRoutingUrl.js
npm notice 7.1kB dist/vendor/shared/visualPreviewCapacity.js
npm notice 9.3kB dist/vendor/shared/voice.js
npm notice 222B dist/vendor/shared/workEvidence.js
npm notice 623B package.json
npm notice Tarball Details
npm notice name: propr-cli
npm notice version: 0.8.15
npm notice filename: propr-cli-0.8.15.tgz
npm notice package size: 374.9 kB
npm notice unpacked size: 1.6 MB
npm notice shasum: a7708f10fbc83e960597a760aaf1ced48de518d9
npm notice integrity: sha512-Sqsczg5ExNClv[...]OkFQAE5foglfw==
npm notice total files: 151
npm notice
propr-cli-0.8.15.tgz

Dry run only. Re-run with --publish to publish to npm.
✅ propr-cli release package verified
Starting changed-area validation process...
Reusing this job's @propr/shared build from CLI packaging.
Reusing this job's @propr/local-setup build from CLI packaging.

--- Core Service: Lint & Build ---
✅ Core Lint passed
✅ Core Source Build passed

--- Core Package: Build & Lint ---
✅ Core Package Build passed
✅ Core Package Lint passed

--- API: Build & Lint ---
✅ API Build passed
❌ API Lint FAILED

> @propr/api@0.8.15 lint
> eslint . --max-warnings 0


/home/runner/work/propr/propr/packages/api/mcp/operationLifecycle.ts
  88:8  warning  Async function 'syncLifecycle' has a complexity of 23. Maximum allowed is 20  complexity

/home/runner/work/propr/propr/packages/api/mcp/operations.ts
  54:3  warning  Async method 'run' has a complexity of 21. Maximum allowed is 20  complexity

✖ 2 problems (0 errors, 2 warnings)

ESLint found too many warnings (maximum: 0).
npm error Lifecycle script `lint` failed with error:
npm error code 1
npm error path /home/runner/work/propr/propr/packages/api
npm error workspace @propr/api@0.8.15
npm error location /home/runner/work/propr/propr/packages/api
npm error command failed
npm error command sh -c eslint . --max-warnings 0

--- Stage outcomes ---
Workflow lint: success
Release shell lint: success
Pre-setup gate: skipped
Node setup: success
Toolchain: success
Dependency install: success
Release metadata: success
CLI packaging: success
Change detection: success
Changed-area checks: failure

View Workflow

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit f97faa8

Processed 2 comments:

  • Comment 1 by @propr-dev[bot] (ID: 5881718524✓)
  • Comment 2 by @github-actions[bot] (ID: 5881718710✓)

Summary of Changes

Fixed the Validate Changes CI failure.

  • Refactored lifecycle outcome/cancellation logic in operationLifecycle.ts.
  • Extracted operation-state resolution in operations.ts.
  • Preserved existing behavior while bringing both functions below ESLint’s complexity limit.

Validation passed:

  • API strict lint (--max-warnings 0)
  • API typecheck
  • Focused operation tests: 6/6
  • Full MCP suite: 114/114
  • git diff --check

No visual preview was created because this is an internal API refactor with no user-visible changes.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-dev[bot], @github-actions[bot]
  • Time: 3m 58s
  • Tokens: 402,933 (399,140 in / 3,793 out)
  • Cost: $0.38

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 IDs: 5881718524✓, 5881718710✓

Fixed the `Validate Changes` CI failure.

- Refactored lifecycle outcome/cancellation logic in [operationLifecycle.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T01-11-33/packages/api/mcp/operationLifecycle.ts).
- Extracted operation-state resolution in [operations.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T01-11-33/packages/api/mcp/operations.ts).
- Preserved existing behavior while bringing both functions below ESLint’s complexity limit.

Validation passed:

- API strict lint (`--max-warnings 0`)
- API typecheck
- Focused operation tests: 6/6
- Full MCP suite: 114/114
- `git diff --check`

No visual preview was created because this is an internal API refactor with no user-visible changes.

PR: #2613
Comment by: @propr-dev[bot] (ID: 5881718524)
Comment by: @github-actions[bot] (ID: 5881718710)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR adds durable lifecycle storage and operation discovery, but needs significant corrections before merge. Several production paths can persist misleading lifecycle data, and the new listing endpoint bypasses authorization checks applied by get_operation.

✅ Terminal-state protection — Conditional updates prevent markStarted and subsequent finish calls from overwriting an already terminal lifecycle.

✅ Migration coverage — The added tests cover legacy-state backfill and SQLite rollback.

✅ Grant isolation — Listing explicitly filters by both owner and grant, with coverage for another grant belonging to the same user.

The supplied current checks show 20 passed and no failures or pending checks. This review uses static analysis only; no commands were run.

Merge blockers

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

F1: 🔴 Listing bypasses current receipt authorization

  • Required behavior: Operation discovery must preserve the current repository and tool-permission boundaries enforced when reading individual receipts.

  • Evidence: packages/api/mcp/tools.ts, list_operations query and projection.

    1. A user creates an operation while authorized for its repository or the original tool’s required permission.
    2. That authorization is subsequently removed while the user retains the same grant and read scope.
    3. get_operation would reject access through policy.repository or policy.requirePermission.
    4. Calling list_operations without a repository returns the stored receipt, including its result, artifacts, failure, and progress.

    Owner/grant filtering does not revalidate current authorization. The listing handler performs neither of the checks present in the neighboring getter, and omitting the repository argument leaves no explicit repository for dispatch to authorize. static trace: comparison of the supplied listing, getter, and read-only dispatch paths.

  • Minimum fix: Apply the getter’s current repository and original-tool permission rules to listed receipts, including cancellation-source authorization where applicable. Ensure filtering and pagination operate over authorized results.

F2: 🔴 Polling acceptance fabricates execution start

  • Required behavior: A lifecycle may become running only when backend execution has been observed.

  • Evidence: packages/api/mcp/operationLifecycle.ts:121-122, syncLifecycle; packages/api/mcp/operations.ts:69-70, initial insertion.

    1. run inserts a receipt with compatibility state running and lifecycle accepted, then awaits invoke(id).
    2. While that callback remains pending, a concurrent idempotent request retrieves the receipt and its operation ID.
    3. The caller polls get_operation before the callback returns. There is no result or backend target yet, so the projected compatibility state remains running.
    4. syncLifecycle interprets that wrapper state as execution evidence and persists running plus started_at.
    5. The callback subsequently returns a queued response. run updates the compatibility state to queued, but its accepted branch does not restore the lifecycle.

    The stored lifecycle now asserts execution started even though only acceptance was observed. The interruption point is the awaited callback; the insert is already committed, and no transaction prevents concurrent reads. static trace: the supplied insert, duplicate-request projection, getter, and synchronization paths.

  • Minimum fix: Distinguish the wrapper’s initial running state from tracker-confirmed execution, and call markStarted only with actual backend evidence.

F3: 🔴 Initial receipts omit already known artifacts

  • Required behavior: Persisted lifecycle artifacts must capture output handles already returned by the mutation.

  • Evidence: packages/api/mcp/operations.ts:74-100, run; packages/api/mcp/tools.ts:325, the sole lifecycle synchronization call.

    1. A mutation returns output handles, such as the supplied test response containing pullRequest: 42 and commentId: 99.
    2. run persists the result and lifecycle state but never invokes artifact extraction.
    3. Its response therefore contains lifecycle.artifacts: {}.
    4. Idempotent replay and list_operations project the same empty artifacts indefinitely unless someone separately calls get_operation.

    This also affects completed mutations, for which clients have no reason to poll. The artifact test manually invokes recordArtifacts, so it does not verify the production capture path. static trace: run, replay, listing projection, and the only call to syncLifecycle.

  • Minimum fix: Extract and persist known artifacts when saving the mutation result, before returning its initial receipt. Retain polling enrichment for handles discovered later.

F4: 🔴 Backend failures lose the lifecycle error

  • Required behavior: Observed backend failures must populate the durable lifecycle failure information when the tracker provides a reason.

  • Evidence: packages/api/mcp/operationLifecycle.ts:125-128, failure extraction; packages/api/mcp/operations.ts:132-139, finish.

    1. An accepted task reaches a failed history event with a populated reason.
    2. trackTask sets receipt.state to failed and exposes that reason in receipt.targetState; it does not create receipt.result.error.
    3. syncLifecycle reads only result.error and calls finish without failure information.
    4. finish permanently stores lifecycle failed with failure: null.

    Clients reading the new lifecycle failure field receive no explanation despite the tracker supplying one. A later finish cannot enrich this field because its predicate excludes terminal lifecycles. The added failure assertion covers a thrown invocation error, which follows a different path. static trace: supplied trackTask excerpt, synchronization, and conditional terminal update.

  • Minimum fix: Normalize tracker failure information into the public error envelope before finishing the operation, including task reasons and failed review results where available.

F5: 🔴 Tracker uncertainty never reaches lifecycle

  • Required behavior: The persisted lifecycle must reflect an unresolved execution outcome when tracking explicitly reports unknown.

  • Evidence: packages/api/mcp/operationLifecycle.ts:119-131, syncLifecycle.

    1. A queued operation starts with lifecycle accepted.
    2. Its tracker cannot find an execution task after the tracking timeout, or cannot resolve its queued job.
    3. trackExecution sets and persists compatibility state unknown.
    4. syncLifecycle handles only starting and terminal outcomes, leaving lifecycle accepted.
    5. list_operations subsequently includes this uncertain operation in its active results and excludes it from the unknown filter.

    Repeated polling cannot repair the mismatch because there is no synchronization branch for uncertainty. In contrast, invocation-time uncertainty explicitly updates the lifecycle in run. static trace: supplied tracker timeout/queue paths, synchronization branches, and listing filters.

  • Minimum fix: Persist tracker-reported uncertainty for nonterminal operations, while preserving observed timestamps and protecting terminal outcomes. Allow subsequent confirmed tracking to resolve that uncertainty.

Suggestions

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

S1: 🟢 Document the lifecycle contract

Update docs/mcp.md with the lifecycle fields, their relationship to compatibility state, and the filtering and refresh behavior of list_operations. The existing documentation describes only the older receipt contract; this is an optional documentation follow-up.

Score

The migration and conditional terminal updates provide a useful foundation, and current checks pass. Authorization and lifecycle synchronization defects still prevent merge readiness.

Score: 5/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 26s
  • Tokens: 58,724 (56,081 in / 2,643 out)
  • Cost: $0.69

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

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit db33843

Addressed 1 AI review comment (IDs: 5881894266)

Addressed findings F1, F2, F3, F4, F5

Summary of Changes

Implemented F1–F5 only.

  • list_operations now revalidates repository, tool permission, and cancellation-source authorization before pagination.
  • Initial receipts remain accepted until backend execution is observed.
  • Initial mutation artifacts are persisted before returning and survive replay.
  • Tracker task/review failures populate durable error envelopes.
  • Tracker unknown states persist, preserve timestamps and terminal outcomes, and can resolve from later evidence.
  • Added focused authorization, pagination, concurrency, artifact, failure, and uncertainty regressions.

Validation:

  • API typecheck passed.
  • Focused operation tests: 10/10 passed.
  • Full MCP suite: 118/118 passed.
  • git diff --check passed.
  • No visual preview needed.
  • Independent discoveries: none.

🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-ultrafix
  • Time: 10m 20s
  • Tokens: 2,927,951 (2,906,677 in / 21,274 out)
  • Cost: $2.38

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

Implemented F1–F5 only.

- `list_operations` now revalidates repository, tool permission, and cancellation-source authorization before pagination.
- Initial receipts remain `accepted` until backend execution is observed.
- Initial mutation artifacts are persisted before returning and survive replay.
- Tracker task/review failures populate durable error envelopes.
- Tracker `unknown` states persist, preserve timestamps and terminal outcomes, and can resolve from later evidence.
- Added focused authorization, pagination, concurrency, artifact, failure, and uncertainty regressions.

Validation:

- API typecheck passed.
- Focused operation tests: 10/10 passed.
- Full MCP suite: 118/118 passed.
- `git diff --check` passed.
- No visual preview needed.
- Independent discoveries: none.

PR: #2613
Comment by: @propr-ultrafix (ID: 0)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

CI failed: Validate Changes

Please investigate and fix this CI failure.

Failure evidence

.github:8
Process completed with exit code 1.

.github:360
Process completed with exit code 1.

.github:1
"The ubuntu-latest label will migrate to Ubuntu 26 beginning October 19, 2026. For more information, see actions/runner-images#14748"

@github-actions

Copy link
Copy Markdown

Checks Failed

Validation failed during setup, tests, CLI packaging, linting, or build checks.

View Logs
Build check diagnostics for run 36508988156, attempt 1
Checkout outcome: success

--- GitHub Actions workflow lint ---
docker.io/rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667: Pulling from rhysd/actionlint
589002ba0eae: Pulling fs layer
c09af8888d6a: Pulling fs layer
4ccd7d76ca98: Pulling fs layer
b6b4b7b0e257: Pulling fs layer
b6b4b7b0e257: Waiting
4ccd7d76ca98: Verifying Checksum
4ccd7d76ca98: Download complete
c09af8888d6a: Verifying Checksum
c09af8888d6a: Download complete
589002ba0eae: Verifying Checksum
589002ba0eae: Download complete
589002ba0eae: Pull complete
c09af8888d6a: Pull complete
b6b4b7b0e257: Verifying Checksum
b6b4b7b0e257: Download complete
4ccd7d76ca98: Pull complete
b6b4b7b0e257: Pull complete
Digest: sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667
Status: Downloaded newer image for rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667
docker.io/rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667

--- Release shell script lint ---

--- Toolchain ---
v22.23.2
10.9.8

--- Dependency installation ---
npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated gar@1.0.4: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm warn deprecated glob@7.2.3: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me

added 1126 packages, and audited 1135 packages in 12s

322 packages are looking for funding
  run `npm fund` for details

3 moderate severity vulnerabilities

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

--- CLI release package ---
Verifying propr-cli release package...

> propr@0.8.15 cli:pack
> node packages/cli/scripts/build-publish.mjs


> @propr/shared@0.8.15 build
> tsc


> @propr/local-setup@0.8.15 build
> tsc


> @propr/cli@0.8.15 build
> tsc && node scripts/copy-assets.mjs

copy-assets: copied 9 file(s) into the CLI package.

Staged propr-cli@0.8.15 at /home/runner/work/propr/propr/dist-publish/propr-cli
npm notice
npm notice 📦  propr-cli@0.8.15
npm notice Tarball Contents
npm notice 25.7kB README.md
npm notice 36.3kB dist/agentSkill.js
npm notice 754B dist/api/agentRuntime.js
npm notice 7.1kB dist/api/agents.js
npm notice 1.2kB dist/api/agentTank.js
npm notice 10.9kB dist/api/client.js
npm notice 4.7kB dist/api/errors.js
npm notice 5.4kB dist/api/implement.js
npm notice 2.0kB dist/api/index.js
npm notice 1.9kB dist/api/logs.js
npm notice 5.7kB dist/api/plans.js
npm notice 4.2kB dist/api/relay.js
npm notice 10.4kB dist/api/repos.js
npm notice 11.1kB dist/api/settings.js
npm notice 1.6kB dist/api/syntheticPools.js
npm notice 1.8kB dist/api/system.js
npm notice 5.8kB dist/api/tasks.js
npm notice 2.9kB dist/api/todos.js
npm notice 86B dist/api/types.js
npm notice 313B dist/api/visualPreviewAuth.js
npm notice 26.1kB dist/assets/env.example.txt
npm notice 5.7kB dist/auth/githubLogin.js
npm notice 19.5kB dist/commands/agentCommands.js
npm notice 4.2kB dist/commands/agentPoolCommands.js
npm notice 4.3kB dist/commands/agentSkillCommands.js
npm notice 28.7kB dist/commands/agentValidation.js
npm notice 65.2kB dist/commands/checkCommands.js
npm notice 8.9kB dist/commands/configCommands.js
npm notice 14.6kB dist/commands/connectCommand.js
npm notice 2.4kB dist/commands/imageCommands.js
npm notice 7.2kB dist/commands/implementCommands.js
npm notice 1.6kB dist/commands/index.js
npm notice 6.0kB dist/commands/initCommands.js
npm notice 14.1kB dist/commands/initStack.js
npm notice 6.1kB dist/commands/logCommands.js
npm notice 24.9kB dist/commands/planCommands.js
npm notice 8.4kB dist/commands/relayCommands.js
npm notice 30.1kB dist/commands/repoCommands.js
npm notice 7.5kB dist/commands/runtimeCommands.js
npm notice 16.1kB dist/commands/settingCommands.js
npm notice 4.1kB dist/commands/setup/agentHostActions.js
npm notice 123B dist/commands/setup/agents.js
npm notice 4.5kB dist/commands/setup/engine.js
npm notice 51B dist/commands/setup/github.js
npm notice 15.3kB dist/commands/setup/hostActions.js
npm notice 25.3kB dist/commands/setup/sequential.js
npm notice 51B dist/commands/setup/state.js
npm notice 51B dist/commands/setup/types.js
npm notice 10.7kB dist/commands/setupCommand.js
npm notice 4.4kB dist/commands/stackCommands.js
npm notice 1.4kB dist/commands/startCommand.js
npm notice 13.5kB dist/commands/systemCommands.js
npm notice 2.2kB dist/commands/tankCommands.js
npm notice 43.2kB dist/commands/taskCommands.js
npm notice 29.6kB dist/commands/todoCommands.js
npm notice 31.0kB dist/commands/tunnelCommand.js
npm notice 2.7kB dist/commands/uiDocsCommands.js
npm notice 8.3kB dist/completion.js
npm notice 20.0kB dist/config/ConfigManager.js
npm notice 276B dist/config/index.js
npm notice 989B dist/config/rootKey.js
npm notice 345B dist/config/types.js
npm notice 42.9kB dist/connectIdentity.js
npm notice 25.0kB dist/connectRootAuthority.js
npm notice 32.5kB dist/connectWindowsAuthority.js
npm notice 2.2kB dist/desktopDiscovery.js
npm notice 12.4kB dist/desktopLocalSetup.js
npm notice 16.3kB dist/index.js
npm notice 4.5kB dist/native/darwin-authority-broker.c
npm notice 8.9kB dist/native/directory-operations.c
npm notice 50.1kB dist/native/prebuilds/darwin-arm64/connect-authority-broker
npm notice 54.2kB dist/native/prebuilds/darwin-arm64/directory-operations.node
npm notice 17.1kB dist/native/prebuilds/darwin-x64/connect-authority-broker
npm notice 24.3kB dist/native/prebuilds/darwin-x64/directory-operations.node
npm notice 28.9kB dist/native/prebuilds/linux-arm64/directory-operations.node
npm notice 17.4kB dist/native/prebuilds/linux-x64/directory-operations.node
npm notice 1.2kB dist/native/README.md
npm notice 3.1kB dist/orchestrator/format.js
npm notice 13.4kB dist/orchestrator/index.js
npm notice 307B dist/orchestrator/manifest.json
npm notice 117.7kB dist/orchestrator/orchestrator.mjs
npm notice 408B dist/orchestrator/types.js
npm notice 237B dist/skill/propr/agents/openai.yaml
npm notice 6.1kB dist/skill/propr/SKILL.md
npm notice 4.5kB dist/tui/AgentTableApp.js
npm notice 4.5kB dist/tui/app.js
npm notice 11.6kB dist/tui/CheckApp.js
npm notice 5.1kB dist/tui/render.js
npm notice 29.9kB dist/tui/SetupApp.js
npm notice 12.6kB dist/tui/SetupApp.test.js
npm notice 8.2kB dist/tui/StartApp.js
npm notice 3.6kB dist/utils/apiErrorPresentation.js
npm notice 12.0kB dist/utils/directoryDescriptor.js
npm notice 739B dist/utils/dockerPort.js
npm notice 5.0kB dist/utils/envFile.js
npm notice 520B dist/utils/index.js
npm notice 5.7kB dist/utils/io.js
npm notice 1.5kB dist/utils/nativeArtifact.js
npm notice 544B dist/utils/parseState.js
npm notice 417B dist/utils/positiveInteger.js
npm notice 4.5kB dist/utils/privateFilesystem.js
npm notice 4.2kB dist/utils/resolveProject.js
npm notice 7.2kB dist/vendor/local-setup/agents.js
npm notice 62.7kB dist/vendor/local-setup/engine.js
npm notice 5.0kB dist/vendor/local-setup/envFile.js
npm notice 9.1kB dist/vendor/local-setup/github.js
npm notice 188B dist/vendor/local-setup/index.js
npm notice 25.1kB dist/vendor/local-setup/publicInstanceIdentity.js
npm notice 14.6kB dist/vendor/local-setup/state.js
npm notice 2.8kB dist/vendor/local-setup/types.js
npm notice 1.0kB dist/vendor/shared/accountStatusTimestamp.js
npm notice 7.6kB dist/vendor/shared/activityEvents.js
npm notice 3.4kB dist/vendor/shared/agentLogin.js
npm notice 5.1kB dist/vendor/shared/apiOrigin.js
npm notice 8.5kB dist/vendor/shared/connectDiscovery.js
npm notice 254B dist/vendor/shared/demoMode.js
npm notice 4.3kB dist/vendor/shared/desktopPairing.js
npm notice 481B dist/vendor/shared/desktopTokenRevocation.js
npm notice 8.9kB dist/vendor/shared/events.js
npm notice 1.6kB dist/vendor/shared/githubAuthMode.js
npm notice 2.0kB dist/vendor/shared/githubEventIntakeMode.js
npm notice 10.7kB dist/vendor/shared/index.js
npm notice 214B dist/vendor/shared/instanceAuthorization.js
npm notice 54B dist/vendor/shared/instanceCatalog.js
npm notice 4.0kB dist/vendor/shared/intakeModePrerequisites.js
npm notice 2.9kB dist/vendor/shared/labelUtils.js
npm notice 18.1kB dist/vendor/shared/modelDefinitions.js
npm notice 3.5kB dist/vendor/shared/notificationLinks.js
npm notice 51.3kB dist/vendor/shared/notifications.js
npm notice 866B dist/vendor/shared/projectSlug.js
npm notice 3.8kB dist/vendor/shared/proprCompatibility.js
npm notice 13.0kB dist/vendor/shared/proprServiceUrls.js
npm notice 2.2kB dist/vendor/shared/publishedVisualPreviews.js
npm notice 2.3kB dist/vendor/shared/reasoningLevels.js
npm notice 9.4kB dist/vendor/shared/reviewContextBudget.js
npm notice 5.3kB dist/vendor/shared/reviewFeedbackIds.js
npm notice 1.7kB dist/vendor/shared/reviewPrompt.js
npm notice 861B dist/vendor/shared/sessionSecret.js
npm notice 603B dist/vendor/shared/statusKeys.js
npm notice 7.5kB dist/vendor/shared/syntheticAgents.js
npm notice 2.2kB dist/vendor/shared/taskIdentifiers.js
npm notice 647B dist/vendor/shared/taskLifecycle.js
npm notice 3.7kB dist/vendor/shared/usageTips.js
npm notice 503B dist/vendor/shared/usageTypes.js
npm notice 1.1kB dist/vendor/shared/userWhitelist.js
npm notice 758B dist/vendor/shared/validateRelayUrl.js
npm notice 2.2kB dist/vendor/shared/validateRoutingUrl.js
npm notice 7.1kB dist/vendor/shared/visualPreviewCapacity.js
npm notice 9.3kB dist/vendor/shared/voice.js
npm notice 222B dist/vendor/shared/workEvidence.js
npm notice 623B package.json
npm notice Tarball Details
npm notice name: propr-cli
npm notice version: 0.8.15
npm notice filename: propr-cli-0.8.15.tgz
npm notice package size: 374.9 kB
npm notice unpacked size: 1.6 MB
npm notice shasum: 5a3aa38403d32be1961439b66926a5c3fde56ffe
npm notice integrity: sha512-NVpe7E2ikUwMI[...]IiU9xTFihxing==
npm notice total files: 151
npm notice
propr-cli-0.8.15.tgz

Dry run only. Re-run with --publish to publish to npm.
✅ propr-cli release package verified
Starting changed-area validation process...
Reusing this job's @propr/shared build from CLI packaging.
Reusing this job's @propr/local-setup build from CLI packaging.

--- Core Service: Lint & Build ---
✅ Core Lint passed
✅ Core Source Build passed

--- Core Package: Build & Lint ---
✅ Core Package Build passed
✅ Core Package Lint passed

--- API: Build & Lint ---
✅ API Build passed
❌ API Lint FAILED

> @propr/api@0.8.15 lint
> eslint . --max-warnings 0


/home/runner/work/propr/propr/packages/api/mcp/tools.ts
  432:1  warning  File has too many lines (425). Maximum allowed is 400  max-lines

✖ 1 problem (0 errors, 1 warning)

ESLint found too many warnings (maximum: 0).
npm error Lifecycle script `lint` failed with error:
npm error code 1
npm error path /home/runner/work/propr/propr/packages/api
npm error workspace @propr/api@0.8.15
npm error location /home/runner/work/propr/propr/packages/api
npm error command failed
npm error command sh -c eslint . --max-warnings 0

--- Stage outcomes ---
Workflow lint: success
Release shell lint: success
Pre-setup gate: skipped
Node setup: success
Toolchain: success
Dependency install: success
Release metadata: success
CLI packaging: success
Change detection: success
Changed-area checks: failure

View Workflow

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit e518a56

Processed 2 comments:

  • Comment 1 by @propr-dev[bot] (ID: 5882022708✓)
  • Comment 2 by @github-actions[bot] (ID: 5882023203✓)

Summary of Changes

Fixed the Validate Changes failure by adding the repository-standard documented max-lines exemption to the centralized MCP tool catalog in tools.ts.

Validation passed:

  • API lint with --max-warnings 0
  • API TypeScript build
  • git diff --check

No visual preview was generated because this is a lint-only, non-visual change.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-dev[bot], @github-actions[bot]
  • Time: 2m 13s
  • Tokens: 372,119 (370,074 in / 2,045 out)
  • Cost: $0.30

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 IDs: 5882022708✓, 5882023203✓

…ry-standard documented `max-lines` exemption to the centralized MCP tool catalog in [tools.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T01-41-50/packages/api/mcp/tools.ts:1).

Fixed the `Validate Changes` failure by adding the repository-standard documented `max-lines` exemption to the centralized MCP tool catalog in [tools.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T01-41-50/packages/api/mcp/tools.ts:1).

Validation passed:

- API lint with `--max-warnings 0`
- API TypeScript build
- `git diff --check`

No visual preview was generated because this is a lint-only, non-visual change.

PR: #2613
Comment by: @propr-dev[bot] (ID: 5882022708)
Comment by: @github-actions[bot] (ID: 5882023203)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR adds durable lifecycle fields, tracker synchronization, and scoped operation discovery. It needs changes before merge because interrupted invocations can now remain accepted indefinitely.

✅ Terminal-state protection — Conditional lifecycle updates prevent later start or uncertainty observations from overwriting a terminal outcome.

✅ Scoped discovery — list_operations filters by owner and grant, then checks current repository and tool permissions before pagination.

✅ Migration coverage — Added tests cover SQLite backfill and rollback, concurrent transitions, and receipt durability after history deletion. The authoritative current checks report no failures or pending checks.

Merge blockers

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

F6: 🔴 Interrupted invocations remain accepted forever

  • Required behavior: Durable receipts must represent interrupted or uncertain execution honestly and retain the existing interruption-detection behavior.

  • Evidence: packages/api/mcp/operations.ts:69, McpOperations.run; packages/api/mcp/operations.ts:162, McpOperations.project.

    1. A mutation inserts its receipt with the newly introduced state: 'accepted', then awaits invoke(id).
    2. The process stops during that invocation, before the result update. The persisted receipt has state: 'accepted' and a null result; the external action may already have occurred.
    3. After restart and more than 120 seconds, project() still reports accepted because its interruption check applies exclusively to row.state === 'running'.
    4. get_operation cannot resolve this receipt: the supplied execution and cancellation trackers return when row.result is absent, and syncLifecycle() does not transition an accepted receipt without evidence. Reusing the idempotency key also returns the same receipt without invoking the callback.

    The operation therefore remains accepted indefinitely, continues requesting polling, and appears in active-operation discovery despite having no surviving invocation or known outcome. Before this change, the initially persisted running state reached the existing unknown projection after the timeout. Idempotency correctly prevents replay but does not resolve the misleading status.

    static trace: Verified against the supplied insertion, projection, tracker guards, and synchronization paths; no commands or tests were run.

  • Minimum fix: Extend interruption handling to accepted receipts whose invocation result was never persisted, using an invocation timestamp that polling does not refresh. Persist or consistently project unknown for these receipts without treating legitimate queued operations as interrupted or replaying their effects.

Suggestions

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

S2: 🟢 Document the lifecycle contract

Update docs/mcp.md with the lifecycle block, list_operations filters, and its explicit lack of tracker refresh. The existing documentation describes only legacy receipt states; documenting the distinction would help callers interpret both fields. This is an optional documentation follow-up.

S3: 🟢 Exercise stale progress observations

Add coverage for a stale poll recording progress after another poll records terminal progress. recordProgress() updates unconditionally, whereas lifecycle transitions have terminal guards. Clarifying whether progress means the latest write or the newest backend observation would strengthen the concurrency contract; the supplied objective does not establish a required ordering for progress snapshots.

Score

The implementation has useful authorization and lifecycle protections, with passing current checks, but the interrupted-invocation regression undermines the new durable status contract. A localized correction is needed before merge.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 58s
  • Tokens: 58,904 (57,152 in / 1,752 out)
  • Cost: $0.66

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

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 855dc77

Addressed 1 AI review comment (IDs: 5882180565)

Addressed finding F6

Summary of Changes

Implemented F6 only.

  • Stale accepted receipts with no result now become durably unknown after 120 seconds using stable accepted_at.
  • Replay, get_operation, projection, and list_operations consistently reflect interruption.
  • Acknowledged queued operations remain active.
  • Late authoritative invocation results resolve the timeout race correctly.
  • Added focused regression coverage in mcpOperations.test.ts.

Validation passed:

  • Focused operation tests: 11/11
  • Full MCP suite: 119/119
  • API typecheck
  • API lint
  • git diff --check

No visual preview was created because this is backend-only. No independent issues discovered.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-ultrafix
  • Time: 6m 19s
  • Tokens: 1,857,655 (1,846,300 in / 11,355 out)
  • Cost: $1.29

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

Implemented F6 only.

- Stale `accepted` receipts with no result now become durably `unknown` after 120 seconds using stable `accepted_at`.
- Replay, `get_operation`, projection, and `list_operations` consistently reflect interruption.
- Acknowledged queued operations remain active.
- Late authoritative invocation results resolve the timeout race correctly.
- Added focused regression coverage in [mcpOperations.test.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T01-58-26/packages/api/test/mcpOperations.test.ts:149).

Validation passed:

- Focused operation tests: 11/11
- Full MCP suite: 119/119
- API typecheck
- API lint
- `git diff --check`

No visual preview was created because this is backend-only. No independent issues discovered.

PR: #2613
Comment by: @propr-ultrafix (ID: 0)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR adds durable lifecycle metadata, tracker synchronization, and scoped operation discovery. It needs a localized correction before merge: interrupted receipts created before the migration are misclassified by the new lifecycle API.

✅ Scoped discovery — Listing checks owner, grant, current repository access, and tool permissions before pagination.

✅ Terminal transition protection — Conditional lifecycle updates prevent later start or uncertainty observations from replacing persisted terminal outcomes.

✅ Focused coverage — Added tests cover SQLite rollback, concurrent transitions, grant isolation, and persistence after task-history deletion. The supplied current-head status reports 20 passed checks and no failures or pending checks.

Merge blockers

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

F7: 🔴 Reconcile interrupted legacy receipts

  • Required behavior: The persisted lifecycle and lifecycle filters must honestly represent interrupted operations, including receipts backfilled during migration.

  • Evidence: packages/core/src/db/migrations/20261001000000_add_mcp_operation_lifecycle.js:15, packages/api/mcp/operations.ts:125, packages/api/mcp/operations.ts:187

    1. Before upgrading, a process exits after the old run implementation inserts state: 'running', but before it writes a result. The receipt remains running with result: null.
    2. After more than two minutes, the migration maps that receipt to lifecycle accepted.
    3. list_operations calls markInterruptedInvocations, whose predicate only accepts legacy state accepted. It therefore leaves this older receipt untouched.
    4. The active filter includes the receipt, while the unknown filter excludes it. Projection simultaneously returns compatibility state unknown and lifecycle state accepted.
    5. Polling get_operation initially changes its lifecycle to unknown through syncLifecycle. That update refreshes updated_at without changing legacy state running. An immediate subsequent poll therefore reports compatibility state running and restores a retry hint, despite there being no result or backend execution evidence.

    The interruption checks exclude legacy running rows, and the existing timeout test creates only the new accepted form. static trace: verified against the old insertion shown in the diff, migration backfill, interruption predicates, projection, and synchronization paths. No commands or tests were executed.

  • Minimum fix: Reconcile stale, result-less legacy running receipts alongside new accepted receipts, durably setting both states to unknown. Add a migration regression covering active/unknown filtering and repeated polling of that legacy row.

Suggestions

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

S4: 🟢 Document lifecycle discovery

Update docs/mcp.md with the lifecycle block, list_operations filters, and its explicit refresh behavior. The tool descriptions already explain these semantics, so this is an optional documentation improvement.

Score

The implementation has useful transition safeguards and substantial focused coverage, with all reported checks passing. The demonstrated upgrade-path inconsistency needs correction before merge.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 10s
  • Tokens: 63,094 (61,217 in / 1,877 out)
  • Cost: $0.60

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

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 44e71e6

Addressed 1 AI review comment (IDs: 5882380709)

Addressed finding F7

Summary of Changes

Implemented F7 only.

  • Legacy result-less running receipts now share the interruption predicate with accepted receipts and are durably transitioned to compatibility/lifecycle unknown (operations.ts, operations.ts).
  • Added a migration regression covering backfill, active/unknown filtering, repeated polling, retry-hint removal, and persisted state (mcpOperations.test.ts).

Validation:

  • Focused operation suite: 12/12 passed.
  • API typecheck passed.
  • Affected-file lint and git diff --check passed.
  • Broader MCP glob encountered unrelated Node 22 incompatibilities where existing tests require mock.module; the affected operation suite passed within that run.

No visual preview was generated because this is API/database-only behavior. No additional defects were changed.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-ultrafix
  • Time: 7m 47s
  • Tokens: 1,632,619 (1,623,881 in / 8,738 out)
  • Cost: $1.11

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

Implemented F7 only.

- Legacy result-less `running` receipts now share the interruption predicate with `accepted` receipts and are durably transitioned to compatibility/lifecycle `unknown` ([operations.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T02-18-39/packages/api/mcp/operations.ts:53), [operations.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T02-18-39/packages/api/mcp/operations.ts:126)).
- Added a migration regression covering backfill, active/unknown filtering, repeated polling, retry-hint removal, and persisted state ([mcpOperations.test.ts](/tmp/git-processor/worktrees/integry/propr/pr-2613-followup-2026-09-29T02-18-39/packages/api/test/mcpOperations.test.ts:388)).

Validation:

- Focused operation suite: 12/12 passed.
- API typecheck passed.
- Affected-file lint and `git diff --check` passed.
- Broader MCP glob encountered unrelated Node 22 incompatibilities where existing tests require `mock.module`; the affected operation suite passed within that run.

No visual preview was generated because this is API/database-only behavior. No additional defects were changed.

PR: #2613
Comment by: @propr-ultrafix (ID: 0)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR adds durable lifecycle fields, tracker synchronization, and scoped operation discovery. It needs a localized durability fix before merge. The supplied current-head checks report no failures.

✅ Scoped discovery — Listing filters by owner and grant, then applies current repository and tool permissions before pagination.

✅ Terminal-state protection — Lifecycle updates prevent subsequent start or uncertainty observations from overwriting a persisted terminal outcome.

✅ Relevant regression coverage — Tests cover SQLite rollback, concurrent transitions, interrupted invocations, and grant isolation.

Merge blockers

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

F8: 🔴 Persist receipt and lifecycle consistently

  • Required behavior: Durable lifecycle state must remain consistent with an acknowledged, persisted operation outcome across process interruption and restart.

  • Evidence: packages/api/mcp/operations.ts, McpOperations.run, replay, and project; packages/api/mcp/tools.ts, list_operations and the syncLifecycle call in get_operation.

    1. A synchronous mutation succeeds and returns HTTP 200.
    2. run commits its first update, saving state: 'completed' and the successful result. The lifecycle remains accepted.
    3. The process stops after that awaited database update and before the separate finish update. These writes have no enclosing transaction.
    4. After restart, replay projects the existing row without completing lifecycle synchronization. markInterruptedInvocations cannot repair it because it requires a null result and an accepted/running legacy state.
    5. list_operations({ lifecycle: 'active' }) consequently includes the successfully completed operation, while the completed filter excludes it. Repeated replay and listing preserve this inconsistency.

    The tracker path has the same persistence boundary: trackers save legacy outcomes before get_operation invokes syncLifecycle. A later successful get_operation can repair the lifecycle, but replay and discovery do not perform that repair.

    static trace: verified from the separate awaited writes, interruption predicates, replay projection, and lifecycle filtering in the supplied code. Existing tests exercise orderly completion and result-less interruption, not interruption between outcome persistence and lifecycle persistence.

  • Minimum fix: Atomically persist related receipt/lifecycle changes where possible, or reconcile persisted terminal receipt outcomes before replay projection and lifecycle filtering. Cover both invocation completion and tracker completion with an interruption regression.

Suggestions

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

S5: 🟢 Document lifecycle discovery

Update docs/mcp.md to explain the lifecycle block, list_operations, its default time window, and the distinction between stored discovery results and live refresh through get_operation. The tool descriptions already communicate these semantics, so this is an optional documentation improvement.

Score

The implementation has useful authorization safeguards and focused tests, with all supplied current checks passing. The demonstrated restart inconsistency affects the central durability feature and should be corrected before merge.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 16s
  • Tokens: 65,657 (63,301 in / 2,356 out)
  • Cost: $0.75

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

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR adds durable lifecycle storage, recovery, artifact tracking, and scoped operation discovery. It needs a localized concurrency correction before merge.

✅ Grant isolation — Receipt queries constrain both owner and grant, and listing checks current repository and tool permissions before pagination.

✅ Recovery coverage — Added tests cover interrupted receipt writes, cancellation propagation, SQLite rollback, and artifact survival after backend deletion.

The supplied current checks report 20 passed and no failures. This review used static analysis only, as requested.

Merge blockers

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

F17: 🔴 Prevent stale polls from replacing terminal progress

  • Required behavior: Persisted lifecycle progress must preserve terminal execution evidence against older concurrent observations.

  • Evidence: packages/api/mcp/operationLifecycle.ts:syncLifecycle, packages/api/mcp/operations.ts:recordProgress, and packages/api/mcp/operationTracking.ts:trackExecution.

    1. Poll A for review_pull_request reads a task’s processing event, then waits in refreshPullRequestContext on the GitHub request.
    2. The task completes. Poll B reads its terminal event, finishes its GitHub request first, and persists the completed receipt, terminal progress, and completed lifecycle.
    3. Poll A resumes. Its unconditional tracker update replaces the stored result with its older observation. Its subsequent syncLifecycle call unconditionally replaces progress with the earlier processing snapshot.
    4. markStarted correctly refuses to change the completed lifecycle, leaving a durable receipt whose lifecycle is completed but whose progress has regressed and lost the terminal timestamp and reason. Listing and replay expose that inconsistent snapshot without refreshing trackers.

    No transaction or observation-version check spans these awaits. The lifecycle predicates protect the lifecycle column, but recordProgress has no equivalent protection. If backend history is subsequently pruned, the terminal progress cannot be reconstructed from the overwritten receipt. The existing concurrent tests use matching terminal observations and do not exercise this ordering.

    static trace: verified through the supplied tracker, synchronization, persistence, listing, and replay implementations; no commands were run.

  • Minimum fix: Fence progress updates against superseded observations and prevent nonterminal snapshots from replacing terminal progress. Preserve the terminal recovery snapshot against stale tracker writes as well, so subsequent reconciliation cannot restore older evidence.

Suggestions

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

S11: 🟢 Bound reconciliation work

list_operations reconciles every terminal receipt for the grant before applying the requested time window, filters, or limit. Consider limiting reconciliation to receipts needing repair or processing it in bounded batches. This is an optional scalability improvement; the supplied code does not establish a current operational failure.

Score

The implementation has substantial recovery and isolation coverage, and current checks pass. The demonstrated concurrent-progress regression prevents merge readiness.
Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 15s
  • Tokens: 66,486 (64,304 in / 2,182 out)
  • Cost: $0.65

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

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit be834ee

Addressed 1 AI review comment (IDs: 5884027913)

Addressed finding F17

Summary of Changes

Implemented F17.

  • Terminal progress and lifecycle now persist atomically.
  • Stale nonterminal tracker and delayed invocation writes cannot replace terminal receipts.
  • Stale callers reload the winning durable terminal observation.
  • Added regressions covering the GitHub-await ordering, delayed invocation results, history pruning, replay, and listing.
  • No visual preview was needed because the change is backend-only.

Validation:

  • Focused operations suite: 18 passed
  • Full MCP suite: 125 passed
  • API typecheck: passed
  • API lint: passed
  • git diff --check: passed

Independent discovery, left unchanged as out of scope: the pre-existing task-submission tracker has a similar unconditional write after an await in toolsTaskSubmissions.ts; it was not introduced or modified by this PR.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @propr-ultrafix
  • Time: 9m 5s
  • Tokens: 2,126,816 (2,109,943 in / 16,873 out)
  • Cost: $1.63

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

Implemented F17.

- Terminal progress and lifecycle now persist atomically.
- Stale nonterminal tracker and delayed invocation writes cannot replace terminal receipts.
- Stale callers reload the winning durable terminal observation.
- Added regressions covering the GitHub-await ordering, delayed invocation results, history pruning, replay, and listing.
- No visual preview was needed because the change is backend-only.

Validation:

- Focused operations suite: 18 passed
- Full MCP suite: 125 passed
- API typecheck: passed
- API lint: passed
- `git diff --check`: passed

Independent discovery, left unchanged as out of scope: the pre-existing task-submission tracker has a similar unconditional write after an await in `toolsTaskSubmissions.ts`; it was not introduced or modified by this PR.

PR: #2613
Comment by: @propr-ultrafix (ID: 0)
Model: gpt-5.6-sol
@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

⚠️ Ultrafix stopped before reaching its goal. Requested goal: 8/10. Last score: unknown. Max cycles were exhausted, so manual review and merge are now required.

@integry

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/review astra fable

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 2 reviews:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — astra

Overall Evaluation

This PR adds durable lifecycle metadata, tracker synchronization, and scoped operation discovery. It needs a localized correction before merge: recovery can permanently preserve running progress after recording completion.

✅ Scoped discovery — Listing filters by owner and grant, then checks repository, tool permission, and cancellation-source authorization before pagination.

✅ Terminal receipt protection — trackExecution guards terminal writes and reloads the winning receipt when a concurrent update loses.

The authoritative current-head checks report 20 passed and no failures. This review used static analysis of the supplied code; no commands were run.

Merge blockers

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

F18: 🔴 Recovery freezes nonterminal progress

  • Required behavior: Durable lifecycle recovery must preserve the terminal snapshot already recorded by the tracker, including when earlier polling persisted nonterminal progress.

  • Evidence: packages/api/mcp/operations.ts, reconcileTerminalLifecycles and finish; packages/api/mcp/operationTracking.ts, trackExecution.

    1. A task is processing. A successful get_operation poll persists its processing snapshot through recordProgress.
    2. The task completes. The next poll’s trackExecution writes state: 'completed' and a completed result.targetState. The process stops after that awaited database update, before syncLifecycle runs.
    3. After restart, get_operation invokes reconciliation. lifecycleMissing is true, but progressMissing is false because the processing snapshot exists. Reconciliation sets the lifecycle to completed without replacing progress.
    4. Execution tracking restores the saved completed target, but syncLifecycle cannot repair progress: finish excludes an already-completed lifecycle. Replay and listing likewise leave the non-null progress untouched.

    The durable receipt consequently reports completion alongside permanently stale processing progress, despite retaining the correct terminal snapshot in its result. The conditional updates prevent terminal regression but also prevent this repair. The added recovery tests begin with null progress, so they do not cover this sequence.

    static trace: verified through the supplied tracker write, recovery predicates, and terminal update guards; this interruption scenario was not executed.

  • Minimum fix: When reconciliation transitions a lifecycle to terminal, persist the durable terminal targetState in the same guarded update even when progress is already non-null. Add a regression covering processing progress followed by interruption after the terminal tracker write.

Suggestions

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

S12: 🟢 Bound listing reconciliation work

list_operations reconciles every terminal receipt belonging to the grant before applying its time window, filters, or limit. Consider bounded reconciliation or an indexed repair predicate as history grows. This is an optional performance follow-up; no runtime performance failure was demonstrated.

S13: 🟢 Document the lifecycle contract

Update docs/mcp.md with the lifecycle fields, their relationship to compatibility state, and the fact that listing does not refresh backend trackers. The tool descriptions already explain these distinctions, so this is documentation follow-up rather than a merge blocker.

Score

The implementation has substantial regression coverage and passing current checks, but its recovery path can permanently retain an incorrect progress snapshot. The required correction is localized.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 2m 18s
  • Tokens: 77,084 (73,152 in / 3,932 out)
  • Cost: $0.93

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

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — fable

Overall Evaluation

This PR implements issue #2594 by adding a durable lifecycle block (state, acceptedAt/startedAt/finishedAt, failure, artifacts, progress) to every MCP operation receipt, a backfilling migration, monotonic lifecycle transition helpers (markStarted/markUnknown/markAccepted/finish/finishCancellationSource), post-crash reconciliation, interrupted-invocation detection, and a new scoped, paginated list_operations tool. The implementation is careful about concurrency and durability, and the test additions are extensive and exercise real interleavings rather than mocks. Head checks are green.

✅ Terminal writes are properly fenced — trackExecution (operationTracking.ts) and run() (operations.ts) now update only whereNotIn('state', terminal), and when the write loses, trackExecution re-reads the durable terminal row and returns that snapshot instead of the stale observation. The "stale concurrent polls" test drives this with a real suspended GitHub request.

✅ Recovery cannot overwrite newer evidence — reconcileTerminalLifecycles conditions its update on state, lifecycle, and the exact result text observed at read time, so an interleaved tracker write invalidates the recovery instead of being clobbered.

✅ Cancellation propagation is grant-isolated — finishCancellationSource re-reads the durable cancellation receipt and scopes the source update to the cancelling receipt's owner_id/grant_id; the test confirms a foreign-grant source is untouched.

One PR-introduced regression on pre-migration terminal receipts needs a small fix before merge; the rest are follow-ups.

Merge blockers

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

F19: 🔴 Legacy terminal receipts lose their live target state on get_operation

  • Required behavior: Changed behavior must not regress the existing get_operation contract ("returns ... plus available target state", docs/mcp.md:222-223); the new restoreResolvedTarget path replaces, rather than preserves, the target state that get_operation had already resolved for receipts that predate this PR.

  • Evidence: packages/api/mcp/operationTracking.ts:72-81 (restoreResolvedTarget) and :107-110 (early-return branch in trackExecution)

    Static trace:

    1. Start with any tracked receipt persisted before this PR (e.g. send_task_followup, review_pull_request, run_ultrafix) that is terminal: state='completed', result = { continuation: { taskId: 'T', ... }, executionResolved: true }. Pre-PR trackExecution never wrote result.targetState, so every such row has result.targetState === undefined. The migration maps it to lifecycle='completed' and leaves progress=null.
    2. Client calls get_operation. tools.ts sets receipt.targetState = task_history ... first('state','timestamp') for continuation.taskId → { state: 'completed', timestamp: ... } (unchanged behavior).
    3. New trackExecution: findExecutionTask finds task T (still present in tasks). Because result.executionResolved && terminalStates.includes(row.state), it calls restoreResolvedTarget(receipt, result, task); persistedTarget = {} and task is defined, so receipt.targetState is overwritten with { taskId: 'T', pr_number }, discarding the state/timestamp resolved in step 2.
    4. syncLifecycle cannot recover this either: finish() is not eligible (lifecycle already terminal) and reconcileTerminalLifecycles sees targetState === undefined in the durable result, so lifecycle.progress stays null.

    Observable consequence: for every terminal receipt that existed before the migration, get_operation now returns targetState without the target's state/timestamp, whereas pre-PR it returned { state, timestamp }. Neither targetState nor lifecycle.progress carries the backend outcome evidence for these rows. The existing tests only exercise receipts created after the change (which have a persisted targetState), so they do not detect this. Proposed regression: insert a terminal mcp_operations row whose result has executionResolved: true and no targetState, with a matching tasks/task_history row, call get_operation, and assert targetState.state is still present.

  • Minimum fix: In restoreResolvedTarget, fall back to the target state already on the receipt when nothing was persisted, e.g. const persistedTarget = result.targetState ?? (receipt.targetState as Record<string, unknown> | undefined) ?? {}; so { state, timestamp } from get_operation is merged with taskId/pr_number instead of dropped. Add the regression test above.

Suggestions

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

S14: 🟢 Compat state contradicts terminal lifecycle on untracked receipts

For tools whose terminal outcome is only derived at read time (create_goal via updateReceiptState, implement_plan, plans), row.state never becomes terminal, so project() emits state: 'accepted' with retryAfterSeconds: 3 while lifecycle.state is 'completed'/'failed'. This is visible in the new list_operations output and in get_operation once the goal row is gone. Consider deriving the compat state from a terminal lifecycle in project() (and dropping retryAfterSeconds in that case) so the receipt is internally consistent. Pre-existing for get_operation, newly exposed by list_operations; not required for merge.

S15: 🟢 reconcileTerminalLifecycles scans every terminal row per list_operations call

list_operations calls reconcileTerminalLifecycles(principal) with no id and no time bound, loading and JSON-parsing every terminal receipt for the grant (plus one extra SELECT per confirmed cancellation) on every listing, even though listing is bounded by sinceMinutes. Restricting the reconciliation to the same COALESCE(accepted_at, created_at) >= ? window, or to rows where finished_at IS NULL OR progress IS NULL OR artifacts = '{}', would keep the tool cheap as history grows.

S16: 🟢 startedAt parses task_history.timestamp without a timezone

epochMilliseconds uses Date.parse on target.timestamp/currentTask.timestamp. If production task history rows are written with SQLite CURRENT_TIMESTAMP (YYYY-MM-DD HH:MM:SS, UTC), V8 parses that form as local time, so lifecycle.startedAt would be skewed by the server's UTC offset. I could not verify the production write format from the supplied context; if it is not ISO-8601 with an offset, normalize before parsing.

S17: 🟢 Bound the list_operations authorization scan

wanted = offset + limit + 1 with offset up to 100000 means the loop may materialize and authorize up to ~100k rows in 50-row batches. Because authorization must run before filtering, a lower offset cap (or cursor-based paging keyed on accepted_at, id) would avoid pathological requests.

S18: 🟢 Migration backfills failure for non-failed states

The backfill extracts $.error into failure for every row with valid JSON, not just failed/unknown ones, and a string-valued error yields non-JSON text that json() later drops as null. Limiting the CASE to state IN ('failed','unknown') and to json_type(result,'$.error') = 'object' would keep the column semantically clean. Harmless in practice.

S19: 🟢 Document the new lifecycle block and list_operations

docs/mcp.md still describes receipts only in terms of the legacy state. Adding the lifecycle shape, the meaning of accepted vs running, refreshWith, and the list_operations filters would keep the public contract discoverable.

Score

The lifecycle design, concurrency fencing, and test coverage are strong and checks are green, but the PR introduces a small, easily fixed regression that strips the resolved target state from all pre-migration terminal receipts.

Score: 6/10


🤖 Review Details

  • Model: Claude Fable 5.1
  • Time: 5m 20s
  • Tokens: 124,257 (98,798 in / 25,459 out)
  • Cost: $2.51

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

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Reviews seem to be stuck on legacy support, but we don't need it. MCP support hasn't been publicly released yet, so we don't care about legacy. Clean the code up accordingly.

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 0e6f4b7

Summary of Changes

Implemented the MCP legacy cleanup:

  • Folded lifecycle columns into the original MCP schema migration.
  • Removed the additive migration, backfill logic, legacy lifecycle mapping, and legacy receipt tests/fallbacks.
  • Required lifecycle metadata for all newly created MCP operations.
  • Preserved current crash recovery and fixed terminal recovery replacing stale progress.
  • Updated affected fixtures and tests.

Validation passed:

  • Full MCP suite: 124/124
  • API typecheck
  • API and core lint
  • git diff --check

No visual preview was created because these are API/database-only changes.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @integry
  • Time: 10m 6s
  • Tokens: 3,965,568 (3,947,080 in / 18,488 out)
  • Cost: $2.53

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: 5885821150✓

Implemented the MCP legacy cleanup:

- Folded lifecycle columns into the original MCP schema migration.
- Removed the additive migration, backfill logic, legacy lifecycle mapping, and legacy receipt tests/fallbacks.
- Required lifecycle metadata for all newly created MCP operations.
- Preserved current crash recovery and fixed terminal recovery replacing stale progress.
- Updated affected fixtures and tests.

Validation passed:

- Full MCP suite: 124/124
- API typecheck
- API and core lint
- `git diff --check`

No visual preview was created because these are API/database-only changes.

PR: #2613
Comment by: @integry (ID: 5885821150)
Model: gpt-5.6-sol
@integry

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/review astra fable

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 2 reviews:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — astra

Overall Evaluation

The PR adds persisted operation lifecycles, tracker synchronization, recovery, and scoped operation discovery. It is merge-ready within the supplied scope; I found no verified PR-introduced blocker.

✅ Scoped discovery — Listing checks owner, grant, current repository access, tool permissions, and cancellation-source authorization before pagination.

✅ Terminal persistence — Execution tracking guards terminal writes and reloads the winning receipt when a concurrent poll loses its update.

✅ Focused regression coverage — Added tests exercise interruption recovery, cancellation propagation, concurrent polling, and durability after backend history removal.

This assessment is based on static review of the supplied code. No commands were run. Current-head checks report 20 passed and no failures or pending checks.

Merge blockers

No merge blockers.

Suggestions

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

S20: 🟢 Bound discovery reconciliation

reconcileTerminalLifecycles loads every terminal operation for the grant before list_operations applies its time window and pagination. Consider batching reconciliation or restricting it to relevant candidates as operation history grows. This is an optional scalability improvement; the supplied evidence does not establish a current operational failure.

S21: 🟢 Document lifecycle discovery

Update docs/mcp.md with the lifecycle block, list_operations filters, and the distinction between persisted discovery and live refresh through get_operation. Tool descriptions already explain these semantics, so this documentation addition is non-blocking.

Score

The implementation has substantial targeted coverage and passing current-head checks. Static review found no demonstrated blocker, with follow-up opportunities in documentation and reconciliation efficiency.

Score: 8/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 17s
  • Tokens: 73,792 (71,500 in / 2,292 out)
  • Cost: $0.73

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

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — fable

Overall Evaluation

This PR gives MCP mutation receipts a durable lifecycle: new lifecycle, accepted_at/started_at/finished_at, failure, artifacts and progress columns (added directly to the unreleased add_mcp migration, per the author's no-legacy guidance), a new operationLifecycle.ts that projects tracker observations into guarded, monotonic column writes, a new list_operations tool with per-row authorization and offset paging, recovery of interrupted lifecycle synchronization on every read path, and a broad set of concurrency/recovery/isolation tests. Current-head checks are all green.

The core write paths are careful and the design is largely sound. The one merge-relevant problem is that the new durable lifecycle block and the pre-existing top-level state/retryAfterSeconds fields can now contradict each other in list_operations and replay output for tools whose receipt state is only ever computed in memory (goals, plans, implement_plan, cancellation sources), which undermines the "honest receipt" contract the PR adds. That is a localized fix in project()/finish().

✅ Monotonic, guarded lifecycle writes — finish, markStarted, markAccepted, recordProgress in packages/api/mcp/operations.ts all gate on the current lifecycle/state inside the UPDATE, and finish writes the terminal progress snapshot in the same guarded statement, so the stale-poll interleavings exercised in mcpOperations.test.ts ("stale concurrent polls…", "a delayed invocation result…") genuinely cannot regress terminal facts.

✅ Tracker terminal writes are now fenced — trackExecution (operationTracking.ts) changed the previously unguarded mcp_operations update to whereNotIn('state', terminalStates) and adopts the durable winner when the write loses, closing a pre-existing overwrite window rather than merely adding columns beside it.

✅ Grant isolation and discovery hiding are tested end to end — finishCancellationSource scopes the source update to the cancelling receipt's owner/grant, and list_operations swallows only 403/404 from per-row authorization; both are covered by the foreign-grant and forbidden-repository tests.

Merge blockers

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

F20: 🔴 Projected state/retryAfterSeconds contradict a terminal lifecycle in list_operations and replay output

  • Required behavior: The receipt returned by the new list_operations tool and by idempotent replay must be internally consistent: a receipt whose durable lifecycle is terminal must not simultaneously report a non-terminal state and instruct the client to keep polling.

  • Evidence: packages/api/mcp/operations.ts, project() and finish(); packages/api/mcp/tools.ts, list_operations (page.map(row => ({ ...operations.project(row), … })))

    Static trace:

    1. Client calls create_goal; run() stores state: 'accepted' (202) and lifecycle: 'accepted'.
    2. The goal reaches result_state = 'completed'. Client calls get_operation: updateReceiptState sets the in-memory receipt.state = 'completed', syncLifecycle → finish(id, 'completed', …) sets lifecycle = 'completed'. No writer persists state for create_goal — trackExecution/trackCancellation/trackTaskSubmission do not cover it, and finish() updates only lifecycle columns — so mcp_operations.state remains 'accepted'.
    3. Client calls list_operations (or re-invokes create_goal with the same idempotencyKey, which returns project(previous)). project() reads state = row.state = 'accepted', appends retryAfterSeconds: 3, and emits lifecycle: { state: 'completed', finishedAt: … } in the same object.

    Observable outcome: a terminal receipt that says state: 'accepted', retryAfterSeconds: 3, and lifecycle.state: 'completed'. Agents following the documented receipt state (docs/mcp.md line 223) keep polling or misclassify the operation; the active filter is correct but the item payload is not. The same sequence applies to generate_plan/refine_plan (state derived only in updateReceiptState), implement_plan (state derived only inside get_operation), and any source operation cancelled via finishCancellationSource (lifecycle cancelled, state still accepted). The new test goal failures and generated task pull requests remain durable… asserts lifecycle on the listed items but never asserts their top-level state/retryAfterSeconds, so it does not catch this. get_operation itself is unaffected because it recomputes receipt.state from live target state before returning.

    Proposed regression: after the goal completes and one get_operation poll, assert list_operations().operations[0].state === 'completed' and retryAfterSeconds === undefined.

  • Minimum fix: In project(), when row.lifecycle is one of completed/failed/cancelled, report state: row.lifecycle and omit retryAfterSeconds and the stale message (or, equivalently, have finish()/finishCancellationSource() also set state to the outcome inside their already-guarded UPDATEs). Add the assertion above to the list/replay tests.

Suggestions

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

S22: 🟢 Bound reconcileTerminalLifecycles when called without an id

list_operations calls reconcileTerminalLifecycles(principal) with no id, which selects every terminal mcp_operations row for the owner/grant (unbounded by sinceMinutes), parses each result JSON, recomputes artifacts, and for every confirmed cancel_operation row issues a select plus an update in finishCancellationSource. This grows linearly with a user's total history on every list call. Consider restricting the scan to rows inside the requested accepted_at window, or to rows that still look unreconciled (finished_at IS NULL OR lifecycle NOT IN terminal OR (state='failed' AND failure IS NULL)), and skipping cancellation propagation for rows whose source is already terminal.

S23: 🟢 startedAt reflects the latest observed event, not execution start

observedStartTimestamp returns currentTask.timestamp ?? target.timestamp, i.e. the timestamp of the newest history row. For goal tasks observed only after completed/failed (both in executedGoalTaskStates) this stamps startedAt with the completion time; for ordinary tasks observed only after completion it stays null. list_tasks already computes a true start as min(timestamp) over the started states; reusing that (or falling back to Date.now() only when no earlier evidence exists) would make startedAt meaningful. Also note Date.parse on SQLite CURRENT_TIMESTAMP text (YYYY-MM-DD HH:MM:SS, no Z) is parsed as local time by V8, so started_at can be offset by the server timezone unless the worker writes ISO strings.

S24: 🟢 Guard trackTaskSubmission's state write like the other trackers

Unchanged toolsTaskSubmissions.ts:90 still updates state/result without whereNotIn('state', terminalStates). Under two concurrent get_operation polls of a create_task receipt it can transiently roll a terminal state back to the submission's non-terminal state (lifecycle columns are protected, so the durable outcome survives). This is pre-existing, but since this PR tightened the sibling write in trackExecution, aligning this one would make the state column as monotonic as lifecycle.

S25: 🟢 Durable unknown loses the interruption message

project() only attaches "Execution may have been interrupted…" when invocationInterrupted (state accepted, null result) or stale running. Once markInterruptedInvocations persists state: 'unknown', subsequent get_operation/list_operations show state: 'unknown' with no message and no trackers run (result is null). Emitting the same message for state === 'unknown' && result === null keeps guidance consistent before and after the durable transition.

S26: 🟢 Clarify lifecycle semantics for cancellation and pause/resume receipts

For cancel_operation/cancel_task receipts, observedStartTimestamp marks the cancellation receipt running whenever the target task is processing, and pause_goal/resume_goal receipts never reach a terminal lifecycle because nothing derives an outcome for them, so they remain in the active filter indefinitely. Documenting or adjusting these (e.g. finishing pause/resume on the next observed desired_state match) would make list_operations(lifecycle: 'active') more useful.

S27: 🟢 implement_plan artifacts collapse multiple issues to one PR/task

artifactsFromReceipt picks the first pr_number/taskId found across targetState.issues, so a plan implementation spanning several issues reports only one pullRequest and one taskId even though issues[] lists all issue numbers. Consider emitting per-issue pullRequest/taskId inside artifacts.issues[].

S28: 🟢 Stale running heuristic in list output

Because list_operations does not refresh trackers, a receipt whose persisted state is running but has not been polled for >120 s is projected as state: 'unknown' with the interruption message while lifecycle.state is running. That is defensible (no recent observation), but a short note in the tool description, or suppressing the message when lifecycle is running, would avoid alarming agents about healthy long-running work.

Score

The lifecycle persistence, guarded monotonic writes, recovery paths and test coverage are solid and all current checks pass, but the new list_operations/replay output can present a terminal lifecycle alongside state: 'accepted' and retryAfterSeconds: 3 for goal, plan and cancelled-source receipts, which contradicts the PR's honest-receipt contract and needs a small projection fix before merge.

Score: 6/10


🤖 Review Details

  • Model: Claude Fable 5.1
  • Time: 6m 57s
  • Tokens: 128,797 (95,660 in / 33,137 out)
  • Cost: $2.85

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

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/fix

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit 10c4207

Addressed 3 AI review comments (IDs: 5885677493, 5885750949, 5886507946)

Addressed findings F18, F19, F20

Summary of Changes

Implemented F18, F19, and F20 only.

  • F18: verified the existing guarded recovery overwrites stale nonterminal progress with the durable terminal target snapshot; its interruption regression passes.
  • F19: terminal execution restoration now preserves live targetState fields and enriches them with task/PR identity.
  • F20: terminal lifecycle state now governs replay/list projections, removing contradictory polling hints and stale messages.
  • Added focused get_operation, replay, and listing regressions.

Validation:

  • Full MCP suite: 125/125 passed
  • Focused operations suite: 17/17 passed
  • API typecheck passed
  • API lint passed
  • git diff --check passed

Independent pre-existing findings, intentionally unchanged: resolved cancellation tracking can similarly clear a missing persisted target, and follow-up task lookup assumes a job ID is present. Both are outside PR-changed behavior. No visual preview was generated because the changes are API-only.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @integry
  • Time: 6m 27s
  • Tokens: 1,687,130 (1,677,350 in / 9,780 out)
  • Cost: $1.22

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: 5886663832✓

Implemented F18, F19, and F20 only.

- F18: verified the existing guarded recovery overwrites stale nonterminal progress with the durable terminal target snapshot; its interruption regression passes.
- F19: terminal execution restoration now preserves live `targetState` fields and enriches them with task/PR identity.
- F20: terminal lifecycle state now governs replay/list projections, removing contradictory polling hints and stale messages.
- Added focused `get_operation`, replay, and listing regressions.

Validation:

- Full MCP suite: 125/125 passed
- Focused operations suite: 17/17 passed
- API typecheck passed
- API lint passed
- `git diff --check` passed

Independent pre-existing findings, intentionally unchanged: resolved cancellation tracking can similarly clear a missing persisted target, and follow-up task lookup assumes a job ID is present. Both are outside PR-changed behavior. No visual preview was generated because the changes are API-only.

PR: #2613
Comment by: @integry (ID: 5886663832)
Model: gpt-5.6-sol
@integry

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/review fable

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — fable

Overall Evaluation

This PR gives MCP mutation receipts a durable lifecycle block (lifecycle, acceptedAt/startedAt/finishedAt, failure, artifacts, progress) persisted in new mcp_operations columns, adds syncLifecycle to project tracker observations into those columns with monotonic guards, adds crash-recovery reconciliation (reconcileTerminalLifecycles, markInterruptedInvocations), guards the tracker writes in trackExecution/trackCancellation against overwriting terminal evidence, and introduces a paginated, authorization-filtered list_operations tool. Per the maintainer's direction, the schema change lives in the original add_mcp migration rather than a follow-up migration. The implementation is careful about concurrency and grant isolation, and the test additions are substantial and target real interleavings rather than happy paths.

✅ Terminal evidence is protected against stale concurrent polls — trackExecution now uses whereNotIn('state', terminal) and, when the write is rejected, re-reads the winning durable receipt (operationTracking.ts), and finish() persists the terminal progress snapshot in the same guarded UPDATE with a CASE on the pre-update lifecycle. The "stale concurrent polls" test drives the exact await window (inside the GitHub request) and asserts both the DB row and the stale caller's returned receipt.

✅ Cancellation propagation is grant-scoped — finishCancellationSource re-reads the cancellation row under owner_id/grant_id/tool = 'cancel_operation'/terminal state before touching the source, so a forged cancellation: 'confirmed' receipt cannot flip a foreign grant's operation; the test with foreignSource verifies it.

✅ Recovery is idempotent and read-consistent — reconcileTerminalLifecycles conditions its UPDATE on the exact state/lifecycle/result it read, uses COALESCE/json_patch so repeated runs are no-ops, and preserves updated_at as finished_at evidence rather than the recovery time.

The PR needs one localized correction (below) before it is merge-ready; otherwise it is in good shape. All current-head checks pass.

Merge blockers

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

F21: 🔴 list_operations/replay projection contradicts its own lifecycle for running work

  • Required behavior: The changed receipt contract states "running" means execution was observed and the lifecycle block is meant to be the honest durable state. A single projected receipt must not simultaneously report state: 'unknown' with an "Execution may have been interrupted" message and lifecycle.state: 'running'.

  • Evidence: packages/api/mcp/operations.ts, project() — const stale = !terminal && (interrupted || (row.state === 'running' && Date.now() - Number(row.updated_at) > interruptionTimeoutMs)) and const state = terminal ? row.lifecycle : stale ? 'unknown' : row.state.

    Static trace:

    1. A tracked mutation (e.g. review_pull_request) is accepted. While the review task is in processing, the client polls get_operation once: trackTask sets receipt.state = 'running', trackExecution persists state: 'running', updated_at: now (guarded update succeeds), and syncLifecycle → markStarted sets lifecycle = 'running', started_at.
    2. The client stops polling for more than 120 s while the task keeps running (review tasks routinely take minutes). Nothing refreshes updated_at because, as the tool description says, list_operations deliberately does not refresh trackers.
    3. The client calls list_operations({ lifecycle: 'active' }). markInterruptedInvocations does not touch the row (result is non-null). The row is selected by lifecycle IN ('accepted','running'). project(row) computes terminal = false, interrupted = false, stale = true → the item is { state: 'unknown', message: 'Execution may have been interrupted. Inspect the target; this action will not be replayed automatically.', lifecycle: { state: 'running', startedAt: <set>, ... } } with retryAfterSeconds removed.
    4. The same projection is returned from McpOperations.run on a duplicate idempotency key and from replay().

    Observable consequence: an agent filtering for active work is told the operation may have been interrupted and should not be polled (no retryAfterSeconds), while the durable lifecycle in the same object says it is running. Nothing is actually interrupted.

    Why existing protections don't cover it: before this PR the initial row state was 'running' (set at insert), so the updated_at staleness clause meant "invoke still in flight after 2 minutes". This PR changes the insert state to 'accepted' and detects in-flight interruption via invocationInterrupted (accepted_at + null result), so the retained 'running' clause now only fires on tracker-written rows that simply have not been polled — exactly the population list_operations exposes. The new tests only exercise the interrupted path with result-less rows (interrupted accepted invocations become durable unknown…), never a tracker-written running row older than 120 s.

    Verification: static trace over the supplied diff. Proposed regression: run a tracked tool, poll get_operation while the task is processing, set updated_at to Date.now() - 120_001 on the row, call list_operations({ lifecycle: 'active' }) and assert the item's state is 'running' and no interruption message is present.

  • Minimum fix: In project(), drop the row.state === 'running' && updated_at staleness clause (interruption is now detected by invocationInterrupted), or at minimum do not classify a row as stale when row.lifecycle === 'running'; add the regression above.

Suggestions

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

S29: 🟢 Bound reconcileTerminalLifecycles in list_operations

list_operations calls reconcileTerminalLifecycles(principal) with no id, which selects every terminal receipt the owner/grant has ever produced, JSON-parses each result, and issues a read (plus possibly a write) per cancel_operation row. Docs say receipt rows must never be deleted, so this is an unbounded, growing per-call cost on a read-only tool. Restricting the reconciliation to the same accepted_at >= now - sinceMinutes window as the listing query (or to rows where lifecycle is non-terminal / finished_at IS NULL) keeps the recovery intent while making the call O(page). Optional because correctness is unaffected.

S30: 🟢 Guard trackTaskSubmission's receipt write like the other trackers

packages/api/mcp/toolsTaskSubmissions.ts:90 still updates state/result without whereNotIn('state', terminal). Two concurrent get_operation polls on a create_task receipt where the task reaches a terminal history row between poll A's trackExecution and poll B's trackTaskSubmission would let B overwrite A's executionResolved/targetState with a non-resolved submission snapshot. The lifecycle column is protected by finish()'s guard and the next poll re-resolves, so impact is limited and self-healing unless task_history is pruned in between; it is a pre-existing unguarded write, but applying the same guard the PR added to trackExecution would close the last window.

S31: 🟢 Do not derive a failure envelope for non-failed outcomes in run()

McpOperations.run computes failure = errorEnvelope(data.error) ?? failureFromReceipt(receipt) and passes it to finish() for every terminal state, including 'completed'. failureFromReceipt matches on generic keys (result.reason, reviewResults, loop.completionStatus), so any 200 response that happens to carry a reason string would attach an EXECUTION_FAILED envelope to a completed receipt. syncLifecycle already gates this on outcome === 'failed'; mirroring that gate in run() (and in reconcileTerminalLifecycles, which already does) removes the inconsistency. I could not identify a concrete current tool that returns such a field, hence a suggestion.

S32: 🟢 startedAt stays null for plain tasks that finish between polls

observedStartTimestamp treats completed/failed as start evidence for goal currentTask states but not for the receipt's own targetState.state. A send_task_followup whose task goes pending → processing → completed between two get_operation polls ends completed with startedAt: null, while an equivalent goal task would get a start timestamp. Consider treating a terminal task state (at least completed) as evidence that execution began, using the earliest processing history row when available.

S33: 🟢 Multi-issue artifacts collapse to a single task/PR

For implement_plan, artifactsFromReceipt sets artifacts.taskId to the first issue's task_id and artifacts.pullRequest to the first issue's PR, which is arbitrary when a plan publishes several issues. The issues array is complete, but consumers reading taskId/pullRequest may assume they are the operation's single output. Either omit the singular handles when targetState.issues has more than one entry or attach taskId/pullRequest per issue.

S34: 🟢 Document the lifecycle block and list_operations; note the in-place migration edit

docs/mcp.md:219-232 still describes only the top-level state contract and docs/mcp-coverage.md:35 does not list list_operations. Also, because the columns were added by editing the already-existing 20260910220000_add_mcp.js (per the maintainer's no-legacy instruction), any environment that has already applied that migration will not receive the new NOT NULL columns and every MCP mutation will fail on insert; a short operator note to roll back/re-run the MCP migration on such instances would avoid a surprise.

S35: 🟢 Prefer UTC-safe timestamp parsing for started_at

epochMilliseconds uses Date.parse on task_history.timestamp. If any writer stores SQLite CURRENT_TIMESTAMP-style YYYY-MM-DD HH:MM:SS strings (the test schemas default to db.fn.now()), V8 parses that format as local time, skewing startedAt by the host offset on non-UTC hosts. Normalizing such strings to ...Z before parsing is a cheap hardening.

Score

The implementation is careful and well-tested, and the concurrency/recovery design holds up under tracing, but the retained updated_at staleness clause now yields self-contradictory receipts on the new list_operations surface and needs a small fix before merge.

Score: 6/10


🤖 Review Details

  • Model: Claude Fable 5.1
  • Time: 7m 15s
  • Tokens: 132,803 (97,792 in / 35,011 out)
  • Cost: $2.97

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

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/fix

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ Applied the requested follow-up changes in commit bbe1192

Addressed 1 AI review comment (IDs: 5887060574)

Addressed finding F21

Summary of Changes

Implemented F21 only.

  • Removed timestamp-based interruption classification for tracker-confirmed running operations in operations.ts.
  • Added regression coverage for list_operations, replay, and duplicate idempotency responses in mcpOperations.test.ts.
  • Preserved result-less accepted invocation interruption handling.

Validation passed:

  • Full MCP suite: 126/126 tests
  • API TypeScript check
  • Scoped ESLint
  • git diff --check

No independent related defects found. No visual preview was needed for this API-only change.


🤖 Implementation Details

  • Model: GPT-5.6 Sol (high)
  • Requested By: @integry
  • Time: 5m 11s
  • Tokens: 2,090,151 (2,083,215 in / 6,936 out)
  • Cost: $1.25

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: 5887302103✓

Implemented F21 only.

- Removed timestamp-based interruption classification for tracker-confirmed `running` operations in [operations.ts](/home/node/workspace/packages/api/mcp/operations.ts:301).
- Added regression coverage for `list_operations`, replay, and duplicate idempotency responses in [mcpOperations.test.ts](/home/node/workspace/packages/api/test/mcpOperations.test.ts:538).
- Preserved result-less `accepted` invocation interruption handling.

Validation passed:

- Full MCP suite: 126/126 tests
- API TypeScript check
- Scoped ESLint
- `git diff --check`

No independent related defects found. No visual preview was needed for this API-only change.

PR: #2613
Comment by: @integry (ID: 5887302103)
Model: gpt-5.6-sol
@integry

integry commented Sep 29, 2026

Copy link
Copy Markdown
Owner

/review fable

@propr-dev

propr-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — fable

Overall Evaluation

This PR gives every MCP mutation receipt a durable lifecycle block (accepted → running → completed|failed|cancelled|unknown), persisted timestamps, a normalized failure envelope, output artifacts and a progress snapshot, plus a scoped/paginated list_operations tool. In line with the maintainer's "no legacy" instruction, the columns were folded into the original add_mcp migration instead of a follow-up migration with backfill. The implementation is careful about the concurrency hazards that matter here: every lifecycle write is a guarded conditional UPDATE (nonterminal-only, COALESCE for first-observation timestamps, json_patch for artifact merges), and the tracker/wrapper race that previously let a stale poll overwrite terminal evidence is closed on both sides. Test coverage of the changed behavior is unusually thorough (interleaved polls, delayed invocation results, interrupted-process recovery, grant isolation, discovery leak checks). All current-head checks pass.

I traced the changed paths through toolExecution.ts, get_operation, list_operations, replay, run, and the tracker/lifecycle sync, including cancellation propagation, interruption detection, and terminal restoration, and did not find a reachable regression that I could drive to an incorrect observable outcome. The PR is merge-ready within scope; the items below are optional refinements.

✅ Monotonic lifecycle writes are genuinely race-safe — finish, markStarted, markUnknown, markAccepted, recordProgress in packages/api/mcp/operations.ts all gate on the current lifecycle/state in the WHERE clause and use COALESCE/CASE for first-write-wins fields, so stale concurrent polls cannot regress a terminal outcome or move started_at/finished_at; the "stale concurrent polls" and "delayed invocation result" tests exercise the actual interleavings.

✅ Lost-race path returns the durable winner — trackExecution (packages/api/mcp/operationTracking.ts) now checks the affected-row count of its whereNotIn(terminal) update and, on loss, replaces the in-flight receipt with the persisted terminal result/target so the caller and syncLifecycle never act on the stale snapshot.

✅ Discovery does not leak hidden receipts — list_operations (packages/api/mcp/tools.ts) applies the same per-row authorization as get_operation (repository, tool permission, cancellation source), swallows only 403/404, and computes nextOffset from the authorized sequence, so neither pagination nor counts reveal forbidden rows; verified by the "before paging" test including the secret-marker assertion.

Merge blockers

No merge blockers.

Suggestions

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

S36: 🟢 Keep post-invocation persistence outside the invoke try/catch

In packages/api/mcp/operations.ts run(), recordArtifacts, finish, markAccepted and the unknown-branch update now execute inside the same try whose catch classifies the error as an invocation failure and overwrites result with { error: envelope } (guarded only by whereNotIn(state, terminal)). If any of those bookkeeping writes threw after a successful 202 (queued/posted), the durable continuation (taskId/jobId/commentId) would be replaced by an error envelope and every subsequent poll would return early on result.error, permanently losing tracking of a real side effect. A DB failure at that exact point is unlikely with a single SQLite connection (and the prior code already had one update inside the try), so this is hardening rather than a blocker; splitting the block so only invoke() is classified would make the receipt strictly more honest.

S37: 🟢 Bound reconcileTerminalLifecycles when called without an id

list_operations (packages/api/mcp/tools.ts) calls operations.reconcileTerminalLifecycles(principal) with no id, which selects every terminal row for the owner/grant (including full result text), recomputes artifacts/failure for each, and issues a finishCancellationSource lookup per cancel_operation row — on every list call, regardless of sinceMinutes. The repair itself is correct, but the cost grows with receipt history until pruning. Restricting the scan to rows where finished_at IS NULL OR lifecycle NOT IN (terminal) (or to the sinceMinutes window) would keep the per-call cost proportional to what actually needs repair.

S38: 🟢 startedAt for goals can equal the completion timestamp

observedStartTimestamp in packages/api/mcp/operationLifecycle.ts treats a goal's currentTask in completed/failed as evidence of start (executedGoalTaskStates) and uses that event's timestamp as started_at. When the first poll lands after the task already finished, startedAt === finishedAt, which is technically "observed" but misleading. For ordinary tasks the same situation leaves started_at null (only processing-family states count), so the two paths are also inconsistent. Consider looking up the earliest processing event for the task (as list_tasks already does via its taskStart subquery) when only a terminal event is visible.

S39: 🟢 browser_required receipts stay accepted and appear in active forever

operationState maps a browser_required response to state browser_required while the lifecycle column remains accepted; nothing ever transitions it, and invocationInterrupted requires state === 'accepted', so list_operations { lifecycle: 'active' } will list these indefinitely even though no backend work was handed off. A small dedicated handling (e.g. lifecycle unknown with a BROWSER_REQUIRED envelope, or excluding that state from active) would keep the lifecycle contract in the tool description accurate.

S40: 🟢 Planner generation/refinement never reaches running

For generate_plan/refine_plan the targetState is { status, paused, mcp_revision }; observedStartTimestamp only inspects state/currentTask/queueState/loop, so an actively generating/refining draft is reported as lifecycle accepted. Treating those statuses as observed start would make running consistent across tool families. Optional, since the projection is still truthful (execution merely wasn't "observed").

S41: 🟢 Misleading failure message when goal fails without failure_reason

failureFromReceipt falls back through target.failure_reason → currentTask.reason → target.reason. A goal with result_state: 'failed' but no failure_reason whose current task finished successfully would surface EXECUTION_FAILED: "Goal task completed successfully". Skipping currentTask.reason unless currentTask.state === 'failed' would avoid attributing a success message to a failure.

S42: 🟢 Multi-issue implement_plan artifacts pick the first issue's task/PR

artifactsFromReceipt collects all issues, but taskId and pullRequest come from the first matching targetIssues entry, so a plan execution spanning several issues reports one task and one PR at the top level. If per-issue task/PR handles are useful to clients, attaching taskId/pullRequest to each issues[] entry would be more faithful; today's shape is not wrong, just lossy.

S43: 🟢 In-place migration edit requires resetting existing dev databases

Folding the new columns into packages/core/src/db/migrations/20260910220000_add_mcp.js matches the maintainer's decision that no pre-release MCP data needs migrating, but any environment where that migration already ran will have a schema missing lifecycle/accepted_at/artifacts (all NOT NULL), and every insert in run() will fail. Worth a note in the merge/release notes so developers re-run the migration from scratch.

Score

No verified blockers, all current-head checks pass, and the changed concurrency/recovery behavior is backed by targeted interleaving and interruption tests; remaining items are semantic polish and performance hardening.

Score: 8/10


🤖 Review Details

  • Model: Claude Fable 5.1
  • Time: 6m 25s
  • Tokens: 126,725 (95,675 in / 31,050 out)
  • Cost: $2.75

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
integry merged commit 8f4fb19 into 2593-epic-mcp-observability-cf6 Sep 29, 2026
39 checks passed
@integry
integry deleted the 2594/gpt-5.6-sol-mcp-give-operation-receip-20260929-0054-naw branch September 29, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant