Skip to content

feat: add kata-backed pi tasks plugin#15

Draft
mariusvniekerk wants to merge 18 commits into
mainfrom
pi-tasks-kata-plugin
Draft

feat: add kata-backed pi tasks plugin#15
mariusvniekerk wants to merge 18 commits into
mainfrom
pi-tasks-kata-plugin

Conversation

@mariusvniekerk
Copy link
Copy Markdown
Collaborator

Summary

  • add an in-repo TypeScript Pi extension that exposes pi-tasks-style tools backed by Kata issues
  • map TaskCreate/List/Get/Update to Kata CLI operations and labels/links/owners
  • implement TaskExecute as a Kata claim/start bridge for pi-subagents with lifecycle comments and close/failure handling
  • include design/implementation docs and Vitest coverage for command translation, formatting, and lifecycle races

Validation

  • npm test
  • npm run typecheck
  • npm run build
  • git diff --check
  • GOFLAGS=-buildvcs=false go test ./...

Note: this worktree needs Go's suggested -buildvcs=false flag for the full suite because plain go test ./... fails while building helper binaries with VCS stamping.

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (d7e277e)

Medium-risk lifecycle bugs remain in the Kata task execution flow; no High or Critical findings were reported.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:117
    claimForExecution allows tasks already labeled in_progress to be executed again, which can spawn duplicate subagents for the same Kata issue.

    Fix: Reject execution when the task already has the in_progress label, or make execution idempotent by tracking and returning the existing run.

  • plugins/pi-tasks-kata/src/index.ts:110
    If claimForExecution partially mutates the issue, such as assigning it or adding in_progress, and then a later claim step fails, claimed remains false, so cleanup is skipped and the task can be left claimed or in progress without any spawned agent.

    Fix: Make the claim operation compensate its own partial mutations on failure, or expose partial-claim state so TaskExecute can call failExecution.

  • plugins/pi-tasks-kata/src/index.ts:123
    recordAgentSpawn runs inside the same failure path as spawning. If only the spawn comment fails after the subagent was created, the code records execution failure and removes in_progress even though the agent is running.

    Fix: Treat post-spawn comment failures separately, logging or surfacing them without marking the task execution as failed.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk mariusvniekerk marked this pull request as draft May 5, 2026 17:35
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (ad6352e)

Verdict: Changes have 1 High and 2 Medium findings that should be addressed before merge.

High

  • Location: plugins/pi-tasks-kata/src/kata.ts:115
  • Problem: claimForExecution uses a check-then-mutate sequence (show, then assign, then label add) with no atomic guard, so two concurrent TaskExecute calls can both observe the task as not in progress and both spawn agents for the same Kata issue.
  • Fix: Make claiming conditional/atomic in Kata, or add a durable claim token/compare-and-set step and verify ownership/claim state before spawning.

Medium

  • Location: plugins/pi-tasks-kata/src/kata.ts:197

  • Problem: removeLabel swallows every failure, not just “label is absent”; TaskUpdate status=pending, failed executions, and completions can report success while the in_progress label remains due to a real Kata error.

  • Fix: Only ignore the specific “label not present” case, and let other label-removal failures propagate; use a separate best-effort cleanup path for rollback.

  • Location: plugins/pi-tasks-kata/src/subagents.ts:24

  • Problem: unsubscribe is declared as a const and initialized by events.on, but it is called within the event handler. If events.on fires the callback synchronously, it will throw a ReferenceError for accessing the variable before initialization.

  • Fix: Declare let unsubscribe: () => void; before the setTimeout, and assign it from events.on.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (e3ed3af)

Medium-risk issues found in task execution lifecycle and claim handling; no critical or high findings.

Medium

  • plugins/pi-tasks-kata/src/index.ts:135
    If kata.failExecution() fails while handling a spawn failure, TaskExecute throws out of the loop, skips remaining task IDs, and does not return the collected failure text.
    Fix: Wrap failure recording in its own try/catch, log cleanup errors, still append the original task failure, and continue processing the rest of the requested tasks.

  • plugins/pi-tasks-kata/src/kata.ts:190
    removeLabel() suppresses every Kata error, not just “label is absent”; on real CLI/workspace failures, failed executions can leave in_progress on the task while still adding a failure comment, making the task continue to appear active.
    Fix: Only ignore the specific absent-label case, or check current labels before removal and let unexpected command failures propagate.

  • plugins/pi-tasks-kata/src/kata.ts:113
    Task claiming is a non-atomic read-then-write sequence, so two concurrent TaskExecute calls can both observe an open task without in_progress and both spawn agents for it.
    Fix: Use an atomic Kata claim/conditional update if available, or add a lock/compare-and-set mechanism that makes the label/owner transition fail when another executor has already claimed the task.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (876b488)

Medium: rollback can erase pre-existing task ownership during claim failure.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:142
    • Problem: Claim rollback always unassigns the task after a partial claim failure, even if the task had a pre-existing owner before assign() overwrote it. This can silently erase existing ownership metadata on any assigned open task when the later label/comment step fails.
    • Fix: Preserve detail.issue.owner before assigning and restore that owner on rollback, or reject execution for tasks already owned by another agent instead of overwriting ownership.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (5ddc3be)

Medium severity findings need attention before merge.

Medium

  • plugins/pi-tasks-kata/src/index.ts:136
    When TaskExecute successfully claims a previously unowned task but subagent spawn fails or times out, cleanup calls failExecution, which only removes in_progress and comments. The task remains assigned to the current author, and claimForExecution rejects other owners, so the task can become stranded for other agents.
    Fix: Track whether the claim created the assignment and unassign during spawn-failure cleanup only for newly assigned tasks.

  • plugins/pi-tasks-kata/src/kata.ts:127
    Claiming is a check-then-mutate sequence protected only by an in-memory lock. Two plugin processes can both read the task as open/unowned, then both assign/label/spawn, causing duplicate execution.
    Fix: Use a Kata-level atomic claim/compare-and-set operation, or add a durable lock/conditional assignment that fails if owner/status changed before spawning.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (8c94fc7)

Changes need one fix before merge: status updates can leave Kata issues in the wrong open/closed state.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:89 - TaskUpdate status transitions do not account for the current Kata issue state. Setting status: "in_progress" only adds the label, so a closed issue remains closed and still formats as completed; setting status: "pending" always runs kata reopen, which can fail or create partial updates for already-open tasks. Fetch the current task before status mutations, reopen only when the issue is closed, and reopen before adding in_progress when moving a closed task back into progress.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (6f459ae)

Summary verdict: One medium issue should be addressed before merging.

Medium

  • Sensitive tool input can be exposed in errors/logs
    plugins/pi-tasks-kata/src/kata.ts:303

    defaultKataRunner builds failure messages with args.join(" "). Some command paths pass user-controlled or potentially sensitive content in CLI args, especially comment --body values from metadata, activeForm, subagent results, and subagent errors. If kata comment or another mutation fails, the thrown error can include the full comment body, which may then be returned by a tool handler or logged through lifecycle cleanup paths such as plugins/pi-tasks-kata/src/index.ts:127.

    Remediation: redact sensitive argument values in runner errors. For example, replace values for --body, --idempotency-key, and similar value-bearing flags with placeholders, or report only the subcommand, exit code, and stderr/stdout. Keep full args available only behind explicit debug behavior that still redacts by default.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (1e37f81)

Summary verdict: Medium-risk issues remain around sensitive output leakage, CLI argument validation, and task ownership enforcement.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:304
    Runner errors redact the command label but still append raw stderr/stdout, which can leak task bodies, metadata, comments, or other sensitive arguments if Kata echoes them in failures.
    Fix: Redact or omit child output in thrown errors, or apply the same sensitive-value filtering to stderr/stdout.

  • plugins/pi-tasks-kata/src/kata.ts:63
    Public task IDs and dependency IDs are accepted as arbitrary strings and passed as CLI positionals, so values beginning with - can be parsed as Kata options instead of issue numbers.
    Fix: Validate issue IDs as positive integers before constructing CLI args, or use a supported -- separator for positional user values.

  • plugins/pi-tasks-kata/src/index.ts:86, plugins/pi-tasks-kata/src/kata.ts:78
    TaskUpdate can bypass task ownership checks. TaskExecute refuses to claim tasks owned by another agent, but TaskUpdate exposes owner, status, addBlocks, and addBlockedBy without checking the current owner or caller identity. A caller could take ownership, alter status, or close/reopen another agent’s task, then call TaskExecute.
    Fix: Enforce the same ownership rules in TaskUpdate before applying mutations. Load the current issue, reject updates to tasks owned by a different agent unless a privileged/admin path exists, and restrict non-owners from changing owner or status.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (d4d3fea)

Summary verdict: Medium-risk lifecycle issues remain; no security findings were reported.

Medium

  • Location: plugins/pi-tasks-kata/src/kata.ts:338
    Problem: defaultKataRunner omits stderr/stdout from thrown errors, but removeLabel() relies on the error text to detect benign “label not found” failures. In real Kata CLI use, completing or resetting a task without in_progress can fail instead of being treated as idempotent.
    Fix: Throw a structured/sanitized command error that still exposes stderr for programmatic checks, or return exit metadata so isAbsentLabelError() can inspect the CLI output without leaking secrets.

  • Location: plugins/pi-tasks-kata/src/kata.ts:216
    Problem: completeExecution() removes in_progress before closing the issue. If label removal succeeds but kata close fails, the task is left open and no longer marked in progress, making it appear claimable again after a completed subagent.
    Fix: Close the issue first, then remove the label; a closed issue is already rendered as completed even if the label cleanup later fails.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (a2040c1)

High confidence: the PR has two actionable findings, including one High-severity duplicate-claim risk; no Critical findings were reported.

High

  • plugins/pi-tasks-kata/src/kata.ts:42 - The durable claim lock defaults to process.cwd() or the raw KATA_WORKSPACE value rather than Kata’s resolved project root, so two Pi sessions operating on the same Kata project from different directories can take different locks. Because the claim itself is a non-atomic show/assign/label sequence, both sessions can pass the preflight and spawn duplicate subagents for the same task.
    • Fix: Store claim locks under a canonical Kata project/workspace path, or move claiming into an atomic Kata CLI/daemon operation that fails when owner or in_progress changed.

Medium

  • plugins/pi-tasks-kata/src/format.ts:16 - TaskList renders blockers from issue.blockedBy, but kata list --json returns outgoing blocks, not incoming blockedBy. Blocked tasks therefore appear as ordinary pending tasks in list output even though TaskExecute later refuses to run them.
    • Fix: Hydrate incoming blockers from show/links when listing, or extend/normalize Kata list output to include blockedBy.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (0bad1d9)

Medium issue found; no Critical or High findings reported.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:118 - TaskUpdate status=completed removes the in_progress label before closing the issue, so a failed kata close leaves the task open but no longer marked in progress. Close the issue first, then remove in_progress, or restore the label if close fails.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (b54cf18)

Medium issue found: sanitized Kata command failures may still leak raw stderr/stdout through logged error objects.

Medium

Raw Kata command output can leak to logs

Locations: plugins/pi-tasks-kata/src/kata.ts:321, plugins/pi-tasks-kata/src/kata.ts:351, plugins/pi-tasks-kata/src/index.ts:129, plugins/pi-tasks-kata/src/index.ts:142, plugins/pi-tasks-kata/src/index.ts:203, plugins/pi-tasks-kata/src/index.ts:214

KataCommandError sanitizes the public error message, but still stores raw stderr/stdout on the error object via output. Several catch blocks pass the full error object to console.error, and Node commonly prints enumerable custom properties on Error objects. That can expose sensitive Kata output such as task bodies, comments, subagent results, or command failure context in process or CI logs.

A likely leak path is a subagent completion or failure containing sensitive text, followed by a failed kata comment mutation where Kata echoes the rejected body or context in stderr/stdout. The full KataCommandError can then be logged by lifecycle mutation handling.

Suggested fix: log only sanitized text, such as error instanceof Error ? error.message : String(error). Avoid passing raw Kata command errors directly to console.error. If raw output is still needed for absent-label detection, make it non-enumerable/private or replace it with a sanitized classification signal.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 5, 2026

roborev: Combined Review (86fe876)

Clean overall: no Medium, High, or Critical findings were reported.

All review agents found no actionable issues at Medium severity or above.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

mariusvniekerk and others added 16 commits May 17, 2026 13:52
Kata no longer exposes stable per-project issue numbers after the short_id cutover, so the pi task adapter must treat task ids as Kata refs instead of integers. Without this, created tasks could not record metadata and execution/update flows passed legacy numeric identifiers back to the CLI.

Keep legacy number fallback only for old fixtures, but prefer short_id/qualified refs in displays, dependency resolution, locks, lifecycle logging, and user-facing tool descriptions.

Validation: GOFLAGS=-buildvcs=false go test ./... && cd plugins/pi-tasks-kata && npm test -- --run && npm run typecheck && npm run build

Generated with Pi
Co-authored-by: Pi <pi@users.noreply.github.com>
@mariusvniekerk mariusvniekerk force-pushed the pi-tasks-kata-plugin branch from 86fe876 to 8015921 Compare May 17, 2026 18:02
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 17, 2026

roborev: Combined Review (8015921)

The PR is mostly clean, but there is one Medium concurrency/security issue to address before merge.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:149, plugins/pi-tasks-kata/src/kata.ts:151, plugins/pi-tasks-kata/src/kata.ts:289
    claimForExecution locks using the raw caller-supplied taskId, while the plugin accepts multiple refs for the same Kata task, such as short_id, qualified ref, or ULID. Concurrent TaskExecute calls using different aliases can create different lock names and bypass both the file lock and in-memory claimLocks, allowing multiple executions to pass claim checks before labels are applied.

    Impact: duplicate subagents can execute the same task concurrently, causing conflicting mutations and bypassing the intended single-executor lifecycle.

    Suggested fix: canonicalize the task before locking and lock on a stable server-derived identifier such as issue.uid or short_id. Re-read issue state after acquiring the canonical lock before assigning, labeling, and spawning. If Kata supports an atomic claim/compare-and-set operation, use that instead.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 21, 2026

roborev: Combined Review (8015921)

Medium issue found: alias-based task claiming can race and allow duplicate execution.

Medium

  • plugins/pi-tasks-kata/src/kata.ts:144claimForExecution locks using the caller-supplied task ref before resolving the issue. Because aliases can refer to the same Kata issue, concurrent TaskExecute calls with different refs can acquire different locks and both pass open/not-in-progress checks before mutation is visible.

    Fix: Resolve the issue first under a temporary validation lock, then lock on a canonical server-derived ref such as issue.short_id or issue.uid, re-read the issue, and only then assign/label/spawn. Add a test that races two aliases for the same returned issue.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Task execution accepts multiple Kata refs for the same issue, but claim locking previously used the caller-supplied ref directly. That left alias pairs such as a short id and qualified ref able to take separate in-process and durable locks before either mutation made the task visibly in progress.

Resolve the issue first, then claim under a stable server-derived ref and re-read inside that canonical lock before assigning, labeling, or spawning. This keeps the existing rollback behavior while closing the duplicate-execution race.

Validation: cd plugins/pi-tasks-kata && npm test && npm run typecheck && npm run build

Generated with Codex

Co-authored-by: Codex <codex@openai.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 26, 2026

roborev: Combined Review (2768f04)

High: plugins/pi-tasks-kata/src/kata.ts:120 calls kata close <ref> --reason done --json for TaskUpdate status=completed and completeExecution without the required close message and evidence, so normal done closes will be rejected by the CLI/daemon. This breaks completed updates and successful TaskExecute lifecycle closure. Supply a substantive --message and accepted --evidence, or route through an API that provides those fields.

Medium: plugins/pi-tasks-kata/src/kata.ts:129 invokes kata block ... for dependency updates, but the CLI has no block subcommand. Relationships are added via kata edit --blocks / --blocked-by, so TaskUpdate with addBlocks or addBlockedBy will fail at runtime. Replace these calls with kata edit <taskId> --blocks <target> --json and kata edit <taskId> --blocked-by <blocker> --json.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Kata close now requires done closes to carry substantive closure metadata, and relationship updates are edited through the issue mutation command rather than the retired block subcommand. The adapter needs to follow that CLI contract or TaskUpdate and TaskExecute lifecycle paths fail at runtime despite passing earlier unit-level assumptions.

Use the current close flags with task-specific messages and typed evidence, and route dependency additions through kata edit relationship flags so the plugin exercises the shipped command surface.

Validation: cd plugins/pi-tasks-kata && npm test && npm run typecheck && npm run build

Generated with Codex

Co-authored-by: Codex <codex@openai.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 26, 2026

roborev: Combined Review (ae109f0)

No Medium, High, or Critical issues found.

All reviewed agents reported the code as clean.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

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