Skip to content

fix: harden workflow runner monitoring#6

Open
pascalandr wants to merge 2 commits into
NeuralNomadsAI:devfrom
Pagecran:pr/workflow-runner-monitoring-hardening
Open

fix: harden workflow runner monitoring#6
pascalandr wants to merge 2 commits into
NeuralNomadsAI:devfrom
Pagecran:pr/workflow-runner-monitoring-hardening

Conversation

@pascalandr
Copy link
Copy Markdown

Summary

  • Hardens Workflow Runner monitoring against prompt results that do not expose runResult.data.parts.
  • Adds defensive prompt-result extraction and test coverage for the observed runtime shape.
  • Keeps shared-worktree implementation workflow monitoring safer and less crash-prone.

Why

A real runtime failure showed that the monitor assumed a narrower OpenCode result shape than it should. This fix makes the delegated workflow path more resilient without changing the workflow model.

Review notes

  • Independent PR targeting dev.
  • Focused runtime robustness bugfix.
  • Excludes PAI/session sync and auto-onboarding changes.

Verification

  • Branch verification: npm test — PASS, 11 tests.

Extract workflow runner prompt results defensively so monitor notifications survive OpenCode client response shape changes.
@pascalandr
Copy link
Copy Markdown
Author

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown

codenomadbot Bot commented May 15, 2026

Gatekeeper review for latest PR state.

Understood request: review PR #6 ruthlessly for regressions and better implementations, without modifying scope or code.

Verdict

No blocking regression found. This PR is narrow, addresses the observed crash path, and passes local verification after installing dependencies.

Findings

Low: src/index.js:779-782 extracts from data.parts / top-level parts, but returns immediately for any non-empty parts array even if no text is actually extracted. If OpenCode returns non-text parts, ignored parts, or a mixed shape where data.parts is present but empty/non-useful and useful text exists elsewhere, PMA could receive a blank final summary instead of the fallback message. Better implementation: reuse the existing extractTextParts() semantics for part arrays and only return that branch when extracted text is non-empty, then fall through to data.text, message, JSON fallback, and final default.

Low: tests/plugin.test.js:38 uses a fixed setTimeout(10) to wait for the background monitor. It is probably fine with these synchronous mocks, but it is still timing-based. A more deterministic test would expose/await the monitor promise, or poll/flush until promptAsync has been called, so the test fails because behavior is wrong rather than because the local event loop was slow.

Regression Check

No regression found in the main workflow-runner path. The prior crash source, assuming runResult.data.parts, is removed and replaced with defensive extraction. activeWorkflows.delete(sessionId) behavior remains preserved on both success and failure paths.

Better Implementation Notes

The new helper is acceptable, but it overlaps with existing extractTextParts(parts) at src/index.js:349. Consolidating part extraction would reduce drift and make filtering behavior consistent across session-message, discussion-summary, and workflow-runner result handling.

Verification

  • npm ci completed successfully from lockfile.
  • npm test passed: 2 suites, 11 tests.
  • npm run build passed.
  • Worktree remained clean after validation.

Residual Risk

npm ci reports existing dependency audit issues: 3 vulnerabilities, 2 moderate and 1 high. I did not treat this as PR-blocking because this PR does not change dependencies, but it should be tracked separately.

--
Yours,
CodeNomadBot

Reuse text-part extraction semantics before falling back to other prompt result fields, and replace the brittle monitor test delay with a poll for the notification call.
@pascalandr
Copy link
Copy Markdown
Author

@CodeNomadBot
same review

@codenomadbot
Copy link
Copy Markdown

codenomadbot Bot commented May 15, 2026

Gatekeeper re-review for latest PR state.

Understood request: repeat the same ruthless PR gatekeeper review on the updated PR #6 state. Assumption: review-only, no code changes requested.

Verdict

No blocking regression found. The latest commit directly addresses the two low-severity issues from the prior review, and the branch passes verification.

What Improved Since Prior Review

  • src/index.js:778-783 now reuses existing extractTextParts() behavior for part extraction and only returns the parts branch when useful text exists.
  • tests/plugin.test.js:15-20 replaces the fixed setTimeout(10) with deterministic mock-call waiting.
  • tests/plugin.test.js:59-97 adds explicit coverage for empty/blank parts falling back to data.text.

Findings

Low: src/index.js:779 still selects runResult.data.parts before runResult.parts with ||. Because arrays are truthy, if a future client shape includes data.parts: [] plus useful top-level parts, the top-level parts will be skipped and extraction will fall through to text/message/JSON fallback. This is a narrow edge case, not a blocker. A more exhaustive implementation would try both candidate arrays in priority order and return the first non-empty extracted text.

Regression Check

No regression found in the workflow-runner monitor path. The previous crash on missing runResult.data.parts remains fixed, blank part text now falls through correctly, and workflow cleanup via activeWorkflows.delete(sessionId) is unchanged on success and failure.

Verification

  • npm test passed: 2 suites, 12 tests.
  • npm run build passed.
  • Worktree remained clean after validation.

Residual Risk

The dependency audit warnings observed during the prior dependency install remain outside this PR’s scope because this PR does not change dependencies.

--
Yours,
CodeNomadBot

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant